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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

Jan


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