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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Tue, 27 Nov 2018 at 07:20, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
>
> From: Achin Gupta <achin.gupta@arm.com>
>
> 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: Sughosh Ganu <sughosh.ganu@arm.com>
> ---
>  ArmPkg/ArmPkg.dec                                                                                                                                       |   1 +
>  ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf => ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf |  23 +--
>  ArmPkg/Include/Library/{ArmMmuLib.h => StandaloneMmMmuLib.h}                                                                                            |  38 +---
>  ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c                                                                                       | 185 ++++++++++++++++++++
>  4 files changed, 203 insertions(+), 44 deletions(-)
>
> diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
> index 0db7aa9d301c..d99eb6769ff1 100644
> --- a/ArmPkg/ArmPkg.dec
> +++ b/ArmPkg/ArmPkg.dec
> @@ -42,6 +42,7 @@ [LibraryClasses.common]
>    ArmMtlLib|ArmPlatformPkg/Include/Library/ArmMtlLib.h
>    ArmSvcLib|Include/Library/ArmSvcLib.h
>    OpteeLib|Include/Library/OpteeLib.h
> +  StandaloneMmMmuLib|Include/Library/StandaloneMmMmuLib.h
>
>  [Guids.common]
>    gArmTokenSpaceGuid       = { 0xBB11ECFE, 0x820F, 0x4968, { 0xBB, 0xA6, 0xF7, 0x6A, 0xFE, 0x30, 0x25, 0x96 } }
> diff --git a/ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
> similarity index 56%
> copy from ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf
> copy to ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
> index bd6ac8039844..d589b236033c 100644
> --- a/ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf
> +++ b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
> @@ -1,7 +1,6 @@
>  #/** @file
> -#  Implement ArmGenericTimerCounterLib for Xen using the virtual timer
>  #
> -#  Copyright (c) 2014 - 2018, Linaro Ltd. All rights reserved.<BR>
> +#  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
> @@ -15,19 +14,23 @@
>
>  [Defines]
>    INF_VERSION                    = 0x0001001A
> -  BASE_NAME                      = XenArmGenericTimerVirtCounterLib
> -  FILE_GUID                      = e3913319-96ac-4ac0-808b-8edb8776a51c
> -  MODULE_TYPE                    = BASE
> +  BASE_NAME                      = ArmMmuStandaloneMmCoreLib
> +  FILE_GUID                      = da8f0232-fb14-42f0-922c-63104d2c70bd
> +  MODULE_TYPE                    = MM_CORE_STANDALONE
>    VERSION_STRING                 = 1.0
> -  LIBRARY_CLASS                  = ArmGenericTimerCounterLib
> +  LIBRARY_CLASS                  = StandaloneMmMmuLib
> +  PI_SPECIFICATION_VERSION       = 0x00010032
>
> -[Sources]
> -  XenArmGenericTimerVirtCounterLib.c
> +[Sources.AARCH64]
> +  Aarch64/ArmMmuStandaloneMmLib.c
>
>  [Packages]
> -  MdePkg/MdePkg.dec
>    ArmPkg/ArmPkg.dec
> +  MdePkg/MdePkg.dec
>
>  [LibraryClasses]
>    ArmLib
> -  BaseLib
> +  CacheMaintenanceLib
> +  MemoryAllocationLib
> +
> +
> diff --git a/ArmPkg/Include/Library/ArmMmuLib.h b/ArmPkg/Include/Library/StandaloneMmMmuLib.h
> similarity index 55%
> copy from ArmPkg/Include/Library/ArmMmuLib.h
> copy to ArmPkg/Include/Library/StandaloneMmMmuLib.h
> index fb7fd006417c..1f7653d00430 100644
> --- a/ArmPkg/Include/Library/ArmMmuLib.h
> +++ b/ArmPkg/Include/Library/StandaloneMmMmuLib.h
> @@ -1,6 +1,6 @@
>  /** @file
>
> -  Copyright (c) 2015 - 2016, Linaro Ltd. All rights reserved.<BR>
> +  Copyright (c) 2018, ARM Ltd. All rights reserved.
>
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of the BSD License
> @@ -12,61 +12,31 @@
>
>  **/
>
> -#ifndef __ARM_MMU_LIB__
> -#define __ARM_MMU_LIB__
> -
> -#include <Uefi/UefiBaseType.h>
> -
> -#include <Library/ArmLib.h>
> -
> -EFI_STATUS
> -EFIAPI
> -ArmConfigureMmu (
> -  IN  ARM_MEMORY_REGION_DESCRIPTOR  *MemoryTable,
> -  OUT VOID                          **TranslationTableBase OPTIONAL,
> -  OUT UINTN                         *TranslationTableSize  OPTIONAL
> -  );
> +#ifndef __STANDALONEMM_MMU_LIB__
> +#define __STANDALONEMM_MMU_LIB__
>
>  EFI_STATUS
> -EFIAPI
>  ArmSetMemoryRegionNoExec (
>    IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
>    IN  UINT64                    Length
>    );
>
>  EFI_STATUS
> -EFIAPI
>  ArmClearMemoryRegionNoExec (
>    IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
>    IN  UINT64                    Length
>    );
>
>  EFI_STATUS
> -EFIAPI
>  ArmSetMemoryRegionReadOnly (
>    IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
>    IN  UINT64                    Length
>    );
>
>  EFI_STATUS
> -EFIAPI
>  ArmClearMemoryRegionReadOnly (
>    IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
>    IN  UINT64                    Length
>    );
>
> -VOID
> -EFIAPI
> -ArmReplaceLiveTranslationEntry (
> -  IN  UINT64  *Entry,
> -  IN  UINT64  Value
> -  );
> -
> -EFI_STATUS
> -ArmSetMemoryAttributes (
> -  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
> -  IN UINT64                    Length,
> -  IN UINT64                    Attributes
> -  );
> -
> -#endif
> +#endif /* __STANDALONEMM_MMU_LIB__ */
> diff --git a/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c b/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c
> new file mode 100644
> index 000000000000..d7d87b7d5d69
> --- /dev/null
> +++ b/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c
> @@ -0,0 +1,185 @@
> +/** @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>

Why do you need this include? If you can drop it, can you also make
the library generic (i.e., supporting ARM as well as AArch64) ?

(apologies for not spotting this before)

> +#include <IndustryStandard/ArmMmSvc.h>
> +
> +#include <Library/ArmLib.h>
> +#include <Library/ArmMmuLib.h>
> +#include <Library/ArmSvcLib.h>
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +
> +STATIC
> +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;
> +}
> +
> +STATIC
> +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 = EFI_SIZE_TO_PAGES(Length);
> +  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;
> +  UINT32 CodePermission;
> +
> +  Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);
> +  if (Status != EFI_INVALID_PARAMETER) {
> +    CodePermission = SET_MEM_ATTR_CODE_PERM_XN << SET_MEM_ATTR_CODE_PERM_SHIFT;
> +    return RequestMemoryPermissionChange (
> +             BaseAddress,
> +             Length,
> +             MemoryAttributes | CodePermission
> +             );
> +  }
> +  return EFI_INVALID_PARAMETER;
> +}
> +
> +EFI_STATUS
> +ArmClearMemoryRegionNoExec (
> +  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
> +  IN  UINT64                    Length
> +  )
> +{
> +  EFI_STATUS    Status;
> +  UINT32 MemoryAttributes;
> +  UINT32 CodePermission;
> +
> +  Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);
> +  if (Status != EFI_INVALID_PARAMETER) {
> +    CodePermission = SET_MEM_ATTR_CODE_PERM_XN << SET_MEM_ATTR_CODE_PERM_SHIFT;
> +    return RequestMemoryPermissionChange (
> +             BaseAddress,
> +             Length,
> +             MemoryAttributes & ~CodePermission
> +             );
> +  }
> +  return EFI_INVALID_PARAMETER;
> +}
> +
> +EFI_STATUS
> +ArmSetMemoryRegionReadOnly (
> +  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
> +  IN  UINT64                    Length
> +  )
> +{
> +  EFI_STATUS    Status;
> +  UINT32 MemoryAttributes;
> +  UINT32 DataPermission;
> +
> +  Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);
> +  if (Status != EFI_INVALID_PARAMETER) {
> +    DataPermission = SET_MEM_ATTR_DATA_PERM_RO << SET_MEM_ATTR_DATA_PERM_SHIFT;
> +    return RequestMemoryPermissionChange (
> +             BaseAddress,
> +             Length,
> +             MemoryAttributes | DataPermission
> +             );
> +  }
> +  return EFI_INVALID_PARAMETER;
> +}
> +
> +EFI_STATUS
> +ArmClearMemoryRegionReadOnly (
> +  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
> +  IN  UINT64                    Length
> +  )
> +{
> +  EFI_STATUS    Status;
> +  UINT32 MemoryAttributes;
> +  UINT32 PermissionRequest;
> +
> +  Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);
> +  if (Status != EFI_INVALID_PARAMETER) {
> +    PermissionRequest = SET_MEM_ATTR_MAKE_PERM_REQUEST (SET_MEM_ATTR_DATA_PERM_RW, MemoryAttributes);
> +    return RequestMemoryPermissionChange (
> +             BaseAddress,
> +             Length,
> +             PermissionRequest
> +             );
> +  }
> +  return EFI_INVALID_PARAMETER;
> +}
> --
> 2.7.4
>