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

["Sami Mujawar" <sami.mujawar@arm.com>]

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