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 (#106429): https://edk2.groups.io/g/devel/message/106429
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]
-=-=-=-=-=-=-=-=-=-=-=-
