From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web10.15207.1687876576042796135 for ; Tue, 27 Jun 2023 07:36:16 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=aX8bZr68; spf=pass (domain: kernel.org, ip: 139.178.84.217, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 73EF5611BF for ; Tue, 27 Jun 2023 14:36:15 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id C0367C433CD for ; Tue, 27 Jun 2023 14:36:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1687876574; bh=4IRHPT2q21ZQy/roHYvVuXX2Xxd/xZUFDmgZ8q5xK9A=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=aX8bZr68qgOcMOsGwLgcrlokRAfqG4vl/ligUKrP7yJC3a7gldupSPHDHLonlhsIl O5Hqa8P/HiT33sHmt3dooItVMivCiV1qqdW3fyBiRR85aWkyBl7ic4c3FtHoFE0egV 4UOMjsAD2ghACKITfXP8hmSwX6knCS9v5G9j/nUj49lu+A1ErFc3psWI704yGuIa8m fd88zXRxcYLTSlhU19eWRVnIRqErmV3Y7ZryEiUmf7GI/thvySJaGMqaofj0IlAV+U 9iLeGvWzc48bl6SjX66D3OP3wbKfRabG1Y9PKuIqlyTmZVaKQ0196SrEFA+ys7mxjh KndKj0KXqX4aw== Received: by mail-lf1-f45.google.com with SMTP id 2adb3069b0e04-4f8735ac3e3so6534522e87.2 for ; Tue, 27 Jun 2023 07:36:14 -0700 (PDT) X-Gm-Message-State: AC+VfDzbXOxddM7LxxuvadKJmP7V8n49Cd/onRJkpTGtByB3CT7M1uoR WPJb7R8yItwMu1iioNr7ODvJYYSTpYyFG0BhSps= X-Google-Smtp-Source: ACHHUZ44emmJk4C9J5GvgjXLMXyEcwLksIdrinv3mma0OK6yw0/Ued8pUhNeTzBAqNCfGoRj2xFT/CkFEUzLjaoVY30= X-Received: by 2002:a05:6512:3587:b0:4fb:8948:2b2b with SMTP id m7-20020a056512358700b004fb89482b2bmr789304lfr.48.1687876572746; Tue, 27 Jun 2023 07:36:12 -0700 (PDT) MIME-Version: 1.0 References: <20230626195953.1807-1-kuqin12@gmail.com> <20230626195953.1807-2-kuqin12@gmail.com> In-Reply-To: <20230626195953.1807-2-kuqin12@gmail.com> From: "Ard Biesheuvel" Date: Tue, 27 Jun 2023 16:36:01 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v2 1/2] ArmPkg: MmCommunicationPei: Introduce MM communicate in PEI To: Kun Qin Cc: devel@edk2.groups.io, Leif Lindholm , Ard Biesheuvel , Sami Mujawar , Ronny Hansen , Shriram Masanamuthu Chinnathurai , Preshit Harlikar Content-Type: text/plain; charset="UTF-8" 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 >