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::143; helo=mail-it1-x143.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@ml01.01.org Received: from mail-it1-x143.google.com (mail-it1-x143.google.com [IPv6:2607:f8b0:4864:20::143]) (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 E47FE211AF9E8 for ; Wed, 23 Jan 2019 01:26:53 -0800 (PST) Received: by mail-it1-x143.google.com with SMTP id w18so2277487ite.1 for ; Wed, 23 Jan 2019 01:26:53 -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=FjvbMKRBwHPrAHB2B27SkAoEQKj9CaNggmcqrucdvcM=; b=i4xBuIFplyWVOM5TNYt8IfujneMgSW39zCXbUxaqf5vLmT4pxxKpJ+ea6OscNfa6jL WlaDQd1RWqr9in6Rf3vOUcViY1+cnJqmrnEerrnKTywHRUO9SRfX7MeJSHl2OBV9saKL jHXI9yWNe/ZoeVhqS93oVQWUymjT3JVD6pg8k= 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=FjvbMKRBwHPrAHB2B27SkAoEQKj9CaNggmcqrucdvcM=; b=R7w9sb/M2mjWMiR0hvNJHVMHK3zAXF9lZDRDnQD3FcsjTkptTTApAIOhPxy708vYqB JpHH+yWWciUmoWl8I2U3P8HaYamdCADe4Y5UZrB+6MvAZ3jnP8u2cUuXHzk+UuyXw/h3 lizbSSfERfKMGy3TicTXKSQTx3C1A074TVI8BGKYIR6sFi43BbuEb5UToS4yquPg8F3C B6m1vw4sf9LVJeMxLVQlw9LA0BfJaULv92O30xaXOINcINMcRROXZQW+rOgpwXPhhWug yljkNUbrUcWVl5PYq6NKQmPHWNJhU3xlYMPxgrhTiZS+MKt8LXuviP7Oq3qTdqE801xZ KPcw== X-Gm-Message-State: AJcUukfzp7/vLXC5Q8NYgmgILy4Kk1CskyqpEeAhJoZ/lW7XJIw+Ijuj QVD/TOzAuBwz641qHkwzQSsA/A88hsGtqDgvGg1XqA== X-Google-Smtp-Source: ALg8bN48KRjS0XykyZvQpfETijqeDMDTSRWfzFiF2KNBhx/LxmyuQXO4t3fEbFE3afSMkwLC1XwOOlT1yE1555qwEEk= X-Received: by 2002:a02:734b:: with SMTP id a11mr463805jae.62.1548235612498; Wed, 23 Jan 2019 01:26:52 -0800 (PST) MIME-Version: 1.0 References: <1449471969-16949-1-git-send-email-ard.biesheuvel@linaro.org> <2dd4294c-76f0-f433-cbd2-bf0b37114aee@redhat.com> In-Reply-To: <2dd4294c-76f0-f433-cbd2-bf0b37114aee@redhat.com> From: Ard Biesheuvel Date: Wed, 23 Jan 2019 10:26:41 +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: Wed, 23 Jan 2019 09:26:54 -0000 Content-Type: text/plain; charset="UTF-8" On Wed, 23 Jan 2019 at 10:14, Laszlo Ersek wrote: > > 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 !!!!!!!! > This is puzzling, given that the exact same binary works on Mustang. 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?