[PATCH v4 0/5] arm: add UEFI runtime services support

[PATCH v4 0/5] arm: add UEFI runtime services support

[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.
It also depends on Mark's "efi: add helper function to get UEFI params
from FDT" patch.

Changes since v3:
- Use new common function (from arm64 set) for extracting UEFI params
  from FDT.
- Use new generic early_ioremap implementation (with early_memunmap).
- Reworked/simplified phys_call code, using new shared functions/macros.
- Slightly refactored to reduce number of explicit casts.
- Also preserve and map EFI_ACPI_MEMORY_NVS regions.
- Added some ACKs.

Change 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).

Leif Lindholm (5):
  arm: break part of __soft_restart out into separate function
  arm: add new asm macro update_sctlr
  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                 |   16 ++
 arch/arm/include/asm/assembler.h |   13 ++
 arch/arm/include/asm/idmap.h     |    1 +
 arch/arm/include/asm/uefi.h      |   28 +++
 arch/arm/kernel/Makefile         |    2 +
 arch/arm/kernel/process.c        |   12 +-
 arch/arm/kernel/setup.c          |    4 +
 arch/arm/kernel/uefi.c           |  418 ++++++++++++++++++++++++++++++++++++++
 arch/arm/kernel/uefi_phys.S      |   67 ++++++
 arch/arm/mm/idmap.c              |   15 ++
 include/linux/efi.h              |    2 +-
 init/main.c                      |    4 +
 14 files changed, 634 insertions(+), 12 deletions(-)
 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 v4 1/5] arm: break part of __soft_restart out into separate function

[Leif Lindholm]

Certain operations can be considered mandatory for any piece of code
preparing to switch off the MMU. Break this out into separate function
dmap_prepare().

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
Suggested-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/include/asm/idmap.h |    1 +
 arch/arm/kernel/process.c    |   12 +-----------
 arch/arm/mm/idmap.c          |   15 +++++++++++++++
 3 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/arch/arm/include/asm/idmap.h b/arch/arm/include/asm/idmap.h
index bf863ed..2e914a8 100644
--- a/arch/arm/include/asm/idmap.h
+++ b/arch/arm/include/asm/idmap.h
@@ -10,5 +10,6 @@
 extern pgd_t *idmap_pgd;
 
 void setup_mm_for_reboot(void);
+void idmap_prepare(void);
 
 #endif	/* __ASM_IDMAP_H */
diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 92f7b15..91b4cec 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -75,17 +75,7 @@ static void __soft_restart(void *addr)
 {
 	phys_reset_t phys_reset;
 
-	/* 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();
+	idmap_prepare();
 
 	/* Switch to the identity mapping. */
 	phys_reset = (phys_reset_t)(unsigned long)virt_to_phys(cpu_reset);
diff --git a/arch/arm/mm/idmap.c b/arch/arm/mm/idmap.c
index 8e0e52e..5c85779 100644
--- a/arch/arm/mm/idmap.c
+++ b/arch/arm/mm/idmap.c
@@ -122,3 +122,18 @@ void setup_mm_for_reboot(void)
 	local_flush_tlb_all();
 #endif
 }
+
+void idmap_prepare(void)
+{
+	/* 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();
+}
-- 
1.7.10.4

[PATCH v4 2/5] arm: add new asm macro update_sctlr

[Leif Lindholm]

A new macro for setting/clearing bits in the SCTLR.

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
Suggested-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/include/asm/assembler.h |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h
index 5c22851..aba6458 100644
--- a/arch/arm/include/asm/assembler.h
+++ b/arch/arm/include/asm/assembler.h
@@ -383,4 +383,17 @@ THUMB(	orr	\reg , \reg , #PSR_T_BIT	)
 #endif
 	.endm
 
+#ifdef CONFIG_CPU_CP15
+/* Macro for setting/clearing bits in sctlr */
+	.macro update_sctlr, set:req, clear:req, tmp:req, tmp2:req
+	mrc	p15, 0, \tmp, c1, c0, 0
+	ldr	\tmp2, =\set
+	orr	\tmp, \tmp, \tmp2
+	ldr	\tmp2, =\clear
+	mvn	\tmp2, \tmp2
+	and	\tmp, \tmp, \tmp2
+	mcr	p15, 0, \tmp, c1, c0, 0
+	.endm
+#endif
+
 #endif /* __ASM_ASSEMBLER_H__ */
-- 
1.7.10.4

[PATCH v4 3/5] 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.

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>
---
 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..04da9ce
--- /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 extends 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 v4 4/5] 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.

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
Reviewed-by: Grant Likely <grant.likely@linaro.org>

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Will Deacon <will.deacon@arm.com>
---
 arch/arm/Kconfig            |   16 ++
 arch/arm/include/asm/uefi.h |   28 +++
 arch/arm/kernel/Makefile    |    2 +
 arch/arm/kernel/setup.c     |    4 +
 arch/arm/kernel/uefi.c      |  418 +++++++++++++++++++++++++++++++++++++++++++
 arch/arm/kernel/uefi_phys.S |   67 +++++++
 include/linux/efi.h         |    2 +-
 7 files changed, 536 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..1ab24cc 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1853,6 +1853,20 @@ 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
+	select UEFI_PARAMS_FROM_FDT
+	---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 +2286,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..eff27da
--- /dev/null
+++ b/arch/arm/include/asm/uefi.h
@@ -0,0 +1,28 @@
+#ifndef _ASM_ARM_EFI_H
+#define _ASM_ARM_EFI_H
+
+#ifdef CONFIG_EFI
+#include <asm/mach/map.h>
+
+extern void uefi_memblock_arm_reserve_range(void);
+
+typedef efi_status_t uefi_phys_call_t(efi_set_virtual_address_map_t *f,
+				      u32 virt_phys_offset,
+				      u32 memory_map_size,
+				      u32 descriptor_size,
+				      u32 descriptor_version,
+				      efi_memory_desc_t *dsc);
+
+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))
+
+#else
+#define uefi_memblock_arm_reserve_range()
+#endif /* CONFIG_EFI */
+
+#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 038fb75..57c33dd 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"
 
@@ -897,6 +899,8 @@ void __init setup_arch(char **cmdline_p)
 	sanity_check_meminfo();
 	arm_memblock_init(&meminfo, mdesc);
 
+	uefi_memblock_arm_reserve_range();
+
 	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..65e7b05
--- /dev/null
+++ b/arch/arm/kernel/uefi.c
@@ -0,0 +1,418 @@
+/*
+ * Based on Unified Extensible Firmware Interface Specification version 2.3.1
+ *
+ * Copyright (C) 2013-2014  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 want to wire up a debugger and debug the UEFI side, set to 0.
+ */
+#define DISCARD_UNUSED_REGIONS 1
+
+/*
+ * If you need to (temporarily) support buggy firmware, set to 0.
+ */
+#define DISCARD_BOOT_SERVICES_REGIONS 1
+
+/*
+ * 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 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 2.30 or greater\n",
+			efi.systab->hdr.revision >> 16,
+			efi.systab->hdr.revision & 0xffff);
+
+	/* Show what we know for posterity */
+	c16 = 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_memunmap(c16, sizeof(vendor));
+	early_memunmap(efi.systab,  sizeof(efi_system_table_t));
+
+	return retval;
+}
+
+static __init int is_discardable_region(efi_memory_desc_t *md)
+{
+	if (md->attribute & EFI_MEMORY_RUNTIME)
+		return 0;
+
+	switch (md->type) {
+	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_MEMORY_NVS:
+	case EFI_ACPI_RECLAIM_MEMORY:
+		return 0;
+	/* Preserve */
+	case EFI_RESERVED_TYPE:
+		return 0;
+	}
+
+	return DISCARD_UNUSED_REGIONS;
+}
+
+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(uefi_boot_mmap, uefi_boot_mmap_size);
+	if (!memmap.phys_map)
+		return 1;
+
+	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_memunmap(memmap.phys_map, uefi_boot_mmap_size);
+
+	if (uefi_debug)
+		pr_info("%d regions preserved.\n", memmap.nr_map);
+
+	remove_sections(uefi_boot_mmap, uefi_boot_mmap_size);
+
+	return 0;
+}
+
+void __init uefi_memblock_arm_reserve_range(void)
+{
+	struct efi_fdt_params params;
+
+	/* Grab UEFI information placed in FDT by stub */
+	if (!efi_get_fdt_params(&params, uefi_debug))
+		return;
+
+	uefi_system_table = params.system_table;
+
+	uefi_boot_mmap = params.mmap;
+	uefi_boot_mmap_size = params.mmap_size;
+	uefi_mmap_desc_size = params.desc_size;
+	uefi_mmap_desc_ver = params.desc_ver;
+	if (uefi_boot_mmap > UINT_MAX) {
+		pr_err("UEFI memory map located above 4GB - unusable!");
+		return;
+	}
+
+	if (uefi_init() < 0)
+		return;
+
+	remove_regions();
+
+	set_bit(EFI_BOOT, &arm_uefi_facility);
+}
+
+/*
+ * Disable instrrupts, enable idmap and disable caches.
+ */
+static void __init phys_call_prologue(void)
+{
+	local_irq_disable();
+
+	outer_disable();
+
+	idmap_prepare();
+}
+
+/*
+ * 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);
+
+	local_flush_bp_all();
+	local_flush_tlb_all();
+
+	outer_resume();
+
+	local_irq_enable();
+}
+
+static int __init remap_region(efi_memory_desc_t *md, efi_memory_desc_t *entry)
+{
+	u32 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 = (u32)uefi_remap(paddr, size);
+	else
+		va = (u32)uefi_ioremap(paddr, size);
+	if (!va)
+		return 0;
+	entry->virt_addr = va;
+
+	if (uefi_debug)
+		pr_info("  %016llx-%016llx => 0x%08x : (%s)\n",
+			paddr, paddr + size - 1, 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 = 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_ATOMIC);
+	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(efi.set_virtual_address_map,
+			      PAGE_OFFSET - PHYS_OFFSET,
+			      memory_map_size, descriptor_size,
+			      descriptor_version, dsc);
+
+	phys_call_epilogue();
+
+	return status;
+}
+
+/*
+ * Called explicitly from init/main.c
+ */
+void __init efi_enter_virtual_mode(void)
+{
+	efi_status_t status;
+	u32 mmap_phys_addr;
+
+	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 = runtime->set_virtual_address_map;
+
+	/*
+	 * __virt_to_phys() takes an unsigned long and returns a phys_addr_t
+	 * memmap.phys_map is a void *
+	 * The gymnastics below makes this compile validly with/without LPAE.
+	 */
+	mmap_phys_addr = __virt_to_phys((u32)memmap.map);
+	memmap.phys_map = (void *)mmap_phys_addr;
+
+	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 = runtime->get_variable;
+	efi.get_next_variable = runtime->get_next_variable;
+	efi.set_variable = runtime->set_variable;
+	efi.set_virtual_address_map = NULL;
+
+	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..415c86a
--- /dev/null
+++ b/arch/arm/kernel/uefi_phys.S
@@ -0,0 +1,67 @@
+/*
+ * 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 <asm/assembler.h>
+#include <asm/cp15.h>
+#include <linux/linkage.h>
+#define PAR_MASK 0xfff
+
+	.text
+@ uefi_phys_call(*f, virt_phys_offset, a, b, c, d, ...)
+	.align  5
+	.pushsection    .idmap.text, "ax"
+ENTRY(uefi_phys_call)
+	@ Save physical context
+	mov	r12, sp
+	ldr	sp, =tmpstack
+	stmfd	sp, {r4-r5, r12, lr}	@ push is redefined by asm/assembler.h
+
+	mov	r4, r1
+
+	@ Extract function pointer (don't write lr again before call)
+	mov	lr, r0
+
+	@ Shift arguments down
+	mov	r0, r2
+	mov	r1, r3
+	ldr	r2, [r12], #4
+	ldr	r3, [r12], #4
+
+	@ Convert sp to physical
+	sub	r12, r12, r4
+	mov	sp, r12
+
+	@ Disable MMU
+	update_sctlr	0, (CR_M), r4, r5
+	isb
+
+	@ Make call
+	blx	lr
+
+	@ Enable MMU + Caches
+	update_sctlr	(CR_I | CR_C | CR_M), 0, r4, r5
+	isb
+
+	ldr	sp, =tmpstack_top
+	ldmfd	sp, {r4-r5, r12, lr}
+
+	@ Restore virtual sp and return
+	mov	sp, r12
+	bx	lr
+
+	.align	3
+tmpstack_top:
+	.long	0	@ r4
+	.long	0	@ r5
+	.long	0	@ r12
+	.long	0	@ lr
+tmpstack:
+ENDPROC(uefi_phys_call)
+	.popsection
diff --git a/include/linux/efi.h b/include/linux/efi.h
index fa7d950..afaeb85 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -664,7 +664,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 v4 5/5] 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>
Acked-by: Grant Likely <grant.likely@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 v4 4/5] arm: Add [U]EFI runtime services support

[Matt Fleming]

On Sat, 11 Jan, at 01:05:23PM, 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.
> 
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> Reviewed-by: Grant Likely <grant.likely@linaro.org>
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm/Kconfig            |   16 ++
>  arch/arm/include/asm/uefi.h |   28 +++
>  arch/arm/kernel/Makefile    |    2 +
>  arch/arm/kernel/setup.c     |    4 +
>  arch/arm/kernel/uefi.c      |  418 +++++++++++++++++++++++++++++++++++++++++++
>  arch/arm/kernel/uefi_phys.S |   67 +++++++
>  include/linux/efi.h         |    2 +-
>  7 files changed, 536 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

[...]

> @@ -664,7 +664,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)

This looks like it's going to conflict with Mark's "[PATCH 6/6] arm64:
add EFI runtime services".

Guys, what's going on here? Srsly. The dependency chain is so nuts that
I'm struggling to even review these patches. Good luck to the maintainer
that wants to merge this stuff.

Can't you guys work together a bit more closely so that all of your
patches feel like one logical progression of work?

-- 
Matt Fleming, Intel Open Source Technology Center

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

[Arnd Bergmann]

On Saturday 11 January 2014, Leif Lindholm wrote:
> 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();

What is the dependency on CONFIG_ARM here? Wouldn't most other
architectures need the same? I'd rather not see this turn into
a long list of CONFIG_$(ARCH) checks if other architectures
enable it in the same place.

I also wonder why the three architectures implementing it all
call this from wildly different places during init/main.c, namely
(very early) setup_arch() on ia64, (relatively early) start_kernel
on x86 and (relatively late) kernel_init_freeable on arm.

In general, I'd be happy with adding this as late in the startup
code as possible, but it may be better to use the same place as
x86 in order to avoid surprises with unexpected dependencies.
One such dependency that may cause problems is the fact that
we (try to) call efi_late_init() before efi_enter_virtual_mode()
now.

	Arnd

Re: [PATCH v4 4/5] arm: Add [U]EFI runtime services support

[Arnd Bergmann]

On Saturday 11 January 2014, 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.

As far as I'm concerned there are no plans to have ACPI support on ARM32,
so I wonder what the code to populate the ACPI tables is about. Can
you clarify this?
 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 78a79a6a..1ab24cc 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1853,6 +1853,20 @@ 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

What is the dependency on !CPU_BIG_ENDIAN? We try hard to have
all kernel code support both big-endian and little-endian, and
I'm guessing there is a significant overlap between the people
that want UEFI support and those that want big-endian kernels.

> +struct efi_memory_map memmap;

"memmap" is not a good name for a global identifier, particularly because
it's easily confused with the well-known "mem_map" array. This
wants namespace prefix like you other variable, or a "static" tag,
preferably both.

> +/*
> + * 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.

                   kexec

> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index fa7d950..afaeb85 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -664,7 +664,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)

Maybe better #ifndef CONFIG_IA64? It seems that is the odd one out and
all possible future architectures would be like x86 and ARM.

	Arnd

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

[Leif Lindholm]

On Mon, Jan 13, 2014 at 07:29:06PM +0100, Arnd Bergmann wrote:
> On Saturday 11 January 2014, Leif Lindholm wrote:
> > 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();
> 
> What is the dependency on CONFIG_ARM here? Wouldn't most other
> architectures need the same?

Most 64-bit architectures could get away from it.
x86 does it where its particular init environment forces it to.

For arm, the strict ordering requirement is for efi_enter_virtual_mode
to be called after init_static_idmap.

If ordering between early_initcalls was possible in a sane way, I could
do that instead, but I don't think a patch that swapped order of kernel/
and mm/ in arch/arm/Makefile would be accepted :)

> I'd rather not see this turn into
> a long list of CONFIG_$(ARCH) checks if other architectures
> enable it in the same place.
> 
> I also wonder why the three architectures implementing it all
> call this from wildly different places during init/main.c, namely
> (very early) setup_arch() on ia64,

Likewise arm64.

> (relatively early) start_kernel
> on x86 and (relatively late) kernel_init_freeable on arm.

As I said - the pure 64-bit archs have less of an issue, since they
can have their kernel somewhere that won't clash with the 1:1 mapping
of RAM required by UEFI SetVirtualAddressMap.

> In general, I'd be happy with adding this as late in the startup
> code as possible, but it may be better to use the same place as
> x86 in order to avoid surprises with unexpected dependencies.

I _really_ don't want to call SetVirtualAddressMap after smp_init.

> One such dependency that may cause problems is the fact that
> we (try to) call efi_late_init() before efi_enter_virtual_mode()
> now.

Well, efi_late_init() is an inline {} on everything !x86.

/
    Leif

Re: [PATCH v4 4/5] arm: Add [U]EFI runtime services support

[Leif Lindholm]

On Mon, Jan 13, 2014 at 07:43:09PM +0100, Arnd Bergmann 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.
> 
> As far as I'm concerned there are no plans to have ACPI support on ARM32,
> so I wonder what the code to populate the ACPI tables is about. Can
> you clarify this?

Are you suggesting that I should #ifndef ARM in common code, or that I
should neglect to document what the common code will do with data it is
given by UEFI?

> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index 78a79a6a..1ab24cc 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -1853,6 +1853,20 @@ 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
> 
> What is the dependency on !CPU_BIG_ENDIAN?

Mainly on code not being implemented to byte-reverse UCS strings.

> We try hard to have
> all kernel code support both big-endian and little-endian, and
> I'm guessing there is a significant overlap between the people
> that want UEFI support and those that want big-endian kernels.

Not really. There might be some. Also, it is not necessarily the case
that those people want to run the big-endian kernel at EL2.

If a need is seen, this support can be added at a later date.

> > +struct efi_memory_map memmap;
> 
> "memmap" is not a good name for a global identifier, particularly because
> it's easily confused with the well-known "mem_map" array. This
> wants namespace prefix like you other variable, or a "static" tag,
> preferably both.

It is defined by include/linux/efi.h.

> > +/*
> > + * 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.
> 
>                    kexec

Ok.

> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index fa7d950..afaeb85 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -664,7 +664,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)
> 
> Maybe better #ifndef CONFIG_IA64? It seems that is the odd one out and
> all possible future architectures would be like x86 and ARM.

This was pointed out by Matt Fleming earlier, so it will change.
Mark Salter suggested introducing something like ARCH_USES_EFI_FACILITY
would be a bit cleaner.

/
    Leif

Re: [PATCH v4 4/5] arm: Add [U]EFI runtime services support

[Arnd Bergmann]

On Monday 13 January 2014, Leif Lindholm wrote:
> On Mon, Jan 13, 2014 at 07:43:09PM +0100, Arnd Bergmann 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.
> > 
> > As far as I'm concerned there are no plans to have ACPI support on ARM32,
> > so I wonder what the code to populate the ACPI tables is about. Can
> > you clarify this?
> 
> Are you suggesting that I should #ifndef ARM in common code, or that I
> should neglect to document what the common code will do with data it is
> given by UEFI?

It would probably be good to document the fact that it won't work,
possibly even having a BUG_ON statement in the code for this case.

> > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > > index 78a79a6a..1ab24cc 100644
> > > --- a/arch/arm/Kconfig
> > > +++ b/arch/arm/Kconfig
> > > @@ -1853,6 +1853,20 @@ 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
> > 
> > What is the dependency on !CPU_BIG_ENDIAN?
> 
> Mainly on code not being implemented to byte-reverse UCS strings.

Why would you byte-reverse /strings/? They normally just come in
order of the characters, and UTF-16 strings are already defined
as being big-endian or marked by the BOM character.

> > We try hard to have
> > all kernel code support both big-endian and little-endian, and
> > I'm guessing there is a significant overlap between the people
> > that want UEFI support and those that want big-endian kernels.
> 
> Not really. There might be some. Also, it is not necessarily the case
> that those people want to run the big-endian kernel at EL2.
> 
> If a need is seen, this support can be added at a later date.

Ok.

> > > +struct efi_memory_map memmap;
> > 
> > "memmap" is not a good name for a global identifier, particularly because
> > it's easily confused with the well-known "mem_map" array. This
> > wants namespace prefix like you other variable, or a "static" tag,
> > preferably both.
> 
> It is defined by include/linux/efi.h.

This seems to be a mistake: there is no user of this variable outside
of arch/x86/platform/efi/efi.c and arch/x86/platform/efi/efi_64.c.
I think it should just be moved into an x86 specific header file,
or preferably be renamed in the process. There is also efi->memmap,
which seems to be the same pointer.

Note that a number of drivers have local memmap variables.

	Arnd

Re: [PATCH v4 4/5] arm: Add [U]EFI runtime services support

[Leif Lindholm]

On Tue, Jan 14, 2014 at 07:52:32AM +0100, Arnd Bergmann wrote:
> > > > It uses the generic configuration table scanning to populate ACPI and
> > > > SMBIOS pointers.
> > > 
> > > As far as I'm concerned there are no plans to have ACPI support on ARM32,
> > > so I wonder what the code to populate the ACPI tables is about. Can
> > > you clarify this?
> > 
> > Are you suggesting that I should #ifndef ARM in common code, or that I
> > should neglect to document what the common code will do with data it is
> > given by UEFI?
> 
> It would probably be good to document the fact that it won't work,
> possibly even having a BUG_ON statement in the code for this case.

Why?

You'll only touch that pointer if you enable CONFIG_ACPI, and if you
do you probably want that address. Sounds a bit hostile to throw a BUG
in the face of someone who's (for example) just succeeded to get Linux
running on a Windows RT device.

> > > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > > > index 78a79a6a..1ab24cc 100644
> > > > --- a/arch/arm/Kconfig
> > > > +++ b/arch/arm/Kconfig
> > > > @@ -1853,6 +1853,20 @@ 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
> > > 
> > > What is the dependency on !CPU_BIG_ENDIAN?
> > 
> > Mainly on code not being implemented to byte-reverse UCS strings.
> 
> Why would you byte-reverse /strings/? They normally just come in
> order of the characters, and UTF-16 strings are already defined
> as being big-endian or marked by the BOM character.

Well, then that bit might just work[tm].

Although no other architectures supported by UEFI support big-endian,
so to be honest, I don't want to have to be the first one to validate
that in order to get the basic support into the kernel.

Some of the data structures might need swizzling though...
Again - if there is demand, this can be dealt with at a later date.

> > > > +struct efi_memory_map memmap;
> > > 
> > > "memmap" is not a good name for a global identifier, particularly because
> > > it's easily confused with the well-known "mem_map" array. This
> > > wants namespace prefix like you other variable, or a "static" tag,
> > > preferably both.
> > 
> > It is defined by include/linux/efi.h.
> 
> This seems to be a mistake: there is no user of this variable outside
> of arch/x86/platform/efi/efi.c and arch/x86/platform/efi/efi_64.c.
> I think it should just be moved into an x86 specific header file,
> or preferably be renamed in the process. There is also efi->memmap,
> which seems to be the same pointer.

Humm, seems I unknowingly removed the only remaining x86-external
reference to this variable when I made efi_lookup_mapped_address a
common function.
Yeah, changing this makes sense.

/
    Leif

Re: [PATCH v4 4/5] arm: Add [U]EFI runtime services support

[Arnd Bergmann]

On Tuesday 14 January 2014, Leif Lindholm wrote:
> On Tue, Jan 14, 2014 at 07:52:32AM +0100, Arnd Bergmann wrote:
> > > > > It uses the generic configuration table scanning to populate ACPI and
> > > > > SMBIOS pointers.
> > > > 
> > > > As far as I'm concerned there are no plans to have ACPI support on ARM32,
> > > > so I wonder what the code to populate the ACPI tables is about. Can
> > > > you clarify this?
> > > 
> > > Are you suggesting that I should #ifndef ARM in common code, or that I
> > > should neglect to document what the common code will do with data it is
> > > given by UEFI?
> > 
> > It would probably be good to document the fact that it won't work,
> > possibly even having a BUG_ON statement in the code for this case.
> 
> Why?
> 
> You'll only touch that pointer if you enable CONFIG_ACPI, and if you
> do you probably want that address. Sounds a bit hostile to throw a BUG
> in the face of someone who's (for example) just succeeded to get Linux
> running on a Windows RT device.

But we know that it can't work unless a lot of other things get changed
in the kernel.

> > > > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > > > > index 78a79a6a..1ab24cc 100644
> > > > > --- a/arch/arm/Kconfig
> > > > > +++ b/arch/arm/Kconfig
> > > > > @@ -1853,6 +1853,20 @@ 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
> > > > 
> > > > What is the dependency on !CPU_BIG_ENDIAN?
> > > 
> > > Mainly on code not being implemented to byte-reverse UCS strings.
> > 
> > Why would you byte-reverse /strings/? They normally just come in
> > order of the characters, and UTF-16 strings are already defined
> > as being big-endian or marked by the BOM character.
> 
> Well, then that bit might just work[tm].
> 
> Although no other architectures supported by UEFI support big-endian,
> so to be honest, I don't want to have to be the first one to validate
> that in order to get the basic support into the kernel.

I think there was a project to run UEFI on PowerPC on some stage, though
I can't find any code now.

> Some of the data structures might need swizzling though...
> Again - if there is demand, this can be dealt with at a later date.

Ok.

	Arnd

Re: [PATCH v4 4/5] arm: Add [U]EFI runtime services support

[Leif Lindholm]

On 14 January 2014 13:26, Arnd Bergmann <arnd@arndb.de> wrote:
>> > > Are you suggesting that I should #ifndef ARM in common code, or that I
>> > > should neglect to document what the common code will do with data it is
>> > > given by UEFI?
>> >
>> > It would probably be good to document the fact that it won't work,
>> > possibly even having a BUG_ON statement in the code for this case.
>>
>> Why?
>>
>> You'll only touch that pointer if you enable CONFIG_ACPI, and if you
>> do you probably want that address. Sounds a bit hostile to throw a BUG
>> in the face of someone who's (for example) just succeeded to get Linux
>> running on a Windows RT device.
>
> But we know that it can't work unless a lot of other things get changed
> in the kernel.

We know using ACPI cannot work without updates to the kernel.
That doesn't mean we need to throw a BUG just because the
firmware tells us a table exists.

>> Although no other architectures supported by UEFI support big-endian,
>> so to be honest, I don't want to have to be the first one to validate
>> that in order to get the basic support into the kernel.
>
> I think there was a project to run UEFI on PowerPC on some stage, though
> I can't find any code now.

That does sound familiar, but there is nothing in the specification.

/
    Leif

Re: [PATCH v4 1/5] arm: break part of __soft_restart out into separate function

[Will Deacon]

On Sat, Jan 11, 2014 at 01:05:20PM +0000, Leif Lindholm wrote:
> Certain operations can be considered mandatory for any piece of code
> preparing to switch off the MMU. Break this out into separate function
> dmap_prepare().

idmap_prepare. dmap_prepare sounds like something *completely* different!

With that:

  Acked-by: Will Deacon <will.deacon@arm.com>

Will

Re: [PATCH v4 2/5] arm: add new asm macro update_sctlr

[Will Deacon]

On Sat, Jan 11, 2014 at 01:05:21PM +0000, Leif Lindholm wrote:
> A new macro for setting/clearing bits in the SCTLR.
> 
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> Suggested-by: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm/include/asm/assembler.h |   13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h
> index 5c22851..aba6458 100644
> --- a/arch/arm/include/asm/assembler.h
> +++ b/arch/arm/include/asm/assembler.h
> @@ -383,4 +383,17 @@ THUMB(	orr	\reg , \reg , #PSR_T_BIT	)
>  #endif
>  	.endm
>  
> +#ifdef CONFIG_CPU_CP15
> +/* Macro for setting/clearing bits in sctlr */
> +	.macro update_sctlr, set:req, clear:req, tmp:req, tmp2:req
> +	mrc	p15, 0, \tmp, c1, c0, 0
> +	ldr	\tmp2, =\set
> +	orr	\tmp, \tmp, \tmp2
> +	ldr	\tmp2, =\clear
> +	mvn	\tmp2, \tmp2
> +	and	\tmp, \tmp, \tmp2
> +	mcr	p15, 0, \tmp, c1, c0, 0

I think this would be cleaner if you force the caller to put set and clear
into registers beforehand, rather than have to do the literal load every
time. Also, I don't think set and clear should be required (and then you can
lose tmp2 as well).

Will

Re: [PATCH v4 2/5] arm: add new asm macro update_sctlr

[Leif Lindholm]

On Wed, Jan 22, 2014 at 11:20:55AM +0000, Will Deacon wrote:
> > +#ifdef CONFIG_CPU_CP15
> > +/* Macro for setting/clearing bits in sctlr */
> > +	.macro update_sctlr, set:req, clear:req, tmp:req, tmp2:req
> > +	mrc	p15, 0, \tmp, c1, c0, 0
> > +	ldr	\tmp2, =\set
> > +	orr	\tmp, \tmp, \tmp2
> > +	ldr	\tmp2, =\clear
> > +	mvn	\tmp2, \tmp2
> > +	and	\tmp, \tmp, \tmp2
> > +	mcr	p15, 0, \tmp, c1, c0, 0
> 
> I think this would be cleaner if you force the caller to put set and clear
> into registers beforehand, rather than have to do the literal load every
> time. Also, I don't think set and clear should be required (and then you can
> lose tmp2 as well).

I can't figure out how to make register-parameters non-required
(i.e. conditionalise on whether an optional parameter was provided),
so my attempt of refactoring actually ends up using an additional
register:

#ifdef CONFIG_CPU_CP15
/* Macro for setting/clearing bits in sctlr */
	.macro	update_sctlr, set:req, clear:req, tmp:req
	mrc	p15, 0, \tmp, c1, c0, 0
	orr	\tmp, \set
	mvn	\clear, \clear
	and	\tmp, \tmp, \clear
	mcr	p15, 0, \tmp, c1, c0, 0
	.endm
#endif

If you think that's an improvement I can do that, and I have (just)
enough registers to spare.
If I'm being daft with my macro issues, do point it out.

/
    Leif

Re: [PATCH v4 2/5] arm: add new asm macro update_sctlr

[Will Deacon]

Hi Leif,

On Wed, Jan 29, 2014 at 06:28:05PM +0000, Leif Lindholm wrote:
> On Wed, Jan 22, 2014 at 11:20:55AM +0000, Will Deacon wrote:
> > > +#ifdef CONFIG_CPU_CP15
> > > +/* Macro for setting/clearing bits in sctlr */
> > > +	.macro update_sctlr, set:req, clear:req, tmp:req, tmp2:req
> > > +	mrc	p15, 0, \tmp, c1, c0, 0
> > > +	ldr	\tmp2, =\set
> > > +	orr	\tmp, \tmp, \tmp2
> > > +	ldr	\tmp2, =\clear
> > > +	mvn	\tmp2, \tmp2
> > > +	and	\tmp, \tmp, \tmp2
> > > +	mcr	p15, 0, \tmp, c1, c0, 0
> > 
> > I think this would be cleaner if you force the caller to put set and clear
> > into registers beforehand, rather than have to do the literal load every
> > time. Also, I don't think set and clear should be required (and then you can
> > lose tmp2 as well).
> 
> I can't figure out how to make register-parameters non-required
> (i.e. conditionalise on whether an optional parameter was provided),
> so my attempt of refactoring actually ends up using an additional
> register:
> 
> #ifdef CONFIG_CPU_CP15
> /* Macro for setting/clearing bits in sctlr */
> 	.macro	update_sctlr, set:req, clear:req, tmp:req
> 	mrc	p15, 0, \tmp, c1, c0, 0
> 	orr	\tmp, \set
> 	mvn	\clear, \clear
> 	and	\tmp, \tmp, \clear
> 	mcr	p15, 0, \tmp, c1, c0, 0
> 	.endm
> #endif
> 
> If you think that's an improvement I can do that, and I have (just)
> enough registers to spare.
> If I'm being daft with my macro issues, do point it out.

I was thinking along the lines of:

	.macro  update_sctlr, tmp:req, set=0, clear=0
	.if	\set
	orr ...
	.endif
	.if	\clear
	bic ...
	.endif
	.endm

however, that puts us back to the problem of having to pass immediates
instead of registers. Gas *does* seem to accept the following:

	.macro  update_sctlr, tmp:req, set=0, clear=0
	.if	\set != 0
	orr ...
	.endif
	.if	\clear != 0
	bic ...
	.endif
	.endm

which is filthy, so we'd need to see how beautiful the caller ends up being
to justify that!

Will

Re: [PATCH v4 2/5] arm: add new asm macro update_sctlr

[Mark Salter]

On Wed, 2014-01-29 at 18:28 +0000, Leif Lindholm wrote:
> On Wed, Jan 22, 2014 at 11:20:55AM +0000, Will Deacon wrote:
> > > +#ifdef CONFIG_CPU_CP15
> > > +/* Macro for setting/clearing bits in sctlr */
> > > +   .macro update_sctlr, set:req, clear:req, tmp:req, tmp2:req
> > > +   mrc     p15, 0, \tmp, c1, c0, 0
> > > +   ldr     \tmp2, =\set
> > > +   orr     \tmp, \tmp, \tmp2
> > > +   ldr     \tmp2, =\clear
> > > +   mvn     \tmp2, \tmp2
> > > +   and     \tmp, \tmp, \tmp2
> > > +   mcr     p15, 0, \tmp, c1, c0, 0
> > 
> > I think this would be cleaner if you force the caller to put set and clear
> > into registers beforehand, rather than have to do the literal load every
> > time. Also, I don't think set and clear should be required (and then you can
> > lose tmp2 as well).
> 
> I can't figure out how to make register-parameters non-required
> (i.e. conditionalise on whether an optional parameter was provided),
> so my attempt of refactoring actually ends up using an additional
> register:
> 

Register parameters are just strings, so how about this:

	.macro foo bar=, baz=
	.ifnc \bar,
	mov \bar,#0
	.endif
	.ifnc \baz,
	mov \baz,#1
	.endif
	.endm

	foo x0
	foo
	foo x1, x2
	foo ,x3

Results in:

0000000000000000 <.text>:
   0:	d2800000 	mov	x0, #0x0                   	// #0
   4:	d2800001 	mov	x1, #0x0                   	// #0
   8:	d2800022 	mov	x2, #0x1                   	// #1
   c:	d2800023 	mov	x3, #0x1                   	// #1

Re: [PATCH v4 2/5] arm: add new asm macro update_sctlr

[Leif Lindholm]

On Wed, Jan 29, 2014 at 03:58:44PM -0500, Mark Salter wrote:
> > (i.e. conditionalise on whether an optional parameter was provided),
> > so my attempt of refactoring actually ends up using an additional
> > register:
> > 
> 
> Register parameters are just strings, so how about this:
> 
> 	.macro foo bar=, baz=
> 	.ifnc \bar,
> 	mov \bar,#0
> 	.endif
> 	.ifnc \baz,
> 	mov \baz,#1
> 	.endif
> 	.endm
> 
> 	foo x0
> 	foo
> 	foo x1, x2
> 	foo ,x3
> 
> Results in:
> 
> 0000000000000000 <.text>:
>    0:	d2800000 	mov	x0, #0x0                   	// #0
>    4:	d2800001 	mov	x1, #0x0                   	// #0
>    8:	d2800022 	mov	x2, #0x1                   	// #1
>    c:	d2800023 	mov	x3, #0x1                   	// #1

Oh, that's neat - thanks!

Well, given that, I can think of two less horrible options:
1)
	.macro  update_sctlr, tmp:req, set=, clear=
        mrc	p15, 0, \tmp, c1, c0, 0
	.ifnc	\set,
        orr	\tmp, \set
	.endif
	.ifnc	\clear,
	mvn	\clear, \clear
	and	\tmp, \tmp, \clear
	.endif
        mcr	p15, 0, \tmp, c1, c0, 0
	.endm

With the two call sites in uefi_phys.S as:

	ldr	r5, =(CR_M)
	update_sctlr	r12, , r5
and
	ldr	r4, =(CR_I | CR_C | CR_M)
	update_sctlr	r12, r4

Which disassembles as:

  2c:   e3a05001        mov     r5, #1
  30:   ee11cf10        mrc     15, 0, ip, cr1, cr0, {0}
  34:   e1e05005        mvn     r5, r5
  38:   e00cc005        and     ip, ip, r5
  3c:   ee01cf10        mcr     15, 0, ip, cr1, cr0, {0}
and
  48:   e59f4034        ldr     r4, [pc, #52]   ; 84 <tmpstack+0x4>
  4c:   ee11cf10        mrc     15, 0, ip, cr1, cr0, {0}
  50:   e18cc004        orr     ip, ip, r4
  54:   ee01cf10        mcr     15, 0, ip, cr1, cr0, {0}


2)
	.macro update_sctlr, tmp:req, tmp2:req, set=, clear=
	mrc	p15, 0, \tmp, c1, c0, 0
	.ifnc	\set,
	ldr	\tmp2, =\set
	orr	\tmp, \tmp, \tmp2
	.endif
	.ifnc	\clear,
	ldr	\tmp2, =\clear
	mvn	\tmp2, \tmp2
	and	\tmp, \tmp, \tmp2
	.endif
	mcr	p15, 0, \tmp, c1, c0, 0
	.endm

With the two call sites in uefi_phys.S as: 

	update_sctlr	r4, r5, , (CR_M)
and
	update_sctlr	r4, r5, (CR_I | CR_C | CR_M)

Which disassembles as:

  2c:   ee114f10        mrc     15, 0, r4, cr1, cr0, {0}
  30:   e3a05001        mov     r5, #1
  34:   e1e05005        mvn     r5, r5
  38:   e0044005        and     r4, r4, r5
  3c:   ee014f10        mcr     15, 0, r4, cr1, cr0, {0}
and
  48:   ee114f10        mrc     15, 0, r4, cr1, cr0, {0}
  4c:   e59f5030        ldr     r5, [pc, #48]   ; 84 <tmpstack+0x4>
  50:   e1844005        orr     r4, r4, r5
  54:   ee014f10        mcr     15, 0, r4, cr1, cr0, {0}


The benefit of 2) is a cleaner call site, and one fewer register
used if setting and clearing simultaneously.

The benefit of 1) is that the macro could then easily be used with
the crval mask in mm/proc*.S

So, Will, which one do you want?

/
    Leif

Re: [PATCH v4 2/5] arm: add new asm macro update_sctlr

[Will Deacon]

On Thu, Jan 30, 2014 at 01:12:47PM +0000, Leif Lindholm wrote:
> Oh, that's neat - thanks!
> 
> Well, given that, I can think of two less horrible options:
> 1)
> 	.macro  update_sctlr, tmp:req, set=, clear=
>         mrc	p15, 0, \tmp, c1, c0, 0
> 	.ifnc	\set,
>         orr	\tmp, \set
> 	.endif
> 	.ifnc	\clear,
> 	mvn	\clear, \clear
> 	and	\tmp, \tmp, \clear

Can't you use bic here?

> 	.endif
>         mcr	p15, 0, \tmp, c1, c0, 0
> 	.endm
> 
> With the two call sites in uefi_phys.S as:
> 
> 	ldr	r5, =(CR_M)
> 	update_sctlr	r12, , r5
> and
> 	ldr	r4, =(CR_I | CR_C | CR_M)
> 	update_sctlr	r12, r4

These ldr= could be movs, right?

If so, I definitely prefer this to putting an ldr = into the macro itself
(option 2).

Cheers,

Will

Re: [PATCH v4 2/5] arm: add new asm macro update_sctlr

[Leif Lindholm]

On Mon, Feb 03, 2014 at 10:34:15AM +0000, Will Deacon wrote:
> On Thu, Jan 30, 2014 at 01:12:47PM +0000, Leif Lindholm wrote:
> > Oh, that's neat - thanks!
> > 
> > Well, given that, I can think of two less horrible options:
> > 1)
> > 	.macro  update_sctlr, tmp:req, set=, clear=
> >         mrc	p15, 0, \tmp, c1, c0, 0
> > 	.ifnc	\set,
> >         orr	\tmp, \set
> > 	.endif
> > 	.ifnc	\clear,
> > 	mvn	\clear, \clear
> > 	and	\tmp, \tmp, \clear
> 
> Can't you use bic here?

Yeah.

> > 	.endif
> >         mcr	p15, 0, \tmp, c1, c0, 0
> > 	.endm
> > 
> > With the two call sites in uefi_phys.S as:
> > 
> > 	ldr	r5, =(CR_M)
> > 	update_sctlr	r12, , r5
> > and
> > 	ldr	r4, =(CR_I | CR_C | CR_M)
> > 	update_sctlr	r12, r4
> 
> These ldr= could be movs, right?

The first one could.
The second one could be movw on armv7+.

> If so, I definitely prefer this to putting an ldr = into the macro itself
> (option 2).

And your preference between 1) and 2) is?

/
    Leif

Re: [PATCH v4 2/5] arm: add new asm macro update_sctlr

[Will Deacon]

On Mon, Feb 03, 2014 at 03:55:42PM +0000, Leif Lindholm wrote:
> On Mon, Feb 03, 2014 at 10:34:15AM +0000, Will Deacon wrote:
> > On Thu, Jan 30, 2014 at 01:12:47PM +0000, Leif Lindholm wrote:
> > > Oh, that's neat - thanks!
> > > 
> > > Well, given that, I can think of two less horrible options:
> > > 1)
> > > 	.macro  update_sctlr, tmp:req, set=, clear=
> > >         mrc	p15, 0, \tmp, c1, c0, 0
> > > 	.ifnc	\set,
> > >         orr	\tmp, \set
> > > 	.endif
> > > 	.ifnc	\clear,
> > > 	mvn	\clear, \clear
> > > 	and	\tmp, \tmp, \clear
> > 
> > Can't you use bic here?
> 
> Yeah.
> 
> > > 	.endif
> > >         mcr	p15, 0, \tmp, c1, c0, 0
> > > 	.endm
> > > 
> > > With the two call sites in uefi_phys.S as:
> > > 
> > > 	ldr	r5, =(CR_M)
> > > 	update_sctlr	r12, , r5
> > > and
> > > 	ldr	r4, =(CR_I | CR_C | CR_M)
> > > 	update_sctlr	r12, r4
> > 
> > These ldr= could be movs, right?
> 
> The first one could.
> The second one could be movw on armv7+.
> 
> > If so, I definitely prefer this to putting an ldr = into the macro itself
> > (option 2).
> 
> And your preference between 1) and 2) is?

(1), using bic and mov[tw] where possible.

Will

Re: [PATCH v4 2/5] arm: add new asm macro update_sctlr

[Rob Herring]

On Mon, Feb 3, 2014 at 10:00 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Mon, Feb 03, 2014 at 03:55:42PM +0000, Leif Lindholm wrote:
>> On Mon, Feb 03, 2014 at 10:34:15AM +0000, Will Deacon wrote:
>> > On Thu, Jan 30, 2014 at 01:12:47PM +0000, Leif Lindholm wrote:
>> > > Oh, that's neat - thanks!
>> > >
>> > > Well, given that, I can think of two less horrible options:
>> > > 1)
>> > >   .macro  update_sctlr, tmp:req, set=, clear=
>> > >         mrc       p15, 0, \tmp, c1, c0, 0
>> > >   .ifnc   \set,
>> > >         orr       \tmp, \set
>> > >   .endif
>> > >   .ifnc   \clear,
>> > >   mvn     \clear, \clear
>> > >   and     \tmp, \tmp, \clear
>> >
>> > Can't you use bic here?
>>
>> Yeah.
>>
>> > >   .endif
>> > >         mcr       p15, 0, \tmp, c1, c0, 0
>> > >   .endm
>> > >
>> > > With the two call sites in uefi_phys.S as:
>> > >
>> > >   ldr     r5, =(CR_M)
>> > >   update_sctlr    r12, , r5
>> > > and
>> > >   ldr     r4, =(CR_I | CR_C | CR_M)
>> > >   update_sctlr    r12, r4
>> >
>> > These ldr= could be movs, right?
>>
>> The first one could.
>> The second one could be movw on armv7+.
>>
>> > If so, I definitely prefer this to putting an ldr = into the macro itself
>> > (option 2).
>>
>> And your preference between 1) and 2) is?
>
> (1), using bic and mov[tw] where possible.

Using mov[tw] will break on V6 enabled builds.

Rob

Re: [PATCH v4 2/5] arm: add new asm macro update_sctlr

[Leif Lindholm]

On Mon, Feb 03, 2014 at 04:00:51PM +0000, Will Deacon wrote:
> > > > With the two call sites in uefi_phys.S as:
> > > > 
> > > > 	ldr	r5, =(CR_M)
> > > > 	update_sctlr	r12, , r5
> > > > and
> > > > 	ldr	r4, =(CR_I | CR_C | CR_M)
> > > > 	update_sctlr	r12, r4
> > > 
> > > These ldr= could be movs, right?
> > 
> > The first one could.
> > The second one could be movw on armv7+.
> > 
> > > If so, I definitely prefer this to putting an ldr = into the macro itself
> > > (option 2).
> > 
> > And your preference between 1) and 2) is?
> 
> (1), using bic and mov[tw] where possible.

(1): ok, thanks.

bic: sure, that was an oversight.

mov[tw]: why?
Then we end up battling different available immediate fields in A32/T32
instruction sets and v5/v6/v7 architecture versions.

/
    Leif

Re: [PATCH v4 2/5] arm: add new asm macro update_sctlr

[Will Deacon]

On Mon, Feb 03, 2014 at 04:46:36PM +0000, Leif Lindholm wrote:
> On Mon, Feb 03, 2014 at 04:00:51PM +0000, Will Deacon wrote:
> > > > > With the two call sites in uefi_phys.S as:
> > > > > 
> > > > > 	ldr	r5, =(CR_M)
> > > > > 	update_sctlr	r12, , r5
> > > > > and
> > > > > 	ldr	r4, =(CR_I | CR_C | CR_M)
> > > > > 	update_sctlr	r12, r4
> > > > 
> > > > These ldr= could be movs, right?
> > > 
> > > The first one could.
> > > The second one could be movw on armv7+.
> > > 
> > > > If so, I definitely prefer this to putting an ldr = into the macro itself
> > > > (option 2).
> > > 
> > > And your preference between 1) and 2) is?
> > 
> > (1), using bic and mov[tw] where possible.
> 
> (1): ok, thanks.
> 
> bic: sure, that was an oversight.
> 
> mov[tw]: why?
> Then we end up battling different available immediate fields in A32/T32
> instruction sets and v5/v6/v7 architecture versions.

I was making the assumption that UEFI was going to be v7 only... is this not
true?

Will

Re: [PATCH v4 2/5] arm: add new asm macro update_sctlr

[Leif Lindholm]

On Mon, Feb 03, 2014 at 04:57:18PM +0000, Will Deacon wrote:
> > mov[tw]: why?
> > Then we end up battling different available immediate fields in A32/T32
> > instruction sets and v5/v6/v7 architecture versions.
> 
> I was making the assumption that UEFI was going to be v7 only... is this not
> true?

There is no such requirement in the specification.
It even mentions requirements for ARMv4 in one place :)

But I also don't understand why ldr= should be avoided.
This is not performance sensitive (called once on system boot), and
it's already executing with the caches off, so even if it ends up
being a literal load it does not pollute.

/
    Leif