From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f180.google.com (mail-pl1-f180.google.com [209.85.214.180]) by mx.groups.io with SMTP id smtpd.web10.4704.1687909984492493475 for ; Tue, 27 Jun 2023 16:53:04 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20221208 header.b=F+Eqa0iw; spf=pass (domain: gmail.com, ip: 209.85.214.180, mailfrom: kuqin12@gmail.com) Received: by mail-pl1-f180.google.com with SMTP id d9443c01a7336-1b801e6ce85so2832805ad.1 for ; Tue, 27 Jun 2023 16:53:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1687909984; x=1690501984; h=content-transfer-encoding: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=W76mu7h57TcQ8zBNPTFOFsAIi1m7EB9/vbR3PlsS2A8=; b=F+Eqa0iwQaKwdK+aREqKExYIwwzPgZoKiDU/M2/vx2YdrKV0uJyPrRWGf1nSHH9FnW SxmZyD0r/j17v4EaKRsf1QJyyOssY/HSJDLCWmCRwmlkoXuqGyn5xAsZenX2DNrSeww5 SdkwZyICgUAX5WQK4vC5rs/VcHtRryZtvuay8F0XraUC434vU7AhUrFMpomqB9ea7iHO YM0UW2pf+TeIBqSLNeQ9QzEt51uIRhYM/fnHh/24eVgFNWp7GHSdku37nwCzyf9eDOAA yQ4XEfbZPJkvg8WB8xPI/o6llHniqUNdPYCKFTTkrZwX/+sRaLJ7/NLA/w1H1kdSEYxJ 1CjQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1687909984; x=1690501984; h=content-transfer-encoding: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=W76mu7h57TcQ8zBNPTFOFsAIi1m7EB9/vbR3PlsS2A8=; b=WRpFqlXokaC4ixPINdq5fD04k+8ceUfJEX4wzdvhpowHkBMvkpZwY1XYy2NJMpeVux +WqIQVEjhwHKNdSCwieo5VjasOYPaF8geF/GKr3ioxs5zriehRSrf8Qiz2kbFCumztDt Xc+smkuAolXoJybeGtrmMSz6NTGr3xDcHKty9gdxd0VAXDMeSZLVSld1cJxjLd5IQ5zx YuNCT7gF/Q3DKX26le1IWRvDfOu6oTd5K1X8RpVSHChMdmo6rIVCV6d98uItKGnPfhLW JYfmu6ExeP59oefyPre4JUENEwQ04G/PQsPzEaNvBhksxl0KbZWs3GTr6bNHnmVpKq0D LRtA== X-Gm-Message-State: AC+VfDyreCpj+VD7PZ8AeRU/MOr5wFtlFAZld+wcAZDyfOXPlG6iCXB3 AjXVMgCx617BWoUMzdEknOI= X-Google-Smtp-Source: ACHHUZ6XERGZtR4ucoMoJ2+7pYqFHTr1erVr7tl25wsGzLaMfRLptxEhRrT6+hSzfGBsbQG3Q+LoAQ== X-Received: by 2002:a17:902:f7cb:b0:1b7:f99f:63ca with SMTP id h11-20020a170902f7cb00b001b7f99f63camr8863351plw.34.1687909983796; Tue, 27 Jun 2023 16:53:03 -0700 (PDT) Return-Path: Received: from ?IPV6:2001:4898:d8:33:e0ed:268c:453f:a14b? ([2001:4898:80e8:f:6101:268c:453f:a14b]) by smtp.gmail.com with ESMTPSA id iy3-20020a170903130300b001ae3b51269dsm4122799plb.262.2023.06.27.16.53.03 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 27 Jun 2023 16:53:03 -0700 (PDT) Message-ID: Date: Tue, 27 Jun 2023 16:53:02 -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 v2 1/2] ArmPkg: MmCommunicationPei: Introduce MM communicate in PEI To: Ard Biesheuvel Cc: devel@edk2.groups.io, Leif Lindholm , Ard Biesheuvel , Sami Mujawar , Ronny Hansen , Shriram Masanamuthu Chinnathurai , Preshit Harlikar References: <20230626195953.1807-1-kuqin12@gmail.com> <20230626195953.1807-2-kuqin12@gmail.com> From: "Kun Qin" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Hi Ard, Thanks for the feedback. I updated the patch as you suggested and sent out a v3 here: https://edk2.groups.io/g/devel/message/106443. The only call-out was the "CONST" qualifier for "mPeiMmCommunication" was cast away when initializing "mPeiMmCommunicationPpi". This was because the "Ppi" field inside "EFI_PEI_PPI_DESCRIPTOR" was defined as "VOID*" and not doing so will cause the build failure. Please let me know if you have further feedback. Thanks in advance. Regards, Kun On 6/27/2023 7:36 AM, Ard Biesheuvel wrote: > On Mon, 26 Jun 2023 at 22:00, 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 >> --- >> >> 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.
>> + 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 >> + >> +#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..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.
>> +# 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 >>