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]

Date:	Wed, 24 Apr 2013 14:10:38 -0600
From:	Bjorn Helgaas <bhelgaas@...gle.com>
To:	Greg Rose <gregory.v.rose@...el.com>
Cc:	Alexander Duyck <alexander.h.duyck@...el.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 Tue, Apr 23, 2013 at 1:51 PM, Greg Rose <gregory.v.rose@...el.com> wrote:
> 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.

That's not a very convincing argument :)

And I don't like having to iterate through all PCI devs in the system
to figure this out.

But this is no worse in either respect than the code it replaces, and
it's certainly better to have it all in one place, so:

Acked-by: Bjorn Helgaas <bhelgaas@...gle.com>

If you have a chance, you could remove the "extern" from the
declaration in include/linux/pci.h.  There's a mix there right now,
and I have a patch in the queue that removes them all.
--
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