Re: [PATCH v3 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>
> ---
> 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..972825bf06 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;
> @@ -1201,13 +1209,27 @@ int gicv3_its_init(void)
>  
>      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();
>  
> +    if ( list_empty(&host_its_list) )
> +        return 0;

What is the purpose of this check here? I'd expect to see it before the
first list_for_each_entry() loop.

> +
> +    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;
>  }

[...]


-- 
WBR, Volodymyr