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
