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

[Salil Mehta]

>  From: Jonathan Cameron <jonathan.cameron@xxxxxxxxxx>
>  Sent: Wednesday, April 17, 2024 5:55 PM
>  
>  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.


Sorry, I missed that. Yes, this logic is already present in later patches.


Thanks
Salil.


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