Re: [PATCH 2/2] Revert "arch: arm: use dt and UCLASS_SYSCON to get gic lpi details"

[Marc Zyngier <maz@kernel.org>]

On Sun, 31 Oct 2021 16:45:41 +0000,
"Z.Q. Hou" <zhiqiang.hou@nxp.com> wrote:
> 
> 
> 
> > -----Original Message-----
> > From: Marc Zyngier [mailto:maz@kernel.org]
> > Sent: 2021年10月29日 5:09
> > To: Michael Walle <michael@walle.cc>
> > Cc: u-boot@lists.denx.de; Vladimir Oltean <vladimir.oltean@nxp.com>; Z.Q. Hou
> > <zhiqiang.hou@nxp.com>; Bharat Gooty <bharat.gooty@broadcom.com>;
> > Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>; Simon Glass
> > <sjg@chromium.org>; Priyanka Jain <priyanka.jain@nxp.com>; Tom Rini
> > <trini@konsulko.com>
> > Subject: Re: [PATCH 2/2] Revert "arch: arm: use dt and UCLASS_SYSCON to get gic
> > lpi details"
> > 
> > On Wed, 27 Oct 2021 17:54:54 +0100,
> > Michael Walle <michael@walle.cc> wrote:
> > >
> > > Stop using the device tree as a source for ad-hoc information.
> > >
> > > This reverts commit 2ae7adc659f7fca9ea65df4318e5bca2b8274310.
> > >
> > > Signed-off-by: Michael Walle <michael@walle.cc>
> > > ---
> > >  arch/arm/Kconfig                        |  2 -
> > >  arch/arm/cpu/armv8/fsl-layerscape/soc.c | 27 +++++++++-
> > >  arch/arm/include/asm/gic-v3.h           |  4 +-
> > >  arch/arm/lib/gic-v3-its.c               | 66 +++----------------------
> > >  4 files changed, 36 insertions(+), 63 deletions(-)
> > >
> > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index
> > > 02f8306f15..86c1ebde05 100644
> > > --- a/arch/arm/Kconfig
> > > +++ b/arch/arm/Kconfig
> > > @@ -82,8 +82,6 @@ config GICV3
> > >
> > >  config GIC_V3_ITS
> > >  	bool "ARM GICV3 ITS"
> > > -	select REGMAP
> > > -	select SYSCON
> > >  	select IRQ
> > >  	help
> > >  	  ARM GICV3 Interrupt translation service (ITS).
> > > diff --git a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> > > b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> > > index c0e100d21c..a08ed3f544 100644
> > > --- a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> > > +++ b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> > 
> > Why is this FSL specific?
> > 
> > > @@ -41,11 +41,36 @@ DECLARE_GLOBAL_DATA_PTR;  #endif
> > >
> > >  #ifdef CONFIG_GIC_V3_ITS
> > > +#define PENDTABLE_MAX_SZ	ALIGN(BIT(ITS_MAX_LPI_NRBITS), SZ_64K)
> > > +#define PROPTABLE_MAX_SZ	ALIGN(BIT(ITS_MAX_LPI_NRBITS) / 8,
> > SZ_64K)
> > 
> > This looks completely wrong.
> > 
> > The pending table needs one bit per LPI, and the property table one byte per LPI.
> > Here, you have it the other way around.
> 
> It's a typo, will fix after the revert patch applied.

A typo that has the potential to corrupt to corrupt memory. This
clearly was never tested.

> 
> > Also, the property table alignment requirement is 4kB, not 64kB,
> > and its size is defined as the maximum number of LPIs - 8192.
> 
> As in the accessor gic_lpi_tables_init() there isn't alignment
> operation for both property table and pending table, we have to pass
> a 64KB alignment address, even though the property table only
> requires 4KB alignment.

So how about fixing it?

> 
> > 
> > Finally, ITS_MAX_LPI_NRBITS is hardcoded to 16, while it can
> > actually vary from 14 to 32 (and even further limited by some
> > hypervisors), depending on the implementation. Granted, this was
> > broken before this patch, and in most cases, 64k is more than
> > enough.
> >
> 
> This is only for Layerscape platforms, so hardcoded to 16 bit works.

Let me say it again: hardcoding things for a specific platform for no
good reason is utterly wasteful. There is no reason to write SoC
specific code here, and allocating tables should be done in an
architectural way.

> 
> > However, given that this defining the number of LPIs for the
> > lifetime of the system, it would be better to actually allocate
> > what the HW advertises (GICD_TYPER.IDbits, capped by
> > GICD_TYPER.num_LPIs).
> > 
> > > +#define GIC_LPI_SIZE		ALIGN(cpu_numcores() * PENDTABLE_MAX_SZ + \
> > > +				PROPTABLE_MAX_SZ, SZ_1M)
> > 
> > Why the 1MB alignment? There is no such requirement in the
> > architecture (64kB for the pending tables, 4kB for the property
> > table).
> 
> This is definition of the size instead of address, 1MB size
> alignment is to ensure we have enough space to do address alignment,
> perhaps 64KB should be enough.

Again: straying out of the architecture requirement is wasteful. The
architecture states all the requirements. How about you follow them
instead of picking random numbers?

> 
> > 
> > > +static int fdt_add_resv_mem_gic_rd_tables(void *blob, u64 base,
> > > +size_t size) {
> > > +	int err;
> > > +	struct fdt_memory gic_rd_tables;
> > > +
> > > +	gic_rd_tables.start = base;
> > > +	gic_rd_tables.end = base + size - 1;
> > > +	err = fdtdec_add_reserved_memory(blob, "gic-rd-tables", &gic_rd_tables,
> > > +					 NULL, 0, NULL, 0);
> > > +	if (err < 0)
> > > +		debug("%s: failed to add reserved memory: %d\n", __func__, err);
> > > +
> > > +	return err;
> > > +}
> > > +
> > >  int ls_gic_rd_tables_init(void *blob)  {
> > > +	u64 gic_lpi_base;
> > >  	int ret;
> > >
> > > -	ret = gic_lpi_tables_init();
> > > +	gic_lpi_base = ALIGN(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;
> > > +
> > > +	ret = gic_lpi_tables_init(gic_lpi_base, cpu_numcores());
> > 
> > This really should fetch the number of CPUs from the DT rather
> > then some SoC specific black magic...
> 
> Currently in most Layerscape platforms' DTS file there isn't cpu
> nodes. On Layerscape platforms the implemented core number can be
> get from GUT register.

The CPU nodes will eventually be generated, if only for the sake of
being able to boot an operating system. Given that the table
allocation serves no purpose for u-boot itself, such allocation can be
delayed until the nodes are populated, and the number of CPUs known.

No need for anything SoC specific whatsoever. Really, you should lose
the SoC-specific mindset here, because you are:

- reinventing the wheel
- introducing stupid bugs
- making life difficult for everyone else

All of which are the hallmarks of bad engineering.

	M.

-- 
Without deviation from the norm, progress is not possible.