Re: [Xen-devel] [PATCH v13 4/9] x86: add multiboot2 protocol support for EFI platforms

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.01.17 at 23:49, <daniel.kiper@xxxxxxxxxx> wrote:
> On Wed, Jan 25, 2017 at 04:20:30PM -0600, Doug Goldstein wrote:
>> This is a huge change and would really be helpful to have the diff of
>> what's changed to work from.
> 
> Please look below...

Thanks for providing this - I'll comment this rather than the full patch:

> @@ -123,6 +116,15 @@ efi_platform:
>  .Lbad_ldr_nih: .asciz "ERR: EFI ImageHandle is not provided by bootloader!"
>  .Lbad_efi_msg: .asciz "ERR: EFI IA-32 platforms are not supported!"
>  
> +        .section .init.data, "a", @progbits

This needs to be a writable section.

> @@ -170,6 +172,12 @@ not_multiboot:
>          .code64
>  
>  __efi64_mb2_start:
> +        /*
> +         * Multiboot2 spec says that here CPU is in 64-bit mode. However, 
> there
> +         * is also guarantee that all code and data is always put by the 
> bootloader
> +         * below 4 GiB. Hence, we can safely use in most cases 32-bit 
> addressing.
> +         */

"use 32-bit addressing" is misleading: I don't think you use any such,
since - as pointed out during earlier review - this would needlessly
cause 0x67 prefixes to be emitted. Instead what you mean is that
we can safely truncate addresses.

> @@ -180,7 +188,7 @@ __efi64_mb2_start:
>          je      .Lefi_multiboot2_proto
>  
>          /* Jump to not_multiboot after switching CPU to x86_32 mode. */
> -        lea     not_multiboot(%rip),%edi
> +        lea     not_multiboot(%rip),%r15d

In cases like this, where a REX prefix is needed anyway, please
use the full register unless you strictly need it zero-extended
from 32 bits.

> +        /* Align the stack as UEFI spec requires. */
> +        add     $15,%rsp
> +        and     $~15,%rsp

How come you _add_ something here first? Simply do the AND and
be done. Also please extend the comment along the lines of what
I had asked for before: To warn of future changes to the number
of items pushed onto the stack below.

> @@ -280,13 +286,13 @@ run_bs:
>  
>          pop     %rdi
>  
> +        /* Align the stack as UEFI spec requires. */
> +        push    %rdi

Please combine the two into "mov (%rsp), %rdi" and make the
comment say "Keep the stack aligned; don't pop a single item
off it" or some such.

> @@ -424,26 +433,44 @@ trampoline_bios_setup:
>          cmp     %ecx,%edx           /* compare with BDA value */
>          cmovb   %edx,%ecx           /* and use the smaller */
>  
> -        /* Reserve memory for the trampoline. */
> -        sub     $((MB_TRAMPOLINE_SIZE+MB_TRAMPOLINE_STACK_SIZE)>>4),%ecx
> +        /* Reserve memory for the trampoline and the low-memory stack. */
> +        sub     $((TRAMPOLINE_SPACE+TRAMPOLINE_STACK_SPACE)>>4),%ecx
>  
>          /* From arch/x86/smpboot.c: start_eip had better be page-aligned! */
>          xor     %cl, %cl
>  
>  trampoline_setup:
> -        /* Save trampoline address for later use. */
>          shl     $4, %ecx
>          mov     %ecx,sym_phys(trampoline_phys)
>  
> +        /* Get topmost low-memory stack address. */
> +        add     $TRAMPOLINE_SPACE,%ecx

The top-most address of the stack is
%ecx + TRAMPOLINE_SPACE + TRAMPOLINE_STACK_SPACE - 1.
Please don't add misleading comments.

>          /* Save the Multiboot info struct (after relocation) for later use. 
> */
>          mov     $sym_phys(cpu0_stack)+1024,%esp
> -        push    %ecx                /* Boot trampoline address. */
> +        push    %ecx                /* Topmost low-memory stack address. */
>          push    %ebx                /* Multiboot information address. */
>          push    %eax                /* Multiboot magic. */
>          call    reloc
>          mov     %eax,sym_phys(multiboot_ptr)
>  
>          /*
> +         * Now trampoline_phys points to the following structure (lowest
> +         * address is at the top):
> +         *
> +         * +------------------------+
> +         * |    TRAMPOLINE_SPACE    |
> +         * +- - - - - - - - - - - - +
> +         * |       mbi struct       |
> +         * +------------------------+
> +         * | TRAMPOLINE_STACK_SPACE |
> +         * +------------------------+
> +         *
> +         * mbi struct lives at the end of TRAMPOLINE_SPACE. The rest of
> +         * TRAMPOLINE_SPACE is reserved for trampoline code and data.
> +         */

Please can you clarify here that the MBI data grows downwards
from the beginning of the stack to the end of the trampoline?

> @@ -696,8 +699,8 @@ paddr_t __init efi_multiboot2(EFI_HANDLE ImageHandle, 
> EFI_SYSTEM_TABLE *SystemTa
>  
>      efi_exit_boot(ImageHandle, SystemTable);
>  
> -    /* Return highest allocated memory address below 1 MiB. */
> -    return cfg.addr + cfg.size;
> +    /* Return trampoline address. */
> +    return trampoline_phys;
>  }

With this it would be less confusing if you move the trampoline_setup
label down a few more lines. Perhaps the function here could then
return void.

> --- a/xen/arch/x86/efi/stub.c
> +++ b/xen/arch/x86/efi/stub.c
> @@ -20,7 +20,8 @@
>  paddr_t __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle,
>                                         EFI_SYSTEM_TABLE *SystemTable)
>  {
> -    CHAR16 *err = L"Xen does not have EFI code build in!!!\r\nSystem 
> halted!!!\r\n";
> +    static const CHAR16 __initconst err[] =
> +        L"(XEN) Xen does not have EFI code build in!\r\n(XEN) System 
> halted!\r\n";

Why did you add these (XEN) prefixes?

> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -334,5 +334,5 @@ ASSERT(IS_ALIGNED(trampoline_end,   4), "trampoline_end 
> misaligned")
>  ASSERT(IS_ALIGNED(__bss_start,      8), "__bss_start misaligned")
>  ASSERT(IS_ALIGNED(__bss_end,        8), "__bss_end misaligned")
>  
> -ASSERT((trampoline_end - trampoline_start) < MB_TRAMPOLINE_SIZE,
> +ASSERT((trampoline_end - trampoline_start) < TRAMPOLINE_SPACE,
>      "not enough room for trampoline")

Didn't you mean to make sure there are at least two pages for
MBI data?

Jan

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