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.
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 41911 Build 38799: arc lint + arc unit
Event Timeline
Comment Actions
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".
Comment Actions
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.... |
Comment Actions
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.
Comment Actions
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.
Comment Actions
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:
- Use OpenFirmware device-tree or ACPI table?
- 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.
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.
Comment Actions
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.
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.
Comment Actions
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.
Comment Actions
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).
Comment Actions
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.
Comment Actions
I cannot tell you what and how arm platforms interpret statement "UEFI boot", but on amd64 the efi_boot == true means two things:
- There is functional ESP partition, and management tools can look for it (used by installer)
- 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 | ||
|---|---|---|
| 1302–1303 | How could firmware_type not be FW_NONE there? | |
| 1303 | If doing this way, the condition should be firmware_type == FW_BIOS | |
| 1308 | 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 | ||
| 49 ↗ | (On Diff #95333) | This needs to be static |
| 62 ↗ | (On Diff #95333) | 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. | |
Comment Actions
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 | ||
|---|---|---|
| 1302–1303 | 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. | |
| 1303 | That changes the logic if firmware_type == FW_HYPER (of course that change could be correct). | |
| 1308 | 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 | ||
| 49 ↗ | (On Diff #95333) | Okay (marking as done, though Phabricator may not be immediately updated). |
| 62 ↗ | (On Diff #95333) | 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. | |
| sys/amd64/amd64/machdep.c | ||
|---|---|---|
| 1308 | Always spot the typo afterwards. s/FW_TYPE/firmware_type/ | |
Comment Actions
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.
Comment Actions
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.
| sys/amd64/amd64/machdep.c | ||
|---|---|---|
| 1302–1303 | 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 ↗ | (On Diff #95347) | 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 ↗ | (On Diff #95347) | Initialize bootmethod with "other" |
| 59 ↗ | (On Diff #95347) | This part is not needed I believe. At most you could MPASS(firmware_type < nitems(types)). |
| 62 ↗ | (On Diff #95333) | Most obvious place is kern/subr_param.c |
Comment Actions
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 ↗ | (On Diff #95347) | 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 ↗ | (On Diff #95347) | 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. |
Comment Actions
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 | ||
|---|---|---|
| 1302–1303 | 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. | |
Comment Actions
Updating to match current D32132. Small reorg to increase similarity to current vm_guest formatting.
Comment Actions
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).