From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=217.140.101.70; helo=foss.arm.com; envelope-from=supreeth.venkatesh@arm.com; receiver=edk2-devel@lists.01.org Received: from foss.arm.com (foss.arm.com [217.140.101.70]) by ml01.01.org (Postfix) with ESMTP id 432E32220D1E7 for ; Tue, 9 Jan 2018 12:13:26 -0800 (PST) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 73CE01435; Tue, 9 Jan 2018 12:18:37 -0800 (PST) Received: from [10.0.2.15] (u203142.usa.arm.com [10.118.30.15]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id 2DEFD3F318; Tue, 9 Jan 2018 12:18:37 -0800 (PST) Message-ID: <1515529115.3113.10.camel@arm.com> From: Supreeth Venkatesh To: Meenakshi Aggarwal , "edk2-devel@lists.01.org" Cc: "leif.lindholm@linaro.org" , "ard.biesheuvel@linaro.org" Date: Tue, 09 Jan 2018 14:18:35 -0600 In-Reply-To: References: <20171117164805.13119-1-supreeth.venkatesh@arm.com> <20171117164805.13119-3-supreeth.venkatesh@arm.com> X-Mailer: Evolution 3.18.5.2-0ubuntu3.2 Mime-Version: 1.0 Subject: Re: [edk2 PATCH v3 2/2] ArmPkg/Drivers: Add EFI_MM_COMMUNICATION_PROTOCOL DXE driver. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 09 Jan 2018 20:13:27 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit On Wed, 2017-12-13 at 11:26 +0000, Meenakshi Aggarwal wrote: > Hi Supreeth, > > Few comments inline. Thank you for your comments. > > > > > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf > > Of > > Supreeth Venkatesh > > Sent: Friday, November 17, 2017 10:18 PM > > To: edk2-devel@lists.01.org > > Cc: leif.lindholm@linaro.org; ard.biesheuvel@linaro.org > > Subject: [edk2] [edk2 PATCH v3 2/2] ArmPkg/Drivers: Add > > EFI_MM_COMMUNICATION_PROTOCOL DXE driver. > > > > 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 > > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fi > > nfo > > center.arm.com%2Fhelp%2Ftopic%2Fcom.arm.doc.den0060a%2FDEN0060A_ > > ARM_MM_Interface_Specification.pdf&data=02%7C01%7Cmeenakshi.aggar > > wal%40nxp.com%7C27ae12fc41414e501f8208d52ddb09f3%7C686ea1d3bc2b4 > > c6fa92cd99c5c301635%7C0%7C1%7C636465341155830632&sdata=pYKl2uUUF > > VCS4M87y%2BRK8TE852QqcusN0Fm208IkjtU%3D&reserved=0 > > 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: Achin Gupta > > Signed-off-by: Supreeth Venkatesh > > --- > >  .../Drivers/MmCommunicationDxe/MmCommunication.c   | 339 > > +++++++++++++++++++++ > >  .../Drivers/MmCommunicationDxe/MmCommunication.inf |  50 +++ > >  2 files changed, 389 insertions(+) > >  create mode 100644 > > ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c > >  create mode 100644 > > ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf > > > > diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c > > b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c > > new file mode 100644 > > index 0000000000..e801c1c601 > > --- /dev/null > > +++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c > > @@ -0,0 +1,339 @@ > > +/** @file > > + > > +  Copyright (c) 2016-2017, 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 > > + > > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fo > > pe > > nsource.org%2Flicenses%2Fbsd- > > license.php&data=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7C27ae12 > > fc41414e501f8208d52ddb09f3%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0 > > %7C0%7C636465341155830632&sdata=GPd9o7ovyTU10etIxP%2BBYNsYUKq > > m37tPcc%2BQDKtext4%3D&reserved=0 > > + > > +  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 > > + > > +// > > +// 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; > > + > > +  CommunicateHeader = CommBuffer; > > +  Status = EFI_ACCESS_DENIED; > > +  BufferSize = 0; > > + > > +  ZeroMem (&CommunicateSmcArgs, sizeof (ARM_SMC_ARGS)); > > + > > +  // > > +  // Check parameters > > +  // > > +  if (CommBuffer == NULL) { > > +    return EFI_INVALID_PARAMETER; > > +  } > > + > > +  // If the length of the CommBuffer is 0 then return the expected > > length. > > +  if (CommSize) { > > +    if (*CommSize == 0) { > > +      *CommSize = mNsCommBuffMemRegion.Length; > > +      return EFI_BAD_BUFFER_SIZE; > > +    } > > +    // > > +    // CommSize must hold HeaderGuid and MessageLength > > +    // > > +    if (*CommSize < sizeof (EFI_MM_COMMUNICATE_HEADER)) { > > +        return EFI_INVALID_PARAMETER; > > +    } > > +    BufferSize = *CommSize; > > +  } else { > > +    BufferSize = CommunicateHeader->MessageLength + > > +                 sizeof (CommunicateHeader->HeaderGuid) + > > +                 sizeof (CommunicateHeader->MessageLength); > > +  } > > + > > +  // > > +  // 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; > > + > > +  // Reserved for Future. Must be Zero. > > +  CommunicateSmcArgs.Arg1 = 0; > > + > > +  if (mNsCommBuffMemRegion.VirtualBase) { > > +    CopyMem ((VOID *)mNsCommBuffMemRegion.VirtualBase, > > CommBuffer, BufferSize); > > +  } else { > > +    return EFI_ACCESS_DENIED; > > +  } > > + > > +  // For the SMC64 version, this parameter is a 64-bit Physical > > Address (PA) > > +  // or Intermediate Physical Address (IPA). > > +  // For the SMC32 version, this parameter is a 32-bit PA or IPA. > > +  CommunicateSmcArgs.Arg2 = > > (UINTN)mNsCommBuffMemRegion.PhysicalBase; > > + > > +  // comm_size_address is a PA or an IPA that holds the size of > > the > > +  // communication buffer being passed in. This parameter is > > optional > > +  // and can be omitted by passing a zero. > > +  // ARM does not recommend using it since this might require the > > +  // implementation to create a separate memory mapping for the > > parameter. > > +  // ARM recommends storing the buffer size in the buffer itself. > > +  CommunicateSmcArgs.Arg3 = 0; > > + > > +  // Call the Standalone MM environment. > > +  ArmCallSmc (&CommunicateSmcArgs); > > + > > +  switch (CommunicateSmcArgs.Arg0) { > > +  case ARM_SMC_MM_RET_SUCCESS: > > +    // On exit, the size of data being returned is inferred from > > +    // CommSize or MessageLength + Header. > > +    CopyMem (CommBuffer, > > +             (const VOID *)mNsCommBuffMemRegion.VirtualBase, > > +             BufferSize); > > +    Status = EFI_SUCCESS; > > +    break; > > + > > +  case ARM_SMC_MM_RET_NOT_SUPPORTED: > > +  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 > > +  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)); > > +  } > > + > > +} > > + > > +/** > > +  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; > > + > > +  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; > > +  } > > + > [Meenakshi Aggarwal] VirtualBase address assignment should be done > after checking PhysicalBase. If you prefer it that way. However, there is no major optimization, if it is before. If this is not merged yet, I will incorporate this into the next version. > > > > > +  if (mNsCommBuffMemRegion.Length == 0) { > > +    DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: " > > +            "Maximum Buffer Size is zero.\n")); > > +    goto ReturnErrorStatus; > > +  } > > + > > +  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; > [Meenakshi Aggarwal] no need of goto statement here, we can directly > return error as no cleanup is being performed  under goto statement. Use of goto here is to have more readable and easier to understand code. Even though its a "goto", its being used in a structured manner to have a only one return point for success and failure cases. > > > > +  } > > + > > +  Status = gDS- > > > > > > SetMemorySpaceAttributes(mNsCommBuffMemRegion.PhysicalBase, > [Meenakshi Aggarwal] space before opening bracket please Thanks. Ok. > > > > +                                         mNsCommBuffMemRegion.Leng > > th, > > +                                         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; > > +  } > > + > > +  // Install the communication protocol > > +  Status = gBS->InstallProtocolInterface (&mMmCommunicateHandle, > > +                                          &gEfiMmCommunicationProt > > ocolGuid, > > +                                          EFI_NATIVE_INTERFACE, > > +                                          &mMmCommunication); > > +  if (EFI_ERROR(Status)) { > [Meenakshi Aggarwal] Space after EFI_ERROR please Thanks. Ok. > > > > > +    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,  // > > Type > > +                             TPL_NOTIFY,                         / > > / NotifyTpl > > +                             NotifySetVirtualAddressMap,         / > > / NotifyFunction > > +                             NULL,                               / > > / NotifyContext > > +                             &mSetVirtualAddressMapEvent         / > > / Event > > +                            ); > > +  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; > > +} > > diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf > > b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf > > new file mode 100644 > > index 0000000000..d39ee5fdd7 > > --- /dev/null > > +++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf > > @@ -0,0 +1,50 @@ > > +#/** @file > > +# > > +#  DXE MM Communicate driver > > +# > > +#  Copyright (c) 2016 - 2017, 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 > > +# > > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fo > > pe > > nsource.org%2Flicenses%2Fbsd- > > license.php&data=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7C27ae12 > > fc41414e501f8208d52ddb09f3%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0 > > %7C0%7C636465341155830632&sdata=GPd9o7ovyTU10etIxP%2BBYNsYUKq > > m37tPcc%2BQDKtext4%3D&reserved=0 > > +# > > +#  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 > > + > > +[Sources.Common] > > +  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] > > +  TRUE > > -- > > 2.14.1 > > > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2F > > list > > s.01.org%2Fmailman%2Flistinfo%2Fedk2- > > devel&data=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7C27ae12fc4141 > > 4e501f8208d52ddb09f3%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0% > > 7C636465341155830632&sdata=pWv6hx1oHdtDyEnKx1HpyBnpmn7XikuwA85 > > 8yu7148E%3D&reserved=0