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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > 
> > ---
> > 
> > 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel