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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 25.08.17 at 15:51, <chao.gao@intel.com> wrote:
> On Fri, Aug 25, 2017 at 03:39:38AM -0600, Jan Beulich wrote:
>>>>> On 25.08.17 at 07:27, <chao.gao@intel.com> 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@intel.com>
>>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>>> ---
>>>  - 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.

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

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

Not in this patch for sure. I also wouldn't want to fix it by simply
removing the "else" (see below). But without it being possible to
test the change I'm not sure what to do; first of all I of course
wanted to see if I'm wrong with the observation.

Jan

--- unstable.orig/xen/drivers/passthrough/pci.c
+++ unstable/xen/drivers/passthrough/pci.c
@@ -615,10 +615,7 @@ int pci_add_device(u16 seg, u8 bus, u8 d
         pdev_type = "virtual function";
     }
     else
-    {
-        info = NULL;
         pdev_type = "device";
-    }
 
     ret = xsm_resource_plug_pci(XSM_PRIV, (seg << 16) | (bus << 8) | devfn);
     if ( ret )
@@ -638,7 +635,8 @@ int pci_add_device(u16 seg, u8 bus, u8 d
 
     if ( info )
         pdev->info = *info;
-    else if ( !pdev->vf_rlen[0] )
+
+    if ( (!info || !info->is_virtfn) && !pdev->vf_rlen[0] )
     {
         unsigned int pos = pci_find_ext_capability(seg, bus, devfn,
                                                    PCI_EXT_CAP_ID_SRIOV);


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