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

["Michael Kubacki" <mikuback@linux.microsoft.com>]

Reviewed-by: Michael Kubacki <michael.kubacki@microsoft.com>

On 5/23/2023 4:36 PM, Oliver Smith-Denny 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>
> ---
>   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) ||