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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Wed, Sep 28, 2016 at 02:57:10AM -0600, Jan Beulich wrote:
> >>> On 27.09.16 at 20:11, <daniel.kiper@xxxxxxxxxx> wrote:
> > 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 */
>
> So how did you expect anyone having looked at previous version
> to spot that? Please, in your changes section, rather than
> tediously listing who suggested which change, please make that
> section useful for incremental reviews.

I think that in general it is useful, however, I can agree that
this thing could be described better.

> >> > +        /*
> >> > +         * 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?
>
> Depends. Part of my problem here is that you convert, in your answer,
> my "validly" to "blindly". And then I'd like to see especially "storage for

I am not sure why the difference, IMO minor, is important for you here.

> boot data" better qualified: What exactly is it that you mean to store
> there?

mbi struct created in reloc.c from original multiboot(2) data passed by boot 
loader.

> I don't recall having noticed either this or that area being used
> as stack anywhere in the series, so I'm afraid I've overlooked

Hmmm... I do not know why I put stack here. It should be dropped from comment.

Daniel

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