public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v1 00/12] Add support for using FF-A calls
@ 2020-10-21 11:32 sughosh.ganu
  2020-10-21 11:32 ` [PATCH v1 01/12] ArmPkg/IndustryStandard: Add barebones FF-A header Sughosh Ganu
                   ` (13 more replies)
  0 siblings, 14 replies; 28+ messages in thread
From: sughosh.ganu @ 2020-10-21 11:32 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Sami Mujawar, Jiewen Yao

Achin Gupta (8):
  ArmPkg/IndustryStandard: Add barebones FF-A header
  ArmPkg/ArmSvcLib: Return x4-x7 in output parameters
  StandaloneMmPkg: Use FF-A header file in Standalone MM Core entry
    point
  StandaloneMmPkg: Add option to use FF-A calls for getting SPM version
  StandaloneMmPkg: Add option to use FF-A calls for communication with
    SPM
  StandaloneMmPkg: Use FF-A header file in Standalone MM Arm MMU library
  ArmPkg/StandaloneMmMmuLib: Add option to use FF-A calls to get memory
    region's permissions
  ArmPkg/StandaloneMmMmuLib: Add option to use FF-A calls to set memory
    region's permissions

Ilias Apalodimas (2):
  MdeModulePkg/VariableStandaloneMm: Set PcdFlashNvStorageVariableBase
    to Pcd
  StandaloneMmPkg: Allow sending FFA Direct Request message to
    StandaloneMm

Sughosh Ganu (1):
  ArmPkg: Introduce support for PcdFfaEnable

Sughossh Ganu (1):
  StandaloneMmPkg: Add the SPM version for FF-A

 ArmPkg/ArmPkg.dec                             |   3 +
 .../ArmMmuStandaloneMmLib.inf                 |   3 +
 .../RuntimeDxe/VariableStandaloneMm.inf       |   6 +-
 .../StandaloneMmCoreEntryPoint.inf            |   3 +
 ArmPkg/Include/IndustryStandard/ArmFfaSvc.h   |  16 +++
 .../AArch64/ArmMmuStandaloneMmLib.c           |  53 +++++++--
 .../StandaloneMmCpu/AArch64/EventHandle.c     |   4 +-
 .../AArch64/StandaloneMmCoreEntryPoint.c      | 103 ++++++++++++++----
 ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S     |   2 +
 9 files changed, 155 insertions(+), 38 deletions(-)
 create mode 100644 ArmPkg/Include/IndustryStandard/ArmFfaSvc.h

-- 
2.17.1



^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v1 01/12] ArmPkg/IndustryStandard: Add barebones FF-A header
  2020-10-21 11:32 [PATCH v1 00/12] Add support for using FF-A calls sughosh.ganu
@ 2020-10-21 11:32 ` Sughosh Ganu
  2020-11-05 10:33   ` Sami Mujawar
  2020-10-21 11:32 ` [PATCH v1 02/12] ArmPkg/ArmSvcLib: Return x4-x7 in output parameters Sughosh Ganu
                   ` (12 subsequent siblings)
  13 siblings, 1 reply; 28+ messages in thread
From: Sughosh Ganu @ 2020-10-21 11:32 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Sami Mujawar, Jiewen Yao, Achin Gupta

From: Achin Gupta <achin.gupta@arm.com>

This patch adds a rudimentary header file with defines for FF-A ABIs
that will be used as the transport between S-EL0 and the SPM

Signed-off-by: Achin Gupta <achin.gupta@arm.com>
---
 ArmPkg/Include/IndustryStandard/ArmFfaSvc.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h b/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
new file mode 100644
index 0000000000..a8914085de
--- /dev/null
+++ b/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
@@ -0,0 +1,16 @@
+/** @file
+*
+*  Copyright (c) 2020, ARM Limited. All rights reserved.
+*
+*  SPDX-License-Identifier: BSD-2-Clause-Patent
+*
+**/
+
+#ifndef __ARM_FFA_SVC_H__
+#define __ARM_FFA_SVC_H__
+
+#define ARM_SVC_ID_FFA_VERSION_AARCH32                  0x84000063
+#define ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64      0xC400006F
+#define ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64     0xC4000070
+
+#endif
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v1 02/12] ArmPkg/ArmSvcLib: Return x4-x7 in output parameters
  2020-10-21 11:32 [PATCH v1 00/12] Add support for using FF-A calls sughosh.ganu
  2020-10-21 11:32 ` [PATCH v1 01/12] ArmPkg/IndustryStandard: Add barebones FF-A header Sughosh Ganu
@ 2020-10-21 11:32 ` Sughosh Ganu
  2020-11-10 12:02   ` Sami Mujawar
  2020-10-21 11:32 ` [PATCH v1 03/12] StandaloneMmPkg: Use FF-A header file in Standalone MM Core entry point Sughosh Ganu
                   ` (11 subsequent siblings)
  13 siblings, 1 reply; 28+ messages in thread
From: Sughosh Ganu @ 2020-10-21 11:32 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Sami Mujawar, Jiewen Yao, Achin Gupta

From: Achin Gupta <achin.gupta@arm.com>

The Arm SMC calling convention standard v1.2 allows 8 input and output
parameter registers. The FF-A specification relies on this
communication. This patch extends the number of output registers
returned by ArmCallSvc() to match this convention.

Signed-off-by: Achin Gupta <achin.gupta@arm.com>
---
 ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S b/ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S
index ee265f94b9..8cb5c45582 100644
--- a/ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S
+++ b/ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S
@@ -36,6 +36,8 @@ ASM_PFX(ArmCallSvc):
   // A SVC call can return up to 4 values - we do not need to store back x4-x7.
   stp   x0, x1, [x9, #0]
   stp   x2, x3, [x9, #16]
+  stp   x4, x5, [x9, #32]
+  stp   x6, x7, [x9, #48]
 
   mov   x0, x9
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v1 03/12] StandaloneMmPkg: Use FF-A header file in Standalone MM Core entry point
  2020-10-21 11:32 [PATCH v1 00/12] Add support for using FF-A calls sughosh.ganu
  2020-10-21 11:32 ` [PATCH v1 01/12] ArmPkg/IndustryStandard: Add barebones FF-A header Sughosh Ganu
  2020-10-21 11:32 ` [PATCH v1 02/12] ArmPkg/ArmSvcLib: Return x4-x7 in output parameters Sughosh Ganu
@ 2020-10-21 11:32 ` Sughosh Ganu
  2020-10-21 11:32 ` [PATCH v1 04/12] ArmPkg: Introduce support for PcdFfaEnable Sughosh Ganu
                   ` (10 subsequent siblings)
  13 siblings, 0 replies; 28+ messages in thread
From: Sughosh Ganu @ 2020-10-21 11:32 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Sami Mujawar, Jiewen Yao, Achin Gupta

From: Achin Gupta <achin.gupta@arm.com>

Signed-off-by: Achin Gupta <achin.gupta@arm.com>
---
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
index 9cecfa667b..c4132b0d78 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
@@ -26,6 +26,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 #include <IndustryStandard/ArmStdSmc.h>
 #include <IndustryStandard/ArmMmSvc.h>
+#include <IndustryStandard/ArmFfaSvc.h>
 
 #define SPM_MAJOR_VER_MASK        0xFFFF0000
 #define SPM_MINOR_VER_MASK        0x0000FFFF
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v1 04/12] ArmPkg: Introduce support for PcdFfaEnable
  2020-10-21 11:32 [PATCH v1 00/12] Add support for using FF-A calls sughosh.ganu
                   ` (2 preceding siblings ...)
  2020-10-21 11:32 ` [PATCH v1 03/12] StandaloneMmPkg: Use FF-A header file in Standalone MM Core entry point Sughosh Ganu
@ 2020-10-21 11:32 ` Sughosh Ganu
  2020-11-19  9:56   ` Sami Mujawar
  2020-10-21 11:32 ` [PATCH v1 05/12] StandaloneMmPkg: Add the SPM version for FF-A Sughosh Ganu
                   ` (9 subsequent siblings)
  13 siblings, 1 reply; 28+ messages in thread
From: Sughosh Ganu @ 2020-10-21 11:32 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Sami Mujawar, Jiewen Yao, Sughosh Ganu

The Secure Partition(SP) can request services from the Secure
Partition Manager Core(SPMC) either through FF-A calls or through the
existing SVC calls. Add a feature flag Pcd for enabling the FF-A
method -- when this is set to FALSE, the SP uses the existing SVC
calls for making the requests.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 ArmPkg/ArmPkg.dec | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
index eaf1072d9e..979f3e2699 100644
--- a/ArmPkg/ArmPkg.dec
+++ b/ArmPkg/ArmPkg.dec
@@ -78,6 +78,9 @@
   # Define if the GICv3 controller should use the GICv2 legacy
   gArmTokenSpaceGuid.PcdArmGicV3WithV2Legacy|FALSE|BOOLEAN|0x00000042
 
+  # Used to select method for requesting services from S-EL1
+  gArmTokenSpaceGuid.PcdFfaEnable|FALSE|BOOLEAN|0x0000005B
+
 [PcdsFeatureFlag.ARM]
   # Whether to map normal memory as non-shareable. FALSE is the safe choice, but
   # TRUE may be appropriate to fix performance problems if you don't care about
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v1 05/12] StandaloneMmPkg: Add the SPM version for FF-A
  2020-10-21 11:32 [PATCH v1 00/12] Add support for using FF-A calls sughosh.ganu
                   ` (3 preceding siblings ...)
  2020-10-21 11:32 ` [PATCH v1 04/12] ArmPkg: Introduce support for PcdFfaEnable Sughosh Ganu
@ 2020-10-21 11:32 ` Sughosh Ganu
  2020-11-19  9:54   ` Sami Mujawar
  2020-10-21 11:32 ` [PATCH v1 06/12] StandaloneMmPkg: Add option to use FF-A calls for getting SPM version Sughosh Ganu
                   ` (8 subsequent siblings)
  13 siblings, 1 reply; 28+ messages in thread
From: Sughosh Ganu @ 2020-10-21 11:32 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Sami Mujawar, Jiewen Yao, Sughossh Ganu

From: Sughossh Ganu <sughosh.ganu@linaro.org>

The Firmware Framework(FF-A) requires implementation of SPM version
v1.0. Add new macros for the version that will be used for FF-A.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
index c4132b0d78..33f0db654f 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
@@ -32,6 +32,9 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #define SPM_MINOR_VER_MASK        0x0000FFFF
 #define SPM_MAJOR_VER_SHIFT       16
 
+CONST UINT32 SPM_MAJOR_VER_FFA = 1;
+CONST UINT32 SPM_MINOR_VER_FFA = 0;
+
 CONST UINT32 SPM_MAJOR_VER = 0;
 CONST UINT32 SPM_MINOR_VER = 1;
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v1 06/12] StandaloneMmPkg: Add option to use FF-A calls for getting SPM version
  2020-10-21 11:32 [PATCH v1 00/12] Add support for using FF-A calls sughosh.ganu
                   ` (4 preceding siblings ...)
  2020-10-21 11:32 ` [PATCH v1 05/12] StandaloneMmPkg: Add the SPM version for FF-A Sughosh Ganu
@ 2020-10-21 11:32 ` Sughosh Ganu
  2020-11-23 10:10   ` Sami Mujawar
  2020-10-21 11:32 ` [PATCH v1 07/12] StandaloneMmPkg: Add option to use FF-A calls for communication with SPM Sughosh Ganu
                   ` (7 subsequent siblings)
  13 siblings, 1 reply; 28+ messages in thread
From: Sughosh Ganu @ 2020-10-21 11:32 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Sami Mujawar, Jiewen Yao, Achin Gupta

From: Achin Gupta <achin.gupta@arm.com>

With the introduction of Firmware Framework(FF-A), a Secure Partition
can get the SPM version either using FF-A calls or through the
existing svc calls. Use a runtime check to use either of the two
methods based on the Pcd feature flag value.

Signed-off-by: Achin Gupta <achin.gupta@arm.com>
Co-developed-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf       |  3 ++
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c | 31 ++++++++++++++------
 2 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
index 7d6ee4e08e..f8184f501b 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
@@ -49,5 +49,8 @@
   gEfiStandaloneMmNonSecureBufferGuid
   gEfiArmTfCpuDriverEpDescriptorGuid
 
+[Pcd]
+  gArmTokenSpaceGuid.PcdFfaEnable
+
 [BuildOptions]
   GCC:*_*_*_CC_FLAGS = -fpie
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
index 33f0db654f..7d1e3c4d7c 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
@@ -23,6 +23,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Library/BaseLib.h>
 #include <Library/BaseMemoryLib.h>
 #include <Library/SerialPortLib.h>
+#include <Library/PcdLib.h>
 
 #include <IndustryStandard/ArmStdSmc.h>
 #include <IndustryStandard/ArmMmSvc.h>
@@ -165,19 +166,31 @@ EFI_STATUS
 GetSpmVersion (VOID)
 {
   EFI_STATUS   Status;
-  UINT16       SpmMajorVersion;
-  UINT16       SpmMinorVersion;
+  UINT16       CurSpmMajorVer;
+  UINT16       SpmMajorVer;
+  UINT16       CurSpmMinorVer;
+  UINT16       SpmMinorVer;
   UINT32       SpmVersion;
   ARM_SVC_ARGS SpmVersionArgs;
 
-  SpmVersionArgs.Arg0 = ARM_SVC_ID_SPM_VERSION_AARCH32;
+  if (FeaturePcdGet (PcdFfaEnable)) {
+    SpmVersionArgs.Arg0 = ARM_SVC_ID_FFA_VERSION_AARCH32;
+    SpmVersionArgs.Arg1 = SPM_MAJOR_VER << SPM_MAJOR_VER_SHIFT;
+    SpmVersionArgs.Arg1 |= SPM_MINOR_VER;
+    SpmMajorVer = SPM_MAJOR_VER_FFA;
+    SpmMinorVer = SPM_MINOR_VER_FFA;
+  } else {
+    SpmVersionArgs.Arg0 = ARM_SVC_ID_SPM_VERSION_AARCH32;
+    SpmMajorVer = SPM_MAJOR_VER;
+    SpmMinorVer = SPM_MINOR_VER;
+  }
 
   ArmCallSvc (&SpmVersionArgs);
 
   SpmVersion = SpmVersionArgs.Arg0;
 
-  SpmMajorVersion = ((SpmVersion & SPM_MAJOR_VER_MASK) >> SPM_MAJOR_VER_SHIFT);
-  SpmMinorVersion = ((SpmVersion & SPM_MINOR_VER_MASK) >> 0);
+  CurSpmMajorVer = ((SpmVersion & SPM_MAJOR_VER_MASK) >> SPM_MAJOR_VER_SHIFT);
+  CurSpmMinorVer = ((SpmVersion & SPM_MINOR_VER_MASK) >> 0);
 
   // Different major revision values indicate possibly incompatible functions.
   // For two revisions, A and B, for which the major revision values are
@@ -186,17 +199,17 @@ GetSpmVersion (VOID)
   // revision A must work in a compatible way with revision B.
   // However, it is possible for revision B to have a higher
   // function count than revision A.
-  if ((SpmMajorVersion == SPM_MAJOR_VER) &&
-      (SpmMinorVersion >= SPM_MINOR_VER))
+  if ((CurSpmMajorVer == SpmMajorVer) &&
+      (CurSpmMinorVer >= SpmMinorVer))
   {
     DEBUG ((DEBUG_INFO, "SPM Version: Major=0x%x, Minor=0x%x\n",
-           SpmMajorVersion, SpmMinorVersion));
+           CurSpmMajorVer, CurSpmMinorVer));
     Status = EFI_SUCCESS;
   }
   else
   {
     DEBUG ((DEBUG_INFO, "Incompatible SPM Versions.\n Current Version: Major=0x%x, Minor=0x%x.\n Expected: Major=0x%x, Minor>=0x%x.\n",
-            SpmMajorVersion, SpmMinorVersion, SPM_MAJOR_VER, SPM_MINOR_VER));
+            CurSpmMajorVer, CurSpmMinorVer, SpmMajorVer, SpmMinorVer));
     Status = EFI_UNSUPPORTED;
   }
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v1 07/12] StandaloneMmPkg: Add option to use FF-A calls for communication with SPM
  2020-10-21 11:32 [PATCH v1 00/12] Add support for using FF-A calls sughosh.ganu
                   ` (5 preceding siblings ...)
  2020-10-21 11:32 ` [PATCH v1 06/12] StandaloneMmPkg: Add option to use FF-A calls for getting SPM version Sughosh Ganu
@ 2020-10-21 11:32 ` Sughosh Ganu
  2020-11-23 12:38   ` Sami Mujawar
  2020-10-21 11:32 ` [PATCH v1 08/12] StandaloneMmPkg: Use FF-A header file in Standalone MM Arm MMU library Sughosh Ganu
                   ` (6 subsequent siblings)
  13 siblings, 1 reply; 28+ messages in thread
From: Sughosh Ganu @ 2020-10-21 11:32 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Sami Mujawar, Jiewen Yao, Achin Gupta

From: Achin Gupta <achin.gupta@arm.com>

Add support for reporting completion of a MM request using either the
Firmware Framework(FF-A) ABI transport or through the earlier used SVC
calls.

Signed-off-by: Achin Gupta <achin.gupta@arm.com>
Co-developed-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c | 68 ++++++++++++++++----
 1 file changed, 54 insertions(+), 14 deletions(-)

diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
index 7d1e3c4d7c..06a8f487f9 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
@@ -112,6 +112,7 @@ DelegatedEventLoop (
   IN ARM_SVC_ARGS *EventCompleteSvcArgs
   )
 {
+  BOOLEAN FfaEnabled;
   EFI_STATUS Status;
   UINTN SvcStatus;
 
@@ -123,16 +124,32 @@ DelegatedEventLoop (
     DEBUG ((DEBUG_INFO, "X1 :  0x%x\n", (UINT32) EventCompleteSvcArgs->Arg1));
     DEBUG ((DEBUG_INFO, "X2 :  0x%x\n", (UINT32) EventCompleteSvcArgs->Arg2));
     DEBUG ((DEBUG_INFO, "X3 :  0x%x\n", (UINT32) EventCompleteSvcArgs->Arg3));
-
-    Status = CpuDriverEntryPoint (
-               EventCompleteSvcArgs->Arg0,
-               EventCompleteSvcArgs->Arg3,
-               EventCompleteSvcArgs->Arg1
-               );
-
-    if (EFI_ERROR (Status)) {
-      DEBUG ((DEBUG_ERROR, "Failed delegated event 0x%x, Status 0x%x\n",
-              EventCompleteSvcArgs->Arg0, Status));
+    DEBUG ((DEBUG_INFO, "X4 :  0x%x\n", (UINT32) EventCompleteSvcArgs->Arg4));
+    DEBUG ((DEBUG_INFO, "X5 :  0x%x\n", (UINT32) EventCompleteSvcArgs->Arg5));
+    DEBUG ((DEBUG_INFO, "X6 :  0x%x\n", (UINT32) EventCompleteSvcArgs->Arg6));
+    DEBUG ((DEBUG_INFO, "X7 :  0x%x\n", (UINT32) EventCompleteSvcArgs->Arg7));
+
+    FfaEnabled = FeaturePcdGet (PcdFfaEnable);
+    if (FfaEnabled) {
+      Status = CpuDriverEntryPoint (
+                 EventCompleteSvcArgs->Arg0,
+                 EventCompleteSvcArgs->Arg6,
+                 EventCompleteSvcArgs->Arg3
+                 );
+      if (EFI_ERROR (Status)) {
+        DEBUG ((DEBUG_ERROR, "Failed delegated event 0x%x, Status 0x%x\n",
+                EventCompleteSvcArgs->Arg3, Status));
+      }
+    } else {
+      Status = CpuDriverEntryPoint (
+                 EventCompleteSvcArgs->Arg0,
+                 EventCompleteSvcArgs->Arg3,
+                 EventCompleteSvcArgs->Arg1
+                 );
+      if (EFI_ERROR (Status)) {
+        DEBUG ((DEBUG_ERROR, "Failed delegated event 0x%x, Status 0x%x\n",
+                EventCompleteSvcArgs->Arg0, Status));
+      }
     }
 
     switch (Status) {
@@ -156,8 +173,16 @@ DelegatedEventLoop (
       break;
     }
 
-    EventCompleteSvcArgs->Arg0 = ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64;
-    EventCompleteSvcArgs->Arg1 = SvcStatus;
+    if (FfaEnabled) {
+      EventCompleteSvcArgs->Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64;
+      EventCompleteSvcArgs->Arg1 = 0;
+      EventCompleteSvcArgs->Arg2 = 0;
+      EventCompleteSvcArgs->Arg3 = ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64;
+      EventCompleteSvcArgs->Arg4 = SvcStatus;
+    } else {
+      EventCompleteSvcArgs->Arg0 = ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64;
+      EventCompleteSvcArgs->Arg1 = SvcStatus;
+    }
   }
 }
 
@@ -216,6 +241,22 @@ GetSpmVersion (VOID)
   return Status;
 }
 
+STATIC
+VOID
+InitArmSvcArgs (ARM_SVC_ARGS *InitMmFoundationSvcArgs, EFI_STATUS *Status)
+{
+  if (FeaturePcdGet (PcdFfaEnable)) {
+    InitMmFoundationSvcArgs->Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64;
+    InitMmFoundationSvcArgs->Arg1 = 0;
+    InitMmFoundationSvcArgs->Arg2 = 0;
+    InitMmFoundationSvcArgs->Arg3 = ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64;
+    InitMmFoundationSvcArgs->Arg4 = *Status;
+  } else {
+    InitMmFoundationSvcArgs->Arg0 = ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64;
+    InitMmFoundationSvcArgs->Arg1 = *Status;
+  }
+}
+
 /**
   The entry point of Standalone MM Foundation.
 
@@ -329,7 +370,6 @@ _ModuleEntryPoint (
 
 finish:
   ZeroMem (&InitMmFoundationSvcArgs, sizeof(InitMmFoundationSvcArgs));
-  InitMmFoundationSvcArgs.Arg0 = ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64;
-  InitMmFoundationSvcArgs.Arg1 = Status;
+  InitArmSvcArgs(&InitMmFoundationSvcArgs, &Status);
   DelegatedEventLoop (&InitMmFoundationSvcArgs);
 }
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v1 08/12] StandaloneMmPkg: Use FF-A header file in Standalone MM Arm MMU library
  2020-10-21 11:32 [PATCH v1 00/12] Add support for using FF-A calls sughosh.ganu
                   ` (6 preceding siblings ...)
  2020-10-21 11:32 ` [PATCH v1 07/12] StandaloneMmPkg: Add option to use FF-A calls for communication with SPM Sughosh Ganu
@ 2020-10-21 11:32 ` Sughosh Ganu
  2020-10-21 11:32 ` [PATCH v1 09/12] ArmPkg/StandaloneMmMmuLib: Add option to use FF-A calls to get memory region's permissions Sughosh Ganu
                   ` (5 subsequent siblings)
  13 siblings, 0 replies; 28+ messages in thread
From: Sughosh Ganu @ 2020-10-21 11:32 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Sami Mujawar, Jiewen Yao, Achin Gupta

From: Achin Gupta <achin.gupta@arm.com>

Signed-off-by: Achin Gupta <achin.gupta@arm.com>
---
 ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c b/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
index 3806490f70..362b1a0f8a 100644
--- a/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
+++ b/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
@@ -9,6 +9,7 @@
 
 #include <Uefi.h>
 #include <IndustryStandard/ArmMmSvc.h>
+#include <IndustryStandard/ArmFfaSvc.h>
 
 #include <Library/ArmLib.h>
 #include <Library/ArmMmuLib.h>
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v1 09/12] ArmPkg/StandaloneMmMmuLib: Add option to use FF-A calls to get memory region's permissions
  2020-10-21 11:32 [PATCH v1 00/12] Add support for using FF-A calls sughosh.ganu
                   ` (7 preceding siblings ...)
  2020-10-21 11:32 ` [PATCH v1 08/12] StandaloneMmPkg: Use FF-A header file in Standalone MM Arm MMU library Sughosh Ganu
@ 2020-10-21 11:32 ` Sughosh Ganu
  2020-11-23 13:26   ` Sami Mujawar
  2020-10-21 11:32 ` [PATCH v1 10/12] ArmPkg/StandaloneMmMmuLib: Add option to use FF-A calls to set " Sughosh Ganu
                   ` (4 subsequent siblings)
  13 siblings, 1 reply; 28+ messages in thread
From: Sughosh Ganu @ 2020-10-21 11:32 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Sami Mujawar, Jiewen Yao, Achin Gupta

From: Achin Gupta <achin.gupta@arm.com>

Allow getting memory region's permissions using either of the Firmware
Framework(FF-A) ABI transport or through the earlier used SVC calls.

Signed-off-by: Achin Gupta <achin.gupta@arm.com>
Co-developed-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf       |  3 +++
 ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c | 28 +++++++++++++++-----
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
index 85973687f5..a29dd800b5 100644
--- a/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
+++ b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
@@ -23,6 +23,9 @@
   ArmPkg/ArmPkg.dec
   MdePkg/MdePkg.dec
 
+[Pcd]
+  gArmTokenSpaceGuid.PcdFfaEnable
+
 [LibraryClasses]
   ArmLib
   CacheMaintenanceLib
diff --git a/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c b/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
index 362b1a0f8a..ab13602556 100644
--- a/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
+++ b/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
@@ -16,6 +16,7 @@
 #include <Library/ArmSvcLib.h>
 #include <Library/BaseLib.h>
 #include <Library/DebugLib.h>
+#include <Library/PcdLib.h>
 
 STATIC
 EFI_STATUS
@@ -25,19 +26,32 @@ GetMemoryPermissions (
   )
 {
   ARM_SVC_ARGS  GetMemoryPermissionsSvcArgs = {0};
-
-  GetMemoryPermissionsSvcArgs.Arg0 = ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64;
-  GetMemoryPermissionsSvcArgs.Arg1 = BaseAddress;
-  GetMemoryPermissionsSvcArgs.Arg2 = 0;
-  GetMemoryPermissionsSvcArgs.Arg3 = 0;
+  BOOLEAN FfaEnabled;
+
+  FfaEnabled = FeaturePcdGet (PcdFfaEnable);
+  if (FfaEnabled) {
+    GetMemoryPermissionsSvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64;
+    GetMemoryPermissionsSvcArgs.Arg1 = 0x3;
+    GetMemoryPermissionsSvcArgs.Arg2 = 0;
+    GetMemoryPermissionsSvcArgs.Arg3 = ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64;
+    GetMemoryPermissionsSvcArgs.Arg4 = BaseAddress;
+  } else {
+    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) {
+  if (GetMemoryPermissionsSvcArgs.Arg0 == ARM_SVC_SPM_RET_INVALID_PARAMS ||
+      GetMemoryPermissionsSvcArgs.Arg3 == ARM_SVC_SPM_RET_INVALID_PARAMS) {
     *MemoryAttributes = 0;
     return EFI_INVALID_PARAMETER;
   }
 
-  *MemoryAttributes = GetMemoryPermissionsSvcArgs.Arg0;
+  *MemoryAttributes = FfaEnabled ?
+    GetMemoryPermissionsSvcArgs.Arg3 : GetMemoryPermissionsSvcArgs.Arg0;
+
   return EFI_SUCCESS;
 }
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v1 10/12] ArmPkg/StandaloneMmMmuLib: Add option to use FF-A calls to set memory region's permissions
  2020-10-21 11:32 [PATCH v1 00/12] Add support for using FF-A calls sughosh.ganu
                   ` (8 preceding siblings ...)
  2020-10-21 11:32 ` [PATCH v1 09/12] ArmPkg/StandaloneMmMmuLib: Add option to use FF-A calls to get memory region's permissions Sughosh Ganu
@ 2020-10-21 11:32 ` Sughosh Ganu
  2020-11-23 13:42   ` Sami Mujawar
       [not found]   ` <164A26E48323EBBE.21019@groups.io>
  2020-10-21 11:32 ` [PATCH v1 11/12] MdeModulePkg/VariableStandaloneMm: Set PcdFlashNvStorageVariableBase to Pcd Sughosh Ganu
                   ` (3 subsequent siblings)
  13 siblings, 2 replies; 28+ messages in thread
From: Sughosh Ganu @ 2020-10-21 11:32 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Sami Mujawar, Jiewen Yao, Achin Gupta

From: Achin Gupta <achin.gupta@arm.com>

Allow setting memory region's permissions using either of the Firmware
Framework(FF-A) ABI transport or through the earlier used SVC calls.

Signed-off-by: Achin Gupta <achin.gupta@arm.com>
Co-developed-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c | 24 ++++++++++++++++----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c b/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
index ab13602556..fef5eb4d22 100644
--- a/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
+++ b/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
@@ -63,17 +63,31 @@ RequestMemoryPermissionChange (
   IN  UINTN                     Permissions
   )
 {
+  BOOLEAN       FfaEnabled;
   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;
+  FfaEnabled = FeaturePcdGet (PcdFfaEnable);
+
+  if (FfaEnabled) {
+    ChangeMemoryPermissionsSvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64;
+    ChangeMemoryPermissionsSvcArgs.Arg1 = 0x3;
+    ChangeMemoryPermissionsSvcArgs.Arg2 = 0;
+    ChangeMemoryPermissionsSvcArgs.Arg3 = ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64;
+    ChangeMemoryPermissionsSvcArgs.Arg4 = BaseAddress;
+    ChangeMemoryPermissionsSvcArgs.Arg5 = EFI_SIZE_TO_PAGES(Length);
+    ChangeMemoryPermissionsSvcArgs.Arg6 = Permissions;
+  } else {
+    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;
+  Status = FfaEnabled ?
+    ChangeMemoryPermissionsSvcArgs.Arg3 : ChangeMemoryPermissionsSvcArgs.Arg0;
 
   switch (Status) {
   case ARM_SVC_SPM_RET_SUCCESS:
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v1 11/12] MdeModulePkg/VariableStandaloneMm: Set PcdFlashNvStorageVariableBase to Pcd
  2020-10-21 11:32 [PATCH v1 00/12] Add support for using FF-A calls sughosh.ganu
                   ` (9 preceding siblings ...)
  2020-10-21 11:32 ` [PATCH v1 10/12] ArmPkg/StandaloneMmMmuLib: Add option to use FF-A calls to set " Sughosh Ganu
@ 2020-10-21 11:32 ` Sughosh Ganu
  2020-11-23 13:52   ` Sami Mujawar
  2020-10-21 11:32 ` [PATCH v1 12/12] StandaloneMmPkg: Allow sending FFA Direct Request message to StandaloneMm Sughosh Ganu
                   ` (2 subsequent siblings)
  13 siblings, 1 reply; 28+ messages in thread
From: Sughosh Ganu @ 2020-10-21 11:32 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Sami Mujawar, Jiewen Yao, Ilias Apalodimas

From: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Instead of running StMM as a SP, OP-TEE creates a new secure partition,
which emulates SPM and isolates StMM from the rest of the Trusted
Applications (TAs). We can then compile StMM as an FD image and run it
in OP-TEE. With the addition of a new RPMB driver, we can leverage OP-TEE
and store variables to an RPMB device.

Since EDK2 upper layers expect byte addressable code, for the RPMB to
work, we need to allocate memory and sync it with the hardware on
read/writes. Since DynamicPCDs are not supported in that context we
can only use PatchablePCDs. So let's switch them to Pcd instead of
FixedPcd and accomodate the new driver.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf
index 6e17f6cdf5..dfed7fe069 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf
@@ -115,10 +115,12 @@
   ## SOMETIMES_PRODUCES   ## Variable:L"VarErrorFlag"
   gEdkiiVarErrorFlagGuid
 
-[FixedPcd]
-  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize       ## CONSUMES
+[Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase       ## SOMETIMES_CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64     ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize       ## CONSUMES
+
+[FixedPcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize                  ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize              ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize          ## CONSUMES
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v1 12/12] StandaloneMmPkg: Allow sending FFA Direct Request message to StandaloneMm
  2020-10-21 11:32 [PATCH v1 00/12] Add support for using FF-A calls sughosh.ganu
                   ` (10 preceding siblings ...)
  2020-10-21 11:32 ` [PATCH v1 11/12] MdeModulePkg/VariableStandaloneMm: Set PcdFlashNvStorageVariableBase to Pcd Sughosh Ganu
@ 2020-10-21 11:32 ` Sughosh Ganu
  2020-11-23 14:17   ` Sami Mujawar
  2020-11-05 10:29 ` [PATCH v1 00/12] Add support for using FF-A calls Sami Mujawar
  2020-11-23 14:33 ` Sami Mujawar
  13 siblings, 1 reply; 28+ messages in thread
From: Sughosh Ganu @ 2020-10-21 11:32 UTC (permalink / raw)
  To: devel
  Cc: Ard Biesheuvel, Sami Mujawar, Jiewen Yao, Ilias Apalodimas,
	Sughosh Ganu

From: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Allow passing of a request to StandaloneMm Core through the Firmware
Framework(FF-A) using FFA_MSG_SEND_DIRECT_REQ method. This method is
used as a mechanism for requesting some service from StandaloneMm.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
index 6a25c4c548..199441f7d2 100644
--- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
+++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
@@ -22,6 +22,7 @@
 #include <Guid/ZeroGuid.h>
 #include <Guid/MmramMemoryReserve.h>
 
+#include <IndustryStandard/ArmFfaSvc.h>
 #include <IndustryStandard/ArmStdSmc.h>
 
 #include "StandaloneMmCpu.h"
@@ -78,7 +79,8 @@ PiMmStandaloneArmTfCpuDriverEntry (
   // receipt of a synchronous MM request. Use the Event ID to distinguish
   // between synchronous and asynchronous events.
   //
-  if (ARM_SMC_ID_MM_COMMUNICATE_AARCH64 != EventId) {
+  if (ARM_SMC_ID_MM_COMMUNICATE_AARCH64 != EventId &&
+    ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64 != EventId) {
     DEBUG ((DEBUG_INFO, "UnRecognized Event - 0x%x\n", EventId));
     return EFI_INVALID_PARAMETER;
   }
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH v1 00/12] Add support for using FF-A calls
  2020-10-21 11:32 [PATCH v1 00/12] Add support for using FF-A calls sughosh.ganu
                   ` (11 preceding siblings ...)
  2020-10-21 11:32 ` [PATCH v1 12/12] StandaloneMmPkg: Allow sending FFA Direct Request message to StandaloneMm Sughosh Ganu
@ 2020-11-05 10:29 ` Sami Mujawar
  2020-11-23 14:33 ` Sami Mujawar
  13 siblings, 0 replies; 28+ messages in thread
From: Sami Mujawar @ 2020-11-05 10:29 UTC (permalink / raw)
  To: Sughosh Ganu, devel@edk2.groups.io; +Cc: Ard Biesheuvel, Jiewen Yao, nd

Hi Sughosh,

Is it possible to share the link to your GitHub branch that has these changes, please?

Regards,

Sami Mujawar

-----Original Message-----
From: Sughosh Ganu <sughosh.ganu@linaro.org> 
Sent: 21 October 2020 12:32 PM
To: devel@edk2.groups.io
Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Sami Mujawar <Sami.Mujawar@arm.com>; Jiewen Yao <jiewen.yao@intel.com>
Subject: [PATCH v1 00/12] Add support for using FF-A calls

Achin Gupta (8):
  ArmPkg/IndustryStandard: Add barebones FF-A header
  ArmPkg/ArmSvcLib: Return x4-x7 in output parameters
  StandaloneMmPkg: Use FF-A header file in Standalone MM Core entry
    point
  StandaloneMmPkg: Add option to use FF-A calls for getting SPM version
  StandaloneMmPkg: Add option to use FF-A calls for communication with
    SPM
  StandaloneMmPkg: Use FF-A header file in Standalone MM Arm MMU library
  ArmPkg/StandaloneMmMmuLib: Add option to use FF-A calls to get memory
    region's permissions
  ArmPkg/StandaloneMmMmuLib: Add option to use FF-A calls to set memory
    region's permissions

Ilias Apalodimas (2):
  MdeModulePkg/VariableStandaloneMm: Set PcdFlashNvStorageVariableBase
    to Pcd
  StandaloneMmPkg: Allow sending FFA Direct Request message to
    StandaloneMm

Sughosh Ganu (1):
  ArmPkg: Introduce support for PcdFfaEnable

Sughossh Ganu (1):
  StandaloneMmPkg: Add the SPM version for FF-A

 ArmPkg/ArmPkg.dec                             |   3 +
 .../ArmMmuStandaloneMmLib.inf                 |   3 +
 .../RuntimeDxe/VariableStandaloneMm.inf       |   6 +-
 .../StandaloneMmCoreEntryPoint.inf            |   3 +
 ArmPkg/Include/IndustryStandard/ArmFfaSvc.h   |  16 +++
 .../AArch64/ArmMmuStandaloneMmLib.c           |  53 +++++++--
 .../StandaloneMmCpu/AArch64/EventHandle.c     |   4 +-
 .../AArch64/StandaloneMmCoreEntryPoint.c      | 103 ++++++++++++++----
 ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S     |   2 +
 9 files changed, 155 insertions(+), 38 deletions(-)
 create mode 100644 ArmPkg/Include/IndustryStandard/ArmFfaSvc.h

-- 
2.17.1



^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v1 01/12] ArmPkg/IndustryStandard: Add barebones FF-A header
  2020-10-21 11:32 ` [PATCH v1 01/12] ArmPkg/IndustryStandard: Add barebones FF-A header Sughosh Ganu
@ 2020-11-05 10:33   ` Sami Mujawar
  2020-11-06  9:06     ` Sughosh Ganu
  0 siblings, 1 reply; 28+ messages in thread
From: Sami Mujawar @ 2020-11-05 10:33 UTC (permalink / raw)
  To: Sughosh Ganu, devel@edk2.groups.io
  Cc: Ard Biesheuvel, Jiewen Yao, Achin Gupta, nd

Hi Sughosh,

Thank you for this patch.

Please see my response inline marked [SAMI].

Regards,

Sami Mujawar

-----Original Message-----
From: Sughosh Ganu <sughosh.ganu@linaro.org> 
Sent: 21 October 2020 12:32 PM
To: devel@edk2.groups.io
Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Sami Mujawar <Sami.Mujawar@arm.com>; Jiewen Yao <jiewen.yao@intel.com>; Achin Gupta <Achin.Gupta@arm.com>
Subject: [PATCH v1 01/12] ArmPkg/IndustryStandard: Add barebones FF-A header

From: Achin Gupta <achin.gupta@arm.com>

This patch adds a rudimentary header file with defines for FF-A ABIs
that will be used as the transport between S-EL0 and the SPM

Signed-off-by: Achin Gupta <achin.gupta@arm.com>
---
 ArmPkg/Include/IndustryStandard/ArmFfaSvc.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h b/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
new file mode 100644
index 0000000000..a8914085de
--- /dev/null
+++ b/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
@@ -0,0 +1,16 @@
+/** @file
[SAMI] Please add file description, see section 5.2.3.1 of EDKII coding standard specification (https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/52_spacing)
[/SAMI]
+*
+*  Copyright (c) 2020, ARM Limited. All rights reserved.
+*
+*  SPDX-License-Identifier: BSD-2-Clause-Patent
[SAMI] Add reference to the specification, see section 5.2.3.5 of EDKII coding standard specification.
[/SAMI]
+*
+**/
+
+#ifndef __ARM_FFA_SVC_H__
+#define __ARM_FFA_SVC_H__
[SAMI] Make include guard compliant with EDKII coding standard, see section 5.3.5 (https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/53_include_files)
[/SAMI]
+
+#define ARM_SVC_ID_FFA_VERSION_AARCH32                  0x84000063
+#define ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64      0xC400006F
+#define ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64     0xC4000070
+
+#endif
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH v1 01/12] ArmPkg/IndustryStandard: Add barebones FF-A header
  2020-11-05 10:33   ` Sami Mujawar
@ 2020-11-06  9:06     ` Sughosh Ganu
  0 siblings, 0 replies; 28+ messages in thread
From: Sughosh Ganu @ 2020-11-06  9:06 UTC (permalink / raw)
  To: Sami Mujawar
  Cc: devel@edk2.groups.io, Ard Biesheuvel, Jiewen Yao, Achin Gupta, nd

[-- Attachment #1: Type: text/plain, Size: 2345 bytes --]

hi Sami,

On Thu, 5 Nov 2020 at 16:04, Sami Mujawar <Sami.Mujawar@arm.com> wrote:

> Hi Sughosh,
>
> Thank you for this patch.
>
> Please see my response inline marked [SAMI].
>

Thanks for your review comments. I will incorporate the comments in the v2
series. Will wait for a few more days for any other review comments on the
patch series.

-sughosh


>
> Regards,
>
> Sami Mujawar
>
> -----Original Message-----
> From: Sughosh Ganu <sughosh.ganu@linaro.org>
> Sent: 21 October 2020 12:32 PM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Sami Mujawar <
> Sami.Mujawar@arm.com>; Jiewen Yao <jiewen.yao@intel.com>; Achin Gupta <
> Achin.Gupta@arm.com>
> Subject: [PATCH v1 01/12] ArmPkg/IndustryStandard: Add barebones FF-A
> header
>
> From: Achin Gupta <achin.gupta@arm.com>
>
> This patch adds a rudimentary header file with defines for FF-A ABIs
> that will be used as the transport between S-EL0 and the SPM
>
> Signed-off-by: Achin Gupta <achin.gupta@arm.com>
> ---
>  ArmPkg/Include/IndustryStandard/ArmFfaSvc.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
> b/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
> new file mode 100644
> index 0000000000..a8914085de
> --- /dev/null
> +++ b/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
> @@ -0,0 +1,16 @@
> +/** @file
> [SAMI] Please add file description, see section 5.2.3.1 of EDKII coding
> standard specification (
> https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/52_spacing
> )
> [/SAMI]
> +*
> +*  Copyright (c) 2020, ARM Limited. All rights reserved.
> +*
> +*  SPDX-License-Identifier: BSD-2-Clause-Patent
> [SAMI] Add reference to the specification, see section 5.2.3.5 of EDKII
> coding standard specification.
> [/SAMI]
> +*
> +**/
> +
> +#ifndef __ARM_FFA_SVC_H__
> +#define __ARM_FFA_SVC_H__
> [SAMI] Make include guard compliant with EDKII coding standard, see
> section 5.3.5 (
> https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/53_include_files
> )
> [/SAMI]
> +
> +#define ARM_SVC_ID_FFA_VERSION_AARCH32                  0x84000063
> +#define ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64      0xC400006F
> +#define ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64     0xC4000070
> +
> +#endif
> --
> 2.17.1
>
>

[-- Attachment #2: Type: text/html, Size: 3800 bytes --]

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v1 02/12] ArmPkg/ArmSvcLib: Return x4-x7 in output parameters
  2020-10-21 11:32 ` [PATCH v1 02/12] ArmPkg/ArmSvcLib: Return x4-x7 in output parameters Sughosh Ganu
@ 2020-11-10 12:02   ` Sami Mujawar
  0 siblings, 0 replies; 28+ messages in thread
From: Sami Mujawar @ 2020-11-10 12:02 UTC (permalink / raw)
  To: Sughosh Ganu, devel@edk2.groups.io
  Cc: Ard Biesheuvel, Jiewen Yao, Achin Gupta, nd

Hi Sughosh,

Please find my response inline marked [SAMI].

General feedback:
- Add reference to the SMCC v1.2 spec and FF-A spec in the file header.
- Do you plan to submit a similar patch for AArch32?
- Can you update the function documentation for ArmCallSvc () in ArmPkg\Include\Library\ArmSvcLib.h, please? 
  Ref: https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/appendix_a_common_examples#functioon-declarations

Regards,

Sami Mujawar

-----Original Message-----
From: Sughosh Ganu <sughosh.ganu@linaro.org> 
Sent: 21 October 2020 12:32 PM
To: devel@edk2.groups.io
Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Sami Mujawar <Sami.Mujawar@arm.com>; Jiewen Yao <jiewen.yao@intel.com>; Achin Gupta <Achin.Gupta@arm.com>
Subject: [PATCH v1 02/12] ArmPkg/ArmSvcLib: Return x4-x7 in output parameters

From: Achin Gupta <achin.gupta@arm.com>

The Arm SMC calling convention standard v1.2 allows 8 input and output
parameter registers. The FF-A specification relies on this
communication. This patch extends the number of output registers
returned by ArmCallSvc() to match this convention.

Signed-off-by: Achin Gupta <achin.gupta@arm.com>
---
 ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S b/ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S
index ee265f94b9..8cb5c45582 100644
--- a/ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S
+++ b/ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S
@@ -36,6 +36,8 @@ ASM_PFX(ArmCallSvc):
   // A SVC call can return up to 4 values - we do not need to store back x4-x7.

[SAMI] Please update the comment to reflect that 8 values can be returned. 
[/SAMI]

   stp   x0, x1, [x9, #0]
   stp   x2, x3, [x9, #16]
+  stp   x4, x5, [x9, #32]
+  stp   x6, x7, [x9, #48]
 
   mov   x0, x9
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH v1 05/12] StandaloneMmPkg: Add the SPM version for FF-A
  2020-10-21 11:32 ` [PATCH v1 05/12] StandaloneMmPkg: Add the SPM version for FF-A Sughosh Ganu
@ 2020-11-19  9:54   ` Sami Mujawar
  0 siblings, 0 replies; 28+ messages in thread
From: Sami Mujawar @ 2020-11-19  9:54 UTC (permalink / raw)
  To: Sughosh Ganu, devel@edk2.groups.io; +Cc: Ard Biesheuvel, Jiewen Yao, nd

Hi Sughosh,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

-----Original Message-----
From: Sughosh Ganu <sughosh.ganu@linaro.org> 
Sent: 21 October 2020 12:32 PM
To: devel@edk2.groups.io
Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Sami Mujawar <Sami.Mujawar@arm.com>; Jiewen Yao <jiewen.yao@intel.com>; Sughossh Ganu <sughosh.ganu@linaro.org>
Subject: [PATCH v1 05/12] StandaloneMmPkg: Add the SPM version for FF-A

From: Sughossh Ganu <sughosh.ganu@linaro.org>

The Firmware Framework(FF-A) requires implementation of SPM version
v1.0. Add new macros for the version that will be used for FF-A.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
index c4132b0d78..33f0db654f 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
@@ -32,6 +32,9 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #define SPM_MINOR_VER_MASK        0x0000FFFF
 #define SPM_MAJOR_VER_SHIFT       16
 
+CONST UINT32 SPM_MAJOR_VER_FFA = 1;
+CONST UINT32 SPM_MINOR_VER_FFA = 0;
[SAMI] I think these should be defined using macros, see https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/4_naming_conventions/43_identifiers#4-3-5-1-use-all-capital-letters-for-both-define-and-typedef-declarations
Or 
These variables can be declared as static and the variable naming convention as defined in https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/4_naming_conventions/43_identifiers#4-3-3-2-any-variable-with-file-scope-or-better-shall-be-prefixed-by-an-m-or-g
must be followed.
[/SAMI]

+
 CONST UINT32 SPM_MAJOR_VER = 0;
 CONST UINT32 SPM_MINOR_VER = 1;
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH v1 04/12] ArmPkg: Introduce support for PcdFfaEnable
  2020-10-21 11:32 ` [PATCH v1 04/12] ArmPkg: Introduce support for PcdFfaEnable Sughosh Ganu
@ 2020-11-19  9:56   ` Sami Mujawar
  0 siblings, 0 replies; 28+ messages in thread
From: Sami Mujawar @ 2020-11-19  9:56 UTC (permalink / raw)
  To: Sughosh Ganu, devel@edk2.groups.io; +Cc: Ard Biesheuvel, Jiewen Yao, nd

Hi Sughosh,

Please find my response inline marked [SAMI].

With that changed:
Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

-----Original Message-----
From: Sughosh Ganu <sughosh.ganu@linaro.org> 
Sent: 21 October 2020 12:32 PM
To: devel@edk2.groups.io
Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Sami Mujawar <Sami.Mujawar@arm.com>; Jiewen Yao <jiewen.yao@intel.com>; Sughosh Ganu <sughosh.ganu@linaro.org>
Subject: [PATCH v1 04/12] ArmPkg: Introduce support for PcdFfaEnable

The Secure Partition(SP) can request services from the Secure
Partition Manager Core(SPMC) either through FF-A calls or through the
existing SVC calls. Add a feature flag Pcd for enabling the FF-A
method -- when this is set to FALSE, the SP uses the existing SVC
calls for making the requests.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 ArmPkg/ArmPkg.dec | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
index eaf1072d9e..979f3e2699 100644
--- a/ArmPkg/ArmPkg.dec
+++ b/ArmPkg/ArmPkg.dec
@@ -78,6 +78,9 @@
   # Define if the GICv3 controller should use the GICv2 legacy
   gArmTokenSpaceGuid.PcdArmGicV3WithV2Legacy|FALSE|BOOLEAN|0x00000042
 
+  # Used to select method for requesting services from S-EL1
[SAMI] Can you follow the documentation style for PCD definition as done in MdePkg\MdePkg.dec, please?
More information can be found at https://github.com/tianocore-docs/edk2-DecSpecification/blob/master/3_edk_ii_dec_file_format/310_pcd_sections.md
[/SAMI]
+  gArmTokenSpaceGuid.PcdFfaEnable|FALSE|BOOLEAN|0x0000005B
+
 [PcdsFeatureFlag.ARM]
   # Whether to map normal memory as non-shareable. FALSE is the safe choice, but
   # TRUE may be appropriate to fix performance problems if you don't care about
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH v1 06/12] StandaloneMmPkg: Add option to use FF-A calls for getting SPM version
  2020-10-21 11:32 ` [PATCH v1 06/12] StandaloneMmPkg: Add option to use FF-A calls for getting SPM version Sughosh Ganu
@ 2020-11-23 10:10   ` Sami Mujawar
  0 siblings, 0 replies; 28+ messages in thread
From: Sami Mujawar @ 2020-11-23 10:10 UTC (permalink / raw)
  To: Sughosh Ganu, devel@edk2.groups.io
  Cc: Ard Biesheuvel, Jiewen Yao, Achin Gupta, nd

Hi Sughosh,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

-----Original Message-----
From: Sughosh Ganu <sughosh.ganu@linaro.org> 
Sent: 21 October 2020 12:32 PM
To: devel@edk2.groups.io
Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Sami Mujawar <Sami.Mujawar@arm.com>; Jiewen Yao <jiewen.yao@intel.com>; Achin Gupta <Achin.Gupta@arm.com>
Subject: [PATCH v1 06/12] StandaloneMmPkg: Add option to use FF-A calls for getting SPM version

From: Achin Gupta <achin.gupta@arm.com>

With the introduction of Firmware Framework(FF-A), a Secure Partition
can get the SPM version either using FF-A calls or through the
existing svc calls. Use a runtime check to use either of the two
methods based on the Pcd feature flag value.

Signed-off-by: Achin Gupta <achin.gupta@arm.com>
Co-developed-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf       |  3 ++
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c | 31 ++++++++++++++------
 2 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
index 7d6ee4e08e..f8184f501b 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
@@ -49,5 +49,8 @@
   gEfiStandaloneMmNonSecureBufferGuid
   gEfiArmTfCpuDriverEpDescriptorGuid
 
+[Pcd]
+  gArmTokenSpaceGuid.PcdFfaEnable
+
 [BuildOptions]
   GCC:*_*_*_CC_FLAGS = -fpie
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
index 33f0db654f..7d1e3c4d7c 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
@@ -23,6 +23,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Library/BaseLib.h>
 #include <Library/BaseMemoryLib.h>
 #include <Library/SerialPortLib.h>
+#include <Library/PcdLib.h>
 
 #include <IndustryStandard/ArmStdSmc.h>
 #include <IndustryStandard/ArmMmSvc.h>
@@ -165,19 +166,31 @@ EFI_STATUS
 GetSpmVersion (VOID)
 {
   EFI_STATUS   Status;
-  UINT16       SpmMajorVersion;
-  UINT16       SpmMinorVersion;
+  UINT16       CurSpmMajorVer;
+  UINT16       SpmMajorVer;

[SAMI] I think a 'Caller & Callee' prefix would add clarity. See FFA Spec, Section 8.1.1 [/SAMI]

+  UINT16       CurSpmMinorVer;
+  UINT16       SpmMinorVer;
   UINT32       SpmVersion;
   ARM_SVC_ARGS SpmVersionArgs;
 
-  SpmVersionArgs.Arg0 = ARM_SVC_ID_SPM_VERSION_AARCH32;
+  if (FeaturePcdGet (PcdFfaEnable)) {
+    SpmVersionArgs.Arg0 = ARM_SVC_ID_FFA_VERSION_AARCH32;
+    SpmVersionArgs.Arg1 = SPM_MAJOR_VER << SPM_MAJOR_VER_SHIFT;
+    SpmVersionArgs.Arg1 |= SPM_MINOR_VER;
+    SpmMajorVer = SPM_MAJOR_VER_FFA;
+    SpmMinorVer = SPM_MINOR_VER_FFA;
+  } else {
+    SpmVersionArgs.Arg0 = ARM_SVC_ID_SPM_VERSION_AARCH32;
+    SpmMajorVer = SPM_MAJOR_VER;
+    SpmMinorVer = SPM_MINOR_VER;
+  }
 
   ArmCallSvc (&SpmVersionArgs);
 
   SpmVersion = SpmVersionArgs.Arg0;

 [SAMI] The Version FID for FFA and for SPM based on the MM interface both return NOT_SUPPORTED as an error code. 
Can we check the error code for failure, please?
[/SAMI]

-  SpmMajorVersion = ((SpmVersion & SPM_MAJOR_VER_MASK) >> SPM_MAJOR_VER_SHIFT);
-  SpmMinorVersion = ((SpmVersion & SPM_MINOR_VER_MASK) >> 0);
+  CurSpmMajorVer = ((SpmVersion & SPM_MAJOR_VER_MASK) >> SPM_MAJOR_VER_SHIFT);
+  CurSpmMinorVer = ((SpmVersion & SPM_MINOR_VER_MASK) >> 0);
 
   // Different major revision values indicate possibly incompatible functions.
   // For two revisions, A and B, for which the major revision values are
@@ -186,17 +199,17 @@ GetSpmVersion (VOID)
   // revision A must work in a compatible way with revision B.
   // However, it is possible for revision B to have a higher
   // function count than revision A.
-  if ((SpmMajorVersion == SPM_MAJOR_VER) &&
-      (SpmMinorVersion >= SPM_MINOR_VER))
+  if ((CurSpmMajorVer == SpmMajorVer) &&
+      (CurSpmMinorVer >= SpmMinorVer))
   {
     DEBUG ((DEBUG_INFO, "SPM Version: Major=0x%x, Minor=0x%x\n",
-           SpmMajorVersion, SpmMinorVersion));
+           CurSpmMajorVer, CurSpmMinorVer));
     Status = EFI_SUCCESS;
   }
   else
   {
     DEBUG ((DEBUG_INFO, "Incompatible SPM Versions.\n Current Version: Major=0x%x, Minor=0x%x.\n Expected: Major=0x%x, Minor>=0x%x.\n",
-            SpmMajorVersion, SpmMinorVersion, SPM_MAJOR_VER, SPM_MINOR_VER));
+            CurSpmMajorVer, CurSpmMinorVer, SpmMajorVer, SpmMinorVer));
     Status = EFI_UNSUPPORTED;
   }
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH v1 07/12] StandaloneMmPkg: Add option to use FF-A calls for communication with SPM
  2020-10-21 11:32 ` [PATCH v1 07/12] StandaloneMmPkg: Add option to use FF-A calls for communication with SPM Sughosh Ganu
@ 2020-11-23 12:38   ` Sami Mujawar
  0 siblings, 0 replies; 28+ messages in thread
From: Sami Mujawar @ 2020-11-23 12:38 UTC (permalink / raw)
  To: Sughosh Ganu, devel@edk2.groups.io
  Cc: Ard Biesheuvel, Jiewen Yao, Achin Gupta, nd

Hi Sughosh,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar
-----Original Message-----
From: Sughosh Ganu <mailto:sughosh.ganu@linaro.org> 
Sent: 21 October 2020 12:32 PM
To: mailto:devel@edk2.groups.io
Cc: Ard Biesheuvel <mailto:Ard.Biesheuvel@arm.com>; Sami Mujawar <mailto:Sami.Mujawar@arm.com>; Jiewen Yao <mailto:jiewen.yao@intel.com>; Achin Gupta <mailto:Achin.Gupta@arm.com>
Subject: [PATCH v1 07/12] StandaloneMmPkg: Add option to use FF-A calls for communication with SPM

From: Achin Gupta <mailto:achin.gupta@arm.com>

Add support for reporting completion of a MM request using either the
Firmware Framework(FF-A) ABI transport or through the earlier used SVC
calls.

Signed-off-by: Achin Gupta <mailto:achin.gupta@arm.com>
Co-developed-by: Sughosh Ganu <mailto:sughosh.ganu@linaro.org>
---
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c | 68 ++++++++++++++++----
 1 file changed, 54 insertions(+), 14 deletions(-)

diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
index 7d1e3c4d7c..06a8f487f9 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
@@ -112,6 +112,7 @@ DelegatedEventLoop (
   IN ARM_SVC_ARGS *EventCompleteSvcArgs
   )
 {
+  BOOLEAN FfaEnabled;
   EFI_STATUS Status;
   UINTN SvcStatus;
 
@@ -123,16 +124,32 @@ DelegatedEventLoop (
     DEBUG ((DEBUG_INFO, "X1 :  0x%x\n", (UINT32) EventCompleteSvcArgs->Arg1));
     DEBUG ((DEBUG_INFO, "X2 :  0x%x\n", (UINT32) EventCompleteSvcArgs->Arg2));
     DEBUG ((DEBUG_INFO, "X3 :  0x%x\n", (UINT32) EventCompleteSvcArgs->Arg3));
-
-    Status = CpuDriverEntryPoint (
-               EventCompleteSvcArgs->Arg0,
-               EventCompleteSvcArgs->Arg3,
-               EventCompleteSvcArgs->Arg1
-               );
-
-    if (EFI_ERROR (Status)) {
-      DEBUG ((DEBUG_ERROR, "Failed delegated event 0x%x, Status 0x%x\n",
-              EventCompleteSvcArgs->Arg0, Status));
+    DEBUG ((DEBUG_INFO, "X4 :  0x%x\n", (UINT32) EventCompleteSvcArgs->Arg4));
+    DEBUG ((DEBUG_INFO, "X5 :  0x%x\n", (UINT32) EventCompleteSvcArgs->Arg5));
+    DEBUG ((DEBUG_INFO, "X6 :  0x%x\n", (UINT32) EventCompleteSvcArgs->Arg6));
+    DEBUG ((DEBUG_INFO, "X7 :  0x%x\n", (UINT32) EventCompleteSvcArgs->Arg7));
+
+    FfaEnabled = FeaturePcdGet (PcdFfaEnable);
+    if (FfaEnabled) {
+      Status = CpuDriverEntryPoint (
+                 EventCompleteSvcArgs->Arg0,
+                 EventCompleteSvcArgs->Arg6,
+                 EventCompleteSvcArgs->Arg3
+                 );
+      if (EFI_ERROR (Status)) {
+        DEBUG ((DEBUG_ERROR, "Failed delegated event 0x%x, Status 0x%x\n",
+                EventCompleteSvcArgs->Arg3, Status));

[SAMI] Please align as suggested in the coding guide, 
see https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/52_spacing#5-2-2-4-subsequent-lines-of-multi-line-function-calls-should-line-up-two-spaces-from-the-beginning-of-the-function-name
[/SAMI]

+      }
+    } else {
+      Status = CpuDriverEntryPoint (
+                 EventCompleteSvcArgs->Arg0,
+                 EventCompleteSvcArgs->Arg3,
+                 EventCompleteSvcArgs->Arg1
+                 );
+      if (EFI_ERROR (Status)) {
+        DEBUG ((DEBUG_ERROR, "Failed delegated event 0x%x, Status 0x%x\n",
+                EventCompleteSvcArgs->Arg0, Status));

[SAMI] Please align as suggested in the coding guide. [/SAMI]

+      }
     }
 
     switch (Status) {
@@ -156,8 +173,16 @@ DelegatedEventLoop (
       break;
     }
 
-    EventCompleteSvcArgs->Arg0 = ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64;
-    EventCompleteSvcArgs->Arg1 = SvcStatus;
+    if (FfaEnabled) {
+      EventCompleteSvcArgs->Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64;
+      EventCompleteSvcArgs->Arg1 = 0;
+      EventCompleteSvcArgs->Arg2 = 0;
+      EventCompleteSvcArgs->Arg3 = ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64;
+      EventCompleteSvcArgs->Arg4 = SvcStatus;
+    } else {
+      EventCompleteSvcArgs->Arg0 = ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64;
+      EventCompleteSvcArgs->Arg1 = SvcStatus;
+    }
   }
 }
 
@@ -216,6 +241,22 @@ GetSpmVersion (VOID)
   return Status;
 }
 
+STATIC
+VOID
+InitArmSvcArgs (ARM_SVC_ARGS *InitMmFoundationSvcArgs, EFI_STATUS *Status)
[SAMI] Please follow coding guidelines for function definition and documentation.
Ref: https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/57_c_programming#5-7-1-function-definition-layout 
[/SAMI]

+{
+  if (FeaturePcdGet (PcdFfaEnable)) {
+    InitMmFoundationSvcArgs->Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64;
+    InitMmFoundationSvcArgs->Arg1 = 0;
+    InitMmFoundationSvcArgs->Arg2 = 0;
+    InitMmFoundationSvcArgs->Arg3 = ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64;
+    InitMmFoundationSvcArgs->Arg4 = *Status;
+  } else {
+    InitMmFoundationSvcArgs->Arg0 = ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64;
+    InitMmFoundationSvcArgs->Arg1 = *Status;
[SAMI] The 'Event Status code' is a 32-bit value, see https://trustedfirmware-a.readthedocs.io/en/latest/components/secure-partition-manager-mm.html#mm-sp-event-complete-aarch64
When this code is compiled for AARCH64, EFI_STATUS is 64-Bit value. In case of an error the status code will have the bit 63 set, see definition for macro ENCODE_ERROR(). 
This would mean a 32-bit positive value would be returned in an error condition, right? 
[/SAMI]

+  }
+}
+
 /**
   The entry point of Standalone MM Foundation.
 
@@ -329,7 +370,6 @@ _ModuleEntryPoint (
 
 finish:
   ZeroMem (&InitMmFoundationSvcArgs, sizeof(InitMmFoundationSvcArgs));
-  InitMmFoundationSvcArgs.Arg0 = ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64;
-  InitMmFoundationSvcArgs.Arg1 = Status;
+  InitArmSvcArgs(&InitMmFoundationSvcArgs, &Status);
[SAMI] Space needed between function name and starting parentheses.
Ref: https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/52_spacing#5-2-2-6-always-put-space-before-an-open-parenthesis
[/SAMI] 

   DelegatedEventLoop (&InitMmFoundationSvcArgs);
 }
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH v1 09/12] ArmPkg/StandaloneMmMmuLib: Add option to use FF-A calls to get memory region's permissions
  2020-10-21 11:32 ` [PATCH v1 09/12] ArmPkg/StandaloneMmMmuLib: Add option to use FF-A calls to get memory region's permissions Sughosh Ganu
@ 2020-11-23 13:26   ` Sami Mujawar
  0 siblings, 0 replies; 28+ messages in thread
From: Sami Mujawar @ 2020-11-23 13:26 UTC (permalink / raw)
  To: Sughosh Ganu, devel@edk2.groups.io
  Cc: Ard Biesheuvel, Jiewen Yao, Achin Gupta, nd

Hi Sughosh,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

-----Original Message-----
From: Sughosh Ganu <sughosh.ganu@linaro.org> 
Sent: 21 October 2020 12:33 PM
To: devel@edk2.groups.io
Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Sami Mujawar <Sami.Mujawar@arm.com>; Jiewen Yao <jiewen.yao@intel.com>; Achin Gupta <Achin.Gupta@arm.com>
Subject: [PATCH v1 09/12] ArmPkg/StandaloneMmMmuLib: Add option to use FF-A calls to get memory region's permissions

[SAMI] Please abbreviate the subject line if possible. [/SAMI]

From: Achin Gupta <achin.gupta@arm.com>

Allow getting memory region's permissions using either of the Firmware
Framework(FF-A) ABI transport or through the earlier used SVC calls.

Signed-off-by: Achin Gupta <achin.gupta@arm.com>
Co-developed-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf       |  3 +++
 ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c | 28 +++++++++++++++-----
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
index 85973687f5..a29dd800b5 100644
--- a/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
+++ b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
@@ -23,6 +23,9 @@
   ArmPkg/ArmPkg.dec
   MdePkg/MdePkg.dec
 
+[Pcd]
+  gArmTokenSpaceGuid.PcdFfaEnable
+
 [LibraryClasses]
   ArmLib
   CacheMaintenanceLib
diff --git a/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c b/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
index 362b1a0f8a..ab13602556 100644
--- a/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
+++ b/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
@@ -16,6 +16,7 @@
 #include <Library/ArmSvcLib.h>
 #include <Library/BaseLib.h>
 #include <Library/DebugLib.h>
+#include <Library/PcdLib.h>
 
 STATIC
 EFI_STATUS
@@ -25,19 +26,32 @@ GetMemoryPermissions (
   )
 {
   ARM_SVC_ARGS  GetMemoryPermissionsSvcArgs = {0};
-
-  GetMemoryPermissionsSvcArgs.Arg0 = ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64;
-  GetMemoryPermissionsSvcArgs.Arg1 = BaseAddress;
-  GetMemoryPermissionsSvcArgs.Arg2 = 0;
-  GetMemoryPermissionsSvcArgs.Arg3 = 0;
+  BOOLEAN FfaEnabled;
+
+  FfaEnabled = FeaturePcdGet (PcdFfaEnable);
+  if (FfaEnabled) {
+    GetMemoryPermissionsSvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64;
+    GetMemoryPermissionsSvcArgs.Arg1 = 0x3;
+    GetMemoryPermissionsSvcArgs.Arg2 = 0;
+    GetMemoryPermissionsSvcArgs.Arg3 = ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64;
+    GetMemoryPermissionsSvcArgs.Arg4 = BaseAddress;
+  } else {
+    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) {
+  if (GetMemoryPermissionsSvcArgs.Arg0 == ARM_SVC_SPM_RET_INVALID_PARAMS ||
+      GetMemoryPermissionsSvcArgs.Arg3 == ARM_SVC_SPM_RET_INVALID_PARAMS) {

[SAMI] Other error codes can also be returned and must be checked. 
See FFA Spec, section 10.2 and also https://trustedfirmware-a.readthedocs.io/en/latest/components/secure-partition-manager-mm.html#mm-sp-memory-attributes-get-aarch64

For FFA the error codes would be returned in Arg0 & Arg3. For SPM based on the MM interface the error code would be in Arg0.
It might be better to check if FfaEnabled is enabled here and handle the errors appropriately. Also, it could be combined with the following code that returns the Memory attributes.
[/SAMI]

     *MemoryAttributes = 0;
     return EFI_INVALID_PARAMETER;
   }
 
-  *MemoryAttributes = GetMemoryPermissionsSvcArgs.Arg0;
+  *MemoryAttributes = FfaEnabled ?
+    GetMemoryPermissionsSvcArgs.Arg3 : GetMemoryPermissionsSvcArgs.Arg0;
+
   return EFI_SUCCESS;
 }
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH v1 10/12] ArmPkg/StandaloneMmMmuLib: Add option to use FF-A calls to set memory region's permissions
  2020-10-21 11:32 ` [PATCH v1 10/12] ArmPkg/StandaloneMmMmuLib: Add option to use FF-A calls to set " Sughosh Ganu
@ 2020-11-23 13:42   ` Sami Mujawar
       [not found]   ` <164A26E48323EBBE.21019@groups.io>
  1 sibling, 0 replies; 28+ messages in thread
From: Sami Mujawar @ 2020-11-23 13:42 UTC (permalink / raw)
  To: Sughosh Ganu, devel@edk2.groups.io
  Cc: Ard Biesheuvel, Jiewen Yao, Achin Gupta, nd

Hi Sughosh,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

-----Original Message-----
From: Sughosh Ganu <sughosh.ganu@linaro.org> 
Sent: 21 October 2020 12:33 PM
To: devel@edk2.groups.io
Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Sami Mujawar <Sami.Mujawar@arm.com>; Jiewen Yao <jiewen.yao@intel.com>; Achin Gupta <Achin.Gupta@arm.com>
Subject: [PATCH v1 10/12] ArmPkg/StandaloneMmMmuLib: Add option to use FF-A calls to set memory region's permissions

From: Achin Gupta <achin.gupta@arm.com>

Allow setting memory region's permissions using either of the Firmware
Framework(FF-A) ABI transport or through the earlier used SVC calls.

Signed-off-by: Achin Gupta <achin.gupta@arm.com>
Co-developed-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c | 24 ++++++++++++++++----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c b/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
index ab13602556..fef5eb4d22 100644
--- a/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
+++ b/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
@@ -63,17 +63,31 @@ RequestMemoryPermissionChange (
   IN  UINTN                     Permissions
   )
 {
+  BOOLEAN       FfaEnabled;
   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;
+  FfaEnabled = FeaturePcdGet (PcdFfaEnable);
+
+  if (FfaEnabled) {
+    ChangeMemoryPermissionsSvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64;
+    ChangeMemoryPermissionsSvcArgs.Arg1 = 0x3;
+    ChangeMemoryPermissionsSvcArgs.Arg2 = 0;
+    ChangeMemoryPermissionsSvcArgs.Arg3 = ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64;
+    ChangeMemoryPermissionsSvcArgs.Arg4 = BaseAddress;
+    ChangeMemoryPermissionsSvcArgs.Arg5 = EFI_SIZE_TO_PAGES(Length);

[SAMI] Add a space between EFI_SIZE_TO_PAGES and starting parenthesis.  [/SAMI]

+    ChangeMemoryPermissionsSvcArgs.Arg6 = Permissions;
+  } else {
+    ChangeMemoryPermissionsSvcArgs.Arg0 = ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64;
+    ChangeMemoryPermissionsSvcArgs.Arg1 = BaseAddress;
+    ChangeMemoryPermissionsSvcArgs.Arg2 = EFI_SIZE_TO_PAGES(Length);

[SAMI] Add a space between EFI_SIZE_TO_PAGES and starting parenthesis.  [/SAMI]

+    ChangeMemoryPermissionsSvcArgs.Arg3 = Permissions;
+  }
 
   ArmCallSvc (&ChangeMemoryPermissionsSvcArgs);
 
-  Status = ChangeMemoryPermissionsSvcArgs.Arg0;
+  Status = FfaEnabled ?
+    ChangeMemoryPermissionsSvcArgs.Arg3 : ChangeMemoryPermissionsSvcArgs.Arg0;
[SAMI] In case of FFA, Arg0 may also contain an error code, right? See FFA spec, section 10.2.
[/SAMI]
 
   switch (Status) {
   case ARM_SVC_SPM_RET_SUCCESS:
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH v1 11/12] MdeModulePkg/VariableStandaloneMm: Set PcdFlashNvStorageVariableBase to Pcd
  2020-10-21 11:32 ` [PATCH v1 11/12] MdeModulePkg/VariableStandaloneMm: Set PcdFlashNvStorageVariableBase to Pcd Sughosh Ganu
@ 2020-11-23 13:52   ` Sami Mujawar
  0 siblings, 0 replies; 28+ messages in thread
From: Sami Mujawar @ 2020-11-23 13:52 UTC (permalink / raw)
  To: Sughosh Ganu, devel@edk2.groups.io
  Cc: Ard Biesheuvel, Jiewen Yao, Ilias Apalodimas, nd

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

-----Original Message-----
From: Sughosh Ganu <sughosh.ganu@linaro.org> 
Sent: 21 October 2020 12:33 PM
To: devel@edk2.groups.io
Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Sami Mujawar <Sami.Mujawar@arm.com>; Jiewen Yao <jiewen.yao@intel.com>; Ilias Apalodimas <ilias.apalodimas@linaro.org>
Subject: [PATCH v1 11/12] MdeModulePkg/VariableStandaloneMm: Set PcdFlashNvStorageVariableBase to Pcd

From: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Instead of running StMM as a SP, OP-TEE creates a new secure partition,
which emulates SPM and isolates StMM from the rest of the Trusted
Applications (TAs). We can then compile StMM as an FD image and run it
in OP-TEE. With the addition of a new RPMB driver, we can leverage OP-TEE
and store variables to an RPMB device.

Since EDK2 upper layers expect byte addressable code, for the RPMB to
work, we need to allocate memory and sync it with the hardware on
read/writes. Since DynamicPCDs are not supported in that context we
can only use PatchablePCDs. So let's switch them to Pcd instead of
FixedPcd and accomodate the new driver.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf
index 6e17f6cdf5..dfed7fe069 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf
@@ -115,10 +115,12 @@
   ## SOMETIMES_PRODUCES   ## Variable:L"VarErrorFlag"
   gEdkiiVarErrorFlagGuid
 
-[FixedPcd]
-  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize       ## CONSUMES
+[Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase       ## SOMETIMES_CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64     ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize       ## CONSUMES
+
+[FixedPcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize                  ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize              ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize          ## CONSUMES
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH v1 12/12] StandaloneMmPkg: Allow sending FFA Direct Request message to StandaloneMm
  2020-10-21 11:32 ` [PATCH v1 12/12] StandaloneMmPkg: Allow sending FFA Direct Request message to StandaloneMm Sughosh Ganu
@ 2020-11-23 14:17   ` Sami Mujawar
  0 siblings, 0 replies; 28+ messages in thread
From: Sami Mujawar @ 2020-11-23 14:17 UTC (permalink / raw)
  To: Sughosh Ganu, devel@edk2.groups.io
  Cc: Ard Biesheuvel, Jiewen Yao, Ilias Apalodimas, nd

Hi Sughosh,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

-----Original Message-----
From: Sughosh Ganu <sughosh.ganu@linaro.org> 
Sent: 21 October 2020 12:33 PM
To: devel@edk2.groups.io
Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Sami Mujawar <Sami.Mujawar@arm.com>; Jiewen Yao <jiewen.yao@intel.com>; Ilias Apalodimas <ilias.apalodimas@linaro.org>; Sughosh Ganu <sughosh.ganu@linaro.org>
Subject: [PATCH v1 12/12] StandaloneMmPkg: Allow sending FFA Direct Request message to StandaloneMm

From: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Allow passing of a request to StandaloneMm Core through the Firmware
Framework(FF-A) using FFA_MSG_SEND_DIRECT_REQ method. This method is
used as a mechanism for requesting some service from StandaloneMm.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
index 6a25c4c548..199441f7d2 100644
--- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
+++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
@@ -22,6 +22,7 @@
 #include <Guid/ZeroGuid.h>
 #include <Guid/MmramMemoryReserve.h>
 
+#include <IndustryStandard/ArmFfaSvc.h>
 #include <IndustryStandard/ArmStdSmc.h>
 
 #include "StandaloneMmCpu.h"
@@ -78,7 +79,8 @@ PiMmStandaloneArmTfCpuDriverEntry (
   // receipt of a synchronous MM request. Use the Event ID to distinguish
   // between synchronous and asynchronous events.
   //
-  if (ARM_SMC_ID_MM_COMMUNICATE_AARCH64 != EventId) {
+  if (ARM_SMC_ID_MM_COMMUNICATE_AARCH64 != EventId &&
+    ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64 != EventId) {
[SAMI] Please use additional parenthesis for clarity. 
Ref: https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/52_spacing#5-2-2-10-use-extra-parentheses-rather-than-depending-on-in-depth-knowledge-of-the-order-of-precedence-of-c
[/SAMI]
     DEBUG ((DEBUG_INFO, "UnRecognized Event - 0x%x\n", EventId));
     return EFI_INVALID_PARAMETER;
   }
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [edk2-devel] [PATCH v1 10/12] ArmPkg/StandaloneMmMmuLib: Add option to use FF-A calls to set memory region's permissions
       [not found]   ` <164A26E48323EBBE.21019@groups.io>
@ 2020-11-23 14:21     ` Sami Mujawar
  0 siblings, 0 replies; 28+ messages in thread
From: Sami Mujawar @ 2020-11-23 14:21 UTC (permalink / raw)
  To: devel@edk2.groups.io, Sami Mujawar, Sughosh Ganu
  Cc: Ard Biesheuvel, Jiewen Yao, Achin Gupta, nd

One more suggestion added inline marked [SAMI].

Regards,

Sami Mujawar

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sami Mujawar via groups.io
Sent: 23 November 2020 01:43 PM
To: Sughosh Ganu <sughosh.ganu@linaro.org>; devel@edk2.groups.io
Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Jiewen Yao <jiewen.yao@intel.com>; Achin Gupta <Achin.Gupta@arm.com>; nd <nd@arm.com>
Subject: Re: [edk2-devel] [PATCH v1 10/12] ArmPkg/StandaloneMmMmuLib: Add option to use FF-A calls to set memory region's permissions

Hi Sughosh,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

-----Original Message-----
From: Sughosh Ganu <sughosh.ganu@linaro.org> 
Sent: 21 October 2020 12:33 PM
To: devel@edk2.groups.io
Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Sami Mujawar <Sami.Mujawar@arm.com>; Jiewen Yao <jiewen.yao@intel.com>; Achin Gupta <Achin.Gupta@arm.com>
Subject: [PATCH v1 10/12] ArmPkg/StandaloneMmMmuLib: Add option to use FF-A calls to set memory region's permissions

From: Achin Gupta <achin.gupta@arm.com>

Allow setting memory region's permissions using either of the Firmware
Framework(FF-A) ABI transport or through the earlier used SVC calls.

Signed-off-by: Achin Gupta <achin.gupta@arm.com>
Co-developed-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c | 24 ++++++++++++++++----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c b/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
index ab13602556..fef5eb4d22 100644
--- a/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
+++ b/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
@@ -63,17 +63,31 @@ RequestMemoryPermissionChange (
   IN  UINTN                     Permissions
   )
 {
+  BOOLEAN       FfaEnabled;
   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;
+  FfaEnabled = FeaturePcdGet (PcdFfaEnable);
+
+  if (FfaEnabled) {
+    ChangeMemoryPermissionsSvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64;
+    ChangeMemoryPermissionsSvcArgs.Arg1 = 0x3;
[SAMI] Can you define macros for the endpoint IDs, please? Also add reference to where information/documentation about the endpoint Ids can be found.
[/SAMI]

+    ChangeMemoryPermissionsSvcArgs.Arg2 = 0;
+    ChangeMemoryPermissionsSvcArgs.Arg3 = ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64;
+    ChangeMemoryPermissionsSvcArgs.Arg4 = BaseAddress;
+    ChangeMemoryPermissionsSvcArgs.Arg5 = EFI_SIZE_TO_PAGES(Length);

[SAMI] Add a space between EFI_SIZE_TO_PAGES and starting parenthesis.  [/SAMI]

+    ChangeMemoryPermissionsSvcArgs.Arg6 = Permissions;
+  } else {
+    ChangeMemoryPermissionsSvcArgs.Arg0 = ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64;
+    ChangeMemoryPermissionsSvcArgs.Arg1 = BaseAddress;
+    ChangeMemoryPermissionsSvcArgs.Arg2 = EFI_SIZE_TO_PAGES(Length);

[SAMI] Add a space between EFI_SIZE_TO_PAGES and starting parenthesis.  [/SAMI]

+    ChangeMemoryPermissionsSvcArgs.Arg3 = Permissions;
+  }
 
   ArmCallSvc (&ChangeMemoryPermissionsSvcArgs);
 
-  Status = ChangeMemoryPermissionsSvcArgs.Arg0;
+  Status = FfaEnabled ?
+    ChangeMemoryPermissionsSvcArgs.Arg3 : ChangeMemoryPermissionsSvcArgs.Arg0;
[SAMI] In case of FFA, Arg0 may also contain an error code, right? See FFA spec, section 10.2.
[/SAMI]
 
   switch (Status) {
   case ARM_SVC_SPM_RET_SUCCESS:
-- 
2.17.1







^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH v1 00/12] Add support for using FF-A calls
  2020-10-21 11:32 [PATCH v1 00/12] Add support for using FF-A calls sughosh.ganu
                   ` (12 preceding siblings ...)
  2020-11-05 10:29 ` [PATCH v1 00/12] Add support for using FF-A calls Sami Mujawar
@ 2020-11-23 14:33 ` Sami Mujawar
  2020-11-24  7:48   ` Sughosh Ganu
  13 siblings, 1 reply; 28+ messages in thread
From: Sami Mujawar @ 2020-11-23 14:33 UTC (permalink / raw)
  To: Sughosh Ganu, devel@edk2.groups.io; +Cc: Ard Biesheuvel, Jiewen Yao, nd

Hi Sughosh,

I have completed reviewing this patch series.

Please add some description in the commit message for the following patches. Otherwise patch 3 & 8 look good to me.
[PATCH v1 08/12] StandaloneMmPkg: Use FF-A header file in Standalone MM Arm MMU library
[PATCH v1 03/12] StandaloneMmPkg: Use FF-A header file in Standalone MM Core entry point

Regards,

Sami Mujawar

-----Original Message-----
From: Sughosh Ganu <sughosh.ganu@linaro.org> 
Sent: 21 October 2020 12:32 PM
To: devel@edk2.groups.io
Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Sami Mujawar <Sami.Mujawar@arm.com>; Jiewen Yao <jiewen.yao@intel.com>
Subject: [PATCH v1 00/12] Add support for using FF-A calls

Achin Gupta (8):
  ArmPkg/IndustryStandard: Add barebones FF-A header
  ArmPkg/ArmSvcLib: Return x4-x7 in output parameters
  StandaloneMmPkg: Use FF-A header file in Standalone MM Core entry
    point
  StandaloneMmPkg: Add option to use FF-A calls for getting SPM version
  StandaloneMmPkg: Add option to use FF-A calls for communication with
    SPM
  StandaloneMmPkg: Use FF-A header file in Standalone MM Arm MMU library
  ArmPkg/StandaloneMmMmuLib: Add option to use FF-A calls to get memory
    region's permissions
  ArmPkg/StandaloneMmMmuLib: Add option to use FF-A calls to set memory
    region's permissions

Ilias Apalodimas (2):
  MdeModulePkg/VariableStandaloneMm: Set PcdFlashNvStorageVariableBase
    to Pcd
  StandaloneMmPkg: Allow sending FFA Direct Request message to
    StandaloneMm

Sughosh Ganu (1):
  ArmPkg: Introduce support for PcdFfaEnable

Sughossh Ganu (1):
  StandaloneMmPkg: Add the SPM version for FF-A

 ArmPkg/ArmPkg.dec                             |   3 +
 .../ArmMmuStandaloneMmLib.inf                 |   3 +
 .../RuntimeDxe/VariableStandaloneMm.inf       |   6 +-
 .../StandaloneMmCoreEntryPoint.inf            |   3 +
 ArmPkg/Include/IndustryStandard/ArmFfaSvc.h   |  16 +++
 .../AArch64/ArmMmuStandaloneMmLib.c           |  53 +++++++--
 .../StandaloneMmCpu/AArch64/EventHandle.c     |   4 +-
 .../AArch64/StandaloneMmCoreEntryPoint.c      | 103 ++++++++++++++----
 ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S     |   2 +
 9 files changed, 155 insertions(+), 38 deletions(-)
 create mode 100644 ArmPkg/Include/IndustryStandard/ArmFfaSvc.h

-- 
2.17.1



^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v1 00/12] Add support for using FF-A calls
  2020-11-23 14:33 ` Sami Mujawar
@ 2020-11-24  7:48   ` Sughosh Ganu
  0 siblings, 0 replies; 28+ messages in thread
From: Sughosh Ganu @ 2020-11-24  7:48 UTC (permalink / raw)
  To: Sami Mujawar; +Cc: devel@edk2.groups.io, Ard Biesheuvel, Jiewen Yao, nd

[-- Attachment #1: Type: text/plain, Size: 2608 bytes --]

hi Sami,

On Mon, 23 Nov 2020 at 20:03, Sami Mujawar <Sami.Mujawar@arm.com> wrote:

> Hi Sughosh,
>
> I have completed reviewing this patch series.
>
> Please add some description in the commit message for the following
> patches. Otherwise patch 3 & 8 look good to me.
> [PATCH v1 08/12] StandaloneMmPkg: Use FF-A header file in Standalone MM
> Arm MMU library
> [PATCH v1 03/12] StandaloneMmPkg: Use FF-A header file in Standalone MM
> Core entry point
>

Thanks a lot for reviewing the patches. I will incorporate your review
comments on all the patches, and send a V2.

-sughosh


>
> Regards,
>
> Sami Mujawar
>
> -----Original Message-----
> From: Sughosh Ganu <sughosh.ganu@linaro.org>
> Sent: 21 October 2020 12:32 PM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Sami Mujawar <
> Sami.Mujawar@arm.com>; Jiewen Yao <jiewen.yao@intel.com>
> Subject: [PATCH v1 00/12] Add support for using FF-A calls
>
> Achin Gupta (8):
>   ArmPkg/IndustryStandard: Add barebones FF-A header
>   ArmPkg/ArmSvcLib: Return x4-x7 in output parameters
>   StandaloneMmPkg: Use FF-A header file in Standalone MM Core entry
>     point
>   StandaloneMmPkg: Add option to use FF-A calls for getting SPM version
>   StandaloneMmPkg: Add option to use FF-A calls for communication with
>     SPM
>   StandaloneMmPkg: Use FF-A header file in Standalone MM Arm MMU library
>   ArmPkg/StandaloneMmMmuLib: Add option to use FF-A calls to get memory
>     region's permissions
>   ArmPkg/StandaloneMmMmuLib: Add option to use FF-A calls to set memory
>     region's permissions
>
> Ilias Apalodimas (2):
>   MdeModulePkg/VariableStandaloneMm: Set PcdFlashNvStorageVariableBase
>     to Pcd
>   StandaloneMmPkg: Allow sending FFA Direct Request message to
>     StandaloneMm
>
> Sughosh Ganu (1):
>   ArmPkg: Introduce support for PcdFfaEnable
>
> Sughossh Ganu (1):
>   StandaloneMmPkg: Add the SPM version for FF-A
>
>  ArmPkg/ArmPkg.dec                             |   3 +
>  .../ArmMmuStandaloneMmLib.inf                 |   3 +
>  .../RuntimeDxe/VariableStandaloneMm.inf       |   6 +-
>  .../StandaloneMmCoreEntryPoint.inf            |   3 +
>  ArmPkg/Include/IndustryStandard/ArmFfaSvc.h   |  16 +++
>  .../AArch64/ArmMmuStandaloneMmLib.c           |  53 +++++++--
>  .../StandaloneMmCpu/AArch64/EventHandle.c     |   4 +-
>  .../AArch64/StandaloneMmCoreEntryPoint.c      | 103 ++++++++++++++----
>  ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S     |   2 +
>  9 files changed, 155 insertions(+), 38 deletions(-)
>  create mode 100644 ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
>
> --
> 2.17.1
>
>
>

[-- Attachment #2: Type: text/html, Size: 3708 bytes --]

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2020-11-24  7:49 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-10-21 11:32 [PATCH v1 00/12] Add support for using FF-A calls sughosh.ganu
2020-10-21 11:32 ` [PATCH v1 01/12] ArmPkg/IndustryStandard: Add barebones FF-A header Sughosh Ganu
2020-11-05 10:33   ` Sami Mujawar
2020-11-06  9:06     ` Sughosh Ganu
2020-10-21 11:32 ` [PATCH v1 02/12] ArmPkg/ArmSvcLib: Return x4-x7 in output parameters Sughosh Ganu
2020-11-10 12:02   ` Sami Mujawar
2020-10-21 11:32 ` [PATCH v1 03/12] StandaloneMmPkg: Use FF-A header file in Standalone MM Core entry point Sughosh Ganu
2020-10-21 11:32 ` [PATCH v1 04/12] ArmPkg: Introduce support for PcdFfaEnable Sughosh Ganu
2020-11-19  9:56   ` Sami Mujawar
2020-10-21 11:32 ` [PATCH v1 05/12] StandaloneMmPkg: Add the SPM version for FF-A Sughosh Ganu
2020-11-19  9:54   ` Sami Mujawar
2020-10-21 11:32 ` [PATCH v1 06/12] StandaloneMmPkg: Add option to use FF-A calls for getting SPM version Sughosh Ganu
2020-11-23 10:10   ` Sami Mujawar
2020-10-21 11:32 ` [PATCH v1 07/12] StandaloneMmPkg: Add option to use FF-A calls for communication with SPM Sughosh Ganu
2020-11-23 12:38   ` Sami Mujawar
2020-10-21 11:32 ` [PATCH v1 08/12] StandaloneMmPkg: Use FF-A header file in Standalone MM Arm MMU library Sughosh Ganu
2020-10-21 11:32 ` [PATCH v1 09/12] ArmPkg/StandaloneMmMmuLib: Add option to use FF-A calls to get memory region's permissions Sughosh Ganu
2020-11-23 13:26   ` Sami Mujawar
2020-10-21 11:32 ` [PATCH v1 10/12] ArmPkg/StandaloneMmMmuLib: Add option to use FF-A calls to set " Sughosh Ganu
2020-11-23 13:42   ` Sami Mujawar
     [not found]   ` <164A26E48323EBBE.21019@groups.io>
2020-11-23 14:21     ` [edk2-devel] " Sami Mujawar
2020-10-21 11:32 ` [PATCH v1 11/12] MdeModulePkg/VariableStandaloneMm: Set PcdFlashNvStorageVariableBase to Pcd Sughosh Ganu
2020-11-23 13:52   ` Sami Mujawar
2020-10-21 11:32 ` [PATCH v1 12/12] StandaloneMmPkg: Allow sending FFA Direct Request message to StandaloneMm Sughosh Ganu
2020-11-23 14:17   ` Sami Mujawar
2020-11-05 10:29 ` [PATCH v1 00/12] Add support for using FF-A calls Sami Mujawar
2020-11-23 14:33 ` Sami Mujawar
2020-11-24  7:48   ` Sughosh Ganu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox