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

Add support for 64-bit machines with 32-bit UEFI implementations #1098

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

VexedUXR
Copy link
Contributor

@VexedUXR VexedUXR commented Feb 3, 2024

Some machines (mostly baytrail notebooks) have 64-bit capable cpus but are stuck on
32-bit uefi firmware, with so CSM.

Add support for them by making the amd64 loader compile itself again with DO32=1,
(see the amd64 Makefile.inc) all of the extra ia32 specific code is in loader/arch/amd64/ia32.
The loader can be disabled using MK_LOADER_IA32.

Tested on qemu and the Acer one 10 s1002
Also cross-compiled to armv6 to make sure nothing broke.

@VexedUXR
Copy link
Contributor Author

VexedUXR commented Feb 3, 2024

Cant reproduce build errors locally.
Maybe this can be fixed by using __aligned to force padding?
Edit: Instead of casting

@VexedUXR
Copy link
Contributor Author

VexedUXR commented Feb 3, 2024

Clang seems to ignore -Wint-to-pointer-cast and -Wpointer-to-int-cast when -m32 is specified.
Can reproduce errors with gcc toolchain.

@bsdimp
Copy link
Member

bsdimp commented Feb 3, 2024

This makes it easier for me to look at this and to have a conversation. I got too busy last time and naybe this way we'll keep momentum.

@VexedUXR
Copy link
Contributor Author

VexedUXR commented Feb 3, 2024

Fixed.
Everything looks fine now

@VexedUXR
Copy link
Contributor Author

VexedUXR commented Feb 4, 2024

Fixed rsdp cast

@bsdimp
Copy link
Member

bsdimp commented Feb 6, 2024

OK. I've ordered an inexpensive laptop to see if I can use it to test this.
Generally I like this, but it's all one commit
and I've left a few comments, but I'm thinking about others
Some of this I think that I can land quickly, but other bits need a little refinement. If things are broken down better I can do that. I'll tag a couple of things first.

Copy link
Member

@bsdimp bsdimp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get started at breaking this down into smaller chunks, but I think we can do that as a series of commits. Remember to git push --force-with-lease to update the changes and to rebase forward please. Thanks!

share/man/man5/src.conf.5 Outdated Show resolved Hide resolved
stand/defs.mk Show resolved Hide resolved
stand/efi/libefi32/Makefile Show resolved Hide resolved
stand/efi/loader/Makefile Outdated Show resolved Hide resolved
stand/efi/loader/arch/amd64/ia32/amd64_tramp32.S Outdated Show resolved Hide resolved
stand/efi/loader/bootinfo.c Outdated Show resolved Hide resolved
usr.sbin/bsdinstall/scripts/bootconfig Outdated Show resolved Hide resolved
@VexedUXR
Copy link
Contributor Author

VexedUXR commented Feb 7, 2024

Alright, I split everything into a total of 5 commits:

  1. Allow overriding NEWVERSWHAT
  2. Add the LOADER_IA32 build option
  3. Add libefi32
  4. The main commit
  5. Script changes, copy the ia32 loader

I put LOADER_IA32 on its own since it doesn't really fit with adding libefi32.
Also about the comment in defs.mk, I ended up moving the comment explaining DO32
up to its first use.

Of course, let me know if you want any of these reverted, thanks!

@bsdimp
Copy link
Member

bsdimp commented Feb 7, 2024

I'll take a look at this.
IIRC, I can use qemu-system-x86_64 to test this if I load the i386 EDKII firmware, right?
Thanks for the updates.

@VexedUXR
Copy link
Contributor Author

VexedUXR commented Feb 7, 2024

IIRC, I can use qemu-system-x86_64 to test this if I load the i386 EDKII firmware, right?

Yea, thats how I did it

@VexedUXR
Copy link
Contributor Author

Just a little cleanup: Use NULL instead of 0.
Also, put the 32-bit compatibility fixes in their own commit.

@bsdimp
Copy link
Member

bsdimp commented Mar 12, 2024

I'll try to get to this before we have the 14.1 freeze.

@VexedUXR
Copy link
Contributor Author

I'll try to get to this before we have the 14.1 freeze.

Great :)

@VexedUXR
Copy link
Contributor Author

VexedUXR commented Mar 13, 2024

I've changed all of the __uefi64__ to __i386__ and fixed the merge conflicts, I also updated the linker script stuff in accordance with 5b3b9a5. I added my name to the copyright notice at the top of the amd64_tramp32.S file since I rewrote most of it, let me know if you dont what that.

Also, thers a typo at https://github.com/freebsd/freebsd-src/blob/main/stand/liblua/lutils.h#L37 and I have no idea why it causes the 32-bit loader to crash, but not the 64-bit one, probably some linker weirdness.

Edit: Oh, I didn't realize I had to request a review manually.

@VexedUXR VexedUXR requested a review from bsdimp March 22, 2024 20:20
@bsdimp
Copy link
Member

bsdimp commented Mar 23, 2024

I thought I'd fixed that typo... It should make it not compile at all..

@VexedUXR
Copy link
Contributor Author

Wouldn't it just make a set called _LUA_COMPILE_SET instead of Xlua_compile_set

As long as the set isn't accessed outside the macros, there shouldn't be any problems
Of course in our case, it is causing problems since we objcopy -j set_Xlua_compile_set

@VexedUXR
Copy link
Contributor Author

I was actually interested in finding out why it only crashed on the 32-bit loader, turns out its fairly straightforward.

With the 32-bit loader, the addend is expected to be present in the set_Xlua_compile_set section, which doesn't exist in this case. The address this section is expected to start at is just full of uninitialized data, so when LUA_FOREACH_SET is used we end up jumping to some garbage and crashing. With the 64-bit loader we get the addend from the relocation entry, so thats not a problem.

Ah, also, while looking into this I noticed that the i386 script I re-added wasn't up to date with d024bc7, I'll check the rest of the script and push a fix soon.

stand/efi/loader/framebuffer.c Outdated Show resolved Hide resolved
stand/efi/loader/framebuffer.c Outdated Show resolved Hide resolved
stand/efi/loader/framebuffer.c Outdated Show resolved Hide resolved
stand/loader.mk Outdated Show resolved Hide resolved
stand/loader.mk Outdated Show resolved Hide resolved
tools/boot/install-boot.sh Outdated Show resolved Hide resolved
usr.sbin/bsdinstall/scripts/bootconfig Outdated Show resolved Hide resolved
stand/efi/loader/arch/amd64/Makefile.inc Outdated Show resolved Hide resolved
stand/efi/loader/arch/amd64/ia32/elf64_freebsd32.c Outdated Show resolved Hide resolved
stand/efi/loader/arch/amd64/ia32/elf64_freebsd32.c Outdated Show resolved Hide resolved
@VexedUXR
Copy link
Contributor Author

Pushed the changes that aren't being discussed / that I don't have questions about.
Also rebased to eb690a0

freebsd-git pushed a commit that referenced this pull request May 29, 2024
This can be useful when making alternate versions of the loader.

Reviewed by: imp
Pull Request: #1098
freebsd-git pushed a commit that referenced this pull request May 29, 2024
Prevent G(4) and over from overflowing for 32-bit builds.

Reviewed by: imp
Pull Request: #1098
@bsdimp
Copy link
Member

bsdimp commented May 29, 2024

OK. I landed a little bit of this PR, and updated the PR to rebase it and resolve the conflicts that I created due to minor tweaking I did. Hope that doesn't mess up any other WIP around this.

@VexedUXR
Copy link
Contributor Author

VexedUXR commented Jun 3, 2024

Okay, a lot of changes this time. Hopefully I don't forget anything.
Instead of scattering everything across a bunch of review comments, I'll just put everything in one place.

We can't get addresses over 4G from UEFI

The UEFI spec requires all memory to be identity mapped if paging is enabled. It also allows PAE, but that doesn't change the size of the virtual address space. So we don't have to worry about getting addresses over 4G.

elf64_freebsd32.c cleanup

Given that last point, we can simplify this file and imo make it more readable. There was no functional change here just some refactoring. There are still some externs in the file though, same as the original elf64_freebsd.c. It would be good to move those but I don't know what header those externs would go in. I also copied the G(x) and M(x) macros from copy.c to use them here, that should also be in a header but I'm not sure which one. (Maybe save this for another PR?)

Using SUBDIR instead of explicitly calling make

This one took me a few tries, as seen here. I ended up just creating a new makefile for each loader, e.g loader_lua-ia32. I was reluctant to do it this way in the beginning since it involved a lot of duplicated makefiles, but I feel like every other way I found to do this either had some caveat or was close to working but would fail on make install (also they were all pretty janky). I did actually find one other way that I didn't post here, but it involved messing with one of auto.obj.mk's internal variables so I feel like that was a no-go.

Cleanup/Fixing the copying of the ia32 loader in the scripts

This worked* before, but it would just jankily copy the ia32 loader along with the 64-bit one if it found it. In order to make this better, I needed a way to figure out which one we needed. I was using efibootmgr then checking if it failed to figure out if we booted from the ia32 loader. This had two problems: 1. It looked pretty cryptic, so I probably needed to comment what was happening each time I used it. 2: One could simply not have efirt loaded and it would give a false positive. In order to get around this I added a new sysctl, machdep.efi_arch which would report the UEFI architecture we booted from using metadata given by the loader. This made it much simpler to detect what we booted from (though there are still some weird cases, which I'll get onto now). This works just fine in the bsdinstall script, we simply check the sysctl then copy the required loader. In install-boot.sh however, it gets a little more complicated. We try to figure out which loader we need by checking a few things. If updatesystem is set (which implies that we are booting on the same system the script is running on) and the machdep.efi_arch sysctl is i386, we can safely assume that we only need the ia32 loader. We also check for an existing loader, if there is one, we use file to figure out weather it's the ia32 one or not then copy the new one accordingly. However, if we can't figure out which one we need, we just copy both. The 64-bit one goes to loader.efi and the 32-bit one goes to loader-ia32.efi.

That turned out longer than expected, hopefully it wasn't too long.
Hopefully I can catch a Cirrus CI run :)
Thanks.

@VexedUXR VexedUXR requested review from bsdimp and jrtc27 June 3, 2024 12:56
@VexedUXR
Copy link
Contributor Author

VexedUXR commented Jun 3, 2024

Kernel build failures seem unrelated (it doesn't even get to buildkernel).
Can confirm that my local buildworld and buildkernel work.

Edit: Ah, looks like its failing on other PRs too

Edit2: Looks like everything builds and runs fine. Cirrus-CI just complains about src.conf.5 not being regenerated.

Edit3: I see the style issues, I'll fix them the next time I push.

@@ -0,0 +1,5 @@

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra blank line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't a blank line left at the top of the other Makefiles? (At least, the ones that don't start with comments)
If that's not an enforced style, then yeah I'll remove it.

@VexedUXR
Copy link
Contributor Author

VexedUXR commented Jun 3, 2024

Moved everything to arch/i386.
Also made a single loader_ia32 instead of all the variations (It just uses LOADER_DEFAULT_INTERP). Let me know if I misunderstood what you meant though.

I'm not requesting a review this time since I'm not sure it it would mark this as resolved.
Looks like it won't

@VexedUXR VexedUXR requested review from jrtc27, emaste and bsdimp June 3, 2024 23:43
In preparation for supporting 64-bit machines with 32-bit UEFI firmware,
add a build option for compiling the ia32 loader. Currently unused.
In preparation for supporting 64-bit machines with 32-bit UEFI firmware,
add a 32-bit variant of libefi since we need to compile both the 64-bit version and
the 32-bit version at the same time.
Using AllocateMaxAddress here means that gfx_state->tg_shadow_fb is
treated as the highest address we can receive. Since gfx_state->tg_shadow_fb
is NULL, we never receive anything. Use AllocateAnyPages instead.
main.c - Fix rsdp cast.
framebuffer.c -
	- Use temp variable instead of directly passing pointer when EFI_PHYSICAL_ADDRESS is expected.
	  Also fix FreePages cast.
	- Mask framebuffer address given to us by UEFI.
Some machines have 64-bit capable cpus but are stuck on 32-bit uefi firmware.

Add support for them by building a new "loader_ia32" with
LOADER_DEFAULT_INTERP along with the 64-bit one. The loader
can be disabled using MK_LOADER_IA32.
With the new 32-bit UEFI loader, it's convenient to have a sysctl to
figure out how we booted. Can be accessed at machdep.efi_arch
This handles copying in install-boot.sh and bsdinstall's bootconfig.

In install-boot.sh we try to figure which loader we need. If we do, it's
copied to .../EFI/freebsd/loader.efi. However, if we can't, we copy
both. The 64-bit one goes to loader.efi and the 32-bit one goes to
loader_ia32.efi

bsdconfig simply checks the machdep.efi_arch variable and copies
accordingly.
@VexedUXR
Copy link
Contributor Author

VexedUXR commented Jun 4, 2024

Some small cleanup. Diff
Usually I'd keep these local until the next set of changes, but I figured these might get mentioned in a review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants