Re: [PATCH] ArmPkg: update InvalidateInstructionCacheRange to flush only to PoU

[Laszlo Ersek <lersek@redhat.com>]

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" <eugene@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 <eugene@hp.com>
>
> Added AARCH64 and ARM/GCC implementations of the above.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@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?

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.

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