Re: [PATCH v4 15/26] efi/x86: Support extracting kernel from libstub

[Evgeniy Baskov]

On 2023-03-09 19:00, Ard Biesheuvel wrote:
> On Thu, 15 Dec 2022 at 13:40, Evgeniy Baskov <baskov@xxxxxxxxx> wrote:
> > Doing it that way allows setting up stricter memory attributes,
> > simplifies boot code path and removes potential relocation
> > of kernel image.
> >
> > Wire up required interfaces and minimally initialize zero page
> > fields needed for it to function correctly.
> >
> > Tested-by: Peter Jones <pjones@xxxxxxxxxx>
> > Signed-off-by: Evgeniy Baskov <baskov@xxxxxxxxx>
> > ---
> >  arch/x86/boot/compressed/head_32.S            |  50 ++++-
> >  arch/x86/boot/compressed/head_64.S            |  58 ++++-
> >  drivers/firmware/efi/Kconfig                  |   2 +
> >  drivers/firmware/efi/libstub/Makefile         |   2 +-
> >  .../firmware/efi/libstub/x86-extract-direct.c | 208 
> > ++++++++++++++++++
> >  drivers/firmware/efi/libstub/x86-stub.c       | 119 +---------
> >  drivers/firmware/efi/libstub/x86-stub.h       |  14 ++
> >  7 files changed, 338 insertions(+), 115 deletions(-)
> >  create mode 100644 drivers/firmware/efi/libstub/x86-extract-direct.c
> >  create mode 100644 drivers/firmware/efi/libstub/x86-stub.h
> >
> > diff --git a/arch/x86/boot/compressed/head_32.S 
> > b/arch/x86/boot/compressed/head_32.S
> > index ead6007df1e5..0be75e5072ae 100644
> > --- a/arch/x86/boot/compressed/head_32.S
> > +++ b/arch/x86/boot/compressed/head_32.S
> > @@ -152,11 +152,57 @@ SYM_FUNC_END(startup_32)
> >
> >  #ifdef CONFIG_EFI_STUB
> >  SYM_FUNC_START(efi32_stub_entry)
> > +/*
> > + * Calculate the delta between where we were compiled to run
> > + * at and where we were actually loaded at.  This can only be done
> > + * with a short local call on x86.  Nothing  else will tell us what
> > + * address we are running at.  The reserved chunk of the real-mode
> > + * data at 0x1e4 (defined as a scratch field) are used as the stack
> > + * for this calculation. Only 4 bytes are needed.
> > + */
>
> Please drop this comment

Will do.

> > +       call    1f
> > +1:     popl    %ebx
> > +       addl    $_GLOBAL_OFFSET_TABLE_+(.-1b), %ebx
>
> Please drop this and ...
>
> > +
> > +       /* Clear BSS */
> > +       xorl    %eax, %eax
> > +       leal    _bss@GOTOFF(%ebx), %edi
> > +       leal    _ebss@GOTOFF(%ebx), %ecx
>
> just use (_bss - 1b) here (etc)

I was trying to be consistent with the code below, but it will
indeed be better to do this like that. I guess, this will be
fine to stop putting GOT address to the %ebx, since the extraction
code does not use calls via PLT?

> > +       subl    %edi, %ecx
> > +       shrl    $2, %ecx
> > +       rep     stosl
> > +
> >         add     $0x4, %esp
> >         movl    8(%esp), %esi   /* save boot_params pointer */
> > +       movl    %edx, %edi      /* save GOT address */
>
> What does this do?

Hmm... It seems to be a remnant of the previous implementation
that I forgot to remove. I will remove that in the v5.

> >         call    efi_main
> > -       /* efi_main returns the possibly relocated address of 
> > startup_32 */
> > -       jmp     *%eax
> > +       movl    %eax, %ecx
> > +
> > +       /*
> > +        * efi_main returns the possibly
> > +        * relocated address of extracted kernel entry point.
> > +        */
> > +
> > +       cli
> > +
> > +       /* Load new GDT */
> > +       leal    gdt@GOTOFF(%ebx), %eax
> > +       movl    %eax, 2(%eax)
> > +       lgdt    (%eax)
> > +
> > +       /* Load segment registers with our descriptors */
> > +       movl    $__BOOT_DS, %eax
> > +       movl    %eax, %ds
> > +       movl    %eax, %es
> > +       movl    %eax, %fs
> > +       movl    %eax, %gs
> > +       movl    %eax, %ss
> > +
> > +       /* Zero EFLAGS */
> > +       pushl   $0
> > +       popfl
> > +
> > +       jmp     *%ecx
> >  SYM_FUNC_END(efi32_stub_entry)
> >  SYM_FUNC_ALIAS(efi_stub_entry, efi32_stub_entry)
> >  #endif
> ...

Thanks,
Evgeniy Baskov