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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.06.17 at 12:51, <chao.gao@xxxxxxxxx> wrote:
> 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 ...
> 
> Hi, Jan. in pci_add_device():
> 
>     else if (info->is_virtfn)
>     {
>         pcidevs_lock();
>         pdev = pci_get_pdev(seg, info->physfn.bus, info->physfn.devfn);
>         pcidevs_unlock();
>         if ( !pdev )
>             pci_add_device(seg, info->physfn.bus, info->physfn.devfn,
>                            NULL, node);
>         pdev_type = "virtual function";
>     }
> 
> could you recall in which case, we can't get the PF by
> pci_get_pdev() above?

This is just a safety measure to make sure we have a PF device
to refer to (and we don't want to return failure because of that).
I'm not aware of the path actually being needed (and even if it
was taken, I'd expect a subsequent hypercall to report the PF).

Jan


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