Re: [PATCH 3/3] x86/boot: Rewrite EFI start part in C

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.09.2024 18:16, Frediano Ziglio wrote:
> No need to have it coded in assembly.

As to the title: It's the EFI/MB2 case you re-write. That wants reflecting
there, as the "normal" EFI start part is all C already anyway. I also
think you mean "partly"?

> @@ -255,34 +246,29 @@ __efi64_mb2_start:
>          rep stosq
>          mov     %edx, %eax

This can be dropped then, by making ...

> -        /* 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    %rax

... this use %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. */
> +        push    %rbx

%rbx doesn't need preserving around a C function call (which will do
so itself if necessary). I understand a 2nd PUSH may be necessary
anyway, to keep the stack aligned, yet that then would need
commenting differently. Plus as long as we call our own functions
only, we don't require such alignment.

> -        /* 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: %eax - error string.

Nit: %rax according to the code below.

> +         */
> +        mov     %eax, %edi
> +        mov     %ebx, %esi

This latter one would better be a 64-bit MOV, for it being a pointer?

> +        call    efi_parse_mbi2
> +        test    %rax, %rax
> +        lea     .Ldirect_error(%rip), %r15
> +        jnz     x86_32_switch

As requested by Andrew in a related context: LEA first please to follow
the pattern allowing macro fusion, even if here it is less because of
performance concerns but more to avoid giving a bad example.

> --- a/xen/arch/x86/efi/Makefile
> +++ b/xen/arch/x86/efi/Makefile
> @@ -13,6 +13,7 @@ $(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-y += parse-mbi2.o

obj-bin-y I suppose, for it all being __init / __initdata, and hence the
string literals in particular also wanting to move to .init.rodata.

> --- /dev/null
> +++ b/xen/arch/x86/efi/parse-mbi2.c
> @@ -0,0 +1,54 @@
> +/* 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>

I don't think all of these are needed? Please limit #include-s to just
what is actually used.

> +void asmlinkage __init efi_multiboot2(EFI_HANDLE ImageHandle,
> +                                      EFI_SYSTEM_TABLE *SystemTable,
> +                                      const char *cmdline);

This i now solely called from C code and hence shouldn't be asmlinkage.

> +const char *__init efi_parse_mbi2(uint32_t magic, const multiboot2_fixed_t 
> *mbi)

Whereas this, called from assembly code and not having / needing a
declaration, should be.

> +{
> +    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 Multiboot bootloader!";

Assembly code merely re-used a message. Now that it separate, please make
it say "Multiboot2".

> +    /* Skip Multiboot2 information fixed part. */
> +    for ( tag = _p(ROUNDUP((unsigned long)(mbi + 1), MULTIBOOT2_TAG_ALIGN));

The comment is placed as if it applied to the entire loop. It wants to move
inside the for() to make clear it only applies to the loop setup.

> +          (void *)tag - (void *)mbi < mbi->total_size && tag->type != 
> MULTIBOOT2_TAG_TYPE_END;
> +          tag = _p(ROUNDUP((unsigned long)((void *)tag + tag->size), 
> MULTIBOOT2_TAG_ALIGN)) )

Now that this is done in C, I think the checking wants to be more
thorough, to no only make sure the start of a sub-struct is within
the specified size, but all of it (se we won't even access past
->total_size).

Further looks like there's a line length issue here.

Also please don't cast away const-ness from pointers.

> +    {
> +        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);

This being invoked from here now makes me wonder about the (new)
function's name and whether a separate new function is actually
needed: Can't the new code then be integrated right into
efi_multiboot2(), thus eliminating the question on the naming of
the function?

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

This, if not a separate change, wants mentioning in the description.
It's a related observation that this wasn't properly updated, but
nothing that necessarily needs doing here. Question is whether the
declaration of the function wouldn't better go into a header now in
the first place.

Jan