Re: [PATCH v6 2/3] xen/arm: Enable the existing x86 virtual PCI support for ARM.

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Roger,

> On 15 Oct 2021, at 09:32, Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> 
> On Thu, Oct 14, 2021 at 03:49:50PM +0100, Bertrand Marquis wrote:
>> From: Rahul Singh <rahul.singh@xxxxxxx>
>> 
>> 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.
>> 
>> For Dom0less systems scan_pci_devices() would be used to discover the
>> PCI device in XEN and VPCI handler will be added during XEN boots.
>> 
>> This patch is also doing some small fixes to fix compilation errors on
>> arm32 of vpci:
>> - add a cast to unsigned long in print in header.c
>> - add a cast to uint64_t in vpci_ecam_mmio_write
>> 
>> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>> ---
>> Changes in v6:
>> - Use new vpci_ecam_ helpers for PCI access
>> - Do not set XEN_DOMCTL_CDF_vpci for dom0 for now (will be done in a
>> future patch once everything is ready)
> 
> Isn't the series missing a revert of XEN_DOMCTL_CDF_vpci? I think
> that's what we agreed would be the way forward.

A separate reverse patch for that has already been merged:
https://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=9516d01ac3015f522528ed6dafb3f584eaa7ed2c

> 
>> - rename REGISTER_OFFSET to ECAM_REG_OFFSET and move it to pci.h
>> - remove not needed local variables in vpci_mmio_write, the one in read
>> has been kept to ensure proper compilation on arm32
>> - move call to vpci_add_handlers before iommu init to simplify exit path
>> - move call to pci_cleanup_msi in the out section of pci_add_device if
>> pdev is not NULL and on error
>> - initialize pdev to NULL to handle properly exit path call of
>> pci_cleanup_msi
>> - keep has_vpci to return false for now as CFG_vpci has been removed.
>> Added a comment on top of the definition.
>> - fix compilation errors on arm32 (print in header.c, cast missing in
>> mmio_write.
>> - local variable was kept in vpci_mmio_read on arm to prevent a cast
>> error in arm32.
>> Change in v5:
>> - Add pci_cleanup_msi(pdev) incleanup part.
>> - Added Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> Change in v4:
>> - Move addition of XEN_DOMCTL_CDF_vpci flag to separate patch
>> Change in v3:
>> - Use is_pci_passthrough_enabled() in place of pci_passthrough_enabled 
>> variable
>> - Reject XEN_DOMCTL_CDF_vpci for x86 in arch_sanitise_domain_config()
>> - Remove IS_ENABLED(CONFIG_HAS_VPCI) from has_vpci()
>> Change in v2:
>> - Add new XEN_DOMCTL_CDF_vpci flag
>> - modify has_vpci() to include XEN_DOMCTL_CDF_vpci
>> - enable vpci support when pci-passthough option is enabled.
>> ---
>> ---
>> xen/arch/arm/Makefile         |  1 +
>> xen/arch/arm/domain.c         |  4 ++
>> xen/arch/arm/vpci.c           | 74 +++++++++++++++++++++++++++++++++++
>> xen/arch/arm/vpci.h           | 36 +++++++++++++++++
>> xen/drivers/passthrough/pci.c | 18 ++++++++-
>> xen/drivers/vpci/header.c     |  3 +-
>> xen/drivers/vpci/vpci.c       |  2 +-
>> xen/include/asm-arm/domain.h  |  1 +
>> xen/include/asm-x86/pci.h     |  2 -
>> xen/include/public/arch-arm.h |  7 ++++
>> xen/include/xen/pci.h         |  3 ++
>> 11 files changed, 146 insertions(+), 5 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 64518848b2..07f634508e 100644
>> --- a/xen/arch/arm/Makefile
>> +++ b/xen/arch/arm/Makefile
>> @@ -7,6 +7,7 @@ ifneq ($(CONFIG_NO_PLAT),y)
>> obj-y += platforms/
>> endif
>> obj-$(CONFIG_TEE) += tee/
>> +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 eef0661beb..96e1b23550 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -39,6 +39,7 @@
>> #include <asm/vgic.h>
>> #include <asm/vtimer.h>
>> 
>> +#include "vpci.h"
>> #include "vuart.h"
>> 
>> DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
>> @@ -773,6 +774,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..7c3552b65d
>> --- /dev/null
>> +++ b/xen/arch/arm/vpci.c
>> @@ -0,0 +1,74 @@
>> +/*
>> + * xen/arch/arm/vpci.c
>> + *
>> + * 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>
>> +#include <xen/vpci.h>
>> +
>> +#include <asm/mmio.h>
>> +
>> +static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
>> +                          register_t *r, void *p)
>> +{
>> +    pci_sbdf_t sbdf;
>> +    /* data is needed to prevent a pointer cast on 32bit */
>> +    unsigned long data = ~0ul;
>> +    int ret;
>> +
>> +    /* We ignore segment part and always handle segment 0 */
>> +    sbdf.sbdf = ECAM_BDF(info->gpa);
>> +
>> +    ret = vpci_ecam_mmio_read(sbdf, ECAM_REG_OFFSET(info->gpa),
>> +                              1U << info->dabt.size, &data);
>> +
>> +    *r = data;
>> +
>> +    return ret;
>> +}
>> +
>> +static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info,
>> +                           register_t r, void *p)
>> +{
>> +    pci_sbdf_t sbdf;
>> +
>> +    /* We ignore segment part and always handle segment 0 */
>> +    sbdf.sbdf = ECAM_BDF(info->gpa);
>> +
>> +    return vpci_ecam_mmio_write(sbdf, ECAM_REG_OFFSET(info->gpa),
>> +                                1U << info->dabt.size, r);
>> +}
> 
> I'm not sure returning an error value here is helpful, as I'm not sure
> how native Arm behaves and how this error is propagated into the
> guest. FWIWreturning ~0 or dropping writes is what we do in x86 when
> vPCI is not capable of handling the access.

Mmio handlers can take a return code on arm if something did not work
so I think this is the right way to do it on arm.
Now has agreed with Jan, we will change the return type of 
vpci_ecam_write (also renamed) to be a boolean.

> 
>> +
>> +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..d8a7b0e3e8
>> --- /dev/null
>> +++ b/xen/arch/arm/vpci.h
>> @@ -0,0 +1,36 @@
>> +/*
>> + * xen/arch/arm/vpci.h
>> + *
>> + * 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 3aa8c3175f..8cc529ecec 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -658,7 +658,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>                    const struct pci_dev_info *info, nodeid_t node)
>> {
>>     struct pci_seg *pseg;
>> -    struct pci_dev *pdev;
>> +    struct pci_dev *pdev = NULL;
>>     unsigned int slot = PCI_SLOT(devfn), func = PCI_FUNC(devfn);
>>     const char *pdev_type;
>>     int ret;
>> @@ -752,6 +752,19 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>> 
>>     check_pdev(pdev);
>> 
>> +#ifdef CONFIG_ARM
>> +    /*
>> +     * On ARM PCI devices discovery will be done by Dom0. Add vpci handler 
>> when
>> +     * Dom0 inform XEN to add the PCI devices in XEN.
>> +     */
>> +    ret = vpci_add_handlers(pdev);
>> +    if ( ret )
>> +    {
>> +        printk(XENLOG_ERR "Setup of vPCI failed: %d\n", ret);
>> +        goto out;
>> +    }
>> +#endif
> 
> I think vpci_add_handlers should be called after checking that
> pdev->domain is != NULL, so I would move this chunk a bit below.

On arm this would prevent the dom0less use case or to have the PCI
bus enumerated from an other domain.
@oleksandr: can you comment on this one, you might have a better
answer than me on this ?

> 
>> +
>>     ret = 0;
>>     if ( !pdev->domain )
>>     {
>> @@ -784,6 +797,9 @@ out:
>>                    &PCI_SBDF(seg, bus, slot, func));
>>         }
>>     }
>> +    else if ( pdev )
>> +        pci_cleanup_msi(pdev);
> 
> I'm slightly lost at why you add this chunk, is this strictly related
> to the patch?

This was discussed a lot in previous version of the patch and
requested by Stefano. The idea here is that as soon as handlers
are added some bits might be modified in the PCI config space
leading possibly to msi interrupts. So it is safer to cleanup on the
error path. For references please see discussion on v4 and v5 where
this was actually added (to much references as the discussion was
long so here [1] and [2] are the patchwork thread).

[1] 
https://patchwork.kernel.org/project/xen-devel/patch/9bdca2cda5d2e83f94dc2423e55714273539760a.1633540842.git.rahul.singh@xxxxxxx/
[2] 
https://patchwork.kernel.org/project/xen-devel/patch/f093de681c2560a7196895bcd666ef8840885c1d.1633340795.git.rahul.singh@xxxxxxx/

Regards
Bertrand