[PATCH v3 0/3] (U)EFI runtime services for arm

[PATCH v3 0/3] (U)EFI runtime services for arm

[Leif Lindholm]

In systems based on [U]EFI-conformant firmware, runtime services provide
a standardised way for the kernel to update firmware environment
variables. This is used for example by efibootmgr to update which image
should be loaded on next boot.

This patchset implements basic support for UEFI runtime services on ARM
platforms, as well as the basic underlying EFI support. It also defines
a mechanism by which the required information is passed from the
bootloader (the EFI stub submitted separately) to the kernel via FDT
entries.

This patchset depends on the presence of early_ioremap(). It has been
validated against Mark Salter's generic implementation.

Changes from v2:
- Updated FDT bindings.
- The EFI stub is now the only loader that will support the kernel
  enabling runtime services.
- Documentation updates.

Changes from v1:
- Updated FDT bindings, based on feedback.
- Use common config table scanning and address lookup code.
- Add dependency on !CPU_BIG_ENDIAN (for now).
- Add proper efi_enabled() facility.
- Documentation updates.


Leif Lindholm (3):
  Documentation: arm: add UEFI support documentation
  arm: Add [U]EFI runtime services support
  init: efi: arm: enable (U)EFI runtime services on arm

 Documentation/arm/00-INDEX  |    3 +
 Documentation/arm/uefi.txt  |   61 ++++++
 arch/arm/Kconfig            |   15 ++
 arch/arm/include/asm/uefi.h |   22 ++
 arch/arm/kernel/Makefile    |    2 +
 arch/arm/kernel/setup.c     |    6 +
 arch/arm/kernel/uefi.c      |  469 +++++++++++++++++++++++++++++++++++++++++++
 arch/arm/kernel/uefi_phys.S |   59 ++++++
 include/linux/efi.h         |    2 +-
 init/main.c                 |    4 +
 10 files changed, 642 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/arm/uefi.txt
 create mode 100644 arch/arm/include/asm/uefi.h
 create mode 100644 arch/arm/kernel/uefi.c
 create mode 100644 arch/arm/kernel/uefi_phys.S

-- 
1.7.10.4

[PATCH v3 1/3] Documentation: arm: add UEFI support documentation

[Leif Lindholm]

This patch provides documentation of the [U]EFI runtime service and
configuration features for the arm architecture.

Changes since v1/v2:
- Complete rewrite.
- New FDT bindings.

Cc: Rob Landley <rob@landley.net>
Cc: linux-doc@vger.kernel.org

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 Documentation/arm/00-INDEX |    3 +++
 Documentation/arm/uefi.txt |   61 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)
 create mode 100644 Documentation/arm/uefi.txt

diff --git a/Documentation/arm/00-INDEX b/Documentation/arm/00-INDEX
index 36420e1..b3af704 100644
--- a/Documentation/arm/00-INDEX
+++ b/Documentation/arm/00-INDEX
@@ -34,3 +34,6 @@ nwfpe/
 	- NWFPE floating point emulator documentation
 swp_emulation
 	- SWP/SWPB emulation handler/logging description
+
+uefi.txt
+	- [U]EFI configuration and runtime services documentation
diff --git a/Documentation/arm/uefi.txt b/Documentation/arm/uefi.txt
new file mode 100644
index 0000000..9ba59509
--- /dev/null
+++ b/Documentation/arm/uefi.txt
@@ -0,0 +1,61 @@
+UEFI, the Unified Extensible Firmware Interface is, a specification
+governing the behaviours of compatible firmware interfaces. It is
+maintained by the UEFI Forum - http://www.uefi.org/.
+
+UEFI is an evolution of its predecessor 'EFI', so the terms EFI and
+UEFI are used somewhat interchangeably in this document and associated
+source code. As a rule, anything new uses 'UEFI', whereas 'EFI' refers
+to legacy code or specifications.
+
+UEFI support in Linux
+=====================
+Booting on a platform with firmware compliant with the UEFI specification
+makes it possible for the kernel to support additional features:
+- UEFI Runtime Services
+- Retrieving various configuration information through the standardised
+  interface of UEFI configuration tables. (ACPI, SMBIOS, ...)
+
+For actually enabling [U]EFI support, enable:
+- CONFIG_EFI=y
+- CONFIG_EFI_VARS=y or m
+
+The implementation depends on receiving information about the UEFI environment
+in a Flattened Device Tree (FDT) - so is only available with CONFIG_OF.
+
+UEFI stub
+=========
+The "stub" is a feature that turns the Image/zImage into a valid UEFI PE/COFF
+executable, including a loader application that makes it possible to load the
+kernel directly from the UEFI shell, boot menu, or one of the lightweight
+bootloaders like Gummiboot or rEFInd.
+
+The kernel image built with stub support remains a valid kernel image for
+booting in non-UEFI environments.
+
+UEFI kernel support on ARM
+==========================
+UEFI kernel support on the ARM architectures (arm and arm64) is only available
+when boot is performed through the stub.
+
+The stub populates the FDT /chosen node with (and the kernel scans for) the
+following parameters:
+________________________________________________________________________________
+Name                      | Size   | Description
+================================================================================
+linux,uefi-system-table   | 64-bit | Physical address of the UEFI System Table.
+--------------------------------------------------------------------------------
+linux,uefi-mmap-start     | 64-bit | Physical address of the UEFI memory map,
+                          |        | populated by the UEFI GetMemoryMap() call.
+--------------------------------------------------------------------------------
+linux,uefi-mmap-size      | 32-bit | Size in bytes of the UEFI memory map
+                          |        | pointed to in previous entry.
+--------------------------------------------------------------------------------
+linux,uefi-mmap-desc-size | 32-bit | Size in bytes of each entry in the UEFI
+                          |        | memory map.
+--------------------------------------------------------------------------------
+linux,uefi-mmap-desc-ver  | 32-bit | Version of the mmap descriptor format.
+--------------------------------------------------------------------------------
+linux,uefi-stub-kern-ver  | string | Copy of linux_banner from build.
+--------------------------------------------------------------------------------
+
+For verbose debug messages, specify 'uefi_debug' on the kernel command line.
-- 
1.7.10.4

[PATCH v3 2/3] arm: Add [U]EFI runtime services support

[Leif Lindholm]

This patch implements basic support for UEFI runtime services in the
ARM architecture - a requirement for using efibootmgr to read and update
the system boot configuration.

It uses the generic configuration table scanning to populate ACPI and
SMBIOS pointers.

Changes since v2:
- Updated FDT bindings.
- Preserve regions marked RESERVED (but don't map them).
- Rename 'efi' -> 'uefi' within this new port (leaving core code as is).

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 arch/arm/Kconfig            |   15 ++
 arch/arm/include/asm/uefi.h |   22 ++
 arch/arm/kernel/Makefile    |    2 +
 arch/arm/kernel/setup.c     |    6 +
 arch/arm/kernel/uefi.c      |  469 +++++++++++++++++++++++++++++++++++++++++++
 arch/arm/kernel/uefi_phys.S |   59 ++++++
 include/linux/efi.h         |    2 +-
 7 files changed, 574 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/include/asm/uefi.h
 create mode 100644 arch/arm/kernel/uefi.c
 create mode 100644 arch/arm/kernel/uefi_phys.S

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 78a79a6a..db8d212 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1853,6 +1853,19 @@ config EARLY_IOREMAP
 	  the same virtual memory range as kmap so all early mappings must
 	  be unapped before paging_init() is called.
 
+config EFI
+	bool "UEFI runtime service support"
+	depends on OF && !CPU_BIG_ENDIAN
+	select UCS2_STRING
+	select EARLY_IOREMAP
+	---help---
+	  This enables the kernel to use UEFI runtime services that are
+	  available (such as the UEFI variable services).
+
+	  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 SECCOMP
 	bool
 	prompt "Enable seccomp to safely compute untrusted bytecode"
@@ -2272,6 +2285,8 @@ source "net/Kconfig"
 
 source "drivers/Kconfig"
 
+source "drivers/firmware/Kconfig"
+
 source "fs/Kconfig"
 
 source "arch/arm/Kconfig.debug"
diff --git a/arch/arm/include/asm/uefi.h b/arch/arm/include/asm/uefi.h
new file mode 100644
index 0000000..519ca18
--- /dev/null
+++ b/arch/arm/include/asm/uefi.h
@@ -0,0 +1,22 @@
+#ifndef _ASM_ARM_EFI_H
+#define _ASM_ARM_EFI_H
+
+#include <asm/mach/map.h>
+
+extern int uefi_memblock_arm_reserve_range(void);
+
+typedef efi_status_t uefi_phys_call_t(u32 memory_map_size,
+				      u32 descriptor_size,
+				      u32 descriptor_version,
+				      efi_memory_desc_t *dsc,
+				      efi_set_virtual_address_map_t *f);
+
+extern efi_status_t uefi_phys_call(u32, u32, u32, efi_memory_desc_t *,
+				   efi_set_virtual_address_map_t *);
+
+#define uefi_remap(cookie, size) __arm_ioremap((cookie), (size), MT_MEMORY)
+#define uefi_ioremap(cookie, size) __arm_ioremap((cookie), (size), MT_DEVICE)
+#define uefi_unmap(cookie) __arm_iounmap((cookie))
+#define uefi_iounmap(cookie) __arm_iounmap((cookie))
+
+#endif /* _ASM_ARM_EFI_H */
diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index a30fc9b..736cce4 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -98,4 +98,6 @@ obj-y				+= psci.o
 obj-$(CONFIG_SMP)		+= psci_smp.o
 endif
 
+obj-$(CONFIG_EFI)		+= uefi.o uefi_phys.o
+
 extra-y := $(head-y) vmlinux.lds
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 04c1757..9d44edd 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -30,6 +30,7 @@
 #include <linux/bug.h>
 #include <linux/compiler.h>
 #include <linux/sort.h>
+#include <linux/efi.h>
 
 #include <asm/unified.h>
 #include <asm/cp15.h>
@@ -57,6 +58,7 @@
 #include <asm/unwind.h>
 #include <asm/memblock.h>
 #include <asm/virt.h>
+#include <asm/uefi.h>
 
 #include "atags.h"
 
@@ -898,6 +900,10 @@ void __init setup_arch(char **cmdline_p)
 	sanity_check_meminfo();
 	arm_memblock_init(&meminfo, mdesc);
 
+#ifdef CONFIG_EFI
+	uefi_memblock_arm_reserve_range();
+#endif
+
 	paging_init(mdesc);
 	request_standard_resources(mdesc);
 
diff --git a/arch/arm/kernel/uefi.c b/arch/arm/kernel/uefi.c
new file mode 100644
index 0000000..f771026
--- /dev/null
+++ b/arch/arm/kernel/uefi.c
@@ -0,0 +1,469 @@
+/*
+ * Based on Unified Extensible Firmware Interface Specification version 2.3.1
+ *
+ * Copyright (C) 2013  Linaro 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.
+ */
+
+#include <linux/efi.h>
+#include <linux/export.h>
+#include <linux/memblock.h>
+#include <linux/of.h>
+#include <linux/of_fdt.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+
+#include <asm/cacheflush.h>
+#include <asm/idmap.h>
+#include <asm/tlbflush.h>
+#include <asm/uefi.h>
+
+struct efi_memory_map memmap;
+
+static efi_runtime_services_t *runtime;
+
+static phys_addr_t uefi_system_table;
+static phys_addr_t uefi_boot_mmap;
+static u32 uefi_boot_mmap_size;
+static u32 uefi_mmap_desc_size;
+static u32 uefi_mmap_desc_ver;
+
+static unsigned long arm_uefi_facility;
+
+/*
+ * If you're planning to wire up a debugger and debug the UEFI side ...
+ */
+#undef KEEP_ALL_REGIONS
+
+/*
+ * If you need to (temporarily) support buggy firmware.
+ */
+#define KEEP_BOOT_SERVICES_REGIONS
+
+/*
+ * Returns 1 if 'facility' is enabled, 0 otherwise.
+ */
+int efi_enabled(int facility)
+{
+	return test_bit(facility, &arm_uefi_facility) != 0;
+}
+EXPORT_SYMBOL(efi_enabled);
+
+static int uefi_debug __initdata;
+static int __init uefi_debug_setup(char *str)
+{
+	uefi_debug = 1;
+
+	return 0;
+}
+early_param("uefi_debug", uefi_debug_setup);
+
+static struct {
+	const char *name;
+	const char *propname;
+	int numcells;
+	void *target;
+} dt_params[] = {
+	{
+		"UEFI System Table", "linux,uefi-system-table",
+		2, &uefi_system_table
+	}, {
+		"UEFI mmap address", "linux,uefi-mmap-start",
+		2, &uefi_boot_mmap
+	}, {
+		"UEFI mmap size", "linux,uefi-mmap-size",
+		1, &uefi_boot_mmap_size
+	}, {
+		"UEFI mmap descriptor size", "linux,uefi-mmap-desc-size",
+		1, &uefi_mmap_desc_size
+	}, {
+		"UEFI mmap descriptor version", "linux,uefi-mmap-desc-ver",
+		1, &uefi_mmap_desc_ver
+	}
+};
+
+static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
+				       int depth, void *data)
+{
+	void *prop;
+	int i;
+
+	if (depth != 1 ||
+	    (strcmp(uname, "chosen") != 0 && strcmp(uname, "chosen@0") != 0))
+		return 0;
+
+	pr_info("Getting UEFI parameters from FDT.\n");
+
+	for (i = 0; i < sizeof(dt_params)/sizeof(*dt_params); i++) {
+		prop = of_get_flat_dt_prop(node, dt_params[i].propname, NULL);
+		if (!prop)
+			return 0;
+		if (dt_params[i].numcells == 1) {
+			u32 *target = dt_params[i].target;
+			*target = of_read_ulong(prop, 1);
+		} else {
+			u64 *target = dt_params[i].target;
+			*target = of_read_number(prop, 2);
+		}
+
+		if (uefi_debug)
+			pr_info("  %s @ 0x%08x\n", dt_params[i].name,
+				*((u32 *)dt_params[i].target));
+	}
+
+	return 1;
+}
+
+static int __init uefi_init(void)
+{
+	efi_char16_t *c16;
+	char vendor[100] = "unknown";
+	int i, retval;
+
+	efi.systab = early_memremap(uefi_system_table,
+				    sizeof(efi_system_table_t));
+
+	/*
+	 * Verify the UEFI System Table
+	 */
+	if (efi.systab == NULL)
+		panic("Whoa! Can't find UEFI system table.\n");
+	if (efi.systab->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
+		panic("Whoa! UEFI system table signature incorrect\n");
+	if ((efi.systab->hdr.revision >> 16) == 0)
+		pr_warn("Warning: UEFI system table version %d.%02d, expected 1.00 or greater\n",
+			efi.systab->hdr.revision >> 16,
+			efi.systab->hdr.revision & 0xffff);
+
+	/* Show what we know for posterity */
+	c16 = (efi_char16_t *)early_memremap(efi.systab->fw_vendor,
+					    sizeof(vendor));
+	if (c16) {
+		for (i = 0; i < (int) sizeof(vendor) - 1 && *c16; ++i)
+			vendor[i] = c16[i];
+		vendor[i] = '\0';
+	}
+
+	pr_info("UEFI v%u.%.02u by %s\n",
+		efi.systab->hdr.revision >> 16,
+		efi.systab->hdr.revision & 0xffff, vendor);
+
+	retval = efi_config_init(NULL);
+	if (retval == 0)
+		set_bit(EFI_CONFIG_TABLES, &arm_uefi_facility);
+
+	early_iounmap(c16, sizeof(vendor));
+	early_iounmap(efi.systab,  sizeof(efi_system_table_t));
+
+	return retval;
+}
+
+static __init int is_discardable_region(efi_memory_desc_t *md)
+{
+#ifdef KEEP_ALL_REGIONS
+	return 0;
+#endif
+
+	if (md->attribute & EFI_MEMORY_RUNTIME)
+		return 0;
+
+	switch (md->type) {
+#ifdef KEEP_BOOT_SERVICES_REGIONS
+	case EFI_BOOT_SERVICES_CODE:
+	case EFI_BOOT_SERVICES_DATA:
+#endif
+	/* Keep tables around for any future kexec operations */
+	case EFI_ACPI_RECLAIM_MEMORY:
+		return 0;
+	/* Preserve */
+	case EFI_RESERVED_TYPE:
+		return 0;
+	}
+
+	return 1;
+}
+
+static __initdata struct {
+	u32 type;
+	const char *name;
+}  memory_type_name_map[] = {
+	{EFI_RESERVED_TYPE, "reserved"},
+	{EFI_LOADER_CODE, "loader code"},
+	{EFI_LOADER_DATA, "loader data"},
+	{EFI_BOOT_SERVICES_CODE, "boot services code"},
+	{EFI_BOOT_SERVICES_DATA, "boot services data"},
+	{EFI_RUNTIME_SERVICES_CODE, "runtime services code"},
+	{EFI_RUNTIME_SERVICES_DATA, "runtime services data"},
+	{EFI_CONVENTIONAL_MEMORY, "conventional memory"},
+	{EFI_UNUSABLE_MEMORY, "unusable memory"},
+	{EFI_ACPI_RECLAIM_MEMORY, "ACPI reclaim memory"},
+	{EFI_ACPI_MEMORY_NVS, "ACPI memory nvs"},
+	{EFI_MEMORY_MAPPED_IO, "memory mapped I/O"},
+	{EFI_MEMORY_MAPPED_IO_PORT_SPACE, "memory mapped I/O port space"},
+	{EFI_PAL_CODE, "pal code"},
+	{EFI_MAX_MEMORY_TYPE, NULL},
+};
+
+static __init void remove_sections(phys_addr_t addr, unsigned long size)
+{
+	unsigned long section_offset;
+	unsigned long num_sections;
+
+	section_offset = addr - (addr & SECTION_MASK);
+	num_sections = size / SECTION_SIZE;
+	if (size % SECTION_SIZE)
+		num_sections++;
+
+	memblock_remove(addr - section_offset, num_sections * SECTION_SIZE);
+}
+
+static __init int remove_regions(void)
+{
+	efi_memory_desc_t *md;
+	int count = 0;
+	void *p, *e;
+
+	if (uefi_debug)
+		pr_info("Processing UEFI memory map:\n");
+
+	memmap.phys_map = early_memremap((phys_addr_t)(u32) uefi_boot_mmap,
+					 uefi_boot_mmap_size);
+	if (!memmap.phys_map)
+		return 1;
+
+	p = memmap.phys_map;
+	e = (void *)((u32)p + uefi_boot_mmap_size);
+	for (; p < e; p += uefi_mmap_desc_size) {
+		md = p;
+		if (is_discardable_region(md))
+			continue;
+
+		if (uefi_debug)
+			pr_info("  %8llu pages @ %016llx (%s)\n",
+				md->num_pages, md->phys_addr,
+				memory_type_name_map[md->type].name);
+
+		if (md->type != EFI_MEMORY_MAPPED_IO) {
+			remove_sections(md->phys_addr,
+					md->num_pages * PAGE_SIZE);
+			count++;
+		}
+		memmap.nr_map++;
+	}
+
+	early_iounmap(memmap.phys_map, uefi_boot_mmap_size);
+
+	if (uefi_debug)
+		pr_info("%d regions preserved.\n", memmap.nr_map);
+
+	return 0;
+}
+
+int __init uefi_memblock_arm_reserve_range(void)
+{
+	if (!of_scan_flat_dt(fdt_find_uefi_params, NULL))
+		return 0;
+
+	set_bit(EFI_BOOT, &arm_uefi_facility);
+
+	uefi_init();
+
+	remove_regions();
+
+	return 0;
+}
+
+/*
+ * Disable instrrupts, enable idmap and disable caches.
+ */
+static void __init phys_call_prologue(void)
+{
+	local_irq_disable();
+
+	/* Take out a flat memory mapping. */
+	setup_mm_for_reboot();
+
+	/* Clean and invalidate caches */
+	flush_cache_all();
+
+	/* Turn off caching */
+	cpu_proc_fin();
+
+	/* Push out any further dirty data, and ensure cache is empty */
+	flush_cache_all();
+}
+
+/*
+ * Restore original memory map and re-enable interrupts.
+ */
+static void __init phys_call_epilogue(void)
+{
+	static struct mm_struct *mm = &init_mm;
+
+	/* Restore original memory mapping */
+	cpu_switch_mm(mm->pgd, mm);
+
+	/* Flush branch predictor and TLBs */
+	local_flush_bp_all();
+#ifdef CONFIG_CPU_HAS_ASID
+	local_flush_tlb_all();
+#endif
+
+	local_irq_enable();
+}
+
+static int __init remap_region(efi_memory_desc_t *md, efi_memory_desc_t *entry)
+{
+	u64 va;
+	u64 paddr;
+	u64 size;
+
+	*entry = *md;
+	paddr = entry->phys_addr;
+	size = entry->num_pages << EFI_PAGE_SHIFT;
+
+	/*
+	 * Map everything writeback-capable as coherent memory,
+	 * anything else as device.
+	 */
+	if (md->attribute & EFI_MEMORY_WB)
+		va = (u64)((u32)uefi_remap(paddr, size) & 0xffffffffUL);
+	else
+		va = (u64)((u32)uefi_ioremap(paddr, size) & 0xffffffffUL);
+	if (!va)
+		return 0;
+	entry->virt_addr = va;
+
+	if (uefi_debug)
+		pr_info("  %016llx-%016llx => 0x%08x : (%s)\n",
+			paddr, paddr + size - 1, (u32)va,
+			md->attribute &  EFI_MEMORY_WB ? "WB" : "I/O");
+
+	return 1;
+}
+
+static int __init remap_regions(void)
+{
+	void *p, *next;
+	efi_memory_desc_t *md;
+
+	memmap.phys_map = uefi_remap(uefi_boot_mmap, uefi_boot_mmap_size);
+	if (!memmap.phys_map)
+		return 0;
+	memmap.map_end = (void *)memmap.phys_map + uefi_boot_mmap_size;
+	memmap.desc_size = uefi_mmap_desc_size;
+	memmap.desc_version = uefi_mmap_desc_ver;
+
+	/* Allocate space for the physical region map */
+	memmap.map = kzalloc(memmap.nr_map * memmap.desc_size, GFP_KERNEL);
+	if (!memmap.map)
+		return 0;
+
+	next = memmap.map;
+	for (p = memmap.phys_map; p < memmap.map_end; p += memmap.desc_size) {
+		md = p;
+		if (is_discardable_region(md) || md->type == EFI_RESERVED_TYPE)
+			continue;
+
+		if (!remap_region(p, next))
+			return 0;
+
+		next += memmap.desc_size;
+	}
+
+	memmap.map_end = next;
+	efi.memmap = &memmap;
+
+	uefi_unmap(memmap.phys_map);
+	memmap.phys_map = efi_lookup_mapped_addr(uefi_boot_mmap);
+	efi.systab = efi_lookup_mapped_addr(uefi_system_table);
+	if (efi.systab)
+		set_bit(EFI_SYSTEM_TABLES, &arm_uefi_facility);
+	/*
+	 * efi.systab->runtime is a 32-bit pointer to something guaranteed by
+	 * the UEFI specification to be 1:1 mapped in a 4GB address space.
+	 */
+	runtime = efi_lookup_mapped_addr((u32)efi.systab->runtime);
+
+	return 1;
+}
+
+
+/*
+ * This function switches the UEFI runtime services to virtual mode.
+ * This operation must be performed only once in the system's lifetime,
+ * including any kecec calls.
+ *
+ * This must be done with a 1:1 mapping. The current implementation
+ * resolves this by disabling the MMU.
+ */
+efi_status_t  __init phys_set_virtual_address_map(u32 memory_map_size,
+						  u32 descriptor_size,
+						  u32 descriptor_version,
+						  efi_memory_desc_t *dsc)
+{
+	uefi_phys_call_t *phys_set_map;
+	efi_status_t status;
+
+	phys_call_prologue();
+
+	phys_set_map = (void *)(unsigned long)virt_to_phys(uefi_phys_call);
+
+	/* Called with caches disabled, returns with caches enabled */
+	status = phys_set_map(memory_map_size, descriptor_size,
+			      descriptor_version, dsc,
+			      efi.set_virtual_address_map)
+;
+	phys_call_epilogue();
+
+	return status;
+}
+
+/*
+ * Called explicitly from init/mm.c
+ */
+void __init efi_enter_virtual_mode(void)
+{
+	efi_status_t status;
+
+	if (!efi_enabled(EFI_BOOT)) {
+		pr_info("UEFI services will not be available.\n");
+		return;
+	}
+
+	pr_info("Remapping and enabling UEFI services.\n");
+
+	/* Map the regions we memblock_remove:d earlier into kernel
+	   address space */
+	if (!remap_regions()) {
+		pr_info("Failed to remap UEFI regions - runtime services will not be available.\n");
+		return;
+	}
+
+	/* Call SetVirtualAddressMap with the physical address of the map */
+	efi.set_virtual_address_map =
+		(efi_set_virtual_address_map_t *)
+		runtime->set_virtual_address_map;
+	memmap.phys_map =
+		(efi_memory_desc_t *)(u32) __virt_to_phys((u32)memmap.map);
+
+	status = phys_set_virtual_address_map(memmap.nr_map * memmap.desc_size,
+					      memmap.desc_size,
+					      memmap.desc_version,
+					      memmap.phys_map);
+
+	if (status != EFI_SUCCESS) {
+		pr_info("Failed to set UEFI virtual address map!\n");
+		return;
+	}
+
+	/* Set up function pointers for efivars */
+	efi.get_variable = (efi_get_variable_t *)runtime->get_variable;
+	efi.get_next_variable =
+		(efi_get_next_variable_t *)runtime->get_next_variable;
+	efi.set_variable = (efi_set_variable_t *)runtime->set_variable;
+	set_bit(EFI_RUNTIME_SERVICES, &arm_uefi_facility);
+}
diff --git a/arch/arm/kernel/uefi_phys.S b/arch/arm/kernel/uefi_phys.S
new file mode 100644
index 0000000..e9a1542
--- /dev/null
+++ b/arch/arm/kernel/uefi_phys.S
@@ -0,0 +1,59 @@
+/*
+ * arch/arm/kernel/uefi_phys.S
+ *
+ * Copyright (C) 2013  Linaro 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.
+ */
+
+#include <linux/linkage.h>
+#define PAR_MASK 0xfff
+
+	.text
+@ uefi_phys_call(a, b, c, d, *f)
+	.align  5
+        .pushsection    .idmap.text, "ax"
+ENTRY(uefi_phys_call)
+	@ Save physical context
+	mov	r12, sp
+	push	{r4-r5, r12, lr}
+
+	@ Extract function pointer (don't write r12 beyond this)
+	ldr	r12, [sp, #16]
+
+	@ Convert sp to 32-bit physical
+	mov	lr, sp
+	ldr	r4, =PAR_MASK
+	and	r5, lr, r4			@ Extract lower 12 bits of sp
+	mcr	p15, 0, lr, c7, c8, 1		@ Write VA -> ATS1CPW
+	mrc	p15, 0, lr, c7, c4, 0		@ Physical Address Register
+	mvn	r4, r4
+	and	lr, lr, r4			@ Clear lower 12 bits of PA
+	add	lr, lr, r5			@ Calculate phys sp
+	mov	sp, lr				@ Update
+
+	@ Disable MMU
+        mrc     p15, 0, lr, c1, c0, 0           @ ctrl register
+        bic     lr, lr, #0x1                    @ ...............m
+        mcr     p15, 0, lr, c1, c0, 0           @ disable MMU
+	isb
+
+	@ Make call
+	blx	r12
+
+	pop	{r4-r5, r12, lr}
+
+	@ Enable MMU + Caches
+        mrc     p15, 0, r1, c1, c0, 0		@ ctrl register
+        orr     r1, r1, #0x1000			@ ...i............
+        orr     r1, r1, #0x0005			@ .............c.m
+        mcr     p15, 0, r1, c1, c0, 0		@ enable MMU
+	isb
+
+	@ Restore virtual sp and return
+	mov	sp, r12
+	bx	lr
+ENDPROC(uefi_phys_call)
+        .popsection
diff --git a/include/linux/efi.h b/include/linux/efi.h
index bc5687d..ebb34ee 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -655,7 +655,7 @@ extern int __init efi_setup_pcdp_console(char *);
 #define EFI_64BIT		5	/* Is the firmware 64-bit? */
 
 #ifdef CONFIG_EFI
-# ifdef CONFIG_X86
+# if defined(CONFIG_X86) || defined(CONFIG_ARM)
 extern int efi_enabled(int facility);
 # else
 static inline int efi_enabled(int facility)
-- 
1.7.10.4

[PATCH v3 3/3] init: efi: arm: enable (U)EFI runtime services on arm

[Leif Lindholm]

Since the efi_set_virtual_address_map call has strict init ordering
requirements, add an explicit hook in the required place.

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 init/main.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/init/main.c b/init/main.c
index febc511..1331829 100644
--- a/init/main.c
+++ b/init/main.c
@@ -905,6 +905,10 @@ static noinline void __init kernel_init_freeable(void)
 	smp_prepare_cpus(setup_max_cpus);
 
 	do_pre_smp_initcalls();
+
+	if (IS_ENABLED(CONFIG_ARM) && efi_enabled(EFI_BOOT))
+		efi_enter_virtual_mode();
+
 	lockup_detector_init();
 
 	smp_init();
-- 
1.7.10.4

Re: [PATCH v3 0/3] (U)EFI runtime services for arm

[Matt Fleming]

On Thu, 28 Nov, at 04:41:20PM, Leif Lindholm wrote:
> In systems based on [U]EFI-conformant firmware, runtime services provide
> a standardised way for the kernel to update firmware environment
> variables. This is used for example by efibootmgr to update which image
> should be loaded on next boot.
> 
> This patchset implements basic support for UEFI runtime services on ARM
> platforms, as well as the basic underlying EFI support. It also defines
> a mechanism by which the required information is passed from the
> bootloader (the EFI stub submitted separately) to the kernel via FDT
> entries.

This all looks pretty good to me. Is this series going through the ARM
trees?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v3 2/3] arm: Add [U]EFI runtime services support

[Will Deacon]

Hi Leif,

On Thu, Nov 28, 2013 at 04:41:22PM +0000, Leif Lindholm wrote:
> This patch implements basic support for UEFI runtime services in the
> ARM architecture - a requirement for using efibootmgr to read and update
> the system boot configuration.
> 
> It uses the generic configuration table scanning to populate ACPI and
> SMBIOS pointers.

I've got a bunch of implementation questions, so hopefully you can help me
to understand what this is doing!

> diff --git a/arch/arm/kernel/uefi.c b/arch/arm/kernel/uefi.c
> new file mode 100644
> index 0000000..f771026
> --- /dev/null
> +++ b/arch/arm/kernel/uefi.c
> @@ -0,0 +1,469 @@
> +/*
> + * Based on Unified Extensible Firmware Interface Specification version 2.3.1
> + *
> + * Copyright (C) 2013  Linaro 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.
> + */
> +
> +#include <linux/efi.h>
> +#include <linux/export.h>
> +#include <linux/memblock.h>
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +
> +#include <asm/cacheflush.h>
> +#include <asm/idmap.h>
> +#include <asm/tlbflush.h>
> +#include <asm/uefi.h>
> +
> +struct efi_memory_map memmap;
> +
> +static efi_runtime_services_t *runtime;
> +
> +static phys_addr_t uefi_system_table;
> +static phys_addr_t uefi_boot_mmap;
> +static u32 uefi_boot_mmap_size;
> +static u32 uefi_mmap_desc_size;
> +static u32 uefi_mmap_desc_ver;
> +
> +static unsigned long arm_uefi_facility;
> +
> +/*
> + * If you're planning to wire up a debugger and debug the UEFI side ...
> + */
> +#undef KEEP_ALL_REGIONS
> +
> +/*
> + * If you need to (temporarily) support buggy firmware.
> + */
> +#define KEEP_BOOT_SERVICES_REGIONS
> +
> +/*
> + * Returns 1 if 'facility' is enabled, 0 otherwise.
> + */
> +int efi_enabled(int facility)
> +{
> +       return test_bit(facility, &arm_uefi_facility) != 0;

!test_bit(...) ?

> +static int __init uefi_init(void)
> +{
> +       efi_char16_t *c16;
> +       char vendor[100] = "unknown";
> +       int i, retval;
> +
> +       efi.systab = early_memremap(uefi_system_table,
> +                                   sizeof(efi_system_table_t));
> +
> +       /*
> +        * Verify the UEFI System Table
> +        */
> +       if (efi.systab == NULL)
> +               panic("Whoa! Can't find UEFI system table.\n");
> +       if (efi.systab->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
> +               panic("Whoa! UEFI system table signature incorrect\n");
> +       if ((efi.systab->hdr.revision >> 16) == 0)
> +               pr_warn("Warning: UEFI system table version %d.%02d, expected 1.00 or greater\n",
> +                       efi.systab->hdr.revision >> 16,
> +                       efi.systab->hdr.revision & 0xffff);
> +
> +       /* Show what we know for posterity */
> +       c16 = (efi_char16_t *)early_memremap(efi.systab->fw_vendor,
> +                                           sizeof(vendor));
> +       if (c16) {
> +               for (i = 0; i < (int) sizeof(vendor) - 1 && *c16; ++i)
> +                       vendor[i] = c16[i];
> +               vendor[i] = '\0';
> +       }
> +
> +       pr_info("UEFI v%u.%.02u by %s\n",
> +               efi.systab->hdr.revision >> 16,
> +               efi.systab->hdr.revision & 0xffff, vendor);
> +
> +       retval = efi_config_init(NULL);
> +       if (retval == 0)
> +               set_bit(EFI_CONFIG_TABLES, &arm_uefi_facility);

Does this actually need to be atomic?

> +
> +       early_iounmap(c16, sizeof(vendor));
> +       early_iounmap(efi.systab,  sizeof(efi_system_table_t));
> +
> +       return retval;
> +}
> +
> +static __init int is_discardable_region(efi_memory_desc_t *md)
> +{
> +#ifdef KEEP_ALL_REGIONS
> +       return 0;
> +#endif

Maybe just have a dummy is_discardable_region in this case, like we usually
do instead of inlining the #ifdef?

> +       if (md->attribute & EFI_MEMORY_RUNTIME)
> +               return 0;
> +
> +       switch (md->type) {
> +#ifdef KEEP_BOOT_SERVICES_REGIONS
> +       case EFI_BOOT_SERVICES_CODE:
> +       case EFI_BOOT_SERVICES_DATA:
> +#endif
> +       /* Keep tables around for any future kexec operations */
> +       case EFI_ACPI_RECLAIM_MEMORY:
> +               return 0;
> +       /* Preserve */
> +       case EFI_RESERVED_TYPE:
> +               return 0;
> +       }
> +
> +       return 1;
> +}

[...]

> +int __init uefi_memblock_arm_reserve_range(void)
> +{
> +       if (!of_scan_flat_dt(fdt_find_uefi_params, NULL))
> +               return 0;
> +
> +       set_bit(EFI_BOOT, &arm_uefi_facility);

Similar comment wrt atomicity here (and in the rest of this patch).

> +       uefi_init();
> +
> +       remove_regions();
> +
> +       return 0;
> +}
> +
> +/*
> + * Disable instrrupts, enable idmap and disable caches.

interrupts

> + */
> +static void __init phys_call_prologue(void)
> +{
> +       local_irq_disable();
> +
> +       /* Take out a flat memory mapping. */
> +       setup_mm_for_reboot();
> +
> +       /* Clean and invalidate caches */
> +       flush_cache_all();
> +
> +       /* Turn off caching */
> +       cpu_proc_fin();
> +
> +       /* Push out any further dirty data, and ensure cache is empty */
> +       flush_cache_all();

Do we care about outer caches here? This all looks suspiciously like
copy-paste from __soft_restart; perhaps some refactoring/reuse is in order?

> +}
> +
> +/*
> + * Restore original memory map and re-enable interrupts.
> + */
> +static void __init phys_call_epilogue(void)
> +{
> +       static struct mm_struct *mm = &init_mm;
> +
> +       /* Restore original memory mapping */
> +       cpu_switch_mm(mm->pgd, mm);
> +
> +       /* Flush branch predictor and TLBs */
> +       local_flush_bp_all();
> +#ifdef CONFIG_CPU_HAS_ASID
> +       local_flush_tlb_all();
> +#endif

... and this looks like copy-paste from setup_mm_for_reboot.

> +       local_irq_enable();
> +}
> +
> +static int __init remap_region(efi_memory_desc_t *md, efi_memory_desc_t *entry)
> +{
> +       u64 va;
> +       u64 paddr;
> +       u64 size;
> +
> +       *entry = *md;
> +       paddr = entry->phys_addr;
> +       size = entry->num_pages << EFI_PAGE_SHIFT;
> +
> +       /*
> +        * Map everything writeback-capable as coherent memory,
> +        * anything else as device.
> +        */
> +       if (md->attribute & EFI_MEMORY_WB)
> +               va = (u64)((u32)uefi_remap(paddr, size) & 0xffffffffUL);
> +       else
> +               va = (u64)((u32)uefi_ioremap(paddr, size) & 0xffffffffUL);

Do you really need all those casts/masking?

> +       if (!va)
> +               return 0;
> +       entry->virt_addr = va;
> +
> +       if (uefi_debug)
> +               pr_info("  %016llx-%016llx => 0x%08x : (%s)\n",
> +                       paddr, paddr + size - 1, (u32)va,
> +                       md->attribute &  EFI_MEMORY_WB ? "WB" : "I/O");
> +
> +       return 1;
> +}
> +
> +static int __init remap_regions(void)
> +{
> +       void *p, *next;
> +       efi_memory_desc_t *md;
> +
> +       memmap.phys_map = uefi_remap(uefi_boot_mmap, uefi_boot_mmap_size);
> +       if (!memmap.phys_map)
> +               return 0;
> +       memmap.map_end = (void *)memmap.phys_map + uefi_boot_mmap_size;
> +       memmap.desc_size = uefi_mmap_desc_size;
> +       memmap.desc_version = uefi_mmap_desc_ver;
> +
> +       /* Allocate space for the physical region map */
> +       memmap.map = kzalloc(memmap.nr_map * memmap.desc_size, GFP_KERNEL);
> +       if (!memmap.map)
> +               return 0;
> +
> +       next = memmap.map;
> +       for (p = memmap.phys_map; p < memmap.map_end; p += memmap.desc_size) {
> +               md = p;
> +               if (is_discardable_region(md) || md->type == EFI_RESERVED_TYPE)
> +                       continue;
> +
> +               if (!remap_region(p, next))
> +                       return 0;

What about the kzalloc above, does that need freeing?

> +
> +               next += memmap.desc_size;
> +       }
> +
> +       memmap.map_end = next;
> +       efi.memmap = &memmap;
> +
> +       uefi_unmap(memmap.phys_map);
> +       memmap.phys_map = efi_lookup_mapped_addr(uefi_boot_mmap);
> +       efi.systab = efi_lookup_mapped_addr(uefi_system_table);
> +       if (efi.systab)
> +               set_bit(EFI_SYSTEM_TABLES, &arm_uefi_facility);
> +       /*
> +        * efi.systab->runtime is a 32-bit pointer to something guaranteed by
> +        * the UEFI specification to be 1:1 mapped in a 4GB address space.
> +        */
> +       runtime = efi_lookup_mapped_addr((u32)efi.systab->runtime);
> +
> +       return 1;
> +}
> +
> +
> +/*
> + * This function switches the UEFI runtime services to virtual mode.
> + * This operation must be performed only once in the system's lifetime,
> + * including any kecec calls.
> + *
> + * This must be done with a 1:1 mapping. The current implementation
> + * resolves this by disabling the MMU.
> + */
> +efi_status_t  __init phys_set_virtual_address_map(u32 memory_map_size,
> +                                                 u32 descriptor_size,
> +                                                 u32 descriptor_version,
> +                                                 efi_memory_desc_t *dsc)
> +{
> +       uefi_phys_call_t *phys_set_map;
> +       efi_status_t status;
> +
> +       phys_call_prologue();
> +
> +       phys_set_map = (void *)(unsigned long)virt_to_phys(uefi_phys_call);
> +
> +       /* Called with caches disabled, returns with caches enabled */
> +       status = phys_set_map(memory_map_size, descriptor_size,
> +                             descriptor_version, dsc,
> +                             efi.set_virtual_address_map)
> +;

Guessing this relies on a physically-tagged cache? Wouldn't hurt to put
something in the epilogue code to deal with vivt caches if they're not
prohibited by construction (e.g. due to some build dependency magic).

> +       phys_call_epilogue();
> +
> +       return status;
> +}
> +
> +/*
> + * Called explicitly from init/mm.c
> + */
> +void __init efi_enter_virtual_mode(void)
> +{
> +       efi_status_t status;
> +
> +       if (!efi_enabled(EFI_BOOT)) {
> +               pr_info("UEFI services will not be available.\n");
> +               return;
> +       }
> +
> +       pr_info("Remapping and enabling UEFI services.\n");
> +
> +       /* Map the regions we memblock_remove:d earlier into kernel
> +          address space */
> +       if (!remap_regions()) {
> +               pr_info("Failed to remap UEFI regions - runtime services will not be available.\n");
> +               return;
> +       }
> +
> +       /* Call SetVirtualAddressMap with the physical address of the map */
> +       efi.set_virtual_address_map =
> +               (efi_set_virtual_address_map_t *)
> +               runtime->set_virtual_address_map;
> +       memmap.phys_map =
> +               (efi_memory_desc_t *)(u32) __virt_to_phys((u32)memmap.map);
> +
> +       status = phys_set_virtual_address_map(memmap.nr_map * memmap.desc_size,
> +                                             memmap.desc_size,
> +                                             memmap.desc_version,
> +                                             memmap.phys_map);
> +
> +       if (status != EFI_SUCCESS) {
> +               pr_info("Failed to set UEFI virtual address map!\n");
> +               return;
> +       }
> +
> +       /* Set up function pointers for efivars */
> +       efi.get_variable = (efi_get_variable_t *)runtime->get_variable;
> +       efi.get_next_variable =
> +               (efi_get_next_variable_t *)runtime->get_next_variable;
> +       efi.set_variable = (efi_set_variable_t *)runtime->set_variable;
> +       set_bit(EFI_RUNTIME_SERVICES, &arm_uefi_facility);
> +}
> diff --git a/arch/arm/kernel/uefi_phys.S b/arch/arm/kernel/uefi_phys.S
> new file mode 100644
> index 0000000..e9a1542
> --- /dev/null
> +++ b/arch/arm/kernel/uefi_phys.S
> @@ -0,0 +1,59 @@
> +/*
> + * arch/arm/kernel/uefi_phys.S
> + *
> + * Copyright (C) 2013  Linaro 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.
> + */
> +
> +#include <linux/linkage.h>
> +#define PAR_MASK 0xfff
> +
> +       .text
> +@ uefi_phys_call(a, b, c, d, *f)
> +       .align  5
> +        .pushsection    .idmap.text, "ax"
> +ENTRY(uefi_phys_call)
> +       @ Save physical context
> +       mov     r12, sp
> +       push    {r4-r5, r12, lr}
> +
> +       @ Extract function pointer (don't write r12 beyond this)
> +       ldr     r12, [sp, #16]
> +
> +       @ Convert sp to 32-bit physical
> +       mov     lr, sp
> +       ldr     r4, =PAR_MASK
> +       and     r5, lr, r4                      @ Extract lower 12 bits of sp
> +       mcr     p15, 0, lr, c7, c8, 1           @ Write VA -> ATS1CPW

This is broken without an isb but, more to the point, why can't we just do
the standard lowmem virt_to_phys conversion here instead?

> +       mrc     p15, 0, lr, c7, c4, 0           @ Physical Address Register
> +       mvn     r4, r4
> +       and     lr, lr, r4                      @ Clear lower 12 bits of PA
> +       add     lr, lr, r5                      @ Calculate phys sp
> +       mov     sp, lr                          @ Update
> +
> +       @ Disable MMU
> +        mrc     p15, 0, lr, c1, c0, 0           @ ctrl register
> +        bic     lr, lr, #0x1                    @ ...............m
> +        mcr     p15, 0, lr, c1, c0, 0           @ disable MMU
> +       isb
> +
> +       @ Make call
> +       blx     r12

This is basically a copy-paste of cpu_v7_reset. Perhaps using some assembler
macros would help here?

> +
> +       pop     {r4-r5, r12, lr}
> +
> +       @ Enable MMU + Caches
> +        mrc     p15, 0, r1, c1, c0, 0          @ ctrl register
> +        orr     r1, r1, #0x1000                        @ ...i............
> +        orr     r1, r1, #0x0005                        @ .............c.m
> +        mcr     p15, 0, r1, c1, c0, 0          @ enable MMU
> +       isb

Why do we have to enable the caches so early?

Will

Re: [PATCH v3 2/3] arm: Add [U]EFI runtime services support

[Leif Lindholm]

Hi Will,

Thanks for having a look.

On Fri, Nov 29, 2013 at 04:10:16PM +0000, Will Deacon wrote:
> > This patch implements basic support for UEFI runtime services in the
> > ARM architecture - a requirement for using efibootmgr to read and update
> > the system boot configuration.
> > 
> > It uses the generic configuration table scanning to populate ACPI and
> > SMBIOS pointers.
> 
> I've got a bunch of implementation questions, so hopefully you can help me
> to understand what this is doing!

I can try :)

> > diff --git a/arch/arm/kernel/uefi.c b/arch/arm/kernel/uefi.c
> > new file mode 100644
> > index 0000000..f771026
> > --- /dev/null
> > +++ b/arch/arm/kernel/uefi.c

> > +/*
> > + * Returns 1 if 'facility' is enabled, 0 otherwise.
> > + */
> > +int efi_enabled(int facility)
> > +{
> > +       return test_bit(facility, &arm_uefi_facility) != 0;
> 
> !test_bit(...) ?

Could do. Cloned from the x86 implementation. Matt Fleming has
indicated some rework coming in this area.

> > +static int __init uefi_init(void)
> > +{
> > +       efi_char16_t *c16;
> > +       char vendor[100] = "unknown";
> > +       int i, retval;
> > +
> > +       efi.systab = early_memremap(uefi_system_table,
> > +                                   sizeof(efi_system_table_t));
> > +
> > +       /*
> > +        * Verify the UEFI System Table
> > +        */
> > +       if (efi.systab == NULL)
> > +               panic("Whoa! Can't find UEFI system table.\n");
> > +       if (efi.systab->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
> > +               panic("Whoa! UEFI system table signature incorrect\n");
> > +       if ((efi.systab->hdr.revision >> 16) == 0)
> > +               pr_warn("Warning: UEFI system table version %d.%02d, expected 1.00 or greater\n",
> > +                       efi.systab->hdr.revision >> 16,
> > +                       efi.systab->hdr.revision & 0xffff);
> > +
> > +       /* Show what we know for posterity */
> > +       c16 = (efi_char16_t *)early_memremap(efi.systab->fw_vendor,
> > +                                           sizeof(vendor));
> > +       if (c16) {
> > +               for (i = 0; i < (int) sizeof(vendor) - 1 && *c16; ++i)
> > +                       vendor[i] = c16[i];
> > +               vendor[i] = '\0';
> > +       }
> > +
> > +       pr_info("UEFI v%u.%.02u by %s\n",
> > +               efi.systab->hdr.revision >> 16,
> > +               efi.systab->hdr.revision & 0xffff, vendor);
> > +
> > +       retval = efi_config_init(NULL);
> > +       if (retval == 0)
> > +               set_bit(EFI_CONFIG_TABLES, &arm_uefi_facility);
> 
> Does this actually need to be atomic?

Not necessarily, but it's neater than masking, and only called once.

> > +
> > +       early_iounmap(c16, sizeof(vendor));
> > +       early_iounmap(efi.systab,  sizeof(efi_system_table_t));
> > +
> > +       return retval;
> > +}
> > +
> > +static __init int is_discardable_region(efi_memory_desc_t *md)
> > +{
> > +#ifdef KEEP_ALL_REGIONS
> > +       return 0;
> > +#endif
> 
> Maybe just have a dummy is_discardable_region in this case, like we usually
> do instead of inlining the #ifdef?
 
OK.

> > +       if (md->attribute & EFI_MEMORY_RUNTIME)
> > +               return 0;
> > +
> > +       switch (md->type) {
> > +#ifdef KEEP_BOOT_SERVICES_REGIONS
> > +       case EFI_BOOT_SERVICES_CODE:
> > +       case EFI_BOOT_SERVICES_DATA:
> > +#endif
> > +       /* Keep tables around for any future kexec operations */
> > +       case EFI_ACPI_RECLAIM_MEMORY:
> > +               return 0;
> > +       /* Preserve */
> > +       case EFI_RESERVED_TYPE:
> > +               return 0;
> > +       }
> > +
> > +       return 1;
> > +}
> 
> [...]
> 
> > +int __init uefi_memblock_arm_reserve_range(void)
> > +{
> > +       if (!of_scan_flat_dt(fdt_find_uefi_params, NULL))
> > +               return 0;
> > +
> > +       set_bit(EFI_BOOT, &arm_uefi_facility);
> 
> Similar comment wrt atomicity here (and in the rest of this patch).

Similar response.

> > +       uefi_init();
> > +
> > +       remove_regions();
> > +
> > +       return 0;
> > +}
> > +
> > +/*
> > + * Disable instrrupts, enable idmap and disable caches.
> 
> interrupts

Yeah.

> > + */
> > +static void __init phys_call_prologue(void)
> > +{
> > +       local_irq_disable();
> > +
> > +       /* Take out a flat memory mapping. */
> > +       setup_mm_for_reboot();
> > +
> > +       /* Clean and invalidate caches */
> > +       flush_cache_all();
> > +
> > +       /* Turn off caching */
> > +       cpu_proc_fin();
> > +
> > +       /* Push out any further dirty data, and ensure cache is empty */
> > +       flush_cache_all();
> 
> Do we care about outer caches here?

I think we do. We jump into UEFI and make it relocate itself to the
new virtual addresses, with MMU disabled (so all accesses NC).

> This all looks suspiciously like
> copy-paste from __soft_restart;

Yeah, 'cause you told me to :)

> perhaps some refactoring/reuse is in order?
 
Could do. Turn this into a process.c:idcall_prepare(), or something less
daftly named?

> > +}
> > +
> > +/*
> > + * Restore original memory map and re-enable interrupts.
> > + */
> > +static void __init phys_call_epilogue(void)
> > +{
> > +       static struct mm_struct *mm = &init_mm;
> > +
> > +       /* Restore original memory mapping */
> > +       cpu_switch_mm(mm->pgd, mm);
> > +
> > +       /* Flush branch predictor and TLBs */
> > +       local_flush_bp_all();
> > +#ifdef CONFIG_CPU_HAS_ASID
> > +       local_flush_tlb_all();
> > +#endif
> 
> ... and this looks like copy-paste from setup_mm_for_reboot.

You told me that too!
Only this goes the other way.

Is the refactoring worth the extra code?

> > +       local_irq_enable();
> > +}
> > +
> > +static int __init remap_region(efi_memory_desc_t *md, efi_memory_desc_t *entry)
> > +{
> > +       u64 va;
> > +       u64 paddr;
> > +       u64 size;
> > +
> > +       *entry = *md;
> > +       paddr = entry->phys_addr;
> > +       size = entry->num_pages << EFI_PAGE_SHIFT;
> > +
> > +       /*
> > +        * Map everything writeback-capable as coherent memory,
> > +        * anything else as device.
> > +        */
> > +       if (md->attribute & EFI_MEMORY_WB)
> > +               va = (u64)((u32)uefi_remap(paddr, size) & 0xffffffffUL);
> > +       else
> > +               va = (u64)((u32)uefi_ioremap(paddr, size) & 0xffffffffUL);
> 
> Do you really need all those casts/masking?

I didn't find a better way to avoid warnings when building this code
for both LPAE/non-LPAE.

We are guaranteed by the UEFI spec that all addresses will be <4GB,
but the descriptor format is still 64-bit physical/virtual addresses,
and we need to pass them back as such.

> > +       if (!va)
> > +               return 0;
> > +       entry->virt_addr = va;
> > +
> > +       if (uefi_debug)
> > +               pr_info("  %016llx-%016llx => 0x%08x : (%s)\n",
> > +                       paddr, paddr + size - 1, (u32)va,
> > +                       md->attribute &  EFI_MEMORY_WB ? "WB" : "I/O");
> > +
> > +       return 1;
> > +}
> > +
> > +static int __init remap_regions(void)
> > +{
> > +       void *p, *next;
> > +       efi_memory_desc_t *md;
> > +
> > +       memmap.phys_map = uefi_remap(uefi_boot_mmap, uefi_boot_mmap_size);
> > +       if (!memmap.phys_map)
> > +               return 0;
> > +       memmap.map_end = (void *)memmap.phys_map + uefi_boot_mmap_size;
> > +       memmap.desc_size = uefi_mmap_desc_size;
> > +       memmap.desc_version = uefi_mmap_desc_ver;
> > +
> > +       /* Allocate space for the physical region map */
> > +       memmap.map = kzalloc(memmap.nr_map * memmap.desc_size, GFP_KERNEL);
> > +       if (!memmap.map)
> > +               return 0;
> > +
> > +       next = memmap.map;
> > +       for (p = memmap.phys_map; p < memmap.map_end; p += memmap.desc_size) {
> > +               md = p;
> > +               if (is_discardable_region(md) || md->type == EFI_RESERVED_TYPE)
> > +                       continue;
> > +
> > +               if (!remap_region(p, next))
> > +                       return 0;
> 
> What about the kzalloc above, does that need freeing?

In case of failure? Yes, good point.

> > +/*
> > + * This function switches the UEFI runtime services to virtual mode.
> > + * This operation must be performed only once in the system's lifetime,
> > + * including any kecec calls.
> > + *
> > + * This must be done with a 1:1 mapping. The current implementation
> > + * resolves this by disabling the MMU.
> > + */
> > +efi_status_t  __init phys_set_virtual_address_map(u32 memory_map_size,
> > +                                                 u32 descriptor_size,
> > +                                                 u32 descriptor_version,
> > +                                                 efi_memory_desc_t *dsc)
> > +{
> > +       uefi_phys_call_t *phys_set_map;
> > +       efi_status_t status;
> > +
> > +       phys_call_prologue();
> > +
> > +       phys_set_map = (void *)(unsigned long)virt_to_phys(uefi_phys_call);
> > +
> > +       /* Called with caches disabled, returns with caches enabled */
> > +       status = phys_set_map(memory_map_size, descriptor_size,
> > +                             descriptor_version, dsc,
> > +                             efi.set_virtual_address_map)
> > +;
> 
> Guessing this relies on a physically-tagged cache? Wouldn't hurt to put
> something in the epilogue code to deal with vivt caches if they're not
> prohibited by construction (e.g. due to some build dependency magic).

Sorry, I don't quite follow?
How would it depend on a physically-tagged cache?

> > +       phys_call_epilogue();
> > +
> > +       return status;
> > +}


> > diff --git a/arch/arm/kernel/uefi_phys.S b/arch/arm/kernel/uefi_phys.S
> > new file mode 100644
> > index 0000000..e9a1542
> > --- /dev/null
> > +++ b/arch/arm/kernel/uefi_phys.S
> > @@ -0,0 +1,59 @@
> > +/*
> > + * arch/arm/kernel/uefi_phys.S
> > + *
> > + * Copyright (C) 2013  Linaro 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.
> > + */
> > +
> > +#include <linux/linkage.h>
> > +#define PAR_MASK 0xfff
> > +
> > +       .text
> > +@ uefi_phys_call(a, b, c, d, *f)
> > +       .align  5
> > +        .pushsection    .idmap.text, "ax"
> > +ENTRY(uefi_phys_call)
> > +       @ Save physical context
> > +       mov     r12, sp
> > +       push    {r4-r5, r12, lr}
> > +
> > +       @ Extract function pointer (don't write r12 beyond this)
> > +       ldr     r12, [sp, #16]
> > +
> > +       @ Convert sp to 32-bit physical
> > +       mov     lr, sp
> > +       ldr     r4, =PAR_MASK
> > +       and     r5, lr, r4                      @ Extract lower 12 bits of sp
> > +       mcr     p15, 0, lr, c7, c8, 1           @ Write VA -> ATS1CPW
> 
> This is broken without an isb but, more to the point, why can't we just do
> the standard lowmem virt_to_phys conversion here instead?

I can't use that from within asm (right?). Are you suggesting that I
should duplicate the mechanism of virt_to_phys here?

> > +       mrc     p15, 0, lr, c7, c4, 0           @ Physical Address Register
> > +       mvn     r4, r4
> > +       and     lr, lr, r4                      @ Clear lower 12 bits of PA
> > +       add     lr, lr, r5                      @ Calculate phys sp
> > +       mov     sp, lr                          @ Update
> > +
> > +       @ Disable MMU
> > +        mrc     p15, 0, lr, c1, c0, 0           @ ctrl register
> > +        bic     lr, lr, #0x1                    @ ...............m
> > +        mcr     p15, 0, lr, c1, c0, 0           @ disable MMU
> > +       isb
> > +
> > +       @ Make call
> > +       blx     r12
> 
> This is basically a copy-paste of cpu_v7_reset. Perhaps using some assembler
> macros would help here?

Well, I explicitly don't want to touch SCTLR.TE.
We could macro-ize it, but I think that would increase the amount of
code.

> > +
> > +       pop     {r4-r5, r12, lr}
> > +
> > +       @ Enable MMU + Caches
> > +        mrc     p15, 0, r1, c1, c0, 0          @ ctrl register
> > +        orr     r1, r1, #0x1000                        @ ...i............
> > +        orr     r1, r1, #0x0005                        @ .............c.m
> > +        mcr     p15, 0, r1, c1, c0, 0          @ enable MMU
> > +       isb
> 
> Why do we have to enable the caches so early?

You'd prefer it being done back in phys_call_epilogue()?

/
    Leif

Re: [PATCH v3 0/3] (U)EFI runtime services for arm

[Leif Lindholm]

On Fri, Nov 29, 2013 at 11:53:19AM +0000, Matt Fleming wrote:
> On Thu, 28 Nov, at 04:41:20PM, Leif Lindholm wrote:
> > In systems based on [U]EFI-conformant firmware, runtime services provide
> > a standardised way for the kernel to update firmware environment
> > variables. This is used for example by efibootmgr to update which image
> > should be loaded on next boot.
> > 
> > This patchset implements basic support for UEFI runtime services on ARM
> > platforms, as well as the basic underlying EFI support. It also defines
> > a mechanism by which the required information is passed from the
> > bootloader (the EFI stub submitted separately) to the kernel via FDT
> > entries.
> 
> This all looks pretty good to me. Is this series going through the ARM
> trees?

Thanks.
That's the plan.

/
    Leif

Re: [PATCH v3 1/3] Documentation: arm: add UEFI support documentation

[Matt Sealey]

> +UEFI kernel support on ARM
> +==========================
> +UEFI kernel support on the ARM architectures (arm and arm64) is only available
> +when boot is performed through the stub.
> +
> +The stub populates the FDT /chosen node with (and the kernel scans for) the
> +following parameters:
> +________________________________________________________________________________
> +Name                      | Size   | Description
> +================================================================================
> +linux,uefi-system-table   | 64-bit | Physical address of the UEFI System Table.
> +--------------------------------------------------------------------------------
> +linux,uefi-mmap-start     | 64-bit | Physical address of the UEFI memory map,
> +                          |        | populated by the UEFI GetMemoryMap() call.
> +--------------------------------------------------------------------------------
> +linux,uefi-mmap-size      | 32-bit | Size in bytes of the UEFI memory map
> +                          |        | pointed to in previous entry.
> +--------------------------------------------------------------------------------
> +linux,uefi-mmap-desc-size | 32-bit | Size in bytes of each entry in the UEFI
> +                          |        | memory map.
> +--------------------------------------------------------------------------------
> +linux,uefi-mmap-desc-ver  | 32-bit | Version of the mmap descriptor format.
> +--------------------------------------------------------------------------------
> +linux,uefi-stub-kern-ver  | string | Copy of linux_banner from build.
> +--------------------------------------------------------------------------------

This flies in the face of actually using #address-cells and
#size-cells to define how big addresses and sizes are. You're limited
here by the root node definitions... that's the spec.

Here's where I think this whole thing falls down as being the weirdest
possible implementation of this. It defies logic to put this
information in the device tree /chosen node while also attempting to
boot the kernel using an EFI stub; the stub is going to have this
information because it is going to have the pointer to the system
System Table (since it was called by StartImage()). Why not stash the
System Table pointer somewhere safe in the stub?

The information in the device tree is all accessible from Boot
Services and as long as the System Table isn't being thrown away (my
suggestion would be.. stuff it in r2, and set r1 = "EFI\0" then work
with arch/arm/kernel/head{-common,}.S code to do the right thing)

It seems like the advantages of booting from UEFI and having all this
information and API around are being thrown away very early, and
picked up when it's no longer relevant to gain access to the very
minimal runtime services. What's missing is a UUID for a "Device Tree
Blob" in the Configuration Table, so you can very easily go grab that
information from the firmware.

As implemented, these patches employ a very long-winded and complex
method of recovering UEFI after throwing the system table pointer away
early in boot, and then implements an EFI calling convention which
isn't strictly necessary according to the UEFI spec - the question is,
is this a workaround for SetVirtualAddressMap() not actually doing the
right thing on ARM UEFI implementations? If you can't guarantee that
most of the calls from Boot Services or Runtime Services are going to
allow this, then having any UEFI support in the kernel at all seems
rather weird.

What I'm worried about is that this is basically a hack to try and
shoehorn an existing UEFI implementation to an existing Linux boot
method - and implements it in a way nobody is ever going to be able to
justify improving. Part of the reason the OpenFirmware CIF got thrown
away early in SPARC/PowerPC boot (after "flattening" the device tree
using the CIF calls to parse it out) was because you had to disable
the MMU, caches, interrupts etc. which caused all kinds of slow
firmware code to be all kinds of extra-slow.

What that meant is nobody bothered to implement working, re-entrant,
re-locatable firmware to a great degree. This ended up being a
self-fulfilling prophecy of "don't trust the bootloader" and "get rid
of it as soon as we can," which essentially meant Linux never took
advantage of the resources available. In OF's case, the CIF sucked by
specification. In UEFI's case here, it's been implemented in Linux in
such a way that guarantees poor-performing firmware code with huge
penalties to call them, which isn't even required by UEFI if the
earlier boot code did the right things in the first place.

Ta,
Matt Sealey <neko@bakuhatsu.net>

Re: [PATCH v3 1/3] Documentation: arm: add UEFI support documentation

[Leif Lindholm]

On Mon, Dec 02, 2013 at 01:51:22PM -0600, Matt Sealey wrote:
> Here's where I think this whole thing falls down as being the weirdest
> possible implementation of this. It defies logic to put this
> information in the device tree /chosen node while also attempting to
> boot the kernel using an EFI stub; the stub is going to have this
> information because it is going to have the pointer to the system
> System Table (since it was called by StartImage()). Why not stash the
> System Table pointer somewhere safe in the stub?

We do. In the DT.

> The information in the device tree is all accessible from Boot
> Services and as long as the System Table isn't being thrown away (my
> suggestion would be.. stuff it in r2, and set r1 = "EFI\0" then work
> with arch/arm/kernel/head{-common,}.S code to do the right thing)

You left out the bit of redefining the kernel boot protocol to permit
calling it with caches, MMU and interrupts enabled - also known as
before ExitBootServices().

> It seems like the advantages of booting from UEFI and having all this
> information and API around are being thrown away very early, and
> picked up when it's no longer relevant to gain access to the very
> minimal runtime services. What's missing is a UUID for a "Device Tree
> Blob" in the Configuration Table, so you can very easily go grab that
> information from the firmware.

Which is what we are going to implement anyway in order to permit
firmware to supply DT hardware description in the same way as with
ACPI. Yes, we could pass the system table pointer directly - but that
doesn't get us the memory map.

> As implemented, these patches employ a very long-winded and complex
> method of recovering UEFI after throwing the system table pointer away
> early in boot, and then implements an EFI calling convention which
> isn't strictly necessary according to the UEFI spec - the question is,
> is this a workaround for SetVirtualAddressMap() not actually doing the
> right thing on ARM UEFI implementations? If you can't guarantee that
> most of the calls from Boot Services or Runtime Services are going to
> allow this, then having any UEFI support in the kernel at all seems
> rather weird.

No, it is a workaround for it being explicitly against the kernel boot
protocol (not to mention slightly hairy) to enter head.S with MMU and
caches enabled and interrupts firing.

The EFI calling convention (as pointed out in the patch itself) is
there in order to not have to duplicate code already there for x86.

> What I'm worried about is that this is basically a hack to try and
> shoehorn an existing UEFI implementation to an existing Linux boot
> method - and implements it in a way nobody is ever going to be able to
> justify improving. Part of the reason the OpenFirmware CIF got thrown
> away early in SPARC/PowerPC boot (after "flattening" the device tree
> using the CIF calls to parse it out) was because you had to disable
> the MMU, caches, interrupts etc. which caused all kinds of slow
> firmware code to be all kinds of extra-slow.

I prefer to see it as a way to not reinvent things that do not need
reinventing, while not adding more special-case code to the kernel.

> What that meant is nobody bothered to implement working, re-entrant,
> re-locatable firmware to a great degree. This ended up being a
> self-fulfilling prophecy of "don't trust the bootloader" and "get rid
> of it as soon as we can," which essentially meant Linux never took
> advantage of the resources available. In OF's case, the CIF sucked by
> specification. In UEFI's case here, it's been implemented in Linux in
> such a way that guarantees poor-performing firmware code with huge
> penalties to call them, which isn't even required by UEFI if the
> earlier boot code did the right things in the first place.

I don't follow. In which way does this implementation result in poor
performance or reduced functionality?

We deal with a highly quirky set of requirements for calling
SetVirtualAddressMap() in a clunky way - after which calls into UEFI
are direct and cachable.

/
    Leif

Re: [PATCH v3 1/3] Documentation: arm: add UEFI support documentation

[Matt Sealey]

On Mon, Dec 2, 2013 at 3:07 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Mon, Dec 02, 2013 at 01:51:22PM -0600, Matt Sealey wrote:
>> Here's where I think this whole thing falls down as being the weirdest
>> possible implementation of this. It defies logic to put this
>> information in the device tree /chosen node while also attempting to
>> boot the kernel using an EFI stub; the stub is going to have this
>> information because it is going to have the pointer to the system
>> System Table (since it was called by StartImage()). Why not stash the
>> System Table pointer somewhere safe in the stub?
>
> We do. In the DT.

Hang on... see way below about "reinventing the wheel"

>> The information in the device tree is all accessible from Boot
>> Services and as long as the System Table isn't being thrown away (my
>> suggestion would be.. stuff it in r2, and set r1 = "EFI\0" then work
>> with arch/arm/kernel/head{-common,}.S code to do the right thing)
>
> You left out the bit of redefining the kernel boot protocol to permit
> calling it with caches, MMU and interrupts enabled - also known as
> before ExitBootServices().

And that's a horrible idea because of what?

What's evident here is there could be two major ways to generate an
image that boots from a UEFI implementation;

* one whereby UEFI is jostled or coerced by second stage bootloader to
load a plain zImage and you lose all information about UEFI except in
the event that that information is preserved in the device tree by the
firmware
* one whereby a 'stock' UEFI is used and it boots only on UEFI because
it is in a format very reasonably only capable of being booted by UEFI
and, subordinately,
 - one where that plain zImage got glued to an EFI stub just like the
decompressor is glued to the Image
 - one where the kernel needs to be built with support for UEFI and
that somewhat changes the boot path

By the time we get half-way through arm/kernel/head.S the cache and
MMU has been turned off and on and off again by the decompressor, and
after a large amount of guesswork and arbitrary restriction-based
implementation, there's no guarantee that the kernel hasn't been
decompressed over some important UEFI feature or some memory hasn't
been trashed. You can't make that guarantee because by entering the
plain zImage, you forfeited that information. This is at worst case
going to be lots of blank screens and blinking serial console prompts
and little more than frustration..

Most of the guessing is ideally not required to be a guess at all, the
restrictions are purely to deal with the lack of trust for the
bootloader environment. Why can't we trust UEFI? Or at least hold it
to a higher standard. If someone ships a broken UEFI, they screw a
feature or have a horrible bug and ship it, laud the fact Linux
doesn't boot on it and the fact that it's their fault - over their
head. It actually works these days, Linux actually has "market share,"
companies really go out of their way to rescue their "image" and
resolve the situation when someone blogs about a serious UEFI bug on
their $1300 laptops, or even $300 tablets.

> Which is what we are going to implement anyway in order to permit
> firmware to supply DT hardware description in the same way as with
> ACPI. Yes, we could pass the system table pointer directly - but that
> doesn't get us the memory map.

Boot Services gives you the ability to get the memory map.. and the
kinds of things that live in those spots in the memory map. It's at
least a better guess than "I am located at a specific place and can
infer from linker data and masking off the bottom bits that there's
probably this amount of RAM that starts at this location or
thereabouts". It at least gives the ability to 'allocate' memory to
put the page table instead of having a firmware call walk all over it,
or having the kernel walk over some parts of firmware, or even not
have to do anything except link in a decompressor (eh, sure, it means
duplicating decompressor code in some cases, but I also don't think
it's a sane requirement to include the entire decompression suite in
the kernel proper if it only gets used once at early boot).

> I prefer to see it as a way to not reinvent things that do not need
> reinventing, while not adding more special-case code to the kernel.

Isn't putting the System Table pointer in the DT specifically
reinventing the UEFI boot process?

Booting from UEFI is a special case in itself.. the EFI stub here is
putting a round block in a square hole.

There are two much, much better solutions: put the round block in a
round hole. Put a square block in that square hole. We could do so
much better than gluing the round block into the square hole.

>> What that meant is nobody bothered to implement working, re-entrant,
>> re-locatable firmware to a great degree. This ended up being a
>> self-fulfilling prophecy of "don't trust the bootloader" and "get rid
>> of it as soon as we can," which essentially meant Linux never took
>> advantage of the resources available. In OF's case, the CIF sucked by
>> specification. In UEFI's case here, it's been implemented in Linux in
>> such a way that guarantees poor-performing firmware code with huge
>> penalties to call them, which isn't even required by UEFI if the
>> earlier boot code did the right things in the first place.
>
> I don't follow. In which way does this implementation result in poor
> performance or reduced functionality?

I believe what I am trying to object to is this weird process of
getting to a state where you can get to UEFI, and why anyone would
bother gluing the existing Linux kernel image to the back of an
externally-built stub, only to do some really quite obnoxious tricks
to enable it to go into a decompressor and then through, kernel setup
head, that make a bunch of assumptions about the bootloader interface,
then to try and recover the information that got thrown away and THEN
attempt to reinstate some kind of UEFI functionality.

If your platform has UEFI, then your platform has UEFI - if you built
a multiplatform kernel that needs to boot on U-Boot, then you glued an
EFI stub to it to make it boot. At some point between the stub and the
runtime services driver, you're going through 10,000 lines of code
with the information that it *is* running on top of UEFI completely
lost to the boot process.

I believe I am also objecting to the idea that the way this is BEST
implemented is to take a stock zImage (decompressor+Image payload) and
glue a stub in front to resolve the interface issue when the
implication is extra complication to the boot process.

By not actually using it, nobody actually bothered to improve the
firmware or fix bugs in the places where it could have been used. This
ends up as a self-fulfilling prophecy of exhausting amounts of broken
and unoptimized firmware.

Nobody in firmware-land has any impetus to fix those bugs or add
useful optional features.

By "by not actually using it," I do mean the case where someone has
UEFI and somehow boots a plain zImage and a DTB modified to include
the System Table pointer. Because that door is completely wide open..

Personally I think having a well known environment at StartImage()
jumping to your EFI application entry point is a great place to
simplify the decompressor by integrating it into the stub.

At the point you then jump into kernel/head.S - you can still know
you're on UEFI, with data in r1 and r2 strongly implying this is UEFI,
you can branch to a much, MUCH simpler path for initialization where
quite a lot of the work it's trying to do may have already been
performed by the stub., and quite a lot of the bare-metalling doesn't
need to be done.

I am sure, even if modifying head.S for any reason than to fix a bug
or implement some architectural requirement is somehow frowned upon,
that comparing r1 to a known constant machine id and branching to a
uefi_start() (which, at that point, may as well be a C function, if
the stub saw fit to keep around/throw in an early stack) is not going
to cause anyone any problems (even if it does add 4 instructions to
the entry and slow everyone else down by a nanosecond or two).

Everybody keeps their absolutely fixed entry point to the image
proper, that way, so you can still glue your stub (with or without the
decompressor as part of the stub) to the front with no changes to the
build process for the image or the code path for non-UEFI.. one
conditional branch and you can gain a lot of much, much easier to
maintain boot process..

> We deal with a highly quirky set of requirements for calling
> SetVirtualAddressMap() in a clunky way - after which calls into UEFI
> are direct and cachable.

If the kernel boot process now has been derived from years upon years
of trial and error and engineering, then it does seem a shame to go do
things a different way, you would be right to say it would be a shame
not to promote code-reuse of the existing process by not touching the
zImage stuff or core kernel boot, and just working on the glue and
some not-so-early-init code.

But what it does is make the boot process *more* complicated than it's
already complicated implementation, in the face of a very nice
specification of the correct way to deal with booting something from a
UEFI implementation..

What might be a much better route to take could be to define a nice,
shiny new way of getting Linux to the point that it has full control
over it's own destiny which does a hell of a lot less, with a less
schizophrenic view of using UEFI or not.

Ta,
Matt Sealey <neko@bakuhatsu.net>

Re: [PATCH v3 1/3] Documentation: arm: add UEFI support documentation

[Mark Salter]

On Wed, 2013-12-04 at 15:06 -0600, Matt Sealey wrote:
> On Mon, Dec 2, 2013 at 3:07 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Mon, Dec 02, 2013 at 01:51:22PM -0600, Matt Sealey wrote:
> >> Here's where I think this whole thing falls down as being the weirdest
> >> possible implementation of this. It defies logic to put this
> >> information in the device tree /chosen node while also attempting to
> >> boot the kernel using an EFI stub; the stub is going to have this
> >> information because it is going to have the pointer to the system
> >> System Table (since it was called by StartImage()). Why not stash the
> >> System Table pointer somewhere safe in the stub?
> >
> > We do. In the DT.
> 
> Hang on... see way below about "reinventing the wheel"
> 
> >> The information in the device tree is all accessible from Boot
> >> Services and as long as the System Table isn't being thrown away (my
> >> suggestion would be.. stuff it in r2, and set r1 = "EFI\0" then work
> >> with arch/arm/kernel/head{-common,}.S code to do the right thing)
> >
> > You left out the bit of redefining the kernel boot protocol to permit
> > calling it with caches, MMU and interrupts enabled - also known as
> > before ExitBootServices().
> 
> And that's a horrible idea because of what?

Talk about reinventing the wheel.

I look at it like this. UEFI applications have a specific boot protocol.
The kernel has a different boot protocol. The purpose of the stub is to
go from the UEFI protocol to the kernel protocol. The kernel protocol
doesn't currently include an explicit way to pass UEFI info (system table
and memory map). It does have a way to pass a DT. Much like x86 and ia64
pass the UEFI info in an already existing boot_params block, arm and arm64
pass that info in the device tree. Not changing the kernel boot protocol
seems like the simplest and best way to get the job done. Maybe x86 and
now arm are going about the wrong way and should be doing it differently,
but so far, I'm not convinced that is the case.

--Mark

Re: [PATCH v3 1/3] Documentation: arm: add UEFI support documentation

[Matthew Garrett]

On Wed, Dec 04, 2013 at 03:06:47PM -0600, Matt Sealey wrote:

> there's no guarantee that the kernel hasn't been decompressed over 
> some important UEFI feature or some memory hasn't been trashed. You 
> can't make that guarantee because by entering the plain zImage, you 
> forfeited that information.

The stub is responsible for ensuring that the compressed kernel is 
loaded at a suitable address. Take a look at efi_relocate_kernel().

> Most of the guessing is ideally not required to be a guess at all, the
> restrictions are purely to deal with the lack of trust for the
> bootloader environment. Why can't we trust UEFI? Or at least hold it
> to a higher standard. If someone ships a broken UEFI, they screw a
> feature or have a horrible bug and ship it, laud the fact Linux
> doesn't boot on it and the fact that it's their fault - over their
> head. It actually works these days, Linux actually has "market share,"
> companies really go out of their way to rescue their "image" and
> resolve the situation when someone blogs about a serious UEFI bug on
> their $1300 laptops, or even $300 tablets.

Yeah, that hasn't actually worked out too well for us.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH v3 1/3] Documentation: arm: add UEFI support documentation

[Grant Likely]

On Mon, 2 Dec 2013 13:51:22 -0600, Matt Sealey <neko@bakuhatsu.net> wrote:
> > +UEFI kernel support on ARM
> > +==========================
> > +UEFI kernel support on the ARM architectures (arm and arm64) is only available
> > +when boot is performed through the stub.
> > +
> > +The stub populates the FDT /chosen node with (and the kernel scans for) the
> > +following parameters:
> > +________________________________________________________________________________
> > +Name                      | Size   | Description
> > +================================================================================
> > +linux,uefi-system-table   | 64-bit | Physical address of the UEFI System Table.
> > +--------------------------------------------------------------------------------
> > +linux,uefi-mmap-start     | 64-bit | Physical address of the UEFI memory map,
> > +                          |        | populated by the UEFI GetMemoryMap() call.
> > +--------------------------------------------------------------------------------
> > +linux,uefi-mmap-size      | 32-bit | Size in bytes of the UEFI memory map
> > +                          |        | pointed to in previous entry.
> > +--------------------------------------------------------------------------------
> > +linux,uefi-mmap-desc-size | 32-bit | Size in bytes of each entry in the UEFI
> > +                          |        | memory map.
> > +--------------------------------------------------------------------------------
> > +linux,uefi-mmap-desc-ver  | 32-bit | Version of the mmap descriptor format.
> > +--------------------------------------------------------------------------------
> > +linux,uefi-stub-kern-ver  | string | Copy of linux_banner from build.
> > +--------------------------------------------------------------------------------
> 
> This flies in the face of actually using #address-cells and
> #size-cells to define how big addresses and sizes are. You're limited
> here by the root node definitions... that's the spec.
> 
> Here's where I think this whole thing falls down as being the weirdest
> possible implementation of this. It defies logic to put this
> information in the device tree /chosen node while also attempting to
> boot the kernel using an EFI stub; the stub is going to have this
> information because it is going to have the pointer to the system
> System Table (since it was called by StartImage()). Why not stash the
> System Table pointer somewhere safe in the stub?

Everything here is about getting from the stub to the kernel. We have a
boot interface for the kernel proper. It works, and this patch set works
with it. Also, this is effectively a kernel-internal interface between
the stub and the kernel so there aren't any ABI issues that we need to
deail with.

Go ahead and propose patches for a better interface, but in the mean
time I see no reason not to merge this series.

g.

Re: [PATCH v3 1/3] Documentation: arm: add UEFI support documentation

[Grant Likely]

On Wed, 4 Dec 2013 15:06:47 -0600, Matt Sealey <neko@bakuhatsu.net> wrote:
> If your platform has UEFI, then your platform has UEFI - if you built
> a multiplatform kernel that needs to boot on U-Boot, then you glued an
> EFI stub to it to make it boot. At some point between the stub and the
> runtime services driver, you're going through 10,000 lines of code
> with the information that it *is* running on top of UEFI completely
> lost to the boot process.
> 
> I believe I am also objecting to the idea that the way this is BEST
> implemented is to take a stock zImage (decompressor+Image payload) and
> glue a stub in front to resolve the interface issue when the
> implication is extra complication to the boot process.

Adding UEFI support to an existing image type was a design goal when we
started. Having yet another image format which is not compatibile with
existing firmware adds yet another barrier to migrating from U-Boot to
UEFI, or to supporting multiplatforms.

g.

Re: [PATCH v3 1/3] Documentation: arm: add UEFI support documentation

[Grant Likely]

On Thu, 28 Nov 2013 16:41:21 +0000, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> This patch provides documentation of the [U]EFI runtime service and
> configuration features for the arm architecture.
> 
> Changes since v1/v2:
> - Complete rewrite.
> - New FDT bindings.
> 
> Cc: Rob Landley <rob@landley.net>
> Cc: linux-doc@vger.kernel.org
> 
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>

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

With a few minor comments below.

g.

> ---
>  Documentation/arm/00-INDEX |    3 +++
>  Documentation/arm/uefi.txt |   61 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 64 insertions(+)
>  create mode 100644 Documentation/arm/uefi.txt
> 
> diff --git a/Documentation/arm/00-INDEX b/Documentation/arm/00-INDEX
> index 36420e1..b3af704 100644
> --- a/Documentation/arm/00-INDEX
> +++ b/Documentation/arm/00-INDEX
> @@ -34,3 +34,6 @@ nwfpe/
>  	- NWFPE floating point emulator documentation
>  swp_emulation
>  	- SWP/SWPB emulation handler/logging description
> +
> +uefi.txt
> +	- [U]EFI configuration and runtime services documentation
> diff --git a/Documentation/arm/uefi.txt b/Documentation/arm/uefi.txt
> new file mode 100644
> index 0000000..9ba59509
> --- /dev/null
> +++ b/Documentation/arm/uefi.txt
> @@ -0,0 +1,61 @@
> +UEFI, the Unified Extensible Firmware Interface is, a specification

Nit: Comma in the wrong place.

> +governing the behaviours of compatible firmware interfaces. It is
> +maintained by the UEFI Forum - http://www.uefi.org/.
> +
> +UEFI is an evolution of its predecessor 'EFI', so the terms EFI and
> +UEFI are used somewhat interchangeably in this document and associated
> +source code. As a rule, anything new uses 'UEFI', whereas 'EFI' refers
> +to legacy code or specifications.
> +
> +UEFI support in Linux
> +=====================
> +Booting on a platform with firmware compliant with the UEFI specification
> +makes it possible for the kernel to support additional features:
> +- UEFI Runtime Services
> +- Retrieving various configuration information through the standardised
> +  interface of UEFI configuration tables. (ACPI, SMBIOS, ...)
> +
> +For actually enabling [U]EFI support, enable:
> +- CONFIG_EFI=y
> +- CONFIG_EFI_VARS=y or m
> +
> +The implementation depends on receiving information about the UEFI environment
> +in a Flattened Device Tree (FDT) - so is only available with CONFIG_OF.
> +
> +UEFI stub
> +=========
> +The "stub" is a feature that turns the Image/zImage into a valid UEFI PE/COFF

Nit: s/turns/extends/

> +executable, including a loader application that makes it possible to load the
> +kernel directly from the UEFI shell, boot menu, or one of the lightweight
> +bootloaders like Gummiboot or rEFInd.
> +
> +The kernel image built with stub support remains a valid kernel image for
> +booting in non-UEFI environments.
> +
> +UEFI kernel support on ARM
> +==========================
> +UEFI kernel support on the ARM architectures (arm and arm64) is only available
> +when boot is performed through the stub.
> +
> +The stub populates the FDT /chosen node with (and the kernel scans for) the
> +following parameters:
> +________________________________________________________________________________
> +Name                      | Size   | Description
> +================================================================================
> +linux,uefi-system-table   | 64-bit | Physical address of the UEFI System Table.
> +--------------------------------------------------------------------------------
> +linux,uefi-mmap-start     | 64-bit | Physical address of the UEFI memory map,
> +                          |        | populated by the UEFI GetMemoryMap() call.
> +--------------------------------------------------------------------------------
> +linux,uefi-mmap-size      | 32-bit | Size in bytes of the UEFI memory map
> +                          |        | pointed to in previous entry.
> +--------------------------------------------------------------------------------
> +linux,uefi-mmap-desc-size | 32-bit | Size in bytes of each entry in the UEFI
> +                          |        | memory map.
> +--------------------------------------------------------------------------------
> +linux,uefi-mmap-desc-ver  | 32-bit | Version of the mmap descriptor format.
> +--------------------------------------------------------------------------------
> +linux,uefi-stub-kern-ver  | string | Copy of linux_banner from build.
> +--------------------------------------------------------------------------------
> +
> +For verbose debug messages, specify 'uefi_debug' on the kernel command line.
> -- 
> 1.7.10.4
> 

Re: [PATCH v3 2/3] arm: Add [U]EFI runtime services support

[Grant Likely]

On Thu, 28 Nov 2013 16:41:22 +0000, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> This patch implements basic support for UEFI runtime services in the
> ARM architecture - a requirement for using efibootmgr to read and update
> the system boot configuration.
> 
> It uses the generic configuration table scanning to populate ACPI and
> SMBIOS pointers.
> 
> Changes since v2:
> - Updated FDT bindings.
> - Preserve regions marked RESERVED (but don't map them).
> - Rename 'efi' -> 'uefi' within this new port (leaving core code as is).
> 
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>

Hi Leif,

I've made a bunch of comments below. I've got concerns about the amount
of casting going on in this code, but the rest are pretty minor

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

> ---
>  arch/arm/Kconfig            |   15 ++
>  arch/arm/include/asm/uefi.h |   22 ++
>  arch/arm/kernel/Makefile    |    2 +
>  arch/arm/kernel/setup.c     |    6 +
>  arch/arm/kernel/uefi.c      |  469 +++++++++++++++++++++++++++++++++++++++++++
>  arch/arm/kernel/uefi_phys.S |   59 ++++++
>  include/linux/efi.h         |    2 +-
>  7 files changed, 574 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/include/asm/uefi.h
>  create mode 100644 arch/arm/kernel/uefi.c
>  create mode 100644 arch/arm/kernel/uefi_phys.S
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 78a79a6a..db8d212 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1853,6 +1853,19 @@ config EARLY_IOREMAP
>  	  the same virtual memory range as kmap so all early mappings must
>  	  be unapped before paging_init() is called.
>  
> +config EFI
> +	bool "UEFI runtime service support"
> +	depends on OF && !CPU_BIG_ENDIAN
> +	select UCS2_STRING
> +	select EARLY_IOREMAP
> +	---help---
> +	  This enables the kernel to use UEFI runtime services that are
> +	  available (such as the UEFI variable services).
> +
> +	  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 SECCOMP
>  	bool
>  	prompt "Enable seccomp to safely compute untrusted bytecode"
> @@ -2272,6 +2285,8 @@ source "net/Kconfig"
>  
>  source "drivers/Kconfig"
>  
> +source "drivers/firmware/Kconfig"
> +
>  source "fs/Kconfig"
>  
>  source "arch/arm/Kconfig.debug"
> diff --git a/arch/arm/include/asm/uefi.h b/arch/arm/include/asm/uefi.h
> new file mode 100644
> index 0000000..519ca18
> --- /dev/null
> +++ b/arch/arm/include/asm/uefi.h
> @@ -0,0 +1,22 @@
> +#ifndef _ASM_ARM_EFI_H
> +#define _ASM_ARM_EFI_H
> +
> +#include <asm/mach/map.h>
> +
> +extern int uefi_memblock_arm_reserve_range(void);
> +
> +typedef efi_status_t uefi_phys_call_t(u32 memory_map_size,
> +				      u32 descriptor_size,
> +				      u32 descriptor_version,
> +				      efi_memory_desc_t *dsc,
> +				      efi_set_virtual_address_map_t *f);
> +
> +extern efi_status_t uefi_phys_call(u32, u32, u32, efi_memory_desc_t *,
> +				   efi_set_virtual_address_map_t *);
> +
> +#define uefi_remap(cookie, size) __arm_ioremap((cookie), (size), MT_MEMORY)
> +#define uefi_ioremap(cookie, size) __arm_ioremap((cookie), (size), MT_DEVICE)
> +#define uefi_unmap(cookie) __arm_iounmap((cookie))
> +#define uefi_iounmap(cookie) __arm_iounmap((cookie))
> +
> +#endif /* _ASM_ARM_EFI_H */
> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> index a30fc9b..736cce4 100644
> --- a/arch/arm/kernel/Makefile
> +++ b/arch/arm/kernel/Makefile
> @@ -98,4 +98,6 @@ obj-y				+= psci.o
>  obj-$(CONFIG_SMP)		+= psci_smp.o
>  endif
>  
> +obj-$(CONFIG_EFI)		+= uefi.o uefi_phys.o
> +
>  extra-y := $(head-y) vmlinux.lds
> diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> index 04c1757..9d44edd 100644
> --- a/arch/arm/kernel/setup.c
> +++ b/arch/arm/kernel/setup.c
> @@ -30,6 +30,7 @@
>  #include <linux/bug.h>
>  #include <linux/compiler.h>
>  #include <linux/sort.h>
> +#include <linux/efi.h>
>  
>  #include <asm/unified.h>
>  #include <asm/cp15.h>
> @@ -57,6 +58,7 @@
>  #include <asm/unwind.h>
>  #include <asm/memblock.h>
>  #include <asm/virt.h>
> +#include <asm/uefi.h>
>  
>  #include "atags.h"
>  
> @@ -898,6 +900,10 @@ void __init setup_arch(char **cmdline_p)
>  	sanity_check_meminfo();
>  	arm_memblock_init(&meminfo, mdesc);
>  
> +#ifdef CONFIG_EFI
> +	uefi_memblock_arm_reserve_range();
> +#endif
> +

#ifdef block in code? You know better. :-) Make an empty stub in the
header files instead.

>  	paging_init(mdesc);
>  	request_standard_resources(mdesc);
>  
> diff --git a/arch/arm/kernel/uefi.c b/arch/arm/kernel/uefi.c
> new file mode 100644
> index 0000000..f771026
> --- /dev/null
> +++ b/arch/arm/kernel/uefi.c
> @@ -0,0 +1,469 @@
> +/*
> + * Based on Unified Extensible Firmware Interface Specification version 2.3.1
> + *
> + * Copyright (C) 2013  Linaro 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.
> + */
> +
> +#include <linux/efi.h>
> +#include <linux/export.h>
> +#include <linux/memblock.h>
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +
> +#include <asm/cacheflush.h>
> +#include <asm/idmap.h>
> +#include <asm/tlbflush.h>
> +#include <asm/uefi.h>
> +
> +struct efi_memory_map memmap;
> +
> +static efi_runtime_services_t *runtime;
> +
> +static phys_addr_t uefi_system_table;
> +static phys_addr_t uefi_boot_mmap;
> +static u32 uefi_boot_mmap_size;
> +static u32 uefi_mmap_desc_size;
> +static u32 uefi_mmap_desc_ver;
> +
> +static unsigned long arm_uefi_facility;
> +
> +/*
> + * If you're planning to wire up a debugger and debug the UEFI side ...
> + */
> +#undef KEEP_ALL_REGIONS
> +
> +/*
> + * If you need to (temporarily) support buggy firmware.
> + */
> +#define KEEP_BOOT_SERVICES_REGIONS
> +
> +/*
> + * Returns 1 if 'facility' is enabled, 0 otherwise.
> + */
> +int efi_enabled(int facility)
> +{
> +	return test_bit(facility, &arm_uefi_facility) != 0;
> +}
> +EXPORT_SYMBOL(efi_enabled);
> +
> +static int uefi_debug __initdata;
> +static int __init uefi_debug_setup(char *str)
> +{
> +	uefi_debug = 1;
> +
> +	return 0;
> +}
> +early_param("uefi_debug", uefi_debug_setup);
> +
> +static struct {
> +	const char *name;
> +	const char *propname;
> +	int numcells;
> +	void *target;
> +} dt_params[] = {
> +	{
> +		"UEFI System Table", "linux,uefi-system-table",
> +		2, &uefi_system_table
> +	}, {
> +		"UEFI mmap address", "linux,uefi-mmap-start",
> +		2, &uefi_boot_mmap
> +	}, {
> +		"UEFI mmap size", "linux,uefi-mmap-size",
> +		1, &uefi_boot_mmap_size
> +	}, {
> +		"UEFI mmap descriptor size", "linux,uefi-mmap-desc-size",
> +		1, &uefi_mmap_desc_size
> +	}, {
> +		"UEFI mmap descriptor version", "linux,uefi-mmap-desc-ver",
> +		1, &uefi_mmap_desc_ver
> +	}
> +};
> +
> +static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
> +				       int depth, void *data)
> +{
> +	void *prop;
> +	int i;
> +
> +	if (depth != 1 ||
> +	    (strcmp(uname, "chosen") != 0 && strcmp(uname, "chosen@0") != 0))
> +		return 0;
> +
> +	pr_info("Getting UEFI parameters from FDT.\n");
> +
> +	for (i = 0; i < sizeof(dt_params)/sizeof(*dt_params); i++) {
> +		prop = of_get_flat_dt_prop(node, dt_params[i].propname, NULL);
> +		if (!prop)
> +			return 0;
> +		if (dt_params[i].numcells == 1) {
> +			u32 *target = dt_params[i].target;
> +			*target = of_read_ulong(prop, 1);
> +		} else {
> +			u64 *target = dt_params[i].target;
> +			*target = of_read_number(prop, 2);
> +		}
> +
> +		if (uefi_debug)
> +			pr_info("  %s @ 0x%08x\n", dt_params[i].name,
> +				*((u32 *)dt_params[i].target));
> +	}
> +
> +	return 1;
> +}
> +
> +static int __init uefi_init(void)
> +{
> +	efi_char16_t *c16;
> +	char vendor[100] = "unknown";
> +	int i, retval;
> +
> +	efi.systab = early_memremap(uefi_system_table,
> +				    sizeof(efi_system_table_t));
> +
> +	/*
> +	 * Verify the UEFI System Table
> +	 */
> +	if (efi.systab == NULL)
> +		panic("Whoa! Can't find UEFI system table.\n");
> +	if (efi.systab->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
> +		panic("Whoa! UEFI system table signature incorrect\n");

Do you really want to panic here? The kernel may be able to continue
with the data in the DT. I would move these tests into
uefi_memblock_arm_reserve_range() and let it bail out doing nothing if
the tests fail (although it is good to print a warning).

> +	if ((efi.systab->hdr.revision >> 16) == 0)
> +		pr_warn("Warning: UEFI system table version %d.%02d, expected 1.00 or greater\n",
> +			efi.systab->hdr.revision >> 16,
> +			efi.systab->hdr.revision & 0xffff);
> +
> +	/* Show what we know for posterity */
> +	c16 = (efi_char16_t *)early_memremap(efi.systab->fw_vendor,
> +					    sizeof(vendor));

If you have to resort to a cast, then you're doing it wrong.
early_memremap() should already return a void*. Does that not work here?

> +	if (c16) {
> +		for (i = 0; i < (int) sizeof(vendor) - 1 && *c16; ++i)
> +			vendor[i] = c16[i];
> +		vendor[i] = '\0';
> +	}

We need a better parser here, or at least filter out non-printable
characters. The best would be a conversion to UTF-8 I think (not that
we're doing that on the rest of UEFI code right now, so I would put this
as a followup item).

> +
> +	pr_info("UEFI v%u.%.02u by %s\n",
> +		efi.systab->hdr.revision >> 16,
> +		efi.systab->hdr.revision & 0xffff, vendor);
> +
> +	retval = efi_config_init(NULL);
> +	if (retval == 0)
> +		set_bit(EFI_CONFIG_TABLES, &arm_uefi_facility);
> +
> +	early_iounmap(c16, sizeof(vendor));
> +	early_iounmap(efi.systab,  sizeof(efi_system_table_t));
> +
> +	return retval;
> +}
> +
> +static __init int is_discardable_region(efi_memory_desc_t *md)
> +{
> +#ifdef KEEP_ALL_REGIONS
> +	return 0;
> +#endif
> +
> +	if (md->attribute & EFI_MEMORY_RUNTIME)
> +		return 0;
> +
> +	switch (md->type) {
> +#ifdef KEEP_BOOT_SERVICES_REGIONS
> +	case EFI_BOOT_SERVICES_CODE:
> +	case EFI_BOOT_SERVICES_DATA:
> +#endif

Having the #ifdef in code is gross. I would leave the block in place,
but make the return value configured by a #define value.

ie.

#define DISCARD_BOOT_SERVICES_REGIONS 1 /* Can be set to 0 */

...

	case EFI_BOOT_SERVICES_CODE:
	case EFI_BOOT_SERVICES_DATA:
		return DISCARD_BOOT_SERVICES_REGIONS;

> +	/* Keep tables around for any future kexec operations */
> +	case EFI_ACPI_RECLAIM_MEMORY:
> +		return 0;
> +	/* Preserve */
> +	case EFI_RESERVED_TYPE:
> +		return 0;
> +	}
> +
> +	return 1;
> +}
> +
> +static __initdata struct {
> +	u32 type;
> +	const char *name;
> +}  memory_type_name_map[] = {
> +	{EFI_RESERVED_TYPE, "reserved"},
> +	{EFI_LOADER_CODE, "loader code"},
> +	{EFI_LOADER_DATA, "loader data"},
> +	{EFI_BOOT_SERVICES_CODE, "boot services code"},
> +	{EFI_BOOT_SERVICES_DATA, "boot services data"},
> +	{EFI_RUNTIME_SERVICES_CODE, "runtime services code"},
> +	{EFI_RUNTIME_SERVICES_DATA, "runtime services data"},
> +	{EFI_CONVENTIONAL_MEMORY, "conventional memory"},
> +	{EFI_UNUSABLE_MEMORY, "unusable memory"},
> +	{EFI_ACPI_RECLAIM_MEMORY, "ACPI reclaim memory"},
> +	{EFI_ACPI_MEMORY_NVS, "ACPI memory nvs"},
> +	{EFI_MEMORY_MAPPED_IO, "memory mapped I/O"},
> +	{EFI_MEMORY_MAPPED_IO_PORT_SPACE, "memory mapped I/O port space"},
> +	{EFI_PAL_CODE, "pal code"},
> +	{EFI_MAX_MEMORY_TYPE, NULL},
> +};

Can this be in common code?

> +
> +static __init void remove_sections(phys_addr_t addr, unsigned long size)
> +{
> +	unsigned long section_offset;
> +	unsigned long num_sections;
> +
> +	section_offset = addr - (addr & SECTION_MASK);
> +	num_sections = size / SECTION_SIZE;
> +	if (size % SECTION_SIZE)
> +		num_sections++;
> +
> +	memblock_remove(addr - section_offset, num_sections * SECTION_SIZE);
> +}
> +
> +static __init int remove_regions(void)
> +{
> +	efi_memory_desc_t *md;
> +	int count = 0;
> +	void *p, *e;
> +
> +	if (uefi_debug)
> +		pr_info("Processing UEFI memory map:\n");
> +
> +	memmap.phys_map = early_memremap((phys_addr_t)(u32) uefi_boot_mmap,
> +					 uefi_boot_mmap_size);

Double casting? Why isn't only "(phys_addr_t)" sufficient? In fact,
uefi_boot_mmap is already phys_addr_t. Why is the cast needed at all?

> +	if (!memmap.phys_map)
> +		return 1;
> +
> +	p = memmap.phys_map;
> +	e = (void *)((u32)p + uefi_boot_mmap_size);
> +	for (; p < e; p += uefi_mmap_desc_size) {

Again, the casting looks wrong. p is a void pointer. Adding
uefi_boot_mmap_size without casting should be just fine. I would do the
following to be more readable:

e = memmap.phys_map + uefi_boot_mmap_size;
for (p = memmap.phys_map; p < e; p += uefi_mmap_desc_size)

> +		md = p;
> +		if (is_discardable_region(md))
> +			continue;
> +
> +		if (uefi_debug)
> +			pr_info("  %8llu pages @ %016llx (%s)\n",
> +				md->num_pages, md->phys_addr,
> +				memory_type_name_map[md->type].name);
> +
> +		if (md->type != EFI_MEMORY_MAPPED_IO) {
> +			remove_sections(md->phys_addr,
> +					md->num_pages * PAGE_SIZE);
> +			count++;
> +		}
> +		memmap.nr_map++;
> +	}
> +
> +	early_iounmap(memmap.phys_map, uefi_boot_mmap_size);
> +
> +	if (uefi_debug)
> +		pr_info("%d regions preserved.\n", memmap.nr_map);
> +
> +	return 0;
> +}
> +
> +int __init uefi_memblock_arm_reserve_range(void)
> +{
> +	if (!of_scan_flat_dt(fdt_find_uefi_params, NULL))
> +		return 0;
> +
> +	set_bit(EFI_BOOT, &arm_uefi_facility);
> +
> +	uefi_init();
> +
> +	remove_regions();
> +
> +	return 0;
> +}
> +
> +/*
> + * Disable instrrupts, enable idmap and disable caches.
> + */
> +static void __init phys_call_prologue(void)
> +{
> +	local_irq_disable();
> +
> +	/* Take out a flat memory mapping. */
> +	setup_mm_for_reboot();
> +
> +	/* Clean and invalidate caches */
> +	flush_cache_all();
> +
> +	/* Turn off caching */
> +	cpu_proc_fin();
> +
> +	/* Push out any further dirty data, and ensure cache is empty */
> +	flush_cache_all();
> +}
> +
> +/*
> + * Restore original memory map and re-enable interrupts.
> + */
> +static void __init phys_call_epilogue(void)
> +{
> +	static struct mm_struct *mm = &init_mm;
> +
> +	/* Restore original memory mapping */
> +	cpu_switch_mm(mm->pgd, mm);
> +
> +	/* Flush branch predictor and TLBs */
> +	local_flush_bp_all();
> +#ifdef CONFIG_CPU_HAS_ASID
> +	local_flush_tlb_all();
> +#endif

Empty stub please

> +
> +	local_irq_enable();
> +}
> +
> +static int __init remap_region(efi_memory_desc_t *md, efi_memory_desc_t *entry)
> +{
> +	u64 va;
> +	u64 paddr;
> +	u64 size;
> +
> +	*entry = *md;
> +	paddr = entry->phys_addr;
> +	size = entry->num_pages << EFI_PAGE_SHIFT;
> +
> +	/*
> +	 * Map everything writeback-capable as coherent memory,
> +	 * anything else as device.
> +	 */
> +	if (md->attribute & EFI_MEMORY_WB)
> +		va = (u64)((u32)uefi_remap(paddr, size) & 0xffffffffUL);
> +	else
> +		va = (u64)((u32)uefi_ioremap(paddr, size) & 0xffffffffUL);

Casting? Is it appropriate to throw away the upper 32 bits? On a large
memory system could that mean aliasing high regions into low?

> +	if (!va)
> +		return 0;
> +	entry->virt_addr = va;
> +
> +	if (uefi_debug)
> +		pr_info("  %016llx-%016llx => 0x%08x : (%s)\n",
> +			paddr, paddr + size - 1, (u32)va,
> +			md->attribute &  EFI_MEMORY_WB ? "WB" : "I/O");
> +
> +	return 1;
> +}
> +
> +static int __init remap_regions(void)
> +{
> +	void *p, *next;
> +	efi_memory_desc_t *md;
> +
> +	memmap.phys_map = uefi_remap(uefi_boot_mmap, uefi_boot_mmap_size);
> +	if (!memmap.phys_map)
> +		return 0;
> +	memmap.map_end = (void *)memmap.phys_map + uefi_boot_mmap_size;
> +	memmap.desc_size = uefi_mmap_desc_size;
> +	memmap.desc_version = uefi_mmap_desc_ver;
> +
> +	/* Allocate space for the physical region map */
> +	memmap.map = kzalloc(memmap.nr_map * memmap.desc_size, GFP_KERNEL);
> +	if (!memmap.map)
> +		return 0;
> +
> +	next = memmap.map;
> +	for (p = memmap.phys_map; p < memmap.map_end; p += memmap.desc_size) {
> +		md = p;
> +		if (is_discardable_region(md) || md->type == EFI_RESERVED_TYPE)
> +			continue;
> +
> +		if (!remap_region(p, next))
> +			return 0;
> +
> +		next += memmap.desc_size;
> +	}
> +
> +	memmap.map_end = next;
> +	efi.memmap = &memmap;
> +
> +	uefi_unmap(memmap.phys_map);
> +	memmap.phys_map = efi_lookup_mapped_addr(uefi_boot_mmap);
> +	efi.systab = efi_lookup_mapped_addr(uefi_system_table);
> +	if (efi.systab)
> +		set_bit(EFI_SYSTEM_TABLES, &arm_uefi_facility);
> +	/*
> +	 * efi.systab->runtime is a 32-bit pointer to something guaranteed by
> +	 * the UEFI specification to be 1:1 mapped in a 4GB address space.
> +	 */
> +	runtime = efi_lookup_mapped_addr((u32)efi.systab->runtime);
> +
> +	return 1;
> +}
> +
> +
> +/*
> + * This function switches the UEFI runtime services to virtual mode.
> + * This operation must be performed only once in the system's lifetime,
> + * including any kecec calls.
> + *
> + * This must be done with a 1:1 mapping. The current implementation
> + * resolves this by disabling the MMU.
> + */
> +efi_status_t  __init phys_set_virtual_address_map(u32 memory_map_size,
> +						  u32 descriptor_size,
> +						  u32 descriptor_version,
> +						  efi_memory_desc_t *dsc)
> +{
> +	uefi_phys_call_t *phys_set_map;
> +	efi_status_t status;
> +
> +	phys_call_prologue();
> +
> +	phys_set_map = (void *)(unsigned long)virt_to_phys(uefi_phys_call);
> +
> +	/* Called with caches disabled, returns with caches enabled */
> +	status = phys_set_map(memory_map_size, descriptor_size,
> +			      descriptor_version, dsc,
> +			      efi.set_virtual_address_map)
> +;
> +	phys_call_epilogue();
> +
> +	return status;
> +}
> +
> +/*
> + * Called explicitly from init/mm.c
> + */
> +void __init efi_enter_virtual_mode(void)
> +{
> +	efi_status_t status;
> +
> +	if (!efi_enabled(EFI_BOOT)) {
> +		pr_info("UEFI services will not be available.\n");
> +		return;
> +	}
> +
> +	pr_info("Remapping and enabling UEFI services.\n");
> +
> +	/* Map the regions we memblock_remove:d earlier into kernel
> +	   address space */
> +	if (!remap_regions()) {
> +		pr_info("Failed to remap UEFI regions - runtime services will not be available.\n");
> +		return;
> +	}
> +
> +	/* Call SetVirtualAddressMap with the physical address of the map */
> +	efi.set_virtual_address_map =
> +		(efi_set_virtual_address_map_t *)
> +		runtime->set_virtual_address_map;

I don't think the cast is necessary.

> +	memmap.phys_map =
> +		(efi_memory_desc_t *)(u32) __virt_to_phys((u32)memmap.map);

casting?

> +
> +	status = phys_set_virtual_address_map(memmap.nr_map * memmap.desc_size,
> +					      memmap.desc_size,
> +					      memmap.desc_version,
> +					      memmap.phys_map);
> +
> +	if (status != EFI_SUCCESS) {
> +		pr_info("Failed to set UEFI virtual address map!\n");
> +		return;
> +	}
> +
> +	/* Set up function pointers for efivars */
> +	efi.get_variable = (efi_get_variable_t *)runtime->get_variable;
> +	efi.get_next_variable =
> +		(efi_get_next_variable_t *)runtime->get_next_variable;
> +	efi.set_variable = (efi_set_variable_t *)runtime->set_variable;
> +	set_bit(EFI_RUNTIME_SERVICES, &arm_uefi_facility);

ditto

> +}
> diff --git a/arch/arm/kernel/uefi_phys.S b/arch/arm/kernel/uefi_phys.S
> new file mode 100644
> index 0000000..e9a1542
> --- /dev/null
> +++ b/arch/arm/kernel/uefi_phys.S
> @@ -0,0 +1,59 @@
> +/*
> + * arch/arm/kernel/uefi_phys.S
> + *
> + * Copyright (C) 2013  Linaro 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.
> + */
> +
> +#include <linux/linkage.h>
> +#define PAR_MASK 0xfff
> +
> +	.text
> +@ uefi_phys_call(a, b, c, d, *f)
> +	.align  5
> +        .pushsection    .idmap.text, "ax"
> +ENTRY(uefi_phys_call)
> +	@ Save physical context
> +	mov	r12, sp
> +	push	{r4-r5, r12, lr}
> +
> +	@ Extract function pointer (don't write r12 beyond this)
> +	ldr	r12, [sp, #16]
> +
> +	@ Convert sp to 32-bit physical
> +	mov	lr, sp
> +	ldr	r4, =PAR_MASK
> +	and	r5, lr, r4			@ Extract lower 12 bits of sp
> +	mcr	p15, 0, lr, c7, c8, 1		@ Write VA -> ATS1CPW
> +	mrc	p15, 0, lr, c7, c4, 0		@ Physical Address Register
> +	mvn	r4, r4
> +	and	lr, lr, r4			@ Clear lower 12 bits of PA
> +	add	lr, lr, r5			@ Calculate phys sp
> +	mov	sp, lr				@ Update
> +
> +	@ Disable MMU
> +        mrc     p15, 0, lr, c1, c0, 0           @ ctrl register
> +        bic     lr, lr, #0x1                    @ ...............m
> +        mcr     p15, 0, lr, c1, c0, 0           @ disable MMU
> +	isb

Nit: whitespace (tabs vs. spaces)

> +
> +	@ Make call
> +	blx	r12
> +
> +	pop	{r4-r5, r12, lr}
> +
> +	@ Enable MMU + Caches
> +        mrc     p15, 0, r1, c1, c0, 0		@ ctrl register
> +        orr     r1, r1, #0x1000			@ ...i............
> +        orr     r1, r1, #0x0005			@ .............c.m
> +        mcr     p15, 0, r1, c1, c0, 0		@ enable MMU
> +	isb

Nit: whitespace

> +
> +	@ Restore virtual sp and return
> +	mov	sp, r12
> +	bx	lr
> +ENDPROC(uefi_phys_call)
> +        .popsection
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index bc5687d..ebb34ee 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -655,7 +655,7 @@ extern int __init efi_setup_pcdp_console(char *);
>  #define EFI_64BIT		5	/* Is the firmware 64-bit? */
>  
>  #ifdef CONFIG_EFI
> -# ifdef CONFIG_X86
> +# if defined(CONFIG_X86) || defined(CONFIG_ARM)

ARM64 will need this too. Can we instead create a new empty config to
test here? CONFIG_EFI_ENABLED_ARCH perhaps?

g.

Re: [PATCH v3 3/3] init: efi: arm: enable (U)EFI runtime services on arm

[Grant Likely]

On Thu, 28 Nov 2013 16:41:23 +0000, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> Since the efi_set_virtual_address_map call has strict init ordering
> requirements, add an explicit hook in the required place.
> 
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  init/main.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/init/main.c b/init/main.c
> index febc511..1331829 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -905,6 +905,10 @@ static noinline void __init kernel_init_freeable(void)
>  	smp_prepare_cpus(setup_max_cpus);
>  
>  	do_pre_smp_initcalls();
> +
> +	if (IS_ENABLED(CONFIG_ARM) && efi_enabled(EFI_BOOT))
> +		efi_enter_virtual_mode();
> +

Personally, I would put the IS_ENABLED() and efi_enabled() tests into
efi_enter_virtual_mode() itself (or an empty stub for the !IS_ENABLED()
case), but that is mostly a nit.

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

>  	lockup_detector_init();
>  
>  	smp_init();
> -- 
> 1.7.10.4
> 

Re: [PATCH v3 0/3] (U)EFI runtime services for arm

[Grant Likely]

On Fri, 29 Nov 2013 18:58:51 +0100, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Fri, Nov 29, 2013 at 11:53:19AM +0000, Matt Fleming wrote:
> > On Thu, 28 Nov, at 04:41:20PM, Leif Lindholm wrote:
> > > In systems based on [U]EFI-conformant firmware, runtime services provide
> > > a standardised way for the kernel to update firmware environment
> > > variables. This is used for example by efibootmgr to update which image
> > > should be loaded on next boot.
> > > 
> > > This patchset implements basic support for UEFI runtime services on ARM
> > > platforms, as well as the basic underlying EFI support. It also defines
> > > a mechanism by which the required information is passed from the
> > > bootloader (the EFI stub submitted separately) to the kernel via FDT
> > > entries.
> > 
> > This all looks pretty good to me. Is this series going through the ARM
> > trees?
> 
> Thanks.
> That's the plan.

FWIW, I would like to see this merged this cycle. I think it is ready
enough and can be fixed up in-place.

g.

Re: [PATCH v3 1/3] Documentation: arm: add UEFI support documentation

[Leif Lindholm]

On Wed, Dec 04, 2013 at 03:06:47PM -0600, Matt Sealey wrote:
> By the time we get half-way through arm/kernel/head.S the cache and
> MMU has been turned off and on and off again by the decompressor, and
> after a large amount of guesswork and arbitrary restriction-based
> implementation, there's no guarantee that the kernel hasn't been
> decompressed over some important UEFI feature or some memory hasn't
> been trashed. You can't make that guarantee because by entering the
> plain zImage, you forfeited that information. This is at worst case
> going to be lots of blank screens and blinking serial console prompts
> and little more than frustration..

So, Grant covered the reason _why_ we coexist with zImage, so I won't
go into that. I will however point out that we are explicitly using the
UEFI interfaces to allocate the regions the zImage will decompress into.
This isn't guesswork, and has in fact already turned up issues with a
couple of UEFI board ports that reserved memory near 0 (which were
indeed previously being silently overwritten by the kernel
decompression).

We _are_ planning to do more development for subsequent patches, making
more use of the UEFI memory map. And by subsequent, I mean hopefully in
time for 3.14. I sneekily included this in the version of uefi.txt sent
out for separate review early November:
http://permalink.gmane.org/gmane.linux.kernel.efi/2657, but not in
the one included with this patch set (since the code isn't there yet).
But we considered it more important to get the basic support ready
first.

At that point, you will see the stub reading the dram_base from the
UEFI memory map rather than DT, and memblock_init getting its input
from there too.

Regards,

Leif

Re: [PATCH v3 2/3] arm: Add [U]EFI runtime services support

[Arnd Bergmann]

On Thursday 28 November 2013, Leif Lindholm wrote:
> @@ -898,6 +900,10 @@ void __init setup_arch(char **cmdline_p)
>  	sanity_check_meminfo();
>  	arm_memblock_init(&meminfo, mdesc);
>  
> +#ifdef CONFIG_EFI
> +	uefi_memblock_arm_reserve_range();
> +#endif
> +

Better use 

	if (IS_ENABLED(CONFIG_EFI))

here for readability and better build-time checking of the interface when
CONFIG_EFI is disabled.

> +/*
> + * Returns 1 if 'facility' is enabled, 0 otherwise.
> + */
> +int efi_enabled(int facility)
> +{
> +	return test_bit(facility, &arm_uefi_facility) != 0;
> +}
> +EXPORT_SYMBOL(efi_enabled);

I'd use EXPORT_SYMBOL_GPL() unless there is a documented reason why
a symbol should be available to GPL-incompatible modules.

> +static __init int is_discardable_region(efi_memory_desc_t *md)
> +{
> +#ifdef KEEP_ALL_REGIONS
> +	return 0;
> +#endif

IS_ENABLED() again.

> +	if (md->attribute & EFI_MEMORY_RUNTIME)
> +		return 0;
> +
> +	switch (md->type) {
> +#ifdef KEEP_BOOT_SERVICES_REGIONS
> +	case EFI_BOOT_SERVICES_CODE:
> +	case EFI_BOOT_SERVICES_DATA:
> +#endif

and I think it can be used here too:

	switch (md->type) {
	case EFI_BOOT_SERVICES_CODE:
	case EFI_BOOT_SERVICES_DATA:
		if (IS_ENABLED(KEEP_BOOT_SERVICES_REGIONS))
			return 1;
		/* fallthrough */
	case EFI_ACPI_RECLAIM_MEMORY:

> +	memmap.phys_map = early_memremap((phys_addr_t)(u32) uefi_boot_mmap,
> +					 uefi_boot_mmap_size);
> +	if (!memmap.phys_map)
> +		return 1;
> +
> +	p = memmap.phys_map;
> +	e = (void *)((u32)p + uefi_boot_mmap_size);

You are doing a lot of type casts here, which is normally an indication
that you have the types wrong in some way. I can't spot a mistake
here, but maybe you can give it some more thought and see if it can be
changed.

> +static int __init remap_region(efi_memory_desc_t *md, efi_memory_desc_t *entry)
> +{
> +	u64 va;
> +	u64 paddr;
> +	u64 size;
> +
> +	*entry = *md;
> +	paddr = entry->phys_addr;
> +	size = entry->num_pages << EFI_PAGE_SHIFT;
> +
> +	/*
> +	 * Map everything writeback-capable as coherent memory,
> +	 * anything else as device.
> +	 */
> +	if (md->attribute & EFI_MEMORY_WB)
> +		va = (u64)((u32)uefi_remap(paddr, size) & 0xffffffffUL);
> +	else
> +		va = (u64)((u32)uefi_ioremap(paddr, size) & 0xffffffffUL);

Same here. Why is 'va' a u64?

	Arnd

Re: [PATCH v3 2/3] arm: Add [U]EFI runtime services support

[Will Deacon]

Hi Leif,

Sorry it took so long to get back to you on this...

On Fri, Nov 29, 2013 at 05:43:04PM +0000, Leif Lindholm wrote:
> > > + */
> > > +static void __init phys_call_prologue(void)
> > > +{
> > > +       local_irq_disable();
> > > +
> > > +       /* Take out a flat memory mapping. */
> > > +       setup_mm_for_reboot();
> > > +
> > > +       /* Clean and invalidate caches */
> > > +       flush_cache_all();
> > > +
> > > +       /* Turn off caching */
> > > +       cpu_proc_fin();
> > > +
> > > +       /* Push out any further dirty data, and ensure cache is empty */
> > > +       flush_cache_all();
> >
> > Do we care about outer caches here?
> 
> I think we do. We jump into UEFI and make it relocate itself to the
> new virtual addresses, with MMU disabled (so all accesses NC).

Ok, so that means you need some calls to the outer_* cache ops.

> > This all looks suspiciously like
> > copy-paste from __soft_restart;
> 
> Yeah, 'cause you told me to :)
> 
> > perhaps some refactoring/reuse is in order?
> 
> Could do. Turn this into a process.c:idcall_prepare(), or something less
> daftly named?

Sure, something like that (can't think of a good name either, right
now...).

> > > + * Restore original memory map and re-enable interrupts.
> > > + */
> > > +static void __init phys_call_epilogue(void)
> > > +{
> > > +       static struct mm_struct *mm = &init_mm;
> > > +
> > > +       /* Restore original memory mapping */
> > > +       cpu_switch_mm(mm->pgd, mm);
> > > +
> > > +       /* Flush branch predictor and TLBs */
> > > +       local_flush_bp_all();
> > > +#ifdef CONFIG_CPU_HAS_ASID
> > > +       local_flush_tlb_all();
> > > +#endif
> >
> > ... and this looks like copy-paste from setup_mm_for_reboot.
> 
> You told me that too!

Hehe, I don't recall the conversation but I was probably talking in terms of
`let's get something working, then tidy it up afterwards'.

> Only this goes the other way.
> 
> Is the refactoring worth the extra code?

I think so. This code is pretty subtle, and I don't like it getting copied
around, particularly if we have to fix issues in it later on.

> > > +       local_irq_enable();
> > > +}
> > > +
> > > +static int __init remap_region(efi_memory_desc_t *md, efi_memory_desc_t *entry)
> > > +{
> > > +       u64 va;
> > > +       u64 paddr;
> > > +       u64 size;
> > > +
> > > +       *entry = *md;
> > > +       paddr = entry->phys_addr;
> > > +       size = entry->num_pages << EFI_PAGE_SHIFT;
> > > +
> > > +       /*
> > > +        * Map everything writeback-capable as coherent memory,
> > > +        * anything else as device.
> > > +        */
> > > +       if (md->attribute & EFI_MEMORY_WB)
> > > +               va = (u64)((u32)uefi_remap(paddr, size) & 0xffffffffUL);
> > > +       else
> > > +               va = (u64)((u32)uefi_ioremap(paddr, size) & 0xffffffffUL);
> >
> > Do you really need all those casts/masking?
> 
> I didn't find a better way to avoid warnings when building this code
> for both LPAE/non-LPAE.

Hmm, well phys_addr_t will be appropriately sized, if that helps you.

> > > + * This function switches the UEFI runtime services to virtual mode.
> > > + * This operation must be performed only once in the system's lifetime,
> > > + * including any kecec calls.
> > > + *
> > > + * This must be done with a 1:1 mapping. The current implementation
> > > + * resolves this by disabling the MMU.
> > > + */
> > > +efi_status_t  __init phys_set_virtual_address_map(u32 memory_map_size,
> > > +                                                 u32 descriptor_size,
> > > +                                                 u32 descriptor_version,
> > > +                                                 efi_memory_desc_t *dsc)
> > > +{
> > > +       uefi_phys_call_t *phys_set_map;
> > > +       efi_status_t status;
> > > +
> > > +       phys_call_prologue();
> > > +
> > > +       phys_set_map = (void *)(unsigned long)virt_to_phys(uefi_phys_call);
> > > +
> > > +       /* Called with caches disabled, returns with caches enabled */
> > > +       status = phys_set_map(memory_map_size, descriptor_size,
> > > +                             descriptor_version, dsc,
> > > +                             efi.set_virtual_address_map)
> > > +;
> >
> > Guessing this relies on a physically-tagged cache? Wouldn't hurt to put
> > something in the epilogue code to deal with vivt caches if they're not
> > prohibited by construction (e.g. due to some build dependency magic).
> 
> Sorry, I don't quite follow?
> How would it depend on a physically-tagged cache?

I was wondering if you could run into issues in the epilogue, since you've
changed page tables without cleaning the cache, but actually cpu_switch_mm
takes care of that.

> > > +       phys_call_epilogue();
> > > +
> > > +       return status;
> > > +}
> 
> 
> > > diff --git a/arch/arm/kernel/uefi_phys.S b/arch/arm/kernel/uefi_phys.S
> > > new file mode 100644
> > > index 0000000..e9a1542
> > > --- /dev/null
> > > +++ b/arch/arm/kernel/uefi_phys.S
> > > @@ -0,0 +1,59 @@
> > > +/*
> > > + * arch/arm/kernel/uefi_phys.S
> > > + *
> > > + * Copyright (C) 2013  Linaro 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.
> > > + */
> > > +
> > > +#include <linux/linkage.h>
> > > +#define PAR_MASK 0xfff
> > > +
> > > +       .text
> > > +@ uefi_phys_call(a, b, c, d, *f)
> > > +       .align  5
> > > +        .pushsection    .idmap.text, "ax"
> > > +ENTRY(uefi_phys_call)
> > > +       @ Save physical context
> > > +       mov     r12, sp
> > > +       push    {r4-r5, r12, lr}
> > > +
> > > +       @ Extract function pointer (don't write r12 beyond this)
> > > +       ldr     r12, [sp, #16]
> > > +
> > > +       @ Convert sp to 32-bit physical
> > > +       mov     lr, sp
> > > +       ldr     r4, =PAR_MASK
> > > +       and     r5, lr, r4                      @ Extract lower 12 bits of sp
> > > +       mcr     p15, 0, lr, c7, c8, 1           @ Write VA -> ATS1CPW
> >
> > This is broken without an isb but, more to the point, why can't we just do
> > the standard lowmem virt_to_phys conversion here instead?
> 
> I can't use that from within asm (right?). Are you suggesting that I
> should duplicate the mechanism of virt_to_phys here?

I think you need an extra argument: either the physical SP, or the virt->
phys offset.

> > > +       mrc     p15, 0, lr, c7, c4, 0           @ Physical Address Register
> > > +       mvn     r4, r4
> > > +       and     lr, lr, r4                      @ Clear lower 12 bits of PA
> > > +       add     lr, lr, r5                      @ Calculate phys sp
> > > +       mov     sp, lr                          @ Update
> > > +
> > > +       @ Disable MMU
> > > +        mrc     p15, 0, lr, c1, c0, 0           @ ctrl register
> > > +        bic     lr, lr, #0x1                    @ ...............m
> > > +        mcr     p15, 0, lr, c1, c0, 0           @ disable MMU
> > > +       isb
> > > +
> > > +       @ Make call
> > > +       blx     r12
> >
> > This is basically a copy-paste of cpu_v7_reset. Perhaps using some assembler
> > macros would help here?
> 
> Well, I explicitly don't want to touch SCTLR.TE.
> We could macro-ize it, but I think that would increase the amount of
> code.

How would using a macro increase the amount of code? Just make the macro
take the bits to clear and the bits to set as arguments.

> > > +
> > > +       pop     {r4-r5, r12, lr}
> > > +
> > > +       @ Enable MMU + Caches
> > > +        mrc     p15, 0, r1, c1, c0, 0          @ ctrl register
> > > +        orr     r1, r1, #0x1000                        @ ...i............
> > > +        orr     r1, r1, #0x0005                        @ .............c.m
> > > +        mcr     p15, 0, r1, c1, c0, 0          @ enable MMU
> > > +       isb
> >
> > Why do we have to enable the caches so early?
> 
> You'd prefer it being done back in phys_call_epilogue()?

Unless there's a good reason to move stuff into early assembly, it would be
better off using functions/macros from C code imo.

Will

Re: [PATCH v3 1/3] Documentation: arm: add UEFI support documentation

[Matt Sealey]

On Wed, Dec 4, 2013 at 4:44 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Wed, Dec 04, 2013 at 03:06:47PM -0600, Matt Sealey wrote:
>
>> there's no guarantee that the kernel hasn't been decompressed over
>> some important UEFI feature or some memory hasn't been trashed. You
>> can't make that guarantee because by entering the plain zImage, you
>> forfeited that information.
>
> The stub is responsible for ensuring that the compressed kernel is
> loaded at a suitable address. Take a look at efi_relocate_kernel().

My objection is the suitable address is based on a restriction that
booting from UEFI doesn't have and information UEFI provides that
makes kernel features from head.S (both of them) easier to get around.
The kernel doesn't need to be within a particular range of the start
of memory, nor does the device tree or ramdisk require being in a
particular place. What the code before efi_relocate_kernel does is
allocate a maximum-sized-buffer to safely decompress in, which is just
a gross way to do it, then crosses it's fingers based on the way it
has historically worked - while you might want to assume that the
decompression process is quite well defined and reliable, I keep
seeing patches come in that stop it from doing weird unsavory behavior
- for example decompressing over it's own page table.

The decompressor - and the kernel head it jumps to after decompression
- *guess* all the information UEFI could have provided and completely
regenerate the environment for the decompressor itself (stacks, hacky
memory allocations, cache on, off, on, off, on... fudging locations of
page tables, zreladdr fixup, low level debug message output, in
context of UEFI - reimplementation of memcpy, memset). It forfeits a
more controlled and lean boot process to capitulate to a historical
legacy. Since you're taking over the decompressor head.S anyway, why
not take control of the decompression process?

It sets up a page table location the hard way (as above.. also patched
recently not to decompress over it's own page table). It doesn't need
to relocate itself past the end of the decompressed image. It doesn't
need to set up the C environment - UEFI did that for it. It makes
assumptions about the stack and hacks memory allocations for the
decompression.. it turns the cache on, decompresses, then turns it off
again... you can just walk through the code under the EFI stub in
compressed/head.S and see all this can just fall away.

There's one immediate advantage too, if it's actually implemented and
working, which is that for kernel images that are compressed using the
standard UEFI compression method no actual decompression code needs to
be added to the stub, and the functionality gets the exact length of
the required decompression buffer.. that doesn't reduce flexibility in
kernel compression as long as there is still the possibility of adding
additional compression code to the stub.

The second immediate advantage is that the EFI stub/decompressor can
actually verify that the *decompressed* image meets Secure Boot
requirements.

Once you get past the decompressor and into the kernel proper head.S,
creating the page tables (again) and turning the MMU on, pv table
patching.. if you still had the information around, that gets simpler
too.

Grant suggested I should propose some patches; sure, if I'm not otherwise busy.

Maybe the Linaro guys can recommend a platform (real or emulated) that
would be best to test it on with the available UEFI?

>> Most of the guessing is ideally not required to be a guess at all, the
>> restrictions are purely to deal with the lack of trust for the
>> bootloader environment. Why can't we trust UEFI? Or at least hold it
>> to a higher standard. If someone ships a broken UEFI, they screw a
>> feature or have a horrible bug and ship it, laud the fact Linux
>> doesn't boot on it and the fact that it's their fault - over their
>> head. It actually works these days, Linux actually has "market share,"
>> companies really go out of their way to rescue their "image" and
>> resolve the situation when someone blogs about a serious UEFI bug on
>> their $1300 laptops, or even $300 tablets.
>
> Yeah, that hasn't actually worked out too well for us.

Aside from Teething problems caused by a rush to market ;)

For the "ARM server market" rather than the "get the cheapest
tablet/ultrabook out of the door that runs Windows 8/RT" I am sure
this is going to get to be VERY important for vendors to take into
account. Imagine if Dell shipped a *server* where Linux would brick it
out of the box just for setting a variable.. however, if it works the
day they ship the server, and Linux gains better support for UEFI
booting which breaks the server in question, that's our fault for not
doing it in the right way in the first place, and Dell can be just as
angry at us as we would be at them. Vendors won't test code that
doesn't exist for obvious reasons.

This is what I was trying to get at about them not updating their
firmware support for the more firmware-aware method if it works with
the "ditch firmware early" method worked well for them (which means
the functionality in the firmware never gets stressed and the
self-fulfilling prophecy of untrustworthy firmware vendors persists).
That firmware quality assurance - if not the code itself - will
trickle down to consumer tablets and ARM thin laptop kind of devices.

Ta,
Matt Sealey <neko@bakuhatsu.net>

Re: [PATCH v3 2/3] arm: Add [U]EFI runtime services support

[Leif Lindholm]

Hi Arnd,

Thank you for your comments.

On Fri, Dec 06, 2013 at 02:59:48AM +0100, Arnd Bergmann wrote:
> On Thursday 28 November 2013, Leif Lindholm wrote:
> > @@ -898,6 +900,10 @@ void __init setup_arch(char **cmdline_p)
> >  	sanity_check_meminfo();
> >  	arm_memblock_init(&meminfo, mdesc);
> >  
> > +#ifdef CONFIG_EFI
> > +	uefi_memblock_arm_reserve_range();
> > +#endif
> > +
> 
> Better use 
> 
> 	if (IS_ENABLED(CONFIG_EFI))
> 
> here for readability and better build-time checking of the interface when
> CONFIG_EFI is disabled.

I think I'll do what Grant suggested and stub it out in asm/uefi.h.

> > +/*
> > + * Returns 1 if 'facility' is enabled, 0 otherwise.
> > + */
> > +int efi_enabled(int facility)
> > +{
> > +	return test_bit(facility, &arm_uefi_facility) != 0;
> > +}
> > +EXPORT_SYMBOL(efi_enabled);
> 
> I'd use EXPORT_SYMBOL_GPL() unless there is a documented reason why
> a symbol should be available to GPL-incompatible modules.

So ... this is duplicating x86 code which Matt Fleming has promised
to make generic. I'd prefer to keep it identical to x86 until this
copy goes away.

> > +static __init int is_discardable_region(efi_memory_desc_t *md)
> > +{
> > +#ifdef KEEP_ALL_REGIONS
> > +	return 0;
> > +#endif
> 
> IS_ENABLED() again.
 
Ok.

> > +	if (md->attribute & EFI_MEMORY_RUNTIME)
> > +		return 0;
> > +
> > +	switch (md->type) {
> > +#ifdef KEEP_BOOT_SERVICES_REGIONS
> > +	case EFI_BOOT_SERVICES_CODE:
> > +	case EFI_BOOT_SERVICES_DATA:
> > +#endif
> 
> and I think it can be used here too:
> 
> 	switch (md->type) {
> 	case EFI_BOOT_SERVICES_CODE:
> 	case EFI_BOOT_SERVICES_DATA:
> 		if (IS_ENABLED(KEEP_BOOT_SERVICES_REGIONS))
> 			return 1;
> 		/* fallthrough */
> 	case EFI_ACPI_RECLAIM_MEMORY:

Will redesign for next version.

> > +	memmap.phys_map = early_memremap((phys_addr_t)(u32) uefi_boot_mmap,
> > +					 uefi_boot_mmap_size);
> > +	if (!memmap.phys_map)
> > +		return 1;
> > +
> > +	p = memmap.phys_map;
> > +	e = (void *)((u32)p + uefi_boot_mmap_size);
> 
> You are doing a lot of type casts here, which is normally an indication
> that you have the types wrong in some way. I can't spot a mistake
> here, but maybe you can give it some more thought and see if it can be
> changed.

Grant made much the same comments here. It is possible the change to
the DT bindings to have all addresses 64-bit actually makes this go
away. I will revisit for next version.

> > +static int __init remap_region(efi_memory_desc_t *md, efi_memory_desc_t *entry)
> > +{
> > +	u64 va;
> > +	u64 paddr;
> > +	u64 size;
> > +
> > +	*entry = *md;
> > +	paddr = entry->phys_addr;
> > +	size = entry->num_pages << EFI_PAGE_SHIFT;
> > +
> > +	/*
> > +	 * Map everything writeback-capable as coherent memory,
> > +	 * anything else as device.
> > +	 */
> > +	if (md->attribute & EFI_MEMORY_WB)
> > +		va = (u64)((u32)uefi_remap(paddr, size) & 0xffffffffUL);
> > +	else
> > +		va = (u64)((u32)uefi_ioremap(paddr, size) & 0xffffffffUL);
> 
> Same here. Why is 'va' a u64?

Because the field in the structure (defined by UEFI) is 64-bit:

        entry->virt_addr = va;

/
    Leif

Re: [PATCH v3 2/3] arm: Add [U]EFI runtime services support

[Arnd Bergmann]

On Friday 06 December 2013, Leif Lindholm wrote:
> 
> On Fri, Dec 06, 2013 at 02:59:48AM +0100, Arnd Bergmann wrote:
> > On Thursday 28 November 2013, Leif Lindholm wrote:
> > > @@ -898,6 +900,10 @@ void __init setup_arch(char **cmdline_p)
> > >  	sanity_check_meminfo();
> > >  	arm_memblock_init(&meminfo, mdesc);
> > >  
> > > +#ifdef CONFIG_EFI
> > > +	uefi_memblock_arm_reserve_range();
> > > +#endif
> > > +
> > 
> > Better use 
> > 
> > 	if (IS_ENABLED(CONFIG_EFI))
> > 
> > here for readability and better build-time checking of the interface when
> > CONFIG_EFI is disabled.
> 
> I think I'll do what Grant suggested and stub it out in asm/uefi.h.

That's ok, too. I usually prefer the IS_ENABLED() method if there is only
one caller, since it's harder to get wrong, but the result is similar.

> > > +/*
> > > + * Returns 1 if 'facility' is enabled, 0 otherwise.
> > > + */
> > > +int efi_enabled(int facility)
> > > +{
> > > +	return test_bit(facility, &arm_uefi_facility) != 0;
> > > +}
> > > +EXPORT_SYMBOL(efi_enabled);
> > 
> > I'd use EXPORT_SYMBOL_GPL() unless there is a documented reason why
> > a symbol should be available to GPL-incompatible modules.
> 
> So ... this is duplicating x86 code which Matt Fleming has promised
> to make generic. I'd prefer to keep it identical to x86 until this
> copy goes away.

Ok, makes sense.

> > > +static int __init remap_region(efi_memory_desc_t *md, efi_memory_desc_t *entry)
> > > +{
> > > +	u64 va;
> > > +	u64 paddr;
> > > +	u64 size;
> > > +
> > > +	*entry = *md;
> > > +	paddr = entry->phys_addr;
> > > +	size = entry->num_pages << EFI_PAGE_SHIFT;
> > > +
> > > +	/*
> > > +	 * Map everything writeback-capable as coherent memory,
> > > +	 * anything else as device.
> > > +	 */
> > > +	if (md->attribute & EFI_MEMORY_WB)
> > > +		va = (u64)((u32)uefi_remap(paddr, size) & 0xffffffffUL);
> > > +	else
> > > +		va = (u64)((u32)uefi_ioremap(paddr, size) & 0xffffffffUL);
> > 
> > Same here. Why is 'va' a u64?
> 
> Because the field in the structure (defined by UEFI) is 64-bit:
> 
>         entry->virt_addr = va;

I see. Maybe put the typecast in that final assignment then?

	Arnd

Re: [PATCH v3 1/3] Documentation: arm: add UEFI support documentation

[Grant Likely]

On Fri, 6 Dec 2013 11:20:45 -0600, Matt Sealey <neko@bakuhatsu.net> wrote:
> On Wed, Dec 4, 2013 at 4:44 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > On Wed, Dec 04, 2013 at 03:06:47PM -0600, Matt Sealey wrote:
> Grant suggested I should propose some patches; sure, if I'm not otherwise busy.
> 
> Maybe the Linaro guys can recommend a platform (real or emulated) that
> would be best to test it on with the available UEFI?

Roy Franz (cc'd) has got UEFI running under QEMU. A few modifications
were required to both stock UEFI and QEMU. I'm not sure what the status
of mainlining those patches is. I think there are still a few things
that Roy has to fix, but you should be able to get the current patches
from him to get going.

g.

Re: [PATCH v3 1/3] Documentation: arm: add UEFI support documentation

[Roy Franz]

On Tue, Dec 10, 2013 at 4:30 AM, Grant Likely <grant.likely@linaro.org> wrote:
> On Fri, 6 Dec 2013 11:20:45 -0600, Matt Sealey <neko@bakuhatsu.net> wrote:
>> On Wed, Dec 4, 2013 at 4:44 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>> > On Wed, Dec 04, 2013 at 03:06:47PM -0600, Matt Sealey wrote:
>> Grant suggested I should propose some patches; sure, if I'm not otherwise busy.
>>
>> Maybe the Linaro guys can recommend a platform (real or emulated) that
>> would be best to test it on with the available UEFI?
>
> Roy Franz (cc'd) has got UEFI running under QEMU. A few modifications
> were required to both stock UEFI and QEMU. I'm not sure what the status
> of mainlining those patches is. I think there are still a few things
> that Roy has to fix, but you should be able to get the current patches
> from him to get going.
>
> g.
>

Hi Grant and Matt,

I have put together a quick wiki page describing the current status,
with git trees for
UEFI and QEMU, and instructions for running the model.  I just whipped
this up now, so it
is pretty basic, but should have all the required information.

https://wiki.linaro.org/LEG/Engineering/Kernel/UEFI/VersatileExpress/QEMU

Please let me know if you have any questions.

Thanks,
Roy

Re: [PATCH v3 1/3] Documentation: arm: add UEFI support documentation

[Grant Likely]

On Tue, 10 Dec 2013 10:29:34 -0800, Roy Franz <roy.franz@linaro.org> wrote:
> On Tue, Dec 10, 2013 at 4:30 AM, Grant Likely <grant.likely@linaro.org> wrote:
> > On Fri, 6 Dec 2013 11:20:45 -0600, Matt Sealey <neko@bakuhatsu.net> wrote:
> >> On Wed, Dec 4, 2013 at 4:44 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> >> > On Wed, Dec 04, 2013 at 03:06:47PM -0600, Matt Sealey wrote:
> >> Grant suggested I should propose some patches; sure, if I'm not otherwise busy.
> >>
> >> Maybe the Linaro guys can recommend a platform (real or emulated) that
> >> would be best to test it on with the available UEFI?
> >
> > Roy Franz (cc'd) has got UEFI running under QEMU. A few modifications
> > were required to both stock UEFI and QEMU. I'm not sure what the status
> > of mainlining those patches is. I think there are still a few things
> > that Roy has to fix, but you should be able to get the current patches
> > from him to get going.
> >
> > g.
> >
> 
> Hi Grant and Matt,
> 
> I have put together a quick wiki page describing the current status,
> with git trees for
> UEFI and QEMU, and instructions for running the model.  I just whipped
> this up now, so it
> is pretty basic, but should have all the required information.
> 
> https://wiki.linaro.org/LEG/Engineering/Kernel/UEFI/VersatileExpress/QEMU
> 
> Please let me know if you have any questions.

Thanks Roy.

g.