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

[Chao Gao <chao.gao@intel.com>]

On Thu, Jun 22, 2017 at 03:26:04AM -0600, Jan Beulich wrote:
>>>> On 21.06.17 at 12:47, <chao.gao@intel.com> 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.

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

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

Thanks
Chao

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