Re: [Xen-devel] [PATCH RESEND v9] VT-d: use correct BDF for VF to search VT-d unit

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.08.17 at 07:27, <chao.gao@xxxxxxxxx> wrote:
> When SR-IOV is enabled, 'Virtual Functions' of a 'Physical Function' are under
> the scope of the same VT-d unit as the 'Physical Function'. A 'Physical
> Function' can be a 'Traditional Function' or an ARI 'Extended Function'.
> And furthermore, 'Extended Functions' on an endpoint are under the scope of
> the same VT-d unit as the 'Traditional Functions' on the endpoint. To search
> VT-d unit, the BDF of PF or the BDF of a traditional function may be used. 
> And
> it depends on whether the PF is an extended function or not.
> 
> Current code uses PCI_SLOT() to recognize an ARI 'Extended Funcion'. But it
> is conceptually wrong w/o checking whether PF is an extended function and
> would lead to match VFs of a RC endpoint to a wrong VT-d unit.
> 
> This patch uses VF's 'is_extfn' field to indicate whether the PF of this VF 
> is
> an extended function. The field helps to use correct BDF to search VT-d unit.
> 
> Reported-by: Crawford, Eric R <Eric.R.Crawford@xxxxxxxxx>
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> ---
>  - RESEND for the previous email has no subject. 
> 
> v9:
>  - check 'is_virtfn' first in pci_add_device() to avoid potential error if
>  linux side sets VF's 'is_extfn'
>  - comments changes to make it clear that we use VF's 'is_extfn' 
> intentionally
>  otherwise the patch seems like a workaround.
> 
> v8:
>  - use "conceptually wrong", instead of "a corner case" in commit message
>  - check 'is_virtfn' first in acpi_find_matched_drhd_unit()
> 
> v7:
>  - Drop Eric's tested-by
>  - Change commit message to be clearer
>  - Re-use VF's is_extfn field
>  - access PF's is_extfn field in locked area
> 
> ---
>  xen/drivers/passthrough/pci.c      | 14 ++++++++++----
>  xen/drivers/passthrough/vtd/dmar.c | 12 ++++++------
>  xen/include/xen/pci.h              |  1 +
>  3 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 27bdb71..0e27e29 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -599,21 +599,24 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>      unsigned int slot = PCI_SLOT(devfn), func = PCI_FUNC(devfn);
>      const char *pdev_type;
>      int ret;
> +    bool pf_is_extfn = false;
>  
> -    if (!info)
> +    if ( !info )
>          pdev_type = "device";
> -    else if (info->is_extfn)
> -        pdev_type = "extended function";
> -    else if (info->is_virtfn)
> +    else if ( info->is_virtfn )
>      {
>          pcidevs_lock();
>          pdev = pci_get_pdev(seg, info->physfn.bus, info->physfn.devfn);
> +        if ( pdev )
> +            pf_is_extfn = pdev->info.is_extfn;
>          pcidevs_unlock();
>          if ( !pdev )
>              pci_add_device(seg, info->physfn.bus, info->physfn.devfn,
>                             NULL, node);
>          pdev_type = "virtual function";
>      }
> +    else if ( info->is_extfn )
> +        pdev_type = "extended function";
>      else
>      {
>          info = NULL;
> @@ -707,6 +710,9 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>                     seg, bus, slot, func, ctrl);
>      }
>  
> +    /* VF's 'is_extfn' is used to indicate whether PF is an extended 
> function */
> +    if ( pdev->info.is_virtfn )
> +        pdev->info.is_extfn = pf_is_extfn;
>      check_pdev(pdev);

Can this please be moved up right next to

        pdev->info = *info;

, so that information is right from the point it is being stored? And
looking at that code I can't really work out why the SR-IOV device
handling is in an "else if" to that path. I can't check that case
myself, as by box'es root ports don't support ARI forwarding, so
despite PF and VF being ARI-capable it can't be enabled, and
hence I'm not seeing the devices reported as extended functions.

Jan


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