From patchwork Thu Nov 29 18:59:07 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christoffer Dall X-Patchwork-Id: 1822251 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 074A7DF23A for ; Thu, 29 Nov 2012 19:02:37 +0000 (UTC) Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1Te9KO-0007zr-IL; Thu, 29 Nov 2012 18:59:16 +0000 Received: from mail-vc0-f177.google.com ([209.85.220.177]) by merlin.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1Te9KH-0007zZ-80 for linux-arm-kernel@lists.infradead.org; Thu, 29 Nov 2012 18:59:13 +0000 Received: by mail-vc0-f177.google.com with SMTP id f13so9485004vcb.36 for ; Thu, 29 Nov 2012 10:59:07 -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=RvGDrDAjq2mqdf4bncayrPYTV9qTSdoXMlNOzSj87DM=; b=LQDfNjJtWHUQ21eEhx+mGrjrZ+Ky2zCiie5aQqCAbJ/v9DonGqJZawfZXeqod2qyqC y1cp0+I35xvTSmp8V1RHrZXsjrhv19qnPLwW6qUP0vb2h3+Z4d1ort05qt9Lx3yAJ4C1 GclPvaskUXyaUOwi4L0FcPw8GHbH/C2AOh3sRjjdkEGColBle5WSLkwJ+YCkyVVHOORh Ht6I7W5iwppTleSsAt7arDfYGbLTLuCLFqLrUmo6h7cHWQ92LWk/c+I4rDgQ5ZTqVz4n 3ZtawyYzP4SqOmwsKYuGnONNDO2W5KuL8BrCrOyV8BMczBGsn4Oxa4EqL9rtbhjVXga3 ASFg== MIME-Version: 1.0 Received: by 10.59.13.197 with SMTP id fa5mr33632697ved.47.1354215547391; Thu, 29 Nov 2012 10:59:07 -0800 (PST) Received: by 10.58.33.198 with HTTP; Thu, 29 Nov 2012 10:59:07 -0800 (PST) X-Originating-IP: [128.59.22.176] In-Reply-To: <20121119141631.GU3205@mudshark.cambridge.arm.com> References: <20121110154203.2836.46686.stgit@chazy-air> <20121110154224.2836.21775.stgit@chazy-air> <20121119141631.GU3205@mudshark.cambridge.arm.com> Date: Thu, 29 Nov 2012 13:59:07 -0500 Message-ID: Subject: Re: [PATCH v4 02/14] ARM: Section based HYP idmap From: Christoffer Dall To: Will Deacon X-Gm-Message-State: ALoCoQnwfKs2R7g2c0Xiy3NQ8ohrcf65oQEZmTAHS9u18xlDyTu3L05zn19yB8xgkdPUHxaxQ6er X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20121129_135909_425966_431245F9 X-CRM114-Status: GOOD ( 31.08 ) 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.220.177 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 Mon, Nov 19, 2012 at 9:16 AM, Will Deacon wrote: > On Sat, Nov 10, 2012 at 03:42:24PM +0000, Christoffer Dall wrote: >> Add a method (hyp_idmap_setup) to populate a hyp pgd with an >> identity mapping of the code contained in the .hyp.idmap.text >> section. >> >> Offer a method to drop this identity mapping through >> hyp_idmap_teardown. >> >> Make all the above depend on CONFIG_ARM_VIRT_EXT and CONFIG_ARM_LPAE. >> >> Cc: Will Deacon >> Reviewed-by: Marcelo Tosatti >> Signed-off-by: Marc Zyngier >> Signed-off-by: Christoffer Dall >> --- >> arch/arm/include/asm/idmap.h | 5 ++ >> arch/arm/include/asm/pgtable-3level-hwdef.h | 1 >> arch/arm/kernel/vmlinux.lds.S | 6 ++ >> arch/arm/mm/idmap.c | 74 +++++++++++++++++++++++---- >> 4 files changed, 73 insertions(+), 13 deletions(-) >> >> diff --git a/arch/arm/include/asm/idmap.h b/arch/arm/include/asm/idmap.h >> index bf863ed..36708ba 100644 >> --- a/arch/arm/include/asm/idmap.h >> +++ b/arch/arm/include/asm/idmap.h >> @@ -11,4 +11,9 @@ 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); >> +#endif > > I wonder whether the dependency is quite right here. Functionally, we're > only dependent on LPAE but it doesn't make sense if !ARM_VIRT_EXT. In > fact, I start to question whether CONFIG_ARM_VIRT_EXT is worthwhile at > all as it stands. Maybe it would be better to add the LPAE dependency > there and make the hyp_stub stuff that's currently in mainline > unconditional? > perhaps it would be more clean, but it's not something that would break to fix later, and not in that sense something this patch series would depend on. If the hyp_stub stuff is truly compatible with all legacy stuff then yes, we could remove that define, but if not, then I guess it's necessary. For now, I'll simply remove these ifdef's as Rob pointed out. >> +/* >> + * 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) >> +{ >> + 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)) >> + hyp_idmap_del_pmd(pgd, addr); >> + } while (pgd++, addr = next, addr < end); >> +} >> +EXPORT_SYMBOL_GPL(hyp_idmap_teardown); >> + >> +void hyp_idmap_setup(pgd_t *hyp_pgd) >> +{ >> + identity_mapping_add(hyp_pgd, __hyp_idmap_text_start, >> + __hyp_idmap_text_end, PMD_SECT_AP1); >> +} >> +EXPORT_SYMBOL_GPL(hyp_idmap_setup); >> +#endif > > Again, do we need these exports? no we don't > > 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. Additionally, other potential users of Hyp mode might have a separate hyp_pgd, in theory. While KVM is the only current user of hyp_idmap_teardown it's not really KVM specific, and the could theoretically be other users of hyp idmaps. Also, the externs __hyp_idmap_text_ would have to be either moved to a header file or duplicated inside KVM, which is also not that pretty. I see how you would like to clean this up, but it's not really hard to read or understand, imho: The ifdef cleanup patch: From 9cbe2830b74072b4d7167254562fc06c08f724e1 Mon Sep 17 00:00:00 2001 From: Christoffer Dall Date: Thu, 29 Nov 2012 13:56:03 -0500 Subject: [PATCH] KVM: ARM: Remove exports and ifdefs in hyp idmap code The exports are no longer needed as KVM/ARM cannot be compiled as a module and the the #ifdef is not required around a declaration. Cc: Will Deacon Cc: Rob Herring Signed-off-by: Christoffer Dall --- arch/arm/include/asm/idmap.h | 2 -- arch/arm/mm/idmap.c | 2 -- 2 files changed, 4 deletions(-) diff --git a/arch/arm/include/asm/idmap.h b/arch/arm/include/asm/idmap.h index 36708ba..6ddb707 100644 --- a/arch/arm/include/asm/idmap.h +++ b/arch/arm/include/asm/idmap.h @@ -11,9 +11,7 @@ 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); -#endif #endif /* __ASM_IDMAP_H */ diff --git a/arch/arm/mm/idmap.c b/arch/arm/mm/idmap.c index ea7430e..0b002ee 100644 --- a/arch/arm/mm/idmap.c +++ b/arch/arm/mm/idmap.c @@ -136,14 +136,12 @@ void hyp_idmap_teardown(pgd_t *hyp_pgd) hyp_idmap_del_pmd(pgd, addr); } while (pgd++, addr = next, addr < end); } -EXPORT_SYMBOL_GPL(hyp_idmap_teardown); void hyp_idmap_setup(pgd_t *hyp_pgd) { identity_mapping_add(hyp_pgd, __hyp_idmap_text_start, __hyp_idmap_text_end, PMD_SECT_AP1); } -EXPORT_SYMBOL_GPL(hyp_idmap_setup); #endif /*