Re: [PATCH] vfio/pci: Separate SR-IOV VF dev_set

[Alex Williamson <alex.williamson@redhat.com>]

On Thu, 3 Jul 2025 10:23:50 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Thu, Jul 03, 2025 at 06:10:19AM +0000, Tian, Kevin wrote:
> > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > Sent: Thursday, July 3, 2025 1:56 AM
> > > 
> > > On Wed, Jul 02, 2025 at 11:50:32AM -0600, Alex Williamson wrote:  
> > > > I haven't tried it, but it may be possible to trigger a hot reset
> > > > on a user owned PF while there are open VFs.  If that is possible, I
> > > > wonder if it isn't just a userspace problem though, it doesn't seem
> > > > there's anything fundamentally wrong with it from a vfio perspective.
> > > > The vf-token already indicates at the kernel level that there is
> > > > collaboration between PF and VF userspace drivers.  
> > > 
> > > I think it will disable SRIOV and that will leave something of a
> > > mess. Arguably we should be blocking resets that disable SRIOV inside
> > > vfio?
> > >   
> > 
> > Is there any reset which doesn't disable SRIOV? According to PCIe
> > spec both conventional reset and FLR targeting a PF clears the
> > VF enable bit.  
> 
> This is my understanding, I think there might be a little hole here in
> the vfio SRIOV support?

I wrote a test case and we don't prevent a vfio-pci userspace driver
from resetting the PF while also having open a VF, but I'm also not
sure what problem that causes.

pci_restore_state() calls pci_restore_iov_state(), so VF Enable does get
cleared by the reset (we don't actively tear down SR-IOV before reset),
but it's restored.  VFs are not technically on a subordinate bus, so
none of those check prevent a bus reset.  I think this is why we have
the VF token, we cannot guarantee that a userspace owner of the PF
doesn't do something stupid while the VF is in use.

Also, PF->bus != VF->bus, so VFs don't get added to the PF dev_set.
The PF will do a hot reset with just the PF group fd and of course FLR
doesn't require proof of ownership of other devices.  Again, I don't
think giving each VF its own dev_set changes anything in this respect.

Should we do more here?  What problem are we solving?  Thanks,

Alex