[PATCH v7 0/2] x86/boot: Reduce assembly code

[PATCH v7 0/2] 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.
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.

Changes since v3:
- changed new function name;
- declare efi_multiboot2 in a separate header;
- distinguish entry point from using magic number;
- other minor changes (see commens in commits).

Changes since v4:
- rebase on staging;
- set %fs and %gs as other segment registers;
- style and other changes.

Changes since v5:
- fixed a typo.

Changes since v6:
- remove merged patch;
- comment and style;
- change some pointer checks to avoid overflows;
- rename parse-mbi2.c to mbi2.c.

Frediano Ziglio (2):
  x86/boot: Rewrite EFI/MBI2 code partly in C
  x86/boot: Improve MBI2 structure check

 xen/arch/x86/boot/head.S       | 146 +++++++--------------------------
 xen/arch/x86/efi/Makefile      |   1 +
 xen/arch/x86/efi/efi-boot.h    |   7 +-
 xen/arch/x86/efi/mbi2.c        |  66 +++++++++++++++
 xen/arch/x86/efi/stub.c        |  10 +--
 xen/arch/x86/include/asm/efi.h |  18 ++++
 6 files changed, 123 insertions(+), 125 deletions(-)
 create mode 100644 xen/arch/x86/efi/mbi2.c
 create mode 100644 xen/arch/x86/include/asm/efi.h

-- 
2.34.1

[PATCH v7 1/2] x86/boot: Rewrite EFI/MBI2 code partly in C

[Frediano Ziglio]

No need to have it coded in assembly.
Declare efi_multiboot2 in a new header to reuse between implementations
and caller.

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).

Changes since v3:
- rename new function to efi_multiboot2_prelude;
- declare efi_multiboot2 in a separate header.

Changes since v4:
- fix some style and space;
- fix MISRA requirement.

Changes since v6:
- include new header to get common declaration;
- add a comment in assembly code;
- rename parse-mbi2.c to mbi2.c.
---
 xen/arch/x86/boot/head.S       | 146 +++++++--------------------------
 xen/arch/x86/efi/Makefile      |   1 +
 xen/arch/x86/efi/efi-boot.h    |   7 +-
 xen/arch/x86/efi/mbi2.c        |  63 ++++++++++++++
 xen/arch/x86/efi/stub.c        |  10 +--
 xen/arch/x86/include/asm/efi.h |  18 ++++
 6 files changed, 120 insertions(+), 125 deletions(-)
 create mode 100644 xen/arch/x86/efi/mbi2.c
 create mode 100644 xen/arch/x86/include/asm/efi.h

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index e0901ee400..987345fa34 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -121,8 +121,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!"
@@ -161,17 +159,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
@@ -189,6 +176,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:
@@ -235,53 +226,49 @@ __efi64_mb2_start:
 
         /*
          * Align the stack as UEFI spec requires. Keep it aligned
-         * before efi_multiboot2() call by pushing/popping even
+         * before efi_multiboot2_prelude() call by pushing/popping even
          * numbers of items on it.
          */
         and     $~15, %rsp
 
+        /* Save magic number, we need it later but we need to use %eax. */
+        mov     %eax, %edx
+
         /*
          * Initialize BSS (no nasty surprises!).
          * It must be done earlier than in BIOS case
-         * because efi_multiboot2() touches it.
+         * because efi_multiboot2_prelude() 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
-
-        /* Jump to .Lnot_multiboot after switching CPU to x86_32 mode. */
-        lea     .Lnot_multiboot(%rip), %r15
-        jmp     x86_32_switch
 
-.Lefi_multiboot2_proto:
-        /* Zero EFI SystemTable, EFI ImageHandle addresses and cmdline. */
-        xor     %esi,%esi
-        xor     %edi,%edi
-        xor     %edx,%edx
-
-        /* Skip Multiboot2 information fixed part. */
-        lea     (MB2_fixed_sizeof+MULTIBOOT2_TAG_ALIGN-1)(%rbx),%ecx
-        and     $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
-
-.Lefi_mb2_tsize:
-        /* Check Multiboot2 information total size. */
-        mov     %ecx,%r8d
-        sub     %ebx,%r8d
-        cmp     %r8d,MB2_fixed_total_size(%rbx)
-        jbe     .Lrun_bs
+        /*
+         * Spill MB2 magic.
+         * Spill the pointer too, to keep the stack aligned.
+         */
+        push    %rdx
+        push    %rbx
 
-        /* Are EFI boot services available? */
-        cmpl    $MULTIBOOT2_TAG_TYPE_EFI_BS,MB2_tag_type(%rcx)
-        jne     .Lefi_mb2_st
+        /*
+         * efi_multiboot2_prelude() 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_multiboot2_prelude
+        lea     .Ldirect_error(%rip), %r15
+        test    %rax, %rax
+        jnz     x86_32_switch
+
+        /* Restore Multiboot2 pointer and magic. */
+        pop     %rbx
+        pop     %rax
 
         /* We are on EFI platform and EFI boot services are available. */
         incb    efi_platform(%rip)
@@ -291,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..7e2b5c07de 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 += 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..94f3443364 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -10,6 +10,7 @@
 #include <asm/msr.h>
 #include <asm/setup.h>
 #include <asm/trampoline.h>
+#include <asm/efi.h>
 
 static struct file __initdata ucode;
 static multiboot_info_t __initdata mbi = {
@@ -816,9 +817,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/mbi2.c b/xen/arch/x86/efi/mbi2.c
new file mode 100644
index 0000000000..55a1777483
--- /dev/null
+++ b/xen/arch/x86/efi/mbi2.c
@@ -0,0 +1,63 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include <xen/efi.h>
+#include <xen/init.h>
+#include <xen/multiboot2.h>
+#include <asm/asm_defns.h>
+#include <asm/efi.h>
+
+const char * asmlinkage __init
+efi_multiboot2_prelude(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)tag + tag->size,
+                   MULTIBOOT2_TAG_ALIGN)) )
+    {
+        switch ( tag->type )
+        {
+        case MULTIBOOT2_TAG_TYPE_EFI_BS:
+            have_bs = true;
+            break;
+
+        case MULTIBOOT2_TAG_TYPE_EFI64:
+            SystemTable = _p(((const multiboot2_tag_efi64_t *)tag)->pointer);
+            break;
+
+        case MULTIBOOT2_TAG_TYPE_EFI64_IH:
+            ImageHandle = _p(((const multiboot2_tag_efi64_ih_t *)tag)->pointer);
+            break;
+
+        case MULTIBOOT2_TAG_TYPE_CMDLINE:
+            cmdline = ((const multiboot2_tag_string_t *)tag)->string;
+            break;
+
+        default:
+            /* Satisfy MISRA requirement. */
+            break;
+        }
+    }
+
+    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..7d824151a7 100644
--- a/xen/arch/x86/efi/stub.c
+++ b/xen/arch/x86/efi/stub.c
@@ -1,13 +1,8 @@
 #include <xen/efi.h>
 #include <xen/init.h>
 #include <asm/asm_defns.h>
-#include <asm/efibind.h>
+#include <asm/efi.h>
 #include <asm/page.h>
-#include <efi/efidef.h>
-#include <efi/eficapsule.h>
-#include <efi/eficon.h>
-#include <efi/efidevp.h>
-#include <efi/efiapi.h>
 
 /*
  * Here we are in EFI stub. EFI calls are not supported due to lack
@@ -17,7 +12,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";
diff --git a/xen/arch/x86/include/asm/efi.h b/xen/arch/x86/include/asm/efi.h
new file mode 100644
index 0000000000..575a33e302
--- /dev/null
+++ b/xen/arch/x86/include/asm/efi.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef X86_ASM_EFI_H
+#define X86_ASM_EFI_H
+
+#include <xen/types.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 efi_multiboot2(EFI_HANDLE ImageHandle,
+                    EFI_SYSTEM_TABLE *SystemTable,
+                    const char *cmdline);
+
+#endif /* X86_ASM_EFI_H */
-- 
2.34.1

Re: [PATCH v7 1/2] x86/boot: Rewrite EFI/MBI2 code partly in C

[Jan Beulich]

On 01.10.2024 12:22, Frediano Ziglio wrote:
> No need to have it coded in assembly.
> Declare efi_multiboot2 in a new header to reuse between implementations
> and caller.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v7 1/2] x86/boot: Rewrite EFI/MBI2 code partly in C

[Daniel P. Smith]

On 10/1/24 06:22, Frediano Ziglio wrote:
> No need to have it coded in assembly.
> Declare efi_multiboot2 in a new header to reuse between implementations
> and caller.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>

I unfortunately do not have time to test this myself, but I have given a 
read through and it looks good to me. I will give it an R-b and let 
Marek provide the A-b when he is comfortable that CI failure is an 
artifact of the test system and not this series.

Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>

Re: [PATCH v7 1/2] x86/boot: Rewrite EFI/MBI2 code partly in C

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 879 bytes --]

On Wed, Oct 02, 2024 at 10:31:50AM -0400, Daniel P. Smith wrote:
> On 10/1/24 06:22, Frediano Ziglio wrote:
> > No need to have it coded in assembly.
> > Declare efi_multiboot2 in a new header to reuse between implementations
> > and caller.
> > 
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
> 
> I unfortunately do not have time to test this myself, but I have given a
> read through and it looks good to me. I will give it an R-b and let Marek
> provide the A-b when he is comfortable that CI failure is an artifact of the
> test system and not this series.
> 
> Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>

Since it seems it's only the other patch causing issues, for this one:

Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v7 2/2] 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>
---
Changes since v6:
- compare against total_size every time to avoid overflows.
---
 xen/arch/x86/efi/mbi2.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/efi/mbi2.c b/xen/arch/x86/efi/mbi2.c
index 55a1777483..935f3ae5d0 100644
--- a/xen/arch/x86/efi/mbi2.c
+++ b/xen/arch/x86/efi/mbi2.c
@@ -13,6 +13,7 @@ efi_multiboot2_prelude(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_raw = (const void *)mbi;
     bool have_bs = false;
 
     if ( magic != MULTIBOOT2_BOOTLOADER_MAGIC )
@@ -21,7 +22,9 @@ efi_multiboot2_prelude(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_raw <= mbi->total_size &&
+            tag->size >= sizeof(*tag) &&
+            (const void *)tag + tag->size - mbi_raw <= mbi->total_size &&
             tag->type != MULTIBOOT2_TAG_TYPE_END;
           tag = _p(ROUNDUP((unsigned long)tag + tag->size,
                    MULTIBOOT2_TAG_ALIGN)) )
-- 
2.34.1

Re: [PATCH v7 2/2] x86/boot: Improve MBI2 structure check

[Jan Beulich]

On 01.10.2024 12:22, Frediano Ziglio wrote:
> --- a/xen/arch/x86/efi/mbi2.c
> +++ b/xen/arch/x86/efi/mbi2.c
> @@ -13,6 +13,7 @@ efi_multiboot2_prelude(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_raw = (const void *)mbi;
>      bool have_bs = false;
>  
>      if ( magic != MULTIBOOT2_BOOTLOADER_MAGIC )
> @@ -21,7 +22,9 @@ efi_multiboot2_prelude(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_raw <= mbi->total_size &&
> +            tag->size >= sizeof(*tag) &&
> +            (const void *)tag + tag->size - mbi_raw <= mbi->total_size &&
>              tag->type != MULTIBOOT2_TAG_TYPE_END;
>            tag = _p(ROUNDUP((unsigned long)tag + tag->size,
>                     MULTIBOOT2_TAG_ALIGN)) )

Hmm, looks like what I said on the earlier version still wasn't unambiguous
enough; I'm sorry. There is still potential for intermediate overflows in
the calculations. _If_ we care about avoiding overflows, we need to avoid
all of that. Even more so that Misra may not like this sort of pointer
calculations. You know tag >= mbi_raw, so tag - mbi_raw is always valid to
calculate. tag->size can further be checked to be less than mbi->total_size,
at which point mbi->total_size - tag->size can also be calculated without
risking {over,under}flow. (Similar then for the earlier (tag + 1) check.)

Jan

Re: [PATCH v7 2/2] x86/boot: Improve MBI2 structure check

[Frediano Ziglio]

On Tue, Oct 1, 2024 at 5:02 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 01.10.2024 12:22, Frediano Ziglio wrote:
> > --- a/xen/arch/x86/efi/mbi2.c
> > +++ b/xen/arch/x86/efi/mbi2.c
> > @@ -13,6 +13,7 @@ efi_multiboot2_prelude(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_raw = (const void *)mbi;
> >      bool have_bs = false;
> >
> >      if ( magic != MULTIBOOT2_BOOTLOADER_MAGIC )
> > @@ -21,7 +22,9 @@ efi_multiboot2_prelude(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_raw <= mbi->total_size &&
> > +            tag->size >= sizeof(*tag) &&
> > +            (const void *)tag + tag->size - mbi_raw <= mbi->total_size &&
> >              tag->type != MULTIBOOT2_TAG_TYPE_END;
> >            tag = _p(ROUNDUP((unsigned long)tag + tag->size,
> >                     MULTIBOOT2_TAG_ALIGN)) )
>
> Hmm, looks like what I said on the earlier version still wasn't unambiguous
> enough; I'm sorry. There is still potential for intermediate overflows in
> the calculations. _If_ we care about avoiding overflows, we need to avoid
> all of that. Even more so that Misra may not like this sort of pointer
> calculations. You know tag >= mbi_raw, so tag - mbi_raw is always valid to
> calculate. tag->size can further be checked to be less than mbi->total_size,
> at which point mbi->total_size - tag->size can also be calculated without
> risking {over,under}flow. (Similar then for the earlier (tag + 1) check.)
>
> Jan

Hi,
  personally, I don't care much about checking for overflows here.
It's not that we are in a higher security level, and we need to check
malicious intentions (like when user calls the kernel or a VM calls
the hypervisor), if the loader (which is at the same security level)
is passing garbage we are going to crash.

Saying that with this commit Marek checks are failing, I was thinking
about dropping this commit completely. Yes, in theory better checks
are better, but if we cause regression and boot failures maybe it's
better to allow some wrong data. That's one reason I wanted to keep
this commit separate from the other, which is just translating what
assembly code was doing, not introducing any regression (good or bad)
in behavior. I think it would be worth investigating what Marek
machine is passing in order to make sure we accept whatever wrong data
we are receiving (or understanding why more checks cause the issue).

Frediano

Re: [PATCH v7 0/2] x86/boot: Reduce assembly code

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 2416 bytes --]

On Tue, Oct 01, 2024 at 11:22:37AM +0100, Frediano Ziglio wrote:
> This series came from part of the work of removing duplications between
> boot code and rewriting part of code from assembly to C.
> Rewrites EFI code in pure C.

The MB2+EFI tests on Adler Lake fail with this series:
https://gitlab.com/xen-project/people/marmarek/xen/-/pipelines/1478766782
Looking at the VGA output (unfortunately not collected by the test
itself) it hangs just after bootloader, before printing anything on the
screen (or even clearing it after bootloader). The serial is silent too.

It does pass on Zen 3+ runners.

Since there were some issues with the ADL runner today on plain staging,
I'm not 100% sure if it isn't some infrastructure issue yet. But the
symptoms look different than usual infra issues (and different than
todays failures on staging), so I think it's more likely an issue with
the patches here.

> 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.
> 
> Changes since v3:
> - changed new function name;
> - declare efi_multiboot2 in a separate header;
> - distinguish entry point from using magic number;
> - other minor changes (see commens in commits).
> 
> Changes since v4:
> - rebase on staging;
> - set %fs and %gs as other segment registers;
> - style and other changes.
> 
> Changes since v5:
> - fixed a typo.
> 
> Changes since v6:
> - remove merged patch;
> - comment and style;
> - change some pointer checks to avoid overflows;
> - rename parse-mbi2.c to mbi2.c.
> 
> Frediano Ziglio (2):
>   x86/boot: Rewrite EFI/MBI2 code partly in C
>   x86/boot: Improve MBI2 structure check
> 
>  xen/arch/x86/boot/head.S       | 146 +++++++--------------------------
>  xen/arch/x86/efi/Makefile      |   1 +
>  xen/arch/x86/efi/efi-boot.h    |   7 +-
>  xen/arch/x86/efi/mbi2.c        |  66 +++++++++++++++
>  xen/arch/x86/efi/stub.c        |  10 +--
>  xen/arch/x86/include/asm/efi.h |  18 ++++
>  6 files changed, 123 insertions(+), 125 deletions(-)
>  create mode 100644 xen/arch/x86/efi/mbi2.c
>  create mode 100644 xen/arch/x86/include/asm/efi.h
> 
> -- 
> 2.34.1
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v7 0/2] x86/boot: Reduce assembly code

[Frediano Ziglio]

On Wed, Oct 2, 2024 at 3:04 PM Marek Marczykowski-Górecki
<marmarek@invisiblethingslab.com> wrote:
>
> On Tue, Oct 01, 2024 at 11:22:37AM +0100, Frediano Ziglio wrote:
> > This series came from part of the work of removing duplications between
> > boot code and rewriting part of code from assembly to C.
> > Rewrites EFI code in pure C.
>
> The MB2+EFI tests on Adler Lake fail with this series:
> https://gitlab.com/xen-project/people/marmarek/xen/-/pipelines/1478766782
> Looking at the VGA output (unfortunately not collected by the test
> itself) it hangs just after bootloader, before printing anything on the
> screen (or even clearing it after bootloader). The serial is silent too.
>

I tried multiple times to take a look at the logs, but I keep getting error 500.

> It does pass on Zen 3+ runners.
>
> Since there were some issues with the ADL runner today on plain staging,
> I'm not 100% sure if it isn't some infrastructure issue yet. But the
> symptoms look different than usual infra issues (and different than
> todays failures on staging), so I think it's more likely an issue with
> the patches here.
>
> > 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.
> >
> > Changes since v3:
> > - changed new function name;
> > - declare efi_multiboot2 in a separate header;
> > - distinguish entry point from using magic number;
> > - other minor changes (see commens in commits).
> >
> > Changes since v4:
> > - rebase on staging;
> > - set %fs and %gs as other segment registers;
> > - style and other changes.
> >
> > Changes since v5:
> > - fixed a typo.
> >
> > Changes since v6:
> > - remove merged patch;
> > - comment and style;
> > - change some pointer checks to avoid overflows;
> > - rename parse-mbi2.c to mbi2.c.
> >
> > Frediano Ziglio (2):
> >   x86/boot: Rewrite EFI/MBI2 code partly in C
> >   x86/boot: Improve MBI2 structure check
> >
> >  xen/arch/x86/boot/head.S       | 146 +++++++--------------------------
> >  xen/arch/x86/efi/Makefile      |   1 +
> >  xen/arch/x86/efi/efi-boot.h    |   7 +-
> >  xen/arch/x86/efi/mbi2.c        |  66 +++++++++++++++
> >  xen/arch/x86/efi/stub.c        |  10 +--
> >  xen/arch/x86/include/asm/efi.h |  18 ++++
> >  6 files changed, 123 insertions(+), 125 deletions(-)
> >  create mode 100644 xen/arch/x86/efi/mbi2.c
> >  create mode 100644 xen/arch/x86/include/asm/efi.h
> >

Frediano

Re: [PATCH v7 0/2] x86/boot: Reduce assembly code

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 3826 bytes --]

On Wed, Oct 02, 2024 at 04:27:19PM +0100, Frediano Ziglio wrote:
> On Wed, Oct 2, 2024 at 3:04 PM Marek Marczykowski-Górecki
> <marmarek@invisiblethingslab.com> wrote:
> >
> > On Tue, Oct 01, 2024 at 11:22:37AM +0100, Frediano Ziglio wrote:
> > > This series came from part of the work of removing duplications between
> > > boot code and rewriting part of code from assembly to C.
> > > Rewrites EFI code in pure C.
> >
> > The MB2+EFI tests on Adler Lake fail with this series:
> > https://gitlab.com/xen-project/people/marmarek/xen/-/pipelines/1478766782
> > Looking at the VGA output (unfortunately not collected by the test
> > itself) it hangs just after bootloader, before printing anything on the
> > screen (or even clearing it after bootloader). The serial is silent too.
> >
> 
> I tried multiple times to take a look at the logs, but I keep getting error 500.

500? That's weird. Anyway, serial log is empty, so you haven't lost
much.
But also, I've ran it a couple more times and it is some regression.
Staging reliably passes while staging+this series fails.

Unfortunately I don't have any more info besides it hangs before
printing anything on serial or VGA. Maybe it hanging only on Intel but
not AMD is some hint? Or maybe it's some memory layout or firmware
differences that matter here (bootloader is exactly the same)?
FWIW some links:
successful staging run on ADL: https://gitlab.com/xen-project/people/marmarek/xen/-/jobs/7980146338
failed staging+this run on ADL: https://gitlab.com/xen-project/people/marmarek/xen/-/jobs/7980330394
successful staging run on Zen3+: https://gitlab.com/xen-project/people/marmarek/xen/-/jobs/7980146359
successful staging+this run on Zen3+: https://gitlab.com/xen-project/people/marmarek/xen/-/jobs/7980146359

> > It does pass on Zen 3+ runners.
> >
> > Since there were some issues with the ADL runner today on plain staging,
> > I'm not 100% sure if it isn't some infrastructure issue yet. But the
> > symptoms look different than usual infra issues (and different than
> > todays failures on staging), so I think it's more likely an issue with
> > the patches here.
> >
> > > 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.
> > >
> > > Changes since v3:
> > > - changed new function name;
> > > - declare efi_multiboot2 in a separate header;
> > > - distinguish entry point from using magic number;
> > > - other minor changes (see commens in commits).
> > >
> > > Changes since v4:
> > > - rebase on staging;
> > > - set %fs and %gs as other segment registers;
> > > - style and other changes.
> > >
> > > Changes since v5:
> > > - fixed a typo.
> > >
> > > Changes since v6:
> > > - remove merged patch;
> > > - comment and style;
> > > - change some pointer checks to avoid overflows;
> > > - rename parse-mbi2.c to mbi2.c.
> > >
> > > Frediano Ziglio (2):
> > >   x86/boot: Rewrite EFI/MBI2 code partly in C
> > >   x86/boot: Improve MBI2 structure check
> > >
> > >  xen/arch/x86/boot/head.S       | 146 +++++++--------------------------
> > >  xen/arch/x86/efi/Makefile      |   1 +
> > >  xen/arch/x86/efi/efi-boot.h    |   7 +-
> > >  xen/arch/x86/efi/mbi2.c        |  66 +++++++++++++++
> > >  xen/arch/x86/efi/stub.c        |  10 +--
> > >  xen/arch/x86/include/asm/efi.h |  18 ++++
> > >  6 files changed, 123 insertions(+), 125 deletions(-)
> > >  create mode 100644 xen/arch/x86/efi/mbi2.c
> > >  create mode 100644 xen/arch/x86/include/asm/efi.h
> > >
> 
> Frediano

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v7 0/2] x86/boot: Reduce assembly code

[Andrew Cooper]

On 03/10/2024 2:11 am, Marek Marczykowski-Górecki wrote:
> On Wed, Oct 02, 2024 at 04:27:19PM +0100, Frediano Ziglio wrote:
>> On Wed, Oct 2, 2024 at 3:04 PM Marek Marczykowski-Górecki
>> <marmarek@invisiblethingslab.com> wrote:
>>> On Tue, Oct 01, 2024 at 11:22:37AM +0100, Frediano Ziglio wrote:
>>>> This series came from part of the work of removing duplications between
>>>> boot code and rewriting part of code from assembly to C.
>>>> Rewrites EFI code in pure C.
>>> The MB2+EFI tests on Adler Lake fail with this series:
>>> https://gitlab.com/xen-project/people/marmarek/xen/-/pipelines/1478766782
>>> Looking at the VGA output (unfortunately not collected by the test
>>> itself) it hangs just after bootloader, before printing anything on the
>>> screen (or even clearing it after bootloader). The serial is silent too.
>>>
>> I tried multiple times to take a look at the logs, but I keep getting error 500.
> 500? That's weird. Anyway, serial log is empty, so you haven't lost
> much.
> But also, I've ran it a couple more times and it is some regression.
> Staging reliably passes while staging+this series fails.
>
> Unfortunately I don't have any more info besides it hangs before
> printing anything on serial or VGA. Maybe it hanging only on Intel but
> not AMD is some hint? Or maybe it's some memory layout or firmware
> differences that matter here (bootloader is exactly the same)?
> FWIW some links:
> successful staging run on ADL: https://gitlab.com/xen-project/people/marmarek/xen/-/jobs/7980146338
> failed staging+this run on ADL: https://gitlab.com/xen-project/people/marmarek/xen/-/jobs/7980330394
> successful staging run on Zen3+: https://gitlab.com/xen-project/people/marmarek/xen/-/jobs/7980146359
> successful staging+this run on Zen3+: https://gitlab.com/xen-project/people/marmarek/xen/-/jobs/7980146359

Ok, we should do several things.

Frediano, can you split out the efi_multiboot2() prototype in asm/efi.h
and submit it separately from introducing efi_multiboot2_prelude(). 
Mechanically, it's a somewhat-large part of the patch, and won't be
related to whatever other failure is going on here.

Everyone, is it time to consider earlyprintk() ?  This is not the first
time something like this has happened, and it wont be the last.  I for
one am quite bored of doing ad-hoc debugging each time...

~Andrew

Re: [PATCH v7 0/2] x86/boot: Reduce assembly code

[Frediano Ziglio]

On Thu, Oct 3, 2024 at 2:11 AM Marek Marczykowski-Górecki
<marmarek@invisiblethingslab.com> wrote:
>
> On Wed, Oct 02, 2024 at 04:27:19PM +0100, Frediano Ziglio wrote:
> > On Wed, Oct 2, 2024 at 3:04 PM Marek Marczykowski-Górecki
> > <marmarek@invisiblethingslab.com> wrote:
> > >
> > > On Tue, Oct 01, 2024 at 11:22:37AM +0100, Frediano Ziglio wrote:
> > > > This series came from part of the work of removing duplications between
> > > > boot code and rewriting part of code from assembly to C.
> > > > Rewrites EFI code in pure C.
> > >
> > > The MB2+EFI tests on Adler Lake fail with this series:
> > > https://gitlab.com/xen-project/people/marmarek/xen/-/pipelines/1478766782
> > > Looking at the VGA output (unfortunately not collected by the test
> > > itself) it hangs just after bootloader, before printing anything on the
> > > screen (or even clearing it after bootloader). The serial is silent too.
> > >
> >
> > I tried multiple times to take a look at the logs, but I keep getting error 500.
>
> 500? That's weird. Anyway, serial log is empty, so you haven't lost
> much.

I'm getting pretty consistently. I can see the full pipeline, but not
the single logs. I tried with both Firefox and Chrome, I also tried
from home (just to check for something like firewall issues), always
error 500.

> But also, I've ran it a couple more times and it is some regression.
> Staging reliably passes while staging+this series fails.
>
> Unfortunately I don't have any more info besides it hangs before
> printing anything on serial or VGA. Maybe it hanging only on Intel but
> not AMD is some hint? Or maybe it's some memory layout or firmware
> differences that matter here (bootloader is exactly the same)?
> FWIW some links:
> successful staging run on ADL: https://gitlab.com/xen-project/people/marmarek/xen/-/jobs/7980146338
> failed staging+this run on ADL: https://gitlab.com/xen-project/people/marmarek/xen/-/jobs/7980330394
> successful staging run on Zen3+: https://gitlab.com/xen-project/people/marmarek/xen/-/jobs/7980146359
> successful staging+this run on Zen3+: https://gitlab.com/xen-project/people/marmarek/xen/-/jobs/7980146359
>

Furthermore, I tried with 2 additional machines in our Lab, one Intel,
another AMD, both working for me.
Either your compiler did something different or something special on
the firmware.

I could try downloading your executables and machines there, but as I
said, I'm getting error 500 (not sure if we package artifacts).

Can you try without last commit ?

Frediano

> > > It does pass on Zen 3+ runners.
> > >
> > > Since there were some issues with the ADL runner today on plain staging,
> > > I'm not 100% sure if it isn't some infrastructure issue yet. But the
> > > symptoms look different than usual infra issues (and different than
> > > todays failures on staging), so I think it's more likely an issue with
> > > the patches here.
> > >
> > > > 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.
> > > >
> > > > Changes since v3:
> > > > - changed new function name;
> > > > - declare efi_multiboot2 in a separate header;
> > > > - distinguish entry point from using magic number;
> > > > - other minor changes (see commens in commits).
> > > >
> > > > Changes since v4:
> > > > - rebase on staging;
> > > > - set %fs and %gs as other segment registers;
> > > > - style and other changes.
> > > >
> > > > Changes since v5:
> > > > - fixed a typo.
> > > >
> > > > Changes since v6:
> > > > - remove merged patch;
> > > > - comment and style;
> > > > - change some pointer checks to avoid overflows;
> > > > - rename parse-mbi2.c to mbi2.c.
> > > >
> > > > Frediano Ziglio (2):
> > > >   x86/boot: Rewrite EFI/MBI2 code partly in C
> > > >   x86/boot: Improve MBI2 structure check
> > > >
> > > >  xen/arch/x86/boot/head.S       | 146 +++++++--------------------------
> > > >  xen/arch/x86/efi/Makefile      |   1 +
> > > >  xen/arch/x86/efi/efi-boot.h    |   7 +-
> > > >  xen/arch/x86/efi/mbi2.c        |  66 +++++++++++++++
> > > >  xen/arch/x86/efi/stub.c        |  10 +--
> > > >  xen/arch/x86/include/asm/efi.h |  18 ++++
> > > >  6 files changed, 123 insertions(+), 125 deletions(-)
> > > >  create mode 100644 xen/arch/x86/efi/mbi2.c
> > > >  create mode 100644 xen/arch/x86/include/asm/efi.h
> > > >

Re: [PATCH v7 0/2] x86/boot: Reduce assembly code

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 2914 bytes --]

On Thu, Oct 03, 2024 at 10:27:15AM +0100, Frediano Ziglio wrote:
> On Thu, Oct 3, 2024 at 2:11 AM Marek Marczykowski-Górecki
> <marmarek@invisiblethingslab.com> wrote:
> >
> > On Wed, Oct 02, 2024 at 04:27:19PM +0100, Frediano Ziglio wrote:
> > > On Wed, Oct 2, 2024 at 3:04 PM Marek Marczykowski-Górecki
> > > <marmarek@invisiblethingslab.com> wrote:
> > > >
> > > > On Tue, Oct 01, 2024 at 11:22:37AM +0100, Frediano Ziglio wrote:
> > > > > This series came from part of the work of removing duplications between
> > > > > boot code and rewriting part of code from assembly to C.
> > > > > Rewrites EFI code in pure C.
> > > >
> > > > The MB2+EFI tests on Adler Lake fail with this series:
> > > > https://gitlab.com/xen-project/people/marmarek/xen/-/pipelines/1478766782
> > > > Looking at the VGA output (unfortunately not collected by the test
> > > > itself) it hangs just after bootloader, before printing anything on the
> > > > screen (or even clearing it after bootloader). The serial is silent too.
> > > >
> > >
> > > I tried multiple times to take a look at the logs, but I keep getting error 500.
> >
> > 500? That's weird. Anyway, serial log is empty, so you haven't lost
> > much.
> 
> I'm getting pretty consistently. I can see the full pipeline, but not
> the single logs. I tried with both Firefox and Chrome, I also tried
> from home (just to check for something like firewall issues), always
> error 500.
> 
> > But also, I've ran it a couple more times and it is some regression.
> > Staging reliably passes while staging+this series fails.
> >
> > Unfortunately I don't have any more info besides it hangs before
> > printing anything on serial or VGA. Maybe it hanging only on Intel but
> > not AMD is some hint? Or maybe it's some memory layout or firmware
> > differences that matter here (bootloader is exactly the same)?
> > FWIW some links:
> > successful staging run on ADL: https://gitlab.com/xen-project/people/marmarek/xen/-/jobs/7980146338
> > failed staging+this run on ADL: https://gitlab.com/xen-project/people/marmarek/xen/-/jobs/7980330394
> > successful staging run on Zen3+: https://gitlab.com/xen-project/people/marmarek/xen/-/jobs/7980146359
> > successful staging+this run on Zen3+: https://gitlab.com/xen-project/people/marmarek/xen/-/jobs/7980146359
> >
> 
> Furthermore, I tried with 2 additional machines in our Lab, one Intel,
> another AMD, both working for me.
> Either your compiler did something different or something special on
> the firmware.
> 
> I could try downloading your executables and machines there, but as I
> said, I'm getting error 500 (not sure if we package artifacts).
> 
> Can you try without last commit ?

Yes, this seems to work:
https://gitlab.com/xen-project/people/marmarek/xen/-/pipelines/1480052345

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]