On Sun, 22 Dec 2019 at 22:13, Arvind Sankar <nivedita@xxxxxxxxxxxx> wrote: > > On Sun, Dec 22, 2019 at 04:29:48PM +0100, Ard Biesheuvel wrote: > > On Sun, 22 Dec 2019 at 13:46, Andy Lutomirski <luto@xxxxxxxxxxxxxx> wrote: > > > > > > Hmm. Most of the kernel is compiled with the stack alignment set to 8, and there a lot of asm that makes no effort to preserve alignment beyond 8 bytes. So if EFI calls need 16 byte alignment, you may need to do something special. > > > > > > On new enough gcc (the versions that actually support the flags to set the alignment to 8), maybe you can use function attributes, or maybe you can stick a 16-byte-aligned local variable in functions that call EFI functions? The latter would be rather fragile. > > > > This patch replaces open coded SysV to MS calling convention > > translation to GCC generated code (using __attribute__((ms_abi)) which > > we have been using for a long time in EDK2), because the former relies > > on a wrapper function > > > > efi_call(fn, ...) > > > > which is type unsafe and relies on a lot of nasty casting, especially > > combined with the mixed mode support. efi_call() is implemented as > > below, and as Hans reports, omitting this sequence causes a boot > > regression on one of the platforms he has tested this on. > > > > So the question is which of the pieces below this UEFI implementation > > is actually relying on, and the stack pointer alignment is my first > > guess, but it could be any of the other things as well. Once we > > identify what it is we are missing, I can simply stick it back in, but > > without reverting to using the efi_call() thunk. > > > > Note that the decompressor/stub are built with the default stack > > alignment of 16 afaict, but if GRUB enters the decompressor with a > > misaligned stack, we probably wouldn't notice until we hit something > > like a movaps, right? > > > > Thanks, > > Ard. > > > > Won't the entry code misalign the stack when efi_main is called, > assuming the stack was properly aligned at efi_stub_entry? There needs > to be a sub $8, %rsp in there, no? > > arch/x86/boot/compressed/head_64.S: > > #ifdef CONFIG_EFI_STUB > .org 0x390 > SYM_FUNC_START(efi64_stub_entry) > SYM_FUNC_START_ALIAS(efi_stub_entry) > movq $1, %rcx > handover_entry: > call efi_main <--- this will enter efi_main with a misaligned stack? > movq %rax,%rsi > movl BP_code32_start(%esi), %eax > leaq startup_64(%rax), %rax > jmp *%rax > SYM_FUNC_END(efi64_stub_entry) > SYM_FUNC_END_ALIAS(efi_stub_entry) > #endif > Indeed, well spotted. Note that the above is the version from the top of that branch, but the version that Hans tested isn't any different.
