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 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.  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.

Do you think it is bug? if yes, should it be fixed in this patch?

Thanks
Chao

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