[PATCH 1/1] ArmPlatformPkg: Remove AP support from PrePi/PrePeiCore


Rebecca Cran <quic_rcran@...>
 

Modern platforms use TF-A, so there's no need for support of
secondary cores in EDK2 since TF-A will keep them in a holding
pen until the PSCI_CPU_ON SMC call is received.

Therefore, remove the code that handles secondary CPUs from
PrePeiCore and PrePi and add ASSERTs if a secondary core
reaches the functions.

Signed-off-by: Rebecca Cran <rebecca@...>
---
ArmPlatformPkg/PrePeiCore/MainMPCore.c | 92 --------------------
ArmPlatformPkg/PrePeiCore/MainUniCore.c | 9 --
ArmPlatformPkg/PrePeiCore/PrePeiCore.c | 37 ++++----
ArmPlatformPkg/PrePi/MainMPCore.c | 69 ---------------
ArmPlatformPkg/PrePi/MainUniCore.c | 9 --
ArmPlatformPkg/PrePi/PrePi.c | 36 ++++----
6 files changed, 34 insertions(+), 218 deletions(-)

diff --git a/ArmPlatformPkg/PrePeiCore/MainMPCore.c b/ArmPlatformPkg/PrePeiCore/MainMPCore.c
index b5d0d3a6442f..44850a4f3946 100644
--- a/ArmPlatformPkg/PrePeiCore/MainMPCore.c
+++ b/ArmPlatformPkg/PrePeiCore/MainMPCore.c
@@ -12,98 +12,6 @@

#include "PrePeiCore.h"

-/*
- * This is the main function for secondary cores. They loop around until a non Null value is written to
- * SYS_FLAGS register.The SYS_FLAGS register is platform specific.
- * Note:The secondary cores, while executing secondary_main, assumes that:
- * : SGI 0 is configured as Non-secure interrupt
- * : Priority Mask is configured to allow SGI 0
- * : Interrupt Distributor and CPU interfaces are enabled
- *
- */
-VOID
-EFIAPI
-SecondaryMain (
- IN UINTN MpId
- )
-{
- EFI_STATUS Status;
- UINTN PpiListSize;
- UINTN PpiListCount;
- EFI_PEI_PPI_DESCRIPTOR *PpiList;
- ARM_MP_CORE_INFO_PPI *ArmMpCoreInfoPpi;
- UINTN Index;
- UINTN ArmCoreCount;
- ARM_CORE_INFO *ArmCoreInfoTable;
- UINT32 ClusterId;
- UINT32 CoreId;
-
- VOID (*SecondaryStart)(
- VOID
- );
- UINTN SecondaryEntryAddr;
- UINTN AcknowledgeInterrupt;
- UINTN InterruptId;
-
- ClusterId = GET_CLUSTER_ID (MpId);
- CoreId = GET_CORE_ID (MpId);
-
- // Get the gArmMpCoreInfoPpiGuid
- PpiListSize = 0;
- ArmPlatformGetPlatformPpiList (&PpiListSize, &PpiList);
- PpiListCount = PpiListSize / sizeof (EFI_PEI_PPI_DESCRIPTOR);
- for (Index = 0; Index < PpiListCount; Index++, PpiList++) {
- if (CompareGuid (PpiList->Guid, &gArmMpCoreInfoPpiGuid) == TRUE) {
- break;
- }
- }
-
- // On MP Core Platform we must implement the ARM MP Core Info PPI
- ASSERT (Index != PpiListCount);
-
- ArmMpCoreInfoPpi = PpiList->Ppi;
- ArmCoreCount = 0;
- Status = ArmMpCoreInfoPpi->GetMpCoreInfo (&ArmCoreCount, &ArmCoreInfoTable);
- ASSERT_EFI_ERROR (Status);
-
- // Find the core in the ArmCoreTable
- for (Index = 0; Index < ArmCoreCount; Index++) {
- if ((GET_MPIDR_AFF1 (ArmCoreInfoTable[Index].Mpidr) == ClusterId) &&
- (GET_MPIDR_AFF0 (ArmCoreInfoTable[Index].Mpidr) == CoreId))
- {
- break;
- }
- }
-
- // The ARM Core Info Table must define every core
- ASSERT (Index != ArmCoreCount);
-
- // Clear Secondary cores MailBox
- MmioWrite32 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue);
-
- do {
- ArmCallWFI ();
-
- // Read the Mailbox
- SecondaryEntryAddr = MmioRead32 (ArmCoreInfoTable[Index].MailboxGetAddress);
-
- // Acknowledge the interrupt and send End of Interrupt signal.
- AcknowledgeInterrupt = ArmGicAcknowledgeInterrupt (PcdGet64 (PcdGicInterruptInterfaceBase), &InterruptId);
- // Check if it is a valid interrupt ID
- if (InterruptId < ArmGicGetMaxNumInterrupts (PcdGet64 (PcdGicDistributorBase))) {
- // Got a valid SGI number hence signal End of Interrupt
- ArmGicEndOfInterrupt (PcdGet64 (PcdGicInterruptInterfaceBase), AcknowledgeInterrupt);
- }
- } while (SecondaryEntryAddr == 0);
-
- // Jump to secondary core entry point.
- SecondaryStart = (VOID (*)()) SecondaryEntryAddr;
- SecondaryStart ();
-
- // The secondaries shouldn't reach here
- ASSERT (FALSE);
-}
-
VOID
EFIAPI
PrimaryMain (
diff --git a/ArmPlatformPkg/PrePeiCore/MainUniCore.c b/ArmPlatformPkg/PrePeiCore/MainUniCore.c
index 1c2580eb923b..3d3c6caaa32a 100644
--- a/ArmPlatformPkg/PrePeiCore/MainUniCore.c
+++ b/ArmPlatformPkg/PrePeiCore/MainUniCore.c
@@ -8,15 +8,6 @@

#include "PrePeiCore.h"

-VOID
-EFIAPI
-SecondaryMain (
- IN UINTN MpId
- )
-{
- ASSERT (FALSE);
-}
-
VOID
EFIAPI
PrimaryMain (
diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
index 42a7ccc9c6a0..64d1ef601ea3 100644
--- a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
+++ b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
@@ -117,27 +117,26 @@ CEntryPoint (

// Note: The MMU will be enabled by MemoryPeim. Only the primary core will have the MMU on.

- // If not primary Jump to Secondary Main
- if (ArmPlatformIsPrimaryCore (MpId)) {
- // Invoke "ProcessLibraryConstructorList" to have all library constructors
- // called.
- ProcessLibraryConstructorList ();
-
- PrintFirmwareVersion ();
-
- // Initialize the Debug Agent for Source Level Debugging
- InitializeDebugAgent (DEBUG_AGENT_INIT_POSTMEM_SEC, NULL, NULL);
- SaveAndSetDebugTimerInterrupt (TRUE);
-
- // Initialize the platform specific controllers
- ArmPlatformInitialize (MpId);
-
- // Goto primary Main.
- PrimaryMain (PeiCoreEntryPoint);
- } else {
- SecondaryMain (MpId);
+ if (!ArmPlatformIsPrimaryCore (MpId)) {
+ ASSERT (FALSE);
}

+ // Invoke "ProcessLibraryConstructorList" to have all library constructors
+ // called.
+ ProcessLibraryConstructorList ();
+
+ PrintFirmwareVersion ();
+
+ // Initialize the Debug Agent for Source Level Debugging
+ InitializeDebugAgent (DEBUG_AGENT_INIT_POSTMEM_SEC, NULL, NULL);
+ SaveAndSetDebugTimerInterrupt (TRUE);
+
+ // Initialize the platform specific controllers
+ ArmPlatformInitialize (MpId);
+
+ // Goto primary Main.
+ PrimaryMain (PeiCoreEntryPoint);
+
// PEI Core should always load and never return
ASSERT (FALSE);
}
diff --git a/ArmPlatformPkg/PrePi/MainMPCore.c b/ArmPlatformPkg/PrePi/MainMPCore.c
index 68a7c13298d0..ce7058a2846f 100644
--- a/ArmPlatformPkg/PrePi/MainMPCore.c
+++ b/ArmPlatformPkg/PrePi/MainMPCore.c
@@ -33,72 +33,3 @@ PrimaryMain (
// We must never return
ASSERT (FALSE);
}
-
-VOID
-SecondaryMain (
- IN UINTN MpId
- )
-{
- EFI_STATUS Status;
- ARM_MP_CORE_INFO_PPI *ArmMpCoreInfoPpi;
- UINTN Index;
- UINTN ArmCoreCount;
- ARM_CORE_INFO *ArmCoreInfoTable;
- UINT32 ClusterId;
- UINT32 CoreId;
-
- VOID (*SecondaryStart)(
- VOID
- );
- UINTN SecondaryEntryAddr;
- UINTN AcknowledgeInterrupt;
- UINTN InterruptId;
-
- ClusterId = GET_CLUSTER_ID (MpId);
- CoreId = GET_CORE_ID (MpId);
-
- // On MP Core Platform we must implement the ARM MP Core Info PPI (gArmMpCoreInfoPpiGuid)
- Status = GetPlatformPpi (&gArmMpCoreInfoPpiGuid, (VOID **)&ArmMpCoreInfoPpi);
- ASSERT_EFI_ERROR (Status);
-
- ArmCoreCount = 0;
- Status = ArmMpCoreInfoPpi->GetMpCoreInfo (&ArmCoreCount, &ArmCoreInfoTable);
- ASSERT_EFI_ERROR (Status);
-
- // Find the core in the ArmCoreTable
- for (Index = 0; Index < ArmCoreCount; Index++) {
- if ((GET_MPIDR_AFF1 (ArmCoreInfoTable[Index].Mpidr) == ClusterId) &&
- (GET_MPIDR_AFF0 (ArmCoreInfoTable[Index].Mpidr) == CoreId))
- {
- break;
- }
- }
-
- // The ARM Core Info Table must define every core
- ASSERT (Index != ArmCoreCount);
-
- // Clear Secondary cores MailBox
- MmioWrite32 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue);
-
- do {
- ArmCallWFI ();
-
- // Read the Mailbox
- SecondaryEntryAddr = MmioRead32 (ArmCoreInfoTable[Index].MailboxGetAddress);
-
- // Acknowledge the interrupt and send End of Interrupt signal.
- AcknowledgeInterrupt = ArmGicAcknowledgeInterrupt (PcdGet64 (PcdGicInterruptInterfaceBase), &InterruptId);
- // Check if it is a valid interrupt ID
- if (InterruptId < ArmGicGetMaxNumInterrupts (PcdGet64 (PcdGicDistributorBase))) {
- // Got a valid SGI number hence signal End of Interrupt
- ArmGicEndOfInterrupt (PcdGet64 (PcdGicInterruptInterfaceBase), AcknowledgeInterrupt);
- }
- } while (SecondaryEntryAddr == 0);
-
- // Jump to secondary core entry point.
- SecondaryStart = (VOID (*)()) SecondaryEntryAddr;
- SecondaryStart ();
-
- // The secondaries shouldn't reach here
- ASSERT (FALSE);
-}
diff --git a/ArmPlatformPkg/PrePi/MainUniCore.c b/ArmPlatformPkg/PrePi/MainUniCore.c
index 6162d1241f84..7449facacd51 100644
--- a/ArmPlatformPkg/PrePi/MainUniCore.c
+++ b/ArmPlatformPkg/PrePi/MainUniCore.c
@@ -20,12 +20,3 @@ PrimaryMain (
// We must never return
ASSERT (FALSE);
}
-
-VOID
-SecondaryMain (
- IN UINTN MpId
- )
-{
- // We must never get into this function on UniCore system
- ASSERT (FALSE);
-}
diff --git a/ArmPlatformPkg/PrePi/PrePi.c b/ArmPlatformPkg/PrePi/PrePi.c
index 9b127b94a67c..60061b8b6963 100644
--- a/ArmPlatformPkg/PrePi/PrePi.c
+++ b/ArmPlatformPkg/PrePi/PrePi.c
@@ -177,7 +177,11 @@ CEntryPoint (
// Initialize the platform specific controllers
ArmPlatformInitialize (MpId);

- if (ArmPlatformIsPrimaryCore (MpId) && PerformanceMeasurementEnabled ()) {
+ if (!ArmPlatformIsPrimaryCore (MpId)) {
+ ASSERT (FALSE);
+ }
+
+ if (PerformanceMeasurementEnabled ()) {
// Initialize the Timer Library to setup the Timer HW controller
TimerConstructor ();
// We cannot call yet the PerformanceLib because the HOB List has not been initialized
@@ -195,29 +199,21 @@ CEntryPoint (

// Define the Global Variable region when we are not running in XIP
if (!IS_XIP ()) {
- if (ArmPlatformIsPrimaryCore (MpId)) {
- if (ArmIsMpCore ()) {
- // Signal the Global Variable Region is defined (event: ARM_CPU_EVENT_DEFAULT)
- ArmCallSEV ();
- }
- } else {
- // Wait the Primary core has defined the address of the Global Variable region (event: ARM_CPU_EVENT_DEFAULT)
- ArmCallWFE ();
+ if (ArmIsMpCore ()) {
+ // Signal the Global Variable Region is defined (event: ARM_CPU_EVENT_DEFAULT)
+ ArmCallSEV ();
}
}

- // If not primary Jump to Secondary Main
- if (ArmPlatformIsPrimaryCore (MpId)) {
- InvalidateDataCacheRange (
- (VOID *)UefiMemoryBase,
- FixedPcdGet32 (PcdSystemMemoryUefiRegionSize)
- );
+ InvalidateDataCacheRange (
+ (VOID *)UefiMemoryBase,
+ FixedPcdGet32 (PcdSystemMemoryUefiRegionSize)
+ );

- // Goto primary Main.
- PrimaryMain (UefiMemoryBase, StacksBase, StartTimeStamp);
- } else {
- SecondaryMain (MpId);
- }
+ PrePiMain (UefiMemoryBase, StacksBase, StartTimeStamp);
+
+ // We must never return
+ ASSERT (FALSE);

// DXE Core should always load and never return
ASSERT (FALSE);
--
2.30.2


Ard Biesheuvel
 

On Thu, 27 Oct 2022 at 19:31, Rebecca Cran <quic_rcran@...> wrote:

Modern platforms use TF-A, so there's no need for support of
secondary cores in EDK2 since TF-A will keep them in a holding
pen until the PSCI_CPU_ON SMC call is received.

Therefore, remove the code that handles secondary CPUs from
PrePeiCore and PrePi and add ASSERTs if a secondary core
reaches the functions.

Signed-off-by: Rebecca Cran <rebecca@...>
No objections to this patch, but this change will break the old SMP
32-bit ARM platforms in edk2-platforms so you will need to propose a
solution for those as well.


---
ArmPlatformPkg/PrePeiCore/MainMPCore.c | 92 --------------------
ArmPlatformPkg/PrePeiCore/MainUniCore.c | 9 --
ArmPlatformPkg/PrePeiCore/PrePeiCore.c | 37 ++++----
ArmPlatformPkg/PrePi/MainMPCore.c | 69 ---------------
ArmPlatformPkg/PrePi/MainUniCore.c | 9 --
ArmPlatformPkg/PrePi/PrePi.c | 36 ++++----
6 files changed, 34 insertions(+), 218 deletions(-)

diff --git a/ArmPlatformPkg/PrePeiCore/MainMPCore.c b/ArmPlatformPkg/PrePeiCore/MainMPCore.c
index b5d0d3a6442f..44850a4f3946 100644
--- a/ArmPlatformPkg/PrePeiCore/MainMPCore.c
+++ b/ArmPlatformPkg/PrePeiCore/MainMPCore.c
@@ -12,98 +12,6 @@

#include "PrePeiCore.h"

-/*
- * This is the main function for secondary cores. They loop around until a non Null value is written to
- * SYS_FLAGS register.The SYS_FLAGS register is platform specific.
- * Note:The secondary cores, while executing secondary_main, assumes that:
- * : SGI 0 is configured as Non-secure interrupt
- * : Priority Mask is configured to allow SGI 0
- * : Interrupt Distributor and CPU interfaces are enabled
- *
- */
-VOID
-EFIAPI
-SecondaryMain (
- IN UINTN MpId
- )
-{
- EFI_STATUS Status;
- UINTN PpiListSize;
- UINTN PpiListCount;
- EFI_PEI_PPI_DESCRIPTOR *PpiList;
- ARM_MP_CORE_INFO_PPI *ArmMpCoreInfoPpi;
- UINTN Index;
- UINTN ArmCoreCount;
- ARM_CORE_INFO *ArmCoreInfoTable;
- UINT32 ClusterId;
- UINT32 CoreId;
-
- VOID (*SecondaryStart)(
- VOID
- );
- UINTN SecondaryEntryAddr;
- UINTN AcknowledgeInterrupt;
- UINTN InterruptId;
-
- ClusterId = GET_CLUSTER_ID (MpId);
- CoreId = GET_CORE_ID (MpId);
-
- // Get the gArmMpCoreInfoPpiGuid
- PpiListSize = 0;
- ArmPlatformGetPlatformPpiList (&PpiListSize, &PpiList);
- PpiListCount = PpiListSize / sizeof (EFI_PEI_PPI_DESCRIPTOR);
- for (Index = 0; Index < PpiListCount; Index++, PpiList++) {
- if (CompareGuid (PpiList->Guid, &gArmMpCoreInfoPpiGuid) == TRUE) {
- break;
- }
- }
-
- // On MP Core Platform we must implement the ARM MP Core Info PPI
- ASSERT (Index != PpiListCount);
-
- ArmMpCoreInfoPpi = PpiList->Ppi;
- ArmCoreCount = 0;
- Status = ArmMpCoreInfoPpi->GetMpCoreInfo (&ArmCoreCount, &ArmCoreInfoTable);
- ASSERT_EFI_ERROR (Status);
-
- // Find the core in the ArmCoreTable
- for (Index = 0; Index < ArmCoreCount; Index++) {
- if ((GET_MPIDR_AFF1 (ArmCoreInfoTable[Index].Mpidr) == ClusterId) &&
- (GET_MPIDR_AFF0 (ArmCoreInfoTable[Index].Mpidr) == CoreId))
- {
- break;
- }
- }
-
- // The ARM Core Info Table must define every core
- ASSERT (Index != ArmCoreCount);
-
- // Clear Secondary cores MailBox
- MmioWrite32 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue);
-
- do {
- ArmCallWFI ();
-
- // Read the Mailbox
- SecondaryEntryAddr = MmioRead32 (ArmCoreInfoTable[Index].MailboxGetAddress);
-
- // Acknowledge the interrupt and send End of Interrupt signal.
- AcknowledgeInterrupt = ArmGicAcknowledgeInterrupt (PcdGet64 (PcdGicInterruptInterfaceBase), &InterruptId);
- // Check if it is a valid interrupt ID
- if (InterruptId < ArmGicGetMaxNumInterrupts (PcdGet64 (PcdGicDistributorBase))) {
- // Got a valid SGI number hence signal End of Interrupt
- ArmGicEndOfInterrupt (PcdGet64 (PcdGicInterruptInterfaceBase), AcknowledgeInterrupt);
- }
- } while (SecondaryEntryAddr == 0);
-
- // Jump to secondary core entry point.
- SecondaryStart = (VOID (*)()) SecondaryEntryAddr;
- SecondaryStart ();
-
- // The secondaries shouldn't reach here
- ASSERT (FALSE);
-}
-
VOID
EFIAPI
PrimaryMain (
diff --git a/ArmPlatformPkg/PrePeiCore/MainUniCore.c b/ArmPlatformPkg/PrePeiCore/MainUniCore.c
index 1c2580eb923b..3d3c6caaa32a 100644
--- a/ArmPlatformPkg/PrePeiCore/MainUniCore.c
+++ b/ArmPlatformPkg/PrePeiCore/MainUniCore.c
@@ -8,15 +8,6 @@

#include "PrePeiCore.h"

-VOID
-EFIAPI
-SecondaryMain (
- IN UINTN MpId
- )
-{
- ASSERT (FALSE);
-}
-
VOID
EFIAPI
PrimaryMain (
diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
index 42a7ccc9c6a0..64d1ef601ea3 100644
--- a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
+++ b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
@@ -117,27 +117,26 @@ CEntryPoint (

// Note: The MMU will be enabled by MemoryPeim. Only the primary core will have the MMU on.

- // If not primary Jump to Secondary Main
- if (ArmPlatformIsPrimaryCore (MpId)) {
- // Invoke "ProcessLibraryConstructorList" to have all library constructors
- // called.
- ProcessLibraryConstructorList ();
-
- PrintFirmwareVersion ();
-
- // Initialize the Debug Agent for Source Level Debugging
- InitializeDebugAgent (DEBUG_AGENT_INIT_POSTMEM_SEC, NULL, NULL);
- SaveAndSetDebugTimerInterrupt (TRUE);
-
- // Initialize the platform specific controllers
- ArmPlatformInitialize (MpId);
-
- // Goto primary Main.
- PrimaryMain (PeiCoreEntryPoint);
- } else {
- SecondaryMain (MpId);
+ if (!ArmPlatformIsPrimaryCore (MpId)) {
+ ASSERT (FALSE);
}

+ // Invoke "ProcessLibraryConstructorList" to have all library constructors
+ // called.
+ ProcessLibraryConstructorList ();
+
+ PrintFirmwareVersion ();
+
+ // Initialize the Debug Agent for Source Level Debugging
+ InitializeDebugAgent (DEBUG_AGENT_INIT_POSTMEM_SEC, NULL, NULL);
+ SaveAndSetDebugTimerInterrupt (TRUE);
+
+ // Initialize the platform specific controllers
+ ArmPlatformInitialize (MpId);
+
+ // Goto primary Main.
+ PrimaryMain (PeiCoreEntryPoint);
+
// PEI Core should always load and never return
ASSERT (FALSE);
}
diff --git a/ArmPlatformPkg/PrePi/MainMPCore.c b/ArmPlatformPkg/PrePi/MainMPCore.c
index 68a7c13298d0..ce7058a2846f 100644
--- a/ArmPlatformPkg/PrePi/MainMPCore.c
+++ b/ArmPlatformPkg/PrePi/MainMPCore.c
@@ -33,72 +33,3 @@ PrimaryMain (
// We must never return
ASSERT (FALSE);
}
-
-VOID
-SecondaryMain (
- IN UINTN MpId
- )
-{
- EFI_STATUS Status;
- ARM_MP_CORE_INFO_PPI *ArmMpCoreInfoPpi;
- UINTN Index;
- UINTN ArmCoreCount;
- ARM_CORE_INFO *ArmCoreInfoTable;
- UINT32 ClusterId;
- UINT32 CoreId;
-
- VOID (*SecondaryStart)(
- VOID
- );
- UINTN SecondaryEntryAddr;
- UINTN AcknowledgeInterrupt;
- UINTN InterruptId;
-
- ClusterId = GET_CLUSTER_ID (MpId);
- CoreId = GET_CORE_ID (MpId);
-
- // On MP Core Platform we must implement the ARM MP Core Info PPI (gArmMpCoreInfoPpiGuid)
- Status = GetPlatformPpi (&gArmMpCoreInfoPpiGuid, (VOID **)&ArmMpCoreInfoPpi);
- ASSERT_EFI_ERROR (Status);
-
- ArmCoreCount = 0;
- Status = ArmMpCoreInfoPpi->GetMpCoreInfo (&ArmCoreCount, &ArmCoreInfoTable);
- ASSERT_EFI_ERROR (Status);
-
- // Find the core in the ArmCoreTable
- for (Index = 0; Index < ArmCoreCount; Index++) {
- if ((GET_MPIDR_AFF1 (ArmCoreInfoTable[Index].Mpidr) == ClusterId) &&
- (GET_MPIDR_AFF0 (ArmCoreInfoTable[Index].Mpidr) == CoreId))
- {
- break;
- }
- }
-
- // The ARM Core Info Table must define every core
- ASSERT (Index != ArmCoreCount);
-
- // Clear Secondary cores MailBox
- MmioWrite32 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue);
-
- do {
- ArmCallWFI ();
-
- // Read the Mailbox
- SecondaryEntryAddr = MmioRead32 (ArmCoreInfoTable[Index].MailboxGetAddress);
-
- // Acknowledge the interrupt and send End of Interrupt signal.
- AcknowledgeInterrupt = ArmGicAcknowledgeInterrupt (PcdGet64 (PcdGicInterruptInterfaceBase), &InterruptId);
- // Check if it is a valid interrupt ID
- if (InterruptId < ArmGicGetMaxNumInterrupts (PcdGet64 (PcdGicDistributorBase))) {
- // Got a valid SGI number hence signal End of Interrupt
- ArmGicEndOfInterrupt (PcdGet64 (PcdGicInterruptInterfaceBase), AcknowledgeInterrupt);
- }
- } while (SecondaryEntryAddr == 0);
-
- // Jump to secondary core entry point.
- SecondaryStart = (VOID (*)()) SecondaryEntryAddr;
- SecondaryStart ();
-
- // The secondaries shouldn't reach here
- ASSERT (FALSE);
-}
diff --git a/ArmPlatformPkg/PrePi/MainUniCore.c b/ArmPlatformPkg/PrePi/MainUniCore.c
index 6162d1241f84..7449facacd51 100644
--- a/ArmPlatformPkg/PrePi/MainUniCore.c
+++ b/ArmPlatformPkg/PrePi/MainUniCore.c
@@ -20,12 +20,3 @@ PrimaryMain (
// We must never return
ASSERT (FALSE);
}
-
-VOID
-SecondaryMain (
- IN UINTN MpId
- )
-{
- // We must never get into this function on UniCore system
- ASSERT (FALSE);
-}
diff --git a/ArmPlatformPkg/PrePi/PrePi.c b/ArmPlatformPkg/PrePi/PrePi.c
index 9b127b94a67c..60061b8b6963 100644
--- a/ArmPlatformPkg/PrePi/PrePi.c
+++ b/ArmPlatformPkg/PrePi/PrePi.c
@@ -177,7 +177,11 @@ CEntryPoint (
// Initialize the platform specific controllers
ArmPlatformInitialize (MpId);

- if (ArmPlatformIsPrimaryCore (MpId) && PerformanceMeasurementEnabled ()) {
+ if (!ArmPlatformIsPrimaryCore (MpId)) {
+ ASSERT (FALSE);
+ }
+
+ if (PerformanceMeasurementEnabled ()) {
// Initialize the Timer Library to setup the Timer HW controller
TimerConstructor ();
// We cannot call yet the PerformanceLib because the HOB List has not been initialized
@@ -195,29 +199,21 @@ CEntryPoint (

// Define the Global Variable region when we are not running in XIP
if (!IS_XIP ()) {
- if (ArmPlatformIsPrimaryCore (MpId)) {
- if (ArmIsMpCore ()) {
- // Signal the Global Variable Region is defined (event: ARM_CPU_EVENT_DEFAULT)
- ArmCallSEV ();
- }
- } else {
- // Wait the Primary core has defined the address of the Global Variable region (event: ARM_CPU_EVENT_DEFAULT)
- ArmCallWFE ();
+ if (ArmIsMpCore ()) {
+ // Signal the Global Variable Region is defined (event: ARM_CPU_EVENT_DEFAULT)
+ ArmCallSEV ();
}
}

- // If not primary Jump to Secondary Main
- if (ArmPlatformIsPrimaryCore (MpId)) {
- InvalidateDataCacheRange (
- (VOID *)UefiMemoryBase,
- FixedPcdGet32 (PcdSystemMemoryUefiRegionSize)
- );
+ InvalidateDataCacheRange (
+ (VOID *)UefiMemoryBase,
+ FixedPcdGet32 (PcdSystemMemoryUefiRegionSize)
+ );

- // Goto primary Main.
- PrimaryMain (UefiMemoryBase, StacksBase, StartTimeStamp);
- } else {
- SecondaryMain (MpId);
- }
+ PrePiMain (UefiMemoryBase, StacksBase, StartTimeStamp);
+
+ // We must never return
+ ASSERT (FALSE);

// DXE Core should always load and never return
ASSERT (FALSE);
--
2.30.2






Leif Lindholm
 

On Thu, Oct 27, 2022 at 20:10:33 +0200, Ard Biesheuvel wrote:
On Thu, 27 Oct 2022 at 19:31, Rebecca Cran <quic_rcran@...> wrote:

Modern platforms use TF-A, so there's no need for support of
secondary cores in EDK2 since TF-A will keep them in a holding
pen until the PSCI_CPU_ON SMC call is received.

Therefore, remove the code that handles secondary CPUs from
PrePeiCore and PrePi and add ASSERTs if a secondary core
reaches the functions.

Signed-off-by: Rebecca Cran <rebecca@...>
No objections to this patch, but this change will break the old SMP
32-bit ARM platforms in edk2-platforms so you will need to propose a
solution for those as well.
I think TC2 is the last one of those standing. And I don't see much
value in keeping all of this around for such a niche (and old)
platform. If someone ports TF-A to it, we could always add it back in
later.

Single-core non-PSCI platforms (i.e. beagleboard) aren't affected.

Sami: would you be OK with deleting the TC2 support in edk2-platforms?

/
Leif

---
ArmPlatformPkg/PrePeiCore/MainMPCore.c | 92 --------------------
ArmPlatformPkg/PrePeiCore/MainUniCore.c | 9 --
ArmPlatformPkg/PrePeiCore/PrePeiCore.c | 37 ++++----
ArmPlatformPkg/PrePi/MainMPCore.c | 69 ---------------
ArmPlatformPkg/PrePi/MainUniCore.c | 9 --
ArmPlatformPkg/PrePi/PrePi.c | 36 ++++----
6 files changed, 34 insertions(+), 218 deletions(-)

diff --git a/ArmPlatformPkg/PrePeiCore/MainMPCore.c b/ArmPlatformPkg/PrePeiCore/MainMPCore.c
index b5d0d3a6442f..44850a4f3946 100644
--- a/ArmPlatformPkg/PrePeiCore/MainMPCore.c
+++ b/ArmPlatformPkg/PrePeiCore/MainMPCore.c
@@ -12,98 +12,6 @@

#include "PrePeiCore.h"

-/*
- * This is the main function for secondary cores. They loop around until a non Null value is written to
- * SYS_FLAGS register.The SYS_FLAGS register is platform specific.
- * Note:The secondary cores, while executing secondary_main, assumes that:
- * : SGI 0 is configured as Non-secure interrupt
- * : Priority Mask is configured to allow SGI 0
- * : Interrupt Distributor and CPU interfaces are enabled
- *
- */
-VOID
-EFIAPI
-SecondaryMain (
- IN UINTN MpId
- )
-{
- EFI_STATUS Status;
- UINTN PpiListSize;
- UINTN PpiListCount;
- EFI_PEI_PPI_DESCRIPTOR *PpiList;
- ARM_MP_CORE_INFO_PPI *ArmMpCoreInfoPpi;
- UINTN Index;
- UINTN ArmCoreCount;
- ARM_CORE_INFO *ArmCoreInfoTable;
- UINT32 ClusterId;
- UINT32 CoreId;
-
- VOID (*SecondaryStart)(
- VOID
- );
- UINTN SecondaryEntryAddr;
- UINTN AcknowledgeInterrupt;
- UINTN InterruptId;
-
- ClusterId = GET_CLUSTER_ID (MpId);
- CoreId = GET_CORE_ID (MpId);
-
- // Get the gArmMpCoreInfoPpiGuid
- PpiListSize = 0;
- ArmPlatformGetPlatformPpiList (&PpiListSize, &PpiList);
- PpiListCount = PpiListSize / sizeof (EFI_PEI_PPI_DESCRIPTOR);
- for (Index = 0; Index < PpiListCount; Index++, PpiList++) {
- if (CompareGuid (PpiList->Guid, &gArmMpCoreInfoPpiGuid) == TRUE) {
- break;
- }
- }
-
- // On MP Core Platform we must implement the ARM MP Core Info PPI
- ASSERT (Index != PpiListCount);
-
- ArmMpCoreInfoPpi = PpiList->Ppi;
- ArmCoreCount = 0;
- Status = ArmMpCoreInfoPpi->GetMpCoreInfo (&ArmCoreCount, &ArmCoreInfoTable);
- ASSERT_EFI_ERROR (Status);
-
- // Find the core in the ArmCoreTable
- for (Index = 0; Index < ArmCoreCount; Index++) {
- if ((GET_MPIDR_AFF1 (ArmCoreInfoTable[Index].Mpidr) == ClusterId) &&
- (GET_MPIDR_AFF0 (ArmCoreInfoTable[Index].Mpidr) == CoreId))
- {
- break;
- }
- }
-
- // The ARM Core Info Table must define every core
- ASSERT (Index != ArmCoreCount);
-
- // Clear Secondary cores MailBox
- MmioWrite32 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue);
-
- do {
- ArmCallWFI ();
-
- // Read the Mailbox
- SecondaryEntryAddr = MmioRead32 (ArmCoreInfoTable[Index].MailboxGetAddress);
-
- // Acknowledge the interrupt and send End of Interrupt signal.
- AcknowledgeInterrupt = ArmGicAcknowledgeInterrupt (PcdGet64 (PcdGicInterruptInterfaceBase), &InterruptId);
- // Check if it is a valid interrupt ID
- if (InterruptId < ArmGicGetMaxNumInterrupts (PcdGet64 (PcdGicDistributorBase))) {
- // Got a valid SGI number hence signal End of Interrupt
- ArmGicEndOfInterrupt (PcdGet64 (PcdGicInterruptInterfaceBase), AcknowledgeInterrupt);
- }
- } while (SecondaryEntryAddr == 0);
-
- // Jump to secondary core entry point.
- SecondaryStart = (VOID (*)()) SecondaryEntryAddr;
- SecondaryStart ();
-
- // The secondaries shouldn't reach here
- ASSERT (FALSE);
-}
-
VOID
EFIAPI
PrimaryMain (
diff --git a/ArmPlatformPkg/PrePeiCore/MainUniCore.c b/ArmPlatformPkg/PrePeiCore/MainUniCore.c
index 1c2580eb923b..3d3c6caaa32a 100644
--- a/ArmPlatformPkg/PrePeiCore/MainUniCore.c
+++ b/ArmPlatformPkg/PrePeiCore/MainUniCore.c
@@ -8,15 +8,6 @@

#include "PrePeiCore.h"

-VOID
-EFIAPI
-SecondaryMain (
- IN UINTN MpId
- )
-{
- ASSERT (FALSE);
-}
-
VOID
EFIAPI
PrimaryMain (
diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
index 42a7ccc9c6a0..64d1ef601ea3 100644
--- a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
+++ b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
@@ -117,27 +117,26 @@ CEntryPoint (

// Note: The MMU will be enabled by MemoryPeim. Only the primary core will have the MMU on.

- // If not primary Jump to Secondary Main
- if (ArmPlatformIsPrimaryCore (MpId)) {
- // Invoke "ProcessLibraryConstructorList" to have all library constructors
- // called.
- ProcessLibraryConstructorList ();
-
- PrintFirmwareVersion ();
-
- // Initialize the Debug Agent for Source Level Debugging
- InitializeDebugAgent (DEBUG_AGENT_INIT_POSTMEM_SEC, NULL, NULL);
- SaveAndSetDebugTimerInterrupt (TRUE);
-
- // Initialize the platform specific controllers
- ArmPlatformInitialize (MpId);
-
- // Goto primary Main.
- PrimaryMain (PeiCoreEntryPoint);
- } else {
- SecondaryMain (MpId);
+ if (!ArmPlatformIsPrimaryCore (MpId)) {
+ ASSERT (FALSE);
}

+ // Invoke "ProcessLibraryConstructorList" to have all library constructors
+ // called.
+ ProcessLibraryConstructorList ();
+
+ PrintFirmwareVersion ();
+
+ // Initialize the Debug Agent for Source Level Debugging
+ InitializeDebugAgent (DEBUG_AGENT_INIT_POSTMEM_SEC, NULL, NULL);
+ SaveAndSetDebugTimerInterrupt (TRUE);
+
+ // Initialize the platform specific controllers
+ ArmPlatformInitialize (MpId);
+
+ // Goto primary Main.
+ PrimaryMain (PeiCoreEntryPoint);
+
// PEI Core should always load and never return
ASSERT (FALSE);
}
diff --git a/ArmPlatformPkg/PrePi/MainMPCore.c b/ArmPlatformPkg/PrePi/MainMPCore.c
index 68a7c13298d0..ce7058a2846f 100644
--- a/ArmPlatformPkg/PrePi/MainMPCore.c
+++ b/ArmPlatformPkg/PrePi/MainMPCore.c
@@ -33,72 +33,3 @@ PrimaryMain (
// We must never return
ASSERT (FALSE);
}
-
-VOID
-SecondaryMain (
- IN UINTN MpId
- )
-{
- EFI_STATUS Status;
- ARM_MP_CORE_INFO_PPI *ArmMpCoreInfoPpi;
- UINTN Index;
- UINTN ArmCoreCount;
- ARM_CORE_INFO *ArmCoreInfoTable;
- UINT32 ClusterId;
- UINT32 CoreId;
-
- VOID (*SecondaryStart)(
- VOID
- );
- UINTN SecondaryEntryAddr;
- UINTN AcknowledgeInterrupt;
- UINTN InterruptId;
-
- ClusterId = GET_CLUSTER_ID (MpId);
- CoreId = GET_CORE_ID (MpId);
-
- // On MP Core Platform we must implement the ARM MP Core Info PPI (gArmMpCoreInfoPpiGuid)
- Status = GetPlatformPpi (&gArmMpCoreInfoPpiGuid, (VOID **)&ArmMpCoreInfoPpi);
- ASSERT_EFI_ERROR (Status);
-
- ArmCoreCount = 0;
- Status = ArmMpCoreInfoPpi->GetMpCoreInfo (&ArmCoreCount, &ArmCoreInfoTable);
- ASSERT_EFI_ERROR (Status);
-
- // Find the core in the ArmCoreTable
- for (Index = 0; Index < ArmCoreCount; Index++) {
- if ((GET_MPIDR_AFF1 (ArmCoreInfoTable[Index].Mpidr) == ClusterId) &&
- (GET_MPIDR_AFF0 (ArmCoreInfoTable[Index].Mpidr) == CoreId))
- {
- break;
- }
- }
-
- // The ARM Core Info Table must define every core
- ASSERT (Index != ArmCoreCount);
-
- // Clear Secondary cores MailBox
- MmioWrite32 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue);
-
- do {
- ArmCallWFI ();
-
- // Read the Mailbox
- SecondaryEntryAddr = MmioRead32 (ArmCoreInfoTable[Index].MailboxGetAddress);
-
- // Acknowledge the interrupt and send End of Interrupt signal.
- AcknowledgeInterrupt = ArmGicAcknowledgeInterrupt (PcdGet64 (PcdGicInterruptInterfaceBase), &InterruptId);
- // Check if it is a valid interrupt ID
- if (InterruptId < ArmGicGetMaxNumInterrupts (PcdGet64 (PcdGicDistributorBase))) {
- // Got a valid SGI number hence signal End of Interrupt
- ArmGicEndOfInterrupt (PcdGet64 (PcdGicInterruptInterfaceBase), AcknowledgeInterrupt);
- }
- } while (SecondaryEntryAddr == 0);
-
- // Jump to secondary core entry point.
- SecondaryStart = (VOID (*)()) SecondaryEntryAddr;
- SecondaryStart ();
-
- // The secondaries shouldn't reach here
- ASSERT (FALSE);
-}
diff --git a/ArmPlatformPkg/PrePi/MainUniCore.c b/ArmPlatformPkg/PrePi/MainUniCore.c
index 6162d1241f84..7449facacd51 100644
--- a/ArmPlatformPkg/PrePi/MainUniCore.c
+++ b/ArmPlatformPkg/PrePi/MainUniCore.c
@@ -20,12 +20,3 @@ PrimaryMain (
// We must never return
ASSERT (FALSE);
}
-
-VOID
-SecondaryMain (
- IN UINTN MpId
- )
-{
- // We must never get into this function on UniCore system
- ASSERT (FALSE);
-}
diff --git a/ArmPlatformPkg/PrePi/PrePi.c b/ArmPlatformPkg/PrePi/PrePi.c
index 9b127b94a67c..60061b8b6963 100644
--- a/ArmPlatformPkg/PrePi/PrePi.c
+++ b/ArmPlatformPkg/PrePi/PrePi.c
@@ -177,7 +177,11 @@ CEntryPoint (
// Initialize the platform specific controllers
ArmPlatformInitialize (MpId);

- if (ArmPlatformIsPrimaryCore (MpId) && PerformanceMeasurementEnabled ()) {
+ if (!ArmPlatformIsPrimaryCore (MpId)) {
+ ASSERT (FALSE);
+ }
+
+ if (PerformanceMeasurementEnabled ()) {
// Initialize the Timer Library to setup the Timer HW controller
TimerConstructor ();
// We cannot call yet the PerformanceLib because the HOB List has not been initialized
@@ -195,29 +199,21 @@ CEntryPoint (

// Define the Global Variable region when we are not running in XIP
if (!IS_XIP ()) {
- if (ArmPlatformIsPrimaryCore (MpId)) {
- if (ArmIsMpCore ()) {
- // Signal the Global Variable Region is defined (event: ARM_CPU_EVENT_DEFAULT)
- ArmCallSEV ();
- }
- } else {
- // Wait the Primary core has defined the address of the Global Variable region (event: ARM_CPU_EVENT_DEFAULT)
- ArmCallWFE ();
+ if (ArmIsMpCore ()) {
+ // Signal the Global Variable Region is defined (event: ARM_CPU_EVENT_DEFAULT)
+ ArmCallSEV ();
}
}

- // If not primary Jump to Secondary Main
- if (ArmPlatformIsPrimaryCore (MpId)) {
- InvalidateDataCacheRange (
- (VOID *)UefiMemoryBase,
- FixedPcdGet32 (PcdSystemMemoryUefiRegionSize)
- );
+ InvalidateDataCacheRange (
+ (VOID *)UefiMemoryBase,
+ FixedPcdGet32 (PcdSystemMemoryUefiRegionSize)
+ );

- // Goto primary Main.
- PrimaryMain (UefiMemoryBase, StacksBase, StartTimeStamp);
- } else {
- SecondaryMain (MpId);
- }
+ PrePiMain (UefiMemoryBase, StacksBase, StartTimeStamp);
+
+ // We must never return
+ ASSERT (FALSE);

// DXE Core should always load and never return
ASSERT (FALSE);
--
2.30.2






Rebecca Cran <quic_rcran@...>
 

Hi Sami,

I was wondering if you could answer Leif's question?

Thanks.
Rebecca Cran

On 10/28/22 03:17, Leif Lindholm wrote:
On Thu, Oct 27, 2022 at 20:10:33 +0200, Ard Biesheuvel wrote:
On Thu, 27 Oct 2022 at 19:31, Rebecca Cran <quic_rcran@...> wrote:

Modern platforms use TF-A, so there's no need for support of
secondary cores in EDK2 since TF-A will keep them in a holding
pen until the PSCI_CPU_ON SMC call is received.

Therefore, remove the code that handles secondary CPUs from
PrePeiCore and PrePi and add ASSERTs if a secondary core
reaches the functions.

Signed-off-by: Rebecca Cran <rebecca@...>
No objections to this patch, but this change will break the old SMP
32-bit ARM platforms in edk2-platforms so you will need to propose a
solution for those as well.
I think TC2 is the last one of those standing. And I don't see much
value in keeping all of this around for such a niche (and old)
platform. If someone ports TF-A to it, we could always add it back in
later.
Single-core non-PSCI platforms (i.e. beagleboard) aren't affected.
Sami: would you be OK with deleting the TC2 support in edk2-platforms?
/
Leif

---
ArmPlatformPkg/PrePeiCore/MainMPCore.c | 92 --------------------
ArmPlatformPkg/PrePeiCore/MainUniCore.c | 9 --
ArmPlatformPkg/PrePeiCore/PrePeiCore.c | 37 ++++----
ArmPlatformPkg/PrePi/MainMPCore.c | 69 ---------------
ArmPlatformPkg/PrePi/MainUniCore.c | 9 --
ArmPlatformPkg/PrePi/PrePi.c | 36 ++++----
6 files changed, 34 insertions(+), 218 deletions(-)

diff --git a/ArmPlatformPkg/PrePeiCore/MainMPCore.c b/ArmPlatformPkg/PrePeiCore/MainMPCore.c
index b5d0d3a6442f..44850a4f3946 100644
--- a/ArmPlatformPkg/PrePeiCore/MainMPCore.c
+++ b/ArmPlatformPkg/PrePeiCore/MainMPCore.c
@@ -12,98 +12,6 @@

#include "PrePeiCore.h"

-/*
- * This is the main function for secondary cores. They loop around until a non Null value is written to
- * SYS_FLAGS register.The SYS_FLAGS register is platform specific.
- * Note:The secondary cores, while executing secondary_main, assumes that:
- * : SGI 0 is configured as Non-secure interrupt
- * : Priority Mask is configured to allow SGI 0
- * : Interrupt Distributor and CPU interfaces are enabled
- *
- */
-VOID
-EFIAPI
-SecondaryMain (
- IN UINTN MpId
- )
-{
- EFI_STATUS Status;
- UINTN PpiListSize;
- UINTN PpiListCount;
- EFI_PEI_PPI_DESCRIPTOR *PpiList;
- ARM_MP_CORE_INFO_PPI *ArmMpCoreInfoPpi;
- UINTN Index;
- UINTN ArmCoreCount;
- ARM_CORE_INFO *ArmCoreInfoTable;
- UINT32 ClusterId;
- UINT32 CoreId;
-
- VOID (*SecondaryStart)(
- VOID
- );
- UINTN SecondaryEntryAddr;
- UINTN AcknowledgeInterrupt;
- UINTN InterruptId;
-
- ClusterId = GET_CLUSTER_ID (MpId);
- CoreId = GET_CORE_ID (MpId);
-
- // Get the gArmMpCoreInfoPpiGuid
- PpiListSize = 0;
- ArmPlatformGetPlatformPpiList (&PpiListSize, &PpiList);
- PpiListCount = PpiListSize / sizeof (EFI_PEI_PPI_DESCRIPTOR);
- for (Index = 0; Index < PpiListCount; Index++, PpiList++) {
- if (CompareGuid (PpiList->Guid, &gArmMpCoreInfoPpiGuid) == TRUE) {
- break;
- }
- }
-
- // On MP Core Platform we must implement the ARM MP Core Info PPI
- ASSERT (Index != PpiListCount);
-
- ArmMpCoreInfoPpi = PpiList->Ppi;
- ArmCoreCount = 0;
- Status = ArmMpCoreInfoPpi->GetMpCoreInfo (&ArmCoreCount, &ArmCoreInfoTable);
- ASSERT_EFI_ERROR (Status);
-
- // Find the core in the ArmCoreTable
- for (Index = 0; Index < ArmCoreCount; Index++) {
- if ((GET_MPIDR_AFF1 (ArmCoreInfoTable[Index].Mpidr) == ClusterId) &&
- (GET_MPIDR_AFF0 (ArmCoreInfoTable[Index].Mpidr) == CoreId))
- {
- break;
- }
- }
-
- // The ARM Core Info Table must define every core
- ASSERT (Index != ArmCoreCount);
-
- // Clear Secondary cores MailBox
- MmioWrite32 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue);
-
- do {
- ArmCallWFI ();
-
- // Read the Mailbox
- SecondaryEntryAddr = MmioRead32 (ArmCoreInfoTable[Index].MailboxGetAddress);
-
- // Acknowledge the interrupt and send End of Interrupt signal.
- AcknowledgeInterrupt = ArmGicAcknowledgeInterrupt (PcdGet64 (PcdGicInterruptInterfaceBase), &InterruptId);
- // Check if it is a valid interrupt ID
- if (InterruptId < ArmGicGetMaxNumInterrupts (PcdGet64 (PcdGicDistributorBase))) {
- // Got a valid SGI number hence signal End of Interrupt
- ArmGicEndOfInterrupt (PcdGet64 (PcdGicInterruptInterfaceBase), AcknowledgeInterrupt);
- }
- } while (SecondaryEntryAddr == 0);
-
- // Jump to secondary core entry point.
- SecondaryStart = (VOID (*)()) SecondaryEntryAddr;
- SecondaryStart ();
-
- // The secondaries shouldn't reach here
- ASSERT (FALSE);
-}
-
VOID
EFIAPI
PrimaryMain (
diff --git a/ArmPlatformPkg/PrePeiCore/MainUniCore.c b/ArmPlatformPkg/PrePeiCore/MainUniCore.c
index 1c2580eb923b..3d3c6caaa32a 100644
--- a/ArmPlatformPkg/PrePeiCore/MainUniCore.c
+++ b/ArmPlatformPkg/PrePeiCore/MainUniCore.c
@@ -8,15 +8,6 @@

#include "PrePeiCore.h"

-VOID
-EFIAPI
-SecondaryMain (
- IN UINTN MpId
- )
-{
- ASSERT (FALSE);
-}
-
VOID
EFIAPI
PrimaryMain (
diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
index 42a7ccc9c6a0..64d1ef601ea3 100644
--- a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
+++ b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
@@ -117,27 +117,26 @@ CEntryPoint (

// Note: The MMU will be enabled by MemoryPeim. Only the primary core will have the MMU on.

- // If not primary Jump to Secondary Main
- if (ArmPlatformIsPrimaryCore (MpId)) {
- // Invoke "ProcessLibraryConstructorList" to have all library constructors
- // called.
- ProcessLibraryConstructorList ();
-
- PrintFirmwareVersion ();
-
- // Initialize the Debug Agent for Source Level Debugging
- InitializeDebugAgent (DEBUG_AGENT_INIT_POSTMEM_SEC, NULL, NULL);
- SaveAndSetDebugTimerInterrupt (TRUE);
-
- // Initialize the platform specific controllers
- ArmPlatformInitialize (MpId);
-
- // Goto primary Main.
- PrimaryMain (PeiCoreEntryPoint);
- } else {
- SecondaryMain (MpId);
+ if (!ArmPlatformIsPrimaryCore (MpId)) {
+ ASSERT (FALSE);
}

+ // Invoke "ProcessLibraryConstructorList" to have all library constructors
+ // called.
+ ProcessLibraryConstructorList ();
+
+ PrintFirmwareVersion ();
+
+ // Initialize the Debug Agent for Source Level Debugging
+ InitializeDebugAgent (DEBUG_AGENT_INIT_POSTMEM_SEC, NULL, NULL);
+ SaveAndSetDebugTimerInterrupt (TRUE);
+
+ // Initialize the platform specific controllers
+ ArmPlatformInitialize (MpId);
+
+ // Goto primary Main.
+ PrimaryMain (PeiCoreEntryPoint);
+
// PEI Core should always load and never return
ASSERT (FALSE);
}
diff --git a/ArmPlatformPkg/PrePi/MainMPCore.c b/ArmPlatformPkg/PrePi/MainMPCore.c
index 68a7c13298d0..ce7058a2846f 100644
--- a/ArmPlatformPkg/PrePi/MainMPCore.c
+++ b/ArmPlatformPkg/PrePi/MainMPCore.c
@@ -33,72 +33,3 @@ PrimaryMain (
// We must never return
ASSERT (FALSE);
}
-
-VOID
-SecondaryMain (
- IN UINTN MpId
- )
-{
- EFI_STATUS Status;
- ARM_MP_CORE_INFO_PPI *ArmMpCoreInfoPpi;
- UINTN Index;
- UINTN ArmCoreCount;
- ARM_CORE_INFO *ArmCoreInfoTable;
- UINT32 ClusterId;
- UINT32 CoreId;
-
- VOID (*SecondaryStart)(
- VOID
- );
- UINTN SecondaryEntryAddr;
- UINTN AcknowledgeInterrupt;
- UINTN InterruptId;
-
- ClusterId = GET_CLUSTER_ID (MpId);
- CoreId = GET_CORE_ID (MpId);
-
- // On MP Core Platform we must implement the ARM MP Core Info PPI (gArmMpCoreInfoPpiGuid)
- Status = GetPlatformPpi (&gArmMpCoreInfoPpiGuid, (VOID **)&ArmMpCoreInfoPpi);
- ASSERT_EFI_ERROR (Status);
-
- ArmCoreCount = 0;
- Status = ArmMpCoreInfoPpi->GetMpCoreInfo (&ArmCoreCount, &ArmCoreInfoTable);
- ASSERT_EFI_ERROR (Status);
-
- // Find the core in the ArmCoreTable
- for (Index = 0; Index < ArmCoreCount; Index++) {
- if ((GET_MPIDR_AFF1 (ArmCoreInfoTable[Index].Mpidr) == ClusterId) &&
- (GET_MPIDR_AFF0 (ArmCoreInfoTable[Index].Mpidr) == CoreId))
- {
- break;
- }
- }
-
- // The ARM Core Info Table must define every core
- ASSERT (Index != ArmCoreCount);
-
- // Clear Secondary cores MailBox
- MmioWrite32 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue);
-
- do {
- ArmCallWFI ();
-
- // Read the Mailbox
- SecondaryEntryAddr = MmioRead32 (ArmCoreInfoTable[Index].MailboxGetAddress);
-
- // Acknowledge the interrupt and send End of Interrupt signal.
- AcknowledgeInterrupt = ArmGicAcknowledgeInterrupt (PcdGet64 (PcdGicInterruptInterfaceBase), &InterruptId);
- // Check if it is a valid interrupt ID
- if (InterruptId < ArmGicGetMaxNumInterrupts (PcdGet64 (PcdGicDistributorBase))) {
- // Got a valid SGI number hence signal End of Interrupt
- ArmGicEndOfInterrupt (PcdGet64 (PcdGicInterruptInterfaceBase), AcknowledgeInterrupt);
- }
- } while (SecondaryEntryAddr == 0);
-
- // Jump to secondary core entry point.
- SecondaryStart = (VOID (*)()) SecondaryEntryAddr;
- SecondaryStart ();
-
- // The secondaries shouldn't reach here
- ASSERT (FALSE);
-}
diff --git a/ArmPlatformPkg/PrePi/MainUniCore.c b/ArmPlatformPkg/PrePi/MainUniCore.c
index 6162d1241f84..7449facacd51 100644
--- a/ArmPlatformPkg/PrePi/MainUniCore.c
+++ b/ArmPlatformPkg/PrePi/MainUniCore.c
@@ -20,12 +20,3 @@ PrimaryMain (
// We must never return
ASSERT (FALSE);
}
-
-VOID
-SecondaryMain (
- IN UINTN MpId
- )
-{
- // We must never get into this function on UniCore system
- ASSERT (FALSE);
-}
diff --git a/ArmPlatformPkg/PrePi/PrePi.c b/ArmPlatformPkg/PrePi/PrePi.c
index 9b127b94a67c..60061b8b6963 100644
--- a/ArmPlatformPkg/PrePi/PrePi.c
+++ b/ArmPlatformPkg/PrePi/PrePi.c
@@ -177,7 +177,11 @@ CEntryPoint (
// Initialize the platform specific controllers
ArmPlatformInitialize (MpId);

- if (ArmPlatformIsPrimaryCore (MpId) && PerformanceMeasurementEnabled ()) {
+ if (!ArmPlatformIsPrimaryCore (MpId)) {
+ ASSERT (FALSE);
+ }
+
+ if (PerformanceMeasurementEnabled ()) {
// Initialize the Timer Library to setup the Timer HW controller
TimerConstructor ();
// We cannot call yet the PerformanceLib because the HOB List has not been initialized
@@ -195,29 +199,21 @@ CEntryPoint (

// Define the Global Variable region when we are not running in XIP
if (!IS_XIP ()) {
- if (ArmPlatformIsPrimaryCore (MpId)) {
- if (ArmIsMpCore ()) {
- // Signal the Global Variable Region is defined (event: ARM_CPU_EVENT_DEFAULT)
- ArmCallSEV ();
- }
- } else {
- // Wait the Primary core has defined the address of the Global Variable region (event: ARM_CPU_EVENT_DEFAULT)
- ArmCallWFE ();
+ if (ArmIsMpCore ()) {
+ // Signal the Global Variable Region is defined (event: ARM_CPU_EVENT_DEFAULT)
+ ArmCallSEV ();
}
}

- // If not primary Jump to Secondary Main
- if (ArmPlatformIsPrimaryCore (MpId)) {
- InvalidateDataCacheRange (
- (VOID *)UefiMemoryBase,
- FixedPcdGet32 (PcdSystemMemoryUefiRegionSize)
- );
+ InvalidateDataCacheRange (
+ (VOID *)UefiMemoryBase,
+ FixedPcdGet32 (PcdSystemMemoryUefiRegionSize)
+ );

- // Goto primary Main.
- PrimaryMain (UefiMemoryBase, StacksBase, StartTimeStamp);
- } else {
- SecondaryMain (MpId);
- }
+ PrePiMain (UefiMemoryBase, StacksBase, StartTimeStamp);
+
+ // We must never return
+ ASSERT (FALSE);

// DXE Core should always load and never return
ASSERT (FALSE);
--
2.30.2





Sami Mujawar
 

Hi Leif, Rebecca,

Apologies for the delay in getting back.

We can drop the TC2 support in edk2-platforms.

Regards,

Sami Mujawar

On 18/11/2022 02:09 pm, Rebecca Cran wrote:
Hi Sami,

I was wondering if you could answer Leif's question?

Thanks.
Rebecca Cran

On 10/28/22 03:17, Leif Lindholm wrote:
On Thu, Oct 27, 2022 at 20:10:33 +0200, Ard Biesheuvel wrote:
On Thu, 27 Oct 2022 at 19:31, Rebecca Cran <quic_rcran@...> wrote:

Modern platforms use TF-A, so there's no need for support of
secondary cores in EDK2 since TF-A will keep them in a holding
pen until the PSCI_CPU_ON SMC call is received.

Therefore, remove the code that handles secondary CPUs from
PrePeiCore and PrePi and add ASSERTs if a secondary core
reaches the functions.

Signed-off-by: Rebecca Cran <rebecca@...>
No objections to this patch, but this change will break the old SMP
32-bit ARM platforms in edk2-platforms so you will need to propose a
solution for those as well.
I think TC2 is the last one of those standing. And I don't see much
value in keeping all of this around for such a niche (and old)
platform. If someone ports TF-A to it, we could always add it back in
later.

Single-core non-PSCI platforms (i.e. beagleboard) aren't affected.

Sami: would you be OK with deleting the TC2 support in edk2-platforms?

/
     Leif

---
  ArmPlatformPkg/PrePeiCore/MainMPCore.c  | 92 --------------------
  ArmPlatformPkg/PrePeiCore/MainUniCore.c |  9 --
  ArmPlatformPkg/PrePeiCore/PrePeiCore.c  | 37 ++++----
  ArmPlatformPkg/PrePi/MainMPCore.c       | 69 ---------------
  ArmPlatformPkg/PrePi/MainUniCore.c      |  9 --
  ArmPlatformPkg/PrePi/PrePi.c            | 36 ++++----
  6 files changed, 34 insertions(+), 218 deletions(-)

diff --git a/ArmPlatformPkg/PrePeiCore/MainMPCore.c b/ArmPlatformPkg/PrePeiCore/MainMPCore.c
index b5d0d3a6442f..44850a4f3946 100644
--- a/ArmPlatformPkg/PrePeiCore/MainMPCore.c
+++ b/ArmPlatformPkg/PrePeiCore/MainMPCore.c
@@ -12,98 +12,6 @@

  #include "PrePeiCore.h"

-/*
- * This is the main function for secondary cores. They loop around until a non Null value is written to
- * SYS_FLAGS register.The SYS_FLAGS register is platform specific.
- * Note:The secondary cores, while executing secondary_main, assumes that:
- *      : SGI 0 is configured as Non-secure interrupt
- *      : Priority Mask is configured to allow SGI 0
- *      : Interrupt Distributor and CPU interfaces are enabled
- *
- */
-VOID
-EFIAPI
-SecondaryMain (
-  IN UINTN  MpId
-  )
-{
-  EFI_STATUS              Status;
-  UINTN                   PpiListSize;
-  UINTN                   PpiListCount;
-  EFI_PEI_PPI_DESCRIPTOR  *PpiList;
-  ARM_MP_CORE_INFO_PPI    *ArmMpCoreInfoPpi;
-  UINTN                   Index;
-  UINTN                   ArmCoreCount;
-  ARM_CORE_INFO           *ArmCoreInfoTable;
-  UINT32                  ClusterId;
-  UINT32                  CoreId;
-
-  VOID  (*SecondaryStart)(
-    VOID
-    );
-  UINTN  SecondaryEntryAddr;
-  UINTN  AcknowledgeInterrupt;
-  UINTN  InterruptId;
-
-  ClusterId = GET_CLUSTER_ID (MpId);
-  CoreId    = GET_CORE_ID (MpId);
-
-  // Get the gArmMpCoreInfoPpiGuid
-  PpiListSize = 0;
-  ArmPlatformGetPlatformPpiList (&PpiListSize, &PpiList);
-  PpiListCount = PpiListSize / sizeof (EFI_PEI_PPI_DESCRIPTOR);
-  for (Index = 0; Index < PpiListCount; Index++, PpiList++) {
-    if (CompareGuid (PpiList->Guid, &gArmMpCoreInfoPpiGuid) == TRUE) {
-      break;
-    }
-  }
-
-  // On MP Core Platform we must implement the ARM MP Core Info PPI
-  ASSERT (Index != PpiListCount);
-
-  ArmMpCoreInfoPpi = PpiList->Ppi;
-  ArmCoreCount     = 0;
-  Status           = ArmMpCoreInfoPpi->GetMpCoreInfo (&ArmCoreCount, &ArmCoreInfoTable);
-  ASSERT_EFI_ERROR (Status);
-
-  // Find the core in the ArmCoreTable
-  for (Index = 0; Index < ArmCoreCount; Index++) {
-    if ((GET_MPIDR_AFF1 (ArmCoreInfoTable[Index].Mpidr) == ClusterId) &&
-        (GET_MPIDR_AFF0 (ArmCoreInfoTable[Index].Mpidr) == CoreId))
-    {
-      break;
-    }
-  }
-
-  // The ARM Core Info Table must define every core
-  ASSERT (Index != ArmCoreCount);
-
-  // Clear Secondary cores MailBox
-  MmioWrite32 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue);
-
-  do {
-    ArmCallWFI ();
-
-    // Read the Mailbox
-    SecondaryEntryAddr = MmioRead32 (ArmCoreInfoTable[Index].MailboxGetAddress);
-
-    // Acknowledge the interrupt and send End of Interrupt signal.
-    AcknowledgeInterrupt = ArmGicAcknowledgeInterrupt (PcdGet64 (PcdGicInterruptInterfaceBase), &InterruptId);
-    // Check if it is a valid interrupt ID
-    if (InterruptId < ArmGicGetMaxNumInterrupts (PcdGet64 (PcdGicDistributorBase))) {
-      // Got a valid SGI number hence signal End of Interrupt
-      ArmGicEndOfInterrupt (PcdGet64 (PcdGicInterruptInterfaceBase), AcknowledgeInterrupt);
-    }
-  } while (SecondaryEntryAddr == 0);
-
-  // Jump to secondary core entry point.
-  SecondaryStart = (VOID (*)()) SecondaryEntryAddr;
-  SecondaryStart ();
-
-  // The secondaries shouldn't reach here
-  ASSERT (FALSE);
-}
-
  VOID
  EFIAPI
  PrimaryMain (
diff --git a/ArmPlatformPkg/PrePeiCore/MainUniCore.c b/ArmPlatformPkg/PrePeiCore/MainUniCore.c
index 1c2580eb923b..3d3c6caaa32a 100644
--- a/ArmPlatformPkg/PrePeiCore/MainUniCore.c
+++ b/ArmPlatformPkg/PrePeiCore/MainUniCore.c
@@ -8,15 +8,6 @@

  #include "PrePeiCore.h"

-VOID
-EFIAPI
-SecondaryMain (
-  IN UINTN  MpId
-  )
-{
-  ASSERT (FALSE);
-}
-
  VOID
  EFIAPI
  PrimaryMain (
diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
index 42a7ccc9c6a0..64d1ef601ea3 100644
--- a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
+++ b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
@@ -117,27 +117,26 @@ CEntryPoint (

    // Note: The MMU will be enabled by MemoryPeim. Only the primary core will have the MMU on.

-  // If not primary Jump to Secondary Main
-  if (ArmPlatformIsPrimaryCore (MpId)) {
-    // Invoke "ProcessLibraryConstructorList" to have all library constructors
-    // called.
-    ProcessLibraryConstructorList ();
-
-    PrintFirmwareVersion ();
-
-    // Initialize the Debug Agent for Source Level Debugging
-    InitializeDebugAgent (DEBUG_AGENT_INIT_POSTMEM_SEC, NULL, NULL);
-    SaveAndSetDebugTimerInterrupt (TRUE);
-
-    // Initialize the platform specific controllers
-    ArmPlatformInitialize (MpId);
-
-    // Goto primary Main.
-    PrimaryMain (PeiCoreEntryPoint);
-  } else {
-    SecondaryMain (MpId);
+  if (!ArmPlatformIsPrimaryCore (MpId)) {
+    ASSERT (FALSE);
    }

+  // Invoke "ProcessLibraryConstructorList" to have all library constructors
+  // called.
+  ProcessLibraryConstructorList ();
+
+  PrintFirmwareVersion ();
+
+  // Initialize the Debug Agent for Source Level Debugging
+  InitializeDebugAgent (DEBUG_AGENT_INIT_POSTMEM_SEC, NULL, NULL);
+  SaveAndSetDebugTimerInterrupt (TRUE);
+
+  // Initialize the platform specific controllers
+  ArmPlatformInitialize (MpId);
+
+  // Goto primary Main.
+  PrimaryMain (PeiCoreEntryPoint);
+
    // PEI Core should always load and never return
    ASSERT (FALSE);
  }
diff --git a/ArmPlatformPkg/PrePi/MainMPCore.c b/ArmPlatformPkg/PrePi/MainMPCore.c
index 68a7c13298d0..ce7058a2846f 100644
--- a/ArmPlatformPkg/PrePi/MainMPCore.c
+++ b/ArmPlatformPkg/PrePi/MainMPCore.c
@@ -33,72 +33,3 @@ PrimaryMain (
    // We must never return
    ASSERT (FALSE);
  }
-
-VOID
-SecondaryMain (
-  IN  UINTN  MpId
-  )
-{
-  EFI_STATUS            Status;
-  ARM_MP_CORE_INFO_PPI  *ArmMpCoreInfoPpi;
-  UINTN                 Index;
-  UINTN                 ArmCoreCount;
-  ARM_CORE_INFO         *ArmCoreInfoTable;
-  UINT32                ClusterId;
-  UINT32                CoreId;
-
-  VOID  (*SecondaryStart)(
-    VOID
-    );
-  UINTN  SecondaryEntryAddr;
-  UINTN  AcknowledgeInterrupt;
-  UINTN  InterruptId;
-
-  ClusterId = GET_CLUSTER_ID (MpId);
-  CoreId    = GET_CORE_ID (MpId);
-
-  // On MP Core Platform we must implement the ARM MP Core Info PPI (gArmMpCoreInfoPpiGuid)
-  Status = GetPlatformPpi (&gArmMpCoreInfoPpiGuid, (VOID **)&ArmMpCoreInfoPpi);
-  ASSERT_EFI_ERROR (Status);
-
-  ArmCoreCount = 0;
-  Status       = ArmMpCoreInfoPpi->GetMpCoreInfo (&ArmCoreCount, &ArmCoreInfoTable);
-  ASSERT_EFI_ERROR (Status);
-
-  // Find the core in the ArmCoreTable
-  for (Index = 0; Index < ArmCoreCount; Index++) {
-    if ((GET_MPIDR_AFF1 (ArmCoreInfoTable[Index].Mpidr) == ClusterId) &&
-        (GET_MPIDR_AFF0 (ArmCoreInfoTable[Index].Mpidr) == CoreId))
-    {
-      break;
-    }
-  }
-
-  // The ARM Core Info Table must define every core
-  ASSERT (Index != ArmCoreCount);
-
-  // Clear Secondary cores MailBox
-  MmioWrite32 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue);
-
-  do {
-    ArmCallWFI ();
-
-    // Read the Mailbox
-    SecondaryEntryAddr = MmioRead32 (ArmCoreInfoTable[Index].MailboxGetAddress);
-
-    // Acknowledge the interrupt and send End of Interrupt signal.
-    AcknowledgeInterrupt = ArmGicAcknowledgeInterrupt (PcdGet64 (PcdGicInterruptInterfaceBase), &InterruptId);
-    // Check if it is a valid interrupt ID
-    if (InterruptId < ArmGicGetMaxNumInterrupts (PcdGet64 (PcdGicDistributorBase))) {
-      // Got a valid SGI number hence signal End of Interrupt
-      ArmGicEndOfInterrupt (PcdGet64 (PcdGicInterruptInterfaceBase), AcknowledgeInterrupt);
-    }
-  } while (SecondaryEntryAddr == 0);
-
-  // Jump to secondary core entry point.
-  SecondaryStart = (VOID (*)()) SecondaryEntryAddr;
-  SecondaryStart ();
-
-  // The secondaries shouldn't reach here
-  ASSERT (FALSE);
-}
diff --git a/ArmPlatformPkg/PrePi/MainUniCore.c b/ArmPlatformPkg/PrePi/MainUniCore.c
index 6162d1241f84..7449facacd51 100644
--- a/ArmPlatformPkg/PrePi/MainUniCore.c
+++ b/ArmPlatformPkg/PrePi/MainUniCore.c
@@ -20,12 +20,3 @@ PrimaryMain (
    // We must never return
    ASSERT (FALSE);
  }
-
-VOID
-SecondaryMain (
-  IN  UINTN  MpId
-  )
-{
-  // We must never get into this function on UniCore system
-  ASSERT (FALSE);
-}
diff --git a/ArmPlatformPkg/PrePi/PrePi.c b/ArmPlatformPkg/PrePi/PrePi.c
index 9b127b94a67c..60061b8b6963 100644
--- a/ArmPlatformPkg/PrePi/PrePi.c
+++ b/ArmPlatformPkg/PrePi/PrePi.c
@@ -177,7 +177,11 @@ CEntryPoint (
    // Initialize the platform specific controllers
    ArmPlatformInitialize (MpId);

-  if (ArmPlatformIsPrimaryCore (MpId) && PerformanceMeasurementEnabled ()) {
+  if (!ArmPlatformIsPrimaryCore (MpId)) {
+    ASSERT (FALSE);
+  }
+
+  if (PerformanceMeasurementEnabled ()) {
      // Initialize the Timer Library to setup the Timer HW controller
      TimerConstructor ();
      // We cannot call yet the PerformanceLib because the HOB List has not been initialized
@@ -195,29 +199,21 @@ CEntryPoint (

    // Define the Global Variable region when we are not running in XIP
    if (!IS_XIP ()) {
-    if (ArmPlatformIsPrimaryCore (MpId)) {
-      if (ArmIsMpCore ()) {
-        // Signal the Global Variable Region is defined (event: ARM_CPU_EVENT_DEFAULT)
-        ArmCallSEV ();
-      }
-    } else {
-      // Wait the Primary core has defined the address of the Global Variable region (event: ARM_CPU_EVENT_DEFAULT)
-      ArmCallWFE ();
+    if (ArmIsMpCore ()) {
+      // Signal the Global Variable Region is defined (event: ARM_CPU_EVENT_DEFAULT)
+      ArmCallSEV ();
      }
    }

-  // If not primary Jump to Secondary Main
-  if (ArmPlatformIsPrimaryCore (MpId)) {
-    InvalidateDataCacheRange (
-      (VOID *)UefiMemoryBase,
-      FixedPcdGet32 (PcdSystemMemoryUefiRegionSize)
-      );
+  InvalidateDataCacheRange (
+    (VOID *)UefiMemoryBase,
+    FixedPcdGet32 (PcdSystemMemoryUefiRegionSize)
+    );

-    // Goto primary Main.
-    PrimaryMain (UefiMemoryBase, StacksBase, StartTimeStamp);
-  } else {
-    SecondaryMain (MpId);
-  }
+  PrePiMain (UefiMemoryBase, StacksBase, StartTimeStamp);
+
+  // We must never return
+  ASSERT (FALSE);

    // DXE Core should always load and never return
    ASSERT (FALSE);
--
2.30.2





Sami Mujawar
 

Hi Leif, Rebecca,

Apologies for the delay in getting back.

We can drop the TC2 support in edk2-platforms.

Regards,

Sami Mujawar

On 18/11/2022 02:09 pm, Rebecca Cran wrote:
Hi Sami,

I was wondering if you could answer Leif's question?

Thanks.
Rebecca Cran

On 10/28/22 03:17, Leif Lindholm wrote:
On Thu, Oct 27, 2022 at 20:10:33 +0200, Ard Biesheuvel wrote:
On Thu, 27 Oct 2022 at 19:31, Rebecca Cran <quic_rcran@...> wrote:

Modern platforms use TF-A, so there's no need for support of
secondary cores in EDK2 since TF-A will keep them in a holding
pen until the PSCI_CPU_ON SMC call is received.

Therefore, remove the code that handles secondary CPUs from
PrePeiCore and PrePi and add ASSERTs if a secondary core
reaches the functions.

Signed-off-by: Rebecca Cran <rebecca@...>
No objections to this patch, but this change will break the old SMP
32-bit ARM platforms in edk2-platforms so you will need to propose a
solution for those as well.
I think TC2 is the last one of those standing. And I don't see much
value in keeping all of this around for such a niche (and old)
platform. If someone ports TF-A to it, we could always add it back in
later.

Single-core non-PSCI platforms (i.e. beagleboard) aren't affected.

Sami: would you be OK with deleting the TC2 support in edk2-platforms?

/
     Leif

---
  ArmPlatformPkg/PrePeiCore/MainMPCore.c  | 92 --------------------
  ArmPlatformPkg/PrePeiCore/MainUniCore.c |  9 --
  ArmPlatformPkg/PrePeiCore/PrePeiCore.c  | 37 ++++----
  ArmPlatformPkg/PrePi/MainMPCore.c       | 69 ---------------
  ArmPlatformPkg/PrePi/MainUniCore.c      |  9 --
  ArmPlatformPkg/PrePi/PrePi.c            | 36 ++++----
  6 files changed, 34 insertions(+), 218 deletions(-)

diff --git a/ArmPlatformPkg/PrePeiCore/MainMPCore.c b/ArmPlatformPkg/PrePeiCore/MainMPCore.c
index b5d0d3a6442f..44850a4f3946 100644
--- a/ArmPlatformPkg/PrePeiCore/MainMPCore.c
+++ b/ArmPlatformPkg/PrePeiCore/MainMPCore.c
@@ -12,98 +12,6 @@

  #include "PrePeiCore.h"

-/*
- * This is the main function for secondary cores. They loop around until a non Null value is written to
- * SYS_FLAGS register.The SYS_FLAGS register is platform specific.
- * Note:The secondary cores, while executing secondary_main, assumes that:
- *      : SGI 0 is configured as Non-secure interrupt
- *      : Priority Mask is configured to allow SGI 0
- *      : Interrupt Distributor and CPU interfaces are enabled
- *
- */
-VOID
-EFIAPI
-SecondaryMain (
-  IN UINTN  MpId
-  )
-{
-  EFI_STATUS              Status;
-  UINTN                   PpiListSize;
-  UINTN                   PpiListCount;
-  EFI_PEI_PPI_DESCRIPTOR  *PpiList;
-  ARM_MP_CORE_INFO_PPI    *ArmMpCoreInfoPpi;
-  UINTN                   Index;
-  UINTN                   ArmCoreCount;
-  ARM_CORE_INFO           *ArmCoreInfoTable;
-  UINT32                  ClusterId;
-  UINT32                  CoreId;
-
-  VOID  (*SecondaryStart)(
-    VOID
-    );
-  UINTN  SecondaryEntryAddr;
-  UINTN  AcknowledgeInterrupt;
-  UINTN  InterruptId;
-
-  ClusterId = GET_CLUSTER_ID (MpId);
-  CoreId    = GET_CORE_ID (MpId);
-
-  // Get the gArmMpCoreInfoPpiGuid
-  PpiListSize = 0;
-  ArmPlatformGetPlatformPpiList (&PpiListSize, &PpiList);
-  PpiListCount = PpiListSize / sizeof (EFI_PEI_PPI_DESCRIPTOR);
-  for (Index = 0; Index < PpiListCount; Index++, PpiList++) {
-    if (CompareGuid (PpiList->Guid, &gArmMpCoreInfoPpiGuid) == TRUE) {
-      break;
-    }
-  }
-
-  // On MP Core Platform we must implement the ARM MP Core Info PPI
-  ASSERT (Index != PpiListCount);
-
-  ArmMpCoreInfoPpi = PpiList->Ppi;
-  ArmCoreCount     = 0;
-  Status           = ArmMpCoreInfoPpi->GetMpCoreInfo (&ArmCoreCount, &ArmCoreInfoTable);
-  ASSERT_EFI_ERROR (Status);
-
-  // Find the core in the ArmCoreTable
-  for (Index = 0; Index < ArmCoreCount; Index++) {
-    if ((GET_MPIDR_AFF1 (ArmCoreInfoTable[Index].Mpidr) == ClusterId) &&
-        (GET_MPIDR_AFF0 (ArmCoreInfoTable[Index].Mpidr) == CoreId))
-    {
-      break;
-    }
-  }
-
-  // The ARM Core Info Table must define every core
-  ASSERT (Index != ArmCoreCount);
-
-  // Clear Secondary cores MailBox
-  MmioWrite32 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue);
-
-  do {
-    ArmCallWFI ();
-
-    // Read the Mailbox
-    SecondaryEntryAddr = MmioRead32 (ArmCoreInfoTable[Index].MailboxGetAddress);
-
-    // Acknowledge the interrupt and send End of Interrupt signal.
-    AcknowledgeInterrupt = ArmGicAcknowledgeInterrupt (PcdGet64 (PcdGicInterruptInterfaceBase), &InterruptId);
-    // Check if it is a valid interrupt ID
-    if (InterruptId < ArmGicGetMaxNumInterrupts (PcdGet64 (PcdGicDistributorBase))) {
-      // Got a valid SGI number hence signal End of Interrupt
-      ArmGicEndOfInterrupt (PcdGet64 (PcdGicInterruptInterfaceBase), AcknowledgeInterrupt);
-    }
-  } while (SecondaryEntryAddr == 0);
-
-  // Jump to secondary core entry point.
-  SecondaryStart = (VOID (*)()) SecondaryEntryAddr;
-  SecondaryStart ();
-
-  // The secondaries shouldn't reach here
-  ASSERT (FALSE);
-}
-
  VOID
  EFIAPI
  PrimaryMain (
diff --git a/ArmPlatformPkg/PrePeiCore/MainUniCore.c b/ArmPlatformPkg/PrePeiCore/MainUniCore.c
index 1c2580eb923b..3d3c6caaa32a 100644
--- a/ArmPlatformPkg/PrePeiCore/MainUniCore.c
+++ b/ArmPlatformPkg/PrePeiCore/MainUniCore.c
@@ -8,15 +8,6 @@

  #include "PrePeiCore.h"

-VOID
-EFIAPI
-SecondaryMain (
-  IN UINTN  MpId
-  )
-{
-  ASSERT (FALSE);
-}
-
  VOID
  EFIAPI
  PrimaryMain (
diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
index 42a7ccc9c6a0..64d1ef601ea3 100644
--- a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
+++ b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
@@ -117,27 +117,26 @@ CEntryPoint (

    // Note: The MMU will be enabled by MemoryPeim. Only the primary core will have the MMU on.

-  // If not primary Jump to Secondary Main
-  if (ArmPlatformIsPrimaryCore (MpId)) {
-    // Invoke "ProcessLibraryConstructorList" to have all library constructors
-    // called.
-    ProcessLibraryConstructorList ();
-
-    PrintFirmwareVersion ();
-
-    // Initialize the Debug Agent for Source Level Debugging
-    InitializeDebugAgent (DEBUG_AGENT_INIT_POSTMEM_SEC, NULL, NULL);
-    SaveAndSetDebugTimerInterrupt (TRUE);
-
-    // Initialize the platform specific controllers
-    ArmPlatformInitialize (MpId);
-
-    // Goto primary Main.
-    PrimaryMain (PeiCoreEntryPoint);
-  } else {
-    SecondaryMain (MpId);
+  if (!ArmPlatformIsPrimaryCore (MpId)) {
+    ASSERT (FALSE);
    }

+  // Invoke "ProcessLibraryConstructorList" to have all library constructors
+  // called.
+  ProcessLibraryConstructorList ();
+
+  PrintFirmwareVersion ();
+
+  // Initialize the Debug Agent for Source Level Debugging
+  InitializeDebugAgent (DEBUG_AGENT_INIT_POSTMEM_SEC, NULL, NULL);
+  SaveAndSetDebugTimerInterrupt (TRUE);
+
+  // Initialize the platform specific controllers
+  ArmPlatformInitialize (MpId);
+
+  // Goto primary Main.
+  PrimaryMain (PeiCoreEntryPoint);
+
    // PEI Core should always load and never return
    ASSERT (FALSE);
  }
diff --git a/ArmPlatformPkg/PrePi/MainMPCore.c b/ArmPlatformPkg/PrePi/MainMPCore.c
index 68a7c13298d0..ce7058a2846f 100644
--- a/ArmPlatformPkg/PrePi/MainMPCore.c
+++ b/ArmPlatformPkg/PrePi/MainMPCore.c
@@ -33,72 +33,3 @@ PrimaryMain (
    // We must never return
    ASSERT (FALSE);
  }
-
-VOID
-SecondaryMain (
-  IN  UINTN  MpId
-  )
-{
-  EFI_STATUS            Status;
-  ARM_MP_CORE_INFO_PPI  *ArmMpCoreInfoPpi;
-  UINTN                 Index;
-  UINTN                 ArmCoreCount;
-  ARM_CORE_INFO         *ArmCoreInfoTable;
-  UINT32                ClusterId;
-  UINT32                CoreId;
-
-  VOID  (*SecondaryStart)(
-    VOID
-    );
-  UINTN  SecondaryEntryAddr;
-  UINTN  AcknowledgeInterrupt;
-  UINTN  InterruptId;
-
-  ClusterId = GET_CLUSTER_ID (MpId);
-  CoreId    = GET_CORE_ID (MpId);
-
-  // On MP Core Platform we must implement the ARM MP Core Info PPI (gArmMpCoreInfoPpiGuid)
-  Status = GetPlatformPpi (&gArmMpCoreInfoPpiGuid, (VOID **)&ArmMpCoreInfoPpi);
-  ASSERT_EFI_ERROR (Status);
-
-  ArmCoreCount = 0;
-  Status       = ArmMpCoreInfoPpi->GetMpCoreInfo (&ArmCoreCount, &ArmCoreInfoTable);
-  ASSERT_EFI_ERROR (Status);
-
-  // Find the core in the ArmCoreTable
-  for (Index = 0; Index < ArmCoreCount; Index++) {
-    if ((GET_MPIDR_AFF1 (ArmCoreInfoTable[Index].Mpidr) == ClusterId) &&
-        (GET_MPIDR_AFF0 (ArmCoreInfoTable[Index].Mpidr) == CoreId))
-    {
-      break;
-    }
-  }
-
-  // The ARM Core Info Table must define every core
-  ASSERT (Index != ArmCoreCount);
-
-  // Clear Secondary cores MailBox
-  MmioWrite32 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue);
-
-  do {
-    ArmCallWFI ();
-
-    // Read the Mailbox
-    SecondaryEntryAddr = MmioRead32 (ArmCoreInfoTable[Index].MailboxGetAddress);
-
-    // Acknowledge the interrupt and send End of Interrupt signal.
-    AcknowledgeInterrupt = ArmGicAcknowledgeInterrupt (PcdGet64 (PcdGicInterruptInterfaceBase), &InterruptId);
-    // Check if it is a valid interrupt ID
-    if (InterruptId < ArmGicGetMaxNumInterrupts (PcdGet64 (PcdGicDistributorBase))) {
-      // Got a valid SGI number hence signal End of Interrupt
-      ArmGicEndOfInterrupt (PcdGet64 (PcdGicInterruptInterfaceBase), AcknowledgeInterrupt);
-    }
-  } while (SecondaryEntryAddr == 0);
-
-  // Jump to secondary core entry point.
-  SecondaryStart = (VOID (*)()) SecondaryEntryAddr;
-  SecondaryStart ();
-
-  // The secondaries shouldn't reach here
-  ASSERT (FALSE);
-}
diff --git a/ArmPlatformPkg/PrePi/MainUniCore.c b/ArmPlatformPkg/PrePi/MainUniCore.c
index 6162d1241f84..7449facacd51 100644
--- a/ArmPlatformPkg/PrePi/MainUniCore.c
+++ b/ArmPlatformPkg/PrePi/MainUniCore.c
@@ -20,12 +20,3 @@ PrimaryMain (
    // We must never return
    ASSERT (FALSE);
  }
-
-VOID
-SecondaryMain (
-  IN  UINTN  MpId
-  )
-{
-  // We must never get into this function on UniCore system
-  ASSERT (FALSE);
-}
diff --git a/ArmPlatformPkg/PrePi/PrePi.c b/ArmPlatformPkg/PrePi/PrePi.c
index 9b127b94a67c..60061b8b6963 100644
--- a/ArmPlatformPkg/PrePi/PrePi.c
+++ b/ArmPlatformPkg/PrePi/PrePi.c
@@ -177,7 +177,11 @@ CEntryPoint (
    // Initialize the platform specific controllers
    ArmPlatformInitialize (MpId);

-  if (ArmPlatformIsPrimaryCore (MpId) && PerformanceMeasurementEnabled ()) {
+  if (!ArmPlatformIsPrimaryCore (MpId)) {
+    ASSERT (FALSE);
+  }
+
+  if (PerformanceMeasurementEnabled ()) {
      // Initialize the Timer Library to setup the Timer HW controller
      TimerConstructor ();
      // We cannot call yet the PerformanceLib because the HOB List has not been initialized
@@ -195,29 +199,21 @@ CEntryPoint (

    // Define the Global Variable region when we are not running in XIP
    if (!IS_XIP ()) {
-    if (ArmPlatformIsPrimaryCore (MpId)) {
-      if (ArmIsMpCore ()) {
-        // Signal the Global Variable Region is defined (event: ARM_CPU_EVENT_DEFAULT)
-        ArmCallSEV ();
-      }
-    } else {
-      // Wait the Primary core has defined the address of the Global Variable region (event: ARM_CPU_EVENT_DEFAULT)
-      ArmCallWFE ();
+    if (ArmIsMpCore ()) {
+      // Signal the Global Variable Region is defined (event: ARM_CPU_EVENT_DEFAULT)
+      ArmCallSEV ();
      }
    }

-  // If not primary Jump to Secondary Main
-  if (ArmPlatformIsPrimaryCore (MpId)) {
-    InvalidateDataCacheRange (
-      (VOID *)UefiMemoryBase,
-      FixedPcdGet32 (PcdSystemMemoryUefiRegionSize)
-      );
+  InvalidateDataCacheRange (
+    (VOID *)UefiMemoryBase,
+    FixedPcdGet32 (PcdSystemMemoryUefiRegionSize)
+    );

-    // Goto primary Main.
-    PrimaryMain (UefiMemoryBase, StacksBase, StartTimeStamp);
-  } else {
-    SecondaryMain (MpId);
-  }
+  PrePiMain (UefiMemoryBase, StacksBase, StartTimeStamp);
+
+  // We must never return
+  ASSERT (FALSE);

    // DXE Core should always load and never return
    ASSERT (FALSE);
--
2.30.2