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