Hi Sami, Please see my comments below. On Thu, Sep 28, 2023 at 9:16 AM Sami Mujawar wrote: > Hi Tuan, > > Thank you for this patch. > > Please see my response inline marked [SAMI]. > > Regards, > > Sami Mujawar > > On 15/09/2023 12:10 am, Tuan Phan wrote: > > Update entry point library for Arm to use the new platform independent > > [SAMI] Should this be worded as architecture independent instead of > platform independent? > > Can you also check the subject line and commit message for patch 1/2, > please? > [Tuan] Sure, that makes sense. > > [/SAMI] > > > StandaloneMmCpu driver. > > > > Signed-off-by: Tuan Phan > > --- > > .../Library/Arm/StandaloneMmCoreEntryPoint.h | 17 ++------ > > .../Arm/CreateHobList.c | 43 ++++++++++--------- > > .../Arm/StandaloneMmCoreEntryPoint.c | 15 ++++++- > > .../StandaloneMmCoreEntryPoint.inf | 2 +- > > 4 files changed, 40 insertions(+), 37 deletions(-) > > > > diff --git > a/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h > b/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h > > index 41bf0f132b4f..dbb81610ff8e 100644 > > --- a/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h > > +++ b/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h > > @@ -10,6 +10,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > #ifndef __STANDALONEMMCORE_ENTRY_POINT_H__ > > > > #define __STANDALONEMMCORE_ENTRY_POINT_H__ > > > > > > > > +#include > > > > #include > > > > #include > > > > > > > > @@ -47,18 +48,6 @@ typedef struct { > > EFI_SECURE_PARTITION_CPU_INFO *CpuInfo; > > > > } EFI_SECURE_PARTITION_BOOT_INFO; > > > > > > > > -typedef > > > > -EFI_STATUS > > > > -(*PI_MM_ARM_TF_CPU_DRIVER_ENTRYPOINT) ( > > > > - IN UINTN EventId, > > > > - IN UINTN CpuNumber, > > > > - IN UINTN NsCommBufferAddr > > > > - ); > > > > - > > > > -typedef struct { > > > > - PI_MM_ARM_TF_CPU_DRIVER_ENTRYPOINT *ArmTfCpuDriverEpPtr; > > > > -} ARM_TF_CPU_DRIVER_EP_DESCRIPTOR; > > > > - > > > > typedef RETURN_STATUS (*REGION_PERMISSION_UPDATE_FUNC) ( > > > > IN EFI_PHYSICAL_ADDRESS BaseAddress, > > > > IN UINT64 Length > > > > @@ -145,8 +134,8 @@ LocateStandaloneMmCorePeCoffData ( > > VOID * > > > > EFIAPI > > > > CreateHobListFromBootInfo ( > > > > - IN OUT PI_MM_ARM_TF_CPU_DRIVER_ENTRYPOINT *CpuDriverEntryPoint, > > > > - IN EFI_SECURE_PARTITION_BOOT_INFO *PayloadBootInfo > > > > + IN OUT PI_MM_CPU_DRIVER_ENTRYPOINT *CpuDriverEntryPoint, > > > > + IN EFI_SECURE_PARTITION_BOOT_INFO *PayloadBootInfo > > > > ); > > > > > > > > /** > > > > diff --git > a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c > b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c > > index 2ac2d354f06a..80ed532352af 100644 > > --- > a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c > > +++ > b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c > > @@ -13,6 +13,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > #include > > > > #include > > > > > > > > +#include > > > > #include > > > > #include > > > > #include > > > > @@ -39,7 +40,7 @@ extern EFI_GUID gEfiStandaloneMmNonSecureBufferGuid; > > // GUID to identify HOB where the entry point of the CPU driver will be > > > > // populated to allow this entry point driver to invoke it upon > receipt of an > > > > // event > > > > -extern EFI_GUID gEfiArmTfCpuDriverEpDescriptorGuid; > > > > +extern EFI_GUID gEfiMmCpuDriverEpDescriptorGuid; > > > > > > > > /** > > > > Use the boot information passed by privileged firmware to populate a > HOB list > > > > @@ -52,22 +53,22 @@ extern EFI_GUID gEfiArmTfCpuDriverEpDescriptorGuid; > > **/ > > > > VOID * > > > > CreateHobListFromBootInfo ( > > > > - IN OUT PI_MM_ARM_TF_CPU_DRIVER_ENTRYPOINT *CpuDriverEntryPoint, > > > > - IN EFI_SECURE_PARTITION_BOOT_INFO *PayloadBootInfo > > > > + IN OUT PI_MM_CPU_DRIVER_ENTRYPOINT *CpuDriverEntryPoint, > > > > + IN EFI_SECURE_PARTITION_BOOT_INFO *PayloadBootInfo > > > > ) > > > > { > > > > - EFI_HOB_HANDOFF_INFO_TABLE *HobStart; > > > > - EFI_RESOURCE_ATTRIBUTE_TYPE Attributes; > > > > - UINT32 Index; > > > > - UINT32 BufferSize; > > > > - UINT32 Flags; > > > > - EFI_MMRAM_HOB_DESCRIPTOR_BLOCK *MmramRangesHob; > > > > - EFI_MMRAM_DESCRIPTOR *MmramRanges; > > > > - EFI_MMRAM_DESCRIPTOR *NsCommBufMmramRange; > > > > - MP_INFORMATION_HOB_DATA *MpInformationHobData; > > > > - EFI_PROCESSOR_INFORMATION *ProcInfoBuffer; > > > > - EFI_SECURE_PARTITION_CPU_INFO *CpuInfo; > > > > - ARM_TF_CPU_DRIVER_EP_DESCRIPTOR *CpuDriverEntryPointDesc; > > > > + EFI_HOB_HANDOFF_INFO_TABLE *HobStart; > > > > + EFI_RESOURCE_ATTRIBUTE_TYPE Attributes; > > > > + UINT32 Index; > > > > + UINT32 BufferSize; > > > > + UINT32 Flags; > > > > + EFI_MMRAM_HOB_DESCRIPTOR_BLOCK *MmramRangesHob; > > > > + EFI_MMRAM_DESCRIPTOR *MmramRanges; > > > > + EFI_MMRAM_DESCRIPTOR *NsCommBufMmramRange; > > > > + MP_INFORMATION_HOB_DATA *MpInformationHobData; > > > > + EFI_PROCESSOR_INFORMATION *ProcInfoBuffer; > > > > + EFI_SECURE_PARTITION_CPU_INFO *CpuInfo; > > > > + MM_CPU_DRIVER_EP_DESCRIPTOR *CpuDriverEntryPointDesc; > > > > > > > > // Create a hoblist with a PHIT and EOH > > > > HobStart = HobConstructor ( > > > > @@ -144,13 +145,13 @@ CreateHobListFromBootInfo ( > > > > > > // Create a Guided HOB to enable the ARM TF CPU driver to share its > entry > > > > // point and populate it with the address of the shared buffer > > > > - CpuDriverEntryPointDesc = (ARM_TF_CPU_DRIVER_EP_DESCRIPTOR > *)BuildGuidHob ( > > > > - > &gEfiArmTfCpuDriverEpDescriptorGuid, > > > > - sizeof > (ARM_TF_CPU_DRIVER_EP_DESCRIPTOR) > > > > - ); > > > > + CpuDriverEntryPointDesc = (MM_CPU_DRIVER_EP_DESCRIPTOR *)BuildGuidHob > ( > > > > + > &gEfiMmCpuDriverEpDescriptorGuid, > > > > + sizeof > (MM_CPU_DRIVER_EP_DESCRIPTOR) > > > > + ); > > > > > > > > - *CpuDriverEntryPoint = NULL; > > > > - CpuDriverEntryPointDesc->ArmTfCpuDriverEpPtr = CpuDriverEntryPoint; > > > > + *CpuDriverEntryPoint = NULL; > > > > + CpuDriverEntryPointDesc->MmCpuDriverEpPtr = CpuDriverEntryPoint; > > > > > > > > // Find the size of the GUIDed HOB with SRAM ranges > > > > BufferSize = sizeof (EFI_MMRAM_HOB_DESCRIPTOR_BLOCK); > > > > diff --git > a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c > b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c > > index 96de10405af8..900e0252ef9f 100644 > > --- > a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c > > +++ > b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c > > @@ -15,6 +15,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > #include > > > > #include > > > > > > > > +#include > > > > #include > > > > #include > > > > #include > > > > @@ -41,7 +42,7 @@ STATIC CONST UINT32 mSpmMinorVerFfa = > SPM_MINOR_VERSION_FFA; > > > > > > #define BOOT_PAYLOAD_VERSION 1 > > > > > > > > -PI_MM_ARM_TF_CPU_DRIVER_ENTRYPOINT CpuDriverEntryPoint = NULL; > > > > +PI_MM_CPU_DRIVER_ENTRYPOINT CpuDriverEntryPoint = NULL; > > > > > > > > /** > > > > Retrieve a pointer to and print the boot information passed by > privileged > > > > @@ -140,6 +141,18 @@ DelegatedEventLoop ( > > DEBUG ((DEBUG_INFO, "X6 : 0x%x\n", > (UINT32)EventCompleteSvcArgs->Arg6)); > > > > DEBUG ((DEBUG_INFO, "X7 : 0x%x\n", > (UINT32)EventCompleteSvcArgs->Arg7)); > > > > > > > > + // > > > > + // ARM TF passes SMC FID of the MM_COMMUNICATE interface as the > Event ID upon > > > > + // receipt of a synchronous MM request. Use the Event ID to > distinguish > > > > + // between synchronous and asynchronous events. > > > > + // > > > > + if ((ARM_SMC_ID_MM_COMMUNICATE != > (UINT32)EventCompleteSvcArgs->Arg0) && > > > > + (ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ != > (UINT32)EventCompleteSvcArgs->Arg0)) > > > > + { > > > > + DEBUG ((DEBUG_ERROR, "UnRecognized Event - 0x%x\n", > (UINT32)EventCompleteSvcArgs->Arg0)); > > > > + continue; > > [SAMI] I think an error needs to be returned instead of continuing > otherwise this changes the original behaviour. > > Status needs to be set to EFI_INVALID_PARAMETER here. > > [/SAMI] > > > > + } > > [SAMI] The code from this point needs to be enclosed in an else block > until before the switch statement. > > That way the proper error code would be returned. Can you check, please? > > [/SAMI] > [Tuan] Thanks for the catch. Will fix it > > > > > + > > > > FfaEnabled = FeaturePcdGet (PcdFfaEnable); > > > > if (FfaEnabled) { > > > > Status = CpuDriverEntryPoint ( > > > > diff --git > a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf > b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf > > index 75cfb98c0e75..d41d7630b614 100644 > > --- > a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf > > +++ > b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf > > @@ -49,7 +49,7 @@ > > gMpInformationHobGuid > > > > gEfiMmPeiMmramMemoryReserveGuid > > > > gEfiStandaloneMmNonSecureBufferGuid > > > > - gEfiArmTfCpuDriverEpDescriptorGuid > > > > + gEfiMmCpuDriverEpDescriptorGuid > > > > > > > > [FeaturePcd.ARM, FeaturePcd.AARCH64] > > > > gArmTokenSpaceGuid.PcdFfaEnable > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109174): https://edk2.groups.io/g/devel/message/109174 Mute This Topic: https://groups.io/mt/101369647/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-