[PATCH 1/2 V3] PCI: introduce device assignment interface and refactory related code

[PATCH 1/2 V3] PCI: introduce device assignment interface and refactory related code

[Ethan Zhao]

This patch introduces two new device assignment functions

pci_iov_assign_device(),
pci_iov_deassign_device()

along with the existed one

pci_vfs_assigned()

They construct the VFs assignment management interface, used to assign/
deassign device to VM and query the VFs reference counter. instead of
direct manipulation of device flag.

This patch refashioned the related code and make them atomic.

v3: change the naming of device assignment helpers, because they work
for all kind of PCI device, not only SR-IOV (david.vrabel@citrix.com)

v2: reorder the patchset and make it bisectable and atomic, steps clear
between interface defination and implemenation according to the
suggestion from alex.williamson@redhat.com

Signed-off-by: Ethan Zhao <ethan.zhao@oracle.com>
---
 drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c |   17 ++---------------
 drivers/pci/iov.c                                  |   20 ++++++++++++++++++++
 drivers/xen/xen-pciback/pci_stub.c                 |    4 ++--
 include/linux/pci.h                                |    4 ++++
 virt/kvm/assigned-dev.c                            |    2 +-
 virt/kvm/iommu.c                                   |    4 ++--
 6 files changed, 31 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
index 02c11a7..781040e 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
@@ -693,22 +693,9 @@ complete_reset:
 static bool i40e_vfs_are_assigned(struct i40e_pf *pf)
 {
 	struct pci_dev *pdev = pf->pdev;
-	struct pci_dev *vfdev;
-
-	/* loop through all the VFs to see if we own any that are assigned */
-	vfdev = pci_get_device(PCI_VENDOR_ID_INTEL, I40E_DEV_ID_VF , NULL);
-	while (vfdev) {
-		/* if we don't own it we don't care */
-		if (vfdev->is_virtfn && pci_physfn(vfdev) == pdev) {
-			/* if it is assigned we cannot release it */
-			if (vfdev->dev_flags & PCI_DEV_FLAGS_ASSIGNED)
-				return true;
-		}
 
-		vfdev = pci_get_device(PCI_VENDOR_ID_INTEL,
-				       I40E_DEV_ID_VF,
-				       vfdev);
-	}
+	if (pci_vfs_assigned(pdev))
+		return true;
 
 	return false;
 }
diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index de7a747..090f827 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -644,6 +644,26 @@ int pci_vfs_assigned(struct pci_dev *dev)
 EXPORT_SYMBOL_GPL(pci_vfs_assigned);
 
 /**
+ * pci_iov_assign_device - assign device to VM
+ * @pdev: the device to be assigned.
+ */
+void pci_iov_assign_device(struct pci_dev *pdev)
+{
+	pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
+}
+EXPORT_SYMBOL_GPL(pci_iov_assign_device);
+
+/**
+ * pci_iov_deassign_device - deasign device from VM
+ * @pdev: the device to be deassigned.
+ */
+void pci_iov_deassign_device(struct pci_dev *pdev)
+{
+	pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
+}
+EXPORT_SYMBOL_GPL(pci_iov_deassign_device);
+
+/**
  * 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/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index 62fcd48..27e00d1 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -133,7 +133,7 @@ static void pcistub_device_release(struct kref *kref)
 	xen_pcibk_config_free_dyn_fields(dev);
 	xen_pcibk_config_free_dev(dev);
 
-	dev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
+	pci_iov_deassign_device(dev);
 	pci_dev_put(dev);
 
 	kfree(psdev);
@@ -404,7 +404,7 @@ static int pcistub_init_device(struct pci_dev *dev)
 	dev_dbg(&dev->dev, "reset device\n");
 	xen_pcibk_reset_device(dev);
 
-	dev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
+	pci_iov_assign_device(dev);
 	return 0;
 
 config_release:
diff --git a/include/linux/pci.h b/include/linux/pci.h
index aab57b4..5ece6d6 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1603,6 +1603,8 @@ int pci_num_vf(struct pci_dev *dev);
 int pci_vfs_assigned(struct pci_dev *dev);
 int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
 int pci_sriov_get_totalvfs(struct pci_dev *dev);
+void pci_iov_assign_device(struct pci_dev *dev);
+void pci_iov_deassign_device(struct pci_dev *dev);
 #else
 static inline int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
 { return -ENODEV; }
@@ -1614,6 +1616,8 @@ 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)
 { return 0; }
+static inline void pci_iov_assign_device(struct pci_dev *dev) { }
+static inline void pci_iov_deassign_device(struct pci_dev *dev) { }
 #endif
 
 #if defined(CONFIG_HOTPLUG_PCI) || defined(CONFIG_HOTPLUG_PCI_MODULE)
diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
index bf06577..7fac58d 100644
--- a/virt/kvm/assigned-dev.c
+++ b/virt/kvm/assigned-dev.c
@@ -302,7 +302,7 @@ static void kvm_free_assigned_device(struct kvm *kvm,
 	else
 		pci_restore_state(assigned_dev->dev);
 
-	assigned_dev->dev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
+	pci_iov_deassign_device(assigned_dev->dev);
 
 	pci_release_regions(assigned_dev->dev);
 	pci_disable_device(assigned_dev->dev);
diff --git a/virt/kvm/iommu.c b/virt/kvm/iommu.c
index 0df7d4b..641ee73 100644
--- a/virt/kvm/iommu.c
+++ b/virt/kvm/iommu.c
@@ -194,7 +194,7 @@ int kvm_assign_device(struct kvm *kvm,
 			goto out_unmap;
 	}
 
-	pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
+	pci_iov_assign_device(pdev);
 
 	dev_info(&pdev->dev, "kvm assign device\n");
 
@@ -220,7 +220,7 @@ int kvm_deassign_device(struct kvm *kvm,
 
 	iommu_detach_device(domain, &pdev->dev);
 
-	pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
+	pci_iov_deassign_device(pdev);
 
 	dev_info(&pdev->dev, "kvm deassign device\n");
 
-- 
1.7.1

[PATCH 2/2 V3] PCI: implement VFs assignment reference counter

[Ethan Zhao]

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 introduces an atomic reference counter for VFs those
were assigned to VM in struct pci_sriov. and simplify the code in
pci_vfs_assigned().

v3: change the naming of device assignment helpers, because they work
for all kind of PCI device, not only SR-IOV (david.vrabel@citrix.com)

v2: reorder the patchset according to the suggestion from
alex.williamson@redhat.com

Signed-off-by: Ethan Zhao <ethan.zhao@oracle.com>
---
 drivers/pci/iov.c |   42 ++++++++++++++++--------------------------
 drivers/pci/pci.h |    1 +
 2 files changed, 17 insertions(+), 26 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 090f827..10efe43 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,43 +604,22 @@ 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.
  * 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;
 
-	/*
-	 * 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;
+	if (dev->sriov)
+		return atomic_read(&dev->sriov->VFs_assigned_cnt);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(pci_vfs_assigned);
 
@@ -650,6 +630,11 @@ EXPORT_SYMBOL_GPL(pci_vfs_assigned);
 void pci_iov_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_iov_assign_device);
 
@@ -660,6 +645,11 @@ EXPORT_SYMBOL_GPL(pci_iov_assign_device);
 void pci_iov_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_iov_deassign_device);
 
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 */
-- 
1.7.1

Re: [PATCH 2/2 V3] PCI: implement VFs assignment reference counter

[Varka Bhadram]

On 07/11/2014 06:00 PM, 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.

(...)

> @@ -650,6 +630,11 @@ EXPORT_SYMBOL_GPL(pci_vfs_assigned);
>   void pci_iov_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)

Why don't we make last two 'if' conditions into single 'if'

if (pdev->physfn && pdev->physfn->sriov)

> +				atomic_inc(&pdev->physfn->sriov->
> +					VFs_assigned_cnt);
>   }
>   EXPORT_SYMBOL_GPL(pci_iov_assign_device);
>   
> @@ -660,6 +645,11 @@ EXPORT_SYMBOL_GPL(pci_iov_assign_device);
>   void pci_iov_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)

same...

> +				atomic_dec(&pdev->physfn->sriov->
> +					VFs_assigned_cnt);
>   }
>   EXPORT_SYMBOL_GPL(pci_iov_deassign_device);
>   
> 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 */

-- 
Regards,
Varka Bhadram.

Re: [PATCH 1/2 V3] PCI: introduce device assignment interface and refactory related code

[Alex Williamson]

On Fri, 2014-07-11 at 20:30 +0800, Ethan Zhao wrote:
> This patch introduces two new device assignment functions
> 
> pci_iov_assign_device(),
> pci_iov_deassign_device()
> 
> along with the existed one
> 
> pci_vfs_assigned()
> 
> They construct the VFs assignment management interface, used to assign/
> deassign device to VM and query the VFs reference counter. instead of
> direct manipulation of device flag.
> 
> This patch refashioned the related code and make them atomic.
> 
> v3: change the naming of device assignment helpers, because they work
> for all kind of PCI device, not only SR-IOV (david.vrabel@citrix.com)
> 
> v2: reorder the patchset and make it bisectable and atomic, steps clear
> between interface defination and implemenation according to the
> suggestion from alex.williamson@redhat.com
> 
> Signed-off-by: Ethan Zhao <ethan.zhao@oracle.com>
> ---

- Use a cover patch to describe the series

- Version information goes here, below the "---", not above it

- I stand by the patch breakdown I suggested originally, there are too
many logical changes here in patch 1.  There are easily 3 separate
patches here.

- Renaming s/sriov/iov/ doesn't address the problem David raised.  The
name is still synonymous with SR-IOV and it's defined on
drivers/pci/iov.c, which is only built with CONFIG_PCI_IOV.

- PCI_DEV_FLAGS_ASSIGNED should be unused after this series, why is it
not removed?

>  drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c |   17 ++---------------
>  drivers/pci/iov.c                                  |   20 ++++++++++++++++++++
>  drivers/xen/xen-pciback/pci_stub.c                 |    4 ++--
>  include/linux/pci.h                                |    4 ++++
>  virt/kvm/assigned-dev.c                            |    2 +-
>  virt/kvm/iommu.c                                   |    4 ++--

- This patch touch 4 separate logical code areas, who do you expect to
ack and commit it?  Split it up.

>  6 files changed, 31 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> index 02c11a7..781040e 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> @@ -693,22 +693,9 @@ complete_reset:
>  static bool i40e_vfs_are_assigned(struct i40e_pf *pf)
>  {
>  	struct pci_dev *pdev = pf->pdev;
> -	struct pci_dev *vfdev;
> -
> -	/* loop through all the VFs to see if we own any that are assigned */
> -	vfdev = pci_get_device(PCI_VENDOR_ID_INTEL, I40E_DEV_ID_VF , NULL);
> -	while (vfdev) {
> -		/* if we don't own it we don't care */
> -		if (vfdev->is_virtfn && pci_physfn(vfdev) == pdev) {
> -			/* if it is assigned we cannot release it */
> -			if (vfdev->dev_flags & PCI_DEV_FLAGS_ASSIGNED)
> -				return true;
> -		}
>  
> -		vfdev = pci_get_device(PCI_VENDOR_ID_INTEL,
> -				       I40E_DEV_ID_VF,
> -				       vfdev);
> -	}
> +	if (pci_vfs_assigned(pdev))
> +		return true;
>  
>  	return false;
>  }
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index de7a747..090f827 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -644,6 +644,26 @@ int pci_vfs_assigned(struct pci_dev *dev)
>  EXPORT_SYMBOL_GPL(pci_vfs_assigned);
>  
>  /**
> + * pci_iov_assign_device - assign device to VM
> + * @pdev: the device to be assigned.
> + */
> +void pci_iov_assign_device(struct pci_dev *pdev)
> +{
> +	pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
> +}
> +EXPORT_SYMBOL_GPL(pci_iov_assign_device);
> +
> +/**
> + * pci_iov_deassign_device - deasign device from VM
> + * @pdev: the device to be deassigned.
> + */
> +void pci_iov_deassign_device(struct pci_dev *pdev)
> +{
> +	pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
> +}
> +EXPORT_SYMBOL_GPL(pci_iov_deassign_device);
> +
> +/**
>   * 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/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
> index 62fcd48..27e00d1 100644
> --- a/drivers/xen/xen-pciback/pci_stub.c
> +++ b/drivers/xen/xen-pciback/pci_stub.c
> @@ -133,7 +133,7 @@ static void pcistub_device_release(struct kref *kref)
>  	xen_pcibk_config_free_dyn_fields(dev);
>  	xen_pcibk_config_free_dev(dev);
>  
> -	dev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
> +	pci_iov_deassign_device(dev);
>  	pci_dev_put(dev);
>  
>  	kfree(psdev);
> @@ -404,7 +404,7 @@ static int pcistub_init_device(struct pci_dev *dev)
>  	dev_dbg(&dev->dev, "reset device\n");
>  	xen_pcibk_reset_device(dev);
>  
> -	dev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
> +	pci_iov_assign_device(dev);
>  	return 0;
>  
>  config_release:
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index aab57b4..5ece6d6 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1603,6 +1603,8 @@ int pci_num_vf(struct pci_dev *dev);
>  int pci_vfs_assigned(struct pci_dev *dev);
>  int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
>  int pci_sriov_get_totalvfs(struct pci_dev *dev);
> +void pci_iov_assign_device(struct pci_dev *dev);
> +void pci_iov_deassign_device(struct pci_dev *dev);
>  #else
>  static inline int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
>  { return -ENODEV; }
> @@ -1614,6 +1616,8 @@ 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)
>  { return 0; }
> +static inline void pci_iov_assign_device(struct pci_dev *dev) { }
> +static inline void pci_iov_deassign_device(struct pci_dev *dev) { }
>  #endif
>  
>  #if defined(CONFIG_HOTPLUG_PCI) || defined(CONFIG_HOTPLUG_PCI_MODULE)
> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> index bf06577..7fac58d 100644
> --- a/virt/kvm/assigned-dev.c
> +++ b/virt/kvm/assigned-dev.c
> @@ -302,7 +302,7 @@ static void kvm_free_assigned_device(struct kvm *kvm,
>  	else
>  		pci_restore_state(assigned_dev->dev);
>  
> -	assigned_dev->dev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
> +	pci_iov_deassign_device(assigned_dev->dev);
>  
>  	pci_release_regions(assigned_dev->dev);
>  	pci_disable_device(assigned_dev->dev);
> diff --git a/virt/kvm/iommu.c b/virt/kvm/iommu.c
> index 0df7d4b..641ee73 100644
> --- a/virt/kvm/iommu.c
> +++ b/virt/kvm/iommu.c
> @@ -194,7 +194,7 @@ int kvm_assign_device(struct kvm *kvm,
>  			goto out_unmap;
>  	}
>  
> -	pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
> +	pci_iov_assign_device(pdev);
>  
>  	dev_info(&pdev->dev, "kvm assign device\n");
>  
> @@ -220,7 +220,7 @@ int kvm_deassign_device(struct kvm *kvm,
>  
>  	iommu_detach_device(domain, &pdev->dev);
>  
> -	pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
> +	pci_iov_deassign_device(pdev);
>  
>  	dev_info(&pdev->dev, "kvm deassign device\n");
>  

Re: [PATCH 1/2 V3] PCI: introduce device assignment interface and refactory related code

[Alex Williamson]

On Fri, 2014-07-11 at 07:13 -0600, Alex Williamson wrote:
> On Fri, 2014-07-11 at 20:30 +0800, Ethan Zhao wrote:
> > This patch introduces two new device assignment functions
> > 
> > pci_iov_assign_device(),
> > pci_iov_deassign_device()
> > 
> > along with the existed one
> > 
> > pci_vfs_assigned()
> > 
> > They construct the VFs assignment management interface, used to assign/
> > deassign device to VM and query the VFs reference counter. instead of
> > direct manipulation of device flag.
> > 
> > This patch refashioned the related code and make them atomic.
> > 
> > v3: change the naming of device assignment helpers, because they work
> > for all kind of PCI device, not only SR-IOV (david.vrabel@citrix.com)
> > 
> > v2: reorder the patchset and make it bisectable and atomic, steps clear
> > between interface defination and implemenation according to the
> > suggestion from alex.williamson@redhat.com
> > 
> > Signed-off-by: Ethan Zhao <ethan.zhao@oracle.com>
> > ---
> 
> - Use a cover patch to describe the series
> 
> - Version information goes here, below the "---", not above it
> 
> - I stand by the patch breakdown I suggested originally, there are too
> many logical changes here in patch 1.  There are easily 3 separate
> patches here.
> 
> - Renaming s/sriov/iov/ doesn't address the problem David raised.  The
> name is still synonymous with SR-IOV and it's defined on
> drivers/pci/iov.c, which is only built with CONFIG_PCI_IOV.
> 
> - PCI_DEV_FLAGS_ASSIGNED should be unused after this series, why is it
> not removed?

I guess it's still used, which is even worse because now we have
separate data elements, one tracking how many VFs are assigned from a PF
and another tracking each device that's assigned.  What are we actually
gaining or fixing by doing this?

> >  drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c |   17 ++---------------
> >  drivers/pci/iov.c                                  |   20 ++++++++++++++++++++
> >  drivers/xen/xen-pciback/pci_stub.c                 |    4 ++--
> >  include/linux/pci.h                                |    4 ++++
> >  virt/kvm/assigned-dev.c                            |    2 +-
> >  virt/kvm/iommu.c                                   |    4 ++--
> 
> - This patch touch 4 separate logical code areas, who do you expect to
> ack and commit it?  Split it up.
> 
> >  6 files changed, 31 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> > index 02c11a7..781040e 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> > @@ -693,22 +693,9 @@ complete_reset:
> >  static bool i40e_vfs_are_assigned(struct i40e_pf *pf)
> >  {
> >  	struct pci_dev *pdev = pf->pdev;
> > -	struct pci_dev *vfdev;
> > -
> > -	/* loop through all the VFs to see if we own any that are assigned */
> > -	vfdev = pci_get_device(PCI_VENDOR_ID_INTEL, I40E_DEV_ID_VF , NULL);
> > -	while (vfdev) {
> > -		/* if we don't own it we don't care */
> > -		if (vfdev->is_virtfn && pci_physfn(vfdev) == pdev) {
> > -			/* if it is assigned we cannot release it */
> > -			if (vfdev->dev_flags & PCI_DEV_FLAGS_ASSIGNED)
> > -				return true;
> > -		}
> >  
> > -		vfdev = pci_get_device(PCI_VENDOR_ID_INTEL,
> > -				       I40E_DEV_ID_VF,
> > -				       vfdev);
> > -	}
> > +	if (pci_vfs_assigned(pdev))
> > +		return true;
> >  
> >  	return false;
> >  }
> > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> > index de7a747..090f827 100644
> > --- a/drivers/pci/iov.c
> > +++ b/drivers/pci/iov.c
> > @@ -644,6 +644,26 @@ int pci_vfs_assigned(struct pci_dev *dev)
> >  EXPORT_SYMBOL_GPL(pci_vfs_assigned);
> >  
> >  /**
> > + * pci_iov_assign_device - assign device to VM
> > + * @pdev: the device to be assigned.
> > + */
> > +void pci_iov_assign_device(struct pci_dev *pdev)
> > +{
> > +	pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_iov_assign_device);
> > +
> > +/**
> > + * pci_iov_deassign_device - deasign device from VM
> > + * @pdev: the device to be deassigned.
> > + */
> > +void pci_iov_deassign_device(struct pci_dev *pdev)
> > +{
> > +	pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_iov_deassign_device);
> > +
> > +/**
> >   * 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/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
> > index 62fcd48..27e00d1 100644
> > --- a/drivers/xen/xen-pciback/pci_stub.c
> > +++ b/drivers/xen/xen-pciback/pci_stub.c
> > @@ -133,7 +133,7 @@ static void pcistub_device_release(struct kref *kref)
> >  	xen_pcibk_config_free_dyn_fields(dev);
> >  	xen_pcibk_config_free_dev(dev);
> >  
> > -	dev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
> > +	pci_iov_deassign_device(dev);
> >  	pci_dev_put(dev);
> >  
> >  	kfree(psdev);
> > @@ -404,7 +404,7 @@ static int pcistub_init_device(struct pci_dev *dev)
> >  	dev_dbg(&dev->dev, "reset device\n");
> >  	xen_pcibk_reset_device(dev);
> >  
> > -	dev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
> > +	pci_iov_assign_device(dev);
> >  	return 0;
> >  
> >  config_release:
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index aab57b4..5ece6d6 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -1603,6 +1603,8 @@ int pci_num_vf(struct pci_dev *dev);
> >  int pci_vfs_assigned(struct pci_dev *dev);
> >  int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
> >  int pci_sriov_get_totalvfs(struct pci_dev *dev);
> > +void pci_iov_assign_device(struct pci_dev *dev);
> > +void pci_iov_deassign_device(struct pci_dev *dev);
> >  #else
> >  static inline int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
> >  { return -ENODEV; }
> > @@ -1614,6 +1616,8 @@ 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)
> >  { return 0; }
> > +static inline void pci_iov_assign_device(struct pci_dev *dev) { }
> > +static inline void pci_iov_deassign_device(struct pci_dev *dev) { }
> >  #endif
> >  
> >  #if defined(CONFIG_HOTPLUG_PCI) || defined(CONFIG_HOTPLUG_PCI_MODULE)
> > diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> > index bf06577..7fac58d 100644
> > --- a/virt/kvm/assigned-dev.c
> > +++ b/virt/kvm/assigned-dev.c
> > @@ -302,7 +302,7 @@ static void kvm_free_assigned_device(struct kvm *kvm,
> >  	else
> >  		pci_restore_state(assigned_dev->dev);
> >  
> > -	assigned_dev->dev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
> > +	pci_iov_deassign_device(assigned_dev->dev);
> >  
> >  	pci_release_regions(assigned_dev->dev);
> >  	pci_disable_device(assigned_dev->dev);
> > diff --git a/virt/kvm/iommu.c b/virt/kvm/iommu.c
> > index 0df7d4b..641ee73 100644
> > --- a/virt/kvm/iommu.c
> > +++ b/virt/kvm/iommu.c
> > @@ -194,7 +194,7 @@ int kvm_assign_device(struct kvm *kvm,
> >  			goto out_unmap;
> >  	}
> >  
> > -	pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
> > +	pci_iov_assign_device(pdev);
> >  
> >  	dev_info(&pdev->dev, "kvm assign device\n");
> >  
> > @@ -220,7 +220,7 @@ int kvm_deassign_device(struct kvm *kvm,
> >  
> >  	iommu_detach_device(domain, &pdev->dev);
> >  
> > -	pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
> > +	pci_iov_deassign_device(pdev);
> >  
> >  	dev_info(&pdev->dev, "kvm deassign device\n");
> >  
> 
> 

Re: [PATCH 1/2 V3] PCI: introduce device assignment interface and refactory related code

[Alexander Duyck]

On 07/11/2014 05:30 AM, Ethan Zhao wrote:
> This patch introduces two new device assignment functions
> 
> pci_iov_assign_device(),
> pci_iov_deassign_device()
> 
> along with the existed one
> 
> pci_vfs_assigned()
> 
> They construct the VFs assignment management interface, used to assign/
> deassign device to VM and query the VFs reference counter. instead of
> direct manipulation of device flag.
> 
> This patch refashioned the related code and make them atomic.
> 
> v3: change the naming of device assignment helpers, because they work
> for all kind of PCI device, not only SR-IOV (david.vrabel@citrix.com)
> 
> v2: reorder the patchset and make it bisectable and atomic, steps clear
> between interface defination and implemenation according to the
> suggestion from alex.williamson@redhat.com
> 
> Signed-off-by: Ethan Zhao <ethan.zhao@oracle.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c |   17 ++---------------
>  drivers/pci/iov.c                                  |   20 ++++++++++++++++++++
>  drivers/xen/xen-pciback/pci_stub.c                 |    4 ++--
>  include/linux/pci.h                                |    4 ++++
>  virt/kvm/assigned-dev.c                            |    2 +-
>  virt/kvm/iommu.c                                   |    4 ++--
>  6 files changed, 31 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> index 02c11a7..781040e 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> @@ -693,22 +693,9 @@ complete_reset:
>  static bool i40e_vfs_are_assigned(struct i40e_pf *pf)
>  {
>  	struct pci_dev *pdev = pf->pdev;
> -	struct pci_dev *vfdev;
> -
> -	/* loop through all the VFs to see if we own any that are assigned */
> -	vfdev = pci_get_device(PCI_VENDOR_ID_INTEL, I40E_DEV_ID_VF , NULL);
> -	while (vfdev) {
> -		/* if we don't own it we don't care */
> -		if (vfdev->is_virtfn && pci_physfn(vfdev) == pdev) {
> -			/* if it is assigned we cannot release it */
> -			if (vfdev->dev_flags & PCI_DEV_FLAGS_ASSIGNED)
> -				return true;
> -		}
>  
> -		vfdev = pci_get_device(PCI_VENDOR_ID_INTEL,
> -				       I40E_DEV_ID_VF,
> -				       vfdev);
> -	}
> +	if (pci_vfs_assigned(pdev))
> +		return true;
>  
>  	return false;
>  }

This portion for i40e should be in one patch by itself.  It shouldn't be
included in the bits below.  Normally this would go through netdev.  The
rest of this below would go through linux-pci.

> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index de7a747..090f827 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -644,6 +644,26 @@ int pci_vfs_assigned(struct pci_dev *dev)
>  EXPORT_SYMBOL_GPL(pci_vfs_assigned);
>  
>  /**
> + * pci_iov_assign_device - assign device to VM
> + * @pdev: the device to be assigned.
> + */
> +void pci_iov_assign_device(struct pci_dev *pdev)
> +{
> +	pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
> +}
> +EXPORT_SYMBOL_GPL(pci_iov_assign_device);
> +
> +/**
> + * pci_iov_deassign_device - deasign device from VM
> + * @pdev: the device to be deassigned.
> + */
> +void pci_iov_deassign_device(struct pci_dev *pdev)
> +{
> +	pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
> +}
> +EXPORT_SYMBOL_GPL(pci_iov_deassign_device);
> +
> +/**
>   * pci_sriov_set_totalvfs -- reduce the TotalVFs available
>   * @dev: the PCI PF device
>   * @numvfs: number that should be used for TotalVFs supported

The two functions above don't have anything to do with IOV.  You can
direct assign a device that doesn't even support SR-IOV or MR-IOV.  You
might be better off defining this as something like
"pci_set_flag_assigned/pci_clear_flag_assigned".  I would likely also
make them inline and possibly move them to pci.h since it would likely
result in less actual code after you consider the overhead to push
everything on the stack prior to making the call.

> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
> index 62fcd48..27e00d1 100644
> --- a/drivers/xen/xen-pciback/pci_stub.c
> +++ b/drivers/xen/xen-pciback/pci_stub.c
> @@ -133,7 +133,7 @@ static void pcistub_device_release(struct kref *kref)
>  	xen_pcibk_config_free_dyn_fields(dev);
>  	xen_pcibk_config_free_dev(dev);
>  
> -	dev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
> +	pci_iov_deassign_device(dev);
>  	pci_dev_put(dev);
>  
>  	kfree(psdev);
> @@ -404,7 +404,7 @@ static int pcistub_init_device(struct pci_dev *dev)
>  	dev_dbg(&dev->dev, "reset device\n");
>  	xen_pcibk_reset_device(dev);
>  
> -	dev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
> +	pci_iov_assign_device(dev);
>  	return 0;
>  
>  config_release:
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index aab57b4..5ece6d6 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1603,6 +1603,8 @@ int pci_num_vf(struct pci_dev *dev);
>  int pci_vfs_assigned(struct pci_dev *dev);
>  int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
>  int pci_sriov_get_totalvfs(struct pci_dev *dev);
> +void pci_iov_assign_device(struct pci_dev *dev);
> +void pci_iov_deassign_device(struct pci_dev *dev);
>  #else
>  static inline int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
>  { return -ENODEV; }
> @@ -1614,6 +1616,8 @@ 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)
>  { return 0; }
> +static inline void pci_iov_assign_device(struct pci_dev *dev) { }
> +static inline void pci_iov_deassign_device(struct pci_dev *dev) { }
>  #endif
>  
>  #if defined(CONFIG_HOTPLUG_PCI) || defined(CONFIG_HOTPLUG_PCI_MODULE)
> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> index bf06577..7fac58d 100644
> --- a/virt/kvm/assigned-dev.c
> +++ b/virt/kvm/assigned-dev.c
> @@ -302,7 +302,7 @@ static void kvm_free_assigned_device(struct kvm *kvm,
>  	else
>  		pci_restore_state(assigned_dev->dev);
>  
> -	assigned_dev->dev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
> +	pci_iov_deassign_device(assigned_dev->dev);
>  
>  	pci_release_regions(assigned_dev->dev);
>  	pci_disable_device(assigned_dev->dev);
> diff --git a/virt/kvm/iommu.c b/virt/kvm/iommu.c
> index 0df7d4b..641ee73 100644
> --- a/virt/kvm/iommu.c
> +++ b/virt/kvm/iommu.c
> @@ -194,7 +194,7 @@ int kvm_assign_device(struct kvm *kvm,
>  			goto out_unmap;
>  	}
>  
> -	pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
> +	pci_iov_assign_device(pdev);
>  
>  	dev_info(&pdev->dev, "kvm assign device\n");
>  
> @@ -220,7 +220,7 @@ int kvm_deassign_device(struct kvm *kvm,
>  
>  	iommu_detach_device(domain, &pdev->dev);
>  
> -	pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
> +	pci_iov_deassign_device(pdev);
>  
>  	dev_info(&pdev->dev, "kvm deassign device\n");
>  
>