Re: [Xen-devel] [PATCH for-4.5 V9] Add ARM EFI boot support

[Roy Franz <roy.franz@xxxxxxxxxx>]

On Tue, Sep 30, 2014 at 1:25 AM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> On Mon, 2014-09-29 at 09:47 +0100, Jan Beulich wrote:
>> >>> On 27.09.14 at 00:25, <roy.franz@xxxxxxxxxx> wrote:
>> > This patch adds EFI boot support for ARM based on the previous refactoring 
>> > of
>> > the x86 EFI boot code.  All ARM specific code is in the ARM efi-boot.h 
>> > header
>> > file, with the main EFI entry point common/efi/boot.c.  The PE/COFF header
>> > is
>> > open-coded in head.S, which allows us to have a single binary be both an 
>> > EFI
>> > executable and a normal arm64 IMAGE file. There is currently no PE/COFF
>> > toolchain support for arm64, so it is not possible to create the PE/COFF
>> > header
>> > in the same manner as on x86.  This also simplifies the build as compared 
>> > to
>> > x86, as we always build the same executable, whereas x86 builds 2.  An ARM
>> > version of efi-bind.h is added, which is based on the x86_64 version with 
>> > the
>> > x86 specific portions removed.  The Makefile in common/efi is different for
>> > x86
>> > and ARM, as for ARM we always build in EFI support.
>> > NR_MEM_BANKS is increased, as memory regions are now added from the UEFI
>> > memory map,
>> > rather than memory banks from a DTB.  The UEFI memory map may be fragmented
>> > so a larger
>> > number of regions will be used.
>> >
>> > Signed-off-by: Roy Franz <roy.franz@xxxxxxxxxx>
>>
>> For the non-ARM parts:
>> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
>
> Konrad, we would really like to get this into 4.5. It will allow Xen to
> boot on EFI hardware which is already starting to appear in the field.
> It also unblocks other work such as the upstreaming of the Xen on ARM
> boot protocol work which Linaro are doing on grub.
>
> All of the precursor refactoring is already in so the impact of this
> change on x86 is minimal. On the ARM side the changes to the
> common/non-EFI boot path are using well-worn techniques from the Linux
> equivalent so I think we can be reasonably confident of them, the
> remaining code is self contained and can't regress anything else.
>
>>
>> Two minor comments nevertheless:
>>
>> > --- a/xen/common/efi/boot.c
>> > +++ b/xen/common/efi/boot.c
>> > @@ -2,6 +2,7 @@
>> >  #include <efi/efiprot.h>
>> >  #include <efi/efipciio.h>
>> >  #include <public/xen.h>
>> > +#include <xen/bitops.h>
>> >  #include <xen/compile.h>
>> >  #include <xen/ctype.h>
>> >  #include <xen/dmi.h>
>> > @@ -18,9 +19,16 @@
>> >  #include <xen/string.h>
>> >  #include <xen/stringify.h>
>> >  #include <xen/vga.h>
>> > +#ifdef CONFIG_X86
>>
>> The context above shows xen/vga.h - this surely isn't needed in the
>> common code anymore?
>
> Seems likely it isn't. Shall I fixup on commit (I always build test on
> commit, so if it breaks I will certainly catch it) or shall we fix in a
> followup?

It's not needed in boot.c, but is in x86/efi/efi-boot.h.  It should have been
moved along with the vga code.  The following moves it from
boot.c to x86/efi/efi-boot.h.

Ian - can you apply this fixup when you take the patch?


diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index b44746f..82bb74a 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -7,6 +7,7 @@
 #include <asm/edd.h>
 #include <asm/msr.h>
 #include <asm/processor.h>
+#include <xen/vga.h>

 static struct file __initdata ucode;
 static multiboot_info_t __initdata mbi = {
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index e1b243c..f272171 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -18,7 +18,6 @@
 #endif
 #include <xen/string.h>
 #include <xen/stringify.h>
-#include <xen/vga.h>
 #ifdef CONFIG_X86
 /*
  * Keep this arch-specific modified include in the common file, as moving


>
>> > --- /dev/null
>> > +++ b/xen/include/asm-arm/arm64/efibind.h
>> > @@ -0,0 +1,216 @@
>> > +/*++
>> > +
>> > +Copyright (c) 1998  Intel Corporation
>> > +
>> > +Module Name:
>> > +
>> > +    efefind.h
>>
>> Is this so badly misspelled in the original?
>
> Apparently!
>
> $ git grep efefind.h
> xen/include/asm-x86/x86_64/efibind.h:    efefind.h
>
> Shall I fix both cases on commit?
>
> Ian.
>

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