All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH RFC 00/19] arm64 EFI stub
@ 2014-06-28  1:25 Roy Franz
  2014-06-28  1:25 ` [PATCH RFC 01/19] HACK: Add -fshort-wchar to global build Roy Franz
                   ` (22 more replies)
  0 siblings, 23 replies; 44+ messages in thread
From: Roy Franz @ 2014-06-28  1:25 UTC (permalink / raw)
  To: xen-devel, ian.campbell, stefano.stabellini, tim, jbeulich, keir
  Cc: Roy Franz, fu.wei, linaro-uefi


This patch series is an RFC series that adds EFI support for arm64 in the 
form of a EFI stub in similar fashion to the linux kernel EFI support.  
A PE/COFF header is created in head.S, as there is no toolchain support
for PE/COFF on arm64.  This also has the advantage that the file is both 
an "Image" file and a PE/COFF executable - the same binary can be loaded 
and run either way.  The EFI 'stub' code is a shim layer that serves as 
the loader for the XEN kernel in the EFI environment.  The stub loads
the dom0 kernel and initrd if required, and adds entries for them as well 
as for the EFI data structures into the device tree passed to XEN.  Once 
the device tree is constructed, EFI boot services are exited, and the stub 
transfers control to the normal XEN entry point.  The only indication
XEN has that it was loaded via the stub is that the device tree contains 
EFI properties.  This is all very similar to the arm/arm64 Linux kernel 
EFI stubs.


The first 14 patches refactor and move x86 EFI code so that it can be 
shared with the arm64 EFI stub.  The remaining 5 patches add the new
arm64 EFI stub.

One significant omission from this series is proper EFI memory map 
handling in XEN itself.  This patch instead creates FDT memory nodes based 
on the EFI memory map.  This is functional, but not how we want to do 
it long term.  The XEN kernel updates for this will be largely
disjoint from this series, and I will be starting on that next.  I wanted 
to get this portion out for review without waiting for that portion of the 
code to be done.  The ADD_EFI_MEMORY_TO_FDT macro isolates code that will 
change when the proper EFI memory map handling is added to XEN.

I have some some simple tests booting from the EFI shell on arm64 (FVP 
base model) and x86_64 (vmware.)

This patch series is available on my git tree:
git://git.linaro.org/people/roy.franz/xen.git
tag: xen-efi-stub-rfc-20140627


There are a few open issues in this patch series that I would appreciate 
feedback/suggestions on:

1) Build system changes.  The 'efi-shared.c' file should be properly 
shared, rather than symbolicly linked.  One complication is that the EFI 
code (for both archs) needs to be compiled with "-fshort-wchar".  I also 
likely need to create an efi subdir for arm64.

2) Is it valid to start XEN with a device tree that only contains 
multi-boot and EFI info? (As would be the case if the stub doesn't get a 
device tree as input.)  Currently this isn't supported, some libfdt 
functions are missing, so I'm checking if this is desired before I add 
that.

3) I'm not sure arm64 needs it's own copy of efibind.h.  The x86_64 
version worked fine as is, but has some Microsoft related defines in 
there.  The arm64 version I created is a proper subset with the exception 
of the EFI_STUB_ERROR define.


(I am on vacation the week of June 30th-July 4th, so my responses will be 
delayed.)

Roy Franz (19):
  HACK: Add -fshort-wchar to global build
  Create efi-shared.[ch], and move string functions
  Move more functions from boot.c to efi-shared.c
  rename printErrMsg to PrintErrMesgExit
  Add PrintErrMesg function that doesn't exit
  Refactor read_file() so it can be shared.
  move read_file() to efi-shared.c
  Move removal of leading spaces from split_value to get_value()
  replace split_value() with truncate_string()
  move truncate_string() to efi-shared.c
  add read_config_file() function for XEN EFI config file
  create handle_cmdline() function
  Refactor get_argv() for sharing
  Move get_argv() and handle_cmdline() to efi-shared.c
  Add PE/COFF header in head.S
  create ARM EFI headers, based on x86
  Remove x86 specific defintions from efibind.h
  Add assembler use support for efibind.h
  Add EFI stub for ARM64

 Config.mk                           |   2 +
 xen/arch/arm/Makefile               |   2 +
 xen/arch/arm/arm64/head.S           | 178 +++++++++-
 xen/arch/arm/efi-shared.c           |   1 +
 xen/arch/arm/efi.c                  | 686 ++++++++++++++++++++++++++++++++++++
 xen/arch/arm/efi.h                  |  11 +
 xen/arch/arm/xen.lds.S              |   1 +
 xen/arch/x86/Makefile               |   4 +-
 xen/arch/x86/efi/Makefile           |   2 +-
 xen/arch/x86/efi/boot.c             | 614 +++-----------------------------
 xen/arch/x86/efi/efi-shared.c       | 620 ++++++++++++++++++++++++++++++++
 xen/include/asm-arm/arm64/efibind.h | 206 +++++++++++
 xen/include/asm-arm/efibind.h       |   2 +
 xen/include/efi/efi-shared.h        |  71 ++++
 14 files changed, 1832 insertions(+), 568 deletions(-)
 create mode 120000 xen/arch/arm/efi-shared.c
 create mode 100644 xen/arch/arm/efi.c
 create mode 100644 xen/arch/arm/efi.h
 create mode 100644 xen/arch/x86/efi/efi-shared.c
 create mode 100644 xen/include/asm-arm/arm64/efibind.h
 create mode 100644 xen/include/asm-arm/efibind.h
 create mode 100644 xen/include/efi/efi-shared.h

-- 
2.0.0

^ permalink raw reply	[flat|nested] 44+ messages in thread

* [PATCH RFC 01/19] HACK: Add -fshort-wchar to global build
  2014-06-28  1:25 [PATCH RFC 00/19] arm64 EFI stub Roy Franz
@ 2014-06-28  1:25 ` Roy Franz
  2014-06-30 11:13   ` Jan Beulich
  2014-06-28  1:25 ` [PATCH RFC 02/19] Create efi-shared.[ch], and move string functions Roy Franz
                   ` (21 subsequent siblings)
  22 siblings, 1 reply; 44+ messages in thread
From: Roy Franz @ 2014-06-28  1:25 UTC (permalink / raw)
  To: xen-devel, ian.campbell, stefano.stabellini, tim, jbeulich, keir
  Cc: Roy Franz, fu.wei, linaro-uefi

The EFI related code needs to be buil with "-fshort-wchar", as EFI
defines 16 bit wide characters and uses them in various APIs.
This fixes the prelink target build failures in the arch/x86 directory,
and is needed for the ARM EFI stub files as well.  The prelink build
failures may only happen during parallel builds.

This will be removed as part of getting the x86 and ARM archs to share
code and have it built correctly.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 Config.mk | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Config.mk b/Config.mk
index 5e03e87..b323c59 100644
--- a/Config.mk
+++ b/Config.mk
@@ -189,6 +189,8 @@ CFLAGS += -std=gnu99
 
 CFLAGS += -Wall -Wstrict-prototypes
 
+CFLAGS += -fshort-wchar
+
 # Clang complains about macros that expand to 'if ( ( foo == bar ) ) ...'
 # and is over-zealous with the printf format lint
 # and is a bit too fierce about unused return values
-- 
2.0.0

^ permalink raw reply related	[flat|nested] 44+ messages in thread

* [PATCH RFC 02/19] Create efi-shared.[ch], and move string functions
  2014-06-28  1:25 [PATCH RFC 00/19] arm64 EFI stub Roy Franz
  2014-06-28  1:25 ` [PATCH RFC 01/19] HACK: Add -fshort-wchar to global build Roy Franz
@ 2014-06-28  1:25 ` Roy Franz
  2014-06-30 11:15   ` Jan Beulich
  2014-06-28  1:25 ` [PATCH RFC 03/19] Move more functions from boot.c to efi-shared.c Roy Franz
                   ` (20 subsequent siblings)
  22 siblings, 1 reply; 44+ messages in thread
From: Roy Franz @ 2014-06-28  1:25 UTC (permalink / raw)
  To: xen-devel, ian.campbell, stefano.stabellini, tim, jbeulich, keir
  Cc: Roy Franz, fu.wei, linaro-uefi

Create the files for EFI code that will be shared with the ARM stub,
and move string functions and some global variables.

TODO: proper sharing of file (with proper -fshort-char compile option)

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 xen/arch/x86/Makefile         |   4 +-
 xen/arch/x86/efi/Makefile     |   2 +-
 xen/arch/x86/efi/boot.c       | 127 +++---------------------------------------
 xen/arch/x86/efi/efi-shared.c | 122 ++++++++++++++++++++++++++++++++++++++++
 xen/include/efi/efi-shared.h  |  39 +++++++++++++
 5 files changed, 171 insertions(+), 123 deletions(-)
 create mode 100644 xen/arch/x86/efi/efi-shared.c
 create mode 100644 xen/include/efi/efi-shared.h

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 6c90b1b..7504780 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -87,13 +87,13 @@ prelink-efi_lto.o: $(ALL_OBJS) efi/runtime.o efi/compat.o
 prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o
 	$(LD) $(LDFLAGS) -r -o $@ $^
 
-prelink-efi.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink-efi_lto.o efi/boot.init.o
+prelink-efi.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink-efi_lto.o efi/boot.init.o efi/efi-shared.o
 	$(guard) $(LD) $(LDFLAGS) -r -o $@ $^
 else
 prelink.o: $(ALL_OBJS)
 	$(LD) $(LDFLAGS) -r -o $@ $^
 
-prelink-efi.o: $(ALL_OBJS) efi/boot.init.o efi/runtime.o efi/compat.o
+prelink-efi.o: $(ALL_OBJS) efi/boot.init.o efi/runtime.o efi/compat.o efi/efi-shared.o
 	$(guard) $(LD) $(LDFLAGS) -r -o $@ $(filter-out %/efi/built_in.o,$^)
 endif
 
diff --git a/xen/arch/x86/efi/Makefile b/xen/arch/x86/efi/Makefile
index 1daa7ac..9c198d2 100644
--- a/xen/arch/x86/efi/Makefile
+++ b/xen/arch/x86/efi/Makefile
@@ -9,6 +9,6 @@ efi := $(if $(efi),$(shell $(CC) $(filter-out $(CFLAGS-y) .%.d,$(CFLAGS)) -c che
 efi := $(if $(efi),$(shell $(LD) -mi386pep --subsystem=10 -o check.efi check.o 2>disabled && echo y))
 efi := $(if $(efi),$(shell rm disabled)y,$(shell $(call create,boot.init.o); $(call create,runtime.o)))
 
-extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o
+extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o efi-shared.o
 
 stub.o: $(extra-y)
diff --git a/xen/arch/x86/efi/boot.c b/xen/arch/x86/efi/boot.c
index 2b515f2..2b6bea3 100644
--- a/xen/arch/x86/efi/boot.c
+++ b/xen/arch/x86/efi/boot.c
@@ -1,6 +1,7 @@
 #include "efi.h"
 #include <efi/efiprot.h>
 #include <efi/efipciio.h>
+#include <efi/efi-shared.h>
 #include <public/xen.h>
 #include <xen/compile.h>
 #include <xen/ctype.h>
@@ -44,25 +45,16 @@ typedef struct {
 extern char start[];
 extern u32 cpuid_ext_features;
 
-union string {
-    CHAR16 *w;
-    char *s;
-    const char *cs;
-};
 
-struct file {
-    UINTN size;
-    union {
-        EFI_PHYSICAL_ADDRESS addr;
-        void *ptr;
-    };
-};
+/* Variables supplied/used by shared EFI code. */
+extern CHAR16 __initdata newline[];
+extern EFI_BOOT_SERVICES *__initdata efi_bs;
+extern SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdOut;
+extern SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdErr;
+
 
-static EFI_BOOT_SERVICES *__initdata efi_bs;
 static EFI_HANDLE __initdata efi_ih;
 
-static SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdOut;
-static SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdErr;
 
 static UINT32 __initdata mdesc_ver;
 
@@ -77,111 +69,6 @@ static multiboot_info_t __initdata mbi = {
 };
 static module_t __initdata mb_modules[3];
 
-static CHAR16 __initdata newline[] = L"\r\n";
-
-#define PrintStr(s) StdOut->OutputString(StdOut, s)
-#define PrintErr(s) StdErr->OutputString(StdErr, s)
-
-static CHAR16 *__init FormatDec(UINT64 Val, CHAR16 *Buffer)
-{
-    if ( Val >= 10 )
-        Buffer = FormatDec(Val / 10, Buffer);
-    *Buffer = (CHAR16)(L'0' + Val % 10);
-    return Buffer + 1;
-}
-
-static CHAR16 *__init FormatHex(UINT64 Val, UINTN Width, CHAR16 *Buffer)
-{
-    if ( Width > 1 || Val >= 0x10 )
-        Buffer = FormatHex(Val >> 4, Width ? Width - 1 : 0, Buffer);
-    *Buffer = (CHAR16)((Val &= 0xf) < 10 ? L'0' + Val : L'a' + Val - 10);
-    return Buffer + 1;
-}
-
-static void __init DisplayUint(UINT64 Val, INTN Width)
-{
-    CHAR16 PrintString[32], *end;
-
-    if (Width < 0)
-        end = FormatDec(Val, PrintString);
-    else
-    {
-        PrintStr(L"0x");
-        end = FormatHex(Val, Width, PrintString);
-    }
-    *end = 0;
-    PrintStr(PrintString);
-}
-
-static CHAR16 *__init wstrcpy(CHAR16 *d, const CHAR16 *s)
-{
-    CHAR16 *r = d;
-
-    while ( (*d++ = *s++) != 0 )
-        ;
-    return r;
-}
-
-static int __init wstrcmp(const CHAR16 *s1, const CHAR16 *s2)
-{
-    while ( *s1 && *s1 == *s2 )
-    {
-        ++s1;
-        ++s2;
-    }
-    return *s1 - *s2;
-}
-
-static int __init wstrncmp(const CHAR16 *s1, const CHAR16 *s2, UINTN n)
-{
-    while ( n && *s1 && *s1 == *s2 )
-    {
-        --n;
-        ++s1;
-        ++s2;
-    }
-    return n ? *s1 - *s2 : 0;
-}
-
-static CHAR16 *__init s2w(union string *str)
-{
-    const char *s = str->s;
-    CHAR16 *w;
-    void *ptr;
-
-    if ( efi_bs->AllocatePool(EfiLoaderData, (strlen(s) + 1) * sizeof(*w),
-                              &ptr) != EFI_SUCCESS )
-        return NULL;
-
-    w = str->w = ptr;
-    do {
-        *w = *s++;
-    } while ( *w++ );
-
-    return str->w;
-}
-
-static char *__init w2s(const union string *str)
-{
-    const CHAR16 *w = str->w;
-    char *s = str->s;
-
-    do {
-        if ( *w > 0x007f )
-            return NULL;
-        *s = *w++;
-    } while ( *s++ );
-
-    return str->s;
-}
-
-static bool_t __init match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2)
-{
-    return guid1->Data1 == guid2->Data1 &&
-           guid1->Data2 == guid2->Data2 &&
-           guid1->Data3 == guid2->Data3 &&
-           !memcmp(guid1->Data4, guid2->Data4, sizeof(guid1->Data4));
-}
 
 static void __init noreturn blexit(const CHAR16 *str)
 {
diff --git a/xen/arch/x86/efi/efi-shared.c b/xen/arch/x86/efi/efi-shared.c
new file mode 100644
index 0000000..b9c563a
--- /dev/null
+++ b/xen/arch/x86/efi/efi-shared.c
@@ -0,0 +1,122 @@
+/* EFI code shared between architectures. */
+
+#include "efi.h"
+#include <efi/efiprot.h>
+#include <efi/efipciio.h>
+#include <efi/efi-shared.h>
+#include <public/xen.h>
+#include <efi/efi-shared.h>
+#include <xen/compile.h>
+#include <xen/ctype.h>
+#include <xen/init.h>
+#include <asm/processor.h>
+
+
+SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdOut;
+SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdErr;
+EFI_BOOT_SERVICES *__initdata efi_bs;
+
+
+CHAR16 __initdata newline[] = L"\r\n";
+
+CHAR16 *__init FormatDec(UINT64 Val, CHAR16 *Buffer)
+{
+    if ( Val >= 10 )
+        Buffer = FormatDec(Val / 10, Buffer);
+    *Buffer = (CHAR16)(L'0' + Val % 10);
+    return Buffer + 1;
+}
+
+CHAR16 *__init FormatHex(UINT64 Val, UINTN Width, CHAR16 *Buffer)
+{
+    if ( Width > 1 || Val >= 0x10 )
+        Buffer = FormatHex(Val >> 4, Width ? Width - 1 : 0, Buffer);
+    *Buffer = (CHAR16)((Val &= 0xf) < 10 ? L'0' + Val : L'a' + Val - 10);
+    return Buffer + 1;
+}
+
+
+void __init DisplayUint(UINT64 Val, INTN Width)
+{
+    CHAR16 PrintString[32], *end;
+
+    if (Width < 0)
+        end = FormatDec(Val, PrintString);
+    else
+    {
+        PrintStr(L"0x");
+        end = FormatHex(Val, Width, PrintString);
+    }
+    *end = 0;
+    PrintStr(PrintString);
+}
+
+CHAR16 *__init wstrcpy(CHAR16 *d, const CHAR16 *s)
+{
+    CHAR16 *r = d;
+
+    while ( (*d++ = *s++) != 0 )
+        ;
+    return r;
+}
+
+int __init wstrcmp(const CHAR16 *s1, const CHAR16 *s2)
+{
+    while ( *s1 && *s1 == *s2 )
+    {
+        ++s1;
+        ++s2;
+    }
+    return *s1 - *s2;
+}
+
+int __init wstrncmp(const CHAR16 *s1, const CHAR16 *s2, UINTN n)
+{
+    while ( n && *s1 && *s1 == *s2 )
+    {
+        --n;
+        ++s1;
+        ++s2;
+    }
+    return n ? *s1 - *s2 : 0;
+}
+
+CHAR16 *__init s2w(union string *str)
+{
+    const char *s = str->s;
+    CHAR16 *w;
+    void *ptr;
+
+    if ( efi_bs->AllocatePool(EfiLoaderData, (strlen(s) + 1) * sizeof(*w),
+                              &ptr) != EFI_SUCCESS )
+        return NULL;
+
+    w = str->w = ptr;
+    do {
+        *w = *s++;
+    } while ( *w++ );
+
+    return str->w;
+}
+
+char *__init w2s(const union string *str)
+{
+    const CHAR16 *w = str->w;
+    char *s = str->s;
+
+    do {
+        if ( *w > 0x007f )
+            return NULL;
+        *s = *w++;
+    } while ( *s++ );
+
+    return str->s;
+}
+
+bool_t __init match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2)
+{
+    return guid1->Data1 == guid2->Data1 &&
+           guid1->Data2 == guid2->Data2 &&
+           guid1->Data3 == guid2->Data3 &&
+           !memcmp(guid1->Data4, guid2->Data4, sizeof(guid1->Data4));
+}
diff --git a/xen/include/efi/efi-shared.h b/xen/include/efi/efi-shared.h
new file mode 100644
index 0000000..b4c7ac5
--- /dev/null
+++ b/xen/include/efi/efi-shared.h
@@ -0,0 +1,39 @@
+#ifndef __EFI_SHARED_H__
+#define __EFI_SHARED_H__
+
+#include <efi/efidef.h>
+#include <xen/init.h>
+
+
+union string {
+    CHAR16 *w;
+    char *s;
+    const char *cs;
+};
+
+struct file {
+    UINTN size;
+    union {
+        EFI_PHYSICAL_ADDRESS addr;
+        void *ptr;
+    };
+};
+
+
+#define PrintStr(s) StdOut->OutputString(StdOut, s)
+#define PrintErr(s) StdErr->OutputString(StdErr, s)
+
+
+
+CHAR16 * FormatDec(UINT64 Val, CHAR16 *Buffer);
+CHAR16 * FormatHex(UINT64 Val, UINTN Width, CHAR16 *Buffer);
+
+void __init DisplayUint(UINT64 Val, INTN Width);
+CHAR16 *__init wstrcpy(CHAR16 *d, const CHAR16 *s);
+int __init wstrcmp(const CHAR16 *s1, const CHAR16 *s2);
+int __init wstrncmp(const CHAR16 *s1, const CHAR16 *s2, UINTN n);
+CHAR16 *__init s2w(union string *str);
+char *__init w2s(const union string *str);
+bool_t __init match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2);
+
+#endif
-- 
2.0.0

^ permalink raw reply related	[flat|nested] 44+ messages in thread

* [PATCH RFC 03/19] Move more functions from boot.c to efi-shared.c
  2014-06-28  1:25 [PATCH RFC 00/19] arm64 EFI stub Roy Franz
  2014-06-28  1:25 ` [PATCH RFC 01/19] HACK: Add -fshort-wchar to global build Roy Franz
  2014-06-28  1:25 ` [PATCH RFC 02/19] Create efi-shared.[ch], and move string functions Roy Franz
@ 2014-06-28  1:25 ` Roy Franz
  2014-06-28  1:25 ` [PATCH RFC 04/19] rename printErrMsg to PrintErrMesgExit Roy Franz
                   ` (19 subsequent siblings)
  22 siblings, 0 replies; 44+ messages in thread
From: Roy Franz @ 2014-06-28  1:25 UTC (permalink / raw)
  To: xen-devel, ian.campbell, stefano.stabellini, tim, jbeulich, keir
  Cc: Roy Franz, fu.wei, linaro-uefi

Move the remainder of the functions that can be moved as is to efi-shared.c.
This change is strictly moving of code without functional changes.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 xen/arch/x86/efi/boot.c       | 212 +----------------------------------------
 xen/arch/x86/efi/efi-shared.c | 214 +++++++++++++++++++++++++++++++++++++++++-
 xen/include/efi/efi-shared.h  |  17 ++++
 3 files changed, 231 insertions(+), 212 deletions(-)

diff --git a/xen/arch/x86/efi/boot.c b/xen/arch/x86/efi/boot.c
index 2b6bea3..2d32d7b 100644
--- a/xen/arch/x86/efi/boot.c
+++ b/xen/arch/x86/efi/boot.c
@@ -7,7 +7,6 @@
 #include <xen/ctype.h>
 #include <xen/dmi.h>
 #include <xen/init.h>
-#include <xen/keyhandler.h>
 #include <xen/lib.h>
 #include <xen/mm.h>
 #include <xen/multiboot.h>
@@ -70,7 +69,7 @@ static multiboot_info_t __initdata mbi = {
 static module_t __initdata mb_modules[3];
 
 
-static void __init noreturn blexit(const CHAR16 *str)
+void __init noreturn blexit(const CHAR16 *str)
 {
     if ( str )
         PrintStr((CHAR16 *)str);
@@ -91,56 +90,6 @@ static void __init noreturn blexit(const CHAR16 *str)
     unreachable(); /* not reached */
 }
 
-/* generic routine for printing error messages */
-static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode)
-{
-    StdOut = StdErr;
-    PrintErr((CHAR16 *)mesg);
-    PrintErr(L": ");
-
-    switch (ErrCode)
-    {
-    case EFI_NOT_FOUND:
-        mesg = L"Not found";
-        break;
-    case EFI_NO_MEDIA:
-        mesg = L"The device has no media";
-        break;
-    case EFI_MEDIA_CHANGED:
-        mesg = L"Media changed";
-        break;
-    case EFI_DEVICE_ERROR:
-        mesg = L"Device error";
-        break;
-    case EFI_VOLUME_CORRUPTED:
-        mesg = L"Volume corrupted";
-        break;
-    case EFI_ACCESS_DENIED:
-        mesg = L"Access denied";
-        break;
-    case EFI_OUT_OF_RESOURCES:
-        mesg = L"Out of resources";
-        break;
-    case EFI_VOLUME_FULL:
-        mesg = L"Volume is full";
-        break;
-    case EFI_SECURITY_VIOLATION:
-        mesg = L"Security violation";
-        break;
-    case EFI_CRC_ERROR:
-        mesg = L"CRC error";
-        break;
-    case EFI_COMPROMISED_DATA:
-        mesg = L"Compromised data";
-        break;
-    default:
-        PrintErr(L"ErrCode: ");
-        DisplayUint(ErrCode, 0);
-        mesg = NULL;
-        break;
-    }
-    blexit(mesg);
-}
 
 static void __init place_string(u32 *addr, const char *s)
 {
@@ -215,108 +164,6 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
     return argc;
 }
 
-static EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
-                                                CHAR16 **leaf)
-{
-    static EFI_GUID __initdata fs_protocol = SIMPLE_FILE_SYSTEM_PROTOCOL;
-    EFI_FILE_HANDLE dir_handle;
-    EFI_DEVICE_PATH *dp;
-    CHAR16 *pathend, *ptr;
-    EFI_STATUS ret;
-
-    do {
-        EFI_FILE_IO_INTERFACE *fio;
-
-        /* Get the file system interface. */
-        ret = efi_bs->HandleProtocol(loaded_image->DeviceHandle,
-                                     &fs_protocol, (void **)&fio);
-        if ( EFI_ERROR(ret) )
-            PrintErrMesg(L"Couldn't obtain the File System Protocol Interface",
-                         ret);
-        ret = fio->OpenVolume(fio, &dir_handle);
-    } while ( ret == EFI_MEDIA_CHANGED );
-    if ( ret != EFI_SUCCESS )
-        PrintErrMesg(L"OpenVolume failure", ret);
-
-#define buffer ((CHAR16 *)keyhandler_scratch)
-#define BUFFERSIZE sizeof(keyhandler_scratch)
-    for ( dp = loaded_image->FilePath, *buffer = 0;
-          DevicePathType(dp) != END_DEVICE_PATH_TYPE;
-          dp = (void *)dp + DevicePathNodeLength(dp) )
-    {
-        FILEPATH_DEVICE_PATH *fp;
-
-        if ( DevicePathType(dp) != MEDIA_DEVICE_PATH ||
-             DevicePathSubType(dp) != MEDIA_FILEPATH_DP )
-            blexit(L"Unsupported device path component");
-
-        if ( *buffer )
-        {
-            EFI_FILE_HANDLE new_handle;
-
-            ret = dir_handle->Open(dir_handle, &new_handle, buffer,
-                                   EFI_FILE_MODE_READ, 0);
-            if ( ret != EFI_SUCCESS )
-            {
-                PrintErr(L"Open failed for ");
-                PrintErrMesg(buffer, ret);
-            }
-            dir_handle->Close(dir_handle);
-            dir_handle = new_handle;
-        }
-        fp = (void *)dp;
-        if ( BUFFERSIZE < DevicePathNodeLength(dp) -
-                          sizeof(*dp) + sizeof(*buffer) )
-            blexit(L"Increase BUFFERSIZE");
-        memcpy(buffer, fp->PathName, DevicePathNodeLength(dp) - sizeof(*dp));
-        buffer[(DevicePathNodeLength(dp) - sizeof(*dp)) / sizeof(*buffer)] = 0;
-    }
-    for ( ptr = buffer, pathend = NULL; *ptr; ++ptr )
-        if ( *ptr == L'\\' )
-            pathend = ptr;
-    if ( pathend )
-    {
-        *pathend = 0;
-        *leaf = pathend + 1;
-        if ( *buffer )
-        {
-            EFI_FILE_HANDLE new_handle;
-
-            ret = dir_handle->Open(dir_handle, &new_handle, buffer,
-                                   EFI_FILE_MODE_READ, 0);
-            if ( ret != EFI_SUCCESS ) {
-                PrintErr(L"Open failed for ");
-                PrintErrMesg(buffer, ret);
-            }
-            dir_handle->Close(dir_handle);
-            dir_handle = new_handle;
-        }
-    }
-    else
-        *leaf = buffer;
-#undef BUFFERSIZE
-#undef buffer
-
-    return dir_handle;
-}
-
-static CHAR16 *__init point_tail(CHAR16 *fn)
-{
-    CHAR16 *tail = NULL;
-
-    for ( ; ; ++fn )
-        switch ( *fn )
-        {
-        case 0:
-            return tail;
-        case L'.':
-        case L'-':
-        case L'_':
-            tail = fn;
-            break;
-        }
-}
-
 static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
                                struct file *file)
 {
@@ -393,63 +240,6 @@ static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
     return 1;
 }
 
-static void __init pre_parse(const struct file *cfg)
-{
-    char *ptr = cfg->ptr, *end = ptr + cfg->size;
-    bool_t start = 1, comment = 0;
-
-    for ( ; ptr < end; ++ptr )
-    {
-        if ( iscntrl(*ptr) )
-        {
-            comment = 0;
-            start = 1;
-            *ptr = 0;
-        }
-        else if ( comment || (start && isspace(*ptr)) )
-            *ptr = 0;
-        else if ( *ptr == '#' || (start && *ptr == ';') )
-        {
-            comment = 1;
-            *ptr = 0;
-        }
-        else
-            start = 0;
-    }
-    if ( cfg->size && end[-1] )
-         PrintStr(L"No newline at end of config file,"
-                   " last line will be ignored.\r\n");
-}
-
-static char *__init get_value(const struct file *cfg, const char *section,
-                              const char *item)
-{
-    char *ptr = cfg->ptr, *end = ptr + cfg->size;
-    size_t slen = section ? strlen(section) : 0, ilen = strlen(item);
-    bool_t match = !slen;
-
-    for ( ; ptr < end; ++ptr )
-    {
-        switch ( *ptr )
-        {
-        case 0:
-            continue;
-        case '[':
-            if ( !slen )
-                break;
-            if ( match )
-                return NULL;
-            match = strncmp(++ptr, section, slen) == 0 && ptr[slen] == ']';
-            break;
-        default:
-            if ( match && strncmp(ptr, item, ilen) == 0 && ptr[ilen] == '=' )
-                return ptr + ilen + 1;
-            break;
-        }
-        ptr += strlen(ptr);
-    }
-    return NULL;
-}
 
 static void __init split_value(char *s)
 {
diff --git a/xen/arch/x86/efi/efi-shared.c b/xen/arch/x86/efi/efi-shared.c
index b9c563a..0f8618c 100644
--- a/xen/arch/x86/efi/efi-shared.c
+++ b/xen/arch/x86/efi/efi-shared.c
@@ -10,7 +10,7 @@
 #include <xen/ctype.h>
 #include <xen/init.h>
 #include <asm/processor.h>
-
+#include <xen/keyhandler.h>
 
 SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdOut;
 SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdErr;
@@ -120,3 +120,215 @@ bool_t __init match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2)
            guid1->Data3 == guid2->Data3 &&
            !memcmp(guid1->Data4, guid2->Data4, sizeof(guid1->Data4));
 }
+
+
+/* generic routine for printing error messages */
+void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode)
+{
+    StdOut = StdErr;
+    PrintErr((CHAR16 *)mesg);
+    PrintErr(L": ");
+
+    switch (ErrCode)
+    {
+    case EFI_NOT_FOUND:
+        mesg = L"Not found";
+        break;
+    case EFI_NO_MEDIA:
+        mesg = L"The device has no media";
+        break;
+    case EFI_MEDIA_CHANGED:
+        mesg = L"Media changed";
+        break;
+    case EFI_DEVICE_ERROR:
+        mesg = L"Device error";
+        break;
+    case EFI_VOLUME_CORRUPTED:
+        mesg = L"Volume corrupted";
+        break;
+    case EFI_ACCESS_DENIED:
+        mesg = L"Access denied";
+        break;
+    case EFI_OUT_OF_RESOURCES:
+        mesg = L"Out of resources";
+        break;
+    case EFI_VOLUME_FULL:
+        mesg = L"Volume is full";
+        break;
+    case EFI_SECURITY_VIOLATION:
+        mesg = L"Security violation";
+        break;
+    case EFI_CRC_ERROR:
+        mesg = L"CRC error";
+        break;
+    case EFI_COMPROMISED_DATA:
+        mesg = L"Compromised data";
+        break;
+    default:
+        PrintErr(L"ErrCode: ");
+        DisplayUint(ErrCode, 0);
+        mesg = NULL;
+        break;
+    }
+    blexit(mesg);
+}
+
+EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
+                                                CHAR16 **leaf)
+{
+    static EFI_GUID __initdata fs_protocol = SIMPLE_FILE_SYSTEM_PROTOCOL;
+    EFI_FILE_HANDLE dir_handle;
+    EFI_DEVICE_PATH *dp;
+    CHAR16 *pathend, *ptr;
+    EFI_STATUS ret;
+
+    do {
+        EFI_FILE_IO_INTERFACE *fio;
+
+        /* Get the file system interface. */
+        ret = efi_bs->HandleProtocol(loaded_image->DeviceHandle,
+                                     &fs_protocol, (void **)&fio);
+        if ( EFI_ERROR(ret) )
+            PrintErrMesg(L"Couldn't obtain the File System Protocol Interface",
+                         ret);
+        ret = fio->OpenVolume(fio, &dir_handle);
+    } while ( ret == EFI_MEDIA_CHANGED );
+    if ( ret != EFI_SUCCESS )
+        PrintErrMesg(L"OpenVolume failure", ret);
+
+#define buffer ((CHAR16 *)keyhandler_scratch)
+#define BUFFERSIZE sizeof(keyhandler_scratch)
+    for ( dp = loaded_image->FilePath, *buffer = 0;
+          DevicePathType(dp) != END_DEVICE_PATH_TYPE;
+          dp = (void *)dp + DevicePathNodeLength(dp) )
+    {
+        FILEPATH_DEVICE_PATH *fp;
+
+        if ( DevicePathType(dp) != MEDIA_DEVICE_PATH ||
+             DevicePathSubType(dp) != MEDIA_FILEPATH_DP )
+            blexit(L"Unsupported device path component");
+
+        if ( *buffer )
+        {
+            EFI_FILE_HANDLE new_handle;
+
+            ret = dir_handle->Open(dir_handle, &new_handle, buffer,
+                                   EFI_FILE_MODE_READ, 0);
+            if ( ret != EFI_SUCCESS )
+            {
+                PrintErr(L"Open failed for ");
+                PrintErrMesg(buffer, ret);
+            }
+            dir_handle->Close(dir_handle);
+            dir_handle = new_handle;
+        }
+        fp = (void *)dp;
+        if ( BUFFERSIZE < DevicePathNodeLength(dp) -
+                          sizeof(*dp) + sizeof(*buffer) )
+            blexit(L"Increase BUFFERSIZE");
+        memcpy(buffer, fp->PathName, DevicePathNodeLength(dp) - sizeof(*dp));
+        buffer[(DevicePathNodeLength(dp) - sizeof(*dp)) / sizeof(*buffer)] = 0;
+    }
+    for ( ptr = buffer, pathend = NULL; *ptr; ++ptr )
+        if ( *ptr == L'\\' )
+            pathend = ptr;
+    if ( pathend )
+    {
+        *pathend = 0;
+        *leaf = pathend + 1;
+        if ( *buffer )
+        {
+            EFI_FILE_HANDLE new_handle;
+
+            ret = dir_handle->Open(dir_handle, &new_handle, buffer,
+                                   EFI_FILE_MODE_READ, 0);
+            if ( ret != EFI_SUCCESS ) {
+                PrintErr(L"Open failed for ");
+                PrintErrMesg(buffer, ret);
+            }
+            dir_handle->Close(dir_handle);
+            dir_handle = new_handle;
+        }
+    }
+    else
+        *leaf = buffer;
+#undef BUFFERSIZE
+#undef buffer
+
+    return dir_handle;
+}
+
+CHAR16 *__init point_tail(CHAR16 *fn)
+{
+    CHAR16 *tail = NULL;
+
+    for ( ; ; ++fn )
+        switch ( *fn )
+        {
+        case 0:
+            return tail;
+        case L'.':
+        case L'-':
+        case L'_':
+            tail = fn;
+            break;
+        }
+}
+
+void __init pre_parse(const struct file *cfg)
+{
+    char *ptr = cfg->ptr, *end = ptr + cfg->size;
+    bool_t start = 1, comment = 0;
+
+    for ( ; ptr < end; ++ptr )
+    {
+        if ( iscntrl(*ptr) )
+        {
+            comment = 0;
+            start = 1;
+            *ptr = 0;
+        }
+        else if ( comment || (start && isspace(*ptr)) )
+            *ptr = 0;
+        else if ( *ptr == '#' || (start && *ptr == ';') )
+        {
+            comment = 1;
+            *ptr = 0;
+        }
+        else
+            start = 0;
+    }
+    if ( cfg->size && end[-1] )
+         PrintStr(L"No newline at end of config file,"
+                   " last line will be ignored.\r\n");
+}
+
+char *__init get_value(const struct file *cfg, const char *section,
+                              const char *item)
+{
+    char *ptr = cfg->ptr, *end = ptr + cfg->size;
+    size_t slen = section ? strlen(section) : 0, ilen = strlen(item);
+    bool_t match = !slen;
+
+    for ( ; ptr < end; ++ptr )
+    {
+        switch ( *ptr )
+        {
+        case 0:
+            continue;
+        case '[':
+            if ( !slen )
+                break;
+            if ( match )
+                return NULL;
+            match = strncmp(++ptr, section, slen) == 0 && ptr[slen] == ']';
+            break;
+        default:
+            if ( match && strncmp(ptr, item, ilen) == 0 && ptr[ilen] == '=' )
+                return ptr + ilen + 1;
+            break;
+        }
+        ptr += strlen(ptr);
+    }
+    return NULL;
+}
diff --git a/xen/include/efi/efi-shared.h b/xen/include/efi/efi-shared.h
index b4c7ac5..214a6ff 100644
--- a/xen/include/efi/efi-shared.h
+++ b/xen/include/efi/efi-shared.h
@@ -2,6 +2,7 @@
 #define __EFI_SHARED_H__
 
 #include <efi/efidef.h>
+#include <efi/efiprot.h>
 #include <xen/init.h>
 
 
@@ -36,4 +37,20 @@ CHAR16 *__init s2w(union string *str);
 char *__init w2s(const union string *str);
 bool_t __init match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2);
 
+void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode);
+
+EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
+                                                CHAR16 **leaf);
+CHAR16 *__init point_tail(CHAR16 *fn);
+
+
+void __init pre_parse(const struct file *cfg);
+char *__init get_value(const struct file *cfg, const char *section,
+                              const char *item);
+
+
+
+/* These functions need to be provided by the architecture's EFI code */
+void __init noreturn blexit(const CHAR16 *str);
+
 #endif
-- 
2.0.0

^ permalink raw reply related	[flat|nested] 44+ messages in thread

* [PATCH RFC 04/19] rename printErrMsg to PrintErrMesgExit
  2014-06-28  1:25 [PATCH RFC 00/19] arm64 EFI stub Roy Franz
                   ` (2 preceding siblings ...)
  2014-06-28  1:25 ` [PATCH RFC 03/19] Move more functions from boot.c to efi-shared.c Roy Franz
@ 2014-06-28  1:25 ` Roy Franz
  2014-06-28  1:25 ` [PATCH RFC 05/19] Add PrintErrMesg function that doesn't exit Roy Franz
                   ` (18 subsequent siblings)
  22 siblings, 0 replies; 44+ messages in thread
From: Roy Franz @ 2014-06-28  1:25 UTC (permalink / raw)
  To: xen-devel, ian.campbell, stefano.stabellini, tim, jbeulich, keir
  Cc: Roy Franz, fu.wei, linaro-uefi

The function prints an error message, then exits the program.  Update name
in preparation for adding a print-only version.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 xen/arch/x86/efi/boot.c       | 10 +++++-----
 xen/arch/x86/efi/efi-shared.c |  6 +++---
 xen/include/efi/efi-shared.h  |  2 +-
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/efi/boot.c b/xen/arch/x86/efi/boot.c
index 2d32d7b..89d7954 100644
--- a/xen/arch/x86/efi/boot.c
+++ b/xen/arch/x86/efi/boot.c
@@ -173,7 +173,7 @@ static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
     CHAR16 *what = NULL;
 
     if ( !name )
-        PrintErrMesg(L"No filename", EFI_OUT_OF_RESOURCES);
+        PrintErrMesgExit(L"No filename", EFI_OUT_OF_RESOURCES);
     ret = dir_handle->Open(dir_handle, &FileHandle, name,
                            EFI_FILE_MODE_READ, 0);
     if ( file == &cfg && ret == EFI_NOT_FOUND )
@@ -234,7 +234,7 @@ static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
     {
         PrintErr(what);
         PrintErr(L" failed for ");
-        PrintErrMesg(name, ret);
+        PrintErrMesgExit(name, ret);
     }
 
     return 1;
@@ -256,7 +256,7 @@ static void __init edd_put_string(u8 *dst, size_t n, const char *src)
     while ( n-- && *src )
        *dst++ = *src++;
     if ( *src )
-       PrintErrMesg(L"Internal error populating EDD info",
+       PrintErrMesgExit(L"Internal error populating EDD info",
                     EFI_BUFFER_TOO_SMALL);
     while ( n-- )
        *dst++ = ' ';
@@ -478,7 +478,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     status = efi_bs->HandleProtocol(ImageHandle, &loaded_image_guid,
                                     (void **)&loaded_image);
     if ( status != EFI_SUCCESS )
-        PrintErrMesg(L"No Loaded Image Protocol", status);
+        PrintErrMesgExit(L"No Loaded Image Protocol", status);
 
     xen_phys_start = (UINTN)loaded_image->ImageBase;
     if ( (xen_phys_start + loaded_image->ImageSize - 1) >> 32 )
@@ -1127,7 +1127,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
 
     status = efi_bs->ExitBootServices(ImageHandle, map_key);
     if ( EFI_ERROR(status) )
-        PrintErrMesg(L"Cannot exit boot services", status);
+        PrintErrMesgExit(L"Cannot exit boot services", status);
 
     /* Adjust pointers into EFI. */
     efi_ct = (void *)efi_ct + DIRECTMAP_VIRT_START;
diff --git a/xen/arch/x86/efi/efi-shared.c b/xen/arch/x86/efi/efi-shared.c
index 0f8618c..ad668cb 100644
--- a/xen/arch/x86/efi/efi-shared.c
+++ b/xen/arch/x86/efi/efi-shared.c
@@ -123,7 +123,7 @@ bool_t __init match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2)
 
 
 /* generic routine for printing error messages */
-void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode)
+void __init PrintErrMesgExit(const CHAR16 *mesg, EFI_STATUS ErrCode)
 {
     StdOut = StdErr;
     PrintErr((CHAR16 *)mesg);
@@ -217,7 +217,7 @@ EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
             if ( ret != EFI_SUCCESS )
             {
                 PrintErr(L"Open failed for ");
-                PrintErrMesg(buffer, ret);
+                PrintErrMesgExit(buffer, ret);
             }
             dir_handle->Close(dir_handle);
             dir_handle = new_handle;
@@ -244,7 +244,7 @@ EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
                                    EFI_FILE_MODE_READ, 0);
             if ( ret != EFI_SUCCESS ) {
                 PrintErr(L"Open failed for ");
-                PrintErrMesg(buffer, ret);
+                PrintErrMesgExit(buffer, ret);
             }
             dir_handle->Close(dir_handle);
             dir_handle = new_handle;
diff --git a/xen/include/efi/efi-shared.h b/xen/include/efi/efi-shared.h
index 214a6ff..30a41e4 100644
--- a/xen/include/efi/efi-shared.h
+++ b/xen/include/efi/efi-shared.h
@@ -37,7 +37,7 @@ CHAR16 *__init s2w(union string *str);
 char *__init w2s(const union string *str);
 bool_t __init match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2);
 
-void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode);
+void __init PrintErrMesgExit(const CHAR16 *mesg, EFI_STATUS ErrCode);
 
 EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
                                                 CHAR16 **leaf);
-- 
2.0.0

^ permalink raw reply related	[flat|nested] 44+ messages in thread

* [PATCH RFC 05/19] Add PrintErrMesg function that doesn't exit
  2014-06-28  1:25 [PATCH RFC 00/19] arm64 EFI stub Roy Franz
                   ` (3 preceding siblings ...)
  2014-06-28  1:25 ` [PATCH RFC 04/19] rename printErrMsg to PrintErrMesgExit Roy Franz
@ 2014-06-28  1:25 ` Roy Franz
  2014-06-28  1:25 ` [PATCH RFC 06/19] Refactor read_file() so it can be shared Roy Franz
                   ` (17 subsequent siblings)
  22 siblings, 0 replies; 44+ messages in thread
From: Roy Franz @ 2014-06-28  1:25 UTC (permalink / raw)
  To: xen-devel, ian.campbell, stefano.stabellini, tim, jbeulich, keir
  Cc: Roy Franz, fu.wei, linaro-uefi

Add the PrintErrMesg() function that just prints the error information,
but does not exit the program. PrintErrMesgExit() is updated to be composed
of a call to PrintErrMesg() and blexit().

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 xen/arch/x86/efi/efi-shared.c | 8 +++++++-
 xen/include/efi/efi-shared.h  | 1 +
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/efi/efi-shared.c b/xen/arch/x86/efi/efi-shared.c
index ad668cb..d997b5c 100644
--- a/xen/arch/x86/efi/efi-shared.c
+++ b/xen/arch/x86/efi/efi-shared.c
@@ -123,7 +123,7 @@ bool_t __init match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2)
 
 
 /* generic routine for printing error messages */
-void __init PrintErrMesgExit(const CHAR16 *mesg, EFI_STATUS ErrCode)
+void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode)
 {
     StdOut = StdErr;
     PrintErr((CHAR16 *)mesg);
@@ -170,6 +170,12 @@ void __init PrintErrMesgExit(const CHAR16 *mesg, EFI_STATUS ErrCode)
         mesg = NULL;
         break;
     }
+}
+
+
+void __init PrintErrMesgExit(const CHAR16 *mesg, EFI_STATUS ErrCode)
+{
+    PrintErrMesg(mesg, ErrCode);
     blexit(mesg);
 }
 
diff --git a/xen/include/efi/efi-shared.h b/xen/include/efi/efi-shared.h
index 30a41e4..d4b6e13 100644
--- a/xen/include/efi/efi-shared.h
+++ b/xen/include/efi/efi-shared.h
@@ -38,6 +38,7 @@ char *__init w2s(const union string *str);
 bool_t __init match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2);
 
 void __init PrintErrMesgExit(const CHAR16 *mesg, EFI_STATUS ErrCode);
+void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode);
 
 EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
                                                 CHAR16 **leaf);
-- 
2.0.0

^ permalink raw reply related	[flat|nested] 44+ messages in thread

* [PATCH RFC 06/19] Refactor read_file() so it can be shared.
  2014-06-28  1:25 [PATCH RFC 00/19] arm64 EFI stub Roy Franz
                   ` (4 preceding siblings ...)
  2014-06-28  1:25 ` [PATCH RFC 05/19] Add PrintErrMesg function that doesn't exit Roy Franz
@ 2014-06-28  1:25 ` Roy Franz
  2014-06-28  1:25 ` [PATCH RFC 07/19] move read_file() to efi-shared.c Roy Franz
                   ` (16 subsequent siblings)
  22 siblings, 0 replies; 44+ messages in thread
From: Roy Franz @ 2014-06-28  1:25 UTC (permalink / raw)
  To: xen-devel, ian.campbell, stefano.stabellini, tim, jbeulich, keir
  Cc: Roy Franz, fu.wei, linaro-uefi

The read_file() function updated some multiboot specific data structures
as it was loading a file.  These changes make read_file() more generic,
and create a load_file() wrapper for x86 that updates the multiboot
data structures.  read_file() no longer does special handling of
the configuration file, as this was only needed to avoid adding
it to the multiboot structures.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 xen/arch/x86/efi/boot.c | 58 +++++++++++++++++++++++++++++--------------------
 1 file changed, 34 insertions(+), 24 deletions(-)

diff --git a/xen/arch/x86/efi/boot.c b/xen/arch/x86/efi/boot.c
index 89d7954..8904a4e 100644
--- a/xen/arch/x86/efi/boot.c
+++ b/xen/arch/x86/efi/boot.c
@@ -173,11 +173,11 @@ static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
     CHAR16 *what = NULL;
 
     if ( !name )
-        PrintErrMesgExit(L"No filename", EFI_OUT_OF_RESOURCES);
+        PrintErrMesgExit(L"No Filename", EFI_OUT_OF_RESOURCES);
+
     ret = dir_handle->Open(dir_handle, &FileHandle, name,
                            EFI_FILE_MODE_READ, 0);
-    if ( file == &cfg && ret == EFI_NOT_FOUND )
-        return 0;
+
     if ( EFI_ERROR(ret) )
         what = L"Open";
     else
@@ -206,40 +206,50 @@ static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
     }
     else
     {
-        if ( file != &cfg )
-        {
-            PrintStr(name);
-            PrintStr(L": ");
-            DisplayUint(file->addr, 2 * sizeof(file->addr));
-            PrintStr(L"-");
-            DisplayUint(file->addr + size, 2 * sizeof(file->addr));
-            PrintStr(newline);
-            mb_modules[mbi.mods_count].mod_start = file->addr >> PAGE_SHIFT;
-            mb_modules[mbi.mods_count].mod_end = size;
-            ++mbi.mods_count;
-        }
 
         file->size = size;
         ret = FileHandle->Read(FileHandle, &file->size, file->ptr);
         if ( !EFI_ERROR(ret) && file->size != size )
             ret = EFI_ABORTED;
         if ( EFI_ERROR(ret) )
-            what = L"Read";
+        {
+            what = what ?: L"Read";
+            efi_bs->FreePages(file->addr, PFN_UP(file->size));
+            file->addr = 0;
+        }
     }
 
     if ( FileHandle )
         FileHandle->Close(FileHandle);
 
+
     if ( what )
     {
-        PrintErr(what);
-        PrintErr(L" failed for ");
-        PrintErrMesgExit(name, ret);
+        PrintErrMesg(what, ret);
+        blexit(L"Unable to load file");
+    }
+    else
+    {
+        PrintStr(name);
+        PrintStr(L": ");
+        DisplayUint(file->addr, 2 * sizeof(file->addr));
+        PrintStr(L"-");
+        DisplayUint(file->addr + file->size, 2 * sizeof(file->addr));
+        PrintStr(newline);
+        return 1;
     }
 
-    return 1;
 }
 
+/* Only call with non-config files. */
+void __init load_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
+                               struct file *file)
+{
+    read_file(dir_handle, name, file);
+    mb_modules[mbi.mods_count].mod_start = file->addr >> PAGE_SHIFT;
+    mb_modules[mbi.mods_count].mod_end = file->size;
+    ++mbi.mods_count;
+}
 
 static void __init split_value(char *s)
 {
@@ -640,7 +650,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     if ( !name.s )
         blexit(L"No Dom0 kernel image specified.");
     split_value(name.s);
-    read_file(dir_handle, s2w(&name), &kernel);
+    load_file(dir_handle, s2w(&name), &kernel);
     efi_bs->FreePool(name.w);
 
     if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
@@ -652,7 +662,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     if ( name.s )
     {
         split_value(name.s);
-        read_file(dir_handle, s2w(&name), &ramdisk);
+        load_file(dir_handle, s2w(&name), &ramdisk);
         efi_bs->FreePool(name.w);
     }
 
@@ -663,7 +673,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     {
         microcode_set_module(mbi.mods_count);
         split_value(name.s);
-        read_file(dir_handle, s2w(&name), &ucode);
+        load_file(dir_handle, s2w(&name), &ucode);
         efi_bs->FreePool(name.w);
     }
 
@@ -671,7 +681,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     if ( name.s )
     {
         split_value(name.s);
-        read_file(dir_handle, s2w(&name), &xsm);
+        load_file(dir_handle, s2w(&name), &xsm);
         efi_bs->FreePool(name.w);
     }
 
-- 
2.0.0

^ permalink raw reply related	[flat|nested] 44+ messages in thread

* [PATCH RFC 07/19] move read_file() to efi-shared.c
  2014-06-28  1:25 [PATCH RFC 00/19] arm64 EFI stub Roy Franz
                   ` (5 preceding siblings ...)
  2014-06-28  1:25 ` [PATCH RFC 06/19] Refactor read_file() so it can be shared Roy Franz
@ 2014-06-28  1:25 ` Roy Franz
  2014-06-28  1:25 ` [PATCH RFC 08/19] Move removal of leading spaces from split_value to get_value() Roy Franz
                   ` (15 subsequent siblings)
  22 siblings, 0 replies; 44+ messages in thread
From: Roy Franz @ 2014-06-28  1:25 UTC (permalink / raw)
  To: xen-devel, ian.campbell, stefano.stabellini, tim, jbeulich, keir
  Cc: Roy Franz, fu.wei, linaro-uefi

The now refactored read_file() is shareable with the ARM code that will be
added, so move it to the shared C file.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 xen/arch/x86/efi/boot.c       | 77 ---------------------------------------
 xen/arch/x86/efi/efi-shared.c | 83 +++++++++++++++++++++++++++++++++++++++++++
 xen/include/efi/efi-shared.h  |  2 ++
 3 files changed, 85 insertions(+), 77 deletions(-)

diff --git a/xen/arch/x86/efi/boot.c b/xen/arch/x86/efi/boot.c
index 8904a4e..2e3d341 100644
--- a/xen/arch/x86/efi/boot.c
+++ b/xen/arch/x86/efi/boot.c
@@ -164,83 +164,6 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
     return argc;
 }
 
-static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
-                               struct file *file)
-{
-    EFI_FILE_HANDLE FileHandle = NULL;
-    UINT64 size;
-    EFI_STATUS ret;
-    CHAR16 *what = NULL;
-
-    if ( !name )
-        PrintErrMesgExit(L"No Filename", EFI_OUT_OF_RESOURCES);
-
-    ret = dir_handle->Open(dir_handle, &FileHandle, name,
-                           EFI_FILE_MODE_READ, 0);
-
-    if ( EFI_ERROR(ret) )
-        what = L"Open";
-    else
-        ret = FileHandle->SetPosition(FileHandle, -1);
-    if ( EFI_ERROR(ret) )
-        what = what ?: L"Seek";
-    else
-        ret = FileHandle->GetPosition(FileHandle, &size);
-    if ( EFI_ERROR(ret) )
-        what = what ?: L"Get size";
-    else
-        ret = FileHandle->SetPosition(FileHandle, 0);
-    if ( EFI_ERROR(ret) )
-        what = what ?: L"Seek";
-    else
-    {
-        file->addr = min(1UL << (32 + PAGE_SHIFT),
-                         HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
-        ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
-                                    PFN_UP(size), &file->addr);
-    }
-    if ( EFI_ERROR(ret) )
-    {
-        file->addr = 0;
-        what = what ?: L"Allocation";
-    }
-    else
-    {
-
-        file->size = size;
-        ret = FileHandle->Read(FileHandle, &file->size, file->ptr);
-        if ( !EFI_ERROR(ret) && file->size != size )
-            ret = EFI_ABORTED;
-        if ( EFI_ERROR(ret) )
-        {
-            what = what ?: L"Read";
-            efi_bs->FreePages(file->addr, PFN_UP(file->size));
-            file->addr = 0;
-        }
-    }
-
-    if ( FileHandle )
-        FileHandle->Close(FileHandle);
-
-
-    if ( what )
-    {
-        PrintErrMesg(what, ret);
-        blexit(L"Unable to load file");
-    }
-    else
-    {
-        PrintStr(name);
-        PrintStr(L": ");
-        DisplayUint(file->addr, 2 * sizeof(file->addr));
-        PrintStr(L"-");
-        DisplayUint(file->addr + file->size, 2 * sizeof(file->addr));
-        PrintStr(newline);
-        return 1;
-    }
-
-}
-
 /* Only call with non-config files. */
 void __init load_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
                                struct file *file)
diff --git a/xen/arch/x86/efi/efi-shared.c b/xen/arch/x86/efi/efi-shared.c
index d997b5c..98ad5db 100644
--- a/xen/arch/x86/efi/efi-shared.c
+++ b/xen/arch/x86/efi/efi-shared.c
@@ -11,6 +11,12 @@
 #include <xen/init.h>
 #include <asm/processor.h>
 #include <xen/keyhandler.h>
+#include <xen/pfn.h>
+#include <xen/kernel.h>
+#if EFI_PAGE_SIZE != PAGE_SIZE
+# error Cannot use xen/pfn.h here!
+#endif
+
 
 SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdOut;
 SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdErr;
@@ -338,3 +344,80 @@ char *__init get_value(const struct file *cfg, const char *section,
     }
     return NULL;
 }
+
+bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
+                               struct file *file)
+{
+    EFI_FILE_HANDLE FileHandle = NULL;
+    UINT64 size;
+    EFI_STATUS ret;
+    CHAR16 *what = NULL;
+
+    if ( !name )
+        PrintErrMesgExit(L"No Filename", EFI_OUT_OF_RESOURCES);
+
+    ret = dir_handle->Open(dir_handle, &FileHandle, name,
+                           EFI_FILE_MODE_READ, 0);
+
+    if ( EFI_ERROR(ret) )
+        what = L"Open";
+    else
+        ret = FileHandle->SetPosition(FileHandle, -1);
+    if ( EFI_ERROR(ret) )
+        what = what ?: L"Seek";
+    else
+        ret = FileHandle->GetPosition(FileHandle, &size);
+    if ( EFI_ERROR(ret) )
+        what = what ?: L"Get size";
+    else
+        ret = FileHandle->SetPosition(FileHandle, 0);
+    if ( EFI_ERROR(ret) )
+        what = what ?: L"Seek";
+    else
+    {
+        file->addr = min(1UL << (32 + PAGE_SHIFT),
+                         HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
+        ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
+                                    PFN_UP(size), &file->addr);
+    }
+    if ( EFI_ERROR(ret) )
+    {
+        file->addr = 0;
+        what = what ?: L"Allocation";
+    }
+    else
+    {
+
+        file->size = size;
+        ret = FileHandle->Read(FileHandle, &file->size, file->ptr);
+        if ( !EFI_ERROR(ret) && file->size != size )
+            ret = EFI_ABORTED;
+        if ( EFI_ERROR(ret) )
+        {
+            what = what ?: L"Read";
+            efi_bs->FreePages(file->addr, PFN_UP(file->size));
+            file->addr = 0;
+        }
+    }
+
+    if ( FileHandle )
+        FileHandle->Close(FileHandle);
+
+
+    if ( what )
+    {
+        PrintErrMesg(what, ret);
+        blexit(L"Unable to load file");
+    }
+    else
+    {
+        PrintStr(name);
+        PrintStr(L": ");
+        DisplayUint(file->addr, 2 * sizeof(file->addr));
+        PrintStr(L"-");
+        DisplayUint(file->addr + file->size, 2 * sizeof(file->addr));
+        PrintStr(newline);
+        return 1;
+    }
+
+}
diff --git a/xen/include/efi/efi-shared.h b/xen/include/efi/efi-shared.h
index d4b6e13..168cf2a 100644
--- a/xen/include/efi/efi-shared.h
+++ b/xen/include/efi/efi-shared.h
@@ -54,4 +54,6 @@ char *__init get_value(const struct file *cfg, const char *section,
 /* These functions need to be provided by the architecture's EFI code */
 void __init noreturn blexit(const CHAR16 *str);
 
+bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
+                               struct file *file);
 #endif
-- 
2.0.0

^ permalink raw reply related	[flat|nested] 44+ messages in thread

* [PATCH RFC 08/19] Move removal of leading spaces from split_value to get_value()
  2014-06-28  1:25 [PATCH RFC 00/19] arm64 EFI stub Roy Franz
                   ` (6 preceding siblings ...)
  2014-06-28  1:25 ` [PATCH RFC 07/19] move read_file() to efi-shared.c Roy Franz
@ 2014-06-28  1:25 ` Roy Franz
  2014-06-28  1:25 ` [PATCH RFC 09/19] replace split_value() with truncate_string() Roy Franz
                   ` (14 subsequent siblings)
  22 siblings, 0 replies; 44+ messages in thread
From: Roy Franz @ 2014-06-28  1:25 UTC (permalink / raw)
  To: xen-devel, ian.campbell, stefano.stabellini, tim, jbeulich, keir
  Cc: Roy Franz, fu.wei, linaro-uefi

Move the removal of leading whitespace from values read from the
configuration file to the code that reads them from the config file.
This allows more sharing of string related functions.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 xen/arch/x86/efi/boot.c       | 2 --
 xen/arch/x86/efi/efi-shared.c | 8 +++++++-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/efi/boot.c b/xen/arch/x86/efi/boot.c
index 2e3d341..045714b 100644
--- a/xen/arch/x86/efi/boot.c
+++ b/xen/arch/x86/efi/boot.c
@@ -176,8 +176,6 @@ void __init load_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
 
 static void __init split_value(char *s)
 {
-    while ( *s && isspace(*s) )
-        ++s;
     place_string(&mb_modules[mbi.mods_count].string, s);
     while ( *s && !isspace(*s) )
         ++s;
diff --git a/xen/arch/x86/efi/efi-shared.c b/xen/arch/x86/efi/efi-shared.c
index 98ad5db..0a67363 100644
--- a/xen/arch/x86/efi/efi-shared.c
+++ b/xen/arch/x86/efi/efi-shared.c
@@ -337,7 +337,13 @@ char *__init get_value(const struct file *cfg, const char *section,
             break;
         default:
             if ( match && strncmp(ptr, item, ilen) == 0 && ptr[ilen] == '=' )
-                return ptr + ilen + 1;
+            {
+                ptr += ilen + 1;
+                /* strip off any leading spaces */
+                while ( *ptr && isspace(*ptr) )
+                    ptr++;
+                return ptr;
+            }
             break;
         }
         ptr += strlen(ptr);
-- 
2.0.0

^ permalink raw reply related	[flat|nested] 44+ messages in thread

* [PATCH RFC 09/19] replace split_value() with truncate_string()
  2014-06-28  1:25 [PATCH RFC 00/19] arm64 EFI stub Roy Franz
                   ` (7 preceding siblings ...)
  2014-06-28  1:25 ` [PATCH RFC 08/19] Move removal of leading spaces from split_value to get_value() Roy Franz
@ 2014-06-28  1:25 ` Roy Franz
  2014-06-28  1:25 ` [PATCH RFC 10/19] move truncate_string() to efi-shared.c Roy Franz
                   ` (13 subsequent siblings)
  22 siblings, 0 replies; 44+ messages in thread
From: Roy Franz @ 2014-06-28  1:25 UTC (permalink / raw)
  To: xen-devel, ian.campbell, stefano.stabellini, tim, jbeulich, keir
  Cc: Roy Franz, fu.wei, linaro-uefi

Replace the split_value() function with a more generic string handling
function truncate_string().  split_value() used to update the multiboot
structures directly, and this has been moved to the call sites to allow
truncate_string() to be more generic.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 xen/arch/x86/efi/boot.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/efi/boot.c b/xen/arch/x86/efi/boot.c
index 045714b..e61f9c9 100644
--- a/xen/arch/x86/efi/boot.c
+++ b/xen/arch/x86/efi/boot.c
@@ -174,12 +174,19 @@ void __init load_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
     ++mbi.mods_count;
 }
 
-static void __init split_value(char *s)
+/* Truncate string at first space, and return pointer
+ * to remainder of string.
+ */
+char * __init truncate_string(char *s)
 {
-    place_string(&mb_modules[mbi.mods_count].string, s);
     while ( *s && !isspace(*s) )
         ++s;
-    *s = 0;
+    if (*s)
+    {
+        *s = 0;
+        return(s + 1);
+    }
+    return(NULL);
 }
 
 static void __init edd_put_string(u8 *dst, size_t n, const char *src)
@@ -570,7 +577,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     }
     if ( !name.s )
         blexit(L"No Dom0 kernel image specified.");
-    split_value(name.s);
+    place_string(&mb_modules[mbi.mods_count].string, name.s);
+    truncate_string(name.s);
     load_file(dir_handle, s2w(&name), &kernel);
     efi_bs->FreePool(name.w);
 
@@ -582,7 +590,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     name.s = get_value(&cfg, section.s, "ramdisk");
     if ( name.s )
     {
-        split_value(name.s);
+        place_string(&mb_modules[mbi.mods_count].string, name.s);
+        truncate_string(name.s);
         load_file(dir_handle, s2w(&name), &ramdisk);
         efi_bs->FreePool(name.w);
     }
@@ -593,7 +602,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     if ( name.s )
     {
         microcode_set_module(mbi.mods_count);
-        split_value(name.s);
+        place_string(&mb_modules[mbi.mods_count].string, name.s);
+        truncate_string(name.s);
         load_file(dir_handle, s2w(&name), &ucode);
         efi_bs->FreePool(name.w);
     }
@@ -601,7 +611,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     name.s = get_value(&cfg, section.s, "xsm");
     if ( name.s )
     {
-        split_value(name.s);
+        place_string(&mb_modules[mbi.mods_count].string, name.s);
+        truncate_string(name.s);
         load_file(dir_handle, s2w(&name), &xsm);
         efi_bs->FreePool(name.w);
     }
-- 
2.0.0

^ permalink raw reply related	[flat|nested] 44+ messages in thread

* [PATCH RFC 10/19] move truncate_string() to efi-shared.c
  2014-06-28  1:25 [PATCH RFC 00/19] arm64 EFI stub Roy Franz
                   ` (8 preceding siblings ...)
  2014-06-28  1:25 ` [PATCH RFC 09/19] replace split_value() with truncate_string() Roy Franz
@ 2014-06-28  1:25 ` Roy Franz
  2014-06-28  1:25 ` [PATCH RFC 11/19] add read_config_file() function for XEN EFI config file Roy Franz
                   ` (12 subsequent siblings)
  22 siblings, 0 replies; 44+ messages in thread
From: Roy Franz @ 2014-06-28  1:25 UTC (permalink / raw)
  To: xen-devel, ian.campbell, stefano.stabellini, tim, jbeulich, keir
  Cc: Roy Franz, fu.wei, linaro-uefi

Move the newly created truncate_string() function to efi-shared.c so it can
be shared with the ARM EFI stub.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 xen/arch/x86/efi/boot.c       | 14 --------------
 xen/arch/x86/efi/efi-shared.c | 15 +++++++++++++++
 xen/include/efi/efi-shared.h  |  1 +
 3 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/efi/boot.c b/xen/arch/x86/efi/boot.c
index e61f9c9..0583c6a 100644
--- a/xen/arch/x86/efi/boot.c
+++ b/xen/arch/x86/efi/boot.c
@@ -174,20 +174,6 @@ void __init load_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
     ++mbi.mods_count;
 }
 
-/* Truncate string at first space, and return pointer
- * to remainder of string.
- */
-char * __init truncate_string(char *s)
-{
-    while ( *s && !isspace(*s) )
-        ++s;
-    if (*s)
-    {
-        *s = 0;
-        return(s + 1);
-    }
-    return(NULL);
-}
 
 static void __init edd_put_string(u8 *dst, size_t n, const char *src)
 {
diff --git a/xen/arch/x86/efi/efi-shared.c b/xen/arch/x86/efi/efi-shared.c
index 0a67363..6abbc88 100644
--- a/xen/arch/x86/efi/efi-shared.c
+++ b/xen/arch/x86/efi/efi-shared.c
@@ -185,6 +185,21 @@ void __init PrintErrMesgExit(const CHAR16 *mesg, EFI_STATUS ErrCode)
     blexit(mesg);
 }
 
+/* Truncate string at first space, and return pointer
+ * to remainder of string.
+ */
+char * __init truncate_string(char *s)
+{
+    while ( *s && !isspace(*s) )
+        ++s;
+    if (*s)
+    {
+        *s = 0;
+        return(s + 1);
+    }
+    return(NULL);
+}
+
 EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
                                                 CHAR16 **leaf)
 {
diff --git a/xen/include/efi/efi-shared.h b/xen/include/efi/efi-shared.h
index 168cf2a..98e9e89 100644
--- a/xen/include/efi/efi-shared.h
+++ b/xen/include/efi/efi-shared.h
@@ -56,4 +56,5 @@ void __init noreturn blexit(const CHAR16 *str);
 
 bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
                                struct file *file);
+char * __init truncate_string(char *s);
 #endif
-- 
2.0.0

^ permalink raw reply related	[flat|nested] 44+ messages in thread

* [PATCH RFC 11/19] add read_config_file() function for XEN EFI config file
  2014-06-28  1:25 [PATCH RFC 00/19] arm64 EFI stub Roy Franz
                   ` (9 preceding siblings ...)
  2014-06-28  1:25 ` [PATCH RFC 10/19] move truncate_string() to efi-shared.c Roy Franz
@ 2014-06-28  1:25 ` Roy Franz
  2014-06-28  1:25 ` [PATCH RFC 12/19] create handle_cmdline() function Roy Franz
                   ` (11 subsequent siblings)
  22 siblings, 0 replies; 44+ messages in thread
From: Roy Franz @ 2014-06-28  1:25 UTC (permalink / raw)
  To: xen-devel, ian.campbell, stefano.stabellini, tim, jbeulich, keir
  Cc: Roy Franz, fu.wei, linaro-uefi

Move open-coded reading of the XEN EFI configuration file into a shared
fuction read_config_file().  The open-coded version returned the dom0
kernel name in a global variable.  This has been replaced by an explicit
lookup of the dom0 kernel name after the initial processing of the config
file has completed.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 xen/arch/x86/efi/boot.c       | 49 +++------------------------------
 xen/arch/x86/efi/efi-shared.c | 63 +++++++++++++++++++++++++++++++++++++++++++
 xen/include/efi/efi-shared.h  |  4 +++
 3 files changed, 70 insertions(+), 46 deletions(-)

diff --git a/xen/arch/x86/efi/boot.c b/xen/arch/x86/efi/boot.c
index 0583c6a..912b6de 100644
--- a/xen/arch/x86/efi/boot.c
+++ b/xen/arch/x86/efi/boot.c
@@ -514,53 +514,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     if ( EFI_ERROR(status) )
         gop = NULL;
 
-    /* Read and parse the config file. */
-    if ( !cfg_file_name )
-    {
-        CHAR16 *tail;
+    read_config_file(&dir_handle, &cfg, cfg_file_name,
+                     &section, file_name);
 
-        while ( (tail = point_tail(file_name)) != NULL )
-        {
-            wstrcpy(tail, L".cfg");
-            if ( read_file(dir_handle, file_name, &cfg) )
-                break;
-            *tail = 0;
-        }
-        if ( !tail )
-            blexit(L"No configuration file found.");
-        PrintStr(L"Using configuration file '");
-        PrintStr(file_name);
-        PrintStr(L"'\r\n");
-    }
-    else if ( !read_file(dir_handle, cfg_file_name, &cfg) )
-        blexit(L"Configuration file not found.");
-    pre_parse(&cfg);
-
-    if ( section.w )
-        w2s(&section);
-    else
-        section.s = get_value(&cfg, "global", "default");
-
-    for ( ; ; )
-    {
-        name.s = get_value(&cfg, section.s, "kernel");
-        if ( name.s )
-            break;
-        name.s = get_value(&cfg, "global", "chain");
-        if ( !name.s )
-            break;
-        efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
-        cfg.addr = 0;
-        if ( !read_file(dir_handle, s2w(&name), &cfg) )
-        {
-            PrintStr(L"Chained configuration file '");
-            PrintStr(name.w);
-            efi_bs->FreePool(name.w);
-            blexit(L"'not found.");
-        }
-        pre_parse(&cfg);
-        efi_bs->FreePool(name.w);
-    }
+    name.s = get_value(&cfg, section.s, "kernel");
     if ( !name.s )
         blexit(L"No Dom0 kernel image specified.");
     place_string(&mb_modules[mbi.mods_count].string, name.s);
diff --git a/xen/arch/x86/efi/efi-shared.c b/xen/arch/x86/efi/efi-shared.c
index 6abbc88..43601fe 100644
--- a/xen/arch/x86/efi/efi-shared.c
+++ b/xen/arch/x86/efi/efi-shared.c
@@ -200,6 +200,69 @@ char * __init truncate_string(char *s)
     return(NULL);
 }
 
+
+
+void __init read_config_file(EFI_FILE_HANDLE *cfg_dir_handle,
+                             struct file *cfg, CHAR16 *cfg_file_name,
+                             union string *section,
+                             CHAR16 *xen_file_name)
+{
+
+    /* Read and parse the config file. */
+    if ( !cfg_file_name )
+    {
+        CHAR16 *tail;
+
+        while ( (tail = point_tail(xen_file_name)) != NULL )
+        {
+            wstrcpy(tail, L".cfg");
+            if ( read_file(*cfg_dir_handle, xen_file_name, cfg) )
+                break;
+            *tail = 0;
+        }
+        if ( !tail )
+            blexit(L"No configuration file found.");
+        PrintStr(L"Using configuration file '");
+        PrintStr(xen_file_name);
+        PrintStr(L"'\r\n");
+    }
+    else if ( !read_file(*cfg_dir_handle, cfg_file_name, cfg) )
+        blexit(L"Configuration file not found.");
+    pre_parse(cfg);
+
+    if ( section->w )
+        w2s(section);
+    else
+        section->s = get_value(cfg, "global", "default");
+
+
+    for ( ; ; )
+    {
+        union string dom0_kernel_name;
+        // RFRANZ_TODO - would like to get rid of this kernel lookup,
+        // but can't as it allows both kernel _and_ chain to be specified
+        // with kernel overriding chain.
+        // !! different sections....
+        dom0_kernel_name.s = get_value(cfg, section->s, "kernel");
+        if ( dom0_kernel_name.s )
+            break;
+        dom0_kernel_name.s = get_value(cfg, "global", "chain");
+        if ( !dom0_kernel_name.s )
+            break;
+        efi_bs->FreePages(cfg->addr, PFN_UP(cfg->size));
+        cfg->addr = 0;
+        if ( !read_file(*cfg_dir_handle, s2w(&dom0_kernel_name), cfg) )
+        {
+            PrintStr(L"Chained configuration file '");
+            PrintStr(dom0_kernel_name.w);
+            efi_bs->FreePool(dom0_kernel_name.w);
+            blexit(L"'not found.");
+        }
+        pre_parse(cfg);
+        efi_bs->FreePool(dom0_kernel_name.w);
+    }
+}
+
 EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
                                                 CHAR16 **leaf)
 {
diff --git a/xen/include/efi/efi-shared.h b/xen/include/efi/efi-shared.h
index 98e9e89..a1028d1 100644
--- a/xen/include/efi/efi-shared.h
+++ b/xen/include/efi/efi-shared.h
@@ -57,4 +57,8 @@ void __init noreturn blexit(const CHAR16 *str);
 bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
                                struct file *file);
 char * __init truncate_string(char *s);
+void __init read_config_file(EFI_FILE_HANDLE *cfg_dir_handle,
+                             struct file *cfg, CHAR16 *cfg_file_name,
+                             union string *section,
+                             CHAR16 *xen_file_name);
 #endif
-- 
2.0.0

^ permalink raw reply related	[flat|nested] 44+ messages in thread

* [PATCH RFC 12/19] create handle_cmdline() function
  2014-06-28  1:25 [PATCH RFC 00/19] arm64 EFI stub Roy Franz
                   ` (10 preceding siblings ...)
  2014-06-28  1:25 ` [PATCH RFC 11/19] add read_config_file() function for XEN EFI config file Roy Franz
@ 2014-06-28  1:25 ` Roy Franz
  2014-06-28  1:25 ` [PATCH RFC 13/19] Refactor get_argv() for sharing Roy Franz
                   ` (10 subsequent siblings)
  22 siblings, 0 replies; 44+ messages in thread
From: Roy Franz @ 2014-06-28  1:25 UTC (permalink / raw)
  To: xen-devel, ian.campbell, stefano.stabellini, tim, jbeulich, keir
  Cc: Roy Franz, fu.wei, linaro-uefi

Create handle_cmdline() function in preparation for sharing to allow x86 and
ARM architectures to share the command line processing.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 xen/arch/x86/efi/boot.c | 126 ++++++++++++++++++++++++++++++------------------
 1 file changed, 79 insertions(+), 47 deletions(-)

diff --git a/xen/arch/x86/efi/boot.c b/xen/arch/x86/efi/boot.c
index 912b6de..6ee3c1f 100644
--- a/xen/arch/x86/efi/boot.c
+++ b/xen/arch/x86/efi/boot.c
@@ -345,6 +345,74 @@ static void __init relocate_image(unsigned long delta)
     }
 }
 
+
+void __init handle_cmdline(EFI_LOADED_IMAGE *loaded_image,
+                           CHAR16 **cfg_file_name, bool_t *base_video,
+                           CHAR16 **image_name, CHAR16 **section_name)
+{
+
+    unsigned int i, argc;
+    CHAR16 **argv;
+
+
+    if ( !cfg_file_name || !base_video || !image_name )
+    {
+        PrintStr(L"Invalid args to handle_cmdline\r\n");
+        blexit(NULL);
+    }
+
+    argc = get_argv(0, NULL, loaded_image->LoadOptions,
+                    loaded_image->LoadOptionsSize);
+    if ( argc > 0 &&
+         efi_bs->AllocatePool(EfiLoaderData,
+                              (argc + 1) * sizeof(*argv) +
+                                  loaded_image->LoadOptionsSize,
+                              (void **)&argv) == EFI_SUCCESS )
+        get_argv(argc, argv, loaded_image->LoadOptions,
+                 loaded_image->LoadOptionsSize);
+    else
+        argc = 0;
+
+    for ( i = 1; i < argc; ++i )
+    {
+        CHAR16 *ptr = argv[i];
+
+        if ( !ptr )
+            break;
+        if ( *ptr == L'/' || *ptr == L'-' )
+        {
+            if ( wstrcmp(ptr + 1, L"basevideo") == 0 )
+                *base_video = 1;
+            else if ( wstrncmp(ptr + 1, L"cfg=", 4) == 0 )
+                *cfg_file_name = ptr + 5;
+            else if ( i + 1 < argc && wstrcmp(ptr + 1, L"cfg") == 0 )
+                *cfg_file_name = argv[++i];
+            else if ( wstrcmp(ptr + 1, L"help") == 0 ||
+                      (ptr[1] == L'?' && !ptr[2]) )
+            {
+                PrintStr(L"Xen EFI Loader options:\r\n");
+                PrintStr(L"-basevideo   retain current video mode\r\n");
+                PrintStr(L"-cfg=<file>  specify configuration file\r\n");
+                PrintStr(L"-help, -?    display this help\r\n");
+                blexit(NULL);
+            }
+            else
+            {
+                PrintStr(L"WARNING: Unknown command line option '");
+                PrintStr(ptr);
+                PrintStr(L"' ignored\r\n");
+            }
+        }
+        else
+            *section_name = ptr;
+    }
+
+    if ( argc )
+    {
+        *image_name = *argv;
+    }
+}
+
 extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[];
 extern const s32 __trampoline_seg_start[], __trampoline_seg_stop[];
 
@@ -374,8 +442,9 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID;
     EFI_LOADED_IMAGE *loaded_image;
     EFI_STATUS status;
-    unsigned int i, argc;
-    CHAR16 **argv, *file_name, *cfg_file_name = NULL;
+    unsigned int i;
+    CHAR16 *file_name, *cfg_file_name = NULL, *image_name = NULL;
+    CHAR16 *section_name = NULL;
     UINTN cols, rows, depth, size, map_key, info_size, gop_mode = ~0;
     EFI_HANDLE *handles = NULL;
     EFI_SHIM_LOCK_PROTOCOL *shim_lock;
@@ -414,50 +483,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     /* Get the file system interface. */
     dir_handle = get_parent_handle(loaded_image, &file_name);
 
-    argc = get_argv(0, NULL, loaded_image->LoadOptions,
-                    loaded_image->LoadOptionsSize);
-    if ( argc > 0 &&
-         efi_bs->AllocatePool(EfiLoaderData,
-                              (argc + 1) * sizeof(*argv) +
-                                  loaded_image->LoadOptionsSize,
-                              (void **)&argv) == EFI_SUCCESS )
-        get_argv(argc, argv, loaded_image->LoadOptions,
-                 loaded_image->LoadOptionsSize);
-    else
-        argc = 0;
-    for ( i = 1; i < argc; ++i )
-    {
-        CHAR16 *ptr = argv[i];
+    handle_cmdline(loaded_image,
+                             &cfg_file_name,
+                             &base_video,
+                             &image_name,
+                                &section_name);
 
-        if ( !ptr )
-            break;
-        if ( *ptr == L'/' || *ptr == L'-' )
-        {
-            if ( wstrcmp(ptr + 1, L"basevideo") == 0 )
-                base_video = 1;
-            else if ( wstrncmp(ptr + 1, L"cfg=", 4) == 0 )
-                cfg_file_name = ptr + 5;
-            else if ( i + 1 < argc && wstrcmp(ptr + 1, L"cfg") == 0 )
-                cfg_file_name = argv[++i];
-            else if ( wstrcmp(ptr + 1, L"help") == 0 ||
-                      (ptr[1] == L'?' && !ptr[2]) )
-            {
-                PrintStr(L"Xen EFI Loader options:\r\n");
-                PrintStr(L"-basevideo   retain current video mode\r\n");
-                PrintStr(L"-cfg=<file>  specify configuration file\r\n");
-                PrintStr(L"-help, -?    display this help\r\n");
-                blexit(NULL);
-            }
-            else
-            {
-                PrintStr(L"WARNING: Unknown command line option '");
-                PrintStr(ptr);
-                PrintStr(L"' ignored\r\n");
-            }
-        }
-        else
-            section.w = ptr;
-    }
+    section.w = section_name;
 
     if ( !base_video )
     {
@@ -564,9 +596,9 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     if ( name.s )
         place_string(&mbi.cmdline, name.s);
     /* Insert image name last, as it gets prefixed to the other options. */
-    if ( argc )
+    if ( image_name )
     {
-        name.w = *argv;
+        name.w = image_name;
         w2s(&name);
     }
     else
-- 
2.0.0

^ permalink raw reply related	[flat|nested] 44+ messages in thread

* [PATCH RFC 13/19] Refactor get_argv() for sharing
  2014-06-28  1:25 [PATCH RFC 00/19] arm64 EFI stub Roy Franz
                   ` (11 preceding siblings ...)
  2014-06-28  1:25 ` [PATCH RFC 12/19] create handle_cmdline() function Roy Franz
@ 2014-06-28  1:25 ` Roy Franz
  2014-06-28  1:25 ` [PATCH RFC 14/19] Move get_argv() and handle_cmdline() to efi-shared.c Roy Franz
                   ` (9 subsequent siblings)
  22 siblings, 0 replies; 44+ messages in thread
From: Roy Franz @ 2014-06-28  1:25 UTC (permalink / raw)
  To: xen-devel, ian.campbell, stefano.stabellini, tim, jbeulich, keir
  Cc: Roy Franz, fu.wei, linaro-uefi

Refactor get_argv() to prepare for sharing by removing direct updating of the
multiboot structures.  The remaining command line is now returned in an
argument rather than being directly updated by the get_argv() function.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 xen/arch/x86/efi/boot.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/efi/boot.c b/xen/arch/x86/efi/boot.c
index 6ee3c1f..49e0c15 100644
--- a/xen/arch/x86/efi/boot.c
+++ b/xen/arch/x86/efi/boot.c
@@ -118,7 +118,8 @@ static void __init place_string(u32 *addr, const char *s)
 }
 
 static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
-                                    CHAR16 *cmdline, UINTN cmdsize)
+                                    CHAR16 *cmdline, UINTN cmdsize,
+                             CHAR16 **cmdline_remain)
 {
     CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL;
     bool_t prev_sep = TRUE;
@@ -144,10 +145,9 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
                 ++argc;
             else if ( prev && wstrcmp(prev, L"--") == 0 )
             {
-                union string rest = { .w = cmdline };
-
                 --argv;
-                place_string(&mbi.cmdline, w2s(&rest));
+                if (**cmdline_remain)
+                    *cmdline_remain = cmdline;
                 break;
             }
             else
@@ -348,7 +348,8 @@ static void __init relocate_image(unsigned long delta)
 
 void __init handle_cmdline(EFI_LOADED_IMAGE *loaded_image,
                            CHAR16 **cfg_file_name, bool_t *base_video,
-                           CHAR16 **image_name, CHAR16 **section_name)
+                           CHAR16 **image_name, CHAR16 **section_name,
+                           CHAR16 **cmdline_remain)
 {
 
     unsigned int i, argc;
@@ -362,14 +363,14 @@ void __init handle_cmdline(EFI_LOADED_IMAGE *loaded_image,
     }
 
     argc = get_argv(0, NULL, loaded_image->LoadOptions,
-                    loaded_image->LoadOptionsSize);
+                    loaded_image->LoadOptionsSize, NULL);
     if ( argc > 0 &&
          efi_bs->AllocatePool(EfiLoaderData,
                               (argc + 1) * sizeof(*argv) +
                                   loaded_image->LoadOptionsSize,
                               (void **)&argv) == EFI_SUCCESS )
         get_argv(argc, argv, loaded_image->LoadOptions,
-                 loaded_image->LoadOptionsSize);
+                 loaded_image->LoadOptionsSize, cmdline_remain);
     else
         argc = 0;
 
@@ -445,6 +446,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     unsigned int i;
     CHAR16 *file_name, *cfg_file_name = NULL, *image_name = NULL;
     CHAR16 *section_name = NULL;
+    union string cmdline = { NULL };
     UINTN cols, rows, depth, size, map_key, info_size, gop_mode = ~0;
     EFI_HANDLE *handles = NULL;
     EFI_SHIM_LOCK_PROTOCOL *shim_lock;
@@ -487,7 +489,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
                              &cfg_file_name,
                              &base_video,
                              &image_name,
-                                &section_name);
+                                &section_name,
+                   &cmdline.w);
+
+    if (cmdline.w)
+    {
+        place_string(&mbi.cmdline, w2s(&cmdline));
+    }
 
     section.w = section_name;
 
-- 
2.0.0

^ permalink raw reply related	[flat|nested] 44+ messages in thread

* [PATCH RFC 14/19] Move get_argv() and handle_cmdline() to efi-shared.c
  2014-06-28  1:25 [PATCH RFC 00/19] arm64 EFI stub Roy Franz
                   ` (12 preceding siblings ...)
  2014-06-28  1:25 ` [PATCH RFC 13/19] Refactor get_argv() for sharing Roy Franz
@ 2014-06-28  1:25 ` Roy Franz
  2014-06-28  1:25 ` [PATCH RFC 15/19] Add PE/COFF header in head.S Roy Franz
                   ` (8 subsequent siblings)
  22 siblings, 0 replies; 44+ messages in thread
From: Roy Franz @ 2014-06-28  1:25 UTC (permalink / raw)
  To: xen-devel, ian.campbell, stefano.stabellini, tim, jbeulich, keir
  Cc: Roy Franz, fu.wei, linaro-uefi

Move  get_argv() and handle_cmdline() to efi-shared.c now that both are
shareable. No functional changes, only moving of code in this changeset.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 xen/arch/x86/efi/boot.c       | 114 ------------------------------------------
 xen/arch/x86/efi/efi-shared.c | 113 +++++++++++++++++++++++++++++++++++++++++
 xen/include/efi/efi-shared.h  |   7 +++
 3 files changed, 120 insertions(+), 114 deletions(-)

diff --git a/xen/arch/x86/efi/boot.c b/xen/arch/x86/efi/boot.c
index 49e0c15..38edd5e 100644
--- a/xen/arch/x86/efi/boot.c
+++ b/xen/arch/x86/efi/boot.c
@@ -117,53 +117,6 @@ static void __init place_string(u32 *addr, const char *s)
     *addr = (long)alloc;
 }
 
-static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
-                                    CHAR16 *cmdline, UINTN cmdsize,
-                             CHAR16 **cmdline_remain)
-{
-    CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL;
-    bool_t prev_sep = TRUE;
-
-    for ( ; cmdsize > sizeof(*cmdline) && *cmdline;
-            cmdsize -= sizeof(*cmdline), ++cmdline )
-    {
-        bool_t cur_sep = *cmdline == L' ' || *cmdline == L'\t';
-
-        if ( !prev_sep )
-        {
-            if ( cur_sep )
-                ++ptr;
-            else if ( argv )
-            {
-                *ptr = *cmdline;
-                *++ptr = 0;
-            }
-        }
-        else if ( !cur_sep )
-        {
-            if ( !argv )
-                ++argc;
-            else if ( prev && wstrcmp(prev, L"--") == 0 )
-            {
-                --argv;
-                if (**cmdline_remain)
-                    *cmdline_remain = cmdline;
-                break;
-            }
-            else
-            {
-                *argv++ = prev = ptr;
-                *ptr = *cmdline;
-                *++ptr = 0;
-            }
-        }
-        prev_sep = cur_sep;
-    }
-    if ( argv )
-        *argv = NULL;
-    return argc;
-}
-
 /* Only call with non-config files. */
 void __init load_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
                                struct file *file)
@@ -346,73 +299,6 @@ static void __init relocate_image(unsigned long delta)
 }
 
 
-void __init handle_cmdline(EFI_LOADED_IMAGE *loaded_image,
-                           CHAR16 **cfg_file_name, bool_t *base_video,
-                           CHAR16 **image_name, CHAR16 **section_name,
-                           CHAR16 **cmdline_remain)
-{
-
-    unsigned int i, argc;
-    CHAR16 **argv;
-
-
-    if ( !cfg_file_name || !base_video || !image_name )
-    {
-        PrintStr(L"Invalid args to handle_cmdline\r\n");
-        blexit(NULL);
-    }
-
-    argc = get_argv(0, NULL, loaded_image->LoadOptions,
-                    loaded_image->LoadOptionsSize, NULL);
-    if ( argc > 0 &&
-         efi_bs->AllocatePool(EfiLoaderData,
-                              (argc + 1) * sizeof(*argv) +
-                                  loaded_image->LoadOptionsSize,
-                              (void **)&argv) == EFI_SUCCESS )
-        get_argv(argc, argv, loaded_image->LoadOptions,
-                 loaded_image->LoadOptionsSize, cmdline_remain);
-    else
-        argc = 0;
-
-    for ( i = 1; i < argc; ++i )
-    {
-        CHAR16 *ptr = argv[i];
-
-        if ( !ptr )
-            break;
-        if ( *ptr == L'/' || *ptr == L'-' )
-        {
-            if ( wstrcmp(ptr + 1, L"basevideo") == 0 )
-                *base_video = 1;
-            else if ( wstrncmp(ptr + 1, L"cfg=", 4) == 0 )
-                *cfg_file_name = ptr + 5;
-            else if ( i + 1 < argc && wstrcmp(ptr + 1, L"cfg") == 0 )
-                *cfg_file_name = argv[++i];
-            else if ( wstrcmp(ptr + 1, L"help") == 0 ||
-                      (ptr[1] == L'?' && !ptr[2]) )
-            {
-                PrintStr(L"Xen EFI Loader options:\r\n");
-                PrintStr(L"-basevideo   retain current video mode\r\n");
-                PrintStr(L"-cfg=<file>  specify configuration file\r\n");
-                PrintStr(L"-help, -?    display this help\r\n");
-                blexit(NULL);
-            }
-            else
-            {
-                PrintStr(L"WARNING: Unknown command line option '");
-                PrintStr(ptr);
-                PrintStr(L"' ignored\r\n");
-            }
-        }
-        else
-            *section_name = ptr;
-    }
-
-    if ( argc )
-    {
-        *image_name = *argv;
-    }
-}
 
 extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[];
 extern const s32 __trampoline_seg_start[], __trampoline_seg_stop[];
diff --git a/xen/arch/x86/efi/efi-shared.c b/xen/arch/x86/efi/efi-shared.c
index 43601fe..f392ec4 100644
--- a/xen/arch/x86/efi/efi-shared.c
+++ b/xen/arch/x86/efi/efi-shared.c
@@ -185,6 +185,119 @@ void __init PrintErrMesgExit(const CHAR16 *mesg, EFI_STATUS ErrCode)
     blexit(mesg);
 }
 
+unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
+                                    CHAR16 *cmdline, UINTN cmdsize,
+                             CHAR16 **cmdline_remain)
+{
+    CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL;
+    bool_t prev_sep = TRUE;
+
+    for ( ; cmdsize > sizeof(*cmdline) && *cmdline;
+            cmdsize -= sizeof(*cmdline), ++cmdline )
+    {
+        bool_t cur_sep = *cmdline == L' ' || *cmdline == L'\t';
+
+        if ( !prev_sep )
+        {
+            if ( cur_sep )
+                ++ptr;
+            else if ( argv )
+            {
+                *ptr = *cmdline;
+                *++ptr = 0;
+            }
+        }
+        else if ( !cur_sep )
+        {
+            if ( !argv )
+                ++argc;
+            else if ( prev && wstrcmp(prev, L"--") == 0 )
+            {
+                --argv;
+                if (**cmdline_remain)
+                    *cmdline_remain = cmdline;
+                break;
+            }
+            else
+            {
+                *argv++ = prev = ptr;
+                *ptr = *cmdline;
+                *++ptr = 0;
+            }
+        }
+        prev_sep = cur_sep;
+    }
+    if ( argv )
+        *argv = NULL;
+    return argc;
+}
+
+
+void __init handle_cmdline(EFI_LOADED_IMAGE *loaded_image,
+                           CHAR16 **cfg_file_name, bool_t *base_video,
+                           CHAR16 **image_name, CHAR16 **section_name,
+                           CHAR16 **cmdline_remain)
+{
+
+    unsigned int i, argc;
+    CHAR16 **argv;
+
+
+    if ( !cfg_file_name || !base_video || !image_name )
+        blexit(L"Invalid args to handle_cmdline\r\n");
+
+    argc = get_argv(0, NULL, loaded_image->LoadOptions,
+                    loaded_image->LoadOptionsSize, NULL);
+    if ( argc > 0 &&
+         efi_bs->AllocatePool(EfiLoaderData,
+                              (argc + 1) * sizeof(*argv) +
+                                  loaded_image->LoadOptionsSize,
+                              (void **)&argv) == EFI_SUCCESS )
+        get_argv(argc, argv, loaded_image->LoadOptions,
+                 loaded_image->LoadOptionsSize, cmdline_remain);
+    else
+        argc = 0;
+
+    for ( i = 1; i < argc; ++i )
+    {
+        CHAR16 *ptr = argv[i];
+
+        if ( !ptr )
+            break;
+        if ( *ptr == L'/' || *ptr == L'-' )
+        {
+            if ( wstrcmp(ptr + 1, L"basevideo") == 0 )
+                *base_video = 1;
+            else if ( wstrncmp(ptr + 1, L"cfg=", 4) == 0 )
+                *cfg_file_name = ptr + 5;
+            else if ( i + 1 < argc && wstrcmp(ptr + 1, L"cfg") == 0 )
+                *cfg_file_name = argv[++i];
+            else if ( wstrcmp(ptr + 1, L"help") == 0 ||
+                      (ptr[1] == L'?' && !ptr[2]) )
+            {
+                PrintStr(L"Xen EFI Loader options:\r\n");
+                PrintStr(L"-basevideo   retain current video mode\r\n");
+                PrintStr(L"-cfg=<file>  specify configuration file\r\n");
+                PrintStr(L"-help, -?    display this help\r\n");
+                blexit(NULL);
+            }
+            else
+            {
+                PrintStr(L"WARNING: Unknown command line option '");
+                PrintStr(ptr);
+                PrintStr(L"' ignored\r\n");
+            }
+        }
+        else
+            *section_name = ptr;
+    }
+
+    if ( argc )
+    {
+        *image_name = *argv;
+    }
+}
+
 /* Truncate string at first space, and return pointer
  * to remainder of string.
  */
diff --git a/xen/include/efi/efi-shared.h b/xen/include/efi/efi-shared.h
index a1028d1..b21793b 100644
--- a/xen/include/efi/efi-shared.h
+++ b/xen/include/efi/efi-shared.h
@@ -48,6 +48,9 @@ CHAR16 *__init point_tail(CHAR16 *fn);
 void __init pre_parse(const struct file *cfg);
 char *__init get_value(const struct file *cfg, const char *section,
                               const char *item);
+unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
+                             CHAR16 *cmdline, UINTN cmdsize,
+                             CHAR16 **cmdline_remain);
 
 
 
@@ -61,4 +64,8 @@ void __init read_config_file(EFI_FILE_HANDLE *cfg_dir_handle,
                              struct file *cfg, CHAR16 *cfg_file_name,
                              union string *section,
                              CHAR16 *xen_file_name);
+void __init handle_cmdline(EFI_LOADED_IMAGE *loaded_image,
+                           CHAR16 **cfg_file_name, bool_t *base_video,
+                           CHAR16 **image_name, CHAR16 **section_name,
+                           CHAR16 **cmdline_remain);
 #endif
-- 
2.0.0

^ permalink raw reply related	[flat|nested] 44+ messages in thread

* [PATCH RFC 15/19] Add PE/COFF header in head.S
  2014-06-28  1:25 [PATCH RFC 00/19] arm64 EFI stub Roy Franz
                   ` (13 preceding siblings ...)
  2014-06-28  1:25 ` [PATCH RFC 14/19] Move get_argv() and handle_cmdline() to efi-shared.c Roy Franz
@ 2014-06-28  1:25 ` Roy Franz
  2014-07-02 12:02   ` Ian Campbell
  2014-06-28  1:25 ` [PATCH RFC 16/19] create ARM EFI headers, based on x86 Roy Franz
                   ` (7 subsequent siblings)
  22 siblings, 1 reply; 44+ messages in thread
From: Roy Franz @ 2014-06-28  1:25 UTC (permalink / raw)
  To: xen-devel, ian.campbell, stefano.stabellini, tim, jbeulich, keir
  Cc: Roy Franz, fu.wei, linaro-uefi

This patch adds a simple PE/COFF header in head.S.  PE/COFF linker support
is not available for arm64, so a native build is not possible.  Also, this
allows the binary to be both a PE/COFF EFI application, and a normal Image
file bootable like a Linux kernel.  The arm and arm64 Linux kernels use the
same methodology to create a single image that is both an EFI application and
a zImage/Image file.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 xen/arch/arm/arm64/head.S | 118 +++++++++++++++++++++++++++++++++++++++++++++-
 xen/arch/arm/xen.lds.S    |   1 +
 2 files changed, 117 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 2a13527..9b88aeb 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -84,7 +84,7 @@
 #endif /* !CONFIG_EARLY_PRINTK */
 
         /*.aarch64*/
-
+#define CONFIG_EFI_STUB
         /*
          * Kernel startup entry point.
          * ---------------------------
@@ -100,12 +100,24 @@
          */
 
         .global start
+efi_stub_entry:  /* Dummy symbol so we can compile before actual stub added */
 start:
+#ifdef CONFIG_EFI_STUB
         /*
          * DO NOT MODIFY. Image header expected by Linux boot-loaders.
          */
-        b       real_start           /* branch to kernel start, magic */
+efi_head:
+        /*
+         * This add instruction has no meaningful effect except that
+         * its opcode forms the magic "MZ" signature of a PE/COFF file
+	 * that is required for UEFI applications.
+         */
+        add     x13, x18, #0x16
+        b       real_start           /* branch to kernel start */
+#else
+        b       real_start           /* branch to kernel start */
         .long   0                    /* reserved */
+#endif
         .quad   0                    /* Image load offset from start of RAM */
         .quad   0                    /* reserved */
         .quad   0                    /* reserved */
@@ -116,7 +128,109 @@ start:
         .byte   0x52
         .byte   0x4d
         .byte   0x64
+#ifdef CONFIG_EFI_STUB
+	.long	pe_header - efi_head	/* Offset to the PE header. */
+#else
         .word   0                    /* reserved */
+#endif
+
+#ifdef CONFIG_EFI_STUB
+	.align 3
+pe_header:
+	.ascii	"PE"
+	.short 	0
+coff_header:
+	.short	0xaa64				// AArch64
+	.short	2				// nr_sections
+	.long	0 				// TimeDateStamp
+	.long	0				// PointerToSymbolTable
+	.long	1				// NumberOfSymbols
+	.short	section_table - optional_header	// SizeOfOptionalHeader
+	.short	0x206				// Characteristics.
+						// IMAGE_FILE_DEBUG_STRIPPED |
+						// IMAGE_FILE_EXECUTABLE_IMAGE |
+						// IMAGE_FILE_LINE_NUMS_STRIPPED
+optional_header:
+	.short	0x20b				// PE32+ format
+	.byte	0x02				// MajorLinkerVersion
+	.byte	0x14				// MinorLinkerVersion
+	.long	__init_end_efi - real_start			// SizeOfCode
+	.long	0				// SizeOfInitializedData
+	.long	0				// SizeOfUninitializedData
+	.long	efi_stub_entry - efi_head	// AddressOfEntryPoint
+	.long	real_start - efi_head		// BaseOfCode
+
+extra_header_fields:
+	.quad	0				// ImageBase
+	.long	0x200000			// SectionAlignment (2MByte)
+	.long	0x8				// FileAlignment
+	.short	0				// MajorOperatingSystemVersion
+	.short	0				// MinorOperatingSystemVersion
+	.short	0				// MajorImageVersion
+	.short	0				// MinorImageVersion
+	.short	0				// MajorSubsystemVersion
+	.short	0				// MinorSubsystemVersion
+	.long	0				// Win32VersionValue
+
+	.long	__init_end_efi - efi_head		// SizeOfImage
+
+	// Everything before the kernel image is considered part of the header
+	.long	real_start - efi_head			// SizeOfHeaders
+	.long	0				// CheckSum
+	.short	0xa				// Subsystem (EFI application)
+	.short	0				// DllCharacteristics
+	.quad	0				// SizeOfStackReserve
+	.quad	0				// SizeOfStackCommit
+	.quad	0				// SizeOfHeapReserve
+	.quad	0				// SizeOfHeapCommit
+	.long	0				// LoaderFlags
+	.long	0x6				// NumberOfRvaAndSizes
+
+	.quad	0				// ExportTable
+	.quad	0				// ImportTable
+	.quad	0				// ResourceTable
+	.quad	0				// ExceptionTable
+	.quad	0				// CertificationTable
+	.quad	0				// BaseRelocationTable
+
+	// Section table
+section_table:
+
+	/*
+	 * The EFI application loader requires a relocation section
+	 * because EFI applications must be relocatable.  This is a
+	 * dummy section as far as we are concerned.
+	 */
+	.ascii	".reloc"
+	.byte	0
+	.byte	0			// end of 0 padding of section name
+	.long	0
+	.long	0
+	.long	0			// SizeOfRawData
+	.long	0			// PointerToRawData
+	.long	0			// PointerToRelocations
+	.long	0			// PointerToLineNumbers
+	.short	0			// NumberOfRelocations
+	.short	0			// NumberOfLineNumbers
+	.long	0x42100040		// Characteristics (section flags)
+
+
+	.ascii	".text"
+	.byte	0
+	.byte	0
+	.byte	0        		// end of 0 padding of section name
+	.long	__init_end_efi - real_start		// VirtualSize
+	.long	real_start - efi_head	// VirtualAddress
+	.long	__init_end_efi - real_start		// SizeOfRawData
+	.long	real_start - efi_head	// PointerToRawData
+
+	.long	0		// PointerToRelocations (0 for executables)
+	.long	0		// PointerToLineNumbers (0 for executables)
+	.short	0		// NumberOfRelocations  (0 for executables)
+	.short	0		// NumberOfLineNumbers  (0 for executables)
+	.long	0xe0500020	// Characteristics (section flags)
+	.align 5
+#endif
 
 real_start:
         msr   DAIFSet, 0xf           /* Disable all interrupts */
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index be55dad..62b9d5b 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -135,6 +135,7 @@ SECTIONS
        *(.xsm_initcall.init)
        __xsm_initcall_end = .;
   } :text
+  __init_end_efi = .;
   . = ALIGN(STACK_SIZE);
   __init_end = .;
 
-- 
2.0.0

^ permalink raw reply related	[flat|nested] 44+ messages in thread

* [PATCH RFC 16/19] create ARM EFI headers, based on x86
  2014-06-28  1:25 [PATCH RFC 00/19] arm64 EFI stub Roy Franz
                   ` (14 preceding siblings ...)
  2014-06-28  1:25 ` [PATCH RFC 15/19] Add PE/COFF header in head.S Roy Franz
@ 2014-06-28  1:25 ` Roy Franz
  2014-06-28  1:25 ` [PATCH RFC 17/19] Remove x86 specific defintions from efibind.h Roy Franz
                   ` (6 subsequent siblings)
  22 siblings, 0 replies; 44+ messages in thread
From: Roy Franz @ 2014-06-28  1:25 UTC (permalink / raw)
  To: xen-devel, ian.campbell, stefano.stabellini, tim, jbeulich, keir
  Cc: Roy Franz, fu.wei, linaro-uefi

Create the EFI header files, by copying from x86.  Next patch will remove x86
specific content.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 xen/arch/arm/efi.h                  |  11 ++
 xen/include/asm-arm/arm64/efibind.h | 291 ++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/efibind.h       |   2 +
 3 files changed, 304 insertions(+)
 create mode 100644 xen/arch/arm/efi.h
 create mode 100644 xen/include/asm-arm/arm64/efibind.h
 create mode 100644 xen/include/asm-arm/efibind.h

diff --git a/xen/arch/arm/efi.h b/xen/arch/arm/efi.h
new file mode 100644
index 0000000..204fa6e
--- /dev/null
+++ b/xen/arch/arm/efi.h
@@ -0,0 +1,11 @@
+#define EFIAPI
+#include <asm/efibind.h>
+#include <efi/efidef.h>
+#include <efi/efierr.h>
+#include <efi/eficon.h>
+#include <efi/efidevp.h>
+#include <efi/eficapsule.h>
+#include <efi/efiapi.h>
+#include <xen/efi.h>
+#include <xen/spinlock.h>
+#include <asm/page.h>
diff --git a/xen/include/asm-arm/arm64/efibind.h b/xen/include/asm-arm/arm64/efibind.h
new file mode 100644
index 0000000..2db3568
--- /dev/null
+++ b/xen/include/asm-arm/arm64/efibind.h
@@ -0,0 +1,291 @@
+/*++
+
+Copyright (c) 1998  Intel Corporation
+
+Module Name:
+
+    efefind.h
+
+Abstract:
+
+    EFI to compile bindings
+
+
+
+
+Revision History
+
+--*/
+
+#ifndef __GNUC__
+#pragma pack()
+#endif
+
+//
+// Basic int types of various widths
+//
+
+#if !defined(__STDC_VERSION__) || (__STDC_VERSION__ < 199901L )
+
+    // No ANSI C 1999/2000 stdint.h integer width declarations
+
+    #if defined(_MSC_EXTENSIONS)
+
+        // Use Microsoft C compiler integer width declarations
+
+        typedef unsigned __int64    uint64_t;
+        typedef __int64             int64_t;
+        typedef unsigned __int32    uint32_t;
+        typedef __int32             int32_t;
+        typedef unsigned short      uint16_t;
+        typedef short               int16_t;
+        typedef unsigned char       uint8_t;
+        typedef char                int8_t;
+    #elif defined(__GNUC__)
+        typedef unsigned long long  uint64_t __attribute__((aligned (8)));
+        typedef long long           int64_t __attribute__((aligned (8)));
+        typedef unsigned int        uint32_t;
+        typedef int                 int32_t;
+        typedef unsigned short      uint16_t;
+        typedef short               int16_t;
+        typedef unsigned char       uint8_t;
+        typedef char                int8_t;
+    #elif defined(UNIX_LP64)
+
+        /*  Use LP64 programming model from C_FLAGS for integer width declarations */
+
+       typedef unsigned long       uint64_t;
+       typedef long                int64_t;
+       typedef unsigned int        uint32_t;
+       typedef int                 int32_t;
+       typedef unsigned short      uint16_t;
+       typedef short               int16_t;
+       typedef unsigned char       uint8_t;
+       typedef char                int8_t;
+    #else
+
+       /*  Assume P64 programming model from C_FLAGS for integer width declarations */
+
+       typedef unsigned long long  uint64_t __attribute__((aligned (8)));
+       typedef long long           int64_t __attribute__((aligned (8)));
+       typedef unsigned int        uint32_t;
+       typedef int                 int32_t;
+       typedef unsigned short      uint16_t;
+       typedef short               int16_t;
+       typedef unsigned char       uint8_t;
+       typedef char                int8_t;
+    #endif
+#endif
+
+//
+// Basic EFI types of various widths
+//
+
+#ifndef __WCHAR_TYPE__
+# define __WCHAR_TYPE__ short
+#endif
+
+typedef uint64_t   UINT64;
+typedef int64_t    INT64;
+
+#ifndef _BASETSD_H_
+    typedef uint32_t   UINT32;
+    typedef int32_t    INT32;
+#endif
+
+typedef uint16_t   UINT16;
+typedef int16_t    INT16;
+typedef uint8_t    UINT8;
+typedef int8_t     INT8;
+typedef __WCHAR_TYPE__ WCHAR;
+
+#undef VOID
+#define VOID    void
+
+
+typedef int64_t    INTN;
+typedef uint64_t   UINTN;
+
+#ifdef EFI_NT_EMULATOR
+    #define POST_CODE(_Data)
+#else
+    #ifdef EFI_DEBUG
+#define POST_CODE(_Data)    __asm mov eax,(_Data) __asm out 0x80,al
+    #else
+        #define POST_CODE(_Data)
+    #endif
+#endif
+
+#define EFIERR(a)           (0x8000000000000000 | a)
+#define EFI_ERROR_MASK      0x8000000000000000
+#define EFIERR_OEM(a)       (0xc000000000000000 | a)
+
+
+#define BAD_POINTER         0xFBFBFBFBFBFBFBFB
+#define MAX_ADDRESS         0xFFFFFFFFFFFFFFFF
+
+#ifdef EFI_NT_EMULATOR
+    #define BREAKPOINT()        __asm { int 3 }
+#else
+    #define BREAKPOINT()        while (TRUE);    // Make it hang on Bios[Dbg]32
+#endif
+
+//
+// Pointers must be aligned to these address to function
+//
+
+#define MIN_ALIGNMENT_SIZE  4
+
+#define ALIGN_VARIABLE(Value ,Adjustment) \
+            (UINTN)Adjustment = 0; \
+            if((UINTN)Value % MIN_ALIGNMENT_SIZE) \
+                (UINTN)Adjustment = MIN_ALIGNMENT_SIZE - ((UINTN)Value % MIN_ALIGNMENT_SIZE); \
+            Value = (UINTN)Value + (UINTN)Adjustment
+
+
+//
+// Define macros to build data structure signatures from characters.
+//
+
+#define EFI_SIGNATURE_16(A,B)             ((A) | (B<<8))
+#define EFI_SIGNATURE_32(A,B,C,D)         (EFI_SIGNATURE_16(A,B)     | (EFI_SIGNATURE_16(C,D)     << 16))
+#define EFI_SIGNATURE_64(A,B,C,D,E,F,G,H) (EFI_SIGNATURE_32(A,B,C,D) | ((UINT64)(EFI_SIGNATURE_32(E,F,G,H)) << 32))
+//
+// To export & import functions in the EFI emulator environment
+//
+
+#ifdef EFI_NT_EMULATOR
+    #define EXPORTAPI           __declspec( dllexport )
+#else
+    #define EXPORTAPI
+#endif
+
+
+//
+// EFIAPI - prototype calling convention for EFI function pointers
+// BOOTSERVICE - prototype for implementation of a boot service interface
+// RUNTIMESERVICE - prototype for implementation of a runtime service interface
+// RUNTIMEFUNCTION - prototype for implementation of a runtime function that is not a service
+// RUNTIME_CODE - pragma macro for declaring runtime code
+//
+
+#ifndef EFIAPI                  // Forces EFI calling conventions reguardless of compiler options
+    #ifdef _MSC_EXTENSIONS
+        #define EFIAPI __cdecl  // Force C calling convention for Microsoft C compiler
+    #elif __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4)
+        #define EFIAPI __attribute__((__ms_abi__))  // Force Microsoft ABI
+    #else
+        #define EFIAPI          // Substitute expresion to force C calling convention
+    #endif
+#endif
+
+#define BOOTSERVICE
+//#define RUNTIMESERVICE(proto,a)    alloc_text("rtcode",a); proto a
+//#define RUNTIMEFUNCTION(proto,a)   alloc_text("rtcode",a); proto a
+#define RUNTIMESERVICE
+#define RUNTIMEFUNCTION
+
+
+#define RUNTIME_CODE(a)         alloc_text("rtcode", a)
+#define BEGIN_RUNTIME_DATA()    data_seg("rtdata")
+#define END_RUNTIME_DATA()      data_seg("")
+
+#define VOLATILE    volatile
+
+#define MEMORY_FENCE()
+
+#ifdef EFI_NT_EMULATOR
+
+//
+// To help ensure proper coding of integrated drivers, they are
+// compiled as DLLs.  In NT they require a dll init entry pointer.
+// The macro puts a stub entry point into the DLL so it will load.
+//
+
+#define EFI_DRIVER_ENTRY_POINT(InitFunction)    \
+    UINTN                                       \
+    __stdcall                                   \
+    _DllMainCRTStartup (                        \
+        UINTN    Inst,                          \
+        UINTN    reason_for_call,               \
+        VOID    *rserved                        \
+        )                                       \
+    {                                           \
+        return 1;                               \
+    }                                           \
+                                                \
+    int                                         \
+    EXPORTAPI                                   \
+    __cdecl                                     \
+    InitializeDriver (                          \
+        void *ImageHandle,                      \
+        void *SystemTable                       \
+        )                                       \
+    {                                           \
+        return InitFunction(ImageHandle, SystemTable);       \
+    }
+
+
+    #define LOAD_INTERNAL_DRIVER(_if, type, name, entry)      \
+        (_if)->LoadInternal(type, name, NULL)
+
+#else // EFI_NT_EMULATOR
+
+//
+// When build similiar to FW, then link everything together as
+// one big module.
+//
+
+    #define EFI_DRIVER_ENTRY_POINT(InitFunction)    \
+        UINTN                                       \
+        InitializeDriver (                          \
+            VOID    *ImageHandle,                   \
+            VOID    *SystemTable                    \
+            )                                       \
+        {                                           \
+            return InitFunction(ImageHandle,        \
+                    SystemTable);                   \
+        }                                           \
+                                                    \
+        EFI_STATUS efi_main(                        \
+            EFI_HANDLE image,                       \
+            EFI_SYSTEM_TABLE *systab                \
+            ) __attribute__((weak,                  \
+                    alias ("InitializeDriver")));
+
+    #define LOAD_INTERNAL_DRIVER(_if, type, name, entry)    \
+            (_if)->LoadInternal(type, name, entry)
+
+#endif // EFI_FW_NT
+
+//
+// Some compilers don't support the forward reference construct:
+//  typedef struct XXXXX
+//
+// The following macro provide a workaround for such cases.
+//
+#ifdef NO_INTERFACE_DECL
+#define INTERFACE_DECL(x)
+#else
+#ifdef __GNUC__
+#define INTERFACE_DECL(x) struct x
+#else
+#define INTERFACE_DECL(x) typedef struct x
+#endif
+#endif
+
+#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4)
+#define uefi_call_wrapper(func, va_num, ...)	func(__VA_ARGS__)
+#else
+/* for x86_64, EFI_FUNCTION_WRAPPER must be defined */
+#ifdef  EFI_FUNCTION_WRAPPER
+UINTN uefi_call_wrapper(void *func, unsigned long va_num, ...);
+#else
+#error "EFI_FUNCTION_WRAPPER must be defined for x86_64 architecture"
+#endif
+#endif
+
+#ifdef _MSC_EXTENSIONS
+#pragma warning ( disable : 4731 )  // Suppress warnings about modification of EBP
+#endif
+
diff --git a/xen/include/asm-arm/efibind.h b/xen/include/asm-arm/efibind.h
new file mode 100644
index 0000000..09dca7a
--- /dev/null
+++ b/xen/include/asm-arm/efibind.h
@@ -0,0 +1,2 @@
+#include <xen/types.h>
+#include <asm/arm64/efibind.h>
-- 
2.0.0

^ permalink raw reply related	[flat|nested] 44+ messages in thread

* [PATCH RFC 17/19] Remove x86 specific defintions from efibind.h
  2014-06-28  1:25 [PATCH RFC 00/19] arm64 EFI stub Roy Franz
                   ` (15 preceding siblings ...)
  2014-06-28  1:25 ` [PATCH RFC 16/19] create ARM EFI headers, based on x86 Roy Franz
@ 2014-06-28  1:25 ` Roy Franz
  2014-07-02 12:03   ` Ian Campbell
  2014-06-28  1:25 ` [PATCH RFC 18/19] Add assembler use support for efibind.h Roy Franz
                   ` (5 subsequent siblings)
  22 siblings, 1 reply; 44+ messages in thread
From: Roy Franz @ 2014-06-28  1:25 UTC (permalink / raw)
  To: xen-devel, ian.campbell, stefano.stabellini, tim, jbeulich, keir
  Cc: Roy Franz, fu.wei, linaro-uefi

Clean up arm64 efibind.h to not include x86 specific definitions.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 xen/include/asm-arm/arm64/efibind.h | 125 ++++++------------------------------
 1 file changed, 18 insertions(+), 107 deletions(-)

diff --git a/xen/include/asm-arm/arm64/efibind.h b/xen/include/asm-arm/arm64/efibind.h
index 2db3568..fe53a3b 100644
--- a/xen/include/asm-arm/arm64/efibind.h
+++ b/xen/include/asm-arm/arm64/efibind.h
@@ -29,19 +29,7 @@ Revision History
 
     // No ANSI C 1999/2000 stdint.h integer width declarations
 
-    #if defined(_MSC_EXTENSIONS)
-
-        // Use Microsoft C compiler integer width declarations
-
-        typedef unsigned __int64    uint64_t;
-        typedef __int64             int64_t;
-        typedef unsigned __int32    uint32_t;
-        typedef __int32             int32_t;
-        typedef unsigned short      uint16_t;
-        typedef short               int16_t;
-        typedef unsigned char       uint8_t;
-        typedef char                int8_t;
-    #elif defined(__GNUC__)
+    #if defined(__GNUC__)
         typedef unsigned long long  uint64_t __attribute__((aligned (8)));
         typedef long long           int64_t __attribute__((aligned (8)));
         typedef unsigned int        uint32_t;
@@ -106,15 +94,7 @@ typedef __WCHAR_TYPE__ WCHAR;
 typedef int64_t    INTN;
 typedef uint64_t   UINTN;
 
-#ifdef EFI_NT_EMULATOR
-    #define POST_CODE(_Data)
-#else
-    #ifdef EFI_DEBUG
-#define POST_CODE(_Data)    __asm mov eax,(_Data) __asm out 0x80,al
-    #else
-        #define POST_CODE(_Data)
-    #endif
-#endif
+#define POST_CODE(_Data)
 
 #define EFIERR(a)           (0x8000000000000000 | a)
 #define EFI_ERROR_MASK      0x8000000000000000
@@ -124,11 +104,7 @@ typedef uint64_t   UINTN;
 #define BAD_POINTER         0xFBFBFBFBFBFBFBFB
 #define MAX_ADDRESS         0xFFFFFFFFFFFFFFFF
 
-#ifdef EFI_NT_EMULATOR
-    #define BREAKPOINT()        __asm { int 3 }
-#else
-    #define BREAKPOINT()        while (TRUE);    // Make it hang on Bios[Dbg]32
-#endif
+#define BREAKPOINT()        while (TRUE);    // Make it hang on Bios[Dbg]32
 
 //
 // Pointers must be aligned to these address to function
@@ -150,15 +126,8 @@ typedef uint64_t   UINTN;
 #define EFI_SIGNATURE_16(A,B)             ((A) | (B<<8))
 #define EFI_SIGNATURE_32(A,B,C,D)         (EFI_SIGNATURE_16(A,B)     | (EFI_SIGNATURE_16(C,D)     << 16))
 #define EFI_SIGNATURE_64(A,B,C,D,E,F,G,H) (EFI_SIGNATURE_32(A,B,C,D) | ((UINT64)(EFI_SIGNATURE_32(E,F,G,H)) << 32))
-//
-// To export & import functions in the EFI emulator environment
-//
 
-#ifdef EFI_NT_EMULATOR
-    #define EXPORTAPI           __declspec( dllexport )
-#else
-    #define EXPORTAPI
-#endif
+#define EXPORTAPI
 
 
 //
@@ -170,13 +139,7 @@ typedef uint64_t   UINTN;
 //
 
 #ifndef EFIAPI                  // Forces EFI calling conventions reguardless of compiler options
-    #ifdef _MSC_EXTENSIONS
-        #define EFIAPI __cdecl  // Force C calling convention for Microsoft C compiler
-    #elif __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4)
-        #define EFIAPI __attribute__((__ms_abi__))  // Force Microsoft ABI
-    #else
         #define EFIAPI          // Substitute expresion to force C calling convention
-    #endif
 #endif
 
 #define BOOTSERVICE
@@ -194,69 +157,32 @@ typedef uint64_t   UINTN;
 
 #define MEMORY_FENCE()
 
-#ifdef EFI_NT_EMULATOR
 
 //
-// To help ensure proper coding of integrated drivers, they are
-// compiled as DLLs.  In NT they require a dll init entry pointer.
-// The macro puts a stub entry point into the DLL so it will load.
+// When build similiar to FW, then link everything together as
+// one big module.
 //
 
 #define EFI_DRIVER_ENTRY_POINT(InitFunction)    \
     UINTN                                       \
-    __stdcall                                   \
-    _DllMainCRTStartup (                        \
-        UINTN    Inst,                          \
-        UINTN    reason_for_call,               \
-        VOID    *rserved                        \
+    InitializeDriver (                          \
+        VOID    *ImageHandle,                   \
+        VOID    *SystemTable                    \
         )                                       \
     {                                           \
-        return 1;                               \
+        return InitFunction(ImageHandle,        \
+                SystemTable);                   \
     }                                           \
                                                 \
-    int                                         \
-    EXPORTAPI                                   \
-    __cdecl                                     \
-    InitializeDriver (                          \
-        void *ImageHandle,                      \
-        void *SystemTable                       \
-        )                                       \
-    {                                           \
-        return InitFunction(ImageHandle, SystemTable);       \
-    }
+    EFI_STATUS efi_main(                        \
+        EFI_HANDLE image,                       \
+        EFI_SYSTEM_TABLE *systab                \
+        ) __attribute__((weak,                  \
+                alias ("InitializeDriver")));
 
+#define LOAD_INTERNAL_DRIVER(_if, type, name, entry)    \
+        (_if)->LoadInternal(type, name, entry)
 
-    #define LOAD_INTERNAL_DRIVER(_if, type, name, entry)      \
-        (_if)->LoadInternal(type, name, NULL)
-
-#else // EFI_NT_EMULATOR
-
-//
-// When build similiar to FW, then link everything together as
-// one big module.
-//
-
-    #define EFI_DRIVER_ENTRY_POINT(InitFunction)    \
-        UINTN                                       \
-        InitializeDriver (                          \
-            VOID    *ImageHandle,                   \
-            VOID    *SystemTable                    \
-            )                                       \
-        {                                           \
-            return InitFunction(ImageHandle,        \
-                    SystemTable);                   \
-        }                                           \
-                                                    \
-        EFI_STATUS efi_main(                        \
-            EFI_HANDLE image,                       \
-            EFI_SYSTEM_TABLE *systab                \
-            ) __attribute__((weak,                  \
-                    alias ("InitializeDriver")));
-
-    #define LOAD_INTERNAL_DRIVER(_if, type, name, entry)    \
-            (_if)->LoadInternal(type, name, entry)
-
-#endif // EFI_FW_NT
 
 //
 // Some compilers don't support the forward reference construct:
@@ -274,18 +200,3 @@ typedef uint64_t   UINTN;
 #endif
 #endif
 
-#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4)
-#define uefi_call_wrapper(func, va_num, ...)	func(__VA_ARGS__)
-#else
-/* for x86_64, EFI_FUNCTION_WRAPPER must be defined */
-#ifdef  EFI_FUNCTION_WRAPPER
-UINTN uefi_call_wrapper(void *func, unsigned long va_num, ...);
-#else
-#error "EFI_FUNCTION_WRAPPER must be defined for x86_64 architecture"
-#endif
-#endif
-
-#ifdef _MSC_EXTENSIONS
-#pragma warning ( disable : 4731 )  // Suppress warnings about modification of EBP
-#endif
-
-- 
2.0.0

^ permalink raw reply related	[flat|nested] 44+ messages in thread

* [PATCH RFC 18/19] Add assembler use support for efibind.h
  2014-06-28  1:25 [PATCH RFC 00/19] arm64 EFI stub Roy Franz
                   ` (16 preceding siblings ...)
  2014-06-28  1:25 ` [PATCH RFC 17/19] Remove x86 specific defintions from efibind.h Roy Franz
@ 2014-06-28  1:25 ` Roy Franz
  2014-06-28  1:25 ` [PATCH RFC 19/19] Add EFI stub for ARM64 Roy Franz
                   ` (4 subsequent siblings)
  22 siblings, 0 replies; 44+ messages in thread
From: Roy Franz @ 2014-06-28  1:25 UTC (permalink / raw)
  To: xen-devel, ian.campbell, stefano.stabellini, tim, jbeulich, keir
  Cc: Roy Franz, fu.wei, linaro-uefi

The C efi_entry() function returns the address of the FDT for XEN to use,
so we add the EFI_STUB_ERROR define to efibind.h and make it includable in
assembly files.  EFI_STUB_ERROR is defined as ~0, which is an invalid
address for a FDT to be loaded at.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 xen/include/asm-arm/arm64/efibind.h | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/xen/include/asm-arm/arm64/efibind.h b/xen/include/asm-arm/arm64/efibind.h
index fe53a3b..ecc819d 100644
--- a/xen/include/asm-arm/arm64/efibind.h
+++ b/xen/include/asm-arm/arm64/efibind.h
@@ -21,6 +21,16 @@ Revision History
 #pragma pack()
 #endif
 
+#define EFIERR(a)           (0x8000000000000000 | a)
+#define EFI_ERROR_MASK      0x8000000000000000
+#define EFIERR_OEM(a)       (0xc000000000000000 | a)
+
+#define BAD_POINTER         0xFBFBFBFBFBFBFBFB
+#define MAX_ADDRESS         0xFFFFFFFFFFFFFFFF
+
+#define EFI_STUB_ERROR      MAX_ADDRESS
+
+#ifndef __ASSEMBLY__
 //
 // Basic int types of various widths
 //
@@ -96,13 +106,6 @@ typedef uint64_t   UINTN;
 
 #define POST_CODE(_Data)
 
-#define EFIERR(a)           (0x8000000000000000 | a)
-#define EFI_ERROR_MASK      0x8000000000000000
-#define EFIERR_OEM(a)       (0xc000000000000000 | a)
-
-
-#define BAD_POINTER         0xFBFBFBFBFBFBFBFB
-#define MAX_ADDRESS         0xFFFFFFFFFFFFFFFF
 
 #define BREAKPOINT()        while (TRUE);    // Make it hang on Bios[Dbg]32
 
@@ -200,3 +203,4 @@ typedef uint64_t   UINTN;
 #endif
 #endif
 
+#endif
-- 
2.0.0

^ permalink raw reply related	[flat|nested] 44+ messages in thread

* [PATCH RFC 19/19] Add EFI stub for ARM64
  2014-06-28  1:25 [PATCH RFC 00/19] arm64 EFI stub Roy Franz
                   ` (17 preceding siblings ...)
  2014-06-28  1:25 ` [PATCH RFC 18/19] Add assembler use support for efibind.h Roy Franz
@ 2014-06-28  1:25 ` Roy Franz
  2014-07-02 12:34   ` Ian Campbell
  2014-06-28 15:34 ` [PATCH RFC 00/19] arm64 EFI stub Roy Franz
                   ` (3 subsequent siblings)
  22 siblings, 1 reply; 44+ messages in thread
From: Roy Franz @ 2014-06-28  1:25 UTC (permalink / raw)
  To: xen-devel, ian.campbell, stefano.stabellini, tim, jbeulich, keir
  Cc: Roy Franz, fu.wei, linaro-uefi

This patch adds the real EFI application entry point in head.S,
and adds the efi.c file that will contain the EFI stub implementation.
The assembly wrapper is responsible for returning the processor state
to what XEN expects when starting.  The EFI stub processes the
XEN EFI configuration file to load the dom0 kernel, ramdisk, etc and
constructs a device tree for XEN to use, then transfers control to
the normal XEN image entry point.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 xen/arch/arm/Makefile     |   2 +
 xen/arch/arm/arm64/head.S |  62 ++++-
 xen/arch/arm/efi-shared.c |   1 +
 xen/arch/arm/efi.c        | 686 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 750 insertions(+), 1 deletion(-)
 create mode 120000 xen/arch/arm/efi-shared.c
 create mode 100644 xen/arch/arm/efi.c

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 63e0460..c3ed74f 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -33,6 +33,8 @@ obj-y += hvm.o
 obj-y += device.o
 obj-y += decode.o
 obj-y += processor.o
+obj-y += efi.o
+obj-y += efi-shared.o
 
 #obj-bin-y += ....o
 
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 9b88aeb..9882519 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -24,6 +24,8 @@
 #include <asm/page.h>
 #include <asm/asm_defns.h>
 #include <asm/early_printk.h>
+#include <efi/efierr.h>
+#include <asm/arm64/efibind.h>
 
 #define PT_PT     0xf7f /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=1 P=1 */
 #define PT_MEM    0xf7d /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=0 P=1 */
@@ -100,7 +102,6 @@
          */
 
         .global start
-efi_stub_entry:  /* Dummy symbol so we can compile before actual stub added */
 start:
 #ifdef CONFIG_EFI_STUB
         /*
@@ -667,6 +668,65 @@ GLOBAL(lookup_processor_type)
         mov  x0, #0
         ret
 
+
+
+ENTRY(efi_stub_entry)
+	stp	x29, x30, [sp, #-32]!
+
+	/*
+	 * Call efi_entry to do the real work.
+	 * x0 and x1 are already set up by firmware.
+	 *
+	 * unsigned long efi_entry(EFI_HANDLE handle,
+	 *  			   EFI_SYSTEM_TABLE *sys_table);
+	 */
+
+	bl	efi_entry
+	cmp	x0, EFI_STUB_ERROR
+	b.eq	efi_load_fail
+
+	/*
+	 * efi_entry() will return here with device tree address in x0.
+	 *  Save value in register which is preserved by __flush_dcache_all.
+	 */
+	mov	x20, x0
+
+	/* Turn off Dcache and MMU */
+	mrs	x0, CurrentEL
+	cmp	x0, #PSR_MODE_EL2t
+	ccmp	x0, #PSR_MODE_EL2h, #0x4, ne
+	b.ne	1f
+	mrs	x0, sctlr_el2
+	bic	x0, x0, #1 << 0	// clear SCTLR.M
+	bic	x0, x0, #1 << 2	// clear SCTLR.C
+	msr	sctlr_el2, x0
+	isb
+	b	2f
+1:
+	mrs	x0, sctlr_el1
+	bic	x0, x0, #1 << 0	// clear SCTLR.M
+	bic	x0, x0, #1 << 2	// clear SCTLR.C
+	msr	sctlr_el1, x0
+	isb
+2:
+/*	bl	__flush_dcache_all */  /* RFRANZ_TODO */
+
+	/* Jump to XEN entry point */
+	mov	x0, x20
+	mov	x1, xzr
+	mov	x2, xzr
+	mov	x3, xzr
+	b	real_start
+
+efi_load_fail:
+	mov	x0, #EFI_LOAD_ERROR
+	ldp	x29, x30, [sp], #32
+	ret
+
+ENDPROC(efi_stub_entry)
+
+
+
 /*
  * Local variables:
  * mode: ASM
diff --git a/xen/arch/arm/efi-shared.c b/xen/arch/arm/efi-shared.c
new file mode 120000
index 0000000..096ee3a
--- /dev/null
+++ b/xen/arch/arm/efi-shared.c
@@ -0,0 +1 @@
+../x86/efi/efi-shared.c
\ No newline at end of file
diff --git a/xen/arch/arm/efi.c b/xen/arch/arm/efi.c
new file mode 100644
index 0000000..d13e718
--- /dev/null
+++ b/xen/arch/arm/efi.c
@@ -0,0 +1,686 @@
+#include "efi.h"
+#include <efi/efiprot.h>
+#include <efi/eficon.h>
+#include <efi/efipciio.h>
+#include <efi/efi-shared.h>
+#include <public/xen.h>
+#include <xen/compile.h>
+#include <xen/ctype.h>
+#include <xen/init.h>
+#include <xen/keyhandler.h>
+#include <xen/lib.h>
+#include <xen/mm.h>
+#include <xen/pfn.h>
+#if EFI_PAGE_SIZE != PAGE_SIZE
+# error Cannot use xen/pfn.h here!
+#endif
+#include <xen/string.h>
+#include <xen/stringify.h>
+#include <xen/libfdt/libfdt.h>
+
+/*
+ * This define and the code associated with it is a temporary, and will
+ * be replaced by changes to the XEN kernel to use the EFI memory map
+ * that is passed in the device tree instead of the normal FDT memory map.
+ * The XEN kernel changes are largely indpendent of the stub, so this code
+ * has been added to allow review and some testing of the stub code while
+ * the XEN kernel code is being developed.
+ */
+#define ADD_EFI_MEMORY_TO_FDT
+
+#define DEVICE_TREE_GUID \
+{0xb1b621d5, 0xf19c, 0x41a5, {0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0}}
+
+extern CHAR16 __initdata newline[];
+extern SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdOut;
+extern SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdERR;
+extern EFI_BOOT_SERVICES *__initdata efi_bs;
+
+
+static EFI_HANDLE __initdata efi_ih;
+
+static struct file __initdata cfg;
+static struct file __initdata kernel;
+static struct file __initdata ramdisk;
+static struct file __initdata dtb;
+
+static unsigned long mmap_size;
+static EFI_MEMORY_DESCRIPTOR *mmap_ptr;
+
+static void *new_fdt;
+
+#ifdef ADD_EFI_MEMORY_TO_FDT
+static int IsLinuxReservedRegion(int MemoryType)
+{
+    switch ( MemoryType )
+    {
+        case EfiMemoryMappedIO:
+        case EfiMemoryMappedIOPortSpace:
+        case EfiRuntimeServicesCode:
+        case EfiRuntimeServicesData:
+        case EfiUnusableMemory:
+        case EfiACPIReclaimMemory:
+        case EfiACPIMemoryNVS:
+        case EfiLoaderData:
+            return 1;
+        default:
+            return 0;
+    }
+}
+
+
+static EFI_STATUS __init efi_process_memory_map(EFI_MEMORY_DESCRIPTOR *map,
+                                                unsigned long mmap_size,
+                                                unsigned long desc_size,
+                                                void *fdt)
+{
+    int Index;
+    int err;
+
+    EFI_MEMORY_DESCRIPTOR *desc_ptr = map;
+
+    /* Go through the list and add the reserved region to the Device Tree */
+    for ( Index = 0; Index < (mmap_size / desc_size); Index++ )
+    {
+        if ( IsLinuxReservedRegion(desc_ptr->Type) )
+        {
+            err = fdt_add_mem_rsv(fdt, desc_ptr->PhysicalStart, desc_ptr->NumberOfPages * EFI_PAGE_SIZE);
+            if ( err != 0 )
+                PrintStr(L"Warning: Fail to add 'memreserve'\n\n");
+        }
+        desc_ptr = NextMemoryDescriptor(desc_ptr, desc_size);
+    }
+
+    return EFI_SUCCESS;
+
+}
+#endif
+
+static EFI_STATUS __init efi_get_memory_map(EFI_SYSTEM_TABLE *sys_table_arg,
+                                            EFI_MEMORY_DESCRIPTOR **map,
+                                            unsigned long *mmap_size,
+                                            unsigned long *desc_size,
+                                            UINT32 *desc_ver,
+                                            unsigned long *key_ptr)
+{
+    EFI_MEMORY_DESCRIPTOR *m = NULL;
+    EFI_STATUS status;
+    unsigned long key;
+    u32 desc_version;
+
+    *mmap_size = sizeof(*m) * 32;
+again:
+    /*
+     * Add an additional 2 efi_memory_desc_t because we're doing an
+         * allocation which may be in a new descriptor region, and this
+         * may create additional entries.
+     */
+    *mmap_size += 2 * sizeof(*m);
+    status = sys_table_arg->BootServices->AllocatePool(EfiLoaderData,
+                                                       *mmap_size, (void **)&m);
+    if ( status != EFI_SUCCESS )
+        goto fail;
+
+    *desc_size = 0;
+    key = 0;
+    status = sys_table_arg->BootServices->GetMemoryMap(mmap_size, m,
+                                                       &key, desc_size, &desc_version);
+    if ( status == EFI_BUFFER_TOO_SMALL )
+    {
+        sys_table_arg->BootServices->FreePool(m);
+        goto again;
+    }
+
+    if ( status != EFI_SUCCESS )
+        sys_table_arg->BootServices->FreePool(m);
+
+    if ( key_ptr && status == EFI_SUCCESS )
+        *key_ptr = key;
+    if ( desc_ver && status == EFI_SUCCESS )
+        *desc_ver = desc_version;
+
+fail:
+    *map = m;
+    return status;
+}
+
+
+static void __init *lookup_fdt_config_table(EFI_SYSTEM_TABLE *sys_table)
+{
+    const EFI_GUID fdt_guid = DEVICE_TREE_GUID;
+    EFI_CONFIGURATION_TABLE *tables;
+    void *fdt;
+    int i;
+
+    tables = sys_table->ConfigurationTable;
+    fdt = NULL;
+
+    for ( i = 0; i < sys_table->NumberOfTableEntries; i++ ) if ( match_guid(&tables[i].VendorGuid, &fdt_guid) == 0 )
+        {
+            fdt = tables[i].VendorTable;
+            break;
+        }
+    return fdt;
+}
+
+/*
+ * Get (or set if not present) the #addr-cells and #size cells
+ * properties of the chose node.  We need to know these to properly
+ * construct the address ranges used to describe the files loaded
+ * by the stub.
+ */
+static int setup_chosen_node(void *fdt, int *addr_cells, int *size_cells)
+{
+    int node;
+    const struct fdt_property *prop;
+    int len;
+    uint32_t val;
+
+    if (!fdt || !addr_cells || !size_cells)
+        return -1;
+
+
+    /* locate chosen node, which is where we add XEN module info. */
+    node = fdt_subnode_offset(fdt, 0, "chosen");
+    if (node < 0)
+    {
+            node = fdt_add_subnode(fdt, 0, "chosen");
+            if (node < 0)
+                return node;
+    }
+
+    /* Get or set #address-cells and #size-cells */
+    prop = fdt_get_property(fdt, node, "#address-cells", &len);
+    if (!prop)
+    {
+        PrintStr(L"No #address-cells in chosen node, setting to 2\r\n");
+        val = cpu_to_fdt32(2);
+        if (fdt_setprop(fdt, node, "#address-cells", &val, sizeof(val)))
+            return -1;
+        *addr_cells = 2;
+    }
+    else
+        *addr_cells = fdt32_to_cpu(*((uint32_t *)prop->data));
+
+    prop = fdt_get_property(fdt, node, "#size-cells", &len);
+    if (!prop)
+    {
+        PrintStr(L"No #size-cells in chosen node, setting to 2\r\n");
+        val = cpu_to_fdt32(2);
+        if (fdt_setprop(fdt, node, "#size-cells", &val, sizeof(val)))
+            return -1;
+        *size_cells = 2;
+    }
+    else
+        *size_cells = fdt32_to_cpu(*((uint32_t *)prop->data));
+
+    /*
+     * Make sure ranges is empty if it exists, otherwise create empty ranges
+     * property.
+     */
+    prop = fdt_get_property(fdt, node, "ranges", &len);
+    if (!prop)
+    {
+        PrintStr(L"No ranges in chosen node, creating empty\r\n");
+        val = cpu_to_fdt32(2);
+        if (fdt_setprop(fdt, node, "#size-cells", &val, 0))
+            return -1;
+    }
+    else
+    {
+        if (fdt32_to_cpu(prop->len))
+        {
+            PrintStr(L"Non-empty ranges in chosen node, aborting\r\n");
+            return -1;
+        }
+    }
+    return node;
+}
+
+
+/*
+ * Set a single 'reg' property taking into account the
+ * configured addr and size cell sizes.
+ */
+static int fdt_set_reg(void *fdt, int node, int addr_cells, int size_cells,
+                       uint64_t addr, uint64_t len)
+{
+    uint8_t data[16]; /* at most 2 64 bit words */
+    void *p = data;
+
+    /* Make sure that the values provided can be represented in
+     * the reg property.
+     */
+    if (addr_cells == 1 && (addr >> 32))
+        return -1;
+    if (size_cells == 1 && (len >> 32))
+        return -1;
+
+    if (addr_cells == 1)
+    {
+        *(uint32_t *)p = cpu_to_fdt32(addr);
+        p += sizeof(uint32_t);
+    }
+    else if (addr_cells == 2)
+    {
+        *(uint64_t *)p = cpu_to_fdt64(addr);
+        p += sizeof(uint64_t);
+    }
+    else
+        return -1;
+
+
+    if (size_cells == 1)
+    {
+        *(uint32_t *)p = cpu_to_fdt32(len);
+        p += sizeof(uint32_t);
+    }
+    else if (size_cells == 2)
+    {
+        *(uint64_t *)p = cpu_to_fdt64(len);
+        p += sizeof(uint64_t);
+    }
+    else
+        return -1;
+
+    return(fdt_setprop(fdt, node, "reg", data, p - (void *)data));
+}
+
+/*
+ * Add the FDT nodes for the standard EFI information, which consist
+ * of the System table address, the address of the final EFI memory map,
+ * and memory map information.
+ */
+static EFI_STATUS fdt_add_uefi_nodes(EFI_SYSTEM_TABLE *sys_table, void *fdt,
+                               EFI_MEMORY_DESCRIPTOR *memory_map,
+                               unsigned long map_size, unsigned long desc_size,
+                               u32 desc_ver)
+{
+    int node;
+    int status;
+    u32 fdt_val32;
+    u64 fdt_val64;
+
+#ifndef ADD_EFI_MEMORY_TO_FDT
+    int prev;
+    /*
+     * Delete any memory nodes present.  The EFI memory map is the only
+     * memory description provided to XEN.
+     */
+    prev = 0;
+    for (;;)
+    {
+        const char *type;
+        int len;
+
+        node = fdt_next_node(fdt, prev, NULL);
+        if (node < 0)
+        break;
+
+        type = fdt_getprop(fdt, node, "device_type", &len);
+        if (type && strncmp(type, "memory", len) == 0)
+        {
+            fdt_del_node(fdt, node);
+            continue;
+        }
+
+        prev = node;
+    }
+#endif
+
+    node = fdt_subnode_offset(fdt, 0, "chosen");
+    if ( node < 0 )
+    {
+        node = fdt_add_subnode(fdt, 0, "chosen");
+        if ( node < 0 )
+        {
+            status = node; /* node is error code when negative */
+            goto fdt_set_fail;
+        }
+    }
+
+    /* Add FDT entries for EFI runtime services in chosen node. */
+    node = fdt_subnode_offset(fdt, 0, "chosen");
+    fdt_val64 = cpu_to_fdt64((u64)(unsigned long)sys_table);
+    status = fdt_setprop(fdt, node, "linux,uefi-system-table",
+                         &fdt_val64, sizeof(fdt_val64));
+    if ( status )
+        goto fdt_set_fail;
+
+    fdt_val64 = cpu_to_fdt64((u64)(unsigned long)memory_map);
+    status = fdt_setprop(fdt, node, "linux,uefi-mmap-start",
+                         &fdt_val64,  sizeof(fdt_val64));
+    if ( status )
+        goto fdt_set_fail;
+
+    fdt_val32 = cpu_to_fdt32(map_size);
+    status = fdt_setprop(fdt, node, "linux,uefi-mmap-size",
+                         &fdt_val32,  sizeof(fdt_val32));
+    if ( status )
+        goto fdt_set_fail;
+
+    fdt_val32 = cpu_to_fdt32(desc_size);
+    status = fdt_setprop(fdt, node, "linux,uefi-mmap-desc-size",
+                         &fdt_val32, sizeof(fdt_val32));
+    if ( status )
+        goto fdt_set_fail;
+
+    fdt_val32 = cpu_to_fdt32(desc_ver);
+    status = fdt_setprop(fdt, node, "linux,uefi-mmap-desc-ver",
+                         &fdt_val32, sizeof(fdt_val32));
+    if ( status )
+        goto fdt_set_fail;
+
+    return EFI_SUCCESS;
+
+fdt_set_fail:
+    if ( status == -FDT_ERR_NOSPACE )
+        return EFI_BUFFER_TOO_SMALL;
+
+    return EFI_LOAD_ERROR;
+}
+
+
+
+/*
+ * Allocates new memory for a larger FDT, and frees existing memory if
+ * struct file size is non-zero.  Updates file struct with new memory
+ * address/size for later freeing.  If fdtfile.ptr is NULL, an empty FDT
+ * is created.
+ */
+static void __init *fdt_increase_size(struct file *fdtfile, int add_size)
+{
+    EFI_STATUS status;
+    EFI_PHYSICAL_ADDRESS fdt_addr;
+    int fdt_size;
+    int pages;
+    void *new_fdt;
+
+
+    if ( fdtfile->ptr )
+        fdt_size = fdt_totalsize(fdtfile->ptr);
+    else
+    {
+        /* RFRANZ_TODO - missing fdt_create_empty_tree() in libfdt. */
+        return NULL;
+    }
+
+    pages = PFN_UP(fdt_size) + PFN_UP(add_size);
+    status = efi_bs->AllocatePages(AllocateAnyPages, EfiLoaderData,
+                                   pages, &fdt_addr);
+
+    if ( status != EFI_SUCCESS )
+        return NULL;
+
+    new_fdt = (void *)fdt_addr;
+
+    if ( fdt_open_into(dtb.ptr, new_fdt, pages * EFI_PAGE_SIZE) )
+        return NULL;
+
+    /*
+     * Now that we have the new FDT allocated and copied, free the
+     * original and update the struct file so that the error handling
+     * code will free it.  If the original FDT came from a configuration
+     * table, we don't own that memory and can't free it.
+     */
+    if ( dtb.size )
+        efi_bs->FreePages(dtb.addr, PFN_UP(dtb.size));
+
+    /* Update 'file' info for new memory so we clean it up on error exits */
+    dtb.addr = fdt_addr;
+    dtb.size = pages * EFI_PAGE_SIZE;
+    return new_fdt;
+}
+
+
+/*
+ * Allocate a new FDT with enough space for EFI and XEN related updates,
+ * populating with content from a FDT specified in the configuration file
+ * or configuration table if present.  If neither is available, create an
+ * empty FDT.
+ */
+static void __init *create_new_fdt(EFI_SYSTEM_TABLE *SystemTable,
+                            EFI_FILE_HANDLE dir_handle, struct file *cfgfile,
+                            const char *section)
+{
+    union string name = { NULL };
+
+    /* load dtb from config file or configuration table */
+    name.s = get_value(cfgfile, section, "dtb");
+    if ( name.s )
+    {
+        truncate_string(name.s);
+        read_file(dir_handle, s2w(&name), &dtb);
+        efi_bs->FreePool(name.w);
+    }
+    else
+    {
+        /* Get DTB from configuration table. */
+        dtb.ptr = lookup_fdt_config_table(SystemTable);
+        if ( dtb.ptr )
+        {
+            /* Set dtb.size to zero so config table memory is not freed. */
+            dtb.size = 0;
+        }
+        else
+        {
+            /*
+             * No FDT was provided, so create an empty one to populate with
+             * the EFI and module related params.
+             *
+             * RFRANZ_TODO: libfdt in XEN is missing fdt_create_empty_tree()
+             * likely need libfdt update to support this.
+             */
+            blexit(L"No FDT specified.");
+        }
+    }
+
+
+    /*
+     * Allocate space for new FDT, making sure we have enough space
+     * for the fields we are adding, so we don't have to deal
+     * with increasing the size again later, which complicates
+     * things.  Use the size of the configuration file as an uppper
+     * bound on how much size can be added based on configuration
+     * file contents.
+     */
+    return fdt_increase_size(&dtb, cfg.size + EFI_PAGE_SIZE);
+}
+
+
+#define COMPAT_BUF_SIZE 500 /* FDT string buffer size. */
+unsigned long efi_entry(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
+{
+    EFI_GUID loaded_image_guid = LOADED_IMAGE_PROTOCOL;
+    EFI_LOADED_IMAGE *loaded_image;
+    EFI_FILE_HANDLE dir_handle;
+    EFI_STATUS status;
+    union string section = { NULL }, cmdline = { NULL }, name = { NULL };
+    CHAR16 * file_name,*cfg_file_name = NULL,*image_name = NULL;
+    bool_t base_video = 0;
+    int node;
+    int chosen;
+    int addr_len, size_len;
+    char *options;
+    char compat_buf[COMPAT_BUF_SIZE];
+    int compat_len = 0;
+    unsigned long desc_size;
+    UINT32 desc_ver = 0;
+    unsigned long map_key = 0;
+
+    efi_ih = ImageHandle;
+    efi_bs = SystemTable->BootServices;
+    StdOut = SystemTable->ConOut;
+
+    /* Check if we were booted by the EFI firmware */
+    if ( SystemTable->Hdr.Signature != EFI_SYSTEM_TABLE_SIGNATURE )
+        goto fail;
+
+    /* Get loaded image protocol */
+    status = efi_bs->HandleProtocol(ImageHandle, &loaded_image_guid,
+                                    (void **)&loaded_image);
+    if ( status != EFI_SUCCESS )
+        blexit(L"ERROR - no loaded image protocol\r\n");
+
+    PrintStr(L"Xen " __stringify(XEN_VERSION)"." __stringify(XEN_SUBVERSION)
+             XEN_EXTRAVERSION " (c/s " XEN_CHANGESET ") EFI loader\r\n");
+
+    if ( (unsigned long)loaded_image->ImageBase & ((1 << 20) - 1) )
+        blexit(L"Xen must be loaded at a 2MByte boundary.");
+
+    /* Get the file system interface. */
+    dir_handle = get_parent_handle(loaded_image, &file_name);
+
+    handle_cmdline(loaded_image, &cfg_file_name, &base_video, &image_name,
+                   &section.w, &cmdline.w);
+
+    if ( cmdline.w )
+        w2s(&cmdline);
+
+    /* Open and read config file */
+    read_config_file(&dir_handle, &cfg, cfg_file_name,
+                     &section, file_name);
+
+    new_fdt = create_new_fdt(SystemTable, dir_handle, &cfg, section.s);
+    if ( !new_fdt )
+        blexit(L"Unable to create new FDT\r\n");
+
+    chosen = setup_chosen_node(new_fdt, &addr_len, &size_len);
+    if ( chosen < 0 )
+        blexit(L"Unable to setup chosen node\r\n");
+
+
+    name.s = get_value(&cfg, section.s, "kernel");
+    if ( !name.s )
+        blexit(L"No Dom0 kernel image specified.");
+    options = truncate_string(name.s);
+    if ( options )
+        fdt_setprop_string(new_fdt, chosen, "xen,dom0-bootargs", options);
+    s2w(&name);
+    read_file(dir_handle, name.w, &kernel);
+
+    node = fdt_add_subnode(new_fdt, chosen, "kernel");
+    if ( node < 0 )
+        blexit(L"Error adding dom0 FDT node.");
+
+    compat_len = 0;
+    compat_len += snprintf(compat_buf + compat_len, COMPAT_BUF_SIZE - compat_len, "xen,linux-zimage") + 1;
+    if ( compat_len > COMPAT_BUF_SIZE )
+        blexit(L"FDT string overflow");
+    compat_len += snprintf(compat_buf + compat_len, COMPAT_BUF_SIZE - compat_len, "xen,multiboot-module") + 1;
+    if ( compat_len > COMPAT_BUF_SIZE )
+        blexit(L"FDT string overflow");
+    if ( fdt_setprop(new_fdt, node, "compatible", compat_buf, compat_len) < 0 )
+        blexit(L"unable to set compatible property.");
+    fdt_set_reg(new_fdt, node, addr_len, size_len, kernel.addr, kernel.size);
+    efi_bs->FreePool(name.w);
+
+
+    name.s = get_value(&cfg, section.s, "ramdisk");
+    if ( name.s )
+    {
+        truncate_string(name.s);
+        read_file(dir_handle, s2w(&name), &ramdisk);
+
+        node = fdt_add_subnode(new_fdt, chosen, "ramdisk");
+        if ( node < 0 )
+            blexit(L"Error adding ramdisk FDT node.");
+
+        compat_len = 0;
+        compat_len += snprintf(compat_buf + compat_len, COMPAT_BUF_SIZE - compat_len, "xen,linux-initrd") + 1;
+        if ( compat_len > COMPAT_BUF_SIZE )
+            blexit(L"FDT string overflow");
+        compat_len += snprintf(compat_buf + compat_len, COMPAT_BUF_SIZE - compat_len, "xen,multiboot-module") + 1;
+        if ( compat_len > COMPAT_BUF_SIZE )
+            blexit(L"FDT string overflow");
+        if ( fdt_setprop(new_fdt, node, "compatible", compat_buf, compat_len) < 0 )
+            blexit(L"unable to set compatible property.");
+        fdt_set_reg(new_fdt, node, addr_len, size_len, ramdisk.addr, ramdisk.size);
+        efi_bs->FreePool(name.w);
+    }
+
+
+    /*
+     * cmdline has remaining options from EFI command line.  Prepend these
+     * to the options from the configuration file.  Put the image name at
+     * the beginning of the bootargs.
+     *
+     */
+    if ( image_name )
+    {
+        name.w = image_name;
+        w2s(&name);
+    }
+    else
+        name.s = "xen";
+
+    compat_len = 0;
+    compat_len += snprintf(compat_buf + compat_len, COMPAT_BUF_SIZE - compat_len, "%s", name.s);
+    if ( compat_len >= COMPAT_BUF_SIZE )
+        blexit(L"FDT string overflow");
+    if ( cmdline.s )
+    {
+        compat_len += snprintf(compat_buf + compat_len, COMPAT_BUF_SIZE - compat_len, " %s", cmdline.s);
+        if ( compat_len >= COMPAT_BUF_SIZE )
+            blexit(L"FDT string overflow");
+    }
+    name.s = get_value(&cfg, section.s, "options");
+    if ( name.s )
+    {
+        compat_len += snprintf(compat_buf + compat_len, COMPAT_BUF_SIZE - compat_len, " %s", name.s);
+        if ( compat_len >= COMPAT_BUF_SIZE )
+            blexit(L"FDT string overflow");
+    }
+
+
+    if ( fdt_setprop_string(new_fdt, chosen, "xen,xen-bootargs", compat_buf) < 0 )
+        blexit(L"unable to set xen,xen-bootargs property.");
+
+    status = efi_get_memory_map(SystemTable, &mmap_ptr, &mmap_size, &desc_size, &desc_ver, &map_key);
+    if ( status != EFI_SUCCESS )
+        blexit(L"unable to get EFI memory map");
+
+#ifdef ADD_EFI_MEMORY_TO_FDT
+    efi_process_memory_map(mmap_ptr, mmap_size, desc_size, new_fdt);
+#endif
+
+    status = fdt_add_uefi_nodes(SystemTable, new_fdt, mmap_ptr, mmap_size, desc_size, desc_ver);
+    if ( status != EFI_SUCCESS )
+    {
+        if ( status == EFI_BUFFER_TOO_SMALL )
+            PrintStr(L"ERROR: FDT buffer too small\r\n");
+        blexit(L"Unable to create new FDT with UEFI nodes");
+    }
+
+    status = efi_bs->ExitBootServices(ImageHandle, map_key);
+    if ( status != EFI_SUCCESS )
+        blexit(L"Unable to exit boot services.");
+
+    return((unsigned long)new_fdt);
+
+
+fail:
+    blexit(L"ERROR: Unable to start XEN\r\n");
+}
+
+
+void __init noreturn blexit(const CHAR16 *str)
+{
+    if ( str )
+        PrintStr((CHAR16 *)str);
+    PrintStr(newline);
+
+    if ( cfg.addr )
+        efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
+    if ( kernel.addr )
+        efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size));
+    if ( ramdisk.addr )
+        efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size));
+    if ( dtb.addr && dtb.size )
+        efi_bs->FreePages(dtb.addr, PFN_UP(dtb.size));
+    if ( mmap_ptr )
+        efi_bs->FreePool(mmap_ptr);
+
+    efi_bs->Exit(efi_ih, EFI_SUCCESS, 0, NULL);
+    unreachable(); /* not reached */
+}
-- 
2.0.0

^ permalink raw reply related	[flat|nested] 44+ messages in thread

* Re: [PATCH RFC 00/19] arm64 EFI stub
  2014-06-28  1:25 [PATCH RFC 00/19] arm64 EFI stub Roy Franz
                   ` (18 preceding siblings ...)
  2014-06-28  1:25 ` [PATCH RFC 19/19] Add EFI stub for ARM64 Roy Franz
@ 2014-06-28 15:34 ` Roy Franz
  2014-06-29 16:42 ` [Linaro-uefi] " Julien Grall
                   ` (2 subsequent siblings)
  22 siblings, 0 replies; 44+ messages in thread
From: Roy Franz @ 2014-06-28 15:34 UTC (permalink / raw)
  To: xen-devel, Ian Campbell, Stefano Stabellini, tim, Jan Beulich, keir
  Cc: Roy Franz, Fu Wei, linaro-uefi

On Fri, Jun 27, 2014 at 6:25 PM, Roy Franz <roy.franz@linaro.org> wrote:
>
> This patch series is an RFC series that adds EFI support for arm64 in the
> form of a EFI stub in similar fashion to the linux kernel EFI support.
> A PE/COFF header is created in head.S, as there is no toolchain support
> for PE/COFF on arm64.  This also has the advantage that the file is both
> an "Image" file and a PE/COFF executable - the same binary can be loaded
> and run either way.  The EFI 'stub' code is a shim layer that serves as
> the loader for the XEN kernel in the EFI environment.  The stub loads
> the dom0 kernel and initrd if required, and adds entries for them as well
> as for the EFI data structures into the device tree passed to XEN.  Once
> the device tree is constructed, EFI boot services are exited, and the stub
> transfers control to the normal XEN entry point.  The only indication
> XEN has that it was loaded via the stub is that the device tree contains
> EFI properties.  This is all very similar to the arm/arm64 Linux kernel
> EFI stubs.
>
>
> The first 14 patches refactor and move x86 EFI code so that it can be
> shared with the arm64 EFI stub.  The remaining 5 patches add the new
> arm64 EFI stub.
>
> One significant omission from this series is proper EFI memory map
> handling in XEN itself.  This patch instead creates FDT memory nodes based
> on the EFI memory map.  This is functional, but not how we want to do
> it long term.  The XEN kernel updates for this will be largely
> disjoint from this series, and I will be starting on that next.  I wanted
> to get this portion out for review without waiting for that portion of the
> code to be done.  The ADD_EFI_MEMORY_TO_FDT macro isolates code that will
> change when the proper EFI memory map handling is added to XEN.
>
> I have some some simple tests booting from the EFI shell on arm64 (FVP
> base model) and x86_64 (vmware.)
>
> This patch series is available on my git tree:
> git://git.linaro.org/people/roy.franz/xen.git
> tag: xen-efi-stub-rfc-20140627
>
>
> There are a few open issues in this patch series that I would appreciate
> feedback/suggestions on:
>
> 1) Build system changes.  The 'efi-shared.c' file should be properly
> shared, rather than symbolicly linked.  One complication is that the EFI
> code (for both archs) needs to be compiled with "-fshort-wchar".  I also
> likely need to create an efi subdir for arm64.
>
> 2) Is it valid to start XEN with a device tree that only contains
> multi-boot and EFI info? (As would be the case if the stub doesn't get a
> device tree as input.)  Currently this isn't supported, some libfdt
> functions are missing, so I'm checking if this is desired before I add
> that.
>
> 3) I'm not sure arm64 needs it's own copy of efibind.h.  The x86_64
> version worked fine as is, but has some Microsoft related defines in
> there.  The arm64 version I created is a proper subset with the exception
> of the EFI_STUB_ERROR define.
>
Also, the ASM wrapper needs to flush the DCACHE as part of transitioning from
the EFI environment to the processor state XEN expects.  (My understanding is
that it is like Linux in that regard.)
>
> (I am on vacation the week of June 30th-July 4th, so my responses will be
> delayed.)
>
> Roy Franz (19):
>   HACK: Add -fshort-wchar to global build
>   Create efi-shared.[ch], and move string functions
>   Move more functions from boot.c to efi-shared.c
>   rename printErrMsg to PrintErrMesgExit
>   Add PrintErrMesg function that doesn't exit
>   Refactor read_file() so it can be shared.
>   move read_file() to efi-shared.c
>   Move removal of leading spaces from split_value to get_value()
>   replace split_value() with truncate_string()
>   move truncate_string() to efi-shared.c
>   add read_config_file() function for XEN EFI config file
>   create handle_cmdline() function
>   Refactor get_argv() for sharing
>   Move get_argv() and handle_cmdline() to efi-shared.c
>   Add PE/COFF header in head.S
>   create ARM EFI headers, based on x86
>   Remove x86 specific defintions from efibind.h
>   Add assembler use support for efibind.h
>   Add EFI stub for ARM64
>
>  Config.mk                           |   2 +
>  xen/arch/arm/Makefile               |   2 +
>  xen/arch/arm/arm64/head.S           | 178 +++++++++-
>  xen/arch/arm/efi-shared.c           |   1 +
>  xen/arch/arm/efi.c                  | 686 ++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/efi.h                  |  11 +
>  xen/arch/arm/xen.lds.S              |   1 +
>  xen/arch/x86/Makefile               |   4 +-
>  xen/arch/x86/efi/Makefile           |   2 +-
>  xen/arch/x86/efi/boot.c             | 614 +++-----------------------------
>  xen/arch/x86/efi/efi-shared.c       | 620 ++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/arm64/efibind.h | 206 +++++++++++
>  xen/include/asm-arm/efibind.h       |   2 +
>  xen/include/efi/efi-shared.h        |  71 ++++
>  14 files changed, 1832 insertions(+), 568 deletions(-)
>  create mode 120000 xen/arch/arm/efi-shared.c
>  create mode 100644 xen/arch/arm/efi.c
>  create mode 100644 xen/arch/arm/efi.h
>  create mode 100644 xen/arch/x86/efi/efi-shared.c
>  create mode 100644 xen/include/asm-arm/arm64/efibind.h
>  create mode 100644 xen/include/asm-arm/efibind.h
>  create mode 100644 xen/include/efi/efi-shared.h
>
> --
> 2.0.0
>

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [Linaro-uefi] [PATCH RFC 00/19] arm64 EFI stub
  2014-06-28  1:25 [PATCH RFC 00/19] arm64 EFI stub Roy Franz
                   ` (19 preceding siblings ...)
  2014-06-28 15:34 ` [PATCH RFC 00/19] arm64 EFI stub Roy Franz
@ 2014-06-29 16:42 ` Julien Grall
  2014-06-30 11:12 ` Jan Beulich
  2014-07-02 11:52 ` Ian Campbell
  22 siblings, 0 replies; 44+ messages in thread
From: Julien Grall @ 2014-06-29 16:42 UTC (permalink / raw)
  To: Roy Franz, xen-devel, ian.campbell, stefano.stabellini, tim,
	jbeulich, keir
  Cc: linaro-uefi

Hi Roy,

Thank you for this series. I haven't look closely to this series but 
I've tried to compile it for arm32 and it's fails.

It looks like the EFI stub is also included for arm32, which you don't 
plan to support right?

I would use your CONFIG_EFI_STUB you've hardcoded in arm64/head.s to 
know if we need to include the EFI stub or not in the compilation.

On 28/06/14 02:25, Roy Franz wrote:
> There are a few open issues in this patch series that I would appreciate
> feedback/suggestions on:
>
> 1) Build system changes.  The 'efi-shared.c' file should be properly
> shared, rather than symbolicly linked.  One complication is that the EFI
> code (for both archs) needs to be compiled with "-fshort-wchar".  I also
> likely need to create an efi subdir for arm64.

I would create a directly xen/drivers/efi so you can move efi code in there.

Hence, you will be able to use -fshort-wchar only for this directory.

> 2) Is it valid to start XEN with a device tree that only contains
> multi-boot and EFI info? (As would be the case if the stub doesn't get a
> device tree as input.)  Currently this isn't supported, some libfdt
> functions are missing, so I'm checking if this is desired before I add
> that.
>
> 3) I'm not sure arm64 needs it's own copy of efibind.h.  The x86_64
> version worked fine as is, but has some Microsoft related defines in
> there.  The arm64 version I created is a proper subset with the exception
> of the EFI_STUB_ERROR define.

Maybe you can create a new header in include/efi and use asm/efi.h for 
architectural bindings.

Regards,

-- 
Julien Grall

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH RFC 00/19] arm64 EFI stub
  2014-06-28  1:25 [PATCH RFC 00/19] arm64 EFI stub Roy Franz
                   ` (20 preceding siblings ...)
  2014-06-29 16:42 ` [Linaro-uefi] " Julien Grall
@ 2014-06-30 11:12 ` Jan Beulich
  2014-07-02 11:52 ` Ian Campbell
  22 siblings, 0 replies; 44+ messages in thread
From: Jan Beulich @ 2014-06-30 11:12 UTC (permalink / raw)
  To: Roy Franz
  Cc: keir, ian.campbell, tim, xen-devel, stefano.stabellini,
	linaro-uefi, fu.wei

>>> On 28.06.14 at 03:25, <roy.franz@linaro.org> wrote:
> 1) Build system changes.  The 'efi-shared.c' file should be properly 
> shared, rather than symbolicly linked.  One complication is that the EFI 
> code (for both archs) needs to be compiled with "-fshort-wchar".  I also 
> likely need to create an efi subdir for arm64.

If it's going to be just a single file (which I don't think it'll be in the
end), it should just be xen/common/efi.c. If there are multiple ones
(and I'd expect most of xen/arch/x86/efi/ to be re-usable), you
should just move all non-x86 code to xen/common/efi/.

> 3) I'm not sure arm64 needs it's own copy of efibind.h.  The x86_64 
> version worked fine as is, but has some Microsoft related defines in 
> there.  The arm64 version I created is a proper subset with the exception 
> of the EFI_STUB_ERROR define.

Yes, I'm certainly of the opinion that each architecture should have
its own binding header, mirroring what (afaik) EFI itself does.

Jan

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH RFC 01/19] HACK: Add -fshort-wchar to global build
  2014-06-28  1:25 ` [PATCH RFC 01/19] HACK: Add -fshort-wchar to global build Roy Franz
@ 2014-06-30 11:13   ` Jan Beulich
  0 siblings, 0 replies; 44+ messages in thread
From: Jan Beulich @ 2014-06-30 11:13 UTC (permalink / raw)
  To: Roy Franz
  Cc: keir, ian.campbell, tim, xen-devel, stefano.stabellini,
	linaro-uefi, fu.wei

>>> On 28.06.14 at 03:25, <roy.franz@linaro.org> wrote:
> The EFI related code needs to be buil with "-fshort-wchar", as EFI
> defines 16 bit wide characters and uses them in various APIs.
> This fixes the prelink target build failures in the arch/x86 directory,
> and is needed for the ARM EFI stub files as well.  The prelink build
> failures may only happen during parallel builds.
> 
> This will be removed as part of getting the x86 and ARM archs to share
> code and have it built correctly.

And I'm sure if you ordered things suitable this could be avoided.
IOW I'm not seeing this change go in.

Jan

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH RFC 02/19] Create efi-shared.[ch], and move string functions
  2014-06-28  1:25 ` [PATCH RFC 02/19] Create efi-shared.[ch], and move string functions Roy Franz
@ 2014-06-30 11:15   ` Jan Beulich
  2014-07-02 11:49     ` Ian Campbell
  2014-07-02 11:55     ` Ian Campbell
  0 siblings, 2 replies; 44+ messages in thread
From: Jan Beulich @ 2014-06-30 11:15 UTC (permalink / raw)
  To: Roy Franz
  Cc: keir, ian.campbell, tim, xen-devel, stefano.stabellini,
	linaro-uefi, fu.wei

>>> On 28.06.14 at 03:25, <roy.franz@linaro.org> wrote:
> Create the files for EFI code that will be shared with the ARM stub,
> and move string functions and some global variables.
> 
> TODO: proper sharing of file (with proper -fshort-char compile option)

Already at this point of the series I wonder what the value of
posting it is if even the basic question of placement within the tree
hasn't been sorted out yet. What are reviewers expected to review
(considering that code should just be getting moved, not modified)?

Jan

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH RFC 02/19] Create efi-shared.[ch], and move string functions
  2014-06-30 11:15   ` Jan Beulich
@ 2014-07-02 11:49     ` Ian Campbell
  2014-07-02 11:55     ` Ian Campbell
  1 sibling, 0 replies; 44+ messages in thread
From: Ian Campbell @ 2014-07-02 11:49 UTC (permalink / raw)
  To: Jan Beulich
  Cc: keir, tim, xen-devel, Roy Franz, stefano.stabellini, linaro-uefi, fu.wei

On Mon, 2014-06-30 at 12:15 +0100, Jan Beulich wrote:
> >>> On 28.06.14 at 03:25, <roy.franz@linaro.org> wrote:
> > Create the files for EFI code that will be shared with the ARM stub,
> > and move string functions and some global variables.
> > 
> > TODO: proper sharing of file (with proper -fshort-char compile option)
> 
> Already at this point of the series I wonder what the value of
> posting it is if even the basic question of placement within the tree
> hasn't been sorted out yet. What are reviewers expected to review
> (considering that code should just be getting moved, not modified)?

I think feedback on the correct placement is useful feedback at this
stage. I think you gave advise in the 00 mail though (xen/common/efi)
which makes sense to me too.

Ian.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH RFC 00/19] arm64 EFI stub
  2014-06-28  1:25 [PATCH RFC 00/19] arm64 EFI stub Roy Franz
                   ` (21 preceding siblings ...)
  2014-06-30 11:12 ` Jan Beulich
@ 2014-07-02 11:52 ` Ian Campbell
  2014-07-02 12:31   ` Ian Campbell
  22 siblings, 1 reply; 44+ messages in thread
From: Ian Campbell @ 2014-07-02 11:52 UTC (permalink / raw)
  To: Roy Franz
  Cc: keir, tim, xen-devel, stefano.stabellini, jbeulich, linaro-uefi, fu.wei

On Sat, 2014-06-28 at 02:25 +0100, Roy Franz wrote:
> 2) Is it valid to start XEN with a device tree that only contains 
> multi-boot and EFI info? (As would be the case if the stub doesn't get a 
> device tree as input.)

This would be the case when running on an ACPI system I guess? In which
case everything which would be in the DT would instead be in the ACPI
tables, which is something Naresh is working on I think.

>   Currently this isn't supported, some libfdt 
> functions are missing, so I'm checking if this is desired before I add 
> that.

Unless this==booting with ACPI I'm not sure what it is and if its ACPI
I'm not sure what needs to be done in the stub, could you explain
please?

(I have a feeling I'm missing something obvious)

Ian.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH RFC 02/19] Create efi-shared.[ch], and move string functions
  2014-06-30 11:15   ` Jan Beulich
  2014-07-02 11:49     ` Ian Campbell
@ 2014-07-02 11:55     ` Ian Campbell
  2014-07-09 18:31       ` Roy Franz
  1 sibling, 1 reply; 44+ messages in thread
From: Ian Campbell @ 2014-07-02 11:55 UTC (permalink / raw)
  To: Jan Beulich
  Cc: keir, tim, xen-devel, Roy Franz, stefano.stabellini, linaro-uefi, fu.wei

On Mon, 2014-06-30 at 12:15 +0100, Jan Beulich wrote:
> >>> On 28.06.14 at 03:25, <roy.franz@linaro.org> wrote:
> > Create the files for EFI code that will be shared with the ARM stub,
> > and move string functions and some global variables.
> > 
> > TODO: proper sharing of file (with proper -fshort-char compile option)
> 
> Already at this point of the series I wonder what the value of
> posting it is if even the basic question of placement within the tree
> hasn't been sorted out yet. What are reviewers expected to review
> (considering that code should just be getting moved, not modified)?

FWIW I found that reading the remainder of the series with an implicit
s,xen/arch/x86/efi/efi-shared.c,xen/common/efi/blah.c in my head was
worthwhile -- there's some useful refactoring in here as well as the
code motion into efi-shared^Wblah.c.

Ian.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH RFC 15/19] Add PE/COFF header in head.S
  2014-06-28  1:25 ` [PATCH RFC 15/19] Add PE/COFF header in head.S Roy Franz
@ 2014-07-02 12:02   ` Ian Campbell
  2014-07-09 18:35     ` Roy Franz
  0 siblings, 1 reply; 44+ messages in thread
From: Ian Campbell @ 2014-07-02 12:02 UTC (permalink / raw)
  To: Roy Franz
  Cc: keir, tim, xen-devel, stefano.stabellini, jbeulich, linaro-uefi, fu.wei

On Sat, 2014-06-28 at 02:25 +0100, Roy Franz wrote:
> This patch adds a simple PE/COFF header in head.S.  PE/COFF linker support
> is not available for arm64, so a native build is not possible.  Also, this
> allows the binary to be both a PE/COFF EFI application, and a normal Image
> file bootable like a Linux kernel.  The arm and arm64 Linux kernels use the
> same methodology to create a single image that is both an EFI application and
> a zImage/Image file.

This looks good to me. A few minor comments:

Some of the whitespace is off (please use space and not hard tabs
throughout)

I think we could perhaps get rid of CONFIG_EFI_STUB at least at the
arm64 subarch level. We don't in general support the same level of
configurability in Xen as Linux -- it's mostly just a single
configuration for everyone. It'd need to be retain at the arch level to
cope with arm32 not doing efi, but removing it here would make things
simpler I think?

Would it be possible to find sort of link/reference to a spec for the
header structs which you are adding and stick them in comments before
each one please.

> 
> Signed-off-by: Roy Franz <roy.franz@linaro.org>
> ---
>  xen/arch/arm/arm64/head.S | 118 +++++++++++++++++++++++++++++++++++++++++++++-
>  xen/arch/arm/xen.lds.S    |   1 +
>  2 files changed, 117 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 2a13527..9b88aeb 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -84,7 +84,7 @@
>  #endif /* !CONFIG_EARLY_PRINTK */
>  
>          /*.aarch64*/
> -
> +#define CONFIG_EFI_STUB
>          /*
>           * Kernel startup entry point.
>           * ---------------------------
> @@ -100,12 +100,24 @@
>           */
>  
>          .global start
> +efi_stub_entry:  /* Dummy symbol so we can compile before actual stub added */
>  start:
> +#ifdef CONFIG_EFI_STUB
>          /*
>           * DO NOT MODIFY. Image header expected by Linux boot-loaders.
>           */
> -        b       real_start           /* branch to kernel start, magic */
> +efi_head:
> +        /*
> +         * This add instruction has no meaningful effect except that
> +         * its opcode forms the magic "MZ" signature of a PE/COFF file
> +	 * that is required for UEFI applications.
> +         */
> +        add     x13, x18, #0x16

I love this hack ;-)

Ian.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH RFC 17/19] Remove x86 specific defintions from efibind.h
  2014-06-28  1:25 ` [PATCH RFC 17/19] Remove x86 specific defintions from efibind.h Roy Franz
@ 2014-07-02 12:03   ` Ian Campbell
  2014-07-09 18:27     ` Roy Franz
  0 siblings, 1 reply; 44+ messages in thread
From: Ian Campbell @ 2014-07-02 12:03 UTC (permalink / raw)
  To: Roy Franz
  Cc: keir, tim, xen-devel, stefano.stabellini, jbeulich, linaro-uefi, fu.wei

On Sat, 2014-06-28 at 02:25 +0100, Roy Franz wrote:
> Clean up arm64 efibind.h to not include x86 specific definitions.

For the majority of this series I really appreciate the refactor in one
patch and then move in another approach, but I think for this specific
case you could fold these together into a single "introduce the arm64
efi bindings" patch.

Ian.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH RFC 00/19] arm64 EFI stub
  2014-07-02 11:52 ` Ian Campbell
@ 2014-07-02 12:31   ` Ian Campbell
  2014-07-09 18:24     ` Roy Franz
  0 siblings, 1 reply; 44+ messages in thread
From: Ian Campbell @ 2014-07-02 12:31 UTC (permalink / raw)
  To: Roy Franz
  Cc: keir, tim, xen-devel, stefano.stabellini, jbeulich, linaro-uefi, fu.wei

On Wed, 2014-07-02 at 12:52 +0100, Ian Campbell wrote:
> On Sat, 2014-06-28 at 02:25 +0100, Roy Franz wrote:
> > 2) Is it valid to start XEN with a device tree that only contains 
> > multi-boot and EFI info? (As would be the case if the stub doesn't get a 
> > device tree as input.)
> 
> This would be the case when running on an ACPI system I guess? In which
> case everything which would be in the DT would instead be in the ACPI
> tables, which is something Naresh is working on I think.
> 
> >   Currently this isn't supported, some libfdt 
> > functions are missing, so I'm checking if this is desired before I add 
> > that.
> 
> Unless this==booting with ACPI I'm not sure what it is and if its ACPI
> I'm not sure what needs to be done in the stub, could you explain
> please?
> 
> (I have a feeling I'm missing something obvious)

Ah, I think this is from the final patch:

> +    if ( fdtfile->ptr )
> +        fdt_size = fdt_totalsize(fdtfile->ptr);
> +    else
> +    {
> +        /* RFRANZ_TODO - missing fdt_create_empty_tree() in libfdt. */
> +        return NULL;
> +    }


I don't think we currently support booting with no FDT at all, and even
in the ACPI world we would get one. So I think this would be an error.
IOW no need to worry about it.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH RFC 19/19] Add EFI stub for ARM64
  2014-06-28  1:25 ` [PATCH RFC 19/19] Add EFI stub for ARM64 Roy Franz
@ 2014-07-02 12:34   ` Ian Campbell
  2014-07-09 19:15     ` Roy Franz
  0 siblings, 1 reply; 44+ messages in thread
From: Ian Campbell @ 2014-07-02 12:34 UTC (permalink / raw)
  To: Roy Franz
  Cc: keir, tim, xen-devel, stefano.stabellini, jbeulich, linaro-uefi, fu.wei

On Sat, 2014-06-28 at 02:25 +0100, Roy Franz wrote:
> This patch adds the real EFI application entry point in head.S,
> and adds the efi.c file that will contain the EFI stub implementation.
> The assembly wrapper is responsible for returning the processor state
> to what XEN expects when starting.  The EFI stub processes the
> XEN EFI configuration file to load the dom0 kernel, ramdisk, etc and
> constructs a device tree for XEN to use, then transfers control to
> the normal XEN image entry point.
> 
> Signed-off-by: Roy Franz <roy.franz@linaro.org>
> ---
>  xen/arch/arm/Makefile     |   2 +
>  xen/arch/arm/arm64/head.S |  62 ++++-
>  xen/arch/arm/efi-shared.c |   1 +
>  xen/arch/arm/efi.c        | 686 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 750 insertions(+), 1 deletion(-)
>  create mode 120000 xen/arch/arm/efi-shared.c
>  create mode 100644 xen/arch/arm/efi.c
> 
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 63e0460..c3ed74f 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -33,6 +33,8 @@ obj-y += hvm.o
>  obj-y += device.o
>  obj-y += decode.o
>  obj-y += processor.o
> +obj-y += efi.o
> +obj-y += efi-shared.o

While the latter will eventually be in xen/common/efi/something.c and
hence -fshort-char is taken care of I suppose efi.o needs it too?

Probably the easiest thing to do there is to create
xen/arch/arm/efi/stub.c and do the fshort-char trick for that entire
folder, since I don't think our build system makes it especially easy to
change CFLAGS for individual object files.

> +
> +	/*
> +	 * efi_entry() will return here with device tree address in x0.
> +	 *  Save value in register which is preserved by __flush_dcache_all.
> +	 */
> +	mov	x20, x0
> +
> +	/* Turn off Dcache and MMU */

AIUI EFI leaves us in a 1:1 mapping, hence this is safe to do.

> +	mrs	x0, CurrentEL
> +	cmp	x0, #PSR_MODE_EL2t
> +	ccmp	x0, #PSR_MODE_EL2h, #0x4, ne
> +	b.ne	1f
> +	mrs	x0, sctlr_el2
> +	bic	x0, x0, #1 << 0	// clear SCTLR.M
> +	bic	x0, x0, #1 << 2	// clear SCTLR.C
> +	msr	sctlr_el2, x0
> +	isb
> +	b	2f
> +1:
> +	mrs	x0, sctlr_el1

I hope there is no way to get here under Xen ;-) I suppose you do it
this way so that we reach the regular head.S which then prints all the
error messages etc?

> +	bic	x0, x0, #1 << 0	// clear SCTLR.M
> +	bic	x0, x0, #1 << 2	// clear SCTLR.C
> +	msr	sctlr_el1, x0
> +	isb
> +2:
> +/*	bl	__flush_dcache_all */  /* RFRANZ_TODO */

We just don't have this function right now, correct? I suppose you are
running on models without cache modelling turned on?

Eventually this will implement a total by set/way, right? That's safe
because we are uniprocessor at this point etc etc.

> +/*
> + * This define and the code associated with it is a temporary, and will
> + * be replaced by changes to the XEN kernel to use the EFI memory map
> + * that is passed in the device tree instead of the normal FDT memory map.
> + * The XEN kernel changes are largely indpendent of the stub, so this code

FWIW given this is totally temporary: "independent".

> + * has been added to allow review and some testing of the stub code while
> + * the XEN kernel code is being developed.
> + */
> +#define ADD_EFI_MEMORY_TO_FDT
> +
> +#define DEVICE_TREE_GUID \
> +{0xb1b621d5, 0xf19c, 0x41a5, {0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0}}
> +
> +extern CHAR16 __initdata newline[];
> +extern SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdOut;
> +extern SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdERR;
> +extern EFI_BOOT_SERVICES *__initdata efi_bs;
> +
> +
> +static EFI_HANDLE __initdata efi_ih;
> +
> +static struct file __initdata cfg;
> +static struct file __initdata kernel;
> +static struct file __initdata ramdisk;
> +static struct file __initdata dtb;
> +
> +static unsigned long mmap_size;
> +static EFI_MEMORY_DESCRIPTOR *mmap_ptr;
> +
> +static void *new_fdt;
> +
> +#ifdef ADD_EFI_MEMORY_TO_FDT
> +static int IsLinuxReservedRegion(int MemoryType)

Linux? I guess this is a hangover from the Linux stub?

(Even there I'd have thought the regions were reserved by EFI not Linux
though, perhaps I'm looking at it backwards though)

> +{
> +    switch ( MemoryType )
> +    {
> +        case EfiMemoryMappedIO:
> +        case EfiMemoryMappedIOPortSpace:
> +        case EfiRuntimeServicesCode:
> +        case EfiRuntimeServicesData:
> +        case EfiUnusableMemory:
> +        case EfiACPIReclaimMemory:
> +        case EfiACPIMemoryNVS:
> +        case EfiLoaderData:
> +            return 1;
> +        default:
> +            return 0;
> +    }
> +}
> +
> +
> +static EFI_STATUS __init efi_process_memory_map(EFI_MEMORY_DESCRIPTOR *map,
> +                                                unsigned long mmap_size,
> +                                                unsigned long desc_size,
> +                                                void *fdt)
> +{
> +    int Index;
> +    int err;
> +
> +    EFI_MEMORY_DESCRIPTOR *desc_ptr = map;
> +
> +    /* Go through the list and add the reserved region to the Device Tree */
> +    for ( Index = 0; Index < (mmap_size / desc_size); Index++ )
> +    {
> +        if ( IsLinuxReservedRegion(desc_ptr->Type) )
> +        {
> +            err = fdt_add_mem_rsv(fdt, desc_ptr->PhysicalStart, desc_ptr->NumberOfPages * EFI_PAGE_SIZE);
> +            if ( err != 0 )
> +                PrintStr(L"Warning: Fail to add 'memreserve'\n\n");
> +        }
> +        desc_ptr = NextMemoryDescriptor(desc_ptr, desc_size);
> +    }
> +
> +    return EFI_SUCCESS;
> +
> +}
> +#endif
> +
> +static EFI_STATUS __init efi_get_memory_map(EFI_SYSTEM_TABLE *sys_table_arg,
> +                                            EFI_MEMORY_DESCRIPTOR **map,
> +                                            unsigned long *mmap_size,
> +                                            unsigned long *desc_size,
> +                                            UINT32 *desc_ver,
> +                                            unsigned long *key_ptr)
> +{
> +    EFI_MEMORY_DESCRIPTOR *m = NULL;
> +    EFI_STATUS status;
> +    unsigned long key;
> +    u32 desc_version;
> +
> +    *mmap_size = sizeof(*m) * 32;
> +again:
> +    /*
> +     * Add an additional 2 efi_memory_desc_t because we're doing an
> +         * allocation which may be in a new descriptor region, and this
> +         * may create additional entries.
> +     */
> +    *mmap_size += 2 * sizeof(*m);
> +    status = sys_table_arg->BootServices->AllocatePool(EfiLoaderData,
> +                                                       *mmap_size, (void **)&m);
> +    if ( status != EFI_SUCCESS )
> +        goto fail;
> +
> +    *desc_size = 0;
> +    key = 0;
> +    status = sys_table_arg->BootServices->GetMemoryMap(mmap_size, m,
> +                                                       &key, desc_size, &desc_version);
> +    if ( status == EFI_BUFFER_TOO_SMALL )
> +    {
> +        sys_table_arg->BootServices->FreePool(m);
> +        goto again;
> +    }
> +
> +    if ( status != EFI_SUCCESS )
> +        sys_table_arg->BootServices->FreePool(m);
> +
> +    if ( key_ptr && status == EFI_SUCCESS )
> +        *key_ptr = key;
> +    if ( desc_ver && status == EFI_SUCCESS )
> +        *desc_ver = desc_version;
> +
> +fail:
> +    *map = m;

I think if status != EFI_SUCCESS m is a free'd pointer? Probably best
not to tempt the caller with such a thing on failure.

> +    return status;
> +}
> +
> +
> +static void __init *lookup_fdt_config_table(EFI_SYSTEM_TABLE *sys_table)
> +{
> +    const EFI_GUID fdt_guid = DEVICE_TREE_GUID;
> +    EFI_CONFIGURATION_TABLE *tables;
> +    void *fdt;
> +    int i;
> +
> +    tables = sys_table->ConfigurationTable;
> +    fdt = NULL;
> +
> +    for ( i = 0; i < sys_table->NumberOfTableEntries; i++ ) if ( match_guid(&tables[i].VendorGuid, &fdt_guid) == 0 )

I think this long line is the result of an accidentally omitted \n?

> +        {
> +            fdt = tables[i].VendorTable;
> +            break;
> +        }
> +    return fdt;
> +}
> +
> +/*
> + * Get (or set if not present) the #addr-cells and #size cells
> + * properties of the chose node.  We need to know these to properly

                             ^chosen

> + * construct the address ranges used to describe the files loaded
> + * by the stub.
> + */
> +static int setup_chosen_node(void *fdt, int *addr_cells, int *size_cells)

These bits aren't __init?

> +
> +/*
> + * Set a single 'reg' property taking into account the
> + * configured addr and size cell sizes.
> + */
> +static int fdt_set_reg(void *fdt, int node, int addr_cells, int size_cells,
> +                       uint64_t addr, uint64_t len)

FWIW I think we have such a helper already somewhere, but perhaps this
file with -fshort-char etc is unable to call it?

> +{
> +    uint8_t data[16]; /* at most 2 64 bit words */
> +    void *p = data;
> +
> +    /* Make sure that the values provided can be represented in
> +     * the reg property.
> +     */
> +    if (addr_cells == 1 && (addr >> 32))

Xen coding style puts spaces more often than you may be used to. In
particular inside the brackets of an if (and while etc). So here:
        if ( addr_cells == 1 && ( addr >> 32 ) )


> +
> +/*
> + * Add the FDT nodes for the standard EFI information, which consist
> + * of the System table address, the address of the final EFI memory map,
> + * and memory map information.
> + */
> +static EFI_STATUS fdt_add_uefi_nodes(EFI_SYSTEM_TABLE *sys_table, void *fdt,
> +                               EFI_MEMORY_DESCRIPTOR *memory_map,
> +                               unsigned long map_size, unsigned long desc_size,
> +                               u32 desc_ver)
> +{
> +    int node;
> +    int status;
> +    u32 fdt_val32;
> +    u64 fdt_val64;
> +
> +#ifndef ADD_EFI_MEMORY_TO_FDT
> +    int prev;
> +    /*
> +     * Delete any memory nodes present.  The EFI memory map is the only
> +     * memory description provided to XEN.
> +     */
> +    prev = 0;
> +    for (;;)
> +    {
> +        const char *type;
> +        int len;
> +
> +        node = fdt_next_node(fdt, prev, NULL);
> +        if (node < 0)
> +        break;
> +
> +        type = fdt_getprop(fdt, node, "device_type", &len);
> +        if (type && strncmp(type, "memory", len) == 0)
> +        {
> +            fdt_del_node(fdt, node);
> +            continue;
> +        }
> +
> +        prev = node;
> +    }
> +#endif
> +
> +    node = fdt_subnode_offset(fdt, 0, "chosen");
> +    if ( node < 0 )
> +    {
> +        node = fdt_add_subnode(fdt, 0, "chosen");
> +        if ( node < 0 )
> +        {
> +            status = node; /* node is error code when negative */
> +            goto fdt_set_fail;
> +        }
> +    }
> +
> +    /* Add FDT entries for EFI runtime services in chosen node. */
> +    node = fdt_subnode_offset(fdt, 0, "chosen");

You looked this up above (or created it), and with proper error
handling. I think you don't need to look again?

> [...]
> +}

Please can you include the emacs magic block you'll find in other files
at the end (and any other new files too)

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH RFC 00/19] arm64 EFI stub
  2014-07-02 12:31   ` Ian Campbell
@ 2014-07-09 18:24     ` Roy Franz
  2014-07-10  7:58       ` Ian Campbell
  0 siblings, 1 reply; 44+ messages in thread
From: Roy Franz @ 2014-07-09 18:24 UTC (permalink / raw)
  To: Ian Campbell
  Cc: keir, tim, xen-devel, Stefano Stabellini, Jan Beulich,
	linaro-uefi, Fu Wei

On Wed, Jul 2, 2014 at 5:31 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Wed, 2014-07-02 at 12:52 +0100, Ian Campbell wrote:
>> On Sat, 2014-06-28 at 02:25 +0100, Roy Franz wrote:
>> > 2) Is it valid to start XEN with a device tree that only contains
>> > multi-boot and EFI info? (As would be the case if the stub doesn't get a
>> > device tree as input.)
>>
>> This would be the case when running on an ACPI system I guess? In which
>> case everything which would be in the DT would instead be in the ACPI
>> tables, which is something Naresh is working on I think.
>>
>> >   Currently this isn't supported, some libfdt
>> > functions are missing, so I'm checking if this is desired before I add
>> > that.
>>
>> Unless this==booting with ACPI I'm not sure what it is and if its ACPI
>> I'm not sure what needs to be done in the stub, could you explain
>> please?
>>
>> (I have a feeling I'm missing something obvious)
>
> Ah, I think this is from the final patch:
>
>> +    if ( fdtfile->ptr )
>> +        fdt_size = fdt_totalsize(fdtfile->ptr);
>> +    else
>> +    {
>> +        /* RFRANZ_TODO - missing fdt_create_empty_tree() in libfdt. */
>> +        return NULL;
>> +    }
>
>
> I don't think we currently support booting with no FDT at all, and even
> in the ACPI world we would get one. So I think this would be an error.
> IOW no need to worry about it.
>

I think that for the ACPI-only case, the EFI stub will be the one
responsible for creating the FDT for passing the UEFI/ACPI to the XEN
kernel.  (I had overlooked the ACPI-only
case when considering the no-FDT case - for Linux we support this
since a kernel could be configured such that it needs nothing from the
FDT other than UEFI stuff, so we need to support it.)
So I think we will need this, but only for the ACPI-only case.  I'll
look at adding the required libfdt file that provides
fdt_create_empty_tree() - when I took a quick look it seemed that was
in a file by itself.  Matching the version of this new file with the
existing libfdt may add some complication, but I'll look into it.

Thanks,
Roy

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH RFC 17/19] Remove x86 specific defintions from efibind.h
  2014-07-02 12:03   ` Ian Campbell
@ 2014-07-09 18:27     ` Roy Franz
  2014-07-10  7:59       ` Ian Campbell
  0 siblings, 1 reply; 44+ messages in thread
From: Roy Franz @ 2014-07-09 18:27 UTC (permalink / raw)
  To: Ian Campbell
  Cc: keir, tim, xen-devel, Stefano Stabellini, Jan Beulich,
	linaro-uefi, Fu Wei

On Wed, Jul 2, 2014 at 5:03 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Sat, 2014-06-28 at 02:25 +0100, Roy Franz wrote:
>> Clean up arm64 efibind.h to not include x86 specific definitions.
>
> For the majority of this series I really appreciate the refactor in one
> patch and then move in another approach, but I think for this specific
> case you could fold these together into a single "introduce the arm64
> efi bindings" patch.
>
> Ian.
>

I'll collapse these, and keep the arm64 efibind.h.  I try very hard to
keep code movement isolated,
as it's easy to miss small changes when mixed, but it does lead to
longer series.

Roy

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH RFC 02/19] Create efi-shared.[ch], and move string functions
  2014-07-02 11:55     ` Ian Campbell
@ 2014-07-09 18:31       ` Roy Franz
  0 siblings, 0 replies; 44+ messages in thread
From: Roy Franz @ 2014-07-09 18:31 UTC (permalink / raw)
  To: Ian Campbell
  Cc: keir, tim, xen-devel, Stefano Stabellini, Jan Beulich,
	linaro-uefi, Fu Wei

On Wed, Jul 2, 2014 at 4:55 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Mon, 2014-06-30 at 12:15 +0100, Jan Beulich wrote:
>> >>> On 28.06.14 at 03:25, <roy.franz@linaro.org> wrote:
>> > Create the files for EFI code that will be shared with the ARM stub,
>> > and move string functions and some global variables.
>> >
>> > TODO: proper sharing of file (with proper -fshort-char compile option)
>>
>> Already at this point of the series I wonder what the value of
>> posting it is if even the basic question of placement within the tree
>> hasn't been sorted out yet. What are reviewers expected to review
>> (considering that code should just be getting moved, not modified)?
>
> FWIW I found that reading the remainder of the series with an implicit
> s,xen/arch/x86/efi/efi-shared.c,xen/common/efi/blah.c in my head was
> worthwhile -- there's some useful refactoring in here as well as the
> code motion into efi-shared^Wblah.c.
>
> Ian.
>
>


I'll move the shared C code to xen/common/efi/ in the next series.
For building with
the -fshort-wchar I'll likely need a subdirectory even if it has only
a single file for now.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH RFC 15/19] Add PE/COFF header in head.S
  2014-07-02 12:02   ` Ian Campbell
@ 2014-07-09 18:35     ` Roy Franz
  0 siblings, 0 replies; 44+ messages in thread
From: Roy Franz @ 2014-07-09 18:35 UTC (permalink / raw)
  To: Ian Campbell
  Cc: keir, tim, xen-devel, Stefano Stabellini, Jan Beulich,
	linaro-uefi, Fu Wei

On Wed, Jul 2, 2014 at 5:02 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Sat, 2014-06-28 at 02:25 +0100, Roy Franz wrote:
>> This patch adds a simple PE/COFF header in head.S.  PE/COFF linker support
>> is not available for arm64, so a native build is not possible.  Also, this
>> allows the binary to be both a PE/COFF EFI application, and a normal Image
>> file bootable like a Linux kernel.  The arm and arm64 Linux kernels use the
>> same methodology to create a single image that is both an EFI application and
>> a zImage/Image file.
>
> This looks good to me. A few minor comments:
>
> Some of the whitespace is off (please use space and not hard tabs
> throughout)
I'll clean that up.

>
> I think we could perhaps get rid of CONFIG_EFI_STUB at least at the
> arm64 subarch level. We don't in general support the same level of
> configurability in Xen as Linux -- it's mostly just a single
> configuration for everyone. It'd need to be retain at the arch level to
> cope with arm32 not doing efi, but removing it here would make things
> simpler I think?
I agree. I don't think the arm64 stub needs to be configurable, as it
should not
affect non-stub users when it is enabled, and should be a fairly small amount
of code.  It will need to be disabled for arm32.

>
> Would it be possible to find sort of link/reference to a spec for the
> header structs which you are adding and stick them in comments before
> each one please.
I will try to track down some documentation to link to.  It may end up
being a single
link to a PDF (or even a microsoft Word document, since it is a Microsoft spec.)

>
>>
>> Signed-off-by: Roy Franz <roy.franz@linaro.org>
>> ---
>>  xen/arch/arm/arm64/head.S | 118 +++++++++++++++++++++++++++++++++++++++++++++-
>>  xen/arch/arm/xen.lds.S    |   1 +
>>  2 files changed, 117 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index 2a13527..9b88aeb 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -84,7 +84,7 @@
>>  #endif /* !CONFIG_EARLY_PRINTK */
>>
>>          /*.aarch64*/
>> -
>> +#define CONFIG_EFI_STUB
>>          /*
>>           * Kernel startup entry point.
>>           * ---------------------------
>> @@ -100,12 +100,24 @@
>>           */
>>
>>          .global start
>> +efi_stub_entry:  /* Dummy symbol so we can compile before actual stub added */
>>  start:
>> +#ifdef CONFIG_EFI_STUB
>>          /*
>>           * DO NOT MODIFY. Image header expected by Linux boot-loaders.
>>           */
>> -        b       real_start           /* branch to kernel start, magic */
>> +efi_head:
>> +        /*
>> +         * This add instruction has no meaningful effect except that
>> +         * its opcode forms the magic "MZ" signature of a PE/COFF file
>> +      * that is required for UEFI applications.
>> +         */
>> +        add     x13, x18, #0x16
>
> I love this hack ;-)
>
> Ian.
>

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH RFC 19/19] Add EFI stub for ARM64
  2014-07-02 12:34   ` Ian Campbell
@ 2014-07-09 19:15     ` Roy Franz
  2014-07-10  8:13       ` Ian Campbell
  0 siblings, 1 reply; 44+ messages in thread
From: Roy Franz @ 2014-07-09 19:15 UTC (permalink / raw)
  To: Ian Campbell
  Cc: keir, tim, xen-devel, Stefano Stabellini, Jan Beulich,
	linaro-uefi, Fu Wei

On Wed, Jul 2, 2014 at 5:34 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Sat, 2014-06-28 at 02:25 +0100, Roy Franz wrote:
>> This patch adds the real EFI application entry point in head.S,
>> and adds the efi.c file that will contain the EFI stub implementation.
>> The assembly wrapper is responsible for returning the processor state
>> to what XEN expects when starting.  The EFI stub processes the
>> XEN EFI configuration file to load the dom0 kernel, ramdisk, etc and
>> constructs a device tree for XEN to use, then transfers control to
>> the normal XEN image entry point.
>>
>> Signed-off-by: Roy Franz <roy.franz@linaro.org>
>> ---
>>  xen/arch/arm/Makefile     |   2 +
>>  xen/arch/arm/arm64/head.S |  62 ++++-
>>  xen/arch/arm/efi-shared.c |   1 +
>>  xen/arch/arm/efi.c        | 686 ++++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 750 insertions(+), 1 deletion(-)
>>  create mode 120000 xen/arch/arm/efi-shared.c
>>  create mode 100644 xen/arch/arm/efi.c
>>
>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>> index 63e0460..c3ed74f 100644
>> --- a/xen/arch/arm/Makefile
>> +++ b/xen/arch/arm/Makefile
>> @@ -33,6 +33,8 @@ obj-y += hvm.o
>>  obj-y += device.o
>>  obj-y += decode.o
>>  obj-y += processor.o
>> +obj-y += efi.o
>> +obj-y += efi-shared.o
>
> While the latter will eventually be in xen/common/efi/something.c and
> hence -fshort-char is taken care of I suppose efi.o needs it too?
>
> Probably the easiest thing to do there is to create
> xen/arch/arm/efi/stub.c and do the fshort-char trick for that entire
> folder, since I don't think our build system makes it especially easy to
> change CFLAGS for individual object files.

Yes and yes.  I will likely end up with some single file directories due to the
build option differences.  Is there a XEN build system expert that I can ask
for help if needed?  (Or should I just post to the list?)

>
>> +
>> +     /*
>> +      * efi_entry() will return here with device tree address in x0.
>> +      *  Save value in register which is preserved by __flush_dcache_all.
>> +      */
>> +     mov     x20, x0
>> +
>> +     /* Turn off Dcache and MMU */
>
> AIUI EFI leaves us in a 1:1 mapping, hence this is safe to do.

Yes, EFI specifies a 1:1 mapping.  The assembly wrapper here is the same as we
use for Linux, so it still needs some tweaks.  Linux expects MMU/DCACHE off.

>
>> +     mrs     x0, CurrentEL
>> +     cmp     x0, #PSR_MODE_EL2t
>> +     ccmp    x0, #PSR_MODE_EL2h, #0x4, ne
>> +     b.ne    1f
>> +     mrs     x0, sctlr_el2
>> +     bic     x0, x0, #1 << 0 // clear SCTLR.M
>> +     bic     x0, x0, #1 << 2 // clear SCTLR.C
>> +     msr     sctlr_el2, x0
>> +     isb
>> +     b       2f
>> +1:
>> +     mrs     x0, sctlr_el1
>
> I hope there is no way to get here under Xen ;-) I suppose you do it
> this way so that we reach the regular head.S which then prints all the
> error messages etc?

I'll take a closer look at this.

>
>> +     bic     x0, x0, #1 << 0 // clear SCTLR.M
>> +     bic     x0, x0, #1 << 2 // clear SCTLR.C
>> +     msr     sctlr_el1, x0
>> +     isb
>> +2:
>> +/*   bl      __flush_dcache_all */  /* RFRANZ_TODO */
>
> We just don't have this function right now, correct? I suppose you are
> running on models without cache modelling turned on?
>
> Eventually this will implement a total by set/way, right? That's safe
> because we are uniprocessor at this point etc etc.

This works on the model since I'm not modeling the cache, but we do need it
if we are going to turn the dcache off.  If we can leave the dcache enabled
for XEN then I don't think we need to flush it here.

>
>> +/*
>> + * This define and the code associated with it is a temporary, and will
>> + * be replaced by changes to the XEN kernel to use the EFI memory map
>> + * that is passed in the device tree instead of the normal FDT memory map.
>> + * The XEN kernel changes are largely indpendent of the stub, so this code
>
> FWIW given this is totally temporary: "independent".

They will become very dependent once XEN uses the EFI memory map instead of
the FDT memory map.  The code protected with ADD_EFI_MEMORY_TO_FDT is all
a temporary crutch to allow testing/development of the stub before the changes
to the XEN kernel to use the EFI memory map directly are made.

>
>> + * has been added to allow review and some testing of the stub code while
>> + * the XEN kernel code is being developed.
>> + */
>> +#define ADD_EFI_MEMORY_TO_FDT
>> +
>> +#define DEVICE_TREE_GUID \
>> +{0xb1b621d5, 0xf19c, 0x41a5, {0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0}}
>> +
>> +extern CHAR16 __initdata newline[];
>> +extern SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdOut;
>> +extern SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdERR;
>> +extern EFI_BOOT_SERVICES *__initdata efi_bs;
>> +
>> +
>> +static EFI_HANDLE __initdata efi_ih;
>> +
>> +static struct file __initdata cfg;
>> +static struct file __initdata kernel;
>> +static struct file __initdata ramdisk;
>> +static struct file __initdata dtb;
>> +
>> +static unsigned long mmap_size;
>> +static EFI_MEMORY_DESCRIPTOR *mmap_ptr;
>> +
>> +static void *new_fdt;
>> +
>> +#ifdef ADD_EFI_MEMORY_TO_FDT
>> +static int IsLinuxReservedRegion(int MemoryType)
>
> Linux? I guess this is a hangover from the Linux stub?
>
> (Even there I'd have thought the regions were reserved by EFI not Linux
> though, perhaps I'm looking at it backwards though)

Since this is part of a temporary hack, I didn't rename anything :)
All this will go away.

>
>> +{
>> +    switch ( MemoryType )
>> +    {
>> +        case EfiMemoryMappedIO:
>> +        case EfiMemoryMappedIOPortSpace:
>> +        case EfiRuntimeServicesCode:
>> +        case EfiRuntimeServicesData:
>> +        case EfiUnusableMemory:
>> +        case EfiACPIReclaimMemory:
>> +        case EfiACPIMemoryNVS:
>> +        case EfiLoaderData:
>> +            return 1;
>> +        default:
>> +            return 0;
>> +    }
>> +}
>> +
>> +
>> +static EFI_STATUS __init efi_process_memory_map(EFI_MEMORY_DESCRIPTOR *map,
>> +                                                unsigned long mmap_size,
>> +                                                unsigned long desc_size,
>> +                                                void *fdt)
>> +{
>> +    int Index;
>> +    int err;
>> +
>> +    EFI_MEMORY_DESCRIPTOR *desc_ptr = map;
>> +
>> +    /* Go through the list and add the reserved region to the Device Tree */
>> +    for ( Index = 0; Index < (mmap_size / desc_size); Index++ )
>> +    {
>> +        if ( IsLinuxReservedRegion(desc_ptr->Type) )
>> +        {
>> +            err = fdt_add_mem_rsv(fdt, desc_ptr->PhysicalStart, desc_ptr->NumberOfPages * EFI_PAGE_SIZE);
>> +            if ( err != 0 )
>> +                PrintStr(L"Warning: Fail to add 'memreserve'\n\n");
>> +        }
>> +        desc_ptr = NextMemoryDescriptor(desc_ptr, desc_size);
>> +    }
>> +
>> +    return EFI_SUCCESS;
>> +
>> +}
>> +#endif
>> +
>> +static EFI_STATUS __init efi_get_memory_map(EFI_SYSTEM_TABLE *sys_table_arg,
>> +                                            EFI_MEMORY_DESCRIPTOR **map,
>> +                                            unsigned long *mmap_size,
>> +                                            unsigned long *desc_size,
>> +                                            UINT32 *desc_ver,
>> +                                            unsigned long *key_ptr)
>> +{
>> +    EFI_MEMORY_DESCRIPTOR *m = NULL;
>> +    EFI_STATUS status;
>> +    unsigned long key;
>> +    u32 desc_version;
>> +
>> +    *mmap_size = sizeof(*m) * 32;
>> +again:
>> +    /*
>> +     * Add an additional 2 efi_memory_desc_t because we're doing an
>> +         * allocation which may be in a new descriptor region, and this
>> +         * may create additional entries.
>> +     */
>> +    *mmap_size += 2 * sizeof(*m);
>> +    status = sys_table_arg->BootServices->AllocatePool(EfiLoaderData,
>> +                                                       *mmap_size, (void **)&m);
>> +    if ( status != EFI_SUCCESS )
>> +        goto fail;
>> +
>> +    *desc_size = 0;
>> +    key = 0;
>> +    status = sys_table_arg->BootServices->GetMemoryMap(mmap_size, m,
>> +                                                       &key, desc_size, &desc_version);
>> +    if ( status == EFI_BUFFER_TOO_SMALL )
>> +    {
>> +        sys_table_arg->BootServices->FreePool(m);
>> +        goto again;
>> +    }
>> +
>> +    if ( status != EFI_SUCCESS )
>> +        sys_table_arg->BootServices->FreePool(m);
>> +
>> +    if ( key_ptr && status == EFI_SUCCESS )
>> +        *key_ptr = key;
>> +    if ( desc_ver && status == EFI_SUCCESS )
>> +        *desc_ver = desc_version;
>> +
>> +fail:
>> +    *map = m;
>
> I think if status != EFI_SUCCESS m is a free'd pointer? Probably best
> not to tempt the caller with such a thing on failure.

Yeah, a freed pointer, or an undefined one if AllocatePool fails.  I'll update
this to set m to NULL on failure.

>
>> +    return status;
>> +}
>> +
>> +
>> +static void __init *lookup_fdt_config_table(EFI_SYSTEM_TABLE *sys_table)
>> +{
>> +    const EFI_GUID fdt_guid = DEVICE_TREE_GUID;
>> +    EFI_CONFIGURATION_TABLE *tables;
>> +    void *fdt;
>> +    int i;
>> +
>> +    tables = sys_table->ConfigurationTable;
>> +    fdt = NULL;
>> +
>> +    for ( i = 0; i < sys_table->NumberOfTableEntries; i++ ) if ( match_guid(&tables[i].VendorGuid, &fdt_guid) == 0 )
>
> I think this long line is the result of an accidentally omitted \n?
>
>> +        {
>> +            fdt = tables[i].VendorTable;
>> +            break;
>> +        }
>> +    return fdt;
>> +}
>> +
>> +/*
>> + * Get (or set if not present) the #addr-cells and #size cells
>> + * properties of the chose node.  We need to know these to properly
>
>                              ^chosen
>
>> + * construct the address ranges used to describe the files loaded
>> + * by the stub.
>> + */
>> +static int setup_chosen_node(void *fdt, int *addr_cells, int *size_cells)
>
> These bits aren't __init?

They should be...
>
>> +
>> +/*
>> + * Set a single 'reg' property taking into account the
>> + * configured addr and size cell sizes.
>> + */
>> +static int fdt_set_reg(void *fdt, int node, int addr_cells, int size_cells,
>> +                       uint64_t addr, uint64_t len)
>
> FWIW I think we have such a helper already somewhere, but perhaps this
> file with -fshort-char etc is unable to call it?

I'll try to find it.  I think it should be callable, since wide
characters aren't used
within this function of in its interface.

>
>> +{
>> +    uint8_t data[16]; /* at most 2 64 bit words */
>> +    void *p = data;
>> +
>> +    /* Make sure that the values provided can be represented in
>> +     * the reg property.
>> +     */
>> +    if (addr_cells == 1 && (addr >> 32))
>
> Xen coding style puts spaces more often than you may be used to. In
> particular inside the brackets of an if (and while etc). So here:
>         if ( addr_cells == 1 && ( addr >> 32 ) )
>
I tried to be good about that :)  I was hoping for a checkpatch script
to catch all
that kind of stuff for me :)
I'll review this and the use of tabs again.

>
>> +
>> +/*
>> + * Add the FDT nodes for the standard EFI information, which consist
>> + * of the System table address, the address of the final EFI memory map,
>> + * and memory map information.
>> + */
>> +static EFI_STATUS fdt_add_uefi_nodes(EFI_SYSTEM_TABLE *sys_table, void *fdt,
>> +                               EFI_MEMORY_DESCRIPTOR *memory_map,
>> +                               unsigned long map_size, unsigned long desc_size,
>> +                               u32 desc_ver)
>> +{
>> +    int node;
>> +    int status;
>> +    u32 fdt_val32;
>> +    u64 fdt_val64;
>> +
>> +#ifndef ADD_EFI_MEMORY_TO_FDT
>> +    int prev;
>> +    /*
>> +     * Delete any memory nodes present.  The EFI memory map is the only
>> +     * memory description provided to XEN.
>> +     */
>> +    prev = 0;
>> +    for (;;)
>> +    {
>> +        const char *type;
>> +        int len;
>> +
>> +        node = fdt_next_node(fdt, prev, NULL);
>> +        if (node < 0)
>> +        break;
>> +
>> +        type = fdt_getprop(fdt, node, "device_type", &len);
>> +        if (type && strncmp(type, "memory", len) == 0)
>> +        {
>> +            fdt_del_node(fdt, node);
>> +            continue;
>> +        }
>> +
>> +        prev = node;
>> +    }
>> +#endif
>> +
>> +    node = fdt_subnode_offset(fdt, 0, "chosen");
>> +    if ( node < 0 )
>> +    {
>> +        node = fdt_add_subnode(fdt, 0, "chosen");
>> +        if ( node < 0 )
>> +        {
>> +            status = node; /* node is error code when negative */
>> +            goto fdt_set_fail;
>> +        }
>> +    }
>> +
>> +    /* Add FDT entries for EFI runtime services in chosen node. */
>> +    node = fdt_subnode_offset(fdt, 0, "chosen");
>
> You looked this up above (or created it), and with proper error
> handling. I think you don't need to look again?

That does appear to be redundant.
>
>> [...]
>> +}
>
> Please can you include the emacs magic block you'll find in other files
> at the end (and any other new files too)
>
I will add this..

Thanks Ian, Stefano and Jan for all your feedback.

Roy

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH RFC 00/19] arm64 EFI stub
  2014-07-09 18:24     ` Roy Franz
@ 2014-07-10  7:58       ` Ian Campbell
  2014-07-10 17:33         ` Roy Franz
  0 siblings, 1 reply; 44+ messages in thread
From: Ian Campbell @ 2014-07-10  7:58 UTC (permalink / raw)
  To: Roy Franz
  Cc: keir, tim, xen-devel, Stefano Stabellini, Jan Beulich,
	linaro-uefi, Fu Wei

On Wed, 2014-07-09 at 11:24 -0700, Roy Franz wrote:
> >> +    if ( fdtfile->ptr )
> >> +        fdt_size = fdt_totalsize(fdtfile->ptr);
> >> +    else
> >> +    {
> >> +        /* RFRANZ_TODO - missing fdt_create_empty_tree() in libfdt. */
> >> +        return NULL;
> >> +    }
> >
> >
> > I don't think we currently support booting with no FDT at all, and even
> > in the ACPI world we would get one. So I think this would be an error.
> > IOW no need to worry about it.
> >
> 
> I think that for the ACPI-only case, the EFI stub will be the one
> responsible for creating the FDT for passing the UEFI/ACPI to the XEN
> kernel.  (I had overlooked the ACPI-only
> case when considering the no-FDT case - for Linux we support this
> since a kernel could be configured such that it needs nothing from the
> FDT other than UEFI stuff, so we need to support it.)
> So I think we will need this, but only for the ACPI-only case.

I think you probably know better than I but I thought that the way the
presence of ACPI was communicated to the kernel (whether via zImage or
UEFI stub) was via entries in the FDT's /chosen/ node containing the
address of the RSDP. Am I wrong about that?

Or maybe I'm thinking about the UEFI services tables being referenced
via /chosen/?

Ian.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH RFC 17/19] Remove x86 specific defintions from efibind.h
  2014-07-09 18:27     ` Roy Franz
@ 2014-07-10  7:59       ` Ian Campbell
  0 siblings, 0 replies; 44+ messages in thread
From: Ian Campbell @ 2014-07-10  7:59 UTC (permalink / raw)
  To: Roy Franz
  Cc: keir, tim, xen-devel, Stefano Stabellini, Jan Beulich,
	linaro-uefi, Fu Wei

On Wed, 2014-07-09 at 11:27 -0700, Roy Franz wrote:
> On Wed, Jul 2, 2014 at 5:03 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Sat, 2014-06-28 at 02:25 +0100, Roy Franz wrote:
> >> Clean up arm64 efibind.h to not include x86 specific definitions.
> >
> > For the majority of this series I really appreciate the refactor in one
> > patch and then move in another approach, but I think for this specific
> > case you could fold these together into a single "introduce the arm64
> > efi bindings" patch.
> >
> > Ian.
> >
> 
> I'll collapse these, and keep the arm64 efibind.h.

Thanks.

>   I try very hard to
> keep code movement isolated,
> as it's easy to miss small changes when mixed, but it does lead to
> longer series.

It's appreciated, much easier to review IMHO since you can trivially ack
half the patches if they are pure motion.

Ian.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH RFC 19/19] Add EFI stub for ARM64
  2014-07-09 19:15     ` Roy Franz
@ 2014-07-10  8:13       ` Ian Campbell
  2014-07-10  9:21         ` [Linaro-uefi] " Julien Grall
  0 siblings, 1 reply; 44+ messages in thread
From: Ian Campbell @ 2014-07-10  8:13 UTC (permalink / raw)
  To: Roy Franz
  Cc: keir, tim, xen-devel, Stefano Stabellini, Jan Beulich,
	linaro-uefi, Fu Wei

On Wed, 2014-07-09 at 12:15 -0700, Roy Franz wrote:
> On Wed, Jul 2, 2014 at 5:34 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Sat, 2014-06-28 at 02:25 +0100, Roy Franz wrote:
> >> This patch adds the real EFI application entry point in head.S,
> >> and adds the efi.c file that will contain the EFI stub implementation.
> >> The assembly wrapper is responsible for returning the processor state
> >> to what XEN expects when starting.  The EFI stub processes the
> >> XEN EFI configuration file to load the dom0 kernel, ramdisk, etc and
> >> constructs a device tree for XEN to use, then transfers control to
> >> the normal XEN image entry point.
> >>
> >> Signed-off-by: Roy Franz <roy.franz@linaro.org>
> >> ---
> >>  xen/arch/arm/Makefile     |   2 +
> >>  xen/arch/arm/arm64/head.S |  62 ++++-
> >>  xen/arch/arm/efi-shared.c |   1 +
> >>  xen/arch/arm/efi.c        | 686 ++++++++++++++++++++++++++++++++++++++++++++++
> >>  4 files changed, 750 insertions(+), 1 deletion(-)
> >>  create mode 120000 xen/arch/arm/efi-shared.c
> >>  create mode 100644 xen/arch/arm/efi.c
> >>
> >> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> >> index 63e0460..c3ed74f 100644
> >> --- a/xen/arch/arm/Makefile
> >> +++ b/xen/arch/arm/Makefile
> >> @@ -33,6 +33,8 @@ obj-y += hvm.o
> >>  obj-y += device.o
> >>  obj-y += decode.o
> >>  obj-y += processor.o
> >> +obj-y += efi.o
> >> +obj-y += efi-shared.o
> >
> > While the latter will eventually be in xen/common/efi/something.c and
> > hence -fshort-char is taken care of I suppose efi.o needs it too?
> >
> > Probably the easiest thing to do there is to create
> > xen/arch/arm/efi/stub.c and do the fshort-char trick for that entire
> > folder, since I don't think our build system makes it especially easy to
> > change CFLAGS for individual object files.
> 
> Yes and yes.  I will likely end up with some single file directories due to the
> build option differences.  Is there a XEN build system expert that I can ask
> for help if needed?  (Or should I just post to the list?)

I'm not sure anyone is an expert on this stuff but feel free to prod me
either here or on #xenarm.

I think you just need something like, in xen/arch/arm/Makefile
   subdir-$(arm64) += efi # or CONFIG_EFI etc
and in xen/arch/arm/efi/Makefile:
   CFLAGS += -fshort-wchar
   obj-y += foo.o etc.o

much the same for xen/common/efi.

> >
> >> +
> >> +     /*
> >> +      * efi_entry() will return here with device tree address in x0.
> >> +      *  Save value in register which is preserved by __flush_dcache_all.
> >> +      */
> >> +     mov     x20, x0
> >> +
> >> +     /* Turn off Dcache and MMU */
> >
> > AIUI EFI leaves us in a 1:1 mapping, hence this is safe to do.
> 
> Yes, EFI specifies a 1:1 mapping.

Could you append to the comment ", we are running on EFI's 1:1 mapping"
or something please.

>   The assembly wrapper here is the same as we
> use for Linux, so it still needs some tweaks.  Linux expects MMU/DCACHE off.

Xen's main entry point is the same. I was wondering recently about
allowing the stub to enter it with them on, I think all that would be
needed is some cache maintenance as we build the boot page tables
(which would be harmless in the zImage case).

I think that's very much a thing for a future enhancement though...

> >
> >> +     bic     x0, x0, #1 << 0 // clear SCTLR.M
> >> +     bic     x0, x0, #1 << 2 // clear SCTLR.C
> >> +     msr     sctlr_el1, x0
> >> +     isb
> >> +2:
> >> +/*   bl      __flush_dcache_all */  /* RFRANZ_TODO */
> >
> > We just don't have this function right now, correct? I suppose you are
> > running on models without cache modelling turned on?
> >
> > Eventually this will implement a total by set/way, right? That's safe
> > because we are uniprocessor at this point etc etc.
> 
> This works on the model since I'm not modeling the cache, but we do need it
> if we are going to turn the dcache off.  If we can leave the dcache enabled
> for XEN then I don't think we need to flush it here.

It's up to you I think, if you want to just fill in __flush_dcache_all
(which I expect can be cribbed from Linux) that's fine, but if you
really want to you could instead put the necessary flushes into head.S
to make it safe to run with or without mmu+caches.

> >> + * has been added to allow review and some testing of the stub code while
> >> + * the XEN kernel code is being developed.
> >> + */
> >> +#define ADD_EFI_MEMORY_TO_FDT
> >> +
> >> +#define DEVICE_TREE_GUID \
> >> +{0xb1b621d5, 0xf19c, 0x41a5, {0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0}}
> >> +
> >> +extern CHAR16 __initdata newline[];
> >> +extern SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdOut;
> >> +extern SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdERR;
> >> +extern EFI_BOOT_SERVICES *__initdata efi_bs;
> >> +
> >> +
> >> +static EFI_HANDLE __initdata efi_ih;
> >> +
> >> +static struct file __initdata cfg;
> >> +static struct file __initdata kernel;
> >> +static struct file __initdata ramdisk;
> >> +static struct file __initdata dtb;
> >> +
> >> +static unsigned long mmap_size;
> >> +static EFI_MEMORY_DESCRIPTOR *mmap_ptr;
> >> +
> >> +static void *new_fdt;
> >> +
> >> +#ifdef ADD_EFI_MEMORY_TO_FDT
> >> +static int IsLinuxReservedRegion(int MemoryType)
> >
> > Linux? I guess this is a hangover from the Linux stub?
> >
> > (Even there I'd have thought the regions were reserved by EFI not Linux
> > though, perhaps I'm looking at it backwards though)
> 
> Since this is part of a temporary hack, I didn't rename anything :)

Ah yes, the #ifdef right above should have clued me in to that!

> >> +/*
> >> + * Set a single 'reg' property taking into account the
> >> + * configured addr and size cell sizes.
> >> + */
> >> +static int fdt_set_reg(void *fdt, int node, int addr_cells, int size_cells,
> >> +                       uint64_t addr, uint64_t len)
> >
> > FWIW I think we have such a helper already somewhere, but perhaps this
> > file with -fshort-char etc is unable to call it?
> 
> I'll try to find it.  I think it should be callable, since wide
> characters aren't used
> within this function of in its interface.

I just had a look and I think I imagined it...

There is one in the userspace domain builder, but that is no use to you
here.

I expect that xen/arch/arm/domain_build.c could make use of this helper
too, but don't worry about that in this series.

> >> +{
> >> +    uint8_t data[16]; /* at most 2 64 bit words */
> >> +    void *p = data;
> >> +
> >> +    /* Make sure that the values provided can be represented in
> >> +     * the reg property.
> >> +     */
> >> +    if (addr_cells == 1 && (addr >> 32))
> >
> > Xen coding style puts spaces more often than you may be used to. In
> > particular inside the brackets of an if (and while etc). So here:
> >         if ( addr_cells == 1 && ( addr >> 32 ) )
> >
> I tried to be good about that :)  I was hoping for a checkpatch script
> to catch all that kind of stuff for me :)

I'm afraid I don't think we have one :-(

> I'll review this and the use of tabs again.

Thanks. 

>  Thanks Ian, Stefano and Jan for all your feedback.

Thanks for you work on this!

Ian.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [Linaro-uefi] [PATCH RFC 19/19] Add EFI stub for ARM64
  2014-07-10  8:13       ` Ian Campbell
@ 2014-07-10  9:21         ` Julien Grall
  2014-07-10  9:33           ` Ian Campbell
  0 siblings, 1 reply; 44+ messages in thread
From: Julien Grall @ 2014-07-10  9:21 UTC (permalink / raw)
  To: Ian Campbell, Roy Franz
  Cc: keir, tim, xen-devel, Stefano Stabellini, Jan Beulich, linaro-uefi

Hi Ian & Roy,

On 10/07/14 09:13, Ian Campbell wrote:
>>> Probably the easiest thing to do there is to create
>>> xen/arch/arm/efi/stub.c and do the fshort-char trick for that entire
>>> folder, since I don't think our build system makes it especially easy to
>>> change CFLAGS for individual object files.
>>
>> Yes and yes.  I will likely end up with some single file directories due to the
>> build option differences.  Is there a XEN build system expert that I can ask
>> for help if needed?  (Or should I just post to the list?)
>
> I'm not sure anyone is an expert on this stuff but feel free to prod me
> either here or on #xenarm.
>
> I think you just need something like, in xen/arch/arm/Makefile
>     subdir-$(arm64) += efi # or CONFIG_EFI etc
> and in xen/arch/arm/efi/Makefile:
>     CFLAGS += -fshort-wchar
>     obj-y += foo.o etc.o
>
> much the same for xen/common/efi.

The Xen buildsystem doesn't provide CFLAGS facilities as the Kernel, but 
the Makefile has a syntax to append data in variables for a specific target.

If your efi.c is in xen/arch/arm/, you need to add in xen/arch/arm/Makefile:

efi.o: CFLAGS += -fshort-wchar

Regards,

-- 
Julien Grall

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [Linaro-uefi] [PATCH RFC 19/19] Add EFI stub for ARM64
  2014-07-10  9:21         ` [Linaro-uefi] " Julien Grall
@ 2014-07-10  9:33           ` Ian Campbell
  0 siblings, 0 replies; 44+ messages in thread
From: Ian Campbell @ 2014-07-10  9:33 UTC (permalink / raw)
  To: Julien Grall
  Cc: keir, tim, xen-devel, Roy Franz, Stefano Stabellini, Jan Beulich,
	linaro-uefi

On Thu, 2014-07-10 at 10:21 +0100, Julien Grall wrote:
> Hi Ian & Roy,
> 
> On 10/07/14 09:13, Ian Campbell wrote:
> >>> Probably the easiest thing to do there is to create
> >>> xen/arch/arm/efi/stub.c and do the fshort-char trick for that entire
> >>> folder, since I don't think our build system makes it especially easy to
> >>> change CFLAGS for individual object files.
> >>
> >> Yes and yes.  I will likely end up with some single file directories due to the
> >> build option differences.  Is there a XEN build system expert that I can ask
> >> for help if needed?  (Or should I just post to the list?)
> >
> > I'm not sure anyone is an expert on this stuff but feel free to prod me
> > either here or on #xenarm.
> >
> > I think you just need something like, in xen/arch/arm/Makefile
> >     subdir-$(arm64) += efi # or CONFIG_EFI etc
> > and in xen/arch/arm/efi/Makefile:
> >     CFLAGS += -fshort-wchar
> >     obj-y += foo.o etc.o
> >
> > much the same for xen/common/efi.
> 
> The Xen buildsystem doesn't provide CFLAGS facilities as the Kernel, but 
> the Makefile has a syntax to append data in variables for a specific target.
> 
> If your efi.c is in xen/arch/arm/, you need to add in xen/arch/arm/Makefile:
> 
> efi.o: CFLAGS += -fshort-wchar

That would work but I think I'd rather keep the efi stuff in its own
subdirectory, even if right now it is only a single C file.

Ian.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH RFC 00/19] arm64 EFI stub
  2014-07-10  7:58       ` Ian Campbell
@ 2014-07-10 17:33         ` Roy Franz
  2014-07-11  8:57           ` Ian Campbell
  0 siblings, 1 reply; 44+ messages in thread
From: Roy Franz @ 2014-07-10 17:33 UTC (permalink / raw)
  To: Ian Campbell
  Cc: keir, tim, xen-devel, Stefano Stabellini, Jan Beulich,
	linaro-uefi, Fu Wei

On Thu, Jul 10, 2014 at 12:58 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Wed, 2014-07-09 at 11:24 -0700, Roy Franz wrote:
>> >> +    if ( fdtfile->ptr )
>> >> +        fdt_size = fdt_totalsize(fdtfile->ptr);
>> >> +    else
>> >> +    {
>> >> +        /* RFRANZ_TODO - missing fdt_create_empty_tree() in libfdt. */
>> >> +        return NULL;
>> >> +    }
>> >
>> >
>> > I don't think we currently support booting with no FDT at all, and even
>> > in the ACPI world we would get one. So I think this would be an error.
>> > IOW no need to worry about it.
>> >
>>
>> I think that for the ACPI-only case, the EFI stub will be the one
>> responsible for creating the FDT for passing the UEFI/ACPI to the XEN
>> kernel.  (I had overlooked the ACPI-only
>> case when considering the no-FDT case - for Linux we support this
>> since a kernel could be configured such that it needs nothing from the
>> FDT other than UEFI stuff, so we need to support it.)
>> So I think we will need this, but only for the ACPI-only case.
>
> I think you probably know better than I but I thought that the way the
> presence of ACPI was communicated to the kernel (whether via zImage or
> UEFI stub) was via entries in the FDT's /chosen/ node containing the
> address of the RSDP. Am I wrong about that?
>
> Or maybe I'm thinking about the UEFI services tables being referenced
> via /chosen/?
>
> Ian.
>
The ACPI tables are all referenced from the EFI system table, which is put
into the FDT by the EFI stub.  The EFI stub itself is completely ACPI agnostic -
it just passes along the EFI structures that may or may not contain ACPI tables.
The kernel will determine if ACPI support is present by looking for
the ACPI tables
in the EFI system table.

Roy

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH RFC 00/19] arm64 EFI stub
  2014-07-10 17:33         ` Roy Franz
@ 2014-07-11  8:57           ` Ian Campbell
  0 siblings, 0 replies; 44+ messages in thread
From: Ian Campbell @ 2014-07-11  8:57 UTC (permalink / raw)
  To: Roy Franz
  Cc: keir, tim, xen-devel, Stefano Stabellini, Jan Beulich,
	linaro-uefi, Fu Wei

On Thu, 2014-07-10 at 10:33 -0700, Roy Franz wrote:
> On Thu, Jul 10, 2014 at 12:58 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Wed, 2014-07-09 at 11:24 -0700, Roy Franz wrote:
> >> >> +    if ( fdtfile->ptr )
> >> >> +        fdt_size = fdt_totalsize(fdtfile->ptr);
> >> >> +    else
> >> >> +    {
> >> >> +        /* RFRANZ_TODO - missing fdt_create_empty_tree() in libfdt. */
> >> >> +        return NULL;
> >> >> +    }
> >> >
> >> >
> >> > I don't think we currently support booting with no FDT at all, and even
> >> > in the ACPI world we would get one. So I think this would be an error.
> >> > IOW no need to worry about it.
> >> >
> >>
> >> I think that for the ACPI-only case, the EFI stub will be the one
> >> responsible for creating the FDT for passing the UEFI/ACPI to the XEN
> >> kernel.  (I had overlooked the ACPI-only
> >> case when considering the no-FDT case - for Linux we support this
> >> since a kernel could be configured such that it needs nothing from the
> >> FDT other than UEFI stuff, so we need to support it.)
> >> So I think we will need this, but only for the ACPI-only case.
> >
> > I think you probably know better than I but I thought that the way the
> > presence of ACPI was communicated to the kernel (whether via zImage or
> > UEFI stub) was via entries in the FDT's /chosen/ node containing the
> > address of the RSDP. Am I wrong about that?
> >
> > Or maybe I'm thinking about the UEFI services tables being referenced
> > via /chosen/?
> >
> > Ian.
> >
> The ACPI tables are all referenced from the EFI system table, which is put
> into the FDT by the EFI stub.  The EFI stub itself is completely ACPI agnostic -
> it just passes along the EFI structures that may or may not contain ACPI tables.
> The kernel will determine if ACPI support is present by looking for
> the ACPI tables
> in the EFI system table.

Some folks at the Linaro ACPI sprint explained this to me. I think I was
confusing the ACPI pointers with the EFI handle, but even then I was
wrong since this can be passed as a formal parameter to the UEFI entry
point without any DTB being required. So you are right ;-)

Ian.

^ permalink raw reply	[flat|nested] 44+ messages in thread

end of thread, other threads:[~2014-07-11  8:57 UTC | newest]

Thread overview: 44+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-28  1:25 [PATCH RFC 00/19] arm64 EFI stub Roy Franz
2014-06-28  1:25 ` [PATCH RFC 01/19] HACK: Add -fshort-wchar to global build Roy Franz
2014-06-30 11:13   ` Jan Beulich
2014-06-28  1:25 ` [PATCH RFC 02/19] Create efi-shared.[ch], and move string functions Roy Franz
2014-06-30 11:15   ` Jan Beulich
2014-07-02 11:49     ` Ian Campbell
2014-07-02 11:55     ` Ian Campbell
2014-07-09 18:31       ` Roy Franz
2014-06-28  1:25 ` [PATCH RFC 03/19] Move more functions from boot.c to efi-shared.c Roy Franz
2014-06-28  1:25 ` [PATCH RFC 04/19] rename printErrMsg to PrintErrMesgExit Roy Franz
2014-06-28  1:25 ` [PATCH RFC 05/19] Add PrintErrMesg function that doesn't exit Roy Franz
2014-06-28  1:25 ` [PATCH RFC 06/19] Refactor read_file() so it can be shared Roy Franz
2014-06-28  1:25 ` [PATCH RFC 07/19] move read_file() to efi-shared.c Roy Franz
2014-06-28  1:25 ` [PATCH RFC 08/19] Move removal of leading spaces from split_value to get_value() Roy Franz
2014-06-28  1:25 ` [PATCH RFC 09/19] replace split_value() with truncate_string() Roy Franz
2014-06-28  1:25 ` [PATCH RFC 10/19] move truncate_string() to efi-shared.c Roy Franz
2014-06-28  1:25 ` [PATCH RFC 11/19] add read_config_file() function for XEN EFI config file Roy Franz
2014-06-28  1:25 ` [PATCH RFC 12/19] create handle_cmdline() function Roy Franz
2014-06-28  1:25 ` [PATCH RFC 13/19] Refactor get_argv() for sharing Roy Franz
2014-06-28  1:25 ` [PATCH RFC 14/19] Move get_argv() and handle_cmdline() to efi-shared.c Roy Franz
2014-06-28  1:25 ` [PATCH RFC 15/19] Add PE/COFF header in head.S Roy Franz
2014-07-02 12:02   ` Ian Campbell
2014-07-09 18:35     ` Roy Franz
2014-06-28  1:25 ` [PATCH RFC 16/19] create ARM EFI headers, based on x86 Roy Franz
2014-06-28  1:25 ` [PATCH RFC 17/19] Remove x86 specific defintions from efibind.h Roy Franz
2014-07-02 12:03   ` Ian Campbell
2014-07-09 18:27     ` Roy Franz
2014-07-10  7:59       ` Ian Campbell
2014-06-28  1:25 ` [PATCH RFC 18/19] Add assembler use support for efibind.h Roy Franz
2014-06-28  1:25 ` [PATCH RFC 19/19] Add EFI stub for ARM64 Roy Franz
2014-07-02 12:34   ` Ian Campbell
2014-07-09 19:15     ` Roy Franz
2014-07-10  8:13       ` Ian Campbell
2014-07-10  9:21         ` [Linaro-uefi] " Julien Grall
2014-07-10  9:33           ` Ian Campbell
2014-06-28 15:34 ` [PATCH RFC 00/19] arm64 EFI stub Roy Franz
2014-06-29 16:42 ` [Linaro-uefi] " Julien Grall
2014-06-30 11:12 ` Jan Beulich
2014-07-02 11:52 ` Ian Campbell
2014-07-02 12:31   ` Ian Campbell
2014-07-09 18:24     ` Roy Franz
2014-07-10  7:58       ` Ian Campbell
2014-07-10 17:33         ` Roy Franz
2014-07-11  8:57           ` Ian Campbell

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.