* [PATCH 2/4] xen-pciback: use PCI VFs assignment helper functions
2014-07-11 0:47 [PATCH 1/4] PCI: introduce VFs reference counter and simplify pci_vfs_assigned() implementation Ethan Zhao
@ 2014-07-11 0:47 ` Ethan Zhao
2014-07-11 0:47 ` [PATCH 3/4] KVM: " Ethan Zhao
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Ethan Zhao @ 2014-07-11 0:47 UTC (permalink / raw)
To: bhelgaas, konrad.wilk, boris.ostrovsky, david.vrabel, gleb,
pbonzini, jeffrey.t.kirsher, jesse.brandeburg, bruce.w.allan,
carolyn.wyborny, donald.c.skidmore, gregory.v.rose,
alexander.h.duyck, john.ronciak, mitch.a.williams
Cc: linux-pci, kvm, linux.nics, e1000-devel, netdev, linux-kernel,
ethan.kernel, vaughan.cao, Ethan Zhao
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 <ethan.zhao@oracle.com>
---
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 3/4] KVM: use PCI VFs assignment helper functions
2014-07-11 0:47 [PATCH 1/4] PCI: introduce VFs reference counter and simplify pci_vfs_assigned() implementation Ethan Zhao
2014-07-11 0:47 ` [PATCH 2/4] xen-pciback: use PCI VFs assignment helper functions Ethan Zhao
@ 2014-07-11 0:47 ` Ethan Zhao
2014-07-11 0:47 ` [PATCH 4/4] i40e: use PCI VFs assignment helper function simplify i40e_vfs_are_assigned() Ethan Zhao
2014-07-11 1:48 ` [PATCH 1/4] PCI: introduce VFs reference counter and simplify pci_vfs_assigned() implementation Alex Williamson
3 siblings, 0 replies; 10+ messages in thread
From: Ethan Zhao @ 2014-07-11 0:47 UTC (permalink / raw)
To: bhelgaas, konrad.wilk, boris.ostrovsky, david.vrabel, gleb,
pbonzini, jeffrey.t.kirsher, jesse.brandeburg, bruce.w.allan,
carolyn.wyborny, donald.c.skidmore, gregory.v.rose,
alexander.h.duyck, john.ronciak, mitch.a.williams
Cc: linux-pci, kvm, linux.nics, e1000-devel, netdev, linux-kernel,
ethan.kernel, vaughan.cao, Ethan Zhao
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 <ethan.zhao@oracle.com>
---
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4/4] i40e: use PCI VFs assignment helper function simplify i40e_vfs_are_assigned()
2014-07-11 0:47 [PATCH 1/4] PCI: introduce VFs reference counter and simplify pci_vfs_assigned() implementation Ethan Zhao
2014-07-11 0:47 ` [PATCH 2/4] xen-pciback: use PCI VFs assignment helper functions Ethan Zhao
2014-07-11 0:47 ` [PATCH 3/4] KVM: " Ethan Zhao
@ 2014-07-11 0:47 ` Ethan Zhao
2014-07-11 1:48 ` [PATCH 1/4] PCI: introduce VFs reference counter and simplify pci_vfs_assigned() implementation Alex Williamson
3 siblings, 0 replies; 10+ messages in thread
From: Ethan Zhao @ 2014-07-11 0:47 UTC (permalink / raw)
To: bhelgaas, konrad.wilk, boris.ostrovsky, david.vrabel, gleb,
pbonzini, jeffrey.t.kirsher, jesse.brandeburg, bruce.w.allan,
carolyn.wyborny, donald.c.skidmore, gregory.v.rose,
alexander.h.duyck, john.ronciak, mitch.a.williams
Cc: linux-pci, kvm, linux.nics, e1000-devel, netdev, linux-kernel,
ethan.kernel, vaughan.cao, Ethan Zhao
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 <ethan.zhao@oracle.com>
---
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 1/4] PCI: introduce VFs reference counter and simplify pci_vfs_assigned() implementation
2014-07-11 0:47 [PATCH 1/4] PCI: introduce VFs reference counter and simplify pci_vfs_assigned() implementation Ethan Zhao
` (2 preceding siblings ...)
2014-07-11 0:47 ` [PATCH 4/4] i40e: use PCI VFs assignment helper function simplify i40e_vfs_are_assigned() Ethan Zhao
@ 2014-07-11 1:48 ` Alex Williamson
2014-07-11 2:10 ` ethan zhao
3 siblings, 1 reply; 10+ messages in thread
From: Alex Williamson @ 2014-07-11 1:48 UTC (permalink / raw)
To: Ethan Zhao
Cc: bhelgaas, konrad.wilk, boris.ostrovsky, david.vrabel, gleb,
pbonzini, jeffrey.t.kirsher, jesse.brandeburg, bruce.w.allan,
carolyn.wyborny, donald.c.skidmore, gregory.v.rose,
alexander.h.duyck, john.ronciak, mitch.a.williams, linux-pci, kvm,
linux.nics, e1000-devel, netdev, linux-kernel, ethan.kernel,
vaughan.cao
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 <ethan.zhao@oracle.com>
> ---
> drivers/pci/iov.c | 65 ++++++++++++++++++++++++++++----------------------
> drivers/pci/pci.h | 1 +
> include/linux/pci.h | 4 +++
> 3 files changed, 41 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index de7a747..72e267f 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -382,6 +382,7 @@ found:
> iov->nres = nres;
> iov->ctrl = ctrl;
> iov->total_VFs = total;
> + atomic_set(&iov->VFs_assigned_cnt, 0);
> iov->offset = offset;
> iov->stride = stride;
> iov->pgsz = pgsz;
> @@ -603,45 +604,51 @@ int pci_num_vf(struct pci_dev *dev)
> EXPORT_SYMBOL_GPL(pci_num_vf);
>
> /**
> - * pci_vfs_assigned - returns number of VFs are assigned to a guest
> - * @dev: the PCI device
> + * pci_vfs_assigned - returns number of VFs are assigned to VM
> + * @dev: the physical PCI device that contains the VFs.
> *
> - * Returns number of VFs belonging to this device that are assigned to a guest.
> + * Returns number of VFs belonging to this device that are assigned to VM.
> * If device is not a physical function returns 0.
> */
> int pci_vfs_assigned(struct pci_dev *dev)
> {
> - struct pci_dev *vfdev;
> - unsigned int vfs_assigned = 0;
> - unsigned short dev_id;
> -
> - /* only search if we are a PF */
> if (!dev->is_physfn)
> return 0;
> + if (dev->sriov)
> + return atomic_read(&dev->sriov->VFs_assigned_cnt);
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(pci_vfs_assigned);
>
> - /*
> - * determine the device ID for the VFs, the vendor ID will be the
> - * same as the PF so there is no need to check for that one
> - */
> - pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_VF_DID, &dev_id);
> -
> - /* loop through all the VFs to see if we own any that are assigned */
> - vfdev = pci_get_device(dev->vendor, dev_id, NULL);
> - while (vfdev) {
> - /*
> - * It is considered assigned if it is a virtual function with
> - * our dev as the physical function and the assigned bit is set
> - */
> - if (vfdev->is_virtfn && (vfdev->physfn == dev) &&
> - (vfdev->dev_flags & PCI_DEV_FLAGS_ASSIGNED))
> - vfs_assigned++;
> -
> - vfdev = pci_get_device(dev->vendor, dev_id, vfdev);
> - }
> +/**
> + * pci_sriov_assign_device - assign device to VM
> + * @pdev: the device to be assigned.
> + */
> +void pci_sriov_assign_device(struct pci_dev *pdev)
> +{
> + pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
> + if (pdev->is_virtfn && !pdev->is_physfn)
> + if (pdev->physfn)
> + if (pdev->physfn->sriov)
> + atomic_inc(&pdev->physfn->sriov->
> + VFs_assigned_cnt);
> +}
> +EXPORT_SYMBOL_GPL(pci_sriov_assign_device);
>
> - return vfs_assigned;
> +/**
> + * pci_sriov_deassign_device - deasign device from VM
> + * @pdev: the device to be deassigned.
> + */
> +void pci_sriov_deassign_device(struct pci_dev *pdev)
> +{
> + pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
> + if (pdev->is_virtfn && !pdev->is_physfn)
> + if (pdev->physfn)
> + if (pdev->physfn->sriov)
> + atomic_dec(&pdev->physfn->sriov->
> + VFs_assigned_cnt);
> }
> -EXPORT_SYMBOL_GPL(pci_vfs_assigned);
> +EXPORT_SYMBOL_GPL(pci_sriov_deassign_device);
>
> /**
> * pci_sriov_set_totalvfs -- reduce the TotalVFs available
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 6bd0822..d17bda2 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -235,6 +235,7 @@ struct pci_sriov {
> u32 pgsz; /* page size for BAR alignment */
> u8 link; /* Function Dependency Link */
> u16 driver_max_VFs; /* max num VFs driver supports */
> + atomic_t VFs_assigned_cnt; /* counter of VFs assigned to VM */
> struct pci_dev *dev; /* lowest numbered PF */
> struct pci_dev *self; /* this PF */
> struct mutex lock; /* lock for VF bus */
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index aab57b4..5cf6833 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1601,6 +1601,8 @@ int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
> void pci_disable_sriov(struct pci_dev *dev);
> int pci_num_vf(struct pci_dev *dev);
> int pci_vfs_assigned(struct pci_dev *dev);
> +void pci_sriov_assign_device(struct pci_dev *dev);
> +void pci_sriov_deassign_device(struct pci_dev *dev);
> int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
> int pci_sriov_get_totalvfs(struct pci_dev *dev);
> #else
> @@ -1610,6 +1612,8 @@ static inline void pci_disable_sriov(struct pci_dev *dev) { }
> static inline int pci_num_vf(struct pci_dev *dev) { return 0; }
> static inline int pci_vfs_assigned(struct pci_dev *dev)
> { return 0; }
> +static inline void pci_sriov_assign_device(struct pci_dev *dev) { }
> +static inline void pci_sriov_deassign_device(struct pci_dev *dev) { }
> static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
> { return 0; }
> static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/4] PCI: introduce VFs reference counter and simplify pci_vfs_assigned() implementation
2014-07-11 1:48 ` [PATCH 1/4] PCI: introduce VFs reference counter and simplify pci_vfs_assigned() implementation Alex Williamson
@ 2014-07-11 2:10 ` ethan zhao
2014-07-11 2:22 ` Alex Williamson
0 siblings, 1 reply; 10+ messages in thread
From: ethan zhao @ 2014-07-11 2:10 UTC (permalink / raw)
To: Alex Williamson
Cc: bhelgaas, konrad.wilk, boris.ostrovsky, david.vrabel, gleb,
pbonzini, jeffrey.t.kirsher, jesse.brandeburg, bruce.w.allan,
carolyn.wyborny, donald.c.skidmore, gregory.v.rose,
alexander.h.duyck, john.ronciak, mitch.a.williams, linux-pci, kvm,
linux.nics, e1000-devel, netdev, linux-kernel, ethan.kernel,
vaughan.cao
Alex,
Thanks for your reviewing, when I create the patch order, I thought
about the question you concerned for
quit a while, make every patch be independent to each other as possible
as I could, so we can do bisect when hit
problem.
I manage to take more time to figure out better patch order.
Thanks,
Ethan
On 2014/7/11 9:48, Alex Williamson wrote:
> Since there's no 0th patch, I guess I'll comment here. This series is
> not bisectable, patch 1 breaks the existing implementation. I'd
> suggest:
>
> patch 1 - fix i40e
i40e only could be fixed with new interface, so it couldn't
be the first one.
> patch 2 - create assign/deassign that uses dev_flags
This will be the first ?
> patch 3 - convert users to new interface
Have to be the later step.
> patch 4 - convert interface to use atomic_t
Could it be standalone step ?
Let me think about it.
> IMHO, the assigned flag is a horrible hack and I don't know why drivers
> like xenback need to use it. KVM needs to use it because it doesn't
> actually have a driver to bind to when a device is assigned, it's happy
> to assign devices without any driver. Thanks,
>
> Alex
>
>
> On Fri, 2014-07-11 at 08:47 +0800, Ethan Zhao wrote:
>> Current implementation of helper function pci_vfs_assigned() is a little complex, to
>> get sum of VFs that assigned to VM, access low level configuration space register and
>> then loop in traversing device tree.
>>
>> This patch introduce an atomic reference counter for VFs that assigned to VM in struct
>> pci_sriov, and compose two more helper functions
>>
>> pci_sriov_assign_device(),
>> pci_sriov_deassign_device()
>>
>> to replace manipulation to device flag and the meanwhile increase and decease the counter.
>>
>> Passed building on 3.15.5
>>
>> Signed-off-by: Ethan Zhao <ethan.zhao@oracle.com>
>> ---
>> drivers/pci/iov.c | 65 ++++++++++++++++++++++++++++----------------------
>> drivers/pci/pci.h | 1 +
>> include/linux/pci.h | 4 +++
>> 3 files changed, 41 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>> index de7a747..72e267f 100644
>> --- a/drivers/pci/iov.c
>> +++ b/drivers/pci/iov.c
>> @@ -382,6 +382,7 @@ found:
>> iov->nres = nres;
>> iov->ctrl = ctrl;
>> iov->total_VFs = total;
>> + atomic_set(&iov->VFs_assigned_cnt, 0);
>> iov->offset = offset;
>> iov->stride = stride;
>> iov->pgsz = pgsz;
>> @@ -603,45 +604,51 @@ int pci_num_vf(struct pci_dev *dev)
>> EXPORT_SYMBOL_GPL(pci_num_vf);
>>
>> /**
>> - * pci_vfs_assigned - returns number of VFs are assigned to a guest
>> - * @dev: the PCI device
>> + * pci_vfs_assigned - returns number of VFs are assigned to VM
>> + * @dev: the physical PCI device that contains the VFs.
>> *
>> - * Returns number of VFs belonging to this device that are assigned to a guest.
>> + * Returns number of VFs belonging to this device that are assigned to VM.
>> * If device is not a physical function returns 0.
>> */
>> int pci_vfs_assigned(struct pci_dev *dev)
>> {
>> - struct pci_dev *vfdev;
>> - unsigned int vfs_assigned = 0;
>> - unsigned short dev_id;
>> -
>> - /* only search if we are a PF */
>> if (!dev->is_physfn)
>> return 0;
>> + if (dev->sriov)
>> + return atomic_read(&dev->sriov->VFs_assigned_cnt);
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(pci_vfs_assigned);
>>
>> - /*
>> - * determine the device ID for the VFs, the vendor ID will be the
>> - * same as the PF so there is no need to check for that one
>> - */
>> - pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_VF_DID, &dev_id);
>> -
>> - /* loop through all the VFs to see if we own any that are assigned */
>> - vfdev = pci_get_device(dev->vendor, dev_id, NULL);
>> - while (vfdev) {
>> - /*
>> - * It is considered assigned if it is a virtual function with
>> - * our dev as the physical function and the assigned bit is set
>> - */
>> - if (vfdev->is_virtfn && (vfdev->physfn == dev) &&
>> - (vfdev->dev_flags & PCI_DEV_FLAGS_ASSIGNED))
>> - vfs_assigned++;
>> -
>> - vfdev = pci_get_device(dev->vendor, dev_id, vfdev);
>> - }
>> +/**
>> + * pci_sriov_assign_device - assign device to VM
>> + * @pdev: the device to be assigned.
>> + */
>> +void pci_sriov_assign_device(struct pci_dev *pdev)
>> +{
>> + pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
>> + if (pdev->is_virtfn && !pdev->is_physfn)
>> + if (pdev->physfn)
>> + if (pdev->physfn->sriov)
>> + atomic_inc(&pdev->physfn->sriov->
>> + VFs_assigned_cnt);
>> +}
>> +EXPORT_SYMBOL_GPL(pci_sriov_assign_device);
>>
>> - return vfs_assigned;
>> +/**
>> + * pci_sriov_deassign_device - deasign device from VM
>> + * @pdev: the device to be deassigned.
>> + */
>> +void pci_sriov_deassign_device(struct pci_dev *pdev)
>> +{
>> + pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
>> + if (pdev->is_virtfn && !pdev->is_physfn)
>> + if (pdev->physfn)
>> + if (pdev->physfn->sriov)
>> + atomic_dec(&pdev->physfn->sriov->
>> + VFs_assigned_cnt);
>> }
>> -EXPORT_SYMBOL_GPL(pci_vfs_assigned);
>> +EXPORT_SYMBOL_GPL(pci_sriov_deassign_device);
>>
>> /**
>> * pci_sriov_set_totalvfs -- reduce the TotalVFs available
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 6bd0822..d17bda2 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -235,6 +235,7 @@ struct pci_sriov {
>> u32 pgsz; /* page size for BAR alignment */
>> u8 link; /* Function Dependency Link */
>> u16 driver_max_VFs; /* max num VFs driver supports */
>> + atomic_t VFs_assigned_cnt; /* counter of VFs assigned to VM */
>> struct pci_dev *dev; /* lowest numbered PF */
>> struct pci_dev *self; /* this PF */
>> struct mutex lock; /* lock for VF bus */
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index aab57b4..5cf6833 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -1601,6 +1601,8 @@ int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
>> void pci_disable_sriov(struct pci_dev *dev);
>> int pci_num_vf(struct pci_dev *dev);
>> int pci_vfs_assigned(struct pci_dev *dev);
>> +void pci_sriov_assign_device(struct pci_dev *dev);
>> +void pci_sriov_deassign_device(struct pci_dev *dev);
>> int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
>> int pci_sriov_get_totalvfs(struct pci_dev *dev);
>> #else
>> @@ -1610,6 +1612,8 @@ static inline void pci_disable_sriov(struct pci_dev *dev) { }
>> static inline int pci_num_vf(struct pci_dev *dev) { return 0; }
>> static inline int pci_vfs_assigned(struct pci_dev *dev)
>> { return 0; }
>> +static inline void pci_sriov_assign_device(struct pci_dev *dev) { }
>> +static inline void pci_sriov_deassign_device(struct pci_dev *dev) { }
>> static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
>> { return 0; }
>> static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/4] PCI: introduce VFs reference counter and simplify pci_vfs_assigned() implementation
2014-07-11 2:10 ` ethan zhao
@ 2014-07-11 2:22 ` Alex Williamson
2014-07-11 2:29 ` ethan zhao
0 siblings, 1 reply; 10+ messages in thread
From: Alex Williamson @ 2014-07-11 2:22 UTC (permalink / raw)
To: ethan zhao
Cc: bhelgaas, konrad.wilk, boris.ostrovsky, david.vrabel, gleb,
pbonzini, jeffrey.t.kirsher, jesse.brandeburg, bruce.w.allan,
carolyn.wyborny, donald.c.skidmore, gregory.v.rose,
alexander.h.duyck, john.ronciak, mitch.a.williams, linux-pci, kvm,
linux.nics, e1000-devel, netdev, linux-kernel, ethan.kernel,
vaughan.cao
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 <ethan.zhao@oracle.com>
> >> ---
> >> drivers/pci/iov.c | 65 ++++++++++++++++++++++++++++----------------------
> >> drivers/pci/pci.h | 1 +
> >> include/linux/pci.h | 4 +++
> >> 3 files changed, 41 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> >> index de7a747..72e267f 100644
> >> --- a/drivers/pci/iov.c
> >> +++ b/drivers/pci/iov.c
> >> @@ -382,6 +382,7 @@ found:
> >> iov->nres = nres;
> >> iov->ctrl = ctrl;
> >> iov->total_VFs = total;
> >> + atomic_set(&iov->VFs_assigned_cnt, 0);
> >> iov->offset = offset;
> >> iov->stride = stride;
> >> iov->pgsz = pgsz;
> >> @@ -603,45 +604,51 @@ int pci_num_vf(struct pci_dev *dev)
> >> EXPORT_SYMBOL_GPL(pci_num_vf);
> >>
> >> /**
> >> - * pci_vfs_assigned - returns number of VFs are assigned to a guest
> >> - * @dev: the PCI device
> >> + * pci_vfs_assigned - returns number of VFs are assigned to VM
> >> + * @dev: the physical PCI device that contains the VFs.
> >> *
> >> - * Returns number of VFs belonging to this device that are assigned to a guest.
> >> + * Returns number of VFs belonging to this device that are assigned to VM.
> >> * If device is not a physical function returns 0.
> >> */
> >> int pci_vfs_assigned(struct pci_dev *dev)
> >> {
> >> - struct pci_dev *vfdev;
> >> - unsigned int vfs_assigned = 0;
> >> - unsigned short dev_id;
> >> -
> >> - /* only search if we are a PF */
> >> if (!dev->is_physfn)
> >> return 0;
> >> + if (dev->sriov)
> >> + return atomic_read(&dev->sriov->VFs_assigned_cnt);
> >> + return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(pci_vfs_assigned);
> >>
> >> - /*
> >> - * determine the device ID for the VFs, the vendor ID will be the
> >> - * same as the PF so there is no need to check for that one
> >> - */
> >> - pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_VF_DID, &dev_id);
> >> -
> >> - /* loop through all the VFs to see if we own any that are assigned */
> >> - vfdev = pci_get_device(dev->vendor, dev_id, NULL);
> >> - while (vfdev) {
> >> - /*
> >> - * It is considered assigned if it is a virtual function with
> >> - * our dev as the physical function and the assigned bit is set
> >> - */
> >> - if (vfdev->is_virtfn && (vfdev->physfn == dev) &&
> >> - (vfdev->dev_flags & PCI_DEV_FLAGS_ASSIGNED))
> >> - vfs_assigned++;
> >> -
> >> - vfdev = pci_get_device(dev->vendor, dev_id, vfdev);
> >> - }
> >> +/**
> >> + * pci_sriov_assign_device - assign device to VM
> >> + * @pdev: the device to be assigned.
> >> + */
> >> +void pci_sriov_assign_device(struct pci_dev *pdev)
> >> +{
> >> + pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
> >> + if (pdev->is_virtfn && !pdev->is_physfn)
> >> + if (pdev->physfn)
> >> + if (pdev->physfn->sriov)
> >> + atomic_inc(&pdev->physfn->sriov->
> >> + VFs_assigned_cnt);
> >> +}
> >> +EXPORT_SYMBOL_GPL(pci_sriov_assign_device);
> >>
> >> - return vfs_assigned;
> >> +/**
> >> + * pci_sriov_deassign_device - deasign device from VM
> >> + * @pdev: the device to be deassigned.
> >> + */
> >> +void pci_sriov_deassign_device(struct pci_dev *pdev)
> >> +{
> >> + pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
> >> + if (pdev->is_virtfn && !pdev->is_physfn)
> >> + if (pdev->physfn)
> >> + if (pdev->physfn->sriov)
> >> + atomic_dec(&pdev->physfn->sriov->
> >> + VFs_assigned_cnt);
> >> }
> >> -EXPORT_SYMBOL_GPL(pci_vfs_assigned);
> >> +EXPORT_SYMBOL_GPL(pci_sriov_deassign_device);
> >>
> >> /**
> >> * pci_sriov_set_totalvfs -- reduce the TotalVFs available
> >> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> >> index 6bd0822..d17bda2 100644
> >> --- a/drivers/pci/pci.h
> >> +++ b/drivers/pci/pci.h
> >> @@ -235,6 +235,7 @@ struct pci_sriov {
> >> u32 pgsz; /* page size for BAR alignment */
> >> u8 link; /* Function Dependency Link */
> >> u16 driver_max_VFs; /* max num VFs driver supports */
> >> + atomic_t VFs_assigned_cnt; /* counter of VFs assigned to VM */
> >> struct pci_dev *dev; /* lowest numbered PF */
> >> struct pci_dev *self; /* this PF */
> >> struct mutex lock; /* lock for VF bus */
> >> diff --git a/include/linux/pci.h b/include/linux/pci.h
> >> index aab57b4..5cf6833 100644
> >> --- a/include/linux/pci.h
> >> +++ b/include/linux/pci.h
> >> @@ -1601,6 +1601,8 @@ int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
> >> void pci_disable_sriov(struct pci_dev *dev);
> >> int pci_num_vf(struct pci_dev *dev);
> >> int pci_vfs_assigned(struct pci_dev *dev);
> >> +void pci_sriov_assign_device(struct pci_dev *dev);
> >> +void pci_sriov_deassign_device(struct pci_dev *dev);
> >> int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
> >> int pci_sriov_get_totalvfs(struct pci_dev *dev);
> >> #else
> >> @@ -1610,6 +1612,8 @@ static inline void pci_disable_sriov(struct pci_dev *dev) { }
> >> static inline int pci_num_vf(struct pci_dev *dev) { return 0; }
> >> static inline int pci_vfs_assigned(struct pci_dev *dev)
> >> { return 0; }
> >> +static inline void pci_sriov_assign_device(struct pci_dev *dev) { }
> >> +static inline void pci_sriov_deassign_device(struct pci_dev *dev) { }
> >> static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
> >> { return 0; }
> >> static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
> >
> >
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/4] PCI: introduce VFs reference counter and simplify pci_vfs_assigned() implementation
2014-07-11 2:22 ` Alex Williamson
@ 2014-07-11 2:29 ` ethan zhao
2014-07-11 2:33 ` Alex Williamson
0 siblings, 1 reply; 10+ messages in thread
From: ethan zhao @ 2014-07-11 2:29 UTC (permalink / raw)
To: Alex Williamson
Cc: bhelgaas, konrad.wilk, boris.ostrovsky, david.vrabel, gleb,
pbonzini, jeffrey.t.kirsher, jesse.brandeburg, bruce.w.allan,
carolyn.wyborny, donald.c.skidmore, gregory.v.rose,
alexander.h.duyck, john.ronciak, mitch.a.williams, linux-pci, kvm,
linux.nics, e1000-devel, netdev, linux-kernel, ethan.kernel,
vaughan.cao
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 <ethan.zhao@oracle.com>
>>>> ---
>>>> drivers/pci/iov.c | 65 ++++++++++++++++++++++++++++----------------------
>>>> drivers/pci/pci.h | 1 +
>>>> include/linux/pci.h | 4 +++
>>>> 3 files changed, 41 insertions(+), 29 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>>>> index de7a747..72e267f 100644
>>>> --- a/drivers/pci/iov.c
>>>> +++ b/drivers/pci/iov.c
>>>> @@ -382,6 +382,7 @@ found:
>>>> iov->nres = nres;
>>>> iov->ctrl = ctrl;
>>>> iov->total_VFs = total;
>>>> + atomic_set(&iov->VFs_assigned_cnt, 0);
>>>> iov->offset = offset;
>>>> iov->stride = stride;
>>>> iov->pgsz = pgsz;
>>>> @@ -603,45 +604,51 @@ int pci_num_vf(struct pci_dev *dev)
>>>> EXPORT_SYMBOL_GPL(pci_num_vf);
>>>>
>>>> /**
>>>> - * pci_vfs_assigned - returns number of VFs are assigned to a guest
>>>> - * @dev: the PCI device
>>>> + * pci_vfs_assigned - returns number of VFs are assigned to VM
>>>> + * @dev: the physical PCI device that contains the VFs.
>>>> *
>>>> - * Returns number of VFs belonging to this device that are assigned to a guest.
>>>> + * Returns number of VFs belonging to this device that are assigned to VM.
>>>> * If device is not a physical function returns 0.
>>>> */
>>>> int pci_vfs_assigned(struct pci_dev *dev)
>>>> {
>>>> - struct pci_dev *vfdev;
>>>> - unsigned int vfs_assigned = 0;
>>>> - unsigned short dev_id;
>>>> -
>>>> - /* only search if we are a PF */
>>>> if (!dev->is_physfn)
>>>> return 0;
>>>> + if (dev->sriov)
>>>> + return atomic_read(&dev->sriov->VFs_assigned_cnt);
>>>> + return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(pci_vfs_assigned);
>>>>
>>>> - /*
>>>> - * determine the device ID for the VFs, the vendor ID will be the
>>>> - * same as the PF so there is no need to check for that one
>>>> - */
>>>> - pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_VF_DID, &dev_id);
>>>> -
>>>> - /* loop through all the VFs to see if we own any that are assigned */
>>>> - vfdev = pci_get_device(dev->vendor, dev_id, NULL);
>>>> - while (vfdev) {
>>>> - /*
>>>> - * It is considered assigned if it is a virtual function with
>>>> - * our dev as the physical function and the assigned bit is set
>>>> - */
>>>> - if (vfdev->is_virtfn && (vfdev->physfn == dev) &&
>>>> - (vfdev->dev_flags & PCI_DEV_FLAGS_ASSIGNED))
>>>> - vfs_assigned++;
>>>> -
>>>> - vfdev = pci_get_device(dev->vendor, dev_id, vfdev);
>>>> - }
>>>> +/**
>>>> + * pci_sriov_assign_device - assign device to VM
>>>> + * @pdev: the device to be assigned.
>>>> + */
>>>> +void pci_sriov_assign_device(struct pci_dev *pdev)
>>>> +{
>>>> + pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
>>>> + if (pdev->is_virtfn && !pdev->is_physfn)
>>>> + if (pdev->physfn)
>>>> + if (pdev->physfn->sriov)
>>>> + atomic_inc(&pdev->physfn->sriov->
>>>> + VFs_assigned_cnt);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(pci_sriov_assign_device);
>>>>
>>>> - return vfs_assigned;
>>>> +/**
>>>> + * pci_sriov_deassign_device - deasign device from VM
>>>> + * @pdev: the device to be deassigned.
>>>> + */
>>>> +void pci_sriov_deassign_device(struct pci_dev *pdev)
>>>> +{
>>>> + pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
>>>> + if (pdev->is_virtfn && !pdev->is_physfn)
>>>> + if (pdev->physfn)
>>>> + if (pdev->physfn->sriov)
>>>> + atomic_dec(&pdev->physfn->sriov->
>>>> + VFs_assigned_cnt);
>>>> }
>>>> -EXPORT_SYMBOL_GPL(pci_vfs_assigned);
>>>> +EXPORT_SYMBOL_GPL(pci_sriov_deassign_device);
>>>>
>>>> /**
>>>> * pci_sriov_set_totalvfs -- reduce the TotalVFs available
>>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>>>> index 6bd0822..d17bda2 100644
>>>> --- a/drivers/pci/pci.h
>>>> +++ b/drivers/pci/pci.h
>>>> @@ -235,6 +235,7 @@ struct pci_sriov {
>>>> u32 pgsz; /* page size for BAR alignment */
>>>> u8 link; /* Function Dependency Link */
>>>> u16 driver_max_VFs; /* max num VFs driver supports */
>>>> + atomic_t VFs_assigned_cnt; /* counter of VFs assigned to VM */
>>>> struct pci_dev *dev; /* lowest numbered PF */
>>>> struct pci_dev *self; /* this PF */
>>>> struct mutex lock; /* lock for VF bus */
>>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>>> index aab57b4..5cf6833 100644
>>>> --- a/include/linux/pci.h
>>>> +++ b/include/linux/pci.h
>>>> @@ -1601,6 +1601,8 @@ int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
>>>> void pci_disable_sriov(struct pci_dev *dev);
>>>> int pci_num_vf(struct pci_dev *dev);
>>>> int pci_vfs_assigned(struct pci_dev *dev);
>>>> +void pci_sriov_assign_device(struct pci_dev *dev);
>>>> +void pci_sriov_deassign_device(struct pci_dev *dev);
>>>> int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
>>>> int pci_sriov_get_totalvfs(struct pci_dev *dev);
>>>> #else
>>>> @@ -1610,6 +1612,8 @@ static inline void pci_disable_sriov(struct pci_dev *dev) { }
>>>> static inline int pci_num_vf(struct pci_dev *dev) { return 0; }
>>>> static inline int pci_vfs_assigned(struct pci_dev *dev)
>>>> { return 0; }
>>>> +static inline void pci_sriov_assign_device(struct pci_dev *dev) { }
>>>> +static inline void pci_sriov_deassign_device(struct pci_dev *dev) { }
>>>> static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
>>>> { return 0; }
>>>> static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
>>>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/4] PCI: introduce VFs reference counter and simplify pci_vfs_assigned() implementation
2014-07-11 2:29 ` ethan zhao
@ 2014-07-11 2:33 ` Alex Williamson
2014-07-11 3:11 ` ethan zhao
0 siblings, 1 reply; 10+ messages in thread
From: Alex Williamson @ 2014-07-11 2:33 UTC (permalink / raw)
To: ethan zhao
Cc: bhelgaas, konrad.wilk, boris.ostrovsky, david.vrabel, gleb,
pbonzini, jeffrey.t.kirsher, jesse.brandeburg, bruce.w.allan,
carolyn.wyborny, donald.c.skidmore, gregory.v.rose,
alexander.h.duyck, john.ronciak, mitch.a.williams, linux-pci, kvm,
linux.nics, e1000-devel, netdev, linux-kernel, ethan.kernel,
vaughan.cao
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 <ethan.zhao@oracle.com>
> >>>> ---
> >>>> drivers/pci/iov.c | 65 ++++++++++++++++++++++++++++----------------------
> >>>> drivers/pci/pci.h | 1 +
> >>>> include/linux/pci.h | 4 +++
> >>>> 3 files changed, 41 insertions(+), 29 deletions(-)
> >>>>
> >>>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> >>>> index de7a747..72e267f 100644
> >>>> --- a/drivers/pci/iov.c
> >>>> +++ b/drivers/pci/iov.c
> >>>> @@ -382,6 +382,7 @@ found:
> >>>> iov->nres = nres;
> >>>> iov->ctrl = ctrl;
> >>>> iov->total_VFs = total;
> >>>> + atomic_set(&iov->VFs_assigned_cnt, 0);
> >>>> iov->offset = offset;
> >>>> iov->stride = stride;
> >>>> iov->pgsz = pgsz;
> >>>> @@ -603,45 +604,51 @@ int pci_num_vf(struct pci_dev *dev)
> >>>> EXPORT_SYMBOL_GPL(pci_num_vf);
> >>>>
> >>>> /**
> >>>> - * pci_vfs_assigned - returns number of VFs are assigned to a guest
> >>>> - * @dev: the PCI device
> >>>> + * pci_vfs_assigned - returns number of VFs are assigned to VM
> >>>> + * @dev: the physical PCI device that contains the VFs.
> >>>> *
> >>>> - * Returns number of VFs belonging to this device that are assigned to a guest.
> >>>> + * Returns number of VFs belonging to this device that are assigned to VM.
> >>>> * If device is not a physical function returns 0.
> >>>> */
> >>>> int pci_vfs_assigned(struct pci_dev *dev)
> >>>> {
> >>>> - struct pci_dev *vfdev;
> >>>> - unsigned int vfs_assigned = 0;
> >>>> - unsigned short dev_id;
> >>>> -
> >>>> - /* only search if we are a PF */
> >>>> if (!dev->is_physfn)
> >>>> return 0;
> >>>> + if (dev->sriov)
> >>>> + return atomic_read(&dev->sriov->VFs_assigned_cnt);
> >>>> + return 0;
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(pci_vfs_assigned);
> >>>>
> >>>> - /*
> >>>> - * determine the device ID for the VFs, the vendor ID will be the
> >>>> - * same as the PF so there is no need to check for that one
> >>>> - */
> >>>> - pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_VF_DID, &dev_id);
> >>>> -
> >>>> - /* loop through all the VFs to see if we own any that are assigned */
> >>>> - vfdev = pci_get_device(dev->vendor, dev_id, NULL);
> >>>> - while (vfdev) {
> >>>> - /*
> >>>> - * It is considered assigned if it is a virtual function with
> >>>> - * our dev as the physical function and the assigned bit is set
> >>>> - */
> >>>> - if (vfdev->is_virtfn && (vfdev->physfn == dev) &&
> >>>> - (vfdev->dev_flags & PCI_DEV_FLAGS_ASSIGNED))
> >>>> - vfs_assigned++;
> >>>> -
> >>>> - vfdev = pci_get_device(dev->vendor, dev_id, vfdev);
> >>>> - }
> >>>> +/**
> >>>> + * pci_sriov_assign_device - assign device to VM
> >>>> + * @pdev: the device to be assigned.
> >>>> + */
> >>>> +void pci_sriov_assign_device(struct pci_dev *pdev)
> >>>> +{
> >>>> + pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
> >>>> + if (pdev->is_virtfn && !pdev->is_physfn)
> >>>> + if (pdev->physfn)
> >>>> + if (pdev->physfn->sriov)
> >>>> + atomic_inc(&pdev->physfn->sriov->
> >>>> + VFs_assigned_cnt);
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(pci_sriov_assign_device);
> >>>>
> >>>> - return vfs_assigned;
> >>>> +/**
> >>>> + * pci_sriov_deassign_device - deasign device from VM
> >>>> + * @pdev: the device to be deassigned.
> >>>> + */
> >>>> +void pci_sriov_deassign_device(struct pci_dev *pdev)
> >>>> +{
> >>>> + pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
> >>>> + if (pdev->is_virtfn && !pdev->is_physfn)
> >>>> + if (pdev->physfn)
> >>>> + if (pdev->physfn->sriov)
> >>>> + atomic_dec(&pdev->physfn->sriov->
> >>>> + VFs_assigned_cnt);
> >>>> }
> >>>> -EXPORT_SYMBOL_GPL(pci_vfs_assigned);
> >>>> +EXPORT_SYMBOL_GPL(pci_sriov_deassign_device);
> >>>>
> >>>> /**
> >>>> * pci_sriov_set_totalvfs -- reduce the TotalVFs available
> >>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> >>>> index 6bd0822..d17bda2 100644
> >>>> --- a/drivers/pci/pci.h
> >>>> +++ b/drivers/pci/pci.h
> >>>> @@ -235,6 +235,7 @@ struct pci_sriov {
> >>>> u32 pgsz; /* page size for BAR alignment */
> >>>> u8 link; /* Function Dependency Link */
> >>>> u16 driver_max_VFs; /* max num VFs driver supports */
> >>>> + atomic_t VFs_assigned_cnt; /* counter of VFs assigned to VM */
> >>>> struct pci_dev *dev; /* lowest numbered PF */
> >>>> struct pci_dev *self; /* this PF */
> >>>> struct mutex lock; /* lock for VF bus */
> >>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
> >>>> index aab57b4..5cf6833 100644
> >>>> --- a/include/linux/pci.h
> >>>> +++ b/include/linux/pci.h
> >>>> @@ -1601,6 +1601,8 @@ int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
> >>>> void pci_disable_sriov(struct pci_dev *dev);
> >>>> int pci_num_vf(struct pci_dev *dev);
> >>>> int pci_vfs_assigned(struct pci_dev *dev);
> >>>> +void pci_sriov_assign_device(struct pci_dev *dev);
> >>>> +void pci_sriov_deassign_device(struct pci_dev *dev);
> >>>> int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
> >>>> int pci_sriov_get_totalvfs(struct pci_dev *dev);
> >>>> #else
> >>>> @@ -1610,6 +1612,8 @@ static inline void pci_disable_sriov(struct pci_dev *dev) { }
> >>>> static inline int pci_num_vf(struct pci_dev *dev) { return 0; }
> >>>> static inline int pci_vfs_assigned(struct pci_dev *dev)
> >>>> { return 0; }
> >>>> +static inline void pci_sriov_assign_device(struct pci_dev *dev) { }
> >>>> +static inline void pci_sriov_deassign_device(struct pci_dev *dev) { }
> >>>> static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
> >>>> { return 0; }
> >>>> static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
> >>>
> >
> >
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/4] PCI: introduce VFs reference counter and simplify pci_vfs_assigned() implementation
2014-07-11 2:33 ` Alex Williamson
@ 2014-07-11 3:11 ` ethan zhao
0 siblings, 0 replies; 10+ messages in thread
From: ethan zhao @ 2014-07-11 3:11 UTC (permalink / raw)
To: Alex Williamson
Cc: bhelgaas, konrad.wilk, boris.ostrovsky, david.vrabel, gleb,
pbonzini, jeffrey.t.kirsher, jesse.brandeburg, bruce.w.allan,
carolyn.wyborny, donald.c.skidmore, gregory.v.rose,
alexander.h.duyck, john.ronciak, mitch.a.williams, linux-pci, kvm,
linux.nics, e1000-devel, netdev, linux-kernel, ethan.kernel,
vaughan.cao
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 <ethan.zhao@oracle.com>
>>>>>> ---
>>>>>> drivers/pci/iov.c | 65 ++++++++++++++++++++++++++++----------------------
>>>>>> drivers/pci/pci.h | 1 +
>>>>>> include/linux/pci.h | 4 +++
>>>>>> 3 files changed, 41 insertions(+), 29 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>>>>>> index de7a747..72e267f 100644
>>>>>> --- a/drivers/pci/iov.c
>>>>>> +++ b/drivers/pci/iov.c
>>>>>> @@ -382,6 +382,7 @@ found:
>>>>>> iov->nres = nres;
>>>>>> iov->ctrl = ctrl;
>>>>>> iov->total_VFs = total;
>>>>>> + atomic_set(&iov->VFs_assigned_cnt, 0);
>>>>>> iov->offset = offset;
>>>>>> iov->stride = stride;
>>>>>> iov->pgsz = pgsz;
>>>>>> @@ -603,45 +604,51 @@ int pci_num_vf(struct pci_dev *dev)
>>>>>> EXPORT_SYMBOL_GPL(pci_num_vf);
>>>>>>
>>>>>> /**
>>>>>> - * pci_vfs_assigned - returns number of VFs are assigned to a guest
>>>>>> - * @dev: the PCI device
>>>>>> + * pci_vfs_assigned - returns number of VFs are assigned to VM
>>>>>> + * @dev: the physical PCI device that contains the VFs.
>>>>>> *
>>>>>> - * Returns number of VFs belonging to this device that are assigned to a guest.
>>>>>> + * Returns number of VFs belonging to this device that are assigned to VM.
>>>>>> * If device is not a physical function returns 0.
>>>>>> */
>>>>>> int pci_vfs_assigned(struct pci_dev *dev)
>>>>>> {
>>>>>> - struct pci_dev *vfdev;
>>>>>> - unsigned int vfs_assigned = 0;
>>>>>> - unsigned short dev_id;
>>>>>> -
>>>>>> - /* only search if we are a PF */
>>>>>> if (!dev->is_physfn)
>>>>>> return 0;
>>>>>> + if (dev->sriov)
>>>>>> + return atomic_read(&dev->sriov->VFs_assigned_cnt);
>>>>>> + return 0;
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(pci_vfs_assigned);
>>>>>>
>>>>>> - /*
>>>>>> - * determine the device ID for the VFs, the vendor ID will be the
>>>>>> - * same as the PF so there is no need to check for that one
>>>>>> - */
>>>>>> - pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_VF_DID, &dev_id);
>>>>>> -
>>>>>> - /* loop through all the VFs to see if we own any that are assigned */
>>>>>> - vfdev = pci_get_device(dev->vendor, dev_id, NULL);
>>>>>> - while (vfdev) {
>>>>>> - /*
>>>>>> - * It is considered assigned if it is a virtual function with
>>>>>> - * our dev as the physical function and the assigned bit is set
>>>>>> - */
>>>>>> - if (vfdev->is_virtfn && (vfdev->physfn == dev) &&
>>>>>> - (vfdev->dev_flags & PCI_DEV_FLAGS_ASSIGNED))
>>>>>> - vfs_assigned++;
>>>>>> -
>>>>>> - vfdev = pci_get_device(dev->vendor, dev_id, vfdev);
>>>>>> - }
>>>>>> +/**
>>>>>> + * pci_sriov_assign_device - assign device to VM
>>>>>> + * @pdev: the device to be assigned.
>>>>>> + */
>>>>>> +void pci_sriov_assign_device(struct pci_dev *pdev)
>>>>>> +{
>>>>>> + pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
>>>>>> + if (pdev->is_virtfn && !pdev->is_physfn)
>>>>>> + if (pdev->physfn)
>>>>>> + if (pdev->physfn->sriov)
>>>>>> + atomic_inc(&pdev->physfn->sriov->
>>>>>> + VFs_assigned_cnt);
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(pci_sriov_assign_device);
>>>>>>
>>>>>> - return vfs_assigned;
>>>>>> +/**
>>>>>> + * pci_sriov_deassign_device - deasign device from VM
>>>>>> + * @pdev: the device to be deassigned.
>>>>>> + */
>>>>>> +void pci_sriov_deassign_device(struct pci_dev *pdev)
>>>>>> +{
>>>>>> + pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
>>>>>> + if (pdev->is_virtfn && !pdev->is_physfn)
>>>>>> + if (pdev->physfn)
>>>>>> + if (pdev->physfn->sriov)
>>>>>> + atomic_dec(&pdev->physfn->sriov->
>>>>>> + VFs_assigned_cnt);
>>>>>> }
>>>>>> -EXPORT_SYMBOL_GPL(pci_vfs_assigned);
>>>>>> +EXPORT_SYMBOL_GPL(pci_sriov_deassign_device);
>>>>>>
>>>>>> /**
>>>>>> * pci_sriov_set_totalvfs -- reduce the TotalVFs available
>>>>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>>>>>> index 6bd0822..d17bda2 100644
>>>>>> --- a/drivers/pci/pci.h
>>>>>> +++ b/drivers/pci/pci.h
>>>>>> @@ -235,6 +235,7 @@ struct pci_sriov {
>>>>>> u32 pgsz; /* page size for BAR alignment */
>>>>>> u8 link; /* Function Dependency Link */
>>>>>> u16 driver_max_VFs; /* max num VFs driver supports */
>>>>>> + atomic_t VFs_assigned_cnt; /* counter of VFs assigned to VM */
>>>>>> struct pci_dev *dev; /* lowest numbered PF */
>>>>>> struct pci_dev *self; /* this PF */
>>>>>> struct mutex lock; /* lock for VF bus */
>>>>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>>>>> index aab57b4..5cf6833 100644
>>>>>> --- a/include/linux/pci.h
>>>>>> +++ b/include/linux/pci.h
>>>>>> @@ -1601,6 +1601,8 @@ int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
>>>>>> void pci_disable_sriov(struct pci_dev *dev);
>>>>>> int pci_num_vf(struct pci_dev *dev);
>>>>>> int pci_vfs_assigned(struct pci_dev *dev);
>>>>>> +void pci_sriov_assign_device(struct pci_dev *dev);
>>>>>> +void pci_sriov_deassign_device(struct pci_dev *dev);
>>>>>> int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
>>>>>> int pci_sriov_get_totalvfs(struct pci_dev *dev);
>>>>>> #else
>>>>>> @@ -1610,6 +1612,8 @@ static inline void pci_disable_sriov(struct pci_dev *dev) { }
>>>>>> static inline int pci_num_vf(struct pci_dev *dev) { return 0; }
>>>>>> static inline int pci_vfs_assigned(struct pci_dev *dev)
>>>>>> { return 0; }
>>>>>> +static inline void pci_sriov_assign_device(struct pci_dev *dev) { }
>>>>>> +static inline void pci_sriov_deassign_device(struct pci_dev *dev) { }
>>>>>> static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
>>>>>> { return 0; }
>>>>>> static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
>>>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread