[PATCH v2 0/2] xen/arm: GICv3: Only initialize ITS when LPIs are available

[PATCH v2 0/2] xen/arm: GICv3: Only initialize ITS when LPIs are available

[Julien Grall]

Hi all,

This small patch series fix an issue I discovered when using the Foundation
model and the DT provided by Linux upstream.

Indeed the Device-Tree is exposing an ITS but LPIs are not available.
Resulting to an early crash on Xen.

Whilst this looks a DT issue, Linux is able to cope with it. So I think Xen
should also cope with such DT.

Cheers,

Julien Grall (2):
  xen/arm: GICv3: Parse ITS information from the firmware tables later
    on
  xen/arm: GICv3: Only initialize ITS when the distributor supports
    LPIs.

 xen/arch/arm/gic-v3-its.c        | 47 +++++++++++++++++++++++++---------------
 xen/arch/arm/gic-v3.c            | 19 +++++++++-------
 xen/include/asm-arm/gic_v3_its.h | 12 ----------
 3 files changed, 41 insertions(+), 37 deletions(-)

-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 1/2] xen/arm: GICv3: Parse ITS information from the firmware tables later on

[Julien Grall]

There are Device Tree (e.g for the Foundation Model) out that describes the
ITS but LPIs is not supported by the platform. Booting with such DT will
result to an early Data Abort. The same DT is booting fine with a
baremetal  Linux because ITS will be initialized only when LPIs is
supported.

While this is a bug in the DT, I think Xen should be boot with the same
hardware level support (e.g ITS will not be used) as with a baremetal
Linux.

The slight problem is Xen is relying on gicv3_its_host_has_its() to know
if ITS can be used. The list is populated by gicv3_its_{dt,acpi}_init().
It would theoritical be possible to gate those with a check of
GICD_TYPER.LPIS because we don't know yet whether the HW is an actual
GICv3/GICv4.

Looking at the callers of gicv3_its_host_has_its(), they will only be
done after gicv3_its_init() is called. Therefore move the parsing of ITS
information from firmware tables later on.

Note that gicv3_its_init() has been moved at the end of the file to
avoid forward declaration.

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---

I can move the code movement in a separate patch if necessary. It was
small enough that I thought it would not be worth.

    Changes in v2:
        - Actually test compilation and boot of this patch...
---
 xen/arch/arm/gic-v3-its.c        | 47 +++++++++++++++++++++++++---------------
 xen/arch/arm/gic-v3.c            |  5 -----
 xen/include/asm-arm/gic_v3_its.h | 12 ----------
 3 files changed, 30 insertions(+), 34 deletions(-)

diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index e57ae05771..6127894d0b 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -515,21 +515,6 @@ static int gicv3_its_init_single_its(struct host_its *hw_its)
     return 0;
 }
 
-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);
-        if ( ret )
-            return ret;
-    }
-
-    return 0;
-}
-
 /*
  * TODO: Investigate the interaction when a guest removes a device while
  * some LPIs are still in flight.
@@ -1019,7 +1004,7 @@ static void add_to_host_its_list(paddr_t addr, paddr_t size,
 }
 
 /* Scan the DT for any ITS nodes and create a list of host ITSes out of it. */
-void gicv3_its_dt_init(const struct dt_device_node *node)
+static void gicv3_its_dt_init(const struct dt_device_node *node)
 {
     const struct dt_device_node *its = NULL;
 
@@ -1056,7 +1041,7 @@ static int gicv3_its_acpi_probe(struct acpi_subtable_header *header,
     return 0;
 }
 
-void gicv3_its_acpi_init(void)
+static void gicv3_its_acpi_init(void)
 {
     /* Parse ITS information */
     acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
@@ -1081,8 +1066,36 @@ unsigned long gicv3_its_make_hwdom_madt(const struct domain *d, void *base_ptr)
 
     return sizeof(struct acpi_madt_generic_translator) * vgic_v3_its_count(d);
 }
+#else /* !CONFIG_ACPI */
+
+static void gicv3_its_acpi_init(void)
+{
+    ASSERT_UNREACHABLE();
+}
+
 #endif
 
+int gicv3_its_init(void)
+{
+    struct host_its *hw_its;
+    int ret;
+
+    if ( acpi_disabled )
+        gicv3_its_dt_init(dt_interrupt_controller);
+    else
+        gicv3_its_acpi_init();
+
+    list_for_each_entry(hw_its, &host_its_list, entry)
+    {
+        ret = gicv3_its_init_single_its(hw_its);
+        if ( ret )
+            return ret;
+    }
+
+    return 0;
+}
+
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index a0d290b55c..9f9cf59f82 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1314,9 +1314,6 @@ static void __init gicv3_dt_init(void)
     if ( !res )
         dt_device_get_address(node, 1 + gicv3.rdist_count + 2,
                               &vbase, &vsize);
-
-    /* Check for ITS child nodes and build the host ITS list accordingly. */
-    gicv3_its_dt_init(node);
 }
 
 static int gicv3_iomem_deny_access(const struct domain *d)
@@ -1608,8 +1605,6 @@ static void __init gicv3_acpi_init(void)
 
     gicv3.rdist_stride = 0;
 
-    gicv3_its_acpi_init();
-
     /*
      * In ACPI, 0 is considered as the invalid address. However the rest
      * of the initialization rely on the invalid address to be
diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h
index 40dffdc0fa..78a9bb14de 100644
--- a/xen/include/asm-arm/gic_v3_its.h
+++ b/xen/include/asm-arm/gic_v3_its.h
@@ -133,11 +133,7 @@ struct host_its {
 
 extern struct list_head host_its_list;
 
-/* Parse the host DT and pick up all host ITSes. */
-void gicv3_its_dt_init(const struct dt_device_node *node);
-
 #ifdef CONFIG_ACPI
-void gicv3_its_acpi_init(void);
 unsigned long gicv3_its_make_hwdom_madt(const struct domain *d,
                                         void *base_ptr);
 #endif
@@ -202,15 +198,7 @@ void gicv3_lpi_update_host_entry(uint32_t host_lpi, int domain_id,
 
 #else
 
-static inline void gicv3_its_dt_init(const struct dt_device_node *node)
-{
-}
-
 #ifdef CONFIG_ACPI
-static inline void gicv3_its_acpi_init(void)
-{
-}
-
 static inline unsigned long gicv3_its_make_hwdom_madt(const struct domain *d,
                                                       void *base_ptr)
 {
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 1/2] xen/arm: GICv3: Parse ITS information from the firmware tables later on

[Stefano Stabellini]

On Wed, 24 Jan 2018, Julien Grall wrote:
> There are Device Tree (e.g for the Foundation Model) out that describes the
> ITS but LPIs is not supported by the platform. Booting with such DT will
> result to an early Data Abort. The same DT is booting fine with a
> baremetal  Linux because ITS will be initialized only when LPIs is
> supported.
> 
> While this is a bug in the DT, I think Xen should be boot with the same
> hardware level support (e.g ITS will not be used) as with a baremetal
> Linux.
> 
> The slight problem is Xen is relying on gicv3_its_host_has_its() to know
> if ITS can be used. The list is populated by gicv3_its_{dt,acpi}_init().
> It would theoritical be possible to gate those with a check of
           ^ theoretically


> GICD_TYPER.LPIS because we don't know yet whether the HW is an actual
> GICv3/GICv4.
> 
> Looking at the callers of gicv3_its_host_has_its(), they will only be
> done after gicv3_its_init() is called. Therefore move the parsing of ITS
> information from firmware tables later on.
> 
> Note that gicv3_its_init() has been moved at the end of the file to
> avoid forward declaration.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
> 
> I can move the code movement in a separate patch if necessary. It was
> small enough that I thought it would not be worth.

Yeah, that's OK, thanks for pointing it out

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


>     Changes in v2:
>         - Actually test compilation and boot of this patch...
> ---
>  xen/arch/arm/gic-v3-its.c        | 47 +++++++++++++++++++++++++---------------
>  xen/arch/arm/gic-v3.c            |  5 -----
>  xen/include/asm-arm/gic_v3_its.h | 12 ----------
>  3 files changed, 30 insertions(+), 34 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index e57ae05771..6127894d0b 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -515,21 +515,6 @@ static int gicv3_its_init_single_its(struct host_its *hw_its)
>      return 0;
>  }
>  
> -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);
> -        if ( ret )
> -            return ret;
> -    }
> -
> -    return 0;
> -}
> -
>  /*
>   * TODO: Investigate the interaction when a guest removes a device while
>   * some LPIs are still in flight.
> @@ -1019,7 +1004,7 @@ static void add_to_host_its_list(paddr_t addr, paddr_t size,
>  }
>  
>  /* Scan the DT for any ITS nodes and create a list of host ITSes out of it. */
> -void gicv3_its_dt_init(const struct dt_device_node *node)
> +static void gicv3_its_dt_init(const struct dt_device_node *node)
>  {
>      const struct dt_device_node *its = NULL;
>  
> @@ -1056,7 +1041,7 @@ static int gicv3_its_acpi_probe(struct acpi_subtable_header *header,
>      return 0;
>  }
>  
> -void gicv3_its_acpi_init(void)
> +static void gicv3_its_acpi_init(void)
>  {
>      /* Parse ITS information */
>      acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
> @@ -1081,8 +1066,36 @@ unsigned long gicv3_its_make_hwdom_madt(const struct domain *d, void *base_ptr)
>  
>      return sizeof(struct acpi_madt_generic_translator) * vgic_v3_its_count(d);
>  }
> +#else /* !CONFIG_ACPI */
> +
> +static void gicv3_its_acpi_init(void)
> +{
> +    ASSERT_UNREACHABLE();
> +}
> +
>  #endif
>  
> +int gicv3_its_init(void)
> +{
> +    struct host_its *hw_its;
> +    int ret;
> +
> +    if ( acpi_disabled )
> +        gicv3_its_dt_init(dt_interrupt_controller);
> +    else
> +        gicv3_its_acpi_init();
> +
> +    list_for_each_entry(hw_its, &host_its_list, entry)
> +    {
> +        ret = gicv3_its_init_single_its(hw_its);
> +        if ( ret )
> +            return ret;
> +    }
> +
> +    return 0;
> +}
> +
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index a0d290b55c..9f9cf59f82 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1314,9 +1314,6 @@ static void __init gicv3_dt_init(void)
>      if ( !res )
>          dt_device_get_address(node, 1 + gicv3.rdist_count + 2,
>                                &vbase, &vsize);
> -
> -    /* Check for ITS child nodes and build the host ITS list accordingly. */
> -    gicv3_its_dt_init(node);
>  }
>  
>  static int gicv3_iomem_deny_access(const struct domain *d)
> @@ -1608,8 +1605,6 @@ static void __init gicv3_acpi_init(void)
>  
>      gicv3.rdist_stride = 0;
>  
> -    gicv3_its_acpi_init();
> -
>      /*
>       * In ACPI, 0 is considered as the invalid address. However the rest
>       * of the initialization rely on the invalid address to be
> diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h
> index 40dffdc0fa..78a9bb14de 100644
> --- a/xen/include/asm-arm/gic_v3_its.h
> +++ b/xen/include/asm-arm/gic_v3_its.h
> @@ -133,11 +133,7 @@ struct host_its {
>  
>  extern struct list_head host_its_list;
>  
> -/* Parse the host DT and pick up all host ITSes. */
> -void gicv3_its_dt_init(const struct dt_device_node *node);
> -
>  #ifdef CONFIG_ACPI
> -void gicv3_its_acpi_init(void);
>  unsigned long gicv3_its_make_hwdom_madt(const struct domain *d,
>                                          void *base_ptr);
>  #endif
> @@ -202,15 +198,7 @@ void gicv3_lpi_update_host_entry(uint32_t host_lpi, int domain_id,
>  
>  #else
>  
> -static inline void gicv3_its_dt_init(const struct dt_device_node *node)
> -{
> -}
> -
>  #ifdef CONFIG_ACPI
> -static inline void gicv3_its_acpi_init(void)
> -{
> -}
> -
>  static inline unsigned long gicv3_its_make_hwdom_madt(const struct domain *d,
>                                                        void *base_ptr)
>  {
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 2/2] xen/arm: GICv3: Only initialize ITS when the distributor supports LPIs.

[Julien Grall]

There are firmware tables out describing the ITS but does not support
LPIs. This will result to a data abort when trying to initialize ITS.

While this can be consider a bug in the Device-Tree, same configuration
boots on Linux. So gate the ITS initialization with the support of LPIs
in the distributor.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/gic-v3.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 9f9cf59f82..730450e34b 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1637,6 +1637,11 @@ static unsigned long gicv3_get_hwdom_extra_madt_size(const struct domain *d)
 }
 #endif
 
+static bool gic_dist_supports_lpis(void)
+{
+    return (readl_relaxed(GICD + GICD_TYPER) & GICD_TYPE_LPIS);
+}
+
 /* Set up the GIC */
 static int __init gicv3_init(void)
 {
@@ -1699,9 +1704,12 @@ static int __init gicv3_init(void)
 
     gicv3_dist_init();
 
-    res = gicv3_its_init();
-    if ( res )
-        panic("GICv3: ITS: initialization failed: %d\n", res);
+    if ( gic_dist_supports_lpis() )
+    {
+        res = gicv3_its_init();
+        if ( res )
+            panic("GICv3: ITS: initialization failed: %d\n", res);
+    }
 
     res = gicv3_cpu_init();
     if ( res )
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 2/2] xen/arm: GICv3: Only initialize ITS when the distributor supports LPIs.

[Stefano Stabellini]

On Wed, 24 Jan 2018, Julien Grall wrote:
> There are firmware tables out describing the ITS but does not support
> LPIs. This will result to a data abort when trying to initialize ITS.
> 
> While this can be consider a bug in the Device-Tree, same configuration
> boots on Linux. So gate the ITS initialization with the support of LPIs
> in the distributor.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

> ---
>  xen/arch/arm/gic-v3.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 9f9cf59f82..730450e34b 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1637,6 +1637,11 @@ static unsigned long gicv3_get_hwdom_extra_madt_size(const struct domain *d)
>  }
>  #endif
>  
> +static bool gic_dist_supports_lpis(void)
> +{
> +    return (readl_relaxed(GICD + GICD_TYPER) & GICD_TYPE_LPIS);
> +}
> +
>  /* Set up the GIC */
>  static int __init gicv3_init(void)
>  {
> @@ -1699,9 +1704,12 @@ static int __init gicv3_init(void)
>  
>      gicv3_dist_init();
>  
> -    res = gicv3_its_init();
> -    if ( res )
> -        panic("GICv3: ITS: initialization failed: %d\n", res);
> +    if ( gic_dist_supports_lpis() )
> +    {
> +        res = gicv3_its_init();
> +        if ( res )
> +            panic("GICv3: ITS: initialization failed: %d\n", res);
> +    }
>  
>      res = gicv3_cpu_init();
>      if ( res )
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel