[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On Sun, Sep 15, 2024 at 8:00 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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"?
>

Updated to "x86/boot: Rewrite EFI/MBI2 code partly in C".

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

Done (also below)

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

Extended comment.
16-byte alignment is also in SystemV ABI, I won't remove it in this series.


> > -        /* 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.
>

Done

> > +         */
> > +        mov     %eax, %edi
> > +        mov     %ebx, %esi
>
> This latter one would better be a 64-bit MOV, for it being a pointer?
>

Done

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

Done

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

Okay

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

I had the same idea, but if you comment out any of them code stop compiling.

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

Done

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

Done

> > +{
> > +    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".
>

Done

> > +    /* 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.
>

Separated in a different line.

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

I would first just translate the assembly code, then add improvements
in a separate commit.

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

Fixed

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

Done

> > +    {
> > +        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?
>

If you are suggesting putting this parsing code inside efi_multiboot2
in ef-boot.h that would change the behavior, which I would do in a
different commit.
Currently, there are 2 different efi_multiboot2 functions, one if
ms_abi is supported, the other an empty stubs. However, some checks
and tests are done in both cases (ms_abi supported or not). Also, both
paths uses SystemTable, so I need to parse MBI2 in any case.

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

I had the same though about a header. But currently there's no such
header, I mean it should be able to be included by both stub.c and
efi-boot.h which are both x86 only code so it should go in
xen/arch/x86/ I suppose. Suggestions? Maybe a different solution would
be to have a xen/arch/x86/efi/efi-boot-stub.h instead of
xen/arch/x86/efi/stub.c ?

> Jan

Frediano



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.