Re: [PATCH v1 10/12] ArmPkg/StandaloneMmMmuLib: Add option to use FF-A calls to set 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 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