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