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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Tue, Jan 31, 2017 at 05:33:12AM -0700, Jan Beulich wrote:
> >>> 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:

If you wish I can do that next time too.

> > @@ -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.

AIUI, it is by default but if you wish I can replace "a" with "aw" here.

> > @@ -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.

OK.

> > @@ -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.

OK.

> > +        /* 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

Facepalm! Err... Why I forgot here that stack grows downward...

> I had asked for before: To warn of future changes to the number
> of items pushed onto the stack below.

OK.

> > @@ -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.

OK.

> > @@ -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.

Right, it is misleading. Do you think that:

Get the lowest low-memory stack address.

...is better?

> >          /* 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?

OK, but I think that "beginning of the stack" should be replaced
with "the end of TRAMPOLINE_SPACE" here.

> > @@ -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.

If you wish.

> > --- 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?

To align message format with messages printed from most places. And I realized
that it will be nice to do the same thing in head.S and the rest of EFI code.
This way it is much easier to differentiate between a bootloader and Xen 
messages.
Though it begs another patch series.

> > --- 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?

Do you wish plain number here or constant like MBI_SIZE defined somewhere.
I think that constant is better thing.

Daniel

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