irqchip/gic-v3-its: Fix GICv4.1 initialization after kexec

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 <[email protected]>

---
 drivers/irqchip/irq-gic-v3-its.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

drivers/irqchip/irq-gic-v3-its.c +0 -0

index 0022852ce494..63d1743f08cc 100644

drivers/irqchip/irq-gic-v3-its.c +16 -0

+	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

drivers/irqchip/irq-gic-v3-its.c +0 -6

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

drivers/irqchip/irq-gic-v3-its.c static void its_cpu_init_lpis(void) · line 3214

Quoted 7 lines · Oliver Upton

drivers/irqchip/irq-gic-v3-its.c +2 -1

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

drivers/irqchip/irq-gic-v3-its.c static void its_cpu_init_lpis(void) · line 3210

Quoted 10 lines

drivers/irqchip/irq-gic-v3-its.c +0 -3

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

Quoted 12 lines · Marc Zyngier

drivers/irqchip/irq-gic-v3-its.c static void its_cpu_init_lpis(void) · line 3214

Quoted 7 lines · Oliver Upton

drivers/irqchip/irq-gic-v3-its.c +2 -1

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

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 <[email protected]>

---
 drivers/irqchip/irq-gic-v3-its.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

drivers/irqchip/irq-gic-v3-its.c +2 -1

index 63d1743f08cc..c3ef9665a2ad 100644
@@ -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;

drivers/irqchip/irq-gic-v3-its.c +0 -0

 	/* 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);

drivers/irqchip/irq-gic-v3-its.c static int allocate_vpe_l1_table(void) · line 2956

Quoted 9 lines · Oliver Upton

drivers/irqchip/irq-gic-v3-its.c +6 -3

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

Quoted 13 lines · Marc Zyngier

drivers/irqchip/irq-gic-v3-its.c static int allocate_vpe_l1_table(void) · line 2956

Quoted 9 lines · Oliver Upton

drivers/irqchip/irq-gic-v3-its.c +6 -3

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

Quoted 10 lines

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?