Re: [edk2-devel] [PATCH 1/1] ArmPlatformPkg: Remove AP support from PrePi/PrePeiCore

["Ard Biesheuvel" <ardb@kernel.org>]

On Thu, 27 Oct 2022 at 19:31, Rebecca Cran <quic_rcran@quicinc.com> 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@quicinc.com>

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
>
>
>
> 
>
>