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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Thu, Jun 22, 2017 at 09:31:50AM -0600, Jan Beulich wrote:
> >>> On 22.06.17 at 16:21, <chao.gao@xxxxxxxxx> wrote:
> > On Thu, Jun 22, 2017 at 03:26:04AM -0600, Jan Beulich wrote:
> >>>>> On 21.06.17 at 12:47, <chao.gao@xxxxxxxxx> wrote:
> >>> The problem is 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.
> >>> 
> >>> To search VT-d unit for a VF, the BDF of the PF is used. And If the
> >>> PF is an Extended Function, the BDF of one traditional function is
> >>> used.  The following line (from acpi_find_matched_drhd_unit()):
> >>>     devfn = PCI_SLOT(pdev->info.physfn.devfn) ? 0 : 
> >>> pdev->info.physfn.devfn;
> >>> sets 'devfn' to 0 if PF's devfn > 7. Apparently, it treats all
> >>> PFs which has devfn > 7 as extended function. However, it is wrong for
> >>> a RC integrated PF, which is not ARI-capable but may have devfn > 7.
> >>
> >>I'm again having trouble with you talking about ARI and RC
> >>integrated here, but not checking for either in any way in the
> >>new code. Please make sure you establish the full connection
> >>in the description.
> > 
> > Sorry for this. Let me explain this again.
> > 
> > From SRIOV spec 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 I _think_ 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.
> > The latter shall support ARI. Only for extended functions, one
> > traditional function's BDF should be used to search VT-d unit. And
> > according to PCIE spec, Extended function means within an ARI Device, a
> > Function whose Function Number is greater than 7. So the former
> > can't be an extended function. The latter is an extended function as
> > long as PF's devfn > 7, this check is exactly what the original code
> > did. So I think the original code didn't aware the former
> > (aka, RC integrated endpoints.). This patch checks the is_extfn
> > directly. All of this is only my understanding. I need you and Kevin's
> > help to decide it's right or not.
> 
> This makes sense to me, but as said, the patch description will need
> to include this in some form.
> 
> >>> --- a/xen/drivers/passthrough/vtd/dmar.c
> >>> +++ b/xen/drivers/passthrough/vtd/dmar.c
> >>> @@ -218,8 +218,18 @@ struct acpi_drhd_unit 
> >>> *acpi_find_matched_drhd_unit(const 
> >>> struct pci_dev *pdev)
> >>>      }
> >>>      else if ( pdev->info.is_virtfn )
> >>>      {
> >>> +        struct pci_dev *physfn;
> >>
> >>const
> >>
> >>>          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);
> >>> +        pcidevs_unlock();
> >>> +        ASSERT(physfn);
> >>> +        devfn = physfn->info.is_extfn ? 0 : pdev->info.physfn.devfn;
> >>
> >>This change looks to be fine is we assume that is_extfn is always
> >>set correctly. Looking at the Linux code setting it, I'm not sure
> >>though: I can't see any connection to the PF needing to be RC
> >>integrated there.
> > 
> > Linux code sets it when
> >  pci_ari_enabled(pci_dev->bus) && PCI_SLOT(pci_dev->devfn)
> > 
> >  I _think_ pci_ari_enabled(pci_dev->bus) means ARIforwarding is enabled
> >  in the immediatedly upstream Downstream port. Thus, I think the pci_dev
> >  is an ARI-capable device for PCIe spec 6.13 says:
> > 
> > It is strongly recommended that software in general Set the ARI
> > Forwarding Enable bit in a 5 Downstream Port only if software is certain
> > that the device immediately below the Downstream Port is an ARI Device.
> > If the bit is Set when a non-ARI Device is present, the non-ARI Device
> > can respond to Configuration Space accesses under what it interprets as
> > being different Device Numbers, and its Functions can be aliased under
> > multiple Device Numbers, generally leading to undesired behavior.
> > 
> > and the pci_dev can't be a RC integrated endpoints. From another side, it
> > also means the is_extfn won't be set for RC integrated PF. Is that
> > right?
> 
> Well, I'm not sure about the Linux parts here? Konrad, do you
> happen to know? Or do you know someone who does?

Including Govinda and Venu,

> 
> >>I'd also suggest doing error handling not by ASSERT(), but by
> >>checking physfn in the conditional expression.
> > 
> > do you mean this：
> > devfn = (physfn && physfn->info.is_extfn) ? 0 : pdev->info.physfn.devfn;
> 
> Yes.
> 
> Jan

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