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.

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);
+    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;
-- 
2.43.0

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