Kernel: Implement ACPI-based shutdown (revival attempt) #25653

RatcheT2497 · 2025-01-16T15:50:20Z

This is an attempt at reviving IdanHo's PR #19556 , which has gone stale.

Original description:
This includes a couple of bug-fixes, and the pretty big pre-requisite of an AML parser, but we can finally shutdown some* baremetal PCs :^)

* Those that don't require AML execution, which is not part of this PR.

Memory::map_typed maps the physical address as readable, not writable, so we would get a kernel page fault and panic when trying to access SystemMemory backed generic addresses. This patch switches to using a writable mapping instead.

It is not strictly specified if SystemMemory addreses should use the access_size or bit_width field of the generic address structure, and in practice both are used in the wild. Support the bit width case as well.

This patch adds a minimal implementation of an AML parser - minimal in the sense that it only supports the subset of AML encodings required to parse QEMU's AML blob. Adding the rest of the encodings that might be used on other systems is left as a TODO for the future. This should allow us to parse the AML blobs that are part of the ACPI DSDT and SSDT tables, which will let us implement proper ACPI shutdown on bare-metal systems. Note that this is only a parser, not an interpreter. Executing AML methods is another issue we will have to tackle in the future, once we start looking into more fine-grained power management.

Now that we have a bare-bones AML parser we can use it to actually parse the AML blobs that are contained in the ACPI DSDT and SSDT tables. These AML blobs should contain the values we will need to implement ACPI shutdown.

Now that we have the contents of the ACPI AML blobs parsed, all we need to do is to write them to the right place. Note that another step we are missing is calling the '_PTS' AML method before actually initiating the shutdown. (This is supposed to let the platform controller know our intent and give it time to do any preparations it needs to before-hands) The specification does still allow doing the shutdown without calling it, but it's still strongly encouraged. (as some platforms might not be fully compatible and require it to properly shutdown) Since QEMU does not actually have a _PTS method, I've left it as a TODO for the future.

This makes sure we actually try shutting down via ACPI if it's available when shutdown is initiated by the user.

Now that we support ACPI/AML based shutdown, enable it by default.

Hendiadyoin1

All the stuff I found quickly going over this, all nits, I guess

Hendiadyoin1 · 2025-01-16T18:43:37Z

Kernel/Firmware/ACPI/AML/AST.cpp

+    for (auto i = 0u; i < indent; ++i) {
+        dbg("  ");
+    }


IIRC we can use dbg("{:>{}}","",indent)
not sure if thats better

Also see the device tree printer, that uses a similar pattern (IIRC)

Hendiadyoin1 · 2025-01-16T18:48:03Z

Kernel/Firmware/ACPI/AML/Opcodes.h

+// 20.3 AML Byte Stream Byte Values
+
+static constexpr u8 AliasOp = 0x06;


NIT: I prefer enum class OpCode : u8 for this

Hendiadyoin1 · 2025-01-16T18:54:40Z

Kernel/Firmware/ACPI/AML/Parser.h

+    [[nodiscard]] u8 consume()
+    {
+        auto byte = current();
+        m_offset++;
+        return byte;
+    }
+    void skip() { m_offset++; }


To this pattern:
a memory stream would be nicer, and would make some helpers redundant
If you want to change it, feel free to do that in a separate commit to make ease the commit management

Hendiadyoin1 · 2025-01-16T18:56:02Z

Kernel/Firmware/ACPI/AML/AST.h

Not sure if we should expose this whole thing as a userspace library, like the fdt stuff
That would make testing simpler, like through lagom

Hendiadyoin1 · 2025-01-16T18:59:41Z

Kernel/Firmware/ACPI/Parser.cpp

+    SLP_TYPa = static_cast<AML::IntegerData&>(*elements[0]).value();
+    SLP_TYPb = static_cast<AML::IntegerData&>(*elements[1]).value();


bit_cast<...>(...) is preferable

Hendiadyoin1 · 2025-01-16T19:00:04Z

Kernel/Firmware/ACPI/Parser.cpp

+    auto s5_object = MUST(m_root_namespace->get_node({ "_S5"sv }));
+    if (!s5_object->is_define_package())
+        return false;
+    auto elements = static_cast<AML::DefinePackage&>(*s5_object).element_list();


Hendiadyoin1 · 2025-01-16T19:03:33Z

Kernel/Firmware/ACPI/AML/Parser.h

+enum class IntegerBitness {
+    IntegersAre32Bit,
+    IntegersAre64Bit
+};


Do not like this name, but do not have a better one
(FDT was nice enough to always count in cells, so it was cell_size there, but it does not make sense in AML i guess)

Well that's an ACPI limitation, ACPI 2.0 added support for 64-bit addresses. Before that only 16-bit and 32-bit systems were supported.

Quick Idea

enum class IntSize { I32, I64 };

?
(Late night phone review)

Bitness is a real word or what do you not like about the name?
I think this name is fine.

Hendiadyoin1 · 2025-01-16T19:04:03Z

Kernel/Firmware/ACPI/Definitions.h

+static constexpr size_t PM1_CNT_SLP_TYP_offset = 10;
+static constexpr u16 PM1_CNT_SLP_EN = 1u << 13;
+
+static constexpr size_t SLEEP_CONTROL_SLP_TYP_offset = 2;
+static constexpr u8 SLEEP_CONTROL_SLP_EN = 1u << 5;


Can you find spec links for these?

Side note: IIRC the spec links in here might be out of date/dead

d-tatianin · 2025-01-17T09:42:31Z

I think this part is fundamentally flawed and should not be merged as is:

Note that this is only a parser, not an interpreter. Executing AML
methods is another issue we will have to tackle in the future, once
we start looking into more fine-grained power management.

Top level code found in DSDT/SSDT/PSDT is exactly the same as "executing AML methods", because it is a method, just one with an extra rule: named objects created inside are not destroyed upon exit. Other than that it is a method, that can use Locals, branches, Whiles, call into other methods etc.

How is this "parser" going to handle conditionally declared objects? (even QEMU AML does this for some Sx objects iirc)

If (S5EN) {
    Name (_S5, ...)
}

You cannot just parse this, same as you cannot just parse python bytecode, because you're supposed to execute it.

IdanHo added 7 commits January 16, 2025 15:49

Kernel: Support accessing ACPI memory addresses based on bit width

602ed13

It is not strictly specified if SystemMemory addreses should use the access_size or bit_width field of the generic address structure, and in practice both are used in the wild. Support the bit width case as well.

Kernel: Try using ACPI shutdown if available during poweroff

c951d91

This makes sure we actually try shutting down via ACPI if it's available when shutdown is initiated by the user.

Kernel: Switch default ACPI feature level to enabled instead of limited

f29bd83

Now that we support ACPI/AML based shutdown, enable it by default.

github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Jan 16, 2025

Hendiadyoin1 reviewed Jan 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kernel: Implement ACPI-based shutdown (revival attempt) #25653

Kernel: Implement ACPI-based shutdown (revival attempt) #25653

RatcheT2497 commented Jan 16, 2025

Hendiadyoin1 left a comment

Hendiadyoin1 Jan 16, 2025

Hendiadyoin1 Jan 16, 2025

Hendiadyoin1 Jan 16, 2025

Hendiadyoin1 Jan 16, 2025

Hendiadyoin1 Jan 16, 2025

Hendiadyoin1 Jan 16, 2025

Hendiadyoin1 Jan 16, 2025

Hendiadyoin1 Jan 16, 2025

spholz Jan 16, 2025

Hendiadyoin1 Jan 16, 2025

spholz Jan 16, 2025

Hendiadyoin1 Jan 16, 2025

Hendiadyoin1 Jan 16, 2025

d-tatianin commented Jan 17, 2025

		// 20.3 AML Byte Stream Byte Values

		static constexpr u8 AliasOp = 0x06;

		SLP_TYPa = static_cast<AML::IntegerData&>(*elements[0]).value();
		SLP_TYPb = static_cast<AML::IntegerData&>(*elements[1]).value();

Kernel: Implement ACPI-based shutdown (revival attempt) #25653

Are you sure you want to change the base?

Kernel: Implement ACPI-based shutdown (revival attempt) #25653

Conversation

RatcheT2497 commented Jan 16, 2025

Hendiadyoin1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-tatianin commented Jan 17, 2025