Re: [PATCH 1/4] PCI: introduce VFs reference counter and simplify pci_vfs_assigned() implementation

[ethan zhao <ethan.zhao@oracle.com>]

Alex,
    Thanks for your reviewing, when I create the patch order, I thought 
about the question you concerned for
quit a while, make every patch be independent to each other as possible 
as I could, so we can do bisect when hit
problem.

    I manage to take more time to figure out better patch order.

    Thanks,
    Ethan

On 2014/7/11 9:48, Alex Williamson wrote:
> Since there's no 0th patch, I guess I'll comment here.  This series is
> not bisectable, patch 1 breaks the existing implementation.  I'd
> suggest:
>
> patch 1 - fix i40e
             i40e only could be fixed with new interface, so it couldn't 
be the first one.
> patch 2 - create assign/deassign that uses dev_flags
             This will be the first ?
> patch 3 - convert users to new interface
             Have to be the later step.
> patch 4 - convert interface to use atomic_t
             Could it be standalone step ?

  Let me think about it.

> IMHO, the assigned flag is a horrible hack and I don't know why drivers
> like xenback need to use it.  KVM needs to use it because it doesn't
> actually have a driver to bind to when a device is assigned, it's happy
> to assign devices without any driver.  Thanks,
>
> Alex
>
>
> On Fri, 2014-07-11 at 08:47 +0800, Ethan Zhao wrote:
>> Current implementation of helper function pci_vfs_assigned() is a little complex, to
>> get sum of VFs that assigned to VM, access low level configuration space register and
>> then loop in traversing device tree.
>>
>> This patch introduce an atomic reference counter for VFs that assigned to VM in struct
>> pci_sriov, and compose two more helper functions
>>
>> pci_sriov_assign_device(),
>> pci_sriov_deassign_device()
>>
>> to replace manipulation to device flag and the meanwhile increase and decease the counter.
>>
>> Passed building on 3.15.5
>>
>> Signed-off-by: Ethan Zhao <ethan.zhao@oracle.com>
>> ---
>>   drivers/pci/iov.c   |   65 ++++++++++++++++++++++++++++----------------------
>>   drivers/pci/pci.h   |    1 +
>>   include/linux/pci.h |    4 +++
>>   3 files changed, 41 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>> index de7a747..72e267f 100644
>> --- a/drivers/pci/iov.c
>> +++ b/drivers/pci/iov.c
>> @@ -382,6 +382,7 @@ found:
>>   	iov->nres = nres;
>>   	iov->ctrl = ctrl;
>>   	iov->total_VFs = total;
>> +	atomic_set(&iov->VFs_assigned_cnt, 0);
>>   	iov->offset = offset;
>>   	iov->stride = stride;
>>   	iov->pgsz = pgsz;
>> @@ -603,45 +604,51 @@ 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
>> + * pci_vfs_assigned - returns number of VFs are assigned to VM
>> + * @dev: the physical PCI device that contains the VFs.
>>    *
>> - * Returns number of VFs belonging to this device that are assigned to a guest.
>> + * Returns number of VFs belonging to this device that are assigned to VM.
>>    * If device is not a physical function returns 0.
>>    */
>>   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 0;
>> +	if (dev->sriov)
>> +		return atomic_read(&dev->sriov->VFs_assigned_cnt);
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(pci_vfs_assigned);
>>   
>> -	/*
>> -	 * 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);
>> -	}
>> +/**
>> + * pci_sriov_assign_device - assign device to VM
>> + * @pdev: the device to be assigned.
>> + */
>> +void pci_sriov_assign_device(struct pci_dev *pdev)
>> +{
>> +	pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
>> +	if (pdev->is_virtfn && !pdev->is_physfn)
>> +		if (pdev->physfn)
>> +			if (pdev->physfn->sriov)
>> +				atomic_inc(&pdev->physfn->sriov->
>> +						VFs_assigned_cnt);
>> +}
>> +EXPORT_SYMBOL_GPL(pci_sriov_assign_device);
>>   
>> -	return vfs_assigned;
>> +/**
>> + * pci_sriov_deassign_device - deasign device from VM
>> + * @pdev: the device to be deassigned.
>> + */
>> +void pci_sriov_deassign_device(struct pci_dev *pdev)
>> +{
>> +	pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
>> +	if (pdev->is_virtfn && !pdev->is_physfn)
>> +		if (pdev->physfn)
>> +			if (pdev->physfn->sriov)
>> +				atomic_dec(&pdev->physfn->sriov->
>> +						VFs_assigned_cnt);
>>   }
>> -EXPORT_SYMBOL_GPL(pci_vfs_assigned);
>> +EXPORT_SYMBOL_GPL(pci_sriov_deassign_device);
>>   
>>   /**
>>    * pci_sriov_set_totalvfs -- reduce the TotalVFs available
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 6bd0822..d17bda2 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -235,6 +235,7 @@ struct pci_sriov {
>>   	u32 pgsz;		/* page size for BAR alignment */
>>   	u8 link;		/* Function Dependency Link */
>>   	u16 driver_max_VFs;	/* max num VFs driver supports */
>> +	atomic_t VFs_assigned_cnt; /* counter of VFs assigned to VM */
>>   	struct pci_dev *dev;	/* lowest numbered PF */
>>   	struct pci_dev *self;	/* this PF */
>>   	struct mutex lock;	/* lock for VF bus */
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index aab57b4..5cf6833 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -1601,6 +1601,8 @@ int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
>>   void pci_disable_sriov(struct pci_dev *dev);
>>   int pci_num_vf(struct pci_dev *dev);
>>   int pci_vfs_assigned(struct pci_dev *dev);
>> +void pci_sriov_assign_device(struct pci_dev *dev);
>> +void pci_sriov_deassign_device(struct pci_dev *dev);
>>   int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
>>   int pci_sriov_get_totalvfs(struct pci_dev *dev);
>>   #else
>> @@ -1610,6 +1612,8 @@ static inline void pci_disable_sriov(struct pci_dev *dev) { }
>>   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 void pci_sriov_assign_device(struct pci_dev *dev) { }
>> +static inline void pci_sriov_deassign_device(struct pci_dev *dev) { }
>>   static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
>>   { return 0; }
>>   static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
>
>