* [PATCH 0/2] Fix Xen boot on XGene
@ 2016-10-25 22:46 Stefano Stabellini
2016-10-25 22:46 ` [PATCH 1/2] Revert "xen/arm: platform: Drop the quirks callback" Stefano Stabellini
2016-10-26 17:08 ` [PATCH 0/2] Fix Xen boot on XGene Julien Grall
0 siblings, 2 replies; 11+ messages in thread
From: Stefano Stabellini @ 2016-10-25 22:46 UTC (permalink / raw)
To: xen-devel; +Cc: julien.grall, sstabellini, wei.liu2
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
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/2] Revert "xen/arm: platform: Drop the quirks callback" 2016-10-25 22:46 [PATCH 0/2] Fix Xen boot on XGene Stefano Stabellini @ 2016-10-25 22:46 ` Stefano Stabellini 2016-10-25 22:46 ` [PATCH 2/2] Partially revert 21550029f709072aacf3b90edd574e7d3021b400 Stefano Stabellini 2016-10-26 11:27 ` [PATCH 1/2] Revert "xen/arm: platform: Drop the quirks callback" Wei Liu 2016-10-26 17:08 ` [PATCH 0/2] Fix Xen boot on XGene Julien Grall 1 sibling, 2 replies; 11+ messages in thread From: Stefano Stabellini @ 2016-10-25 22:46 UTC (permalink / raw) To: xen-devel; +Cc: julien.grall, sstabellini, wei.liu2 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] Partially revert 21550029f709072aacf3b90edd574e7d3021b400 2016-10-25 22:46 ` [PATCH 1/2] Revert "xen/arm: platform: Drop the quirks callback" Stefano Stabellini @ 2016-10-25 22:46 ` Stefano Stabellini 2016-10-26 11:27 ` Wei Liu 2016-10-26 11:27 ` [PATCH 1/2] Revert "xen/arm: platform: Drop the quirks callback" Wei Liu 1 sibling, 1 reply; 11+ messages in thread From: Stefano Stabellini @ 2016-10-25 22:46 UTC (permalink / raw) To: xen-devel; +Cc: julien.grall, sstabellini, wei.liu2 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] Partially revert 21550029f709072aacf3b90edd574e7d3021b400 2016-10-25 22:46 ` [PATCH 2/2] Partially revert 21550029f709072aacf3b90edd574e7d3021b400 Stefano Stabellini @ 2016-10-26 11:27 ` Wei Liu 0 siblings, 0 replies; 11+ messages in thread From: Wei Liu @ 2016-10-26 11:27 UTC (permalink / raw) To: Stefano Stabellini; +Cc: xen-devel, julien.grall, wei.liu2 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] Revert "xen/arm: platform: Drop the quirks callback" 2016-10-25 22:46 ` [PATCH 1/2] Revert "xen/arm: platform: Drop the quirks callback" Stefano Stabellini 2016-10-25 22:46 ` [PATCH 2/2] Partially revert 21550029f709072aacf3b90edd574e7d3021b400 Stefano Stabellini @ 2016-10-26 11:27 ` Wei Liu 1 sibling, 0 replies; 11+ messages in thread From: Wei Liu @ 2016-10-26 11:27 UTC (permalink / raw) To: Stefano Stabellini; +Cc: xen-devel, julien.grall, wei.liu2 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] Fix Xen boot on XGene 2016-10-25 22:46 [PATCH 0/2] Fix Xen boot on XGene Stefano Stabellini 2016-10-25 22:46 ` [PATCH 1/2] Revert "xen/arm: platform: Drop the quirks callback" Stefano Stabellini @ 2016-10-26 17:08 ` Julien Grall 2016-10-26 22:12 ` Stefano Stabellini 1 sibling, 1 reply; 11+ messages in thread From: Julien Grall @ 2016-10-26 17:08 UTC (permalink / raw) To: Stefano Stabellini, xen-devel; +Cc: julien.grall, wei.liu2 [-- 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] Fix Xen boot on XGene 2016-10-26 17:08 ` [PATCH 0/2] Fix Xen boot on XGene Julien Grall @ 2016-10-26 22:12 ` Stefano Stabellini 2016-10-31 14:05 ` Julien Grall 0 siblings, 1 reply; 11+ messages in thread From: Stefano Stabellini @ 2016-10-26 22:12 UTC (permalink / raw) To: Julien Grall; +Cc: xen-devel, julien.grall, Stefano Stabellini, wei.liu2 [-- 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] Fix Xen boot on XGene 2016-10-26 22:12 ` Stefano Stabellini @ 2016-10-31 14:05 ` Julien Grall 2016-11-01 19:29 ` Stefano Stabellini 0 siblings, 1 reply; 11+ messages in thread From: Julien Grall @ 2016-10-31 14:05 UTC (permalink / raw) To: Stefano Stabellini, Julien Grall; +Cc: xen-devel, wei.liu2 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] Fix Xen boot on XGene 2016-10-31 14:05 ` Julien Grall @ 2016-11-01 19:29 ` Stefano Stabellini 2016-11-02 10:20 ` Julien Grall 0 siblings, 1 reply; 11+ messages in thread From: Stefano Stabellini @ 2016-11-01 19:29 UTC (permalink / raw) To: Julien Grall; +Cc: Julien Grall, xen-devel, Stefano Stabellini, wei.liu2 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] Fix Xen boot on XGene 2016-11-01 19:29 ` Stefano Stabellini @ 2016-11-02 10:20 ` Julien Grall 2016-11-04 20:24 ` Stefano Stabellini 0 siblings, 1 reply; 11+ messages in thread From: Julien Grall @ 2016-11-02 10:20 UTC (permalink / raw) To: Stefano Stabellini; +Cc: Julien Grall, xen-devel, wei.liu2, Steve Capper 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] Fix Xen boot on XGene 2016-11-02 10:20 ` Julien Grall @ 2016-11-04 20:24 ` Stefano Stabellini 0 siblings, 0 replies; 11+ messages in thread From: Stefano Stabellini @ 2016-11-04 20:24 UTC (permalink / raw) To: Julien Grall Cc: Julien Grall, xen-devel, Stefano Stabellini, wei.liu2, Steve Capper 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-11-04 20:24 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-10-25 22:46 [PATCH 0/2] Fix Xen boot on XGene Stefano Stabellini 2016-10-25 22:46 ` [PATCH 1/2] Revert "xen/arm: platform: Drop the quirks callback" Stefano Stabellini 2016-10-25 22:46 ` [PATCH 2/2] Partially revert 21550029f709072aacf3b90edd574e7d3021b400 Stefano Stabellini 2016-10-26 11:27 ` Wei Liu 2016-10-26 11:27 ` [PATCH 1/2] Revert "xen/arm: platform: Drop the quirks callback" Wei Liu 2016-10-26 17:08 ` [PATCH 0/2] Fix Xen boot on XGene Julien Grall 2016-10-26 22:12 ` Stefano Stabellini 2016-10-31 14:05 ` Julien Grall 2016-11-01 19:29 ` Stefano Stabellini 2016-11-02 10:20 ` Julien Grall 2016-11-04 20:24 ` Stefano Stabellini
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.