Re: [PATCH 3/3] irqchip/gic-v3-its: Print the vPE table installed in redistributor

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
Date: Sat, 24 Feb 2024 10:41:36 +0000	[thread overview]
Message-ID: <87bk869man.wl-maz@kernel.org> (raw)
In-Reply-To: <20240219185809.286724-4-oliver.upton@linux.dev>

On Mon, 19 Feb 2024 18:58:08 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> 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.
> 
> Promote to pr_info() and add some additional context, such as the
> provenance of the installed vPE table.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-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 = SZ_64K;
>  	unsigned int np, epp, esz;
> -	struct page *page;
> +	struct page *page = NULL;
> +	bool from_its = false;
>  
>  	if (!gic_rdists->has_rvpeid)
>  		return 0;
> @@ -2865,8 +2866,10 @@ static int allocate_vpe_l1_table(void)
>  		return -ENOMEM;
>  
>  	val = inherit_vpe_l1_table_from_its();
> -	if (val & GICR_VPROPBASER_4_1_VALID)
> +	if (val & GICR_VPROPBASER_4_1_VALID) {
> +		from_its = true;
>  		goto out;
> +	}

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

>  
>  	/* First probe the page size */
>  	val = 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);
>  
> -	pr_debug("CPU%d: VPROPBASER = %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"));

From 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.

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