From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web09.5381.1645606726266514471 for ; Wed, 23 Feb 2022 00:58:46 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=KMIuMXD9; spf=pass (domain: kernel.org, ip: 139.178.84.217, mailfrom: maz@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 34B7B61617; Wed, 23 Feb 2022 08:58:45 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 8C792C340E7; Wed, 23 Feb 2022 08:58:44 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1645606724; bh=6eZf3uTzr8tEF99eaN6MKixougQQUgaF4rBtcXrmQWc=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=KMIuMXD9PL8w3hT2weIbsfCThG+s2CJCPXCFee1dsV8G44RglDYXSbwMVD4R5Fshf qd1X6L85m4ycin+1vbVuRq/zWNpV0APywVOI0NSeWkifvD6GRnshi/dHwFzHgzOVZc Ah6ZWQz0R+mSW4QJ8NoXYodN3ntL9jBzmnKWF7lFviW9H45IQXNJ+xVeH82Q1zdIzQ wufyTIum776fDZae9UmOGTJpRmZozJTWDMxjxdDrpS2isgQj8j4svxaxufmYG3fU25 67ADKr7f/Cv2dn8Lj5A8MyFk4npj8HNsRTzTKpv+P6bdfgfNNzr3p9H5NDmx4qBpVL 12mt2+rTKdX6w== Received: from sofa.misterjones.org ([185.219.108.64] helo=why.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1nMnTu-009mKi-4m; Wed, 23 Feb 2022 08:58:42 +0000 Date: Wed, 23 Feb 2022 08:58:41 +0000 Message-ID: <877d9m3qny.wl-maz@kernel.org> From: Marc Zyngier To: Ard Biesheuvel , Ashish Singhal Cc: edk2-devel-groups-io , Sami Mujawar , Ard Biesheuvel , Leif Lindholm Subject: Re: [PATCH] ArmPkg: Invalidate Instruction Cache On MMU Enable In-Reply-To: References: <122c32bb19ed0730ef166b9f46d3b112bc9ed937.1645497637.git.ashishsingha@nvidia.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: ardb@kernel.org, ashishsingha@nvidia.com, devel@edk2.groups.io, sami.mujawar@arm.com, ardb+tianocore@kernel.org, quic_llindhol@quicinc.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Content-Type: text/plain; charset=US-ASCII On Wed, 23 Feb 2022 07:02:28 +0000, Ard Biesheuvel wrote: > > (+ Marc) > > On Tue, 22 Feb 2022 at 03:42, Ashish Singhal wrote: > > > > Even with MMU turned off, instruction cache can speculate > > and fetch instructions. This can cause a crash if region > > being executed has been modified recently. With this patch, > > we ensure that instruction cache is invalidated right after > > MMU has been enabled and any potentially stale instruction > > fetched earlier has been discarded. Are you doing code patching while the MMU is off? > > > > I don't follow this reasoning. Every time the UEFI image loader loads > an image into memory, it does so with MMU and caches enabled, and the > code regions are cleaned to the PoU before being invalidated in the > I-cache. So how could these regions be stale? The only code that needs > special care here is the little trampoline that executes with the MMU > off, but this is being taken care of explicitly, by cleaning the code > to the PoC before invoking it. > > > This is specially helpful when the memory attributes of a > > region in MMU are being changed and some instructions > > operating on the region are prefetched in the instruction > > cache. > > Huh, this is way too vague. How do you expect anyone to understand your problem based on this? > > This sounds like a TLB problem not a cache problem. For the sake of > posterity, could you include a more detailed description of the issue > in the commit log? IIRC, this is about speculative instruction fetches > hitting device memory locations? > > As I mentioned before, the architecture does not permit speculative > instruction fetches for regions mapped with the XN attribute. > > Is the issue occurring under virtualization? Is there a stage 2 > mapping that lacks the XN attribute for the device memory region in > question? > > > Signed-off-by: Ashish Singhal > > --- > > ArmPkg/Library/ArmLib/AArch64/AArch64Support.S | 4 +++- > > ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S | 2 ++ > > 2 files changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S > > index d3cc1e8671..9648245182 100644 > > --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S > > +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S > > @@ -89,7 +89,9 @@ ASM_FUNC(ArmEnableMmu) > > dsb nsh > > isb > > msr sctlr_el3, x0 // Write back > > -4: isb > > +4: ic iallu > > + dsb sy Why DSB SY? Given that you are only invalidating on a single CPU, DSB NSH should be enough. > > + isb > > ret > > > > > > diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S > > index 66ebca571e..56cc2dd73f 100644 > > --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S > > +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S > > @@ -37,6 +37,8 @@ > > > > // re-enable the MMU > > msr sctlr_el\el, x8 > > + ic iallu > > + dsb sy Same thing here. M. -- Without deviation from the norm, progress is not possible.