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

2018-05-04 Thread Supreeth Venkatesh 
My response inline.

-Original Message-
From: Achin Gupta
Sent: Wednesday, April 11, 2018 2:22 PM
To: Supreeth Venkatesh 
Cc: edk2-devel@lists.01.org; michael.d.kin...@intel.com; liming@intel.com; 
jiewen@intel.com; leif.lindh...@linaro.org; ard.biesheu...@linaro.org; nd 

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 
> Signed-off-by: Supreeth Venkatesh 
> ---
>  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 00..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 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +EFI_STATUS
> +RequestMemoryPermissionChange(
> +  IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
> +  IN  UINT64Length,
> +  IN  UINTN Permissions
> +  )
> +{
> +  EFI_STATUSStatus;
> +  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();
> +
> +  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  UINT64Length
> +  )
> +{
> +  return RequestMemoryPermissionChange(BaseAddress,
> +   Length,
> +   SET_MEM_ATTR_MAKE_PERM_REQUEST( \
> +

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

2018-04-11 Thread Achin Gupta 
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.

> 
> 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 
> Signed-off-by: Supreeth Venkatesh 
> ---
>  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.

> new file mode 100644
> index 00..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 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +EFI_STATUS
> +RequestMemoryPermissionChange(
> +  IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
> +  IN  UINT64Length,
> +  IN  UINTN Permissions
> +  )
> +{
> +  EFI_STATUSStatus;
> +  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();
> +
> +  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  UINT64Length
> +  )
> +{
> +  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  UINT64Length
> +  )
> +{
> +  return RequestMemoryPermissionChange(BaseAddress,
> +   Length,
> +   SET_MEM_ATTR_MAKE_PERM_REQUEST( \
> + SET_MEM_ATTR_DATA_PERM_RO, \
> +

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

2018-04-06 Thread Supreeth Venkatesh 
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.

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 
Signed-off-by: Supreeth Venkatesh 
---
 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
new file mode 100644
index 00..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.
+*
+*  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 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+EFI_STATUS
+RequestMemoryPermissionChange(
+  IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
+  IN  UINT64Length,
+  IN  UINTN Permissions
+  )
+{
+  EFI_STATUSStatus;
+  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();
+
+  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  UINT64Length
+  )
+{
+  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  UINT64Length
+  )
+{
+  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  UINT64Length
+  )
+{
+  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  UINT64Length
+  )
+{
+  return RequestMemoryPermissionChange(BaseAddress,
+   Length,
+   SET_MEM_ATTR_MAKE_PERM_REQUEST( \
+ SET_MEM_ATTR_DATA_PERM_RW, \
+ SET_MEM_ATTR_CODE_PERM_XN));
+}
+
+EFI_STATUS
+EFIAPI
+ArmConfigureMmu (
+  IN  ARM_MEMORY_REGION_DESCRIPTOR  *MemoryTable,
+  OUT VOID  **TranslationTableBase OPTIONAL,
+  OUT UINTN