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?
This patch series adds EFI support for arm64 in the form of a EFI stub in similar fashion to the linux kernel EFI support. A PE/COFF header is created in head.S, as there is no toolchain support for PE/COFF on arm64. This also has the advantage that the file is both an "Image" file and a PE/COFF executable - the same binary can be loaded and run either way. The EFI 'stub' code is a shim layer that serves as the loader for the XEN kernel in the EFI environment. The stub loads the dom0 kernel and initrd if required, and adds entries for them as well as for the EFI data structures into the device tree passed to XEN. Once the device tree is constructed, EFI boot services are exited, and the stub transfers control to the normal XEN entry point. The only indication XEN has that it was loaded via the stub is that the device tree contains EFI properties. This is all very similar to the arm/arm64 Linux kernel EFI stubs.
This patchset is based on the staging branch to get Ian's multiboot/device tree patches.
This series is also available at: git://git.linaro.org/people/roy.franz/xen.git arm64-efi-stub-v2
Changes since v1: (I have tried to address all feedback on v1) * Added common/efi directory for shared EFI code, and arch/arm/efi for arm-specfic code. Global build hacking of -fshort-wchar removed. arm32, arm64, and x86 with/without EFI enabled toolchain all build. The x86 build previously autodetected whether the EFI version should be built or not based on toolchain support. I couldn't get this working nicely with the common code, so for x86 I have the common code always build, and the EFI autodection works as normal for building the EFI version. * Basic use of the EFI memory map instead of FDT based memory description. More work needed to resolve differences between FDT description of a small number of large memory banks with reserved regions, and EFI's potentially long list of available regions, which can be long. * More refactoring of common EFI code to not directly exit using blexit(), as this broke the pre-linking targets. All shared code returns status, and it is up to the caller to exit and clean up. * Reduced the number of patches. Refactoring of x86 code first, then moving all code to efi-shared.c in one patch. * Fixed formatting/tab issues in new files, added Emacs footer. * Fixed efi_get_memory_map to return NULL map pointer on error in addition to failed status. * Added comments in head.S regarding PE/COFF specification, and 1:1 mapping used by EFI code. * Updated device tree bindings to use new multiboot bindings. Since the stub is always built into XEN, we don't have to support older bindings.
Roy Franz (12): Create efi-shared.[ch], and move string functions rename printErrMsg to PrintErrMesgExit Refactor get_parent_handle for sharing Refactor read_file() so it can be shared. replace split_value() with truncate_string() add read_config_file() function for XEN EFI config file create handle_cmdline() function Refactor get_argv() for sharing Move shared EFI functions to efi-shared.c add arm64 cache flushing code from linux Add fdt_create_empty_tree() function. Add EFI stub for arm64
xen/arch/arm/Makefile | 5 + xen/arch/arm/Rules.mk | 1 + xen/arch/arm/arm64/Makefile | 1 + xen/arch/arm/arm64/cache.S | 100 +++++ 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/arch/x86/Rules.mk | 1 + xen/arch/x86/efi/boot.c | 625 ++++--------------------------- xen/common/Makefile | 2 + xen/common/efi/Makefile | 3 + xen/common/efi/efi-shared.c | 648 ++++++++++++++++++++++++++++++++ xen/common/libfdt/Makefile.libfdt | 2 +- xen/common/libfdt/fdt_empty_tree.c | 84 +++++ xen/include/asm-arm/arm64/efibind.h | 216 +++++++++++ xen/include/asm-arm/efibind.h | 2 + xen/include/asm-arm/setup.h | 2 +- xen/include/efi/efi-shared.h | 72 ++++ xen/include/xen/libfdt/libfdt.h | 1 + 20 files changed, 2101 insertions(+), 566 deletions(-) create mode 100644 xen/arch/arm/arm64/cache.S create mode 100644 xen/arch/arm/efi/Makefile create mode 100644 xen/arch/arm/efi/efi.c create mode 100644 xen/common/efi/Makefile create mode 100644 xen/common/efi/efi-shared.c create mode 100644 xen/common/libfdt/fdt_empty_tree.c create mode 100644 xen/include/asm-arm/arm64/efibind.h create mode 100644 xen/include/asm-arm/efibind.h create mode 100644 xen/include/efi/efi-shared.h
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: + */
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
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.
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
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.
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
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.
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
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
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
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
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
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;
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
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 &&
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
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");
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
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
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
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
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 )
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
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
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
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
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
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);
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
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
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
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
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.
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
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 )
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
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
__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)
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.
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
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);
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);
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
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,
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.
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
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 */
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, #0ret
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_entrycmp x0, EFI_STUB_ERRORb.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, x0bl __flush_dcache_allic 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.
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.
-
Ian Campbell -
Jan Beulich -
Jan Beulich
-
Julien Grall -
Roy Franz