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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

> +          tag = _p(ROUNDUP((unsigned long)((const void *)tag + tag->size),
> +                   MULTIBOOT2_TAG_ALIGN)) )

No need to cast through (const void *) I don't think.  It's
byte-granular when unsigned long too.  This should do:

    _p(ROUNDUP(((unsigned long)tag + tag->size), MULTIBOOT2_TAG_ALIGN))

> +    {
> +        switch ( tag->type )
> +        {
> +        case MULTIBOOT2_TAG_TYPE_EFI_BS:
> +            have_bs = true;
> +            break;

Newlines after break's please.

> +        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:
    /* This comment is here because MISRA thinks you can't read the code
without it. */
    break;

Probably not that wording, but it reflects what I think of this
particular rule.

I can fix all on commit, but this needs an EFI ack.

~Andrew