| Message ID | 20240910161612.242702-4-frediano.ziglio@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/boot: Reduce assembly code | expand |
On 10.09.2024 18:16, Frediano Ziglio wrote: > No need to have it coded in assembly. As to the title: It's the EFI/MB2 case you re-write. That wants reflecting there, as the "normal" EFI start part is all C already anyway. I also think you mean "partly"? > @@ -255,34 +246,29 @@ __efi64_mb2_start: > rep stosq > mov %edx, %eax This can be dropped then, by making ... > - /* 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 %rax ... this use %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. */ > + push %rbx %rbx doesn't need preserving around a C function call (which will do so itself if necessary). I understand a 2nd PUSH may be necessary anyway, to keep the stack aligned, yet that then would need commenting differently. Plus as long as we call our own functions only, we don't require such alignment. > - /* 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. > + * - OUT: %eax - error string. Nit: %rax according to the code below. > + */ > + mov %eax, %edi > + mov %ebx, %esi This latter one would better be a 64-bit MOV, for it being a pointer? > + call efi_parse_mbi2 > + test %rax, %rax > + lea .Ldirect_error(%rip), %r15 > + jnz x86_32_switch As requested by Andrew in a related context: LEA first please to follow the pattern allowing macro fusion, even if here it is less because of performance concerns but more to avoid giving a bad example. > --- a/xen/arch/x86/efi/Makefile > +++ b/xen/arch/x86/efi/Makefile > @@ -13,6 +13,7 @@ $(addprefix $(obj)/,$(EFIOBJ-y)): CFLAGS_stack_boundary := $(cflags-stack-bounda > > obj-y := common-stub.o stub.o > obj-$(XEN_BUILD_EFI) := $(filter-out %.init.o,$(EFIOBJ-y)) > +obj-y += parse-mbi2.o obj-bin-y I suppose, for it all being __init / __initdata, and hence the string literals in particular also wanting to move to .init.rodata. > --- /dev/null > +++ b/xen/arch/x86/efi/parse-mbi2.c > @@ -0,0 +1,54 @@ > +/* 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> I don't think all of these are needed? Please limit #include-s to just what is actually used. > +void asmlinkage __init efi_multiboot2(EFI_HANDLE ImageHandle, > + EFI_SYSTEM_TABLE *SystemTable, > + const char *cmdline); This i now solely called from C code and hence shouldn't be asmlinkage. > +const char *__init efi_parse_mbi2(uint32_t magic, const multiboot2_fixed_t *mbi) Whereas this, called from assembly code and not having / needing a declaration, should be. > +{ > + 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 Multiboot bootloader!"; Assembly code merely re-used a message. Now that it separate, please make it say "Multiboot2". > + /* Skip Multiboot2 information fixed part. */ > + for ( tag = _p(ROUNDUP((unsigned long)(mbi + 1), MULTIBOOT2_TAG_ALIGN)); The comment is placed as if it applied to the entire loop. It wants to move inside the for() to make clear it only applies to the loop setup. > + (void *)tag - (void *)mbi < mbi->total_size && tag->type != MULTIBOOT2_TAG_TYPE_END; > + tag = _p(ROUNDUP((unsigned long)((void *)tag + tag->size), MULTIBOOT2_TAG_ALIGN)) ) Now that this is done in C, I think the checking wants to be more thorough, to no only make sure the start of a sub-struct is within the specified size, but all of it (se we won't even access past ->total_size). Further looks like there's a line length issue here. Also please don't cast away const-ness from pointers. > + { > + 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; > + } > + > + if ( !have_bs ) > + return "ERR: Bootloader shutdown EFI x64 boot services!"; > + if ( !SystemTable ) > + return "ERR: EFI SystemTable is not provided by bootloader!"; > + if ( !ImageHandle ) > + return "ERR: EFI ImageHandle is not provided by bootloader!"; > + > + efi_multiboot2(ImageHandle, SystemTable, cmdline); This being invoked from here now makes me wonder about the (new) function's name and whether a separate new function is actually needed: Can't the new code then be integrated right into efi_multiboot2(), thus eliminating the question on the naming of the function? > --- a/xen/arch/x86/efi/stub.c > +++ b/xen/arch/x86/efi/stub.c > @@ -17,7 +17,8 @@ > */ > > void __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle, > - EFI_SYSTEM_TABLE *SystemTable) > + EFI_SYSTEM_TABLE *SystemTable, > + const char *cmdline) > { > static const CHAR16 __initconst err[] = > L"Xen does not have EFI code build in!\r\nSystem halted!\r\n"; This, if not a separate change, wants mentioning in the description. It's a related observation that this wasn't properly updated, but nothing that necessarily needs doing here. Question is whether the declaration of the function wouldn't better go into a header now in the first place. Jan
On Sun, Sep 15, 2024 at 8:00 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 10.09.2024 18:16, Frediano Ziglio wrote: > > No need to have it coded in assembly. > > As to the title: It's the EFI/MB2 case you re-write. That wants reflecting > there, as the "normal" EFI start part is all C already anyway. I also > think you mean "partly"? > Updated to "x86/boot: Rewrite EFI/MBI2 code partly in C". > > @@ -255,34 +246,29 @@ __efi64_mb2_start: > > rep stosq > > mov %edx, %eax > > This can be dropped then, by making ... > > > - /* 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 %rax > > ... this use %rdx. > Done (also below) > > -.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. */ > > + push %rbx > > %rbx doesn't need preserving around a C function call (which will do > so itself if necessary). I understand a 2nd PUSH may be necessary > anyway, to keep the stack aligned, yet that then would need > commenting differently. Plus as long as we call our own functions > only, we don't require such alignment. > Extended comment. 16-byte alignment is also in SystemV ABI, I won't remove it in this series. > > - /* 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. > > + * - OUT: %eax - error string. > > Nit: %rax according to the code below. > Done > > + */ > > + mov %eax, %edi > > + mov %ebx, %esi > > This latter one would better be a 64-bit MOV, for it being a pointer? > Done > > + call efi_parse_mbi2 > > + test %rax, %rax > > + lea .Ldirect_error(%rip), %r15 > > + jnz x86_32_switch > > As requested by Andrew in a related context: LEA first please to follow > the pattern allowing macro fusion, even if here it is less because of > performance concerns but more to avoid giving a bad example. > Done > > --- a/xen/arch/x86/efi/Makefile > > +++ b/xen/arch/x86/efi/Makefile > > @@ -13,6 +13,7 @@ $(addprefix $(obj)/,$(EFIOBJ-y)): CFLAGS_stack_boundary := $(cflags-stack-bounda > > > > obj-y := common-stub.o stub.o > > obj-$(XEN_BUILD_EFI) := $(filter-out %.init.o,$(EFIOBJ-y)) > > +obj-y += parse-mbi2.o > > obj-bin-y I suppose, for it all being __init / __initdata, and hence the > string literals in particular also wanting to move to .init.rodata. > Okay > > --- /dev/null > > +++ b/xen/arch/x86/efi/parse-mbi2.c > > @@ -0,0 +1,54 @@ > > +/* 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> > > I don't think all of these are needed? Please limit #include-s to just > what is actually used. > I had the same idea, but if you comment out any of them code stop compiling. > > +void asmlinkage __init efi_multiboot2(EFI_HANDLE ImageHandle, > > + EFI_SYSTEM_TABLE *SystemTable, > > + const char *cmdline); > > This i now solely called from C code and hence shouldn't be asmlinkage. > Done > > +const char *__init efi_parse_mbi2(uint32_t magic, const multiboot2_fixed_t *mbi) > > Whereas this, called from assembly code and not having / needing a > declaration, should be. > Done > > +{ > > + 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 Multiboot bootloader!"; > > Assembly code merely re-used a message. Now that it separate, please make > it say "Multiboot2". > Done > > + /* Skip Multiboot2 information fixed part. */ > > + for ( tag = _p(ROUNDUP((unsigned long)(mbi + 1), MULTIBOOT2_TAG_ALIGN)); > > The comment is placed as if it applied to the entire loop. It wants to move > inside the for() to make clear it only applies to the loop setup. > Separated in a different line. > > + (void *)tag - (void *)mbi < mbi->total_size && tag->type != MULTIBOOT2_TAG_TYPE_END; > > + tag = _p(ROUNDUP((unsigned long)((void *)tag + tag->size), MULTIBOOT2_TAG_ALIGN)) ) > > Now that this is done in C, I think the checking wants to be more > thorough, to no only make sure the start of a sub-struct is within > the specified size, but all of it (se we won't even access past > ->total_size). > I would first just translate the assembly code, then add improvements in a separate commit. > Further looks like there's a line length issue here. > Fixed > Also please don't cast away const-ness from pointers. > Done > > + { > > + 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; > > + } > > + > > + if ( !have_bs ) > > + return "ERR: Bootloader shutdown EFI x64 boot services!"; > > + if ( !SystemTable ) > > + return "ERR: EFI SystemTable is not provided by bootloader!"; > > + if ( !ImageHandle ) > > + return "ERR: EFI ImageHandle is not provided by bootloader!"; > > + > > + efi_multiboot2(ImageHandle, SystemTable, cmdline); > > This being invoked from here now makes me wonder about the (new) > function's name and whether a separate new function is actually > needed: Can't the new code then be integrated right into > efi_multiboot2(), thus eliminating the question on the naming of > the function? > If you are suggesting putting this parsing code inside efi_multiboot2 in ef-boot.h that would change the behavior, which I would do in a different commit. Currently, there are 2 different efi_multiboot2 functions, one if ms_abi is supported, the other an empty stubs. However, some checks and tests are done in both cases (ms_abi supported or not). Also, both paths uses SystemTable, so I need to parse MBI2 in any case. > > --- a/xen/arch/x86/efi/stub.c > > +++ b/xen/arch/x86/efi/stub.c > > @@ -17,7 +17,8 @@ > > */ > > > > void __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle, > > - EFI_SYSTEM_TABLE *SystemTable) > > + EFI_SYSTEM_TABLE *SystemTable, > > + const char *cmdline) > > { > > static const CHAR16 __initconst err[] = > > L"Xen does not have EFI code build in!\r\nSystem halted!\r\n"; > > This, if not a separate change, wants mentioning in the description. > It's a related observation that this wasn't properly updated, but > nothing that necessarily needs doing here. Question is whether the > declaration of the function wouldn't better go into a header now in > the first place. > I had the same though about a header. But currently there's no such header, I mean it should be able to be included by both stub.c and efi-boot.h which are both x86 only code so it should go in xen/arch/x86/ I suppose. Suggestions? Maybe a different solution would be to have a xen/arch/x86/efi/efi-boot-stub.h instead of xen/arch/x86/efi/stub.c ? > Jan Frediano
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index a91413184c..4b05b564a0 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -122,8 +122,6 @@ multiboot2_header: .Lbad_cpu_msg: .asciz "ERR: Not a 64-bit CPU!" .Lbad_ldr_msg: .asciz "ERR: Not a Multiboot bootloader!" .Lbad_ldr_nbs: .asciz "ERR: Bootloader shutdown EFI x64 boot services!" -.Lbad_ldr_nst: .asciz "ERR: EFI SystemTable is not provided by bootloader!" -.Lbad_ldr_nih: .asciz "ERR: EFI ImageHandle is not provided by bootloader!" .Lbad_efi_msg: .asciz "ERR: EFI IA-32 platforms are not supported!" .Lbag_alg_msg: .asciz "ERR: Xen must be loaded at a 2Mb boundary!" .Lno_nx_msg: .asciz "ERR: Not an NX-capable CPU!" @@ -162,17 +160,6 @@ early_error: /* Here to improve the disassembly. */ mov $sym_offs(.Lno_nx_msg), %ecx jmp .Lget_vtb #endif -.Lmb2_no_st: - /* - * Here we are on EFI platform. vga_text_buffer was zapped earlier - * because there is pretty good chance that VGA is unavailable. - */ - mov $sym_offs(.Lbad_ldr_nst), %ecx - jmp .Lget_vtb -.Lmb2_no_ih: - /* Ditto. */ - mov $sym_offs(.Lbad_ldr_nih), %ecx - jmp .Lget_vtb .Lmb2_no_bs: /* * Ditto. Additionally, here there is a chance that Xen was started @@ -190,6 +177,10 @@ early_error: /* Here to improve the disassembly. */ mov $sym_offs(.Lbad_efi_msg), %ecx xor %edi,%edi # No VGA text buffer jmp .Lprint_err +.Ldirect_error: + mov sym_esi(vga_text_buffer), %edi + mov %eax, %esi + jmp 1f .Lget_vtb: mov sym_esi(vga_text_buffer), %edi .Lprint_err: @@ -255,34 +246,29 @@ __efi64_mb2_start: 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 %rax -.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. */ + push %rbx - /* 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. + * - OUT: %eax - error string. + */ + mov %eax, %edi + mov %ebx, %esi + call efi_parse_mbi2 + test %rax, %rax + lea .Ldirect_error(%rip), %r15 + 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 /* We are on EFI platform and EFI boot services are available. */ incb efi_platform(%rip) @@ -292,77 +278,6 @@ __efi64_mb2_start: * be run on EFI platforms. */ incb skip_realmode(%rip) - jmp .Lefi_mb2_next_tag - -.Lefi_mb2_st: - /* Get EFI SystemTable address from Multiboot2 information. */ - cmpl $MULTIBOOT2_TAG_TYPE_EFI64,MB2_tag_type(%rcx) - cmove MB2_efi64_st(%rcx),%rsi - je .Lefi_mb2_next_tag - - /* Get EFI ImageHandle address from Multiboot2 information. */ - cmpl $MULTIBOOT2_TAG_TYPE_EFI64_IH,MB2_tag_type(%rcx) - cmove MB2_efi64_ih(%rcx),%rdi - je .Lefi_mb2_next_tag - - /* Get command line from Multiboot2 information. */ - cmpl $MULTIBOOT2_TAG_TYPE_CMDLINE, MB2_tag_type(%rcx) - jne .Lno_cmdline - lea MB2_tag_string(%rcx), %rdx - jmp .Lefi_mb2_next_tag -.Lno_cmdline: - - /* Is it the end of Multiboot2 information? */ - cmpl $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx) - je .Lrun_bs - -.Lefi_mb2_next_tag: - /* Go to next Multiboot2 information tag. */ - add MB2_tag_size(%rcx),%ecx - add $(MULTIBOOT2_TAG_ALIGN-1),%ecx - and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx - jmp .Lefi_mb2_tsize - -.Lrun_bs: - /* Are EFI boot services available? */ - cmpb $0,efi_platform(%rip) - - /* Jump to .Lmb2_no_bs after switching CPU to x86_32 mode. */ - lea .Lmb2_no_bs(%rip),%r15 - jz x86_32_switch - - /* Is EFI SystemTable address provided by boot loader? */ - test %rsi,%rsi - - /* Jump to .Lmb2_no_st after switching CPU to x86_32 mode. */ - lea .Lmb2_no_st(%rip),%r15 - jz x86_32_switch - - /* Is EFI ImageHandle address provided by boot loader? */ - test %rdi,%rdi - - /* Jump to .Lmb2_no_ih after switching CPU to x86_32 mode. */ - lea .Lmb2_no_ih(%rip),%r15 - jz x86_32_switch - - /* Save Multiboot2 magic on the stack. */ - push %rax - - /* Save EFI ImageHandle on the stack. */ - push %rdi - - /* - * efi_multiboot2() is called according to System V AMD64 ABI: - * - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable, - * %rdx - MB2 cmdline - */ - call efi_multiboot2 - - /* Just pop an item from the stack. */ - pop %rax - - /* Restore Multiboot2 magic. */ - pop %rax /* Jump to trampoline_setup after switching CPU to x86_32 mode. */ lea trampoline_setup(%rip),%r15 diff --git a/xen/arch/x86/efi/Makefile b/xen/arch/x86/efi/Makefile index 24dfecfad1..0bbb29dc0e 100644 --- a/xen/arch/x86/efi/Makefile +++ b/xen/arch/x86/efi/Makefile @@ -13,6 +13,7 @@ $(addprefix $(obj)/,$(EFIOBJ-y)): CFLAGS_stack_boundary := $(cflags-stack-bounda obj-y := common-stub.o stub.o obj-$(XEN_BUILD_EFI) := $(filter-out %.init.o,$(EFIOBJ-y)) +obj-y += parse-mbi2.o obj-bin-$(XEN_BUILD_EFI) := $(filter %.init.o,$(EFIOBJ-y)) extra-$(XEN_BUILD_EFI) += buildid.o relocs-dummy.o nocov-$(XEN_BUILD_EFI) += stub.o diff --git a/xen/arch/x86/efi/parse-mbi2.c b/xen/arch/x86/efi/parse-mbi2.c new file mode 100644 index 0000000000..4dfcac326e --- /dev/null +++ b/xen/arch/x86/efi/parse-mbi2.c @@ -0,0 +1,54 @@ +/* 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 asmlinkage __init efi_multiboot2(EFI_HANDLE ImageHandle, + EFI_SYSTEM_TABLE *SystemTable, + const char *cmdline); + +const char *__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 Multiboot bootloader!"; + + /* Skip Multiboot2 information fixed part. */ + for ( tag = _p(ROUNDUP((unsigned long)(mbi + 1), MULTIBOOT2_TAG_ALIGN)); + (void *)tag - (void *)mbi < mbi->total_size && tag->type != MULTIBOOT2_TAG_TYPE_END; + tag = _p(ROUNDUP((unsigned long)((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; + } + + if ( !have_bs ) + return "ERR: Bootloader shutdown EFI x64 boot services!"; + if ( !SystemTable ) + return "ERR: EFI SystemTable is not provided by bootloader!"; + if ( !ImageHandle ) + return "ERR: EFI ImageHandle is not provided by bootloader!"; + + efi_multiboot2(ImageHandle, SystemTable, cmdline); + + return NULL; +} diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.c index 2cd5c8d4dc..27d40964d5 100644 --- a/xen/arch/x86/efi/stub.c +++ b/xen/arch/x86/efi/stub.c @@ -17,7 +17,8 @@ */ void __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle, - EFI_SYSTEM_TABLE *SystemTable) + EFI_SYSTEM_TABLE *SystemTable, + const char *cmdline) { static const CHAR16 __initconst err[] = L"Xen does not have EFI code build in!\r\nSystem halted!\r\n";
No need to have it coded in assembly. Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> --- xen/arch/x86/boot/head.S | 131 ++++++---------------------------- xen/arch/x86/efi/Makefile | 1 + xen/arch/x86/efi/parse-mbi2.c | 54 ++++++++++++++ xen/arch/x86/efi/stub.c | 3 +- 4 files changed, 80 insertions(+), 109 deletions(-) create mode 100644 xen/arch/x86/efi/parse-mbi2.c