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

["Ard Biesheuvel" <ardb@kernel.org>]

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 :-)

>
> 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
> >>