Re: [PATCH v12 05/10] x86: add multiboot2 protocol support for EFI platforms

["Jan Beulich" <JBeulich@suse.com>]

>>> On 20.01.17 at 12:43, <daniel.kiper@oracle.com> wrote:
> On Fri, Jan 20, 2017 at 02:46:47AM -0700, Jan Beulich wrote:
>> >>> On 20.01.17 at 02:34, <daniel.kiper@oracle.com> wrote:
>> > @@ -100,20 +107,48 @@ multiboot2_header_start:
>> >  gdt_boot_descr:
>> >          .word   6*8-1
>> >          .long   sym_phys(trampoline_gdt)
>> > +        .long   0 /* Needed for 64-bit lgdt */
>> > +
>> > +        .align 4
>> > +vga_text_buffer:
>> > +        .long   0xb8000
>> > +
>> > +efi_platform:
>> > +        .byte   0
>>
>> You mean to modify these fields, but you add them to a r/o section.
> 
> Yep. So, I think that we should move them to .init.data. Is it OK for you?

That's what I'm asking for, yes.

>> > +.Lefi_multiboot2_proto:
>> > +        /* Zero EFI SystemTable and EFI ImageHandle addresses. */
>> > +        xor     %esi,%esi
>> > +        xor     %edi,%edi
>> > +
>> > +        /* Skip Multiboot2 information fixed part. */
>> > +        lea     (MB2_fixed_sizeof+MULTIBOOT2_TAG_ALIGN-1)(%rbx),%ecx
>>
>> In an earlier reply to Andrew's inquiry regarding the possible
>> truncation here you said that grub can be made obey to such a
>> load restriction. None of the tags added here or in patch 2
>> appear to have such an effect, so would you please clarify how
>> the address restriction is being enforced?
> 
> GRUB2 itself does allocations for all multiboot2 stuff always below 4 GiB.
> So, there is no need for extra tags.
> 
> Additionally, multiboot2 spec says this:
> 
> The bootloader must not load any part of the kernel, the modules, the Multiboot2
> information structure, etc. higher than 4 GiB - 1. This requirement is put in
> force because most of currently specified tags supports 32-bit addresses only.
> Additionally, some kernels, even if they run on EFI 64-bit platform, still
> execute some parts of its initialization code in 32-bit mode.

Okay, that's good enough for now, even if it escapes me how that's
supposed to work on systems without any memory below 4Gb.

>> > +        /* Are EFI boot services available? */
>> > +        cmpb    $0,efi_platform(%rip)
>> > +        jnz     0f
>> > +
>> > +        /* Jump to .Lmb2_no_bs after switching CPU to x86_32 mode. */
>> > +        lea     .Lmb2_no_bs(%rip),%edi
>> > +        jmp     x86_32_switch
>> > +0:
>>
>> I realize you need to avoid clobbering %rdi in the non-error case,
>> but the register choice seems sub-optimal: If you used a register
>> which you can clobber here (e.g. %edx as it seems, using %edi in
>> its place at x86_32_switch and cs32_switch), you could simplify
>> this to
>>
>>     cmpb ...
>>     lea ...
>>     je x86_32_switch
>>
>> at once avoiding all the numeric labels here.
> 
> If you do not care that it will be always loaded then OK.

That's okay, of course. The main thing is that we should prefer the
one branch variant over the two branches one.

> However, I think
> that %r15d is a bit better here because if we need to add another argument
> to efi_multiboot2() and we use %edx then we must change code in more places.
> Of course we should do "mov %r15d,%edi" after x86_32_switch label then.

As long as all affected code lives outside of the trampoline, using
any of the high 8 registers is certainly fine here.

>> > +2:
>> > +        push    %rax
>> > +        push    %rdi
>> > +
>> > +        /*
>> > +         * Initialize BSS (no nasty surprises!).
>> > +         * It must be done earlier than in BIOS case
>> > +         * because efi_multiboot2() touches it.
>> > +         */
>> > +        lea     __bss_start(%rip),%edi
>> > +        lea     __bss_end(%rip),%ecx
>> > +        sub     %edi,%ecx
>> > +        shr     $3,%ecx
>> > +        xor     %eax,%eax
>> > +        rep stosq
>> > +
>> > +        pop     %rdi
>> > +
>> > +        /*
>> > +         * efi_multiboot2() is called according to System V AMD64 ABI:
>> > +         *   - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable,
>> > +         *   - OUT: %rax - highest allocated memory address below 1 MiB;
>> > +         *                 memory below this address is used for trampoline
>> > +         *                 stack, trampoline itself and as a storage for
>> > +         *                 mbi struct created in reloc().
>> > +         *
>> > +         * MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO tag is not provided
>> > +         * on EFI platforms. Hence, it could not be used like
>> > +         * on legacy BIOS platforms.
>> > +         */
>> > +        call    efi_multiboot2
>>
>> Now that we had a need for commit f6b7fedc89 ("x86/EFI: meet
>> further spec requirements for runtime calls") I'm worried about stack
>> alignment here. Does GrUB call or jmp to our entry point (and is that
>> firmly specified to be the case)? Perhaps it would be a good idea to
>> align the stack earlier on in any case. Or if not (and if alignment at
>> this call is ABI conforming), a comment should be left here to warn
>> people of future modifications to the amount of items pushed onto /
>> popped off the stack.
> 
> Multiboot2 spec requires that stack, among others, is passed to loaded
> image according to UEFI spec. Though, IIRC, there are no extra stack checks
> there. Maybe we should add something to GRUB2 if it does not exists.

That would imply they do a call (and not a jmp), which would make
the present code correct afaict. As said, imo there should still be a
warning added here, for anyone wanting to modify the stack layout.

>> > --- a/xen/arch/x86/efi/efi-boot.h
>> > +++ b/xen/arch/x86/efi/efi-boot.h
>> > @@ -100,6 +100,9 @@ static void __init relocate_trampoline(unsigned long phys)
>> >  {
>> >      const s32 *trampoline_ptr;
>> >
>> > +    if ( !efi_enabled(EFI_LOADER) || trampoline_phys )
>> > +        return;
>> > +
>> >      trampoline_phys = phys;
>> >      /* Apply relocations to trampoline. */
>> >      for ( trampoline_ptr = __trampoline_rel_start;
>> > @@ -210,12 +213,10 @@ static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size)
>> >
>> >  static void __init efi_arch_pre_exit_boot(void)
>> >  {
>> > -    if ( !trampoline_phys )
>> > -    {
>> > -        if ( !cfg.addr )
>> > -            blexit(L"No memory for trampoline");
>> > -        relocate_trampoline(cfg.addr);
>> > -    }
>> > +    if ( !cfg.addr )
>> > +        blexit(L"No memory for trampoline");
>> > +
>> > +    relocate_trampoline(cfg.addr);
>> >  }
>>
>> Why can't this function be left untouched, and the change to
>> relocate_trampoline() above also be reduced or even be removed
>> altogether?
> 
> There is pretty good chance that efi_arch_pre_exit_boot() can be left
> untouched though relocate_trampoline() needs at least
> 
> if ( !efi_enabled(EFI_LOADER) )
>     return;

Right, hence the "reduced".

>> > +    SIMPLE_TEXT_OUTPUT_INTERFACE *StdErr;
>> > +
>> > +    StdErr = SystemTable->StdErr ? SystemTable->StdErr : SystemTable->ConOut;
>> > +
>> > +    /*
>> > +     * Print error message and halt the system.
>> > +     *
>> > +     * We have to open code MS x64 calling convention
>> > +     * in assembly because here this convention may
>> > +     * not be directly supported by C compiler.
>> > +     */
>> > +    asm volatile(
>> > +    "    call *%2                     \n"
>> > +    "0:  hlt                          \n"
>> > +    "    jmp  0b                      \n"
>> > +       : "+c" (StdErr), "+d" (err) : "g" (StdErr->OutputString)
>>
>> The "g" really wants to be "rm": While the kind of expression doesn't
>> really allow for an immediate, you still shouldn't give wrong examples
>> of constraints (with the * now properly added to the call operand, it
>> doesn't allow for an immediate anymore).
> 
> I was considering that change once but I was not sure. Though if you
> think that it make sense I will do that.

Yes please.

>> > --- a/xen/include/asm-x86/config.h
>> > +++ b/xen/include/asm-x86/config.h
>> > @@ -73,6 +73,11 @@
>> >  #define STACK_ORDER 3
>> >  #define STACK_SIZE  (PAGE_SIZE << STACK_ORDER)
>> >
>> > +#define MB_TRAMPOLINE_STACK_SIZE        PAGE_SIZE
>> > +#define MB_TRAMPOLINE_SIZE              (KB(64) - MB_TRAMPOLINE_STACK_SIZE)
>>
>> What's the reason for the MB_ prefixes here? The trampoline is
>> always the same size, isn't it? Nor am I convinced we really need
> 
> Please take a look at efi_arch_memory_setup(). Amount of memory allocated
> for trampoline and other stuff depends on boot loader type. So, when I use
> "MB_" I would like underline that this is relevant for multiboot protocols.
> Though I think that we can use the same size for all protocols. However,
> I would not like to touch native EFI loader case.

You already don't touch it, and I see no reason why this should
change. Yet this is orthogonal to the naming of the constants here.
As said, the trampoline itself is always the same, and the stack
portion of it is simply unused in the native EFI loader case. Plus
MB_TRAMPOLINE_SIZE is in no way the size of the trampoline in the
first place. Perhaps TRAMPOLINE_SPACE (and then covering both
parts)?

>> two defines - except in the link time assertion you always use
>> the sum of the two.
>>
>> > +#define MBI_SIZE                        (2 * PAGE_SIZE)
>>
>> On top of Doug's question - if it is needed at all, what does this
> 
> Please look around reloc() call in xen/arch/x86/boot/head.S. You quickly
> realize that there is following memory layout from top to bottom:
> 
>         +------------------+
>         | TRAMPOLINE_STACK |
>         +------------------+
>         |    TRAMPOLINE    |
>         +------------------+
>         |    mbi struct    |
>         +------------------+
> 
>> match up with, i.e. how come this is 2 pages (and not 1 or 3)?
> 
> Just thought that 8 KiB will be sufficient for Xen/modules arguments,
> memory map and other minor things.

Even with a couple of dozen modules passed? But the primary
question was left unanswered anyway: Does this need placing in
the low megabyte at all? And even if it does, especially if you
limit it to 8k, I don't see why it wouldn't fit inside the 64k
trampoline area.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel