Re: [Xen-devel] [PATCH RESEND v9] VT-d: use correct BDF for VF to search VT-d unit

[Chao Gao <chao.gao@xxxxxxxxx>]

On Fri, Aug 25, 2017 at 09:20:23AM -0600, Jan Beulich wrote:
>>>> On 25.08.17 at 15:51, <chao.gao@xxxxxxxxx> wrote:
>> On Fri, Aug 25, 2017 at 03:39:38AM -0600, Jan Beulich wrote:
>>>>>> On 25.08.17 at 07:27, <chao.gao@xxxxxxxxx> wrote:
>>>> When SR-IOV is enabled, 'Virtual Functions' of a 'Physical Function' are 
>>>> under
>>>> the scope of the same VT-d unit as the 'Physical Function'. A 'Physical
>>>> Function' can be a 'Traditional Function' or an ARI 'Extended Function'.
>>>> And furthermore, 'Extended Functions' on an endpoint are under the scope of
>>>> the same VT-d unit as the 'Traditional Functions' on the endpoint. To 
>>>> search
>>>> VT-d unit, the BDF of PF or the BDF of a traditional function may be used. 
>>>> And
>>>> it depends on whether the PF is an extended function or not.
>>>> 
>>>> Current code uses PCI_SLOT() to recognize an ARI 'Extended Funcion'. But it
>>>> is conceptually wrong w/o checking whether PF is an extended function and
>>>> would lead to match VFs of a RC endpoint to a wrong VT-d unit.
>>>> 
>>>> This patch uses VF's 'is_extfn' field to indicate whether the PF of this 
>>>> VF 
>>>> is
>>>> an extended function. The field helps to use correct BDF to search VT-d 
>>>> unit.
>>>> 
>>>> Reported-by: Crawford, Eric R <Eric.R.Crawford@xxxxxxxxx>
>>>> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
>>>> ---
>>>>  - RESEND for the previous email has no subject. 
>>>> 
>>>> v9:
>>>>  - check 'is_virtfn' first in pci_add_device() to avoid potential error if
>>>>  linux side sets VF's 'is_extfn'
>>>>  - comments changes to make it clear that we use VF's 'is_extfn' 
>>>> intentionally
>>>>  otherwise the patch seems like a workaround.
>>>> 
>>>> v8:
>>>>  - use "conceptually wrong", instead of "a corner case" in commit message
>>>>  - check 'is_virtfn' first in acpi_find_matched_drhd_unit()
>>>> 
>>>> v7:
>>>>  - Drop Eric's tested-by
>>>>  - Change commit message to be clearer
>>>>  - Re-use VF's is_extfn field
>>>>  - access PF's is_extfn field in locked area
>>>> 
>>>> ---
>>>>  xen/drivers/passthrough/pci.c      | 14 ++++++++++----
>>>>  xen/drivers/passthrough/vtd/dmar.c | 12 ++++++------
>>>>  xen/include/xen/pci.h              |  1 +
>>>>  3 files changed, 17 insertions(+), 10 deletions(-)
>>>> 
>>>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>>>> index 27bdb71..0e27e29 100644
>>>> --- a/xen/drivers/passthrough/pci.c
>>>> +++ b/xen/drivers/passthrough/pci.c
>>>> @@ -599,21 +599,24 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>>>      unsigned int slot = PCI_SLOT(devfn), func = PCI_FUNC(devfn);
>>>>      const char *pdev_type;
>>>>      int ret;
>>>> +    bool pf_is_extfn = false;
>>>>  
>>>> -    if (!info)
>>>> +    if ( !info )
>>>>          pdev_type = "device";
>>>> -    else if (info->is_extfn)
>>>> -        pdev_type = "extended function";
>>>> -    else if (info->is_virtfn)
>>>> +    else if ( info->is_virtfn )
>>>>      {
>>>>          pcidevs_lock();
>>>>          pdev = pci_get_pdev(seg, info->physfn.bus, info->physfn.devfn);
>>>> +        if ( pdev )
>>>> +            pf_is_extfn = pdev->info.is_extfn;
>>>>          pcidevs_unlock();
>>>>          if ( !pdev )
>>>>              pci_add_device(seg, info->physfn.bus, info->physfn.devfn,
>>>>                             NULL, node);
>>>>          pdev_type = "virtual function";
>>>>      }
>>>> +    else if ( info->is_extfn )
>>>> +        pdev_type = "extended function";
>>>>      else
>>>>      {
>>>>          info = NULL;
>>>> @@ -707,6 +710,9 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>>>                     seg, bus, slot, func, ctrl);
>>>>      }
>>>>  
>>>> +    /* VF's 'is_extfn' is used to indicate whether PF is an extended 
>> function */
>>>> +    if ( pdev->info.is_virtfn )
>>>> +        pdev->info.is_extfn = pf_is_extfn;
>>>>      check_pdev(pdev);
>>>
>>>Can this please be moved up right next to
>>>
>>>        pdev->info = *info;
>>>
>>>, so that information is right from the point it is being stored? And
>> 
>> Yes. I will.
>> 
>>>looking at that code I can't really work out why the SR-IOV device
>>>handling is in an "else if" to that path. I can't check that case
>>>myself, as by box'es root ports don't support ARI forwarding, so
>>>despite PF and VF being ARI-capable it can't be enabled, and
>>>hence I'm not seeing the devices reported as extended functions.
>> 
>> Yeah. I think we should remove "else if" for it is the only place
>> where vf_rlen[] is set, otherwise extended PF's vf_rlen[] won't be
>> initialized. I think we don't have extended PF at present, so the bug
>> isn't triggered.
>
>So none of your chipsets implement ARI forwarding? I would have
>hoped you could test this somewhere.

No. I mean PF always has function number <= 7 at present. Thus, it can't
be extended function even ARI forwarding enabled in upstream bridge.

>
>>  Currently, VF won't implement SRIOV feature, seeing
>> SRIOV specv1.1 chapter 3.7 PCI Express Extended Capabilities. Even VF
>> will implement SRIOV later, I think as long as a function is SRIOV
>> capable, we can initialize vf_rlen[] here.
>
>How could a VF itself implement SR-IOV?

I don't know.

Thanks
Chao


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel