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

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

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