2013-06-25 18:06:20

by Leif Lindholm

[permalink] [raw]
Subject: [PATCH 0/4] arm: [U]EFI runtime services support

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
to the kernel via FDT entries.

This patchset depends on the previously submitted early_ioremap() patchset.

Leif Lindholm (4):
Documentation: arm: [U]EFI runtime services
x86: efi: break efi_lookup_mapped_addr out to generic code
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 | 39 ++++
arch/arm/Kconfig | 15 ++
arch/arm/include/asm/efi.h | 22 ++
arch/arm/kernel/Makefile | 2 +
arch/arm/kernel/efi.c | 456 +++++++++++++++++++++++++++++++++++++
arch/arm/kernel/efi_phys.S | 59 +++++
arch/arm/kernel/setup.c | 5 +
arch/x86/platform/efi/efi.c | 28 ---
drivers/firmware/efi/Makefile | 2 +-
drivers/firmware/efi/efi-helper.c | 33 +++
init/main.c | 6 +
12 files changed, 641 insertions(+), 29 deletions(-)
create mode 100644 Documentation/arm/uefi.txt
create mode 100644 arch/arm/include/asm/efi.h
create mode 100644 arch/arm/kernel/efi.c
create mode 100644 arch/arm/kernel/efi_phys.S
create mode 100644 drivers/firmware/efi/efi-helper.c

--
1.7.10.4


2013-06-25 18:06:23

by Leif Lindholm

[permalink] [raw]
Subject: [PATCH 1/4] Documentation: arm: [U]EFI runtime services

This patch provides documentation of the [U]EFI runtime services and
configuration features.

Signed-off-by: Leif Lindholm <[email protected]>
---
Documentation/arm/00-INDEX | 3 +++
Documentation/arm/uefi.txt | 39 +++++++++++++++++++++++++++++++++++++++
2 files changed, 42 insertions(+)
create mode 100644 Documentation/arm/uefi.txt

diff --git a/Documentation/arm/00-INDEX b/Documentation/arm/00-INDEX
index 4978456..87e01d1 100644
--- a/Documentation/arm/00-INDEX
+++ b/Documentation/arm/00-INDEX
@@ -36,3 +36,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..5c48271
--- /dev/null
+++ b/Documentation/arm/uefi.txt
@@ -0,0 +1,39 @@
+The nomenclature EFI and UEFI are used interchangeably in this document.
+
+The implementation depends on receiving pointers to the UEFI memory map
+and System Table in a Flattened Device Tree - so is only available with
+CONFIG_OF.
+
+It (early) parses the FDT for the following parameters:
+- 'efi-system-table':
+ Physical address of the system table. (required)
+- 'efi-runtime-mmap':
+ Physical address of an EFI memory map, containing at least
+ the regions to be preserved. (required)
+- 'efi-runtime-mmap-size':
+ Size in bytes of the provided memory map. (required)
+- 'efi-mmap-desc-size':
+ Size of each descriptor in the memory map. (override default)
+- 'efi-mmap-desc-ver':
+ Memory descriptor format version. (override default)
+
+Since UEFI firmware on ARM systems are required to use a 1:1 memory map
+even on LPAE-capable systems, the above fields are 32-bit regardless.
+
+It also depends on early_ioremap to parse the memory map and preserve
+the regions required for runtime services.
+
+For actually enabling [U]EFI support, enable:
+- CONFIG_EFI=y
+- CONFIG_EFI_VARS=y or m
+
+After the kernel has mapped the required regions into its address space,
+a SetVirtualAddressMap() call is made into UEFI in order to update
+relocations. This call must be performed with all the code in a 1:1
+mapping. This implementation achieves this by temporarily disabling the
+MMU for the duration of this call. This can only be done safely:
+- before secondary CPUs are brought online.
+- after early_initcalls have completed, sinze it uses setup_mm_for_reboot().
+
+For verbose debug messages, specify 'uefi_debug' on the kernel command
+line.
--
1.7.10.4

2013-06-25 18:06:29

by Leif Lindholm

[permalink] [raw]
Subject: [PATCH 3/4] arm: Add [U]EFI runtime services support

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 also locates any presented SMBIOS configuration table and stores it
for potential later use by DMI.

Signed-off-by: Leif Lindholm <[email protected]>
---
arch/arm/Kconfig | 15 ++
arch/arm/include/asm/efi.h | 22 +++
arch/arm/kernel/Makefile | 2 +
arch/arm/kernel/efi.c | 456 ++++++++++++++++++++++++++++++++++++++++++++
arch/arm/kernel/efi_phys.S | 59 ++++++
arch/arm/kernel/setup.c | 5 +
6 files changed, 559 insertions(+)
create mode 100644 arch/arm/include/asm/efi.h
create mode 100644 arch/arm/kernel/efi.c
create mode 100644 arch/arm/kernel/efi_phys.S

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index bf8e55d..022d2eb 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1763,6 +1763,19 @@ config EARLY_IOREMAP
Provides a mechanism for kernel initialisation code to temporarily
map, in a highmem-agnostic way, memory pages in before paging_init().

+config EFI
+bool "UEFI runtime service support"
+ depends on OF
+ 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 should
+ continue to boot on existing non-UEFI platforms.
+
config SECCOMP
bool
prompt "Enable seccomp to safely compute untrusted bytecode"
@@ -2217,6 +2230,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/efi.h b/arch/arm/include/asm/efi.h
new file mode 100644
index 0000000..aead94c
--- /dev/null
+++ b/arch/arm/include/asm/efi.h
@@ -0,0 +1,22 @@
+#ifndef _ASM_ARM_EFI_H
+#define _ASM_ARM_EFI_H
+
+#include <asm/mach/map.h>
+
+extern int efi_memblock_arm_reserve_range(void);
+
+typedef efi_status_t efi_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 efi_phys_call(u32, u32, u32, efi_memory_desc_t *,
+ efi_set_virtual_address_map_t *);
+
+#define efi_remap(cookie, size) __arm_ioremap((cookie), (size), MT_MEMORY)
+#define efi_ioremap(cookie, size) __arm_ioremap((cookie), (size), MT_DEVICE)
+#define efi_unmap(cookie) __arm_iounmap((cookie))
+#define efi_iounmap(cookie) __arm_iounmap((cookie))
+
+#endif /* _ASM_ARM_EFI_H */
diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index 5f3338e..12b9c30 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -84,4 +84,6 @@ obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
obj-$(CONFIG_ARM_VIRT_EXT) += hyp-stub.o
obj-$(CONFIG_ARM_PSCI) += psci.o

+obj-$(CONFIG_EFI) += efi.o efi_phys.o
+
extra-y := $(head-y) vmlinux.lds
diff --git a/arch/arm/kernel/efi.c b/arch/arm/kernel/efi.c
new file mode 100644
index 0000000..43ecf0b
--- /dev/null
+++ b/arch/arm/kernel/efi.c
@@ -0,0 +1,456 @@
+/*
+ * Extensible Firmware Interface
+ *
+ * Based on Extensible Firmware Interface Specification version 2.3.1
+ *
+ * Copyright (C) 2013 Linaro Ltd.
+ *
+ */
+
+#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/efi.h>
+#include <asm/idmap.h>
+#include <asm/tlbflush.h>
+
+struct efi efi;
+EXPORT_SYMBOL(efi);
+struct efi_memory_map memmap;
+
+static efi_runtime_services_t *runtime;
+
+static phys_addr_t efi_system_table;
+static phys_addr_t efi_boot_mmap;
+static u32 efi_boot_mmap_size;
+static u32 efi_mmap_desc_size;
+static u32 efi_mmap_desc_ver;
+
+/* Default memory map descriptor information */
+#define DESC_SIZE 48
+#define DESC_VER 1
+
+/* If you're planning to wire up a debugger and debug the UEFI side ... */
+#undef KEEP_ALL_REGIONS
+#define KEEP_BOOT_SERVICES_REGIONS
+
+static int __initdata uefi_debug;
+static int __init uefi_debug_setup(char *str)
+{
+ uefi_debug = 1;
+
+ return 0;
+}
+early_param("uefi_debug", uefi_debug_setup);
+
+static int __init fdt_find_efi_params(unsigned long node, const char *uname,
+ int depth, void *data)
+{
+ unsigned long len;
+ __be32 *prop;
+
+ if (depth != 1 ||
+ (strcmp(uname, "chosen") != 0 && strcmp(uname, "chosen@0") != 0))
+ return 0;
+
+ pr_info("Getting EFI parameters from FDT.\n");
+
+ prop = of_get_flat_dt_prop(node, "efi-system-table", &len);
+ if (!prop)
+ return 0;
+ efi_system_table = of_read_ulong(prop, len/4);
+
+ prop = of_get_flat_dt_prop(node, "efi-runtime-mmap", &len);
+ if (!prop)
+ return 0;
+ efi_boot_mmap = of_read_ulong(prop, len/4);
+
+ prop = of_get_flat_dt_prop(node, "efi-runtime-mmap-size", &len);
+ if (!prop)
+ return 0;
+ efi_boot_mmap_size = of_read_ulong(prop, len/4);
+
+ prop = of_get_flat_dt_prop(node, "efi-mmap-desc-size", &len);
+ if (prop)
+ efi_mmap_desc_size = of_read_ulong(prop, len/4);
+ else
+ efi_mmap_desc_size = DESC_SIZE;
+
+ prop = of_get_flat_dt_prop(node, "efi-mmap-desc-ver", &len);
+ if (prop)
+ efi_mmap_desc_ver = of_read_ulong(prop, len/4);
+ else
+ efi_mmap_desc_ver = DESC_VER;
+
+ if (uefi_debug) {
+ pr_info(" EFI system table @ 0x%08x\n",
+ (unsigned int) efi_system_table);
+ pr_info(" EFI mmap @ 0x%08x\n",
+ (unsigned int) efi_boot_mmap);
+ pr_info(" EFI mmap size = 0x%08x\n",
+ (unsigned int) efi_boot_mmap_size);
+ pr_info(" EFI mmap descriptor size = 0x%08x\n",
+ (unsigned int) efi_mmap_desc_size);
+ pr_info(" EFI mmap descriptor version = 0x%08x\n",
+ (unsigned int) efi_mmap_desc_ver);
+ }
+
+ return 1;
+}
+
+static int __init uefi_config_init(void)
+{
+ efi_char16_t *c16;
+ char vendor[100] = "unknown";
+ efi_config_table_t *config_tables;
+ u32 nr_tables;
+ int i;
+
+ efi.systab = early_ioremap(efi_system_table,
+ sizeof(efi_system_table_t));
+
+ /*
+ * Verify the EFI Table
+ */
+ if (efi.systab == NULL)
+ panic("Whoa! Can't find EFI system table.\n");
+ if (efi.systab->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
+ panic("Whoa! EFI system table signature incorrect\n");
+ if ((efi.systab->hdr.revision >> 16) == 0)
+ pr_warn("Warning: EFI 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_ioremap(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("EFI v%u.%.02u by %s\n",
+ efi.systab->hdr.revision >> 16,
+ efi.systab->hdr.revision & 0xffff, vendor);
+
+
+ nr_tables = efi.systab->nr_tables;
+
+ config_tables = early_ioremap(efi.systab->tables,
+ sizeof(efi_config_table_t) * nr_tables);
+
+ for (i = 0; i < nr_tables; i++) {
+ efi_guid_t guid;
+ unsigned long table;
+ u8 str[38];
+
+ guid = config_tables[i].guid;
+ table = config_tables[i].table;
+
+ efi_guid_unparse(&guid, str);
+ if (uefi_debug)
+ pr_info(" Config Table: UUID=%s @ 0x%08x\n",
+ str, (u32)table);
+ if (!efi_guidcmp(guid, SMBIOS_TABLE_GUID))
+ efi.smbios = table;
+ }
+
+ early_iounmap(config_tables, sizeof(efi_config_table_t) * nr_tables);
+ early_iounmap(c16, sizeof(vendor));
+ early_iounmap(efi.systab, sizeof(efi_system_table_t));
+
+ return 0;
+}
+
+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;
+ }
+
+ return 1;
+}
+
+static __initdata struct {
+ u32 type;
+ const char *name;
+} memory_type_name_map[] = {
+ {EFI_RESERVED_TYPE, "EFI reserved"},
+ {EFI_LOADER_CODE, "EFI loader code"},
+ {EFI_LOADER_DATA, "EFI loader data"},
+ {EFI_BOOT_SERVICES_CODE, "EFI boot services code"},
+ {EFI_BOOT_SERVICES_DATA, "EFI boot services data"},
+ {EFI_RUNTIME_SERVICES_CODE, "EFI runtime services code"},
+ {EFI_RUNTIME_SERVICES_DATA, "EFI runtime services data"},
+ {EFI_CONVENTIONAL_MEMORY, "EFI conventional memory"},
+ {EFI_UNUSABLE_MEMORY, "EFI unusable memory"},
+ {EFI_ACPI_RECLAIM_MEMORY, "EFI ACPI reclaim memory"},
+ {EFI_ACPI_MEMORY_NVS, "EFI ACPI memory nvs"},
+ {EFI_MEMORY_MAPPED_IO, "EFI memory mapped I/O"},
+ {EFI_MEMORY_MAPPED_IO_PORT_SPACE, "EFI memory mapped I/O port space"},
+ {EFI_PAL_CODE, "EFI 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;
+
+ memmap.phys_map = early_ioremap(efi_boot_mmap, efi_boot_mmap_size);
+
+ memmap.desc_size = efi_mmap_desc_size;
+ memmap.desc_version = efi_mmap_desc_ver;
+ memmap.map_end = (void *) memmap.phys_map + efi_boot_mmap_size;
+ memmap.nr_map = 0;
+
+ if (uefi_debug)
+ pr_info("Processing EFI memory map:\n");
+
+ for (p = memmap.phys_map; p < memmap.map_end; p += memmap.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++;
+ }
+
+ if (uefi_debug)
+ pr_info("%d regions preserved.\n", memmap.nr_map);
+
+ early_iounmap(memmap.phys_map, efi_boot_mmap_size);
+
+ return 0;
+}
+
+int __init efi_memblock_arm_reserve_range(void)
+{
+ /* Grab system table location out of FDT (or ACPI) */
+ of_scan_flat_dt(fdt_find_efi_params, NULL);
+
+ if (!efi_system_table || !efi_boot_mmap || !efi_boot_mmap_size)
+ return 0;
+
+ remove_regions();
+
+ uefi_config_init();
+
+ 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();
+}
+
+/*
+ * This function switches the EFI 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)
+{
+ efi_phys_call_t *phys_set_map;
+ efi_status_t status;
+
+ phys_call_prologue();
+
+ phys_set_map = (void *)(unsigned long)virt_to_phys(efi_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;
+}
+
+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)efi_remap(paddr, size) & 0xffffffffUL);
+ else
+ va = (u64)((u32)efi_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 void __init remap_regions(void)
+{
+ void *p, *next;
+ efi_memory_desc_t *md;
+
+ memmap.phys_map = efi_remap(efi_boot_mmap, efi_boot_mmap_size);
+ memmap.map_end = (void *)memmap.phys_map + efi_boot_mmap_size;
+
+ /* Allocate space for the physical region map */
+ memmap.map = kzalloc(memmap.nr_map * memmap.desc_size, GFP_KERNEL);
+ if (!memmap.map)
+ return;
+
+ next = memmap.map;
+ for (p = memmap.phys_map; p < memmap.map_end; p += memmap.desc_size) {
+ md = p;
+ if (is_discardable_region(md))
+ continue;
+
+ if (remap_region(p, next))
+ next += memmap.desc_size;
+ }
+
+ memmap.map_end = next;
+
+ efi_unmap(memmap.phys_map);
+
+ efi.systab = efi_lookup_mapped_addr(efi_system_table);
+ /*
+ * 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);
+}
+
+
+/*
+ * Called explicitly from init/mm.c
+ */
+void __init efi_enter_virtual_mode(void)
+{
+ efi_status_t status;
+
+ if (efi.systab == NULL) {
+ pr_info("No EFI system table - EFI services will not be available.\n");
+ return;
+ } else {
+ pr_info("Remapping and enabling EFI services.\n");
+ }
+
+ /* Map the regions we memblock_remove:d earlier into kernel
+ address space */
+ remap_regions();
+
+ /* 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);
+
+ /* 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;
+}
diff --git a/arch/arm/kernel/efi_phys.S b/arch/arm/kernel/efi_phys.S
new file mode 100644
index 0000000..e36cc17
--- /dev/null
+++ b/arch/arm/kernel/efi_phys.S
@@ -0,0 +1,59 @@
+/*
+ * arch/arm/kernel/efi_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
+@ efi_phys_call(a, b, c, d, *f)
+ .align 5
+ .pushsection .idmap.text, "ax"
+ENTRY(efi_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(efi_phys_call)
+ .popsection
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 290c561..e36c2a6 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>
@@ -56,6 +57,7 @@
#include <asm/unwind.h>
#include <asm/memblock.h>
#include <asm/virt.h>
+#include <asm/efi.h>

#include "atags.h"

@@ -790,6 +792,9 @@ void __init setup_arch(char **cmdline_p)
sanity_check_meminfo();
arm_memblock_init(&meminfo, mdesc);

+ if (efi_enabled(EFI_BOOT))
+ efi_memblock_arm_reserve_range();
+
paging_init(mdesc);
request_standard_resources(mdesc);

--
1.7.10.4

2013-06-25 18:06:31

by Leif Lindholm

[permalink] [raw]
Subject: [PATCH 4/4] init: efi: arm: enable (U)EFI runtime services on arm

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 <[email protected]>
---
init/main.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/init/main.c b/init/main.c
index 9484f4b..c61706e 100644
--- a/init/main.c
+++ b/init/main.c
@@ -872,6 +872,12 @@ static noinline void __init kernel_init_freeable(void)
smp_prepare_cpus(setup_max_cpus);

do_pre_smp_initcalls();
+
+#ifdef CONFIG_ARM
+ if (efi_enabled(EFI_RUNTIME_SERVICES))
+ efi_enter_virtual_mode();
+#endif
+
lockup_detector_init();

smp_init();
--
1.7.10.4

2013-06-25 18:06:59

by Leif Lindholm

[permalink] [raw]
Subject: [PATCH 2/4] x86: efi: break efi_lookup_mapped_addr out to generic code

efi_lookup_mapped_addr is a handy helper function for translating
a physical address to the corresponding virtual one by scanning
through memmap.map.

This patch breaks it out into a new file for use elsewhere.

Signed-off-by: Leif Lindholm <[email protected]>
---
arch/x86/platform/efi/efi.c | 28 ----------------------------
drivers/firmware/efi/Makefile | 2 +-
drivers/firmware/efi/efi-helper.c | 33 +++++++++++++++++++++++++++++++++
3 files changed, 34 insertions(+), 29 deletions(-)
create mode 100644 drivers/firmware/efi/efi-helper.c

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 5ae2eb0..d1a1b6b 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -814,34 +814,6 @@ static void __init runtime_code_page_mkexec(void)
}
}

-/*
- * We can't ioremap data in EFI boot services RAM, because we've already mapped
- * it as RAM. So, look it up in the existing EFI memory map instead. Only
- * callable after efi_enter_virtual_mode and before efi_free_boot_services.
- */
-void __iomem *efi_lookup_mapped_addr(u64 phys_addr)
-{
- void *p;
- if (WARN_ON(!memmap.map))
- return NULL;
- for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
- efi_memory_desc_t *md = p;
- u64 size = md->num_pages << EFI_PAGE_SHIFT;
- u64 end = md->phys_addr + size;
- if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
- md->type != EFI_BOOT_SERVICES_CODE &&
- md->type != EFI_BOOT_SERVICES_DATA)
- continue;
- if (!md->virt_addr)
- continue;
- if (phys_addr >= md->phys_addr && phys_addr < end) {
- phys_addr += md->virt_addr - md->phys_addr;
- return (__force void __iomem *)(unsigned long)phys_addr;
- }
- }
- return NULL;
-}
-
void efi_memory_uc(u64 addr, unsigned long size)
{
unsigned long page_shift = 1UL << EFI_PAGE_SHIFT;
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 99245ab..629a513 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -1,6 +1,6 @@
#
# Makefile for linux kernel
#
-obj-y += efi.o vars.o
+obj-y += efi.o vars.o efi-helper.o
obj-$(CONFIG_EFI_VARS) += efivars.o
obj-$(CONFIG_EFI_VARS_PSTORE) += efi-pstore.o
diff --git a/drivers/firmware/efi/efi-helper.c b/drivers/firmware/efi/efi-helper.c
new file mode 100644
index 0000000..c5c2c72
--- /dev/null
+++ b/drivers/firmware/efi/efi-helper.c
@@ -0,0 +1,33 @@
+/*
+ * Common [U]EFI support helper functions across architectures.
+ */
+
+#include <linux/efi.h>
+
+/*
+ * We can't ioremap data in EFI boot services RAM, because we've already mapped
+ * it as RAM. So, look it up in the existing EFI memory map instead. Only
+ * callable after efi_enter_virtual_mode and before efi_free_boot_services.
+ */
+void __iomem *efi_lookup_mapped_addr(u64 phys_addr)
+{
+ void *p;
+ if (WARN_ON(!memmap.map))
+ return NULL;
+ for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
+ efi_memory_desc_t *md = p;
+ u64 size = md->num_pages << EFI_PAGE_SHIFT;
+ u64 end = md->phys_addr + size;
+ if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
+ md->type != EFI_BOOT_SERVICES_CODE &&
+ md->type != EFI_BOOT_SERVICES_DATA)
+ continue;
+ if (!md->virt_addr)
+ continue;
+ if (phys_addr >= md->phys_addr && phys_addr < end) {
+ phys_addr += md->virt_addr - md->phys_addr;
+ return (__force void __iomem *)(unsigned long)phys_addr;
+ }
+ }
+ return NULL;
+}
--
1.7.10.4

2013-06-25 18:20:33

by Matthew Garrett

[permalink] [raw]
Subject: Re: [PATCH 3/4] arm: Add [U]EFI runtime services support

On Tue, Jun 25, 2013 at 07:11:02PM +0100, 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 also locates any presented SMBIOS configuration table and stores it
> for potential later use by DMI.

This appears to duplicate code that's already duplicated between x86 and
ia64. We made a mistake there originally - let's not do it again. Having
this code in three places is inevitably going to lead to skew and missed
bugfixes.

--
Matthew Garrett | [email protected]

2013-06-25 18:46:43

by Christopher Covington

[permalink] [raw]
Subject: Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services

On 06/25/2013 02:11 PM, Leif Lindholm wrote:
> This patch provides documentation of the [U]EFI runtime services and
> configuration features.
>
> Signed-off-by: Leif Lindholm <[email protected]>
> ---
> Documentation/arm/00-INDEX | 3 +++
> Documentation/arm/uefi.txt | 39 +++++++++++++++++++++++++++++++++++++++
> 2 files changed, 42 insertions(+)
> create mode 100644 Documentation/arm/uefi.txt
>
> diff --git a/Documentation/arm/00-INDEX b/Documentation/arm/00-INDEX
> index 4978456..87e01d1 100644
> --- a/Documentation/arm/00-INDEX
> +++ b/Documentation/arm/00-INDEX
> @@ -36,3 +36,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..5c48271
> --- /dev/null
> +++ b/Documentation/arm/uefi.txt
> @@ -0,0 +1,39 @@
> +The nomenclature EFI and UEFI are used interchangeably in this document.
> +
> +The implementation depends on receiving pointers to the UEFI memory map
> +and System Table in a Flattened Device Tree - so is only available with
> +CONFIG_OF.
> +
> +It (early) parses the FDT for the following parameters:
> +- 'efi-system-table':
> + Physical address of the system table. (required)
> +- 'efi-runtime-mmap':
> + Physical address of an EFI memory map, containing at least
> + the regions to be preserved. (required)
> +- 'efi-runtime-mmap-size':
> + Size in bytes of the provided memory map. (required)
> +- 'efi-mmap-desc-size':
> + Size of each descriptor in the memory map. (override default)
> +- 'efi-mmap-desc-ver':
> + Memory descriptor format version. (override default)
> +
> +Since UEFI firmware on ARM systems are required to use a 1:1 memory map
> +even on LPAE-capable systems, the above fields are 32-bit regardless.
> +
> +It also depends on early_ioremap to parse the memory map and preserve
> +the regions required for runtime services.
> +
> +For actually enabling [U]EFI support, enable:
> +- CONFIG_EFI=y
> +- CONFIG_EFI_VARS=y or m
> +
> +After the kernel has mapped the required regions into its address space,
> +a SetVirtualAddressMap() call is made into UEFI in order to update
> +relocations. This call must be performed with all the code in a 1:1
> +mapping. This implementation achieves this by temporarily disabling the
> +MMU for the duration of this call. This can only be done safely:
> +- before secondary CPUs are brought online.
> +- after early_initcalls have completed, sinze it uses setup_mm_for_reboot().

since

> +
> +For verbose debug messages, specify 'uefi_debug' on the kernel command
> +line.
>

Christopher

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by the Linux Foundation.

2013-06-25 23:43:07

by Stephen Warren

[permalink] [raw]
Subject: Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services

On 06/25/2013 12:11 PM, Leif Lindholm wrote:
> This patch provides documentation of the [U]EFI runtime services and
> configuration features.


> diff --git a/Documentation/arm/uefi.txt b/Documentation/arm/uefi.txt

> +The implementation depends on receiving pointers to the UEFI memory map
> +and System Table in a Flattened Device Tree - so is only available with
> +CONFIG_OF.
> +
> +It (early) parses the FDT for the following parameters:

Part of this document (the raw requirements for DT content, rather than
the discussion of OS implementation/behaviour in parsing/interpreting
the properties) should be part of a file in
Documentation/devicetree/bindings/ (arm/uefi.txt?).

What node are these properties expected to be contained within?

Shouldn't that node be required to contain a compatible value, which
would define the schema for the other properties?

> +- 'efi-system-table':
> + Physical address of the system table. (required)
> +- 'efi-runtime-mmap':
> + Physical address of an EFI memory map, containing at least
> + the regions to be preserved. (required)
> +- 'efi-runtime-mmap-size':
> + Size in bytes of the provided memory map. (required)
> +- 'efi-mmap-desc-size':
> + Size of each descriptor in the memory map. (override default)
> +- 'efi-mmap-desc-ver':
> + Memory descriptor format version. (override default)
> +
> +Since UEFI firmware on ARM systems are required to use a 1:1 memory map
> +even on LPAE-capable systems, the above fields are 32-bit regardless.

What about ARMv8? Is the intention to have a separate definition for the
UEFI bindings on ARMv8, so that compatibility isn't an issue? What if a
future version of UEFI allows LPAE usage?

It may be better to explicitly state that the size of those properties
is either #address-cells from the parent node (presumably the top-level
of the DT), and/or introduce some property to explicitly state the size
of the properties. Those mechanisms would allow forward-compatibility to
LPAE usage or ARMv8 without requiring the text of the binding definition
to change.

Also, it seems legal to state the physical addresses using 64-bits even
if the actual values themselves are restricted to 32-bit range by the
UEFI spec. Illegal values would presumably cause SW that interprets them
to fail error-checks, and be rejected.

> +After the kernel has mapped the required regions into its address space,
> +a SetVirtualAddressMap() call is made into UEFI in order to update
> +relocations. This call must be performed with all the code in a 1:1

Presumably "all the code" also includes "all .data and .bss", or
whatever the UEFI-equivalent may be? I'm not familiar with UEFI at all;
does the "EFI memory map" mentioned above describe all the memory
regions that must be mapped to use UEFI?

> +mapping. This implementation achieves this by temporarily disabling the
> +MMU for the duration of this call. This can only be done safely:
> +- before secondary CPUs are brought online.
> +- after early_initcalls have completed, sinze it uses setup_mm_for_reboot().

2013-06-26 13:14:03

by Grant Likely

[permalink] [raw]
Subject: Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services

On Tue, Jun 25, 2013 at 7:11 PM, Leif Lindholm <[email protected]> wrote:
> This patch provides documentation of the [U]EFI runtime services and
> configuration features.
>
> Signed-off-by: Leif Lindholm <[email protected]>
> ---
> Documentation/arm/00-INDEX | 3 +++
> Documentation/arm/uefi.txt | 39 +++++++++++++++++++++++++++++++++++++++
> 2 files changed, 42 insertions(+)
> create mode 100644 Documentation/arm/uefi.txt
>
> diff --git a/Documentation/arm/00-INDEX b/Documentation/arm/00-INDEX
> index 4978456..87e01d1 100644
> --- a/Documentation/arm/00-INDEX
> +++ b/Documentation/arm/00-INDEX
> @@ -36,3 +36,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..5c48271
> --- /dev/null
> +++ b/Documentation/arm/uefi.txt
> @@ -0,0 +1,39 @@
> +The nomenclature EFI and UEFI are used interchangeably in this document.
> +
> +The implementation depends on receiving pointers to the UEFI memory map
> +and System Table in a Flattened Device Tree - so is only available with
> +CONFIG_OF.
> +
> +It (early) parses the FDT for the following parameters:

Need to state which node these properties can be found in. I recommend /chosen

I would also prefix all of the following properties with "linux,"
since they are very much about what the Linux kernel needs for
booting. EFI stub will be creating these properties specifically for
Linux, and the LinuxLoader should do the same (until we eventually
kill it).

> +- 'efi-system-table':
> + Physical address of the system table. (required)

Need to mention the size of this address. Is it 64 bit or 32 bit? I
would follow the lead of 'linux,initrd-start' here and make the size
of property the size of the address. ie. If it is 8 bytes long, then
it is a 64 bit address.

> +- 'efi-runtime-mmap':
> + Physical address of an EFI memory map, containing at least
> + the regions to be preserved. (required)
> +- 'efi-runtime-mmap-size':
> + Size in bytes of the provided memory map. (required)

I would collapse the above two properties into a single property that
actually contains the memory map instead of pointing to it. You will
also need to specify the exact format of the data in this property.

> +- 'efi-mmap-desc-size':
> + Size of each descriptor in the memory map. (override default)
> +- 'efi-mmap-desc-ver':
> + Memory descriptor format version. (override default)

I don't understand how these properties will actually work. What
changes in the parsing if these properties are set?

> +
> +Since UEFI firmware on ARM systems are required to use a 1:1 memory map
> +even on LPAE-capable systems, the above fields are 32-bit regardless.
> +
> +It also depends on early_ioremap to parse the memory map and preserve
> +the regions required for runtime services.
> +
> +For actually enabling [U]EFI support, enable:
> +- CONFIG_EFI=y
> +- CONFIG_EFI_VARS=y or m
> +
> +After the kernel has mapped the required regions into its address space,
> +a SetVirtualAddressMap() call is made into UEFI in order to update
> +relocations. This call must be performed with all the code in a 1:1
> +mapping. This implementation achieves this by temporarily disabling the
> +MMU for the duration of this call. This can only be done safely:
> +- before secondary CPUs are brought online.
> +- after early_initcalls have completed, sinze it uses setup_mm_for_reboot().
> +
> +For verbose debug messages, specify 'uefi_debug' on the kernel command
> +line.

Looks good otherwise, Thanks!

g.

2013-06-26 13:20:46

by Grant Likely

[permalink] [raw]
Subject: Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services

On Wed, Jun 26, 2013 at 12:42 AM, Stephen Warren <[email protected]> wrote:
> On 06/25/2013 12:11 PM, Leif Lindholm wrote:
>> This patch provides documentation of the [U]EFI runtime services and
>> configuration features.
>
>
>> diff --git a/Documentation/arm/uefi.txt b/Documentation/arm/uefi.txt
>
>> +The implementation depends on receiving pointers to the UEFI memory map
>> +and System Table in a Flattened Device Tree - so is only available with
>> +CONFIG_OF.
>> +
>> +It (early) parses the FDT for the following parameters:
>
> Part of this document (the raw requirements for DT content, rather than
> the discussion of OS implementation/behaviour in parsing/interpreting
> the properties) should be part of a file in
> Documentation/devicetree/bindings/ (arm/uefi.txt?).
>
> What node are these properties expected to be contained within?
>
> Shouldn't that node be required to contain a compatible value, which
> would define the schema for the other properties?

Typically, a compatible property isn't only used for nodes that
represent a device or a so-called 'virtual' device (ie. such as to
describe how all the sound complex is wired together) since that
should be the clue to an OS that a device driver will bind against the
node. I think these properties can be dropped into /chosen without
defining a new compatible value.

>> +- 'efi-system-table':
>> + Physical address of the system table. (required)
>> +- 'efi-runtime-mmap':
>> + Physical address of an EFI memory map, containing at least
>> + the regions to be preserved. (required)
>> +- 'efi-runtime-mmap-size':
>> + Size in bytes of the provided memory map. (required)
>> +- 'efi-mmap-desc-size':
>> + Size of each descriptor in the memory map. (override default)
>> +- 'efi-mmap-desc-ver':
>> + Memory descriptor format version. (override default)
>> +
>> +Since UEFI firmware on ARM systems are required to use a 1:1 memory map
>> +even on LPAE-capable systems, the above fields are 32-bit regardless.
>
> What about ARMv8? Is the intention to have a separate definition for the
> UEFI bindings on ARMv8, so that compatibility isn't an issue? What if a
> future version of UEFI allows LPAE usage?

It is unlikely that will happen on v7 since newer versions of UEFI are
expected to remain backwards compatible with the current spec.

> It may be better to explicitly state that the size of those properties
> is either #address-cells from the parent node (presumably the top-level
> of the DT), and/or introduce some property to explicitly state the size
> of the properties. Those mechanisms would allow forward-compatibility to
> LPAE usage or ARMv8 without requiring the text of the binding definition
> to change.
>
> Also, it seems legal to state the physical addresses using 64-bits even
> if the actual values themselves are restricted to 32-bit range by the
> UEFI spec. Illegal values would presumably cause SW that interprets them
> to fail error-checks, and be rejected.
>
>> +After the kernel has mapped the required regions into its address space,
>> +a SetVirtualAddressMap() call is made into UEFI in order to update
>> +relocations. This call must be performed with all the code in a 1:1
>
> Presumably "all the code" also includes "all .data and .bss", or
> whatever the UEFI-equivalent may be? I'm not familiar with UEFI at all;
> does the "EFI memory map" mentioned above describe all the memory
> regions that must be mapped to use UEFI?

yes. Actually, there is an API for retrieving the memory map from UEFI
at runtime, but it is difficult to call from within the kernel.
Actually, my preference would be to not require GRUB or the Linux
Loader to add the above properties at all and instead have the kernel
proper retrieve the memory map directly. That would reduce the
dependency on GRUB/LinuxLoader doing things correctly, but Leif knows
better how feasible that would be.

>
>> +mapping. This implementation achieves this by temporarily disabling the
>> +MMU for the duration of this call. This can only be done safely:
>> +- before secondary CPUs are brought online.
>> +- after early_initcalls have completed, sinze it uses setup_mm_for_reboot().
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

2013-06-26 13:24:34

by Grant Likely

[permalink] [raw]
Subject: Re: [PATCH 4/4] init: efi: arm: enable (U)EFI runtime services on arm

On Tue, Jun 25, 2013 at 7:11 PM, Leif Lindholm <[email protected]> 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 <[email protected]>
> ---
> init/main.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/init/main.c b/init/main.c
> index 9484f4b..c61706e 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -872,6 +872,12 @@ static noinline void __init kernel_init_freeable(void)
> smp_prepare_cpus(setup_max_cpus);
>
> do_pre_smp_initcalls();
> +
> +#ifdef CONFIG_ARM
> + if (efi_enabled(EFI_RUNTIME_SERVICES))
> + efi_enter_virtual_mode();
> +#endif

Nitpick: Alternately you could use:

if (IS_ENABLED(CONFIG_ARM) && efi_enabled(EFI_RUNTIME_SERVICES))
efi_enter_virtual_mode();

That would get away from the #ifdef

g.

2013-06-26 13:30:56

by Grant Likely

[permalink] [raw]
Subject: Re: [PATCH 2/4] x86: efi: break efi_lookup_mapped_addr out to generic code

On Tue, Jun 25, 2013 at 7:11 PM, Leif Lindholm <[email protected]> wrote:
> efi_lookup_mapped_addr is a handy helper function for translating
> a physical address to the corresponding virtual one by scanning
> through memmap.map.
>
> This patch breaks it out into a new file for use elsewhere.
>
> Signed-off-by: Leif Lindholm <[email protected]>
> ---
> arch/x86/platform/efi/efi.c | 28 ----------------------------
> drivers/firmware/efi/Makefile | 2 +-
> drivers/firmware/efi/efi-helper.c | 33 +++++++++++++++++++++++++++++++++
> 3 files changed, 34 insertions(+), 29 deletions(-)
> create mode 100644 drivers/firmware/efi/efi-helper.c

I /think/ you should be able to put this directly into
drivers/firmware/efi/efi.c

> diff --git a/drivers/firmware/efi/efi-helper.c b/drivers/firmware/efi/efi-helper.c
> new file mode 100644
> index 0000000..c5c2c72
> --- /dev/null
> +++ b/drivers/firmware/efi/efi-helper.c
> @@ -0,0 +1,33 @@
> +/*
> + * Common [U]EFI support helper functions across architectures.
> + */
> +
> +#include <linux/efi.h>
> +
> +/*
> + * We can't ioremap data in EFI boot services RAM, because we've already mapped
> + * it as RAM. So, look it up in the existing EFI memory map instead. Only
> + * callable after efi_enter_virtual_mode and before efi_free_boot_services.
> + */
> +void __iomem *efi_lookup_mapped_addr(u64 phys_addr)

Can be __init annotated. Although it's a good idea to do that in a
separate patch.

Otherwise looks good to me.

Reviewed-by: Grant Likely <[email protected]>

2013-06-26 13:32:23

by Matt Fleming

[permalink] [raw]
Subject: Re: [PATCH 2/4] x86: efi: break efi_lookup_mapped_addr out to generic code

On Tue, 25 Jun, at 07:11:01PM, Leif Lindholm wrote:
> efi_lookup_mapped_addr is a handy helper function for translating
> a physical address to the corresponding virtual one by scanning
> through memmap.map.
>
> This patch breaks it out into a new file for use elsewhere.
>
> Signed-off-by: Leif Lindholm <[email protected]>
> ---
> arch/x86/platform/efi/efi.c | 28 ----------------------------
> drivers/firmware/efi/Makefile | 2 +-
> drivers/firmware/efi/efi-helper.c | 33 +++++++++++++++++++++++++++++++++
> 3 files changed, 34 insertions(+), 29 deletions(-)
> create mode 100644 drivers/firmware/efi/efi-helper.c

I'm not sure this function needs its own file. drivers/firmware/efi/efi.c
is a suitable place for this.

--
Matt Fleming, Intel Open Source Technology Center

2013-06-26 13:46:32

by Grant Likely

[permalink] [raw]
Subject: Re: [PATCH 3/4] arm: Add [U]EFI runtime services support

On Tue, Jun 25, 2013 at 7:11 PM, Leif Lindholm <[email protected]> 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 also locates any presented SMBIOS configuration table and stores it
> for potential later use by DMI.
>
> Signed-off-by: Leif Lindholm <[email protected]>
> ---
> arch/arm/Kconfig | 15 ++
> arch/arm/include/asm/efi.h | 22 +++
> arch/arm/kernel/Makefile | 2 +
> arch/arm/kernel/efi.c | 456 ++++++++++++++++++++++++++++++++++++++++++++
> arch/arm/kernel/efi_phys.S | 59 ++++++
> arch/arm/kernel/setup.c | 5 +
> 6 files changed, 559 insertions(+)
> create mode 100644 arch/arm/include/asm/efi.h
> create mode 100644 arch/arm/kernel/efi.c
> create mode 100644 arch/arm/kernel/efi_phys.S
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index bf8e55d..022d2eb 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1763,6 +1763,19 @@ config EARLY_IOREMAP
> Provides a mechanism for kernel initialisation code to temporarily
> map, in a highmem-agnostic way, memory pages in before paging_init().
>
> +config EFI
> +bool "UEFI runtime service support"
> + depends on OF
> + 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 should

Be confident! s/should/will/ :-)

> + continue to boot on existing non-UEFI platforms.

s/existing//

> +
> config SECCOMP
> bool
> prompt "Enable seccomp to safely compute untrusted bytecode"
> @@ -2217,6 +2230,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/efi.h b/arch/arm/include/asm/efi.h
> new file mode 100644
> index 0000000..aead94c
> --- /dev/null
> +++ b/arch/arm/include/asm/efi.h
> @@ -0,0 +1,22 @@
> +#ifndef _ASM_ARM_EFI_H
> +#define _ASM_ARM_EFI_H
> +
> +#include <asm/mach/map.h>
> +
> +extern int efi_memblock_arm_reserve_range(void);
> +
> +typedef efi_status_t efi_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 efi_phys_call(u32, u32, u32, efi_memory_desc_t *,
> + efi_set_virtual_address_map_t *);
> +
> +#define efi_remap(cookie, size) __arm_ioremap((cookie), (size), MT_MEMORY)
> +#define efi_ioremap(cookie, size) __arm_ioremap((cookie), (size), MT_DEVICE)
> +#define efi_unmap(cookie) __arm_iounmap((cookie))
> +#define efi_iounmap(cookie) __arm_iounmap((cookie))
> +
> +#endif /* _ASM_ARM_EFI_H */
> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> index 5f3338e..12b9c30 100644
> --- a/arch/arm/kernel/Makefile
> +++ b/arch/arm/kernel/Makefile
> @@ -84,4 +84,6 @@ obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
> obj-$(CONFIG_ARM_VIRT_EXT) += hyp-stub.o
> obj-$(CONFIG_ARM_PSCI) += psci.o
>
> +obj-$(CONFIG_EFI) += efi.o efi_phys.o
> +
> extra-y := $(head-y) vmlinux.lds
> diff --git a/arch/arm/kernel/efi.c b/arch/arm/kernel/efi.c
> new file mode 100644
> index 0000000..43ecf0b
> --- /dev/null
> +++ b/arch/arm/kernel/efi.c
> @@ -0,0 +1,456 @@
> +/*
> + * Extensible Firmware Interface
> + *
> + * Based on Extensible Firmware Interface Specification version 2.3.1
> + *
> + * Copyright (C) 2013 Linaro Ltd.

Was any of the above code extracted from the x86/ia64 efi code base?
If so then make sure the copyright header and license text gets
retained.

> + *
> + */
> +
> +#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/efi.h>
> +#include <asm/idmap.h>
> +#include <asm/tlbflush.h>
> +
> +struct efi efi;
> +EXPORT_SYMBOL(efi);
> +struct efi_memory_map memmap;

The above symbols should be pulled out of x86 and ia64 and made part
of drivers/efi/efi.c. Although in my quick look I don't see memmap
defined for ia64. You'll need to make sure that it actually exists
before moving it. That will also affect your earlier patch which moves
the memmap lookup function. I suspect that function won't build on
ia64.

> +
> +static efi_runtime_services_t *runtime;
> +
> +static phys_addr_t efi_system_table;
> +static phys_addr_t efi_boot_mmap;
> +static u32 efi_boot_mmap_size;
> +static u32 efi_mmap_desc_size;
> +static u32 efi_mmap_desc_ver;
> +
> +/* Default memory map descriptor information */
> +#define DESC_SIZE 48
> +#define DESC_VER 1
> +
> +/* If you're planning to wire up a debugger and debug the UEFI side ... */
> +#undef KEEP_ALL_REGIONS
> +#define KEEP_BOOT_SERVICES_REGIONS
> +
> +static int __initdata uefi_debug;
> +static int __init uefi_debug_setup(char *str)
> +{
> + uefi_debug = 1;
> +
> + return 0;
> +}
> +early_param("uefi_debug", uefi_debug_setup);
> +
> +static int __init fdt_find_efi_params(unsigned long node, const char *uname,
> + int depth, void *data)
> +{
> + unsigned long len;
> + __be32 *prop;
> +
> + if (depth != 1 ||
> + (strcmp(uname, "chosen") != 0 && strcmp(uname, "chosen@0") != 0))
> + return 0;
> +
> + pr_info("Getting EFI parameters from FDT.\n");
> +
> + prop = of_get_flat_dt_prop(node, "efi-system-table", &len);
> + if (!prop)
> + return 0;
> + efi_system_table = of_read_ulong(prop, len/4);
> +
> + prop = of_get_flat_dt_prop(node, "efi-runtime-mmap", &len);
> + if (!prop)
> + return 0;
> + efi_boot_mmap = of_read_ulong(prop, len/4);
> +
> + prop = of_get_flat_dt_prop(node, "efi-runtime-mmap-size", &len);
> + if (!prop)
> + return 0;
> + efi_boot_mmap_size = of_read_ulong(prop, len/4);
> +
> + prop = of_get_flat_dt_prop(node, "efi-mmap-desc-size", &len);
> + if (prop)
> + efi_mmap_desc_size = of_read_ulong(prop, len/4);
> + else
> + efi_mmap_desc_size = DESC_SIZE;
> +
> + prop = of_get_flat_dt_prop(node, "efi-mmap-desc-ver", &len);
> + if (prop)
> + efi_mmap_desc_ver = of_read_ulong(prop, len/4);
> + else
> + efi_mmap_desc_ver = DESC_VER;

As discussed in the binding patch, if the memmap is being passed in
from GRUB, then it can be embedded directly into the device tree which
will change the above code.

> +
> + if (uefi_debug) {
> + pr_info(" EFI system table @ 0x%08x\n",
> + (unsigned int) efi_system_table);
> + pr_info(" EFI mmap @ 0x%08x\n",
> + (unsigned int) efi_boot_mmap);
> + pr_info(" EFI mmap size = 0x%08x\n",
> + (unsigned int) efi_boot_mmap_size);
> + pr_info(" EFI mmap descriptor size = 0x%08x\n",
> + (unsigned int) efi_mmap_desc_size);
> + pr_info(" EFI mmap descriptor version = 0x%08x\n",
> + (unsigned int) efi_mmap_desc_ver);
> + }
> +
> + return 1;
> +}
> +
> +static int __init uefi_config_init(void)
> +{
> + efi_char16_t *c16;
> + char vendor[100] = "unknown";
> + efi_config_table_t *config_tables;
> + u32 nr_tables;
> + int i;
> +
> + efi.systab = early_ioremap(efi_system_table,
> + sizeof(efi_system_table_t));
> +
> + /*
> + * Verify the EFI Table
> + */
> + if (efi.systab == NULL)
> + panic("Whoa! Can't find EFI system table.\n");
> + if (efi.systab->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
> + panic("Whoa! EFI system table signature incorrect\n");
> + if ((efi.systab->hdr.revision >> 16) == 0)
> + pr_warn("Warning: EFI 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_ioremap(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("EFI v%u.%.02u by %s\n",
> + efi.systab->hdr.revision >> 16,
> + efi.systab->hdr.revision & 0xffff, vendor);
> +
> +
> + nr_tables = efi.systab->nr_tables;
> +
> + config_tables = early_ioremap(efi.systab->tables,
> + sizeof(efi_config_table_t) * nr_tables);
> +
> + for (i = 0; i < nr_tables; i++) {
> + efi_guid_t guid;
> + unsigned long table;
> + u8 str[38];
> +
> + guid = config_tables[i].guid;
> + table = config_tables[i].table;
> +
> + efi_guid_unparse(&guid, str);
> + if (uefi_debug)
> + pr_info(" Config Table: UUID=%s @ 0x%08x\n",
> + str, (u32)table);
> + if (!efi_guidcmp(guid, SMBIOS_TABLE_GUID))
> + efi.smbios = table;
> + }
> +
> + early_iounmap(config_tables, sizeof(efi_config_table_t) * nr_tables);
> + early_iounmap(c16, sizeof(vendor));
> + early_iounmap(efi.systab, sizeof(efi_system_table_t));
> +
> + return 0;
> +}
> +
> +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;
> + }
> +
> + return 1;
> +}
> +
> +static __initdata struct {
> + u32 type;
> + const char *name;
> +} memory_type_name_map[] = {
> + {EFI_RESERVED_TYPE, "EFI reserved"},
> + {EFI_LOADER_CODE, "EFI loader code"},
> + {EFI_LOADER_DATA, "EFI loader data"},
> + {EFI_BOOT_SERVICES_CODE, "EFI boot services code"},
> + {EFI_BOOT_SERVICES_DATA, "EFI boot services data"},
> + {EFI_RUNTIME_SERVICES_CODE, "EFI runtime services code"},
> + {EFI_RUNTIME_SERVICES_DATA, "EFI runtime services data"},
> + {EFI_CONVENTIONAL_MEMORY, "EFI conventional memory"},
> + {EFI_UNUSABLE_MEMORY, "EFI unusable memory"},
> + {EFI_ACPI_RECLAIM_MEMORY, "EFI ACPI reclaim memory"},
> + {EFI_ACPI_MEMORY_NVS, "EFI ACPI memory nvs"},
> + {EFI_MEMORY_MAPPED_IO, "EFI memory mapped I/O"},
> + {EFI_MEMORY_MAPPED_IO_PORT_SPACE, "EFI memory mapped I/O port space"},
> + {EFI_PAL_CODE, "EFI pal code"},
> + {EFI_MAX_MEMORY_TYPE, NULL},
> +};

The above *definitely* belongs 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;
> +
> + memmap.phys_map = early_ioremap(efi_boot_mmap, efi_boot_mmap_size);
> +
> + memmap.desc_size = efi_mmap_desc_size;
> + memmap.desc_version = efi_mmap_desc_ver;
> + memmap.map_end = (void *) memmap.phys_map + efi_boot_mmap_size;
> + memmap.nr_map = 0;
> +
> + if (uefi_debug)
> + pr_info("Processing EFI memory map:\n");
> +
> + for (p = memmap.phys_map; p < memmap.map_end; p += memmap.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++;
> + }
> +
> + if (uefi_debug)
> + pr_info("%d regions preserved.\n", memmap.nr_map);
> +
> + early_iounmap(memmap.phys_map, efi_boot_mmap_size);
> +
> + return 0;
> +}
> +
> +int __init efi_memblock_arm_reserve_range(void)
> +{
> + /* Grab system table location out of FDT (or ACPI) */
> + of_scan_flat_dt(fdt_find_efi_params, NULL);
> +
> + if (!efi_system_table || !efi_boot_mmap || !efi_boot_mmap_size)
> + return 0;
> +
> + remove_regions();
> +
> + uefi_config_init();
> +
> + 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();
> +}
> +
> +/*
> + * This function switches the EFI 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)
> +{
> + efi_phys_call_t *phys_set_map;
> + efi_status_t status;
> +
> + phys_call_prologue();
> +
> + phys_set_map = (void *)(unsigned long)virt_to_phys(efi_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;
> +}

Eventually we'll need to look at how this interacts with kexec. A
kexec'd kernel will need to use the mapping already chosen by a
previous kernel, but that's an issue for another patch series.

The patch needs some rework, but otherwise looks good, although I'm
concerned about duplicate code. However, I've not dug deep into the
mmu handling code. Others more cluefull that I on the MMU details will
need to weigh in on that.

g.

2013-06-26 13:46:45

by Grant Likely

[permalink] [raw]
Subject: Re: [PATCH 3/4] arm: Add [U]EFI runtime services support

On Tue, Jun 25, 2013 at 7:20 PM, Matthew Garrett <[email protected]> wrote:
> On Tue, Jun 25, 2013 at 07:11:02PM +0100, 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 also locates any presented SMBIOS configuration table and stores it
>> for potential later use by DMI.
>
> This appears to duplicate code that's already duplicated between x86 and
> ia64. We made a mistake there originally - let's not do it again. Having
> this code in three places is inevitably going to lead to skew and missed
> bugfixes.

+1

>
> --
> Matthew Garrett | [email protected]
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

2013-06-26 13:53:23

by Leif Lindholm

[permalink] [raw]
Subject: Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services

On Wed, Jun 26, 2013 at 02:20:23PM +0100, Grant Likely wrote:
> On Wed, Jun 26, 2013 at 12:42 AM, Stephen Warren <[email protected]> wrote:
> > the properties) should be part of a file in
> > Documentation/devicetree/bindings/ (arm/uefi.txt?).
> >
> > What node are these properties expected to be contained within?
> >
> > Shouldn't that node be required to contain a compatible value, which
> > would define the schema for the other properties?
>
> Typically, a compatible property isn't only used for nodes that
> represent a device or a so-called 'virtual' device (ie. such as to
> describe how all the sound complex is wired together) since that
> should be the clue to an OS that a device driver will bind against the
> node. I think these properties can be dropped into /chosen without
> defining a new compatible value.

That would be my preference.
But yes, that should be documented, and will be in the next version.

> >> +Since UEFI firmware on ARM systems are required to use a 1:1 memory map
> >> +even on LPAE-capable systems, the above fields are 32-bit regardless.
> >
> > What about ARMv8? Is the intention to have a separate definition for the
> > UEFI bindings on ARMv8, so that compatibility isn't an issue? What if a
> > future version of UEFI allows LPAE usage?
>
> It is unlikely that will happen on v7 since newer versions of UEFI are
> expected to remain backwards compatible with the current spec.

I'm going to go out on a limb here and claim that it wouldn't be possible
to do that compatibly. The current spec doesn't ban LPAE (or "use of long
descriptors"). But it does specify that all RAM known to UEFI must use a
1:1 mapping.

> >> +After the kernel has mapped the required regions into its address space,
> >> +a SetVirtualAddressMap() call is made into UEFI in order to update
> >> +relocations. This call must be performed with all the code in a 1:1
> >
> > Presumably "all the code" also includes "all .data and .bss", or
> > whatever the UEFI-equivalent may be? I'm not familiar with UEFI at all;
> > does the "EFI memory map" mentioned above describe all the memory
> > regions that must be mapped to use UEFI?
>
> yes.Actually, there is an API for retrieving the memory map from UEFI
> at runtime, but it is difficult to call from within the kernel.
> Actually, my preference would be to not require GRUB or the Linux
> Loader to add the above properties at all and instead have the kernel
> proper retrieve the memory map directly. That would reduce the
> dependency on GRUB/LinuxLoader doing things correctly, but Leif knows
> better how feasible that would be.

It's completely feasible, but we'd need to use a different method to do
the boot services call with a 1:1 mapping (idmap support is not available
until much later in the boot process).

The System Table pointer still needs to be passed across though.

/
Leif

2013-06-26 13:54:23

by Matt Fleming

[permalink] [raw]
Subject: Re: [PATCH 3/4] arm: Add [U]EFI runtime services support

On Wed, 26 Jun, at 02:46:09PM, Grant Likely wrote:
> Eventually we'll need to look at how this interacts with kexec. A
> kexec'd kernel will need to use the mapping already chosen by a
> previous kernel, but that's an issue for another patch series.

FYI, this is exactly what Borislav has been tackling on x86 recently. It
would be nice if we could find one scheme that suits everyone.

--
Matt Fleming, Intel Open Source Technology Center

2013-06-26 13:59:38

by Matt Fleming

[permalink] [raw]
Subject: Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services

On Wed, 26 Jun, at 03:53:11PM, Leif Lindholm wrote:
> It's completely feasible, but we'd need to use a different method to do
> the boot services call with a 1:1 mapping (idmap support is not available
> until much later in the boot process).

At least if you no longer relied upon the idmap we could potentially
have a single efi_enter_virtual_mode() call-site in init/main.c, which
would be nice.

--
Matt Fleming, Intel Open Source Technology Center

2013-06-26 14:05:10

by Leif Lindholm

[permalink] [raw]
Subject: Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services

On Wed, Jun 26, 2013 at 02:13:39PM +0100, Grant Likely wrote:
> > diff --git a/Documentation/arm/uefi.txt b/Documentation/arm/uefi.txt
> > +It (early) parses the FDT for the following parameters:
>
> Need to state which node these properties can be found in. I recommend /chosen

Will do.

> I would also prefix all of the following properties with "linux,"
> since they are very much about what the Linux kernel needs for
> booting. EFI stub will be creating these properties specifically for
> Linux, and the LinuxLoader should do the same (until we eventually
> kill it).

And that.

> > +- 'efi-system-table':
> > + Physical address of the system table. (required)
>
> Need to mention the size of this address. Is it 64 bit or 32 bit? I
> would follow the lead of 'linux,initrd-start' here and make the size
> of property the size of the address. ie. If it is 8 bytes long, then
> it is a 64 bit address.

Currently, it's a 4-byte address.
Although technically possible to be >32-bit in an LPAE system, the 1:1
mappign requirement of the UEFI spec forces it to reside in the lower
4GB on a 32-bit system.

> > +- 'efi-runtime-mmap':
> > + Physical address of an EFI memory map, containing at least
> > + the regions to be preserved. (required)
> > +- 'efi-runtime-mmap-size':
> > + Size in bytes of the provided memory map. (required)
>
> I would collapse the above two properties into a single property that
> actually contains the memory map instead of pointing to it. You will
> also need to specify the exact format of the data in this property.

Ok, that makes sense.

Hmm. The data is an array of struct EFI_MEMORY_DESCRIPTOR entries,
known in Linux as efi_memory_desc_t. Is that a good enough description?

> > +- 'efi-mmap-desc-size':
> > + Size of each descriptor in the memory map. (override default)
> > +- 'efi-mmap-desc-ver':
> > + Memory descriptor format version. (override default)
>
> I don't understand how these properties will actually work. What
> changes in the parsing if these properties are set?

I guess the intended use is that these options would permit you to
append new fields to the struct and have old code correctly parse the
array anyway.

/
Leif

2013-06-26 14:11:39

by Leif Lindholm

[permalink] [raw]
Subject: Re: [PATCH 2/4] x86: efi: break efi_lookup_mapped_addr out to generic code

On Wed, Jun 26, 2013 at 02:32:17PM +0100, Matt Fleming wrote:
> On Tue, 25 Jun, at 07:11:01PM, Leif Lindholm wrote:
> > efi_lookup_mapped_addr is a handy helper function for translating
> > a physical address to the corresponding virtual one by scanning
> > through memmap.map.
> >
> > This patch breaks it out into a new file for use elsewhere.
> >
> > Signed-off-by: Leif Lindholm <[email protected]>
> > ---
> > arch/x86/platform/efi/efi.c | 28 ----------------------------
> > drivers/firmware/efi/Makefile | 2 +-
> > drivers/firmware/efi/efi-helper.c | 33 +++++++++++++++++++++++++++++++++
> > 3 files changed, 34 insertions(+), 29 deletions(-)
> > create mode 100644 drivers/firmware/efi/efi-helper.c
>
> I'm not sure this function needs its own file. drivers/firmware/efi/efi.c
> is a suitable place for this.

Sure.
Would you be happy for me to start moving the other things mentioned
(config table parsing, common debug printout, global structs) in there too?

/
Leif

2013-06-26 14:15:34

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCH 3/4] arm: Add [U]EFI runtime services support

On Wed, Jun 26, 2013 at 02:54:17PM +0100, Matt Fleming wrote:
> On Wed, 26 Jun, at 02:46:09PM, Grant Likely wrote:
> > Eventually we'll need to look at how this interacts with kexec. A
> > kexec'd kernel will need to use the mapping already chosen by a
> > previous kernel, but that's an issue for another patch series.
>
> FYI, this is exactly what Borislav has been tackling on x86 recently. It
> would be nice if we could find one scheme that suits everyone.

Is this arm 32 or 64-bit? Because we haven't talked about 32-bit on x86
either. From skimming over the code, I'm not sure the same top-down
allocation and 1:1 mapping would work there. But I haven't looked hard
yet so I dunno.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

2013-06-26 14:22:48

by Leif Lindholm

[permalink] [raw]
Subject: Re: [PATCH 3/4] arm: Add [U]EFI runtime services support

On Wed, Jun 26, 2013 at 02:46:09PM +0100, Grant Likely 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 also locates any presented SMBIOS configuration table and stores it
> > for potential later use by DMI.
> >
> > Signed-off-by: Leif Lindholm <[email protected]>
> > ---
> > arch/arm/Kconfig | 15 ++
> > arch/arm/include/asm/efi.h | 22 +++
> > arch/arm/kernel/Makefile | 2 +
> > arch/arm/kernel/efi.c | 456 ++++++++++++++++++++++++++++++++++++++++++++
> > arch/arm/kernel/efi_phys.S | 59 ++++++
> > arch/arm/kernel/setup.c | 5 +
> > 6 files changed, 559 insertions(+)
> > create mode 100644 arch/arm/include/asm/efi.h
> > create mode 100644 arch/arm/kernel/efi.c
> > create mode 100644 arch/arm/kernel/efi_phys.S
> >
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index bf8e55d..022d2eb 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -1763,6 +1763,19 @@ config EARLY_IOREMAP
> > Provides a mechanism for kernel initialisation code to temporarily
> > map, in a highmem-agnostic way, memory pages in before paging_init().
> >
> > +config EFI
> > +bool "UEFI runtime service support"
> > + depends on OF
> > + 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 should
>
> Be confident! s/should/will/ :-)

Ok.

> > + continue to boot on existing non-UEFI platforms.
>
> s/existing//

Ok.

> > +
> > config SECCOMP
> > bool
> > prompt "Enable seccomp to safely compute untrusted bytecode"
> > @@ -2217,6 +2230,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/efi.h b/arch/arm/include/asm/efi.h
> > new file mode 100644
> > index 0000000..aead94c
> > --- /dev/null
> > +++ b/arch/arm/include/asm/efi.h
> > @@ -0,0 +1,22 @@
> > +#ifndef _ASM_ARM_EFI_H
> > +#define _ASM_ARM_EFI_H
> > +
> > +#include <asm/mach/map.h>
> > +
> > +extern int efi_memblock_arm_reserve_range(void);
> > +
> > +typedef efi_status_t efi_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 efi_phys_call(u32, u32, u32, efi_memory_desc_t *,
> > + efi_set_virtual_address_map_t *);
> > +
> > +#define efi_remap(cookie, size) __arm_ioremap((cookie), (size), MT_MEMORY)
> > +#define efi_ioremap(cookie, size) __arm_ioremap((cookie), (size), MT_DEVICE)
> > +#define efi_unmap(cookie) __arm_iounmap((cookie))
> > +#define efi_iounmap(cookie) __arm_iounmap((cookie))
> > +
> > +#endif /* _ASM_ARM_EFI_H */
> > diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> > index 5f3338e..12b9c30 100644
> > --- a/arch/arm/kernel/Makefile
> > +++ b/arch/arm/kernel/Makefile
> > @@ -84,4 +84,6 @@ obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
> > obj-$(CONFIG_ARM_VIRT_EXT) += hyp-stub.o
> > obj-$(CONFIG_ARM_PSCI) += psci.o
> >
> > +obj-$(CONFIG_EFI) += efi.o efi_phys.o
> > +
> > extra-y := $(head-y) vmlinux.lds
> > diff --git a/arch/arm/kernel/efi.c b/arch/arm/kernel/efi.c
> > new file mode 100644
> > index 0000000..43ecf0b
> > --- /dev/null
> > +++ b/arch/arm/kernel/efi.c
> > @@ -0,0 +1,456 @@
> > +/*
> > + * Extensible Firmware Interface
> > + *
> > + * Based on Extensible Firmware Interface Specification version 2.3.1
> > + *
> > + * Copyright (C) 2013 Linaro Ltd.
>
> Was any of the above code extracted from the x86/ia64 efi code base?
> If so then make sure the copyright header and license text gets
> retained.

Well, some of efi_config_init() was, but it looks like I will be movng
it out now.

> > + *
> > + */
> > +
> > +#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/efi.h>
> > +#include <asm/idmap.h>
> > +#include <asm/tlbflush.h>
> > +
> > +struct efi efi;
> > +EXPORT_SYMBOL(efi);
> > +struct efi_memory_map memmap;
>
> The above symbols should be pulled out of x86 and ia64 and made part
> of drivers/efi/efi.c. Although in my quick look I don't see memmap
> defined for ia64. You'll need to make sure that it actually exists
> before moving it.

Happy to do that. Can be ifdef'd if need be.

> That will also affect your earlier patch which moves
> the memmap lookup function. I suspect that function won't build on
> ia64.

Argh, indeed. I had that ifdef'd ARM/X86, but it seems to have fallen
out.

> > +
> > +static efi_runtime_services_t *runtime;
> > +
> > +static phys_addr_t efi_system_table;
> > +static phys_addr_t efi_boot_mmap;
> > +static u32 efi_boot_mmap_size;
> > +static u32 efi_mmap_desc_size;
> > +static u32 efi_mmap_desc_ver;
> > +
> > +/* Default memory map descriptor information */
> > +#define DESC_SIZE 48
> > +#define DESC_VER 1
> > +
> > +/* If you're planning to wire up a debugger and debug the UEFI side ... */
> > +#undef KEEP_ALL_REGIONS
> > +#define KEEP_BOOT_SERVICES_REGIONS
> > +
> > +static int __initdata uefi_debug;
> > +static int __init uefi_debug_setup(char *str)
> > +{
> > + uefi_debug = 1;
> > +
> > + return 0;
> > +}
> > +early_param("uefi_debug", uefi_debug_setup);
> > +
> > +static int __init fdt_find_efi_params(unsigned long node, const char *uname,
> > + int depth, void *data)
> > +{
> > + unsigned long len;
> > + __be32 *prop;
> > +
> > + if (depth != 1 ||
> > + (strcmp(uname, "chosen") != 0 && strcmp(uname, "chosen@0") != 0))
> > + return 0;
> > +
> > + pr_info("Getting EFI parameters from FDT.\n");
> > +
> > + prop = of_get_flat_dt_prop(node, "efi-system-table", &len);
> > + if (!prop)
> > + return 0;
> > + efi_system_table = of_read_ulong(prop, len/4);
> > +
> > + prop = of_get_flat_dt_prop(node, "efi-runtime-mmap", &len);
> > + if (!prop)
> > + return 0;
> > + efi_boot_mmap = of_read_ulong(prop, len/4);
> > +
> > + prop = of_get_flat_dt_prop(node, "efi-runtime-mmap-size", &len);
> > + if (!prop)
> > + return 0;
> > + efi_boot_mmap_size = of_read_ulong(prop, len/4);
> > +
> > + prop = of_get_flat_dt_prop(node, "efi-mmap-desc-size", &len);
> > + if (prop)
> > + efi_mmap_desc_size = of_read_ulong(prop, len/4);
> > + else
> > + efi_mmap_desc_size = DESC_SIZE;
> > +
> > + prop = of_get_flat_dt_prop(node, "efi-mmap-desc-ver", &len);
> > + if (prop)
> > + efi_mmap_desc_ver = of_read_ulong(prop, len/4);
> > + else
> > + efi_mmap_desc_ver = DESC_VER;
>
> As discussed in the binding patch, if the memmap is being passed in
> from GRUB, then it can be embedded directly into the device tree which
> will change the above code.

Yes.

> > +
> > + if (uefi_debug) {
> > + pr_info(" EFI system table @ 0x%08x\n",
> > + (unsigned int) efi_system_table);
> > + pr_info(" EFI mmap @ 0x%08x\n",
> > + (unsigned int) efi_boot_mmap);
> > + pr_info(" EFI mmap size = 0x%08x\n",
> > + (unsigned int) efi_boot_mmap_size);
> > + pr_info(" EFI mmap descriptor size = 0x%08x\n",
> > + (unsigned int) efi_mmap_desc_size);
> > + pr_info(" EFI mmap descriptor version = 0x%08x\n",
> > + (unsigned int) efi_mmap_desc_ver);
> > + }
> > +
> > + return 1;
> > +}
> > +
> > +static int __init uefi_config_init(void)
> > +{
> > + efi_char16_t *c16;
> > + char vendor[100] = "unknown";
> > + efi_config_table_t *config_tables;
> > + u32 nr_tables;
> > + int i;
> > +
> > + efi.systab = early_ioremap(efi_system_table,
> > + sizeof(efi_system_table_t));
> > +
> > + /*
> > + * Verify the EFI Table
> > + */
> > + if (efi.systab == NULL)
> > + panic("Whoa! Can't find EFI system table.\n");
> > + if (efi.systab->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
> > + panic("Whoa! EFI system table signature incorrect\n");
> > + if ((efi.systab->hdr.revision >> 16) == 0)
> > + pr_warn("Warning: EFI 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_ioremap(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("EFI v%u.%.02u by %s\n",
> > + efi.systab->hdr.revision >> 16,
> > + efi.systab->hdr.revision & 0xffff, vendor);
> > +
> > +
> > + nr_tables = efi.systab->nr_tables;
> > +
> > + config_tables = early_ioremap(efi.systab->tables,
> > + sizeof(efi_config_table_t) * nr_tables);
> > +
> > + for (i = 0; i < nr_tables; i++) {
> > + efi_guid_t guid;
> > + unsigned long table;
> > + u8 str[38];
> > +
> > + guid = config_tables[i].guid;
> > + table = config_tables[i].table;
> > +
> > + efi_guid_unparse(&guid, str);
> > + if (uefi_debug)
> > + pr_info(" Config Table: UUID=%s @ 0x%08x\n",
> > + str, (u32)table);
> > + if (!efi_guidcmp(guid, SMBIOS_TABLE_GUID))
> > + efi.smbios = table;
> > + }
> > +
> > + early_iounmap(config_tables, sizeof(efi_config_table_t) * nr_tables);
> > + early_iounmap(c16, sizeof(vendor));
> > + early_iounmap(efi.systab, sizeof(efi_system_table_t));
> > +
> > + return 0;
> > +}
> > +
> > +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;
> > + }
> > +
> > + return 1;
> > +}
> > +
> > +static __initdata struct {
> > + u32 type;
> > + const char *name;
> > +} memory_type_name_map[] = {
> > + {EFI_RESERVED_TYPE, "EFI reserved"},
> > + {EFI_LOADER_CODE, "EFI loader code"},
> > + {EFI_LOADER_DATA, "EFI loader data"},
> > + {EFI_BOOT_SERVICES_CODE, "EFI boot services code"},
> > + {EFI_BOOT_SERVICES_DATA, "EFI boot services data"},
> > + {EFI_RUNTIME_SERVICES_CODE, "EFI runtime services code"},
> > + {EFI_RUNTIME_SERVICES_DATA, "EFI runtime services data"},
> > + {EFI_CONVENTIONAL_MEMORY, "EFI conventional memory"},
> > + {EFI_UNUSABLE_MEMORY, "EFI unusable memory"},
> > + {EFI_ACPI_RECLAIM_MEMORY, "EFI ACPI reclaim memory"},
> > + {EFI_ACPI_MEMORY_NVS, "EFI ACPI memory nvs"},
> > + {EFI_MEMORY_MAPPED_IO, "EFI memory mapped I/O"},
> > + {EFI_MEMORY_MAPPED_IO_PORT_SPACE, "EFI memory mapped I/O port space"},
> > + {EFI_PAL_CODE, "EFI pal code"},
> > + {EFI_MAX_MEMORY_TYPE, NULL},
> > +};
>
> The above *definitely* belongs 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;
> > +
> > + memmap.phys_map = early_ioremap(efi_boot_mmap, efi_boot_mmap_size);
> > +
> > + memmap.desc_size = efi_mmap_desc_size;
> > + memmap.desc_version = efi_mmap_desc_ver;
> > + memmap.map_end = (void *) memmap.phys_map + efi_boot_mmap_size;
> > + memmap.nr_map = 0;
> > +
> > + if (uefi_debug)
> > + pr_info("Processing EFI memory map:\n");
> > +
> > + for (p = memmap.phys_map; p < memmap.map_end; p += memmap.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++;
> > + }
> > +
> > + if (uefi_debug)
> > + pr_info("%d regions preserved.\n", memmap.nr_map);
> > +
> > + early_iounmap(memmap.phys_map, efi_boot_mmap_size);
> > +
> > + return 0;
> > +}
> > +
> > +int __init efi_memblock_arm_reserve_range(void)
> > +{
> > + /* Grab system table location out of FDT (or ACPI) */
> > + of_scan_flat_dt(fdt_find_efi_params, NULL);
> > +
> > + if (!efi_system_table || !efi_boot_mmap || !efi_boot_mmap_size)
> > + return 0;
> > +
> > + remove_regions();
> > +
> > + uefi_config_init();
> > +
> > + 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();
> > +}
> > +
> > +/*
> > + * This function switches the EFI 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)
> > +{
> > + efi_phys_call_t *phys_set_map;
> > + efi_status_t status;
> > +
> > + phys_call_prologue();
> > +
> > + phys_set_map = (void *)(unsigned long)virt_to_phys(efi_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;
> > +}
>
> Eventually we'll need to look at how this interacts with kexec. A
> kexec'd kernel will need to use the mapping already chosen by a
> previous kernel, but that's an issue for another patch series.

Yes please :]

> The patch needs some rework, but otherwise looks good, although I'm
> concerned about duplicate code. However, I've not dug deep into the
> mmu handling code. Others more cluefull that I on the MMU details will
> need to weigh in on that.

Thanks.

/
Leif

2013-06-26 14:35:28

by Grant Likely

[permalink] [raw]
Subject: Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services

On Wed, Jun 26, 2013 at 3:04 PM, Leif Lindholm <[email protected]> wrote:
> On Wed, Jun 26, 2013 at 02:13:39PM +0100, Grant Likely wrote:
>> > +- 'efi-runtime-mmap':
>> > + Physical address of an EFI memory map, containing at least
>> > + the regions to be preserved. (required)
>> > +- 'efi-runtime-mmap-size':
>> > + Size in bytes of the provided memory map. (required)
>>
>> I would collapse the above two properties into a single property that
>> actually contains the memory map instead of pointing to it. You will
>> also need to specify the exact format of the data in this property.
>
> Ok, that makes sense.
>
> Hmm. The data is an array of struct EFI_MEMORY_DESCRIPTOR entries,
> known in Linux as efi_memory_desc_t. Is that a good enough description?

Yes, it is perfectly valid to point at another spec and state "it is
in that format". You'll also want to be specific that the data is
using the UEFI byte ordering, and not the ordering normally used by
FDT. One could argue that it should be 'translated' into a native DT
data format, but I think it is better to view it as a BLOB that DT
doesn't have anything to say about.

>> > +- 'efi-mmap-desc-size':
>> > + Size of each descriptor in the memory map. (override default)
>> > +- 'efi-mmap-desc-ver':
>> > + Memory descriptor format version. (override default)
>>
>> I don't understand how these properties will actually work. What
>> changes in the parsing if these properties are set?
>
> I guess the intended use is that these options would permit you to
> append new fields to the struct and have old code correctly parse the
> array anyway.

Let's leave them out as part of the binding until it is actually needed.

g.

2013-06-26 14:36:17

by Grant Likely

[permalink] [raw]
Subject: Re: [PATCH 3/4] arm: Add [U]EFI runtime services support

On Wed, Jun 26, 2013 at 3:15 PM, Borislav Petkov <[email protected]> wrote:
> On Wed, Jun 26, 2013 at 02:54:17PM +0100, Matt Fleming wrote:
>> On Wed, 26 Jun, at 02:46:09PM, Grant Likely wrote:
>> > Eventually we'll need to look at how this interacts with kexec. A
>> > kexec'd kernel will need to use the mapping already chosen by a
>> > previous kernel, but that's an issue for another patch series.
>>
>> FYI, this is exactly what Borislav has been tackling on x86 recently. It
>> would be nice if we could find one scheme that suits everyone.
>
> Is this arm 32 or 64-bit? Because we haven't talked about 32-bit on x86
> either. From skimming over the code, I'm not sure the same top-down
> allocation and 1:1 mapping would work there. But I haven't looked hard
> yet so I dunno.

This is arm 32. We'll be looking at arm 64 next.

g.

2013-06-26 14:38:26

by James Bottomley

[permalink] [raw]
Subject: Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services

On Wed, 2013-06-26 at 14:59 +0100, Matt Fleming wrote:
> On Wed, 26 Jun, at 03:53:11PM, Leif Lindholm wrote:
> > It's completely feasible, but we'd need to use a different method to do
> > the boot services call with a 1:1 mapping (idmap support is not available
> > until much later in the boot process).
>
> At least if you no longer relied upon the idmap we could potentially
> have a single efi_enter_virtual_mode() call-site in init/main.c, which
> would be nice.

The fixed virtual address scheme currently being looked at for x86_64 to
make SetVirtualAddressMap() kexec invariant doesn't work on 32 bit
because the address space isn't big enough. For ARM, given that we've
much more opportunity to work with the vendors, can we just avoid
transitioning to a virtual address map and always just install a
physical mapping before doing efi calls?

James

2013-06-26 14:40:49

by Matt Fleming

[permalink] [raw]
Subject: Re: [PATCH 2/4] x86: efi: break efi_lookup_mapped_addr out to generic code

On Wed, 26 Jun, at 04:11:30PM, Leif Lindholm wrote:
> Sure.
> Would you be happy for me to start moving the other things mentioned
> (config table parsing, common debug printout, global structs) in there too?

Super, super happy.

--
Matt Fleming, Intel Open Source Technology Center

2013-06-26 18:32:35

by Stephen Warren

[permalink] [raw]
Subject: Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services

On 06/26/2013 07:20 AM, Grant Likely wrote:
> On Wed, Jun 26, 2013 at 12:42 AM, Stephen Warren <[email protected]> wrote:
>> On 06/25/2013 12:11 PM, Leif Lindholm wrote:
>>> This patch provides documentation of the [U]EFI runtime services and
>>> configuration features.
>>
>>
>>> diff --git a/Documentation/arm/uefi.txt b/Documentation/arm/uefi.txt
>>
>>> +The implementation depends on receiving pointers to the UEFI memory map
>>> +and System Table in a Flattened Device Tree - so is only available with
>>> +CONFIG_OF.
>>> +
>>> +It (early) parses the FDT for the following parameters:
>>
>> Part of this document (the raw requirements for DT content, rather than
>> the discussion of OS implementation/behaviour in parsing/interpreting
>> the properties) should be part of a file in
>> Documentation/devicetree/bindings/ (arm/uefi.txt?).
>>
>> What node are these properties expected to be contained within?
>>
>> Shouldn't that node be required to contain a compatible value, which
>> would define the schema for the other properties?
>
> Typically, a compatible property isn't only used for nodes that
> represent a device or a so-called 'virtual' device (ie. such as to
> describe how all the sound complex is wired together) since that
> should be the clue to an OS that a device driver will bind against the
> node. I think these properties can be dropped into /chosen without
> defining a new compatible value.
>
>>> +- 'efi-system-table':
>>> + Physical address of the system table. (required)
>>> +- 'efi-runtime-mmap':
>>> + Physical address of an EFI memory map, containing at least
>>> + the regions to be preserved. (required)
>>> +- 'efi-runtime-mmap-size':
>>> + Size in bytes of the provided memory map. (required)
>>> +- 'efi-mmap-desc-size':
>>> + Size of each descriptor in the memory map. (override default)
>>> +- 'efi-mmap-desc-ver':
>>> + Memory descriptor format version. (override default)
>>> +
>>> +Since UEFI firmware on ARM systems are required to use a 1:1 memory map
>>> +even on LPAE-capable systems, the above fields are 32-bit regardless.
>>
>> What about ARMv8? Is the intention to have a separate definition for the
>> UEFI bindings on ARMv8, so that compatibility isn't an issue? What if a
>> future version of UEFI allows LPAE usage?
>
> It is unlikely that will happen on v7 since newer versions of UEFI are
> expected to remain backwards compatible with the current spec.

The expectation of backwards-compatibility sounds nice, but it seems a
little dangerous to outright rely on it.

Even if not a regular compatible property, can we define a property that
indicates the UEFI revision or revision of this DT binding, so that if
we ever have to change it, there is some way of explicitly indicating
which exact schema the DT corresponds to, rather than having to
reverse-engineer it from the set of properties that "just happen" to be
present in DT?

This is rather like the firmware node discussion that happened recently,
where we were expecting to represent a firmware (secure mode) interface
by a DT node, which would have a compatible value, which in turn would
convey information about which "OS" the secure firmware was running, and
well as any potential SoC-/OEM-/board-specific interface provided by it.

And who knows, what if UEFI gets replaced someday; presumably we'd want
some way of explicitly stating "running under UEFI" vs. "running under
something else"?

2013-06-26 19:32:08

by Leif Lindholm

[permalink] [raw]
Subject: Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services

On Wed, Jun 26, 2013 at 12:32:30PM -0600, Stephen Warren wrote:
> >> What about ARMv8? Is the intention to have a separate definition for the
> >> UEFI bindings on ARMv8, so that compatibility isn't an issue? What if a
> >> future version of UEFI allows LPAE usage?
> >
> > It is unlikely that will happen on v7 since newer versions of UEFI are
> > expected to remain backwards compatible with the current spec.
>
> The expectation of backwards-compatibility sounds nice, but it seems a
> little dangerous to outright rely on it.
>
> Even if not a regular compatible property, can we define a property that
> indicates the UEFI revision or revision of this DT binding, so that if
> we ever have to change it, there is some way of explicitly indicating
> which exact schema the DT corresponds to, rather than having to
> reverse-engineer it from the set of properties that "just happen" to be
> present in DT?
>
> This is rather like the firmware node discussion that happened recently,
> where we were expecting to represent a firmware (secure mode) interface
> by a DT node, which would have a compatible value, which in turn would
> convey information about which "OS" the secure firmware was running, and
> well as any potential SoC-/OEM-/board-specific interface provided by it.
>
> And who knows, what if UEFI gets replaced someday; presumably we'd want
> some way of explicitly stating "running under UEFI" vs. "running under
> something else"?

To me, these concerns are all covered by the existence of the
efi-system-table node, and the version number that you can extract
from the table (mandatory in any UEFI implementation) located at that
address. There is no reverse-engineering involved.

/
Leif

2013-06-27 01:32:44

by Matthew Garrett

[permalink] [raw]
Subject: Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services

On Wed, Jun 26, 2013 at 07:38:19AM -0700, James Bottomley wrote:
> The fixed virtual address scheme currently being looked at for x86_64 to
> make SetVirtualAddressMap() kexec invariant doesn't work on 32 bit
> because the address space isn't big enough. For ARM, given that we've
> much more opportunity to work with the vendors, can we just avoid
> transitioning to a virtual address map and always just install a
> physical mapping before doing efi calls?

We can probably get away with that now, but it does risk us ending up
with some firmware that expects to run in physical mode (boards designed
for Linux) and some firmware that expects to run in virtual mode (boards
designed for Windows). The degree of lockdown in the Windows ecosystem
at present means it's not a real problem at the moment, but if that ever
changes we're going to risk incompatibility.

--
Matthew Garrett | [email protected]

2013-06-27 06:23:47

by Grant Likely

[permalink] [raw]
Subject: Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services

On Thu, Jun 27, 2013 at 2:32 AM, Matthew Garrett <[email protected]> wrote:
> On Wed, Jun 26, 2013 at 07:38:19AM -0700, James Bottomley wrote:
>> The fixed virtual address scheme currently being looked at for x86_64 to
>> make SetVirtualAddressMap() kexec invariant doesn't work on 32 bit
>> because the address space isn't big enough. For ARM, given that we've
>> much more opportunity to work with the vendors, can we just avoid
>> transitioning to a virtual address map and always just install a
>> physical mapping before doing efi calls?
>
> We can probably get away with that now, but it does risk us ending up
> with some firmware that expects to run in physical mode (boards designed
> for Linux) and some firmware that expects to run in virtual mode (boards
> designed for Windows). The degree of lockdown in the Windows ecosystem
> at present means it's not a real problem at the moment, but if that ever
> changes we're going to risk incompatibility.

What is the problem trying to be avoided by not using the virtual map?
Is it passing the virtual mapping data from one kernel to the next
when kexecing? Or something else?

g.

2013-06-27 06:33:45

by James Bottomley

[permalink] [raw]
Subject: Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services

On Thu, 2013-06-27 at 07:23 +0100, Grant Likely wrote:
> On Thu, Jun 27, 2013 at 2:32 AM, Matthew Garrett <[email protected]> wrote:
> > On Wed, Jun 26, 2013 at 07:38:19AM -0700, James Bottomley wrote:
> >> The fixed virtual address scheme currently being looked at for x86_64 to
> >> make SetVirtualAddressMap() kexec invariant doesn't work on 32 bit
> >> because the address space isn't big enough. For ARM, given that we've
> >> much more opportunity to work with the vendors, can we just avoid
> >> transitioning to a virtual address map and always just install a
> >> physical mapping before doing efi calls?
> >
> > We can probably get away with that now, but it does risk us ending up
> > with some firmware that expects to run in physical mode (boards designed
> > for Linux) and some firmware that expects to run in virtual mode (boards
> > designed for Windows). The degree of lockdown in the Windows ecosystem
> > at present means it's not a real problem at the moment, but if that ever
> > changes we're going to risk incompatibility.
>
> What is the problem trying to be avoided by not using the virtual map?
> Is it passing the virtual mapping data from one kernel to the next
> when kexecing? Or something else?

Where to begin ... SetVirtualAddressMap() is one massive hack job ...
just look at the tiano core implementation. Basically it has a fixed
idea of where all the pointers are and it tries to convert them all to
the new address space. The problem we see in x86 is that this
conversion process isn't exhaustive due to implementation cockups, so
the post virtual address map image occasionally tries to access
unconverted pointers via the old physical address and oopses the kernel.

The problem for kexec is that SetVirtualAddressMap isn't idempotent. In
fact by API fiat it can only ever be called once for the entire lifetime
of the UEFI bios, which could be many kernels in a kexec situation. So,
somehow the subsequent kernels have to know not to call it, plus,
obviously, the virtual address map of the previous kernel has to work in
the next because it can't set up a new one.

James

2013-06-27 09:01:02

by Leif Lindholm

[permalink] [raw]
Subject: Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services

On Thu, Jun 27, 2013 at 02:32:19AM +0100, Matthew Garrett wrote:
> On Wed, Jun 26, 2013 at 07:38:19AM -0700, James Bottomley wrote:
> > The fixed virtual address scheme currently being looked at for x86_64 to
> > make SetVirtualAddressMap() kexec invariant doesn't work on 32 bit
> > because the address space isn't big enough. For ARM, given that we've
> > much more opportunity to work with the vendors, can we just avoid
> > transitioning to a virtual address map and always just install a
> > physical mapping before doing efi calls?
>
> We can probably get away with that now, but it does risk us ending up
> with some firmware that expects to run in physical mode (boards designed
> for Linux) and some firmware that expects to run in virtual mode (boards
> designed for Windows). The degree of lockdown in the Windows ecosystem
> at present means it's not a real problem at the moment, but if that ever
> changes we're going to risk incompatibility.

Is there anything preventing calling SetVirtualAddressMap() with a
1:1 map?

Or do you simply mean that some platforms might cruise along with
undetected bugs in their relocation hooks?

/
Leif

2013-06-27 14:23:35

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services

On Wednesday 26 June 2013, Grant Likely wrote:
> > index 0000000..5c48271
> > --- /dev/null
> > +++ b/Documentation/arm/uefi.txt
> > @@ -0,0 +1,39 @@
> > +The nomenclature EFI and UEFI are used interchangeably in this document.
> > +
> > +The implementation depends on receiving pointers to the UEFI memory map
> > +and System Table in a Flattened Device Tree - so is only available with
> > +CONFIG_OF.
> > +
> > +It (early) parses the FDT for the following parameters:
>
> Need to state which node these properties can be found in. I recommend /chosen
>
> I would also prefix all of the following properties with "linux,"
> since they are very much about what the Linux kernel needs for
> booting. EFI stub will be creating these properties specifically for
> Linux, and the LinuxLoader should do the same (until we eventually
> kill it).

Why not make it a separate /efi node instead, mirroring what we
have for hypervisor specific information and things like rtas?

Arnd

2013-06-27 14:37:30

by Matthew Garrett

[permalink] [raw]
Subject: Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services

On Wed, Jun 26, 2013 at 11:33:41PM -0700, James Bottomley wrote:
> On Thu, 2013-06-27 at 07:23 +0100, Grant Likely wrote:
> > What is the problem trying to be avoided by not using the virtual map?
> > Is it passing the virtual mapping data from one kernel to the next
> > when kexecing? Or something else?
>
> Where to begin ... SetVirtualAddressMap() is one massive hack job ...
> just look at the tiano core implementation. Basically it has a fixed
> idea of where all the pointers are and it tries to convert them all to
> the new address space. The problem we see in x86 is that this
> conversion process isn't exhaustive due to implementation cockups, so
> the post virtual address map image occasionally tries to access
> unconverted pointers via the old physical address and oopses the kernel.

And yet it's the only mode in which the firmrware is actually tested
against an OS, so we don't have any real choice in the matter.

--
Matthew Garrett | [email protected]

2013-06-27 14:38:10

by Matthew Garrett

[permalink] [raw]
Subject: Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services

On Thu, Jun 27, 2013 at 11:00:50AM +0200, Leif Lindholm wrote:
> On Thu, Jun 27, 2013 at 02:32:19AM +0100, Matthew Garrett wrote:
> > We can probably get away with that now, but it does risk us ending up
> > with some firmware that expects to run in physical mode (boards designed
> > for Linux) and some firmware that expects to run in virtual mode (boards
> > designed for Windows). The degree of lockdown in the Windows ecosystem
> > at present means it's not a real problem at the moment, but if that ever
> > changes we're going to risk incompatibility.
>
> Is there anything preventing calling SetVirtualAddressMap() with a
> 1:1 map?

No, but we've seen bugs as a result on some x86 systems. As far as the
spec, though, you're fine.

--
Matthew Garrett | [email protected]

2013-06-27 14:54:55

by Grant Likely

[permalink] [raw]
Subject: Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services

On Thu, Jun 27, 2013 at 7:33 AM, James Bottomley
<[email protected]> wrote:
> On Thu, 2013-06-27 at 07:23 +0100, Grant Likely wrote:
>> On Thu, Jun 27, 2013 at 2:32 AM, Matthew Garrett <[email protected]> wrote:
>> > On Wed, Jun 26, 2013 at 07:38:19AM -0700, James Bottomley wrote:
>> >> The fixed virtual address scheme currently being looked at for x86_64 to
>> >> make SetVirtualAddressMap() kexec invariant doesn't work on 32 bit
>> >> because the address space isn't big enough. For ARM, given that we've
>> >> much more opportunity to work with the vendors, can we just avoid
>> >> transitioning to a virtual address map and always just install a
>> >> physical mapping before doing efi calls?
>> >
>> > We can probably get away with that now, but it does risk us ending up
>> > with some firmware that expects to run in physical mode (boards designed
>> > for Linux) and some firmware that expects to run in virtual mode (boards
>> > designed for Windows). The degree of lockdown in the Windows ecosystem
>> > at present means it's not a real problem at the moment, but if that ever
>> > changes we're going to risk incompatibility.
>>
>> What is the problem trying to be avoided by not using the virtual map?
>> Is it passing the virtual mapping data from one kernel to the next
>> when kexecing? Or something else?
>
> Where to begin ... SetVirtualAddressMap() is one massive hack job ...
> just look at the tiano core implementation. Basically it has a fixed
> idea of where all the pointers are and it tries to convert them all to
> the new address space. The problem we see in x86 is that this
> conversion process isn't exhaustive due to implementation cockups, so
> the post virtual address map image occasionally tries to access
> unconverted pointers via the old physical address and oopses the kernel.

Would it be possible to run the UEFI hooks in some form of pseudo
userspace thread that protects against dereferencing addresses that
are no longer UEFI addresses?

> The problem for kexec is that SetVirtualAddressMap isn't idempotent. In
> fact by API fiat it can only ever be called once for the entire lifetime
> of the UEFI bios, which could be many kernels in a kexec situation. So,
> somehow the subsequent kernels have to know not to call it, plus,
> obviously, the virtual address map of the previous kernel has to work in
> the next because it can't set up a new one.

For this problem at least I think we've got a solution on ARM because
the virtual map can be passed across the kexec boundary via the device
tree. It will still (probably) need to be located in the ioremap
region and the size of the map will push down the maximum address for
ioremapping. The value of VMALLOC_END on arm 32bit is 0xff000000 and
that is a pretty stable number. As long as both the new and old
kernels have the same VMALLOC_END (very likely) then it should be okay
to pass the map over.

Let me know if I'm missing something important.

g.

2013-06-27 15:04:51

by James Bottomley

[permalink] [raw]
Subject: Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services

On Thu, 2013-06-27 at 15:54 +0100, Grant Likely wrote:
> On Thu, Jun 27, 2013 at 7:33 AM, James Bottomley
> <[email protected]> wrote:
> > On Thu, 2013-06-27 at 07:23 +0100, Grant Likely wrote:
> >> On Thu, Jun 27, 2013 at 2:32 AM, Matthew Garrett <[email protected]> wrote:
> >> > On Wed, Jun 26, 2013 at 07:38:19AM -0700, James Bottomley wrote:
> >> >> The fixed virtual address scheme currently being looked at for x86_64 to
> >> >> make SetVirtualAddressMap() kexec invariant doesn't work on 32 bit
> >> >> because the address space isn't big enough. For ARM, given that we've
> >> >> much more opportunity to work with the vendors, can we just avoid
> >> >> transitioning to a virtual address map and always just install a
> >> >> physical mapping before doing efi calls?
> >> >
> >> > We can probably get away with that now, but it does risk us ending up
> >> > with some firmware that expects to run in physical mode (boards designed
> >> > for Linux) and some firmware that expects to run in virtual mode (boards
> >> > designed for Windows). The degree of lockdown in the Windows ecosystem
> >> > at present means it's not a real problem at the moment, but if that ever
> >> > changes we're going to risk incompatibility.
> >>
> >> What is the problem trying to be avoided by not using the virtual map?
> >> Is it passing the virtual mapping data from one kernel to the next
> >> when kexecing? Or something else?
> >
> > Where to begin ... SetVirtualAddressMap() is one massive hack job ...
> > just look at the tiano core implementation. Basically it has a fixed
> > idea of where all the pointers are and it tries to convert them all to
> > the new address space. The problem we see in x86 is that this
> > conversion process isn't exhaustive due to implementation cockups, so
> > the post virtual address map image occasionally tries to access
> > unconverted pointers via the old physical address and oopses the kernel.
>
> Would it be possible to run the UEFI hooks in some form of pseudo
> userspace thread that protects against dereferencing addresses that
> are no longer UEFI addresses?

That's what the x86_64 proposal from Borislav Petkov does. We alter the
page tables before calling into the UEFI hooks to make sure both the
physical and virtual addresses work. Your problem on ARM with this
approach is that you're a VI platform, not a PI platform like intel, so
now you have to worry about inequivalent aliasing. I think you can
actually fix this by making sure you call SetVirtualAddressMap with a
1:1 offset mapping that's equivalent to the old physical addresses.

> > The problem for kexec is that SetVirtualAddressMap isn't idempotent. In
> > fact by API fiat it can only ever be called once for the entire lifetime
> > of the UEFI bios, which could be many kernels in a kexec situation. So,
> > somehow the subsequent kernels have to know not to call it, plus,
> > obviously, the virtual address map of the previous kernel has to work in
> > the next because it can't set up a new one.
>
> For this problem at least I think we've got a solution on ARM because
> the virtual map can be passed across the kexec boundary via the device
> tree. It will still (probably) need to be located in the ioremap
> region and the size of the map will push down the maximum address for
> ioremapping. The value of VMALLOC_END on arm 32bit is 0xff000000 and
> that is a pretty stable number. As long as both the new and old
> kernels have the same VMALLOC_END (very likely) then it should be okay
> to pass the map over.
>
> Let me know if I'm missing something important.

No, that works. We have to use a fixed address as an ABI on x86_64
because we don't have a data capsule that survives kexec.

James

2013-06-27 15:09:56

by James Bottomley

[permalink] [raw]
Subject: Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services

On Thu, 2013-06-27 at 15:37 +0100, Matthew Garrett wrote:
> On Wed, Jun 26, 2013 at 11:33:41PM -0700, James Bottomley wrote:
> > On Thu, 2013-06-27 at 07:23 +0100, Grant Likely wrote:
> > > What is the problem trying to be avoided by not using the virtual map?
> > > Is it passing the virtual mapping data from one kernel to the next
> > > when kexecing? Or something else?
> >
> > Where to begin ... SetVirtualAddressMap() is one massive hack job ...
> > just look at the tiano core implementation. Basically it has a fixed
> > idea of where all the pointers are and it tries to convert them all to
> > the new address space. The problem we see in x86 is that this
> > conversion process isn't exhaustive due to implementation cockups, so
> > the post virtual address map image occasionally tries to access
> > unconverted pointers via the old physical address and oopses the kernel.
>
> And yet it's the only mode in which the firmrware is actually tested
> against an OS, so we don't have any real choice in the matter.

Agree for x86 ... we just have to cope with the implementations we see
in the field. However, ARM has much more scope to have the UEFI
implementation developed collaboratively with Linux as the reference
platform. If we can convince the ARM implementors that
SetVirtualAddressMap is an accident waiting to happen, they might be
more flexible.

James

2013-06-27 15:37:38

by Grant Likely

[permalink] [raw]
Subject: Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services

On Thu, Jun 27, 2013 at 4:09 PM, James Bottomley
<[email protected]> wrote:
> On Thu, 2013-06-27 at 15:37 +0100, Matthew Garrett wrote:
>> On Wed, Jun 26, 2013 at 11:33:41PM -0700, James Bottomley wrote:
>> > On Thu, 2013-06-27 at 07:23 +0100, Grant Likely wrote:
>> > > What is the problem trying to be avoided by not using the virtual map?
>> > > Is it passing the virtual mapping data from one kernel to the next
>> > > when kexecing? Or something else?
>> >
>> > Where to begin ... SetVirtualAddressMap() is one massive hack job ...
>> > just look at the tiano core implementation. Basically it has a fixed
>> > idea of where all the pointers are and it tries to convert them all to
>> > the new address space. The problem we see in x86 is that this
>> > conversion process isn't exhaustive due to implementation cockups, so
>> > the post virtual address map image occasionally tries to access
>> > unconverted pointers via the old physical address and oopses the kernel.
>>
>> And yet it's the only mode in which the firmrware is actually tested
>> against an OS, so we don't have any real choice in the matter.
>
> Agree for x86 ... we just have to cope with the implementations we see
> in the field. However, ARM has much more scope to have the UEFI
> implementation developed collaboratively with Linux as the reference
> platform. If we can convince the ARM implementors that
> SetVirtualAddressMap is an accident waiting to happen, they might be
> more flexible.

We may not have any success convincing them of that; but given the
larger scope for Linux on ARM UEFI implementations, it will actually
get tested. If Linux chooses to use a 1:1 mapping, then the hardware
vendors will make sure a 1:1 mapping will actually work.

I must say that I'm a whole lot more comfortable with this approach.
I've never been comfortable with calling out to UEFI functions while
leaving the entirety of kernel space accessable. Sure UEFI can still
do nasty things if it really wants to and important devices get
mapped, but at least it will protect against accidental accesses.

g.

2013-06-27 17:28:29

by Matthew Garrett

[permalink] [raw]
Subject: Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services

On Thu, Jun 27, 2013 at 08:09:50AM -0700, James Bottomley wrote:
> On Thu, 2013-06-27 at 15:37 +0100, Matthew Garrett wrote:
> > And yet it's the only mode in which the firmrware is actually tested
> > against an OS, so we don't have any real choice in the matter.
>
> Agree for x86 ... we just have to cope with the implementations we see
> in the field. However, ARM has much more scope to have the UEFI
> implementation developed collaboratively with Linux as the reference
> platform. If we can convince the ARM implementors that
> SetVirtualAddressMap is an accident waiting to happen, they might be
> more flexible.

The majority of existing ARM UEFI implementations have only ever been
used to boot Windows, so like I said, this really isn't a safe
assumption.

--
Matthew Garrett | [email protected]

2013-06-27 18:10:18

by Stephen Warren

[permalink] [raw]
Subject: Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services

On 06/26/2013 01:31 PM, Leif Lindholm wrote:
> On Wed, Jun 26, 2013 at 12:32:30PM -0600, Stephen Warren wrote:
>>>> What about ARMv8? Is the intention to have a separate definition for the
>>>> UEFI bindings on ARMv8, so that compatibility isn't an issue? What if a
>>>> future version of UEFI allows LPAE usage?
>>>
>>> It is unlikely that will happen on v7 since newer versions of UEFI are
>>> expected to remain backwards compatible with the current spec.
>>
>> The expectation of backwards-compatibility sounds nice, but it seems a
>> little dangerous to outright rely on it.
>>
>> Even if not a regular compatible property, can we define a property that
>> indicates the UEFI revision or revision of this DT binding, so that if
>> we ever have to change it, there is some way of explicitly indicating
>> which exact schema the DT corresponds to, rather than having to
>> reverse-engineer it from the set of properties that "just happen" to be
>> present in DT?
>>
>> This is rather like the firmware node discussion that happened recently,
>> where we were expecting to represent a firmware (secure mode) interface
>> by a DT node, which would have a compatible value, which in turn would
>> convey information about which "OS" the secure firmware was running, and
>> well as any potential SoC-/OEM-/board-specific interface provided by it.
>>
>> And who knows, what if UEFI gets replaced someday; presumably we'd want
>> some way of explicitly stating "running under UEFI" vs. "running under
>> something else"?
>
> To me, these concerns are all covered by the existence of the
> efi-system-table node, and the version number that you can extract
> from the table (mandatory in any UEFI implementation) located at that
> address. There is no reverse-engineering involved.

So, what you're saying is that the existence (or lack thereof) of the
efi-system-table property is the indicator whether EFI is present? I
guess if we assume that EFI will always evolve at least compatibly
enough that the system table will always exist and be formatted
identically at least to the extent of allowing the EFI version number to
be parsed then that's workable. If that guarantee is broken, then we can
always define a different property that points at the new format of the
table.

This still seems a little implicit to me; having an explicit node with
an explicit compatible value is much more in line with what's been
discussed for other firmware interfaces.

However, I suppose it will work fine.

2013-06-27 18:33:19

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services

On 06/26/2013 07:38 AM, James Bottomley wrote:
> On Wed, 2013-06-26 at 14:59 +0100, Matt Fleming wrote:
>> On Wed, 26 Jun, at 03:53:11PM, Leif Lindholm wrote:
>>> It's completely feasible, but we'd need to use a different method to do
>>> the boot services call with a 1:1 mapping (idmap support is not available
>>> until much later in the boot process).
>>
>> At least if you no longer relied upon the idmap we could potentially
>> have a single efi_enter_virtual_mode() call-site in init/main.c, which
>> would be nice.
>
> The fixed virtual address scheme currently being looked at for x86_64 to
> make SetVirtualAddressMap() kexec invariant doesn't work on 32 bit
> because the address space isn't big enough. For ARM, given that we've
> much more opportunity to work with the vendors, can we just avoid
> transitioning to a virtual address map and always just install a
> physical mapping before doing efi calls?
>

What we could do on x86-32 is to map from 0xc0000000 downwards. It
wouldn't be invariant across kernel builds with different user/kernel
split... but I'm not sure we can win that one.

The other option is to say sod it and just use straight 1:1 mapping on
32 bits...

-hpa

2013-06-27 18:33:53

by Russell King - ARM Linux

[permalink] [raw]
Subject: Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services

On Thu, Jun 27, 2013 at 08:04:46AM -0700, James Bottomley wrote:
> That's what the x86_64 proposal from Borislav Petkov does. We alter the
> page tables before calling into the UEFI hooks to make sure both the
> physical and virtual addresses work. Your problem on ARM with this
> approach is that you're a VI platform, not a PI platform like intel, so

Let me correct that. Historically, ARM has had virtual indexed caches,
and some of its caches still are (eg, the instruction cache). Some
data caches might still be technically indexed by virtual address but
as the virtual index uses the address bits below the page size, it's
equivalent to a physically indexed cache.

>From ARMv7, practially all data caches now do not suffer from aliasing
with themselves. We do still suffer from data <-> instruction, and
cache <-> dma incoherence though.

2013-06-27 20:12:13

by Grant Likely

[permalink] [raw]
Subject: Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services

On Thu, Jun 27, 2013 at 7:04 PM, Stephen Warren <[email protected]> wrote:
> On 06/26/2013 01:31 PM, Leif Lindholm wrote:
>> On Wed, Jun 26, 2013 at 12:32:30PM -0600, Stephen Warren wrote:
>>>>> What about ARMv8? Is the intention to have a separate definition for the
>>>>> UEFI bindings on ARMv8, so that compatibility isn't an issue? What if a
>>>>> future version of UEFI allows LPAE usage?
>>>>
>>>> It is unlikely that will happen on v7 since newer versions of UEFI are
>>>> expected to remain backwards compatible with the current spec.
>>>
>>> The expectation of backwards-compatibility sounds nice, but it seems a
>>> little dangerous to outright rely on it.
>>>
>>> Even if not a regular compatible property, can we define a property that
>>> indicates the UEFI revision or revision of this DT binding, so that if
>>> we ever have to change it, there is some way of explicitly indicating
>>> which exact schema the DT corresponds to, rather than having to
>>> reverse-engineer it from the set of properties that "just happen" to be
>>> present in DT?
>>>
>>> This is rather like the firmware node discussion that happened recently,
>>> where we were expecting to represent a firmware (secure mode) interface
>>> by a DT node, which would have a compatible value, which in turn would
>>> convey information about which "OS" the secure firmware was running, and
>>> well as any potential SoC-/OEM-/board-specific interface provided by it.
>>>
>>> And who knows, what if UEFI gets replaced someday; presumably we'd want
>>> some way of explicitly stating "running under UEFI" vs. "running under
>>> something else"?
>>
>> To me, these concerns are all covered by the existence of the
>> efi-system-table node, and the version number that you can extract
>> from the table (mandatory in any UEFI implementation) located at that
>> address. There is no reverse-engineering involved.
>
> So, what you're saying is that the existence (or lack thereof) of the
> efi-system-table property is the indicator whether EFI is present? I
> guess if we assume that EFI will always evolve at least compatibly
> enough that the system table will always exist and be formatted
> identically at least to the extent of allowing the EFI version number to
> be parsed then that's workable. If that guarantee is broken, then we can
> always define a different property that points at the new format of the
> table.

Yes, that is what it means, and there is *immense* pressure from the
OEM market to not break that contract in a non-backwards-compatible
way.

g.

2013-06-30 03:21:38

by Rob Landley

[permalink] [raw]
Subject: Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services

On 06/25/2013 01:11:00 PM, Leif Lindholm wrote:
> This patch provides documentation of the [U]EFI runtime services and
> configuration features.
>
> Signed-off-by: Leif Lindholm <[email protected]>
> ---
> Documentation/arm/00-INDEX | 3 +++
> Documentation/arm/uefi.txt | 39
> +++++++++++++++++++++++++++++++++++++++
> 2 files changed, 42 insertions(+)
> create mode 100644 Documentation/arm/uefi.txt
>
> diff --git a/Documentation/arm/00-INDEX b/Documentation/arm/00-INDEX
> index 4978456..87e01d1 100644
> --- a/Documentation/arm/00-INDEX
> +++ b/Documentation/arm/00-INDEX
> @@ -36,3 +36,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..5c48271
> --- /dev/null
> +++ b/Documentation/arm/uefi.txt
> @@ -0,0 +1,39 @@
> +The nomenclature EFI and UEFI are used interchangeably in this
> document.
> +

And the acronym expands to...?

> +The implementation depends on receiving pointers to the UEFI memory
> map
> +and System Table in a Flattened Device Tree - so is only available
> with
> +CONFIG_OF.
> +
> +It (early) parses the FDT for the following parameters:
> +- 'efi-system-table':
> + Physical address of the system table. (required)
> +- 'efi-runtime-mmap':
> + Physical address of an EFI memory map, containing at least
> + the regions to be preserved. (required)
> +- 'efi-runtime-mmap-size':
> + Size in bytes of the provided memory map. (required)
> +- 'efi-mmap-desc-size':
> + Size of each descriptor in the memory map. (override default)
> +- 'efi-mmap-desc-ver':
> + Memory descriptor format version. (override default)
> +
> +Since UEFI firmware on ARM systems are required to use a 1:1 memory
> map
> +even on LPAE-capable systems, the above fields are 32-bit regardless.
> +
> +It also depends on early_ioremap to parse the memory map and preserve
> +the regions required for runtime services.
> +
> +For actually enabling [U]EFI support, enable:
> +- CONFIG_EFI=y
> +- CONFIG_EFI_VARS=y or m
> +
> +After the kernel has mapped the required regions into its address
> space,
> +a SetVirtualAddressMap() call is made into UEFI in order to update
> +relocations. This call must be performed with all the code in a 1:1
> +mapping. This implementation achieves this by temporarily disabling
> the
> +MMU for the duration of this call. This can only be done safely:
> +- before secondary CPUs are brought online.
> +- after early_initcalls have completed, sinze it uses
> setup_mm_for_reboot().

Typo: since

> +
> +For verbose debug messages, specify 'uefi_debug' on the kernel
> command
> +line.

At a wild guess, this parses a memory descriptor table to find out
where the physical and device memory on an arm board live, according to
some standard? I'm guessing because the file never quite says what this
mechanism is _for_, just describes knobs on a black box...

(I can google, but it's a touch newbie-unfriendly. I tend to prefer
documentation that's aimed at people who _don't_ already know whatever
it is. Personal idiosyncrasy.)

Acked-by: Rob Landley <[email protected]>

Rob-