| Message ID | 396b08e2598cf0338e0c7f4ad3cd5cb66db89224.1634221830.git.bertrand.marquis@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI devices passthrough on Arm | expand |
On Thu, 14 Oct 2021, Bertrand Marquis wrote: > From: Rahul Singh <rahul.singh@arm.com> > > 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 Thank you for these! Strictly speaking they are not required now but they are welcome. I would also be OK if they were removed from this patch but it is fine to have them in here. There is an issues with this patch, see below at the bottom > Signed-off-by: Rahul Singh <rahul.singh@arm.com> > Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> > > --- > 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) > - 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@kernel.org> > 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); > +} > + > +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 > + > ret = 0; > if ( !pdev->domain ) > { > @@ -784,6 +797,9 @@ out: > &PCI_SBDF(seg, bus, slot, func)); > } > } > + else if ( pdev ) > + pci_cleanup_msi(pdev); > + > return ret; > } > > diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c > index f8cd55e7c0..c5b025b88b 100644 > --- a/xen/drivers/vpci/header.c > +++ b/xen/drivers/vpci/header.c > @@ -374,7 +374,8 @@ static void bar_write(const struct pci_dev *pdev, unsigned int reg, > if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) ) > gprintk(XENLOG_WARNING, > "%pp: ignored BAR %lu write with memory decoding enabled\n", > - &pdev->sbdf, bar - pdev->vpci->header.bars + hi); > + &pdev->sbdf, > + (unsigned long)(bar - pdev->vpci->header.bars + hi)); > return; > } > > diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c > index c0853176d7..2bd67fc27a 100644 > --- a/xen/drivers/vpci/vpci.c > +++ b/xen/drivers/vpci/vpci.c > @@ -507,7 +507,7 @@ int vpci_ecam_mmio_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int len, > > vpci_write(sbdf, reg, min(4u, len), data); > if ( len == 8 ) > - vpci_write(sbdf, reg + 4, 4, data >> 32); > + vpci_write(sbdf, reg + 4, 4, (uint64_t)data >> 32); > > return 1; > } > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h > index 14e575288f..9b3647587a 100644 > --- a/xen/include/asm-arm/domain.h > +++ b/xen/include/asm-arm/domain.h > @@ -263,6 +263,7 @@ static inline void arch_vcpu_block(struct vcpu *v) {} > > #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag) > > +/* vPCI is not available on Arm */ > #define has_vpci(d) ({ (void)(d); false; }) > > #endif /* __ASM_DOMAIN_H__ */ > diff --git a/xen/include/asm-x86/pci.h b/xen/include/asm-x86/pci.h > index a0df5c1279..443f25347d 100644 > --- a/xen/include/asm-x86/pci.h > +++ b/xen/include/asm-x86/pci.h > @@ -6,8 +6,6 @@ > #define CF8_ADDR_HI(cf8) ( ((cf8) & 0x0f000000) >> 16) > #define CF8_ENABLED(cf8) (!!((cf8) & 0x80000000)) > > -#define ECAM_BDF(addr) ( ((addr) & 0x0ffff000) >> 12) > - > #define IS_SNB_GFX(id) (id == 0x01068086 || id == 0x01168086 \ > || id == 0x01268086 || id == 0x01028086 \ > || id == 0x01128086 || id == 0x01228086 \ > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h > index d46c61fca9..44be337dec 100644 > --- a/xen/include/public/arch-arm.h > +++ b/xen/include/public/arch-arm.h > @@ -418,6 +418,13 @@ typedef uint64_t xen_callback_t; > #define GUEST_GICV3_GICR0_BASE xen_mk_ullong(0x03020000) /* vCPU0..127 */ > #define GUEST_GICV3_GICR0_SIZE xen_mk_ullong(0x01000000) > > +/* > + * 256 MB is reserved for VPCI configuration space based on calculation > + * 256 buses × 32 devices × 8 functions × 4 KB = 256 MB > + */ Somehow 3 non-ascii characters sneaked into this patch. The 'x' are not 'x' but are 0xc3 and cause the following errors in a few gitlab-ci builds: python3 mkheader.py arm32 arm32.h.tmp /builds/xen-project/people/sstabellini/xen/tools/include/xen-foreign/../../../xen/include/public/arch-arm.h /builds/xen-project/people/sstabellini/xen/tools/include/xen-foreign/../../../xen/include/public/xen.h Traceback (most recent call last): File "mkheader.py", line 120, in <module> input += f.read(); File "/usr/lib/python3.5/encodings/ascii.py", line 26, in decode return codecs.ascii_decode(input, self.errors)[0] UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position 14641: ordinal not in range(128) Makefile:28: recipe for target 'arm32.h' failed make[2]: *** [arm32.h] Error 1 Full logs here: https://gitlab.com/xen-project/people/sstabellini/xen/-/pipelines/38855078 https://gitlab.com/xen-project/people/sstabellini/xen/-/jobs/16810108756 Replacing the 3 characters with 'x' solves the problem. https://gitlab.com/xen-project/people/sstabellini/xen/-/pipelines/388721262 With the three 'x' changed to ascii: Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > +#define GUEST_VPCI_ECAM_BASE xen_mk_ullong(0x10000000) > +#define GUEST_VPCI_ECAM_SIZE xen_mk_ullong(0x10000000) > + > /* ACPI tables physical address */ > #define GUEST_ACPI_BASE xen_mk_ullong(0x20000000) > #define GUEST_ACPI_SIZE xen_mk_ullong(0x02000000) > diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h > index 70ac25345c..db18cb7639 100644 > --- a/xen/include/xen/pci.h > +++ b/xen/include/xen/pci.h > @@ -40,6 +40,9 @@ > #define PCI_SBDF3(s,b,df) \ > ((pci_sbdf_t){ .sbdf = (((s) & 0xffff) << 16) | PCI_BDF2(b, df) }) > > +#define ECAM_BDF(addr) ( ((addr) & 0x0ffff000) >> 12) > +#define ECAM_REG_OFFSET(addr) ((addr) & 0x00000fff) > + > typedef union { > uint32_t sbdf; > struct { > -- > 2.25.1 >
On 15.10.2021 01:49, Stefano Stabellini wrote: > On Thu, 14 Oct 2021, Bertrand Marquis wrote: >> --- a/xen/include/public/arch-arm.h >> +++ b/xen/include/public/arch-arm.h >> @@ -418,6 +418,13 @@ typedef uint64_t xen_callback_t; >> #define GUEST_GICV3_GICR0_BASE xen_mk_ullong(0x03020000) /* vCPU0..127 */ >> #define GUEST_GICV3_GICR0_SIZE xen_mk_ullong(0x01000000) >> >> +/* >> + * 256 MB is reserved for VPCI configuration space based on calculation >> + * 256 buses × 32 devices × 8 functions × 4 KB = 256 MB >> + */ > > Somehow 3 non-ascii characters sneaked into this patch. The 'x' are not > 'x' but are 0xc3 and cause the following errors in a few gitlab-ci > builds: > > python3 mkheader.py arm32 arm32.h.tmp /builds/xen-project/people/sstabellini/xen/tools/include/xen-foreign/../../../xen/include/public/arch-arm.h /builds/xen-project/people/sstabellini/xen/tools/include/xen-foreign/../../../xen/include/public/xen.h > Traceback (most recent call last): > File "mkheader.py", line 120, in <module> > input += f.read(); > File "/usr/lib/python3.5/encodings/ascii.py", line 26, in decode > return codecs.ascii_decode(input, self.errors)[0] > UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position 14641: ordinal not in range(128) > Makefile:28: recipe for target 'arm32.h' failed > make[2]: *** [arm32.h] Error 1 > > Full logs here: > https://gitlab.com/xen-project/people/sstabellini/xen/-/pipelines/38855078 > https://gitlab.com/xen-project/people/sstabellini/xen/-/jobs/16810108756 > > > Replacing the 3 characters with 'x' solves the problem. > https://gitlab.com/xen-project/people/sstabellini/xen/-/pipelines/388721262 Interesting. I thought we permit UTF-8 in the sources; see e.g. tools/tests/x86_emulator/simd-sha.c using Σ. Is mkheader.py in need of adjustment? Of course I can see that right now the easiest is to use ASCII x, but I think it was deliberate to use × here. Then again, with the goal of the public headers being usable with pretty old compilers as well (C89 being the assumed baseline), excluding them from the permission to use UTF-8 may also be quite reasonable. Jan
On 14.10.2021 16:49, Bertrand Marquis wrote: > @@ -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 > + > ret = 0; > if ( !pdev->domain ) Elsewhere I did point out that you need to be careful wrt pdev->domain being NULL. As the code in context clearly documents, you're now adding handlers before that field was set. In comments to a prior version I did already suggest to consider placing the new code inside the if() in question (albeit at the time this was mainly to make clear that error cleanup may not fiddle with things not put in place by the iommu_enable_device() alternative path). This would have the further benefit of making is crystal clear that here only handlers for Dom0 would get put in place (and hence their installing for DomU-s would need to be looked for elsewhere). > @@ -784,6 +797,9 @@ out: > &PCI_SBDF(seg, bus, slot, func)); > } > } > + else if ( pdev ) > + pci_cleanup_msi(pdev); Have you thoroughly checked that this call is benign on x86? There's no wording to that effect in the description afaics, and I can't easily convince myself that it would be correct when the iommu_enable_device() path was taken. (I'm also not going to exclude that it should have been there even prior to your work, albeit then adding this would want to be a separate bugfix patch.) > --- a/xen/drivers/vpci/header.c > +++ b/xen/drivers/vpci/header.c > @@ -374,7 +374,8 @@ static void bar_write(const struct pci_dev *pdev, unsigned int reg, > if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) ) > gprintk(XENLOG_WARNING, > "%pp: ignored BAR %lu write with memory decoding enabled\n", > - &pdev->sbdf, bar - pdev->vpci->header.bars + hi); > + &pdev->sbdf, > + (unsigned long)(bar - pdev->vpci->header.bars + hi)); This looks like an entirely unrelated change which I'm afraid I don't even understand why it needs making. The description says this is for Arm32, but it remains unclear what the compilation error would have been. My best guess is that perhaps you really mean to change the format specifier to %zu (really this should be %td, but our vsprintf() doesn't support 't' for whatever reason). Please recall that we try to avoid casts where possible. > --- a/xen/drivers/vpci/vpci.c > +++ b/xen/drivers/vpci/vpci.c > @@ -507,7 +507,7 @@ int vpci_ecam_mmio_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int len, > > vpci_write(sbdf, reg, min(4u, len), data); > if ( len == 8 ) > - vpci_write(sbdf, reg + 4, 4, data >> 32); > + vpci_write(sbdf, reg + 4, 4, (uint64_t)data >> 32); I assume the need for this change will go away with the use of CONFIG_64BIT in the earlier patch. > --- a/xen/include/xen/pci.h > +++ b/xen/include/xen/pci.h > @@ -40,6 +40,9 @@ > #define PCI_SBDF3(s,b,df) \ > ((pci_sbdf_t){ .sbdf = (((s) & 0xffff) << 16) | PCI_BDF2(b, df) }) > > +#define ECAM_BDF(addr) ( ((addr) & 0x0ffff000) >> 12) > +#define ECAM_REG_OFFSET(addr) ((addr) & 0x00000fff) The latter is fine to be put here (i.e. FTAOD I'm fine with it staying here). For the former I even question its original placement in asm-x86/pci.h: It's not generally correct as per the PCI spec, as the bus portion of the address can be anywhere from 1 to 8 bits. And in fact there is a reason why this macro was/is used in only a single place, but not e.g. in x86'es handling of physical MCFG. It is merely an implementation choice in vPCI that the entire segment 0 has a linear address range covering all 256 buses. Hence I think this wants to move to xen/vpci.h and then perhaps also be named VPCI_ECAM_BDF(). Also, as already pointed out on a much earlier version, while the blank following the opening parenthesis was warranted in asm-x86/pci.h for alignment reasons, it is no longer warranted here. It was certainly gone in v4 and v5. And one final nit: I don't think we commonly use full stops in patch titles. (Personally I therefore also think titles shouldn't start with a capital letter, but I know others think differently.) Jan
On Thu, Oct 14, 2021 at 03:49:50PM +0100, Bertrand Marquis wrote: > From: Rahul Singh <rahul.singh@arm.com> > > 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@arm.com> > Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> > --- > 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. > - 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@kernel.org> > 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. > + > +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. > + > 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? > return ret; > } > > diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c > index f8cd55e7c0..c5b025b88b 100644 > --- a/xen/drivers/vpci/header.c > +++ b/xen/drivers/vpci/header.c > @@ -374,7 +374,8 @@ static void bar_write(const struct pci_dev *pdev, unsigned int reg, > if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) ) > gprintk(XENLOG_WARNING, > "%pp: ignored BAR %lu write with memory decoding enabled\n", > - &pdev->sbdf, bar - pdev->vpci->header.bars + hi); > + &pdev->sbdf, > + (unsigned long)(bar - pdev->vpci->header.bars + hi)); > return; > } > > diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c > index c0853176d7..2bd67fc27a 100644 > --- a/xen/drivers/vpci/vpci.c > +++ b/xen/drivers/vpci/vpci.c > @@ -507,7 +507,7 @@ int vpci_ecam_mmio_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int len, > > vpci_write(sbdf, reg, min(4u, len), data); > if ( len == 8 ) > - vpci_write(sbdf, reg + 4, 4, data >> 32); > + vpci_write(sbdf, reg + 4, 4, (uint64_t)data >> 32); > > return 1; > } > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h > index 14e575288f..9b3647587a 100644 > --- a/xen/include/asm-arm/domain.h > +++ b/xen/include/asm-arm/domain.h > @@ -263,6 +263,7 @@ static inline void arch_vcpu_block(struct vcpu *v) {} > > #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag) > > +/* vPCI is not available on Arm */ > #define has_vpci(d) ({ (void)(d); false; }) > > #endif /* __ASM_DOMAIN_H__ */ > diff --git a/xen/include/asm-x86/pci.h b/xen/include/asm-x86/pci.h > index a0df5c1279..443f25347d 100644 > --- a/xen/include/asm-x86/pci.h > +++ b/xen/include/asm-x86/pci.h > @@ -6,8 +6,6 @@ > #define CF8_ADDR_HI(cf8) ( ((cf8) & 0x0f000000) >> 16) > #define CF8_ENABLED(cf8) (!!((cf8) & 0x80000000)) > > -#define ECAM_BDF(addr) ( ((addr) & 0x0ffff000) >> 12) > - > #define IS_SNB_GFX(id) (id == 0x01068086 || id == 0x01168086 \ > || id == 0x01268086 || id == 0x01028086 \ > || id == 0x01128086 || id == 0x01228086 \ > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h > index d46c61fca9..44be337dec 100644 > --- a/xen/include/public/arch-arm.h > +++ b/xen/include/public/arch-arm.h > @@ -418,6 +418,13 @@ typedef uint64_t xen_callback_t; > #define GUEST_GICV3_GICR0_BASE xen_mk_ullong(0x03020000) /* vCPU0..127 */ > #define GUEST_GICV3_GICR0_SIZE xen_mk_ullong(0x01000000) > > +/* > + * 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) > + > /* ACPI tables physical address */ > #define GUEST_ACPI_BASE xen_mk_ullong(0x20000000) > #define GUEST_ACPI_SIZE xen_mk_ullong(0x02000000) > diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h > index 70ac25345c..db18cb7639 100644 > --- a/xen/include/xen/pci.h > +++ b/xen/include/xen/pci.h > @@ -40,6 +40,9 @@ > #define PCI_SBDF3(s,b,df) \ > ((pci_sbdf_t){ .sbdf = (((s) & 0xffff) << 16) | PCI_BDF2(b, df) }) > > +#define ECAM_BDF(addr) ( ((addr) & 0x0ffff000) >> 12) ^ extra space? Thanks, Roger.
On 15.10.2021 10:32, Roger Pau Monné wrote: > On Thu, Oct 14, 2021 at 03:49:50PM +0100, Bertrand Marquis wrote: >> From: Rahul Singh <rahul.singh@arm.com> >> >> 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@arm.com> >> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> >> --- >> 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. > The revert patch 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@kernel.org> >> 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. > >> + >> +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. > >> + >> 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? > >> return ret; >> } >> >> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c >> index f8cd55e7c0..c5b025b88b 100644 >> --- a/xen/drivers/vpci/header.c >> +++ b/xen/drivers/vpci/header.c >> @@ -374,7 +374,8 @@ static void bar_write(const struct pci_dev *pdev, unsigned int reg, >> if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) ) >> gprintk(XENLOG_WARNING, >> "%pp: ignored BAR %lu write with memory decoding enabled\n", >> - &pdev->sbdf, bar - pdev->vpci->header.bars + hi); >> + &pdev->sbdf, >> + (unsigned long)(bar - pdev->vpci->header.bars + hi)); >> return; >> } >> >> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c >> index c0853176d7..2bd67fc27a 100644 >> --- a/xen/drivers/vpci/vpci.c >> +++ b/xen/drivers/vpci/vpci.c >> @@ -507,7 +507,7 @@ int vpci_ecam_mmio_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int len, >> >> vpci_write(sbdf, reg, min(4u, len), data); >> if ( len == 8 ) >> - vpci_write(sbdf, reg + 4, 4, data >> 32); >> + vpci_write(sbdf, reg + 4, 4, (uint64_t)data >> 32); >> >> return 1; >> } >> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h >> index 14e575288f..9b3647587a 100644 >> --- a/xen/include/asm-arm/domain.h >> +++ b/xen/include/asm-arm/domain.h >> @@ -263,6 +263,7 @@ static inline void arch_vcpu_block(struct vcpu *v) {} >> >> #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag) >> >> +/* vPCI is not available on Arm */ >> #define has_vpci(d) ({ (void)(d); false; }) >> >> #endif /* __ASM_DOMAIN_H__ */ >> diff --git a/xen/include/asm-x86/pci.h b/xen/include/asm-x86/pci.h >> index a0df5c1279..443f25347d 100644 >> --- a/xen/include/asm-x86/pci.h >> +++ b/xen/include/asm-x86/pci.h >> @@ -6,8 +6,6 @@ >> #define CF8_ADDR_HI(cf8) ( ((cf8) & 0x0f000000) >> 16) >> #define CF8_ENABLED(cf8) (!!((cf8) & 0x80000000)) >> >> -#define ECAM_BDF(addr) ( ((addr) & 0x0ffff000) >> 12) >> - >> #define IS_SNB_GFX(id) (id == 0x01068086 || id == 0x01168086 \ >> || id == 0x01268086 || id == 0x01028086 \ >> || id == 0x01128086 || id == 0x01228086 \ >> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h >> index d46c61fca9..44be337dec 100644 >> --- a/xen/include/public/arch-arm.h >> +++ b/xen/include/public/arch-arm.h >> @@ -418,6 +418,13 @@ typedef uint64_t xen_callback_t; >> #define GUEST_GICV3_GICR0_BASE xen_mk_ullong(0x03020000) /* vCPU0..127 */ >> #define GUEST_GICV3_GICR0_SIZE xen_mk_ullong(0x01000000) >> >> +/* >> + * 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) >> + >> /* ACPI tables physical address */ >> #define GUEST_ACPI_BASE xen_mk_ullong(0x20000000) >> #define GUEST_ACPI_SIZE xen_mk_ullong(0x02000000) >> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h >> index 70ac25345c..db18cb7639 100644 >> --- a/xen/include/xen/pci.h >> +++ b/xen/include/xen/pci.h >> @@ -40,6 +40,9 @@ >> #define PCI_SBDF3(s,b,df) \ >> ((pci_sbdf_t){ .sbdf = (((s) & 0xffff) << 16) | PCI_BDF2(b, df) }) >> >> +#define ECAM_BDF(addr) ( ((addr) & 0x0ffff000) >> 12) > ^ extra space? > > Thanks, Roger. >
Hi Roger, > On 15 Oct 2021, at 09:32, Roger Pau Monné <roger.pau@citrix.com> wrote: > > On Thu, Oct 14, 2021 at 03:49:50PM +0100, Bertrand Marquis wrote: >> From: Rahul Singh <rahul.singh@arm.com> >> >> 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@arm.com> >> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> >> --- >> 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@kernel.org> >> 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@arm.com/ [2] https://patchwork.kernel.org/project/xen-devel/patch/f093de681c2560a7196895bcd666ef8840885c1d.1633340795.git.rahul.singh@arm.com/ Regards Bertrand
Jan Beulich writes ("Re: [PATCH v6 2/3] xen/arm: Enable the existing x86 virtual PCI support for ARM."):
> On 15.10.2021 01:49, Stefano Stabellini wrote:
> > Replacing the 3 characters with 'x' solves the problem.
> > https://gitlab.com/xen-project/people/sstabellini/xen/-/pipelines/388721262
>
> Interesting. I thought we permit UTF-8 in the sources; see e.g.
> tools/tests/x86_emulator/simd-sha.c using Σ. Is mkheader.py in need
> of adjustment? Of course I can see that right now the easiest is to
> use ASCII x, but I think it was deliberate to use × here.
I think in general we should permit UTF-8 in sources.
But:
> Then again, with the goal of the public headers being usable with
> pretty old compilers as well (C89 being the assumed baseline),
> excluding them from the permission to use UTF-8 may also be quite
> reasonable.
This is a good reason to do otherwise for the public headers.
Maybe this should be documented in CODING_STYLE (can be done after
feature freeze obviously).
That python has such botched unicode handling is a bug of course but
we could decide we think it's a feature :-). In which case maybe
mkheaders should be adjusted to explicitly set IO to ascii so that
this will fail in local builds too.
Anyway, for now we need to replace the UTF-8 in this patch.
Thanks,
Ian.
Hi Jan, > On 15 Oct 2021, at 09:00, Jan Beulich <jbeulich@suse.com> wrote: > > On 14.10.2021 16:49, Bertrand Marquis wrote: >> @@ -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 >> + >> ret = 0; >> if ( !pdev->domain ) > > Elsewhere I did point out that you need to be careful wrt pdev->domain > being NULL. As the code in context clearly documents, you're now > adding handlers before that field was set. In comments to a prior > version I did already suggest to consider placing the new code inside > the if() in question (albeit at the time this was mainly to make clear > that error cleanup may not fiddle with things not put in place by the > iommu_enable_device() alternative path). This would have the further > benefit of making is crystal clear that here only handlers for Dom0 > would get put in place (and hence their installing for DomU-s would > need to be looked for elsewhere). I asked Oleksandr for confirmation here but on arm there will be 2 other use cases: - PCI own by a DomD so device enumeration done from there - dom0less with devices detection done inside Xen > >> @@ -784,6 +797,9 @@ out: >> &PCI_SBDF(seg, bus, slot, func)); >> } >> } >> + else if ( pdev ) >> + pci_cleanup_msi(pdev); > > Have you thoroughly checked that this call is benign on x86? There's > no wording to that effect in the description afaics, and I can't > easily convince myself that it would be correct when the > iommu_enable_device() path was taken. (I'm also not going to > exclude that it should have been there even prior to your work, > albeit then adding this would want to be a separate bugfix patch.) This was not in the original serie and requested by Stefano. I must admit I am not completely sure on the details here so I am really ok to remove this but this would go against what was requested on the previous versions (4 and 5). > >> --- a/xen/drivers/vpci/header.c >> +++ b/xen/drivers/vpci/header.c >> @@ -374,7 +374,8 @@ static void bar_write(const struct pci_dev *pdev, unsigned int reg, >> if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) ) >> gprintk(XENLOG_WARNING, >> "%pp: ignored BAR %lu write with memory decoding enabled\n", >> - &pdev->sbdf, bar - pdev->vpci->header.bars + hi); >> + &pdev->sbdf, >> + (unsigned long)(bar - pdev->vpci->header.bars + hi)); > > This looks like an entirely unrelated change which I'm afraid I don't > even understand why it needs making. The description says this is for > Arm32, but it remains unclear what the compilation error would have > been. My best guess is that perhaps you really mean to change the > format specifier to %zu (really this should be %td, but our vsprintf() > doesn't support 't' for whatever reason). Compilation error is about an invalid %lu print for an unsigned int value when compiled for arm32. I will use zu instead in the next version and remove the cast. > > Please recall that we try to avoid casts where possible. Sure I will. > >> --- a/xen/drivers/vpci/vpci.c >> +++ b/xen/drivers/vpci/vpci.c >> @@ -507,7 +507,7 @@ int vpci_ecam_mmio_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int len, >> >> vpci_write(sbdf, reg, min(4u, len), data); >> if ( len == 8 ) >> - vpci_write(sbdf, reg + 4, 4, data >> 32); >> + vpci_write(sbdf, reg + 4, 4, (uint64_t)data >> 32); > > I assume the need for this change will go away with the use of > CONFIG_64BIT in the earlier patch. Yes > >> --- a/xen/include/xen/pci.h >> +++ b/xen/include/xen/pci.h >> @@ -40,6 +40,9 @@ >> #define PCI_SBDF3(s,b,df) \ >> ((pci_sbdf_t){ .sbdf = (((s) & 0xffff) << 16) | PCI_BDF2(b, df) }) >> >> +#define ECAM_BDF(addr) ( ((addr) & 0x0ffff000) >> 12) >> +#define ECAM_REG_OFFSET(addr) ((addr) & 0x00000fff) > > The latter is fine to be put here (i.e. FTAOD I'm fine with it > staying here). For the former I even question its original placement > in asm-x86/pci.h: It's not generally correct as per the PCI spec, as > the bus portion of the address can be anywhere from 1 to 8 bits. And > in fact there is a reason why this macro was/is used in only a > single place, but not e.g. in x86'es handling of physical MCFG. It > is merely an implementation choice in vPCI that the entire segment 0 > has a linear address range covering all 256 buses. Hence I think > this wants to move to xen/vpci.h and then perhaps also be named > VPCI_ECAM_BDF(). On previous version it was request to renamed this to ECAM and agreed to put is here. Now you want me to rename it to VPCI and move it again. I would like to have a confirmation that this is ok and the final move if possible. @Roger can you confirm this is what is wanted ? Also if I have to do this I will do the same for REG_OFFSET of course. > > Also, as already pointed out on a much earlier version, while the > blank following the opening parenthesis was warranted in > asm-x86/pci.h for alignment reasons, it is no longer warranted here. > It was certainly gone in v4 and v5. Yes my mistake during the process I did a copy and paste from the original one and did not modify. I will fix that. > > And one final nit: I don't think we commonly use full stops in patch > titles. (Personally I therefore also think titles shouldn't start > with a capital letter, but I know others think differently.) Ok I will fix the patch name. Cheers Bertrand > > Jan >
Hi Stefano, > On 15 Oct 2021, at 00:49, Stefano Stabellini <sstabellini@kernel.org> wrote: > > On Thu, 14 Oct 2021, Bertrand Marquis wrote: >> From: Rahul Singh <rahul.singh@arm.com> >> >> 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 > > Thank you for these! Strictly speaking they are not required now but > they are welcome. I would also be OK if they were removed from this > patch but it is fine to have them in here. > > There is an issues with this patch, see below at the bottom > > >> Signed-off-by: Rahul Singh <rahul.singh@arm.com> >> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> >> >> --- >> 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) >> - 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@kernel.org> >> 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); >> +} >> + >> +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 >> + >> ret = 0; >> if ( !pdev->domain ) >> { >> @@ -784,6 +797,9 @@ out: >> &PCI_SBDF(seg, bus, slot, func)); >> } >> } >> + else if ( pdev ) >> + pci_cleanup_msi(pdev); >> + >> return ret; >> } >> >> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c >> index f8cd55e7c0..c5b025b88b 100644 >> --- a/xen/drivers/vpci/header.c >> +++ b/xen/drivers/vpci/header.c >> @@ -374,7 +374,8 @@ static void bar_write(const struct pci_dev *pdev, unsigned int reg, >> if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) ) >> gprintk(XENLOG_WARNING, >> "%pp: ignored BAR %lu write with memory decoding enabled\n", >> - &pdev->sbdf, bar - pdev->vpci->header.bars + hi); >> + &pdev->sbdf, >> + (unsigned long)(bar - pdev->vpci->header.bars + hi)); >> return; >> } >> >> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c >> index c0853176d7..2bd67fc27a 100644 >> --- a/xen/drivers/vpci/vpci.c >> +++ b/xen/drivers/vpci/vpci.c >> @@ -507,7 +507,7 @@ int vpci_ecam_mmio_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int len, >> >> vpci_write(sbdf, reg, min(4u, len), data); >> if ( len == 8 ) >> - vpci_write(sbdf, reg + 4, 4, data >> 32); >> + vpci_write(sbdf, reg + 4, 4, (uint64_t)data >> 32); >> >> return 1; >> } >> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h >> index 14e575288f..9b3647587a 100644 >> --- a/xen/include/asm-arm/domain.h >> +++ b/xen/include/asm-arm/domain.h >> @@ -263,6 +263,7 @@ static inline void arch_vcpu_block(struct vcpu *v) {} >> >> #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag) >> >> +/* vPCI is not available on Arm */ >> #define has_vpci(d) ({ (void)(d); false; }) >> >> #endif /* __ASM_DOMAIN_H__ */ >> diff --git a/xen/include/asm-x86/pci.h b/xen/include/asm-x86/pci.h >> index a0df5c1279..443f25347d 100644 >> --- a/xen/include/asm-x86/pci.h >> +++ b/xen/include/asm-x86/pci.h >> @@ -6,8 +6,6 @@ >> #define CF8_ADDR_HI(cf8) ( ((cf8) & 0x0f000000) >> 16) >> #define CF8_ENABLED(cf8) (!!((cf8) & 0x80000000)) >> >> -#define ECAM_BDF(addr) ( ((addr) & 0x0ffff000) >> 12) >> - >> #define IS_SNB_GFX(id) (id == 0x01068086 || id == 0x01168086 \ >> || id == 0x01268086 || id == 0x01028086 \ >> || id == 0x01128086 || id == 0x01228086 \ >> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h >> index d46c61fca9..44be337dec 100644 >> --- a/xen/include/public/arch-arm.h >> +++ b/xen/include/public/arch-arm.h >> @@ -418,6 +418,13 @@ typedef uint64_t xen_callback_t; >> #define GUEST_GICV3_GICR0_BASE xen_mk_ullong(0x03020000) /* vCPU0..127 */ >> #define GUEST_GICV3_GICR0_SIZE xen_mk_ullong(0x01000000) >> >> +/* >> + * 256 MB is reserved for VPCI configuration space based on calculation >> + * 256 buses × 32 devices × 8 functions × 4 KB = 256 MB >> + */ > > Somehow 3 non-ascii characters sneaked into this patch. The 'x' are not > 'x' but are 0xc3 and cause the following errors in a few gitlab-ci > builds: > > python3 mkheader.py arm32 arm32.h.tmp /builds/xen-project/people/sstabellini/xen/tools/include/xen-foreign/../../../xen/include/public/arch-arm.h /builds/xen-project/people/sstabellini/xen/tools/include/xen-foreign/../../../xen/include/public/xen.h > Traceback (most recent call last): > File "mkheader.py", line 120, in <module> > input += f.read(); > File "/usr/lib/python3.5/encodings/ascii.py", line 26, in decode > return codecs.ascii_decode(input, self.errors)[0] > UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position 14641: ordinal not in range(128) > Makefile:28: recipe for target 'arm32.h' failed > make[2]: *** [arm32.h] Error 1 > > Full logs here: > https://gitlab.com/xen-project/people/sstabellini/xen/-/pipelines/38855078 > https://gitlab.com/xen-project/people/sstabellini/xen/-/jobs/16810108756 > > > Replacing the 3 characters with 'x' solves the problem. > https://gitlab.com/xen-project/people/sstabellini/xen/-/pipelines/388721262 > > > With the three 'x' changed to ascii: > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> I will fix that but sadly seeing all other changes to be done I do not think I can keep your R-b. Thanks Bertrand > > > >> +#define GUEST_VPCI_ECAM_BASE xen_mk_ullong(0x10000000) >> +#define GUEST_VPCI_ECAM_SIZE xen_mk_ullong(0x10000000) >> + >> /* ACPI tables physical address */ >> #define GUEST_ACPI_BASE xen_mk_ullong(0x20000000) >> #define GUEST_ACPI_SIZE xen_mk_ullong(0x02000000) >> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h >> index 70ac25345c..db18cb7639 100644 >> --- a/xen/include/xen/pci.h >> +++ b/xen/include/xen/pci.h >> @@ -40,6 +40,9 @@ >> #define PCI_SBDF3(s,b,df) \ >> ((pci_sbdf_t){ .sbdf = (((s) & 0xffff) << 16) | PCI_BDF2(b, df) }) >> >> +#define ECAM_BDF(addr) ( ((addr) & 0x0ffff000) >> 12) >> +#define ECAM_REG_OFFSET(addr) ((addr) & 0x00000fff) >> + >> typedef union { >> uint32_t sbdf; >> struct { >> -- >> 2.25.1
> On 15 Oct 2021, at 10:52, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote: > > Hi Roger, > >> On 15 Oct 2021, at 09:32, Roger Pau Monné <roger.pau@citrix.com> wrote: >> >> On Thu, Oct 14, 2021 at 03:49:50PM +0100, Bertrand Marquis wrote: >>> From: Rahul Singh <rahul.singh@arm.com> >>> >>> 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@arm.com> >>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> >>> --- >>> 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@kernel.org> >>> 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 ? For my understanding, as also Jan pointed out, this should be put inside the If ( !pdev->domain ) because also as the comment suggests, this operation should be performed only when there is a Dom0 and when we are using a dom0less setup the pdev->domain should be NULL if I’m not wrong. @oleksandr maybe can confirm > >> >>> + >>> 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@arm.com/ > [2] https://patchwork.kernel.org/project/xen-devel/patch/f093de681c2560a7196895bcd666ef8840885c1d.1633340795.git.rahul.singh@arm.com/ > > Regards > Bertrand
Bertrand Marquis writes ("Re: [PATCH v6 2/3] xen/arm: Enable the existing x86 virtual PCI support for ARM."):
> > On 15 Oct 2021, at 09:00, Jan Beulich <jbeulich@suse.com> wrote:
> > The latter is fine to be put here (i.e. FTAOD I'm fine with it
> > staying here). For the former I even question its original placement
> > in asm-x86/pci.h: It's not generally correct as per the PCI spec, as
> > the bus portion of the address can be anywhere from 1 to 8 bits. And
> > in fact there is a reason why this macro was/is used in only a
> > single place, but not e.g. in x86'es handling of physical MCFG. It
> > is merely an implementation choice in vPCI that the entire segment 0
> > has a linear address range covering all 256 buses. Hence I think
> > this wants to move to xen/vpci.h and then perhaps also be named
> > VPCI_ECAM_BDF().
>
> On previous version it was request to renamed this to ECAM and agreed
> to put is here. Now you want me to rename it to VPCI and move it again.
> I would like to have a confirmation that this is ok and the final move if possible.
>
> @Roger can you confirm this is what is wanted ?
I think Roger is not available today I'm afraid.
Bertrand, can you give me a link to the comment from Roger ?
Assuming that it says what I think it will say:
I think the best thing to do will be to leave the name as it was in
the most recent version of your series. I don't think it makes sense
to block this patch over a naming disagreement. And it would be best
to minimise unnecessary churn.
I would be happy to release-ack a name change (perhaps proposed bo Jan
or Roger) supposing that that is the ultimate maintainer consensus.
Jan, would that approach be OK with you ?
Ian.
Hi Luca, > On 15 Oct 2021, at 11:13, Luca Fancellu <Luca.Fancellu@arm.com> wrote: > > > >> On 15 Oct 2021, at 10:52, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote: >> >> Hi Roger, >> >>> On 15 Oct 2021, at 09:32, Roger Pau Monné <roger.pau@citrix.com> wrote: >>> >>> On Thu, Oct 14, 2021 at 03:49:50PM +0100, Bertrand Marquis wrote: >>>> From: Rahul Singh <rahul.singh@arm.com> >>>> >>>> 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@arm.com> >>>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> >>>> --- >>>> 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@kernel.org> >>>> 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 ? > > For my understanding, as also Jan pointed out, this should be put inside the > If ( !pdev->domain ) because also as the comment suggests, this operation > should be performed only when there is a Dom0 and when we are using > a dom0less setup the pdev->domain should be NULL if I’m not wrong. > @oleksandr maybe can confirm You are right. I will fix that and move that into an if. Regards Bertrand > >> >>> >>>> + >>>> 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@arm.com/ >> [2] https://patchwork.kernel.org/project/xen-devel/patch/f093de681c2560a7196895bcd666ef8840885c1d.1633340795.git.rahul.singh@arm.com/ >> >> Regards >> Bertrand
On 15.10.2021 12:14, Ian Jackson wrote: > Bertrand Marquis writes ("Re: [PATCH v6 2/3] xen/arm: Enable the existing x86 virtual PCI support for ARM."): >>> On 15 Oct 2021, at 09:00, Jan Beulich <jbeulich@suse.com> wrote: >>> The latter is fine to be put here (i.e. FTAOD I'm fine with it >>> staying here). For the former I even question its original placement >>> in asm-x86/pci.h: It's not generally correct as per the PCI spec, as >>> the bus portion of the address can be anywhere from 1 to 8 bits. And >>> in fact there is a reason why this macro was/is used in only a >>> single place, but not e.g. in x86'es handling of physical MCFG. It >>> is merely an implementation choice in vPCI that the entire segment 0 >>> has a linear address range covering all 256 buses. Hence I think >>> this wants to move to xen/vpci.h and then perhaps also be named >>> VPCI_ECAM_BDF(). >> >> On previous version it was request to renamed this to ECAM and agreed >> to put is here. Now you want me to rename it to VPCI and move it again. >> I would like to have a confirmation that this is ok and the final move if possible. >> >> @Roger can you confirm this is what is wanted ? > > I think Roger is not available today I'm afraid. > > Bertrand, can you give me a link to the comment from Roger ? > Assuming that it says what I think it will say: > > I think the best thing to do will be to leave the name as it was in > the most recent version of your series. I don't think it makes sense > to block this patch over a naming disagreement. And it would be best > to minimise unnecessary churn. > > I would be happy to release-ack a name change (perhaps proposed bo Jan > or Roger) supposing that that is the ultimate maintainer consensus. > > Jan, would that approach be OK with you ? Well, yes, if a subsequent name change is okay, then I could live with that. I'd still find it odd to rename a function immediately after it already got renamed. As expressed elsewhere, I suspect in his request Roger did not pay attention to a use of the function in non-ECAM code. Jan
On Fri, Oct 15, 2021 at 09:52:28AM +0000, Bertrand Marquis wrote: > Hi Roger, > > > On 15 Oct 2021, at 09:32, Roger Pau Monné <roger.pau@citrix.com> wrote: > > > > On Thu, Oct 14, 2021 at 03:49:50PM +0100, Bertrand Marquis wrote: > >> From: Rahul Singh <rahul.singh@arm.com> > >> 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. vpci_add_handlers will try to access pdev->domain, so passing a pdev without domain being set is wrong. > @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). pci_add_device being solely used by trusted domains, I think it would be fine to require that the domain doesn't poke the PCI config space until the call to pci_add_device has finished. Else it's likely to get inconsistent results, or mess up with the device state. In any case, such addition needs some kind of reasoning added to the commit message if it's really required. Thanks, Roger.
On 15.10.2021 11:52, Bertrand Marquis wrote: >> On 15 Oct 2021, at 09:32, Roger Pau Monné <roger.pau@citrix.com> wrote: >> On Thu, Oct 14, 2021 at 03:49:50PM +0100, Bertrand Marquis wrote: >>> @@ -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 ? Well, without Xen doing the enumeration, some other entity would need to do so, including the reporting to Xen. Obviously without a Dom0 it would be ambiguous which domain to assign the device to; perhaps it should be the caller in this case? That would make that caller domain a pseudo-hwdom though, as far as PCI is concerned, which may not be desirable according to my (limited) understanding of dom0less. >>> @@ -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@arm.com/ > [2] https://patchwork.kernel.org/project/xen-devel/patch/f093de681c2560a7196895bcd666ef8840885c1d.1633340795.git.rahul.singh@arm.com/ The addition of this call has repeatedly raised questions. This is a good indication that sufficient discussion thereof has been lacking from the patch description. Jan
Hi, > On 15 Oct 2021, at 11:19, Roger Pau Monné <roger.pau@citrix.com> wrote: > > On Fri, Oct 15, 2021 at 09:52:28AM +0000, Bertrand Marquis wrote: >> Hi Roger, >> >>> On 15 Oct 2021, at 09:32, Roger Pau Monné <roger.pau@citrix.com> wrote: >>> >>> On Thu, Oct 14, 2021 at 03:49:50PM +0100, Bertrand Marquis wrote: >>>> From: Rahul Singh <rahul.singh@arm.com> >>>> 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. > > vpci_add_handlers will try to access pdev->domain, so passing a pdev > without domain being set is wrong. Right and the initial comment from Rahul in the code is saying so. I will move the block inside the if. > >> @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). > > pci_add_device being solely used by trusted domains, I think it would > be fine to require that the domain doesn't poke the PCI config space > until the call to pci_add_device has finished. Else it's likely to get > inconsistent results, or mess up with the device state. Ack. > > In any case, such addition needs some kind of reasoning added to the > commit message if it's really required. With the code moving inside the following else and pci_cleanup_msi being empty for now on arm, I will remove the pci_cleanup_msi as it does not related directly to this change for now and might change the behaviour on x86. If this is needed at some point due to arm, this will be done once msi support will be added. Regards Bertrand > > Thanks, Roger.
> On 15 Oct 2021, at 11:24, Jan Beulich <jbeulich@suse.com> wrote: > > On 15.10.2021 11:52, Bertrand Marquis wrote: >>> On 15 Oct 2021, at 09:32, Roger Pau Monné <roger.pau@citrix.com> wrote: >>> On Thu, Oct 14, 2021 at 03:49:50PM +0100, Bertrand Marquis wrote: >>>> @@ -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 ? > > Well, without Xen doing the enumeration, some other entity would need > to do so, including the reporting to Xen. Obviously without a Dom0 it > would be ambiguous which domain to assign the device to; perhaps it > should be the caller in this case? That would make that caller domain > a pseudo-hwdom though, as far as PCI is concerned, which may not be > desirable according to my (limited) understanding of dom0less. This is not really related to this patch but the plan is the following: - enumeration would have to be done by the firmware or boot loader before - xen will have some code to detect PCI devices - dom0less can be used to assign PCI devices to guest Anyway does not change the fact that this must be called when domain is not NULL and I will fix that. > >>>> @@ -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@arm.com/ >> [2] https://patchwork.kernel.org/project/xen-devel/patch/f093de681c2560a7196895bcd666ef8840885c1d.1633340795.git.rahul.singh@arm.com/ > > The addition of this call has repeatedly raised questions. This is a > good indication that sufficient discussion thereof has been lacking > from the patch description. Yes and I will remove it as it only impacts x86 right now. If this is needed, we will have to do it while adding MSI support on Arm. Regards Bertrand > > Jan >
On 15.10.2021 12:09, Bertrand Marquis wrote: >> On 15 Oct 2021, at 09:00, Jan Beulich <jbeulich@suse.com> wrote: >> On 14.10.2021 16:49, Bertrand Marquis wrote: >>> @@ -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 >>> + >>> ret = 0; >>> if ( !pdev->domain ) >> >> Elsewhere I did point out that you need to be careful wrt pdev->domain >> being NULL. As the code in context clearly documents, you're now >> adding handlers before that field was set. In comments to a prior >> version I did already suggest to consider placing the new code inside >> the if() in question (albeit at the time this was mainly to make clear >> that error cleanup may not fiddle with things not put in place by the >> iommu_enable_device() alternative path). This would have the further >> benefit of making is crystal clear that here only handlers for Dom0 >> would get put in place (and hence their installing for DomU-s would >> need to be looked for elsewhere). > > I asked Oleksandr for confirmation here but on arm there will be 2 other use cases: > - PCI own by a DomD so device enumeration done from there > - dom0less with devices detection done inside Xen Question is whether at this time you mean to handle all these cases. Installing handlers when device detection happens in Xen might need to be done differently, for example - e.g. more along the lines of what x86 PVH Dom0 has done. Anything you don't mean to handle (and is safe to be left out, i.e. without breaking existing cases) will want spelling out in the description. >>> @@ -784,6 +797,9 @@ out: >>> &PCI_SBDF(seg, bus, slot, func)); >>> } >>> } >>> + else if ( pdev ) >>> + pci_cleanup_msi(pdev); >> >> Have you thoroughly checked that this call is benign on x86? There's >> no wording to that effect in the description afaics, and I can't >> easily convince myself that it would be correct when the >> iommu_enable_device() path was taken. (I'm also not going to >> exclude that it should have been there even prior to your work, >> albeit then adding this would want to be a separate bugfix patch.) > > This was not in the original serie and requested by Stefano. I must admit > I am not completely sure on the details here so I am really ok to remove this > but this would go against what was requested on the previous versions (4 and 5). I understand this, but a request to add something still requires that it be checked that the addition can't have bad effects in other cases. A compromise here (which I wouldn't like very much) might be to add another #ifdef CONFIG_ARM, solely to leave the existing x86 case undisturbed. Yet then, with the called function doing nothing on Arm, not adding the code here might as well be an option (which it looked like Stefano would be amenable to). A TODO item would then perhaps best be left here in a comment. >>> --- a/xen/include/xen/pci.h >>> +++ b/xen/include/xen/pci.h >>> @@ -40,6 +40,9 @@ >>> #define PCI_SBDF3(s,b,df) \ >>> ((pci_sbdf_t){ .sbdf = (((s) & 0xffff) << 16) | PCI_BDF2(b, df) }) >>> >>> +#define ECAM_BDF(addr) ( ((addr) & 0x0ffff000) >> 12) >>> +#define ECAM_REG_OFFSET(addr) ((addr) & 0x00000fff) >> >> The latter is fine to be put here (i.e. FTAOD I'm fine with it >> staying here). For the former I even question its original placement >> in asm-x86/pci.h: It's not generally correct as per the PCI spec, as >> the bus portion of the address can be anywhere from 1 to 8 bits. And >> in fact there is a reason why this macro was/is used in only a >> single place, but not e.g. in x86'es handling of physical MCFG. It >> is merely an implementation choice in vPCI that the entire segment 0 >> has a linear address range covering all 256 buses. Hence I think >> this wants to move to xen/vpci.h and then perhaps also be named >> VPCI_ECAM_BDF(). > > On previous version it was request to renamed this to ECAM and agreed > to put is here. Now you want me to rename it to VPCI and move it again. > I would like to have a confirmation that this is ok and the final move > if possible. A final confirmation is, unfortunately, only as final as it can be at the very moment it is given. It was the MCFG vs ECAM naming discussion which made me pull out again the section in the spec, reminding me of aspects I didn't previously take into consideration. I'm sorry for this, but it's an iterative process on all sides. So, FTAOD, as a maintainer of this headed I will continue to object to a non-spec-compliant construct to be put into here. It'll be Roger, being the vPCI maintainer, to confirm that putting it in xen/vpci.h is fine. > Also if I have to do this I will do the same for REG_OFFSET of course. As said - that one, being in line with the PCI spec, is fine to remain as and where you have it. Jan
On 15.10.2021 12:33, Bertrand Marquis wrote: >> On 15 Oct 2021, at 11:24, Jan Beulich <jbeulich@suse.com> wrote: >> On 15.10.2021 11:52, Bertrand Marquis wrote: >>>> On 15 Oct 2021, at 09:32, Roger Pau Monné <roger.pau@citrix.com> wrote: >>>> On Thu, Oct 14, 2021 at 03:49:50PM +0100, Bertrand Marquis wrote: >>>>> @@ -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 ? >> >> Well, without Xen doing the enumeration, some other entity would need >> to do so, including the reporting to Xen. Obviously without a Dom0 it >> would be ambiguous which domain to assign the device to; perhaps it >> should be the caller in this case? That would make that caller domain >> a pseudo-hwdom though, as far as PCI is concerned, which may not be >> desirable according to my (limited) understanding of dom0less. > > This is not really related to this patch but the plan is the following: > - enumeration would have to be done by the firmware or boot loader before > - xen will have some code to detect PCI devices > - dom0less can be used to assign PCI devices to guest > > Anyway does not change the fact that this must be called when domain is > not NULL and I will fix that. Since we now all seem to agree that the NULL would have been a problem, may I ask in how far any of this has actually been tested? Jan
Hi Jan, > On 15 Oct 2021, at 11:41, Jan Beulich <jbeulich@suse.com> wrote: > > On 15.10.2021 12:33, Bertrand Marquis wrote: >>> On 15 Oct 2021, at 11:24, Jan Beulich <jbeulich@suse.com> wrote: >>> On 15.10.2021 11:52, Bertrand Marquis wrote: >>>>> On 15 Oct 2021, at 09:32, Roger Pau Monné <roger.pau@citrix.com> wrote: >>>>> On Thu, Oct 14, 2021 at 03:49:50PM +0100, Bertrand Marquis wrote: >>>>>> @@ -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 ? >>> >>> Well, without Xen doing the enumeration, some other entity would need >>> to do so, including the reporting to Xen. Obviously without a Dom0 it >>> would be ambiguous which domain to assign the device to; perhaps it >>> should be the caller in this case? That would make that caller domain >>> a pseudo-hwdom though, as far as PCI is concerned, which may not be >>> desirable according to my (limited) understanding of dom0less. >> >> This is not really related to this patch but the plan is the following: >> - enumeration would have to be done by the firmware or boot loader before >> - xen will have some code to detect PCI devices >> - dom0less can be used to assign PCI devices to guest >> >> Anyway does not change the fact that this must be called when domain is >> not NULL and I will fix that. > > Since we now all seem to agree that the NULL would have been a problem, > may I ask in how far any of this has actually been tested? With the whole serie currently on gitlab we have extensively tested passing through PCI devices on Arm in several configuration (number of device, MSI, MSI-X) and check that PCI was still functional on x86. With the patches pushed to Xen right now it was checked that: - xen compiles properly on arm32, arm64 and x86 - xen compiles properly with VPCI activated (using a patch) on arm32 and arm64 - xen on x86 is functionnal (using basic test on QEMU) - xen on arm64 is functionnal (with some extensive tests on different targets) We are only lacking some actual testing on arm32 internally. Regards Bertrand > > Jan
On 15.10.2021 12:48, Bertrand Marquis wrote: > Hi Jan, > >> On 15 Oct 2021, at 11:41, Jan Beulich <jbeulich@suse.com> wrote: >> >> On 15.10.2021 12:33, Bertrand Marquis wrote: >>>> On 15 Oct 2021, at 11:24, Jan Beulich <jbeulich@suse.com> wrote: >>>> On 15.10.2021 11:52, Bertrand Marquis wrote: >>>>>> On 15 Oct 2021, at 09:32, Roger Pau Monné <roger.pau@citrix.com> wrote: >>>>>> On Thu, Oct 14, 2021 at 03:49:50PM +0100, Bertrand Marquis wrote: >>>>>>> @@ -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 ? >>>> >>>> Well, without Xen doing the enumeration, some other entity would need >>>> to do so, including the reporting to Xen. Obviously without a Dom0 it >>>> would be ambiguous which domain to assign the device to; perhaps it >>>> should be the caller in this case? That would make that caller domain >>>> a pseudo-hwdom though, as far as PCI is concerned, which may not be >>>> desirable according to my (limited) understanding of dom0less. >>> >>> This is not really related to this patch but the plan is the following: >>> - enumeration would have to be done by the firmware or boot loader before >>> - xen will have some code to detect PCI devices >>> - dom0less can be used to assign PCI devices to guest >>> >>> Anyway does not change the fact that this must be called when domain is >>> not NULL and I will fix that. >> >> Since we now all seem to agree that the NULL would have been a problem, >> may I ask in how far any of this has actually been tested? > > With the whole serie currently on gitlab we have extensively tested passing > through PCI devices on Arm in several configuration (number of device, MSI, > MSI-X) and check that PCI was still functional on x86. > > With the patches pushed to Xen right now it was checked that: > - xen compiles properly on arm32, arm64 and x86 > - xen compiles properly with VPCI activated (using a patch) on arm32 and arm64 > - xen on x86 is functionnal (using basic test on QEMU) > - xen on arm64 is functionnal (with some extensive tests on different targets) But somehow in the testing done you must have avoided the code path in question, or else you would have observed a crash. Jan
Hi, > On 15 Oct 2021, at 11:51, Jan Beulich <jbeulich@suse.com> wrote: > > On 15.10.2021 12:48, Bertrand Marquis wrote: >> Hi Jan, >> >>> On 15 Oct 2021, at 11:41, Jan Beulich <jbeulich@suse.com> wrote: >>> >>> On 15.10.2021 12:33, Bertrand Marquis wrote: >>>>> On 15 Oct 2021, at 11:24, Jan Beulich <jbeulich@suse.com> wrote: >>>>> On 15.10.2021 11:52, Bertrand Marquis wrote: >>>>>>> On 15 Oct 2021, at 09:32, Roger Pau Monné <roger.pau@citrix.com> wrote: >>>>>>> On Thu, Oct 14, 2021 at 03:49:50PM +0100, Bertrand Marquis wrote: >>>>>>>> @@ -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 ? >>>>> >>>>> Well, without Xen doing the enumeration, some other entity would need >>>>> to do so, including the reporting to Xen. Obviously without a Dom0 it >>>>> would be ambiguous which domain to assign the device to; perhaps it >>>>> should be the caller in this case? That would make that caller domain >>>>> a pseudo-hwdom though, as far as PCI is concerned, which may not be >>>>> desirable according to my (limited) understanding of dom0less. >>>> >>>> This is not really related to this patch but the plan is the following: >>>> - enumeration would have to be done by the firmware or boot loader before >>>> - xen will have some code to detect PCI devices >>>> - dom0less can be used to assign PCI devices to guest >>>> >>>> Anyway does not change the fact that this must be called when domain is >>>> not NULL and I will fix that. >>> >>> Since we now all seem to agree that the NULL would have been a problem, >>> may I ask in how far any of this has actually been tested? >> >> With the whole serie currently on gitlab we have extensively tested passing >> through PCI devices on Arm in several configuration (number of device, MSI, >> MSI-X) and check that PCI was still functional on x86. >> >> With the patches pushed to Xen right now it was checked that: >> - xen compiles properly on arm32, arm64 and x86 >> - xen compiles properly with VPCI activated (using a patch) on arm32 and arm64 >> - xen on x86 is functionnal (using basic test on QEMU) >> - xen on arm64 is functionnal (with some extensive tests on different targets) > > But somehow in the testing done you must have avoided the code path > in question, or else you would have observed a crash. Device_add is always called by dom0 so we did not have issues here. Dom0less PCI passthrough implementation is not done right now. So yes we did not go through this path. Bertrand > > Jan >
On Fri, Oct 15, 2021 at 12:18:59PM +0200, Jan Beulich wrote: > On 15.10.2021 12:14, Ian Jackson wrote: > > Bertrand Marquis writes ("Re: [PATCH v6 2/3] xen/arm: Enable the existing x86 virtual PCI support for ARM."): > >>> On 15 Oct 2021, at 09:00, Jan Beulich <jbeulich@suse.com> wrote: > >>> The latter is fine to be put here (i.e. FTAOD I'm fine with it > >>> staying here). For the former I even question its original placement > >>> in asm-x86/pci.h: It's not generally correct as per the PCI spec, as > >>> the bus portion of the address can be anywhere from 1 to 8 bits. And > >>> in fact there is a reason why this macro was/is used in only a > >>> single place, but not e.g. in x86'es handling of physical MCFG. It > >>> is merely an implementation choice in vPCI that the entire segment 0 > >>> has a linear address range covering all 256 buses. Hence I think > >>> this wants to move to xen/vpci.h and then perhaps also be named > >>> VPCI_ECAM_BDF(). > >> > >> On previous version it was request to renamed this to ECAM and agreed > >> to put is here. Now you want me to rename it to VPCI and move it again. > >> I would like to have a confirmation that this is ok and the final move if possible. > >> > >> @Roger can you confirm this is what is wanted ? > > > > I think Roger is not available today I'm afraid. > > > > Bertrand, can you give me a link to the comment from Roger ? > > Assuming that it says what I think it will say: > > > > I think the best thing to do will be to leave the name as it was in > > the most recent version of your series. I don't think it makes sense > > to block this patch over a naming disagreement. And it would be best > > to minimise unnecessary churn. > > > > I would be happy to release-ack a name change (perhaps proposed bo Jan > > or Roger) supposing that that is the ultimate maintainer consensus. > > > > Jan, would that approach be OK with you ? > > Well, yes, if a subsequent name change is okay, then I could live with > that. I'd still find it odd to rename a function immediately after it > already got renamed. As expressed elsewhere, I suspect in his request > Roger did not pay attention to a use of the function in non-ECAM code. Using MMCFG_BDF was original requested by Julien, not myself I think: https://lore.kernel.org/xen-devel/a868e1e7-8400-45df-6eaa-69f1e2c99383@xen.org/ I'm slightly loss in so many messages. On x86 we subtract the MCFG start address from the passed one before getting the BDF, and then we add the startting bus address passed in the ACPI table. This is so far not need on Arm AFAICT because of the fixed nature of the selected virtual ECAM region. Thanks, Roger.
Hi Roger, > On 15 Oct 2021, at 12:35, Roger Pau Monné <roger.pau@citrix.com> wrote: > > On Fri, Oct 15, 2021 at 12:18:59PM +0200, Jan Beulich wrote: >> On 15.10.2021 12:14, Ian Jackson wrote: >>> Bertrand Marquis writes ("Re: [PATCH v6 2/3] xen/arm: Enable the existing x86 virtual PCI support for ARM."): >>>>> On 15 Oct 2021, at 09:00, Jan Beulich <jbeulich@suse.com> wrote: >>>>> The latter is fine to be put here (i.e. FTAOD I'm fine with it >>>>> staying here). For the former I even question its original placement >>>>> in asm-x86/pci.h: It's not generally correct as per the PCI spec, as >>>>> the bus portion of the address can be anywhere from 1 to 8 bits. And >>>>> in fact there is a reason why this macro was/is used in only a >>>>> single place, but not e.g. in x86'es handling of physical MCFG. It >>>>> is merely an implementation choice in vPCI that the entire segment 0 >>>>> has a linear address range covering all 256 buses. Hence I think >>>>> this wants to move to xen/vpci.h and then perhaps also be named >>>>> VPCI_ECAM_BDF(). >>>> >>>> On previous version it was request to renamed this to ECAM and agreed >>>> to put is here. Now you want me to rename it to VPCI and move it again. >>>> I would like to have a confirmation that this is ok and the final move if possible. >>>> >>>> @Roger can you confirm this is what is wanted ? >>> >>> I think Roger is not available today I'm afraid. >>> >>> Bertrand, can you give me a link to the comment from Roger ? >>> Assuming that it says what I think it will say: >>> >>> I think the best thing to do will be to leave the name as it was in >>> the most recent version of your series. I don't think it makes sense >>> to block this patch over a naming disagreement. And it would be best >>> to minimise unnecessary churn. >>> >>> I would be happy to release-ack a name change (perhaps proposed bo Jan >>> or Roger) supposing that that is the ultimate maintainer consensus. >>> >>> Jan, would that approach be OK with you ? >> >> Well, yes, if a subsequent name change is okay, then I could live with >> that. I'd still find it odd to rename a function immediately after it >> already got renamed. As expressed elsewhere, I suspect in his request >> Roger did not pay attention to a use of the function in non-ECAM code. > > Using MMCFG_BDF was original requested by Julien, not myself I think: > > https://lore.kernel.org/xen-devel/a868e1e7-8400-45df-6eaa-69f1e2c99383@xen.org/ > > I'm slightly loss in so many messages. On x86 we subtract the MCFG > start address from the passed one before getting the BDF, and then we > add the startting bus address passed in the ACPI table. This is so far > not need on Arm AFAICT because of the fixed nature of the selected > virtual ECAM region. At the end my patch will add in xen/pci.h: #define ECAM_BDF(addr) (((addr) & 0x0ffff000) >> 12) #define ECAM_REG_OFFSET(addr) ((addr) & 0x00000fff) Now seeing the comment the question is should those be renamed with a VPCI prefix and be moved to xen/vpci.h. So far ECAM_BDF is only used in vpci_mmcfg_decode_addr which is only called before calling vpci_ecam_{read/write}. ECAM_REG_OFFSET is only used in arm vpci code. Do you think the current state is ok of the renaming + moving should be done ? Cheers Bertrand
On 15.10.2021 14:13, Bertrand Marquis wrote: > Hi Roger, > >> On 15 Oct 2021, at 12:35, Roger Pau Monné <roger.pau@citrix.com> wrote: >> >> On Fri, Oct 15, 2021 at 12:18:59PM +0200, Jan Beulich wrote: >>> On 15.10.2021 12:14, Ian Jackson wrote: >>>> Bertrand Marquis writes ("Re: [PATCH v6 2/3] xen/arm: Enable the existing x86 virtual PCI support for ARM."): >>>>>> On 15 Oct 2021, at 09:00, Jan Beulich <jbeulich@suse.com> wrote: >>>>>> The latter is fine to be put here (i.e. FTAOD I'm fine with it >>>>>> staying here). For the former I even question its original placement >>>>>> in asm-x86/pci.h: It's not generally correct as per the PCI spec, as >>>>>> the bus portion of the address can be anywhere from 1 to 8 bits. And >>>>>> in fact there is a reason why this macro was/is used in only a >>>>>> single place, but not e.g. in x86'es handling of physical MCFG. It >>>>>> is merely an implementation choice in vPCI that the entire segment 0 >>>>>> has a linear address range covering all 256 buses. Hence I think >>>>>> this wants to move to xen/vpci.h and then perhaps also be named >>>>>> VPCI_ECAM_BDF(). >>>>> >>>>> On previous version it was request to renamed this to ECAM and agreed >>>>> to put is here. Now you want me to rename it to VPCI and move it again. >>>>> I would like to have a confirmation that this is ok and the final move if possible. >>>>> >>>>> @Roger can you confirm this is what is wanted ? >>>> >>>> I think Roger is not available today I'm afraid. >>>> >>>> Bertrand, can you give me a link to the comment from Roger ? >>>> Assuming that it says what I think it will say: >>>> >>>> I think the best thing to do will be to leave the name as it was in >>>> the most recent version of your series. I don't think it makes sense >>>> to block this patch over a naming disagreement. And it would be best >>>> to minimise unnecessary churn. >>>> >>>> I would be happy to release-ack a name change (perhaps proposed bo Jan >>>> or Roger) supposing that that is the ultimate maintainer consensus. >>>> >>>> Jan, would that approach be OK with you ? >>> >>> Well, yes, if a subsequent name change is okay, then I could live with >>> that. I'd still find it odd to rename a function immediately after it >>> already got renamed. As expressed elsewhere, I suspect in his request >>> Roger did not pay attention to a use of the function in non-ECAM code. >> >> Using MMCFG_BDF was original requested by Julien, not myself I think: >> >> https://lore.kernel.org/xen-devel/a868e1e7-8400-45df-6eaa-69f1e2c99383@xen.org/ >> >> I'm slightly loss in so many messages. On x86 we subtract the MCFG >> start address from the passed one before getting the BDF, and then we >> add the startting bus address passed in the ACPI table. This is so far >> not need on Arm AFAICT because of the fixed nature of the selected >> virtual ECAM region. > > At the end my patch will add in xen/pci.h: > #define ECAM_BDF(addr) (((addr) & 0x0ffff000) >> 12) Since you still make this proposal, once again: I'm not going to accept such a macro in this header, whatever the name. Its prior placement was wrong as well. Only ... > #define ECAM_REG_OFFSET(addr) ((addr) & 0x00000fff) ... this one is fine to live here (and presumably it could gain uses elsewhere). Jan > Now seeing the comment the question is should those be renamed with a VPCI > prefix and be moved to xen/vpci.h. > > So far ECAM_BDF is only used in vpci_mmcfg_decode_addr which is only called > before calling vpci_ecam_{read/write}. > > ECAM_REG_OFFSET is only used in arm vpci code. > > Do you think the current state is ok of the renaming + moving should be done ? > > Cheers > Bertrand >
Hi Jan, > On 15 Oct 2021, at 13:18, Jan Beulich <jbeulich@suse.com> wrote: > > On 15.10.2021 14:13, Bertrand Marquis wrote: >> Hi Roger, >> >>> On 15 Oct 2021, at 12:35, Roger Pau Monné <roger.pau@citrix.com> wrote: >>> >>> On Fri, Oct 15, 2021 at 12:18:59PM +0200, Jan Beulich wrote: >>>> On 15.10.2021 12:14, Ian Jackson wrote: >>>>> Bertrand Marquis writes ("Re: [PATCH v6 2/3] xen/arm: Enable the existing x86 virtual PCI support for ARM."): >>>>>>> On 15 Oct 2021, at 09:00, Jan Beulich <jbeulich@suse.com> wrote: >>>>>>> The latter is fine to be put here (i.e. FTAOD I'm fine with it >>>>>>> staying here). For the former I even question its original placement >>>>>>> in asm-x86/pci.h: It's not generally correct as per the PCI spec, as >>>>>>> the bus portion of the address can be anywhere from 1 to 8 bits. And >>>>>>> in fact there is a reason why this macro was/is used in only a >>>>>>> single place, but not e.g. in x86'es handling of physical MCFG. It >>>>>>> is merely an implementation choice in vPCI that the entire segment 0 >>>>>>> has a linear address range covering all 256 buses. Hence I think >>>>>>> this wants to move to xen/vpci.h and then perhaps also be named >>>>>>> VPCI_ECAM_BDF(). >>>>>> >>>>>> On previous version it was request to renamed this to ECAM and agreed >>>>>> to put is here. Now you want me to rename it to VPCI and move it again. >>>>>> I would like to have a confirmation that this is ok and the final move if possible. >>>>>> >>>>>> @Roger can you confirm this is what is wanted ? >>>>> >>>>> I think Roger is not available today I'm afraid. >>>>> >>>>> Bertrand, can you give me a link to the comment from Roger ? >>>>> Assuming that it says what I think it will say: >>>>> >>>>> I think the best thing to do will be to leave the name as it was in >>>>> the most recent version of your series. I don't think it makes sense >>>>> to block this patch over a naming disagreement. And it would be best >>>>> to minimise unnecessary churn. >>>>> >>>>> I would be happy to release-ack a name change (perhaps proposed bo Jan >>>>> or Roger) supposing that that is the ultimate maintainer consensus. >>>>> >>>>> Jan, would that approach be OK with you ? >>>> >>>> Well, yes, if a subsequent name change is okay, then I could live with >>>> that. I'd still find it odd to rename a function immediately after it >>>> already got renamed. As expressed elsewhere, I suspect in his request >>>> Roger did not pay attention to a use of the function in non-ECAM code. >>> >>> Using MMCFG_BDF was original requested by Julien, not myself I think: >>> >>> https://lore.kernel.org/xen-devel/a868e1e7-8400-45df-6eaa-69f1e2c99383@xen.org/ >>> >>> I'm slightly loss in so many messages. On x86 we subtract the MCFG >>> start address from the passed one before getting the BDF, and then we >>> add the startting bus address passed in the ACPI table. This is so far >>> not need on Arm AFAICT because of the fixed nature of the selected >>> virtual ECAM region. >> >> At the end my patch will add in xen/pci.h: >> #define ECAM_BDF(addr) (((addr) & 0x0ffff000) >> 12) > > Since you still make this proposal, once again: I'm not going to > accept such a macro in this header, whatever the name. Its prior > placement was wrong as well. Only ... > >> #define ECAM_REG_OFFSET(addr) ((addr) & 0x00000fff) > > ... this one is fine to live here (and presumably it could gain uses > elsewhere). So you would agree if they are both moved to vpci.h with a VPCI_ prefix ? Bertrand > > Jan > >> Now seeing the comment the question is should those be renamed with a VPCI >> prefix and be moved to xen/vpci.h. >> >> So far ECAM_BDF is only used in vpci_mmcfg_decode_addr which is only called >> before calling vpci_ecam_{read/write}. >> >> ECAM_REG_OFFSET is only used in arm vpci code. >> >> Do you think the current state is ok of the renaming + moving should be done ? >> >> Cheers >> Bertrand
On 15.10.2021 14:28, Bertrand Marquis wrote: >> On 15 Oct 2021, at 13:18, Jan Beulich <jbeulich@suse.com> wrote: >> On 15.10.2021 14:13, Bertrand Marquis wrote: >>>> On 15 Oct 2021, at 12:35, Roger Pau Monné <roger.pau@citrix.com> wrote: >>>> On Fri, Oct 15, 2021 at 12:18:59PM +0200, Jan Beulich wrote: >>>>> On 15.10.2021 12:14, Ian Jackson wrote: >>>>>> Bertrand Marquis writes ("Re: [PATCH v6 2/3] xen/arm: Enable the existing x86 virtual PCI support for ARM."): >>>>>>>> On 15 Oct 2021, at 09:00, Jan Beulich <jbeulich@suse.com> wrote: >>>>>>>> The latter is fine to be put here (i.e. FTAOD I'm fine with it >>>>>>>> staying here). For the former I even question its original placement >>>>>>>> in asm-x86/pci.h: It's not generally correct as per the PCI spec, as >>>>>>>> the bus portion of the address can be anywhere from 1 to 8 bits. And >>>>>>>> in fact there is a reason why this macro was/is used in only a >>>>>>>> single place, but not e.g. in x86'es handling of physical MCFG. It >>>>>>>> is merely an implementation choice in vPCI that the entire segment 0 >>>>>>>> has a linear address range covering all 256 buses. Hence I think >>>>>>>> this wants to move to xen/vpci.h and then perhaps also be named >>>>>>>> VPCI_ECAM_BDF(). >>>>>>> >>>>>>> On previous version it was request to renamed this to ECAM and agreed >>>>>>> to put is here. Now you want me to rename it to VPCI and move it again. >>>>>>> I would like to have a confirmation that this is ok and the final move if possible. >>>>>>> >>>>>>> @Roger can you confirm this is what is wanted ? >>>>>> >>>>>> I think Roger is not available today I'm afraid. >>>>>> >>>>>> Bertrand, can you give me a link to the comment from Roger ? >>>>>> Assuming that it says what I think it will say: >>>>>> >>>>>> I think the best thing to do will be to leave the name as it was in >>>>>> the most recent version of your series. I don't think it makes sense >>>>>> to block this patch over a naming disagreement. And it would be best >>>>>> to minimise unnecessary churn. >>>>>> >>>>>> I would be happy to release-ack a name change (perhaps proposed bo Jan >>>>>> or Roger) supposing that that is the ultimate maintainer consensus. >>>>>> >>>>>> Jan, would that approach be OK with you ? >>>>> >>>>> Well, yes, if a subsequent name change is okay, then I could live with >>>>> that. I'd still find it odd to rename a function immediately after it >>>>> already got renamed. As expressed elsewhere, I suspect in his request >>>>> Roger did not pay attention to a use of the function in non-ECAM code. >>>> >>>> Using MMCFG_BDF was original requested by Julien, not myself I think: >>>> >>>> https://lore.kernel.org/xen-devel/a868e1e7-8400-45df-6eaa-69f1e2c99383@xen.org/ >>>> >>>> I'm slightly loss in so many messages. On x86 we subtract the MCFG >>>> start address from the passed one before getting the BDF, and then we >>>> add the startting bus address passed in the ACPI table. This is so far >>>> not need on Arm AFAICT because of the fixed nature of the selected >>>> virtual ECAM region. >>> >>> At the end my patch will add in xen/pci.h: >>> #define ECAM_BDF(addr) (((addr) & 0x0ffff000) >> 12) >> >> Since you still make this proposal, once again: I'm not going to >> accept such a macro in this header, whatever the name. Its prior >> placement was wrong as well. Only ... >> >>> #define ECAM_REG_OFFSET(addr) ((addr) & 0x00000fff) >> >> ... this one is fine to live here (and presumably it could gain uses >> elsewhere). > > So you would agree if they are both moved to vpci.h with a VPCI_ prefix ? I wouldn't object, but as said several times before I see no reason to also move and rename ECAM_REG_OFFSET(). If you moved it and if later we find a use for it outside of vPCI, we'd need to rename and move it again. Jan
Hi Jan, > On 15 Oct 2021, at 14:00, Jan Beulich <jbeulich@suse.com> wrote: > > On 15.10.2021 14:28, Bertrand Marquis wrote: >>> On 15 Oct 2021, at 13:18, Jan Beulich <jbeulich@suse.com> wrote: >>> On 15.10.2021 14:13, Bertrand Marquis wrote: >>>>> On 15 Oct 2021, at 12:35, Roger Pau Monné <roger.pau@citrix.com> wrote: >>>>> On Fri, Oct 15, 2021 at 12:18:59PM +0200, Jan Beulich wrote: >>>>>> On 15.10.2021 12:14, Ian Jackson wrote: >>>>>>> Bertrand Marquis writes ("Re: [PATCH v6 2/3] xen/arm: Enable the existing x86 virtual PCI support for ARM."): >>>>>>>>> On 15 Oct 2021, at 09:00, Jan Beulich <jbeulich@suse.com> wrote: >>>>>>>>> The latter is fine to be put here (i.e. FTAOD I'm fine with it >>>>>>>>> staying here). For the former I even question its original placement >>>>>>>>> in asm-x86/pci.h: It's not generally correct as per the PCI spec, as >>>>>>>>> the bus portion of the address can be anywhere from 1 to 8 bits. And >>>>>>>>> in fact there is a reason why this macro was/is used in only a >>>>>>>>> single place, but not e.g. in x86'es handling of physical MCFG. It >>>>>>>>> is merely an implementation choice in vPCI that the entire segment 0 >>>>>>>>> has a linear address range covering all 256 buses. Hence I think >>>>>>>>> this wants to move to xen/vpci.h and then perhaps also be named >>>>>>>>> VPCI_ECAM_BDF(). >>>>>>>> >>>>>>>> On previous version it was request to renamed this to ECAM and agreed >>>>>>>> to put is here. Now you want me to rename it to VPCI and move it again. >>>>>>>> I would like to have a confirmation that this is ok and the final move if possible. >>>>>>>> >>>>>>>> @Roger can you confirm this is what is wanted ? >>>>>>> >>>>>>> I think Roger is not available today I'm afraid. >>>>>>> >>>>>>> Bertrand, can you give me a link to the comment from Roger ? >>>>>>> Assuming that it says what I think it will say: >>>>>>> >>>>>>> I think the best thing to do will be to leave the name as it was in >>>>>>> the most recent version of your series. I don't think it makes sense >>>>>>> to block this patch over a naming disagreement. And it would be best >>>>>>> to minimise unnecessary churn. >>>>>>> >>>>>>> I would be happy to release-ack a name change (perhaps proposed bo Jan >>>>>>> or Roger) supposing that that is the ultimate maintainer consensus. >>>>>>> >>>>>>> Jan, would that approach be OK with you ? >>>>>> >>>>>> Well, yes, if a subsequent name change is okay, then I could live with >>>>>> that. I'd still find it odd to rename a function immediately after it >>>>>> already got renamed. As expressed elsewhere, I suspect in his request >>>>>> Roger did not pay attention to a use of the function in non-ECAM code. >>>>> >>>>> Using MMCFG_BDF was original requested by Julien, not myself I think: >>>>> >>>>> https://lore.kernel.org/xen-devel/a868e1e7-8400-45df-6eaa-69f1e2c99383@xen.org/ >>>>> >>>>> I'm slightly loss in so many messages. On x86 we subtract the MCFG >>>>> start address from the passed one before getting the BDF, and then we >>>>> add the startting bus address passed in the ACPI table. This is so far >>>>> not need on Arm AFAICT because of the fixed nature of the selected >>>>> virtual ECAM region. >>>> >>>> At the end my patch will add in xen/pci.h: >>>> #define ECAM_BDF(addr) (((addr) & 0x0ffff000) >> 12) >>> >>> Since you still make this proposal, once again: I'm not going to >>> accept such a macro in this header, whatever the name. Its prior >>> placement was wrong as well. Only ... >>> >>>> #define ECAM_REG_OFFSET(addr) ((addr) & 0x00000fff) >>> >>> ... this one is fine to live here (and presumably it could gain uses >>> elsewhere). >> >> So you would agree if they are both moved to vpci.h with a VPCI_ prefix ? > > I wouldn't object, but as said several times before I see no reason > to also move and rename ECAM_REG_OFFSET(). If you moved it and if > later we find a use for it outside of vPCI, we'd need to rename and > move it again. I will move BDF to vpci.h and had VPCI prefix and keep REG_OFFSET as it is and where it is then. Cheers Bertrand > > Jan >
On Fri, Oct 15, 2021 at 10:48:41AM +0000, Bertrand Marquis wrote: > Hi Jan, > > > On 15 Oct 2021, at 11:41, Jan Beulich <jbeulich@suse.com> wrote: > > > > On 15.10.2021 12:33, Bertrand Marquis wrote: > >>> On 15 Oct 2021, at 11:24, Jan Beulich <jbeulich@suse.com> wrote: > >>> On 15.10.2021 11:52, Bertrand Marquis wrote: > >>>>> On 15 Oct 2021, at 09:32, Roger Pau Monné <roger.pau@citrix.com> wrote: > >>>>> On Thu, Oct 14, 2021 at 03:49:50PM +0100, Bertrand Marquis wrote: > >>>>>> @@ -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 ? > >>> > >>> Well, without Xen doing the enumeration, some other entity would need > >>> to do so, including the reporting to Xen. Obviously without a Dom0 it > >>> would be ambiguous which domain to assign the device to; perhaps it > >>> should be the caller in this case? That would make that caller domain > >>> a pseudo-hwdom though, as far as PCI is concerned, which may not be > >>> desirable according to my (limited) understanding of dom0less. > >> > >> This is not really related to this patch but the plan is the following: > >> - enumeration would have to be done by the firmware or boot loader before > >> - xen will have some code to detect PCI devices > >> - dom0less can be used to assign PCI devices to guest > >> > >> Anyway does not change the fact that this must be called when domain is > >> not NULL and I will fix that. > > > > Since we now all seem to agree that the NULL would have been a problem, > > may I ask in how far any of this has actually been tested? > > With the whole serie currently on gitlab we have extensively tested passing > through PCI devices on Arm in several configuration (number of device, MSI, > MSI-X) and check that PCI was still functional on x86. > > With the patches pushed to Xen right now it was checked that: > - xen compiles properly on arm32, arm64 and x86 > - xen compiles properly with VPCI activated (using a patch) on arm32 and arm64 > - xen on x86 is functionnal (using basic test on QEMU) > - xen on arm64 is functionnal (with some extensive tests on different targets) I thinks it's unlikely, but since I haven't checked myself, could you see if the vpci user-space test harness (tools/tests/vpci) still builds and functions properly? Thanks, Roger.
> On 15 Oct 2021, at 14:47, Roger Pau Monné <roger.pau@citrix.com> wrote: > > On Fri, Oct 15, 2021 at 10:48:41AM +0000, Bertrand Marquis wrote: >> Hi Jan, >> >>> On 15 Oct 2021, at 11:41, Jan Beulich <jbeulich@suse.com> wrote: >>> >>> On 15.10.2021 12:33, Bertrand Marquis wrote: >>>>> On 15 Oct 2021, at 11:24, Jan Beulich <jbeulich@suse.com> wrote: >>>>> On 15.10.2021 11:52, Bertrand Marquis wrote: >>>>>>> On 15 Oct 2021, at 09:32, Roger Pau Monné <roger.pau@citrix.com> wrote: >>>>>>> On Thu, Oct 14, 2021 at 03:49:50PM +0100, Bertrand Marquis wrote: >>>>>>>> @@ -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 ? >>>>> >>>>> Well, without Xen doing the enumeration, some other entity would need >>>>> to do so, including the reporting to Xen. Obviously without a Dom0 it >>>>> would be ambiguous which domain to assign the device to; perhaps it >>>>> should be the caller in this case? That would make that caller domain >>>>> a pseudo-hwdom though, as far as PCI is concerned, which may not be >>>>> desirable according to my (limited) understanding of dom0less. >>>> >>>> This is not really related to this patch but the plan is the following: >>>> - enumeration would have to be done by the firmware or boot loader before >>>> - xen will have some code to detect PCI devices >>>> - dom0less can be used to assign PCI devices to guest >>>> >>>> Anyway does not change the fact that this must be called when domain is >>>> not NULL and I will fix that. >>> >>> Since we now all seem to agree that the NULL would have been a problem, >>> may I ask in how far any of this has actually been tested? >> >> With the whole serie currently on gitlab we have extensively tested passing >> through PCI devices on Arm in several configuration (number of device, MSI, >> MSI-X) and check that PCI was still functional on x86. >> >> With the patches pushed to Xen right now it was checked that: >> - xen compiles properly on arm32, arm64 and x86 >> - xen compiles properly with VPCI activated (using a patch) on arm32 and arm64 >> - xen on x86 is functionnal (using basic test on QEMU) >> - xen on arm64 is functionnal (with some extensive tests on different targets) > > I thinks it's unlikely, but since I haven't checked myself, could you > see if the vpci user-space test harness (tools/tests/vpci) still > builds and functions properly? Hi Roger, Bertrand is going to send the v7, however I’m keen to do this test but unfortunately I’ve never used it before and I don’t have an x86 environment, do you have any resource that I can read or some guidance? Cheers, Luca > > Thanks, Roger. >
On Fri, Oct 15, 2021 at 03:00:21PM +0100, Luca Fancellu wrote: > > > > On 15 Oct 2021, at 14:47, Roger Pau Monné <roger.pau@citrix.com> wrote: > > > > On Fri, Oct 15, 2021 at 10:48:41AM +0000, Bertrand Marquis wrote: > >> Hi Jan, > >> > >>> On 15 Oct 2021, at 11:41, Jan Beulich <jbeulich@suse.com> wrote: > >>> > >>> On 15.10.2021 12:33, Bertrand Marquis wrote: > >>>>> On 15 Oct 2021, at 11:24, Jan Beulich <jbeulich@suse.com> wrote: > >>>>> On 15.10.2021 11:52, Bertrand Marquis wrote: > >>>>>>> On 15 Oct 2021, at 09:32, Roger Pau Monné <roger.pau@citrix.com> wrote: > >>>>>>> On Thu, Oct 14, 2021 at 03:49:50PM +0100, Bertrand Marquis wrote: > >>>>>>>> @@ -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 ? > >>>>> > >>>>> Well, without Xen doing the enumeration, some other entity would need > >>>>> to do so, including the reporting to Xen. Obviously without a Dom0 it > >>>>> would be ambiguous which domain to assign the device to; perhaps it > >>>>> should be the caller in this case? That would make that caller domain > >>>>> a pseudo-hwdom though, as far as PCI is concerned, which may not be > >>>>> desirable according to my (limited) understanding of dom0less. > >>>> > >>>> This is not really related to this patch but the plan is the following: > >>>> - enumeration would have to be done by the firmware or boot loader before > >>>> - xen will have some code to detect PCI devices > >>>> - dom0less can be used to assign PCI devices to guest > >>>> > >>>> Anyway does not change the fact that this must be called when domain is > >>>> not NULL and I will fix that. > >>> > >>> Since we now all seem to agree that the NULL would have been a problem, > >>> may I ask in how far any of this has actually been tested? > >> > >> With the whole serie currently on gitlab we have extensively tested passing > >> through PCI devices on Arm in several configuration (number of device, MSI, > >> MSI-X) and check that PCI was still functional on x86. > >> > >> With the patches pushed to Xen right now it was checked that: > >> - xen compiles properly on arm32, arm64 and x86 > >> - xen compiles properly with VPCI activated (using a patch) on arm32 and arm64 > >> - xen on x86 is functionnal (using basic test on QEMU) > >> - xen on arm64 is functionnal (with some extensive tests on different targets) > > > > I thinks it's unlikely, but since I haven't checked myself, could you > > see if the vpci user-space test harness (tools/tests/vpci) still > > builds and functions properly? > > Hi Roger, > > Bertrand is going to send the v7, however I’m keen to do this test but unfortunately > I’ve never used it before and I don’t have an x86 environment, do you have any > resource that I can read or some guidance? Oh, you just need to build it (make) and then run it (make run). If it returns success everything is fine. Regards, Roger.
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); +} + +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 + ret = 0; if ( !pdev->domain ) { @@ -784,6 +797,9 @@ out: &PCI_SBDF(seg, bus, slot, func)); } } + else if ( pdev ) + pci_cleanup_msi(pdev); + return ret; } diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c index f8cd55e7c0..c5b025b88b 100644 --- a/xen/drivers/vpci/header.c +++ b/xen/drivers/vpci/header.c @@ -374,7 +374,8 @@ static void bar_write(const struct pci_dev *pdev, unsigned int reg, if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) ) gprintk(XENLOG_WARNING, "%pp: ignored BAR %lu write with memory decoding enabled\n", - &pdev->sbdf, bar - pdev->vpci->header.bars + hi); + &pdev->sbdf, + (unsigned long)(bar - pdev->vpci->header.bars + hi)); return; } diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c index c0853176d7..2bd67fc27a 100644 --- a/xen/drivers/vpci/vpci.c +++ b/xen/drivers/vpci/vpci.c @@ -507,7 +507,7 @@ int vpci_ecam_mmio_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int len, vpci_write(sbdf, reg, min(4u, len), data); if ( len == 8 ) - vpci_write(sbdf, reg + 4, 4, data >> 32); + vpci_write(sbdf, reg + 4, 4, (uint64_t)data >> 32); return 1; } diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index 14e575288f..9b3647587a 100644 --- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -263,6 +263,7 @@ static inline void arch_vcpu_block(struct vcpu *v) {} #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag) +/* vPCI is not available on Arm */ #define has_vpci(d) ({ (void)(d); false; }) #endif /* __ASM_DOMAIN_H__ */ diff --git a/xen/include/asm-x86/pci.h b/xen/include/asm-x86/pci.h index a0df5c1279..443f25347d 100644 --- a/xen/include/asm-x86/pci.h +++ b/xen/include/asm-x86/pci.h @@ -6,8 +6,6 @@ #define CF8_ADDR_HI(cf8) ( ((cf8) & 0x0f000000) >> 16) #define CF8_ENABLED(cf8) (!!((cf8) & 0x80000000)) -#define ECAM_BDF(addr) ( ((addr) & 0x0ffff000) >> 12) - #define IS_SNB_GFX(id) (id == 0x01068086 || id == 0x01168086 \ || id == 0x01268086 || id == 0x01028086 \ || id == 0x01128086 || id == 0x01228086 \ diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h index d46c61fca9..44be337dec 100644 --- a/xen/include/public/arch-arm.h +++ b/xen/include/public/arch-arm.h @@ -418,6 +418,13 @@ typedef uint64_t xen_callback_t; #define GUEST_GICV3_GICR0_BASE xen_mk_ullong(0x03020000) /* vCPU0..127 */ #define GUEST_GICV3_GICR0_SIZE xen_mk_ullong(0x01000000) +/* + * 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) + /* ACPI tables physical address */ #define GUEST_ACPI_BASE xen_mk_ullong(0x20000000) #define GUEST_ACPI_SIZE xen_mk_ullong(0x02000000) diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h index 70ac25345c..db18cb7639 100644 --- a/xen/include/xen/pci.h +++ b/xen/include/xen/pci.h @@ -40,6 +40,9 @@ #define PCI_SBDF3(s,b,df) \ ((pci_sbdf_t){ .sbdf = (((s) & 0xffff) << 16) | PCI_BDF2(b, df) }) +#define ECAM_BDF(addr) ( ((addr) & 0x0ffff000) >> 12) +#define ECAM_REG_OFFSET(addr) ((addr) & 0x00000fff) + typedef union { uint32_t sbdf; struct {