> 在 2014年7月11日，下午11:23，Alex Williamson <[email protected]> 写道：
>
>> On Fri, 2014-07-11 at 23:07 +0800, Ethan Zhao wrote:
>>
>>> 在 2014年7月11日，下午10:50，Alex Williamson <[email protected]> 写道：
>>>
>>> On Fri, 2014-07-11 at 22:20 +0800, Ethan Zhao wrote:
>>>>>> 在 2014年7月11日，下午9:13，Alex Williamson <[email protected]> 写道：
>>>>>>
>>>>>> On Fri, 2014-07-11 at 20:30 +0800, Ethan Zhao wrote:
>>>>>> This patch introduces two new device assignment functions
>>>>>>
>>>>>> pci_iov_assign_device(),
>>>>>> pci_iov_deassign_device()
>>>>>>
>>>>>> along with the existed one
>>>>>>
>>>>>> pci_vfs_assigned()
>>>>>>
>>>>>> They construct the VFs assignment management interface, used to assign/
>>>>>> deassign device to VM and query the VFs reference counter. instead of
>>>>>> direct manipulation of device flag.
>>>>>>
>>>>>> This patch refashioned the related code and make them atomic.
>>>>>>
>>>>>> v3: change the naming of device assignment helpers, because they work
>>>>>> for all kind of PCI device, not only SR-IOV ([email protected])
>>>>>>
>>>>>> v2: reorder the patchset and make it bisectable and atomic, steps clear
>>>>>> between interface defination and implemenation according to the
>>>>>> suggestion from [email protected]
>>>>>>
>>>>>> Signed-off-by: Ethan Zhao <[email protected]>
>>>>>> ---
>>>>>
>>>>> - Use a cover patch to describe the series
>>>> Hmmm,
>>>>> - Version information goes here, below the "---", not above it
>>>>>
>>>>> - I stand by the patch breakdown I suggested originally, there are too
>>>>> many logical changes here in patch 1. There are easily 3 separate
>>>>> patches here.
>>>> You prefer v1 again ?
>>>
>>> That's not at all what I'm suggesting. In response to v1 I suggested a
>>> specific reordering that would allow the series to be bisectable. I'm
>>> surprised to see v2 ignored that and combined the first 3 suggested
>>> patches.
>> If what I understand right. Your points are.
>> 1. To be bisectable.
>> Combine the code into 2 parches, is really bisect able, without 2, 1
>> works right.
>> If 1 was broken into 3, any of them is lost, buggy. They must work
>> together.
>
> No, I never said 2 patches. I said to fix and abstract the usage, then
> change the implementation. Fixing and abstracting the usage should be
> multiple patches. You should not be concerned about patches in a series
> getting lost, you should be concerned that each step along the way is
> functional.
>
How about 5 patches ?
- 1 . Fix i40e with pci_vfs_assigned()
- 2. Define assign/design helper.
- 3. KVM uses helpers
- 4. Xen uses helpers
- 5. Introduce VF ref counter.
This series looks like you first suggestion.
>>> pci_vfs_assigned
>> 2. Define and abstract first then implement.
>> 1 &. 2 follows this point.
>>>
>>>>> - Renaming s/sriov/iov/ doesn't address the problem David raised. The
>>>>> name is still synonymous with SR-IOV and it's defined on
>>>>> drivers/pci/iov.c, which is only built with CONFIG_PCI_IOV.
>>>> This is real issue, the interface should work without CONFIG_PCI_IOV
>>>> defined.
>>>> Should be fixed.
>>>>
>>>>> - PCI_DEV_FLAGS_ASSIGNED should be unused after this series, why is it
>>>>> not removed?
>>>> you wanna do all things in one step ? Be practical. That is not my
>>>> original purpose.
>>>
>>> I never suggested collapsing steps. Maybe I'd understand the original
>>> purpose better if there was an overall description in a cover patch.
>> Compose a cover letter seems necessary to describe patch set if they are too
>> complex.
>
> Any series with more than a single patch should have a cover letter.
>
>>>
>>>>>> drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 17
>>>>>> ++---------------
>>>>>> drivers/pci/iov.c | 20
>>>>>> ++++++++++++++++++++
>>>>>> drivers/xen/xen-pciback/pci_stub.c | 4 ++--
>>>>>> include/linux/pci.h | 4 ++++
>>>>>> virt/kvm/assigned-dev.c | 2 +-
>>>>>> virt/kvm/iommu.c | 4 ++--
>>>>>
>>>>> - This patch touch 4 separate logical code areas, who do you expect to
>>>>> ack and commit it? Split it up.
>>>> Such would go back to the v1, four patches, if one lost to be committed,
>>>> kernel could be buggy.
>>>
>>> There seems to be confusion between multiple patches, which is fine, and
>>> the patch ordering from v1, which is wrong.
>> Do you want them work standalone, if no, the confusion is nothing.
>
> Each step in a series should be functional. There's absolutely no
> requirement that any individual patch from a series can be pulled out
> separately and work on it's own. Patches build on each other.
>
>>>>>> 6 files changed, 31 insertions(+), 20 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
>>>>>> b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
>>>>>> index 02c11a7..781040e 100644
>>>>>> --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
>>>>>> +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
>>>>>> @@ -693,22 +693,9 @@ complete_reset:
>>>>>> static bool i40e_vfs_are_assigned(struct i40e_pf *pf)
>>>>>> {
>>>>>> struct pci_dev *pdev = pf->pdev;
>>>>>> - struct pci_dev *vfdev;
>>>>>> -
>>>>>> - /* loop through all the VFs to see if we own any that are assigned
>>>>>> */
>>>>>> - vfdev = pci_get_device(PCI_VENDOR_ID_INTEL, I40E_DEV_ID_VF , NULL);
>>>>>> - while (vfdev) {
>>>>>> - /* if we don't own it we don't care */
>>>>>> - if (vfdev->is_virtfn && pci_physfn(vfdev) == pdev) {
>>>>>> - /* if it is assigned we cannot release it */
>>>>>> - if (vfdev->dev_flags & PCI_DEV_FLAGS_ASSIGNED)
>>>>>> - return true;
>>>>>> - }
>>>>>>
>>>>>> - vfdev = pci_get_device(PCI_VENDOR_ID_INTEL,
>>>>>> - I40E_DEV_ID_VF,
>>>>>> - vfdev);
>>>>>> - }
>>>>>> + if (pci_vfs_assigned(pdev))
>>>>>> + return true;
>>>>>>
>>>>>> return false;
>>>>>> }
>>>>>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>>>>>> index de7a747..090f827 100644
>>>>>> --- a/drivers/pci/iov.c
>>>>>> +++ b/drivers/pci/iov.c
>>>>>> @@ -644,6 +644,26 @@ int pci_vfs_assigned(struct pci_dev *dev)
>>>>>> EXPORT_SYMBOL_GPL(pci_vfs_assigned);
>>>>>>
>>>>>> /**
>>>>>> + * pci_iov_assign_device - assign device to VM
>>>>>> + * @pdev: the device to be assigned.
>>>>>> + */
>>>>>> +void pci_iov_assign_device(struct pci_dev *pdev)
>>>>>> +{
>>>>>> + pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(pci_iov_assign_device);
>>>>>> +
>>>>>> +/**
>>>>>> + * pci_iov_deassign_device - deasign device from VM
>>>>>> + * @pdev: the device to be deassigned.
>>>>>> + */
>>>>>> +void pci_iov_deassign_device(struct pci_dev *pdev)
>>>>>> +{
>>>>>> + pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(pci_iov_deassign_device);
>>>>>> +
>>>>>> +/**
>>>>>> * pci_sriov_set_totalvfs -- reduce the TotalVFs available
>>>>>> * @dev: the PCI PF device
>>>>>> * @numvfs: number that should be used for TotalVFs supported
>>>>>> diff --git a/drivers/xen/xen-pciback/pci_stub.c
>>>>>> b/drivers/xen/xen-pciback/pci_stub.c
>>>>>> index 62fcd48..27e00d1 100644
>>>>>> --- a/drivers/xen/xen-pciback/pci_stub.c
>>>>>> +++ b/drivers/xen/xen-pciback/pci_stub.c
>>>>>> @@ -133,7 +133,7 @@ static void pcistub_device_release(struct kref *kref)
>>>>>> xen_pcibk_config_free_dyn_fields(dev);
>>>>>> xen_pcibk_config_free_dev(dev);
>>>>>>
>>>>>> - dev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
>>>>>> + pci_iov_deassign_device(dev);
>>>>>> pci_dev_put(dev);
>>>>>>
>>>>>> kfree(psdev);
>>>>>> @@ -404,7 +404,7 @@ static int pcistub_init_device(struct pci_dev *dev)
>>>>>> dev_dbg(&dev->dev, "reset device\n");
>>>>>> xen_pcibk_reset_device(dev);
>>>>>>
>>>>>> - dev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
>>>>>> + pci_iov_assign_device(dev);
>>>>>> return 0;
>>>>>>
>>>>>> config_release:
>>>>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>>>>> index aab57b4..5ece6d6 100644
>>>>>> --- a/include/linux/pci.h
>>>>>> +++ b/include/linux/pci.h
>>>>>> @@ -1603,6 +1603,8 @@ int pci_num_vf(struct pci_dev *dev);
>>>>>> int pci_vfs_assigned(struct pci_dev *dev);
>>>>>> int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
>>>>>> int pci_sriov_get_totalvfs(struct pci_dev *dev);
>>>>>> +void pci_iov_assign_device(struct pci_dev *dev);
>>>>>> +void pci_iov_deassign_device(struct pci_dev *dev);
>>>>>> #else
>>>>>> static inline int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
>>>>>> { return -ENODEV; }
>>>>>> @@ -1614,6 +1616,8 @@ static inline int pci_sriov_set_totalvfs(struct
>>>>>> pci_dev *dev, u16 numvfs)
>>>>>> { return 0; }
>>>>>> static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
>>>>>> { return 0; }
>>>>>> +static inline void pci_iov_assign_device(struct pci_dev *dev) { }
>>>>>> +static inline void pci_iov_deassign_device(struct pci_dev *dev) { }
>>>>>> #endif
>>>>>>
>>>>>> #if defined(CONFIG_HOTPLUG_PCI) || defined(CONFIG_HOTPLUG_PCI_MODULE)
>>>>>> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
>>>>>> index bf06577..7fac58d 100644
>>>>>> --- a/virt/kvm/assigned-dev.c
>>>>>> +++ b/virt/kvm/assigned-dev.c
>>>>>> @@ -302,7 +302,7 @@ static void kvm_free_assigned_device(struct kvm *kvm,
>>>>>> else
>>>>>> pci_restore_state(assigned_dev->dev);
>>>>>>
>>>>>> - assigned_dev->dev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
>>>>>> + pci_iov_deassign_device(assigned_dev->dev);
>>>>>>
>>>>>> pci_release_regions(assigned_dev->dev);
>>>>>> pci_disable_device(assigned_dev->dev);
>>>>>> diff --git a/virt/kvm/iommu.c b/virt/kvm/iommu.c
>>>>>> index 0df7d4b..641ee73 100644
>>>>>> --- a/virt/kvm/iommu.c
>>>>>> +++ b/virt/kvm/iommu.c
>>>>>> @@ -194,7 +194,7 @@ int kvm_assign_device(struct kvm *kvm,
>>>>>> goto out_unmap;
>>>>>> }
>>>>>>
>>>>>> - pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
>>>>>> + pci_iov_assign_device(pdev);
>>>>>>
>>>>>> dev_info(&pdev->dev, "kvm assign device\n");
>>>>>>
>>>>>> @@ -220,7 +220,7 @@ int kvm_deassign_device(struct kvm *kvm,
>>>>>>
>>>>>> iommu_detach_device(domain, &pdev->dev);
>>>>>>
>>>>>> - pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
>>>>>> + pci_iov_deassign_device(pdev);
>>>>>>
>>>>>> dev_info(&pdev->dev, "kvm deassign device\n");
>
>
>
------------------------------------------------------------------------------
_______________________________________________
E1000-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit
http://communities.intel.com/community/wired
