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

[Venu Busireddy <venu.busireddy@xxxxxxxxxx>]

On 2017-06-22 11:52:50 -0400, Konrad Rzeszutek Wilk wrote:
> 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?

pci_ari_enabled() and related code trusts that an RC integrated endpoint
does not present the PCI_EXT_CAP_ID_ARI capability. As long as we do
not have rogue endpoints that don't follow the spec, this code works fine.

> 
> 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