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
>
