Re: [PATCH v6 13/16] arm64: arch_register_cpu() variant to check if an ACPI handle is now available.

[Jonathan Cameron]

On Wed, 17 Apr 2024 17:33:02 +0100
Salil Mehta <salil.mehta@xxxxxxxxxx> wrote:

> Hi Jonathan,
> 
> >  From: Jonathan Cameron <jonathan.cameron@xxxxxxxxxx>
> >  Sent: Wednesday, April 17, 2024 2:19 PM
> >  
> >  The ARM64 architecture does not support physical CPU HP today.
> >  To avoid any possibility of a bug against such an architecture if defined in
> >  future, check for the physical CPU HP case (not present) and return an error
> >  on any such attempt.
> >  
> >  On ARM64 virtual CPU Hotplug relies on the status value that can be queried
> >  via the AML method _STA for the CPU object.
> >  
> >  There are two conditions in which the CPU can be registered.
> >  1) ACPI disabled.
> >  2) ACPI enabled and the acpi_handle is available.
> >     _STA evaluates to the CPU is both enabled and present.
> >     (Note that in absence of the _STA method they are always in this
> >      state).
> >  
> >  If neither of these conditions is met the CPU is not 'yet' ready to be used
> >  and -EPROBE_DEFER is returned.
> >  
> >  Success occurs in the early attempt to register the CPUs if we are booting
> >  with DT (no concept yet of vCPU HP) if not it succeeds for already enabled
> >  CPUs when the ACPI Processor driver attaches to them.  Finally it may
> >  succeed via the CPU Hotplug code indicating that the CPU is now enabled.
> >  
> >  For ACPI if CONFIG_ACPI_PROCESSOR the only path to get to
> >  arch_register_cpu() with that handle set is via
> >  acpi_processor_hot_add_init() which is only called from an ACPI bus scan in
> >  which _STA has already been queried there is no need to repeat it here.
> >  Add a comment to remind us of this in the future.
> >  
> >  Suggested-by: Rafael J. Wysocki <rafael@xxxxxxxxxx>
> >  Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> >  ---
> >  v6: Add protection again Physical CPU HP to the arch specific code
> >      and don't actually check _STA
> >  
> >  Tested on arm64 with ACPI + DT build and DT only builds, booting with ACPI
> >  and DT as appropriate.
> >  ---
> >   arch/arm64/kernel/smp.c | 53
> >  +++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 53 insertions(+)
> >  
> >  diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c index
> >  dc0e0b3ec2d4..ccb6ad347df9 100644
> >  --- a/arch/arm64/kernel/smp.c
> >  +++ b/arch/arm64/kernel/smp.c
> >  @@ -504,6 +504,59 @@ static int __init smp_cpu_setup(int cpu)  static bool
> >  bootcpu_valid __initdata;  static unsigned int cpu_count = 1;
> >  
> >  +int arch_register_cpu(int cpu)
> >  +{
> >  +	acpi_handle acpi_handle = acpi_get_processor_handle(cpu);
> >  +	struct cpu *c = &per_cpu(cpu_devices, cpu);
> >  +
> >  +	if (!acpi_disabled && !acpi_handle &&
> >  +	    IS_ENABLED(CONFIG_ACPI_HOTPLUG_CPU))
> >  +		return -EPROBE_DEFER;
> >  +
> >  +#ifdef CONFIG_ACPI_HOTPLUG_CPU
> >  +	/* For now block anything that looks like physical CPU Hotplug */
> >  +	if (invalid_logical_cpuid(cpu) || !cpu_present(cpu)) {
> >  +		pr_err_once("Changing CPU present bit is not
> >  supported\n");
> >  +		return -ENODEV;
> >  +	}
> >  +#endif
> >  +
> >  +	/*
> >  +	 * Availability of the acpi handle is sufficient to establish
> >  +	 * that _STA has aleady been checked. No need to recheck here.
> >  +	 */
> >  +	c->hotpluggable = arch_cpu_is_hotpluggable(cpu);
> >  +  
> 
> 
> We would still need 'enabled' bitmask as applications need a way to clearly
> get which processors are enabled and usable in case of ARM64. Otherwise,
> they will end up scanning the entire MAX CPU space to figure out which
> processors have been plugged or unplugged. It is inefficient to bank upon
> errors to detect this and unnecessary to scan again and again.
>            
> +            set_cpu_enabled(cpu, true);   // will need this change
> 
> 
> And its corresponding additions of enabled bitmask along side the present masks.
> 
> I think we had this discussion in Linaro Open Discussions group few years
> back.

Agreed - but if I understand correctly that is  handled in patch 16 -
which introduced the enabled bitmask. I tested that works and it all seems fine.
Done for all architectures in register_cpu() and unregister_cpu() rather
than in arch specific code.

Jonathan


> 
> 
> >  +	return register_cpu(c, cpu);
> >  +}
> >  +
> >  +#ifdef CONFIG_ACPI_HOTPLUG_CPU
> >  +void arch_unregister_cpu(int cpu)
> >  +{
> >  +	acpi_handle acpi_handle = acpi_get_processor_handle(cpu);
> >  +	struct cpu *c = &per_cpu(cpu_devices, cpu);
> >  +	acpi_status status;
> >  +	unsigned long long sta;
> >  +
> >  +	if (!acpi_handle) {
> >  +		pr_err_once("Removing a CPU without associated ACPI
> >  handle\n");
> >  +		return;
> >  +	}
> >  +
> >  +	status = acpi_evaluate_integer(acpi_handle, "_STA", NULL, &sta);
> >  +	if (ACPI_FAILURE(status))
> >  +		return;
> >  +
> >  +	/* For now do not allow anything that looks like physical CPU HP */
> >  +	if (cpu_present(cpu) && !(sta & ACPI_STA_DEVICE_PRESENT)) {
> >  +		pr_err_once("Changing CPU present bit is not
> >  supported\n");
> >  +		return;
> >  +	}
> >  +  
> 
> For the same reasons as above:
> 
> +            set_cpu_enabled(cpu, flase);   // will need this change
> 
> 
> >  +	unregister_cpu(c);
> >  +}
> >  +#endif /* CONFIG_ACPI_HOTPLUG_CPU */
> >  +
> >   #ifdef CONFIG_ACPI
> >   static struct acpi_madt_generic_interrupt cpu_madt_gicc[NR_CPUS];
> >  
> >  --
> >  2.39.2  
>