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

["Sami Mujawar" <sami.mujawar@arm.com>]

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

Hi Kun,

Thank you for this patch.

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

On 08/06/2023 09:44 pm, Kun Qin 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>
> ---
>   ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.c   | 178 ++++++++++++++++++++
>   ArmPkg/ArmPkg.dsc                                        |   2 +
>   ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.h   |  76 +++++++++
>   ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.inf |  41 +++++
>   4 files changed, 297 insertions(+)
>
> diff --git a/ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.c b/ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.c
> new file mode 100644
> index 000000000000..0f1f763a347d
> --- /dev/null
> +++ b/ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.c
> @@ -0,0 +1,178 @@
> +/** @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
>
> +//
>
> +EFI_PEI_MM_COMMUNICATION_PPI  mPeiMmCommunication = {
>
> +  MmCommunicationPeim
>
> +};
>
> +
>
> +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 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
>
> +  )
>
> +{
>
> +  EFI_MM_COMMUNICATE_HEADER  *CommunicateHeader;
>
> +  ARM_SMC_ARGS               CommunicateSmcArgs;
>
> +  EFI_STATUS                 Status;
>
> +  UINTN                      BufferSize;
>
> +
>
> +  Status     = EFI_ACCESS_DENIED;
>
> +  BufferSize = 0;
[SAMI] Minor optimisation: The above initialisations are probably not 
required.
>
> +
>
> +  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);
>
> +
>
> +  //
>
> +  // Check parameters
>
> +  //
>
> +  if (CommBuffer == NULL) {
>
> +    return EFI_INVALID_PARAMETER;
>
> +  }
[SAMI] Should there be a check for CommSize as well? Otherwise the code 
will crash a few lines below when doing CopyMem().
> +
>
> +  // 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 != NULL) && \
>
> +      ((*CommSize == 0) || (*CommSize > (UINTN)PcdGet64 (PcdMmBufferSize))))
>
> +  {
>
> +    *CommSize = (UINTN)PcdGet64 (PcdMmBufferSize);
>
> +    return EFI_BAD_BUFFER_SIZE;
>
> +  }
>
> +
>
> +  CommunicateHeader = (EFI_MM_COMMUNICATE_HEADER *)(UINTN)(PcdGet64 (PcdMmBufferBase));
>
> +
>
> +  CopyMem ((VOID *)CommunicateHeader, CommBuffer, *CommSize);
[SAMI] If CommSize is NULL, the above the above line will result in a 
crash, right?
>
> +
>
> +  // 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 = CommunicateHeader->MessageLength +
>
> +               sizeof (CommunicateHeader->HeaderGuid) +
>
> +               sizeof (CommunicateHeader->MessageLength);
>
> +
>
> +  //
>
> +  // If CommSize is supplied it must match MessageLength + sizeof (EFI_MM_COMMUNICATE_HEADER);
>
> +  //
>
> +  if ((CommSize != NULL) && (*CommSize != BufferSize)) {
>
> +    return EFI_INVALID_PARAMETER;
>
> +  }
[SAMI] It may be better to do this check earlier in the code by casting 
CommBuffer to EFI_MM_COMMUNICATE_HEADER * and calculating the 
BufferSize. That way the CopyMem() above can be avoided if the above 
test fails.
>
> +
>
> +  // 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);
>
> +      CopyMem (CommBuffer, (VOID *)CommunicateHeader, BufferSize);

[SAMI] Can there be a case where the returned MessageLength results in 
the CommBuffer size being smaller, i.e. BufferSize returned > *CommSize ?

I expect  ARM_SMC_MM_RET_NO_MEMORY to have been returned in the first 
place, but it may be worth adding a check to avoid potential issues. 
What do you think?

>
> +      if (CommSize != 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);
>
> +  }
>
> +
>
> +  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
> @@ -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..f4e359dafd75
> --- /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                    = 0x00010005
[SAMI] The version should be |0x0001001B. See 
https://github.com/tianocore-docs/edk2-InfSpecification/blob/master/3_edk_ii_inf_file_format/34_%5Bdefines%5D_section.md
|
>
> +  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
>

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