Current implementation of helper function pci_vfs_assigned() is a little complex, to
get sum of VFs that assigned to VM, access low level configuration space register and
then loop in traversing device tree.
This patch introduce an atomic reference counter for VFs that assigned to VM in struct
pci_sriov, and compose two more helper functions
pci_sriov_assign_device(),
pci_sriov_deassign_device()
to replace manipulation to device flag and the meanwhile increase and decease the counter.
Passed building on 3.15.5
Signed-off-by: Ethan Zhao <[email protected]>
---
drivers/pci/iov.c | 65 ++++++++++++++++++++++++++++----------------------
drivers/pci/pci.h | 1 +
include/linux/pci.h | 4 +++
3 files changed, 41 insertions(+), 29 deletions(-)
diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index de7a747..72e267f 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -382,6 +382,7 @@ found:
iov->nres = nres;
iov->ctrl = ctrl;
iov->total_VFs = total;
+ atomic_set(&iov->VFs_assigned_cnt, 0);
iov->offset = offset;
iov->stride = stride;
iov->pgsz = pgsz;
@@ -603,45 +604,51 @@ int pci_num_vf(struct pci_dev *dev)
EXPORT_SYMBOL_GPL(pci_num_vf);
/**
- * pci_vfs_assigned - returns number of VFs are assigned to a guest
- * @dev: the PCI device
+ * pci_vfs_assigned - returns number of VFs are assigned to VM
+ * @dev: the physical PCI device that contains the VFs.
*
- * Returns number of VFs belonging to this device that are assigned to a guest.
+ * Returns number of VFs belonging to this device that are assigned to VM.
* If device is not a physical function returns 0.
*/
int pci_vfs_assigned(struct pci_dev *dev)
{
- struct pci_dev *vfdev;
- unsigned int vfs_assigned = 0;
- unsigned short dev_id;
-
- /* only search if we are a PF */
if (!dev->is_physfn)
return 0;
+ if (dev->sriov)
+ return atomic_read(&dev->sriov->VFs_assigned_cnt);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(pci_vfs_assigned);
- /*
- * determine the device ID for the VFs, the vendor ID will be the
- * same as the PF so there is no need to check for that one
- */
- pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_VF_DID, &dev_id);
-
- /* loop through all the VFs to see if we own any that are assigned */
- vfdev = pci_get_device(dev->vendor, dev_id, NULL);
- while (vfdev) {
- /*
- * It is considered assigned if it is a virtual function with
- * our dev as the physical function and the assigned bit is set
- */
- if (vfdev->is_virtfn && (vfdev->physfn == dev) &&
- (vfdev->dev_flags & PCI_DEV_FLAGS_ASSIGNED))
- vfs_assigned++;
-
- vfdev = pci_get_device(dev->vendor, dev_id, vfdev);
- }
+/**
+ * pci_sriov_assign_device - assign device to VM
+ * @pdev: the device to be assigned.
+ */
+void pci_sriov_assign_device(struct pci_dev *pdev)
+{
+ pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
+ if (pdev->is_virtfn && !pdev->is_physfn)
+ if (pdev->physfn)
+ if (pdev->physfn->sriov)
+ atomic_inc(&pdev->physfn->sriov->
+ VFs_assigned_cnt);
+}
+EXPORT_SYMBOL_GPL(pci_sriov_assign_device);
- return vfs_assigned;
+/**
+ * pci_sriov_deassign_device - deasign device from VM
+ * @pdev: the device to be deassigned.
+ */
+void pci_sriov_deassign_device(struct pci_dev *pdev)
+{
+ pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
+ if (pdev->is_virtfn && !pdev->is_physfn)
+ if (pdev->physfn)
+ if (pdev->physfn->sriov)
+ atomic_dec(&pdev->physfn->sriov->
+ VFs_assigned_cnt);
}
-EXPORT_SYMBOL_GPL(pci_vfs_assigned);
+EXPORT_SYMBOL_GPL(pci_sriov_deassign_device);
/**
* pci_sriov_set_totalvfs -- reduce the TotalVFs available
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 6bd0822..d17bda2 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -235,6 +235,7 @@ struct pci_sriov {
u32 pgsz; /* page size for BAR alignment */
u8 link; /* Function Dependency Link */
u16 driver_max_VFs; /* max num VFs driver supports */
+ atomic_t VFs_assigned_cnt; /* counter of VFs assigned to VM */
struct pci_dev *dev; /* lowest numbered PF */
struct pci_dev *self; /* this PF */
struct mutex lock; /* lock for VF bus */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index aab57b4..5cf6833 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1601,6 +1601,8 @@ int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
void pci_disable_sriov(struct pci_dev *dev);
int pci_num_vf(struct pci_dev *dev);
int pci_vfs_assigned(struct pci_dev *dev);
+void pci_sriov_assign_device(struct pci_dev *dev);
+void pci_sriov_deassign_device(struct pci_dev *dev);
int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
int pci_sriov_get_totalvfs(struct pci_dev *dev);
#else
@@ -1610,6 +1612,8 @@ static inline void pci_disable_sriov(struct pci_dev *dev) { }
static inline int pci_num_vf(struct pci_dev *dev) { return 0; }
static inline int pci_vfs_assigned(struct pci_dev *dev)
{ return 0; }
+static inline void pci_sriov_assign_device(struct pci_dev *dev) { }
+static inline void pci_sriov_deassign_device(struct pci_dev *dev) { }
static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
{ return 0; }
static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
--
1.7.1
New VFs reference counter mechanism and VFs assignment helper functions are introduced to
PCI SRIOV, use them instead of manipulating device flag directly.
Signed-off-by: Ethan Zhao <[email protected]>
---
virt/kvm/assigned-dev.c | 2 +-
virt/kvm/iommu.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
index bf06577..80b477f 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_sriov_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..6d3f5ef 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_sriov_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_sriov_deassign_device(pdev);
dev_info(&pdev->dev, "kvm deassign device\n");
--
1.7.1
New VFs reference counter mechanism and VFs assignment helper functions are introduced to
PCI SRIOV, use them instead of manipulating device flag directly.
Signed-off-by: Ethan Zhao <[email protected]>
---
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 17 ++---------------
1 files changed, 2 insertions(+), 15 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;
}
--
1.7.1
New VFs reference counter mechanism and VFs assignment helper functions are introduced to
PCI SRIOV, use them instead of manipulating device flag directly.
Signed-off-by: Ethan Zhao <[email protected]>
---
drivers/xen/xen-pciback/pci_stub.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index 62fcd48..1d7b747 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_sriov_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_sriov_assign_device(dev);
return 0;
config_release:
--
1.7.1
Since there's no 0th patch, I guess I'll comment here. This series is
not bisectable, patch 1 breaks the existing implementation. I'd
suggest:
patch 1 - fix i40e
patch 2 - create assign/deassign that uses dev_flags
patch 3 - convert users to new interface
patch 4 - convert interface to use atomic_t
IMHO, the assigned flag is a horrible hack and I don't know why drivers
like xenback need to use it. KVM needs to use it because it doesn't
actually have a driver to bind to when a device is assigned, it's happy
to assign devices without any driver. Thanks,
Alex
On Fri, 2014-07-11 at 08:47 +0800, Ethan Zhao wrote:
> Current implementation of helper function pci_vfs_assigned() is a little complex, to
> get sum of VFs that assigned to VM, access low level configuration space register and
> then loop in traversing device tree.
>
> This patch introduce an atomic reference counter for VFs that assigned to VM in struct
> pci_sriov, and compose two more helper functions
>
> pci_sriov_assign_device(),
> pci_sriov_deassign_device()
>
> to replace manipulation to device flag and the meanwhile increase and decease the counter.
>
> Passed building on 3.15.5
>
> Signed-off-by: Ethan Zhao <[email protected]>
> ---
> drivers/pci/iov.c | 65 ++++++++++++++++++++++++++++----------------------
> drivers/pci/pci.h | 1 +
> include/linux/pci.h | 4 +++
> 3 files changed, 41 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index de7a747..72e267f 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -382,6 +382,7 @@ found:
> iov->nres = nres;
> iov->ctrl = ctrl;
> iov->total_VFs = total;
> + atomic_set(&iov->VFs_assigned_cnt, 0);
> iov->offset = offset;
> iov->stride = stride;
> iov->pgsz = pgsz;
> @@ -603,45 +604,51 @@ int pci_num_vf(struct pci_dev *dev)
> EXPORT_SYMBOL_GPL(pci_num_vf);
>
> /**
> - * pci_vfs_assigned - returns number of VFs are assigned to a guest
> - * @dev: the PCI device
> + * pci_vfs_assigned - returns number of VFs are assigned to VM
> + * @dev: the physical PCI device that contains the VFs.
> *
> - * Returns number of VFs belonging to this device that are assigned to a guest.
> + * Returns number of VFs belonging to this device that are assigned to VM.
> * If device is not a physical function returns 0.
> */
> int pci_vfs_assigned(struct pci_dev *dev)
> {
> - struct pci_dev *vfdev;
> - unsigned int vfs_assigned = 0;
> - unsigned short dev_id;
> -
> - /* only search if we are a PF */
> if (!dev->is_physfn)
> return 0;
> + if (dev->sriov)
> + return atomic_read(&dev->sriov->VFs_assigned_cnt);
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(pci_vfs_assigned);
>
> - /*
> - * determine the device ID for the VFs, the vendor ID will be the
> - * same as the PF so there is no need to check for that one
> - */
> - pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_VF_DID, &dev_id);
> -
> - /* loop through all the VFs to see if we own any that are assigned */
> - vfdev = pci_get_device(dev->vendor, dev_id, NULL);
> - while (vfdev) {
> - /*
> - * It is considered assigned if it is a virtual function with
> - * our dev as the physical function and the assigned bit is set
> - */
> - if (vfdev->is_virtfn && (vfdev->physfn == dev) &&
> - (vfdev->dev_flags & PCI_DEV_FLAGS_ASSIGNED))
> - vfs_assigned++;
> -
> - vfdev = pci_get_device(dev->vendor, dev_id, vfdev);
> - }
> +/**
> + * pci_sriov_assign_device - assign device to VM
> + * @pdev: the device to be assigned.
> + */
> +void pci_sriov_assign_device(struct pci_dev *pdev)
> +{
> + pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
> + if (pdev->is_virtfn && !pdev->is_physfn)
> + if (pdev->physfn)
> + if (pdev->physfn->sriov)
> + atomic_inc(&pdev->physfn->sriov->
> + VFs_assigned_cnt);
> +}
> +EXPORT_SYMBOL_GPL(pci_sriov_assign_device);
>
> - return vfs_assigned;
> +/**
> + * pci_sriov_deassign_device - deasign device from VM
> + * @pdev: the device to be deassigned.
> + */
> +void pci_sriov_deassign_device(struct pci_dev *pdev)
> +{
> + pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
> + if (pdev->is_virtfn && !pdev->is_physfn)
> + if (pdev->physfn)
> + if (pdev->physfn->sriov)
> + atomic_dec(&pdev->physfn->sriov->
> + VFs_assigned_cnt);
> }
> -EXPORT_SYMBOL_GPL(pci_vfs_assigned);
> +EXPORT_SYMBOL_GPL(pci_sriov_deassign_device);
>
> /**
> * pci_sriov_set_totalvfs -- reduce the TotalVFs available
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 6bd0822..d17bda2 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -235,6 +235,7 @@ struct pci_sriov {
> u32 pgsz; /* page size for BAR alignment */
> u8 link; /* Function Dependency Link */
> u16 driver_max_VFs; /* max num VFs driver supports */
> + atomic_t VFs_assigned_cnt; /* counter of VFs assigned to VM */
> struct pci_dev *dev; /* lowest numbered PF */
> struct pci_dev *self; /* this PF */
> struct mutex lock; /* lock for VF bus */
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index aab57b4..5cf6833 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1601,6 +1601,8 @@ int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
> void pci_disable_sriov(struct pci_dev *dev);
> int pci_num_vf(struct pci_dev *dev);
> int pci_vfs_assigned(struct pci_dev *dev);
> +void pci_sriov_assign_device(struct pci_dev *dev);
> +void pci_sriov_deassign_device(struct pci_dev *dev);
> int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
> int pci_sriov_get_totalvfs(struct pci_dev *dev);
> #else
> @@ -1610,6 +1612,8 @@ static inline void pci_disable_sriov(struct pci_dev *dev) { }
> static inline int pci_num_vf(struct pci_dev *dev) { return 0; }
> static inline int pci_vfs_assigned(struct pci_dev *dev)
> { return 0; }
> +static inline void pci_sriov_assign_device(struct pci_dev *dev) { }
> +static inline void pci_sriov_deassign_device(struct pci_dev *dev) { }
> static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
> { return 0; }
> static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
Alex,
Thanks for your reviewing, when I create the patch order, I thought
about the question you concerned for
quit a while, make every patch be independent to each other as possible
as I could, so we can do bisect when hit
problem.
I manage to take more time to figure out better patch order.
Thanks,
Ethan
On 2014/7/11 9:48, Alex Williamson wrote:
> Since there's no 0th patch, I guess I'll comment here. This series is
> not bisectable, patch 1 breaks the existing implementation. I'd
> suggest:
>
> patch 1 - fix i40e
i40e only could be fixed with new interface, so it couldn't
be the first one.
> patch 2 - create assign/deassign that uses dev_flags
This will be the first ?
> patch 3 - convert users to new interface
Have to be the later step.
> patch 4 - convert interface to use atomic_t
Could it be standalone step ?
Let me think about it.
> IMHO, the assigned flag is a horrible hack and I don't know why drivers
> like xenback need to use it. KVM needs to use it because it doesn't
> actually have a driver to bind to when a device is assigned, it's happy
> to assign devices without any driver. Thanks,
>
> Alex
>
>
> On Fri, 2014-07-11 at 08:47 +0800, Ethan Zhao wrote:
>> Current implementation of helper function pci_vfs_assigned() is a little complex, to
>> get sum of VFs that assigned to VM, access low level configuration space register and
>> then loop in traversing device tree.
>>
>> This patch introduce an atomic reference counter for VFs that assigned to VM in struct
>> pci_sriov, and compose two more helper functions
>>
>> pci_sriov_assign_device(),
>> pci_sriov_deassign_device()
>>
>> to replace manipulation to device flag and the meanwhile increase and decease the counter.
>>
>> Passed building on 3.15.5
>>
>> Signed-off-by: Ethan Zhao <[email protected]>
>> ---
>> drivers/pci/iov.c | 65 ++++++++++++++++++++++++++++----------------------
>> drivers/pci/pci.h | 1 +
>> include/linux/pci.h | 4 +++
>> 3 files changed, 41 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>> index de7a747..72e267f 100644
>> --- a/drivers/pci/iov.c
>> +++ b/drivers/pci/iov.c
>> @@ -382,6 +382,7 @@ found:
>> iov->nres = nres;
>> iov->ctrl = ctrl;
>> iov->total_VFs = total;
>> + atomic_set(&iov->VFs_assigned_cnt, 0);
>> iov->offset = offset;
>> iov->stride = stride;
>> iov->pgsz = pgsz;
>> @@ -603,45 +604,51 @@ int pci_num_vf(struct pci_dev *dev)
>> EXPORT_SYMBOL_GPL(pci_num_vf);
>>
>> /**
>> - * pci_vfs_assigned - returns number of VFs are assigned to a guest
>> - * @dev: the PCI device
>> + * pci_vfs_assigned - returns number of VFs are assigned to VM
>> + * @dev: the physical PCI device that contains the VFs.
>> *
>> - * Returns number of VFs belonging to this device that are assigned to a guest.
>> + * Returns number of VFs belonging to this device that are assigned to VM.
>> * If device is not a physical function returns 0.
>> */
>> int pci_vfs_assigned(struct pci_dev *dev)
>> {
>> - struct pci_dev *vfdev;
>> - unsigned int vfs_assigned = 0;
>> - unsigned short dev_id;
>> -
>> - /* only search if we are a PF */
>> if (!dev->is_physfn)
>> return 0;
>> + if (dev->sriov)
>> + return atomic_read(&dev->sriov->VFs_assigned_cnt);
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(pci_vfs_assigned);
>>
>> - /*
>> - * determine the device ID for the VFs, the vendor ID will be the
>> - * same as the PF so there is no need to check for that one
>> - */
>> - pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_VF_DID, &dev_id);
>> -
>> - /* loop through all the VFs to see if we own any that are assigned */
>> - vfdev = pci_get_device(dev->vendor, dev_id, NULL);
>> - while (vfdev) {
>> - /*
>> - * It is considered assigned if it is a virtual function with
>> - * our dev as the physical function and the assigned bit is set
>> - */
>> - if (vfdev->is_virtfn && (vfdev->physfn == dev) &&
>> - (vfdev->dev_flags & PCI_DEV_FLAGS_ASSIGNED))
>> - vfs_assigned++;
>> -
>> - vfdev = pci_get_device(dev->vendor, dev_id, vfdev);
>> - }
>> +/**
>> + * pci_sriov_assign_device - assign device to VM
>> + * @pdev: the device to be assigned.
>> + */
>> +void pci_sriov_assign_device(struct pci_dev *pdev)
>> +{
>> + pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
>> + if (pdev->is_virtfn && !pdev->is_physfn)
>> + if (pdev->physfn)
>> + if (pdev->physfn->sriov)
>> + atomic_inc(&pdev->physfn->sriov->
>> + VFs_assigned_cnt);
>> +}
>> +EXPORT_SYMBOL_GPL(pci_sriov_assign_device);
>>
>> - return vfs_assigned;
>> +/**
>> + * pci_sriov_deassign_device - deasign device from VM
>> + * @pdev: the device to be deassigned.
>> + */
>> +void pci_sriov_deassign_device(struct pci_dev *pdev)
>> +{
>> + pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
>> + if (pdev->is_virtfn && !pdev->is_physfn)
>> + if (pdev->physfn)
>> + if (pdev->physfn->sriov)
>> + atomic_dec(&pdev->physfn->sriov->
>> + VFs_assigned_cnt);
>> }
>> -EXPORT_SYMBOL_GPL(pci_vfs_assigned);
>> +EXPORT_SYMBOL_GPL(pci_sriov_deassign_device);
>>
>> /**
>> * pci_sriov_set_totalvfs -- reduce the TotalVFs available
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 6bd0822..d17bda2 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -235,6 +235,7 @@ struct pci_sriov {
>> u32 pgsz; /* page size for BAR alignment */
>> u8 link; /* Function Dependency Link */
>> u16 driver_max_VFs; /* max num VFs driver supports */
>> + atomic_t VFs_assigned_cnt; /* counter of VFs assigned to VM */
>> struct pci_dev *dev; /* lowest numbered PF */
>> struct pci_dev *self; /* this PF */
>> struct mutex lock; /* lock for VF bus */
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index aab57b4..5cf6833 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -1601,6 +1601,8 @@ int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
>> void pci_disable_sriov(struct pci_dev *dev);
>> int pci_num_vf(struct pci_dev *dev);
>> int pci_vfs_assigned(struct pci_dev *dev);
>> +void pci_sriov_assign_device(struct pci_dev *dev);
>> +void pci_sriov_deassign_device(struct pci_dev *dev);
>> int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
>> int pci_sriov_get_totalvfs(struct pci_dev *dev);
>> #else
>> @@ -1610,6 +1612,8 @@ static inline void pci_disable_sriov(struct pci_dev *dev) { }
>> static inline int pci_num_vf(struct pci_dev *dev) { return 0; }
>> static inline int pci_vfs_assigned(struct pci_dev *dev)
>> { return 0; }
>> +static inline void pci_sriov_assign_device(struct pci_dev *dev) { }
>> +static inline void pci_sriov_deassign_device(struct pci_dev *dev) { }
>> static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
>> { return 0; }
>> static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
>
>
On Fri, 2014-07-11 at 10:10 +0800, ethan zhao wrote:
> Alex,
> Thanks for your reviewing, when I create the patch order, I thought
> about the question you concerned for
> quit a while, make every patch be independent to each other as possible
> as I could, so we can do bisect when hit
> problem.
>
> I manage to take more time to figure out better patch order.
>
> Thanks,
> Ethan
>
> On 2014/7/11 9:48, Alex Williamson wrote:
> > Since there's no 0th patch, I guess I'll comment here. This series is
> > not bisectable, patch 1 breaks the existing implementation. I'd
> > suggest:
> >
> > patch 1 - fix i40e
> i40e only could be fixed with new interface, so it couldn't
> be the first one.
It looks like i40e just has it's own copy of pci_vfs_assigned(), why
can't your current patch 4/4 be applied now?
> > patch 2 - create assign/deassign that uses dev_flags
> This will be the first ?
> > patch 3 - convert users to new interface
> Have to be the later step.
> > patch 4 - convert interface to use atomic_t
> Could it be standalone step ?
>
> Let me think about it.
>
> > IMHO, the assigned flag is a horrible hack and I don't know why drivers
> > like xenback need to use it. KVM needs to use it because it doesn't
> > actually have a driver to bind to when a device is assigned, it's happy
> > to assign devices without any driver. Thanks,
> >
> > Alex
> >
> >
> > On Fri, 2014-07-11 at 08:47 +0800, Ethan Zhao wrote:
> >> Current implementation of helper function pci_vfs_assigned() is a little complex, to
> >> get sum of VFs that assigned to VM, access low level configuration space register and
> >> then loop in traversing device tree.
> >>
> >> This patch introduce an atomic reference counter for VFs that assigned to VM in struct
> >> pci_sriov, and compose two more helper functions
> >>
> >> pci_sriov_assign_device(),
> >> pci_sriov_deassign_device()
> >>
> >> to replace manipulation to device flag and the meanwhile increase and decease the counter.
> >>
> >> Passed building on 3.15.5
> >>
> >> Signed-off-by: Ethan Zhao <[email protected]>
> >> ---
> >> drivers/pci/iov.c | 65 ++++++++++++++++++++++++++++----------------------
> >> drivers/pci/pci.h | 1 +
> >> include/linux/pci.h | 4 +++
> >> 3 files changed, 41 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> >> index de7a747..72e267f 100644
> >> --- a/drivers/pci/iov.c
> >> +++ b/drivers/pci/iov.c
> >> @@ -382,6 +382,7 @@ found:
> >> iov->nres = nres;
> >> iov->ctrl = ctrl;
> >> iov->total_VFs = total;
> >> + atomic_set(&iov->VFs_assigned_cnt, 0);
> >> iov->offset = offset;
> >> iov->stride = stride;
> >> iov->pgsz = pgsz;
> >> @@ -603,45 +604,51 @@ int pci_num_vf(struct pci_dev *dev)
> >> EXPORT_SYMBOL_GPL(pci_num_vf);
> >>
> >> /**
> >> - * pci_vfs_assigned - returns number of VFs are assigned to a guest
> >> - * @dev: the PCI device
> >> + * pci_vfs_assigned - returns number of VFs are assigned to VM
> >> + * @dev: the physical PCI device that contains the VFs.
> >> *
> >> - * Returns number of VFs belonging to this device that are assigned to a guest.
> >> + * Returns number of VFs belonging to this device that are assigned to VM.
> >> * If device is not a physical function returns 0.
> >> */
> >> int pci_vfs_assigned(struct pci_dev *dev)
> >> {
> >> - struct pci_dev *vfdev;
> >> - unsigned int vfs_assigned = 0;
> >> - unsigned short dev_id;
> >> -
> >> - /* only search if we are a PF */
> >> if (!dev->is_physfn)
> >> return 0;
> >> + if (dev->sriov)
> >> + return atomic_read(&dev->sriov->VFs_assigned_cnt);
> >> + return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(pci_vfs_assigned);
> >>
> >> - /*
> >> - * determine the device ID for the VFs, the vendor ID will be the
> >> - * same as the PF so there is no need to check for that one
> >> - */
> >> - pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_VF_DID, &dev_id);
> >> -
> >> - /* loop through all the VFs to see if we own any that are assigned */
> >> - vfdev = pci_get_device(dev->vendor, dev_id, NULL);
> >> - while (vfdev) {
> >> - /*
> >> - * It is considered assigned if it is a virtual function with
> >> - * our dev as the physical function and the assigned bit is set
> >> - */
> >> - if (vfdev->is_virtfn && (vfdev->physfn == dev) &&
> >> - (vfdev->dev_flags & PCI_DEV_FLAGS_ASSIGNED))
> >> - vfs_assigned++;
> >> -
> >> - vfdev = pci_get_device(dev->vendor, dev_id, vfdev);
> >> - }
> >> +/**
> >> + * pci_sriov_assign_device - assign device to VM
> >> + * @pdev: the device to be assigned.
> >> + */
> >> +void pci_sriov_assign_device(struct pci_dev *pdev)
> >> +{
> >> + pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
> >> + if (pdev->is_virtfn && !pdev->is_physfn)
> >> + if (pdev->physfn)
> >> + if (pdev->physfn->sriov)
> >> + atomic_inc(&pdev->physfn->sriov->
> >> + VFs_assigned_cnt);
> >> +}
> >> +EXPORT_SYMBOL_GPL(pci_sriov_assign_device);
> >>
> >> - return vfs_assigned;
> >> +/**
> >> + * pci_sriov_deassign_device - deasign device from VM
> >> + * @pdev: the device to be deassigned.
> >> + */
> >> +void pci_sriov_deassign_device(struct pci_dev *pdev)
> >> +{
> >> + pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
> >> + if (pdev->is_virtfn && !pdev->is_physfn)
> >> + if (pdev->physfn)
> >> + if (pdev->physfn->sriov)
> >> + atomic_dec(&pdev->physfn->sriov->
> >> + VFs_assigned_cnt);
> >> }
> >> -EXPORT_SYMBOL_GPL(pci_vfs_assigned);
> >> +EXPORT_SYMBOL_GPL(pci_sriov_deassign_device);
> >>
> >> /**
> >> * pci_sriov_set_totalvfs -- reduce the TotalVFs available
> >> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> >> index 6bd0822..d17bda2 100644
> >> --- a/drivers/pci/pci.h
> >> +++ b/drivers/pci/pci.h
> >> @@ -235,6 +235,7 @@ struct pci_sriov {
> >> u32 pgsz; /* page size for BAR alignment */
> >> u8 link; /* Function Dependency Link */
> >> u16 driver_max_VFs; /* max num VFs driver supports */
> >> + atomic_t VFs_assigned_cnt; /* counter of VFs assigned to VM */
> >> struct pci_dev *dev; /* lowest numbered PF */
> >> struct pci_dev *self; /* this PF */
> >> struct mutex lock; /* lock for VF bus */
> >> diff --git a/include/linux/pci.h b/include/linux/pci.h
> >> index aab57b4..5cf6833 100644
> >> --- a/include/linux/pci.h
> >> +++ b/include/linux/pci.h
> >> @@ -1601,6 +1601,8 @@ int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
> >> void pci_disable_sriov(struct pci_dev *dev);
> >> int pci_num_vf(struct pci_dev *dev);
> >> int pci_vfs_assigned(struct pci_dev *dev);
> >> +void pci_sriov_assign_device(struct pci_dev *dev);
> >> +void pci_sriov_deassign_device(struct pci_dev *dev);
> >> int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
> >> int pci_sriov_get_totalvfs(struct pci_dev *dev);
> >> #else
> >> @@ -1610,6 +1612,8 @@ static inline void pci_disable_sriov(struct pci_dev *dev) { }
> >> static inline int pci_num_vf(struct pci_dev *dev) { return 0; }
> >> static inline int pci_vfs_assigned(struct pci_dev *dev)
> >> { return 0; }
> >> +static inline void pci_sriov_assign_device(struct pci_dev *dev) { }
> >> +static inline void pci_sriov_deassign_device(struct pci_dev *dev) { }
> >> static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
> >> { return 0; }
> >> static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
> >
> >
>
On 2014/7/11 10:22, Alex Williamson wrote:
> On Fri, 2014-07-11 at 10:10 +0800, ethan zhao wrote:
>> Alex,
>> Thanks for your reviewing, when I create the patch order, I thought
>> about the question you concerned for
>> quit a while, make every patch be independent to each other as possible
>> as I could, so we can do bisect when hit
>> problem.
>>
>> I manage to take more time to figure out better patch order.
>>
>> Thanks,
>> Ethan
>>
>> On 2014/7/11 9:48, Alex Williamson wrote:
>>> Since there's no 0th patch, I guess I'll comment here. This series is
>>> not bisectable, patch 1 breaks the existing implementation. I'd
>>> suggest:
>>>
>>> patch 1 - fix i40e
>> i40e only could be fixed with new interface, so it couldn't
>> be the first one.
> It looks like i40e just has it's own copy of pci_vfs_assigned(), why
> can't your current patch 4/4 be applied now?
Yes, i40e has its local copy of pci_vfs_assigned(),it could be
simplified .
with new interface,in another word, its a user of new interface.
Thanks,
Ethan
>
>>> patch 2 - create assign/deassign that uses dev_flags
>> This will be the first ?
>>> patch 3 - convert users to new interface
>> Have to be the later step.
>>> patch 4 - convert interface to use atomic_t
>> Could it be standalone step ?
>>
>> Let me think about it.
>>
>>> IMHO, the assigned flag is a horrible hack and I don't know why drivers
>>> like xenback need to use it. KVM needs to use it because it doesn't
>>> actually have a driver to bind to when a device is assigned, it's happy
>>> to assign devices without any driver. Thanks,
>>>
>>> Alex
>>>
>>>
>>> On Fri, 2014-07-11 at 08:47 +0800, Ethan Zhao wrote:
>>>> Current implementation of helper function pci_vfs_assigned() is a little complex, to
>>>> get sum of VFs that assigned to VM, access low level configuration space register and
>>>> then loop in traversing device tree.
>>>>
>>>> This patch introduce an atomic reference counter for VFs that assigned to VM in struct
>>>> pci_sriov, and compose two more helper functions
>>>>
>>>> pci_sriov_assign_device(),
>>>> pci_sriov_deassign_device()
>>>>
>>>> to replace manipulation to device flag and the meanwhile increase and decease the counter.
>>>>
>>>> Passed building on 3.15.5
>>>>
>>>> Signed-off-by: Ethan Zhao <[email protected]>
>>>> ---
>>>> drivers/pci/iov.c | 65 ++++++++++++++++++++++++++++----------------------
>>>> drivers/pci/pci.h | 1 +
>>>> include/linux/pci.h | 4 +++
>>>> 3 files changed, 41 insertions(+), 29 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>>>> index de7a747..72e267f 100644
>>>> --- a/drivers/pci/iov.c
>>>> +++ b/drivers/pci/iov.c
>>>> @@ -382,6 +382,7 @@ found:
>>>> iov->nres = nres;
>>>> iov->ctrl = ctrl;
>>>> iov->total_VFs = total;
>>>> + atomic_set(&iov->VFs_assigned_cnt, 0);
>>>> iov->offset = offset;
>>>> iov->stride = stride;
>>>> iov->pgsz = pgsz;
>>>> @@ -603,45 +604,51 @@ int pci_num_vf(struct pci_dev *dev)
>>>> EXPORT_SYMBOL_GPL(pci_num_vf);
>>>>
>>>> /**
>>>> - * pci_vfs_assigned - returns number of VFs are assigned to a guest
>>>> - * @dev: the PCI device
>>>> + * pci_vfs_assigned - returns number of VFs are assigned to VM
>>>> + * @dev: the physical PCI device that contains the VFs.
>>>> *
>>>> - * Returns number of VFs belonging to this device that are assigned to a guest.
>>>> + * Returns number of VFs belonging to this device that are assigned to VM.
>>>> * If device is not a physical function returns 0.
>>>> */
>>>> int pci_vfs_assigned(struct pci_dev *dev)
>>>> {
>>>> - struct pci_dev *vfdev;
>>>> - unsigned int vfs_assigned = 0;
>>>> - unsigned short dev_id;
>>>> -
>>>> - /* only search if we are a PF */
>>>> if (!dev->is_physfn)
>>>> return 0;
>>>> + if (dev->sriov)
>>>> + return atomic_read(&dev->sriov->VFs_assigned_cnt);
>>>> + return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(pci_vfs_assigned);
>>>>
>>>> - /*
>>>> - * determine the device ID for the VFs, the vendor ID will be the
>>>> - * same as the PF so there is no need to check for that one
>>>> - */
>>>> - pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_VF_DID, &dev_id);
>>>> -
>>>> - /* loop through all the VFs to see if we own any that are assigned */
>>>> - vfdev = pci_get_device(dev->vendor, dev_id, NULL);
>>>> - while (vfdev) {
>>>> - /*
>>>> - * It is considered assigned if it is a virtual function with
>>>> - * our dev as the physical function and the assigned bit is set
>>>> - */
>>>> - if (vfdev->is_virtfn && (vfdev->physfn == dev) &&
>>>> - (vfdev->dev_flags & PCI_DEV_FLAGS_ASSIGNED))
>>>> - vfs_assigned++;
>>>> -
>>>> - vfdev = pci_get_device(dev->vendor, dev_id, vfdev);
>>>> - }
>>>> +/**
>>>> + * pci_sriov_assign_device - assign device to VM
>>>> + * @pdev: the device to be assigned.
>>>> + */
>>>> +void pci_sriov_assign_device(struct pci_dev *pdev)
>>>> +{
>>>> + pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
>>>> + if (pdev->is_virtfn && !pdev->is_physfn)
>>>> + if (pdev->physfn)
>>>> + if (pdev->physfn->sriov)
>>>> + atomic_inc(&pdev->physfn->sriov->
>>>> + VFs_assigned_cnt);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(pci_sriov_assign_device);
>>>>
>>>> - return vfs_assigned;
>>>> +/**
>>>> + * pci_sriov_deassign_device - deasign device from VM
>>>> + * @pdev: the device to be deassigned.
>>>> + */
>>>> +void pci_sriov_deassign_device(struct pci_dev *pdev)
>>>> +{
>>>> + pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
>>>> + if (pdev->is_virtfn && !pdev->is_physfn)
>>>> + if (pdev->physfn)
>>>> + if (pdev->physfn->sriov)
>>>> + atomic_dec(&pdev->physfn->sriov->
>>>> + VFs_assigned_cnt);
>>>> }
>>>> -EXPORT_SYMBOL_GPL(pci_vfs_assigned);
>>>> +EXPORT_SYMBOL_GPL(pci_sriov_deassign_device);
>>>>
>>>> /**
>>>> * pci_sriov_set_totalvfs -- reduce the TotalVFs available
>>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>>>> index 6bd0822..d17bda2 100644
>>>> --- a/drivers/pci/pci.h
>>>> +++ b/drivers/pci/pci.h
>>>> @@ -235,6 +235,7 @@ struct pci_sriov {
>>>> u32 pgsz; /* page size for BAR alignment */
>>>> u8 link; /* Function Dependency Link */
>>>> u16 driver_max_VFs; /* max num VFs driver supports */
>>>> + atomic_t VFs_assigned_cnt; /* counter of VFs assigned to VM */
>>>> struct pci_dev *dev; /* lowest numbered PF */
>>>> struct pci_dev *self; /* this PF */
>>>> struct mutex lock; /* lock for VF bus */
>>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>>> index aab57b4..5cf6833 100644
>>>> --- a/include/linux/pci.h
>>>> +++ b/include/linux/pci.h
>>>> @@ -1601,6 +1601,8 @@ int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
>>>> void pci_disable_sriov(struct pci_dev *dev);
>>>> int pci_num_vf(struct pci_dev *dev);
>>>> int pci_vfs_assigned(struct pci_dev *dev);
>>>> +void pci_sriov_assign_device(struct pci_dev *dev);
>>>> +void pci_sriov_deassign_device(struct pci_dev *dev);
>>>> int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
>>>> int pci_sriov_get_totalvfs(struct pci_dev *dev);
>>>> #else
>>>> @@ -1610,6 +1612,8 @@ static inline void pci_disable_sriov(struct pci_dev *dev) { }
>>>> static inline int pci_num_vf(struct pci_dev *dev) { return 0; }
>>>> static inline int pci_vfs_assigned(struct pci_dev *dev)
>>>> { return 0; }
>>>> +static inline void pci_sriov_assign_device(struct pci_dev *dev) { }
>>>> +static inline void pci_sriov_deassign_device(struct pci_dev *dev) { }
>>>> static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
>>>> { return 0; }
>>>> static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
>>>
>
>
On Fri, 2014-07-11 at 10:29 +0800, ethan zhao wrote:
> On 2014/7/11 10:22, Alex Williamson wrote:
> > On Fri, 2014-07-11 at 10:10 +0800, ethan zhao wrote:
> >> Alex,
> >> Thanks for your reviewing, when I create the patch order, I thought
> >> about the question you concerned for
> >> quit a while, make every patch be independent to each other as possible
> >> as I could, so we can do bisect when hit
> >> problem.
> >>
> >> I manage to take more time to figure out better patch order.
> >>
> >> Thanks,
> >> Ethan
> >>
> >> On 2014/7/11 9:48, Alex Williamson wrote:
> >>> Since there's no 0th patch, I guess I'll comment here. This series is
> >>> not bisectable, patch 1 breaks the existing implementation. I'd
> >>> suggest:
> >>>
> >>> patch 1 - fix i40e
> >> i40e only could be fixed with new interface, so it couldn't
> >> be the first one.
> > It looks like i40e just has it's own copy of pci_vfs_assigned(), why
> > can't your current patch 4/4 be applied now?
> Yes, i40e has its local copy of pci_vfs_assigned(),it could be
> simplified .
> with new interface,in another word, its a user of new interface.
It's a user of the existing interface. You're missing the point,
abstract all the users of the assigned dev_flags first, then you're free
to fix the implementation of the interface in one shot without breaking
anything.
> >>> patch 2 - create assign/deassign that uses dev_flags
> >> This will be the first ?
> >>> patch 3 - convert users to new interface
> >> Have to be the later step.
> >>> patch 4 - convert interface to use atomic_t
> >> Could it be standalone step ?
> >>
> >> Let me think about it.
> >>
> >>> IMHO, the assigned flag is a horrible hack and I don't know why drivers
> >>> like xenback need to use it. KVM needs to use it because it doesn't
> >>> actually have a driver to bind to when a device is assigned, it's happy
> >>> to assign devices without any driver. Thanks,
> >>>
> >>> Alex
> >>>
> >>>
> >>> On Fri, 2014-07-11 at 08:47 +0800, Ethan Zhao wrote:
> >>>> Current implementation of helper function pci_vfs_assigned() is a little complex, to
> >>>> get sum of VFs that assigned to VM, access low level configuration space register and
> >>>> then loop in traversing device tree.
> >>>>
> >>>> This patch introduce an atomic reference counter for VFs that assigned to VM in struct
> >>>> pci_sriov, and compose two more helper functions
> >>>>
> >>>> pci_sriov_assign_device(),
> >>>> pci_sriov_deassign_device()
> >>>>
> >>>> to replace manipulation to device flag and the meanwhile increase and decease the counter.
> >>>>
> >>>> Passed building on 3.15.5
> >>>>
> >>>> Signed-off-by: Ethan Zhao <[email protected]>
> >>>> ---
> >>>> drivers/pci/iov.c | 65 ++++++++++++++++++++++++++++----------------------
> >>>> drivers/pci/pci.h | 1 +
> >>>> include/linux/pci.h | 4 +++
> >>>> 3 files changed, 41 insertions(+), 29 deletions(-)
> >>>>
> >>>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> >>>> index de7a747..72e267f 100644
> >>>> --- a/drivers/pci/iov.c
> >>>> +++ b/drivers/pci/iov.c
> >>>> @@ -382,6 +382,7 @@ found:
> >>>> iov->nres = nres;
> >>>> iov->ctrl = ctrl;
> >>>> iov->total_VFs = total;
> >>>> + atomic_set(&iov->VFs_assigned_cnt, 0);
> >>>> iov->offset = offset;
> >>>> iov->stride = stride;
> >>>> iov->pgsz = pgsz;
> >>>> @@ -603,45 +604,51 @@ int pci_num_vf(struct pci_dev *dev)
> >>>> EXPORT_SYMBOL_GPL(pci_num_vf);
> >>>>
> >>>> /**
> >>>> - * pci_vfs_assigned - returns number of VFs are assigned to a guest
> >>>> - * @dev: the PCI device
> >>>> + * pci_vfs_assigned - returns number of VFs are assigned to VM
> >>>> + * @dev: the physical PCI device that contains the VFs.
> >>>> *
> >>>> - * Returns number of VFs belonging to this device that are assigned to a guest.
> >>>> + * Returns number of VFs belonging to this device that are assigned to VM.
> >>>> * If device is not a physical function returns 0.
> >>>> */
> >>>> int pci_vfs_assigned(struct pci_dev *dev)
> >>>> {
> >>>> - struct pci_dev *vfdev;
> >>>> - unsigned int vfs_assigned = 0;
> >>>> - unsigned short dev_id;
> >>>> -
> >>>> - /* only search if we are a PF */
> >>>> if (!dev->is_physfn)
> >>>> return 0;
> >>>> + if (dev->sriov)
> >>>> + return atomic_read(&dev->sriov->VFs_assigned_cnt);
> >>>> + return 0;
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(pci_vfs_assigned);
> >>>>
> >>>> - /*
> >>>> - * determine the device ID for the VFs, the vendor ID will be the
> >>>> - * same as the PF so there is no need to check for that one
> >>>> - */
> >>>> - pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_VF_DID, &dev_id);
> >>>> -
> >>>> - /* loop through all the VFs to see if we own any that are assigned */
> >>>> - vfdev = pci_get_device(dev->vendor, dev_id, NULL);
> >>>> - while (vfdev) {
> >>>> - /*
> >>>> - * It is considered assigned if it is a virtual function with
> >>>> - * our dev as the physical function and the assigned bit is set
> >>>> - */
> >>>> - if (vfdev->is_virtfn && (vfdev->physfn == dev) &&
> >>>> - (vfdev->dev_flags & PCI_DEV_FLAGS_ASSIGNED))
> >>>> - vfs_assigned++;
> >>>> -
> >>>> - vfdev = pci_get_device(dev->vendor, dev_id, vfdev);
> >>>> - }
> >>>> +/**
> >>>> + * pci_sriov_assign_device - assign device to VM
> >>>> + * @pdev: the device to be assigned.
> >>>> + */
> >>>> +void pci_sriov_assign_device(struct pci_dev *pdev)
> >>>> +{
> >>>> + pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
> >>>> + if (pdev->is_virtfn && !pdev->is_physfn)
> >>>> + if (pdev->physfn)
> >>>> + if (pdev->physfn->sriov)
> >>>> + atomic_inc(&pdev->physfn->sriov->
> >>>> + VFs_assigned_cnt);
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(pci_sriov_assign_device);
> >>>>
> >>>> - return vfs_assigned;
> >>>> +/**
> >>>> + * pci_sriov_deassign_device - deasign device from VM
> >>>> + * @pdev: the device to be deassigned.
> >>>> + */
> >>>> +void pci_sriov_deassign_device(struct pci_dev *pdev)
> >>>> +{
> >>>> + pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
> >>>> + if (pdev->is_virtfn && !pdev->is_physfn)
> >>>> + if (pdev->physfn)
> >>>> + if (pdev->physfn->sriov)
> >>>> + atomic_dec(&pdev->physfn->sriov->
> >>>> + VFs_assigned_cnt);
> >>>> }
> >>>> -EXPORT_SYMBOL_GPL(pci_vfs_assigned);
> >>>> +EXPORT_SYMBOL_GPL(pci_sriov_deassign_device);
> >>>>
> >>>> /**
> >>>> * pci_sriov_set_totalvfs -- reduce the TotalVFs available
> >>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> >>>> index 6bd0822..d17bda2 100644
> >>>> --- a/drivers/pci/pci.h
> >>>> +++ b/drivers/pci/pci.h
> >>>> @@ -235,6 +235,7 @@ struct pci_sriov {
> >>>> u32 pgsz; /* page size for BAR alignment */
> >>>> u8 link; /* Function Dependency Link */
> >>>> u16 driver_max_VFs; /* max num VFs driver supports */
> >>>> + atomic_t VFs_assigned_cnt; /* counter of VFs assigned to VM */
> >>>> struct pci_dev *dev; /* lowest numbered PF */
> >>>> struct pci_dev *self; /* this PF */
> >>>> struct mutex lock; /* lock for VF bus */
> >>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
> >>>> index aab57b4..5cf6833 100644
> >>>> --- a/include/linux/pci.h
> >>>> +++ b/include/linux/pci.h
> >>>> @@ -1601,6 +1601,8 @@ int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
> >>>> void pci_disable_sriov(struct pci_dev *dev);
> >>>> int pci_num_vf(struct pci_dev *dev);
> >>>> int pci_vfs_assigned(struct pci_dev *dev);
> >>>> +void pci_sriov_assign_device(struct pci_dev *dev);
> >>>> +void pci_sriov_deassign_device(struct pci_dev *dev);
> >>>> int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
> >>>> int pci_sriov_get_totalvfs(struct pci_dev *dev);
> >>>> #else
> >>>> @@ -1610,6 +1612,8 @@ static inline void pci_disable_sriov(struct pci_dev *dev) { }
> >>>> static inline int pci_num_vf(struct pci_dev *dev) { return 0; }
> >>>> static inline int pci_vfs_assigned(struct pci_dev *dev)
> >>>> { return 0; }
> >>>> +static inline void pci_sriov_assign_device(struct pci_dev *dev) { }
> >>>> +static inline void pci_sriov_deassign_device(struct pci_dev *dev) { }
> >>>> static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
> >>>> { return 0; }
> >>>> static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
> >>>
> >
> >
>
On 2014/7/11 10:33, Alex Williamson wrote:
> On Fri, 2014-07-11 at 10:29 +0800, ethan zhao wrote:
>> On 2014/7/11 10:22, Alex Williamson wrote:
>>> On Fri, 2014-07-11 at 10:10 +0800, ethan zhao wrote:
>>>> Alex,
>>>> Thanks for your reviewing, when I create the patch order, I thought
>>>> about the question you concerned for
>>>> quit a while, make every patch be independent to each other as possible
>>>> as I could, so we can do bisect when hit
>>>> problem.
>>>>
>>>> I manage to take more time to figure out better patch order.
>>>>
>>>> Thanks,
>>>> Ethan
>>>>
>>>> On 2014/7/11 9:48, Alex Williamson wrote:
>>>>> Since there's no 0th patch, I guess I'll comment here. This series is
>>>>> not bisectable, patch 1 breaks the existing implementation. I'd
>>>>> suggest:
>>>>>
>>>>> patch 1 - fix i40e
>>>> i40e only could be fixed with new interface, so it couldn't
>>>> be the first one.
>>> It looks like i40e just has it's own copy of pci_vfs_assigned(), why
>>> can't your current patch 4/4 be applied now?
>> Yes, i40e has its local copy of pci_vfs_assigned(),it could be
>> simplified .
>> with new interface,in another word, its a user of new interface.
> It's a user of the existing interface. You're missing the point,
> abstract all the users of the assigned dev_flags first, then you're free
> to fix the implementation of the interface in one shot without breaking
> anything.
Great,you hit the center this time, agree !
>>>>> patch 2 - create assign/deassign that uses dev_flags
>>>> This will be the first ?
>>>>> patch 3 - convert users to new interface
>>>> Have to be the later step.
>>>>> patch 4 - convert interface to use atomic_t
>>>> Could it be standalone step ?
>>>>
>>>> Let me think about it.
>>>>
>>>>> IMHO, the assigned flag is a horrible hack and I don't know why drivers
>>>>> like xenback need to use it. KVM needs to use it because it doesn't
>>>>> actually have a driver to bind to when a device is assigned, it's happy
>>>>> to assign devices without any driver. Thanks,
>>>>>
>>>>> Alex
>>>>>
>>>>>
>>>>> On Fri, 2014-07-11 at 08:47 +0800, Ethan Zhao wrote:
>>>>>> Current implementation of helper function pci_vfs_assigned() is a little complex, to
>>>>>> get sum of VFs that assigned to VM, access low level configuration space register and
>>>>>> then loop in traversing device tree.
>>>>>>
>>>>>> This patch introduce an atomic reference counter for VFs that assigned to VM in struct
>>>>>> pci_sriov, and compose two more helper functions
>>>>>>
>>>>>> pci_sriov_assign_device(),
>>>>>> pci_sriov_deassign_device()
>>>>>>
>>>>>> to replace manipulation to device flag and the meanwhile increase and decease the counter.
>>>>>>
>>>>>> Passed building on 3.15.5
>>>>>>
>>>>>> Signed-off-by: Ethan Zhao <[email protected]>
>>>>>> ---
>>>>>> drivers/pci/iov.c | 65 ++++++++++++++++++++++++++++----------------------
>>>>>> drivers/pci/pci.h | 1 +
>>>>>> include/linux/pci.h | 4 +++
>>>>>> 3 files changed, 41 insertions(+), 29 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>>>>>> index de7a747..72e267f 100644
>>>>>> --- a/drivers/pci/iov.c
>>>>>> +++ b/drivers/pci/iov.c
>>>>>> @@ -382,6 +382,7 @@ found:
>>>>>> iov->nres = nres;
>>>>>> iov->ctrl = ctrl;
>>>>>> iov->total_VFs = total;
>>>>>> + atomic_set(&iov->VFs_assigned_cnt, 0);
>>>>>> iov->offset = offset;
>>>>>> iov->stride = stride;
>>>>>> iov->pgsz = pgsz;
>>>>>> @@ -603,45 +604,51 @@ int pci_num_vf(struct pci_dev *dev)
>>>>>> EXPORT_SYMBOL_GPL(pci_num_vf);
>>>>>>
>>>>>> /**
>>>>>> - * pci_vfs_assigned - returns number of VFs are assigned to a guest
>>>>>> - * @dev: the PCI device
>>>>>> + * pci_vfs_assigned - returns number of VFs are assigned to VM
>>>>>> + * @dev: the physical PCI device that contains the VFs.
>>>>>> *
>>>>>> - * Returns number of VFs belonging to this device that are assigned to a guest.
>>>>>> + * Returns number of VFs belonging to this device that are assigned to VM.
>>>>>> * If device is not a physical function returns 0.
>>>>>> */
>>>>>> int pci_vfs_assigned(struct pci_dev *dev)
>>>>>> {
>>>>>> - struct pci_dev *vfdev;
>>>>>> - unsigned int vfs_assigned = 0;
>>>>>> - unsigned short dev_id;
>>>>>> -
>>>>>> - /* only search if we are a PF */
>>>>>> if (!dev->is_physfn)
>>>>>> return 0;
>>>>>> + if (dev->sriov)
>>>>>> + return atomic_read(&dev->sriov->VFs_assigned_cnt);
>>>>>> + return 0;
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(pci_vfs_assigned);
>>>>>>
>>>>>> - /*
>>>>>> - * determine the device ID for the VFs, the vendor ID will be the
>>>>>> - * same as the PF so there is no need to check for that one
>>>>>> - */
>>>>>> - pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_VF_DID, &dev_id);
>>>>>> -
>>>>>> - /* loop through all the VFs to see if we own any that are assigned */
>>>>>> - vfdev = pci_get_device(dev->vendor, dev_id, NULL);
>>>>>> - while (vfdev) {
>>>>>> - /*
>>>>>> - * It is considered assigned if it is a virtual function with
>>>>>> - * our dev as the physical function and the assigned bit is set
>>>>>> - */
>>>>>> - if (vfdev->is_virtfn && (vfdev->physfn == dev) &&
>>>>>> - (vfdev->dev_flags & PCI_DEV_FLAGS_ASSIGNED))
>>>>>> - vfs_assigned++;
>>>>>> -
>>>>>> - vfdev = pci_get_device(dev->vendor, dev_id, vfdev);
>>>>>> - }
>>>>>> +/**
>>>>>> + * pci_sriov_assign_device - assign device to VM
>>>>>> + * @pdev: the device to be assigned.
>>>>>> + */
>>>>>> +void pci_sriov_assign_device(struct pci_dev *pdev)
>>>>>> +{
>>>>>> + pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
>>>>>> + if (pdev->is_virtfn && !pdev->is_physfn)
>>>>>> + if (pdev->physfn)
>>>>>> + if (pdev->physfn->sriov)
>>>>>> + atomic_inc(&pdev->physfn->sriov->
>>>>>> + VFs_assigned_cnt);
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(pci_sriov_assign_device);
>>>>>>
>>>>>> - return vfs_assigned;
>>>>>> +/**
>>>>>> + * pci_sriov_deassign_device - deasign device from VM
>>>>>> + * @pdev: the device to be deassigned.
>>>>>> + */
>>>>>> +void pci_sriov_deassign_device(struct pci_dev *pdev)
>>>>>> +{
>>>>>> + pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
>>>>>> + if (pdev->is_virtfn && !pdev->is_physfn)
>>>>>> + if (pdev->physfn)
>>>>>> + if (pdev->physfn->sriov)
>>>>>> + atomic_dec(&pdev->physfn->sriov->
>>>>>> + VFs_assigned_cnt);
>>>>>> }
>>>>>> -EXPORT_SYMBOL_GPL(pci_vfs_assigned);
>>>>>> +EXPORT_SYMBOL_GPL(pci_sriov_deassign_device);
>>>>>>
>>>>>> /**
>>>>>> * pci_sriov_set_totalvfs -- reduce the TotalVFs available
>>>>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>>>>>> index 6bd0822..d17bda2 100644
>>>>>> --- a/drivers/pci/pci.h
>>>>>> +++ b/drivers/pci/pci.h
>>>>>> @@ -235,6 +235,7 @@ struct pci_sriov {
>>>>>> u32 pgsz; /* page size for BAR alignment */
>>>>>> u8 link; /* Function Dependency Link */
>>>>>> u16 driver_max_VFs; /* max num VFs driver supports */
>>>>>> + atomic_t VFs_assigned_cnt; /* counter of VFs assigned to VM */
>>>>>> struct pci_dev *dev; /* lowest numbered PF */
>>>>>> struct pci_dev *self; /* this PF */
>>>>>> struct mutex lock; /* lock for VF bus */
>>>>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>>>>> index aab57b4..5cf6833 100644
>>>>>> --- a/include/linux/pci.h
>>>>>> +++ b/include/linux/pci.h
>>>>>> @@ -1601,6 +1601,8 @@ int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
>>>>>> void pci_disable_sriov(struct pci_dev *dev);
>>>>>> int pci_num_vf(struct pci_dev *dev);
>>>>>> int pci_vfs_assigned(struct pci_dev *dev);
>>>>>> +void pci_sriov_assign_device(struct pci_dev *dev);
>>>>>> +void pci_sriov_deassign_device(struct pci_dev *dev);
>>>>>> int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
>>>>>> int pci_sriov_get_totalvfs(struct pci_dev *dev);
>>>>>> #else
>>>>>> @@ -1610,6 +1612,8 @@ static inline void pci_disable_sriov(struct pci_dev *dev) { }
>>>>>> static inline int pci_num_vf(struct pci_dev *dev) { return 0; }
>>>>>> static inline int pci_vfs_assigned(struct pci_dev *dev)
>>>>>> { return 0; }
>>>>>> +static inline void pci_sriov_assign_device(struct pci_dev *dev) { }
>>>>>> +static inline void pci_sriov_deassign_device(struct pci_dev *dev) { }
>>>>>> static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
>>>>>> { return 0; }
>>>>>> static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
>>>
>
>