Re: [edk2-devel][PATCH v1 1/1] ArmPkg: CpuDxe: Report AARCH64 Memory Protections Attributes To GCD

["Oliver Smith-Denny" <osde@linux.microsoft.com>]

On 6/7/2023 9:21 AM, Ard Biesheuvel wrote:
> On Wed, 7 Jun 2023 at 18:13, Oliver Smith-Denny
> <osde@linux.microsoft.com> wrote:
>>
>> Hi Ard,
>>
>> Thanks for the review. Looks like there isn't much conversation
>> on this one, just reviewed-by's from Michael and you. Can this be
>> merged? It is ideal for this to go in before the GCD sync patchset,
>> because without this patch, the GCD sync will show all memory as
>> EFI_MEMORY_RP.
>>
> 
> I already attempted to merge this iirc, but whether it got merged in
> the end is anyone's guess :-)
> 

:). Thanks Ard, I checked it has been merged.

>>
>> On 5/23/2023 1:48 PM, Ard Biesheuvel wrote:
>>> On Tue, 23 May 2023 at 22:36, Oliver Smith-Denny
>>> <osde@linux.microsoft.com> wrote:
>>>>
>>>> When the AARCH64 CpuDxe attempts to SyncCacheConfig() with the GCD,
>>>> it collects the page attributes as:
>>>>
>>>> EntryAttribute = Entry & TT_ATTR_INDX_MASK
>>>>
>>>> However, TT_ATTR_INDX_MASK only masks the cacheability attributes
>>>> and drops the memory protections attributes. Importantly, it also
>>>> drops the TT_AF (access flag) which is now wired up in EDK2 to
>>>> represent EFI_MEMORY_RP, so by default all SystemMem pages will
>>>> report as EFI_MEMORY_RP to the GCD. The GCD currently drops that
>>>> silently, because the Capabilities field in the GCD does not support
>>>> EFI_MEMORY_RP by default.
>>>>
>>>> However, some ranges may support EFI_MEMORY_RP and incorrectly
>>>> mark those ranges as read protected. In conjunction with
>>>> another change on the mailing list (see:
>>>> https://edk2.groups.io/g/devel/topic/98505340#:~:text=This%20patch%20follows%20the%20UefiCpuPkg%20pattern%20and%20adds%3D0D,between%20the%20GCD%20and%20the%20page%20table.%3D0D%20%3D0D),
>>>> this causes an access flag fault incorrectly. See the linked
>>>> BZ below for full details.
>>>>
>>>> This patch exposes all memory protections attributes to the GCD
>>>> layer so it can correctly set pages as EFI_MEMORY[RP|XP|RO] when
>>>> it initially syncs.
>>>>
>>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4463
>>>> Personal GitHub PR: https://github.com/tianocore/edk2/pull/4423
>>>> Github branch: https://github.com/os-d/edk2/tree/aarch64_report_af_v1
>>>>
>>>> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
>>>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>>>> Cc: Sami Mujawar <sami.mujawar@arm.com>
>>>> Cc: Taylor Beebe <t@taylorbeebe.com>
>>>> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
>>>> Cc: Sean Brogan <sean.brogan@microsoft.com>
>>>>
>>>> Signed-off-by: Oliver Smith-Denny <osde@linux.microsoft.com>
>>>
>>> Thanks for the fix.
>>>
>>> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
>>>
>>>> ---
>>>>    ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
>>>> index 0859c7418a1f..1d02e41e18d8 100644
>>>> --- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
>>>> +++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
>>>> @@ -120,7 +120,7 @@ GetFirstPageAttribute (
>>>>      } else if (((FirstEntry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY) ||
>>>>                 ((TableLevel == 3) && ((FirstEntry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY_LEVEL3)))
>>>>      {
>>>> -    return FirstEntry & TT_ATTR_INDX_MASK;
>>>> +    return FirstEntry & TT_ATTRIBUTES_MASK;
>>>>      } else {
>>>>        return INVALID_ENTRY;
>>>>      }
>>>> @@ -158,7 +158,7 @@ GetNextEntryAttribute (
>>>>      for (Index = 0; Index < EntryCount; Index++) {
>>>>        Entry          = TableAddress[Index];
>>>>        EntryType      = Entry & TT_TYPE_MASK;
>>>> -    EntryAttribute = Entry  & TT_ATTR_INDX_MASK;
>>>> +    EntryAttribute = Entry & TT_ATTRIBUTES_MASK;
>>>>
>>>>        // If Entry is a Table Descriptor type entry then go through the sub-level table
>>>>        if ((EntryType == TT_TYPE_BLOCK_ENTRY) ||
>>>> --
>>>> 2.40.1
>>>>
> 
> 
> 
>