From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=217.140.101.70; helo=foss.arm.com; envelope-from=mark.rutland@arm.com; receiver=edk2-devel@ml01.01.org Received: from foss.arm.com (usa-sjc-mx-foss1.foss.arm.com [217.140.101.70]) by ml01.01.org (Postfix) with ESMTP id D2752211B85E3 for ; Mon, 28 Jan 2019 02:23:17 -0800 (PST) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 15C46EBD; Mon, 28 Jan 2019 02:23:17 -0800 (PST) Received: from blommer (usa-sjc-mx-foss1.foss.arm.com [217.140.101.70]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 615873F589; Mon, 28 Jan 2019 02:23:15 -0800 (PST) Date: Mon, 28 Jan 2019 10:23:12 +0000 From: Mark Rutland To: Ard Biesheuvel Cc: Laszlo Ersek , Marc Zyngier , "edk2-devel@lists.01.org" , Christoffer Dall , Tanxiaojun Message-ID: <20190128102311.ailneth53n63tsbh@blommer> References: <1449471969-16949-1-git-send-email-ard.biesheuvel@linaro.org> <2dd4294c-76f0-f433-cbd2-bf0b37114aee@redhat.com> <12fa0861-e25d-eba7-48ea-2bd7d47d58fb@redhat.com> MIME-Version: 1.0 In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH] ArmPkg: update InvalidateInstructionCacheRange to flush only to PoU X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 28 Jan 2019 10:23:18 -0000 X-List-Received-Date: Mon, 28 Jan 2019 10:23:18 -0000 X-List-Received-Date: Mon, 28 Jan 2019 10:23:18 -0000 X-List-Received-Date: Mon, 28 Jan 2019 10:23:18 -0000 X-List-Received-Date: Mon, 28 Jan 2019 10:23:18 -0000 X-List-Received-Date: Mon, 28 Jan 2019 10:23:18 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Wed, Jan 23, 2019 at 03:02:14PM +0100, Ard Biesheuvel wrote: > On Wed, 23 Jan 2019 at 10:55, Laszlo Ersek wrote: > > > > On 01/23/19 10:26, Ard Biesheuvel wrote: > > > On Wed, 23 Jan 2019 at 10:14, Laszlo Ersek wrote: > > >> On 01/22/19 16:37, Ard Biesheuvel wrote: > > > > >>> Is SetUefiImageMemoryAttributes() being > > >>> called to remap the memory R-X ? > > >> > > >> No, it is not; the grub binary in question doesn't have the required > > >> section alignment (... I hope at least that that's what your question > > >> refers to): > > >> > > >>> ProtectUefiImageCommon - 0x3E6C54C0 > > >>> - 0x000000013BEEF000 - 0x0000000000030600 > > >>> !!!!!!!! ProtectUefiImageCommon - Section Alignment(0x200) is > > >> incorrect !!!!!!!! > > >> > > > > > > This is puzzling, given that the exact same binary works on Mustang. > > > > And even on the original (unspecified) hardware, the same binary works > > frequently. My understanding is that there are five VMs executing reboot > > loops in parallel, on the same host, and 4 out of 5 may hit the issue in > > a reasonable time period (300 reboots or so). > > > > > So when loaded, GRUB should cover the following regions: > > > > > > 0x13beef0000 - 0x13bf000000 (0x11000) > > > 0x13bf000000 - 0x13bf01f600 (0x1f600) > > > > > > where neither covers a 2 MB block fully, which means that the TLB > > > entry that we are hitting is stale. > > > > > > Since ProtectUefiImageCommon() does not do anything in this case, the > > > stale translation must be the result of > > > PcdDxeNxMemoryProtectionPolicy, which either sets the wrong > > > permissions for EfiLoaderCode (relying on ProtectUefiImageCommon), or > > > we don't flush the TLBs correctly after updating the permissions when > > > converting the memory from EfiConventionalMemory to EfiLoaderCode > > > > > > Are you using the default value for PcdDxeNxMemoryProtectionPolicy? > > > > Yes, we have > > > > ArmVirtPkg/ArmVirt.dsc.inc: > > gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC000000000007FD1 > > > > from commit 1acd7c54a724 ("ArmVirtPkg AARCH64: enable NX memory > > protection for all platforms", 2017-03-01). > > > > The binary is from the RPM > > "edk2-aarch64-20180508gitee3198e672e2-5.el8+1789+f0947240.noarch", which > > is basically upstream ee3198e672e2 plus a small number of backports and > > downstream customizations. > > > > This might help: > > diff --git a/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S > b/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S > index b7173e00b039..4c0b4b4efbd5 100644 > --- a/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S > +++ b/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S > @@ -138,7 +138,7 @@ ASM_FUNC(ArmUpdateTranslationTableEntry) > > ASM_FUNC(ArmInvalidateTlb) > EL1_OR_EL2_OR_EL3(x0) > -1: tlbi vmalle1 > +1: tlbi vmalle1is > b 4f > 2: tlbi alle2 > b 4f > diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S > b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S > index 90192df24f55..d54b1c19accf 100644 > --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S > +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S > @@ -34,7 +34,7 @@ > > // flush the TLBs > .if \el == 1 > - tlbi vmalle1 > + tlbi vmalle1is > .else > tlbi alle\el > .endif Assuming that hardware is working correctly, this change shouldn't be necessary. KVM sets HCR_EL2.FB, so all TLBI ops will behave as their *IS variant. Likewise it sets HCR_EL2.BSU, so barriers apply to the inner shareable domain too. On bare-metal, NSH should be sufficient. Thanks, Mark.