Re: [PATCH v1 1/3] ArmPkg/Drivers: Add EFI_MM_COMMUNICATION_PROTOCOL DXE driver.

[Supreeth Venkatesh <supreeth.venkatesh@arm.com>]

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
> <supreeth.venkatesh@arm.com> 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 <supreeth.venkatesh@arm.com>
> > ---
> >  .../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 <Library/ArmLib.h>
> > +#include <Library/ArmSmcLib.h>
> > +#include <Library/BaseMemoryLib.h>
> > +#include <Library/DebugLib.h>
> > +#include <Library/DxeServicesTableLib.h>
> > +#include <Library/HobLib.h>
> > +#include <Library/PcdLib.h>
> > +#include <Library/UefiBootServicesTableLib.h>
> > +#include <Library/UefiRuntimeServicesTableLib.h>
> > +
> > +#include <Protocol/MmCommunication.h>
> > +
> > +#include <IndustryStandard/ArmStdSmc.h>
> > +
> > +/**
> > +  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