[PATCH v8 00/16] ACPI/arm64: add support for virtual cpu hotplug

[PATCH v8 00/16] ACPI/arm64: add support for virtual cpu hotplug

[Jonathan Cameron]

Thanks to Rafael, Marc, Hanjun and Gavin for reviews of v8 and tags.

v8: Details and more minor stuff in individual patch descriptions
  - Rewrite the handling of not enabled redistributors in the gicv3 driver
    to keep impacts local. Note this needs a dance with a new cpuhp
    callback that runs before CPU bring up reaches the section that
    cannot fail. This avoids clearing bits in cpu_present_mask and
    cpu_possible_mask making the solution much less fragile.
  - Do not carry on for that CPU if acpi_processor_set_per_cpu() detects
    a problem, but continue to leave enough in place to perform buggy bios
    checks.
  - Protect against a buggy BIOS that has apparently enabled CPUs in DSDT
    that are not in MADT. The check makes sense anyway as on arm64 we have
    not way to 'generate' an ID if one isn't present at acpi_map_cpu().

Updated version of James' original introduction.

This series adds what looks like cpuhotplug support to arm64 for use in
virtual machines. It does this by moving the cpu_register() calls for
architectures that support ACPI into an arch specific call made from
the ACPI processor driver.
 
The kubernetes folk really want to be able to add CPUs to an existing VM,
in exactly the same way they do on x86. The use-case is pre-booting guests
with one CPU, then adding the number that were actually needed when the
workload is provisioned.

Wait? Doesn't arm64 support cpuhotplug already!?
In the arm world, cpuhotplug gets used to mean removing the power from a CPU.
The CPU is offline, and remains present. For x86, and ACPI, cpuhotplug
has the additional step of physically removing the CPU, so that it isn't
present anymore.
 
Arm64 doesn't support this, and can't support it: CPUs are really a slice
of the SoC, and there is not enough information in the existing ACPI tables
to describe which bits of the slice also got removed. Without a reference
machine: adding this support to the spec is a wild goose chase.
 
Critically: everything described in the firmware tables must remain present.
 
For a virtual machine this is easy as all the other bits of 'virtual SoC'
are emulated, so they can (and do) remain present when a vCPU is 'removed'.

On a system that supports cpuhotplug the MADT has to describe every possible
CPU at boot. Under KVM, the vGIC needs to know about every possible vCPU before
the guest is started.
With these constraints, virtual-cpuhotplug is really just a hypervisor/firmware
policy about which CPUs can be brought online.
 
This series adds support for virtual-cpuhotplug as exactly that: firmware
policy. This may even work on a physical machine too; for a guest the part of
firmware is played by the VMM. (typically Qemu).
 
PSCI support is modified to return 'DENIED' if the CPU can't be brought
online/enabled yet. The CPU object's _STA method's enabled bit is used to
indicate firmware's current disposition. If the CPU has its enabled bit clear,
it will not be registered with sysfs, and attempts to bring it online will
fail. The notifications that _STA has changed its value then work in the same
way as physical hotplug, and firmware can cause the CPU to be registered some
time later, allowing it to be brought online.
 
This creates something that looks like cpuhotplug to user-space and the
kernel beyond arm64 architecture specific code, as the sysfs
files appear and disappear, and the udev notifications look the same.
 
One notable difference is the CPU present mask, which is exposed via sysfs.
Because the CPUs remain present throughout, they can still be seen in that mask.
This value does get used by webbrowsers to estimate the number of CPUs
as the CPU online mask is constantly changed on mobile phones.
 
Linux is tolerant of PSCI returning errors, as its always been allowed to do
that. To avoid confusing OS that can't tolerate this, we needed an additional
bit in the MADT GICC flags. This series copies ACPI_MADT_ONLINE_CAPABLE, which
appears to be for this purpose, but calls it ACPI_MADT_GICC_CPU_CAPABLE as it
has a different bit position in the GICC.
 
This code is unconditionally enabled for all ACPI architectures, though for
now only arm64 will have deferred the cpu_register() calls.

If folk want to play along at home, you'll need a copy of Qemu that supports this.
https://github.com/salil-mehta/qemu.git virt-cpuhp-armv8/rfc-v2

Replace your '-smp' argument with something like:
 | -smp cpus=1,maxcpus=3,cores=3,threads=1,sockets=1
 
 then feed the following to the Qemu montior;
 | (qemu) device_add driver=host-arm-cpu,core-id=1,id=cpu1
 | (qemu) device_del cpu1
 

James Morse (7):
  ACPI: processor: Register deferred CPUs from acpi_processor_get_info()
  ACPI: Add post_eject to struct acpi_scan_handler for cpu hotplug
  arm64: acpi: Move get_cpu_for_acpi_id() to a header
  irqchip/gic-v3: Don't return errors from gic_acpi_match_gicc()
  irqchip/gic-v3: Add support for ACPI's disabled but 'online capable'
    CPUs
  arm64: document virtual CPU hotplug's expectations
  cpumask: Add enabled cpumask for present CPUs that can be brought
    online

Jean-Philippe Brucker (1):
  arm64: psci: Ignore DENIED CPUs

Jonathan Cameron (8):
  ACPI: processor: Simplify initial onlining to use same path for cold
    and hotplug
  cpu: Do not warn on arch_register_cpu() returning -EPROBE_DEFER
  ACPI: processor: Drop duplicated check on _STA (enabled + present)
  ACPI: processor: Move checks and availability of acpi_processor
    earlier
  ACPI: processor: Add acpi_get_processor_handle() helper
  ACPI: scan: switch to flags for acpi_scan_check_and_detach()
  arm64: arch_register_cpu() variant to check if an ACPI handle is now
    available.
  arm64: Kconfig: Enable hotplug CPU on arm64 if ACPI_PROCESSOR is
    enabled.

 .../ABI/testing/sysfs-devices-system-cpu      |   6 +
 Documentation/arch/arm64/cpu-hotplug.rst      |  79 ++++++++++++
 Documentation/arch/arm64/index.rst            |   1 +
 arch/arm64/Kconfig                            |   1 +
 arch/arm64/include/asm/acpi.h                 |  11 ++
 arch/arm64/kernel/acpi.c                      |  22 ++++
 arch/arm64/kernel/acpi_numa.c                 |  11 --
 arch/arm64/kernel/psci.c                      |   2 +-
 arch/arm64/kernel/smp.c                       |  56 ++++++++-
 drivers/acpi/acpi_processor.c                 | 115 +++++++++++-------
 drivers/acpi/processor_driver.c               |  44 ++-----
 drivers/acpi/scan.c                           |  47 +++++--
 drivers/base/cpu.c                            |  12 +-
 drivers/irqchip/irq-gic-v3.c                  |  49 ++++++--
 include/acpi/acpi_bus.h                       |   1 +
 include/acpi/processor.h                      |   2 +-
 include/linux/acpi.h                          |  10 +-
 include/linux/cpumask.h                       |  25 ++++
 kernel/cpu.c                                  |   3 +
 19 files changed, 383 insertions(+), 114 deletions(-)
 create mode 100644 Documentation/arch/arm64/cpu-hotplug.rst

-- 
2.39.2

[PATCH v8 01/16] ACPI: processor: Simplify initial onlining to use same path for cold and hotplug

[Jonathan Cameron]

Separate code paths, combined with a flag set in acpi_processor.c to
indicate a struct acpi_processor was for a hotplugged CPU ensured that
per CPU data was only set up the first time that a CPU was initialized.
This appears to be unnecessary as the paths can be combined by letting
the online logic also handle any CPUs online at the time of driver load.

Motivation for this change, beyond simplification, is that ARM64
virtual CPU HP uses the same code paths for hotplug and cold path in
acpi_processor.c so had no easy way to set the flag for hotplug only.
Removing this necessity will enable ARM64 vCPU HP to reuse the existing
code paths.

Leave noisy pr_info() in place but update it to not state the CPU
was hotplugged.

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Hanjun Guo <guohanjun@huawei.com>
Tested-by: Miguel Luis <miguel.luis@oracle.com>
Reviewed-by: Gavin Shan <gshan@redhat.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

---
v8: No change
---
 drivers/acpi/acpi_processor.c   |  1 -
 drivers/acpi/processor_driver.c | 44 ++++++++++-----------------------
 include/acpi/processor.h        |  2 +-
 3 files changed, 14 insertions(+), 33 deletions(-)

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index 7a0dd35d62c9..7fc924aeeed0 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -216,7 +216,6 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
 	 * gets online for the first time.
 	 */
 	pr_info("CPU%d has been hot-added\n", pr->id);
-	pr->flags.need_hotplug_init = 1;
 
 out:
 	cpus_write_unlock();
diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index 67db60eda370..55782eac3ff1 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -33,7 +33,6 @@ MODULE_AUTHOR("Paul Diefenbaugh");
 MODULE_DESCRIPTION("ACPI Processor Driver");
 MODULE_LICENSE("GPL");
 
-static int acpi_processor_start(struct device *dev);
 static int acpi_processor_stop(struct device *dev);
 
 static const struct acpi_device_id processor_device_ids[] = {
@@ -47,7 +46,6 @@ static struct device_driver acpi_processor_driver = {
 	.name = "processor",
 	.bus = &cpu_subsys,
 	.acpi_match_table = processor_device_ids,
-	.probe = acpi_processor_start,
 	.remove = acpi_processor_stop,
 };
 
@@ -115,12 +113,10 @@ static int acpi_soft_cpu_online(unsigned int cpu)
 	 * CPU got physically hotplugged and onlined for the first time:
 	 * Initialize missing things.
 	 */
-	if (pr->flags.need_hotplug_init) {
+	if (!pr->flags.previously_online) {
 		int ret;
 
-		pr_info("Will online and init hotplugged CPU: %d\n",
-			pr->id);
-		pr->flags.need_hotplug_init = 0;
+		pr_info("Will online and init CPU: %d\n", pr->id);
 		ret = __acpi_processor_start(device);
 		WARN(ret, "Failed to start CPU: %d\n", pr->id);
 	} else {
@@ -167,9 +163,6 @@ static int __acpi_processor_start(struct acpi_device *device)
 	if (!pr)
 		return -ENODEV;
 
-	if (pr->flags.need_hotplug_init)
-		return 0;
-
 	result = acpi_cppc_processor_probe(pr);
 	if (result && !IS_ENABLED(CONFIG_ACPI_CPU_FREQ_PSS))
 		dev_dbg(&device->dev, "CPPC data invalid or not present\n");
@@ -185,32 +178,21 @@ static int __acpi_processor_start(struct acpi_device *device)
 
 	status = acpi_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
 					     acpi_processor_notify, device);
-	if (ACPI_SUCCESS(status))
-		return 0;
+	if (!ACPI_SUCCESS(status)) {
+		result = -ENODEV;
+		goto err_thermal_exit;
+	}
+	pr->flags.previously_online = 1;
 
-	result = -ENODEV;
-	acpi_processor_thermal_exit(pr, device);
+	return 0;
 
+err_thermal_exit:
+	acpi_processor_thermal_exit(pr, device);
 err_power_exit:
 	acpi_processor_power_exit(pr);
 	return result;
 }
 
-static int acpi_processor_start(struct device *dev)
-{
-	struct acpi_device *device = ACPI_COMPANION(dev);
-	int ret;
-
-	if (!device)
-		return -ENODEV;
-
-	/* Protect against concurrent CPU hotplug operations */
-	cpu_hotplug_disable();
-	ret = __acpi_processor_start(device);
-	cpu_hotplug_enable();
-	return ret;
-}
-
 static int acpi_processor_stop(struct device *dev)
 {
 	struct acpi_device *device = ACPI_COMPANION(dev);
@@ -279,9 +261,9 @@ static int __init acpi_processor_driver_init(void)
 	if (result < 0)
 		return result;
 
-	result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
-					   "acpi/cpu-drv:online",
-					   acpi_soft_cpu_online, NULL);
+	result = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
+				   "acpi/cpu-drv:online",
+				   acpi_soft_cpu_online, NULL);
 	if (result < 0)
 		goto err;
 	hp_online = result;
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 3f34ebb27525..e6f6074eadbf 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -217,7 +217,7 @@ struct acpi_processor_flags {
 	u8 has_lpi:1;
 	u8 power_setup_done:1;
 	u8 bm_rld_set:1;
-	u8 need_hotplug_init:1;
+	u8 previously_online:1;
 };
 
 struct acpi_processor {
-- 
2.39.2

[PATCH v8 02/16] cpu: Do not warn on arch_register_cpu() returning -EPROBE_DEFER

[Jonathan Cameron]

For arm64 the CPU registration cannot complete until the ACPI
interpreter us up and running so in those cases the arch specific
arch_register_cpu() will return -EPROBE_DEFER at this stage and the
registration will be attempted later.

Suggested-by: Rafael J. Wysocki <rafael@kernel.org>
Acked-by: Rafael J. Wysocki <rafael@kernel.org>
Reviewed-by: Hanjun Guo <guohanjun@huawei.com>
Reviewed-by: Gavin Shan <gshan@redhat.com>
Tested-by: Miguel Luis <miguel.luis@oracle.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

---
v8: No change
---
 drivers/base/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index 56fba44ba391..7b83e9c87d7c 100644
--- a/drivers/base/cpu.c
+++ b/drivers/base/cpu.c
@@ -558,7 +558,7 @@ static void __init cpu_dev_register_generic(void)
 
 	for_each_present_cpu(i) {
 		ret = arch_register_cpu(i);
-		if (ret)
+		if (ret && ret != -EPROBE_DEFER)
 			pr_warn("register_cpu %d failed (%d)\n", i, ret);
 	}
 }
-- 
2.39.2

[PATCH v8 03/16] ACPI: processor: Drop duplicated check on _STA (enabled + present)

[Jonathan Cameron]

The ACPI bus scan will only result in acpi_processor_add() being called
if _STA has already been checked and the result is that the
processor is enabled and present.  Hence drop this additional check.

Suggested-by: Rafael J. Wysocki <rafael@kernel.org>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Hanjun Guo <guohanjun@huawei.com>
Reviewed-by: Gavin Shan <gshan@redhat.com>
Tested-by: Miguel Luis <miguel.luis@oracle.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

---
V8: No change (tags added)
---
 drivers/acpi/acpi_processor.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index 7fc924aeeed0..ba0a6f0ac841 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -186,17 +186,11 @@ static void __init acpi_pcc_cpufreq_init(void) {}
 #ifdef CONFIG_ACPI_HOTPLUG_CPU
 static int acpi_processor_hotadd_init(struct acpi_processor *pr)
 {
-	unsigned long long sta;
-	acpi_status status;
 	int ret;
 
 	if (invalid_phys_cpuid(pr->phys_id))
 		return -ENODEV;
 
-	status = acpi_evaluate_integer(pr->handle, "_STA", NULL, &sta);
-	if (ACPI_FAILURE(status) || !(sta & ACPI_STA_DEVICE_PRESENT))
-		return -ENODEV;
-
 	cpu_maps_update_begin();
 	cpus_write_lock();
 
-- 
2.39.2

[PATCH v8 04/16] ACPI: processor: Move checks and availability of acpi_processor earlier

[Jonathan Cameron]

Make the per_cpu(processors, cpu) entries available earlier so that
they are available in arch_register_cpu() as ARM64 will need access
to the acpi_handle to distinguish between acpi_processor_add()
and earlier registration attempts (which will fail as _STA cannot
be checked).

Reorder the remove flow to clear this per_cpu() after
arch_unregister_cpu() has completed, allowing it to be used in
there as well.

Note that on x86 for the CPU hotplug case, the pr->id prior to
acpi_map_cpu() may be invalid. Thus the per_cpu() structures
must be initialized after that call or after checking the ID
is valid (not hotplug path).

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

---
v8: On buggy bios detection when setting per_cpu structures
    do not carry on.
    Fix up the clearing of per cpu structures to remove unwanted
    side effects and ensure an error code isn't use to reference them.
---
 drivers/acpi/acpi_processor.c | 79 +++++++++++++++++++++--------------
 1 file changed, 48 insertions(+), 31 deletions(-)

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index ba0a6f0ac841..3b180e21f325 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -183,8 +183,38 @@ static void __init acpi_pcc_cpufreq_init(void) {}
 #endif /* CONFIG_X86 */
 
 /* Initialization */
+static DEFINE_PER_CPU(void *, processor_device_array);
+
+static bool acpi_processor_set_per_cpu(struct acpi_processor *pr,
+				       struct acpi_device *device)
+{
+	BUG_ON(pr->id >= nr_cpu_ids);
+	/*
+	 * Buggy BIOS check.
+	 * ACPI id of processors can be reported wrongly by the BIOS.
+	 * Don't trust it blindly
+	 */
+	if (per_cpu(processor_device_array, pr->id) != NULL &&
+	    per_cpu(processor_device_array, pr->id) != device) {
+		dev_warn(&device->dev,
+			 "BIOS reported wrong ACPI id %d for the processor\n",
+			 pr->id);
+		/* Give up, but do not abort the namespace scan. */
+		return false;
+	}
+	/*
+	 * processor_device_array is not cleared on errors to allow buggy BIOS
+	 * checks.
+	 */
+	per_cpu(processor_device_array, pr->id) = device;
+	per_cpu(processors, pr->id) = pr;
+
+	return true;
+}
+
 #ifdef CONFIG_ACPI_HOTPLUG_CPU
-static int acpi_processor_hotadd_init(struct acpi_processor *pr)
+static int acpi_processor_hotadd_init(struct acpi_processor *pr,
+				      struct acpi_device *device)
 {
 	int ret;
 
@@ -198,8 +228,15 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
 	if (ret)
 		goto out;
 
+	if (!acpi_processor_set_per_cpu(pr, device)) {
+		acpi_unmap_cpu(pr->id);
+		goto out;
+	}
+
 	ret = arch_register_cpu(pr->id);
 	if (ret) {
+		/* Leave the processor device array in place to detect buggy bios */
+		per_cpu(processors, pr->id) = NULL;
 		acpi_unmap_cpu(pr->id);
 		goto out;
 	}
@@ -217,7 +254,8 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
 	return ret;
 }
 #else
-static inline int acpi_processor_hotadd_init(struct acpi_processor *pr)
+static inline int acpi_processor_hotadd_init(struct acpi_processor *pr,
+					     struct acpi_device *device)
 {
 	return -ENODEV;
 }
@@ -316,10 +354,13 @@ static int acpi_processor_get_info(struct acpi_device *device)
 	 *  because cpuid <-> apicid mapping is persistent now.
 	 */
 	if (invalid_logical_cpuid(pr->id) || !cpu_present(pr->id)) {
-		int ret = acpi_processor_hotadd_init(pr);
+		int ret = acpi_processor_hotadd_init(pr, device);
 
 		if (ret)
 			return ret;
+	} else {
+		if (!acpi_processor_set_per_cpu(pr, device))
+			return 0;
 	}
 
 	/*
@@ -365,8 +406,6 @@ static int acpi_processor_get_info(struct acpi_device *device)
  * (cpu_data(cpu)) values, like CPU feature flags, family, model, etc.
  * Such things have to be put in and set up by the processor driver's .probe().
  */
-static DEFINE_PER_CPU(void *, processor_device_array);
-
 static int acpi_processor_add(struct acpi_device *device,
 					const struct acpi_device_id *id)
 {
@@ -395,28 +434,6 @@ static int acpi_processor_add(struct acpi_device *device,
 	if (result) /* Processor is not physically present or unavailable */
 		return 0;
 
-	BUG_ON(pr->id >= nr_cpu_ids);
-
-	/*
-	 * Buggy BIOS check.
-	 * ACPI id of processors can be reported wrongly by the BIOS.
-	 * Don't trust it blindly
-	 */
-	if (per_cpu(processor_device_array, pr->id) != NULL &&
-	    per_cpu(processor_device_array, pr->id) != device) {
-		dev_warn(&device->dev,
-			"BIOS reported wrong ACPI id %d for the processor\n",
-			pr->id);
-		/* Give up, but do not abort the namespace scan. */
-		goto err;
-	}
-	/*
-	 * processor_device_array is not cleared on errors to allow buggy BIOS
-	 * checks.
-	 */
-	per_cpu(processor_device_array, pr->id) = device;
-	per_cpu(processors, pr->id) = pr;
-
 	dev = get_cpu_device(pr->id);
 	if (!dev) {
 		result = -ENODEV;
@@ -469,10 +486,6 @@ static void acpi_processor_remove(struct acpi_device *device)
 	device_release_driver(pr->dev);
 	acpi_unbind_one(pr->dev);
 
-	/* Clean up. */
-	per_cpu(processor_device_array, pr->id) = NULL;
-	per_cpu(processors, pr->id) = NULL;
-
 	cpu_maps_update_begin();
 	cpus_write_lock();
 
@@ -480,6 +493,10 @@ static void acpi_processor_remove(struct acpi_device *device)
 	arch_unregister_cpu(pr->id);
 	acpi_unmap_cpu(pr->id);
 
+	/* Clean up. */
+	per_cpu(processor_device_array, pr->id) = NULL;
+	per_cpu(processors, pr->id) = NULL;
+
 	cpus_write_unlock();
 	cpu_maps_update_done();
 
-- 
2.39.2

[PATCH v8 05/16] ACPI: processor: Add acpi_get_processor_handle() helper

[Jonathan Cameron]

If CONFIG_ACPI_PROCESSOR provide a helper to retrieve the
acpi_handle for a given CPU allowing access to methods
in DSDT.

Tested-by: Miguel Luis <miguel.luis@oracle.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

---
v8: Code simplification suggested by Rafael.
    Fixup ;; spotted by Gavin
---
 drivers/acpi/acpi_processor.c | 11 +++++++++++
 include/linux/acpi.h          |  7 +++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index 3b180e21f325..ecc2721fecae 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -35,6 +35,17 @@ EXPORT_PER_CPU_SYMBOL(processors);
 struct acpi_processor_errata errata __read_mostly;
 EXPORT_SYMBOL_GPL(errata);
 
+acpi_handle acpi_get_processor_handle(int cpu)
+{
+	struct acpi_processor *pr;
+
+	pr = per_cpu(processors, cpu);
+	if (pr)
+		return pr->handle;
+
+	return NULL;
+}
+
 static int acpi_processor_errata_piix4(struct pci_dev *dev)
 {
 	u8 value1 = 0;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 34829f2c517a..9844a3f9c4e5 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -309,6 +309,8 @@ int acpi_map_cpu(acpi_handle handle, phys_cpuid_t physid, u32 acpi_id,
 int acpi_unmap_cpu(int cpu);
 #endif /* CONFIG_ACPI_HOTPLUG_CPU */
 
+acpi_handle acpi_get_processor_handle(int cpu);
+
 #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC
 int acpi_get_ioapic_id(acpi_handle handle, u32 gsi_base, u64 *phys_addr);
 #endif
@@ -1077,6 +1079,11 @@ static inline bool acpi_sleep_state_supported(u8 sleep_state)
 	return false;
 }
 
+static inline acpi_handle acpi_get_processor_handle(int cpu)
+{
+	return NULL;
+}
+
 #endif	/* !CONFIG_ACPI */
 
 extern void arch_post_acpi_subsys_init(void);
-- 
2.39.2

[PATCH v8 06/16] ACPI: processor: Register deferred CPUs from acpi_processor_get_info()

[Jonathan Cameron]

From: James Morse <james.morse@arm.com>

The arm64 specific arch_register_cpu() call may defer CPU registration
until the ACPI interpreter is available and the _STA method can
be evaluated.

If this occurs, then a second attempt is made in
acpi_processor_get_info(). Note that the arm64 specific call has
not yet been added so for now this will be called for the original
hotplug case.

For architectures that do not defer until the ACPI Processor
driver loads (e.g. x86), for initially present CPUs there will
already be a CPU device. If present do not try to register again.

Systems can still be booted with 'acpi=off', or not include an
ACPI description at all as in these cases arch_register_cpu()
will not have deferred registration when first called.

This moves the CPU register logic back to a subsys_initcall(),
while the memory nodes will have been registered earlier.
Note this is where the call was prior to the cleanup series so
there should be no side effects of moving it back again for this
specific case.

[PATCH 00/21] Initial cleanups for vCPU HP.
https://lore.kernel.org/all/ZVyz%2FVe5pPu8AWoA@shell.armlinux.org.uk/
commit 5b95f94c3b9f ("x86/topology: Switch over to GENERIC_CPU_DEVICES")

Signed-off-by: James Morse <james.morse@arm.com>
Reviewed-by: Gavin Shan <gshan@redhat.com>
Tested-by: Miguel Luis <miguel.luis@oracle.com>
Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
Tested-by: Jianyong Wu <jianyong.wu@arm.com>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Co-developed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Hanjun Guo <guohanjun@huawei.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

---
v8: Fxed spelling of my own name...
(tags picked up)
---
 drivers/acpi/acpi_processor.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index ecc2721fecae..88108aecbcfa 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -357,14 +357,14 @@ static int acpi_processor_get_info(struct acpi_device *device)
 	}
 
 	/*
-	 *  Extra Processor objects may be enumerated on MP systems with
-	 *  less than the max # of CPUs. They should be ignored _iff
-	 *  they are physically not present.
-	 *
-	 *  NOTE: Even if the processor has a cpuid, it may not be present
-	 *  because cpuid <-> apicid mapping is persistent now.
+	 *  This code is not called unless we know the CPU is present and
+	 *  enabled. The two paths are:
+	 *  a) Initially present CPUs on architectures that do not defer
+	 *     their arch_register_cpu() calls until this point.
+	 *  b) Hotplugged CPUs (enabled bit in _STA has transitioned from not
+	 *     enabled to enabled)
 	 */
-	if (invalid_logical_cpuid(pr->id) || !cpu_present(pr->id)) {
+	if (!get_cpu_device(pr->id)) {
 		int ret = acpi_processor_hotadd_init(pr, device);
 
 		if (ret)
-- 
2.39.2

[PATCH v8 07/16] ACPI: scan: switch to flags for acpi_scan_check_and_detach()

[Jonathan Cameron]

Precursor patch adds the ability to pass a uintptr_t of flags into
acpi_scan_check_and detach() so that additional flags can be
added to indicate whether to defer portions of the eject flow.
The new flag follows in the next patch.

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Hanjun Guo <guohanjun@huawei.com>
Reviewed-by: Gavin Shan <gshan@redhat.com>
Tested-by: Miguel Luis <miguel.luis@oracle.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

---
v8: No change (tags picked up)
---
 drivers/acpi/scan.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 7c157bf92695..fb5427caf0f4 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -244,13 +244,16 @@ static int acpi_scan_try_to_offline(struct acpi_device *device)
 	return 0;
 }
 
-static int acpi_scan_check_and_detach(struct acpi_device *adev, void *check)
+#define ACPI_SCAN_CHECK_FLAG_STATUS	BIT(0)
+
+static int acpi_scan_check_and_detach(struct acpi_device *adev, void *p)
 {
 	struct acpi_scan_handler *handler = adev->handler;
+	uintptr_t flags = (uintptr_t)p;
 
-	acpi_dev_for_each_child_reverse(adev, acpi_scan_check_and_detach, check);
+	acpi_dev_for_each_child_reverse(adev, acpi_scan_check_and_detach, p);
 
-	if (check) {
+	if (flags & ACPI_SCAN_CHECK_FLAG_STATUS) {
 		acpi_bus_get_status(adev);
 		/*
 		 * Skip devices that are still there and take the enabled
@@ -288,7 +291,9 @@ static int acpi_scan_check_and_detach(struct acpi_device *adev, void *check)
 
 static void acpi_scan_check_subtree(struct acpi_device *adev)
 {
-	acpi_scan_check_and_detach(adev, (void *)true);
+	uintptr_t flags = ACPI_SCAN_CHECK_FLAG_STATUS;
+
+	acpi_scan_check_and_detach(adev, (void *)flags);
 }
 
 static int acpi_scan_hot_remove(struct acpi_device *device)
@@ -2600,7 +2605,9 @@ EXPORT_SYMBOL(acpi_bus_scan);
  */
 void acpi_bus_trim(struct acpi_device *adev)
 {
-	acpi_scan_check_and_detach(adev, NULL);
+	uintptr_t flags = 0;
+
+	acpi_scan_check_and_detach(adev, (void *)flags);
 }
 EXPORT_SYMBOL_GPL(acpi_bus_trim);
 
-- 
2.39.2

[PATCH v8 08/16] ACPI: Add post_eject to struct acpi_scan_handler for cpu hotplug

[Jonathan Cameron]

From: James Morse <james.morse@arm.com>

struct acpi_scan_handler has a detach callback that is used to remove
a driver when a bus is changed. When interacting with an eject-request,
the detach callback is called before _EJ0.

This means the ACPI processor driver can't use _STA to determine if a
CPU has been made not-present, or some of the other _STA bits have been
changed. acpi_processor_remove() needs to know the value of _STA after
_EJ0 has been called.

Add a post_eject callback to struct acpi_scan_handler. This is called
after acpi_scan_hot_remove() has successfully called _EJ0. Because
acpi_scan_check_and_detach() also clears the handler pointer,
it needs to be told if the caller will go on to call
acpi_bus_post_eject(), so that acpi_device_clear_enumerated()
and clearing the handler pointer can be deferred.
An extra flag is added to flags field introduced in the previous
patch to achieve this.

Signed-off-by: James Morse <james.morse@arm.com>
Reviewed-by: Gavin Shan <gshan@redhat.com>
Tested-by: Miguel Luis <miguel.luis@oracle.com>
Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
Tested-by: Jianyong Wu <jianyong.wu@arm.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Hanjun Guo <guohanjun@huawei.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

---
v8: No change.
---
 drivers/acpi/acpi_processor.c |  4 ++--
 drivers/acpi/scan.c           | 30 +++++++++++++++++++++++++++---
 include/acpi/acpi_bus.h       |  1 +
 3 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index 88108aecbcfa..179d0dcc3847 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -475,7 +475,7 @@ static int acpi_processor_add(struct acpi_device *device,
 
 #ifdef CONFIG_ACPI_HOTPLUG_CPU
 /* Removal */
-static void acpi_processor_remove(struct acpi_device *device)
+static void acpi_processor_post_eject(struct acpi_device *device)
 {
 	struct acpi_processor *pr;
 
@@ -643,7 +643,7 @@ static struct acpi_scan_handler processor_handler = {
 	.ids = processor_device_ids,
 	.attach = acpi_processor_add,
 #ifdef CONFIG_ACPI_HOTPLUG_CPU
-	.detach = acpi_processor_remove,
+	.post_eject = acpi_processor_post_eject,
 #endif
 	.hotplug = {
 		.enabled = true,
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index fb5427caf0f4..7ebb3f9cea42 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -245,6 +245,7 @@ static int acpi_scan_try_to_offline(struct acpi_device *device)
 }
 
 #define ACPI_SCAN_CHECK_FLAG_STATUS	BIT(0)
+#define ACPI_SCAN_CHECK_FLAG_EJECT	BIT(1)
 
 static int acpi_scan_check_and_detach(struct acpi_device *adev, void *p)
 {
@@ -273,8 +274,6 @@ static int acpi_scan_check_and_detach(struct acpi_device *adev, void *p)
 	if (handler) {
 		if (handler->detach)
 			handler->detach(adev);
-
-		adev->handler = NULL;
 	} else {
 		device_release_driver(&adev->dev);
 	}
@@ -284,6 +283,28 @@ static int acpi_scan_check_and_detach(struct acpi_device *adev, void *p)
 	 */
 	acpi_device_set_power(adev, ACPI_STATE_D3_COLD);
 	adev->flags.initialized = false;
+
+	/* For eject this is deferred to acpi_bus_post_eject() */
+	if (!(flags & ACPI_SCAN_CHECK_FLAG_EJECT)) {
+		adev->handler = NULL;
+		acpi_device_clear_enumerated(adev);
+	}
+	return 0;
+}
+
+static int acpi_bus_post_eject(struct acpi_device *adev, void *not_used)
+{
+	struct acpi_scan_handler *handler = adev->handler;
+
+	acpi_dev_for_each_child_reverse(adev, acpi_bus_post_eject, NULL);
+
+	if (handler) {
+		if (handler->post_eject)
+			handler->post_eject(adev);
+
+		adev->handler = NULL;
+	}
+
 	acpi_device_clear_enumerated(adev);
 
 	return 0;
@@ -301,6 +322,7 @@ static int acpi_scan_hot_remove(struct acpi_device *device)
 	acpi_handle handle = device->handle;
 	unsigned long long sta;
 	acpi_status status;
+	uintptr_t flags = ACPI_SCAN_CHECK_FLAG_EJECT;
 
 	if (device->handler && device->handler->hotplug.demand_offline) {
 		if (!acpi_scan_is_offline(device, true))
@@ -313,7 +335,7 @@ static int acpi_scan_hot_remove(struct acpi_device *device)
 
 	acpi_handle_debug(handle, "Ejecting\n");
 
-	acpi_bus_trim(device);
+	acpi_scan_check_and_detach(device, (void *)flags);
 
 	acpi_evaluate_lck(handle, 0);
 	/*
@@ -336,6 +358,8 @@ static int acpi_scan_hot_remove(struct acpi_device *device)
 	} else if (sta & ACPI_STA_DEVICE_ENABLED) {
 		acpi_handle_warn(handle,
 			"Eject incomplete - status 0x%llx\n", sta);
+	} else {
+		acpi_bus_post_eject(device, NULL);
 	}
 
 	return 0;
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 5de954e2b18a..d5c0fb413697 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -129,6 +129,7 @@ struct acpi_scan_handler {
 	bool (*match)(const char *idstr, const struct acpi_device_id **matchid);
 	int (*attach)(struct acpi_device *dev, const struct acpi_device_id *id);
 	void (*detach)(struct acpi_device *dev);
+	void (*post_eject)(struct acpi_device *dev);
 	void (*bind)(struct device *phys_dev);
 	void (*unbind)(struct device *phys_dev);
 	struct acpi_hotplug_profile hotplug;
-- 
2.39.2

[PATCH v8 09/16] arm64: acpi: Move get_cpu_for_acpi_id() to a header

[Jonathan Cameron]

From: James Morse <james.morse@arm.com>

ACPI identifies CPUs by UID. get_cpu_for_acpi_id() maps the ACPI UID
to the Linux CPU number.

The helper to retrieve this mapping is only available in arm64's NUMA
code.

Move it to live next to get_acpi_id_for_cpu().

Signed-off-by: James Morse <james.morse@arm.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Gavin Shan <gshan@redhat.com>
Tested-by: Miguel Luis <miguel.luis@oracle.com>
Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
Tested-by: Jianyong Wu <jianyong.wu@arm.com>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Acked-by: Hanjun Guo <guohanjun@huawei.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

---
v8: Picked up tags.
---
 arch/arm64/include/asm/acpi.h | 11 +++++++++++
 arch/arm64/kernel/acpi_numa.c | 11 -----------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index 6792a1f83f2a..bc9a6656fc0c 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -119,6 +119,17 @@ static inline u32 get_acpi_id_for_cpu(unsigned int cpu)
 	return	acpi_cpu_get_madt_gicc(cpu)->uid;
 }
 
+static inline int get_cpu_for_acpi_id(u32 uid)
+{
+	int cpu;
+
+	for (cpu = 0; cpu < nr_cpu_ids; cpu++)
+		if (uid == get_acpi_id_for_cpu(cpu))
+			return cpu;
+
+	return -EINVAL;
+}
+
 static inline void arch_fix_phys_package_id(int num, u32 slot) { }
 void __init acpi_init_cpus(void);
 int apei_claim_sea(struct pt_regs *regs);
diff --git a/arch/arm64/kernel/acpi_numa.c b/arch/arm64/kernel/acpi_numa.c
index e51535a5f939..0c036a9a3c33 100644
--- a/arch/arm64/kernel/acpi_numa.c
+++ b/arch/arm64/kernel/acpi_numa.c
@@ -34,17 +34,6 @@ int __init acpi_numa_get_nid(unsigned int cpu)
 	return acpi_early_node_map[cpu];
 }
 
-static inline int get_cpu_for_acpi_id(u32 uid)
-{
-	int cpu;
-
-	for (cpu = 0; cpu < nr_cpu_ids; cpu++)
-		if (uid == get_acpi_id_for_cpu(cpu))
-			return cpu;
-
-	return -EINVAL;
-}
-
 static int __init acpi_parse_gicc_pxm(union acpi_subtable_headers *header,
 				      const unsigned long end)
 {
-- 
2.39.2

[PATCH v8 10/16] irqchip/gic-v3: Don't return errors from gic_acpi_match_gicc()

[Jonathan Cameron]

From: James Morse <james.morse@arm.com>

gic_acpi_match_gicc() is only called via gic_acpi_count_gicr_regions().
It should only count the number of enabled redistributors, but it
also tries to sanity check the GICC entry, currently returning an
error if the Enabled bit is set, but the gicr_base_address is zero.

Adding support for the online-capable bit to the sanity check will
complicate it, for no benefit. The existing check implicitly depends on
gic_acpi_count_gicr_regions() previous failing to find any GICR regions
(as it is valid to have gicr_base_address of zero if the redistributors
are described via a GICR entry).

Instead of complicating the check, remove it. Failures that happen at
this point cause the irqchip not to register, meaning no irqs can be
requested. The kernel grinds to a panic() pretty quickly.

Without the check, MADT tables that exhibit this problem are still
caught by gic_populate_rdist(), which helpfully also prints what went
wrong:
| CPU4: mpidr 100 has no re-distributor!

Signed-off-by: James Morse <james.morse@arm.com>
Reviewed-by: Gavin Shan <gshan@redhat.com>
Tested-by: Miguel Luis <miguel.luis@oracle.com>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

---
V8: No change
---
 drivers/irqchip/irq-gic-v3.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 6fb276504bcc..10af15f93d4d 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -2415,19 +2415,10 @@ static int __init gic_acpi_match_gicc(union acpi_subtable_headers *header,
 	 * If GICC is enabled and has valid gicr base address, then it means
 	 * GICR base is presented via GICC
 	 */
-	if (acpi_gicc_is_usable(gicc) && gicc->gicr_base_address) {
+	if (acpi_gicc_is_usable(gicc) && gicc->gicr_base_address)
 		acpi_data.enabled_rdists++;
-		return 0;
-	}
 
-	/*
-	 * It's perfectly valid firmware can pass disabled GICC entry, driver
-	 * should not treat as errors, skip the entry instead of probe fail.
-	 */
-	if (!acpi_gicc_is_usable(gicc))
-		return 0;
-
-	return -ENODEV;
+	return 0;
 }
 
 static int __init gic_acpi_count_gicr_regions(void)
-- 
2.39.2

[PATCH v8 11/16] irqchip/gic-v3: Add support for ACPI's disabled but 'online capable' CPUs

[Jonathan Cameron]

From: James Morse <james.morse@arm.com>

To support virtual CPU hotplug, ACPI has added an 'online capable' bit
to the MADT GICC entries. This indicates a disabled CPU entry may not
be possible to online via PSCI until firmware has set enabled bit in
_STA.

This means that a "usable" GIC is one that is marked as either enabled,
or online capable. Therefore, change acpi_gicc_is_usable() to check both
bits. However, we need to change the test in gic_acpi_match_gicc() back
to testing just the enabled bit so the count of enabled distributors is
correct.

What about the redistributor in the GICC entry? ACPI doesn't want to say.
Assume the worst: When a redistributor is described in the GICC entry,
but the entry is marked as disabled at boot, assume the redistributor
is inaccessible.

The GICv3 driver doesn't support late online of redistributors, so this
means the corresponding CPU can't be brought online either.
Rather than modifying cpu masks that may already have been used,
register a new cpuhp callback to fail this case. This must run earlier
than the main gic_starting_cpu() so that this case can be rejected
before the section of cpuhp that runs on the CPU that is coming up as
that is not allowed to fail. This solution keeps the handling of this
broken firmware corner case local to the GIC driver. As precise ordering
of this callback doesn't need to be controlled as long as it is
in that initial prepare phase, use CPUHP_BP_PREPARE_DYN.

Systems that want CPU hotplug in a VM can ensure their redistributors
are always-on, and describe them that way with a GICR entry in the MADT.

Suggested-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: James Morse <james.morse@arm.com>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Tested-by: Miguel Luis <miguel.luis@oracle.com>
Co-developed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

---
Thanks to Marc for review and suggestions!
v8: Change the handling of broken rdists to fail cpuhp rather than
    modifying the cpu_present and cpu_possible masks.
    Updated commit text to reflect that.
    Added a sb tag for Marc given this is more or less what he put
    in his review comment.
---
 drivers/irqchip/irq-gic-v3.c | 38 ++++++++++++++++++++++++++++++++++--
 include/linux/acpi.h         |  3 ++-
 2 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 10af15f93d4d..b4685991953e 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -44,6 +44,8 @@
 
 #define GIC_IRQ_TYPE_PARTITION	(GIC_IRQ_TYPE_LPI + 1)
 
+static struct cpumask broken_rdists __read_mostly;
+
 struct redist_region {
 	void __iomem		*redist_base;
 	phys_addr_t		phys_base;
@@ -1293,6 +1295,18 @@ static void gic_cpu_init(void)
 #define MPIDR_TO_SGI_RS(mpidr)	(MPIDR_RS(mpidr) << ICC_SGI1R_RS_SHIFT)
 #define MPIDR_TO_SGI_CLUSTER_ID(mpidr)	((mpidr) & ~0xFUL)
 
+/*
+ * gic_starting_cpu() is called after the last point where cpuhp is allowed
+ * to fail. So pre check for problems earlier.
+ */
+static int gic_check_rdist(unsigned int cpu)
+{
+	if (cpumask_test_cpu(cpu, &broken_rdists))
+		return -EINVAL;
+
+	return 0;
+}
+
 static int gic_starting_cpu(unsigned int cpu)
 {
 	gic_cpu_init();
@@ -1384,6 +1398,10 @@ static void __init gic_smp_init(void)
 	};
 	int base_sgi;
 
+	cpuhp_setup_state_nocalls(CPUHP_BP_PREPARE_DYN,
+				  "irqchip/arm/gicv3:checkrdist",
+				  gic_check_rdist, NULL);
+
 	cpuhp_setup_state_nocalls(CPUHP_AP_IRQ_GIC_STARTING,
 				  "irqchip/arm/gicv3:starting",
 				  gic_starting_cpu, NULL);
@@ -2363,11 +2381,24 @@ gic_acpi_parse_madt_gicc(union acpi_subtable_headers *header,
 				(struct acpi_madt_generic_interrupt *)header;
 	u32 reg = readl_relaxed(acpi_data.dist_base + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
 	u32 size = reg == GIC_PIDR2_ARCH_GICv4 ? SZ_64K * 4 : SZ_64K * 2;
+	int cpu = get_cpu_for_acpi_id(gicc->uid);
 	void __iomem *redist_base;
 
 	if (!acpi_gicc_is_usable(gicc))
 		return 0;
 
+	/*
+	 * Capable but disabled CPUs can be brought online later. What about
+	 * the redistributor? ACPI doesn't want to say!
+	 * Virtual hotplug systems can use the MADT's "always-on" GICR entries.
+	 * Otherwise, prevent such CPUs from being brought online.
+	 */
+	if (!(gicc->flags & ACPI_MADT_ENABLED)) {
+		pr_warn_once("CPU %u's redistributor is inaccessible: this CPU can't be brought online\n", cpu);
+		cpumask_set_cpu(cpu, &broken_rdists);
+		return 0;
+	}
+
 	redist_base = ioremap(gicc->gicr_base_address, size);
 	if (!redist_base)
 		return -ENOMEM;
@@ -2413,9 +2444,12 @@ static int __init gic_acpi_match_gicc(union acpi_subtable_headers *header,
 
 	/*
 	 * If GICC is enabled and has valid gicr base address, then it means
-	 * GICR base is presented via GICC
+	 * GICR base is presented via GICC. The redistributor is only known to
+	 * be accessible if the GICC is marked as enabled. If this bit is not
+	 * set, we'd need to add the redistributor at runtime, which isn't
+	 * supported.
 	 */
-	if (acpi_gicc_is_usable(gicc) && gicc->gicr_base_address)
+	if (gicc->flags & ACPI_MADT_ENABLED && gicc->gicr_base_address)
 		acpi_data.enabled_rdists++;
 
 	return 0;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 9844a3f9c4e5..fcfb7bb6789e 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -239,7 +239,8 @@ void acpi_table_print_madt_entry (struct acpi_subtable_header *madt);
 
 static inline bool acpi_gicc_is_usable(struct acpi_madt_generic_interrupt *gicc)
 {
-	return gicc->flags & ACPI_MADT_ENABLED;
+	return gicc->flags & (ACPI_MADT_ENABLED |
+			      ACPI_MADT_GICC_ONLINE_CAPABLE);
 }
 
 /* the following numa functions are architecture-dependent */
-- 
2.39.2

[PATCH v8 12/16] arm64: psci: Ignore DENIED CPUs

[Jonathan Cameron]

From: Jean-Philippe Brucker <jean-philippe@linaro.org>

When a CPU is marked as disabled, but online capable in the MADT, PSCI
applies some firmware policy to control when it can be brought online.
PSCI returns DENIED to a CPU_ON request if this is not currently
permitted. The OS can learn the current policy from the _STA enabled bit.

Handle the PSCI DENIED return code gracefully instead of printing an
error.

Note the alternatives to the PSCI cpu_boot() callback do not
return -EPERM so the change in smp.c has no affect.

See https://developer.arm.com/documentation/den0022/f/?lang=en page 58.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
[ morse: Rewrote commit message ]
Signed-off-by: James Morse <james.morse@arm.com>
Tested-by: Miguel Luis <miguel.luis@oracle.com>
Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
Tested-by: Jianyong Wu <jianyong.wu@arm.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
v8: Note in commit message that the -EPERM guard on the error print
    only affects PSCI as other options never use this error code.
    Should they do so in future, that may well indicate that they
    now support similar refusal to boot.
---
 arch/arm64/kernel/psci.c | 2 +-
 arch/arm64/kernel/smp.c  | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c
index 29a8e444db83..fabd732d0a2d 100644
--- a/arch/arm64/kernel/psci.c
+++ b/arch/arm64/kernel/psci.c
@@ -40,7 +40,7 @@ static int cpu_psci_cpu_boot(unsigned int cpu)
 {
 	phys_addr_t pa_secondary_entry = __pa_symbol(secondary_entry);
 	int err = psci_ops.cpu_on(cpu_logical_map(cpu), pa_secondary_entry);
-	if (err)
+	if (err && err != -EPERM)
 		pr_err("failed to boot CPU%d (%d)\n", cpu, err);
 
 	return err;
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 4ced34f62dab..dc0e0b3ec2d4 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -132,7 +132,8 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)
 	/* Now bring the CPU into our world */
 	ret = boot_secondary(cpu, idle);
 	if (ret) {
-		pr_err("CPU%u: failed to boot: %d\n", cpu, ret);
+		if (ret != -EPERM)
+			pr_err("CPU%u: failed to boot: %d\n", cpu, ret);
 		return ret;
 	}
 
-- 
2.39.2

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

[Jonathan Cameron]

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@kernel.org>
Tested-by: Miguel Luis <miguel.luis@oracle.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
V8: No change. (tags collected only)
---
 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);
+
+	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;
+	}
+
+	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

[PATCH v8 14/16] arm64: Kconfig: Enable hotplug CPU on arm64 if ACPI_PROCESSOR is enabled.

[Jonathan Cameron]

In order to move arch_register_cpu() to be called via the same path
for initially present CPUs described by ACPI and hotplugged CPUs
ACPI_HOTPLUG_CPU needs to be enabled.

The protection against invalid IDs in acpi_map_cpu() is needed as
at least one production BIOS is in the wild which reports entries
in DSDT (with no _STA method, so assumed enabled and present)
that don't match MADT.

Tested-by: Miguel Luis <miguel.luis@oracle.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

---
v8: If acpi_map_cpu() stub is passed an error code as pcpu, don't
    pretend it worked.
    Note that x86 relies on passing an invalid error code in here
    to indicate a CPU ID needs to be allocated, on ARM64 if we
    don't get something valid there is nothing we can do about it.
---
 arch/arm64/Kconfig       |  1 +
 arch/arm64/kernel/acpi.c | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 7b11c98b3e84..fed7d0d54179 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -5,6 +5,7 @@ config ARM64
 	select ACPI_CCA_REQUIRED if ACPI
 	select ACPI_GENERIC_GSI if ACPI
 	select ACPI_GTDT if ACPI
+	select ACPI_HOTPLUG_CPU if ACPI_PROCESSOR
 	select ACPI_IORT if ACPI
 	select ACPI_REDUCED_HARDWARE_ONLY if ACPI
 	select ACPI_MCFG if (ACPI && PCI)
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index dba8fcec7f33..fb9368197c74 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -29,6 +29,7 @@
 #include <linux/pgtable.h>
 
 #include <acpi/ghes.h>
+#include <acpi/processor.h>
 #include <asm/cputype.h>
 #include <asm/cpu_ops.h>
 #include <asm/daifflags.h>
@@ -413,6 +414,27 @@ void arch_reserve_mem_area(acpi_physical_address addr, size_t size)
 	memblock_mark_nomap(addr, size);
 }
 
+#ifdef CONFIG_ACPI_HOTPLUG_CPU
+int acpi_map_cpu(acpi_handle handle, phys_cpuid_t physid, u32 apci_id,
+		 int *pcpu)
+{
+	/* If an error code is passed in this stub can't fix it */
+	if (*pcpu < 0) {
+		pr_warn_once("Unable to map CPU to valid ID\n");
+		return *pcpu;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(acpi_map_cpu);
+
+int acpi_unmap_cpu(int cpu)
+{
+	return 0;
+}
+EXPORT_SYMBOL(acpi_unmap_cpu);
+#endif /* CONFIG_ACPI_HOTPLUG_CPU */
+
 #ifdef CONFIG_ACPI_FFH
 /*
  * Implements ARM64 specific callbacks to support ACPI FFH Operation Region as
-- 
2.39.2

[PATCH v8 15/16] arm64: document virtual CPU hotplug's expectations

[Jonathan Cameron]

From: James Morse <james.morse@arm.com>

Add a description of physical and virtual CPU hotplug, explain the
differences and elaborate on what is required in ACPI for a working
virtual hotplug system.

Signed-off-by: James Morse <james.morse@arm.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Tested-by: Miguel Luis <miguel.luis@oracle.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 Documentation/arch/arm64/cpu-hotplug.rst | 79 ++++++++++++++++++++++++
 Documentation/arch/arm64/index.rst       |  1 +
 2 files changed, 80 insertions(+)

diff --git a/Documentation/arch/arm64/cpu-hotplug.rst b/Documentation/arch/arm64/cpu-hotplug.rst
new file mode 100644
index 000000000000..76ba8d932c72
--- /dev/null
+++ b/Documentation/arch/arm64/cpu-hotplug.rst
@@ -0,0 +1,79 @@
+.. SPDX-License-Identifier: GPL-2.0
+.. _cpuhp_index:
+
+====================
+CPU Hotplug and ACPI
+====================
+
+CPU hotplug in the arm64 world is commonly used to describe the kernel taking
+CPUs online/offline using PSCI. This document is about ACPI firmware allowing
+CPUs that were not available during boot to be added to the system later.
+
+``possible`` and ``present`` refer to the state of the CPU as seen by linux.
+
+
+CPU Hotplug on physical systems - CPUs not present at boot
+----------------------------------------------------------
+
+Physical systems need to mark a CPU that is ``possible`` but not ``present`` as
+being ``present``. An example would be a dual socket machine, where the package
+in one of the sockets can be replaced while the system is running.
+
+This is not supported.
+
+In the arm64 world CPUs are not a single device but a slice of the system.
+There are no systems that support the physical addition (or removal) of CPUs
+while the system is running, and ACPI is not able to sufficiently describe
+them.
+
+e.g. New CPUs come with new caches, but the platform's cache toplogy is
+described in a static table, the PPTT. How caches are shared between CPUs is
+not discoverable, and must be described by firmware.
+
+e.g. The GIC redistributor for each CPU must be accessed by the driver during
+boot to discover the system wide supported features. ACPI's MADT GICC
+structures can describe a redistributor associated with a disabled CPU, but
+can't describe whether the redistributor is accessible, only that it is not
+'always on'.
+
+arm64's ACPI tables assume that everything described is ``present``.
+
+
+CPU Hotplug on virtual systems - CPUs not enabled at boot
+---------------------------------------------------------
+
+Virtual systems have the advantage that all the properties the system will
+ever have can be described at boot. There are no power-domain considerations
+as such devices are emulated.
+
+CPU Hotplug on virtual systems is supported. It is distinct from physical
+CPU Hotplug as all resources are described as ``present``, but CPUs may be
+marked as disabled by firmware. Only the CPU's online/offline behaviour is
+influenced by firmware. An example is where a virtual machine boots with a
+single CPU, and additional CPUs are added once a cloud orchestrator deploys
+the workload.
+
+For a virtual machine, the VMM (e.g. Qemu) plays the part of firmware.
+
+Virtual hotplug is implemented as a firmware policy affecting which CPUs can be
+brought online. Firmware can enforce its policy via PSCI's return codes. e.g.
+``DENIED``.
+
+The ACPI tables must describe all the resources of the virtual machine. CPUs
+that firmware wishes to disable either from boot (or later) should not be
+``enabled`` in the MADT GICC structures, but should have the ``online capable``
+bit set, to indicate they can be enabled later. The boot CPU must be marked as
+``enabled``.  The 'always on' GICR structure must be used to describe the
+redistributors.
+
+CPUs described as ``online capable`` but not ``enabled`` can be set to enabled
+by the DSDT's Processor object's _STA method. On virtual systems the _STA method
+must always report the CPU as ``present``. Changes to the firmware policy can
+be notified to the OS via device-check or eject-request.
+
+CPUs described as ``enabled`` in the static table, should not have their _STA
+modified dynamically by firmware. Soft-restart features such as kexec will
+re-read the static properties of the system from these static tables, and
+may malfunction if these no longer describe the running system. Linux will
+re-discover the dynamic properties of the system from the _STA method later
+during boot.
diff --git a/Documentation/arch/arm64/index.rst b/Documentation/arch/arm64/index.rst
index d08e924204bf..78544de0a8a9 100644
--- a/Documentation/arch/arm64/index.rst
+++ b/Documentation/arch/arm64/index.rst
@@ -13,6 +13,7 @@ ARM64 Architecture
     asymmetric-32bit
     booting
     cpu-feature-registers
+    cpu-hotplug
     elf_hwcaps
     hugetlbpage
     kdump
-- 
2.39.2

[PATCH v8 16/16] cpumask: Add enabled cpumask for present CPUs that can be brought online

[Jonathan Cameron]

From: James Morse <james.morse@arm.com>

The 'offline' file in sysfs shows all offline CPUs, including those
that aren't present. User-space is expected to remove not-present CPUs
from this list to learn which CPUs could be brought online.

CPUs can be present but not-enabled. These CPUs can't be brought online
until the firmware policy changes, which comes with an ACPI notification
that will register the CPUs.

With only the offline and present files, user-space is unable to
determine which CPUs it can try to bring online. Add a new CPU mask
that shows this based on all the registered CPUs.

Signed-off-by: James Morse <james.morse@arm.com>
Tested-by: Miguel Luis <miguel.luis@oracle.com>
Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
Tested-by: Jianyong Wu <jianyong.wu@arm.com>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 .../ABI/testing/sysfs-devices-system-cpu      |  6 +++++
 drivers/base/cpu.c                            | 10 ++++++++
 include/linux/cpumask.h                       | 25 +++++++++++++++++++
 kernel/cpu.c                                  |  3 +++
 4 files changed, 44 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu b/Documentation/ABI/testing/sysfs-devices-system-cpu
index 710d47be11e0..808efb5b860a 100644
--- a/Documentation/ABI/testing/sysfs-devices-system-cpu
+++ b/Documentation/ABI/testing/sysfs-devices-system-cpu
@@ -694,3 +694,9 @@ Description:
 		(RO) indicates whether or not the kernel directly supports
 		modifying the crash elfcorehdr for CPU hot un/plug and/or
 		on/offline changes.
+
+What:		/sys/devices/system/cpu/enabled
+Date:		Nov 2022
+Contact:	Linux kernel mailing list <linux-kernel@vger.kernel.org>
+Description:
+		(RO) the list of CPUs that can be brought online.
diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index 7b83e9c87d7c..353ee39a5cbe 100644
--- a/drivers/base/cpu.c
+++ b/drivers/base/cpu.c
@@ -95,6 +95,7 @@ void unregister_cpu(struct cpu *cpu)
 {
 	int logical_cpu = cpu->dev.id;
 
+	set_cpu_enabled(logical_cpu, false);
 	unregister_cpu_under_node(logical_cpu, cpu_to_node(logical_cpu));
 
 	device_unregister(&cpu->dev);
@@ -273,6 +274,13 @@ static ssize_t print_cpus_offline(struct device *dev,
 }
 static DEVICE_ATTR(offline, 0444, print_cpus_offline, NULL);
 
+static ssize_t print_cpus_enabled(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%*pbl\n", cpumask_pr_args(cpu_enabled_mask));
+}
+static DEVICE_ATTR(enabled, 0444, print_cpus_enabled, NULL);
+
 static ssize_t print_cpus_isolated(struct device *dev,
 				  struct device_attribute *attr, char *buf)
 {
@@ -413,6 +421,7 @@ int register_cpu(struct cpu *cpu, int num)
 	register_cpu_under_node(num, cpu_to_node(num));
 	dev_pm_qos_expose_latency_limit(&cpu->dev,
 					PM_QOS_RESUME_LATENCY_NO_CONSTRAINT);
+	set_cpu_enabled(num, true);
 
 	return 0;
 }
@@ -494,6 +503,7 @@ static struct attribute *cpu_root_attrs[] = {
 	&cpu_attrs[2].attr.attr,
 	&dev_attr_kernel_max.attr,
 	&dev_attr_offline.attr,
+	&dev_attr_enabled.attr,
 	&dev_attr_isolated.attr,
 #ifdef CONFIG_NO_HZ_FULL
 	&dev_attr_nohz_full.attr,
diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index 1c29947db848..4b202b94c97a 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -93,6 +93,7 @@ static inline void set_nr_cpu_ids(unsigned int nr)
  *
  *     cpu_possible_mask- has bit 'cpu' set iff cpu is populatable
  *     cpu_present_mask - has bit 'cpu' set iff cpu is populated
+ *     cpu_enabled_mask  - has bit 'cpu' set iff cpu can be brought online
  *     cpu_online_mask  - has bit 'cpu' set iff cpu available to scheduler
  *     cpu_active_mask  - has bit 'cpu' set iff cpu available to migration
  *
@@ -125,11 +126,13 @@ static inline void set_nr_cpu_ids(unsigned int nr)
 
 extern struct cpumask __cpu_possible_mask;
 extern struct cpumask __cpu_online_mask;
+extern struct cpumask __cpu_enabled_mask;
 extern struct cpumask __cpu_present_mask;
 extern struct cpumask __cpu_active_mask;
 extern struct cpumask __cpu_dying_mask;
 #define cpu_possible_mask ((const struct cpumask *)&__cpu_possible_mask)
 #define cpu_online_mask   ((const struct cpumask *)&__cpu_online_mask)
+#define cpu_enabled_mask   ((const struct cpumask *)&__cpu_enabled_mask)
 #define cpu_present_mask  ((const struct cpumask *)&__cpu_present_mask)
 #define cpu_active_mask   ((const struct cpumask *)&__cpu_active_mask)
 #define cpu_dying_mask    ((const struct cpumask *)&__cpu_dying_mask)
@@ -1009,6 +1012,7 @@ extern const DECLARE_BITMAP(cpu_all_bits, NR_CPUS);
 #else
 #define for_each_possible_cpu(cpu) for_each_cpu((cpu), cpu_possible_mask)
 #define for_each_online_cpu(cpu)   for_each_cpu((cpu), cpu_online_mask)
+#define for_each_enabled_cpu(cpu)   for_each_cpu((cpu), cpu_enabled_mask)
 #define for_each_present_cpu(cpu)  for_each_cpu((cpu), cpu_present_mask)
 #endif
 
@@ -1031,6 +1035,15 @@ set_cpu_possible(unsigned int cpu, bool possible)
 		cpumask_clear_cpu(cpu, &__cpu_possible_mask);
 }
 
+static inline void
+set_cpu_enabled(unsigned int cpu, bool can_be_onlined)
+{
+	if (can_be_onlined)
+		cpumask_set_cpu(cpu, &__cpu_enabled_mask);
+	else
+		cpumask_clear_cpu(cpu, &__cpu_enabled_mask);
+}
+
 static inline void
 set_cpu_present(unsigned int cpu, bool present)
 {
@@ -1112,6 +1125,7 @@ static __always_inline unsigned int num_online_cpus(void)
 	return raw_atomic_read(&__num_online_cpus);
 }
 #define num_possible_cpus()	cpumask_weight(cpu_possible_mask)
+#define num_enabled_cpus()	cpumask_weight(cpu_enabled_mask)
 #define num_present_cpus()	cpumask_weight(cpu_present_mask)
 #define num_active_cpus()	cpumask_weight(cpu_active_mask)
 
@@ -1120,6 +1134,11 @@ static inline bool cpu_online(unsigned int cpu)
 	return cpumask_test_cpu(cpu, cpu_online_mask);
 }
 
+static inline bool cpu_enabled(unsigned int cpu)
+{
+	return cpumask_test_cpu(cpu, cpu_enabled_mask);
+}
+
 static inline bool cpu_possible(unsigned int cpu)
 {
 	return cpumask_test_cpu(cpu, cpu_possible_mask);
@@ -1144,6 +1163,7 @@ static inline bool cpu_dying(unsigned int cpu)
 
 #define num_online_cpus()	1U
 #define num_possible_cpus()	1U
+#define num_enabled_cpus()	1U
 #define num_present_cpus()	1U
 #define num_active_cpus()	1U
 
@@ -1157,6 +1177,11 @@ static inline bool cpu_possible(unsigned int cpu)
 	return cpu == 0;
 }
 
+static inline bool cpu_enabled(unsigned int cpu)
+{
+	return cpu == 0;
+}
+
 static inline bool cpu_present(unsigned int cpu)
 {
 	return cpu == 0;
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 8f6affd051f7..537099bf5d02 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -3117,6 +3117,9 @@ EXPORT_SYMBOL(__cpu_possible_mask);
 struct cpumask __cpu_online_mask __read_mostly;
 EXPORT_SYMBOL(__cpu_online_mask);
 
+struct cpumask __cpu_enabled_mask __read_mostly;
+EXPORT_SYMBOL(__cpu_enabled_mask);
+
 struct cpumask __cpu_present_mask __read_mostly;
 EXPORT_SYMBOL(__cpu_present_mask);
 
-- 
2.39.2

Re: [PATCH v8 10/16] irqchip/gic-v3: Don't return errors from gic_acpi_match_gicc()

[Marc Zyngier]

On Fri, 26 Apr 2024 14:51:20 +0100,
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> 
> From: James Morse <james.morse@arm.com>
> 
> gic_acpi_match_gicc() is only called via gic_acpi_count_gicr_regions().
> It should only count the number of enabled redistributors, but it
> also tries to sanity check the GICC entry, currently returning an
> error if the Enabled bit is set, but the gicr_base_address is zero.
> 
> Adding support for the online-capable bit to the sanity check will
> complicate it, for no benefit. The existing check implicitly depends on
> gic_acpi_count_gicr_regions() previous failing to find any GICR regions
> (as it is valid to have gicr_base_address of zero if the redistributors
> are described via a GICR entry).
> 
> Instead of complicating the check, remove it. Failures that happen at
> this point cause the irqchip not to register, meaning no irqs can be
> requested. The kernel grinds to a panic() pretty quickly.
>
> Without the check, MADT tables that exhibit this problem are still
> caught by gic_populate_rdist(), which helpfully also prints what went
> wrong:
> | CPU4: mpidr 100 has no re-distributor!
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> Reviewed-by: Gavin Shan <gshan@redhat.com>
> Tested-by: Miguel Luis <miguel.luis@oracle.com>
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Reviewed-by: Marc Zyngier <maz@kernel.org>

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v8 01/16] ACPI: processor: Simplify initial onlining to use same path for cold and hotplug

[Miguel Luis]

> On 26 Apr 2024, at 13:51, Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> 
> Separate code paths, combined with a flag set in acpi_processor.c to
> indicate a struct acpi_processor was for a hotplugged CPU ensured that
> per CPU data was only set up the first time that a CPU was initialized.
> This appears to be unnecessary as the paths can be combined by letting
> the online logic also handle any CPUs online at the time of driver load.
> 
> Motivation for this change, beyond simplification, is that ARM64
> virtual CPU HP uses the same code paths for hotplug and cold path in
> acpi_processor.c so had no easy way to set the flag for hotplug only.
> Removing this necessity will enable ARM64 vCPU HP to reuse the existing
> code paths.
> 
> Leave noisy pr_info() in place but update it to not state the CPU
> was hotplugged.
> 
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Reviewed-by: Hanjun Guo <guohanjun@huawei.com>
> Tested-by: Miguel Luis <miguel.luis@oracle.com>
> Reviewed-by: Gavin Shan <gshan@redhat.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> ---
> v8: No change
> ---
> drivers/acpi/acpi_processor.c   |  1 -
> drivers/acpi/processor_driver.c | 44 ++++++++++-----------------------
> include/acpi/processor.h        |  2 +-
> 3 files changed, 14 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> index 7a0dd35d62c9..7fc924aeeed0 100644
> --- a/drivers/acpi/acpi_processor.c
> +++ b/drivers/acpi/acpi_processor.c
> @@ -216,7 +216,6 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
> * gets online for the first time.
> */
> pr_info("CPU%d has been hot-added\n", pr->id);
> - pr->flags.need_hotplug_init = 1;
> 
> out:
> cpus_write_unlock();
> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> index 67db60eda370..55782eac3ff1 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -33,7 +33,6 @@ MODULE_AUTHOR("Paul Diefenbaugh");
> MODULE_DESCRIPTION("ACPI Processor Driver");
> MODULE_LICENSE("GPL");
> 
> -static int acpi_processor_start(struct device *dev);
> static int acpi_processor_stop(struct device *dev);
> 
> static const struct acpi_device_id processor_device_ids[] = {
> @@ -47,7 +46,6 @@ static struct device_driver acpi_processor_driver = {
> .name = "processor",
> .bus = &cpu_subsys,
> .acpi_match_table = processor_device_ids,
> - .probe = acpi_processor_start,
> .remove = acpi_processor_stop,
> };
> 
> @@ -115,12 +113,10 @@ static int acpi_soft_cpu_online(unsigned int cpu)
> * CPU got physically hotplugged and onlined for the first time:
> * Initialize missing things.
> */
> - if (pr->flags.need_hotplug_init) {
> + if (!pr->flags.previously_online) {
> int ret;
> 
> - pr_info("Will online and init hotplugged CPU: %d\n",
> - pr->id);
> - pr->flags.need_hotplug_init = 0;
> + pr_info("Will online and init CPU: %d\n", pr->id);
> ret = __acpi_processor_start(device);
> WARN(ret, "Failed to start CPU: %d\n", pr->id);
> } else {
> @@ -167,9 +163,6 @@ static int __acpi_processor_start(struct acpi_device *device)
> if (!pr)
> return -ENODEV;
> 
> - if (pr->flags.need_hotplug_init)
> - return 0;
> -
> result = acpi_cppc_processor_probe(pr);
> if (result && !IS_ENABLED(CONFIG_ACPI_CPU_FREQ_PSS))
> dev_dbg(&device->dev, "CPPC data invalid or not present\n");
> @@ -185,32 +178,21 @@ static int __acpi_processor_start(struct acpi_device *device)
> 
> status = acpi_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
>     acpi_processor_notify, device);
> - if (ACPI_SUCCESS(status))
> - return 0;
> + if (!ACPI_SUCCESS(status)) {
> + result = -ENODEV;
> + goto err_thermal_exit;
> + }
> + pr->flags.previously_online = 1;
> 
> - result = -ENODEV;
> - acpi_processor_thermal_exit(pr, device);
> + return 0;
> 
> +err_thermal_exit:
> + acpi_processor_thermal_exit(pr, device);
> err_power_exit:
> acpi_processor_power_exit(pr);
> return result;
> }
> 
> -static int acpi_processor_start(struct device *dev)
> -{
> - struct acpi_device *device = ACPI_COMPANION(dev);
> - int ret;
> -
> - if (!device)
> - return -ENODEV;
> -
> - /* Protect against concurrent CPU hotplug operations */
> - cpu_hotplug_disable();
> - ret = __acpi_processor_start(device);
> - cpu_hotplug_enable();
> - return ret;
> -}
> -
> static int acpi_processor_stop(struct device *dev)
> {
> struct acpi_device *device = ACPI_COMPANION(dev);
> @@ -279,9 +261,9 @@ static int __init acpi_processor_driver_init(void)
> if (result < 0)
> return result;
> 
> - result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
> -   "acpi/cpu-drv:online",
> -   acpi_soft_cpu_online, NULL);
> + result = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> +   "acpi/cpu-drv:online",
> +   acpi_soft_cpu_online, NULL);
> if (result < 0)
> goto err;
> hp_online = result;
> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> index 3f34ebb27525..e6f6074eadbf 100644
> --- a/include/acpi/processor.h
> +++ b/include/acpi/processor.h
> @@ -217,7 +217,7 @@ struct acpi_processor_flags {
> u8 has_lpi:1;
> u8 power_setup_done:1;
> u8 bm_rld_set:1;
> - u8 need_hotplug_init:1;
> + u8 previously_online:1;

Reviewed-by: Miguel Luis <miguel.luis@oracle.com>

Miguel

> };
> 
> struct acpi_processor {
> -- 
> 2.39.2
> 

Re: [PATCH v8 11/16] irqchip/gic-v3: Add support for ACPI's disabled but 'online capable' CPUs

[Marc Zyngier]

On Fri, 26 Apr 2024 14:51:21 +0100,
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> 
> From: James Morse <james.morse@arm.com>
> 
> To support virtual CPU hotplug, ACPI has added an 'online capable' bit
> to the MADT GICC entries. This indicates a disabled CPU entry may not
> be possible to online via PSCI until firmware has set enabled bit in
> _STA.
> 
> This means that a "usable" GIC is one that is marked as either enabled,

nit: "GIC" usually designs the whole HW infrastructure (distributor,
redistributors, and ITSs). My understanding is that you are only
referring to the redistributors.

> or online capable. Therefore, change acpi_gicc_is_usable() to check both
> bits. However, we need to change the test in gic_acpi_match_gicc() back
> to testing just the enabled bit so the count of enabled distributors is
> correct.
> 
> What about the redistributor in the GICC entry? ACPI doesn't want to say.
> Assume the worst: When a redistributor is described in the GICC entry,
> but the entry is marked as disabled at boot, assume the redistributor
> is inaccessible.
> 
> The GICv3 driver doesn't support late online of redistributors, so this
> means the corresponding CPU can't be brought online either.
> Rather than modifying cpu masks that may already have been used,
> register a new cpuhp callback to fail this case. This must run earlier
> than the main gic_starting_cpu() so that this case can be rejected
> before the section of cpuhp that runs on the CPU that is coming up as
> that is not allowed to fail. This solution keeps the handling of this
> broken firmware corner case local to the GIC driver. As precise ordering
> of this callback doesn't need to be controlled as long as it is
> in that initial prepare phase, use CPUHP_BP_PREPARE_DYN.
> 
> Systems that want CPU hotplug in a VM can ensure their redistributors
> are always-on, and describe them that way with a GICR entry in the MADT.
> 
> Suggested-by: Marc Zyngier <maz@kernel.org>
> Signed-off-by: James Morse <james.morse@arm.com>
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> Tested-by: Miguel Luis <miguel.luis@oracle.com>
> Co-developed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> ---
> Thanks to Marc for review and suggestions!
> v8: Change the handling of broken rdists to fail cpuhp rather than
>     modifying the cpu_present and cpu_possible masks.
>     Updated commit text to reflect that.
>     Added a sb tag for Marc given this is more or less what he put
>     in his review comment.
> ---
>  drivers/irqchip/irq-gic-v3.c | 38 ++++++++++++++++++++++++++++++++++--
>  include/linux/acpi.h         |  3 ++-
>  2 files changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 10af15f93d4d..b4685991953e 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -44,6 +44,8 @@
>  
>  #define GIC_IRQ_TYPE_PARTITION	(GIC_IRQ_TYPE_LPI + 1)
>  
> +static struct cpumask broken_rdists __read_mostly;
> +
>  struct redist_region {
>  	void __iomem		*redist_base;
>  	phys_addr_t		phys_base;
> @@ -1293,6 +1295,18 @@ static void gic_cpu_init(void)
>  #define MPIDR_TO_SGI_RS(mpidr)	(MPIDR_RS(mpidr) << ICC_SGI1R_RS_SHIFT)
>  #define MPIDR_TO_SGI_CLUSTER_ID(mpidr)	((mpidr) & ~0xFUL)
>  
> +/*
> + * gic_starting_cpu() is called after the last point where cpuhp is allowed
> + * to fail. So pre check for problems earlier.
> + */
> +static int gic_check_rdist(unsigned int cpu)
> +{
> +	if (cpumask_test_cpu(cpu, &broken_rdists))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  static int gic_starting_cpu(unsigned int cpu)
>  {
>  	gic_cpu_init();
> @@ -1384,6 +1398,10 @@ static void __init gic_smp_init(void)
>  	};
>  	int base_sgi;
>  
> +	cpuhp_setup_state_nocalls(CPUHP_BP_PREPARE_DYN,
> +				  "irqchip/arm/gicv3:checkrdist",
> +				  gic_check_rdist, NULL);
> +
>  	cpuhp_setup_state_nocalls(CPUHP_AP_IRQ_GIC_STARTING,
>  				  "irqchip/arm/gicv3:starting",
>  				  gic_starting_cpu, NULL);
> @@ -2363,11 +2381,24 @@ gic_acpi_parse_madt_gicc(union acpi_subtable_headers *header,
>  				(struct acpi_madt_generic_interrupt *)header;
>  	u32 reg = readl_relaxed(acpi_data.dist_base + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
>  	u32 size = reg == GIC_PIDR2_ARCH_GICv4 ? SZ_64K * 4 : SZ_64K * 2;
> +	int cpu = get_cpu_for_acpi_id(gicc->uid);
>  	void __iomem *redist_base;
>  
>  	if (!acpi_gicc_is_usable(gicc))
>  		return 0;
>  
> +	/*
> +	 * Capable but disabled CPUs can be brought online later. What about
> +	 * the redistributor? ACPI doesn't want to say!
> +	 * Virtual hotplug systems can use the MADT's "always-on" GICR entries.
> +	 * Otherwise, prevent such CPUs from being brought online.
> +	 */
> +	if (!(gicc->flags & ACPI_MADT_ENABLED)) {

Now this makes the above acpi_gicc_is_usable() very odd. It checks for
MADT_ENABLED *or* GICC_ONLINE_CAPABLE. But we definitely don't want to
deal with the lack of MADT_ENABLED.

So why don't we explicitly check for individual flags and get rid of
acpi_gicc_is_usable(), as its new definition doesn't tell you anything
useful?

> +		pr_warn_once("CPU %u's redistributor is inaccessible: this CPU can't be brought online\n", cpu);
> +		cpumask_set_cpu(cpu, &broken_rdists);

Given that get_cpu_for_acpi_id() can return -EINVAL, you'd want to
check that. Also, I'd like to drop the _once on the warning.
Indicating all the broken CPUs is useful information, and only happens
once per boot.

> +		return 0;
> +	}
> +
>  	redist_base = ioremap(gicc->gicr_base_address, size);
>  	if (!redist_base)
>  		return -ENOMEM;
> @@ -2413,9 +2444,12 @@ static int __init gic_acpi_match_gicc(union acpi_subtable_headers *header,
>  
>  	/*
>  	 * If GICC is enabled and has valid gicr base address, then it means
> -	 * GICR base is presented via GICC
> +	 * GICR base is presented via GICC. The redistributor is only known to
> +	 * be accessible if the GICC is marked as enabled. If this bit is not
> +	 * set, we'd need to add the redistributor at runtime, which isn't
> +	 * supported.
>  	 */
> -	if (acpi_gicc_is_usable(gicc) && gicc->gicr_base_address)
> +	if (gicc->flags & ACPI_MADT_ENABLED && gicc->gicr_base_address)
>  		acpi_data.enabled_rdists++;
>  
>  	return 0;
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 9844a3f9c4e5..fcfb7bb6789e 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -239,7 +239,8 @@ void acpi_table_print_madt_entry (struct acpi_subtable_header *madt);
>  
>  static inline bool acpi_gicc_is_usable(struct acpi_madt_generic_interrupt *gicc)
>  {
> -	return gicc->flags & ACPI_MADT_ENABLED;
> +	return gicc->flags & (ACPI_MADT_ENABLED |
> +			      ACPI_MADT_GICC_ONLINE_CAPABLE);
>  }
>  
>  /* the following numa functions are architecture-dependent */

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v8 01/16] ACPI: processor: Simplify initial onlining to use same path for cold and hotplug

[Miguel Luis]

Hi Jonathan, 

> On 26 Apr 2024, at 16:05, Miguel Luis <miguel.luis@oracle.com> wrote:
> 
> 
> 
>> On 26 Apr 2024, at 13:51, Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
>> 
>> Separate code paths, combined with a flag set in acpi_processor.c to
>> indicate a struct acpi_processor was for a hotplugged CPU ensured that
>> per CPU data was only set up the first time that a CPU was initialized.
>> This appears to be unnecessary as the paths can be combined by letting
>> the online logic also handle any CPUs online at the time of driver load.
>> 
>> Motivation for this change, beyond simplification, is that ARM64
>> virtual CPU HP uses the same code paths for hotplug and cold path in
>> acpi_processor.c so had no easy way to set the flag for hotplug only.
>> Removing this necessity will enable ARM64 vCPU HP to reuse the existing
>> code paths.
>> 
>> Leave noisy pr_info() in place but update it to not state the CPU
>> was hotplugged.

On a second thought, do we want to keep it? Can't we just assume that no 
news is good news while keeping the warn right after __acpi_processor_start ?

Miguel

>> 
>> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Reviewed-by: Hanjun Guo <guohanjun@huawei.com>
>> Tested-by: Miguel Luis <miguel.luis@oracle.com>
>> Reviewed-by: Gavin Shan <gshan@redhat.com>
>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> 
>> ---
>> v8: No change
>> ---
>> drivers/acpi/acpi_processor.c   |  1 -
>> drivers/acpi/processor_driver.c | 44 ++++++++++-----------------------
>> include/acpi/processor.h        |  2 +-
>> 3 files changed, 14 insertions(+), 33 deletions(-)
>> 
>> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
>> index 7a0dd35d62c9..7fc924aeeed0 100644
>> --- a/drivers/acpi/acpi_processor.c
>> +++ b/drivers/acpi/acpi_processor.c
>> @@ -216,7 +216,6 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
>> * gets online for the first time.
>> */
>> pr_info("CPU%d has been hot-added\n", pr->id);
>> - pr->flags.need_hotplug_init = 1;
>> 
>> out:
>> cpus_write_unlock();
>> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
>> index 67db60eda370..55782eac3ff1 100644
>> --- a/drivers/acpi/processor_driver.c
>> +++ b/drivers/acpi/processor_driver.c
>> @@ -33,7 +33,6 @@ MODULE_AUTHOR("Paul Diefenbaugh");
>> MODULE_DESCRIPTION("ACPI Processor Driver");
>> MODULE_LICENSE("GPL");
>> 
>> -static int acpi_processor_start(struct device *dev);
>> static int acpi_processor_stop(struct device *dev);
>> 
>> static const struct acpi_device_id processor_device_ids[] = {
>> @@ -47,7 +46,6 @@ static struct device_driver acpi_processor_driver = {
>> .name = "processor",
>> .bus = &cpu_subsys,
>> .acpi_match_table = processor_device_ids,
>> - .probe = acpi_processor_start,
>> .remove = acpi_processor_stop,
>> };
>> 
>> @@ -115,12 +113,10 @@ static int acpi_soft_cpu_online(unsigned int cpu)
>> * CPU got physically hotplugged and onlined for the first time:
>> * Initialize missing things.
>> */
>> - if (pr->flags.need_hotplug_init) {
>> + if (!pr->flags.previously_online) {
>> int ret;
>> 
>> - pr_info("Will online and init hotplugged CPU: %d\n",
>> - pr->id);
>> - pr->flags.need_hotplug_init = 0;
>> + pr_info("Will online and init CPU: %d\n", pr->id);
>> ret = __acpi_processor_start(device);
>> WARN(ret, "Failed to start CPU: %d\n", pr->id);
>> } else {
>> @@ -167,9 +163,6 @@ static int __acpi_processor_start(struct acpi_device *device)
>> if (!pr)
>> return -ENODEV;
>> 
>> - if (pr->flags.need_hotplug_init)
>> - return 0;
>> -
>> result = acpi_cppc_processor_probe(pr);
>> if (result && !IS_ENABLED(CONFIG_ACPI_CPU_FREQ_PSS))
>> dev_dbg(&device->dev, "CPPC data invalid or not present\n");
>> @@ -185,32 +178,21 @@ static int __acpi_processor_start(struct acpi_device *device)
>> 
>> status = acpi_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
>>    acpi_processor_notify, device);
>> - if (ACPI_SUCCESS(status))
>> - return 0;
>> + if (!ACPI_SUCCESS(status)) {
>> + result = -ENODEV;
>> + goto err_thermal_exit;
>> + }
>> + pr->flags.previously_online = 1;
>> 
>> - result = -ENODEV;
>> - acpi_processor_thermal_exit(pr, device);
>> + return 0;
>> 
>> +err_thermal_exit:
>> + acpi_processor_thermal_exit(pr, device);
>> err_power_exit:
>> acpi_processor_power_exit(pr);
>> return result;
>> }
>> 
>> -static int acpi_processor_start(struct device *dev)
>> -{
>> - struct acpi_device *device = ACPI_COMPANION(dev);
>> - int ret;
>> -
>> - if (!device)
>> - return -ENODEV;
>> -
>> - /* Protect against concurrent CPU hotplug operations */
>> - cpu_hotplug_disable();
>> - ret = __acpi_processor_start(device);
>> - cpu_hotplug_enable();
>> - return ret;
>> -}
>> -
>> static int acpi_processor_stop(struct device *dev)
>> {
>> struct acpi_device *device = ACPI_COMPANION(dev);
>> @@ -279,9 +261,9 @@ static int __init acpi_processor_driver_init(void)
>> if (result < 0)
>> return result;
>> 
>> - result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
>> -   "acpi/cpu-drv:online",
>> -   acpi_soft_cpu_online, NULL);
>> + result = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
>> +   "acpi/cpu-drv:online",
>> +   acpi_soft_cpu_online, NULL);
>> if (result < 0)
>> goto err;
>> hp_online = result;
>> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
>> index 3f34ebb27525..e6f6074eadbf 100644
>> --- a/include/acpi/processor.h
>> +++ b/include/acpi/processor.h
>> @@ -217,7 +217,7 @@ struct acpi_processor_flags {
>> u8 has_lpi:1;
>> u8 power_setup_done:1;
>> u8 bm_rld_set:1;
>> - u8 need_hotplug_init:1;
>> + u8 previously_online:1;
> 
> Reviewed-by: Miguel Luis <miguel.luis@oracle.com>
> 
> Miguel
> 
>> };
>> 
>> struct acpi_processor {
>> -- 
>> 2.39.2
>> 
> 

Re: [PATCH v8 01/16] ACPI: processor: Simplify initial onlining to use same path for cold and hotplug

[Jonathan Cameron]

On Fri, 26 Apr 2024 17:21:41 +0000
Miguel Luis <miguel.luis@oracle.com> wrote:

> Hi Jonathan, 
> 
> > On 26 Apr 2024, at 16:05, Miguel Luis <miguel.luis@oracle.com> wrote:
> > 
> > 
> >   
> >> On 26 Apr 2024, at 13:51, Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> >> 
> >> Separate code paths, combined with a flag set in acpi_processor.c to
> >> indicate a struct acpi_processor was for a hotplugged CPU ensured that
> >> per CPU data was only set up the first time that a CPU was initialized.
> >> This appears to be unnecessary as the paths can be combined by letting
> >> the online logic also handle any CPUs online at the time of driver load.
> >> 
> >> Motivation for this change, beyond simplification, is that ARM64
> >> virtual CPU HP uses the same code paths for hotplug and cold path in
> >> acpi_processor.c so had no easy way to set the flag for hotplug only.
> >> Removing this necessity will enable ARM64 vCPU HP to reuse the existing
> >> code paths.
> >> 
> >> Leave noisy pr_info() in place but update it to not state the CPU
> >> was hotplugged.  
> 
> On a second thought, do we want to keep it? Can't we just assume that no 
> news is good news while keeping the warn right after __acpi_processor_start ?

Good question - my inclination was to keep this in place for now as removing
it would remove a source of information people may expect on x86 hotplug.

Then maybe propose dropping it as overly noisy kernel as a follow up
patch after this series is merged.  Felt like a potential rat hole I didn't
want to go down if I could avoid it.

If any x86 experts want to shout that no one cares then I'll happily drop
the print.  We've carefully made it so that on arm64 we have no way to tell
if this is hotplug or normal cpu bring up so we can't just print it on
hotplug.

Jonathan


> 
> Miguel
> 
> >> 
> >> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> Reviewed-by: Hanjun Guo <guohanjun@huawei.com>
> >> Tested-by: Miguel Luis <miguel.luis@oracle.com>
> >> Reviewed-by: Gavin Shan <gshan@redhat.com>
> >> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >> 
> >> ---
> >> v8: No change
> >> ---
> >> drivers/acpi/acpi_processor.c   |  1 -
> >> drivers/acpi/processor_driver.c | 44 ++++++++++-----------------------
> >> include/acpi/processor.h        |  2 +-
> >> 3 files changed, 14 insertions(+), 33 deletions(-)
> >> 
> >> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> >> index 7a0dd35d62c9..7fc924aeeed0 100644
> >> --- a/drivers/acpi/acpi_processor.c
> >> +++ b/drivers/acpi/acpi_processor.c
> >> @@ -216,7 +216,6 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
> >> * gets online for the first time.
> >> */
> >> pr_info("CPU%d has been hot-added\n", pr->id);
> >> - pr->flags.need_hotplug_init = 1;
> >> 
> >> out:
> >> cpus_write_unlock();
> >> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> >> index 67db60eda370..55782eac3ff1 100644
> >> --- a/drivers/acpi/processor_driver.c
> >> +++ b/drivers/acpi/processor_driver.c
> >> @@ -33,7 +33,6 @@ MODULE_AUTHOR("Paul Diefenbaugh");
> >> MODULE_DESCRIPTION("ACPI Processor Driver");
> >> MODULE_LICENSE("GPL");
> >> 
> >> -static int acpi_processor_start(struct device *dev);
> >> static int acpi_processor_stop(struct device *dev);
> >> 
> >> static const struct acpi_device_id processor_device_ids[] = {
> >> @@ -47,7 +46,6 @@ static struct device_driver acpi_processor_driver = {
> >> .name = "processor",
> >> .bus = &cpu_subsys,
> >> .acpi_match_table = processor_device_ids,
> >> - .probe = acpi_processor_start,
> >> .remove = acpi_processor_stop,
> >> };
> >> 
> >> @@ -115,12 +113,10 @@ static int acpi_soft_cpu_online(unsigned int cpu)
> >> * CPU got physically hotplugged and onlined for the first time:
> >> * Initialize missing things.
> >> */
> >> - if (pr->flags.need_hotplug_init) {
> >> + if (!pr->flags.previously_online) {
> >> int ret;
> >> 
> >> - pr_info("Will online and init hotplugged CPU: %d\n",
> >> - pr->id);
> >> - pr->flags.need_hotplug_init = 0;
> >> + pr_info("Will online and init CPU: %d\n", pr->id);
> >> ret = __acpi_processor_start(device);
> >> WARN(ret, "Failed to start CPU: %d\n", pr->id);
> >> } else {
> >> @@ -167,9 +163,6 @@ static int __acpi_processor_start(struct acpi_device *device)
> >> if (!pr)
> >> return -ENODEV;
> >> 
> >> - if (pr->flags.need_hotplug_init)
> >> - return 0;
> >> -
> >> result = acpi_cppc_processor_probe(pr);
> >> if (result && !IS_ENABLED(CONFIG_ACPI_CPU_FREQ_PSS))
> >> dev_dbg(&device->dev, "CPPC data invalid or not present\n");
> >> @@ -185,32 +178,21 @@ static int __acpi_processor_start(struct acpi_device *device)
> >> 
> >> status = acpi_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
> >>    acpi_processor_notify, device);
> >> - if (ACPI_SUCCESS(status))
> >> - return 0;
> >> + if (!ACPI_SUCCESS(status)) {
> >> + result = -ENODEV;
> >> + goto err_thermal_exit;
> >> + }
> >> + pr->flags.previously_online = 1;
> >> 
> >> - result = -ENODEV;
> >> - acpi_processor_thermal_exit(pr, device);
> >> + return 0;
> >> 
> >> +err_thermal_exit:
> >> + acpi_processor_thermal_exit(pr, device);
> >> err_power_exit:
> >> acpi_processor_power_exit(pr);
> >> return result;
> >> }
> >> 
> >> -static int acpi_processor_start(struct device *dev)
> >> -{
> >> - struct acpi_device *device = ACPI_COMPANION(dev);
> >> - int ret;
> >> -
> >> - if (!device)
> >> - return -ENODEV;
> >> -
> >> - /* Protect against concurrent CPU hotplug operations */
> >> - cpu_hotplug_disable();
> >> - ret = __acpi_processor_start(device);
> >> - cpu_hotplug_enable();
> >> - return ret;
> >> -}
> >> -
> >> static int acpi_processor_stop(struct device *dev)
> >> {
> >> struct acpi_device *device = ACPI_COMPANION(dev);
> >> @@ -279,9 +261,9 @@ static int __init acpi_processor_driver_init(void)
> >> if (result < 0)
> >> return result;
> >> 
> >> - result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
> >> -   "acpi/cpu-drv:online",
> >> -   acpi_soft_cpu_online, NULL);
> >> + result = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> >> +   "acpi/cpu-drv:online",
> >> +   acpi_soft_cpu_online, NULL);
> >> if (result < 0)
> >> goto err;
> >> hp_online = result;
> >> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> >> index 3f34ebb27525..e6f6074eadbf 100644
> >> --- a/include/acpi/processor.h
> >> +++ b/include/acpi/processor.h
> >> @@ -217,7 +217,7 @@ struct acpi_processor_flags {
> >> u8 has_lpi:1;
> >> u8 power_setup_done:1;
> >> u8 bm_rld_set:1;
> >> - u8 need_hotplug_init:1;
> >> + u8 previously_online:1;  
> > 
> > Reviewed-by: Miguel Luis <miguel.luis@oracle.com>
> > 
> > Miguel
> >   
> >> };
> >> 
> >> struct acpi_processor {
> >> -- 
> >> 2.39.2
> >>   
> >   
> 

Re: [PATCH v8 01/16] ACPI: processor: Simplify initial onlining to use same path for cold and hotplug

[Rafael J. Wysocki]

On Fri, Apr 26, 2024 at 7:49 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Fri, 26 Apr 2024 17:21:41 +0000
> Miguel Luis <miguel.luis@oracle.com> wrote:
>
> > Hi Jonathan,
> >
> > > On 26 Apr 2024, at 16:05, Miguel Luis <miguel.luis@oracle.com> wrote:
> > >
> > >
> > >
> > >> On 26 Apr 2024, at 13:51, Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> > >>
> > >> Separate code paths, combined with a flag set in acpi_processor.c to
> > >> indicate a struct acpi_processor was for a hotplugged CPU ensured that
> > >> per CPU data was only set up the first time that a CPU was initialized.
> > >> This appears to be unnecessary as the paths can be combined by letting
> > >> the online logic also handle any CPUs online at the time of driver load.
> > >>
> > >> Motivation for this change, beyond simplification, is that ARM64
> > >> virtual CPU HP uses the same code paths for hotplug and cold path in
> > >> acpi_processor.c so had no easy way to set the flag for hotplug only.
> > >> Removing this necessity will enable ARM64 vCPU HP to reuse the existing
> > >> code paths.
> > >>
> > >> Leave noisy pr_info() in place but update it to not state the CPU
> > >> was hotplugged.
> >
> > On a second thought, do we want to keep it? Can't we just assume that no
> > news is good news while keeping the warn right after __acpi_processor_start ?
>
> Good question - my inclination was to keep this in place for now as removing
> it would remove a source of information people may expect on x86 hotplug.
>
> Then maybe propose dropping it as overly noisy kernel as a follow up
> patch after this series is merged.  Felt like a potential rat hole I didn't
> want to go down if I could avoid it.
>
> If any x86 experts want to shout that no one cares then I'll happily drop
> the print.

You can do that I think and see if anyone complains.  I'm not really
expecting this to happen, though.

Re: [PATCH v8 01/16] ACPI: processor: Simplify initial onlining to use same path for cold and hotplug

[Jonathan Cameron]

On Fri, 26 Apr 2024 18:49:49 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Fri, 26 Apr 2024 17:21:41 +0000
> Miguel Luis <miguel.luis@oracle.com> wrote:
> 
> > Hi Jonathan, 
> >   
> > > On 26 Apr 2024, at 16:05, Miguel Luis <miguel.luis@oracle.com> wrote:
> > > 
> > > 
> > >     
> > >> On 26 Apr 2024, at 13:51, Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> > >> 
> > >> Separate code paths, combined with a flag set in acpi_processor.c to
> > >> indicate a struct acpi_processor was for a hotplugged CPU ensured that
> > >> per CPU data was only set up the first time that a CPU was initialized.
> > >> This appears to be unnecessary as the paths can be combined by letting
> > >> the online logic also handle any CPUs online at the time of driver load.
> > >> 
> > >> Motivation for this change, beyond simplification, is that ARM64
> > >> virtual CPU HP uses the same code paths for hotplug and cold path in
> > >> acpi_processor.c so had no easy way to set the flag for hotplug only.
> > >> Removing this necessity will enable ARM64 vCPU HP to reuse the existing
> > >> code paths.
> > >> 
> > >> Leave noisy pr_info() in place but update it to not state the CPU
> > >> was hotplugged.    
> > 
> > On a second thought, do we want to keep it? Can't we just assume that no 
> > news is good news while keeping the warn right after __acpi_processor_start ?  
> 
> Good question - my inclination was to keep this in place for now as removing
> it would remove a source of information people may expect on x86 hotplug.
> 
> Then maybe propose dropping it as overly noisy kernel as a follow up
> patch after this series is merged.  Felt like a potential rat hole I didn't
> want to go down if I could avoid it.
> 
> If any x86 experts want to shout that no one cares then I'll happily drop
> the print.  We've carefully made it so that on arm64 we have no way to tell
> if this is hotplug or normal cpu bring up so we can't just print it on
> hotplug.

I'm being silly. This is just one of the messages shouting out hotplug
happened and for that matter only occurs at online anyway which is trivially
detected.

There is a much more informative
  ACPI: CPU3: has been hot-added message
for example on the actual hotplug event.

Let's drop it for v9.

There is also a stale comment about a flag being set that is no longer
the case that I'll drop.

> 
> Jonathan
> 
> 
> > 
> > Miguel
> >   
> > >> 
> > >> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >> Reviewed-by: Hanjun Guo <guohanjun@huawei.com>
> > >> Tested-by: Miguel Luis <miguel.luis@oracle.com>
> > >> Reviewed-by: Gavin Shan <gshan@redhat.com>
> > >> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > >> 
> > >> ---
> > >> v8: No change
> > >> ---
> > >> drivers/acpi/acpi_processor.c   |  1 -
> > >> drivers/acpi/processor_driver.c | 44 ++++++++++-----------------------
> > >> include/acpi/processor.h        |  2 +-
> > >> 3 files changed, 14 insertions(+), 33 deletions(-)
> > >> 
> > >> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> > >> index 7a0dd35d62c9..7fc924aeeed0 100644
> > >> --- a/drivers/acpi/acpi_processor.c
> > >> +++ b/drivers/acpi/acpi_processor.c
> > >> @@ -216,7 +216,6 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
> > >> * gets online for the first time.
> > >> */
> > >> pr_info("CPU%d has been hot-added\n", pr->id);
> > >> - pr->flags.need_hotplug_init = 1;
> > >> 
> > >> out:
> > >> cpus_write_unlock();
> > >> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> > >> index 67db60eda370..55782eac3ff1 100644
> > >> --- a/drivers/acpi/processor_driver.c
> > >> +++ b/drivers/acpi/processor_driver.c
> > >> @@ -33,7 +33,6 @@ MODULE_AUTHOR("Paul Diefenbaugh");
> > >> MODULE_DESCRIPTION("ACPI Processor Driver");
> > >> MODULE_LICENSE("GPL");
> > >> 
> > >> -static int acpi_processor_start(struct device *dev);
> > >> static int acpi_processor_stop(struct device *dev);
> > >> 
> > >> static const struct acpi_device_id processor_device_ids[] = {
> > >> @@ -47,7 +46,6 @@ static struct device_driver acpi_processor_driver = {
> > >> .name = "processor",
> > >> .bus = &cpu_subsys,
> > >> .acpi_match_table = processor_device_ids,
> > >> - .probe = acpi_processor_start,
> > >> .remove = acpi_processor_stop,
> > >> };
> > >> 
> > >> @@ -115,12 +113,10 @@ static int acpi_soft_cpu_online(unsigned int cpu)
> > >> * CPU got physically hotplugged and onlined for the first time:
> > >> * Initialize missing things.
> > >> */
> > >> - if (pr->flags.need_hotplug_init) {
> > >> + if (!pr->flags.previously_online) {
> > >> int ret;
> > >> 
> > >> - pr_info("Will online and init hotplugged CPU: %d\n",
> > >> - pr->id);
> > >> - pr->flags.need_hotplug_init = 0;
> > >> + pr_info("Will online and init CPU: %d\n", pr->id);
> > >> ret = __acpi_processor_start(device);
> > >> WARN(ret, "Failed to start CPU: %d\n", pr->id);
> > >> } else {
> > >> @@ -167,9 +163,6 @@ static int __acpi_processor_start(struct acpi_device *device)
> > >> if (!pr)
> > >> return -ENODEV;
> > >> 
> > >> - if (pr->flags.need_hotplug_init)
> > >> - return 0;
> > >> -
> > >> result = acpi_cppc_processor_probe(pr);
> > >> if (result && !IS_ENABLED(CONFIG_ACPI_CPU_FREQ_PSS))
> > >> dev_dbg(&device->dev, "CPPC data invalid or not present\n");
> > >> @@ -185,32 +178,21 @@ static int __acpi_processor_start(struct acpi_device *device)
> > >> 
> > >> status = acpi_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
> > >>    acpi_processor_notify, device);
> > >> - if (ACPI_SUCCESS(status))
> > >> - return 0;
> > >> + if (!ACPI_SUCCESS(status)) {
> > >> + result = -ENODEV;
> > >> + goto err_thermal_exit;
> > >> + }
> > >> + pr->flags.previously_online = 1;
> > >> 
> > >> - result = -ENODEV;
> > >> - acpi_processor_thermal_exit(pr, device);
> > >> + return 0;
> > >> 
> > >> +err_thermal_exit:
> > >> + acpi_processor_thermal_exit(pr, device);
> > >> err_power_exit:
> > >> acpi_processor_power_exit(pr);
> > >> return result;
> > >> }
> > >> 
> > >> -static int acpi_processor_start(struct device *dev)
> > >> -{
> > >> - struct acpi_device *device = ACPI_COMPANION(dev);
> > >> - int ret;
> > >> -
> > >> - if (!device)
> > >> - return -ENODEV;
> > >> -
> > >> - /* Protect against concurrent CPU hotplug operations */
> > >> - cpu_hotplug_disable();
> > >> - ret = __acpi_processor_start(device);
> > >> - cpu_hotplug_enable();
> > >> - return ret;
> > >> -}
> > >> -
> > >> static int acpi_processor_stop(struct device *dev)
> > >> {
> > >> struct acpi_device *device = ACPI_COMPANION(dev);
> > >> @@ -279,9 +261,9 @@ static int __init acpi_processor_driver_init(void)
> > >> if (result < 0)
> > >> return result;
> > >> 
> > >> - result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
> > >> -   "acpi/cpu-drv:online",
> > >> -   acpi_soft_cpu_online, NULL);
> > >> + result = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> > >> +   "acpi/cpu-drv:online",
> > >> +   acpi_soft_cpu_online, NULL);
> > >> if (result < 0)
> > >> goto err;
> > >> hp_online = result;
> > >> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> > >> index 3f34ebb27525..e6f6074eadbf 100644
> > >> --- a/include/acpi/processor.h
> > >> +++ b/include/acpi/processor.h
> > >> @@ -217,7 +217,7 @@ struct acpi_processor_flags {
> > >> u8 has_lpi:1;
> > >> u8 power_setup_done:1;
> > >> u8 bm_rld_set:1;
> > >> - u8 need_hotplug_init:1;
> > >> + u8 previously_online:1;    
> > > 
> > > Reviewed-by: Miguel Luis <miguel.luis@oracle.com>
> > > 
> > > Miguel
> > >     
> > >> };
> > >> 
> > >> struct acpi_processor {
> > >> -- 
> > >> 2.39.2
> > >>     
> > >     
> >   
> 

Re: [PATCH v8 11/16] irqchip/gic-v3: Add support for ACPI's disabled but 'online capable' CPUs

[Jonathan Cameron]

> > @@ -2363,11 +2381,24 @@ gic_acpi_parse_madt_gicc(union acpi_subtable_headers *header,
> >  				(struct acpi_madt_generic_interrupt *)header;
> >  	u32 reg = readl_relaxed(acpi_data.dist_base + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
> >  	u32 size = reg == GIC_PIDR2_ARCH_GICv4 ? SZ_64K * 4 : SZ_64K * 2;
> > +	int cpu = get_cpu_for_acpi_id(gicc->uid);
> >  	void __iomem *redist_base;
> >  
> >  	if (!acpi_gicc_is_usable(gicc))
> >  		return 0;
> >  
> > +	/*
> > +	 * Capable but disabled CPUs can be brought online later. What about
> > +	 * the redistributor? ACPI doesn't want to say!
> > +	 * Virtual hotplug systems can use the MADT's "always-on" GICR entries.
> > +	 * Otherwise, prevent such CPUs from being brought online.
> > +	 */
> > +	if (!(gicc->flags & ACPI_MADT_ENABLED)) {  
> 
> Now this makes the above acpi_gicc_is_usable() very odd. It checks for
> MADT_ENABLED *or* GICC_ONLINE_CAPABLE. But we definitely don't want to
> deal with the lack of MADT_ENABLED.
> 
> So why don't we explicitly check for individual flags and get rid of
> acpi_gicc_is_usable(), as its new definition doesn't tell you anything
> useful?

That does seem to have evolved to something rather odd.

I messed around with various reorganizations of the boolean logic
and ended up with same 2 conditions as here as otherwise
the indent gets deep and the code becomes fiddlier to reason about
(see below for result)

> 
> > +		return 0;
> > +	}
> > +
> >  	redist_base = ioremap(gicc->gicr_base_address, size);
> >  	if (!redist_base)
> >  		return -ENOMEM;
> > @@ -2413,9 +2444,12 @@ static int __init gic_acpi_match_gicc(union acpi_subtable_headers *header,
> >  
> >  	/*
> >  	 * If GICC is enabled and has valid gicr base address, then it means
> > -	 * GICR base is presented via GICC
> > +	 * GICR base is presented via GICC. The redistributor is only known to
> > +	 * be accessible if the GICC is marked as enabled. If this bit is not
> > +	 * set, we'd need to add the redistributor at runtime, which isn't
> > +	 * supported.
> >  	 */
> > -	if (acpi_gicc_is_usable(gicc) && gicc->gicr_base_address)
> > +	if (gicc->flags & ACPI_MADT_ENABLED && gicc->gicr_base_address)
> >  		acpi_data.enabled_rdists++;
> >  
> >  	return 0;
> > diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> > index 9844a3f9c4e5..fcfb7bb6789e 100644
> > --- a/include/linux/acpi.h
> > +++ b/include/linux/acpi.h
> > @@ -239,7 +239,8 @@ void acpi_table_print_madt_entry (struct acpi_subtable_header *madt);
> >  
> >  static inline bool acpi_gicc_is_usable(struct acpi_madt_generic_interrupt *gicc)
> >  {
> > -	return gicc->flags & ACPI_MADT_ENABLED;
> > +	return gicc->flags & (ACPI_MADT_ENABLED |
> > +			      ACPI_MADT_GICC_ONLINE_CAPABLE);
> >  }
> >  
> >  /* the following numa functions are architecture-dependent */  
> 
> Thanks,

I'll not send a formal v9 until early next week, so here is the current state
if you have time to take another look before then.

From a8a54cfbadccf1782b7cc04b93eb875dedbee7a9 Mon Sep 17 00:00:00 2001
From: James Morse <james.morse@arm.com>
Date: Thu, 18 Apr 2024 14:54:07 +0100
Subject: [PATCH] irqchip/gic-v3: Add support for ACPI's disabled but 'online
 capable' CPUs

To support virtual CPU hotplug, ACPI has added an 'online capable' bit
to the MADT GICC entries. This indicates a disabled CPU entry may not
be possible to online via PSCI until firmware has set enabled bit in
_STA.

This means that a "usable" GIC redistributor is one that is marked as
either enabled, or online capable. The meaning of the
acpi_gicc_is_usable() would become less clear than just checking the
pair of flags at call sites. As such, drop that helper function.
The test in gic_acpi_match_gicc() remains as testing just the
enabled bit so the count of enabled distributors is correct.

What about the redistributor in the GICC entry? ACPI doesn't want to say.
Assume the worst: When a redistributor is described in the GICC entry,
but the entry is marked as disabled at boot, assume the redistributor
is inaccessible.

The GICv3 driver doesn't support late online of redistributors, so this
means the corresponding CPU can't be brought online either.
Rather than modifying cpu masks that may already have been used,
register a new cpuhp callback to fail this case. This must run earlier
than the main gic_starting_cpu() so that this case can be rejected
before the section of cpuhp that runs on the CPU that is coming up as
that is not allowed to fail. This solution keeps the handling of this
broken firmware corner case local to the GIC driver. As precise ordering
of this callback doesn't need to be controlled as long as it is
in that initial prepare phase, use CPUHP_BP_PREPARE_DYN.

Systems that want CPU hotplug in a VM can ensure their redistributors
are always-on, and describe them that way with a GICR entry in the MADT.

Suggested-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: James Morse <james.morse@arm.com>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Tested-by: Miguel Luis <miguel.luis@oracle.com>
Co-developed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

---
v9: Thanks to Marc for quick follow up.
Fix up description and drop the acpi_gicc_is_usable() check given that
now doesn't actually mean they are usable.

Thanks to Marc for review and suggestions!
v8: Change the handling of broken rdists to fail cpuhp rather than
    modifying the cpu_present and cpu_possible masks.
    Updated commit text to reflect that.
    Added a sb tag for Marc given this is more or less what he put
    in his review comment.
---
 arch/arm64/kernel/smp.c       |  3 ++-
 drivers/acpi/processor_core.c |  3 ++-
 drivers/irqchip/irq-gic-v3.c  | 44 +++++++++++++++++++++++++++++++----
 include/linux/acpi.h          |  5 ----
 4 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 4ced34f62dab..afe835c1cbe2 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -523,7 +523,8 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor)
 {
 	u64 hwid = processor->arm_mpidr;
 
-	if (!acpi_gicc_is_usable(processor)) {
+	if (!(processor->flags &
+	      (ACPI_MADT_ENABLED | ACPI_MADT_GICC_ONLINE_CAPABLE))) {
 		pr_debug("skipping disabled CPU entry with 0x%llx MPIDR\n", hwid);
 		return;
 	}
diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
index b203cfe28550..b04b684f3190 100644
--- a/drivers/acpi/processor_core.c
+++ b/drivers/acpi/processor_core.c
@@ -90,7 +90,8 @@ static int map_gicc_mpidr(struct acpi_subtable_header *entry,
 	struct acpi_madt_generic_interrupt *gicc =
 	    container_of(entry, struct acpi_madt_generic_interrupt, header);
 
-	if (!acpi_gicc_is_usable(gicc))
+	if (!(gicc->flags &
+	      (ACPI_MADT_ENABLED | ACPI_MADT_GICC_ONLINE_CAPABLE)))
 		return -ENODEV;
 
 	/* device_declaration means Device object in DSDT, in the
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 10af15f93d4d..45272316d155 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -44,6 +44,8 @@
 
 #define GIC_IRQ_TYPE_PARTITION	(GIC_IRQ_TYPE_LPI + 1)
 
+static struct cpumask broken_rdists __read_mostly;
+
 struct redist_region {
 	void __iomem		*redist_base;
 	phys_addr_t		phys_base;
@@ -1293,6 +1295,18 @@ static void gic_cpu_init(void)
 #define MPIDR_TO_SGI_RS(mpidr)	(MPIDR_RS(mpidr) << ICC_SGI1R_RS_SHIFT)
 #define MPIDR_TO_SGI_CLUSTER_ID(mpidr)	((mpidr) & ~0xFUL)
 
+/*
+ * gic_starting_cpu() is called after the last point where cpuhp is allowed
+ * to fail. So pre check for problems earlier.
+ */
+static int gic_check_rdist(unsigned int cpu)
+{
+	if (cpumask_test_cpu(cpu, &broken_rdists))
+		return -EINVAL;
+
+	return 0;
+}
+
 static int gic_starting_cpu(unsigned int cpu)
 {
 	gic_cpu_init();
@@ -1384,6 +1398,10 @@ static void __init gic_smp_init(void)
 	};
 	int base_sgi;
 
+	cpuhp_setup_state_nocalls(CPUHP_BP_PREPARE_DYN,
+				  "irqchip/arm/gicv3:checkrdist",
+				  gic_check_rdist, NULL);
+
 	cpuhp_setup_state_nocalls(CPUHP_AP_IRQ_GIC_STARTING,
 				  "irqchip/arm/gicv3:starting",
 				  gic_starting_cpu, NULL);
@@ -2363,11 +2381,25 @@ gic_acpi_parse_madt_gicc(union acpi_subtable_headers *header,
 				(struct acpi_madt_generic_interrupt *)header;
 	u32 reg = readl_relaxed(acpi_data.dist_base + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
 	u32 size = reg == GIC_PIDR2_ARCH_GICv4 ? SZ_64K * 4 : SZ_64K * 2;
+	int cpu = get_cpu_for_acpi_id(gicc->uid);
 	void __iomem *redist_base;
 
-	if (!acpi_gicc_is_usable(gicc))
+	/* Neither enabled or online capable means it doesn't exist, skip it */
+	if (!(gicc->flags & (ACPI_MADT_ENABLED | ACPI_MADT_GICC_ONLINE_CAPABLE)))
 		return 0;
 
+	/*
+	 * Capable but disabled CPUs can be brought online later. What about
+	 * the redistributor? ACPI doesn't want to say!
+	 * Virtual hotplug systems can use the MADT's "always-on" GICR entries.
+	 * Otherwise, prevent such CPUs from being brought online.
+	 */
+	if (!(gicc->flags & ACPI_MADT_ENABLED)) {
+		pr_warn("CPU %u's redistributor is inaccessible: this CPU can't be brought online\n", cpu);
+		cpumask_set_cpu(cpu, &broken_rdists);
+		return 0;
+	}
+
 	redist_base = ioremap(gicc->gicr_base_address, size);
 	if (!redist_base)
 		return -ENOMEM;
@@ -2413,9 +2445,12 @@ static int __init gic_acpi_match_gicc(union acpi_subtable_headers *header,
 
 	/*
 	 * If GICC is enabled and has valid gicr base address, then it means
-	 * GICR base is presented via GICC
+	 * GICR base is presented via GICC. The redistributor is only known to
+	 * be accessible if the GICC is marked as enabled. If this bit is not
+	 * set, we'd need to add the redistributor at runtime, which isn't
+	 * supported.
 	 */
-	if (acpi_gicc_is_usable(gicc) && gicc->gicr_base_address)
+	if (gicc->flags & ACPI_MADT_ENABLED && gicc->gicr_base_address)
 		acpi_data.enabled_rdists++;
 
 	return 0;
@@ -2474,7 +2509,8 @@ static int __init gic_acpi_parse_virt_madt_gicc(union acpi_subtable_headers *hea
 	int maint_irq_mode;
 	static int first_madt = true;
 
-	if (!acpi_gicc_is_usable(gicc))
+	if (!(gicc->flags &
+	      (ACPI_MADT_ENABLED | ACPI_MADT_GICC_ONLINE_CAPABLE)))
 		return 0;
 
 	maint_irq_mode = (gicc->flags & ACPI_MADT_VGIC_IRQ_MODE) ?
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 9844a3f9c4e5..cf5d2a6950ec 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -237,11 +237,6 @@ acpi_table_parse_cedt(enum acpi_cedt_type id,
 int acpi_parse_mcfg (struct acpi_table_header *header);
 void acpi_table_print_madt_entry (struct acpi_subtable_header *madt);
 
-static inline bool acpi_gicc_is_usable(struct acpi_madt_generic_interrupt *gicc)
-{
-	return gicc->flags & ACPI_MADT_ENABLED;
-}
-
 /* the following numa functions are architecture-dependent */
 void acpi_numa_slit_init (struct acpi_table_slit *slit);
 
-- 
2.39.2



> 
> 	M.
> 

Re: [PATCH v8 11/16] irqchip/gic-v3: Add support for ACPI's disabled but 'online capable' CPUs

[Marc Zyngier]

On Fri, 26 Apr 2024 19:28:58 +0100,
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> 
> 
> I'll not send a formal v9 until early next week, so here is the current state
> if you have time to take another look before then.

Don't bother resending this on my account -- you only sent it on
Friday and there hasn't been much response to it yet. There is still a
problem (see below), but looks otherwise OK.

[...]

> @@ -2363,11 +2381,25 @@ gic_acpi_parse_madt_gicc(union acpi_subtable_headers *header,
>  				(struct acpi_madt_generic_interrupt *)header;
>  	u32 reg = readl_relaxed(acpi_data.dist_base + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
>  	u32 size = reg == GIC_PIDR2_ARCH_GICv4 ? SZ_64K * 4 : SZ_64K * 2;
> +	int cpu = get_cpu_for_acpi_id(gicc->uid);

I already commented that get_cpu_for_acpi_id() can...

>  	void __iomem *redist_base;
>  
> -	if (!acpi_gicc_is_usable(gicc))
> +	/* Neither enabled or online capable means it doesn't exist, skip it */
> +	if (!(gicc->flags & (ACPI_MADT_ENABLED | ACPI_MADT_GICC_ONLINE_CAPABLE)))
>  		return 0;
>  
> +	/*
> +	 * Capable but disabled CPUs can be brought online later. What about
> +	 * the redistributor? ACPI doesn't want to say!
> +	 * Virtual hotplug systems can use the MADT's "always-on" GICR entries.
> +	 * Otherwise, prevent such CPUs from being brought online.
> +	 */
> +	if (!(gicc->flags & ACPI_MADT_ENABLED)) {
> +		pr_warn("CPU %u's redistributor is inaccessible: this CPU can't be brought online\n", cpu);
> +		cpumask_set_cpu(cpu, &broken_rdists);

... return -EINVAL, and then be passed to cpumask_set_cpu(), with
interesting effects. It shouldn't happen, but I trust anything that
comes from firmware tables as much as I trust a campaigning
politician's promises. This should really result in the RD being
considered unusable, but without affecting any CPU (there is no valid
CPU the first place).

Another question is what get_cpu_for acpi_id() returns for a disabled
CPU. A valid CPU number? Or -EINVAL?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v8 11/16] irqchip/gic-v3: Add support for ACPI's disabled but 'online capable' CPUs

[Jonathan Cameron]

On Sun, 28 Apr 2024 12:28:03 +0100
Marc Zyngier <maz@kernel.org> wrote:

> On Fri, 26 Apr 2024 19:28:58 +0100,
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> > 
> > 
> > I'll not send a formal v9 until early next week, so here is the current state
> > if you have time to take another look before then.  
> 
> Don't bother resending this on my account -- you only sent it on
> Friday and there hasn't been much response to it yet. There is still a
> problem (see below), but looks otherwise OK.
> 
> [...]
> 
> > @@ -2363,11 +2381,25 @@ gic_acpi_parse_madt_gicc(union acpi_subtable_headers *header,
> >  				(struct acpi_madt_generic_interrupt *)header;
> >  	u32 reg = readl_relaxed(acpi_data.dist_base + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
> >  	u32 size = reg == GIC_PIDR2_ARCH_GICv4 ? SZ_64K * 4 : SZ_64K * 2;
> > +	int cpu = get_cpu_for_acpi_id(gicc->uid);  
> 
> I already commented that get_cpu_for_acpi_id() can...

Indeed sorry - I blame Friday syndrome for me failing to address that.

> 
> >  	void __iomem *redist_base;
> >  
> > -	if (!acpi_gicc_is_usable(gicc))
> > +	/* Neither enabled or online capable means it doesn't exist, skip it */
> > +	if (!(gicc->flags & (ACPI_MADT_ENABLED | ACPI_MADT_GICC_ONLINE_CAPABLE)))
> >  		return 0;
> >  
> > +	/*
> > +	 * Capable but disabled CPUs can be brought online later. What about
> > +	 * the redistributor? ACPI doesn't want to say!
> > +	 * Virtual hotplug systems can use the MADT's "always-on" GICR entries.
> > +	 * Otherwise, prevent such CPUs from being brought online.
> > +	 */
> > +	if (!(gicc->flags & ACPI_MADT_ENABLED)) {
> > +		pr_warn("CPU %u's redistributor is inaccessible: this CPU can't be brought online\n", cpu);
> > +		cpumask_set_cpu(cpu, &broken_rdists);  
> 
> ... return -EINVAL, and then be passed to cpumask_set_cpu(), with
> interesting effects. It shouldn't happen, but I trust anything that
> comes from firmware tables as much as I trust a campaigning
> politician's promises. This should really result in the RD being
> considered unusable, but without affecting any CPU (there is no valid
> CPU the first place).
> 
> Another question is what get_cpu_for acpi_id() returns for a disabled
> CPU. A valid CPU number? Or -EINVAL?
It's a match function that works by iterating over 0 to nr_cpu_ids and

if (uid == get_acpi_id_for_cpu(cpu))

So the question become does get_acpi_id_for_cpu() return a valid CPU
number for a disabled CPU.

That uses acpi_cpu_get_madt_gicc(cpu)->uid so this all gets a bit circular.
That looks it up via cpu_madt_gicc[cpu] which after the proposed updated
patch is set if enabled or online capable.  There are however a few other
error checks in acpi_map_gic_cpu_interface() that could lead to it
not being set (MPIDR validity checks). I suspect all of these end up being
fatal elsewhere which is why this hasn't blown up before.

If any of those cases are possible we could get a null pointer
dereference.

Easy to harden this case via the following (which will leave us with
-EINVAL.  There are other call sites that might trip over this.
I'm inclined to harden them as a separate issue though so as not
to get in the way of this patch set.


diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index bc9a6656fc0c..a407f9cd549e 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -124,7 +124,8 @@ static inline int get_cpu_for_acpi_id(u32 uid)
        int cpu;

        for (cpu = 0; cpu < nr_cpu_ids; cpu++)
-               if (uid == get_acpi_id_for_cpu(cpu))
+               if (acpi_cpu_get_madt_gicc(cpu) &&
+                   uid == get_acpi_id_for_cpu(cpu))
                        return cpu;

        return -EINVAL;

I'll spin an additional patch to make that change after testing I haven't
messed it up.

At the call site in gic_acpi_parse_madt_gicc() I'm not sure we can do better
than just skipping setting broken_rdists. I'll also pull the declaration of
that cpu variable down into this condition so it's more obvious we only
care about it in this error path.

Jonathan





> 
> Thanks,
> 
> 	M.
> 

Re: [PATCH v8 04/16] ACPI: processor: Move checks and availability of acpi_processor earlier

[Gavin Shan]

On 4/26/24 23:51, Jonathan Cameron wrote:
> Make the per_cpu(processors, cpu) entries available earlier so that
> they are available in arch_register_cpu() as ARM64 will need access
> to the acpi_handle to distinguish between acpi_processor_add()
> and earlier registration attempts (which will fail as _STA cannot
> be checked).
> 
> Reorder the remove flow to clear this per_cpu() after
> arch_unregister_cpu() has completed, allowing it to be used in
> there as well.
> 
> Note that on x86 for the CPU hotplug case, the pr->id prior to
> acpi_map_cpu() may be invalid. Thus the per_cpu() structures
> must be initialized after that call or after checking the ID
> is valid (not hotplug path).
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> ---
> v8: On buggy bios detection when setting per_cpu structures
>      do not carry on.
>      Fix up the clearing of per cpu structures to remove unwanted
>      side effects and ensure an error code isn't use to reference them.
> ---
>   drivers/acpi/acpi_processor.c | 79 +++++++++++++++++++++--------------
>   1 file changed, 48 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> index ba0a6f0ac841..3b180e21f325 100644
> --- a/drivers/acpi/acpi_processor.c
> +++ b/drivers/acpi/acpi_processor.c
> @@ -183,8 +183,38 @@ static void __init acpi_pcc_cpufreq_init(void) {}
>   #endif /* CONFIG_X86 */
>   
>   /* Initialization */
> +static DEFINE_PER_CPU(void *, processor_device_array);
> +
> +static bool acpi_processor_set_per_cpu(struct acpi_processor *pr,
> +				       struct acpi_device *device)
> +{
> +	BUG_ON(pr->id >= nr_cpu_ids);

One blank line after BUG_ON() if we need to follow original implementation.

> +	/*
> +	 * Buggy BIOS check.
> +	 * ACPI id of processors can be reported wrongly by the BIOS.
> +	 * Don't trust it blindly
> +	 */
> +	if (per_cpu(processor_device_array, pr->id) != NULL &&
> +	    per_cpu(processor_device_array, pr->id) != device) {
> +		dev_warn(&device->dev,
> +			 "BIOS reported wrong ACPI id %d for the processor\n",
> +			 pr->id);
> +		/* Give up, but do not abort the namespace scan. */

It depends on how the return value is handled by the caller if the namespace
is continued to be scanned. The caller can be acpi_processor_hotadd_init()
and acpi_processor_get_info() after this patch is applied. So I think this
specific comment need to be moved to the caller.

Besides, it seems acpi_processor_set_per_cpu() isn't properly called and
memory leakage can happen. More details are given below.

> +		return false;
> +	}
> +	/*
> +	 * processor_device_array is not cleared on errors to allow buggy BIOS
> +	 * checks.
> +	 */
> +	per_cpu(processor_device_array, pr->id) = device;
> +	per_cpu(processors, pr->id) = pr;
> +
> +	return true;
> +}
> +
>   #ifdef CONFIG_ACPI_HOTPLUG_CPU
> -static int acpi_processor_hotadd_init(struct acpi_processor *pr)
> +static int acpi_processor_hotadd_init(struct acpi_processor *pr,
> +				      struct acpi_device *device)
>   {
>   	int ret;
>   
> @@ -198,8 +228,15 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
>   	if (ret)
>   		goto out;
>   
> +	if (!acpi_processor_set_per_cpu(pr, device)) {
> +		acpi_unmap_cpu(pr->id);
> +		goto out;
> +	}
> +

With the 'goto out', zero is returned from acpi_processor_hotadd_init() to acpi_processor_get_info().
The zero return value is carried from acpi_map_cpu() in acpi_processor_hotadd_init(). If I'm correct,
we need return errno from acpi_processor_get_info() to acpi_processor_add() so that cleanup can be
done. For example, the cleanup corresponding to the 'err' tag can be done in acpi_processor_add().
Otherwise, we will have memory leakage.

>   	ret = arch_register_cpu(pr->id);
>   	if (ret) {
> +		/* Leave the processor device array in place to detect buggy bios */
> +		per_cpu(processors, pr->id) = NULL;
>   		acpi_unmap_cpu(pr->id);
>   		goto out;
>   	}
> @@ -217,7 +254,8 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
>   	return ret;
>   }
>   #else
> -static inline int acpi_processor_hotadd_init(struct acpi_processor *pr)
> +static inline int acpi_processor_hotadd_init(struct acpi_processor *pr,
> +					     struct acpi_device *device)
>   {
>   	return -ENODEV;
>   }
> @@ -316,10 +354,13 @@ static int acpi_processor_get_info(struct acpi_device *device)
>   	 *  because cpuid <-> apicid mapping is persistent now.
>   	 */
>   	if (invalid_logical_cpuid(pr->id) || !cpu_present(pr->id)) {
> -		int ret = acpi_processor_hotadd_init(pr);
> +		int ret = acpi_processor_hotadd_init(pr, device);
>   
>   		if (ret)
>   			return ret;
> +	} else {
> +		if (!acpi_processor_set_per_cpu(pr, device))
> +			return 0;
>   	}
>   

For non-hotplug case, we still need pass the error to acpi_processor_add() so that
cleanup corresponding 'err' tag can be done. Otherwise, we will have memory leakage.

>   	/*
> @@ -365,8 +406,6 @@ static int acpi_processor_get_info(struct acpi_device *device)
>    * (cpu_data(cpu)) values, like CPU feature flags, family, model, etc.
>    * Such things have to be put in and set up by the processor driver's .probe().
>    */
> -static DEFINE_PER_CPU(void *, processor_device_array);
> -
>   static int acpi_processor_add(struct acpi_device *device,
>   					const struct acpi_device_id *id)
>   {
> @@ -395,28 +434,6 @@ static int acpi_processor_add(struct acpi_device *device,
>   	if (result) /* Processor is not physically present or unavailable */
>   		return 0;
>   
> -	BUG_ON(pr->id >= nr_cpu_ids);
> -
> -	/*
> -	 * Buggy BIOS check.
> -	 * ACPI id of processors can be reported wrongly by the BIOS.
> -	 * Don't trust it blindly
> -	 */
> -	if (per_cpu(processor_device_array, pr->id) != NULL &&
> -	    per_cpu(processor_device_array, pr->id) != device) {
> -		dev_warn(&device->dev,
> -			"BIOS reported wrong ACPI id %d for the processor\n",
> -			pr->id);
> -		/* Give up, but do not abort the namespace scan. */
> -		goto err;
> -	}
> -	/*
> -	 * processor_device_array is not cleared on errors to allow buggy BIOS
> -	 * checks.
> -	 */
> -	per_cpu(processor_device_array, pr->id) = device;
> -	per_cpu(processors, pr->id) = pr;
> -
>   	dev = get_cpu_device(pr->id);
>   	if (!dev) {
>   		result = -ENODEV;
> @@ -469,10 +486,6 @@ static void acpi_processor_remove(struct acpi_device *device)
>   	device_release_driver(pr->dev);
>   	acpi_unbind_one(pr->dev);
>   
> -	/* Clean up. */
> -	per_cpu(processor_device_array, pr->id) = NULL;
> -	per_cpu(processors, pr->id) = NULL;
> -
>   	cpu_maps_update_begin();
>   	cpus_write_lock();
>   
> @@ -480,6 +493,10 @@ static void acpi_processor_remove(struct acpi_device *device)
>   	arch_unregister_cpu(pr->id);
>   	acpi_unmap_cpu(pr->id);
>   
> +	/* Clean up. */
> +	per_cpu(processor_device_array, pr->id) = NULL;
> +	per_cpu(processors, pr->id) = NULL;
> +
>   	cpus_write_unlock();
>   	cpu_maps_update_done();
>   

Thanks,
Gavin

Re: [PATCH v8 05/16] ACPI: processor: Add acpi_get_processor_handle() helper

[Gavin Shan]

On 4/26/24 23:51, Jonathan Cameron wrote:
> If CONFIG_ACPI_PROCESSOR provide a helper to retrieve the
> acpi_handle for a given CPU allowing access to methods
> in DSDT.
> 
> Tested-by: Miguel Luis <miguel.luis@oracle.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> ---
> v8: Code simplification suggested by Rafael.
>      Fixup ;; spotted by Gavin
> ---
>   drivers/acpi/acpi_processor.c | 11 +++++++++++
>   include/linux/acpi.h          |  7 +++++++
>   2 files changed, 18 insertions(+)
> 

Reviewed-by: Gavin Shan <gshan@redhat.com>

> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> index 3b180e21f325..ecc2721fecae 100644
> --- a/drivers/acpi/acpi_processor.c
> +++ b/drivers/acpi/acpi_processor.c
> @@ -35,6 +35,17 @@ EXPORT_PER_CPU_SYMBOL(processors);
>   struct acpi_processor_errata errata __read_mostly;
>   EXPORT_SYMBOL_GPL(errata);
>   
> +acpi_handle acpi_get_processor_handle(int cpu)
> +{
> +	struct acpi_processor *pr;
> +
> +	pr = per_cpu(processors, cpu);
> +	if (pr)
> +		return pr->handle;
> +
> +	return NULL;
> +}
> +

Maybe it's worthy to have more check here, something like below.
However, it's also fine without the extra check.

	if (cpu >= nr_cpu_ids || !cpu_possible(cpu))
		return NULL;

>   static int acpi_processor_errata_piix4(struct pci_dev *dev)
>   {
>   	u8 value1 = 0;
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 34829f2c517a..9844a3f9c4e5 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -309,6 +309,8 @@ int acpi_map_cpu(acpi_handle handle, phys_cpuid_t physid, u32 acpi_id,
>   int acpi_unmap_cpu(int cpu);
>   #endif /* CONFIG_ACPI_HOTPLUG_CPU */
>   
> +acpi_handle acpi_get_processor_handle(int cpu);
> +
>   #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC
>   int acpi_get_ioapic_id(acpi_handle handle, u32 gsi_base, u64 *phys_addr);
>   #endif
> @@ -1077,6 +1079,11 @@ static inline bool acpi_sleep_state_supported(u8 sleep_state)
>   	return false;
>   }
>   
> +static inline acpi_handle acpi_get_processor_handle(int cpu)
> +{
> +	return NULL;
> +}
> +
>   #endif	/* !CONFIG_ACPI */
>   
>   extern void arch_post_acpi_subsys_init(void);

Thanks,
Gavin

Re: [PATCH v8 12/16] arm64: psci: Ignore DENIED CPUs

[Gavin Shan]

On 4/26/24 23:51, Jonathan Cameron wrote:
> From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> 
> When a CPU is marked as disabled, but online capable in the MADT, PSCI
> applies some firmware policy to control when it can be brought online.
> PSCI returns DENIED to a CPU_ON request if this is not currently
> permitted. The OS can learn the current policy from the _STA enabled bit.
> 
> Handle the PSCI DENIED return code gracefully instead of printing an
> error.
> 
> Note the alternatives to the PSCI cpu_boot() callback do not
> return -EPERM so the change in smp.c has no affect.
> 
> See https://developer.arm.com/documentation/den0022/f/?lang=en page 58.
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> [ morse: Rewrote commit message ]
> Signed-off-by: James Morse <james.morse@arm.com>
> Tested-by: Miguel Luis <miguel.luis@oracle.com>
> Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
> Tested-by: Jianyong Wu <jianyong.wu@arm.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> v8: Note in commit message that the -EPERM guard on the error print
>      only affects PSCI as other options never use this error code.
>      Should they do so in future, that may well indicate that they
>      now support similar refusal to boot.
> ---
>   arch/arm64/kernel/psci.c | 2 +-
>   arch/arm64/kernel/smp.c  | 3 ++-
>   2 files changed, 3 insertions(+), 2 deletions(-)
> 

Thanks for the check and clarification that -EPERM is only sensible
to PSCI. With the clarification:

Reviewed-by: Gavin Shan <gshan@redhat.com>

Thanks,
Gavin

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

[Gavin Shan]

On 4/26/24 23:51, Jonathan Cameron wrote:
> 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@kernel.org>
> Tested-by: Miguel Luis <miguel.luis@oracle.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> V8: No change. (tags collected only)
> ---
>   arch/arm64/kernel/smp.c | 53 +++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 53 insertions(+)
> 

Reviewed-by: Gavin Shan <gshan@redhat.com>

Re: [PATCH v8 04/16] ACPI: processor: Move checks and availability of acpi_processor earlier

[Jonathan Cameron]

On Tue, 30 Apr 2024 14:17:24 +1000
Gavin Shan <gshan@redhat.com> wrote:

> On 4/26/24 23:51, Jonathan Cameron wrote:
> > Make the per_cpu(processors, cpu) entries available earlier so that
> > they are available in arch_register_cpu() as ARM64 will need access
> > to the acpi_handle to distinguish between acpi_processor_add()
> > and earlier registration attempts (which will fail as _STA cannot
> > be checked).
> > 
> > Reorder the remove flow to clear this per_cpu() after
> > arch_unregister_cpu() has completed, allowing it to be used in
> > there as well.
> > 
> > Note that on x86 for the CPU hotplug case, the pr->id prior to
> > acpi_map_cpu() may be invalid. Thus the per_cpu() structures
> > must be initialized after that call or after checking the ID
> > is valid (not hotplug path).
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > ---
> > v8: On buggy bios detection when setting per_cpu structures
> >      do not carry on.
> >      Fix up the clearing of per cpu structures to remove unwanted
> >      side effects and ensure an error code isn't use to reference them.
> > ---
> >   drivers/acpi/acpi_processor.c | 79 +++++++++++++++++++++--------------
> >   1 file changed, 48 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> > index ba0a6f0ac841..3b180e21f325 100644
> > --- a/drivers/acpi/acpi_processor.c
> > +++ b/drivers/acpi/acpi_processor.c
> > @@ -183,8 +183,38 @@ static void __init acpi_pcc_cpufreq_init(void) {}
> >   #endif /* CONFIG_X86 */
> >   
> >   /* Initialization */
> > +static DEFINE_PER_CPU(void *, processor_device_array);
> > +
> > +static bool acpi_processor_set_per_cpu(struct acpi_processor *pr,
> > +				       struct acpi_device *device)
> > +{
> > +	BUG_ON(pr->id >= nr_cpu_ids);  
> 
> One blank line after BUG_ON() if we need to follow original implementation.

Sure unintentional - I'll put that back.

> 
> > +	/*
> > +	 * Buggy BIOS check.
> > +	 * ACPI id of processors can be reported wrongly by the BIOS.
> > +	 * Don't trust it blindly
> > +	 */
> > +	if (per_cpu(processor_device_array, pr->id) != NULL &&
> > +	    per_cpu(processor_device_array, pr->id) != device) {
> > +		dev_warn(&device->dev,
> > +			 "BIOS reported wrong ACPI id %d for the processor\n",
> > +			 pr->id);
> > +		/* Give up, but do not abort the namespace scan. */  
> 
> It depends on how the return value is handled by the caller if the namespace
> is continued to be scanned. The caller can be acpi_processor_hotadd_init()
> and acpi_processor_get_info() after this patch is applied. So I think this
> specific comment need to be moved to the caller.

Good point. This gets messy and was an unintended change.

Previously the options were:
1) acpi_processor_get_info() failed for other reasons - this code was never called.
2) acpi_processor_get_info() succeeded without acpi_processor_hotadd_init (non hotplug)
   this code then ran and would paper over the problem doing a bunch of cleanup under err.
3) acpi_processor_get_info() succeeded with acpi_processor_hotadd_init called.
   This code then ran and would paper over the problem doing a bunch of cleanup under err.

We should maintain that or argue cleanly against it.

This isn't helped the the fact I have no idea which cases we care about for that bios
bug handling.  Do any of those bios's ever do hotplug?  Guess we have to try and maintain
whatever protection this was offering.

Also, the original code leaks data in some paths and I have limited idea
of whether it is intentional or not. So to tidy the issue up that you've identified
I'll need to try and make that code consistent first.

I suspect the only way to do that is going to be to duplicate the allocations we
'want' to leak to deal with the bios bug detection.

For example acpi_processor_get_info() failing leaks pr and pr->throttling.shared_cpu_map
before this series. After this series we need pr to leak because it's used for the detection
via processor_device_array.

I'll work through this but it's going to be tricky to tell if we get right.
Step 1 will be closing the existing leaks and then we will have something
consistent to build on.

> 
> Besides, it seems acpi_processor_set_per_cpu() isn't properly called and
> memory leakage can happen. More details are given below.
> 
> > +		return false;
> > +	}
> > +	/*
> > +	 * processor_device_array is not cleared on errors to allow buggy BIOS
> > +	 * checks.
> > +	 */
> > +	per_cpu(processor_device_array, pr->id) = device;
> > +	per_cpu(processors, pr->id) = pr;
> > +
> > +	return true;
> > +}
> > +
> >   #ifdef CONFIG_ACPI_HOTPLUG_CPU
> > -static int acpi_processor_hotadd_init(struct acpi_processor *pr)
> > +static int acpi_processor_hotadd_init(struct acpi_processor *pr,
> > +				      struct acpi_device *device)
> >   {
> >   	int ret;
> >   
> > @@ -198,8 +228,15 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
> >   	if (ret)
> >   		goto out;
> >   
> > +	if (!acpi_processor_set_per_cpu(pr, device)) {
> > +		acpi_unmap_cpu(pr->id);
> > +		goto out;
> > +	}
> > +  
> 
> With the 'goto out', zero is returned from acpi_processor_hotadd_init() to acpi_processor_get_info().
> The zero return value is carried from acpi_map_cpu() in acpi_processor_hotadd_init(). If I'm correct,
> we need return errno from acpi_processor_get_info() to acpi_processor_add() so that cleanup can be
> done. For example, the cleanup corresponding to the 'err' tag can be done in acpi_processor_add().
> Otherwise, we will have memory leakage.
> 
> >   	ret = arch_register_cpu(pr->id);
> >   	if (ret) {
> > +		/* Leave the processor device array in place to detect buggy bios */
> > +		per_cpu(processors, pr->id) = NULL;
> >   		acpi_unmap_cpu(pr->id);
> >   		goto out;
> >   	}
> > @@ -217,7 +254,8 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
> >   	return ret;
> >   }
> >   #else
> > -static inline int acpi_processor_hotadd_init(struct acpi_processor *pr)
> > +static inline int acpi_processor_hotadd_init(struct acpi_processor *pr,
> > +					     struct acpi_device *device)
> >   {
> >   	return -ENODEV;
> >   }
> > @@ -316,10 +354,13 @@ static int acpi_processor_get_info(struct acpi_device *device)
> >   	 *  because cpuid <-> apicid mapping is persistent now.
> >   	 */
> >   	if (invalid_logical_cpuid(pr->id) || !cpu_present(pr->id)) {
> > -		int ret = acpi_processor_hotadd_init(pr);
> > +		int ret = acpi_processor_hotadd_init(pr, device);
> >   
> >   		if (ret)
> >   			return ret;
> > +	} else {
> > +		if (!acpi_processor_set_per_cpu(pr, device))
> > +			return 0;
> >   	}
> >     
> 
> For non-hotplug case, we still need pass the error to acpi_processor_add() so that
> cleanup corresponding 'err' tag can be done. Otherwise, we will have memory leakage.
> 
> >   	/*
> > @@ -365,8 +406,6 @@ static int acpi_processor_get_info(struct acpi_device *device)
> >    * (cpu_data(cpu)) values, like CPU feature flags, family, model, etc.
> >    * Such things have to be put in and set up by the processor driver's .probe().
> >    */
> > -static DEFINE_PER_CPU(void *, processor_device_array);
> > -
> >   static int acpi_processor_add(struct acpi_device *device,
> >   					const struct acpi_device_id *id)
> >   {
> > @@ -395,28 +434,6 @@ static int acpi_processor_add(struct acpi_device *device,
> >   	if (result) /* Processor is not physically present or unavailable */
> >   		return 0;
> >   
> > -	BUG_ON(pr->id >= nr_cpu_ids);
> > -
> > -	/*
> > -	 * Buggy BIOS check.
> > -	 * ACPI id of processors can be reported wrongly by the BIOS.
> > -	 * Don't trust it blindly
> > -	 */
> > -	if (per_cpu(processor_device_array, pr->id) != NULL &&
> > -	    per_cpu(processor_device_array, pr->id) != device) {
> > -		dev_warn(&device->dev,
> > -			"BIOS reported wrong ACPI id %d for the processor\n",
> > -			pr->id);
> > -		/* Give up, but do not abort the namespace scan. */
> > -		goto err;
> > -	}
> > -	/*
> > -	 * processor_device_array is not cleared on errors to allow buggy BIOS
> > -	 * checks.
> > -	 */
> > -	per_cpu(processor_device_array, pr->id) = device;
> > -	per_cpu(processors, pr->id) = pr;
> > -
> >   	dev = get_cpu_device(pr->id);
> >   	if (!dev) {
> >   		result = -ENODEV;
> > @@ -469,10 +486,6 @@ static void acpi_processor_remove(struct acpi_device *device)
> >   	device_release_driver(pr->dev);
> >   	acpi_unbind_one(pr->dev);
> >   
> > -	/* Clean up. */
> > -	per_cpu(processor_device_array, pr->id) = NULL;
> > -	per_cpu(processors, pr->id) = NULL;
> > -
> >   	cpu_maps_update_begin();
> >   	cpus_write_lock();
> >   
> > @@ -480,6 +493,10 @@ static void acpi_processor_remove(struct acpi_device *device)
> >   	arch_unregister_cpu(pr->id);
> >   	acpi_unmap_cpu(pr->id);
> >   
> > +	/* Clean up. */
> > +	per_cpu(processor_device_array, pr->id) = NULL;
> > +	per_cpu(processors, pr->id) = NULL;
> > +
> >   	cpus_write_unlock();
> >   	cpu_maps_update_done();
> >     
> 
> Thanks,
> Gavin
> 

Re: [PATCH v8 04/16] ACPI: processor: Move checks and availability of acpi_processor earlier

[Rafael J. Wysocki]

On Tue, Apr 30, 2024 at 11:28 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Tue, 30 Apr 2024 14:17:24 +1000
> Gavin Shan <gshan@redhat.com> wrote:
>
> > On 4/26/24 23:51, Jonathan Cameron wrote:
> > > Make the per_cpu(processors, cpu) entries available earlier so that
> > > they are available in arch_register_cpu() as ARM64 will need access
> > > to the acpi_handle to distinguish between acpi_processor_add()
> > > and earlier registration attempts (which will fail as _STA cannot
> > > be checked).
> > >
> > > Reorder the remove flow to clear this per_cpu() after
> > > arch_unregister_cpu() has completed, allowing it to be used in
> > > there as well.
> > >
> > > Note that on x86 for the CPU hotplug case, the pr->id prior to
> > > acpi_map_cpu() may be invalid. Thus the per_cpu() structures
> > > must be initialized after that call or after checking the ID
> > > is valid (not hotplug path).
> > >
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > >
> > > ---
> > > v8: On buggy bios detection when setting per_cpu structures
> > >      do not carry on.
> > >      Fix up the clearing of per cpu structures to remove unwanted
> > >      side effects and ensure an error code isn't use to reference them.
> > > ---
> > >   drivers/acpi/acpi_processor.c | 79 +++++++++++++++++++++--------------
> > >   1 file changed, 48 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> > > index ba0a6f0ac841..3b180e21f325 100644
> > > --- a/drivers/acpi/acpi_processor.c
> > > +++ b/drivers/acpi/acpi_processor.c
> > > @@ -183,8 +183,38 @@ static void __init acpi_pcc_cpufreq_init(void) {}
> > >   #endif /* CONFIG_X86 */
> > >
> > >   /* Initialization */
> > > +static DEFINE_PER_CPU(void *, processor_device_array);
> > > +
> > > +static bool acpi_processor_set_per_cpu(struct acpi_processor *pr,
> > > +                                  struct acpi_device *device)
> > > +{
> > > +   BUG_ON(pr->id >= nr_cpu_ids);
> >
> > One blank line after BUG_ON() if we need to follow original implementation.
>
> Sure unintentional - I'll put that back.
>
> >
> > > +   /*
> > > +    * Buggy BIOS check.
> > > +    * ACPI id of processors can be reported wrongly by the BIOS.
> > > +    * Don't trust it blindly
> > > +    */
> > > +   if (per_cpu(processor_device_array, pr->id) != NULL &&
> > > +       per_cpu(processor_device_array, pr->id) != device) {
> > > +           dev_warn(&device->dev,
> > > +                    "BIOS reported wrong ACPI id %d for the processor\n",
> > > +                    pr->id);
> > > +           /* Give up, but do not abort the namespace scan. */
> >
> > It depends on how the return value is handled by the caller if the namespace
> > is continued to be scanned. The caller can be acpi_processor_hotadd_init()
> > and acpi_processor_get_info() after this patch is applied. So I think this
> > specific comment need to be moved to the caller.
>
> Good point. This gets messy and was an unintended change.
>
> Previously the options were:
> 1) acpi_processor_get_info() failed for other reasons - this code was never called.
> 2) acpi_processor_get_info() succeeded without acpi_processor_hotadd_init (non hotplug)
>    this code then ran and would paper over the problem doing a bunch of cleanup under err.
> 3) acpi_processor_get_info() succeeded with acpi_processor_hotadd_init called.
>    This code then ran and would paper over the problem doing a bunch of cleanup under err.
>
> We should maintain that or argue cleanly against it.

The return value needs to be propagated to acpi_processor_add() so it
can decide what to do with it.

Now, acpi_processor_add() can only return 1 if the CPU has been
successfully registered and initialized, so it is regarded as
available (but it may not be online to start with).

Returning 0 from it may get messy, because acpi_default_enumeration()
will get called and it will attempt to create a platform device for
the CPU, so in all cases in which the CPU is not regarded as available
when acpi_processor_add() returns, it should return an error code (the
exact value doesn't matter for its caller so long as it is negative).

> This isn't helped the the fact I have no idea which cases we care about for that bios
> bug handling.  Do any of those bios's ever do hotplug?  Guess we have to try and maintain
> whatever protection this was offering.
>
> Also, the original code leaks data in some paths and I have limited idea
> of whether it is intentional or not. So to tidy the issue up that you've identified
> I'll need to try and make that code consistent first.

I agree.

> I suspect the only way to do that is going to be to duplicate the allocations we
> 'want' to leak to deal with the bios bug detection.
>
> For example acpi_processor_get_info() failing leaks pr and pr->throttling.shared_cpu_map
> before this series. After this series we need pr to leak because it's used for the detection
> via processor_device_array.
>
> I'll work through this but it's going to be tricky to tell if we get right.
> Step 1 will be closing the existing leaks and then we will have something
> consistent to build on.

Sounds good to me.

Re: [PATCH v8 04/16] ACPI: processor: Move checks and availability of acpi_processor earlier

[Jonathan Cameron]

On Tue, 30 Apr 2024 10:28:38 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Tue, 30 Apr 2024 14:17:24 +1000
> Gavin Shan <gshan@redhat.com> wrote:
> 
> > On 4/26/24 23:51, Jonathan Cameron wrote:  
> > > Make the per_cpu(processors, cpu) entries available earlier so that
> > > they are available in arch_register_cpu() as ARM64 will need access
> > > to the acpi_handle to distinguish between acpi_processor_add()
> > > and earlier registration attempts (which will fail as _STA cannot
> > > be checked).
> > > 
> > > Reorder the remove flow to clear this per_cpu() after
> > > arch_unregister_cpu() has completed, allowing it to be used in
> > > there as well.
> > > 
> > > Note that on x86 for the CPU hotplug case, the pr->id prior to
> > > acpi_map_cpu() may be invalid. Thus the per_cpu() structures
> > > must be initialized after that call or after checking the ID
> > > is valid (not hotplug path).
> > > 
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > 
> > > ---
> > > v8: On buggy bios detection when setting per_cpu structures
> > >      do not carry on.
> > >      Fix up the clearing of per cpu structures to remove unwanted
> > >      side effects and ensure an error code isn't use to reference them.
> > > ---
> > >   drivers/acpi/acpi_processor.c | 79 +++++++++++++++++++++--------------
> > >   1 file changed, 48 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> > > index ba0a6f0ac841..3b180e21f325 100644
> > > --- a/drivers/acpi/acpi_processor.c
> > > +++ b/drivers/acpi/acpi_processor.c
> > > @@ -183,8 +183,38 @@ static void __init acpi_pcc_cpufreq_init(void) {}
> > >   #endif /* CONFIG_X86 */
> > >   
> > >   /* Initialization */
> > > +static DEFINE_PER_CPU(void *, processor_device_array);
> > > +
> > > +static bool acpi_processor_set_per_cpu(struct acpi_processor *pr,
> > > +				       struct acpi_device *device)
> > > +{
> > > +	BUG_ON(pr->id >= nr_cpu_ids);    
> > 
> > One blank line after BUG_ON() if we need to follow original implementation.  
> 
> Sure unintentional - I'll put that back.
> 
> >   
> > > +	/*
> > > +	 * Buggy BIOS check.
> > > +	 * ACPI id of processors can be reported wrongly by the BIOS.
> > > +	 * Don't trust it blindly
> > > +	 */
> > > +	if (per_cpu(processor_device_array, pr->id) != NULL &&
> > > +	    per_cpu(processor_device_array, pr->id) != device) {
> > > +		dev_warn(&device->dev,
> > > +			 "BIOS reported wrong ACPI id %d for the processor\n",
> > > +			 pr->id);
> > > +		/* Give up, but do not abort the namespace scan. */    
> > 
> > It depends on how the return value is handled by the caller if the namespace
> > is continued to be scanned. The caller can be acpi_processor_hotadd_init()
> > and acpi_processor_get_info() after this patch is applied. So I think this
> > specific comment need to be moved to the caller.  
> 
> Good point. This gets messy and was an unintended change.
> 
> Previously the options were:
> 1) acpi_processor_get_info() failed for other reasons - this code was never called.
> 2) acpi_processor_get_info() succeeded without acpi_processor_hotadd_init (non hotplug)
>    this code then ran and would paper over the problem doing a bunch of cleanup under err.
> 3) acpi_processor_get_info() succeeded with acpi_processor_hotadd_init called.
>    This code then ran and would paper over the problem doing a bunch of cleanup under err.
> 
> We should maintain that or argue cleanly against it.
> 
> This isn't helped the the fact I have no idea which cases we care about for that bios
> bug handling.  Do any of those bios's ever do hotplug?  Guess we have to try and maintain
> whatever protection this was offering.
> 
> Also, the original code leaks data in some paths and I have limited idea
> of whether it is intentional or not. So to tidy the issue up that you've identified
> I'll need to try and make that code consistent first.
> 
> I suspect the only way to do that is going to be to duplicate the allocations we
> 'want' to leak to deal with the bios bug detection.
> 
> For example acpi_processor_get_info() failing leaks pr and pr->throttling.shared_cpu_map
> before this series. After this series we need pr to leak because it's used for the detection
> via processor_device_array.
> 
> I'll work through this but it's going to be tricky to tell if we get right.
> Step 1 will be closing the existing leaks and then we will have something
> consistent to build on.
> 
I 'think' that fixing the original leaks makes this all much more straight forward.
That return 0 for acpi_processor_get_info() never made sense as far as I can tell.
The pr isn't used after this point.

What about something along lines of.

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index 161c95c9d60a..97cff4492304 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -392,8 +392,10 @@ static int acpi_processor_add(struct acpi_device *device,
        device->driver_data = pr;

        result = acpi_processor_get_info(device);
-       if (result) /* Processor is not physically present or unavailable */
-               return 0;
+       if (result) { /* Processor is not physically present or unavailable */
+               result = 0;
+               goto err_free_throttling_mask;
+       }

        BUG_ON(pr->id >= nr_cpu_ids);

@@ -408,7 +410,7 @@ static int acpi_processor_add(struct acpi_device *device,
                        "BIOS reported wrong ACPI id %d for the processor\n",
                        pr->id);
                /* Give up, but do not abort the namespace scan. */
-               goto err;
+               goto err_clear_driver_data;
        }
        /*
         * processor_device_array is not cleared on errors to allow buggy BIOS
@@ -420,12 +422,12 @@ static int acpi_processor_add(struct acpi_device *device,
        dev = get_cpu_device(pr->id);
        if (!dev) {
                result = -ENODEV;
-               goto err;
+               goto err_clear_per_cpu;
        }

        result = acpi_bind_one(dev, device);
        if (result)
-               goto err;
+               goto err_clear_per_cpu;

        pr->dev = dev;

@@ -436,10 +438,12 @@ static int acpi_processor_add(struct acpi_device *device,
        dev_err(dev, "Processor driver could not be attached\n");
        acpi_unbind_one(dev);

- err:
-       free_cpumask_var(pr->throttling.shared_cpu_map);
-       device->driver_data = NULL;
+ err_clear_per_cpu:
        per_cpu(processors, pr->id) = NULL;
+ err_clear_driver_data:
+       device->driver_data = NULL;
+ err_free_throttling_mask:
+       free_cpumask_var(pr->throttling.shared_cpu_map);
  err_free_pr:
        kfree(pr);
        return result;

Then the diff on this patch is simply:

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index 3c49eae1e943..3b75f5aeb7ab 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -200,7 +200,6 @@ static bool acpi_processor_set_per_cpu(struct acpi_processor *pr,
                dev_warn(&device->dev,
                         "BIOS reported wrong ACPI id %d for the processor\n",
                         pr->id);
-               /* Give up, but do not abort the namespace scan. */
                return false;
        }
        /*
@@ -230,13 +229,14 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr,
                goto out;

        if (!acpi_processor_set_per_cpu(pr, device)) {
+               ret = -EINVAL;
                acpi_unmap_cpu(pr->id);
                goto out;
        }

        ret = arch_register_cpu(pr->id);
        if (ret) {
-               /* Leave the processor device array in place to detect buggy bios */
+x              /* Leave the processor device array in place to detect buggy bios */
                per_cpu(processors, pr->id) = NULL;
                acpi_unmap_cpu(pr->id);
                goto out;
@@ -262,7 +262,7 @@ static inline int acpi_processor_hotadd_init(struct acpi_processor *pr,
 }
 #endif /* CONFIG_ACPI_HOTPLUG_CPU */

-static int acpi_processor_get_info(struct acpi_device *device)
+static int acpi_processor_get_info(struct acpi_device *device, bool bios_bug)
 {
        union acpi_object object = { 0 };
        struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
@@ -361,7 +361,7 @@ static int acpi_processor_get_info(struct acpi_device *device)
                        return ret;
        } else {
                if (!acpi_processor_set_per_cpu(pr, device))
-                       return 0;
+                       return -EINVAL;
        }

        /*
> > 
> > Besides, it seems acpi_processor_set_per_cpu() isn't properly called and
> > memory leakage can happen. More details are given below.
> >   
> > > +		return false;
> > > +	}
> > > +	/*
> > > +	 * processor_device_array is not cleared on errors to allow buggy BIOS
> > > +	 * checks.
> > > +	 */
> > > +	per_cpu(processor_device_array, pr->id) = device;
> > > +	per_cpu(processors, pr->id) = pr;
> > > +
> > > +	return true;
> > > +}
> > > +
> > >   #ifdef CONFIG_ACPI_HOTPLUG_CPU
> > > -static int acpi_processor_hotadd_init(struct acpi_processor *pr)
> > > +static int acpi_processor_hotadd_init(struct acpi_processor *pr,
> > > +				      struct acpi_device *device)
> > >   {
> > >   	int ret;
> > >   
> > > @@ -198,8 +228,15 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
> > >   	if (ret)
> > >   		goto out;
> > >   
> > > +	if (!acpi_processor_set_per_cpu(pr, device)) {
> > > +		acpi_unmap_cpu(pr->id);
> > > +		goto out;
> > > +	}
> > > +    
> > 
> > With the 'goto out', zero is returned from acpi_processor_hotadd_init() to acpi_processor_get_info().

Indeed a bug :(

> > The zero return value is carried from acpi_map_cpu() in acpi_processor_hotadd_init(). If I'm correct,
> > we need return errno from acpi_processor_get_info() to acpi_processor_add() so that cleanup can be
> > done. For example, the cleanup corresponding to the 'err' tag can be done in acpi_processor_add().
> > Otherwise, we will have memory leakage.

The confusion here was that previously acpi_processor_add() was missing error cleanup for
acpi_processor_get_info().  With that in place I think it's all much simpler.

Thanks for your eagle eyes!

Jonathan


> >   
> > >   	ret = arch_register_cpu(pr->id);
> > >   	if (ret) {
> > > +		/* Leave the processor device array in place to detect buggy bios */
> > > +		per_cpu(processors, pr->id) = NULL;
> > >   		acpi_unmap_cpu(pr->id);
> > >   		goto out;
> > >   	}
> > > @@ -217,7 +254,8 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
> > >   	return ret;
> > >   }
> > >   #else
> > > -static inline int acpi_processor_hotadd_init(struct acpi_processor *pr)
> > > +static inline int acpi_processor_hotadd_init(struct acpi_processor *pr,
> > > +					     struct acpi_device *device)
> > >   {
> > >   	return -ENODEV;
> > >   }
> > > @@ -316,10 +354,13 @@ static int acpi_processor_get_info(struct acpi_device *device)
> > >   	 *  because cpuid <-> apicid mapping is persistent now.
> > >   	 */
> > >   	if (invalid_logical_cpuid(pr->id) || !cpu_present(pr->id)) {
> > > -		int ret = acpi_processor_hotadd_init(pr);
> > > +		int ret = acpi_processor_hotadd_init(pr, device);
> > >   
> > >   		if (ret)
> > >   			return ret;
> > > +	} else {
> > > +		if (!acpi_processor_set_per_cpu(pr, device))
> > > +			return 0;
> > >   	}
> > >       
> > 
> > For non-hotplug case, we still need pass the error to acpi_processor_add() so that
> > cleanup corresponding 'err' tag can be done. Otherwise, we will have memory leakage.
> >   
> > >   	/*
> > > @@ -365,8 +406,6 @@ static int acpi_processor_get_info(struct acpi_device *device)
> > >    * (cpu_data(cpu)) values, like CPU feature flags, family, model, etc.
> > >    * Such things have to be put in and set up by the processor driver's .probe().
> > >    */
> > > -static DEFINE_PER_CPU(void *, processor_device_array);
> > > -
> > >   static int acpi_processor_add(struct acpi_device *device,
> > >   					const struct acpi_device_id *id)
> > >   {
> > > @@ -395,28 +434,6 @@ static int acpi_processor_add(struct acpi_device *device,
> > >   	if (result) /* Processor is not physically present or unavailable */
> > >   		return 0;
> > >   
> > > -	BUG_ON(pr->id >= nr_cpu_ids);
> > > -
> > > -	/*
> > > -	 * Buggy BIOS check.
> > > -	 * ACPI id of processors can be reported wrongly by the BIOS.
> > > -	 * Don't trust it blindly
> > > -	 */
> > > -	if (per_cpu(processor_device_array, pr->id) != NULL &&
> > > -	    per_cpu(processor_device_array, pr->id) != device) {
> > > -		dev_warn(&device->dev,
> > > -			"BIOS reported wrong ACPI id %d for the processor\n",
> > > -			pr->id);
> > > -		/* Give up, but do not abort the namespace scan. */
> > > -		goto err;
> > > -	}
> > > -	/*
> > > -	 * processor_device_array is not cleared on errors to allow buggy BIOS
> > > -	 * checks.
> > > -	 */
> > > -	per_cpu(processor_device_array, pr->id) = device;
> > > -	per_cpu(processors, pr->id) = pr;
> > > -
> > >   	dev = get_cpu_device(pr->id);
> > >   	if (!dev) {
> > >   		result = -ENODEV;
> > > @@ -469,10 +486,6 @@ static void acpi_processor_remove(struct acpi_device *device)
> > >   	device_release_driver(pr->dev);
> > >   	acpi_unbind_one(pr->dev);
> > >   
> > > -	/* Clean up. */
> > > -	per_cpu(processor_device_array, pr->id) = NULL;
> > > -	per_cpu(processors, pr->id) = NULL;
> > > -
> > >   	cpu_maps_update_begin();
> > >   	cpus_write_lock();
> > >   
> > > @@ -480,6 +493,10 @@ static void acpi_processor_remove(struct acpi_device *device)
> > >   	arch_unregister_cpu(pr->id);
> > >   	acpi_unmap_cpu(pr->id);
> > >   
> > > +	/* Clean up. */
> > > +	per_cpu(processor_device_array, pr->id) = NULL;
> > > +	per_cpu(processors, pr->id) = NULL;
> > > +
> > >   	cpus_write_unlock();
> > >   	cpu_maps_update_done();
> > >       
> > 
> > Thanks,
> > Gavin
> >   
> 

Re: [PATCH v8 04/16] ACPI: processor: Move checks and availability of acpi_processor earlier

[Rafael J. Wysocki]

On Tue, Apr 30, 2024 at 12:13 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Tue, 30 Apr 2024 10:28:38 +0100
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
>
> > On Tue, 30 Apr 2024 14:17:24 +1000
> > Gavin Shan <gshan@redhat.com> wrote:
> >
> > > On 4/26/24 23:51, Jonathan Cameron wrote:
> > > > Make the per_cpu(processors, cpu) entries available earlier so that
> > > > they are available in arch_register_cpu() as ARM64 will need access
> > > > to the acpi_handle to distinguish between acpi_processor_add()
> > > > and earlier registration attempts (which will fail as _STA cannot
> > > > be checked).
> > > >
> > > > Reorder the remove flow to clear this per_cpu() after
> > > > arch_unregister_cpu() has completed, allowing it to be used in
> > > > there as well.
> > > >
> > > > Note that on x86 for the CPU hotplug case, the pr->id prior to
> > > > acpi_map_cpu() may be invalid. Thus the per_cpu() structures
> > > > must be initialized after that call or after checking the ID
> > > > is valid (not hotplug path).
> > > >
> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > >
> > > > ---
> > > > v8: On buggy bios detection when setting per_cpu structures
> > > >      do not carry on.
> > > >      Fix up the clearing of per cpu structures to remove unwanted
> > > >      side effects and ensure an error code isn't use to reference them.
> > > > ---
> > > >   drivers/acpi/acpi_processor.c | 79 +++++++++++++++++++++--------------
> > > >   1 file changed, 48 insertions(+), 31 deletions(-)
> > > >
> > > > diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> > > > index ba0a6f0ac841..3b180e21f325 100644
> > > > --- a/drivers/acpi/acpi_processor.c
> > > > +++ b/drivers/acpi/acpi_processor.c
> > > > @@ -183,8 +183,38 @@ static void __init acpi_pcc_cpufreq_init(void) {}
> > > >   #endif /* CONFIG_X86 */
> > > >
> > > >   /* Initialization */
> > > > +static DEFINE_PER_CPU(void *, processor_device_array);
> > > > +
> > > > +static bool acpi_processor_set_per_cpu(struct acpi_processor *pr,
> > > > +                                struct acpi_device *device)
> > > > +{
> > > > + BUG_ON(pr->id >= nr_cpu_ids);
> > >
> > > One blank line after BUG_ON() if we need to follow original implementation.
> >
> > Sure unintentional - I'll put that back.
> >
> > >
> > > > + /*
> > > > +  * Buggy BIOS check.
> > > > +  * ACPI id of processors can be reported wrongly by the BIOS.
> > > > +  * Don't trust it blindly
> > > > +  */
> > > > + if (per_cpu(processor_device_array, pr->id) != NULL &&
> > > > +     per_cpu(processor_device_array, pr->id) != device) {
> > > > +         dev_warn(&device->dev,
> > > > +                  "BIOS reported wrong ACPI id %d for the processor\n",
> > > > +                  pr->id);
> > > > +         /* Give up, but do not abort the namespace scan. */
> > >
> > > It depends on how the return value is handled by the caller if the namespace
> > > is continued to be scanned. The caller can be acpi_processor_hotadd_init()
> > > and acpi_processor_get_info() after this patch is applied. So I think this
> > > specific comment need to be moved to the caller.
> >
> > Good point. This gets messy and was an unintended change.
> >
> > Previously the options were:
> > 1) acpi_processor_get_info() failed for other reasons - this code was never called.
> > 2) acpi_processor_get_info() succeeded without acpi_processor_hotadd_init (non hotplug)
> >    this code then ran and would paper over the problem doing a bunch of cleanup under err.
> > 3) acpi_processor_get_info() succeeded with acpi_processor_hotadd_init called.
> >    This code then ran and would paper over the problem doing a bunch of cleanup under err.
> >
> > We should maintain that or argue cleanly against it.
> >
> > This isn't helped the the fact I have no idea which cases we care about for that bios
> > bug handling.  Do any of those bios's ever do hotplug?  Guess we have to try and maintain
> > whatever protection this was offering.
> >
> > Also, the original code leaks data in some paths and I have limited idea
> > of whether it is intentional or not. So to tidy the issue up that you've identified
> > I'll need to try and make that code consistent first.
> >
> > I suspect the only way to do that is going to be to duplicate the allocations we
> > 'want' to leak to deal with the bios bug detection.
> >
> > For example acpi_processor_get_info() failing leaks pr and pr->throttling.shared_cpu_map
> > before this series. After this series we need pr to leak because it's used for the detection
> > via processor_device_array.
> >
> > I'll work through this but it's going to be tricky to tell if we get right.
> > Step 1 will be closing the existing leaks and then we will have something
> > consistent to build on.
> >
> I 'think' that fixing the original leaks makes this all much more straight forward.
> That return 0 for acpi_processor_get_info() never made sense as far as I can tell.
> The pr isn't used after this point.
>
> What about something along lines of.
>
> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> index 161c95c9d60a..97cff4492304 100644
> --- a/drivers/acpi/acpi_processor.c
> +++ b/drivers/acpi/acpi_processor.c
> @@ -392,8 +392,10 @@ static int acpi_processor_add(struct acpi_device *device,
>         device->driver_data = pr;
>
>         result = acpi_processor_get_info(device);
> -       if (result) /* Processor is not physically present or unavailable */
> -               return 0;
> +       if (result) { /* Processor is not physically present or unavailable */
> +               result = 0;

As per my previous message (just sent) this should be an error code,
as returning 0 from acpi_processor_add() is generally problematic.

> +               goto err_free_throttling_mask;
> +       }
>
>         BUG_ON(pr->id >= nr_cpu_ids);
>

Re: [PATCH v8 04/16] ACPI: processor: Move checks and availability of acpi_processor earlier

[Jonathan Cameron]

On Tue, 30 Apr 2024 12:17:38 +0200
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> On Tue, Apr 30, 2024 at 12:13 PM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Tue, 30 Apr 2024 10:28:38 +0100
> > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> >  
> > > On Tue, 30 Apr 2024 14:17:24 +1000
> > > Gavin Shan <gshan@redhat.com> wrote:
> > >  
> > > > On 4/26/24 23:51, Jonathan Cameron wrote:  
> > > > > Make the per_cpu(processors, cpu) entries available earlier so that
> > > > > they are available in arch_register_cpu() as ARM64 will need access
> > > > > to the acpi_handle to distinguish between acpi_processor_add()
> > > > > and earlier registration attempts (which will fail as _STA cannot
> > > > > be checked).
> > > > >
> > > > > Reorder the remove flow to clear this per_cpu() after
> > > > > arch_unregister_cpu() has completed, allowing it to be used in
> > > > > there as well.
> > > > >
> > > > > Note that on x86 for the CPU hotplug case, the pr->id prior to
> > > > > acpi_map_cpu() may be invalid. Thus the per_cpu() structures
> > > > > must be initialized after that call or after checking the ID
> > > > > is valid (not hotplug path).
> > > > >
> > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > >
> > > > > ---
> > > > > v8: On buggy bios detection when setting per_cpu structures
> > > > >      do not carry on.
> > > > >      Fix up the clearing of per cpu structures to remove unwanted
> > > > >      side effects and ensure an error code isn't use to reference them.
> > > > > ---
> > > > >   drivers/acpi/acpi_processor.c | 79 +++++++++++++++++++++--------------
> > > > >   1 file changed, 48 insertions(+), 31 deletions(-)
> > > > >
> > > > > diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> > > > > index ba0a6f0ac841..3b180e21f325 100644
> > > > > --- a/drivers/acpi/acpi_processor.c
> > > > > +++ b/drivers/acpi/acpi_processor.c
> > > > > @@ -183,8 +183,38 @@ static void __init acpi_pcc_cpufreq_init(void) {}
> > > > >   #endif /* CONFIG_X86 */
> > > > >
> > > > >   /* Initialization */
> > > > > +static DEFINE_PER_CPU(void *, processor_device_array);
> > > > > +
> > > > > +static bool acpi_processor_set_per_cpu(struct acpi_processor *pr,
> > > > > +                                struct acpi_device *device)
> > > > > +{
> > > > > + BUG_ON(pr->id >= nr_cpu_ids);  
> > > >
> > > > One blank line after BUG_ON() if we need to follow original implementation.  
> > >
> > > Sure unintentional - I'll put that back.
> > >  
> > > >  
> > > > > + /*
> > > > > +  * Buggy BIOS check.
> > > > > +  * ACPI id of processors can be reported wrongly by the BIOS.
> > > > > +  * Don't trust it blindly
> > > > > +  */
> > > > > + if (per_cpu(processor_device_array, pr->id) != NULL &&
> > > > > +     per_cpu(processor_device_array, pr->id) != device) {
> > > > > +         dev_warn(&device->dev,
> > > > > +                  "BIOS reported wrong ACPI id %d for the processor\n",
> > > > > +                  pr->id);
> > > > > +         /* Give up, but do not abort the namespace scan. */  
> > > >
> > > > It depends on how the return value is handled by the caller if the namespace
> > > > is continued to be scanned. The caller can be acpi_processor_hotadd_init()
> > > > and acpi_processor_get_info() after this patch is applied. So I think this
> > > > specific comment need to be moved to the caller.  
> > >
> > > Good point. This gets messy and was an unintended change.
> > >
> > > Previously the options were:
> > > 1) acpi_processor_get_info() failed for other reasons - this code was never called.
> > > 2) acpi_processor_get_info() succeeded without acpi_processor_hotadd_init (non hotplug)
> > >    this code then ran and would paper over the problem doing a bunch of cleanup under err.
> > > 3) acpi_processor_get_info() succeeded with acpi_processor_hotadd_init called.
> > >    This code then ran and would paper over the problem doing a bunch of cleanup under err.
> > >
> > > We should maintain that or argue cleanly against it.
> > >
> > > This isn't helped the the fact I have no idea which cases we care about for that bios
> > > bug handling.  Do any of those bios's ever do hotplug?  Guess we have to try and maintain
> > > whatever protection this was offering.
> > >
> > > Also, the original code leaks data in some paths and I have limited idea
> > > of whether it is intentional or not. So to tidy the issue up that you've identified
> > > I'll need to try and make that code consistent first.
> > >
> > > I suspect the only way to do that is going to be to duplicate the allocations we
> > > 'want' to leak to deal with the bios bug detection.
> > >
> > > For example acpi_processor_get_info() failing leaks pr and pr->throttling.shared_cpu_map
> > > before this series. After this series we need pr to leak because it's used for the detection
> > > via processor_device_array.
> > >
> > > I'll work through this but it's going to be tricky to tell if we get right.
> > > Step 1 will be closing the existing leaks and then we will have something
> > > consistent to build on.
> > >  
> > I 'think' that fixing the original leaks makes this all much more straight forward.
> > That return 0 for acpi_processor_get_info() never made sense as far as I can tell.
> > The pr isn't used after this point.
> >
> > What about something along lines of.
> >
> > diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> > index 161c95c9d60a..97cff4492304 100644
> > --- a/drivers/acpi/acpi_processor.c
> > +++ b/drivers/acpi/acpi_processor.c
> > @@ -392,8 +392,10 @@ static int acpi_processor_add(struct acpi_device *device,
> >         device->driver_data = pr;
> >
> >         result = acpi_processor_get_info(device);
> > -       if (result) /* Processor is not physically present or unavailable */
> > -               return 0;
> > +       if (result) { /* Processor is not physically present or unavailable */
> > +               result = 0;  
> 
> As per my previous message (just sent) this should be an error code,
> as returning 0 from acpi_processor_add() is generally problematic.
Ok. I'll switch to that, but as a separate precusor patch. Independent of
the memory leak fixes.

Jonathan

> 
> > +               goto err_free_throttling_mask;
> > +       }
> >
> >         BUG_ON(pr->id >= nr_cpu_ids);
> >  

Re: [PATCH v8 04/16] ACPI: processor: Move checks and availability of acpi_processor earlier

[Rafael J. Wysocki]

On Tue, Apr 30, 2024 at 12:45 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Tue, 30 Apr 2024 12:17:38 +0200
> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
>
> > On Tue, Apr 30, 2024 at 12:13 PM Jonathan Cameron
> > <Jonathan.Cameron@huawei.com> wrote:
> > >
> > > On Tue, 30 Apr 2024 10:28:38 +0100
> > > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> > >
> > > > On Tue, 30 Apr 2024 14:17:24 +1000
> > > > Gavin Shan <gshan@redhat.com> wrote:
> > > >
> > > > > On 4/26/24 23:51, Jonathan Cameron wrote:
> > > > > > Make the per_cpu(processors, cpu) entries available earlier so that
> > > > > > they are available in arch_register_cpu() as ARM64 will need access
> > > > > > to the acpi_handle to distinguish between acpi_processor_add()
> > > > > > and earlier registration attempts (which will fail as _STA cannot
> > > > > > be checked).
> > > > > >
> > > > > > Reorder the remove flow to clear this per_cpu() after
> > > > > > arch_unregister_cpu() has completed, allowing it to be used in
> > > > > > there as well.
> > > > > >
> > > > > > Note that on x86 for the CPU hotplug case, the pr->id prior to
> > > > > > acpi_map_cpu() may be invalid. Thus the per_cpu() structures
> > > > > > must be initialized after that call or after checking the ID
> > > > > > is valid (not hotplug path).
> > > > > >
> > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > >
> > > > > > ---
> > > > > > v8: On buggy bios detection when setting per_cpu structures
> > > > > >      do not carry on.
> > > > > >      Fix up the clearing of per cpu structures to remove unwanted
> > > > > >      side effects and ensure an error code isn't use to reference them.
> > > > > > ---
> > > > > >   drivers/acpi/acpi_processor.c | 79 +++++++++++++++++++++--------------
> > > > > >   1 file changed, 48 insertions(+), 31 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> > > > > > index ba0a6f0ac841..3b180e21f325 100644
> > > > > > --- a/drivers/acpi/acpi_processor.c
> > > > > > +++ b/drivers/acpi/acpi_processor.c
> > > > > > @@ -183,8 +183,38 @@ static void __init acpi_pcc_cpufreq_init(void) {}
> > > > > >   #endif /* CONFIG_X86 */
> > > > > >
> > > > > >   /* Initialization */
> > > > > > +static DEFINE_PER_CPU(void *, processor_device_array);
> > > > > > +
> > > > > > +static bool acpi_processor_set_per_cpu(struct acpi_processor *pr,
> > > > > > +                                struct acpi_device *device)
> > > > > > +{
> > > > > > + BUG_ON(pr->id >= nr_cpu_ids);
> > > > >
> > > > > One blank line after BUG_ON() if we need to follow original implementation.
> > > >
> > > > Sure unintentional - I'll put that back.
> > > >
> > > > >
> > > > > > + /*
> > > > > > +  * Buggy BIOS check.
> > > > > > +  * ACPI id of processors can be reported wrongly by the BIOS.
> > > > > > +  * Don't trust it blindly
> > > > > > +  */
> > > > > > + if (per_cpu(processor_device_array, pr->id) != NULL &&
> > > > > > +     per_cpu(processor_device_array, pr->id) != device) {
> > > > > > +         dev_warn(&device->dev,
> > > > > > +                  "BIOS reported wrong ACPI id %d for the processor\n",
> > > > > > +                  pr->id);
> > > > > > +         /* Give up, but do not abort the namespace scan. */
> > > > >
> > > > > It depends on how the return value is handled by the caller if the namespace
> > > > > is continued to be scanned. The caller can be acpi_processor_hotadd_init()
> > > > > and acpi_processor_get_info() after this patch is applied. So I think this
> > > > > specific comment need to be moved to the caller.
> > > >
> > > > Good point. This gets messy and was an unintended change.
> > > >
> > > > Previously the options were:
> > > > 1) acpi_processor_get_info() failed for other reasons - this code was never called.
> > > > 2) acpi_processor_get_info() succeeded without acpi_processor_hotadd_init (non hotplug)
> > > >    this code then ran and would paper over the problem doing a bunch of cleanup under err.
> > > > 3) acpi_processor_get_info() succeeded with acpi_processor_hotadd_init called.
> > > >    This code then ran and would paper over the problem doing a bunch of cleanup under err.
> > > >
> > > > We should maintain that or argue cleanly against it.
> > > >
> > > > This isn't helped the the fact I have no idea which cases we care about for that bios
> > > > bug handling.  Do any of those bios's ever do hotplug?  Guess we have to try and maintain
> > > > whatever protection this was offering.
> > > >
> > > > Also, the original code leaks data in some paths and I have limited idea
> > > > of whether it is intentional or not. So to tidy the issue up that you've identified
> > > > I'll need to try and make that code consistent first.
> > > >
> > > > I suspect the only way to do that is going to be to duplicate the allocations we
> > > > 'want' to leak to deal with the bios bug detection.
> > > >
> > > > For example acpi_processor_get_info() failing leaks pr and pr->throttling.shared_cpu_map
> > > > before this series. After this series we need pr to leak because it's used for the detection
> > > > via processor_device_array.
> > > >
> > > > I'll work through this but it's going to be tricky to tell if we get right.
> > > > Step 1 will be closing the existing leaks and then we will have something
> > > > consistent to build on.
> > > >
> > > I 'think' that fixing the original leaks makes this all much more straight forward.
> > > That return 0 for acpi_processor_get_info() never made sense as far as I can tell.
> > > The pr isn't used after this point.
> > >
> > > What about something along lines of.
> > >
> > > diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> > > index 161c95c9d60a..97cff4492304 100644
> > > --- a/drivers/acpi/acpi_processor.c
> > > +++ b/drivers/acpi/acpi_processor.c
> > > @@ -392,8 +392,10 @@ static int acpi_processor_add(struct acpi_device *device,
> > >         device->driver_data = pr;
> > >
> > >         result = acpi_processor_get_info(device);
> > > -       if (result) /* Processor is not physically present or unavailable */
> > > -               return 0;
> > > +       if (result) { /* Processor is not physically present or unavailable */
> > > +               result = 0;
> >
> > As per my previous message (just sent) this should be an error code,
> > as returning 0 from acpi_processor_add() is generally problematic.
> Ok. I'll switch to that, but as a separate precusor patch. Independent of
> the memory leak fixes.

Sure, thank you!

Re: [PATCH v8 05/16] ACPI: processor: Add acpi_get_processor_handle() helper

[Jonathan Cameron]

On Tue, 30 Apr 2024 14:26:06 +1000
Gavin Shan <gshan@redhat.com> wrote:

> On 4/26/24 23:51, Jonathan Cameron wrote:
> > If CONFIG_ACPI_PROCESSOR provide a helper to retrieve the
> > acpi_handle for a given CPU allowing access to methods
> > in DSDT.
> > 
> > Tested-by: Miguel Luis <miguel.luis@oracle.com>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > ---
> > v8: Code simplification suggested by Rafael.
> >      Fixup ;; spotted by Gavin
> > ---
> >   drivers/acpi/acpi_processor.c | 11 +++++++++++
> >   include/linux/acpi.h          |  7 +++++++
> >   2 files changed, 18 insertions(+)
> >   
> 
> Reviewed-by: Gavin Shan <gshan@redhat.com>
> 
Thanks,
> > diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> > index 3b180e21f325..ecc2721fecae 100644
> > --- a/drivers/acpi/acpi_processor.c
> > +++ b/drivers/acpi/acpi_processor.c
> > @@ -35,6 +35,17 @@ EXPORT_PER_CPU_SYMBOL(processors);
> >   struct acpi_processor_errata errata __read_mostly;
> >   EXPORT_SYMBOL_GPL(errata);
> >   
> > +acpi_handle acpi_get_processor_handle(int cpu)
> > +{
> > +	struct acpi_processor *pr;
> > +
> > +	pr = per_cpu(processors, cpu);
> > +	if (pr)
> > +		return pr->handle;
> > +
> > +	return NULL;
> > +}
> > +  
> 
> Maybe it's worthy to have more check here, something like below.
> However, it's also fine without the extra check.

We could harden this, but for now the call sites are only
int arch_(un)register_cpu() so if we get there with either
of these failing something went very wrong.

Maybe if it gets used more widely this defense would be wise.

Jonathan

> 
> 	if (cpu >= nr_cpu_ids || !cpu_possible(cpu))
> 		return NULL;
> 
> >   static int acpi_processor_errata_piix4(struct pci_dev *dev)
> >   {
> >   	u8 value1 = 0;
> > diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> > index 34829f2c517a..9844a3f9c4e5 100644
> > --- a/include/linux/acpi.h
> > +++ b/include/linux/acpi.h
> > @@ -309,6 +309,8 @@ int acpi_map_cpu(acpi_handle handle, phys_cpuid_t physid, u32 acpi_id,
> >   int acpi_unmap_cpu(int cpu);
> >   #endif /* CONFIG_ACPI_HOTPLUG_CPU */
> >   
> > +acpi_handle acpi_get_processor_handle(int cpu);
> > +
> >   #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC
> >   int acpi_get_ioapic_id(acpi_handle handle, u32 gsi_base, u64 *phys_addr);
> >   #endif
> > @@ -1077,6 +1079,11 @@ static inline bool acpi_sleep_state_supported(u8 sleep_state)
> >   	return false;
> >   }
> >   
> > +static inline acpi_handle acpi_get_processor_handle(int cpu)
> > +{
> > +	return NULL;
> > +}
> > +
> >   #endif	/* !CONFIG_ACPI */
> >   
> >   extern void arch_post_acpi_subsys_init(void);  
> 
> Thanks,
> Gavin
> 

Re: [PATCH v8 11/16] irqchip/gic-v3: Add support for ACPI's disabled but 'online capable' CPUs

[Jonathan Cameron]

On Mon, 29 Apr 2024 10:21:31 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Sun, 28 Apr 2024 12:28:03 +0100
> Marc Zyngier <maz@kernel.org> wrote:
> 
> > On Fri, 26 Apr 2024 19:28:58 +0100,
> > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:  
> > > 
> > > 
> > > I'll not send a formal v9 until early next week, so here is the current state
> > > if you have time to take another look before then.    
> > 
> > Don't bother resending this on my account -- you only sent it on
> > Friday and there hasn't been much response to it yet. There is still a
> > problem (see below), but looks otherwise OK.
> > 
> > [...]
> >   
> > > @@ -2363,11 +2381,25 @@ gic_acpi_parse_madt_gicc(union acpi_subtable_headers *header,
> > >  				(struct acpi_madt_generic_interrupt *)header;
> > >  	u32 reg = readl_relaxed(acpi_data.dist_base + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
> > >  	u32 size = reg == GIC_PIDR2_ARCH_GICv4 ? SZ_64K * 4 : SZ_64K * 2;
> > > +	int cpu = get_cpu_for_acpi_id(gicc->uid);    
> > 
> > I already commented that get_cpu_for_acpi_id() can...  
> 
> Indeed sorry - I blame Friday syndrome for me failing to address that.
> 
> >   
> > >  	void __iomem *redist_base;
> > >  
> > > -	if (!acpi_gicc_is_usable(gicc))
> > > +	/* Neither enabled or online capable means it doesn't exist, skip it */
> > > +	if (!(gicc->flags & (ACPI_MADT_ENABLED | ACPI_MADT_GICC_ONLINE_CAPABLE)))
> > >  		return 0;
> > >  
> > > +	/*
> > > +	 * Capable but disabled CPUs can be brought online later. What about
> > > +	 * the redistributor? ACPI doesn't want to say!
> > > +	 * Virtual hotplug systems can use the MADT's "always-on" GICR entries.
> > > +	 * Otherwise, prevent such CPUs from being brought online.
> > > +	 */
> > > +	if (!(gicc->flags & ACPI_MADT_ENABLED)) {
> > > +		pr_warn("CPU %u's redistributor is inaccessible: this CPU can't be brought online\n", cpu);
> > > +		cpumask_set_cpu(cpu, &broken_rdists);    
> > 
> > ... return -EINVAL, and then be passed to cpumask_set_cpu(), with
> > interesting effects. It shouldn't happen, but I trust anything that
> > comes from firmware tables as much as I trust a campaigning
> > politician's promises. This should really result in the RD being
> > considered unusable, but without affecting any CPU (there is no valid
> > CPU the first place).
> > 
> > Another question is what get_cpu_for acpi_id() returns for a disabled
> > CPU. A valid CPU number? Or -EINVAL?  
> It's a match function that works by iterating over 0 to nr_cpu_ids and
> 
> if (uid == get_acpi_id_for_cpu(cpu))
> 
> So the question become does get_acpi_id_for_cpu() return a valid CPU
> number for a disabled CPU.
> 
> That uses acpi_cpu_get_madt_gicc(cpu)->uid so this all gets a bit circular.
> That looks it up via cpu_madt_gicc[cpu] which after the proposed updated
> patch is set if enabled or online capable.  There are however a few other
> error checks in acpi_map_gic_cpu_interface() that could lead to it
> not being set (MPIDR validity checks). I suspect all of these end up being
> fatal elsewhere which is why this hasn't blown up before.
> 
> If any of those cases are possible we could get a null pointer
> dereference.
> 
> Easy to harden this case via the following (which will leave us with
> -EINVAL.  There are other call sites that might trip over this.
> I'm inclined to harden them as a separate issue though so as not
> to get in the way of this patch set.
> 
> 
> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index bc9a6656fc0c..a407f9cd549e 100644
> --- a/arch/arm64/include/asm/acpi.h
> +++ b/arch/arm64/include/asm/acpi.h
> @@ -124,7 +124,8 @@ static inline int get_cpu_for_acpi_id(u32 uid)
>         int cpu;
> 
>         for (cpu = 0; cpu < nr_cpu_ids; cpu++)
> -               if (uid == get_acpi_id_for_cpu(cpu))
> +               if (acpi_cpu_get_madt_gicc(cpu) &&
> +                   uid == get_acpi_id_for_cpu(cpu))
>                         return cpu;
> 
>         return -EINVAL;
> 
> I'll spin an additional patch to make that change after testing I haven't
> messed it up.
> 
> At the call site in gic_acpi_parse_madt_gicc() I'm not sure we can do better
> than just skipping setting broken_rdists. I'll also pull the declaration of
> that cpu variable down into this condition so it's more obvious we only
> care about it in this error path.

Just for the record, for my deliberately broken test case it seems that it returns
a valid CPU ID anyway. That's what I'd expect given acpi_parse_and_init_cpus()
doesn't check if the gicc entrees are enabled or not.

Jonathan

> 
> Jonathan
> 
> 
> 
> 
> 
> > 
> > Thanks,
> > 
> > 	M.
> >   
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v8 04/16] ACPI: processor: Move checks and availability of acpi_processor earlier

[Jonathan Cameron]

On Tue, 30 Apr 2024 11:13:41 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> On Tue, 30 Apr 2024 10:28:38 +0100
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> 
> > On Tue, 30 Apr 2024 14:17:24 +1000
> > Gavin Shan <gshan@redhat.com> wrote:
> >   
> > > On 4/26/24 23:51, Jonathan Cameron wrote:    
> > > > Make the per_cpu(processors, cpu) entries available earlier so that
> > > > they are available in arch_register_cpu() as ARM64 will need access
> > > > to the acpi_handle to distinguish between acpi_processor_add()
> > > > and earlier registration attempts (which will fail as _STA cannot
> > > > be checked).
> > > > 
> > > > Reorder the remove flow to clear this per_cpu() after
> > > > arch_unregister_cpu() has completed, allowing it to be used in
> > > > there as well.
> > > > 
> > > > Note that on x86 for the CPU hotplug case, the pr->id prior to
> > > > acpi_map_cpu() may be invalid. Thus the per_cpu() structures
> > > > must be initialized after that call or after checking the ID
> > > > is valid (not hotplug path).
> > > > 
> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > 
> > > > ---
> > > > v8: On buggy bios detection when setting per_cpu structures
> > > >      do not carry on.
> > > >      Fix up the clearing of per cpu structures to remove unwanted
> > > >      side effects and ensure an error code isn't use to reference them.
> > > > ---
> > > >   drivers/acpi/acpi_processor.c | 79 +++++++++++++++++++++--------------
> > > >   1 file changed, 48 insertions(+), 31 deletions(-)
> > > > 
> > > > diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> > > > index ba0a6f0ac841..3b180e21f325 100644
> > > > --- a/drivers/acpi/acpi_processor.c
> > > > +++ b/drivers/acpi/acpi_processor.c
> > > > @@ -183,8 +183,38 @@ static void __init acpi_pcc_cpufreq_init(void) {}
> > > >   #endif /* CONFIG_X86 */
> > > >   
> > > >   /* Initialization */
> > > > +static DEFINE_PER_CPU(void *, processor_device_array);
> > > > +
> > > > +static bool acpi_processor_set_per_cpu(struct acpi_processor *pr,
> > > > +				       struct acpi_device *device)
> > > > +{
> > > > +	BUG_ON(pr->id >= nr_cpu_ids);      
> > > 
> > > One blank line after BUG_ON() if we need to follow original implementation.    
> > 
> > Sure unintentional - I'll put that back.
> >   
> > >     
> > > > +	/*
> > > > +	 * Buggy BIOS check.
> > > > +	 * ACPI id of processors can be reported wrongly by the BIOS.
> > > > +	 * Don't trust it blindly
> > > > +	 */
> > > > +	if (per_cpu(processor_device_array, pr->id) != NULL &&
> > > > +	    per_cpu(processor_device_array, pr->id) != device) {
> > > > +		dev_warn(&device->dev,
> > > > +			 "BIOS reported wrong ACPI id %d for the processor\n",
> > > > +			 pr->id);
> > > > +		/* Give up, but do not abort the namespace scan. */      
> > > 
> > > It depends on how the return value is handled by the caller if the namespace
> > > is continued to be scanned. The caller can be acpi_processor_hotadd_init()
> > > and acpi_processor_get_info() after this patch is applied. So I think this
> > > specific comment need to be moved to the caller.    
> > 
> > Good point. This gets messy and was an unintended change.
> > 
> > Previously the options were:
> > 1) acpi_processor_get_info() failed for other reasons - this code was never called.
> > 2) acpi_processor_get_info() succeeded without acpi_processor_hotadd_init (non hotplug)
> >    this code then ran and would paper over the problem doing a bunch of cleanup under err.
> > 3) acpi_processor_get_info() succeeded with acpi_processor_hotadd_init called.
> >    This code then ran and would paper over the problem doing a bunch of cleanup under err.
> > 
> > We should maintain that or argue cleanly against it.
> > 
> > This isn't helped the the fact I have no idea which cases we care about for that bios
> > bug handling.  Do any of those bios's ever do hotplug?  Guess we have to try and maintain
> > whatever protection this was offering.
> > 
> > Also, the original code leaks data in some paths and I have limited idea
> > of whether it is intentional or not. So to tidy the issue up that you've identified
> > I'll need to try and make that code consistent first.
> > 
> > I suspect the only way to do that is going to be to duplicate the allocations we
> > 'want' to leak to deal with the bios bug detection.
> > 
> > For example acpi_processor_get_info() failing leaks pr and pr->throttling.shared_cpu_map
> > before this series. After this series we need pr to leak because it's used for the detection
> > via processor_device_array.
> > 
> > I'll work through this but it's going to be tricky to tell if we get right.
> > Step 1 will be closing the existing leaks and then we will have something
> > consistent to build on.
> >   
> I 'think' that fixing the original leaks makes this all much more straight forward.
> That return 0 for acpi_processor_get_info() never made sense as far as I can tell.
> The pr isn't used after this point.
> 
> What about something along lines of.
> 
> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> index 161c95c9d60a..97cff4492304 100644
> --- a/drivers/acpi/acpi_processor.c
> +++ b/drivers/acpi/acpi_processor.c
> @@ -392,8 +392,10 @@ static int acpi_processor_add(struct acpi_device *device,
>         device->driver_data = pr;
> 
>         result = acpi_processor_get_info(device);
> -       if (result) /* Processor is not physically present or unavailable */
> -               return 0;
> +       if (result) { /* Processor is not physically present or unavailable */
> +               result = 0;
> +               goto err_free_throttling_mask;

FWIW this is wrong, should be goto err_clear_driver_data
(you can see it set just at the top of this block and that never fails!)
The err_free_throttling_mask label should be unused and hence won't exist in v9.

> +       }
> 
>         BUG_ON(pr->id >= nr_cpu_ids);
> 
> @@ -408,7 +410,7 @@ static int acpi_processor_add(struct acpi_device *device,
>                         "BIOS reported wrong ACPI id %d for the processor\n",
>                         pr->id);
>                 /* Give up, but do not abort the namespace scan. */
> -               goto err;
> +               goto err_clear_driver_data;
>         }
>         /*
>          * processor_device_array is not cleared on errors to allow buggy BIOS
> @@ -420,12 +422,12 @@ static int acpi_processor_add(struct acpi_device *device,
>         dev = get_cpu_device(pr->id);
>         if (!dev) {
>                 result = -ENODEV;
> -               goto err;
> +               goto err_clear_per_cpu;
>         }
> 
>         result = acpi_bind_one(dev, device);
>         if (result)
> -               goto err;
> +               goto err_clear_per_cpu;
> 
>         pr->dev = dev;
> 
> @@ -436,10 +438,12 @@ static int acpi_processor_add(struct acpi_device *device,
>         dev_err(dev, "Processor driver could not be attached\n");
>         acpi_unbind_one(dev);
> 
> - err:
> -       free_cpumask_var(pr->throttling.shared_cpu_map);
> -       device->driver_data = NULL;
> + err_clear_per_cpu:
>         per_cpu(processors, pr->id) = NULL;
> + err_clear_driver_data:
> +       device->driver_data = NULL;
> + err_free_throttling_mask:
> +       free_cpumask_var(pr->throttling.shared_cpu_map);
>   err_free_pr:
>         kfree(pr);
>         return result;
> 
> Then the diff on this patch is simply:
> 
> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> index 3c49eae1e943..3b75f5aeb7ab 100644
> --- a/drivers/acpi/acpi_processor.c
> +++ b/drivers/acpi/acpi_processor.c
> @@ -200,7 +200,6 @@ static bool acpi_processor_set_per_cpu(struct acpi_processor *pr,
>                 dev_warn(&device->dev,
>                          "BIOS reported wrong ACPI id %d for the processor\n",
>                          pr->id);
> -               /* Give up, but do not abort the namespace scan. */
>                 return false;
>         }
>         /*
> @@ -230,13 +229,14 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr,
>                 goto out;
> 
>         if (!acpi_processor_set_per_cpu(pr, device)) {
> +               ret = -EINVAL;
>                 acpi_unmap_cpu(pr->id);
>                 goto out;
>         }
> 
>         ret = arch_register_cpu(pr->id);
>         if (ret) {
> -               /* Leave the processor device array in place to detect buggy bios */
> +x              /* Leave the processor device array in place to detect buggy bios */
>                 per_cpu(processors, pr->id) = NULL;
>                 acpi_unmap_cpu(pr->id);
>                 goto out;
> @@ -262,7 +262,7 @@ static inline int acpi_processor_hotadd_init(struct acpi_processor *pr,
>  }
>  #endif /* CONFIG_ACPI_HOTPLUG_CPU */
> 
> -static int acpi_processor_get_info(struct acpi_device *device)
> +static int acpi_processor_get_info(struct acpi_device *device, bool bios_bug)
>  {
>         union acpi_object object = { 0 };
>         struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
> @@ -361,7 +361,7 @@ static int acpi_processor_get_info(struct acpi_device *device)
>                         return ret;
>         } else {
>                 if (!acpi_processor_set_per_cpu(pr, device))
> -                       return 0;
> +                       return -EINVAL;
>         }
> 
>         /*
> > > 
> > > Besides, it seems acpi_processor_set_per_cpu() isn't properly called and
> > > memory leakage can happen. More details are given below.
> > >     
> > > > +		return false;
> > > > +	}
> > > > +	/*
> > > > +	 * processor_device_array is not cleared on errors to allow buggy BIOS
> > > > +	 * checks.
> > > > +	 */
> > > > +	per_cpu(processor_device_array, pr->id) = device;
> > > > +	per_cpu(processors, pr->id) = pr;
> > > > +
> > > > +	return true;
> > > > +}
> > > > +
> > > >   #ifdef CONFIG_ACPI_HOTPLUG_CPU
> > > > -static int acpi_processor_hotadd_init(struct acpi_processor *pr)
> > > > +static int acpi_processor_hotadd_init(struct acpi_processor *pr,
> > > > +				      struct acpi_device *device)
> > > >   {
> > > >   	int ret;
> > > >   
> > > > @@ -198,8 +228,15 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
> > > >   	if (ret)
> > > >   		goto out;
> > > >   
> > > > +	if (!acpi_processor_set_per_cpu(pr, device)) {
> > > > +		acpi_unmap_cpu(pr->id);
> > > > +		goto out;
> > > > +	}
> > > > +      
> > > 
> > > With the 'goto out', zero is returned from acpi_processor_hotadd_init() to acpi_processor_get_info().  
> 
> Indeed a bug :(
> 
> > > The zero return value is carried from acpi_map_cpu() in acpi_processor_hotadd_init(). If I'm correct,
> > > we need return errno from acpi_processor_get_info() to acpi_processor_add() so that cleanup can be
> > > done. For example, the cleanup corresponding to the 'err' tag can be done in acpi_processor_add().
> > > Otherwise, we will have memory leakage.  
> 
> The confusion here was that previously acpi_processor_add() was missing error cleanup for
> acpi_processor_get_info().  With that in place I think it's all much simpler.
> 
> Thanks for your eagle eyes!
> 
> Jonathan
> 
> 
> > >     
> > > >   	ret = arch_register_cpu(pr->id);
> > > >   	if (ret) {
> > > > +		/* Leave the processor device array in place to detect buggy bios */
> > > > +		per_cpu(processors, pr->id) = NULL;
> > > >   		acpi_unmap_cpu(pr->id);
> > > >   		goto out;
> > > >   	}
> > > > @@ -217,7 +254,8 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
> > > >   	return ret;
> > > >   }
> > > >   #else
> > > > -static inline int acpi_processor_hotadd_init(struct acpi_processor *pr)
> > > > +static inline int acpi_processor_hotadd_init(struct acpi_processor *pr,
> > > > +					     struct acpi_device *device)
> > > >   {
> > > >   	return -ENODEV;
> > > >   }
> > > > @@ -316,10 +354,13 @@ static int acpi_processor_get_info(struct acpi_device *device)
> > > >   	 *  because cpuid <-> apicid mapping is persistent now.
> > > >   	 */
> > > >   	if (invalid_logical_cpuid(pr->id) || !cpu_present(pr->id)) {
> > > > -		int ret = acpi_processor_hotadd_init(pr);
> > > > +		int ret = acpi_processor_hotadd_init(pr, device);
> > > >   
> > > >   		if (ret)
> > > >   			return ret;
> > > > +	} else {
> > > > +		if (!acpi_processor_set_per_cpu(pr, device))
> > > > +			return 0;
> > > >   	}
> > > >         
> > > 
> > > For non-hotplug case, we still need pass the error to acpi_processor_add() so that
> > > cleanup corresponding 'err' tag can be done. Otherwise, we will have memory leakage.
> > >     
> > > >   	/*
> > > > @@ -365,8 +406,6 @@ static int acpi_processor_get_info(struct acpi_device *device)
> > > >    * (cpu_data(cpu)) values, like CPU feature flags, family, model, etc.
> > > >    * Such things have to be put in and set up by the processor driver's .probe().
> > > >    */
> > > > -static DEFINE_PER_CPU(void *, processor_device_array);
> > > > -
> > > >   static int acpi_processor_add(struct acpi_device *device,
> > > >   					const struct acpi_device_id *id)
> > > >   {
> > > > @@ -395,28 +434,6 @@ static int acpi_processor_add(struct acpi_device *device,
> > > >   	if (result) /* Processor is not physically present or unavailable */
> > > >   		return 0;
> > > >   
> > > > -	BUG_ON(pr->id >= nr_cpu_ids);
> > > > -
> > > > -	/*
> > > > -	 * Buggy BIOS check.
> > > > -	 * ACPI id of processors can be reported wrongly by the BIOS.
> > > > -	 * Don't trust it blindly
> > > > -	 */
> > > > -	if (per_cpu(processor_device_array, pr->id) != NULL &&
> > > > -	    per_cpu(processor_device_array, pr->id) != device) {
> > > > -		dev_warn(&device->dev,
> > > > -			"BIOS reported wrong ACPI id %d for the processor\n",
> > > > -			pr->id);
> > > > -		/* Give up, but do not abort the namespace scan. */
> > > > -		goto err;
> > > > -	}
> > > > -	/*
> > > > -	 * processor_device_array is not cleared on errors to allow buggy BIOS
> > > > -	 * checks.
> > > > -	 */
> > > > -	per_cpu(processor_device_array, pr->id) = device;
> > > > -	per_cpu(processors, pr->id) = pr;
> > > > -
> > > >   	dev = get_cpu_device(pr->id);
> > > >   	if (!dev) {
> > > >   		result = -ENODEV;
> > > > @@ -469,10 +486,6 @@ static void acpi_processor_remove(struct acpi_device *device)
> > > >   	device_release_driver(pr->dev);
> > > >   	acpi_unbind_one(pr->dev);
> > > >   
> > > > -	/* Clean up. */
> > > > -	per_cpu(processor_device_array, pr->id) = NULL;
> > > > -	per_cpu(processors, pr->id) = NULL;
> > > > -
> > > >   	cpu_maps_update_begin();
> > > >   	cpus_write_lock();
> > > >   
> > > > @@ -480,6 +493,10 @@ static void acpi_processor_remove(struct acpi_device *device)
> > > >   	arch_unregister_cpu(pr->id);
> > > >   	acpi_unmap_cpu(pr->id);
> > > >   
> > > > +	/* Clean up. */
> > > > +	per_cpu(processor_device_array, pr->id) = NULL;
> > > > +	per_cpu(processors, pr->id) = NULL;
> > > > +
> > > >   	cpus_write_unlock();
> > > >   	cpu_maps_update_done();
> > > >         
> > > 
> > > Thanks,
> > > Gavin
> > >     
> >   
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v8 09/16] arm64: acpi: Move get_cpu_for_acpi_id() to a header

[Lorenzo Pieralisi]

On Fri, Apr 26, 2024 at 02:51:19PM +0100, Jonathan Cameron wrote:
> From: James Morse <james.morse@arm.com>
> 
> ACPI identifies CPUs by UID. get_cpu_for_acpi_id() maps the ACPI UID
> to the Linux CPU number.
> 
> The helper to retrieve this mapping is only available in arm64's NUMA
> code.
> 
> Move it to live next to get_acpi_id_for_cpu().
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Reviewed-by: Gavin Shan <gshan@redhat.com>
> Tested-by: Miguel Luis <miguel.luis@oracle.com>
> Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
> Tested-by: Jianyong Wu <jianyong.wu@arm.com>
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> Acked-by: Hanjun Guo <guohanjun@huawei.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> ---
> v8: Picked up tags.
> ---
>  arch/arm64/include/asm/acpi.h | 11 +++++++++++
>  arch/arm64/kernel/acpi_numa.c | 11 -----------
>  2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index 6792a1f83f2a..bc9a6656fc0c 100644
> --- a/arch/arm64/include/asm/acpi.h
> +++ b/arch/arm64/include/asm/acpi.h
> @@ -119,6 +119,17 @@ static inline u32 get_acpi_id_for_cpu(unsigned int cpu)
>  	return	acpi_cpu_get_madt_gicc(cpu)->uid;
>  }
>  
> +static inline int get_cpu_for_acpi_id(u32 uid)
> +{
> +	int cpu;
> +
> +	for (cpu = 0; cpu < nr_cpu_ids; cpu++)
> +		if (uid == get_acpi_id_for_cpu(cpu))
> +			return cpu;
> +
> +	return -EINVAL;
> +}
> +
>  static inline void arch_fix_phys_package_id(int num, u32 slot) { }
>  void __init acpi_init_cpus(void);
>  int apei_claim_sea(struct pt_regs *regs);
> diff --git a/arch/arm64/kernel/acpi_numa.c b/arch/arm64/kernel/acpi_numa.c
> index e51535a5f939..0c036a9a3c33 100644
> --- a/arch/arm64/kernel/acpi_numa.c
> +++ b/arch/arm64/kernel/acpi_numa.c
> @@ -34,17 +34,6 @@ int __init acpi_numa_get_nid(unsigned int cpu)
>  	return acpi_early_node_map[cpu];
>  }
>  
> -static inline int get_cpu_for_acpi_id(u32 uid)
> -{
> -	int cpu;
> -
> -	for (cpu = 0; cpu < nr_cpu_ids; cpu++)
> -		if (uid == get_acpi_id_for_cpu(cpu))
> -			return cpu;
> -
> -	return -EINVAL;
> -}
> -

Apologies for the late reply.

If anything, it may make sense to squash this patch into the commit that
is actually needing it, lest we might end up with a change that is
useless if it is taken stand alone.

That's all there is to say so:

Reviewed-by: Lorenzo Pieralisi <lpieralisi@kernel.org>

>  static int __init acpi_parse_gicc_pxm(union acpi_subtable_headers *header,
>  				      const unsigned long end)
>  {
> -- 
> 2.39.2
>