Re: [PATCH v2] vfio/pci: Fix vf_token mechanism when device-specific VF drivers are used

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Apr 12, 2022 at 12:25:44PM -0600, Alex Williamson wrote:
> On Mon, 11 Apr 2022 10:56:31 -0300
> Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> > @@ -1732,10 +1705,28 @@ static int vfio_pci_bus_notifier(struct notifier_block *nb,
> >  static int vfio_pci_vf_init(struct vfio_pci_core_device *vdev)
> >  {
> >  	struct pci_dev *pdev = vdev->pdev;
> > +	struct vfio_pci_core_device *cur;
> > +	struct pci_dev *physfn;
> >  	int ret;
> >  
> > -	if (!pdev->is_physfn)
> > +	if (!pdev->is_physfn) {
> > +		/*
> > +		 * If this VF was created by our vfio_pci_core_sriov_configure()
> > +		 * then we can find the PF vfio_pci_core_device now, and due to
> > +		 * the locking in pci_disable_sriov() it cannot change until
> > +		 * this VF device driver is removed.
> > +		 */
> > +		physfn = pci_physfn(vdev->pdev);
> > +		mutex_lock(&vfio_pci_sriov_pfs_mutex);
> > +		list_for_each_entry (cur, &vfio_pci_sriov_pfs, sriov_pfs_item) {
> > +			if (cur->pdev == physfn) {
> > +				vdev->sriov_pf_core_dev = cur;
> > +				break;
> > +			}
> > +		}
> > +		mutex_unlock(&vfio_pci_sriov_pfs_mutex);
> >  		return 0;
> > +	}
> >  
> >  	vdev->vf_token = kzalloc(sizeof(*vdev->vf_token), GFP_KERNEL);
> >  	if (!vdev->vf_token)
> 
> One more comment on final review; are we equating !is_physfn to
> is_virtfn above?  This branch was originally meant to kick out both VFs
> and non-SRIOV PFs.  Calling pci_physfn() on a !is_virtfn device will
> return itself, so we should never find a list match, but we also don't
> need to look for a match for !is_virtfn, so it's a bit confusing and
> slightly inefficient.  Should the new code be added in a separate
> is_virtfn branch above the existing !is_physfn test?  Thanks,

I started at it for a while and came the same conclusion, I
misunderstood that is_physfn is really trying to be
is_sriov_physfn.. So not a bug, but not really clear code.

I added this, I'll repost it.

diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 8bf0f18e668a32..3c6493957abe19 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -1709,7 +1709,7 @@ static int vfio_pci_vf_init(struct vfio_pci_core_device *vdev)
 	struct pci_dev *physfn;
 	int ret;
 
-	if (!pdev->is_physfn) {
+	if (pdev->is_virtfn) {
 		/*
 		 * If this VF was created by our vfio_pci_core_sriov_configure()
 		 * then we can find the PF vfio_pci_core_device now, and due to
@@ -1728,6 +1728,10 @@ static int vfio_pci_vf_init(struct vfio_pci_core_device *vdev)
 		return 0;
 	}
 
+	/* Not a SRIOV PF */
+	if (!pdev->is_physfn)
+		return 0;
+
 	vdev->vf_token = kzalloc(sizeof(*vdev->vf_token), GFP_KERNEL);
 	if (!vdev->vf_token)
 		return -ENOMEM;


Thanks,
Jason