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

["Rebecca Cran" <quic_rcran@quicinc.com>]

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