Detect ACPI early by FADT, not SPCR

[dan.kotowski_a9development.com]

Summary

Since EDK2 commit d8e36289cef7bde628b023219cd65fa8e8d4562a, the Graphical console may completely hide SPCR, causing panics later when locating timers. As such we should rely on a different mandatory table. Because has_acpi is set by walking the XSDT, we cannot rely on RSDP, RSDT, or XSDT.

Test Plan

Tested on SolidRun LX2160a Honeycomb with https://github.com/SolidRun/lx2160a_uefi at commit 78307bd9d2c5cfdda54a262b25bee062625dc4cd

[manu]

Looks ok to me.
I can't find anything in the acpi spec that say that FADT is mandatory but I guess it is ?
Maybe we need a function acpi_is_present that just try to get the rsdp pointer and returns a boolean if valid ?

[andrew]

I think we can just check for the existence of the ACPI tables, i.e. by checking if AcpiOsGetRootPointer returns a non-zero value.

[dan.kotowski_a9development.com]

AcpiOsGetRootPointer just wraps resource_long_value in sys/bus.h which we already include in sys/arm64/arm64/machdep.c. Since that returns 0 when successful, what about something like this?

long acpi_root;
has_acpi = (resource_long_value("acpi", 0, "rsdp", &acpi_root) == 0);

[jhb]

In the description, do you mean has_acpi is set "before" walking the XDST (rather than "by")? If so, I think Andrew's suggestion of AcpiOsRootPointer() would be good. I think it's a bit more readable than a bare resource_int_fetch, and it more closely matches what OF does for example (calling OF_peer() rather than making use of it's implementation details).

[dan.kotowski_a9development.com]

We could do that, but it seems like more code than necessary to do the same exact thing.

static u_long
acpi_get_root_from_loader(void)
{
	long acpi_root;

	if (resource_long_value("acpi", 0, "rsdp", &acpi_root) == 0)
		return (acpi_root);

	return (0);
}

ACPI_PHYSICAL_ADDRESS
AcpiOsGetRootPointer(void)
{

	if (acpi_root_phys == 0)
		acpi_root_phys = acpi_get_root_from_loader();

	return (acpi_root_phys);
}

[jhb]

I looked and on x86 we use acpi_identify() for this sort of thing, though a bit later when deciding which nexus driver to attach (on x86, we either use an ACPI "nexus0" device when using ACPI, or we use a legacy "nexus0" device). acpi_identify() honors 'hint.acpi.0.disabled=1' as well as applying sanity checks on the RSDP and RSDT/XSDT. Here, kern.cfg.order seems to duplicate some of that functionality but in a different way.

In terms of the amount of code executed, all of it is fairly short assuming arm64 never grows a fallback to search memory for the RSDP but always relies on the boot loader providing it (and if arm64 ever grew that, you'd have to go replicate it here anyway), so to me readability is the main factor, and has_acpi = (AcpiOsRootPointer() != 0); seems the most readable (and consistent with the OF_peer() line above).

[dan.kotowski_a9development.com]

You're right, that's much easier to read and makes more sense long term. And I just retested and it works fine in all combinations of serial and video.

Should I update this diff or reject and recreate?

[manu]

In D27306#611573, @dan.kotowski_a9development.com wrote:

You're right, that's much easier to read and makes more sense long term. And I just retested and it works fine in all combinations of serial and video.

Should I update this diff or reject and recreate?

Update this diff, it will be easier in the future to have all the discussion.

[dan.kotowski_a9development.com]

Using AcpiOsGetRootPointer() instead of FADT or SPCR