Details

Reviewers

markj
emaste
alc
tsoome
manu
kevans

Commits

rGf75caed644a5: amd64 UEFI loader: stop copying staging area to 2M physical
rG041b7317f75e: Add pmap_vm_page_alloc_check()
rGe18380e34141: amd64: do not assume that kernel is loaded at 2M physical
rGd6717f877872: amd64: rework AP startup
rG48216088b115: amd64: do not touch low memory in AP startup unless we used legacy boot
rG6a3821369f43: amd64: make efi_boot global
rGc8bae074d9e1: amd64: add pmap_alloc_page_below_4g()
rG34516d4ad19b: amd64 pti init: fix calculation of the kernel text start

Summary

instead activate kernel in-place whenever the EFI boot env found a free chunk.

The advantages are that we:

do not silently destroy EFI RT code/data or any other memory that BIOS still needs at runtime, if it happens to be located at 2M/kernel size.
do not corrupt trampoline if it happens to be allocated at the location that is copied to
the 1G 1:1 all over VA trick is eliminated, kernel and other loaded blobs can be located anywhere below 4G
the AP startup is streamlined, AP now directly loads %cr3 with pointer to kernel page table without intermediate page, which removed the need to reserve low memory at early boot, and made startup compatible with the new boot scheme

The new kernel is compatbile with an old loader.

Loader code currently support both old and new handoff scheme. Loader automaticly detects supported handoff scheme for booting kernel. A new loader command copy_staging does that manually. Do copy_staging enable to switch loader into legacy mode, copy_staging disable sets new mode, 'auto' uses autodetected. Without argument, it prints current mode.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

I tested this patch on my Sandy Bridge Samsung laptop. It doesn't boot.

I tested both the previous version and today's updated version of the patch by applying it to the main git branch and building an amd64-memstick image.

With copy_staging disabled (default), the laptop reboots immediately after the bootloader displays the EFI framebuffer information.
With copy_staging enabled, the laptop freezes after the EFI framebuffer information is displayed, but does not reboot (same situation as before without D30828 applied).
In legacy mode (BIOS instead of UEFI), the laptop boots fine like it did before this patch.

I fixed my problem: when constructing the page table, the index "i" was defined as int, and it overflowed. After changing the type of "i" from int to uint64, it boots.

I added a few more comments, but only to the parts of code that I read.
Are fixes from D30828 going to be implemented?

stand/efi/loader/arch/amd64/elf64_freebsd.c
108	"i" would otherwise overflow during virtual page table construction
170	It would be great if return values like this were not ignored.
249	i was defined as int and could overflow
265	i overflowed when defined as int
266

In D31121#701485, @dasebek_gmail.com wrote:

Are fixes from D30828 going to be implemented?

I think that it would still be useful, for copy case which is not going away in near future. But I believe that more work is needed for D30828; for now I fixate on nocopy and then we will discuss D30828

stand/efi/loader/arch/amd64/elf64_freebsd.c
249	Indeed, there was an overflow, but it is not there. i itself cannot overflow, NPTEPG is just 512.
266	I explicitly casted i to pd_entry_t, I believe this is the self-explaining way of fixing the issue.

Loader:

Overflow fix.
Check and report errors allocating trampoline and trampoline page table.

dasebek_gmail.com added inline comments.Jul 14 2021, 3:23 PM

stand/efi/loader/bootinfo.c
402	I looked at the UEFI specification, and it says that some boot services may actually be gone even if ExitBootServices() does not return success. I looked at: UEFI 2.0, which does not seem to mention what happens when ExitBootServices() returns a failure UEFI 2.3.1 mentions that only GetMemoryMap() may be called after the first call to ExitBootServices() UEFI 2.8 mentions that only Memory Allocation Services may be called after the first call to ExitBootServices() So it seems like GetMemoryMap() is the only boot service function that can be expected to work after ExitBootServices() is called and fails. My laptop freezes, whenever printf() or panic() is called after ExitBootServices() because printing to screen apparently uses boot services. On my laptop, ExitBootServices() succeeds the first time it is called. I wonder if eliminating all calls to boot service functions other than GetMemoryMap() after the first unsuccessful call to ExitBootServices() would fix boot freeze problems on some computers.
stand/efi/loader/copy.c
371–372	I don't think it is possible to print messages on the screen after ExitBootServices() has been called.

kib marked an inline comment as done.Jul 14 2021, 3:56 PM

kib added inline comments.

stand/efi/loader/copy.c
371–372	On screen perhaps not, on the directly driven serial port we can (which is e.g. my test configuration). There not much can be done anyway at this point.

grahamperrin added a subscriber: grahamperrin.Jul 15 2021, 3:41 PM

Add autodetection of the kernel type WRT copying of the staging area. Infer it from the type and size of the kernphys symbol. I am very happy with this hack because it allows us to not depend on any special notes or rather expensive and fragile mechanism. Instead, loader directly tests for the present functionality.

WIth this added, patch should be functionally complete, except Xen is not sorted out.

kib edited the summary of this revision. (Show Details)Jul 16 2021, 1:39 PM

emaste added a subscriber: royger.Jul 16 2021, 1:40 PM

Restore automatic state of copy_staging if kernel exec failed.

Tested, to the best of my ability, with an HP ProBook 440 G7 – one of the HP models that feature in bug 255073.

I'm delighted to confirm that the computer boots, with a build of 14.0-CURRENT (main-n247985-d1a0eab9fbc). Observations, thanks to some private guidance:

if I copy_staging enable at the loader prompt, then boot leads to the symptom that was originally reported in bug 255073 – boot fails
the kernel panics in response to service zfs onestart (I imagine that this is off-topic; I'll update the OS).

The kernel panic after service zfs onestart seems to be caused by this patch. I can reproduce it every time I run the command from an installation media on a KVM virtual machine.

Without this patch, service zfs onestart succeeds.
With this patch and copy_staging enable, the service also starts.
With this patch, and with the default value of copy_staging, the kernel panics every time I run service zfs onestart. The error message is panic: vm_radix_insert: key 7fffffffc16 is already present.

In D31121#702429, @dasebek_gmail.com wrote:

The kernel panic after service zfs onestart seems to be caused by this patch. I can reproduce it every time I run the command from an installation media on a KVM virtual machine.

Without this patch, service zfs onestart succeeds.

With this patch and copy_staging enable, the service also starts.

With this patch, and with the default value of copy_staging, the kernel panics every time I run service zfs onestart. The error message is panic: vm_radix_insert: key 7fffffffc16 is already present.

This is another bug due to compat mapping of zero address at KERNBASE. As result kernel page table pages indexes were off by one, which matters for promotion of KVA mappings. zfs.ko has large enough text (>4M) to trigger promotion on load.

Fix indexes for kernel page table pages.

Herald added a subscriber: delphij. · View Herald TranscriptJul 17 2021, 3:04 PM

markj added inline comments.Jul 17 2021, 4:54 PM

stand/efi/loader/copy.c
239	I think some enum for these values would be worthwhile.
241
376
379
sys/amd64/amd64/machdep.c
1630 ↗	(On Diff #92306)	Out of paranoia, perhaps CR3_PCID_SAVE should be masked off too.
sys/amd64/amd64/pmap.c
1813 ↗	(On Diff #92306)
2088 ↗	(On Diff #92306)	Should be conditional on `zeroed` too, otherwise we might as well not specify `zeroed` at all.

kib marked 8 inline comments as done.Jul 17 2021, 5:36 PM

kib added inline comments.

sys/amd64/amd64/machdep.c
1630 ↗	(On Diff #92306)	I am quite sure that we cannot read CR3_PCID_SAVE back from %CR3. It is not defined in SDM as a bit in the register, Intel only talks about 'operation that has bit 63 set in the source operand' as something that preserves TLB. In fact, we already relay on that in when storing SAVED_UCR3. Also, loader does not enable PCID at all, and I am sure that firmware cannot safely pass control to OS with PCID enabled either. I can add it if you insist, but this is excessive IMO.

Add symbolic names for copy_staging values.
Fix pmap_page_alloc_below_4g() to use zeroed for bzero() call.
Comments fixes and style.

markj added inline comments.Jul 17 2021, 5:43 PM

sys/amd64/amd64/machdep.c
1630 ↗	(On Diff #92306)	Ok, I don't insist at all. I just wondered if it would be sensible to mask off any upper flag bits, but I didn't look at the CR3 layout closely.

markj added inline comments.Jul 19 2021, 4:49 PM

stand/efi/loader/copy.c
385	I wonder if this should be larger. We allocate at least 1 page per CPU, for example, for pcpu. Doesn't that come from this region?
sys/amd64/amd64/machdep.c
1637 ↗	(On Diff #92312)	Logically, shouldn't it be `(vm_paddr_t)((vm_offset_t)hammer_time - KERNBASE)`?

Set slop to 8M for amd64, and 0 for other arches.
Add 'staging_slop' loader command to set slop to any value.

Make kernphys calculation pedantically type-correct.

In vmparam.h, the KERNLOAD definition implies that the kernel is still always loaded at 0x200000.

sys/amd64/amd64/machdep.c
1688 ↗	(On Diff #92446)	IMO it would be nice to provide a KERNSTART or kernvirt (corresponding to kernphys) equal to KERNBASE+NBPDR.
sys/amd64/amd64/mpboot.S
98 ↗	(On Diff #92446)	Why does PGE now need to be set here? In particular, the load_cr4() in init_secondary_tail() which sets CR4_PGE no longer flushes the TLB, I believe.

KERNSTART
Remove second set of CR4_PGE
Update comment about KERNLOAD
Do not corrupt low memory doing AP startup if EFI boot

markj accepted this revision.Jul 22 2021, 5:58 PM

markj added inline comments.

stand/efi/loader/copy.c
218	Strange indentation here.

This revision is now accepted and ready to land.Jul 22 2021, 5:58 PM

kib marked an inline comment as done.Jul 23 2021, 4:37 PM

kib added inline comments.

sys/amd64/amd64/machdep.c
1688 ↗	(On Diff #92446)	Yes, I pondered this as well.
sys/amd64/amd64/mpboot.S
98 ↗	(On Diff #92446)	Because I am switching to the kernel page table which in some configurations have PG_G bit set in PTEs. If not enabled there, CPU reacts with #gp when paging is enabled. I removed reload of %cr4 for PGE from the amd64-specific block in init_secondary_tail(). There is no need to flush TLB, we are already on the right page table.

Closed by commit rG34516d4ad19b: amd64 pti init: fix calculation of the kernel text start (authored by kib). · Explain WhyJul 24 2021, 3:53 PM

This revision was automatically updated to reflect the committed changes.

kib added a commit: rG34516d4ad19b: amd64 pti init: fix calculation of the kernel text start.

kib added a commit: rGc8bae074d9e1: amd64: add pmap_alloc_page_below_4g().

kib added a commit: rG6a3821369f43: amd64: make efi_boot global.

kib added a commit: rG48216088b115: amd64: do not touch low memory in AP startup unless we used legacy boot.

Rebase after some trivial bits were committed.

Herald added a reviewer: manu. · View Herald TranscriptJul 24 2021, 3:57 PM

Herald added subscribers: jhibbits, andrew. · View Herald Transcript

kib added a reviewer: kevans.Jul 24 2021, 3:59 PM

This revision was not accepted when it landed; it landed in state Needs Review.Jul 27 2021, 5:20 PM

Closed by commit rGd6717f877872: amd64: rework AP startup (authored by kib). · Explain Why

This revision was automatically updated to reflect the committed changes.

kib added a commit: rGd6717f877872: amd64: rework AP startup.

Rebase after AP startup bits were committed upstream.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 31 2021, 1:54 PM

Closed by commit rGe18380e34141: amd64: do not assume that kernel is loaded at 2M physical (authored by kib). · Explain Why

This revision was automatically updated to reflect the committed changes.

kib added a commit: rGe18380e34141: amd64: do not assume that kernel is loaded at 2M physical.

kib added a commit: rG041b7317f75e: Add pmap_vm_page_alloc_check().

Rebase after kernphys commit

kevans accepted this revision.Aug 8 2021, 5:56 AM

This revision is now accepted and ready to land.Aug 8 2021, 5:56 AM

Closed by commit rGf75caed644a5: amd64 UEFI loader: stop copying staging area to 2M physical (authored by kib). · Explain WhyAug 8 2021, 7:53 PM

This revision was automatically updated to reflect the committed changes.

kib added a commit: rGf75caed644a5: amd64 UEFI loader: stop copying staging area to 2M physical.

@kib @emaste Hey guys :) First of all thanks for your work. Ed, i highlight you because you commented on bug 209821 and bug 256722, also tagging Konstantin as author.

I saw that you merged the patches to stable/13 and aren't planning to merge them to stable/12. We currently use 12.2 and don't plan to upgrade to 13 soon, since we have a policy to always wait for at least a .2- or even better a .3-release for stability reasons.
We currently use the 12.1 efi-bootloader as a workaround to have our hardware (HPE Blades) working with 12.2 (debugged back then with @tsoome). Sadly we can't use that fix for our network-boot-images. Is there any chance to get the fixes in stable/12 and maybe even in the 12.3 release?
I think there are quite a bunch of users that would be happy if this would be possible. Thanks for taking a look at it.

Herald added a subscriber: imp. · View Herald TranscriptOct 11 2021, 9:46 AM

amd64 UEFI boot: stop copying staging area to 2M phys
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 93416

stand/common/bootstrap.h

stand/common/load_elf.c

stand/efi/loader/arch/amd64/elf64_freebsd.c

stand/efi/loader/bootinfo.c

stand/efi/loader/copy.c

stand/efi/loader/loader_efi.h

amd64 UEFI boot: stop copying staging area to 2M phys ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 93416

stand/common/bootstrap.h

stand/common/load_elf.c

stand/efi/loader/arch/amd64/elf64_freebsd.c

stand/efi/loader/bootinfo.c

stand/efi/loader/copy.c

stand/efi/loader/loader_efi.h

amd64 UEFI boot: stop copying staging area to 2M phys
ClosedPublic
Actions

Revision Contents
Changeset List