From patchwork Fri Nov 30 16:29:27 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christoffer Dall X-Patchwork-Id: 1825721 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) by patchwork2.kernel.org (Postfix) with ESMTP id DC8EEDF24C for ; Fri, 30 Nov 2012 16:32:50 +0000 (UTC) Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1TeTT3-0002oL-VD; Fri, 30 Nov 2012 16:29:34 +0000 Received: from mail-vb0-f49.google.com ([209.85.212.49]) by merlin.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1TeTT0-0002o1-B2 for linux-arm-kernel@lists.infradead.org; Fri, 30 Nov 2012 16:29:31 +0000 Received: by mail-vb0-f49.google.com with SMTP id r6so11550267vbi.22 for ; Fri, 30 Nov 2012 08:29:28 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:x-originating-ip:in-reply-to:references:date :message-id:subject:from:to:cc:content-type:x-gm-message-state; bh=EbYr1ALrcQ/o4X5qyWLFU0b5LAqmAcMSo+xMyh3p9pU=; b=PLfvnPzgwlDd4FzTJ4bS8NxdfrhaoHkwfW4s/rKQcx9qJ03jN2uPEJsJ/Uy4R/71fh a1DPS+l5kiBlhU9/siKrYP83QjHLV31pv5WFljq+AKWHkf01tHcTnxr5EGnxzIQHaRun hAcgyb/Nv1Yy6UcUhHM/qoXDwWAhfuyPHScxW5084sHY6WNQtF/kLEjwt9Uk9OLr9n+Q Hjwkqp4a1vHvOSpmvQr1X4exnJ4vZxkfGWHWUojqzPP5OhFQ78pdSCDtDRBeK84CkQhh kTkfgiWafhEBe6R6WscKNn3iMcmbnyLqLkPDNwkyjGsnhcFy0MFf7dg7lpiTBYEhVrb2 oHZw== MIME-Version: 1.0 Received: by 10.58.2.71 with SMTP id 7mr1462855ves.42.1354292967856; Fri, 30 Nov 2012 08:29:27 -0800 (PST) Received: by 10.220.127.16 with HTTP; Fri, 30 Nov 2012 08:29:27 -0800 (PST) X-Originating-IP: [72.80.83.148] In-Reply-To: <20121130105843.GB26305@mudshark.cambridge.arm.com> References: <20121110154203.2836.46686.stgit@chazy-air> <20121110154224.2836.21775.stgit@chazy-air> <20121119141631.GU3205@mudshark.cambridge.arm.com> <20121130105843.GB26305@mudshark.cambridge.arm.com> Date: Fri, 30 Nov 2012 11:29:27 -0500 Message-ID: Subject: Re: [PATCH v4 02/14] ARM: Section based HYP idmap From: Christoffer Dall To: Will Deacon X-Gm-Message-State: ALoCoQkq3vhh0GjmN/Z5G6WGbSUr7NWHrvWsCtAntnU5vhMKM6RhjAxSwIINnftAY3mAaWcjeoH8 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20121130_112930_512672_36401383 X-CRM114-Status: GOOD ( 32.59 ) X-Spam-Score: -2.6 (--) X-Spam-Report: SpamAssassin version 3.3.2 on merlin.infradead.org summary: Content analysis details: (-2.6 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [209.85.212.49 listed in list.dnswl.org] -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] Cc: "dave.martin@linaro.org" , "kvm@vger.kernel.org" , Marc Zyngier , Marcelo Tosatti , "kvmarm@lists.cs.columbia.edu" , "linux-arm-kernel@lists.infradead.org" X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-arm-kernel-bounces@lists.infradead.org Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org On Fri, Nov 30, 2012 at 5:58 AM, Will Deacon 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 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: * In order to soft-boot, we need to switch to a 1:1 mapping for the --- -Christoffer 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); /*