[PATCH 0/2] Fix Xen boot on XGene

[PATCH 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. 



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 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 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 to SZ_128K for mapping if PLATFORM_QUIRK_GIC_64K_STRIDE

Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
---
 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..2c6b8f1 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -1146,6 +1146,7 @@ static int gicv2_make_hwdom_madt(const struct domain *d, u32 offset)
 static int __init gicv2_init(void)
 {
     uint32_t aliased_offset = 0;
+    paddr_t csize_map;
 
     if ( acpi_disabled )
         gicv2_dt_init();
@@ -1165,15 +1166,20 @@ static int __init gicv2_init(void)
          (hbase & ~PAGE_MASK) || (vbase & ~PAGE_MASK) )
         panic("GICv2 interfaces not page aligned");
 
+    if ( platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
+        csize_map = SZ_128K;
+    else
+        csize_map = csize;
+
     gicv2.map_dbase = ioremap_nocache(dbase, PAGE_SIZE);
     if ( !gicv2.map_dbase )
         panic("GICv2: Failed to ioremap for GIC distributor\n");
 
-    gicv2.map_cbase = ioremap_nocache(cbase, csize);
+    gicv2.map_cbase = ioremap_nocache(cbase, csize_map);
     if ( !gicv2.map_cbase )
         panic("GICv2: Failed to ioremap for GIC CPU interface\n");
 
-    if ( gicv2_is_aliased(cbase, csize) )
+    if ( gicv2_is_aliased(cbase, csize_map) )
     {
         /*
          * Move the base up by 60kB, so that we have a 8kB contiguous
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 2/2] Partially revert 21550029f709072aacf3b90edd574e7d3021b400

[Wei Liu]

On Tue, Oct 25, 2016 at 03:46:55PM -0700, 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 to SZ_128K for mapping if PLATFORM_QUIRK_GIC_64K_STRIDE
> 
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>

Sounds like a reasonable approach to me.

Code-wise also looks good.

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

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

[Wei Liu]

On Tue, Oct 25, 2016 at 03:46:54PM -0700, Stefano Stabellini wrote:
> This reverts commit 14fa16961b03a23e9b883e5f0ed06b6837a489d8.
> Do not reintroduce platform_dom0_evtchn_ppi.
> 
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>

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

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

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

[Julien Grall]

[-- Attachment #1.1: Type: text/plain, Size: 1222 bytes --]

Hi,

Apologize for not respecting the netiquette.

On Tue, 25 Oct 2016, 5:49 p.m. Stefano Stabellini, <sstabellini@kernel.org>
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.
>

The platform you are using has likely a buggy firmware (I cannot find the
mail from APM stating that).

I am not convinced that we should support a such case. IIRC unmodified
Linux will not work properly on those platform (e.g the GIC will be drived
as a GICv1).

Cheers,

-- 
Julien Grall

[-- Attachment #1.2: Type: text/html, Size: 2152 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

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

[Stefano Stabellini]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2179 bytes --]

On Wed, 26 Oct 2016, Julien Grall wrote:
> Hi,
> Apologize for not respecting the netiquette.
> 
> On Tue, 25 Oct 2016, 5:49 p.m. Stefano Stabellini, <sstabellini@kernel.org> 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.
> 
> 
> The platform you are using has likely a buggy firmware (I cannot find the mail from
> APM stating that).
> 
> I am not convinced that we should support a such case. IIRC unmodified Linux will
> not work properly on those platform (e.g the GIC will be drived as a GICv1).

I agree with you that it is buggy firmware, but unfortunately it is
still out there, deployed. I am using a regular unmodified old Linux
kernel (4.0) which works fine (and should still work fine with modern
Xen hypervisors). I believe this DTS comes from upstream Linux 4.0.

The question is: should we carry this workaround to make our users' life
easier? Or should we push back the burden of fixing the firmware to the
users? There are valid arguments for both, eventually it comes down to
finding the right compromise.

To be clear I don't feel strongly either way, but I think it is easier
for us to carry this workaround than for our users to update the
firmware. So in this case I would take these patches. But it might not
be the case in the future for other, more invasive, workarounds. Let me
know what you think.

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

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

[Julien Grall]

Hi Stefano,

On 26/10/16 23:12, Stefano Stabellini wrote:
> On Wed, 26 Oct 2016, Julien Grall wrote:
>> Hi,
>> Apologize for not respecting the netiquette.
>>
>> On Tue, 25 Oct 2016, 5:49 p.m. Stefano Stabellini, <sstabellini@kernel.org> 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.
>>
>>
>> The platform you are using has likely a buggy firmware (I cannot find the mail from
>> APM stating that).
>>
>> I am not convinced that we should support a such case. IIRC unmodified Linux will
>> not work properly on those platform (e.g the GIC will be drived as a GICv1).
>
> I agree with you that it is buggy firmware, but unfortunately it is
> still out there, deployed. I am using a regular unmodified old Linux
> kernel (4.0) which works fine (and should still work fine with modern
> Xen hypervisors). I believe this DTS comes from upstream Linux 4.0.
>
> The question is: should we carry this workaround to make our users' life
> easier? Or should we push back the burden of fixing the firmware to the
> users? There are valid arguments for both, eventually it comes down to
> finding the right compromise.

In general it is better to get the newest firmware as other unrelated 
bugs may be present on older version or there is missing workaround for 
broken hardware (e.g enabling chicken bits). For instance, we have 
already decided to not support some version of the X-gene firmware (see 
commit 784c2d1 "xen/arm: Drop support of platform where GICH_LR_HW is 
not working correctly").

In this specific case, you assume that the GIC device tree node will 
always point to the beginning of the first 64K page. It might be 
possible in the future to have an updated firmware pointing to the last 
alias of the first page, so the second 4K page will follow directly.

>
> To be clear I don't feel strongly either way, but I think it is easier
> for us to carry this workaround than for our users to update the
> firmware. So in this case I would take these patches. But it might not
> be the case in the future for other, more invasive, workarounds. Let me
> know what you think.

I would much prefer something in Xen to check whether the user has to 
upgrade his firmware based on known broken features.

Regards,

-- 
Julien Grall

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

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

[Stefano Stabellini]

On Mon, 31 Oct 2016, Julien Grall wrote:
> Hi Stefano,
> 
> On 26/10/16 23:12, Stefano Stabellini wrote:
> > On Wed, 26 Oct 2016, Julien Grall wrote:
> > > Hi,
> > > Apologize for not respecting the netiquette.
> > > 
> > > On Tue, 25 Oct 2016, 5:49 p.m. Stefano Stabellini,
> > > <sstabellini@kernel.org> 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.
> > > 
> > > 
> > > The platform you are using has likely a buggy firmware (I cannot find the
> > > mail from
> > > APM stating that).
> > > 
> > > I am not convinced that we should support a such case. IIRC unmodified
> > > Linux will
> > > not work properly on those platform (e.g the GIC will be drived as a
> > > GICv1).
> > 
> > I agree with you that it is buggy firmware, but unfortunately it is
> > still out there, deployed. I am using a regular unmodified old Linux
> > kernel (4.0) which works fine (and should still work fine with modern
> > Xen hypervisors). I believe this DTS comes from upstream Linux 4.0.
> > 
> > The question is: should we carry this workaround to make our users' life
> > easier? Or should we push back the burden of fixing the firmware to the
> > users? There are valid arguments for both, eventually it comes down to
> > finding the right compromise.
> 
> In general it is better to get the newest firmware as other unrelated bugs may
> be present on older version or there is missing workaround for broken hardware
> (e.g enabling chicken bits). For instance, we have already decided to not
> support some version of the X-gene firmware (see commit 784c2d1 "xen/arm: Drop
> support of platform where GICH_LR_HW is not working correctly").
> 
> In this specific case, you assume that the GIC device tree node will always
> point to the beginning of the first 64K page. It might be possible in the
> future to have an updated firmware pointing to the last alias of the first
> page, so the second 4K page will follow directly.

Such firmware could have a version number we could check to disable
PLATFORM_QUIRK_GIC_64K_STRIDE.


> > To be clear I don't feel strongly either way, but I think it is easier
> > for us to carry this workaround than for our users to update the
> > firmware. So in this case I would take these patches. But it might not
> > be the case in the future for other, more invasive, workarounds. Let me
> > know what you think.
> 
> I would much prefer something in Xen to check whether the user has to upgrade
> his firmware based on known broken features.

Yes, we should warn the user in any case, that would be helpful. I can
produce a patch for that.

Regarding firmware updates, for example in this case I cannot actually
perform one, as I don't control the environment, I am just a cloud user
like any other. I can ask u-boot to use a different DTB, but then I
cannot provide a good one because the device tree for m400 has not been
properly pushed upstream in Linux. In other words, there isn't really a
good update path on this platform.

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

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

[Julien Grall]

Hi Stefano,

On 01/11/2016 19:29, Stefano Stabellini wrote:
> On Mon, 31 Oct 2016, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 26/10/16 23:12, Stefano Stabellini wrote:
>>> On Wed, 26 Oct 2016, Julien Grall wrote:
>>>> Hi,
>>>> Apologize for not respecting the netiquette.
>>>>
>>>> On Tue, 25 Oct 2016, 5:49 p.m. Stefano Stabellini,
>>>> <sstabellini@kernel.org> 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.
>>>>
>>>>
>>>> The platform you are using has likely a buggy firmware (I cannot find the
>>>> mail from
>>>> APM stating that).
>>>>
>>>> I am not convinced that we should support a such case. IIRC unmodified
>>>> Linux will
>>>> not work properly on those platform (e.g the GIC will be drived as a
>>>> GICv1).
>>>
>>> I agree with you that it is buggy firmware, but unfortunately it is
>>> still out there, deployed. I am using a regular unmodified old Linux
>>> kernel (4.0) which works fine (and should still work fine with modern
>>> Xen hypervisors). I believe this DTS comes from upstream Linux 4.0.
>>>
>>> The question is: should we carry this workaround to make our users' life
>>> easier? Or should we push back the burden of fixing the firmware to the
>>> users? There are valid arguments for both, eventually it comes down to
>>> finding the right compromise.
>>
>> In general it is better to get the newest firmware as other unrelated bugs may
>> be present on older version or there is missing workaround for broken hardware
>> (e.g enabling chicken bits). For instance, we have already decided to not
>> support some version of the X-gene firmware (see commit 784c2d1 "xen/arm: Drop
>> support of platform where GICH_LR_HW is not working correctly").
>>
>> In this specific case, you assume that the GIC device tree node will always
>> point to the beginning of the first 64K page. It might be possible in the
>> future to have an updated firmware pointing to the last alias of the first
>> page, so the second 4K page will follow directly.
>
> Such firmware could have a version number we could check to disable
> PLATFORM_QUIRK_GIC_64K_STRIDE.

Is there any way to retrieve the firmware version number from Xen?

Also, you mentioned later that it is possible to provide a different DTB 
with U-boot on m410. A user could decide to provide a modified one (e.g 
pointing to a different base address) and will not be possible to
boot Xen because the quirk will screw up the base addresses.

I am wondering if you could turn on the quirk only when certain 
condition met (like a given GIC base address and the size is not 128K). 
This would save us some future trouble and that would address my 
concern. What do you think?

>
>>> To be clear I don't feel strongly either way, but I think it is easier
>>> for us to carry this workaround than for our users to update the
>>> firmware. So in this case I would take these patches. But it might not
>>> be the case in the future for other, more invasive, workarounds. Let me
>>> know what you think.
>>
>> I would much prefer something in Xen to check whether the user has to upgrade
>> his firmware based on known broken features.
>
> Yes, we should warn the user in any case, that would be helpful. I can
> produce a patch for that.
>
> Regarding firmware updates, for example in this case I cannot actually
> perform one, as I don't control the environment, I am just a cloud user
> like any other. I can ask u-boot to use a different DTB, but then I
> cannot provide a good one because the device tree for m400 has not been
> properly pushed upstream in Linux. In other words, there isn't really a
> good update path on this platform.

As I mentioned running an older firmware is usually a very bad idea. On 
ARM, the firmware will usually contain work-around for some errata when 
there are chicken bits available. So no matter the outcome of this 
discussion, I would strongly recommend to request the owner to upgrade 
the firmware.

You could extract the device-tree from the platform (via 
/proc/device-tree) and modifying it.

Regards,

-- 
Julien Grall

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

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

[Stefano Stabellini]

On Wed, 2 Nov 2016, Julien Grall wrote:
> Hi Stefano,
> 
> On 01/11/2016 19:29, Stefano Stabellini wrote:
> > On Mon, 31 Oct 2016, Julien Grall wrote:
> > > Hi Stefano,
> > > 
> > > On 26/10/16 23:12, Stefano Stabellini wrote:
> > > > On Wed, 26 Oct 2016, Julien Grall wrote:
> > > > > Hi,
> > > > > Apologize for not respecting the netiquette.
> > > > > 
> > > > > On Tue, 25 Oct 2016, 5:49 p.m. Stefano Stabellini,
> > > > > <sstabellini@kernel.org> 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.
> > > > > 
> > > > > 
> > > > > The platform you are using has likely a buggy firmware (I cannot find
> > > > > the
> > > > > mail from
> > > > > APM stating that).
> > > > > 
> > > > > I am not convinced that we should support a such case. IIRC unmodified
> > > > > Linux will
> > > > > not work properly on those platform (e.g the GIC will be drived as a
> > > > > GICv1).
> > > > 
> > > > I agree with you that it is buggy firmware, but unfortunately it is
> > > > still out there, deployed. I am using a regular unmodified old Linux
> > > > kernel (4.0) which works fine (and should still work fine with modern
> > > > Xen hypervisors). I believe this DTS comes from upstream Linux 4.0.
> > > > 
> > > > The question is: should we carry this workaround to make our users' life
> > > > easier? Or should we push back the burden of fixing the firmware to the
> > > > users? There are valid arguments for both, eventually it comes down to
> > > > finding the right compromise.
> > > 
> > > In general it is better to get the newest firmware as other unrelated bugs
> > > may
> > > be present on older version or there is missing workaround for broken
> > > hardware
> > > (e.g enabling chicken bits). For instance, we have already decided to not
> > > support some version of the X-gene firmware (see commit 784c2d1 "xen/arm:
> > > Drop
> > > support of platform where GICH_LR_HW is not working correctly").
> > > 
> > > In this specific case, you assume that the GIC device tree node will
> > > always
> > > point to the beginning of the first 64K page. It might be possible in the
> > > future to have an updated firmware pointing to the last alias of the first
> > > page, so the second 4K page will follow directly.
> > 
> > Such firmware could have a version number we could check to disable
> > PLATFORM_QUIRK_GIC_64K_STRIDE.
> 
> Is there any way to retrieve the firmware version number from Xen?
> 
> Also, you mentioned later that it is possible to provide a different DTB with
> U-boot on m410. A user could decide to provide a modified one (e.g pointing to
> a different base address) and will not be possible to
> boot Xen because the quirk will screw up the base addresses.
> 
> I am wondering if you could turn on the quirk only when certain condition met
> (like a given GIC base address and the size is not 128K). This would save us
> some future trouble and that would address my concern. What do you think?

I think you are right. Concidentally the csize is 4K instead of 8K with
this DTB, we already have a workaround for that in gicv2_dt_init. I am
thinking of checking PLATFORM_QUIRK_GIC_64K_STRIDE only if the csize is
4K, which we know it is wrong. Good dtbs wouldn't be affected.

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