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

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

On Wed, 2018-05-09 at 21:09 +0000, Udit Kumar wrote:
> 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.
> 

Please refer to https://github.com/ARM-software/arm-trusted-firmware/bl
ob/master/docs/secure-partition-manager-design.rst#communication-
initiated-by-secure-partition for "SP_MEMORY_ATTRIBUTES_GET_AARCH64"
Syntax.

For the time being, SPM in arm-tf does a translation table walk to find
the block or page descriptor that maps the address. It checks whether
the table entry is part of the S-EL0 ( MM) space.

Also, SPM defines resources/address block mappings at boot time for MM
image.  

However, this interface is being modified/updated in secure partition
runtime interface (spec) which will address your concern.
We will revisit this, once this gets implemented in arm-tf.


> 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%2Fo
> > penso
> > 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_R
> > EQUEST
> > +                                            ( \
> > +                                             SET_MEM_ATTR_DATA_PER
> > M_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%2F
> > lists.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