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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

>> > +        /*
>> > +         * 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
boot data" better qualified: What exactly is it that you mean to store
there? 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
something which may invalidate a good part of the review done.

Jan


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