[PATCH 0/3] irqchip/gic-v3-its: Fix LPI pending table handling vs PREEMPT_RT

[PATCH 0/3] irqchip/gic-v3-its: Fix LPI pending table handling vs PREEMPT_RT

[Valentin Schneider]

Hi folks,

This is my take at fixing [1]. Reading about the LPI tables situation was
entertaining.

Tested against kexec on an Ampere eMAG. Seems to be working fine atop
5.15-rc6. On the other hand, I can only issue one kexec from 5.15-rc6-rt12 - if
I then issue another one on the new kernel, I get tasks hanging. That is true
even without my patches and without CONFIG_PREEMPT_RT.

[1]: http://lore.kernel.org/r/20210810134127.1394269-3-valentin.schneider@arm.com

Cheers,
Valentin

Valentin Schneider (3):
  irqchip/gic-v3-its: Give the percpu rdist struct its own flags field
  irqchip/gic-v3-its: Postpone LPI pending table freeing and memreserve
  irqchip/gic-v3-its: Limit memreserve cpuhp state lifetime

 drivers/irqchip/irq-gic-v3-its.c   | 108 ++++++++++++++++++++++++-----
 include/linux/irqchip/arm-gic-v3.h |   3 +-
 2 files changed, 94 insertions(+), 17 deletions(-)

--
2.25.1

[PATCH 1/3] irqchip/gic-v3-its: Give the percpu rdist struct its own flags field

[Valentin Schneider]

Later patches will require tracking some per-rdist status. Reuse the bytes
"lost" to padding within the __percpu rdist struct as a flags field, and
re-encode ->lpi_enabled within said flags.

No change in functionality intended.

Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
---
 drivers/irqchip/irq-gic-v3-its.c   | 26 ++++++++++++++------------
 include/linux/irqchip/arm-gic-v3.h |  2 +-
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index eb0882d15366..a688ed5c21e8 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -43,8 +43,10 @@
 #define ITS_FLAGS_WORKAROUND_CAVIUM_22375	(1ULL << 1)
 #define ITS_FLAGS_WORKAROUND_CAVIUM_23144	(1ULL << 2)
 
-#define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING	(1 << 0)
-#define RDIST_FLAGS_RD_TABLES_PREALLOCATED	(1 << 1)
+#define RDISTS_FLAGS_PROPBASE_NEEDS_FLUSHING	(1 << 0)
+#define RDISTS_FLAGS_RD_TABLES_PREALLOCATED	(1 << 1)
+
+#define RDIST_FLAGS_LPI_ENABLED                 BIT(0)
 
 static u32 lpi_id_bits;
 
@@ -1415,7 +1417,7 @@ static void lpi_write_config(struct irq_data *d, u8 clr, u8 set)
 	 * And yes, we're flushing exactly: One. Single. Byte.
 	 * Humpf...
 	 */
-	if (gic_rdists->flags & RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING)
+	if (gic_rdists->flags & RDISTS_FLAGS_PROPBASE_NEEDS_FLUSHING)
 		gic_flush_dcache_to_poc(cfg, sizeof(*cfg));
 	else
 		dsb(ishst);
@@ -2224,7 +2226,7 @@ static int gic_reserve_range(phys_addr_t addr, unsigned long size)
 
 static int __init its_setup_lpi_prop_table(void)
 {
-	if (gic_rdists->flags & RDIST_FLAGS_RD_TABLES_PREALLOCATED) {
+	if (gic_rdists->flags & RDISTS_FLAGS_RD_TABLES_PREALLOCATED) {
 		u64 val;
 
 		val = gicr_read_propbaser(gic_data_rdist_rd_base() + GICR_PROPBASER);
@@ -2978,8 +2980,8 @@ static int __init allocate_lpi_tables(void)
 	 */
 	val = readl_relaxed(gic_data_rdist_rd_base() + GICR_CTLR);
 	if ((val & GICR_CTLR_ENABLE_LPIS) && enabled_lpis_allowed()) {
-		gic_rdists->flags |= (RDIST_FLAGS_RD_TABLES_PREALLOCATED |
-				      RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING);
+		gic_rdists->flags |= (RDISTS_FLAGS_RD_TABLES_PREALLOCATED |
+				      RDISTS_FLAGS_PROPBASE_NEEDS_FLUSHING);
 		pr_info("GICv3: Using preallocated redistributor tables\n");
 	}
 
@@ -3044,11 +3046,11 @@ static void its_cpu_init_lpis(void)
 	phys_addr_t paddr;
 	u64 val, tmp;
 
-	if (gic_data_rdist()->lpi_enabled)
+	if (gic_data_rdist()->flags & RDIST_FLAGS_LPI_ENABLED)
 		return;
 
 	val = readl_relaxed(rbase + GICR_CTLR);
-	if ((gic_rdists->flags & RDIST_FLAGS_RD_TABLES_PREALLOCATED) &&
+	if ((gic_rdists->flags & RDISTS_FLAGS_RD_TABLES_PREALLOCATED) &&
 	    (val & GICR_CTLR_ENABLE_LPIS)) {
 		/*
 		 * Check that we get the same property table on all
@@ -3095,7 +3097,7 @@ static void its_cpu_init_lpis(void)
 			gicr_write_propbaser(val, rbase + GICR_PROPBASER);
 		}
 		pr_info_once("GIC: using cache flushing for LPI property table\n");
-		gic_rdists->flags |= RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING;
+		gic_rdists->flags |= RDISTS_FLAGS_PROPBASE_NEEDS_FLUSHING;
 	}
 
 	/* set PENDBASE */
@@ -3158,7 +3160,7 @@ static void its_cpu_init_lpis(void)
 	/* Make sure the GIC has seen the above */
 	dsb(sy);
 out:
-	gic_data_rdist()->lpi_enabled = true;
+	gic_data_rdist()->flags |= RDIST_FLAGS_LPI_ENABLED;
 	pr_info("GICv3: CPU%d: using %s LPI pending table @%pa\n",
 		smp_processor_id(),
 		gic_data_rdist()->pend_page ? "allocated" : "reserved",
@@ -5138,8 +5140,8 @@ static int redist_disable_lpis(void)
 	 *
 	 * If running with preallocated tables, there is nothing to do.
 	 */
-	if (gic_data_rdist()->lpi_enabled ||
-	    (gic_rdists->flags & RDIST_FLAGS_RD_TABLES_PREALLOCATED))
+	if ((gic_data_rdist()->flags & RDIST_FLAGS_LPI_ENABLED) ||
+	    (gic_rdists->flags & RDISTS_FLAGS_RD_TABLES_PREALLOCATED))
 		return 0;
 
 	/*
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index 81cbf85f73de..0dc34d7d735a 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -615,7 +615,7 @@ struct rdists {
 		void __iomem	*rd_base;
 		struct page	*pend_page;
 		phys_addr_t	phys_base;
-		bool		lpi_enabled;
+		u64             flags;
 		cpumask_t	*vpe_table_mask;
 		void		*vpe_l1_base;
 	} __percpu		*rdist;
-- 
2.25.1

[PATCH 2/3] irqchip/gic-v3-its: Postpone LPI pending table freeing and memreserve

[Valentin Schneider]

Memory used by the LPI tables have to be made persistent for kexec to have
a chance to work, as explained in [1]. If they have been made persistent
and we are booting into a kexec'd kernel, we also need to free the pages
that were preemptively allocated by the new kernel for those tables.

Both of those operations currently happen during its_cpu_init(), which
happens in a _STARTING (IOW atomic) cpuhp callback for secondary
CPUs. efi_mem_reserve_iomem() issues a GFP_ATOMIC allocation, which
unfortunately doesn't work under PREEMPT_RT (this ends up grabbing a
non-raw spinlock, which can sleep under PREEMPT_RT). Similarly, freeing the
pages ends up grabbing a sleepable spinlock.

Since the memreserve is only required by kexec, it doesn't have to be
done so early in the secondary boot process. Issue the reservation in a new
CPUHP_AP_ONLINE_DYN cpuhp callback, and piggy-back the page freeing on top
of it. As kexec issues a machine_shutdown() prior to machine_kexec(), it
will be serialized vs a CPU being plugged to life by the hotplug machinery -
either the CPU will have been brought up and have had its redistributor's
pending table memreserved, or it never went online and will have its table
allocated by the new kernel.

[1]: https://lore.kernel.org/lkml/20180921195954.21574-1-marc.zyngier@arm.com/
Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
---
 drivers/irqchip/irq-gic-v3-its.c | 65 ++++++++++++++++++++++++++++++--
 1 file changed, 61 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index a688ed5c21e8..a6a4af59205e 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -47,6 +47,8 @@
 #define RDISTS_FLAGS_RD_TABLES_PREALLOCATED	(1 << 1)
 
 #define RDIST_FLAGS_LPI_ENABLED                 BIT(0)
+#define RDIST_FLAGS_PENDTABLE_RESERVED          BIT(1)
+#define RDIST_FLAGS_PENDTABLE_PREALLOCATED      BIT(2)
 
 static u32 lpi_id_bits;
 
@@ -3065,15 +3067,15 @@ static void its_cpu_init_lpis(void)
 		paddr &= GENMASK_ULL(51, 16);
 
 		WARN_ON(!gic_check_reserved_range(paddr, LPI_PENDBASE_SZ));
-		its_free_pending_table(gic_data_rdist()->pend_page);
-		gic_data_rdist()->pend_page = NULL;
+		gic_data_rdist()->flags |=
+			RDIST_FLAGS_PENDTABLE_RESERVED |
+			RDIST_FLAGS_PENDTABLE_PREALLOCATED;
 
 		goto out;
 	}
 
 	pend_page = gic_data_rdist()->pend_page;
 	paddr = page_to_phys(pend_page);
-	WARN_ON(gic_reserve_range(paddr, LPI_PENDBASE_SZ));
 
 	/* set PROPBASE */
 	val = (gic_rdists->prop_table_pa |
@@ -3163,7 +3165,8 @@ static void its_cpu_init_lpis(void)
 	gic_data_rdist()->flags |= RDIST_FLAGS_LPI_ENABLED;
 	pr_info("GICv3: CPU%d: using %s LPI pending table @%pa\n",
 		smp_processor_id(),
-		gic_data_rdist()->pend_page ? "allocated" : "reserved",
+		gic_data_rdist()->flags & RDIST_FLAGS_PENDTABLE_PREALLOCATED ?
+		"reserved" : "allocated",
 		&paddr);
 }
 
@@ -5202,6 +5205,39 @@ int its_cpu_init(void)
 	return 0;
 }
 
+#ifdef CONFIG_EFI
+static int its_cpu_memreserve_lpi(unsigned int cpu)
+{
+	struct page *pend_page = gic_data_rdist()->pend_page;
+	phys_addr_t paddr;
+
+	/*
+	 * If the pending table was pre-programmed, free the memory we
+	 * preemptively allocated.
+	 */
+	if (pend_page &&
+	    (gic_data_rdist()->flags & RDIST_FLAGS_PENDTABLE_PREALLOCATED)) {
+		its_free_pending_table(gic_data_rdist()->pend_page);
+		gic_data_rdist()->pend_page = NULL;
+	}
+
+	/*
+	 * Did we already issue a memreserve? This could be via a previous
+	 * invocation of this callback, or in a previous life before kexec.
+	 */
+	if (gic_data_rdist()->flags & RDIST_FLAGS_PENDTABLE_RESERVED)
+		return 0;
+
+	gic_data_rdist()->flags |= RDIST_FLAGS_PENDTABLE_RESERVED;
+
+	pend_page = gic_data_rdist()->pend_page;
+	paddr = page_to_phys(pend_page);
+	WARN_ON(gic_reserve_range(paddr, LPI_PENDBASE_SZ));
+
+	return 0;
+}
+#endif
+
 static const struct of_device_id its_device_id[] = {
 	{	.compatible	= "arm,gic-v3-its",	},
 	{},
@@ -5385,6 +5421,23 @@ static void __init its_acpi_probe(void)
 static void __init its_acpi_probe(void) { }
 #endif
 
+static int __init its_lpi_memreserve_init(void)
+{
+	int state;
+
+	if (!efi_enabled(EFI_CONFIG_TABLES))
+		return 0;
+
+	state = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
+				"irqchip/arm/gicv3/memreserve:online",
+				its_cpu_memreserve_lpi,
+				NULL);
+	if (state < 0)
+		return state;
+
+	return 0;
+}
+
 int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
 		    struct irq_domain *parent_domain)
 {
@@ -5412,6 +5465,10 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
 	if (err)
 		return err;
 
+	err = its_lpi_memreserve_init();
+	if (err)
+		return err;
+
 	list_for_each_entry(its, &its_nodes, entry) {
 		has_v4 |= is_v4(its);
 		has_v4_1 |= is_v4_1(its);
-- 
2.25.1

[PATCH 3/3] irqchip/gic-v3-its: Limit memreserve cpuhp state lifetime

[Valentin Schneider]

The new memreserve cpuhp callback only needs to survive up until a point
where every CPU in the system has booted once. Beyond that, it becomes a
no-op and can be put in the bin.

Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
---
 drivers/irqchip/irq-gic-v3-its.c   | 23 ++++++++++++++++++++---
 include/linux/irqchip/arm-gic-v3.h |  1 +
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index a6a4af59205e..4ae9ae6b90fe 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -5206,6 +5206,15 @@ int its_cpu_init(void)
 }
 
 #ifdef CONFIG_EFI
+static void rdist_memreserve_cpuhp_cleanup_workfn(struct work_struct *work)
+{
+	cpuhp_remove_state_nocalls(gic_rdists->cpuhp_memreserve_state);
+	gic_rdists->cpuhp_memreserve_state = CPUHP_INVALID;
+}
+
+static DECLARE_WORK(rdist_memreserve_cpuhp_cleanup_work,
+		    rdist_memreserve_cpuhp_cleanup_workfn);
+
 static int its_cpu_memreserve_lpi(unsigned int cpu)
 {
 	struct page *pend_page = gic_data_rdist()->pend_page;
@@ -5226,7 +5235,7 @@ static int its_cpu_memreserve_lpi(unsigned int cpu)
 	 * invocation of this callback, or in a previous life before kexec.
 	 */
 	if (gic_data_rdist()->flags & RDIST_FLAGS_PENDTABLE_RESERVED)
-		return 0;
+		goto out;
 
 	gic_data_rdist()->flags |= RDIST_FLAGS_PENDTABLE_RESERVED;
 
@@ -5234,6 +5243,11 @@ static int its_cpu_memreserve_lpi(unsigned int cpu)
 	paddr = page_to_phys(pend_page);
 	WARN_ON(gic_reserve_range(paddr, LPI_PENDBASE_SZ));
 
+out:
+	/* This only needs to run once per CPU */
+	if (cpumask_equal(&cpus_booted_once_mask, cpu_possible_mask))
+		schedule_work(&rdist_memreserve_cpuhp_cleanup_work);
+
 	return 0;
 }
 #endif
@@ -5421,13 +5435,14 @@ static void __init its_acpi_probe(void)
 static void __init its_acpi_probe(void) { }
 #endif
 
-static int __init its_lpi_memreserve_init(void)
+static int __init its_lpi_memreserve_init(struct rdists *rdists)
 {
 	int state;
 
 	if (!efi_enabled(EFI_CONFIG_TABLES))
 		return 0;
 
+	rdists->cpuhp_memreserve_state = CPUHP_INVALID;
 	state = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
 				"irqchip/arm/gicv3/memreserve:online",
 				its_cpu_memreserve_lpi,
@@ -5435,6 +5450,8 @@ static int __init its_lpi_memreserve_init(void)
 	if (state < 0)
 		return state;
 
+	rdists->cpuhp_memreserve_state = state;
+
 	return 0;
 }
 
@@ -5465,7 +5482,7 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
 	if (err)
 		return err;
 
-	err = its_lpi_memreserve_init();
+	err = its_lpi_memreserve_init(rdists);
 	if (err)
 		return err;
 
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index 0dc34d7d735a..95479b315918 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -624,6 +624,7 @@ struct rdists {
 	u64			flags;
 	u32			gicd_typer;
 	u32			gicd_typer2;
+	int                     cpuhp_memreserve_state;
 	bool			has_vlpis;
 	bool			has_rvpeid;
 	bool			has_direct_lpi;
-- 
2.25.1

Re: [PATCH 1/3] irqchip/gic-v3-its: Give the percpu rdist struct its own flags field

[Marc Zyngier]

On Fri, 22 Oct 2021 11:33:05 +0100,
Valentin Schneider <valentin.schneider@arm.com> wrote:
> 
> Later patches will require tracking some per-rdist status. Reuse the bytes
> "lost" to padding within the __percpu rdist struct as a flags field, and
> re-encode ->lpi_enabled within said flags.
> 
> No change in functionality intended.
> 
> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
> ---
>  drivers/irqchip/irq-gic-v3-its.c   | 26 ++++++++++++++------------
>  include/linux/irqchip/arm-gic-v3.h |  2 +-
>  2 files changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index eb0882d15366..a688ed5c21e8 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -43,8 +43,10 @@
>  #define ITS_FLAGS_WORKAROUND_CAVIUM_22375	(1ULL << 1)
>  #define ITS_FLAGS_WORKAROUND_CAVIUM_23144	(1ULL << 2)
>  
> -#define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING	(1 << 0)
> -#define RDIST_FLAGS_RD_TABLES_PREALLOCATED	(1 << 1)
> +#define RDISTS_FLAGS_PROPBASE_NEEDS_FLUSHING	(1 << 0)
> +#define RDISTS_FLAGS_RD_TABLES_PREALLOCATED	(1 << 1)
> +
> +#define RDIST_FLAGS_LPI_ENABLED                 BIT(0)

Just to reduce the churn and for me not to misread things (because
RDIST/RDISTS is pretty confusing), how about leaving the original
flags as is, and name the per-RD ones like:

#define	RD_LOCAL_LPI_ENABLED	BIT(0)

?

Or something else that'd be adequately different from the original
flags?

Otherwise looks sensible.

	M.

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

Re: [PATCH 2/3] irqchip/gic-v3-its: Postpone LPI pending table freeing and memreserve

[Marc Zyngier]

On Fri, 22 Oct 2021 11:33:06 +0100,
Valentin Schneider <valentin.schneider@arm.com> wrote:
> 
> Memory used by the LPI tables have to be made persistent for kexec to have
> a chance to work, as explained in [1]. If they have been made persistent
> and we are booting into a kexec'd kernel, we also need to free the pages
> that were preemptively allocated by the new kernel for those tables.
> 
> Both of those operations currently happen during its_cpu_init(), which
> happens in a _STARTING (IOW atomic) cpuhp callback for secondary
> CPUs. efi_mem_reserve_iomem() issues a GFP_ATOMIC allocation, which
> unfortunately doesn't work under PREEMPT_RT (this ends up grabbing a
> non-raw spinlock, which can sleep under PREEMPT_RT). Similarly, freeing the
> pages ends up grabbing a sleepable spinlock.
> 
> Since the memreserve is only required by kexec, it doesn't have to be
> done so early in the secondary boot process. Issue the reservation in a new
> CPUHP_AP_ONLINE_DYN cpuhp callback, and piggy-back the page freeing on top
> of it. As kexec issues a machine_shutdown() prior to machine_kexec(), it
> will be serialized vs a CPU being plugged to life by the hotplug machinery -
> either the CPU will have been brought up and have had its redistributor's
> pending table memreserved, or it never went online and will have its table
> allocated by the new kernel.
> 
> [1]: https://lore.kernel.org/lkml/20180921195954.21574-1-marc.zyngier@arm.com/
> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 65 ++++++++++++++++++++++++++++++--
>  1 file changed, 61 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index a688ed5c21e8..a6a4af59205e 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -47,6 +47,8 @@
>  #define RDISTS_FLAGS_RD_TABLES_PREALLOCATED	(1 << 1)
>  
>  #define RDIST_FLAGS_LPI_ENABLED                 BIT(0)
> +#define RDIST_FLAGS_PENDTABLE_RESERVED          BIT(1)
> +#define RDIST_FLAGS_PENDTABLE_PREALLOCATED      BIT(2)
>  
>  static u32 lpi_id_bits;
>  
> @@ -3065,15 +3067,15 @@ static void its_cpu_init_lpis(void)
>  		paddr &= GENMASK_ULL(51, 16);
>  
>  		WARN_ON(!gic_check_reserved_range(paddr, LPI_PENDBASE_SZ));
> -		its_free_pending_table(gic_data_rdist()->pend_page);
> -		gic_data_rdist()->pend_page = NULL;
> +		gic_data_rdist()->flags |=
> +			RDIST_FLAGS_PENDTABLE_RESERVED |
> +			RDIST_FLAGS_PENDTABLE_PREALLOCATED;
>  
>  		goto out;
>  	}
>  
>  	pend_page = gic_data_rdist()->pend_page;
>  	paddr = page_to_phys(pend_page);
> -	WARN_ON(gic_reserve_range(paddr, LPI_PENDBASE_SZ));
>  
>  	/* set PROPBASE */
>  	val = (gic_rdists->prop_table_pa |
> @@ -3163,7 +3165,8 @@ static void its_cpu_init_lpis(void)
>  	gic_data_rdist()->flags |= RDIST_FLAGS_LPI_ENABLED;
>  	pr_info("GICv3: CPU%d: using %s LPI pending table @%pa\n",
>  		smp_processor_id(),
> -		gic_data_rdist()->pend_page ? "allocated" : "reserved",
> +		gic_data_rdist()->flags & RDIST_FLAGS_PENDTABLE_PREALLOCATED ?
> +		"reserved" : "allocated",
>  		&paddr);
>  }
>  
> @@ -5202,6 +5205,39 @@ int its_cpu_init(void)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_EFI

Why do we need this? I can't see anything that'd be problematic even
if EFI was disabled.

> +static int its_cpu_memreserve_lpi(unsigned int cpu)
> +{
> +	struct page *pend_page = gic_data_rdist()->pend_page;
> +	phys_addr_t paddr;
> +
> +	/*
> +	 * If the pending table was pre-programmed, free the memory we
> +	 * preemptively allocated.
> +	 */
> +	if (pend_page &&
> +	    (gic_data_rdist()->flags & RDIST_FLAGS_PENDTABLE_PREALLOCATED)) {
> +		its_free_pending_table(gic_data_rdist()->pend_page);
> +		gic_data_rdist()->pend_page = NULL;
> +	}

So you set it to NULL and carry on, ending up reserving a 64kB block
at address 0 if the RESERVED flag isn't set. Can this happen at all?
If, as I suspect, it cannot happen because the two flags are always
set at the same time, why do we need two flags?

My gut feeling is that if pend_page is non-NULL and that the RESERVED
flag is set, you should free the memory and leave the building.
Otherwise, reserve the memory and set the flag. PREALLOCATED doesn't
seem to make much sense on a per-CPU basis here.

> +
> +	/*
> +	 * Did we already issue a memreserve? This could be via a previous
> +	 * invocation of this callback, or in a previous life before kexec.
> +	 */
> +	if (gic_data_rdist()->flags & RDIST_FLAGS_PENDTABLE_RESERVED)
> +		return 0;
> +
> +	gic_data_rdist()->flags |= RDIST_FLAGS_PENDTABLE_RESERVED;
> +
> +	pend_page = gic_data_rdist()->pend_page;
> +	paddr = page_to_phys(pend_page);
> +	WARN_ON(gic_reserve_range(paddr, LPI_PENDBASE_SZ));

Shouldn't the flag setting be moved here and be conditional on the
reservation success? Yes, you'll get a warning each time the CPU comes
back, but at least that'd track the state correctly.

> +
> +	return 0;
> +}
> +#endif
> +
>  static const struct of_device_id its_device_id[] = {
>  	{	.compatible	= "arm,gic-v3-its",	},
>  	{},
> @@ -5385,6 +5421,23 @@ static void __init its_acpi_probe(void)
>  static void __init its_acpi_probe(void) { }
>  #endif
>  
> +static int __init its_lpi_memreserve_init(void)
> +{
> +	int state;
> +
> +	if (!efi_enabled(EFI_CONFIG_TABLES))
> +		return 0;
> +
> +	state = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> +				"irqchip/arm/gicv3/memreserve:online",
> +				its_cpu_memreserve_lpi,
> +				NULL);
> +	if (state < 0)
> +		return state;
> +
> +	return 0;
> +}
> +
>  int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
>  		    struct irq_domain *parent_domain)
>  {
> @@ -5412,6 +5465,10 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
>  	if (err)
>  		return err;
>  
> +	err = its_lpi_memreserve_init();
> +	if (err)
> +		return err;
> +
>  	list_for_each_entry(its, &its_nodes, entry) {
>  		has_v4 |= is_v4(its);
>  		has_v4_1 |= is_v4_1(its);

Thanks,

	M.

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

Re: [PATCH 3/3] irqchip/gic-v3-its: Limit memreserve cpuhp state lifetime

[Marc Zyngier]

On Fri, 22 Oct 2021 11:33:07 +0100,
Valentin Schneider <valentin.schneider@arm.com> wrote:
> 
> The new memreserve cpuhp callback only needs to survive up until a point
> where every CPU in the system has booted once. Beyond that, it becomes a
> no-op and can be put in the bin.
> 
> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
> ---
>  drivers/irqchip/irq-gic-v3-its.c   | 23 ++++++++++++++++++++---
>  include/linux/irqchip/arm-gic-v3.h |  1 +
>  2 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index a6a4af59205e..4ae9ae6b90fe 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -5206,6 +5206,15 @@ int its_cpu_init(void)
>  }
>  
>  #ifdef CONFIG_EFI
> +static void rdist_memreserve_cpuhp_cleanup_workfn(struct work_struct *work)
> +{
> +	cpuhp_remove_state_nocalls(gic_rdists->cpuhp_memreserve_state);
> +	gic_rdists->cpuhp_memreserve_state = CPUHP_INVALID;
> +}
> +
> +static DECLARE_WORK(rdist_memreserve_cpuhp_cleanup_work,
> +		    rdist_memreserve_cpuhp_cleanup_workfn);
> +
>  static int its_cpu_memreserve_lpi(unsigned int cpu)
>  {
>  	struct page *pend_page = gic_data_rdist()->pend_page;
> @@ -5226,7 +5235,7 @@ static int its_cpu_memreserve_lpi(unsigned int cpu)
>  	 * invocation of this callback, or in a previous life before kexec.
>  	 */
>  	if (gic_data_rdist()->flags & RDIST_FLAGS_PENDTABLE_RESERVED)
> -		return 0;
> +		goto out;
>  
>  	gic_data_rdist()->flags |= RDIST_FLAGS_PENDTABLE_RESERVED;
>  
> @@ -5234,6 +5243,11 @@ static int its_cpu_memreserve_lpi(unsigned int cpu)
>  	paddr = page_to_phys(pend_page);
>  	WARN_ON(gic_reserve_range(paddr, LPI_PENDBASE_SZ));
>  
> +out:
> +	/* This only needs to run once per CPU */
> +	if (cpumask_equal(&cpus_booted_once_mask, cpu_possible_mask))
> +		schedule_work(&rdist_memreserve_cpuhp_cleanup_work);

Which makes me wonder. Do we actually need any flag at all if all we
need to check is whether the CPU has been through the callback at
least once? I have the strong feeling that we are tracking the same
state multiple times here.

Also, could the cpuhp callbacks ever run concurrently? If they could,
two CPUs could schedule the cleanup work in parallel, with interesting
results.  You'd need a cmpxchg on the cpuhp state in the workfn.

> +
>  	return 0;
>  }
>  #endif
> @@ -5421,13 +5435,14 @@ static void __init its_acpi_probe(void)
>  static void __init its_acpi_probe(void) { }
>  #endif
>  
> -static int __init its_lpi_memreserve_init(void)
> +static int __init its_lpi_memreserve_init(struct rdists *rdists)
>  {
>  	int state;
>  
>  	if (!efi_enabled(EFI_CONFIG_TABLES))
>  		return 0;
>  
> +	rdists->cpuhp_memreserve_state = CPUHP_INVALID;
>  	state = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
>  				"irqchip/arm/gicv3/memreserve:online",
>  				its_cpu_memreserve_lpi,
> @@ -5435,6 +5450,8 @@ static int __init its_lpi_memreserve_init(void)
>  	if (state < 0)
>  		return state;
>  
> +	rdists->cpuhp_memreserve_state = state;
> +
>  	return 0;
>  }
>  
> @@ -5465,7 +5482,7 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
>  	if (err)
>  		return err;
>  
> -	err = its_lpi_memreserve_init();
> +	err = its_lpi_memreserve_init(rdists);
>  	if (err)
>  		return err;
>  
> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> index 0dc34d7d735a..95479b315918 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -624,6 +624,7 @@ struct rdists {
>  	u64			flags;
>  	u32			gicd_typer;
>  	u32			gicd_typer2;
> +	int                     cpuhp_memreserve_state;
>  	bool			has_vlpis;
>  	bool			has_rvpeid;
>  	bool			has_direct_lpi;

Thanks,

	M.

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

Re: [PATCH 1/3] irqchip/gic-v3-its: Give the percpu rdist struct its own flags field

[Valentin Schneider]

On 23/10/21 10:10, Marc Zyngier wrote:
> On Fri, 22 Oct 2021 11:33:05 +0100,
> Valentin Schneider <valentin.schneider@arm.com> wrote:
>> -#define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING	(1 << 0)
>> -#define RDIST_FLAGS_RD_TABLES_PREALLOCATED	(1 << 1)
>> +#define RDISTS_FLAGS_PROPBASE_NEEDS_FLUSHING	(1 << 0)
>> +#define RDISTS_FLAGS_RD_TABLES_PREALLOCATED	(1 << 1)
>> +
>> +#define RDIST_FLAGS_LPI_ENABLED                 BIT(0)
>
> Just to reduce the churn and for me not to misread things (because
> RDIST/RDISTS is pretty confusing), how about leaving the original
> flags as is, and name the per-RD ones like:
>
> #define	RD_LOCAL_LPI_ENABLED	BIT(0)
>
> ?
>
> Or something else that'd be adequately different from the original
> flags?
>

Aye, sounds like the right thing to do!

Re: [PATCH 2/3] irqchip/gic-v3-its: Postpone LPI pending table freeing and memreserve

[Valentin Schneider]

On 23/10/21 10:48, Marc Zyngier wrote:
> On Fri, 22 Oct 2021 11:33:06 +0100,
> Valentin Schneider <valentin.schneider@arm.com> wrote:
>> @@ -5202,6 +5205,39 @@ int its_cpu_init(void)
>>      return 0;
>>  }
>>
>> +#ifdef CONFIG_EFI
>
> Why do we need this? I can't see anything that'd be problematic even
> if EFI was disabled.
>

You're right, that's not required.

>> +static int its_cpu_memreserve_lpi(unsigned int cpu)
>> +{
>> +	struct page *pend_page = gic_data_rdist()->pend_page;
>> +	phys_addr_t paddr;
>> +
>> +	/*
>> +	 * If the pending table was pre-programmed, free the memory we
>> +	 * preemptively allocated.
>> +	 */
>> +	if (pend_page &&
>> +	    (gic_data_rdist()->flags & RDIST_FLAGS_PENDTABLE_PREALLOCATED)) {
>> +		its_free_pending_table(gic_data_rdist()->pend_page);
>> +		gic_data_rdist()->pend_page = NULL;
>> +	}
>
> So you set it to NULL and carry on, ending up reserving a 64kB block
> at address 0 if the RESERVED flag isn't set. Can this happen at all?
> If, as I suspect, it cannot happen because the two flags are always
> set at the same time, why do we need two flags?
>

PREALLOCATED implies RESERVED, but the reverse isn't true.

> My gut feeling is that if pend_page is non-NULL and that the RESERVED
> flag is set, you should free the memory and leave the building.
> Otherwise, reserve the memory and set the flag. PREALLOCATED doesn't
> seem to make much sense on a per-CPU basis here.
>

One thing I was concerned about is that this cpuhp callback can be invoked
more than once on the same CPU, even with the removal in patch 3.
Consider a system with maxcpus=X on the cmdline; not all secondaries will
be brought up in smp_init(). You then get to userspace which can issue all
sorts of hotplug sequences. Let me try to paint a picture:

  maxcpus=2

  CPU0                      CPU1                      CPU2

  its_init()                                          <nothing ever happens here>
  [...]
  its_cpu_memreserve_lpi()
    flags |= RESERVED
  [...]
  smp_init()
                            its_cpu_memreserve_lpi()
                              flags |= RESERVED

                         [.....]

  cpu_down(CPU1, CPUHP_OFFLINE)
  cpu_up(CPU1, CPUHP_ONLINE)

                            its_cpu_memreserve_lpi()
                              // pend_page != NULL && (flags & RESERVED) != 0
                              // we musn't free the memory

Now, my approach clearly isn't great (I also went through the "wait those
two flags are the same thing" phase, which in hindsight wasn't a good sign).
What we could do instead is only have a PREALLOCATED flag (or RESERVED; in
any case just one rather than two) set in its_cpu_init_lpis(), and ensure
each CPU only ever executes the body of the callback exactly once.

  if (already_booted())
      return 0;

  if (PREALLOCATED)
      its_free_pending_table();
  else
      gic_reserve_range();

  out:
    // callback removal faff here
    return 0;

Unfortunately, the boot CPU will already be present in
cpus_booted_once_mask when this is first invoked for the BP, so AFAICT we'd
need some new tracking utility (either a new RDIST_LOCAL flag or a separate
cpumask).

WDYT?

Re: [PATCH 3/3] irqchip/gic-v3-its: Limit memreserve cpuhp state lifetime

[Valentin Schneider]

On 23/10/21 11:37, Marc Zyngier wrote:
> On Fri, 22 Oct 2021 11:33:07 +0100,
> Valentin Schneider <valentin.schneider@arm.com> wrote:
>> @@ -5234,6 +5243,11 @@ static int its_cpu_memreserve_lpi(unsigned int cpu)
>>      paddr = page_to_phys(pend_page);
>>      WARN_ON(gic_reserve_range(paddr, LPI_PENDBASE_SZ));
>>
>> +out:
>> +	/* This only needs to run once per CPU */
>> +	if (cpumask_equal(&cpus_booted_once_mask, cpu_possible_mask))
>> +		schedule_work(&rdist_memreserve_cpuhp_cleanup_work);
>
> Which makes me wonder. Do we actually need any flag at all if all we
> need to check is whether the CPU has been through the callback at
> least once? I have the strong feeling that we are tracking the same
> state multiple times here.
>

Agreed, cf. my reply on 2/3.

> Also, could the cpuhp callbacks ever run concurrently? If they could,
> two CPUs could schedule the cleanup work in parallel, with interesting
> results.  You'd need a cmpxchg on the cpuhp state in the workfn.
>

So I think the cpuhp callbacks may run concurrently, but at a quick glance
it seems like we can't get two instances of the same work executing
concurrently: schedule_work()->queue_work() doesn't re-queue a work if it's already
pending, and __queue_work() checks a work's previous pool in case it might
still be running there.

Regardless, that's one less thing to worry about if we make the cpuhp
callback body run at most once on each CPU (only a single CPU will be able
to queue the removal work).

Re: [PATCH 2/3] irqchip/gic-v3-its: Postpone LPI pending table freeing and memreserve

[Marc Zyngier]

On Sun, 24 Oct 2021 16:51:53 +0100,
Valentin Schneider <valentin.schneider@arm.com> wrote:
> 
> What we could do instead is only have a PREALLOCATED flag (or RESERVED; in
> any case just one rather than two) set in its_cpu_init_lpis(), and ensure
> each CPU only ever executes the body of the callback exactly once.
> 
>   if (already_booted())
>       return 0;
> 
>   if (PREALLOCATED)
>       its_free_pending_table();
>   else
>       gic_reserve_range();
> 
>   out:
>     // callback removal faff here
>     return 0;
> 
> Unfortunately, the boot CPU will already be present in
> cpus_booted_once_mask when this is first invoked for the BP, so AFAICT we'd
> need some new tracking utility (either a new RDIST_LOCAL flag or a separate
> cpumask).
> 
> WDYT?

It'd certainly look saner. You may even be able to take advantage of
the fact that the boot CPU is always 0.

Thanks,

	M.

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