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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Wed, Sep 25, 2024 at 9:20 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> On 25/09/2024 7:01 am, Frediano Ziglio wrote:
> > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> > index 2d2f56ad22..859f7055dc 100644
> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -252,36 +243,30 @@ __efi64_mb2_start:
> > <snip>
> >
> >          /* We are on EFI platform and EFI boot services are available. */
> >          incb    efi_platform(%rip)
> > @@ -291,77 +276,6 @@ __efi64_mb2_start:
> >           * be run on EFI platforms.
> >           */
> >          incb    skip_realmode(%rip)
>
> Well, these are two unfounded assumptions about the compile-time
> defaults of certain variables.
>
> Lets fix it afterwards, to save interfering with this series.
>
> > diff --git a/xen/arch/x86/efi/parse-mbi2.c b/xen/arch/x86/efi/parse-mbi2.c
> > new file mode 100644
> > index 0000000000..89c562cf6a
> > --- /dev/null
> > +++ b/xen/arch/x86/efi/parse-mbi2.c
> > @@ -0,0 +1,56 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
>
> GPL-2.0-only.  The unsuffixed form is deprecated now.
>
> > +
> > +#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;
>
> && on previous line.
>
> But, this can move into the switch statement and reduce the for()
> expression somewhat.
>

I forgot to reply this, I even though about what to write.
There are multiple reasons:
- having now a switch, it would require a goto/label or an additional
variable to exit the loop, unless I change all other case to continue
and have 2 breaks, either cases not much improving in my opinion;
- that specific part of the for loop is checking for termination and
that condition is doing exactly this;
- there are multiple instances of this kind of loop, and I was
thinking of adding a macro to simplify and reuse that loop so that
form is more suitable to do it.

... omissis ...

Frediano