[PATCH 0/3] irqchip/gic-v3-its: Fix GICv4.1 initialization after kexec

[PATCH 0/3] irqchip/gic-v3-its: Fix GICv4.1 initialization after kexec

[Oliver Upton]

Fix an issue with GICv4.1 redistributor initialization after kexec,
ensuring the vPE table gets reinstalled even if the kernel is using
preallocated LPI tables.

First patch is worthy of a backport back to the introduction of GICv4.1,
second two are intended to avoid these mess-ups in the future.

Tested by kexec'ing into a new kernel on a GICv4.1 system.

Oliver Upton (3):
  irqchip/gic-v3-its: Do not assume vPE tables are preallocated
  irqchip/gic-v3-its: Spin off GICv4 init into a separate function
  irqchip/gic-v3-its: Print the vPE table installed in redistributor

 drivers/irqchip/irq-gic-v3-its.c | 50 +++++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 17 deletions(-)


base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
-- 
2.44.0.rc0.258.g7320e95886-goog

[PATCH 1/3] irqchip/gic-v3-its: Do not assume vPE tables are preallocated

[Oliver Upton]

We go through quite a bit of trouble to make sure we pick up any
preallocated LPI tables on the redistributors, as enabling LPIs is a
one-way switch. There is no such restriction for vLPIs, and for GICv4.1
we expect to allocate a new vPE table at boot.

This works as intended when initializing an ITS, however when setting up
a redistributor in its_cpu_init_lpis() the early return for preallocated
RD tables skips straight past GICv4 setup. This all comes to a head when
trying to kexec into a new kernel, as the new kernel silently fails to
set up GICv4, leading to a complete loss of SGIs and LPIs for KVM VMs
(ouch).

Slap a band-aid on the problem by ensuring its_cpu_init_lpis() always
initializes GICv4 on the way out, even if the other RD tables were
preallocated.

Cc: stable@vger.kernel.org
Fixes: 6479450f72c1 ("irqchip/gic-v4: Fix occasional VLPI drop")
Reported-by: George Cherian <gcherian@marvell.com>
Co-developed-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---

I debated a bit on the fixes tag between the blamed commit and commit
5e5168461c22 ("irqchip/gic-v4.1: VPE table (aka GICR_VPROPBASER)
allocation"), although it would appear GICv4 could be left in an unknown
state after kexec too.

 drivers/irqchip/irq-gic-v3-its.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index d097001c1e3e..0022852ce494 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -3172,6 +3172,7 @@ static void its_cpu_init_lpis(void)
 	val |= GICR_CTLR_ENABLE_LPIS;
 	writel_relaxed(val, rbase + GICR_CTLR);
 
+out:
 	if (gic_rdists->has_vlpis && !gic_rdists->has_rvpeid) {
 		void __iomem *vlpi_base = gic_data_rdist_vlpi_base();
 
@@ -3207,7 +3208,6 @@ static void its_cpu_init_lpis(void)
 
 	/* Make sure the GIC has seen the above */
 	dsb(sy);
-out:
 	gic_data_rdist()->flags |= RD_LOCAL_LPI_ENABLED;
 	pr_info("GICv3: CPU%d: using %s LPI pending table @%pa\n",
 		smp_processor_id(),
-- 
2.44.0.rc0.258.g7320e95886-goog

[PATCH 2/3] irqchip/gic-v3-its: Spin off GICv4 init into a separate function

[Oliver Upton]

Burying the GICv4 redistributor initialization into the routine for LPIs
is a bit confusing, and can lead to sillies where unexpected codepaths
may not fully initialize the RD.

Hoist it out of its_cpu_init_lpis() into a dedicated function.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 drivers/irqchip/irq-gic-v3-its.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 0022852ce494..63d1743f08cc 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -3173,8 +3173,25 @@ static void its_cpu_init_lpis(void)
 	writel_relaxed(val, rbase + GICR_CTLR);
 
 out:
-	if (gic_rdists->has_vlpis && !gic_rdists->has_rvpeid) {
+	/* Make sure the GIC has seen the above */
+	dsb(sy);
+	gic_data_rdist()->flags |= RD_LOCAL_LPI_ENABLED;
+	pr_info("GICv3: CPU%d: using %s LPI pending table @%pa\n",
+		smp_processor_id(),
+		gic_data_rdist()->flags & RD_LOCAL_PENDTABLE_PREALLOCATED ?
+		"reserved" : "allocated",
+		&paddr);
+}
+
+static void its_cpu_init_vlpis(void)
+{
+	/* No vLPIs? No problem. */
+	if (!gic_rdists->has_vlpis)
+		return;
+
+	if (!gic_rdists->has_rvpeid) {
 		void __iomem *vlpi_base = gic_data_rdist_vlpi_base();
+		u64 val;
 
 		/*
 		 * It's possible for CPU to receive VLPIs before it is
@@ -3193,7 +3210,8 @@ static void its_cpu_init_lpis(void)
 		 * ancient programming gets left in and has possibility of
 		 * corrupting memory.
 		 */
-		val = its_clear_vpend_valid(vlpi_base, 0, 0);
+		its_clear_vpend_valid(vlpi_base, 0, 0);
+		return;
 	}
 
 	if (allocate_vpe_l1_table()) {
@@ -3205,15 +3223,6 @@ static void its_cpu_init_lpis(void)
 		gic_rdists->has_rvpeid = false;
 		gic_rdists->has_vlpis = false;
 	}
-
-	/* Make sure the GIC has seen the above */
-	dsb(sy);
-	gic_data_rdist()->flags |= RD_LOCAL_LPI_ENABLED;
-	pr_info("GICv3: CPU%d: using %s LPI pending table @%pa\n",
-		smp_processor_id(),
-		gic_data_rdist()->flags & RD_LOCAL_PENDTABLE_PREALLOCATED ?
-		"reserved" : "allocated",
-		&paddr);
 }
 
 static void its_cpu_init_collection(struct its_node *its)
@@ -5265,6 +5274,7 @@ int its_cpu_init(void)
 			return ret;
 
 		its_cpu_init_lpis();
+		its_cpu_init_vlpis();
 		its_cpu_init_collections();
 	}
 
-- 
2.44.0.rc0.258.g7320e95886-goog

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

[Oliver Upton]

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;
+	}
 
 	/* 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;
 }
-- 
2.44.0.rc0.258.g7320e95886-goog

[tip: irq/urgent] irqchip/gic-v3-its: Do not assume vPE tables are preallocated

[tip-bot2 for Oliver Upton]

The following commit has been merged into the irq/urgent branch of tip:

Commit-ID:     ec4308ecfc887128a468f03fb66b767559c57c23
Gitweb:        https://git.kernel.org/tip/ec4308ecfc887128a468f03fb66b767559c57c23
Author:        Oliver Upton <oliver.upton@linux.dev>
AuthorDate:    Mon, 19 Feb 2024 18:58:06 
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Wed, 21 Feb 2024 21:11:20 +01:00

irqchip/gic-v3-its: Do not assume vPE tables are preallocated

The GIC/ITS code is designed to ensure to pick up any preallocated LPI
tables on the redistributors, as enabling LPIs is a one-way switch. There
is no such restriction for vLPIs, and for GICv4.1 it is expected to
allocate a new vPE table at boot.

This works as intended when initializing an ITS, however when setting up a
redistributor in cpu_init_lpis() the early return for preallocated RD
tables skips straight past the GICv4 setup. This all comes to a head when
trying to kexec() into a new kernel, as the new kernel silently fails to
set up GICv4, leading to a complete loss of SGIs and LPIs for KVM VMs.

Slap a band-aid on the problem by ensuring its_cpu_init_lpis() always
initializes GICv4 on the way out, even if the other RD tables were
preallocated.

Fixes: 6479450f72c1 ("irqchip/gic-v4: Fix occasional VLPI drop")
Reported-by: George Cherian <gcherian@marvell.com>
Co-developed-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/r/20240219185809.286724-2-oliver.upton@linux.dev
---
 drivers/irqchip/irq-gic-v3-its.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 53abd47..b822752 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -3181,6 +3181,7 @@ static void its_cpu_init_lpis(void)
 	val |= GICR_CTLR_ENABLE_LPIS;
 	writel_relaxed(val, rbase + GICR_CTLR);
 
+out:
 	if (gic_rdists->has_vlpis && !gic_rdists->has_rvpeid) {
 		void __iomem *vlpi_base = gic_data_rdist_vlpi_base();
 
@@ -3216,7 +3217,6 @@ static void its_cpu_init_lpis(void)
 
 	/* Make sure the GIC has seen the above */
 	dsb(sy);
-out:
 	gic_data_rdist()->flags |= RD_LOCAL_LPI_ENABLED;
 	pr_info("GICv3: CPU%d: using %s LPI pending table @%pa\n",
 		smp_processor_id(),

Re: [PATCH 2/3] irqchip/gic-v3-its: Spin off GICv4 init into a separate function

[Marc Zyngier]

On Mon, 19 Feb 2024 18:58:07 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Burying the GICv4 redistributor initialization into the routine for LPIs
> is a bit confusing, and can lead to sillies where unexpected codepaths
> may not fully initialize the RD.
> 
> Hoist it out of its_cpu_init_lpis() into a dedicated function.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 32 +++++++++++++++++++++-----------
>  1 file changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 0022852ce494..63d1743f08cc 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -3173,8 +3173,25 @@ static void its_cpu_init_lpis(void)
>  	writel_relaxed(val, rbase + GICR_CTLR);
>  
>  out:
> -	if (gic_rdists->has_vlpis && !gic_rdists->has_rvpeid) {
> +	/* Make sure the GIC has seen the above */
> +	dsb(sy);

So having hoisted the dsb() here...

> +	gic_data_rdist()->flags |= RD_LOCAL_LPI_ENABLED;
> +	pr_info("GICv3: CPU%d: using %s LPI pending table @%pa\n",
> +		smp_processor_id(),
> +		gic_data_rdist()->flags & RD_LOCAL_PENDTABLE_PREALLOCATED ?
> +		"reserved" : "allocated",
> +		&paddr);
> +}
> +
> +static void its_cpu_init_vlpis(void)
> +{
> +	/* No vLPIs? No problem. */
> +	if (!gic_rdists->has_vlpis)
> +		return;
> +
> +	if (!gic_rdists->has_rvpeid) {
>  		void __iomem *vlpi_base = gic_data_rdist_vlpi_base();
> +		u64 val;
>  
>  		/*
>  		 * It's possible for CPU to receive VLPIs before it is
> @@ -3193,7 +3210,8 @@ static void its_cpu_init_lpis(void)
>  		 * ancient programming gets left in and has possibility of
>  		 * corrupting memory.
>  		 */
> -		val = its_clear_vpend_valid(vlpi_base, 0, 0);
> +		its_clear_vpend_valid(vlpi_base, 0, 0);
> +		return;

I'm not sure about the necessity of this return statement.
allocate_vpe_l1_table() checks for rvpeid already, so it should be
fine to carry on.

>  	}
>  
>  	if (allocate_vpe_l1_table()) {
> @@ -3205,15 +3223,6 @@ static void its_cpu_init_lpis(void)
>  		gic_rdists->has_rvpeid = false;
>  		gic_rdists->has_vlpis = false;
>  	}
> -
> -	/* Make sure the GIC has seen the above */
> -	dsb(sy);

... we're now missing a dsb affecting the VPE table programming, as we
expect things to take effect immediately.

> -	gic_data_rdist()->flags |= RD_LOCAL_LPI_ENABLED;
> -	pr_info("GICv3: CPU%d: using %s LPI pending table @%pa\n",
> -		smp_processor_id(),
> -		gic_data_rdist()->flags & RD_LOCAL_PENDTABLE_PREALLOCATED ?
> -		"reserved" : "allocated",
> -		&paddr);
>  }
>  
>  static void its_cpu_init_collection(struct its_node *its)
> @@ -5265,6 +5274,7 @@ int its_cpu_init(void)
>  			return ret;
>  
>  		its_cpu_init_lpis();
> +		its_cpu_init_vlpis();
>  		its_cpu_init_collections();
>  	}
>  

I'm otherwise OK with the idea of splitting things up.

Thanks,

	M.

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

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

[Marc Zyngier]

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.

Re: [PATCH 2/3] irqchip/gic-v3-its: Spin off GICv4 init into a separate function

[Oliver Upton]

On Sat, Feb 24, 2024 at 10:30:04AM +0000, Marc Zyngier wrote:
> On Mon, 19 Feb 2024 18:58:07 +0000, Oliver Upton <oliver.upton@linux.dev> wrote:
> > @@ -3193,7 +3210,8 @@ static void its_cpu_init_lpis(void)
> >  		 * ancient programming gets left in and has possibility of
> >  		 * corrupting memory.
> >  		 */
> > -		val = its_clear_vpend_valid(vlpi_base, 0, 0);
> > +		its_clear_vpend_valid(vlpi_base, 0, 0);
> > +		return;
> 
> I'm not sure about the necessity of this return statement.
> allocate_vpe_l1_table() checks for rvpeid already, so it should be
> fine to carry on.

Yup, definitely not necessary. My aim was to have the control flow make
it a bit more obvious to the reader what's going on.

Having what reads as an allocation helper do a feature check isn't
entirely obvious.

I have no opinion either way though.

> >  	}
> >  
> >  	if (allocate_vpe_l1_table()) {
> > @@ -3205,15 +3223,6 @@ static void its_cpu_init_lpis(void)
> >  		gic_rdists->has_rvpeid = false;
> >  		gic_rdists->has_vlpis = false;
> >  	}
> > -
> > -	/* Make sure the GIC has seen the above */
> > -	dsb(sy);
> 
> ... we're now missing a dsb affecting the VPE table programming, as we
> expect things to take effect immediately.

LOL, and on the back of a bugfix no less. I'll fix this.

-- 
Thanks,
Oliver

Re: [PATCH 2/3] irqchip/gic-v3-its: Spin off GICv4 init into a separate function

[Marc Zyngier]

On Sat, 24 Feb 2024 11:02:40 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Sat, Feb 24, 2024 at 10:30:04AM +0000, Marc Zyngier wrote:
> > On Mon, 19 Feb 2024 18:58:07 +0000, Oliver Upton <oliver.upton@linux.dev> wrote:
> > > @@ -3193,7 +3210,8 @@ static void its_cpu_init_lpis(void)
> > >  		 * ancient programming gets left in and has possibility of
> > >  		 * corrupting memory.
> > >  		 */
> > > -		val = its_clear_vpend_valid(vlpi_base, 0, 0);
> > > +		its_clear_vpend_valid(vlpi_base, 0, 0);
> > > +		return;
> > 
> > I'm not sure about the necessity of this return statement.
> > allocate_vpe_l1_table() checks for rvpeid already, so it should be
> > fine to carry on.
> 
> Yup, definitely not necessary. My aim was to have the control flow make
> it a bit more obvious to the reader what's going on.
> 
> Having what reads as an allocation helper do a feature check isn't
> entirely obvious.
> 
> I have no opinion either way though.

You could move the if (allocate_vpe_l1_table()) as an 'else' branch,
as the two are mutually exclusive.

Thanks,

	M.

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

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

[Oliver Upton]

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?

-- 
Thanks,
Oliver

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

[Marc Zyngier]

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.

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