[PATCH 0/3] ArmPkg/ArmLib: ASSERT() on misuse of set/way ops

[PATCH 0/3] ArmPkg/ArmLib: ASSERT() on misuse of set/way ops

[Ard Biesheuvel]

Talking to Leif on IRC, we decided that deprecating cache maintenance
by set/way, as I proposed in the series I just sent out [0] may be too
strict, especially considering that some v7 based uniprocessor platforms
such as BeagleBoard may actually need it to clean any junk from the caches
before turning them on.

So instead, ensure that this use cache remains supported, but discourage/
prevent misuse by ASSERT()ing that they are only used with the MMU off.

[0] https://edk2.groups.io/g/devel/topic/patch_0_6_armpkg_eradicate/71562844

Ard Biesheuvel (3):
  ArmPkg/ArmLib: clean up library includes
  ArmPkg/ArmLib: remove bogus protocol declaration
  ArmPkg/ArmLib: ASSERT on set/way cache ops being used with MMU on

 ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c | 16 ++++++++++++----
 ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c       | 17 +++++++++++++----
 ArmPkg/Library/ArmLib/ArmBaseLib.inf       |  6 +++---
 ArmPkg/Library/ArmLib/ArmLib.c             |  2 --
 4 files changed, 28 insertions(+), 13 deletions(-)

-- 
2.17.1

[PATCH 1/3] ArmPkg/ArmLib: clean up library includes

[Ard Biesheuvel]

Suspiciously, ArmLib's INF does not contain a [LibraryClasses]
section at all, but it turns out that all the library includes
it contains (except for ArmLib.h itself) are actually bogus so
let's just drop all of them. While at it, replace <Uefi.h> with
the more accurate <Base.h> for a BASE type module, and put the
includes in a consistent order.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c |  9 +++++----
 ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c       | 10 ++++++----
 ArmPkg/Library/ArmLib/ArmLib.c             |  2 --
 3 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
index 0ed8dae9a4b0..924bf48020e0 100644
--- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
+++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
@@ -7,11 +7,12 @@
 
 **/
 
-#include <Uefi.h>
-#include <Chipset/AArch64.h>
+#include <Base.h>
+
 #include <Library/ArmLib.h>
-#include <Library/BaseLib.h>
-#include <Library/IoLib.h>
+
+#include <Chipset/AArch64.h>
+
 #include "AArch64Lib.h"
 #include "ArmLibPrivate.h"
 
diff --git a/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c b/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c
index 38516d4f1b87..5d93aa6e0b8c 100644
--- a/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c
+++ b/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c
@@ -6,11 +6,13 @@
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
-#include <Uefi.h>
-#include <Chipset/ArmV7.h>
+
+#include <Base.h>
+
 #include <Library/ArmLib.h>
-#include <Library/BaseLib.h>
-#include <Library/IoLib.h>
+
+#include <Chipset/ArmV7.h>
+
 #include "ArmV7Lib.h"
 #include "ArmLibPrivate.h"
 
diff --git a/ArmPkg/Library/ArmLib/ArmLib.c b/ArmPkg/Library/ArmLib/ArmLib.c
index c682c3ab6339..3905d02c5e7e 100644
--- a/ArmPkg/Library/ArmLib/ArmLib.c
+++ b/ArmPkg/Library/ArmLib/ArmLib.c
@@ -10,8 +10,6 @@
 #include <Base.h>
 
 #include <Library/ArmLib.h>
-#include <Library/DebugLib.h>
-#include <Library/PcdLib.h>
 
 #include "ArmLibPrivate.h"
 
-- 
2.17.1

[PATCH 2/3] ArmPkg/ArmLib: remove bogus protocol declaration

[Ard Biesheuvel]

ArmLib is a BASE type library, which should not depend or
even be aware on DXE type protocols. So drop the reference
to gEfiCpuArchProtocolGuid.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Library/ArmLib/ArmBaseLib.inf | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/ArmPkg/Library/ArmLib/ArmBaseLib.inf b/ArmPkg/Library/ArmLib/ArmBaseLib.inf
index 5e70990872f2..106a09f821e1 100644
--- a/ArmPkg/Library/ArmLib/ArmBaseLib.inf
+++ b/ArmPkg/Library/ArmLib/ArmBaseLib.inf
@@ -48,8 +48,5 @@ [Packages]
   ArmPkg/ArmPkg.dec
   MdePkg/MdePkg.dec
 
-[Protocols]
-  gEfiCpuArchProtocolGuid
-
 [FeaturePcd.ARM]
   gArmTokenSpaceGuid.PcdNormalMemoryNonshareableOverride
-- 
2.17.1

[PATCH 3/3] ArmPkg/ArmLib: ASSERT on set/way cache ops being used with MMU on

[Ard Biesheuvel]

On ARMv7 and up, doing cache maintenance by set/way is only
permitted in the context of on/offlining a core, and any other
uses should be avoided. Add ASSERT()s in the right place to
ensure that any uses with the MMU enabled are caught in DEBUG
builds.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c | 7 +++++++
 ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c       | 7 +++++++
 ArmPkg/Library/ArmLib/ArmBaseLib.inf       | 3 +++
 3 files changed, 17 insertions(+)

diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
index 924bf48020e0..3fbd591192e2 100644
--- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
+++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
@@ -10,6 +10,7 @@
 #include <Base.h>
 
 #include <Library/ArmLib.h>
+#include <Library/DebugLib.h>
 
 #include <Chipset/AArch64.h>
 
@@ -41,6 +42,8 @@ ArmInvalidateDataCache (
   VOID
   )
 {
+  ASSERT (!ArmMmuEnabled ());
+
   ArmDataSynchronizationBarrier ();
   AArch64DataCacheOperation (ArmInvalidateDataCacheEntryBySetWay);
 }
@@ -51,6 +54,8 @@ ArmCleanInvalidateDataCache (
   VOID
   )
 {
+  ASSERT (!ArmMmuEnabled ());
+
   ArmDataSynchronizationBarrier ();
   AArch64DataCacheOperation (ArmCleanInvalidateDataCacheEntryBySetWay);
 }
@@ -61,6 +66,8 @@ ArmCleanDataCache (
   VOID
   )
 {
+  ASSERT (!ArmMmuEnabled ());
+
   ArmDataSynchronizationBarrier ();
   AArch64DataCacheOperation (ArmCleanDataCacheEntryBySetWay);
 }
diff --git a/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c b/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c
index 5d93aa6e0b8c..2c4a23e1a1b2 100644
--- a/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c
+++ b/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c
@@ -10,6 +10,7 @@
 #include <Base.h>
 
 #include <Library/ArmLib.h>
+#include <Library/DebugLib.h>
 
 #include <Chipset/ArmV7.h>
 
@@ -41,6 +42,8 @@ ArmInvalidateDataCache (
   VOID
   )
 {
+  ASSERT (!ArmMmuEnabled ());
+
   ArmDataSynchronizationBarrier ();
   ArmV7DataCacheOperation (ArmInvalidateDataCacheEntryBySetWay);
 }
@@ -51,6 +54,8 @@ ArmCleanInvalidateDataCache (
   VOID
   )
 {
+  ASSERT (!ArmMmuEnabled ());
+
   ArmDataSynchronizationBarrier ();
   ArmV7DataCacheOperation (ArmCleanInvalidateDataCacheEntryBySetWay);
 }
@@ -61,6 +66,8 @@ ArmCleanDataCache (
   VOID
   )
 {
+  ASSERT (!ArmMmuEnabled ());
+
   ArmDataSynchronizationBarrier ();
   ArmV7DataCacheOperation (ArmCleanDataCacheEntryBySetWay);
 }
diff --git a/ArmPkg/Library/ArmLib/ArmBaseLib.inf b/ArmPkg/Library/ArmLib/ArmBaseLib.inf
index 106a09f821e1..f61c71b673d1 100644
--- a/ArmPkg/Library/ArmLib/ArmBaseLib.inf
+++ b/ArmPkg/Library/ArmLib/ArmBaseLib.inf
@@ -44,6 +44,9 @@ [Sources.AARCH64]
   AArch64/AArch64Support.S
   AArch64/AArch64ArchTimerSupport.S
 
+[LibraryClasses]
+  DebugLib
+
 [Packages]
   ArmPkg/ArmPkg.dec
   MdePkg/MdePkg.dec
-- 
2.17.1