Hi Sami, I just sent the V3 series to address your comments. Regards, On Thu, Sep 28, 2023 at 11:16 AM Tuan Phan via groups.io wrote: > 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 (#109180): https://edk2.groups.io/g/devel/message/109180 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] -=-=-=-=-=-=-=-=-=-=-=-