Hi Sami, Thank you for your feedback. I have update the code as you suggested. Could you please re-review when you have a chance? https://edk2.groups.io/g/devel/message/106372 Thanks, Kun On 6/22/2023 12:17 PM, Sami Mujawar wrote: > > 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 >> >> 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 >> Cc: Ard Biesheuvel >> Cc: Sami Mujawar >> >> Co-authored-by: Ronny Hansen >> Co-authored-by: Shriram Masanamuthu Chinnathurai >> Co-authored-by: Preshit Harlikar >> Signed-off-by: Kun Qin >> --- >> 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.
>> >> + 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 >> >> + >> >> +#include >> >> +#include >> >> +#include >> >> +#include >> >> +#include >> >> +#include >> >> +#include >> >> +#include >> >> + >> >> +#include >> >> + >> >> +#include >> >> + >> >> +#include >> >> + >> >> +/** >> >> + 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.
>> >> +# 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 >>