All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH RESEND v9] VT-d: use correct BDF for VF to search VT-d unit
@ 2017-08-25  5:27 Chao Gao
  2017-08-25  9:39 ` Jan Beulich
  0 siblings, 1 reply; 6+ messages in thread
From: Chao Gao @ 2017-08-25  5:27 UTC (permalink / raw)
  To: xen-devel
  Cc: Kevin Tian, Stefano Stabellini, Wei Liu, George Dunlap,
	Ian Jackson, Tim Deegan, Jan Beulich, Andrew Cooper, Chao Gao,
	Crawford Eric R, Roger Pau Monné

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);
 
     ret = 0;
diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
index 82040dd..75c9c92 100644
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -211,15 +211,15 @@ struct acpi_drhd_unit *acpi_find_matched_drhd_unit(const struct pci_dev *pdev)
     if ( pdev == NULL )
         return NULL;
 
-    if ( pdev->info.is_extfn )
+    if ( pdev->info.is_virtfn )
     {
-        bus = pdev->bus;
-        devfn = 0;
+        bus = pdev->info.physfn.bus;
+        devfn = pdev->info.is_extfn ? 0 : pdev->info.physfn.devfn;
     }
-    else if ( pdev->info.is_virtfn )
+    else if ( pdev->info.is_extfn )
     {
-        bus = pdev->info.physfn.bus;
-        devfn = PCI_SLOT(pdev->info.physfn.devfn) ? 0 : pdev->info.physfn.devfn;
+        bus = pdev->bus;
+        devfn = 0;
     }
     else
     {
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 59b6e8a..4dd42ac 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -39,6 +39,7 @@
 #define PCI_SBDF3(s,b,df) ((((s) & 0xffff) << 16) | PCI_BDF2(b, df))
 
 struct pci_dev_info {
+    /* VF's 'is_extfn' is used to show whether its PF an extended function */
     bool_t is_extfn;
     bool_t is_virtfn;
     struct {
-- 
1.8.3.1


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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH RESEND v9] VT-d: use correct BDF for VF to search VT-d unit
  2017-08-25  5:27 [PATCH RESEND v9] VT-d: use correct BDF for VF to search VT-d unit Chao Gao
@ 2017-08-25  9:39 ` Jan Beulich
  2017-08-25 13:51   ` Chao Gao
  0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2017-08-25  9:39 UTC (permalink / raw)
  To: Chao Gao
  Cc: Tim Deegan, Kevin Tian, Stefano Stabellini, Wei Liu,
	George Dunlap, Andrew Cooper, Ian Jackson, xen-devel,
	Crawford Eric R, Roger Pau Monné

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

Jan


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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH RESEND v9] VT-d: use correct BDF for VF to search VT-d unit
  2017-08-25  9:39 ` Jan Beulich
@ 2017-08-25 13:51   ` Chao Gao
  2017-08-25 15:20     ` Jan Beulich
  0 siblings, 1 reply; 6+ messages in thread
From: Chao Gao @ 2017-08-25 13:51 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Tim Deegan, Kevin Tian, Stefano Stabellini, Wei Liu,
	George Dunlap, Andrew Cooper, Ian Jackson, xen-devel,
	Crawford Eric R, Roger Pau Monné

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.  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@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH RESEND v9] VT-d: use correct BDF for VF to search VT-d unit
  2017-08-25 15:20     ` Jan Beulich
@ 2017-08-25 14:30       ` Chao Gao
  2017-08-28  5:47       ` Tian, Kevin
  1 sibling, 0 replies; 6+ messages in thread
From: Chao Gao @ 2017-08-25 14:30 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Tim Deegan, Kevin Tian, Stefano Stabellini, Wei Liu,
	George Dunlap, Andrew Cooper, Ian Jackson, xen-devel,
	Crawford Eric R, Roger Pau Monné

On Fri, Aug 25, 2017 at 09:20:23AM -0600, Jan Beulich wrote:
>>>> 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.

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@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH RESEND v9] VT-d: use correct BDF for VF to search VT-d unit
  2017-08-25 13:51   ` Chao Gao
@ 2017-08-25 15:20     ` Jan Beulich
  2017-08-25 14:30       ` Chao Gao
  2017-08-28  5:47       ` Tian, Kevin
  0 siblings, 2 replies; 6+ messages in thread
From: Jan Beulich @ 2017-08-25 15:20 UTC (permalink / raw)
  To: Chao Gao
  Cc: Tim Deegan, Kevin Tian, Stefano Stabellini, Wei Liu,
	George Dunlap, Andrew Cooper, Ian Jackson, xen-devel,
	Crawford Eric R, Roger Pau Monné

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH RESEND v9] VT-d: use correct BDF for VF to search VT-d unit
  2017-08-25 15:20     ` Jan Beulich
  2017-08-25 14:30       ` Chao Gao
@ 2017-08-28  5:47       ` Tian, Kevin
  1 sibling, 0 replies; 6+ messages in thread
From: Tian, Kevin @ 2017-08-28  5:47 UTC (permalink / raw)
  To: Jan Beulich, Gao, Chao
  Cc: Tim Deegan, Stefano Stabellini, Wei Liu, George Dunlap,
	Andrew Cooper, Ian Jackson, xen-devel, Crawford, Eric R,
	Roger Pau Monné

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, August 25, 2017 11:20 PM
> 
> 
> >  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?
> 

PCI SRIOV doesn't support such capability, i.e. no nested hardware
i/o virtualization

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-08-28  5:47 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-25  5:27 [PATCH RESEND v9] VT-d: use correct BDF for VF to search VT-d unit Chao Gao
2017-08-25  9:39 ` Jan Beulich
2017-08-25 13:51   ` Chao Gao
2017-08-25 15:20     ` Jan Beulich
2017-08-25 14:30       ` Chao Gao
2017-08-28  5:47       ` Tian, Kevin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.