[edk2-devel] [PATCH] ArmPkg: Invalidate Instruction Cache On MMU Enable

[Ard Biesheuvel]

(+ Marc)

On Tue, 22 Feb 2022 at 03:42, Ashish Singhal <ashishsingha at nvidia.com> wrote:
>
> Even with MMU turned off, instruction cache can speculate
> and fetch instructions. This can cause a crash if region
> being executed has been modified recently. With this patch,
> we ensure that instruction cache is invalidated right after
> MMU has been enabled and any potentially stale instruction
> fetched earlier has been discarded.
>

I don't follow this reasoning. Every time the UEFI image loader loads
an image into memory, it does so with MMU and caches enabled, and the
code regions are cleaned to the PoU before being invalidated in the
I-cache. So how could these regions be stale? The only code that needs
special care here is the little trampoline that executes with the MMU
off, but this is being taken care of explicitly, by cleaning the code
to the PoC before invoking it.

> This is specially helpful when the memory attributes of a
> region in MMU are being changed and some instructions
> operating on the region are prefetched in the instruction
> cache.
>

This sounds like a TLB problem not a cache problem. For the sake of
posterity, could you include a more detailed description of the issue
in the commit log? IIRC, this is about speculative instruction fetches
hitting device memory locations?

As I mentioned before, the architecture does not permit speculative
instruction fetches for regions mapped with the XN attribute.

Is the issue occurring under virtualization? Is there a stage 2
mapping that lacks the XN attribute for the device memory region in
question?

> Signed-off-by: Ashish Singhal <ashishsingha at nvidia.com>
> ---
>  ArmPkg/Library/ArmLib/AArch64/AArch64Support.S           | 4 +++-
>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S | 2 ++
>  2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
> index d3cc1e8671..9648245182 100644
> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
> @@ -89,7 +89,9 @@ ASM_FUNC(ArmEnableMmu)
>     dsb     nsh
>     isb
>     msr     sctlr_el3, x0       // Write back
> -4: isb
> +4: ic      iallu
> +   dsb     sy
> +   isb
>     ret
>
>
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
> index 66ebca571e..56cc2dd73f 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
> @@ -37,6 +37,8 @@
>
>    // re-enable the MMU
>    msr   sctlr_el\el, x8
> +  ic    iallu
> +  dsb   sy
>    isb
>    .endm
>
> --
> 2.17.1
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#86883): https://edk2.groups.io/g/devel/message/86883
Mute This Topic: https://groups.io/mt/89309504/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-