Re: [net-next 08/14] pci: Add SRIOV helper function to determine if VFs are assigned to guest

[Greg Rose <gregory.v.rose@intel.com>]

On Wed, 24 Apr 2013 14:10:38 -0600
Bjorn Helgaas <bhelgaas@google.com> wrote:

> On Tue, Apr 23, 2013 at 1:51 PM, Greg Rose <gregory.v.rose@intel.com>
> wrote:
> > On Mon, 22 Apr 2013 14:50:33 -0700
> > Alexander Duyck <alexander.h.duyck@intel.com> wrote:
> >
> >> On 04/22/2013 01:09 PM, Bjorn Helgaas wrote:
> >> > On Sat, Apr 20, 2013 at 9:31 PM, Jeff Kirsher
> >> > <jeffrey.t.kirsher@intel.com> wrote:
> >> >> On Sat, 2013-04-20 at 02:49 -0700, Jeff Kirsher wrote:
> >> >>> From: Alexander Duyck <alexander.h.duyck@intel.com>
> >> >>>
> >> >>> This function is meant to add a helper function that will
> >> >>> determine if a PF has any VFs that are currently assigned to a
> >> >>> guest.  We currently have been implementing this function per
> >> >>> driver, and going forward I would like to avoid that by making
> >> >>> this function generic and using this helper.
> >> >>>
> >> >>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> >> >>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> >> >> Adding linux-pci mailing list and Bjorn to the CC.
> >> >>
> >> >> Bjorn- David Miller needs a signoff by PCI maintainer.
> >> >>
> >> >>> ---
> >> >>>  drivers/pci/iov.c   | 41
> >> >>> +++++++++++++++++++++++++++++++++++++++++ include/linux/pci.h |
> >> >>> 5 +++++ 2 files changed, 46 insertions(+)
> >> >>>
> >> >>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> >> >>> index ee599f2..fd99720 100644
> >> >>> --- a/drivers/pci/iov.c
> >> >>> +++ b/drivers/pci/iov.c
> >> >>> @@ -729,6 +729,47 @@ int pci_num_vf(struct pci_dev *dev)
> >> >>>  EXPORT_SYMBOL_GPL(pci_num_vf);
> >> >>>
> >> >>>  /**
> >> >>> + * pci_vfs_assigned - returns number of VFs are assigned to a
> >> >>> guest
> >> >>> + * @dev: the PCI device
> >> >>> + *
> >> >>> + * Returns number of VFs belonging to this device that are
> >> >>> assigned to a guest.
> >> >>> + * If device is not a physical function returns -ENODEV.
> >> >>> + */
> >> >>> +int pci_vfs_assigned(struct pci_dev *dev)
> >> > I guess the idea here is to replace be_find_vfs(),
> >> > igb_vfs_are_assigned(), ixgbe_vfs_are_assigned(), etc.  It does
> >> > seem good to reduce duplicated code.
> >>
> >> The general idea was just to remove duplicate code.  As is we have
> >> a couple more drivers on the way that would end up needing a
> >> similar function.
> >>
> >> > I'm trying to figure out why this is safe -- there's no explicit
> >> > synchronization between the iteration through PCI devices looking
> >> > for matching VFs and the device assignment/deassignment paths
> >> > that set or clear PCI_DEV_FLAGS_ASSIGNED, so on the face of it,
> >> > it looks like things could change between calling
> >> > pci_vfs_assigned() and using the result to make a decision.
> >> >
> >> > Most of the calls would be in .remove() functions, so maybe
> >> > there's some sort of synchronization in that path that  makes
> >> > this safe.
> >> >
> >> > Bjorn
> >>
> >> I'm assuming this will be used in regions that are somehow
> >> protected since the main spots where this might be called would be
> >> probe, remove, or when updating the number of VFs.  From what I
> >> can tell in the Xen case there is a driver stub that is loaded
> >> that sets the flag so that is covered by probe/remove.  I don't
> >> know about the KVM case.
> >
> > KVM should be fine.  Setting/clearing the flag occurs while a
> > device is being assigned to or removed from a VM - presumably
> > device assignment is already safe against race conditions.  I'd
> > find it hard to believe that it's not.  Code is
> > in ../virt/assigned_dev.c and ../virt/iommu.c.
> 
> That's not a very convincing argument :)

It's been a long time since I worked on that code.  Sorry, it's the
best I've got, my memory gets real hazy on code that I haven't touched
in a year or two.  But then that's part of my argument - if it were
subject to race conditions it seems like someone would have run into it
in the last couple of years.

But my apologies for the less than convincing argument!

:)

- Greg