[PATCH v10] VT-d: use correct BDF for VF to search VT-d unit

[PATCH v10] VT-d: use correct BDF for VF to search VT-d unit

[Chao Gao]

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 for a VF, if its PF
isn't an extended function, the BDF of PF should be used. Otherwise
the BDF of a traditional function in the same device with the PF
should be used.

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 integrated PF to a wrong
VT-d unit.

This patch overrides VF 'is_extfn' field and uses this 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>
---
v10:
 - move setting vf's is_extfn closer to the place where we set other fields.
 - reverse the conditional expression in acpi_find_matched_drhd_unit()

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      | 19 +++++++++++++++----
 xen/drivers/passthrough/vtd/dmar.c | 12 ++++++------
 xen/include/xen/pci.h              |  4 ++++
 3 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 27bdb71..187a9e7 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;
@@ -637,7 +640,15 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
     pdev->node = node;
 
     if ( info )
+    {
         pdev->info = *info;
+        /*
+         * VF's 'is_extfn' field is used to indicate whether its PF is an
+         * extended function.
+         */
+        if ( pdev->info.is_virtfn )
+            pdev->info.is_extfn = pf_is_extfn;
+    }
     else if ( !pdev->vf_rlen[0] )
     {
         unsigned int pos = pci_find_ext_capability(seg, bus, devfn,
diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
index 82040dd..9676471 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) ? pdev->info.physfn.devfn : 0;
     }
-    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..da1bd22 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -39,6 +39,10 @@
 #define PCI_SBDF3(s,b,df) ((((s) & 0xffff) << 16) | PCI_BDF2(b, df))
 
 struct pci_dev_info {
+    /*
+     * VF's 'is_extfn' field is used to indicate whether its PF is 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

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

[Tian, Kevin]

> From: Gao, Chao
> Sent: Monday, August 28, 2017 10:42 AM
> 
> 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 for a VF, if its PF
> isn't an extended function, the BDF of PF should be used. Otherwise
> the BDF of a traditional function in the same device with the PF
> should be used.
> 
> 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 integrated PF to a wrong
> VT-d unit.
> 
> This patch overrides VF 'is_extfn' field and uses this 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>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

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

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

[Chao Gao]

On Mon, Aug 28, 2017 at 02:16:18AM -0600, Jan Beulich wrote:
>>>> On 28.08.17 at 04:42, <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 for a VF, if its PF
>> isn't an extended function, the BDF of PF should be used. Otherwise
>> the BDF of a traditional function in the same device with the PF
>> should be used.
>> 
>> 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 integrated PF to a wrong
>> VT-d unit.
>> 
>> This patch overrides VF 'is_extfn' field and uses this 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>
>
>Acked-by: Jan Beulich <jbeulich@suse.com>
>albeit ...
>
>> --- 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) ? pdev->info.physfn.devfn : 0;
>
>... if I end up committing this and if I don't forget, I'll likely take the
>liberty to remove the pointless parentheses here.
>

Hi, Eric.

Could you test this patch again and give your Tested-by if it fixes the
problem you reported?

Thanks
Chao

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

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

[Jan Beulich]

>>> On 28.08.17 at 04:42, <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 for a VF, if its PF
> isn't an extended function, the BDF of PF should be used. Otherwise
> the BDF of a traditional function in the same device with the PF
> should be used.
> 
> 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 integrated PF to a wrong
> VT-d unit.
> 
> This patch overrides VF 'is_extfn' field and uses this 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>

Acked-by: Jan Beulich <jbeulich@suse.com>
albeit ...

> --- 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) ? pdev->info.physfn.devfn : 0;

... if I end up committing this and if I don't forget, I'll likely take the
liberty to remove the pointless parentheses here.

Jan


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

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

[Crawford, Eric R]

Sorry for the delay, it looks like this patch resolves the issue.

Tested-by: Crawford, Eric R <Eric.R.Crawford@intel.com>

-Eric

-----Original Message-----
From: Gao, Chao 
Sent: Monday, August 28, 2017 12:27 AM
To: Crawford, Eric R <eric.r.crawford@intel.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>; Wei Liu <wei.liu2@citrix.com>; George Dunlap <George.Dunlap@eu.citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>; Crawford, Eric R <eric.r.crawford@intel.com>; Tian, Kevin <kevin.tian@intel.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-devel@lists.xen.org; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Tim Deegan <tim@xen.org>; Jan Beulich <JBeulich@suse.com>
Subject: Re: [PATCH v10] VT-d: use correct BDF for VF to search VT-d unit

On Mon, Aug 28, 2017 at 02:16:18AM -0600, Jan Beulich wrote:
>>>> On 28.08.17 at 04:42, <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 for a 
>> VF, if its PF isn't an extended function, the BDF of PF should be 
>> used. Otherwise the BDF of a traditional function in the same device 
>> with the PF should be used.
>> 
>> 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 integrated PF to a wrong 
>> VT-d unit.
>> 
>> This patch overrides VF 'is_extfn' field and uses this 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>
>
>Acked-by: Jan Beulich <jbeulich@suse.com> albeit ...
>
>> --- 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) ? pdev->info.physfn.devfn : 
>> + 0;
>
>... if I end up committing this and if I don't forget, I'll likely take 
>the liberty to remove the pointless parentheses here.
>

Hi, Eric.

Could you test this patch again and give your Tested-by if it fixes the problem you reported?

Thanks
Chao

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