Re: [PATCH v2 04/17] ArmPkg/ArmMmuLib: Add MMU Library suitable for use in S-EL0.

[Udit Kumar <udit.kumar@nxp.com>]

Hi Supreeth, 
One question on this patch 
We are asking permission on base-address and changing the permission of 
memory based upon base and size. 
I haven't looked at other part of code which manage this , 
But will there be possibility that, base address is given correctly and length 
may over-lap the other MMU entry.

Regards
Udit 

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Supreeth Venkatesh
> Sent: Saturday, May 5, 2018 2:11 AM
> To: edk2-devel@lists.01.org
> Cc: ard.biesheuvel@linaro.org; leif.lindholm@linaro.org; jiewen.yao@intel.com;
> liming.gao@intel.com; michael.d.kinney@intel.com
> Subject: [edk2] [PATCH v2 04/17] ArmPkg/ArmMmuLib: Add MMU Library
> suitable for use in S-EL0.
> 
> 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>
> ---
>  .../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 0000000000..0f5e68d2d4
> --- /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
> +*
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fopenso
> urce.org%2Flicenses%2Fbsd-
> license.php&data=02%7C01%7Cudit.kumar%40nxp.com%7C776b728240f7402b
> 029708d5b1ff7179%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63
> 6610633113917388&sdata=KGmnTNpIKqIXyS2sdVH1I2EaCd8rhm%2BKI05JuxYv8
> Aw%3D&reserved=0
> +*
> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> BASIS,
> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> EXPRESS OR IMPLIED.
> +*
> +**/
> +
> +#include <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) {

You can reduce to 
if( ! GetMemoryPermissions (BaseAddress, &MemoryAttributes)) {
     return RequestMemoryPermissionChange (BaseAddress
}
Status is not used post this. 
GetMemoryPermissions is giving two errors anyway 
 
> +    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.16.2
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.01
> .org%2Fmailman%2Flistinfo%2Fedk2-
> devel&data=02%7C01%7Cudit.kumar%40nxp.com%7C776b728240f7402b02970
> 8d5b1ff7179%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6366106
> 33113917388&sdata=k1Rdt0%2B0CNMRyz66bckTHkT6lFtINt7nc1FaIoZE4%2FA
> %3D&reserved=0