Re: [edk2-devel] [PATCH v2 2/2] StandaloneMmPkg: Arm: Update to use the new StandaloneMmCpu driver

["Tuan Phan" <tphan@ventanamicro.com>]

[-- Attachment #1: Type: text/plain, Size: 11393 bytes --]

Hi Sami,
Please see my comments below.

On Thu, Sep 28, 2023 at 9:16 AM Sami Mujawar <sami.mujawar@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 <tphan@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 (#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]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 15276 bytes --]