Re: [PATCH v5 12/14] xen/arm: translate virtual PCI bus topology for guests

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, Nov 25, 2021 at 01:02:49PM +0200, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> There are three  originators for the PCI configuration space access:
> 1. The domain that owns physical host bridge: MMIO handlers are
> there so we can update vPCI register handlers with the values
> written by the hardware domain, e.g. physical view of the registers
> vs guest's view on the configuration space.
> 2. Guest access to the passed through PCI devices: we need to properly
> map virtual bus topology to the physical one, e.g. pass the configuration
> space access to the corresponding physical devices.
> 3. Emulated host PCI bridge access. It doesn't exist in the physical
> topology, e.g. it can't be mapped to some physical host bridge.
> So, all access to the host bridge itself needs to be trapped and
> emulated.

I'm kind of lost in this commit message. You are just adding a
translate function in order for domUs to translate from virtual SBDF
to the physical SBDF of the device. I realize you do that based on
whether 'bridge' is set or not, so I assume this is just a way to
signal whether the domain is a hardware domain or not. Ie:
!!bridge == is_hardware_domain(v->domain).

> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> ---
> Since v4:
> - indentation fixes
> - constify struct domain
> - updated commit message
> - updates to the new locking scheme (pdev->vpci_lock)
> Since v3:
> - revisit locking
> - move code to vpci.c
> Since v2:
>  - pass struct domain instead of struct vcpu
>  - constify arguments where possible
>  - gate relevant code with CONFIG_HAS_VPCI_GUEST_SUPPORT
> New in v2
> ---
>  xen/arch/arm/vpci.c     | 18 ++++++++++++++++++
>  xen/drivers/vpci/vpci.c | 27 +++++++++++++++++++++++++++
>  xen/include/xen/vpci.h  |  1 +
>  3 files changed, 46 insertions(+)
> 
> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
> index 8e801f275879..3d134f42d07e 100644
> --- a/xen/arch/arm/vpci.c
> +++ b/xen/arch/arm/vpci.c
> @@ -41,6 +41,15 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
>      /* data is needed to prevent a pointer cast on 32bit */
>      unsigned long data;
>  
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> +    /*
> +     * For the passed through devices we need to map their virtual SBDF
> +     * to the physical PCI device being passed through.
> +     */
> +    if ( !bridge && !vpci_translate_virtual_device(v->domain, &sbdf) )
> +        return 1;

I'm unsure what returning 1 implies for Arm here, but you likely need
to set '*r = ~0ul;'.

> +#endif
> +
>      if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
>                          1U << info->dabt.size, &data) )
>      {
> @@ -59,6 +68,15 @@ static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info,
>      struct pci_host_bridge *bridge = p;
>      pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
>  
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> +    /*
> +     * For the passed through devices we need to map their virtual SBDF
> +     * to the physical PCI device being passed through.
> +     */
> +    if ( !bridge && !vpci_translate_virtual_device(v->domain, &sbdf) )
> +        return 1;
> +#endif
> +
>      return vpci_ecam_write(sbdf, ECAM_REG_OFFSET(info->gpa),
>                             1U << info->dabt.size, r);
>  }
> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> index c2fb4d4db233..bdc8c63f73fa 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -195,6 +195,33 @@ static void vpci_remove_virtual_device(struct domain *d,
>      pdev->vpci->guest_sbdf.sbdf = ~0;
>  }
>  
> +/*
> + * Find the physical device which is mapped to the virtual device
> + * and translate virtual SBDF to the physical one.
> + */
> +bool vpci_translate_virtual_device(const struct domain *d, pci_sbdf_t *sbdf)
> +{
> +    struct pci_dev *pdev;
> +

I would add:

ASSERT(!is_hardware_domain(d));

To make sure this is not used for the hardware domain.

> +    for_each_pdev( d, pdev )
> +    {
> +        bool found;
> +
> +        spin_lock(&pdev->vpci_lock);
> +        found = pdev->vpci && (pdev->vpci->guest_sbdf.sbdf == sbdf->sbdf);
> +        spin_unlock(&pdev->vpci_lock);
> +
> +        if ( found )
> +        {
> +            /* Replace guest SBDF with the physical one. */
> +            *sbdf = pdev->sbdf;
> +            return true;
> +        }
> +    }
> +
> +    return false;
> +}
> +
>  /* Notify vPCI that device is assigned to guest. */
>  int vpci_assign_device(struct domain *d, struct pci_dev *pdev)
>  {
> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
> index e5258bd7ce90..21d76929391f 100644
> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -280,6 +280,7 @@ static inline void vpci_cancel_pending_locked(struct pci_dev *pdev)
>  /* Notify vPCI that device is assigned/de-assigned to/from guest. */
>  int vpci_assign_device(struct domain *d, struct pci_dev *pdev);
>  int vpci_deassign_device(struct domain *d, struct pci_dev *pdev);
> +bool vpci_translate_virtual_device(const struct domain *d, pci_sbdf_t *sbdf);
>  #else
>  static inline int vpci_assign_device(struct domain *d, struct pci_dev *pdev)
>  {

If you add a dummy vpci_translate_virtual_device helper that returns
false unconditionally here you could drop the #ifdefs in arm/vpci.c
AFAICT.

Thanks, Roger.