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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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?

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