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

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

> From: Gao, Chao
> Sent: Tuesday, August 22, 2017 5:52 AM
> 
> 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 problematic for a corner case (a RC endpoint with SRIOV capability

it's not a corner case. It's "conceptually wrong" w/o checking is_extfn.

> and has its own VT-d unit), leading to matching to a wrong VT-d unit.
> 
> This patch reuses 'is_extfn' field in VF's struct pci_dev_info to indicate
> whether the PF of this VF is an extended function. The field helps to use
> correct BDF to search VT-d unit.

We should directly call "whether this VF is an extended function".

SR-IOV spec clearly says:

--
The ARI capability enables a Device to support up to 256 Functions - 
Functions, PFs, or VFs in any combination - associated with the 
captured Bus Number.
--

So a VF with function number >7 is also an extended function.

> 
> Reported-by: Crawford, Eric R <Eric.R.Crawford@xxxxxxxxx>
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> ---
> 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      | 6 ++++++
>  xen/drivers/passthrough/vtd/dmar.c | 2 +-
>  xen/include/xen/pci.h              | 4 ++++
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 27bdb71..2a91320 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -599,6 +599,7 @@ 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)
>          pdev_type = "device";
> @@ -608,6 +609,8 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>      {
>          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,
> @@ -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);
> 
>      ret = 0;
> diff --git a/xen/drivers/passthrough/vtd/dmar.c
> b/xen/drivers/passthrough/vtd/dmar.c
> index 82040dd..83ce5d4 100644
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -219,7 +219,7 @@ struct acpi_drhd_unit
> *acpi_find_matched_drhd_unit(const struct pci_dev *pdev)
>      else if ( pdev->info.is_virtfn )
>      {
>          bus = pdev->info.physfn.bus;
> -        devfn = PCI_SLOT(pdev->info.physfn.devfn) ? 0 : pdev-
> >info.physfn.devfn;
> +        devfn = pdev->info.is_extfn ? 0 : pdev->info.physfn.devfn;
>      }

If you looked at Linux side code, XEN_PCI_DEV_EXTFN is set
for both PF/VF, so you don't need check is_extfn specifically
within is_virtfn branch. checks of extended functions should
be constrained within is_extfn branch

>      else
>      {
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index 59b6e8a..3b0da66 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -39,6 +39,10 @@
>  #define PCI_SBDF3(s,b,df) ((((s) & 0xffff) << 16) | PCI_BDF2(b, df))
> 
>  struct pci_dev_info {
> +    /*
> +     * When 'is_virtfn' is set, 'is_extfn' is re-used to indicate whether
> +     * the PF of this VF is an extended function.
> +     */

this comment is meaningless then, since it does indicate whether
VF is extended function. :-)

>      bool_t is_extfn;
>      bool_t is_virtfn;
>      struct {
> --
> 1.8.3.1


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