Re: [PATCH] vfio/pci: Generate more relevant log messages for reset failures

["Michael S. Tsirkin" <mst@redhat.com>]

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" <mst@redhat.com> 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 <alex.williamson@redhat.com>  
> > 
> > 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