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 57E0C202E60ED for ; Wed, 25 Oct 2017 09:36:24 -0700 (PDT) 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 5208E80D; Wed, 25 Oct 2017 09:40:09 -0700 (PDT) Received: from [10.0.2.15] (u203142.usa.arm.com [10.118.30.109]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id 180763F246; Wed, 25 Oct 2017 09:40:09 -0700 (PDT) Message-ID: <1508949608.2923.2.camel@arm.com> From: Supreeth Venkatesh To: Ard Biesheuvel Cc: "edk2-devel@lists.01.org" , Leif Lindholm Date: Wed, 25 Oct 2017 11:40:08 -0500 In-Reply-To: References: <20171012171352.45168-1-supreeth.venkatesh@arm.com> <20171012171352.45168-2-supreeth.venkatesh@arm.com> X-Mailer: Evolution 3.18.5.2-0ubuntu3.2 Mime-Version: 1.0 Subject: Re: [PATCH v1 1/3] ArmPkg/Drivers: Add EFI_MM_COMMUNICATION_PROTOCOL DXE driver. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 25 Oct 2017 16:36:24 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit On Thu, 2017-10-12 at 18:29 +0100, Ard Biesheuvel wrote: >  Hi Supreeth, Hi Ard, Thanks for your comments. I have rolled all of them in v2 of this patch series except for "static" comment below. > > On 12 October 2017 at 18:13, Supreeth Venkatesh > wrote: > > > > 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 in the SMC Calling Convention specification to > > communicate > > with the standalone MM environment in the secure world. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Supreeth Venkatesh > > --- > >  .../Drivers/MmCommunicationDxe/MmCommunication.c   | 314 > > +++++++++++++++++++++ > >  1 file changed, 314 insertions(+) > >  create mode 100644 > > ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c > > > > diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c > > b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c > > new file mode 100644 > > index 0000000000..6064c7d345 > > --- /dev/null > > +++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c > > @@ -0,0 +1,314 @@ > > +/** @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 > > +  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 > > + > > +/** > > +  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.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       The CommBuffer was NULL. > > +  @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 > Can this be static? If yes, please remove the forward declaration > entirely, and reorder the definition with the reference. > > > > > +EFIAPI > > +MmCommunicationCommunicate ( > > +  IN CONST EFI_MM_COMMUNICATION_PROTOCOL  *This, > > +  IN OUT VOID                             *CommBuffer, > > +  IN OUT UINTN                            *CommSize    OPTIONAL > > +  ); > > + > > +// > > +// 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; > > + > > +// > > +// MM Communication Protocol instance > > +// > > +EFI_MM_COMMUNICATION_PROTOCOL  mMmCommunication = { > Can this be static? > > > > > +  MmCommunicationCommunicate > > +}; > > + > > +/** > > +  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 SMRAM. > > +  @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       The CommBuffer was NULL. > > +  @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 > > +MmCommunicationCommunicate ( > > +  IN CONST EFI_MM_COMMUNICATION_PROTOCOL  *This, > > +  IN OUT VOID                             *CommBuffer, > > +  IN OUT UINTN                            *CommSize > > +  ) > > +{ > > +  EFI_MM_COMMUNICATE_HEADER  *CommunicateHeader; > Please align the * with the other variable names > > > > > +  ARM_SMC_ARGS                CommunicateSmcArgs; > > +  EFI_STATUS                  Status; > > +  UINTN                       BufferSize; > > + > > +  CommunicateHeader = CommBuffer; > > +  Status = EFI_SUCCESS; > > +  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) { > > +    BufferSize = *CommSize; > > +    // > > +    // CommSize must hold HeaderGuid and MessageLength > > +    // > > +    if ( BufferSize < sizeof(EFI_MM_COMMUNICATE_HEADER) ) { > No spaces inside () please > > > > > +        return EFI_INVALID_PARAMETER; > > +    } > > +  } else { > > +    BufferSize = CommunicateHeader->MessageLength + > > sizeof(CommunicateHeader->HeaderGuid) + sizeof(CommunicateHeader- > > >MessageLength); > Spaces before ( please, and limit the line length to 80 columns. > > > > > +  } > > + > > +  // > > +  // If the buffer size if 0 or greater than what can be tolerated > > by the MM > > +  // environment then return the expected size. > > +  // > > +  if ( (BufferSize == 0) || > > +       (BufferSize > mNsCommBuffMemRegion.Length) ) { > No spaces inside () please > > > > > +    CommunicateHeader->MessageLength = > > mNsCommBuffMemRegion.Length; > > +    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; > > + > > +  CopyMem((VOID *)mNsCommBuffMemRegion.VirtualBase, CommBuffer, > > BufferSize); > Space before ( > > > > > + > > +  // 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; > No spaces after (type) casts please. > > > > > + > > +  CommunicateSmcArgs.Arg3 = (UINTN) BufferSize; > > + > > +  // Call the Standalone MM environment. > > +  ArmCallSmc(&CommunicateSmcArgs); > Space before ( > > > > > + > > +  Status = CommunicateSmcArgs.Arg0; > > +  switch (Status) { > > +  case EFI_SUCCESS: > Please don't overload the EFI_STATUS type. If EFI_SUCCESS and > ARM_SMC_MM_RET_SUCCESS happen to have the same numerical value, that > doesn't mean you should cut corners like this. So instead, > > switch (CommunicateSmcArgs.Arg0) { > case ARM_SMC_MM_RET_SUCCESS: >   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: > > +          ASSERT (0); > and don't forget to assign Status here. > > > > > > +  } > > + > > +  return Status; > > +} > > + > > +/** > > +  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 > VOID on a separate line please > > > > > +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 ((EFI_D_ERROR, "NotifySetVirtualAddressMap(): Unable to > > convert MM runtime pointer. Status:0x%x\n", Status)); > please use DEBUG_xxx not EFI_D_xxx (those are deprecated) > > > > > +  } > > + > > +} > > + > > +/** > > +  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 = 0; > > +  mNsCommBuffMemRegion.VirtualBase = 0; > > +  mNsCommBuffMemRegion.Length = 0; > > + > > +  mNsCommBuffMemRegion.PhysicalBase = PcdGet64(PcdMmBufferBase); > Space before ( > > > > > +  // During boot , Virtual and Physical are same > > +  mNsCommBuffMemRegion.VirtualBase = > > mNsCommBuffMemRegion.PhysicalBase; > > +  mNsCommBuffMemRegion.Length = PcdGet64(PcdMmBufferSize); > and here > > > > > + > > +  if (mNsCommBuffMemRegion.PhysicalBase == 0) { > > +    DEBUG ((EFI_D_ERROR, "MmCommunicateInitialize: Invalid MM > > Buffer Base Address.\n")); > > +    return EFI_INVALID_PARAMETER; > > +  } > > + > > +  if (mNsCommBuffMemRegion.Length == 0) { > > +    DEBUG ((EFI_D_ERROR, "MmCommunicateInitialize: Maximum Buffer > > Size is zero.\n")); > > +    return EFI_INVALID_PARAMETER; > > +  } > > + > > +  Status = gDS->AddMemorySpace (EfiGcdMemoryTypeSystemMemory, > > +                                mNsCommBuffMemRegion.PhysicalBase, > > +                                mNsCommBuffMemRegion.Length, > > +                                EFI_MEMORY_UC | > > EFI_MEMORY_RUNTIME); > > +  if (EFI_ERROR (Status)) { > > +    DEBUG ((EFI_D_ERROR, "MmCommunicateInitialize: Failed to add > > MM-NS Buffer Memory Space\n")); > Are you sure it makes sense to proceed after this error? > > > > > +  } > > + > > +  Status = gDS- > > >SetMemorySpaceAttributes(mNsCommBuffMemRegion.PhysicalBase, > > +                                         mNsCommBuffMemRegion.Leng > > th, > > +                                         EFI_MEMORY_UC); > > +  if (EFI_ERROR (Status)) { > > +    DEBUG ((EFI_D_ERROR, "MmCommunicateInitialize: Failed to set > > MM-NS Buffer Memory attributes\n")); > And here > > > > > +  } > > + > > +  Status = gBS->AllocatePages (AllocateAddress, > > +                               EfiRuntimeServicesData, > > +                               EFI_SIZE_TO_PAGES > > (mNsCommBuffMemRegion.Length), > > +                               &mNsCommBuffMemRegion.PhysicalBase) > > ; > > +  if (EFI_ERROR (Status)) { > > +    DEBUG ((EFI_D_ERROR, "MmCommunicateInitialize: Failed to > > allocate MM-NS Buffer Memory Space\n")); > And here > > > > > +  } > > + > > +  DEBUG ((DEBUG_INFO, "MmCommunicateInitialize: NsBufferAddress - > > 0x%x, mNsCommBuffMemRegion.Length - 0x%x\n", > > mNsCommBuffMemRegion.PhysicalBase, mNsCommBuffMemRegion.Length)); > Line length < 80 columns please > > > > > + > > +  // Install the communication protocol > > +  Status = gBS->InstallProtocolInterface (&mMmCommunicateHandle, > > +                                          &gEfiMmCommunicationProt > > ocolGuid, > > +                                          EFI_NATIVE_INTERFACE, > > +                                          &mMmCommunication); > > +  if (EFI_ERROR(Status)) { > > +    DEBUG((EFI_D_ERROR, "MmCommunicationInitialize: Failed to > > install MM communication protocol\n")); > return Status > > > > > +  } > > + > > +  // 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 > > +                            ); > > +  ASSERT_EFI_ERROR (Status); > > + > > +  return Status; > > +} > > -- > > 2.14.1 > > > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel