Hi Ard,

Thanks for the feedback. I updated the patch as you suggested and sent out a v3 here:
https://edk2.groups.io/g/devel/message/106443.

The only call-out was the "CONST" qualifier for "mPeiMmCommunication" was cast away when initializing "mPeiMmCommunicationPpi". This was because the "Ppi" field inside "EFI_PEI_PPI_DESCRIPTOR" was defined as "VOID*" and not doing so will cause the build
failure.

Please let me know if you have further feedback. Thanks in advance.

Regards,
Kun

On 6/27/2023 7:36 AM, Ard Biesheuvel wrote:
On Mon, 26 Jun 2023 at 22:00, Kun Qin <kuqi...@gmail.com> wrote:
From: Kun Qin <ku...@microsoft.com>

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4464

This change introduced the MM communicate support in PEI phase for ARM
based platforms. Similar to the DXE counterpart, `PcdMmBufferBase` is
used as communicate buffer and SMC will be invoked to communicate to
TrustZone when MMI is requested.

Cc: Leif Lindholm <quic_llind...@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org>
Cc: Sami Mujawar <sami.muja...@arm.com>

Co-authored-by: Ronny Hansen <hansen.ro...@microsoft.com>
Co-authored-by: Shriram Masanamuthu Chinnathurai <shrira...@microsoft.com>
Co-authored-by: Preshit Harlikar <pharli...@microsoft.com>
Signed-off-by: Kun Qin <ku...@microsoft.com>
---

Notes:
     v2:
     - Adjustment to CommSize checks [Sami]
     - Added more debug prints for error returns.

  ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.c   | 212 
++++++++++++++++++++
  ArmPkg/ArmPkg.dsc                                        |   2 +
  ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.h   |  76 +++++++
  ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.inf |  41 ++++
  4 files changed, 331 insertions(+)

diff --git a/ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.c 
b/ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.c
new file mode 100644
index 000000000000..8661f0c8ee49
--- /dev/null
+++ b/ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.c
@@ -0,0 +1,212 @@
+/** @file -- MmCommunicationPei.c
+  Provides an interface to send MM request in PEI
+
+  Copyright (c) 2016-2021, Arm Limited. All rights reserved.<BR>
+  Copyright (c) Microsoft Corporation.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include "MmCommunicationPei.h"
+
+//
+// Module globals
+//
STATIC CONST

+EFI_PEI_MM_COMMUNICATION_PPI  mPeiMmCommunication = {
+  MmCommunicationPeim
+};
+
STATIC CONST

+EFI_PEI_PPI_DESCRIPTOR  mPeiMmCommunicationPpi = {
+  (EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST),
+  &gEfiPeiMmCommunicationPpiGuid,
+  &mPeiMmCommunication
+};
+
+/**
+  Entry point of PEI MM Communication driver
+
+  @param  FileHandle   Handle of the file being invoked.
+                       Type EFI_PEI_FILE_HANDLE is defined in 
FfsFindNextFile().
+  @param  PeiServices  General purpose services available to every PEIM.
+
+  @retval EFI_SUCCESS  If the interface could be successfully installed
+  @retval Others       Returned from PeiServicesInstallPpi()
+**/
+EFI_STATUS
+EFIAPI
+MmCommunicationPeiInitialize (
+  IN       EFI_PEI_FILE_HANDLE  FileHandle,
+  IN CONST EFI_PEI_SERVICES     **PeiServices
+  )
+{
+  return PeiServicesInstallPpi (&mPeiMmCommunicationPpi);
+}
+
+/**
+  MmCommunicationPeim
+  Communicates with a registered handler.
+  This function provides a service to send and receive messages from a 
registered UEFI service during PEI.
+
+  @param[in]      This            The EFI_PEI_MM_COMMUNICATION_PPI instance.
+  @param[in, out] CommBuffer      Pointer to the data buffer
+  @param[in, out] CommSize        The size of the data buffer being passed in. 
On exit, the
+                                  size of data being returned. Zero if the 
handler does not
+                                  wish to reply with any data.
+
+  @retval EFI_SUCCESS             The message was successfully posted.
+  @retval EFI_INVALID_PARAMETER   CommBuffer or CommSize was NULL, or 
*CommSize does not
+                                  match MessageLength + sizeof 
(EFI_MM_COMMUNICATE_HEADER).
+  @retval EFI_BAD_BUFFER_SIZE     The buffer is too large for the MM 
implementation.
+                                  If this error is returned, the MessageLength 
field
+                                  in the CommBuffer header or the integer 
pointed by
+                                  CommSize, are updated to reflect the maximum 
payload
+                                  size the implementation can accommodate.
+  @retval EFI_ACCESS_DENIED       The CommunicateBuffer parameter or CommSize 
parameter,
+                                  if not omitted, are in address range that 
cannot be
+                                  accessed by the MM environment.
+**/
This can be STATIC as well - no need to declare it in the header file
either (but you may need to reorder this file to avoid the need for a
forward declaration)


+EFI_STATUS
+EFIAPI
+MmCommunicationPeim (
+  IN CONST EFI_PEI_MM_COMMUNICATION_PPI  *This,
+  IN OUT VOID                            *CommBuffer,
+  IN OUT UINTN                           *CommSize
+  )
+{
+  EFI_MM_COMMUNICATE_HEADER  *CommunicateHeader;
+  EFI_MM_COMMUNICATE_HEADER  *TempCommHeader;
+  ARM_SMC_ARGS               CommunicateSmcArgs;
+  EFI_STATUS                 Status;
+  UINTN                      BufferSize;
+
+  ZeroMem (&CommunicateSmcArgs, sizeof (ARM_SMC_ARGS));
+
+  // Check that our static buffer is looking good.
+  // We are using PcdMmBufferBase to transfer variable data.
+  // We are not using the full size of the buffer since there is a cost
+  // of copying data between Normal and Secure World.
+  ASSERT (PcdGet64 (PcdMmBufferSize) > 0 && PcdGet64 (PcdMmBufferBase) != 0);
+
Please split these up into two separate ASSERT()s

+  //
+  // Check parameters
+  //
+  if ((CommBuffer == NULL) || (CommSize == NULL)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a One or more incoming pointers are NULL %p and %p!\n",
+      __func__,
+      CommBuffer,
+      CommSize
+      ));
Please drop the DEBUG() and use an ASSERT() for each parameter.
(Asserts are self documenting)

+    return EFI_INVALID_PARAMETER;
+  }
+
+  // If the length of the CommBuffer is 0 then return the expected length.
+  // This case can be used by the consumer of this driver to find out the
+  // max size that can be used for allocating CommBuffer.
+  if ((*CommSize == 0) || (*CommSize > (UINTN)PcdGet64 (PcdMmBufferSize))) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a Invalid CommSize value 0x%llx!\n",
+      __func__,
+      *CommSize
+      ));
+    *CommSize = (UINTN)PcdGet64 (PcdMmBufferSize);
+    return EFI_BAD_BUFFER_SIZE;
+  }
+
+  // Given CommBuffer is not NULL here, we use it to test the legitimacy of 
CommSize.
+  TempCommHeader = (EFI_MM_COMMUNICATE_HEADER *)(UINTN)CommBuffer;
+
+  // CommBuffer is a mandatory parameter. Hence, Rely on
+  // MessageLength + Header to ascertain the
+  // total size of the communication payload rather than
+  // rely on optional CommSize parameter
+  BufferSize = TempCommHeader->MessageLength +
+               sizeof (TempCommHeader->HeaderGuid) +
+               sizeof (TempCommHeader->MessageLength);
+
+  //
+  // If CommSize is supplied it must match MessageLength + sizeof 
(EFI_MM_COMMUNICATE_HEADER);
+  //
+  if (*CommSize != BufferSize) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a Unexpected CommSize value, has: 0x%llx vs. expected: 0x%llx!\n",
+      __func__,
+      *CommSize,
+      BufferSize
+      ));
+    return EFI_INVALID_PARAMETER;
+  }
+
+  // Now we know that the size is something we can handle, copy it over to the 
designated comm buffer.
+  CommunicateHeader = (EFI_MM_COMMUNICATE_HEADER *)(UINTN)(PcdGet64 
(PcdMmBufferBase));
+
+  CopyMem ((VOID *)CommunicateHeader, CommBuffer, *CommSize);
No need for a (VOID*) cast

+
+  // SMC Function ID
+  CommunicateSmcArgs.Arg0 = ARM_SMC_ID_MM_COMMUNICATE_AARCH64;
+
+  // Cookie
+  CommunicateSmcArgs.Arg1 = 0;
+
+  // comm_buffer_address (64-bit physical address)
+  CommunicateSmcArgs.Arg2 = (UINTN)CommunicateHeader;
+
+  // comm_size_address (not used, indicated by setting to zero)
+  CommunicateSmcArgs.Arg3 = 0;
+
+  // Call the Standalone MM environment.
+  ArmCallSmc (&CommunicateSmcArgs);
+
+  switch (CommunicateSmcArgs.Arg0) {
+    case ARM_SMC_MM_RET_SUCCESS:
+      // On successful return, the size of data being returned is inferred from
+      // MessageLength + Header.
+      BufferSize = CommunicateHeader->MessageLength +
+                   sizeof (CommunicateHeader->HeaderGuid) +
+                   sizeof (CommunicateHeader->MessageLength);
+      if (BufferSize > (UINTN)PcdGet64 (PcdMmBufferSize)) {
+        // Something bad has happened, we should have landed in 
ARM_SMC_MM_RET_NO_MEMORY
+        DEBUG ((
+          DEBUG_ERROR,
+          "%a Returned buffer exceeds communication buffer limit. Has: 0x%llx vs. 
max: 0x%llx!\n",
+          __func__,
+          BufferSize,
+          (UINTN)PcdGet64 (PcdMmBufferSize)
+          ));
+        Status = EFI_BAD_BUFFER_SIZE;
+        break;
+      }
+
+      CopyMem (CommBuffer, (VOID *)CommunicateHeader, BufferSize);
No need for the (VOID *) cast

+      if (CommSize != NULL) {
Can CommSize be NULL?

+        *CommSize = BufferSize;
+      }
+
+      Status = EFI_SUCCESS;
+      break;
+
+    case ARM_SMC_MM_RET_INVALID_PARAMS:
+      Status = EFI_INVALID_PARAMETER;
+      break;
+
+    case ARM_SMC_MM_RET_DENIED:
+      Status = EFI_ACCESS_DENIED;
+      break;
+
+    case ARM_SMC_MM_RET_NO_MEMORY:
+      // Unexpected error since the CommSize was checked for zero length
+      // prior to issuing the SMC
+      Status = EFI_OUT_OF_RESOURCES;
+      ASSERT (0);
+      break;
+
+    default:
+      Status = EFI_ACCESS_DENIED;
+      ASSERT (0);
+      break;
+  }
+
+  return Status;
+}
diff --git a/ArmPkg/ArmPkg.dsc b/ArmPkg/ArmPkg.dsc
index 6b938ce8b671..4939b3d59b7f 100644
--- a/ArmPkg/ArmPkg.dsc
+++ b/ArmPkg/ArmPkg.dsc
@@ -162,6 +162,8 @@ [Components.common]
    ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf
    ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLibNull.inf

+  ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.inf
+
  [Components.AARCH64]
    ArmPkg/Drivers/ArmPsciMpServicesDxe/ArmPsciMpServicesDxe.inf
    ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf
diff --git a/ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.h 
b/ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.h
new file mode 100644
index 000000000000..a99baa2496a9
--- /dev/null
+++ b/ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.h
I don't think we need this header file at all.

@@ -0,0 +1,76 @@
+/** @file -- MmCommunicationPei.h
+  Provides an interface to send MM request in PEI
+
+  Copyright (c) Microsoft Corporation.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#ifndef MM_COMMUNICATION_PEI_H_
+#define MM_COMMUNICATION_PEI_H_
+
+#include <PiPei.h>
+
+#include <Library/BaseLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/ArmSmcLib.h>
+#include <Library/DebugLib.h>
+#include <Library/PcdLib.h>
+#include <Library/PeimEntryPoint.h>
+#include <Library/PeiServicesLib.h>
+#include <Library/HobLib.h>
+
+#include <Protocol/MmCommunication.h>
+
+#include <IndustryStandard/ArmStdSmc.h>
+
+#include <Ppi/MmCommunication.h>
+
+/**
+  Entry point of PEI MM Communication driver
+
+  @param  FileHandle   Handle of the file being invoked.
+                       Type EFI_PEI_FILE_HANDLE is defined in 
FfsFindNextFile().
+  @param  PeiServices  General purpose services available to every PEIM.
+
+  @retval EFI_SUCCESS  If the interface could be successfully installed
+  @retval Others       Returned from PeiServicesInstallPpi()
+**/
+EFI_STATUS
+EFIAPI
+MmCommunicationPeiInitialize (
+  IN       EFI_PEI_FILE_HANDLE  FileHandle,
+  IN CONST EFI_PEI_SERVICES     **PeiServices
+  );
+
+/**
+  MmCommunicationPeim
+  Communicates with a registered handler.
+  This function provides a service to send and receive messages from a 
registered UEFI service during PEI.
+
+  @param[in]      This            The EFI_PEI_MM_COMMUNICATION_PPI instance.
+  @param[in, out] CommBuffer      Pointer to the data buffer
+  @param[in, out] CommSize        The size of the data buffer being passed in. 
On exit, the
+                                  size of data being returned. Zero if the 
handler does not
+                                  wish to reply with any data.
+
+  @retval EFI_SUCCESS             The message was successfully posted.
+  @retval EFI_INVALID_PARAMETER   CommBuffer was NULL or *CommSize does not 
match
+                                  MessageLength + sizeof 
(EFI_MM_COMMUNICATE_HEADER).
+  @retval EFI_BAD_BUFFER_SIZE     The buffer is too large for the MM 
implementation.
+                                  If this error is returned, the MessageLength 
field
+                                  in the CommBuffer header or the integer 
pointed by
+                                  CommSize, are updated to reflect the maximum 
payload
+                                  size the implementation can accommodate.
+  @retval EFI_ACCESS_DENIED       The CommunicateBuffer parameter or CommSize 
parameter,
+                                  if not omitted, are in address range that 
cannot be
+                                  accessed by the MM environment.
+**/
+EFI_STATUS
+EFIAPI
+MmCommunicationPeim (
+  IN CONST EFI_PEI_MM_COMMUNICATION_PPI  *This,
+  IN OUT VOID                            *CommBuffer,
+  IN OUT UINTN                           *CommSize
+  );
+
+#endif /* MM_COMMUNICATION_PEI_H_ */
diff --git a/ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.inf 
b/ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.inf
new file mode 100644
index 000000000000..7a0adbd9bd2f
--- /dev/null
+++ b/ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.inf
@@ -0,0 +1,41 @@
+## @file -- MmCommunicationPei.inf
+#  PEI MM Communicate driver
+#
+#  Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.<BR>
+#  Copyright (c) Microsoft Corporation.
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+##
+
+[Defines]
+  INF_VERSION                    = 0x0001001B
+  BASE_NAME                      = MmCommunicationPei
+  FILE_GUID                      = 58FFB346-1B75-42C7-AD69-37C652423C1A
+  MODULE_TYPE                    = PEIM
+  VERSION_STRING                 = 1.0
+  ENTRY_POINT                    = MmCommunicationPeiInitialize
+
+[Sources]
+  MmCommunicationPei.c
+  MmCommunicationPei.h
+
+[Packages]
+  MdePkg/MdePkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  ArmPkg/ArmPkg.dec
+
+[LibraryClasses]
+  DebugLib
+  ArmSmcLib
+  PeimEntryPoint
+  PeiServicesLib
+  HobLib
+
+[Pcd]
+  gArmTokenSpaceGuid.PcdMmBufferBase
+  gArmTokenSpaceGuid.PcdMmBufferSize
+
+[Ppis]
+  gEfiPeiMmCommunicationPpiGuid     ## PRODUCES
+
+[Depex]
+  TRUE
--
2.41.0.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106447): https://edk2.groups.io/g/devel/message/106447
Mute This Topic: https://groups.io/mt/99796183/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-


Reply via email to