On Wed, 23 Feb 2022 07:02:28 +0000,
Ard Biesheuvel <a...@kernel.org> wrote:
>
> (+ Marc)
>
> On Tue, 22 Feb 2022 at 03:42, Ashish Singhal <ashishsin...@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 <ashishsin...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
