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

[Ethan Zhao <ethan.kernel@gmail.com>]

> 在 2014年7月11日，下午11:23，Alex Williamson <alex.williamson@redhat.com> 写道：
> 
>> On Fri, 2014-07-11 at 23:07 +0800, Ethan Zhao wrote:
>> 
>>> 在 2014年7月11日，下午10:50，Alex Williamson <alex.williamson@redhat.com> 写道：
>>> 
>>> On Fri, 2014-07-11 at 22:20 +0800, Ethan Zhao wrote:
>>>>>> 在 2014年7月11日，下午9:13，Alex Williamson <alex.williamson@redhat.com> 写道：
>>>>>> 
>>>>>> On Fri, 2014-07-11 at 20:30 +0800, Ethan Zhao wrote:
>>>>>> This patch introduces two new device assignment functions
>>>>>> 
>>>>>> pci_iov_assign_device(),
>>>>>> pci_iov_deassign_device()
>>>>>> 
>>>>>> along with the existed one
>>>>>> 
>>>>>> pci_vfs_assigned()
>>>>>> 
>>>>>> They construct the VFs assignment management interface, used to assign/
>>>>>> deassign device to VM and query the VFs reference counter. instead of
>>>>>> direct manipulation of device flag.
>>>>>> 
>>>>>> This patch refashioned the related code and make them atomic.
>>>>>> 
>>>>>> v3: change the naming of device assignment helpers, because they work
>>>>>> for all kind of PCI device, not only SR-IOV (david.vrabel@citrix.com)
>>>>>> 
>>>>>> v2: reorder the patchset and make it bisectable and atomic, steps clear
>>>>>> between interface defination and implemenation according to the
>>>>>> suggestion from alex.williamson@redhat.com
>>>>>> 
>>>>>> Signed-off-by: Ethan Zhao <ethan.zhao@oracle.com>
>>>>>> ---
>>>>> 
>>>>> - Use a cover patch to describe the series
>>>> Hmmm,
>>>>> - Version information goes here, below the "---", not above it
>>>>> 
>>>>> - I stand by the patch breakdown I suggested originally, there are too
>>>>> many logical changes here in patch 1.  There are easily 3 separate
>>>>> patches here.
>>>> You prefer v1 again ?
>>> 
>>> That's not at all what I'm suggesting.  In response to v1 I suggested a
>>> specific reordering that would allow the series to be bisectable.  I'm
>>> surprised to see v2 ignored that and combined the first 3 suggested
>>> patches.
>> If what I understand right. Your points are.
>> 1. To be bisectable.
>>    Combine the code into 2 parches, is really bisect able, without 2, 1 works right.
>>    If 1 was broken into 3, any of them is lost, buggy. They must work together.
> 
> No, I never said 2 patches.  I said to fix and abstract the usage, then
> change the implementation.  Fixing and abstracting the usage should be
> multiple patches.  You should not be concerned about patches in a series
> getting lost, you should be concerned that each step along the way is
> functional.
> 
How about  5 patches ?
 - 1 . Fix i40e with pci_vfs_assigned()
 - 2.  Define assign/design helper.
 - 3.  KVM uses helpers
 - 4.  Xen uses helpers
 - 5. Introduce VF ref counter.

This series looks like you first suggestion.

>>> pci_vfs_assigned
>> 2. Define and abstract first then implement.
>>   1 &. 2 follows this point.
>>> 
>>>>> - Renaming s/sriov/iov/ doesn't address the problem David raised.  The
>>>>> name is still synonymous with SR-IOV and it's defined on
>>>>> drivers/pci/iov.c, which is only built with CONFIG_PCI_IOV.
>>>> This is real issue, the interface should work without CONFIG_PCI_IOV defined.
>>>> Should be fixed.
>>>> 
>>>>> - PCI_DEV_FLAGS_ASSIGNED should be unused after this series, why is it
>>>>> not removed?
>>>> you wanna do all things in one step ? Be practical. That is not my original purpose.
>>> 
>>> I never suggested collapsing steps.  Maybe I'd understand the original
>>> purpose better if there was an overall description in a cover patch.
>> Compose a cover letter seems necessary to describe patch set if they are too complex.
> 
> Any series with more than a single patch should have a cover letter.
> 
>>> 
>>>>>> drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c |   17 ++---------------
>>>>>> drivers/pci/iov.c                                  |   20 ++++++++++++++++++++
>>>>>> drivers/xen/xen-pciback/pci_stub.c                 |    4 ++--
>>>>>> include/linux/pci.h                                |    4 ++++
>>>>>> virt/kvm/assigned-dev.c                            |    2 +-
>>>>>> virt/kvm/iommu.c                                   |    4 ++--
>>>>> 
>>>>> - This patch touch 4 separate logical code areas, who do you expect to
>>>>> ack and commit it?  Split it up.
>>>>  Such would go back to the v1, four patches, if one lost to be committed, kernel could be buggy.  
>>> 
>>> There seems to be confusion between multiple patches, which is fine, and
>>> the patch ordering from v1, which is wrong.
>> Do you want them work standalone, if no, the confusion is nothing.
> 
> Each step in a series should be functional.  There's absolutely no
> requirement that any individual patch from a series can be pulled out
> separately and work on it's own.  Patches build on each other.
> 
>>>>>> 6 files changed, 31 insertions(+), 20 deletions(-)
>>>>>> 
>>>>>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
>>>>>> index 02c11a7..781040e 100644
>>>>>> --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
>>>>>> +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
>>>>>> @@ -693,22 +693,9 @@ complete_reset:
>>>>>> static bool i40e_vfs_are_assigned(struct i40e_pf *pf)
>>>>>> {
>>>>>>  struct pci_dev *pdev = pf->pdev;
>>>>>> -    struct pci_dev *vfdev;
>>>>>> -
>>>>>> -    /* loop through all the VFs to see if we own any that are assigned */
>>>>>> -    vfdev = pci_get_device(PCI_VENDOR_ID_INTEL, I40E_DEV_ID_VF , NULL);
>>>>>> -    while (vfdev) {
>>>>>> -        /* if we don't own it we don't care */
>>>>>> -        if (vfdev->is_virtfn && pci_physfn(vfdev) == pdev) {
>>>>>> -            /* if it is assigned we cannot release it */
>>>>>> -            if (vfdev->dev_flags & PCI_DEV_FLAGS_ASSIGNED)
>>>>>> -                return true;
>>>>>> -        }
>>>>>> 
>>>>>> -        vfdev = pci_get_device(PCI_VENDOR_ID_INTEL,
>>>>>> -                       I40E_DEV_ID_VF,
>>>>>> -                       vfdev);
>>>>>> -    }
>>>>>> +    if (pci_vfs_assigned(pdev))
>>>>>> +        return true;
>>>>>> 
>>>>>>  return false;
>>>>>> }
>>>>>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>>>>>> index de7a747..090f827 100644
>>>>>> --- a/drivers/pci/iov.c
>>>>>> +++ b/drivers/pci/iov.c
>>>>>> @@ -644,6 +644,26 @@ int pci_vfs_assigned(struct pci_dev *dev)
>>>>>> EXPORT_SYMBOL_GPL(pci_vfs_assigned);
>>>>>> 
>>>>>> /**
>>>>>> + * pci_iov_assign_device - assign device to VM
>>>>>> + * @pdev: the device to be assigned.
>>>>>> + */
>>>>>> +void pci_iov_assign_device(struct pci_dev *pdev)
>>>>>> +{
>>>>>> +    pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(pci_iov_assign_device);
>>>>>> +
>>>>>> +/**
>>>>>> + * pci_iov_deassign_device - deasign device from VM
>>>>>> + * @pdev: the device to be deassigned.
>>>>>> + */
>>>>>> +void pci_iov_deassign_device(struct pci_dev *pdev)
>>>>>> +{
>>>>>> +    pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(pci_iov_deassign_device);
>>>>>> +
>>>>>> +/**
>>>>>> * pci_sriov_set_totalvfs -- reduce the TotalVFs available
>>>>>> * @dev: the PCI PF device
>>>>>> * @numvfs: number that should be used for TotalVFs supported
>>>>>> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
>>>>>> index 62fcd48..27e00d1 100644
>>>>>> --- a/drivers/xen/xen-pciback/pci_stub.c
>>>>>> +++ b/drivers/xen/xen-pciback/pci_stub.c
>>>>>> @@ -133,7 +133,7 @@ static void pcistub_device_release(struct kref *kref)
>>>>>>  xen_pcibk_config_free_dyn_fields(dev);
>>>>>>  xen_pcibk_config_free_dev(dev);
>>>>>> 
>>>>>> -    dev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
>>>>>> +    pci_iov_deassign_device(dev);
>>>>>>  pci_dev_put(dev);
>>>>>> 
>>>>>>  kfree(psdev);
>>>>>> @@ -404,7 +404,7 @@ static int pcistub_init_device(struct pci_dev *dev)
>>>>>>  dev_dbg(&dev->dev, "reset device\n");
>>>>>>  xen_pcibk_reset_device(dev);
>>>>>> 
>>>>>> -    dev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
>>>>>> +    pci_iov_assign_device(dev);
>>>>>>  return 0;
>>>>>> 
>>>>>> config_release:
>>>>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>>>>> index aab57b4..5ece6d6 100644
>>>>>> --- a/include/linux/pci.h
>>>>>> +++ b/include/linux/pci.h
>>>>>> @@ -1603,6 +1603,8 @@ int pci_num_vf(struct pci_dev *dev);
>>>>>> int pci_vfs_assigned(struct pci_dev *dev);
>>>>>> int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
>>>>>> int pci_sriov_get_totalvfs(struct pci_dev *dev);
>>>>>> +void pci_iov_assign_device(struct pci_dev *dev);
>>>>>> +void pci_iov_deassign_device(struct pci_dev *dev);
>>>>>> #else
>>>>>> static inline int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
>>>>>> { return -ENODEV; }
>>>>>> @@ -1614,6 +1616,8 @@ static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
>>>>>> { return 0; }
>>>>>> static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
>>>>>> { return 0; }
>>>>>> +static inline void pci_iov_assign_device(struct pci_dev *dev) { }
>>>>>> +static inline void pci_iov_deassign_device(struct pci_dev *dev) { }
>>>>>> #endif
>>>>>> 
>>>>>> #if defined(CONFIG_HOTPLUG_PCI) || defined(CONFIG_HOTPLUG_PCI_MODULE)
>>>>>> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
>>>>>> index bf06577..7fac58d 100644
>>>>>> --- a/virt/kvm/assigned-dev.c
>>>>>> +++ b/virt/kvm/assigned-dev.c
>>>>>> @@ -302,7 +302,7 @@ static void kvm_free_assigned_device(struct kvm *kvm,
>>>>>>  else
>>>>>>      pci_restore_state(assigned_dev->dev);
>>>>>> 
>>>>>> -    assigned_dev->dev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
>>>>>> +    pci_iov_deassign_device(assigned_dev->dev);
>>>>>> 
>>>>>>  pci_release_regions(assigned_dev->dev);
>>>>>>  pci_disable_device(assigned_dev->dev);
>>>>>> diff --git a/virt/kvm/iommu.c b/virt/kvm/iommu.c
>>>>>> index 0df7d4b..641ee73 100644
>>>>>> --- a/virt/kvm/iommu.c
>>>>>> +++ b/virt/kvm/iommu.c
>>>>>> @@ -194,7 +194,7 @@ int kvm_assign_device(struct kvm *kvm,
>>>>>>          goto out_unmap;
>>>>>>  }
>>>>>> 
>>>>>> -    pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
>>>>>> +    pci_iov_assign_device(pdev);
>>>>>> 
>>>>>>  dev_info(&pdev->dev, "kvm assign device\n");
>>>>>> 
>>>>>> @@ -220,7 +220,7 @@ int kvm_deassign_device(struct kvm *kvm,
>>>>>> 
>>>>>>  iommu_detach_device(domain, &pdev->dev);
>>>>>> 
>>>>>> -    pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
>>>>>> +    pci_iov_deassign_device(pdev);
>>>>>> 
>>>>>>  dev_info(&pdev->dev, "kvm deassign device\n");
> 
> 
> 

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