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

[PATCH v4] 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.

From SRIOV spec REV 1.0 section 3.7.3, it says:
"ARI is not applicable to Root Complex integrated Endpoints; all other
SR-IOV Capable Devices (Devices that include at least one PF) shall
implement the ARI Capability in each Function.". So PFs can be classified to
two kinds: one is RC integrated PF and the other is non-RC integrated PF. The
former can't support ARI and the latter shall support ARI. For Extended
Functions, one traditional function's BDF should be used to search VT-d unit.
And according to PCIe spec, Extened Function means within an ARI device, a
Function whose Function Number is greater than 7. Thus, the former can't be an
extended function, while the latter is as long as its devfn > 7, this check is
exactly what the original code did; The original code wasn't aware the former.

This patch directly looks up the 'is_extfn' field of PF's struct pci_dev
to decide whether the PF is a extended function.

Reported-by: Crawford, Eric R <Eric.R.Crawford@intel.com>
Signed-off-by: Chao Gao <chao.gao@intel.com>
---
v3:
 - access pf's struct pci_pdev between pcidevs_lock() and pcidevs_unlock()
---
 xen/drivers/passthrough/vtd/dmar.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
index 82040dd..27ff471 100644
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -218,8 +218,17 @@ struct acpi_drhd_unit *acpi_find_matched_drhd_unit(const struct pci_dev *pdev)
     }
     else if ( pdev->info.is_virtfn )
     {
+        struct pci_dev *physfn;
+
         bus = pdev->info.physfn.bus;
-        devfn = PCI_SLOT(pdev->info.physfn.devfn) ? 0 : pdev->info.physfn.devfn;
+        /*
+         * Use 0 as 'devfn' to search VT-d unit when the physical function
+         * is an Extended Function.
+         */
+        pcidevs_lock();
+        physfn = pci_get_pdev(pdev->seg, bus, pdev->info.physfn.devfn);
+        devfn = (physfn && physfn->info.is_extfn) ? 0 : pdev->info.physfn.devfn;
+        pcidevs_unlock();
     }
     else
     {
-- 
1.8.3.1


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

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

[Tian, Kevin]

> From: Gao, Chao
> Sent: Friday, June 30, 2017 9:17 AM
> 
> 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.
> 
> From SRIOV spec REV 1.0 section 3.7.3, it says:
> "ARI is not applicable to Root Complex integrated Endpoints; all other
> SR-IOV Capable Devices (Devices that include at least one PF) shall
> implement the ARI Capability in each Function.". So PFs can be classified to
> two kinds: one is RC integrated PF and the other is non-RC integrated PF. The
> former can't support ARI and the latter shall support ARI. For Extended
> Functions, one traditional function's BDF should be used to search VT-d unit.
> And according to PCIe spec, Extened Function means within an ARI device, a
> Function whose Function Number is greater than 7. Thus, the former can't be
> an
> extended function, while the latter is as long as its devfn > 7, this check is
> exactly what the original code did; The original code wasn't aware the former.
> 
> This patch directly looks up the 'is_extfn' field of PF's struct pci_dev
> to decide whether the PF is a extended function.

Above description looks like the bug is caused by ARI problem. But
if you look at the original code (and the problem you described), it's
not related to ARI. ARI comes just when adding a clean fix, so please 
revise the description to make that part clear


> 
> Reported-by: Crawford, Eric R <Eric.R.Crawford@intel.com>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> ---
> v3:
>  - access pf's struct pci_pdev between pcidevs_lock() and pcidevs_unlock()

should be v4.

> ---
>  xen/drivers/passthrough/vtd/dmar.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/passthrough/vtd/dmar.c
> b/xen/drivers/passthrough/vtd/dmar.c
> index 82040dd..27ff471 100644
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -218,8 +218,17 @@ struct acpi_drhd_unit
> *acpi_find_matched_drhd_unit(const struct pci_dev *pdev)
>      }
>      else if ( pdev->info.is_virtfn )
>      {
> +        struct pci_dev *physfn;
> +
>          bus = pdev->info.physfn.bus;
> -        devfn = PCI_SLOT(pdev->info.physfn.devfn) ? 0 : pdev-
> >info.physfn.devfn;
> +        /*
> +         * Use 0 as 'devfn' to search VT-d unit when the physical function
> +         * is an Extended Function.
> +         */
> +        pcidevs_lock();
> +        physfn = pci_get_pdev(pdev->seg, bus, pdev->info.physfn.devfn);
> +        devfn = (physfn && physfn->info.is_extfn) ? 0 : pdev-
> >info.physfn.devfn;

is it legal to have physfn as NULL when is_virtfn is true?

> +        pcidevs_unlock();
>      }
>      else
>      {
> --
> 1.8.3.1


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

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

[Jan Beulich]

>>> "Tian, Kevin" <kevin.tian@intel.com> 06/30/17 11:20 AM >>>
>> From: Gao, Chao
>> Sent: Friday, June 30, 2017 9:17 AM
>> --- a/xen/drivers/passthrough/vtd/dmar.c
>> +++ b/xen/drivers/passthrough/vtd/dmar.c
>> @@ -218,8 +218,17 @@ struct acpi_drhd_unit
>> *acpi_find_matched_drhd_unit(const struct pci_dev *pdev)
>>      }
>>      else if ( pdev->info.is_virtfn )
>>      {
>> +        struct pci_dev *physfn;
>> +
>>          bus = pdev->info.physfn.bus;
>> -        devfn = PCI_SLOT(pdev->info.physfn.devfn) ? 0 : pdev-
>> >info.physfn.devfn;
>> +        /*
>> +         * Use 0 as 'devfn' to search VT-d unit when the physical function
>> +         * is an Extended Function.
>> +         */
>> +        pcidevs_lock();
>> +        physfn = pci_get_pdev(pdev->seg, bus, pdev->info.physfn.devfn);
>> +        devfn = (physfn && physfn->info.is_extfn) ? 0 : pdev-
>> >info.physfn.devfn;
>
>is it legal to have physfn as NULL when is_virtfn is true?

I had asked for the check to be there just to be on the safe side.

Jan


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

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

[Chao Gao]

On Fri, Jun 30, 2017 at 05:19:52PM +0800, Tian, Kevin wrote:
>> From: Gao, Chao
>> Sent: Friday, June 30, 2017 9:17 AM
>> 
>> 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.
>> 
>> From SRIOV spec REV 1.0 section 3.7.3, it says:
>> "ARI is not applicable to Root Complex integrated Endpoints; all other
>> SR-IOV Capable Devices (Devices that include at least one PF) shall
>> implement the ARI Capability in each Function.". So PFs can be classified to
>> two kinds: one is RC integrated PF and the other is non-RC integrated PF. The
>> former can't support ARI and the latter shall support ARI. For Extended
>> Functions, one traditional function's BDF should be used to search VT-d unit.
>> And according to PCIe spec, Extened Function means within an ARI device, a
>> Function whose Function Number is greater than 7. Thus, the former can't be
>> an
>> extended function, while the latter is as long as its devfn > 7, this check is
>> exactly what the original code did; The original code wasn't aware the former.
>> 
>> This patch directly looks up the 'is_extfn' field of PF's struct pci_dev
>> to decide whether the PF is a extended function.
>
>Above description looks like the bug is caused by ARI problem. But
>if you look at the original code (and the problem you described), it's
>not related to ARI. ARI comes just when adding a clean fix, so please 
>revise the description to make that part clear
>

How about this:

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, a traditional function's BDF should be
used to search VT-d unit. Previous code only checks whether Function
Number is greater than 7, without checking the prerequisite that the
function should be within an ARI device. This incurs wrongly using
traditional function's BDF when the PF is RC integrated and thus cannot
be within an ARI device.

Considering 'is_extfn' field of struct pci_dev has been passed down from
Domain0 to indicate whether the function is an extended function, this
patch just looks up that field of PF's struct pci_dev and adjust BDF
used to search VT-d unit accordingly. 

Thanks
Chao

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

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

[Tian, Kevin]

> From: Gao, Chao
> Sent: Monday, July 3, 2017 12:37 PM
> 
> On Fri, Jun 30, 2017 at 05:19:52PM +0800, Tian, Kevin wrote:
> >> From: Gao, Chao
> >> Sent: Friday, June 30, 2017 9:17 AM
> >>
> >> 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.
> >>
> >> From SRIOV spec REV 1.0 section 3.7.3, it says:
> >> "ARI is not applicable to Root Complex integrated Endpoints; all other
> >> SR-IOV Capable Devices (Devices that include at least one PF) shall
> >> implement the ARI Capability in each Function.". So PFs can be classified
> to
> >> two kinds: one is RC integrated PF and the other is non-RC integrated PF.
> The
> >> former can't support ARI and the latter shall support ARI. For Extended
> >> Functions, one traditional function's BDF should be used to search VT-d
> unit.
> >> And according to PCIe spec, Extened Function means within an ARI device,
> a
> >> Function whose Function Number is greater than 7. Thus, the former can't
> be
> >> an
> >> extended function, while the latter is as long as its devfn > 7, this check is
> >> exactly what the original code did; The original code wasn't aware the
> former.
> >>
> >> This patch directly looks up the 'is_extfn' field of PF's struct pci_dev
> >> to decide whether the PF is a extended function.
> >
> >Above description looks like the bug is caused by ARI problem. But
> >if you look at the original code (and the problem you described), it's
> >not related to ARI. ARI comes just when adding a clean fix, so please
> >revise the description to make that part clear
> >
> 
> How about this:
> 
> 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, a traditional function's BDF should be
> used to search VT-d unit. Previous code only checks whether Function
> Number is greater than 7, without checking the prerequisite that the

where did above check come from in original code? 

-        devfn = PCI_SLOT(pdev->info.physfn.devfn) ? 0 : pdev->info.physfn.devfn;

> function should be within an ARI device. This incurs wrongly using
> traditional function's BDF when the PF is RC integrated and thus cannot
> be within an ARI device.
> 
> Considering 'is_extfn' field of struct pci_dev has been passed down from
> Domain0 to indicate whether the function is an extended function, this
> patch just looks up that field of PF's struct pci_dev and adjust BDF
> used to search VT-d unit accordingly.
> 

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

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

[Chao Gao]

On Wed, Jul 05, 2017 at 10:46:39AM +0800, Tian, Kevin wrote:
>> From: Gao, Chao
>> Sent: Monday, July 3, 2017 12:37 PM
>> 
>> On Fri, Jun 30, 2017 at 05:19:52PM +0800, Tian, Kevin wrote:
>> >> From: Gao, Chao
>> >> Sent: Friday, June 30, 2017 9:17 AM
>> >>
>> >> 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.
>> >>
>> >> From SRIOV spec REV 1.0 section 3.7.3, it says:
>> >> "ARI is not applicable to Root Complex integrated Endpoints; all other
>> >> SR-IOV Capable Devices (Devices that include at least one PF) shall
>> >> implement the ARI Capability in each Function.". So PFs can be classified
>> to
>> >> two kinds: one is RC integrated PF and the other is non-RC integrated PF.
>> The
>> >> former can't support ARI and the latter shall support ARI. For Extended
>> >> Functions, one traditional function's BDF should be used to search VT-d
>> unit.
>> >> And according to PCIe spec, Extened Function means within an ARI device,
>> a
>> >> Function whose Function Number is greater than 7. Thus, the former can't
>> be
>> >> an
>> >> extended function, while the latter is as long as its devfn > 7, this check is
>> >> exactly what the original code did; The original code wasn't aware the
>> former.
>> >>
>> >> This patch directly looks up the 'is_extfn' field of PF's struct pci_dev
>> >> to decide whether the PF is a extended function.
>> >
>> >Above description looks like the bug is caused by ARI problem. But
>> >if you look at the original code (and the problem you described), it's
>> >not related to ARI. ARI comes just when adding a clean fix, so please
>> >revise the description to make that part clear
>> >
>> 
>> How about this:
>> 
>> 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, a traditional function's BDF should be
>> used to search VT-d unit. Previous code only checks whether Function
>> Number is greater than 7, without checking the prerequisite that the
>
>where did above check come from in original code? 
>
>-        devfn = PCI_SLOT(pdev->info.physfn.devfn) ? 0 : pdev->info.physfn.devfn;
>

Yes. It is the check I described. This line assigns 0 to 'devfn' if PF's
function number > 7. Otherwise, use PF's real devfn.

Thanks
Chao

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

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

[Tian, Kevin]

> From: Gao, Chao
> Sent: Wednesday, July 5, 2017 12:28 PM
> 
> On Wed, Jul 05, 2017 at 10:46:39AM +0800, Tian, Kevin wrote:
> >> From: Gao, Chao
> >> Sent: Monday, July 3, 2017 12:37 PM
> >>
> >> On Fri, Jun 30, 2017 at 05:19:52PM +0800, Tian, Kevin wrote:
> >> >> From: Gao, Chao
> >> >> Sent: Friday, June 30, 2017 9:17 AM
> >> >>
> >> >> 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.
> >> >>
> >> >> From SRIOV spec REV 1.0 section 3.7.3, it says:
> >> >> "ARI is not applicable to Root Complex integrated Endpoints; all other
> >> >> SR-IOV Capable Devices (Devices that include at least one PF) shall
> >> >> implement the ARI Capability in each Function.". So PFs can be
> classified
> >> to
> >> >> two kinds: one is RC integrated PF and the other is non-RC integrated
> PF.
> >> The
> >> >> former can't support ARI and the latter shall support ARI. For Extended
> >> >> Functions, one traditional function's BDF should be used to search VT-d
> >> unit.
> >> >> And according to PCIe spec, Extened Function means within an ARI
> device,
> >> a
> >> >> Function whose Function Number is greater than 7. Thus, the former
> can't
> >> be
> >> >> an
> >> >> extended function, while the latter is as long as its devfn > 7, this check
> is
> >> >> exactly what the original code did; The original code wasn't aware the
> >> former.
> >> >>
> >> >> This patch directly looks up the 'is_extfn' field of PF's struct pci_dev
> >> >> to decide whether the PF is a extended function.
> >> >
> >> >Above description looks like the bug is caused by ARI problem. But
> >> >if you look at the original code (and the problem you described), it's
> >> >not related to ARI. ARI comes just when adding a clean fix, so please
> >> >revise the description to make that part clear
> >> >
> >>
> >> How about this:
> >>
> >> 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, a traditional function's BDF should be
> >> used to search VT-d unit. Previous code only checks whether Function
> >> Number is greater than 7, without checking the prerequisite that the
> >
> >where did above check come from in original code?
> >
> >-        devfn = PCI_SLOT(pdev->info.physfn.devfn) ? 0 : pdev-
> >info.physfn.devfn;
> >
> 
> Yes. It is the check I described. This line assigns 0 to 'devfn' if PF's
> function number > 7. Otherwise, use PF's real devfn.
> 

sorry I overlooked PCI_SLOT. However your description is still about
the wrong behavior if PF is an extended function. You didn't explain
it's also wrong even when PF is not an extended function.

Thanks
Kevin

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

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

[Chao Gao]

On Wed, Jul 05, 2017 at 01:18:38PM +0800, Tian, Kevin wrote:
>> From: Gao, Chao
>> Sent: Wednesday, July 5, 2017 12:28 PM
>> 
>> On Wed, Jul 05, 2017 at 10:46:39AM +0800, Tian, Kevin wrote:
>> >> From: Gao, Chao
>> >> Sent: Monday, July 3, 2017 12:37 PM
>> >>
>> >> On Fri, Jun 30, 2017 at 05:19:52PM +0800, Tian, Kevin wrote:
>> >> >> From: Gao, Chao
>> >> >> Sent: Friday, June 30, 2017 9:17 AM
>> >> >>
>> >> >> 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.
>> >> >>
>> >> >> From SRIOV spec REV 1.0 section 3.7.3, it says:
>> >> >> "ARI is not applicable to Root Complex integrated Endpoints; all other
>> >> >> SR-IOV Capable Devices (Devices that include at least one PF) shall
>> >> >> implement the ARI Capability in each Function.". So PFs can be
>> classified
>> >> to
>> >> >> two kinds: one is RC integrated PF and the other is non-RC integrated
>> PF.
>> >> The
>> >> >> former can't support ARI and the latter shall support ARI. For Extended
>> >> >> Functions, one traditional function's BDF should be used to search VT-d
>> >> unit.
>> >> >> And according to PCIe spec, Extened Function means within an ARI
>> device,
>> >> a
>> >> >> Function whose Function Number is greater than 7. Thus, the former
>> can't
>> >> be
>> >> >> an
>> >> >> extended function, while the latter is as long as its devfn > 7, this check
>> is
>> >> >> exactly what the original code did; The original code wasn't aware the
>> >> former.
>> >> >>
>> >> >> This patch directly looks up the 'is_extfn' field of PF's struct pci_dev
>> >> >> to decide whether the PF is a extended function.
>> >> >
>> >> >Above description looks like the bug is caused by ARI problem. But
>> >> >if you look at the original code (and the problem you described), it's
>> >> >not related to ARI. ARI comes just when adding a clean fix, so please
>> >> >revise the description to make that part clear
>> >> >
>> >>
>> >> How about this:
>> >>
>> >> 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, a traditional function's BDF should be
>> >> used to search VT-d unit. Previous code only checks whether Function
>> >> Number is greater than 7, without checking the prerequisite that the
>> >
>> >where did above check come from in original code?
>> >
>> >-        devfn = PCI_SLOT(pdev->info.physfn.devfn) ? 0 : pdev-
>> >info.physfn.devfn;
>> >
>> 
>> Yes. It is the check I described. This line assigns 0 to 'devfn' if PF's
>> function number > 7. Otherwise, use PF's real devfn.
>> 
>
>sorry I overlooked PCI_SLOT. However your description is still about
>the wrong behavior if PF is an extended function. You didn't explain
>it's also wrong even when PF is not an extended function.
>

How about changing the second paragraph to:

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 > 7.
But the original code only checks the latter requirement, without
checking the former requirement. It incurs that a function whose Function
Number > 7 but which isn't within an ARI device (such as RC integrated
function with Function Number > 7) is wrongly classified to an extended
function and then we wrongly use 0 as 'devfn' to search VT-d unit for this
case.

Thanks
Chao

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

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

[Tian, Kevin]

> From: Gao, Chao
> Sent: Wednesday, July 5, 2017 3:57 PM
> 
> On Wed, Jul 05, 2017 at 01:18:38PM +0800, Tian, Kevin wrote:
> >> From: Gao, Chao
> >> Sent: Wednesday, July 5, 2017 12:28 PM
> >>
> >> On Wed, Jul 05, 2017 at 10:46:39AM +0800, Tian, Kevin wrote:
> >> >> From: Gao, Chao
> >> >> Sent: Monday, July 3, 2017 12:37 PM
> >> >>
> >> >> On Fri, Jun 30, 2017 at 05:19:52PM +0800, Tian, Kevin wrote:
> >> >> >> From: Gao, Chao
> >> >> >> Sent: Friday, June 30, 2017 9:17 AM
> >> >> >>
> >> >> >> 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.
> >> >> >>
> >> >> >> From SRIOV spec REV 1.0 section 3.7.3, it says:
> >> >> >> "ARI is not applicable to Root Complex integrated Endpoints; all
> other
> >> >> >> SR-IOV Capable Devices (Devices that include at least one PF) shall
> >> >> >> implement the ARI Capability in each Function.". So PFs can be
> >> classified
> >> >> to
> >> >> >> two kinds: one is RC integrated PF and the other is non-RC
> integrated
> >> PF.
> >> >> The
> >> >> >> former can't support ARI and the latter shall support ARI. For
> Extended
> >> >> >> Functions, one traditional function's BDF should be used to search
> VT-d
> >> >> unit.
> >> >> >> And according to PCIe spec, Extened Function means within an ARI
> >> device,
> >> >> a
> >> >> >> Function whose Function Number is greater than 7. Thus, the
> former
> >> can't
> >> >> be
> >> >> >> an
> >> >> >> extended function, while the latter is as long as its devfn > 7, this
> check
> >> is
> >> >> >> exactly what the original code did; The original code wasn't aware
> the
> >> >> former.
> >> >> >>
> >> >> >> This patch directly looks up the 'is_extfn' field of PF's struct pci_dev
> >> >> >> to decide whether the PF is a extended function.
> >> >> >
> >> >> >Above description looks like the bug is caused by ARI problem. But
> >> >> >if you look at the original code (and the problem you described), it's
> >> >> >not related to ARI. ARI comes just when adding a clean fix, so please
> >> >> >revise the description to make that part clear
> >> >> >
> >> >>
> >> >> How about this:
> >> >>
> >> >> 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, a traditional function's BDF should be
> >> >> used to search VT-d unit. Previous code only checks whether Function
> >> >> Number is greater than 7, without checking the prerequisite that the
> >> >
> >> >where did above check come from in original code?
> >> >
> >> >-        devfn = PCI_SLOT(pdev->info.physfn.devfn) ? 0 : pdev-
> >> >info.physfn.devfn;
> >> >
> >>
> >> Yes. It is the check I described. This line assigns 0 to 'devfn' if PF's
> >> function number > 7. Otherwise, use PF's real devfn.
> >>
> >
> >sorry I overlooked PCI_SLOT. However your description is still about
> >the wrong behavior if PF is an extended function. You didn't explain
> >it's also wrong even when PF is not an extended function.
> >
> 
> How about changing the second paragraph to:
> 
> 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 > 7.
> But the original code only checks the latter requirement, without
> checking the former requirement. It incurs that a function whose Function
> Number > 7 but which isn't within an ARI device (such as RC integrated
> function with Function Number > 7) is wrongly classified to an extended
> function and then we wrongly use 0 as 'devfn' to search VT-d unit for this
> case.
> 

good to me.

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

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

[Jan Beulich]

>>> On 05.07.17 at 09:56, <chao.gao@intel.com> wrote:
> How about changing the second paragraph to:
> 
> 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 > 7.
> But the original code only checks the latter requirement, without
> checking the former requirement. It incurs that a function whose Function
> Number > 7 but which isn't within an ARI device (such as RC integrated
> function with Function Number > 7) is wrongly classified to an extended
> function and then we wrongly use 0 as 'devfn' to search VT-d unit for this
> case.

There's one part here which I continue to not understand: The
function number being just 3 bits, how can it possibly be larger
than 7?

Jan


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

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

[Roger Pau Monné]

On Wed, Jul 05, 2017 at 02:19:17AM -0600, Jan Beulich wrote:
> >>> On 05.07.17 at 09:56, <chao.gao@intel.com> wrote:
> > How about changing the second paragraph to:
> > 
> > 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 > 7.
> > But the original code only checks the latter requirement, without
> > checking the former requirement. It incurs that a function whose Function
> > Number > 7 but which isn't within an ARI device (such as RC integrated
> > function with Function Number > 7) is wrongly classified to an extended
> > function and then we wrongly use 0 as 'devfn' to search VT-d unit for this
> > case.
> 
> There's one part here which I continue to not understand: The
> function number being just 3 bits, how can it possibly be larger
> than 7?

It's a special case on the PCIe spec, quoting it:

"Note: for Requests targeting Extended Functions in an ARI Device,
A[19:12] represents the (8-bit) Function Number, which replaces the
(5-bit) Device Number and (3-bit) Function Number fields above."

It's in the PCIe 3.1a spec, page 657. The function number is expanded
from 7 to 255.

What I fail to see is how this device is registered with Xen, is the
devfn field used to store the function number only?

Roger.

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

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

[Chao Gao]

On Wed, Jul 05, 2017 at 09:29:29AM +0100, Roger Pau Monné wrote:
>On Wed, Jul 05, 2017 at 02:19:17AM -0600, Jan Beulich wrote:
>> >>> On 05.07.17 at 09:56, <chao.gao@intel.com> wrote:
>> > How about changing the second paragraph to:
>> > 
>> > 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 > 7.
>> > But the original code only checks the latter requirement, without
>> > checking the former requirement. It incurs that a function whose Function
>> > Number > 7 but which isn't within an ARI device (such as RC integrated
>> > function with Function Number > 7) is wrongly classified to an extended
>> > function and then we wrongly use 0 as 'devfn' to search VT-d unit for this
>> > case.
>> 
>> There's one part here which I continue to not understand: The
>> function number being just 3 bits, how can it possibly be larger
>> than 7?
>
>It's a special case on the PCIe spec, quoting it:
>
>"Note: for Requests targeting Extended Functions in an ARI Device,
>A[19:12] represents the (8-bit) Function Number, which replaces the
>(5-bit) Device Number and (3-bit) Function Number fields above."
>
>It's in the PCIe 3.1a spec, page 657. The function number is expanded
>from 7 to 255.
>
>What I fail to see is how this device is registered with Xen, is the
>devfn field used to store the function number only?

The devfn is passed down by dom0. Judging from how is_extfn is set in
xen_add_device() in linux kernel, I think linux kernel reuses the Device
Fields as the higher 5 bits of an extended function's Function Number. 

Thanks
Chao

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