From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from EUR01-DB5-obe.outbound.protection.outlook.com (EUR01-DB5-obe.outbound.protection.outlook.com [40.107.15.71]) by mx.groups.io with SMTP id smtpd.web11.20490.1687461496484793843 for ; Thu, 22 Jun 2023 12:18:17 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@armh.onmicrosoft.com header.s=selector2-armh-onmicrosoft-com header.b=FjlZDlGC; spf=pass (domain: arm.com, ip: 40.107.15.71, mailfrom: sami.mujawar@arm.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com; s=selector2-armh-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=4KA8nW6FKI/uY8SMFLziagNpSkuSipwJA2KrJPrOi7w=; b=FjlZDlGCj+RHgjg9fIzQP3Rlms69YEJkdNY6wvBRfE1/1U7xf3ksrNV9tlK+XFOTJV9K/CFhWmt7La5dWcC3dn6toKKRF0qfJXpaf9n6CZq148Wt2+Rg8jdlCVVAqp2tq5SEzqitryJk+ehGJZjcvM3BNYynnMc4Es0R9bTlBJc= Received: from DB8PR04CA0017.eurprd04.prod.outlook.com (2603:10a6:10:110::27) by DB4PR08MB8128.eurprd08.prod.outlook.com (2603:10a6:10:381::14) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6521.23; Thu, 22 Jun 2023 19:18:12 +0000 Received: from DBAEUR03FT015.eop-EUR03.prod.protection.outlook.com (2603:10a6:10:110:cafe::d4) by DB8PR04CA0017.outlook.office365.com (2603:10a6:10:110::27) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6521.24 via Frontend Transport; Thu, 22 Jun 2023 19:18:12 +0000 X-MS-Exchange-Authentication-Results: spf=pass (sender IP is 63.35.35.123) smtp.mailfrom=arm.com; dkim=pass (signature was verified) header.d=armh.onmicrosoft.com;dmarc=pass action=none header.from=arm.com; Received-SPF: Pass (protection.outlook.com: domain of arm.com designates 63.35.35.123 as permitted sender) receiver=protection.outlook.com; client-ip=63.35.35.123; helo=64aa7808-outbound-1.mta.getcheckrecipient.com; pr=C Received: from 64aa7808-outbound-1.mta.getcheckrecipient.com (63.35.35.123) by DBAEUR03FT015.mail.protection.outlook.com (100.127.142.112) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6521.26 via Frontend Transport; Thu, 22 Jun 2023 19:18:12 +0000 Received: ("Tessian outbound c08fa2e31830:v142"); Thu, 22 Jun 2023 19:18:11 +0000 X-CheckRecipientChecked: true X-CR-MTA-CID: 5ddf1ed27ecb09ee X-CR-MTA-TID: 64aa7808 Received: from 585d037b6c00.1 by 64aa7808-outbound-1.mta.getcheckrecipient.com id C77D92AC-D166-4D9F-B3E2-B6F1F963A0D2.1; Thu, 22 Jun 2023 19:18:05 +0000 Received: from EUR02-DB5-obe.outbound.protection.outlook.com by 64aa7808-outbound-1.mta.getcheckrecipient.com with ESMTPS id 585d037b6c00.1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384); Thu, 22 Jun 2023 19:18:05 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=F4ziObLgX5ar/u93FfNjC1cN1lenpG1hrGIN/B1r6y46T7BlE0cQhSLGyZFlvK8X1sTmOqAuTykr4tCNadRg2SBMZEsAXM7q16+R1vsVJ04VNN/EjumqYM2HGotjBjVcotavJxYKyh+PxLjwdQqrjXoLMBFt+ZwDxSLc63hEX+zbE6+pyGI8ESzWZtL9jS8fJsFVSC1/wsOAbHQbRFjB/VDpxBJRHSgLPrTPePxWjV3Y2Hg2nJpvetaF+JRb94e1Csl07qk5A6vgGOpBWi47JcRgG2P0sLTzptzQVBIZZYzJdaYoilF17K84aId7CRUH3xMKXbwNHUDPBK1VGyGBeA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=4KA8nW6FKI/uY8SMFLziagNpSkuSipwJA2KrJPrOi7w=; b=mJ70ZtpNVsQVjAuq3NAh/+03B9t+V+Y4bqo3+dDVpmPkzPIU3l+BRrOSkHz52Z/wIAlSWBd+kaASarDtANa77IYMV8nv/dCXcl4mfbEeW0FZgDb3on7ATBLJLTi+hwgOafAFaUPPa90J7INP/6VjOK2Tp+OrwfMAOFcpmkgfZpr/dZr7dCHwCXBPv8zbg48JoWok7k8fwCA4dBChb95ID4cHr/cUx9o2475mvHjtG2yDRyA1z7TJJrfzjuZyGCrmz/KOg7y+01Xjxk/1SBrn5+6jB3wAMbjq0Y6spyvfvOGOgaZed0PUmxADuNr/dabR9wX6WX8UmaPsgV/+hXs/IQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com; s=selector2-armh-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=4KA8nW6FKI/uY8SMFLziagNpSkuSipwJA2KrJPrOi7w=; b=FjlZDlGCj+RHgjg9fIzQP3Rlms69YEJkdNY6wvBRfE1/1U7xf3ksrNV9tlK+XFOTJV9K/CFhWmt7La5dWcC3dn6toKKRF0qfJXpaf9n6CZq148Wt2+Rg8jdlCVVAqp2tq5SEzqitryJk+ehGJZjcvM3BNYynnMc4Es0R9bTlBJc= Authentication-Results-Original: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com; Received: from AS8PR08MB6806.eurprd08.prod.outlook.com (2603:10a6:20b:39b::12) by DU0PR08MB7414.eurprd08.prod.outlook.com (2603:10a6:10:352::8) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6521.23; Thu, 22 Jun 2023 19:18:02 +0000 Received: from AS8PR08MB6806.eurprd08.prod.outlook.com ([fe80::8ef4:aa57:6248:7850]) by AS8PR08MB6806.eurprd08.prod.outlook.com ([fe80::8ef4:aa57:6248:7850%4]) with mapi id 15.20.6521.023; Thu, 22 Jun 2023 19:18:02 +0000 Message-ID: Date: Thu, 22 Jun 2023 20:17:59 +0100 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: Kun Qin , 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: "Sami Mujawar" In-Reply-To: <20230608204434.2325-2-kuqin12@gmail.com> X-ClientProxiedBy: LO4P123CA0634.GBRP123.PROD.OUTLOOK.COM (2603:10a6:600:294::9) To AS8PR08MB6806.eurprd08.prod.outlook.com (2603:10a6:20b:39b::12) MIME-Version: 1.0 X-MS-TrafficTypeDiagnostic: AS8PR08MB6806:EE_|DU0PR08MB7414:EE_|DBAEUR03FT015:EE_|DB4PR08MB8128:EE_ X-MS-Office365-Filtering-Correlation-Id: f511032e-bd68-4218-ac70-08db73556bc7 x-checkrecipientrouted: true NoDisclaimer: true X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam-Untrusted: BCL:0; X-Microsoft-Antispam-Message-Info-Original: uBXhvoMjaVHj5NsKOVNpMa262ZQzPUnyVgmgCKZwT7CyhvixTQiPS6MqEorGYGR5FCgv3BLkcT7VARm9ZG7++bHRB1wwPyll7hVHPSVBiESQM4j3e85fwwpt6Pta/cEf3Xe7rbmr0yrjip0UltLr5bhnsmiym5qDpSIjWf0NTCJYKtNW1Gecp3oc84vLaa9jlfVIB7sx81HWOuXP2uY6KzVaTlcjSimeQXfdDMMGR7U+okv7u7pQKIYsq6Srl+oZre0JVjHurrjrwXrd+dVo3BLNTJ75c9Bb0Poulom+t+o75zI+3Yr2RaVzNvOCAM/ft6P6IDs1agPG7MJguJeNeKJLECuXkfcVOmLiJlGhdSHR/OeDaQe4DOp44SyUtUTwSF+50k+r5xswCAyPYlQhoRHgpKwSOPuqvYia0PgiRBQt77blehFhSetMN0j7JVHB6DZQyFgx0TWk6au+mOcr/KLVaI57vEFIfqz6dsp93DBhrAVhWA06VRCIU6/eqwxRNbNp7dz5GGEBAvHtDWLdLfZEqPlABJNUXkYu0Ur6e56MtuOjEvrfCGnyw9MiT+AizwqiSpl7nODqZi5UrFCCdJmND0DJmbzYWTjrX6OEHiadhXn4/DRyiXJfkBmi+pLPHTaxn4DGcqTmpK/4K7cXgTqGcwWjktXZob5/dLLu8OQ= X-Forefront-Antispam-Report-Untrusted: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:AS8PR08MB6806.eurprd08.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230028)(6029001)(4636009)(39860400002)(396003)(366004)(136003)(346002)(376002)(451199021)(53546011)(38100700002)(26005)(83380400001)(6506007)(186003)(6486002)(2616005)(966005)(44832011)(2906002)(8676002)(41300700001)(8936002)(5660300002)(66556008)(6666004)(36756003)(45080400002)(478600001)(166002)(33964004)(66476007)(66946007)(4326008)(54906003)(6512007)(31696002)(86362001)(19627235002)(316002)(31686004)(30864003)(43740500002)(45980500001);DIR:OUT;SFP:1101; X-MS-Exchange-Transport-CrossTenantHeadersStamped: DU0PR08MB7414 Original-Authentication-Results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com; Return-Path: Sami.Mujawar@arm.com X-EOPAttributedMessage: 0 X-MS-Exchange-Transport-CrossTenantHeadersStripped: DBAEUR03FT015.eop-EUR03.prod.protection.outlook.com X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id-Prvs: 3912e303-c554-48a5-3dc2-08db73556605 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: ciNcaTzax1Myz1/RAKbEsauAJSbAZLQQHxhH8e1P231voiYRNTtQ63meA4uNwK7RKXH7wMYEC/Wo0/k25k1RnRswvpgxVIToUrkJW2z90N7uSH34y6yCt/lthZbBoFkzg15EOKEPBOv3QAWutvSV+WxqiYmzIrh5ajLxdFSsayWB1cL2fof0qqChcQ8h1gtHPUtDBCpQf76ZXHESJ6vj86RSVtVy7N+ywJSqUQjGA0TkEmvArMtAQk1lgmEL40pSF57W6RklMyHf2KCX7+c4ug+lyNEuFu5SXG4AyR2r9M92EUgxqnB5qAPeXU+AvssZs8AfHC8enCDob0Tm6ZT7T8U5JdoTfVLjdGvF8LV50mZ7GkeuBSo7NzukM0leLg8b2ZY1XYvCSfjHdRSD5g6akl1Cifwa8sDHGR2vrRd0c0evouMxp++EFyed0qpDJ35tCxcHXIOexi47siD8jYn0KKhk35wJ4cFDgwnLji3ax51UmIhQBwZ0OJc8xsmvWILCl9LgrMK00VKsBDsTugSZXYdzUnyvs8fUIVIfTbXyfE0P5b8TQQjthN8HO46I7caq0lJv9Clrjq1iAXPaQRL+hRCce53E1F7db+4Yqu9J3E+tmfE4gqyHCEQM0b4KOJ/3wbmNgVj+TlIFfI0rhBCzKrdVIXulWjpDOV+W/INS81wkii1dxtvo3tRrK+FK/iVMLJeqTVEoJWg1SwUF+BHGFJTWeMvT82IOvEf1Er0B2SHBdC2SFezP8Jhl2CgK5LTs7S2WTRUk1R3ChGwRK927gYrTGLd+RG3kibDbjuOnq+E= X-Forefront-Antispam-Report: CIP:63.35.35.123;CTRY:IE;LANG:en;SCL:1;SRV:;IPV:CAL;SFV:NSPM;H:64aa7808-outbound-1.mta.getcheckrecipient.com;PTR:ec2-63-35-35-123.eu-west-1.compute.amazonaws.com;CAT:NONE;SFS:(13230028)(6029001)(4636009)(376002)(396003)(346002)(39860400002)(136003)(451199021)(40470700004)(36840700001)(46966006)(8936002)(8676002)(2906002)(5660300002)(40480700001)(54906003)(19627235002)(6666004)(31686004)(44832011)(316002)(41300700001)(30864003)(4326008)(70586007)(40460700003)(70206006)(336012)(6486002)(45080400002)(36756003)(33964004)(966005)(478600001)(31696002)(166002)(82310400005)(86362001)(186003)(356005)(2616005)(81166007)(83380400001)(47076005)(6512007)(36860700001)(26005)(82740400003)(6506007)(53546011)(43740500002);DIR:OUT;SFP:1101; X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Jun 2023 19:18:12.0563 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: f511032e-bd68-4218-ac70-08db73556bc7 X-MS-Exchange-CrossTenant-Id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=f34e5979-57d9-4aaa-ad4d-b122a662184d;Ip=[63.35.35.123];Helo=[64aa7808-outbound-1.mta.getcheckrecipient.com] X-MS-Exchange-CrossTenant-AuthSource: DBAEUR03FT015.eop-EUR03.prod.protection.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Anonymous X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB4PR08MB8128 Content-Type: multipart/alternative; boundary="------------0VkAUiertriq350oga02JHOZ" --------------0VkAUiertriq350oga02JHOZ Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 > --------------0VkAUiertriq350oga02JHOZ Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

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
--------------0VkAUiertriq350oga02JHOZ--