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

[Chao Gao <chao.gao@xxxxxxxxx>]

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