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

[Stefano Stabellini <sstabellini@kernel.org>]

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