[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


------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

[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


------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

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.


------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired

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

[Ethan Zhao]

On Fri, Jul 11, 2014 at 8:42 PM, Varka Bhadram <varkabhadram@gmail.com> wrote:
> 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)

Yeah， this one looks better,  that style used to tell myself, I might
forget the which side of  && operator  first, if you sure left first,
I prefer that too :>
>
>
>> +                               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...
   same as above.

Thanks,
Ethan
>
>
>> +                               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.
>

------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

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

[Varka Bhadram]

[-- Attachment #1.1: Type: text/plain, Size: 1104 bytes --]

On 07/11/2014 06:21 PM, Ethan Zhao wrote:
> On Fri, Jul 11, 2014 at 8:42 PM, Varka Bhadram <varkabhadram@gmail.com> wrote:
>> 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)
> Yeah， this one looks better,  that style used to tell myself, I might
> forget the which side of  && operator  first, if you sure left first,
> I prefer that too :>

|for &&  operator - evaluation is from Left to Right|

-- 
Regards,
Varka Bhadram.


[-- Attachment #2: Type: text/plain, Size: 372 bytes --]

------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft

[-- Attachment #3: Type: text/plain, Size: 257 bytes --]

_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

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

[Ethan Zhao]

On Fri, Jul 11, 2014 at 8:56 PM, Varka Bhadram <varkabhadram@gmail.com> wrote:
> On 07/11/2014 06:21 PM, Ethan Zhao wrote:
>
> On Fri, Jul 11, 2014 at 8:42 PM, Varka Bhadram <varkabhadram@gmail.com>
> wrote:
>
> 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)
>
> Yeah， this one looks better,  that style used to tell myself, I might
> forget the which side of  && operator  first, if you sure left first,
> I prefer that too :>
>
> for && operator - evaluation is from Left to Right

Definitely right !

Thanks,
Ethan
>
> --
> Regards,
> Varka Bhadram.

------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

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");
>  




------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

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");
> >  
> 
> 




------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

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

[Ethan Zhao]

> 在 2014年7月11日，下午10:15，Alex Williamson <alex.williamson@redhat.com> 写道：
> 
>> 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?
> 

Of course it is used to tracking PF/VF assigned,  what we gained are - we hide the details
In assignment helpers, we wouldn't bother the users anymore and implement what inside
The helpers, my initial purpose is to simplify the ugly  pci_vfs_assigned() with counter.

I don't want to push more change in one step, that would out of control to get perfect result.
 
>>> 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");
> 
> 
> 

------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

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

[Alex Williamson]

On Fri, 2014-07-11 at 22:37 +0800, Ethan Zhao wrote:
> 
> > 在 2014年7月11日，下午10:15，Alex Williamson <alex.williamson@redhat.com> 写道：
> > 
> >> 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?
> > 
> 
> Of course it is used to tracking PF/VF assigned,  what we gained are - we hide the details
> In assignment helpers, we wouldn't bother the users anymore and implement what inside
> The helpers, my initial purpose is to simplify the ugly  pci_vfs_assigned() with counter.
> 
> I don't want to push more change in one step, that would out of control to get perfect result.

But a counter means that we're tracking the data in two separate places,
which is generally a bad idea.  Cleaning up direct access to flags is
fine, but creating a separate counter doesn't really seem to add any
value.

> >>> 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");
> > 
> > 
> > 




------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

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

[Ethan Zhao]

> 在 2014年7月11日，下午10:54，Alex Williamson <alex.williamson@redhat.com> 写道：
> 
>> On Fri, 2014-07-11 at 22:37 +0800, Ethan Zhao wrote:
>> 
>>>> 在 2014年7月11日，下午10:15，Alex Williamson <alex.williamson@redhat.com> 写道：
>>>> 
>>>>> 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?
>> 
>> Of course it is used to tracking PF/VF assigned,  what we gained are - we hide the details
>> In assignment helpers, we wouldn't bother the users anymore and implement what inside
>> The helpers, my initial purpose is to simplify the ugly  pci_vfs_assigned() with counter.
>> 
>> I don't want to push more change in one step, that would out of control to get perfect result.
> 
> But a counter means that we're tracking the data in two separate places,
> which is generally a bad idea.  Cleaning up direct access to flags is
> fine, but creating a separate counter doesn't really seem to add any
> value.

If you are asked how many apples in your bag , you like to count them one by one and
Give the answer ? What are you doing when you put them into the bag ? 

>>>>> 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");
> 
> 
> 

------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

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

[Alex Williamson]

On Fri, 2014-07-11 at 23:19 +0800, Ethan Zhao wrote:
> 
> > 在 2014年7月11日，下午10:54，Alex Williamson <alex.williamson@redhat.com> 写道：
> > 
> >> On Fri, 2014-07-11 at 22:37 +0800, Ethan Zhao wrote:
> >> 
> >>>> 在 2014年7月11日，下午10:15，Alex Williamson <alex.williamson@redhat.com> 写道：
> >>>> 
> >>>>> 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?
> >> 
> >> Of course it is used to tracking PF/VF assigned,  what we gained are - we hide the details
> >> In assignment helpers, we wouldn't bother the users anymore and implement what inside
> >> The helpers, my initial purpose is to simplify the ugly  pci_vfs_assigned() with counter.
> >> 
> >> I don't want to push more change in one step, that would out of control to get perfect result.
> > 
> > But a counter means that we're tracking the data in two separate places,
> > which is generally a bad idea.  Cleaning up direct access to flags is
> > fine, but creating a separate counter doesn't really seem to add any
> > value.
> 
> If you are asked how many apples in your bag , you like to count them one by one and
> Give the answer ? What are you doing when you put them into the bag ?

If performance mattered, then yes, keeping a count may be advantageous.
Performance does not matter in any path where this is used.

> >>>>> 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");
> > 
> > 
> > 




------------------------------------------------------------------------------
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

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

[Ethan Zhao]

> 在 2014年7月11日，下午11:25，Alex Williamson <alex.williamson@redhat.com> 写道：
> 
>> On Fri, 2014-07-11 at 23:19 +0800, Ethan Zhao wrote:
>> 
>>>> 在 2014年7月11日，下午10:54，Alex Williamson <alex.williamson@redhat.com> 写道：
>>>> 
>>>> On Fri, 2014-07-11 at 22:37 +0800, Ethan Zhao wrote:
>>>> 
>>>>>> 在 2014年7月11日，下午10:15，Alex Williamson <alex.williamson@redhat.com> 写道：
>>>>>> 
>>>>>>> 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?
>>>> 
>>>> Of course it is used to tracking PF/VF assigned,  what we gained are - we hide the details
>>>> In assignment helpers, we wouldn't bother the users anymore and implement what inside
>>>> The helpers, my initial purpose is to simplify the ugly  pci_vfs_assigned() with counter.
>>>> 
>>>> I don't want to push more change in one step, that would out of control to get perfect result.
>>> 
>>> But a counter means that we're tracking the data in two separate places,
>>> which is generally a bad idea.  Cleaning up direct access to flags is
>>> fine, but creating a separate counter doesn't really seem to add any
>>> value.
>> 
>> If you are asked how many apples in your bag , you like to count them one by one and
>> Give the answer ? What are you doing when you put them into the bag ?
> 
> If performance mattered, then yes, keeping a count may be advantageous.
> Performance does not matter in any path where this is used.
> 
Not only performance, concurrency side-effect it caused. 
And less complication, lesser buggy. 
>>>>>>> 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");
>>> 
>>> 
>>> 
> 
> 
> 

------------------------------------------------------------------------------
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

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

[Alex Williamson]

On Sat, 2014-07-12 at 00:06 +0800, Ethan Zhao wrote:
> 
> > 在 2014年7月11日，下午11:25，Alex Williamson <alex.williamson@redhat.com> 写道：
> > 
> >> On Fri, 2014-07-11 at 23:19 +0800, Ethan Zhao wrote:
> >> 
> >>>> 在 2014年7月11日，下午10:54，Alex Williamson <alex.williamson@redhat.com> 写道：
> >>>> 
> >>>> On Fri, 2014-07-11 at 22:37 +0800, Ethan Zhao wrote:
> >>>> 
> >>>>>> 在 2014年7月11日，下午10:15，Alex Williamson <alex.williamson@redhat.com> 写道：
> >>>>>> 
> >>>>>>> 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?
> >>>> 
> >>>> Of course it is used to tracking PF/VF assigned,  what we gained are - we hide the details
> >>>> In assignment helpers, we wouldn't bother the users anymore and implement what inside
> >>>> The helpers, my initial purpose is to simplify the ugly  pci_vfs_assigned() with counter.
> >>>> 
> >>>> I don't want to push more change in one step, that would out of control to get perfect result.
> >>> 
> >>> But a counter means that we're tracking the data in two separate places,
> >>> which is generally a bad idea.  Cleaning up direct access to flags is
> >>> fine, but creating a separate counter doesn't really seem to add any
> >>> value.
> >> 
> >> If you are asked how many apples in your bag , you like to count them one by one and
> >> Give the answer ? What are you doing when you put them into the bag ?
> > 
> > If performance mattered, then yes, keeping a count may be advantageous.
> > Performance does not matter in any path where this is used.
> > 
> Not only performance, concurrency side-effect it caused. 
> And less complication, lesser buggy. 

I'd argue that it doesn't fix anything there, the count can still change
after it's been read but before any action has been taken based on the
count.  The only thing it does is eliminate the act of re-counting.

> >>>>>>> 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");
> >>> 
> >>> 
> >>> 
> > 
> > 
> > 




------------------------------------------------------------------------------
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

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

[Ethan Zhao]

> 在 2014年7月12日，上午12:13，Alex Williamson <alex.williamson@redhat.com> 写道：
> 
>> On Sat, 2014-07-12 at 00:06 +0800, Ethan Zhao wrote:
>> 
>>>> 在 2014年7月11日，下午11:25，Alex Williamson <alex.williamson@redhat.com> 写道：
>>>> 
>>>> On Fri, 2014-07-11 at 23:19 +0800, Ethan Zhao wrote:
>>>> 
>>>>>> 在 2014年7月11日，下午10:54，Alex Williamson <alex.williamson@redhat.com> 写道：
>>>>>> 
>>>>>> On Fri, 2014-07-11 at 22:37 +0800, Ethan Zhao wrote:
>>>>>> 
>>>>>>>> 在 2014年7月11日，下午10:15，Alex Williamson <alex.williamson@redhat.com> 写道：
>>>>>>>> 
>>>>>>>>> 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?
>>>>>> 
>>>>>> Of course it is used to tracking PF/VF assigned,  what we gained are - we hide the details
>>>>>> In assignment helpers, we wouldn't bother the users anymore and implement what inside
>>>>>> The helpers, my initial purpose is to simplify the ugly  pci_vfs_assigned() with counter.
>>>>>> 
>>>>>> I don't want to push more change in one step, that would out of control to get perfect result.
>>>>> 
>>>>> But a counter means that we're tracking the data in two separate places,
>>>>> which is generally a bad idea.  Cleaning up direct access to flags is
>>>>> fine, but creating a separate counter doesn't really seem to add any
>>>>> value.
>>>> 
>>>> If you are asked how many apples in your bag , you like to count them one by one and
>>>> Give the answer ? What are you doing when you put them into the bag ?
>>> 
>>> If performance mattered, then yes, keeping a count may be advantageous.
>>> Performance does not matter in any path where this is used.
>> Not only performance, concurrency side-effect it caused. 
>> And less complication, lesser buggy.
> 
> I'd argue that it doesn't fix anything there, the count can still change
> after it's been read but before any action has been taken based on the
> count.  The only thing it does is eliminate the act of re-counting.

If no counter, how to avoid traversing devices and get VFs assigned count ? To avoid 
Re-counting is enough I think. 

> 
>>>>>>>>> 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");
> 
> 
> 

------------------------------------------------------------------------------
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

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

[Ethan Zhao]

> 在 2014年7月12日，上午12:13，Alex Williamson <alex.williamson@redhat.com> 写道：
> 
>> On Sat, 2014-07-12 at 00:06 +0800, Ethan Zhao wrote:
>> 
>>>> 在 2014年7月11日，下午11:25，Alex Williamson <alex.williamson@redhat.com> 写道：
>>>> 
>>>> On Fri, 2014-07-11 at 23:19 +0800, Ethan Zhao wrote:
>>>> 
>>>>>> 在 2014年7月11日，下午10:54，Alex Williamson <alex.williamson@redhat.com> 写道：
>>>>>> 
>>>>>> On Fri, 2014-07-11 at 22:37 +0800, Ethan Zhao wrote:
>>>>>> 
>>>>>>>> 在 2014年7月11日，下午10:15，Alex Williamson <alex.williamson@redhat.com> 写道：
>>>>>>>> 
>>>>>>>>> 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?
>>>>>> 
>>>>>> Of course it is used to tracking PF/VF assigned,  what we gained are - we hide the details
>>>>>> In assignment helpers, we wouldn't bother the users anymore and implement what inside
>>>>>> The helpers, my initial purpose is to simplify the ugly  pci_vfs_assigned() with counter.
>>>>>> 
>>>>>> I don't want to push more change in one step, that would out of control to get perfect result.
>>>>> 
>>>>> But a counter means that we're tracking the data in two separate places,
>>>>> which is generally a bad idea.  Cleaning up direct access to flags is
>>>>> fine, but creating a separate counter doesn't really seem to add any
>>>>> value.
>>>> 
>>>> If you are asked how many apples in your bag , you like to count them one by one and
>>>> Give the answer ? What are you doing when you put them into the bag ?
>>> 
>>> If performance mattered, then yes, keeping a count may be advantageous.
>>> Performance does not matter in any path where this is used.
>> Not only performance, concurrency side-effect it caused. 
>> And less complication, lesser buggy.
> 
> I'd argue that it doesn't fix anything there, the count can still change
> after it's been read but before any action has been taken based on the
> count.  The only thing it does is eliminate the act of re-counting.
> 
why do you stop me simplifying the pci_vfs_assigned() code ?

>>>>>>>>> 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");
> 
> 
> 

------------------------------------------------------------------------------
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

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

[Yuval Mintz]

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

Sorry for the late intrusion into the conversation, and perhaps it's a bit
unrelated, but given that you're creating a 'VF assignment management'
perhaps it's the right place to add some sort of mechanism in order to
prevent module removal once one of its VFs is assigned.
[e.g., incrementing module reference count]

At the moment [to the best of my knowledge] there's no mechanism
preventing the 'not-so-bright' user from removing the driver, and no good
will come out of it.

Thanks,
Yuval

________________________________

This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.
------------------------------------------------------------------------------
Want fast and easy access to all the code in your enterprise? Index and
search up to 200,000 lines of code with a free copy of Black Duck
Code Sight - the same software that powers the world's largest code
search on Ohloh, the Black Duck Open Hub! Try it now.
http://p.sf.net/sfu/bds
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired

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

[Alex Williamson]

On Sun, 2014-07-20 at 14:48 +0000, Yuval Mintz wrote:
> > 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.
> 
> Sorry for the late intrusion into the conversation, and perhaps it's a bit
> unrelated, but given that you're creating a 'VF assignment management'
> perhaps it's the right place to add some sort of mechanism in order to
> prevent module removal once one of its VFs is assigned.
> [e.g., incrementing module reference count]
> 
> At the moment [to the best of my knowledge] there's no mechanism
> preventing the 'not-so-bright' user from removing the driver, and no good
> will come out of it.

On what module would the reference count be increased, the PF?  The
entire "VF assigned" interface is a hack to work around poor
architectures like legacy KVM device assignment where there's no proper
device owner for the VF.  This is "fixed" by using VFIO instead and
hopefully deprecating legacy KVM assignment.  Thanks,

Alex


------------------------------------------------------------------------------
Want fast and easy access to all the code in your enterprise? Index and
search up to 200,000 lines of code with a free copy of Black Duck
Code Sight - the same software that powers the world's largest code
search on Ohloh, the Black Duck Open Hub! Try it now.
http://p.sf.net/sfu/bds
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired

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

[Ethan Zhao]

On Mon, Jul 21, 2014 at 9:58 AM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Sun, 2014-07-20 at 14:48 +0000, Yuval Mintz wrote:
>> > 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.
>>
>> Sorry for the late intrusion into the conversation, and perhaps it's a bit
>> unrelated, but given that you're creating a 'VF assignment management'
>> perhaps it's the right place to add some sort of mechanism in order to
>> prevent module removal once one of its VFs is assigned.
>> [e.g., incrementing module reference count]
>>
>> At the moment [to the best of my knowledge] there's no mechanism
>> preventing the 'not-so-bright' user from removing the driver, and no good
>> will come out of it.
>
> On what module would the reference count be increased, the PF?  The
> entire "VF assigned" interface is a hack to work around poor
> architectures like legacy KVM device assignment where there's no proper
> device owner for the VF.  This is "fixed" by using VFIO instead and
> hopefully deprecating legacy KVM assignment.  Thanks,

  To explain what Alex said, in another word, if VMs access VF via
VFIO driver, the owner of the device is VFIO, in this case, if you
unload PF driver, you still need to unload VFIO first, then unload PF
driver. but the PF driver knows how to notify the VFIO driver to
unload.
  But without VFIO like driver, for example some of current code will
assign devcie (PF/VF) directly to KVM or XEN without driver in the
middle. and the PF driver doesn't know how to unbind the assignment...

Thanks,
Ethan

>
> Alex
>

------------------------------------------------------------------------------
Want fast and easy access to all the code in your enterprise? Index and
search up to 200,000 lines of code with a free copy of Black Duck
Code Sight - the same software that powers the world's largest code
search on Ohloh, the Black Duck Open Hub! Try it now.
http://p.sf.net/sfu/bds
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

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

[Yuval Mintz]

> >> > > > 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.
> >>
> >> Sorry for the late intrusion into the conversation, and perhaps it's
> >> a bit unrelated, but given that you're creating a 'VF assignment management'
> >> perhaps it's the right place to add some sort of mechanism in order
> >> to prevent module removal once one of its VFs is assigned.
> >> [e.g., incrementing module reference count]
> >>
> >> At the moment [to the best of my knowledge] there's no mechanism
> >> preventing the 'not-so-bright' user from removing the driver, and no
> >> good will come out of it.
> >
> > On what module would the reference count be increased, the PF?  The
> > entire "VF assigned" interface is a hack to work around poor
> > architectures like legacy KVM device assignment where there's no
> > proper device owner for the VF.  This is "fixed" by using VFIO instead
> > and hopefully deprecating legacy KVM assignment.  Thanks,
>
>   To explain what Alex said, in another word, if VMs access VF via VFIO driver,
> the owner of the device is VFIO, in this case, if you unload PF driver, you still
> need to unload VFIO first, then unload PF driver. but the PF driver knows how to
> notify the VFIO driver to unload.
>   But without VFIO like driver, for example some of current code will assign
> devcie (PF/VF) directly to KVM or XEN without driver in the middle. and the PF
> driver doesn't know how to unbind the assignment...

Hi,

I thought about perhaps incrementing the reference count of the PF's module
[i.e., the module supplying the driver for the PF] directly, so THAT module won't
be removable as long as the VF is assigned.

Although I don't know whether the module is even accessible; Can you derive a
pointer to a module from a net_device struct?

Thanks,
Yuval

________________________________

This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.
------------------------------------------------------------------------------
Want fast and easy access to all the code in your enterprise? Index and
search up to 200,000 lines of code with a free copy of Black Duck
Code Sight - the same software that powers the world's largest code
search on Ohloh, the Black Duck Open Hub! Try it now.
http://p.sf.net/sfu/bds
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired

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

[Alex Williamson]

On Mon, 2014-07-21 at 13:17 +0000, Yuval Mintz 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.
> > >>
> > >> Sorry for the late intrusion into the conversation, and perhaps it's
> > >> a bit unrelated, but given that you're creating a 'VF assignment management'
> > >> perhaps it's the right place to add some sort of mechanism in order
> > >> to prevent module removal once one of its VFs is assigned.
> > >> [e.g., incrementing module reference count]
> > >>
> > >> At the moment [to the best of my knowledge] there's no mechanism
> > >> preventing the 'not-so-bright' user from removing the driver, and no
> > >> good will come out of it.
> > >
> > > On what module would the reference count be increased, the PF?  The
> > > entire "VF assigned" interface is a hack to work around poor
> > > architectures like legacy KVM device assignment where there's no
> > > proper device owner for the VF.  This is "fixed" by using VFIO instead
> > > and hopefully deprecating legacy KVM assignment.  Thanks,
> >
> >   To explain what Alex said, in another word, if VMs access VF via VFIO driver,
> > the owner of the device is VFIO, in this case, if you unload PF driver, you still
> > need to unload VFIO first, then unload PF driver. but the PF driver knows how to
> > notify the VFIO driver to unload.
> >   But without VFIO like driver, for example some of current code will assign
> > devcie (PF/VF) directly to KVM or XEN without driver in the middle. and the PF
> > driver doesn't know how to unbind the assignment...
> 
> Hi,
> 
> I thought about perhaps incrementing the reference count of the PF's module
> [i.e., the module supplying the driver for the PF] directly, so THAT module won't
> be removable as long as the VF is assigned.
> 
> Although I don't know whether the module is even accessible; Can you derive a
> pointer to a module from a net_device struct?

A VF is not necessarily a net device.  A pci_dev does have a physfn
pointer though.


------------------------------------------------------------------------------
Want fast and easy access to all the code in your enterprise? Index and
search up to 200,000 lines of code with a free copy of Black Duck
Code Sight - the same software that powers the world's largest code
search on Ohloh, the Black Duck Open Hub! Try it now.
http://p.sf.net/sfu/bds
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired

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

[Yuval Mintz]

> > > >> > > > pci_iov_assign_device(),
> > > >> > > > pci_iov_deassign_device()
> > > >> > > >
> > > >> > > > along with the existed one
> > > >> > > >
> > > >> > > > pci_vfs_assigned()
> > > >> > > >
> > > >> > > > They construct the VFs assignment management interface,
> > > >> perhaps it's the right place to add some sort of mechanism in
> > > >> order to prevent module removal once one of its VFs is assigned.
> > > >> [e.g., incrementing module reference count]
> > > >>
> > > > On what module would the reference count be increased, the PF?
> > > > The entire "VF assigned" interface is a hack to work around poor
> > > > architectures like legacy KVM device assignment where there's no
> > > > proper device owner for the VF.  This is "fixed" by using VFIO
> > > > instead and hopefully deprecating legacy KVM assignment.  Thanks,
> > >
> > >   To explain what Alex said, in another word, if VMs access VF via
> > > VFIO driver, the owner of the device is VFIO, in this case, if you
> > > unload PF driver, you still need to unload VFIO first, then unload
> > > PF driver. but the PF driver knows how to notify the VFIO driver to unload.
> > >   But without VFIO like driver, for example some of current code
> > > will assign devcie (PF/VF) directly to KVM or XEN without driver in
> > > the middle. and the PF driver doesn't know how to unbind the assignment...
> >
> > I thought about perhaps incrementing the reference count of the PF's
> > module [i.e., the module supplying the driver for the PF] directly, so
> > THAT module won't be removable as long as the VF is assigned.
> >
> > Although I don't know whether the module is even accessible; Can you
> > derive a pointer to a module from a net_device struct?
>
> A VF is not necessarily a net device.  A pci_dev does have a physfn pointer
> though.

I stand corrected.

Is there anything inherently wrong about this approach, though?

________________________________

This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.
------------------------------------------------------------------------------
Want fast and easy access to all the code in your enterprise? Index and
search up to 200,000 lines of code with a free copy of Black Duck
Code Sight - the same software that powers the world's largest code
search on Ohloh, the Black Duck Open Hub! Try it now.
http://p.sf.net/sfu/bds
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired

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

[Alex Williamson]

On Mon, 2014-07-21 at 14:11 +0000, Yuval Mintz wrote:
> > > > >> > > > pci_iov_assign_device(),
> > > > >> > > > pci_iov_deassign_device()
> > > > >> > > >
> > > > >> > > > along with the existed one
> > > > >> > > >
> > > > >> > > > pci_vfs_assigned()
> > > > >> > > >
> > > > >> > > > They construct the VFs assignment management interface,
> > > > >> perhaps it's the right place to add some sort of mechanism in
> > > > >> order to prevent module removal once one of its VFs is assigned.
> > > > >> [e.g., incrementing module reference count]
> > > > >>
> > > > > On what module would the reference count be increased, the PF?
> > > > > The entire "VF assigned" interface is a hack to work around poor
> > > > > architectures like legacy KVM device assignment where there's no
> > > > > proper device owner for the VF.  This is "fixed" by using VFIO
> > > > > instead and hopefully deprecating legacy KVM assignment.  Thanks,
> > > >
> > > >   To explain what Alex said, in another word, if VMs access VF via
> > > > VFIO driver, the owner of the device is VFIO, in this case, if you
> > > > unload PF driver, you still need to unload VFIO first, then unload
> > > > PF driver. but the PF driver knows how to notify the VFIO driver to unload.
> > > >   But without VFIO like driver, for example some of current code
> > > > will assign devcie (PF/VF) directly to KVM or XEN without driver in
> > > > the middle. and the PF driver doesn't know how to unbind the assignment...
> > >
> > > I thought about perhaps incrementing the reference count of the PF's
> > > module [i.e., the module supplying the driver for the PF] directly, so
> > > THAT module won't be removable as long as the VF is assigned.
> > >
> > > Although I don't know whether the module is even accessible; Can you
> > > derive a pointer to a module from a net_device struct?
> >
> > A VF is not necessarily a net device.  A pci_dev does have a physfn pointer
> > though.
> 
> I stand corrected.
> 
> Is there anything inherently wrong about this approach, though?

What does incrementing the module reference on the PF driver accomplish?
We can still unbind the PF device from the driver.  That's what this "VF
used" flag is supposed to accomplish is preventing the PF from unbinding
from the driver while VFs are in use, but as has been noted, it's a
terrible hack.

BTW, the below corporate disclaimer below is really inappropriate for
the mailing list.  Thanks,

Alex

> ________________________________
> 
> This message and any attached documents contain information from
> QLogic Corporation or its wholly-owned subsidiaries that may be
> confidential. If you are not the intended recipient, you may not read,
> copy, distribute, or use this information. If you have received this
> transmission in error, please notify the sender immediately by reply
> e-mail and then delete this message.




------------------------------------------------------------------------------
Want fast and easy access to all the code in your enterprise? Index and
search up to 200,000 lines of code with a free copy of Black Duck
Code Sight - the same software that powers the world's largest code
search on Ohloh, the Black Duck Open Hub! Try it now.
http://p.sf.net/sfu/bds
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired

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

[Ethan Zhao]

On Mon, Jul 21, 2014 at 10:39 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Mon, 2014-07-21 at 14:11 +0000, Yuval Mintz wrote:
>> > > > >> > > > pci_iov_assign_device(),
>> > > > >> > > > pci_iov_deassign_device()
>> > > > >> > > >
>> > > > >> > > > along with the existed one
>> > > > >> > > >
>> > > > >> > > > pci_vfs_assigned()
>> > > > >> > > >
>> > > > >> > > > They construct the VFs assignment management interface,
>> > > > >> perhaps it's the right place to add some sort of mechanism in
>> > > > >> order to prevent module removal once one of its VFs is assigned.
>> > > > >> [e.g., incrementing module reference count]
>> > > > >>
>> > > > > On what module would the reference count be increased, the PF?
>> > > > > The entire "VF assigned" interface is a hack to work around poor
>> > > > > architectures like legacy KVM device assignment where there's no
>> > > > > proper device owner for the VF.  This is "fixed" by using VFIO
>> > > > > instead and hopefully deprecating legacy KVM assignment.  Thanks,
>> > > >
>> > > >   To explain what Alex said, in another word, if VMs access VF via
>> > > > VFIO driver, the owner of the device is VFIO, in this case, if you
>> > > > unload PF driver, you still need to unload VFIO first, then unload
>> > > > PF driver. but the PF driver knows how to notify the VFIO driver to unload.
>> > > >   But without VFIO like driver, for example some of current code
>> > > > will assign devcie (PF/VF) directly to KVM or XEN without driver in
>> > > > the middle. and the PF driver doesn't know how to unbind the assignment...
>> > >
>> > > I thought about perhaps incrementing the reference count of the PF's
>> > > module [i.e., the module supplying the driver for the PF] directly, so
>> > > THAT module won't be removable as long as the VF is assigned.
>> > >
>> > > Although I don't know whether the module is even accessible; Can you
>> > > derive a pointer to a module from a net_device struct?
>> >
>> > A VF is not necessarily a net device.  A pci_dev does have a physfn pointer
>> > though.
>>
>> I stand corrected.
>>
>> Is there anything inherently wrong about this approach, though?
>
> What does incrementing the module reference on the PF driver accomplish?
> We can still unbind the PF device from the driver.  That's what this "VF

Unbind PF from its original driver should be OK. regarding some current hacked
implementation that assign PF/VF to KVM/XEN without driver, unbing or removal to
them is nightmare, maybe the best thing we could do is "device is in
using, please
release..."

> used" flag is supposed to accomplish is preventing the PF from unbinding
> from the driver while VFs are in use, but as has been noted, it's a
> terrible hack.
Yep.

Ethan
>
> BTW, the below corporate disclaimer below is really inappropriate for
> the mailing list.  Thanks,
>
> Alex
>
>> ________________________________
>
>
>

------------------------------------------------------------------------------
Want fast and easy access to all the code in your enterprise? Index and
search up to 200,000 lines of code with a free copy of Black Duck
Code Sight - the same software that powers the world's largest code
search on Ohloh, the Black Duck Open Hub! Try it now.
http://p.sf.net/sfu/bds
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

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

[Alexander Duyck]

On 07/21/2014 07:39 AM, Alex Williamson wrote:
> On Mon, 2014-07-21 at 14:11 +0000, Yuval Mintz wrote:
>>>>>>>>>> pci_iov_assign_device(),
>>>>>>>>>> pci_iov_deassign_device()
>>>>>>>>>>
>>>>>>>>>> along with the existed one
>>>>>>>>>>
>>>>>>>>>> pci_vfs_assigned()
>>>>>>>>>>
>>>>>>>>>> They construct the VFs assignment management interface,
>>>>>>> perhaps it's the right place to add some sort of mechanism in
>>>>>>> order to prevent module removal once one of its VFs is assigned.
>>>>>>> [e.g., incrementing module reference count]
>>>>>>>
>>>>>> On what module would the reference count be increased, the PF?
>>>>>> The entire "VF assigned" interface is a hack to work around poor
>>>>>> architectures like legacy KVM device assignment where there's no
>>>>>> proper device owner for the VF.  This is "fixed" by using VFIO
>>>>>> instead and hopefully deprecating legacy KVM assignment.  Thanks,
>>>>>
>>>>>   To explain what Alex said, in another word, if VMs access VF via
>>>>> VFIO driver, the owner of the device is VFIO, in this case, if you
>>>>> unload PF driver, you still need to unload VFIO first, then unload
>>>>> PF driver. but the PF driver knows how to notify the VFIO driver to unload.
>>>>>   But without VFIO like driver, for example some of current code
>>>>> will assign devcie (PF/VF) directly to KVM or XEN without driver in
>>>>> the middle. and the PF driver doesn't know how to unbind the assignment...
>>>>
>>>> I thought about perhaps incrementing the reference count of the PF's
>>>> module [i.e., the module supplying the driver for the PF] directly, so
>>>> THAT module won't be removable as long as the VF is assigned.
>>>>
>>>> Although I don't know whether the module is even accessible; Can you
>>>> derive a pointer to a module from a net_device struct?
>>>
>>> A VF is not necessarily a net device.  A pci_dev does have a physfn pointer
>>> though.
>>
>> I stand corrected.
>>
>> Is there anything inherently wrong about this approach, though?
> 
> What does incrementing the module reference on the PF driver accomplish?
> We can still unbind the PF device from the driver.  That's what this "VF
> used" flag is supposed to accomplish is preventing the PF from unbinding
> from the driver while VFs are in use, but as has been noted, it's a
> terrible hack.
> 

Keep in mind there are also use cases such as what occurs in igb/ixgbe
where the PF driver can be swapped out while the VFs are left in place.
 Nothing explicitly says we cannot unload the PF driver while the VFs
are assigned, we just cannot free the VFs or disable SR-IOV while they
are assigned.

In that scenario we just force the link down on the link state down on
the VFs while the PF is not present.  I'm not sure how many people make
use of the behavior but that is currently how we end up supporting it
for igb/ixgbe.

One approach would be to use a reference count for the SR-IOV itself.  I
know in the case of the igb/ixgbe drivers we use the assigned number of
VFs to determine if we can change the number of VFs allocated or not.  I
imagine you could probably just use the assigned count as a reference
count and if it is non-zero prevent SR-IOV from being disabled.

Thanks,

Alex


------------------------------------------------------------------------------
Want fast and easy access to all the code in your enterprise? Index and
search up to 200,000 lines of code with a free copy of Black Duck
Code Sight - the same software that powers the world's largest code
search on Ohloh, the Black Duck Open Hub! Try it now.
http://p.sf.net/sfu/bds
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired

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

[Ethan Zhao]

On Tue, Jul 22, 2014 at 6:41 AM, Alexander Duyck
<alexander.h.duyck@intel.com> wrote:
> On 07/21/2014 07:39 AM, Alex Williamson wrote:
>> On Mon, 2014-07-21 at 14:11 +0000, Yuval Mintz wrote:
>>>>>>>>>>> pci_iov_assign_device(),
>>>>>>>>>>> pci_iov_deassign_device()
>>>>>>>>>>>
>>>>>>>>>>> along with the existed one
>>>>>>>>>>>
>>>>>>>>>>> pci_vfs_assigned()
>>>>>>>>>>>
>>>>>>>>>>> They construct the VFs assignment management interface,
>>>>>>>> perhaps it's the right place to add some sort of mechanism in
>>>>>>>> order to prevent module removal once one of its VFs is assigned.
>>>>>>>> [e.g., incrementing module reference count]
>>>>>>>>
>>>>>>> On what module would the reference count be increased, the PF?
>>>>>>> The entire "VF assigned" interface is a hack to work around poor
>>>>>>> architectures like legacy KVM device assignment where there's no
>>>>>>> proper device owner for the VF.  This is "fixed" by using VFIO
>>>>>>> instead and hopefully deprecating legacy KVM assignment.  Thanks,
>>>>>>
>>>>>>   To explain what Alex said, in another word, if VMs access VF via
>>>>>> VFIO driver, the owner of the device is VFIO, in this case, if you
>>>>>> unload PF driver, you still need to unload VFIO first, then unload
>>>>>> PF driver. but the PF driver knows how to notify the VFIO driver to unload.
>>>>>>   But without VFIO like driver, for example some of current code
>>>>>> will assign devcie (PF/VF) directly to KVM or XEN without driver in
>>>>>> the middle. and the PF driver doesn't know how to unbind the assignment...
>>>>>
>>>>> I thought about perhaps incrementing the reference count of the PF's
>>>>> module [i.e., the module supplying the driver for the PF] directly, so
>>>>> THAT module won't be removable as long as the VF is assigned.
>>>>>
>>>>> Although I don't know whether the module is even accessible; Can you
>>>>> derive a pointer to a module from a net_device struct?
>>>>
>>>> A VF is not necessarily a net device.  A pci_dev does have a physfn pointer
>>>> though.
>>>
>>> I stand corrected.
>>>
>>> Is there anything inherently wrong about this approach, though?
>>
>> What does incrementing the module reference on the PF driver accomplish?
>> We can still unbind the PF device from the driver.  That's what this "VF
>> used" flag is supposed to accomplish is preventing the PF from unbinding
>> from the driver while VFs are in use, but as has been noted, it's a
>> terrible hack.
>>
>
> Keep in mind there are also use cases such as what occurs in igb/ixgbe
> where the PF driver can be swapped out while the VFs are left in place.
>  Nothing explicitly says we cannot unload the PF driver while the VFs
> are assigned, we just cannot free the VFs or disable SR-IOV while they
> are assigned.

  unbind one PF from the driver but keep the VF reside in it ? the meanwhile
  not shutdown the PF ?

>
> In that scenario we just force the link down on the link state down on
> the VFs while the PF is not present.  I'm not sure how many people make
> use of the behavior but that is currently how we end up supporting it
> for igb/ixgbe.

  Roger !
>
> One approach would be to use a reference count for the SR-IOV itself.  I
> know in the case of the igb/ixgbe drivers we use the assigned number of
> VFs to determine if we can change the number of VFs allocated or not.  I
> imagine you could probably just use the assigned count as a reference
> count and if it is non-zero prevent SR-IOV from being disabled.

  I am managing to figure out a way to cut out the VFs assigned checking before
 disable SR_IOV... ...

  Maybe via composing  pseudo PCI driver for every user VF/PF assigned
to them ?
but does it work when PF swap out without touching VF as you mentioned ?

Thanks,
Ethan
>
> Thanks,
>
> Alex
>

------------------------------------------------------------------------------
Want fast and easy access to all the code in your enterprise? Index and
search up to 200,000 lines of code with a free copy of Black Duck
Code Sight - the same software that powers the world's largest code
search on Ohloh, the Black Duck Open Hub! Try it now.
http://p.sf.net/sfu/bds
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

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

[Ethan Zhao]

On Sun, Jul 20, 2014 at 10:48 PM, Yuval Mintz <Yuval.Mintz@qlogic.com> wrote:
>> 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.
>
> Sorry for the late intrusion into the conversation, and perhaps it's a bit
> unrelated, but given that you're creating a 'VF assignment management'
> perhaps it's the right place to add some sort of mechanism in order to
> prevent module removal once one of its VFs is assigned.
> [e.g., incrementing module reference count]

  This patch set is used to make the current hacked code well formed and make
the interface clear to refactory later.  so we could keep the current
direct assignment and woulnd't encounter the removal issue etc. in one
word, manageable.
>
> At the moment [to the best of my knowledge] there's no mechanism
> preventing the 'not-so-bright' user from removing the driver, and no good
> will come out of it.
 The best way is there is real 'driver' between device and the VM
device manager or some similar machanism to make the assignment
manageable.

Thanks,
Ethan

>
> Thanks,
> Yuval
>
> ________________________________
>
> This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.

------------------------------------------------------------------------------
Want fast and easy access to all the code in your enterprise? Index and
search up to 200,000 lines of code with a free copy of Black Duck
Code Sight - the same software that powers the world's largest code
search on Ohloh, the Black Duck Open Hub! Try it now.
http://p.sf.net/sfu/bds
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

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

[Ethan Zhao]

> 在 2014年7月11日，下午9:13，Alex Williamson <alex.williamson@redhat.com> 写道：
> 
>> 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
> 
 Hmmm,
> - 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.
> 
 You prefer v1 again ? 

> - 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.
> 
 This is real issue, the interface should work without CONFIG_PCI_IOV defined.
Should be fixed.

> - PCI_DEV_FLAGS_ASSIGNED should be unused after this series, why is it
> not removed?
you wanna do all things in one step ? Be practical. That is not my original purpose.
> 
>> 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.
> 
   Such would go back to the v1, four patches, if one lost to be committed, kernel could be buggy.  

>> 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");
> 
> 
> 

------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

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

[Alex Williamson]

On Fri, 2014-07-11 at 22:20 +0800, Ethan Zhao wrote:
> > 在 2014年7月11日，下午9:13，Alex Williamson <alex.williamson@redhat.com> 写道：
> > 
> >> 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
> > 
>  Hmmm,
> > - 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.
> > 
>  You prefer v1 again ? 

That's not at all what I'm suggesting.  In response to v1 I suggested a
specific reordering that would allow the series to be bisectable.  I'm
surprised to see v2 ignored that and combined the first 3 suggested
patches.

> > - 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.
> > 
>  This is real issue, the interface should work without CONFIG_PCI_IOV defined.
> Should be fixed.
> 
> > - PCI_DEV_FLAGS_ASSIGNED should be unused after this series, why is it
> > not removed?
> you wanna do all things in one step ? Be practical. That is not my original purpose.

I never suggested collapsing steps.  Maybe I'd understand the original
purpose better if there was an overall description in a cover patch.

> >> 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.
> > 
>    Such would go back to the v1, four patches, if one lost to be committed, kernel could be buggy.  

There seems to be confusion between multiple patches, which is fine, and
the patch ordering from v1, which is wrong.

> >> 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");
> > 
> > 
> > 




------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

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

[Ethan Zhao]

> 在 2014年7月11日，下午10:50，Alex Williamson <alex.williamson@redhat.com> 写道：
> 
> On Fri, 2014-07-11 at 22:20 +0800, Ethan Zhao wrote:
>>> 在 2014年7月11日，下午9:13，Alex Williamson <alex.williamson@redhat.com> 写道：
>>> 
>>>> 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
>> Hmmm,
>>> - 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.
>> You prefer v1 again ?
> 
> That's not at all what I'm suggesting.  In response to v1 I suggested a
> specific reordering that would allow the series to be bisectable.  I'm
> surprised to see v2 ignored that and combined the first 3 suggested
> patches.
If what I understand right. Your points are.
1. To be bisectable.
    Combine the code into 2 parches, is really bisect able, without 2, 1 works right.
    If 1 was broken into 3, any of them is lost, buggy. They must work together.

 2. Define and abstract first then implement.
   1 &. 2 follows this point.
> 
>>> - 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.
>> This is real issue, the interface should work without CONFIG_PCI_IOV defined.
>> Should be fixed.
>> 
>>> - PCI_DEV_FLAGS_ASSIGNED should be unused after this series, why is it
>>> not removed?
>> you wanna do all things in one step ? Be practical. That is not my original purpose.
> 
> I never suggested collapsing steps.  Maybe I'd understand the original
> purpose better if there was an overall description in a cover patch.
Compose a cover letter seems necessary to describe patch set if they are too complex.
 
> 
>>>> 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.
>>   Such would go back to the v1, four patches, if one lost to be committed, kernel could be buggy.  
> 
> There seems to be confusion between multiple patches, which is fine, and
> the patch ordering from v1, which is wrong.
> 
Do you want them work standalone, if no, the confusion is nothing.
>>>> 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");
> 
> 
> 

------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

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

[Alex Williamson]

On Fri, 2014-07-11 at 23:07 +0800, Ethan Zhao wrote:
> 
> > 在 2014年7月11日，下午10:50，Alex Williamson <alex.williamson@redhat.com> 写道：
> > 
> > On Fri, 2014-07-11 at 22:20 +0800, Ethan Zhao wrote:
> >>> 在 2014年7月11日，下午9:13，Alex Williamson <alex.williamson@redhat.com> 写道：
> >>> 
> >>>> 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
> >> Hmmm,
> >>> - 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.
> >> You prefer v1 again ?
> > 
> > That's not at all what I'm suggesting.  In response to v1 I suggested a
> > specific reordering that would allow the series to be bisectable.  I'm
> > surprised to see v2 ignored that and combined the first 3 suggested
> > patches.
> If what I understand right. Your points are.
> 1. To be bisectable.
>     Combine the code into 2 parches, is really bisect able, without 2, 1 works right.
>     If 1 was broken into 3, any of them is lost, buggy. They must work together.

No, I never said 2 patches.  I said to fix and abstract the usage, then
change the implementation.  Fixing and abstracting the usage should be
multiple patches.  You should not be concerned about patches in a series
getting lost, you should be concerned that each step along the way is
functional.

>  2. Define and abstract first then implement.
>    1 &. 2 follows this point.
> > 
> >>> - 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.
> >> This is real issue, the interface should work without CONFIG_PCI_IOV defined.
> >> Should be fixed.
> >> 
> >>> - PCI_DEV_FLAGS_ASSIGNED should be unused after this series, why is it
> >>> not removed?
> >> you wanna do all things in one step ? Be practical. That is not my original purpose.
> > 
> > I never suggested collapsing steps.  Maybe I'd understand the original
> > purpose better if there was an overall description in a cover patch.
> Compose a cover letter seems necessary to describe patch set if they are too complex.

Any series with more than a single patch should have a cover letter.

> > 
> >>>> 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.
> >>   Such would go back to the v1, four patches, if one lost to be committed, kernel could be buggy.  
> > 
> > There seems to be confusion between multiple patches, which is fine, and
> > the patch ordering from v1, which is wrong.
> > 
> Do you want them work standalone, if no, the confusion is nothing.

Each step in a series should be functional.  There's absolutely no
requirement that any individual patch from a series can be pulled out
separately and work on it's own.  Patches build on each other.

> >>>> 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");
> > 
> > 
> > 




------------------------------------------------------------------------------
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

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

[Ethan Zhao]

> 在 2014年7月11日，下午11:23，Alex Williamson <alex.williamson@redhat.com> 写道：
> 
>> On Fri, 2014-07-11 at 23:07 +0800, Ethan Zhao wrote:
>> 
>>> 在 2014年7月11日，下午10:50，Alex Williamson <alex.williamson@redhat.com> 写道：
>>> 
>>> On Fri, 2014-07-11 at 22:20 +0800, Ethan Zhao wrote:
>>>>>> 在 2014年7月11日，下午9:13，Alex Williamson <alex.williamson@redhat.com> 写道：
>>>>>> 
>>>>>> 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
>>>> Hmmm,
>>>>> - 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.
>>>> You prefer v1 again ?
>>> 
>>> That's not at all what I'm suggesting.  In response to v1 I suggested a
>>> specific reordering that would allow the series to be bisectable.  I'm
>>> surprised to see v2 ignored that and combined the first 3 suggested
>>> patches.
>> If what I understand right. Your points are.
>> 1. To be bisectable.
>>    Combine the code into 2 parches, is really bisect able, without 2, 1 works right.
>>    If 1 was broken into 3, any of them is lost, buggy. They must work together.
> 
> No, I never said 2 patches.  I said to fix and abstract the usage, then
> change the implementation.  Fixing and abstracting the usage should be
> multiple patches.  You should not be concerned about patches in a series
> getting lost, you should be concerned that each step along the way is
> functional.
> 
How about  5 patches ?
 - 1 . Fix i40e with pci_vfs_assigned()
 - 2.  Define assign/design helper.
 - 3.  KVM uses helpers
 - 4.  Xen uses helpers
 - 5. Introduce VF ref counter.

This series looks like you first suggestion.

>>> pci_vfs_assigned
>> 2. Define and abstract first then implement.
>>   1 &. 2 follows this point.
>>> 
>>>>> - 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.
>>>> This is real issue, the interface should work without CONFIG_PCI_IOV defined.
>>>> Should be fixed.
>>>> 
>>>>> - PCI_DEV_FLAGS_ASSIGNED should be unused after this series, why is it
>>>>> not removed?
>>>> you wanna do all things in one step ? Be practical. That is not my original purpose.
>>> 
>>> I never suggested collapsing steps.  Maybe I'd understand the original
>>> purpose better if there was an overall description in a cover patch.
>> Compose a cover letter seems necessary to describe patch set if they are too complex.
> 
> Any series with more than a single patch should have a cover letter.
> 
>>> 
>>>>>> 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.
>>>>  Such would go back to the v1, four patches, if one lost to be committed, kernel could be buggy.  
>>> 
>>> There seems to be confusion between multiple patches, which is fine, and
>>> the patch ordering from v1, which is wrong.
>> Do you want them work standalone, if no, the confusion is nothing.
> 
> Each step in a series should be functional.  There's absolutely no
> requirement that any individual patch from a series can be pulled out
> separately and work on it's own.  Patches build on each other.
> 
>>>>>> 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");
> 
> 
> 

------------------------------------------------------------------------------
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

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");
>  
> 

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

[Ethan Zhao]

On Tue, Jul 22, 2014 at 6:29 AM, Alexander Duyck
<alexander.h.duyck@intel.com> wrote:
> 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.

 Agree, thanks.

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

Agree, if I still stand for this set of helpers.

>
>> 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");
>>
>>
>

------------------------------------------------------------------------------
Want fast and easy access to all the code in your enterprise? Index and
search up to 200,000 lines of code with a free copy of Black Duck
Code Sight - the same software that powers the world's largest code
search on Ohloh, the Black Duck Open Hub! Try it now.
http://p.sf.net/sfu/bds
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired