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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 5/6/25 07:16, Roger Pau Monné wrote:
> 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.

No worries, thanks for taking a look.

> 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?

See below

>> 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.

Will do

>>  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.

On Arm, since 9a5e22b64266 ("xen/arm: check read handler behavior") we
assert that the read handlers don't set any bits above the access size.
I had adjusted data here due to returning it directly from vpci_read()
in the current form of the patch. With your suggestion below we would
only need to adjust vpci_read_hw() (and then data here would not
strictly need adjusting).

> And
> you can likely use GENMASK(size * 8, 0) as it's easier to read.

OK. I'll then also provide a definition for GENMASK in
tools/tests/vpci/emul.h.

>>  
>>      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.

Seeing as there is a is_hardware_domain check inside vpci_read_hw(),
that is okay. I'll make the adjustment here and in vpci_write.