EFIRT: SetVirtualAddressMap with 1:1 mapping after exiting boot services
ClosedPublic
Actions

Authored by kevans on Mar 13 2018, 1:27 PM.

Details

Reviewers

kib
andrew

Commits

rS330868: EFIRT: SetVirtualAddressMap with 1:1 mapping after exiting boot services

Summary

This fixes a problem encountered on the Thinkpad X220/Yoga 11e where runtime services would try to inexplicably jump to other parts of memory where it shouldn't be when attempting to enumerate EFI vars, causing a panic.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kevans created this revision.Mar 13 2018, 1:27 PM

Herald added subscribers: emaste, imp. · View Herald TranscriptMar 13 2018, 1:27 PM

Harbormaster completed remote builds in B15538: Diff 40242.Mar 13 2018, 1:27 PM

kib added inline comments.Mar 13 2018, 1:33 PM

stand/efi/loader/bootinfo.c
249 ↗	(On Diff #40242)	We already have NextMemoryDescriptor() macro.
259 ↗	(On Diff #40242)	Copy the whole structure, it is simpler IMO. Also you may assign to desc->VirtualStart before, then you only need to do it once.
360 ↗	(On Diff #40242)	A variable to request not calling SetVirtualAddressMap() might be useful.

Address feedback by @kib:

Use already defined NextMemoryDescriptor
Set VA = PA then memcpy whole descriptor
Allow efi_disable_vmap to be set in loader.conf(5) to disable virtual mapping

kevans marked 3 inline comments as done.Mar 13 2018, 2:10 PM

kevans added inline comments.

stand/efi/loader/bootinfo.c
249 ↗	(On Diff #40242)	I swear that wasn't there when I was throwing this together last night.

kib accepted this revision.Mar 13 2018, 2:19 PM

kib added inline comments.

stand/efi/loader/bootinfo.c
243 ↗	(On Diff #40244)	Why not put all three on the same line ?
259 ↗	(On Diff #40244)	viter = desc;

This revision is now accepted and ready to land.Mar 13 2018, 2:19 PM

kevans marked an inline comment as done.Mar 13 2018, 2:25 PM

kevans added inline comments.

stand/efi/loader/bootinfo.c
259 ↗	(On Diff #40244)	I wasn't sure if this was OK or not- mmsz seems to be larger than sizeof(*desc), and I wasn't sure if the excess was just padding or firmware-specific bits that should be preserved.

kib added inline comments.Mar 13 2018, 2:30 PM

stand/efi/loader/bootinfo.c
259 ↗	(On Diff #40244)	Ok.

Address a couple more concerns:

Consolidate desc, viter, vmap declarations
Just set *viter = *desc; I thought I had observed that mmsz > sizeof(*desc) in early testing yesterday, but mmsz is explicitly set to sizeof(EFI_MEMORY_DESCRIPTOR)...
Don't use implicit boolean tresting for (attribute & EFI_MEMORY_RUNTIME)

This revision now requires review to proceed.Mar 13 2018, 2:46 PM

This revision was not accepted when it landed; it landed in state Needs Review.Mar 13 2018, 5:11 PM

Closed by commit rS330868: EFIRT: SetVirtualAddressMap with 1:1 mapping after exiting boot services (authored by kevans). · Explain Why

This revision was automatically updated to reflect the committed changes.