Re: [PATCH v2 2/3] ArmPkg/Drivers: Add EFI_MM_COMMUNICATION_PROTOCOL DXE driver.

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

On Thu, 2017-10-26 at 11:13 +0100, Achin Gupta wrote:
> Hi Supreeth,
> 
> some CIL,
> 
> On Wed, Oct 25, 2017 at 05:32:57PM +0100, 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 defined in
> > http://infocenter.arm.com/help/topic/com.arm.doc.den0060a/DEN0060A_
> > ARM_MM_Interface_Specification.pdf
> > to communicate with the standalone MM environment in the secure
> > world.
> > 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Achin Gupta <achin.gupta@arm.com>
> > Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
> > ---
> >  .../Drivers/MmCommunicationDxe/MmCommunication.c   | 339
> > +++++++++++++++++++++
> >  1 file changed, 339 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..ecf70e666c
> > --- /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
> > +  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
> > +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 = {
> > +  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;
> > +  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) {
> > +    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);
> > +  }
> Should the else condition not assert as VirtualBase should always be
> non-zero?
It is already handled in Initialize. This is a redundant check.
If the data is not copied over, the secure firmware will be able to
handle this. However, since there is this check/condition, it should
have an corresponding else to assert.  
> 
> > 
> > +
> > +  // 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;
> If CommSize has been used then the spec. does not say if
> CommunicateHeader->MessageLength must match it. IMH, we should update
> the latter
> with the former to prevent a situation where neither indicates the
> size of the
> buffer.
Yes. There is no clear statement in the spec regarding this.
However, MM interface specification recommends not to use this
argument. So, IMHO, the inference for the client of this protocol is
that CommunicateHeader->MessageLength should always indicate the size
of the data excluding the header, since CommSize = (size of data +
HEADER) is optional.
In my opinion, The inference from the specification is
CommunicateHeader->MessageLength is **not** optional and should reflect
the exact data size always. 
> 
> > 
> > +
> > +  // Call the Standalone MM environment.
> > +  ArmCallSmc (&CommunicateSmcArgs);
> > +
> > +  Status = CommunicateSmcArgs.Arg0;
> > +  switch (Status) {
> > +  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);
> > +    break;
> > +
> > +  case ARM_SMC_MM_RET_NOT_SUPPORTED:
> Can we treat this as a spurious error below and assert?
> 
> > 
> > +  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
> A better explanation might be: Since this driver relies on a buffer
> pre-allocated by the secure world this error code should never be
> returned.
> 
> Also, to handle this case there should be an assertion here?
Yes. This is the implicit assumption. 
shouldn't the client handle the returned error code?
> 
> > 
> > +  default:
> > +    Status = EFI_ACCESS_DENIED;
> > +    ASSERT (0);
> Can we print an error string here since the secure world has just
> returned an
> unknown error code.
Shouldn't the client handle the returned error code?
Having Print statements here will work only during boot and will not
work when we are executing this code during UEFI Runtime as the serial
console would need reinitialization at-least in Fixed Virtual Platform.
> 
> > 
> > +  }
> > +
> > +  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
> > +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%x\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;
> > +
> Forgot to mention this earlier but we should first invoke MM_VERSION
> to ensure
> the driver is dealing with the right interface and possibly print
> this.
PI Specification does not mention about this. However there is mention
of this in MM Interface document.
When we have secure implementation to match this, it would be great if
you could post it at that time and I will be happy to review it at that
time.  
> 
> > 
> > +  mNsCommBuffMemRegion.PhysicalBase = 0;
> > +  mNsCommBuffMemRegion.VirtualBase = 0;
> > +  mNsCommBuffMemRegion.Length = 0;
> > +
> > +  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"));
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  if (mNsCommBuffMemRegion.Length == 0) {
> > +    DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: Maximum Buffer
> > Size is zero.\n"));
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  Status = gDS->AddMemorySpace (EfiGcdMemoryTypeSystemMemory,
> > +                                mNsCommBuffMemRegion.PhysicalBase,
> > +                                mNsCommBuffMemRegion.Length,
> > +                                EFI_MEMORY_WB |
> > +                                EFI_MEMORY_XP |
> EFI_MEMORY_XP is not used on AArch64?

In ArmMmuLib - GcdAttributeToPageAttribute(), EFI_MEMORY_XP is being
converted, So I assume it is being used. 
Ard - Correct me, if I am incorrect?
> 
> cheers,
> Achin
> 
> > 
> > +                                EFI_MEMORY_RUNTIME);
> > +  if (EFI_ERROR (Status)) {
> > +    DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: Failed to add
> > MM-NS Buffer Memory Space\n"));
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  Status = gDS-
> > >SetMemorySpaceAttributes(mNsCommBuffMemRegion.PhysicalBase,
> > +                                         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"));
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  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"));
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  // Install the communication protocol
> > +  Status = gBS->InstallProtocolInterface (&mMmCommunicateHandle,
> > +                                          &gEfiMmCommunicationProt
> > ocolGuid,
> > +                                          EFI_NATIVE_INTERFACE,
> > +                                          &mMmCommunication);
> > +  if (EFI_ERROR(Status)) {
> > +    DEBUG ((DEBUG_ERROR, "MmCommunicationInitialize: Failed to
> > install MM communication protocol\n"));
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  // 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
> >