Re: [PATCH v9] PCI: Add device-specific reset for Qualcomm devices

Hi Bjorn and Alex,

Bjorn's questions:

> I guess "bus reset" here refers to Secondary Bus Reset being asserted
> by the bridge upstream from these devices?  Seems a bit surprising if
> that doesn't work.  Or is it just that we can't use SBR because there
> are multiple devices below that bridge?

Yes, SBR. The devices I tested are alone on their bus (single device under
bridge), so it's a device-specific issue, not a topology problem. The
quirk_no_bus_reset patch addresses this for v7.2.

> I don't know enough about VFIO, but I sort of expected that VFIO would
> reset devices between reassignment regardless of how a VM terminates.
> I guess that's not true?

VFIO does attempt reset on every reassignment. Without a working reset method,
the attempt fails and the device remains in undefined state. With this quirk,
D3hot successfully resets the device allowing reassignment.

> Since the device claims to preserve internal state across D3hot->D0
> (and it sounds like at least BARs *are* preserved), is this a
> potential leak of state between VMs?  To play devil's advocate, how do
> we convince a customer that none of their data is ever leaked to a
> subsequent tenant using this device?

This is a valid concern. Testing shows device internals are reset despite
NoSoftRst+ (command register cleared, requires driver reinitialization),
though BARs are preserved. Given these devices have no other reset method,
this provides the only viable mechanism for VFIO reuse. We cannot improve
beyond what D3hot provides - the quirk works because despite advertising
NoSoftRst+, D3hot does clear sufficient internal state for clean
reinitialization.

> If we think this is a viable method, it seems like we should use
> pci_pm_reset(), which takes care of IOMMU and device readiness issues.
>
> We would have to change pci_pm_reset() to deal with the fact that
> PCI_PM_CTRL_NO_SOFT_RESET seems wrong on these devices.  Maybe we
> could cache PCI_PM_CTRL_NO_SOFT_RESET in pci_pm_init(), then override
> it with quirks for these devices?

I explored a similar idea in v2 (PCI_DEV_FLAGS_FORCE_PM_RESET to bypass
NoSoftRst+):
https://lore.kernel.org/linux-pci/[email protected]/
(Note: v2 used driver names ath11k/ath12k instead of device-specific names
WCN6855/WCN7850, which Jeff Johnson later commented on in v7 feedback.)

Alex provided guidance on both approaches and indicated device-specific reset
seemed more appropriate here:

"Device specific resets are made for this scenario. Look at
pci_dev_specific_reset() and pci_dev_reset_methods[]. The supporting
evidence that this performs a worthwhile reset is still a bit weak, but
heuristically it seems better than nothing, which is what we're left
with otherwise. Reset via D3hot for a device that does not expose
NoSoftRst- is not something we should enable or endorse for any common
use case."

The device-specific approach keeps this quirk isolated to proven device IDs.
But I can revisit the pm quirk approach if you both prefer it.

Alex's suggestion:

> It would be better to extract the core of pci_pm_reset() to a
> pci_do_pm_reset() function that's used both here and by the
> pci_pm_reset() function.

Good point about the code duplication. In v9 I kept it as a self-contained
quirk to avoid modifying pci_pm_reset() and touching core pci.c code, trying
to minimize the change footprint. But I agree extracting a helper function
would be cleaner.

Once we confirm the preferred approach (device-specific vs pm quirk per
Bjorn's question above), I'll send v10 with the appropriate implementation
including the helper function if we proceed with the device-specific approach.

Thanks

Best regards
Jose Ignacio