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

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

My response inline.

-----Original Message-----
From: Achin Gupta
Sent: Wednesday, April 11, 2018 2:22 PM
To: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>
Cc: edk2-devel@lists.01.org; michael.d.kinney@intel.com; liming.gao@intel.com; jiewen.yao@intel.com; leif.lindholm@linaro.org; ard.biesheuvel@linaro.org; nd <nd@arm.com>
Subject: Re: [PATCH v1 04/18] ArmPkg/ArmMmuLib: Add MMU Library suitable for use in S-EL0.

Hi Supreeth,

On Fri, Apr 06, 2018 at 03:42:09PM +0100, 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 could still request
> ARM TF to change the memory attributes of memory regions during
> initialization.

The commit message needs more detail to better flesh out why we are doing what we are doing here i.e. the StandaloneMm image is a FV that encapsulates the MM foundation and drivers. These are PE-COFF images with data and text segments. Arm TF does not have visibility of the contents of the FV. Moreover, the driver images are relocated upon dispatch. However, to initialise the MM environment, Arm TF has to create translation tables with sane default attributes for the memory occupied by the FV............

I am hoping you can extrapolate from here and clearly describe what problem this library solves.
[Supreeth] Ok.
>
> This patch adds a simple MMU library suitable for execution in S-EL0
> and requesting operations from higher exception levels.
>
> 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>
> ---
>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuSecLib.c | 146
> ++++++++++++++++++++++++
>  1 file changed, 146 insertions(+)
>  create mode 100644 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuSecLib.c
>
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuSecLib.c
> b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuSecLib.c

I am not sure about the name of the library. ArmMmuSecLib sounds like an MMU library for the SEC phase in the Normal world. Can we call it ArmMmuSecStandaloneMmLib or similar.
[Supreeth] I have renamed it as ArmMmuStandaloneMmCoreLib. Please see version 2.

> new file mode 100644
> index 0000000000..56969e31d1
> --- /dev/null
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuSecLib.c
> @@ -0,0 +1,146 @@
> +/** @file
> +*  File managing the MMU for ARMv8 architecture in S-EL0
> +*
> +*  Copyright (c) 2017, ARM Limited. All rights reserved.

Nit: Copyright 2018? For this and other files?

> +*
> +*  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
> +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
> +  )
> +{
> +  return RequestMemoryPermissionChange(BaseAddress,
> +                                       Length,
> +                                       SET_MEM_ATTR_MAKE_PERM_REQUEST( \
> +                                         SET_MEM_ATTR_DATA_PERM_RO, \
> +                                         SET_MEM_ATTR_CODE_PERM_XN));
> +}
> +
> +EFI_STATUS
> +ArmClearMemoryRegionNoExec (
> +  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
> +  IN  UINT64                    Length
> +  )
> +{
> +  return RequestMemoryPermissionChange(BaseAddress,
> +                                       Length,
> +                                       SET_MEM_ATTR_MAKE_PERM_REQUEST( \
> +                                         SET_MEM_ATTR_DATA_PERM_RO, \
> +                                         SET_MEM_ATTR_CODE_PERM_X));
> +}
> +
> +EFI_STATUS
> +ArmSetMemoryRegionReadOnly (
> +  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
> +  IN  UINT64                    Length
> +  )
> +{
> +  return RequestMemoryPermissionChange(BaseAddress,
> +                                       Length,
> +                                       SET_MEM_ATTR_MAKE_PERM_REQUEST( \
> +                                         SET_MEM_ATTR_DATA_PERM_RO, \
> +                                         SET_MEM_ATTR_CODE_PERM_XN));
> +}
> +
> +EFI_STATUS
> +ArmClearMemoryRegionReadOnly (
> +  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
> +  IN  UINT64                    Length
> +  )
> +{
> +  return RequestMemoryPermissionChange(BaseAddress,
> +                                       Length,
> +                                       SET_MEM_ATTR_MAKE_PERM_REQUEST( \
> +                                         SET_MEM_ATTR_DATA_PERM_RW, \
> +                                         SET_MEM_ATTR_CODE_PERM_XN));
> +}

The above four functions were written as prototypes in the edk2-staging branch. I do not think they are adequate for upstreaming since each function makes assumptions about the current data or instruction access permission of the input memory region instead of only doing what the function's name suggests.

For example, ArmSetMemoryRegionNoExec() is supposed to only set the XN bit. However, it also sets the data access permission to RO. If the region was RW then this will lead to incorrect behaviour. Ditto for the other functions.

We need a GetMemoryPermission() function that first uses the
ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64 call to obtain the memory attributes of the input region. Each of the above functions must use this new function and only change the data or instruction permission attribute as appropriate.
[Supreeth] Ok. Please see version 2.

cheers,
Achin

> +
> +EFI_STATUS
> +EFIAPI
> +ArmConfigureMmu (
> +  IN  ARM_MEMORY_REGION_DESCRIPTOR  *MemoryTable,
> +  OUT VOID                          **TranslationTableBase OPTIONAL,
> +  OUT UINTN                         *TranslationTableSize OPTIONAL
> +  )
> +{
> +  return EFI_UNSUPPORTED;
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +ArmMmuSecLibConstructor (
> +  IN EFI_HANDLE            ImageHandle,
> +  IN EFI_MM_SYSTEM_TABLE   *MmSystemTable
> +  )
> +{
> +  return EFI_SUCCESS;
> +}
> --
> 2.16.2
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.