Re: [PATCH v2 1/2] ArmPkg: MmCommunicationPei: Introduce MM communicate in PEI

["Kun Qin" <kuqin12@gmail.com>]

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 <kuqin12@gmail.com> wrote:
>> From: Kun Qin <kuqin@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_llindhol@quicinc.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Sami Mujawar <sami.mujawar@arm.com>
>>
>> Co-authored-by: Ronny Hansen <hansen.ronny@microsoft.com>
>> Co-authored-by: Shriram Masanamuthu Chinnathurai <shriramma@microsoft.com>
>> Co-authored-by: Preshit Harlikar <pharlikar@microsoft.com>
>> Signed-off-by: Kun Qin <kuqin@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
>>