[PATCH v2 0/3] *** EFI_MM_COMMUNICATION_PROTOCOL ***

[PATCH v2 0/3] *** EFI_MM_COMMUNICATION_PROTOCOL ***

[Supreeth Venkatesh]

***
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.

EFI_MM_COMMUNICATION_PROTOCOL
Summary
  This protocol provides a means of communicating between drivers outside of MM and MMI
  handlers inside of MM.

GUID
  #define EFI_MM_COMMUNICATION_PROTOCOL_GUID \
  { 0xc68ed8e2, 0x9dc6, 0x4cbd, 0x9d, 0x94, 0xdb, 0x65, 0xac, 0xc5, 0xc3, 0x32 }

Prototype
  typedef struct _EFI_MM_COMMUNICATION_PROTOCOL {
  EFI_MM_COMMUNICATE Communicate;
  } EFI_MM_COMMUNICATION_PROTOCOL;

Members
Communicate
  Sends/receives a message for a registered handler. See the Communicate()
  function description.

Description
  This protocol provides runtime services for communicating between DXE drivers and a registered
  MMI handler.

EFI_MM_COMMUNICATION_PROTOCOL.Communicate()
Summary
  Communicates with a registered handler.

Prototype
  typedef
  EFI_STATUS
  (EFIAPI *EFI_MM_COMMUNICATE) (
  IN CONST EFI_MM_COMMUNICATION_PROTOCOL *This,
  IN OUT VOID *CommBuffer,
  IN OUT UINTN *CommSize OPTIONAL
  );

Parameters
  This - The EFI_MM_COMMUNICATION_PROTOCOL instance.
  CommBuffer - Pointer to the buffer to convey into MMRAM.
  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. This parameter is optional
                     and may be NULL.

Description
  This function provides a service to send and receive messages from a registered UEFI service. The EFI_MM_COMMUNICATION_PROTOCOL driver is responsible for doing any of the
  copies such that the data lives in boot-service-accessible RAM.
  A given implementation of the EFI_MM_COMMUNICATION_PROTOCOL may choose to use the EFI_MM_CONTROL_PROTOCOL for effecting the mode transition, or it may use some other method.
  The agent invoking the communication interface at runtime may be virtually mapped. The MM infrastructure code and handlers, on the other hand, execute in physical mode. As a result, the non-MM agent,
  which may be executing in the virtual-mode OS context (as a result of an OS invocation of the UEFI SetVirtualAddressMap() service), should use a contiguous memory buffer with a physical address before
  invoking this service. If the virtual address of the buffer is used, the MM Driver may not know how to do the appropriate virtual-to-physical conversion.
  To avoid confusion in interpreting frames, the CommunicateBuffer parameter should always begin with EFI_MM_COMMUNICATE_HEADER, which is defined in Related Definitions below.
  The header data is mandatory for messages sent into the MM agent.
  If the CommSize parameter is omitted the MessageLength field in the EFI_MM_COMMUNICATE_HEADER,
  in conjunction with the size of the header itself, can be used to ascertain the total size of the communication payload.
  If the MessageLength is zero, or too large for the MM implementation to manage, the MM implementation must update the MessageLength to reflect the size of the Data buffer that it can tolerate.
  If the CommSize parameter is passed into the call, but the integer it points to, has a value of 0, then this must be updated to reflect the maximum size of the CommBuffer that the implementation can tolerate.
  Once inside of MM, the MM infrastructure will call all registered handlers with the same HandlerType as the GUID specified by HeaderGuid and the CommBuffer pointing to Data.
  This function is not reentrant.
  The standard header is used at the beginning of the EFI_MM_INITIALIATION_HEADER structure during MM initialization. See "Related Definitions" below for more information.

Related Definitions
  typedef struct {
  EFI_GUID HeaderGuid;
  UINTN MessageLength;
  UINT8 Data[ANYSIZE_ARRAY];
  } EFI_MM_COMMUNICATE_HEADER;

  HeaderGuid - Allows for disambiguation of the message format. Type EFI_GUID is defined in
                       InstallProtocolInterface() in the UEFI Specification.
  MessageLength - Describes the size of Data (in bytes) and does not include the size of the header.
  Data - Designates an array of bytes that is MessageLength in size.

Status Codes Returned
  EFI_SUCCESS - The message was successfully posted.
  EFI_INVALID_PARAMETER - The buffer was NULL.
  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.
                                           See the function description above for more details.
  EFI_ACCESS_DENIED - The CommunicateBuffer parameter or CommSize
                                        parameter, if not omitted, are in address range that cannot be
                                        accessed by the MM environment.

This patchset implements it on AARCH64 Platform.
The PI concept of a Standalone MM environment is intialized during the
SEC phase in AARCH64 Platform i.e., On AARCH64, this environment is initialized
and locked down in the secure world.
This driver enables communication with the MM environment during boot and
runtime.
***
Changes Since v1:
(*) Update Review Comments from Ard, Jan, Achin.
(*) Reword Commit subject for the patches in the set to convey more meaningful summary. 
***
Supreeth Venkatesh (3):
  ArmPkg: Add PCDs needed for MM communication driver.
  ArmPkg/Drivers: Add EFI_MM_COMMUNICATION_PROTOCOL DXE driver.
  ArmPkg/Drivers: Add ArmMmCommunication Driver information file.

 ArmPkg/ArmPkg.dec                                  |   3 +
 .../Drivers/MmCommunicationDxe/MmCommunication.c   | 339 +++++++++++++++++++++
 .../Drivers/MmCommunicationDxe/MmCommunication.inf |  50 +++
 3 files changed, 392 insertions(+)
 create mode 100644 ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
 create mode 100644 ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf

-- 
2.14.1

[PATCH v2 1/3] ArmPkg: Add PCDs needed for MM communication driver.

[Supreeth Venkatesh]

This patch defines PCDs to describe the base address and size of
communication buffer between normal world (uefi) and 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>
---
 ArmPkg/ArmPkg.dec | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
index f99054a7de..d871ecc654 100644
--- a/ArmPkg/ArmPkg.dec
+++ b/ArmPkg/ArmPkg.dec
@@ -229,6 +229,9 @@
   gArmTokenSpaceGuid.PcdSystemMemoryBase|0|UINT64|0x00000029
   gArmTokenSpaceGuid.PcdSystemMemorySize|0|UINT64|0x0000002A
 
+  gArmTokenSpaceGuid.PcdMmBufferBase|0|UINT64|0x00000045
+  gArmTokenSpaceGuid.PcdMmBufferSize|0|UINT64|0x00000046
+
 [PcdsFixedAtBuild.common, PcdsDynamic.common]
   #
   # ARM Architectural Timer
-- 
2.14.1

Re: [PATCH v2 1/3] ArmPkg: Add PCDs needed for MM communication driver.

[Ard Biesheuvel]

On 25 October 2017 at 17:32, Supreeth Venkatesh
<supreeth.venkatesh@arm.com> wrote:
> This patch defines PCDs to describe the base address and size of
> communication buffer between normal world (uefi) and 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>
> ---
>  ArmPkg/ArmPkg.dec | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
> index f99054a7de..d871ecc654 100644
> --- a/ArmPkg/ArmPkg.dec
> +++ b/ArmPkg/ArmPkg.dec
> @@ -229,6 +229,9 @@
>    gArmTokenSpaceGuid.PcdSystemMemoryBase|0|UINT64|0x00000029
>    gArmTokenSpaceGuid.PcdSystemMemorySize|0|UINT64|0x0000002A
>
> +  gArmTokenSpaceGuid.PcdMmBufferBase|0|UINT64|0x00000045
> +  gArmTokenSpaceGuid.PcdMmBufferSize|0|UINT64|0x00000046
> +

Do we need to hardcode this? Is there no way for the non-secure side
to retrieve this information via SMC calls etc?

>  [PcdsFixedAtBuild.common, PcdsDynamic.common]
>    #
>    # ARM Architectural Timer
> --
> 2.14.1
>

Re: [PATCH v2 1/3] ArmPkg: Add PCDs needed for MM communication driver.

[Supreeth Venkatesh]

On Mon, 2017-11-13 at 10:17 +0000, Ard Biesheuvel wrote:
> On 25 October 2017 at 17:32, Supreeth Venkatesh
> <supreeth.venkatesh@arm.com> wrote:
> > 
> > This patch defines PCDs to describe the base address and size of
> > communication buffer between normal world (uefi) and 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>
> > ---
> >  ArmPkg/ArmPkg.dec | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
> > index f99054a7de..d871ecc654 100644
> > --- a/ArmPkg/ArmPkg.dec
> > +++ b/ArmPkg/ArmPkg.dec
> > @@ -229,6 +229,9 @@
> >    gArmTokenSpaceGuid.PcdSystemMemoryBase|0|UINT64|0x00000029
> >    gArmTokenSpaceGuid.PcdSystemMemorySize|0|UINT64|0x0000002A
> > 
> > +  gArmTokenSpaceGuid.PcdMmBufferBase|0|UINT64|0x00000045
> > +  gArmTokenSpaceGuid.PcdMmBufferSize|0|UINT64|0x00000046
> > +
> Do we need to hardcode this? Is there no way for the non-secure side
> to retrieve this information via SMC calls etc?
> 
This is not in the Specification yet. I am trying to get this added to
specification. Hence, for the time being, we are still using PCDs.

> > 
> >  [PcdsFixedAtBuild.common, PcdsDynamic.common]
> >    #
> >    # ARM Architectural Timer
> > --
> > 2.14.1
> > 

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

[Supreeth Venkatesh]

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);
+  }
+
+  // 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);
+
+  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:
+  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;
+}
+
+/**
+  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;
+
+  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_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.Length,
+                                         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,
+                                          &gEfiMmCommunicationProtocolGuid,
+                                          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

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

[Udit Kumar]

Hi 

> +  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,
> +                                          &gEfiMmCommunicationProtocolGuid,
> +                                          EFI_NATIVE_INTERFACE,
> +                                          &mMmCommunication);  if
> + (EFI_ERROR(Status)) {
> +    DEBUG ((DEBUG_ERROR, "MmCommunicationInitialize: Failed to install MM
> communication protocol\n"));

In case of error, you could free the above allocated memory 

> +    return EFI_INVALID_PARAMETER;
> +  

Regards
Udit 

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Supreeth Venkatesh
> Sent: Wednesday, October 25, 2017 10:03 PM
> To: edk2-devel@lists.01.org
> Cc: leif.lindholm@linaro.org; ard.biesheuvel@linaro.org
> Subject: [edk2] [PATCH v2 2/3] 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%2Finfocen
> ter.arm.com%2Fhelp%2Ftopic%2Fcom.arm.doc.den0060a%2FDEN0060A_ARM_
> MM_Interface_Specification.pdf&data=02%7C01%7Cudit.kumar%40nxp.com%7
> C43194cfd48f84756da6908d51bc6185b%7C686ea1d3bc2b4c6fa92cd99c5c3016
> 35%7C0%7C1%7C636445460008712743&sdata=Nk9bnFSzqB4zxCcyd4C2HyqZ2a
> Iluu%2FWKXOVho4a6g8%3D&reserved=0
> 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
> +
> + https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fopen
> + source.org%2Flicenses%2Fbsd-
> license.php&data=02%7C01%7Cudit.kumar%40nx
> +
> p.com%7C43194cfd48f84756da6908d51bc6185b%7C686ea1d3bc2b4c6fa92cd9
> 9c5c3
> +
> 01635%7C0%7C0%7C636445460008712743&sdata=%2Fw7nb5B%2Bw3NKLV4v
> 5LwcaQ0%2
> + F6gh2Cva%2FEyDsb69NEAM%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 <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);  }
> +
> +  // 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);
> +
> +  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:
> +  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;
> +}
> +
> +/**
> +  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;
> +
> +  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_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.Length,
> +                                         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,
> +                                          &gEfiMmCommunicationProtocolGuid,
> +                                          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
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.01
> .org%2Fmailman%2Flistinfo%2Fedk2-
> devel&data=02%7C01%7Cudit.kumar%40nxp.com%7C43194cfd48f84756da690
> 8d51bc6185b%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6364454
> 60008712743&sdata=3DNXnOuo8%2FVFL6Sha%2FeidNhuh4JoILCVQs1HcGAhvr
> Y%3D&reserved=0

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

[Supreeth Venkatesh]

On Thu, 2017-10-26 at 05:05 +0000, Udit Kumar wrote:
> Hi 
> 
> > 
> > +  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"));
> In case of error, you could free the above allocated memory 
> 
> > 
> > +    return EFI_INVALID_PARAMETER;
> > +  

Thanks. Yes. It could be done. I will fix it in next version.

> Regards
> Udit 
> 
> > 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> > Of
> > Supreeth Venkatesh
> > Sent: Wednesday, October 25, 2017 10:03 PM
> > To: edk2-devel@lists.01.org
> > Cc: leif.lindholm@linaro.org; ard.biesheuvel@linaro.org
> > Subject: [edk2] [PATCH v2 2/3] 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
> > nfocen
> > ter.arm.com%2Fhelp%2Ftopic%2Fcom.arm.doc.den0060a%2FDEN0060A_ARM_
> > MM_Interface_Specification.pdf&data=02%7C01%7Cudit.kumar%40nxp.com%
> > 7
> > C43194cfd48f84756da6908d51bc6185b%7C686ea1d3bc2b4c6fa92cd99c5c3016
> > 35%7C0%7C1%7C636445460008712743&sdata=Nk9bnFSzqB4zxCcyd4C2HyqZ2a
> > Iluu%2FWKXOVho4a6g8%3D&reserved=0
> > 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
> > +
> > + https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2
> > Fopen
> > + source.org%2Flicenses%2Fbsd-
> > license.php&data=02%7C01%7Cudit.kumar%40nx
> > +
> > p.com%7C43194cfd48f84756da6908d51bc6185b%7C686ea1d3bc2b4c6fa92cd9
> > 9c5c3
> > +
> > 01635%7C0%7C0%7C636445460008712743&sdata=%2Fw7nb5B%2Bw3NKLV4v
> > 5LwcaQ0%2
> > + F6gh2Cva%2FEyDsb69NEAM%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 <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);  }
> > +
> > +  // 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);
> > +
> > +  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:
> > +  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;
> > +}
> > +
> > +/**
> > +  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;
> > +
> > +  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_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
> > 
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > lists.01
> > .org%2Fmailman%2Flistinfo%2Fedk2-
> > devel&data=02%7C01%7Cudit.kumar%40nxp.com%7C43194cfd48f84756da690
> > 8d51bc6185b%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6364454
> > 60008712743&sdata=3DNXnOuo8%2FVFL6Sha%2FeidNhuh4JoILCVQs1HcGAhvr
> > Y%3D&reserved=0

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

[Achin Gupta]

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?

> +
> +  // 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.

> +
> +  // 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?

> +  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.

> +  }
> +
> +  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.

> +  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?

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.Length,
> +                                         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,
> +                                          &gEfiMmCommunicationProtocolGuid,
> +                                          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
>

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

[Supreeth Venkatesh]

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
> > 

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

[Ard Biesheuvel]

On 25 October 2017 at 17:32, 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 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 +++++++++++++++++++++

No need to separate the .inf and .c changes, so this patch can be
merged with the previous one.

>  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
> +**/

Please don't use lines > 80 characters.

> +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
> +};
> +

Please move this below the definition of MmCommunicationCommunicate(),
and drop the forward declaration.

> +/**
> +  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.

What does 'convey into SMRAM' mean?

> +  @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.
> +

CommSize is a pointer so it could be NULL but not zero. Or do you mean
*CommSize?

> +  @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
> +**/

STATIC ?

> +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;

Here you return EFI_BAD_BUFFER_SIZE for a 0 sized buffer, while the
comment says is it returned when the buffer is too large.

> +    }
> +    //
> +    // 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? What happens if VirtualBase == NULL? Can we still proceed without copying?

> +  // 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);
> +
> +  Status = CommunicateSmcArgs.Arg0;
> +  switch (Status) {

I think I mentioned this before, but please don't use Status for the
ARM_SMC_MM_xxx enum space. Use a separate, correctly typed variable
instead.

> +  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:
> +  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;
> +}
> +
> +/**
> +  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));

Please use %r for reporting EFI_STATUS values.

> +  }
> +
> +}
> +
> +/**
> +  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;
> +

Please drop the 0 assignmentsm since they are overridden immediately.

> +  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_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.Length,
> +                                         EFI_MEMORY_WB | EFI_MEMORY_XP);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: Failed to set MM-NS Buffer Memory attributes\n"));

cleanup?

> +    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"));

cleanup?

> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  // 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"));

cleanup?

> +    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);
> +

cleanup?

> +  return Status;
> +}
> --
> 2.14.1
>

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

[Supreeth Venkatesh]

On Mon, 2017-11-13 at 10:30 +0000, Ard Biesheuvel wrote:
> On 25 October 2017 at 17:32, 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 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
> > +++++++++++++++++++++
> No need to separate the .inf and .c changes, so this patch can be
> merged with the previous one.
Thanks for more comments/feedback.
Ok. Please wait for v3.
> 
> > 
> >  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
> > +**/
> Please don't use lines > 80 characters.
Ok.
> 
> > 
> > +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
> > +};
> > +
> Please move this below the definition of
> MmCommunicationCommunicate(),
> and drop the forward declaration.
Ok.
> 
> > 
> > +/**
> > +  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.
> What does 'convey into SMRAM' mean?
> 
> > 
> > +  @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.
> > +
> CommSize is a pointer so it could be NULL but not zero. Or do you
> mean
> *CommSize?
> 
CommSize is an optional parameter. Yeah, if Commsize is used, then its
value (*CommSize) will be zero, if there is no data. This is something
the specification implicitly states.
> > 
> > +  @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
> > +**/
> STATIC ?
> 
This implements an EFIAPI for the protocol MM_COMMUNICATION_PROTOCOL in
MdePkg.Not sure why you want this function static?
> > 
> > +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;
> Here you return EFI_BAD_BUFFER_SIZE for a 0 sized buffer, while the
> comment says is it returned when the buffer is too large.
> 
Specification states this:
"If the CommSize parameter is passed into the call, but the integer it
points to, has a value of 0, then this must be updated to reflect the
maximum size of the CommBuffer that the implementation can tolerate."
Also, Return Values are
EFI_INVALID_PARAMETER - The buffer was NULL.
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.
See the function description above for more details

So, it does not specify what should be the return value, when *CommSize
is zero. It is open to interpretation between EFI_INVALID_PARAMETER and
EFI_BAD_BUFFER_SIZE. I choose EFI_BAD_BUFFER_SIZE.
Let me know what you think.

> > 
> > +    }
> > +    //
> > +    // 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? What happens if VirtualBase == NULL? Can we still proceed
> without copying?
Yup. This was Achin's comment as well. Please wait for v3.
> 
> > 
> > +  // 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);
> > +
> > +  Status = CommunicateSmcArgs.Arg0;
> > +  switch (Status) {
> I think I mentioned this before, but please don't use Status for the
> ARM_SMC_MM_xxx enum space. Use a separate, correctly typed variable
> instead.
Oh. your last email asks to replace EFI_SUCCESS with MM Specific RETURN
SUCCESS. Status is still "UINTN" type variable for both MM and general
UEFI status codes.
Let me know if you feel strongly about it, I will have two different
"UINTN" type variables.
> 
> > 
> > +  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:
> > +  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;
> > +}
> > +
> > +/**
> > +  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));
> Please use %r for reporting EFI_STATUS values.
> 
Ok.
> > 
> > +  }
> > +
> > +}
> > +
> > +/**
> > +  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;
> > +
> Please drop the 0 assignmentsm since they are overridden immediately.
Ok.
> 
> > 
> > +  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_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"));
> cleanup?
Yup. Wait for v3.
> 
> > 
> > +    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"));
> cleanup?
> 
This was Udit's comment as well. Wait for v3.
> > 
> > +    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"));
> cleanup?
Yup. Wait for v3.
> 
> > 
> > +    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);
> > +
> cleanup?
Not required. Since it will still work at boot time.
> 
> > 
> > +  return Status;
> > +}
> > --
> > 2.14.1
> > 

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

[Ard Biesheuvel]

On 13 November 2017 at 11:40, Supreeth Venkatesh
<supreeth.venkatesh@arm.com> wrote:
> On Mon, 2017-11-13 at 10:30 +0000, Ard Biesheuvel wrote:
>> On 25 October 2017 at 17:32, 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 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
>> > +++++++++++++++++++++
>> No need to separate the .inf and .c changes, so this patch can be
>> merged with the previous one.
> Thanks for more comments/feedback.
> Ok. Please wait for v3.
>>
>> >
>> >  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
>> > +**/
>> Please don't use lines > 80 characters.
> Ok.
>>
>> >
>> > +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
>> > +};
>> > +
>> Please move this below the definition of
>> MmCommunicationCommunicate(),
>> and drop the forward declaration.
> Ok.
>>
>> >
>> > +/**
>> > +  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.
>> What does 'convey into SMRAM' mean?
>>
>> >
>> > +  @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.
>> > +
>> CommSize is a pointer so it could be NULL but not zero. Or do you
>> mean
>> *CommSize?
>>
> CommSize is an optional parameter.

OK, please add OPTIONAL the prototype

> Yeah, if Commsize is used, then its
> value (*CommSize) will be zero, if there is no data. This is something
> the specification implicitly states.

OK


>> >
>> > +  @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
>> > +**/
>> STATIC ?
>>
> This implements an EFIAPI for the protocol MM_COMMUNICATION_PROTOCOL in
> MdePkg.Not sure why you want this function static?

Any function that is only callable via function pointers should be static.

>> >
>> > +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;
>> Here you return EFI_BAD_BUFFER_SIZE for a 0 sized buffer, while the
>> comment says is it returned when the buffer is too large.
>>
> Specification states this:
> "If the CommSize parameter is passed into the call, but the integer it
> points to, has a value of 0, then this must be updated to reflect the
> maximum size of the CommBuffer that the implementation can tolerate."
> Also, Return Values are
> EFI_INVALID_PARAMETER - The buffer was NULL.
> 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.
> See the function description above for more details
>
> So, it does not specify what should be the return value, when *CommSize
> is zero. It is open to interpretation between EFI_INVALID_PARAMETER and
> EFI_BAD_BUFFER_SIZE. I choose EFI_BAD_BUFFER_SIZE.
> Let me know what you think.
>

If the specification does not state that an error should be returned,
we should clarify that first.

>> >
>> > +    }
>> > +    //
>> > +    // 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? What happens if VirtualBase == NULL? Can we still proceed
>> without copying?
> Yup. This was Achin's comment as well. Please wait for v3.
>>
>> >
>> > +  // 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);
>> > +
>> > +  Status = CommunicateSmcArgs.Arg0;
>> > +  switch (Status) {
>> I think I mentioned this before, but please don't use Status for the
>> ARM_SMC_MM_xxx enum space. Use a separate, correctly typed variable
>> instead.
> Oh. your last email asks to replace EFI_SUCCESS with MM Specific RETURN
> SUCCESS. Status is still "UINTN" type variable for both MM and general
> UEFI status codes.
> Let me know if you feel strongly about it, I will have two different
> "UINTN" type variables.

Status should be EFI_STATUS not UINTN.

The ARM_SMC_MM_xxx symbolic constants are fundamentally a different
type, and you are relying on the fact that EFI_SUCCESS and
ARM_SMC_MM_RET_SUCCESS happen to be #defined to the same numeric
value. That is very bad coding style.

>>
>> >
>> > +  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:
>> > +  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;
>> > +}
>> > +
>> > +/**
>> > +  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));
>> Please use %r for reporting EFI_STATUS values.
>>
> Ok.
>> >
>> > +  }
>> > +
>> > +}
>> > +
>> > +/**
>> > +  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;
>> > +
>> Please drop the 0 assignmentsm since they are overridden immediately.
> Ok.
>>
>> >
>> > +  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_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"));
>> cleanup?
> Yup. Wait for v3.
>>
>> >
>> > +    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"));
>> cleanup?
>>
> This was Udit's comment as well. Wait for v3.
>> >
>> > +    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"));
>> cleanup?
> Yup. Wait for v3.
>>
>> >
>> > +    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);
>> > +
>> cleanup?
> Not required. Since it will still work at boot time.

If gBS->CreateEvent () fails in a RELEASE build, it will ignore the
ASSERT () and return the error code, which will result in the driver
to be unloaded. Without cleanup, that will leave protocol pointers
pointing into memory which has now been freed. Also, the buffer
allocation will not be freed either.

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

[Supreeth Venkatesh]

On Mon, 2017-11-13 at 11:48 +0000, Ard Biesheuvel wrote:
> On 13 November 2017 at 11:40, Supreeth Venkatesh
> <supreeth.venkatesh@arm.com> wrote:
> > 
> > On Mon, 2017-11-13 at 10:30 +0000, Ard Biesheuvel wrote:
> > > 
> > > On 25 October 2017 at 17:32, 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 defined in
> > > > http://infocenter.arm.com/help/topic/com.arm.doc.den0060a/DEN00
> > > > 60A_
> > > > 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
> > > > +++++++++++++++++++++
> > > No need to separate the .inf and .c changes, so this patch can be
> > > merged with the previous one.
> > Thanks for more comments/feedback.
> > Ok. Please wait for v3.
> > > 
> > > 
> > > > 
> > > > 
> > > >  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
> > > > +**/
> > > Please don't use lines > 80 characters.
> > Ok.
> > > 
> > > 
> > > > 
> > > > 
> > > > +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
> > > > +};
> > > > +
> > > Please move this below the definition of
> > > MmCommunicationCommunicate(),
> > > and drop the forward declaration.
> > Ok.
> > > 
> > > 
> > > > 
> > > > 
> > > > +/**
> > > > +  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.
> > > What does 'convey into SMRAM' mean?
> > > 
This is to mean Management Mode RAM.
There is a concept of System Management RAM which is separate from
Normal World UEFI.
> > > > 
> > > > 
> > > > +  @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.
> > > > +
> > > CommSize is a pointer so it could be NULL but not zero. Or do you
> > > mean
> > > *CommSize?
> > > 
> > CommSize is an optional parameter.
> OK, please add OPTIONAL the prototype
Ok.
> 
> > 
> > Yeah, if Commsize is used, then its
> > value (*CommSize) will be zero, if there is no data. This is
> > something
> > the specification implicitly states.
> OK
> 
> 
> > 
> > > 
> > > > 
> > > > 
> > > > +  @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
> > > > +**/
> > > STATIC ?
> > > 
> > This implements an EFIAPI for the protocol
> > MM_COMMUNICATION_PROTOCOL in
> > MdePkg.Not sure why you want this function static?
> Any function that is only callable via function pointers should be
> static.
> 
Ok.
> > 
> > > 
> > > > 
> > > > 
> > > > +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;
> > > Here you return EFI_BAD_BUFFER_SIZE for a 0 sized buffer, while
> > > the
> > > comment says is it returned when the buffer is too large.
> > > 
> > Specification states this:
> > "If the CommSize parameter is passed into the call, but the integer
> > it
> > points to, has a value of 0, then this must be updated to reflect
> > the
> > maximum size of the CommBuffer that the implementation can
> > tolerate."
> > Also, Return Values are
> > EFI_INVALID_PARAMETER - The buffer was NULL.
> > 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.
> > See the function description above for more details
> > 
> > So, it does not specify what should be the return value, when
> > *CommSize
> > is zero. It is open to interpretation between EFI_INVALID_PARAMETER
> > and
> > EFI_BAD_BUFFER_SIZE. I choose EFI_BAD_BUFFER_SIZE.
> > Let me know what you think.
> > 
> If the specification does not state that an error should be returned,
> we should clarify that first.
I have this in my to-do list, its an error condition (no doubt about
that), its just that the return code in the specification need to have
an updated statement to reflect this.
> 
> > 
> > > 
> > > > 
> > > > 
> > > > +    }
> > > > +    //
> > > > +    // 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? What happens if VirtualBase == NULL? Can we still proceed
> > > without copying?
> > Yup. This was Achin's comment as well. Please wait for v3.
> > > 
> > > 
> > > > 
> > > > 
> > > > +  // 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);
> > > > +
> > > > +  Status = CommunicateSmcArgs.Arg0;
> > > > +  switch (Status) {
> > > I think I mentioned this before, but please don't use Status for
> > > the
> > > ARM_SMC_MM_xxx enum space. Use a separate, correctly typed
> > > variable
> > > instead.
> > Oh. your last email asks to replace EFI_SUCCESS with MM Specific
> > RETURN
> > SUCCESS. Status is still "UINTN" type variable for both MM and
> > general
> > UEFI status codes.
> > Let me know if you feel strongly about it, I will have two
> > different
> > "UINTN" type variables.
> Status should be EFI_STATUS not UINTN.
> 
> The ARM_SMC_MM_xxx symbolic constants are fundamentally a different
> type, and you are relying on the fact that EFI_SUCCESS and
> ARM_SMC_MM_RET_SUCCESS happen to be #defined to the same numeric
> value. That is very bad coding style.
No. I am *not* relying on the fact ARM_SMC_MM_RET_SUCCESS and
EFI_SUCCESS are same. I am just relying on the fact that
EFI_STATUS and MM return Status happen to be "UINTN" type ultimately.
EFI_STATUS -> RETURN_STATUS -> UINTN (MdePkg/Include/Base.h).

However, I get that you are trying to say these can be implemented as
"enums" and hence the reason to have two different variables to convert
from one type to the other. I will change it.



> 
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > +  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:
> > > > +  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;
> > > > +}
> > > > +
> > > > +/**
> > > > +  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));
> > > Please use %r for reporting EFI_STATUS values.
> > > 
> > Ok.
> > > 
> > > > 
> > > > 
> > > > +  }
> > > > +
> > > > +}
> > > > +
> > > > +/**
> > > > +  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;
> > > > +
> > > Please drop the 0 assignmentsm since they are overridden
> > > immediately.
> > Ok.
> > > 
> > > 
> > > > 
> > > > 
> > > > +  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.PhysicalB
> > > > ase,
> > > > +                                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"));
> > > > +    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"));
> > > cleanup?
> > Yup. Wait for v3.
> > > 
> > > 
> > > > 
> > > > 
> > > > +    return EFI_INVALID_PARAMETER;
> > > > +  }
> > > > +
> > > > +  Status = gBS->AllocatePages (AllocateAddress,
> > > > +                               EfiRuntimeServicesData,
> > > > +                               EFI_SIZE_TO_PAGES
> > > > (mNsCommBuffMemRegion.Length),
> > > > +                               &mNsCommBuffMemRegion.PhysicalB
> > > > ase)
> > > > ;
> > > > +  if (EFI_ERROR (Status)) {
> > > > +    DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: Failed to
> > > > allocate MM-NS Buffer Memory Space\n"));
> > > cleanup?
> > > 
> > This was Udit's comment as well. Wait for v3.
> > > 
> > > > 
> > > > 
> > > > +    return EFI_INVALID_PARAMETER;
> > > > +  }
> > > > +
> > > > +  // Install the communication protocol
> > > > +  Status = gBS->InstallProtocolInterface
> > > > (&mMmCommunicateHandle,
> > > > +                                          &gEfiMmCommunication
> > > > Prot
> > > > ocolGuid,
> > > > +                                          EFI_NATIVE_INTERFACE
> > > > ,
> > > > +                                          &mMmCommunication);
> > > > +  if (EFI_ERROR(Status)) {
> > > > +    DEBUG ((DEBUG_ERROR, "MmCommunicationInitialize: Failed to
> > > > install MM communication protocol\n"));
> > > cleanup?
> > Yup. Wait for v3.
> > > 
> > > 
> > > > 
> > > > 
> > > > +    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);
> > > > +
> > > cleanup?
> > Not required. Since it will still work at boot time.
> If gBS->CreateEvent () fails in a RELEASE build, it will ignore the
> ASSERT () and return the error code, which will result in the driver
> to be unloaded. Without cleanup, that will leave protocol pointers
> pointing into memory which has now been freed. Also, the buffer
> allocation will not be freed either.
Ok. So the cleanup here will be to free the buffer and uninstall the
protocol. Is that what you had in mind?

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

[Ard Biesheuvel]

On 13 November 2017 at 12:15, Supreeth Venkatesh
<supreeth.venkatesh@arm.com> wrote:
> On Mon, 2017-11-13 at 11:48 +0000, Ard Biesheuvel wrote:
>> On 13 November 2017 at 11:40, Supreeth Venkatesh
>> <supreeth.venkatesh@arm.com> wrote:
>> >
>> > On Mon, 2017-11-13 at 10:30 +0000, Ard Biesheuvel wrote:
>> > >
>> > > On 25 October 2017 at 17:32, 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 defined in
>> > > > http://infocenter.arm.com/help/topic/com.arm.doc.den0060a/DEN00
>> > > > 60A_
>> > > > 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
>> > > > +++++++++++++++++++++
>> > > No need to separate the .inf and .c changes, so this patch can be
>> > > merged with the previous one.
>> > Thanks for more comments/feedback.
>> > Ok. Please wait for v3.
>> > >
>> > >
>> > > >
>> > > >
>> > > >  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
>> > > > +**/
>> > > Please don't use lines > 80 characters.
>> > Ok.
>> > >
>> > >
>> > > >
>> > > >
>> > > > +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
>> > > > +};
>> > > > +
>> > > Please move this below the definition of
>> > > MmCommunicationCommunicate(),
>> > > and drop the forward declaration.
>> > Ok.
>> > >
>> > >
>> > > >
>> > > >
>> > > > +/**
>> > > > +  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.
>> > > What does 'convey into SMRAM' mean?
>> > >
> This is to mean Management Mode RAM.
> There is a concept of System Management RAM which is separate from
> Normal World UEFI.

I know what SMRAM means. But what does the sentence 'A pointer to the
buffer to convey into SMRAM' mean?

>> > > >
>> > > >
>> > > > +  @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.
>> > > > +
>> > > CommSize is a pointer so it could be NULL but not zero. Or do you
>> > > mean
>> > > *CommSize?
>> > >
>> > CommSize is an optional parameter.
>> OK, please add OPTIONAL the prototype
> Ok.
>>
>> >
>> > Yeah, if Commsize is used, then its
>> > value (*CommSize) will be zero, if there is no data. This is
>> > something
>> > the specification implicitly states.
>> OK
>>
>>
>> >
>> > >
>> > > >
>> > > >
>> > > > +  @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
>> > > > +**/
>> > > STATIC ?
>> > >
>> > This implements an EFIAPI for the protocol
>> > MM_COMMUNICATION_PROTOCOL in
>> > MdePkg.Not sure why you want this function static?
>> Any function that is only callable via function pointers should be
>> static.
>>
> Ok.
>> >
>> > >
>> > > >
>> > > >
>> > > > +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;
>> > > Here you return EFI_BAD_BUFFER_SIZE for a 0 sized buffer, while
>> > > the
>> > > comment says is it returned when the buffer is too large.
>> > >
>> > Specification states this:
>> > "If the CommSize parameter is passed into the call, but the integer
>> > it
>> > points to, has a value of 0, then this must be updated to reflect
>> > the
>> > maximum size of the CommBuffer that the implementation can
>> > tolerate."
>> > Also, Return Values are
>> > EFI_INVALID_PARAMETER - The buffer was NULL.
>> > 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.
>> > See the function description above for more details
>> >
>> > So, it does not specify what should be the return value, when
>> > *CommSize
>> > is zero. It is open to interpretation between EFI_INVALID_PARAMETER
>> > and
>> > EFI_BAD_BUFFER_SIZE. I choose EFI_BAD_BUFFER_SIZE.
>> > Let me know what you think.
>> >
>> If the specification does not state that an error should be returned,
>> we should clarify that first.
> I have this in my to-do list, its an error condition (no doubt about
> that), its just that the return code in the specification need to have
> an updated statement to reflect this.

OK

>>
>> >
>> > >
>> > > >
>> > > >
>> > > > +    }
>> > > > +    //
>> > > > +    // 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? What happens if VirtualBase == NULL? Can we still proceed
>> > > without copying?
>> > Yup. This was Achin's comment as well. Please wait for v3.
>> > >
>> > >
>> > > >
>> > > >
>> > > > +  // 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);
>> > > > +
>> > > > +  Status = CommunicateSmcArgs.Arg0;
>> > > > +  switch (Status) {
>> > > I think I mentioned this before, but please don't use Status for
>> > > the
>> > > ARM_SMC_MM_xxx enum space. Use a separate, correctly typed
>> > > variable
>> > > instead.
>> > Oh. your last email asks to replace EFI_SUCCESS with MM Specific
>> > RETURN
>> > SUCCESS. Status is still "UINTN" type variable for both MM and
>> > general
>> > UEFI status codes.
>> > Let me know if you feel strongly about it, I will have two
>> > different
>> > "UINTN" type variables.
>> Status should be EFI_STATUS not UINTN.
>>
>> The ARM_SMC_MM_xxx symbolic constants are fundamentally a different
>> type, and you are relying on the fact that EFI_SUCCESS and
>> ARM_SMC_MM_RET_SUCCESS happen to be #defined to the same numeric
>> value. That is very bad coding style.
> No. I am *not* relying on the fact ARM_SMC_MM_RET_SUCCESS and
> EFI_SUCCESS are same. I am just relying on the fact that
> EFI_STATUS and MM return Status happen to be "UINTN" type ultimately.
> EFI_STATUS -> RETURN_STATUS -> UINTN (MdePkg/Include/Base.h).
>

Yes you are.

If Status == ARM_SMC_MM_RET_SUCCESS, you return 'Status' from the
function, where it means EFI_SUCCESS.

> However, I get that you are trying to say these can be implemented as
> "enums" and hence the reason to have two different variables to convert
> from one type to the other. I will change it.
>

I am simply saying that Status should only be used to hold EFI_xxx
constants, and that you should use a different variable to hold
ARM_SMC_MM_RET_xxx constants, and any conversion between the two
should involve if () or switch () statements.

>
>
>>
>> >
>> > >
>> > >
>> > > >
>> > > >
>> > > > +  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:
>> > > > +  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;
>> > > > +}
>> > > > +
>> > > > +/**
>> > > > +  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));
>> > > Please use %r for reporting EFI_STATUS values.
>> > >
>> > Ok.
>> > >
>> > > >
>> > > >
>> > > > +  }
>> > > > +
>> > > > +}
>> > > > +
>> > > > +/**
>> > > > +  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;
>> > > > +
>> > > Please drop the 0 assignmentsm since they are overridden
>> > > immediately.
>> > Ok.
>> > >
>> > >
>> > > >
>> > > >
>> > > > +  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.PhysicalB
>> > > > ase,
>> > > > +                                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"));
>> > > > +    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"));
>> > > cleanup?
>> > Yup. Wait for v3.
>> > >
>> > >
>> > > >
>> > > >
>> > > > +    return EFI_INVALID_PARAMETER;
>> > > > +  }
>> > > > +
>> > > > +  Status = gBS->AllocatePages (AllocateAddress,
>> > > > +                               EfiRuntimeServicesData,
>> > > > +                               EFI_SIZE_TO_PAGES
>> > > > (mNsCommBuffMemRegion.Length),
>> > > > +                               &mNsCommBuffMemRegion.PhysicalB
>> > > > ase)
>> > > > ;
>> > > > +  if (EFI_ERROR (Status)) {
>> > > > +    DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: Failed to
>> > > > allocate MM-NS Buffer Memory Space\n"));
>> > > cleanup?
>> > >
>> > This was Udit's comment as well. Wait for v3.
>> > >
>> > > >
>> > > >
>> > > > +    return EFI_INVALID_PARAMETER;
>> > > > +  }
>> > > > +
>> > > > +  // Install the communication protocol
>> > > > +  Status = gBS->InstallProtocolInterface
>> > > > (&mMmCommunicateHandle,
>> > > > +                                          &gEfiMmCommunication
>> > > > Prot
>> > > > ocolGuid,
>> > > > +                                          EFI_NATIVE_INTERFACE
>> > > > ,
>> > > > +                                          &mMmCommunication);
>> > > > +  if (EFI_ERROR(Status)) {
>> > > > +    DEBUG ((DEBUG_ERROR, "MmCommunicationInitialize: Failed to
>> > > > install MM communication protocol\n"));
>> > > cleanup?
>> > Yup. Wait for v3.
>> > >
>> > >
>> > > >
>> > > >
>> > > > +    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);
>> > > > +
>> > > cleanup?
>> > Not required. Since it will still work at boot time.
>> If gBS->CreateEvent () fails in a RELEASE build, it will ignore the
>> ASSERT () and return the error code, which will result in the driver
>> to be unloaded. Without cleanup, that will leave protocol pointers
>> pointing into memory which has now been freed. Also, the buffer
>> allocation will not be freed either.
> Ok. So the cleanup here will be to free the buffer and uninstall the
> protocol. Is that what you had in mind?

Yes.

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

[Supreeth Venkatesh]

On Mon, 2017-11-13 at 12:19 +0000, Ard Biesheuvel wrote:
> On 13 November 2017 at 12:15, Supreeth Venkatesh
> <supreeth.venkatesh@arm.com> wrote:
> > 
> > On Mon, 2017-11-13 at 11:48 +0000, Ard Biesheuvel wrote:
> > > 
> > > On 13 November 2017 at 11:40, Supreeth Venkatesh
> > > <supreeth.venkatesh@arm.com> wrote:
> > > > 
> > > > 
> > > > On Mon, 2017-11-13 at 10:30 +0000, Ard Biesheuvel wrote:
> > > > > 
> > > > > 
> > > > > On 25 October 2017 at 17:32, 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 defined in
> > > > > > http://infocenter.arm.com/help/topic/com.arm.doc.den0060a/D
> > > > > > EN00
> > > > > > 60A_
> > > > > > 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.c
> > > > > > om>
> > > > > > ---
> > > > > >  .../Drivers/MmCommunicationDxe/MmCommunication.c   | 339
> > > > > > +++++++++++++++++++++
> > > > > No need to separate the .inf and .c changes, so this patch
> > > > > can be
> > > > > merged with the previous one.
> > > > Thanks for more comments/feedback.
> > > > Ok. Please wait for v3.
> > > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > >  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
> > > > > > +**/
> > > > > Please don't use lines > 80 characters.
> > > > Ok.
> > > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > +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
> > > > > > +};
> > > > > > +
> > > > > Please move this below the definition of
> > > > > MmCommunicationCommunicate(),
> > > > > and drop the forward declaration.
> > > > Ok.
> > > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > +/**
> > > > > > +  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.
> > > > > What does 'convey into SMRAM' mean?
> > > > > 
> > This is to mean Management Mode RAM.
> > There is a concept of System Management RAM which is separate from
> > Normal World UEFI.
> I know what SMRAM means. But what does the sentence 'A pointer to the
> buffer to convey into SMRAM' mean?
> 
So, the contents of this buffer will be conveyed into the space which
is set apart for MM foundation to run. (Which is basically portion of
RAM used by Management Mode foundation.) In AARCH64 case, Management
Mode is run in S-EL0, hence a portion of the Memory which is separated
by trust zone and allowed for use in Management Mode firmware. 
Management Mode Memory is isolated from Normal World Memory and the
mapping is platform dependent and done in privileged firmware. 
if you need more information, we can discuss this off-line. 
> > 
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > +  @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.
> > > > > > +
> > > > > CommSize is a pointer so it could be NULL but not zero. Or do
> > > > > you
> > > > > mean
> > > > > *CommSize?
> > > > > 
> > > > CommSize is an optional parameter.
> > > OK, please add OPTIONAL the prototype
> > Ok.
> > > 
> > > 
> > > > 
> > > > 
> > > > Yeah, if Commsize is used, then its
> > > > value (*CommSize) will be zero, if there is no data. This is
> > > > something
> > > > the specification implicitly states.
> > > OK
> > > 
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > +  @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
> > > > > > +**/
> > > > > STATIC ?
> > > > > 
> > > > This implements an EFIAPI for the protocol
> > > > MM_COMMUNICATION_PROTOCOL in
> > > > MdePkg.Not sure why you want this function static?
> > > Any function that is only callable via function pointers should
> > > be
> > > static.
> > > 
> > Ok.
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > +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;
> > > > > Here you return EFI_BAD_BUFFER_SIZE for a 0 sized buffer,
> > > > > while
> > > > > the
> > > > > comment says is it returned when the buffer is too large.
> > > > > 
> > > > Specification states this:
> > > > "If the CommSize parameter is passed into the call, but the
> > > > integer
> > > > it
> > > > points to, has a value of 0, then this must be updated to
> > > > reflect
> > > > the
> > > > maximum size of the CommBuffer that the implementation can
> > > > tolerate."
> > > > Also, Return Values are
> > > > EFI_INVALID_PARAMETER - The buffer was NULL.
> > > > 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.
> > > > See the function description above for more details
> > > > 
> > > > So, it does not specify what should be the return value, when
> > > > *CommSize
> > > > is zero. It is open to interpretation between
> > > > EFI_INVALID_PARAMETER
> > > > and
> > > > EFI_BAD_BUFFER_SIZE. I choose EFI_BAD_BUFFER_SIZE.
> > > > Let me know what you think.
> > > > 
> > > If the specification does not state that an error should be
> > > returned,
> > > we should clarify that first.
> > I have this in my to-do list, its an error condition (no doubt
> > about
> > that), its just that the return code in the specification need to
> > have
> > an updated statement to reflect this.
> OK
> 
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > +    }
> > > > > > +    //
> > > > > > +    // 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? What happens if VirtualBase == NULL? Can we still
> > > > > proceed
> > > > > without copying?
> > > > Yup. This was Achin's comment as well. Please wait for v3.
> > > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > +  // 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);
> > > > > > +
> > > > > > +  Status = CommunicateSmcArgs.Arg0;
> > > > > > +  switch (Status) {
> > > > > I think I mentioned this before, but please don't use Status
> > > > > for
> > > > > the
> > > > > ARM_SMC_MM_xxx enum space. Use a separate, correctly typed
> > > > > variable
> > > > > instead.
> > > > Oh. your last email asks to replace EFI_SUCCESS with MM
> > > > Specific
> > > > RETURN
> > > > SUCCESS. Status is still "UINTN" type variable for both MM and
> > > > general
> > > > UEFI status codes.
> > > > Let me know if you feel strongly about it, I will have two
> > > > different
> > > > "UINTN" type variables.
> > > Status should be EFI_STATUS not UINTN.
> > > 
> > > The ARM_SMC_MM_xxx symbolic constants are fundamentally a
> > > different
> > > type, and you are relying on the fact that EFI_SUCCESS and
> > > ARM_SMC_MM_RET_SUCCESS happen to be #defined to the same numeric
> > > value. That is very bad coding style.
> > No. I am *not* relying on the fact ARM_SMC_MM_RET_SUCCESS and
> > EFI_SUCCESS are same. I am just relying on the fact that
> > EFI_STATUS and MM return Status happen to be "UINTN" type
> > ultimately.
> > EFI_STATUS -> RETURN_STATUS -> UINTN (MdePkg/Include/Base.h).
> > 
> Yes you are.
> 
> If Status == ARM_SMC_MM_RET_SUCCESS, you return 'Status' from the
> function, where it means EFI_SUCCESS.
> 
> > 
> > However, I get that you are trying to say these can be implemented
> > as
> > "enums" and hence the reason to have two different variables to
> > convert
> > from one type to the other. I will change it.
> > 
> I am simply saying that Status should only be used to hold EFI_xxx
> constants, and that you should use a different variable to hold
> ARM_SMC_MM_RET_xxx constants, and any conversion between the two
> should involve if () or switch () statements.
Sorry, you missed the switch statement for the conversion below, the
part that needs change is to have  EFI_Status variable (to hold
converted Status) and UINTN MmStatus or something similar to hold MM
Return Status, since both of them are technically separate status. Is
this what you mean?
> 
> > 
> > 
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > +  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:
> > > > > > +  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;
> > > > > > +}
> > > > > > +
> > > > > > +/**
> > > > > > +  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));
> > > > > Please use %r for reporting EFI_STATUS values.
> > > > > 
> > > > Ok.
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > +  }
> > > > > > +
> > > > > > +}
> > > > > > +
> > > > > > +/**
> > > > > > +  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;
> > > > > > +
> > > > > Please drop the 0 assignmentsm since they are overridden
> > > > > immediately.
> > > > Ok.
> > > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > +  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.Physi
> > > > > > calB
> > > > > > ase,
> > > > > > +                                mNsCommBuffMemRegion.Lengt
> > > > > > h,
> > > > > > +                                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"));
> > > > > > +    return EFI_INVALID_PARAMETER;
> > > > > > +  }
> > > > > > +
> > > > > > +  Status = gDS-
> > > > > > > 
> > > > > > > 
> > > > > > > SetMemorySpaceAttributes(mNsCommBuffMemRegion.PhysicalBas
> > > > > > > e,
> > > > > > +                                         mNsCommBuffMemReg
> > > > > > ion.
> > > > > > 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"));
> > > > > cleanup?
> > > > Yup. Wait for v3.
> > > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > +    return EFI_INVALID_PARAMETER;
> > > > > > +  }
> > > > > > +
> > > > > > +  Status = gBS->AllocatePages (AllocateAddress,
> > > > > > +                               EfiRuntimeServicesData,
> > > > > > +                               EFI_SIZE_TO_PAGES
> > > > > > (mNsCommBuffMemRegion.Length),
> > > > > > +                               &mNsCommBuffMemRegion.Physi
> > > > > > calB
> > > > > > ase)
> > > > > > ;
> > > > > > +  if (EFI_ERROR (Status)) {
> > > > > > +    DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: Failed
> > > > > > to
> > > > > > allocate MM-NS Buffer Memory Space\n"));
> > > > > cleanup?
> > > > > 
> > > > This was Udit's comment as well. Wait for v3.
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > +    return EFI_INVALID_PARAMETER;
> > > > > > +  }
> > > > > > +
> > > > > > +  // Install the communication protocol
> > > > > > +  Status = gBS->InstallProtocolInterface
> > > > > > (&mMmCommunicateHandle,
> > > > > > +                                          &gEfiMmCommunica
> > > > > > tion
> > > > > > Prot
> > > > > > ocolGuid,
> > > > > > +                                          EFI_NATIVE_INTER
> > > > > > FACE
> > > > > > ,
> > > > > > +                                          &mMmCommunicatio
> > > > > > n);
> > > > > > +  if (EFI_ERROR(Status)) {
> > > > > > +    DEBUG ((DEBUG_ERROR, "MmCommunicationInitialize:
> > > > > > Failed to
> > > > > > install MM communication protocol\n"));
> > > > > cleanup?
> > > > Yup. Wait for v3.
> > > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > +    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);
> > > > > > +
> > > > > cleanup?
> > > > Not required. Since it will still work at boot time.
> > > If gBS->CreateEvent () fails in a RELEASE build, it will ignore
> > > the
> > > ASSERT () and return the error code, which will result in the
> > > driver
> > > to be unloaded. Without cleanup, that will leave protocol
> > > pointers
> > > pointing into memory which has now been freed. Also, the buffer
> > > allocation will not be freed either.
> > Ok. So the cleanup here will be to free the buffer and uninstall
> > the
> > protocol. Is that what you had in mind?
> Yes.

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

[Ard Biesheuvel]

On 13 November 2017 at 12:42, Supreeth Venkatesh
<supreeth.venkatesh@arm.com> wrote:
> On Mon, 2017-11-13 at 12:19 +0000, Ard Biesheuvel wrote:
>> On 13 November 2017 at 12:15, Supreeth Venkatesh
>> <supreeth.venkatesh@arm.com> wrote:
>> >
>> > On Mon, 2017-11-13 at 11:48 +0000, Ard Biesheuvel wrote:
>> > >
>> > > On 13 November 2017 at 11:40, Supreeth Venkatesh
>> > > <supreeth.venkatesh@arm.com> wrote:
>> > > >
>> > > >
>> > > > On Mon, 2017-11-13 at 10:30 +0000, Ard Biesheuvel wrote:
>> > > > >
>> > > > >
>> > > > > On 25 October 2017 at 17:32, 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 defined in
>> > > > > > http://infocenter.arm.com/help/topic/com.arm.doc.den0060a/D
>> > > > > > EN00
>> > > > > > 60A_
>> > > > > > 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.c
>> > > > > > om>
>> > > > > > ---
>> > > > > >  .../Drivers/MmCommunicationDxe/MmCommunication.c   | 339
>> > > > > > +++++++++++++++++++++
>> > > > > No need to separate the .inf and .c changes, so this patch
>> > > > > can be
>> > > > > merged with the previous one.
>> > > > Thanks for more comments/feedback.
>> > > > Ok. Please wait for v3.
>> > > > >
>> > > > >
>> > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >  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
>> > > > > > +**/
>> > > > > Please don't use lines > 80 characters.
>> > > > Ok.
>> > > > >
>> > > > >
>> > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > > +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
>> > > > > > +};
>> > > > > > +
>> > > > > Please move this below the definition of
>> > > > > MmCommunicationCommunicate(),
>> > > > > and drop the forward declaration.
>> > > > Ok.
>> > > > >
>> > > > >
>> > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > > +/**
>> > > > > > +  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.
>> > > > > What does 'convey into SMRAM' mean?
>> > > > >
>> > This is to mean Management Mode RAM.
>> > There is a concept of System Management RAM which is separate from
>> > Normal World UEFI.
>> I know what SMRAM means. But what does the sentence 'A pointer to the
>> buffer to convey into SMRAM' mean?
>>
> So, the contents of this buffer will be conveyed into the space which
> is set apart for MM foundation to run. (Which is basically portion of
> RAM used by Management Mode foundation.) In AARCH64 case, Management
> Mode is run in S-EL0, hence a portion of the Memory which is separated
> by trust zone and allowed for use in Management Mode firmware.
> Management Mode Memory is isolated from Normal World Memory and the
> mapping is platform dependent and done in privileged firmware.
> if you need more information, we can discuss this off-line.
>> >
>> > >
>> > > >
>> > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > > +  @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.
>> > > > > > +
>> > > > > CommSize is a pointer so it could be NULL but not zero. Or do
>> > > > > you
>> > > > > mean
>> > > > > *CommSize?
>> > > > >
>> > > > CommSize is an optional parameter.
>> > > OK, please add OPTIONAL the prototype
>> > Ok.
>> > >
>> > >
>> > > >
>> > > >
>> > > > Yeah, if Commsize is used, then its
>> > > > value (*CommSize) will be zero, if there is no data. This is
>> > > > something
>> > > > the specification implicitly states.
>> > > OK
>> > >
>> > >
>> > > >
>> > > >
>> > > > >
>> > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > > +  @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
>> > > > > > +**/
>> > > > > STATIC ?
>> > > > >
>> > > > This implements an EFIAPI for the protocol
>> > > > MM_COMMUNICATION_PROTOCOL in
>> > > > MdePkg.Not sure why you want this function static?
>> > > Any function that is only callable via function pointers should
>> > > be
>> > > static.
>> > >
>> > Ok.
>> > >
>> > > >
>> > > >
>> > > > >
>> > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > > +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;
>> > > > > Here you return EFI_BAD_BUFFER_SIZE for a 0 sized buffer,
>> > > > > while
>> > > > > the
>> > > > > comment says is it returned when the buffer is too large.
>> > > > >
>> > > > Specification states this:
>> > > > "If the CommSize parameter is passed into the call, but the
>> > > > integer
>> > > > it
>> > > > points to, has a value of 0, then this must be updated to
>> > > > reflect
>> > > > the
>> > > > maximum size of the CommBuffer that the implementation can
>> > > > tolerate."
>> > > > Also, Return Values are
>> > > > EFI_INVALID_PARAMETER - The buffer was NULL.
>> > > > 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.
>> > > > See the function description above for more details
>> > > >
>> > > > So, it does not specify what should be the return value, when
>> > > > *CommSize
>> > > > is zero. It is open to interpretation between
>> > > > EFI_INVALID_PARAMETER
>> > > > and
>> > > > EFI_BAD_BUFFER_SIZE. I choose EFI_BAD_BUFFER_SIZE.
>> > > > Let me know what you think.
>> > > >
>> > > If the specification does not state that an error should be
>> > > returned,
>> > > we should clarify that first.
>> > I have this in my to-do list, its an error condition (no doubt
>> > about
>> > that), its just that the return code in the specification need to
>> > have
>> > an updated statement to reflect this.
>> OK
>>
>> >
>> > >
>> > >
>> > > >
>> > > >
>> > > > >
>> > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > > +    }
>> > > > > > +    //
>> > > > > > +    // 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? What happens if VirtualBase == NULL? Can we still
>> > > > > proceed
>> > > > > without copying?
>> > > > Yup. This was Achin's comment as well. Please wait for v3.
>> > > > >
>> > > > >
>> > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > > +  // 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);
>> > > > > > +
>> > > > > > +  Status = CommunicateSmcArgs.Arg0;
>> > > > > > +  switch (Status) {
>> > > > > I think I mentioned this before, but please don't use Status
>> > > > > for
>> > > > > the
>> > > > > ARM_SMC_MM_xxx enum space. Use a separate, correctly typed
>> > > > > variable
>> > > > > instead.
>> > > > Oh. your last email asks to replace EFI_SUCCESS with MM
>> > > > Specific
>> > > > RETURN
>> > > > SUCCESS. Status is still "UINTN" type variable for both MM and
>> > > > general
>> > > > UEFI status codes.
>> > > > Let me know if you feel strongly about it, I will have two
>> > > > different
>> > > > "UINTN" type variables.
>> > > Status should be EFI_STATUS not UINTN.
>> > >
>> > > The ARM_SMC_MM_xxx symbolic constants are fundamentally a
>> > > different
>> > > type, and you are relying on the fact that EFI_SUCCESS and
>> > > ARM_SMC_MM_RET_SUCCESS happen to be #defined to the same numeric
>> > > value. That is very bad coding style.
>> > No. I am *not* relying on the fact ARM_SMC_MM_RET_SUCCESS and
>> > EFI_SUCCESS are same. I am just relying on the fact that
>> > EFI_STATUS and MM return Status happen to be "UINTN" type
>> > ultimately.
>> > EFI_STATUS -> RETURN_STATUS -> UINTN (MdePkg/Include/Base.h).
>> >
>> Yes you are.
>>
>> If Status == ARM_SMC_MM_RET_SUCCESS, you return 'Status' from the
>> function, where it means EFI_SUCCESS.
>>
>> >
>> > However, I get that you are trying to say these can be implemented
>> > as
>> > "enums" and hence the reason to have two different variables to
>> > convert
>> > from one type to the other. I will change it.
>> >
>> I am simply saying that Status should only be used to hold EFI_xxx
>> constants, and that you should use a different variable to hold
>> ARM_SMC_MM_RET_xxx constants, and any conversion between the two
>> should involve if () or switch () statements.
> Sorry, you missed the switch statement for the conversion below, the
> part that needs change is to have  EFI_Status variable (to hold
> converted Status) and UINTN MmStatus or something similar to hold MM
> Return Status, since both of them are technically separate status. Is
> this what you mean?


Your code does

EFI_STATUS                  Status;
...
Status = CommunicateSmcArgs.Arg0;
switch (Status) {
case ARM_SMC_MM_RET_SUCCESS:
...
break;
...
return Status;

so it ultimately returns the value of Arg0 directly from the function.


Instead, you could do

switch (CommunicateSmcArgs.Arg0){
...
Status = EFI_xxx

and it would be fine.




>>
>> >
>> >
>> >
>> > >
>> > >
>> > > >
>> > > >
>> > > > >
>> > > > >
>> > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > > +  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:
>> > > > > > +  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;
>> > > > > > +}
>> > > > > > +
>> > > > > > +/**
>> > > > > > +  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));
>> > > > > Please use %r for reporting EFI_STATUS values.
>> > > > >
>> > > > Ok.
>> > > > >
>> > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > > +  }
>> > > > > > +
>> > > > > > +}
>> > > > > > +
>> > > > > > +/**
>> > > > > > +  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;
>> > > > > > +
>> > > > > Please drop the 0 assignmentsm since they are overridden
>> > > > > immediately.
>> > > > Ok.
>> > > > >
>> > > > >
>> > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > > +  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.Physi
>> > > > > > calB
>> > > > > > ase,
>> > > > > > +                                mNsCommBuffMemRegion.Lengt
>> > > > > > h,
>> > > > > > +                                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"));
>> > > > > > +    return EFI_INVALID_PARAMETER;
>> > > > > > +  }
>> > > > > > +
>> > > > > > +  Status = gDS-
>> > > > > > >
>> > > > > > >
>> > > > > > > SetMemorySpaceAttributes(mNsCommBuffMemRegion.PhysicalBas
>> > > > > > > e,
>> > > > > > +                                         mNsCommBuffMemReg
>> > > > > > ion.
>> > > > > > 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"));
>> > > > > cleanup?
>> > > > Yup. Wait for v3.
>> > > > >
>> > > > >
>> > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > > +    return EFI_INVALID_PARAMETER;
>> > > > > > +  }
>> > > > > > +
>> > > > > > +  Status = gBS->AllocatePages (AllocateAddress,
>> > > > > > +                               EfiRuntimeServicesData,
>> > > > > > +                               EFI_SIZE_TO_PAGES
>> > > > > > (mNsCommBuffMemRegion.Length),
>> > > > > > +                               &mNsCommBuffMemRegion.Physi
>> > > > > > calB
>> > > > > > ase)
>> > > > > > ;
>> > > > > > +  if (EFI_ERROR (Status)) {
>> > > > > > +    DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: Failed
>> > > > > > to
>> > > > > > allocate MM-NS Buffer Memory Space\n"));
>> > > > > cleanup?
>> > > > >
>> > > > This was Udit's comment as well. Wait for v3.
>> > > > >
>> > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > > +    return EFI_INVALID_PARAMETER;
>> > > > > > +  }
>> > > > > > +
>> > > > > > +  // Install the communication protocol
>> > > > > > +  Status = gBS->InstallProtocolInterface
>> > > > > > (&mMmCommunicateHandle,
>> > > > > > +                                          &gEfiMmCommunica
>> > > > > > tion
>> > > > > > Prot
>> > > > > > ocolGuid,
>> > > > > > +                                          EFI_NATIVE_INTER
>> > > > > > FACE
>> > > > > > ,
>> > > > > > +                                          &mMmCommunicatio
>> > > > > > n);
>> > > > > > +  if (EFI_ERROR(Status)) {
>> > > > > > +    DEBUG ((DEBUG_ERROR, "MmCommunicationInitialize:
>> > > > > > Failed to
>> > > > > > install MM communication protocol\n"));
>> > > > > cleanup?
>> > > > Yup. Wait for v3.
>> > > > >
>> > > > >
>> > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > > +    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);
>> > > > > > +
>> > > > > cleanup?
>> > > > Not required. Since it will still work at boot time.
>> > > If gBS->CreateEvent () fails in a RELEASE build, it will ignore
>> > > the
>> > > ASSERT () and return the error code, which will result in the
>> > > driver
>> > > to be unloaded. Without cleanup, that will leave protocol
>> > > pointers
>> > > pointing into memory which has now been freed. Also, the buffer
>> > > allocation will not be freed either.
>> > Ok. So the cleanup here will be to free the buffer and uninstall
>> > the
>> > protocol. Is that what you had in mind?
>> Yes.

[PATCH v2 3/3] ArmPkg/Drivers: Add ArmMmCommunication Driver information file.

[Supreeth Venkatesh]

This patch 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 <achin.gupta@arm.com>
Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
---
 .../Drivers/MmCommunicationDxe/MmCommunication.inf | 50 ++++++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100644 ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf

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
+#  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
+
+[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