| Message ID | 370f4f87c148eaee5ac5ec69346828e6473f0f2d.1629366665.git.rahul.singh@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI devices passthrough on Arm | expand |
On 19.08.2021 14:02, Rahul Singh wrote: > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -767,6 +767,13 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, > else > iommu_enable_device(pdev); > > +#ifdef CONFIG_ARM > + ret = vpci_add_handlers(pdev); > + if ( ret ) { > + printk(XENLOG_ERR "setup of vPCI for failed: %d\n",ret); > + goto out; > + } > +#endif > pci_enable_acs(pdev); I'm afraid I can't see why this is to be Arm-specific. The present placement of the existing call to vpci_add_handlers() looks to be sub-optimal anyway - did you look into whether it could be moved to a place (potentially right here) fitting everyone? If you did, would mind justifying the Arm-specific code in a non-Arm-specific file in at least a post-commit-message remark? If it were to remain like this, please add a blank line after the #endif. > --- a/xen/drivers/vpci/Makefile > +++ b/xen/drivers/vpci/Makefile > @@ -1 +1,2 @@ > -obj-y += vpci.o header.o msi.o msix.o > +obj-y += vpci.o header.o > +obj-$(CONFIG_HAS_PCI_MSI) += msi.o msix.o I continue to consider this a wrong connection - HAS_PCI_MSI expresses (quoting text from patch 1 of this series) "code that implements MSI functionality to support MSI within XEN", while here we're talking of vPCI (i.e. guest support). I can accept that this is transiently the best you can do, but then please add a comment to this effect (if need be in multiple places), such that future readers or people wanting to further adjust this understand why it is the way it is. But perhaps you instead want to introduce a HAS_VPCI_MSI (or, less desirable because of possible ambiguity, HAS_VMSI) Kconfig option? > --- a/xen/drivers/vpci/header.c > +++ b/xen/drivers/vpci/header.c > @@ -96,8 +96,10 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd, > * FIXME: punching holes after the p2m has been set up might be racy for > * DomU usage, needs to be revisited. > */ > +#ifdef CONFIG_HAS_PCI_MSI > if ( map && !rom_only && vpci_make_msix_hole(pdev) ) > return; > +#endif (This would be another such place.) > --- a/xen/include/asm-arm/domain.h > +++ b/xen/include/asm-arm/domain.h > @@ -262,7 +262,10 @@ static inline void arch_vcpu_block(struct vcpu *v) {} > > #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag) > > -#define has_vpci(d) ({ (void)(d); false; }) > +/* For X86 VPCI is enabled and tested for PVH DOM0 only but > + * for ARM we enable support VPCI for guest domain also. > + */ Comment style (/* goes on its own line). > +#define has_vpci(d) ({ (void)(d); IS_ENABLED(CONFIG_HAS_VPCI); }) Personally I'd recommend to get away without using extensions whenever possible, i.e. use #define has_vpci(d) ((void)(d), IS_ENABLED(CONFIG_HAS_VPCI)) here. > --- a/xen/include/asm-arm/pci.h > +++ b/xen/include/asm-arm/pci.h > @@ -27,6 +27,14 @@ struct arch_pci_dev { > struct device dev; > }; > > +/* Arch-specific MSI data for vPCI. */ > +struct vpci_arch_msi { > +}; > + > +/* Arch-specific MSI-X entry data for vPCI. */ > +struct vpci_arch_msix_entry { > +}; But isn't it that you don't support vPCI's MSI in the first place? Perhaps the need for these would go away with CONFIG_HAS_VCPI_MSI? Jan
Hi, Jan! On 24.08.21 19:09, Jan Beulich wrote: > On 19.08.2021 14:02, Rahul Singh wrote: >> --- a/xen/drivers/passthrough/pci.c >> +++ b/xen/drivers/passthrough/pci.c >> @@ -767,6 +767,13 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, >> else >> iommu_enable_device(pdev); >> >> +#ifdef CONFIG_ARM >> + ret = vpci_add_handlers(pdev); >> + if ( ret ) { >> + printk(XENLOG_ERR "setup of vPCI for failed: %d\n",ret); >> + goto out; >> + } >> +#endif >> pci_enable_acs(pdev); > I'm afraid I can't see why this is to be Arm-specific. The present > placement of the existing call to vpci_add_handlers() looks to be > sub-optimal anyway - did you look into whether it could be moved > to a place (potentially right here) fitting everyone? If you did, > would mind justifying the Arm-specific code in a non-Arm-specific > file in at least a post-commit-message remark? > > If it were to remain like this, please add a blank line after the #endif. > >> --- a/xen/drivers/vpci/Makefile >> +++ b/xen/drivers/vpci/Makefile >> @@ -1 +1,2 @@ >> -obj-y += vpci.o header.o msi.o msix.o >> +obj-y += vpci.o header.o >> +obj-$(CONFIG_HAS_PCI_MSI) += msi.o msix.o > I continue to consider this a wrong connection - HAS_PCI_MSI expresses > (quoting text from patch 1 of this series) "code that implements MSI > functionality to support MSI within XEN", while here we're talking of > vPCI (i.e. guest support). I can accept that this is transiently the > best you can do, but then please add a comment to this effect (if > need be in multiple places), such that future readers or people > wanting to further adjust this understand why it is the way it is. > > But perhaps you instead want to introduce a HAS_VPCI_MSI (or, less > desirable because of possible ambiguity, HAS_VMSI) Kconfig option? Eventually we'll have the code like that: obj-y += vpci.o header.o msi.o msix.o obj-$(CONFIG_X86) += x86/ obj-$(CONFIG_ARM) += arm/ So, this is indeed a transitional change. Will you be ok with a comment about that then? > >> --- a/xen/drivers/vpci/header.c >> +++ b/xen/drivers/vpci/header.c >> @@ -96,8 +96,10 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd, >> * FIXME: punching holes after the p2m has been set up might be racy for >> * DomU usage, needs to be revisited. >> */ >> +#ifdef CONFIG_HAS_PCI_MSI >> if ( map && !rom_only && vpci_make_msix_hole(pdev) ) >> return; >> +#endif > (This would be another such place.) > >> --- a/xen/include/asm-arm/domain.h >> +++ b/xen/include/asm-arm/domain.h >> @@ -262,7 +262,10 @@ static inline void arch_vcpu_block(struct vcpu *v) {} >> >> #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag) >> >> -#define has_vpci(d) ({ (void)(d); false; }) >> +/* For X86 VPCI is enabled and tested for PVH DOM0 only but >> + * for ARM we enable support VPCI for guest domain also. >> + */ > Comment style (/* goes on its own line). > >> +#define has_vpci(d) ({ (void)(d); IS_ENABLED(CONFIG_HAS_VPCI); }) > Personally I'd recommend to get away without using extensions whenever > possible, i.e. use > > #define has_vpci(d) ((void)(d), IS_ENABLED(CONFIG_HAS_VPCI)) > > here. > >> --- a/xen/include/asm-arm/pci.h >> +++ b/xen/include/asm-arm/pci.h >> @@ -27,6 +27,14 @@ struct arch_pci_dev { >> struct device dev; >> }; >> >> +/* Arch-specific MSI data for vPCI. */ >> +struct vpci_arch_msi { >> +}; >> + >> +/* Arch-specific MSI-X entry data for vPCI. */ >> +struct vpci_arch_msix_entry { >> +}; > But isn't it that you don't support vPCI's MSI in the first place? > Perhaps the need for these would go away with CONFIG_HAS_VCPI_MSI? > > Jan > >
On 25.08.2021 07:44, Oleksandr Andrushchenko wrote: > Hi, Jan! > > On 24.08.21 19:09, Jan Beulich wrote: >> On 19.08.2021 14:02, Rahul Singh wrote: >>> --- a/xen/drivers/passthrough/pci.c >>> +++ b/xen/drivers/passthrough/pci.c >>> @@ -767,6 +767,13 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, >>> else >>> iommu_enable_device(pdev); >>> >>> +#ifdef CONFIG_ARM >>> + ret = vpci_add_handlers(pdev); >>> + if ( ret ) { >>> + printk(XENLOG_ERR "setup of vPCI for failed: %d\n",ret); >>> + goto out; >>> + } >>> +#endif >>> pci_enable_acs(pdev); >> I'm afraid I can't see why this is to be Arm-specific. The present >> placement of the existing call to vpci_add_handlers() looks to be >> sub-optimal anyway - did you look into whether it could be moved >> to a place (potentially right here) fitting everyone? If you did, >> would mind justifying the Arm-specific code in a non-Arm-specific >> file in at least a post-commit-message remark? >> >> If it were to remain like this, please add a blank line after the #endif. >> >>> --- a/xen/drivers/vpci/Makefile >>> +++ b/xen/drivers/vpci/Makefile >>> @@ -1 +1,2 @@ >>> -obj-y += vpci.o header.o msi.o msix.o >>> +obj-y += vpci.o header.o >>> +obj-$(CONFIG_HAS_PCI_MSI) += msi.o msix.o >> I continue to consider this a wrong connection - HAS_PCI_MSI expresses >> (quoting text from patch 1 of this series) "code that implements MSI >> functionality to support MSI within XEN", while here we're talking of >> vPCI (i.e. guest support). I can accept that this is transiently the >> best you can do, but then please add a comment to this effect (if >> need be in multiple places), such that future readers or people >> wanting to further adjust this understand why it is the way it is. >> >> But perhaps you instead want to introduce a HAS_VPCI_MSI (or, less >> desirable because of possible ambiguity, HAS_VMSI) Kconfig option? > > Eventually we'll have the code like that: > > obj-y += vpci.o header.o msi.o msix.o > obj-$(CONFIG_X86) += x86/ > obj-$(CONFIG_ARM) += arm/ > > So, this is indeed a transitional change. Will you be ok with a comment > about that then? Well, yes, as said - if this is a transitional state, then a comment will do. I was suggesting the further Kconfig option merely because all I've been hearing so far was that MSI works entirely differently on Arm, and hence the MSI code we have is not going to be used there at all. If that was a correct understanding of mine (including its extending to vPCI's MSI code), then adding just a comment would continue to be misleading imo. Jan
Hi Rahul, On 19/08/2021 13:02, Rahul Singh wrote: > The existing VPCI support available for X86 is adapted for Arm. > When the device is added to XEN via the hyper call > “PHYSDEVOP_pci_device_add”, VPCI handler for the config space > access is added to the Xen to emulate the PCI devices config space. > > A MMIO trap handler for the PCI ECAM space is registered in XEN > so that when guest is trying to access the PCI config space,XEN > will trap the access and emulate read/write using the VPCI and > not the real PCI hardware. > > Signed-off-by: Rahul Singh <rahul.singh@arm.com> > --- > xen/arch/arm/Makefile | 1 + > xen/arch/arm/domain.c | 4 ++ > xen/arch/arm/vpci.c | 96 +++++++++++++++++++++++++++++++++++ > xen/arch/arm/vpci.h | 37 ++++++++++++++ > xen/drivers/passthrough/pci.c | 7 +++ > xen/drivers/vpci/Makefile | 3 +- > xen/drivers/vpci/header.c | 2 + > xen/include/asm-arm/domain.h | 5 +- > xen/include/asm-arm/pci.h | 8 +++ > xen/include/public/arch-arm.h | 4 ++ > 10 files changed, 165 insertions(+), 2 deletions(-) > create mode 100644 xen/arch/arm/vpci.c > create mode 100644 xen/arch/arm/vpci.h > > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile > index 0e14a5e5c8..7cdce684a4 100644 > --- a/xen/arch/arm/Makefile > +++ b/xen/arch/arm/Makefile > @@ -7,6 +7,7 @@ obj-y += platforms/ > endif > obj-$(CONFIG_TEE) += tee/ > obj-$(CONFIG_HAS_PCI) += pci/ > +obj-$(CONFIG_HAS_VPCI) += vpci.o > > obj-$(CONFIG_HAS_ALTERNATIVE) += alternative.o > obj-y += bootfdt.init.o > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > index 19c756ac3d..d99c653626 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -40,6 +40,7 @@ > #include <asm/vtimer.h> > > #include "vuart.h" > +#include "vpci.h" Please order the includes alphabetically. So this one should go before "vuart.h". > > DEFINE_PER_CPU(struct vcpu *, curr_vcpu); > > @@ -767,6 +768,9 @@ int arch_domain_create(struct domain *d, > if ( is_hardware_domain(d) && (rc = domain_vuart_init(d)) ) > goto fail; > > + if ( (rc = domain_vpci_init(d)) != 0 ) > + goto fail; > + > return 0; > > fail: > diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c > new file mode 100644 > index 0000000000..da8b1ca13c > --- /dev/null > +++ b/xen/arch/arm/vpci.c > @@ -0,0 +1,96 @@ > +/* > + * xen/arch/arm/vpci.c > + * Copyright (c) 2021 Arm Ltd. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > +#include <xen/sched.h> NIT: Please add a newline between generic and arch specific includes. > +#include <asm/mmio.h> > + > +/* Do some sanity checks. */ > +static bool vpci_mmio_access_allowed(unsigned int reg, unsigned int len) > +{ > + /* Check access size. */ > + if ( len != 1 && len != 2 && len != 4 && len != 8 ) > + return false; You will allow all the possible value of len (this is coming from the HW). So I feels this is a bit too much to check for every I/O to the vPCI. If you really want to keep the check then you can simply check that len is < 8 because the two callers compute it with (1 << S). So there is no way to set it 3, 5, 6 and 7. > + > + /* Check that access is size aligned. */ > + if ( (reg & (len - 1)) ) > + return false; > + > + return true; > +} > + > +static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, > + register_t *r, void *p) > +{ > + unsigned int reg; > + pci_sbdf_t sbdf; > + uint32_t data = 0; > + unsigned int size = 1U << info->dabt.size; > + > + sbdf.sbdf = (((info->gpa) & 0x0ffff000) >> 12); > + reg = (((info->gpa) & 0x00000ffc) | (info->gpa & 3)); This logic is the same as below in vpci_mmio_write(). So I think you want to provide an helper because this is not trivial to read. Also, for the first line, I think you can re-use MMCFG_BDF() from the x86 code. For the second line, I would define the value so it is clearer to understand that they mean (although & 3 is fine to me) . > + > + if ( !vpci_mmio_access_allowed(reg, size) ) > + return 1; So, you will a guest will read 0 if the access is unaligned. This seems an odd behavior given this is not an allowed access. AFAIU, the HW would likely trow a data abort because you can't do unalign access on uncachable memory. So I think we should return 0 here to let the MMIO handler inject a data abort. > + > + data = vpci_read(sbdf, reg, size); So in vpci_mmio_access_allowed(), you will allow a guest to read a 64-bit value. But... vpci_read() will return a 32-bit value. Looking at the x86 code, they have a second call to vpci_read() to handle the top 32-bit. Any reason why this was not implemented on Arm? If we need to implement it then I think this should be implement in vpci_read() to avoid duplication between x86 and arm. > + > + memcpy(r, &data, size); From my understanding, any unused bit should be 0. So this should be: *r = data; > + > + return 1; > +} > + > +static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info, > + register_t r, void *p) My remarks on vpci_mmio_read() applies here too. > +{ > + unsigned int reg; > + pci_sbdf_t sbdf; > + uint32_t data = r; > + unsigned int size = 1U << info->dabt.size; > + > + sbdf.sbdf = (((info->gpa) & 0x0ffff000) >> 12); > + reg = (((info->gpa) & 0x00000ffc) | (info->gpa & 3)); > + > + if ( !vpci_mmio_access_allowed(reg, size) ) > + return 1; > + > + vpci_write(sbdf, reg, size, data); > + > + return 1; > +} > + > +static const struct mmio_handler_ops vpci_mmio_handler = { > + .read = vpci_mmio_read, > + .write = vpci_mmio_write, > +}; > + > +int domain_vpci_init(struct domain *d) > +{ > + if ( !has_vpci(d) ) > + return 0; > + > + register_mmio_handler(d, &vpci_mmio_handler, > + GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL); > + > + return 0; > +} > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * indent-tabs-mode: nil > + * End: > + */ > + > diff --git a/xen/arch/arm/vpci.h b/xen/arch/arm/vpci.h > new file mode 100644 > index 0000000000..8a093bb705 > --- /dev/null > +++ b/xen/arch/arm/vpci.h > @@ -0,0 +1,37 @@ > +/* > + * xen/arch/arm/vpci.h > + * Copyright (c) 2021 Arm Ltd. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#ifndef __ARCH_ARM_VPCI_H__ > +#define __ARCH_ARM_VPCI_H__ > + > +#ifdef CONFIG_HAS_VPCI > +int domain_vpci_init(struct domain *d); > +#else > +static inline int domain_vpci_init(struct domain *d) > +{ > + return 0; > +} > +#endif > + > +#endif /* __ARCH_ARM_VPCI_H__ */ > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * indent-tabs-mode: nil > + * End: > + */ > diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c > index c23c8cb06b..56e261e9bd 100644 > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -767,6 +767,13 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, > else > iommu_enable_device(pdev); > > +#ifdef CONFIG_ARM > + ret = vpci_add_handlers(pdev); > + if ( ret ) { > + printk(XENLOG_ERR "setup of vPCI for failed: %d\n",ret); > + goto out; > + } > +#endif > pci_enable_acs(pdev); > > out: > diff --git a/xen/drivers/vpci/Makefile b/xen/drivers/vpci/Makefile > index 55d1bdfda0..1a1413b93e 100644 > --- a/xen/drivers/vpci/Makefile > +++ b/xen/drivers/vpci/Makefile > @@ -1 +1,2 @@ > -obj-y += vpci.o header.o msi.o msix.o > +obj-y += vpci.o header.o > +obj-$(CONFIG_HAS_PCI_MSI) += msi.o msix.o > diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c > index ba9a036202..f8cd55e7c0 100644 > --- a/xen/drivers/vpci/header.c > +++ b/xen/drivers/vpci/header.c > @@ -96,8 +96,10 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd, > * FIXME: punching holes after the p2m has been set up might be racy for > * DomU usage, needs to be revisited. > */ > +#ifdef CONFIG_HAS_PCI_MSI > if ( map && !rom_only && vpci_make_msix_hole(pdev) ) > return; > +#endif > > for ( i = 0; i < ARRAY_SIZE(header->bars); i++ ) > { > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h > index c9277b5c6d..d742b94bd6 100644 > --- a/xen/include/asm-arm/domain.h > +++ b/xen/include/asm-arm/domain.h > @@ -262,7 +262,10 @@ static inline void arch_vcpu_block(struct vcpu *v) {} > > #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag) > > -#define has_vpci(d) ({ (void)(d); false; }) > +/* For X86 VPCI is enabled and tested for PVH DOM0 only but > + * for ARM we enable support VPCI for guest domain also. > + */ > +#define has_vpci(d) ({ (void)(d); IS_ENABLED(CONFIG_HAS_VPCI); }) > > #endif /* __ASM_DOMAIN_H__ */ > > diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h > index 756f8637ab..c58152de80 100644 > --- a/xen/include/asm-arm/pci.h > +++ b/xen/include/asm-arm/pci.h > @@ -27,6 +27,14 @@ struct arch_pci_dev { > struct device dev; > }; > > +/* Arch-specific MSI data for vPCI. */ > +struct vpci_arch_msi { > +}; > + > +/* Arch-specific MSI-X entry data for vPCI. */ > +struct vpci_arch_msix_entry { > +}; > + > /* > * struct to hold the mappings of a config space window. This > * is expected to be used as sysdata for PCI controllers that > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h > index 64a2ca30da..0a9749e768 100644 > --- a/xen/include/public/arch-arm.h > +++ b/xen/include/public/arch-arm.h > @@ -422,6 +422,10 @@ typedef uint64_t xen_callback_t; > #define GUEST_PL011_BASE xen_mk_ullong(0x22000000) > #define GUEST_PL011_SIZE xen_mk_ullong(0x00001000) > > +/* VPCI ECAM mappings */ > +#define GUEST_VPCI_ECAM_BASE xen_mk_ullong(0x10000000) All the values for the memory layout has been defined in ascending order. So please add the vCPI at the correct place. If I am not mistaken, dhis should be before the GUEST_ACPI_*. > +#define GUEST_VPCI_ECAM_SIZE xen_mk_ullong(0x10000000) Please document how to decide the size. This is important for the future if we need to change the size. Cheers,
On Thu, 19 Aug 2021, Rahul Singh wrote: > The existing VPCI support available for X86 is adapted for Arm. > When the device is added to XEN via the hyper call > “PHYSDEVOP_pci_device_add”, VPCI handler for the config space > access is added to the Xen to emulate the PCI devices config space. This is done just for device discovery, right? Although it is currently not implemented (and I am not asking to implement it now, I am only trying to understand the architecture), it would be possible to discover all PCI devices just by walking down the PCI hierarchy by ourselves in Xen (no Dom0 interactions) given that we have an ECAM driver. I take that would be the way to implement PCI support for Dom0less? > A MMIO trap handler for the PCI ECAM space is registered in XEN > so that when guest is trying to access the PCI config space,XEN > will trap the access and emulate read/write using the VPCI and > not the real PCI hardware. > > Signed-off-by: Rahul Singh <rahul.singh@arm.com> > --- > xen/arch/arm/Makefile | 1 + > xen/arch/arm/domain.c | 4 ++ > xen/arch/arm/vpci.c | 96 +++++++++++++++++++++++++++++++++++ > xen/arch/arm/vpci.h | 37 ++++++++++++++ > xen/drivers/passthrough/pci.c | 7 +++ > xen/drivers/vpci/Makefile | 3 +- > xen/drivers/vpci/header.c | 2 + > xen/include/asm-arm/domain.h | 5 +- > xen/include/asm-arm/pci.h | 8 +++ > xen/include/public/arch-arm.h | 4 ++ > 10 files changed, 165 insertions(+), 2 deletions(-) > create mode 100644 xen/arch/arm/vpci.c > create mode 100644 xen/arch/arm/vpci.h > > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile > index 0e14a5e5c8..7cdce684a4 100644 > --- a/xen/arch/arm/Makefile > +++ b/xen/arch/arm/Makefile > @@ -7,6 +7,7 @@ obj-y += platforms/ > endif > obj-$(CONFIG_TEE) += tee/ > obj-$(CONFIG_HAS_PCI) += pci/ > +obj-$(CONFIG_HAS_VPCI) += vpci.o > > obj-$(CONFIG_HAS_ALTERNATIVE) += alternative.o > obj-y += bootfdt.init.o > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > index 19c756ac3d..d99c653626 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -40,6 +40,7 @@ > #include <asm/vtimer.h> > > #include "vuart.h" > +#include "vpci.h" > > DEFINE_PER_CPU(struct vcpu *, curr_vcpu); > > @@ -767,6 +768,9 @@ int arch_domain_create(struct domain *d, > if ( is_hardware_domain(d) && (rc = domain_vuart_init(d)) ) > goto fail; > > + if ( (rc = domain_vpci_init(d)) != 0 ) > + goto fail; > + > return 0; > > fail: > diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c > new file mode 100644 > index 0000000000..da8b1ca13c > --- /dev/null > +++ b/xen/arch/arm/vpci.c > @@ -0,0 +1,96 @@ > +/* > + * xen/arch/arm/vpci.c > + * Copyright (c) 2021 Arm Ltd. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > +#include <xen/sched.h> > +#include <asm/mmio.h> > + > +/* Do some sanity checks. */ > +static bool vpci_mmio_access_allowed(unsigned int reg, unsigned int len) > +{ > + /* Check access size. */ > + if ( len != 1 && len != 2 && len != 4 && len != 8 ) > + return false; > + > + /* Check that access is size aligned. */ > + if ( (reg & (len - 1)) ) > + return false; > + > + return true; > +} > + > +static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, > + register_t *r, void *p) > +{ > + unsigned int reg; > + pci_sbdf_t sbdf; > + uint32_t data = 0; > + unsigned int size = 1U << info->dabt.size; > + > + sbdf.sbdf = (((info->gpa) & 0x0ffff000) >> 12); > + reg = (((info->gpa) & 0x00000ffc) | (info->gpa & 3)); > + > + if ( !vpci_mmio_access_allowed(reg, size) ) > + return 1; > + > + data = vpci_read(sbdf, reg, size); > + > + memcpy(r, &data, size); > + > + return 1; > +} > + > +static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info, > + register_t r, void *p) > +{ > + unsigned int reg; > + pci_sbdf_t sbdf; > + uint32_t data = r; > + unsigned int size = 1U << info->dabt.size; > + > + sbdf.sbdf = (((info->gpa) & 0x0ffff000) >> 12); > + reg = (((info->gpa) & 0x00000ffc) | (info->gpa & 3)); > + > + if ( !vpci_mmio_access_allowed(reg, size) ) > + return 1; > + > + vpci_write(sbdf, reg, size, data); > + > + return 1; > +} > + > +static const struct mmio_handler_ops vpci_mmio_handler = { > + .read = vpci_mmio_read, > + .write = vpci_mmio_write, > +}; > + > +int domain_vpci_init(struct domain *d) > +{ > + if ( !has_vpci(d) ) > + return 0; > + > + register_mmio_handler(d, &vpci_mmio_handler, > + GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL); > + > + return 0; > +} > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * indent-tabs-mode: nil > + * End: > + */ > + > diff --git a/xen/arch/arm/vpci.h b/xen/arch/arm/vpci.h > new file mode 100644 > index 0000000000..8a093bb705 > --- /dev/null > +++ b/xen/arch/arm/vpci.h > @@ -0,0 +1,37 @@ > +/* > + * xen/arch/arm/vpci.h > + * Copyright (c) 2021 Arm Ltd. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#ifndef __ARCH_ARM_VPCI_H__ > +#define __ARCH_ARM_VPCI_H__ > + > +#ifdef CONFIG_HAS_VPCI > +int domain_vpci_init(struct domain *d); > +#else > +static inline int domain_vpci_init(struct domain *d) > +{ > + return 0; > +} > +#endif > + > +#endif /* __ARCH_ARM_VPCI_H__ */ > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * indent-tabs-mode: nil > + * End: > + */ > diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c > index c23c8cb06b..56e261e9bd 100644 > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -767,6 +767,13 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, > else > iommu_enable_device(pdev); > > +#ifdef CONFIG_ARM > + ret = vpci_add_handlers(pdev); > + if ( ret ) { > + printk(XENLOG_ERR "setup of vPCI for failed: %d\n",ret); > + goto out; > + } > +#endif > pci_enable_acs(pdev); > > out: > diff --git a/xen/drivers/vpci/Makefile b/xen/drivers/vpci/Makefile > index 55d1bdfda0..1a1413b93e 100644 > --- a/xen/drivers/vpci/Makefile > +++ b/xen/drivers/vpci/Makefile > @@ -1 +1,2 @@ > -obj-y += vpci.o header.o msi.o msix.o > +obj-y += vpci.o header.o > +obj-$(CONFIG_HAS_PCI_MSI) += msi.o msix.o > diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c > index ba9a036202..f8cd55e7c0 100644 > --- a/xen/drivers/vpci/header.c > +++ b/xen/drivers/vpci/header.c > @@ -96,8 +96,10 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd, > * FIXME: punching holes after the p2m has been set up might be racy for > * DomU usage, needs to be revisited. > */ > +#ifdef CONFIG_HAS_PCI_MSI > if ( map && !rom_only && vpci_make_msix_hole(pdev) ) > return; > +#endif > > for ( i = 0; i < ARRAY_SIZE(header->bars); i++ ) > { > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h > index c9277b5c6d..d742b94bd6 100644 > --- a/xen/include/asm-arm/domain.h > +++ b/xen/include/asm-arm/domain.h > @@ -262,7 +262,10 @@ static inline void arch_vcpu_block(struct vcpu *v) {} > > #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag) > > -#define has_vpci(d) ({ (void)(d); false; }) > +/* For X86 VPCI is enabled and tested for PVH DOM0 only but > + * for ARM we enable support VPCI for guest domain also. > + */ > +#define has_vpci(d) ({ (void)(d); IS_ENABLED(CONFIG_HAS_VPCI); }) > > #endif /* __ASM_DOMAIN_H__ */ > > diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h > index 756f8637ab..c58152de80 100644 > --- a/xen/include/asm-arm/pci.h > +++ b/xen/include/asm-arm/pci.h > @@ -27,6 +27,14 @@ struct arch_pci_dev { > struct device dev; > }; > > +/* Arch-specific MSI data for vPCI. */ > +struct vpci_arch_msi { > +}; > + > +/* Arch-specific MSI-X entry data for vPCI. */ > +struct vpci_arch_msix_entry { > +}; > + > /* > * struct to hold the mappings of a config space window. This > * is expected to be used as sysdata for PCI controllers that > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h > index 64a2ca30da..0a9749e768 100644 > --- a/xen/include/public/arch-arm.h > +++ b/xen/include/public/arch-arm.h > @@ -422,6 +422,10 @@ typedef uint64_t xen_callback_t; > #define GUEST_PL011_BASE xen_mk_ullong(0x22000000) > #define GUEST_PL011_SIZE xen_mk_ullong(0x00001000) > > +/* VPCI ECAM mappings */ > +#define GUEST_VPCI_ECAM_BASE xen_mk_ullong(0x10000000) > +#define GUEST_VPCI_ECAM_SIZE xen_mk_ullong(0x10000000) Isn't 256MB a bit too small? Usually PCI comes with two ranges: - a small range below 4GB like this one - an additional large range above 4GB, around 1GB or larger We are missing the second larger range?
Hi Julien, > On 9 Sep 2021, at 2:50 pm, Julien Grall <julien@xen.org> wrote: > > Hi Rahul, > > On 19/08/2021 13:02, Rahul Singh wrote: >> The existing VPCI support available for X86 is adapted for Arm. >> When the device is added to XEN via the hyper call >> “PHYSDEVOP_pci_device_add”, VPCI handler for the config space >> access is added to the Xen to emulate the PCI devices config space. >> A MMIO trap handler for the PCI ECAM space is registered in XEN >> so that when guest is trying to access the PCI config space,XEN >> will trap the access and emulate read/write using the VPCI and >> not the real PCI hardware. >> Signed-off-by: Rahul Singh <rahul.singh@arm.com> >> --- >> xen/arch/arm/Makefile | 1 + >> xen/arch/arm/domain.c | 4 ++ >> xen/arch/arm/vpci.c | 96 +++++++++++++++++++++++++++++++++++ >> xen/arch/arm/vpci.h | 37 ++++++++++++++ >> xen/drivers/passthrough/pci.c | 7 +++ >> xen/drivers/vpci/Makefile | 3 +- >> xen/drivers/vpci/header.c | 2 + >> xen/include/asm-arm/domain.h | 5 +- >> xen/include/asm-arm/pci.h | 8 +++ >> xen/include/public/arch-arm.h | 4 ++ >> 10 files changed, 165 insertions(+), 2 deletions(-) >> create mode 100644 xen/arch/arm/vpci.c >> create mode 100644 xen/arch/arm/vpci.h >> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile >> index 0e14a5e5c8..7cdce684a4 100644 >> --- a/xen/arch/arm/Makefile >> +++ b/xen/arch/arm/Makefile >> @@ -7,6 +7,7 @@ obj-y += platforms/ >> endif >> obj-$(CONFIG_TEE) += tee/ >> obj-$(CONFIG_HAS_PCI) += pci/ >> +obj-$(CONFIG_HAS_VPCI) += vpci.o >> obj-$(CONFIG_HAS_ALTERNATIVE) += alternative.o >> obj-y += bootfdt.init.o >> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c >> index 19c756ac3d..d99c653626 100644 >> --- a/xen/arch/arm/domain.c >> +++ b/xen/arch/arm/domain.c >> @@ -40,6 +40,7 @@ >> #include <asm/vtimer.h> >> #include "vuart.h" >> +#include "vpci.h" > > Please order the includes alphabetically. So this one should go before "vuart.h". > >> DEFINE_PER_CPU(struct vcpu *, curr_vcpu); >> @@ -767,6 +768,9 @@ int arch_domain_create(struct domain *d, >> if ( is_hardware_domain(d) && (rc = domain_vuart_init(d)) ) >> goto fail; >> + if ( (rc = domain_vpci_init(d)) != 0 ) >> + goto fail; >> + >> return 0; >> fail: >> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c >> new file mode 100644 >> index 0000000000..da8b1ca13c >> --- /dev/null >> +++ b/xen/arch/arm/vpci.c >> @@ -0,0 +1,96 @@ >> +/* >> + * xen/arch/arm/vpci.c >> + * Copyright (c) 2021 Arm Ltd. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + */ >> +#include <xen/sched.h> > > NIT: Please add a newline between generic and arch specific includes. Ack. > >> +#include <asm/mmio.h> >> + >> +/* Do some sanity checks. */ >> +static bool vpci_mmio_access_allowed(unsigned int reg, unsigned int len) >> +{ >> + /* Check access size. */ >> + if ( len != 1 && len != 2 && len != 4 && len != 8 ) >> + return false; > You will allow all the possible value of len (this is coming from the HW). So I feels this is a bit too much to check for every I/O to the vPCI. > > If you really want to keep the check then you can simply check that len is < 8 because the two callers compute it with (1 << S). So there is no way to set it 3, 5, 6 and 7. I will remove the check for 1,2,4,8 and will have check ( len < 8). > >> + >> + /* Check that access is size aligned. */ >> + if ( (reg & (len - 1)) ) >> + return false; >> + >> + return true; >> +} >> + >> +static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, >> + register_t *r, void *p) >> +{ >> + unsigned int reg; >> + pci_sbdf_t sbdf; >> + uint32_t data = 0; >> + unsigned int size = 1U << info->dabt.size; >> + >> + sbdf.sbdf = (((info->gpa) & 0x0ffff000) >> 12); >> + reg = (((info->gpa) & 0x00000ffc) | (info->gpa & 3)); > > This logic is the same as below in vpci_mmio_write(). So I think you want to provide an helper because this is not trivial to read. Ok. > > Also, for the first line, I think you can re-use MMCFG_BDF() from the x86 code. For the second line, I would define the value so it is clearer to understand that they mean (although & 3 is fine to me) . Ok. > >> + >> + if ( !vpci_mmio_access_allowed(reg, size) ) >> + return 1; > So, you will a guest will read 0 if the access is unaligned. This seems an odd behavior given this is not an allowed access. AFAIU, the HW would likely trow a data abort because you can't do unalign access on uncachable memory. So I think we should return 0 here to let the MMIO handler inject a data abort. Ack. > >> + >> + data = vpci_read(sbdf, reg, size); > > So in vpci_mmio_access_allowed(), you will allow a guest to read a 64-bit value. But... vpci_read() will return a 32-bit value. > > Looking at the x86 code, they have a second call to vpci_read() to handle the top 32-bit. Any reason why this was not implemented on Arm? I missed that I will fix this in next version. > > If we need to implement it then I think this should be implement in vpci_read() to avoid duplication between x86 and arm. I think it will be good to make vpci_read() & vpci_write() simple not to add extra logic based on 64 bit value. I will add second call for vpci_read() and vpci_write() for ARM. > >> + >> + memcpy(r, &data, size); > > From my understanding, any unused bit should be 0. So this should be: > > *r = data; Ack. > >> + >> + return 1; >> +} >> + >> +static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info, >> + register_t r, void *p) > > My remarks on vpci_mmio_read() applies here too. > >> +{ >> + unsigned int reg; >> + pci_sbdf_t sbdf; >> + uint32_t data = r; >> + unsigned int size = 1U << info->dabt.size; >> + >> + sbdf.sbdf = (((info->gpa) & 0x0ffff000) >> 12); >> + reg = (((info->gpa) & 0x00000ffc) | (info->gpa & 3)); >> + >> + if ( !vpci_mmio_access_allowed(reg, size) ) >> + return 1; >> + >> + vpci_write(sbdf, reg, size, data); >> + >> + return 1; >> +} >> + >> +static const struct mmio_handler_ops vpci_mmio_handler = { >> + .read = vpci_mmio_read, >> + .write = vpci_mmio_write, >> +}; >> + >> +int domain_vpci_init(struct domain *d) >> +{ >> + if ( !has_vpci(d) ) >> + return 0; >> + >> + register_mmio_handler(d, &vpci_mmio_handler, >> + GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL); >> + >> + return 0; >> +} >> + >> +/* >> + * Local variables: >> + * mode: C >> + * c-file-style: "BSD" >> + * c-basic-offset: 4 >> + * indent-tabs-mode: nil >> + * End: >> + */ >> + >> diff --git a/xen/arch/arm/vpci.h b/xen/arch/arm/vpci.h >> new file mode 100644 >> index 0000000000..8a093bb705 >> --- /dev/null >> +++ b/xen/arch/arm/vpci.h >> @@ -0,0 +1,37 @@ >> +/* >> + * xen/arch/arm/vpci.h >> + * Copyright (c) 2021 Arm Ltd. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + */ >> + >> +#ifndef __ARCH_ARM_VPCI_H__ >> +#define __ARCH_ARM_VPCI_H__ >> + >> +#ifdef CONFIG_HAS_VPCI >> +int domain_vpci_init(struct domain *d); >> +#else >> +static inline int domain_vpci_init(struct domain *d) >> +{ >> + return 0; >> +} >> +#endif >> + >> +#endif /* __ARCH_ARM_VPCI_H__ */ >> + >> +/* >> + * Local variables: >> + * mode: C >> + * c-file-style: "BSD" >> + * c-basic-offset: 4 >> + * indent-tabs-mode: nil >> + * End: >> + */ >> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c >> index c23c8cb06b..56e261e9bd 100644 >> --- a/xen/drivers/passthrough/pci.c >> +++ b/xen/drivers/passthrough/pci.c >> @@ -767,6 +767,13 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, >> else >> iommu_enable_device(pdev); >> +#ifdef CONFIG_ARM >> + ret = vpci_add_handlers(pdev); >> + if ( ret ) { >> + printk(XENLOG_ERR "setup of vPCI for failed: %d\n",ret); >> + goto out; >> + } >> +#endif >> pci_enable_acs(pdev); >> out: >> diff --git a/xen/drivers/vpci/Makefile b/xen/drivers/vpci/Makefile >> index 55d1bdfda0..1a1413b93e 100644 >> --- a/xen/drivers/vpci/Makefile >> +++ b/xen/drivers/vpci/Makefile >> @@ -1 +1,2 @@ >> -obj-y += vpci.o header.o msi.o msix.o >> +obj-y += vpci.o header.o >> +obj-$(CONFIG_HAS_PCI_MSI) += msi.o msix.o >> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c >> index ba9a036202..f8cd55e7c0 100644 >> --- a/xen/drivers/vpci/header.c >> +++ b/xen/drivers/vpci/header.c >> @@ -96,8 +96,10 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd, >> * FIXME: punching holes after the p2m has been set up might be racy for >> * DomU usage, needs to be revisited. >> */ >> +#ifdef CONFIG_HAS_PCI_MSI >> if ( map && !rom_only && vpci_make_msix_hole(pdev) ) >> return; >> +#endif >> for ( i = 0; i < ARRAY_SIZE(header->bars); i++ ) >> { >> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h >> index c9277b5c6d..d742b94bd6 100644 >> --- a/xen/include/asm-arm/domain.h >> +++ b/xen/include/asm-arm/domain.h >> @@ -262,7 +262,10 @@ static inline void arch_vcpu_block(struct vcpu *v) {} >> #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag) >> -#define has_vpci(d) ({ (void)(d); false; }) >> +/* For X86 VPCI is enabled and tested for PVH DOM0 only but >> + * for ARM we enable support VPCI for guest domain also. >> + */ >> +#define has_vpci(d) ({ (void)(d); IS_ENABLED(CONFIG_HAS_VPCI); }) >> #endif /* __ASM_DOMAIN_H__ */ >> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h >> index 756f8637ab..c58152de80 100644 >> --- a/xen/include/asm-arm/pci.h >> +++ b/xen/include/asm-arm/pci.h >> @@ -27,6 +27,14 @@ struct arch_pci_dev { >> struct device dev; >> }; >> +/* Arch-specific MSI data for vPCI. */ >> +struct vpci_arch_msi { >> +}; >> + >> +/* Arch-specific MSI-X entry data for vPCI. */ >> +struct vpci_arch_msix_entry { >> +}; >> + >> /* >> * struct to hold the mappings of a config space window. This >> * is expected to be used as sysdata for PCI controllers that >> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h >> index 64a2ca30da..0a9749e768 100644 >> --- a/xen/include/public/arch-arm.h >> +++ b/xen/include/public/arch-arm.h >> @@ -422,6 +422,10 @@ typedef uint64_t xen_callback_t; >> #define GUEST_PL011_BASE xen_mk_ullong(0x22000000) >> #define GUEST_PL011_SIZE xen_mk_ullong(0x00001000) >> +/* VPCI ECAM mappings */ >> +#define GUEST_VPCI_ECAM_BASE xen_mk_ullong(0x10000000) > > All the values for the memory layout has been defined in ascending order. So please add the vCPI at the correct place. If I am not mistaken, this should be before the GUEST_ACPI_*. Ack. > >> +#define GUEST_VPCI_ECAM_SIZE xen_mk_ullong(0x10000000) > > Please document how to decide the size. This is important for the future if we need to change the size. Ok let me document in next version. /* * 256 MB is reserved for VPCI configuration space based on calculation * 256 buses × 32 devices × 8 functions × 4 KB = 256 MB */ #define GUEST_VPCI_ECAM_BASE xen_mk_ullong(0x10000000) #define GUEST_VPCI_ECAM_SIZE xen_mk_ullong(0x10000000) Regrads, Rahul > > Cheers, > > -- > Julien Grall
Hi Stefano, > On 10 Sep 2021, at 1:26 am, Stefano Stabellini <sstabellini@kernel.org> wrote: > > On Thu, 19 Aug 2021, Rahul Singh wrote: >> The existing VPCI support available for X86 is adapted for Arm. >> When the device is added to XEN via the hyper call >> “PHYSDEVOP_pci_device_add”, VPCI handler for the config space >> access is added to the Xen to emulate the PCI devices config space. > > This is done just for device discovery, right? > > Although it is currently not implemented (and I am not asking to > implement it now, I am only trying to understand the architecture), it > would be possible to discover all PCI devices just by walking down the > PCI hierarchy by ourselves in Xen (no Dom0 interactions) given that we > have an ECAM driver. > > I take that would be the way to implement PCI support for Dom0less? It is not possible to discover PCI devices in XEN if enumeration is not done before XEN boot. If boot firmware did the enumeration, XEN will discover the PCI device. > > >> A MMIO trap handler for the PCI ECAM space is registered in XEN >> so that when guest is trying to access the PCI config space,XEN >> will trap the access and emulate read/write using the VPCI and >> not the real PCI hardware. >> >> Signed-off-by: Rahul Singh <rahul.singh@arm.com> >> --- >> xen/arch/arm/Makefile | 1 + >> xen/arch/arm/domain.c | 4 ++ >> xen/arch/arm/vpci.c | 96 +++++++++++++++++++++++++++++++++++ >> xen/arch/arm/vpci.h | 37 ++++++++++++++ >> xen/drivers/passthrough/pci.c | 7 +++ >> xen/drivers/vpci/Makefile | 3 +- >> xen/drivers/vpci/header.c | 2 + >> xen/include/asm-arm/domain.h | 5 +- >> xen/include/asm-arm/pci.h | 8 +++ >> xen/include/public/arch-arm.h | 4 ++ >> 10 files changed, 165 insertions(+), 2 deletions(-) >> create mode 100644 xen/arch/arm/vpci.c >> create mode 100644 xen/arch/arm/vpci.h >> >> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile >> index 0e14a5e5c8..7cdce684a4 100644 >> --- a/xen/arch/arm/Makefile >> +++ b/xen/arch/arm/Makefile >> @@ -7,6 +7,7 @@ obj-y += platforms/ >> endif >> obj-$(CONFIG_TEE) += tee/ >> obj-$(CONFIG_HAS_PCI) += pci/ >> +obj-$(CONFIG_HAS_VPCI) += vpci.o >> >> obj-$(CONFIG_HAS_ALTERNATIVE) += alternative.o >> obj-y += bootfdt.init.o >> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c >> index 19c756ac3d..d99c653626 100644 >> --- a/xen/arch/arm/domain.c >> +++ b/xen/arch/arm/domain.c >> @@ -40,6 +40,7 @@ >> #include <asm/vtimer.h> >> >> #include "vuart.h" >> +#include "vpci.h" >> >> DEFINE_PER_CPU(struct vcpu *, curr_vcpu); >> >> @@ -767,6 +768,9 @@ int arch_domain_create(struct domain *d, >> if ( is_hardware_domain(d) && (rc = domain_vuart_init(d)) ) >> goto fail; >> >> + if ( (rc = domain_vpci_init(d)) != 0 ) >> + goto fail; >> + >> return 0; >> >> fail: >> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c >> new file mode 100644 >> index 0000000000..da8b1ca13c >> --- /dev/null >> +++ b/xen/arch/arm/vpci.c >> @@ -0,0 +1,96 @@ >> +/* >> + * xen/arch/arm/vpci.c >> + * Copyright (c) 2021 Arm Ltd. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + */ >> +#include <xen/sched.h> >> +#include <asm/mmio.h> >> + >> +/* Do some sanity checks. */ >> +static bool vpci_mmio_access_allowed(unsigned int reg, unsigned int len) >> +{ >> + /* Check access size. */ >> + if ( len != 1 && len != 2 && len != 4 && len != 8 ) >> + return false; >> + >> + /* Check that access is size aligned. */ >> + if ( (reg & (len - 1)) ) >> + return false; >> + >> + return true; >> +} >> + >> +static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, >> + register_t *r, void *p) >> +{ >> + unsigned int reg; >> + pci_sbdf_t sbdf; >> + uint32_t data = 0; >> + unsigned int size = 1U << info->dabt.size; >> + >> + sbdf.sbdf = (((info->gpa) & 0x0ffff000) >> 12); >> + reg = (((info->gpa) & 0x00000ffc) | (info->gpa & 3)); >> + >> + if ( !vpci_mmio_access_allowed(reg, size) ) >> + return 1; >> + >> + data = vpci_read(sbdf, reg, size); >> + >> + memcpy(r, &data, size); >> + >> + return 1; >> +} >> + >> +static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info, >> + register_t r, void *p) >> +{ >> + unsigned int reg; >> + pci_sbdf_t sbdf; >> + uint32_t data = r; >> + unsigned int size = 1U << info->dabt.size; >> + >> + sbdf.sbdf = (((info->gpa) & 0x0ffff000) >> 12); >> + reg = (((info->gpa) & 0x00000ffc) | (info->gpa & 3)); >> + >> + if ( !vpci_mmio_access_allowed(reg, size) ) >> + return 1; >> + >> + vpci_write(sbdf, reg, size, data); >> + >> + return 1; >> +} >> + >> +static const struct mmio_handler_ops vpci_mmio_handler = { >> + .read = vpci_mmio_read, >> + .write = vpci_mmio_write, >> +}; >> + >> +int domain_vpci_init(struct domain *d) >> +{ >> + if ( !has_vpci(d) ) >> + return 0; >> + >> + register_mmio_handler(d, &vpci_mmio_handler, >> + GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL); >> + >> + return 0; >> +} >> + >> +/* >> + * Local variables: >> + * mode: C >> + * c-file-style: "BSD" >> + * c-basic-offset: 4 >> + * indent-tabs-mode: nil >> + * End: >> + */ >> + >> diff --git a/xen/arch/arm/vpci.h b/xen/arch/arm/vpci.h >> new file mode 100644 >> index 0000000000..8a093bb705 >> --- /dev/null >> +++ b/xen/arch/arm/vpci.h >> @@ -0,0 +1,37 @@ >> +/* >> + * xen/arch/arm/vpci.h >> + * Copyright (c) 2021 Arm Ltd. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + */ >> + >> +#ifndef __ARCH_ARM_VPCI_H__ >> +#define __ARCH_ARM_VPCI_H__ >> + >> +#ifdef CONFIG_HAS_VPCI >> +int domain_vpci_init(struct domain *d); >> +#else >> +static inline int domain_vpci_init(struct domain *d) >> +{ >> + return 0; >> +} >> +#endif >> + >> +#endif /* __ARCH_ARM_VPCI_H__ */ >> + >> +/* >> + * Local variables: >> + * mode: C >> + * c-file-style: "BSD" >> + * c-basic-offset: 4 >> + * indent-tabs-mode: nil >> + * End: >> + */ >> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c >> index c23c8cb06b..56e261e9bd 100644 >> --- a/xen/drivers/passthrough/pci.c >> +++ b/xen/drivers/passthrough/pci.c >> @@ -767,6 +767,13 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, >> else >> iommu_enable_device(pdev); >> >> +#ifdef CONFIG_ARM >> + ret = vpci_add_handlers(pdev); >> + if ( ret ) { >> + printk(XENLOG_ERR "setup of vPCI for failed: %d\n",ret); >> + goto out; >> + } >> +#endif >> pci_enable_acs(pdev); >> >> out: >> diff --git a/xen/drivers/vpci/Makefile b/xen/drivers/vpci/Makefile >> index 55d1bdfda0..1a1413b93e 100644 >> --- a/xen/drivers/vpci/Makefile >> +++ b/xen/drivers/vpci/Makefile >> @@ -1 +1,2 @@ >> -obj-y += vpci.o header.o msi.o msix.o >> +obj-y += vpci.o header.o >> +obj-$(CONFIG_HAS_PCI_MSI) += msi.o msix.o >> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c >> index ba9a036202..f8cd55e7c0 100644 >> --- a/xen/drivers/vpci/header.c >> +++ b/xen/drivers/vpci/header.c >> @@ -96,8 +96,10 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd, >> * FIXME: punching holes after the p2m has been set up might be racy for >> * DomU usage, needs to be revisited. >> */ >> +#ifdef CONFIG_HAS_PCI_MSI >> if ( map && !rom_only && vpci_make_msix_hole(pdev) ) >> return; >> +#endif >> >> for ( i = 0; i < ARRAY_SIZE(header->bars); i++ ) >> { >> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h >> index c9277b5c6d..d742b94bd6 100644 >> --- a/xen/include/asm-arm/domain.h >> +++ b/xen/include/asm-arm/domain.h >> @@ -262,7 +262,10 @@ static inline void arch_vcpu_block(struct vcpu *v) {} >> >> #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag) >> >> -#define has_vpci(d) ({ (void)(d); false; }) >> +/* For X86 VPCI is enabled and tested for PVH DOM0 only but >> + * for ARM we enable support VPCI for guest domain also. >> + */ >> +#define has_vpci(d) ({ (void)(d); IS_ENABLED(CONFIG_HAS_VPCI); }) >> >> #endif /* __ASM_DOMAIN_H__ */ >> >> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h >> index 756f8637ab..c58152de80 100644 >> --- a/xen/include/asm-arm/pci.h >> +++ b/xen/include/asm-arm/pci.h >> @@ -27,6 +27,14 @@ struct arch_pci_dev { >> struct device dev; >> }; >> >> +/* Arch-specific MSI data for vPCI. */ >> +struct vpci_arch_msi { >> +}; >> + >> +/* Arch-specific MSI-X entry data for vPCI. */ >> +struct vpci_arch_msix_entry { >> +}; >> + >> /* >> * struct to hold the mappings of a config space window. This >> * is expected to be used as sysdata for PCI controllers that >> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h >> index 64a2ca30da..0a9749e768 100644 >> --- a/xen/include/public/arch-arm.h >> +++ b/xen/include/public/arch-arm.h >> @@ -422,6 +422,10 @@ typedef uint64_t xen_callback_t; >> #define GUEST_PL011_BASE xen_mk_ullong(0x22000000) >> #define GUEST_PL011_SIZE xen_mk_ullong(0x00001000) >> >> +/* VPCI ECAM mappings */ >> +#define GUEST_VPCI_ECAM_BASE xen_mk_ullong(0x10000000) >> +#define GUEST_VPCI_ECAM_SIZE xen_mk_ullong(0x10000000) > > Isn't 256MB a bit too small? Usually PCI comes with two ranges: > > - a small range below 4GB like this one > - an additional large range above 4GB, around 1GB or larger > > We are missing the second larger range? As per PCI specification PCI Express extends the Configuration Space to 4096 bytes( 4KB) per Function and based on calculation "256 buses × 32 devices × 8 functions × 4 KB = 256 MB”, 256 MB is sufficient as per my understaning. Regards, Rahul
On Thu, 16 Sep 2021, Rahul Singh wrote: > Hi Stefano, > > > On 10 Sep 2021, at 1:26 am, Stefano Stabellini <sstabellini@kernel.org> wrote: > > > > On Thu, 19 Aug 2021, Rahul Singh wrote: > >> The existing VPCI support available for X86 is adapted for Arm. > >> When the device is added to XEN via the hyper call > >> “PHYSDEVOP_pci_device_add”, VPCI handler for the config space > >> access is added to the Xen to emulate the PCI devices config space. > > > > This is done just for device discovery, right? > > > > Although it is currently not implemented (and I am not asking to > > implement it now, I am only trying to understand the architecture), it > > would be possible to discover all PCI devices just by walking down the > > PCI hierarchy by ourselves in Xen (no Dom0 interactions) given that we > > have an ECAM driver. > > > > I take that would be the way to implement PCI support for Dom0less? > > It is not possible to discover PCI devices in XEN if enumeration is not done before XEN boot. > If boot firmware did the enumeration, XEN will discover the PCI device. OK, but if the boot firmware does the enumeration, how will Xen discover the PCI device exactly? Will Xen discover it because the PCI device will be present in device tree explicitly (will have its own device tree node)? Or will Xen discover it by walking the PCI bus?
Hi Stefano, > On 16 Sep 2021, at 9:26 pm, Stefano Stabellini <sstabellini@kernel.org> wrote: > > On Thu, 16 Sep 2021, Rahul Singh wrote: >> Hi Stefano, >> >>> On 10 Sep 2021, at 1:26 am, Stefano Stabellini <sstabellini@kernel.org> wrote: >>> >>> On Thu, 19 Aug 2021, Rahul Singh wrote: >>>> The existing VPCI support available for X86 is adapted for Arm. >>>> When the device is added to XEN via the hyper call >>>> “PHYSDEVOP_pci_device_add”, VPCI handler for the config space >>>> access is added to the Xen to emulate the PCI devices config space. >>> >>> This is done just for device discovery, right? >>> >>> Although it is currently not implemented (and I am not asking to >>> implement it now, I am only trying to understand the architecture), it >>> would be possible to discover all PCI devices just by walking down the >>> PCI hierarchy by ourselves in Xen (no Dom0 interactions) given that we >>> have an ECAM driver. >>> >>> I take that would be the way to implement PCI support for Dom0less? >> >> It is not possible to discover PCI devices in XEN if enumeration is not done before XEN boot. >> If boot firmware did the enumeration, XEN will discover the PCI device. > > OK, but if the boot firmware does the enumeration, how will Xen discover > the PCI device exactly? Will Xen discover it because the PCI device will > be present in device tree explicitly (will have its own device tree > node)? Or will Xen discover it by walking the PCI bus? Yes Xen discover it by walking the PCI bus. There is already a function in XEN scan_pci_devices() that we can use to discover the PCI device for dom0less system. Regards, Rahul
On Tue, 21 Sep 2021, Rahul Singh wrote: > Hi Stefano, > > > On 16 Sep 2021, at 9:26 pm, Stefano Stabellini <sstabellini@kernel.org> wrote: > > > > On Thu, 16 Sep 2021, Rahul Singh wrote: > >> Hi Stefano, > >> > >>> On 10 Sep 2021, at 1:26 am, Stefano Stabellini <sstabellini@kernel.org> wrote: > >>> > >>> On Thu, 19 Aug 2021, Rahul Singh wrote: > >>>> The existing VPCI support available for X86 is adapted for Arm. > >>>> When the device is added to XEN via the hyper call > >>>> “PHYSDEVOP_pci_device_add”, VPCI handler for the config space > >>>> access is added to the Xen to emulate the PCI devices config space. > >>> > >>> This is done just for device discovery, right? > >>> > >>> Although it is currently not implemented (and I am not asking to > >>> implement it now, I am only trying to understand the architecture), it > >>> would be possible to discover all PCI devices just by walking down the > >>> PCI hierarchy by ourselves in Xen (no Dom0 interactions) given that we > >>> have an ECAM driver. > >>> > >>> I take that would be the way to implement PCI support for Dom0less? > >> > >> It is not possible to discover PCI devices in XEN if enumeration is not done before XEN boot. > >> If boot firmware did the enumeration, XEN will discover the PCI device. > > > > OK, but if the boot firmware does the enumeration, how will Xen discover > > the PCI device exactly? Will Xen discover it because the PCI device will > > be present in device tree explicitly (will have its own device tree > > node)? Or will Xen discover it by walking the PCI bus? > > Yes Xen discover it by walking the PCI bus. There is already a function in XEN scan_pci_devices() that we can use > to discover the PCI device for dom0less system. OK. Please add a statement to the commit message on how dom0less support could be implemented. It doesn't have to be implemented now of course but it would be good to keep a note for future reference.
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile index 0e14a5e5c8..7cdce684a4 100644 --- a/xen/arch/arm/Makefile +++ b/xen/arch/arm/Makefile @@ -7,6 +7,7 @@ obj-y += platforms/ endif obj-$(CONFIG_TEE) += tee/ obj-$(CONFIG_HAS_PCI) += pci/ +obj-$(CONFIG_HAS_VPCI) += vpci.o obj-$(CONFIG_HAS_ALTERNATIVE) += alternative.o obj-y += bootfdt.init.o diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 19c756ac3d..d99c653626 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -40,6 +40,7 @@ #include <asm/vtimer.h> #include "vuart.h" +#include "vpci.h" DEFINE_PER_CPU(struct vcpu *, curr_vcpu); @@ -767,6 +768,9 @@ int arch_domain_create(struct domain *d, if ( is_hardware_domain(d) && (rc = domain_vuart_init(d)) ) goto fail; + if ( (rc = domain_vpci_init(d)) != 0 ) + goto fail; + return 0; fail: diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c new file mode 100644 index 0000000000..da8b1ca13c --- /dev/null +++ b/xen/arch/arm/vpci.c @@ -0,0 +1,96 @@ +/* + * xen/arch/arm/vpci.c + * Copyright (c) 2021 Arm Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ +#include <xen/sched.h> +#include <asm/mmio.h> + +/* Do some sanity checks. */ +static bool vpci_mmio_access_allowed(unsigned int reg, unsigned int len) +{ + /* Check access size. */ + if ( len != 1 && len != 2 && len != 4 && len != 8 ) + return false; + + /* Check that access is size aligned. */ + if ( (reg & (len - 1)) ) + return false; + + return true; +} + +static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, + register_t *r, void *p) +{ + unsigned int reg; + pci_sbdf_t sbdf; + uint32_t data = 0; + unsigned int size = 1U << info->dabt.size; + + sbdf.sbdf = (((info->gpa) & 0x0ffff000) >> 12); + reg = (((info->gpa) & 0x00000ffc) | (info->gpa & 3)); + + if ( !vpci_mmio_access_allowed(reg, size) ) + return 1; + + data = vpci_read(sbdf, reg, size); + + memcpy(r, &data, size); + + return 1; +} + +static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info, + register_t r, void *p) +{ + unsigned int reg; + pci_sbdf_t sbdf; + uint32_t data = r; + unsigned int size = 1U << info->dabt.size; + + sbdf.sbdf = (((info->gpa) & 0x0ffff000) >> 12); + reg = (((info->gpa) & 0x00000ffc) | (info->gpa & 3)); + + if ( !vpci_mmio_access_allowed(reg, size) ) + return 1; + + vpci_write(sbdf, reg, size, data); + + return 1; +} + +static const struct mmio_handler_ops vpci_mmio_handler = { + .read = vpci_mmio_read, + .write = vpci_mmio_write, +}; + +int domain_vpci_init(struct domain *d) +{ + if ( !has_vpci(d) ) + return 0; + + register_mmio_handler(d, &vpci_mmio_handler, + GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL); + + return 0; +} + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */ + diff --git a/xen/arch/arm/vpci.h b/xen/arch/arm/vpci.h new file mode 100644 index 0000000000..8a093bb705 --- /dev/null +++ b/xen/arch/arm/vpci.h @@ -0,0 +1,37 @@ +/* + * xen/arch/arm/vpci.h + * Copyright (c) 2021 Arm Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef __ARCH_ARM_VPCI_H__ +#define __ARCH_ARM_VPCI_H__ + +#ifdef CONFIG_HAS_VPCI +int domain_vpci_init(struct domain *d); +#else +static inline int domain_vpci_init(struct domain *d) +{ + return 0; +} +#endif + +#endif /* __ARCH_ARM_VPCI_H__ */ + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index c23c8cb06b..56e261e9bd 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -767,6 +767,13 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, else iommu_enable_device(pdev); +#ifdef CONFIG_ARM + ret = vpci_add_handlers(pdev); + if ( ret ) { + printk(XENLOG_ERR "setup of vPCI for failed: %d\n",ret); + goto out; + } +#endif pci_enable_acs(pdev); out: diff --git a/xen/drivers/vpci/Makefile b/xen/drivers/vpci/Makefile index 55d1bdfda0..1a1413b93e 100644 --- a/xen/drivers/vpci/Makefile +++ b/xen/drivers/vpci/Makefile @@ -1 +1,2 @@ -obj-y += vpci.o header.o msi.o msix.o +obj-y += vpci.o header.o +obj-$(CONFIG_HAS_PCI_MSI) += msi.o msix.o diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c index ba9a036202..f8cd55e7c0 100644 --- a/xen/drivers/vpci/header.c +++ b/xen/drivers/vpci/header.c @@ -96,8 +96,10 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd, * FIXME: punching holes after the p2m has been set up might be racy for * DomU usage, needs to be revisited. */ +#ifdef CONFIG_HAS_PCI_MSI if ( map && !rom_only && vpci_make_msix_hole(pdev) ) return; +#endif for ( i = 0; i < ARRAY_SIZE(header->bars); i++ ) { diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index c9277b5c6d..d742b94bd6 100644 --- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -262,7 +262,10 @@ static inline void arch_vcpu_block(struct vcpu *v) {} #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag) -#define has_vpci(d) ({ (void)(d); false; }) +/* For X86 VPCI is enabled and tested for PVH DOM0 only but + * for ARM we enable support VPCI for guest domain also. + */ +#define has_vpci(d) ({ (void)(d); IS_ENABLED(CONFIG_HAS_VPCI); }) #endif /* __ASM_DOMAIN_H__ */ diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h index 756f8637ab..c58152de80 100644 --- a/xen/include/asm-arm/pci.h +++ b/xen/include/asm-arm/pci.h @@ -27,6 +27,14 @@ struct arch_pci_dev { struct device dev; }; +/* Arch-specific MSI data for vPCI. */ +struct vpci_arch_msi { +}; + +/* Arch-specific MSI-X entry data for vPCI. */ +struct vpci_arch_msix_entry { +}; + /* * struct to hold the mappings of a config space window. This * is expected to be used as sysdata for PCI controllers that diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h index 64a2ca30da..0a9749e768 100644 --- a/xen/include/public/arch-arm.h +++ b/xen/include/public/arch-arm.h @@ -422,6 +422,10 @@ typedef uint64_t xen_callback_t; #define GUEST_PL011_BASE xen_mk_ullong(0x22000000) #define GUEST_PL011_SIZE xen_mk_ullong(0x00001000) +/* VPCI ECAM mappings */ +#define GUEST_VPCI_ECAM_BASE xen_mk_ullong(0x10000000) +#define GUEST_VPCI_ECAM_SIZE xen_mk_ullong(0x10000000) + /* * 16MB == 4096 pages reserved for guest to use as a region to map its * grant table in.
The existing VPCI support available for X86 is adapted for Arm. When the device is added to XEN via the hyper call “PHYSDEVOP_pci_device_add”, VPCI handler for the config space access is added to the Xen to emulate the PCI devices config space. A MMIO trap handler for the PCI ECAM space is registered in XEN so that when guest is trying to access the PCI config space,XEN will trap the access and emulate read/write using the VPCI and not the real PCI hardware. Signed-off-by: Rahul Singh <rahul.singh@arm.com> --- xen/arch/arm/Makefile | 1 + xen/arch/arm/domain.c | 4 ++ xen/arch/arm/vpci.c | 96 +++++++++++++++++++++++++++++++++++ xen/arch/arm/vpci.h | 37 ++++++++++++++ xen/drivers/passthrough/pci.c | 7 +++ xen/drivers/vpci/Makefile | 3 +- xen/drivers/vpci/header.c | 2 + xen/include/asm-arm/domain.h | 5 +- xen/include/asm-arm/pci.h | 8 +++ xen/include/public/arch-arm.h | 4 ++ 10 files changed, 165 insertions(+), 2 deletions(-) create mode 100644 xen/arch/arm/vpci.c create mode 100644 xen/arch/arm/vpci.h