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:	Tue, 23 Apr 2013 17:16:43 -0400
From:	Don Dutile <ddutile@...hat.com>
To:	Greg Rose <gregory.v.rose@...el.com>
CC:	Alexander Duyck <alexander.h.duyck@...el.com>,
	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>,
	Chris Wright <chrisw@...hat.com>
Subject: Re: [net-next 08/14] pci: Add SRIOV helper function to determine
 if VFs are assigned to guest

On 04/23/2013 03:51 PM, Greg Rose 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.
>
> - Greg
>
Added Chris to this thread since he has history here...

Q: Why not link-list the VF pci-dev's off the PF's pci-dev when they are created?
    (yes, voices in my head have asked me this question in the past! ;-) )
    -- that way, finding them is easy, and locked down when doing a get on the PF pci-dev.
    a backptr of the VF's pci-dev to it's PF pci-dev would be handy too!
    --> it'd also simplify that rmmod PF driver while VF drivers loaded (& assigned)
        case as well.


>>
>> 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 linux-pci" 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