From: "Sami Mujawar" <sami.mujawar@arm.com>
To: Rebecca Cran <rebecca@quicinc.com>,
Leif Lindholm <quic_llindhol@quicinc.com>,
Ard Biesheuvel <ardb@kernel.org>,
Thomas Abraham <thomas.abraham@arm.com>
Cc: devel@edk2.groups.io, quic_rcran@quicinc.com,
Ard Biesheuvel <ardb+tianocore@kernel.org>,
"nd@arm.com" <nd@arm.com>
Subject: Re: [edk2-devel] [PATCH 1/1] ArmPlatformPkg: Remove AP support from PrePi/PrePeiCore
Date: Thu, 24 Nov 2022 14:52:02 +0000 [thread overview]
Message-ID: <40e22f09-d7de-bd40-dd1f-9e0d9e29238c@arm.com> (raw)
In-Reply-To: <1a0045d6-d826-ab1d-4297-cac91ca7cac9@quicinc.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
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
prev parent reply other threads:[~2022-11-24 14:52 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-27 17:31 [PATCH 1/1] ArmPlatformPkg: Remove AP support from PrePi/PrePeiCore Rebecca Cran
2022-10-27 18:10 ` [edk2-devel] " Ard Biesheuvel
2022-10-28 9:17 ` Leif Lindholm
2022-11-18 14:09 ` Rebecca Cran
2022-11-24 14:51 ` Sami Mujawar
2022-11-24 14:52 ` Sami Mujawar [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=40e22f09-d7de-bd40-dd1f-9e0d9e29238c@arm.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox