Re: [PATCH v4 1/4] xen/arm: its: initialize host LPI state before activating ITSes

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Mykola,

Mykola Kvach <mykola_kvach@xxxxxxxx> writes:

> The boot CPU pending table must use the memory attributes selected by
> ITS quirks. gicv3_lpi_init_host_lpis() is therefore called after
> gicv3_its_init(). However, gicv3_its_init() also programs and enables
> each ITS before host LPI state is allocated. No ITS commands are
> submitted at that point, but this ordering relies on that implementation
> detail.
>
> Split per-ITS initialization into preparation and activation phases.
> First map and disable every ITS and collect its quirks. Then initialize
> host LPI state. Only after that, allocate and program the ITS tables and
> command queue, and enable each ITS.
>
> The subsequent gicv3_cpu_init() sequence remains unchanged: it programs
> the Redistributor LPI tables, enables LPIs, and then submits the first
> MAPC and SYNC commands.
>
> Suggested-by: Julien Grall <julien@xxxxxxx>
> Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>

Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>

> ---
> Changes in v4:
> - Move the empty ITS list check before the preparation loop.
>
> Changes in v3:
> - New patch implementing the post-4.22 initialization order discussed
>   during review of the ordering fix.
>
> Link: 
> https://patchew.org/Xen/341edd8de63dcd84ccc6e7b6c03e9e8fc7105184.1781847061.git.mykola._5Fkvach@xxxxxxxx/
> ---
>  xen/arch/arm/gic-v3-its.c             | 32 ++++++++++++++++++++++-----
>  xen/arch/arm/gic-v3.c                 | 15 ++-----------
>  xen/arch/arm/include/asm/gic_v3_its.h |  4 ++--
>  3 files changed, 31 insertions(+), 20 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 325835b0ad..511e49a116 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -11,6 +11,7 @@
>  #include <xen/lib.h>
>  #include <xen/delay.h>
>  #include <xen/iocap.h>
> +#include <xen/init.h>
>  #include <xen/libfdt/libfdt.h>
>  #include <xen/mm.h>
>  #include <xen/rbtree.h>
> @@ -549,10 +550,9 @@ static int gicv3_disable_its(struct host_its *hw_its)
>      return -ETIMEDOUT;
>  }
>  
> -static int gicv3_its_init_single_its(struct host_its *hw_its)
> +static int __init gicv3_its_prepare_single_its(struct host_its *hw_its)
>  {
> -    uint64_t reg;
> -    int i, ret;
> +    int ret;
>  
>      hw_its->its_base = ioremap_nocache(hw_its->addr, hw_its->size);
>      if ( !hw_its->its_base )
> @@ -564,6 +564,14 @@ static int gicv3_its_init_single_its(struct host_its 
> *hw_its)
>  
>      gicv3_its_enable_quirks(hw_its);
>  
> +    return 0;
> +}
> +
> +static int __init gicv3_its_init_single_its(struct host_its *hw_its)
> +{
> +    uint64_t reg;
> +    int i, ret;
> +
>      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);
> @@ -1189,7 +1197,7 @@ static void gicv3_its_acpi_init(void)
>  
>  #endif
>  
> -int gicv3_its_init(void)
> +int __init gicv3_its_init(unsigned int host_lpi_bits)
>  {
>      struct host_its *hw_its;
>      int ret;
> @@ -1199,15 +1207,29 @@ int gicv3_its_init(void)
>      else
>          gicv3_its_acpi_init();
>  
> +    if ( list_empty(&host_its_list) )
> +        return 0;
> +
>      list_for_each_entry(hw_its, &host_its_list, entry)
>      {
> -        ret = gicv3_its_init_single_its(hw_its);
> +        ret = gicv3_its_prepare_single_its(hw_its);
>          if ( ret )
>              return ret;
>      }
>  
>      gicv3_its_validate_quirks();
>  
> +    ret = gicv3_lpi_init_host_lpis(host_lpi_bits);
> +    if ( ret )
> +        return ret;
> +
> +    list_for_each_entry(hw_its, &host_its_list, entry)
> +    {
> +        ret = gicv3_its_init_single_its(hw_its);
> +        if ( ret )
> +            return ret;
> +    }
> +
>      return 0;
>  }
>  
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index acdac22953..6bc3e313be 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1984,20 +1984,9 @@ static int __init gicv3_init(void)
>  
>      if ( gic_dist_supports_lpis() )
>      {
> -        res = gicv3_its_init();
> +        res = gicv3_its_init(intid_bits);
>          if ( res )
> -            panic("GICv3: ITS: initialization failed: %d\n", res);
> -
> -        /*
> -         * Host LPI allocation uses ITS-derived memory attributes, so defer 
> it
> -         * until after gicv3_its_init() has discovered ITS workarounds.
> -         */
> -        if ( gicv3_its_host_has_its() )
> -        {
> -            res = gicv3_lpi_init_host_lpis(intid_bits);
> -            if ( res )
> -                panic("GICv3: LPI initialization failed: %d\n", res);
> -        }
> +            panic("GICv3: ITS/LPI initialization failed: %d\n", res);
>      }
>  
>      res = gicv3_cpu_init();
> diff --git a/xen/arch/arm/include/asm/gic_v3_its.h 
> b/xen/arch/arm/include/asm/gic_v3_its.h
> index 02c083f210..e1322b94f8 100644
> --- a/xen/arch/arm/include/asm/gic_v3_its.h
> +++ b/xen/arch/arm/include/asm/gic_v3_its.h
> @@ -156,7 +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);
> -int gicv3_its_init(void);
> +int gicv3_its_init(unsigned int host_lpi_bits);
>  
>  /* Store the physical address and ID for each redistributor as read from DT. 
> */
>  void gicv3_set_redist_address(paddr_t address, unsigned int redist_id);
> @@ -245,7 +245,7 @@ static inline int gicv3_lpi_init_host_lpis(unsigned int 
> host_lpi_bits)
>      return 0;
>  }
>  
> -static inline int gicv3_its_init(void)
> +static inline int gicv3_its_init(unsigned int host_lpi_bits)
>  {
>      return 0;
>  }

-- 
WBR, Volodymyr