Re: [PATCH v20 2/2] vpci: translate virtual PCI bus topology for guests

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

Hello,

Sorry I haven't looked at this before, I was confused with the cover
letter having ARM in the subject and somehow assumed all the code was
ARM related.

On Fri, Apr 18, 2025 at 02:58:37PM -0400, Stewart Hildebrand wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> 
> There are two 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.
> 
> In vpci_read(), use the access size to create a mask so as to not set
> any bits above the access size when returning an error.

I see this here, and the code below, yet I'm not following why this is
a requirement for the code added here.  It seems like an unrelated
change?

> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
> ---
> In v20:
> * call translate_virtual_device() from within locked context of
>   vpci_{read,write}
> * update commit message
> In v19:
> * move locking to pre-patch
> In v18:
> * address warning in vpci test suite
> In v17:
> * move lock to inside vpci_translate_virtual_device()
> * acks were previously given for Arm [0] and vPCI [1], but since it was
>   two releases ago and the patch has changed, I didn't pick them up
> 
> [0] 
> https://lore.kernel.org/xen-devel/4afe33f2-72e6-4755-98ce-d7f9da374e90@xxxxxxx/
> [1] https://lore.kernel.org/xen-devel/Zk70udmiriruMt0r@macbook/
> 
> In v15:
> - base on top of ("arm/vpci: honor access size when returning an error")
> In v11:
> - Fixed format issues
> - Added ASSERT_UNREACHABLE() to the dummy implementation of
> vpci_translate_virtual_device()
> - Moved variable in vpci_sbdf_from_gpa(), now it is easier to follow
> the logic in the function
> Since v9:
> - Commend about required lock replaced with ASSERT()
> - Style fixes
> - call to vpci_translate_virtual_device folded into vpci_sbdf_from_gpa
> Since v8:
> - locks moved out of vpci_translate_virtual_device()
> Since v6:
> - add pcidevs locking to vpci_translate_virtual_device
> - update wrt to the new locking scheme
> Since v5:
> - add vpci_translate_virtual_device for #ifndef CONFIG_HAS_VPCI_GUEST_SUPPORT
>   case to simplify ifdefery
> - add ASSERT(!is_hardware_domain(d)); to vpci_translate_virtual_device
> - reset output register on failed virtual SBDF translation
> 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
> ---
>  tools/tests/vpci/emul.h |  2 +-
>  xen/arch/arm/vpci.c     |  4 +++
>  xen/drivers/vpci/vpci.c | 73 +++++++++++++++++++++++++++++++++++++----
>  3 files changed, 71 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/tests/vpci/emul.h b/tools/tests/vpci/emul.h
> index da446bba86b4..dd048cffbf9d 100644
> --- a/tools/tests/vpci/emul.h
> +++ b/tools/tests/vpci/emul.h
> @@ -89,7 +89,7 @@ typedef union {
>  
>  #define __hwdom_init
>  
> -#define is_hardware_domain(d) ((void)(d), false)
> +#define is_hardware_domain(d) ((void)(d), true)
>  
>  #define has_vpci(d) true
>  
> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
> index b63a356bb4a8..618ddb7f6547 100644
> --- a/xen/arch/arm/vpci.c
> +++ b/xen/arch/arm/vpci.c
> @@ -34,6 +34,8 @@ 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;
>  
> +    ASSERT(!bridge == !is_hardware_domain(v->domain));
> +
>      if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
>                          1U << info->dabt.size, &data) )
>      {
> @@ -52,6 +54,8 @@ 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);
>  
> +    ASSERT(!bridge == !is_hardware_domain(v->domain));
> +
>      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 1e6aa5d799b9..fc409f3fc346 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -174,6 +174,41 @@ int vpci_assign_device(struct pci_dev *pdev)
>  }
>  #endif /* __XEN__ */
>  
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> +/*
> + * Find the physical device which is mapped to the virtual device
> + * and translate virtual SBDF to the physical one.
> + */
> +static const struct pci_dev *translate_virtual_device(const struct domain *d,
> +                                                      pci_sbdf_t *sbdf)
> +{
> +    const struct pci_dev *pdev;
> +
> +    ASSERT(!is_hardware_domain(d));
> +    ASSERT(rw_is_locked(&d->pci_lock));
> +
> +    for_each_pdev ( d, pdev )
> +    {
> +        if ( pdev->vpci && (pdev->vpci->guest_sbdf.sbdf == sbdf->sbdf) )
> +        {
> +            /* Replace guest SBDF with the physical one. */
> +            *sbdf = pdev->sbdf;
> +            return pdev;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +#else
> +static const struct pci_dev *translate_virtual_device(const struct domain *d,
> +                                                      pci_sbdf_t *sbdf)
> +{
> +    ASSERT_UNREACHABLE();
> +
> +    return NULL;
> +}
> +#endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */

Jan's suggestion avoids having duplicate headers, and results in less
code overall:

static const struct pci_dev *translate_virtual_device(const struct domain *d,
                                                      pci_sbdf_t *sbdf)
{
#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
    const struct pci_dev *pdev;
    ...
#else /* !CONFIG_HAS_VPCI_GUEST_SUPPORT */
    ASSERT_UNREACHABLE()
#endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */

    return NULL;
}

I've not overly fuzzed either way, but if changes are required you
might as well change to this form.

>  static int vpci_register_cmp(const struct vpci_register *r1,
>                               const struct vpci_register *r2)
>  {
> @@ -438,7 +473,7 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, 
> unsigned int size)
>      const struct pci_dev *pdev;
>      const struct vpci_register *r;
>      unsigned int data_offset = 0;
> -    uint32_t data = ~(uint32_t)0;
> +    uint32_t data = 0xffffffffU >> (32 - 8 * size);

This seems kind of unrelated to the rest of the code in the patch,
why is this needed?  Isn't it always fine to return all ones, and let
the caller truncate to the required size?

Otherwise the code in vpci_read_hw() also needs to be adjusted.  And
you can likely use GENMASK(size * 8, 0) as it's easier to read.

>  
>      if ( !size )
>      {
> @@ -453,9 +488,21 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, 
> unsigned int size)
>       * pci_lock is sufficient.
>       */
>      read_lock(&d->pci_lock);
> -    pdev = pci_get_pdev(d, sbdf);
> -    if ( !pdev && is_hardware_domain(d) )
> -        pdev = pci_get_pdev(dom_xen, sbdf);
> +    if ( is_hardware_domain(d) )
> +    {
> +        pdev = pci_get_pdev(d, sbdf);
> +        if ( !pdev )
> +            pdev = pci_get_pdev(dom_xen, sbdf);
> +    }
> +    else
> +    {
> +        pdev = translate_virtual_device(d, &sbdf);
> +        if ( !pdev )
> +        {
> +            read_unlock(&d->pci_lock);
> +            return data;
> +        }

You don't need this checking here, as the check below will already be
enough AFAICT, iow:

    if ( is_hardware_domain(d) )
    {
        pdev = pci_get_pdev(d, sbdf);
        if ( !pdev )
            pdev = pci_get_pdev(dom_xen, sbdf);
    }
    else
        pdev = translate_virtual_device(d, &sbdf);

    if ( !pdev || !pdev->vpci )
    {
        read_unlock(&d->pci_lock);
        return vpci_read_hw(sbdf, reg, size);
    }

Achieves the same and is more compact by having a single return for
all the possible cases?  Same for the write case below.

Thanks, Roger.