diff mbox series

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

Message ID 20240219185809.286724-4-oliver.upton@linux.dev
State New
Headers
Series irqchip/gic-v3-its: Fix GICv4.1 initialization after kexec |

Commit Message

Oliver Upton Feb. 19, 2024, 6:58 p.m. UTC 
  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(-)

Comments

Marc Zyngier Feb. 24, 2024, 10:41 a.m. UTC | #1 
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.
Oliver Upton Feb. 24, 2024, 11:11 a.m. UTC | #2 
On Sat, Feb 24, 2024 at 10:41:36AM +0000, Marc Zyngier wrote:
> On Mon, 19 Feb 2024 18:58:08 +0000, Oliver Upton <oliver.upton@linux.dev> wrote:
> > -	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.

My reasoning behind it was that the change expanded the table mask by
way of printing what's going on at every RD. But easy enough to throw
back in!

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

Yeah, what'd be really nice is a way to enable pr_debug() on a per
file / driver / whatever basis, since turning on all of it becomes a bit
of a firehose... But I guess that's what grep is for.

WDYT about leaving it at pr_debug() for now, with the additional context
of what exactly VPROPBASE is getting programmend with?
Marc Zyngier Feb. 24, 2024, 11:31 a.m. UTC | #3 
On Sat, 24 Feb 2024 11:11:41 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Sat, Feb 24, 2024 at 10:41:36AM +0000, Marc Zyngier wrote:
> > On Mon, 19 Feb 2024 18:58:08 +0000, Oliver Upton <oliver.upton@linux.dev> wrote:
> > > -	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.
> 
> My reasoning behind it was that the change expanded the table mask by
> way of printing what's going on at every RD. But easy enough to throw
> back in!

Yeah, true enough. But the mask presents a nice, concise way to
express the affinities that comparing a bunch of lines doesn't exactly
convey.

> 
> > 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.
> 
> Yeah, what'd be really nice is a way to enable pr_debug() on a per
> file / driver / whatever basis, since turning on all of it becomes a bit
> of a firehose... But I guess that's what grep is for.
> 
> WDYT about leaving it at pr_debug() for now, with the additional context
> of what exactly VPROPBASE is getting programmend with?

Yup, I'm all for that. Hopefully we can find a good way to way to
control the debug output in the near future.

Thanks,

	M.
diff mbox series

Patch

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;
+	}
 
 	/* 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"));
 
 	return 0;
 }