| Message ID | 20230913163823.7880-33-james.morse@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | ACPI/arm64: add support for virtual cpuhotplug | expand |
On Wed, 13 Sep 2023 16:38:20 +0000 James Morse <james.morse@arm.com> wrote: > acpi_processor_get_info() registers all present CPUs. Registering a > CPU is what creates the sysfs entries and triggers the udev > notifications. > > arm64 virtual machines that support 'virtual cpu hotplug' use the > enabled bit to indicate whether the CPU can be brought online, as > the existing ACPI tables require all hardware to be described and > present. > > If firmware describes a CPU as present, but disabled, skip the > registration. Such CPUs are present, but can't be brought online for > whatever reason. (e.g. firmware/hypervisor policy). > > Once firmware sets the enabled bit, the CPU can be registered and > brought online by user-space. Online CPUs, or CPUs that are missing > an _STA method must always be registered. > > Signed-off-by: James Morse <james.morse@arm.com> A small argument with myself inline. Feel free to ignore. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/acpi/acpi_processor.c | 31 ++++++++++++++++++++++++++++++- > 1 file changed, 30 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c > index b67616079751..b49859eab01a 100644 > --- a/drivers/acpi/acpi_processor.c > +++ b/drivers/acpi/acpi_processor.c > @@ -227,6 +227,32 @@ static int acpi_processor_make_present(struct acpi_processor *pr) > return ret; > } > > +static int acpi_processor_make_enabled(struct acpi_processor *pr) > +{ > + unsigned long long sta; > + acpi_status status; > + bool present, enabled; > + > + if (!acpi_has_method(pr->handle, "_STA")) > + return arch_register_cpu(pr->id); > + > + status = acpi_evaluate_integer(pr->handle, "_STA", NULL, &sta); > + if (ACPI_FAILURE(status)) > + return -ENODEV; > + > + present = sta & ACPI_STA_DEVICE_PRESENT; > + enabled = sta & ACPI_STA_DEVICE_ENABLED; > + > + if (cpu_online(pr->id) && (!present || !enabled)) { > + pr_err_once(FW_BUG "CPU %u is online, but described as not present or disabled!\n", pr->id); Why once? If this for some reason happened on multiple CPUs I think we'd want to know. > + add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK); > + } else if (!present || !enabled) { > + return -ENODEV; > + } I guess you didn't do a nested if here to avoid even longer lines. Could flip things around though I don't like this much either as it makes the normal good path exit mid way down. if (present && enabled) return arch_register_cpu(pr->id); if (!cpu_online(pr->id)) return -ENODEV; pr_err... add_taint(... return arch_register_cpu(pr->id); Ah well. Some code just has to be less than pretty. > + > + return arch_register_cpu(pr->id); > +} > + > static int acpi_processor_get_info(struct acpi_device *device) > { > union acpi_object object = { 0 }; > @@ -318,7 +344,7 @@ static int acpi_processor_get_info(struct acpi_device *device) > */ > if (!invalid_logical_cpuid(pr->id) && cpu_present(pr->id) && > !get_cpu_device(pr->id)) { > - int ret = arch_register_cpu(pr->id); > + int ret = acpi_processor_make_enabled(pr); > > if (ret) > return ret; > @@ -526,6 +552,9 @@ static void acpi_processor_post_eject(struct acpi_device *device) > acpi_processor_make_not_present(device); > return; > } > + > + if (cpu_present(pr->id) && !(sta & ACPI_STA_DEVICE_ENABLED)) > + arch_unregister_cpu(pr->id); > } > > #ifdef CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC
On 9/14/23 02:38, James Morse wrote: > acpi_processor_get_info() registers all present CPUs. Registering a > CPU is what creates the sysfs entries and triggers the udev > notifications. > > arm64 virtual machines that support 'virtual cpu hotplug' use the > enabled bit to indicate whether the CPU can be brought online, as > the existing ACPI tables require all hardware to be described and > present. > > If firmware describes a CPU as present, but disabled, skip the > registration. Such CPUs are present, but can't be brought online for > whatever reason. (e.g. firmware/hypervisor policy). > > Once firmware sets the enabled bit, the CPU can be registered and > brought online by user-space. Online CPUs, or CPUs that are missing > an _STA method must always be registered. > > Signed-off-by: James Morse <james.morse@arm.com> > --- > drivers/acpi/acpi_processor.c | 31 ++++++++++++++++++++++++++++++- > 1 file changed, 30 insertions(+), 1 deletion(-) > With below nits addressed: Reviewed-by: Gavin Shan <gshan@redhat.com> > diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c > index b67616079751..b49859eab01a 100644 > --- a/drivers/acpi/acpi_processor.c > +++ b/drivers/acpi/acpi_processor.c > @@ -227,6 +227,32 @@ static int acpi_processor_make_present(struct acpi_processor *pr) > return ret; > } > > +static int acpi_processor_make_enabled(struct acpi_processor *pr) > +{ > + unsigned long long sta; > + acpi_status status; > + bool present, enabled; > + > + if (!acpi_has_method(pr->handle, "_STA")) > + return arch_register_cpu(pr->id); > + > + status = acpi_evaluate_integer(pr->handle, "_STA", NULL, &sta); > + if (ACPI_FAILURE(status)) > + return -ENODEV; > + > + present = sta & ACPI_STA_DEVICE_PRESENT; > + enabled = sta & ACPI_STA_DEVICE_ENABLED; > + > + if (cpu_online(pr->id) && (!present || !enabled)) { > + pr_err_once(FW_BUG "CPU %u is online, but described as not present or disabled!\n", pr->id); > + add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK); > + } else if (!present || !enabled) { > + return -ENODEV; > + } > + > + return arch_register_cpu(pr->id); > +} > + The message needs to be split up into multiple lines to make ./scripts/checkpatch.pl happy: pr_err_once(FW_BUG "CPU %u is online, but described " "as not present or disabled!\n", pr->id); > static int acpi_processor_get_info(struct acpi_device *device) > { > union acpi_object object = { 0 }; > @@ -318,7 +344,7 @@ static int acpi_processor_get_info(struct acpi_device *device) > */ > if (!invalid_logical_cpuid(pr->id) && cpu_present(pr->id) && > !get_cpu_device(pr->id)) { > - int ret = arch_register_cpu(pr->id); > + int ret = acpi_processor_make_enabled(pr); > > if (ret) > return ret; > @@ -526,6 +552,9 @@ static void acpi_processor_post_eject(struct acpi_device *device) > acpi_processor_make_not_present(device); > return; > } > + > + if (cpu_present(pr->id) && !(sta & ACPI_STA_DEVICE_ENABLED)) > + arch_unregister_cpu(pr->id); > } > > #ifdef CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC Thanks, Gavin
On Tue, Sep 19, 2023 at 02:46:22PM +1000, Gavin Shan wrote: > The message needs to be split up into multiple lines to make ./scripts/checkpatch.pl > happy: > > pr_err_once(FW_BUG "CPU %u is online, but described " > "as not present or disabled!\n", pr->id); No. checkpatch is wrong on this point. Splitting the message like this hurts debuggability, because the message can no longer be grepped for. What James has done is perfectly fine.
On Thu, Sep 14, 2023 at 05:13:41PM +0100, Jonathan Cameron wrote: > On Wed, 13 Sep 2023 16:38:20 +0000 > James Morse <james.morse@arm.com> wrote: > > +static int acpi_processor_make_enabled(struct acpi_processor *pr) > > +{ > > + unsigned long long sta; > > + acpi_status status; > > + bool present, enabled; > > + > > + if (!acpi_has_method(pr->handle, "_STA")) > > + return arch_register_cpu(pr->id); > > + > > + status = acpi_evaluate_integer(pr->handle, "_STA", NULL, &sta); > > + if (ACPI_FAILURE(status)) > > + return -ENODEV; > > + > > + present = sta & ACPI_STA_DEVICE_PRESENT; > > + enabled = sta & ACPI_STA_DEVICE_ENABLED; > > + > > + if (cpu_online(pr->id) && (!present || !enabled)) { > > + pr_err_once(FW_BUG "CPU %u is online, but described as not present or disabled!\n", pr->id); > > Why once? If this for some reason happened on multiple CPUs I think we'd want to know. > > > + add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK); > > + } else if (!present || !enabled) { > > + return -ENODEV; > > + } > > I guess you didn't do a nested if here to avoid even longer lines. > Could flip things around though I don't like this much either as it makes > the normal good path exit mid way down. > > if (present && enabled) > return arch_register_cpu(pr->id); > > if (!cpu_online(pr->id)) > return -ENODEV; > > pr_err... > add_taint(... > > return arch_register_cpu(pr->id); > > Ah well. Some code just has to be less than pretty. How about: static int acpi_processor_should_register_cpu(struct acpi_processor *pr) { unsigned long long sta; acpi_status status; if (!acpi_has_method(pr->handle, "_STA")) return 0; status = acpi_evaluate_integer(pr->handle, "_STA", NULL, &sta); if (ACPI_FAILURE(status)) return -ENODEV; if (sta & ACPI_STA_DEVICE_PRESENT && sta & ACPI_STA_DEVICE_ENABLED) return 0; if (cpu_online(pr->id)) { pr_err_once(FW_BUG "CPU %u is online, but described as not present or disabled!\n", pr->id); /* Register the CPU anyway */ return 0; } return -ENODEV; } static int acpi_processor_make_enabled(struct acpi_processor *pr) { int ret = acpi_processor_should_register_cpu(pr); if (ret) return ret; return arch_register_cpu(pr->id); } I would keep the "cpu online but !present and !disabled" as a sub-block because it makes better reading flow, but instead break the message as I've indicated above.
diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c index b67616079751..b49859eab01a 100644 --- a/drivers/acpi/acpi_processor.c +++ b/drivers/acpi/acpi_processor.c @@ -227,6 +227,32 @@ static int acpi_processor_make_present(struct acpi_processor *pr) return ret; } +static int acpi_processor_make_enabled(struct acpi_processor *pr) +{ + unsigned long long sta; + acpi_status status; + bool present, enabled; + + if (!acpi_has_method(pr->handle, "_STA")) + return arch_register_cpu(pr->id); + + status = acpi_evaluate_integer(pr->handle, "_STA", NULL, &sta); + if (ACPI_FAILURE(status)) + return -ENODEV; + + present = sta & ACPI_STA_DEVICE_PRESENT; + enabled = sta & ACPI_STA_DEVICE_ENABLED; + + if (cpu_online(pr->id) && (!present || !enabled)) { + pr_err_once(FW_BUG "CPU %u is online, but described as not present or disabled!\n", pr->id); + add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK); + } else if (!present || !enabled) { + return -ENODEV; + } + + return arch_register_cpu(pr->id); +} + static int acpi_processor_get_info(struct acpi_device *device) { union acpi_object object = { 0 }; @@ -318,7 +344,7 @@ static int acpi_processor_get_info(struct acpi_device *device) */ if (!invalid_logical_cpuid(pr->id) && cpu_present(pr->id) && !get_cpu_device(pr->id)) { - int ret = arch_register_cpu(pr->id); + int ret = acpi_processor_make_enabled(pr); if (ret) return ret; @@ -526,6 +552,9 @@ static void acpi_processor_post_eject(struct acpi_device *device) acpi_processor_make_not_present(device); return; } + + if (cpu_present(pr->id) && !(sta & ACPI_STA_DEVICE_ENABLED)) + arch_unregister_cpu(pr->id); } #ifdef CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC
acpi_processor_get_info() registers all present CPUs. Registering a CPU is what creates the sysfs entries and triggers the udev notifications. arm64 virtual machines that support 'virtual cpu hotplug' use the enabled bit to indicate whether the CPU can be brought online, as the existing ACPI tables require all hardware to be described and present. If firmware describes a CPU as present, but disabled, skip the registration. Such CPUs are present, but can't be brought online for whatever reason. (e.g. firmware/hypervisor policy). Once firmware sets the enabled bit, the CPU can be registered and brought online by user-space. Online CPUs, or CPUs that are missing an _STA method must always be registered. Signed-off-by: James Morse <james.morse@arm.com> --- drivers/acpi/acpi_processor.c | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-)