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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

I'd also suggest doing error handling not by ASSERT(), but by
checking physfn in the conditional expression.

Jan


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