| 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
On 16.09.2024 10:25, Frediano Ziglio wrote: > On Sun, Sep 15, 2024 at 8:00 AM Jan Beulich <jbeulich@suse.com> wrote: >> On 10.09.2024 18:16, Frediano Ziglio wrote: >>> -.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. Except that we build with -mpreferred-stack-boundary=3, not respecting the ABI in this regard anyway. >>> + { >>> + 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. It could be slightly less parsing, but I get your point. Then, as indicated, the function's name needs to change. The present name simply fails to account for the important-ish fact that efi_multiboot2() is (tail-)called. >>> --- 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 ? It's not quite the right place, but maybe (ab)using asm/efibind.h would be slightly better than introducing asm/efi.h just for a single decl? Jan
On Mon, Sep 23, 2024 at 3:26 PM Jan Beulich <jbeulich@suse.com> wrote: > ... omissis ... > > >>> --- 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 ? > > It's not quite the right place, but maybe (ab)using asm/efibind.h would > be slightly better than introducing asm/efi.h just for a single decl? > > Jan Okay, I found the comment on the header to place the declaration. Kind of works... but the headers are very crazily depending on each other, the header change is diff --git a/xen/arch/x86/include/asm/efibind.h b/xen/arch/x86/include/asm/efibind.h index bce02f3707..1fa5522a0d 100644 --- a/xen/arch/x86/include/asm/efibind.h +++ b/xen/arch/x86/include/asm/efibind.h @@ -1,2 +1,13 @@ #include <xen/types.h> +#include <xen/init.h> #include <asm/x86_64/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); + How does it sound ? Frediano
On 24.09.2024 17:21, Frediano Ziglio wrote: > On Mon, Sep 23, 2024 at 3:26 PM Jan Beulich <jbeulich@suse.com> wrote: >> > > ... omissis ... > >> >>>>> --- 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 ? >> >> It's not quite the right place, but maybe (ab)using asm/efibind.h would >> be slightly better than introducing asm/efi.h just for a single decl? >> >> Jan > > Okay, I found the comment on the header to place the declaration. > > Kind of works... but the headers are very crazily depending on each > other, the header change is > > diff --git a/xen/arch/x86/include/asm/efibind.h > b/xen/arch/x86/include/asm/efibind.h > index bce02f3707..1fa5522a0d 100644 > --- a/xen/arch/x86/include/asm/efibind.h > +++ b/xen/arch/x86/include/asm/efibind.h > @@ -1,2 +1,13 @@ > #include <xen/types.h> > +#include <xen/init.h> > #include <asm/x86_64/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); > + > > How does it sound ? Hmm, no, not good at all. All these #include-s are against the purpose of the header. We'll need asm/efi.h then, I think. As an aside - you don't need xen/init.h here, as declarations shouldn't have __init attributes. Jan
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