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