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

Supreeth Venkatesh posted 18 patches 6 years, 6 months ago
There is a newer version of this series
[edk2] [PATCH v1 04/18] ArmPkg/ArmMmuLib: Add MMU Library suitable for use in S-EL0.
Posted by Supreeth Venkatesh 6 years, 6 months ago
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 <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
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.
+*
+*  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));
+}
+
+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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v1 04/18] ArmPkg/ArmMmuLib: Add MMU Library suitable for use in S-EL0.
Posted by Achin Gupta 6 years, 6 months ago
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 <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.

> 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.

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
> 
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v1 04/18] ArmPkg/ArmMmuLib: Add MMU Library suitable for use in S-EL0.
Posted by Supreeth Venkatesh 6 years, 6 months ago
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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel