Re: [PATCH v1 11/14] xen/arm: Enable the existing x86 virtual PCI support for ARM.

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Julien,

> On 9 Sep 2021, at 2:50 pm, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Rahul,
> 
> On 19/08/2021 13:02, Rahul Singh wrote:
>> The existing VPCI support available for X86 is adapted for Arm.
>> When the device is added to XEN via the hyper call
>> “PHYSDEVOP_pci_device_add”, VPCI handler for the config space
>> access is added to the Xen to emulate the PCI devices config space.
>> A MMIO trap handler for the PCI ECAM space is registered in XEN
>> so that when guest is trying to access the PCI config space,XEN
>> will trap the access and emulate read/write using the VPCI and
>> not the real PCI hardware.
>> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
>> ---
>>  xen/arch/arm/Makefile         |  1 +
>>  xen/arch/arm/domain.c         |  4 ++
>>  xen/arch/arm/vpci.c           | 96 +++++++++++++++++++++++++++++++++++
>>  xen/arch/arm/vpci.h           | 37 ++++++++++++++
>>  xen/drivers/passthrough/pci.c |  7 +++
>>  xen/drivers/vpci/Makefile     |  3 +-
>>  xen/drivers/vpci/header.c     |  2 +
>>  xen/include/asm-arm/domain.h  |  5 +-
>>  xen/include/asm-arm/pci.h     |  8 +++
>>  xen/include/public/arch-arm.h |  4 ++
>>  10 files changed, 165 insertions(+), 2 deletions(-)
>>  create mode 100644 xen/arch/arm/vpci.c
>>  create mode 100644 xen/arch/arm/vpci.h
>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>> index 0e14a5e5c8..7cdce684a4 100644
>> --- a/xen/arch/arm/Makefile
>> +++ b/xen/arch/arm/Makefile
>> @@ -7,6 +7,7 @@ obj-y += platforms/
>>  endif
>>  obj-$(CONFIG_TEE) += tee/
>>  obj-$(CONFIG_HAS_PCI) += pci/
>> +obj-$(CONFIG_HAS_VPCI) += vpci.o
>>    obj-$(CONFIG_HAS_ALTERNATIVE) += alternative.o
>>  obj-y += bootfdt.init.o
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index 19c756ac3d..d99c653626 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -40,6 +40,7 @@
>>  #include <asm/vtimer.h>
>>    #include "vuart.h"
>> +#include "vpci.h"
> 
> Please order the includes alphabetically. So this one should go before 
> "vuart.h".
> 
>>    DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
>>  @@ -767,6 +768,9 @@ int arch_domain_create(struct domain *d,
>>      if ( is_hardware_domain(d) && (rc = domain_vuart_init(d)) )
>>          goto fail;
>>  +    if ( (rc = domain_vpci_init(d)) != 0 )
>> +        goto fail;
>> +
>>      return 0;
>>    fail:
>> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
>> new file mode 100644
>> index 0000000000..da8b1ca13c
>> --- /dev/null
>> +++ b/xen/arch/arm/vpci.c
>> @@ -0,0 +1,96 @@
>> +/*
>> + * xen/arch/arm/vpci.c
>> + * Copyright (c) 2021 Arm Ltd.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +#include <xen/sched.h>
> 
> NIT: Please add a newline between generic and arch specific includes.
Ack.
> 
>> +#include <asm/mmio.h>
>> +
>> +/* Do some sanity checks. */
>> +static bool vpci_mmio_access_allowed(unsigned int reg, unsigned int len)
>> +{
>> +    /* Check access size. */
>> +    if ( len != 1 && len != 2 && len != 4 && len != 8 )
>> +        return false;
> You will allow all the possible value of len (this is coming from the HW). So 
> I feels this is a bit too much to check for every I/O to the vPCI.
> 
> If you really want to keep the check then you can simply check that len is < 
> 8 because the two callers compute it with (1 << S). So there is no way to set 
> it 3, 5, 6 and 7.

I will remove the check for 1,2,4,8 and will have check ( len < 8).
> 
>> +
>> +    /* Check that access is size aligned. */
>> +    if ( (reg & (len - 1)) )
>> +        return false;
>> +
>> +    return true;
>> +}
>> +
>> +static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
>> +                          register_t *r, void *p)
>> +{
>> +    unsigned int reg;
>> +    pci_sbdf_t sbdf;
>> +    uint32_t data = 0;
>> +    unsigned int size = 1U << info->dabt.size;
>> +
>> +    sbdf.sbdf = (((info->gpa) & 0x0ffff000) >> 12);
>> +    reg = (((info->gpa) & 0x00000ffc) | (info->gpa & 3));
> 
> This logic is the same as below in vpci_mmio_write(). So I think you want to 
> provide an helper because this is not trivial to read.
Ok.
> 
> Also, for the first line, I think you can re-use MMCFG_BDF() from the x86 
> code. For the second line, I would define the value so it is clearer to 
> understand that they mean (although & 3 is fine to me) .
Ok.
> 
>> +
>> +    if ( !vpci_mmio_access_allowed(reg, size) )
>> +        return 1;
> So, you will a guest will read 0 if the access is unaligned. This seems an 
> odd behavior given this is not an allowed access. AFAIU, the HW would likely 
> trow a data abort because you can't do unalign access on uncachable memory. 
> So I think we should return 0 here to let the MMIO handler inject a data 
> abort.

Ack. 
> 
>> +
>> +    data = vpci_read(sbdf, reg, size);
> 
> So in vpci_mmio_access_allowed(), you will allow a guest to read a 64-bit 
> value. But... vpci_read() will return a 32-bit value.
> 
> Looking at the x86 code, they have a second call to vpci_read() to handle the 
> top 32-bit. Any reason why this was not implemented on Arm?

I missed that I will fix this in next version.

> 
> If we need to implement it then I think this should be implement in 
> vpci_read() to avoid duplication between x86 and arm.
I think it will be good to make vpci_read() & vpci_write() simple not to add 
extra logic based on 64 bit value. 
I will add second call for vpci_read() and vpci_write() for ARM. 
> 
>> +
>> +    memcpy(r, &data, size);
> 
> From my understanding, any unused bit should be 0. So this should be:
> 
> *r = data;
Ack.
> 
>> +
>> +    return 1;
>> +}
>> +
>> +static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info,
>> +                           register_t r, void *p)
> 
> My remarks on vpci_mmio_read() applies here too.
> 
>> +{
>> +    unsigned int reg;
>> +    pci_sbdf_t sbdf;
>> +    uint32_t data = r;
>> +    unsigned int size = 1U << info->dabt.size;
>> +
>> +    sbdf.sbdf = (((info->gpa) & 0x0ffff000) >> 12);
>> +    reg = (((info->gpa) & 0x00000ffc) | (info->gpa & 3));
>> +
>> +    if ( !vpci_mmio_access_allowed(reg, size) )
>> +        return 1;
>> +
>> +    vpci_write(sbdf, reg, size, data);
>> +
>> +    return 1;
>> +}
>> +
>> +static const struct mmio_handler_ops vpci_mmio_handler = {
>> +    .read  = vpci_mmio_read,
>> +    .write = vpci_mmio_write,
>> +};
>> +
>> +int domain_vpci_init(struct domain *d)
>> +{
>> +    if ( !has_vpci(d) )
>> +        return 0;
>> +
>> +    register_mmio_handler(d, &vpci_mmio_handler,
>> +                          GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL);
>> +
>> +    return 0;
>> +}
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> +
>> diff --git a/xen/arch/arm/vpci.h b/xen/arch/arm/vpci.h
>> new file mode 100644
>> index 0000000000..8a093bb705
>> --- /dev/null
>> +++ b/xen/arch/arm/vpci.h
>> @@ -0,0 +1,37 @@
>> +/*
>> + * xen/arch/arm/vpci.h
>> + * Copyright (c) 2021 Arm Ltd.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#ifndef __ARCH_ARM_VPCI_H__
>> +#define __ARCH_ARM_VPCI_H__
>> +
>> +#ifdef CONFIG_HAS_VPCI
>> +int domain_vpci_init(struct domain *d);
>> +#else
>> +static inline int domain_vpci_init(struct domain *d)
>> +{
>> +    return 0;
>> +}
>> +#endif
>> +
>> +#endif /* __ARCH_ARM_VPCI_H__ */
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>> index c23c8cb06b..56e261e9bd 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -767,6 +767,13 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>      else
>>          iommu_enable_device(pdev);
>>  +#ifdef CONFIG_ARM
>> +    ret = vpci_add_handlers(pdev);
>> +    if ( ret ) {
>> +        printk(XENLOG_ERR "setup of vPCI for failed: %d\n",ret);
>> +        goto out;
>> +    }
>> +#endif
>>      pci_enable_acs(pdev);
>>    out:
>> diff --git a/xen/drivers/vpci/Makefile b/xen/drivers/vpci/Makefile
>> index 55d1bdfda0..1a1413b93e 100644
>> --- a/xen/drivers/vpci/Makefile
>> +++ b/xen/drivers/vpci/Makefile
>> @@ -1 +1,2 @@
>> -obj-y += vpci.o header.o msi.o msix.o
>> +obj-y += vpci.o header.o
>> +obj-$(CONFIG_HAS_PCI_MSI) += msi.o msix.o
>> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
>> index ba9a036202..f8cd55e7c0 100644
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -96,8 +96,10 @@ static void modify_decoding(const struct pci_dev *pdev, 
>> uint16_t cmd,
>>       * FIXME: punching holes after the p2m has been set up might be racy for
>>       * DomU usage, needs to be revisited.
>>       */
>> +#ifdef CONFIG_HAS_PCI_MSI
>>      if ( map && !rom_only && vpci_make_msix_hole(pdev) )
>>          return;
>> +#endif
>>        for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
>>      {
>> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
>> index c9277b5c6d..d742b94bd6 100644
>> --- a/xen/include/asm-arm/domain.h
>> +++ b/xen/include/asm-arm/domain.h
>> @@ -262,7 +262,10 @@ static inline void arch_vcpu_block(struct vcpu *v) {}
>>    #define arch_vm_assist_valid_mask(d) (1UL << 
>> VMASST_TYPE_runstate_update_flag)
>>  -#define has_vpci(d)    ({ (void)(d); false; })
>> +/* For X86 VPCI is enabled and tested for PVH DOM0 only but
>> + * for ARM we enable support VPCI for guest domain also.
>> + */
>> +#define has_vpci(d) ({ (void)(d); IS_ENABLED(CONFIG_HAS_VPCI); })
>>    #endif /* __ASM_DOMAIN_H__ */
>>  diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
>> index 756f8637ab..c58152de80 100644
>> --- a/xen/include/asm-arm/pci.h
>> +++ b/xen/include/asm-arm/pci.h
>> @@ -27,6 +27,14 @@ struct arch_pci_dev {
>>      struct device dev;
>>  };
>>  +/* Arch-specific MSI data for vPCI. */
>> +struct vpci_arch_msi {
>> +};
>> +
>> +/* Arch-specific MSI-X entry data for vPCI. */
>> +struct vpci_arch_msix_entry {
>> +};
>> +
>>  /*
>>   * struct to hold the mappings of a config space window. This
>>   * is expected to be used as sysdata for PCI controllers that
>> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
>> index 64a2ca30da..0a9749e768 100644
>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -422,6 +422,10 @@ typedef uint64_t xen_callback_t;
>>  #define GUEST_PL011_BASE    xen_mk_ullong(0x22000000)
>>  #define GUEST_PL011_SIZE    xen_mk_ullong(0x00001000)
>>  +/* VPCI ECAM mappings */
>> +#define GUEST_VPCI_ECAM_BASE    xen_mk_ullong(0x10000000)
> 
> All the values for the memory layout has been defined in ascending order. So 
> please add the vCPI at the correct place. If I am not mistaken, this should 
> be before the GUEST_ACPI_*.

Ack. 
> 
>> +#define GUEST_VPCI_ECAM_SIZE    xen_mk_ullong(0x10000000)
> 
> Please document how to decide the size. This is important for the future if 
> we need to change the size.

Ok let me document in next version.

/*                                                                              
 * 256 MB is reserved for VPCI configuration space based on calculation         
 * 256 buses × 32 devices × 8 functions × 4 KB = 256 MB                         
 */                                                                             
#define GUEST_VPCI_ECAM_BASE    xen_mk_ullong(0x10000000)                       
#define GUEST_VPCI_ECAM_SIZE    xen_mk_ullong(0x10000000)


Regrads,
Rahul
> 
> Cheers,
> 
> -- 
> Julien Grall