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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 26/09/2024 10:14 am, Frediano Ziglio wrote:
> 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,

Oh, of course.  Sorry, my mistake.  Leave it as is.

~Andrew