[PATCH] armv8: fsl-layerscape: use previous aligned address for gic_lpi_base

[Wasim Khan (OSS)]

Hi Marc, Zhiqiang

> -----Original Message-----
> From: Marc Zyngier <maz at kernel.org>
> Sent: Monday, February 21, 2022 4:23 PM
> To: Michael Walle <michael at walle.cc>
> Cc: Wasim Khan (OSS) <wasim.khan at oss.nxp.com>; sjg at chromium.org;
> Priyanka Jain <priyanka.jain at nxp.com>; treding at nvidia.com;
> twarren at nvidia.com; Varun Sethi <V.Sethi at nxp.com>; u-
> boot at lists.denx.de; Wasim Khan <wasim.khan at nxp.com>
> Subject: Re: [PATCH] armv8: fsl-layerscape: use previous aligned address for
> gic_lpi_base
> 
> On Mon, 21 Feb 2022 10:24:36 +0000,
> Michael Walle <michael at walle.cc> wrote:
> >
> > Hi,
> >
> > Am 2022-02-21 11:16, schrieb Wasim Khan:
> > > From: Wasim Khan <wasim.khan at nxp.com>
> > >
> > > Memory after gd->arch.resv_ram is reserved for MC block.
> > > Use ALIGN_DOWN to avoid updating MC block for unaligned address.
> >
> > I cannot really tell what you are trying to do here. But I know Marc
> > has offered to also take a look at the GIC/LPI stuff. So I've put him
> > on CC.
> >
> > -michael
> >
> > >
> > > Signed-off-by: Wasim Khan <wasim.khan at nxp.com>
> > > ---
> > >  arch/arm/cpu/armv8/fsl-layerscape/soc.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> > > b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> > > index d3a5cfaac1..746c93cf51 100644
> > > --- a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> > > +++ b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> > > @@ -65,7 +65,7 @@ int ls_gic_rd_tables_init(void *blob)
> > >  	u64 gic_lpi_base;
> > >  	int ret;
> > >
> > > -	gic_lpi_base = ALIGN(gd->arch.resv_ram - GIC_LPI_SIZE, SZ_64K);
> > > +	gic_lpi_base = ALIGN_DOWN(gd->arch.resv_ram - GIC_LPI_SIZE,
> > > +SZ_64K);
> > >  	ret = fdt_add_resv_mem_gic_rd_tables(blob, gic_lpi_base,
> > > GIC_LPI_SIZE);
> > >  	if (ret)
> > >  		return ret;
> >
> 
> It is the usual accumulation of nonsense. We have
> 
> #define ITS_MAX_LPI_NRBITS        16
> 
> which is not necessarily what the HW exposes
> 
> #define PENDTABLE_MAX_SZ        ALIGN(BIT(ITS_MAX_LPI_NRBITS), SZ_64K)
> 
> The *base* of the pending table has to be 64kB aligned, but not its size. Yes,
> that's a helpful shortcut, but that's still wrong.
> 
> #define PROPTABLE_MAX_SZ        ALIGN(BIT(ITS_MAX_LPI_NRBITS) / 8,
> SZ_64K)
> 
> This 64kB alignment is silly, specially considering the hardcoding of the
> number of ID bits.
> 
> #define GIC_LPI_SIZE            ALIGN(cpu_numcores() * PENDTABLE_MAX_SZ +
> \
>                                 PROPTABLE_MAX_SZ, SZ_1M)
> 
> This 1MB alignment doesn't exist. Convenience again?
> 
> And then this patch adds some bizarre alignment for reasons that have
> nothing to do with the GIC, but because there is so other reservations this
> steps on, which probably means that the allocator is doing something wrong...
> 
> The whole thing needs reworking from first principle, at which point it will
> become clearer what this is trying to do.
> 
> 	M.
> 
> --
> Without deviation from the norm, progress is not possible.


Added Zhiqiang to respond to Marc's questions.


Regarding this patch:
if gd->arch.resv_ram points to some address which is not 64K aligned (as per current code), Using ALIGN can be problematic in that case.

Ex:
Currently MC reserved regions is [0x27_8000_0000 , 0x27_FFFF_FFFF]
gd->arch.resv_ram point to 0x27_8000_0000. 

Now suppose if I have requirement to reserve a block (say XYZ) of 32KB size just before MC block,  i will update gd->arch.resv_ram to point to 0x27_7FFF_8000.
Now, Calling ' gic_lpi_base = ALIGN(gd->arch.resv_ram - GIC_LPI_SIZE, SZ_64K);' will eat up space reserved for XYZ.

So, I think ALIGN_DOWN should be used for gic_lpi_base .
gic_lpi_base = ALIGN_DOWN(gd->arch.resv_ram - GIC_LPI_SIZE, SZ_64K);