public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [edk2-devel][PATCH v1 1/1] ArmPkg: CpuDxe: Report AARCH64 Memory Protections Attributes To GCD
@ 2023-05-23 20:36 Oliver Smith-Denny
  2023-05-23 20:48 ` Ard Biesheuvel
  2023-05-26 21:07 ` Michael Kubacki
  0 siblings, 2 replies; 6+ messages in thread
From: Oliver Smith-Denny @ 2023-05-23 20:36 UTC (permalink / raw)
  To: devel
  Cc: Leif Lindholm, Ard Biesheuvel, Sami Mujawar, Taylor Beebe,
	Michael Kubacki, Sean Brogan

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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [edk2-devel][PATCH v1 1/1] ArmPkg: CpuDxe: Report AARCH64 Memory Protections Attributes To GCD
  2023-05-23 20:36 [edk2-devel][PATCH v1 1/1] ArmPkg: CpuDxe: Report AARCH64 Memory Protections Attributes To GCD Oliver Smith-Denny
@ 2023-05-23 20:48 ` Ard Biesheuvel
  2023-06-07 16:12   ` Oliver Smith-Denny
  2023-05-26 21:07 ` Michael Kubacki
  1 sibling, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2023-05-23 20:48 UTC (permalink / raw)
  To: Oliver Smith-Denny
  Cc: devel, Leif Lindholm, Ard Biesheuvel, Sami Mujawar, Taylor Beebe,
	Michael Kubacki, Sean Brogan

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
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [edk2-devel][PATCH v1 1/1] ArmPkg: CpuDxe: Report AARCH64 Memory Protections Attributes To GCD
  2023-05-23 20:36 [edk2-devel][PATCH v1 1/1] ArmPkg: CpuDxe: Report AARCH64 Memory Protections Attributes To GCD Oliver Smith-Denny
  2023-05-23 20:48 ` Ard Biesheuvel
@ 2023-05-26 21:07 ` Michael Kubacki
  1 sibling, 0 replies; 6+ messages in thread
From: Michael Kubacki @ 2023-05-26 21:07 UTC (permalink / raw)
  To: Oliver Smith-Denny, devel
  Cc: Leif Lindholm, Ard Biesheuvel, Sami Mujawar, Taylor Beebe,
	Sean Brogan

Reviewed-by: Michael Kubacki <michael.kubacki@microsoft.com>

On 5/23/2023 4:36 PM, 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 <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>
> ---
>   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) ||

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [edk2-devel][PATCH v1 1/1] ArmPkg: CpuDxe: Report AARCH64 Memory Protections Attributes To GCD
  2023-05-23 20:48 ` Ard Biesheuvel
@ 2023-06-07 16:12   ` Oliver Smith-Denny
  2023-06-07 16:21     ` Ard Biesheuvel
  0 siblings, 1 reply; 6+ messages in thread
From: Oliver Smith-Denny @ 2023-06-07 16:12 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: devel, Leif Lindholm, Ard Biesheuvel, Sami Mujawar, Taylor Beebe,
	Michael Kubacki, Sean Brogan

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [edk2-devel][PATCH v1 1/1] ArmPkg: CpuDxe: Report AARCH64 Memory Protections Attributes To GCD
  2023-06-07 16:12   ` Oliver Smith-Denny
@ 2023-06-07 16:21     ` Ard Biesheuvel
  2023-06-07 16:36       ` Oliver Smith-Denny
  0 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2023-06-07 16:21 UTC (permalink / raw)
  To: Oliver Smith-Denny
  Cc: devel, Leif Lindholm, Ard Biesheuvel, Sami Mujawar, Taylor Beebe,
	Michael Kubacki, Sean Brogan

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [edk2-devel][PATCH v1 1/1] ArmPkg: CpuDxe: Report AARCH64 Memory Protections Attributes To GCD
  2023-06-07 16:21     ` Ard Biesheuvel
@ 2023-06-07 16:36       ` Oliver Smith-Denny
  0 siblings, 0 replies; 6+ messages in thread
From: Oliver Smith-Denny @ 2023-06-07 16:36 UTC (permalink / raw)
  To: devel, ardb
  Cc: Leif Lindholm, Ard Biesheuvel, Sami Mujawar, Taylor Beebe,
	Michael Kubacki, Sean Brogan

On 6/7/2023 9:21 AM, Ard Biesheuvel wrote:
> 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 :-)
> 

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-06-07 16:36 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-23 20:36 [edk2-devel][PATCH v1 1/1] ArmPkg: CpuDxe: Report AARCH64 Memory Protections Attributes To GCD Oliver Smith-Denny
2023-05-23 20:48 ` Ard Biesheuvel
2023-06-07 16:12   ` Oliver Smith-Denny
2023-06-07 16:21     ` Ard Biesheuvel
2023-06-07 16:36       ` Oliver Smith-Denny
2023-05-26 21:07 ` Michael Kubacki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox