From patchwork Wed Nov 7 20:10:49 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nicolas Pitre X-Patchwork-Id: 1712711 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 B4A79DFB7A for ; Wed, 7 Nov 2012 20:12:44 +0000 (UTC) Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1TWBxg-0006ia-8y; Wed, 07 Nov 2012 20:10:56 +0000 Received: from mail-qa0-f42.google.com ([209.85.216.42]) by merlin.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1TWBxc-0006hj-Fe for linux-arm-kernel@lists.infradead.org; Wed, 07 Nov 2012 20:10:53 +0000 Received: by mail-qa0-f42.google.com with SMTP id b33so2349018qad.15 for ; Wed, 07 Nov 2012 12:10:51 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=date:from:to:cc:subject:in-reply-to:message-id:references :user-agent:mime-version:content-type:x-gm-message-state; bh=01Uj/bvmCIoQczLH8Dw52LbeD3GQS3gJrp7N4DgnJcg=; b=f7lWOJiBrRvUTVid0irbRH5tXqyHvC9ytrsi8HoYFmVIcBQxLoo+tEOEOZcgjlZP5s sK3tYxH6F2bpxSUdqEIm8nAgzwpJ4oW5xJXrjDn62irUlhDhQT6ci+ckUc1jI3RNVYX4 Pual2SErVzhuu+/+hYTly90HxGq6SDuR9hewwd4AqHe3QqtHoxeUxoyU0mJHEzQfZziK veMr8eeQ9NeWzxhaMp4ooJXfhsXvwr/bsjdOcjm1VCXvxQgCboxHx1uaFBFjDS6pd1yd lzQHI4hfy3Gd0pGFZdvJVI9+hiyPnKzwq3TP5/B59AVRX9CPNwnXrCE646f4EPXDTiEs SI6Q== Received: by 10.224.70.140 with SMTP id d12mr8503280qaj.53.1352319051431; Wed, 07 Nov 2012 12:10:51 -0800 (PST) Received: from xanadu.home (modemcable203.213-202-24.mc.videotron.ca. [24.202.213.203]) by mx.google.com with ESMTPS id jt10sm12812974qeb.4.2012.11.07.12.10.50 (version=TLSv1/SSLv3 cipher=OTHER); Wed, 07 Nov 2012 12:10:50 -0800 (PST) Date: Wed, 7 Nov 2012 15:10:49 -0500 (EST) From: Nicolas Pitre To: Will Deacon Subject: Re: [PATCH] ARM: setup_mm_for_reboot(): use flush_cache_louis() In-Reply-To: <20121107184129.GB18293@mudshark.cambridge.arm.com> Message-ID: References: <20121106220458.GP28327@n2100.arm.linux.org.uk> <20121107095106.GA23305@mudshark.cambridge.arm.com> <20121107095635.GV28327@n2100.arm.linux.org.uk> <20121107100835.GB23305@mudshark.cambridge.arm.com> <20121107184129.GB18293@mudshark.cambridge.arm.com> User-Agent: Alpine 2.02 (LFD 1266 2009-07-14) MIME-Version: 1.0 X-Gm-Message-State: ALoCoQk4lWKdGaqCXaCCimCevVUcjkPp0iU9JtIkSYZAvqFY18xQ0mTvvm1dicFKT2n7WCGzC8LQ X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20121107_151052_671991_5CF451EA X-CRM114-Status: GOOD ( 38.82 ) 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.216.42 listed in list.dnswl.org] -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] Cc: Santosh Shilimkar , Lorenzo Pieralisi , Russell King - ARM Linux , "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 Wed, 7 Nov 2012, Will Deacon wrote: > On Wed, Nov 07, 2012 at 06:03:40PM +0000, Nicolas Pitre wrote: > > On Wed, 7 Nov 2012, Will Deacon wrote: > > > On Wed, Nov 07, 2012 at 09:56:35AM +0000, Russell King - ARM Linux wrote: > > > > On Wed, Nov 07, 2012 at 09:51:06AM +0000, Will Deacon wrote: > > > > > Wouldn't the L2 flush in this case be included with the code that turns off > > > > > caching? For reboot, that's currently done in __sort_restart -- the cache > > > > > flush in setup_mm_for_reboot is just to ensure that the idmap tables are > > > > > visible to the hardware walker iirc. > > > > > > > > Good point - but it does raise another issue. Why do we do this flush and > > > > TLB invalidate afterwards in setup_mm_for_reboot() ? It makes sense if > > > > we change existing page tables, but we don't anymore, we're just switching > > > > them, and cpu_switch_mm() will do whatever's necessary to make the new > > > > page tables visible. So I think we can get rid of that flusing in there. > > > > > > Hmm, I'm not sure about that. Looking at cpu_v7_switch_mm (the 2-level > > > version) for example, we set the ASID and then set the new TTBR. There's no > > > flushing of page tables like we get in set_pte and no TLB flushing either. > > > > > > Now, given that the idmap_pgd is populated as an early_initcall, I really > > > doubt we need that flush_cache_all but the TLB invalidate is surely > > > required? > > > > Why wouldn't cpu_switch_mm() take care of that already if that is > > necessary? Hmmm, I suppose the asid of the init task isn't associated > > with the idmap in any way, hence the need for flushing the tlbs. > > Yes, cpu_switch_mm can't know about whether tables are visible or an ASID is > dirty so all it can do is defer that to the caller or do the cleaning and > invalidation every time. > > > I'd not rely on the early_initcall to assume the new page table is moved > > out of the cache though. > > Yeah, it's not nice, I was just pointing out that it's all hanging off an > initcall now (before it was created ad-hoc by its users). > > > What about this alternate patch: > > > > diff --git a/arch/arm/mm/idmap.c b/arch/arm/mm/idmap.c > > index ab88ed4f8e..1ab429761c 100644 > > --- a/arch/arm/mm/idmap.c > > +++ b/arch/arm/mm/idmap.c > > @@ -92,6 +92,9 @@ static int __init init_static_idmap(void) > > (long long)idmap_start, (long long)idmap_end); > > identity_mapping_add(idmap_pgd, idmap_start, idmap_end); > > > > + /* Flush L1 for the hardware to see this page table content */ > > + flush_cache_louis(); > > + > > return 0; > > } > > Yep, we can do this now. Good thinking! At some point I'll get around to > making this conditional, as I don't think it's needed on A5, A7, A9 or A15. > > > early_initcall(init_static_idmap); > > @@ -103,12 +106,12 @@ early_initcall(init_static_idmap); > > */ > > void setup_mm_for_reboot(void) > > { > > - /* Clean and invalidate L1. */ > > - flush_cache_all(); > > - > > /* Switch to the identity mapping. */ > > cpu_switch_mm(idmap_pgd, &init_mm); > > > > - /* Flush the TLB. */ > > + /* > > + * On ARMv7, the ASID of the init task is not associated with > > + * this mapping. TLBs must be flushed. > > + */ > > local_flush_tlb_all(); > > } > > Can we change this comment slightly please? Basically, we don't have a clean > ASID for the identity mapping, which may clash with virtual addresses of the > previous page tables and therefore potentially in the TLB. That's why > we need the invalidation: so that we don't hit stale entries from before. > > Other than that, looks good to me: > > Acked-by: Will Deacon Here's my latest version. I made the tlb flush conditional. Please review again before I add your ACK. ---- >8 From: Nicolas Pitre Subject: [PATCH] ARM: idmap: use flush_cache_louis() and flush TLBs only when necessary Flushing the cache is needed for the hardware to see the idmap table and therefore can be done at init time. On ARMv7 it is not necessary to flush L2 so flush_cache_louis() is used here instead. There is no point flushing the cache in setup_mm_for_reboot() as the caller should, and already is, taking care of this. If switching the memory map requires a cache flush, then cpu_switch_mm() already includes that operation. What is not done by cpu_switch_mm() on ASID capable CPUs is TLB flushing as the whole point of the ASID is to tag the TLBs and avoid flushing them on a context switch. Since we don't have a clean ASID for the identity mapping, we need to flush the TLB explicitly in that case. Otherwise this is already performed by cpu_switch_mm(). Signed-off-by: Nicolas Pitre Acked-by: Will Deacon diff --git a/arch/arm/mm/idmap.c b/arch/arm/mm/idmap.c index ab88ed4f8e..99db769307 100644 --- a/arch/arm/mm/idmap.c +++ b/arch/arm/mm/idmap.c @@ -92,6 +92,9 @@ static int __init init_static_idmap(void) (long long)idmap_start, (long long)idmap_end); identity_mapping_add(idmap_pgd, idmap_start, idmap_end); + /* Flush L1 for the hardware to see this page table content */ + flush_cache_louis(); + return 0; } early_initcall(init_static_idmap); @@ -103,12 +106,15 @@ early_initcall(init_static_idmap); */ void setup_mm_for_reboot(void) { - /* Clean and invalidate L1. */ - flush_cache_all(); - /* Switch to the identity mapping. */ cpu_switch_mm(idmap_pgd, &init_mm); - /* Flush the TLB. */ +#ifdef CONFIG_CPU_HAS_ASID + /* + * We don't have a clean ASID for the identity mapping, which + * may clash with virtual addresses of the previous page tables + * and therefore potentially in the TLB. + */ local_flush_tlb_all(); +#endif }