From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web10.2318.1686154385245747905 for ; Wed, 07 Jun 2023 09:13:05 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=joE4ecmD; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: osde@linux.microsoft.com) Received: from [10.137.194.171] (unknown [131.107.1.171]) by linux.microsoft.com (Postfix) with ESMTPSA id 9116320BE4BA; Wed, 7 Jun 2023 09:12:56 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 9116320BE4BA DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1686154384; bh=1nFti9z70eD9lWYkitrM4RquwLd0PdjoTB7ftRS/pUs=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=joE4ecmDqFrCIrPeZFbdZufCtD1gaVXlOCAzcGpqhZqGDrabHbn4k+KEDYF7JjecQ Mcv6vJULJzZX/scu0csiRvgcmmaZB0A+wEQ2ZT8pvhIgXwnNCzxgW74IxblOzuD8/l 8k51Tq7MazrEPpwXUMexc5W8/u/YHBw0Rsd0LXCA= Message-ID: <3462b86b-24c5-fd46-8e47-3009b09a41cb@linux.microsoft.com> Date: Wed, 7 Jun 2023 09:12:56 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [edk2-devel][PATCH v1 1/1] ArmPkg: CpuDxe: Report AARCH64 Memory Protections Attributes To GCD To: Ard Biesheuvel Cc: devel@edk2.groups.io, Leif Lindholm , Ard Biesheuvel , Sami Mujawar , Taylor Beebe , Michael Kubacki , Sean Brogan References: <20230523203623.2387-1-osde@linux.microsoft.com> From: "Oliver Smith-Denny" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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. Thanks, Oliver On 5/23/2023 1:48 PM, Ard Biesheuvel wrote: > On Tue, 23 May 2023 at 22:36, 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 >> Cc: Ard Biesheuvel >> Cc: Sami Mujawar >> Cc: Taylor Beebe >> Cc: Michael Kubacki >> Cc: Sean Brogan >> >> Signed-off-by: Oliver Smith-Denny > > Thanks for the fix. > > Reviewed-by: Ard Biesheuvel > >> --- >> 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 >>