Re: [PATCH 4/4] xen/arm: its: pre-initialize ITS quirks before LPI setup

[Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>]

Hi,

Mykola Kvach <xakep.amatop@gmail.com> writes:

> From: Mykola Kvach <mykola_kvach@epam.com>
>
> In the current initialization flow, gicv3_init() calls gicv3_dist_init()
> before gicv3_its_init().
>
> When LPIs are supported, gicv3_dist_init() calls
> gicv3_lpi_init_host_lpis(), which initializes host LPI state and allocates
> the boot CPU pending table before ITS quirk flags are computed. Non-boot
> CPUs allocate their pending tables later from the CPU_UP_PREPARE notifier,
> while redistributor LPI programming happens separately in
> gicv3_lpi_init_rdist().
>
> This means the boot CPU LPI setup can observe default ITS memory attributes
> before dma-noncoherent and other ITS quirks are applied.
>
> Introduce gicv3_its_preinit() and call it before gicv3_dist_init(). This
> keeps the actual ITS hardware initialization in gicv3_its_init(), but moves
> ITS discovery, quirk validation and quirk flag setup early enough for the
> subsequent LPI initialization to use the correct attributes.

I must say that it was hard to review this patch, because it does more
description says.

>
> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> ---
> TODO: Think about separating Redistributor/LPI attributes from ITS.
> ---
>  xen/arch/arm/gic-v3-its.c             | 36 +++++++++++++++++----------
>  xen/arch/arm/gic-v3.c                 |  7 ++++++
>  xen/arch/arm/include/asm/gic_v3_its.h |  5 ++++
>  3 files changed, 35 insertions(+), 13 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index ee432088cd..0251d91630 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -63,6 +63,7 @@ struct its_quirk {
>      uint32_t flags;
>  };
>  
> +/* TODO: Separate Redistributor/LPI attributes from ITS quirks. */
>  static uint32_t __ro_after_init its_quirk_flags;
>  
>  static bool gicv3_its_match_quirk_gen4(const struct host_its *hw_its)
> @@ -148,9 +149,15 @@ static uint32_t gicv3_its_collect_quirks(const struct host_its *hw_its,
>      return flags;
>  }
>  
> -static void gicv3_its_enable_quirks(struct host_its *hw_its)
> +static void gicv3_its_enable_quirks(void)
>  {
>      const struct its_quirk *quirk;
> +    const struct host_its *hw_its;
> +
> +    if ( list_empty(&host_its_list) )
> +        return;
> +
> +    hw_its = list_first_entry(&host_its_list, struct host_its, entry);
>  
>      its_quirk_flags = gicv3_its_collect_quirks(hw_its, &quirk);
>  
> @@ -603,16 +610,10 @@ static int gicv3_its_init_single_its(struct host_its *hw_its)
>      uint64_t reg;
>      int i, ret;
>  
> -    hw_its->its_base = ioremap_nocache(hw_its->addr, hw_its->size);
> -    if ( !hw_its->its_base )
> -        return -ENOMEM;
> -
>      ret = gicv3_disable_its(hw_its);
>      if ( ret )
>          return ret;
>  
> -    gicv3_its_enable_quirks(hw_its);
> -
>      reg = readq_relaxed(hw_its->its_base + GITS_TYPER);
>      hw_its->devid_bits = GITS_TYPER_DEVICE_ID_BITS(reg);
>      hw_its->evid_bits = GITS_TYPER_EVENT_ID_BITS(reg);
> @@ -1161,6 +1162,11 @@ static void add_to_host_its_list(paddr_t addr, paddr_t size,
>      its_data->size = size;
>      its_data->dt_node = node;
>  
> +    its_data->its_base = ioremap_nocache(its_data->addr, its_data->size);

It is very confusing that add_to_host_its_list() function ioremaps
ITS. It should not do this. Or at least renamed it to
map_and_add_to_host_its_list() or consider moving this to
gicv3_its_preinit(), maybe?

> +    if ( !its_data->its_base )
> +        panic("GICv3: Cannot map ITS frame: 0x%lx, 0x%lx\n",
> +            its_data->addr, its_data->size);
> +
>      printk("GICv3: Found ITS @0x%lx\n", addr);
>  
>      list_add_tail(&its_data->entry, &host_its_list);
> @@ -1238,16 +1244,22 @@ static void gicv3_its_acpi_init(void)
>  
>  #endif
>  
> -int gicv3_its_init(void)
> +void __init gicv3_its_preinit(void)
>  {
> -    struct host_its *hw_its;
> -    int ret;
> -
>      if ( acpi_disabled )
>          gicv3_its_dt_init(dt_interrupt_controller);
>      else
>          gicv3_its_acpi_init();
>  
> +    gicv3_its_validate_quirks();
> +    gicv3_its_enable_quirks();
> +}
> +
> +int gicv3_its_init(void)
> +{
> +    struct host_its *hw_its;
> +    int ret;
> +
>      list_for_each_entry(hw_its, &host_its_list, entry)
>      {
>          ret = gicv3_its_init_single_its(hw_its);
> @@ -1255,8 +1267,6 @@ int gicv3_its_init(void)
>              return ret;
>      }
>  
> -    gicv3_its_validate_quirks();
> -
>      return 0;
>  }
>  
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index bc07f97c16..6e44d23d64 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1974,6 +1974,13 @@ static int __init gicv3_init(void)
>  
>      spin_lock(&gicv3.lock);
>  
> +    if ( gic_dist_supports_lpis() )
> +        /*
> +         * Apply ITS quirks before gicv3_dist_init() sets up host LPIs,
> +         * so pending tables use the correct ITS memory attributes.
> +         */
> +        gicv3_its_preinit();
> +
>      gicv3_dist_init();
>  
>      if ( gic_dist_supports_lpis() )
> diff --git a/xen/arch/arm/include/asm/gic_v3_its.h b/xen/arch/arm/include/asm/gic_v3_its.h
> index fc5a84892c..e1d7522ea5 100644
> --- a/xen/arch/arm/include/asm/gic_v3_its.h
> +++ b/xen/arch/arm/include/asm/gic_v3_its.h
> @@ -156,6 +156,7 @@ int gicv3_lpi_init_rdist(void __iomem * rdist_base);
>  
>  /* Initialize the host structures for LPIs and the host ITSes. */
>  int gicv3_lpi_init_host_lpis(unsigned int host_lpi_bits);
> +void gicv3_its_preinit(void);
>  int gicv3_its_init(void);
>  
>  /* Store the physical address and ID for each redistributor as read from DT. */
> @@ -219,6 +220,10 @@ static inline int gicv3_its_deny_access(struct domain *d)
>      return 0;
>  }
>  
> +static inline void gicv3_its_preinit(void)
> +{
> +}
> +
>  static inline bool gicv3_its_host_has_its(void)
>  {
>      return false;

-- 
WBR, Volodymyr