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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Mon, Sep 30, 2024 at 4:51 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 26.09.2024 11:21, Frediano Ziglio wrote:
> > @@ -243,7 +234,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_multiboot2_prelude() touches it.
> >           */
> >          mov     %eax, %edx
>
> I think this MOV wants to gain a comment, now that ...
>
> >          lea     __bss_start(%rip), %edi
> > @@ -252,36 +243,30 @@ __efi64_mb2_start:
> >          shr     $3, %ecx
> >          xor     %eax, %eax
> >          rep stosq
> > -        mov     %edx, %eax
>
> ... this one's going away.
>

What about

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 89b5e2af88..6fa6ea93e5 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -231,12 +231,14 @@ __efi64_mb2_start:
         */
        and     $~15, %rsp

+       /* Move away 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_prelude() touches it.
         */
-        mov     %eax, %edx
        lea     __bss_start(%rip), %edi
        lea     __bss_end(%rip), %ecx
        sub     %edi, %ecx

??

> > --- 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)
>
> While with ...
>
> > --- /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);
>
> ... the declaration now (supposedly) in scope the need for that earlier
> adjustment may be a little more obvious, you still ought to mention it
> in the description. If you did, you'd might have noticed that stub.c now
> also needs to include the new asm/efi.h, for the declaration to actually
> have its full intended effect.
>
> Jan

Done, both stub and normal code.

Frediano