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: 1822241 Return-Path: X-Original-To: patchwork-kvm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork1.kernel.org (Postfix) with ESMTP id 503BC3FC23 for ; Thu, 29 Nov 2012 18:59:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752745Ab2K2S7J (ORCPT ); Thu, 29 Nov 2012 13:59:09 -0500 Received: from mail-vb0-f46.google.com ([209.85.212.46]:51146 "EHLO mail-vb0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751191Ab2K2S7I (ORCPT ); Thu, 29 Nov 2012 13:59:08 -0500 Received: by mail-vb0-f46.google.com with SMTP id ff1so9578012vbb.19 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=CacPPCAQ3+C1ved8yMk51Dh1yau1lPJLDiQUd2q6lZUPmmdzR4eRLGwrcRhHRBN/f0 X1KzHK1+2aCRWCtUuiDwvo2ixDRmv2EKhYU6Mh5vnXQ1gmBNfbfYzJKopQRvGV0s5iWb lwtkDXGcEmKPo3rF+Iv7ugSpd2Pe+B+No3ZW+D/IaqmveXul0B2nhglNHEKtKEpcOkl9 08G3Mp6ZzTyoLgmcmITOfnJIzndY2nKnWCyh3qqMdAfkNIT0ylBoLdc0knzEfInqeUFR DwtYSOBnGNn85Mpa4fct3CFLK4ttxdYNGC2ioVLiLZDza7CJ2HR4OmQ+GSplU4Jtg2GH 1nWQ== 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 Cc: "kvm@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "kvmarm@lists.cs.columbia.edu" , Marc Zyngier , Marcelo Tosatti , dave.martin@linaro.org, robherring2@gmail.com X-Gm-Message-State: ALoCoQmyGN6w6y7kuzVAlSLWLGYmKbVE8OCG3BskzVRCFPrbohfVB1AOc2imZdocDmr7VqUKzgre Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.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 /*