| Message ID | aedda92afd26caac474870d44504074d3b2ff6d0.1570918263.git-series.marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Optionally call EFI SetVirtualAddressMap() | expand |
On 12/10/2019 23:11, Marek Marczykowski-Górecki wrote: > Some UEFI implementations are not happy about running in 1:1 addressing, > but really virtual address space. Specifically, some access > EfiBootServices{Code,Data}, or even totally unmapped areas. Example > crash of GetVariable() call on Thinkpad W540: > > Xen call trace: > [<0000000000000080>] 0000000000000080 > [<8c2b0398e0000daa>] 8c2b0398e0000daa > > Pagetable walk from ffffffff858483a1: > L4[0x1ff] = 0000000000000000 ffffffffffffffff > > **************************************** > Panic on CPU 0: > FATAL PAGE FAULT > [error_code=0002] > Faulting linear address: ffffffff858483a1 > **************************************** > > Fix this by calling SetVirtualAddressMap() runtime service, giving it > 1:1 map for areas marked as needed during runtime. The address space in > which EFI runtime services are called is unchanged, but UEFI view of it > may be. > SetVirtualAddressMap() can be called only once, so it might make > future kexec EFI plumbing more complex or incompatible with this option. There is no such thing as "future incompatibilities". There is "one more piece of plumbing someone in the future needs to do with passing EFI details". Given how the rest of this looks, I'd just drop all references to Kexec. I think what is here is misleading at best. ~Andrew
On 13.10.2019 00:11, Marek Marczykowski-Górecki wrote: > Some UEFI implementations are not happy about running in 1:1 addressing, > but really virtual address space. I have to admit that I find this misleading. There's no true "physical mode" on x86-64 anyway. What I assume happens is that people abuse the address map change notification to do things beyond the necessary ConvertPointer(() calls. > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -88,6 +88,19 @@ config KEXEC > > If unsure, say Y. > > +config SET_VIRTUAL_ADDRESS_MAP I'm of the strong opinion that this wants to have an EFI_ prefix. > + bool "EFI: call SetVirtualAddressMap()" if EXPERT = "y" > + default n I don't think you need this line. > @@ -1094,6 +1100,26 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste > if ( EFI_ERROR(status) ) > PrintErrMesg(L"Cannot exit boot services", status); > > +#ifdef CONFIG_SET_VIRTUAL_ADDRESS_MAP > + for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size ) > + { > + EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i; > + > + if ( desc->Attribute & EFI_MEMORY_RUNTIME ) > + desc->VirtualStart = desc->PhysicalStart; > + else > + desc->VirtualStart = INVALID_VIRTUAL_ADDRESS; > + } > + status = efi_rs->SetVirtualAddressMap(efi_memmap_size, efi_mdesc_size, > + mdesc_ver, efi_memmap); > + if ( status != EFI_SUCCESS ) > + { > + printk(XENLOG_ERR "EFI: SetVirtualAddressMap() failed (%#lx), disabling runtime services\n", > + status); > + __clear_bit(EFI_RS, &efi_flags); > + } > +#endif This new placement undermines (or at least complicates afaict) the original intention to allow picking virtual addresses which don't match the directmap. I can accept this as an intended tradeoff (as you validly mention in the other patch we don't honor the 1:1 map requirement at the time of the call with its original placement), but it should be mentioned in one of the two patch descriptions. Jan
On Wed, Oct 23, 2019 at 05:37:33PM +0200, Jan Beulich wrote: > On 13.10.2019 00:11, Marek Marczykowski-Górecki wrote: > > Some UEFI implementations are not happy about running in 1:1 addressing, > > but really virtual address space. > > I have to admit that I find this misleading. There's no true "physical > mode" on x86-64 anyway. What I assume happens is that people abuse the > address map change notification to do things beyond the necessary > ConvertPointer(() calls. That would indeed match the behaviour. I'll add it to commit message. > > --- a/xen/common/Kconfig > > +++ b/xen/common/Kconfig > > @@ -88,6 +88,19 @@ config KEXEC > > > > If unsure, say Y. > > > > +config SET_VIRTUAL_ADDRESS_MAP > > I'm of the strong opinion that this wants to have an EFI_ prefix. Ok. > > + bool "EFI: call SetVirtualAddressMap()" if EXPERT = "y" > > + default n > > I don't think you need this line. > > > @@ -1094,6 +1100,26 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste > > if ( EFI_ERROR(status) ) > > PrintErrMesg(L"Cannot exit boot services", status); > > > > +#ifdef CONFIG_SET_VIRTUAL_ADDRESS_MAP > > + for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size ) > > + { > > + EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i; > > + > > + if ( desc->Attribute & EFI_MEMORY_RUNTIME ) > > + desc->VirtualStart = desc->PhysicalStart; > > + else > > + desc->VirtualStart = INVALID_VIRTUAL_ADDRESS; > > + } > > + status = efi_rs->SetVirtualAddressMap(efi_memmap_size, efi_mdesc_size, > > + mdesc_ver, efi_memmap); > > + if ( status != EFI_SUCCESS ) > > + { > > + printk(XENLOG_ERR "EFI: SetVirtualAddressMap() failed (%#lx), disabling runtime services\n", > > + status); > > + __clear_bit(EFI_RS, &efi_flags); > > + } > > +#endif > > This new placement undermines (or at least complicates afaict) the > original intention to allow picking virtual addresses which don't > match the directmap. If I read it right, the original intention was to specifically use directmap, not some other virtual addresses. Which is flawed, because directmap is mapped with NX, so at least EfiRuntimeServicesCode will break. This means, even when using directmap, Xen would need to switch page tables for the runtime call time to allow executing that code. There is of course an option to rewrite it completely differently, mapping EFI runtime regions somewhere else (not 1:1 and not re-use directmap). But I don't think it worth the effort, and also is definitely too complex this far in 4.13 release cycle. > I can accept this as an intended tradeoff (as > you validly mention in the other patch we don't honor the 1:1 map > requirement at the time of the call with its original placement), > but it should be mentioned in one of the two patch descriptions. This is one of the reason why I've decided to split this change into two parts - remove the old one and add the new one. It is really not "fixing the old approach", but doing this very differently. I think this patch description referencing old, never working and never even enabled approach would be misleading at least. The patch removing the old approach do list reasons why it was broken.
On 23.10.2019 18:07, Marek Marczykowski-Górecki wrote: > On Wed, Oct 23, 2019 at 05:37:33PM +0200, Jan Beulich wrote: >> On 13.10.2019 00:11, Marek Marczykowski-Górecki wrote: >>> @@ -1094,6 +1100,26 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste >>> if ( EFI_ERROR(status) ) >>> PrintErrMesg(L"Cannot exit boot services", status); >>> >>> +#ifdef CONFIG_SET_VIRTUAL_ADDRESS_MAP >>> + for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size ) >>> + { >>> + EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i; >>> + >>> + if ( desc->Attribute & EFI_MEMORY_RUNTIME ) >>> + desc->VirtualStart = desc->PhysicalStart; >>> + else >>> + desc->VirtualStart = INVALID_VIRTUAL_ADDRESS; >>> + } >>> + status = efi_rs->SetVirtualAddressMap(efi_memmap_size, efi_mdesc_size, >>> + mdesc_ver, efi_memmap); >>> + if ( status != EFI_SUCCESS ) >>> + { >>> + printk(XENLOG_ERR "EFI: SetVirtualAddressMap() failed (%#lx), disabling runtime services\n", >>> + status); >>> + __clear_bit(EFI_RS, &efi_flags); >>> + } >>> +#endif >> >> This new placement undermines (or at least complicates afaict) the >> original intention to allow picking virtual addresses which don't >> match the directmap. > > If I read it right, the original intention was to specifically use > directmap, not some other virtual addresses. Which is flawed, because > directmap is mapped with NX, so at least EfiRuntimeServicesCode will > break. This means, even when using directmap, Xen would need to switch > page tables for the runtime call time to allow executing that code. Just FYI: The NX-ifying post-dates the EFI work by several years. > There is of course an option to rewrite it completely differently, > mapping EFI runtime regions somewhere else (not 1:1 and not re-use > directmap). But I don't think it worth the effort, and also is definitely > too complex this far in 4.13 release cycle. Especially on this last point - fully agree. Jan
diff --git a/xen/common/Kconfig b/xen/common/Kconfig index 16829f6..4ad4534 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -88,6 +88,19 @@ config KEXEC If unsure, say Y. +config SET_VIRTUAL_ADDRESS_MAP + bool "EFI: call SetVirtualAddressMap()" if EXPERT = "y" + default n + ---help--- + Call EFI SetVirtualAddressMap() runtime service to setup memory map for + further runtime services. According to UEFI spec, it isn't strictly + necessary, but many UEFI implementations misbehave when this call is + missing. On the other hand, this call can be made only once, which might + be incompatible with future EFI support in Xen's kexec. Kexec ability + in the current form is not affected. + + If unsure, say N. + config XENOPROF def_bool y prompt "Xen Oprofile Support" if EXPERT = "y" diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index cddf3de..6eaabd4 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -1056,11 +1056,17 @@ static void __init efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN gop efi_arch_video_init(gop, info_size, mode_info); } +#define INVALID_VIRTUAL_ADDRESS (0xBAAADUL << \ + (EFI_PAGE_SHIFT + BITS_PER_LONG - 32)) + static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) { EFI_STATUS status; UINTN info_size = 0, map_key; bool retry; +#ifdef CONFIG_SET_VIRTUAL_ADDRESS_MAP + unsigned int i; +#endif efi_bs->GetMemoryMap(&info_size, NULL, &map_key, &efi_mdesc_size, &mdesc_ver); @@ -1094,6 +1100,26 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste if ( EFI_ERROR(status) ) PrintErrMesg(L"Cannot exit boot services", status); +#ifdef CONFIG_SET_VIRTUAL_ADDRESS_MAP + for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size ) + { + EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i; + + if ( desc->Attribute & EFI_MEMORY_RUNTIME ) + desc->VirtualStart = desc->PhysicalStart; + else + desc->VirtualStart = INVALID_VIRTUAL_ADDRESS; + } + status = efi_rs->SetVirtualAddressMap(efi_memmap_size, efi_mdesc_size, + mdesc_ver, efi_memmap); + if ( status != EFI_SUCCESS ) + { + printk(XENLOG_ERR "EFI: SetVirtualAddressMap() failed (%#lx), disabling runtime services\n", + status); + __clear_bit(EFI_RS, &efi_flags); + } +#endif + /* Adjust pointers into EFI. */ efi_ct = (void *)efi_ct + DIRECTMAP_VIRT_START; efi_memmap = (void *)efi_memmap + DIRECTMAP_VIRT_START; @@ -1460,8 +1486,6 @@ static bool __init rt_range_valid(unsigned long smfn, unsigned long emfn) return true; } -#define INVALID_VIRTUAL_ADDRESS (0xBAAADUL << \ - (EFI_PAGE_SHIFT + BITS_PER_LONG - 32)) void __init efi_init_memory(void) {
Some UEFI implementations are not happy about running in 1:1 addressing, but really virtual address space. Specifically, some access EfiBootServices{Code,Data}, or even totally unmapped areas. Example crash of GetVariable() call on Thinkpad W540: Xen call trace: [<0000000000000080>] 0000000000000080 [<8c2b0398e0000daa>] 8c2b0398e0000daa Pagetable walk from ffffffff858483a1: L4[0x1ff] = 0000000000000000 ffffffffffffffff **************************************** Panic on CPU 0: FATAL PAGE FAULT [error_code=0002] Faulting linear address: ffffffff858483a1 **************************************** Fix this by calling SetVirtualAddressMap() runtime service, giving it 1:1 map for areas marked as needed during runtime. The address space in which EFI runtime services are called is unchanged, but UEFI view of it may be. SetVirtualAddressMap() can be called only once, so it might make future kexec EFI plumbing more complex or incompatible with this option. Since it's fairly late in Xen 4.13 development cycle, disable it by default and hide behind EXPERT. Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes in v2: - call SetVirtualAddressMap() before adjusting efi pointers; especially efi_memmap at this point still needs to use physical address, not a directmap one Changes in v3: - clarify impact (or rather: lack of it) on kexec, drop !KEXEC dependency. --- xen/common/Kconfig | 13 +++++++++++++ xen/common/efi/boot.c | 28 ++++++++++++++++++++++++++-- 2 files changed, 39 insertions(+), 2 deletions(-)