On Wed, Jan 05, 2022 at 03:12:26PM -0700, Alex Williamson wrote: > On Wed, 5 Jan 2022 16:05:45 -0500 > "Michael S. Tsirkin" <[email protected]> wrote: > > > On Wed, Jan 05, 2022 at 12:56:42PM -0700, Alex Williamson wrote: > > > The VFIO_DEVICE_RESET ioctl might be backed by several different reset > > > methods, including a device specific reset (ie. custom reset code in > > > kernel), an ACPI reset (ie. custom reset code in firmware), FLR, PM, > > > and bus resets. This listing is also the default priority order used > > > by the kernel for trying reset methods. Traditionally we've had some > > > FUD regarding the PM reset as the extent of a "Soft Reset" is not well > > > defined in the PCI specification. Therefore we try to guess what type > > > of reset a device might use for the VFIO_DEVICE_RESET and insert a bus > > > reset via the vfio hot reset interface if we think it could be a PM > > > reset. > > > > > > This results in a couple odd tests for PM reset in our hot reset code, > > > as we assume if we don't detect has_pm_reset support that we can't > > > reset the device otherwise. Starting with kernel v5.15, the kernel > > > exposes a sysfs attribute for devices that can tell us the priority > > > order for device resets, so long term (not implemented here) we no > > > longer need to play this guessing game, and if permissions allow we > > > could manipulate the order ourselves so that we don't need to inject > > > our own hot reset. > > > > > > In the shorter term, implemented here, let's not assume we're out of > > > reset methods if we can't perform a hot reset and the device doesn't > > > support PM reset. We can use reset_works as the authority, which > > > allows us to generate more comprehensible error messages for the case > > > when it actually doesn't work. > > > > > > The impetus for this change is a result of commit d5daff7d3126 ("pcie: > > > implement slot power control for pcie root ports"), where powering off > > > a slot now results in a device reset. If the slot is powered off as a > > > result of qdev_unplug() via the device request event, that device > > > request is potentially the result of an unbind operation in the host. > > > That unbind operation holds the kernel device lock, which causes the > > > VFIO_DEVICE_RESET ioctl to fail (or in the case of some kernels, has > > > cleared the flag indicating support of a device reset function). We > > > can then end up with an SR-IOV VF device trying to trigger a hot reset, > > > which finds that it needs ownership of the PF group to perform such a > > > reset, resulting in confusing log messages. > > > > > > Ultimately the above commit still introduces a log message that we > > > didn't have prior on such an unplug, but it's not unjustified to > > > perform such a reset, though it might be considered unnecessary. > > > Arguably failure to reset the device should always generate some sort > > > of meaningful log message. > > > > > > Signed-off-by: Alex Williamson <[email protected]> > > > > Looks reasonable. Just an extra idea: do we want to maybe validate the > > return code from the ioctl? I assume it's something like EBUSY right? > > Ideally it'd be EAGAIN to denote the lock contention,
Or EBUSY? > but for some > reason there was a recent time when the kernel would clear the > pci_dev.reset_fn flag as part of pci_stop_dev() before unbinding the > driver from the device, in that case we get an ENOTTY. > > Hmm, I'm remembering now that an issue with this approach to log all > device reset failures is that we're going to get false positives every > time we reboot a VM where we need a bus reset for multiple devices. We > handle multiple devices via a reset handler but we'll still get a > redundant per device reset and we have no way to associate that per > device reset to a VM reset where the reset handler multi-device > mechanism may have been successful :-\ This would be very common with > desktop GPUs. I'll plug away at this some more. Thanks, > > Alex
