Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

arm64: Force Morello kernels to use the FDT #2316

Merged
merged 1 commit into from
Feb 10, 2025

Conversation

markjdb
Copy link
Contributor

@markjdb markjdb commented Feb 5, 2025

No description provided.

@markjdb markjdb requested a review from bsdjhb February 5, 2025 15:23
@jrtc27
Copy link
Member

jrtc27 commented Feb 5, 2025

Older firmware doesn’t provide an FDT, and being EDK2 won’t load one. We install one as part of bsdinstall, and enforce it in loader.conf, but that doesn’t help the installer itself. On such systems we still have to boot with ACPI, and the lack of GPU drivers isn’t a problem because it can use efifb for the VT.

Also, QEMU uses ACPI. It maybe can also provide an FDT, but we want to use ACPI there, both because it’s the fancier thing and because it tests ACPICA in CI.

@jrtc27
Copy link
Member

jrtc27 commented Feb 5, 2025

I think what we want is for GENERIC-MORELLO to just reverse the order back to what it used to be.

@markjdb
Copy link
Contributor Author

markjdb commented Feb 5, 2025

I think what we want is for GENERIC-MORELLO to just reverse the order back to what it used to be.

I need to test in QEMU, but I believe this change has exactly that effect: if there's an FDT, we'll use it, otherwise we'll fall back to the default, which is ACPI.

@bsdjhb
Copy link
Collaborator

bsdjhb commented Feb 5, 2025

The code in question does fall back to auto-guessing and will find ACPI if there is no FDT table:

	env = kern_getenv("kern.cfg.order");
	if (env != NULL) {
		order = env;
		while (order != NULL) {
			if (has_acpi &&
			    strncmp(order, "acpi", 4) == 0 &&
			    (order[4] == ',' || order[4] == '\0')) {
				arm64_bus_method = ARM64_BUS_ACPI;
				break;
			}
			if (has_fdt &&
			    strncmp(order, "fdt", 3) == 0 &&
			    (order[3] == ',' || order[3] == '\0')) {
				arm64_bus_method = ARM64_BUS_FDT;
				break;
			}
			order = strchr(order, ',');
			if (order != NULL)
				order++;	/* Skip comma */
		}
		freeenv(env);

		/* If we set the bus method it is valid */
		if (arm64_bus_method != ARM64_BUS_NONE)
			return (true);
	}
	/* If no order or an invalid order was set use the default */
	if (arm64_bus_method == ARM64_BUS_NONE) {
		if (has_acpi)
			arm64_bus_method = ARM64_BUS_ACPI;
		else if (has_fdt)
			arm64_bus_method = ARM64_BUS_FDT;
	}

@bsdjhb
Copy link
Collaborator

bsdjhb commented Feb 5, 2025

Though the CI failures seem to imply that the code didn't work the way I thought it would.

@bsdjhb
Copy link
Collaborator

bsdjhb commented Feb 5, 2025

I do wonder if the static environment is overriding the environment from the loader (in which case the kernel won't "see" the ACPI tables)? I thought Kyle fixed it so that we merged the static and dynamic environments, but perhaps not?

@jrtc27
Copy link
Member

jrtc27 commented Feb 5, 2025

	/*
	 * If no option was set the default is valid, otherwise we are
	 * setting one to get cninit() working, then calling panic to tell
	 * the user about the invalid bus setup.
	 */
	return (env == NULL);

So it'll fall back on picking the right default, but since acpi isn't listed in the env it'll panic as soon as it's used that to configure the console to tell the user that it couldn't find the requested method.

@jrtc27
Copy link
Member

jrtc27 commented Feb 5, 2025

(That is, like I said, it needs to be a reversal of the order, not requesting only fdt)

Otherwise, after upstream commit 33f2cf4 we boot using ACPI tables, which
prevents the panfrost driver from attaching.
@markjdb markjdb merged commit 8a70a45 into CTSRD-CHERI:dev Feb 10, 2025
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants