Re: [Xen-devel] [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Mon, Sep 26, 2016 at 07:47:42AM -0600, Jan Beulich wrote:
> >>> On 23.09.16 at 23:47, <daniel.kiper@xxxxxxxxxx> wrote:
> > This way Xen can be loaded on EFI platforms using GRUB2 and
> > other boot loaders which support multiboot2 protocol.
> >
> > Signed-off-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx>
> > Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> > ---
> > v7 - suggestions/fixes:
> >    - do not allocate twice memory for trampoline if we were
> >      loaded via multiboot2 protocol on EFI platform,
>
> If you fix bugs not noticed by a reviewer but by yourself, please
> drop all acks/R-b-s covering the code in question. And then I'm

OK.

> afraid I haven't even been able to locate that change - could you
> please point out what you did where?

The change is very subtle. I add trampoline_setup label behind

         /* From arch/x86/smpboot.c: start_eip had better be page-aligned! */
         xor     %cl, %cl

instead of

         cmp     %ecx,%edx           /* compare with BDA value */
         cmovb   %edx,%ecx           /* and use the smaller */

> > +        /*
> > +         * Initialize BSS (no nasty surprises!).
> > +         * It must be done earlier than in BIOS case
> > +         * because efi_multiboot2() touches it.
> > +         */
> > +        lea     .startof.(.bss)(%rip),%edi
> > +        mov     $.sizeof.(.bss),%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 usable memory address below 1 MiB;
> > +         *                 memory above this address is reserved for 
> > trampoline;
> > +         *                 memory below this address is used for stack and 
> > as
> > +         *                 a storage for boot data.
>
> How can you validly use memory below this address? (And I'd like to

Right, I should not do that blindly. As quick fix we can check in 
efi_arch_process_memory_map()
that chosen memory region has size cfg.size plus let's say 64 KiB. Is it 
sufficient
for you? However, I think that later (for 4.9?) we should consider what we 
discussed
here https://lists.xen.org/archives/html/xen-devel/2016-09/msg01424.html

> note that this also changed from v6, and the change to comments
> listed in the v7 section and supposedly suggested by me can't cover
> this, as I don't recall having asked for such an adjustment.)

Ah, sorry about that. I should be more precise.

Daniel

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