[PATCH v3 0/4] x86/boot: Reduce assembly code

[PATCH v3 0/4] x86/boot: Reduce assembly code

[Frediano Ziglio]

This series came from part of the work of removing duplications between
boot code and rewriting part of code from assembly to C.
First 2 patches rework BIOS/PVH paths to reuse some code.
Third patch rewrites EFI code in pure C.

Changes since v1, more details in specific commits:
- style updates;
- comments and descriptions improvements;
- other improvements.

Changes since v2:
- rebased on master, resolved conflicts;
- add comment on trampoline section.

Frediano Ziglio (4):
  x86/boot: Initialise BSS sooner
  x86/boot: Refactor BIOS/PVH start
  x86/boot: Rewrite EFI/MBI2 code partly in C
  x86/boot: Improve MBI2 structure check

 xen/arch/x86/boot/head.S      | 288 +++++++++++-----------------------
 xen/arch/x86/efi/Makefile     |   1 +
 xen/arch/x86/efi/efi-boot.h   |   6 +-
 xen/arch/x86/efi/parse-mbi2.c |  61 +++++++
 xen/arch/x86/efi/stub.c       |   3 +-
 5 files changed, 155 insertions(+), 204 deletions(-)
 create mode 100644 xen/arch/x86/efi/parse-mbi2.c

-- 
2.34.1

[PATCH v3 1/4] x86/boot: Initialise BSS sooner

[Frediano Ziglio]

Allows to call C code earlier.
In order to safely call C code we need to setup stack, selectors and BSS.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
Changes since v1:
- improve commit message;
- improve some comments;
- fix some code style (spacing);
- set trampoline_phys as 32 bit value;
- use PAGE_SIZE mnemonic instead of 0x1000;
- use local label.
---
 xen/arch/x86/boot/head.S | 77 ++++++++++++++++++++--------------------
 1 file changed, 39 insertions(+), 38 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index cfc5a7b47d..fa21024042 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    .Linitialise_bss
+
         mov     %ebx, sym_esi(pvh_start_info_pa)
 
         /* Force xen console.  Will revert to user choice in init code. */
@@ -459,6 +460,20 @@ __pvh_start:
 
 #endif /* CONFIG_PVH_GUEST */
 
+.Linitialise_bss:
+	/* Initialise the BSS. */
+        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
+	ret
+
 __start:
         cld
         cli
@@ -489,6 +504,8 @@ __start:
         /* Set up stack. */
         lea     STACK_SIZE - CPUINFO_sizeof + sym_esi(cpu0_stack), %esp
 
+        call    .Linitialise_bss
+
         /* Bootloaders may set multiboot{1,2}.mem_lower to a nonzero value. */
         xor     %edx,%edx
 
@@ -645,22 +662,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. */
-        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
-- 
2.34.1

Re: [PATCH v3 1/4] x86/boot: Initialise BSS sooner

[Andrew Cooper]

On 24/09/2024 11:28 am, Frediano Ziglio wrote:
> @@ -459,6 +460,20 @@ __pvh_start:
>  
>  #endif /* CONFIG_PVH_GUEST */
>  
> +.Linitialise_bss:
> +	/* Initialise the BSS. */

You've had some tabs sneak in here.  I can drop on commit.

Also, I'd suggest:

        /* Initialise the BSS.  Preserve %eax (BOOTLOADER_MAGIC). */

With that, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

[PATCH v3 2/4] x86/boot: Refactor BIOS/PVH start

[Frediano Ziglio]

The 2 code paths were sharing quite some common code, reuse it instead
of having duplications.
Use %dl register to store boot type before running common code.
Using a 8 bit register reduces code size.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
Changes since v1:
- use %dl instead of %ebp to reduce code size;
- fold cli instruction;
- update some comments.
---
 xen/arch/x86/boot/head.S | 117 +++++++++++++++------------------------
 1 file changed, 45 insertions(+), 72 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index fa21024042..80bba6ff21 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -25,6 +25,9 @@
 #define MB2_HT(name)      (MULTIBOOT2_HEADER_TAG_##name)
 #define MB2_TT(name)      (MULTIBOOT2_TAG_TYPE_##name)
 
+#define BOOT_TYPE_BIOS 1
+#define BOOT_TYPE_PVH 2
+
         .macro mb2ht_args arg:req, args:vararg
         .long \arg
         .ifnb \args
@@ -409,17 +412,31 @@ cs32_switch:
 ELFNOTE(Xen, XEN_ELFNOTE_PHYS32_ENTRY, .long sym_offs(__pvh_start))
 
 __pvh_start:
-        cld
+        mov     $BOOT_TYPE_PVH, %dl
+        jmp     .Lcommon_bios_pvh
+#endif /* CONFIG_PVH_GUEST */
+
+__start:
+        mov     $BOOT_TYPE_BIOS, %dl
+
+.Lcommon_bios_pvh:
         cli
+        cld
 
         /*
-         * We need one call (i.e. push) to determine the load address.  See
-         * __start for a discussion on how to do this safely using the PVH
-         * info structure.
+         * Multiboot (both 1 and 2) and PVH specify the stack pointer as
+         * undefined.  This is unhelpful for relocatable images, where one
+         * call (i.e. push) is required to calculate the image's load address.
+         *
+         * Durig BIOS boot, there is one area of memory we know about with
+         * reasonable confidence that it isn't overlapped by Xen, and that's
+         * the Multiboot info structure in %ebx.  Use it as a temporary stack.
+         *
+         * During PVH boot use info structure in %ebx.
          */
 
         /* Preserve the field we're about to clobber. */
-        mov     (%ebx), %edx
+        mov     (%ebx), %ecx
         lea     4(%ebx), %esp
 
         /* Calculate the load base address. */
@@ -428,19 +445,12 @@ __pvh_start:
         sub     $sym_offs(1b), %esi
 
         /* Restore the clobbered field. */
-        mov     %edx, (%ebx)
+        mov     %ecx, (%ebx)
 
         /* Set up stack. */
         lea     STACK_SIZE - CPUINFO_sizeof + sym_esi(cpu0_stack), %esp
 
-        call    .Linitialise_bss
-
-        mov     %ebx, sym_esi(pvh_start_info_pa)
-
-        /* Force xen console.  Will revert to user choice in init code. */
-        movb    $-1, sym_esi(opt_console_xen)
-
-        /* Prepare gdt and segments */
+        /* Initialize GDTR and basic data segments. */
         add     %esi, sym_esi(gdt_boot_base)
         lgdt    sym_esi(gdt_boot_descr)
 
@@ -449,62 +459,40 @@ __pvh_start:
         mov     %ecx, %es
         mov     %ecx, %ss
 
-        /* Skip bootloader setup and bios setup, go straight to trampoline */
-        movb    $1, sym_esi(pvh_boot)
-        movb    $1, sym_esi(skip_realmode)
-
-        /* Set trampoline_phys to use mfn 1 to avoid having a mapping at VA 0 */
-        movw    $0x1000, sym_esi(trampoline_phys)
-        mov     (%ebx), %eax /* mov $XEN_HVM_START_MAGIC_VALUE, %eax */
-        jmp     trampoline_setup
-
-#endif /* CONFIG_PVH_GUEST */
+        /* Load null selector to unused segment registers. */
+        xor     %ecx, %ecx
+        mov     %ecx, %fs
+        mov     %ecx, %gs
 
-.Linitialise_bss:
 	/* Initialise the BSS. */
-        mov     %eax, %edx
-
+        mov     %eax, %ebp
         lea     sym_esi(__bss_start), %edi
         lea     sym_esi(__bss_end), %ecx
         sub     %edi, %ecx
         xor     %eax, %eax
         shr     $2, %ecx
         rep stosl
+        mov     %ebp, %eax
 
-        mov     %edx, %eax
-	ret
-
-__start:
-        cld
-        cli
-
-        /*
-         * Multiboot (both 1 and 2) specify the stack pointer as undefined
-         * when entering in BIOS circumstances.  This is unhelpful for
-         * relocatable images, where one call (i.e. push) is required to
-         * calculate the image's load address.
-         *
-         * This early in boot, there is one area of memory we know about with
-         * reasonable confidence that it isn't overlapped by Xen, and that's
-         * the Multiboot info structure in %ebx.  Use it as a temporary stack.
-         */
-
-        /* Preserve the field we're about to clobber. */
-        mov     (%ebx), %edx
-        lea     4(%ebx), %esp
+#ifdef CONFIG_PVH_GUEST
+        cmp     $BOOT_TYPE_PVH, %dl
+        jne     1f
 
-        /* Calculate the load base address. */
-        call    1f
-1:      pop     %esi
-        sub     $sym_offs(1b), %esi
+        mov     %ebx, sym_esi(pvh_start_info_pa)
 
-        /* Restore the clobbered field. */
-        mov     %edx, (%ebx)
+        /* Force xen console.  Will revert to user choice in init code. */
+        movb    $-1, sym_esi(opt_console_xen)
 
-        /* Set up stack. */
-        lea     STACK_SIZE - CPUINFO_sizeof + sym_esi(cpu0_stack), %esp
+        /* Skip bootloader setup and bios setup, go straight to trampoline */
+        movb    $1, sym_esi(pvh_boot)
+        movb    $1, sym_esi(skip_realmode)
 
-        call    .Linitialise_bss
+        /* Set trampoline_phys to use mfn 1 to avoid having a mapping at VA 0 */
+        movl    $PAGE_SIZE, sym_esi(trampoline_phys)
+        mov     (%ebx), %eax /* mov $XEN_HVM_START_MAGIC_VALUE, %eax */
+        jmp     trampoline_setup
+1:
+#endif /* CONFIG_PVH_GUEST */
 
         /* Bootloaders may set multiboot{1,2}.mem_lower to a nonzero value. */
         xor     %edx,%edx
@@ -563,22 +551,7 @@ __start:
 trampoline_bios_setup:
         /*
          * Called on legacy BIOS platforms only.
-         *
-         * Initialize GDTR and basic data segments.
          */
-        add     %esi,sym_esi(gdt_boot_base)
-        lgdt    sym_esi(gdt_boot_descr)
-
-        mov     $BOOT_DS,%ecx
-        mov     %ecx,%ds
-        mov     %ecx,%es
-        mov     %ecx,%ss
-        /* %esp is initialized later. */
-
-        /* Load null descriptor to unused segment registers. */
-        xor     %ecx,%ecx
-        mov     %ecx,%fs
-        mov     %ecx,%gs
 
         /* Set up trampoline segment 64k below EBDA */
         movzwl  0x40e,%ecx          /* EBDA segment */
-- 
2.34.1

Re: [PATCH v3 2/4] x86/boot: Refactor BIOS/PVH start

[Andrew Cooper]

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 fa21024042..80bba6ff21 100644
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
>  ELFNOTE(Xen, XEN_ELFNOTE_PHYS32_ENTRY, .long sym_offs(__pvh_start))
>  
>  __pvh_start:
> -        cld
> +        mov     $BOOT_TYPE_PVH, %dl
> +        jmp     .Lcommon_bios_pvh
> +#endif /* CONFIG_PVH_GUEST */
> +
> +__start:
> +        mov     $BOOT_TYPE_BIOS, %dl

Even if we're generally using %dl, these must be full %edx writes.

%edx commonly contains FMS on entry, and we don't want part of FMS left
in the upper half of the register.

> +
> +.Lcommon_bios_pvh:
>          cli
> +        cld
>  
>          /*
> -         * We need one call (i.e. push) to determine the load address.  See
> -         * __start for a discussion on how to do this safely using the PVH
> -         * info structure.
> +         * Multiboot (both 1 and 2) and PVH specify the stack pointer as
> +         * undefined.  This is unhelpful for relocatable images, where one
> +         * call (i.e. push) is required to calculate the image's load address.
> +         *
> +         * Durig BIOS boot, there is one area of memory we know about with
> +         * reasonable confidence that it isn't overlapped by Xen, and that's
> +         * the Multiboot info structure in %ebx.  Use it as a temporary stack.
> +         *
> +         * During PVH boot use info structure in %ebx.
>           */
>  
>          /* Preserve the field we're about to clobber. */
> -        mov     (%ebx), %edx
> +        mov     (%ebx), %ecx

Both here, and ...

>          lea     4(%ebx), %esp
>  
>          /* Calculate the load base address. */
> @@ -449,62 +459,40 @@ __pvh_start:
>          mov     %ecx, %es
>          mov     %ecx, %ss
>  
> -        /* Skip bootloader setup and bios setup, go straight to trampoline */
> -        movb    $1, sym_esi(pvh_boot)
> -        movb    $1, sym_esi(skip_realmode)
> -
> -        /* Set trampoline_phys to use mfn 1 to avoid having a mapping at VA 0 */
> -        movw    $0x1000, sym_esi(trampoline_phys)
> -        mov     (%ebx), %eax /* mov $XEN_HVM_START_MAGIC_VALUE, %eax */
> -        jmp     trampoline_setup
> -
> -#endif /* CONFIG_PVH_GUEST */
> +        /* Load null selector to unused segment registers. */
> +        xor     %ecx, %ecx
> +        mov     %ecx, %fs
> +        mov     %ecx, %gs
>  
> -.Linitialise_bss:
>  	/* Initialise the BSS. */
> -        mov     %eax, %edx
> -
> +        mov     %eax, %ebp

... here, we've got changes caused by now using %edx for a long-lived
purpose (and a change in linebreaks.)

For this, %ebp should be used straight away in patch 1.  I've not
committed it yet, so can fix that up.


I have to admit that I think this patch would be easier if the "use %ebx
for BOOT_TYPE_*" change was split out of "better merge the BIOS/PVH
paths".  That would at least get the incidental %edx changes out of the way.

Also, inserting

#ifdef CONFIG_PVH_GUEST
        cmp     $BOOT_TYPE_PVH, %dl
        jne     1f
1:
#endif /* CONFIG_PVH_GUEST */

in the same patch will probably make the subsequent diff far more legible.

Thoughts?

I might give this a quick go, and see how it ends up looking...

~Andrew

Re: [PATCH v3 2/4] x86/boot: Refactor BIOS/PVH start

[Andrew Cooper]

On 24/09/2024 2:25 pm, Andrew Cooper wrote:
> 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 fa21024042..80bba6ff21 100644
>> --- a/xen/arch/x86/boot/head.S
>> +++ b/xen/arch/x86/boot/head.S
>>  ELFNOTE(Xen, XEN_ELFNOTE_PHYS32_ENTRY, .long sym_offs(__pvh_start))
>>  
>>  __pvh_start:
>> -        cld
>> +        mov     $BOOT_TYPE_PVH, %dl
>> +        jmp     .Lcommon_bios_pvh
>> +#endif /* CONFIG_PVH_GUEST */
>> +
>> +__start:
>> +        mov     $BOOT_TYPE_BIOS, %dl
> Even if we're generally using %dl, these must be full %edx writes.
>
> %edx commonly contains FMS on entry, and we don't want part of FMS left
> in the upper half of the register.
>
>> +
>> +.Lcommon_bios_pvh:
>>          cli
>> +        cld
>>  
>>          /*
>> -         * We need one call (i.e. push) to determine the load address.  See
>> -         * __start for a discussion on how to do this safely using the PVH
>> -         * info structure.
>> +         * Multiboot (both 1 and 2) and PVH specify the stack pointer as
>> +         * undefined.  This is unhelpful for relocatable images, where one
>> +         * call (i.e. push) is required to calculate the image's load address.
>> +         *
>> +         * Durig BIOS boot, there is one area of memory we know about with
>> +         * reasonable confidence that it isn't overlapped by Xen, and that's
>> +         * the Multiboot info structure in %ebx.  Use it as a temporary stack.
>> +         *
>> +         * During PVH boot use info structure in %ebx.
>>           */
>>  
>>          /* Preserve the field we're about to clobber. */
>> -        mov     (%ebx), %edx
>> +        mov     (%ebx), %ecx
> Both here, and ...
>
>>          lea     4(%ebx), %esp
>>  
>>          /* Calculate the load base address. */
>> @@ -449,62 +459,40 @@ __pvh_start:
>>          mov     %ecx, %es
>>          mov     %ecx, %ss
>>  
>> -        /* Skip bootloader setup and bios setup, go straight to trampoline */
>> -        movb    $1, sym_esi(pvh_boot)
>> -        movb    $1, sym_esi(skip_realmode)
>> -
>> -        /* Set trampoline_phys to use mfn 1 to avoid having a mapping at VA 0 */
>> -        movw    $0x1000, sym_esi(trampoline_phys)
>> -        mov     (%ebx), %eax /* mov $XEN_HVM_START_MAGIC_VALUE, %eax */
>> -        jmp     trampoline_setup
>> -
>> -#endif /* CONFIG_PVH_GUEST */
>> +        /* Load null selector to unused segment registers. */
>> +        xor     %ecx, %ecx
>> +        mov     %ecx, %fs
>> +        mov     %ecx, %gs
>>  
>> -.Linitialise_bss:
>>  	/* Initialise the BSS. */
>> -        mov     %eax, %edx
>> -
>> +        mov     %eax, %ebp
> ... here, we've got changes caused by now using %edx for a long-lived
> purpose (and a change in linebreaks.)
>
> For this, %ebp should be used straight away in patch 1.  I've not
> committed it yet, so can fix that up.
>
>
> I have to admit that I think this patch would be easier if the "use %ebx
> for BOOT_TYPE_*" change was split out of "better merge the BIOS/PVH
> paths".  That would at least get the incidental %edx changes out of the way.
>
> Also, inserting
>
> #ifdef CONFIG_PVH_GUEST
>         cmp     $BOOT_TYPE_PVH, %dl
>         jne     1f
> 1:
> #endif /* CONFIG_PVH_GUEST */
>
> in the same patch will probably make the subsequent diff far more legible.
>
> Thoughts?
>
> I might give this a quick go, and see how it ends up looking...

Actually, why do we need BOOT_TYPE_* at all?  We've already got
BOOTLOADER_MAGIC in %eax which can be used to identify PVH.

~Andrew

Re: [PATCH v3 2/4] x86/boot: Refactor BIOS/PVH start

[Andrew Cooper]

On 24/09/2024 2:30 pm, Andrew Cooper wrote:
> On 24/09/2024 2:25 pm, Andrew Cooper wrote:
>> 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 fa21024042..80bba6ff21 100644
>>> --- a/xen/arch/x86/boot/head.S
>>> +++ b/xen/arch/x86/boot/head.S
>>>  ELFNOTE(Xen, XEN_ELFNOTE_PHYS32_ENTRY, .long sym_offs(__pvh_start))
>>>  
>>>  __pvh_start:
>>> -        cld
>>> +        mov     $BOOT_TYPE_PVH, %dl
>>> +        jmp     .Lcommon_bios_pvh
>>> +#endif /* CONFIG_PVH_GUEST */
>>> +
>>> +__start:
>>> +        mov     $BOOT_TYPE_BIOS, %dl
>> Even if we're generally using %dl, these must be full %edx writes.
>>
>> %edx commonly contains FMS on entry, and we don't want part of FMS left
>> in the upper half of the register.
>>
>>> +
>>> +.Lcommon_bios_pvh:
>>>          cli
>>> +        cld
>>>  
>>>          /*
>>> -         * We need one call (i.e. push) to determine the load address.  See
>>> -         * __start for a discussion on how to do this safely using the PVH
>>> -         * info structure.
>>> +         * Multiboot (both 1 and 2) and PVH specify the stack pointer as
>>> +         * undefined.  This is unhelpful for relocatable images, where one
>>> +         * call (i.e. push) is required to calculate the image's load address.
>>> +         *
>>> +         * Durig BIOS boot, there is one area of memory we know about with
>>> +         * reasonable confidence that it isn't overlapped by Xen, and that's
>>> +         * the Multiboot info structure in %ebx.  Use it as a temporary stack.
>>> +         *
>>> +         * During PVH boot use info structure in %ebx.
>>>           */
>>>  
>>>          /* Preserve the field we're about to clobber. */
>>> -        mov     (%ebx), %edx
>>> +        mov     (%ebx), %ecx
>> Both here, and ...
>>
>>>          lea     4(%ebx), %esp
>>>  
>>>          /* Calculate the load base address. */
>>> @@ -449,62 +459,40 @@ __pvh_start:
>>>          mov     %ecx, %es
>>>          mov     %ecx, %ss
>>>  
>>> -        /* Skip bootloader setup and bios setup, go straight to trampoline */
>>> -        movb    $1, sym_esi(pvh_boot)
>>> -        movb    $1, sym_esi(skip_realmode)
>>> -
>>> -        /* Set trampoline_phys to use mfn 1 to avoid having a mapping at VA 0 */
>>> -        movw    $0x1000, sym_esi(trampoline_phys)
>>> -        mov     (%ebx), %eax /* mov $XEN_HVM_START_MAGIC_VALUE, %eax */
>>> -        jmp     trampoline_setup
>>> -
>>> -#endif /* CONFIG_PVH_GUEST */
>>> +        /* Load null selector to unused segment registers. */
>>> +        xor     %ecx, %ecx
>>> +        mov     %ecx, %fs
>>> +        mov     %ecx, %gs
>>>  
>>> -.Linitialise_bss:
>>>  	/* Initialise the BSS. */
>>> -        mov     %eax, %edx
>>> -
>>> +        mov     %eax, %ebp
>> ... here, we've got changes caused by now using %edx for a long-lived
>> purpose (and a change in linebreaks.)
>>
>> For this, %ebp should be used straight away in patch 1.  I've not
>> committed it yet, so can fix that up.
>>
>>
>> I have to admit that I think this patch would be easier if the "use %ebx
>> for BOOT_TYPE_*" change was split out of "better merge the BIOS/PVH
>> paths".  That would at least get the incidental %edx changes out of the way.
>>
>> Also, inserting
>>
>> #ifdef CONFIG_PVH_GUEST
>>         cmp     $BOOT_TYPE_PVH, %dl
>>         jne     1f
>> 1:
>> #endif /* CONFIG_PVH_GUEST */
>>
>> in the same patch will probably make the subsequent diff far more legible.
>>
>> Thoughts?
>>
>> I might give this a quick go, and see how it ends up looking...
> Actually, why do we need BOOT_TYPE_* at all?  We've already got
> BOOTLOADER_MAGIC in %eax which can be used to identify PVH.

Summary of the "quick look".

The suggested empty ifdefary doesn't really help.

However, moving the

    mov     (%ebx), %eax /* mov $XEN_HVM_START_MAGIC_VALUE, %eax */

into __pvh_entry avoids the need for any BOOT_TYPE_* being held in %edx.

The one place where it's needed can be `cmp $XEN_ ...; jne` and this
avoids needing to shuffle the register scheduling.

~Andrew

[PATCH v3 3/4] x86/boot: Rewrite EFI/MBI2 code partly in C

[Frediano Ziglio]

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

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";
-- 
2.34.1

Re: [PATCH v3 3/4] x86/boot: Rewrite EFI/MBI2 code partly in C

[Andrew Cooper]

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

Re: [PATCH v3 3/4] x86/boot: Rewrite EFI/MBI2 code partly in C

[Jan Beulich]

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

Re: [PATCH v3 3/4] x86/boot: Rewrite EFI/MBI2 code partly in C

[Frediano Ziglio]

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

Re: [PATCH v3 3/4] x86/boot: Rewrite EFI/MBI2 code partly in C

[Jan Beulich]

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

[PATCH v3 4/4] x86/boot: Improve MBI2 structure check

[Frediano Ziglio]

Tag structure should contain at least the tag header.
Entire tag structure must be contained inside MBI2 data.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
 xen/arch/x86/efi/parse-mbi2.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/efi/parse-mbi2.c b/xen/arch/x86/efi/parse-mbi2.c
index 6038f35b16..7efda8fab2 100644
--- a/xen/arch/x86/efi/parse-mbi2.c
+++ b/xen/arch/x86/efi/parse-mbi2.c
@@ -22,6 +22,7 @@ efi_parse_mbi2(uint32_t magic, const multiboot2_fixed_t *mbi)
     EFI_HANDLE ImageHandle = NULL;
     EFI_SYSTEM_TABLE *SystemTable = NULL;
     const char *cmdline = NULL;
+    const void *const mbi_end = (const void *)mbi + mbi->total_size;
     bool have_bs = false;
 
     if ( magic != MULTIBOOT2_BOOTLOADER_MAGIC )
@@ -30,7 +31,9 @@ efi_parse_mbi2(uint32_t magic, const multiboot2_fixed_t *mbi)
     /* 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
+    for ( ; (const void *)(tag + 1) <= mbi_end
+            && tag->size >= sizeof(*tag)
+            && (const void *)tag + tag->size <= mbi_end
             && tag->type != MULTIBOOT2_TAG_TYPE_END;
           tag = _p(ROUNDUP((unsigned long)((const void *)tag + tag->size),
                    MULTIBOOT2_TAG_ALIGN)) )
-- 
2.34.1

Re: [PATCH v3 4/4] x86/boot: Improve MBI2 structure check

[Andrew Cooper]

On 24/09/2024 11:28 am, Frediano Ziglio wrote:
> Tag structure should contain at least the tag header.
> Entire tag structure must be contained inside MBI2 data.
>
> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
> ---
>  xen/arch/x86/efi/parse-mbi2.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/efi/parse-mbi2.c b/xen/arch/x86/efi/parse-mbi2.c
> index 6038f35b16..7efda8fab2 100644
> --- a/xen/arch/x86/efi/parse-mbi2.c
> +++ b/xen/arch/x86/efi/parse-mbi2.c
> @@ -22,6 +22,7 @@ efi_parse_mbi2(uint32_t magic, const multiboot2_fixed_t *mbi)
>      EFI_HANDLE ImageHandle = NULL;
>      EFI_SYSTEM_TABLE *SystemTable = NULL;
>      const char *cmdline = NULL;
> +    const void *const mbi_end = (const void *)mbi + mbi->total_size;
>      bool have_bs = false;
>  
>      if ( magic != MULTIBOOT2_BOOTLOADER_MAGIC )
> @@ -30,7 +31,9 @@ efi_parse_mbi2(uint32_t magic, const multiboot2_fixed_t *mbi)
>      /* 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
> +    for ( ; (const void *)(tag + 1) <= mbi_end
> +            && tag->size >= sizeof(*tag)
> +            && (const void *)tag + tag->size <= mbi_end
>              && tag->type != MULTIBOOT2_TAG_TYPE_END;
>            tag = _p(ROUNDUP((unsigned long)((const void *)tag + tag->size),
>                     MULTIBOOT2_TAG_ALIGN)) )

I'd merge this into the previous patch.  There's no reason to keep it
separate.

Also a minor style note I forgot, &&'s on the end of the previous line
please.

~Andrew

Re: [PATCH v3 4/4] x86/boot: Improve MBI2 structure check

[Frediano Ziglio]

On Tue, Sep 24, 2024 at 3:15 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 24/09/2024 11:28 am, Frediano Ziglio wrote:
> > Tag structure should contain at least the tag header.
> > Entire tag structure must be contained inside MBI2 data.
> >
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
> > ---
> >  xen/arch/x86/efi/parse-mbi2.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/x86/efi/parse-mbi2.c b/xen/arch/x86/efi/parse-mbi2.c
> > index 6038f35b16..7efda8fab2 100644
> > --- a/xen/arch/x86/efi/parse-mbi2.c
> > +++ b/xen/arch/x86/efi/parse-mbi2.c
> > @@ -22,6 +22,7 @@ efi_parse_mbi2(uint32_t magic, const multiboot2_fixed_t *mbi)
> >      EFI_HANDLE ImageHandle = NULL;
> >      EFI_SYSTEM_TABLE *SystemTable = NULL;
> >      const char *cmdline = NULL;
> > +    const void *const mbi_end = (const void *)mbi + mbi->total_size;
> >      bool have_bs = false;
> >
> >      if ( magic != MULTIBOOT2_BOOTLOADER_MAGIC )
> > @@ -30,7 +31,9 @@ efi_parse_mbi2(uint32_t magic, const multiboot2_fixed_t *mbi)
> >      /* 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
> > +    for ( ; (const void *)(tag + 1) <= mbi_end
> > +            && tag->size >= sizeof(*tag)
> > +            && (const void *)tag + tag->size <= mbi_end
> >              && tag->type != MULTIBOOT2_TAG_TYPE_END;
> >            tag = _p(ROUNDUP((unsigned long)((const void *)tag + tag->size),
> >                     MULTIBOOT2_TAG_ALIGN)) )
>
> I'd merge this into the previous patch.  There's no reason to keep it
> separate.
>
> Also a minor style note I forgot, &&'s on the end of the previous line
> please.
>

The reason is that the rationale is different.
The previous patch is converting assembly to C, this is improving.
Merging would make the conversion point void.

Frediano