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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Wed, 23 Jan 2019 at 10:55, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 01/23/19 10:26, Ard Biesheuvel wrote:
> > On Wed, 23 Jan 2019 at 10:14, Laszlo Ersek <lersek@redhat.com> wrote:
> >> On 01/22/19 16:37, Ard Biesheuvel wrote:
>
> >>> 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.
>
> And even on the original (unspecified) hardware, the same binary works
> frequently. My understanding is that there are five VMs executing reboot
> loops in parallel, on the same host, and 4 out of 5 may hit the issue in
> a reasonable time period (300 reboots or so).
>
> > 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?
>
> Yes, we have
>
> ArmVirtPkg/ArmVirt.dsc.inc:
> gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC000000000007FD1
>
> from commit 1acd7c54a724 ("ArmVirtPkg AARCH64: enable NX memory
> protection for all platforms", 2017-03-01).
>
> The binary is from the RPM
> "edk2-aarch64-20180508gitee3198e672e2-5.el8+1789+f0947240.noarch", which
> is basically upstream ee3198e672e2 plus a small number of backports and
> downstream customizations.
>

This might help:

diff --git a/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
b/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
index b7173e00b039..4c0b4b4efbd5 100644
--- a/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
+++ b/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
@@ -138,7 +138,7 @@ ASM_FUNC(ArmUpdateTranslationTableEntry)

 ASM_FUNC(ArmInvalidateTlb)
    EL1_OR_EL2_OR_EL3(x0)
-1: tlbi  vmalle1
+1: tlbi  vmalle1is
    b     4f
 2: tlbi  alle2
    b     4f
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
index 90192df24f55..d54b1c19accf 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
@@ -34,7 +34,7 @@

   // flush the TLBs
   .if   \el == 1
-  tlbi  vmalle1
+  tlbi  vmalle1is
   .else
   tlbi  alle\el
   .endif