[edk2] [PATCH 0/7] ArmPkg/ArmVirtPkg: GIC revision detection

[Ard Biesheuvel]

The current GIC revision detection code infers the GIC revision from
the AA64PFR0_EL1.GIC feature bit that tells us whether the GIC system
register interface is implemented in the hardware, and then proceeds
to attempt and enable it.

The library containing this code deliberately does not cache the
detected revision since it may execute in the SEC phase and may
be running from NOR flash and not RAM.

However, since the detection code runs very often, and is quite
heavy-weight when running under virtualization (especially KVM),
this series refactors the GIC revision detection to:
- use fewer system register accesses if possible
- provide an alternative that does cache the detected revision
- use the DT supplied revision when executing on a DT based platform


Ard Biesheuvel (7):
ArmPkg: reduce sysreg access count in GIC revision probe
ArmPkg: merge ArmGicV[23]Lib.h into ArmGicLib.h
ArmPkg: split off ArmGicArchLib from ArmGicLib
ArmPkg: copy ArmGicArchLib to ArmGicArchSecLib
ArmPkg: cache detected revision in ArmGicArchLib
ArmVirtPkg: record GIC revision in dynamic PCD
ArmVirtPkg: implement DT-based ArmGicArchLib

ArmPkg/ArmPkg.dec | 1 +
ArmPkg/ArmPkg.dsc | 1 +
ArmPkg/Drivers/ArmGic/ArmGicLib.c | 3 -
ArmPkg/Drivers/ArmGic/ArmGicLib.inf | 3 +-
ArmPkg/Drivers/ArmGic/ArmGicSecLib.c | 2 -
ArmPkg/Drivers/ArmGic/ArmGicSecLib.inf | 3 +-
ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c | 3 +-
ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.h | 54 ------
ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 3 +-
ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Lib.h | 68 -------
ArmPkg/Include/Library/ArmGicArchLib.h | 33 ++++
ArmPkg/Include/Library/ArmGicLib.h | 108 ++++++++++--
ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c | 66 +++++++
ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c | 66 +++++++
ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf | 34 ++++
ArmPkg/{Drivers/ArmGic => Library/ArmGicArchSecLib}/AArch64/ArmGicArchLib.c | 12 +-
ArmPkg/{Drivers/ArmGic => Library/ArmGicArchSecLib}/Arm/ArmGicArchLib.c | 12 +-
ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf | 33 ++++
ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc | 2 +
ArmPlatformPkg/ArmPlatformPkg.dsc | 3 +
ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc | 3 +
ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc | 3 +
ArmVirtPkg/ArmVirt.dsc.inc | 1 +
ArmVirtPkg/ArmVirtPkg.dec | 7 +-
ArmVirtPkg/ArmVirtQemu.dsc | 1 +
ArmVirtPkg/ArmVirtXen.dsc | 1 +
ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c | 75 ++++++++
ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf | 40 +++++
ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c | 2 +
ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf | 1 +
30 files changed, 489 insertions(+), 155 deletions(-)
delete mode 100644 ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.h
delete mode 100644 ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Lib.h
create mode 100644 ArmPkg/Include/Library/ArmGicArchLib.h
create mode 100644 ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
create mode 100644 ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
create mode 100644 ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
rename ArmPkg/{Drivers/ArmGic => Library/ArmGicArchSecLib}/AArch64/ArmGicArchLib.c (82%)
rename ArmPkg/{Drivers/ArmGic => Library/ArmGicArchSecLib}/Arm/ArmGicArchLib.c (82%)
create mode 100644 ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
create mode 100644 ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
create mode 100644 ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf

[Ard Biesheuvel]

Accesses to system registers are disproportionately heavy-weight
when executed under virtualization, since each one involves two
world switches (from guest to host and back again).

So change the sequence that enables the GIC SRE interface so that
it performs only a single sysreg read to test whether the SRE
interface is enabled already, and only performs a write and an
additional read if that turns out not to be the case.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <***@linaro.org>
---
ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c | 10 ++++++++--
ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c | 10 ++++++++--
2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c b/ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c
index 9da69b2131e3..88fa4621e613 100644
--- a/ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c
+++ b/ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c
@@ -23,6 +23,8 @@ 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
@@ -37,8 +39,12 @@ ArmGicGetSupportedArchRevision (
// 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.
- ArmGicV3SetControlSystemRegisterEnable (ArmGicV3GetControlSystemRegisterEnable () | ICC_SRE_EL2_SRE);
- if (ArmGicV3GetControlSystemRegisterEnable () & ICC_SRE_EL2_SRE) {
+ 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;
}
}
diff --git a/ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c b/ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c
index f360a405833d..9ef56efeaa7b 100644
--- a/ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c
+++ b/ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c
@@ -23,6 +23,8 @@ 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
@@ -37,8 +39,12 @@ ArmGicGetSupportedArchRevision (
// 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.
- ArmGicV3SetControlSystemRegisterEnable (ArmGicV3GetControlSystemRegisterEnable () | ICC_SRE_EL2_SRE);
- if (ArmGicV3GetControlSystemRegisterEnable () & ICC_SRE_EL2_SRE) {
+ 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;
}
}

[Olivier Martin]

Post by Ard Biesheuvel
Accesses to system registers are disproportionately heavy-weight
when executed under virtualization, since each one involves two
world switches (from guest to host and back again).
So change the sequence that enables the GIC SRE interface so that
it performs only a single sysreg read to test whether the SRE
interface is enabled already, and only performs a write and an
additional read if that turns out not to be the case.
Contributed-under: TianoCore Contribution Agreement 1.0
---
ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c | 10 ++++++++--
ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c | 10 ++++++++--
2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c b/ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c
index 9da69b2131e3..88fa4621e613 100644
--- a/ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c
+++ b/ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c
@@ -23,6 +23,8 @@ 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
@@ -37,8 +39,12 @@ ArmGicGetSupportedArchRevision (
// 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.
- ArmGicV3SetControlSystemRegisterEnable (ArmGicV3GetControlSystemRegisterEnable () | ICC_SRE_EL2_SRE);
- if (ArmGicV3GetControlSystemRegisterEnable () & ICC_SRE_EL2_SRE) {
+ 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;
}
}
diff --git a/ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c b/ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c
index f360a405833d..9ef56efeaa7b 100644
--- a/ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c
+++ b/ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c
@@ -23,6 +23,8 @@ 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
@@ -37,8 +39,12 @@ ArmGicGetSupportedArchRevision (
// 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.
- ArmGicV3SetControlSystemRegisterEnable (ArmGicV3GetControlSystemRegisterEnable () | ICC_SRE_EL2_SRE);
- if (ArmGicV3GetControlSystemRegisterEnable () & ICC_SRE_EL2_SRE) {
+ 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;
}
}

-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782


------------------------------------------------------------------------------

[Ard Biesheuvel]

The current implementation of ArmGicGetSupportedArchRevision ()
that is used by all ARM platforms is entirely stateless (in order
to support being executed from flash) so it needs to interrogate
the hardware for the supported GIC revision upon each invocation.

However, this statelessness is only needed for SEC type modules;
in all other cases, we could easily determine the GIC revision once,
and store the result in a global variable.

In preparation of having separate early and normal versions, this patch
introduces the ArmGicArchLib library class and default implementation,
and moves the existing ArmGicGetSupportedArchRevision () into it.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <***@linaro.org>
---
ArmPkg/ArmPkg.dec | 1 +
ArmPkg/ArmPkg.dsc | 1 +
ArmPkg/Drivers/ArmGic/ArmGicLib.inf | 3 +-
ArmPkg/Drivers/ArmGic/ArmGicSecLib.inf | 3 +-
ArmPkg/Include/Library/ArmGicArchLib.h | 33 ++++++++++++
ArmPkg/Include/Library/ArmGicLib.h | 14 +----
ArmPkg/{Drivers/ArmGic => Library/ArmGicArchLib}/AArch64/ArmGicArchLib.c | 0
ArmPkg/{Drivers/ArmGic => Library/ArmGicArchLib}/Arm/ArmGicArchLib.c | 0
ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf | 33 ++++++++++++
ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc | 1 +
ArmPlatformPkg/ArmPlatformPkg.dsc | 1 +
ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc | 1 +
ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc | 1 +
ArmVirtPkg/ArmVirt.dsc.inc | 1 +
14 files changed, 76 insertions(+), 17 deletions(-)

diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
index b30de9152c13..bf7278b50815 100644
--- a/ArmPkg/ArmPkg.dec
+++ b/ArmPkg/ArmPkg.dec
@@ -38,6 +38,7 @@
UncachedMemoryAllocationLib|Include/Library/UncachedMemoryAllocationLib.h
DefaultExceptionHandlerLib|Include/Library/DefaultExceptionHandlerLib.h
ArmDisassemblerLib|Include/Library/ArmDisassemblerLib.h
+ ArmGicArchLib|Include/Library/ArmGicArchLib.h

[Guids.common]
gArmTokenSpaceGuid = { 0xBB11ECFE, 0x820F, 0x4968, { 0xBB, 0xA6, 0xF7, 0x6A, 0xFE, 0x30, 0x25, 0x96 } }
diff --git a/ArmPkg/ArmPkg.dsc b/ArmPkg/ArmPkg.dsc
index 6959fde22c12..36eb394a2e37 100644
--- a/ArmPkg/ArmPkg.dsc
+++ b/ArmPkg/ArmPkg.dsc
@@ -66,6 +66,7 @@

CpuLib|MdePkg/Library/BaseCpuLib/BaseCpuLib.inf
ArmGicLib|ArmPkg/Drivers/ArmGic/ArmGicLib.inf
+ ArmGicArchLib|ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
ArmGenericTimerCounterLib|ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib.inf
ArmSmcLib|ArmPkg/Library/ArmSmcLib/ArmSmcLib.inf
ArmDisassemblerLib|ArmPkg/Library/ArmDisassemblerLib/ArmDisassemblerLib.inf
diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.inf b/ArmPkg/Drivers/ArmGic/ArmGicLib.inf
index 2ae3fd31e892..047adac85ff4 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicLib.inf
+++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.inf
@@ -27,18 +27,17 @@
GicV2/ArmGicV2NonSecLib.c

[Sources.ARM]
- Arm/ArmGicArchLib.c
GicV3/Arm/ArmGicV3.S | GCC
GicV3/Arm/ArmGicV3.asm | RVCT

[Sources.AARCH64]
- AArch64/ArmGicArchLib.c
GicV3/AArch64/ArmGicV3.S

[LibraryClasses]
ArmLib
DebugLib
IoLib
+ ArmGicArchLib

[Packages]
ArmPkg/ArmPkg.dec
diff --git a/ArmPkg/Drivers/ArmGic/ArmGicSecLib.inf b/ArmPkg/Drivers/ArmGic/ArmGicSecLib.inf
index 7d4e49e4b96f..fc2e1bc01efe 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicSecLib.inf
+++ b/ArmPkg/Drivers/ArmGic/ArmGicSecLib.inf
@@ -27,12 +27,10 @@
GicV2/ArmGicV2SecLib.c

[Sources.ARM]
- Arm/ArmGicArchLib.c
GicV3/Arm/ArmGicV3.S | GCC
GicV3/Arm/ArmGicV3.asm | RVCT

[Sources.AARCH64]
- AArch64/ArmGicArchLib.c
GicV3/AArch64/ArmGicV3.S

[Packages]
@@ -45,6 +43,7 @@
ArmLib
DebugLib
IoLib
+ ArmGicArchLib

[Pcd]
gArmPlatformTokenSpaceGuid.PcdCoreCount
diff --git a/ArmPkg/Include/Library/ArmGicArchLib.h b/ArmPkg/Include/Library/ArmGicArchLib.h
new file mode 100644
index 000000000000..e6964a2d5790
--- /dev/null
+++ b/ArmPkg/Include/Library/ArmGicArchLib.h
@@ -0,0 +1,33 @@
+/** @file
+*
+* Copyright (c) 2015, Linaro Ltd. All rights reserved.
+*
+* This program and the accompanying materials are licensed and made available
+* under the terms and conditions of the BSD License which accompanies this
+* distribution. The full text of the license may be found at
+* http://opensource.org/licenses/bsd-license.php
+*
+* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+*
+**/
+
+#ifndef __ARM_GIC_ARCH_LIB_H__
+#define __ARM_GIC_ARCH_LIB_H__
+
+//
+// GIC definitions
+//
+typedef enum {
+ ARM_GIC_ARCH_REVISION_2,
+ ARM_GIC_ARCH_REVISION_3
+} ARM_GIC_ARCH_REVISION;
+
+
+ARM_GIC_ARCH_REVISION
+EFIAPI
+ArmGicGetSupportedArchRevision (
+ VOID
+ );
+
+#endif
diff --git a/ArmPkg/Include/Library/ArmGicLib.h b/ArmPkg/Include/Library/ArmGicLib.h
index e3db9c0d250f..10c4a9d72eb2 100644
--- a/ArmPkg/Include/Library/ArmGicLib.h
+++ b/ArmPkg/Include/Library/ArmGicLib.h
@@ -15,13 +15,7 @@
#ifndef __ARMGIC_H
#define __ARMGIC_H

-//
-// GIC definitions
-//
-typedef enum {
- ARM_GIC_ARCH_REVISION_2,
- ARM_GIC_ARCH_REVISION_3
-} ARM_GIC_ARCH_REVISION;
+#include <Library/ArmGicArchLib.h>

//
// GIC Distributor
@@ -103,12 +97,6 @@ typedef enum {
// Bit Mask for
#define ARM_GIC_ICCIAR_ACKINTID 0x3FF

-ARM_GIC_ARCH_REVISION
-EFIAPI
-ArmGicGetSupportedArchRevision (
- VOID
- );
-
UINTN
EFIAPI
ArmGicGetInterfaceIdentification (
diff --git a/ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
similarity index 100%
rename from ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c
rename to ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
diff --git a/ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
similarity index 100%
rename from ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c
rename to ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
diff --git a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
new file mode 100644
index 000000000000..d71b2adc3027
--- /dev/null
+++ b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
@@ -0,0 +1,33 @@
+#/* @file
+# Copyright (c) 2015, Linaro Ltd. All rights reserved.
+#
+# This program and the accompanying materials
+# are licensed and made available under the terms and conditions of the BSD License
+# which accompanies this distribution. The full text of the license may be found at
+# http://opensource.org/licenses/bsd-license.php
+#
+# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+#
+#*/
+
+[Defines]
+ INF_VERSION = 0x00010005
+ BASE_NAME = ArmGicArchLib
+ FILE_GUID = cd67f41a-26e9-4482-90c9-a9aff803382a
+ MODULE_TYPE = BASE
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = ArmGicArchLib
+
+[Sources.ARM]
+ Arm/ArmGicArchLib.c
+
+[Sources.AARCH64]
+ AArch64/ArmGicArchLib.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ ArmPkg/ArmPkg.dec
+
+[LibraryClasses]
+ ArmGicLib
diff --git a/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc b/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
index 4cfb913b8ed3..0859bc31ff00 100644
--- a/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
+++ b/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
@@ -78,6 +78,7 @@
ArmDisassemblerLib|ArmPkg/Library/ArmDisassemblerLib/ArmDisassemblerLib.inf
DmaLib|ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
ArmGicLib|ArmPkg/Drivers/ArmGic/ArmGicLib.inf
+ ArmGicArchLib|ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
ArmPlatformStackLib|ArmPlatformPkg/Library/ArmPlatformStackLib/ArmPlatformStackLib.inf
ArmSmcLib|ArmPkg/Library/ArmSmcLib/ArmSmcLib.inf

diff --git a/ArmPlatformPkg/ArmPlatformPkg.dsc b/ArmPlatformPkg/ArmPlatformPkg.dsc
index f9f217cd61fd..be31025d9bd0 100644
--- a/ArmPlatformPkg/ArmPlatformPkg.dsc
+++ b/ArmPlatformPkg/ArmPlatformPkg.dsc
@@ -78,6 +78,7 @@
ArmDisassemblerLib|ArmPkg/Library/ArmDisassemblerLib/ArmDisassemblerLib.inf
DmaLib|ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
ArmGicLib|ArmPkg/Drivers/ArmGic/ArmGicLib.inf
+ ArmGicArchLib|ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
ArmSmcLib|ArmPkg/Library/ArmSmcLib/ArmSmcLib.inf

SerialPortLib|MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortLibNull.inf
diff --git a/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc b/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
index 9f5b384c85bd..19de3816ae5c 100644
--- a/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
+++ b/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
@@ -61,6 +61,7 @@
ArmDisassemblerLib|ArmPkg/Library/ArmDisassemblerLib/ArmDisassemblerLib.inf
DmaLib|ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
ArmGicLib|ArmPkg/Drivers/ArmGic/ArmGicLib.inf
+ ArmGicArchLib|ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
ArmSmcLib|ArmPkg/Library/ArmSmcLib/ArmSmcLib.inf
ArmGenericTimerCounterLib|ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib.inf

diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
index 00dfbec74fb1..84e2a9987f7d 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
+++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
@@ -67,6 +67,7 @@
ArmDisassemblerLib|ArmPkg/Library/ArmDisassemblerLib/ArmDisassemblerLib.inf
DmaLib|ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
ArmGicLib|ArmPkg/Drivers/ArmGic/ArmGicLib.inf
+ ArmGicArchLib|ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
ArmPlatformStackLib|ArmPlatformPkg/Library/ArmPlatformStackLib/ArmPlatformStackLib.inf
ArmSmcLib|ArmPkg/Library/ArmSmcLib/ArmSmcLib.inf
ArmGenericTimerCounterLib|ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib.inf
diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index 51b1079003df..900d5c3d9dd0 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -71,6 +71,7 @@
ArmDisassemblerLib|ArmPkg/Library/ArmDisassemblerLib/ArmDisassemblerLib.inf
DmaLib|ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
ArmGicLib|ArmPkg/Drivers/ArmGic/ArmGicLib.inf
+ ArmGicArchLib|ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
ArmPlatformStackLib|ArmPlatformPkg/Library/ArmPlatformStackLib/ArmPlatformStackLib.inf
ArmSmcLib|ArmPkg/Library/ArmSmcLib/ArmSmcLib.inf
ArmHvcLib|ArmPkg/Library/ArmHvcLib/ArmHvcLib.inf

[Laszlo Ersek]

Post by Ard Biesheuvel
diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index 51b1079003df..900d5c3d9dd0 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -71,6 +71,7 @@
ArmDisassemblerLib|ArmPkg/Library/ArmDisassemblerLib/ArmDisassemblerLib.inf
DmaLib|ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
ArmGicLib|ArmPkg/Drivers/ArmGic/ArmGicLib.inf
+ ArmGicArchLib|ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
ArmPlatformStackLib|ArmPlatformPkg/Library/ArmPlatformStackLib/ArmPlatformStackLib.inf
ArmSmcLib|ArmPkg/Library/ArmSmcLib/ArmSmcLib.inf
ArmHvcLib|ArmPkg/Library/ArmHvcLib/ArmHvcLib.inf

Acked-by: Laszlo Ersek <***@redhat.com>


------------------------------------------------------------------------------

[Ard Biesheuvel]

Before splitting off ArmGicArchLib and moving it out of
ArmPkg/Drivers/ArmGic into ArmPkg/Library, make sure that the
GIC specific declarations it depends on are not hidden away in
local headers "GicV2/GicV2Lib.h" and "GicV3/GicV3Lib.h".

So merge them with <Library/ArmGicLib.h>. This is entirely
appropriate, since this is not a header that declares a public
interface into ArmGicLib, but defines implementation internals.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <***@linaro.org>
---
ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c | 2 -
ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c | 2 -
ArmPkg/Drivers/ArmGic/ArmGicLib.c | 3 -
ArmPkg/Drivers/ArmGic/ArmGicSecLib.c | 2 -
ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c | 3 +-
ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.h | 54 -------
ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 3 +-
ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Lib.h | 68 --------
ArmPkg/Include/Library/ArmGicLib.h | 94 ++++++++++++
9 files changed, 98 insertions(+), 133 deletions(-)

diff --git a/ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c b/ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c
index 88fa4621e613..0e0fa3b9f33e 100644
--- a/ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c
+++ b/ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c
@@ -15,8 +15,6 @@
#include <Library/ArmLib.h>
#include <Library/ArmGicLib.h>

-#include "GicV3/ArmGicV3Lib.h"
-
ARM_GIC_ARCH_REVISION
EFIAPI
ArmGicGetSupportedArchRevision (
diff --git a/ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c b/ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c
index 9ef56efeaa7b..f256de704631 100644
--- a/ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c
+++ b/ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c
@@ -15,8 +15,6 @@
#include <Library/ArmLib.h>
#include <Library/ArmGicLib.h>

-#include "GicV3/ArmGicV3Lib.h"
-
ARM_GIC_ARCH_REVISION
EFIAPI
ArmGicGetSupportedArchRevision (
diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
index 48708e3812b4..248e896c4b94 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
@@ -19,9 +19,6 @@
#include <Library/IoLib.h>
#include <Library/PcdLib.h>

-#include "GicV2/ArmGicV2Lib.h"
-#include "GicV3/ArmGicV3Lib.h"
-
/**
* Return the base address of the GIC redistributor for the current CPU
*
diff --git a/ArmPkg/Drivers/ArmGic/ArmGicSecLib.c b/ArmPkg/Drivers/ArmGic/ArmGicSecLib.c
index 1fdd4d73bd7d..d64806d2f16b 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicSecLib.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicSecLib.c
@@ -17,8 +17,6 @@
#include <Library/IoLib.h>
#include <Library/ArmGicLib.h>

-#include "GicV2/ArmGicV2Lib.h"
-
/*
* This function configures the interrupts set by the mask to be secure.
*
diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
index 743c534e04d9..e649ac1bc6af 100644
--- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
+++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
@@ -22,8 +22,9 @@ Abstract:

--*/

+#include <Library/ArmGicLib.h>
+
#include "ArmGicDxe.h"
-#include "GicV2/ArmGicV2Lib.h"

#define ARM_GIC_DEFAULT_PRIORITY 0x80

diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.h b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.h
deleted file mode 100644
index 6803467346a3..000000000000
--- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.h
+++ /dev/null
@@ -1,54 +0,0 @@
-/** @file
-*
-* Copyright (c) 2013-2014, ARM Limited. All rights reserved.
-*
-* This program and the accompanying materials
-* are licensed and made available under the terms and conditions of the BSD License
-* which accompanies this distribution. The full text of the license may be found at
-* http://opensource.org/licenses/bsd-license.php
-*
-* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
-* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
-*
-**/
-
-#ifndef _ARM_GIC_V2_H_
-#define _ARM_GIC_V2_H_
-
-// Interrupts from 1020 to 1023 are considered as special interrupts (eg: spurious interrupts)
-#define ARM_GIC_IS_SPECIAL_INTERRUPTS(Interrupt) (((Interrupt) >= 1020) && ((Interrupt) <= 1023))
-
-VOID
-EFIAPI
-ArmGicV2SetupNonSecure (
- IN UINTN MpId,
- IN INTN GicDistributorBase,
- IN INTN GicInterruptInterfaceBase
- );
-
-VOID
-EFIAPI
-ArmGicV2EnableInterruptInterface (
- IN INTN GicInterruptInterfaceBase
- );
-
-VOID
-EFIAPI
-ArmGicV2DisableInterruptInterface (
- IN INTN GicInterruptInterfaceBase
- );
-
-UINTN
-EFIAPI
-ArmGicV2AcknowledgeInterrupt (
- IN UINTN GicInterruptInterfaceBase
- );
-
-VOID
-EFIAPI
-ArmGicV2EndOfInterrupt (
- IN UINTN GicInterruptInterfaceBase,
- IN UINTN Source
- );
-
-#endif
diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
index 73cac8774056..4afa3d5a09c2 100644
--- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
+++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
@@ -12,8 +12,9 @@
*
**/

+#include <Library/ArmGicLib.h>
+
#include "ArmGicDxe.h"
-#include "GicV3/ArmGicV3Lib.h"

#define ARM_GIC_DEFAULT_PRIORITY 0x80

diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Lib.h b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Lib.h
deleted file mode 100644
index 794e8788a60b..000000000000
--- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Lib.h
+++ /dev/null
@@ -1,68 +0,0 @@
-/** @file
-*
-* Copyright (c) 2014-2015, ARM Limited. All rights reserved.
-*
-* This program and the accompanying materials are licensed and made available
-* under the terms and conditions of the BSD License which accompanies this
-* distribution. The full text of the license may be found at
-* http://opensource.org/licenses/bsd-license.php
-*
-* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
-* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
-*
-**/
-
-#ifndef _ARM_GIC_V3_H_
-#define _ARM_GIC_V3_H_
-
-#define ICC_SRE_EL2_SRE (1 << 0)
-
-#define ARM_GICD_IROUTER_IRM BIT31
-
-UINT32
-EFIAPI
-ArmGicV3GetControlSystemRegisterEnable (
- VOID
- );
-
-VOID
-EFIAPI
-ArmGicV3SetControlSystemRegisterEnable (
- IN UINT32 ControlSystemRegisterEnable
- );
-
-VOID
-EFIAPI
-ArmGicV3EnableInterruptInterface (
- VOID
- );
-
-VOID
-EFIAPI
-ArmGicV3DisableInterruptInterface (
- VOID
- );
-
-UINTN
-EFIAPI
-ArmGicV3AcknowledgeInterrupt (
- VOID
- );
-
-VOID
-EFIAPI
-ArmGicV3EndOfInterrupt (
- IN UINTN Source
- );
-
-VOID
-ArmGicV3SetBinaryPointer (
- IN UINTN BinaryPoint
- );
-
-VOID
-ArmGicV3SetPriorityMask (
- IN UINTN Priority
- );
-
-#endif
diff --git a/ArmPkg/Include/Library/ArmGicLib.h b/ArmPkg/Include/Library/ArmGicLib.h
index e2a4818c4c0c..e3db9c0d250f 100644
--- a/ArmPkg/Include/Library/ArmGicLib.h
+++ b/ArmPkg/Include/Library/ArmGicLib.h
@@ -231,4 +231,98 @@ ArmGicIsInterruptEnabled (
IN UINTN Source
);

+//
+// GIC revision 2 specific declarations
+//
+
+// Interrupts from 1020 to 1023 are considered as special interrupts (eg: spurious interrupts)
+#define ARM_GIC_IS_SPECIAL_INTERRUPTS(Interrupt) (((Interrupt) >= 1020) && ((Interrupt) <= 1023))
+
+VOID
+EFIAPI
+ArmGicV2SetupNonSecure (
+ IN UINTN MpId,
+ IN INTN GicDistributorBase,
+ IN INTN GicInterruptInterfaceBase
+ );
+
+VOID
+EFIAPI
+ArmGicV2EnableInterruptInterface (
+ IN INTN GicInterruptInterfaceBase
+ );
+
+VOID
+EFIAPI
+ArmGicV2DisableInterruptInterface (
+ IN INTN GicInterruptInterfaceBase
+ );
+
+UINTN
+EFIAPI
+ArmGicV2AcknowledgeInterrupt (
+ IN UINTN GicInterruptInterfaceBase
+ );
+
+VOID
+EFIAPI
+ArmGicV2EndOfInterrupt (
+ IN UINTN GicInterruptInterfaceBase,
+ IN UINTN Source
+ );
+
+//
+// GIC revision 3 specific declarations
+//
+
+#define ICC_SRE_EL2_SRE (1 << 0)
+
+#define ARM_GICD_IROUTER_IRM BIT31
+
+UINT32
+EFIAPI
+ArmGicV3GetControlSystemRegisterEnable (
+ VOID
+ );
+
+VOID
+EFIAPI
+ArmGicV3SetControlSystemRegisterEnable (
+ IN UINT32 ControlSystemRegisterEnable
+ );
+
+VOID
+EFIAPI
+ArmGicV3EnableInterruptInterface (
+ VOID
+ );
+
+VOID
+EFIAPI
+ArmGicV3DisableInterruptInterface (
+ VOID
+ );
+
+UINTN
+EFIAPI
+ArmGicV3AcknowledgeInterrupt (
+ VOID
+ );
+
+VOID
+EFIAPI
+ArmGicV3EndOfInterrupt (
+ IN UINTN Source
+ );
+
+VOID
+ArmGicV3SetBinaryPointer (
+ IN UINTN BinaryPoint
+ );
+
+VOID
+ArmGicV3SetPriorityMask (
+ IN UINTN Priority
+ );
+
#endif

[Laszlo Ersek]

Post by Ard Biesheuvel
Before splitting off ArmGicArchLib and moving it out of
ArmPkg/Drivers/ArmGic into ArmPkg/Library, make sure that the
GIC specific declarations it depends on are not hidden away in
local headers "GicV2/GicV2Lib.h" and "GicV3/GicV3Lib.h".
So merge them with <Library/ArmGicLib.h>. This is entirely
appropriate, since this is not a header that declares a public
interface into ArmGicLib, but defines implementation internals.

The code movements in this patch seem okay to me, but I'm unsure about
your claim "Library/ArmGicLib.h [...] is not a header that declares a
public interface into ArmGicLib".

For example, take the function ArmGicSetupNonSecure(). This function is
declared in "ArmPkg/Include/Library/ArmGicLib.h", which one could say is
the library class itself (and your claim is the opposite).

The function is called in "ArmPlatformPkg/Sec/Sec.c" (the library client).

The funcion is implemented in "ArmPkg/Drivers/ArmGic/ArmGicSecLib.c",
which is built as part of "ArmPkg/Drivers/ArmGic/ArmGicSecLib.inf". That
INF file has LIBRARY_CLASS = ArmGicLib.

This kinda looks to counter your claim.

... Hm... on the other hand. Functions in a library class header must be
implemented by all library instances. And, we have another ArmGicLib
instance under

ArmPkg/Drivers/ArmGic/ArmGicLib.inf

that does not build "ArmGicSecLib.c", therefore it will lack an
implementation for ArmGicSetupNonSecure().

In addition, [LibraryClasses.common] in "ArmPkg/ArmPkg.dec" does not
list the header file.

So I guess you're right, "ArmPkg/Include/Library/ArmGicLib.h" is already
a "dumping ground" for declarations of internal-use functions.

Reviewed-by: Laszlo Ersek <***@redhat.com>

Laszlo

Post by Ard Biesheuvel
Contributed-under: TianoCore Contribution Agreement 1.0
---
ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c | 2 -
ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c | 2 -
ArmPkg/Drivers/ArmGic/ArmGicLib.c | 3 -
ArmPkg/Drivers/ArmGic/ArmGicSecLib.c | 2 -
ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c | 3 +-
ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.h | 54 -------
ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 3 +-
ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Lib.h | 68 --------
ArmPkg/Include/Library/ArmGicLib.h | 94 ++++++++++++
9 files changed, 98 insertions(+), 133 deletions(-)
diff --git a/ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c b/ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c
index 88fa4621e613..0e0fa3b9f33e 100644
--- a/ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c
+++ b/ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c
@@ -15,8 +15,6 @@
#include <Library/ArmLib.h>
#include <Library/ArmGicLib.h>
-#include "GicV3/ArmGicV3Lib.h"
-
ARM_GIC_ARCH_REVISION
EFIAPI
ArmGicGetSupportedArchRevision (
diff --git a/ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c b/ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c
index 9ef56efeaa7b..f256de704631 100644
--- a/ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c
+++ b/ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c
@@ -15,8 +15,6 @@
#include <Library/ArmLib.h>
#include <Library/ArmGicLib.h>
-#include "GicV3/ArmGicV3Lib.h"
-
ARM_GIC_ARCH_REVISION
EFIAPI
ArmGicGetSupportedArchRevision (
diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
index 48708e3812b4..248e896c4b94 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
@@ -19,9 +19,6 @@
#include <Library/IoLib.h>
#include <Library/PcdLib.h>
-#include "GicV2/ArmGicV2Lib.h"
-#include "GicV3/ArmGicV3Lib.h"
-
/**
* Return the base address of the GIC redistributor for the current CPU
*
diff --git a/ArmPkg/Drivers/ArmGic/ArmGicSecLib.c b/ArmPkg/Drivers/ArmGic/ArmGicSecLib.c
index 1fdd4d73bd7d..d64806d2f16b 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicSecLib.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicSecLib.c
@@ -17,8 +17,6 @@
#include <Library/IoLib.h>
#include <Library/ArmGicLib.h>
-#include "GicV2/ArmGicV2Lib.h"
-
/*
* This function configures the interrupts set by the mask to be secure.
*
diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
index 743c534e04d9..e649ac1bc6af 100644
--- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
+++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
--*/
+#include <Library/ArmGicLib.h>
+
#include "ArmGicDxe.h"
-#include "GicV2/ArmGicV2Lib.h"
#define ARM_GIC_DEFAULT_PRIORITY 0x80
diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.h b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.h
deleted file mode 100644
index 6803467346a3..000000000000
--- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.h
+++ /dev/null
@@ -1,54 +0,0 @@
-*
-* Copyright (c) 2013-2014, ARM Limited. All rights reserved.
-*
-* This program and the accompanying materials
-* are licensed and made available under the terms and conditions of the BSD License
-* which accompanies this distribution. The full text of the license may be found at
-* http://opensource.org/licenses/bsd-license.php
-*
-* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
-* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
-*
-**/
-
-#ifndef _ARM_GIC_V2_H_
-#define _ARM_GIC_V2_H_
-
-// Interrupts from 1020 to 1023 are considered as special interrupts (eg: spurious interrupts)
-#define ARM_GIC_IS_SPECIAL_INTERRUPTS(Interrupt) (((Interrupt) >= 1020) && ((Interrupt) <= 1023))
-
-VOID
-EFIAPI
-ArmGicV2SetupNonSecure (
- IN UINTN MpId,
- IN INTN GicDistributorBase,
- IN INTN GicInterruptInterfaceBase
- );
-
-VOID
-EFIAPI
-ArmGicV2EnableInterruptInterface (
- IN INTN GicInterruptInterfaceBase
- );
-
-VOID
-EFIAPI
-ArmGicV2DisableInterruptInterface (
- IN INTN GicInterruptInterfaceBase
- );
-
-UINTN
-EFIAPI
-ArmGicV2AcknowledgeInterrupt (
- IN UINTN GicInterruptInterfaceBase
- );
-
-VOID
-EFIAPI
-ArmGicV2EndOfInterrupt (
- IN UINTN GicInterruptInterfaceBase,
- IN UINTN Source
- );
-
-#endif
diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
index 73cac8774056..4afa3d5a09c2 100644
--- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
+++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
@@ -12,8 +12,9 @@
*
**/
+#include <Library/ArmGicLib.h>
+
#include "ArmGicDxe.h"
-#include "GicV3/ArmGicV3Lib.h"
#define ARM_GIC_DEFAULT_PRIORITY 0x80
diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Lib.h b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Lib.h
deleted file mode 100644
index 794e8788a60b..000000000000
--- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Lib.h
+++ /dev/null
@@ -1,68 +0,0 @@
-*
-* Copyright (c) 2014-2015, ARM Limited. All rights reserved.
-*
-* This program and the accompanying materials are licensed and made available
-* under the terms and conditions of the BSD License which accompanies this
-* distribution. The full text of the license may be found at
-* http://opensource.org/licenses/bsd-license.php
-*
-* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
-* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
-*
-**/
-
-#ifndef _ARM_GIC_V3_H_
-#define _ARM_GIC_V3_H_
-
-#define ICC_SRE_EL2_SRE (1 << 0)
-
-#define ARM_GICD_IROUTER_IRM BIT31
-
-UINT32
-EFIAPI
-ArmGicV3GetControlSystemRegisterEnable (
- VOID
- );
-
-VOID
-EFIAPI
-ArmGicV3SetControlSystemRegisterEnable (
- IN UINT32 ControlSystemRegisterEnable
- );
-
-VOID
-EFIAPI
-ArmGicV3EnableInterruptInterface (
- VOID
- );
-
-VOID
-EFIAPI
-ArmGicV3DisableInterruptInterface (
- VOID
- );
-
-UINTN
-EFIAPI
-ArmGicV3AcknowledgeInterrupt (
- VOID
- );
-
-VOID
-EFIAPI
-ArmGicV3EndOfInterrupt (
- IN UINTN Source
- );
-
-VOID
-ArmGicV3SetBinaryPointer (
- IN UINTN BinaryPoint
- );
-
-VOID
-ArmGicV3SetPriorityMask (
- IN UINTN Priority
- );
-
-#endif
diff --git a/ArmPkg/Include/Library/ArmGicLib.h b/ArmPkg/Include/Library/ArmGicLib.h
index e2a4818c4c0c..e3db9c0d250f 100644
--- a/ArmPkg/Include/Library/ArmGicLib.h
+++ b/ArmPkg/Include/Library/ArmGicLib.h
@@ -231,4 +231,98 @@ ArmGicIsInterruptEnabled (
IN UINTN Source
);
+//
+// GIC revision 2 specific declarations
+//
+
+// Interrupts from 1020 to 1023 are considered as special interrupts (eg: spurious interrupts)
+#define ARM_GIC_IS_SPECIAL_INTERRUPTS(Interrupt) (((Interrupt) >= 1020) && ((Interrupt) <= 1023))
+
+VOID
+EFIAPI
+ArmGicV2SetupNonSecure (
+ IN UINTN MpId,
+ IN INTN GicDistributorBase,
+ IN INTN GicInterruptInterfaceBase
+ );
+
+VOID
+EFIAPI
+ArmGicV2EnableInterruptInterface (
+ IN INTN GicInterruptInterfaceBase
+ );
+
+VOID
+EFIAPI
+ArmGicV2DisableInterruptInterface (
+ IN INTN GicInterruptInterfaceBase
+ );
+
+UINTN
+EFIAPI
+ArmGicV2AcknowledgeInterrupt (
+ IN UINTN GicInterruptInterfaceBase
+ );
+
+VOID
+EFIAPI
+ArmGicV2EndOfInterrupt (
+ IN UINTN GicInterruptInterfaceBase,
+ IN UINTN Source
+ );
+
+//
+// GIC revision 3 specific declarations
+//
+
+#define ICC_SRE_EL2_SRE (1 << 0)
+
+#define ARM_GICD_IROUTER_IRM BIT31
+
+UINT32
+EFIAPI
+ArmGicV3GetControlSystemRegisterEnable (
+ VOID
+ );
+
+VOID
+EFIAPI
+ArmGicV3SetControlSystemRegisterEnable (
+ IN UINT32 ControlSystemRegisterEnable
+ );
+
+VOID
+EFIAPI
+ArmGicV3EnableInterruptInterface (
+ VOID
+ );
+
+VOID
+EFIAPI
+ArmGicV3DisableInterruptInterface (
+ VOID
+ );
+
+UINTN
+EFIAPI
+ArmGicV3AcknowledgeInterrupt (
+ VOID
+ );
+
+VOID
+EFIAPI
+ArmGicV3EndOfInterrupt (
+ IN UINTN Source
+ );
+
+VOID
+ArmGicV3SetBinaryPointer (
+ IN UINTN BinaryPoint
+ );
+
+VOID
+ArmGicV3SetPriorityMask (
+ IN UINTN Priority
+ );
+
#endif

------------------------------------------------------------------------------

[Ard Biesheuvel]

Clone ArmGicArchLib into a SEC phase specific ArmGicArchSecLib
so that we can modify the former in a subsequent patch to cache
the GIC revision in a global variable.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <***@linaro.org>
---
ArmPkg/Library/{ArmGicArchLib => ArmGicArchSecLib}/AArch64/ArmGicArchLib.c | 0
ArmPkg/Library/{ArmGicArchLib => ArmGicArchSecLib}/Arm/ArmGicArchLib.c | 0
ArmPkg/Library/{ArmGicArchLib/ArmGicArchLib.inf => ArmGicArchSecLib/ArmGicArchSecLib.inf} | 6 +++---
ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc | 1 +
ArmPlatformPkg/ArmPlatformPkg.dsc | 2 ++
ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc | 2 ++
ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc | 2 ++
7 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchSecLib/AArch64/ArmGicArchLib.c
similarity index 100%
copy from ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
copy to ArmPkg/Library/ArmGicArchSecLib/AArch64/ArmGicArchLib.c
diff --git a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchSecLib/Arm/ArmGicArchLib.c
similarity index 100%
copy from ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
copy to ArmPkg/Library/ArmGicArchSecLib/Arm/ArmGicArchLib.c
diff --git a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf b/ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
similarity index 79%
copy from ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
copy to ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
index d71b2adc3027..a2fb623a8537 100644
--- a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
+++ b/ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
@@ -13,11 +13,11 @@

[Defines]
INF_VERSION = 0x00010005
- BASE_NAME = ArmGicArchLib
- FILE_GUID = cd67f41a-26e9-4482-90c9-a9aff803382a
+ BASE_NAME = ArmGicArchSecLib
+ FILE_GUID = c1dd9745-9459-4e9a-9f5b-99cbd233c27d
MODULE_TYPE = BASE
VERSION_STRING = 1.0
- LIBRARY_CLASS = ArmGicArchLib
+ LIBRARY_CLASS = ArmGicArchLib|SEC

[Sources.ARM]
Arm/ArmGicArchLib.c
diff --git a/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc b/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
index 0859bc31ff00..6e6687c8a735 100644
--- a/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
+++ b/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
@@ -138,6 +138,7 @@
PrePiHobListPointerLib|ArmPlatformPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
PerformanceLib|MdeModulePkg/Library/PeiPerformanceLib/PeiPerformanceLib.inf
PlatformPeiLib|ArmPlatformPkg/PlatformPei/PlatformPeiLib.inf
+ ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf

[LibraryClasses.common.SEC, LibraryClasses.common.PEIM]
MemoryInitPeiLib|ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf
diff --git a/ArmPlatformPkg/ArmPlatformPkg.dsc b/ArmPlatformPkg/ArmPlatformPkg.dsc
index be31025d9bd0..14d82f61ec76 100644
--- a/ArmPlatformPkg/ArmPlatformPkg.dsc
+++ b/ArmPlatformPkg/ArmPlatformPkg.dsc
@@ -134,6 +134,8 @@
DebugAgentLib|ArmPkg/Library/DebugAgentSymbolsBaseLib/DebugAgentSymbolsBaseLib.inf
DefaultExceptionHandlerLib|ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLibBase.inf

+ ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
+
[LibraryClasses.common.SEC, LibraryClasses.common.PEIM]
MemoryInitPeiLib|ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf

diff --git a/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc b/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
index 19de3816ae5c..4b4867f9926a 100644
--- a/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
+++ b/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
@@ -128,6 +128,8 @@
PrePiHobListPointerLib|ArmPlatformPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
!endif

+ ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
+
[LibraryClasses.common.SEC, LibraryClasses.common.PEIM]
MemoryInitPeiLib|ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf

diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
index 84e2a9987f7d..8f7b5f1644de 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
+++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
@@ -144,6 +144,8 @@
# Trustzone Support
ArmTrustedMonitorLib|ArmPlatformPkg/Library/ArmTrustedMonitorLibNull/ArmTrustedMonitorLibNull.inf

+ ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
+
[LibraryClasses.common.PEI_CORE]
HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf

[Laszlo Ersek]

Post by Ard Biesheuvel
Clone ArmGicArchLib into a SEC phase specific ArmGicArchSecLib
so that we can modify the former in a subsequent patch to cache
the GIC revision in a global variable.
Contributed-under: TianoCore Contribution Agreement 1.0
---
ArmPkg/Library/{ArmGicArchLib => ArmGicArchSecLib}/AArch64/ArmGicArchLib.c | 0
ArmPkg/Library/{ArmGicArchLib => ArmGicArchSecLib}/Arm/ArmGicArchLib.c | 0
ArmPkg/Library/{ArmGicArchLib/ArmGicArchLib.inf => ArmGicArchSecLib/ArmGicArchSecLib.inf} | 6 +++---
ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc | 1 +
ArmPlatformPkg/ArmPlatformPkg.dsc | 2 ++
ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc | 2 ++
ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc | 2 ++
7 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchSecLib/AArch64/ArmGicArchLib.c
similarity index 100%
copy from ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
copy to ArmPkg/Library/ArmGicArchSecLib/AArch64/ArmGicArchLib.c
diff --git a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchSecLib/Arm/ArmGicArchLib.c
similarity index 100%
copy from ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
copy to ArmPkg/Library/ArmGicArchSecLib/Arm/ArmGicArchLib.c
diff --git a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf b/ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
similarity index 79%
copy from ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
copy to ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
index d71b2adc3027..a2fb623a8537 100644
--- a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
+++ b/ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
@@ -13,11 +13,11 @@
[Defines]
INF_VERSION = 0x00010005
- BASE_NAME = ArmGicArchLib
- FILE_GUID = cd67f41a-26e9-4482-90c9-a9aff803382a
+ BASE_NAME = ArmGicArchSecLib
+ FILE_GUID = c1dd9745-9459-4e9a-9f5b-99cbd233c27d
MODULE_TYPE = BASE
VERSION_STRING = 1.0
- LIBRARY_CLASS = ArmGicArchLib
+ LIBRARY_CLASS = ArmGicArchLib|SEC

* Okay, so this patch does the clone for SEC modules and points the SEC
library class resolutions in the DSC files to the clone, overriding the
default resolutions in them.

ArmPkg/ArmPkg.dsc and ArmVirtPkg/ArmVirt.dsc.inc are not updated. For
ArmVirtPkg, the default resolution stays intact in this patch, which
makes sense, because patch #7 handles it separately.

But, is there any particular reason to leave ArmPkg/ArmPkg.dsc unchanged
in this patch? Hm... I guess it should be safe, because patch #5 is
explicit about DXE_DRIVER, UEFI_DRIVER, UEFI_APPLICATION; so if there's
a problem later, it will show up as a build time failure. Okay.

* Anyway, my main concern was PEIMs. Especially because in patch #3 you
wrote,

Post by Ard Biesheuvel
this statelessness is only needed for SEC type modules

This is in general not true, because PEIMs also may execute-in-place
from flash. However, from patch #5 I can see that the caching version
will be available only to DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION,
leaving PEIMs uncovered -- which is safe, and also good enough since
(apparently) no PEIMs depend on this library class.

If you have to respin the series for any reason, please consider
mentioning PEIMs in the commit message of patch #3 and patch #4, and
making ArmGicArchSecLib available to PEIMs too. However, as I said
above, that would be quite speculative, and this could be addressed any
time later, if a new PEIM actually needed the library. So feel free to
ignore this note.

Reviewed-by: Laszlo Ersek <***@redhat.com>

Thanks
Laszlo

Post by Ard Biesheuvel
[Sources.ARM]
Arm/ArmGicArchLib.c
diff --git a/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc b/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
index 0859bc31ff00..6e6687c8a735 100644
--- a/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
+++ b/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
@@ -138,6 +138,7 @@
PrePiHobListPointerLib|ArmPlatformPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
PerformanceLib|MdeModulePkg/Library/PeiPerformanceLib/PeiPerformanceLib.inf
PlatformPeiLib|ArmPlatformPkg/PlatformPei/PlatformPeiLib.inf
+ ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
[LibraryClasses.common.SEC, LibraryClasses.common.PEIM]
MemoryInitPeiLib|ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf
diff --git a/ArmPlatformPkg/ArmPlatformPkg.dsc b/ArmPlatformPkg/ArmPlatformPkg.dsc
index be31025d9bd0..14d82f61ec76 100644
--- a/ArmPlatformPkg/ArmPlatformPkg.dsc
+++ b/ArmPlatformPkg/ArmPlatformPkg.dsc
@@ -134,6 +134,8 @@
DebugAgentLib|ArmPkg/Library/DebugAgentSymbolsBaseLib/DebugAgentSymbolsBaseLib.inf
DefaultExceptionHandlerLib|ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLibBase.inf
+ ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
+
[LibraryClasses.common.SEC, LibraryClasses.common.PEIM]
MemoryInitPeiLib|ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf
diff --git a/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc b/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
index 19de3816ae5c..4b4867f9926a 100644
--- a/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
+++ b/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
@@ -128,6 +128,8 @@
PrePiHobListPointerLib|ArmPlatformPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
!endif
+ ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
+
[LibraryClasses.common.SEC, LibraryClasses.common.PEIM]
MemoryInitPeiLib|ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf
diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
index 84e2a9987f7d..8f7b5f1644de 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
+++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
@@ -144,6 +144,8 @@
# Trustzone Support
ArmTrustedMonitorLib|ArmPlatformPkg/Library/ArmTrustedMonitorLibNull/ArmTrustedMonitorLibNull.inf
+ ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
+
[LibraryClasses.common.PEI_CORE]
HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf

------------------------------------------------------------------------------

[Ard Biesheuvel]

Post by Laszlo Ersek

Post by Ard Biesheuvel
Clone ArmGicArchLib into a SEC phase specific ArmGicArchSecLib
so that we can modify the former in a subsequent patch to cache
the GIC revision in a global variable.
Contributed-under: TianoCore Contribution Agreement 1.0
---
ArmPkg/Library/{ArmGicArchLib => ArmGicArchSecLib}/AArch64/ArmGicArchLib.c | 0
ArmPkg/Library/{ArmGicArchLib => ArmGicArchSecLib}/Arm/ArmGicArchLib.c | 0
ArmPkg/Library/{ArmGicArchLib/ArmGicArchLib.inf => ArmGicArchSecLib/ArmGicArchSecLib.inf} | 6 +++---
ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc | 1 +
ArmPlatformPkg/ArmPlatformPkg.dsc | 2 ++
ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc | 2 ++
ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc | 2 ++
7 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchSecLib/AArch64/ArmGicArchLib.c
similarity index 100%
copy from ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
copy to ArmPkg/Library/ArmGicArchSecLib/AArch64/ArmGicArchLib.c
diff --git a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchSecLib/Arm/ArmGicArchLib.c
similarity index 100%
copy from ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
copy to ArmPkg/Library/ArmGicArchSecLib/Arm/ArmGicArchLib.c
diff --git a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf b/ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
similarity index 79%
copy from ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
copy to ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
index d71b2adc3027..a2fb623a8537 100644
--- a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
+++ b/ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
@@ -13,11 +13,11 @@
[Defines]
INF_VERSION = 0x00010005
- BASE_NAME = ArmGicArchLib
- FILE_GUID = cd67f41a-26e9-4482-90c9-a9aff803382a
+ BASE_NAME = ArmGicArchSecLib
+ FILE_GUID = c1dd9745-9459-4e9a-9f5b-99cbd233c27d
MODULE_TYPE = BASE
VERSION_STRING = 1.0
- LIBRARY_CLASS = ArmGicArchLib
+ LIBRARY_CLASS = ArmGicArchLib|SEC

* Okay, so this patch does the clone for SEC modules and points the SEC
library class resolutions in the DSC files to the clone, overriding the
default resolutions in them.
ArmPkg/ArmPkg.dsc and ArmVirtPkg/ArmVirt.dsc.inc are not updated. For
ArmVirtPkg, the default resolution stays intact in this patch, which
makes sense, because patch #7 handles it separately.
But, is there any particular reason to leave ArmPkg/ArmPkg.dsc unchanged
in this patch?

Actually, that is an oversight. I should probably add it there for
completeness, although the exact purpose of these package .DSCs
escapes me, to be honest.
Anyway, ArmPkg.dsc still builds ok with these changes, so I assume
there is no SEC module being built that [transitively] relies on
ArmGicArchLib

Post by Laszlo Ersek
Hm... I guess it should be safe, because patch #5 is
explicit about DXE_DRIVER, UEFI_DRIVER, UEFI_APPLICATION; so if there's
a problem later, it will show up as a build time failure. Okay.
* Anyway, my main concern was PEIMs. Especially because in patch #3 you
wrote,

Post by Ard Biesheuvel
this statelessness is only needed for SEC type modules

This is in general not true, because PEIMs also may execute-in-place
from flash. However, from patch #5 I can see that the caching version
will be available only to DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION,
leaving PEIMs uncovered -- which is safe, and also good enough since
(apparently) no PEIMs depend on this library class.

i have you on record (and I can dig up the email, if you like) saying
that you prefer the LIBRARY_CLASS phase modifiers to be minimal.

Post by Laszlo Ersek
If you have to respin the series for any reason, please consider
mentioning PEIMs in the commit message of patch #3 and patch #4, and
making ArmGicArchSecLib available to PEIMs too. However, as I said
above, that would be quite speculative, and this could be addressed any
time later, if a new PEIM actually needed the library. So feel free to
ignore this note.

Thanks,
Ard,

Post by Laszlo Ersek

Post by Ard Biesheuvel
[Sources.ARM]
Arm/ArmGicArchLib.c
diff --git a/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc b/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
index 0859bc31ff00..6e6687c8a735 100644
--- a/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
+++ b/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
@@ -138,6 +138,7 @@
PrePiHobListPointerLib|ArmPlatformPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
PerformanceLib|MdeModulePkg/Library/PeiPerformanceLib/PeiPerformanceLib.inf
PlatformPeiLib|ArmPlatformPkg/PlatformPei/PlatformPeiLib.inf
+ ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
[LibraryClasses.common.SEC, LibraryClasses.common.PEIM]
MemoryInitPeiLib|ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf
diff --git a/ArmPlatformPkg/ArmPlatformPkg.dsc b/ArmPlatformPkg/ArmPlatformPkg.dsc
index be31025d9bd0..14d82f61ec76 100644
--- a/ArmPlatformPkg/ArmPlatformPkg.dsc
+++ b/ArmPlatformPkg/ArmPlatformPkg.dsc
@@ -134,6 +134,8 @@
DebugAgentLib|ArmPkg/Library/DebugAgentSymbolsBaseLib/DebugAgentSymbolsBaseLib.inf
DefaultExceptionHandlerLib|ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLibBase.inf
+ ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
+
[LibraryClasses.common.SEC, LibraryClasses.common.PEIM]
MemoryInitPeiLib|ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf
diff --git a/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc b/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
index 19de3816ae5c..4b4867f9926a 100644
--- a/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
+++ b/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
@@ -128,6 +128,8 @@
PrePiHobListPointerLib|ArmPlatformPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
!endif
+ ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
+
[LibraryClasses.common.SEC, LibraryClasses.common.PEIM]
MemoryInitPeiLib|ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf
diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
index 84e2a9987f7d..8f7b5f1644de 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
+++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
@@ -144,6 +144,8 @@
# Trustzone Support
ArmTrustedMonitorLib|ArmPlatformPkg/Library/ArmTrustedMonitorLibNull/ArmTrustedMonitorLibNull.inf
+ ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
+
[LibraryClasses.common.PEI_CORE]
HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf

------------------------------------------------------------------------------

[Laszlo Ersek]

Post by Ard Biesheuvel

Post by Laszlo Ersek

Post by Ard Biesheuvel
Clone ArmGicArchLib into a SEC phase specific ArmGicArchSecLib
so that we can modify the former in a subsequent patch to cache
the GIC revision in a global variable.
Contributed-under: TianoCore Contribution Agreement 1.0
---
ArmPkg/Library/{ArmGicArchLib => ArmGicArchSecLib}/AArch64/ArmGicArchLib.c | 0
ArmPkg/Library/{ArmGicArchLib => ArmGicArchSecLib}/Arm/ArmGicArchLib.c | 0
ArmPkg/Library/{ArmGicArchLib/ArmGicArchLib.inf => ArmGicArchSecLib/ArmGicArchSecLib.inf} | 6 +++---
ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc | 1 +
ArmPlatformPkg/ArmPlatformPkg.dsc | 2 ++
ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc | 2 ++
ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc | 2 ++
7 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchSecLib/AArch64/ArmGicArchLib.c
similarity index 100%
copy from ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
copy to ArmPkg/Library/ArmGicArchSecLib/AArch64/ArmGicArchLib.c
diff --git a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchSecLib/Arm/ArmGicArchLib.c
similarity index 100%
copy from ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
copy to ArmPkg/Library/ArmGicArchSecLib/Arm/ArmGicArchLib.c
diff --git a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf b/ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
similarity index 79%
copy from ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
copy to ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
index d71b2adc3027..a2fb623a8537 100644
--- a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
+++ b/ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
@@ -13,11 +13,11 @@
[Defines]
INF_VERSION = 0x00010005
- BASE_NAME = ArmGicArchLib
- FILE_GUID = cd67f41a-26e9-4482-90c9-a9aff803382a
+ BASE_NAME = ArmGicArchSecLib
+ FILE_GUID = c1dd9745-9459-4e9a-9f5b-99cbd233c27d
MODULE_TYPE = BASE
VERSION_STRING = 1.0
- LIBRARY_CLASS = ArmGicArchLib
+ LIBRARY_CLASS = ArmGicArchLib|SEC

* Okay, so this patch does the clone for SEC modules and points the SEC
library class resolutions in the DSC files to the clone, overriding the
default resolutions in them.
ArmPkg/ArmPkg.dsc and ArmVirtPkg/ArmVirt.dsc.inc are not updated. For
ArmVirtPkg, the default resolution stays intact in this patch, which
makes sense, because patch #7 handles it separately.
But, is there any particular reason to leave ArmPkg/ArmPkg.dsc unchanged
in this patch?

Actually, that is an oversight. I should probably add it there for
completeness, although the exact purpose of these package .DSCs
escapes me, to be honest.

Me too. :)

Post by Ard Biesheuvel
Anyway, ArmPkg.dsc still builds ok with these changes, so I assume
there is no SEC module being built that [transitively] relies on
ArmGicArchLib

Right.

Post by Ard Biesheuvel

Post by Laszlo Ersek
Hm... I guess it should be safe, because patch #5 is
explicit about DXE_DRIVER, UEFI_DRIVER, UEFI_APPLICATION; so if there's
a problem later, it will show up as a build time failure. Okay.
* Anyway, my main concern was PEIMs. Especially because in patch #3 you
wrote,

Post by Ard Biesheuvel
this statelessness is only needed for SEC type modules

This is in general not true, because PEIMs also may execute-in-place
from flash. However, from patch #5 I can see that the caching version
will be available only to DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION,
leaving PEIMs uncovered -- which is safe, and also good enough since
(apparently) no PEIMs depend on this library class.

i have you on record (and I can dig up the email, if you like) saying
that you prefer the LIBRARY_CLASS phase modifiers to be minimal.

Do you also have me on record for never contradicting myself? ;)

So, don't bother.

Cheers
Laszlo

Post by Ard Biesheuvel

Post by Laszlo Ersek
If you have to respin the series for any reason, please consider
mentioning PEIMs in the commit message of patch #3 and patch #4, and
making ArmGicArchSecLib available to PEIMs too. However, as I said
above, that would be quite speculative, and this could be addressed any
time later, if a new PEIM actually needed the library. So feel free to
ignore this note.

Thanks,
Ard,

Post by Laszlo Ersek

Post by Ard Biesheuvel
[Sources.ARM]
Arm/ArmGicArchLib.c
diff --git a/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc b/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
index 0859bc31ff00..6e6687c8a735 100644
--- a/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
+++ b/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
@@ -138,6 +138,7 @@
PrePiHobListPointerLib|ArmPlatformPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
PerformanceLib|MdeModulePkg/Library/PeiPerformanceLib/PeiPerformanceLib.inf
PlatformPeiLib|ArmPlatformPkg/PlatformPei/PlatformPeiLib.inf
+ ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
[LibraryClasses.common.SEC, LibraryClasses.common.PEIM]
MemoryInitPeiLib|ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf
diff --git a/ArmPlatformPkg/ArmPlatformPkg.dsc b/ArmPlatformPkg/ArmPlatformPkg.dsc
index be31025d9bd0..14d82f61ec76 100644
--- a/ArmPlatformPkg/ArmPlatformPkg.dsc
+++ b/ArmPlatformPkg/ArmPlatformPkg.dsc
@@ -134,6 +134,8 @@
DebugAgentLib|ArmPkg/Library/DebugAgentSymbolsBaseLib/DebugAgentSymbolsBaseLib.inf
DefaultExceptionHandlerLib|ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLibBase.inf
+ ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
+
[LibraryClasses.common.SEC, LibraryClasses.common.PEIM]
MemoryInitPeiLib|ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf
diff --git a/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc b/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
index 19de3816ae5c..4b4867f9926a 100644
--- a/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
+++ b/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
@@ -128,6 +128,8 @@
PrePiHobListPointerLib|ArmPlatformPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
!endif
+ ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
+
[LibraryClasses.common.SEC, LibraryClasses.common.PEIM]
MemoryInitPeiLib|ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf
diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
index 84e2a9987f7d..8f7b5f1644de 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
+++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
@@ -144,6 +144,8 @@
# Trustzone Support
ArmTrustedMonitorLib|ArmPlatformPkg/Library/ArmTrustedMonitorLibNull/ArmTrustedMonitorLibNull.inf
+ ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
+
[LibraryClasses.common.PEI_CORE]
HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf

------------------------------------------------------------------------------

[Ard Biesheuvel]

Instead of inferring the GIC revision from the CPU id registers
and the presence/availability of the system register interface
upon each invocation, move the logic to a constructor and cache
the result.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <***@linaro.org>
---
ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c | 23 ++++++++++--
ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c | 23 ++++++++++--
ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf | 3 +-
3 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
index 0e0fa3b9f33e..9853c7ba8566 100644
--- a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
+++ b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
@@ -15,9 +15,11 @@
#include <Library/ArmLib.h>
#include <Library/ArmGicLib.h>

-ARM_GIC_ARCH_REVISION
+STATIC ARM_GIC_ARCH_REVISION mGicArchRevision;
+
+RETURN_STATUS
EFIAPI
-ArmGicGetSupportedArchRevision (
+ArmGicArchLibInitialize (
VOID
)
{
@@ -43,9 +45,22 @@ ArmGicGetSupportedArchRevision (
IccSre = ArmGicV3GetControlSystemRegisterEnable ();
}
if (IccSre & ICC_SRE_EL2_SRE) {
- return ARM_GIC_ARCH_REVISION_3;
+ mGicArchRevision = ARM_GIC_ARCH_REVISION_3;
+ goto Done;
}
}

- return ARM_GIC_ARCH_REVISION_2;
+ 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/Arm/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
index f256de704631..f8822a224580 100644
--- a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
+++ b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
@@ -15,9 +15,11 @@
#include <Library/ArmLib.h>
#include <Library/ArmGicLib.h>

-ARM_GIC_ARCH_REVISION
+STATIC ARM_GIC_ARCH_REVISION mGicArchRevision;
+
+RETURN_STATUS
EFIAPI
-ArmGicGetSupportedArchRevision (
+ArmGicArchLibInitialize (
VOID
)
{
@@ -43,9 +45,22 @@ ArmGicGetSupportedArchRevision (
IccSre = ArmGicV3GetControlSystemRegisterEnable ();
}
if (IccSre & ICC_SRE_EL2_SRE) {
- return ARM_GIC_ARCH_REVISION_3;
+ mGicArchRevision = ARM_GIC_ARCH_REVISION_3;
+ goto Done;
}
}

- return ARM_GIC_ARCH_REVISION_2;
+ 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/ArmGicArchLib.inf b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
index d71b2adc3027..7dbcb08f50d6 100644
--- a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
+++ b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
@@ -17,7 +17,8 @@
FILE_GUID = cd67f41a-26e9-4482-90c9-a9aff803382a
MODULE_TYPE = BASE
VERSION_STRING = 1.0
- LIBRARY_CLASS = ArmGicArchLib
+ LIBRARY_CLASS = ArmGicArchLib|DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION
+ CONSTRUCTOR = ArmGicArchLibInitialize

[Sources.ARM]
Arm/ArmGicArchLib.c

[Laszlo Ersek]

Post by Ard Biesheuvel
Instead of inferring the GIC revision from the CPU id registers
and the presence/availability of the system register interface
upon each invocation, move the logic to a constructor and cache
the result.
Contributed-under: TianoCore Contribution Agreement 1.0
---
ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c | 23 ++++++++++--
ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c | 23 ++++++++++--
ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf | 3 +-
3 files changed, 40 insertions(+), 9 deletions(-)
diff --git a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
index 0e0fa3b9f33e..9853c7ba8566 100644
--- a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
+++ b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
@@ -15,9 +15,11 @@
#include <Library/ArmLib.h>
#include <Library/ArmGicLib.h>
-ARM_GIC_ARCH_REVISION
+STATIC ARM_GIC_ARCH_REVISION mGicArchRevision;
+
+RETURN_STATUS
EFIAPI
-ArmGicGetSupportedArchRevision (
+ArmGicArchLibInitialize (
VOID
)
{
@@ -43,9 +45,22 @@ ArmGicGetSupportedArchRevision (
IccSre = ArmGicV3GetControlSystemRegisterEnable ();
}
if (IccSre & ICC_SRE_EL2_SRE) {
- return ARM_GIC_ARCH_REVISION_3;
+ mGicArchRevision = ARM_GIC_ARCH_REVISION_3;
+ goto Done;
}
}
- return ARM_GIC_ARCH_REVISION_2;
+ mGicArchRevision = ARM_GIC_ARCH_REVISION_2;
+
+ return RETURN_SUCCESS;
+}
+
+ARM_GIC_ARCH_REVISION
+EFIAPI
+ArmGicGetSupportedArchRevision (
+ VOID
+ )
+{
+ return mGicArchRevision;
}
diff --git a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
index f256de704631..f8822a224580 100644
--- a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
+++ b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
@@ -15,9 +15,11 @@
#include <Library/ArmLib.h>
#include <Library/ArmGicLib.h>
-ARM_GIC_ARCH_REVISION
+STATIC ARM_GIC_ARCH_REVISION mGicArchRevision;
+
+RETURN_STATUS
EFIAPI
-ArmGicGetSupportedArchRevision (
+ArmGicArchLibInitialize (
VOID
)
{
@@ -43,9 +45,22 @@ ArmGicGetSupportedArchRevision (
IccSre = ArmGicV3GetControlSystemRegisterEnable ();
}
if (IccSre & ICC_SRE_EL2_SRE) {
- return ARM_GIC_ARCH_REVISION_3;
+ mGicArchRevision = ARM_GIC_ARCH_REVISION_3;
+ goto Done;
}
}
- return ARM_GIC_ARCH_REVISION_2;
+ mGicArchRevision = ARM_GIC_ARCH_REVISION_2;
+
+ return RETURN_SUCCESS;
+}
+
+ARM_GIC_ARCH_REVISION
+EFIAPI
+ArmGicGetSupportedArchRevision (
+ VOID
+ )
+{
+ return mGicArchRevision;
}
diff --git a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
index d71b2adc3027..7dbcb08f50d6 100644
--- a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
+++ b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
@@ -17,7 +17,8 @@
FILE_GUID = cd67f41a-26e9-4482-90c9-a9aff803382a
MODULE_TYPE = BASE
VERSION_STRING = 1.0
- LIBRARY_CLASS = ArmGicArchLib
+ LIBRARY_CLASS = ArmGicArchLib|DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION
+ CONSTRUCTOR = ArmGicArchLibInitialize
[Sources.ARM]
Arm/ArmGicArchLib.c

I trust that you rebuilt all of the DSCs that use this (now restricted)
library instance via their default library class resolutions, and there
were no errors (ie. no other referring module types than spelled out
here) :)

Code-wise, it looks fine.

Reviewed-by: Laszlo Ersek <***@redhat.com>

I think I'm finished reviewing the still pending patches. At the end of
this series, we have three instances for the library class:

ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf:
LIBRARY_CLASS = ArmGicArchLib|SEC

ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf:
LIBRARY_CLASS = ArmGicArchLib|DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION

ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf:
LIBRARY_CLASS = ArmGicArchLib|DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION

The first one (without caching) is used for SEC modules in:
- ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
- ArmPlatformPkg/ArmPlatformPkg.dsc
- ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
- ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc

The second one (with caching in a static variable) is used as the
default resolution in:
- ArmPkg/ArmPkg.dsc
- ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
- ArmPlatformPkg/ArmPlatformPkg.dsc
- ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
- ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc

The third one (also with caching in a static variable, but taken from a
PCD, not probed from hardware) is used as the default resolution in:
- ArmVirtPkg/ArmVirt.dsc.inc

Looks good!

Thanks
Laszlo

------------------------------------------------------------------------------

[Ard Biesheuvel]

Post by Laszlo Ersek

Post by Ard Biesheuvel
Instead of inferring the GIC revision from the CPU id registers
and the presence/availability of the system register interface
upon each invocation, move the logic to a constructor and cache
the result.
Contributed-under: TianoCore Contribution Agreement 1.0
---
ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c | 23 ++++++++++--
ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c | 23 ++++++++++--
ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf | 3 +-
3 files changed, 40 insertions(+), 9 deletions(-)
diff --git a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
index 0e0fa3b9f33e..9853c7ba8566 100644
--- a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
+++ b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
@@ -15,9 +15,11 @@
#include <Library/ArmLib.h>
#include <Library/ArmGicLib.h>
-ARM_GIC_ARCH_REVISION
+STATIC ARM_GIC_ARCH_REVISION mGicArchRevision;
+
+RETURN_STATUS
EFIAPI
-ArmGicGetSupportedArchRevision (
+ArmGicArchLibInitialize (
VOID
)
{
@@ -43,9 +45,22 @@ ArmGicGetSupportedArchRevision (
IccSre = ArmGicV3GetControlSystemRegisterEnable ();
}
if (IccSre & ICC_SRE_EL2_SRE) {
- return ARM_GIC_ARCH_REVISION_3;
+ mGicArchRevision = ARM_GIC_ARCH_REVISION_3;
+ goto Done;
}
}
- return ARM_GIC_ARCH_REVISION_2;
+ mGicArchRevision = ARM_GIC_ARCH_REVISION_2;
+
+ return RETURN_SUCCESS;
+}
+
+ARM_GIC_ARCH_REVISION
+EFIAPI
+ArmGicGetSupportedArchRevision (
+ VOID
+ )
+{
+ return mGicArchRevision;
}
diff --git a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
index f256de704631..f8822a224580 100644
--- a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
+++ b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
@@ -15,9 +15,11 @@
#include <Library/ArmLib.h>
#include <Library/ArmGicLib.h>
-ARM_GIC_ARCH_REVISION
+STATIC ARM_GIC_ARCH_REVISION mGicArchRevision;
+
+RETURN_STATUS
EFIAPI
-ArmGicGetSupportedArchRevision (
+ArmGicArchLibInitialize (
VOID
)
{
@@ -43,9 +45,22 @@ ArmGicGetSupportedArchRevision (
IccSre = ArmGicV3GetControlSystemRegisterEnable ();
}
if (IccSre & ICC_SRE_EL2_SRE) {
- return ARM_GIC_ARCH_REVISION_3;
+ mGicArchRevision = ARM_GIC_ARCH_REVISION_3;
+ goto Done;
}
}
- return ARM_GIC_ARCH_REVISION_2;
+ mGicArchRevision = ARM_GIC_ARCH_REVISION_2;
+
+ return RETURN_SUCCESS;
+}
+
+ARM_GIC_ARCH_REVISION
+EFIAPI
+ArmGicGetSupportedArchRevision (
+ VOID
+ )
+{
+ return mGicArchRevision;
}
diff --git a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
index d71b2adc3027..7dbcb08f50d6 100644
--- a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
+++ b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
@@ -17,7 +17,8 @@
FILE_GUID = cd67f41a-26e9-4482-90c9-a9aff803382a
MODULE_TYPE = BASE
VERSION_STRING = 1.0
- LIBRARY_CLASS = ArmGicArchLib
+ LIBRARY_CLASS = ArmGicArchLib|DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION
+ CONSTRUCTOR = ArmGicArchLibInitialize
[Sources.ARM]
Arm/ArmGicArchLib.c

I trust that you rebuilt all of the DSCs that use this (now restricted)
library instance via their default library class resolutions, and there
were no errors (ie. no other referring module types than spelled out
here) :)
Code-wise, it looks fine.
I think I'm finished reviewing the still pending patches. At the end of
LIBRARY_CLASS = ArmGicArchLib|SEC
LIBRARY_CLASS = ArmGicArchLib|DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION
LIBRARY_CLASS = ArmGicArchLib|DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION
- ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
- ArmPlatformPkg/ArmPlatformPkg.dsc
- ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
- ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
The second one (with caching in a static variable) is used as the
- ArmPkg/ArmPkg.dsc
- ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
- ArmPlatformPkg/ArmPlatformPkg.dsc
- ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
- ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
The third one (also with caching in a static variable, but taken from a
- ArmVirtPkg/ArmVirt.dsc.inc
Looks good!

Thanks for another elaborate review.

It turns out that Olivier is on vacation until next Monday, so
hopefully he will get around to reviewing these patches soon after.

[Ard Biesheuvel]

Since it is arguably incorrect to infer the GIC revision from CPU
ID and GIC feature registers on platforms that describe the GIC in
the device tree, this implements the library class ArmGicArchLib
tailored for such platforms.

The supported GIC revision is retrieved from the dynamic PCD that
is set based on the GIC DT node.

This means this library can only execute post DXE core, but this is
not a problem for any of the virt platforms.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <***@linaro.org>
---
ArmVirtPkg/ArmVirt.dsc.inc | 2 +-
ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c | 75 ++++++++++++
ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf | 40 ++++++
3 files changed, 116 insertions(+), 1 deletion(-)

diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index 900d5c3d9dd0..fd20ff39a068 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -71,7 +71,7 @@
ArmDisassemblerLib|ArmPkg/Library/ArmDisassemblerLib/ArmDisassemblerLib.inf
DmaLib|ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
ArmGicLib|ArmPkg/Drivers/ArmGic/ArmGicLib.inf
- ArmGicArchLib|ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
+ ArmGicArchLib|ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf
ArmPlatformStackLib|ArmPlatformPkg/Library/ArmPlatformStackLib/ArmPlatformStackLib.inf
ArmSmcLib|ArmPkg/Library/ArmSmcLib/ArmSmcLib.inf
ArmHvcLib|ArmPkg/Library/ArmHvcLib/ArmHvcLib.inf
diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
new file mode 100644
index 000000000000..732860cadfe6
--- /dev/null
+++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
@@ -0,0 +1,75 @@
+/** @file
+ ArmGicArchLib library class implementation for DT based virt platforms
+
+ Copyright (c) 2015, Linaro Ltd. All rights reserved.<BR>
+
+ This program and the accompanying materials
+ are licensed and made available under the terms and conditions of the BSD License
+ which accompanies this distribution. The full text of the license may be found at
+ http://opensource.org/licenses/bsd-license.php
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include <Base.h>
+
+#include <Library/ArmGicLib.h>
+#include <Library/ArmGicArchLib.h>
+#include <Library/PcdLib.h>
+#include <Library/DebugLib.h>
+
+STATIC ARM_GIC_ARCH_REVISION mGicArchRevision;
+
+RETURN_STATUS
+EFIAPI
+ArmVirtGicArchLibConstructor (
+ VOID
+ )
+{
+ UINT32 IccSre;
+
+ switch (PcdGet32 (PcdArmGicRevision)) {
+
+ case 3:
+ //
+ // The default implementation of ArmGicArchLib is responsible for enabling
+ // the system register interface on the GICv3 if one is found. So let's do
+ // the same here.
+ //
+ IccSre = ArmGicV3GetControlSystemRegisterEnable ();
+ if (!(IccSre & ICC_SRE_EL2_SRE)) {
+ ArmGicV3SetControlSystemRegisterEnable (IccSre | ICC_SRE_EL2_SRE);
+ IccSre = ArmGicV3GetControlSystemRegisterEnable ();
+ }
+
+ //
+ // Unlike the default implementation, there is no fall through to GICv2
+ // mode if this GICv3 cannot be driven in native mode due to the fact
+ // that the System Register interface is unavailable.
+ //
+ ASSERT (IccSre & ICC_SRE_EL2_SRE);
+
+ mGicArchRevision = ARM_GIC_ARCH_REVISION_3;
+ break;
+
+ case 2:
+ mGicArchRevision = ARM_GIC_ARCH_REVISION_2;
+ break;
+
+ default:
+ DEBUG ((EFI_D_ERROR, "%a: No GIC revision specified!\n", __FUNCTION__));
+ return RETURN_NOT_FOUND;
+ }
+ return RETURN_SUCCESS;
+}
+
+ARM_GIC_ARCH_REVISION
+EFIAPI
+ArmGicGetSupportedArchRevision (
+ VOID
+ )
+{
+ return mGicArchRevision;
+}
diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf
new file mode 100644
index 000000000000..c85b2d44d856
--- /dev/null
+++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf
@@ -0,0 +1,40 @@
+#/** @file
+#
+# Component description file for ArmVirtGicArchLib module
+#
+# Copyright (c) 2015, Linaro Ltd. All rights reserved.<BR>
+#
+# This program and the accompanying materials
+# are licensed and made available under the terms and conditions of the BSD License
+# which accompanies this distribution. The full text of the license may be found at
+# http://opensource.org/licenses/bsd-license.php
+#
+# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+#
+#**/
+
+[Defines]
+ INF_VERSION = 0x00010005
+ BASE_NAME = ArmVirtGicArchLib
+ FILE_GUID = 87b0dc84-4661-4deb-a789-97977ff636ed
+ MODULE_TYPE = BASE
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = ArmGicArchLib|DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION
+ CONSTRUCTOR = ArmVirtGicArchLibConstructor
+
+[Sources]
+ ArmVirtGicArchLib.c
+
+[LibraryClasses]
+ PcdLib
+ DebugLib
+ ArmGicLib
+
+[Packages]
+ MdePkg/MdePkg.dec
+ ArmPkg/ArmPkg.dec
+ ArmVirtPkg/ArmVirtPkg.dec
+
+[Pcd]
+ gArmVirtTokenSpaceGuid.PcdArmGicRevision

[Laszlo Ersek]

one question below

Post by Ard Biesheuvel
Since it is arguably incorrect to infer the GIC revision from CPU
ID and GIC feature registers on platforms that describe the GIC in
the device tree, this implements the library class ArmGicArchLib
tailored for such platforms.
The supported GIC revision is retrieved from the dynamic PCD that
is set based on the GIC DT node.
This means this library can only execute post DXE core, but this is
not a problem for any of the virt platforms.
Contributed-under: TianoCore Contribution Agreement 1.0
---
ArmVirtPkg/ArmVirt.dsc.inc | 2 +-
ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c | 75 ++++++++++++
ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf | 40 ++++++
3 files changed, 116 insertions(+), 1 deletion(-)
diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index 900d5c3d9dd0..fd20ff39a068 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -71,7 +71,7 @@
ArmDisassemblerLib|ArmPkg/Library/ArmDisassemblerLib/ArmDisassemblerLib.inf
DmaLib|ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
ArmGicLib|ArmPkg/Drivers/ArmGic/ArmGicLib.inf
- ArmGicArchLib|ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
+ ArmGicArchLib|ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf
ArmPlatformStackLib|ArmPlatformPkg/Library/ArmPlatformStackLib/ArmPlatformStackLib.inf
ArmSmcLib|ArmPkg/Library/ArmSmcLib/ArmSmcLib.inf
ArmHvcLib|ArmPkg/Library/ArmHvcLib/ArmHvcLib.inf
diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
new file mode 100644
index 000000000000..732860cadfe6
--- /dev/null
+++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
@@ -0,0 +1,75 @@
+ ArmGicArchLib library class implementation for DT based virt platforms
+
+ Copyright (c) 2015, Linaro Ltd. All rights reserved.<BR>
+
+ This program and the accompanying materials
+ are licensed and made available under the terms and conditions of the BSD License
+ which accompanies this distribution. The full text of the license may be found at
+ http://opensource.org/licenses/bsd-license.php
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include <Base.h>
+
+#include <Library/ArmGicLib.h>
+#include <Library/ArmGicArchLib.h>
+#include <Library/PcdLib.h>
+#include <Library/DebugLib.h>
+
+STATIC ARM_GIC_ARCH_REVISION mGicArchRevision;
+
+RETURN_STATUS
+EFIAPI
+ArmVirtGicArchLibConstructor (
+ VOID
+ )
+{
+ UINT32 IccSre;
+
+ switch (PcdGet32 (PcdArmGicRevision)) {
+
+ //
+ // The default implementation of ArmGicArchLib is responsible for enabling
+ // the system register interface on the GICv3 if one is found. So let's do
+ // the same here.
+ //
+ IccSre = ArmGicV3GetControlSystemRegisterEnable ();

Are you satisfied with the performance improvements of this version? The
ArmReadIdPfr0() call has been eliminated indeed, but
ArmGicV3GetControlSystemRegisterEnable() is still called by each binary
that is linked against this library.

Since in the blurb you said

Post by Ard Biesheuvel
the detection code runs very often, and is quite
heavy-weight when running under virtualization (especially KVM)

I'm uncertain about the perf improvements here. The frequency of the
code is exactly the same (*), so any significant savings would have to
come from eliminating ArmReadIdPfr0().

If that's a big enough win, then:

Reviewed-by: Laszlo Ersek <***@redhat.com>

Otherwise, you could move ArmGicV3GetControlSystemRegisterEnable() to
VirtFdtDxe (patch #6), and call it when you see PropertyTypeGicV3. It
wouldn't be very nice, but certainly not uglier than the
VirtioMmioInstallDevice() calls! :) That's just what VirtFdtDxe does.

(*) Hm, wait, I missed something. I skipped patches 1-5, but now I can
see that in "ArmPkg/Library/ArmGicArchSecLib/AArch64/ArmGicArchLib.c",
it's actually ArmGicGetSupportedArchRevision() that contains all of the
detection code. That may indeed be a big change in frequency, so I can
imagine the big wins. Thus, my R-b is unconditional after all.

Tell me when this part of the series is ready for being applied. (Hm,
actually, you could push them yourself!)

Thanks
Laszlo

Post by Ard Biesheuvel
+ if (!(IccSre & ICC_SRE_EL2_SRE)) {
+ ArmGicV3SetControlSystemRegisterEnable (IccSre | ICC_SRE_EL2_SRE);
+ IccSre = ArmGicV3GetControlSystemRegisterEnable ();
+ }
+
+ //
+ // Unlike the default implementation, there is no fall through to GICv2
+ // mode if this GICv3 cannot be driven in native mode due to the fact
+ // that the System Register interface is unavailable.
+ //
+ ASSERT (IccSre & ICC_SRE_EL2_SRE);
+
+ mGicArchRevision = ARM_GIC_ARCH_REVISION_3;
+ break;
+
+ mGicArchRevision = ARM_GIC_ARCH_REVISION_2;
+ break;
+
+ DEBUG ((EFI_D_ERROR, "%a: No GIC revision specified!\n", __FUNCTION__));
+ return RETURN_NOT_FOUND;
+ }
+ return RETURN_SUCCESS;
+}
+
+ARM_GIC_ARCH_REVISION
+EFIAPI
+ArmGicGetSupportedArchRevision (
+ VOID
+ )
+{
+ return mGicArchRevision;
+}
diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf
new file mode 100644
index 000000000000..c85b2d44d856
--- /dev/null
+++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf
@@ -0,0 +1,40 @@
+#
+# Component description file for ArmVirtGicArchLib module
+#
+# Copyright (c) 2015, Linaro Ltd. All rights reserved.<BR>
+#
+# This program and the accompanying materials
+# are licensed and made available under the terms and conditions of the BSD License
+# which accompanies this distribution. The full text of the license may be found at
+# http://opensource.org/licenses/bsd-license.php
+#
+# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+#
+#**/
+
+[Defines]
+ INF_VERSION = 0x00010005
+ BASE_NAME = ArmVirtGicArchLib
+ FILE_GUID = 87b0dc84-4661-4deb-a789-97977ff636ed
+ MODULE_TYPE = BASE
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = ArmGicArchLib|DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION
+ CONSTRUCTOR = ArmVirtGicArchLibConstructor
+
+[Sources]
+ ArmVirtGicArchLib.c
+
+[LibraryClasses]
+ PcdLib
+ DebugLib
+ ArmGicLib
+
+[Packages]
+ MdePkg/MdePkg.dec
+ ArmPkg/ArmPkg.dec
+ ArmVirtPkg/ArmVirtPkg.dec
+
+[Pcd]
+ gArmVirtTokenSpaceGuid.PcdArmGicRevision

------------------------------------------------------------------------------

[Ard Biesheuvel]

Post by Laszlo Ersek
one question below

Post by Ard Biesheuvel
Since it is arguably incorrect to infer the GIC revision from CPU
ID and GIC feature registers on platforms that describe the GIC in
the device tree, this implements the library class ArmGicArchLib
tailored for such platforms.
The supported GIC revision is retrieved from the dynamic PCD that
is set based on the GIC DT node.
This means this library can only execute post DXE core, but this is
not a problem for any of the virt platforms.
Contributed-under: TianoCore Contribution Agreement 1.0
---
ArmVirtPkg/ArmVirt.dsc.inc | 2 +-
ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c | 75 ++++++++++++
ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf | 40 ++++++
3 files changed, 116 insertions(+), 1 deletion(-)
diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index 900d5c3d9dd0..fd20ff39a068 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -71,7 +71,7 @@
ArmDisassemblerLib|ArmPkg/Library/ArmDisassemblerLib/ArmDisassemblerLib.inf
DmaLib|ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
ArmGicLib|ArmPkg/Drivers/ArmGic/ArmGicLib.inf
- ArmGicArchLib|ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
+ ArmGicArchLib|ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf
ArmPlatformStackLib|ArmPlatformPkg/Library/ArmPlatformStackLib/ArmPlatformStackLib.inf
ArmSmcLib|ArmPkg/Library/ArmSmcLib/ArmSmcLib.inf
ArmHvcLib|ArmPkg/Library/ArmHvcLib/ArmHvcLib.inf
diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
new file mode 100644
index 000000000000..732860cadfe6
--- /dev/null
+++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
@@ -0,0 +1,75 @@
+ ArmGicArchLib library class implementation for DT based virt platforms
+
+ Copyright (c) 2015, Linaro Ltd. All rights reserved.<BR>
+
+ This program and the accompanying materials
+ are licensed and made available under the terms and conditions of the BSD License
+ which accompanies this distribution. The full text of the license may be found at
+ http://opensource.org/licenses/bsd-license.php
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include <Base.h>
+
+#include <Library/ArmGicLib.h>
+#include <Library/ArmGicArchLib.h>
+#include <Library/PcdLib.h>
+#include <Library/DebugLib.h>
+
+STATIC ARM_GIC_ARCH_REVISION mGicArchRevision;
+
+RETURN_STATUS
+EFIAPI
+ArmVirtGicArchLibConstructor (
+ VOID
+ )
+{
+ UINT32 IccSre;
+
+ switch (PcdGet32 (PcdArmGicRevision)) {
+
+ //
+ // The default implementation of ArmGicArchLib is responsible for enabling
+ // the system register interface on the GICv3 if one is found. So let's do
+ // the same here.
+ //
+ IccSre = ArmGicV3GetControlSystemRegisterEnable ();

Are you satisfied with the performance improvements of this version? The
ArmReadIdPfr0() call has been eliminated indeed, but
ArmGicV3GetControlSystemRegisterEnable() is still called by each binary
that is linked against this library.
Since in the blurb you said

Post by Ard Biesheuvel
the detection code runs very often, and is quite
heavy-weight when running under virtualization (especially KVM)

I'm uncertain about the perf improvements here. The frequency of the
code is exactly the same (*), so any significant savings would have to
come from eliminating ArmReadIdPfr0().
Otherwise, you could move ArmGicV3GetControlSystemRegisterEnable() to
VirtFdtDxe (patch #6), and call it when you see PropertyTypeGicV3. It
wouldn't be very nice, but certainly not uglier than the
VirtioMmioInstallDevice() calls! :) That's just what VirtFdtDxe does.
(*) Hm, wait, I missed something. I skipped patches 1-5, but now I can
see that in "ArmPkg/Library/ArmGicArchSecLib/AArch64/ArmGicArchLib.c",
it's actually ArmGicGetSupportedArchRevision() that contains all of the
detection code. That may indeed be a big change in frequency, so I can
imagine the big wins. Thus, my R-b is unconditional after all.

Indeed. This series is slightly redundant since it
- reduces the sysreg access frequency for all versions
- adds the caching for the new non-SEC version
- uses the DT for the virt targets

and any two out of those three would largely eliminate the issue.
However, since the DT based one is actually more correct as well, I
decided to propose all three, and decide how to proceed based on the
discussion that follows. (Re correctness: the current code would break
when emulating an out-of-kernel GICv3 irqchip under KVM on hardware
that has GICv2 hardware only. This is purely theoretical though)

Post by Laszlo Ersek
Tell me when this part of the series is ready for being applied. (Hm,
actually, you could push them yourself!)

I will wait for Olivier and Leif to comment on the remaining patches.
We could drop the ArmGicArchSecLib patches, but I need the split off
in #3 for this patch to do anything meaningful.
--
Ard.

Post by Laszlo Ersek

Post by Ard Biesheuvel
+ if (!(IccSre & ICC_SRE_EL2_SRE)) {
+ ArmGicV3SetControlSystemRegisterEnable (IccSre | ICC_SRE_EL2_SRE);
+ IccSre = ArmGicV3GetControlSystemRegisterEnable ();
+ }
+
+ //
+ // Unlike the default implementation, there is no fall through to GICv2
+ // mode if this GICv3 cannot be driven in native mode due to the fact
+ // that the System Register interface is unavailable.
+ //
+ ASSERT (IccSre & ICC_SRE_EL2_SRE);
+
+ mGicArchRevision = ARM_GIC_ARCH_REVISION_3;
+ break;
+
+ mGicArchRevision = ARM_GIC_ARCH_REVISION_2;
+ break;
+
+ DEBUG ((EFI_D_ERROR, "%a: No GIC revision specified!\n", __FUNCTION__));
+ return RETURN_NOT_FOUND;
+ }
+ return RETURN_SUCCESS;
+}
+
+ARM_GIC_ARCH_REVISION
+EFIAPI
+ArmGicGetSupportedArchRevision (
+ VOID
+ )
+{
+ return mGicArchRevision;
+}
diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf
new file mode 100644
index 000000000000..c85b2d44d856
--- /dev/null
+++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf
@@ -0,0 +1,40 @@
+#
+# Component description file for ArmVirtGicArchLib module
+#
+# Copyright (c) 2015, Linaro Ltd. All rights reserved.<BR>
+#
+# This program and the accompanying materials
+# are licensed and made available under the terms and conditions of the BSD License
+# which accompanies this distribution. The full text of the license may be found at
+# http://opensource.org/licenses/bsd-license.php
+#
+# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+#
+#**/
+
+[Defines]
+ INF_VERSION = 0x00010005
+ BASE_NAME = ArmVirtGicArchLib
+ FILE_GUID = 87b0dc84-4661-4deb-a789-97977ff636ed
+ MODULE_TYPE = BASE
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = ArmGicArchLib|DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION
+ CONSTRUCTOR = ArmVirtGicArchLibConstructor
+
+[Sources]
+ ArmVirtGicArchLib.c
+
+[LibraryClasses]
+ PcdLib
+ DebugLib
+ ArmGicLib
+
+[Packages]
+ MdePkg/MdePkg.dec
+ ArmPkg/ArmPkg.dec
+ ArmVirtPkg/ArmVirtPkg.dec
+
+[Pcd]
+ gArmVirtTokenSpaceGuid.PcdArmGicRevision

------------------------------------------------------------------------------

[Ard Biesheuvel]

In order to allow a ArmGicArchLib to be implemented that returns
the supported GIC revision based on the device tree, add handling
to VirtFdtDxe to record the GIC revision at DT parsing time.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <***@linaro.org>
---
ArmVirtPkg/ArmVirtPkg.dec | 7 ++++++-
ArmVirtPkg/ArmVirtQemu.dsc | 1 +
ArmVirtPkg/ArmVirtXen.dsc | 1 +
ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c | 2 ++
ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf | 1 +
5 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
index 7bbd9fff570e..bf8de4db97be 100644
--- a/ArmVirtPkg/ArmVirtPkg.dec
+++ b/ArmVirtPkg/ArmVirtPkg.dec
@@ -61,6 +61,11 @@
gArmVirtTokenSpaceGuid.PcdFwCfgSelectorAddress|0x0|UINT64|0x00000004
gArmVirtTokenSpaceGuid.PcdFwCfgDataAddress|0x0|UINT64|0x00000005

+ #
+ # Supported GIC revision (2, 3, ...)
+ #
+ gArmVirtTokenSpaceGuid.PcdArmGicRevision|0x0|UINT32|0x00000006
+
[PcdsFeatureFlag]
#
# "Map PCI MMIO as Cached"
@@ -84,4 +89,4 @@
#
# The default is to turn off the kludge; DSC's can selectively enable it.
#
- gArmVirtTokenSpaceGuid.PcdKludgeMapPciMmioAsCached|FALSE|BOOLEAN|0x00000006
+ gArmVirtTokenSpaceGuid.PcdKludgeMapPciMmioAsCached|FALSE|BOOLEAN|0x00000007
diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 374cf7a9ee02..592f07ee6d38 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -192,6 +192,7 @@
gArmTokenSpaceGuid.PcdGicDistributorBase|0x0
gArmTokenSpaceGuid.PcdGicRedistributorsBase|0x0
gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase|0x0
+ gArmVirtTokenSpaceGuid.PcdArmGicRevision|0x0

## PL031 RealTimeClock
gArmPlatformTokenSpaceGuid.PcdPL031RtcBase|0x0
diff --git a/ArmVirtPkg/ArmVirtXen.dsc b/ArmVirtPkg/ArmVirtXen.dsc
index 3a53debedd2c..b4007ff3c6a4 100644
--- a/ArmVirtPkg/ArmVirtXen.dsc
+++ b/ArmVirtPkg/ArmVirtXen.dsc
@@ -127,6 +127,7 @@
gArmTokenSpaceGuid.PcdGicDistributorBase|0x0
gArmTokenSpaceGuid.PcdGicRedistributorsBase|0x0
gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase|0x0
+ gArmVirtTokenSpaceGuid.PcdArmGicRevision|0x0

## PL031 RealTimeClock
gArmPlatformTokenSpaceGuid.PcdPL031RtcBase|0x0
diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
index c9a181a871b8..73db63078ffb 100644
--- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
+++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
@@ -441,6 +441,7 @@ InitializeVirtFdtDxe (

PcdSet32 (PcdGicDistributorBase, (UINT32)DistBase);
PcdSet32 (PcdGicInterruptInterfaceBase, (UINT32)CpuBase);
+ PcdSet32 (PcdArmGicRevision, 2);

DEBUG ((EFI_D_INFO, "Found GIC @ 0x%Lx/0x%Lx\n", DistBase, CpuBase));
break;
@@ -470,6 +471,7 @@ InitializeVirtFdtDxe (

PcdSet32 (PcdGicDistributorBase, (UINT32)DistBase);
PcdSet32 (PcdGicRedistributorsBase, (UINT32)RedistBase);
+ PcdSet32 (PcdArmGicRevision, 3);

DEBUG ((EFI_D_INFO, "Found GIC v3 (re)distributor @ 0x%Lx (0x%Lx)\n",
DistBase, RedistBase));
diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
index 3477db039f93..657b4e88019d 100644
--- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
+++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
@@ -53,6 +53,7 @@
gArmVirtTokenSpaceGuid.PcdArmPsciMethod
gArmVirtTokenSpaceGuid.PcdFwCfgSelectorAddress
gArmVirtTokenSpaceGuid.PcdFwCfgDataAddress
+ gArmVirtTokenSpaceGuid.PcdArmGicRevision
gArmTokenSpaceGuid.PcdGicDistributorBase
gArmTokenSpaceGuid.PcdGicRedistributorsBase
gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase

[Laszlo Ersek]

Post by Ard Biesheuvel
In order to allow a ArmGicArchLib to be implemented that returns
the supported GIC revision based on the device tree, add handling
to VirtFdtDxe to record the GIC revision at DT parsing time.
Contributed-under: TianoCore Contribution Agreement 1.0
---
ArmVirtPkg/ArmVirtPkg.dec | 7 ++++++-
ArmVirtPkg/ArmVirtQemu.dsc | 1 +
ArmVirtPkg/ArmVirtXen.dsc | 1 +
ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c | 2 ++
ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf | 1 +
5 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
index 7bbd9fff570e..bf8de4db97be 100644
--- a/ArmVirtPkg/ArmVirtPkg.dec
+++ b/ArmVirtPkg/ArmVirtPkg.dec
@@ -61,6 +61,11 @@
gArmVirtTokenSpaceGuid.PcdFwCfgSelectorAddress|0x0|UINT64|0x00000004
gArmVirtTokenSpaceGuid.PcdFwCfgDataAddress|0x0|UINT64|0x00000005
+ #
+ # Supported GIC revision (2, 3, ...)
+ #
+ gArmVirtTokenSpaceGuid.PcdArmGicRevision|0x0|UINT32|0x00000006
+
[PcdsFeatureFlag]
#
# "Map PCI MMIO as Cached"
@@ -84,4 +89,4 @@
#
# The default is to turn off the kludge; DSC's can selectively enable it.
#
- gArmVirtTokenSpaceGuid.PcdKludgeMapPciMmioAsCached|FALSE|BOOLEAN|0x00000006
+ gArmVirtTokenSpaceGuid.PcdKludgeMapPciMmioAsCached|FALSE|BOOLEAN|0x00000007

This is not a problem, but usually we don't shift preexistent PCD
numbers, just locate the highest value in the DEC file, and assign the
next one to the next PCD.

Post by Ard Biesheuvel
diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 374cf7a9ee02..592f07ee6d38 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -192,6 +192,7 @@
gArmTokenSpaceGuid.PcdGicDistributorBase|0x0
gArmTokenSpaceGuid.PcdGicRedistributorsBase|0x0
gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase|0x0
+ gArmVirtTokenSpaceGuid.PcdArmGicRevision|0x0
## PL031 RealTimeClock
gArmPlatformTokenSpaceGuid.PcdPL031RtcBase|0x0
diff --git a/ArmVirtPkg/ArmVirtXen.dsc b/ArmVirtPkg/ArmVirtXen.dsc
index 3a53debedd2c..b4007ff3c6a4 100644
--- a/ArmVirtPkg/ArmVirtXen.dsc
+++ b/ArmVirtPkg/ArmVirtXen.dsc
@@ -127,6 +127,7 @@
gArmTokenSpaceGuid.PcdGicDistributorBase|0x0
gArmTokenSpaceGuid.PcdGicRedistributorsBase|0x0
gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase|0x0
+ gArmVirtTokenSpaceGuid.PcdArmGicRevision|0x0
## PL031 RealTimeClock
gArmPlatformTokenSpaceGuid.PcdPL031RtcBase|0x0
diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
index c9a181a871b8..73db63078ffb 100644
--- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
+++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
@@ -441,6 +441,7 @@ InitializeVirtFdtDxe (
PcdSet32 (PcdGicDistributorBase, (UINT32)DistBase);
PcdSet32 (PcdGicInterruptInterfaceBase, (UINT32)CpuBase);
+ PcdSet32 (PcdArmGicRevision, 2);
break;
@@ -470,6 +471,7 @@ InitializeVirtFdtDxe (
PcdSet32 (PcdGicDistributorBase, (UINT32)DistBase);
PcdSet32 (PcdGicRedistributorsBase, (UINT32)RedistBase);
+ PcdSet32 (PcdArmGicRevision, 3);
DistBase, RedistBase));
diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
index 3477db039f93..657b4e88019d 100644
--- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
+++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
@@ -53,6 +53,7 @@
gArmVirtTokenSpaceGuid.PcdArmPsciMethod
gArmVirtTokenSpaceGuid.PcdFwCfgSelectorAddress
gArmVirtTokenSpaceGuid.PcdFwCfgDataAddress
+ gArmVirtTokenSpaceGuid.PcdArmGicRevision
gArmTokenSpaceGuid.PcdGicDistributorBase
gArmTokenSpaceGuid.PcdGicRedistributorsBase
gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase

Reviewed-by: Laszlo Ersek <***@redhat.com>

------------------------------------------------------------------------------

[Ard Biesheuvel]

Post by Laszlo Ersek

Post by Ard Biesheuvel
In order to allow a ArmGicArchLib to be implemented that returns
the supported GIC revision based on the device tree, add handling
to VirtFdtDxe to record the GIC revision at DT parsing time.
Contributed-under: TianoCore Contribution Agreement 1.0
---
ArmVirtPkg/ArmVirtPkg.dec | 7 ++++++-
ArmVirtPkg/ArmVirtQemu.dsc | 1 +
ArmVirtPkg/ArmVirtXen.dsc | 1 +
ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c | 2 ++
ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf | 1 +
5 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
index 7bbd9fff570e..bf8de4db97be 100644
--- a/ArmVirtPkg/ArmVirtPkg.dec
+++ b/ArmVirtPkg/ArmVirtPkg.dec
@@ -61,6 +61,11 @@
gArmVirtTokenSpaceGuid.PcdFwCfgSelectorAddress|0x0|UINT64|0x00000004
gArmVirtTokenSpaceGuid.PcdFwCfgDataAddress|0x0|UINT64|0x00000005
+ #
+ # Supported GIC revision (2, 3, ...)
+ #
+ gArmVirtTokenSpaceGuid.PcdArmGicRevision|0x0|UINT32|0x00000006
+
[PcdsFeatureFlag]
#
# "Map PCI MMIO as Cached"
@@ -84,4 +89,4 @@
#
# The default is to turn off the kludge; DSC's can selectively enable it.
#
- gArmVirtTokenSpaceGuid.PcdKludgeMapPciMmioAsCached|FALSE|BOOLEAN|0x00000006
+ gArmVirtTokenSpaceGuid.PcdKludgeMapPciMmioAsCached|FALSE|BOOLEAN|0x00000007

This is not a problem, but usually we don't shift preexistent PCD
numbers, just locate the highest value in the DEC file, and assign the
next one to the next PCD.

OK, I wondered about that. I will leave it like this for now, but I
won't bother next time.

Post by Laszlo Ersek

Post by Ard Biesheuvel
diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 374cf7a9ee02..592f07ee6d38 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -192,6 +192,7 @@
gArmTokenSpaceGuid.PcdGicDistributorBase|0x0
gArmTokenSpaceGuid.PcdGicRedistributorsBase|0x0
gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase|0x0
+ gArmVirtTokenSpaceGuid.PcdArmGicRevision|0x0
## PL031 RealTimeClock
gArmPlatformTokenSpaceGuid.PcdPL031RtcBase|0x0
diff --git a/ArmVirtPkg/ArmVirtXen.dsc b/ArmVirtPkg/ArmVirtXen.dsc
index 3a53debedd2c..b4007ff3c6a4 100644
--- a/ArmVirtPkg/ArmVirtXen.dsc
+++ b/ArmVirtPkg/ArmVirtXen.dsc
@@ -127,6 +127,7 @@
gArmTokenSpaceGuid.PcdGicDistributorBase|0x0
gArmTokenSpaceGuid.PcdGicRedistributorsBase|0x0
gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase|0x0
+ gArmVirtTokenSpaceGuid.PcdArmGicRevision|0x0
## PL031 RealTimeClock
gArmPlatformTokenSpaceGuid.PcdPL031RtcBase|0x0
diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
index c9a181a871b8..73db63078ffb 100644
--- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
+++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
@@ -441,6 +441,7 @@ InitializeVirtFdtDxe (
PcdSet32 (PcdGicDistributorBase, (UINT32)DistBase);
PcdSet32 (PcdGicInterruptInterfaceBase, (UINT32)CpuBase);
+ PcdSet32 (PcdArmGicRevision, 2);
break;
@@ -470,6 +471,7 @@ InitializeVirtFdtDxe (
PcdSet32 (PcdGicDistributorBase, (UINT32)DistBase);
PcdSet32 (PcdGicRedistributorsBase, (UINT32)RedistBase);
+ PcdSet32 (PcdArmGicRevision, 3);
DistBase, RedistBase));
diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
index 3477db039f93..657b4e88019d 100644
--- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
+++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
@@ -53,6 +53,7 @@
gArmVirtTokenSpaceGuid.PcdArmPsciMethod
gArmVirtTokenSpaceGuid.PcdFwCfgSelectorAddress
gArmVirtTokenSpaceGuid.PcdFwCfgDataAddress
+ gArmVirtTokenSpaceGuid.PcdArmGicRevision
gArmTokenSpaceGuid.PcdGicDistributorBase
gArmTokenSpaceGuid.PcdGicRedistributorsBase
gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase

Thanks,
Ard.

------------------------------------------------------------------------------

[Ard Biesheuvel]

Post by Ard Biesheuvel
The current GIC revision detection code infers the GIC revision from
the AA64PFR0_EL1.GIC feature bit that tells us whether the GIC system
register interface is implemented in the hardware, and then proceeds
to attempt and enable it.
The library containing this code deliberately does not cache the
detected revision since it may execute in the SEC phase and may
be running from NOR flash and not RAM.
However, since the detection code runs very often, and is quite
heavy-weight when running under virtualization (especially KVM),
- use fewer system register accesses if possible
- provide an alternative that does cache the detected revision
- use the DT supplied revision when executing on a DT based platform
ArmPkg: reduce sysreg access count in GIC revision probe
ArmPkg: merge ArmGicV[23]Lib.h into ArmGicLib.h
ArmPkg: split off ArmGicArchLib from ArmGicLib
ArmPkg: copy ArmGicArchLib to ArmGicArchSecLib
ArmPkg: cache detected revision in ArmGicArchLib
ArmVirtPkg: record GIC revision in dynamic PCD
ArmVirtPkg: implement DT-based ArmGicArchLib

Hello Olivier,

May we please have your input regarding patches #2 and #3 in this
series? Those are prerequisites for patch #7, which is essentially
both a performance and a correctness fix for ArmVirtPkg.
(Patches #4 and #5 only affect bare-metal platforms, and are not
depended upon by subsequent patches, but I'd still like your opinion
on those as well)

[Ard Biesheuvel]

Post by Ard Biesheuvel

Post by Ard Biesheuvel
The current GIC revision detection code infers the GIC revision from
the AA64PFR0_EL1.GIC feature bit that tells us whether the GIC system
register interface is implemented in the hardware, and then proceeds
to attempt and enable it.
The library containing this code deliberately does not cache the
detected revision since it may execute in the SEC phase and may
be running from NOR flash and not RAM.
However, since the detection code runs very often, and is quite
heavy-weight when running under virtualization (especially KVM),
- use fewer system register accesses if possible
- provide an alternative that does cache the detected revision
- use the DT supplied revision when executing on a DT based platform
ArmPkg: reduce sysreg access count in GIC revision probe
ArmPkg: merge ArmGicV[23]Lib.h into ArmGicLib.h
ArmPkg: split off ArmGicArchLib from ArmGicLib
ArmPkg: copy ArmGicArchLib to ArmGicArchSecLib
ArmPkg: cache detected revision in ArmGicArchLib
ArmVirtPkg: record GIC revision in dynamic PCD
ArmVirtPkg: implement DT-based ArmGicArchLib

Hello Olivier,
May we please have your input regarding patches #2 and #3 in this
series? Those are prerequisites for patch #7, which is essentially
both a performance and a correctness fix for ArmVirtPkg.
(Patches #4 and #5 only affect bare-metal platforms, and are not
depended upon by subsequent patches, but I'd still like your opinion
on those as well)

OK, I will go ahead and submit patch #1, which Olivier has already
reviewed, and should reduce the number of sysreg accesses (and thus
the number of world switches under KVM) by two thirds. But I would
still like some feedbacks (and acks/r-b) on patches #2 - #5 please?

Regards,
Ard.

------------------------------------------------------------------------------