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

[Laszlo Ersek <lersek@redhat.com>]

On 01/23/19 15:02, 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
> 

"Invalidate all EL1&0 regime stage 1 TLB entries for the current VMID
>>>on all PEs in the same Inner Shareable domain<<<".

I'll soon try to build a new pkg with this applied, for testing by the
reporter. Many thanks!

Laszlo