Re: [PATCH v1 09/12] ArmPkg/StandaloneMmMmuLib: Add option to use FF-A calls to get memory region's permissions

["Sami Mujawar" <sami.mujawar@arm.com>]

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