[PATCH v2 0/2] Fix Xen boot on XGene

[PATCH v2 0/2] Fix Xen boot on XGene

[Stefano Stabellini]

Hi all,

the following commit:

commit 21550029f709072aacf3b90edd574e7d3021b400
Author: Julien Grall <julien.grall@citrix.com>
Date:   Thu Oct 8 19:23:53 2015 +0100

    xen/arm: gic-v2: Automatically detect aliased GIC400


removed PLATFORM_QUIRK_GIC_64K_STRIDE and introduced an heuristic to
check whether the two GICC pages have a 64K stride. However the
heuristic needs device tree to report a GICC region size of 128K to work
properly. That is not the case for some versions of XGene (including the
one I am using, kindly provided by CloudLab.us).

The patch series fixes the issue by reintroducing platform quirks,
PLATFORM_QUIRK_GIC_64K_STRIDE, and forcing GICC size to 128K if
PLATFORM_QUIRK_GIC_64K_STRIDE.

We should consider this series for 4.8. 


Changes in v2:
- only set csize to SZ_128K if it is initially SZ_4K
- set vsize to match
- add warning if gicc is SZ_128K and not aliased


Stefano Stabellini (2):
      Revert "xen/arm: platform: Drop the quirks callback"
      Partially revert 21550029f709072aacf3b90edd574e7d3021b400

 xen/arch/arm/gic-v2.c                | 10 ++++++++--
 xen/arch/arm/platform.c              | 10 ++++++++++
 xen/arch/arm/platforms/xgene-storm.c |  6 ++++++
 xen/include/asm-arm/platform.h       | 13 +++++++++++++
 4 files changed, 37 insertions(+), 2 deletions(-)

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

[PATCH v2 1/2] Revert "xen/arm: platform: Drop the quirks callback"

[Stefano Stabellini]

This reverts commit 14fa16961b03a23e9b883e5f0ed06b6837a489d8.
Do not reintroduce platform_dom0_evtchn_ppi.

Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/arm/platform.c        | 10 ++++++++++
 xen/include/asm-arm/platform.h |  7 +++++++
 2 files changed, 17 insertions(+)

diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
index b0bfaa9..0af6d57 100644
--- a/xen/arch/arm/platform.c
+++ b/xen/arch/arm/platform.c
@@ -127,6 +127,16 @@ void platform_poweroff(void)
         platform->poweroff();
 }
 
+bool_t platform_has_quirk(uint32_t quirk)
+{
+    uint32_t quirks = 0;
+
+    if ( platform && platform->quirks )
+        quirks = platform->quirks();
+
+    return !!(quirks & quirk);
+}
+
 bool_t platform_device_is_blacklisted(const struct dt_device_node *node)
 {
     const struct dt_device_match *blacklist = NULL;
diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h
index f97315d..c6e5010 100644
--- a/xen/include/asm-arm/platform.h
+++ b/xen/include/asm-arm/platform.h
@@ -27,6 +27,12 @@ struct platform_desc {
     /* Platform power-off */
     void (*poweroff)(void);
     /*
+     * Platform quirks
+     * Defined has a function because a platform can support multiple
+     * board with different quirk on each
+     */
+    uint32_t (*quirks)(void);
+    /*
      * Platform blacklist devices
      * List of devices which must not pass-through to a guest
      */
@@ -42,6 +48,7 @@ int platform_cpu_up(int cpu);
 #endif
 void platform_reset(void);
 void platform_poweroff(void);
+bool_t platform_has_quirk(uint32_t quirk);
 bool_t platform_device_is_blacklisted(const struct dt_device_node *node);
 
 #define PLATFORM_START(_name, _namestr)                         \
-- 
1.9.1


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

[PATCH v2 2/2] Partially revert 21550029f709072aacf3b90edd574e7d3021b400

[Stefano Stabellini]

Commit 21550029f709072aacf3b90edd574e7d3021b400 removed the
PLATFORM_QUIRK_GIC_64K_STRIDE quirk and introduced a way to
automatically detect that the two GICC pages have a 64K stride.

However the heuristic requires that the device tree for the platform
reports a GICC size == 128K, which is not the case for some versions of
XGene.

Fix the issue by partially reverting
21550029f709072aacf3b90edd574e7d3021b400:

- reintroduce PLATFORM_QUIRK_GIC_64K_STRIDE for XGene
- force csize and vsize to SZ_128K if csize is initially 4K and if
  PLATFORM_QUIRK_GIC_64K_STRIDE

Also add a warning in case GICC is SZ_128K but not aliased.

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

---

Changes in v2:
- only set csize to SZ_128K if it is initially SZ_4K
- set vsize to match
- add warning if gicc is SZ_128K and not aliased
---
 xen/arch/arm/gic-v2.c                | 10 ++++++++--
 xen/arch/arm/platforms/xgene-storm.c |  6 ++++++
 xen/include/asm-arm/platform.h       |  6 ++++++
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 9bd9d0b..fd2f3b4 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -965,7 +965,10 @@ static void __init gicv2_dt_init(void)
         printk(XENLOG_WARNING "GICv2: WARNING: "
                "The GICC size is too small: %#"PRIx64" expected %#x\n",
                csize, SZ_8K);
-        csize = SZ_8K;
+        if ( platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
+            vsize = csize = SZ_128K;
+        else
+            csize = SZ_8K;
     }
 
     /*
@@ -1189,7 +1192,10 @@ static int __init gicv2_init(void)
         printk(XENLOG_WARNING
                "GICv2: Adjusting CPU interface base to %#"PRIx64"\n",
                cbase + aliased_offset);
-    }
+    } else if ( csize == SZ_128K )
+        printk(XENLOG_WARNING
+                "GICv2: GICC size=%lu but not aliased\n",
+                csize);
 
     gicv2.map_hbase = ioremap_nocache(hbase, PAGE_SIZE);
     if ( !gicv2.map_hbase )
diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
index 686b19b..c795a95 100644
--- a/xen/arch/arm/platforms/xgene-storm.c
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -67,6 +67,11 @@ static void __init xgene_check_pirq_eoi(void)
               "Please upgrade your firmware to the latest version");
 }
 
+static uint32_t xgene_storm_quirks(void)
+{
+    return PLATFORM_QUIRK_GIC_64K_STRIDE;
+}
+
 static void xgene_storm_reset(void)
 {
     void __iomem *addr;
@@ -116,6 +121,7 @@ PLATFORM_START(xgene_storm, "APM X-GENE STORM")
     .compatible = xgene_storm_dt_compat,
     .init = xgene_storm_init,
     .reset = xgene_storm_reset,
+    .quirks = xgene_storm_quirks,
 PLATFORM_END
 
 /*
diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h
index c6e5010..2ea9d61 100644
--- a/xen/include/asm-arm/platform.h
+++ b/xen/include/asm-arm/platform.h
@@ -39,6 +39,12 @@ struct platform_desc {
     const struct dt_device_match *blacklist_dev;
 };
 
+/*
+ * Quirk for platforms where the 4K GIC register ranges are placed at
+ * 64K stride.
+ */
+#define PLATFORM_QUIRK_GIC_64K_STRIDE (1 << 0)
+
 void __init platform_init(void);
 int __init platform_init_time(void);
 int __init platform_specific_mapping(struct domain *d);
-- 
1.9.1


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

Re: [PATCH v2 1/2] Revert "xen/arm: platform: Drop the quirks callback"

[Julien Grall]

Hi Stefano,

On 04/11/16 21:04, Stefano Stabellini wrote:
> This reverts commit 14fa16961b03a23e9b883e5f0ed06b6837a489d8.
> Do not reintroduce platform_dom0_evtchn_ppi.
>
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>

Acked-by: Julien Grall <julien.grall@arm.com>

Regards,

-- 
Julien Grall

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

Re: [PATCH v2 2/2] Partially revert 21550029f709072aacf3b90edd574e7d3021b400

[Julien Grall]

Hi Stefano,

On 04/11/16 21:04, Stefano Stabellini wrote:
> Commit 21550029f709072aacf3b90edd574e7d3021b400 removed the
> PLATFORM_QUIRK_GIC_64K_STRIDE quirk and introduced a way to
> automatically detect that the two GICC pages have a 64K stride.
>
> However the heuristic requires that the device tree for the platform
> reports a GICC size == 128K, which is not the case for some versions of
> XGene.
>
> Fix the issue by partially reverting
> 21550029f709072aacf3b90edd574e7d3021b400:
>
> - reintroduce PLATFORM_QUIRK_GIC_64K_STRIDE for XGene
> - force csize and vsize to SZ_128K if csize is initially 4K and if
>   PLATFORM_QUIRK_GIC_64K_STRIDE
>
> Also add a warning in case GICC is SZ_128K but not aliased.
>
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
>
> ---
>
> Changes in v2:
> - only set csize to SZ_128K if it is initially SZ_4K
> - set vsize to match
> - add warning if gicc is SZ_128K and not aliased
> ---
>  xen/arch/arm/gic-v2.c                | 10 ++++++++--
>  xen/arch/arm/platforms/xgene-storm.c |  6 ++++++
>  xen/include/asm-arm/platform.h       |  6 ++++++
>  3 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 9bd9d0b..fd2f3b4 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -965,7 +965,10 @@ static void __init gicv2_dt_init(void)
>          printk(XENLOG_WARNING "GICv2: WARNING: "
>                 "The GICC size is too small: %#"PRIx64" expected %#x\n",
>                 csize, SZ_8K);
> -        csize = SZ_8K;
> +        if ( platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
> +            vsize = csize = SZ_128K;

The function does not care of the vsize and does not update it when the 
size is wrong (see the else you add). So I would stay consistent and 
drop it.

Also, I would add a warning letting the user know that the quirk has 
been used.

> +        else
> +            csize = SZ_8K;
>      }
>
>      /*
> @@ -1189,7 +1192,10 @@ static int __init gicv2_init(void)
>          printk(XENLOG_WARNING
>                 "GICv2: Adjusting CPU interface base to %#"PRIx64"\n",
>                 cbase + aliased_offset);
> -    }
> +    } else if ( csize == SZ_128K )
> +        printk(XENLOG_WARNING
> +                "GICv2: GICC size=%lu but not aliased\n",
> +                csize);
>
>      gicv2.map_hbase = ioremap_nocache(hbase, PAGE_SIZE);
>      if ( !gicv2.map_hbase )
> diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
> index 686b19b..c795a95 100644
> --- a/xen/arch/arm/platforms/xgene-storm.c
> +++ b/xen/arch/arm/platforms/xgene-storm.c
> @@ -67,6 +67,11 @@ static void __init xgene_check_pirq_eoi(void)
>                "Please upgrade your firmware to the latest version");
>  }
>
> +static uint32_t xgene_storm_quirks(void)
> +{
> +    return PLATFORM_QUIRK_GIC_64K_STRIDE;
> +}
> +
>  static void xgene_storm_reset(void)
>  {
>      void __iomem *addr;
> @@ -116,6 +121,7 @@ PLATFORM_START(xgene_storm, "APM X-GENE STORM")
>      .compatible = xgene_storm_dt_compat,
>      .init = xgene_storm_init,
>      .reset = xgene_storm_reset,
> +    .quirks = xgene_storm_quirks,
>  PLATFORM_END
>
>  /*
> diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h
> index c6e5010..2ea9d61 100644
> --- a/xen/include/asm-arm/platform.h
> +++ b/xen/include/asm-arm/platform.h
> @@ -39,6 +39,12 @@ struct platform_desc {
>      const struct dt_device_match *blacklist_dev;
>  };
>
> +/*
> + * Quirk for platforms where the 4K GIC register ranges are placed at
> + * 64K stride.

I believe this comment needs to be updated as the quirk is only used 
when the device-tree reports 4K.

> + */
> +#define PLATFORM_QUIRK_GIC_64K_STRIDE (1 << 0)
> +
>  void __init platform_init(void);
>  int __init platform_init_time(void);
>  int __init platform_specific_mapping(struct domain *d);
>

Regards,

-- 
Julien Grall

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

Re: [PATCH v2 0/2] Fix Xen boot on XGene

[Wei Liu]

On Fri, Nov 04, 2016 at 02:04:21PM -0700, Stefano Stabellini wrote:
> Hi all,
> 
> the following commit:
> 
> commit 21550029f709072aacf3b90edd574e7d3021b400
> Author: Julien Grall <julien.grall@citrix.com>
> Date:   Thu Oct 8 19:23:53 2015 +0100
> 
>     xen/arm: gic-v2: Automatically detect aliased GIC400
> 
> 
> removed PLATFORM_QUIRK_GIC_64K_STRIDE and introduced an heuristic to
> check whether the two GICC pages have a 64K stride. However the
> heuristic needs device tree to report a GICC region size of 128K to work
> properly. That is not the case for some versions of XGene (including the
> one I am using, kindly provided by CloudLab.us).
> 
> The patch series fixes the issue by reintroducing platform quirks,
> PLATFORM_QUIRK_GIC_64K_STRIDE, and forcing GICC size to 128K if
> PLATFORM_QUIRK_GIC_64K_STRIDE.
> 
> We should consider this series for 4.8. 

I think this patch series is rather self-contained:

Release-acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH v2 2/2] Partially revert 21550029f709072aacf3b90edd574e7d3021b400

[Stefano Stabellini]

On Mon, 7 Nov 2016, Julien Grall wrote:
> Hi Stefano,
> 
> On 04/11/16 21:04, Stefano Stabellini wrote:
> > Commit 21550029f709072aacf3b90edd574e7d3021b400 removed the
> > PLATFORM_QUIRK_GIC_64K_STRIDE quirk and introduced a way to
> > automatically detect that the two GICC pages have a 64K stride.
> > 
> > However the heuristic requires that the device tree for the platform
> > reports a GICC size == 128K, which is not the case for some versions of
> > XGene.
> > 
> > Fix the issue by partially reverting
> > 21550029f709072aacf3b90edd574e7d3021b400:
> > 
> > - reintroduce PLATFORM_QUIRK_GIC_64K_STRIDE for XGene
> > - force csize and vsize to SZ_128K if csize is initially 4K and if
> >   PLATFORM_QUIRK_GIC_64K_STRIDE
> > 
> > Also add a warning in case GICC is SZ_128K but not aliased.
> > 
> > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> > 
> > ---
> > 
> > Changes in v2:
> > - only set csize to SZ_128K if it is initially SZ_4K
> > - set vsize to match
> > - add warning if gicc is SZ_128K and not aliased
> > ---
> >  xen/arch/arm/gic-v2.c                | 10 ++++++++--
> >  xen/arch/arm/platforms/xgene-storm.c |  6 ++++++
> >  xen/include/asm-arm/platform.h       |  6 ++++++
> >  3 files changed, 20 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> > index 9bd9d0b..fd2f3b4 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -965,7 +965,10 @@ static void __init gicv2_dt_init(void)
> >          printk(XENLOG_WARNING "GICv2: WARNING: "
> >                 "The GICC size is too small: %#"PRIx64" expected %#x\n",
> >                 csize, SZ_8K);
> > -        csize = SZ_8K;
> > +        if ( platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
> > +            vsize = csize = SZ_128K;
> 
> The function does not care of the vsize and does not update it when the size
> is wrong (see the else you add). So I would stay consistent and drop it.

Actually a couple of lines below in the code:

  if ( csize != vsize )
      panic("GICv2: Sizes of GICC (%#"PRIpaddr") and GICV (%#"PRIpaddr") don't match\n",
             csize, vsize);

I think we need to keep it.


> Also, I would add a warning letting the user know that the quirk has been
> used.

Make sense.


> > +        else
> > +            csize = SZ_8K;
> >      }
> > 
> >      /*
> > @@ -1189,7 +1192,10 @@ static int __init gicv2_init(void)
> >          printk(XENLOG_WARNING
> >                 "GICv2: Adjusting CPU interface base to %#"PRIx64"\n",
> >                 cbase + aliased_offset);
> > -    }
> > +    } else if ( csize == SZ_128K )
> > +        printk(XENLOG_WARNING
> > +                "GICv2: GICC size=%lu but not aliased\n",
> > +                csize);
> > 
> >      gicv2.map_hbase = ioremap_nocache(hbase, PAGE_SIZE);
> >      if ( !gicv2.map_hbase )
> > diff --git a/xen/arch/arm/platforms/xgene-storm.c
> > b/xen/arch/arm/platforms/xgene-storm.c
> > index 686b19b..c795a95 100644
> > --- a/xen/arch/arm/platforms/xgene-storm.c
> > +++ b/xen/arch/arm/platforms/xgene-storm.c
> > @@ -67,6 +67,11 @@ static void __init xgene_check_pirq_eoi(void)
> >                "Please upgrade your firmware to the latest version");
> >  }
> > 
> > +static uint32_t xgene_storm_quirks(void)
> > +{
> > +    return PLATFORM_QUIRK_GIC_64K_STRIDE;
> > +}
> > +
> >  static void xgene_storm_reset(void)
> >  {
> >      void __iomem *addr;
> > @@ -116,6 +121,7 @@ PLATFORM_START(xgene_storm, "APM X-GENE STORM")
> >      .compatible = xgene_storm_dt_compat,
> >      .init = xgene_storm_init,
> >      .reset = xgene_storm_reset,
> > +    .quirks = xgene_storm_quirks,
> >  PLATFORM_END
> > 
> >  /*
> > diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h
> > index c6e5010..2ea9d61 100644
> > --- a/xen/include/asm-arm/platform.h
> > +++ b/xen/include/asm-arm/platform.h
> > @@ -39,6 +39,12 @@ struct platform_desc {
> >      const struct dt_device_match *blacklist_dev;
> >  };
> > 
> > +/*
> > + * Quirk for platforms where the 4K GIC register ranges are placed at
> > + * 64K stride.
> 
> I believe this comment needs to be updated as the quirk is only used when the
> device-tree reports 4K.
> 
> > + */
> > +#define PLATFORM_QUIRK_GIC_64K_STRIDE (1 << 0)
> > +
> >  void __init platform_init(void);
> >  int __init platform_init_time(void);
> >  int __init platform_specific_mapping(struct domain *d);
> > 
> 
> Regards,
> 
> -- 
> Julien Grall
> 

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