From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@ml01.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 7E34D211B819E for ; Tue, 22 Jan 2019 07:09:48 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id DB5203680F; Tue, 22 Jan 2019 15:09:47 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-61.rdu2.redhat.com [10.10.120.61]) by smtp.corp.redhat.com (Postfix) with ESMTP id A59506252A; Tue, 22 Jan 2019 15:09:45 +0000 (UTC) To: Ard Biesheuvel , Leif Lindholm , "Cohen, Eugene" References: <1449471969-16949-1-git-send-email-ard.biesheuvel@linaro.org> Cc: edk2-devel@ml01.01.org, tanxiaojun@huawei.com, Marc Zyngier , Christoffer Dall From: Laszlo Ersek Message-ID: Date: Tue, 22 Jan 2019 16:09:44 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <1449471969-16949-1-git-send-email-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Tue, 22 Jan 2019 15:09:48 +0000 (UTC) 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: Tue, 22 Jan 2019 15:09:49 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Performing thread-necromancy on a patch from 2015, which is today known as commit b7de7e3cab3f. Also CC'ing Christoffer and Marc. Question at the bottom. On 12/07/15 08:06, Ard Biesheuvel wrote: > From: "Cohen, Eugene" > > This patch updates the ArmPkg variant of InvalidateInstructionCacheRange to > flush the data cache only to the point of unification (PoU). This improves > performance and also allows invalidation in scenarios where it would be > inappropriate to flush to the point of coherency (like when executing code > from L2 configured as cache-as-ram). > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Eugene Cohen > > Added AARCH64 and ARM/GCC implementations of the above. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel > --- > ArmPkg/Include/Library/ArmLib.h | 8 +++++++- > ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c | 2 +- > ArmPkg/Library/ArmLib/AArch64/AArch64Support.S | 6 ++++++ > ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S | 6 ++++++ > ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm | 5 +++++ > 5 files changed, 25 insertions(+), 2 deletions(-) > > diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h > index 9622444ec63f..85fa1f600ba9 100644 > --- a/ArmPkg/Include/Library/ArmLib.h > +++ b/ArmPkg/Include/Library/ArmLib.h > @@ -183,12 +183,18 @@ ArmInvalidateDataCacheEntryByMVA ( > > VOID > EFIAPI > -ArmCleanDataCacheEntryByMVA ( > +ArmCleanDataCacheEntryToPoUByMVA( > IN UINTN Address > ); > > VOID > EFIAPI > +ArmCleanDataCacheEntryByMVA( > +IN UINTN Address > +); > + > +VOID > +EFIAPI > ArmCleanInvalidateDataCacheEntryByMVA ( > IN UINTN Address > ); > diff --git a/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c b/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c > index feab4497ac1b..1045f9068f4d 100644 > --- a/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c > +++ b/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c > @@ -64,7 +64,7 @@ InvalidateInstructionCacheRange ( > IN UINTN Length > ) > { > - CacheRangeOperation (Address, Length, ArmCleanDataCacheEntryByMVA); > + CacheRangeOperation (Address, Length, ArmCleanDataCacheEntryToPoUByMVA); > ArmInvalidateInstructionCache (); > return Address; > } > diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S > index c530d19e897e..db21f73f0ed7 100644 > --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S > +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S > @@ -22,6 +22,7 @@ > GCC_ASM_EXPORT (ArmInvalidateInstructionCache) > GCC_ASM_EXPORT (ArmInvalidateDataCacheEntryByMVA) > GCC_ASM_EXPORT (ArmCleanDataCacheEntryByMVA) > +GCC_ASM_EXPORT (ArmCleanDataCacheEntryToPoUByMVA) > GCC_ASM_EXPORT (ArmCleanInvalidateDataCacheEntryByMVA) > GCC_ASM_EXPORT (ArmInvalidateDataCacheEntryBySetWay) > GCC_ASM_EXPORT (ArmCleanDataCacheEntryBySetWay) > @@ -72,6 +73,11 @@ ASM_PFX(ArmCleanDataCacheEntryByMVA): > ret > > > +ASM_PFX(ArmCleanDataCacheEntryToPoUByMVA): > + dc cvau, x0 // Clean single data cache line to PoU > + ret > + > + > ASM_PFX(ArmCleanInvalidateDataCacheEntryByMVA): > dc civac, x0 // Clean and invalidate single data cache line > ret > diff --git a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S > index 5f030d92de31..7de1b11ef818 100644 > --- a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S > +++ b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S > @@ -19,6 +19,7 @@ > GCC_ASM_EXPORT (ArmInvalidateInstructionCache) > GCC_ASM_EXPORT (ArmInvalidateDataCacheEntryByMVA) > GCC_ASM_EXPORT (ArmCleanDataCacheEntryByMVA) > +GCC_ASM_EXPORT (ArmCleanDataCacheEntryToPoUByMVA) > GCC_ASM_EXPORT (ArmCleanInvalidateDataCacheEntryByMVA) > GCC_ASM_EXPORT (ArmInvalidateDataCacheEntryBySetWay) > GCC_ASM_EXPORT (ArmCleanDataCacheEntryBySetWay) > @@ -69,6 +70,11 @@ ASM_PFX(ArmCleanDataCacheEntryByMVA): > bx lr > > > +ASM_PFX(ArmCleanDataCacheEntryToPoUByMVA): > + mcr p15, 0, r0, c7, c11, 1 @clean single data cache line to PoU > + bx lr > + > + > ASM_PFX(ArmCleanInvalidateDataCacheEntryByMVA): > mcr p15, 0, r0, c7, c14, 1 @clean and invalidate single data cache line > bx lr > diff --git a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm > index df7e22dca2d9..a460bd2da7a9 100644 > --- a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm > +++ b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm > @@ -34,6 +34,11 @@ CTRL_I_BIT EQU (1 << 12) > bx lr > > > + RVCT_ASM_EXPORT ArmCleanDataCacheEntryToPoUByMVA > + mcr p15, 0, r0, c7, c11, 1 ; clean single data cache line to PoU > + bx lr > + > + > RVCT_ASM_EXPORT ArmCleanInvalidateDataCacheEntryByMVA > mcr p15, 0, r0, c7, c14, 1 ; clean and invalidate single data cache line > bx lr > The implementation of the LoadImage boot service calls InvalidateInstructionCacheRange(): CoreLoadImage() [MdeModulePkg/Core/Dxe/Image/Image.c] CoreLoadImageCommon() [MdeModulePkg/Core/Dxe/Image/Image.c] CoreLoadPeImage() [MdeModulePkg/Core/Dxe/Image/Image.c] InvalidateInstructionCacheRange() [ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c] Assuming that this code runs in a virtual machine, is any one the following scenarios plausible? (1) the dcache is flushed to PoU on physical socket #M, and the icache is invalidated to PoU on physical socket #N, M being unequal to N (2) the dcache cleaning + icache invalidation happen on the same physical socket #M, but the image execution (via StartImage) commences on socket #N In either case, DRAM (PoC) would not form a "bridge" between the physical CPUs sitting in said sockets. If the host kernel / KVM re-scheduled VCPU#0 from socket #M to socket #N in the "middle" of the above scenarios (1) and (2), could that result in StartImage executing *not* what LoadImage loaded? Because, we got a report -- sorry, private bug :/ -- in which the *very first* instruction of grub crashes. That is, the instruction that forms the entry point of the image. The symptom is: > ESR : EC 0x21 IL 0x1 ISS 0x0000000E > > Instruction abort: Permission fault, second level If I'm parsing the ARM ARM correctly: - "ESR" stands for "Exception Syndrome Register". - "EC 0x21" stands for Exception Class 0x21, "Instruction Abort taken without a change of Exception level. Used for MMU faults generated by instruction accesses, and for synchronous external aborts, including synchronous parity errors." - "ISS" stands for "Instruction Specific Syndrome". - "ISS 0x00E" (24 bit value), given EC 0x21, stands for "Permission fault, second level", as the log itself says. The crash reproduces intermittently / unpredictably, and it happens on unspecified hardware. Personally I couldn't reproduce it on my APM Mustang A3, using the exact same guest ISO. So, I'm thinking that, as soon as CoreStartImage() calls Image->EntryPoint(), the instruction fetch blows up, because we didn't clean the dcache / invalidate the icache *to PoC* in CoreLoadImage(), and KVM moved VCPU#0 to a different physical socket meanwhile. Am I totally lost? :) (I plan to undo commit cf580da1bc4c ("ArmPkg/ArmLib: don't invalidate entire I-cache on range operation", 2016-05-12) and commit b7de7e3cab3f ("ArmPkg: update InvalidateInstructionCacheRange to flush only to PoU", 2015-12-08), and to ask the reporter to reproduce the issue like that, but I figured I'd ask first.) Thanks! Laszlo