[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 3/4] x86/boot: Rewrite EFI/MBI2 code partly in C
On 24/09/2024 11:28 am, Frediano Ziglio wrote: > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S > index 80bba6ff21..6d8eec554b 100644 > --- a/xen/arch/x86/boot/head.S > +++ b/xen/arch/x86/boot/head.S > @@ -253,36 +244,30 @@ __efi64_mb2_start: > shr $3, %ecx > xor %eax, %eax > rep stosq > - mov %edx, %eax > > - /* Check for Multiboot2 bootloader. */ > - cmp $MULTIBOOT2_BOOTLOADER_MAGIC,%eax > - je .Lefi_multiboot2_proto > - > - /* Jump to .Lnot_multiboot after switching CPU to x86_32 mode. */ > - lea .Lnot_multiboot(%rip), %r15 > - jmp x86_32_switch > + /* Save Multiboot2 magic on the stack. */ > + push %rdx > > -.Lefi_multiboot2_proto: > - /* Zero EFI SystemTable, EFI ImageHandle addresses and cmdline. */ > - xor %esi,%esi > - xor %edi,%edi > - xor %edx,%edx > + /* Save Multiboot2 pointer on the stack, keep the stack aligned. */ > + push %rbx I'd merge these two pushes, so /* Spill MB2 magic. Spill the pointer too, to keep the stack aligned. */ push %rdx push %rbx and ... > > - /* Skip Multiboot2 information fixed part. */ > - lea (MB2_fixed_sizeof+MULTIBOOT2_TAG_ALIGN-1)(%rbx),%ecx > - and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx > + /* > + * efi_parse_mbi2() is called according to System V AMD64 ABI: > + * - IN: %edi - Multiboot2 magic, %rsi - Multiboot2 pointer. %rsi below %edi please, for readability. > + * - OUT: %rax - error string. > + */ > + mov %edx, %edi > + mov %rbx, %rsi > + call efi_parse_mbi2 > + lea .Ldirect_error(%rip), %r15 > + test %rax, %rax > + jnz x86_32_switch > > -.Lefi_mb2_tsize: > - /* Check Multiboot2 information total size. */ > - mov %ecx,%r8d > - sub %ebx,%r8d > - cmp %r8d,MB2_fixed_total_size(%rbx) > - jbe .Lrun_bs > + /* Restore Multiboot2 pointer. */ > + pop %rbx > > - /* Are EFI boot services available? */ > - cmpl $MULTIBOOT2_TAG_TYPE_EFI_BS,MB2_tag_type(%rcx) > - jne .Lefi_mb2_st > + /* Restore Multiboot2 magic. */ > + pop %rax ... merge these pops too. (It's a shame the pushes/pops implement a %edx -> %eax swap for magic, but it's for BSS clearing purposes.) > diff --git a/xen/arch/x86/efi/parse-mbi2.c b/xen/arch/x86/efi/parse-mbi2.c > new file mode 100644 > index 0000000000..6038f35b16 > --- /dev/null > +++ b/xen/arch/x86/efi/parse-mbi2.c > @@ -0,0 +1,58 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +#include <xen/efi.h> > +#include <xen/init.h> > +#include <xen/multiboot2.h> > +#include <asm/asm_defns.h> > +#include <asm/efibind.h> > +#include <efi/efidef.h> > +#include <efi/eficapsule.h> > +#include <efi/eficon.h> > +#include <efi/efidevp.h> > +#include <efi/efiapi.h> > + > +void __init efi_multiboot2(EFI_HANDLE ImageHandle, > + EFI_SYSTEM_TABLE *SystemTable, > + const char *cmdline); This wants to be in a header file seen by all references. I see you you've fixed up an error in the stub clearly caused by the absence of a shared header. > + > +const char * asmlinkage __init > +efi_parse_mbi2(uint32_t magic, const multiboot2_fixed_t *mbi) > +{ > + const multiboot2_tag_t *tag; > + EFI_HANDLE ImageHandle = NULL; > + EFI_SYSTEM_TABLE *SystemTable = NULL; > + const char *cmdline = NULL; > + bool have_bs = false; > + > + if ( magic != MULTIBOOT2_BOOTLOADER_MAGIC ) > + return "ERR: Not a Multiboot2 bootloader!"; > + > + /* Skip Multiboot2 information fixed part. */ > + tag = _p(ROUNDUP((unsigned long)(mbi + 1), MULTIBOOT2_TAG_ALIGN)); > + > + for ( ; (const void *)tag - (const void *)mbi < mbi->total_size > + && tag->type != MULTIBOOT2_TAG_TYPE_END; > + tag = _p(ROUNDUP((unsigned long)((const void *)tag + tag->size), > + MULTIBOOT2_TAG_ALIGN)) ) > + { > + if ( tag->type == MULTIBOOT2_TAG_TYPE_EFI_BS ) > + have_bs = true; > + else if ( tag->type == MULTIBOOT2_TAG_TYPE_EFI64 ) > + SystemTable = _p(((const multiboot2_tag_efi64_t *)tag)->pointer); > + else if ( tag->type == MULTIBOOT2_TAG_TYPE_EFI64_IH ) > + ImageHandle = _p(((const multiboot2_tag_efi64_ih_t > *)tag)->pointer); > + else if ( tag->type == MULTIBOOT2_TAG_TYPE_CMDLINE ) > + cmdline = ((const multiboot2_tag_string_t *)tag)->string; switch ( tag->type ) please. It's more lines, but more legible. Otherwise, LGTM. Definitely nice to move this out of asm. ~Andrew
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |