| Message ID | 847d430fdeb19e695176ddea71eeb11dcef8b23e.1634305870.git.bertrand.marquis@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI devices passthrough on Arm | expand |
On Fri, Oct 15, 2021 at 02:59:19PM +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 and prevent 64bit accesses on 32bit: > - use %zu instead of lu in header.c for print > - prevent 64bit accesses in vpci_access_allowed > - ifdef out using CONFIG_64BIT handling of len 8 in > vpci_ecam_{read/write} > > Signed-off-by: Rahul Singh <rahul.singh@arm.com> > Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> The vpci bits looks fine to me, so: Acked-by: Roger Pau Monné <roger.pau@citrix.com> I have one question however related to the placement of the vpci setup call in pci_add_device. > diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c > index 3aa8c3175f..082892c8a2 100644 > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -766,7 +766,21 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, > list_add(&pdev->domain_list, &hardware_domain->pdev_list); > } > else > + { > +#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; > + } I'm likely lost here, but shouldn't this also be done for devices that belong to the hardware domain and are assigned to it in the first branch of this conditional? Or else you will end up with devices assigned to the hardware domain that don't have vPCI setup for them. Thanks, Roger.
Hi Roger, > On 15 Oct 2021, at 15:30, Roger Pau Monné <roger.pau@citrix.com> wrote: > > On Fri, Oct 15, 2021 at 02:59:19PM +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 and prevent 64bit accesses on 32bit: >> - use %zu instead of lu in header.c for print >> - prevent 64bit accesses in vpci_access_allowed >> - ifdef out using CONFIG_64BIT handling of len 8 in >> vpci_ecam_{read/write} >> >> Signed-off-by: Rahul Singh <rahul.singh@arm.com> >> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> > > The vpci bits looks fine to me, so: > > Acked-by: Roger Pau Monné <roger.pau@citrix.com> Thanks > > I have one question however related to the placement of the vpci setup > call in pci_add_device. > >> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c >> index 3aa8c3175f..082892c8a2 100644 >> --- a/xen/drivers/passthrough/pci.c >> +++ b/xen/drivers/passthrough/pci.c >> @@ -766,7 +766,21 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, >> list_add(&pdev->domain_list, &hardware_domain->pdev_list); >> } >> else >> + { >> +#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; >> + } > > I'm likely lost here, but shouldn't this also be done for devices that > belong to the hardware domain and are assigned to it in the first > branch of this conditional? > > Or else you will end up with devices assigned to the hardware domain > that don't have vPCI setup for them. I might be wrong but when the hardware domain is declaring the devices they are added to him. Then later when those device are assigned to a guest, they are removed from the hardware domain. Regards Bertrand > > Thanks, Roger.
Hi Bertrand, On 15/10/2021 14:59, Bertrand Marquis wrote: > diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c > index 3aa8c3175f..082892c8a2 100644 > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -766,7 +766,21 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, > list_add(&pdev->domain_list, &hardware_domain->pdev_list); > } > else > + { > +#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); Sorry for only noticing it now. Looking at the last staging vpci_add_handlers() is annotated with __hwdom_init. On Arm, __hwdom_init means the function will disappear after boot. However, pci_add_device() can be called from a physdev op. So I think we would need to drop __hwdom_init. I can't seem to find this change in this series. Did I miss anything? The rest of the changes LGTM. Cheers,
Hi, > On 15 Oct 2021, at 16:10, Julien Grall <julien@xen.org> wrote: > > Hi Bertrand, > > On 15/10/2021 14:59, Bertrand Marquis wrote: >> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c >> index 3aa8c3175f..082892c8a2 100644 >> --- a/xen/drivers/passthrough/pci.c >> +++ b/xen/drivers/passthrough/pci.c >> @@ -766,7 +766,21 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, >> list_add(&pdev->domain_list, &hardware_domain->pdev_list); >> } >> else >> + { >> +#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); > > Sorry for only noticing it now. Looking at the last staging > vpci_add_handlers() is annotated with __hwdom_init. On Arm, __hwdom_init means the function will disappear after boot. > > However, pci_add_device() can be called from a physdev op. So I think we would need to drop __hwdom_init. I can't seem to find this change in this series. Did I miss anything? Good catch and not this is not in the serie. Can we consider that a bug so that I can send a new patch or should I send a v8 ? Cheers Bertrand > > The rest of the changes LGTM. > > Cheers, > > -- > Julien Grall
Hi Bertrand, On 15/10/2021 16:06, Bertrand Marquis wrote: >> On 15 Oct 2021, at 15:30, Roger Pau Monné <roger.pau@citrix.com> wrote: >> >> On Fri, Oct 15, 2021 at 02:59:19PM +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 and prevent 64bit accesses on 32bit: >>> - use %zu instead of lu in header.c for print >>> - prevent 64bit accesses in vpci_access_allowed >>> - ifdef out using CONFIG_64BIT handling of len 8 in >>> vpci_ecam_{read/write} >>> >>> Signed-off-by: Rahul Singh <rahul.singh@arm.com> >>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> >> >> The vpci bits looks fine to me, so: >> >> Acked-by: Roger Pau Monné <roger.pau@citrix.com> > > Thanks > >> >> I have one question however related to the placement of the vpci setup >> call in pci_add_device. >> >>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c >>> index 3aa8c3175f..082892c8a2 100644 >>> --- a/xen/drivers/passthrough/pci.c >>> +++ b/xen/drivers/passthrough/pci.c >>> @@ -766,7 +766,21 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, >>> list_add(&pdev->domain_list, &hardware_domain->pdev_list); >>> } >>> else >>> + { >>> +#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; >>> + } >> >> I'm likely lost here, but shouldn't this also be done for devices that >> belong to the hardware domain and are assigned to it in the first >> branch of this conditional? >> >> Or else you will end up with devices assigned to the hardware domain >> that don't have vPCI setup for them. > > I might be wrong but when the hardware domain is declaring the devices they are added to him. > Then later when those device are assigned to a guest, they are removed from the hardware domain. From my understanding, when the device is initially registered we would go through the first branch because pdev->domain is not yet set. The else would be taken only with subsequent call of PHYSDEVOP_manage_pci_add & co. For the device assignment, a different path would be taken. This would go through the domctl XEN_DOMCTL_assign_device. Therefore, I think Roger is right and the call belongs to the first branch. Otherwise, we would miss out the vpci handlers in some cases. Cheers,
On Fri, 15 Oct 2021, Bertrand Marquis wrote: > Hi, > > > On 15 Oct 2021, at 16:10, Julien Grall <julien@xen.org> wrote: > > > > Hi Bertrand, > > > > On 15/10/2021 14:59, Bertrand Marquis wrote: > >> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c > >> index 3aa8c3175f..082892c8a2 100644 > >> --- a/xen/drivers/passthrough/pci.c > >> +++ b/xen/drivers/passthrough/pci.c > >> @@ -766,7 +766,21 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, > >> list_add(&pdev->domain_list, &hardware_domain->pdev_list); > >> } > >> else > >> + { > >> +#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); > > > > Sorry for only noticing it now. Looking at the last staging > > vpci_add_handlers() is annotated with __hwdom_init. On Arm, __hwdom_init means the function will disappear after boot. > > > > However, pci_add_device() can be called from a physdev op. So I think we would need to drop __hwdom_init. I can't seem to find this change in this series. Did I miss anything? > > Good catch and not this is not in the serie. > > Can we consider that a bug so that I can send a new patch or should I send a v8 ? We don't typically do that, but I could make the change on commit, or merge a second patch from you with this one on commit, after I run all the gitlab-ci tests. (I still have to read the series but FYI)
Hi, > On 15 Oct 2021, at 16:20, Stefano Stabellini <sstabellini@kernel.org> wrote: > > On Fri, 15 Oct 2021, Bertrand Marquis wrote: >> Hi, >> >>> On 15 Oct 2021, at 16:10, Julien Grall <julien@xen.org> wrote: >>> >>> Hi Bertrand, >>> >>> On 15/10/2021 14:59, Bertrand Marquis wrote: >>>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c >>>> index 3aa8c3175f..082892c8a2 100644 >>>> --- a/xen/drivers/passthrough/pci.c >>>> +++ b/xen/drivers/passthrough/pci.c >>>> @@ -766,7 +766,21 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, >>>> list_add(&pdev->domain_list, &hardware_domain->pdev_list); >>>> } >>>> else >>>> + { >>>> +#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); >>> >>> Sorry for only noticing it now. Looking at the last staging >>> vpci_add_handlers() is annotated with __hwdom_init. On Arm, __hwdom_init means the function will disappear after boot. >>> >>> However, pci_add_device() can be called from a physdev op. So I think we would need to drop __hwdom_init. I can't seem to find this change in this series. Did I miss anything? >> >> Good catch and not this is not in the serie. >> >> Can we consider that a bug so that I can send a new patch or should I send a v8 ? > > We don't typically do that, but I could make the change on commit, or > merge a second patch from you with this one on commit, after I run all > the gitlab-ci tests. Thanks but we need to sort out the where first (Julien’s mail). I guess a v8 will be required. Cheers Bertrand > > (I still have to read the series but FYI)
Hi Julien, > On 15 Oct 2021, at 16:18, Julien Grall <julien@xen.org> wrote: > > Hi Bertrand, > > On 15/10/2021 16:06, Bertrand Marquis wrote: >>> On 15 Oct 2021, at 15:30, Roger Pau Monné <roger.pau@citrix.com> wrote: >>> >>> On Fri, Oct 15, 2021 at 02:59:19PM +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 and prevent 64bit accesses on 32bit: >>>> - use %zu instead of lu in header.c for print >>>> - prevent 64bit accesses in vpci_access_allowed >>>> - ifdef out using CONFIG_64BIT handling of len 8 in >>>> vpci_ecam_{read/write} >>>> >>>> Signed-off-by: Rahul Singh <rahul.singh@arm.com> >>>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> >>> >>> The vpci bits looks fine to me, so: >>> >>> Acked-by: Roger Pau Monné <roger.pau@citrix.com> >> Thanks >>> >>> I have one question however related to the placement of the vpci setup >>> call in pci_add_device. >>> >>>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c >>>> index 3aa8c3175f..082892c8a2 100644 >>>> --- a/xen/drivers/passthrough/pci.c >>>> +++ b/xen/drivers/passthrough/pci.c >>>> @@ -766,7 +766,21 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, >>>> list_add(&pdev->domain_list, &hardware_domain->pdev_list); >>>> } >>>> else >>>> + { >>>> +#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; >>>> + } >>> >>> I'm likely lost here, but shouldn't this also be done for devices that >>> belong to the hardware domain and are assigned to it in the first >>> branch of this conditional? >>> >>> Or else you will end up with devices assigned to the hardware domain >>> that don't have vPCI setup for them. >> I might be wrong but when the hardware domain is declaring the devices they are added to him. >> Then later when those device are assigned to a guest, they are removed from the hardware domain. > > From my understanding, when the device is initially registered we would go through the first branch because pdev->domain is not yet set. > > The else would be taken only with subsequent call of PHYSDEVOP_manage_pci_add & co. > > For the device assignment, a different path would be taken. This would go through the domctl XEN_DOMCTL_assign_device. > > Therefore, I think Roger is right and the call belongs to the first branch. Otherwise, we would miss out the vpci handlers in some cases. Yes we only want this to be done once on the first call. So in fact it should be done when pdev->domain is NULL in the first branch. I will do this in v8. Thanks a lot for the analysis, saying it makes things more clear :-) Cheers Bertrand > > Cheers, > > -- > Julien Grall
Hi, > On 15 Oct 2021, at 14:59, Bertrand Marquis <bertrand.marquis@arm.com> 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 and prevent 64bit accesses on 32bit: > - use %zu instead of lu in header.c for print > - prevent 64bit accesses in vpci_access_allowed > - ifdef out using CONFIG_64BIT handling of len 8 in > vpci_ecam_{read/write} > > Signed-off-by: Rahul Singh <rahul.singh@arm.com> > Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> > --- > Changes in v7: > - adapt to changes in vpci generic functions (name and type) > - remove call to pci_cleanup_msi on error exit > - move pci_add_handlers to be only done when pdev->domain is not NULL > - Remove cast to unsigned long in header.c and use %zu for print > - Fix non ascii chars in arch-arm.h > - Use CONFIG_64BIT inside vpci_access_allowed to prevent 64bit access on > 32bit platforms > - Use CONFIG_64BIT to compile out 64bit cases in vpci_ecam_{read/write} > 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 | 77 +++++++++++++++++++++++++++++++++++ > xen/arch/arm/vpci.h | 36 ++++++++++++++++ > xen/drivers/passthrough/pci.c | 14 +++++++ > xen/drivers/vpci/header.c | 2 +- > xen/drivers/vpci/vpci.c | 10 +++++ > xen/include/asm-arm/domain.h | 1 + > xen/include/public/arch-arm.h | 7 ++++ > xen/include/xen/pci.h | 2 + > 10 files changed, 153 insertions(+), 1 deletion(-) > 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..a312d02f3d > --- /dev/null > +++ b/xen/arch/arm/vpci.c > @@ -0,0 +1,77 @@ > +/* > + * 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; > + > + /* We ignore segment part and always handle segment 0 */ > + sbdf.sbdf = VCPI_ECAM_BDF(info->gpa); Typo: s/VCPI/VPCI !! Sorry for that. Will fix in v8. Cheers Bertrand > + > + if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa), > + 1U << info->dabt.size, &data) ) > + { > + *r = data; > + return 1; > + } > + > + *r = ~0ul; > + > + return 0; > +} > + > +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 = VCPI_ECAM_BDF(info->gpa); Typo: s/VCPI/VPCI ! > + > + return vpci_ecam_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..082892c8a2 100644 > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -766,7 +766,21 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, > list_add(&pdev->domain_list, &hardware_domain->pdev_list); > } > else > + { > +#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 > iommu_enable_device(pdev); > + } > > pci_enable_acs(pdev); > > diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c > index f8cd55e7c0..40ff79c33f 100644 > --- a/xen/drivers/vpci/header.c > +++ b/xen/drivers/vpci/header.c > @@ -373,7 +373,7 @@ static void bar_write(const struct pci_dev *pdev, unsigned int reg, > /* If the value written is the current one avoid printing a warning. */ > if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) ) > gprintk(XENLOG_WARNING, > - "%pp: ignored BAR %lu write with memory decoding enabled\n", > + "%pp: ignored BAR %zu write with memory decoding enabled\n", > &pdev->sbdf, bar - pdev->vpci->header.bars + hi); > return; > } > diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c > index ef690f15a9..decf7d87a1 100644 > --- a/xen/drivers/vpci/vpci.c > +++ b/xen/drivers/vpci/vpci.c > @@ -485,6 +485,12 @@ bool vpci_access_allowed(unsigned int reg, unsigned int len) > if ( len != 1 && len != 2 && len != 4 && len != 8 ) > return false; > > +#ifndef CONFIG_64BIT > + /* Prevent 64bit accesses on 32bit */ > + if ( len == 8 ) > + return false; > +#endif > + > /* Check that access is size aligned. */ > if ( (reg & (len - 1)) ) > return false; > @@ -500,8 +506,10 @@ bool vpci_ecam_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int len, > return false; > > vpci_write(sbdf, reg, min(4u, len), data); > +#ifdef CONFIG_64BIT > if ( len == 8 ) > vpci_write(sbdf, reg + 4, 4, data >> 32); > +#endif > > return true; > } > @@ -526,8 +534,10 @@ bool vpci_ecam_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int len, > * 4byte accesses. > */ > *data = vpci_read(sbdf, reg, min(4u, len)); > +#ifdef CONFIG_64BIT > if ( len == 8 ) > *data |= (uint64_t)vpci_read(sbdf, reg + 4, 4) << 32; > +#endif > > return true; > } > 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/public/arch-arm.h b/xen/include/public/arch-arm.h > index 96ead3de07..b4c615bcdf 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 x 32 devices x 8 functions x 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..b6d7e454f8 100644 > --- a/xen/include/xen/pci.h > +++ b/xen/include/xen/pci.h > @@ -40,6 +40,8 @@ > #define PCI_SBDF3(s,b,df) \ > ((pci_sbdf_t){ .sbdf = (((s) & 0xffff) << 16) | PCI_BDF2(b, df) }) > > +#define ECAM_REG_OFFSET(addr) ((addr) & 0x00000fff) > + > typedef union { > uint32_t sbdf; > struct { > -- > 2.25.1 > >
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..a312d02f3d --- /dev/null +++ b/xen/arch/arm/vpci.c @@ -0,0 +1,77 @@ +/* + * 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; + + /* We ignore segment part and always handle segment 0 */ + sbdf.sbdf = VCPI_ECAM_BDF(info->gpa); + + if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa), + 1U << info->dabt.size, &data) ) + { + *r = data; + return 1; + } + + *r = ~0ul; + + return 0; +} + +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 = VCPI_ECAM_BDF(info->gpa); + + return vpci_ecam_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..082892c8a2 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -766,7 +766,21 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, list_add(&pdev->domain_list, &hardware_domain->pdev_list); } else + { +#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 iommu_enable_device(pdev); + } pci_enable_acs(pdev); diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c index f8cd55e7c0..40ff79c33f 100644 --- a/xen/drivers/vpci/header.c +++ b/xen/drivers/vpci/header.c @@ -373,7 +373,7 @@ static void bar_write(const struct pci_dev *pdev, unsigned int reg, /* If the value written is the current one avoid printing a warning. */ if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) ) gprintk(XENLOG_WARNING, - "%pp: ignored BAR %lu write with memory decoding enabled\n", + "%pp: ignored BAR %zu write with memory decoding enabled\n", &pdev->sbdf, bar - pdev->vpci->header.bars + hi); return; } diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c index ef690f15a9..decf7d87a1 100644 --- a/xen/drivers/vpci/vpci.c +++ b/xen/drivers/vpci/vpci.c @@ -485,6 +485,12 @@ bool vpci_access_allowed(unsigned int reg, unsigned int len) if ( len != 1 && len != 2 && len != 4 && len != 8 ) return false; +#ifndef CONFIG_64BIT + /* Prevent 64bit accesses on 32bit */ + if ( len == 8 ) + return false; +#endif + /* Check that access is size aligned. */ if ( (reg & (len - 1)) ) return false; @@ -500,8 +506,10 @@ bool vpci_ecam_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int len, return false; vpci_write(sbdf, reg, min(4u, len), data); +#ifdef CONFIG_64BIT if ( len == 8 ) vpci_write(sbdf, reg + 4, 4, data >> 32); +#endif return true; } @@ -526,8 +534,10 @@ bool vpci_ecam_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int len, * 4byte accesses. */ *data = vpci_read(sbdf, reg, min(4u, len)); +#ifdef CONFIG_64BIT if ( len == 8 ) *data |= (uint64_t)vpci_read(sbdf, reg + 4, 4) << 32; +#endif return true; } 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/public/arch-arm.h b/xen/include/public/arch-arm.h index 96ead3de07..b4c615bcdf 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 x 32 devices x 8 functions x 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..b6d7e454f8 100644 --- a/xen/include/xen/pci.h +++ b/xen/include/xen/pci.h @@ -40,6 +40,8 @@ #define PCI_SBDF3(s,b,df) \ ((pci_sbdf_t){ .sbdf = (((s) & 0xffff) << 16) | PCI_BDF2(b, df) }) +#define ECAM_REG_OFFSET(addr) ((addr) & 0x00000fff) + typedef union { uint32_t sbdf; struct {