sys: unify boot/firmware variables

[ehem_freebsd_m5p.com]

Summary

x86 had "bootmethod", ARM64 had "arm64_bus_method". Unify these to the
enum "firmware_type". This allows machine-independent drivers to
identify which interface to use.

[ehem_freebsd_m5p.com]

This is the approach I strongly prefer to D31065, fix the root of the problem. I don't how often hardware clocks are on ARM64 UEFI systems, but such could be present. In which case being able to access the clock inside Xen would be useful.

Since I lack such hardware, this is untested beyond it compiles. This is also sort of fallout from D28619.

I don't know whether machdep.bootmethod is worth preserving, but I could see installation scripts wanting it. I doubt reporting "XEN" triggers a special case for such. I'm unsure whether it is even worth having FW_HYPER since this starts overlapping with "vm_guest".

[imp]

I kinda like this, but kinda don't... IT seems to conflate too many things together into firmware.

sys/arm64/arm64/mp_machdep.c

672 ↗

(On Diff #95141)

I'm not so sure on this....
UEFI is orthogonal to DEVTREE vs ACPI for bus enumeration.
We can make UEFI callbacks for both. These part of the change seem wrong.

[ehem_freebsd_m5p.com]

I agree to an extent, but I feel the distinction isn't worth preserving. Notably if there was a real issue with this there should already be far more values for the variables involved, yet those don't exist.

"bootmethod" and "arm64_bus_method" only have 3 defined states. Since there is now a driver which needs to be made machine-independent (sys/dev/xen/efi/pvefi.c), merging these together is now quite desirable.

[andrew]

This is confusing two mostly independent concepts. One is the firmware type. On arm64 it is almost always UEFI, however it could also boot via the same ABI as a Linux kernel. The other is the hardware description method.

[kib]

On amd64 there is already 'efi_boot' var.

[ehem_freebsd_m5p.com]

In D31955#720853, @andrew wrote:

This is confusing two mostly independent concepts. One is the firmware type. On arm64 it is almost always UEFI, however it could also boot via the same ABI as a Linux kernel. The other is the hardware description method.

Yes, they are somewhat independent, but they are interrelated and strongly correlate.

Of the spots touched by this they all appear to ask one of two questions:

1. Use OpenFirmware device-tree or ACPI table?
2. Are UEFI runtime services available?

I'm under the impression using device-trees pretty well excludes full UEFI runtime services. The only case where these mix at all is U-Boot's UEFI support, which mostly appears to be good enough for bootloader use and only minimally implement the UEFI spec.
As such this seems to be a spectrum with device-trees on one end and full UEFI the other.

In D31955#720857, @kib wrote:

On amd64 there is already 'efi_boot' var.

That actually concerns me. So AMD64 has two global variables, "bootmethod" and "efi_boot" which serve overlapping purposes? This suggests the situation already needs to be cleaned up.

I'll concede it might need two variables. My real concern is making this architecture independent as this resolves the situation with sys/dev/xen/efi/pvefi.c rather more elegantly than the approach of D31065 does.

[mhorne]

In D31955#721084, @ehem_freebsd_m5p.com wrote:

In D31955#720853, @andrew wrote:

This is confusing two mostly independent concepts. One is the firmware type. On arm64 it is almost always UEFI, however it could also boot via the same ABI as a Linux kernel. The other is the hardware description method.

Yes, they are somewhat independent, but they are interrelated and strongly correlate.

Of the spots touched by this they all appear to ask one of two questions:

1. Use OpenFirmware device-tree or ACPI table?
2. Are UEFI runtime services available?

I'm under the impression using device-trees pretty well excludes full UEFI runtime services. The only case where these mix at all is U-Boot's UEFI support, which mostly appears to be good enough for bootloader use and only minimally implement the UEFI spec.
As such this seems to be a spectrum with device-trees on one end and full UEFI the other.

Why should the use of a device tree exclude the presence UEFI runtime services? The u-boot devs are cramming in a lot of new UEFI functionality these days.

In D31955#720857, @kib wrote:

On amd64 there is already 'efi_boot' var.

That actually concerns me. So AMD64 has two global variables, "bootmethod" and "efi_boot" which serve overlapping purposes? This suggests the situation already needs to be cleaned up.

I'll concede it might need two variables. My real concern is making this architecture independent as this resolves the situation with sys/dev/xen/efi/pvefi.c rather more elegantly than the approach of D31065 does.

I would suggest that you just implement the machdep.bootmethod sysctl and accompanying bootmethod variable on arm64. This idea has been brought up before, and there are scripts in the base system that check this sysctl already (e.g. bsdinstall), so it is established and not going away on amd64. The values could be {"UEFI", "LinuxABI", "XEN"}, or something along these lines.

If there are some UEFI runtime services that are not implemented on the Xen host, say get/set var, what would happen? It looks like the functions in pvefi.c would just propagate any errors and behave sensibly.

[ehem_freebsd_m5p.com]

In D31955#721437, @mhorne wrote:

In D31955#721084, @ehem_freebsd_m5p.com wrote:

In D31955#720857, @kib wrote:

On amd64 there is already 'efi_boot' var.

That actually concerns me. So AMD64 has two global variables, "bootmethod" and "efi_boot" which serve overlapping purposes? This suggests the situation already needs to be cleaned up.

I'll concede it might need two variables. My real concern is making this architecture independent as this resolves the situation with sys/dev/xen/efi/pvefi.c rather more elegantly than the approach of D31065 does.

I would suggest that you just implement the machdep.bootmethod sysctl and accompanying bootmethod variable on arm64. This idea has been brought up before, and there are scripts in the base system that check this sysctl already (e.g. bsdinstall), so it is established and not going away on amd64. The values could be {"UEFI", "LinuxABI", "XEN"}, or something along these lines.

You should note the first version did something along this line. I preserved machdep.bootmethod as a sysctl derived from the value of firmware_type (see sys/x86/x86/cpu_machdep.c).

This seems an argument for making this even more architecture-independent. In which case where should set_bootmethod() and firmware_type be placed? New file in sys/kern?

I'm also unsure of where to get the appropriate values on ARM64. There are some reasonable spots to set firmware_type = FW_UEFI;, but not too many good spots for the other values.

[ehem_freebsd_m5p.com]

Updating, merging in efi_boot, leaving out arm64_bus_method. Broke machdep.bootmethod away from the x86 area, so now added to ARM64 (though unavailable in most cases).

[ehem_freebsd_m5p.com]

Issues I'm wondering about with the current revision...

I'm wondering about native_parse_memmap(). I'm unsure it helps that much to avoid the redundancy and I'm wondering whether that should even be part of this patch.

I added bootmethod.c to files.arm64 and files.x86 since there is presently no support on other architectures. Does the core feel this should be put in files to encourage implementing similar functionality on the other architectures?

I'm wondering whether setting firmware_type should be added to native_parse_preload_data(). This would remove the if after that call and simply have a rule of it should always be set in the init_ops.parse_preload_data() call.

Now there is the problem ARM64 never had support for reporting the boot method, and I don't see good spots for detecting anything besides FW_UEFI.

[ehem_freebsd_m5p.com]

Fixing the diff split, total delta is same.

[kib]

I cannot tell you what and how arm platforms interpret statement "UEFI boot", but on amd64 the efi_boot == true means two things:

1. There is functional ESP partition, and management tools can look for it (used by installer)
2. There is no BIOS or CSM, so we must not touch miscellaneous low memory locations expecting that to affect reset/reboot/resume behavior of the platform.

This explicitly excludes e.g. answering the question "are runtime services present".

sys/amd64/amd64/machdep.c

1636–1637

How could firmware_type not be FW_NONE there?

1637

If doing this way, the condition should be firmware_type == FW_BIOS

1642

firmware_type = preload_search_info(kmdp, MODINFO_METADATA | MODINFOMD_EFI_MAP) != NULL ? FW_UEFI : FW_BIOS;

sys/amd64/amd64/mp_machdep.c

396

In all that cases, check for == FW_BIOS, and not != FW_UEFI

sys/kern/bootmethod.c

50

This needs to be static

63

This simply does not deserve a dedicated file.

sys/sys/systm.h

80

I do not think sys/systm.h is the right trash bin for this definition. More, I suspect usermode might want to see this enum, in which case sys/systm.h is even more inappropriate.

[ehem_freebsd_m5p.com]

My goal is fixing sys/dev/xen/efi/pvefi.c to work with ARM64 kernel builds. Ideally this increases similarity between FreeBSD architectures. I was guessing some of the bits were depending on runtime services, but I could well be mistaken.

sys/amd64/amd64/machdep.c

1636–1637

If xen_pvh_parse_reload_data() (as init_ops.parse_preload_data()) sets firmware_type = FW_HYPER;.

Your asking this seems to suggest you think init_ops.parse_preload_data() should always set firmware_type even though that results in a bit of code duplication (both native_parse_preload_data() and xen_pvh_parse_reload_data() end up with this conditional). I had been wondering whether to do that instead.

1637

That changes the logic if firmware_type == FW_HYPER (of course that change could be correct).

1642

What you typed works if inside the outer if. firmware_type = preload_search_info(kmdp, MODINFO_METADATA | MODINFOMD_EFI_MAP) != NULL ? FW_UEFI : firmware_type == FW_NONE ? FW_BIOS : FW_TYPE; if you want to capture everything.

I was emulating existing style and shying away from the conditional operator.

sys/amd64/amd64/mp_machdep.c

396

Same issue as above, #define efi_boot (firmware_type == FW_UEFI) would preserve existing logic. Now I can believe the current logic is wrong, but you're certain this is a correct change?

sys/kern/bootmethod.c

50

Okay (marking as done, though Phabricator may not be immediately updated).

63

That doesn't surprise me, but I was unsure of where to place this. Advice on where you think it should be placed would be appreciated.

sys/sys/systm.h

80

I was following vm_guest since that is what I was looking at. State where you think it should go instead and I'll be happy to adjust.

[ehem_freebsd_m5p.com]

sys/amd64/amd64/machdep.c

1642

Always spot the typo afterwards. s/FW_TYPE/firmware_type/

[ehem_freebsd_m5p.com]

Putting in the one requested adjustment. My impression is the slightly different approach is desired, so putting that in.

I had been thinking several hypervisors had slightly specialize boot approaches, but on further checking I believe Xen is the only one. As such put Xen back in as a distinct case. Yet another potential tweak.

[ehem_freebsd_m5p.com]

In other news in a test Xen VM on ARM64:

# sysctl machdep.bootmethod
machdep.bootmethod: UEFI
#

As the initial boot was a Tianocore/EDK2 image, that is the expected result. As such this does appear to give expected results on ARM64, for the one case which can be tested.

[kib]

sys/amd64/amd64/machdep.c

1636–1637

So does xen pv emulate low bios data? I highly doubt it, but I cannot deny it.

I do believe that the answer is no, and think that all existing uses of if (!efi_boot) should be translated into if (firmware_type == FW_BIOS).

That said, why do you need FW_HYPER? Also why not FW_XEN?

sys/kern/bootmethod.c

40

If you change bootmethod from array to pointer, you would not need strlcpy() in sysinit. Also you can pre-init it with "other":

const char *bootmethod = "other";

56

Initialize bootmethod with "other"

59

This part is not needed I believe. At most you could MPASS(firmware_type < nitems(types)).

63

Most obvious place is kern/subr_param.c

[ehem_freebsd_m5p.com]

Presently I still need a recommendation for where to place the firmware_type declaration. @kib, you didn't like systm.h, but I'm unsure where else to place it.

Now an interesting bit with moving handling of the setting the sysctl value to subr_param.c is it looks reasonable to merge the setting of machdep.bootmethod with the setting of kern.vm_guest (both use similar constructs). I'll likely need a bit to get to implementing that though.

sys/kern/bootmethod.c

40

Based on what Clang complained about when trying this, I'm pretty sure SYSCTL_STRING() needs a fixed address so a pointer doesn't work. I would have preferred this approach, but if it was setup in a way which makes this impossible.

59

Originally I wasn't expecting the list to be exhaustive and wanted to allow for empty entries. Of the current list though, clearly I've got them all, so perhaps that should be gotten rid of.

[ehem_freebsd_m5p.com]

Now making use of D32132 for handling reporting of the enum to userspace. This seems more elegant, though means some portions of review now apply to D32132 instead.

[ehem_freebsd_m5p.com]

I'll concede several/most of the places checking for FW_UEFI should likely instead be checking for FW_BIOS (Xen not emulating that portion of the environment). Thing is, I believe fixing those is distinct from D31955.

My reason for creating D31955 is to make sys/dev/xen/efi/pvefi.c architecture-independent. For which the global x86 "bootmethod" and "efi_boot" variables need to be dealt with.

sys/amd64/amd64/machdep.c

1636–1637

Originally I was thinking set firmare_type = FW_HYPER; vm_guest = VM_GUEST_XEN; would be the way to handle this. The response would be Xen is the only one which features direct kernel loading. Other hypervisors load an emulated BIOS or UEFI environment, and use that to load FreeBSD or other OS. In this case Xen really is a genuinely distinct firmware_type/bootmethod and deserves a distinct value.

So, having thought about it, the current version has FW_XEN, rather than FW_HYPER.

[ehem_freebsd_m5p.com]

sys/kern/subr_param.c

178–189 ↗

(On Diff #95700)

Note, this is matches the previous version of D32132. Current version of D32132 changes this slightly, but I'm unsure whether that slightly more advanced version will be accepted or not, so presently I'm leaving this alone.

[ehem_freebsd_m5p.com]

Updating to match current D32132. Small reorg to increase similarity to current vm_guest formatting.

[ehem_freebsd_m5p.com]

Any reviews? @kib wants reviews from ARM and/or Xen side of FreeBSD.

I suspect @kib is correct and those various tests should in fact be firmware_type == FW_BIOS, rather than firmware_type != FW_UEFI. I though think that should be distinct from D31955. Mainly the topic of D31955 is attempting to merge down the number of boot method variables and make the single remaining one architecture-independent (thus making the "pvefi.c" file architecture-independent), while leaving the logic unchanged.

This successfully detects UEFI on ARM, resulting in the sysctl value of machdep.bootmethod being "UEFI" in that type of boot. Otherwise it should get an empty string (if someone can point to an appropriate spot for selecting "LinuxABI" that can be added).