Re: [PATCH 09/11] xen/arm: Setup MMIO range trap handlers for hardware domain

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 3 Sep 2021, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> 
> In order vPCI to work it needs all access to PCI configuration space
> (ECAM) to be synchronized among all entities, e.g. hardware domain and
> guests. For that implement PCI host bridge specific callbacks to
> properly setup those ranges depending on particular host bridge
> implementation.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> ---
>  xen/arch/arm/pci/ecam.c            | 11 +++++++++++
>  xen/arch/arm/pci/pci-host-common.c | 16 ++++++++++++++++
>  xen/arch/arm/vpci.c                | 13 +++++++++++++
>  xen/include/asm-arm/pci.h          |  8 ++++++++
>  4 files changed, 48 insertions(+)
> 
> diff --git a/xen/arch/arm/pci/ecam.c b/xen/arch/arm/pci/ecam.c
> index 91c691b41fdf..92ecb2e0762b 100644
> --- a/xen/arch/arm/pci/ecam.c
> +++ b/xen/arch/arm/pci/ecam.c
> @@ -42,6 +42,16 @@ void __iomem *pci_ecam_map_bus(struct pci_host_bridge 
> *bridge,
>      return base + (PCI_DEVFN(sbdf_t.dev, sbdf_t.fn) << devfn_shift) + where;
>  }
>  
> +static int pci_ecam_register_mmio_handler(struct domain *d,
> +                                          struct pci_host_bridge *bridge,
> +                                          const struct mmio_handler_ops *ops)
> +{
> +    struct pci_config_window *cfg = bridge->sysdata;
> +
> +    register_mmio_handler(d, ops, cfg->phys_addr, cfg->size, NULL);
> +    return 0;
> +}

Given that struct pci_config_window is generic (it is not specific to
one bridge), I wonder if we even need the .register_mmio_handler
callback here.

In fact, pci_host_bridge->sysdata doesn't even need to be a void*, it
could be a struct pci_config_window*, right?

We could simply call:

register_mmio_handler(d, ops, cfg->phys_addr, cfg->size, NULL);

for each bridge directly from domain_vpci_init ?




>  /* ECAM ops */
>  const struct pci_ecam_ops pci_generic_ecam_ops = {
>      .bus_shift  = 20,
> @@ -49,6 +59,7 @@ const struct pci_ecam_ops pci_generic_ecam_ops = {
>          .map_bus                = pci_ecam_map_bus,
>          .read                   = pci_generic_config_read,
>          .write                  = pci_generic_config_write,
> +        .register_mmio_handler  = pci_ecam_register_mmio_handler,
>      }
>  };
>  
> diff --git a/xen/arch/arm/pci/pci-host-common.c 
> b/xen/arch/arm/pci/pci-host-common.c
> index d2fef5476b8e..a89112bfbb7c 100644
> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -318,6 +318,22 @@ 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 (*clb)(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 = clb(d, bridge);
> +        if ( err )
> +            return err;
> +    }
> +    return 0;
> +}
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
> index da8b1ca13c07..258134292458 100644
> --- a/xen/arch/arm/vpci.c
> +++ b/xen/arch/arm/vpci.c
> @@ -74,11 +74,24 @@ 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)
> +{
> +    if ( bridge->ops->register_mmio_handler )
> +        return bridge->ops->register_mmio_handler(d, bridge,
> +                                                  &vpci_mmio_handler);
> +    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);
>  
> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
> index 7dc4c8dc9026..2c7c7649e00f 100644
> --- a/xen/include/asm-arm/pci.h
> +++ b/xen/include/asm-arm/pci.h
> @@ -17,6 +17,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)
> @@ -77,6 +79,9 @@ struct pci_ops {
>                  uint32_t reg, uint32_t len, uint32_t *value);
>      int (*write)(struct pci_host_bridge *bridge, uint32_t sbdf,
>                   uint32_t reg, uint32_t len, uint32_t value);
> +    int (*register_mmio_handler)(struct domain *d,
> +                                 struct pci_host_bridge *bridge,
> +                                 const struct mmio_handler_ops *ops);
>  };
>  
>  /*
> @@ -107,6 +112,9 @@ int pci_get_host_bridge_segment(const struct 
> dt_device_node *node,
>                                  uint16_t *segment);
>  struct dt_device_node *pci_find_host_bridge_node(struct device *dev);
>  
> +int pci_host_iterate_bridges(struct domain *d,
> +                             int (*clb)(struct domain *d,
> +                                        struct pci_host_bridge *bridge));
>  #else   /*!CONFIG_HAS_PCI*/
>  
>  struct arch_pci_dev { };
> -- 
> 2.25.1
>