From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=217.140.101.70; helo=foss.arm.com; envelope-from=supreeth.venkatesh@arm.com; receiver=edk2-devel@lists.01.org Received: from foss.arm.com (usa-sjc-mx-foss1.foss.arm.com [217.140.101.70]) by ml01.01.org (Postfix) with ESMTP id BBD7F203BA4EE for ; Thu, 10 May 2018 12:02:48 -0700 (PDT) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id D7C2380D; Thu, 10 May 2018 12:02:47 -0700 (PDT) Received: from [10.0.2.15] (U203142.usa.Arm.com [10.118.30.82]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id 8D12F3F23C; Thu, 10 May 2018 12:02:47 -0700 (PDT) Message-ID: <1525978966.3002.15.camel@arm.com> From: Supreeth Venkatesh To: Udit Kumar , "edk2-devel@lists.01.org" Date: Thu, 10 May 2018 14:02:46 -0500 In-Reply-To: References: <20180504204109.3354-1-supreeth.venkatesh@arm.com> <20180504204109.3354-5-supreeth.venkatesh@arm.com> X-Mailer: Evolution 3.18.5.2-0ubuntu3.2 Mime-Version: 1.0 Subject: Re: [PATCH v2 04/17] ArmPkg/ArmMmuLib: Add MMU Library suitable for use in S-EL0. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 10 May 2018 19:02:48 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit 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 > > Signed-off-by: Supreeth Venkatesh > > --- > >  .../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 > > +#include > > +#include > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +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