On Wed, 23 Jan 2019 at 10:14, Laszlo Ersek <ler...@redhat.com> wrote:
>
> On 01/22/19 16:37, Ard Biesheuvel wrote:
> > On Tue, 22 Jan 2019 at 16:09, Laszlo Ersek <ler...@redhat.com> 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" <eug...@hp.com>
> >>>
> >>> 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 <eug...@hp.com>
> >>>
> >>> Added AARCH64 and ARM/GCC implementations of the above.
> >>>
> >>> Contributed-under: TianoCore Contribution Agreement 1.0
> >>> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
> >>> ---
> >>> 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
> >> <https://bugzilla.redhat.com/show_bug.cgi?id=1666280> -- 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?
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
