Received-SPF: pass (google.com: domain of linux-kernel+bounces-79570-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Sat, 24 Feb 2024 10:41:36 +0000
Message-ID: <87bk869man.wl-maz@kernel.org>
From: Marc Zyngier <maz@kernel.org>
To: Oliver Upton <oliver.upton@linux.dev>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Zenghui Yu <yuzenghui@huawei.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	kvmarm@lists.linux.dev,
	Jing Zhang <jingzhangos@google.com>
Subject: Re: [PATCH 3/3] irqchip/gic-v3-its: Print the vPE table installed in redistributor
In-Reply-To: <20240219185809.286724-4-oliver.upton@linux.dev>
References: <20240219185809.286724-1-oliver.upton@linux.dev>
	<20240219185809.286724-4-oliver.upton@linux.dev>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
Precedence: bulk
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: quoted-printable

On Mon, 19 Feb 2024 18:58:08 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
>=20
> Hindsight is 20/20 of course, but the recent vPE table programming bug
> could've been root caused a bit more quickly if we print the table
> getting installed at every redistributor.
>=20
> Promote to pr_info() and add some additional context, such as the
> provenance of the installed vPE table.
>=20
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>=20
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v=
3-its.c
> index 63d1743f08cc..c3ef9665a2ad 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -2835,7 +2835,8 @@ static int allocate_vpe_l1_table(void)
>  	u64 val, gpsz, npg, pa;
>  	unsigned int psz =3D SZ_64K;
>  	unsigned int np, epp, esz;
> -	struct page *page;
> +	struct page *page =3D NULL;
> +	bool from_its =3D false;
> =20
>  	if (!gic_rdists->has_rvpeid)
>  		return 0;
> @@ -2865,8 +2866,10 @@ static int allocate_vpe_l1_table(void)
>  		return -ENOMEM;
> =20
>  	val =3D inherit_vpe_l1_table_from_its();
> -	if (val & GICR_VPROPBASER_4_1_VALID)
> +	if (val & GICR_VPROPBASER_4_1_VALID) {
> +		from_its =3D true;
>  		goto out;
> +	}

nit:
	from_its =3D val & GICR_VPROPBASER_4_1_VALID;
	if (from_its)
		...

> =20
>  	/* First probe the page size */
>  	val =3D FIELD_PREP(GICR_VPROPBASER_4_1_PAGE_SIZE, GIC_PAGE_SIZE_64K);
> @@ -2945,9 +2948,12 @@ static int allocate_vpe_l1_table(void)
>  	gicr_write_vpropbaser(val, vlpi_base + GICR_VPROPBASER);
>  	cpumask_set_cpu(smp_processor_id(), gic_data_rdist()->vpe_table_mask);
> =20
> -	pr_debug("CPU%d: VPROPBASER =3D %llx %*pbl\n",
> -		 smp_processor_id(), val,
> -		 cpumask_pr_args(gic_data_rdist()->vpe_table_mask));
> +	pr_info("CPU%d: Using %s vPE table @%llx (%s)\n",
> +		smp_processor_id(),
> +		(val & GICR_VPROPBASER_4_1_INDIRECT) ? "indirect" : "direct",
> +		val & GICR_VPROPBASER_4_1_ADDR,
> +		(page) ? "allocated" :
> +			 ((from_its) ? "inherited from ITS" : "inherited from RD"));

=46rom past experience, having the vpe_table_mask value displayed did
help tracking VPE table affinity bugs.

This said, my problem with this patch is that we already have tons of
these statement printed once per CPU/RD. This is really huge and
accounts for a significant part of the boot time on large machines
(64+ CPUs).

Before we add more of those, I'd really want to have a way to tone
them down and only print them at runtime *if* required by the user.
Kind of a dymanic debug, but driven from the command-line and present
early enough.

What do you think?

	M.

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