[edk2-devel] [PATCH v1 0/2] ArmPkg/CpuDxe: Use upper and lower attributes

[edk2-devel] [PATCH v1 0/2] ArmPkg/CpuDxe: Use upper and lower attributes

[Michael Kubacki]

From: Michael Kubacki <michael.kubacki@microsoft.com>

This series contains two changes:

1. To fix a compiler warning with the current state of code.
2. To update the code to pass the integer width needed for a
   comparison to set EFI_MEMORY_XP in the GCD attribute returned
   for a given page attribute.

Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>

Michael Kubacki (2):
  ArmPkg/Drivers/CpuDxe: Explicitly cast table entry
  ArmPkg/Drivers/CpuDxe: Use lower and upper attributes

 ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
2.42.0.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111766): https://edk2.groups.io/g/devel/message/111766
Mute This Topic: https://groups.io/mt/102841999/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 1/2] ArmPkg/Drivers/CpuDxe: Explicitly cast table entry

[Michael Kubacki]

From: Michael Kubacki <michael.kubacki@microsoft.com>

GetNextEntryAttribute() assigns a 64-bit integer to 32-bit integers.

This change explicitly casts the assigned value as UINT32 to prevent
the following Visual Studio compiler warning:

  '=': conversion from 'UINT64' to 'UINT32', possible loss of data

Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Michael Kubacki <michael.kubacki@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 e14eb47ce4c6..4555fdb5c2c8 100644
--- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
@@ -195,8 +195,8 @@ GetNextEntryAttribute (
   // the subsequent ones should be filled up
   for (Index = 0; Index < EntryCount; Index++) {
     Entry          = TableAddress[Index];
-    EntryType      = Entry & TT_TYPE_MASK;
-    EntryAttribute = Entry & TT_ATTRIBUTES_MASK;
+    EntryType      = (UINT32)(Entry & TT_TYPE_MASK);
+    EntryAttribute = (UINT32)(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.42.0.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111767): https://edk2.groups.io/g/devel/message/111767
Mute This Topic: https://groups.io/mt/102842002/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 2/2] ArmPkg/Drivers/CpuDxe: Use lower and upper attributes

[Michael Kubacki]

From: Michael Kubacki <michael.kubacki@microsoft.com>

GetNextEntryAttribute() is currently applying a 64-bit mask
(TT_ATTRIBUTES_MASK) to a 32-bit descriptor value (EntryType).
The original descriptor was 64 bits containing the upper and
lower attributes which are included in TT_ATTRIBUTES_MASK.

The PrevEntryAttribute parameter is also a UINT32, but passed to
PageAttributeToGcdAttribute() for a UINT64 parameter where the
function checks masks in the upper 32 bits of the integer value:

  PageAttributeToGcdAttribute (*PrevEntryAttribute)
  ...
  STATIC
  UINT64
  PageAttributeToGcdAttribute (
    IN UINT64  PageAttributes
    )
  ...
  if ((PageAttributes & (TT_PXN_MASK | TT_UXN_MASK)) != 0) {
    GcdAttributes |= EFI_MEMORY_XP;
  }
  ...
  #define TT_PXN_MASK  BIT53
  #define TT_UXN_MASK  BIT54  // EL1&0

This change removes UINT32 intermediary values. For EntryType,
eliminating an unncessary cast. For EntryAttribute, preserving the
upper and lower attributes for evaluation in
PageAttributeToGcdAttribute().

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
 ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
index 4555fdb5c2c8..ff14c2f814b2 100644
--- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
@@ -13,7 +13,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Library/MemoryAllocationLib.h>
 #include "CpuDxe.h"
 
-#define INVALID_ENTRY  ((UINT32)~0)
+#define INVALID_ENTRY  ((UINT64)~0)
 
 #define MIN_T0SZ        16
 #define BITS_PER_LEVEL  9
@@ -169,14 +169,14 @@ GetNextEntryAttribute (
   IN     UINTN   EntryCount,
   IN     UINTN   TableLevel,
   IN     UINT64  BaseAddress,
-  IN OUT UINT32  *PrevEntryAttribute,
+  IN OUT UINT64  *PrevEntryAttribute,
   IN OUT UINT64  *StartGcdRegion
   )
 {
   UINTN                            Index;
   UINT64                           Entry;
-  UINT32                           EntryAttribute;
-  UINT32                           EntryType;
+  UINT64                           EntryAttribute;
+  UINT64                           EntryType;
   EFI_STATUS                       Status;
   UINTN                            NumberOfDescriptors;
   EFI_GCD_MEMORY_SPACE_DESCRIPTOR  *MemorySpaceMap;
@@ -195,8 +195,8 @@ GetNextEntryAttribute (
   // the subsequent ones should be filled up
   for (Index = 0; Index < EntryCount; Index++) {
     Entry          = TableAddress[Index];
-    EntryType      = (UINT32)(Entry & TT_TYPE_MASK);
-    EntryAttribute = (UINT32)(Entry & TT_ATTRIBUTES_MASK);
+    EntryType      = Entry & TT_TYPE_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) ||
@@ -271,7 +271,7 @@ SyncCacheConfig (
   )
 {
   EFI_STATUS                       Status;
-  UINT32                           PageAttribute;
+  UINT64                           PageAttribute;
   UINT64                           *FirstLevelTableAddress;
   UINTN                            TableLevel;
   UINTN                            TableCount;
-- 
2.42.0.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111768): https://edk2.groups.io/g/devel/message/111768
Mute This Topic: https://groups.io/mt/102842005/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/2] ArmPkg/CpuDxe: Use upper and lower attributes

[Ard Biesheuvel]

On Tue, 28 Nov 2023 at 01:15, <mikuback@linux.microsoft.com> wrote:
>
> From: Michael Kubacki <michael.kubacki@microsoft.com>
>
> This series contains two changes:
>
> 1. To fix a compiler warning with the current state of code.
> 2. To update the code to pass the integer width needed for a
>    comparison to set EFI_MEMORY_XP in the GCD attribute returned
>    for a given page attribute.
>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
>
> Michael Kubacki (2):
>   ArmPkg/Drivers/CpuDxe: Explicitly cast table entry
>   ArmPkg/Drivers/CpuDxe: Use lower and upper attributes
>

Thanks for the fixes. This code is in a rather poor state, unfortunately.

I don't quite get the motivation for fixing this using two different
patches: the implicit UINT32 cast obviously loses some attributes (the
ones in the 12 upper bits, notably UXN and PXN), so making it explicit
removes the warning but preserves the bug. (If GCC had better
diagnostics, we'd spotted this problem years ago)

Maybe it is sufficient to simply squash the two patches together?


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111803): https://edk2.groups.io/g/devel/message/111803
Mute This Topic: https://groups.io/mt/102841999/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/2] ArmPkg/CpuDxe: Use upper and lower attributes

[Michael Kubacki]

On 11/28/2023 4:51 AM, Ard Biesheuvel wrote:
> On Tue, 28 Nov 2023 at 01:15, <mikuback@linux.microsoft.com> wrote:
>>
>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>
>> This series contains two changes:
>>
>> 1. To fix a compiler warning with the current state of code.
>> 2. To update the code to pass the integer width needed for a
>>     comparison to set EFI_MEMORY_XP in the GCD attribute returned
>>     for a given page attribute.
>>
>> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Sami Mujawar <sami.mujawar@arm.com>
>>
>> Michael Kubacki (2):
>>    ArmPkg/Drivers/CpuDxe: Explicitly cast table entry
>>    ArmPkg/Drivers/CpuDxe: Use lower and upper attributes
>>
> 
> Thanks for the fixes. This code is in a rather poor state, unfortunately.
> 
> I don't quite get the motivation for fixing this using two different
> patches: the implicit UINT32 cast obviously loses some attributes (the
> ones in the 12 upper bits, notably UXN and PXN), so making it explicit
> removes the warning but preserves the bug. (If GCC had better
> diagnostics, we'd spotted this problem years ago)
> 
> Maybe it is sufficient to simply squash the two patches together?
> 
I wanted to leave that open to discussion and maintainer discretion. I 
think a squash is fine and sent a v2 with it done.

> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111816): https://edk2.groups.io/g/devel/message/111816
Mute This Topic: https://groups.io/mt/102841999/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/2] ArmPkg/CpuDxe: Use upper and lower attributes

[Ard Biesheuvel]

On Tue, 28 Nov 2023 at 18:15, Michael Kubacki
<mikuback@linux.microsoft.com> wrote:
>
> On 11/28/2023 4:51 AM, Ard Biesheuvel wrote:
> > On Tue, 28 Nov 2023 at 01:15, <mikuback@linux.microsoft.com> wrote:
> >>
> >> From: Michael Kubacki <michael.kubacki@microsoft.com>
> >>
> >> This series contains two changes:
> >>
> >> 1. To fix a compiler warning with the current state of code.
> >> 2. To update the code to pass the integer width needed for a
> >>     comparison to set EFI_MEMORY_XP in the GCD attribute returned
> >>     for a given page attribute.
> >>
> >> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> >> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> >> Cc: Sami Mujawar <sami.mujawar@arm.com>
> >>
> >> Michael Kubacki (2):
> >>    ArmPkg/Drivers/CpuDxe: Explicitly cast table entry
> >>    ArmPkg/Drivers/CpuDxe: Use lower and upper attributes
> >>
> >
> > Thanks for the fixes. This code is in a rather poor state, unfortunately.
> >
> > I don't quite get the motivation for fixing this using two different
> > patches: the implicit UINT32 cast obviously loses some attributes (the
> > ones in the 12 upper bits, notably UXN and PXN), so making it explicit
> > removes the warning but preserves the bug. (If GCC had better
> > diagnostics, we'd spotted this problem years ago)
> >
> > Maybe it is sufficient to simply squash the two patches together?
> >
> I wanted to leave that open to discussion and maintainer discretion. I
> think a squash is fine and sent a v2 with it done.
>

Thanks. I'll go and merge that.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111818): https://edk2.groups.io/g/devel/message/111818
Mute This Topic: https://groups.io/mt/102841999/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-