From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::144; helo=mail-it1-x144.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@ml01.01.org Received: from mail-it1-x144.google.com (mail-it1-x144.google.com [IPv6:2607:f8b0:4864:20::144]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 4CCF22117D75A for ; Tue, 22 Jan 2019 07:37:35 -0800 (PST) Received: by mail-it1-x144.google.com with SMTP id z7so22064916iti.0 for ; Tue, 22 Jan 2019 07:37:35 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Wbai4whQhldVwR+xIEr0vgtaglb4vWXzj5r6apLoYFQ=; b=SgCoqwLETjhMNr1rO3AnPqBxXNiGPJVsBfBffAvoi8SbxXVy1PJazffVSp3C24hxxE ThE7DFe+Glh+cO3QxPuUBWz/SWdpWewpGiSnFhuvkHihl1MilNMgF5iEaRS0sU5RerP0 KSkWadjJenc6tENgezNtisiXo3c+xa5yHTWtE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Wbai4whQhldVwR+xIEr0vgtaglb4vWXzj5r6apLoYFQ=; b=N+8xAEpy3JfNI7BRqezJRZmQoQD6jgu/vDpJG67MC7R+AMF7Z9JZJ89Yc3ydRsKjkl qvKdr5hIvLcZd6f72FzkvjXgITSgMNJQQ//+xsKRnxVX8MmciA8ezEle8GkL62JyKPU9 MYDL1h1AZE04LKyhgR9tvE9QzCLQjle+pkMqRVUnFJ+tbrhmrfMTX66qxFymaPO+euF0 9E8N8lk04l8114UHvdfhgStNWe3VyN9auK0gEIau8LOY9V/a1w+T9SS26hxMe1mLnbiU QvV5fcJaiakCUm70tLkk9HKvT0WGoUXd2E49XVHxVkzowxp9tQQ05BRIjmgK5y5ylTI8 Dgzg== X-Gm-Message-State: AJcUukdENLTsyhPrzodA40E5riALSXuIicviJ7ie0/1Q64KuCCI1wGUg KQEq6Jyb8vR5Un0Q9vC8qUK7rGGEMyqgsfvNSUtzwg== X-Google-Smtp-Source: ALg8bN6k5mCzCUng98Z52vpBE+Vxk+y2onruW1rUJ+q8a4XnKEBEKPZDzQedKXisPVz/UF+oTSPntdmmO9SYKrRDdcY= X-Received: by 2002:a02:183:: with SMTP id 3mr20936950jak.130.1548171454061; Tue, 22 Jan 2019 07:37:34 -0800 (PST) MIME-Version: 1.0 References: <1449471969-16949-1-git-send-email-ard.biesheuvel@linaro.org> In-Reply-To: From: Ard Biesheuvel Date: Tue, 22 Jan 2019 16:37:22 +0100 Message-ID: To: Laszlo Ersek Cc: Leif Lindholm , "Cohen, Eugene" , "edk2-devel@lists.01.org" , Tanxiaojun , Marc Zyngier , Christoffer Dall 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:37:35 -0000 Content-Type: text/plain; charset="UTF-8" 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) > 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? Is SetUefiImageMemoryAttributes() being called to remap the memory R-X ? > 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