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.web11.3019.1686155814757054508 for ; Wed, 07 Jun 2023 09:36:54 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=Kqiy8u/+; 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 2BA6C20BE4BA; Wed, 7 Jun 2023 09:36:54 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 2BA6C20BE4BA DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1686155814; bh=xV6I3cEgv5108Ev5xMDf5dSZ0pt1psOdICuKV9PP03c=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=Kqiy8u/+eSD4nncupNo2f8sLbMA6IYklkqUn0w4yXWQtqUSW6dMqnCFvoKeyxynpw LYhJjMWgFz1STQin1D4IRf3U04rmY+2O7pX+oEkacypYA5kasDvyL9I8qAGTmepE26 2ulsPnWAUv5ZYdlleF8lkDtAhmx1ECEaY7wAORCA= Message-ID: <1f6f168e-f70a-f827-271f-a80605312f64@linux.microsoft.com> Date: Wed, 7 Jun 2023 09:36:53 -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: devel@edk2.groups.io, ardb@kernel.org Cc: Leif Lindholm , Ard Biesheuvel , Sami Mujawar , Taylor Beebe , Michael Kubacki , Sean Brogan References: <20230523203623.2387-1-osde@linux.microsoft.com> <3462b86b-24c5-fd46-8e47-3009b09a41cb@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 On 6/7/2023 9:21 AM, Ard Biesheuvel wrote: > 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 :-) > :). Thanks Ard, I checked it has been merged. >> >> 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 >>>> > > > >