<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> </head> <body> 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? <a class="moz-txt-link-freetext" href="https://edk2.groups.io/g/devel/message/106372">https://edk2.groups.io/g/devel/message/106372</a> Thanks, Kun <div class="moz-cite-prefix">On 6/22/2023 12:17 PM, Sami Mujawar wrote: </div> <blockquote type="cite" cite="mid:f2f64742-0d89-d391-7927-1b46831e7bbd@arm.com"> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> Hi Kun, Thank you for this patch. Please find my response inline marked [SAMI]. Regards, Sami Mujawar <div class="moz-cite-prefix">On 08/06/2023 09:44 pm, Kun Qin wrote: </div> <blockquote type="cite" cite="mid:20230608204434.2325-2-kuqin12@gmail.com"> <pre class="moz-quote-pre" wrap="">From: Kun Qin <a class="moz-txt-link-rfc2396E" href="mailto:kuqin@microsoft.com" moz-do-not-send="true"><kuqin@microsoft.com></a> REF: <a class="moz-txt-link-freetext" href="https://bugzilla.tianocore.org/show_bug.cgi?id=4464" moz-do-not-send="true">https://bugzilla.tianocore.org/show_bug.cgi?id=4464</a> 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 <a class="moz-txt-link-rfc2396E" href="mailto:quic_llindhol@quicinc.com" moz-do-not-send="true"><quic_llindhol@quicinc.com></a> Cc: Ard Biesheuvel <a class="moz-txt-link-rfc2396E" href="mailto:ardb+tianocore@kernel.org" moz-do-not-send="true"><ardb+tianocore@kernel.org></a> Cc: Sami Mujawar <a class="moz-txt-link-rfc2396E" href="mailto:sami.mujawar@arm.com" moz-do-not-send="true"><sami.mujawar@arm.com></a> Co-authored-by: Ronny Hansen <a class="moz-txt-link-rfc2396E" href="mailto:hansen.ronny@microsoft.com" moz-do-not-send="true"><hansen.ronny@microsoft.com></a> Co-authored-by: Shriram Masanamuthu Chinnathurai <a class="moz-txt-link-rfc2396E" href="mailto:shriramma@microsoft.com" moz-do-not-send="true"><shriramma@microsoft.com></a> Co-authored-by: Preshit Harlikar <a class="moz-txt-link-rfc2396E" href="mailto:pharlikar@microsoft.com" moz-do-not-send="true"><pharlikar@microsoft.com></a> Signed-off-by: Kun Qin <a class="moz-txt-link-rfc2396E" href="mailto:kuqin@microsoft.com" moz-do-not-send="true"><kuqin@microsoft.com></a> --- 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;</pre> </blockquote> [SAMI] Minor optimisation: The above initialisations are probably not required. <blockquote type="cite" cite="mid:20230608204434.2325-2-kuqin12@gmail.com"> <pre class="moz-quote-pre" wrap="">+ + 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; + } </pre> </blockquote> [SAMI] Should there be a check for CommSize as well? Otherwise the code will crash a few lines below when doing CopyMem(). <blockquote type="cite" cite="mid:20230608204434.2325-2-kuqin12@gmail.com"> <pre class="moz-quote-pre" wrap="">+ + // 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);</pre> </blockquote> [SAMI] If CommSize is NULL, the above the above line will result in a crash, right? <blockquote type="cite" cite="mid:20230608204434.2325-2-kuqin12@gmail.com"> <pre class="moz-quote-pre" wrap="">+ + // 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; + }</pre> </blockquote> [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. <blockquote type="cite" cite="mid:20230608204434.2325-2-kuqin12@gmail.com"> <pre class="moz-quote-pre" wrap="">+ + // 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);</pre> </blockquote> [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? <blockquote type="cite" cite="mid:20230608204434.2325-2-kuqin12@gmail.com"> <pre class="moz-quote-pre" wrap="">+ 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. +# Copyright (c) Microsoft Corporation. +# SPDX-License-Identifier: BSD-2-Clause-Patent +## + +[Defines] + INF_VERSION = 0x00010005</pre> </blockquote> [SAMI] The version should be <code>0x0001001B. See <a class="moz-txt-link-freetext" href="https://github.com/tianocore-docs/edk2-InfSpecification/blob/master/3_edk_ii_inf_file_format/34_%5Bdefines%5D_section.md" moz-do-not-send="true">https://github.com/tianocore-docs/edk2-InfSpecification/blob/master/3_edk_ii_inf_file_format/34_%5Bdefines%5D_section.md</a> </code> <blockquote type="cite" cite="mid:20230608204434.2325-2-kuqin12@gmail.com"> <pre class="moz-quote-pre" wrap="">+ 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 </pre> </blockquote> </blockquote> </body> </html> <div width="1" style="color:white;clear:both">_._,_._,_</div> <hr> Groups.io Links: You receive all messages sent to this group. <a target="_blank" href="https://edk2.groups.io/g/devel/message/106375">View/Reply Online (#106375)</a> | | <a target="_blank" href="https://groups.io/mt/99415825/1813853">Mute This Topic</a> | <a href="https://edk2.groups.io/g/devel/post">New Topic</a> <a href="https://edk2.groups.io/g/devel/editsub/1813853">Your Subscription</a> | <a href="mailto:devel+owner@edk2.groups.io">Contact Group Owner</a> | <a href="https://edk2.groups.io/g/devel/unsub">Unsubscribe</a> [edk2-devel-archive@redhat.com] <div width="1" style="color:white;clear:both">_._,_._,_</div>