Page MenuHomeFreeBSD

loader: lua: assume late ACPI detection if the feature isn't enabled
ClosedPublic

Authored by kevans on Nov 23 2023, 6:59 AM.
Tags
None
Referenced Files
Unknown Object (File)
May 8 2024, 5:08 PM
Unknown Object (File)
May 8 2024, 1:13 PM
Unknown Object (File)
Apr 21 2024, 7:58 PM
Unknown Object (File)
Apr 16 2024, 5:26 AM
Unknown Object (File)
Mar 14 2024, 8:21 PM
Unknown Object (File)
Mar 14 2024, 8:21 PM
Unknown Object (File)
Mar 11 2024, 7:23 AM
Unknown Object (File)
Mar 11 2024, 7:23 AM
Subscribers

Details

Summary

While we're here, enable the feature in the places we detect ACPI. This
lets us side-step the existing issues and provide a path forward for
folks upgrading from previous releases that haven't updated their ESP
yet.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

stand/lua/core.lua
144

This seems a bit hinky...

Old binary: will return false. This causes us in core.getACPI to return false.
which causes us in recordDefaults to setACPI(false) which disables it.
Now we need the acpi_load=yes part of that function for sure, but not the rest.

I agree with the point you raised on IRC: why do this dance? We should default to loading ACPI always, and only set disabled in response to the menu item.

167–168

These two feel wrong.

stand/lua/core.lua
144

whoops, there was supposed to be a not in front of that

stand/lua/core.lua
167–168

I went to unravel this, and I think I understand now. On all platforms, we call nexus_acpi_probe() -> acpi_identify(), in acpi_identify we'll end up printing some additional noise if we haven't hinted acpi away:

https://cgit.freebsd.org/src/tree/sys/dev/acpica/acpi.c#n422

I can see where that might sound scary if you're a user, when the reality is likely that we just don't have any ACPI tables -- we can't really distinguish at the moment between acpi_Startup() failing because we don't have ACPI or failing due to other errors in initialization.

While we're there, I just noticed this:

https://cgit.freebsd.org/src/tree/sys/dev/acpica/acpi.c#n383

So right now, by default, if we detected ACPI in loader then we're forcing it down the users' throat even if we wanted to distrust ACPI because of BIOS age:

https://cgit.freebsd.org/src/tree/sys/i386/acpica/acpi_machdep.c#n103

Fix the sense of the feature test

Fix core.setACPI to actually load the acpi module, though it's likely built in
on the platforms that need it. env vars aren't checked for foo_load. The hint
remains because ACPI makes a lot of noise even when we haven't detected it
unless we've disabled it via hint.

So, we should start to unravel the whininess of ACPI in the kernel. Let it decide and not force it to be disabled if it doesn't exist. That's an orthogonal problem, though.
This isn't perfect, but it's way better than the kludge I put in, and I think 100% perfection is not desirable due to its long-term cost.

tl;dr: Shit It

stand/lua/core.lua
144

This looks better. We have two cases to consider, and this "breaks" one of them (imho, it's OK):

(1) The platform is an ACPI platform (x86, arm, riscv64). We do the right thing. Old binaries won't set this feature, and we'll rightly assume the platform has ACPI.
(2) The platform is powerpc. We'll wrongly assume it has ACPI.

Case 1 is the right thing to do. Old binaries with new /boot/lua works, ditto old /boot/lua and new loader.

Case 2 isn't the wrong thing to do. We. fail to disable ACPI and enable loading of ACPI. The loading will fail so the loader will complain about that (this is the 'breakage'). However, the system will keep going and the only breakage is the whine about ACPI. So we fail safe for all combinations.

We bogusly set hint.acpu.0.disabled for all platforms that don't support ACPI at all, but bogus only in the sense that it will never be used. We'd need a 'can have acpi' function for that. But having a hint that's not used on some obscure platforms is fine: it's a less bad problem than having this code get super complicated to support this weird edge case.

This revision is now accepted and ready to land.Dec 8 2023, 4:15 PM