No need to have it coded in assembly.
Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
Changes since v1:
- update some comments;
- explain why %ebx is saved before calling efi_parse_mbi2;
- move lea before test instruction;
- removed asmlinkage from efi_multiboot2 and add to efi_parse_mbi2;
- fix line length;
- update an error message specifying "Multiboot2" instead of "Multiboot";
- use obj-bin-X instead of obj-X in Makefile;
- avoid restoring %eax (MBI magic).
Changes since v3:
- rename new function to efi_multiboot2_prelude;
- declare efi_multiboot2 in a separate header.
---
xen/arch/x86/boot/head.S | 142 +++++++--------------------------
xen/arch/x86/efi/Makefile | 1 +
xen/arch/x86/efi/efi-boot.h | 6 +-
xen/arch/x86/efi/parse-mbi2.c | 56 +++++++++++++
xen/arch/x86/efi/stub.c | 3 +-
xen/arch/x86/include/asm/efi.h | 18 +++++
6 files changed, 108 insertions(+), 118 deletions(-)
create mode 100644 xen/arch/x86/efi/parse-mbi2.c
create mode 100644 xen/arch/x86/include/asm/efi.h
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
@@ -121,8 +121,6 @@ multiboot2_header:
.Lbad_cpu_msg: .asciz "ERR: Not a 64-bit CPU!"
.Lbad_ldr_msg: .asciz "ERR: Not a Multiboot bootloader!"
.Lbad_ldr_nbs: .asciz "ERR: Bootloader shutdown EFI x64 boot services!"
-.Lbad_ldr_nst: .asciz "ERR: EFI SystemTable is not provided by bootloader!"
-.Lbad_ldr_nih: .asciz "ERR: EFI ImageHandle is not provided by bootloader!"
.Lbad_efi_msg: .asciz "ERR: EFI IA-32 platforms are not supported!"
.Lbag_alg_msg: .asciz "ERR: Xen must be loaded at a 2Mb boundary!"
.Lno_nx_msg: .asciz "ERR: Not an NX-capable CPU!"
@@ -161,17 +159,6 @@ early_error: /* Here to improve the disassembly. */
mov $sym_offs(.Lno_nx_msg), %ecx
jmp .Lget_vtb
#endif
-.Lmb2_no_st:
- /*
- * Here we are on EFI platform. vga_text_buffer was zapped earlier
- * because there is pretty good chance that VGA is unavailable.
- */
- mov $sym_offs(.Lbad_ldr_nst), %ecx
- jmp .Lget_vtb
-.Lmb2_no_ih:
- /* Ditto. */
- mov $sym_offs(.Lbad_ldr_nih), %ecx
- jmp .Lget_vtb
.Lmb2_no_bs:
/*
* Ditto. Additionally, here there is a chance that Xen was started
@@ -189,6 +176,10 @@ early_error: /* Here to improve the disassembly. */
mov $sym_offs(.Lbad_efi_msg), %ecx
xor %edi,%edi # No VGA text buffer
jmp .Lprint_err
+.Ldirect_error:
+ mov sym_esi(vga_text_buffer), %edi
+ mov %eax, %esi
+ jmp 1f
.Lget_vtb:
mov sym_esi(vga_text_buffer), %edi
.Lprint_err:
@@ -235,7 +226,7 @@ __efi64_mb2_start:
/*
* Align the stack as UEFI spec requires. Keep it aligned
- * before efi_multiboot2() call by pushing/popping even
+ * before efi_multiboot2_prelude() call by pushing/popping even
* numbers of items on it.
*/
and $~15, %rsp
@@ -243,7 +234,7 @@ __efi64_mb2_start:
/*
* Initialize BSS (no nasty surprises!).
* It must be done earlier than in BIOS case
- * because efi_multiboot2() touches it.
+ * because efi_multiboot2_prelude() touches it.
*/
mov %eax, %edx
lea __bss_start(%rip), %edi
@@ -252,36 +243,30 @@ __efi64_mb2_start:
shr $3, %ecx
xor %eax, %eax
rep stosq
- mov %edx, %eax
-
- /* 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
-
-.Lefi_multiboot2_proto:
- /* Zero EFI SystemTable, EFI ImageHandle addresses and cmdline. */
- xor %esi,%esi
- xor %edi,%edi
- xor %edx,%edx
- /* Skip Multiboot2 information fixed part. */
- lea (MB2_fixed_sizeof+MULTIBOOT2_TAG_ALIGN-1)(%rbx),%ecx
- and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
-
-.Lefi_mb2_tsize:
- /* Check Multiboot2 information total size. */
- mov %ecx,%r8d
- sub %ebx,%r8d
- cmp %r8d,MB2_fixed_total_size(%rbx)
- jbe .Lrun_bs
+ /*
+ * Spill MB2 magic.
+ * Spill the pointer too, to keep the stack aligned.
+ */
+ push %rdx
+ push %rbx
- /* Are EFI boot services available? */
- cmpl $MULTIBOOT2_TAG_TYPE_EFI_BS,MB2_tag_type(%rcx)
- jne .Lefi_mb2_st
+ /*
+ * efi_multiboot2_prelude() is called according to System V AMD64 ABI:
+ * - IN: %edi - Multiboot2 magic,
+ * %rsi - Multiboot2 pointer.
+ * - OUT: %rax - error string.
+ */
+ mov %edx, %edi
+ mov %rbx, %rsi
+ call efi_multiboot2_prelude
+ lea .Ldirect_error(%rip), %r15
+ test %rax, %rax
+ jnz x86_32_switch
+
+ /* Restore Multiboot2 pointer and magic. */
+ pop %rbx
+ pop %rax
/* 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)
- jmp .Lefi_mb2_next_tag
-
-.Lefi_mb2_st:
- /* Get EFI SystemTable address from Multiboot2 information. */
- cmpl $MULTIBOOT2_TAG_TYPE_EFI64,MB2_tag_type(%rcx)
- cmove MB2_efi64_st(%rcx),%rsi
- je .Lefi_mb2_next_tag
-
- /* Get EFI ImageHandle address from Multiboot2 information. */
- cmpl $MULTIBOOT2_TAG_TYPE_EFI64_IH,MB2_tag_type(%rcx)
- cmove MB2_efi64_ih(%rcx),%rdi
- je .Lefi_mb2_next_tag
-
- /* Get command line from Multiboot2 information. */
- cmpl $MULTIBOOT2_TAG_TYPE_CMDLINE, MB2_tag_type(%rcx)
- jne .Lno_cmdline
- lea MB2_tag_string(%rcx), %rdx
- jmp .Lefi_mb2_next_tag
-.Lno_cmdline:
-
- /* Is it the end of Multiboot2 information? */
- cmpl $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx)
- je .Lrun_bs
-
-.Lefi_mb2_next_tag:
- /* Go to next Multiboot2 information tag. */
- add MB2_tag_size(%rcx),%ecx
- add $(MULTIBOOT2_TAG_ALIGN-1),%ecx
- and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
- jmp .Lefi_mb2_tsize
-
-.Lrun_bs:
- /* Are EFI boot services available? */
- cmpb $0,efi_platform(%rip)
-
- /* Jump to .Lmb2_no_bs after switching CPU to x86_32 mode. */
- lea .Lmb2_no_bs(%rip),%r15
- jz x86_32_switch
-
- /* Is EFI SystemTable address provided by boot loader? */
- test %rsi,%rsi
-
- /* Jump to .Lmb2_no_st after switching CPU to x86_32 mode. */
- lea .Lmb2_no_st(%rip),%r15
- jz x86_32_switch
-
- /* Is EFI ImageHandle address provided by boot loader? */
- test %rdi,%rdi
-
- /* Jump to .Lmb2_no_ih after switching CPU to x86_32 mode. */
- lea .Lmb2_no_ih(%rip),%r15
- jz x86_32_switch
-
- /* Save Multiboot2 magic on the stack. */
- push %rax
-
- /* Save EFI ImageHandle on the stack. */
- push %rdi
-
- /*
- * efi_multiboot2() is called according to System V AMD64 ABI:
- * - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable,
- * %rdx - MB2 cmdline
- */
- call efi_multiboot2
-
- /* Just pop an item from the stack. */
- pop %rax
-
- /* Restore Multiboot2 magic. */
- pop %rax
/* Jump to trampoline_setup after switching CPU to x86_32 mode. */
lea trampoline_setup(%rip),%r15
diff --git a/xen/arch/x86/efi/Makefile b/xen/arch/x86/efi/Makefile
index 24dfecfad1..51140061fc 100644
--- a/xen/arch/x86/efi/Makefile
+++ b/xen/arch/x86/efi/Makefile
@@ -14,5 +14,6 @@ $(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-bin-$(XEN_BUILD_EFI) := $(filter %.init.o,$(EFIOBJ-y))
+obj-bin-y += parse-mbi2.o
extra-$(XEN_BUILD_EFI) += buildid.o relocs-dummy.o
nocov-$(XEN_BUILD_EFI) += stub.o
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 7aa55e7aaf..859c01c13f 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -816,9 +816,9 @@ static const char *__init get_option(const char *cmd, const char *opt)
return o;
}
-void asmlinkage __init efi_multiboot2(EFI_HANDLE ImageHandle,
- EFI_SYSTEM_TABLE *SystemTable,
- const char *cmdline)
+void __init efi_multiboot2(EFI_HANDLE ImageHandle,
+ EFI_SYSTEM_TABLE *SystemTable,
+ const char *cmdline)
{
EFI_GRAPHICS_OUTPUT_PROTOCOL *gop;
EFI_HANDLE gop_handle;
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 */
+
+#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;
+ tag = _p(ROUNDUP((unsigned long)((const void *)tag + tag->size),
+ MULTIBOOT2_TAG_ALIGN)) )
+ {
+ switch ( tag->type )
+ {
+ case MULTIBOOT2_TAG_TYPE_EFI_BS:
+ have_bs = true;
+ break;
+ 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;
+ }
+ }
+
+ 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);
+
+ return NULL;
+}
diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.c
index 2cd5c8d4dc..27d40964d5 100644
--- 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";
diff --git a/xen/arch/x86/include/asm/efi.h b/xen/arch/x86/include/asm/efi.h
new file mode 100644
index 0000000000..575a33e302
--- /dev/null
+++ b/xen/arch/x86/include/asm/efi.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef X86_ASM_EFI_H
+#define X86_ASM_EFI_H
+
+#include <xen/types.h>
+#include <asm/x86_64/efibind.h>
+#include <efi/efidef.h>
+#include <efi/eficapsule.h>
+#include <efi/eficon.h>
+#include <efi/efidevp.h>
+#include <efi/efiapi.h>
+
+void efi_multiboot2(EFI_HANDLE ImageHandle,
+ EFI_SYSTEM_TABLE *SystemTable,
+ const char *cmdline);
+
+#endif /* X86_ASM_EFI_H */
--
2.34.1On 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
On Wed, Sep 25, 2024 at 9:20 PM Andrew Cooper <andrew.cooper3@citrix.com> 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
On 26/09/2024 10:14 am, Frediano Ziglio wrote:
> On Wed, Sep 25, 2024 at 9:20 PM Andrew Cooper <andrew.cooper3@citrix.com> 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
On 25.09.2024 22:20, Andrew Cooper wrote:
> On 25/09/2024 7:01 am, Frediano Ziglio wrote:
>> +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.
While it can in principle, it would require further adjustments to the
loop body, which I'm uncertain would be desirable. I notice Frediano
also didn't make any change in v5 for this particular comment. Frediano:
As indicated before, sending a new version without addressing all
comments isn't nice. If you don't agree with a comment and hence don't
make a change requested, you will want to "address" the comment verbally.
Jan
On Thu, Sep 26, 2024 at 7:58 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 25.09.2024 22:20, Andrew Cooper wrote:
> > On 25/09/2024 7:01 am, Frediano Ziglio wrote:
> >> +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.
>
> While it can in principle, it would require further adjustments to the
> loop body, which I'm uncertain would be desirable. I notice Frediano
> also didn't make any change in v5 for this particular comment. Frediano:
> As indicated before, sending a new version without addressing all
> comments isn't nice. If you don't agree with a comment and hence don't
> make a change requested, you will want to "address" the comment verbally.
>
> Jan
I was going to reply, then I forgot. Yes, I'm trying to reply and
address all comments. In this specific instance, for some reason while
I was addressing the others, I forgot this.
Frediano
© 2016 - 2026 Red Hat, Inc.