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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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