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 D8875211963EC for ; Wed, 23 Jan 2019 01:14:30 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8BEEA8764F; Wed, 23 Jan 2019 09:14:29 +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 AB570101963F; Wed, 23 Jan 2019 09:14:26 +0000 (UTC) To: Ard Biesheuvel Cc: Leif Lindholm , "Cohen, Eugene" , "edk2-devel@lists.01.org" , Tanxiaojun , Marc Zyngier , Christoffer Dall References: <1449471969-16949-1-git-send-email-ard.biesheuvel@linaro.org> From: Laszlo Ersek Message-ID: <2dd4294c-76f0-f433-cbd2-bf0b37114aee@redhat.com> Date: Wed, 23 Jan 2019 10:14:25 +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: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Wed, 23 Jan 2019 09:14:29 +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: Wed, 23 Jan 2019 09:14:31 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 01/22/19 16:37, Ard Biesheuvel wrote: > On Tue, 22 Jan 2019 at 16:09, Laszlo Ersek wrote: >> >> 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? >> > > Cache maintenance by virtual address to the PoU is broadcast to the > inner shareable shareability domain. So this should be sufficient to > ensure that the CPU executing the instruction sees the correct data. > > Our code uses full system barriers after each stage of cache > maintenance, so the code looks correct to me. However, it does rely on > the host firmware to configure the inner shareable shareability domain > correctly (and use the correct memory attributes for all memory it > maps) OK, thank you. >> 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. >> > > This appears to be a TLB maintenance issue, not a cache maintenance > issue. Level 2 mappings are 2 MB block mappings, and so we usually > have smaller mappings for executables (unless GRUB is much larger than > 2 MB and it covers an entire 2 MB naturally aligned block) > > Does it happen in DEBUG mode? Yes, it does, with full debug logs enabled. > 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 !!!!!!!! Thank you, Laszlo