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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130423125101.00005bb7@unknown>
Date:	Tue, 23 Apr 2013 12:51:01 -0700
From:	Greg Rose <gregory.v.rose@...el.com>
To:	Alexander Duyck <alexander.h.duyck@...el.com>
CC:	Bjorn Helgaas <bhelgaas@...gle.com>,
	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
	David Miller <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>, <gospo@...hat.com>,
	<sassmann@...hat.com>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>
Subject: Re: [net-next 08/14] pci: Add SRIOV helper function to determine if
 VFs are assigned to guest

On Mon, 22 Apr 2013 14:50:33 -0700
Alexander Duyck <alexander.h.duyck@...el.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@...el.com> wrote:
> >> On Sat, 2013-04-20 at 02:49 -0700, Jeff Kirsher wrote:
> >>> From: Alexander Duyck <alexander.h.duyck@...el.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@...el.com>
> >>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@...el.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.

- Greg

> 
> Thanks,
> 
> Alex
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html