On Wed, 7 Jun 2023 at 18:13, Oliver Smith-Denny <o...@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 > > <o...@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_llind...@quicinc.com> > >> Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org> > >> Cc: Sami Mujawar <sami.muja...@arm.com> > >> Cc: Taylor Beebe <t...@taylorbeebe.com> > >> Cc: Michael Kubacki <mikub...@linux.microsoft.com> > >> Cc: Sean Brogan <sean.bro...@microsoft.com> > >> > >> Signed-off-by: Oliver Smith-Denny <o...@linux.microsoft.com> > > > > Thanks for the fix. > > > > Reviewed-by: Ard Biesheuvel <a...@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 > >> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105872): https://edk2.groups.io/g/devel/message/105872 Mute This Topic: https://groups.io/mt/99095992/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-