[PATCH 0/9] ArmPkg: Start cleaning up ID register handling

[PATCH 0/9] ArmPkg: Start cleaning up ID register handling

[Leif Lindholm]

As discussed in https://edk2.groups.io/g/devel/topic/78784065,
the ARM and AArch64 ports were using an unfortunate overloading
of accessor functions for the ID registers which describe the
presence and versions of features implemented in a CPU.

This makes no sense, since the layout, and indeed the names of
the registers for the different architectural states differ.

This set replaces the use of the current accessor functions
with some helper functions in ArmLib. It also moves the
accessor function prototypes into private headers so they
can no longer be called from outside ArmLib

As a pure side effect, we're able to throw away half of
ArmGicArchLib/ArmGicArchSecLib.

Note that some additional cleanup can also be done in the ARM
portion of ArmMmuLib (which implements its own direct accessor
for ID_MMFR0), and internally in ArmLib where some assembly
code does its own feature checks, which could be moved to C
helpers.

Furthermore, it might be useful to move the ID register field
definitions to a private header.

Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>

Leif Lindholm (9):
  ArmPkg/ArmLib: add ArmHasGicSystemRegisters () helper function
  ArmPkg: use ID register helper for ArmGicArch(Sec)Lib
  ArmPkg: remove duplicated ARM/AArch64 ArmGicArchLib sources
  ArmPkg: remove duplicated ARM/AArch64 ArmGicArchSecLib sources
  ArmPkg: add ArmHasSecurityExtensions () helper function
  ArmPkg: use helper to check for Security extensions in ArmArchTimerLib
  ArmPkg/ArmLib: delete AArch64 version of ArmReadIdPfr1
  ArmPkg/ArmLib: rename AArch64 variant of ArmReadIdPfr0
  ArmPkg/ArmLib: move ArmReadIdPfr0/1 into private header ArmV7Lib.h

 ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf                |  7 +--
 ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf          |  7 +--
 ArmPkg/Include/Library/ArmLib.h                               | 47 +++++++++++----
 ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h                    |  6 ++
 ArmPkg/Library/ArmLib/Arm/ArmV7Lib.h                          | 12 ++++
 ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c              |  2 +-
 ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c              | 60 --------------------
 ArmPkg/Library/ArmGicArchLib/{AArch64 => }/ArmGicArchLib.c    |  2 +-
 ArmPkg/Library/ArmGicArchSecLib/Arm/ArmGicArchLib.c           | 45 ---------------
 ArmPkg/Library/ArmGicArchSecLib/{AArch64 => }/ArmGicArchLib.c |  2 +-
 ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c                    | 16 ++++++
 ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c                          | 31 ++++++++++
 ArmPkg/Library/ArmLib/AArch64/AArch64Support.S                | 13 +----
 13 files changed, 110 insertions(+), 140 deletions(-)
 delete mode 100644 ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
 rename ArmPkg/Library/ArmGicArchLib/{AArch64 => }/ArmGicArchLib.c (94%)
 delete mode 100644 ArmPkg/Library/ArmGicArchSecLib/Arm/ArmGicArchLib.c
 rename ArmPkg/Library/ArmGicArchSecLib/{AArch64 => }/ArmGicArchLib.c (94%)

-- 
2.20.1

[PATCH 1/9] ArmPkg/ArmLib: add ArmHasGicSystemRegisters () helper function

[Leif Lindholm]

Create a helper function to eliminate direct feature register reading,
which gets messy in code shared between ARM/AArch64.

Returns BOOLEAN True if the CPU implements the GIC System Register
Interface (any version), otherwise returns BOOL False.

Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
 ArmPkg/Include/Library/ArmLib.h            | 18 ++++++++++++++++++
 ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c | 16 ++++++++++++++++
 ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c       | 16 ++++++++++++++++
 3 files changed, 50 insertions(+)

diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h
index 5a27b7c2fc27..8a364f2ca96f 100644
--- a/ArmPkg/Include/Library/ArmLib.h
+++ b/ArmPkg/Include/Library/ArmLib.h
@@ -2,6 +2,7 @@
 
   Copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
   Copyright (c) 2011 - 2016, ARM Ltd. All rights reserved.<BR>
+  Copyright (c) 2020, NUVIA Inc. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -715,4 +716,21 @@ ArmGetPhysicalAddressBits (
   VOID
   );
 
+
+///
+///  ID Register Helper functions
+///
+
+/**
+  Check whether the CPU supports the GIC system register interface (any version)
+
+  @return   Whether GIC System Register Interface is supported
+
+**/
+BOOLEAN
+EFIAPI
+ArmHasGicSystemRegisters (
+  VOID
+  );
+
 #endif // __ARM_LIB__
diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
index 3fbd591192e2..5b10eb33c97d 100644
--- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
+++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
@@ -2,6 +2,7 @@
 
   Copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
   Portions copyright (c) 2011 - 2014, ARM Ltd. All rights reserved.<BR>
+  Copyright (c) 2020, NUVIA Inc. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -71,3 +72,18 @@ ArmCleanDataCache (
   ArmDataSynchronizationBarrier ();
   AArch64DataCacheOperation (ArmCleanDataCacheEntryBySetWay);
 }
+
+/**
+  Check whether the CPU supports the GIC system register interface (any version)
+
+  @return   Whether GIC System Register Interface is supported
+
+**/
+BOOLEAN
+EFIAPI
+ArmHasGicSystemRegisters (
+  VOID
+  )
+{
+  return ((ArmReadIdPfr0 () & AARCH64_PFR0_GIC) != 0);
+}
diff --git a/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c b/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c
index 2c4a23e1a1b2..3faada3a6539 100644
--- a/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c
+++ b/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c
@@ -2,6 +2,7 @@
 
   Copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
   Copyright (c) 2011 - 2014, ARM Limited. All rights reserved.
+  Copyright (c) 2020, NUVIA Inc. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -71,3 +72,18 @@ ArmCleanDataCache (
   ArmDataSynchronizationBarrier ();
   ArmV7DataCacheOperation (ArmCleanDataCacheEntryBySetWay);
 }
+
+/**
+  Check whether the CPU supports the GIC system register interface (any version)
+
+  @return   Whether GIC System Register Interface is supported
+
+**/
+BOOLEAN
+EFIAPI
+ArmHasGicSystemRegisters (
+  VOID
+  )
+{
+  return ((ArmReadIdPfr1 () & ARM_PFR1_GIC) != 0);
+}
-- 
2.20.1

[PATCH 2/9] ArmPkg: use ID register helper for ArmGicArch(Sec)Lib

[Leif Lindholm]

Use ArmHasGicSystemRegisters () instead of direct ID register tests.

Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
 ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c    | 2 +-
 ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c        | 2 +-
 ArmPkg/Library/ArmGicArchSecLib/AArch64/ArmGicArchLib.c | 2 +-
 ArmPkg/Library/ArmGicArchSecLib/Arm/ArmGicArchLib.c     | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
index 4086a294dafd..6fd69658e0e5 100644
--- a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
+++ b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
@@ -25,7 +25,7 @@ ArmGicArchLibInitialize (
   // feature is implemented on the CPU. This is also convenient as our GICv3
   // driver requires SRE. If only Memory mapped access is available we try to
   // drive the GIC as a v2.
-  if (ArmReadIdPfr0 () & AARCH64_PFR0_GIC) {
+  if (ArmHasGicSystemRegisters ()) {
     // Make sure System Register access is enabled (SRE). This depends on the
     // higher privilege level giving us permission, otherwise we will either
     // cause an exception here, or the write doesn't stick in which case we need
diff --git a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
index 222d8059825d..7e7e46e69faa 100644
--- a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
+++ b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
@@ -25,7 +25,7 @@ ArmGicArchLibInitialize (
   // feature is implemented on the CPU. This is also convenient as our GICv3
   // driver requires SRE. If only Memory mapped access is available we try to
   // drive the GIC as a v2.
-  if (ArmReadIdPfr1 () & ARM_PFR1_GIC) {
+  if (ArmHasGicSystemRegisters ()) {
     // Make sure System Register access is enabled (SRE). This depends on the
     // higher privilege level giving us permission, otherwise we will either
     // cause an exception here, or the write doesn't stick in which case we need
diff --git a/ArmPkg/Library/ArmGicArchSecLib/AArch64/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchSecLib/AArch64/ArmGicArchLib.c
index 4f2479e70c74..ca81951b2b2b 100644
--- a/ArmPkg/Library/ArmGicArchSecLib/AArch64/ArmGicArchLib.c
+++ b/ArmPkg/Library/ArmGicArchSecLib/AArch64/ArmGicArchLib.c
@@ -23,7 +23,7 @@ ArmGicGetSupportedArchRevision (
   // feature is implemented on the CPU. This is also convenient as our GICv3
   // driver requires SRE. If only Memory mapped access is available we try to
   // drive the GIC as a v2.
-  if (ArmReadIdPfr0 () & AARCH64_PFR0_GIC) {
+  if (ArmHasGicSystemRegisters ()) {
     // Make sure System Register access is enabled (SRE). This depends on the
     // higher privilege level giving us permission, otherwise we will either
     // cause an exception here, or the write doesn't stick in which case we need
diff --git a/ArmPkg/Library/ArmGicArchSecLib/Arm/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchSecLib/Arm/ArmGicArchLib.c
index 8e1baeee201a..2373ca409a17 100644
--- a/ArmPkg/Library/ArmGicArchSecLib/Arm/ArmGicArchLib.c
+++ b/ArmPkg/Library/ArmGicArchSecLib/Arm/ArmGicArchLib.c
@@ -23,7 +23,7 @@ ArmGicGetSupportedArchRevision (
   // feature is implemented on the CPU. This is also convenient as our GICv3
   // driver requires SRE. If only Memory mapped access is available we try to
   // drive the GIC as a v2.
-  if (ArmReadIdPfr1 () & ARM_PFR1_GIC) {
+  if (ArmHasGicSystemRegisters ()) {
     // Make sure System Register access is enabled (SRE). This depends on the
     // higher privilege level giving us permission, otherwise we will either
     // cause an exception here, or the write doesn't stick in which case we need
-- 
2.20.1

[PATCH 3/9] ArmPkg: remove duplicated ARM/AArch64 ArmGicArchLib sources

[Leif Lindholm]

The ID register access was the only difference between them, so
after switching to the ArmHasGicSystemRegisters () helper, there
is no longer any need to have separate ARM/AArch64 source files
for ArmGicArchLib, so unify them and drop the subdirectories.

Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
 ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf             |  7 +--
 ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c           | 60 --------------------
 ArmPkg/Library/ArmGicArchLib/{AArch64 => }/ArmGicArchLib.c |  0
 3 files changed, 2 insertions(+), 65 deletions(-)

diff --git a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
index 92ac11c2f5e5..bedddff93955 100644
--- a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
+++ b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
@@ -14,11 +14,8 @@ [Defines]
   LIBRARY_CLASS                  = ArmGicArchLib|DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION
   CONSTRUCTOR                    = ArmGicArchLibInitialize
 
-[Sources.ARM]
-  Arm/ArmGicArchLib.c
-
-[Sources.AARCH64]
-  AArch64/ArmGicArchLib.c
+[Sources]
+  ArmGicArchLib.c
 
 [Packages]
   MdePkg/MdePkg.dec
diff --git a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
deleted file mode 100644
index 7e7e46e69faa..000000000000
--- a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
+++ /dev/null
@@ -1,60 +0,0 @@
-/** @file
-*
-*  Copyright (c) 2014, ARM Limited. All rights reserved.
-*
-*  SPDX-License-Identifier: BSD-2-Clause-Patent
-*
-**/
-
-#include <Library/ArmLib.h>
-#include <Library/ArmGicLib.h>
-
-STATIC ARM_GIC_ARCH_REVISION        mGicArchRevision;
-
-RETURN_STATUS
-EFIAPI
-ArmGicArchLibInitialize (
-  VOID
-  )
-{
-  UINT32    IccSre;
-
-  // Ideally we would like to use the GICC IIDR Architecture version here, but
-  // this does not seem to be very reliable as the implementation could easily
-  // get it wrong. It is more reliable to check if the GICv3 System Register
-  // feature is implemented on the CPU. This is also convenient as our GICv3
-  // driver requires SRE. If only Memory mapped access is available we try to
-  // drive the GIC as a v2.
-  if (ArmHasGicSystemRegisters ()) {
-    // Make sure System Register access is enabled (SRE). This depends on the
-    // higher privilege level giving us permission, otherwise we will either
-    // cause an exception here, or the write doesn't stick in which case we need
-    // to fall back to the GICv2 MMIO interface.
-    // Note: We do not need to set ICC_SRE_EL2.Enable because the OS is started
-    // at the same exception level.
-    // It is the OS responsibility to set this bit.
-    IccSre = ArmGicV3GetControlSystemRegisterEnable ();
-    if (!(IccSre & ICC_SRE_EL2_SRE)) {
-      ArmGicV3SetControlSystemRegisterEnable (IccSre| ICC_SRE_EL2_SRE);
-      IccSre = ArmGicV3GetControlSystemRegisterEnable ();
-    }
-    if (IccSre & ICC_SRE_EL2_SRE) {
-      mGicArchRevision = ARM_GIC_ARCH_REVISION_3;
-      goto Done;
-    }
-  }
-
-  mGicArchRevision = ARM_GIC_ARCH_REVISION_2;
-
-Done:
-  return RETURN_SUCCESS;
-}
-
-ARM_GIC_ARCH_REVISION
-EFIAPI
-ArmGicGetSupportedArchRevision (
-  VOID
-  )
-{
-  return mGicArchRevision;
-}
diff --git a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.c
similarity index 100%
rename from ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
rename to ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.c
-- 
2.20.1

[PATCH 4/9] ArmPkg: remove duplicated ARM/AArch64 ArmGicArchSecLib sources

[Leif Lindholm]

The ID register access was the only difference between them, so
after switching to the ArmHasGicSystemRegisters () helper, there
is no longer any need to have separate ARM/AArch64 source files
for ArmGicArchSecLib, so unify them and drop the subdirectories.

Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
 ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf          |  7 +--
 ArmPkg/Library/ArmGicArchSecLib/Arm/ArmGicArchLib.c           | 45 --------------------
 ArmPkg/Library/ArmGicArchSecLib/{AArch64 => }/ArmGicArchLib.c |  0
 3 files changed, 2 insertions(+), 50 deletions(-)

diff --git a/ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf b/ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
index 1a1179a98012..ccebabe451cc 100644
--- a/ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
+++ b/ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
@@ -13,11 +13,8 @@ [Defines]
   VERSION_STRING                 = 1.0
   LIBRARY_CLASS                  = ArmGicArchLib|SEC
 
-[Sources.ARM]
-  Arm/ArmGicArchLib.c
-
-[Sources.AARCH64]
-  AArch64/ArmGicArchLib.c
+[Sources]
+  ArmGicArchLib.c
 
 [Packages]
   MdePkg/MdePkg.dec
diff --git a/ArmPkg/Library/ArmGicArchSecLib/Arm/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchSecLib/Arm/ArmGicArchLib.c
deleted file mode 100644
index 2373ca409a17..000000000000
--- a/ArmPkg/Library/ArmGicArchSecLib/Arm/ArmGicArchLib.c
+++ /dev/null
@@ -1,45 +0,0 @@
-/** @file
-*
-*  Copyright (c) 2014, ARM Limited. All rights reserved.
-*
-*  SPDX-License-Identifier: BSD-2-Clause-Patent
-*
-**/
-
-#include <Library/ArmLib.h>
-#include <Library/ArmGicLib.h>
-
-ARM_GIC_ARCH_REVISION
-EFIAPI
-ArmGicGetSupportedArchRevision (
-  VOID
-  )
-{
-  UINT32    IccSre;
-
-  // Ideally we would like to use the GICC IIDR Architecture version here, but
-  // this does not seem to be very reliable as the implementation could easily
-  // get it wrong. It is more reliable to check if the GICv3 System Register
-  // feature is implemented on the CPU. This is also convenient as our GICv3
-  // driver requires SRE. If only Memory mapped access is available we try to
-  // drive the GIC as a v2.
-  if (ArmHasGicSystemRegisters ()) {
-    // Make sure System Register access is enabled (SRE). This depends on the
-    // higher privilege level giving us permission, otherwise we will either
-    // cause an exception here, or the write doesn't stick in which case we need
-    // to fall back to the GICv2 MMIO interface.
-    // Note: We do not need to set ICC_SRE_EL2.Enable because the OS is started
-    // at the same exception level.
-    // It is the OS responsibility to set this bit.
-    IccSre = ArmGicV3GetControlSystemRegisterEnable ();
-    if (!(IccSre & ICC_SRE_EL2_SRE)) {
-      ArmGicV3SetControlSystemRegisterEnable (IccSre| ICC_SRE_EL2_SRE);
-      IccSre = ArmGicV3GetControlSystemRegisterEnable ();
-    }
-    if (IccSre & ICC_SRE_EL2_SRE) {
-      return ARM_GIC_ARCH_REVISION_3;
-    }
-  }
-
-  return ARM_GIC_ARCH_REVISION_2;
-}
diff --git a/ArmPkg/Library/ArmGicArchSecLib/AArch64/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchSecLib/ArmGicArchLib.c
similarity index 100%
rename from ArmPkg/Library/ArmGicArchSecLib/AArch64/ArmGicArchLib.c
rename to ArmPkg/Library/ArmGicArchSecLib/ArmGicArchLib.c
-- 
2.20.1

[PATCH 5/9] ArmPkg: add ArmHasSecurityExtensions () helper function

[Leif Lindholm]

Create a helper function to eliminate direct feature register reading.

Returns BOOLEAN True if the CPU implements the Security extensions,
otherwise returns BOOL False.

This function is only implemented for ARM, not AArch64.

Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
 ArmPkg/Include/Library/ArmLib.h      | 17 +++++++++++++++++
 ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c | 15 +++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h
index 8a364f2ca96f..c2ed72112b73 100644
--- a/ArmPkg/Include/Library/ArmLib.h
+++ b/ArmPkg/Include/Library/ArmLib.h
@@ -733,4 +733,21 @@ ArmHasGicSystemRegisters (
   VOID
   );
 
+#ifdef MDE_CPU_ARM
+///
+/// AArch32-only ID Register Helper functions
+///
+/**
+  Check whether the CPU supports the Security extensions
+
+  @return   Whether the Security extensions are implemented
+
+**/
+BOOLEAN
+EFIAPI
+ArmHasSecurityExtensions (
+  VOID
+  );
+#endif // MDE_CPU_ARM
+
 #endif // __ARM_LIB__
diff --git a/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c b/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c
index 3faada3a6539..9f81a7223732 100644
--- a/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c
+++ b/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c
@@ -87,3 +87,18 @@ ArmHasGicSystemRegisters (
 {
   return ((ArmReadIdPfr1 () & ARM_PFR1_GIC) != 0);
 }
+
+/**
+  Check whether the CPU supports the Security extensions
+
+  @return   Whether the Security extensions are implemented
+
+**/
+BOOLEAN
+EFIAPI
+ArmHasSecurityExtensions (
+  VOID
+  )
+{
+  return ((ArmReadIdPfr1 () & ARM_PFR1_SEC) != 0);
+}
-- 
2.20.1

[PATCH 6/9] ArmPkg: use helper to check for Security extensions in ArmArchTimerLib

[Leif Lindholm]

Use the helper ArmHasSecurityExtensions () instead of accessing
ID_PFR1 directly. Only affects ARM build.

Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
 ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c b/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
index 7c698fe471f3..24d9dae4e660 100644
--- a/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
+++ b/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
@@ -56,7 +56,7 @@ TimerConstructor (
       // If the security extension is not implemented, set Timer Frequency
       // here.
       //
-      if ((ArmReadIdPfr1 () & ARM_PFR1_SEC) == 0x0) {
+      if (ArmHasSecurityExtensions ()) {
         ArmGenericTimerSetTimerFreq (PcdGet32 (PcdArmArchTimerFreqInHz));
       }
 #endif
-- 
2.20.1

[PATCH 7/9] ArmPkg/ArmLib: delete AArch64 version of ArmReadIdPfr1

[Leif Lindholm]

The AArch64 version of ArmReadIdPfr1 is not used by any code in tree,
or in edk2-platforms. Delete it.

Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
 ArmPkg/Library/ArmLib/AArch64/AArch64Support.S | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
index 199374ff59e3..7f942c29ea66 100644
--- a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
+++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
@@ -453,14 +453,6 @@ ASM_FUNC(ArmReadIdPfr0)
   ret
 
 
-// Q: id_aa64pfr1_el1 not defined yet. What does this function want to access?
-// A: used to setup arch timer. Check if we have security extensions, permissions to set stuff.
-//    See: ArmPkg/Library/ArmArchTimerLib/AArch64/ArmArchTimerLib.c
-//    Not defined yet, but stick in here for now, should read all zeros.
-ASM_FUNC(ArmReadIdPfr1)
-  mrs   x0, id_aa64pfr1_el1   // Read ID_PFR1 Register
-  ret
-
 // VOID ArmWriteHcr(UINTN Hcr)
 ASM_FUNC(ArmWriteHcr)
   msr   hcr_el2, x0        // Write the passed HCR value
-- 
2.20.1

[PATCH 8/9] ArmPkg/ArmLib: rename AArch64 variant of ArmReadIdPfr0

[Leif Lindholm]

ArmReadIdPfr0 is now used only inside ArmLib. Rename the AArch64
variant ArmReadIdAA64Pfr0 and add a declaration of that only into
local header AArch64/AArch64Lib.h.

Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
 ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h     | 6 ++++++
 ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c     | 2 +-
 ArmPkg/Library/ArmLib/AArch64/AArch64Support.S | 5 +++--
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h
index b2c8a8ea0b84..85bcecda730f 100644
--- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h
+++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h
@@ -2,6 +2,7 @@
 
   Copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
   Portions Copyright (c) 2011 - 2013, ARM Ltd. All rights reserved.<BR>
+  Copyright (c) 2020, NUVIA Inc. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -35,5 +36,10 @@ ArmCleanInvalidateDataCacheEntryBySetWay (
   IN  UINTN   SetWayFormat
   );
 
+UINTN
+EFIAPI
+ArmReadIdAA64Pfr0 (
+  VOID
+  );
 #endif // __AARCH64_LIB_H__
 
diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
index 5b10eb33c97d..53e593bc994b 100644
--- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
+++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
@@ -85,5 +85,5 @@ ArmHasGicSystemRegisters (
   VOID
   )
 {
-  return ((ArmReadIdPfr0 () & AARCH64_PFR0_GIC) != 0);
+  return ((ArmReadIdAA64Pfr0 () & AARCH64_PFR0_GIC) != 0);
 }
diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
index 7f942c29ea66..129205d2ac27 100644
--- a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
+++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
@@ -3,6 +3,7 @@
 # Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
 # Copyright (c) 2011 - 2017, ARM Limited. All rights reserved.
 # Copyright (c) 2016, Linaro Limited. All rights reserved.
+# Copyright (c) 2020, NUVIA Inc. All rights reserved.
 #
 # SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -395,7 +396,7 @@ ASM_FUNC(ArmReadVBar)
 ASM_FUNC(ArmEnableVFP)
   // Check whether floating-point is implemented in the processor.
   mov   x1, x30                 // Save LR
-  bl    ArmReadIdPfr0           // Read EL1 Processor Feature Register (PFR0)
+  bl    ArmReadIdAA64Pfr0       // Read EL1 Processor Feature Register (PFR0)
   mov   x30, x1                 // Restore LR
   ubfx  x0, x0, #16, #4         // Extract the FP bits 16:19
   cmp   x0, #0xF                // Check if FP bits are '1111b',
@@ -448,7 +449,7 @@ ASM_FUNC(ArmIsArchTimerImplemented)
   ret
 
 
-ASM_FUNC(ArmReadIdPfr0)
+ASM_FUNC(ArmReadIdAA64Pfr0)
   mrs   x0, id_aa64pfr0_el1   // Read ID_AA64PFR0 Register
   ret
 
-- 
2.20.1

[PATCH 9/9] ArmPkg/ArmLib: move ArmReadIdPfr0/1 into private header ArmV7Lib.h

[Leif Lindholm]

ArmReadIdPfr0 () and ArmReadIdPfr1 () are now used only inside ArmLib.
Remove the prototypes from the public header to discourage new id
register accessor additions, and direct id register access in general.
Move them into local header Arm/ArmV7Lib.h.

Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
 ArmPkg/Include/Library/ArmLib.h      | 12 ------------
 ArmPkg/Library/ArmLib/Arm/ArmV7Lib.h | 12 ++++++++++++
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h
index c2ed72112b73..26cb05def0a2 100644
--- a/ArmPkg/Include/Library/ArmLib.h
+++ b/ArmPkg/Include/Library/ArmLib.h
@@ -133,18 +133,6 @@ ArmIsArchTimerImplemented (
   VOID
   );
 
-UINTN
-EFIAPI
-ArmReadIdPfr0 (
-  VOID
-  );
-
-UINTN
-EFIAPI
-ArmReadIdPfr1 (
-  VOID
-  );
-
 UINTN
 EFIAPI
 ArmCacheInfo (
diff --git a/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.h b/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.h
index 93183e67230e..bb7bda0a3aeb 100644
--- a/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.h
+++ b/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.h
@@ -48,5 +48,17 @@ ArmCleanInvalidateDataCacheEntryBySetWay (
   IN  UINTN   SetWayFormat
   );
 
+UINTN
+EFIAPI
+ArmReadIdPfr0 (
+  VOID
+  );
+
+UINTN
+EFIAPI
+ArmReadIdPfr1 (
+  VOID
+  );
+
 #endif // __ARM_V7_LIB_H__
 
-- 
2.20.1

Re: [PATCH 0/9] ArmPkg: Start cleaning up ID register handling

[Ard Biesheuvel]

On 12/18/20 3:16 PM, Leif Lindholm wrote:
> As discussed in https://edk2.groups.io/g/devel/topic/78784065,
> the ARM and AArch64 ports were using an unfortunate overloading
> of accessor functions for the ID registers which describe the
> presence and versions of features implemented in a CPU.
> 
> This makes no sense, since the layout, and indeed the names of
> the registers for the different architectural states differ.
> 
> This set replaces the use of the current accessor functions
> with some helper functions in ArmLib. It also moves the
> accessor function prototypes into private headers so they
> can no longer be called from outside ArmLib
> 
> As a pure side effect, we're able to throw away half of
> ArmGicArchLib/ArmGicArchSecLib.
> 
> Note that some additional cleanup can also be done in the ARM
> portion of ArmMmuLib (which implements its own direct accessor
> for ID_MMFR0), and internally in ArmLib where some assembly
> code does its own feature checks, which could be moved to C
> helpers.
> 
> Furthermore, it might be useful to move the ID register field
> definitions to a private header.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> 
> Leif Lindholm (9):
>   ArmPkg/ArmLib: add ArmHasGicSystemRegisters () helper function
>   ArmPkg: use ID register helper for ArmGicArch(Sec)Lib
>   ArmPkg: remove duplicated ARM/AArch64 ArmGicArchLib sources
>   ArmPkg: remove duplicated ARM/AArch64 ArmGicArchSecLib sources
>   ArmPkg: add ArmHasSecurityExtensions () helper function
>   ArmPkg: use helper to check for Security extensions in ArmArchTimerLib
>   ArmPkg/ArmLib: delete AArch64 version of ArmReadIdPfr1
>   ArmPkg/ArmLib: rename AArch64 variant of ArmReadIdPfr0
>   ArmPkg/ArmLib: move ArmReadIdPfr0/1 into private header ArmV7Lib.h
> 

Thanks for getting started on this.

For the series,

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@arm.com>

>  ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf                |  7 +--
>  ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf          |  7 +--
>  ArmPkg/Include/Library/ArmLib.h                               | 47 +++++++++++----
>  ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h                    |  6 ++
>  ArmPkg/Library/ArmLib/Arm/ArmV7Lib.h                          | 12 ++++
>  ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c              |  2 +-
>  ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c              | 60 --------------------
>  ArmPkg/Library/ArmGicArchLib/{AArch64 => }/ArmGicArchLib.c    |  2 +-
>  ArmPkg/Library/ArmGicArchSecLib/Arm/ArmGicArchLib.c           | 45 ---------------
>  ArmPkg/Library/ArmGicArchSecLib/{AArch64 => }/ArmGicArchLib.c |  2 +-
>  ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c                    | 16 ++++++
>  ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c                          | 31 ++++++++++
>  ArmPkg/Library/ArmLib/AArch64/AArch64Support.S                | 13 +----
>  13 files changed, 110 insertions(+), 140 deletions(-)
>  delete mode 100644 ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
>  rename ArmPkg/Library/ArmGicArchLib/{AArch64 => }/ArmGicArchLib.c (94%)
>  delete mode 100644 ArmPkg/Library/ArmGicArchSecLib/Arm/ArmGicArchLib.c
>  rename ArmPkg/Library/ArmGicArchSecLib/{AArch64 => }/ArmGicArchLib.c (94%)
> 

Re: [PATCH 0/9] ArmPkg: Start cleaning up ID register handling

[Leif Lindholm]

On Fri, Dec 18, 2020 at 16:02:07 +0100, Ard Biesheuvel wrote:
> On 12/18/20 3:16 PM, Leif Lindholm wrote:
> > As discussed in https://edk2.groups.io/g/devel/topic/78784065,
> > the ARM and AArch64 ports were using an unfortunate overloading
> > of accessor functions for the ID registers which describe the
> > presence and versions of features implemented in a CPU.
> > 
> > This makes no sense, since the layout, and indeed the names of
> > the registers for the different architectural states differ.
> > 
> > This set replaces the use of the current accessor functions
> > with some helper functions in ArmLib. It also moves the
> > accessor function prototypes into private headers so they
> > can no longer be called from outside ArmLib
> > 
> > As a pure side effect, we're able to throw away half of
> > ArmGicArchLib/ArmGicArchSecLib.
> > 
> > Note that some additional cleanup can also be done in the ARM
> > portion of ArmMmuLib (which implements its own direct accessor
> > for ID_MMFR0), and internally in ArmLib where some assembly
> > code does its own feature checks, which could be moved to C
> > helpers.
> > 
> > Furthermore, it might be useful to move the ID register field
> > definitions to a private header.
> > 
> > Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > 
> > Leif Lindholm (9):
> >   ArmPkg/ArmLib: add ArmHasGicSystemRegisters () helper function
> >   ArmPkg: use ID register helper for ArmGicArch(Sec)Lib
> >   ArmPkg: remove duplicated ARM/AArch64 ArmGicArchLib sources
> >   ArmPkg: remove duplicated ARM/AArch64 ArmGicArchSecLib sources
> >   ArmPkg: add ArmHasSecurityExtensions () helper function
> >   ArmPkg: use helper to check for Security extensions in ArmArchTimerLib
> >   ArmPkg/ArmLib: delete AArch64 version of ArmReadIdPfr1
> >   ArmPkg/ArmLib: rename AArch64 variant of ArmReadIdPfr0
> >   ArmPkg/ArmLib: move ArmReadIdPfr0/1 into private header ArmV7Lib.h
> > 
> 
> Thanks for getting started on this.
> 
> For the series,
> 
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@arm.com>

Thanks!

Pushed as 6573ae8c8575..e2bfd172e4b9 (#1244).

> >  ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf                |  7 +--
> >  ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf          |  7 +--
> >  ArmPkg/Include/Library/ArmLib.h                               | 47 +++++++++++----
> >  ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h                    |  6 ++
> >  ArmPkg/Library/ArmLib/Arm/ArmV7Lib.h                          | 12 ++++
> >  ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c              |  2 +-
> >  ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c              | 60 --------------------
> >  ArmPkg/Library/ArmGicArchLib/{AArch64 => }/ArmGicArchLib.c    |  2 +-
> >  ArmPkg/Library/ArmGicArchSecLib/Arm/ArmGicArchLib.c           | 45 ---------------
> >  ArmPkg/Library/ArmGicArchSecLib/{AArch64 => }/ArmGicArchLib.c |  2 +-
> >  ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c                    | 16 ++++++
> >  ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c                          | 31 ++++++++++
> >  ArmPkg/Library/ArmLib/AArch64/AArch64Support.S                | 13 +----
> >  13 files changed, 110 insertions(+), 140 deletions(-)
> >  delete mode 100644 ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
> >  rename ArmPkg/Library/ArmGicArchLib/{AArch64 => }/ArmGicArchLib.c (94%)
> >  delete mode 100644 ArmPkg/Library/ArmGicArchSecLib/Arm/ArmGicArchLib.c
> >  rename ArmPkg/Library/ArmGicArchSecLib/{AArch64 => }/ArmGicArchLib.c (94%)
> > 
>