A few comments in line. (Prefixed with [GM])

Best Regards
Girish

On 7/11/2023 8:36 AM, Nishant Sharma via groups.io wrote:
External email: Use caution opening links or attachments


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

This patch adds support for discovering the presence of the SP using the
EFI_MM_COMMUNICATION_PROTOCOL GUID that implements Standalone MM
drivers. This is done by querying the framework through
FFA_PARTITION_INFO_GET whether any partition that implements the
EFI_MM_COMMUNICATION_PROTOCOL is present or not. The partition ID and
its properties are stashed for use in subsequent communication with the
StMM SP.

Signed-off-by: Achin Gupta <achin.gu...@arm.com>
Signed-off-by: Nishant Sharma <nishant.sha...@arm.com>
---
  ArmPkg/Include/IndustryStandard/ArmFfaSvc.h         | 24 +++++
  ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c | 93 +++++++++++++++++++-
  2 files changed, 114 insertions(+), 3 deletions(-)

diff --git a/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h 
b/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
index f78442a465e1..530af8bd3c2e 100644
--- a/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
+++ b/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
@@ -19,6 +19,9 @@
  #define ARM_SVC_ID_FFA_VERSION_AARCH32               0x84000063
  #define ARM_SVC_ID_FFA_RXTX_MAP_AARCH32              0x84000066
  #define ARM_SVC_ID_FFA_RXTX_MAP_AARCH64              0xC4000066
+#define ARM_SVC_ID_FFA_RX_RELEASE_AARCH32            0x84000065
+#define ARM_SVC_ID_FFA_RXTX_UNMAP_AARCH32            0x84000067
+#define ARM_SVC_ID_FFA_PARTITION_INFO_GET_AARCH32    0x84000068
  #define ARM_SVC_ID_FFA_ID_GET_AARCH32                0x84000069
  #define ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH32   0x8400006F
  #define ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH32  0x84000070
@@ -154,4 +157,25 @@ typedef struct {
    UINT64 Reserved;
  } EFI_FFA_BOOT_INFO_HEADER;

+// FF-A partition information descriptor
+typedef struct {
+  UINT16 PartId;
+  UINT16 EcCnt;
+  UINT32 PartProps;
+  UINT32 PartGuid[4];
+} EFI_FFA_PART_INFO_DESC;
+
+#define PART_INFO_PROP_MASK                    0x3f
+#define PART_INFO_PROP_SHIFT                   0
+#define PART_INFO_PROP_DIR_MSG_RECV_BIT        (1u << 0)
+#define PART_INFO_PROP_DIR_MSG_SEND_BIT        (1u << 1)
+#define PART_INFO_PROP_INDIR_MSG_BIT           (1u << 2)
+#define PART_INFO_PROP_NOTIFICATIONS_BIT       (1u << 3)
+#define PART_INFO_PROP_EP_TYPE_MASK            0x3
+#define PART_INFO_PROP_EP_TYPE_SHIFT           4
+#define PART_INFO_PROP_EP_PE                   0
+#define PART_INFO_PROP_EP_SEPID_IND            1
+#define PART_INFO_PROP_EP_SEPID_DEP            2
+#define PART_INFO_PROP_EP_AUX                  3
+
  #endif // ARM_FFA_SVC_H_
diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c 
b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
index 39a1b329b9ea..94a5d96c051d 100644
--- a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
+++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
@@ -8,6 +8,7 @@

  #include <Library/ArmLib.h>
  #include <Library/ArmSmcLib.h>
+#include <Library/BaseLib.h>
  #include <Library/BaseMemoryLib.h>
  #include <Library/DebugLib.h>
  #include <Library/DxeServicesTableLib.h>
@@ -28,6 +29,11 @@
  //
  STATIC UINT16  mFfaPartId;

+// Partition information of the StMM SP if FF-A support is enabled
+// TODO: Revisit assumption that there is only a single StMM SP
+//
+STATIC EFI_FFA_PART_INFO_DESC mStmmPartInfo;
+
  //
  // RX/TX pair if FF-A support is enabled
  //
@@ -298,7 +304,9 @@ GetMmCompatibility (
  {
    EFI_STATUS    Status;
    UINT32        MmVersion;
+  UINT32        SmccUuid[4];
    ARM_SMC_ARGS  SmcArgs = {0};
+  EFI_GUID      MmCommProtGuid = EFI_MM_COMMUNICATION_PROTOCOL_GUID;

    if (FixedPcdGet32 (PcdFfaEnable) != 0) {
      SmcArgs.Arg0 = ARM_SVC_ID_FFA_VERSION_AARCH32;
@@ -335,14 +343,21 @@ GetMmCompatibility (
      Status = EFI_UNSUPPORTED;
    }

-  // If FF-A is supported then discover our ID and register our RX/TX buffers.
+  // If FF-A is supported then discover the StMM SP's presence, ID, our ID and
+  // register our RX/TX buffers.
    if (FixedPcdGet32 (PcdFfaEnable) != 0) {
+    EFI_FFA_PART_INFO_DESC *StmmPartInfo;
+
[GM]
Probably better off putting this entire thing (discovering the VMId in a different function).

      // Get our ID
      ZeroMem(&SmcArgs, sizeof(SmcArgs));
      SmcArgs.Arg0 = ARM_SVC_ID_FFA_ID_GET_AARCH32;
      ArmCallSmc (&SmcArgs);
      if (SmcArgs.Arg0 == ARM_SVC_ID_FFA_ERROR_AARCH32) {
-      DEBUG ((DEBUG_ERROR, "Unable to retrieve FF-A partition ID (%d).\n", 
SmcArgs.Arg2));
+      DEBUG ((
+        DEBUG_ERROR,
+        "Unable to retrieve FF-A partition ID (%d).\n",
+        SmcArgs.Arg2
+        ));
        return EFI_UNSUPPORTED;
      }
      DEBUG ((DEBUG_INFO, "FF-A partition ID = 0x%lx.\n", SmcArgs.Arg2));
@@ -355,11 +370,83 @@ GetMmCompatibility (
      SmcArgs.Arg3 = EFI_PAGE_SIZE / SIZE_4KB;
      ArmCallSmc (&SmcArgs);
      if (SmcArgs.Arg0 == ARM_SVC_ID_FFA_ERROR_AARCH32) {
-      DEBUG ((DEBUG_ERROR, "Unable to register FF-A RX/TX buffers (%d).\n", 
SmcArgs.Arg2));
+      DEBUG ((
+        DEBUG_ERROR,
+        "Unable to register FF-A RX/TX buffers (%d).\n",
+        SmcArgs.Arg2
+        ));
        return EFI_UNSUPPORTED;
      } >
+    // Discover the StMM SP after converting the EFI_GUID to a format TF-A will
+    // understand.
+    SmcArgs.Arg0 = ARM_SVC_ID_FFA_PARTITION_INFO_GET_AARCH32;
+    MmCommProtGuid.Data2 += MmCommProtGuid.Data3;
+    MmCommProtGuid.Data3 = MmCommProtGuid.Data2 - MmCommProtGuid.Data3;
+    MmCommProtGuid.Data2 = MmCommProtGuid.Data2 - MmCommProtGuid.Data3;
+    CopyMem ((VOID *) SmccUuid, (VOID *) &MmCommProtGuid, sizeof(EFI_GUID));
+    SmcArgs.Arg1 = SmccUuid[0];
+    SmcArgs.Arg2 = SmccUuid[1];
+    SmcArgs.Arg3 = SmccUuid[2];
+    SmcArgs.Arg3 = SwapBytes32(SmcArgs.Arg3);
+    SmcArgs.Arg4 = SmccUuid[3];
+    SmcArgs.Arg4 = SwapBytes32(SmcArgs.Arg4);
+    ArmCallSmc (&SmcArgs);
+    if (SmcArgs.Arg0 == ARM_SVC_ID_FFA_ERROR_AARCH32) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "Unable to discover FF-A StMM SP (%d).\n",
+        SmcArgs.Arg2
+        ));
+      goto ffa_init_error;
+    }
+

[GM]
Shouldn't you also check Arg2 to ensure you have discovered at least 1 SP.

+    // Retrieve the partition information from the RX buffer
+    StmmPartInfo = (EFI_FFA_PART_INFO_DESC *) FfaRxBuf;
+
+    // TODO: Sanity check the partition type.
+    DEBUG ((DEBUG_INFO, "Discovered FF-A StMM SP."));
+    DEBUG ((
+      DEBUG_INFO,
+      "ID = 0x%lx, Execution contexts = %d, Properties = 0x%lx. \n",
+      StmmPartInfo->PartId,
+      StmmPartInfo->EcCnt,
+      StmmPartInfo->PartProps
+      ));
+
+    // Make a local copy
+    mStmmPartInfo = *StmmPartInfo;
+
+    // Release the RX buffer
+    ZeroMem(&SmcArgs, sizeof(SmcArgs));
+    SmcArgs.Arg0 = ARM_SVC_ID_FFA_RX_RELEASE_AARCH32;
+    SmcArgs.Arg1 = mFfaPartId;
+    ArmCallSmc (&SmcArgs);
+
+    // This should really never fail since there is only a single CPU booting
+    // and another CPU could not have released the RX buffer before us.
+    if (SmcArgs.Arg0 == ARM_SVC_ID_FFA_ERROR_AARCH32) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "Unable to release FF-A RX buffer (%d).\n",
+        SmcArgs.Arg2
+        ));
+      ASSERT (0);
+      goto ffa_init_error;
+    }
+
      return EFI_SUCCESS;
+
+  ffa_init_error:
+    // Release the RX/TX pair before exiting.
+    ZeroMem(&SmcArgs, sizeof(SmcArgs));
+    SmcArgs.Arg0 = ARM_SVC_ID_FFA_RXTX_UNMAP_AARCH32;
+    SmcArgs.Arg1 = mFfaPartId << 16;  // TODO: Use a macro for shift
+    ArmCallSmc (&SmcArgs);
+
+    // We do not bother checking the error code of the RXTX_UNMAP invocation
+    // since we did map the buffers and this call must succeed.
+    return EFI_UNSUPPORTED;
    }

    return Status;
--
2.34.1



-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106810): https://edk2.groups.io/g/devel/message/106810
Mute This Topic: https://groups.io/mt/100079892/6098446
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [gmahade...@nvidia.com]
-=-=-=-=-=-=




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106887): https://edk2.groups.io/g/devel/message/106887
Mute This Topic: https://groups.io/mt/100079892/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-


Reply via email to