[PATCH V5 0/6] Add ARM EFI stub

[PATCH V5 0/6] Add ARM EFI stub

[Roy Franz]

This patch series adds EFI stub support for the ARM architecture.  The 
stub for ARM is implemented in a similar manner to x86 in that it is a 
shim layer between EFI and the normal zImage/bzImage boot process, and 
that an image with the stub configured is bootable as both a zImage and 
EFI application.

This patch depends on Leif Lindholm's v7 runtime services patchset, which 
in turn depends on Mark Salter's early_io_remap() patchset.  This EFI stub 
patchset can be tested without those by simply modifying the Kconfig for 
EFI_STUB to not depend on EFI.  The kernel will boot using the stub, 
however no EFI support will be available in the kernel.  Updated versions
of these other patchsets should be posted in the near future.

Changes since v4:
* Common and x86 EFI stub changes that this patch depends on were merged
  in 3.13.
* Updated FDT bindings to match agreed upon bindings in Leif's runtime
  services patchset.  This includes adding the kernel_banner version string,
  and changing how the EFI memory map is passed.  The EFI memory map is now
  allocated as EFI_RUNTIME_SERVICES_DATA since it may be passed to the kernel.
  The FDT bindings for arm/arm64 are identical.
* Added space in PE/COFF header for signature to allow signing of the image
  for EFI secure boot.
* Moved shared FDT function to drivers/firmware/efi/fdt.c.  This will be shared
  with arm64.  (Mark Salter has a patchset for the arm64 stub.)


Changes since v3:
* Common and x86 EFI stub changes have been broken out and submitted separately.
  This patch series depends on the "[PATCH V5 00/18] ARM EFI stub common code"
  series as merged by Matt Fleming. (Patch 10 of that series omitted)
* Re-added -fno-stack-protector.  Needed for compilation with 
  CONFIG_CC_STACKPROTECTOR option.  Cannot easily add support for stack 
  protection since there are no functions in stub that never return, which 
  is a requirement for use.  (x86 stub also disables stack protector.)
  Note that even in the absense of -fno-stack-protector, stack protection
  was never present in the decompressor, as the required functions are not
  implemented.
* Fixed long summaries in commit messages.
* Further cleanups of assembly in head.S


Changes since v2:
* Now depends on "arm: Add [U]EFI runtime services support" patches by leif.lindholm@linaro.org.
  The EFI memory map is the only memory map passed to the kernel, so EFI support
  is now required.
* remove "-fno-stack-protector" from decompressor Makefile.  The current code doesn't
  trigger the stack protection, so the decompressor now compiles with it still
  on.  Note that there has never been any stack protection in the decompressor
  since the stack usage doesn't trigger the heuristic in GCC, so right now
  the "-fno-stack-protector" is a noop.
* Changed EFI command line handling to not have a fixed limit.
* Change FDT memory allocation to retry with a larger allocation if
  first educated guess is inadequate.
* Correctly set 'SizeOfCode' in PE/COFF header.
* Reviewed ".setup" section that is in x86 PE/COFF header.  This is used for x86
  to account for code that is not in the .text section.  We don't need this
  for ARM, as all of our code is in the .text section, or in the PE/COFF header
  itself.
* Moved EFI_STUB_ERROR #define to header file to share between stub C and ASM.
* Variety of cleanups and fixes in head.S.
* Changed update_fdt_and_exit_boot() to just update the device tree, and
  renamed appropriately.  Memory allocations moved out of this function as
  well, which enables the retries if the initial FDT size is too small.
  Note that in order to do the retried allocations, the original FDT and 
  command line memory regions are left allocated.  This is OK since the kernel
  has the memory map and will free these allocations along with the initrd
  and new fdt allocations.
* Added prefix to all prints, reduced number of prints, and reviewed all
  messages.
* Change mixed usage of dtb/fdt to all be fdt or "device tree" in efi-stub.c
* remove unnecessary zimage_size variable from relocate_kernel()
* correct return types on EFI functions - should be efi_status_t, not int.

Roy Franz (6):
  efi-stub.txt updates for ARM
  Add shared update_fdt() function for ARM/ARM64
  Add strstr to compressed string.c for ARM.
  Add EFI stub for ARM
  Disable stack protection for decompressor/stub
  Add config EFI_STUB for ARM to Kconfig

 Documentation/efi-stub.txt             |   27 ++-
 arch/arm/Kconfig                       |   10 ++
 arch/arm/boot/compressed/Makefile      |   17 +-
 arch/arm/boot/compressed/efi-header.S  |  117 +++++++++++++
 arch/arm/boot/compressed/efi-stub.c    |  291 ++++++++++++++++++++++++++++++++
 arch/arm/boot/compressed/efi-stub.h    |    5 +
 arch/arm/boot/compressed/head.S        |   83 ++++++++-
 arch/arm/boot/compressed/string.c      |   21 +++
 drivers/firmware/efi/efi-stub-helper.c |    9 +-
 drivers/firmware/efi/fdt.c             |  115 +++++++++++++
 10 files changed, 678 insertions(+), 17 deletions(-)
 create mode 100644 arch/arm/boot/compressed/efi-header.S
 create mode 100644 arch/arm/boot/compressed/efi-stub.c
 create mode 100644 arch/arm/boot/compressed/efi-stub.h
 create mode 100644 drivers/firmware/efi/fdt.c

-- 
1.7.10.4

[PATCH V5 1/6] efi-stub.txt updates for ARM

[Roy Franz]

Update efi-stub.txt documentation to be more general
and not x86 specific.  Add ARM only "dtb=" command
line option description.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 Documentation/efi-stub.txt |   27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/Documentation/efi-stub.txt b/Documentation/efi-stub.txt
index 44e6bb6..19e897c 100644
--- a/Documentation/efi-stub.txt
+++ b/Documentation/efi-stub.txt
@@ -1,13 +1,16 @@
 			  The EFI Boot Stub
 		     ---------------------------
 
-On the x86 platform, a bzImage can masquerade as a PE/COFF image,
-thereby convincing EFI firmware loaders to load it as an EFI
-executable. The code that modifies the bzImage header, along with the
-EFI-specific entry point that the firmware loader jumps to are
-collectively known as the "EFI boot stub", and live in
+On the x86 and ARM platforms, a kernel zImage/bzImage can masquerade
+as a PE/COFF image, thereby convincing EFI firmware loaders to load
+it as an EFI executable. The code that modifies the bzImage header,
+along with the EFI-specific entry point that the firmware loader
+jumps to are collectively known as the "EFI boot stub", and live in
 arch/x86/boot/header.S and arch/x86/boot/compressed/eboot.c,
-respectively.
+respectively.  For ARM the EFI stub is implemented in
+arch/arm/boot/compressed/efi-header.S and
+arch/arm/boot/compressed/efi-stub.c.  EFI stub code that is shared
+between architectures is in drivers/firmware/efi/efi-stub-helper.c.
 
 By using the EFI boot stub it's possible to boot a Linux kernel
 without the use of a conventional EFI boot loader, such as grub or
@@ -23,7 +26,9 @@ The bzImage located in arch/x86/boot/bzImage must be copied to the EFI
 System Partiion (ESP) and renamed with the extension ".efi". Without
 the extension the EFI firmware loader will refuse to execute it. It's
 not possible to execute bzImage.efi from the usual Linux file systems
-because EFI firmware doesn't have support for them.
+because EFI firmware doesn't have support for them.  For ARM the
+arch/arm/boot/zImage should be copied to the system partition, and it
+may not need to be renamed.
 
 
 **** Passing kernel parameters from the EFI shell
@@ -63,3 +68,11 @@ Notice how bzImage.efi can be specified with a relative path. That's
 because the image we're executing is interpreted by the EFI shell,
 which understands relative paths, whereas the rest of the command line
 is passed to bzImage.efi.
+
+
+**** The "dtb=" option
+
+For the ARM architecture, we also need to be able to provide a device
+tree to the kernel.  This is done with the "dtb=" command line option,
+and is process in the same manner as the "initrd=" option that is described
+above.
-- 
1.7.10.4

[PATCH V5 2/6] Add shared update_fdt() function for ARM/ARM64

[Roy Franz]

Both ARM and ARM64 stubs will update the device tree
that they pass to the kernel.  In both cases they
primarily need to add the same UEFI related information,
so the function can be shared.
Create a new FDT related file for this to avoid use
of architecture #ifdefs in efi-stub-helper.c
Change EFI allocation type for memory map to
EFI_RUNTIME_SERVICES_DATA, since we are passing
this buffer to the kernel, or immediately freeing it.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 drivers/firmware/efi/efi-stub-helper.c |    9 ++-
 drivers/firmware/efi/fdt.c             |  115 ++++++++++++++++++++++++++++++++
 2 files changed, 123 insertions(+), 1 deletion(-)
 create mode 100644 drivers/firmware/efi/fdt.c

diff --git a/drivers/firmware/efi/efi-stub-helper.c b/drivers/firmware/efi/efi-stub-helper.c
index b6bffbf..bc9e37a 100644
--- a/drivers/firmware/efi/efi-stub-helper.c
+++ b/drivers/firmware/efi/efi-stub-helper.c
@@ -4,6 +4,7 @@
  * implementation files.
  *
  * Copyright 2011 Intel Corporation; author Matt Fleming
+ * Copyright 2013 Linaro Limited; author Roy Franz
  *
  * This file is part of the Linux kernel, and is made available
  * under the terms of the GNU General Public License version 2.
@@ -63,10 +64,16 @@ again:
 	/*
 	 * Add an additional efi_memory_desc_t because we're doing an
 	 * allocation which may be in a new descriptor region.
+	 * We allocate as EFI_RUNTIME_SERVICES_DATA since this is what
+	 * we want for when we pass the memory map to the kernel.  This
+	 * function is also used to get the memory map for other uses,
+	 * but is always freed by the stub so the allocation type
+	 * doesn't matter.
 	 */
 	*map_size += sizeof(*m);
 	status = efi_call_phys3(sys_table_arg->boottime->allocate_pool,
-				EFI_LOADER_DATA, *map_size, (void **)&m);
+				EFI_RUNTIME_SERVICES_DATA, *map_size,
+				(void **)&m);
 	if (status != EFI_SUCCESS)
 		goto fail;
 
diff --git a/drivers/firmware/efi/fdt.c b/drivers/firmware/efi/fdt.c
new file mode 100644
index 0000000..7f1431b
--- /dev/null
+++ b/drivers/firmware/efi/fdt.c
@@ -0,0 +1,115 @@
+/*
+ * FDT related Helper functions used by the EFI stub on multiple
+ * architectures. This should be #included by the EFI stub
+ * implementation files.
+ *
+ * Copyright 2013 Linaro Limited; author Roy Franz
+ *
+ * This file is part of the Linux kernel, and is made available
+ * under the terms of the GNU General Public License version 2.
+ *
+ */
+
+static efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
+			       void *fdt, int new_fdt_size, char *cmdline_ptr,
+			       u64 initrd_addr, u64 initrd_size,
+			       efi_memory_desc_t *memory_map,
+			       unsigned long map_size, unsigned long desc_size,
+			       u32 desc_ver)
+{
+	int node;
+	int status;
+	u32 fdt_val32;
+	u64 fdt_val64;
+	/* Copy definition of linux_banner here.  Since this code is
+	 * built as part of the decompressor for ARM v7, pulling
+	 * in version.c where linux_banner is defined for the
+	 * kernel brings other kernel dependencies with it.
+	 */
+	const char linux_banner[] =
+	    "Linux version " UTS_RELEASE " (" LINUX_COMPILE_BY "@"
+	    LINUX_COMPILE_HOST ") (" LINUX_COMPILER ") " UTS_VERSION "\n";
+
+	status = fdt_open_into(orig_fdt, fdt, new_fdt_size);
+	if (status != 0)
+		goto fdt_set_fail;
+
+	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;
+		}
+	}
+
+	if ((cmdline_ptr != NULL) && (strlen(cmdline_ptr) > 0)) {
+		status = fdt_setprop(fdt, node, "bootargs", cmdline_ptr,
+				     strlen(cmdline_ptr) + 1);
+		if (status)
+			goto fdt_set_fail;
+	}
+
+	/* Set intird address/end in device tree, if present */
+	if (initrd_size != 0) {
+		u64 initrd_image_end;
+		u64 initrd_image_start = cpu_to_fdt64(initrd_addr);
+		status = fdt_setprop(fdt, node, "linux,initrd-start",
+				     &initrd_image_start, sizeof(u64));
+		if (status)
+			goto fdt_set_fail;
+		initrd_image_end = cpu_to_fdt64(initrd_addr + initrd_size);
+		status = fdt_setprop(fdt, node, "linux,initrd-end",
+				     &initrd_image_end, sizeof(u64));
+		if (status)
+			goto fdt_set_fail;
+	}
+
+	/* Add FDT entries for EFI runtime services in chosen node. */
+	node = fdt_subnode_offset(fdt, 0, "chosen");
+	fdt_val64 = cpu_to_fdt64((u64)(unsigned long)sys_table);
+	status = fdt_setprop(fdt, node, "linux,uefi-system-table",
+			     &fdt_val64, sizeof(fdt_val64));
+	if (status)
+		goto fdt_set_fail;
+
+	fdt_val64 = cpu_to_fdt64((u64)(unsigned long)memory_map);
+	status = fdt_setprop(fdt, node, "linux,uefi-mmap-start",
+			     &fdt_val64,  sizeof(fdt_val64));
+	if (status)
+		goto fdt_set_fail;
+
+	fdt_val32 = cpu_to_fdt32(map_size);
+	status = fdt_setprop(fdt, node, "linux,uefi-mmap-size",
+			     &fdt_val32,  sizeof(fdt_val32));
+	if (status)
+		goto fdt_set_fail;
+
+	fdt_val32 = cpu_to_fdt32(desc_size);
+	status = fdt_setprop(fdt, node, "linux,uefi-mmap-desc-size",
+			     &fdt_val32, sizeof(fdt_val32));
+	if (status)
+		goto fdt_set_fail;
+
+	fdt_val32 = cpu_to_fdt32(desc_ver);
+	status = fdt_setprop(fdt, node, "linux,uefi-mmap-desc-ver",
+			     &fdt_val32, sizeof(fdt_val32));
+	if (status)
+		goto fdt_set_fail;
+
+	/* Add kernel version banner so stub/kernel match can be
+	 * verified.
+	 */
+	status = fdt_setprop_string(fdt, node, "linux,uefi-stub-kern-ver",
+			     linux_banner);
+	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;
+}
-- 
1.7.10.4

[PATCH V5 3/6] Add strstr to compressed string.c for ARM.

[Roy Franz]

The shared efi-stub-helper.c functions require a strstr
implementation.
Implementation copied from arch/x86/boot/string.c

Signed-off-by: Roy Franz <roy.franz@linaro.org>
Reviewed-by: Grant Likely <grant.likely@linaro.org>
---
 arch/arm/boot/compressed/string.c |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/arch/arm/boot/compressed/string.c b/arch/arm/boot/compressed/string.c
index 36e53ef..5397792 100644
--- a/arch/arm/boot/compressed/string.c
+++ b/arch/arm/boot/compressed/string.c
@@ -111,6 +111,27 @@ char *strchr(const char *s, int c)
 	return (char *)s;
 }
 
+/**
+ * strstr - Find the first substring in a %NUL terminated string
+ * @s1: The string to be searched
+ * @s2: The string to search for
+ */
+char *strstr(const char *s1, const char *s2)
+{
+	size_t l1, l2;
+
+	l2 = strlen(s2);
+	if (!l2)
+		return (char *)s1;
+	l1 = strlen(s1);
+	while (l1 >= l2) {
+		l1--;
+		if (!memcmp(s1, s2, l2))
+			return (char *)s1;
+		s1++;
+	}
+	return NULL;
+}
 #undef memset
 
 void *memset(void *s, int c, size_t count)
-- 
1.7.10.4

[PATCH V5 4/6] Add EFI stub for ARM

[Roy Franz]

This patch adds EFI stub support for the ARM Linux kernel.  The EFI stub
operates similarly to the x86 stub: it is a shim between the EFI firmware
and the normal zImage entry point, and sets up the environment that the
zImage is expecting.  This includes loading the initrd (optionaly) and
device tree from the system partition based on the kernel command line.
The stub updates the device tree as necessary, adding entries for EFI
runtime services. The PE/COFF "MZ" header at offset 0 results in the
first instruction being an add that corrupts r5, which is not used by
the zImage interface.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 arch/arm/boot/compressed/Makefile     |   15 +-
 arch/arm/boot/compressed/efi-header.S |  117 +++++++++++++
 arch/arm/boot/compressed/efi-stub.c   |  291 +++++++++++++++++++++++++++++++++
 arch/arm/boot/compressed/efi-stub.h   |    5 +
 arch/arm/boot/compressed/head.S       |   83 +++++++++-
 5 files changed, 503 insertions(+), 8 deletions(-)
 create mode 100644 arch/arm/boot/compressed/efi-header.S
 create mode 100644 arch/arm/boot/compressed/efi-stub.c
 create mode 100644 arch/arm/boot/compressed/efi-stub.h

diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
index e7190bb..c0c7fee 100644
--- a/arch/arm/boot/compressed/Makefile
+++ b/arch/arm/boot/compressed/Makefile
@@ -99,11 +99,22 @@ libfdt_objs	:= $(addsuffix .o, $(basename $(libfdt)))
 $(addprefix $(obj)/,$(libfdt) $(libfdt_hdrs)): $(obj)/%: $(srctree)/scripts/dtc/libfdt/%
 	$(call cmd,shipped)
 
-$(addprefix $(obj)/,$(libfdt_objs) atags_to_fdt.o): \
+$(addprefix $(obj)/,$(libfdt_objs) atags_to_fdt.o efi-stub.o): \
 	$(addprefix $(obj)/,$(libfdt_hdrs))
 
 ifeq ($(CONFIG_ARM_ATAG_DTB_COMPAT),y)
-OBJS	+= $(libfdt_objs) atags_to_fdt.o
+OBJS	+= atags_to_fdt.o
+USE_LIBFDT = y
+endif
+
+ifeq ($(CONFIG_EFI_STUB),y)
+CFLAGS_efi-stub.o += -DTEXT_OFFSET=$(TEXT_OFFSET)
+OBJS	+= efi-stub.o
+USE_LIBFDT = y
+endif
+
+ifeq ($(USE_LIBFDT),y)
+OBJS	+= $(libfdt_objs)
 endif
 
 targets       := vmlinux vmlinux.lds \
diff --git a/arch/arm/boot/compressed/efi-header.S b/arch/arm/boot/compressed/efi-header.S
new file mode 100644
index 0000000..dbb7101
--- /dev/null
+++ b/arch/arm/boot/compressed/efi-header.S
@@ -0,0 +1,117 @@
+@ Copyright (C) 2013 Linaro Ltd;  <roy.franz@linaro.org>
+@
+@ This file contains the PE/COFF header that is part of the
+@ EFI stub.
+@
+
+	.org	0x3c
+	@
+	@ The PE header can be anywhere in the file, but for
+	@ simplicity we keep it together with the MSDOS header
+	@ The offset to the PE/COFF header needs to be at offset
+	@ 0x3C in the MSDOS header.
+	@ The only 2 fields of the MSDOS header that are used are this
+	@ PE/COFF offset, and the "MZ" bytes at offset 0x0.
+	@
+	.long	pe_header			@ Offset to the PE header.
+
+      .align 3
+pe_header:
+	.ascii	"PE"
+	.short 	0
+
+coff_header:
+	.short	0x01c2				@ ARM or Thumb
+	.short	2				@ nr_sections
+	.long	0 				@ TimeDateStamp
+	.long	0				@ PointerToSymbolTable
+	.long	1				@ NumberOfSymbols
+	.short	section_table - optional_header	@ SizeOfOptionalHeader
+	.short	0x306				@ Characteristics.
+						@ IMAGE_FILE_32BIT_MACHINE |
+						@ IMAGE_FILE_DEBUG_STRIPPED |
+						@ IMAGE_FILE_EXECUTABLE_IMAGE |
+						@ IMAGE_FILE_LINE_NUMS_STRIPPED
+
+optional_header:
+	.short	0x10b				@ PE32 format
+	.byte	0x02				@ MajorLinkerVersion
+	.byte	0x14				@ MinorLinkerVersion
+
+	.long	_edata - efi_stub_entry		@ SizeOfCode
+
+	.long	0				@ SizeOfInitializedData
+	.long	0				@ SizeOfUninitializedData
+
+	.long	efi_stub_entry			@ AddressOfEntryPoint
+	.long	efi_stub_entry			@ BaseOfCode
+	.long	0				@ data
+
+extra_header_fields:
+	.long	0				@ ImageBase
+	.long	0x20				@ SectionAlignment
+	.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	_edata				@ SizeOfImage
+
+	@ Everything before the entry point is considered part of the header
+	.long	efi_stub_entry			@ SizeOfHeaders
+	.long	0				@ CheckSum
+	.short	0xa				@ Subsystem (EFI application)
+	.short	0				@ DllCharacteristics
+	.long	0				@ SizeOfStackReserve
+	.long	0				@ SizeOfStackCommit
+	.long	0				@ SizeOfHeapReserve
+	.long	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	_edata - efi_stub_entry		@ VirtualSize
+	.long	efi_stub_entry			@ VirtualAddress
+	.long	_edata - efi_stub_entry		@ SizeOfRawData
+	.long	efi_stub_entry			@ 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)
diff --git a/arch/arm/boot/compressed/efi-stub.c b/arch/arm/boot/compressed/efi-stub.c
new file mode 100644
index 0000000..2bd559d
--- /dev/null
+++ b/arch/arm/boot/compressed/efi-stub.c
@@ -0,0 +1,291 @@
+/*
+ * linux/arch/arm/boot/compressed/efi-stub.c
+ *
+ * Copyright (C) 2013 Linaro Ltd;  <roy.franz@linaro.org>
+ *
+ * This file implements the EFI boot stub for the ARM kernel
+ *
+ * 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.
+ *
+ */
+#include <linux/efi.h>
+#include <libfdt.h>
+#include "efi-stub.h"
+
+/* EFI function call wrappers.  These are not required for
+ * ARM, but wrappers are required for X86 to convert between
+ * ABIs.  These wrappers are provided to allow code sharing
+ * between X86 and ARM.  Since these wrappers directly invoke the
+ * EFI function pointer, the function pointer type must be properly
+ * defined, which is not the case for X86  One advantage of this is
+ * it allows for type checking of arguments, which is not
+ * possible with the X86 wrappers.
+ */
+#define efi_call_phys0(f)			f()
+#define efi_call_phys1(f, a1)			f(a1)
+#define efi_call_phys2(f, a1, a2)		f(a1, a2)
+#define efi_call_phys3(f, a1, a2, a3)		f(a1, a2, a3)
+#define efi_call_phys4(f, a1, a2, a3, a4)	f(a1, a2, a3, a4)
+#define efi_call_phys5(f, a1, a2, a3, a4, a5)	f(a1, a2, a3, a4, a5)
+
+/* The maximum uncompressed kernel size is 32 MBytes, so we will reserve
+ * that for the decompressed kernel.  We have no easy way to tell what
+ * the actuall size of code + data the uncompressed kernel will use.
+ */
+#define MAX_UNCOMP_KERNEL_SIZE	0x02000000
+
+/* The kernel zImage should be located between 32 Mbytes
+ * and 128 MBytes from the base of DRAM.  The min
+ * address leaves space for a maximal size uncompressed image,
+ * and the max address is due to how the zImage decompressor
+ * picks a destination address.
+ */
+#define ZIMAGE_OFFSET_LIMIT	0x08000000
+#define MIN_ZIMAGE_OFFSET	MAX_UNCOMP_KERNEL_SIZE
+
+#define PRINTK_PREFIX		"EFIstub: "
+
+struct fdt_region {
+	u64 base;
+	u64 size;
+};
+
+
+/* Include shared EFI stub code, and required headers. */
+#include "../../../../include/generated/compile.h"
+#include "../../../../include/generated/utsrelease.h"
+#include "../../../../drivers/firmware/efi/efi-stub-helper.c"
+#include "../../../../drivers/firmware/efi/fdt.c"
+
+
+int efi_entry(void *handle, efi_system_table_t *sys_table,
+	      unsigned long *zimage_addr)
+{
+	efi_loaded_image_t *image;
+	int status;
+	unsigned long nr_pages;
+	const struct fdt_region *region;
+
+	void *fdt;
+	int err;
+	int node;
+	unsigned long zimage_size = 0;
+	unsigned long dram_base;
+	/* addr/point and size pairs for memory management*/
+	unsigned long initrd_addr;
+	unsigned long initrd_size = 0;
+	unsigned long fdt_addr;
+	unsigned long fdt_size = 0;
+	efi_physical_addr_t kernel_reserve_addr;
+	unsigned long kernel_reserve_size = 0;
+	char *cmdline_ptr;
+	int cmdline_size = 0;
+
+	unsigned long map_size, desc_size;
+	u32 desc_ver;
+	unsigned long mmap_key;
+	efi_memory_desc_t *memory_map;
+
+	unsigned long new_fdt_size;
+	unsigned long new_fdt_addr;
+
+	efi_guid_t proto = LOADED_IMAGE_PROTOCOL_GUID;
+
+	/* Check if we were booted by the EFI firmware */
+	if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
+		goto fail;
+
+	efi_printk(sys_table, PRINTK_PREFIX"Booting Linux using EFI stub.\n");
+
+
+	/* get the command line from EFI, using the LOADED_IMAGE protocol */
+	status = efi_call_phys3(sys_table->boottime->handle_protocol,
+				handle, &proto, (void *)&image);
+	if (status != EFI_SUCCESS) {
+		efi_printk(sys_table, PRINTK_PREFIX"ERROR: Failed to get handle for LOADED_IMAGE_PROTOCOL.\n");
+		goto fail;
+	}
+
+	/* We are going to copy the command line into the device tree,
+	 * so this memory just needs to not conflict with boot protocol
+	 * requirements.
+	 */
+	cmdline_ptr = efi_convert_cmdline_to_ascii(sys_table, image,
+						   &cmdline_size);
+	if (!cmdline_ptr) {
+		efi_printk(sys_table, PRINTK_PREFIX"ERROR: Unable to allocate memory for command line.\n");
+		goto fail;
+	}
+
+	/* We first load the device tree, as we need to get the base address of
+	 * DRAM from the device tree.  The zImage, device tree, and initrd
+	 * have address restrictions that are relative to the base of DRAM.
+	 */
+	status = handle_cmdline_files(sys_table, image, cmdline_ptr, "dtb=",
+				      0xffffffff, &fdt_addr, &fdt_size);
+	if (status != EFI_SUCCESS) {
+		efi_printk(sys_table, PRINTK_PREFIX"ERROR: Unable to load device tree blob.\n");
+		goto fail_free_cmdline;
+	}
+
+	err = fdt_check_header((void *)fdt_addr);
+	if (err != 0) {
+		efi_printk(sys_table, PRINTK_PREFIX"ERROR: Device tree header not valid.\n");
+		goto fail_free_fdt;
+	}
+	if (fdt_totalsize((void *)fdt_addr) > fdt_size) {
+		efi_printk(sys_table, PRINTK_PREFIX"ERROR: Incomplete device tree.\n");
+		goto fail_free_fdt;
+
+	}
+
+
+	/* Look up the base of DRAM from the device tree. */
+	fdt = (void *)fdt_addr;
+	node = fdt_subnode_offset(fdt, 0, "memory");
+	region = fdt_getprop(fdt, node, "reg", NULL);
+	if (region) {
+		dram_base = fdt64_to_cpu(region->base);
+	} else {
+		/* There is no way to get amount or addresses of physical
+		 * memory installed using EFI calls.  If the device tree
+		 * we read from disk doesn't have this, there is no way
+		 * for us to construct this informaion.
+		 */
+		efi_printk(sys_table, PRINTK_PREFIX"ERROR: No 'memory' node in device tree.\n");
+		goto fail_free_fdt;
+	}
+
+	/* Reserve memory for the uncompressed kernel image. This is
+	 * all that prevents any future allocations from conflicting
+	 * with the kernel.  Since we can't tell from the compressed
+	 * image how much DRAM the kernel actually uses (due to BSS
+	 * size uncertainty) we allocate the maximum possible size.
+	 */
+	kernel_reserve_addr = dram_base;
+	kernel_reserve_size = MAX_UNCOMP_KERNEL_SIZE;
+	nr_pages = round_up(kernel_reserve_size, EFI_PAGE_SIZE) / EFI_PAGE_SIZE;
+	status = efi_call_phys4(sys_table->boottime->allocate_pages,
+				EFI_ALLOCATE_ADDRESS, EFI_LOADER_DATA,
+				nr_pages, &kernel_reserve_addr);
+	if (status != EFI_SUCCESS) {
+		efi_printk(sys_table, PRINTK_PREFIX"ERROR: Unable to allocate memory for uncompressed kernel.\n");
+		goto fail_free_fdt;
+	}
+
+	/* Relocate the zImage, if required.  ARM doesn't have a
+	 * preferred address, so we set it to 0, as we want to allocate
+	 * as low in memory as possible.
+	 */
+	zimage_size = image->image_size;
+	status = efi_relocate_kernel(sys_table, zimage_addr, zimage_size,
+				     zimage_size, 0, 0);
+	if (status != EFI_SUCCESS) {
+		efi_printk(sys_table, PRINTK_PREFIX"ERROR: Failed to relocate kernel.\n");
+		goto fail_free_kernel_reserve;
+	}
+
+	/* Check to see if we were able to allocate memory low enough
+	 * in memory.
+	 */
+	if (*zimage_addr + zimage_size > dram_base + ZIMAGE_OFFSET_LIMIT) {
+		efi_printk(sys_table, PRINTK_PREFIX"ERROR: Failed to relocate kernel, no low memory available.\n");
+		goto fail_free_zimage;
+	}
+	status = handle_cmdline_files(sys_table, image, cmdline_ptr, "initrd=",
+				      dram_base + ZIMAGE_OFFSET_LIMIT,
+				      &initrd_addr, &initrd_size);
+	if (status != EFI_SUCCESS) {
+		efi_printk(sys_table, PRINTK_PREFIX"ERROR: Unable to load initrd.\n");
+		goto fail_free_zimage;
+	}
+
+	/* Estimate size of new FDT, and allocate memory for it. We
+	 * will allocate a bigger buffer if this ends up being too
+	 * small, so a rough guess is OK here.*/
+	new_fdt_size = fdt_size + cmdline_size + 0x800;
+	while (1) {
+		status = efi_high_alloc(sys_table, new_fdt_size, 0,
+					&new_fdt_addr,
+					dram_base + ZIMAGE_OFFSET_LIMIT);
+		if (status != EFI_SUCCESS) {
+			efi_printk(sys_table, PRINTK_PREFIX"ERROR: Unable to allocate memory for new device tree.\n");
+			goto fail_free_initrd;
+		}
+
+		/* Now that we have done our final memory allocation (and free)
+		 * we can get the memory map key  needed for
+		 * exit_boot_services().
+		 */
+		status = efi_get_memory_map(sys_table, &memory_map, &map_size,
+					    &desc_size, &desc_ver, &mmap_key);
+		if (status != EFI_SUCCESS)
+			goto fail_free_new_fdt;
+
+		status = update_fdt(sys_table,
+				    fdt, (void *)new_fdt_addr, new_fdt_size,
+				    cmdline_ptr,
+				    initrd_addr, initrd_size,
+				    memory_map, map_size, desc_size, desc_ver);
+
+		/* Succeeding the first time is the expected case. */
+		if (status == EFI_SUCCESS)
+			break;
+
+		if (status == EFI_BUFFER_TOO_SMALL) {
+			/* We need to allocate more space for the new
+			 * device tree, so free existing buffer that is
+			 * too small.  Also free memory map, as we will need
+			 * to get new one that reflects the free/alloc we do
+			 * on the device tree buffer. */
+			efi_free(sys_table, new_fdt_size, new_fdt_addr);
+			efi_call_phys1(sys_table->boottime->free_pool,
+				       memory_map);
+			new_fdt_size += new_fdt_size / 4;
+		} else {
+			efi_printk(sys_table, PRINTK_PREFIX"ERROR: Unable to constuct new device tree.\n");
+			goto fail_free_mmap;
+		}
+	}
+
+	/* Now we are ready to exit_boot_services.*/
+	status = efi_call_phys2(sys_table->boottime->exit_boot_services,
+				handle, mmap_key);
+
+	if (status != EFI_SUCCESS) {
+		efi_printk(sys_table, PRINTK_PREFIX"ERROR: Exit boot services failed.\n");
+		goto fail_free_mmap;
+	}
+
+
+	/* Now we need to return the FDT address to the calling
+	 * assembly to this can be used as part of normal boot.
+	 */
+	return new_fdt_addr;
+
+fail_free_mmap:
+	efi_call_phys1(sys_table->boottime->free_pool, memory_map);
+
+fail_free_new_fdt:
+	efi_free(sys_table, new_fdt_size, new_fdt_addr);
+
+fail_free_initrd:
+	efi_free(sys_table, initrd_size, initrd_addr);
+
+fail_free_zimage:
+	efi_free(sys_table, zimage_size, *zimage_addr);
+
+fail_free_kernel_reserve:
+	efi_free(sys_table, kernel_reserve_size, kernel_reserve_addr);
+
+fail_free_fdt:
+	efi_free(sys_table, fdt_size, fdt_addr);
+
+fail_free_cmdline:
+	efi_free(sys_table, cmdline_size, (u32)cmdline_ptr);
+
+fail:
+	return EFI_STUB_ERROR;
+}
diff --git a/arch/arm/boot/compressed/efi-stub.h b/arch/arm/boot/compressed/efi-stub.h
new file mode 100644
index 0000000..0fe9376
--- /dev/null
+++ b/arch/arm/boot/compressed/efi-stub.h
@@ -0,0 +1,5 @@
+#ifndef _ARM_EFI_STUB_H
+#define _ARM_EFI_STUB_H
+/* Error code returned to ASM code instead of valid FDT address. */
+#define EFI_STUB_ERROR		(~0)
+#endif
diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
index 066b034..eeb394c 100644
--- a/arch/arm/boot/compressed/head.S
+++ b/arch/arm/boot/compressed/head.S
@@ -10,6 +10,7 @@
  */
 #include <linux/linkage.h>
 #include <asm/assembler.h>
+#include "efi-stub.h"
 
 	.arch	armv7-a
 /*
@@ -120,22 +121,93 @@
  */
 		.align
 		.arm				@ Always enter in ARM state
+		.text
 start:
 		.type	start,#function
-		.rept	7
+#ifdef CONFIG_EFI_STUB
+		@ Magic MSDOS signature for PE/COFF + ADD opcode
+		@ the EFI stub only supports little endian, as the EFI functions
+		@ it invokes are little endian.
+		.word	0x62805a4d
+#else
+		mov	r0, r0
+#endif
+		.rept	5
 		mov	r0, r0
 		.endr
-   ARM(		mov	r0, r0		)
-   ARM(		b	1f		)
- THUMB(		adr	r12, BSYM(1f)	)
- THUMB(		bx	r12		)
+
+		adrl	r12, BSYM(zimage_continue)
+ ARM(		mov     pc, r12 )
+ THUMB(		bx	r12     )
+		@ zimage_continue will be in ARM or thumb mode as configured
 
 		.word	0x016f2818		@ Magic numbers to help the loader
 		.word	start			@ absolute load/run zImage address
 		.word	_edata			@ zImage end address
+
+#ifdef CONFIG_EFI_STUB
+		@ Portions of the MSDOS file header must be at offset
+		@ 0x3c from the start of the file.  All PE/COFF headers
+		@ are kept contiguous for simplicity.
+#include "efi-header.S"
+
+efi_stub_entry:
+		@ The EFI stub entry point is not at a fixed address, however
+		@ this address must be set in the PE/COFF header.
+		@ EFI entry point is in A32 mode, switch to T32 if configured.
+ THUMB(		adr	r12, BSYM(1f)	)
+ THUMB(		bx	r12		)
  THUMB(		.thumb			)
 1:
  ARM_BE8(	setend	be )			@ go BE8 if compiled for BE8
+		@ Save lr on stack for possible return to EFI firmware.
+		@ Don't care about fp, but need 64 bit alignment....
+		stmfd	sp!, {fp, lr}
+
+		@ allocate space on stack for passing current zImage address
+		@ and for the EFI stub to return of new entry point of
+		@ zImage, as EFI stub may copy the kernel.  Pointer address
+		@ is passed in r2.  r0 and r1 are passed through from the
+		@ EFI firmware to efi_entry
+		adr	r3, start
+		str	r3, [sp, #-8]!
+		mov	r2, sp			@ pass pointer in r2
+		bl	efi_entry
+		ldr	r3, [sp], #8	@ get new zImage address from stack
+
+		@ Check for error return from EFI stub.  r0 has FDT address
+		@ or EFI_STUB_ERROR error code.
+		cmp	r0, #EFI_STUB_ERROR
+		beq	efi_load_fail
+
+		@ Save return values of efi_entry
+		stmfd	sp!, {r0, r3}
+		bl	cache_clean_flush
+		bl	cache_off
+		ldmfd   sp!, {r0, r3}
+
+		@ Set parameters for booting zImage according to boot protocol
+		@ put FDT address in r2, it was returned by efi_entry()
+		@ r1 is FDT machine type, and r0 needs to be 0
+		mov	r2, r0
+		mov	r1, #0xFFFFFFFF
+		mov	r0, #0
+
+		@ Branch to (possibly) relocated zImage that is in r3
+		@ Make sure we are in A32 mode, as zImage requires
+ THUMB(		bx	r3		)
+ ARM(		mov	pc, r3		)
+
+efi_load_fail:
+		@ Return EFI_LOAD_ERROR to EFI firmware on error.
+		@ Switch back to ARM mode for EFI is done based on
+		@ return address on stack in case we are in THUMB mode
+		ldr	r0, =0x80000001
+		ldmfd	sp!, {fp, pc}		@ put lr from stack into pc
+#endif
+
+ THUMB(		.thumb			)
+zimage_continue:
 		mrs	r9, cpsr
 #ifdef CONFIG_ARM_VIRT_EXT
 		bl	__hyp_stub_install	@ get into SVC mode, reversibly
@@ -168,7 +240,6 @@ not_angel:
 		 * by the linker here, but it should preserve r7, r8, and r9.
 		 */
 
-		.text
 
 #ifdef CONFIG_AUTO_ZRELADDR
 		@ determine final kernel image address
-- 
1.7.10.4

[PATCH V5 5/6] Disable stack protection for decompressor/stub

[Roy Franz]

The ARM decompressor/EFI stub do not implement the functions
(__stack_chk_guard_setup, etc) that are required for support of
stack protection.  The actual enablement of stack protection is
controlled by heuristics in GCC, which the code added for the EFI
stub triggers when CONFIG_STACKPROTECTOR is set.  Even with
CONFIG_STACKPROTECTOR set, the decompressor was never compiled
with stack protection actually enabled. Adding -fno-stack-protector
to the decompressor/stub build keeps it building without stack
protection as it has always been built.
The x86 decompressor/stub is also built with -fno-stack-protector.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 arch/arm/boot/compressed/Makefile |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
index c0c7fee..7974791 100644
--- a/arch/arm/boot/compressed/Makefile
+++ b/arch/arm/boot/compressed/Makefile
@@ -132,7 +132,7 @@ ORIG_CFLAGS := $(KBUILD_CFLAGS)
 KBUILD_CFLAGS = $(subst -pg, , $(ORIG_CFLAGS))
 endif
 
-ccflags-y := -fpic -mno-single-pic-base -fno-builtin -I$(obj)
+ccflags-y := -fpic -mno-single-pic-base -fno-builtin -fno-stack-protector -I$(obj)
 asflags-y := -DZIMAGE
 
 # Supply kernel BSS size to the decompressor via a linker symbol.
-- 
1.7.10.4

[PATCH V5 6/6] Add config EFI_STUB for ARM to Kconfig

[Roy Franz]

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 arch/arm/Kconfig |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index a1b4758..6601985 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1857,6 +1857,16 @@ config EFI
 	  This option is only useful on systems that have UEFI firmware.
 	  However, even with this option, the resultant kernel will
 	  continue to boot on non-UEFI platforms.
+config EFI_STUB
+	bool "EFI stub support"
+	depends on EFI && !CPU_BIG_ENDIAN
+	---help---
+	  This kernel feature allows a zImage to be loaded directly
+	  by EFI firmware without the use of a bootloader.  A PE/COFF
+	  header is added to the zImage in a way that makes the binary
+	  both a Linux zImage and an PE/COFF executable that can be
+	  executed directly by EFI firmware.
+	  See Documentation/efi-stub.txt for more information.
 
 config SECCOMP
 	bool
-- 
1.7.10.4

Re: [PATCH V5 1/6] efi-stub.txt updates for ARM

[Matt Fleming]

On Wed, 27 Nov, at 03:31:50PM, Roy Franz wrote:
> Update efi-stub.txt documentation to be more general
> and not x86 specific.  Add ARM only "dtb=" command
> line option description.
> 
> Signed-off-by: Roy Franz <roy.franz@linaro.org>
> ---
>  Documentation/efi-stub.txt |   27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 

[...]

> -respectively.
> +respectively.  For ARM the EFI stub is implemented in
> +arch/arm/boot/compressed/efi-header.S and
> +arch/arm/boot/compressed/efi-stub.c.  EFI stub code that is shared
> +between architectures is in drivers/firmware/efi/efi-stub-helper.c.
  
No double spaces between full-stop and the beginning of a sentence
please. That convention isn't used anywhere else in this file.

> +For the ARM architecture, we also need to be able to provide a device
> +tree to the kernel.  This is done with the "dtb=" command line option,
> +and is process in the same manner as the "initrd=" option that is described

         ^^ processed ?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH V5 2/6] Add shared update_fdt() function for ARM/ARM64

[Matt Fleming]

On Wed, 27 Nov, at 03:31:51PM, Roy Franz wrote:
> Both ARM and ARM64 stubs will update the device tree
> that they pass to the kernel.  In both cases they
> primarily need to add the same UEFI related information,
> so the function can be shared.
> Create a new FDT related file for this to avoid use
> of architecture #ifdefs in efi-stub-helper.c
> Change EFI allocation type for memory map to
> EFI_RUNTIME_SERVICES_DATA, since we are passing
> this buffer to the kernel, or immediately freeing it.
> 
> Signed-off-by: Roy Franz <roy.franz@linaro.org>
> ---
>  drivers/firmware/efi/efi-stub-helper.c |    9 ++-
>  drivers/firmware/efi/fdt.c             |  115 ++++++++++++++++++++++++++++++++
>  2 files changed, 123 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/firmware/efi/fdt.c
> 
> diff --git a/drivers/firmware/efi/efi-stub-helper.c b/drivers/firmware/efi/efi-stub-helper.c
> index b6bffbf..bc9e37a 100644
> --- a/drivers/firmware/efi/efi-stub-helper.c
> +++ b/drivers/firmware/efi/efi-stub-helper.c
> @@ -4,6 +4,7 @@
>   * implementation files.
>   *
>   * Copyright 2011 Intel Corporation; author Matt Fleming
> + * Copyright 2013 Linaro Limited; author Roy Franz
>   *
>   * This file is part of the Linux kernel, and is made available
>   * under the terms of the GNU General Public License version 2.
> @@ -63,10 +64,16 @@ again:
>  	/*
>  	 * Add an additional efi_memory_desc_t because we're doing an
>  	 * allocation which may be in a new descriptor region.
> +	 * We allocate as EFI_RUNTIME_SERVICES_DATA since this is what
> +	 * we want for when we pass the memory map to the kernel.  This
> +	 * function is also used to get the memory map for other uses,
> +	 * but is always freed by the stub so the allocation type
> +	 * doesn't matter.
>  	 */
>  	*map_size += sizeof(*m);
>  	status = efi_call_phys3(sys_table_arg->boottime->allocate_pool,
> -				EFI_LOADER_DATA, *map_size, (void **)&m);
> +				EFI_RUNTIME_SERVICES_DATA, *map_size,
> +				(void **)&m);
>  	if (status != EFI_SUCCESS)
>  		goto fail;
  

OK, this needs a stronger justification. Presumably the reason for this
change is that you want the allocation to hang around once the kernel is
running? We have this problem on x86 and it's solved by reserving the
memory early in the kernel, e.g. memblock_reserve().
EFI_RUNTIME_SERVICES_DATA is for firmware use, not for kernel data.

> diff --git a/drivers/firmware/efi/fdt.c b/drivers/firmware/efi/fdt.c
> new file mode 100644
> index 0000000..7f1431b
> --- /dev/null
> +++ b/drivers/firmware/efi/fdt.c
> @@ -0,0 +1,115 @@
> +/*
> + * FDT related Helper functions used by the EFI stub on multiple
> + * architectures. This should be #included by the EFI stub
> + * implementation files.
> + *
> + * Copyright 2013 Linaro Limited; author Roy Franz
> + *
> + * This file is part of the Linux kernel, and is made available
> + * under the terms of the GNU General Public License version 2.
> + *
> + */
> +
> +static efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
> +			       void *fdt, int new_fdt_size, char *cmdline_ptr,
> +			       u64 initrd_addr, u64 initrd_size,
> +			       efi_memory_desc_t *memory_map,
> +			       unsigned long map_size, unsigned long desc_size,
> +			       u32 desc_ver)
> +{
> +	int node;
> +	int status;
> +	u32 fdt_val32;
> +	u64 fdt_val64;

Space here before the comment block please.

> +	/* Copy definition of linux_banner here.  Since this code is
> +	 * built as part of the decompressor for ARM v7, pulling
> +	 * in version.c where linux_banner is defined for the
> +	 * kernel brings other kernel dependencies with it.
> +	 */

	/*
	 * This is the multi-line comment format.
	 */

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH V5 2/6] Add shared update_fdt() function for ARM/ARM64

[Mark Salter]

On Fri, 2013-11-29 at 11:30 +0000, Matt Fleming wrote:
> >       /*
> >        * Add an additional efi_memory_desc_t because we're doing an
> >        * allocation which may be in a new descriptor region.
> > +      * We allocate as EFI_RUNTIME_SERVICES_DATA since this is what
> > +      * we want for when we pass the memory map to the kernel.  This
> > +      * function is also used to get the memory map for other uses,
> > +      * but is always freed by the stub so the allocation type
> > +      * doesn't matter.
> >        */
> >       *map_size += sizeof(*m);
> >       status = efi_call_phys3(sys_table_arg->boottime->allocate_pool,
> > -                             EFI_LOADER_DATA, *map_size, (void **)&m);
> > +                             EFI_RUNTIME_SERVICES_DATA, *map_size,
> > +                             (void **)&m);
> >       if (status != EFI_SUCCESS)
> >               goto fail;
>   
> 
> OK, this needs a stronger justification. Presumably the reason for this
> change is that you want the allocation to hang around once the kernel is
> running? We have this problem on x86 and it's solved by reserving the
> memory early in the kernel, e.g. memblock_reserve().
> EFI_RUNTIME_SERVICES_DATA is for firmware use, not for kernel data.
> 

Yeah, arm64 reserves it and is okay with it being EFI_LOADER_DATA.

--Mark

Re: [PATCH V5 2/6] Add shared update_fdt() function for ARM/ARM64

[Roy Franz]

On Mon, Dec 2, 2013 at 6:56 AM, Mark Salter <msalter@redhat.com> wrote:
> On Fri, 2013-11-29 at 11:30 +0000, Matt Fleming wrote:
>> >       /*
>> >        * Add an additional efi_memory_desc_t because we're doing an
>> >        * allocation which may be in a new descriptor region.
>> > +      * We allocate as EFI_RUNTIME_SERVICES_DATA since this is what
>> > +      * we want for when we pass the memory map to the kernel.  This
>> > +      * function is also used to get the memory map for other uses,
>> > +      * but is always freed by the stub so the allocation type
>> > +      * doesn't matter.
>> >        */
>> >       *map_size += sizeof(*m);
>> >       status = efi_call_phys3(sys_table_arg->boottime->allocate_pool,
>> > -                             EFI_LOADER_DATA, *map_size, (void **)&m);
>> > +                             EFI_RUNTIME_SERVICES_DATA, *map_size,
>> > +                             (void **)&m);
>> >       if (status != EFI_SUCCESS)
>> >               goto fail;
>>
>>
>> OK, this needs a stronger justification. Presumably the reason for this
>> change is that you want the allocation to hang around once the kernel is
>> running? We have this problem on x86 and it's solved by reserving the
>> memory early in the kernel, e.g. memblock_reserve().
>> EFI_RUNTIME_SERVICES_DATA is for firmware use, not for kernel data.
>>
>
> Yeah, arm64 reserves it and is okay with it being EFI_LOADER_DATA.
>
> --Mark

I have discussed this with Leif and will change this back to
EFI_LOADER_DATA, and Leif will update
his runtime services patch to reserve this as done in your arm64 implementation.

Thanks,
Roy

Re: [PATCH V5 1/6] efi-stub.txt updates for ARM

[Grant Likely]

On Wed, 27 Nov 2013 15:31:50 -0800, Roy Franz <roy.franz@linaro.org> wrote:
> Update efi-stub.txt documentation to be more general
> and not x86 specific.  Add ARM only "dtb=" command
> line option description.
> 
> Signed-off-by: Roy Franz <roy.franz@linaro.org>

Acked-by: Grant Likely <grant.likely@linaro.org>

> ---
>  Documentation/efi-stub.txt |   27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/efi-stub.txt b/Documentation/efi-stub.txt
> index 44e6bb6..19e897c 100644
> --- a/Documentation/efi-stub.txt
> +++ b/Documentation/efi-stub.txt
> @@ -1,13 +1,16 @@
>  			  The EFI Boot Stub
>  		     ---------------------------
>  
> -On the x86 platform, a bzImage can masquerade as a PE/COFF image,
> -thereby convincing EFI firmware loaders to load it as an EFI
> -executable. The code that modifies the bzImage header, along with the
> -EFI-specific entry point that the firmware loader jumps to are
> -collectively known as the "EFI boot stub", and live in
> +On the x86 and ARM platforms, a kernel zImage/bzImage can masquerade
> +as a PE/COFF image, thereby convincing EFI firmware loaders to load
> +it as an EFI executable. The code that modifies the bzImage header,
> +along with the EFI-specific entry point that the firmware loader
> +jumps to are collectively known as the "EFI boot stub", and live in
>  arch/x86/boot/header.S and arch/x86/boot/compressed/eboot.c,
> -respectively.
> +respectively.  For ARM the EFI stub is implemented in
> +arch/arm/boot/compressed/efi-header.S and
> +arch/arm/boot/compressed/efi-stub.c.  EFI stub code that is shared
> +between architectures is in drivers/firmware/efi/efi-stub-helper.c.
>  
>  By using the EFI boot stub it's possible to boot a Linux kernel
>  without the use of a conventional EFI boot loader, such as grub or
> @@ -23,7 +26,9 @@ The bzImage located in arch/x86/boot/bzImage must be copied to the EFI
>  System Partiion (ESP) and renamed with the extension ".efi". Without
>  the extension the EFI firmware loader will refuse to execute it. It's
>  not possible to execute bzImage.efi from the usual Linux file systems
> -because EFI firmware doesn't have support for them.
> +because EFI firmware doesn't have support for them.  For ARM the
> +arch/arm/boot/zImage should be copied to the system partition, and it
> +may not need to be renamed.
>  
>  
>  **** Passing kernel parameters from the EFI shell
> @@ -63,3 +68,11 @@ Notice how bzImage.efi can be specified with a relative path. That's
>  because the image we're executing is interpreted by the EFI shell,
>  which understands relative paths, whereas the rest of the command line
>  is passed to bzImage.efi.
> +
> +
> +**** The "dtb=" option
> +
> +For the ARM architecture, we also need to be able to provide a device
> +tree to the kernel.  This is done with the "dtb=" command line option,
> +and is process in the same manner as the "initrd=" option that is described
> +above.
> -- 
> 1.7.10.4
> 

Re: [PATCH V5 2/6] Add shared update_fdt() function for ARM/ARM64

[Grant Likely]

On Wed, 27 Nov 2013 15:31:51 -0800, Roy Franz <roy.franz@linaro.org> wrote:
> Both ARM and ARM64 stubs will update the device tree
> that they pass to the kernel.  In both cases they
> primarily need to add the same UEFI related information,
> so the function can be shared.
> Create a new FDT related file for this to avoid use
> of architecture #ifdefs in efi-stub-helper.c
> Change EFI allocation type for memory map to
> EFI_RUNTIME_SERVICES_DATA, since we are passing
> this buffer to the kernel, or immediately freeing it.
> 
> Signed-off-by: Roy Franz <roy.franz@linaro.org>

Hi Roy,

Comments below...

> +static efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
> +			       void *fdt, int new_fdt_size, char *cmdline_ptr,
> +			       u64 initrd_addr, u64 initrd_size,
> +			       efi_memory_desc_t *memory_map,
> +			       unsigned long map_size, unsigned long desc_size,
> +			       u32 desc_ver)
> +{
> +	int node;
> +	int status;
> +	u32 fdt_val32;
> +	u64 fdt_val64;
> +	/* Copy definition of linux_banner here.  Since this code is
> +	 * built as part of the decompressor for ARM v7, pulling
> +	 * in version.c where linux_banner is defined for the
> +	 * kernel brings other kernel dependencies with it.
> +	 */
> +	const char linux_banner[] =
> +	    "Linux version " UTS_RELEASE " (" LINUX_COMPILE_BY "@"
> +	    LINUX_COMPILE_HOST ") (" LINUX_COMPILER ") " UTS_VERSION "\n";

This is inherently fragile because the string is now defined in two
places. This issue shouldn't hold off merging the patch, but a follow up
patch should be crafted to make it possible to pull in version.c without
the other dependencies....

Then again, the version.c strings have a very big "DON'T TOUCH" warning
on them so I'm probably worrying about nothing.  :-)

> +
> +	status = fdt_open_into(orig_fdt, fdt, new_fdt_size);
> +	if (status != 0)
> +		goto fdt_set_fail;
> +
> +	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;
> +		}
> +	}
> +
> +	if ((cmdline_ptr != NULL) && (strlen(cmdline_ptr) > 0)) {
> +		status = fdt_setprop(fdt, node, "bootargs", cmdline_ptr,
> +				     strlen(cmdline_ptr) + 1);
> +		if (status)
> +			goto fdt_set_fail;
> +	}
> +
> +	/* Set intird address/end in device tree, if present */

sp. initrd

None of the above comments are blockers though.

Acked-by: Grant Likely <grant.likely@linaro.org>

Re: [PATCH V5 2/6] Add shared update_fdt() function for ARM/ARM64

[Grant Likely]

On Wed, 27 Nov 2013 15:31:51 -0800, Roy Franz <roy.franz@linaro.org> wrote:
> Both ARM and ARM64 stubs will update the device tree
> that they pass to the kernel.  In both cases they
> primarily need to add the same UEFI related information,
> so the function can be shared.
> Create a new FDT related file for this to avoid use
> of architecture #ifdefs in efi-stub-helper.c
> Change EFI allocation type for memory map to
> EFI_RUNTIME_SERVICES_DATA, since we are passing
> this buffer to the kernel, or immediately freeing it.

Nit: You're using a very short line length for the commit block. 72
columns is more customary.

g.

Re: [PATCH V5 4/6] Add EFI stub for ARM

[Grant Likely]

On Wed, 27 Nov 2013 15:31:53 -0800, Roy Franz <roy.franz@linaro.org> wrote:
> This patch adds EFI stub support for the ARM Linux kernel.  The EFI stub
> operates similarly to the x86 stub: it is a shim between the EFI firmware
> and the normal zImage entry point, and sets up the environment that the
> zImage is expecting.  This includes loading the initrd (optionaly) and
> device tree from the system partition based on the kernel command line.
> The stub updates the device tree as necessary, adding entries for EFI
> runtime services. The PE/COFF "MZ" header at offset 0 results in the
> first instruction being an add that corrupts r5, which is not used by
> the zImage interface.
> 
> Signed-off-by: Roy Franz <roy.franz@linaro.org>

I'm going to review this patch side-by-side with the ARM64 patch written
by Mark. There are things that can still be consolidated. Otherwise the
patch looks good.

> ---
> diff --git a/arch/arm/boot/compressed/efi-stub.c b/arch/arm/boot/compressed/efi-stub.c
> new file mode 100644
> index 0000000..2bd559d
> --- /dev/null
> +++ b/arch/arm/boot/compressed/efi-stub.c
> @@ -0,0 +1,291 @@
> +/*
> + * linux/arch/arm/boot/compressed/efi-stub.c
> + *
> + * Copyright (C) 2013 Linaro Ltd;  <roy.franz@linaro.org>
> + *
> + * This file implements the EFI boot stub for the ARM kernel
> + *
> + * 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.
> + *
> + */
> +#include <linux/efi.h>
> +#include <libfdt.h>
> +#include "efi-stub.h"
> +
> +/* EFI function call wrappers.  These are not required for
> + * ARM, but wrappers are required for X86 to convert between
> + * ABIs.  These wrappers are provided to allow code sharing
> + * between X86 and ARM.  Since these wrappers directly invoke the
> + * EFI function pointer, the function pointer type must be properly
> + * defined, which is not the case for X86  One advantage of this is
> + * it allows for type checking of arguments, which is not
> + * possible with the X86 wrappers.
> + */
> +#define efi_call_phys0(f)			f()
> +#define efi_call_phys1(f, a1)			f(a1)
> +#define efi_call_phys2(f, a1, a2)		f(a1, a2)
> +#define efi_call_phys3(f, a1, a2, a3)		f(a1, a2, a3)
> +#define efi_call_phys4(f, a1, a2, a3, a4)	f(a1, a2, a3, a4)
> +#define efi_call_phys5(f, a1, a2, a3, a4, a5)	f(a1, a2, a3, a4, a5)

Identical to ARM64. I would put this into a common header usable by
platforms that can call the functions directly.

> +
> +/* The maximum uncompressed kernel size is 32 MBytes, so we will reserve
> + * that for the decompressed kernel.  We have no easy way to tell what
> + * the actuall size of code + data the uncompressed kernel will use.
> + */
> +#define MAX_UNCOMP_KERNEL_SIZE	0x02000000
> +
> +/* The kernel zImage should be located between 32 Mbytes
> + * and 128 MBytes from the base of DRAM.  The min
> + * address leaves space for a maximal size uncompressed image,
> + * and the max address is due to how the zImage decompressor
> + * picks a destination address.
> + */
> +#define ZIMAGE_OFFSET_LIMIT	0x08000000
> +#define MIN_ZIMAGE_OFFSET	MAX_UNCOMP_KERNEL_SIZE
> +
> +#define PRINTK_PREFIX		"EFIstub: "
> +
> +struct fdt_region {
> +	u64 base;
> +	u64 size;
> +};

Identical to ARM64; move to header.

> +
> +
> +/* Include shared EFI stub code, and required headers. */
> +#include "../../../../include/generated/compile.h"
> +#include "../../../../include/generated/utsrelease.h"
> +#include "../../../../drivers/firmware/efi/efi-stub-helper.c"
> +#include "../../../../drivers/firmware/efi/fdt.c"
> +
> +
> +int efi_entry(void *handle, efi_system_table_t *sys_table,
> +	      unsigned long *zimage_addr)
> +{
> +	efi_loaded_image_t *image;
> +	int status;
> +	unsigned long nr_pages;
> +	const struct fdt_region *region;
> +
> +	void *fdt;
> +	int err;
> +	int node;
> +	unsigned long zimage_size = 0;
> +	unsigned long dram_base;
> +	/* addr/point and size pairs for memory management*/
> +	unsigned long initrd_addr;
> +	unsigned long initrd_size = 0;
> +	unsigned long fdt_addr;
> +	unsigned long fdt_size = 0;
> +	efi_physical_addr_t kernel_reserve_addr;
> +	unsigned long kernel_reserve_size = 0;
> +	char *cmdline_ptr;
> +	int cmdline_size = 0;
> +
> +	unsigned long map_size, desc_size;
> +	u32 desc_ver;
> +	unsigned long mmap_key;
> +	efi_memory_desc_t *memory_map;
> +
> +	unsigned long new_fdt_size;
> +	unsigned long new_fdt_addr;
> +
> +	efi_guid_t proto = LOADED_IMAGE_PROTOCOL_GUID;
> +
> +	/* Check if we were booted by the EFI firmware */
> +	if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
> +		goto fail;
> +
> +	efi_printk(sys_table, PRINTK_PREFIX"Booting Linux using EFI stub.\n");

Gratuitously different between ARM and ARM64. efi_printk() vs. pr_efi()
and the string is different.

> +	/* get the command line from EFI, using the LOADED_IMAGE protocol */
> +	status = efi_call_phys3(sys_table->boottime->handle_protocol,
> +				handle, &proto, (void *)&image);
> +	if (status != EFI_SUCCESS) {
> +		efi_printk(sys_table, PRINTK_PREFIX"ERROR: Failed to get handle for LOADED_IMAGE_PROTOCOL.\n");
> +		goto fail;
> +	}
> +
> +	/* We are going to copy the command line into the device tree,
> +	 * so this memory just needs to not conflict with boot protocol
> +	 * requirements.
> +	 */
> +	cmdline_ptr = efi_convert_cmdline_to_ascii(sys_table, image,
> +						   &cmdline_size);
> +	if (!cmdline_ptr) {
> +		efi_printk(sys_table, PRINTK_PREFIX"ERROR: Unable to allocate memory for command line.\n");
> +		goto fail;
> +	}
> +
> +	/* We first load the device tree, as we need to get the base address of
> +	 * DRAM from the device tree.  The zImage, device tree, and initrd
> +	 * have address restrictions that are relative to the base of DRAM.
> +	 */
> +	status = handle_cmdline_files(sys_table, image, cmdline_ptr, "dtb=",
> +				      0xffffffff, &fdt_addr, &fdt_size);
> +	if (status != EFI_SUCCESS) {
> +		efi_printk(sys_table, PRINTK_PREFIX"ERROR: Unable to load device tree blob.\n");
> +		goto fail_free_cmdline;
> +	}
> +
> +	err = fdt_check_header((void *)fdt_addr);
> +	if (err != 0) {
> +		efi_printk(sys_table, PRINTK_PREFIX"ERROR: Device tree header not valid.\n");
> +		goto fail_free_fdt;
> +	}
> +	if (fdt_totalsize((void *)fdt_addr) > fdt_size) {
> +		efi_printk(sys_table, PRINTK_PREFIX"ERROR: Incomplete device tree.\n");
> +		goto fail_free_fdt;
> +
> +	}

All of the above is basically identical.

> +	/* Look up the base of DRAM from the device tree. */
> +	fdt = (void *)fdt_addr;
> +	node = fdt_subnode_offset(fdt, 0, "memory");
> +	region = fdt_getprop(fdt, node, "reg", NULL);
> +	if (region) {
> +		dram_base = fdt64_to_cpu(region->base);
> +	} else {
> +		/* There is no way to get amount or addresses of physical
> +		 * memory installed using EFI calls.  If the device tree
> +		 * we read from disk doesn't have this, there is no way
> +		 * for us to construct this informaion.
> +		 */
> +		efi_printk(sys_table, PRINTK_PREFIX"ERROR: No 'memory' node in device tree.\n");
> +		goto fail_free_fdt;
> +	}

ARM and ARM64 differ here. ARM64 uses get_dram_base(), this open codes
some stuff.

> +
> +	/* Reserve memory for the uncompressed kernel image. This is
> +	 * all that prevents any future allocations from conflicting
> +	 * with the kernel.  Since we can't tell from the compressed
> +	 * image how much DRAM the kernel actually uses (due to BSS
> +	 * size uncertainty) we allocate the maximum possible size.
> +	 */
> +	kernel_reserve_addr = dram_base;
> +	kernel_reserve_size = MAX_UNCOMP_KERNEL_SIZE;
> +	nr_pages = round_up(kernel_reserve_size, EFI_PAGE_SIZE) / EFI_PAGE_SIZE;
> +	status = efi_call_phys4(sys_table->boottime->allocate_pages,
> +				EFI_ALLOCATE_ADDRESS, EFI_LOADER_DATA,
> +				nr_pages, &kernel_reserve_addr);
> +	if (status != EFI_SUCCESS) {
> +		efi_printk(sys_table, PRINTK_PREFIX"ERROR: Unable to allocate memory for uncompressed kernel.\n");
> +		goto fail_free_fdt;
> +	}

This is ARM only since arm64 doesn't have a compressed version. It would
be possible to stub out.

> +
> +	/* Relocate the zImage, if required.  ARM doesn't have a
> +	 * preferred address, so we set it to 0, as we want to allocate
> +	 * as low in memory as possible.
> +	 */
> +	zimage_size = image->image_size;
> +	status = efi_relocate_kernel(sys_table, zimage_addr, zimage_size,
> +				     zimage_size, 0, 0);
> +	if (status != EFI_SUCCESS) {
> +		efi_printk(sys_table, PRINTK_PREFIX"ERROR: Failed to relocate kernel.\n");
> +		goto fail_free_kernel_reserve;
> +	}

Slight differences here.

> +
> +	/* Check to see if we were able to allocate memory low enough
> +	 * in memory.
> +	 */
> +	if (*zimage_addr + zimage_size > dram_base + ZIMAGE_OFFSET_LIMIT) {
> +		efi_printk(sys_table, PRINTK_PREFIX"ERROR: Failed to relocate kernel, no low memory available.\n");
> +		goto fail_free_zimage;
> +	}

Back to same code below this point, and is the same through to the
bottom of the function. That is a lot of identical code. I think you can
generalize the whole function with arch specific callouts in a couple of
places.

I really don't want to hold up the merging of this series. Aside from
the duplication I think it is ready to be merged. However, the
duplication is so obvious that I'm not comfortable with it. If I were
an ARM or ARM64 core maintainer then I would push back on it.

Go ahead an add my Acked-by for this patch. I'll support merging it as
is provided that you promise me to merge the two versions with a
follow-up patch ASAP. However, if you can I would still recommend
respinning the series with the common bits split out right away since
there's a much greater chance of getting the arm and arm64 maintainers
to accept them.

Acked-by: Grant Likely <grant.likely@linaro.org>

g.

Re: [PATCH V5 6/6] Add config EFI_STUB for ARM to Kconfig

[Grant Likely]

On Wed, 27 Nov 2013 15:31:55 -0800, Roy Franz <roy.franz@linaro.org> wrote:
> Signed-off-by: Roy Franz <roy.franz@linaro.org>

Dude. Must have commit text. You know better.  :-)

> ---
>  arch/arm/Kconfig |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index a1b4758..6601985 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1857,6 +1857,16 @@ config EFI
>  	  This option is only useful on systems that have UEFI firmware.
>  	  However, even with this option, the resultant kernel will
>  	  continue to boot on non-UEFI platforms.
> +config EFI_STUB

Need a blank line between the previous stanza and config EFI_STUB

> +	bool "EFI stub support"
> +	depends on EFI && !CPU_BIG_ENDIAN
> +	---help---
> +	  This kernel feature allows a zImage to be loaded directly
> +	  by EFI firmware without the use of a bootloader.  A PE/COFF
> +	  header is added to the zImage in a way that makes the binary
> +	  both a Linux zImage and an PE/COFF executable that can be
> +	  executed directly by EFI firmware.
> +	  See Documentation/efi-stub.txt for more information.

Nit: 80 column word wrap is customary.

g.

Re: [PATCH V5 2/6] Add shared update_fdt() function for ARM/ARM64

[Roy Franz]

On Thu, Dec 5, 2013 at 11:24 AM, Grant Likely <grant.likely@linaro.org> wrote:
> On Wed, 27 Nov 2013 15:31:51 -0800, Roy Franz <roy.franz@linaro.org> wrote:
>> Both ARM and ARM64 stubs will update the device tree
>> that they pass to the kernel.  In both cases they
>> primarily need to add the same UEFI related information,
>> so the function can be shared.
>> Create a new FDT related file for this to avoid use
>> of architecture #ifdefs in efi-stub-helper.c
>> Change EFI allocation type for memory map to
>> EFI_RUNTIME_SERVICES_DATA, since we are passing
>> this buffer to the kernel, or immediately freeing it.
>>
>> Signed-off-by: Roy Franz <roy.franz@linaro.org>
>
> Hi Roy,
>
> Comments below...
>
>> +static efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
>> +                            void *fdt, int new_fdt_size, char *cmdline_ptr,
>> +                            u64 initrd_addr, u64 initrd_size,
>> +                            efi_memory_desc_t *memory_map,
>> +                            unsigned long map_size, unsigned long desc_size,
>> +                            u32 desc_ver)
>> +{
>> +     int node;
>> +     int status;
>> +     u32 fdt_val32;
>> +     u64 fdt_val64;
>> +     /* Copy definition of linux_banner here.  Since this code is
>> +      * built as part of the decompressor for ARM v7, pulling
>> +      * in version.c where linux_banner is defined for the
>> +      * kernel brings other kernel dependencies with it.
>> +      */
>> +     const char linux_banner[] =
>> +         "Linux version " UTS_RELEASE " (" LINUX_COMPILE_BY "@"
>> +         LINUX_COMPILE_HOST ") (" LINUX_COMPILER ") " UTS_VERSION "\n";
>
> This is inherently fragile because the string is now defined in two
> places. This issue shouldn't hold off merging the patch, but a follow up
> patch should be crafted to make it possible to pull in version.c without
> the other dependencies....
>
> Then again, the version.c strings have a very big "DON'T TOUCH" warning
> on them so I'm probably worrying about nothing.  :-)

I wasn't really happy about copying the definition here, but as this
will be verified by UEFI
code in the kernel if this every is changed it should not be much of a
problem in practice.
I can take a look at what changes would be required for using
version.c here.  I haven't worked
out what would need to be done, but was hesitant to make changes there
to support a one-off use
of the kernel version string outside of the kernel (ie in the
decompressor/stub), and was trying to keep
this patchset limited to changing arm/EFI code.

>
>> +
>> +     status = fdt_open_into(orig_fdt, fdt, new_fdt_size);
>> +     if (status != 0)
>> +             goto fdt_set_fail;
>> +
>> +     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;
>> +             }
>> +     }
>> +
>> +     if ((cmdline_ptr != NULL) && (strlen(cmdline_ptr) > 0)) {
>> +             status = fdt_setprop(fdt, node, "bootargs", cmdline_ptr,
>> +                                  strlen(cmdline_ptr) + 1);
>> +             if (status)
>> +                     goto fdt_set_fail;
>> +     }
>> +
>> +     /* Set intird address/end in device tree, if present */
>
> sp. initrd
>
> None of the above comments are blockers though.
>
> Acked-by: Grant Likely <grant.likely@linaro.org>
>

Thanks,
Roy

Re: [PATCH V5 4/6] Add EFI stub for ARM

[Roy Franz]

On Thu, Dec 5, 2013 at 12:00 PM, Grant Likely <grant.likely@linaro.org> wrote:
> On Wed, 27 Nov 2013 15:31:53 -0800, Roy Franz <roy.franz@linaro.org> wrote:
>> This patch adds EFI stub support for the ARM Linux kernel.  The EFI stub
>> operates similarly to the x86 stub: it is a shim between the EFI firmware
>> and the normal zImage entry point, and sets up the environment that the
>> zImage is expecting.  This includes loading the initrd (optionaly) and
>> device tree from the system partition based on the kernel command line.
>> The stub updates the device tree as necessary, adding entries for EFI
>> runtime services. The PE/COFF "MZ" header at offset 0 results in the
>> first instruction being an add that corrupts r5, which is not used by
>> the zImage interface.
>>
>> Signed-off-by: Roy Franz <roy.franz@linaro.org>
>
> I'm going to review this patch side-by-side with the ARM64 patch written
> by Mark. There are things that can still be consolidated. Otherwise the
> patch looks good.
>
>> ---
>> diff --git a/arch/arm/boot/compressed/efi-stub.c b/arch/arm/boot/compressed/efi-stub.c
>> new file mode 100644
>> index 0000000..2bd559d
>> --- /dev/null
>> +++ b/arch/arm/boot/compressed/efi-stub.c
>> @@ -0,0 +1,291 @@
>> +/*
>> + * linux/arch/arm/boot/compressed/efi-stub.c
>> + *
>> + * Copyright (C) 2013 Linaro Ltd;  <roy.franz@linaro.org>
>> + *
>> + * This file implements the EFI boot stub for the ARM kernel
>> + *
>> + * 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.
>> + *
>> + */
>> +#include <linux/efi.h>
>> +#include <libfdt.h>
>> +#include "efi-stub.h"
>> +
>> +/* EFI function call wrappers.  These are not required for
>> + * ARM, but wrappers are required for X86 to convert between
>> + * ABIs.  These wrappers are provided to allow code sharing
>> + * between X86 and ARM.  Since these wrappers directly invoke the
>> + * EFI function pointer, the function pointer type must be properly
>> + * defined, which is not the case for X86  One advantage of this is
>> + * it allows for type checking of arguments, which is not
>> + * possible with the X86 wrappers.
>> + */
>> +#define efi_call_phys0(f)                    f()
>> +#define efi_call_phys1(f, a1)                        f(a1)
>> +#define efi_call_phys2(f, a1, a2)            f(a1, a2)
>> +#define efi_call_phys3(f, a1, a2, a3)                f(a1, a2, a3)
>> +#define efi_call_phys4(f, a1, a2, a3, a4)    f(a1, a2, a3, a4)
>> +#define efi_call_phys5(f, a1, a2, a3, a4, a5)        f(a1, a2, a3, a4, a5)
>
> Identical to ARM64. I would put this into a common header usable by
> platforms that can call the functions directly.
>
>> +
>> +/* The maximum uncompressed kernel size is 32 MBytes, so we will reserve
>> + * that for the decompressed kernel.  We have no easy way to tell what
>> + * the actuall size of code + data the uncompressed kernel will use.
>> + */
>> +#define MAX_UNCOMP_KERNEL_SIZE       0x02000000
>> +
>> +/* The kernel zImage should be located between 32 Mbytes
>> + * and 128 MBytes from the base of DRAM.  The min
>> + * address leaves space for a maximal size uncompressed image,
>> + * and the max address is due to how the zImage decompressor
>> + * picks a destination address.
>> + */
>> +#define ZIMAGE_OFFSET_LIMIT  0x08000000
>> +#define MIN_ZIMAGE_OFFSET    MAX_UNCOMP_KERNEL_SIZE
>> +
>> +#define PRINTK_PREFIX                "EFIstub: "
>> +
>> +struct fdt_region {
>> +     u64 base;
>> +     u64 size;
>> +};
>
> Identical to ARM64; move to header.
>
>> +
>> +
>> +/* Include shared EFI stub code, and required headers. */
>> +#include "../../../../include/generated/compile.h"
>> +#include "../../../../include/generated/utsrelease.h"
>> +#include "../../../../drivers/firmware/efi/efi-stub-helper.c"
>> +#include "../../../../drivers/firmware/efi/fdt.c"
>> +
>> +
>> +int efi_entry(void *handle, efi_system_table_t *sys_table,
>> +           unsigned long *zimage_addr)
>> +{
>> +     efi_loaded_image_t *image;
>> +     int status;
>> +     unsigned long nr_pages;
>> +     const struct fdt_region *region;
>> +
>> +     void *fdt;
>> +     int err;
>> +     int node;
>> +     unsigned long zimage_size = 0;
>> +     unsigned long dram_base;
>> +     /* addr/point and size pairs for memory management*/
>> +     unsigned long initrd_addr;
>> +     unsigned long initrd_size = 0;
>> +     unsigned long fdt_addr;
>> +     unsigned long fdt_size = 0;
>> +     efi_physical_addr_t kernel_reserve_addr;
>> +     unsigned long kernel_reserve_size = 0;
>> +     char *cmdline_ptr;
>> +     int cmdline_size = 0;
>> +
>> +     unsigned long map_size, desc_size;
>> +     u32 desc_ver;
>> +     unsigned long mmap_key;
>> +     efi_memory_desc_t *memory_map;
>> +
>> +     unsigned long new_fdt_size;
>> +     unsigned long new_fdt_addr;
>> +
>> +     efi_guid_t proto = LOADED_IMAGE_PROTOCOL_GUID;
>> +
>> +     /* Check if we were booted by the EFI firmware */
>> +     if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
>> +             goto fail;
>> +
>> +     efi_printk(sys_table, PRINTK_PREFIX"Booting Linux using EFI stub.\n");
>
> Gratuitously different between ARM and ARM64. efi_printk() vs. pr_efi()
> and the string is different.
>
>> +     /* get the command line from EFI, using the LOADED_IMAGE protocol */
>> +     status = efi_call_phys3(sys_table->boottime->handle_protocol,
>> +                             handle, &proto, (void *)&image);
>> +     if (status != EFI_SUCCESS) {
>> +             efi_printk(sys_table, PRINTK_PREFIX"ERROR: Failed to get handle for LOADED_IMAGE_PROTOCOL.\n");
>> +             goto fail;
>> +     }
>> +
>> +     /* We are going to copy the command line into the device tree,
>> +      * so this memory just needs to not conflict with boot protocol
>> +      * requirements.
>> +      */
>> +     cmdline_ptr = efi_convert_cmdline_to_ascii(sys_table, image,
>> +                                                &cmdline_size);
>> +     if (!cmdline_ptr) {
>> +             efi_printk(sys_table, PRINTK_PREFIX"ERROR: Unable to allocate memory for command line.\n");
>> +             goto fail;
>> +     }
>> +
>> +     /* We first load the device tree, as we need to get the base address of
>> +      * DRAM from the device tree.  The zImage, device tree, and initrd
>> +      * have address restrictions that are relative to the base of DRAM.
>> +      */
>> +     status = handle_cmdline_files(sys_table, image, cmdline_ptr, "dtb=",
>> +                                   0xffffffff, &fdt_addr, &fdt_size);
>> +     if (status != EFI_SUCCESS) {
>> +             efi_printk(sys_table, PRINTK_PREFIX"ERROR: Unable to load device tree blob.\n");
>> +             goto fail_free_cmdline;
>> +     }
>> +
>> +     err = fdt_check_header((void *)fdt_addr);
>> +     if (err != 0) {
>> +             efi_printk(sys_table, PRINTK_PREFIX"ERROR: Device tree header not valid.\n");
>> +             goto fail_free_fdt;
>> +     }
>> +     if (fdt_totalsize((void *)fdt_addr) > fdt_size) {
>> +             efi_printk(sys_table, PRINTK_PREFIX"ERROR: Incomplete device tree.\n");
>> +             goto fail_free_fdt;
>> +
>> +     }
>
> All of the above is basically identical.
>
>> +     /* Look up the base of DRAM from the device tree. */
>> +     fdt = (void *)fdt_addr;
>> +     node = fdt_subnode_offset(fdt, 0, "memory");
>> +     region = fdt_getprop(fdt, node, "reg", NULL);
>> +     if (region) {
>> +             dram_base = fdt64_to_cpu(region->base);
>> +     } else {
>> +             /* There is no way to get amount or addresses of physical
>> +              * memory installed using EFI calls.  If the device tree
>> +              * we read from disk doesn't have this, there is no way
>> +              * for us to construct this informaion.
>> +              */
>> +             efi_printk(sys_table, PRINTK_PREFIX"ERROR: No 'memory' node in device tree.\n");
>> +             goto fail_free_fdt;
>> +     }
>
> ARM and ARM64 differ here. ARM64 uses get_dram_base(), this open codes
> some stuff.
>
>> +
>> +     /* Reserve memory for the uncompressed kernel image. This is
>> +      * all that prevents any future allocations from conflicting
>> +      * with the kernel.  Since we can't tell from the compressed
>> +      * image how much DRAM the kernel actually uses (due to BSS
>> +      * size uncertainty) we allocate the maximum possible size.
>> +      */
>> +     kernel_reserve_addr = dram_base;
>> +     kernel_reserve_size = MAX_UNCOMP_KERNEL_SIZE;
>> +     nr_pages = round_up(kernel_reserve_size, EFI_PAGE_SIZE) / EFI_PAGE_SIZE;
>> +     status = efi_call_phys4(sys_table->boottime->allocate_pages,
>> +                             EFI_ALLOCATE_ADDRESS, EFI_LOADER_DATA,
>> +                             nr_pages, &kernel_reserve_addr);
>> +     if (status != EFI_SUCCESS) {
>> +             efi_printk(sys_table, PRINTK_PREFIX"ERROR: Unable to allocate memory for uncompressed kernel.\n");
>> +             goto fail_free_fdt;
>> +     }
>
> This is ARM only since arm64 doesn't have a compressed version. It would
> be possible to stub out.
>
>> +
>> +     /* Relocate the zImage, if required.  ARM doesn't have a
>> +      * preferred address, so we set it to 0, as we want to allocate
>> +      * as low in memory as possible.
>> +      */
>> +     zimage_size = image->image_size;
>> +     status = efi_relocate_kernel(sys_table, zimage_addr, zimage_size,
>> +                                  zimage_size, 0, 0);
>> +     if (status != EFI_SUCCESS) {
>> +             efi_printk(sys_table, PRINTK_PREFIX"ERROR: Failed to relocate kernel.\n");
>> +             goto fail_free_kernel_reserve;
>> +     }
>
> Slight differences here.
>
>> +
>> +     /* Check to see if we were able to allocate memory low enough
>> +      * in memory.
>> +      */
>> +     if (*zimage_addr + zimage_size > dram_base + ZIMAGE_OFFSET_LIMIT) {
>> +             efi_printk(sys_table, PRINTK_PREFIX"ERROR: Failed to relocate kernel, no low memory available.\n");
>> +             goto fail_free_zimage;
>> +     }
>
> Back to same code below this point, and is the same through to the
> bottom of the function. That is a lot of identical code. I think you can
> generalize the whole function with arch specific callouts in a couple of
> places.
>
> I really don't want to hold up the merging of this series. Aside from
> the duplication I think it is ready to be merged. However, the
> duplication is so obvious that I'm not comfortable with it. If I were
> an ARM or ARM64 core maintainer then I would push back on it.
>
> Go ahead an add my Acked-by for this patch. I'll support merging it as
> is provided that you promise me to merge the two versions with a
> follow-up patch ASAP. However, if you can I would still recommend
> respinning the series with the common bits split out right away since
> there's a much greater chance of getting the arm and arm64 maintainers
> to accept them.
>
> Acked-by: Grant Likely <grant.likely@linaro.org>
>
> g.

Hi Grant,

   You're not the first to bring this up, and I've discussed with Mark
making more of the code common.
My next series will pull more code into the shared files, so this will
be at least partly addressed then.  Further
consolidation if required will be much easier after both are merged.

Roy