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

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

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 3463 bytes --]

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)
> +{
> +	struct pci_dev *vfdev;
> +	unsigned int vfs_assigned = 0;
> +	unsigned short dev_id;
> +
> +	/* only search if we are a PF */
> +	if (!dev->is_physfn)
> +		return -ENODEV;
> +
> +	/*
> +	 * determine the device ID for the VFs, the vendor ID will be the
> +	 * same as the PF so there is no need to check for that one
> +	 */
> +	pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_VF_DID, &dev_id);
> +
> +	/* loop through all the VFs to see if we own any that are assigned */
> +	vfdev = pci_get_device(dev->vendor, dev_id, NULL);
> +	while (vfdev) {
> +		/*
> +		 * It is considered assigned if it is a virtual function with
> +		 * our dev as the physical function and the assigned bit is set
> +		 */
> +		if (vfdev->is_virtfn && (vfdev->physfn == dev) &&
> +		    (vfdev->dev_flags & PCI_DEV_FLAGS_ASSIGNED))
> +			vfs_assigned++;
> +
> +		vfdev = pci_get_device(dev->vendor, dev_id, vfdev);
> +	}
> +
> +	return vfs_assigned;
> +}
> +EXPORT_SYMBOL_GPL(pci_vfs_assigned);
> +
> +/**
>   * pci_sriov_set_totalvfs -- reduce the TotalVFs available
>   * @dev: the PCI PF device
>   * @numvfs: number that should be used for TotalVFs supported
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 2461033a..03b4d3c 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1643,6 +1643,7 @@ extern int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
>  extern void pci_disable_sriov(struct pci_dev *dev);
>  extern irqreturn_t pci_sriov_migration(struct pci_dev *dev);
>  extern int pci_num_vf(struct pci_dev *dev);
> +extern int pci_vfs_assigned(struct pci_dev *dev);
>  extern int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
>  extern int pci_sriov_get_totalvfs(struct pci_dev *dev);
>  #else
> @@ -1661,6 +1662,10 @@ static inline int pci_num_vf(struct pci_dev *dev)
>  {
>  	return 0;
>  }
> +static inline int pci_vfs_assigned(struct pci_dev *dev)
> +{
> +	return 0;
> +}
>  static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
>  {
>  	return 0;



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Alexander Duyck]

On 04/20/2013 10:14 PM, Gavin Shan wrote:
> On Sat, Apr 20, 2013 at 08:31:28PM -0700, Jeff Kirsher 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)
>>> +{
>>> +	struct pci_dev *vfdev;
>>> +	unsigned int vfs_assigned = 0;
>>> +	unsigned short dev_id;
>>> +
>>> +	/* only search if we are a PF */
>>> +	if (!dev->is_physfn)
>>> +		return -ENODEV;
> I think it's more reasonable to return zero here?

I suppose that is true, it would be more in keeping with how pci_num_vf
works so I will make that change.

Thanks,

Alex

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

[Bjorn Helgaas]

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.

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

>> +{
>> +     struct pci_dev *vfdev;
>> +     unsigned int vfs_assigned = 0;
>> +     unsigned short dev_id;
>> +
>> +     /* only search if we are a PF */
>> +     if (!dev->is_physfn)
>> +             return -ENODEV;
>> +
>> +     /*
>> +      * determine the device ID for the VFs, the vendor ID will be the
>> +      * same as the PF so there is no need to check for that one
>> +      */
>> +     pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_VF_DID, &dev_id);
>> +
>> +     /* loop through all the VFs to see if we own any that are assigned */
>> +     vfdev = pci_get_device(dev->vendor, dev_id, NULL);
>> +     while (vfdev) {
>> +             /*
>> +              * It is considered assigned if it is a virtual function with
>> +              * our dev as the physical function and the assigned bit is set
>> +              */
>> +             if (vfdev->is_virtfn && (vfdev->physfn == dev) &&
>> +                 (vfdev->dev_flags & PCI_DEV_FLAGS_ASSIGNED))
>> +                     vfs_assigned++;
>> +
>> +             vfdev = pci_get_device(dev->vendor, dev_id, vfdev);
>> +     }
>> +
>> +     return vfs_assigned;
>> +}
>> +EXPORT_SYMBOL_GPL(pci_vfs_assigned);
>> +
>> +/**
>>   * pci_sriov_set_totalvfs -- reduce the TotalVFs available
>>   * @dev: the PCI PF device
>>   * @numvfs: number that should be used for TotalVFs supported
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 2461033a..03b4d3c 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -1643,6 +1643,7 @@ extern int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
>>  extern void pci_disable_sriov(struct pci_dev *dev);
>>  extern irqreturn_t pci_sriov_migration(struct pci_dev *dev);
>>  extern int pci_num_vf(struct pci_dev *dev);
>> +extern int pci_vfs_assigned(struct pci_dev *dev);
>>  extern int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
>>  extern int pci_sriov_get_totalvfs(struct pci_dev *dev);
>>  #else
>> @@ -1661,6 +1662,10 @@ static inline int pci_num_vf(struct pci_dev *dev)
>>  {
>>       return 0;
>>  }
>> +static inline int pci_vfs_assigned(struct pci_dev *dev)
>> +{
>> +     return 0;
>> +}
>>  static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
>>  {
>>       return 0;
>
>

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

[Alexander Duyck]

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.

Thanks,

Alex

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

[Greg Rose]

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.

- Greg

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

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

[Don Dutile]

On 04/23/2013 03:51 PM, Greg Rose 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.
>
> - 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@vger.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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Bjorn Helgaas]

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 :)

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@google.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.

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

[David Miller]

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

> 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@google.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.

Jeff please respin your pull request with Bjorn's ACK as well as
the "extern" removal he's asking for.

Thanks.

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

[Greg Rose]

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

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

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 756 bytes --]

On Wed, 2013-04-24 at 16:18 -0400, David Miller wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> Date: Wed, 24 Apr 2013 14:10:38 -0600
> 
> > 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@google.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.
> 
> Jeff please respin your pull request with Bjorn's ACK as well as
> the "extern" removal he's asking for.
> 
> Thanks.

Working on it, I will have a updated patch (and series) submitted later
tonight.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]