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

[Marc Zyngier <maz@kernel.org>]

On Mon, 21 Feb 2022 10:24:36 +0000,
Michael Walle <michael@walle.cc> wrote:
> 
> Hi,
> 
> Am 2022-02-21 11:16, schrieb Wasim Khan:
> > From: Wasim Khan <wasim.khan@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@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.