From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f176.google.com (mail-pl1-f176.google.com [209.85.214.176]) by mx.groups.io with SMTP id smtpd.web10.2135.1687810048630657353 for ; Mon, 26 Jun 2023 13:07:28 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20221208 header.b=NukZEX9V; spf=pass (domain: gmail.com, ip: 209.85.214.176, mailfrom: kuqin12@gmail.com) Received: by mail-pl1-f176.google.com with SMTP id d9443c01a7336-1b8033987baso9540235ad.0 for ; Mon, 26 Jun 2023 13:07:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1687810048; x=1690402048; h=in-reply-to:from:references:cc:to:content-language:subject :user-agent:mime-version:date:message-id:from:to:cc:subject:date :message-id:reply-to; bh=Z+rVGr+/fs74DclHJw7/u3UC1Xd15xPSbanA/2YkLws=; b=NukZEX9VztY8KLSyAdyAfw4oCbUgYupVyJSATHisSUeJNo1rkDwTeTOUCngJYSBFAn /4WZr9J7gNEgMPjj5v6vQgMIyJUxB1sE7mL40nX2HYSJM9KHwIzmkOuZugRPO9zX2uLN 2qBr9KoVWDPQrkzTuUq8koFG0lu6X1Jrva8q8N4Oen7Wu9H9MSf1IBf1WNJqkUvyWAMf XfqPwlhH9Ti/q6OefJXy/x6kY3GdLBrr7Z+50wLdkujVz8oRLPHOo9EOk5km8j5Y5vys QJ+tAYwlnomtfieeSa/KCsXp/V+dOgDZWOqrLmWC/7oCpFMjpEwY74l1gTFQYYFdQrtk eSwA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1687810048; x=1690402048; h=in-reply-to:from:references:cc:to:content-language:subject :user-agent:mime-version:date:message-id:x-gm-message-state:from:to :cc:subject:date:message-id:reply-to; bh=Z+rVGr+/fs74DclHJw7/u3UC1Xd15xPSbanA/2YkLws=; b=DXgvCvGhRZa27F1tCMzSL38EZo4dXgHUhYTP8usLCYrKVLgLmShbzKwnyZO2SCR2uf pbIDAuBT9rRQDcejvEX+rztk70QvdKyHtFdgcP/0RTGVdMC1nKPZCEvMNaZvA/Ji9da9 GudlCF89yVSCCEkRpHRW+38KkS8i2qe3JOtoRA9DKXEG2cmDxa8lVfNy9cwAhyveUxM7 FPk8zL9KH48HN3Y8sjk7gaOClS2i8Rx0FvnvebuuQtCJnW2yx6eeWl2u+Z5tki15CkT4 9R7NdhEvw+LwkJIDXyH3iIjJLO2JY6HvY7uUAY5qnNCv+C1BDh/5ysdQpAopOkqsft4m IHFA== X-Gm-Message-State: AC+VfDzrOUssq7xtpHtn8O8v9DFKu1/tbNQVmutmbngJYVTepPE9eOzV 22qtSpR5nK1XEt0plfRifUY= X-Google-Smtp-Source: ACHHUZ6tQYCbhyNqp94jsLrussDznCIV6v47Sjf1ti2YwuDEMIJePXfAu2dHo7x+uhelBbbbFoLcAg== X-Received: by 2002:a17:903:22c8:b0:1b6:99a1:177d with SMTP id y8-20020a17090322c800b001b699a1177dmr9737798plg.31.1687810047862; Mon, 26 Jun 2023 13:07:27 -0700 (PDT) Return-Path: Received: from ?IPV6:2001:4898:d8:33:bdb9:b5a5:d301:15a6? ([2001:4898:80e8:1:3ddb:b5a5:d301:15a6]) by smtp.gmail.com with ESMTPSA id bd8-20020a170902830800b001b7f9963febsm2954131plb.175.2023.06.26.13.07.26 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 26 Jun 2023 13:07:27 -0700 (PDT) Message-ID: <43a23b18-cffd-11fd-952e-d1de86ad0e9c@gmail.com> Date: Mon, 26 Jun 2023 13:07:26 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.12.0 Subject: Re: [PATCH v1 1/2] ArmPkg: MmCommunicationPei: Introduce MM communicate in PEI To: Sami Mujawar , devel@edk2.groups.io Cc: Leif Lindholm , Ard Biesheuvel , Ronny Hansen , Shriram Masanamuthu Chinnathurai , Preshit Harlikar , "nd@arm.com" References: <20230608204434.2325-1-kuqin12@gmail.com> <20230608204434.2325-2-kuqin12@gmail.com> From: "Kun Qin" In-Reply-To: Content-Type: multipart/alternative; boundary="------------CeZl2wYKZqUPJiq4AWwvdUmd" Content-Language: en-US --------------CeZl2wYKZqUPJiq4AWwvdUmd Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 >> --------------CeZl2wYKZqUPJiq4AWwvdUmd Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

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 <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
--------------CeZl2wYKZqUPJiq4AWwvdUmd--