Hi Leif,
Sorry it took so long to get back to you on this...
On Fri, Nov 29, 2013 at 05:43:04PM +0000, Leif Lindholm wrote:
- */
+static void __init phys_call_prologue(void) +{
local_irq_disable();/* Take out a flat memory mapping. */setup_mm_for_reboot();/* Clean and invalidate caches */flush_cache_all();/* Turn off caching */cpu_proc_fin();/* Push out any further dirty data, and ensure cache is empty */flush_cache_all();Do we care about outer caches here?
I think we do. We jump into UEFI and make it relocate itself to the new virtual addresses, with MMU disabled (so all accesses NC).
Ok, so that means you need some calls to the outer_* cache ops.
This all looks suspiciously like copy-paste from __soft_restart;
Yeah, 'cause you told me to :)
perhaps some refactoring/reuse is in order?
Could do. Turn this into a process.c:idcall_prepare(), or something less daftly named?
Sure, something like that (can't think of a good name either, right now...).
- Restore original memory map and re-enable interrupts.
- */
+static void __init phys_call_epilogue(void) +{
static struct mm_struct *mm = &init_mm;/* Restore original memory mapping */cpu_switch_mm(mm->pgd, mm);/* Flush branch predictor and TLBs */local_flush_bp_all();+#ifdef CONFIG_CPU_HAS_ASID
local_flush_tlb_all();+#endif
... and this looks like copy-paste from setup_mm_for_reboot.
You told me that too!
Hehe, I don't recall the conversation but I was probably talking in terms of `let's get something working, then tidy it up afterwards'.
Only this goes the other way.
Is the refactoring worth the extra code?
I think so. This code is pretty subtle, and I don't like it getting copied around, particularly if we have to fix issues in it later on.
local_irq_enable();+}
+static int __init remap_region(efi_memory_desc_t *md, efi_memory_desc_t *entry) +{
u64 va;u64 paddr;u64 size;*entry = *md;paddr = entry->phys_addr;size = entry->num_pages << EFI_PAGE_SHIFT;/** Map everything writeback-capable as coherent memory,* anything else as device.*/if (md->attribute & EFI_MEMORY_WB)va = (u64)((u32)uefi_remap(paddr, size) & 0xffffffffUL);elseva = (u64)((u32)uefi_ioremap(paddr, size) & 0xffffffffUL);Do you really need all those casts/masking?
I didn't find a better way to avoid warnings when building this code for both LPAE/non-LPAE.
Hmm, well phys_addr_t will be appropriately sized, if that helps you.
- This function switches the UEFI runtime services to virtual mode.
- This operation must be performed only once in the system's lifetime,
- including any kecec calls.
- This must be done with a 1:1 mapping. The current implementation
- resolves this by disabling the MMU.
- */
+efi_status_t __init phys_set_virtual_address_map(u32 memory_map_size,
u32 descriptor_size,u32 descriptor_version,efi_memory_desc_t *dsc)+{
uefi_phys_call_t *phys_set_map;efi_status_t status;phys_call_prologue();phys_set_map = (void *)(unsigned long)virt_to_phys(uefi_phys_call);/* Called with caches disabled, returns with caches enabled */status = phys_set_map(memory_map_size, descriptor_size,descriptor_version, dsc,efi.set_virtual_address_map)+;
Guessing this relies on a physically-tagged cache? Wouldn't hurt to put something in the epilogue code to deal with vivt caches if they're not prohibited by construction (e.g. due to some build dependency magic).
Sorry, I don't quite follow? How would it depend on a physically-tagged cache?
I was wondering if you could run into issues in the epilogue, since you've changed page tables without cleaning the cache, but actually cpu_switch_mm takes care of that.
phys_call_epilogue();return status;+}
diff --git a/arch/arm/kernel/uefi_phys.S b/arch/arm/kernel/uefi_phys.S new file mode 100644 index 0000000..e9a1542 --- /dev/null +++ b/arch/arm/kernel/uefi_phys.S @@ -0,0 +1,59 @@ +/*
- arch/arm/kernel/uefi_phys.S
- Copyright (C) 2013 Linaro Ltd.
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License version 2 as
- published by the Free Software Foundation.
- */
+#include <linux/linkage.h> +#define PAR_MASK 0xfff
.text+@ uefi_phys_call(a, b, c, d, *f)
.align 5.pushsection .idmap.text, "ax"+ENTRY(uefi_phys_call)
@ Save physical contextmov r12, sppush {r4-r5, r12, lr}@ Extract function pointer (don't write r12 beyond this)ldr r12, [sp, #16]@ Convert sp to 32-bit physicalmov lr, spldr r4, =PAR_MASKand r5, lr, r4 @ Extract lower 12 bits of spmcr p15, 0, lr, c7, c8, 1 @ Write VA -> ATS1CPWThis is broken without an isb but, more to the point, why can't we just do the standard lowmem virt_to_phys conversion here instead?
I can't use that from within asm (right?). Are you suggesting that I should duplicate the mechanism of virt_to_phys here?
I think you need an extra argument: either the physical SP, or the virt-> phys offset.
mrc p15, 0, lr, c7, c4, 0 @ Physical Address Registermvn r4, r4and lr, lr, r4 @ Clear lower 12 bits of PAadd lr, lr, r5 @ Calculate phys spmov sp, lr @ Update@ Disable MMUmrc p15, 0, lr, c1, c0, 0 @ ctrl registerbic lr, lr, #0x1 @ ...............mmcr p15, 0, lr, c1, c0, 0 @ disable MMUisb@ Make callblx r12This is basically a copy-paste of cpu_v7_reset. Perhaps using some assembler macros would help here?
Well, I explicitly don't want to touch SCTLR.TE. We could macro-ize it, but I think that would increase the amount of code.
How would using a macro increase the amount of code? Just make the macro take the bits to clear and the bits to set as arguments.
pop {r4-r5, r12, lr}@ Enable MMU + Cachesmrc p15, 0, r1, c1, c0, 0 @ ctrl registerorr r1, r1, #0x1000 @ ...i............orr r1, r1, #0x0005 @ .............c.mmcr p15, 0, r1, c1, c0, 0 @ enable MMUisbWhy do we have to enable the caches so early?
You'd prefer it being done back in phys_call_epilogue()?
Unless there's a good reason to move stuff into early assembly, it would be better off using functions/macros from C code imo.
Will