* [PATCH V2 01/12] Create efi-shared.[ch], and move string functions
2014-07-22 0:43 [PATCH V2 00/12] arm64 EFI stub Roy Franz
@ 2014-07-22 0:43 ` Roy Franz
2014-07-23 16:31 ` Jan Beulich
2014-07-22 0:43 ` [PATCH V2 02/12] rename printErrMsg to PrintErrMesgExit Roy Franz
` (11 subsequent siblings)
12 siblings, 1 reply; 50+ messages in thread
From: Roy Franz @ 2014-07-22 0:43 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.
Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
xen/arch/x86/Rules.mk | 1 +
xen/arch/x86/efi/boot.c | 127 +++-----------------------------------
xen/common/Makefile | 2 +
xen/common/efi/Makefile | 3 +
xen/common/efi/efi-shared.c | 142 +++++++++++++++++++++++++++++++++++++++++++
xen/include/efi/efi-shared.h | 50 +++++++++++++++
6 files changed, 205 insertions(+), 120 deletions(-)
create mode 100644 xen/common/efi/Makefile
create mode 100644 xen/common/efi/efi-shared.c
create mode 100644 xen/include/efi/efi-shared.h
diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
index 576985e..48f79a5 100644
--- a/xen/arch/x86/Rules.mk
+++ b/xen/arch/x86/Rules.mk
@@ -13,6 +13,7 @@ HAS_EHCI := y
HAS_KEXEC := y
HAS_GDBSX := y
xenoprof := y
+EFI := y
#
# If you change any of these configuration options then you must
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/common/Makefile b/xen/common/Makefile
index 3683ae3..42db42f 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -66,5 +66,7 @@ subdir-$(x86_64) += hvm
subdir-$(coverage) += gcov
+subdir-$(EFI) += efi
+
subdir-y += libelf
subdir-$(HAS_DEVICE_TREE) += libfdt
diff --git a/xen/common/efi/Makefile b/xen/common/efi/Makefile
new file mode 100644
index 0000000..c724ac2
--- /dev/null
+++ b/xen/common/efi/Makefile
@@ -0,0 +1,3 @@
+obj-y += efi-shared.o
+
+CFLAGS += -fshort-wchar
diff --git a/xen/common/efi/efi-shared.c b/xen/common/efi/efi-shared.c
new file mode 100644
index 0000000..b990292
--- /dev/null
+++ b/xen/common/efi/efi-shared.c
@@ -0,0 +1,142 @@
+/* EFI code shared between architectures. */
+
+#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>
+#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));
+}
+
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/efi/efi-shared.h b/xen/include/efi/efi-shared.h
new file mode 100644
index 0000000..38f8f39
--- /dev/null
+++ b/xen/include/efi/efi-shared.h
@@ -0,0 +1,50 @@
+#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
+
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.0.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH V2 01/12] Create efi-shared.[ch], and move string functions
2014-07-22 0:43 ` [PATCH V2 01/12] Create efi-shared.[ch], and move string functions Roy Franz
@ 2014-07-23 16:31 ` Jan Beulich
2014-07-28 15:41 ` Ian Campbell
2014-08-06 23:42 ` Roy Franz
0 siblings, 2 replies; 50+ messages in thread
From: Jan Beulich @ 2014-07-23 16:31 UTC (permalink / raw)
To: Roy Franz
Cc: keir, ian.campbell, tim, xen-devel, stefano.stabellini,
linaro-uefi, fu.wei
>>> On 22.07.14 at 02:43, <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.
>
> Signed-off-by: Roy Franz <roy.franz@linaro.org>
> ---
> xen/arch/x86/Rules.mk | 1 +
> xen/arch/x86/efi/boot.c | 127 +++-----------------------------------
> xen/common/Makefile | 2 +
> xen/common/efi/Makefile | 3 +
> xen/common/efi/efi-shared.c | 142 +++++++++++++++++++++++++++++++++++++++++++
This clearly should be just efi.c or, provided you keep the separation
between boot and runtime code, boot.c.
> xen/include/efi/efi-shared.h | 50 +++++++++++++++
This one should at least get the efi- prefix dropped as being redundant
with the efi/ one, or even be named internal.h.
> --- 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;
Why are these declarations not being moved into the shared
header? Plus, with them being just declarations, they should not
have the __initdata tags. And, with them being external now, they
should be properly prefixed with efi_ or Efi.
> +
>
Please avoid introducing double blank lines (not just here).
> -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;
In the end I'm not really happy anyway with them becoming non-
static - rather than building separate .o-s in the common efi/
directory, would it not be possible to just have the .c files there,
but include them from the arch ones? This would also address the
tool chain detection issue you described in the cover letter (without
the addressing of which I can't see how things will ultimately work).
> --- /dev/null
> +++ b/xen/common/efi/efi-shared.c
> @@ -0,0 +1,142 @@
> +/* EFI code shared between architectures. */
> +
> +#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>
> +#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>
Looks like you blindly copied all includes - I'd prefer only those to be
added that are actually needed.
> --- /dev/null
> +++ b/xen/include/efi/efi-shared.h
> @@ -0,0 +1,50 @@
> +#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);
Again no __init on declarations please. And hence no need to include
xen/init.h here.
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH V2 01/12] Create efi-shared.[ch], and move string functions
2014-07-23 16:31 ` Jan Beulich
@ 2014-07-28 15:41 ` Ian Campbell
2014-07-28 15:52 ` Jan Beulich
2014-08-06 23:42 ` Roy Franz
1 sibling, 1 reply; 50+ messages in thread
From: Ian Campbell @ 2014-07-28 15:41 UTC (permalink / raw)
To: Jan Beulich
Cc: keir, tim, xen-devel, Roy Franz, stefano.stabellini, linaro-uefi, fu.wei
On Wed, 2014-07-23 at 17:31 +0100, Jan Beulich wrote:
> > -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;
>
> In the end I'm not really happy anyway with them becoming non-
> static - rather than building separate .o-s in the common efi/
> directory, would it not be possible to just have the .c files there,
> but include them from the arch ones?
That alternative seems pretty gross to me, what is the problem with a
global EfiStdOut or something like that?
> This would also address the
> tool chain detection issue you described in the cover letter (without
> the addressing of which I can't see how things will ultimately work).
In the case where the toolchain doesn't EFI won't the unnecessary code
built in xen/common/efi simply get discarded by the linker?
Even if not it looks like ~20K of mostly __init stuff, which doesn't
seem like the end of the world, especially given that more and more
toolstacks do support EFI with time.
Ian.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH V2 01/12] Create efi-shared.[ch], and move string functions
2014-07-28 15:41 ` Ian Campbell
@ 2014-07-28 15:52 ` Jan Beulich
2014-07-28 15:56 ` Ian Campbell
0 siblings, 1 reply; 50+ messages in thread
From: Jan Beulich @ 2014-07-28 15:52 UTC (permalink / raw)
To: Ian Campbell
Cc: keir, tim, xen-devel, Roy Franz, stefano.stabellini, linaro-uefi, fu.wei
>>> On 28.07.14 at 17:41, <Ian.Campbell@citrix.com> wrote:
> On Wed, 2014-07-23 at 17:31 +0100, Jan Beulich wrote:
>> > -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;
>>
>> In the end I'm not really happy anyway with them becoming non-
>> static - rather than building separate .o-s in the common efi/
>> directory, would it not be possible to just have the .c files there,
>> but include them from the arch ones?
>
> That alternative seems pretty gross to me, what is the problem with a
> global EfiStdOut or something like that?
It's not a big problem, but I still prefer to avoid making symbols
global whenever possible.
>> This would also address the
>> tool chain detection issue you described in the cover letter (without
>> the addressing of which I can't see how things will ultimately work).
>
> In the case where the toolchain doesn't EFI won't the unnecessary code
> built in xen/common/efi simply get discarded by the linker?
Not that I'm aware of - afaik no code or data inside a .o would
ever get thrown away by the linker without it being specifically
asked to do so.
> Even if not it looks like ~20K of mostly __init stuff, which doesn't
> seem like the end of the world, especially given that more and more
> toolstacks do support EFI with time.
Right now - with the runtime code not moved over yet - it's
mostly __init. Plus (with the linker not being able to discard that
code) it carries the risk of having references to symbols that
don't exist in the non-EFI build.
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH V2 01/12] Create efi-shared.[ch], and move string functions
2014-07-28 15:52 ` Jan Beulich
@ 2014-07-28 15:56 ` Ian Campbell
2014-07-28 16:00 ` Jan Beulich
0 siblings, 1 reply; 50+ messages in thread
From: Ian Campbell @ 2014-07-28 15:56 UTC (permalink / raw)
To: Jan Beulich
Cc: keir, tim, xen-devel, Roy Franz, stefano.stabellini, linaro-uefi, fu.wei
On Mon, 2014-07-28 at 16:52 +0100, Jan Beulich wrote:
> >> This would also address the
> >> tool chain detection issue you described in the cover letter (without
> >> the addressing of which I can't see how things will ultimately work).
> >
> > In the case where the toolchain doesn't EFI won't the unnecessary code
> > built in xen/common/efi simply get discarded by the linker?
>
> Not that I'm aware of - afaik no code or data inside a .o would
> ever get thrown away by the linker without it being specifically
> asked to do so.
Hrm perhaps I'm thinking of one of the whole programmer optimisation
things then.
>
> > Even if not it looks like ~20K of mostly __init stuff, which doesn't
> > seem like the end of the world, especially given that more and more
> > toolstacks do support EFI with time.
>
> Right now - with the runtime code not moved over yet - it's
> mostly __init. Plus (with the linker not being able to discard that
> code) it carries the risk of having references to symbols that
> don't exist in the non-EFI build.
Perhaps we can put the relevant code into efi specific sections and DTRT
in xen.lds.S?
Ian.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH V2 01/12] Create efi-shared.[ch], and move string functions
2014-07-28 15:56 ` Ian Campbell
@ 2014-07-28 16:00 ` Jan Beulich
2014-07-28 16:04 ` Ian Campbell
0 siblings, 1 reply; 50+ messages in thread
From: Jan Beulich @ 2014-07-28 16:00 UTC (permalink / raw)
To: Ian Campbell
Cc: keir, tim, xen-devel, Roy Franz, stefano.stabellini, linaro-uefi, fu.wei
>>> On 28.07.14 at 17:56, <Ian.Campbell@citrix.com> wrote:
> On Mon, 2014-07-28 at 16:52 +0100, Jan Beulich wrote:
>> > Even if not it looks like ~20K of mostly __init stuff, which doesn't
>> > seem like the end of the world, especially given that more and more
>> > toolstacks do support EFI with time.
>>
>> Right now - with the runtime code not moved over yet - it's
>> mostly __init. Plus (with the linker not being able to discard that
>> code) it carries the risk of having references to symbols that
>> don't exist in the non-EFI build.
>
> Perhaps we can put the relevant code into efi specific sections and DTRT
> in xen.lds.S?
Maybe it could be made work, but I'd be wary of linker version issues
then.
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH V2 01/12] Create efi-shared.[ch], and move string functions
2014-07-28 16:00 ` Jan Beulich
@ 2014-07-28 16:04 ` Ian Campbell
2014-07-28 16:10 ` Jan Beulich
0 siblings, 1 reply; 50+ messages in thread
From: Ian Campbell @ 2014-07-28 16:04 UTC (permalink / raw)
To: Jan Beulich
Cc: keir, tim, xen-devel, Roy Franz, stefano.stabellini, linaro-uefi, fu.wei
On Mon, 2014-07-28 at 17:00 +0100, Jan Beulich wrote:
> >>> On 28.07.14 at 17:56, <Ian.Campbell@citrix.com> wrote:
> > On Mon, 2014-07-28 at 16:52 +0100, Jan Beulich wrote:
> >> > Even if not it looks like ~20K of mostly __init stuff, which doesn't
> >> > seem like the end of the world, especially given that more and more
> >> > toolstacks do support EFI with time.
> >>
> >> Right now - with the runtime code not moved over yet - it's
> >> mostly __init. Plus (with the linker not being able to discard that
> >> code) it carries the risk of having references to symbols that
> >> don't exist in the non-EFI build.
> >
> > Perhaps we can put the relevant code into efi specific sections and DTRT
> > in xen.lds.S?
>
> Maybe it could be made work, but I'd be wary of linker version issues
> then.
Rather than arch .c files including common .c (or .inc) files how about
making xen/arch/*/efi/Makefile link xen/commmon/efi/built-in.o into it's
own built-in.o instead of having xen/common/Makefile do it like would
normally happen?
Ian.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH V2 01/12] Create efi-shared.[ch], and move string functions
2014-07-28 16:04 ` Ian Campbell
@ 2014-07-28 16:10 ` Jan Beulich
2014-08-06 23:55 ` Roy Franz
0 siblings, 1 reply; 50+ messages in thread
From: Jan Beulich @ 2014-07-28 16:10 UTC (permalink / raw)
To: Ian Campbell
Cc: keir, tim, xen-devel, Roy Franz, stefano.stabellini, linaro-uefi, fu.wei
>>> On 28.07.14 at 18:04, <Ian.Campbell@citrix.com> wrote:
> On Mon, 2014-07-28 at 17:00 +0100, Jan Beulich wrote:
>> >>> On 28.07.14 at 17:56, <Ian.Campbell@citrix.com> wrote:
>> > On Mon, 2014-07-28 at 16:52 +0100, Jan Beulich wrote:
>> >> > Even if not it looks like ~20K of mostly __init stuff, which doesn't
>> >> > seem like the end of the world, especially given that more and more
>> >> > toolstacks do support EFI with time.
>> >>
>> >> Right now - with the runtime code not moved over yet - it's
>> >> mostly __init. Plus (with the linker not being able to discard that
>> >> code) it carries the risk of having references to symbols that
>> >> don't exist in the non-EFI build.
>> >
>> > Perhaps we can put the relevant code into efi specific sections and DTRT
>> > in xen.lds.S?
>>
>> Maybe it could be made work, but I'd be wary of linker version issues
>> then.
>
> Rather than arch .c files including common .c (or .inc) files how about
> making xen/arch/*/efi/Makefile link xen/commmon/efi/built-in.o into it's
> own built-in.o instead of having xen/common/Makefile do it like would
> normally happen?
That's an option. But I agree the inclusion of .c in another .c isn't
really nice; I would therefore anyway favor a (set of) arch header
file(s) providing everything the common code can't do on its own.
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH V2 01/12] Create efi-shared.[ch], and move string functions
2014-07-28 16:10 ` Jan Beulich
@ 2014-08-06 23:55 ` Roy Franz
2014-08-07 6:17 ` Jan Beulich
0 siblings, 1 reply; 50+ messages in thread
From: Roy Franz @ 2014-08-06 23:55 UTC (permalink / raw)
To: Jan Beulich
Cc: keir, Ian Campbell, tim, xen-devel, Stefano Stabellini,
linaro-uefi, Fu Wei
On Mon, Jul 28, 2014 at 9:10 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 28.07.14 at 18:04, <Ian.Campbell@citrix.com> wrote:
>> On Mon, 2014-07-28 at 17:00 +0100, Jan Beulich wrote:
>>> >>> On 28.07.14 at 17:56, <Ian.Campbell@citrix.com> wrote:
>>> > On Mon, 2014-07-28 at 16:52 +0100, Jan Beulich wrote:
>>> >> > Even if not it looks like ~20K of mostly __init stuff, which doesn't
>>> >> > seem like the end of the world, especially given that more and more
>>> >> > toolstacks do support EFI with time.
>>> >>
>>> >> Right now - with the runtime code not moved over yet - it's
>>> >> mostly __init. Plus (with the linker not being able to discard that
>>> >> code) it carries the risk of having references to symbols that
>>> >> don't exist in the non-EFI build.
>>> >
>>> > Perhaps we can put the relevant code into efi specific sections and DTRT
>>> > in xen.lds.S?
>>>
>>> Maybe it could be made work, but I'd be wary of linker version issues
>>> then.
>>
>> Rather than arch .c files including common .c (or .inc) files how about
>> making xen/arch/*/efi/Makefile link xen/commmon/efi/built-in.o into it's
>> own built-in.o instead of having xen/common/Makefile do it like would
>> normally happen?
>
> That's an option. But I agree the inclusion of .c in another .c isn't
> really nice; I would therefore anyway favor a (set of) arch header
> file(s) providing everything the common code can't do on its own.
>
> Jan
>
I think I have pretty good handle addressing all of the feedback with
the exception of this build issue. Jan - I don't understand your above
suggestion.
Another way to handle this would be for the x86 EFI code to copy
(or link) the efi-shared.c file locally for building, where it's use would
be controlled by the PE/COFF toolchain autodetection. It seems that
some kind of games will need to be played to have the autodetection
cooperate with building shared code.
Thanks,
Roy
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH V2 01/12] Create efi-shared.[ch], and move string functions
2014-08-06 23:55 ` Roy Franz
@ 2014-08-07 6:17 ` Jan Beulich
2014-08-09 0:27 ` Roy Franz
0 siblings, 1 reply; 50+ messages in thread
From: Jan Beulich @ 2014-08-07 6:17 UTC (permalink / raw)
To: Roy Franz
Cc: keir, Ian Campbell, tim, xen-devel, Stefano Stabellini,
linaro-uefi, Fu Wei
>>> On 07.08.14 at 01:55, <roy.franz@linaro.org> wrote:
> On Mon, Jul 28, 2014 at 9:10 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 28.07.14 at 18:04, <Ian.Campbell@citrix.com> wrote:
>>> Rather than arch .c files including common .c (or .inc) files how about
>>> making xen/arch/*/efi/Makefile link xen/commmon/efi/built-in.o into it's
>>> own built-in.o instead of having xen/common/Makefile do it like would
>>> normally happen?
>>
>> That's an option. But I agree the inclusion of .c in another .c isn't
>> really nice; I would therefore anyway favor a (set of) arch header
>> file(s) providing everything the common code can't do on its own.
>
> I think I have pretty good handle addressing all of the feedback with
> the exception of this build issue. Jan - I don't understand your above
> suggestion.
The process would be
- move all files containing pieces you want to re-use from arch/x86/efi/
to common/efi/, with no other changes than to make them build again
- possibly in a step by step manner, replace any code portions not
suitable for non-x86 by inline functions placed in said header
- add ARM variants of anything moved to the x86-specific helper
header (and possible empty x86 variants of stuff ARM needs that
x86 doesn't)
> Another way to handle this would be for the x86 EFI code to copy
> (or link) the efi-shared.c file locally for building, where it's use would
> be controlled by the PE/COFF toolchain autodetection. It seems that
> some kind of games will need to be played to have the autodetection
> cooperate with building shared code.
Yes, that would be an option too, with the linking done in a prerequisite
build step. While I generally dislike this model, I do see its simplicity
being a possible advantage here.
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH V2 01/12] Create efi-shared.[ch], and move string functions
2014-08-07 6:17 ` Jan Beulich
@ 2014-08-09 0:27 ` Roy Franz
0 siblings, 0 replies; 50+ messages in thread
From: Roy Franz @ 2014-08-09 0:27 UTC (permalink / raw)
To: Jan Beulich
Cc: keir, Ian Campbell, tim, xen-devel, Stefano Stabellini,
linaro-uefi, Fu Wei
On Wed, Aug 6, 2014 at 11:17 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 07.08.14 at 01:55, <roy.franz@linaro.org> wrote:
>> On Mon, Jul 28, 2014 at 9:10 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 28.07.14 at 18:04, <Ian.Campbell@citrix.com> wrote:
>>>> Rather than arch .c files including common .c (or .inc) files how about
>>>> making xen/arch/*/efi/Makefile link xen/commmon/efi/built-in.o into it's
>>>> own built-in.o instead of having xen/common/Makefile do it like would
>>>> normally happen?
>>>
>>> That's an option. But I agree the inclusion of .c in another .c isn't
>>> really nice; I would therefore anyway favor a (set of) arch header
>>> file(s) providing everything the common code can't do on its own.
>>
>> I think I have pretty good handle addressing all of the feedback with
>> the exception of this build issue. Jan - I don't understand your above
>> suggestion.
>
> The process would be
> - move all files containing pieces you want to re-use from arch/x86/efi/
> to common/efi/, with no other changes than to make them build again
> - possibly in a step by step manner, replace any code portions not
> suitable for non-x86 by inline functions placed in said header
> - add ARM variants of anything moved to the x86-specific helper
> header (and possible empty x86 variants of stuff ARM needs that
> x86 doesn't)
>
OK, I was missing the inline function bit of it. I took a quick look at
more of what the x86 boot.c code does and I'm still not a fan of this.
I think the differences of device tree versus not, e820 maps, etc will lead
to either conditional code, or awkward helper functions.
>From a more practical view, this would be a major refactor of the x86 code,
which I don't think I could complete in the XEN 4.5 timeline.
>> Another way to handle this would be for the x86 EFI code to copy
>> (or link) the efi-shared.c file locally for building, where it's use would
>> be controlled by the PE/COFF toolchain autodetection. It seems that
>> some kind of games will need to be played to have the autodetection
>> cooperate with building shared code.
>
> Yes, that would be an option too, with the linking done in a prerequisite
> build step. While I generally dislike this model, I do see its simplicity
> being a possible advantage here.
I'm not a huge fan of it either, but I think it's nicer that
#including a C file,
and it keeps all of the clever makefile bits in the x86/efi directory. It's the
x86 EFI autoconfig that is complicating the build, so this keeps all
the trickery
together. The Linux self-decompressor uses this to get various bits of libfdt.
I think the makefile copying the shared c file is better than what I have now,
as it avoids the dead code in the non-efi x86 case (which was a
consequence I had
overlooked at the time.)
I'm going to implement this as part of my next version of the patch
series, which I should
have out early next week.
Roy
>
> Jan
>
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH V2 01/12] Create efi-shared.[ch], and move string functions
2014-07-23 16:31 ` Jan Beulich
2014-07-28 15:41 ` Ian Campbell
@ 2014-08-06 23:42 ` Roy Franz
1 sibling, 0 replies; 50+ messages in thread
From: Roy Franz @ 2014-08-06 23:42 UTC (permalink / raw)
To: Jan Beulich
Cc: keir, Ian Campbell, tim, xen-devel, Stefano Stabellini,
linaro-uefi, Fu Wei
On Wed, Jul 23, 2014 at 9:31 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 22.07.14 at 02:43, <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.
>>
>> Signed-off-by: Roy Franz <roy.franz@linaro.org>
>> ---
>> xen/arch/x86/Rules.mk | 1 +
>> xen/arch/x86/efi/boot.c | 127 +++-----------------------------------
>> xen/common/Makefile | 2 +
>> xen/common/efi/Makefile | 3 +
>> xen/common/efi/efi-shared.c | 142 +++++++++++++++++++++++++++++++++++++++++++
>
> This clearly should be just efi.c or, provided you keep the separation
> between boot and runtime code, boot.c.
>
>> xen/include/efi/efi-shared.h | 50 +++++++++++++++
>
> This one should at least get the efi- prefix dropped as being redundant
> with the efi/ one, or even be named internal.h.
>
>> --- 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;
>
> Why are these declarations not being moved into the shared
> header? Plus, with them being just declarations, they should not
> have the __initdata tags. And, with them being external now, they
> should be properly prefixed with efi_ or Efi.
moved/fixed
>
>> +
>>
>
> Please avoid introducing double blank lines (not just here).
>
>> -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;
>
> In the end I'm not really happy anyway with them becoming non-
> static - rather than building separate .o-s in the common efi/
> directory, would it not be possible to just have the .c files there,
> but include them from the arch ones? This would also address the
> tool chain detection issue you described in the cover letter (without
> the addressing of which I can't see how things will ultimately work).
>
>> --- /dev/null
>> +++ b/xen/common/efi/efi-shared.c
>> @@ -0,0 +1,142 @@
>> +/* EFI code shared between architectures. */
>> +
>> +#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>
>> +#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>
>
> Looks like you blindly copied all includes - I'd prefer only those to be
> added that are actually needed.
fixed
>
>> --- /dev/null
>> +++ b/xen/include/efi/efi-shared.h
>> @@ -0,0 +1,50 @@
>> +#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);
>
> Again no __init on declarations please. And hence no need to include
> xen/init.h here.
fixed.
>
> Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH V2 02/12] rename printErrMsg to PrintErrMesgExit
2014-07-22 0:43 [PATCH V2 00/12] arm64 EFI stub Roy Franz
2014-07-22 0:43 ` [PATCH V2 01/12] Create efi-shared.[ch], and move string functions Roy Franz
@ 2014-07-22 0:43 ` Roy Franz
2014-07-23 16:33 ` Jan Beulich
2014-07-22 0:43 ` [PATCH V2 03/12] Refactor get_parent_handle for sharing Roy Franz
` (10 subsequent siblings)
12 siblings, 1 reply; 50+ messages in thread
From: Roy Franz @ 2014-07-22 0:43 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.
Add PrintErrMesg that doesn't exit.
Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
xen/arch/x86/efi/boot.c | 30 ++++++++++++++++++------------
1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/xen/arch/x86/efi/boot.c b/xen/arch/x86/efi/boot.c
index 2b6bea3..849dc2d 100644
--- a/xen/arch/x86/efi/boot.c
+++ b/xen/arch/x86/efi/boot.c
@@ -92,7 +92,7 @@ static void __init noreturn blexit(const CHAR16 *str)
}
/* generic routine for printing error messages */
-static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode)
+void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode)
{
StdOut = StdErr;
PrintErr((CHAR16 *)mesg);
@@ -139,6 +139,12 @@ static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode)
mesg = NULL;
break;
}
+}
+
+/* generic routine for printing error messages */
+static void __init PrintErrMesgExit(const CHAR16 *mesg, EFI_STATUS ErrCode)
+{
+ PrintErrMesg(mesg, ErrCode);
blexit(mesg);
}
@@ -231,12 +237,12 @@ static EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
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",
+ PrintErrMesgExit(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);
+ PrintErrMesgExit(L"OpenVolume failure", ret);
#define buffer ((CHAR16 *)keyhandler_scratch)
#define BUFFERSIZE sizeof(keyhandler_scratch)
@@ -259,7 +265,7 @@ static 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;
@@ -286,7 +292,7 @@ static 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;
@@ -326,7 +332,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 )
@@ -387,7 +393,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;
@@ -466,7 +472,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++ = ' ';
@@ -688,7 +694,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 )
@@ -856,7 +862,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
(void **)&shim_lock)) &&
(status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS )
- PrintErrMesg(L"Dom0 kernel image could not be verified", status);
+ PrintErrMesgExit(L"Dom0 kernel image could not be verified", status);
name.s = get_value(&cfg, section.s, "ramdisk");
if ( name.s )
@@ -1277,7 +1283,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
status = efi_bs->GetMemoryMap(&efi_memmap_size, efi_memmap, &map_key,
&efi_mdesc_size, &mdesc_ver);
if ( EFI_ERROR(status) )
- PrintErrMesg(L"Cannot obtain memory map", status);
+ PrintErrMesgExit(L"Cannot obtain memory map", status);
/* Populate E820 table and check trampoline area availability. */
e = e820map - 1;
@@ -1337,7 +1343,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;
--
2.0.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH V2 02/12] rename printErrMsg to PrintErrMesgExit
2014-07-22 0:43 ` [PATCH V2 02/12] rename printErrMsg to PrintErrMesgExit Roy Franz
@ 2014-07-23 16:33 ` Jan Beulich
0 siblings, 0 replies; 50+ messages in thread
From: Jan Beulich @ 2014-07-23 16:33 UTC (permalink / raw)
To: Roy Franz
Cc: keir, ian.campbell, tim, xen-devel, stefano.stabellini,
linaro-uefi, fu.wei
>>> On 22.07.14 at 02:43, <roy.franz@linaro.org> wrote:
> The function prints an error message, then exits the program.
> Add PrintErrMesg that doesn't exit.
Is there indeed a case where an error message is to be printed, but
we don't want to exit? Also, if the patch is being retained please fix
the subject to have the function name case-correct.
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH V2 03/12] Refactor get_parent_handle for sharing
2014-07-22 0:43 [PATCH V2 00/12] arm64 EFI stub Roy Franz
2014-07-22 0:43 ` [PATCH V2 01/12] Create efi-shared.[ch], and move string functions Roy Franz
2014-07-22 0:43 ` [PATCH V2 02/12] rename printErrMsg to PrintErrMesgExit Roy Franz
@ 2014-07-22 0:43 ` Roy Franz
2014-07-23 16:37 ` Jan Beulich
2014-07-22 0:43 ` [PATCH V2 04/12] Refactor read_file() so it can be shared Roy Franz
` (9 subsequent siblings)
12 siblings, 1 reply; 50+ messages in thread
From: Roy Franz @ 2014-07-22 0:43 UTC (permalink / raw)
To: xen-devel, ian.campbell, stefano.stabellini, tim, jbeulich, keir
Cc: Roy Franz, fu.wei, linaro-uefi
Refactor get_parent_handle() to return NULL on error rather than directly
exiting, as the exit cleanup code is arch specific. Having the common code
reference the architecture specific code causes pre-linking to fail.
Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
xen/arch/x86/efi/boot.c | 29 +++++++++++++++++++++++------
1 file changed, 23 insertions(+), 6 deletions(-)
diff --git a/xen/arch/x86/efi/boot.c b/xen/arch/x86/efi/boot.c
index 849dc2d..c0546ec 100644
--- a/xen/arch/x86/efi/boot.c
+++ b/xen/arch/x86/efi/boot.c
@@ -237,12 +237,18 @@ static EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
ret = efi_bs->HandleProtocol(loaded_image->DeviceHandle,
&fs_protocol, (void **)&fio);
if ( EFI_ERROR(ret) )
- PrintErrMesgExit(L"Couldn't obtain the File System Protocol Interface",
+ {
+ PrintErrMesg(L"Couldn't obtain the File System Protocol Interface",
ret);
+ return NULL;
+ }
ret = fio->OpenVolume(fio, &dir_handle);
} while ( ret == EFI_MEDIA_CHANGED );
if ( ret != EFI_SUCCESS )
- PrintErrMesgExit(L"OpenVolume failure", ret);
+ {
+ PrintErrMesg(L"OpenVolume failure", ret);
+ return NULL;
+ }
#define buffer ((CHAR16 *)keyhandler_scratch)
#define BUFFERSIZE sizeof(keyhandler_scratch)
@@ -254,7 +260,10 @@ static EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
if ( DevicePathType(dp) != MEDIA_DEVICE_PATH ||
DevicePathSubType(dp) != MEDIA_FILEPATH_DP )
- blexit(L"Unsupported device path component");
+ {
+ PrintErr(L"Unsupported device path component");
+ return NULL;
+ }
if ( *buffer )
{
@@ -265,7 +274,8 @@ static EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
if ( ret != EFI_SUCCESS )
{
PrintErr(L"Open failed for ");
- PrintErrMesgExit(buffer, ret);
+ PrintErrMesg(buffer, ret);
+ return NULL;
}
dir_handle->Close(dir_handle);
dir_handle = new_handle;
@@ -273,7 +283,10 @@ static EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
fp = (void *)dp;
if ( BUFFERSIZE < DevicePathNodeLength(dp) -
sizeof(*dp) + sizeof(*buffer) )
- blexit(L"Increase BUFFERSIZE");
+ {
+ PrintErr(L"Increase BUFFERSIZE");
+ return NULL;
+ }
memcpy(buffer, fp->PathName, DevicePathNodeLength(dp) - sizeof(*dp));
buffer[(DevicePathNodeLength(dp) - sizeof(*dp)) / sizeof(*buffer)] = 0;
}
@@ -292,7 +305,8 @@ static 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 ");
- PrintErrMesgExit(buffer, ret);
+ PrintErrMesg(buffer, ret);
+ return NULL;
}
dir_handle->Close(dir_handle);
dir_handle = new_handle;
@@ -706,6 +720,9 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
/* Get the file system interface. */
dir_handle = get_parent_handle(loaded_image, &file_name);
+ if ( !dir_handle )
+ blexit(L"EFI get_parent_handle failed.");
+
argc = get_argv(0, NULL, loaded_image->LoadOptions,
loaded_image->LoadOptionsSize);
if ( argc > 0 &&
--
2.0.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH V2 03/12] Refactor get_parent_handle for sharing
2014-07-22 0:43 ` [PATCH V2 03/12] Refactor get_parent_handle for sharing Roy Franz
@ 2014-07-23 16:37 ` Jan Beulich
0 siblings, 0 replies; 50+ messages in thread
From: Jan Beulich @ 2014-07-23 16:37 UTC (permalink / raw)
To: Roy Franz
Cc: keir, ian.campbell, tim, xen-devel, stefano.stabellini,
linaro-uefi, fu.wei
>>> On 22.07.14 at 02:43, <roy.franz@linaro.org> wrote:
> Refactor get_parent_handle() to return NULL on error rather than directly
> exiting, as the exit cleanup code is arch specific.
Is it? Not at this point at least.
> Having the common code
> reference the architecture specific code causes pre-linking to fail.
Which certainly could be addressed if you told us details.
> @@ -706,6 +720,9 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
> /* Get the file system interface. */
> dir_handle = get_parent_handle(loaded_image, &file_name);
>
> + if ( !dir_handle )
Please avoid the rather pointless blank line above the if().
> + blexit(L"EFI get_parent_handle failed.");
And please don't issue meaningless messages - the called function
already prints messages on all failure paths.
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH V2 04/12] Refactor read_file() so it can be shared.
2014-07-22 0:43 [PATCH V2 00/12] arm64 EFI stub Roy Franz
` (2 preceding siblings ...)
2014-07-22 0:43 ` [PATCH V2 03/12] Refactor get_parent_handle for sharing Roy Franz
@ 2014-07-22 0:43 ` Roy Franz
2014-07-24 7:09 ` Jan Beulich
2014-07-22 0:43 ` [PATCH V2 05/12] replace split_value() with truncate_string() Roy Franz
` (8 subsequent siblings)
12 siblings, 1 reply; 50+ messages in thread
From: Roy Franz @ 2014-07-22 0:43 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. read_file() and load_file() return
error codes rather than directly exiting on error to facilicate
sharing. Different architectures may require different max allocation
addresses so take that as an argument.
Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
xen/arch/x86/efi/boot.c | 89 ++++++++++++++++++++++++++++++++-----------------
1 file changed, 59 insertions(+), 30 deletions(-)
diff --git a/xen/arch/x86/efi/boot.c b/xen/arch/x86/efi/boot.c
index c0546ec..17aaa67 100644
--- a/xen/arch/x86/efi/boot.c
+++ b/xen/arch/x86/efi/boot.c
@@ -338,7 +338,7 @@ static CHAR16 *__init point_tail(CHAR16 *fn)
}
static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
- struct file *file)
+ struct file *file, EFI_PHYSICAL_ADDRESS max_addr)
{
EFI_FILE_HANDLE FileHandle = NULL;
UINT64 size;
@@ -346,11 +346,14 @@ 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);
+ {
+ PrintErrMesg(L"No Filename", EFI_OUT_OF_RESOURCES);
+ return 0;
+ }
+
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
@@ -367,8 +370,7 @@ static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
what = what ?: L"Seek";
else
{
- file->addr = min(1UL << (32 + PAGE_SHIFT),
- HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
+ file->addr = max_addr;
ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
PFN_UP(size), &file->addr);
}
@@ -379,25 +381,17 @@ 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 )
@@ -405,12 +399,21 @@ static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
if ( what )
{
- PrintErr(what);
- PrintErr(L" failed for ");
- PrintErrMesgExit(name, ret);
+ PrintErrMesg(what, ret);
+ PrintErr(L"Unable to load file");
+ return 0;
+ }
+ 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;
}
static void __init pre_parse(const struct file *cfg)
@@ -470,6 +473,21 @@ static char *__init get_value(const struct file *cfg, const char *section,
}
return NULL;
}
+/* Only call with non-config files. */
+bool_t __init load_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
+ struct file *file)
+{
+ EFI_PHYSICAL_ADDRESS max = min(1UL << (32 + PAGE_SHIFT),
+ HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
+ if ( read_file(dir_handle, name, file, max) )
+ {
+ mb_modules[mbi.mods_count].mod_start = file->addr >> PAGE_SHIFT;
+ mb_modules[mbi.mods_count].mod_end = file->size;
+ ++mbi.mods_count;
+ return 1;
+ }
+ return 0;
+}
static void __init split_value(char *s)
{
@@ -692,6 +710,9 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
struct e820entry *e;
u64 efer;
bool_t base_video = 0;
+ bool_t load_ok = 0;
+ EFI_PHYSICAL_ADDRESS max_addr = min(1UL << (32 + PAGE_SHIFT),
+ HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
efi_ih = ImageHandle;
efi_bs = SystemTable->BootServices;
@@ -831,7 +852,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
while ( (tail = point_tail(file_name)) != NULL )
{
wstrcpy(tail, L".cfg");
- if ( read_file(dir_handle, file_name, &cfg) )
+ if ( read_file(dir_handle, file_name, &cfg, max_addr) )
break;
*tail = 0;
}
@@ -841,7 +862,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
PrintStr(file_name);
PrintStr(L"'\r\n");
}
- else if ( !read_file(dir_handle, cfg_file_name, &cfg) )
+ else if ( !read_file(dir_handle, cfg_file_name, &cfg, max_addr) )
blexit(L"Configuration file not found.");
pre_parse(&cfg);
@@ -860,7 +881,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
break;
efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
cfg.addr = 0;
- if ( !read_file(dir_handle, s2w(&name), &cfg) )
+ if ( !read_file(dir_handle, s2w(&name), &cfg, max_addr) )
{
PrintStr(L"Chained configuration file '");
PrintStr(name.w);
@@ -873,8 +894,10 @@ 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_ok = load_file(dir_handle, s2w(&name), &kernel);
efi_bs->FreePool(name.w);
+ if ( !load_ok )
+ blexit(L"Unable to load Dom0 Kernel image.");
if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
(void **)&shim_lock)) &&
@@ -885,8 +908,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
if ( name.s )
{
split_value(name.s);
- read_file(dir_handle, s2w(&name), &ramdisk);
+ load_ok = load_file(dir_handle, s2w(&name), &ramdisk);
efi_bs->FreePool(name.w);
+ if ( !load_ok )
+ blexit(L"Unable to load ramdisk image.");
}
name.s = get_value(&cfg, section.s, "ucode");
@@ -896,16 +921,20 @@ 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_ok = load_file(dir_handle, s2w(&name), &ucode);
efi_bs->FreePool(name.w);
+ if ( !load_ok )
+ blexit(L"Unable to load ucode image.");
}
name.s = get_value(&cfg, section.s, "xsm");
if ( name.s )
{
split_value(name.s);
- read_file(dir_handle, s2w(&name), &xsm);
+ load_ok = load_file(dir_handle, s2w(&name), &xsm);
efi_bs->FreePool(name.w);
+ if ( !load_ok )
+ blexit(L"Unable to load ucode image.");
}
name.s = get_value(&cfg, section.s, "options");
--
2.0.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH V2 04/12] Refactor read_file() so it can be shared.
2014-07-22 0:43 ` [PATCH V2 04/12] Refactor read_file() so it can be shared Roy Franz
@ 2014-07-24 7:09 ` Jan Beulich
2014-08-06 18:38 ` Roy Franz
0 siblings, 1 reply; 50+ messages in thread
From: Jan Beulich @ 2014-07-24 7:09 UTC (permalink / raw)
To: Roy Franz
Cc: keir, ian.campbell, tim, xen-devel, stefano.stabellini,
linaro-uefi, fu.wei
>>> On 22.07.14 at 02:43, <roy.franz@linaro.org> wrote:
> 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. read_file() and load_file() return
> error codes rather than directly exiting on error to facilicate
> sharing. Different architectures may require different max allocation
> addresses so take that as an argument.
Unless you expect an architecture to pass in different values on
different invocations this clearly can be a #define rather than a
function parameter.
> @@ -405,12 +399,21 @@ static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>
> if ( what )
> {
> - PrintErr(what);
> - PrintErr(L" failed for ");
> - PrintErrMesgExit(name, ret);
> + PrintErrMesg(what, ret);
> + PrintErr(L"Unable to load file");
Is it intentional to make the message less useful by dripping the
printing of the file name?
> + return 0;
> + }
> + else
No need for an "else" after an unconditional "return" in the "if"
branch.
> @@ -470,6 +473,21 @@ static char *__init get_value(const struct file *cfg, const char *section,
> }
> return NULL;
> }
> +/* Only call with non-config files. */
Missing blank line before this comment.
> +bool_t __init load_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
> + struct file *file)
> +{
> + EFI_PHYSICAL_ADDRESS max = min(1UL << (32 + PAGE_SHIFT),
> + HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
> + if ( read_file(dir_handle, name, file, max) )
Missing blank line between declaration and first statement.
> @@ -896,16 +921,20 @@ 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_ok = load_file(dir_handle, s2w(&name), &ucode);
> efi_bs->FreePool(name.w);
> + if ( !load_ok )
> + blexit(L"Unable to load ucode image.");
> }
>
> name.s = get_value(&cfg, section.s, "xsm");
> if ( name.s )
> {
> split_value(name.s);
> - read_file(dir_handle, s2w(&name), &xsm);
> + load_ok = load_file(dir_handle, s2w(&name), &xsm);
> efi_bs->FreePool(name.w);
> + if ( !load_ok )
> + blexit(L"Unable to load ucode image.");
Apart from the same comment made on an earlier patch - no need
for an extra message here when the called function already printed
one - this is a copy'n'paste mistake: You mean XSM instead of ucode
here.
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH V2 04/12] Refactor read_file() so it can be shared.
2014-07-24 7:09 ` Jan Beulich
@ 2014-08-06 18:38 ` Roy Franz
2014-08-07 6:20 ` Jan Beulich
0 siblings, 1 reply; 50+ messages in thread
From: Roy Franz @ 2014-08-06 18:38 UTC (permalink / raw)
To: Jan Beulich
Cc: keir, Ian Campbell, tim, xen-devel, Stefano Stabellini,
linaro-uefi, Fu Wei
On Thu, Jul 24, 2014 at 12:09 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 22.07.14 at 02:43, <roy.franz@linaro.org> wrote:
>> 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. read_file() and load_file() return
>> error codes rather than directly exiting on error to facilicate
>> sharing. Different architectures may require different max allocation
>> addresses so take that as an argument.
>
> Unless you expect an architecture to pass in different values on
> different invocations this clearly can be a #define rather than a
> function parameter.
I'll remove the argument - Ian's module freeing patch removes the need
for this.
>
>> @@ -405,12 +399,21 @@ static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>>
>> if ( what )
>> {
>> - PrintErr(what);
>> - PrintErr(L" failed for ");
>> - PrintErrMesgExit(name, ret);
>> + PrintErrMesg(what, ret);
>> + PrintErr(L"Unable to load file");
>
> Is it intentional to make the message less useful by dripping the
> printing of the file name?
No, I have fixed this.
>
>> + return 0;
>> + }
>> + else
>
> No need for an "else" after an unconditional "return" in the "if"
> branch.
>
>> @@ -470,6 +473,21 @@ static char *__init get_value(const struct file *cfg, const char *section,
>> }
>> return NULL;
>> }
>> +/* Only call with non-config files. */
>
> Missing blank line before this comment.
>
>> +bool_t __init load_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>> + struct file *file)
>> +{
>> + EFI_PHYSICAL_ADDRESS max = min(1UL << (32 + PAGE_SHIFT),
>> + HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
>> + if ( read_file(dir_handle, name, file, max) )
>
> Missing blank line between declaration and first statement.
Fixed this and above issues
>
>> @@ -896,16 +921,20 @@ 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_ok = load_file(dir_handle, s2w(&name), &ucode);
>> efi_bs->FreePool(name.w);
>> + if ( !load_ok )
>> + blexit(L"Unable to load ucode image.");
>> }
>>
>> name.s = get_value(&cfg, section.s, "xsm");
>> if ( name.s )
>> {
>> split_value(name.s);
>> - read_file(dir_handle, s2w(&name), &xsm);
>> + load_ok = load_file(dir_handle, s2w(&name), &xsm);
>> efi_bs->FreePool(name.w);
>> + if ( !load_ok )
>> + blexit(L"Unable to load ucode image.");
>
> Apart from the same comment made on an earlier patch - no need
> for an extra message here when the called function already printed
> one - this is a copy'n'paste mistake: You mean XSM instead of ucode
> here.
Fixed, and I'll review redundant messages.
>
> Jan
>
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH V2 04/12] Refactor read_file() so it can be shared.
2014-08-06 18:38 ` Roy Franz
@ 2014-08-07 6:20 ` Jan Beulich
2014-08-07 17:26 ` Roy Franz
0 siblings, 1 reply; 50+ messages in thread
From: Jan Beulich @ 2014-08-07 6:20 UTC (permalink / raw)
To: Roy Franz
Cc: keir, Ian Campbell, tim, xen-devel, Stefano Stabellini,
linaro-uefi, Fu Wei
>>> On 06.08.14 at 20:38, <roy.franz@linaro.org> wrote:
> On Thu, Jul 24, 2014 at 12:09 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 22.07.14 at 02:43, <roy.franz@linaro.org> wrote:
>>> 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. read_file() and load_file() return
>>> error codes rather than directly exiting on error to facilicate
>>> sharing. Different architectures may require different max allocation
>>> addresses so take that as an argument.
>>
>> Unless you expect an architecture to pass in different values on
>> different invocations this clearly can be a #define rather than a
>> function parameter.
> I'll remove the argument - Ian's module freeing patch removes the need
> for this.
Actually, didn't I see a later patch actually making use of this now
being an argument (passing a different value than the constant one
used in this patch)?
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH V2 04/12] Refactor read_file() so it can be shared.
2014-08-07 6:20 ` Jan Beulich
@ 2014-08-07 17:26 ` Roy Franz
0 siblings, 0 replies; 50+ messages in thread
From: Roy Franz @ 2014-08-07 17:26 UTC (permalink / raw)
To: Jan Beulich
Cc: keir, Ian Campbell, tim, xen-devel, Stefano Stabellini,
linaro-uefi, Fu Wei
On Wed, Aug 6, 2014 at 11:20 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 06.08.14 at 20:38, <roy.franz@linaro.org> wrote:
>> On Thu, Jul 24, 2014 at 12:09 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 22.07.14 at 02:43, <roy.franz@linaro.org> wrote:
>>>> 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. read_file() and load_file() return
>>>> error codes rather than directly exiting on error to facilicate
>>>> sharing. Different architectures may require different max allocation
>>>> addresses so take that as an argument.
>>>
>>> Unless you expect an architecture to pass in different values on
>>> different invocations this clearly can be a #define rather than a
>>> function parameter.
>> I'll remove the argument - Ian's module freeing patch removes the need
>> for this.
>
> Actually, didn't I see a later patch actually making use of this now
> being an argument (passing a different value than the constant one
> used in this patch)?
>
> Jan
>
There wasn't - I was using a constant for now, as determining the real max value
would have been tricky and fragile, so I punted on that, as I figured
there would be
a better way to resolve this issue. Ian's fix for this problem is the
right one,
so this is all going away.
Roy
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH V2 05/12] replace split_value() with truncate_string()
2014-07-22 0:43 [PATCH V2 00/12] arm64 EFI stub Roy Franz
` (3 preceding siblings ...)
2014-07-22 0:43 ` [PATCH V2 04/12] Refactor read_file() so it can be shared Roy Franz
@ 2014-07-22 0:43 ` Roy Franz
2014-07-24 7:19 ` Jan Beulich
2014-07-22 0:43 ` [PATCH V2 06/12] add read_config_file() function for XEN EFI config file Roy Franz
` (7 subsequent siblings)
12 siblings, 1 reply; 50+ messages in thread
From: Roy Franz @ 2014-07-22 0:43 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 | 35 +++++++++++++++++++++++++----------
1 file changed, 25 insertions(+), 10 deletions(-)
diff --git a/xen/arch/x86/efi/boot.c b/xen/arch/x86/efi/boot.c
index 17aaa67..546ff1c 100644
--- a/xen/arch/x86/efi/boot.c
+++ b/xen/arch/x86/efi/boot.c
@@ -466,7 +466,13 @@ static 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);
@@ -489,14 +495,19 @@ bool_t __init load_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
return 0;
}
-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)
{
- while ( *s && isspace(*s) )
- ++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)
@@ -893,7 +904,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_ok = load_file(dir_handle, s2w(&name), &kernel);
efi_bs->FreePool(name.w);
if ( !load_ok )
@@ -907,7 +919,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_ok = load_file(dir_handle, s2w(&name), &ramdisk);
efi_bs->FreePool(name.w);
if ( !load_ok )
@@ -920,7 +933,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_ok = load_file(dir_handle, s2w(&name), &ucode);
efi_bs->FreePool(name.w);
if ( !load_ok )
@@ -930,7 +944,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_ok = load_file(dir_handle, s2w(&name), &xsm);
efi_bs->FreePool(name.w);
if ( !load_ok )
--
2.0.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH V2 05/12] replace split_value() with truncate_string()
2014-07-22 0:43 ` [PATCH V2 05/12] replace split_value() with truncate_string() Roy Franz
@ 2014-07-24 7:19 ` Jan Beulich
2014-08-06 22:37 ` Roy Franz
0 siblings, 1 reply; 50+ messages in thread
From: Jan Beulich @ 2014-07-24 7:19 UTC (permalink / raw)
To: Roy Franz
Cc: keir, ian.campbell, tim, xen-devel, stefano.stabellini,
linaro-uefi, fu.wei
>>> On 22.07.14 at 02:43, <roy.franz@linaro.org> wrote:
> --- a/xen/arch/x86/efi/boot.c
> +++ b/xen/arch/x86/efi/boot.c
> @@ -466,7 +466,13 @@ static 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 */
Coding style.
> + while ( *ptr && isspace(*ptr) )
> + ptr++;
> + return ptr;
> + }
It's unclear how this whole hunk is related to the patch subject.
> @@ -489,14 +495,19 @@ bool_t __init load_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
> return 0;
> }
>
> -static void __init split_value(char *s)
> +/* Truncate string at first space, and return pointer
> + * to remainder of string.
> + */
Coding style again.
> +char * __init truncate_string(char *s)
Non-static function without declaration in any header.
> {
> - while ( *s && isspace(*s) )
> - ++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);
None of the callers uses the return value - why is the function return
type not "void"? Also, if there is a reason, then no parentheses around
the return expression please.
> @@ -893,7 +904,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_ok = load_file(dir_handle, s2w(&name), &kernel);
> efi_bs->FreePool(name.w);
> if ( !load_ok )
> @@ -907,7 +919,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_ok = load_file(dir_handle, s2w(&name), &ramdisk);
> efi_bs->FreePool(name.w);
> if ( !load_ok )
> @@ -920,7 +933,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_ok = load_file(dir_handle, s2w(&name), &ucode);
> efi_bs->FreePool(name.w);
> if ( !load_ok )
> @@ -930,7 +944,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_ok = load_file(dir_handle, s2w(&name), &xsm);
> efi_bs->FreePool(name.w);
> if ( !load_ok )
Looking at all these I wonder why you didn't retain split_value() as a
simple wrapper.
Furthermore splitting out the place_string() doesn't seem very
efficient, as imo the goal ought to be for efi_start() to become
common code (or at least the module loading part of fit), i.e.
there's no win at all from the change you're doing here.
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH V2 05/12] replace split_value() with truncate_string()
2014-07-24 7:19 ` Jan Beulich
@ 2014-08-06 22:37 ` Roy Franz
2014-08-07 6:24 ` Jan Beulich
0 siblings, 1 reply; 50+ messages in thread
From: Roy Franz @ 2014-08-06 22:37 UTC (permalink / raw)
To: Jan Beulich
Cc: keir, Ian Campbell, tim, xen-devel, Stefano Stabellini,
linaro-uefi, Fu Wei
On Thu, Jul 24, 2014 at 12:19 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 22.07.14 at 02:43, <roy.franz@linaro.org> wrote:
>> --- a/xen/arch/x86/efi/boot.c
>> +++ b/xen/arch/x86/efi/boot.c
>> @@ -466,7 +466,13 @@ static 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 */
>
> Coding style.
>
>> + while ( *ptr && isspace(*ptr) )
>> + ptr++;
>> + return ptr;
>> + }
>
> It's unclear how this whole hunk is related to the patch subject.
>
>> @@ -489,14 +495,19 @@ bool_t __init load_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>> return 0;
>> }
>>
>> -static void __init split_value(char *s)
>> +/* Truncate string at first space, and return pointer
>> + * to remainder of string.
>> + */
>
> Coding style again.
>
>> +char * __init truncate_string(char *s)
>
> Non-static function without declaration in any header.
>
>> {
>> - while ( *s && isspace(*s) )
>> - ++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);
>
> None of the callers uses the return value - why is the function return
> type not "void"? Also, if there is a reason, then no parentheses around
> the return expression please.
I am making these changes to make the basic functionality share-able
with the arm64
stub. The users of this function's return value come in when the arm
stub is added.
The declaration is added to efi-shared.h when it is moved - I can make
it static here until then.
I very deliberately am separating code refactoring from code movement
patches which causes
the separation between the creation of a function with a return value
that is not currently used.
>
>> @@ -893,7 +904,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_ok = load_file(dir_handle, s2w(&name), &kernel);
>> efi_bs->FreePool(name.w);
>> if ( !load_ok )
>> @@ -907,7 +919,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_ok = load_file(dir_handle, s2w(&name), &ramdisk);
>> efi_bs->FreePool(name.w);
>> if ( !load_ok )
>> @@ -920,7 +933,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_ok = load_file(dir_handle, s2w(&name), &ucode);
>> efi_bs->FreePool(name.w);
>> if ( !load_ok )
>> @@ -930,7 +944,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_ok = load_file(dir_handle, s2w(&name), &xsm);
>> efi_bs->FreePool(name.w);
>> if ( !load_ok )
>
> Looking at all these I wonder why you didn't retain split_value() as a
> simple wrapper.
I should be able to do that.
>
> Furthermore splitting out the place_string() doesn't seem very
> efficient, as imo the goal ought to be for efi_start() to become
> common code (or at least the module loading part of fit), i.e.
> there's no win at all from the change you're doing here.
I don't think that combining the x86 and arm efi_start() will work out
that cleanly. Arm is using device tree from getting information from
GRUB and/or the firmware, so I think you'd end up with a lot of conditional
code.
>
> Jan
>
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH V2 05/12] replace split_value() with truncate_string()
2014-08-06 22:37 ` Roy Franz
@ 2014-08-07 6:24 ` Jan Beulich
2014-08-18 23:38 ` Roy Franz
0 siblings, 1 reply; 50+ messages in thread
From: Jan Beulich @ 2014-08-07 6:24 UTC (permalink / raw)
To: Roy Franz
Cc: keir, Ian Campbell, tim, xen-devel, Stefano Stabellini,
linaro-uefi, Fu Wei
>>> On 07.08.14 at 00:37, <roy.franz@linaro.org> wrote:
> On Thu, Jul 24, 2014 at 12:19 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> Furthermore splitting out the place_string() doesn't seem very
>> efficient, as imo the goal ought to be for efi_start() to become
>> common code (or at least the module loading part of fit), i.e.
>> there's no win at all from the change you're doing here.
>
> I don't think that combining the x86 and arm efi_start() will work out
> that cleanly. Arm is using device tree from getting information from
> GRUB and/or the firmware, so I think you'd end up with a lot of conditional
> code.
But that is precisely what you'd add (arch-specific) calls out of the
function for, in the extreme case doing nothing on x86. (And that is
also specifically why I'd favor the approach outlined in the earlier
reply to patch 1.)
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH V2 05/12] replace split_value() with truncate_string()
2014-08-07 6:24 ` Jan Beulich
@ 2014-08-18 23:38 ` Roy Franz
2014-08-19 12:56 ` Jan Beulich
0 siblings, 1 reply; 50+ messages in thread
From: Roy Franz @ 2014-08-18 23:38 UTC (permalink / raw)
To: Jan Beulich
Cc: keir, Ian Campbell, tim, xen-devel, Stefano Stabellini,
linaro-uefi, Fu Wei
On Wed, Aug 6, 2014 at 11:24 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 07.08.14 at 00:37, <roy.franz@linaro.org> wrote:
>> On Thu, Jul 24, 2014 at 12:19 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>> Furthermore splitting out the place_string() doesn't seem very
>>> efficient, as imo the goal ought to be for efi_start() to become
>>> common code (or at least the module loading part of fit), i.e.
>>> there's no win at all from the change you're doing here.
>>
>> I don't think that combining the x86 and arm efi_start() will work out
>> that cleanly. Arm is using device tree from getting information from
>> GRUB and/or the firmware, so I think you'd end up with a lot of conditional
>> code.
>
> But that is precisely what you'd add (arch-specific) calls out of the
> function for, in the extreme case doing nothing on x86. (And that is
> also specifically why I'd favor the approach outlined in the earlier
> reply to patch 1.)
>
> Jan
>
Hi Jan,
I've spent a little time prototyping what you are suggesting, and I
think I should be able
to have a patchset in reasonable shape in time for the 4.5 freeze.
This will require more extensive
changes to the x86 code than my previous patchset, but most of it
appears to be benign re-ordering
of various bits of code to bring blocks of arch specific code
together. I don't have an EFI based x86 system
to test on, which makes me a bit nervous about this.
I will be at LinuxCon this week (arriving Tuesday evening, through
Sat.), and it would be great if we could find some time to
chat about this in person.
Thanks,
Roy
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH V2 05/12] replace split_value() with truncate_string()
2014-08-18 23:38 ` Roy Franz
@ 2014-08-19 12:56 ` Jan Beulich
0 siblings, 0 replies; 50+ messages in thread
From: Jan Beulich @ 2014-08-19 12:56 UTC (permalink / raw)
To: roy.franz
Cc: keir, ian.campbell, tim, xen-devel, stefano.stabellini,
linaro-uefi, fu.wei
>>> Roy Franz <roy.franz@linaro.org> 08/19/14 1:38 AM >>>
>I've spent a little time prototyping what you are suggesting, and I
>think I should be able
>to have a patchset in reasonable shape in time for the 4.5 freeze.
>This will require more extensive
>changes to the x86 code than my previous patchset, but most of it
>appears to be benign re-ordering
>of various bits of code to bring blocks of arch specific code
>together. I don't have an EFI based x86 system
>to test on, which makes me a bit nervous about this.
If (almost) all you do is code movement, and if you at least have a
build environment to test that part, I wouldn't be too nervous. Once
there is a final patch series, I could certainly see to help trying it
out before committing (just please be sure to have a small note in
there to lower the chances that I forget).
>I will be at LinuxCon this week (arriving Tuesday evening, through
>Sat.), and it would be great if we could find some time to
>chat about this in person.
Well, I'm supposedly tied up all Wednesday morning and will then
soon be heading to the airport. So the only reasonable chance would
be right after the dev meeting, which is expected to end around 1pm.
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH V2 06/12] add read_config_file() function for XEN EFI config file
2014-07-22 0:43 [PATCH V2 00/12] arm64 EFI stub Roy Franz
` (4 preceding siblings ...)
2014-07-22 0:43 ` [PATCH V2 05/12] replace split_value() with truncate_string() Roy Franz
@ 2014-07-22 0:43 ` Roy Franz
2014-07-24 7:32 ` Jan Beulich
2014-07-22 0:43 ` [PATCH V2 07/12] create handle_cmdline() function Roy Franz
` (6 subsequent siblings)
12 siblings, 1 reply; 50+ messages in thread
From: Roy Franz @ 2014-07-22 0:43 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 | 116 ++++++++++++++++++++++++++++--------------------
1 file changed, 68 insertions(+), 48 deletions(-)
diff --git a/xen/arch/x86/efi/boot.c b/xen/arch/x86/efi/boot.c
index 546ff1c..fa6aca4 100644
--- a/xen/arch/x86/efi/boot.c
+++ b/xen/arch/x86/efi/boot.c
@@ -510,6 +510,70 @@ char * __init truncate_string(char *s)
return(NULL);
}
+bool_t __init read_config_file(EFI_FILE_HANDLE *cfg_dir_handle,
+ struct file *cfg, CHAR16 *cfg_file_name,
+ union string *section,
+ CHAR16 *xen_file_name)
+{
+ /*
+ * This allocation is internal to the EFI stub, so any address is
+ * fine.
+ */
+ EFI_PHYSICAL_ADDRESS max = ~0;
+
+ /* 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, max) )
+ break;
+ *tail = 0;
+ }
+ if ( !tail )
+ return 0;
+ PrintStr(L"Using configuration file '");
+ PrintStr(xen_file_name);
+ PrintStr(L"'\r\n");
+ }
+ else if ( !read_file(*cfg_dir_handle, cfg_file_name, cfg, max) )
+ return 0;
+ pre_parse(cfg);
+
+ if ( section->w )
+ w2s(section);
+ else
+ section->s = get_value(cfg, "global", "default");
+
+
+ for ( ; ; )
+ {
+ union string dom0_kernel_name;
+ 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, max) )
+ {
+ PrintStr(L"Chained configuration file '");
+ PrintStr(dom0_kernel_name.w);
+ efi_bs->FreePool(dom0_kernel_name.w);
+ PrintStr(L"'not found.");
+ return 0;
+ }
+ pre_parse(cfg);
+ efi_bs->FreePool(dom0_kernel_name.w);
+ }
+ return 1;
+}
+
static void __init edd_put_string(u8 *dst, size_t n, const char *src)
{
while ( n-- && *src )
@@ -722,8 +786,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
u64 efer;
bool_t base_video = 0;
bool_t load_ok = 0;
- EFI_PHYSICAL_ADDRESS max_addr = min(1UL << (32 + PAGE_SHIFT),
- HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
efi_ih = ImageHandle;
efi_bs = SystemTable->BootServices;
@@ -855,53 +917,11 @@ 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;
+ if ( !read_config_file(&dir_handle, &cfg, cfg_file_name, §ion,
+ file_name) )
+ blexit(L"Unable to read configuration file.");
- while ( (tail = point_tail(file_name)) != NULL )
- {
- wstrcpy(tail, L".cfg");
- if ( read_file(dir_handle, file_name, &cfg, max_addr) )
- 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, max_addr) )
- blexit(L"Configuration file not found.");
- pre_parse(&cfg);
-
- if ( section.w )
- w2s(§ion);
- 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, max_addr) )
- {
- 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);
--
2.0.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH V2 06/12] add read_config_file() function for XEN EFI config file
2014-07-22 0:43 ` [PATCH V2 06/12] add read_config_file() function for XEN EFI config file Roy Franz
@ 2014-07-24 7:32 ` Jan Beulich
2014-08-06 22:42 ` Roy Franz
0 siblings, 1 reply; 50+ messages in thread
From: Jan Beulich @ 2014-07-24 7:32 UTC (permalink / raw)
To: Roy Franz
Cc: keir, ian.campbell, tim, xen-devel, stefano.stabellini,
linaro-uefi, fu.wei
>>> On 22.07.14 at 02:43, <roy.franz@linaro.org> wrote:
> Move open-coded reading of the XEN EFI configuration file into a shared
> fuction read_config_file().
If the function is shared, why is it being placed in the x86 file instead
of the shared one (with again no declaration added to the shared
header)?
> +bool_t __init read_config_file(EFI_FILE_HANDLE *cfg_dir_handle,
> + struct file *cfg, CHAR16 *cfg_file_name,
> + union string *section,
> + CHAR16 *xen_file_name)
> +{
> + /*
> + * This allocation is internal to the EFI stub, so any address is
> + * fine.
> + */
> + EFI_PHYSICAL_ADDRESS max = ~0;
Ah, okay, here comes the answer to the question I asked in an
earlier patch. However, using AllocateMaxAddress with an
unlimited address seems kind of bogus. I'd prefer this to be done
cleanly by passing a boolean into the function and having that one
use AllocateMaxAddress or AllocateAnyPages.
> +
> + /* 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, max) )
> + break;
> + *tail = 0;
> + }
> + if ( !tail )
> + return 0;
> + PrintStr(L"Using configuration file '");
> + PrintStr(xen_file_name);
> + PrintStr(L"'\r\n");
> + }
> + else if ( !read_file(*cfg_dir_handle, cfg_file_name, cfg, max) )
> + return 0;
> + pre_parse(cfg);
> +
> + if ( section->w )
> + w2s(section);
> + else
> + section->s = get_value(cfg, "global", "default");
> +
> +
> + for ( ; ; )
> + {
> + union string dom0_kernel_name;
> + 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");
Please name the variable differently if it is used for other than the
purpose its current name implies.
Also there are again blank line issues above - I'm not going to
repeat respective comments made in an earlier patch, implying
that you'll take care of these issues throughout the series.
> @@ -855,53 +917,11 @@ 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;
> + if ( !read_config_file(&dir_handle, &cfg, cfg_file_name, §ion,
> + file_name) )
> + blexit(L"Unable to read configuration file.");
>
> - while ( (tail = point_tail(file_name)) != NULL )
> - {
> - wstrcpy(tail, L".cfg");
> - if ( read_file(dir_handle, file_name, &cfg, max_addr) )
> - 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, max_addr) )
> - blexit(L"Configuration file not found.");
> - pre_parse(&cfg);
> -
> - if ( section.w )
> - w2s(§ion);
> - 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, max_addr) )
> - {
> - 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.");
This redundant lookup can be avoided by having the function return
not just a bool_t.
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH V2 06/12] add read_config_file() function for XEN EFI config file
2014-07-24 7:32 ` Jan Beulich
@ 2014-08-06 22:42 ` Roy Franz
0 siblings, 0 replies; 50+ messages in thread
From: Roy Franz @ 2014-08-06 22:42 UTC (permalink / raw)
To: Jan Beulich
Cc: keir, Ian Campbell, tim, xen-devel, Stefano Stabellini,
linaro-uefi, Fu Wei
On Thu, Jul 24, 2014 at 12:32 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 22.07.14 at 02:43, <roy.franz@linaro.org> wrote:
>> Move open-coded reading of the XEN EFI configuration file into a shared
>> fuction read_config_file().
>
> If the function is shared, why is it being placed in the x86 file instead
> of the shared one (with again no declaration added to the shared
> header)?
>
>> +bool_t __init read_config_file(EFI_FILE_HANDLE *cfg_dir_handle,
>> + struct file *cfg, CHAR16 *cfg_file_name,
>> + union string *section,
>> + CHAR16 *xen_file_name)
>> +{
>> + /*
>> + * This allocation is internal to the EFI stub, so any address is
>> + * fine.
>> + */
>> + EFI_PHYSICAL_ADDRESS max = ~0;
>
> Ah, okay, here comes the answer to the question I asked in an
> earlier patch. However, using AllocateMaxAddress with an
> unlimited address seems kind of bogus. I'd prefer this to be done
> cleanly by passing a boolean into the function and having that one
> use AllocateMaxAddress or AllocateAnyPages.
>
This will be going away since Ian's patch resolves the problem this was
addressing.
>> +
>> + /* 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, max) )
>> + break;
>> + *tail = 0;
>> + }
>> + if ( !tail )
>> + return 0;
>> + PrintStr(L"Using configuration file '");
>> + PrintStr(xen_file_name);
>> + PrintStr(L"'\r\n");
>> + }
>> + else if ( !read_file(*cfg_dir_handle, cfg_file_name, cfg, max) )
>> + return 0;
>> + pre_parse(cfg);
>> +
>> + if ( section->w )
>> + w2s(section);
>> + else
>> + section->s = get_value(cfg, "global", "default");
>> +
>> +
>> + for ( ; ; )
>> + {
>> + union string dom0_kernel_name;
>> + 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");
>
> Please name the variable differently if it is used for other than the
> purpose its current name implies.
OK
>
> Also there are again blank line issues above - I'm not going to
> repeat respective comments made in an earlier patch, implying
> that you'll take care of these issues throughout the series.
Yes, I will review the changes again.
>
>> @@ -855,53 +917,11 @@ 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;
>> + if ( !read_config_file(&dir_handle, &cfg, cfg_file_name, §ion,
>> + file_name) )
>> + blexit(L"Unable to read configuration file.");
>>
>> - while ( (tail = point_tail(file_name)) != NULL )
>> - {
>> - wstrcpy(tail, L".cfg");
>> - if ( read_file(dir_handle, file_name, &cfg, max_addr) )
>> - 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, max_addr) )
>> - blexit(L"Configuration file not found.");
>> - pre_parse(&cfg);
>> -
>> - if ( section.w )
>> - w2s(§ion);
>> - 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, max_addr) )
>> - {
>> - 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.");
>
> This redundant lookup can be avoided by having the function return
> not just a bool_t.
Yup, I'll change this.
Roy
>
> Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH V2 07/12] create handle_cmdline() function
2014-07-22 0:43 [PATCH V2 00/12] arm64 EFI stub Roy Franz
` (5 preceding siblings ...)
2014-07-22 0:43 ` [PATCH V2 06/12] add read_config_file() function for XEN EFI config file Roy Franz
@ 2014-07-22 0:43 ` Roy Franz
2014-07-24 7:36 ` Jan Beulich
2014-07-22 0:43 ` [PATCH V2 08/12] Refactor get_argv() for sharing Roy Franz
` (5 subsequent siblings)
12 siblings, 1 reply; 50+ messages in thread
From: Roy Franz @ 2014-07-22 0:43 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 | 127 +++++++++++++++++++++++++++++-------------------
1 file changed, 77 insertions(+), 50 deletions(-)
diff --git a/xen/arch/x86/efi/boot.c b/xen/arch/x86/efi/boot.c
index fa6aca4..2ef86d1 100644
--- a/xen/arch/x86/efi/boot.c
+++ b/xen/arch/x86/efi/boot.c
@@ -744,6 +744,74 @@ static void __init relocate_image(unsigned long delta)
}
}
+
+bool_t __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");
+ return 0;
+ }
+
+ 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");
+ return 0;
+ }
+ else
+ {
+ PrintStr(L"WARNING: Unknown command line option '");
+ PrintStr(ptr);
+ PrintStr(L"' ignored\r\n");
+ }
+ }
+ else
+ *section_name = ptr;
+ }
+
+ if ( argc )
+ *image_name = *argv;
+
+ return 1;
+}
+
extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[];
extern const s32 __trampoline_seg_start[], __trampoline_seg_stop[];
@@ -773,8 +841,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 = NULL, *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;
@@ -814,53 +883,11 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
/* Get the file system interface. */
dir_handle = get_parent_handle(loaded_image, &file_name);
- if ( !dir_handle )
- blexit(L"EFI get_parent_handle failed.");
+ if ( !handle_cmdline(loaded_image, &cfg_file_name, &base_video, &image_name,
+ §ion_name) )
+ 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.w = ptr;
- }
+ section.w = section_name;
if ( !base_video )
{
@@ -976,9 +1003,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] 50+ messages in thread* Re: [PATCH V2 07/12] create handle_cmdline() function
2014-07-22 0:43 ` [PATCH V2 07/12] create handle_cmdline() function Roy Franz
@ 2014-07-24 7:36 ` Jan Beulich
2014-07-28 15:44 ` Ian Campbell
0 siblings, 1 reply; 50+ messages in thread
From: Jan Beulich @ 2014-07-24 7:36 UTC (permalink / raw)
To: Roy Franz
Cc: keir, ian.campbell, tim, xen-devel, stefano.stabellini,
linaro-uefi, fu.wei
>>> On 22.07.14 at 02:43, <roy.franz@linaro.org> wrote:
> Create handle_cmdline() function in preparation for sharing to allow x86 and
> ARM architectures to share the command line processing.
I again can't see why the function doesn't get moved to the shared
file right away. And of course the splitting out again is questionable
considering that efi_start() itself ought to ultimately become a
shared function. By now I think you should have taken this the
other way round: Move the whole xen/arch/x86/efi/ subtree to
xen/common/efi/ and _then_ split out x86 specific code (possibly
into inline functions or #define-s rather than out of line code).
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH V2 07/12] create handle_cmdline() function
2014-07-24 7:36 ` Jan Beulich
@ 2014-07-28 15:44 ` Ian Campbell
2014-07-28 15:57 ` Jan Beulich
0 siblings, 1 reply; 50+ messages in thread
From: Ian Campbell @ 2014-07-28 15:44 UTC (permalink / raw)
To: Jan Beulich
Cc: keir, tim, xen-devel, Roy Franz, stefano.stabellini, linaro-uefi, fu.wei
On Thu, 2014-07-24 at 08:36 +0100, Jan Beulich wrote:
> >>> On 22.07.14 at 02:43, <roy.franz@linaro.org> wrote:
> > Create handle_cmdline() function in preparation for sharing to allow x86 and
> > ARM architectures to share the command line processing.
>
> I again can't see why the function doesn't get moved to the shared
> file right away. And of course the splitting out again is questionable
> considering that efi_start() itself ought to ultimately become a
> shared function. By now I think you should have taken this the
> other way round: Move the whole xen/arch/x86/efi/ subtree to
> xen/common/efi/ and _then_ split out x86 specific code (possibly
> into inline functions or #define-s rather than out of line code).
Are you implying that you want to see it redone that way or just
commenting but thinking "what's done is done"?
Ian.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH V2 07/12] create handle_cmdline() function
2014-07-28 15:44 ` Ian Campbell
@ 2014-07-28 15:57 ` Jan Beulich
0 siblings, 0 replies; 50+ messages in thread
From: Jan Beulich @ 2014-07-28 15:57 UTC (permalink / raw)
To: Ian Campbell
Cc: keir, tim, xen-devel, Roy Franz, stefano.stabellini, linaro-uefi, fu.wei
>>> On 28.07.14 at 17:44, <Ian.Campbell@citrix.com> wrote:
> On Thu, 2014-07-24 at 08:36 +0100, Jan Beulich wrote:
>> >>> On 22.07.14 at 02:43, <roy.franz@linaro.org> wrote:
>> > Create handle_cmdline() function in preparation for sharing to allow x86
> and
>> > ARM architectures to share the command line processing.
>>
>> I again can't see why the function doesn't get moved to the shared
>> file right away. And of course the splitting out again is questionable
>> considering that efi_start() itself ought to ultimately become a
>> shared function. By now I think you should have taken this the
>> other way round: Move the whole xen/arch/x86/efi/ subtree to
>> xen/common/efi/ and _then_ split out x86 specific code (possibly
>> into inline functions or #define-s rather than out of line code).
>
> Are you implying that you want to see it redone that way or just
> commenting but thinking "what's done is done"?
I guess I can live with it remaining the way it's done now, but I'd
much prefer it to be re-done as outlined unless there's a clear
indication against doing so. The extra work this incurs is certainly a
result of starting the coding without first announcing the plan (I
knew the split up would be wanted at some point, but I wasn't aware
that it got already started by the time I saw the first version of the
series, which then also was - I think - quite different from the v2
we're looking at now; had I known, I might have offered seeing to
do this myself).
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH V2 08/12] Refactor get_argv() for sharing
2014-07-22 0:43 [PATCH V2 00/12] arm64 EFI stub Roy Franz
` (6 preceding siblings ...)
2014-07-22 0:43 ` [PATCH V2 07/12] create handle_cmdline() function Roy Franz
@ 2014-07-22 0:43 ` Roy Franz
2014-07-24 7:38 ` Jan Beulich
2014-07-22 0:43 ` [PATCH V2 09/12] Move shared EFI functions to efi-shared.c Roy Franz
` (4 subsequent siblings)
12 siblings, 1 reply; 50+ messages in thread
From: Roy Franz @ 2014-07-22 0:43 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 | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/xen/arch/x86/efi/boot.c b/xen/arch/x86/efi/boot.c
index 2ef86d1..edbdb8a 100644
--- a/xen/arch/x86/efi/boot.c
+++ b/xen/arch/x86/efi/boot.c
@@ -175,7 +175,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;
@@ -201,10 +202,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
@@ -747,7 +747,8 @@ static void __init relocate_image(unsigned long delta)
bool_t __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;
@@ -761,14 +762,14 @@ bool_t __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;
@@ -844,6 +845,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
unsigned int i;
CHAR16 *file_name = NULL, *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;
@@ -884,9 +886,12 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
dir_handle = get_parent_handle(loaded_image, &file_name);
if ( !handle_cmdline(loaded_image, &cfg_file_name, &base_video, &image_name,
- §ion_name) )
+ §ion_name, &cmdline.w) )
blexit(NULL);
+ if (cmdline.w)
+ place_string(&mbi.cmdline, w2s(&cmdline));
+
section.w = section_name;
if ( !base_video )
--
2.0.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH V2 08/12] Refactor get_argv() for sharing
2014-07-22 0:43 ` [PATCH V2 08/12] Refactor get_argv() for sharing Roy Franz
@ 2014-07-24 7:38 ` Jan Beulich
0 siblings, 0 replies; 50+ messages in thread
From: Jan Beulich @ 2014-07-24 7:38 UTC (permalink / raw)
To: Roy Franz
Cc: keir, ian.campbell, tim, xen-devel, stefano.stabellini,
linaro-uefi, fu.wei
>>> On 22.07.14 at 02:43, <roy.franz@linaro.org> wrote:
> @@ -201,10 +202,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)
Coding style (also further down). Looks okay apart from that.
Jan
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH V2 09/12] Move shared EFI functions to efi-shared.c
2014-07-22 0:43 [PATCH V2 00/12] arm64 EFI stub Roy Franz
` (7 preceding siblings ...)
2014-07-22 0:43 ` [PATCH V2 08/12] Refactor get_argv() for sharing Roy Franz
@ 2014-07-22 0:43 ` Roy Franz
2014-07-22 0:43 ` [PATCH V2 10/12] add arm64 cache flushing code from linux Roy Franz
` (3 subsequent siblings)
12 siblings, 0 replies; 50+ messages in thread
From: Roy Franz @ 2014-07-22 0:43 UTC (permalink / raw)
To: xen-devel, ian.campbell, stefano.stabellini, tim, jbeulich, keir
Cc: Roy Franz, fu.wei, linaro-uefi
Move all of the shareable functions from boot.c to the the shared EFI
code. These functions will be used by the arm64 EFI stub.
Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
xen/arch/x86/efi/boot.c | 501 ------------------------------------------
xen/common/efi/efi-shared.c | 506 +++++++++++++++++++++++++++++++++++++++++++
xen/include/efi/efi-shared.h | 22 ++
3 files changed, 528 insertions(+), 501 deletions(-)
diff --git a/xen/arch/x86/efi/boot.c b/xen/arch/x86/efi/boot.c
index edbdb8a..348e237 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>
@@ -91,55 +90,6 @@ static void __init noreturn blexit(const CHAR16 *str)
unreachable(); /* not reached */
}
-/* 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;
- }
-}
/* generic routine for printing error messages */
static void __init PrintErrMesgExit(const CHAR16 *mesg, EFI_STATUS ErrCode)
@@ -174,311 +124,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;
-}
-
-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);
- return NULL;
- }
- ret = fio->OpenVolume(fio, &dir_handle);
- } while ( ret == EFI_MEDIA_CHANGED );
- if ( ret != EFI_SUCCESS )
- {
- PrintErrMesg(L"OpenVolume failure", ret);
- return NULL;
- }
-
-#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 )
- {
- PrintErr(L"Unsupported device path component");
- return NULL;
- }
-
- 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);
- return NULL;
- }
- dir_handle->Close(dir_handle);
- dir_handle = new_handle;
- }
- fp = (void *)dp;
- if ( BUFFERSIZE < DevicePathNodeLength(dp) -
- sizeof(*dp) + sizeof(*buffer) )
- {
- PrintErr(L"Increase BUFFERSIZE");
- return NULL;
- }
- 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);
- return NULL;
- }
- 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, EFI_PHYSICAL_ADDRESS max_addr)
-{
- EFI_FILE_HANDLE FileHandle = NULL;
- UINT64 size;
- EFI_STATUS ret;
- CHAR16 *what = NULL;
-
- if ( !name )
- {
- PrintErrMesg(L"No Filename", EFI_OUT_OF_RESOURCES);
- return 0;
- }
-
- 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 = max_addr;
- 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);
- PrintErr(L"Unable to load file");
- return 0;
- }
- 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;
- }
-
-}
-
-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] == '=' )
- {
- ptr += ilen + 1;
- /* strip off any leading spaces */
- while ( *ptr && isspace(*ptr) )
- ptr++;
- return ptr;
- }
- break;
- }
- ptr += strlen(ptr);
- }
- return NULL;
-}
/* Only call with non-config files. */
bool_t __init load_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
struct file *file)
@@ -495,85 +140,6 @@ bool_t __init load_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
return 0;
}
-/* 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);
-}
-
-bool_t __init read_config_file(EFI_FILE_HANDLE *cfg_dir_handle,
- struct file *cfg, CHAR16 *cfg_file_name,
- union string *section,
- CHAR16 *xen_file_name)
-{
- /*
- * This allocation is internal to the EFI stub, so any address is
- * fine.
- */
- EFI_PHYSICAL_ADDRESS max = ~0;
-
- /* 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, max) )
- break;
- *tail = 0;
- }
- if ( !tail )
- return 0;
- PrintStr(L"Using configuration file '");
- PrintStr(xen_file_name);
- PrintStr(L"'\r\n");
- }
- else if ( !read_file(*cfg_dir_handle, cfg_file_name, cfg, max) )
- return 0;
- pre_parse(cfg);
-
- if ( section->w )
- w2s(section);
- else
- section->s = get_value(cfg, "global", "default");
-
-
- for ( ; ; )
- {
- union string dom0_kernel_name;
- 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, max) )
- {
- PrintStr(L"Chained configuration file '");
- PrintStr(dom0_kernel_name.w);
- efi_bs->FreePool(dom0_kernel_name.w);
- PrintStr(L"'not found.");
- return 0;
- }
- pre_parse(cfg);
- efi_bs->FreePool(dom0_kernel_name.w);
- }
- return 1;
-}
-
static void __init edd_put_string(u8 *dst, size_t n, const char *src)
{
while ( n-- && *src )
@@ -745,73 +311,6 @@ static void __init relocate_image(unsigned long delta)
}
-bool_t __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");
- return 0;
- }
-
- 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");
- return 0;
- }
- else
- {
- PrintStr(L"WARNING: Unknown command line option '");
- PrintStr(ptr);
- PrintStr(L"' ignored\r\n");
- }
- }
- else
- *section_name = ptr;
- }
-
- if ( argc )
- *image_name = *argv;
-
- return 1;
-}
extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[];
extern const s32 __trampoline_seg_start[], __trampoline_seg_stop[];
diff --git a/xen/common/efi/efi-shared.c b/xen/common/efi/efi-shared.c
index b990292..835121d 100644
--- a/xen/common/efi/efi-shared.c
+++ b/xen/common/efi/efi-shared.c
@@ -19,6 +19,11 @@
#include <xen/ctype.h>
#include <xen/init.h>
#include <asm/processor.h>
+#include <xen/keyhandler.h>
+#include <xen/pfn.h>
+#if EFI_PAGE_SIZE != PAGE_SIZE
+# error Cannot use xen/pfn.h here!
+#endif
SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdOut;
@@ -131,6 +136,507 @@ 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)
+{
+ 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;
+ }
+}
+
+
+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);
+ return NULL;
+ }
+ ret = fio->OpenVolume(fio, &dir_handle);
+ } while ( ret == EFI_MEDIA_CHANGED );
+ if ( ret != EFI_SUCCESS )
+ {
+ PrintErrMesg(L"OpenVolume failure", ret);
+ return NULL;
+ }
+
+#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 )
+ {
+ PrintErr(L"Unsupported device path component");
+ return NULL;
+ }
+
+ 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);
+ return NULL;
+ }
+ dir_handle->Close(dir_handle);
+ dir_handle = new_handle;
+ }
+ fp = (void *)dp;
+ if ( BUFFERSIZE < DevicePathNodeLength(dp) -
+ sizeof(*dp) + sizeof(*buffer) )
+ {
+ PrintErr(L"Increase BUFFERSIZE");
+ return NULL;
+ }
+ 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);
+ return NULL;
+ }
+ 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;
+ }
+}
+
+bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
+ struct file *file, EFI_PHYSICAL_ADDRESS max_addr)
+{
+ EFI_FILE_HANDLE FileHandle = NULL;
+ UINT64 size;
+ EFI_STATUS ret;
+ CHAR16 *what = NULL;
+
+ if ( !name )
+ {
+ PrintErrMesg(L"No Filename", EFI_OUT_OF_RESOURCES);
+ return 0;
+ }
+
+ 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 = max_addr;
+ 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);
+ PrintErr(L"Unable to load file");
+ return 0;
+ }
+ 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;
+ }
+
+}
+
+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] == '=' )
+ {
+ ptr += ilen + 1;
+ /* strip off any leading spaces */
+ while ( *ptr && isspace(*ptr) )
+ ptr++;
+ return ptr;
+ }
+ break;
+ }
+ ptr += strlen(ptr);
+ }
+ return NULL;
+}
+
+/* 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);
+}
+
+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;
+}
+
+bool_t __init read_config_file(EFI_FILE_HANDLE *cfg_dir_handle,
+ struct file *cfg, CHAR16 *cfg_file_name,
+ union string *section,
+ CHAR16 *xen_file_name)
+{
+ /*
+ * This allocation is internal to the EFI stub, so any address is
+ * fine.
+ */
+ EFI_PHYSICAL_ADDRESS max = ~0;
+
+ /* 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, max) )
+ break;
+ *tail = 0;
+ }
+ if ( !tail )
+ return 0;
+ PrintStr(L"Using configuration file '");
+ PrintStr(xen_file_name);
+ PrintStr(L"'\r\n");
+ }
+ else if ( !read_file(*cfg_dir_handle, cfg_file_name, cfg, max) )
+ return 0;
+ pre_parse(cfg);
+
+ if ( section->w )
+ w2s(section);
+ else
+ section->s = get_value(cfg, "global", "default");
+
+
+ for ( ; ; )
+ {
+ union string dom0_kernel_name;
+ 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, max) )
+ {
+ PrintStr(L"Chained configuration file '");
+ PrintStr(dom0_kernel_name.w);
+ efi_bs->FreePool(dom0_kernel_name.w);
+ PrintStr(L"'not found.");
+ return 0;
+ }
+ pre_parse(cfg);
+ efi_bs->FreePool(dom0_kernel_name.w);
+ }
+ return 1;
+}
+bool_t __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");
+ return 0;
+ }
+
+ 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");
+ return 0;
+ }
+ else
+ {
+ PrintStr(L"WARNING: Unknown command line option '");
+ PrintStr(ptr);
+ PrintStr(L"' ignored\r\n");
+ }
+ }
+ else
+ *section_name = ptr;
+ }
+
+ if ( argc )
+ *image_name = *argv;
+
+ return 1;
+}
/*
* Local variables:
* mode: C
diff --git a/xen/include/efi/efi-shared.h b/xen/include/efi/efi-shared.h
index 38f8f39..e559bca 100644
--- a/xen/include/efi/efi-shared.h
+++ b/xen/include/efi/efi-shared.h
@@ -36,6 +36,28 @@ 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);
+bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
+ struct file *file, EFI_PHYSICAL_ADDRESS max_addr);
+void __init pre_parse(const struct file *cfg);
+char *__init get_value(const struct file *cfg, const char *section,
+ const char *item);
+
+char * __init truncate_string(char *s);
+
+bool_t __init read_config_file(EFI_FILE_HANDLE *cfg_dir_handle,
+ struct file *cfg, CHAR16 *cfg_file_name,
+ union string *section,
+ CHAR16 *xen_file_name);
+unsigned int __init get_argv(unsigned int argc, CHAR16 **argv, CHAR16 *cmdline,
+ UINTN cmdsize, CHAR16 **cmdline_remain);
+bool_t __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] 50+ messages in thread* [PATCH V2 10/12] add arm64 cache flushing code from linux
2014-07-22 0:43 [PATCH V2 00/12] arm64 EFI stub Roy Franz
` (8 preceding siblings ...)
2014-07-22 0:43 ` [PATCH V2 09/12] Move shared EFI functions to efi-shared.c Roy Franz
@ 2014-07-22 0:43 ` Roy Franz
2014-07-28 15:53 ` Ian Campbell
2014-07-22 0:43 ` [PATCH V2 11/12] Add fdt_create_empty_tree() function Roy Franz
` (2 subsequent siblings)
12 siblings, 1 reply; 50+ messages in thread
From: Roy Franz @ 2014-07-22 0:43 UTC (permalink / raw)
To: xen-devel, ian.campbell, stefano.stabellini, tim, jbeulich, keir
Cc: Roy Franz, fu.wei, linaro-uefi
__flush_dcache_all added from arch/arm64/mm/cache.S, with helper
macros from arch/arm64/include/asm/assembler.h, both from
v3.16-rc6. The cache flushing is required when transitioning
from EFI code that runs with cache enable to XEN startup code
which expects the cache to be disabled.
Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
xen/arch/arm/arm64/Makefile | 1 +
xen/arch/arm/arm64/cache.S | 100 ++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 101 insertions(+)
create mode 100644 xen/arch/arm/arm64/cache.S
diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile
index d2d5875..c7243f5 100644
--- a/xen/arch/arm/arm64/Makefile
+++ b/xen/arch/arm/arm64/Makefile
@@ -7,5 +7,6 @@ obj-y += domain.o
obj-y += vfp.o
obj-y += smpboot.o
obj-y += domctl.o
+obj-y += cache.o
obj-$(EARLY_PRINTK) += debug.o
diff --git a/xen/arch/arm/arm64/cache.S b/xen/arch/arm/arm64/cache.S
new file mode 100644
index 0000000..2c19adb
--- /dev/null
+++ b/xen/arch/arm/arm64/cache.S
@@ -0,0 +1,100 @@
+/*
+ * Cache maintenance
+ *
+ * Copyright (C) 2001 Deep Blue Solutions Ltd.
+ * Copyright (C) 2012 ARM Ltd.
+ * Copyright (C) 1996-2000 Russell King
+ * Copyright (C) 2012 ARM Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+/*
+ * Enable and disable interrupts.
+ */
+ .macro disable_irq
+ msr daifset, #2
+ .endm
+
+ .macro enable_irq
+ msr daifclr, #2
+ .endm
+
+/*
+ * Save/disable and restore interrupts.
+ */
+ .macro save_and_disable_irqs, olddaif
+ mrs \olddaif, daif
+ disable_irq
+ .endm
+
+ .macro restore_irqs, olddaif
+ msr daif, \olddaif
+ .endm
+
+/*
+ * __flush_dcache_all()
+ *
+ * Flush the whole D-cache.
+ *
+ * Corrupted registers: x0-x7, x9-x11
+ */
+ .globl __flush_dcache_all
+__flush_dcache_all:
+ dmb sy // ensure ordering with previous memory accesses
+ mrs x0, clidr_el1 // read clidr
+ and x3, x0, #0x7000000 // extract loc from clidr
+ lsr x3, x3, #23 // left align loc bit field
+ cbz x3, finished // if loc is 0, then no need to clean
+ mov x10, #0 // start clean at cache level 0
+loop1:
+ add x2, x10, x10, lsr #1 // work out 3x current cache level
+ lsr x1, x0, x2 // extract cache type bits from clidr
+ and x1, x1, #7 // mask of the bits for current cache only
+ cmp x1, #2 // see what cache we have at this level
+ b.lt skip // skip if no cache, or just i-cache
+ save_and_disable_irqs x9 // make CSSELR and CCSIDR access atomic
+ msr csselr_el1, x10 // select current cache level in csselr
+ isb // isb to sych the new cssr&csidr
+ mrs x1, ccsidr_el1 // read the new ccsidr
+ restore_irqs x9
+ and x2, x1, #7 // extract the length of the cache lines
+ add x2, x2, #4 // add 4 (line length offset)
+ mov x4, #0x3ff
+ and x4, x4, x1, lsr #3 // find maximum number on the way size
+ clz w5, w4 // find bit position of way size increment
+ mov x7, #0x7fff
+ and x7, x7, x1, lsr #13 // extract max number of the index size
+loop2:
+ mov x9, x4 // create working copy of max way size
+loop3:
+ lsl x6, x9, x5
+ orr x11, x10, x6 // factor way and cache number into x11
+ lsl x6, x7, x2
+ orr x11, x11, x6 // factor index number into x11
+ dc cisw, x11 // clean & invalidate by set/way
+ subs x9, x9, #1 // decrement the way
+ b.ge loop3
+ subs x7, x7, #1 // decrement the index
+ b.ge loop2
+skip:
+ add x10, x10, #2 // increment cache number
+ cmp x3, x10
+ b.gt loop1
+finished:
+ mov x10, #0 // swith back to cache level 0
+ msr csselr_el1, x10 // select current cache level in csselr
+ dsb sy
+ isb
+ ret
+ENDPROC(__flush_dcache_all)
--
2.0.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH V2 10/12] add arm64 cache flushing code from linux
2014-07-22 0:43 ` [PATCH V2 10/12] add arm64 cache flushing code from linux Roy Franz
@ 2014-07-28 15:53 ` Ian Campbell
2014-07-28 16:24 ` Ian Campbell
0 siblings, 1 reply; 50+ messages in thread
From: Ian Campbell @ 2014-07-28 15:53 UTC (permalink / raw)
To: Roy Franz
Cc: keir, tim, xen-devel, stefano.stabellini, jbeulich, linaro-uefi, fu.wei
On Mon, 2014-07-21 at 17:43 -0700, Roy Franz wrote:
> + .globl __flush_dcache_all
> +__flush_dcache_all:
Please use GLOBAL().
Other than that looks good (although I'm mostly taking it on trust that
the Linux version of this isn't broken):
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Ian.
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH V2 10/12] add arm64 cache flushing code from linux
2014-07-28 15:53 ` Ian Campbell
@ 2014-07-28 16:24 ` Ian Campbell
0 siblings, 0 replies; 50+ messages in thread
From: Ian Campbell @ 2014-07-28 16:24 UTC (permalink / raw)
To: Roy Franz
Cc: keir, tim, xen-devel, stefano.stabellini, jbeulich, linaro-uefi, fu.wei
On Mon, 2014-07-28 at 16:53 +0100, Ian Campbell wrote:
> On Mon, 2014-07-21 at 17:43 -0700, Roy Franz wrote:
> > + .globl __flush_dcache_all
> > +__flush_dcache_all:
>
> Please use GLOBAL().
Or rather ENTRY() in this case...
>
> Other than that looks good (although I'm mostly taking it on trust that
> the Linux version of this isn't broken):
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> Ian.
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH V2 11/12] Add fdt_create_empty_tree() function.
2014-07-22 0:43 [PATCH V2 00/12] arm64 EFI stub Roy Franz
` (9 preceding siblings ...)
2014-07-22 0:43 ` [PATCH V2 10/12] add arm64 cache flushing code from linux Roy Franz
@ 2014-07-22 0:43 ` Roy Franz
2014-07-22 16:36 ` [Linaro-uefi] " Julien Grall
2014-07-22 0:43 ` [PATCH V2 12/12] Add EFI stub for arm64 Roy Franz
2014-07-28 15:30 ` [PATCH V2 00/12] arm64 EFI stub Ian Campbell
12 siblings, 1 reply; 50+ messages in thread
From: Roy Franz @ 2014-07-22 0:43 UTC (permalink / raw)
To: xen-devel, ian.campbell, stefano.stabellini, tim, jbeulich, keir
Cc: Roy Franz, fu.wei, linaro-uefi
Add fdt_create_empty_tree() function from v1.4.0 of libfdt taken from
git://git.jdl.com/software/dtc.git
This function was not present in v1.3.0, but is a relatively simple
helper function, and appears to work fine with the v1.3.0 that is
currently present in XEN.
Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
xen/common/libfdt/Makefile.libfdt | 2 +-
xen/common/libfdt/fdt_empty_tree.c | 84 ++++++++++++++++++++++++++++++++++++++
xen/include/xen/libfdt/libfdt.h | 1 +
3 files changed, 86 insertions(+), 1 deletion(-)
create mode 100644 xen/common/libfdt/fdt_empty_tree.c
diff --git a/xen/common/libfdt/Makefile.libfdt b/xen/common/libfdt/Makefile.libfdt
index d55a6f8..4366627 100644
--- a/xen/common/libfdt/Makefile.libfdt
+++ b/xen/common/libfdt/Makefile.libfdt
@@ -6,5 +6,5 @@
LIBFDT_soname = libfdt.$(SHAREDLIB_EXT).1
LIBFDT_INCLUDES = fdt.h libfdt.h
LIBFDT_VERSION = version.lds
-LIBFDT_SRCS = fdt.c fdt_ro.c fdt_wip.c fdt_sw.c fdt_rw.c fdt_strerror.c
+LIBFDT_SRCS = fdt.c fdt_ro.c fdt_wip.c fdt_sw.c fdt_rw.c fdt_strerror.c fdt_empty_tree.c
LIBFDT_OBJS = $(LIBFDT_SRCS:%.c=%.o)
diff --git a/xen/common/libfdt/fdt_empty_tree.c b/xen/common/libfdt/fdt_empty_tree.c
new file mode 100644
index 0000000..f72d13b
--- /dev/null
+++ b/xen/common/libfdt/fdt_empty_tree.c
@@ -0,0 +1,84 @@
+/*
+ * libfdt - Flat Device Tree manipulation
+ * Copyright (C) 2012 David Gibson, IBM Corporation.
+ *
+ * libfdt is dual licensed: you can use it either under the terms of
+ * the GPL, or the BSD license, at your option.
+ *
+ * a) This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of the
+ * License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this library; if not, write to the Free
+ * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston,
+ * MA 02110-1301 USA
+ *
+ * Alternatively,
+ *
+ * b) Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ * copyright notice, this list of conditions and the following
+ * disclaimer.
+ * 2. Redistributions in binary form must reproduce the above
+ * copyright notice, this list of conditions and the following
+ * disclaimer in the documentation and/or other materials
+ * provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
+ * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+ * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+ * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
+ * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE,
+ * EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+#include "libfdt_env.h"
+
+#include <fdt.h>
+#include <libfdt.h>
+
+#include "libfdt_internal.h"
+
+int fdt_create_empty_tree(void *buf, int bufsize)
+{
+ int err;
+
+ err = fdt_create(buf, bufsize);
+ if (err)
+ return err;
+
+ err = fdt_finish_reservemap(buf);
+ if (err)
+ return err;
+
+ err = fdt_begin_node(buf, "");
+ if (err)
+ return err;
+
+ err = fdt_end_node(buf);
+ if (err)
+ return err;
+
+ err = fdt_finish(buf);
+ if (err)
+ return err;
+
+ return fdt_open_into(buf, buf, bufsize);
+}
+
diff --git a/xen/include/xen/libfdt/libfdt.h b/xen/include/xen/libfdt/libfdt.h
index 6086047..f4539fc 100644
--- a/xen/include/xen/libfdt/libfdt.h
+++ b/xen/include/xen/libfdt/libfdt.h
@@ -959,6 +959,7 @@ int fdt_finish(void *fdt);
/* Read-write functions */
/**********************************************************************/
+int fdt_create_empty_tree(void *buf, int bufsize);
int fdt_open_into(const void *fdt, void *buf, int bufsize);
int fdt_pack(void *fdt);
--
2.0.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [Linaro-uefi] [PATCH V2 11/12] Add fdt_create_empty_tree() function.
2014-07-22 0:43 ` [PATCH V2 11/12] Add fdt_create_empty_tree() function Roy Franz
@ 2014-07-22 16:36 ` Julien Grall
2014-07-22 17:12 ` Roy Franz
0 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2014-07-22 16:36 UTC (permalink / raw)
To: Roy Franz, xen-devel, ian.campbell, stefano.stabellini, tim,
jbeulich, keir
Cc: linaro-uefi
Hi Roy,
On 07/22/2014 01:43 AM, Roy Franz wrote:
> Add fdt_create_empty_tree() function from v1.4.0 of libfdt taken from
> git://git.jdl.com/software/dtc.git
> This function was not present in v1.3.0, but is a relatively simple
> helper function, and appears to work fine with the v1.3.0 that is
> currently present in XEN.
Shouldn't we update our internal libfdt to v1.4.0 rather than taking
only the new file?
Regards,
> Signed-off-by: Roy Franz <roy.franz@linaro.org>
> ---
> xen/common/libfdt/Makefile.libfdt | 2 +-
> xen/common/libfdt/fdt_empty_tree.c | 84 ++++++++++++++++++++++++++++++++++++++
> xen/include/xen/libfdt/libfdt.h | 1 +
> 3 files changed, 86 insertions(+), 1 deletion(-)
> create mode 100644 xen/common/libfdt/fdt_empty_tree.c
>
> diff --git a/xen/common/libfdt/Makefile.libfdt b/xen/common/libfdt/Makefile.libfdt
> index d55a6f8..4366627 100644
> --- a/xen/common/libfdt/Makefile.libfdt
> +++ b/xen/common/libfdt/Makefile.libfdt
> @@ -6,5 +6,5 @@
> LIBFDT_soname = libfdt.$(SHAREDLIB_EXT).1
> LIBFDT_INCLUDES = fdt.h libfdt.h
> LIBFDT_VERSION = version.lds
> -LIBFDT_SRCS = fdt.c fdt_ro.c fdt_wip.c fdt_sw.c fdt_rw.c fdt_strerror.c
> +LIBFDT_SRCS = fdt.c fdt_ro.c fdt_wip.c fdt_sw.c fdt_rw.c fdt_strerror.c fdt_empty_tree.c
> LIBFDT_OBJS = $(LIBFDT_SRCS:%.c=%.o)
> diff --git a/xen/common/libfdt/fdt_empty_tree.c b/xen/common/libfdt/fdt_empty_tree.c
> new file mode 100644
> index 0000000..f72d13b
> --- /dev/null
> +++ b/xen/common/libfdt/fdt_empty_tree.c
> @@ -0,0 +1,84 @@
> +/*
> + * libfdt - Flat Device Tree manipulation
> + * Copyright (C) 2012 David Gibson, IBM Corporation.
> + *
> + * libfdt is dual licensed: you can use it either under the terms of
> + * the GPL, or the BSD license, at your option.
> + *
> + * a) This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of the
> + * License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public
> + * License along with this library; if not, write to the Free
> + * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston,
> + * MA 02110-1301 USA
> + *
> + * Alternatively,
> + *
> + * b) Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above
> + * copyright notice, this list of conditions and the following
> + * disclaimer.
> + * 2. Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following
> + * disclaimer in the documentation and/or other materials
> + * provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> + * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
> + * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
> + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
> + * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE,
> + * EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +#include "libfdt_env.h"
> +
> +#include <fdt.h>
> +#include <libfdt.h>
> +
> +#include "libfdt_internal.h"
> +
> +int fdt_create_empty_tree(void *buf, int bufsize)
> +{
> + int err;
> +
> + err = fdt_create(buf, bufsize);
> + if (err)
> + return err;
> +
> + err = fdt_finish_reservemap(buf);
> + if (err)
> + return err;
> +
> + err = fdt_begin_node(buf, "");
> + if (err)
> + return err;
> +
> + err = fdt_end_node(buf);
> + if (err)
> + return err;
> +
> + err = fdt_finish(buf);
> + if (err)
> + return err;
> +
> + return fdt_open_into(buf, buf, bufsize);
> +}
> +
> diff --git a/xen/include/xen/libfdt/libfdt.h b/xen/include/xen/libfdt/libfdt.h
> index 6086047..f4539fc 100644
> --- a/xen/include/xen/libfdt/libfdt.h
> +++ b/xen/include/xen/libfdt/libfdt.h
> @@ -959,6 +959,7 @@ int fdt_finish(void *fdt);
> /* Read-write functions */
> /**********************************************************************/
>
> +int fdt_create_empty_tree(void *buf, int bufsize);
> int fdt_open_into(const void *fdt, void *buf, int bufsize);
> int fdt_pack(void *fdt);
>
>
--
Julien Grall
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [Linaro-uefi] [PATCH V2 11/12] Add fdt_create_empty_tree() function.
2014-07-22 16:36 ` [Linaro-uefi] " Julien Grall
@ 2014-07-22 17:12 ` Roy Franz
2014-07-22 17:15 ` Julien Grall
0 siblings, 1 reply; 50+ messages in thread
From: Roy Franz @ 2014-07-22 17:12 UTC (permalink / raw)
To: Julien Grall
Cc: keir, Ian Campbell, tim, xen-devel, Stefano Stabellini,
Jan Beulich, linaro-uefi
On Tue, Jul 22, 2014 at 9:36 AM, Julien Grall <julien.grall@linaro.org> wrote:
> Hi Roy,
>
> On 07/22/2014 01:43 AM, Roy Franz wrote:
>> Add fdt_create_empty_tree() function from v1.4.0 of libfdt taken from
>> git://git.jdl.com/software/dtc.git
>> This function was not present in v1.3.0, but is a relatively simple
>> helper function, and appears to work fine with the v1.3.0 that is
>> currently present in XEN.
>
> Shouldn't we update our internal libfdt to v1.4.0 rather than taking
> only the new file?
>
> Regards,
I can certainly do that - I was simply being conservative.
Should I prepare an patch independent of the EFI stub series to
just update the libfdt, or would you like it kept as part of the current
series?
Roy
>
>> Signed-off-by: Roy Franz <roy.franz@linaro.org>
>> ---
>> xen/common/libfdt/Makefile.libfdt | 2 +-
>> xen/common/libfdt/fdt_empty_tree.c | 84 ++++++++++++++++++++++++++++++++++++++
>> xen/include/xen/libfdt/libfdt.h | 1 +
>> 3 files changed, 86 insertions(+), 1 deletion(-)
>> create mode 100644 xen/common/libfdt/fdt_empty_tree.c
>>
>> diff --git a/xen/common/libfdt/Makefile.libfdt b/xen/common/libfdt/Makefile.libfdt
>> index d55a6f8..4366627 100644
>> --- a/xen/common/libfdt/Makefile.libfdt
>> +++ b/xen/common/libfdt/Makefile.libfdt
>> @@ -6,5 +6,5 @@
>> LIBFDT_soname = libfdt.$(SHAREDLIB_EXT).1
>> LIBFDT_INCLUDES = fdt.h libfdt.h
>> LIBFDT_VERSION = version.lds
>> -LIBFDT_SRCS = fdt.c fdt_ro.c fdt_wip.c fdt_sw.c fdt_rw.c fdt_strerror.c
>> +LIBFDT_SRCS = fdt.c fdt_ro.c fdt_wip.c fdt_sw.c fdt_rw.c fdt_strerror.c fdt_empty_tree.c
>> LIBFDT_OBJS = $(LIBFDT_SRCS:%.c=%.o)
>> diff --git a/xen/common/libfdt/fdt_empty_tree.c b/xen/common/libfdt/fdt_empty_tree.c
>> new file mode 100644
>> index 0000000..f72d13b
>> --- /dev/null
>> +++ b/xen/common/libfdt/fdt_empty_tree.c
>> @@ -0,0 +1,84 @@
>> +/*
>> + * libfdt - Flat Device Tree manipulation
>> + * Copyright (C) 2012 David Gibson, IBM Corporation.
>> + *
>> + * libfdt is dual licensed: you can use it either under the terms of
>> + * the GPL, or the BSD license, at your option.
>> + *
>> + * a) This library is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 of the
>> + * License, or (at your option) any later version.
>> + *
>> + * This library is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public
>> + * License along with this library; if not, write to the Free
>> + * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston,
>> + * MA 02110-1301 USA
>> + *
>> + * Alternatively,
>> + *
>> + * b) Redistribution and use in source and binary forms, with or
>> + * without modification, are permitted provided that the following
>> + * conditions are met:
>> + *
>> + * 1. Redistributions of source code must retain the above
>> + * copyright notice, this list of conditions and the following
>> + * disclaimer.
>> + * 2. Redistributions in binary form must reproduce the above
>> + * copyright notice, this list of conditions and the following
>> + * disclaimer in the documentation and/or other materials
>> + * provided with the distribution.
>> + *
>> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
>> + * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
>> + * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
>> + * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
>> + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
>> + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
>> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
>> + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
>> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
>> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
>> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
>> + * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE,
>> + * EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>> + */
>> +#include "libfdt_env.h"
>> +
>> +#include <fdt.h>
>> +#include <libfdt.h>
>> +
>> +#include "libfdt_internal.h"
>> +
>> +int fdt_create_empty_tree(void *buf, int bufsize)
>> +{
>> + int err;
>> +
>> + err = fdt_create(buf, bufsize);
>> + if (err)
>> + return err;
>> +
>> + err = fdt_finish_reservemap(buf);
>> + if (err)
>> + return err;
>> +
>> + err = fdt_begin_node(buf, "");
>> + if (err)
>> + return err;
>> +
>> + err = fdt_end_node(buf);
>> + if (err)
>> + return err;
>> +
>> + err = fdt_finish(buf);
>> + if (err)
>> + return err;
>> +
>> + return fdt_open_into(buf, buf, bufsize);
>> +}
>> +
>> diff --git a/xen/include/xen/libfdt/libfdt.h b/xen/include/xen/libfdt/libfdt.h
>> index 6086047..f4539fc 100644
>> --- a/xen/include/xen/libfdt/libfdt.h
>> +++ b/xen/include/xen/libfdt/libfdt.h
>> @@ -959,6 +959,7 @@ int fdt_finish(void *fdt);
>> /* Read-write functions */
>> /**********************************************************************/
>>
>> +int fdt_create_empty_tree(void *buf, int bufsize);
>> int fdt_open_into(const void *fdt, void *buf, int bufsize);
>> int fdt_pack(void *fdt);
>>
>>
>
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [Linaro-uefi] [PATCH V2 11/12] Add fdt_create_empty_tree() function.
2014-07-22 17:12 ` Roy Franz
@ 2014-07-22 17:15 ` Julien Grall
2014-07-23 9:58 ` Ian Campbell
0 siblings, 1 reply; 50+ messages in thread
From: Julien Grall @ 2014-07-22 17:15 UTC (permalink / raw)
To: Roy Franz
Cc: keir, Ian Campbell, tim, xen-devel, Stefano Stabellini,
Jan Beulich, linaro-uefi
On 07/22/2014 06:12 PM, Roy Franz wrote:
> On Tue, Jul 22, 2014 at 9:36 AM, Julien Grall <julien.grall@linaro.org> wrote:
>> On 07/22/2014 01:43 AM, Roy Franz wrote:
>>> Add fdt_create_empty_tree() function from v1.4.0 of libfdt taken from
>>> git://git.jdl.com/software/dtc.git
>>> This function was not present in v1.3.0, but is a relatively simple
>>> helper function, and appears to work fine with the v1.3.0 that is
>>> currently present in XEN.
>>
>> Shouldn't we update our internal libfdt to v1.4.0 rather than taking
>> only the new file?
>>
>> Regards,
>
> I can certainly do that - I was simply being conservative.
It's better to update the whole library to keep track of change. Ian,
Stefano, any thoughts?
> Should I prepare an patch independent of the EFI stub series to
> just update the libfdt, or would you like it kept as part of the current
> series?
I'm fine if you let the libfdt change in this series.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Linaro-uefi] [PATCH V2 11/12] Add fdt_create_empty_tree() function.
2014-07-22 17:15 ` Julien Grall
@ 2014-07-23 9:58 ` Ian Campbell
2014-07-23 16:15 ` Roy Franz
0 siblings, 1 reply; 50+ messages in thread
From: Ian Campbell @ 2014-07-23 9:58 UTC (permalink / raw)
To: Julien Grall
Cc: keir, tim, xen-devel, Roy Franz, Stefano Stabellini, Jan Beulich,
linaro-uefi
On Tue, 2014-07-22 at 18:15 +0100, Julien Grall wrote:
> On 07/22/2014 06:12 PM, Roy Franz wrote:
> > On Tue, Jul 22, 2014 at 9:36 AM, Julien Grall <julien.grall@linaro.org> wrote:
> >> On 07/22/2014 01:43 AM, Roy Franz wrote:
> >>> Add fdt_create_empty_tree() function from v1.4.0 of libfdt taken from
> >>> git://git.jdl.com/software/dtc.git
> >>> This function was not present in v1.3.0, but is a relatively simple
> >>> helper function, and appears to work fine with the v1.3.0 that is
> >>> currently present in XEN.
> >>
> >> Shouldn't we update our internal libfdt to v1.4.0 rather than taking
> >> only the new file?
> >>
> >> Regards,
> >
> > I can certainly do that - I was simply being conservative.
>
> It's better to update the whole library to keep track of change. Ian,
> Stefano, any thoughts?
Updating the library would certainly be preferable if Roy is willing.
> > Should I prepare an patch independent of the EFI stub series to
> > just update the libfdt, or would you like it kept as part of the current
> > series?
>
> I'm fine if you let the libfdt change in this series.
Yeah, that's fine.
Ian.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Linaro-uefi] [PATCH V2 11/12] Add fdt_create_empty_tree() function.
2014-07-23 9:58 ` Ian Campbell
@ 2014-07-23 16:15 ` Roy Franz
0 siblings, 0 replies; 50+ messages in thread
From: Roy Franz @ 2014-07-23 16:15 UTC (permalink / raw)
To: Ian Campbell
Cc: keir, Julien Grall, tim, xen-devel, Stefano Stabellini,
Jan Beulich, linaro-uefi
[-- Attachment #1.1: Type: text/plain, Size: 1410 bytes --]
On Wed, Jul 23, 2014 at 2:58 AM, Ian Campbell <Ian.Campbell@citrix.com>
wrote:
> On Tue, 2014-07-22 at 18:15 +0100, Julien Grall wrote:
> > On 07/22/2014 06:12 PM, Roy Franz wrote:
> > > On Tue, Jul 22, 2014 at 9:36 AM, Julien Grall <julien.grall@linaro.org>
> wrote:
> > >> On 07/22/2014 01:43 AM, Roy Franz wrote:
> > >>> Add fdt_create_empty_tree() function from v1.4.0 of libfdt taken from
> > >>> git://git.jdl.com/software/dtc.git
> > >>> This function was not present in v1.3.0, but is a relatively simple
> > >>> helper function, and appears to work fine with the v1.3.0 that is
> > >>> currently present in XEN.
> > >>
> > >> Shouldn't we update our internal libfdt to v1.4.0 rather than taking
> > >> only the new file?
> > >>
> > >> Regards,
> > >
> > > I can certainly do that - I was simply being conservative.
> >
> > It's better to update the whole library to keep track of change. Ian,
> > Stefano, any thoughts?
>
> Updating the library would certainly be preferable if Roy is willing.
>
> > > Should I prepare an patch independent of the EFI stub series to
> > > just update the libfdt, or would you like it kept as part of the
> current
> > > series?
> >
> > I'm fine if you let the libfdt change in this series.
>
> Yeah, that's fine.
>
> Ian.
>
> I'll do this in my next version. I'm on vacation until August 4th
(starting tomorrow), so my next version
won't be until early August.
Roy
[-- Attachment #1.2: Type: text/html, Size: 2340 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH V2 12/12] Add EFI stub for arm64
2014-07-22 0:43 [PATCH V2 00/12] arm64 EFI stub Roy Franz
` (10 preceding siblings ...)
2014-07-22 0:43 ` [PATCH V2 11/12] Add fdt_create_empty_tree() function Roy Franz
@ 2014-07-22 0:43 ` Roy Franz
2014-07-29 9:46 ` Ian Campbell
2014-07-28 15:30 ` [PATCH V2 00/12] arm64 EFI stub Ian Campbell
12 siblings, 1 reply; 50+ messages in thread
From: Roy Franz @ 2014-07-22 0:43 UTC (permalink / raw)
To: xen-devel, ian.campbell, stefano.stabellini, tim, jbeulich, keir
Cc: Roy Franz, fu.wei, linaro-uefi
This patch adds the EFI stub for arm64. A PE/COFF header is added 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.
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. Device tree description of memory
is no longer used, as the stub uses the EFI memory map to populate bootinfo
memory banks.
Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
xen/arch/arm/Makefile | 5 +
xen/arch/arm/Rules.mk | 1 +
xen/arch/arm/arm64/head.S | 185 +++++++++-
xen/arch/arm/efi/Makefile | 2 +
xen/arch/arm/efi/efi.c | 714 ++++++++++++++++++++++++++++++++++++
xen/arch/arm/xen.lds.S | 1 +
xen/include/asm-arm/arm64/efibind.h | 216 +++++++++++
xen/include/asm-arm/efibind.h | 2 +
xen/include/asm-arm/setup.h | 2 +-
9 files changed, 1123 insertions(+), 5 deletions(-)
create mode 100644 xen/arch/arm/efi/Makefile
create mode 100644 xen/arch/arm/efi/efi.c
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/Makefile b/xen/arch/arm/Makefile
index c13206f..4af6925 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -1,5 +1,6 @@
subdir-$(arm32) += arm32
subdir-$(arm64) += arm64
+subdir-$(EFI) += efi
subdir-y += platforms
obj-$(EARLY_PRINTK) += early_printk.o
@@ -37,6 +38,10 @@ obj-y += processor.o
#obj-bin-y += ....o
+ifeq ($(EFI),y)
+AFLAGS += -DCONFIG_EFI_STUB
+endif
+
ifdef CONFIG_DTB_FILE
obj-y += dtb.o
AFLAGS += -DCONFIG_DTB_FILE=\"$(CONFIG_DTB_FILE)\"
diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
index 8658176..19bef80 100644
--- a/xen/arch/arm/Rules.mk
+++ b/xen/arch/arm/Rules.mk
@@ -31,6 +31,7 @@ CFLAGS += -mcpu=generic
CFLAGS += -mgeneral-regs-only # No fp registers etc
arm32 := n
arm64 := y
+EFI := y
endif
ifneq ($(call cc-option,$(CC),-fvisibility=hidden,n),n)
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 7d53143..ac822d3 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 */
@@ -84,7 +86,6 @@
#endif /* !CONFIG_EARLY_PRINTK */
/*.aarch64*/
-
/*
* Kernel startup entry point.
* ---------------------------
@@ -101,11 +102,22 @@
.global start
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,119 @@ 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
+ /*
+ * Add the PE/COFF header to the file. The address of this header
+ * is at offset 0x3c in the file, and is part of Linux "Image"
+ * header. The arm64 Linux Image format is designed to support
+ * being both an 'Image' format binary and a PE/COFF binary.
+ * The PE/COFF format is defined by Microsoft, and is available
+ * from: http://msdn.microsoft.com/en-us/gg463119.aspx
+ * Version 8.3 adds support for arm64 and UEFI usage.
+ */
+
+ .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 _end - 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 _end - 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 _end - 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 */
@@ -345,7 +469,7 @@ paging:
dsb sy
#if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */
/* Non-boot CPUs don't need to rebuild the fixmap itself, just
- * the mapping from boot_second to xen_fixmap */
+ * the mapping from boot_second to xen_fixmap */
cbnz x22, 1f
/* Add UART to the fixmap table */
@@ -561,9 +685,62 @@ putn: ret
* TODO: For now, the implementation return NULL every time
*/
GLOBAL(lookup_processor_type)
- mov x0, #0
+ 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.
+ * EFI mandates a 1:1 (unity) VA->PA mapping,
+ * so we can turn off the MMU before entering
+ * XEN.
+ *
+ * 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
+ bl __flush_dcache_all
+ ic ialluis
+
+ /* Turn off Dcache and MMU */
+ 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
+
+ /* 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/Makefile b/xen/arch/arm/efi/Makefile
new file mode 100644
index 0000000..74863ba
--- /dev/null
+++ b/xen/arch/arm/efi/Makefile
@@ -0,0 +1,2 @@
+obj-$(EFI) += efi.o
+CFLAGS += -fshort-wchar
diff --git a/xen/arch/arm/efi/efi.c b/xen/arch/arm/efi/efi.c
new file mode 100644
index 0000000..832b324
--- /dev/null
+++ b/xen/arch/arm/efi/efi.c
@@ -0,0 +1,714 @@
+#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>
+#include <efi/efiprot.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>
+#include <asm/setup.h>
+
+
+void __init noreturn blexit(const CHAR16 *str);
+
+#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;
+
+/*
+ * Hacky way to make sure EFI allocations end up in memory that XEN
+ * includes in its mappings.
+ * RFRANZ_TODO - this needs to be resolved properly.
+ */
+static EFI_PHYSICAL_ADDRESS max_addr = 0xffffffff;
+
+static void *new_fdt;
+
+static EFI_STATUS __init efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR *map,
+ unsigned long mmap_size,
+ unsigned long desc_size)
+{
+ int Index;
+ int i = 0;
+
+ EFI_MEMORY_DESCRIPTOR *desc_ptr = map;
+
+ for ( Index = 0; Index < (mmap_size / desc_size); Index++ )
+ {
+ if ( desc_ptr->Type == EfiConventionalMemory
+ || desc_ptr->Type == EfiBootServicesCode
+ || desc_ptr->Type == EfiBootServicesData )
+ {
+ bootinfo.mem.bank[i].start = desc_ptr->PhysicalStart;
+ bootinfo.mem.bank[i].size = desc_ptr->NumberOfPages * EFI_PAGE_SIZE;
+ if ( ++i >= NR_MEM_BANKS )
+ {
+ PrintStr(L"Warning: bootinfo mem banks exhausted\r\n");
+ break;
+ }
+ }
+ desc_ptr = NextMemoryDescriptor(desc_ptr, desc_size);
+ }
+
+ bootinfo.mem.nr_banks = i;
+ return EFI_SUCCESS;
+
+}
+
+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;
+
+ *map = NULL;
+ *mmap_size = EFI_PAGE_SIZE;
+again:
+ *mmap_size += EFI_PAGE_SIZE; /* Page size is allocation granularity */
+ status = sys_table_arg->BootServices->AllocatePool(EfiLoaderData,
+ *mmap_size, (void **)&m);
+ if ( status != EFI_SUCCESS )
+ return status;
+
+ *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);
+ return status;
+ }
+
+ if ( key_ptr && status == EFI_SUCCESS )
+ *key_ptr = key;
+ if ( desc_ver && status == EFI_SUCCESS )
+ *desc_ver = desc_version;
+
+ *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) )
+ {
+ fdt = tables[i].VendorTable;
+ break;
+ }
+ }
+ return fdt;
+}
+
+/*
+ * Get (or set if not present) the #addr-cells and #size cells
+ * properties of the chosen node. We need to know these to
+ * properly construct the address ranges used to describe the files
+ * loaded by the stub.
+ */
+static int __init 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 __init 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 __init 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;
+ 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;
+ }
+
+ /* Add FDT entries for EFI runtime services in chosen node. */
+ 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;
+ }
+ }
+
+ 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
+ fdt_size = 0;
+
+ pages = PFN_UP(fdt_size) + PFN_UP(add_size);
+ fdt_addr = max_addr;
+ status = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
+ pages, &fdt_addr);
+
+ if ( status != EFI_SUCCESS )
+ return NULL;
+
+ new_fdt = (void *)fdt_addr;
+
+ if ( fdt_size )
+ {
+ if ( fdt_open_into(dtb.ptr, new_fdt, pages * EFI_PAGE_SIZE) )
+ return NULL;
+ }
+ else
+ {
+ /*
+ * Create an empty FDT if not provided one, which is the expected case
+ * when booted from the UEFI shell on an ACPI only system. We will use
+ * the FDT to pass the EFI information to XEN, as well as nodes for
+ * any modules the stub loads. The ACPI tables are part of the UEFI
+ * system table that is passed in the FDT.
+ */
+ PrintStr(L"before fdt_create_empty_tree\r\n");
+ if ( fdt_create_empty_tree(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, max_addr);
+ PrintStr(L"Using FDT from file ");
+ PrintStr(name.w);
+ PrintStr(L"\r\n");
+ efi_bs->FreePool(name.w);
+ }
+ else
+ {
+ /* Get DTB from configuration table. */
+ dtb.ptr = lookup_fdt_config_table(SystemTable);
+ if ( dtb.ptr )
+ {
+ PrintStr(L"Using FDT from EFI configuration table\r\n");
+ /* Set dtb.size to zero so config table memory is not freed. */
+ dtb.size = 0;
+ }
+ }
+
+ /*
+ * 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,
+ §ion.w, &cmdline.w);
+
+ if ( cmdline.w )
+ w2s(&cmdline);
+
+ /* Open and read config file */
+ read_config_file(&dir_handle, &cfg, cfg_file_name,
+ §ion, 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, max_addr);
+
+ 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,
+ "multiboot,kernel") + 1;
+ if ( compat_len > COMPAT_BUF_SIZE )
+ blexit(L"FDT string overflow");
+ compat_len += snprintf(compat_buf + compat_len,
+ COMPAT_BUF_SIZE - compat_len,
+ "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, max_addr);
+
+ 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,
+ "multiboot,ramdisk") + 1;
+ if ( compat_len > COMPAT_BUF_SIZE )
+ blexit(L"FDT string overflow");
+ compat_len += snprintf(compat_buf + compat_len,
+ COMPAT_BUF_SIZE - compat_len,
+ "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");
+ }
+
+
+ /* Free config file buffer */
+ efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
+ cfg.addr = 0;
+
+ 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");
+
+
+ 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.");
+
+ /*
+ * Put available EFI memory into bootinfo memory map.
+ */
+ efi_process_memory_map_bootinfo(mmap_ptr, mmap_size, desc_size);
+
+ 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 */
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
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 = .;
diff --git a/xen/include/asm-arm/arm64/efibind.h b/xen/include/asm-arm/arm64/efibind.h
new file mode 100644
index 0000000..2b0bf40
--- /dev/null
+++ b/xen/include/asm-arm/arm64/efibind.h
@@ -0,0 +1,216 @@
+/*++
+
+Copyright (c) 1998 Intel Corporation
+
+Module Name:
+
+ efefind.h
+
+Abstract:
+
+ EFI to compile bindings
+
+
+
+
+Revision History
+
+--*/
+
+#ifndef __GNUC__
+#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
+//
+
+#if !defined(__STDC_VERSION__) || (__STDC_VERSION__ < 199901L )
+
+ // No ANSI C 1999/2000 stdint.h integer width declarations
+
+ #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;
+ 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;
+
+#define POST_CODE(_Data)
+
+
+#define BREAKPOINT() while (TRUE); // Make it hang on Bios[Dbg]32
+
+//
+// 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))
+
+#define EXPORTAPI
+
+
+//
+// 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
+ #define EFIAPI // Substitute expresion to force C calling convention
+#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()
+
+
+//
+// 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)
+
+
+//
+// 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
+
+#endif
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
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>
diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index 36e5704..40814e6 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -3,7 +3,7 @@
#include <public/version.h>
-#define NR_MEM_BANKS 8
+#define NR_MEM_BANKS 32
#define MAX_MODULES 5 /* Current maximum useful modules */
--
2.0.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH V2 12/12] Add EFI stub for arm64
2014-07-22 0:43 ` [PATCH V2 12/12] Add EFI stub for arm64 Roy Franz
@ 2014-07-29 9:46 ` Ian Campbell
0 siblings, 0 replies; 50+ messages in thread
From: Ian Campbell @ 2014-07-29 9:46 UTC (permalink / raw)
To: Roy Franz
Cc: keir, tim, xen-devel, stefano.stabellini, jbeulich, linaro-uefi, fu.wei
On Mon, 2014-07-21 at 17:43 -0700, Roy Franz wrote:
Overall looks good, thanks, I've a bunch of mostly minor comments.
> +ifeq ($(EFI),y)
> +AFLAGS += -DCONFIG_EFI_STUB
AFAICT this is only used in arm64/head.S where there is no need for the
stub to be conditional.
So this can go away, unless there is some common code which I'm missing.
> @@ -84,7 +86,6 @@
> #endif /* !CONFIG_EARLY_PRINTK */
>
> /*.aarch64*/
> -
Spurious change.
> /*
> * Kernel startup entry point.
> * ---------------------------
> @@ -101,11 +102,22 @@
>
> .global start
> start:
> +#ifdef CONFIG_EFI_STUB
As I said above I don't think this needs to be conditional.
> @@ -561,9 +685,62 @@ putn: ret
> * TODO: For now, the implementation return NULL every time
> */
> GLOBAL(lookup_processor_type)
> - mov x0, #0
> + mov x0, #0
> + ret
> +
> +
> +
Please avoid multiple blank lines, just one will do.
> +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.
> + * EFI mandates a 1:1 (unity) VA->PA mapping,
> + * so we can turn off the MMU before entering
> + * XEN.
> + *
> + * 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.
> + */
> +
> +
The comment should abut the mov which it refers to please.
> + mov x20, x0
> + bl __flush_dcache_all
> + ic ialluis
iall... is indented a bit to far.
> diff --git a/xen/arch/arm/efi/Makefile b/xen/arch/arm/efi/Makefile
> new file mode 100644
> index 0000000..74863ba
> --- /dev/null
> +++ b/xen/arch/arm/efi/Makefile
> @@ -0,0 +1,2 @@
> +obj-$(EFI) += efi.o
I assume you don't recurse into here unless efi == y? So you can use
obj-y I think.
> diff --git a/xen/arch/arm/efi/efi.c b/xen/arch/arm/efi/efi.c
> new file mode 100644
> index 0000000..832b324
> --- /dev/null
> +++ b/xen/arch/arm/efi/efi.c
> @@ -0,0 +1,714 @@
> +#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>
> +#include <efi/efiprot.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>
Do you really use all of these? e.g. I don't see why keyhandler is
needed. I didn't check the others.
> +#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>
> +#include <asm/setup.h>
> +
>
> +void __init noreturn blexit(const CHAR16 *str);
Should this be static?
> +#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;
Put externs in a header please. Aren't these defined in common code and
therefore belong in a common header?
> +/*
> + * Hacky way to make sure EFI allocations end up in memory that XEN
> + * includes in its mappings.
This was due to the discard_initial_modules thing, right?
> + * RFRANZ_TODO - this needs to be resolved properly.
> + */
> +static EFI_PHYSICAL_ADDRESS max_addr = 0xffffffff;
> +
> +static void *new_fdt;
> +
> +static EFI_STATUS __init efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR *map,
> + unsigned long mmap_size,
> + unsigned long desc_size)
> +{
> + int Index;
> + int i = 0;
> +
> + EFI_MEMORY_DESCRIPTOR *desc_ptr = map;
> +
> + for ( Index = 0; Index < (mmap_size / desc_size); Index++ )
Perhaps this is my ignorance of EFI showing, but is Index really not
used within the loop?
It almost looks like you want a test on desc_ptr for the end of the list
or something?
> + {
> + if ( desc_ptr->Type == EfiConventionalMemory
> + || desc_ptr->Type == EfiBootServicesCode
> + || desc_ptr->Type == EfiBootServicesData )
> + {
> + bootinfo.mem.bank[i].start = desc_ptr->PhysicalStart;
> + bootinfo.mem.bank[i].size = desc_ptr->NumberOfPages * EFI_PAGE_SIZE;
Having both Index and i is a bit confusing. How about s/i/bank/? Or
perhaps
struct thingumy *bank;
if ( bootinfo.mem.nr_banks == NR_MEM_BANKS )
{
exhausted...
}
bank = &bootinfo.mem.bank[bootinfo.mem.nr_banks++];
bank->start = ...
?
> +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;
> +
> + *map = NULL;
> + *mmap_size = EFI_PAGE_SIZE;
> +again:
> + *mmap_size += EFI_PAGE_SIZE; /* Page size is allocation granularity */
These two assignments together mean that on the first pass you will
allocate two pages, is that what you intended?
> + status = sys_table_arg->BootServices->AllocatePool(EfiLoaderData,
> + *mmap_size, (void **)&m);
I don't think you need the cast here.
> + 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));
Are the bindings for linux,uefi-* described somewhere?
These are used entirely within the stub->kernel (Xen or Linux) interface
boundary, right? Since they are "internal" I wonder if we should use
"xen,uefi-*" (with the same semantics as linux,uefi-*) to avoid any
potential confusion in the future?
If there is any requirement for these to be exposed outside of the
Xen<->Stub interface then we would be better off using the linux naming
to remain compatible.
> + pages = PFN_UP(fdt_size) + PFN_UP(add_size);
pages = PFN_UP(fdt_size + add_size) ?
> + /*
> + * 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
Too many pppps.
> + * bound on how much size can be added based on configuration
> + * file contents.
The size of the configuration file doesn't seem like a very good
estimate, it doesn't account for the linux,uefi-* properties and
a /chosen/module node could be larger than the string in the cfg file
needed to specify it. I suppose the extra EFI_PAGE_SIZE slop, plus the
rounding up to a page probably accounts for this sufficiently.
FWIW in the equivalent Xen side code we just allocate guessed size and
if it isn't enough throw it away and try again from scratch with a
larger size, rather than trying to increase the size on the fly which as
you say gets complicated.
> + /* Check if we were booted by the EFI firmware */
> + if ( SystemTable->Hdr.Signature != EFI_SYSTEM_TABLE_SIGNATURE )
> + goto fail;
You only use goto fail once in this function AFAICT, everywhere else
calls blexit directly with a specific error message. I think you may as
well do the same here.
> +
> + /* 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.");
This restriction has been relaxed to only require 4K alignment now. See
ca59618967fe "xen: arm: Handle 4K aligned hypervisor load address".
> + compat_len = 0;
> + compat_len += snprintf(compat_buf + compat_len,
> + COMPAT_BUF_SIZE - compat_len,
> + "multiboot,kernel") + 1;
> + if ( compat_len > COMPAT_BUF_SIZE )
> + blexit(L"FDT string overflow");
> + compat_len += snprintf(compat_buf + compat_len,
> + COMPAT_BUF_SIZE - compat_len,
> + "multiboot,module") + 1;
> + if ( compat_len > COMPAT_BUF_SIZE )
> + blexit(L"FDT string overflow");
In most places we use
const char *compat = "multiboot,module\0" "multiboot,kernel";
to avoid the faffing around with buffers, snprintf and off by one errors.
> + /*
> + * 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.
Is the prepending of the image name an EFI-ism?
> +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);
Is EFI_SUCCESS correct? I thought this was an error path.
> +
> +//
> +// When build similiar to FW, then link everything together as
"similar" (cut-n-paste error from elsewhere?)
> +// one big module.
> 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>
Should this include some sort of CONFIG_ARM_64 based guard against using
this code from an arm32 build?
#ifndef CONFIG_ARM_64
#error "EFI is arm64 only"
#endif
perhaps?
Ian.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH V2 00/12] arm64 EFI stub
2014-07-22 0:43 [PATCH V2 00/12] arm64 EFI stub Roy Franz
` (11 preceding siblings ...)
2014-07-22 0:43 ` [PATCH V2 12/12] Add EFI stub for arm64 Roy Franz
@ 2014-07-28 15:30 ` Ian Campbell
12 siblings, 0 replies; 50+ messages in thread
From: Ian Campbell @ 2014-07-28 15:30 UTC (permalink / raw)
To: Roy Franz
Cc: keir, tim, xen-devel, stefano.stabellini, jbeulich, linaro-uefi, fu.wei
On Mon, 2014-07-21 at 17:43 -0700, Roy Franz wrote:
> I think this patchset is in good shape, with the exception of the usage of
> the EFI memory map, which I am looking for some suggestions on. The current
> implementation populates the bootinfo memory bank list from the EFI memory map,
> rather than from the device tree, and doesn't use any reserved ranges. This
> works, however the EFI memory map can contain a lot of entries, many of which
> are small. Depending on the layout, this could trigger the code that sets
> up the frametable to discard most of memory. Also, as a side effect of some
> memory being ignored to manage the frametable size, the EFI stub needs to
> ensure that it loads modules into memory that XEN is going to map. The
> FVP model has 2GBytes of DRAM at 0x80000000 and another 2 at 0x800000000,
> so the disjoint case is common.
>
> It seems that both these problems go largely away if sparse frametable mappings are
> supported, but I'm not sure how much effort that would be. I think most
> of the work done to handle things robustly with the non-sparse frametable
> would not apply once a sparse frametable is supported. Does adding support
> for a sparse frametable mapping seem like a reasonable way forward on this?
I think so, we need this for many reasons other than the issues it
causes with EFI anyway.
Ian.
^ permalink raw reply [flat|nested] 50+ messages in thread