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

[Marc Zyngier]

On Wed, 23 Feb 2022 07:02:28 +0000,
Ard Biesheuvel <ardb at kernel.org> wrote:
> 
> (+ 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.

Are you doing code patching while the MMU is off?

> >
> 
> 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.
> >

Huh, this is way too vague. How do you expect anyone to understand
your problem based on this?

> 
> 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

Why DSB SY? Given that you are only invalidating on a single CPU,
DSB NSH should be enough.

> > +   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

Same thing here.

	M.

-- 
Without deviation from the norm, progress is not possible.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#86946): https://edk2.groups.io/g/devel/message/86946
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]
-=-=-=-=-=-=-=-=-=-=-=-