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

From: Daniel Kiper <daniel.kiper@oracle.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Juergen Gross <JGross@suse.com>,
	sstabellini@kernel.org, andrew.cooper3@citrix.com,
	cardoe@cardoe.com, pgnet.dev@gmail.com, ning.sun@intel.com,
	julien.grall@arm.com, david.vrabel@citrix.com,
	xen-devel@lists.xenproject.org, qiaowei.ren@intel.com,
	gang.wei@intel.com, fu.wei@linaro.org
Subject: Re: [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms
Date: Tue, 27 Sep 2016 20:11:24 +0200	[thread overview]
Message-ID: <20160927181124.GJ28753@olila.local.net-space.pl> (raw)
In-Reply-To: <57E9431E02000078001127DB@prv-mh.provo.novell.com>

On Mon, Sep 26, 2016 at 07:47:42AM -0600, Jan Beulich wrote:
> >>> On 23.09.16 at 23:47, <daniel.kiper@oracle.com> 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@oracle.com>
> > Acked-by: Jan Beulich <jbeulich@suse.com>
> > ---
> > 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@lists.xen.org
https://lists.xen.org/xen-devel