| Message ID | 20210923125438.234162-10-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI devices passthrough on Arm, part 2 | expand |
On Thu, 23 Sep 2021, 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 on the configuration space. For example, BARs and ^ of > COMMAND registers require emulation for guests and the guest view > on the registers needs to be in sync with the real contents of the ^ of > 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> The patch looks pretty good, only a couple of minor comments below > --- > 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 | 28 +++++++++++++++++++++++++ > xen/arch/arm/vpci.c | 33 ++++++++++++++++++++++++++++++ > xen/arch/arm/vpci.h | 6 ++++++ > xen/include/asm-arm/pci.h | 7 +++++++ > 5 files changed, 76 insertions(+) > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > index 854e8fed0393..c7b25bc70439 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -733,6 +733,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 1567b6e2956c..155f2a2743af 100644 > --- a/xen/arch/arm/pci/pci-host-common.c > +++ b/xen/arch/arm/pci/pci-host-common.c > @@ -300,6 +300,34 @@ struct dt_device_node *pci_find_host_bridge_node(struct device *dev) > } > return bridge->dt_node; > } > + > +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; > +} > + > +int pci_host_get_num_bridges(void) > +{ > + struct pci_host_bridge *bridge; > + 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 76c12b92814f..14947e975d69 100644 > --- a/xen/arch/arm/vpci.c > +++ b/xen/arch/arm/vpci.c > @@ -80,17 +80,50 @@ static const struct mmio_handler_ops vpci_mmio_handler = { > .write = vpci_mmio_write, > }; > > +static int vpci_setup_mmio_handler(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, NULL); > + 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); > + > + /* 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; > } > > +int domain_vpci_get_num_mmio_handlers(struct domain *d) > +{ > + int count = 0; > + > + if ( is_hardware_domain(d) ) > + /* For each PCI host bridge's configuration space. */ > + count += pci_host_get_num_bridges(); NIT: why += instead of = ? > + else > + /* > + * VPCI_MSIX_MEM_NUM handlers for 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 = VPCI_MSIX_MEM_NUM * 32 + 1; > + > + return count; > +} > + > /* > * Local variables: > * mode: C > diff --git a/xen/arch/arm/vpci.h b/xen/arch/arm/vpci.h > index d8a7b0e3e802..27a2b069abd2 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); > +int domain_vpci_get_num_mmio_handlers(struct domain *d); > #else > static inline int domain_vpci_init(struct domain *d) > { > return 0; > } > + > +static inline 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 5b100556225e..7618f0b6725b 100644 > --- a/xen/include/asm-arm/pci.h > +++ b/xen/include/asm-arm/pci.h > @@ -15,6 +15,8 @@ > #ifndef __ARM_PCI_H__ > #define __ARM_PCI_H__ > > +#include <asm/mmio.h> This should be no longer required. > #ifdef CONFIG_HAS_PCI > > #define pci_to_dev(pcidev) (&(pcidev)->arch.dev) > @@ -109,6 +111,11 @@ static always_inline bool is_pci_passthrough_enabled(void) > { > return !!pci_passthrough_enabled; > } > + > +int pci_host_iterate_bridges(struct domain *d, > + int (*clb)(struct domain *d, > + struct pci_host_bridge *bridge)); > +int pci_host_get_num_bridges(void); > #else /*!CONFIG_HAS_PCI*/ > > #define pci_passthrough_enabled (false)
On 25.09.21 03:18, Stefano Stabellini wrote: > On Thu, 23 Sep 2021, 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 on the configuration space. For example, BARs and > ^ of > Ack >> COMMAND registers require emulation for guests and the guest view >> on the registers needs to be in sync with the real contents of the > ^ of Ack > >> 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> > The patch looks pretty good, only a couple of minor comments below > > >> --- >> 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 | 28 +++++++++++++++++++++++++ >> xen/arch/arm/vpci.c | 33 ++++++++++++++++++++++++++++++ >> xen/arch/arm/vpci.h | 6 ++++++ >> xen/include/asm-arm/pci.h | 7 +++++++ >> 5 files changed, 76 insertions(+) >> >> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c >> index 854e8fed0393..c7b25bc70439 100644 >> --- a/xen/arch/arm/domain.c >> +++ b/xen/arch/arm/domain.c >> @@ -733,6 +733,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 1567b6e2956c..155f2a2743af 100644 >> --- a/xen/arch/arm/pci/pci-host-common.c >> +++ b/xen/arch/arm/pci/pci-host-common.c >> @@ -300,6 +300,34 @@ struct dt_device_node *pci_find_host_bridge_node(struct device *dev) >> } >> return bridge->dt_node; >> } >> + >> +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; >> +} >> + >> +int pci_host_get_num_bridges(void) >> +{ >> + struct pci_host_bridge *bridge; >> + 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 76c12b92814f..14947e975d69 100644 >> --- a/xen/arch/arm/vpci.c >> +++ b/xen/arch/arm/vpci.c >> @@ -80,17 +80,50 @@ static const struct mmio_handler_ops vpci_mmio_handler = { >> .write = vpci_mmio_write, >> }; >> >> +static int vpci_setup_mmio_handler(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, NULL); >> + 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); >> + >> + /* 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; >> } >> >> +int domain_vpci_get_num_mmio_handlers(struct domain *d) >> +{ >> + int count = 0; >> + >> + if ( is_hardware_domain(d) ) >> + /* For each PCI host bridge's configuration space. */ >> + count += pci_host_get_num_bridges(); > NIT: why += instead of = ? Will fix > > >> + else >> + /* >> + * VPCI_MSIX_MEM_NUM handlers for 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 = VPCI_MSIX_MEM_NUM * 32 + 1; >> + >> + return count; >> +} >> + >> /* >> * Local variables: >> * mode: C >> diff --git a/xen/arch/arm/vpci.h b/xen/arch/arm/vpci.h >> index d8a7b0e3e802..27a2b069abd2 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); >> +int domain_vpci_get_num_mmio_handlers(struct domain *d); >> #else >> static inline int domain_vpci_init(struct domain *d) >> { >> return 0; >> } >> + >> +static inline 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 5b100556225e..7618f0b6725b 100644 >> --- a/xen/include/asm-arm/pci.h >> +++ b/xen/include/asm-arm/pci.h >> @@ -15,6 +15,8 @@ >> #ifndef __ARM_PCI_H__ >> #define __ARM_PCI_H__ >> >> +#include <asm/mmio.h> > This should be no longer required. I'll check and remove > > >> #ifdef CONFIG_HAS_PCI >> >> #define pci_to_dev(pcidev) (&(pcidev)->arch.dev) >> @@ -109,6 +111,11 @@ static always_inline bool is_pci_passthrough_enabled(void) >> { >> return !!pci_passthrough_enabled; >> } >> + >> +int pci_host_iterate_bridges(struct domain *d, >> + int (*clb)(struct domain *d, >> + struct pci_host_bridge *bridge)); >> +int pci_host_get_num_bridges(void); >> #else /*!CONFIG_HAS_PCI*/ >> >> #define pci_passthrough_enabled (false) Thank you, Oleksandr
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 854e8fed0393..c7b25bc70439 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -733,6 +733,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 1567b6e2956c..155f2a2743af 100644 --- a/xen/arch/arm/pci/pci-host-common.c +++ b/xen/arch/arm/pci/pci-host-common.c @@ -300,6 +300,34 @@ struct dt_device_node *pci_find_host_bridge_node(struct device *dev) } return bridge->dt_node; } + +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; +} + +int pci_host_get_num_bridges(void) +{ + struct pci_host_bridge *bridge; + 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 76c12b92814f..14947e975d69 100644 --- a/xen/arch/arm/vpci.c +++ b/xen/arch/arm/vpci.c @@ -80,17 +80,50 @@ static const struct mmio_handler_ops vpci_mmio_handler = { .write = vpci_mmio_write, }; +static int vpci_setup_mmio_handler(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, NULL); + 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); + + /* 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; } +int domain_vpci_get_num_mmio_handlers(struct domain *d) +{ + int count = 0; + + if ( is_hardware_domain(d) ) + /* For each PCI host bridge's configuration space. */ + count += pci_host_get_num_bridges(); + else + /* + * VPCI_MSIX_MEM_NUM handlers for 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 = VPCI_MSIX_MEM_NUM * 32 + 1; + + return count; +} + /* * Local variables: * mode: C diff --git a/xen/arch/arm/vpci.h b/xen/arch/arm/vpci.h index d8a7b0e3e802..27a2b069abd2 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); +int domain_vpci_get_num_mmio_handlers(struct domain *d); #else static inline int domain_vpci_init(struct domain *d) { return 0; } + +static inline 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 5b100556225e..7618f0b6725b 100644 --- a/xen/include/asm-arm/pci.h +++ b/xen/include/asm-arm/pci.h @@ -15,6 +15,8 @@ #ifndef __ARM_PCI_H__ #define __ARM_PCI_H__ +#include <asm/mmio.h> + #ifdef CONFIG_HAS_PCI #define pci_to_dev(pcidev) (&(pcidev)->arch.dev) @@ -109,6 +111,11 @@ static always_inline bool is_pci_passthrough_enabled(void) { return !!pci_passthrough_enabled; } + +int pci_host_iterate_bridges(struct domain *d, + int (*clb)(struct domain *d, + struct pci_host_bridge *bridge)); +int pci_host_get_num_bridges(void); #else /*!CONFIG_HAS_PCI*/ #define pci_passthrough_enabled (false)