|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 3/3] x86/boot: Rewrite EFI start part in C
On Sun, Sep 15, 2024 at 8:00 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 10.09.2024 18:16, Frediano Ziglio wrote:
> > No need to have it coded in assembly.
>
> As to the title: It's the EFI/MB2 case you re-write. That wants reflecting
> there, as the "normal" EFI start part is all C already anyway. I also
> think you mean "partly"?
>
Updated to "x86/boot: Rewrite EFI/MBI2 code partly in C".
> > @@ -255,34 +246,29 @@ __efi64_mb2_start:
> > rep stosq
> > mov %edx, %eax
>
> This can be dropped then, by making ...
>
> > - /* 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
> > + /* Save Multiboot2 magic on the stack. */
> > + push %rax
>
> ... this use %rdx.
>
Done (also below)
> > -.Lefi_multiboot2_proto:
> > - /* Zero EFI SystemTable, EFI ImageHandle addresses and cmdline. */
> > - xor %esi,%esi
> > - xor %edi,%edi
> > - xor %edx,%edx
> > + /* Save Multiboot2 pointer on the stack. */
> > + push %rbx
>
> %rbx doesn't need preserving around a C function call (which will do
> so itself if necessary). I understand a 2nd PUSH may be necessary
> anyway, to keep the stack aligned, yet that then would need
> commenting differently. Plus as long as we call our own functions
> only, we don't require such alignment.
>
Extended comment.
16-byte alignment is also in SystemV ABI, I won't remove it in this series.
> > - /* Skip Multiboot2 information fixed part. */
> > - lea (MB2_fixed_sizeof+MULTIBOOT2_TAG_ALIGN-1)(%rbx),%ecx
> > - and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
> > + /*
> > + * efi_parse_mbi2() is called according to System V AMD64 ABI:
> > + * - IN: %edi - Multiboot2 magic, %rsi - Multiboot2 pointer.
> > + * - OUT: %eax - error string.
>
> Nit: %rax according to the code below.
>
Done
> > + */
> > + mov %eax, %edi
> > + mov %ebx, %esi
>
> This latter one would better be a 64-bit MOV, for it being a pointer?
>
Done
> > + call efi_parse_mbi2
> > + test %rax, %rax
> > + lea .Ldirect_error(%rip), %r15
> > + jnz x86_32_switch
>
> As requested by Andrew in a related context: LEA first please to follow
> the pattern allowing macro fusion, even if here it is less because of
> performance concerns but more to avoid giving a bad example.
>
Done
> > --- a/xen/arch/x86/efi/Makefile
> > +++ b/xen/arch/x86/efi/Makefile
> > @@ -13,6 +13,7 @@ $(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-y += parse-mbi2.o
>
> obj-bin-y I suppose, for it all being __init / __initdata, and hence the
> string literals in particular also wanting to move to .init.rodata.
>
Okay
> > --- /dev/null
> > +++ b/xen/arch/x86/efi/parse-mbi2.c
> > @@ -0,0 +1,54 @@
> > +/* 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/efibind.h>
> > +#include <efi/efidef.h>
> > +#include <efi/eficapsule.h>
> > +#include <efi/eficon.h>
> > +#include <efi/efidevp.h>
> > +#include <efi/efiapi.h>
>
> I don't think all of these are needed? Please limit #include-s to just
> what is actually used.
>
I had the same idea, but if you comment out any of them code stop compiling.
> > +void asmlinkage __init efi_multiboot2(EFI_HANDLE ImageHandle,
> > + EFI_SYSTEM_TABLE *SystemTable,
> > + const char *cmdline);
>
> This i now solely called from C code and hence shouldn't be asmlinkage.
>
Done
> > +const char *__init efi_parse_mbi2(uint32_t magic, const multiboot2_fixed_t
> > *mbi)
>
> Whereas this, called from assembly code and not having / needing a
> declaration, should be.
>
Done
> > +{
> > + 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 Multiboot bootloader!";
>
> Assembly code merely re-used a message. Now that it separate, please make
> it say "Multiboot2".
>
Done
> > + /* Skip Multiboot2 information fixed part. */
> > + for ( tag = _p(ROUNDUP((unsigned long)(mbi + 1),
> > MULTIBOOT2_TAG_ALIGN));
>
> The comment is placed as if it applied to the entire loop. It wants to move
> inside the for() to make clear it only applies to the loop setup.
>
Separated in a different line.
> > + (void *)tag - (void *)mbi < mbi->total_size && tag->type !=
> > MULTIBOOT2_TAG_TYPE_END;
> > + tag = _p(ROUNDUP((unsigned long)((void *)tag + tag->size),
> > MULTIBOOT2_TAG_ALIGN)) )
>
> Now that this is done in C, I think the checking wants to be more
> thorough, to no only make sure the start of a sub-struct is within
> the specified size, but all of it (se we won't even access past
> ->total_size).
>
I would first just translate the assembly code, then add improvements
in a separate commit.
> Further looks like there's a line length issue here.
>
Fixed
> Also please don't cast away const-ness from pointers.
>
Done
> > + {
> > + if ( tag->type == MULTIBOOT2_TAG_TYPE_EFI_BS )
> > + have_bs = true;
> > + else if ( tag->type == MULTIBOOT2_TAG_TYPE_EFI64 )
> > + SystemTable = _p(((const multiboot2_tag_efi64_t
> > *)tag)->pointer);
> > + else if ( tag->type == MULTIBOOT2_TAG_TYPE_EFI64_IH )
> > + ImageHandle = _p(((const multiboot2_tag_efi64_ih_t
> > *)tag)->pointer);
> > + else if ( tag->type == MULTIBOOT2_TAG_TYPE_CMDLINE )
> > + cmdline = ((const multiboot2_tag_string_t *)tag)->string;
> > + }
> > +
> > + 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);
>
> This being invoked from here now makes me wonder about the (new)
> function's name and whether a separate new function is actually
> needed: Can't the new code then be integrated right into
> efi_multiboot2(), thus eliminating the question on the naming of
> the function?
>
If you are suggesting putting this parsing code inside efi_multiboot2
in ef-boot.h that would change the behavior, which I would do in a
different commit.
Currently, there are 2 different efi_multiboot2 functions, one if
ms_abi is supported, the other an empty stubs. However, some checks
and tests are done in both cases (ms_abi supported or not). Also, both
paths uses SystemTable, so I need to parse MBI2 in any case.
> > --- 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";
>
> This, if not a separate change, wants mentioning in the description.
> It's a related observation that this wasn't properly updated, but
> nothing that necessarily needs doing here. Question is whether the
> declaration of the function wouldn't better go into a header now in
> the first place.
>
I had the same though about a header. But currently there's no such
header, I mean it should be able to be included by both stub.c and
efi-boot.h which are both x86 only code so it should go in
xen/arch/x86/ I suppose. Suggestions? Maybe a different solution would
be to have a xen/arch/x86/efi/efi-boot-stub.h instead of
xen/arch/x86/efi/stub.c ?
> Jan
Frediano
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |