| Message ID | 20211105063326.939843-4-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI devices passthrough on Arm, part 2 | expand |
On 05.11.21 08:33, Oleksandr Andrushchenko wrote: > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > [snip] > +int pci_host_iterate_bridges(struct domain *d, > + int (*cb)(struct domain *d, > + struct pci_host_bridge *bridge)) > +{ > + struct pci_host_bridge *bridge; > + int err; > + > + list_for_each_entry( bridge, &pci_host_bridges, node ) > + { > + err = cb(d, bridge); > + if ( err ) > + return err; > + } > + return 0; > +} > + > +unsigned int pci_host_get_num_bridges(void) > +{ > + struct pci_host_bridge *bridge; > + unsigned int count = 0; > + > + list_for_each_entry( bridge, &pci_host_bridges, node ) > + count++; > + > + return count; > +} > + > This can be even simpler if pci_host_iterate_bridges returns the count: -int pci_host_iterate_bridges(struct domain *d, - int (*cb)(struct domain *d, - struct pci_host_bridge *bridge)) +int pci_host_iterate_bridges_and_count(struct domain *d, + int (*cb)(struct domain *d, + struct pci_host_bridge *bridge)) { struct pci_host_bridge *bridge; - int err; + int err, count = 0; list_for_each_entry( bridge, &pci_host_bridges, node ) { err = cb(d, bridge); if ( err ) return err; + count += err; } - return 0; -} Then pci_host_get_num_bridges goes away and we can count different entities with the same iterator and a simple callback. This becomes possible as there is a single user for pci_host_iterate_bridges now which sets up MMIOs, so the change above seems to be reasonable I will include this change in v7 Thank you, Oleksandr
Hi Oleksandr, On 05/11/2021 06:33, Oleksandr Andrushchenko wrote: > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > In order for vPCI to work it needs to maintain guest and hardware > domain's views of the configuration space. For example, BARs and > COMMAND registers require emulation for guests and the guest view > of the registers needs to be in sync with the real contents of the > relevant registers. For that ECAM address space needs to also be > trapped for the hardware domain, so we need to implement PCI host > bridge specific callbacks to properly setup MMIO handlers for those > ranges depending on particular host bridge implementation. > > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > --- > Since v5: > - add vpci_sbdf_from_gpa helper for gpa to SBDF translation > - take bridge's bus start into account while calculating SBDF > Since v4: > - unsigned int for functions working with count > - gate number of MMIO handlers needed for CONFIG_HAS_PCI_MSI > and fix their number, e.g. single handler for PBA and > MSI-X tables (Roger) > - re-work code for assigning MMIO handlers to be simpler > and account on the fact that there could multiple host bridges > exist for the hwdom > Since v3: > - fixed comment formatting > Since v2: > - removed unneeded assignment (count = 0) > - removed unneeded header inclusion > - update commit message > Since v1: > - Dynamically calculate the number of MMIO handlers required for vPCI > and update the total number accordingly > - s/clb/cb > - Do not introduce a new callback for MMIO handler setup > --- > xen/arch/arm/domain.c | 2 + > xen/arch/arm/pci/pci-host-common.c | 27 ++++++++++++ > xen/arch/arm/vpci.c | 66 ++++++++++++++++++++++++++---- > xen/arch/arm/vpci.h | 6 +++ > xen/include/asm-arm/pci.h | 5 +++ > 5 files changed, 98 insertions(+), 8 deletions(-) > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > index 96e1b235501d..92a6c509e5c5 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -739,6 +739,8 @@ int arch_domain_create(struct domain *d, > if ( (rc = domain_vgic_register(d, &count)) != 0 ) > goto fail; > > + count += domain_vpci_get_num_mmio_handlers(d); > + > if ( (rc = domain_io_init(d, count + MAX_IO_HANDLER)) != 0 ) > goto fail; > > diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c > index 47104b22b221..0d271a6e8881 100644 > --- a/xen/arch/arm/pci/pci-host-common.c > +++ b/xen/arch/arm/pci/pci-host-common.c > @@ -289,6 +289,33 @@ int pci_get_host_bridge_segment(const struct dt_device_node *node, > return -EINVAL; > } > > +int pci_host_iterate_bridges(struct domain *d, > + int (*cb)(struct domain *d, > + struct pci_host_bridge *bridge)) > +{ > + struct pci_host_bridge *bridge; > + int err; > + > + list_for_each_entry( bridge, &pci_host_bridges, node ) > + { > + err = cb(d, bridge); > + if ( err ) > + return err; > + } > + return 0; > +} > + > +unsigned int pci_host_get_num_bridges(void) > +{ > + struct pci_host_bridge *bridge; > + unsigned int count = 0; How about making this static and... > + > + list_for_each_entry( bridge, &pci_host_bridges, node ) > + count++; ... only call list_for_each_entry() when count is -1? So we would only go through the list once. This should be fine given hostbridge can only be added during boot (we would need to protect pci_host_bridges with a lock otherwise). > + > + return count; > +} > + > /* > * Local variables: > * mode: C > diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c > index 23f45386f4b3..5a6ebd8b9868 100644 > --- a/xen/arch/arm/vpci.c > +++ b/xen/arch/arm/vpci.c > @@ -16,16 +16,31 @@ > > #include <asm/mmio.h> > > +static pci_sbdf_t vpci_sbdf_from_gpa(const struct pci_host_bridge *bridge, > + paddr_t gpa) > +{ > + pci_sbdf_t sbdf; > + > + if ( bridge ) > + { > + sbdf.sbdf = VPCI_ECAM_BDF(gpa - bridge->cfg->phys_addr); > + sbdf.seg = bridge->segment; > + sbdf.bus += bridge->cfg->busn_start; > + } > + else > + sbdf.sbdf = VPCI_ECAM_BDF(gpa - GUEST_VPCI_ECAM_BASE); > + > + return sbdf; > +} > + > static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, > register_t *r, void *p) > { > - pci_sbdf_t sbdf; > + struct pci_host_bridge *bridge = p; > + pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa); > /* data is needed to prevent a pointer cast on 32bit */ > unsigned long data; > > - /* We ignore segment part and always handle segment 0 */ > - sbdf.sbdf = VPCI_ECAM_BDF(info->gpa - GUEST_VPCI_ECAM_BASE); > - > if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa), > 1U << info->dabt.size, &data) ) > { > @@ -41,10 +56,8 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, > 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 = VPCI_ECAM_BDF(info->gpa - GUEST_VPCI_ECAM_BASE); > + struct pci_host_bridge *bridge = p; > + pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa); > > return vpci_ecam_write(sbdf, ECAM_REG_OFFSET(info->gpa), > 1U << info->dabt.size, r); > @@ -55,17 +68,54 @@ static const struct mmio_handler_ops vpci_mmio_handler = { > .write = vpci_mmio_write, > }; > > +static int vpci_setup_mmio_handler_cb(struct domain *d, > + struct pci_host_bridge *bridge) > +{ > + struct pci_config_window *cfg = bridge->cfg; > + > + register_mmio_handler(d, &vpci_mmio_handler, > + cfg->phys_addr, cfg->size, bridge); > + return 0; > +} > + > int domain_vpci_init(struct domain *d) > { > if ( !has_vpci(d) ) > return 0; > > + if ( is_hardware_domain(d) ) > + return pci_host_iterate_bridges(d, vpci_setup_mmio_handler_cb); > + > + /* Guest domains use what is programmed in their device tree. */ I would rather not make the assumption that the guest is using a Device-Tree. So how about: /* * The hardware domain gets one virtual hostbridge by "real" * hostbridges. * Guests get the virtual platform layout (one virtual host bridge for * now). */ The comment would have to be moved before if ( is_hardware_domain(d) ). > register_mmio_handler(d, &vpci_mmio_handler, > GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL); > > return 0; > } > > +unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d) AFAICT, this function would also be called even if vPCI is not enabled for the domain. So we should add: if ( !has_vpci(d) ) return 0; > +{ > + unsigned int count; > + > + if ( is_hardware_domain(d) ) > + /* For each PCI host bridge's configuration space. */ > + count = pci_host_get_num_bridges(); This first part makes sense to me. But... > + else ... I don't understand how the else is related to this commit. Can you clarify it? > + /* > + * There's a single MSI-X MMIO handler that deals with both PBA > + * and MSI-X tables per each PCI device being passed through. > + * Maximum number of supported devices is 32 as virtual bus > + * topology emulates the devices as embedded endpoints. > + * +1 for a single emulated host bridge's configuration space. > + */ > + count = 1; > +#ifdef CONFIG_HAS_PCI_MSI > + count += 32; Surely, this is a decision that is based on other factor in the vPCI code. So can use a define and avoid hardcoding the number? > +#endif > + > + return count; > +} > + > /* > * Local variables: > * mode: C > diff --git a/xen/arch/arm/vpci.h b/xen/arch/arm/vpci.h > index d8a7b0e3e802..3c713f3fcdb5 100644 > --- a/xen/arch/arm/vpci.h > +++ b/xen/arch/arm/vpci.h > @@ -17,11 +17,17 @@ > > #ifdef CONFIG_HAS_VPCI > int domain_vpci_init(struct domain *d); > +unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d); > #else > static inline int domain_vpci_init(struct domain *d) > { > return 0; > } > + > +static inline unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d) > +{ > + return 0; > +} > #endif > > #endif /* __ARCH_ARM_VPCI_H__ */ > diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h > index c20eba643d86..969333043431 100644 > --- a/xen/include/asm-arm/pci.h > +++ b/xen/include/asm-arm/pci.h > @@ -110,6 +110,11 @@ void arch_pci_init_pdev(struct pci_dev *pdev); > > int pci_get_new_domain_nr(void); > > +int pci_host_iterate_bridges(struct domain *d, > + int (*clb)(struct domain *d, NIT: This is more common to call a callback 'cb'. In any case, I would prefer if the names matches the one used in the implementation. > + struct pci_host_bridge *bridge)); > +unsigned int pci_host_get_num_bridges(void); > + > #else /*!CONFIG_HAS_PCI*/ > > struct arch_pci_dev { }; > Cheers,
Hi, Julien! On 16.11.21 21:12, Julien Grall wrote: > Hi Oleksandr, > > On 05/11/2021 06:33, Oleksandr Andrushchenko wrote: >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> >> In order for vPCI to work it needs to maintain guest and hardware >> domain's views of the configuration space. For example, BARs and >> COMMAND registers require emulation for guests and the guest view >> of the registers needs to be in sync with the real contents of the >> relevant registers. For that ECAM address space needs to also be >> trapped for the hardware domain, so we need to implement PCI host >> bridge specific callbacks to properly setup MMIO handlers for those >> ranges depending on particular host bridge implementation. >> >> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> --- >> Since v5: >> - add vpci_sbdf_from_gpa helper for gpa to SBDF translation >> - take bridge's bus start into account while calculating SBDF >> Since v4: >> - unsigned int for functions working with count >> - gate number of MMIO handlers needed for CONFIG_HAS_PCI_MSI >> and fix their number, e.g. single handler for PBA and >> MSI-X tables (Roger) >> - re-work code for assigning MMIO handlers to be simpler >> and account on the fact that there could multiple host bridges >> exist for the hwdom >> Since v3: >> - fixed comment formatting >> Since v2: >> - removed unneeded assignment (count = 0) >> - removed unneeded header inclusion >> - update commit message >> Since v1: >> - Dynamically calculate the number of MMIO handlers required for vPCI >> and update the total number accordingly >> - s/clb/cb >> - Do not introduce a new callback for MMIO handler setup >> --- >> xen/arch/arm/domain.c | 2 + >> xen/arch/arm/pci/pci-host-common.c | 27 ++++++++++++ >> xen/arch/arm/vpci.c | 66 ++++++++++++++++++++++++++---- >> xen/arch/arm/vpci.h | 6 +++ >> xen/include/asm-arm/pci.h | 5 +++ >> 5 files changed, 98 insertions(+), 8 deletions(-) >> >> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c >> index 96e1b235501d..92a6c509e5c5 100644 >> --- a/xen/arch/arm/domain.c >> +++ b/xen/arch/arm/domain.c >> @@ -739,6 +739,8 @@ int arch_domain_create(struct domain *d, >> if ( (rc = domain_vgic_register(d, &count)) != 0 ) >> goto fail; >> + count += domain_vpci_get_num_mmio_handlers(d); >> + >> if ( (rc = domain_io_init(d, count + MAX_IO_HANDLER)) != 0 ) >> goto fail; >> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c >> index 47104b22b221..0d271a6e8881 100644 >> --- a/xen/arch/arm/pci/pci-host-common.c >> +++ b/xen/arch/arm/pci/pci-host-common.c >> @@ -289,6 +289,33 @@ int pci_get_host_bridge_segment(const struct dt_device_node *node, >> return -EINVAL; >> } >> +int pci_host_iterate_bridges(struct domain *d, >> + int (*cb)(struct domain *d, >> + struct pci_host_bridge *bridge)) >> +{ >> + struct pci_host_bridge *bridge; >> + int err; >> + >> + list_for_each_entry( bridge, &pci_host_bridges, node ) >> + { >> + err = cb(d, bridge); >> + if ( err ) >> + return err; >> + } >> + return 0; >> +} >> + >> +unsigned int pci_host_get_num_bridges(void) >> +{ >> + struct pci_host_bridge *bridge; >> + unsigned int count = 0; > > How about making this static and... > >> + >> + list_for_each_entry( bridge, &pci_host_bridges, node ) >> + count++; > > ... only call list_for_each_entry() when count is -1? So we would only go through the list once. > > This should be fine given hostbridge can only be added during boot (we would need to protect pci_host_bridges with a lock otherwise). Ok, I can do that > >> + >> + return count; >> +} >> + >> /* >> * Local variables: >> * mode: C >> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c >> index 23f45386f4b3..5a6ebd8b9868 100644 >> --- a/xen/arch/arm/vpci.c >> +++ b/xen/arch/arm/vpci.c >> @@ -16,16 +16,31 @@ >> #include <asm/mmio.h> >> +static pci_sbdf_t vpci_sbdf_from_gpa(const struct pci_host_bridge *bridge, >> + paddr_t gpa) >> +{ >> + pci_sbdf_t sbdf; >> + >> + if ( bridge ) >> + { >> + sbdf.sbdf = VPCI_ECAM_BDF(gpa - bridge->cfg->phys_addr); >> + sbdf.seg = bridge->segment; >> + sbdf.bus += bridge->cfg->busn_start; >> + } >> + else >> + sbdf.sbdf = VPCI_ECAM_BDF(gpa - GUEST_VPCI_ECAM_BASE); >> + >> + return sbdf; >> +} >> + >> static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, >> register_t *r, void *p) >> { >> - pci_sbdf_t sbdf; >> + struct pci_host_bridge *bridge = p; >> + pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa); >> /* data is needed to prevent a pointer cast on 32bit */ >> unsigned long data; >> - /* We ignore segment part and always handle segment 0 */ >> - sbdf.sbdf = VPCI_ECAM_BDF(info->gpa - GUEST_VPCI_ECAM_BASE); >> - >> if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa), >> 1U << info->dabt.size, &data) ) >> { >> @@ -41,10 +56,8 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, >> 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 = VPCI_ECAM_BDF(info->gpa - GUEST_VPCI_ECAM_BASE); >> + struct pci_host_bridge *bridge = p; >> + pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa); >> return vpci_ecam_write(sbdf, ECAM_REG_OFFSET(info->gpa), >> 1U << info->dabt.size, r); >> @@ -55,17 +68,54 @@ static const struct mmio_handler_ops vpci_mmio_handler = { >> .write = vpci_mmio_write, >> }; >> +static int vpci_setup_mmio_handler_cb(struct domain *d, >> + struct pci_host_bridge *bridge) >> +{ >> + struct pci_config_window *cfg = bridge->cfg; >> + >> + register_mmio_handler(d, &vpci_mmio_handler, >> + cfg->phys_addr, cfg->size, bridge); >> + return 0; >> +} >> + >> int domain_vpci_init(struct domain *d) >> { >> if ( !has_vpci(d) ) >> return 0; >> + if ( is_hardware_domain(d) ) >> + return pci_host_iterate_bridges(d, vpci_setup_mmio_handler_cb); >> + >> + /* Guest domains use what is programmed in their device tree. */ > > I would rather not make the assumption that the guest is using a Device-Tree. So how about: > > /* > * The hardware domain gets one virtual hostbridge by "real" > * hostbridges. > * Guests get the virtual platform layout (one virtual host bridge for > * now). > */ > > The comment would have to be moved before if ( is_hardware_domain(d) ). Sure, I can extend the comment > >> register_mmio_handler(d, &vpci_mmio_handler, >> GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL); >> return 0; >> } >> +unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d) > > AFAICT, this function would also be called even if vPCI is not enabled for the domain. So we should add: > > if ( !has_vpci(d) ) > return 0; > Good catch, will add >> +{ >> + unsigned int count; >> + >> + if ( is_hardware_domain(d) ) >> + /* For each PCI host bridge's configuration space. */ >> + count = pci_host_get_num_bridges(); > > This first part makes sense to me. But... > >> + else > > ... I don't understand how the else is related to this commit. Can you clarify it? > >> + /* >> + * There's a single MSI-X MMIO handler that deals with both PBA >> + * and MSI-X tables per each PCI device being passed through. >> + * Maximum number of supported devices is 32 as virtual bus >> + * topology emulates the devices as embedded endpoints. >> + * +1 for a single emulated host bridge's configuration space. >> + */ >> + count = 1; >> +#ifdef CONFIG_HAS_PCI_MSI >> + count += 32; > > Surely, this is a decision that is based on other factor in the vPCI code. So can use a define and avoid hardcoding the number? Well, in the later series [1] this is defined via PCI_SLOT(~0) + 1 and there is no dedicated constant for that. I can use the same here, e.g. s/32/PCI_SLOT(~0) + 1 > >> +#endif > > >> + >> + return count; >> +} >> + >> /* >> * Local variables: >> * mode: C >> diff --git a/xen/arch/arm/vpci.h b/xen/arch/arm/vpci.h >> index d8a7b0e3e802..3c713f3fcdb5 100644 >> --- a/xen/arch/arm/vpci.h >> +++ b/xen/arch/arm/vpci.h >> @@ -17,11 +17,17 @@ >> #ifdef CONFIG_HAS_VPCI >> int domain_vpci_init(struct domain *d); >> +unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d); >> #else >> static inline int domain_vpci_init(struct domain *d) >> { >> return 0; >> } >> + >> +static inline unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d) >> +{ >> + return 0; >> +} >> #endif >> #endif /* __ARCH_ARM_VPCI_H__ */ >> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h >> index c20eba643d86..969333043431 100644 >> --- a/xen/include/asm-arm/pci.h >> +++ b/xen/include/asm-arm/pci.h >> @@ -110,6 +110,11 @@ void arch_pci_init_pdev(struct pci_dev *pdev); >> int pci_get_new_domain_nr(void); >> +int pci_host_iterate_bridges(struct domain *d, >> + int (*clb)(struct domain *d, > > NIT: This is more common to call a callback 'cb'. In any case, I would prefer if the names matches the one used in the implementation. Will change > >> + struct pci_host_bridge *bridge)); >> +unsigned int pci_host_get_num_bridges(void); >> + >> #else /*!CONFIG_HAS_PCI*/ >> struct arch_pci_dev { }; >> > > Cheers, > Thank you, Oleksandr [1] https://patchwork.kernel.org/project/xen-devel/patch/20211105065629.940943-11-andr2000@gmail.com/
On 18.11.21 09:27, Oleksandr Andrushchenko wrote: > Hi, Julien! > > On 16.11.21 21:12, Julien Grall wrote: >> Hi Oleksandr, >> >> On 05/11/2021 06:33, Oleksandr Andrushchenko wrote: >>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >>> >>> In order for vPCI to work it needs to maintain guest and hardware >>> domain's views of the configuration space. For example, BARs and >>> COMMAND registers require emulation for guests and the guest view >>> of the registers needs to be in sync with the real contents of the >>> relevant registers. For that ECAM address space needs to also be >>> trapped for the hardware domain, so we need to implement PCI host >>> bridge specific callbacks to properly setup MMIO handlers for those >>> ranges depending on particular host bridge implementation. >>> >>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >>> --- >>> Since v5: >>> - add vpci_sbdf_from_gpa helper for gpa to SBDF translation >>> - take bridge's bus start into account while calculating SBDF >>> Since v4: >>> - unsigned int for functions working with count >>> - gate number of MMIO handlers needed for CONFIG_HAS_PCI_MSI >>> and fix their number, e.g. single handler for PBA and >>> MSI-X tables (Roger) >>> - re-work code for assigning MMIO handlers to be simpler >>> and account on the fact that there could multiple host bridges >>> exist for the hwdom >>> Since v3: >>> - fixed comment formatting >>> Since v2: >>> - removed unneeded assignment (count = 0) >>> - removed unneeded header inclusion >>> - update commit message >>> Since v1: >>> - Dynamically calculate the number of MMIO handlers required for vPCI >>> and update the total number accordingly >>> - s/clb/cb >>> - Do not introduce a new callback for MMIO handler setup >>> --- >>> xen/arch/arm/domain.c | 2 + >>> xen/arch/arm/pci/pci-host-common.c | 27 ++++++++++++ >>> xen/arch/arm/vpci.c | 66 ++++++++++++++++++++++++++---- >>> xen/arch/arm/vpci.h | 6 +++ >>> xen/include/asm-arm/pci.h | 5 +++ >>> 5 files changed, 98 insertions(+), 8 deletions(-) >>> >>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c >>> index 96e1b235501d..92a6c509e5c5 100644 >>> --- a/xen/arch/arm/domain.c >>> +++ b/xen/arch/arm/domain.c >>> @@ -739,6 +739,8 @@ int arch_domain_create(struct domain *d, >>> if ( (rc = domain_vgic_register(d, &count)) != 0 ) >>> goto fail; >>> + count += domain_vpci_get_num_mmio_handlers(d); >>> + >>> if ( (rc = domain_io_init(d, count + MAX_IO_HANDLER)) != 0 ) >>> goto fail; >>> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c >>> index 47104b22b221..0d271a6e8881 100644 >>> --- a/xen/arch/arm/pci/pci-host-common.c >>> +++ b/xen/arch/arm/pci/pci-host-common.c >>> @@ -289,6 +289,33 @@ int pci_get_host_bridge_segment(const struct dt_device_node *node, >>> return -EINVAL; >>> } >>> +int pci_host_iterate_bridges(struct domain *d, >>> + int (*cb)(struct domain *d, >>> + struct pci_host_bridge *bridge)) >>> +{ >>> + struct pci_host_bridge *bridge; >>> + int err; >>> + >>> + list_for_each_entry( bridge, &pci_host_bridges, node ) >>> + { >>> + err = cb(d, bridge); >>> + if ( err ) >>> + return err; >>> + } >>> + return 0; >>> +} >>> + >>> +unsigned int pci_host_get_num_bridges(void) >>> +{ >>> + struct pci_host_bridge *bridge; >>> + unsigned int count = 0; >> How about making this static and... >> >>> + >>> + list_for_each_entry( bridge, &pci_host_bridges, node ) >>> + count++; >> ... only call list_for_each_entry() when count is -1? So we would only go through the list once. >> >> This should be fine given hostbridge can only be added during boot (we would need to protect pci_host_bridges with a lock otherwise). > Ok, I can do that I have re-worked the patch so that more code can be re-used: if ( is_hardware_domain(d) ) { int count; count = pci_host_iterate_bridges_and_count(d, vpci_setup_mmio_handler_cb); if ( count < 0 ) return count; return 0; } register_mmio_handler(d, &vpci_mmio_handler, GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL); so pci_host_get_num_bridges goes away. >>> + >>> + return count; >>> +} >>> + >>> /* >>> * Local variables: >>> * mode: C >>> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c >>> index 23f45386f4b3..5a6ebd8b9868 100644 >>> --- a/xen/arch/arm/vpci.c >>> +++ b/xen/arch/arm/vpci.c >>> @@ -16,16 +16,31 @@ >>> #include <asm/mmio.h> >>> +static pci_sbdf_t vpci_sbdf_from_gpa(const struct pci_host_bridge *bridge, >>> + paddr_t gpa) >>> +{ >>> + pci_sbdf_t sbdf; >>> + >>> + if ( bridge ) >>> + { >>> + sbdf.sbdf = VPCI_ECAM_BDF(gpa - bridge->cfg->phys_addr); >>> + sbdf.seg = bridge->segment; >>> + sbdf.bus += bridge->cfg->busn_start; >>> + } >>> + else >>> + sbdf.sbdf = VPCI_ECAM_BDF(gpa - GUEST_VPCI_ECAM_BASE); >>> + >>> + return sbdf; >>> +} >>> + >>> static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, >>> register_t *r, void *p) >>> { >>> - pci_sbdf_t sbdf; >>> + struct pci_host_bridge *bridge = p; >>> + pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa); >>> /* data is needed to prevent a pointer cast on 32bit */ >>> unsigned long data; >>> - /* We ignore segment part and always handle segment 0 */ >>> - sbdf.sbdf = VPCI_ECAM_BDF(info->gpa - GUEST_VPCI_ECAM_BASE); >>> - >>> if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa), >>> 1U << info->dabt.size, &data) ) >>> { >>> @@ -41,10 +56,8 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, >>> 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 = VPCI_ECAM_BDF(info->gpa - GUEST_VPCI_ECAM_BASE); >>> + struct pci_host_bridge *bridge = p; >>> + pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa); >>> return vpci_ecam_write(sbdf, ECAM_REG_OFFSET(info->gpa), >>> 1U << info->dabt.size, r); >>> @@ -55,17 +68,54 @@ static const struct mmio_handler_ops vpci_mmio_handler = { >>> .write = vpci_mmio_write, >>> }; >>> +static int vpci_setup_mmio_handler_cb(struct domain *d, >>> + struct pci_host_bridge *bridge) >>> +{ >>> + struct pci_config_window *cfg = bridge->cfg; >>> + >>> + register_mmio_handler(d, &vpci_mmio_handler, >>> + cfg->phys_addr, cfg->size, bridge); >>> + return 0; >>> +} >>> + >>> int domain_vpci_init(struct domain *d) >>> { >>> if ( !has_vpci(d) ) >>> return 0; >>> + if ( is_hardware_domain(d) ) >>> + return pci_host_iterate_bridges(d, vpci_setup_mmio_handler_cb); >>> + >>> + /* Guest domains use what is programmed in their device tree. */ >> I would rather not make the assumption that the guest is using a Device-Tree. So how about: >> >> /* >> * The hardware domain gets one virtual hostbridge by "real" >> * hostbridges. >> * Guests get the virtual platform layout (one virtual host bridge for >> * now). >> */ >> >> The comment would have to be moved before if ( is_hardware_domain(d) ). > Sure, I can extend the comment /* * The hardware domain gets as many MMIOs as required by the * physical host bridge. * Guests get the virtual platform layout: one virtual host bridge for now. */ >>> register_mmio_handler(d, &vpci_mmio_handler, >>> GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL); >>> return 0; >>> } >>> +unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d) >> AFAICT, this function would also be called even if vPCI is not enabled for the domain. So we should add: >> >> if ( !has_vpci(d) ) >> return 0; >> > Good catch, will add Hm... but we have static inline unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d) +{ + return 0; +} fir that case >>> +{ >>> + unsigned int count; >>> + >>> + if ( is_hardware_domain(d) ) >>> + /* For each PCI host bridge's configuration space. */ >>> + count = pci_host_get_num_bridges(); >> This first part makes sense to me. But... >> >>> + else >> ... I don't understand how the else is related to this commit. Can you clarify it? >> >>> + /* >>> + * There's a single MSI-X MMIO handler that deals with both PBA >>> + * and MSI-X tables per each PCI device being passed through. >>> + * Maximum number of supported devices is 32 as virtual bus >>> + * topology emulates the devices as embedded endpoints. >>> + * +1 for a single emulated host bridge's configuration space. >>> + */ >>> + count = 1; >>> +#ifdef CONFIG_HAS_PCI_MSI >>> + count += 32; >> Surely, this is a decision that is based on other factor in the vPCI code. So can use a define and avoid hardcoding the number? > Well, in the later series [1] this is defined via PCI_SLOT(~0) + 1 and there is no dedicated > constant for that. I can use the same here, e.g. s/32/PCI_SLOT(~0) + 1 >>> +#endif >> >>> + >>> + return count; >>> +} >>> + >>> /* >>> * Local variables: >>> * mode: C >>> diff --git a/xen/arch/arm/vpci.h b/xen/arch/arm/vpci.h >>> index d8a7b0e3e802..3c713f3fcdb5 100644 >>> --- a/xen/arch/arm/vpci.h >>> +++ b/xen/arch/arm/vpci.h >>> @@ -17,11 +17,17 @@ >>> #ifdef CONFIG_HAS_VPCI >>> int domain_vpci_init(struct domain *d); >>> +unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d); >>> #else >>> static inline int domain_vpci_init(struct domain *d) >>> { >>> return 0; >>> } >>> + >>> +static inline unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d) >>> +{ >>> + return 0; >>> +} >>> #endif >>> #endif /* __ARCH_ARM_VPCI_H__ */ >>> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h >>> index c20eba643d86..969333043431 100644 >>> --- a/xen/include/asm-arm/pci.h >>> +++ b/xen/include/asm-arm/pci.h >>> @@ -110,6 +110,11 @@ void arch_pci_init_pdev(struct pci_dev *pdev); >>> int pci_get_new_domain_nr(void); >>> +int pci_host_iterate_bridges(struct domain *d, >>> + int (*clb)(struct domain *d, >> NIT: This is more common to call a callback 'cb'. In any case, I would prefer if the names matches the one used in the implementation. > Will change >>> + struct pci_host_bridge *bridge)); >>> +unsigned int pci_host_get_num_bridges(void); >>> + >>> #else /*!CONFIG_HAS_PCI*/ >>> struct arch_pci_dev { }; >>> >> Cheers, >> > Thank you, > Oleksandr > > [1] https://patchwork.kernel.org/project/xen-devel/patch/20211105065629.940943-11-andr2000@gmail.com/
Hi, On 18/11/2021 10:46, Oleksandr Andrushchenko wrote: > On 18.11.21 09:27, Oleksandr Andrushchenko wrote: >> Hi, Julien! >> >> On 16.11.21 21:12, Julien Grall wrote: >>> Hi Oleksandr, >>> >>> On 05/11/2021 06:33, Oleksandr Andrushchenko wrote: >>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >>>> >>>> In order for vPCI to work it needs to maintain guest and hardware >>>> domain's views of the configuration space. For example, BARs and >>>> COMMAND registers require emulation for guests and the guest view >>>> of the registers needs to be in sync with the real contents of the >>>> relevant registers. For that ECAM address space needs to also be >>>> trapped for the hardware domain, so we need to implement PCI host >>>> bridge specific callbacks to properly setup MMIO handlers for those >>>> ranges depending on particular host bridge implementation. >>>> >>>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >>>> --- >>>> Since v5: >>>> - add vpci_sbdf_from_gpa helper for gpa to SBDF translation >>>> - take bridge's bus start into account while calculating SBDF >>>> Since v4: >>>> - unsigned int for functions working with count >>>> - gate number of MMIO handlers needed for CONFIG_HAS_PCI_MSI >>>> and fix their number, e.g. single handler for PBA and >>>> MSI-X tables (Roger) >>>> - re-work code for assigning MMIO handlers to be simpler >>>> and account on the fact that there could multiple host bridges >>>> exist for the hwdom >>>> Since v3: >>>> - fixed comment formatting >>>> Since v2: >>>> - removed unneeded assignment (count = 0) >>>> - removed unneeded header inclusion >>>> - update commit message >>>> Since v1: >>>> - Dynamically calculate the number of MMIO handlers required for vPCI >>>> and update the total number accordingly >>>> - s/clb/cb >>>> - Do not introduce a new callback for MMIO handler setup >>>> --- >>>> xen/arch/arm/domain.c | 2 + >>>> xen/arch/arm/pci/pci-host-common.c | 27 ++++++++++++ >>>> xen/arch/arm/vpci.c | 66 ++++++++++++++++++++++++++---- >>>> xen/arch/arm/vpci.h | 6 +++ >>>> xen/include/asm-arm/pci.h | 5 +++ >>>> 5 files changed, 98 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c >>>> index 96e1b235501d..92a6c509e5c5 100644 >>>> --- a/xen/arch/arm/domain.c >>>> +++ b/xen/arch/arm/domain.c >>>> @@ -739,6 +739,8 @@ int arch_domain_create(struct domain *d, >>>> if ( (rc = domain_vgic_register(d, &count)) != 0 ) >>>> goto fail; >>>> + count += domain_vpci_get_num_mmio_handlers(d); >>>> + >>>> if ( (rc = domain_io_init(d, count + MAX_IO_HANDLER)) != 0 ) >>>> goto fail; >>>> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c >>>> index 47104b22b221..0d271a6e8881 100644 >>>> --- a/xen/arch/arm/pci/pci-host-common.c >>>> +++ b/xen/arch/arm/pci/pci-host-common.c >>>> @@ -289,6 +289,33 @@ int pci_get_host_bridge_segment(const struct dt_device_node *node, >>>> return -EINVAL; >>>> } >>>> +int pci_host_iterate_bridges(struct domain *d, >>>> + int (*cb)(struct domain *d, >>>> + struct pci_host_bridge *bridge)) >>>> +{ >>>> + struct pci_host_bridge *bridge; >>>> + int err; >>>> + >>>> + list_for_each_entry( bridge, &pci_host_bridges, node ) >>>> + { >>>> + err = cb(d, bridge); >>>> + if ( err ) >>>> + return err; >>>> + } >>>> + return 0; >>>> +} >>>> + >>>> +unsigned int pci_host_get_num_bridges(void) >>>> +{ >>>> + struct pci_host_bridge *bridge; >>>> + unsigned int count = 0; >>> How about making this static and... >>> >>>> + >>>> + list_for_each_entry( bridge, &pci_host_bridges, node ) >>>> + count++; >>> ... only call list_for_each_entry() when count is -1? So we would only go through the list once. >>> >>> This should be fine given hostbridge can only be added during boot (we would need to protect pci_host_bridges with a lock otherwise). >> Ok, I can do that > I have re-worked the patch so that more code can be re-used: > > if ( is_hardware_domain(d) ) > { > int count; > > count = pci_host_iterate_bridges_and_count(d, > vpci_setup_mmio_handler_cb); > if ( count < 0 ) > return count; > > return 0; > } > > register_mmio_handler(d, &vpci_mmio_handler, > GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL); > > so pci_host_get_num_bridges goes away. If this will be the only caller that needs to know the number hostbridges, then I am happy with this appropach. Otherwise, I would prefer to keep the helper pci_host_get_num_bridges(). >>>> + >>>> + return count; >>>> +} >>>> + >>>> /* >>>> * Local variables: >>>> * mode: C >>>> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c >>>> index 23f45386f4b3..5a6ebd8b9868 100644 >>>> --- a/xen/arch/arm/vpci.c >>>> +++ b/xen/arch/arm/vpci.c >>>> @@ -16,16 +16,31 @@ >>>> #include <asm/mmio.h> >>>> +static pci_sbdf_t vpci_sbdf_from_gpa(const struct pci_host_bridge *bridge, >>>> + paddr_t gpa) >>>> +{ >>>> + pci_sbdf_t sbdf; >>>> + >>>> + if ( bridge ) >>>> + { >>>> + sbdf.sbdf = VPCI_ECAM_BDF(gpa - bridge->cfg->phys_addr); >>>> + sbdf.seg = bridge->segment; >>>> + sbdf.bus += bridge->cfg->busn_start; >>>> + } >>>> + else >>>> + sbdf.sbdf = VPCI_ECAM_BDF(gpa - GUEST_VPCI_ECAM_BASE); >>>> + >>>> + return sbdf; >>>> +} >>>> + >>>> static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, >>>> register_t *r, void *p) >>>> { >>>> - pci_sbdf_t sbdf; >>>> + struct pci_host_bridge *bridge = p; >>>> + pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa); >>>> /* data is needed to prevent a pointer cast on 32bit */ >>>> unsigned long data; >>>> - /* We ignore segment part and always handle segment 0 */ >>>> - sbdf.sbdf = VPCI_ECAM_BDF(info->gpa - GUEST_VPCI_ECAM_BASE); >>>> - >>>> if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa), >>>> 1U << info->dabt.size, &data) ) >>>> { >>>> @@ -41,10 +56,8 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, >>>> 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 = VPCI_ECAM_BDF(info->gpa - GUEST_VPCI_ECAM_BASE); >>>> + struct pci_host_bridge *bridge = p; >>>> + pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa); >>>> return vpci_ecam_write(sbdf, ECAM_REG_OFFSET(info->gpa), >>>> 1U << info->dabt.size, r); >>>> @@ -55,17 +68,54 @@ static const struct mmio_handler_ops vpci_mmio_handler = { >>>> .write = vpci_mmio_write, >>>> }; >>>> +static int vpci_setup_mmio_handler_cb(struct domain *d, >>>> + struct pci_host_bridge *bridge) >>>> +{ >>>> + struct pci_config_window *cfg = bridge->cfg; >>>> + >>>> + register_mmio_handler(d, &vpci_mmio_handler, >>>> + cfg->phys_addr, cfg->size, bridge); >>>> + return 0; >>>> +} >>>> + >>>> int domain_vpci_init(struct domain *d) >>>> { >>>> if ( !has_vpci(d) ) >>>> return 0; >>>> + if ( is_hardware_domain(d) ) >>>> + return pci_host_iterate_bridges(d, vpci_setup_mmio_handler_cb); >>>> + >>>> + /* Guest domains use what is programmed in their device tree. */ >>> I would rather not make the assumption that the guest is using a Device-Tree. So how about: >>> >>> /* >>> * The hardware domain gets one virtual hostbridge by "real" >>> * hostbridges. >>> * Guests get the virtual platform layout (one virtual host bridge for >>> * now). >>> */ >>> >>> The comment would have to be moved before if ( is_hardware_domain(d) ). >> Sure, I can extend the comment > /* > * The hardware domain gets as many MMIOs as required by the > * physical host bridge. > * Guests get the virtual platform layout: one virtual host bridge for now. > */ LGTM. > >>>> register_mmio_handler(d, &vpci_mmio_handler, >>>> GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL); >>>> return 0; >>>> } >>>> +unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d) >>> AFAICT, this function would also be called even if vPCI is not enabled for the domain. So we should add: >>> >>> if ( !has_vpci(d) ) >>> return 0; >>> >> Good catch, will add > Hm... but we have > > static inline unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d) > +{ > + return 0; > +} > fir that case This would only cover the case where Xen was built without vPCI support. When Xen is built with vPCI support, we only want to increase the number of regions for domain with vPCI enabled. > >>>> +{ >>>> + unsigned int count; >>>> + >>>> + if ( is_hardware_domain(d) ) >>>> + /* For each PCI host bridge's configuration space. */ >>>> + count = pci_host_get_num_bridges(); >>> This first part makes sense to me. But... >>> >>>> + else >>> ... I don't understand how the else is related to this commit. Can you clarify it? >>> >>>> + /* >>>> + * There's a single MSI-X MMIO handler that deals with both PBA >>>> + * and MSI-X tables per each PCI device being passed through. >>>> + * Maximum number of supported devices is 32 as virtual bus >>>> + * topology emulates the devices as embedded endpoints. >>>> + * +1 for a single emulated host bridge's configuration space. >>>> + */ >>>> + count = 1; >>>> +#ifdef CONFIG_HAS_PCI_MSI >>>> + count += 32; >>> Surely, this is a decision that is based on other factor in the vPCI code. So can use a define and avoid hardcoding the number? >> Well, in the later series [1] this is defined via PCI_SLOT(~0) + 1 and there is no dedicated >> constant for that. I can use the same here, e.g. s/32/PCI_SLOT(~0) + 1 I would prefer if we introduce a new constant for that. This makes easier to update the code if we decide to increase the number of virtual devices. However, I am still not sure how the 'else' part is related to this commit. Can you please clarify it? Cheers,
Hi, Julien! On 22.11.21 19:36, Julien Grall wrote: > Hi, > > On 18/11/2021 10:46, Oleksandr Andrushchenko wrote: >> On 18.11.21 09:27, Oleksandr Andrushchenko wrote: >>> Hi, Julien! >>> >>> On 16.11.21 21:12, Julien Grall wrote: >>>> Hi Oleksandr, >>>> >>>> On 05/11/2021 06:33, Oleksandr Andrushchenko wrote: >>>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >>>>> >>>>> In order for vPCI to work it needs to maintain guest and hardware >>>>> domain's views of the configuration space. For example, BARs and >>>>> COMMAND registers require emulation for guests and the guest view >>>>> of the registers needs to be in sync with the real contents of the >>>>> relevant registers. For that ECAM address space needs to also be >>>>> trapped for the hardware domain, so we need to implement PCI host >>>>> bridge specific callbacks to properly setup MMIO handlers for those >>>>> ranges depending on particular host bridge implementation. >>>>> >>>>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >>>>> --- >>>>> Since v5: >>>>> - add vpci_sbdf_from_gpa helper for gpa to SBDF translation >>>>> - take bridge's bus start into account while calculating SBDF >>>>> Since v4: >>>>> - unsigned int for functions working with count >>>>> - gate number of MMIO handlers needed for CONFIG_HAS_PCI_MSI >>>>> and fix their number, e.g. single handler for PBA and >>>>> MSI-X tables (Roger) >>>>> - re-work code for assigning MMIO handlers to be simpler >>>>> and account on the fact that there could multiple host bridges >>>>> exist for the hwdom >>>>> Since v3: >>>>> - fixed comment formatting >>>>> Since v2: >>>>> - removed unneeded assignment (count = 0) >>>>> - removed unneeded header inclusion >>>>> - update commit message >>>>> Since v1: >>>>> - Dynamically calculate the number of MMIO handlers required for vPCI >>>>> and update the total number accordingly >>>>> - s/clb/cb >>>>> - Do not introduce a new callback for MMIO handler setup >>>>> --- >>>>> xen/arch/arm/domain.c | 2 + >>>>> xen/arch/arm/pci/pci-host-common.c | 27 ++++++++++++ >>>>> xen/arch/arm/vpci.c | 66 ++++++++++++++++++++++++++---- >>>>> xen/arch/arm/vpci.h | 6 +++ >>>>> xen/include/asm-arm/pci.h | 5 +++ >>>>> 5 files changed, 98 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c >>>>> index 96e1b235501d..92a6c509e5c5 100644 >>>>> --- a/xen/arch/arm/domain.c >>>>> +++ b/xen/arch/arm/domain.c >>>>> @@ -739,6 +739,8 @@ int arch_domain_create(struct domain *d, >>>>> if ( (rc = domain_vgic_register(d, &count)) != 0 ) >>>>> goto fail; >>>>> + count += domain_vpci_get_num_mmio_handlers(d); >>>>> + >>>>> if ( (rc = domain_io_init(d, count + MAX_IO_HANDLER)) != 0 ) >>>>> goto fail; >>>>> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c >>>>> index 47104b22b221..0d271a6e8881 100644 >>>>> --- a/xen/arch/arm/pci/pci-host-common.c >>>>> +++ b/xen/arch/arm/pci/pci-host-common.c >>>>> @@ -289,6 +289,33 @@ int pci_get_host_bridge_segment(const struct dt_device_node *node, >>>>> return -EINVAL; >>>>> } >>>>> +int pci_host_iterate_bridges(struct domain *d, >>>>> + int (*cb)(struct domain *d, >>>>> + struct pci_host_bridge *bridge)) >>>>> +{ >>>>> + struct pci_host_bridge *bridge; >>>>> + int err; >>>>> + >>>>> + list_for_each_entry( bridge, &pci_host_bridges, node ) >>>>> + { >>>>> + err = cb(d, bridge); >>>>> + if ( err ) >>>>> + return err; >>>>> + } >>>>> + return 0; >>>>> +} >>>>> + >>>>> +unsigned int pci_host_get_num_bridges(void) >>>>> +{ >>>>> + struct pci_host_bridge *bridge; >>>>> + unsigned int count = 0; >>>> How about making this static and... >>>> >>>>> + >>>>> + list_for_each_entry( bridge, &pci_host_bridges, node ) >>>>> + count++; >>>> ... only call list_for_each_entry() when count is -1? So we would only go through the list once. >>>> >>>> This should be fine given hostbridge can only be added during boot (we would need to protect pci_host_bridges with a lock otherwise). >>> Ok, I can do that >> I have re-worked the patch so that more code can be re-used: >> >> if ( is_hardware_domain(d) ) >> { >> int count; >> >> count = pci_host_iterate_bridges_and_count(d, >> vpci_setup_mmio_handler_cb); >> if ( count < 0 ) >> return count; >> >> return 0; >> } >> >> register_mmio_handler(d, &vpci_mmio_handler, >> GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL); >> >> so pci_host_get_num_bridges goes away. > > If this will be the only caller that needs to know the number hostbridges, then I am happy with this appropach. Otherwise, I would prefer to keep the helper pci_host_get_num_bridges(). pci_host_get_num_bridges won't be needed, so it is ok to not introduce pci_host_get_num_bridges > >>>>> + >>>>> + return count; >>>>> +} >>>>> + >>>>> /* >>>>> * Local variables: >>>>> * mode: C >>>>> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c >>>>> index 23f45386f4b3..5a6ebd8b9868 100644 >>>>> --- a/xen/arch/arm/vpci.c >>>>> +++ b/xen/arch/arm/vpci.c >>>>> @@ -16,16 +16,31 @@ >>>>> #include <asm/mmio.h> >>>>> +static pci_sbdf_t vpci_sbdf_from_gpa(const struct pci_host_bridge *bridge, >>>>> + paddr_t gpa) >>>>> +{ >>>>> + pci_sbdf_t sbdf; >>>>> + >>>>> + if ( bridge ) >>>>> + { >>>>> + sbdf.sbdf = VPCI_ECAM_BDF(gpa - bridge->cfg->phys_addr); >>>>> + sbdf.seg = bridge->segment; >>>>> + sbdf.bus += bridge->cfg->busn_start; >>>>> + } >>>>> + else >>>>> + sbdf.sbdf = VPCI_ECAM_BDF(gpa - GUEST_VPCI_ECAM_BASE); >>>>> + >>>>> + return sbdf; >>>>> +} >>>>> + >>>>> static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, >>>>> register_t *r, void *p) >>>>> { >>>>> - pci_sbdf_t sbdf; >>>>> + struct pci_host_bridge *bridge = p; >>>>> + pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa); >>>>> /* data is needed to prevent a pointer cast on 32bit */ >>>>> unsigned long data; >>>>> - /* We ignore segment part and always handle segment 0 */ >>>>> - sbdf.sbdf = VPCI_ECAM_BDF(info->gpa - GUEST_VPCI_ECAM_BASE); >>>>> - >>>>> if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa), >>>>> 1U << info->dabt.size, &data) ) >>>>> { >>>>> @@ -41,10 +56,8 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, >>>>> 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 = VPCI_ECAM_BDF(info->gpa - GUEST_VPCI_ECAM_BASE); >>>>> + struct pci_host_bridge *bridge = p; >>>>> + pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa); >>>>> return vpci_ecam_write(sbdf, ECAM_REG_OFFSET(info->gpa), >>>>> 1U << info->dabt.size, r); >>>>> @@ -55,17 +68,54 @@ static const struct mmio_handler_ops vpci_mmio_handler = { >>>>> .write = vpci_mmio_write, >>>>> }; >>>>> +static int vpci_setup_mmio_handler_cb(struct domain *d, >>>>> + struct pci_host_bridge *bridge) >>>>> +{ >>>>> + struct pci_config_window *cfg = bridge->cfg; >>>>> + >>>>> + register_mmio_handler(d, &vpci_mmio_handler, >>>>> + cfg->phys_addr, cfg->size, bridge); >>>>> + return 0; >>>>> +} >>>>> + >>>>> int domain_vpci_init(struct domain *d) >>>>> { >>>>> if ( !has_vpci(d) ) >>>>> return 0; >>>>> + if ( is_hardware_domain(d) ) >>>>> + return pci_host_iterate_bridges(d, vpci_setup_mmio_handler_cb); >>>>> + >>>>> + /* Guest domains use what is programmed in their device tree. */ >>>> I would rather not make the assumption that the guest is using a Device-Tree. So how about: >>>> >>>> /* >>>> * The hardware domain gets one virtual hostbridge by "real" >>>> * hostbridges. >>>> * Guests get the virtual platform layout (one virtual host bridge for >>>> * now). >>>> */ >>>> >>>> The comment would have to be moved before if ( is_hardware_domain(d) ). >>> Sure, I can extend the comment >> /* >> * The hardware domain gets as many MMIOs as required by the >> * physical host bridge. >> * Guests get the virtual platform layout: one virtual host bridge for now. >> */ > > LGTM. > >> >>>>> register_mmio_handler(d, &vpci_mmio_handler, >>>>> GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL); >>>>> return 0; >>>>> } >>>>> +unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d) >>>> AFAICT, this function would also be called even if vPCI is not enabled for the domain. So we should add: >>>> >>>> if ( !has_vpci(d) ) >>>> return 0; >>>> >>> Good catch, will add >> Hm... but we have >> >> static inline unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d) >> +{ >> + return 0; >> +} >> fir that case > > This would only cover the case where Xen was built without vPCI support. When Xen is built with vPCI support, we only want to increase the number of regions for domain with vPCI enabled. Yes, you are right, I will add the check > >> >>>>> +{ >>>>> + unsigned int count; >>>>> + >>>>> + if ( is_hardware_domain(d) ) >>>>> + /* For each PCI host bridge's configuration space. */ >>>>> + count = pci_host_get_num_bridges(); >>>> This first part makes sense to me. But... >>>> >>>>> + else >>>> ... I don't understand how the else is related to this commit. Can you clarify it? >>>> >>>>> + /* >>>>> + * There's a single MSI-X MMIO handler that deals with both PBA >>>>> + * and MSI-X tables per each PCI device being passed through. >>>>> + * Maximum number of supported devices is 32 as virtual bus >>>>> + * topology emulates the devices as embedded endpoints. >>>>> + * +1 for a single emulated host bridge's configuration space. >>>>> + */ >>>>> + count = 1; >>>>> +#ifdef CONFIG_HAS_PCI_MSI >>>>> + count += 32; >>>> Surely, this is a decision that is based on other factor in the vPCI code. So can use a define and avoid hardcoding the number? >>> Well, in the later series [1] this is defined via PCI_SLOT(~0) + 1 and there is no dedicated >>> constant for that. I can use the same here, e.g. s/32/PCI_SLOT(~0) + 1 > > I would prefer if we introduce a new constant for that. This makes easier to update the code if we decide to increase the number of virtual devices. > > However, I am still not sure how the 'else' part is related to this commit. Can you please clarify it? Well, yes, this is too early for this patch to introduce some future knowledge, so I'll instead have: unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d) { if ( !has_vpci(d) ) return 0; if ( is_hardware_domain(d) ) { int ret = pci_host_iterate_bridges_and_count(d, vpci_get_num_handlers_cb); return ret < 0 ? 0 : ret; } /* * This is a guest domain: * * 1 for a single emulated host bridge's configuration space. */ return 1; } > > Cheers, > Thank you, Oleksandr
On 23/11/2021 06:58, Oleksandr Andrushchenko wrote: > Hi, Julien! Hi Oleksandr, > On 22.11.21 19:36, Julien Grall wrote: >> On 18/11/2021 10:46, Oleksandr Andrushchenko wrote: >>> On 18.11.21 09:27, Oleksandr Andrushchenko wrote: >>>>>> + unsigned int count; >>>>>> + >>>>>> + if ( is_hardware_domain(d) ) >>>>>> + /* For each PCI host bridge's configuration space. */ >>>>>> + count = pci_host_get_num_bridges(); >>>>> This first part makes sense to me. But... >>>>> >>>>>> + else >>>>> ... I don't understand how the else is related to this commit. Can you clarify it? >>>>> >>>>>> + /* >>>>>> + * There's a single MSI-X MMIO handler that deals with both PBA >>>>>> + * and MSI-X tables per each PCI device being passed through. >>>>>> + * Maximum number of supported devices is 32 as virtual bus >>>>>> + * topology emulates the devices as embedded endpoints. >>>>>> + * +1 for a single emulated host bridge's configuration space. >>>>>> + */ >>>>>> + count = 1; >>>>>> +#ifdef CONFIG_HAS_PCI_MSI >>>>>> + count += 32; >>>>> Surely, this is a decision that is based on other factor in the vPCI code. So can use a define and avoid hardcoding the number? >>>> Well, in the later series [1] this is defined via PCI_SLOT(~0) + 1 and there is no dedicated >>>> constant for that. I can use the same here, e.g. s/32/PCI_SLOT(~0) + 1 >> >> I would prefer if we introduce a new constant for that. This makes easier to update the code if we decide to increase the number of virtual devices. >> >> However, I am still not sure how the 'else' part is related to this commit. Can you please clarify it? > Well, yes, this is too early for this patch to introduce some future knowledge, so I'll instead have: > > unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d) > { > if ( !has_vpci(d) ) > return 0; > > if ( is_hardware_domain(d) ) > { > int ret = pci_host_iterate_bridges_and_count(d, vpci_get_num_handlers_cb); > > return ret < 0 ? 0 : ret; > } > > /* > * This is a guest domain: > * > * 1 for a single emulated host bridge's configuration space. > */ > return 1; I am afraid that my question stands even with this approach. This patch is only meant to handle the hardware domain, therefore the change seems to be out of context. I would prefer if this change is done separately. Cheers,
Hi, Julien! On 23.11.21 18:12, Julien Grall wrote: > > > On 23/11/2021 06:58, Oleksandr Andrushchenko wrote: >> Hi, Julien! > > Hi Oleksandr, > >> On 22.11.21 19:36, Julien Grall wrote: >>> On 18/11/2021 10:46, Oleksandr Andrushchenko wrote: >>>> On 18.11.21 09:27, Oleksandr Andrushchenko wrote: >>>>>>> + unsigned int count; >>>>>>> + >>>>>>> + if ( is_hardware_domain(d) ) >>>>>>> + /* For each PCI host bridge's configuration space. */ >>>>>>> + count = pci_host_get_num_bridges(); >>>>>> This first part makes sense to me. But... >>>>>> >>>>>>> + else >>>>>> ... I don't understand how the else is related to this commit. Can you clarify it? >>>>>> >>>>>>> + /* >>>>>>> + * There's a single MSI-X MMIO handler that deals with both PBA >>>>>>> + * and MSI-X tables per each PCI device being passed through. >>>>>>> + * Maximum number of supported devices is 32 as virtual bus >>>>>>> + * topology emulates the devices as embedded endpoints. >>>>>>> + * +1 for a single emulated host bridge's configuration space. >>>>>>> + */ >>>>>>> + count = 1; >>>>>>> +#ifdef CONFIG_HAS_PCI_MSI >>>>>>> + count += 32; >>>>>> Surely, this is a decision that is based on other factor in the vPCI code. So can use a define and avoid hardcoding the number? >>>>> Well, in the later series [1] this is defined via PCI_SLOT(~0) + 1 and there is no dedicated >>>>> constant for that. I can use the same here, e.g. s/32/PCI_SLOT(~0) + 1 >>> >>> I would prefer if we introduce a new constant for that. This makes easier to update the code if we decide to increase the number of virtual devices. >>> >>> However, I am still not sure how the 'else' part is related to this commit. Can you please clarify it? >> Well, yes, this is too early for this patch to introduce some future knowledge, so I'll instead have: >> >> unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d) >> { >> if ( !has_vpci(d) ) >> return 0; >> >> if ( is_hardware_domain(d) ) >> { >> int ret = pci_host_iterate_bridges_and_count(d, vpci_get_num_handlers_cb); >> >> return ret < 0 ? 0 : ret; >> } >> >> /* >> * This is a guest domain: >> * >> * 1 for a single emulated host bridge's configuration space. >> */ >> return 1; > > I am afraid that my question stands even with this approach. This patch is only meant to handle the hardware domain, therefore the change seems to be out of context. > > I would prefer if this change is done separately. While I do agree that MSI part and virtual bus topology are not belonging to this patch I can't agree with the rest: we already have MMIO handlers for guest domains and we introduce domain_vpci_get_num_mmio_handlers which must also account on guests and stay consistent. So, despite the patch has "hardware domain" in its name it doesn't mean we should break guests here. Thus I do think the above is still correct wrt this patch. > > Cheers, > Thank you, Oleksandr
Hi, On 23/11/2021 16:41, Oleksandr Andrushchenko wrote: > On 23.11.21 18:12, Julien Grall wrote: >> On 23/11/2021 06:58, Oleksandr Andrushchenko wrote: >>> unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d) >>> { >>> if ( !has_vpci(d) ) >>> return 0; >>> >>> if ( is_hardware_domain(d) ) >>> { >>> int ret = pci_host_iterate_bridges_and_count(d, vpci_get_num_handlers_cb); >>> >>> return ret < 0 ? 0 : ret; >>> } >>> >>> /* >>> * This is a guest domain: >>> * >>> * 1 for a single emulated host bridge's configuration space. >>> */ >>> return 1; >> >> I am afraid that my question stands even with this approach. This patch is only meant to handle the hardware domain, therefore the change seems to be out of context. >> >> I would prefer if this change is done separately. > While I do agree that MSI part and virtual bus topology are not belonging to this > patch I can't agree with the rest: we already have MMIO handlers for guest domains > and we introduce domain_vpci_get_num_mmio_handlers which must also account > on guests and stay consistent. > So, despite the patch has "hardware domain" in its name it doesn't mean we should > break guests here. We were already registering the handler for guest domain before your patch. So this is nothing new. At the moment, we always allocate an extra 16 slot for IO handlers (see MAX_IO_HANDLER). So we are not breaking anything. Instead, this is simply a latent bug. > Thus I do think the above is still correct wrt this patch. The idea of splitting patch is to separate bug fix from new code. This helps backporting and review. In this case, we don't care about backport (PCI passthrough is no supported) and the fix a simple enough. So I am not going to insist on splitting to a separate patch. However, this change *must* be explained in the commit message. Cheers,
Hi, Julien! On 23.11.21 18:58, Julien Grall wrote: > Hi, > > On 23/11/2021 16:41, Oleksandr Andrushchenko wrote: >> On 23.11.21 18:12, Julien Grall wrote: >>> On 23/11/2021 06:58, Oleksandr Andrushchenko wrote: >>>> unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d) >>>> { >>>> if ( !has_vpci(d) ) >>>> return 0; >>>> >>>> if ( is_hardware_domain(d) ) >>>> { >>>> int ret = pci_host_iterate_bridges_and_count(d, vpci_get_num_handlers_cb); >>>> >>>> return ret < 0 ? 0 : ret; >>>> } >>>> >>>> /* >>>> * This is a guest domain: >>>> * >>>> * 1 for a single emulated host bridge's configuration space. >>>> */ >>>> return 1; >>> >>> I am afraid that my question stands even with this approach. This patch is only meant to handle the hardware domain, therefore the change seems to be out of context. >>> >>> I would prefer if this change is done separately. >> While I do agree that MSI part and virtual bus topology are not belonging to this >> patch I can't agree with the rest: we already have MMIO handlers for guest domains >> and we introduce domain_vpci_get_num_mmio_handlers which must also account >> on guests and stay consistent. >> So, despite the patch has "hardware domain" in its name it doesn't mean we should >> break guests here. > > We were already registering the handler for guest domain before your patch. So this is nothing new. > > At the moment, we always allocate an extra 16 slot for IO handlers (see MAX_IO_HANDLER). So we are not breaking anything. Instead, this is simply a latent bug. Agree > >> Thus I do think the above is still correct wrt this patch. > > The idea of splitting patch is to separate bug fix from new code. This helps backporting and review. > > In this case, we don't care about backport (PCI passthrough is no supported) and the fix a simple enough. So I am not going to insist on splitting to a separate patch. > > However, this change *must* be explained in the commit message. I will add a dedicated patch to fix that > > Cheers, > Thank you, Oleksandr
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 96e1b235501d..92a6c509e5c5 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -739,6 +739,8 @@ int arch_domain_create(struct domain *d, if ( (rc = domain_vgic_register(d, &count)) != 0 ) goto fail; + count += domain_vpci_get_num_mmio_handlers(d); + if ( (rc = domain_io_init(d, count + MAX_IO_HANDLER)) != 0 ) goto fail; diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c index 47104b22b221..0d271a6e8881 100644 --- a/xen/arch/arm/pci/pci-host-common.c +++ b/xen/arch/arm/pci/pci-host-common.c @@ -289,6 +289,33 @@ int pci_get_host_bridge_segment(const struct dt_device_node *node, return -EINVAL; } +int pci_host_iterate_bridges(struct domain *d, + int (*cb)(struct domain *d, + struct pci_host_bridge *bridge)) +{ + struct pci_host_bridge *bridge; + int err; + + list_for_each_entry( bridge, &pci_host_bridges, node ) + { + err = cb(d, bridge); + if ( err ) + return err; + } + return 0; +} + +unsigned int pci_host_get_num_bridges(void) +{ + struct pci_host_bridge *bridge; + unsigned int count = 0; + + list_for_each_entry( bridge, &pci_host_bridges, node ) + count++; + + return count; +} + /* * Local variables: * mode: C diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c index 23f45386f4b3..5a6ebd8b9868 100644 --- a/xen/arch/arm/vpci.c +++ b/xen/arch/arm/vpci.c @@ -16,16 +16,31 @@ #include <asm/mmio.h> +static pci_sbdf_t vpci_sbdf_from_gpa(const struct pci_host_bridge *bridge, + paddr_t gpa) +{ + pci_sbdf_t sbdf; + + if ( bridge ) + { + sbdf.sbdf = VPCI_ECAM_BDF(gpa - bridge->cfg->phys_addr); + sbdf.seg = bridge->segment; + sbdf.bus += bridge->cfg->busn_start; + } + else + sbdf.sbdf = VPCI_ECAM_BDF(gpa - GUEST_VPCI_ECAM_BASE); + + return sbdf; +} + static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, register_t *r, void *p) { - pci_sbdf_t sbdf; + struct pci_host_bridge *bridge = p; + pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa); /* data is needed to prevent a pointer cast on 32bit */ unsigned long data; - /* We ignore segment part and always handle segment 0 */ - sbdf.sbdf = VPCI_ECAM_BDF(info->gpa - GUEST_VPCI_ECAM_BASE); - if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa), 1U << info->dabt.size, &data) ) { @@ -41,10 +56,8 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, 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 = VPCI_ECAM_BDF(info->gpa - GUEST_VPCI_ECAM_BASE); + struct pci_host_bridge *bridge = p; + pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa); return vpci_ecam_write(sbdf, ECAM_REG_OFFSET(info->gpa), 1U << info->dabt.size, r); @@ -55,17 +68,54 @@ static const struct mmio_handler_ops vpci_mmio_handler = { .write = vpci_mmio_write, }; +static int vpci_setup_mmio_handler_cb(struct domain *d, + struct pci_host_bridge *bridge) +{ + struct pci_config_window *cfg = bridge->cfg; + + register_mmio_handler(d, &vpci_mmio_handler, + cfg->phys_addr, cfg->size, bridge); + return 0; +} + int domain_vpci_init(struct domain *d) { if ( !has_vpci(d) ) return 0; + if ( is_hardware_domain(d) ) + return pci_host_iterate_bridges(d, vpci_setup_mmio_handler_cb); + + /* Guest domains use what is programmed in their device tree. */ register_mmio_handler(d, &vpci_mmio_handler, GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL); return 0; } +unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d) +{ + unsigned int count; + + if ( is_hardware_domain(d) ) + /* For each PCI host bridge's configuration space. */ + count = pci_host_get_num_bridges(); + else + /* + * There's a single MSI-X MMIO handler that deals with both PBA + * and MSI-X tables per each PCI device being passed through. + * Maximum number of supported devices is 32 as virtual bus + * topology emulates the devices as embedded endpoints. + * +1 for a single emulated host bridge's configuration space. + */ + count = 1; +#ifdef CONFIG_HAS_PCI_MSI + count += 32; +#endif + + return count; +} + /* * Local variables: * mode: C diff --git a/xen/arch/arm/vpci.h b/xen/arch/arm/vpci.h index d8a7b0e3e802..3c713f3fcdb5 100644 --- a/xen/arch/arm/vpci.h +++ b/xen/arch/arm/vpci.h @@ -17,11 +17,17 @@ #ifdef CONFIG_HAS_VPCI int domain_vpci_init(struct domain *d); +unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d); #else static inline int domain_vpci_init(struct domain *d) { return 0; } + +static inline unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d) +{ + return 0; +} #endif #endif /* __ARCH_ARM_VPCI_H__ */ diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h index c20eba643d86..969333043431 100644 --- a/xen/include/asm-arm/pci.h +++ b/xen/include/asm-arm/pci.h @@ -110,6 +110,11 @@ void arch_pci_init_pdev(struct pci_dev *pdev); int pci_get_new_domain_nr(void); +int pci_host_iterate_bridges(struct domain *d, + int (*clb)(struct domain *d, + struct pci_host_bridge *bridge)); +unsigned int pci_host_get_num_bridges(void); + #else /*!CONFIG_HAS_PCI*/ struct arch_pci_dev { };