[PATCH v6] VT-d: fix VF of RC integrated PF matched to wrong VT-d unit

[PATCH v6] VT-d: fix VF of RC integrated PF matched to wrong VT-d unit

[Chao Gao]

The problem is for a VF of RC integrated PF (e.g. PF's BDF is
00:02.0), we would wrongly use 00:00.0 to search VT-d unit.

If a PF is an extended function, the BDF of a traditional function
within the same device should be used to search VT-d unit. Otherwise,
the real BDF of PF should be used. According PCI-e spec, an extended
function is a function within an ARI device and Function Number is
greater than 7. The original code tried to tell apart Extended
Function and non-Extended Function through checking PCI_SLOT(),
missing counterpart of pci_ari_enabled() (this function exists in
linux kernel) compared to linux kernel. Without checking whether ARI
is enabled, it incurs a RC integrated PF with PCI_SLOT() >0 is wrongly
classified to an extended function. Note that a RC integrated function
isn't within an ARI device and thus cannot be extended function and in
this case the real BDF should be used.

This patch introduces a new field, pf_is_extfn, in struct
pci_dev_info, to indicate whether the physical function is an extended
function. The new field helps to generate correct BDF to search VT-d
unit.

Reported-by: Crawford, Eric R <Eric.R.Crawford@intel.com>
Tested-by: Crawford, Eric R <Eric.R.Crawford@intel.com>
Signed-off-by: Chao Gao <chao.gao@intel.com>
---
 xen/drivers/passthrough/pci.c      | 6 +++++-
 xen/drivers/passthrough/vtd/dmar.c | 2 +-
 xen/include/xen/pci.h              | 1 +
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 27bdb71..8c2ba33 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -599,6 +599,7 @@ 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)
         pdev_type = "device";
@@ -609,7 +610,9 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
         pcidevs_lock();
         pdev = pci_get_pdev(seg, info->physfn.bus, info->physfn.devfn);
         pcidevs_unlock();
-        if ( !pdev )
+        if ( pdev )
+            pf_is_extfn = pdev->info.is_extfn;
+        else
             pci_add_device(seg, info->physfn.bus, info->physfn.devfn,
                            NULL, node);
         pdev_type = "virtual function";
@@ -707,6 +710,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
                    seg, bus, slot, func, ctrl);
     }
 
+    pdev->info.pf_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..a96558f 100644
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -219,7 +219,7 @@ struct acpi_drhd_unit *acpi_find_matched_drhd_unit(const struct pci_dev *pdev)
     else if ( pdev->info.is_virtfn )
     {
         bus = pdev->info.physfn.bus;
-        devfn = PCI_SLOT(pdev->info.physfn.devfn) ? 0 : pdev->info.physfn.devfn;
+        devfn = pdev->info.pf_is_extfn ? 0 : pdev->info.physfn.devfn;
     }
     else
     {
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 59b6e8a..9e76aa0 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -40,6 +40,7 @@
 
 struct pci_dev_info {
     bool_t is_extfn;
+    bool_t pf_is_extfn; /* Only valid for virtual function */
     bool_t is_virtfn;
     struct {
         u8 bus;
-- 
1.8.3.1


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

Re: [patch v6] vt-d: fix vf of rc integrated pf matched to wrong vt-d unit

[Roger Pau Monné]

on wed, aug 16, 2017 at 01:12:24pm +0800, chao gao wrote:
> the problem is for a vf of rc integrated pf (e.g. pf's bdf is
> 00:02.0), we would wrongly use 00:00.0 to search vt-d unit.
> 
> if a pf is an extended function, the bdf of a traditional function
> within the same device should be used to search vt-d unit. otherwise,
> the real bdf of pf should be used. according pci-e spec, an extended
> function is a function within an ari device and function number is
> greater than 7.

AFAIK, extended functions simply remove the slot and extend the
function number to [0, 255], so it seems correct to expect that the
VT-d unit search should be done using the bus and extended function
parameters, and assume slot is 0. Is this some kind of limitation of
VT-d?

> The original code tried to tell apart Extended
> Function and non-Extended Function through checking PCI_SLOT(),
> missing counterpart of pci_ari_enabled() (this function exists in
> linux kernel) compared to linux kernel. Without checking whether ARI
> is enabled, it incurs a RC integrated PF with PCI_SLOT() >0 is wrongly
> classified to an extended function. Note that a RC integrated function
> isn't within an ARI device and thus cannot be extended function and in
> this case the real BDF should be used.
> 
> This patch introduces a new field, pf_is_extfn, in struct
> pci_dev_info, to indicate whether the physical function is an extended
> function. The new field helps to generate correct BDF to search VT-d
> unit.

[...]
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index 59b6e8a..9e76aa0 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -40,6 +40,7 @@
>  
>  struct pci_dev_info {
>      bool_t is_extfn;
> +    bool_t pf_is_extfn; /* Only valid for virtual function */

Can't you just re-use is_virtfn and is_extfn, and when both are true
it means the pf where this vf belongs is an extended function?

>      bool_t is_virtfn;
>      struct {
>          u8 bus;
> -- 
> 1.8.3.1
> 

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

Re: [patch v6] vt-d: fix vf of rc integrated pf matched to wrong vt-d unit

[Chao Gao]

On Wed, Aug 16, 2017 at 09:17:46AM +0100, Roger Pau Monné wrote:
>on wed, aug 16, 2017 at 01:12:24pm +0800, chao gao wrote:
>> the problem is for a vf of rc integrated pf (e.g. pf's bdf is
>> 00:02.0), we would wrongly use 00:00.0 to search vt-d unit.
>> 
>> if a pf is an extended function, the bdf of a traditional function
>> within the same device should be used to search vt-d unit. otherwise,
>> the real bdf of pf should be used. according pci-e spec, an extended
>> function is a function within an ari device and function number is
>> greater than 7.
>
>AFAIK, extended functions simply remove the slot and extend the
>function number to [0, 255], so it seems correct to expect that the
>VT-d unit search should be done using the bus and extended function
>parameters, and assume slot is 0. Is this some kind of limitation of
>VT-d?

VT-d spec makes such provision for VT-d unit search without any
explaination. But I think it isn't. Whether we can find the right VT-d unit
depends on DMAR. So I would rather regard it as firmware doesn't prepare
entries for extended functions in DMAR.

>
>> The original code tried to tell apart Extended
>> Function and non-Extended Function through checking PCI_SLOT(),
>> missing counterpart of pci_ari_enabled() (this function exists in
>> linux kernel) compared to linux kernel. Without checking whether ARI
>> is enabled, it incurs a RC integrated PF with PCI_SLOT() >0 is wrongly
>> classified to an extended function. Note that a RC integrated function
>> isn't within an ARI device and thus cannot be extended function and in
>> this case the real BDF should be used.
>> 
>> This patch introduces a new field, pf_is_extfn, in struct
>> pci_dev_info, to indicate whether the physical function is an extended
>> function. The new field helps to generate correct BDF to search VT-d
>> unit.
>
>[...]
>> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
>> index 59b6e8a..9e76aa0 100644
>> --- a/xen/include/xen/pci.h
>> +++ b/xen/include/xen/pci.h
>> @@ -40,6 +40,7 @@
>>  
>>  struct pci_dev_info {
>>      bool_t is_extfn;
>> +    bool_t pf_is_extfn; /* Only valid for virtual function */
>
>Can't you just re-use is_virtfn and is_extfn, and when both are true
>it means the pf where this vf belongs is an extended function?

Yes. Reuse vf's is_extfn field is possible.

Thanks
Chao

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

Re: [patch v6] vt-d: fix vf of rc integrated pf matched to wrong vt-d unit

[Roger Pau Monné]

On Wed, Aug 16, 2017 at 04:50:51PM +0800, Chao Gao wrote:
> On Wed, Aug 16, 2017 at 09:17:46AM +0100, Roger Pau Monné wrote:
> >on wed, aug 16, 2017 at 01:12:24pm +0800, chao gao wrote:
> >> the problem is for a vf of rc integrated pf (e.g. pf's bdf is
> >> 00:02.0), we would wrongly use 00:00.0 to search vt-d unit.
> >> 
> >> if a pf is an extended function, the bdf of a traditional function
> >> within the same device should be used to search vt-d unit. otherwise,
> >> the real bdf of pf should be used. according pci-e spec, an extended
> >> function is a function within an ari device and function number is
> >> greater than 7.
> >
> >AFAIK, extended functions simply remove the slot and extend the
> >function number to [0, 255], so it seems correct to expect that the
> >VT-d unit search should be done using the bus and extended function
> >parameters, and assume slot is 0. Is this some kind of limitation of
> >VT-d?
> 
> VT-d spec makes such provision for VT-d unit search without any
> explaination. But I think it isn't. Whether we can find the right VT-d unit
> depends on DMAR. So I would rather regard it as firmware doesn't prepare
> entries for extended functions in DMAR.

Looking again at the proposed changes in acpi_find_matched_drhd_unit,
it seems fine. If the VF belongs to a PF that uses extended functions
just use 0 as devfn, which is the devfn of the PF itself unless I'm
mistaken. It's just that I find the commit log very hard to
parse/understand.

Roger.

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

Re: [PATCH v6] VT-d: fix VF of RC integrated PF matched to wrong VT-d unit

[Tian, Kevin]

> From: Gao, Chao
> Sent: Wednesday, August 16, 2017 1:12 PM
> 
> The problem is for a VF of RC integrated PF (e.g. PF's BDF is
> 00:02.0), we would wrongly use 00:00.0 to search VT-d unit.
> 
> If a PF is an extended function, the BDF of a traditional function
> within the same device should be used to search VT-d unit. Otherwise,
> the real BDF of PF should be used. According PCI-e spec, an extended
> function is a function within an ARI device and Function Number is
> greater than 7. The original code tried to tell apart Extended
> Function and non-Extended Function through checking PCI_SLOT(),
> missing counterpart of pci_ari_enabled() (this function exists in
> linux kernel) compared to linux kernel. Without checking whether ARI
> is enabled, it incurs a RC integrated PF with PCI_SLOT() >0 is wrongly
> classified to an extended function. Note that a RC integrated function
> isn't within an ARI device and thus cannot be extended function and in
> this case the real BDF should be used.
> 
> This patch introduces a new field, pf_is_extfn, in struct
> pci_dev_info, to indicate whether the physical function is an extended
> function. The new field helps to generate correct BDF to search VT-d
> unit.
> 
> Reported-by: Crawford, Eric R <Eric.R.Crawford@intel.com>
> Tested-by: Crawford, Eric R <Eric.R.Crawford@intel.com>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> ---
>  xen/drivers/passthrough/pci.c      | 6 +++++-
>  xen/drivers/passthrough/vtd/dmar.c | 2 +-
>  xen/include/xen/pci.h              | 1 +
>  3 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 27bdb71..8c2ba33 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -599,6 +599,7 @@ 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)
>          pdev_type = "device";
> @@ -609,7 +610,9 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>          pcidevs_lock();
>          pdev = pci_get_pdev(seg, info->physfn.bus, info->physfn.devfn);
>          pcidevs_unlock();
> -        if ( !pdev )
> +        if ( pdev )
> +            pf_is_extfn = pdev->info.is_extfn;

besides Roger's comment, can you move above 2 lines inside lock
protection?

> +        else
>              pci_add_device(seg, info->physfn.bus, info->physfn.devfn,
>                             NULL, node);
>          pdev_type = "virtual function";
> @@ -707,6 +710,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>                     seg, bus, slot, func, ctrl);
>      }
> 
> +    pdev->info.pf_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..a96558f 100644
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -219,7 +219,7 @@ struct acpi_drhd_unit
> *acpi_find_matched_drhd_unit(const struct pci_dev *pdev)
>      else if ( pdev->info.is_virtfn )
>      {
>          bus = pdev->info.physfn.bus;
> -        devfn = PCI_SLOT(pdev->info.physfn.devfn) ? 0 : pdev-
> >info.physfn.devfn;
> +        devfn = pdev->info.pf_is_extfn ? 0 : pdev->info.physfn.devfn;
>      }
>      else
>      {
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index 59b6e8a..9e76aa0 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -40,6 +40,7 @@
> 
>  struct pci_dev_info {
>      bool_t is_extfn;
> +    bool_t pf_is_extfn; /* Only valid for virtual function */
>      bool_t is_virtfn;
>      struct {
>          u8 bus;
> --
> 1.8.3.1


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

Re: [PATCH v6] VT-d: fix VF of RC integrated PF matched to wrong VT-d unit

[Chao Gao]

On Thu, Aug 17, 2017 at 03:43:07PM +0800, Tian, Kevin wrote:
>> From: Gao, Chao
>> Sent: Wednesday, August 16, 2017 1:12 PM
>> 
>> The problem is for a VF of RC integrated PF (e.g. PF's BDF is
>> 00:02.0), we would wrongly use 00:00.0 to search VT-d unit.
>> 
>> If a PF is an extended function, the BDF of a traditional function
>> within the same device should be used to search VT-d unit. Otherwise,
>> the real BDF of PF should be used. According PCI-e spec, an extended
>> function is a function within an ARI device and Function Number is
>> greater than 7. The original code tried to tell apart Extended
>> Function and non-Extended Function through checking PCI_SLOT(),
>> missing counterpart of pci_ari_enabled() (this function exists in
>> linux kernel) compared to linux kernel. Without checking whether ARI
>> is enabled, it incurs a RC integrated PF with PCI_SLOT() >0 is wrongly
>> classified to an extended function. Note that a RC integrated function
>> isn't within an ARI device and thus cannot be extended function and in
>> this case the real BDF should be used.
>> 
>> This patch introduces a new field, pf_is_extfn, in struct
>> pci_dev_info, to indicate whether the physical function is an extended
>> function. The new field helps to generate correct BDF to search VT-d
>> unit.
>> 
>> Reported-by: Crawford, Eric R <Eric.R.Crawford@intel.com>
>> Tested-by: Crawford, Eric R <Eric.R.Crawford@intel.com>
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> ---
>>  xen/drivers/passthrough/pci.c      | 6 +++++-
>>  xen/drivers/passthrough/vtd/dmar.c | 2 +-
>>  xen/include/xen/pci.h              | 1 +
>>  3 files changed, 7 insertions(+), 2 deletions(-)
>> 
>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>> index 27bdb71..8c2ba33 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -599,6 +599,7 @@ 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)
>>          pdev_type = "device";
>> @@ -609,7 +610,9 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>          pcidevs_lock();
>>          pdev = pci_get_pdev(seg, info->physfn.bus, info->physfn.devfn);
>>          pcidevs_unlock();
>> -        if ( !pdev )
>> +        if ( pdev )
>> +            pf_is_extfn = pdev->info.is_extfn;
>
>besides Roger's comment, can you move above 2 lines inside lock
>protection?
>

Hi, Kevin and Roger.

I sent out a new version recently. The new version adopts all your
suggestions. Please review it.

Thanks
Chao

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