From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx0a-0031df01.pphosted.com (mx0a-0031df01.pphosted.com [205.220.168.131]) by mx.groups.io with SMTP id smtpd.web11.4228.1666948688105308881 for ; Fri, 28 Oct 2022 02:18:08 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@quicinc.com header.s=qcppdkim1 header.b=BAUQBcp6; spf=permerror, err=parse error for token &{10 18 %{ir}.%{v}.%{d}.spf.has.pphosted.com}: invalid domain name (domain: quicinc.com, ip: 205.220.168.131, mailfrom: quic_llindhol@quicinc.com) Received: from pps.filterd (m0279866.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.5/8.17.1.5) with ESMTP id 29S6J2iA027434; Fri, 28 Oct 2022 09:18:05 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com; h=date : from : to : cc : subject : message-id : references : mime-version : content-type : in-reply-to; s=qcppdkim1; bh=ZLt8wESorxn4W5UbvIkhV0hB3/TktnXf817EAI24Wic=; b=BAUQBcp6p49LnKWzDbCHTyPvwVAsyF/DgxtDYrUxvkKG9V8oMbuETl8dgfpv/xu7QmIG gvwT5MXMNsLeN9QNGvod6hYUmIfdLlU1NlnA/uHuqp8QwNdctFWCwoSYTzXgR5zTRM/N KgBr4Ngocztpe5lmj84uc5+z80jHZHkHHsaNPYoGT2LG4d6xb9fOUL7pJwlbuH81jiK8 YcQj7ywK8OZUYWSTKUBLAdYAC++tmKr0Ash5nw/SdsjVkCIf8vRKpj5SynZZ2xCkO1XG TckBahIPBRo4aC/gEtw9T4fCHu57Y9jH4uS3N5/UWYDlKnhtYvKxtF0IxaZ64RA5cJOQ cA== Received: from nasanppmta04.qualcomm.com (i-global254.qualcomm.com [199.106.103.254]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3kfyf7j2n8-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 28 Oct 2022 09:18:04 +0000 Received: from nasanex01c.na.qualcomm.com (nasanex01c.na.qualcomm.com [10.45.79.139]) by NASANPPMTA04.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 29S9I4nP003975 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 28 Oct 2022 09:18:04 GMT Received: from qc-i7.hemma.eciton.net (10.80.80.8) by nasanex01c.na.qualcomm.com (10.45.79.139) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.986.29; Fri, 28 Oct 2022 02:18:01 -0700 Date: Fri, 28 Oct 2022 10:17:59 +0100 From: "Leif Lindholm" To: Ard Biesheuvel , Sami Mujawar , Thomas Abraham CC: , , Ard Biesheuvel , Rebecca Cran Subject: Re: [edk2-devel] [PATCH 1/1] ArmPlatformPkg: Remove AP support from PrePi/PrePeiCore Message-ID: References: <20221027173121.754041-1-rebecca@quicinc.com> MIME-Version: 1.0 In-Reply-To: X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01a.na.qualcomm.com (10.52.223.231) To nasanex01c.na.qualcomm.com (10.45.79.139) X-QCInternal: smtphost X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=5800 signatures=585085 X-Proofpoint-ORIG-GUID: Zfm8MZIIAxiHRDkad0DqHLnKWxa9DWYN X-Proofpoint-GUID: Zfm8MZIIAxiHRDkad0DqHLnKWxa9DWYN X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1 definitions=2022-10-28_04,2022-10-27_01,2022-06-22_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 mlxlogscore=778 phishscore=0 spamscore=0 malwarescore=0 clxscore=1011 lowpriorityscore=0 suspectscore=0 priorityscore=1501 mlxscore=0 bulkscore=0 adultscore=0 impostorscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2210170000 definitions=main-2210280058 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline On Thu, Oct 27, 2022 at 20:10:33 +0200, Ard Biesheuvel wrote: > On Thu, 27 Oct 2022 at 19:31, Rebecca Cran 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 > > 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 > > > > > > > > > > > >