| Message ID | 20240924102811.86884-4-frediano.ziglio@cloud.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/boot: Reduce assembly code | expand |
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
On 24.09.2024 12:28, Frediano Ziglio wrote: > No need to have it coded in assembly. > > Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> > --- > Changes since v1: > - update some comments; > - explain why %ebx is saved before calling efi_parse_mbi2; > - move lea before test instruction; > - removed asmlinkage from efi_multiboot2 and add to efi_parse_mbi2; > - fix line length; > - update an error message specifying "Multiboot2" instead of "Multiboot"; > - use obj-bin-X instead of obj-X in Makefile; > - avoid restoring %eax (MBI magic). Despite this long list of changes earlier comments were left unaddressed. The new function is still named as if it did only parsing, the stub change is still in here and (if already not separated out) not mentioned at all in the description, and (as Andrew has now also pointed out) the declaration of efi_multiboot2() didn't move to a header. Maybe I forgot some more. Please make sure you address earlier comments before sending a new version. Jan
On Tue, Sep 24, 2024 at 3:17 PM Jan Beulich <jbeulich@suse.com> wrote: > > On 24.09.2024 12:28, Frediano Ziglio wrote: > > No need to have it coded in assembly. > > > > Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> > > --- > > Changes since v1: > > - update some comments; > > - explain why %ebx is saved before calling efi_parse_mbi2; > > - move lea before test instruction; > > - removed asmlinkage from efi_multiboot2 and add to efi_parse_mbi2; > > - fix line length; > > - update an error message specifying "Multiboot2" instead of "Multiboot"; > > - use obj-bin-X instead of obj-X in Makefile; > > - avoid restoring %eax (MBI magic). > > Despite this long list of changes earlier comments were left unaddressed. > The new function is still named as if it did only parsing, the stub change > is still in here and (if already not separated out) not mentioned at all > in the description, and (as Andrew has now also pointed out) the > declaration of efi_multiboot2() didn't move to a header. Maybe I forgot > some more. Please make sure you address earlier comments before sending a > new version. > > Jan What about renaming efi_parse_mbi2 to "efi_multiboot2_entry_common" and renaming "efi_multiboot2" as "efi_multiboot2_entry". I remember I replied to the stub change and nobody get back, so I thought it was fine as it was. I also replied to the header asking for a location to put it, and I don't remember any reply. Frediano
On 24.09.2024 16:28, Frediano Ziglio wrote: > On Tue, Sep 24, 2024 at 3:17 PM Jan Beulich <jbeulich@suse.com> wrote: >> >> On 24.09.2024 12:28, Frediano Ziglio wrote: >>> No need to have it coded in assembly. >>> >>> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> >>> --- >>> Changes since v1: >>> - update some comments; >>> - explain why %ebx is saved before calling efi_parse_mbi2; >>> - move lea before test instruction; >>> - removed asmlinkage from efi_multiboot2 and add to efi_parse_mbi2; >>> - fix line length; >>> - update an error message specifying "Multiboot2" instead of "Multiboot"; >>> - use obj-bin-X instead of obj-X in Makefile; >>> - avoid restoring %eax (MBI magic). >> >> Despite this long list of changes earlier comments were left unaddressed. >> The new function is still named as if it did only parsing, the stub change >> is still in here and (if already not separated out) not mentioned at all >> in the description, and (as Andrew has now also pointed out) the >> declaration of efi_multiboot2() didn't move to a header. Maybe I forgot >> some more. Please make sure you address earlier comments before sending a >> new version. > > What about renaming efi_parse_mbi2 to "efi_multiboot2_entry_common" > and renaming "efi_multiboot2" as "efi_multiboot2_entry". I don't see a need to rename efi_multiboot2() as well. In "efi_multiboot2_entry_common" I'm having trouble seeing what "common" stands for. Imo a small improvement would already be efi_process_mbi2(), as "process" covers parsing and the handing on of the result of the parsing. However, if already the new code can't be folded into efi_multiboot2(), how about naming the new function efi_multiboot2_prelude()? > I remember I replied to the stub change and nobody get back, so I > thought it was fine as it was. > I also replied to the header asking for a location to put it, and I > don't remember any reply. I'm sure I gave you a reply, but that was only yesterday, after I was back from travelling / PTO. Jan
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 @@ -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: @@ -236,7 +227,7 @@ __efi64_mb2_start: /* * Align the stack as UEFI spec requires. Keep it aligned - * before efi_multiboot2() call by pushing/popping even + * before efi_parse_mbi2() call by pushing/popping even * numbers of items on it. */ and $~15, %rsp @@ -244,7 +235,7 @@ __efi64_mb2_start: /* * Initialize BSS (no nasty surprises!). * It must be done earlier than in BIOS case - * because efi_multiboot2() touches it. + * because efi_parse_mbi2() touches it. */ mov %eax, %edx lea __bss_start(%rip), %edi @@ -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 - /* 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: %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 /* We are on EFI platform and EFI boot services are available. */ incb efi_platform(%rip) @@ -292,77 +277,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..51140061fc 100644 --- a/xen/arch/x86/efi/Makefile +++ b/xen/arch/x86/efi/Makefile @@ -14,5 +14,6 @@ $(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-bin-$(XEN_BUILD_EFI) := $(filter %.init.o,$(EFIOBJ-y)) +obj-bin-y += parse-mbi2.o extra-$(XEN_BUILD_EFI) += buildid.o relocs-dummy.o nocov-$(XEN_BUILD_EFI) += stub.o diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index 7aa55e7aaf..859c01c13f 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -816,9 +816,9 @@ static const char *__init get_option(const char *cmd, const char *opt) return o; } -void asmlinkage __init efi_multiboot2(EFI_HANDLE ImageHandle, - EFI_SYSTEM_TABLE *SystemTable, - const char *cmdline) +void __init efi_multiboot2(EFI_HANDLE ImageHandle, + EFI_SYSTEM_TABLE *SystemTable, + const char *cmdline) { EFI_GRAPHICS_OUTPUT_PROTOCOL *gop; EFI_HANDLE gop_handle; 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); + +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; + } + + 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> --- Changes since v1: - update some comments; - explain why %ebx is saved before calling efi_parse_mbi2; - move lea before test instruction; - removed asmlinkage from efi_multiboot2 and add to efi_parse_mbi2; - fix line length; - update an error message specifying "Multiboot2" instead of "Multiboot"; - use obj-bin-X instead of obj-X in Makefile; - avoid restoring %eax (MBI magic). --- xen/arch/x86/boot/head.S | 136 +++++++--------------------------- xen/arch/x86/efi/Makefile | 1 + xen/arch/x86/efi/efi-boot.h | 6 +- xen/arch/x86/efi/parse-mbi2.c | 58 +++++++++++++++ xen/arch/x86/efi/stub.c | 3 +- 5 files changed, 89 insertions(+), 115 deletions(-) create mode 100644 xen/arch/x86/efi/parse-mbi2.c