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

[Julien Grall <julien@xxxxxxx>]

Hi Oleksandr,

On 03/09/2021 09:33, 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.
I am not entirely sure what you mean by "synchronized" here. Are you 
refer to the content of the configuration space?
> 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);
We have a fixed array for handling the MMIO handlers. So you need to 
make sure we have enough space in it to store one handler per handler.
This is quite similar to the problem we had with the re-distribuor on 
GICv3. Have a look there to see how we dealt with it.
> +    return 0;
> +}
> +
>   /* 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,
NIT: We tend to name callback variable 'cb'.

Cheers,

--
Julien Grall