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

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

On Mon, 28 Jan 2019 at 11:23, Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Wed, Jan 23, 2019 at 03:02:14PM +0100, Ard Biesheuvel wrote:
> > 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
>
> Assuming that hardware is working correctly, this change shouldn't be
> necessary.
>
> KVM sets HCR_EL2.FB, so all TLBI ops will behave as their *IS variant.
> Likewise it sets HCR_EL2.BSU, so barriers apply to the inner shareable domain too.
>
> On bare-metal, NSH should be sufficient.
>

Ah wonderful, thanks for clarifying.