On Fri, 31 Jan 2020 at 09:42, Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> wrote:
>
> Thanks Arvind, this is good stuff.
>
>
> On Thu, 30 Jan 2020 at 21:04, Arvind Sankar <nivedita@xxxxxxxxxxxx> wrote:
> >
> > For the 64-bit kernel, we do not need to setup a GDT in efi_main, as the
> > next step in the boot, startup_64, will set one up.
> >
>
> This seems obvious, but I need a nod from Ingo or Boris before I can take this.
>
> > This series factors out the GDT setup into a separate function and
> > restricts it to the 32-bit kernel. The memory allocation for the GDT is
> > also changed from allocating a full page via efi_alloc_low to the
> > required space (32 bytes) from alloc_pool boot service.
> >
>
> Can we go all the way and simply make this
>
> if (IS_ENABLED(CONFIG_X64_32)) {
> static const struct desc_struct desc[] __aligned(8) = {
> {}, /* The first GDT is a dummy. */
> {}, /* Second entry is unused on 32-bit */
> GDT_ENTRY_INIT(0xa09b, 0, 0xfffff), /* __KERNEL_CS */
> GDT_ENTRY_INIT(0xc093, 0, 0xfffff), /* __KERNEL_DS */
> };
> struct desc_ptr gdt = { sizeof(desc) - 1, (unsigned long)desc };
>
> native_load_gdt(&gdt);
> }
>
> (and drop the #defines from eboot.h that we no longer use)
>
Playing around with this a bit more, I think we should go with
if (IS_ENABLED(CONFIG_X86_32)) {
static const struct desc_struct desc[] __aligned(8) = {
[GDT_ENTRY_BOOT_CS] = GDT_ENTRY_INIT(0xc09b, 0, 0xfffff),
[GDT_ENTRY_BOOT_DS] = GDT_ENTRY_INIT(0xc093, 0, 0xfffff),
};
native_load_gdt(&(struct desc_ptr){ sizeof(desc) - 1,
(unsigned long)desc });
}
> > The final two patches are doc fixes to clarify that the 32-bit EFI
> > handover entry point also requires segments/GDT to be setup, and that
> > for 64-bit mode we don't have any special segment requirements (the
> > 64-bit doc currently is just a copy of the 32-bit doc which isn't
> > right).
> >
> > Arvind Sankar (8):
> > efi/x86: Use C wrapper instead of inline assembly
> > efi/x86: Allocate the GDT pointer on the stack
> > efi/x86: Factor GDT setup code into a function
>
> Given the above, I don't think we need the separate function, but if
> we do, please drop the 64-bit code first, otherwise there is much more
> churn than necessary.
>
> > efi/x86: Only setup the GDT for 32-bit kernel
> > efi/x86: Allocate only the required 32 bytes for the GDT
> > efi/x86: Change __KERNEL_{CS,DS} to __BOOT_{CS,DS}
> > Documentation/x86/boot: Clarify segment requirements for EFI handover
> > Documentation/x86/boot: Correct segment requirements for 64-bit boot
> >
> > Documentation/x86/boot.rst | 15 +-
> > arch/x86/boot/compressed/eboot.c | 174 ++++++++++--------------
> > arch/x86/boot/compressed/efi_thunk_64.S | 4 +-
> > 3 files changed, 85 insertions(+), 108 deletions(-)
> >
> > --
> > 2.24.1
> >
