From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::142; helo=mail-it1-x142.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it1-x142.google.com (mail-it1-x142.google.com [IPv6:2607:f8b0:4864:20::142]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 4E8092119378C for ; Fri, 23 Nov 2018 10:20:51 -0800 (PST) Received: by mail-it1-x142.google.com with SMTP id m15so19072023itl.4 for ; Fri, 23 Nov 2018 10:20:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=rO72TwyytF2etcO0tDjvd1m3PHusSZ43+zr9ZBP6xp0=; b=fEtDyIJLx3cDlb9xv2HlMqZ2bTaYEj0wy+UFiz74UUY3+famN3/NvSUf5UGEUAgsPj Ngk0ZYXCwdxrhK1v25fltum2BNFzMDbBe2FUtprjQ9TVlT50Gb8uKhCdqSEUp0bsjfW0 HsRbsN+PpQTUHt8AzJM9MkLy6MysT68IjCJ2o= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=rO72TwyytF2etcO0tDjvd1m3PHusSZ43+zr9ZBP6xp0=; b=SfC5d+xsU9n48b6Tlfh6TkyeOKbIisb45LFRdl+4YQsN3JDPlzr2ZGDSMRdBlo2yil A8atKGAt70wyIEDfMFi/5wiMngrtZrplbXnuUKN3GawDOf+SDP1kv6fShheCPIAxKb6d j1VrAhDepwns19h0zfh46vQOjaa+tR97mon9VXkqP+8Salh5PGy4ohPo/OIkUaiAdNHM ofDD4ZXgmUIAnFkysc09pePzloMtxTngl9tBxLd9inNZTdaQ7SPcLpuMipqTbvipEMsf Q1LlKmWo7zjmD4vIoUlJf3wJ3o8E4vavg3Ii7dInXd8KLS0UWvgRabOguWZqpHqenGRs ig9g== X-Gm-Message-State: AGRZ1gIAGeAAyQRbbVy+tQcSHVZv+0bSVwVvUe/FYAWoYkt/I73x3yj8 xbDxw95v8yc9XRnU3ltRfSd+Q8OwWQxIiiXpB6G6Ng== X-Google-Smtp-Source: AFSGD/XyBpYtRHkKzPEYFZpMmRLI+JxdYkuW2mlbuJNHWR/whVb/bKfJBxXIY9FfbBuMJQPCJaQLflt4uwwhTZG3BN0= X-Received: by 2002:a24:edc4:: with SMTP id r187mr15442703ith.158.1542997250411; Fri, 23 Nov 2018 10:20:50 -0800 (PST) MIME-Version: 1.0 References: <1540452759-4875-1-git-send-email-sughosh.ganu@arm.com> <1540452759-4875-3-git-send-email-sughosh.ganu@arm.com> In-Reply-To: <1540452759-4875-3-git-send-email-sughosh.ganu@arm.com> From: Ard Biesheuvel Date: Fri, 23 Nov 2018 19:20:38 +0100 Message-ID: To: Sughosh Ganu Cc: "edk2-devel@lists.01.org" , Achin Gupta , Leif Lindholm Subject: Re: [PATCH v3 2/6] ArmPkg/Drivers: Add EFI_MM_COMMUNICATION_PROTOCOL DXE driver. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 23 Nov 2018 18:20:51 -0000 Content-Type: text/plain; charset="UTF-8" Hello all, Apologies for the delay. Some questions below: On Thu, 25 Oct 2018 at 09:33, Sughosh Ganu wrote: > > From: Achin Gupta > > PI v1.5 Specification Volume 4 defines Management Mode Core Interface > and defines EFI_MM_COMMUNICATION_PROTOCOL. This protocol provides a > means of communicating between drivers outside of MM and MMI > handlers inside of MM. > > This patch implements the EFI_MM_COMMUNICATION_PROTOCOL DXE runtime > driver for AARCH64 platforms. It uses SMCs allocated from the standard > SMC range defined in DEN0060A_ARM_MM_Interface_Specification.pdf > to communicate with the standalone MM environment in the secure world. > > This patch also adds the MM Communication driver (.inf) file to > define entry point for this driver and other compile > related information the driver needs. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Sughosh Ganu > --- > ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf | 56 +++ > ArmPkg/Drivers/MmCommunicationDxe/MmCommunicate.h | 28 ++ > ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c | 395 ++++++++++++++++++++ > 3 files changed, 479 insertions(+) > > diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf > new file mode 100644 > index 000000000000..88beafa39c05 > --- /dev/null > +++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf > @@ -0,0 +1,56 @@ > +#/** @file > +# > +# DXE MM Communicate driver > +# > +# Copyright (c) 2016 - 2018, ARM Limited. All rights reserved. > +# > +# This program and the accompanying materials > +# are licensed and made available under the terms and conditions of the BSD License > +# which accompanies this distribution. The full text of the license may be found at > +# http://opensource.org/licenses/bsd-license.php > +# > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +# > +#**/ > + > +[Defines] > + INF_VERSION = 0x0001001A > + BASE_NAME = ArmMmCommunication > + FILE_GUID = 09EE81D3-F15E-43F4-85B4-CB9873DA5D6B > + MODULE_TYPE = DXE_RUNTIME_DRIVER > + VERSION_STRING = 1.0 > + ENTRY_POINT = MmCommunicationInitialize > + > +# > +# The following is for reference only and not required by > +# build tools > +# > +# VALID_ARCHITECTURES = AARCH64 > +# > + > +[Sources.AARCH64] > + MmCommunication.c > + > +[Packages] > + ArmPkg/ArmPkg.dec > + MdePkg/MdePkg.dec > + > +[LibraryClasses] > + ArmLib > + ArmSmcLib > + BaseMemoryLib > + DebugLib > + DxeServicesTableLib > + HobLib > + UefiDriverEntryPoint > + > +[Protocols] > + gEfiMmCommunicationProtocolGuid ## PRODUCES > + > +[Pcd.common] > + gArmTokenSpaceGuid.PcdMmBufferBase > + gArmTokenSpaceGuid.PcdMmBufferSize > + > +[Depex] > + gEfiCpuArchProtocolGuid > diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunicate.h b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunicate.h > new file mode 100644 > index 000000000000..0bf1c8d4ca0e > --- /dev/null > +++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunicate.h > @@ -0,0 +1,28 @@ > +/** @file > + > + Copyright (c) 2016-2018, ARM Limited. All rights reserved. > + > + This program and the accompanying materials > + are licensed and made available under the terms and conditions of the BSD License > + which accompanies this distribution. The full text of the license may be found at > + http://opensource.org/licenses/bsd-license.php > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > + > +**/ > + > +#if !defined _MM_COMMUNICATE_H_ > +#define _MM_COMMUNICATE_H_ > + > +#define MM_MAJOR_VER_MASK 0xEFFF0000 > +#define MM_MINOR_VER_MASK 0x0000FFFF > +#define MM_MAJOR_VER_SHIFT 16 > + > +#define MM_MAJOR_VER(x) (((x) & MM_MAJOR_VER_MASK) >> MM_MAJOR_VER_SHIFT) > +#define MM_MINOR_VER(x) ((x) & MM_MINOR_VER_MASK) > + > +#define MM_CALLER_MAJOR_VER 0x1UL > +#define MM_CALLER_MINOR_VER 0x0 > + > +#endif /* _MM_COMMUNICATE_H_ */ > diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c > new file mode 100644 > index 000000000000..487db00c2a87 > --- /dev/null > +++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c > @@ -0,0 +1,395 @@ > +/** @file > + > + Copyright (c) 2016-2018, ARM Limited. All rights reserved. > + > + This program and the accompanying materials > + are licensed and made available under the terms and conditions of the BSD License > + which accompanies this distribution. The full text of the license may be found at > + http://opensource.org/licenses/bsd-license.php > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > + > +**/ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +#include > + > +#include "MmCommunicate.h" > + > +// > +// Address, Length of the pre-allocated buffer for communication with the secure > +// world. > +// > +STATIC ARM_MEMORY_REGION_DESCRIPTOR mNsCommBuffMemRegion; > + > +// Notification event when virtual address map is set. > +STATIC EFI_EVENT mSetVirtualAddressMapEvent; > + > +// > +// Handle to install the MM Communication Protocol > +// > +STATIC EFI_HANDLE mMmCommunicateHandle; > + > +/** > + Communicates with a registered handler. > + > + This function provides an interface to send and receive messages to the > + Standalone MM environment on behalf of UEFI services. This function is part > + of the MM Communication Protocol that may be called in physical mode prior to > + SetVirtualAddressMap() and in virtual mode after SetVirtualAddressMap(). > + > + @param[in] This The EFI_MM_COMMUNICATION_PROTOCOL > + instance. > + @param[in, out] CommBuffer A pointer to the buffer to convey > + into MMRAM. > + @param[in, out] CommSize The size of the data buffer being > + passed in. This is optional. > + > + @retval EFI_SUCCESS The message was successfully posted. > + @retval EFI_INVALID_PARAMETER The CommBuffer was NULL. > + @retval EFI_BAD_BUFFER_SIZE The buffer size is incorrect 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 > +**/ > +STATIC > +EFI_STATUS > +EFIAPI > +MmCommunicationCommunicate ( > + IN CONST EFI_MM_COMMUNICATION_PROTOCOL *This, > + IN OUT VOID *CommBuffer, > + IN OUT UINTN *CommSize OPTIONAL > + ) > +{ > + EFI_MM_COMMUNICATE_HEADER *CommunicateHeader; > + ARM_SMC_ARGS CommunicateSmcArgs; > + EFI_STATUS Status; > + UINTN BufferSize; > + > + Status = EFI_ACCESS_DENIED; > + BufferSize = 0; > + > + ZeroMem (&CommunicateSmcArgs, sizeof (ARM_SMC_ARGS)); > + > + // > + // Check parameters > + // > + if (CommBuffer == NULL) { > + return EFI_INVALID_PARAMETER; > + } > + > + CommunicateHeader = 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 = CommunicateHeader->MessageLength + > + sizeof (CommunicateHeader->HeaderGuid) + > + sizeof (CommunicateHeader->MessageLength); > + > + // If the length of the CommBuffer is 0 then return the expected length. > + if (CommSize) { > + // 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 > mNsCommBuffMemRegion.Length)) { > + *CommSize = mNsCommBuffMemRegion.Length; > + return EFI_BAD_BUFFER_SIZE; > + } > + // > + // CommSize must match MessageLength + sizeof (EFI_MM_COMMUNICATE_HEADER); > + // > + if (*CommSize != BufferSize) { > + return EFI_INVALID_PARAMETER; > + } > + } > + > + // > + // If the buffer size is 0 or greater than what can be tolerated by the MM > + // environment then return the expected size. > + // > + if ((BufferSize == 0) || > + (BufferSize > mNsCommBuffMemRegion.Length)) { > + CommunicateHeader->MessageLength = mNsCommBuffMemRegion.Length - > + sizeof (CommunicateHeader->HeaderGuid) - > + sizeof (CommunicateHeader->MessageLength); > + return EFI_BAD_BUFFER_SIZE; > + } > + > + // SMC Function ID > + CommunicateSmcArgs.Arg0 = ARM_SMC_ID_MM_COMMUNICATE_AARCH64; > + > + // Cookie > + CommunicateSmcArgs.Arg1 = 0; > + > + // Copy Communication Payload > + CopyMem ((VOID *)mNsCommBuffMemRegion.VirtualBase, CommBuffer, BufferSize); > + > + // comm_buffer_address (64-bit physical address) > + CommunicateSmcArgs.Arg2 = (UINTN)mNsCommBuffMemRegion.PhysicalBase; > + > + // 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: > + ZeroMem (CommBuffer, BufferSize); > + // On successful return, the size of data being returned is inferred from > + // MessageLength + Header. > + CommunicateHeader = (EFI_MM_COMMUNICATE_HEADER *)mNsCommBuffMemRegion.VirtualBase; > + BufferSize = CommunicateHeader->MessageLength + > + sizeof (CommunicateHeader->HeaderGuid) + > + sizeof (CommunicateHeader->MessageLength); > + > + CopyMem ( > + CommBuffer, > + (VOID *)mNsCommBuffMemRegion.VirtualBase, > + 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; > +} > + > +// > +// MM Communication Protocol instance > +// > +EFI_MM_COMMUNICATION_PROTOCOL mMmCommunication = { > + MmCommunicationCommunicate > +}; > + > +/** > + Notification callback on SetVirtualAddressMap event. > + > + This function notifies the MM communication protocol interface on > + SetVirtualAddressMap event and converts pointers used in this driver > + from physical to virtual address. > + > + @param Event SetVirtualAddressMap event. > + @param Context A context when the SetVirtualAddressMap triggered. > + > + @retval EFI_SUCCESS The function executed successfully. > + @retval Other Some error occurred when executing this function. > + > +**/ > +STATIC > +VOID > +EFIAPI > +NotifySetVirtualAddressMap ( > + IN EFI_EVENT Event, > + IN VOID *Context > + ) > +{ > + EFI_STATUS Status; > + > + Status = gRT->ConvertPointer (EFI_OPTIONAL_PTR, > + (VOID **)&mNsCommBuffMemRegion.VirtualBase > + ); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "NotifySetVirtualAddressMap():" > + " Unable to convert MM runtime pointer. Status:0x%r\n", Status)); > + } > + > +} > + > +STATIC > +EFI_STATUS > +GetMmCompatibility () > +{ > + EFI_STATUS Status; > + UINT32 MmVersion; > + ARM_SMC_ARGS MmVersionArgs; > + > + // MM_VERSION uses SMC32 calling conventions > + MmVersionArgs.Arg0 = ARM_SMC_ID_MM_VERSION_AARCH32; > + > + ArmCallSmc (&MmVersionArgs); > + > + MmVersion = MmVersionArgs.Arg0; > + > + if ((MM_MAJOR_VER(MmVersion) == MM_CALLER_MAJOR_VER) && > + (MM_MINOR_VER(MmVersion) >= MM_CALLER_MINOR_VER)) { > + DEBUG ((DEBUG_INFO, "MM Version: Major=0x%x, Minor=0x%x\n", > + MM_MAJOR_VER(MmVersion), MM_MINOR_VER(MmVersion))); > + Status = EFI_SUCCESS; > + } else { > + DEBUG ((DEBUG_ERROR, "Incompatible MM Versions.\n Current Version: Major=0x%x, Minor=0x%x.\n Expected: Major=0x%x, Minor>=0x%x.\n", > + MM_MAJOR_VER(MmVersion), MM_MINOR_VER(MmVersion), MM_CALLER_MAJOR_VER, MM_CALLER_MINOR_VER)); > + Status = EFI_UNSUPPORTED; > + } > + > + return Status; > +} > + > +/** > + The Entry Point for MM Communication > + > + This function installs the MM communication protocol interface and finds out > + what type of buffer management will be required prior to invoking the > + communication SMC. > + > + @param ImageHandle The firmware allocated handle for the EFI image. > + @param SystemTable A pointer to the EFI System Table. > + > + @retval EFI_SUCCESS The entry point is executed successfully. > + @retval Other Some error occurred when executing this entry point. > + > +**/ > +EFI_STATUS > +EFIAPI > +MmCommunicationInitialize ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + EFI_STATUS Status; > + > + // Check if we can make the MM call > + Status = GetMmCompatibility (); > + if (EFI_ERROR(Status)) { > + goto ReturnErrorStatus; > + } > + > + mNsCommBuffMemRegion.PhysicalBase = PcdGet64 (PcdMmBufferBase); > + // During boot , Virtual and Physical are same > + mNsCommBuffMemRegion.VirtualBase = mNsCommBuffMemRegion.PhysicalBase; > + mNsCommBuffMemRegion.Length = PcdGet64 (PcdMmBufferSize); > + > + if (mNsCommBuffMemRegion.PhysicalBase == 0) { > + DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: " > + "Invalid MM Buffer Base Address.\n")); > + goto ReturnErrorStatus; > + } > + > + if (mNsCommBuffMemRegion.Length == 0) { > + DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: " > + "Maximum Buffer Size is zero.\n")); > + goto ReturnErrorStatus; > + } > + Please add ASSERT()s to these checks so the failure is more obvious when these values are not set. > + Status = gDS->AddMemorySpace (EfiGcdMemoryTypeSystemMemory, > + mNsCommBuffMemRegion.PhysicalBase, > + mNsCommBuffMemRegion.Length, > + EFI_MEMORY_WB | > + EFI_MEMORY_XP | > + EFI_MEMORY_RUNTIME); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: " > + "Failed to add MM-NS Buffer Memory Space\n")); > + goto ReturnErrorStatus; > + } > + > + Status = gDS->SetMemorySpaceAttributes ( > + mNsCommBuffMemRegion.PhysicalBase, > + mNsCommBuffMemRegion.Length, > + EFI_MEMORY_WB | EFI_MEMORY_XP > + ); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: " > + "Failed to set MM-NS Buffer Memory attributes\n")); > + goto CleanAddedMemorySpace; > + } > + > + Status = gBS->AllocatePages ( > + AllocateAddress, > + EfiRuntimeServicesData, > + EFI_SIZE_TO_PAGES (mNsCommBuffMemRegion.Length), > + &mNsCommBuffMemRegion.PhysicalBase > + ); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: " > + "Failed to allocate MM-NS Buffer Memory Space\n")); > + goto CleanAddedMemorySpace; > + } > + This is racy. If the shared memory region is at the top of DRAM. If we get interrupted here by an event handler, any page allocation that occurs in that context will be served from our shared memory region. So instead, I propose that we - Use AddMemorySpace() with EfiGcdMemoryTypeReserved, - Add EFI_MEMORY_RUNTIME to the attributes in the SetMemorySpaceAttributes() call - Drop the AllocatePages() call I checked the Linux code: it is the EFI_MEMORY_RUNTIME attribute that decides whether a certain region is remapped for runtime (as per the spec) so this should be safe afaict. > + // Install the communication protocol > + Status = gBS->InstallProtocolInterface ( > + &mMmCommunicateHandle, > + &gEfiMmCommunicationProtocolGuid, > + EFI_NATIVE_INTERFACE, > + &mMmCommunication > + ); > + if (EFI_ERROR(Status)) { > + DEBUG ((DEBUG_ERROR, "MmCommunicationInitialize: " > + "Failed to install MM communication protocol\n")); > + goto CleanAllocatedPages; > + } > + > + // Register notification callback when virtual address is associated > + // with the physical address. > + // Create a Set Virtual Address Map event. > + Status = gBS->CreateEvent ( > + EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE, > + TPL_NOTIFY, > + NotifySetVirtualAddressMap, > + NULL, > + &mSetVirtualAddressMapEvent > + ); > + if (Status == EFI_SUCCESS) { > + return Status; > + } > + > + gBS->UninstallProtocolInterface ( > + mMmCommunicateHandle, > + &gEfiMmCommunicationProtocolGuid, > + &mMmCommunication > + ); > + > +CleanAllocatedPages: > + gBS->FreePages ( > + mNsCommBuffMemRegion.PhysicalBase, > + EFI_SIZE_TO_PAGES (mNsCommBuffMemRegion.Length) > + ); > + > +CleanAddedMemorySpace: > + gDS->RemoveMemorySpace ( > + mNsCommBuffMemRegion.PhysicalBase, > + mNsCommBuffMemRegion.Length > + ); > + > +ReturnErrorStatus: > + return EFI_INVALID_PARAMETER; > +} > -- > 2.7.4 >