Hi Leif,
On Thu, Nov 28, 2013 at 04:41:22PM +0000, Leif Lindholm wrote:
> This patch implements basic support for UEFI runtime services in the
> ARM architecture - a requirement for using efibootmgr to read and update
> the system boot configuration.
>
> It uses the generic configuration table scanning to populate ACPI and
> SMBIOS pointers.
I've got a bunch of implementation questions, so hopefully you can help me
to understand what this is doing!
> diff --git a/arch/arm/kernel/uefi.c b/arch/arm/kernel/uefi.c
> new file mode 100644
> index 0000000..f771026
> --- /dev/null
> +++ b/arch/arm/kernel/uefi.c
> @@ -0,0 +1,469 @@
> +/*
> + * Based on Unified Extensible Firmware Interface Specification version 2.3.1
> + *
> + * Copyright (C) 2013 Linaro Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/efi.h>
> +#include <linux/export.h>
> +#include <linux/memblock.h>
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +
> +#include <asm/cacheflush.h>
> +#include <asm/idmap.h>
> +#include <asm/tlbflush.h>
> +#include <asm/uefi.h>
> +
> +struct efi_memory_map memmap;
> +
> +static efi_runtime_services_t *runtime;
> +
> +static phys_addr_t uefi_system_table;
> +static phys_addr_t uefi_boot_mmap;
> +static u32 uefi_boot_mmap_size;
> +static u32 uefi_mmap_desc_size;
> +static u32 uefi_mmap_desc_ver;
> +
> +static unsigned long arm_uefi_facility;
> +
> +/*
> + * If you're planning to wire up a debugger and debug the UEFI side ...
> + */
> +#undef KEEP_ALL_REGIONS
> +
> +/*
> + * If you need to (temporarily) support buggy firmware.
> + */
> +#define KEEP_BOOT_SERVICES_REGIONS
> +
> +/*
> + * Returns 1 if 'facility' is enabled, 0 otherwise.
> + */
> +int efi_enabled(int facility)
> +{
> + return test_bit(facility, &arm_uefi_facility) != 0;
!test_bit(...) ?
> +static int __init uefi_init(void)
> +{
> + efi_char16_t *c16;
> + char vendor[100] = "unknown";
> + int i, retval;
> +
> + efi.systab = early_memremap(uefi_system_table,
> + sizeof(efi_system_table_t));
> +
> + /*
> + * Verify the UEFI System Table
> + */
> + if (efi.systab == NULL)
> + panic("Whoa! Can't find UEFI system table.\n");
> + if (efi.systab->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
> + panic("Whoa! UEFI system table signature incorrect\n");
> + if ((efi.systab->hdr.revision >> 16) == 0)
> + pr_warn("Warning: UEFI system table version %d.%02d, expected 1.00 or greater\n",
> + efi.systab->hdr.revision >> 16,
> + efi.systab->hdr.revision & 0xffff);
> +
> + /* Show what we know for posterity */
> + c16 = (efi_char16_t *)early_memremap(efi.systab->fw_vendor,
> + sizeof(vendor));
> + if (c16) {
> + for (i = 0; i < (int) sizeof(vendor) - 1 && *c16; ++i)
> + vendor[i] = c16[i];
> + vendor[i] = '\0';
> + }
> +
> + pr_info("UEFI v%u.%.02u by %s\n",
> + efi.systab->hdr.revision >> 16,
> + efi.systab->hdr.revision & 0xffff, vendor);
> +
> + retval = efi_config_init(NULL);
> + if (retval == 0)
> + set_bit(EFI_CONFIG_TABLES, &arm_uefi_facility);
Does this actually need to be atomic?
> +
> + early_iounmap(c16, sizeof(vendor));
> + early_iounmap(efi.systab, sizeof(efi_system_table_t));
> +
> + return retval;
> +}
> +
> +static __init int is_discardable_region(efi_memory_desc_t *md)
> +{
> +#ifdef KEEP_ALL_REGIONS
> + return 0;
> +#endif
Maybe just have a dummy is_discardable_region in this case, like we usually
do instead of inlining the #ifdef?
> + if (md->attribute & EFI_MEMORY_RUNTIME)
> + return 0;
> +
> + switch (md->type) {
> +#ifdef KEEP_BOOT_SERVICES_REGIONS
> + case EFI_BOOT_SERVICES_CODE:
> + case EFI_BOOT_SERVICES_DATA:
> +#endif
> + /* Keep tables around for any future kexec operations */
> + case EFI_ACPI_RECLAIM_MEMORY:
> + return 0;
> + /* Preserve */
> + case EFI_RESERVED_TYPE:
> + return 0;
> + }
> +
> + return 1;
> +}
[...]
> +int __init uefi_memblock_arm_reserve_range(void)
> +{
> + if (!of_scan_flat_dt(fdt_find_uefi_params, NULL))
> + return 0;
> +
> + set_bit(EFI_BOOT, &arm_uefi_facility);
Similar comment wrt atomicity here (and in the rest of this patch).
> + uefi_init();
> +
> + remove_regions();
> +
> + return 0;
> +}
> +
> +/*
> + * Disable instrrupts, enable idmap and disable caches.
interrupts
> + */
> +static void __init phys_call_prologue(void)
> +{
> + local_irq_disable();
> +
> + /* Take out a flat memory mapping. */
> + setup_mm_for_reboot();
> +
> + /* Clean and invalidate caches */
> + flush_cache_all();
> +
> + /* Turn off caching */
> + cpu_proc_fin();
> +
> + /* Push out any further dirty data, and ensure cache is empty */
> + flush_cache_all();
Do we care about outer caches here? This all looks suspiciously like
copy-paste from __soft_restart; perhaps some refactoring/reuse is in order?
> +}
> +
> +/*
> + * Restore original memory map and re-enable interrupts.
> + */
> +static void __init phys_call_epilogue(void)
> +{
> + static struct mm_struct *mm = &init_mm;
> +
> + /* Restore original memory mapping */
> + cpu_switch_mm(mm->pgd, mm);
> +
> + /* Flush branch predictor and TLBs */
> + local_flush_bp_all();
> +#ifdef CONFIG_CPU_HAS_ASID
> + local_flush_tlb_all();
> +#endif
... and this looks like copy-paste from setup_mm_for_reboot.
> + local_irq_enable();
> +}
> +
> +static int __init remap_region(efi_memory_desc_t *md, efi_memory_desc_t *entry)
> +{
> + u64 va;
> + u64 paddr;
> + u64 size;
> +
> + *entry = *md;
> + paddr = entry->phys_addr;
> + size = entry->num_pages << EFI_PAGE_SHIFT;
> +
> + /*
> + * Map everything writeback-capable as coherent memory,
> + * anything else as device.
> + */
> + if (md->attribute & EFI_MEMORY_WB)
> + va = (u64)((u32)uefi_remap(paddr, size) & 0xffffffffUL);
> + else
> + va = (u64)((u32)uefi_ioremap(paddr, size) & 0xffffffffUL);
Do you really need all those casts/masking?
> + if (!va)
> + return 0;
> + entry->virt_addr = va;
> +
> + if (uefi_debug)
> + pr_info(" %016llx-%016llx => 0x%08x : (%s)\n",
> + paddr, paddr + size - 1, (u32)va,
> + md->attribute & EFI_MEMORY_WB ? "WB" : "I/O");
> +
> + return 1;
> +}
> +
> +static int __init remap_regions(void)
> +{
> + void *p, *next;
> + efi_memory_desc_t *md;
> +
> + memmap.phys_map = uefi_remap(uefi_boot_mmap, uefi_boot_mmap_size);
> + if (!memmap.phys_map)
> + return 0;
> + memmap.map_end = (void *)memmap.phys_map + uefi_boot_mmap_size;
> + memmap.desc_size = uefi_mmap_desc_size;
> + memmap.desc_version = uefi_mmap_desc_ver;
> +
> + /* Allocate space for the physical region map */
> + memmap.map = kzalloc(memmap.nr_map * memmap.desc_size, GFP_KERNEL);
> + if (!memmap.map)
> + return 0;
> +
> + next = memmap.map;
> + for (p = memmap.phys_map; p < memmap.map_end; p += memmap.desc_size) {
> + md = p;
> + if (is_discardable_region(md) || md->type == EFI_RESERVED_TYPE)
> + continue;
> +
> + if (!remap_region(p, next))
> + return 0;
What about the kzalloc above, does that need freeing?
> +
> + next += memmap.desc_size;
> + }
> +
> + memmap.map_end = next;
> + efi.memmap = &memmap;
> +
> + uefi_unmap(memmap.phys_map);
> + memmap.phys_map = efi_lookup_mapped_addr(uefi_boot_mmap);
> + efi.systab = efi_lookup_mapped_addr(uefi_system_table);
> + if (efi.systab)
> + set_bit(EFI_SYSTEM_TABLES, &arm_uefi_facility);
> + /*
> + * efi.systab->runtime is a 32-bit pointer to something guaranteed by
> + * the UEFI specification to be 1:1 mapped in a 4GB address space.
> + */
> + runtime = efi_lookup_mapped_addr((u32)efi.systab->runtime);
> +
> + return 1;
> +}
> +
> +
> +/*
> + * This function switches the UEFI runtime services to virtual mode.
> + * This operation must be performed only once in the system's lifetime,
> + * including any kecec calls.
> + *
> + * This must be done with a 1:1 mapping. The current implementation
> + * resolves this by disabling the MMU.
> + */
> +efi_status_t __init phys_set_virtual_address_map(u32 memory_map_size,
> + u32 descriptor_size,
> + u32 descriptor_version,
> + efi_memory_desc_t *dsc)
> +{
> + uefi_phys_call_t *phys_set_map;
> + efi_status_t status;
> +
> + phys_call_prologue();
> +
> + phys_set_map = (void *)(unsigned long)virt_to_phys(uefi_phys_call);
> +
> + /* Called with caches disabled, returns with caches enabled */
> + status = phys_set_map(memory_map_size, descriptor_size,
> + descriptor_version, dsc,
> + efi.set_virtual_address_map)
> +;
Guessing this relies on a physically-tagged cache? Wouldn't hurt to put
something in the epilogue code to deal with vivt caches if they're not
prohibited by construction (e.g. due to some build dependency magic).
> + phys_call_epilogue();
> +
> + return status;
> +}
> +
> +/*
> + * Called explicitly from init/mm.c
> + */
> +void __init efi_enter_virtual_mode(void)
> +{
> + efi_status_t status;
> +
> + if (!efi_enabled(EFI_BOOT)) {
> + pr_info("UEFI services will not be available.\n");
> + return;
> + }
> +
> + pr_info("Remapping and enabling UEFI services.\n");
> +
> + /* Map the regions we memblock_remove:d earlier into kernel
> + address space */
> + if (!remap_regions()) {
> + pr_info("Failed to remap UEFI regions - runtime services will not be available.\n");
> + return;
> + }
> +
> + /* Call SetVirtualAddressMap with the physical address of the map */
> + efi.set_virtual_address_map =
> + (efi_set_virtual_address_map_t *)
> + runtime->set_virtual_address_map;
> + memmap.phys_map =
> + (efi_memory_desc_t *)(u32) __virt_to_phys((u32)memmap.map);
> +
> + status = phys_set_virtual_address_map(memmap.nr_map * memmap.desc_size,
> + memmap.desc_size,
> + memmap.desc_version,
> + memmap.phys_map);
> +
> + if (status != EFI_SUCCESS) {
> + pr_info("Failed to set UEFI virtual address map!\n");
> + return;
> + }
> +
> + /* Set up function pointers for efivars */
> + efi.get_variable = (efi_get_variable_t *)runtime->get_variable;
> + efi.get_next_variable =
> + (efi_get_next_variable_t *)runtime->get_next_variable;
> + efi.set_variable = (efi_set_variable_t *)runtime->set_variable;
> + set_bit(EFI_RUNTIME_SERVICES, &arm_uefi_facility);
> +}
> diff --git a/arch/arm/kernel/uefi_phys.S b/arch/arm/kernel/uefi_phys.S
> new file mode 100644
> index 0000000..e9a1542
> --- /dev/null
> +++ b/arch/arm/kernel/uefi_phys.S
> @@ -0,0 +1,59 @@
> +/*
> + * arch/arm/kernel/uefi_phys.S
> + *
> + * Copyright (C) 2013 Linaro Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/linkage.h>
> +#define PAR_MASK 0xfff
> +
> + .text
> +@ uefi_phys_call(a, b, c, d, *f)
> + .align 5
> + .pushsection .idmap.text, "ax"
> +ENTRY(uefi_phys_call)
> + @ Save physical context
> + mov r12, sp
> + push {r4-r5, r12, lr}
> +
> + @ Extract function pointer (don't write r12 beyond this)
> + ldr r12, [sp, #16]
> +
> + @ Convert sp to 32-bit physical
> + mov lr, sp
> + ldr r4, =PAR_MASK
> + and r5, lr, r4 @ Extract lower 12 bits of sp
> + mcr p15, 0, lr, c7, c8, 1 @ Write VA -> ATS1CPW
This is broken without an isb but, more to the point, why can't we just do
the standard lowmem virt_to_phys conversion here instead?
> + mrc p15, 0, lr, c7, c4, 0 @ Physical Address Register
> + mvn r4, r4
> + and lr, lr, r4 @ Clear lower 12 bits of PA
> + add lr, lr, r5 @ Calculate phys sp
> + mov sp, lr @ Update
> +
> + @ Disable MMU
> + mrc p15, 0, lr, c1, c0, 0 @ ctrl register
> + bic lr, lr, #0x1 @ ...............m
> + mcr p15, 0, lr, c1, c0, 0 @ disable MMU
> + isb
> +
> + @ Make call
> + blx r12
This is basically a copy-paste of cpu_v7_reset. Perhaps using some assembler
macros would help here?
> +
> + pop {r4-r5, r12, lr}
> +
> + @ Enable MMU + Caches
> + mrc p15, 0, r1, c1, c0, 0 @ ctrl register
> + orr r1, r1, #0x1000 @ ...i............
> + orr r1, r1, #0x0005 @ .............c.m
> + mcr p15, 0, r1, c1, c0, 0 @ enable MMU
> + isb
Why do we have to enable the caches so early?
Will
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
