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

[Chao Gao <chao.gao@xxxxxxxxx>]

On Mon, Jun 19, 2017 at 01:43:25AM -0600, Jan Beulich wrote:
>>>> On 19.06.17 at 08:33, <chao.gao@xxxxxxxxx> wrote:
>> On Fri, Jun 16, 2017 at 09:52:11AM -0600, Jan Beulich wrote:
>>>>>> On 16.06.17 at 08:48, <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 > 8.
>>>
>>>Is that really the relevant line? Since you say PF is an Extended
>>>Function, wouldn't
>>>
>>>    if ( pdev->info.is_extfn )
>>>    {
>>>        bus = pdev->bus;
>>>        devfn = 0;
>>>    }
>>>
>>>be the relevant code? Or else - is is_extfn not being set correctly?
>> 
>> I think this field is not being set for VF. And here what we want to
>> know is whether the PF of this VF is an extended functin. We also can add
>> a new field 'is_extfn' in pdev->info.physfn and change the caller in
>> linux kernel accordingly. But it will be not compatible with the old kernel.
>
>Wait, no - I did describe things slightly wrongly, and hence perhaps
>managed to confuse you (besides myself). For the VF we don't want
>to see is_extfn set, but for its PF I'd expect that to be the case.
>With that I'd then think looking up the struct pci_dev for the PF is all
>it takes to tell apart both cases, the more that I'm not sure ...

Impressive!

>
>>>> --- a/xen/drivers/passthrough/vtd/dmar.c
>>>> +++ b/xen/drivers/passthrough/vtd/dmar.c
>>>> @@ -219,7 +219,12 @@ struct acpi_drhd_unit 
>>>> *acpi_find_matched_drhd_unit(const struct pci_dev *pdev)
>>>>      else if ( pdev->info.is_virtfn )
>>>>      {
>>>>          bus = pdev->info.physfn.bus;
>>>> -        devfn = PCI_SLOT(pdev->info.physfn.devfn) ? 0 : 
>>>> pdev->info.physfn.devfn;
>>>> +        /* ARI is not appliable to Root Complex Integrated Endpoints */
>>>> +        if ( PCI_SLOT(pdev->info.physfn.devfn) &&
>>>> +             (pdev->type != DEV_TYPE_RC_ENDPOINT) )
>
>... checking VF's type (instead of PF's) here is appropriate / most
>compatible.

It was a mistake. Sorry for this.

>
>>>> +            devfn = 0;
>>>> +        else
>>>> +            devfn = pdev->info.physfn.devfn;
>>>>      }
>>>
>>>While I think I understand some of PCI, I have to admit that the
>>>connection to ARI is not at all obvious to me at this place in the
>>>sources. Hence I'd appreciate if you would extend the comment.
>> 
>> Ok. How about this:
>> 
>> -        devfn = PCI_SLOT(pdev->info.physfn.devfn) ? 0 : 
>> pdev->info.physfn.devfn;
>> +        /* 
>> +         * Use 0 as the devfn to search VT-d unit when the PF is an Extended
>> +         * Function (means within an ARI Device, a Function whose Function 
>> Number
>> +         * is greater than 7).
>> +         */
>> +        if ( PCI_SLOT(pdev->info.physfn.devfn) &&
>> +             (pci_find_ext_capability(pdev->seg, bus,
>> +                         pdev->info.physfn.devfn, PCI_EXT_CAP_ID_ARI)) )
>> +            devfn = 0;
>> +        else
>> +            devfn = pdev->info.physfn.devfn;
>
>That's better, but I'm still having some difficulty. In the Linux kernel
>we have
>
>       if (pci_dev->is_virtfn) {
>               ...
>       } else
>       if (pci_ari_enabled(pci_dev->bus) && PCI_SLOT(pci_dev->devfn))
>               add->flags = XEN_PCI_DEV_EXTFN;
>
>which tells me that the mere presence of an ARI capability may not
>be enough. Furthermore Linux checks whether devfn is zero in
>pci_configure_ari(), not whether PCI_SLOT(devfn) is non-zero -
>wouldn't that mean you want to pass a zero devfn to
>pci_find_ext_capability() above?

No. pci_configure_ari() is to enable ARI forwarding, which is a feature
of downstream port. ARI capability is a feature of device.
Anyhow, I finally understand what you said, which is actually what I
want. If I can easily handle the _pcidevs_lock to call pci_get_pdev(), it is the
best solution I think.

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