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

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Gao, Chao
> Sent: Friday, June 30, 2017 9:17 AM
> 
> The problem is for 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.
> 
> From SRIOV spec REV 1.0 section 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 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 and the latter shall support ARI. For Extended
> Functions, one traditional function's BDF should be used to search VT-d unit.
> And according to PCIe spec, Extened Function means within an ARI device, a
> Function whose Function Number is greater than 7. Thus, the former can't be
> an
> extended function, while the latter is as long as its devfn > 7, this check is
> exactly what the original code did; The original code wasn't aware the former.
> 
> This patch directly looks up the 'is_extfn' field of PF's struct pci_dev
> to decide whether the PF is a extended function.

Above description looks like the bug is caused by ARI problem. But
if you look at the original code (and the problem you described), it's
not related to ARI. ARI comes just when adding a clean fix, so please 
revise the description to make that part clear


> 
> Reported-by: Crawford, Eric R <Eric.R.Crawford@xxxxxxxxx>
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> ---
> v3:
>  - access pf's struct pci_pdev between pcidevs_lock() and pcidevs_unlock()

should be v4.

> ---
>  xen/drivers/passthrough/vtd/dmar.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/passthrough/vtd/dmar.c
> b/xen/drivers/passthrough/vtd/dmar.c
> index 82040dd..27ff471 100644
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -218,8 +218,17 @@ struct acpi_drhd_unit
> *acpi_find_matched_drhd_unit(const struct pci_dev *pdev)
>      }
>      else if ( pdev->info.is_virtfn )
>      {
> +        struct pci_dev *physfn;
> +
>          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);
> +        devfn = (physfn && physfn->info.is_extfn) ? 0 : pdev-
> >info.physfn.devfn;

is it legal to have physfn as NULL when is_virtfn is true?

> +        pcidevs_unlock();
>      }
>      else
>      {
> --
> 1.8.3.1


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