On Fri, Nov 30, 2012 at 5:58 AM, Will Deacon <will.dea...@arm.com> wrote:
> On Thu, Nov 29, 2012 at 06:59:07PM +0000, Christoffer Dall wrote:
>> On Mon, Nov 19, 2012 at 9:16 AM, Will Deacon <will.dea...@arm.com> wrote:
>> >
>> > I also think it might be cleaner to declare the hyp_pgd next to the
>> > idmap_pgd then add the hyp_idmap_setup code to init_static_idmap, so
>> > that we don't add new entry points to this file. The teardown can all be
>> > moved into the kvm/ code as it doesn't need any of the existing idmap
>> > functions.
>> >
>>
>> hmm, we had some attempts at this earlier, but it wasn't all that
>> nice. Allocating the hyp_pgd inside idmap.c requires some more logic,
>> and the #ifdefs inside init_static_idmap are also not pretty.
>
> [...]
>
>> I see how you would like to clean this up, but it's not really hard to
>> read or understand, imho:
>
> I have to disagree. As it stands, idmap.c has *no* entry points for
> manipulating the page tables, which is a good thing because it puts all the
> mapping code in one place and makes it both easy to use and easy to reason
> about. Your patch starts exposing a load of this stuff to the rest of the
> kernel, which is actively going against the design that we currently have.
>
having not been a part of that design discussion, I still don't quite
see the harm. On the contrary, now we get code in KVM that makes
assumptions about how things are mapped in idmap when unmapping. Also
it feels a bit weird that the highly kvm-specific hyp_pgd is now
exported everywhere and initialized in idmap.c, but nothing that I
can't live with.
In any case, you know much better how the idmap code is viewed, so
I'll try to follow your thoughts.
>
> Here is a quick, untested patch to fix that (modulo the ARM_VIRT_EXT implies
> ARM_LPAE change I suggested).
>
thanks, I tweaked it a bit to avoid the over 80-character lines and
renamed kvm_mmu_exit to kvm_clear_hyp_idmap, which is what it actually
does:
diff --git a/arch/arm/include/asm/idmap.h b/arch/arm/include/asm/idmap.h
index 36708ba..3e79d33 100644
--- a/arch/arm/include/asm/idmap.h
+++ b/arch/arm/include/asm/idmap.h
@@ -9,11 +9,10 @@
extern pgd_t *idmap_pgd;
-void setup_mm_for_reboot(void);
-
#ifdef CONFIG_ARM_VIRT_EXT
-void hyp_idmap_teardown(pgd_t *hyp_pgd);
-void hyp_idmap_setup(pgd_t *hyp_pgd);
+extern pgd_t *hyp_pgd;
#endif
+void setup_mm_for_reboot(void);
+
#endif /* __ASM_IDMAP_H */
diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
index 4d55218..c7f939d 100644
--- a/arch/arm/include/asm/kvm_mmu.h
+++ b/arch/arm/include/asm/kvm_mmu.h
@@ -34,5 +34,5 @@ void kvm_mmu_free_memory_caches(struct kvm_vcpu *vcpu);
unsigned long kvm_mmu_get_httbr(void);
int kvm_mmu_init(void);
-void kvm_mmu_exit(void);
+void kvm_clear_hyp_idmap(void);
#endif /* __ARM_KVM_MMU_H__ */
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index b93da23..5b86d27 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -1004,7 +1004,7 @@ static int init_hyp_mode(void)
/*
* Unmap the identity mapping
*/
- kvm_mmu_exit();
+ kvm_clear_hyp_idmap();
/*
* Map the Hyp-code called directly from the host
diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 59d422f..50ac252 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -33,8 +33,9 @@
#include "trace.h"
+extern char __hyp_idmap_text_start[], __hyp_idmap_text_end[];
+
static DEFINE_MUTEX(kvm_hyp_pgd_mutex);
-static pgd_t *hyp_pgd;
static void kvm_tlb_flush_vmid(struct kvm *kvm)
{
@@ -725,15 +726,34 @@ unsigned long kvm_mmu_get_httbr(void)
int kvm_mmu_init(void)
{
- hyp_pgd = kzalloc(PTRS_PER_PGD * sizeof(pgd_t), GFP_KERNEL);
- if (!hyp_pgd)
- return -ENOMEM;
-
- hyp_idmap_setup(hyp_pgd);
- return 0;
+ return hyp_pgd ? 0 : -ENOMEM;
}
-void kvm_mmu_exit(void)
+/**
+ * kvm_clear_idmap - remove all idmaps from the hyp pgd
+ *
+ * Free the underlying pmds for all pgds in range and clear the pgds (but
+ * don't free them) afterwards.
+ */
+void kvm_clear_hyp_idmap(void)
{
- hyp_idmap_teardown(hyp_pgd);
+ unsigned long addr, end;
+ unsigned long next;
+ pgd_t *pgd = hyp_pgd;
+
+ addr = virt_to_phys(__hyp_idmap_text_start);
+ end = virt_to_phys(__hyp_idmap_text_end);
+
+ pgd += pgd_index(addr);
+ do {
+ next = pgd_addr_end(addr, end);
+ if (pgd_none_or_clear_bad(pgd))
+ continue;
+ pud_t *pud = pud_offset(pgd, addr);
+ pmd_t *pmd = pmd_offset(pud, addr);
+
+ pud_clear(pud);
+ clean_pmd_entry(pmd);
+ pmd_free(NULL, (pmd_t *)((unsigned long)pmd & PAGE_MASK));
+ } while (pgd++, addr = next, addr < end);
}
diff --git a/arch/arm/mm/idmap.c b/arch/arm/mm/idmap.c
index ea7430e..c97612e 100644
--- a/arch/arm/mm/idmap.c
+++ b/arch/arm/mm/idmap.c
@@ -86,65 +86,43 @@ static void identity_mapping_add(pgd_t *pgd, const
char *text_start,
} while (pgd++, addr = next, addr != end);
}
-extern char __idmap_text_start[], __idmap_text_end[];
+#ifdef CONFIG_ARM_VIRT_EXT
+pgd_t *hyp_pgd;
-static int __init init_static_idmap(void)
+extern char __hyp_idmap_text_start[], __hyp_idmap_text_end[];
+
+static int __init init_static_idmap_hyp(void)
{
- idmap_pgd = pgd_alloc(&init_mm);
- if (!idmap_pgd)
+ hyp_pgd = kzalloc(PTRS_PER_PGD * sizeof(pgd_t), GFP_KERNEL);
+ if (!hyp_pgd)
return -ENOMEM;
- identity_mapping_add(idmap_pgd, __idmap_text_start,
- __idmap_text_end, 0);
+ identity_mapping_add(hyp_pgd, __hyp_idmap_text_start,
+ __hyp_idmap_text_end, PMD_SECT_AP1);
return 0;
}
-early_initcall(init_static_idmap);
-
-#if defined(CONFIG_ARM_VIRT_EXT) && defined(CONFIG_ARM_LPAE)
-static void hyp_idmap_del_pmd(pgd_t *pgd, unsigned long addr)
+#else
+static int __init init_static_idmap_hyp(void)
{
- pud_t *pud;
- pmd_t *pmd;
-
- pud = pud_offset(pgd, addr);
- pmd = pmd_offset(pud, addr);
- pud_clear(pud);
- clean_pmd_entry(pmd);
- pmd_free(NULL, (pmd_t *)((unsigned long)pmd & PAGE_MASK));
+ return 0;
}
+#endif
-extern char __hyp_idmap_text_start[], __hyp_idmap_text_end[];
+extern char __idmap_text_start[], __idmap_text_end[];
-/*
- * This version actually frees the underlying pmds for all pgds in range and
- * clear the pgds themselves afterwards.
- */
-void hyp_idmap_teardown(pgd_t *hyp_pgd)
+static int __init init_static_idmap(void)
{
- unsigned long addr, end;
- unsigned long next;
- pgd_t *pgd = hyp_pgd;
-
- addr = virt_to_phys(__hyp_idmap_text_start);
- end = virt_to_phys(__hyp_idmap_text_end);
+ idmap_pgd = pgd_alloc(&init_mm);
+ if (!idmap_pgd)
+ return -ENOMEM;
- pgd += pgd_index(addr);
- do {
- next = pgd_addr_end(addr, end);
- if (!pgd_none_or_clear_bad(pgd))
- hyp_idmap_del_pmd(pgd, addr);
- } while (pgd++, addr = next, addr < end);
-}
-EXPORT_SYMBOL_GPL(hyp_idmap_teardown);
+ identity_mapping_add(idmap_pgd, __idmap_text_start,
+ __idmap_text_end, 0);
-void hyp_idmap_setup(pgd_t *hyp_pgd)
-{
- identity_mapping_add(hyp_pgd, __hyp_idmap_text_start,
- __hyp_idmap_text_end, PMD_SECT_AP1);
+ return init_static_idmap_hyp();
}
-EXPORT_SYMBOL_GPL(hyp_idmap_setup);
-#endif
+early_initcall(init_static_idmap);
/*
* In order to soft-boot, we need to switch to a 1:1 mapping for the
--
-Christoffer
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
