From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web11.2578.1686154915700103834 for ; Wed, 07 Jun 2023 09:21:55 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=qQP0xRcJ; spf=pass (domain: kernel.org, ip: 139.178.84.217, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id ED959638E8 for ; Wed, 7 Jun 2023 16:21:54 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 5F55FC433EF for ; Wed, 7 Jun 2023 16:21:54 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1686154914; bh=hAUZsOB0Wa3l9wNIMykIs6wO3s+4zwr8mKVLeLeRifo=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=qQP0xRcJBue8y1RL1xSqFR0wNS9av5AEp0tr7bAb8iO9tPUkGWWOfAgbBJcw3YZJx k+WFX9lLlqd6+T9DoL2EfEno76iTS8KsGOSDRz3U3TJiQtVLjDUSaKg6d4jm/0TAQW JnVj7Z2Kf1MHNf6OEDd50pPwunLuaSTGXDEvuX2nyhzrd9dAp4o2bydZP9D5Ffiq99 s3JA6jgvRBqzcKBhlRx3mVnenRnXa4dcfRQ6SWilKr5XWXaPhoQ3a2ReQVb1JbZbby wUp76WHoYdcVD64KGgOOfZ0fT8NkoDF1irCoAsVHPwNVJY610oMOhpJiGpC9CnAtdw W83YvQtFrUCSg== Received: by mail-lf1-f54.google.com with SMTP id 2adb3069b0e04-4f620583bc2so5464893e87.1 for ; Wed, 07 Jun 2023 09:21:54 -0700 (PDT) X-Gm-Message-State: AC+VfDxEpvrGkB16xTmzrgR/0MoosX9TZXpz30zH+Jmw0sJbr5jRr0LM uosHWMAawQHPG72+gQE9yMT4ZZqrUPVyk/E+cQw= X-Google-Smtp-Source: ACHHUZ5tISN9HE0If2JSUjMm990057fVuZOdPH1r7dtol5gwmnxaAylsFZGVJXHAXsHHCHg7nHoH86s7tCtAOcE5Fhg= X-Received: by 2002:ac2:5445:0:b0:4f1:26f5:77fb with SMTP id d5-20020ac25445000000b004f126f577fbmr1750085lfn.28.1686154912362; Wed, 07 Jun 2023 09:21:52 -0700 (PDT) MIME-Version: 1.0 References: <20230523203623.2387-1-osde@linux.microsoft.com> <3462b86b-24c5-fd46-8e47-3009b09a41cb@linux.microsoft.com> In-Reply-To: <3462b86b-24c5-fd46-8e47-3009b09a41cb@linux.microsoft.com> From: "Ard Biesheuvel" Date: Wed, 7 Jun 2023 18:21:40 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel][PATCH v1 1/1] ArmPkg: CpuDxe: Report AARCH64 Memory Protections Attributes To GCD To: Oliver Smith-Denny Cc: devel@edk2.groups.io, Leif Lindholm , Ard Biesheuvel , Sami Mujawar , Taylor Beebe , Michael Kubacki , Sean Brogan Content-Type: text/plain; charset="UTF-8" On Wed, 7 Jun 2023 at 18:13, Oliver Smith-Denny 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 > > 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 > >>