ACPI/arm64: add support for virtual cpu hotplug

Hi,

I'm posting James' patch set updated with most of the review comments from his RFC v2 series back in September. Individual patches have a changelog attached at the bottom of the commit message. Those which I have finished updating have my S-o-b on them, those which still have outstanding review comments from RFC v2 do not. In some of these cases I've asked questions and am waiting for responses.

I'm posting this as RFC v3 because there's still some unaddressed comments and it's clearly not ready for merging. Even if it was ready to be merged, it is too late in this development cycle to be taking this change in, so there would be little point posting it non-RFC. Also James stated that he's waiting for confirmation from the Kubernetes/Kata folk - I have no idea what the status is there.

I will be sending each patch individually to a wider audience appropriate for that patch - apologies to those missing out on this cover message. I have added more mailing lists to the series with the exception of the acpica list in a hope of this cover message also reaching those folk.

The changes that aren't included are:

1. Updates for my patch that was merged via Thomas (thanks!):
c4dd854f740c cpu-hotplug: Provide prototypes for arch CPU registration rather than having this change spread through James' patches.

2. New patch - simplification of PA-RISC's smp_prepare_boot_cpu()

3. Moved "ACPI: Use the acpi_device_is_present() helper in more places" and "ACPI: Rename acpi_scan_device_not_present() to be about enumeration" to the beginning of the series - these two patches are already queued up for merging into 6.7.

4. Moved "arm64, irqchip/gic-v3, ACPI: Move MADT GICC enabled check into a helper" to the beginning of the series, which has been submitted, but as yet the fate of that posting isn't known.

The first four patches in this series are provided for completness only.

There is an additional patch in James' git tree that isn't in the set of patches that James posted: "ACPI: processor: Only call arch_unregister_cpu() if HOTPLUG_CPU is selected" which looks to me to be a workaround for arch_unregister_cpu() being under the ifdef. I've commented on this on the RFC v2 posting making a suggestion, but as yet haven't had any response.

I've included almost all of James' original covering body below the diffstat.

The reason that I'm doing this is to help move this code forward so hopefully it can be merged - which is why I have been keen to dig out from James' patches anything that can be merged and submit it separately, since this is a feature for which some users have a definite need for.

Please note that I haven't tested this beyond building for aarch64 at the present time.

The series can be found at:

git://git.armlinux.org.uk/~rmk/linux-arm.git aarch64/hotplug-vcpu/v6.6-rc7

 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/include/asm/cpu.h               |   1 -
 arch/arm64/kernel/acpi_numa.c              |  11 ---
 arch/arm64/kernel/psci.c                   |   2 +-
 arch/arm64/kernel/setup.c                  |  13 +--
 arch/arm64/kernel/smp.c                    |   5 +-
 arch/ia64/Kconfig                          |   3 +
 arch/ia64/include/asm/acpi.h               |   2 +-
 arch/ia64/include/asm/cpu.h                |   6 --
 arch/ia64/kernel/acpi.c                    |   6 +-
 arch/ia64/kernel/setup.c                   |   2 +-
 arch/ia64/kernel/topology.c                |  35 +------
 arch/loongarch/Kconfig                     |   2 +
 arch/loongarch/configs/loongson3_defconfig |   2 +-
 arch/loongarch/kernel/acpi.c               |   4 +-
 arch/loongarch/kernel/topology.c           |  38 +-------
 arch/parisc/kernel/smp.c                   |   8 +-
 arch/riscv/Kconfig                         |   1 +
 arch/riscv/kernel/setup.c                  |  19 +---
 arch/x86/Kconfig                           |   3 +
 arch/x86/include/asm/cpu.h                 |   4 -
 arch/x86/kernel/acpi/boot.c                |   4 +-
 arch/x86/kernel/cpu/intel_epb.c            |   2 +-
 arch/x86/kernel/topology.c                 |  27 +-----
 drivers/acpi/Kconfig                       |  14 ++-
 drivers/acpi/acpi_processor.c              | 151 +++++++++++++++++++++++------
 drivers/acpi/bus.c                         |  16 +++
 drivers/acpi/device_pm.c                   |   2 +-
 drivers/acpi/device_sysfs.c                |   2 +-
 drivers/acpi/internal.h                    |   1 -
 drivers/acpi/processor_core.c              |   2 +-
 drivers/acpi/property.c                    |   2 +-
 drivers/acpi/scan.c                        | 148 ++++++++++++++++++----------
 drivers/base/arch_topology.c               |  38 +++++---
 drivers/base/cpu.c                         |  44 +++++++--
 drivers/base/init.c                        |   2 +-
 drivers/base/node.c                        |   7 --
 drivers/firmware/psci/psci.c               |   2 +
 drivers/irqchip/irq-gic-v3.c               |  38 +++++---
 include/acpi/acpi_bus.h                    |   1 +
 include/acpi/actbl2.h                      |   1 +
 include/linux/acpi.h                       |  13 ++-
 include/linux/cpu.h                        |   4 +
 include/linux/cpumask.h                    |  25 +++++
 kernel/cpu.c                               |   3 +
 48 files changed, 516 insertions(+), 292 deletions(-)

Quoted 151 lines · James Morse

Hello!

Changes since RFC-v1:
* riscv is new, ia64 is gone
* The KVM support is different, and upstream - no need to patch the host.

---

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 out of the arch code by using GENERIC_CPU_DEVICES, then into 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, 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. If there are problems with firmware tables on some devices, the CPUs will already be online by the time the acpi_processor_make_enabled() is called. A mismatch here causes a firmware-bug message and kernel taint. This should only affect people with broken firmware who also boot with maxcpus=1, and bring CPUs online later.

I had a go at switching the remaining architectures over to GENERIC_CPU_DEVICES, so that the Kconfig symbol can be removed, but I got stuck with powerpc and s390.

I've only build tested Loongarch and riscv. I've removed the ia64 specific patches, but left the changes in other patches to make git-grep review of renames easier.

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 salil/virt-cpuhp-armv8/rfc-v2-rc6

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

Why is this still an RFC? I'm still looking for confirmation from the kubernetes/kata folk that this works for them. Because of this I've culled the CC list...

This series is based on v6.6-rc1, and can be retrieved from:
https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git/ virtual_cpu_hotplug/rfc/v2

Thanks,

James Morse (34):
ACPI: Move ACPI_HOTPLUG_CPU to be disabled on arm64 and riscv
drivers: base: Use present CPUs in GENERIC_CPU_DEVICES
drivers: base: Allow parts of GENERIC_CPU_DEVICES to be overridden
drivers: base: Move cpu_dev_init() after node_dev_init()
drivers: base: Print a warning instead of panic() when register_cpu()
fails
arm64: setup: Switch over to GENERIC_CPU_DEVICES using
arch_register_cpu()
x86: intel_epb: Don't rely on link order
x86/topology: Switch over to GENERIC_CPU_DEVICES
LoongArch: Switch over to GENERIC_CPU_DEVICES
riscv: Switch over to GENERIC_CPU_DEVICES
arch_topology: Make register_cpu_capacity_sysctl() tolerant to late
CPUs
ACPI: Use the acpi_device_is_present() helper in more places
ACPI: Rename acpi_scan_device_not_present() to be about enumeration
ACPI: Only enumerate enabled (or functional) devices
ACPI: processor: Add support for processors described as container
packages
ACPI: processor: Register CPUs that are online, but not described in
the DSDT
ACPI: processor: Register all CPUs from acpi_processor_get_info()
ACPI: Rename ACPI_HOTPLUG_CPU to include 'present'
ACPI: Move acpi_bus_trim_one() before acpi_scan_hot_remove()
ACPI: Rename acpi_processor_hotadd_init and remove pre-processor
guards
ACPI: Add post_eject to struct acpi_scan_handler for cpu hotplug
ACPI: Check _STA present bit before making CPUs not present
ACPI: Warn when the present bit changes but the feature is not enabled
drivers: base: Implement weak arch_unregister_cpu()
LoongArch: Use the __weak version of arch_unregister_cpu()
arm64: acpi: Move get_cpu_for_acpi_id() to a header
ACPICA: Add new MADT GICC flags fields [code first?] arm64, irqchip/gic-v3, ACPI: Move MADT GICC enabled check into a
helper
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
ACPI: add support to register CPUs based on the _STA enabled bit
arm64: document virtual CPU hotplug's expectations
ACPI: Add _OSC bits to advertise OS support for toggling CPU
present/enabled
cpumask: Add enabled cpumask for present CPUs that can be brought
online

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

Signature · 2 lines

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

From: James Morse <[email protected]>

Today the ACPI enumeration code 'visits' all devices that are present.

This is a problem for arm64, where CPUs are always present, but not always enabled. When a device-check occurs because the firmware-policy has changed and a CPU is now enabled, the following error occurs:
| acpi ACPI0007:48: Enumeration failure

This is ultimately because acpi_dev_ready_for_enumeration() returns true for a device that is not enabled. The ACPI Processor driver will not register such CPUs as they are not 'decoding their resources'.

Change acpi_dev_ready_for_enumeration() to also check the enabled bit. ACPI allows a device to be functional instead of maintaining the present and enabled bit. Make this behaviour an explicit check with a reference to the spec, and then check the present and enabled bits. This is needed to avoid enumerating present && functional devices that are not enabled.

Signed-off-by James Morse <[email protected]>

Tested-by Miguel Luis <[email protected]>

Tested-by Vishnu Pajjuri <[email protected]>

Tested-by Jianyong Wu <[email protected]>

Signed-off-by Russell King (Oracle) <[email protected]>

---
If this change causes problems on deployed hardware, I suggest an arch opt-in: ACPI_IGNORE_STA_ENABLED, that causes acpi_dev_ready_for_enumeration() to only check the present bit.

Changes since RFC v2:
* Incorporate comment suggestion by Gavin Shan.
Other review comments from Jonathan Cameron not yet addressed.

---
 drivers/acpi/device_pm.c    |  2 +-
 drivers/acpi/device_sysfs.c |  2 +-
 drivers/acpi/internal.h     |  1 -
 drivers/acpi/property.c     |  2 +-
 drivers/acpi/scan.c         | 24 ++++++++++++++----------
 5 files changed, 17 insertions(+), 14 deletions(-)

drivers/acpi/device_pm.c +1 -1

index 3b4d048c4941..e3c80f3b3b57 100644
@@ -313,7 +313,7 @@ int acpi_bus_init_power(struct acpi_device *device)
 		return -EINVAL;
 
 	device->power.state = ACPI_STATE_UNKNOWN;
-	if (!acpi_device_is_present(device)) {
+	if (!acpi_dev_ready_for_enumeration(device)) {
 		device->flags.initialized = false;
 		return -ENXIO;
 	}

drivers/acpi/device_sysfs.c +1 -1

index 23373faa35ec..a0256d2493a7 100644
@@ -141,7 +141,7 @@ static int create_pnp_modalias(const struct acpi_device *acpi_dev, char *modalia
 	struct acpi_hardware_id *id;
 
 	/* Avoid unnecessarily loading modules for non present devices. */
-	if (!acpi_device_is_present(acpi_dev))
+	if (!acpi_dev_ready_for_enumeration(acpi_dev))
 		return 0;
 
 	/*

drivers/acpi/internal.h +0 -1

index 866c7c4ed233..a1b45e345bcc 100644
@@ -107,7 +107,6 @@ int acpi_device_setup_files(struct acpi_device *dev);
 void acpi_device_remove_files(struct acpi_device *dev);
 void acpi_device_add_finalize(struct acpi_device *device);
 void acpi_free_pnp_ids(struct acpi_device_pnp *pnp);
-bool acpi_device_is_present(const struct acpi_device *adev);
 bool acpi_device_is_battery(struct acpi_device *adev);
 bool acpi_device_is_first_physical_node(struct acpi_device *adev,
 					const struct device *dev);

drivers/acpi/property.c +1 -1

index 6979a3f9f90a..14d6948fd88a 100644
@@ -1420,7 +1420,7 @@ static bool acpi_fwnode_device_is_available(const struct fwnode_handle *fwnode)
 	if (!is_acpi_device_node(fwnode))
 		return false;
 
-	return acpi_device_is_present(to_acpi_device_node(fwnode));
+	return acpi_dev_ready_for_enumeration(to_acpi_device_node(fwnode));
 }
 
 static const void *

drivers/acpi/scan.c +4 -9

index 02bb2cce423f..728649a2a251 100644
@@ -304,7 +304,7 @@ static int acpi_scan_device_check(struct acpi_device *adev)
 	int error;
 
 	acpi_bus_get_status(adev);
-	if (acpi_device_is_present(adev)) {
+	if (acpi_dev_ready_for_enumeration(adev)) {
 		/*
 		 * This function is only called for device objects for which
 		 * matching scan handlers exist.  The only situation in which

@@ -338,7 +338,7 @@ static int acpi_scan_bus_check(struct acpi_device *adev, void *not_used)
 	int error;
 
 	acpi_bus_get_status(adev);
-	if (!acpi_device_is_present(adev)) {
+	if (!acpi_dev_ready_for_enumeration(adev)) {
 		acpi_scan_device_not_enumerated(adev);
 		return 0;
 	}

@@ -1913,11 +1913,6 @@ static bool acpi_device_should_be_hidden(acpi_handle handle)
 	return true;
 }
 
-bool acpi_device_is_present(const struct acpi_device *adev)
-{
-	return adev->status.present || adev->status.functional;
-}
-
 static bool acpi_scan_handler_matching(struct acpi_scan_handler *handler,
 				       const char *idstr,
 				       const struct acpi_device_id **matchid)

@@ -2381,16 +2376,25 @@ EXPORT_SYMBOL_GPL(acpi_dev_clear_dependencies);
  * acpi_dev_ready_for_enumeration - Check if the ACPI device is ready for enumeration
  * @device: Pointer to the &struct acpi_device to check
  *
- * Check if the device is present and has no unmet dependencies.
+ * Check if the device is functional or enabled and has no unmet dependencies.
  *
- * Return true if the device is ready for enumeratino. Otherwise, return false.
+ * Return true if the device is ready for enumeration. Otherwise, return false.
  */

drivers/acpi/scan.c +6 -0

+	 * bit is set.
+	 */
+	if (device->status.present)
+		return device->status.enabled;
+	else
+		return device->status.functional;
 }
 EXPORT_SYMBOL_GPL(acpi_dev_ready_for_enumeration);
 

Quoted 25 lines · On Wed, 13 Dec 2023 12:49:16 +0000 Russell King (Oracle)…

From: James Morse <[email protected]>

Today the ACPI enumeration code 'visits' all devices that are present.

This is a problem for arm64, where CPUs are always present, but not always enabled. When a device-check occurs because the firmware-policy has changed and a CPU is now enabled, the following error occurs:
| acpi ACPI0007:48: Enumeration failure

This is ultimately because acpi_dev_ready_for_enumeration() returns true for a device that is not enabled. The ACPI Processor driver will not register such CPUs as they are not 'decoding their resources'.

Change acpi_dev_ready_for_enumeration() to also check the enabled bit. ACPI allows a device to be functional instead of maintaining the present and enabled bit. Make this behaviour an explicit check with a reference to the spec, and then check the present and enabled bits. This is needed to avoid enumerating present && functional devices that are not enabled.

Signed-off-by James Morse <[email protected]>

Tested-by Miguel Luis <[email protected]>

Tested-by Vishnu Pajjuri <[email protected]>

Tested-by Jianyong Wu <[email protected]>

Signed-off-by Russell King (Oracle) <[email protected]>

Quoted 4 lines

---
If this change causes problems on deployed hardware, I suggest an arch opt-in: ACPI_IGNORE_STA_ENABLED, that causes acpi_dev_ready_for_enumeration() to only check the present bit.

My gut feeling (having made ACPI 'fixes' in the past that ran into horribly broken firmware and had to be reverted) is reduce the blast radius preemptively from the start. I'd love to live in a world were that wasn't necessary but I don't trust all the generators of ACPI tables. I'll leave it to Rafael and other ACPI experts suggest how narrow we should make it though - arch opt in might be narrow enough.

Quoted 34 lines · Jonathan Cameron

Quoted 25 lines · On Wed, 13 Dec 2023 12:49:16 +0000 Russell King (Oracle)…

From: James Morse <[email protected]>

Today the ACPI enumeration code 'visits' all devices that are present.

This is a problem for arm64, where CPUs are always present, but not always enabled. When a device-check occurs because the firmware-policy has changed and a CPU is now enabled, the following error occurs:
| acpi ACPI0007:48: Enumeration failure

This is ultimately because acpi_dev_ready_for_enumeration() returns true for a device that is not enabled. The ACPI Processor driver will not register such CPUs as they are not 'decoding their resources'.

Change acpi_dev_ready_for_enumeration() to also check the enabled bit. ACPI allows a device to be functional instead of maintaining the present and enabled bit. Make this behaviour an explicit check with a reference to the spec, and then check the present and enabled bits. This is needed to avoid enumerating present && functional devices that are not enabled.

Signed-off-by James Morse <[email protected]>

Tested-by Miguel Luis <[email protected]>

Tested-by Vishnu Pajjuri <[email protected]>

Tested-by Jianyong Wu <[email protected]>

Signed-off-by Russell King (Oracle) <[email protected]>

Quoted 4 lines

---
If this change causes problems on deployed hardware, I suggest an arch opt-in: ACPI_IGNORE_STA_ENABLED, that causes acpi_dev_ready_for_enumeration() to only check the present bit.

Quoted 6 lines

My gut feeling (having made ACPI 'fixes' in the past that ran into horribly broken firmware and had to be reverted) is reduce the blast radius preemptively from the start. I'd love to live in a world were that wasn't necessary but I don't trust all the generators of ACPI tables. I'll leave it to Rafael and other ACPI experts suggest how narrow we should make it though - arch opt in might be narrow enough.

A chicken bit wouldn't help much IMO, especially in the cases when working setups get broken.

I would very much prefer to limit the scope of it, say to processors only, in the first place.

Quoted 43 lines · Rafael J. Wysocki

Quoted 34 lines · Jonathan Cameron

Quoted 25 lines · On Wed, 13 Dec 2023 12:49:16 +0000 Russell King (Oracle)…

From: James Morse <[email protected]>

Today the ACPI enumeration code 'visits' all devices that are present.

This is a problem for arm64, where CPUs are always present, but not always enabled. When a device-check occurs because the firmware-policy has changed and a CPU is now enabled, the following error occurs:
| acpi ACPI0007:48: Enumeration failure

This is ultimately because acpi_dev_ready_for_enumeration() returns true for a device that is not enabled. The ACPI Processor driver will not register such CPUs as they are not 'decoding their resources'.

Change acpi_dev_ready_for_enumeration() to also check the enabled bit. ACPI allows a device to be functional instead of maintaining the present and enabled bit. Make this behaviour an explicit check with a reference to the spec, and then check the present and enabled bits. This is needed to avoid enumerating present && functional devices that are not enabled.

Signed-off-by James Morse <[email protected]>

Tested-by Miguel Luis <[email protected]>

Tested-by Vishnu Pajjuri <[email protected]>

Tested-by Jianyong Wu <[email protected]>

Signed-off-by Russell King (Oracle) <[email protected]>

Quoted 4 lines

---
If this change causes problems on deployed hardware, I suggest an arch opt-in: ACPI_IGNORE_STA_ENABLED, that causes acpi_dev_ready_for_enumeration() to only check the present bit.

Quoted 6 lines

My gut feeling (having made ACPI 'fixes' in the past that ran into horribly broken firmware and had to be reverted) is reduce the blast radius preemptively from the start. I'd love to live in a world were that wasn't necessary but I don't trust all the generators of ACPI tables. I'll leave it to Rafael and other ACPI experts suggest how narrow we should make it though - arch opt in might be narrow enough.

Quoted 5 lines

A chicken bit wouldn't help much IMO, especially in the cases when working setups get broken.

I would very much prefer to limit the scope of it, say to processors only, in the first place.

Thanks for the feedback and the idea.

I guess we need something like:

        if (device->status.present)
                return device->device_type != ACPI_BUS_TYPE_PROCESSOR ||
                       device->status.enabled;
        else
                return device->status.functional;

so we only check device->status.enabled for processor-type devices?

Quoted 61 lines · Russell King (Oracle)

Quoted 43 lines · Rafael J. Wysocki

Quoted 34 lines · Jonathan Cameron

Quoted 25 lines · On Wed, 13 Dec 2023 12:49:16 +0000 Russell King (Oracle)…

From: James Morse <[email protected]>

Today the ACPI enumeration code 'visits' all devices that are present.

This is a problem for arm64, where CPUs are always present, but not always enabled. When a device-check occurs because the firmware-policy has changed and a CPU is now enabled, the following error occurs:
| acpi ACPI0007:48: Enumeration failure

This is ultimately because acpi_dev_ready_for_enumeration() returns true for a device that is not enabled. The ACPI Processor driver will not register such CPUs as they are not 'decoding their resources'.

Change acpi_dev_ready_for_enumeration() to also check the enabled bit. ACPI allows a device to be functional instead of maintaining the present and enabled bit. Make this behaviour an explicit check with a reference to the spec, and then check the present and enabled bits. This is needed to avoid enumerating present && functional devices that are not enabled.

Signed-off-by James Morse <[email protected]>

Tested-by Miguel Luis <[email protected]>

Tested-by Vishnu Pajjuri <[email protected]>

Tested-by Jianyong Wu <[email protected]>

Signed-off-by Russell King (Oracle) <[email protected]>

Quoted 4 lines

---
If this change causes problems on deployed hardware, I suggest an arch opt-in: ACPI_IGNORE_STA_ENABLED, that causes acpi_dev_ready_for_enumeration() to only check the present bit.

Quoted 6 lines

My gut feeling (having made ACPI 'fixes' in the past that ran into horribly broken firmware and had to be reverted) is reduce the blast radius preemptively from the start. I'd love to live in a world were that wasn't necessary but I don't trust all the generators of ACPI tables. I'll leave it to Rafael and other ACPI experts suggest how narrow we should make it though - arch opt in might be narrow enough.

Quoted 5 lines

A chicken bit wouldn't help much IMO, especially in the cases when working setups get broken.

I would very much prefer to limit the scope of it, say to processors only, in the first place.

Thanks for the feedback and the idea.

I guess we need something like:

        if (device->status.present)
                return device->device_type != ACPI_BUS_TYPE_PROCESSOR ||
                       device->status.enabled;
        else
                return device->status.functional;

Quoted 1 line

so we only check device->status.enabled for processor-type devices?

Yes, something like this.

Quoted 11 lines · Rafael J. Wysocki

Quoted 8 lines · Russell King (Oracle)

I guess we need something like:

        if (device->status.present)
                return device->device_type != ACPI_BUS_TYPE_PROCESSOR ||
                       device->status.enabled;
        else
                return device->status.functional;

Quoted 3 lines

Quoted 1 line

so we only check device->status.enabled for processor-type devices?

Yes, something like this.

However, that is not sufficient, because there are ACPI_BUS_TYPE_DEVICE devices representing processors.

I'm not sure about a clean way to do it ATM.

Quoted 12 lines · Rafael J. Wysocki

Quoted 11 lines · Rafael J. Wysocki

Quoted 8 lines · Russell King (Oracle)

I guess we need something like:

        if (device->status.present)
                return device->device_type != ACPI_BUS_TYPE_PROCESSOR ||
                       device->status.enabled;
        else
                return device->status.functional;

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

so we only check device->status.enabled for processor-type devices?

Yes, something like this.

However, that is not sufficient, because there are ACPI_BUS_TYPE_DEVICE devices representing processors.

I'm not sure about a clean way to do it ATM.

Ok, how about:

static bool acpi_dev_is_processor(const struct acpi_device *device)

{
	struct acpi_hardware_id *hwid;

	if (device->device_type == ACPI_BUS_TYPE_PROCESSOR)
		return true;

	if (device->device_type != ACPI_BUS_TYPE_DEVICE)
		return false;

	list_for_each_entry(hwid, &device->pnp.ids, list)
		if (!strcmp(ACPI_PROCESSOR_OBJECT_HID, hwid->id) ||
		    !strcmp(ACPI_PROCESSOR_DEVICE_HID, hwid->id))
			return true;

	return false;
}

and then:

	if (device->status.present)
		return !acpi_dev_is_processor(device) || device->status.enabled;
	else
		return device->status.functional;

?

Quoted 49 lines · On Fri, 15 Dec 2023 15:31:55 +0000 "Russell King (Oracle)"…

Quoted 12 lines · Rafael J. Wysocki

Quoted 11 lines · Rafael J. Wysocki

Quoted 8 lines · Russell King (Oracle)

I guess we need something like:

        if (device->status.present)
                return device->device_type != ACPI_BUS_TYPE_PROCESSOR ||
                       device->status.enabled;
        else
                return device->status.functional;

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

so we only check device->status.enabled for processor-type devices?

Yes, something like this.

However, that is not sufficient, because there are ACPI_BUS_TYPE_DEVICE devices representing processors.

I'm not sure about a clean way to do it ATM.

Ok, how about:

static bool acpi_dev_is_processor(const struct acpi_device *device)

{
	struct acpi_hardware_id *hwid;

	if (device->device_type == ACPI_BUS_TYPE_PROCESSOR)
		return true;

	if (device->device_type != ACPI_BUS_TYPE_DEVICE)
		return false;

	list_for_each_entry(hwid, &device->pnp.ids, list)
		if (!strcmp(ACPI_PROCESSOR_OBJECT_HID, hwid->id) ||
		    !strcmp(ACPI_PROCESSOR_DEVICE_HID, hwid->id))
			return true;

	return false;
}

and then:

	if (device->status.present)
		return !acpi_dev_is_processor(device) || device->status.enabled;
	else
		return device->status.functional;

Quoted 2 lines

?

Changing it to CPU only for now makes sense to me and I think this code snippet should do the job. Nice and simple.

Quoted 54 lines · CET Jonathan Cameron

Quoted 51 lines · On Fri, 15 Dec 2023 15:31:55 +0000 "Russell King (Oracle)"…

Quoted 12 lines · Rafael J. Wysocki

Quoted 11 lines · Rafael J. Wysocki

Quoted 8 lines · Russell King (Oracle)

I guess we need something like:

        if (device->status.present)
                return device->device_type != ACPI_BUS_TYPE_PROCESSOR ||
                       device->status.enabled;
        else
                return device->status.functional;

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

so we only check device->status.enabled for processor-type devices?

Yes, something like this.

However, that is not sufficient, because there are ACPI_BUS_TYPE_DEVICE devices representing processors.

I'm not sure about a clean way to do it ATM.

Ok, how about:

static bool acpi_dev_is_processor(const struct acpi_device *device)

{
	struct acpi_hardware_id *hwid;

	if (device->device_type == ACPI_BUS_TYPE_PROCESSOR)
		return true;

	if (device->device_type != ACPI_BUS_TYPE_DEVICE)
		return false;

	list_for_each_entry(hwid, &device->pnp.ids, list)
		if (!strcmp(ACPI_PROCESSOR_OBJECT_HID, hwid->id) ||
		    !strcmp(ACPI_PROCESSOR_DEVICE_HID, hwid->id))
			return true;

	return false;
}

and then:

	if (device->status.present)
		return !acpi_dev_is_processor(device) || device->status.enabled;
	else
		return device->status.functional;

?

Quoted 2 lines

Changing it to CPU only for now makes sense to me and I think this code snippet should do the job. Nice and simple.

Well, except that it does checks that are done elsewhere slightly differently, which from the maintenance POV is not nice.

Maybe something like the appended patch (untested).

---
 drivers/acpi/acpi_processor.c |   11 +++++++++++
 drivers/acpi/internal.h       |    3 +++
 drivers/acpi/scan.c           |   24 +++++++++++++++++++++++-
 3 files changed, 37 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/acpi/acpi_processor.c
===================================================================

drivers/acpi/acpi_processor.c +11 -0

@@ -644,6 +644,17 @@ static struct acpi_scan_handler processo
 	},
 };
 
+bool acpi_device_is_processor(const struct acpi_device *adev)
+{
+	if (adev->device_type == ACPI_BUS_TYPE_PROCESSOR)
+		return true;
+
+	if (adev->device_type != ACPI_BUS_TYPE_DEVICE)
+		return false;
+
+	return acpi_scan_check_handler(adev, &processor_handler);
+}
+
 static int acpi_processor_container_attach(struct acpi_device *dev,
 					   const struct acpi_device_id *id)
 {

Index: linux-pm/drivers/acpi/internal.h
===================================================================

drivers/acpi/internal.h +3 -0

@@ -62,6 +62,8 @@ void acpi_sysfs_add_hotplug_profile(stru
 int acpi_scan_add_handler_with_hotplug(struct acpi_scan_handler *handler,
 				       const char *hotplug_profile_name);
 void acpi_scan_hotplug_enabled(struct acpi_hotplug_profile *hotplug, bool val);
+bool acpi_scan_check_handler(const struct acpi_device *adev,
+			     struct acpi_scan_handler *handler);
 
 #ifdef CONFIG_DEBUG_FS
 extern struct dentry *acpi_debugfs_dir;

@@ -133,6 +135,7 @@ int acpi_bus_register_early_device(int t
 const struct acpi_device *acpi_companion_match(const struct device *dev);
 int __acpi_device_uevent_modalias(const struct acpi_device *adev,
 				  struct kobj_uevent_env *env);
+bool acpi_device_is_processor(const struct acpi_device *adev);
 
 /* --------------------------------------------------------------------------
                                   Power Resource

Index: linux-pm/drivers/acpi/scan.c
===================================================================

drivers/acpi/scan.c +23 -1

@@ -1938,6 +1938,19 @@ static bool acpi_scan_handler_matching(s
 	return false;
 }
 
+bool acpi_scan_check_handler(const struct acpi_device *adev,
+			     struct acpi_scan_handler *handler)
+{
+	struct acpi_hardware_id *hwid;
+
+	list_for_each_entry(hwid, &adev->pnp.ids, list) {
+		if (acpi_scan_handler_matching(handler, hwid->id, NULL))
+			return true;
+	}
+
+	return false;
+}
+
 static struct acpi_scan_handler *acpi_scan_match_handler(const char *idstr,
 					const struct acpi_device_id **matchid)
 {

@@ -2410,7 +2423,16 @@ bool acpi_dev_ready_for_enumeration(cons
 	if (device->flags.honor_deps && device->dep_unmet)
 		return false;
 
-	return acpi_device_is_present(device);
+	if (device->status.functional)
+		return true;
+
+	if (!device->status.present)
+		return false;
+
+	if (device->status.enabled)
+		return true; /* Fast path. */
+
+	return !acpi_device_is_processor(device);
 }
 EXPORT_SYMBOL_GPL(acpi_dev_ready_for_enumeration);
 

Quoted 55 lines · On Fri, 15 Dec 2023 20:47:31 +0100 "Rafael J. Wysocki" <[email protected]>…

Quoted 54 lines · CET Jonathan Cameron

Quoted 51 lines · On Fri, 15 Dec 2023 15:31:55 +0000 "Russell King (Oracle)"…

Quoted 12 lines · Rafael J. Wysocki

Quoted 11 lines · Rafael J. Wysocki

Quoted 8 lines · Russell King (Oracle)

I guess we need something like:

        if (device->status.present)
                return device->device_type != ACPI_BUS_TYPE_PROCESSOR ||
                       device->status.enabled;
        else
                return device->status.functional;

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

so we only check device->status.enabled for processor-type devices?

Yes, something like this.

However, that is not sufficient, because there are ACPI_BUS_TYPE_DEVICE devices representing processors.

I'm not sure about a clean way to do it ATM.

Ok, how about:

static bool acpi_dev_is_processor(const struct acpi_device *device)

{
	struct acpi_hardware_id *hwid;

	if (device->device_type == ACPI_BUS_TYPE_PROCESSOR)
		return true;

	if (device->device_type != ACPI_BUS_TYPE_DEVICE)
		return false;

	list_for_each_entry(hwid, &device->pnp.ids, list)
		if (!strcmp(ACPI_PROCESSOR_OBJECT_HID, hwid->id) ||
		    !strcmp(ACPI_PROCESSOR_DEVICE_HID, hwid->id))
			return true;

	return false;
}

and then:

	if (device->status.present)
		return !acpi_dev_is_processor(device) || device->status.enabled;
	else
		return device->status.functional;

?

Quoted 7 lines

Quoted 2 lines

Changing it to CPU only for now makes sense to me and I think this code snippet should do the job. Nice and simple.

Well, except that it does checks that are done elsewhere slightly differently, which from the maintenance POV is not nice.

Maybe something like the appended patch (untested).

Hi Rafael,

As far as I can see that's functionally equivalent, so looks good to me. I'm not set up to test this today though, so will defer to Russell on whether there is anything missing

Thanks for putting this together.

Jonathan

Quoted 85 lines

---
 drivers/acpi/acpi_processor.c |   11 +++++++++++
 drivers/acpi/internal.h       |    3 +++
 drivers/acpi/scan.c           |   24 +++++++++++++++++++++++-
 3 files changed, 37 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/acpi/acpi_processor.c
===================================================================

drivers/acpi/acpi_processor.c +11 -0

@@ -644,6 +644,17 @@ static struct acpi_scan_handler processo
 	},
 };
 
+bool acpi_device_is_processor(const struct acpi_device *adev)
+{
+	if (adev->device_type == ACPI_BUS_TYPE_PROCESSOR)
+		return true;
+
+	if (adev->device_type != ACPI_BUS_TYPE_DEVICE)
+		return false;
+
+	return acpi_scan_check_handler(adev, &processor_handler);
+}
+
 static int acpi_processor_container_attach(struct acpi_device *dev,
 					   const struct acpi_device_id *id)
 {

Index: linux-pm/drivers/acpi/internal.h
===================================================================

drivers/acpi/internal.h +3 -0

@@ -62,6 +62,8 @@ void acpi_sysfs_add_hotplug_profile(stru
 int acpi_scan_add_handler_with_hotplug(struct acpi_scan_handler *handler,
 				       const char *hotplug_profile_name);
 void acpi_scan_hotplug_enabled(struct acpi_hotplug_profile *hotplug, bool val);
+bool acpi_scan_check_handler(const struct acpi_device *adev,
+			     struct acpi_scan_handler *handler);
 
 #ifdef CONFIG_DEBUG_FS
 extern struct dentry *acpi_debugfs_dir;

@@ -133,6 +135,7 @@ int acpi_bus_register_early_device(int t
 const struct acpi_device *acpi_companion_match(const struct device *dev);
 int __acpi_device_uevent_modalias(const struct acpi_device *adev,
 				  struct kobj_uevent_env *env);
+bool acpi_device_is_processor(const struct acpi_device *adev);
 
 /* --------------------------------------------------------------------------
                                   Power Resource

Index: linux-pm/drivers/acpi/scan.c
===================================================================

drivers/acpi/scan.c +16 -1

@@ -1938,6 +1938,19 @@ static bool acpi_scan_handler_matching(s
 	return false;
 }
 
+bool acpi_scan_check_handler(const struct acpi_device *adev,
+			     struct acpi_scan_handler *handler)
+{
+	struct acpi_hardware_id *hwid;
+
+	list_for_each_entry(hwid, &adev->pnp.ids, list) {
+		if (acpi_scan_handler_matching(handler, hwid->id, NULL))
+			return true;
+	}
+
+	return false;
+}
+
 static struct acpi_scan_handler *acpi_scan_match_handler(const char *idstr,
 					const struct acpi_device_id **matchid)
 {

@@ -2410,7 +2423,16 @@ bool acpi_dev_ready_for_enumeration(cons
 	if (device->flags.honor_deps && device->dep_unmet)
 		return false;
 
-	return acpi_device_is_present(device);
+	if (device->status.functional)
+		return true;
+

drivers/acpi/scan.c bool acpi_dev_ready_for_enumeration(cons · line 2438

Quoted 10 lines

drivers/acpi/scan.c +7 -0

+	if (!device->status.present)
+		return false;
+
+	if (device->status.enabled)
+		return true; /* Fast path. */
+
+	return !acpi_device_is_processor(device);
 }
 EXPORT_SYMBOL_GPL(acpi_dev_ready_for_enumeration);
 

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 66 lines · On Tue, 2 Jan 2024 14:39:25 +0000 Jonathan Cameron…

Quoted 55 lines · On Fri, 15 Dec 2023 20:47:31 +0100 "Rafael J. Wysocki" <[email protected]>…

Quoted 54 lines · CET Jonathan Cameron

Quoted 51 lines · On Fri, 15 Dec 2023 15:31:55 +0000 "Russell King (Oracle)"…

Quoted 12 lines · Rafael J. Wysocki

Quoted 11 lines · Rafael J. Wysocki

Quoted 8 lines · Russell King (Oracle)

I guess we need something like:

        if (device->status.present)
                return device->device_type != ACPI_BUS_TYPE_PROCESSOR ||
                       device->status.enabled;
        else
                return device->status.functional;

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

so we only check device->status.enabled for processor-type devices?

Yes, something like this.

However, that is not sufficient, because there are ACPI_BUS_TYPE_DEVICE devices representing processors.

I'm not sure about a clean way to do it ATM.

Ok, how about:

static bool acpi_dev_is_processor(const struct acpi_device *device)

{
	struct acpi_hardware_id *hwid;

	if (device->device_type == ACPI_BUS_TYPE_PROCESSOR)
		return true;

	if (device->device_type != ACPI_BUS_TYPE_DEVICE)
		return false;

	list_for_each_entry(hwid, &device->pnp.ids, list)
		if (!strcmp(ACPI_PROCESSOR_OBJECT_HID, hwid->id) ||
		    !strcmp(ACPI_PROCESSOR_DEVICE_HID, hwid->id))
			return true;

	return false;
}

and then:

	if (device->status.present)
		return !acpi_dev_is_processor(device) || device->status.enabled;
	else
		return device->status.functional;

?

Quoted 7 lines

Quoted 2 lines

Changing it to CPU only for now makes sense to me and I think this code snippet should do the job. Nice and simple.

Well, except that it does checks that are done elsewhere slightly differently, which from the maintenance POV is not nice.

Maybe something like the appended patch (untested).

Quoted 7 lines

Hi Rafael,

As far as I can see that's functionally equivalent, so looks good to me. I'm not set up to test this today though, so will defer to Russell on whether there is anything missing

Thanks for putting this together.

This is rather embarrassing...

I span this up on a QEMU instance with some prints to find out we need the !acpi_device_is_processor() restriction.
On my 'random' test setup it fails on one device. ACPI0017 - which I happen to know rather well. It's the weird pseudo device that lets a CXL aware OS know there is a CEDT table to probe.

Whilst I really don't like that hack (it is all about making software distribution of out of tree modules easier rather than something fundamental), I'm the CXL QEMU maintainer :(

Will fix that, but it shows there is at least one broken firmware out there.

On plus side, Rafael's code seems to work as expected and lets that buggy firwmare carry on working :) So lets pretend the bug in qemu is a deliberate test case!

Quoted 89 lines · Jonathan Cameron

Quoted 66 lines · On Tue, 2 Jan 2024 14:39:25 +0000 Jonathan Cameron…

Quoted 55 lines · On Fri, 15 Dec 2023 20:47:31 +0100 "Rafael J. Wysocki" <[email protected]>…

Quoted 54 lines · CET Jonathan Cameron

Quoted 51 lines · On Fri, 15 Dec 2023 15:31:55 +0000 "Russell King (Oracle)"…

Quoted 12 lines · Rafael J. Wysocki

Quoted 11 lines · Rafael J. Wysocki

Quoted 8 lines · Russell King (Oracle)

I guess we need something like:

        if (device->status.present)
                return device->device_type != ACPI_BUS_TYPE_PROCESSOR ||
                       device->status.enabled;
        else
                return device->status.functional;

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

so we only check device->status.enabled for processor-type devices?

Yes, something like this.

However, that is not sufficient, because there are ACPI_BUS_TYPE_DEVICE devices representing processors.

I'm not sure about a clean way to do it ATM.

Ok, how about:

static bool acpi_dev_is_processor(const struct acpi_device *device)

{
	struct acpi_hardware_id *hwid;

	if (device->device_type == ACPI_BUS_TYPE_PROCESSOR)
		return true;

	if (device->device_type != ACPI_BUS_TYPE_DEVICE)
		return false;

	list_for_each_entry(hwid, &device->pnp.ids, list)
		if (!strcmp(ACPI_PROCESSOR_OBJECT_HID, hwid->id) ||
		    !strcmp(ACPI_PROCESSOR_DEVICE_HID, hwid->id))
			return true;

	return false;
}

and then:

	if (device->status.present)
		return !acpi_dev_is_processor(device) || device->status.enabled;
	else
		return device->status.functional;

?

Quoted 7 lines

Quoted 2 lines

Changing it to CPU only for now makes sense to me and I think this code snippet should do the job. Nice and simple.

Well, except that it does checks that are done elsewhere slightly differently, which from the maintenance POV is not nice.

Maybe something like the appended patch (untested).

Quoted 7 lines

Hi Rafael,

As far as I can see that's functionally equivalent, so looks good to me. I'm not set up to test this today though, so will defer to Russell on whether there is anything missing

Thanks for putting this together.

This is rather embarrassing...

I span this up on a QEMU instance with some prints to find out we need the !acpi_device_is_processor() restriction.
On my 'random' test setup it fails on one device. ACPI0017 - which I happen to know rather well. It's the weird pseudo device that lets a CXL aware OS know there is a CEDT table to probe.

Whilst I really don't like that hack (it is all about making software distribution of out of tree modules easier rather than something fundamental), I'm the CXL QEMU maintainer :(

Quoted 6 lines

Will fix that, but it shows there is at least one broken firmware out there.

On plus side, Rafael's code seems to work as expected and lets that buggy firwmare carry on working :) So lets pretend the bug in qemu is a deliberate test case!

Lol, thanks for a test case and showing that Rafael's approach is indeed necessary.

Would your test quality for a tested-by for this? For reference, this is my current version below with Rafael's update:

8<====
From: Russell King (Oracle) <[email protected]>
Subject: [PATCH] ACPI: Only enumerate enabled (or functional) processor
devices

From: James Morse <[email protected]>

Today the ACPI enumeration code 'visits' all devices that are present.

This is a problem for arm64, where CPUs are always present, but not always enabled. When a device-check occurs because the firmware-policy has changed and a CPU is now enabled, the following error occurs:
| acpi ACPI0007:48: Enumeration failure

This is ultimately because acpi_dev_ready_for_enumeration() returns true for a device that is not enabled. The ACPI Processor driver will not register such CPUs as they are not 'decoding their resources'.

ACPI allows a device to be functional instead of maintaining the present and enabled bit, but we can't simply check the enabled bit for all devices since firmware can be buggy.

If ACPI indicates that the device is present and enabled, then all well and good, we can enumate it. However, if the device is present and not enabled, then we also check whether the device is a processor device to limit the impact of this new check to just processor devices.

This avoids enumerating present && functional processor devices that are not enabled.

Signed-off-by James Morse <[email protected]>

Co-developed-by Rafael J. Wysocki <[email protected]>

Signed-off-by Russell King (Oracle) <[email protected]>

---
Changes since RFC v2:
* Incorporate comment suggestion by Gavin Shan.
Changes since RFC v3:
* Fixed "sert" typo.
Changes since RFC v3 (smaller series):
* Restrict checking the enabled bit to processor devices, update commit comments.
* Use Rafael's suggestion in
https://lore.kernel.org/r/5760569.DvuYhMxLoT@kreacher

---
 drivers/acpi/acpi_processor.c | 11 ++++++++
 drivers/acpi/device_pm.c      |  2 +-
 drivers/acpi/device_sysfs.c   |  2 +-
 drivers/acpi/internal.h       |  4 ++-
 drivers/acpi/property.c       |  2 +-
 drivers/acpi/scan.c           | 49 ++++++++++++++++++++++++++++-------
 6 files changed, 56 insertions(+), 14 deletions(-)

drivers/acpi/acpi_processor.c +11 -0

index 4fe2ef54088c..cf7c1cca69dd 100644
@@ -626,6 +626,17 @@ static struct acpi_scan_handler processor_handler = {
 	},
 };
 
+bool acpi_device_is_processor(const struct acpi_device *adev)
+{
+	if (adev->device_type == ACPI_BUS_TYPE_PROCESSOR)
+		return true;
+
+	if (adev->device_type != ACPI_BUS_TYPE_DEVICE)
+		return false;
+
+	return acpi_scan_check_handler(adev, &processor_handler);
+}
+
 static int acpi_processor_container_attach(struct acpi_device *dev,
 					   const struct acpi_device_id *id)
 {

drivers/acpi/device_pm.c +1 -1

index 3b4d048c4941..e3c80f3b3b57 100644
@@ -313,7 +313,7 @@ int acpi_bus_init_power(struct acpi_device *device)
 		return -EINVAL;
 
 	device->power.state = ACPI_STATE_UNKNOWN;
-	if (!acpi_device_is_present(device)) {
+	if (!acpi_dev_ready_for_enumeration(device)) {
 		device->flags.initialized = false;
 		return -ENXIO;
 	}

drivers/acpi/device_sysfs.c +1 -1

index 23373faa35ec..a0256d2493a7 100644
@@ -141,7 +141,7 @@ static int create_pnp_modalias(const struct acpi_device *acpi_dev, char *modalia
 	struct acpi_hardware_id *id;
 
 	/* Avoid unnecessarily loading modules for non present devices. */
-	if (!acpi_device_is_present(acpi_dev))
+	if (!acpi_dev_ready_for_enumeration(acpi_dev))
 		return 0;
 
 	/*

drivers/acpi/internal.h +3 -1

index 866c7c4ed233..9388d4c8674a 100644
@@ -62,6 +62,8 @@ void acpi_sysfs_add_hotplug_profile(struct acpi_hotplug_profile *hotplug,
 int acpi_scan_add_handler_with_hotplug(struct acpi_scan_handler *handler,
 				       const char *hotplug_profile_name);
 void acpi_scan_hotplug_enabled(struct acpi_hotplug_profile *hotplug, bool val);
+bool acpi_scan_check_handler(const struct acpi_device *adev,
+			     struct acpi_scan_handler *handler);
 
 #ifdef CONFIG_DEBUG_FS
 extern struct dentry *acpi_debugfs_dir;

@@ -107,7 +109,6 @@ int acpi_device_setup_files(struct acpi_device *dev);
 void acpi_device_remove_files(struct acpi_device *dev);
 void acpi_device_add_finalize(struct acpi_device *device);
 void acpi_free_pnp_ids(struct acpi_device_pnp *pnp);
-bool acpi_device_is_present(const struct acpi_device *adev);
 bool acpi_device_is_battery(struct acpi_device *adev);
 bool acpi_device_is_first_physical_node(struct acpi_device *adev,
 					const struct device *dev);

@@ -119,6 +120,7 @@ int acpi_bus_register_early_device(int type);
 const struct acpi_device *acpi_companion_match(const struct device *dev);
 int __acpi_device_uevent_modalias(const struct acpi_device *adev,
 				  struct kobj_uevent_env *env);
+bool acpi_device_is_processor(const struct acpi_device *adev);
 
 /* --------------------------------------------------------------------------
                                   Power Resource

drivers/acpi/property.c +1 -1

index 6979a3f9f90a..14d6948fd88a 100644
@@ -1420,7 +1420,7 @@ static bool acpi_fwnode_device_is_available(const struct fwnode_handle *fwnode)
 	if (!is_acpi_device_node(fwnode))
 		return false;
 
-	return acpi_device_is_present(to_acpi_device_node(fwnode));
+	return acpi_dev_ready_for_enumeration(to_acpi_device_node(fwnode));
 }
 
 static const void *

drivers/acpi/scan.c +39 -10

index 02bb2cce423f..f94d1f744bcc 100644
@@ -304,7 +304,7 @@ static int acpi_scan_device_check(struct acpi_device *adev)
 	int error;
 
 	acpi_bus_get_status(adev);
-	if (acpi_device_is_present(adev)) {
+	if (acpi_dev_ready_for_enumeration(adev)) {
 		/*
 		 * This function is only called for device objects for which
 		 * matching scan handlers exist.  The only situation in which

@@ -338,7 +338,7 @@ static int acpi_scan_bus_check(struct acpi_device *adev, void *not_used)
 	int error;
 
 	acpi_bus_get_status(adev);
-	if (!acpi_device_is_present(adev)) {
+	if (!acpi_dev_ready_for_enumeration(adev)) {
 		acpi_scan_device_not_enumerated(adev);
 		return 0;
 	}

@@ -1913,11 +1913,6 @@ static bool acpi_device_should_be_hidden(acpi_handle handle)
 	return true;
 }
 
-bool acpi_device_is_present(const struct acpi_device *adev)
-{
-	return adev->status.present || adev->status.functional;
-}
-
 static bool acpi_scan_handler_matching(struct acpi_scan_handler *handler,
 				       const char *idstr,
 				       const struct acpi_device_id **matchid)

@@ -1938,6 +1933,18 @@ static bool acpi_scan_handler_matching(struct acpi_scan_handler *handler,
 	return false;
 }
 
+bool acpi_scan_check_handler(const struct acpi_device *adev,
+			     struct acpi_scan_handler *handler)
+{
+	struct acpi_hardware_id *hwid;
+
+	list_for_each_entry(hwid, &adev->pnp.ids, list)
+		if (acpi_scan_handler_matching(handler, hwid->id, NULL))
+			return true;
+
+	return false;
+}
+
 static struct acpi_scan_handler *acpi_scan_match_handler(const char *idstr,
 					const struct acpi_device_id **matchid)
 {

@@ -2381,16 +2388,38 @@ EXPORT_SYMBOL_GPL(acpi_dev_clear_dependencies);
  * acpi_dev_ready_for_enumeration - Check if the ACPI device is ready for enumeration
  * @device: Pointer to the &struct acpi_device to check
  *
- * Check if the device is present and has no unmet dependencies.
+ * Check if the device is functional or enabled and has no unmet dependencies.
  *
- * Return true if the device is ready for enumeratino. Otherwise, return false.
+ * Return true if the device is ready for enumeration. Otherwise, return false.
  */
 bool acpi_dev_ready_for_enumeration(const struct acpi_device *device)
 {
 	if (device->flags.honor_deps && device->dep_unmet)
 		return false;
 
-	return acpi_device_is_present(device);
+	/*
+	 * ACPI 6.5's 6.3.7 "_STA (Device Status)" allows firmware to return
+	 * (!present && functional) for certain types of devices that should be
+	 * enumerated. Note that the enabled bit should not be set unless the
+	 * present bit is set.
+	 *
+	 * However, limit this only to processor devices to reduce possible
+	 * regressions with firmware.
+	 */
+	if (device->status.functional)
+		return true;
+
+	if (!device->status.present)
+		return false;
+
+	/*
+	 * Fast path - if enabled is set, avoid the more expensive test to
+	 * check whether this device is a processor.
+	 */
+	if (device->status.enabled)
+		return true;
+
+	return !acpi_device_is_processor(device);
 }
 EXPORT_SYMBOL_GPL(acpi_dev_ready_for_enumeration);
 

Quoted 15 lines · On Thu, 11 Jan 2024 10:26:15 +0000 "Russell King (Oracle)"…

drivers/acpi/scan.c +2 -2

@@ -2381,16 +2388,38 @@ EXPORT_SYMBOL_GPL(acpi_dev_clear_dependencies);
  * acpi_dev_ready_for_enumeration - Check if the ACPI device is ready for enumeration
  * @device: Pointer to the &struct acpi_device to check
  *
- * Check if the device is present and has no unmet dependencies.
+ * Check if the device is functional or enabled and has no unmet dependencies.
  *
- * Return true if the device is ready for enumeratino. Otherwise, return false.
+ * Return true if the device is ready for enumeration. Otherwise, return false.
  */
 bool acpi_dev_ready_for_enumeration(const struct acpi_device *device)
 {
 	if (device->flags.honor_deps && device->dep_unmet)
 		return false;
 

drivers/acpi/scan.c EXPORT_SYMBOL_GPL(acpi_dev_clear_dependencies); · line 2410

Quoted 12 lines

drivers/acpi/scan.c +11 -1

-	return acpi_device_is_present(device);
+	/*
+	 * ACPI 6.5's 6.3.7 "_STA (Device Status)" allows firmware to return
+	 * (!present && functional) for certain types of devices that should be
+	 * enumerated. Note that the enabled bit should not be set unless the
+	 * present bit is set.
+	 *
+	 * However, limit this only to processor devices to reduce possible
+	 * regressions with firmware.
+	 */
+	if (device->status.functional)
+		return true;

Hi Jonathan,

Quoted 17 lines · Jonathan Cameron

Quoted 15 lines · On Thu, 11 Jan 2024 10:26:15 +0000 "Russell King (Oracle)"…

drivers/acpi/scan.c +2 -2

@@ -2381,16 +2388,38 @@ EXPORT_SYMBOL_GPL(acpi_dev_clear_dependencies);
  * acpi_dev_ready_for_enumeration - Check if the ACPI device is ready for enumeration
  * @device: Pointer to the &struct acpi_device to check
  *
- * Check if the device is present and has no unmet dependencies.
+ * Check if the device is functional or enabled and has no unmet dependencies.
  *
- * Return true if the device is ready for enumeratino. Otherwise, return false.
+ * Return true if the device is ready for enumeration. Otherwise, return false.
  */
 bool acpi_dev_ready_for_enumeration(const struct acpi_device *device)
 {
    if (device->flags.honor_deps && device->dep_unmet)
            return false;
 

Quoted 12 lines

drivers/acpi/scan.c EXPORT_SYMBOL_GPL(acpi_dev_clear_dependencies); · line 2410

Quoted 12 lines

drivers/acpi/scan.c +11 -1

-   return acpi_device_is_present(device);
+   /*
+    * ACPI 6.5's 6.3.7 "_STA (Device Status)" allows firmware to return
+    * (!present && functional) for certain types of devices that should be
+    * enumerated. Note that the enabled bit should not be set unless the
+    * present bit is set.
+    *
+    * However, limit this only to processor devices to reduce possible
+    * regressions with firmware.
+    */
+   if (device->status.functional)
+           return true;

I have a report from within Oracle that this causes testing failures with QEMU using -smp cpus=2,maxcpus=4. I think it needs to be:

        if (!device->status.present)
                return device->status.functional;

        if (device->status.enabled)
                return true;

        return !acpi_device_is_processor(device);

So we can better understand the history here, let's list it as a truth table. P=present, F=functional, E=enabled, Orig=how the code is in mainline, James=James' original proposal, Rafael=the proposed replacement but seems to be buggy, Rmk=the fixed version that passes tests:

P F E   Orig    James   Rafael          Rmk
0 0 0   0       0       0               0
0 0 1   0       0       0               0
0 1 0   1       1       1               1
0 1 1   1       0       1               1
1 0 0   1       0       !processor      !processor
1 0 1   1       1       1               1
1 1 0   1       0       1               !processor
1 1 1   1       1       1               1

Any objections to this?

Quoted 37 lines · Russell King (Oracle)

Hi Jonathan,

Quoted 17 lines · Jonathan Cameron

Quoted 15 lines · On Thu, 11 Jan 2024 10:26:15 +0000 "Russell King (Oracle)"…

drivers/acpi/scan.c +2 -2

@@ -2381,16 +2388,38 @@ EXPORT_SYMBOL_GPL(acpi_dev_clear_dependencies);
  * acpi_dev_ready_for_enumeration - Check if the ACPI device is ready for enumeration
  * @device: Pointer to the &struct acpi_device to check
  *
- * Check if the device is present and has no unmet dependencies.
+ * Check if the device is functional or enabled and has no unmet dependencies.
  *
- * Return true if the device is ready for enumeratino. Otherwise, return false.
+ * Return true if the device is ready for enumeration. Otherwise, return false.
  */
 bool acpi_dev_ready_for_enumeration(const struct acpi_device *device)
 {
    if (device->flags.honor_deps && device->dep_unmet)
            return false;
 

Quoted 12 lines

drivers/acpi/scan.c EXPORT_SYMBOL_GPL(acpi_dev_clear_dependencies); · line 2410

Quoted 12 lines

drivers/acpi/scan.c +11 -1

-   return acpi_device_is_present(device);
+   /*
+    * ACPI 6.5's 6.3.7 "_STA (Device Status)" allows firmware to return
+    * (!present && functional) for certain types of devices that should be
+    * enumerated. Note that the enabled bit should not be set unless the
+    * present bit is set.
+    *
+    * However, limit this only to processor devices to reduce possible
+    * regressions with firmware.
+    */
+   if (device->status.functional)
+           return true;

I have a report from within Oracle that this causes testing failures with QEMU using -smp cpus=2,maxcpus=4. I think it needs to be:

Quoted 7 lines

        if (!device->status.present)
                return device->status.functional;

        if (device->status.enabled)
                return true;

        return !acpi_device_is_processor(device);

The above is fine by me.

Quoted 16 lines

So we can better understand the history here, let's list it as a truth table. P=present, F=functional, E=enabled, Orig=how the code is in mainline, James=James' original proposal, Rafael=the proposed replacement but seems to be buggy, Rmk=the fixed version that passes tests:

P F E   Orig    James   Rafael          Rmk
0 0 0   0       0       0               0
0 0 1   0       0       0               0
0 1 0   1       1       1               1
0 1 1   1       0       1               1
1 0 0   1       0       !processor      !processor
1 0 1   1       1       1               1
1 1 0   1       0       1               !processor
1 1 1   1       1       1               1

Quoted 1 line

Any objections to this?

So AFAIAC it can return false if not enabled, but present and functional. [Side note: I'm wondering what "functional" means then, but whatever.]

Quoted 65 lines · Rafael J. Wysocki

Quoted 37 lines · Russell King (Oracle)

Hi Jonathan,

Quoted 17 lines · Jonathan Cameron

Quoted 15 lines · On Thu, 11 Jan 2024 10:26:15 +0000 "Russell King (Oracle)"…

drivers/acpi/scan.c +2 -2

@@ -2381,16 +2388,38 @@ EXPORT_SYMBOL_GPL(acpi_dev_clear_dependencies);
  * acpi_dev_ready_for_enumeration - Check if the ACPI device is ready for enumeration
  * @device: Pointer to the &struct acpi_device to check
  *
- * Check if the device is present and has no unmet dependencies.
+ * Check if the device is functional or enabled and has no unmet dependencies.
  *
- * Return true if the device is ready for enumeratino. Otherwise, return false.
+ * Return true if the device is ready for enumeration. Otherwise, return false.
  */
 bool acpi_dev_ready_for_enumeration(const struct acpi_device *device)
 {
    if (device->flags.honor_deps && device->dep_unmet)
            return false;
 

Quoted 12 lines

drivers/acpi/scan.c EXPORT_SYMBOL_GPL(acpi_dev_clear_dependencies); · line 2410

Quoted 12 lines

drivers/acpi/scan.c +11 -1

-   return acpi_device_is_present(device);
+   /*
+    * ACPI 6.5's 6.3.7 "_STA (Device Status)" allows firmware to return
+    * (!present && functional) for certain types of devices that should be
+    * enumerated. Note that the enabled bit should not be set unless the
+    * present bit is set.
+    *
+    * However, limit this only to processor devices to reduce possible
+    * regressions with firmware.
+    */
+   if (device->status.functional)
+           return true;

I have a report from within Oracle that this causes testing failures with QEMU using -smp cpus=2,maxcpus=4. I think it needs to be:

Quoted 7 lines

        if (!device->status.present)
                return device->status.functional;

        if (device->status.enabled)
                return true;

        return !acpi_device_is_processor(device);

The above is fine by me.

Quoted 16 lines

So we can better understand the history here, let's list it as a truth table. P=present, F=functional, E=enabled, Orig=how the code is in mainline, James=James' original proposal, Rafael=the proposed replacement but seems to be buggy, Rmk=the fixed version that passes tests:

P F E   Orig    James   Rafael          Rmk
0 0 0   0       0       0               0
0 0 1   0       0       0               0
0 1 0   1       1       1               1
0 1 1   1       0       1               1
1 0 0   1       0       !processor      !processor
1 0 1   1       1       1               1
1 1 0   1       0       1               !processor
1 1 1   1       1       1               1

Quoted 5 lines

Quoted 1 line

Any objections to this?

So AFAIAC it can return false if not enabled, but present and functional. [Side note: I'm wondering what "functional" means then, but whatever.]

From ACPI v6.5 (bit 3 is our "status.functional":

_STA may return bit 0 clear (not present) with bit [3] set (device is functional). This case is used to indicate a valid device for which no device driver should be loaded (for example, a bridge device.) Children of this device may be present and valid. OSPM should continue enumeration below a device whose _STA returns this bit combination.

So, for this case, acpi_dev_ready_for_enumeration() returning true for this case is correct, since we're supposed to enumerate it and child devices.

It's probably also worth pointing out that in the above table, the two combinations with P=0 E=1 goes against the spec, but are included for completness.

Quoted 89 lines · Jonathan Cameron

Quoted 66 lines · On Tue, 2 Jan 2024 14:39:25 +0000 Jonathan Cameron…

Quoted 55 lines · On Fri, 15 Dec 2023 20:47:31 +0100 "Rafael J. Wysocki" <[email protected]>…

Quoted 54 lines · CET Jonathan Cameron

Quoted 51 lines · On Fri, 15 Dec 2023 15:31:55 +0000 "Russell King (Oracle)"…

Quoted 12 lines · Rafael J. Wysocki

Quoted 11 lines · Rafael J. Wysocki

Quoted 8 lines · Russell King (Oracle)

I guess we need something like:

         if (device->status.present)
                 return device->device_type != ACPI_BUS_TYPE_PROCESSOR ||
                        device->status.enabled;
         else
                 return device->status.functional;

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

so we only check device->status.enabled for processor-type devices?

Yes, something like this.

However, that is not sufficient, because there are ACPI_BUS_TYPE_DEVICE devices representing processors.

I'm not sure about a clean way to do it ATM.

Ok, how about:

static bool acpi_dev_is_processor(const struct acpi_device *device)

{
	struct acpi_hardware_id *hwid;

	if (device->device_type == ACPI_BUS_TYPE_PROCESSOR)
		return true;

	if (device->device_type != ACPI_BUS_TYPE_DEVICE)
		return false;

	list_for_each_entry(hwid, &device->pnp.ids, list)
		if (!strcmp(ACPI_PROCESSOR_OBJECT_HID, hwid->id) ||
		    !strcmp(ACPI_PROCESSOR_DEVICE_HID, hwid->id))
			return true;

	return false;
}

and then:

	if (device->status.present)
		return !acpi_dev_is_processor(device) || device->status.enabled;
	else
		return device->status.functional;

?

Quoted 7 lines

Quoted 2 lines

Changing it to CPU only for now makes sense to me and I think this code snippet should do the job. Nice and simple.

Well, except that it does checks that are done elsewhere slightly differently, which from the maintenance POV is not nice.

Maybe something like the appended patch (untested).

Quoted 7 lines

Hi Rafael,

As far as I can see that's functionally equivalent, so looks good to me. I'm not set up to test this today though, so will defer to Russell on whether there is anything missing

Thanks for putting this together.

This is rather embarrassing...

I span this up on a QEMU instance with some prints to find out we need the !acpi_device_is_processor() restriction.
On my 'random' test setup it fails on one device. ACPI0017 - which I happen to know rather well. It's the weird pseudo device that lets a CXL aware OS know there is a CEDT table to probe.

Whilst I really don't like that hack (it is all about making software distribution of out of tree modules easier rather than something fundamental), I'm the CXL QEMU maintainer :(

Quoted 6 lines

Will fix that, but it shows there is at least one broken firmware out there.

On plus side, Rafael's code seems to work as expected and lets that buggy firwmare carry on working :) So lets pretend the bug in qemu is a deliberate test case!

Lol, thanks for a test case and showing that Rafael's approach is indeed necessary.

Would your test quality for a tested-by for this? For reference, this is my current version below with Rafael's update:

8<====
From: Russell King (Oracle) <[email protected]>
Subject: [PATCH] ACPI: Only enumerate enabled (or functional) processor
devices

From: James Morse <[email protected]>

Today the ACPI enumeration code 'visits' all devices that are present.

This is a problem for arm64, where CPUs are always present, but not always enabled. When a device-check occurs because the firmware-policy has changed and a CPU is now enabled, the following error occurs:
| acpi ACPI0007:48: Enumeration failure

This is ultimately because acpi_dev_ready_for_enumeration() returns true for a device that is not enabled. The ACPI Processor driver will not register such CPUs as they are not 'decoding their resources'.

ACPI allows a device to be functional instead of maintaining the present and enabled bit, but we can't simply check the enabled bit for all devices since firmware can be buggy.

If ACPI indicates that the device is present and enabled, then all well and good, we can enumate it. However, if the device is present and not enabled, then we also check whether the device is a processor device to limit the impact of this new check to just processor devices.

This avoids enumerating present && functional processor devices that are not enabled.

Signed-off-by James Morse <[email protected]>

Co-developed-by Rafael J. Wysocki <[email protected]>

Signed-off-by Russell King (Oracle) <[email protected]>

---
Changes since RFC v2:
* Incorporate comment suggestion by Gavin Shan.
Changes since RFC v3:
* Fixed "sert" typo.
Changes since RFC v3 (smaller series):
* Restrict checking the enabled bit to processor devices, update commit comments.
* Use Rafael's suggestion in

    https://lore.kernel.org/r/5760569.DvuYhMxLoT@kreacher
---
  drivers/acpi/acpi_processor.c | 11 ++++++++
  drivers/acpi/device_pm.c      |  2 +-
  drivers/acpi/device_sysfs.c   |  2 +-
  drivers/acpi/internal.h       |  4 ++-
  drivers/acpi/property.c       |  2 +-
  drivers/acpi/scan.c           | 49 ++++++++++++++++++++++++++++-------
  6 files changed, 56 insertions(+), 14 deletions(-)

drivers/acpi/acpi_processor.c +11 -0

index 4fe2ef54088c..cf7c1cca69dd 100644
@@ -626,6 +626,17 @@ static struct acpi_scan_handler processor_handler = {
  	},
  };
 
+bool acpi_device_is_processor(const struct acpi_device *adev)
+{
+	if (adev->device_type == ACPI_BUS_TYPE_PROCESSOR)
+		return true;
+
+	if (adev->device_type != ACPI_BUS_TYPE_DEVICE)
+		return false;
+
+	return acpi_scan_check_handler(adev, &processor_handler);
+}
+
  static int acpi_processor_container_attach(struct acpi_device *dev,
  					   const struct acpi_device_id *id)
  {

drivers/acpi/device_pm.c +1 -1

index 3b4d048c4941..e3c80f3b3b57 100644
@@ -313,7 +313,7 @@ int acpi_bus_init_power(struct acpi_device *device)
  		return -EINVAL;
 
  	device->power.state = ACPI_STATE_UNKNOWN;
-	if (!acpi_device_is_present(device)) {
+	if (!acpi_dev_ready_for_enumeration(device)) {
  		device->flags.initialized = false;
  		return -ENXIO;
  	}

drivers/acpi/device_sysfs.c +1 -1

index 23373faa35ec..a0256d2493a7 100644
@@ -141,7 +141,7 @@ static int create_pnp_modalias(const struct acpi_device *acpi_dev, char *modalia
  	struct acpi_hardware_id *id;
 
  	/* Avoid unnecessarily loading modules for non present devices. */
-	if (!acpi_device_is_present(acpi_dev))
+	if (!acpi_dev_ready_for_enumeration(acpi_dev))
  		return 0;
 
  	/*

drivers/acpi/internal.h +3 -1

index 866c7c4ed233..9388d4c8674a 100644
@@ -62,6 +62,8 @@ void acpi_sysfs_add_hotplug_profile(struct acpi_hotplug_profile *hotplug,
  int acpi_scan_add_handler_with_hotplug(struct acpi_scan_handler *handler,
  				       const char *hotplug_profile_name);
  void acpi_scan_hotplug_enabled(struct acpi_hotplug_profile *hotplug, bool val);
+bool acpi_scan_check_handler(const struct acpi_device *adev,
+			     struct acpi_scan_handler *handler);
 
  #ifdef CONFIG_DEBUG_FS
  extern struct dentry *acpi_debugfs_dir;

@@ -107,7 +109,6 @@ int acpi_device_setup_files(struct acpi_device *dev);
  void acpi_device_remove_files(struct acpi_device *dev);
  void acpi_device_add_finalize(struct acpi_device *device);
  void acpi_free_pnp_ids(struct acpi_device_pnp *pnp);
-bool acpi_device_is_present(const struct acpi_device *adev);
  bool acpi_device_is_battery(struct acpi_device *adev);
  bool acpi_device_is_first_physical_node(struct acpi_device *adev,
  					const struct device *dev);

@@ -119,6 +120,7 @@ int acpi_bus_register_early_device(int type);
  const struct acpi_device *acpi_companion_match(const struct device *dev);
  int __acpi_device_uevent_modalias(const struct acpi_device *adev,
  				  struct kobj_uevent_env *env);
+bool acpi_device_is_processor(const struct acpi_device *adev);
 
  /* --------------------------------------------------------------------------
                                    Power Resource

drivers/acpi/property.c +1 -1

index 6979a3f9f90a..14d6948fd88a 100644
@@ -1420,7 +1420,7 @@ static bool acpi_fwnode_device_is_available(const struct fwnode_handle *fwnode)
  	if (!is_acpi_device_node(fwnode))
  		return false;
 
-	return acpi_device_is_present(to_acpi_device_node(fwnode));
+	return acpi_dev_ready_for_enumeration(to_acpi_device_node(fwnode));
  }
 
  static const void *

drivers/acpi/scan.c +28 -10

index 02bb2cce423f..f94d1f744bcc 100644
@@ -304,7 +304,7 @@ static int acpi_scan_device_check(struct acpi_device *adev)
  	int error;
 
  	acpi_bus_get_status(adev);
-	if (acpi_device_is_present(adev)) {
+	if (acpi_dev_ready_for_enumeration(adev)) {
  		/*
  		 * This function is only called for device objects for which
  		 * matching scan handlers exist.  The only situation in which

@@ -338,7 +338,7 @@ static int acpi_scan_bus_check(struct acpi_device *adev, void *not_used)
  	int error;
 
  	acpi_bus_get_status(adev);
-	if (!acpi_device_is_present(adev)) {
+	if (!acpi_dev_ready_for_enumeration(adev)) {
  		acpi_scan_device_not_enumerated(adev);
  		return 0;
  	}

@@ -1913,11 +1913,6 @@ static bool acpi_device_should_be_hidden(acpi_handle handle)
  	return true;
  }
 
-bool acpi_device_is_present(const struct acpi_device *adev)
-{
-	return adev->status.present || adev->status.functional;
-}
-
  static bool acpi_scan_handler_matching(struct acpi_scan_handler *handler,
  				       const char *idstr,
  				       const struct acpi_device_id **matchid)

@@ -1938,6 +1933,18 @@ static bool acpi_scan_handler_matching(struct acpi_scan_handler *handler,
  	return false;
  }
 
+bool acpi_scan_check_handler(const struct acpi_device *adev,
+			     struct acpi_scan_handler *handler)
+{
+	struct acpi_hardware_id *hwid;
+
+	list_for_each_entry(hwid, &adev->pnp.ids, list)
+		if (acpi_scan_handler_matching(handler, hwid->id, NULL))
+			return true;
+
+	return false;
+}
+
  static struct acpi_scan_handler *acpi_scan_match_handler(const char *idstr,
  					const struct acpi_device_id **matchid)
  {

@@ -2381,16 +2388,38 @@ EXPORT_SYMBOL_GPL(acpi_dev_clear_dependencies);
   * acpi_dev_ready_for_enumeration - Check if the ACPI device is ready for enumeration
   * @device: Pointer to the &struct acpi_device to check
   *
- * Check if the device is present and has no unmet dependencies.
+ * Check if the device is functional or enabled and has no unmet dependencies.
   *
- * Return true if the device is ready for enumeratino. Otherwise, return false.
+ * Return true if the device is ready for enumeration. Otherwise, return false.
   */
  bool acpi_dev_ready_for_enumeration(const struct acpi_device *device)
  {
  	if (device->flags.honor_deps && device->dep_unmet)
  		return false;
 
-	return acpi_device_is_present(device);
+	/*
+	 * ACPI 6.5's 6.3.7 "_STA (Device Status)" allows firmware to return
+	 * (!present && functional) for certain types of devices that should be
+	 * enumerated. Note that the enabled bit should not be set unless the
+	 * present bit is set.
+	 *
+	 * However, limit this only to processor devices to reduce possible
+	 * regressions with firmware.
+	 */
+	if (device->status.functional)
+		return true;
+

Quoted 25 lines · On Wed, 13 Dec 2023 12:49:16 +0000 Russell King (Oracle)…

From: James Morse <[email protected]>

Today the ACPI enumeration code 'visits' all devices that are present.

This is a problem for arm64, where CPUs are always present, but not always enabled. When a device-check occurs because the firmware-policy has changed and a CPU is now enabled, the following error occurs:
| acpi ACPI0007:48: Enumeration failure

This is ultimately because acpi_dev_ready_for_enumeration() returns true for a device that is not enabled. The ACPI Processor driver will not register such CPUs as they are not 'decoding their resources'.

Change acpi_dev_ready_for_enumeration() to also check the enabled bit. ACPI allows a device to be functional instead of maintaining the present and enabled bit. Make this behaviour an explicit check with a reference to the spec, and then check the present and enabled bits. This is needed to avoid enumerating present && functional devices that are not enabled.

Signed-off-by James Morse <[email protected]>

Tested-by Miguel Luis <[email protected]>

Tested-by Vishnu Pajjuri <[email protected]>

Tested-by Jianyong Wu <[email protected]>

Signed-off-by Russell King (Oracle) <[email protected]>

Quoted 4 lines

---
If this change causes problems on deployed hardware, I suggest an arch opt-in: ACPI_IGNORE_STA_ENABLED, that causes acpi_dev_ready_for_enumeration() to only check the present bit.

My gut feeling (having made ACPI 'fixes' in the past that ran into horribly broken firmware and had to be reverted) is reduce the blast radius preemptively from the start. I'd love to live in a world were that wasn't necessary but I don't trust all the generators of ACPI tables. I'll leave it to Rafael and other ACPI experts suggest how narrow we should make it though - arch opt in might be narrow enough.

drivers/acpi/scan.c EXPORT_SYMBOL_GPL(acpi_dev_clear_dependencies); · line 2391

Quoted 4 lines

drivers/acpi/scan.c +4 -0

+	/*
+	 * ACPI 6.5's 6.3.7 "_STA (Device Status)" allows firmware to return
+	 * (!present && functional) for certain types of devices that should be
+	 * enumerated. Note that the enabled bit can't be sert until the present

set until

From: James Morse <[email protected]>

ACPI has two ways of describing processors in the DSDT. From ACPI v6.5, 5.2.12:

"Starting with ACPI Specification 6.3, the use of the Processor() object was deprecated. Only legacy systems should continue with this usage. On the Itanium architecture only, a _UID is provided for the Processor() that is a string object. This usage of _UID is also deprecated since it can preclude an OSPM from being able to match a processor to a non-enumerable device, such as those defined in the MADT. From ACPI Specification 6.3 onward, all processor objects for all architectures except Itanium must now use Device() objects with an _HID of ACPI0007, and use only integer _UID values."

Also see https://uefi.org/specs/ACPI/6.5/08_Processor_Configuration_and_Control.html#declaring-processors

Duplicate descriptions are not allowed, the ACPI processor driver already parses the UID from both devices and containers. acpi_processor_get_info() returns an error if the UID exists twice in the DSDT.

The missing probe for CPUs described as packages creates a problem for moving the cpu_register() calls into the acpi_processor driver, as CPUs described like this don't get registered, leading to errors from other subsystems when they try to add new sysfs entries to the CPU node. (e.g. topology_sysfs_init()'s use of topology_add_dev() via cpuhp)

To fix this, parse the processor container and call acpi_processor_add() for each processor that is discovered like this. The processor container handler is added with acpi_scan_add_handler(), so no detach call will arrive.

Qemu TCG describes CPUs using processor devices in a processor container. For more information, see build_cpus_aml() in Qemu hw/acpi/cpu.c and
https://uefi.org/specs/ACPI/6.5/08_Processor_Configuration_and_Control.html#processor-container-device

Signed-off-by James Morse <[email protected]>

Tested-by Miguel Luis <[email protected]>

Tested-by Vishnu Pajjuri <[email protected]>

Tested-by Jianyong Wu <[email protected]>

---
Outstanding comments:
https://lore.kernel.org/r/[email protected]

https://lore.kernel.org/r/[email protected]

---
 drivers/acpi/acpi_processor.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

drivers/acpi/acpi_processor.c +22 -0

index 4fe2ef54088c..6a542e0ce396 100644
@@ -626,9 +626,31 @@ static struct acpi_scan_handler processor_handler = {
 	},
 };
 
+static acpi_status acpi_processor_container_walk(acpi_handle handle,
+						 u32 lvl,
+						 void *context,
+						 void **rv)
+{
+	struct acpi_device *adev;
+	acpi_status status;
+
+	adev = acpi_get_acpi_dev(handle);
+	if (!adev)
+		return AE_ERROR;
+
+	status = acpi_processor_add(adev, &processor_device_ids[0]);
+	acpi_put_acpi_dev(adev);
+
+	return status;
+}
+
 static int acpi_processor_container_attach(struct acpi_device *dev,
 					   const struct acpi_device_id *id)
 {
+	acpi_walk_namespace(ACPI_TYPE_PROCESSOR, dev->handle,
+			    ACPI_UINT32_MAX, acpi_processor_container_walk,
+			    NULL, NULL, NULL);
+
 	return 1;
 }
 

Quoted 36 lines · On Wed, 13 Dec 2023 12:49:21 +0000 Russell King (Oracle)…

From: James Morse <[email protected]>

ACPI has two ways of describing processors in the DSDT. From ACPI v6.5, 5.2.12:

"Starting with ACPI Specification 6.3, the use of the Processor() object was deprecated. Only legacy systems should continue with this usage. On the Itanium architecture only, a _UID is provided for the Processor() that is a string object. This usage of _UID is also deprecated since it can preclude an OSPM from being able to match a processor to a non-enumerable device, such as those defined in the MADT. From ACPI Specification 6.3 onward, all processor objects for all architectures except Itanium must now use Device() objects with an _HID of ACPI0007, and use only integer _UID values."

Also see https://uefi.org/specs/ACPI/6.5/08_Processor_Configuration_and_Control.html#declaring-processors

Duplicate descriptions are not allowed, the ACPI processor driver already parses the UID from both devices and containers. acpi_processor_get_info() returns an error if the UID exists twice in the DSDT.

The missing probe for CPUs described as packages creates a problem for moving the cpu_register() calls into the acpi_processor driver, as CPUs described like this don't get registered, leading to errors from other subsystems when they try to add new sysfs entries to the CPU node. (e.g. topology_sysfs_init()'s use of topology_add_dev() via cpuhp)

To fix this, parse the processor container and call acpi_processor_add() for each processor that is discovered like this. The processor container handler is added with acpi_scan_add_handler(), so no detach call will arrive.

Qemu TCG describes CPUs using processor devices in a processor container. For more information, see build_cpus_aml() in Qemu hw/acpi/cpu.c and
https://uefi.org/specs/ACPI/6.5/08_Processor_Configuration_and_Control.html#processor-container-device

Quoted 7 lines

Signed-off-by James Morse <[email protected]>

Tested-by Miguel Luis <[email protected]>

Tested-by Vishnu Pajjuri <[email protected]>

Tested-by Jianyong Wu <[email protected]>

---
Outstanding comments:
https://lore.kernel.org/r/[email protected]

Looks like you resolved those (were all patch description things).

So I'm happy.

Reviewed-by Jonathan Cameron <[email protected]>

From: James Morse <[email protected]>

ACPI has two ways of describing processors in the DSDT. From ACPI v6.5, 5.2.12:

"Starting with ACPI Specification 6.3, the use of the Processor() object was deprecated. Only legacy systems should continue with this usage. On the Itanium architecture only, a _UID is provided for the Processor() that is a string object. This usage of _UID is also deprecated since it can preclude an OSPM from being able to match a processor to a non-enumerable device, such as those defined in the MADT. From ACPI Specification 6.3 onward, all processor objects for all architectures except Itanium must now use Device() objects with an _HID of ACPI0007, and use only integer _UID values."

Also see https://uefi.org/specs/ACPI/6.5/08_Processor_Configuration_and_Control.html#declaring-processors

Duplicate descriptions are not allowed, the ACPI processor driver already parses the UID from both devices and containers. acpi_processor_get_info() returns an error if the UID exists twice in the DSDT.

The missing probe for CPUs described as packages

creates a problem for
moving the cpu_register() calls into the acpi_processor driver, as CPUs described like this don't get registered, leading to errors from other subsystems when they try to add new sysfs entries to the CPU node. (e.g. topology_sysfs_init()'s use of topology_add_dev() via cpuhp)

To fix this, parse the processor container and call acpi_processor_add() for each processor that is discovered like this.

The processor container
handler is added with acpi_scan_add_handler(), so no detach call will arrive.

Qemu TCG describes CPUs using processor devices in a processor container. For more information, see build_cpus_aml() in Qemu hw/acpi/cpu.c and
https://uefi.org/specs/ACPI/6.5/08_Processor_Configuration_and_Control.html#processor-container-device

Signed-off-by James Morse <[email protected]>

Tested-by Miguel Luis <[email protected]>

Tested-by Vishnu Pajjuri <[email protected]>

Tested-by Jianyong Wu <[email protected]>

---
Outstanding comments:
https://lore.kernel.org/r/[email protected]
https://lore.kernel.org/r/[email protected]

---
 drivers/acpi/acpi_processor.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

drivers/acpi/acpi_processor.c +1 -0

index 4fe2ef54088c..6a542e0ce396 100644
@@ -626,9 +626,31 @@ static struct acpi_scan_handler processor_handler = {
        },
 };
 
+static acpi_status acpi_processor_container_walk(acpi_handle handle,

drivers/acpi/acpi_processor.c +1 -1

+
        return 1;
 }
 
--

Quoted 16 lines · Russell King

From: James Morse <[email protected]>

ACPI has two ways of describing processors in the DSDT. From ACPI v6.5, 5.2.12:

"Starting with ACPI Specification 6.3, the use of the Processor() object was deprecated. Only legacy systems should continue with this usage. On the Itanium architecture only, a _UID is provided for the Processor() that is a string object. This usage of _UID is also deprecated since it can preclude an OSPM from being able to match a processor to a non-enumerable device, such as those defined in the MADT. From ACPI Specification 6.3 onward, all processor objects for all architectures except Itanium must now use Device() objects with an _HID of ACPI0007, and use only integer _UID values."

Quoted 5 lines

Also see https://uefi.org/specs/ACPI/6.5/08_Processor_Configuration_and_Control.html#declaring-processors

Duplicate descriptions are not allowed, the ACPI processor driver already parses the UID from both devices and containers. acpi_processor_get_info() returns an error if the UID exists twice in the DSDT.

I'm not really sure how the above is related to the actual patch.

Quoted 1 line

The missing probe for CPUs described as packages

It is unclear what exactly is meant by "CPUs described as packages".

From the patch, it looks like those would be Processor() objects defined under a processor container device.

Quoted 8 lines

creates a problem for
moving the cpu_register() calls into the acpi_processor driver, as CPUs described like this don't get registered, leading to errors from other subsystems when they try to add new sysfs entries to the CPU node. (e.g. topology_sysfs_init()'s use of topology_add_dev() via cpuhp)

To fix this, parse the processor container and call acpi_processor_add() for each processor that is discovered like this.

Discovered like what?

Quoted 3 lines

The processor container
handler is added with acpi_scan_add_handler(), so no detach call will arrive.

The above requires clarification too.

Quoted 41 lines · Rafael J. Wysocki

Quoted 16 lines · Russell King

From: James Morse <[email protected]>

ACPI has two ways of describing processors in the DSDT. From ACPI v6.5, 5.2.12:

"Starting with ACPI Specification 6.3, the use of the Processor() object was deprecated. Only legacy systems should continue with this usage. On the Itanium architecture only, a _UID is provided for the Processor() that is a string object. This usage of _UID is also deprecated since it can preclude an OSPM from being able to match a processor to a non-enumerable device, such as those defined in the MADT. From ACPI Specification 6.3 onward, all processor objects for all architectures except Itanium must now use Device() objects with an _HID of ACPI0007, and use only integer _UID values."

Quoted 5 lines

Also see https://uefi.org/specs/ACPI/6.5/08_Processor_Configuration_and_Control.html#declaring-processors

Duplicate descriptions are not allowed, the ACPI processor driver already parses the UID from both devices and containers. acpi_processor_get_info() returns an error if the UID exists twice in the DSDT.

I'm not really sure how the above is related to the actual patch.

Quoted 1 line

The missing probe for CPUs described as packages

It is unclear what exactly is meant by "CPUs described as packages".

From the patch, it looks like those would be Processor() objects defined under a processor container device.

Quoted 8 lines

creates a problem for
moving the cpu_register() calls into the acpi_processor driver, as CPUs described like this don't get registered, leading to errors from other subsystems when they try to add new sysfs entries to the CPU node. (e.g. topology_sysfs_init()'s use of topology_add_dev() via cpuhp)

To fix this, parse the processor container and call acpi_processor_add() for each processor that is discovered like this.

Quoted 7 lines

Discovered like what?

Quoted 3 lines

The processor container
handler is added with acpi_scan_add_handler(), so no detach call will arrive.

The above requires clarification too.

The above comments... yea. As I didn't write the commit description, but James did, and James has basically vanished, I don't think these can be answered, short of rewriting the entire commit message, with me spending a lot of time with the ACPI specification trying to get the terminology right - because at lot of the above on the face of it seems to be things to do with wrong terminology being used.

I wasn't expecting this level of issues with this patch set, and I now feel completely out of my depth with this series. I'm wondering whether I should even continue with it, since I don't have the ACPI knowledge to address a lot of these comments.

Quoted 58 lines · Russell King (Oracle)

Quoted 41 lines · Rafael J. Wysocki

Quoted 16 lines · Russell King

From: James Morse <[email protected]>

ACPI has two ways of describing processors in the DSDT. From ACPI v6.5, 5.2.12:

"Starting with ACPI Specification 6.3, the use of the Processor() object was deprecated. Only legacy systems should continue with this usage. On the Itanium architecture only, a _UID is provided for the Processor() that is a string object. This usage of _UID is also deprecated since it can preclude an OSPM from being able to match a processor to a non-enumerable device, such as those defined in the MADT. From ACPI Specification 6.3 onward, all processor objects for all architectures except Itanium must now use Device() objects with an _HID of ACPI0007, and use only integer _UID values."

Quoted 5 lines

Also see https://uefi.org/specs/ACPI/6.5/08_Processor_Configuration_and_Control.html#declaring-processors

Duplicate descriptions are not allowed, the ACPI processor driver already parses the UID from both devices and containers. acpi_processor_get_info() returns an error if the UID exists twice in the DSDT.

I'm not really sure how the above is related to the actual patch.

Quoted 1 line

The missing probe for CPUs described as packages

It is unclear what exactly is meant by "CPUs described as packages".

From the patch, it looks like those would be Processor() objects defined under a processor container device.

Quoted 8 lines

creates a problem for
moving the cpu_register() calls into the acpi_processor driver, as CPUs described like this don't get registered, leading to errors from other subsystems when they try to add new sysfs entries to the CPU node. (e.g. topology_sysfs_init()'s use of topology_add_dev() via cpuhp)

To fix this, parse the processor container and call acpi_processor_add() for each processor that is discovered like this.

Quoted 7 lines

Discovered like what?

Quoted 3 lines

The processor container
handler is added with acpi_scan_add_handler(), so no detach call will arrive.

The above requires clarification too.

The above comments... yea. As I didn't write the commit description, but James did, and James has basically vanished, I don't think these can be answered, short of rewriting the entire commit message, with me spending a lot of time with the ACPI specification trying to get the terminology right - because at lot of the above on the face of it seems to be things to do with wrong terminology being used.

Quoted 4 lines

I wasn't expecting this level of issues with this patch set, and I now feel completely out of my depth with this series. I'm wondering whether I should even continue with it, since I don't have the ACPI knowledge to address a lot of these comments.

Well, sorry about this.

I met James at the LPC last year, so he seems to be still around, in some way at least..

Quoted 65 lines · Rafael J. Wysocki

Quoted 58 lines · Russell King (Oracle)

Quoted 41 lines · Rafael J. Wysocki

Quoted 16 lines · Russell King

From: James Morse <[email protected]>

ACPI has two ways of describing processors in the DSDT. From ACPI v6.5, 5.2.12:

"Starting with ACPI Specification 6.3, the use of the Processor() object was deprecated. Only legacy systems should continue with this usage. On the Itanium architecture only, a _UID is provided for the Processor() that is a string object. This usage of _UID is also deprecated since it can preclude an OSPM from being able to match a processor to a non-enumerable device, such as those defined in the MADT. From ACPI Specification 6.3 onward, all processor objects for all architectures except Itanium must now use Device() objects with an _HID of ACPI0007, and use only integer _UID values."

Quoted 5 lines

Also see https://uefi.org/specs/ACPI/6.5/08_Processor_Configuration_and_Control.html#declaring-processors

Duplicate descriptions are not allowed, the ACPI processor driver already parses the UID from both devices and containers. acpi_processor_get_info() returns an error if the UID exists twice in the DSDT.

I'm not really sure how the above is related to the actual patch.

Quoted 1 line

The missing probe for CPUs described as packages

It is unclear what exactly is meant by "CPUs described as packages".

From the patch, it looks like those would be Processor() objects defined under a processor container device.

Quoted 8 lines

creates a problem for
moving the cpu_register() calls into the acpi_processor driver, as CPUs described like this don't get registered, leading to errors from other subsystems when they try to add new sysfs entries to the CPU node. (e.g. topology_sysfs_init()'s use of topology_add_dev() via cpuhp)

To fix this, parse the processor container and call acpi_processor_add() for each processor that is discovered like this.

Quoted 7 lines

Discovered like what?

Quoted 3 lines

The processor container
handler is added with acpi_scan_add_handler(), so no detach call will arrive.

The above requires clarification too.

The above comments... yea. As I didn't write the commit description, but James did, and James has basically vanished, I don't think these can be answered, short of rewriting the entire commit message, with me spending a lot of time with the ACPI specification trying to get the terminology right - because at lot of the above on the face of it seems to be things to do with wrong terminology being used.

Quoted 4 lines

I wasn't expecting this level of issues with this patch set, and I now feel completely out of my depth with this series. I'm wondering whether I should even continue with it, since I don't have the ACPI knowledge to address a lot of these comments.

Quoted 4 lines

Well, sorry about this.

I met James at the LPC last year, so he seems to be still around, in some way at least..

On the previous posting, I wanted James to comment on some of the feedback from Jonathan, and despite explicitly asking, there has been nothing but radio silence ever since James' last post of this series.

So, I now deem this work to be completely dead in the water, and not going to happen - not unless others can input on your comments.

Quoted 2 lines · Russell King

From: James Morse <[email protected]>

Quoted 4 lines

ACPI has two ways of describing processors in the DSDT. From ACPI v6.5, 5.2.12:

Quoted 9 lines

"Starting with ACPI Specification 6.3, the use of the Processor() object was deprecated. Only legacy systems should continue with this usage. On the Itanium architecture only, a _UID is provided for the Processor() that is a string object. This usage of _UID is also deprecated since it can preclude an OSPM from being able to match a processor to a non-enumerable device, such as those defined in the MADT. From ACPI Specification 6.3 onward, all processor objects for all architectures except Itanium must now use Device() objects with an _HID of ACPI0007, and use only integer _UID values."

Quoted 6 lines

Also see https://uefi.org/specs/ACPI/6.5/08_Processor_Configuration_and_Control.html#declaring-processors

Duplicate descriptions are not allowed, the ACPI processor driver already parses the UID from both devices and containers. acpi_processor_get_info() returns an error if the UID exists twice in the DSDT.

I'm not really sure how the above is related to the actual patch.

Quoted 2 lines

The missing probe for CPUs described as packages

It is unclear what exactly is meant by "CPUs described as packages".

From the patch, it looks like those would be Processor() objects defined under a processor container device.

Quoted 9 lines

creates a problem for
moving the cpu_register() calls into the acpi_processor driver, as CPUs described like this don't get registered, leading to errors from other subsystems when they try to add new sysfs entries to the CPU node. (e.g. topology_sysfs_init()'s use of topology_add_dev() via cpuhp)

To fix this, parse the processor container and call acpi_processor_add() for each processor that is discovered like this.

Discovered like what?

Quoted 4 lines

The processor container
handler is added with acpi_scan_add_handler(), so no detach call will arrive.

The above requires clarification too.

Quoted 1 line

Qemu TCG describes CPUs using processor devices in a processor container.

Quoted 24 lines

For more information, see build_cpus_aml() in Qemu hw/acpi/cpu.c and
https://uefi.org/specs/ACPI/6.5/08_Processor_Configuration_and_Control.html#processor-container-device

Signed-off-by James Morse <[email protected]>

Tested-by Miguel Luis <[email protected]>

Tested-by Vishnu Pajjuri <[email protected]>

Tested-by Jianyong Wu <[email protected]>

---
Outstanding comments:
https://lore.kernel.org/r/[email protected]
https://lore.kernel.org/r/[email protected]

---
 drivers/acpi/acpi_processor.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

drivers/acpi/acpi_processor.c +1 -0

index 4fe2ef54088c..6a542e0ce396 100644
@@ -626,9 +626,31 @@ static struct acpi_scan_handler processor_handler = {
        },
 };
 
+static acpi_status acpi_processor_container_walk(acpi_handle handle,

drivers/acpi/acpi_processor.c static struct acpi_scan_handler processor_handler = { · line 639

Quoted 10 lines

drivers/acpi/acpi_processor.c +10 -0

+                                                u32 lvl,
+                                                void *context,
+                                                void **rv)
+{
+       struct acpi_device *adev;
+       acpi_status status;
+
+       adev = acpi_get_acpi_dev(handle);
+       if (!adev)
+               return AE_ERROR;  

Why is the reference counting needed here?

Wouldn't acpi_fetch_acpi_dev() suffice?

Also, should the walk really be terminated on the first error?

Quoted 4 lines

drivers/acpi/acpi_processor.c +3 -0

+
+       status = acpi_processor_add(adev, &processor_device_ids[0]);
+       acpi_put_acpi_dev(adev);

drivers/acpi/acpi_processor.c static struct acpi_scan_handler processor_handler = { · line 652

Quoted 10 lines

drivers/acpi/acpi_processor.c +7 -0

+
+       return status;
+}
+
 static int acpi_processor_container_attach(struct acpi_device *dev,
                                           const struct acpi_device_id *id)
 {
+       acpi_walk_namespace(ACPI_TYPE_PROCESSOR, dev->handle,
+                           ACPI_UINT32_MAX, acpi_processor_container_walk,
+                           NULL, NULL, NULL);  

This covers processor objects only, so why is this not needed for processor devices defined under a processor container object?

It is not obvious, so it would be nice to add a comment explaining the difference.

drivers/acpi/acpi_processor.c static struct acpi_scan_handler processor_handler = { · line 656

Quoted 5 lines

drivers/acpi/acpi_processor.c +1 -0

+
        return 1;
 }
 

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 3 lines · On Mon, 18 Dec 2023 21:17:34 +0100 "Rafael J. Wysocki" <[email protected]>…

Quoted 2 lines · Russell King

From: James Morse <[email protected]>

Done some digging + machine faking. This is mid stage results at best.

Summary: I don't think this patch is necessary. If anyone happens to be in the mood for testing on various platforms, can you drop this patch and see if everything still works.

With this patch in place, and a processor container containing Processor() objects acpi_process_add is called twice - once via the path added here and once via acpi_bus_attach etc.

Maybe it's a left over from earlier approaches to some of this?

Both cases are covered by the existing handling without this.

I'm far from clear on why we need this patch. Presumably it's the reference in the description on it breaking for Processor Package containing Processor() objects that matters after a move... I'm struggling to find that move though!

Quoted 13 lines · Jonathan Cameron

Quoted 3 lines · On Mon, 18 Dec 2023 21:17:34 +0100 "Rafael J. Wysocki" <[email protected]>…

Quoted 2 lines · Russell King

From: James Morse <[email protected]>

Done some digging + machine faking. This is mid stage results at best.

Summary: I don't think this patch is necessary. If anyone happens to be in the mood for testing on various platforms, can you drop this patch and see if everything still works.

Quoted 5 lines

With this patch in place, and a processor container containing Processor() objects acpi_process_add is called twice - once via the path added here and once via acpi_bus_attach etc.

Maybe it's a left over from earlier approaches to some of this?

From what you're saying, it seems that way. It would be really good to get a reply from James to see whether he agrees - or at least get the reason why this patch is in the series... but I suspect that will never come.

Quoted 6 lines

Both cases are covered by the existing handling without this.

I'm far from clear on why we need this patch. Presumably it's the reference in the description on it breaking for Processor Package containing Processor() objects that matters after a move... I'm struggling to find that move though!

I do know that James did a lot of testing, so maybe he found some corner case somewhere which made this necessary - but without input from James, we can't know that.

So, maybe the right way forward on this is to re-test the series with this patch dropped, and see whether there's any ill effects. It should be possible to resurect the patch if it does turn out to be necessary.

Does that sound like a good way forward?

Thanks.

Quoted 36 lines · On Thu, 11 Jan 2024 18:46:47 +0000 "Russell King (Oracle)"…

Quoted 13 lines · Jonathan Cameron

Quoted 3 lines · On Mon, 18 Dec 2023 21:17:34 +0100 "Rafael J. Wysocki" <[email protected]>…

Quoted 2 lines · Russell King

From: James Morse <[email protected]>

Done some digging + machine faking. This is mid stage results at best.

Summary: I don't think this patch is necessary. If anyone happens to be in the mood for testing on various platforms, can you drop this patch and see if everything still works.

Quoted 5 lines

With this patch in place, and a processor container containing Processor() objects acpi_process_add is called twice - once via the path added here and once via acpi_bus_attach etc.

Maybe it's a left over from earlier approaches to some of this?

From what you're saying, it seems that way. It would be really good to get a reply from James to see whether he agrees - or at least get the reason why this patch is in the series... but I suspect that will never come.

Quoted 6 lines

Both cases are covered by the existing handling without this.

I'm far from clear on why we need this patch. Presumably it's the reference in the description on it breaking for Processor Package containing Processor() objects that matters after a move... I'm struggling to find that move though!

I do know that James did a lot of testing, so maybe he found some corner case somewhere which made this necessary - but without input from James, we can't know that.

Quoted 9 lines

So, maybe the right way forward on this is to re-test the series with this patch dropped, and see whether there's any ill effects. It should be possible to resurect the patch if it does turn out to be necessary.

Does that sound like a good way forward?

Thanks.

Yes that sounds like the best plan. Note this patch can only make a difference on non arm64 arches because it's a firmware bug to combine Processor() with a GICC entry in APIC/MADT. To even test on ARM64 you have to skip the bug check.

https://elixir.bootlin.com/linux/latest/source/drivers/acpi/processor_core.c#L101

/* device_declaration means Device object in DSDT, in the
* GIC interrupt model, logical processors are required to
* have a Processor Device object in the DSDT, so we should
* check device_declaration here
*/

//      if (device_declaration && (gicc->uid == acpi_id)) {
        if (gicc->uid == acpi_id) {
                *mpidr = gicc->arm_mpidr;
                return 0;
        }

Only alternative is probably to go history diving and try and find another change that would have required this and is now gone.

The ACPI scanning code has had a lot of changes whilst this work has been underway. More than possible that this was papering over some issue that has long since been fixed. I can't find any deliberate functional changes, but there is some code generalization that 'might' have side effects in this area. Rafael, any expectation that anything changed in how scanning processor containers works?

Quoted 71 lines · Jonathan Cameron

Quoted 36 lines · On Thu, 11 Jan 2024 18:46:47 +0000 "Russell King (Oracle)"…

Quoted 13 lines · Jonathan Cameron

Quoted 3 lines · On Mon, 18 Dec 2023 21:17:34 +0100 "Rafael J. Wysocki" <[email protected]>…

Quoted 2 lines · Russell King

From: James Morse <[email protected]>

Done some digging + machine faking. This is mid stage results at best.

Summary: I don't think this patch is necessary. If anyone happens to be in the mood for testing on various platforms, can you drop this patch and see if everything still works.

Quoted 5 lines

With this patch in place, and a processor container containing Processor() objects acpi_process_add is called twice - once via the path added here and once via acpi_bus_attach etc.

Maybe it's a left over from earlier approaches to some of this?

From what you're saying, it seems that way. It would be really good to get a reply from James to see whether he agrees - or at least get the reason why this patch is in the series... but I suspect that will never come.

Quoted 6 lines

Both cases are covered by the existing handling without this.

I'm far from clear on why we need this patch. Presumably it's the reference in the description on it breaking for Processor Package containing Processor() objects that matters after a move... I'm struggling to find that move though!

I do know that James did a lot of testing, so maybe he found some corner case somewhere which made this necessary - but without input from James, we can't know that.

Quoted 9 lines

So, maybe the right way forward on this is to re-test the series with this patch dropped, and see whether there's any ill effects. It should be possible to resurect the patch if it does turn out to be necessary.

Does that sound like a good way forward?

Thanks.

Yes that sounds like the best plan. Note this patch can only make a difference on non arm64 arches because it's a firmware bug to combine Processor() with a GICC entry in APIC/MADT. To even test on ARM64 you have to skip the bug check.

https://elixir.bootlin.com/linux/latest/source/drivers/acpi/processor_core.c#L101

/* device_declaration means Device object in DSDT, in the
* GIC interrupt model, logical processors are required to
* have a Processor Device object in the DSDT, so we should
* check device_declaration here
*/

//      if (device_declaration && (gicc->uid == acpi_id)) {
        if (gicc->uid == acpi_id) {
                *mpidr = gicc->arm_mpidr;
                return 0;
        }

Only alternative is probably to go history diving and try and find another change that would have required this and is now gone.

Quoted 6 lines

The ACPI scanning code has had a lot of changes whilst this work has been underway. More than possible that this was papering over some issue that has long since been fixed. I can't find any deliberate functional changes, but there is some code generalization that 'might' have side effects in this area. Rafael, any expectation that anything changed in how scanning processor containers works?

There have been changes, but I can't recall when exactly without some git history research.

In any case, it is always better to work on top of the current mainline code IMO.

In any case, it is always better to work on top of the current mainline code IMO.

From: James Morse <[email protected]>

ACPI has two descriptions of CPUs, one in the MADT/APIC table, the other in the DSDT. Both are required. (ACPI 6.5's 8.4 "Declaring Processors" says "Each processor in the system must be declared in the ACPI namespace"). Having two descriptions allows firmware authors to get this wrong.

If CPUs are described in the MADT/APIC, they will be brought online early during boot. Once the register_cpu() calls are moved to ACPI, they will be based on the DSDT description of the CPUs. When CPUs are missing from the DSDT description, they will end up online, but not registered.

Add a helper that runs after acpi_init() has completed to register CPUs that are online, but weren't found in the DSDT. Any CPU that is registered by this code triggers a firmware-bug warning and kernel taint.

Qemu TCG only describes the first CPU in the DSDT, unless cpu-hotplug is configured.

Signed-off-by James Morse <[email protected]>

Reviewed-by Jonathan Cameron <[email protected]>

Reviewed-by Gavin Shan <[email protected]>

Tested-by Miguel Luis <[email protected]>

Tested-by Vishnu Pajjuri <[email protected]>

Tested-by Jianyong Wu <[email protected]>

Signed-off-by Russell King (Oracle) <[email protected]>

---
 drivers/acpi/acpi_processor.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

drivers/acpi/acpi_processor.c +2 -0

index 6a542e0ce396..0511f2bc10bc 100644
@@ -791,6 +791,25 @@ void __init acpi_processor_init(void)
        acpi_pcc_cpufreq_init();
 }
 
+static int __init acpi_processor_register_missing_cpus(void)
+{

drivers/acpi/acpi_processor.c +7 -1

+               }
+       }
+
+       return 0;
+}
+subsys_initcall_sync(acpi_processor_register_missing_cpus);
+
 #ifdef CONFIG_ACPI_PROCESSOR_CSTATE
 /**
  * acpi_processor_claim_cst_control - Request _CST control from the platform.
--

Quoted 20 lines · Russell King

From: James Morse <[email protected]>

ACPI has two descriptions of CPUs, one in the MADT/APIC table, the other in the DSDT. Both are required. (ACPI 6.5's 8.4 "Declaring Processors" says "Each processor in the system must be declared in the ACPI namespace"). Having two descriptions allows firmware authors to get this wrong.

If CPUs are described in the MADT/APIC, they will be brought online early during boot. Once the register_cpu() calls are moved to ACPI, they will be based on the DSDT description of the CPUs. When CPUs are missing from the DSDT description, they will end up online, but not registered.

Add a helper that runs after acpi_init() has completed to register CPUs that are online, but weren't found in the DSDT. Any CPU that is registered by this code triggers a firmware-bug warning and kernel taint.

Quoted 2 lines

Qemu TCG only describes the first CPU in the DSDT, unless cpu-hotplug is configured.

So why is this a kernel problem?

Quoted 19 lines

Signed-off-by James Morse <[email protected]>

Reviewed-by Jonathan Cameron <[email protected]>

Reviewed-by Gavin Shan <[email protected]>

Tested-by Miguel Luis <[email protected]>

Tested-by Vishnu Pajjuri <[email protected]>

Tested-by Jianyong Wu <[email protected]>

Signed-off-by Russell King (Oracle) <[email protected]>

---
 drivers/acpi/acpi_processor.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

drivers/acpi/acpi_processor.c +0 -0

index 6a542e0ce396..0511f2bc10bc 100644
@@ -791,6 +791,25 @@ void __init acpi_processor_init(void)
        acpi_pcc_cpufreq_init();
 }
 

drivers/acpi/acpi_processor.c void __init acpi_processor_init(void) · line 805

Quoted 12 lines

drivers/acpi/acpi_processor.c +12 -0

+static int __init acpi_processor_register_missing_cpus(void)
+{
+       int cpu;
+
+       if (acpi_disabled)
+               return 0;
+
+       for_each_online_cpu(cpu) {
+               if (!get_cpu_device(cpu)) {
+                       pr_err_once(FW_BUG "CPU %u has no ACPI namespace description!\n", cpu);
+                       add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
+                       arch_register_cpu(cpu);

Which part of this code is related to ACPI?

Quoted 21 lines · Rafael J. Wysocki

Quoted 20 lines · Russell King

From: James Morse <[email protected]>

ACPI has two descriptions of CPUs, one in the MADT/APIC table, the other in the DSDT. Both are required. (ACPI 6.5's 8.4 "Declaring Processors" says "Each processor in the system must be declared in the ACPI namespace"). Having two descriptions allows firmware authors to get this wrong.

If CPUs are described in the MADT/APIC, they will be brought online early during boot. Once the register_cpu() calls are moved to ACPI, they will be based on the DSDT description of the CPUs. When CPUs are missing from the DSDT description, they will end up online, but not registered.

Add a helper that runs after acpi_init() has completed to register CPUs that are online, but weren't found in the DSDT. Any CPU that is registered by this code triggers a firmware-bug warning and kernel taint.

Quoted 4 lines

Quoted 2 lines

Qemu TCG only describes the first CPU in the DSDT, unless cpu-hotplug is configured.

So why is this a kernel problem?

So what are you proposing should be the behaviour here? What this statement seems to be saying is that QEMU as it exists today only describes the first CPU in DSDT.

As this patch series changes when arch_register_cpu() gets called (as described in the paragraph above) we obviously need to preserve the _existing_ behaviour to avoid causing regressions. So, if changing the kernel causes user visible regressions (e.g. sysfs entries to disappear) then it obviously _is_ a kernel problem that needs to be solved.

We can't say "well fix QEMU then" without invoking the wrath of Linus.

Quoted 20 lines

Quoted 20 lines

Signed-off-by James Morse <[email protected]>

Reviewed-by Jonathan Cameron <[email protected]>

Reviewed-by Gavin Shan <[email protected]>

Tested-by Miguel Luis <[email protected]>

Tested-by Vishnu Pajjuri <[email protected]>

Tested-by Jianyong Wu <[email protected]>

Signed-off-by Russell King (Oracle) <[email protected]>

---
 drivers/acpi/acpi_processor.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

drivers/acpi/acpi_processor.c +0 -0

index 6a542e0ce396..0511f2bc10bc 100644
@@ -791,6 +791,25 @@ void __init acpi_processor_init(void)
        acpi_pcc_cpufreq_init();
 }
 

Quoted 14 lines

drivers/acpi/acpi_processor.c void __init acpi_processor_init(void) · line 805

Quoted 12 lines

drivers/acpi/acpi_processor.c +12 -0

+static int __init acpi_processor_register_missing_cpus(void)
+{
+       int cpu;
+
+       if (acpi_disabled)
+               return 0;
+
+       for_each_online_cpu(cpu) {
+               if (!get_cpu_device(cpu)) {
+                       pr_err_once(FW_BUG "CPU %u has no ACPI namespace description!\n", cpu);
+                       add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
+                       arch_register_cpu(cpu);  

Which part of this code is related to ACPI?

That's a good question, and I suspect it would be more suited to being placed in drivers/base/cpu.c except for the problem that the error message refers to ACPI.

As long as we keep the acpi_disabled test, I guess that's fine. cpu_dev_register_generic() there already tests acpi_disabled.

Quoted 22 lines · On Mon, 15 Jan 2024 11:06:29 +0000 "Russell King (Oracle)"…

Quoted 21 lines · Rafael J. Wysocki

Quoted 20 lines · Russell King

From: James Morse <[email protected]>

ACPI has two descriptions of CPUs, one in the MADT/APIC table, the other in the DSDT. Both are required. (ACPI 6.5's 8.4 "Declaring Processors" says "Each processor in the system must be declared in the ACPI namespace"). Having two descriptions allows firmware authors to get this wrong.

If CPUs are described in the MADT/APIC, they will be brought online early during boot. Once the register_cpu() calls are moved to ACPI, they will be based on the DSDT description of the CPUs. When CPUs are missing from the DSDT description, they will end up online, but not registered.

Add a helper that runs after acpi_init() has completed to register CPUs that are online, but weren't found in the DSDT. Any CPU that is registered by this code triggers a firmware-bug warning and kernel taint.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Qemu TCG only describes the first CPU in the DSDT, unless cpu-hotplug is configured.

So why is this a kernel problem?

So what are you proposing should be the behaviour here? What this statement seems to be saying is that QEMU as it exists today only describes the first CPU in DSDT.

This confuses me somewhat, because I'm far from sure which machines this is true for in QEMU. I'm guessing it's a legacy thing with some old distro version of QEMU - so we'll have to paper over it anyway but for current QEMU I'm not sure it's true.

Helpfully there are a bunch of ACPI table tests so I've been checking through all the multi CPU cases.

CPU hotplug not enabled.
pc/DSDT.dimmpxm - 4x Processor entries. -smp 4 pc/DSDT.acpihmat - 2x Processor entries. -smp 2 q35/DSDT.acpihmat - 2x Processor entries. -smp 2 virt/DSDT.acpihmatvirt - 4x ACPI0007 entries -smp 4 q35/DSDT.acpihmat-noinitiator - 4 x Processor () entries -smp 4 virt/DSDT.topology - 8x ACPI0007 entries

I've also looked at the code and we have various types of CPU hotplug on x86 but they all build appropriate numbers of Processor() entries in DSDT.
Arm likewise seems to build the right number of ACPI0007 entries (and doesn't yet have CPU HP support).

If anyone can add a reference on why this is needed that would be very helpful.

Quoted 51 lines · Jonathan Cameron

Quoted 22 lines · On Mon, 15 Jan 2024 11:06:29 +0000 "Russell King (Oracle)"…

Quoted 21 lines · Rafael J. Wysocki

Quoted 20 lines · Russell King

From: James Morse <[email protected]>

ACPI has two descriptions of CPUs, one in the MADT/APIC table, the other in the DSDT. Both are required. (ACPI 6.5's 8.4 "Declaring Processors" says "Each processor in the system must be declared in the ACPI namespace"). Having two descriptions allows firmware authors to get this wrong.

If CPUs are described in the MADT/APIC, they will be brought online early during boot. Once the register_cpu() calls are moved to ACPI, they will be based on the DSDT description of the CPUs. When CPUs are missing from the DSDT description, they will end up online, but not registered.

Add a helper that runs after acpi_init() has completed to register CPUs that are online, but weren't found in the DSDT. Any CPU that is registered by this code triggers a firmware-bug warning and kernel taint.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Qemu TCG only describes the first CPU in the DSDT, unless cpu-hotplug is configured.

So why is this a kernel problem?

So what are you proposing should be the behaviour here? What this statement seems to be saying is that QEMU as it exists today only describes the first CPU in DSDT.

This confuses me somewhat, because I'm far from sure which machines this is true for in QEMU. I'm guessing it's a legacy thing with some old distro version of QEMU - so we'll have to paper over it anyway but for current QEMU I'm not sure it's true.

Helpfully there are a bunch of ACPI table tests so I've been checking through all the multi CPU cases.

CPU hotplug not enabled.
pc/DSDT.dimmpxm - 4x Processor entries. -smp 4 pc/DSDT.acpihmat - 2x Processor entries. -smp 2 q35/DSDT.acpihmat - 2x Processor entries. -smp 2 virt/DSDT.acpihmatvirt - 4x ACPI0007 entries -smp 4 q35/DSDT.acpihmat-noinitiator - 4 x Processor () entries -smp 4 virt/DSDT.topology - 8x ACPI0007 entries

Quoted 8 lines

I've also looked at the code and we have various types of CPU hotplug on x86 but they all build appropriate numbers of Processor() entries in DSDT.
Arm likewise seems to build the right number of ACPI0007 entries (and doesn't yet have CPU HP support).

If anyone can add a reference on why this is needed that would be very helpful.

Yes, it would.

Personally, I would prefer to assume that it is not necessary until it turns out that (1) there is firmware with this issue actually in use and (2) updating the firmware in question to follow the specification is not practical.

Otherwise, we'd make it easier to ship non-compliant firmware for no good reason.

Quoted 69 lines · Rafael J. Wysocki

Quoted 51 lines · Jonathan Cameron

Quoted 22 lines · On Mon, 15 Jan 2024 11:06:29 +0000 "Russell King (Oracle)"…

Quoted 21 lines · Rafael J. Wysocki

Quoted 20 lines · Russell King

From: James Morse <[email protected]>

ACPI has two descriptions of CPUs, one in the MADT/APIC table, the other in the DSDT. Both are required. (ACPI 6.5's 8.4 "Declaring Processors" says "Each processor in the system must be declared in the ACPI namespace"). Having two descriptions allows firmware authors to get this wrong.

If CPUs are described in the MADT/APIC, they will be brought online early during boot. Once the register_cpu() calls are moved to ACPI, they will be based on the DSDT description of the CPUs. When CPUs are missing from the DSDT description, they will end up online, but not registered.

Add a helper that runs after acpi_init() has completed to register CPUs that are online, but weren't found in the DSDT. Any CPU that is registered by this code triggers a firmware-bug warning and kernel taint.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Qemu TCG only describes the first CPU in the DSDT, unless cpu-hotplug is configured.

So why is this a kernel problem?

So what are you proposing should be the behaviour here? What this statement seems to be saying is that QEMU as it exists today only describes the first CPU in DSDT.

This confuses me somewhat, because I'm far from sure which machines this is true for in QEMU. I'm guessing it's a legacy thing with some old distro version of QEMU - so we'll have to paper over it anyway but for current QEMU I'm not sure it's true.

Helpfully there are a bunch of ACPI table tests so I've been checking through all the multi CPU cases.

CPU hotplug not enabled.
pc/DSDT.dimmpxm - 4x Processor entries. -smp 4 pc/DSDT.acpihmat - 2x Processor entries. -smp 2 q35/DSDT.acpihmat - 2x Processor entries. -smp 2 virt/DSDT.acpihmatvirt - 4x ACPI0007 entries -smp 4 q35/DSDT.acpihmat-noinitiator - 4 x Processor () entries -smp 4 virt/DSDT.topology - 8x ACPI0007 entries

Quoted 8 lines

I've also looked at the code and we have various types of CPU hotplug on x86 but they all build appropriate numbers of Processor() entries in DSDT.
Arm likewise seems to build the right number of ACPI0007 entries (and doesn't yet have CPU HP support).

If anyone can add a reference on why this is needed that would be very helpful.

Yes, it would.

Personally, I would prefer to assume that it is not necessary until it turns out that (1) there is firmware with this issue actually in use and (2) updating the firmware in question to follow the specification is not practical.

Quoted 2 lines

Otherwise, we'd make it easier to ship non-compliant firmware for no good reason.

If Salil can't come up with a reason, then I'm in favour of dropping the patch like already done for patch 2. If the code change serves no useful purpose, there's no point in making the change.

Quoted 70 lines · On Mon, 22 Jan 2024 17:30:05 +0000 "Russell King (Oracle)"…

Quoted 69 lines · Rafael J. Wysocki

Quoted 51 lines · Jonathan Cameron

Quoted 22 lines · On Mon, 15 Jan 2024 11:06:29 +0000 "Russell King (Oracle)"…

Quoted 21 lines · Rafael J. Wysocki

Quoted 20 lines · Russell King

From: James Morse <[email protected]>

ACPI has two descriptions of CPUs, one in the MADT/APIC table, the other in the DSDT. Both are required. (ACPI 6.5's 8.4 "Declaring Processors" says "Each processor in the system must be declared in the ACPI namespace"). Having two descriptions allows firmware authors to get this wrong.

If CPUs are described in the MADT/APIC, they will be brought online early during boot. Once the register_cpu() calls are moved to ACPI, they will be based on the DSDT description of the CPUs. When CPUs are missing from the DSDT description, they will end up online, but not registered.

Add a helper that runs after acpi_init() has completed to register CPUs that are online, but weren't found in the DSDT. Any CPU that is registered by this code triggers a firmware-bug warning and kernel taint.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Qemu TCG only describes the first CPU in the DSDT, unless cpu-hotplug is configured.

So why is this a kernel problem?

So what are you proposing should be the behaviour here? What this statement seems to be saying is that QEMU as it exists today only describes the first CPU in DSDT.

This confuses me somewhat, because I'm far from sure which machines this is true for in QEMU. I'm guessing it's a legacy thing with some old distro version of QEMU - so we'll have to paper over it anyway but for current QEMU I'm not sure it's true.

Helpfully there are a bunch of ACPI table tests so I've been checking through all the multi CPU cases.

CPU hotplug not enabled.
pc/DSDT.dimmpxm - 4x Processor entries. -smp 4 pc/DSDT.acpihmat - 2x Processor entries. -smp 2 q35/DSDT.acpihmat - 2x Processor entries. -smp 2 virt/DSDT.acpihmatvirt - 4x ACPI0007 entries -smp 4 q35/DSDT.acpihmat-noinitiator - 4 x Processor () entries -smp 4 virt/DSDT.topology - 8x ACPI0007 entries

Quoted 8 lines

I've also looked at the code and we have various types of CPU hotplug on x86 but they all build appropriate numbers of Processor() entries in DSDT.
Arm likewise seems to build the right number of ACPI0007 entries (and doesn't yet have CPU HP support).

If anyone can add a reference on why this is needed that would be very helpful.

Yes, it would.

Personally, I would prefer to assume that it is not necessary until it turns out that (1) there is firmware with this issue actually in use and (2) updating the firmware in question to follow the specification is not practical.

Quoted 7 lines

Quoted 2 lines

Otherwise, we'd make it easier to ship non-compliant firmware for no good reason.

If Salil can't come up with a reason, then I'm in favour of dropping the patch like already done for patch 2. If the code change serves no useful purpose, there's no point in making the change.

Salil's out today, but I've messaged him to follow up later in the week.

It 'might' be the odd cold plug path where QEMU half comes up, then extra CPUs are added, then it boots. (used by some orchestration frameworks) I don't have a set up for that and I won't get to creating one today anyway (we all love start of the year planning workshops!)

I've +CC'd a few people have run tests on the various iterations of this work in the past. Maybe one of them can shed some light on this?

Jonathan

Hi

Quoted 85 lines · Jonathan Cameron

Quoted 70 lines · On Mon, 22 Jan 2024 17:30:05 +0000 "Russell King (Oracle)"…

Quoted 69 lines · Rafael J. Wysocki

Quoted 51 lines · Jonathan Cameron

Quoted 22 lines · On Mon, 15 Jan 2024 11:06:29 +0000 "Russell King (Oracle)"…

Quoted 21 lines · Rafael J. Wysocki

Quoted 20 lines · Russell King

From: James Morse <[email protected]>

ACPI has two descriptions of CPUs, one in the MADT/APIC table, the other in the DSDT. Both are required. (ACPI 6.5's 8.4 "Declaring Processors" says "Each processor in the system must be declared in the ACPI namespace"). Having two descriptions allows firmware authors to get this wrong.

If CPUs are described in the MADT/APIC, they will be brought online early during boot. Once the register_cpu() calls are moved to ACPI, they will be based on the DSDT description of the CPUs. When CPUs are missing from the DSDT description, they will end up online, but not registered.

Add a helper that runs after acpi_init() has completed to register CPUs that are online, but weren't found in the DSDT. Any CPU that is registered by this code triggers a firmware-bug warning and kernel taint.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Qemu TCG only describes the first CPU in the DSDT, unless cpu-hotplug is configured.

So why is this a kernel problem?

So what are you proposing should be the behaviour here? What this statement seems to be saying is that QEMU as it exists today only describes the first CPU in DSDT.

This confuses me somewhat, because I'm far from sure which machines this is true for in QEMU. I'm guessing it's a legacy thing with some old distro version of QEMU - so we'll have to paper over it anyway but for current QEMU I'm not sure it's true.

Helpfully there are a bunch of ACPI table tests so I've been checking through all the multi CPU cases.

CPU hotplug not enabled.
pc/DSDT.dimmpxm - 4x Processor entries. -smp 4 pc/DSDT.acpihmat - 2x Processor entries. -smp 2 q35/DSDT.acpihmat - 2x Processor entries. -smp 2 virt/DSDT.acpihmatvirt - 4x ACPI0007 entries -smp 4 q35/DSDT.acpihmat-noinitiator - 4 x Processor () entries -smp 4 virt/DSDT.topology - 8x ACPI0007 entries

Quoted 8 lines

I've also looked at the code and we have various types of CPU hotplug on x86 but they all build appropriate numbers of Processor() entries in DSDT.
Arm likewise seems to build the right number of ACPI0007 entries (and doesn't yet have CPU HP support).

If anyone can add a reference on why this is needed that would be very helpful.

Yes, it would.

Personally, I would prefer to assume that it is not necessary until it turns out that (1) there is firmware with this issue actually in use and (2) updating the firmware in question to follow the specification is not practical.

Quoted 7 lines

Quoted 2 lines

Otherwise, we'd make it easier to ship non-compliant firmware for no good reason.

If Salil can't come up with a reason, then I'm in favour of dropping the patch like already done for patch 2. If the code change serves no useful purpose, there's no point in making the change.

Salil's out today, but I've messaged him to follow up later in the week.

Quoted 8 lines

It 'might' be the odd cold plug path where QEMU half comes up, then extra CPUs are added, then it boots. (used by some orchestration frameworks) I don't have a set up for that and I won't get to creating one today anyway (we all love start of the year planning workshops!)

I've +CC'd a few people have run tests on the various iterations of this work in the past. Maybe one of them can shed some light on this?

IIUC, this patch covers a scenario for non compliant firmware and in which my tests for AArch64 using RFC v2 have been unable to trigger its error message so far. This does not mean, however, this patch should not be taken forward though.

It seems benevolent enough detecting non compliant firmware and still proceed while having whoever uses that firmware to get to know that.

Quoted 70 lines · On Mon, 22 Jan 2024 17:30:05 +0000 "Russell King (Oracle)"…

Quoted 69 lines · Rafael J. Wysocki

Quoted 51 lines · Jonathan Cameron

Quoted 22 lines · On Mon, 15 Jan 2024 11:06:29 +0000 "Russell King (Oracle)"…

Quoted 21 lines · Rafael J. Wysocki

Quoted 20 lines · Russell King

From: James Morse <[email protected]>

ACPI has two descriptions of CPUs, one in the MADT/APIC table, the other in the DSDT. Both are required. (ACPI 6.5's 8.4 "Declaring Processors" says "Each processor in the system must be declared in the ACPI namespace"). Having two descriptions allows firmware authors to get this wrong.

If CPUs are described in the MADT/APIC, they will be brought online early during boot. Once the register_cpu() calls are moved to ACPI, they will be based on the DSDT description of the CPUs. When CPUs are missing from the DSDT description, they will end up online, but not registered.

Add a helper that runs after acpi_init() has completed to register CPUs that are online, but weren't found in the DSDT. Any CPU that is registered by this code triggers a firmware-bug warning and kernel taint.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Qemu TCG only describes the first CPU in the DSDT, unless cpu-hotplug is configured.

So why is this a kernel problem?

So what are you proposing should be the behaviour here? What this statement seems to be saying is that QEMU as it exists today only describes the first CPU in DSDT.

This confuses me somewhat, because I'm far from sure which machines this is true for in QEMU. I'm guessing it's a legacy thing with some old distro version of QEMU - so we'll have to paper over it anyway but for current QEMU I'm not sure it's true.

Helpfully there are a bunch of ACPI table tests so I've been checking through all the multi CPU cases.

CPU hotplug not enabled.
pc/DSDT.dimmpxm - 4x Processor entries. -smp 4 pc/DSDT.acpihmat - 2x Processor entries. -smp 2 q35/DSDT.acpihmat - 2x Processor entries. -smp 2 virt/DSDT.acpihmatvirt - 4x ACPI0007 entries -smp 4 q35/DSDT.acpihmat-noinitiator - 4 x Processor () entries -smp 4 virt/DSDT.topology - 8x ACPI0007 entries

Quoted 8 lines

I've also looked at the code and we have various types of CPU hotplug on x86 but they all build appropriate numbers of Processor() entries in DSDT.
Arm likewise seems to build the right number of ACPI0007 entries (and doesn't yet have CPU HP support).

If anyone can add a reference on why this is needed that would be very helpful.

Yes, it would.

Personally, I would prefer to assume that it is not necessary until it turns out that (1) there is firmware with this issue actually in use and (2) updating the firmware in question to follow the specification is not practical.

Quoted 7 lines

Quoted 2 lines

Otherwise, we'd make it easier to ship non-compliant firmware for no good reason.

If Salil can't come up with a reason, then I'm in favour of dropping the patch like already done for patch 2. If the code change serves no useful purpose, there's no point in making the change.

Salil's out today, but I've messaged him to follow up later in the week.

It 'might' be the odd cold plug path where QEMU half comes up, then extra CPUs are added, then it boots. (used by some orchestration frameworks)

I don't have a set up for that and I won't get to creating one today anyway (we all love start of the year planning workshops!)

I've +CC'd a few people have run tests on the various iterations of this work in the past. Maybe one of them can shed some light on this?

Jonathan

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

I poked this on x86 - it only applies with hotplug enabled anyway so same result as doing the hotplug later - All possible Processor() entries already exist in DSDT. Hence this isn't the source of the mysterious broken configuration.

If anyone does poke this path, the old discussion between James and Salil provides some instructions (mostly the thread is about another issue).
https://op-lists.linaro.org/archives/list/[email protected]/thread/DNAGB2FB5ALVLV2BYWYOCLKGNF77PNXS/

Also on x86 a test involving smp 2,max-cpus=4 and adding cpu-id 3 (so skipping 2) doesn't boot. (this is without Salil's QEMU patches). I guess there are some well known rules in there that I don't know about and QEMU isn't preventing people shooting themselves in the foot.

As I'm concerned, drop this patch.
If there are platforms out there doing this wrong they'll surface once we get this into more test farms (so linux-next). If we need this 'fix' we can apply it when we have a problem firmware to point at.

Quoted 22 lines · On Mon, 15 Jan 2024 11:06:29 +0000 "Russell King (Oracle)"…

Quoted 21 lines · Rafael J. Wysocki

Quoted 20 lines · Russell King

From: James Morse <[email protected]>

ACPI has two descriptions of CPUs, one in the MADT/APIC table, the other in the DSDT. Both are required. (ACPI 6.5's 8.4 "Declaring Processors" says "Each processor in the system must be declared in the ACPI namespace"). Having two descriptions allows firmware authors to get this wrong.

If CPUs are described in the MADT/APIC, they will be brought online early during boot. Once the register_cpu() calls are moved to ACPI, they will be based on the DSDT description of the CPUs. When CPUs are missing from the DSDT description, they will end up online, but not registered.

Add a helper that runs after acpi_init() has completed to register CPUs that are online, but weren't found in the DSDT. Any CPU that is registered by this code triggers a firmware-bug warning and kernel taint.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Qemu TCG only describes the first CPU in the DSDT, unless cpu-hotplug is configured.

So why is this a kernel problem?

So what are you proposing should be the behaviour here? What this statement seems to be saying is that QEMU as it exists today only describes the first CPU in DSDT.

This confuses me somewhat, because I'm far from sure which machines this is true for in QEMU. I'm guessing it's a legacy thing with some old distro version of QEMU - so we'll have to paper over it anyway but for current QEMU I'm not sure it's true.

Helpfully there are a bunch of ACPI table tests so I've been checking through all the multi CPU cases.

CPU hotplug not enabled.
pc/DSDT.dimmpxm - 4x Processor entries. -smp 4 pc/DSDT.acpihmat - 2x Processor entries. -smp 2 q35/DSDT.acpihmat - 2x Processor entries. -smp 2 virt/DSDT.acpihmatvirt - 4x ACPI0007 entries -smp 4 q35/DSDT.acpihmat-noinitiator - 4 x Processor () entries -smp 4 virt/DSDT.topology - 8x ACPI0007 entries

I've also looked at the code and we have various types of CPU hotplug on x86 but they all build appropriate numbers of Processor() entries in DSDT.
Arm likewise seems to build the right number of ACPI0007 entries (and doesn't yet have CPU HP support).

If anyone can add a reference on why this is needed that would be very helpful.

From: James Morse <[email protected]>

To allow ACPI to skip the call to arch_register_cpu() when the _STA value indicates the CPU can't be brought online right now, move the arch_register_cpu() call into acpi_processor_get_info().

Systems can still be booted with 'acpi=off', or not include an ACPI description at all. For these, the CPUs continue to be registered by cpu_dev_register_generic().

This moves the CPU register logic back to a subsys_initcall(), while the memory nodes will have been registered earlier.

Signed-off-by James Morse <[email protected]>

Reviewed-by Gavin Shan <[email protected]>

Tested-by Miguel Luis <[email protected]>

Tested-by Vishnu Pajjuri <[email protected]>

Tested-by Jianyong Wu <[email protected]>

Signed-off-by Russell King (Oracle) <[email protected]>

From: James Morse <[email protected]>

To allow ACPI to skip the call to arch_register_cpu() when the _STA value indicates the CPU can't be brought online right now, move the arch_register_cpu() call into acpi_processor_get_info().

Systems can still be booted with 'acpi=off', or not include an ACPI description at all. For these, the CPUs continue to be registered by cpu_dev_register_generic().

This moves the CPU register logic back to a subsys_initcall(), while the memory nodes will have been registered earlier.

Signed-off-by James Morse <[email protected]>

Reviewed-by Gavin Shan <[email protected]>

Tested-by Miguel Luis <[email protected]>

Tested-by Vishnu Pajjuri <[email protected]>

Tested-by Jianyong Wu <[email protected]>

Signed-off-by Russell King (Oracle) <[email protected]>

---
Changes since RFC v2:
* Fixup comment in acpi_processor_get_info() (Gavin Shan)
* Add comment in cpu_dev_register_generic() (Gavin Shan)

---
 drivers/acpi/acpi_processor.c | 12 ++++++++++++
 drivers/base/cpu.c            |  6 +++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

drivers/acpi/acpi_processor.c +12 -0

index 0511f2bc10bc..e7ed4730cbbe 100644
@@ -314,6 +314,18 @@ static int acpi_processor_get_info(struct acpi_device *device)
                        cpufreq_add_device("acpi-cpufreq");
        }
 
+       /*
+        * Register CPUs that are present. get_cpu_device() is used to skip
+        * duplicate CPU descriptions from firmware.
+        */
+       if (!invalid_logical_cpuid(pr->id) && cpu_present(pr->id) &&
+           !get_cpu_device(pr->id)) {
+               int ret = arch_register_cpu(pr->id);
+
+               if (ret)
+                       return ret;
+       }
+
        /*
         *  Extra Processor objects may be enumerated on MP systems with
         *  less than the max # of CPUs. They should be ignored _iff

drivers/base/cpu.c +5 -2

index 47de0f140ba6..13d052bf13f4 100644
@@ -553,7 +553,11 @@ static void __init cpu_dev_register_generic(void)
 {
        int i, ret;
 
-       if (!IS_ENABLED(CONFIG_GENERIC_CPU_DEVICES))
+       /*
+        * When ACPI is enabled, CPUs are registered via
+        * acpi_processor_get_info().
+        */
+       if (!IS_ENABLED(CONFIG_GENERIC_CPU_DEVICES) || !acpi_disabled)
                return;
 
        for_each_present_cpu(i) {
--

2.30.2

Quoted 6 lines · Russell King

From: James Morse <[email protected]>

To allow ACPI to skip the call to arch_register_cpu() when the _STA value indicates the CPU can't be brought online right now, move the arch_register_cpu() call into acpi_processor_get_info().

This kind of looks backwards to me and has a potential to become super-confusing.

I would instead add a way for the generic code to ask the platform firmware whether or not the given CPU is enabled and so it can be registered.

Quoted 7 lines

Systems can still be booted with 'acpi=off', or not include ano ACPI description at all. For these, the CPUs continue to be registered by cpu_dev_register_generic().

This moves the CPU register logic back to a subsys_initcall(), while the memory nodes will have been registered earlier.

Isn't this somewhat risky?

Quoted 56 lines

Signed-off-by James Morse <[email protected]>

Reviewed-by Gavin Shan <[email protected]>

Tested-by Miguel Luis <[email protected]>

Tested-by Vishnu Pajjuri <[email protected]>

Tested-by Jianyong Wu <[email protected]>

Signed-off-by Russell King (Oracle) <[email protected]>

---
Changes since RFC v2:
* Fixup comment in acpi_processor_get_info() (Gavin Shan)
* Add comment in cpu_dev_register_generic() (Gavin Shan)

---
 drivers/acpi/acpi_processor.c | 12 ++++++++++++
 drivers/base/cpu.c            |  6 +++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

drivers/acpi/acpi_processor.c +12 -0

index 0511f2bc10bc..e7ed4730cbbe 100644
@@ -314,6 +314,18 @@ static int acpi_processor_get_info(struct acpi_device *device)
                        cpufreq_add_device("acpi-cpufreq");
        }
 
+       /*
+        * Register CPUs that are present. get_cpu_device() is used to skip
+        * duplicate CPU descriptions from firmware.
+        */
+       if (!invalid_logical_cpuid(pr->id) && cpu_present(pr->id) &&
+           !get_cpu_device(pr->id)) {
+               int ret = arch_register_cpu(pr->id);
+
+               if (ret)
+                       return ret;
+       }
+
        /*
         *  Extra Processor objects may be enumerated on MP systems with
         *  less than the max # of CPUs. They should be ignored _iff

drivers/base/cpu.c +5 -2

index 47de0f140ba6..13d052bf13f4 100644
@@ -553,7 +553,11 @@ static void __init cpu_dev_register_generic(void)
 {
        int i, ret;
 
-       if (!IS_ENABLED(CONFIG_GENERIC_CPU_DEVICES))
+       /*
+        * When ACPI is enabled, CPUs are registered via
+        * acpi_processor_get_info().
+        */
+       if (!IS_ENABLED(CONFIG_GENERIC_CPU_DEVICES) || !acpi_disabled)
                return;
 
        for_each_present_cpu(i) {
--

Quoted 3 lines

2.30.2

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 11 lines · On Mon, 18 Dec 2023 21:30:50 +0100 "Rafael J. Wysocki" <[email protected]>…

Quoted 6 lines · Russell King

From: James Morse <[email protected]>

To allow ACPI to skip the call to arch_register_cpu() when the _STA value indicates the CPU can't be brought online right now, move the arch_register_cpu() call into acpi_processor_get_info().

This kind of looks backwards to me and has a potential to become super-confusing.

Quoted 3 lines

I would instead add a way for the generic code to ask the platform firmware whether or not the given CPU is enabled and so it can be registered.

Hi Rafael,

The ACPI interpreter isn't up at this stage so we'd need to pull that forwards. I'm not sure if we can pull the interpreter init early enough.

Perhaps pushing the registration back in all cases is the way to go? Given the acpi interpretter is initialized via subsys_initcall() it would need to be after that - I tried pushing cpu_dev_register_generic() immediately after acpi_bus_init() and that seems fine.

We can't leave the rest of cpu_dev_init() that late because a bunch of other stuff relies on it (CPU freq blows up first as a core_init() on my setup).

So to make this work we need it to always move the registration later than the necessary infrastructure, perhaps to subsys_initcall_sync() as is done for missing CPUs (we'd need to combine the two given that needs to run after this, or potentially just stop checking for acpi_disabled and don't taint the kernel!). I think this is probably the most consistent option on basis it at least moves the registration to the same point whatever is going on and can easily use the arch callback you suggest to hide away the logic on deciding if a CPU is there or not.

Quoted 6 lines · Russell King

Systems can still be booted with 'acpi=off', or not include an ACPI description at all. For these, the CPUs continue to be registered by cpu_dev_register_generic().

This moves the CPU register logic back to a subsys_initcall(), while the memory nodes will have been registered earlier.

Isn't this somewhat risky?