* [PATCH v4 1/5] ArmPkg: Add PCDs needed for MM communication driver.
2018-11-27 6:19 [PATCH v4 0/5] ArmPkg related changes for StandaloneMM package Sughosh Ganu
@ 2018-11-27 6:19 ` Sughosh Ganu
2018-11-27 6:19 ` [PATCH v4 2/5] ArmPkg/Drivers: Add EFI_MM_COMMUNICATION_PROTOCOL DXE driver Sughosh Ganu
` (3 subsequent siblings)
4 siblings, 0 replies; 17+ messages in thread
From: Sughosh Ganu @ 2018-11-27 6:19 UTC (permalink / raw)
To: edk2-devel, Ard Biesheuvel, Leif Lindholm, Achin Gupta
From: Achin Gupta <achin.gupta@arm.com>
This patch defines PCDs to describe the base address and size of
communication buffer between normal world (uefi) and standalone MM
environment in the secure world.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Sughosh Ganu <sughosh.ganu@arm.com>
---
ArmPkg/ArmPkg.dec | 3 +++
1 file changed, 3 insertions(+)
diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
index 84e57a0bf01c..0db7aa9d301c 100644
--- a/ArmPkg/ArmPkg.dec
+++ b/ArmPkg/ArmPkg.dec
@@ -240,6 +240,9 @@ [PcdsFixedAtBuild.common, PcdsDynamic.common, PcdsPatchableInModule.common]
gArmTokenSpaceGuid.PcdSystemMemoryBase|0|UINT64|0x00000029
gArmTokenSpaceGuid.PcdSystemMemorySize|0|UINT64|0x0000002A
+ gArmTokenSpaceGuid.PcdMmBufferBase|0|UINT64|0x00000045
+ gArmTokenSpaceGuid.PcdMmBufferSize|0|UINT64|0x00000046
+
[PcdsFixedAtBuild.common, PcdsDynamic.common]
#
# ARM Architectural Timer
--
2.7.4
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v4 2/5] ArmPkg/Drivers: Add EFI_MM_COMMUNICATION_PROTOCOL DXE driver.
2018-11-27 6:19 [PATCH v4 0/5] ArmPkg related changes for StandaloneMM package Sughosh Ganu
2018-11-27 6:19 ` [PATCH v4 1/5] ArmPkg: Add PCDs needed for MM communication driver Sughosh Ganu
@ 2018-11-27 6:19 ` Sughosh Ganu
2018-11-27 7:59 ` Ard Biesheuvel
2018-11-27 6:19 ` [PATCH v4 3/5] ArmPkg/Include: Fix the SPM version SVC ID Sughosh Ganu
` (2 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Sughosh Ganu @ 2018-11-27 6:19 UTC (permalink / raw)
To: edk2-devel, Ard Biesheuvel, Leif Lindholm, Achin Gupta
From: Achin Gupta <achin.gupta@arm.com>
PI v1.5 Specification Volume 4 defines Management Mode Core Interface
and defines EFI_MM_COMMUNICATION_PROTOCOL. This protocol provides a
means of communicating between drivers outside of MM and MMI
handlers inside of MM.
This patch implements the EFI_MM_COMMUNICATION_PROTOCOL DXE runtime
driver for AARCH64 platforms. It uses SMCs allocated from the standard
SMC range defined in DEN0060A_ARM_MM_Interface_Specification.pdf
to communicate with the standalone MM environment in the secure world.
This patch also adds the MM Communication driver (.inf) file to
define entry point for this driver and other compile
related information the driver needs.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Sughosh Ganu <sughosh.ganu@arm.com>
---
ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf | 56 +++
ArmPkg/Drivers/MmCommunicationDxe/MmCommunicate.h | 28 ++
ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c | 378 ++++++++++++++++++++
3 files changed, 462 insertions(+)
diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf
new file mode 100644
index 000000000000..88beafa39c05
--- /dev/null
+++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf
@@ -0,0 +1,56 @@
+#/** @file
+#
+# DXE MM Communicate driver
+#
+# Copyright (c) 2016 - 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
+# 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.
+#
+#**/
+
+[Defines]
+ INF_VERSION = 0x0001001A
+ BASE_NAME = ArmMmCommunication
+ FILE_GUID = 09EE81D3-F15E-43F4-85B4-CB9873DA5D6B
+ MODULE_TYPE = DXE_RUNTIME_DRIVER
+ VERSION_STRING = 1.0
+ ENTRY_POINT = MmCommunicationInitialize
+
+#
+# The following is for reference only and not required by
+# build tools
+#
+# VALID_ARCHITECTURES = AARCH64
+#
+
+[Sources.AARCH64]
+ MmCommunication.c
+
+[Packages]
+ ArmPkg/ArmPkg.dec
+ MdePkg/MdePkg.dec
+
+[LibraryClasses]
+ ArmLib
+ ArmSmcLib
+ BaseMemoryLib
+ DebugLib
+ DxeServicesTableLib
+ HobLib
+ UefiDriverEntryPoint
+
+[Protocols]
+ gEfiMmCommunicationProtocolGuid ## PRODUCES
+
+[Pcd.common]
+ gArmTokenSpaceGuid.PcdMmBufferBase
+ gArmTokenSpaceGuid.PcdMmBufferSize
+
+[Depex]
+ gEfiCpuArchProtocolGuid
diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunicate.h b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunicate.h
new file mode 100644
index 000000000000..0bf1c8d4ca0e
--- /dev/null
+++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunicate.h
@@ -0,0 +1,28 @@
+/** @file
+
+ Copyright (c) 2016-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
+ 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.
+
+**/
+
+#if !defined _MM_COMMUNICATE_H_
+#define _MM_COMMUNICATE_H_
+
+#define MM_MAJOR_VER_MASK 0xEFFF0000
+#define MM_MINOR_VER_MASK 0x0000FFFF
+#define MM_MAJOR_VER_SHIFT 16
+
+#define MM_MAJOR_VER(x) (((x) & MM_MAJOR_VER_MASK) >> MM_MAJOR_VER_SHIFT)
+#define MM_MINOR_VER(x) ((x) & MM_MINOR_VER_MASK)
+
+#define MM_CALLER_MAJOR_VER 0x1UL
+#define MM_CALLER_MINOR_VER 0x0
+
+#endif /* _MM_COMMUNICATE_H_ */
diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
new file mode 100644
index 000000000000..4e1218671d9a
--- /dev/null
+++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
@@ -0,0 +1,378 @@
+/** @file
+
+ Copyright (c) 2016-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
+ 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 <Library/ArmLib.h>
+#include <Library/ArmSmcLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/DxeServicesTableLib.h>
+#include <Library/HobLib.h>
+#include <Library/PcdLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Library/UefiRuntimeServicesTableLib.h>
+
+#include <Protocol/MmCommunication.h>
+
+#include <IndustryStandard/ArmStdSmc.h>
+
+#include "MmCommunicate.h"
+
+//
+// Address, Length of the pre-allocated buffer for communication with the secure
+// world.
+//
+STATIC ARM_MEMORY_REGION_DESCRIPTOR mNsCommBuffMemRegion;
+
+// Notification event when virtual address map is set.
+STATIC EFI_EVENT mSetVirtualAddressMapEvent;
+
+//
+// Handle to install the MM Communication Protocol
+//
+STATIC EFI_HANDLE mMmCommunicateHandle;
+
+/**
+ Communicates with a registered handler.
+
+ This function provides an interface to send and receive messages to the
+ Standalone MM environment on behalf of UEFI services. This function is part
+ of the MM Communication Protocol that may be called in physical mode prior to
+ SetVirtualAddressMap() and in virtual mode after SetVirtualAddressMap().
+
+ @param[in] This The EFI_MM_COMMUNICATION_PROTOCOL
+ instance.
+ @param[in, out] CommBuffer A pointer to the buffer to convey
+ into MMRAM.
+ @param[in, out] CommSize The size of the data buffer being
+ passed in. This is optional.
+
+ @retval EFI_SUCCESS The message was successfully posted.
+ @retval EFI_INVALID_PARAMETER The CommBuffer was NULL.
+ @retval EFI_BAD_BUFFER_SIZE The buffer size is incorrect for the MM
+ implementation. If this error is
+ returned, the MessageLength field in
+ the CommBuffer header or the integer
+ pointed by CommSize are updated to reflect
+ the maximum payload size the
+ implementation can accommodate.
+ @retval EFI_ACCESS_DENIED The CommunicateBuffer parameter
+ or CommSize parameter, if not omitted,
+ are in address range that cannot be
+ accessed by the MM environment
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+MmCommunicationCommunicate (
+ IN CONST EFI_MM_COMMUNICATION_PROTOCOL *This,
+ IN OUT VOID *CommBuffer,
+ IN OUT UINTN *CommSize OPTIONAL
+ )
+{
+ EFI_MM_COMMUNICATE_HEADER *CommunicateHeader;
+ ARM_SMC_ARGS CommunicateSmcArgs;
+ EFI_STATUS Status;
+ UINTN BufferSize;
+
+ Status = EFI_ACCESS_DENIED;
+ BufferSize = 0;
+
+ ZeroMem (&CommunicateSmcArgs, sizeof (ARM_SMC_ARGS));
+
+ //
+ // Check parameters
+ //
+ if (CommBuffer == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ CommunicateHeader = CommBuffer;
+ // CommBuffer is a mandatory parameter. Hence, Rely on
+ // MessageLength + Header to ascertain the
+ // total size of the communication payload rather than
+ // rely on optional CommSize parameter
+ BufferSize = CommunicateHeader->MessageLength +
+ sizeof (CommunicateHeader->HeaderGuid) +
+ sizeof (CommunicateHeader->MessageLength);
+
+ // If the length of the CommBuffer is 0 then return the expected length.
+ if (CommSize) {
+ // This case can be used by the consumer of this driver to find out the
+ // max size that can be used for allocating CommBuffer.
+ if ((*CommSize == 0) ||
+ (*CommSize > mNsCommBuffMemRegion.Length)) {
+ *CommSize = mNsCommBuffMemRegion.Length;
+ return EFI_BAD_BUFFER_SIZE;
+ }
+ //
+ // CommSize must match MessageLength + sizeof (EFI_MM_COMMUNICATE_HEADER);
+ //
+ if (*CommSize != BufferSize) {
+ return EFI_INVALID_PARAMETER;
+ }
+ }
+
+ //
+ // If the buffer size is 0 or greater than what can be tolerated by the MM
+ // environment then return the expected size.
+ //
+ if ((BufferSize == 0) ||
+ (BufferSize > mNsCommBuffMemRegion.Length)) {
+ CommunicateHeader->MessageLength = mNsCommBuffMemRegion.Length -
+ sizeof (CommunicateHeader->HeaderGuid) -
+ sizeof (CommunicateHeader->MessageLength);
+ return EFI_BAD_BUFFER_SIZE;
+ }
+
+ // SMC Function ID
+ CommunicateSmcArgs.Arg0 = ARM_SMC_ID_MM_COMMUNICATE_AARCH64;
+
+ // Cookie
+ CommunicateSmcArgs.Arg1 = 0;
+
+ // Copy Communication Payload
+ CopyMem ((VOID *)mNsCommBuffMemRegion.VirtualBase, CommBuffer, BufferSize);
+
+ // comm_buffer_address (64-bit physical address)
+ CommunicateSmcArgs.Arg2 = (UINTN)mNsCommBuffMemRegion.PhysicalBase;
+
+ // comm_size_address (not used, indicated by setting to zero)
+ CommunicateSmcArgs.Arg3 = 0;
+
+ // Call the Standalone MM environment.
+ ArmCallSmc (&CommunicateSmcArgs);
+
+ switch (CommunicateSmcArgs.Arg0) {
+ case ARM_SMC_MM_RET_SUCCESS:
+ ZeroMem (CommBuffer, BufferSize);
+ // On successful return, the size of data being returned is inferred from
+ // MessageLength + Header.
+ CommunicateHeader = (EFI_MM_COMMUNICATE_HEADER *)mNsCommBuffMemRegion.VirtualBase;
+ BufferSize = CommunicateHeader->MessageLength +
+ sizeof (CommunicateHeader->HeaderGuid) +
+ sizeof (CommunicateHeader->MessageLength);
+
+ CopyMem (
+ CommBuffer,
+ (VOID *)mNsCommBuffMemRegion.VirtualBase,
+ BufferSize
+ );
+ Status = EFI_SUCCESS;
+ break;
+
+ case ARM_SMC_MM_RET_INVALID_PARAMS:
+ Status = EFI_INVALID_PARAMETER;
+ break;
+
+ case ARM_SMC_MM_RET_DENIED:
+ Status = EFI_ACCESS_DENIED;
+ break;
+
+ case ARM_SMC_MM_RET_NO_MEMORY:
+ // Unexpected error since the CommSize was checked for zero length
+ // prior to issuing the SMC
+ Status = EFI_OUT_OF_RESOURCES;
+ ASSERT (0);
+ break;
+
+ default:
+ Status = EFI_ACCESS_DENIED;
+ ASSERT (0);
+ }
+
+ return Status;
+}
+
+//
+// MM Communication Protocol instance
+//
+EFI_MM_COMMUNICATION_PROTOCOL mMmCommunication = {
+ MmCommunicationCommunicate
+};
+
+/**
+ Notification callback on SetVirtualAddressMap event.
+
+ This function notifies the MM communication protocol interface on
+ SetVirtualAddressMap event and converts pointers used in this driver
+ from physical to virtual address.
+
+ @param Event SetVirtualAddressMap event.
+ @param Context A context when the SetVirtualAddressMap triggered.
+
+ @retval EFI_SUCCESS The function executed successfully.
+ @retval Other Some error occurred when executing this function.
+
+**/
+STATIC
+VOID
+EFIAPI
+NotifySetVirtualAddressMap (
+ IN EFI_EVENT Event,
+ IN VOID *Context
+ )
+{
+ EFI_STATUS Status;
+
+ Status = gRT->ConvertPointer (
+ EFI_OPTIONAL_PTR,
+ (VOID **)&mNsCommBuffMemRegion.VirtualBase
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "NotifySetVirtualAddressMap():"
+ " Unable to convert MM runtime pointer. Status:0x%r\n", Status));
+ }
+
+}
+
+STATIC
+EFI_STATUS
+GetMmCompatibility ()
+{
+ EFI_STATUS Status;
+ UINT32 MmVersion;
+ ARM_SMC_ARGS MmVersionArgs;
+
+ // MM_VERSION uses SMC32 calling conventions
+ MmVersionArgs.Arg0 = ARM_SMC_ID_MM_VERSION_AARCH32;
+
+ ArmCallSmc (&MmVersionArgs);
+
+ MmVersion = MmVersionArgs.Arg0;
+
+ if ((MM_MAJOR_VER(MmVersion) == MM_CALLER_MAJOR_VER) &&
+ (MM_MINOR_VER(MmVersion) >= MM_CALLER_MINOR_VER)) {
+ DEBUG ((DEBUG_INFO, "MM Version: Major=0x%x, Minor=0x%x\n",
+ MM_MAJOR_VER(MmVersion), MM_MINOR_VER(MmVersion)));
+ Status = EFI_SUCCESS;
+ } else {
+ DEBUG ((DEBUG_ERROR, "Incompatible MM Versions.\n Current Version: Major=0x%x, Minor=0x%x.\n Expected: Major=0x%x, Minor>=0x%x.\n",
+ MM_MAJOR_VER(MmVersion), MM_MINOR_VER(MmVersion), MM_CALLER_MAJOR_VER, MM_CALLER_MINOR_VER));
+ Status = EFI_UNSUPPORTED;
+ }
+
+ return Status;
+}
+
+/**
+ The Entry Point for MM Communication
+
+ This function installs the MM communication protocol interface and finds out
+ what type of buffer management will be required prior to invoking the
+ communication SMC.
+
+ @param ImageHandle The firmware allocated handle for the EFI image.
+ @param SystemTable A pointer to the EFI System Table.
+
+ @retval EFI_SUCCESS The entry point is executed successfully.
+ @retval Other Some error occurred when executing this entry point.
+
+**/
+EFI_STATUS
+EFIAPI
+MmCommunicationInitialize (
+ IN EFI_HANDLE ImageHandle,
+ IN EFI_SYSTEM_TABLE *SystemTable
+ )
+{
+ EFI_STATUS Status;
+
+ // Check if we can make the MM call
+ Status = GetMmCompatibility ();
+ if (EFI_ERROR(Status)) {
+ goto ReturnErrorStatus;
+ }
+
+ mNsCommBuffMemRegion.PhysicalBase = PcdGet64 (PcdMmBufferBase);
+ // During boot , Virtual and Physical are same
+ mNsCommBuffMemRegion.VirtualBase = mNsCommBuffMemRegion.PhysicalBase;
+ mNsCommBuffMemRegion.Length = PcdGet64 (PcdMmBufferSize);
+
+ ASSERT (mNsCommBuffMemRegion.PhysicalBase != 0);
+
+ ASSERT (mNsCommBuffMemRegion.Length != 0);
+
+ Status = gDS->AddMemorySpace (
+ EfiGcdMemoryTypeReserved,
+ mNsCommBuffMemRegion.PhysicalBase,
+ mNsCommBuffMemRegion.Length,
+ EFI_MEMORY_WB |
+ EFI_MEMORY_XP |
+ EFI_MEMORY_RUNTIME
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: "
+ "Failed to add MM-NS Buffer Memory Space\n"));
+ goto ReturnErrorStatus;
+ }
+
+ Status = gDS->SetMemorySpaceAttributes (
+ mNsCommBuffMemRegion.PhysicalBase,
+ mNsCommBuffMemRegion.Length,
+ EFI_MEMORY_WB | EFI_MEMORY_XP | EFI_MEMORY_RUNTIME
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: "
+ "Failed to set MM-NS Buffer Memory attributes\n"));
+ goto CleanAddedMemorySpace;
+ }
+
+ // Install the communication protocol
+ Status = gBS->InstallProtocolInterface (
+ &mMmCommunicateHandle,
+ &gEfiMmCommunicationProtocolGuid,
+ EFI_NATIVE_INTERFACE,
+ &mMmCommunication
+ );
+ if (EFI_ERROR(Status)) {
+ DEBUG ((DEBUG_ERROR, "MmCommunicationInitialize: "
+ "Failed to install MM communication protocol\n"));
+ goto CleanAllocatedPages;
+ }
+
+ // Register notification callback when virtual address is associated
+ // with the physical address.
+ // Create a Set Virtual Address Map event.
+ Status = gBS->CreateEvent (
+ EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE,
+ TPL_NOTIFY,
+ NotifySetVirtualAddressMap,
+ NULL,
+ &mSetVirtualAddressMapEvent
+ );
+ if (Status == EFI_SUCCESS) {
+ return Status;
+ }
+
+ gBS->UninstallProtocolInterface (
+ mMmCommunicateHandle,
+ &gEfiMmCommunicationProtocolGuid,
+ &mMmCommunication
+ );
+
+CleanAllocatedPages:
+ gBS->FreePages (
+ mNsCommBuffMemRegion.PhysicalBase,
+ EFI_SIZE_TO_PAGES (mNsCommBuffMemRegion.Length)
+ );
+
+CleanAddedMemorySpace:
+ gDS->RemoveMemorySpace (
+ mNsCommBuffMemRegion.PhysicalBase,
+ mNsCommBuffMemRegion.Length
+ );
+
+ReturnErrorStatus:
+ return EFI_INVALID_PARAMETER;
+}
--
2.7.4
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v4 2/5] ArmPkg/Drivers: Add EFI_MM_COMMUNICATION_PROTOCOL DXE driver.
2018-11-27 6:19 ` [PATCH v4 2/5] ArmPkg/Drivers: Add EFI_MM_COMMUNICATION_PROTOCOL DXE driver Sughosh Ganu
@ 2018-11-27 7:59 ` Ard Biesheuvel
2018-11-27 8:12 ` Sughosh Ganu
0 siblings, 1 reply; 17+ messages in thread
From: Ard Biesheuvel @ 2018-11-27 7:59 UTC (permalink / raw)
To: Sughosh Ganu; +Cc: edk2-devel@lists.01.org, Leif Lindholm, Achin Gupta
Hi Sughosh,
On Tue, 27 Nov 2018 at 07:19, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
>
> From: Achin Gupta <achin.gupta@arm.com>
>
> PI v1.5 Specification Volume 4 defines Management Mode Core Interface
> and defines EFI_MM_COMMUNICATION_PROTOCOL. This protocol provides a
> means of communicating between drivers outside of MM and MMI
> handlers inside of MM.
>
> This patch implements the EFI_MM_COMMUNICATION_PROTOCOL DXE runtime
> driver for AARCH64 platforms. It uses SMCs allocated from the standard
> SMC range defined in DEN0060A_ARM_MM_Interface_Specification.pdf
> to communicate with the standalone MM environment in the secure world.
>
> This patch also adds the MM Communication driver (.inf) file to
> define entry point for this driver and other compile
> related information the driver needs.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Sughosh Ganu <sughosh.ganu@arm.com>
> ---
> ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf | 56 +++
> ArmPkg/Drivers/MmCommunicationDxe/MmCommunicate.h | 28 ++
> ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c | 378 ++++++++++++++++++++
> 3 files changed, 462 insertions(+)
>
> diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf
> new file mode 100644
> index 000000000000..88beafa39c05
> --- /dev/null
> +++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf
> @@ -0,0 +1,56 @@
> +#/** @file
> +#
> +# DXE MM Communicate driver
> +#
> +# Copyright (c) 2016 - 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
> +# 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.
> +#
> +#**/
> +
> +[Defines]
> + INF_VERSION = 0x0001001A
> + BASE_NAME = ArmMmCommunication
> + FILE_GUID = 09EE81D3-F15E-43F4-85B4-CB9873DA5D6B
> + MODULE_TYPE = DXE_RUNTIME_DRIVER
> + VERSION_STRING = 1.0
> + ENTRY_POINT = MmCommunicationInitialize
> +
> +#
> +# The following is for reference only and not required by
> +# build tools
> +#
> +# VALID_ARCHITECTURES = AARCH64
> +#
> +
> +[Sources.AARCH64]
> + MmCommunication.c
> +
> +[Packages]
> + ArmPkg/ArmPkg.dec
> + MdePkg/MdePkg.dec
> +
> +[LibraryClasses]
> + ArmLib
> + ArmSmcLib
> + BaseMemoryLib
> + DebugLib
> + DxeServicesTableLib
> + HobLib
> + UefiDriverEntryPoint
> +
> +[Protocols]
> + gEfiMmCommunicationProtocolGuid ## PRODUCES
> +
> +[Pcd.common]
> + gArmTokenSpaceGuid.PcdMmBufferBase
> + gArmTokenSpaceGuid.PcdMmBufferSize
> +
> +[Depex]
> + gEfiCpuArchProtocolGuid
> diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunicate.h b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunicate.h
> new file mode 100644
> index 000000000000..0bf1c8d4ca0e
> --- /dev/null
> +++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunicate.h
> @@ -0,0 +1,28 @@
> +/** @file
> +
> + Copyright (c) 2016-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
> + 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.
> +
> +**/
> +
> +#if !defined _MM_COMMUNICATE_H_
> +#define _MM_COMMUNICATE_H_
> +
> +#define MM_MAJOR_VER_MASK 0xEFFF0000
> +#define MM_MINOR_VER_MASK 0x0000FFFF
> +#define MM_MAJOR_VER_SHIFT 16
> +
> +#define MM_MAJOR_VER(x) (((x) & MM_MAJOR_VER_MASK) >> MM_MAJOR_VER_SHIFT)
> +#define MM_MINOR_VER(x) ((x) & MM_MINOR_VER_MASK)
> +
> +#define MM_CALLER_MAJOR_VER 0x1UL
> +#define MM_CALLER_MINOR_VER 0x0
> +
> +#endif /* _MM_COMMUNICATE_H_ */
> diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
> new file mode 100644
> index 000000000000..4e1218671d9a
> --- /dev/null
> +++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
> @@ -0,0 +1,378 @@
> +/** @file
> +
> + Copyright (c) 2016-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
> + 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 <Library/ArmLib.h>
> +#include <Library/ArmSmcLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/DxeServicesTableLib.h>
> +#include <Library/HobLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiRuntimeServicesTableLib.h>
> +
> +#include <Protocol/MmCommunication.h>
> +
> +#include <IndustryStandard/ArmStdSmc.h>
> +
> +#include "MmCommunicate.h"
> +
> +//
> +// Address, Length of the pre-allocated buffer for communication with the secure
> +// world.
> +//
> +STATIC ARM_MEMORY_REGION_DESCRIPTOR mNsCommBuffMemRegion;
> +
> +// Notification event when virtual address map is set.
> +STATIC EFI_EVENT mSetVirtualAddressMapEvent;
> +
> +//
> +// Handle to install the MM Communication Protocol
> +//
> +STATIC EFI_HANDLE mMmCommunicateHandle;
> +
> +/**
> + Communicates with a registered handler.
> +
> + This function provides an interface to send and receive messages to the
> + Standalone MM environment on behalf of UEFI services. This function is part
> + of the MM Communication Protocol that may be called in physical mode prior to
> + SetVirtualAddressMap() and in virtual mode after SetVirtualAddressMap().
> +
> + @param[in] This The EFI_MM_COMMUNICATION_PROTOCOL
> + instance.
> + @param[in, out] CommBuffer A pointer to the buffer to convey
> + into MMRAM.
> + @param[in, out] CommSize The size of the data buffer being
> + passed in. This is optional.
> +
> + @retval EFI_SUCCESS The message was successfully posted.
> + @retval EFI_INVALID_PARAMETER The CommBuffer was NULL.
> + @retval EFI_BAD_BUFFER_SIZE The buffer size is incorrect for the MM
> + implementation. If this error is
> + returned, the MessageLength field in
> + the CommBuffer header or the integer
> + pointed by CommSize are updated to reflect
> + the maximum payload size the
> + implementation can accommodate.
> + @retval EFI_ACCESS_DENIED The CommunicateBuffer parameter
> + or CommSize parameter, if not omitted,
> + are in address range that cannot be
> + accessed by the MM environment
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +MmCommunicationCommunicate (
> + IN CONST EFI_MM_COMMUNICATION_PROTOCOL *This,
> + IN OUT VOID *CommBuffer,
> + IN OUT UINTN *CommSize OPTIONAL
> + )
> +{
> + EFI_MM_COMMUNICATE_HEADER *CommunicateHeader;
> + ARM_SMC_ARGS CommunicateSmcArgs;
> + EFI_STATUS Status;
> + UINTN BufferSize;
> +
> + Status = EFI_ACCESS_DENIED;
> + BufferSize = 0;
> +
> + ZeroMem (&CommunicateSmcArgs, sizeof (ARM_SMC_ARGS));
> +
> + //
> + // Check parameters
> + //
> + if (CommBuffer == NULL) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + CommunicateHeader = CommBuffer;
> + // CommBuffer is a mandatory parameter. Hence, Rely on
> + // MessageLength + Header to ascertain the
> + // total size of the communication payload rather than
> + // rely on optional CommSize parameter
> + BufferSize = CommunicateHeader->MessageLength +
> + sizeof (CommunicateHeader->HeaderGuid) +
> + sizeof (CommunicateHeader->MessageLength);
> +
> + // If the length of the CommBuffer is 0 then return the expected length.
> + if (CommSize) {
> + // This case can be used by the consumer of this driver to find out the
> + // max size that can be used for allocating CommBuffer.
> + if ((*CommSize == 0) ||
> + (*CommSize > mNsCommBuffMemRegion.Length)) {
> + *CommSize = mNsCommBuffMemRegion.Length;
> + return EFI_BAD_BUFFER_SIZE;
> + }
> + //
> + // CommSize must match MessageLength + sizeof (EFI_MM_COMMUNICATE_HEADER);
> + //
> + if (*CommSize != BufferSize) {
> + return EFI_INVALID_PARAMETER;
> + }
> + }
> +
> + //
> + // If the buffer size is 0 or greater than what can be tolerated by the MM
> + // environment then return the expected size.
> + //
> + if ((BufferSize == 0) ||
> + (BufferSize > mNsCommBuffMemRegion.Length)) {
> + CommunicateHeader->MessageLength = mNsCommBuffMemRegion.Length -
> + sizeof (CommunicateHeader->HeaderGuid) -
> + sizeof (CommunicateHeader->MessageLength);
> + return EFI_BAD_BUFFER_SIZE;
> + }
> +
> + // SMC Function ID
> + CommunicateSmcArgs.Arg0 = ARM_SMC_ID_MM_COMMUNICATE_AARCH64;
> +
> + // Cookie
> + CommunicateSmcArgs.Arg1 = 0;
> +
> + // Copy Communication Payload
> + CopyMem ((VOID *)mNsCommBuffMemRegion.VirtualBase, CommBuffer, BufferSize);
> +
> + // comm_buffer_address (64-bit physical address)
> + CommunicateSmcArgs.Arg2 = (UINTN)mNsCommBuffMemRegion.PhysicalBase;
> +
> + // comm_size_address (not used, indicated by setting to zero)
> + CommunicateSmcArgs.Arg3 = 0;
> +
> + // Call the Standalone MM environment.
> + ArmCallSmc (&CommunicateSmcArgs);
> +
> + switch (CommunicateSmcArgs.Arg0) {
> + case ARM_SMC_MM_RET_SUCCESS:
> + ZeroMem (CommBuffer, BufferSize);
> + // On successful return, the size of data being returned is inferred from
> + // MessageLength + Header.
> + CommunicateHeader = (EFI_MM_COMMUNICATE_HEADER *)mNsCommBuffMemRegion.VirtualBase;
> + BufferSize = CommunicateHeader->MessageLength +
> + sizeof (CommunicateHeader->HeaderGuid) +
> + sizeof (CommunicateHeader->MessageLength);
> +
> + CopyMem (
> + CommBuffer,
> + (VOID *)mNsCommBuffMemRegion.VirtualBase,
> + BufferSize
> + );
> + Status = EFI_SUCCESS;
> + break;
> +
> + case ARM_SMC_MM_RET_INVALID_PARAMS:
> + Status = EFI_INVALID_PARAMETER;
> + break;
> +
> + case ARM_SMC_MM_RET_DENIED:
> + Status = EFI_ACCESS_DENIED;
> + break;
> +
> + case ARM_SMC_MM_RET_NO_MEMORY:
> + // Unexpected error since the CommSize was checked for zero length
> + // prior to issuing the SMC
> + Status = EFI_OUT_OF_RESOURCES;
> + ASSERT (0);
> + break;
> +
> + default:
> + Status = EFI_ACCESS_DENIED;
> + ASSERT (0);
> + }
> +
> + return Status;
> +}
> +
> +//
> +// MM Communication Protocol instance
> +//
> +EFI_MM_COMMUNICATION_PROTOCOL mMmCommunication = {
> + MmCommunicationCommunicate
> +};
> +
> +/**
> + Notification callback on SetVirtualAddressMap event.
> +
> + This function notifies the MM communication protocol interface on
> + SetVirtualAddressMap event and converts pointers used in this driver
> + from physical to virtual address.
> +
> + @param Event SetVirtualAddressMap event.
> + @param Context A context when the SetVirtualAddressMap triggered.
> +
> + @retval EFI_SUCCESS The function executed successfully.
> + @retval Other Some error occurred when executing this function.
> +
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +NotifySetVirtualAddressMap (
> + IN EFI_EVENT Event,
> + IN VOID *Context
> + )
> +{
> + EFI_STATUS Status;
> +
> + Status = gRT->ConvertPointer (
> + EFI_OPTIONAL_PTR,
> + (VOID **)&mNsCommBuffMemRegion.VirtualBase
> + );
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "NotifySetVirtualAddressMap():"
> + " Unable to convert MM runtime pointer. Status:0x%r\n", Status));
> + }
> +
> +}
> +
> +STATIC
> +EFI_STATUS
> +GetMmCompatibility ()
> +{
> + EFI_STATUS Status;
> + UINT32 MmVersion;
> + ARM_SMC_ARGS MmVersionArgs;
> +
> + // MM_VERSION uses SMC32 calling conventions
> + MmVersionArgs.Arg0 = ARM_SMC_ID_MM_VERSION_AARCH32;
> +
> + ArmCallSmc (&MmVersionArgs);
> +
> + MmVersion = MmVersionArgs.Arg0;
> +
> + if ((MM_MAJOR_VER(MmVersion) == MM_CALLER_MAJOR_VER) &&
> + (MM_MINOR_VER(MmVersion) >= MM_CALLER_MINOR_VER)) {
> + DEBUG ((DEBUG_INFO, "MM Version: Major=0x%x, Minor=0x%x\n",
> + MM_MAJOR_VER(MmVersion), MM_MINOR_VER(MmVersion)));
> + Status = EFI_SUCCESS;
> + } else {
> + DEBUG ((DEBUG_ERROR, "Incompatible MM Versions.\n Current Version: Major=0x%x, Minor=0x%x.\n Expected: Major=0x%x, Minor>=0x%x.\n",
> + MM_MAJOR_VER(MmVersion), MM_MINOR_VER(MmVersion), MM_CALLER_MAJOR_VER, MM_CALLER_MINOR_VER));
> + Status = EFI_UNSUPPORTED;
> + }
> +
> + return Status;
> +}
> +
> +/**
> + The Entry Point for MM Communication
> +
> + This function installs the MM communication protocol interface and finds out
> + what type of buffer management will be required prior to invoking the
> + communication SMC.
> +
> + @param ImageHandle The firmware allocated handle for the EFI image.
> + @param SystemTable A pointer to the EFI System Table.
> +
> + @retval EFI_SUCCESS The entry point is executed successfully.
> + @retval Other Some error occurred when executing this entry point.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +MmCommunicationInitialize (
> + IN EFI_HANDLE ImageHandle,
> + IN EFI_SYSTEM_TABLE *SystemTable
> + )
> +{
> + EFI_STATUS Status;
> +
> + // Check if we can make the MM call
> + Status = GetMmCompatibility ();
> + if (EFI_ERROR(Status)) {
> + goto ReturnErrorStatus;
> + }
> +
> + mNsCommBuffMemRegion.PhysicalBase = PcdGet64 (PcdMmBufferBase);
> + // During boot , Virtual and Physical are same
> + mNsCommBuffMemRegion.VirtualBase = mNsCommBuffMemRegion.PhysicalBase;
> + mNsCommBuffMemRegion.Length = PcdGet64 (PcdMmBufferSize);
> +
> + ASSERT (mNsCommBuffMemRegion.PhysicalBase != 0);
> +
> + ASSERT (mNsCommBuffMemRegion.Length != 0);
> +
> + Status = gDS->AddMemorySpace (
> + EfiGcdMemoryTypeReserved,
> + mNsCommBuffMemRegion.PhysicalBase,
> + mNsCommBuffMemRegion.Length,
> + EFI_MEMORY_WB |
> + EFI_MEMORY_XP |
> + EFI_MEMORY_RUNTIME
> + );
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: "
> + "Failed to add MM-NS Buffer Memory Space\n"));
> + goto ReturnErrorStatus;
> + }
> +
> + Status = gDS->SetMemorySpaceAttributes (
> + mNsCommBuffMemRegion.PhysicalBase,
> + mNsCommBuffMemRegion.Length,
> + EFI_MEMORY_WB | EFI_MEMORY_XP | EFI_MEMORY_RUNTIME
> + );
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: "
> + "Failed to set MM-NS Buffer Memory attributes\n"));
> + goto CleanAddedMemorySpace;
> + }
> +
> + // Install the communication protocol
> + Status = gBS->InstallProtocolInterface (
> + &mMmCommunicateHandle,
> + &gEfiMmCommunicationProtocolGuid,
> + EFI_NATIVE_INTERFACE,
> + &mMmCommunication
> + );
> + if (EFI_ERROR(Status)) {
> + DEBUG ((DEBUG_ERROR, "MmCommunicationInitialize: "
> + "Failed to install MM communication protocol\n"));
> + goto CleanAllocatedPages;
> + }
> +
> + // Register notification callback when virtual address is associated
> + // with the physical address.
> + // Create a Set Virtual Address Map event.
> + Status = gBS->CreateEvent (
> + EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE,
> + TPL_NOTIFY,
> + NotifySetVirtualAddressMap,
> + NULL,
> + &mSetVirtualAddressMapEvent
> + );
> + if (Status == EFI_SUCCESS) {
> + return Status;
> + }
> +
> + gBS->UninstallProtocolInterface (
> + mMmCommunicateHandle,
> + &gEfiMmCommunicationProtocolGuid,
> + &mMmCommunication
> + );
> +
> +CleanAllocatedPages:
> + gBS->FreePages (
> + mNsCommBuffMemRegion.PhysicalBase,
> + EFI_SIZE_TO_PAGES (mNsCommBuffMemRegion.Length)
> + );
> +
Do we still need this?
> +CleanAddedMemorySpace:
> + gDS->RemoveMemorySpace (
> + mNsCommBuffMemRegion.PhysicalBase,
> + mNsCommBuffMemRegion.Length
> + );
> +
> +ReturnErrorStatus:
> + return EFI_INVALID_PARAMETER;
> +}
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v4 2/5] ArmPkg/Drivers: Add EFI_MM_COMMUNICATION_PROTOCOL DXE driver.
2018-11-27 7:59 ` Ard Biesheuvel
@ 2018-11-27 8:12 ` Sughosh Ganu
0 siblings, 0 replies; 17+ messages in thread
From: Sughosh Ganu @ 2018-11-27 8:12 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel@lists.01.org, Leif Lindholm, Achin Gupta
hi Ard,
On Tue Nov 27, 2018 at 08:59:30AM +0100, Ard Biesheuvel wrote:
> Hi Sughosh,
>
> On Tue, 27 Nov 2018 at 07:19, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
> >
> > From: Achin Gupta <achin.gupta@arm.com>
> >
> > PI v1.5 Specification Volume 4 defines Management Mode Core Interface
> > and defines EFI_MM_COMMUNICATION_PROTOCOL. This protocol provides a
> > means of communicating between drivers outside of MM and MMI
> > handlers inside of MM.
> >
> > This patch implements the EFI_MM_COMMUNICATION_PROTOCOL DXE runtime
> > driver for AARCH64 platforms. It uses SMCs allocated from the standard
> > SMC range defined in DEN0060A_ARM_MM_Interface_Specification.pdf
> > to communicate with the standalone MM environment in the secure world.
> >
> > This patch also adds the MM Communication driver (.inf) file to
> > define entry point for this driver and other compile
> > related information the driver needs.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@arm.com>
> > ---
> > ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf | 56 +++
> > ArmPkg/Drivers/MmCommunicationDxe/MmCommunicate.h | 28 ++
> > ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c | 378 ++++++++++++++++++++
> > 3 files changed, 462 insertions(+)
<snip>
> > +/**
> > + The Entry Point for MM Communication
> > +
> > + This function installs the MM communication protocol interface and finds out
> > + what type of buffer management will be required prior to invoking the
> > + communication SMC.
> > +
> > + @param ImageHandle The firmware allocated handle for the EFI image.
> > + @param SystemTable A pointer to the EFI System Table.
> > +
> > + @retval EFI_SUCCESS The entry point is executed successfully.
> > + @retval Other Some error occurred when executing this entry point.
> > +
> > +**/
> > +EFI_STATUS
> > +EFIAPI
> > +MmCommunicationInitialize (
> > + IN EFI_HANDLE ImageHandle,
> > + IN EFI_SYSTEM_TABLE *SystemTable
> > + )
> > +{
> > + EFI_STATUS Status;
> > +
> > + // Check if we can make the MM call
> > + Status = GetMmCompatibility ();
> > + if (EFI_ERROR(Status)) {
> > + goto ReturnErrorStatus;
> > + }
> > +
> > + mNsCommBuffMemRegion.PhysicalBase = PcdGet64 (PcdMmBufferBase);
> > + // During boot , Virtual and Physical are same
> > + mNsCommBuffMemRegion.VirtualBase = mNsCommBuffMemRegion.PhysicalBase;
> > + mNsCommBuffMemRegion.Length = PcdGet64 (PcdMmBufferSize);
> > +
> > + ASSERT (mNsCommBuffMemRegion.PhysicalBase != 0);
> > +
> > + ASSERT (mNsCommBuffMemRegion.Length != 0);
> > +
> > + Status = gDS->AddMemorySpace (
> > + EfiGcdMemoryTypeReserved,
> > + mNsCommBuffMemRegion.PhysicalBase,
> > + mNsCommBuffMemRegion.Length,
> > + EFI_MEMORY_WB |
> > + EFI_MEMORY_XP |
> > + EFI_MEMORY_RUNTIME
> > + );
> > + if (EFI_ERROR (Status)) {
> > + DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: "
> > + "Failed to add MM-NS Buffer Memory Space\n"));
> > + goto ReturnErrorStatus;
> > + }
> > +
> > + Status = gDS->SetMemorySpaceAttributes (
> > + mNsCommBuffMemRegion.PhysicalBase,
> > + mNsCommBuffMemRegion.Length,
> > + EFI_MEMORY_WB | EFI_MEMORY_XP | EFI_MEMORY_RUNTIME
> > + );
> > + if (EFI_ERROR (Status)) {
> > + DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: "
> > + "Failed to set MM-NS Buffer Memory attributes\n"));
> > + goto CleanAddedMemorySpace;
> > + }
> > +
> > + // Install the communication protocol
> > + Status = gBS->InstallProtocolInterface (
> > + &mMmCommunicateHandle,
> > + &gEfiMmCommunicationProtocolGuid,
> > + EFI_NATIVE_INTERFACE,
> > + &mMmCommunication
> > + );
> > + if (EFI_ERROR(Status)) {
> > + DEBUG ((DEBUG_ERROR, "MmCommunicationInitialize: "
> > + "Failed to install MM communication protocol\n"));
> > + goto CleanAllocatedPages;
> > + }
> > +
> > + // Register notification callback when virtual address is associated
> > + // with the physical address.
> > + // Create a Set Virtual Address Map event.
> > + Status = gBS->CreateEvent (
> > + EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE,
> > + TPL_NOTIFY,
> > + NotifySetVirtualAddressMap,
> > + NULL,
> > + &mSetVirtualAddressMapEvent
> > + );
> > + if (Status == EFI_SUCCESS) {
> > + return Status;
> > + }
> > +
> > + gBS->UninstallProtocolInterface (
> > + mMmCommunicateHandle,
> > + &gEfiMmCommunicationProtocolGuid,
> > + &mMmCommunication
> > + );
> > +
> > +CleanAllocatedPages:
> > + gBS->FreePages (
> > + mNsCommBuffMemRegion.PhysicalBase,
> > + EFI_SIZE_TO_PAGES (mNsCommBuffMemRegion.Length)
> > + );
> > +
>
> Do we still need this?
Oops. No this can now be removed. Will post a V6. Thanks.
-sughosh
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v4 3/5] ArmPkg/Include: Fix the SPM version SVC ID
2018-11-27 6:19 [PATCH v4 0/5] ArmPkg related changes for StandaloneMM package Sughosh Ganu
2018-11-27 6:19 ` [PATCH v4 1/5] ArmPkg: Add PCDs needed for MM communication driver Sughosh Ganu
2018-11-27 6:19 ` [PATCH v4 2/5] ArmPkg/Drivers: Add EFI_MM_COMMUNICATION_PROTOCOL DXE driver Sughosh Ganu
@ 2018-11-27 6:19 ` Sughosh Ganu
2018-11-27 6:19 ` [PATCH v4 4/5] ArmPkg/Include: Add MM interface SVC return codes Sughosh Ganu
2018-11-27 6:19 ` [PATCH v4 5/5] ArmPkg/ArmMmuLib: Add MMU Library suitable for use in S-EL0 Sughosh Ganu
4 siblings, 0 replies; 17+ messages in thread
From: Sughosh Ganu @ 2018-11-27 6:19 UTC (permalink / raw)
To: edk2-devel, Ard Biesheuvel, Leif Lindholm, Achin Gupta
The MM_VERSION SMC call uses SMC32 calling convention. Fix the macro
to reflect the correct value.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Sughosh Ganu <sughosh.ganu@arm.com>
---
ArmPkg/Include/IndustryStandard/ArmMmSvc.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ArmPkg/Include/IndustryStandard/ArmMmSvc.h b/ArmPkg/Include/IndustryStandard/ArmMmSvc.h
index 4c7b6c338627..81b4654fa5dd 100644
--- a/ArmPkg/Include/IndustryStandard/ArmMmSvc.h
+++ b/ArmPkg/Include/IndustryStandard/ArmMmSvc.h
@@ -20,7 +20,7 @@
* delegated events and request the Secure partition manager to perform
* privileged operations on its behalf.
*/
-#define ARM_SVC_ID_SPM_VERSION_AARCH64 0xC4000060
+#define ARM_SVC_ID_SPM_VERSION_AARCH32 0x84000060
#define ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64 0xC4000061
#define ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64 0xC4000064
#define ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64 0xC4000065
--
2.7.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v4 4/5] ArmPkg/Include: Add MM interface SVC return codes.
2018-11-27 6:19 [PATCH v4 0/5] ArmPkg related changes for StandaloneMM package Sughosh Ganu
` (2 preceding siblings ...)
2018-11-27 6:19 ` [PATCH v4 3/5] ArmPkg/Include: Fix the SPM version SVC ID Sughosh Ganu
@ 2018-11-27 6:19 ` Sughosh Ganu
2018-11-27 6:19 ` [PATCH v4 5/5] ArmPkg/ArmMmuLib: Add MMU Library suitable for use in S-EL0 Sughosh Ganu
4 siblings, 0 replies; 17+ messages in thread
From: Sughosh Ganu @ 2018-11-27 6:19 UTC (permalink / raw)
To: edk2-devel, Ard Biesheuvel, Leif Lindholm, Achin Gupta
From: Achin Gupta <achin.gupta@arm.com>
This patch adds the Management Mode(MM) - Secure Partition
Manager(SPM) SVC return codes.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Sughosh Ganu <sughosh.ganu@arm.com>
---
ArmPkg/Include/IndustryStandard/ArmMmSvc.h | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/ArmPkg/Include/IndustryStandard/ArmMmSvc.h b/ArmPkg/Include/IndustryStandard/ArmMmSvc.h
index 81b4654fa5dd..a64b9ec23c4b 100644
--- a/ArmPkg/Include/IndustryStandard/ArmMmSvc.h
+++ b/ArmPkg/Include/IndustryStandard/ArmMmSvc.h
@@ -40,4 +40,11 @@
((((c_perm) & SET_MEM_ATTR_CODE_PERM_MASK) << SET_MEM_ATTR_CODE_PERM_SHIFT) | \
(( (d_perm) & SET_MEM_ATTR_DATA_PERM_MASK) << SET_MEM_ATTR_DATA_PERM_SHIFT))
+/* MM SVC Return error codes */
+#define ARM_SVC_SPM_RET_SUCCESS 0
+#define ARM_SVC_SPM_RET_NOT_SUPPORTED -1
+#define ARM_SVC_SPM_RET_INVALID_PARAMS -2
+#define ARM_SVC_SPM_RET_DENIED -3
+#define ARM_SVC_SPM_RET_NO_MEMORY -5
+
#endif
--
2.7.4
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v4 5/5] ArmPkg/ArmMmuLib: Add MMU Library suitable for use in S-EL0.
2018-11-27 6:19 [PATCH v4 0/5] ArmPkg related changes for StandaloneMM package Sughosh Ganu
` (3 preceding siblings ...)
2018-11-27 6:19 ` [PATCH v4 4/5] ArmPkg/Include: Add MM interface SVC return codes Sughosh Ganu
@ 2018-11-27 6:19 ` Sughosh Ganu
2018-11-27 8:14 ` Ard Biesheuvel
4 siblings, 1 reply; 17+ messages in thread
From: Sughosh Ganu @ 2018-11-27 6:19 UTC (permalink / raw)
To: edk2-devel, Ard Biesheuvel, Leif Lindholm, Achin Gupta
From: Achin Gupta <achin.gupta@arm.com>
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: Sughosh Ganu <sughosh.ganu@arm.com>
---
ArmPkg/ArmPkg.dec | 1 +
ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf => ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf | 23 +--
ArmPkg/Include/Library/{ArmMmuLib.h => StandaloneMmMmuLib.h} | 38 +---
ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c | 185 ++++++++++++++++++++
4 files changed, 203 insertions(+), 44 deletions(-)
diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
index 0db7aa9d301c..d99eb6769ff1 100644
--- a/ArmPkg/ArmPkg.dec
+++ b/ArmPkg/ArmPkg.dec
@@ -42,6 +42,7 @@ [LibraryClasses.common]
ArmMtlLib|ArmPlatformPkg/Include/Library/ArmMtlLib.h
ArmSvcLib|Include/Library/ArmSvcLib.h
OpteeLib|Include/Library/OpteeLib.h
+ StandaloneMmMmuLib|Include/Library/StandaloneMmMmuLib.h
[Guids.common]
gArmTokenSpaceGuid = { 0xBB11ECFE, 0x820F, 0x4968, { 0xBB, 0xA6, 0xF7, 0x6A, 0xFE, 0x30, 0x25, 0x96 } }
diff --git a/ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
similarity index 56%
copy from ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf
copy to ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
index bd6ac8039844..d589b236033c 100644
--- a/ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf
+++ b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
@@ -1,7 +1,6 @@
#/** @file
-# Implement ArmGenericTimerCounterLib for Xen using the virtual timer
#
-# Copyright (c) 2014 - 2018, Linaro Ltd. All rights reserved.<BR>
+# 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
@@ -15,19 +14,23 @@
[Defines]
INF_VERSION = 0x0001001A
- BASE_NAME = XenArmGenericTimerVirtCounterLib
- FILE_GUID = e3913319-96ac-4ac0-808b-8edb8776a51c
- MODULE_TYPE = BASE
+ BASE_NAME = ArmMmuStandaloneMmCoreLib
+ FILE_GUID = da8f0232-fb14-42f0-922c-63104d2c70bd
+ MODULE_TYPE = MM_CORE_STANDALONE
VERSION_STRING = 1.0
- LIBRARY_CLASS = ArmGenericTimerCounterLib
+ LIBRARY_CLASS = StandaloneMmMmuLib
+ PI_SPECIFICATION_VERSION = 0x00010032
-[Sources]
- XenArmGenericTimerVirtCounterLib.c
+[Sources.AARCH64]
+ Aarch64/ArmMmuStandaloneMmLib.c
[Packages]
- MdePkg/MdePkg.dec
ArmPkg/ArmPkg.dec
+ MdePkg/MdePkg.dec
[LibraryClasses]
ArmLib
- BaseLib
+ CacheMaintenanceLib
+ MemoryAllocationLib
+
+
diff --git a/ArmPkg/Include/Library/ArmMmuLib.h b/ArmPkg/Include/Library/StandaloneMmMmuLib.h
similarity index 55%
copy from ArmPkg/Include/Library/ArmMmuLib.h
copy to ArmPkg/Include/Library/StandaloneMmMmuLib.h
index fb7fd006417c..1f7653d00430 100644
--- a/ArmPkg/Include/Library/ArmMmuLib.h
+++ b/ArmPkg/Include/Library/StandaloneMmMmuLib.h
@@ -1,6 +1,6 @@
/** @file
- Copyright (c) 2015 - 2016, Linaro Ltd. All rights reserved.<BR>
+ Copyright (c) 2018, ARM Ltd. All rights reserved.
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD License
@@ -12,61 +12,31 @@
**/
-#ifndef __ARM_MMU_LIB__
-#define __ARM_MMU_LIB__
-
-#include <Uefi/UefiBaseType.h>
-
-#include <Library/ArmLib.h>
-
-EFI_STATUS
-EFIAPI
-ArmConfigureMmu (
- IN ARM_MEMORY_REGION_DESCRIPTOR *MemoryTable,
- OUT VOID **TranslationTableBase OPTIONAL,
- OUT UINTN *TranslationTableSize OPTIONAL
- );
+#ifndef __STANDALONEMM_MMU_LIB__
+#define __STANDALONEMM_MMU_LIB__
EFI_STATUS
-EFIAPI
ArmSetMemoryRegionNoExec (
IN EFI_PHYSICAL_ADDRESS BaseAddress,
IN UINT64 Length
);
EFI_STATUS
-EFIAPI
ArmClearMemoryRegionNoExec (
IN EFI_PHYSICAL_ADDRESS BaseAddress,
IN UINT64 Length
);
EFI_STATUS
-EFIAPI
ArmSetMemoryRegionReadOnly (
IN EFI_PHYSICAL_ADDRESS BaseAddress,
IN UINT64 Length
);
EFI_STATUS
-EFIAPI
ArmClearMemoryRegionReadOnly (
IN EFI_PHYSICAL_ADDRESS BaseAddress,
IN UINT64 Length
);
-VOID
-EFIAPI
-ArmReplaceLiveTranslationEntry (
- IN UINT64 *Entry,
- IN UINT64 Value
- );
-
-EFI_STATUS
-ArmSetMemoryAttributes (
- IN EFI_PHYSICAL_ADDRESS BaseAddress,
- IN UINT64 Length,
- IN UINT64 Attributes
- );
-
-#endif
+#endif /* __STANDALONEMM_MMU_LIB__ */
diff --git a/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c b/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c
new file mode 100644
index 000000000000..d7d87b7d5d69
--- /dev/null
+++ b/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c
@@ -0,0 +1,185 @@
+/** @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
+* 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>
+
+STATIC
+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;
+}
+
+STATIC
+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 = EFI_SIZE_TO_PAGES(Length);
+ 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;
+ UINT32 CodePermission;
+
+ Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);
+ if (Status != EFI_INVALID_PARAMETER) {
+ CodePermission = SET_MEM_ATTR_CODE_PERM_XN << SET_MEM_ATTR_CODE_PERM_SHIFT;
+ return RequestMemoryPermissionChange (
+ BaseAddress,
+ Length,
+ MemoryAttributes | CodePermission
+ );
+ }
+ return EFI_INVALID_PARAMETER;
+}
+
+EFI_STATUS
+ArmClearMemoryRegionNoExec (
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINT64 Length
+ )
+{
+ EFI_STATUS Status;
+ UINT32 MemoryAttributes;
+ UINT32 CodePermission;
+
+ Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);
+ if (Status != EFI_INVALID_PARAMETER) {
+ CodePermission = SET_MEM_ATTR_CODE_PERM_XN << SET_MEM_ATTR_CODE_PERM_SHIFT;
+ return RequestMemoryPermissionChange (
+ BaseAddress,
+ Length,
+ MemoryAttributes & ~CodePermission
+ );
+ }
+ return EFI_INVALID_PARAMETER;
+}
+
+EFI_STATUS
+ArmSetMemoryRegionReadOnly (
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINT64 Length
+ )
+{
+ EFI_STATUS Status;
+ UINT32 MemoryAttributes;
+ UINT32 DataPermission;
+
+ Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);
+ if (Status != EFI_INVALID_PARAMETER) {
+ DataPermission = SET_MEM_ATTR_DATA_PERM_RO << SET_MEM_ATTR_DATA_PERM_SHIFT;
+ return RequestMemoryPermissionChange (
+ BaseAddress,
+ Length,
+ MemoryAttributes | DataPermission
+ );
+ }
+ return EFI_INVALID_PARAMETER;
+}
+
+EFI_STATUS
+ArmClearMemoryRegionReadOnly (
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINT64 Length
+ )
+{
+ EFI_STATUS Status;
+ UINT32 MemoryAttributes;
+ UINT32 PermissionRequest;
+
+ Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);
+ if (Status != EFI_INVALID_PARAMETER) {
+ PermissionRequest = SET_MEM_ATTR_MAKE_PERM_REQUEST (SET_MEM_ATTR_DATA_PERM_RW, MemoryAttributes);
+ return RequestMemoryPermissionChange (
+ BaseAddress,
+ Length,
+ PermissionRequest
+ );
+ }
+ return EFI_INVALID_PARAMETER;
+}
--
2.7.4
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v4 5/5] ArmPkg/ArmMmuLib: Add MMU Library suitable for use in S-EL0.
2018-11-27 6:19 ` [PATCH v4 5/5] ArmPkg/ArmMmuLib: Add MMU Library suitable for use in S-EL0 Sughosh Ganu
@ 2018-11-27 8:14 ` Ard Biesheuvel
2018-11-27 8:36 ` Sughosh Ganu
0 siblings, 1 reply; 17+ messages in thread
From: Ard Biesheuvel @ 2018-11-27 8:14 UTC (permalink / raw)
To: Sughosh Ganu; +Cc: edk2-devel@lists.01.org, Leif Lindholm, Achin Gupta
On Tue, 27 Nov 2018 at 07:20, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
>
> From: Achin Gupta <achin.gupta@arm.com>
>
> 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: Sughosh Ganu <sughosh.ganu@arm.com>
> ---
> ArmPkg/ArmPkg.dec | 1 +
> ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf => ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf | 23 +--
> ArmPkg/Include/Library/{ArmMmuLib.h => StandaloneMmMmuLib.h} | 38 +---
> ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c | 185 ++++++++++++++++++++
> 4 files changed, 203 insertions(+), 44 deletions(-)
>
> diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
> index 0db7aa9d301c..d99eb6769ff1 100644
> --- a/ArmPkg/ArmPkg.dec
> +++ b/ArmPkg/ArmPkg.dec
> @@ -42,6 +42,7 @@ [LibraryClasses.common]
> ArmMtlLib|ArmPlatformPkg/Include/Library/ArmMtlLib.h
> ArmSvcLib|Include/Library/ArmSvcLib.h
> OpteeLib|Include/Library/OpteeLib.h
> + StandaloneMmMmuLib|Include/Library/StandaloneMmMmuLib.h
>
> [Guids.common]
> gArmTokenSpaceGuid = { 0xBB11ECFE, 0x820F, 0x4968, { 0xBB, 0xA6, 0xF7, 0x6A, 0xFE, 0x30, 0x25, 0x96 } }
> diff --git a/ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
> similarity index 56%
> copy from ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf
> copy to ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
> index bd6ac8039844..d589b236033c 100644
> --- a/ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf
> +++ b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
> @@ -1,7 +1,6 @@
> #/** @file
> -# Implement ArmGenericTimerCounterLib for Xen using the virtual timer
> #
> -# Copyright (c) 2014 - 2018, Linaro Ltd. All rights reserved.<BR>
> +# 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
> @@ -15,19 +14,23 @@
>
> [Defines]
> INF_VERSION = 0x0001001A
> - BASE_NAME = XenArmGenericTimerVirtCounterLib
> - FILE_GUID = e3913319-96ac-4ac0-808b-8edb8776a51c
> - MODULE_TYPE = BASE
> + BASE_NAME = ArmMmuStandaloneMmCoreLib
> + FILE_GUID = da8f0232-fb14-42f0-922c-63104d2c70bd
> + MODULE_TYPE = MM_CORE_STANDALONE
> VERSION_STRING = 1.0
> - LIBRARY_CLASS = ArmGenericTimerCounterLib
> + LIBRARY_CLASS = StandaloneMmMmuLib
> + PI_SPECIFICATION_VERSION = 0x00010032
>
> -[Sources]
> - XenArmGenericTimerVirtCounterLib.c
> +[Sources.AARCH64]
> + Aarch64/ArmMmuStandaloneMmLib.c
>
> [Packages]
> - MdePkg/MdePkg.dec
> ArmPkg/ArmPkg.dec
> + MdePkg/MdePkg.dec
>
> [LibraryClasses]
> ArmLib
> - BaseLib
> + CacheMaintenanceLib
> + MemoryAllocationLib
> +
> +
> diff --git a/ArmPkg/Include/Library/ArmMmuLib.h b/ArmPkg/Include/Library/StandaloneMmMmuLib.h
> similarity index 55%
> copy from ArmPkg/Include/Library/ArmMmuLib.h
> copy to ArmPkg/Include/Library/StandaloneMmMmuLib.h
> index fb7fd006417c..1f7653d00430 100644
> --- a/ArmPkg/Include/Library/ArmMmuLib.h
> +++ b/ArmPkg/Include/Library/StandaloneMmMmuLib.h
> @@ -1,6 +1,6 @@
> /** @file
>
> - Copyright (c) 2015 - 2016, Linaro Ltd. All rights reserved.<BR>
> + Copyright (c) 2018, ARM Ltd. All rights reserved.
>
> This program and the accompanying materials
> are licensed and made available under the terms and conditions of the BSD License
> @@ -12,61 +12,31 @@
>
> **/
>
> -#ifndef __ARM_MMU_LIB__
> -#define __ARM_MMU_LIB__
> -
> -#include <Uefi/UefiBaseType.h>
> -
> -#include <Library/ArmLib.h>
> -
> -EFI_STATUS
> -EFIAPI
> -ArmConfigureMmu (
> - IN ARM_MEMORY_REGION_DESCRIPTOR *MemoryTable,
> - OUT VOID **TranslationTableBase OPTIONAL,
> - OUT UINTN *TranslationTableSize OPTIONAL
> - );
> +#ifndef __STANDALONEMM_MMU_LIB__
> +#define __STANDALONEMM_MMU_LIB__
>
> EFI_STATUS
> -EFIAPI
> ArmSetMemoryRegionNoExec (
> IN EFI_PHYSICAL_ADDRESS BaseAddress,
> IN UINT64 Length
> );
>
> EFI_STATUS
> -EFIAPI
> ArmClearMemoryRegionNoExec (
> IN EFI_PHYSICAL_ADDRESS BaseAddress,
> IN UINT64 Length
> );
>
> EFI_STATUS
> -EFIAPI
> ArmSetMemoryRegionReadOnly (
> IN EFI_PHYSICAL_ADDRESS BaseAddress,
> IN UINT64 Length
> );
>
> EFI_STATUS
> -EFIAPI
> ArmClearMemoryRegionReadOnly (
> IN EFI_PHYSICAL_ADDRESS BaseAddress,
> IN UINT64 Length
> );
>
> -VOID
> -EFIAPI
> -ArmReplaceLiveTranslationEntry (
> - IN UINT64 *Entry,
> - IN UINT64 Value
> - );
> -
> -EFI_STATUS
> -ArmSetMemoryAttributes (
> - IN EFI_PHYSICAL_ADDRESS BaseAddress,
> - IN UINT64 Length,
> - IN UINT64 Attributes
> - );
> -
> -#endif
> +#endif /* __STANDALONEMM_MMU_LIB__ */
> diff --git a/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c b/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c
> new file mode 100644
> index 000000000000..d7d87b7d5d69
> --- /dev/null
> +++ b/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c
> @@ -0,0 +1,185 @@
> +/** @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
> +* 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>
Why do you need this include? If you can drop it, can you also make
the library generic (i.e., supporting ARM as well as AArch64) ?
(apologies for not spotting this before)
> +#include <IndustryStandard/ArmMmSvc.h>
> +
> +#include <Library/ArmLib.h>
> +#include <Library/ArmMmuLib.h>
> +#include <Library/ArmSvcLib.h>
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +
> +STATIC
> +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;
> +}
> +
> +STATIC
> +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 = EFI_SIZE_TO_PAGES(Length);
> + 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;
> + UINT32 CodePermission;
> +
> + Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);
> + if (Status != EFI_INVALID_PARAMETER) {
> + CodePermission = SET_MEM_ATTR_CODE_PERM_XN << SET_MEM_ATTR_CODE_PERM_SHIFT;
> + return RequestMemoryPermissionChange (
> + BaseAddress,
> + Length,
> + MemoryAttributes | CodePermission
> + );
> + }
> + return EFI_INVALID_PARAMETER;
> +}
> +
> +EFI_STATUS
> +ArmClearMemoryRegionNoExec (
> + IN EFI_PHYSICAL_ADDRESS BaseAddress,
> + IN UINT64 Length
> + )
> +{
> + EFI_STATUS Status;
> + UINT32 MemoryAttributes;
> + UINT32 CodePermission;
> +
> + Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);
> + if (Status != EFI_INVALID_PARAMETER) {
> + CodePermission = SET_MEM_ATTR_CODE_PERM_XN << SET_MEM_ATTR_CODE_PERM_SHIFT;
> + return RequestMemoryPermissionChange (
> + BaseAddress,
> + Length,
> + MemoryAttributes & ~CodePermission
> + );
> + }
> + return EFI_INVALID_PARAMETER;
> +}
> +
> +EFI_STATUS
> +ArmSetMemoryRegionReadOnly (
> + IN EFI_PHYSICAL_ADDRESS BaseAddress,
> + IN UINT64 Length
> + )
> +{
> + EFI_STATUS Status;
> + UINT32 MemoryAttributes;
> + UINT32 DataPermission;
> +
> + Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);
> + if (Status != EFI_INVALID_PARAMETER) {
> + DataPermission = SET_MEM_ATTR_DATA_PERM_RO << SET_MEM_ATTR_DATA_PERM_SHIFT;
> + return RequestMemoryPermissionChange (
> + BaseAddress,
> + Length,
> + MemoryAttributes | DataPermission
> + );
> + }
> + return EFI_INVALID_PARAMETER;
> +}
> +
> +EFI_STATUS
> +ArmClearMemoryRegionReadOnly (
> + IN EFI_PHYSICAL_ADDRESS BaseAddress,
> + IN UINT64 Length
> + )
> +{
> + EFI_STATUS Status;
> + UINT32 MemoryAttributes;
> + UINT32 PermissionRequest;
> +
> + Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);
> + if (Status != EFI_INVALID_PARAMETER) {
> + PermissionRequest = SET_MEM_ATTR_MAKE_PERM_REQUEST (SET_MEM_ATTR_DATA_PERM_RW, MemoryAttributes);
> + return RequestMemoryPermissionChange (
> + BaseAddress,
> + Length,
> + PermissionRequest
> + );
> + }
> + return EFI_INVALID_PARAMETER;
> +}
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v4 5/5] ArmPkg/ArmMmuLib: Add MMU Library suitable for use in S-EL0.
2018-11-27 8:14 ` Ard Biesheuvel
@ 2018-11-27 8:36 ` Sughosh Ganu
2018-11-27 8:38 ` Ard Biesheuvel
0 siblings, 1 reply; 17+ messages in thread
From: Sughosh Ganu @ 2018-11-27 8:36 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel@lists.01.org, Leif Lindholm, Achin Gupta
hi Ard,
On Tue Nov 27, 2018 at 09:14:52AM +0100, Ard Biesheuvel wrote:
> On Tue, 27 Nov 2018 at 07:20, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
> >
> > From: Achin Gupta <achin.gupta@arm.com>
> >
> > 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: Sughosh Ganu <sughosh.ganu@arm.com>
> > ---
> > ArmPkg/ArmPkg.dec | 1 +
> > ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf => ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf | 23 +--
> > ArmPkg/Include/Library/{ArmMmuLib.h => StandaloneMmMmuLib.h} | 38 +---
> > ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c | 185 ++++++++++++++++++++
> > 4 files changed, 203 insertions(+), 44 deletions(-)
<snip>
> > +#endif /* __STANDALONEMM_MMU_LIB__ */
> > diff --git a/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c b/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c
> > new file mode 100644
> > index 000000000000..d7d87b7d5d69
> > --- /dev/null
> > +++ b/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c
> > @@ -0,0 +1,185 @@
> > +/** @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
> > +* 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>
>
> Why do you need this include? If you can drop it, can you also make
> the library generic (i.e., supporting ARM as well as AArch64) ?
>
> (apologies for not spotting this before)
I can remove the header file if it is superfluous. But regarding your
comment on making this code generic for Arm as well, i guess we
can work on refactoring the code when/if we actually require to
support this on Arm. I am not sure if we are going to have a use-case
for StandaloneMM on Arm. Currently, we are only supporting it on
Aarch64 based platforms. Is that fine. Please let me know. Thanks.
-sughosh
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v4 5/5] ArmPkg/ArmMmuLib: Add MMU Library suitable for use in S-EL0.
2018-11-27 8:36 ` Sughosh Ganu
@ 2018-11-27 8:38 ` Ard Biesheuvel
2018-11-27 8:50 ` Sughosh Ganu
0 siblings, 1 reply; 17+ messages in thread
From: Ard Biesheuvel @ 2018-11-27 8:38 UTC (permalink / raw)
To: Sughosh Ganu; +Cc: edk2-devel@lists.01.org, Leif Lindholm, Achin Gupta
On Tue, 27 Nov 2018 at 09:36, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
>
> hi Ard,
>
> On Tue Nov 27, 2018 at 09:14:52AM +0100, Ard Biesheuvel wrote:
> > On Tue, 27 Nov 2018 at 07:20, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
> > >
> > > From: Achin Gupta <achin.gupta@arm.com>
> > >
> > > 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: Sughosh Ganu <sughosh.ganu@arm.com>
> > > ---
> > > ArmPkg/ArmPkg.dec | 1 +
> > > ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf => ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf | 23 +--
> > > ArmPkg/Include/Library/{ArmMmuLib.h => StandaloneMmMmuLib.h} | 38 +---
> > > ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c | 185 ++++++++++++++++++++
> > > 4 files changed, 203 insertions(+), 44 deletions(-)
>
>
> <snip>
>
> > > +#endif /* __STANDALONEMM_MMU_LIB__ */
> > > diff --git a/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c b/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c
> > > new file mode 100644
> > > index 000000000000..d7d87b7d5d69
> > > --- /dev/null
> > > +++ b/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c
> > > @@ -0,0 +1,185 @@
> > > +/** @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
> > > +* 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>
> >
> > Why do you need this include? If you can drop it, can you also make
> > the library generic (i.e., supporting ARM as well as AArch64) ?
> >
> > (apologies for not spotting this before)
>
> I can remove the header file if it is superfluous. But regarding your
> comment on making this code generic for Arm as well, i guess we
> can work on refactoring the code when/if we actually require to
> support this on Arm. I am not sure if we are going to have a use-case
> for StandaloneMM on Arm. Currently, we are only supporting it on
> Aarch64 based platforms. Is that fine. Please let me know. Thanks.
>
I'd strongly prefer this code to be generic if you are not using any
AArch64 specific facilities.
AFAICT, we'd simply need to move the file out of the AArch64 directory
and rename [Sources.AARCH64] to [Sources] in the .inf file if the
header dependency is indeed superfluous.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v4 5/5] ArmPkg/ArmMmuLib: Add MMU Library suitable for use in S-EL0.
2018-11-27 8:38 ` Ard Biesheuvel
@ 2018-11-27 8:50 ` Sughosh Ganu
2018-11-27 9:28 ` Ard Biesheuvel
0 siblings, 1 reply; 17+ messages in thread
From: Sughosh Ganu @ 2018-11-27 8:50 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel@lists.01.org, Leif Lindholm, Achin Gupta
On Tue Nov 27, 2018 at 09:38:24AM +0100, Ard Biesheuvel wrote:
> On Tue, 27 Nov 2018 at 09:36, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
> >
> > hi Ard,
> >
> > On Tue Nov 27, 2018 at 09:14:52AM +0100, Ard Biesheuvel wrote:
> > > On Tue, 27 Nov 2018 at 07:20, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
> > > >
> > > > From: Achin Gupta <achin.gupta@arm.com>
> > > >
> > > > 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: Sughosh Ganu <sughosh.ganu@arm.com>
> > > > ---
> > > > ArmPkg/ArmPkg.dec | 1 +
> > > > ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf => ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf | 23 +--
> > > > ArmPkg/Include/Library/{ArmMmuLib.h => StandaloneMmMmuLib.h} | 38 +---
> > > > ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c | 185 ++++++++++++++++++++
> > > > 4 files changed, 203 insertions(+), 44 deletions(-)
> >
> >
> > <snip>
> >
> > > > +#endif /* __STANDALONEMM_MMU_LIB__ */
> > > > diff --git a/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c b/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c
> > > > new file mode 100644
> > > > index 000000000000..d7d87b7d5d69
> > > > --- /dev/null
> > > > +++ b/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c
> > > > @@ -0,0 +1,185 @@
> > > > +/** @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
> > > > +* 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>
> > >
> > > Why do you need this include? If you can drop it, can you also make
> > > the library generic (i.e., supporting ARM as well as AArch64) ?
> > >
> > > (apologies for not spotting this before)
> >
> > I can remove the header file if it is superfluous. But regarding your
> > comment on making this code generic for Arm as well, i guess we
> > can work on refactoring the code when/if we actually require to
> > support this on Arm. I am not sure if we are going to have a use-case
> > for StandaloneMM on Arm. Currently, we are only supporting it on
> > Aarch64 based platforms. Is that fine. Please let me know. Thanks.
> >
>
> I'd strongly prefer this code to be generic if you are not using any
> AArch64 specific facilities.
>
> AFAICT, we'd simply need to move the file out of the AArch64 directory
> and rename [Sources.AARCH64] to [Sources] in the .inf file if the
> header dependency is indeed superfluous.
There are a couple of places where we use something like
ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64. Apart from this, it is
generic.
-sughosh
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v4 5/5] ArmPkg/ArmMmuLib: Add MMU Library suitable for use in S-EL0.
2018-11-27 8:50 ` Sughosh Ganu
@ 2018-11-27 9:28 ` Ard Biesheuvel
2018-11-27 9:58 ` Sughosh Ganu
0 siblings, 1 reply; 17+ messages in thread
From: Ard Biesheuvel @ 2018-11-27 9:28 UTC (permalink / raw)
To: Sughosh Ganu; +Cc: edk2-devel@lists.01.org, Leif Lindholm, Achin Gupta
On Tue, 27 Nov 2018 at 09:50, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
>
> On Tue Nov 27, 2018 at 09:38:24AM +0100, Ard Biesheuvel wrote:
> > On Tue, 27 Nov 2018 at 09:36, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
> > >
> > > hi Ard,
> > >
> > > On Tue Nov 27, 2018 at 09:14:52AM +0100, Ard Biesheuvel wrote:
> > > > On Tue, 27 Nov 2018 at 07:20, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
> > > > >
> > > > > From: Achin Gupta <achin.gupta@arm.com>
> > > > >
> > > > > 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: Sughosh Ganu <sughosh.ganu@arm.com>
> > > > > ---
> > > > > ArmPkg/ArmPkg.dec | 1 +
> > > > > ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf => ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf | 23 +--
> > > > > ArmPkg/Include/Library/{ArmMmuLib.h => StandaloneMmMmuLib.h} | 38 +---
> > > > > ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c | 185 ++++++++++++++++++++
> > > > > 4 files changed, 203 insertions(+), 44 deletions(-)
> > >
> > >
> > > <snip>
> > >
> > > > > +#endif /* __STANDALONEMM_MMU_LIB__ */
> > > > > diff --git a/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c b/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c
> > > > > new file mode 100644
> > > > > index 000000000000..d7d87b7d5d69
> > > > > --- /dev/null
> > > > > +++ b/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c
> > > > > @@ -0,0 +1,185 @@
> > > > > +/** @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
> > > > > +* 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>
> > > >
> > > > Why do you need this include? If you can drop it, can you also make
> > > > the library generic (i.e., supporting ARM as well as AArch64) ?
> > > >
> > > > (apologies for not spotting this before)
> > >
> > > I can remove the header file if it is superfluous. But regarding your
> > > comment on making this code generic for Arm as well, i guess we
> > > can work on refactoring the code when/if we actually require to
> > > support this on Arm. I am not sure if we are going to have a use-case
> > > for StandaloneMM on Arm. Currently, we are only supporting it on
> > > Aarch64 based platforms. Is that fine. Please let me know. Thanks.
> > >
> >
> > I'd strongly prefer this code to be generic if you are not using any
> > AArch64 specific facilities.
> >
> > AFAICT, we'd simply need to move the file out of the AArch64 directory
> > and rename [Sources.AARCH64] to [Sources] in the .inf file if the
> > header dependency is indeed superfluous.
>
> There are a couple of places where we use something like
> ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64. Apart from this, it is
> generic.
>
And how do these deviate from their AArch32 counterparts?
I agree that ARM support is not essential right now, or perhaps never.
But I would like to understand the differences before I sign off on
this.
Thanks,
Ard.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v4 5/5] ArmPkg/ArmMmuLib: Add MMU Library suitable for use in S-EL0.
2018-11-27 9:28 ` Ard Biesheuvel
@ 2018-11-27 9:58 ` Sughosh Ganu
2018-11-27 10:00 ` Ard Biesheuvel
0 siblings, 1 reply; 17+ messages in thread
From: Sughosh Ganu @ 2018-11-27 9:58 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel@lists.01.org, Leif Lindholm, Achin Gupta
On Tue Nov 27, 2018 at 10:28:50AM +0100, Ard Biesheuvel wrote:
> On Tue, 27 Nov 2018 at 09:50, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
> >
> > On Tue Nov 27, 2018 at 09:38:24AM +0100, Ard Biesheuvel wrote:
> > > On Tue, 27 Nov 2018 at 09:36, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
> > > >
> > > > hi Ard,
> > > >
> > > > On Tue Nov 27, 2018 at 09:14:52AM +0100, Ard Biesheuvel wrote:
> > > > > On Tue, 27 Nov 2018 at 07:20, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
> > > > > >
> > > > > > From: Achin Gupta <achin.gupta@arm.com>
> > > > > >
> > > > > > 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: Sughosh Ganu <sughosh.ganu@arm.com>
> > > > > > ---
> > > > > > ArmPkg/ArmPkg.dec | 1 +
> > > > > > ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf => ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf | 23 +--
> > > > > > ArmPkg/Include/Library/{ArmMmuLib.h => StandaloneMmMmuLib.h} | 38 +---
> > > > > > ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c | 185 ++++++++++++++++++++
> > > > > > 4 files changed, 203 insertions(+), 44 deletions(-)
> > > >
> > > >
> > > > <snip>
> > > >
> > > > > > +#endif /* __STANDALONEMM_MMU_LIB__ */
> > > > > > diff --git a/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c b/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..d7d87b7d5d69
> > > > > > --- /dev/null
> > > > > > +++ b/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c
> > > > > > @@ -0,0 +1,185 @@
> > > > > > +/** @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
> > > > > > +* 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>
> > > > >
> > > > > Why do you need this include? If you can drop it, can you also make
> > > > > the library generic (i.e., supporting ARM as well as AArch64) ?
> > > > >
> > > > > (apologies for not spotting this before)
> > > >
> > > > I can remove the header file if it is superfluous. But regarding your
> > > > comment on making this code generic for Arm as well, i guess we
> > > > can work on refactoring the code when/if we actually require to
> > > > support this on Arm. I am not sure if we are going to have a use-case
> > > > for StandaloneMM on Arm. Currently, we are only supporting it on
> > > > Aarch64 based platforms. Is that fine. Please let me know. Thanks.
> > > >
> > >
> > > I'd strongly prefer this code to be generic if you are not using any
> > > AArch64 specific facilities.
> > >
> > > AFAICT, we'd simply need to move the file out of the AArch64 directory
> > > and rename [Sources.AARCH64] to [Sources] in the .inf file if the
> > > header dependency is indeed superfluous.
> >
> > There are a couple of places where we use something like
> > ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64. Apart from this, it is
> > generic.
> >
>
> And how do these deviate from their AArch32 counterparts?
ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64 is passing a SMC64 based smc
call as an argument to S-EL1 shim layer, which will subsequently make
an smc call. We have a couple of functions (GetMemoryPermissions and
RequestMemoryPermissionChange) which are passing these SMC64 based smc
id's. I believe we would be required to define separate Function Id's
for Arm. Hence we would still be required to have these functions
defined under an Aarch64 directory.
>
> I agree that ARM support is not essential right now, or perhaps never.
> But I would like to understand the differences before I sign off on
> this.
Ok. I understand your argument to make this generic, but given that it
mostly would never be used on 32-bit Arm platforms, i thought we can
make these changes when we have that scenario. But if you insist, i
will make the changes. Please let me know. Thanks.
-sughosh
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v4 5/5] ArmPkg/ArmMmuLib: Add MMU Library suitable for use in S-EL0.
2018-11-27 9:58 ` Sughosh Ganu
@ 2018-11-27 10:00 ` Ard Biesheuvel
2018-11-27 10:04 ` Sughosh Ganu
0 siblings, 1 reply; 17+ messages in thread
From: Ard Biesheuvel @ 2018-11-27 10:00 UTC (permalink / raw)
To: Sughosh Ganu; +Cc: edk2-devel@lists.01.org, Leif Lindholm, Achin Gupta
On Tue, 27 Nov 2018 at 10:58, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
>
> On Tue Nov 27, 2018 at 10:28:50AM +0100, Ard Biesheuvel wrote:
> > On Tue, 27 Nov 2018 at 09:50, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
> > >
> > > On Tue Nov 27, 2018 at 09:38:24AM +0100, Ard Biesheuvel wrote:
> > > > On Tue, 27 Nov 2018 at 09:36, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
> > > > >
> > > > > hi Ard,
> > > > >
> > > > > On Tue Nov 27, 2018 at 09:14:52AM +0100, Ard Biesheuvel wrote:
> > > > > > On Tue, 27 Nov 2018 at 07:20, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
> > > > > > >
> > > > > > > From: Achin Gupta <achin.gupta@arm.com>
> > > > > > >
> > > > > > > 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: Sughosh Ganu <sughosh.ganu@arm.com>
> > > > > > > ---
> > > > > > > ArmPkg/ArmPkg.dec | 1 +
> > > > > > > ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf => ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf | 23 +--
> > > > > > > ArmPkg/Include/Library/{ArmMmuLib.h => StandaloneMmMmuLib.h} | 38 +---
> > > > > > > ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c | 185 ++++++++++++++++++++
> > > > > > > 4 files changed, 203 insertions(+), 44 deletions(-)
> > > > >
> > > > >
> > > > > <snip>
> > > > >
> > > > > > > +#endif /* __STANDALONEMM_MMU_LIB__ */
> > > > > > > diff --git a/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c b/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..d7d87b7d5d69
> > > > > > > --- /dev/null
> > > > > > > +++ b/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c
> > > > > > > @@ -0,0 +1,185 @@
> > > > > > > +/** @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
> > > > > > > +* 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>
> > > > > >
> > > > > > Why do you need this include? If you can drop it, can you also make
> > > > > > the library generic (i.e., supporting ARM as well as AArch64) ?
> > > > > >
> > > > > > (apologies for not spotting this before)
> > > > >
> > > > > I can remove the header file if it is superfluous. But regarding your
> > > > > comment on making this code generic for Arm as well, i guess we
> > > > > can work on refactoring the code when/if we actually require to
> > > > > support this on Arm. I am not sure if we are going to have a use-case
> > > > > for StandaloneMM on Arm. Currently, we are only supporting it on
> > > > > Aarch64 based platforms. Is that fine. Please let me know. Thanks.
> > > > >
> > > >
> > > > I'd strongly prefer this code to be generic if you are not using any
> > > > AArch64 specific facilities.
> > > >
> > > > AFAICT, we'd simply need to move the file out of the AArch64 directory
> > > > and rename [Sources.AARCH64] to [Sources] in the .inf file if the
> > > > header dependency is indeed superfluous.
> > >
> > > There are a couple of places where we use something like
> > > ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64. Apart from this, it is
> > > generic.
> > >
> >
> > And how do these deviate from their AArch32 counterparts?
>
> ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64 is passing a SMC64 based smc
> call as an argument to S-EL1 shim layer, which will subsequently make
> an smc call. We have a couple of functions (GetMemoryPermissions and
> RequestMemoryPermissionChange) which are passing these SMC64 based smc
> id's. I believe we would be required to define separate Function Id's
> for Arm. Hence we would still be required to have these functions
> defined under an Aarch64 directory.
>
> >
> > I agree that ARM support is not essential right now, or perhaps never.
> > But I would like to understand the differences before I sign off on
> > this.
>
> Ok. I understand your argument to make this generic, but given that it
> mostly would never be used on 32-bit Arm platforms, i thought we can
> make these changes when we have that scenario. But if you insist, i
> will make the changes. Please let me know. Thanks.
>
No that's fine. I was just trying to assess how many changes would be required.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v4 5/5] ArmPkg/ArmMmuLib: Add MMU Library suitable for use in S-EL0.
2018-11-27 10:00 ` Ard Biesheuvel
@ 2018-11-27 10:04 ` Sughosh Ganu
2018-11-27 10:05 ` Ard Biesheuvel
0 siblings, 1 reply; 17+ messages in thread
From: Sughosh Ganu @ 2018-11-27 10:04 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel
On Tue Nov 27, 2018 at 11:00:51AM +0100, Ard Biesheuvel wrote:
> On Tue, 27 Nov 2018 at 10:58, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
> >
> > On Tue Nov 27, 2018 at 10:28:50AM +0100, Ard Biesheuvel wrote:
> > > On Tue, 27 Nov 2018 at 09:50, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
> > > >
> > > > On Tue Nov 27, 2018 at 09:38:24AM +0100, Ard Biesheuvel wrote:
> > > > > On Tue, 27 Nov 2018 at 09:36, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
> > > > > >
> > > > > > hi Ard,
> > > > > >
> > > > > > On Tue Nov 27, 2018 at 09:14:52AM +0100, Ard Biesheuvel wrote:
> > > > > > > On Tue, 27 Nov 2018 at 07:20, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
> > > > > > > >
> > > > > > > > From: Achin Gupta <achin.gupta@arm.com>
> > > > > > > >
> > > > > > > > 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: Sughosh Ganu <sughosh.ganu@arm.com>
> > > > > > > > ---
> > > > > > > > ArmPkg/ArmPkg.dec | 1 +
> > > > > > > > ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf => ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf | 23 +--
> > > > > > > > ArmPkg/Include/Library/{ArmMmuLib.h => StandaloneMmMmuLib.h} | 38 +---
> > > > > > > > ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c | 185 ++++++++++++++++++++
> > > > > > > > 4 files changed, 203 insertions(+), 44 deletions(-)
> > > > > >
> > > > > >
> > > > > > <snip>
> > > > > >
> > > > > > > > +#endif /* __STANDALONEMM_MMU_LIB__ */
> > > > > > > > diff --git a/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c b/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000000..d7d87b7d5d69
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c
> > > > > > > > @@ -0,0 +1,185 @@
> > > > > > > > +/** @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
> > > > > > > > +* 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>
> > > > > > >
> > > > > > > Why do you need this include? If you can drop it, can you also make
> > > > > > > the library generic (i.e., supporting ARM as well as AArch64) ?
> > > > > > >
> > > > > > > (apologies for not spotting this before)
> > > > > >
> > > > > > I can remove the header file if it is superfluous. But regarding your
> > > > > > comment on making this code generic for Arm as well, i guess we
> > > > > > can work on refactoring the code when/if we actually require to
> > > > > > support this on Arm. I am not sure if we are going to have a use-case
> > > > > > for StandaloneMM on Arm. Currently, we are only supporting it on
> > > > > > Aarch64 based platforms. Is that fine. Please let me know. Thanks.
> > > > > >
> > > > >
> > > > > I'd strongly prefer this code to be generic if you are not using any
> > > > > AArch64 specific facilities.
> > > > >
> > > > > AFAICT, we'd simply need to move the file out of the AArch64 directory
> > > > > and rename [Sources.AARCH64] to [Sources] in the .inf file if the
> > > > > header dependency is indeed superfluous.
> > > >
> > > > There are a couple of places where we use something like
> > > > ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64. Apart from this, it is
> > > > generic.
> > > >
> > >
> > > And how do these deviate from their AArch32 counterparts?
> >
> > ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64 is passing a SMC64 based smc
> > call as an argument to S-EL1 shim layer, which will subsequently make
> > an smc call. We have a couple of functions (GetMemoryPermissions and
> > RequestMemoryPermissionChange) which are passing these SMC64 based smc
> > id's. I believe we would be required to define separate Function Id's
> > for Arm. Hence we would still be required to have these functions
> > defined under an Aarch64 directory.
> >
> > >
> > > I agree that ARM support is not essential right now, or perhaps never.
> > > But I would like to understand the differences before I sign off on
> > > this.
> >
> > Ok. I understand your argument to make this generic, but given that it
> > mostly would never be used on 32-bit Arm platforms, i thought we can
> > make these changes when we have that scenario. But if you insist, i
> > will make the changes. Please let me know. Thanks.
> >
>
> No that's fine. I was just trying to assess how many changes would be required.
So, just for my understanding, i can post a v5 for this patch only
with removal of the header file, if superfluous. Is that right.
-sughosh
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v4 5/5] ArmPkg/ArmMmuLib: Add MMU Library suitable for use in S-EL0.
2018-11-27 10:04 ` Sughosh Ganu
@ 2018-11-27 10:05 ` Ard Biesheuvel
0 siblings, 0 replies; 17+ messages in thread
From: Ard Biesheuvel @ 2018-11-27 10:05 UTC (permalink / raw)
To: Sughosh Ganu; +Cc: edk2-devel@lists.01.org
On Tue, 27 Nov 2018 at 11:04, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
>
> On Tue Nov 27, 2018 at 11:00:51AM +0100, Ard Biesheuvel wrote:
> > On Tue, 27 Nov 2018 at 10:58, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
> > >
> > > On Tue Nov 27, 2018 at 10:28:50AM +0100, Ard Biesheuvel wrote:
> > > > On Tue, 27 Nov 2018 at 09:50, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
> > > > >
> > > > > On Tue Nov 27, 2018 at 09:38:24AM +0100, Ard Biesheuvel wrote:
> > > > > > On Tue, 27 Nov 2018 at 09:36, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
> > > > > > >
> > > > > > > hi Ard,
> > > > > > >
> > > > > > > On Tue Nov 27, 2018 at 09:14:52AM +0100, Ard Biesheuvel wrote:
> > > > > > > > On Tue, 27 Nov 2018 at 07:20, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
> > > > > > > > >
> > > > > > > > > From: Achin Gupta <achin.gupta@arm.com>
> > > > > > > > >
> > > > > > > > > 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: Sughosh Ganu <sughosh.ganu@arm.com>
> > > > > > > > > ---
> > > > > > > > > ArmPkg/ArmPkg.dec | 1 +
> > > > > > > > > ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf => ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf | 23 +--
> > > > > > > > > ArmPkg/Include/Library/{ArmMmuLib.h => StandaloneMmMmuLib.h} | 38 +---
> > > > > > > > > ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c | 185 ++++++++++++++++++++
> > > > > > > > > 4 files changed, 203 insertions(+), 44 deletions(-)
> > > > > > >
> > > > > > >
> > > > > > > <snip>
> > > > > > >
> > > > > > > > > +#endif /* __STANDALONEMM_MMU_LIB__ */
> > > > > > > > > diff --git a/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c b/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c
> > > > > > > > > new file mode 100644
> > > > > > > > > index 000000000000..d7d87b7d5d69
> > > > > > > > > --- /dev/null
> > > > > > > > > +++ b/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c
> > > > > > > > > @@ -0,0 +1,185 @@
> > > > > > > > > +/** @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
> > > > > > > > > +* 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>
> > > > > > > >
> > > > > > > > Why do you need this include? If you can drop it, can you also make
> > > > > > > > the library generic (i.e., supporting ARM as well as AArch64) ?
> > > > > > > >
> > > > > > > > (apologies for not spotting this before)
> > > > > > >
> > > > > > > I can remove the header file if it is superfluous. But regarding your
> > > > > > > comment on making this code generic for Arm as well, i guess we
> > > > > > > can work on refactoring the code when/if we actually require to
> > > > > > > support this on Arm. I am not sure if we are going to have a use-case
> > > > > > > for StandaloneMM on Arm. Currently, we are only supporting it on
> > > > > > > Aarch64 based platforms. Is that fine. Please let me know. Thanks.
> > > > > > >
> > > > > >
> > > > > > I'd strongly prefer this code to be generic if you are not using any
> > > > > > AArch64 specific facilities.
> > > > > >
> > > > > > AFAICT, we'd simply need to move the file out of the AArch64 directory
> > > > > > and rename [Sources.AARCH64] to [Sources] in the .inf file if the
> > > > > > header dependency is indeed superfluous.
> > > > >
> > > > > There are a couple of places where we use something like
> > > > > ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64. Apart from this, it is
> > > > > generic.
> > > > >
> > > >
> > > > And how do these deviate from their AArch32 counterparts?
> > >
> > > ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64 is passing a SMC64 based smc
> > > call as an argument to S-EL1 shim layer, which will subsequently make
> > > an smc call. We have a couple of functions (GetMemoryPermissions and
> > > RequestMemoryPermissionChange) which are passing these SMC64 based smc
> > > id's. I believe we would be required to define separate Function Id's
> > > for Arm. Hence we would still be required to have these functions
> > > defined under an Aarch64 directory.
> > >
> > > >
> > > > I agree that ARM support is not essential right now, or perhaps never.
> > > > But I would like to understand the differences before I sign off on
> > > > this.
> > >
> > > Ok. I understand your argument to make this generic, but given that it
> > > mostly would never be used on 32-bit Arm platforms, i thought we can
> > > make these changes when we have that scenario. But if you insist, i
> > > will make the changes. Please let me know. Thanks.
> > >
> >
> > No that's fine. I was just trying to assess how many changes would be required.
>
> So, just for my understanding, i can post a v5 for this patch only
> with removal of the header file, if superfluous. Is that right.
>
Yes
^ permalink raw reply [flat|nested] 17+ messages in thread