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 <tphan=
ventanamicro....@groups.io> wrote:
> Hi Sami,
> Please see my comments below.
>
> On Thu, Sep 28, 2023 at 9:16 AM Sami Mujawar <sami.muja...@arm.com> 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 <tp...@ventanamicro.com>
>> > ---
>> > .../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 <StandaloneMmCpu.h>
>> >
>> > #include <Library/PeCoffLib.h>
>> >
>> > #include <Library/FvLib.h>
>> >
>> >
>> >
>> > @@ -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 <Guid/MmramMemoryReserve.h>
>> >
>> > #include <Guid/MpInformation.h>
>> >
>> >
>> >
>> > +#include <StandaloneMmCpu.h>
>> >
>> > #include <Library/Arm/StandaloneMmCoreEntryPoint.h>
>> >
>> > #include <Library/ArmMmuLib.h>
>> >
>> > #include <Library/ArmSvcLib.h>
>> >
>> > @@ -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 <Guid/MmramMemoryReserve.h>
>> >
>> > #include <Guid/MpInformation.h>
>> >
>> >
>> >
>> > +#include <StandaloneMmCpu.h>
>> >
>> > #include <Library/ArmSvcLib.h>
>> >
>> > #include <Library/DebugLib.h>
>> >
>> > #include <Library/HobLib.h>
>> >
>> > @@ -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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
