| Message ID | 20240910161612.242702-2-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: > --- a/xen/arch/x86/boot/head.S > +++ b/xen/arch/x86/boot/head.S > @@ -231,6 +231,27 @@ __efi64_mb2_start: > /* VGA is not available on EFI platforms. */ > movl $0,vga_text_buffer(%rip) > > + /* > + * Align the stack as UEFI spec requires. Keep it aligned > + * before efi_multiboot2() call by pushing/popping even > + * numbers of items on it. > + */ > + and $~15,%rsp You don't use the stack below, so it's not clear if/why this needs moving. If it does, please add the missing blank after the comma (like you nicely do everywhere else). Apart from this there's the question on the precise placement of the calls - see respective comment on patch 2 (which I needed to look at first to have an opinion here). Jan
On Sun, Sep 15, 2024 at 7:20 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 10.09.2024 18:16, Frediano Ziglio wrote: > > --- a/xen/arch/x86/boot/head.S > > +++ b/xen/arch/x86/boot/head.S > > @@ -231,6 +231,27 @@ __efi64_mb2_start: > > /* VGA is not available on EFI platforms. */ > > movl $0,vga_text_buffer(%rip) > > > > + /* > > + * Align the stack as UEFI spec requires. Keep it aligned > > + * before efi_multiboot2() call by pushing/popping even > > + * numbers of items on it. > > + */ > > + and $~15,%rsp > > You don't use the stack below, so it's not clear if/why this needs > moving. If it does, please add the missing blank after the comma > (like you nicely do everywhere else). > Fixed the blank. The reason is more clear if you look at the last commit in the series, at least for the EFI part. For the BIOS/PVH part is less clear, but the rationale is the same. The commit came from a larger series where BIOS/PVH is mainly written in C so there is more clear. > Apart from this there's the question on the precise placement of > the calls - see respective comment on patch 2 (which I needed to > look at first to have an opinion here). > I think you removed too much context, not clear what code you are referring to, last hunk above is an "and" instruction. > Jan Frediano
On 16.09.2024 09:44, Frediano Ziglio wrote: > On Sun, Sep 15, 2024 at 7:20 AM Jan Beulich <jbeulich@suse.com> wrote: >> >> On 10.09.2024 18:16, Frediano Ziglio wrote: >>> --- a/xen/arch/x86/boot/head.S >>> +++ b/xen/arch/x86/boot/head.S >>> @@ -231,6 +231,27 @@ __efi64_mb2_start: >>> /* VGA is not available on EFI platforms. */ >>> movl $0,vga_text_buffer(%rip) >>> >>> + /* >>> + * Align the stack as UEFI spec requires. Keep it aligned >>> + * before efi_multiboot2() call by pushing/popping even >>> + * numbers of items on it. >>> + */ >>> + and $~15,%rsp >> >> You don't use the stack below, so it's not clear if/why this needs >> moving. If it does, please add the missing blank after the comma >> (like you nicely do everywhere else). > > Fixed the blank. The reason is more clear if you look at the last > commit in the series, at least for the EFI part. For the BIOS/PVH part > is less clear, but the rationale is the same. The commit came from a > larger series where BIOS/PVH is mainly written in C so there is more > clear. If there's need to do it _here_, that need wants spelling out in the description. >> Apart from this there's the question on the precise placement of >> the calls - see respective comment on patch 2 (which I needed to >> look at first to have an opinion here). > > I think you removed too much context, not clear what code you are > referring to, last hunk above is an "and" instruction. This comment was on the patch as a whole, and I thought it was clear that it would refer to the two calls to the new function. The details of my concern there are, as said, in comments on patch 2. Jan
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index 12bbb97f33..de118d72f2 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -231,6 +231,27 @@ __efi64_mb2_start: /* VGA is not available on EFI platforms. */ movl $0,vga_text_buffer(%rip) + /* + * Align the stack as UEFI spec requires. Keep it aligned + * before efi_multiboot2() call by pushing/popping even + * numbers of items on it. + */ + and $~15,%rsp + + /* + * Initialize BSS (no nasty surprises!). + * It must be done earlier than in BIOS case + * because efi_multiboot2() touches it. + */ + mov %eax, %edx + lea __bss_start(%rip), %edi + lea __bss_end(%rip), %ecx + sub %edi, %ecx + shr $3, %ecx + xor %eax, %eax + rep stosq + mov %edx, %eax + /* Check for Multiboot2 bootloader. */ cmp $MULTIBOOT2_BOOTLOADER_MAGIC,%eax je .Lefi_multiboot2_proto @@ -321,34 +342,12 @@ __efi64_mb2_start: lea .Lmb2_no_ih(%rip),%r15 jz x86_32_switch - /* - * Align the stack as UEFI spec requires. Keep it aligned - * before efi_multiboot2() call by pushing/popping even - * numbers of items on it. - */ - and $~15,%rsp - /* Save Multiboot2 magic on the stack. */ push %rax /* Save EFI ImageHandle on the stack. */ push %rdi - /* - * Initialize BSS (no nasty surprises!). - * It must be done earlier than in BIOS case - * because efi_multiboot2() touches it. - */ - lea __bss_start(%rip),%edi - lea __bss_end(%rip),%ecx - sub %edi,%ecx - shr $3,%ecx - xor %eax,%eax - rep stosq - - /* Keep the stack aligned. Do not pop a single item off it. */ - mov (%rsp),%rdi - /* * efi_multiboot2() is called according to System V AMD64 ABI: * - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable, @@ -434,6 +433,8 @@ __pvh_start: /* Set up stack. */ lea STACK_SIZE - CPUINFO_sizeof + sym_esi(cpu0_stack), %esp + call initialise_bss + mov %ebx, sym_esi(pvh_start_info_pa) /* Force xen console. Will revert to user choice in init code. */ @@ -459,6 +460,25 @@ __pvh_start: #endif /* CONFIG_PVH_GUEST */ +initialise_bss: + /* + * Initialise the BSS. + * + * !!! WARNING - also zeroes the current stack !!! + */ + pop %ebp + mov %eax, %edx + + lea sym_esi(__bss_start), %edi + lea sym_esi(__bss_end), %ecx + sub %edi, %ecx + xor %eax, %eax + shr $2, %ecx + rep stosl + + mov %edx, %eax + jmp *%ebp + __start: cld cli @@ -489,6 +509,8 @@ __start: /* Set up stack. */ lea STACK_SIZE - CPUINFO_sizeof + sym_esi(cpu0_stack), %esp + call initialise_bss + /* Bootloaders may set multiboot{1,2}.mem_lower to a nonzero value. */ xor %edx,%edx @@ -645,26 +667,6 @@ trampoline_setup: * reserved for trampoline code and data. */ - /* - * Do not zero BSS on EFI platform here. - * It was initialized earlier. - */ - cmpb $0, sym_esi(efi_platform) - jnz 1f - - /* - * Initialise the BSS. - * - * !!! WARNING - also zeroes the current stack !!! - */ - lea sym_esi(__bss_start), %edi - lea sym_esi(__bss_end), %ecx - sub %edi,%ecx - xor %eax,%eax - shr $2,%ecx - rep stosl - -1: /* Interrogate CPU extended features via CPUID. */ mov $1, %eax cpuid
Allows to call C code earlier. Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> --- xen/arch/x86/boot/head.S | 86 ++++++++++++++++++++-------------------- 1 file changed, 44 insertions(+), 42 deletions(-)