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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Jul 06, 2017 at 03:57:37PM +0800, Chao Gao wrote:
> 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.
> 
> If a PF is an extended function, the BDF of a traditional function within the
> same device should be used to search VT-d unit. Otherwise, the real BDF of PF
> should be used. According PCI-e spec, an extended function is a function
> within an ARI device and Function Number is greater than 7. The original code
> tried to tell apart them through checking PCI_SLOT(), missing counterpart of
> pci_ari_enabled() (this function exists in linux kernel) compared to linux
> kernel. Without checking whether ARI is enabled, it incurs a RC integrated PF
> with PCI_SLOT() >0 is wrongly classified to an extended function. Note that a
> RC integrated function isn't within an ARI device and thus cannot be extended
> function and in this case the real BDF should be used.
> 
> Considering 'is_extfn' field of struct pci_dev has been passed down from
> Domain0 to indicate whether the function is an extended function, this patch
> just looks up the 'is_extfn' field of PF's struct pci_dev and set 'devfn' to 0
> when 'is_extfn' is true.
> 
> Reported-by: Crawford, Eric R <Eric.R.Crawford@xxxxxxxxx>
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> ---
> v5:
>  - Commit description change.
> v4:
>  - access pf's struct pci_pdev between pcidevs_lock() and pcidevs_unlock()
> 
> ---
>  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..8724f0a 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;
> +        pcidevs_unlock();

Just as a note (not that I intend you to fix this), but AFAICT this
function should be called holding the pcidevs lock, or else there's
the risk that the pdev argument is freed while poking at it.

Roger.

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