Re: [edk2-devel] [edk2-platforms][PATCH V1 18/20] ArmPkg/MmCommunicationDxe: Discover the StMM SP

["Girish Mahadevan" <gmahadevan@nvidia.com>]

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.gupta@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.gupta@arm.com>
> Signed-off-by: Nishant Sharma <nishant.sharma@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+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [gmahadevan@nvidia.com]
> -=-=-=-=-=-=
> 
>