[PATCH 0/6] ArmPkg related changes for StandaloneMM package

[PATCH 0/6] ArmPkg related changes for StandaloneMM package

[Supreeth Venkatesh]

The following patch series adds support for Management Mode related
changes for aarch64 based platforms.

Note: This patch series needs to be applied after applying
Standalone Management Mode core interface support for aarch64
platforms[1]

[1] - https://lists.01.org/pipermail/edk2-devel/2018-July/026778.html

Supreeth Venkatesh (6):
  ArmPkg: Add PCDs needed for MM communication driver.
  ArmPkg/Drivers: Add EFI_MM_COMMUNICATION_PROTOCOL DXE driver.
  ArmPkg/Include: Add MM interface SVC return codes.
  ArmPkg/ArmMmuLib: Add MMU Library suitable for use in S-EL0.
  ArmPkg/ArmMmuLib: Add MMU library inf file suitable for use in S-EL0.
  ArmPkg: Extra action to update permissions for S-ELO MM Image.

 ArmPkg/ArmPkg.dec                                  |   3 +
 .../Drivers/MmCommunicationDxe/MmCommunication.c   | 408 +++++++++++++++++++++
 .../Drivers/MmCommunicationDxe/MmCommunication.inf |  50 +++
 ArmPkg/Include/IndustryStandard/ArmMmSvc.h         |   9 +-
 .../ArmMmuLib/AArch64/ArmMmuStandaloneMmCoreLib.c  | 195 ++++++++++
 .../ArmMmuLib/ArmMmuStandaloneMmCoreLib.inf        |  37 ++
 .../DebugPeCoffExtraActionLib.c                    | 185 +++++++++-
 .../DebugPeCoffExtraActionLib.inf                  |   7 +
 8 files changed, 882 insertions(+), 12 deletions(-)
 create mode 100644 ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
 create mode 100644 ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf
 create mode 100644 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuStandaloneMmCoreLib.c
 create mode 100644 ArmPkg/Library/ArmMmuLib/ArmMmuStandaloneMmCoreLib.inf

-- 
2.7.4

[PATCH 1/6] 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>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/ArmPkg.dec | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
index 3aa229f..ce108f2 100644
--- a/ArmPkg/ArmPkg.dec
+++ b/ArmPkg/ArmPkg.dec
@@ -239,6 +239,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.7.4

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

[Leif Lindholm]

On Tue, Jul 03, 2018 at 03:25:10PM +0530, Supreeth Venkatesh 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.

Please add a cover-letter when sending more than individual (or
completely trivial) patches. Is this set intended for the master
branch, and how would I exercise this new code?

> 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>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPkg/ArmPkg.dec | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
> index 3aa229f..ce108f2 100644
> --- a/ArmPkg/ArmPkg.dec
> +++ b/ArmPkg/ArmPkg.dec
> @@ -239,6 +239,9 @@

Please follow the instructions in
https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers
when submitting patches.

Here, following step 9 would have given us the section in the .dec
file without having to cross-reference the existing source.

Regards,

Leif

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

[PATCH 2/6] 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 DEN0060A_ARM_MM_Interface_Specification.pdf
to communicate with the standalone MM environment in the secure world.

This patch also adds the MM Communication driver (.inf) file to
define entry point for this driver and other compile
related information the driver needs.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Achin Gupta <achin.gupta@arm.com>
Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 .../Drivers/MmCommunicationDxe/MmCommunication.c   | 408 +++++++++++++++++++++
 .../Drivers/MmCommunicationDxe/MmCommunication.inf |  50 +++
 2 files changed, 458 insertions(+)
 create mode 100644 ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
 create mode 100644 ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf

diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
new file mode 100644
index 0000000..8ba1790
--- /dev/null
+++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
@@ -0,0 +1,408 @@
+/** @file
+
+  Copyright (c) 2016-2018, 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>
+
+#define MM_MAJOR_VER_MASK        0xFFFF0000
+#define MM_MINOR_VER_MASK        0x0000FFFF
+#define MM_MAJOR_VER_SHIFT       16
+
+const UINT32 MM_MAJOR_VER = 1;
+const UINT32 MM_MINOR_VER = 0;
+
+//
+// Address, Length of the pre-allocated buffer for communication with the secure
+// world.
+//
+STATIC ARM_MEMORY_REGION_DESCRIPTOR  mNsCommBuffMemRegion;
+
+// Notification event when virtual address map is set.
+STATIC EFI_EVENT  mSetVirtualAddressMapEvent;
+
+//
+// Handle to install the MM Communication Protocol
+//
+STATIC EFI_HANDLE  mMmCommunicateHandle;
+
+/**
+  Communicates with a registered handler.
+
+  This function provides an interface to send and receive messages to the
+  Standalone MM environment on behalf of UEFI services.  This function is part
+  of the MM Communication Protocol that may be called in physical mode prior to
+  SetVirtualAddressMap() and in virtual mode after SetVirtualAddressMap().
+
+  @param[in]      This                The EFI_MM_COMMUNICATION_PROTOCOL
+                                      instance.
+  @param[in, out] CommBuffer          A pointer to the buffer to convey
+                                      into MMRAM.
+  @param[in, out] CommSize            The size of the data buffer being
+                                      passed in. This is optional.
+
+  @retval EFI_SUCCESS                 The message was successfully posted.
+  @retval EFI_INVALID_PARAMETER       The CommBuffer was NULL.
+  @retval EFI_BAD_BUFFER_SIZE         The buffer size is incorrect for the MM
+                                      implementation. If this error is
+                                      returned, the MessageLength field in
+                                      the CommBuffer header or the integer
+                                      pointed by CommSize are updated to reflect
+                                      the maximum payload size the
+                                      implementation can accommodate.
+  @retval EFI_ACCESS_DENIED           The CommunicateBuffer parameter
+                                      or CommSize parameter, if not omitted,
+                                      are in address range that cannot be
+                                      accessed by the MM environment
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+MmCommunicationCommunicate (
+  IN CONST EFI_MM_COMMUNICATION_PROTOCOL  *This,
+  IN OUT VOID                             *CommBuffer,
+  IN OUT UINTN                            *CommSize OPTIONAL
+  )
+{
+  EFI_MM_COMMUNICATE_HEADER   *CommunicateHeader;
+  ARM_SMC_ARGS                CommunicateSmcArgs;
+  EFI_STATUS                  Status;
+  UINTN                       BufferSize;
+
+  Status = EFI_ACCESS_DENIED;
+  BufferSize = 0;
+
+  ZeroMem (&CommunicateSmcArgs, sizeof (ARM_SMC_ARGS));
+
+  //
+  // Check parameters
+  //
+  if (CommBuffer == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  CommunicateHeader = CommBuffer;
+  // CommBuffer is a mandatory parameter. Hence, Rely on
+  // MessageLength + Header to ascertain the
+  // total size of the communication payload rather than
+  // rely on optional CommSize parameter
+  BufferSize = CommunicateHeader->MessageLength +
+               sizeof (CommunicateHeader->HeaderGuid) +
+               sizeof (CommunicateHeader->MessageLength);
+
+  // If the length of the CommBuffer is 0 then return the expected length.
+  if (CommSize) {
+    // This case can be used by the consumer of this driver to find out the
+    // max size that can be used for allocating CommBuffer.
+    if ((*CommSize == 0) ||
+        (*CommSize > mNsCommBuffMemRegion.Length)) {
+      *CommSize = mNsCommBuffMemRegion.Length;
+      return EFI_BAD_BUFFER_SIZE;
+    }
+    //
+    // CommSize must match MessageLength + sizeof (EFI_MM_COMMUNICATE_HEADER);
+    //
+    if (*CommSize != BufferSize) {
+        return EFI_INVALID_PARAMETER;
+    }
+  }
+
+  //
+  // 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;
+
+  // Copy Communication Payload
+  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);
+
+  switch (CommunicateSmcArgs.Arg0) {
+  case ARM_SMC_MM_RET_SUCCESS:
+    ZeroMem (CommBuffer, BufferSize);
+    // On exit, the size of data being returned is inferred from
+    // MessageLength + Header.
+    CommunicateHeader = (EFI_MM_COMMUNICATE_HEADER *)mNsCommBuffMemRegion.VirtualBase;
+    BufferSize = CommunicateHeader->MessageLength +
+                 sizeof (CommunicateHeader->HeaderGuid) +
+                 sizeof (CommunicateHeader->MessageLength);
+
+    // Note: Very important to ensure that the consumer of this driver
+    // has allocated CommBuffer sufficiently so that the return data
+    // can be copied. Otherwise, this will lead to buffer overflow.
+    // Assumption: CommBuffer = malloc (mNsCommBuffMemRegion.Length)
+    // This guidance should be in the PI specification. TODO: ECR.
+    CopyMem (CommBuffer,
+             (const VOID *)mNsCommBuffMemRegion.VirtualBase,
+             BufferSize);
+    Status = EFI_SUCCESS;
+    break;
+
+  case ARM_SMC_MM_RET_INVALID_PARAMS:
+    Status = EFI_INVALID_PARAMETER;
+    break;
+
+  case ARM_SMC_MM_RET_DENIED:
+    Status = EFI_ACCESS_DENIED;
+    break;
+
+  case ARM_SMC_MM_RET_NO_MEMORY:
+    // Unexpected error since the CommSize was checked for zero length
+    // prior to issuing the SMC
+  default:
+    Status = EFI_ACCESS_DENIED;
+    ASSERT (0);
+  }
+
+  return Status;
+}
+
+//
+// MM Communication Protocol instance
+//
+EFI_MM_COMMUNICATION_PROTOCOL  mMmCommunication = {
+  MmCommunicationCommunicate
+};
+
+/**
+  Notification callback on SetVirtualAddressMap event.
+
+  This function notifies the MM communication protocol interface on
+  SetVirtualAddressMap event and converts pointers used in this driver
+  from physical to virtual address.
+
+  @param  Event          SetVirtualAddressMap event.
+  @param  Context        A context when the SetVirtualAddressMap triggered.
+
+  @retval EFI_SUCCESS    The function executed successfully.
+  @retval Other          Some error occurred when executing this function.
+
+**/
+STATIC
+VOID
+EFIAPI
+NotifySetVirtualAddressMap (
+  IN EFI_EVENT  Event,
+  IN VOID      *Context
+  )
+{
+  EFI_STATUS  Status;
+
+  Status = gRT->ConvertPointer (EFI_OPTIONAL_PTR,
+                                (VOID **)&mNsCommBuffMemRegion.VirtualBase
+                               );
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "NotifySetVirtualAddressMap():"
+            " Unable to convert MM runtime pointer. Status:0x%r\n", Status));
+  }
+
+}
+
+STATIC
+EFI_STATUS
+GetMmVersion ()
+{
+  EFI_STATUS   Status;
+  UINT16       MmMajorVersion;
+  UINT16       MmMinorVersion;
+  UINT32       MmVersion;
+  ARM_SMC_ARGS MmVersionArgs;
+
+  MmVersionArgs.Arg0 = ARM_SMC_ID_MM_VERSION_AARCH32;
+
+  ArmCallSmc (&MmVersionArgs);
+
+  MmVersion = MmVersionArgs.Arg0;
+
+  MmMajorVersion = ((MmVersion & MM_MAJOR_VER_MASK) >> MM_MAJOR_VER_SHIFT);
+  MmMinorVersion = ((MmVersion & MM_MINOR_VER_MASK) >> 0);
+
+  // Different major revision values indicate possibly incompatible functions.
+  // For two revisions, A and B, for which the major revision values are
+  // identical, if the minor revision value of revision B is greater than
+  // the minor revision value of revision A, then every function in
+  // revision A must work in a compatible way with revision B.
+  // However, it is possible for revision B to have a higher
+  // function count than revision A.
+  if ((MmMajorVersion == MM_MAJOR_VER) &&
+      (MmMinorVersion >= MM_MINOR_VER))
+  {
+    DEBUG ((DEBUG_INFO, "MM Version: Major=0x%x, Minor=0x%x\n",
+           MmMajorVersion, MmMinorVersion));
+    Status = EFI_SUCCESS;
+  }
+  else
+  {
+    DEBUG ((DEBUG_ERROR, "Incompatible MM Versions.\n Current Version: Major=0x%x, Minor=0x%x.\n Expected: Major=0x%x, Minor>=0x%x.\n",
+            MmMajorVersion, MmMinorVersion, MM_MAJOR_VER, MM_MINOR_VER));
+    Status = EFI_UNSUPPORTED;
+  }
+
+  return 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;
+
+    // Get Secure Partition Manager Version Information
+  Status = GetMmVersion ();
+  if (EFI_ERROR(Status)) {
+    goto ReturnErrorStatus;
+  }
+
+  mNsCommBuffMemRegion.PhysicalBase = PcdGet64 (PcdMmBufferBase);
+  // During boot , Virtual and Physical are same
+  mNsCommBuffMemRegion.VirtualBase = mNsCommBuffMemRegion.PhysicalBase;
+  mNsCommBuffMemRegion.Length = PcdGet64 (PcdMmBufferSize);
+
+  if (mNsCommBuffMemRegion.PhysicalBase == 0) {
+    DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: "
+            "Invalid MM Buffer Base Address.\n"));
+    goto ReturnErrorStatus;
+  }
+
+  if (mNsCommBuffMemRegion.Length == 0) {
+    DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: "
+            "Maximum Buffer Size is zero.\n"));
+    goto ReturnErrorStatus;
+  }
+
+  Status = gDS->AddMemorySpace (EfiGcdMemoryTypeSystemMemory,
+                                mNsCommBuffMemRegion.PhysicalBase,
+                                mNsCommBuffMemRegion.Length,
+                                EFI_MEMORY_WB |
+                                EFI_MEMORY_XP |
+                                EFI_MEMORY_RUNTIME);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: "
+            "Failed to add MM-NS Buffer Memory Space\n"));
+    goto ReturnErrorStatus;
+  }
+
+  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"));
+    goto CleanAddedMemorySpace;
+  }
+
+  Status = gBS->AllocatePages (AllocateAddress,
+                               EfiRuntimeServicesData,
+                               EFI_SIZE_TO_PAGES (mNsCommBuffMemRegion.Length),
+                               &mNsCommBuffMemRegion.PhysicalBase);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: "
+            "Failed to allocate MM-NS Buffer Memory Space\n"));
+    goto CleanAddedMemorySpace;
+  }
+
+  // Install the communication protocol
+  Status = gBS->InstallProtocolInterface (&mMmCommunicateHandle,
+                                          &gEfiMmCommunicationProtocolGuid,
+                                          EFI_NATIVE_INTERFACE,
+                                          &mMmCommunication);
+  if (EFI_ERROR(Status)) {
+    DEBUG ((DEBUG_ERROR, "MmCommunicationInitialize: "
+            "Failed to install MM communication protocol\n"));
+    goto CleanAllocatedPages;
+  }
+
+  // Register notification callback when  virtual address is associated
+  // with the physical address.
+  // Create a Set Virtual Address Map event.
+  //
+  Status = gBS->CreateEvent (EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE,  // Type
+                             TPL_NOTIFY,                         // NotifyTpl
+                             NotifySetVirtualAddressMap,         // NotifyFunction
+                             NULL,                               // NotifyContext
+                             &mSetVirtualAddressMapEvent         // Event
+                            );
+  if (Status == EFI_SUCCESS) {
+    return Status;
+  }
+
+  gBS->UninstallProtocolInterface(mMmCommunicateHandle,
+                                  &gEfiMmCommunicationProtocolGuid,
+                                  &mMmCommunication);
+
+CleanAllocatedPages:
+  gBS->FreePages (mNsCommBuffMemRegion.PhysicalBase,
+                  EFI_SIZE_TO_PAGES (mNsCommBuffMemRegion.Length));
+
+CleanAddedMemorySpace:
+  gDS->RemoveMemorySpace (mNsCommBuffMemRegion.PhysicalBase,
+                          mNsCommBuffMemRegion.Length);
+
+ReturnErrorStatus:
+  return EFI_INVALID_PARAMETER;
+}
diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf
new file mode 100644
index 0000000..7797e3d
--- /dev/null
+++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf
@@ -0,0 +1,50 @@
+#/** @file
+#
+#  DXE MM Communicate driver
+#
+#  Copyright (c) 2016 - 2018, 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.AARCH64]
+  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]
+  gEfiCpuArchProtocolGuid
-- 
2.7.4

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

[Leif Lindholm]

On Tue, Jul 03, 2018 at 03:25:11PM +0530, 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 DEN0060A_ARM_MM_Interface_Specification.pdf

I would prefer the document to be referred to by its official name and
its document number:
ARM Management Mode Interface Specification (ARM DEN0060A)

> to communicate with the standalone MM environment in the secure world.
> 
> This patch also adds the MM Communication driver (.inf) file to
> define entry point for this driver and other compile
> related information the driver needs.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Achin Gupta <achin.gupta@arm.com>
> Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com>

Oh, and only one Signed-off-by per patch please.
If authorship is to be indicated, ensure that's correct in git before
calling format-patch.

> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  .../Drivers/MmCommunicationDxe/MmCommunication.c   | 408 +++++++++++++++++++++
>  .../Drivers/MmCommunicationDxe/MmCommunication.inf |  50 +++

And --stat/--stat-graph-width from
https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-23
would mean I wouldn't have to cross-reference below to see which files
are being added/deleted/modified.

Also, the diff order makes a difference.
So please look into the orderfile as described there - noting that
this can now be set permanently in newer revisions of git with
git config diff.orderFile <full path to file>

>  2 files changed, 458 insertions(+)
>  create mode 100644 ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
>  create mode 100644 ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf
> 
> diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
> new file mode 100644
> index 0000000..8ba1790
> --- /dev/null
> +++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
> @@ -0,0 +1,408 @@
> +/** @file
> +
> +  Copyright (c) 2016-2018, 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>
> +
> +#define MM_MAJOR_VER_MASK        0xFFFF0000

Should this not actually be 0xEFFF0000?

> +#define MM_MINOR_VER_MASK        0x0000FFFF
> +#define MM_MAJOR_VER_SHIFT       16

Nothing wrong with this, but later code would be more easy to read if
these were exposed through macros rather than explicitly extracted:

#define MM_MAJOR_VER(x) (((x) & MM_MAJOR_VER_MASK) >> MM_MAJOR_VER_SHIFT)
#define MM_MINOR_VER(x) ((x) & MM_MINOR_VER_MASK)

> +
> +const UINT32 MM_MAJOR_VER = 1;
> +const UINT32 MM_MINOR_VER = 0;

Meanwhile, these
1) Don't need to be given explicit variables (#define would work fine)
2) Have too generic names. What we're really talking about is
   MM_CALLER_MAJOR_VER and MM_CALLER_MINOR_VER?

Finally please put these macros into a local .h file to be included.

> +
> +//
> +// Address, Length of the pre-allocated buffer for communication with the secure
> +// world.
> +//
> +STATIC ARM_MEMORY_REGION_DESCRIPTOR  mNsCommBuffMemRegion;
> +
> +// Notification event when virtual address map is set.
> +STATIC EFI_EVENT  mSetVirtualAddressMapEvent;
> +
> +//
> +// Handle to install the MM Communication Protocol
> +//
> +STATIC EFI_HANDLE  mMmCommunicateHandle;
> +
> +/**
> +  Communicates with a registered handler.
> +
> +  This function provides an interface to send and receive messages to the
> +  Standalone MM environment on behalf of UEFI services.  This function is part
> +  of the MM Communication Protocol that may be called in physical mode prior to
> +  SetVirtualAddressMap() and in virtual mode after SetVirtualAddressMap().
> +
> +  @param[in]      This                The EFI_MM_COMMUNICATION_PROTOCOL
> +                                      instance.
> +  @param[in, out] CommBuffer          A pointer to the buffer to convey
> +                                      into MMRAM.
> +  @param[in, out] CommSize            The size of the data buffer being
> +                                      passed in. This is optional.
> +
> +  @retval EFI_SUCCESS                 The message was successfully posted.
> +  @retval EFI_INVALID_PARAMETER       The CommBuffer was NULL.
> +  @retval EFI_BAD_BUFFER_SIZE         The buffer size is incorrect for the MM
> +                                      implementation. If this error is
> +                                      returned, the MessageLength field in
> +                                      the CommBuffer header or the integer
> +                                      pointed by CommSize are updated to reflect
> +                                      the maximum payload size the
> +                                      implementation can accommodate.
> +  @retval EFI_ACCESS_DENIED           The CommunicateBuffer parameter
> +                                      or CommSize parameter, if not omitted,
> +                                      are in address range that cannot be
> +                                      accessed by the MM environment
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +MmCommunicationCommunicate (
> +  IN CONST EFI_MM_COMMUNICATION_PROTOCOL  *This,
> +  IN OUT VOID                             *CommBuffer,
> +  IN OUT UINTN                            *CommSize OPTIONAL
> +  )
> +{
> +  EFI_MM_COMMUNICATE_HEADER   *CommunicateHeader;
> +  ARM_SMC_ARGS                CommunicateSmcArgs;
> +  EFI_STATUS                  Status;
> +  UINTN                       BufferSize;
> +
> +  Status = EFI_ACCESS_DENIED;
> +  BufferSize = 0;
> +
> +  ZeroMem (&CommunicateSmcArgs, sizeof (ARM_SMC_ARGS));
> +
> +  //
> +  // Check parameters
> +  //
> +  if (CommBuffer == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  CommunicateHeader = CommBuffer;
> +  // CommBuffer is a mandatory parameter. Hence, Rely on
> +  // MessageLength + Header to ascertain the
> +  // total size of the communication payload rather than
> +  // rely on optional CommSize parameter
> +  BufferSize = CommunicateHeader->MessageLength +
> +               sizeof (CommunicateHeader->HeaderGuid) +
> +               sizeof (CommunicateHeader->MessageLength);
> +
> +  // If the length of the CommBuffer is 0 then return the expected length.
> +  if (CommSize) {
> +    // This case can be used by the consumer of this driver to find out the
> +    // max size that can be used for allocating CommBuffer.
> +    if ((*CommSize == 0) ||
> +        (*CommSize > mNsCommBuffMemRegion.Length)) {
> +      *CommSize = mNsCommBuffMemRegion.Length;
> +      return EFI_BAD_BUFFER_SIZE;
> +    }
> +    //
> +    // CommSize must match MessageLength + sizeof (EFI_MM_COMMUNICATE_HEADER);
> +    //
> +    if (*CommSize != BufferSize) {
> +        return EFI_INVALID_PARAMETER;
> +    }
> +  }
> +
> +  //
> +  // 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;
> +
> +  // Copy Communication Payload
> +  CopyMem ((VOID *)mNsCommBuffMemRegion.VirtualBase, CommBuffer, BufferSize);
> +
> +  // For the SMC64 version, this parameter is a 64-bit Physical Address (PA)
> +  // or Intermediate Physical Address (IPA).

PA or IPA is not really relevant to this context.

On the whole, there are a lot of comments in this patch that are
verbatim from the specification. While that does make it easier to
review the patch for initial inclusion, it adds a lot of text not
generally relevant to what the code is doing. Where comments are
needed, they should mention what is being done and why.

> +  // For the SMC32 version, this parameter is a 32-bit PA or IPA.

But we've already hardcoded above that we always use AARCH64?

The use of (UINTN) below is sufficient to indicate that the intent is
to use the native register width of the current execution state.
I don't think the above 3 comment lines convey anything useful here.

Just say

// comm_buffer_address (64-bit physical address)

?

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

Here, the following would suffice:

// comm_size_address (not used, indicated by setting to zero)

> +  CommunicateSmcArgs.Arg3 = 0;
> +
> +  // Call the Standalone MM environment.
> +  ArmCallSmc (&CommunicateSmcArgs);
> +
> +  switch (CommunicateSmcArgs.Arg0) {
> +  case ARM_SMC_MM_RET_SUCCESS:
> +    ZeroMem (CommBuffer, BufferSize);
> +    // On exit, the size of data being returned is inferred from

exit?

> +    // MessageLength + Header.
> +    CommunicateHeader = (EFI_MM_COMMUNICATE_HEADER *)mNsCommBuffMemRegion.VirtualBase;
> +    BufferSize = CommunicateHeader->MessageLength +
> +                 sizeof (CommunicateHeader->HeaderGuid) +
> +                 sizeof (CommunicateHeader->MessageLength);
> +
> +    // Note: Very important to ensure that the consumer of this driver
> +    // has allocated CommBuffer sufficiently so that the return data
> +    // can be copied. Otherwise, this will lead to buffer overflow.
> +    // Assumption: CommBuffer = malloc (mNsCommBuffMemRegion.Length)
> +    // This guidance should be in the PI specification. TODO: ECR.

Do we need to point out anywhere that passes a buffer and a size that
the buffer must not be greater than the size? Am I missing some
subtlety here?

> +    CopyMem (CommBuffer,
> +             (const VOID *)mNsCommBuffMemRegion.VirtualBase,

Why an explicit const?
If needed, it should be CONST.

> +             BufferSize);
> +    Status = EFI_SUCCESS;
> +    break;
> +
> +  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

Worth stating that you're intentionally falling through to default.
It would potentially be useful to have a separate assert here, and a
different return value.

> +  default:
> +    Status = EFI_ACCESS_DENIED;
> +    ASSERT (0);
> +  }
> +
> +  return Status;
> +}
> +
> +//
> +// MM Communication Protocol instance
> +//
> +EFI_MM_COMMUNICATION_PROTOCOL  mMmCommunication = {
> +  MmCommunicationCommunicate
> +};
> +
> +/**
> +  Notification callback on SetVirtualAddressMap event.
> +
> +  This function notifies the MM communication protocol interface on
> +  SetVirtualAddressMap event and converts pointers used in this driver
> +  from physical to virtual address.
> +
> +  @param  Event          SetVirtualAddressMap event.
> +  @param  Context        A context when the SetVirtualAddressMap triggered.
> +
> +  @retval EFI_SUCCESS    The function executed successfully.
> +  @retval Other          Some error occurred when executing this function.
> +
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +NotifySetVirtualAddressMap (
> +  IN EFI_EVENT  Event,
> +  IN VOID      *Context
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  Status = gRT->ConvertPointer (EFI_OPTIONAL_PTR,
> +                                (VOID **)&mNsCommBuffMemRegion.VirtualBase
> +                               );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "NotifySetVirtualAddressMap():"
> +            " Unable to convert MM runtime pointer. Status:0x%r\n", Status));
> +  }
> +
> +}
> +
> +STATIC
> +EFI_STATUS
> +GetMmVersion ()

This function name is misleading. We are checking whether what's on
the other side of the SMC call is compatible with us. Nothing is
returned about what version it is.

> +{
> +  EFI_STATUS   Status;
> +  UINT16       MmMajorVersion;
> +  UINT16       MmMinorVersion;
> +  UINT32       MmVersion;

So, I would suggest simplifying the function as follows:

Drop the 16-bit variables, they can use the macros in place.
Rename MmVersion Implementation.

> +  ARM_SMC_ARGS MmVersionArgs;
> +
> +  MmVersionArgs.Arg0 = ARM_SMC_ID_MM_VERSION_AARCH32;

This could also use a comment on why hard-coded AARCH32.

> +
> +  ArmCallSmc (&MmVersionArgs);
> +
> +  MmVersion = MmVersionArgs.Arg0;
> +
> +  MmMajorVersion = ((MmVersion & MM_MAJOR_VER_MASK) >> MM_MAJOR_VER_SHIFT);
> +  MmMinorVersion = ((MmVersion & MM_MINOR_VER_MASK) >> 0);

So with the above macros, this would be

  MmMajorVersion = MM_MAJOR_VER (MmVersion);
  MmMinorVersion = MM_MINOR_VER (MmVersion);

> +
> +  // Different major revision values indicate possibly incompatible functions.
> +  // For two revisions, A and B, for which the major revision values are
> +  // identical, if the minor revision value of revision B is greater than
> +  // the minor revision value of revision A, then every function in
> +  // revision A must work in a compatible way with revision B.
> +  // However, it is possible for revision B to have a higher
> +  // function count than revision A.

Again, verbatim from specification, not commenting on the code.

I would expect (given the spec) that the implementation must always be
- equal to MM_CALLER_MAJOR_VER and
- equal to or newer than MM_CALLER_MINOR_VER
which is what the code below does.

But it doesn't distinguish between which version belongs to the
implementation and which belongs to the caller.

> +  if ((MmMajorVersion == MM_MAJOR_VER) &&
> +      (MmMinorVersion >= MM_MINOR_VER))

If the above is replaced with:
  if ((MM_MAJOR_VER (Implementation) == CALLER_MAJOR_VER) &&
      (MM_MINOR_VER (Implementation) >= CALLER_MINOR_VER))
then this logic is already clear, and the whole comment block above
can be dropped.

> +  {

Curly bracket at end of previous line.

> +    DEBUG ((DEBUG_INFO, "MM Version: Major=0x%x, Minor=0x%x\n",
> +           MmMajorVersion, MmMinorVersion));
> +    Status = EFI_SUCCESS;
> +  }
> +  else
> +  {

  } else {

> +    DEBUG ((DEBUG_ERROR, "Incompatible MM Versions.\n Current Version: Major=0x%x, Minor=0x%x.\n Expected: Major=0x%x, Minor>=0x%x.\n",
> +            MmMajorVersion, MmMinorVersion, MM_MAJOR_VER, MM_MINOR_VER));
> +    Status = EFI_UNSUPPORTED;
> +  }
> +
> +  return 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;
> +
> +    // Get Secure Partition Manager Version Information

Indentation.

> +  Status = GetMmVersion ();
> +  if (EFI_ERROR(Status)) {
> +    goto ReturnErrorStatus;
> +  }
> +
> +  mNsCommBuffMemRegion.PhysicalBase = PcdGet64 (PcdMmBufferBase);
> +  // During boot , Virtual and Physical are same
> +  mNsCommBuffMemRegion.VirtualBase = mNsCommBuffMemRegion.PhysicalBase;
> +  mNsCommBuffMemRegion.Length = PcdGet64 (PcdMmBufferSize);
> +
> +  if (mNsCommBuffMemRegion.PhysicalBase == 0) {
> +    DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: "
> +            "Invalid MM Buffer Base Address.\n"));
> +    goto ReturnErrorStatus;
> +  }
> +
> +  if (mNsCommBuffMemRegion.Length == 0) {
> +    DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: "
> +            "Maximum Buffer Size is zero.\n"));
> +    goto ReturnErrorStatus;
> +  }
> +
> +  Status = gDS->AddMemorySpace (EfiGcdMemoryTypeSystemMemory,
> +                                mNsCommBuffMemRegion.PhysicalBase,
> +                                mNsCommBuffMemRegion.Length,
> +                                EFI_MEMORY_WB |
> +                                EFI_MEMORY_XP |
> +                                EFI_MEMORY_RUNTIME);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: "
> +            "Failed to add MM-NS Buffer Memory Space\n"));
> +    goto ReturnErrorStatus;
> +  }
> +
> +  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"));
> +    goto CleanAddedMemorySpace;
> +  }
> +
> +  Status = gBS->AllocatePages (AllocateAddress,
> +                               EfiRuntimeServicesData,
> +                               EFI_SIZE_TO_PAGES (mNsCommBuffMemRegion.Length),
> +                               &mNsCommBuffMemRegion.PhysicalBase);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: "
> +            "Failed to allocate MM-NS Buffer Memory Space\n"));
> +    goto CleanAddedMemorySpace;
> +  }
> +
> +  // Install the communication protocol
> +  Status = gBS->InstallProtocolInterface (&mMmCommunicateHandle,
> +                                          &gEfiMmCommunicationProtocolGuid,
> +                                          EFI_NATIVE_INTERFACE,
> +                                          &mMmCommunication);
> +  if (EFI_ERROR(Status)) {
> +    DEBUG ((DEBUG_ERROR, "MmCommunicationInitialize: "
> +            "Failed to install MM communication protocol\n"));
> +    goto CleanAllocatedPages;
> +  }
> +
> +  // Register notification callback when  virtual address is associated
> +  // with the physical address.
> +  // Create a Set Virtual Address Map event.
> +  //
> +  Status = gBS->CreateEvent (EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE,  // Type
> +                             TPL_NOTIFY,                         // NotifyTpl
> +                             NotifySetVirtualAddressMap,         // NotifyFunction
> +                             NULL,                               // NotifyContext
> +                             &mSetVirtualAddressMapEvent         // Event
> +                            );

The comments for every parameter are a bit overkill. There's nothing
fancy going on.

> +  if (Status == EFI_SUCCESS) {
> +    return Status;
> +  }
> +
> +  gBS->UninstallProtocolInterface(mMmCommunicateHandle,
> +                                  &gEfiMmCommunicationProtocolGuid,
> +                                  &mMmCommunication);
> +
> +CleanAllocatedPages:
> +  gBS->FreePages (mNsCommBuffMemRegion.PhysicalBase,
> +                  EFI_SIZE_TO_PAGES (mNsCommBuffMemRegion.Length));
> +
> +CleanAddedMemorySpace:
> +  gDS->RemoveMemorySpace (mNsCommBuffMemRegion.PhysicalBase,
> +                          mNsCommBuffMemRegion.Length);
> +
> +ReturnErrorStatus:
> +  return EFI_INVALID_PARAMETER;
> +}
> diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf
> new file mode 100644
> index 0000000..7797e3d
> --- /dev/null
> +++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf
> @@ -0,0 +1,50 @@
> +#/** @file
> +#
> +#  DXE MM Communicate driver
> +#
> +#  Copyright (c) 2016 - 2018, 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
> +

Please drop this blank line.

> +  ENTRY_POINT                    = MmCommunicationInitialize
> +

For added clarity, it would be useful to add

#
# The following information is for reference only and not required by
# the build tools.
#
#  VALID_ARCHITECTURES           = AARCH64
#

/
    Leif

> +[Sources.AARCH64]
> +  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]
> +  gEfiCpuArchProtocolGuid
> -- 
> 2.7.4
> 

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

[Sughosh Ganu]

hi Leif,

On Tue, Jul 3, 2018 at 7:42 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Jul 03, 2018 at 03:25:11PM +0530, 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 DEN0060A_ARM_MM_Interface_Specification.pdf
>
> I would prefer the document to be referred to by its official name and
> its document number:
> ARM Management Mode Interface Specification (ARM DEN0060A)
>
>> to communicate with the standalone MM environment in the secure world.
>>
>> This patch also adds the MM Communication driver (.inf) file to
>> define entry point for this driver and other compile
>> related information the driver needs.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Achin Gupta <achin.gupta@arm.com>
>> Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
>
> Oh, and only one Signed-off-by per patch please.
> If authorship is to be indicated, ensure that's correct in git before
> calling format-patch.

Supreeth has moved onto some other work, hence I will be working on
the upstreaming of these patches henceforth. Will handle your comments
on all the patches and send an updated version. Regarding the
inclusion of a single Signed-off-By, i have a doubt. Work on these
patches was initially done by Achin, and then Supreeth. I will be
handling your review comments and posting the updated version. You
have posted a comment saying that we can have only a single s-o-b in
any given patch. In such a scenario, how can we attribute the work
done by all the engineers for these patches. Can you please let me
know on this. As per my understanding, other projects do allow
multiple s-o-b's per patch. Thanks.

-sughosh

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

[Leif Lindholm]

On Mon, Jul 09, 2018 at 02:05:32PM +0530, Sughosh Ganu wrote:
> hi Leif,
> 
> On Tue, Jul 3, 2018 at 7:42 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Tue, Jul 03, 2018 at 03:25:11PM +0530, 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 DEN0060A_ARM_MM_Interface_Specification.pdf
> >
> > I would prefer the document to be referred to by its official name and
> > its document number:
> > ARM Management Mode Interface Specification (ARM DEN0060A)
> >
> >> to communicate with the standalone MM environment in the secure world.
> >>
> >> This patch also adds the MM Communication driver (.inf) file to
> >> define entry point for this driver and other compile
> >> related information the driver needs.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Achin Gupta <achin.gupta@arm.com>
> >> Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
> >
> > Oh, and only one Signed-off-by per patch please.
> > If authorship is to be indicated, ensure that's correct in git before
> > calling format-patch.
> 
> Supreeth has moved onto some other work, hence I will be working on
> the upstreaming of these patches henceforth.

Splendid, welcome aboard.

> Will handle your comments
> on all the patches and send an updated version. Regarding the
> inclusion of a single Signed-off-By, i have a doubt. Work on these
> patches was initially done by Achin, and then Supreeth. I will be
> handling your review comments and posting the updated version. You
> have posted a comment saying that we can have only a single s-o-b in
> any given patch. In such a scenario, how can we attribute the work
> done by all the engineers for these patches. Can you please let me
> know on this. As per my understanding, other projects do allow
> multiple s-o-b's per patch. Thanks.

We do permit multiple s-o-b.
When multiple developers are working together in public, adding
non-trivial bits to a patch in flight, you can add multiple s-o-b.

What is not appropriate is to post a patch containing company-internal
details about who handled the patch before it was sent public.
The signed-off-by is a statement regarding the suitability of the
contribution - it is not attribution.
The Author field is the only attribution.

Regards,

Leif

p.s.
I presume you're taking the set over because you work for ARM - so
please communicate with the mailing list from your @arm.com account.

[PATCH 3/6] ArmPkg/Include: Add MM interface SVC return codes.

[Supreeth Venkatesh]

This patch adds the Management Mode(MM) - Secure Partition
Manager(SPM) SVC return codes.

Also, It corrects SVC ID for retrieving SPM version information.

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>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Include/IndustryStandard/ArmMmSvc.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/ArmPkg/Include/IndustryStandard/ArmMmSvc.h b/ArmPkg/Include/IndustryStandard/ArmMmSvc.h
index 4c7b6c3..a64b9ec 100644
--- a/ArmPkg/Include/IndustryStandard/ArmMmSvc.h
+++ b/ArmPkg/Include/IndustryStandard/ArmMmSvc.h
@@ -20,7 +20,7 @@
  * delegated events and request the Secure partition manager to perform
  * privileged operations on its behalf.
  */
-#define ARM_SVC_ID_SPM_VERSION_AARCH64             0xC4000060
+#define ARM_SVC_ID_SPM_VERSION_AARCH32             0x84000060
 #define ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64       0xC4000061
 #define ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64   0xC4000064
 #define ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64   0xC4000065
@@ -40,4 +40,11 @@
     ((((c_perm) & SET_MEM_ATTR_CODE_PERM_MASK) << SET_MEM_ATTR_CODE_PERM_SHIFT) | \
     (( (d_perm) & SET_MEM_ATTR_DATA_PERM_MASK) << SET_MEM_ATTR_DATA_PERM_SHIFT))
 
+/* MM SVC Return error codes */
+#define ARM_SVC_SPM_RET_SUCCESS               0
+#define ARM_SVC_SPM_RET_NOT_SUPPORTED        -1
+#define ARM_SVC_SPM_RET_INVALID_PARAMS       -2
+#define ARM_SVC_SPM_RET_DENIED               -3
+#define ARM_SVC_SPM_RET_NO_MEMORY            -5
+
 #endif
-- 
2.7.4

Re: [PATCH 3/6] ArmPkg/Include: Add MM interface SVC return codes.

[Leif Lindholm]

On Tue, Jul 03, 2018 at 03:25:12PM +0530, Supreeth Venkatesh wrote:
> This patch adds the Management Mode(MM) - Secure Partition
> Manager(SPM) SVC return codes.
> 
> Also, It corrects SVC ID for retrieving SPM version information.

That sounds unrelated. And looking at it, worthy of a longer
explanation, so probably worth breaking out as a separate patch.

> 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>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPkg/Include/IndustryStandard/ArmMmSvc.h | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/ArmPkg/Include/IndustryStandard/ArmMmSvc.h b/ArmPkg/Include/IndustryStandard/ArmMmSvc.h
> index 4c7b6c3..a64b9ec 100644
> --- a/ArmPkg/Include/IndustryStandard/ArmMmSvc.h
> +++ b/ArmPkg/Include/IndustryStandard/ArmMmSvc.h
> @@ -20,7 +20,7 @@
>   * delegated events and request the Secure partition manager to perform
>   * privileged operations on its behalf.
>   */
> -#define ARM_SVC_ID_SPM_VERSION_AARCH64             0xC4000060
> +#define ARM_SVC_ID_SPM_VERSION_AARCH32             0x84000060

So, this isn't "correcting" a value - it's replacing a 64-bit call
with a 32-bit one. Which is potentially fine, but then it deserves a
separate patch and a commit message clearly describing why this is the
right thing to do.

Regards,

Leif

>  #define ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64       0xC4000061
>  #define ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64   0xC4000064
>  #define ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64   0xC4000065
> @@ -40,4 +40,11 @@
>      ((((c_perm) & SET_MEM_ATTR_CODE_PERM_MASK) << SET_MEM_ATTR_CODE_PERM_SHIFT) | \
>      (( (d_perm) & SET_MEM_ATTR_DATA_PERM_MASK) << SET_MEM_ATTR_DATA_PERM_SHIFT))
>  
> +/* MM SVC Return error codes */
> +#define ARM_SVC_SPM_RET_SUCCESS               0
> +#define ARM_SVC_SPM_RET_NOT_SUPPORTED        -1
> +#define ARM_SVC_SPM_RET_INVALID_PARAMS       -2
> +#define ARM_SVC_SPM_RET_DENIED               -3
> +#define ARM_SVC_SPM_RET_NO_MEMORY            -5
> +
>  #endif
> -- 
> 2.7.4
> 

[PATCH 4/6] ArmPkg/ArmMmuLib: Add MMU Library suitable for use in S-EL0.

[Supreeth Venkatesh]

The Standalone MM environment runs in S-EL0 in AArch64 on ARM Standard
Platforms. Privileged firmware e.g. ARM Trusted Firmware sets up its
architectural context including the initial translation tables for the
S-EL1/EL0 translation regime. The MM environment will still request ARM
TF to change the memory attributes of memory regions during
initialization.

The Standalone MM image is a FV that encapsulates the MM foundation
and drivers. These are PE-COFF images with data and text segments.
To initialise the MM environment, Arm Trusted Firmware has to create
translation tables with sane default attributes for the memory
occupied by the FV. This library sends SVCs to ARM Trusted Firmware
to request memory permissions change for data and text segments.

This patch adds a simple MMU library suitable for execution in S-EL0 and
requesting memory permissions change operations from Arm Trusted Firmware.

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>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 .../ArmMmuLib/AArch64/ArmMmuStandaloneMmCoreLib.c  | 195 +++++++++++++++++++++
 1 file changed, 195 insertions(+)
 create mode 100644 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuStandaloneMmCoreLib.c

diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuStandaloneMmCoreLib.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuStandaloneMmCoreLib.c
new file mode 100644
index 0000000..0f5e68d
--- /dev/null
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuStandaloneMmCoreLib.c
@@ -0,0 +1,195 @@
+/** @file
+*  File managing the MMU for ARMv8 architecture in S-EL0
+*
+*  Copyright (c) 2017 - 2018, 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 <Uefi.h>
+#include <Chipset/AArch64.h>
+#include <IndustryStandard/ArmMmSvc.h>
+
+#include <Library/ArmLib.h>
+#include <Library/ArmMmuLib.h>
+#include <Library/ArmSvcLib.h>
+#include <Library/BaseLib.h>
+#include <Library/DebugLib.h>
+
+EFI_STATUS
+GetMemoryPermissions (
+  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
+  OUT UINT32                    *MemoryAttributes
+  )
+{
+  ARM_SVC_ARGS  GetMemoryPermissionsSvcArgs = {0};
+
+  GetMemoryPermissionsSvcArgs.Arg0 = ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64;
+  GetMemoryPermissionsSvcArgs.Arg1 = BaseAddress;
+  GetMemoryPermissionsSvcArgs.Arg2 = 0;
+  GetMemoryPermissionsSvcArgs.Arg3 = 0;
+
+  ArmCallSvc (&GetMemoryPermissionsSvcArgs);
+  if (GetMemoryPermissionsSvcArgs.Arg0 == ARM_SVC_SPM_RET_INVALID_PARAMS) {
+    *MemoryAttributes = 0;
+    return EFI_INVALID_PARAMETER;
+  }
+
+  *MemoryAttributes = GetMemoryPermissionsSvcArgs.Arg0;
+  return EFI_SUCCESS;
+}
+
+EFI_STATUS
+RequestMemoryPermissionChange (
+  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
+  IN  UINT64                    Length,
+  IN  UINTN                     Permissions
+  )
+{
+  EFI_STATUS    Status;
+  ARM_SVC_ARGS  ChangeMemoryPermissionsSvcArgs = {0};
+
+  ChangeMemoryPermissionsSvcArgs.Arg0 = ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64;
+  ChangeMemoryPermissionsSvcArgs.Arg1 = BaseAddress;
+  ChangeMemoryPermissionsSvcArgs.Arg2 = (Length >= EFI_PAGE_SIZE) ? \
+                                         Length >> EFI_PAGE_SHIFT : 1;
+  ChangeMemoryPermissionsSvcArgs.Arg3 = Permissions;
+
+  ArmCallSvc (&ChangeMemoryPermissionsSvcArgs);
+
+  Status = ChangeMemoryPermissionsSvcArgs.Arg0;
+
+  switch (Status) {
+  case ARM_SVC_SPM_RET_SUCCESS:
+    Status = EFI_SUCCESS;
+    break;
+
+  case ARM_SVC_SPM_RET_NOT_SUPPORTED:
+    Status = EFI_UNSUPPORTED;
+    break;
+
+  case ARM_SVC_SPM_RET_INVALID_PARAMS:
+    Status = EFI_INVALID_PARAMETER;
+    break;
+
+  case ARM_SVC_SPM_RET_DENIED:
+    Status = EFI_ACCESS_DENIED;
+    break;
+
+  case ARM_SVC_SPM_RET_NO_MEMORY:
+    Status = EFI_BAD_BUFFER_SIZE;
+    break;
+
+  default:
+    Status = EFI_ACCESS_DENIED;
+    ASSERT (0);
+  }
+
+  return Status;
+}
+
+EFI_STATUS
+ArmSetMemoryRegionNoExec (
+  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
+  IN  UINT64                    Length
+  )
+{
+  EFI_STATUS    Status;
+  UINT32 MemoryAttributes;
+
+  Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);
+  if (Status != EFI_INVALID_PARAMETER) {
+    return RequestMemoryPermissionChange (BaseAddress,
+                                          Length,
+                                          MemoryAttributes |
+                                          (SET_MEM_ATTR_CODE_PERM_XN << SET_MEM_ATTR_CODE_PERM_SHIFT));
+  }
+  return EFI_INVALID_PARAMETER;
+}
+
+EFI_STATUS
+ArmClearMemoryRegionNoExec (
+  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
+  IN  UINT64                    Length
+  )
+{
+  EFI_STATUS    Status;
+  UINT32 MemoryAttributes;
+
+  Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);
+  if (Status != EFI_INVALID_PARAMETER) {
+    return RequestMemoryPermissionChange (BaseAddress,
+                                          Length,
+                                          MemoryAttributes &
+                                          ~(SET_MEM_ATTR_CODE_PERM_XN << SET_MEM_ATTR_CODE_PERM_SHIFT));
+  }
+  return EFI_INVALID_PARAMETER;
+}
+
+EFI_STATUS
+ArmSetMemoryRegionReadOnly (
+  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
+  IN  UINT64                    Length
+  )
+{
+  EFI_STATUS    Status;
+  UINT32 MemoryAttributes;
+
+  Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);
+  if (Status != EFI_INVALID_PARAMETER) {
+    return RequestMemoryPermissionChange (BaseAddress,
+                                          Length,
+                                          MemoryAttributes |
+                                          (SET_MEM_ATTR_DATA_PERM_RO << SET_MEM_ATTR_DATA_PERM_SHIFT));
+  }
+  return EFI_INVALID_PARAMETER;
+}
+
+EFI_STATUS
+ArmClearMemoryRegionReadOnly (
+  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
+  IN  UINT64                    Length
+  )
+{
+  EFI_STATUS    Status;
+  UINT32 MemoryAttributes;
+
+  Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);
+  if (Status != EFI_INVALID_PARAMETER) {
+    return RequestMemoryPermissionChange (BaseAddress,
+                                          Length,
+                                          SET_MEM_ATTR_MAKE_PERM_REQUEST
+                                            ( \
+                                             SET_MEM_ATTR_DATA_PERM_RW, \
+                                             MemoryAttributes));
+  }
+  return EFI_INVALID_PARAMETER;
+}
+
+EFI_STATUS
+EFIAPI
+ArmConfigureMmu (
+  IN  ARM_MEMORY_REGION_DESCRIPTOR  *MemoryTable,
+  OUT VOID                          **TranslationTableBase OPTIONAL,
+  OUT UINTN                         *TranslationTableSize OPTIONAL
+  )
+{
+  return EFI_UNSUPPORTED;
+}
+
+EFI_STATUS
+EFIAPI
+ArmMmuStandaloneMmCoreLibConstructor (
+  IN EFI_HANDLE            ImageHandle,
+  IN EFI_MM_SYSTEM_TABLE   *MmSystemTable
+  )
+{
+  return EFI_SUCCESS;
+}
-- 
2.7.4

Re: [PATCH 4/6] ArmPkg/ArmMmuLib: Add MMU Library suitable for use in S-EL0.

[Leif Lindholm]

On Tue, Jul 03, 2018 at 03:25:13PM +0530, Supreeth Venkatesh wrote:
> The Standalone MM environment runs in S-EL0 in AArch64 on ARM Standard
> Platforms. Privileged firmware e.g. ARM Trusted Firmware sets up its
> architectural context including the initial translation tables for the
> S-EL1/EL0 translation regime. The MM environment will still request ARM
> TF to change the memory attributes of memory regions during
> initialization.
> 
> The Standalone MM image is a FV that encapsulates the MM foundation
> and drivers. These are PE-COFF images with data and text segments.
> To initialise the MM environment, Arm Trusted Firmware has to create
> translation tables with sane default attributes for the memory
> occupied by the FV. This library sends SVCs to ARM Trusted Firmware
> to request memory permissions change for data and text segments.
> 
> This patch adds a simple MMU library suitable for execution in S-EL0 and
> requesting memory permissions change operations from Arm Trusted Firmware.
> 
> 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>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  .../ArmMmuLib/AArch64/ArmMmuStandaloneMmCoreLib.c  | 195 +++++++++++++++++++++
>  1 file changed, 195 insertions(+)
>  create mode 100644 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuStandaloneMmCoreLib.c
> 
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuStandaloneMmCoreLib.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuStandaloneMmCoreLib.c
> new file mode 100644
> index 0000000..0f5e68d
> --- /dev/null
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuStandaloneMmCoreLib.c
> @@ -0,0 +1,195 @@
> +/** @file
> +*  File managing the MMU for ARMv8 architecture in S-EL0
> +*
> +*  Copyright (c) 2017 - 2018, 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 <Uefi.h>
> +#include <Chipset/AArch64.h>
> +#include <IndustryStandard/ArmMmSvc.h>
> +
> +#include <Library/ArmLib.h>
> +#include <Library/ArmMmuLib.h>
> +#include <Library/ArmSvcLib.h>
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +
> +EFI_STATUS
> +GetMemoryPermissions (
> +  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
> +  OUT UINT32                    *MemoryAttributes
> +  )
> +{
> +  ARM_SVC_ARGS  GetMemoryPermissionsSvcArgs = {0};
> +
> +  GetMemoryPermissionsSvcArgs.Arg0 = ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64;
> +  GetMemoryPermissionsSvcArgs.Arg1 = BaseAddress;
> +  GetMemoryPermissionsSvcArgs.Arg2 = 0;
> +  GetMemoryPermissionsSvcArgs.Arg3 = 0;
> +
> +  ArmCallSvc (&GetMemoryPermissionsSvcArgs);
> +  if (GetMemoryPermissionsSvcArgs.Arg0 == ARM_SVC_SPM_RET_INVALID_PARAMS) {
> +    *MemoryAttributes = 0;
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  *MemoryAttributes = GetMemoryPermissionsSvcArgs.Arg0;
> +  return EFI_SUCCESS;
> +}
> +
> +EFI_STATUS
> +RequestMemoryPermissionChange (
> +  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
> +  IN  UINT64                    Length,
> +  IN  UINTN                     Permissions
> +  )
> +{
> +  EFI_STATUS    Status;
> +  ARM_SVC_ARGS  ChangeMemoryPermissionsSvcArgs = {0};
> +
> +  ChangeMemoryPermissionsSvcArgs.Arg0 = ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64;
> +  ChangeMemoryPermissionsSvcArgs.Arg1 = BaseAddress;
> +  ChangeMemoryPermissionsSvcArgs.Arg2 = (Length >= EFI_PAGE_SIZE) ? \
> +                                         Length >> EFI_PAGE_SHIFT : 1;

Use EFI_SIZE_TO_PAGES?

> +  ChangeMemoryPermissionsSvcArgs.Arg3 = Permissions;
> +
> +  ArmCallSvc (&ChangeMemoryPermissionsSvcArgs);
> +
> +  Status = ChangeMemoryPermissionsSvcArgs.Arg0;
> +
> +  switch (Status) {
> +  case ARM_SVC_SPM_RET_SUCCESS:
> +    Status = EFI_SUCCESS;
> +    break;
> +
> +  case ARM_SVC_SPM_RET_NOT_SUPPORTED:
> +    Status = EFI_UNSUPPORTED;
> +    break;
> +
> +  case ARM_SVC_SPM_RET_INVALID_PARAMS:
> +    Status = EFI_INVALID_PARAMETER;
> +    break;
> +
> +  case ARM_SVC_SPM_RET_DENIED:
> +    Status = EFI_ACCESS_DENIED;
> +    break;
> +
> +  case ARM_SVC_SPM_RET_NO_MEMORY:
> +    Status = EFI_BAD_BUFFER_SIZE;
> +    break;
> +
> +  default:
> +    Status = EFI_ACCESS_DENIED;
> +    ASSERT (0);
> +  }
> +
> +  return Status;
> +}
> +
> +EFI_STATUS
> +ArmSetMemoryRegionNoExec (
> +  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
> +  IN  UINT64                    Length
> +  )
> +{
> +  EFI_STATUS    Status;
> +  UINT32 MemoryAttributes;
> +
> +  Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);
> +  if (Status != EFI_INVALID_PARAMETER) {
> +    return RequestMemoryPermissionChange (BaseAddress,
> +                                          Length,
> +                                          MemoryAttributes |
> +                                          (SET_MEM_ATTR_CODE_PERM_XN << SET_MEM_ATTR_CODE_PERM_SHIFT));

Very long line - please use a temporary variable to shorten.
(Throughout.)

> +  }
> +  return EFI_INVALID_PARAMETER;
> +}
> +
> +EFI_STATUS
> +ArmClearMemoryRegionNoExec (
> +  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
> +  IN  UINT64                    Length
> +  )
> +{
> +  EFI_STATUS    Status;
> +  UINT32 MemoryAttributes;
> +
> +  Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);
> +  if (Status != EFI_INVALID_PARAMETER) {
> +    return RequestMemoryPermissionChange (BaseAddress,
> +                                          Length,
> +                                          MemoryAttributes &
> +                                          ~(SET_MEM_ATTR_CODE_PERM_XN << SET_MEM_ATTR_CODE_PERM_SHIFT));
> +  }
> +  return EFI_INVALID_PARAMETER;
> +}
> +
> +EFI_STATUS
> +ArmSetMemoryRegionReadOnly (
> +  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
> +  IN  UINT64                    Length
> +  )
> +{
> +  EFI_STATUS    Status;
> +  UINT32 MemoryAttributes;
> +
> +  Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);
> +  if (Status != EFI_INVALID_PARAMETER) {
> +    return RequestMemoryPermissionChange (BaseAddress,
> +                                          Length,
> +                                          MemoryAttributes |
> +                                          (SET_MEM_ATTR_DATA_PERM_RO << SET_MEM_ATTR_DATA_PERM_SHIFT));
> +  }
> +  return EFI_INVALID_PARAMETER;
> +}
> +
> +EFI_STATUS
> +ArmClearMemoryRegionReadOnly (
> +  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
> +  IN  UINT64                    Length
> +  )
> +{
> +  EFI_STATUS    Status;
> +  UINT32 MemoryAttributes;
> +
> +  Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);
> +  if (Status != EFI_INVALID_PARAMETER) {
> +    return RequestMemoryPermissionChange (BaseAddress,
> +                                          Length,
> +                                          SET_MEM_ATTR_MAKE_PERM_REQUEST
> +                                            ( \
> +                                             SET_MEM_ATTR_DATA_PERM_RW, \
> +                                             MemoryAttributes));

Use temporary variable.

/
    Leif

> +  }
> +  return EFI_INVALID_PARAMETER;
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +ArmConfigureMmu (
> +  IN  ARM_MEMORY_REGION_DESCRIPTOR  *MemoryTable,
> +  OUT VOID                          **TranslationTableBase OPTIONAL,
> +  OUT UINTN                         *TranslationTableSize OPTIONAL
> +  )
> +{
> +  return EFI_UNSUPPORTED;
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +ArmMmuStandaloneMmCoreLibConstructor (
> +  IN EFI_HANDLE            ImageHandle,
> +  IN EFI_MM_SYSTEM_TABLE   *MmSystemTable
> +  )
> +{
> +  return EFI_SUCCESS;
> +}
> -- 
> 2.7.4
> 

[PATCH 5/6] ArmPkg/ArmMmuLib: Add MMU library inf file suitable for use in S-EL0.

[Supreeth Venkatesh]

This patch adds the definitions, sources, packages and library classes
needed to compile and link MMU Library suitable for use in S-EL0.

Currently, this is used only during the Standalone MM Core
initialization and hence defined as MM_CORE_STANDALONE Module.

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>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 .../ArmMmuLib/ArmMmuStandaloneMmCoreLib.inf        | 37 ++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 ArmPkg/Library/ArmMmuLib/ArmMmuStandaloneMmCoreLib.inf

diff --git a/ArmPkg/Library/ArmMmuLib/ArmMmuStandaloneMmCoreLib.inf b/ArmPkg/Library/ArmMmuLib/ArmMmuStandaloneMmCoreLib.inf
new file mode 100644
index 0000000..9f5593d
--- /dev/null
+++ b/ArmPkg/Library/ArmMmuLib/ArmMmuStandaloneMmCoreLib.inf
@@ -0,0 +1,37 @@
+#/** @file
+#
+#  Copyright (c) 2017 - 2018, 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                      = ArmMmuStandaloneMmCoreLib
+  FILE_GUID                      = da8f0232-fb14-42f0-922c-63104d2c70bd
+  MODULE_TYPE                    = MM_CORE_STANDALONE
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = ArmMmuStandaloneMmCoreLib|MM_CORE_STANDALONE
+  PI_SPECIFICATION_VERSION       = 0x00010032
+  CONSTRUCTOR                    = ArmMmuStandaloneMmCoreLibConstructor
+
+[Sources.AARCH64]
+  AArch64/ArmMmuStandaloneMmCoreLib.c
+
+[Packages]
+  ArmPkg/ArmPkg.dec
+  MdePkg/MdePkg.dec
+
+[LibraryClasses]
+  ArmLib
+  CacheMaintenanceLib
+  MemoryAllocationLib
+
+
-- 
2.7.4

[PATCH 6/6] ArmPkg: Extra action to update permissions for S-ELO MM Image.

[Supreeth Venkatesh]

The Standalone MM drivers runs in S-EL0 in AArch64 on ARM Standard
Platforms and is deployed during SEC phase. The memory allocated to the
Standalone MM drivers should be marked as RO+X.

During PE/COFF Image section parsing, this patch implements extra action
"UpdatePeCoffPermissions" to request the privileged firmware in EL3 to
update the permissions.

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>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 .../DebugPeCoffExtraActionLib.c                    | 185 +++++++++++++++++++--
 .../DebugPeCoffExtraActionLib.inf                  |   7 +
 2 files changed, 181 insertions(+), 11 deletions(-)

diff --git a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
index f298e58..c87aaf0 100644
--- a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
+++ b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
@@ -15,14 +15,165 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 **/
 
 #include <PiDxe.h>
-#include <Library/PeCoffLib.h>
 
+#include <Library/ArmMmuLib.h>
 #include <Library/BaseLib.h>
-#include <Library/DebugLib.h>
 #include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/PcdLib.h>
+#include <Library/PeCoffLib.h>
 #include <Library/PeCoffExtraActionLib.h>
 #include <Library/PrintLib.h>
 
+typedef RETURN_STATUS (*REGION_PERMISSION_UPDATE_FUNC) (
+  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
+  IN  UINT64                    Length
+  );
+
+STATIC
+RETURN_STATUS
+UpdatePeCoffPermissions (
+  IN  CONST PE_COFF_LOADER_IMAGE_CONTEXT      *ImageContext,
+  IN  REGION_PERMISSION_UPDATE_FUNC           NoExecUpdater,
+  IN  REGION_PERMISSION_UPDATE_FUNC           ReadOnlyUpdater
+  )
+{
+  RETURN_STATUS                         Status;
+  EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION   Hdr;
+  EFI_IMAGE_OPTIONAL_HEADER_UNION       HdrData;
+  UINTN                                 Size;
+  UINTN                                 ReadSize;
+  UINT32                                SectionHeaderOffset;
+  UINTN                                 NumberOfSections;
+  UINTN                                 Index;
+  EFI_IMAGE_SECTION_HEADER              SectionHeader;
+  PE_COFF_LOADER_IMAGE_CONTEXT          TmpContext;
+  EFI_PHYSICAL_ADDRESS                  Base;
+
+  //
+  // We need to copy ImageContext since PeCoffLoaderGetImageInfo ()
+  // will mangle the ImageAddress field
+  //
+  CopyMem (&TmpContext, ImageContext, sizeof (TmpContext));
+
+  if (TmpContext.PeCoffHeaderOffset == 0) {
+    Status = PeCoffLoaderGetImageInfo (&TmpContext);
+    if (RETURN_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR,
+        "%a: PeCoffLoaderGetImageInfo () failed (Status = %r)\n",
+        __FUNCTION__, Status));
+      return Status;
+    }
+  }
+
+  if (TmpContext.IsTeImage &&
+      TmpContext.ImageAddress == ImageContext->ImageAddress) {
+    DEBUG ((DEBUG_INFO, "%a: ignoring XIP TE image at 0x%lx\n", __FUNCTION__,
+      ImageContext->ImageAddress));
+    return RETURN_SUCCESS;
+  }
+
+  if (TmpContext.SectionAlignment < EFI_PAGE_SIZE) {
+    //
+    // The sections need to be at least 4 KB aligned, since that is the
+    // granularity at which we can tighten permissions. So just clear the
+    // noexec permissions on the entire region.
+    //
+    if (!TmpContext.IsTeImage) {
+      DEBUG ((DEBUG_WARN,
+        "%a: non-TE Image at 0x%lx has SectionAlignment < 4 KB (%lu)\n",
+        __FUNCTION__, ImageContext->ImageAddress, TmpContext.SectionAlignment));
+    }
+    Base = ImageContext->ImageAddress & ~(EFI_PAGE_SIZE - 1);
+    Size = ImageContext->ImageAddress - Base + ImageContext->ImageSize;
+    return NoExecUpdater (Base, ALIGN_VALUE (Size, EFI_PAGE_SIZE));
+  }
+
+  //
+  // Read the PE/COFF Header. For PE32 (32-bit) this will read in too much
+  // data, but that should not hurt anything. Hdr.Pe32->OptionalHeader.Magic
+  // determines if this is a PE32 or PE32+ image. The magic is in the same
+  // location in both images.
+  //
+  Hdr.Union = &HdrData;
+  Size = sizeof (EFI_IMAGE_OPTIONAL_HEADER_UNION);
+  ReadSize = Size;
+  Status = TmpContext.ImageRead (TmpContext.Handle,
+                         TmpContext.PeCoffHeaderOffset, &Size, Hdr.Pe32);
+  if (RETURN_ERROR (Status) || (Size != ReadSize)) {
+    DEBUG ((DEBUG_ERROR,
+      "%a: TmpContext.ImageRead () failed (Status = %r)\n",
+      __FUNCTION__, Status));
+    return Status;
+  }
+
+  ASSERT (Hdr.Pe32->Signature == EFI_IMAGE_NT_SIGNATURE);
+
+  SectionHeaderOffset = TmpContext.PeCoffHeaderOffset + sizeof (UINT32) +
+                        sizeof (EFI_IMAGE_FILE_HEADER);
+  NumberOfSections    = (UINTN)(Hdr.Pe32->FileHeader.NumberOfSections);
+
+  switch (Hdr.Pe32->OptionalHeader.Magic) {
+    case EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC:
+      SectionHeaderOffset += Hdr.Pe32->FileHeader.SizeOfOptionalHeader;
+      break;
+    case EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC:
+      SectionHeaderOffset += Hdr.Pe32Plus->FileHeader.SizeOfOptionalHeader;
+      break;
+    default:
+      ASSERT (FALSE);
+  }
+
+  //
+  // Iterate over the sections
+  //
+  for (Index = 0; Index < NumberOfSections; Index++) {
+    //
+    // Read section header from file
+    //
+    Size = sizeof (EFI_IMAGE_SECTION_HEADER);
+    ReadSize = Size;
+    Status = TmpContext.ImageRead (TmpContext.Handle, SectionHeaderOffset,
+                                   &Size, &SectionHeader);
+    if (RETURN_ERROR (Status) || (Size != ReadSize)) {
+      DEBUG ((DEBUG_ERROR,
+        "%a: TmpContext.ImageRead () failed (Status = %r)\n",
+        __FUNCTION__, Status));
+      return Status;
+    }
+
+    Base = TmpContext.ImageAddress + SectionHeader.VirtualAddress;
+
+    if ((SectionHeader.Characteristics & EFI_IMAGE_SCN_MEM_EXECUTE) == 0) {
+
+      if ((SectionHeader.Characteristics & EFI_IMAGE_SCN_MEM_WRITE) == 0 &&
+          TmpContext.ImageType != EFI_IMAGE_SUBSYSTEM_EFI_RUNTIME_DRIVER) {
+
+        DEBUG ((DEBUG_INFO,
+          "%a: Mapping section %d of image at 0x%lx with RO-XN permissions and size 0x%x\n",
+          __FUNCTION__, Index, Base, SectionHeader.Misc.VirtualSize));
+        ReadOnlyUpdater (Base, SectionHeader.Misc.VirtualSize);
+      } else {
+        DEBUG ((DEBUG_WARN,
+          "%a: Mapping section %d of image at 0x%lx with RW-XN permissions and size 0x%x\n",
+          __FUNCTION__, Index, Base, SectionHeader.Misc.VirtualSize));
+      }
+    } else {
+        DEBUG ((DEBUG_INFO,
+          "%a: Mapping section %d of image at 0x%lx with RO-XN permissions and size 0x%x\n",
+           __FUNCTION__, Index, Base, SectionHeader.Misc.VirtualSize));
+        ReadOnlyUpdater (Base, SectionHeader.Misc.VirtualSize);
+
+        DEBUG ((DEBUG_INFO,
+          "%a: Mapping section %d of image at 0x%lx with RO-X permissions and size 0x%x\n",
+          __FUNCTION__, Index, Base, SectionHeader.Misc.VirtualSize));
+        NoExecUpdater (Base, SectionHeader.Misc.VirtualSize);
+    }
+
+    SectionHeaderOffset += sizeof (EFI_IMAGE_SECTION_HEADER);
+  }
+  return RETURN_SUCCESS;
+}
 
 /**
   If the build is done on cygwin the paths are cygpaths.
@@ -83,23 +234,29 @@ PeCoffLoaderRelocateImageExtraAction (
   CHAR8 Temp[512];
 #endif
 
+  if (PcdGetBool(PcdStandaloneMmEnable) == TRUE)
+  {
+     UpdatePeCoffPermissions (ImageContext, ArmClearMemoryRegionNoExec,
+                              ArmSetMemoryRegionReadOnly);
+  }
+
   if (ImageContext->PdbPointer) {
 #ifdef __CC_ARM
 #if (__ARMCC_VERSION < 500000)
     // Print out the command for the RVD debugger to load symbols for this image
-    DEBUG ((EFI_D_LOAD | EFI_D_INFO, "load /a /ni /np %a &0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
+    DEBUG ((DEBUG_LOAD | DEBUG_INFO, "load /a /ni /np %a &0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
 #else
     // Print out the command for the DS-5 to load symbols for this image
-    DEBUG ((EFI_D_LOAD | EFI_D_INFO, "add-symbol-file %a 0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
+    DEBUG ((DEBUG_LOAD | DEBUG_INFO, "add-symbol-file %a 0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
 #endif
 #elif __GNUC__
     // This may not work correctly if you generate PE/COFF directlyas then the Offset would not be required
-    DEBUG ((EFI_D_LOAD | EFI_D_INFO, "add-symbol-file %a 0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
+    DEBUG ((DEBUG_LOAD | DEBUG_INFO, "add-symbol-file %a 0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
 #else
-    DEBUG ((EFI_D_LOAD | EFI_D_INFO, "Loading driver at 0x%11p EntryPoint=0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress, FUNCTION_ENTRY_POINT (ImageContext->EntryPoint)));
+    DEBUG ((DEBUG_LOAD | DEBUG_INFO, "Loading driver at 0x%11p EntryPoint=0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress, FUNCTION_ENTRY_POINT (ImageContext->EntryPoint)));
 #endif
   } else {
-    DEBUG ((EFI_D_LOAD | EFI_D_INFO, "Loading driver at 0x%11p EntryPoint=0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress, FUNCTION_ENTRY_POINT (ImageContext->EntryPoint)));
+    DEBUG ((DEBUG_LOAD | DEBUG_INFO, "Loading driver at 0x%11p EntryPoint=0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress, FUNCTION_ENTRY_POINT (ImageContext->EntryPoint)));
   }
 }
 
@@ -125,17 +282,23 @@ PeCoffLoaderUnloadImageExtraAction (
   CHAR8 Temp[512];
 #endif
 
+  if (PcdGetBool(PcdStandaloneMmEnable) == TRUE)
+  {
+     UpdatePeCoffPermissions (ImageContext, ArmSetMemoryRegionNoExec,
+                              ArmClearMemoryRegionReadOnly);
+  }
+
   if (ImageContext->PdbPointer) {
 #ifdef __CC_ARM
     // Print out the command for the RVD debugger to load symbols for this image
-    DEBUG ((EFI_D_ERROR, "unload symbols_only %a\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp))));
+    DEBUG ((DEBUG_ERROR, "unload symbols_only %a\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp))));
 #elif __GNUC__
     // This may not work correctly if you generate PE/COFF directlyas then the Offset would not be required
-    DEBUG ((EFI_D_ERROR, "remove-symbol-file %a 0x%08x\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
+    DEBUG ((DEBUG_ERROR, "remove-symbol-file %a 0x%08x\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
 #else
-    DEBUG ((EFI_D_ERROR, "Unloading %a\n", ImageContext->PdbPointer));
+    DEBUG ((DEBUG_ERROR, "Unloading %a\n", ImageContext->PdbPointer));
 #endif
   } else {
-    DEBUG ((EFI_D_ERROR, "Unloading driver at 0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress));
+    DEBUG ((DEBUG_ERROR, "Unloading driver at 0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress));
   }
 }
diff --git a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.inf b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.inf
index c1f717e..38bf399 100644
--- a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.inf
+++ b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.inf
@@ -33,7 +33,14 @@
   DebugPeCoffExtraActionLib.c
 
 [Packages]
+  ArmPkg/ArmPkg.dec
   MdePkg/MdePkg.dec
+  StandaloneMmPkg/StandaloneMmPkg.dec
+
+[FeaturePcd]
+  gStandaloneMmPkgTokenSpaceGuid.PcdStandaloneMmEnable
 
 [LibraryClasses]
+  ArmMmuLib
   DebugLib
+  PcdLib
-- 
2.7.4

Re: [PATCH 6/6] ArmPkg: Extra action to update permissions for S-ELO MM Image.

[Leif Lindholm]

On Tue, Jul 03, 2018 at 03:25:15PM +0530, Supreeth Venkatesh wrote:
> The Standalone MM drivers runs in S-EL0 in AArch64 on ARM Standard
> Platforms and is deployed during SEC phase. The memory allocated to the
> Standalone MM drivers should be marked as RO+X.
> 
> During PE/COFF Image section parsing, this patch implements extra action
> "UpdatePeCoffPermissions" to request the privileged firmware in EL3 to
> update the permissions.
> 
> 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>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  .../DebugPeCoffExtraActionLib.c                    | 185 +++++++++++++++++++--
>  .../DebugPeCoffExtraActionLib.inf                  |   7 +
>  2 files changed, 181 insertions(+), 11 deletions(-)
> 
> diff --git a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
> index f298e58..c87aaf0 100644
> --- a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
> +++ b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
> @@ -15,14 +15,165 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  **/
>  
>  #include <PiDxe.h>
> -#include <Library/PeCoffLib.h>
>  
> +#include <Library/ArmMmuLib.h>
>  #include <Library/BaseLib.h>
> -#include <Library/DebugLib.h>
>  #include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/PeCoffLib.h>
>  #include <Library/PeCoffExtraActionLib.h>
>  #include <Library/PrintLib.h>
>  
> +typedef RETURN_STATUS (*REGION_PERMISSION_UPDATE_FUNC) (
> +  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
> +  IN  UINT64                    Length
> +  );
> +
> +STATIC
> +RETURN_STATUS
> +UpdatePeCoffPermissions (
> +  IN  CONST PE_COFF_LOADER_IMAGE_CONTEXT      *ImageContext,
> +  IN  REGION_PERMISSION_UPDATE_FUNC           NoExecUpdater,
> +  IN  REGION_PERMISSION_UPDATE_FUNC           ReadOnlyUpdater
> +  )
> +{
> +  RETURN_STATUS                         Status;
> +  EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION   Hdr;
> +  EFI_IMAGE_OPTIONAL_HEADER_UNION       HdrData;
> +  UINTN                                 Size;
> +  UINTN                                 ReadSize;
> +  UINT32                                SectionHeaderOffset;
> +  UINTN                                 NumberOfSections;
> +  UINTN                                 Index;
> +  EFI_IMAGE_SECTION_HEADER              SectionHeader;
> +  PE_COFF_LOADER_IMAGE_CONTEXT          TmpContext;
> +  EFI_PHYSICAL_ADDRESS                  Base;
> +
> +  //
> +  // We need to copy ImageContext since PeCoffLoaderGetImageInfo ()
> +  // will mangle the ImageAddress field
> +  //
> +  CopyMem (&TmpContext, ImageContext, sizeof (TmpContext));
> +
> +  if (TmpContext.PeCoffHeaderOffset == 0) {
> +    Status = PeCoffLoaderGetImageInfo (&TmpContext);
> +    if (RETURN_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR,
> +        "%a: PeCoffLoaderGetImageInfo () failed (Status = %r)\n",
> +        __FUNCTION__, Status));
> +      return Status;
> +    }
> +  }
> +
> +  if (TmpContext.IsTeImage &&
> +      TmpContext.ImageAddress == ImageContext->ImageAddress) {
> +    DEBUG ((DEBUG_INFO, "%a: ignoring XIP TE image at 0x%lx\n", __FUNCTION__,
> +      ImageContext->ImageAddress));
> +    return RETURN_SUCCESS;
> +  }
> +
> +  if (TmpContext.SectionAlignment < EFI_PAGE_SIZE) {
> +    //
> +    // The sections need to be at least 4 KB aligned, since that is the
> +    // granularity at which we can tighten permissions. So just clear the
> +    // noexec permissions on the entire region.
> +    //
> +    if (!TmpContext.IsTeImage) {
> +      DEBUG ((DEBUG_WARN,
> +        "%a: non-TE Image at 0x%lx has SectionAlignment < 4 KB (%lu)\n",
> +        __FUNCTION__, ImageContext->ImageAddress, TmpContext.SectionAlignment));
> +    }
> +    Base = ImageContext->ImageAddress & ~(EFI_PAGE_SIZE - 1);
> +    Size = ImageContext->ImageAddress - Base + ImageContext->ImageSize;
> +    return NoExecUpdater (Base, ALIGN_VALUE (Size, EFI_PAGE_SIZE));
> +  }
> +
> +  //
> +  // Read the PE/COFF Header. For PE32 (32-bit) this will read in too much
> +  // data, but that should not hurt anything. Hdr.Pe32->OptionalHeader.Magic
> +  // determines if this is a PE32 or PE32+ image. The magic is in the same
> +  // location in both images.
> +  //
> +  Hdr.Union = &HdrData;
> +  Size = sizeof (EFI_IMAGE_OPTIONAL_HEADER_UNION);
> +  ReadSize = Size;
> +  Status = TmpContext.ImageRead (TmpContext.Handle,
> +                         TmpContext.PeCoffHeaderOffset, &Size, Hdr.Pe32);
> +  if (RETURN_ERROR (Status) || (Size != ReadSize)) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a: TmpContext.ImageRead () failed (Status = %r)\n",
> +      __FUNCTION__, Status));
> +    return Status;
> +  }
> +
> +  ASSERT (Hdr.Pe32->Signature == EFI_IMAGE_NT_SIGNATURE);
> +
> +  SectionHeaderOffset = TmpContext.PeCoffHeaderOffset + sizeof (UINT32) +
> +                        sizeof (EFI_IMAGE_FILE_HEADER);
> +  NumberOfSections    = (UINTN)(Hdr.Pe32->FileHeader.NumberOfSections);
> +
> +  switch (Hdr.Pe32->OptionalHeader.Magic) {
> +    case EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC:
> +      SectionHeaderOffset += Hdr.Pe32->FileHeader.SizeOfOptionalHeader;
> +      break;
> +    case EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC:
> +      SectionHeaderOffset += Hdr.Pe32Plus->FileHeader.SizeOfOptionalHeader;
> +      break;
> +    default:
> +      ASSERT (FALSE);
> +  }
> +
> +  //
> +  // Iterate over the sections
> +  //
> +  for (Index = 0; Index < NumberOfSections; Index++) {
> +    //
> +    // Read section header from file
> +    //
> +    Size = sizeof (EFI_IMAGE_SECTION_HEADER);
> +    ReadSize = Size;
> +    Status = TmpContext.ImageRead (TmpContext.Handle, SectionHeaderOffset,
> +                                   &Size, &SectionHeader);
> +    if (RETURN_ERROR (Status) || (Size != ReadSize)) {
> +      DEBUG ((DEBUG_ERROR,
> +        "%a: TmpContext.ImageRead () failed (Status = %r)\n",
> +        __FUNCTION__, Status));
> +      return Status;
> +    }
> +
> +    Base = TmpContext.ImageAddress + SectionHeader.VirtualAddress;
> +
> +    if ((SectionHeader.Characteristics & EFI_IMAGE_SCN_MEM_EXECUTE) == 0) {
> +
> +      if ((SectionHeader.Characteristics & EFI_IMAGE_SCN_MEM_WRITE) == 0 &&
> +          TmpContext.ImageType != EFI_IMAGE_SUBSYSTEM_EFI_RUNTIME_DRIVER) {
> +
> +        DEBUG ((DEBUG_INFO,
> +          "%a: Mapping section %d of image at 0x%lx with RO-XN permissions and size 0x%x\n",
> +          __FUNCTION__, Index, Base, SectionHeader.Misc.VirtualSize));
> +        ReadOnlyUpdater (Base, SectionHeader.Misc.VirtualSize);
> +      } else {
> +        DEBUG ((DEBUG_WARN,
> +          "%a: Mapping section %d of image at 0x%lx with RW-XN permissions and size 0x%x\n",
> +          __FUNCTION__, Index, Base, SectionHeader.Misc.VirtualSize));
> +      }
> +    } else {
> +        DEBUG ((DEBUG_INFO,
> +          "%a: Mapping section %d of image at 0x%lx with RO-XN permissions and size 0x%x\n",
> +           __FUNCTION__, Index, Base, SectionHeader.Misc.VirtualSize));
> +        ReadOnlyUpdater (Base, SectionHeader.Misc.VirtualSize);
> +
> +        DEBUG ((DEBUG_INFO,
> +          "%a: Mapping section %d of image at 0x%lx with RO-X permissions and size 0x%x\n",
> +          __FUNCTION__, Index, Base, SectionHeader.Misc.VirtualSize));
> +        NoExecUpdater (Base, SectionHeader.Misc.VirtualSize);
> +    }
> +
> +    SectionHeaderOffset += sizeof (EFI_IMAGE_SECTION_HEADER);
> +  }
> +  return RETURN_SUCCESS;
> +}
>  
>  /**
>    If the build is done on cygwin the paths are cygpaths.
> @@ -83,23 +234,29 @@ PeCoffLoaderRelocateImageExtraAction (
>    CHAR8 Temp[512];
>  #endif
>  
> +  if (PcdGetBool(PcdStandaloneMmEnable) == TRUE)
> +  {

'{' at end of previous line.

> +     UpdatePeCoffPermissions (ImageContext, ArmClearMemoryRegionNoExec,
> +                              ArmSetMemoryRegionReadOnly);
> +  }
> +
>    if (ImageContext->PdbPointer) {
>  #ifdef __CC_ARM
>  #if (__ARMCC_VERSION < 500000)
>      // Print out the command for the RVD debugger to load symbols for this image
> -    DEBUG ((EFI_D_LOAD | EFI_D_INFO, "load /a /ni /np %a &0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
> +    DEBUG ((DEBUG_LOAD | DEBUG_INFO, "load /a /ni /np %a &0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));

This does not look like a functional change.
Please do style changes as a separate patch or not at all.
Applies many times below.

>  #else
>      // Print out the command for the DS-5 to load symbols for this image
> -    DEBUG ((EFI_D_LOAD | EFI_D_INFO, "add-symbol-file %a 0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
> +    DEBUG ((DEBUG_LOAD | DEBUG_INFO, "add-symbol-file %a 0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
>  #endif
>  #elif __GNUC__
>      // This may not work correctly if you generate PE/COFF directlyas then the Offset would not be required
> -    DEBUG ((EFI_D_LOAD | EFI_D_INFO, "add-symbol-file %a 0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
> +    DEBUG ((DEBUG_LOAD | DEBUG_INFO, "add-symbol-file %a 0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
>  #else
> -    DEBUG ((EFI_D_LOAD | EFI_D_INFO, "Loading driver at 0x%11p EntryPoint=0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress, FUNCTION_ENTRY_POINT (ImageContext->EntryPoint)));
> +    DEBUG ((DEBUG_LOAD | DEBUG_INFO, "Loading driver at 0x%11p EntryPoint=0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress, FUNCTION_ENTRY_POINT (ImageContext->EntryPoint)));
>  #endif
>    } else {
> -    DEBUG ((EFI_D_LOAD | EFI_D_INFO, "Loading driver at 0x%11p EntryPoint=0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress, FUNCTION_ENTRY_POINT (ImageContext->EntryPoint)));
> +    DEBUG ((DEBUG_LOAD | DEBUG_INFO, "Loading driver at 0x%11p EntryPoint=0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress, FUNCTION_ENTRY_POINT (ImageContext->EntryPoint)));
>    }
>  }
>  
> @@ -125,17 +282,23 @@ PeCoffLoaderUnloadImageExtraAction (
>    CHAR8 Temp[512];
>  #endif
>  
> +  if (PcdGetBool(PcdStandaloneMmEnable) == TRUE)
> +  {

'{' on preceding line.

/
    Leif

> +     UpdatePeCoffPermissions (ImageContext, ArmSetMemoryRegionNoExec,
> +                              ArmClearMemoryRegionReadOnly);
> +  }
> +
>    if (ImageContext->PdbPointer) {
>  #ifdef __CC_ARM
>      // Print out the command for the RVD debugger to load symbols for this image
> -    DEBUG ((EFI_D_ERROR, "unload symbols_only %a\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp))));
> +    DEBUG ((DEBUG_ERROR, "unload symbols_only %a\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp))));
>  #elif __GNUC__
>      // This may not work correctly if you generate PE/COFF directlyas then the Offset would not be required
> -    DEBUG ((EFI_D_ERROR, "remove-symbol-file %a 0x%08x\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
> +    DEBUG ((DEBUG_ERROR, "remove-symbol-file %a 0x%08x\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
>  #else
> -    DEBUG ((EFI_D_ERROR, "Unloading %a\n", ImageContext->PdbPointer));
> +    DEBUG ((DEBUG_ERROR, "Unloading %a\n", ImageContext->PdbPointer));
>  #endif
>    } else {
> -    DEBUG ((EFI_D_ERROR, "Unloading driver at 0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress));
> +    DEBUG ((DEBUG_ERROR, "Unloading driver at 0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress));
>    }
>  }
> diff --git a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.inf b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.inf
> index c1f717e..38bf399 100644
> --- a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.inf
> +++ b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.inf
> @@ -33,7 +33,14 @@
>    DebugPeCoffExtraActionLib.c
>  
>  [Packages]
> +  ArmPkg/ArmPkg.dec
>    MdePkg/MdePkg.dec
> +  StandaloneMmPkg/StandaloneMmPkg.dec
> +
> +[FeaturePcd]
> +  gStandaloneMmPkgTokenSpaceGuid.PcdStandaloneMmEnable
>  
>  [LibraryClasses]
> +  ArmMmuLib
>    DebugLib
> +  PcdLib
> -- 
> 2.7.4
>