Paths

Table of Contentst

loader.efi: Fix arm64 PE metadata
ClosedPublic
Actions

Authored by markj on Dec 20 2022, 9:36 PM.

Details

Reviewers

manu
allanjude

Commits

rG753f15c24fba: loader.efi: Fix some arm64 PE metadata
rG21d56b796634: loader.efi: Fix some arm64 PE metadata

Summary

Mark the file as an executable in the COFF header.
Provide separate .text and .data sections.
Provide sane file and section alignment values. These values are the defaults defined in the PE specification.
Set appropriate characteristics for each of .text and .data.

This is required for the MS devkit to load our UEFI image.

Obtained from: OpenBSD

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Event Timeline

allanjude created this revision.Dec 20 2022, 9:36 PM

Herald added a reviewer: manu. · View Herald TranscriptDec 20 2022, 9:36 PM

Herald added subscribers: dab, bcran, emaste and 2 others. · View Herald Transcript

Harbormaster returned this revision to the author for changes because remote builds failed.Dec 20 2022, 9:37 PM

Harbormaster failed remote builds in B48779: Diff 114340!

I wonder if we can rework this to link with --target aarch64-unknown-uefi yet and ditch these hacks and the objcopy nonsense....

Otherwise this looks to be largely the same as what OpenBSD does, and looks like it should be good. We should make sure that things still boot on other arm64 boxes :)

fuz mentioned this in D37931: stand/efi/loader/copy.c: make sure kernel image is executable.Jan 2 2023, 9:27 PM

So I think this is fine, but we really should investigate the llvm uefi targets.I don't have time to do that right now, but it shouldn't gate this....

andrew added inline comments.Jan 19 2023, 7:50 AM

stand/efi/loader/arch/arm64/start.S
44	Do you need this?
66	Do we need all these flags? I guess only `IMAGE_FILE_EXECUTABLE_IMAGE` (0x0002) is needed. The other flags are deprecated or don't appear to apply.

we really should investigate the llvm uefi targets

We'd have to build the PE bits in lld, which we don't do right now.

Overall this LGTM with a couple of nits as others have noted. _etext, __data_size, ALIGN(4096) all seem like real improvements

stand/efi/loader/arch/arm64/start.S
66	`IMAGE_FILE_DEBUG_STRIPPED` is still valid, and probably applicable here? `IMAGE_FILE_LINE_NUMS_STRIPPED` (0x0004) is deprecated and should not be set.

emaste added inline comments.Jan 24 2023, 9:18 PM

stand/efi/loader/arch/arm64/start.S
126	not sure why this comment is duplicated

In D37765#862245, @imp wrote:

So I think this is fine, but we really should investigate the llvm uefi targets.I don't have time to do that right now, but it shouldn't gate this....

I investigated. Seems like everybody else is using some hacky thing to make this happen on aarch64. Linux has its own home-built tools, EDK2 has its (different) tools, and the other BSDs generally have copied this file (or have one that's substantially similar).

In D37765#867530, @emaste wrote:

we really should investigate the llvm uefi targets

We'd have to build the PE bits in lld, which we don't do right now.

And even if we did, they seem incomplete for our purposes... We should revisit in the future though.

I just imported what OpenBSD had from https://github.com/openbsd/src/commit/09ce6c0661b90c6c2535c9b67b87f0366c17d369

allanjude foisted this revision upon markj.Feb 19 2023, 2:10 PM

allanjude added a reviewer: allanjude.

fuz added a subscriber: fuz.Mar 4 2023, 6:58 PM

And even if we did, they seem incomplete for our purposes...

LLD should provide a fully functional PE/COFF linker, but either way it's something to look at later.

kevans added a subscriber: kevans.Mar 20 2023, 5:15 PM

fuz mentioned this in D38031: Add the fixed memory type to the pci ecam driver.Apr 3 2023, 9:38 PM

markj marked 2 inline comments as done.Apr 6 2023, 5:42 PM

markj added inline comments.

stand/efi/loader/arch/arm64/start.S
44	I don't think it's required, no.
66	Some reading suggests that the DEBUG_STRIPPED flag really means that debug info is available in a separate file. In my testing, only EXECUTABLE_IMAGE needs to be set.

markj retitled this revision from Give to PE/COFF file proper .text and .data sections, and fill in the characteristics field in the COFF header. Makes our bootloader work in the x13s. With help from mlarkin@. to loader.efi: Fix arm64 PE metadata.Apr 6 2023, 5:42 PM

markj edited the summary of this revision. (Show Details)

I tested this on the devkit, EC2 graviton and an ampere system.

I don't see any issues with this off-hand, though it would probably be good to arrange a test with U-Boot to ensure it's also OK.

In D37765#898590, @kevans wrote:

I don't see any issues with this off-hand, though it would probably be good to arrange a test with U-Boot to ensure it's also OK.

Do you have any suggestions on a test configuration? I think I can boot using uboot->loader.efi on my espressobin, so I could try that. I'm not sure if I can test this in qemu.

In D37765#900834, @markj wrote:

In D37765#898590, @kevans wrote:

I don't see any issues with this off-hand, though it would probably be good to arrange a test with U-Boot to ensure it's also OK.

Do you have any suggestions on a test configuration? I think I can boot using uboot->loader.efi on my espressobin, so I could try that. I'm not sure if I can test this in qemu.

I was able to boot with these modifications on a netgate SG-1100, which is basically an espressobin. The u-boot -> loader.efi -> kernel handoff worked fine.

This revision was not accepted when it landed; it landed in state Changes Planned.Apr 18 2023, 6:49 PM

This revision was landed with ongoing or failed builds.

Closed by commit rG21d56b796634: loader.efi: Fix some arm64 PE metadata (authored by markj). · Explain Why

This revision was automatically updated to reflect the committed changes.

markj added a commit: rG21d56b796634: loader.efi: Fix some arm64 PE metadata.

markj added a commit: rG753f15c24fba: loader.efi: Fix some arm64 PE metadata.May 29 2023, 1:14 PM