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

["Ard Biesheuvel" <ardb@kernel.org>]

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
>