[PATCH v3 1/1] Add ArmPkg/Optee library APIs

[PATCH v3 1/1] Add ArmPkg/Optee library APIs

[Sumit Garg]

Changes in v3:
Removed GlobalPlatform TEE return codes (IndustryStandard/GlobalPlatform.h)
that were rejected by EDK2 maintainers. Rather used custom ones for this
OP-TEE driver.

Changes in v2:
1. Separate patch for MdePkg/Include/IndustryStandard/GlobalPlatform.h.
2. Correct comments style for struct members.
3. Update commit message.

Sumit Garg (1):
  ArmPkg/OpteeLib: Add APIs to communicate with OP-TEE

 ArmPkg/Include/Library/OpteeLib.h    |  90 +++++++++
 ArmPkg/Library/OpteeLib/Optee.c      | 357 +++++++++++++++++++++++++++++++++++
 ArmPkg/Library/OpteeLib/OpteeLib.inf |   2 +
 ArmPkg/Library/OpteeLib/OpteeSmc.h   |  43 +++++
 4 files changed, 492 insertions(+)
 create mode 100644 ArmPkg/Library/OpteeLib/OpteeSmc.h

-- 
2.7.4

[PATCH v3 1/1] ArmPkg/OpteeLib: Add APIs to communicate with OP-TEE

[Sumit Garg]

Add following APIs to communicate with OP-TEE pseudo/early TAs:
1. OpteeInit
2. OpteeOpenSession
3. OpteeCloseSession
4. OpteeInvokeFunc

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 ArmPkg/Include/Library/OpteeLib.h    |  90 +++++++++
 ArmPkg/Library/OpteeLib/Optee.c      | 357 +++++++++++++++++++++++++++++++++++
 ArmPkg/Library/OpteeLib/OpteeLib.inf |   2 +
 ArmPkg/Library/OpteeLib/OpteeSmc.h   |  43 +++++
 4 files changed, 492 insertions(+)
 create mode 100644 ArmPkg/Library/OpteeLib/OpteeSmc.h

diff --git a/ArmPkg/Include/Library/OpteeLib.h b/ArmPkg/Include/Library/OpteeLib.h
index f65d8674d9b8..2d1c60632dfe 100644
--- a/ArmPkg/Include/Library/OpteeLib.h
+++ b/ArmPkg/Include/Library/OpteeLib.h
@@ -25,10 +25,100 @@
 #define OPTEE_OS_UID2          0xaf630002
 #define OPTEE_OS_UID3          0xa5d5c51b
 
+#define OPTEE_MSG_ATTR_TYPE_NONE                0x0
+#define OPTEE_MSG_ATTR_TYPE_VALUE_INPUT         0x1
+#define OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT        0x2
+#define OPTEE_MSG_ATTR_TYPE_VALUE_INOUT         0x3
+#define OPTEE_MSG_ATTR_TYPE_MEM_INPUT           0x9
+#define OPTEE_MSG_ATTR_TYPE_MEM_OUTPUT          0xa
+#define OPTEE_MSG_ATTR_TYPE_MEM_INOUT           0xb
+
+#define OPTEE_MSG_ATTR_TYPE_MASK                0xff
+
+#define OPTEE_ORIGIN_COMMS                      0x00000002
+#define OPTEE_ERROR_COMMS                       0xFFFF000E
+
+typedef struct {
+  UINT64    BufPtr;
+  UINT64    Size;
+  UINT64    ShmRef;
+} OPTEE_MSG_PARAM_MEM;
+
+typedef struct {
+  UINT64    A;
+  UINT64    B;
+  UINT64    C;
+} OPTEE_MSG_PARAM_VALUE;
+
+typedef struct {
+  UINT64 Attr;
+  union {
+    OPTEE_MSG_PARAM_MEM      Mem;
+    OPTEE_MSG_PARAM_VALUE    Value;
+  } U;
+} OPTEE_MSG_PARAM;
+
+#define MAX_PARAMS           4
+
+typedef struct {
+        UINT32             Cmd;
+        UINT32             Func;
+        UINT32             Session;
+        UINT32             CancelId;
+        UINT32             Pad;
+        UINT32             Ret;
+        UINT32             RetOrigin;
+        UINT32             NumParams;
+
+        // NumParams tells the actual number of element in Params
+        OPTEE_MSG_PARAM    Params[MAX_PARAMS];
+} OPTEE_MSG_ARG;
+
+#define OPTEE_UUID_LEN       16
+
+typedef struct {
+        UINT8     Uuid[OPTEE_UUID_LEN]; // [in] UUID of the Trusted Application
+        UINT32    Session;              // [out] Session id
+        UINT32    Ret;                  // [out] Return value
+        UINT32    RetOrigin;            // [out] Origin of the return value
+} OPTEE_OPEN_SESSION_ARG;
+
+typedef struct {
+        UINT32             Func;    // [in] Trusted App func, specific to the TA
+        UINT32             Session; // [in] Session id
+        UINT32             Ret;     // [out] Return value
+        UINT32             RetOrigin; // [out] Origin of the return value
+        OPTEE_MSG_PARAM    Params[MAX_PARAMS]; // Params for func to be invoked
+} OPTEE_INVOKE_FUNC_ARG;
+
 BOOLEAN
 EFIAPI
 IsOpteePresent (
   VOID
   );
 
+EFI_STATUS
+EFIAPI
+OpteeInit (
+  VOID
+  );
+
+EFI_STATUS
+EFIAPI
+OpteeOpenSession (
+  IN OUT OPTEE_OPEN_SESSION_ARG      *OpenSessionArg
+  );
+
+EFI_STATUS
+EFIAPI
+OpteeCloseSession (
+  IN UINT32                      Session
+  );
+
+EFI_STATUS
+EFIAPI
+OpteeInvokeFunc (
+  IN OUT OPTEE_INVOKE_FUNC_ARG       *InvokeFuncArg
+  );
+
 #endif
diff --git a/ArmPkg/Library/OpteeLib/Optee.c b/ArmPkg/Library/OpteeLib/Optee.c
index 574527f8b5ea..bf7872cbbce0 100644
--- a/ArmPkg/Library/OpteeLib/Optee.c
+++ b/ArmPkg/Library/OpteeLib/Optee.c
@@ -14,11 +14,18 @@
 
 **/
 
+#include <Library/ArmMmuLib.h>
 #include <Library/ArmSmcLib.h>
+#include <Library/BaseMemoryLib.h>
 #include <Library/BaseLib.h>
+#include <Library/DebugLib.h>
 #include <Library/OpteeLib.h>
 
 #include <IndustryStandard/ArmStdSmc.h>
+#include <OpteeSmc.h>
+#include <Uefi.h>
+
+STATIC OPTEE_SHARED_MEMORY_INFO OpteeShmInfo = { 0 };
 
 /**
   Check for OP-TEE presence.
@@ -31,6 +38,7 @@ IsOpteePresent (
 {
   ARM_SMC_ARGS ArmSmcArgs;
 
+  ZeroMem (&ArmSmcArgs, sizeof (ARM_SMC_ARGS));
   // Send a Trusted OS Calls UID command
   ArmSmcArgs.Arg0 = ARM_SMC_ID_TOS_UID;
   ArmCallSmc (&ArmSmcArgs);
@@ -44,3 +52,352 @@ IsOpteePresent (
     return FALSE;
   }
 }
+
+STATIC
+EFI_STATUS
+OpteeShmMemRemap (
+  VOID
+  )
+{
+  ARM_SMC_ARGS                 ArmSmcArgs;
+  EFI_PHYSICAL_ADDRESS         Paddr;
+  EFI_PHYSICAL_ADDRESS         Start;
+  EFI_PHYSICAL_ADDRESS         End;
+  EFI_STATUS                   Status;
+  UINTN                        Size;
+
+  ZeroMem (&ArmSmcArgs, sizeof (ARM_SMC_ARGS));
+  ArmSmcArgs.Arg0 = OPTEE_SMC_GET_SHM_CONFIG;
+
+  ArmCallSmc (&ArmSmcArgs);
+  if (ArmSmcArgs.Arg0 != OPTEE_SMC_RETURN_OK) {
+    DEBUG ((DEBUG_WARN, "OP-TEE shared memory not supported\n"));
+    return EFI_UNSUPPORTED;
+  }
+
+  if (ArmSmcArgs.Arg3 != OPTEE_SMC_SHM_CACHED) {
+    DEBUG ((DEBUG_WARN, "OP-TEE: Only normal cached shared memory supported\n"));
+    return EFI_UNSUPPORTED;
+  }
+
+  Start = (ArmSmcArgs.Arg1 + SIZE_4KB - 1) & ~(SIZE_4KB - 1);
+  End = (ArmSmcArgs.Arg1 + ArmSmcArgs.Arg2) & ~(SIZE_4KB - 1);
+  Paddr = Start;
+  Size = End - Start;
+
+  if (Size < SIZE_4KB) {
+    DEBUG ((DEBUG_WARN, "OP-TEE shared memory too small\n"));
+    return EFI_BUFFER_TOO_SMALL;
+  }
+
+  Status = ArmSetMemoryAttributes (Paddr, Size, EFI_MEMORY_WB);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  OpteeShmInfo.Base = (UINTN)Paddr;
+  OpteeShmInfo.Size = Size;
+
+  return EFI_SUCCESS;
+}
+
+EFI_STATUS
+EFIAPI
+OpteeInit (
+  VOID
+  )
+{
+  EFI_STATUS      Status;
+
+  if (!IsOpteePresent ()) {
+    DEBUG ((DEBUG_WARN, "OP-TEE not present\n"));
+    return EFI_UNSUPPORTED;
+  }
+
+  Status = OpteeShmMemRemap ();
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_WARN, "OP-TEE shared memory remap failed\n"));
+    return Status;
+  }
+
+  return EFI_SUCCESS;
+}
+
+/**
+  Does Standard SMC to OP-TEE in secure world.
+
+  @param[in]  Parg   Physical address of message to pass to secure world
+
+  @return            0 on success, secure world return code otherwise
+
+**/
+STATIC
+UINT32
+OpteeCallWithArg (
+  IN EFI_PHYSICAL_ADDRESS Parg
+  )
+{
+  ARM_SMC_ARGS ArmSmcArgs;
+
+  ZeroMem (&ArmSmcArgs, sizeof (ARM_SMC_ARGS));
+  ArmSmcArgs.Arg0 = OPTEE_SMC_CALL_WITH_ARG;
+  ArmSmcArgs.Arg1 = (UINT32)(Parg >> 32);
+  ArmSmcArgs.Arg2 = (UINT32)Parg;
+
+  while (TRUE) {
+    ArmCallSmc (&ArmSmcArgs);
+
+    if (ArmSmcArgs.Arg0 == OPTEE_SMC_RETURN_RPC_FOREIGN_INTR) {
+      //
+      // A foreign interrupt was raised while secure world was
+      // executing, since they are handled in UEFI a dummy RPC is
+      // performed to let UEFI take the interrupt through the normal
+      // vector.
+      //
+      ArmSmcArgs.Arg0 = OPTEE_SMC_RETURN_FROM_RPC;
+    } else {
+      break;
+    }
+  }
+
+  return ArmSmcArgs.Arg0;
+}
+
+EFI_STATUS
+EFIAPI
+OpteeOpenSession (
+  IN OUT OPTEE_OPEN_SESSION_ARG      *OpenSessionArg
+  )
+{
+  OPTEE_MSG_ARG    *MsgArg;
+
+  MsgArg = NULL;
+
+  if (OpteeShmInfo.Base == 0) {
+    DEBUG ((DEBUG_WARN, "OP-TEE not initialized\n"));
+    return EFI_NOT_STARTED;
+  }
+
+  MsgArg = (OPTEE_MSG_ARG *)OpteeShmInfo.Base;
+  ZeroMem (MsgArg, sizeof (OPTEE_MSG_ARG));
+
+  MsgArg->Cmd = OPTEE_MSG_CMD_OPEN_SESSION;
+
+  //
+  // Initialize and add the meta parameters needed when opening a
+  // session.
+  //
+  MsgArg->Params[0].Attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT |
+                           OPTEE_MSG_ATTR_META;
+  MsgArg->Params[1].Attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT |
+                           OPTEE_MSG_ATTR_META;
+  CopyMem (&MsgArg->Params[0].U.Value, OpenSessionArg->Uuid, OPTEE_UUID_LEN);
+  ZeroMem (&MsgArg->Params[1].U.Value, OPTEE_UUID_LEN);
+  MsgArg->Params[1].U.Value.C = TEE_LOGIN_PUBLIC;
+
+  MsgArg->NumParams = 2;
+
+  if (OpteeCallWithArg ((EFI_PHYSICAL_ADDRESS)MsgArg)) {
+    MsgArg->Ret = OPTEE_ERROR_COMMS;
+    MsgArg->RetOrigin = OPTEE_ORIGIN_COMMS;
+  }
+
+  OpenSessionArg->Session = MsgArg->Session;
+  OpenSessionArg->Ret = MsgArg->Ret;
+  OpenSessionArg->RetOrigin = MsgArg->RetOrigin;
+
+  return EFI_SUCCESS;
+}
+
+EFI_STATUS
+EFIAPI
+OpteeCloseSession (
+  IN UINT32                      Session
+  )
+{
+  OPTEE_MSG_ARG    *MsgArg;
+
+  MsgArg = NULL;
+
+  if (OpteeShmInfo.Base == 0) {
+    DEBUG ((DEBUG_WARN, "OP-TEE not initialized\n"));
+    return EFI_NOT_STARTED;
+  }
+
+  MsgArg = (OPTEE_MSG_ARG *)OpteeShmInfo.Base;
+  ZeroMem (MsgArg, sizeof (OPTEE_MSG_ARG));
+
+  MsgArg->Cmd = OPTEE_MSG_CMD_CLOSE_SESSION;
+  MsgArg->Session = Session;
+
+  OpteeCallWithArg ((EFI_PHYSICAL_ADDRESS)MsgArg);
+
+  return EFI_SUCCESS;
+}
+
+STATIC
+EFI_STATUS
+OpteeToMsgParam (
+  OUT OPTEE_MSG_PARAM    *MsgParams,
+  IN UINT32              NumParams,
+  IN OPTEE_MSG_PARAM     *InParams
+  )
+{
+  UINT32                  Idx;
+  UINTN                   ParamShmAddr;
+  UINTN                   ShmSize;
+  UINTN                   Size;
+
+  Size = (sizeof (OPTEE_MSG_ARG) + sizeof (UINT64) - 1) & ~(sizeof (UINT64) - 1);
+  ParamShmAddr = OpteeShmInfo.Base + Size;
+  ShmSize = OpteeShmInfo.Size - Size;
+
+  for (Idx = 0; Idx < NumParams; Idx++) {
+    CONST OPTEE_MSG_PARAM    *Ip;
+    OPTEE_MSG_PARAM          *Mp;
+    UINT32                   Attr;
+
+    Ip = InParams + Idx;
+    Mp = MsgParams + Idx;
+    Attr = Ip->Attr & OPTEE_MSG_ATTR_TYPE_MASK;
+
+    switch (Attr) {
+    case OPTEE_MSG_ATTR_TYPE_NONE:
+      Mp->Attr = OPTEE_MSG_ATTR_TYPE_NONE;
+      ZeroMem (&Mp->U, sizeof (Mp->U));
+      break;
+
+    case OPTEE_MSG_ATTR_TYPE_VALUE_INPUT:
+    case OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT:
+    case OPTEE_MSG_ATTR_TYPE_VALUE_INOUT:
+      Mp->Attr = Attr;
+      Mp->U.Value.A = Ip->U.Value.A;
+      Mp->U.Value.B = Ip->U.Value.B;
+      Mp->U.Value.C = Ip->U.Value.C;
+      break;
+
+    case OPTEE_MSG_ATTR_TYPE_MEM_INPUT:
+    case OPTEE_MSG_ATTR_TYPE_MEM_OUTPUT:
+    case OPTEE_MSG_ATTR_TYPE_MEM_INOUT:
+      Mp->Attr = Attr;
+
+      if (Ip->U.Mem.Size > ShmSize) {
+        return EFI_OUT_OF_RESOURCES;
+      }
+
+      CopyMem ((VOID *)ParamShmAddr, (VOID *)Ip->U.Mem.BufPtr, Ip->U.Mem.Size);
+      Mp->U.Mem.BufPtr = (UINT64)ParamShmAddr;
+      Mp->U.Mem.Size = Ip->U.Mem.Size;
+
+      Size = (Ip->U.Mem.Size + sizeof (UINT64) - 1) & ~(sizeof (UINT64) - 1);
+      ParamShmAddr += Size;
+      ShmSize -= Size;
+      break;
+
+    default:
+      return EFI_INVALID_PARAMETER;
+    }
+  }
+
+  return EFI_SUCCESS;
+}
+
+STATIC
+EFI_STATUS
+OpteeFromMsgParam (
+  OUT OPTEE_MSG_PARAM    *OutParams,
+  IN UINT32              NumParams,
+  IN OPTEE_MSG_PARAM     *MsgParams
+  )
+{
+  UINT32                 Idx;
+
+  for (Idx = 0; Idx < NumParams; Idx++) {
+    OPTEE_MSG_PARAM          *Op;
+    CONST OPTEE_MSG_PARAM    *Mp;
+    UINT32                   Attr;
+
+    Op = OutParams + Idx;
+    Mp = MsgParams + Idx;
+    Attr = Mp->Attr & OPTEE_MSG_ATTR_TYPE_MASK;
+
+    switch (Attr) {
+    case OPTEE_MSG_ATTR_TYPE_NONE:
+      Op->Attr = OPTEE_MSG_ATTR_TYPE_NONE;
+      ZeroMem (&Op->U, sizeof (Op->U));
+      break;
+
+    case OPTEE_MSG_ATTR_TYPE_VALUE_INPUT:
+    case OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT:
+    case OPTEE_MSG_ATTR_TYPE_VALUE_INOUT:
+      Op->Attr = Attr;
+      Op->U.Value.A = Mp->U.Value.A;
+      Op->U.Value.B = Mp->U.Value.B;
+      Op->U.Value.C = Mp->U.Value.C;
+      break;
+
+    case OPTEE_MSG_ATTR_TYPE_MEM_INPUT:
+    case OPTEE_MSG_ATTR_TYPE_MEM_OUTPUT:
+    case OPTEE_MSG_ATTR_TYPE_MEM_INOUT:
+      Op->Attr = Attr;
+
+      if (Mp->U.Mem.Size > Op->U.Mem.Size) {
+        return EFI_BAD_BUFFER_SIZE;
+      }
+
+      CopyMem ((VOID *)Op->U.Mem.BufPtr, (VOID *)Mp->U.Mem.BufPtr, Mp->U.Mem.Size);
+      Op->U.Mem.Size = Mp->U.Mem.Size;
+      break;
+
+    default:
+      return EFI_INVALID_PARAMETER;
+    }
+  }
+
+  return EFI_SUCCESS;
+}
+
+EFI_STATUS
+EFIAPI
+OpteeInvokeFunc (
+  IN OUT OPTEE_INVOKE_FUNC_ARG       *InvokeFuncArg
+  )
+{
+  EFI_STATUS       Status;
+  OPTEE_MSG_ARG    *MsgArg;
+
+  MsgArg = NULL;
+
+  if (OpteeShmInfo.Base == 0) {
+    DEBUG ((DEBUG_WARN, "OP-TEE not initialized\n"));
+    return EFI_NOT_STARTED;
+  }
+
+  MsgArg = (OPTEE_MSG_ARG *)OpteeShmInfo.Base;
+  ZeroMem (MsgArg, sizeof (OPTEE_MSG_ARG));
+
+  MsgArg->Cmd = OPTEE_MSG_CMD_INVOKE_COMMAND;
+  MsgArg->Func = InvokeFuncArg->Func;
+  MsgArg->Session = InvokeFuncArg->Session;
+
+  Status = OpteeToMsgParam (MsgArg->Params, MAX_PARAMS, InvokeFuncArg->Params);
+  if (Status)
+    return Status;
+
+  MsgArg->NumParams = MAX_PARAMS;
+
+  if (OpteeCallWithArg ((EFI_PHYSICAL_ADDRESS)MsgArg)) {
+    MsgArg->Ret = OPTEE_ERROR_COMMS;
+    MsgArg->RetOrigin = OPTEE_ORIGIN_COMMS;
+  }
+
+  if (OpteeFromMsgParam (InvokeFuncArg->Params, MAX_PARAMS, MsgArg->Params)) {
+    MsgArg->Ret = OPTEE_ERROR_COMMS;
+    MsgArg->RetOrigin = OPTEE_ORIGIN_COMMS;
+  }
+
+  InvokeFuncArg->Ret = MsgArg->Ret;
+  InvokeFuncArg->RetOrigin = MsgArg->RetOrigin;
+
+  return EFI_SUCCESS;
+}
diff --git a/ArmPkg/Library/OpteeLib/OpteeLib.inf b/ArmPkg/Library/OpteeLib/OpteeLib.inf
index 5abd427379cc..e03054a7167d 100644
--- a/ArmPkg/Library/OpteeLib/OpteeLib.inf
+++ b/ArmPkg/Library/OpteeLib/OpteeLib.inf
@@ -23,11 +23,13 @@ [Defines]
 
 [Sources]
   Optee.c
+  OpteeSmc.h
 
 [Packages]
   ArmPkg/ArmPkg.dec
   MdePkg/MdePkg.dec
 
 [LibraryClasses]
+  ArmMmuLib
   ArmSmcLib
   BaseLib
diff --git a/ArmPkg/Library/OpteeLib/OpteeSmc.h b/ArmPkg/Library/OpteeLib/OpteeSmc.h
new file mode 100644
index 000000000000..e2ea35784a0a
--- /dev/null
+++ b/ArmPkg/Library/OpteeLib/OpteeSmc.h
@@ -0,0 +1,43 @@
+/** @file
+  OP-TEE SMC header file.
+
+  Copyright (c) 2018, Linaro Ltd. All rights reserved.<BR>
+
+  This program and the accompanying materials
+  are licensed and made available under the terms and conditions of the BSD License
+  which accompanies this distribution.  The full text of the license may be found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#ifndef _OPTEE_SMC_H_
+#define _OPTEE_SMC_H_
+
+/* Returned in Arg0 only from Trusted OS functions */
+#define OPTEE_SMC_RETURN_OK                     0x0
+
+#define OPTEE_SMC_RETURN_FROM_RPC               0x32000003
+#define OPTEE_SMC_CALL_WITH_ARG                 0x32000004
+#define OPTEE_SMC_GET_SHM_CONFIG                0xb2000007
+
+#define OPTEE_SMC_SHM_CACHED                    1
+
+#define OPTEE_SMC_RETURN_RPC_FOREIGN_INTR       0xffff0004
+
+#define OPTEE_MSG_CMD_OPEN_SESSION              0
+#define OPTEE_MSG_CMD_INVOKE_COMMAND            1
+#define OPTEE_MSG_CMD_CLOSE_SESSION             2
+
+#define OPTEE_MSG_ATTR_META                     0x100
+
+#define TEE_LOGIN_PUBLIC                        0x0
+
+typedef struct {
+  UINTN    Base;
+  UINTN    Size;
+} OPTEE_SHARED_MEMORY_INFO;
+
+#endif
-- 
2.7.4

Re: [PATCH v3 1/1] ArmPkg/OpteeLib: Add APIs to communicate with OP-TEE

[Ard Biesheuvel]

On 3 October 2018 at 09:43, Sumit Garg <sumit.garg@linaro.org> wrote:
> Add following APIs to communicate with OP-TEE pseudo/early TAs:
> 1. OpteeInit
> 2. OpteeOpenSession
> 3. OpteeCloseSession
> 4. OpteeInvokeFunc
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>

Given the outcome of the GP discussion, I'm fine with this approach. Leif?

> ---
>  ArmPkg/Include/Library/OpteeLib.h    |  90 +++++++++
>  ArmPkg/Library/OpteeLib/Optee.c      | 357 +++++++++++++++++++++++++++++++++++
>  ArmPkg/Library/OpteeLib/OpteeLib.inf |   2 +
>  ArmPkg/Library/OpteeLib/OpteeSmc.h   |  43 +++++
>  4 files changed, 492 insertions(+)
>  create mode 100644 ArmPkg/Library/OpteeLib/OpteeSmc.h
>
> diff --git a/ArmPkg/Include/Library/OpteeLib.h b/ArmPkg/Include/Library/OpteeLib.h
> index f65d8674d9b8..2d1c60632dfe 100644
> --- a/ArmPkg/Include/Library/OpteeLib.h
> +++ b/ArmPkg/Include/Library/OpteeLib.h
> @@ -25,10 +25,100 @@
>  #define OPTEE_OS_UID2          0xaf630002
>  #define OPTEE_OS_UID3          0xa5d5c51b
>
> +#define OPTEE_MSG_ATTR_TYPE_NONE                0x0
> +#define OPTEE_MSG_ATTR_TYPE_VALUE_INPUT         0x1
> +#define OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT        0x2
> +#define OPTEE_MSG_ATTR_TYPE_VALUE_INOUT         0x3
> +#define OPTEE_MSG_ATTR_TYPE_MEM_INPUT           0x9
> +#define OPTEE_MSG_ATTR_TYPE_MEM_OUTPUT          0xa
> +#define OPTEE_MSG_ATTR_TYPE_MEM_INOUT           0xb
> +
> +#define OPTEE_MSG_ATTR_TYPE_MASK                0xff
> +
> +#define OPTEE_ORIGIN_COMMS                      0x00000002
> +#define OPTEE_ERROR_COMMS                       0xFFFF000E
> +
> +typedef struct {
> +  UINT64    BufPtr;
> +  UINT64    Size;
> +  UINT64    ShmRef;
> +} OPTEE_MSG_PARAM_MEM;
> +
> +typedef struct {
> +  UINT64    A;
> +  UINT64    B;
> +  UINT64    C;
> +} OPTEE_MSG_PARAM_VALUE;
> +
> +typedef struct {
> +  UINT64 Attr;
> +  union {
> +    OPTEE_MSG_PARAM_MEM      Mem;
> +    OPTEE_MSG_PARAM_VALUE    Value;
> +  } U;
> +} OPTEE_MSG_PARAM;
> +
> +#define MAX_PARAMS           4
> +
> +typedef struct {
> +        UINT32             Cmd;
> +        UINT32             Func;
> +        UINT32             Session;
> +        UINT32             CancelId;
> +        UINT32             Pad;
> +        UINT32             Ret;
> +        UINT32             RetOrigin;
> +        UINT32             NumParams;
> +
> +        // NumParams tells the actual number of element in Params
> +        OPTEE_MSG_PARAM    Params[MAX_PARAMS];
> +} OPTEE_MSG_ARG;
> +
> +#define OPTEE_UUID_LEN       16
> +
> +typedef struct {
> +        UINT8     Uuid[OPTEE_UUID_LEN]; // [in] UUID of the Trusted Application
> +        UINT32    Session;              // [out] Session id
> +        UINT32    Ret;                  // [out] Return value
> +        UINT32    RetOrigin;            // [out] Origin of the return value
> +} OPTEE_OPEN_SESSION_ARG;
> +
> +typedef struct {
> +        UINT32             Func;    // [in] Trusted App func, specific to the TA
> +        UINT32             Session; // [in] Session id
> +        UINT32             Ret;     // [out] Return value
> +        UINT32             RetOrigin; // [out] Origin of the return value
> +        OPTEE_MSG_PARAM    Params[MAX_PARAMS]; // Params for func to be invoked
> +} OPTEE_INVOKE_FUNC_ARG;
> +
>  BOOLEAN
>  EFIAPI
>  IsOpteePresent (
>    VOID
>    );
>
> +EFI_STATUS
> +EFIAPI
> +OpteeInit (
> +  VOID
> +  );
> +
> +EFI_STATUS
> +EFIAPI
> +OpteeOpenSession (
> +  IN OUT OPTEE_OPEN_SESSION_ARG      *OpenSessionArg
> +  );
> +
> +EFI_STATUS
> +EFIAPI
> +OpteeCloseSession (
> +  IN UINT32                      Session
> +  );
> +
> +EFI_STATUS
> +EFIAPI
> +OpteeInvokeFunc (
> +  IN OUT OPTEE_INVOKE_FUNC_ARG       *InvokeFuncArg
> +  );
> +
>  #endif
> diff --git a/ArmPkg/Library/OpteeLib/Optee.c b/ArmPkg/Library/OpteeLib/Optee.c
> index 574527f8b5ea..bf7872cbbce0 100644
> --- a/ArmPkg/Library/OpteeLib/Optee.c
> +++ b/ArmPkg/Library/OpteeLib/Optee.c
> @@ -14,11 +14,18 @@
>
>  **/
>
> +#include <Library/ArmMmuLib.h>
>  #include <Library/ArmSmcLib.h>
> +#include <Library/BaseMemoryLib.h>
>  #include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
>  #include <Library/OpteeLib.h>
>
>  #include <IndustryStandard/ArmStdSmc.h>
> +#include <OpteeSmc.h>
> +#include <Uefi.h>
> +
> +STATIC OPTEE_SHARED_MEMORY_INFO OpteeShmInfo = { 0 };
>
>  /**
>    Check for OP-TEE presence.
> @@ -31,6 +38,7 @@ IsOpteePresent (
>  {
>    ARM_SMC_ARGS ArmSmcArgs;
>
> +  ZeroMem (&ArmSmcArgs, sizeof (ARM_SMC_ARGS));
>    // Send a Trusted OS Calls UID command
>    ArmSmcArgs.Arg0 = ARM_SMC_ID_TOS_UID;
>    ArmCallSmc (&ArmSmcArgs);
> @@ -44,3 +52,352 @@ IsOpteePresent (
>      return FALSE;
>    }
>  }
> +
> +STATIC
> +EFI_STATUS
> +OpteeShmMemRemap (
> +  VOID
> +  )
> +{
> +  ARM_SMC_ARGS                 ArmSmcArgs;
> +  EFI_PHYSICAL_ADDRESS         Paddr;
> +  EFI_PHYSICAL_ADDRESS         Start;
> +  EFI_PHYSICAL_ADDRESS         End;
> +  EFI_STATUS                   Status;
> +  UINTN                        Size;
> +
> +  ZeroMem (&ArmSmcArgs, sizeof (ARM_SMC_ARGS));
> +  ArmSmcArgs.Arg0 = OPTEE_SMC_GET_SHM_CONFIG;
> +
> +  ArmCallSmc (&ArmSmcArgs);
> +  if (ArmSmcArgs.Arg0 != OPTEE_SMC_RETURN_OK) {
> +    DEBUG ((DEBUG_WARN, "OP-TEE shared memory not supported\n"));
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  if (ArmSmcArgs.Arg3 != OPTEE_SMC_SHM_CACHED) {
> +    DEBUG ((DEBUG_WARN, "OP-TEE: Only normal cached shared memory supported\n"));
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  Start = (ArmSmcArgs.Arg1 + SIZE_4KB - 1) & ~(SIZE_4KB - 1);
> +  End = (ArmSmcArgs.Arg1 + ArmSmcArgs.Arg2) & ~(SIZE_4KB - 1);
> +  Paddr = Start;
> +  Size = End - Start;
> +
> +  if (Size < SIZE_4KB) {
> +    DEBUG ((DEBUG_WARN, "OP-TEE shared memory too small\n"));
> +    return EFI_BUFFER_TOO_SMALL;
> +  }
> +
> +  Status = ArmSetMemoryAttributes (Paddr, Size, EFI_MEMORY_WB);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  OpteeShmInfo.Base = (UINTN)Paddr;
> +  OpteeShmInfo.Size = Size;
> +
> +  return EFI_SUCCESS;
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +OpteeInit (
> +  VOID
> +  )
> +{
> +  EFI_STATUS      Status;
> +
> +  if (!IsOpteePresent ()) {
> +    DEBUG ((DEBUG_WARN, "OP-TEE not present\n"));
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  Status = OpteeShmMemRemap ();
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_WARN, "OP-TEE shared memory remap failed\n"));
> +    return Status;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Does Standard SMC to OP-TEE in secure world.
> +
> +  @param[in]  Parg   Physical address of message to pass to secure world
> +
> +  @return            0 on success, secure world return code otherwise
> +
> +**/
> +STATIC
> +UINT32
> +OpteeCallWithArg (
> +  IN EFI_PHYSICAL_ADDRESS Parg
> +  )
> +{
> +  ARM_SMC_ARGS ArmSmcArgs;
> +
> +  ZeroMem (&ArmSmcArgs, sizeof (ARM_SMC_ARGS));
> +  ArmSmcArgs.Arg0 = OPTEE_SMC_CALL_WITH_ARG;
> +  ArmSmcArgs.Arg1 = (UINT32)(Parg >> 32);
> +  ArmSmcArgs.Arg2 = (UINT32)Parg;
> +
> +  while (TRUE) {
> +    ArmCallSmc (&ArmSmcArgs);
> +
> +    if (ArmSmcArgs.Arg0 == OPTEE_SMC_RETURN_RPC_FOREIGN_INTR) {
> +      //
> +      // A foreign interrupt was raised while secure world was
> +      // executing, since they are handled in UEFI a dummy RPC is
> +      // performed to let UEFI take the interrupt through the normal
> +      // vector.
> +      //
> +      ArmSmcArgs.Arg0 = OPTEE_SMC_RETURN_FROM_RPC;
> +    } else {
> +      break;
> +    }
> +  }
> +
> +  return ArmSmcArgs.Arg0;
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +OpteeOpenSession (
> +  IN OUT OPTEE_OPEN_SESSION_ARG      *OpenSessionArg
> +  )
> +{
> +  OPTEE_MSG_ARG    *MsgArg;
> +
> +  MsgArg = NULL;
> +
> +  if (OpteeShmInfo.Base == 0) {
> +    DEBUG ((DEBUG_WARN, "OP-TEE not initialized\n"));
> +    return EFI_NOT_STARTED;
> +  }
> +
> +  MsgArg = (OPTEE_MSG_ARG *)OpteeShmInfo.Base;
> +  ZeroMem (MsgArg, sizeof (OPTEE_MSG_ARG));
> +
> +  MsgArg->Cmd = OPTEE_MSG_CMD_OPEN_SESSION;
> +
> +  //
> +  // Initialize and add the meta parameters needed when opening a
> +  // session.
> +  //
> +  MsgArg->Params[0].Attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT |
> +                           OPTEE_MSG_ATTR_META;
> +  MsgArg->Params[1].Attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT |
> +                           OPTEE_MSG_ATTR_META;
> +  CopyMem (&MsgArg->Params[0].U.Value, OpenSessionArg->Uuid, OPTEE_UUID_LEN);
> +  ZeroMem (&MsgArg->Params[1].U.Value, OPTEE_UUID_LEN);
> +  MsgArg->Params[1].U.Value.C = TEE_LOGIN_PUBLIC;
> +
> +  MsgArg->NumParams = 2;
> +
> +  if (OpteeCallWithArg ((EFI_PHYSICAL_ADDRESS)MsgArg)) {
> +    MsgArg->Ret = OPTEE_ERROR_COMMS;
> +    MsgArg->RetOrigin = OPTEE_ORIGIN_COMMS;
> +  }
> +
> +  OpenSessionArg->Session = MsgArg->Session;
> +  OpenSessionArg->Ret = MsgArg->Ret;
> +  OpenSessionArg->RetOrigin = MsgArg->RetOrigin;
> +
> +  return EFI_SUCCESS;
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +OpteeCloseSession (
> +  IN UINT32                      Session
> +  )
> +{
> +  OPTEE_MSG_ARG    *MsgArg;
> +
> +  MsgArg = NULL;
> +
> +  if (OpteeShmInfo.Base == 0) {
> +    DEBUG ((DEBUG_WARN, "OP-TEE not initialized\n"));
> +    return EFI_NOT_STARTED;
> +  }
> +
> +  MsgArg = (OPTEE_MSG_ARG *)OpteeShmInfo.Base;
> +  ZeroMem (MsgArg, sizeof (OPTEE_MSG_ARG));
> +
> +  MsgArg->Cmd = OPTEE_MSG_CMD_CLOSE_SESSION;
> +  MsgArg->Session = Session;
> +
> +  OpteeCallWithArg ((EFI_PHYSICAL_ADDRESS)MsgArg);
> +
> +  return EFI_SUCCESS;
> +}
> +
> +STATIC
> +EFI_STATUS
> +OpteeToMsgParam (
> +  OUT OPTEE_MSG_PARAM    *MsgParams,
> +  IN UINT32              NumParams,
> +  IN OPTEE_MSG_PARAM     *InParams
> +  )
> +{
> +  UINT32                  Idx;
> +  UINTN                   ParamShmAddr;
> +  UINTN                   ShmSize;
> +  UINTN                   Size;
> +
> +  Size = (sizeof (OPTEE_MSG_ARG) + sizeof (UINT64) - 1) & ~(sizeof (UINT64) - 1);
> +  ParamShmAddr = OpteeShmInfo.Base + Size;
> +  ShmSize = OpteeShmInfo.Size - Size;
> +
> +  for (Idx = 0; Idx < NumParams; Idx++) {
> +    CONST OPTEE_MSG_PARAM    *Ip;
> +    OPTEE_MSG_PARAM          *Mp;
> +    UINT32                   Attr;
> +
> +    Ip = InParams + Idx;
> +    Mp = MsgParams + Idx;
> +    Attr = Ip->Attr & OPTEE_MSG_ATTR_TYPE_MASK;
> +
> +    switch (Attr) {
> +    case OPTEE_MSG_ATTR_TYPE_NONE:
> +      Mp->Attr = OPTEE_MSG_ATTR_TYPE_NONE;
> +      ZeroMem (&Mp->U, sizeof (Mp->U));
> +      break;
> +
> +    case OPTEE_MSG_ATTR_TYPE_VALUE_INPUT:
> +    case OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT:
> +    case OPTEE_MSG_ATTR_TYPE_VALUE_INOUT:
> +      Mp->Attr = Attr;
> +      Mp->U.Value.A = Ip->U.Value.A;
> +      Mp->U.Value.B = Ip->U.Value.B;
> +      Mp->U.Value.C = Ip->U.Value.C;
> +      break;
> +
> +    case OPTEE_MSG_ATTR_TYPE_MEM_INPUT:
> +    case OPTEE_MSG_ATTR_TYPE_MEM_OUTPUT:
> +    case OPTEE_MSG_ATTR_TYPE_MEM_INOUT:
> +      Mp->Attr = Attr;
> +
> +      if (Ip->U.Mem.Size > ShmSize) {
> +        return EFI_OUT_OF_RESOURCES;
> +      }
> +
> +      CopyMem ((VOID *)ParamShmAddr, (VOID *)Ip->U.Mem.BufPtr, Ip->U.Mem.Size);
> +      Mp->U.Mem.BufPtr = (UINT64)ParamShmAddr;
> +      Mp->U.Mem.Size = Ip->U.Mem.Size;
> +
> +      Size = (Ip->U.Mem.Size + sizeof (UINT64) - 1) & ~(sizeof (UINT64) - 1);
> +      ParamShmAddr += Size;
> +      ShmSize -= Size;
> +      break;
> +
> +    default:
> +      return EFI_INVALID_PARAMETER;
> +    }
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +STATIC
> +EFI_STATUS
> +OpteeFromMsgParam (
> +  OUT OPTEE_MSG_PARAM    *OutParams,
> +  IN UINT32              NumParams,
> +  IN OPTEE_MSG_PARAM     *MsgParams
> +  )
> +{
> +  UINT32                 Idx;
> +
> +  for (Idx = 0; Idx < NumParams; Idx++) {
> +    OPTEE_MSG_PARAM          *Op;
> +    CONST OPTEE_MSG_PARAM    *Mp;
> +    UINT32                   Attr;
> +
> +    Op = OutParams + Idx;
> +    Mp = MsgParams + Idx;
> +    Attr = Mp->Attr & OPTEE_MSG_ATTR_TYPE_MASK;
> +
> +    switch (Attr) {
> +    case OPTEE_MSG_ATTR_TYPE_NONE:
> +      Op->Attr = OPTEE_MSG_ATTR_TYPE_NONE;
> +      ZeroMem (&Op->U, sizeof (Op->U));
> +      break;
> +
> +    case OPTEE_MSG_ATTR_TYPE_VALUE_INPUT:
> +    case OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT:
> +    case OPTEE_MSG_ATTR_TYPE_VALUE_INOUT:
> +      Op->Attr = Attr;
> +      Op->U.Value.A = Mp->U.Value.A;
> +      Op->U.Value.B = Mp->U.Value.B;
> +      Op->U.Value.C = Mp->U.Value.C;
> +      break;
> +
> +    case OPTEE_MSG_ATTR_TYPE_MEM_INPUT:
> +    case OPTEE_MSG_ATTR_TYPE_MEM_OUTPUT:
> +    case OPTEE_MSG_ATTR_TYPE_MEM_INOUT:
> +      Op->Attr = Attr;
> +
> +      if (Mp->U.Mem.Size > Op->U.Mem.Size) {
> +        return EFI_BAD_BUFFER_SIZE;
> +      }
> +
> +      CopyMem ((VOID *)Op->U.Mem.BufPtr, (VOID *)Mp->U.Mem.BufPtr, Mp->U.Mem.Size);
> +      Op->U.Mem.Size = Mp->U.Mem.Size;
> +      break;
> +
> +    default:
> +      return EFI_INVALID_PARAMETER;
> +    }
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +OpteeInvokeFunc (
> +  IN OUT OPTEE_INVOKE_FUNC_ARG       *InvokeFuncArg
> +  )
> +{
> +  EFI_STATUS       Status;
> +  OPTEE_MSG_ARG    *MsgArg;
> +
> +  MsgArg = NULL;
> +
> +  if (OpteeShmInfo.Base == 0) {
> +    DEBUG ((DEBUG_WARN, "OP-TEE not initialized\n"));
> +    return EFI_NOT_STARTED;
> +  }
> +
> +  MsgArg = (OPTEE_MSG_ARG *)OpteeShmInfo.Base;
> +  ZeroMem (MsgArg, sizeof (OPTEE_MSG_ARG));
> +
> +  MsgArg->Cmd = OPTEE_MSG_CMD_INVOKE_COMMAND;
> +  MsgArg->Func = InvokeFuncArg->Func;
> +  MsgArg->Session = InvokeFuncArg->Session;
> +
> +  Status = OpteeToMsgParam (MsgArg->Params, MAX_PARAMS, InvokeFuncArg->Params);
> +  if (Status)
> +    return Status;
> +
> +  MsgArg->NumParams = MAX_PARAMS;
> +
> +  if (OpteeCallWithArg ((EFI_PHYSICAL_ADDRESS)MsgArg)) {
> +    MsgArg->Ret = OPTEE_ERROR_COMMS;
> +    MsgArg->RetOrigin = OPTEE_ORIGIN_COMMS;
> +  }
> +
> +  if (OpteeFromMsgParam (InvokeFuncArg->Params, MAX_PARAMS, MsgArg->Params)) {
> +    MsgArg->Ret = OPTEE_ERROR_COMMS;
> +    MsgArg->RetOrigin = OPTEE_ORIGIN_COMMS;
> +  }
> +
> +  InvokeFuncArg->Ret = MsgArg->Ret;
> +  InvokeFuncArg->RetOrigin = MsgArg->RetOrigin;
> +
> +  return EFI_SUCCESS;
> +}
> diff --git a/ArmPkg/Library/OpteeLib/OpteeLib.inf b/ArmPkg/Library/OpteeLib/OpteeLib.inf
> index 5abd427379cc..e03054a7167d 100644
> --- a/ArmPkg/Library/OpteeLib/OpteeLib.inf
> +++ b/ArmPkg/Library/OpteeLib/OpteeLib.inf
> @@ -23,11 +23,13 @@ [Defines]
>
>  [Sources]
>    Optee.c
> +  OpteeSmc.h
>
>  [Packages]
>    ArmPkg/ArmPkg.dec
>    MdePkg/MdePkg.dec
>
>  [LibraryClasses]
> +  ArmMmuLib
>    ArmSmcLib
>    BaseLib
> diff --git a/ArmPkg/Library/OpteeLib/OpteeSmc.h b/ArmPkg/Library/OpteeLib/OpteeSmc.h
> new file mode 100644
> index 000000000000..e2ea35784a0a
> --- /dev/null
> +++ b/ArmPkg/Library/OpteeLib/OpteeSmc.h
> @@ -0,0 +1,43 @@
> +/** @file
> +  OP-TEE SMC header file.
> +
> +  Copyright (c) 2018, Linaro Ltd. All rights reserved.<BR>
> +
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD License
> +  which accompanies this distribution.  The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#ifndef _OPTEE_SMC_H_
> +#define _OPTEE_SMC_H_
> +
> +/* Returned in Arg0 only from Trusted OS functions */
> +#define OPTEE_SMC_RETURN_OK                     0x0
> +
> +#define OPTEE_SMC_RETURN_FROM_RPC               0x32000003
> +#define OPTEE_SMC_CALL_WITH_ARG                 0x32000004
> +#define OPTEE_SMC_GET_SHM_CONFIG                0xb2000007
> +
> +#define OPTEE_SMC_SHM_CACHED                    1
> +
> +#define OPTEE_SMC_RETURN_RPC_FOREIGN_INTR       0xffff0004
> +
> +#define OPTEE_MSG_CMD_OPEN_SESSION              0
> +#define OPTEE_MSG_CMD_INVOKE_COMMAND            1
> +#define OPTEE_MSG_CMD_CLOSE_SESSION             2
> +
> +#define OPTEE_MSG_ATTR_META                     0x100
> +
> +#define TEE_LOGIN_PUBLIC                        0x0
> +
> +typedef struct {
> +  UINTN    Base;
> +  UINTN    Size;
> +} OPTEE_SHARED_MEMORY_INFO;
> +
> +#endif
> --
> 2.7.4
>

Re: [PATCH v3 1/1] ArmPkg/OpteeLib: Add APIs to communicate with OP-TEE

[Leif Lindholm]

On Wed, Oct 03, 2018 at 11:33:01AM +0200, Ard Biesheuvel wrote:
> On 3 October 2018 at 09:43, Sumit Garg <sumit.garg@linaro.org> wrote:
> > Add following APIs to communicate with OP-TEE pseudo/early TAs:
> > 1. OpteeInit
> > 2. OpteeOpenSession
> > 3. OpteeCloseSession
> > 4. OpteeInvokeFunc
> >
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> 
> Given the outcome of the GP discussion, I'm fine with this approach. Leif?

Apologies for the delay, I needed some time to think it over.

I'm not super happy about this approach, but I'm happier with this
than I would be with leaving the functionality out of the upstream
tree. I really hope we can get that license either changed or dropped
completely.

I do have a few comments below.

> > ---
> >  ArmPkg/Include/Library/OpteeLib.h    |  90 +++++++++
> >  ArmPkg/Library/OpteeLib/Optee.c      | 357 +++++++++++++++++++++++++++++++++++
> >  ArmPkg/Library/OpteeLib/OpteeLib.inf |   2 +
> >  ArmPkg/Library/OpteeLib/OpteeSmc.h   |  43 +++++

Could you follow the instructions in
https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-23
when generating future patches?
The --stat* effects aren't apparent here, but the -O ones are.

> >  4 files changed, 492 insertions(+)
> >  create mode 100644 ArmPkg/Library/OpteeLib/OpteeSmc.h
> >
> > diff --git a/ArmPkg/Include/Library/OpteeLib.h b/ArmPkg/Include/Library/OpteeLib.h
> > index f65d8674d9b8..2d1c60632dfe 100644
> > --- a/ArmPkg/Include/Library/OpteeLib.h
> > +++ b/ArmPkg/Include/Library/OpteeLib.h
> > @@ -25,10 +25,100 @@
> >  #define OPTEE_OS_UID2          0xaf630002
> >  #define OPTEE_OS_UID3          0xa5d5c51b
> >
> > +#define OPTEE_MSG_ATTR_TYPE_NONE                0x0
> > +#define OPTEE_MSG_ATTR_TYPE_VALUE_INPUT         0x1
> > +#define OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT        0x2
> > +#define OPTEE_MSG_ATTR_TYPE_VALUE_INOUT         0x3
> > +#define OPTEE_MSG_ATTR_TYPE_MEM_INPUT           0x9
> > +#define OPTEE_MSG_ATTR_TYPE_MEM_OUTPUT          0xa
> > +#define OPTEE_MSG_ATTR_TYPE_MEM_INOUT           0xb
> > +
> > +#define OPTEE_MSG_ATTR_TYPE_MASK                0xff
> > +
> > +#define OPTEE_ORIGIN_COMMS                      0x00000002
> > +#define OPTEE_ERROR_COMMS                       0xFFFF000E
> > +
> > +typedef struct {
> > +  UINT64    BufPtr;

If it's a pointer, it has a *.
Otherwise it's an address.

> > +  UINT64    Size;
> > +  UINT64    ShmRef;

Abbreviations in function, variable or type names (other than the ones
defined in [1] are not permitted unless they are explicitly added to a
glossary section of the source file header. Where possible, just write
out the name in full.

[1] https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/v/release/2.20/4_naming_conventions/#table-2-efi-supported-abbreviations

BufPtr (as a name) gets a pass since it's unambiguous and we already
have a bunch of those in the codebase. ShmRef needs to be clear.

(This comment also applies to a lot of things below, I won't point
them all out unless asked to.)

> > +} OPTEE_MSG_PARAM_MEM;
> > +
> > +typedef struct {
> > +  UINT64    A;
> > +  UINT64    B;
> > +  UINT64    C;
> > +} OPTEE_MSG_PARAM_VALUE;
> > +
> > +typedef struct {
> > +  UINT64 Attr;
> > +  union {
> > +    OPTEE_MSG_PARAM_MEM      Mem;
> > +    OPTEE_MSG_PARAM_VALUE    Value;
> > +  } U;
> > +} OPTEE_MSG_PARAM;
> > +
> > +#define MAX_PARAMS           4

This is a very localised macro with a very globalised name.
Suggest adding an OPTEE_ prefix, but also something describing what it
is the maximum parameters for. CALL_?

> > +
> > +typedef struct {
> > +        UINT32             Cmd;
> > +        UINT32             Func;
> > +        UINT32             Session;
> > +        UINT32             CancelId;
> > +        UINT32             Pad;
> > +        UINT32             Ret;
> > +        UINT32             RetOrigin;
> > +        UINT32             NumParams;
> > +
> > +        // NumParams tells the actual number of element in Params
> > +        OPTEE_MSG_PARAM    Params[MAX_PARAMS];
> > +} OPTEE_MSG_ARG;
> > +
> > +#define OPTEE_UUID_LEN       16

UUIDs are UUIDs. If optee decides on an incompatible format, we may
have an interoperability issue. I assume this is not the case, so
perhaps replace with sizeof (EFI_GUID) at each point of use?

> > +
> > +typedef struct {
> > +        UINT8     Uuid[OPTEE_UUID_LEN]; // [in] UUID of the Trusted Application

Is there a strong reason for not using EFI_GUID here?

> > +        UINT32    Session;              // [out] Session id
> > +        UINT32    Ret;                  // [out] Return value
> > +        UINT32    RetOrigin;            // [out] Origin of the return value
> > +} OPTEE_OPEN_SESSION_ARG;
> > +
> > +typedef struct {
> > +        UINT32             Func;    // [in] Trusted App func, specific to the TA
> > +        UINT32             Session; // [in] Session id
> > +        UINT32             Ret;     // [out] Return value
> > +        UINT32             RetOrigin; // [out] Origin of the return value
> > +        OPTEE_MSG_PARAM    Params[MAX_PARAMS]; // Params for func to be invoked
> > +} OPTEE_INVOKE_FUNC_ARG;
> > +
> >  BOOLEAN
> >  EFIAPI
> >  IsOpteePresent (
> >    VOID
> >    );
> >
> > +EFI_STATUS
> > +EFIAPI
> > +OpteeInit (
> > +  VOID
> > +  );
> > +
> > +EFI_STATUS
> > +EFIAPI
> > +OpteeOpenSession (
> > +  IN OUT OPTEE_OPEN_SESSION_ARG      *OpenSessionArg
> > +  );
> > +
> > +EFI_STATUS
> > +EFIAPI
> > +OpteeCloseSession (
> > +  IN UINT32                      Session
> > +  );
> > +
> > +EFI_STATUS
> > +EFIAPI
> > +OpteeInvokeFunc (
> > +  IN OUT OPTEE_INVOKE_FUNC_ARG       *InvokeFuncArg
> > +  );
> > +
> >  #endif
> > diff --git a/ArmPkg/Library/OpteeLib/Optee.c b/ArmPkg/Library/OpteeLib/Optee.c
> > index 574527f8b5ea..bf7872cbbce0 100644
> > --- a/ArmPkg/Library/OpteeLib/Optee.c
> > +++ b/ArmPkg/Library/OpteeLib/Optee.c
> > @@ -14,11 +14,18 @@
> >
> >  **/
> >
> > +#include <Library/ArmMmuLib.h>
> >  #include <Library/ArmSmcLib.h>
> > +#include <Library/BaseMemoryLib.h>
> >  #include <Library/BaseLib.h>
> > +#include <Library/DebugLib.h>
> >  #include <Library/OpteeLib.h>
> >
> >  #include <IndustryStandard/ArmStdSmc.h>
> > +#include <OpteeSmc.h>
> > +#include <Uefi.h>
> > +
> > +STATIC OPTEE_SHARED_MEMORY_INFO OpteeShmInfo = { 0 };
> >
> >  /**
> >    Check for OP-TEE presence.
> > @@ -31,6 +38,7 @@ IsOpteePresent (
> >  {
> >    ARM_SMC_ARGS ArmSmcArgs;
> >
> > +  ZeroMem (&ArmSmcArgs, sizeof (ARM_SMC_ARGS));
> >    // Send a Trusted OS Calls UID command
> >    ArmSmcArgs.Arg0 = ARM_SMC_ID_TOS_UID;
> >    ArmCallSmc (&ArmSmcArgs);
> > @@ -44,3 +52,352 @@ IsOpteePresent (
> >      return FALSE;
> >    }
> >  }
> > +
> > +STATIC
> > +EFI_STATUS
> > +OpteeShmMemRemap (
> > +  VOID
> > +  )
> > +{
> > +  ARM_SMC_ARGS                 ArmSmcArgs;
> > +  EFI_PHYSICAL_ADDRESS         Paddr;
> > +  EFI_PHYSICAL_ADDRESS         Start;
> > +  EFI_PHYSICAL_ADDRESS         End;
> > +  EFI_STATUS                   Status;
> > +  UINTN                        Size;
> > +
> > +  ZeroMem (&ArmSmcArgs, sizeof (ARM_SMC_ARGS));
> > +  ArmSmcArgs.Arg0 = OPTEE_SMC_GET_SHM_CONFIG;
> > +
> > +  ArmCallSmc (&ArmSmcArgs);
> > +  if (ArmSmcArgs.Arg0 != OPTEE_SMC_RETURN_OK) {
> > +    DEBUG ((DEBUG_WARN, "OP-TEE shared memory not supported\n"));
> > +    return EFI_UNSUPPORTED;
> > +  }
> > +
> > +  if (ArmSmcArgs.Arg3 != OPTEE_SMC_SHM_CACHED) {
> > +    DEBUG ((DEBUG_WARN, "OP-TEE: Only normal cached shared memory supported\n"));
> > +    return EFI_UNSUPPORTED;
> > +  }
> > +
> > +  Start = (ArmSmcArgs.Arg1 + SIZE_4KB - 1) & ~(SIZE_4KB - 1);
> > +  End = (ArmSmcArgs.Arg1 + ArmSmcArgs.Arg2) & ~(SIZE_4KB - 1);
> > +  Paddr = Start;
> > +  Size = End - Start;
> > +
> > +  if (Size < SIZE_4KB) {
> > +    DEBUG ((DEBUG_WARN, "OP-TEE shared memory too small\n"));
> > +    return EFI_BUFFER_TOO_SMALL;
> > +  }
> > +
> > +  Status = ArmSetMemoryAttributes (Paddr, Size, EFI_MEMORY_WB);
> > +  if (EFI_ERROR (Status)) {
> > +    return Status;
> > +  }
> > +
> > +  OpteeShmInfo.Base = (UINTN)Paddr;

Would PHYSICAL_ADDRESS (always 64-bit) be more correct here, or is the
OP-TEE end seeing this struct as always having fields of native width?

(This is where the ordefile comes in handy, because it means I see the
struct definition first, not its use.)

> > +  OpteeShmInfo.Size = Size;
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +EFI_STATUS
> > +EFIAPI
> > +OpteeInit (
> > +  VOID
> > +  )
> > +{
> > +  EFI_STATUS      Status;
> > +
> > +  if (!IsOpteePresent ()) {
> > +    DEBUG ((DEBUG_WARN, "OP-TEE not present\n"));
> > +    return EFI_UNSUPPORTED;
> > +  }
> > +
> > +  Status = OpteeShmMemRemap ();
> > +  if (EFI_ERROR (Status)) {
> > +    DEBUG ((DEBUG_WARN, "OP-TEE shared memory remap failed\n"));
> > +    return Status;
> > +  }
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > +  Does Standard SMC to OP-TEE in secure world.
> > +
> > +  @param[in]  Parg   Physical address of message to pass to secure world
> > +
> > +  @return            0 on success, secure world return code otherwise
> > +
> > +**/
> > +STATIC
> > +UINT32
> > +OpteeCallWithArg (
> > +  IN EFI_PHYSICAL_ADDRESS Parg
> > +  )
> > +{
> > +  ARM_SMC_ARGS ArmSmcArgs;
> > +
> > +  ZeroMem (&ArmSmcArgs, sizeof (ARM_SMC_ARGS));
> > +  ArmSmcArgs.Arg0 = OPTEE_SMC_CALL_WITH_ARG;
> > +  ArmSmcArgs.Arg1 = (UINT32)(Parg >> 32);
> > +  ArmSmcArgs.Arg2 = (UINT32)Parg;
> > +
> > +  while (TRUE) {
> > +    ArmCallSmc (&ArmSmcArgs);
> > +
> > +    if (ArmSmcArgs.Arg0 == OPTEE_SMC_RETURN_RPC_FOREIGN_INTR) {
> > +      //
> > +      // A foreign interrupt was raised while secure world was
> > +      // executing, since they are handled in UEFI a dummy RPC is
> > +      // performed to let UEFI take the interrupt through the normal
> > +      // vector.
> > +      //
> > +      ArmSmcArgs.Arg0 = OPTEE_SMC_RETURN_FROM_RPC;
> > +    } else {
> > +      break;
> > +    }
> > +  }
> > +
> > +  return ArmSmcArgs.Arg0;
> > +}
> > +
> > +EFI_STATUS
> > +EFIAPI
> > +OpteeOpenSession (
> > +  IN OUT OPTEE_OPEN_SESSION_ARG      *OpenSessionArg
> > +  )
> > +{
> > +  OPTEE_MSG_ARG    *MsgArg;
> > +
> > +  MsgArg = NULL;
> > +
> > +  if (OpteeShmInfo.Base == 0) {
> > +    DEBUG ((DEBUG_WARN, "OP-TEE not initialized\n"));
> > +    return EFI_NOT_STARTED;
> > +  }
> > +
> > +  MsgArg = (OPTEE_MSG_ARG *)OpteeShmInfo.Base;
> > +  ZeroMem (MsgArg, sizeof (OPTEE_MSG_ARG));
> > +
> > +  MsgArg->Cmd = OPTEE_MSG_CMD_OPEN_SESSION;
> > +
> > +  //
> > +  // Initialize and add the meta parameters needed when opening a
> > +  // session.
> > +  //
> > +  MsgArg->Params[0].Attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT |
> > +                           OPTEE_MSG_ATTR_META;
> > +  MsgArg->Params[1].Attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT |
> > +                           OPTEE_MSG_ATTR_META;
> > +  CopyMem (&MsgArg->Params[0].U.Value, OpenSessionArg->Uuid, OPTEE_UUID_LEN);
> > +  ZeroMem (&MsgArg->Params[1].U.Value, OPTEE_UUID_LEN);
> > +  MsgArg->Params[1].U.Value.C = TEE_LOGIN_PUBLIC;
> > +
> > +  MsgArg->NumParams = 2;
> > +
> > +  if (OpteeCallWithArg ((EFI_PHYSICAL_ADDRESS)MsgArg)) {
> > +    MsgArg->Ret = OPTEE_ERROR_COMMS;
> > +    MsgArg->RetOrigin = OPTEE_ORIGIN_COMMS;
> > +  }
> > +
> > +  OpenSessionArg->Session = MsgArg->Session;
> > +  OpenSessionArg->Ret = MsgArg->Ret;
> > +  OpenSessionArg->RetOrigin = MsgArg->RetOrigin;
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +EFI_STATUS
> > +EFIAPI
> > +OpteeCloseSession (
> > +  IN UINT32                      Session
> > +  )
> > +{
> > +  OPTEE_MSG_ARG    *MsgArg;
> > +
> > +  MsgArg = NULL;
> > +
> > +  if (OpteeShmInfo.Base == 0) {
> > +    DEBUG ((DEBUG_WARN, "OP-TEE not initialized\n"));
> > +    return EFI_NOT_STARTED;
> > +  }
> > +
> > +  MsgArg = (OPTEE_MSG_ARG *)OpteeShmInfo.Base;
> > +  ZeroMem (MsgArg, sizeof (OPTEE_MSG_ARG));
> > +
> > +  MsgArg->Cmd = OPTEE_MSG_CMD_CLOSE_SESSION;
> > +  MsgArg->Session = Session;
> > +
> > +  OpteeCallWithArg ((EFI_PHYSICAL_ADDRESS)MsgArg);
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +STATIC
> > +EFI_STATUS
> > +OpteeToMsgParam (
> > +  OUT OPTEE_MSG_PARAM    *MsgParams,
> > +  IN UINT32              NumParams,
> > +  IN OPTEE_MSG_PARAM     *InParams
> > +  )
> > +{
> > +  UINT32                  Idx;
> > +  UINTN                   ParamShmAddr;
> > +  UINTN                   ShmSize;
> > +  UINTN                   Size;
> > +
> > +  Size = (sizeof (OPTEE_MSG_ARG) + sizeof (UINT64) - 1) & ~(sizeof (UINT64) - 1);
> > +  ParamShmAddr = OpteeShmInfo.Base + Size;
> > +  ShmSize = OpteeShmInfo.Size - Size;
> > +
> > +  for (Idx = 0; Idx < NumParams; Idx++) {
> > +    CONST OPTEE_MSG_PARAM    *Ip;
> > +    OPTEE_MSG_PARAM          *Mp;
> > +    UINT32                   Attr;
> > +
> > +    Ip = InParams + Idx;
> > +    Mp = MsgParams + Idx;
> > +    Attr = Ip->Attr & OPTEE_MSG_ATTR_TYPE_MASK;
> > +
> > +    switch (Attr) {
> > +    case OPTEE_MSG_ATTR_TYPE_NONE:
> > +      Mp->Attr = OPTEE_MSG_ATTR_TYPE_NONE;
> > +      ZeroMem (&Mp->U, sizeof (Mp->U));
> > +      break;
> > +
> > +    case OPTEE_MSG_ATTR_TYPE_VALUE_INPUT:
> > +    case OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT:
> > +    case OPTEE_MSG_ATTR_TYPE_VALUE_INOUT:
> > +      Mp->Attr = Attr;
> > +      Mp->U.Value.A = Ip->U.Value.A;
> > +      Mp->U.Value.B = Ip->U.Value.B;
> > +      Mp->U.Value.C = Ip->U.Value.C;
> > +      break;
> > +
> > +    case OPTEE_MSG_ATTR_TYPE_MEM_INPUT:
> > +    case OPTEE_MSG_ATTR_TYPE_MEM_OUTPUT:
> > +    case OPTEE_MSG_ATTR_TYPE_MEM_INOUT:
> > +      Mp->Attr = Attr;
> > +
> > +      if (Ip->U.Mem.Size > ShmSize) {
> > +        return EFI_OUT_OF_RESOURCES;
> > +      }
> > +
> > +      CopyMem ((VOID *)ParamShmAddr, (VOID *)Ip->U.Mem.BufPtr, Ip->U.Mem.Size);
> > +      Mp->U.Mem.BufPtr = (UINT64)ParamShmAddr;

While I don't think it's possible for this to end up doing something
unexpected, casting pointers to a higher alignment (which this would
do on 32-bit) is a bad habit to get into.

> > +      Mp->U.Mem.Size = Ip->U.Mem.Size;
> > +
> > +      Size = (Ip->U.Mem.Size + sizeof (UINT64) - 1) & ~(sizeof (UINT64) - 1);
> > +      ParamShmAddr += Size;
> > +      ShmSize -= Size;
> > +      break;
> > +
> > +    default:
> > +      return EFI_INVALID_PARAMETER;
> > +    }
> > +  }
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +STATIC
> > +EFI_STATUS
> > +OpteeFromMsgParam (
> > +  OUT OPTEE_MSG_PARAM    *OutParams,
> > +  IN UINT32              NumParams,
> > +  IN OPTEE_MSG_PARAM     *MsgParams
> > +  )
> > +{
> > +  UINT32                 Idx;
> > +
> > +  for (Idx = 0; Idx < NumParams; Idx++) {
> > +    OPTEE_MSG_PARAM          *Op;
> > +    CONST OPTEE_MSG_PARAM    *Mp;
> > +    UINT32                   Attr;
> > +
> > +    Op = OutParams + Idx;
> > +    Mp = MsgParams + Idx;
> > +    Attr = Mp->Attr & OPTEE_MSG_ATTR_TYPE_MASK;
> > +
> > +    switch (Attr) {
> > +    case OPTEE_MSG_ATTR_TYPE_NONE:
> > +      Op->Attr = OPTEE_MSG_ATTR_TYPE_NONE;
> > +      ZeroMem (&Op->U, sizeof (Op->U));
> > +      break;
> > +
> > +    case OPTEE_MSG_ATTR_TYPE_VALUE_INPUT:
> > +    case OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT:
> > +    case OPTEE_MSG_ATTR_TYPE_VALUE_INOUT:
> > +      Op->Attr = Attr;
> > +      Op->U.Value.A = Mp->U.Value.A;
> > +      Op->U.Value.B = Mp->U.Value.B;
> > +      Op->U.Value.C = Mp->U.Value.C;
> > +      break;
> > +
> > +    case OPTEE_MSG_ATTR_TYPE_MEM_INPUT:
> > +    case OPTEE_MSG_ATTR_TYPE_MEM_OUTPUT:
> > +    case OPTEE_MSG_ATTR_TYPE_MEM_INOUT:
> > +      Op->Attr = Attr;
> > +
> > +      if (Mp->U.Mem.Size > Op->U.Mem.Size) {
> > +        return EFI_BAD_BUFFER_SIZE;
> > +      }
> > +
> > +      CopyMem ((VOID *)Op->U.Mem.BufPtr, (VOID *)Mp->U.Mem.BufPtr, Mp->U.Mem.Size);
> > +      Op->U.Mem.Size = Mp->U.Mem.Size;
> > +      break;
> > +
> > +    default:
> > +      return EFI_INVALID_PARAMETER;
> > +    }
> > +  }
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +EFI_STATUS
> > +EFIAPI
> > +OpteeInvokeFunc (
> > +  IN OUT OPTEE_INVOKE_FUNC_ARG       *InvokeFuncArg
> > +  )
> > +{
> > +  EFI_STATUS       Status;
> > +  OPTEE_MSG_ARG    *MsgArg;
> > +
> > +  MsgArg = NULL;
> > +
> > +  if (OpteeShmInfo.Base == 0) {
> > +    DEBUG ((DEBUG_WARN, "OP-TEE not initialized\n"));
> > +    return EFI_NOT_STARTED;
> > +  }
> > +
> > +  MsgArg = (OPTEE_MSG_ARG *)OpteeShmInfo.Base;
> > +  ZeroMem (MsgArg, sizeof (OPTEE_MSG_ARG));
> > +
> > +  MsgArg->Cmd = OPTEE_MSG_CMD_INVOKE_COMMAND;
> > +  MsgArg->Func = InvokeFuncArg->Func;
> > +  MsgArg->Session = InvokeFuncArg->Session;
> > +
> > +  Status = OpteeToMsgParam (MsgArg->Params, MAX_PARAMS, InvokeFuncArg->Params);
> > +  if (Status)
> > +    return Status;

Always use {} (coding style requirement).

> > +
> > +  MsgArg->NumParams = MAX_PARAMS;
> > +
> > +  if (OpteeCallWithArg ((EFI_PHYSICAL_ADDRESS)MsgArg)) {
> > +    MsgArg->Ret = OPTEE_ERROR_COMMS;
> > +    MsgArg->RetOrigin = OPTEE_ORIGIN_COMMS;
> > +  }
> > +
> > +  if (OpteeFromMsgParam (InvokeFuncArg->Params, MAX_PARAMS, MsgArg->Params)) {
> > +    MsgArg->Ret = OPTEE_ERROR_COMMS;
> > +    MsgArg->RetOrigin = OPTEE_ORIGIN_COMMS;
> > +  }
> > +
> > +  InvokeFuncArg->Ret = MsgArg->Ret;
> > +  InvokeFuncArg->RetOrigin = MsgArg->RetOrigin;
> > +
> > +  return EFI_SUCCESS;
> > +}
> > diff --git a/ArmPkg/Library/OpteeLib/OpteeLib.inf b/ArmPkg/Library/OpteeLib/OpteeLib.inf
> > index 5abd427379cc..e03054a7167d 100644
> > --- a/ArmPkg/Library/OpteeLib/OpteeLib.inf
> > +++ b/ArmPkg/Library/OpteeLib/OpteeLib.inf
> > @@ -23,11 +23,13 @@ [Defines]
> >
> >  [Sources]
> >    Optee.c
> > +  OpteeSmc.h
> >
> >  [Packages]
> >    ArmPkg/ArmPkg.dec
> >    MdePkg/MdePkg.dec
> >
> >  [LibraryClasses]
> > +  ArmMmuLib
> >    ArmSmcLib
> >    BaseLib
> > diff --git a/ArmPkg/Library/OpteeLib/OpteeSmc.h b/ArmPkg/Library/OpteeLib/OpteeSmc.h
> > new file mode 100644
> > index 000000000000..e2ea35784a0a
> > --- /dev/null
> > +++ b/ArmPkg/Library/OpteeLib/OpteeSmc.h
> > @@ -0,0 +1,43 @@
> > +/** @file
> > +  OP-TEE SMC header file.
> > +
> > +  Copyright (c) 2018, Linaro Ltd. All rights reserved.<BR>
> > +
> > +  This program and the accompanying materials
> > +  are licensed and made available under the terms and conditions of the BSD License
> > +  which accompanies this distribution.  The full text of the license may be found at
> > +  http://opensource.org/licenses/bsd-license.php
> > +
> > +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> > +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> > +
> > +**/
> > +
> > +#ifndef _OPTEE_SMC_H_
> > +#define _OPTEE_SMC_H_
> > +
> > +/* Returned in Arg0 only from Trusted OS functions */
> > +#define OPTEE_SMC_RETURN_OK                     0x0
> > +
> > +#define OPTEE_SMC_RETURN_FROM_RPC               0x32000003
> > +#define OPTEE_SMC_CALL_WITH_ARG                 0x32000004
> > +#define OPTEE_SMC_GET_SHM_CONFIG                0xb2000007
> > +
> > +#define OPTEE_SMC_SHM_CACHED                    1
> > +
> > +#define OPTEE_SMC_RETURN_RPC_FOREIGN_INTR       0xffff0004
> > +
> > +#define OPTEE_MSG_CMD_OPEN_SESSION              0
> > +#define OPTEE_MSG_CMD_INVOKE_COMMAND            1
> > +#define OPTEE_MSG_CMD_CLOSE_SESSION             2
> > +
> > +#define OPTEE_MSG_ATTR_META                     0x100
> > +
> > +#define TEE_LOGIN_PUBLIC                        0x0

I'm a bit apprehensive about taking over the whole TEE namespace in an
implementation-specific header file.
Can/should this be OPTEE_?

> > +
> > +typedef struct {
> > +  UINTN    Base;
> > +  UINTN    Size;
> > +} OPTEE_SHARED_MEMORY_INFO;

(See comments above.)

/
    Leif

> > +
> > +#endif
> > --
> > 2.7.4
> >

Re: [PATCH v3 1/1] ArmPkg/OpteeLib: Add APIs to communicate with OP-TEE

[Sumit Garg]

On Fri, 5 Oct 2018 at 20:33, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Wed, Oct 03, 2018 at 11:33:01AM +0200, Ard Biesheuvel wrote:
> > On 3 October 2018 at 09:43, Sumit Garg <sumit.garg@linaro.org> wrote:
> > > Add following APIs to communicate with OP-TEE pseudo/early TAs:
> > > 1. OpteeInit
> > > 2. OpteeOpenSession
> > > 3. OpteeCloseSession
> > > 4. OpteeInvokeFunc
> > >
> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> >
> > Given the outcome of the GP discussion, I'm fine with this approach. Leif?
>
> Apologies for the delay, I needed some time to think it over.
>
> I'm not super happy about this approach, but I'm happier with this
> than I would be with leaving the functionality out of the upstream
> tree. I really hope we can get that license either changed or dropped
> completely.
>

Thanks Leif for this go ahead.

> I do have a few comments below.
>
> > > ---
> > >  ArmPkg/Include/Library/OpteeLib.h    |  90 +++++++++
> > >  ArmPkg/Library/OpteeLib/Optee.c      | 357 +++++++++++++++++++++++++++++++++++
> > >  ArmPkg/Library/OpteeLib/OpteeLib.inf |   2 +
> > >  ArmPkg/Library/OpteeLib/OpteeSmc.h   |  43 +++++
>
> Could you follow the instructions in
> https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-23
> when generating future patches?
> The --stat* effects aren't apparent here, but the -O ones are.
>

Sure.

> > >  4 files changed, 492 insertions(+)
> > >  create mode 100644 ArmPkg/Library/OpteeLib/OpteeSmc.h
> > >
> > > diff --git a/ArmPkg/Include/Library/OpteeLib.h b/ArmPkg/Include/Library/OpteeLib.h
> > > index f65d8674d9b8..2d1c60632dfe 100644
> > > --- a/ArmPkg/Include/Library/OpteeLib.h
> > > +++ b/ArmPkg/Include/Library/OpteeLib.h
> > > @@ -25,10 +25,100 @@
> > >  #define OPTEE_OS_UID2          0xaf630002
> > >  #define OPTEE_OS_UID3          0xa5d5c51b
> > >
> > > +#define OPTEE_MSG_ATTR_TYPE_NONE                0x0
> > > +#define OPTEE_MSG_ATTR_TYPE_VALUE_INPUT         0x1
> > > +#define OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT        0x2
> > > +#define OPTEE_MSG_ATTR_TYPE_VALUE_INOUT         0x3
> > > +#define OPTEE_MSG_ATTR_TYPE_MEM_INPUT           0x9
> > > +#define OPTEE_MSG_ATTR_TYPE_MEM_OUTPUT          0xa
> > > +#define OPTEE_MSG_ATTR_TYPE_MEM_INOUT           0xb
> > > +
> > > +#define OPTEE_MSG_ATTR_TYPE_MASK                0xff
> > > +
> > > +#define OPTEE_ORIGIN_COMMS                      0x00000002
> > > +#define OPTEE_ERROR_COMMS                       0xFFFF000E
> > > +
> > > +typedef struct {
> > > +  UINT64    BufPtr;
>
> If it's a pointer, it has a *.
> Otherwise it's an address.
>

Will rather use BufferAddress.

> > > +  UINT64    Size;
> > > +  UINT64    ShmRef;
>
> Abbreviations in function, variable or type names (other than the ones
> defined in [1] are not permitted unless they are explicitly added to a
> glossary section of the source file header. Where possible, just write
> out the name in full.
>
> [1] https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/v/release/2.20/4_naming_conventions/#table-2-efi-supported-abbreviations
>
> BufPtr (as a name) gets a pass since it's unambiguous and we already
> have a bunch of those in the codebase. ShmRef needs to be clear.
>

Will replace it with SharedMemoryReference.

> (This comment also applies to a lot of things below, I won't point
> them all out unless asked to.)
>

I will try to replace most of them with full names. But I have
confusion regarding usage of following not included in [1]:

1. MSG/Msg
2. MEM/Mem
3. INFO/Info
4. FUNC/Func
5. RET/Ret
6. ATTR/Attr
7. CMD/Cmd

But I do see their usage in the codebase. Please help to clarify.
Also can I use 1-2 letter variable or struct member names like Mp, Ip,
Op, U etc.?

> > > +} OPTEE_MSG_PARAM_MEM;
> > > +
> > > +typedef struct {
> > > +  UINT64    A;
> > > +  UINT64    B;
> > > +  UINT64    C;
> > > +} OPTEE_MSG_PARAM_VALUE;
> > > +
> > > +typedef struct {
> > > +  UINT64 Attr;
> > > +  union {
> > > +    OPTEE_MSG_PARAM_MEM      Mem;
> > > +    OPTEE_MSG_PARAM_VALUE    Value;
> > > +  } U;
> > > +} OPTEE_MSG_PARAM;
> > > +
> > > +#define MAX_PARAMS           4
>
> This is a very localised macro with a very globalised name.
> Suggest adding an OPTEE_ prefix, but also something describing what it
> is the maximum parameters for. CALL_?
>

Ok will rename it to OPTEE_CALL_MAX_PARAMS.

> > > +
> > > +typedef struct {
> > > +        UINT32             Cmd;
> > > +        UINT32             Func;
> > > +        UINT32             Session;
> > > +        UINT32             CancelId;
> > > +        UINT32             Pad;
> > > +        UINT32             Ret;
> > > +        UINT32             RetOrigin;
> > > +        UINT32             NumParams;
> > > +
> > > +        // NumParams tells the actual number of element in Params
> > > +        OPTEE_MSG_PARAM    Params[MAX_PARAMS];
> > > +} OPTEE_MSG_ARG;
> > > +
> > > +#define OPTEE_UUID_LEN       16
>
> UUIDs are UUIDs. If optee decides on an incompatible format, we may
> have an interoperability issue. I assume this is not the case, so
> perhaps replace with sizeof (EFI_GUID) at each point of use?
>

Agree will use sizeof (EFI_GUID) instead.

> > > +
> > > +typedef struct {
> > > +        UINT8     Uuid[OPTEE_UUID_LEN]; // [in] UUID of the Trusted Application
>
> Is there a strong reason for not using EFI_GUID here?
>

EFI_GUID can be used here as UUID are also known as GUID. But here we
pass UUID as 16 octets to OP-TEE. So will use EFI_GUID instead and add
an api to convert uuid to octets.

> > > +        UINT32    Session;              // [out] Session id
> > > +        UINT32    Ret;                  // [out] Return value
> > > +        UINT32    RetOrigin;            // [out] Origin of the return value
> > > +} OPTEE_OPEN_SESSION_ARG;
> > > +
> > > +typedef struct {
> > > +        UINT32             Func;    // [in] Trusted App func, specific to the TA
> > > +        UINT32             Session; // [in] Session id
> > > +        UINT32             Ret;     // [out] Return value
> > > +        UINT32             RetOrigin; // [out] Origin of the return value
> > > +        OPTEE_MSG_PARAM    Params[MAX_PARAMS]; // Params for func to be invoked
> > > +} OPTEE_INVOKE_FUNC_ARG;
> > > +
> > >  BOOLEAN
> > >  EFIAPI
> > >  IsOpteePresent (
> > >    VOID
> > >    );
> > >
> > > +EFI_STATUS
> > > +EFIAPI
> > > +OpteeInit (
> > > +  VOID
> > > +  );
> > > +
> > > +EFI_STATUS
> > > +EFIAPI
> > > +OpteeOpenSession (
> > > +  IN OUT OPTEE_OPEN_SESSION_ARG      *OpenSessionArg
> > > +  );
> > > +
> > > +EFI_STATUS
> > > +EFIAPI
> > > +OpteeCloseSession (
> > > +  IN UINT32                      Session
> > > +  );
> > > +
> > > +EFI_STATUS
> > > +EFIAPI
> > > +OpteeInvokeFunc (
> > > +  IN OUT OPTEE_INVOKE_FUNC_ARG       *InvokeFuncArg
> > > +  );
> > > +
> > >  #endif
> > > diff --git a/ArmPkg/Library/OpteeLib/Optee.c b/ArmPkg/Library/OpteeLib/Optee.c
> > > index 574527f8b5ea..bf7872cbbce0 100644
> > > --- a/ArmPkg/Library/OpteeLib/Optee.c
> > > +++ b/ArmPkg/Library/OpteeLib/Optee.c
> > > @@ -14,11 +14,18 @@
> > >
> > >  **/
> > >
> > > +#include <Library/ArmMmuLib.h>
> > >  #include <Library/ArmSmcLib.h>
> > > +#include <Library/BaseMemoryLib.h>
> > >  #include <Library/BaseLib.h>
> > > +#include <Library/DebugLib.h>
> > >  #include <Library/OpteeLib.h>
> > >
> > >  #include <IndustryStandard/ArmStdSmc.h>
> > > +#include <OpteeSmc.h>
> > > +#include <Uefi.h>
> > > +
> > > +STATIC OPTEE_SHARED_MEMORY_INFO OpteeShmInfo = { 0 };
> > >
> > >  /**
> > >    Check for OP-TEE presence.
> > > @@ -31,6 +38,7 @@ IsOpteePresent (
> > >  {
> > >    ARM_SMC_ARGS ArmSmcArgs;
> > >
> > > +  ZeroMem (&ArmSmcArgs, sizeof (ARM_SMC_ARGS));
> > >    // Send a Trusted OS Calls UID command
> > >    ArmSmcArgs.Arg0 = ARM_SMC_ID_TOS_UID;
> > >    ArmCallSmc (&ArmSmcArgs);
> > > @@ -44,3 +52,352 @@ IsOpteePresent (
> > >      return FALSE;
> > >    }
> > >  }
> > > +
> > > +STATIC
> > > +EFI_STATUS
> > > +OpteeShmMemRemap (
> > > +  VOID
> > > +  )
> > > +{
> > > +  ARM_SMC_ARGS                 ArmSmcArgs;
> > > +  EFI_PHYSICAL_ADDRESS         Paddr;
> > > +  EFI_PHYSICAL_ADDRESS         Start;
> > > +  EFI_PHYSICAL_ADDRESS         End;
> > > +  EFI_STATUS                   Status;
> > > +  UINTN                        Size;
> > > +
> > > +  ZeroMem (&ArmSmcArgs, sizeof (ARM_SMC_ARGS));
> > > +  ArmSmcArgs.Arg0 = OPTEE_SMC_GET_SHM_CONFIG;
> > > +
> > > +  ArmCallSmc (&ArmSmcArgs);
> > > +  if (ArmSmcArgs.Arg0 != OPTEE_SMC_RETURN_OK) {
> > > +    DEBUG ((DEBUG_WARN, "OP-TEE shared memory not supported\n"));
> > > +    return EFI_UNSUPPORTED;
> > > +  }
> > > +
> > > +  if (ArmSmcArgs.Arg3 != OPTEE_SMC_SHM_CACHED) {
> > > +    DEBUG ((DEBUG_WARN, "OP-TEE: Only normal cached shared memory supported\n"));
> > > +    return EFI_UNSUPPORTED;
> > > +  }
> > > +
> > > +  Start = (ArmSmcArgs.Arg1 + SIZE_4KB - 1) & ~(SIZE_4KB - 1);
> > > +  End = (ArmSmcArgs.Arg1 + ArmSmcArgs.Arg2) & ~(SIZE_4KB - 1);
> > > +  Paddr = Start;
> > > +  Size = End - Start;
> > > +
> > > +  if (Size < SIZE_4KB) {
> > > +    DEBUG ((DEBUG_WARN, "OP-TEE shared memory too small\n"));
> > > +    return EFI_BUFFER_TOO_SMALL;
> > > +  }
> > > +
> > > +  Status = ArmSetMemoryAttributes (Paddr, Size, EFI_MEMORY_WB);
> > > +  if (EFI_ERROR (Status)) {
> > > +    return Status;
> > > +  }
> > > +
> > > +  OpteeShmInfo.Base = (UINTN)Paddr;
>
> Would PHYSICAL_ADDRESS (always 64-bit) be more correct here, or is the
> OP-TEE end seeing this struct as always having fields of native width?
>

Its a global struct not shared with OP-TEE. Used native width to have
compatibility with 32-bit system.

> (This is where the ordefile comes in handy, because it means I see the
> struct definition first, not its use.)
>
> > > +  OpteeShmInfo.Size = Size;
> > > +
> > > +  return EFI_SUCCESS;
> > > +}
> > > +
> > > +EFI_STATUS
> > > +EFIAPI
> > > +OpteeInit (
> > > +  VOID
> > > +  )
> > > +{
> > > +  EFI_STATUS      Status;
> > > +
> > > +  if (!IsOpteePresent ()) {
> > > +    DEBUG ((DEBUG_WARN, "OP-TEE not present\n"));
> > > +    return EFI_UNSUPPORTED;
> > > +  }
> > > +
> > > +  Status = OpteeShmMemRemap ();
> > > +  if (EFI_ERROR (Status)) {
> > > +    DEBUG ((DEBUG_WARN, "OP-TEE shared memory remap failed\n"));
> > > +    return Status;
> > > +  }
> > > +
> > > +  return EFI_SUCCESS;
> > > +}
> > > +
> > > +/**
> > > +  Does Standard SMC to OP-TEE in secure world.
> > > +
> > > +  @param[in]  Parg   Physical address of message to pass to secure world
> > > +
> > > +  @return            0 on success, secure world return code otherwise
> > > +
> > > +**/
> > > +STATIC
> > > +UINT32
> > > +OpteeCallWithArg (
> > > +  IN EFI_PHYSICAL_ADDRESS Parg
> > > +  )
> > > +{
> > > +  ARM_SMC_ARGS ArmSmcArgs;
> > > +
> > > +  ZeroMem (&ArmSmcArgs, sizeof (ARM_SMC_ARGS));
> > > +  ArmSmcArgs.Arg0 = OPTEE_SMC_CALL_WITH_ARG;
> > > +  ArmSmcArgs.Arg1 = (UINT32)(Parg >> 32);
> > > +  ArmSmcArgs.Arg2 = (UINT32)Parg;
> > > +
> > > +  while (TRUE) {
> > > +    ArmCallSmc (&ArmSmcArgs);
> > > +
> > > +    if (ArmSmcArgs.Arg0 == OPTEE_SMC_RETURN_RPC_FOREIGN_INTR) {
> > > +      //
> > > +      // A foreign interrupt was raised while secure world was
> > > +      // executing, since they are handled in UEFI a dummy RPC is
> > > +      // performed to let UEFI take the interrupt through the normal
> > > +      // vector.
> > > +      //
> > > +      ArmSmcArgs.Arg0 = OPTEE_SMC_RETURN_FROM_RPC;
> > > +    } else {
> > > +      break;
> > > +    }
> > > +  }
> > > +
> > > +  return ArmSmcArgs.Arg0;
> > > +}
> > > +
> > > +EFI_STATUS
> > > +EFIAPI
> > > +OpteeOpenSession (
> > > +  IN OUT OPTEE_OPEN_SESSION_ARG      *OpenSessionArg
> > > +  )
> > > +{
> > > +  OPTEE_MSG_ARG    *MsgArg;
> > > +
> > > +  MsgArg = NULL;
> > > +
> > > +  if (OpteeShmInfo.Base == 0) {
> > > +    DEBUG ((DEBUG_WARN, "OP-TEE not initialized\n"));
> > > +    return EFI_NOT_STARTED;
> > > +  }
> > > +
> > > +  MsgArg = (OPTEE_MSG_ARG *)OpteeShmInfo.Base;
> > > +  ZeroMem (MsgArg, sizeof (OPTEE_MSG_ARG));
> > > +
> > > +  MsgArg->Cmd = OPTEE_MSG_CMD_OPEN_SESSION;
> > > +
> > > +  //
> > > +  // Initialize and add the meta parameters needed when opening a
> > > +  // session.
> > > +  //
> > > +  MsgArg->Params[0].Attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT |
> > > +                           OPTEE_MSG_ATTR_META;
> > > +  MsgArg->Params[1].Attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT |
> > > +                           OPTEE_MSG_ATTR_META;
> > > +  CopyMem (&MsgArg->Params[0].U.Value, OpenSessionArg->Uuid, OPTEE_UUID_LEN);
> > > +  ZeroMem (&MsgArg->Params[1].U.Value, OPTEE_UUID_LEN);
> > > +  MsgArg->Params[1].U.Value.C = TEE_LOGIN_PUBLIC;
> > > +
> > > +  MsgArg->NumParams = 2;
> > > +
> > > +  if (OpteeCallWithArg ((EFI_PHYSICAL_ADDRESS)MsgArg)) {
> > > +    MsgArg->Ret = OPTEE_ERROR_COMMS;
> > > +    MsgArg->RetOrigin = OPTEE_ORIGIN_COMMS;
> > > +  }
> > > +
> > > +  OpenSessionArg->Session = MsgArg->Session;
> > > +  OpenSessionArg->Ret = MsgArg->Ret;
> > > +  OpenSessionArg->RetOrigin = MsgArg->RetOrigin;
> > > +
> > > +  return EFI_SUCCESS;
> > > +}
> > > +
> > > +EFI_STATUS
> > > +EFIAPI
> > > +OpteeCloseSession (
> > > +  IN UINT32                      Session
> > > +  )
> > > +{
> > > +  OPTEE_MSG_ARG    *MsgArg;
> > > +
> > > +  MsgArg = NULL;
> > > +
> > > +  if (OpteeShmInfo.Base == 0) {
> > > +    DEBUG ((DEBUG_WARN, "OP-TEE not initialized\n"));
> > > +    return EFI_NOT_STARTED;
> > > +  }
> > > +
> > > +  MsgArg = (OPTEE_MSG_ARG *)OpteeShmInfo.Base;
> > > +  ZeroMem (MsgArg, sizeof (OPTEE_MSG_ARG));
> > > +
> > > +  MsgArg->Cmd = OPTEE_MSG_CMD_CLOSE_SESSION;
> > > +  MsgArg->Session = Session;
> > > +
> > > +  OpteeCallWithArg ((EFI_PHYSICAL_ADDRESS)MsgArg);
> > > +
> > > +  return EFI_SUCCESS;
> > > +}
> > > +
> > > +STATIC
> > > +EFI_STATUS
> > > +OpteeToMsgParam (
> > > +  OUT OPTEE_MSG_PARAM    *MsgParams,
> > > +  IN UINT32              NumParams,
> > > +  IN OPTEE_MSG_PARAM     *InParams
> > > +  )
> > > +{
> > > +  UINT32                  Idx;
> > > +  UINTN                   ParamShmAddr;
> > > +  UINTN                   ShmSize;
> > > +  UINTN                   Size;
> > > +
> > > +  Size = (sizeof (OPTEE_MSG_ARG) + sizeof (UINT64) - 1) & ~(sizeof (UINT64) - 1);
> > > +  ParamShmAddr = OpteeShmInfo.Base + Size;
> > > +  ShmSize = OpteeShmInfo.Size - Size;
> > > +
> > > +  for (Idx = 0; Idx < NumParams; Idx++) {
> > > +    CONST OPTEE_MSG_PARAM    *Ip;
> > > +    OPTEE_MSG_PARAM          *Mp;
> > > +    UINT32                   Attr;
> > > +
> > > +    Ip = InParams + Idx;
> > > +    Mp = MsgParams + Idx;
> > > +    Attr = Ip->Attr & OPTEE_MSG_ATTR_TYPE_MASK;
> > > +
> > > +    switch (Attr) {
> > > +    case OPTEE_MSG_ATTR_TYPE_NONE:
> > > +      Mp->Attr = OPTEE_MSG_ATTR_TYPE_NONE;
> > > +      ZeroMem (&Mp->U, sizeof (Mp->U));
> > > +      break;
> > > +
> > > +    case OPTEE_MSG_ATTR_TYPE_VALUE_INPUT:
> > > +    case OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT:
> > > +    case OPTEE_MSG_ATTR_TYPE_VALUE_INOUT:
> > > +      Mp->Attr = Attr;
> > > +      Mp->U.Value.A = Ip->U.Value.A;
> > > +      Mp->U.Value.B = Ip->U.Value.B;
> > > +      Mp->U.Value.C = Ip->U.Value.C;
> > > +      break;
> > > +
> > > +    case OPTEE_MSG_ATTR_TYPE_MEM_INPUT:
> > > +    case OPTEE_MSG_ATTR_TYPE_MEM_OUTPUT:
> > > +    case OPTEE_MSG_ATTR_TYPE_MEM_INOUT:
> > > +      Mp->Attr = Attr;
> > > +
> > > +      if (Ip->U.Mem.Size > ShmSize) {
> > > +        return EFI_OUT_OF_RESOURCES;
> > > +      }
> > > +
> > > +      CopyMem ((VOID *)ParamShmAddr, (VOID *)Ip->U.Mem.BufPtr, Ip->U.Mem.Size);
> > > +      Mp->U.Mem.BufPtr = (UINT64)ParamShmAddr;
>
> While I don't think it's possible for this to end up doing something
> unexpected, casting pointers to a higher alignment (which this would
> do on 32-bit) is a bad habit to get into.
>

This is a shared message param structure with OP-TEE. So needed to cast it.

> > > +      Mp->U.Mem.Size = Ip->U.Mem.Size;
> > > +
> > > +      Size = (Ip->U.Mem.Size + sizeof (UINT64) - 1) & ~(sizeof (UINT64) - 1);
> > > +      ParamShmAddr += Size;
> > > +      ShmSize -= Size;
> > > +      break;
> > > +
> > > +    default:
> > > +      return EFI_INVALID_PARAMETER;
> > > +    }
> > > +  }
> > > +
> > > +  return EFI_SUCCESS;
> > > +}
> > > +
> > > +STATIC
> > > +EFI_STATUS
> > > +OpteeFromMsgParam (
> > > +  OUT OPTEE_MSG_PARAM    *OutParams,
> > > +  IN UINT32              NumParams,
> > > +  IN OPTEE_MSG_PARAM     *MsgParams
> > > +  )
> > > +{
> > > +  UINT32                 Idx;
> > > +
> > > +  for (Idx = 0; Idx < NumParams; Idx++) {
> > > +    OPTEE_MSG_PARAM          *Op;
> > > +    CONST OPTEE_MSG_PARAM    *Mp;
> > > +    UINT32                   Attr;
> > > +
> > > +    Op = OutParams + Idx;
> > > +    Mp = MsgParams + Idx;
> > > +    Attr = Mp->Attr & OPTEE_MSG_ATTR_TYPE_MASK;
> > > +
> > > +    switch (Attr) {
> > > +    case OPTEE_MSG_ATTR_TYPE_NONE:
> > > +      Op->Attr = OPTEE_MSG_ATTR_TYPE_NONE;
> > > +      ZeroMem (&Op->U, sizeof (Op->U));
> > > +      break;
> > > +
> > > +    case OPTEE_MSG_ATTR_TYPE_VALUE_INPUT:
> > > +    case OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT:
> > > +    case OPTEE_MSG_ATTR_TYPE_VALUE_INOUT:
> > > +      Op->Attr = Attr;
> > > +      Op->U.Value.A = Mp->U.Value.A;
> > > +      Op->U.Value.B = Mp->U.Value.B;
> > > +      Op->U.Value.C = Mp->U.Value.C;
> > > +      break;
> > > +
> > > +    case OPTEE_MSG_ATTR_TYPE_MEM_INPUT:
> > > +    case OPTEE_MSG_ATTR_TYPE_MEM_OUTPUT:
> > > +    case OPTEE_MSG_ATTR_TYPE_MEM_INOUT:
> > > +      Op->Attr = Attr;
> > > +
> > > +      if (Mp->U.Mem.Size > Op->U.Mem.Size) {
> > > +        return EFI_BAD_BUFFER_SIZE;
> > > +      }
> > > +
> > > +      CopyMem ((VOID *)Op->U.Mem.BufPtr, (VOID *)Mp->U.Mem.BufPtr, Mp->U.Mem.Size);
> > > +      Op->U.Mem.Size = Mp->U.Mem.Size;
> > > +      break;
> > > +
> > > +    default:
> > > +      return EFI_INVALID_PARAMETER;
> > > +    }
> > > +  }
> > > +
> > > +  return EFI_SUCCESS;
> > > +}
> > > +
> > > +EFI_STATUS
> > > +EFIAPI
> > > +OpteeInvokeFunc (
> > > +  IN OUT OPTEE_INVOKE_FUNC_ARG       *InvokeFuncArg
> > > +  )
> > > +{
> > > +  EFI_STATUS       Status;
> > > +  OPTEE_MSG_ARG    *MsgArg;
> > > +
> > > +  MsgArg = NULL;
> > > +
> > > +  if (OpteeShmInfo.Base == 0) {
> > > +    DEBUG ((DEBUG_WARN, "OP-TEE not initialized\n"));
> > > +    return EFI_NOT_STARTED;
> > > +  }
> > > +
> > > +  MsgArg = (OPTEE_MSG_ARG *)OpteeShmInfo.Base;
> > > +  ZeroMem (MsgArg, sizeof (OPTEE_MSG_ARG));
> > > +
> > > +  MsgArg->Cmd = OPTEE_MSG_CMD_INVOKE_COMMAND;
> > > +  MsgArg->Func = InvokeFuncArg->Func;
> > > +  MsgArg->Session = InvokeFuncArg->Session;
> > > +
> > > +  Status = OpteeToMsgParam (MsgArg->Params, MAX_PARAMS, InvokeFuncArg->Params);
> > > +  if (Status)
> > > +    return Status;
>
> Always use {} (coding style requirement).
>

Ok.

> > > +
> > > +  MsgArg->NumParams = MAX_PARAMS;
> > > +
> > > +  if (OpteeCallWithArg ((EFI_PHYSICAL_ADDRESS)MsgArg)) {
> > > +    MsgArg->Ret = OPTEE_ERROR_COMMS;
> > > +    MsgArg->RetOrigin = OPTEE_ORIGIN_COMMS;
> > > +  }
> > > +
> > > +  if (OpteeFromMsgParam (InvokeFuncArg->Params, MAX_PARAMS, MsgArg->Params)) {
> > > +    MsgArg->Ret = OPTEE_ERROR_COMMS;
> > > +    MsgArg->RetOrigin = OPTEE_ORIGIN_COMMS;
> > > +  }
> > > +
> > > +  InvokeFuncArg->Ret = MsgArg->Ret;
> > > +  InvokeFuncArg->RetOrigin = MsgArg->RetOrigin;
> > > +
> > > +  return EFI_SUCCESS;
> > > +}
> > > diff --git a/ArmPkg/Library/OpteeLib/OpteeLib.inf b/ArmPkg/Library/OpteeLib/OpteeLib.inf
> > > index 5abd427379cc..e03054a7167d 100644
> > > --- a/ArmPkg/Library/OpteeLib/OpteeLib.inf
> > > +++ b/ArmPkg/Library/OpteeLib/OpteeLib.inf
> > > @@ -23,11 +23,13 @@ [Defines]
> > >
> > >  [Sources]
> > >    Optee.c
> > > +  OpteeSmc.h
> > >
> > >  [Packages]
> > >    ArmPkg/ArmPkg.dec
> > >    MdePkg/MdePkg.dec
> > >
> > >  [LibraryClasses]
> > > +  ArmMmuLib
> > >    ArmSmcLib
> > >    BaseLib
> > > diff --git a/ArmPkg/Library/OpteeLib/OpteeSmc.h b/ArmPkg/Library/OpteeLib/OpteeSmc.h
> > > new file mode 100644
> > > index 000000000000..e2ea35784a0a
> > > --- /dev/null
> > > +++ b/ArmPkg/Library/OpteeLib/OpteeSmc.h
> > > @@ -0,0 +1,43 @@
> > > +/** @file
> > > +  OP-TEE SMC header file.
> > > +
> > > +  Copyright (c) 2018, Linaro Ltd. All rights reserved.<BR>
> > > +
> > > +  This program and the accompanying materials
> > > +  are licensed and made available under the terms and conditions of the BSD License
> > > +  which accompanies this distribution.  The full text of the license may be found at
> > > +  http://opensource.org/licenses/bsd-license.php
> > > +
> > > +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> > > +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> > > +
> > > +**/
> > > +
> > > +#ifndef _OPTEE_SMC_H_
> > > +#define _OPTEE_SMC_H_
> > > +
> > > +/* Returned in Arg0 only from Trusted OS functions */
> > > +#define OPTEE_SMC_RETURN_OK                     0x0
> > > +
> > > +#define OPTEE_SMC_RETURN_FROM_RPC               0x32000003
> > > +#define OPTEE_SMC_CALL_WITH_ARG                 0x32000004
> > > +#define OPTEE_SMC_GET_SHM_CONFIG                0xb2000007
> > > +
> > > +#define OPTEE_SMC_SHM_CACHED                    1
> > > +
> > > +#define OPTEE_SMC_RETURN_RPC_FOREIGN_INTR       0xffff0004
> > > +
> > > +#define OPTEE_MSG_CMD_OPEN_SESSION              0
> > > +#define OPTEE_MSG_CMD_INVOKE_COMMAND            1
> > > +#define OPTEE_MSG_CMD_CLOSE_SESSION             2
> > > +
> > > +#define OPTEE_MSG_ATTR_META                     0x100
> > > +
> > > +#define TEE_LOGIN_PUBLIC                        0x0
>
> I'm a bit apprehensive about taking over the whole TEE namespace in an
> implementation-specific header file.
> Can/should this be OPTEE_?
>

Ok will change this to OPTEE_.

-Sumit

> > > +
> > > +typedef struct {
> > > +  UINTN    Base;
> > > +  UINTN    Size;
> > > +} OPTEE_SHARED_MEMORY_INFO;
>
> (See comments above.)
>
> /
>     Leif
>
> > > +
> > > +#endif
> > > --
> > > 2.7.4
> > >

Re: [PATCH v3 1/1] ArmPkg/OpteeLib: Add APIs to communicate with OP-TEE

[Leif Lindholm]

On Mon, Oct 08, 2018 at 03:20:27PM +0530, Sumit Garg wrote:
> On Fri, 5 Oct 2018 at 20:33, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >
> > On Wed, Oct 03, 2018 at 11:33:01AM +0200, Ard Biesheuvel wrote:
> > > On 3 October 2018 at 09:43, Sumit Garg <sumit.garg@linaro.org> wrote:
> > > > Add following APIs to communicate with OP-TEE pseudo/early TAs:
> > > > 1. OpteeInit
> > > > 2. OpteeOpenSession
> > > > 3. OpteeCloseSession
> > > > 4. OpteeInvokeFunc
> > > >
> > > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > >
> > > Given the outcome of the GP discussion, I'm fine with this approach. Leif?
> >
> > Apologies for the delay, I needed some time to think it over.
> >
> > I'm not super happy about this approach, but I'm happier with this
> > than I would be with leaving the functionality out of the upstream
> > tree. I really hope we can get that license either changed or dropped
> > completely.
> >
> 
> Thanks Leif for this go ahead.
> 
> > I do have a few comments below.
> >
> > > > ---
> > > >  ArmPkg/Include/Library/OpteeLib.h    |  90 +++++++++
> > > >  ArmPkg/Library/OpteeLib/Optee.c      | 357 +++++++++++++++++++++++++++++++++++
> > > >  ArmPkg/Library/OpteeLib/OpteeLib.inf |   2 +
> > > >  ArmPkg/Library/OpteeLib/OpteeSmc.h   |  43 +++++
> >
> > Could you follow the instructions in
> > https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-23
> > when generating future patches?
> > The --stat* effects aren't apparent here, but the -O ones are.
> >
> 
> Sure.
> 
> > > >  4 files changed, 492 insertions(+)
> > > >  create mode 100644 ArmPkg/Library/OpteeLib/OpteeSmc.h
> > > >
> > > > diff --git a/ArmPkg/Include/Library/OpteeLib.h b/ArmPkg/Include/Library/OpteeLib.h
> > > > index f65d8674d9b8..2d1c60632dfe 100644
> > > > --- a/ArmPkg/Include/Library/OpteeLib.h
> > > > +++ b/ArmPkg/Include/Library/OpteeLib.h
> > > > @@ -25,10 +25,100 @@
> > > >  #define OPTEE_OS_UID2          0xaf630002
> > > >  #define OPTEE_OS_UID3          0xa5d5c51b
> > > >
> > > > +#define OPTEE_MSG_ATTR_TYPE_NONE                0x0
> > > > +#define OPTEE_MSG_ATTR_TYPE_VALUE_INPUT         0x1
> > > > +#define OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT        0x2
> > > > +#define OPTEE_MSG_ATTR_TYPE_VALUE_INOUT         0x3
> > > > +#define OPTEE_MSG_ATTR_TYPE_MEM_INPUT           0x9
> > > > +#define OPTEE_MSG_ATTR_TYPE_MEM_OUTPUT          0xa
> > > > +#define OPTEE_MSG_ATTR_TYPE_MEM_INOUT           0xb
> > > > +
> > > > +#define OPTEE_MSG_ATTR_TYPE_MASK                0xff
> > > > +
> > > > +#define OPTEE_ORIGIN_COMMS                      0x00000002
> > > > +#define OPTEE_ERROR_COMMS                       0xFFFF000E
> > > > +
> > > > +typedef struct {
> > > > +  UINT64    BufPtr;
> >
> > If it's a pointer, it has a *.
> > Otherwise it's an address.
> >
> 
> Will rather use BufferAddress.
> 
> > > > +  UINT64    Size;
> > > > +  UINT64    ShmRef;
> >
> > Abbreviations in function, variable or type names (other than the ones
> > defined in [1] are not permitted unless they are explicitly added to a
> > glossary section of the source file header. Where possible, just write
> > out the name in full.
> >
> > [1] https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/v/release/2.20/4_naming_conventions/#table-2-efi-supported-abbreviations
> >
> > BufPtr (as a name) gets a pass since it's unambiguous and we already
> > have a bunch of those in the codebase. ShmRef needs to be clear.
> >
> 
> Will replace it with SharedMemoryReference.
> 
> > (This comment also applies to a lot of things below, I won't point
> > them all out unless asked to.)
> >
> 
> I will try to replace most of them with full names. But I have
> confusion regarding usage of following not included in [1]:
> 
> 1. MSG/Msg
> 2. MEM/Mem
> 3. INFO/Info
> 4. FUNC/Func
> 5. RET/Ret
> 6. ATTR/Attr
> 7. CMD/Cmd
> 
> But I do see their usage in the codebase. Please help to clarify.

It's a horrible secret, but not all of the codebase conforms to the
coding style. We're continuously trying to improve it.

> Also can I use 1-2 letter variable or struct member names like Mp, Ip,
> Op, U etc.?

Ideally not. I'm not a huge fan of ReallyLongCamelCase coding styles,
but the benefit it does bring is that you don't need to stop and think
about what a variable may be for. And correspondingly, mixing between
abbreviations an non-abbreviations make for hard reading.

Another completely unscientific observation I would make is that once
you force yourself to write a full variable name out, it becomes a lot
more clear when you've left valuable semantic information out.
The codebase ends up with a bunch of fewer variables called only
'Msg', 'Mem', 'Info', Func', 'Ret', 'Attr' 'Cmd' (, or 'Data' ...).
(Alternatively, the mailing list ends up with a lot fewer emails
asking "what message", "what memory", "what information" ...)

> > > > +} OPTEE_MSG_PARAM_MEM;
> > > > +
> > > > +typedef struct {
> > > > +  UINT64    A;
> > > > +  UINT64    B;
> > > > +  UINT64    C;
> > > > +} OPTEE_MSG_PARAM_VALUE;
> > > > +
> > > > +typedef struct {
> > > > +  UINT64 Attr;
> > > > +  union {
> > > > +    OPTEE_MSG_PARAM_MEM      Mem;
> > > > +    OPTEE_MSG_PARAM_VALUE    Value;
> > > > +  } U;
> > > > +} OPTEE_MSG_PARAM;
> > > > +
> > > > +#define MAX_PARAMS           4
> >
> > This is a very localised macro with a very globalised name.
> > Suggest adding an OPTEE_ prefix, but also something describing what it
> > is the maximum parameters for. CALL_?
> >
> 
> Ok will rename it to OPTEE_CALL_MAX_PARAMS.

I'd take that or OPTEE_MAX_CALL_PARAMS.

> > > > +
> > > > +typedef struct {
> > > > +        UINT32             Cmd;
> > > > +        UINT32             Func;
> > > > +        UINT32             Session;
> > > > +        UINT32             CancelId;
> > > > +        UINT32             Pad;
> > > > +        UINT32             Ret;
> > > > +        UINT32             RetOrigin;
> > > > +        UINT32             NumParams;
> > > > +
> > > > +        // NumParams tells the actual number of element in Params
> > > > +        OPTEE_MSG_PARAM    Params[MAX_PARAMS];
> > > > +} OPTEE_MSG_ARG;
> > > > +
> > > > +#define OPTEE_UUID_LEN       16
> >
> > UUIDs are UUIDs. If optee decides on an incompatible format, we may
> > have an interoperability issue. I assume this is not the case, so
> > perhaps replace with sizeof (EFI_GUID) at each point of use?
> 
> Agree will use sizeof (EFI_GUID) instead.
> 
> > > > +
> > > > +typedef struct {
> > > > +        UINT8     Uuid[OPTEE_UUID_LEN]; // [in] UUID of the Trusted Application
> >
> > Is there a strong reason for not using EFI_GUID here?
> >
> 
> EFI_GUID can be used here as UUID are also known as GUID. But here we
> pass UUID as 16 octets to OP-TEE. So will use EFI_GUID instead and add
> an api to convert uuid to octets.

Right. That may be tedious, but I think it will be an improvement.
If OPTEE uses UUIDs without the Microsoft timestamp endianness variant
(as described in the UEFI spec), then ideally those functions could
byte reverse the timestamp as well.

> > > > +        UINT32    Session;              // [out] Session id
> > > > +        UINT32    Ret;                  // [out] Return value
> > > > +        UINT32    RetOrigin;            // [out] Origin of the return value
> > > > +} OPTEE_OPEN_SESSION_ARG;
> > > > +
> > > > +typedef struct {
> > > > +        UINT32             Func;    // [in] Trusted App func, specific to the TA
> > > > +        UINT32             Session; // [in] Session id
> > > > +        UINT32             Ret;     // [out] Return value
> > > > +        UINT32             RetOrigin; // [out] Origin of the return value
> > > > +        OPTEE_MSG_PARAM    Params[MAX_PARAMS]; // Params for func to be invoked
> > > > +} OPTEE_INVOKE_FUNC_ARG;
> > > > +
> > > >  BOOLEAN
> > > >  EFIAPI
> > > >  IsOpteePresent (
> > > >    VOID
> > > >    );
> > > >
> > > > +EFI_STATUS
> > > > +EFIAPI
> > > > +OpteeInit (
> > > > +  VOID
> > > > +  );
> > > > +
> > > > +EFI_STATUS
> > > > +EFIAPI
> > > > +OpteeOpenSession (
> > > > +  IN OUT OPTEE_OPEN_SESSION_ARG      *OpenSessionArg
> > > > +  );
> > > > +
> > > > +EFI_STATUS
> > > > +EFIAPI
> > > > +OpteeCloseSession (
> > > > +  IN UINT32                      Session
> > > > +  );
> > > > +
> > > > +EFI_STATUS
> > > > +EFIAPI
> > > > +OpteeInvokeFunc (
> > > > +  IN OUT OPTEE_INVOKE_FUNC_ARG       *InvokeFuncArg
> > > > +  );
> > > > +
> > > >  #endif
> > > > diff --git a/ArmPkg/Library/OpteeLib/Optee.c b/ArmPkg/Library/OpteeLib/Optee.c
> > > > index 574527f8b5ea..bf7872cbbce0 100644
> > > > --- a/ArmPkg/Library/OpteeLib/Optee.c
> > > > +++ b/ArmPkg/Library/OpteeLib/Optee.c
> > > > @@ -14,11 +14,18 @@
> > > >
> > > >  **/
> > > >
> > > > +#include <Library/ArmMmuLib.h>
> > > >  #include <Library/ArmSmcLib.h>
> > > > +#include <Library/BaseMemoryLib.h>
> > > >  #include <Library/BaseLib.h>
> > > > +#include <Library/DebugLib.h>
> > > >  #include <Library/OpteeLib.h>
> > > >
> > > >  #include <IndustryStandard/ArmStdSmc.h>
> > > > +#include <OpteeSmc.h>
> > > > +#include <Uefi.h>
> > > > +
> > > > +STATIC OPTEE_SHARED_MEMORY_INFO OpteeShmInfo = { 0 };
> > > >
> > > >  /**
> > > >    Check for OP-TEE presence.
> > > > @@ -31,6 +38,7 @@ IsOpteePresent (
> > > >  {
> > > >    ARM_SMC_ARGS ArmSmcArgs;
> > > >
> > > > +  ZeroMem (&ArmSmcArgs, sizeof (ARM_SMC_ARGS));
> > > >    // Send a Trusted OS Calls UID command
> > > >    ArmSmcArgs.Arg0 = ARM_SMC_ID_TOS_UID;
> > > >    ArmCallSmc (&ArmSmcArgs);
> > > > @@ -44,3 +52,352 @@ IsOpteePresent (
> > > >      return FALSE;
> > > >    }
> > > >  }
> > > > +
> > > > +STATIC
> > > > +EFI_STATUS
> > > > +OpteeShmMemRemap (
> > > > +  VOID
> > > > +  )
> > > > +{
> > > > +  ARM_SMC_ARGS                 ArmSmcArgs;
> > > > +  EFI_PHYSICAL_ADDRESS         Paddr;
> > > > +  EFI_PHYSICAL_ADDRESS         Start;
> > > > +  EFI_PHYSICAL_ADDRESS         End;
> > > > +  EFI_STATUS                   Status;
> > > > +  UINTN                        Size;
> > > > +
> > > > +  ZeroMem (&ArmSmcArgs, sizeof (ARM_SMC_ARGS));
> > > > +  ArmSmcArgs.Arg0 = OPTEE_SMC_GET_SHM_CONFIG;
> > > > +
> > > > +  ArmCallSmc (&ArmSmcArgs);
> > > > +  if (ArmSmcArgs.Arg0 != OPTEE_SMC_RETURN_OK) {
> > > > +    DEBUG ((DEBUG_WARN, "OP-TEE shared memory not supported\n"));
> > > > +    return EFI_UNSUPPORTED;
> > > > +  }
> > > > +
> > > > +  if (ArmSmcArgs.Arg3 != OPTEE_SMC_SHM_CACHED) {
> > > > +    DEBUG ((DEBUG_WARN, "OP-TEE: Only normal cached shared memory supported\n"));
> > > > +    return EFI_UNSUPPORTED;
> > > > +  }
> > > > +
> > > > +  Start = (ArmSmcArgs.Arg1 + SIZE_4KB - 1) & ~(SIZE_4KB - 1);
> > > > +  End = (ArmSmcArgs.Arg1 + ArmSmcArgs.Arg2) & ~(SIZE_4KB - 1);
> > > > +  Paddr = Start;
> > > > +  Size = End - Start;
> > > > +
> > > > +  if (Size < SIZE_4KB) {
> > > > +    DEBUG ((DEBUG_WARN, "OP-TEE shared memory too small\n"));
> > > > +    return EFI_BUFFER_TOO_SMALL;
> > > > +  }
> > > > +
> > > > +  Status = ArmSetMemoryAttributes (Paddr, Size, EFI_MEMORY_WB);
> > > > +  if (EFI_ERROR (Status)) {
> > > > +    return Status;
> > > > +  }
> > > > +
> > > > +  OpteeShmInfo.Base = (UINTN)Paddr;
> >
> > Would PHYSICAL_ADDRESS (always 64-bit) be more correct here, or is the
> > OP-TEE end seeing this struct as always having fields of native width?
> 
> Its a global struct not shared with OP-TEE. Used native width to have
> compatibility with 32-bit system.

OK, that is not a problem then.

> > (This is where the ordefile comes in handy, because it means I see the
> > struct definition first, not its use.)
> >
> > > > +  OpteeShmInfo.Size = Size;
> > > > +
> > > > +  return EFI_SUCCESS;
> > > > +}
> > > > +
> > > > +EFI_STATUS
> > > > +EFIAPI
> > > > +OpteeInit (
> > > > +  VOID
> > > > +  )
> > > > +{
> > > > +  EFI_STATUS      Status;
> > > > +
> > > > +  if (!IsOpteePresent ()) {
> > > > +    DEBUG ((DEBUG_WARN, "OP-TEE not present\n"));
> > > > +    return EFI_UNSUPPORTED;
> > > > +  }
> > > > +
> > > > +  Status = OpteeShmMemRemap ();
> > > > +  if (EFI_ERROR (Status)) {
> > > > +    DEBUG ((DEBUG_WARN, "OP-TEE shared memory remap failed\n"));
> > > > +    return Status;
> > > > +  }
> > > > +
> > > > +  return EFI_SUCCESS;
> > > > +}
> > > > +
> > > > +/**
> > > > +  Does Standard SMC to OP-TEE in secure world.
> > > > +
> > > > +  @param[in]  Parg   Physical address of message to pass to secure world
> > > > +
> > > > +  @return            0 on success, secure world return code otherwise
> > > > +
> > > > +**/
> > > > +STATIC
> > > > +UINT32
> > > > +OpteeCallWithArg (
> > > > +  IN EFI_PHYSICAL_ADDRESS Parg
> > > > +  )
> > > > +{
> > > > +  ARM_SMC_ARGS ArmSmcArgs;
> > > > +
> > > > +  ZeroMem (&ArmSmcArgs, sizeof (ARM_SMC_ARGS));
> > > > +  ArmSmcArgs.Arg0 = OPTEE_SMC_CALL_WITH_ARG;
> > > > +  ArmSmcArgs.Arg1 = (UINT32)(Parg >> 32);
> > > > +  ArmSmcArgs.Arg2 = (UINT32)Parg;
> > > > +
> > > > +  while (TRUE) {
> > > > +    ArmCallSmc (&ArmSmcArgs);
> > > > +
> > > > +    if (ArmSmcArgs.Arg0 == OPTEE_SMC_RETURN_RPC_FOREIGN_INTR) {
> > > > +      //
> > > > +      // A foreign interrupt was raised while secure world was
> > > > +      // executing, since they are handled in UEFI a dummy RPC is
> > > > +      // performed to let UEFI take the interrupt through the normal
> > > > +      // vector.
> > > > +      //
> > > > +      ArmSmcArgs.Arg0 = OPTEE_SMC_RETURN_FROM_RPC;
> > > > +    } else {
> > > > +      break;
> > > > +    }
> > > > +  }
> > > > +
> > > > +  return ArmSmcArgs.Arg0;
> > > > +}
> > > > +
> > > > +EFI_STATUS
> > > > +EFIAPI
> > > > +OpteeOpenSession (
> > > > +  IN OUT OPTEE_OPEN_SESSION_ARG      *OpenSessionArg
> > > > +  )
> > > > +{
> > > > +  OPTEE_MSG_ARG    *MsgArg;
> > > > +
> > > > +  MsgArg = NULL;
> > > > +
> > > > +  if (OpteeShmInfo.Base == 0) {
> > > > +    DEBUG ((DEBUG_WARN, "OP-TEE not initialized\n"));
> > > > +    return EFI_NOT_STARTED;
> > > > +  }
> > > > +
> > > > +  MsgArg = (OPTEE_MSG_ARG *)OpteeShmInfo.Base;
> > > > +  ZeroMem (MsgArg, sizeof (OPTEE_MSG_ARG));
> > > > +
> > > > +  MsgArg->Cmd = OPTEE_MSG_CMD_OPEN_SESSION;
> > > > +
> > > > +  //
> > > > +  // Initialize and add the meta parameters needed when opening a
> > > > +  // session.
> > > > +  //
> > > > +  MsgArg->Params[0].Attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT |
> > > > +                           OPTEE_MSG_ATTR_META;
> > > > +  MsgArg->Params[1].Attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT |
> > > > +                           OPTEE_MSG_ATTR_META;
> > > > +  CopyMem (&MsgArg->Params[0].U.Value, OpenSessionArg->Uuid, OPTEE_UUID_LEN);
> > > > +  ZeroMem (&MsgArg->Params[1].U.Value, OPTEE_UUID_LEN);
> > > > +  MsgArg->Params[1].U.Value.C = TEE_LOGIN_PUBLIC;
> > > > +
> > > > +  MsgArg->NumParams = 2;
> > > > +
> > > > +  if (OpteeCallWithArg ((EFI_PHYSICAL_ADDRESS)MsgArg)) {
> > > > +    MsgArg->Ret = OPTEE_ERROR_COMMS;
> > > > +    MsgArg->RetOrigin = OPTEE_ORIGIN_COMMS;
> > > > +  }
> > > > +
> > > > +  OpenSessionArg->Session = MsgArg->Session;
> > > > +  OpenSessionArg->Ret = MsgArg->Ret;
> > > > +  OpenSessionArg->RetOrigin = MsgArg->RetOrigin;
> > > > +
> > > > +  return EFI_SUCCESS;
> > > > +}
> > > > +
> > > > +EFI_STATUS
> > > > +EFIAPI
> > > > +OpteeCloseSession (
> > > > +  IN UINT32                      Session
> > > > +  )
> > > > +{
> > > > +  OPTEE_MSG_ARG    *MsgArg;
> > > > +
> > > > +  MsgArg = NULL;
> > > > +
> > > > +  if (OpteeShmInfo.Base == 0) {
> > > > +    DEBUG ((DEBUG_WARN, "OP-TEE not initialized\n"));
> > > > +    return EFI_NOT_STARTED;
> > > > +  }
> > > > +
> > > > +  MsgArg = (OPTEE_MSG_ARG *)OpteeShmInfo.Base;
> > > > +  ZeroMem (MsgArg, sizeof (OPTEE_MSG_ARG));
> > > > +
> > > > +  MsgArg->Cmd = OPTEE_MSG_CMD_CLOSE_SESSION;
> > > > +  MsgArg->Session = Session;
> > > > +
> > > > +  OpteeCallWithArg ((EFI_PHYSICAL_ADDRESS)MsgArg);
> > > > +
> > > > +  return EFI_SUCCESS;
> > > > +}
> > > > +
> > > > +STATIC
> > > > +EFI_STATUS
> > > > +OpteeToMsgParam (
> > > > +  OUT OPTEE_MSG_PARAM    *MsgParams,
> > > > +  IN UINT32              NumParams,
> > > > +  IN OPTEE_MSG_PARAM     *InParams
> > > > +  )
> > > > +{
> > > > +  UINT32                  Idx;
> > > > +  UINTN                   ParamShmAddr;
> > > > +  UINTN                   ShmSize;
> > > > +  UINTN                   Size;
> > > > +
> > > > +  Size = (sizeof (OPTEE_MSG_ARG) + sizeof (UINT64) - 1) & ~(sizeof (UINT64) - 1);
> > > > +  ParamShmAddr = OpteeShmInfo.Base + Size;
> > > > +  ShmSize = OpteeShmInfo.Size - Size;
> > > > +
> > > > +  for (Idx = 0; Idx < NumParams; Idx++) {
> > > > +    CONST OPTEE_MSG_PARAM    *Ip;
> > > > +    OPTEE_MSG_PARAM          *Mp;
> > > > +    UINT32                   Attr;
> > > > +
> > > > +    Ip = InParams + Idx;
> > > > +    Mp = MsgParams + Idx;
> > > > +    Attr = Ip->Attr & OPTEE_MSG_ATTR_TYPE_MASK;
> > > > +
> > > > +    switch (Attr) {
> > > > +    case OPTEE_MSG_ATTR_TYPE_NONE:
> > > > +      Mp->Attr = OPTEE_MSG_ATTR_TYPE_NONE;
> > > > +      ZeroMem (&Mp->U, sizeof (Mp->U));
> > > > +      break;
> > > > +
> > > > +    case OPTEE_MSG_ATTR_TYPE_VALUE_INPUT:
> > > > +    case OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT:
> > > > +    case OPTEE_MSG_ATTR_TYPE_VALUE_INOUT:
> > > > +      Mp->Attr = Attr;
> > > > +      Mp->U.Value.A = Ip->U.Value.A;
> > > > +      Mp->U.Value.B = Ip->U.Value.B;
> > > > +      Mp->U.Value.C = Ip->U.Value.C;
> > > > +      break;
> > > > +
> > > > +    case OPTEE_MSG_ATTR_TYPE_MEM_INPUT:
> > > > +    case OPTEE_MSG_ATTR_TYPE_MEM_OUTPUT:
> > > > +    case OPTEE_MSG_ATTR_TYPE_MEM_INOUT:
> > > > +      Mp->Attr = Attr;
> > > > +
> > > > +      if (Ip->U.Mem.Size > ShmSize) {
> > > > +        return EFI_OUT_OF_RESOURCES;
> > > > +      }
> > > > +
> > > > +      CopyMem ((VOID *)ParamShmAddr, (VOID *)Ip->U.Mem.BufPtr, Ip->U.Mem.Size);
> > > > +      Mp->U.Mem.BufPtr = (UINT64)ParamShmAddr;
> >
> > While I don't think it's possible for this to end up doing something
> > unexpected, casting pointers to a higher alignment (which this would
> > do on 32-bit) is a bad habit to get into.
> 
> This is a shared message param structure with OP-TEE. So needed to cast it.

Then you need to also ensure that you are not accidentally casting a
less-than-8-byte aligned pointer here.
I would suggest an explicit test with an error return and an
assert. Then if that thing ever does come back to bite you, you will
know why, rather than needing to spend a few days debugging.

Alternatively, make ParamShmAddr an UINT64 * to begin with.

> > > > +      Mp->U.Mem.Size = Ip->U.Mem.Size;
> > > > +
> > > > +      Size = (Ip->U.Mem.Size + sizeof (UINT64) - 1) & ~(sizeof (UINT64) - 1);
> > > > +      ParamShmAddr += Size;
> > > > +      ShmSize -= Size;
> > > > +      break;
> > > > +
> > > > +    default:
> > > > +      return EFI_INVALID_PARAMETER;
> > > > +    }
> > > > +  }
> > > > +
> > > > +  return EFI_SUCCESS;
> > > > +}
> > > > +
> > > > +STATIC
> > > > +EFI_STATUS
> > > > +OpteeFromMsgParam (
> > > > +  OUT OPTEE_MSG_PARAM    *OutParams,
> > > > +  IN UINT32              NumParams,
> > > > +  IN OPTEE_MSG_PARAM     *MsgParams
> > > > +  )
> > > > +{
> > > > +  UINT32                 Idx;
> > > > +
> > > > +  for (Idx = 0; Idx < NumParams; Idx++) {
> > > > +    OPTEE_MSG_PARAM          *Op;
> > > > +    CONST OPTEE_MSG_PARAM    *Mp;
> > > > +    UINT32                   Attr;
> > > > +
> > > > +    Op = OutParams + Idx;
> > > > +    Mp = MsgParams + Idx;
> > > > +    Attr = Mp->Attr & OPTEE_MSG_ATTR_TYPE_MASK;
> > > > +
> > > > +    switch (Attr) {
> > > > +    case OPTEE_MSG_ATTR_TYPE_NONE:
> > > > +      Op->Attr = OPTEE_MSG_ATTR_TYPE_NONE;
> > > > +      ZeroMem (&Op->U, sizeof (Op->U));
> > > > +      break;
> > > > +
> > > > +    case OPTEE_MSG_ATTR_TYPE_VALUE_INPUT:
> > > > +    case OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT:
> > > > +    case OPTEE_MSG_ATTR_TYPE_VALUE_INOUT:
> > > > +      Op->Attr = Attr;
> > > > +      Op->U.Value.A = Mp->U.Value.A;
> > > > +      Op->U.Value.B = Mp->U.Value.B;
> > > > +      Op->U.Value.C = Mp->U.Value.C;
> > > > +      break;
> > > > +
> > > > +    case OPTEE_MSG_ATTR_TYPE_MEM_INPUT:
> > > > +    case OPTEE_MSG_ATTR_TYPE_MEM_OUTPUT:
> > > > +    case OPTEE_MSG_ATTR_TYPE_MEM_INOUT:
> > > > +      Op->Attr = Attr;
> > > > +
> > > > +      if (Mp->U.Mem.Size > Op->U.Mem.Size) {
> > > > +        return EFI_BAD_BUFFER_SIZE;
> > > > +      }
> > > > +
> > > > +      CopyMem ((VOID *)Op->U.Mem.BufPtr, (VOID *)Mp->U.Mem.BufPtr, Mp->U.Mem.Size);
> > > > +      Op->U.Mem.Size = Mp->U.Mem.Size;
> > > > +      break;
> > > > +
> > > > +    default:
> > > > +      return EFI_INVALID_PARAMETER;
> > > > +    }
> > > > +  }
> > > > +
> > > > +  return EFI_SUCCESS;
> > > > +}
> > > > +
> > > > +EFI_STATUS
> > > > +EFIAPI
> > > > +OpteeInvokeFunc (
> > > > +  IN OUT OPTEE_INVOKE_FUNC_ARG       *InvokeFuncArg
> > > > +  )
> > > > +{
> > > > +  EFI_STATUS       Status;
> > > > +  OPTEE_MSG_ARG    *MsgArg;
> > > > +
> > > > +  MsgArg = NULL;
> > > > +
> > > > +  if (OpteeShmInfo.Base == 0) {
> > > > +    DEBUG ((DEBUG_WARN, "OP-TEE not initialized\n"));
> > > > +    return EFI_NOT_STARTED;
> > > > +  }
> > > > +
> > > > +  MsgArg = (OPTEE_MSG_ARG *)OpteeShmInfo.Base;
> > > > +  ZeroMem (MsgArg, sizeof (OPTEE_MSG_ARG));
> > > > +
> > > > +  MsgArg->Cmd = OPTEE_MSG_CMD_INVOKE_COMMAND;
> > > > +  MsgArg->Func = InvokeFuncArg->Func;
> > > > +  MsgArg->Session = InvokeFuncArg->Session;
> > > > +
> > > > +  Status = OpteeToMsgParam (MsgArg->Params, MAX_PARAMS, InvokeFuncArg->Params);
> > > > +  if (Status)
> > > > +    return Status;
> >
> > Always use {} (coding style requirement).
> >
> 
> Ok.
> 
> > > > +
> > > > +  MsgArg->NumParams = MAX_PARAMS;
> > > > +
> > > > +  if (OpteeCallWithArg ((EFI_PHYSICAL_ADDRESS)MsgArg)) {
> > > > +    MsgArg->Ret = OPTEE_ERROR_COMMS;
> > > > +    MsgArg->RetOrigin = OPTEE_ORIGIN_COMMS;
> > > > +  }
> > > > +
> > > > +  if (OpteeFromMsgParam (InvokeFuncArg->Params, MAX_PARAMS, MsgArg->Params)) {
> > > > +    MsgArg->Ret = OPTEE_ERROR_COMMS;
> > > > +    MsgArg->RetOrigin = OPTEE_ORIGIN_COMMS;
> > > > +  }
> > > > +
> > > > +  InvokeFuncArg->Ret = MsgArg->Ret;
> > > > +  InvokeFuncArg->RetOrigin = MsgArg->RetOrigin;
> > > > +
> > > > +  return EFI_SUCCESS;
> > > > +}
> > > > diff --git a/ArmPkg/Library/OpteeLib/OpteeLib.inf b/ArmPkg/Library/OpteeLib/OpteeLib.inf
> > > > index 5abd427379cc..e03054a7167d 100644
> > > > --- a/ArmPkg/Library/OpteeLib/OpteeLib.inf
> > > > +++ b/ArmPkg/Library/OpteeLib/OpteeLib.inf
> > > > @@ -23,11 +23,13 @@ [Defines]
> > > >
> > > >  [Sources]
> > > >    Optee.c
> > > > +  OpteeSmc.h
> > > >
> > > >  [Packages]
> > > >    ArmPkg/ArmPkg.dec
> > > >    MdePkg/MdePkg.dec
> > > >
> > > >  [LibraryClasses]
> > > > +  ArmMmuLib
> > > >    ArmSmcLib
> > > >    BaseLib
> > > > diff --git a/ArmPkg/Library/OpteeLib/OpteeSmc.h b/ArmPkg/Library/OpteeLib/OpteeSmc.h
> > > > new file mode 100644
> > > > index 000000000000..e2ea35784a0a
> > > > --- /dev/null
> > > > +++ b/ArmPkg/Library/OpteeLib/OpteeSmc.h
> > > > @@ -0,0 +1,43 @@
> > > > +/** @file
> > > > +  OP-TEE SMC header file.
> > > > +
> > > > +  Copyright (c) 2018, Linaro Ltd. All rights reserved.<BR>
> > > > +
> > > > +  This program and the accompanying materials
> > > > +  are licensed and made available under the terms and conditions of the BSD License
> > > > +  which accompanies this distribution.  The full text of the license may be found at
> > > > +  http://opensource.org/licenses/bsd-license.php
> > > > +
> > > > +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> > > > +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> > > > +
> > > > +**/
> > > > +
> > > > +#ifndef _OPTEE_SMC_H_
> > > > +#define _OPTEE_SMC_H_
> > > > +
> > > > +/* Returned in Arg0 only from Trusted OS functions */
> > > > +#define OPTEE_SMC_RETURN_OK                     0x0
> > > > +
> > > > +#define OPTEE_SMC_RETURN_FROM_RPC               0x32000003
> > > > +#define OPTEE_SMC_CALL_WITH_ARG                 0x32000004
> > > > +#define OPTEE_SMC_GET_SHM_CONFIG                0xb2000007
> > > > +
> > > > +#define OPTEE_SMC_SHM_CACHED                    1
> > > > +
> > > > +#define OPTEE_SMC_RETURN_RPC_FOREIGN_INTR       0xffff0004
> > > > +
> > > > +#define OPTEE_MSG_CMD_OPEN_SESSION              0
> > > > +#define OPTEE_MSG_CMD_INVOKE_COMMAND            1
> > > > +#define OPTEE_MSG_CMD_CLOSE_SESSION             2
> > > > +
> > > > +#define OPTEE_MSG_ATTR_META                     0x100
> > > > +
> > > > +#define TEE_LOGIN_PUBLIC                        0x0
> >
> > I'm a bit apprehensive about taking over the whole TEE namespace in an
> > implementation-specific header file.
> > Can/should this be OPTEE_?
> >
> 
> Ok will change this to OPTEE_.

Thanks!


/
    Leif

Re: [PATCH v3 1/1] ArmPkg/OpteeLib: Add APIs to communicate with OP-TEE

[Sumit Garg]

On Mon, 8 Oct 2018 at 19:35, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Mon, Oct 08, 2018 at 03:20:27PM +0530, Sumit Garg wrote:
> > On Fri, 5 Oct 2018 at 20:33, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > >
> > > On Wed, Oct 03, 2018 at 11:33:01AM +0200, Ard Biesheuvel wrote:
> > > > On 3 October 2018 at 09:43, Sumit Garg <sumit.garg@linaro.org> wrote:
> > > > > Add following APIs to communicate with OP-TEE pseudo/early TAs:
> > > > > 1. OpteeInit
> > > > > 2. OpteeOpenSession
> > > > > 3. OpteeCloseSession
> > > > > 4. OpteeInvokeFunc
> > > > >
> > > > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > > > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > > >
> > > > Given the outcome of the GP discussion, I'm fine with this approach. Leif?
> > >
> > > Apologies for the delay, I needed some time to think it over.
> > >
> > > I'm not super happy about this approach, but I'm happier with this
> > > than I would be with leaving the functionality out of the upstream
> > > tree. I really hope we can get that license either changed or dropped
> > > completely.
> > >
> >
> > Thanks Leif for this go ahead.
> >
> > > I do have a few comments below.
> > >
> > > > > ---
> > > > >  ArmPkg/Include/Library/OpteeLib.h    |  90 +++++++++
> > > > >  ArmPkg/Library/OpteeLib/Optee.c      | 357 +++++++++++++++++++++++++++++++++++
> > > > >  ArmPkg/Library/OpteeLib/OpteeLib.inf |   2 +
> > > > >  ArmPkg/Library/OpteeLib/OpteeSmc.h   |  43 +++++
> > >
> > > Could you follow the instructions in
> > > https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-23
> > > when generating future patches?
> > > The --stat* effects aren't apparent here, but the -O ones are.
> > >
> >
> > Sure.
> >
> > > > >  4 files changed, 492 insertions(+)
> > > > >  create mode 100644 ArmPkg/Library/OpteeLib/OpteeSmc.h
> > > > >
> > > > > diff --git a/ArmPkg/Include/Library/OpteeLib.h b/ArmPkg/Include/Library/OpteeLib.h
> > > > > index f65d8674d9b8..2d1c60632dfe 100644
> > > > > --- a/ArmPkg/Include/Library/OpteeLib.h
> > > > > +++ b/ArmPkg/Include/Library/OpteeLib.h
> > > > > @@ -25,10 +25,100 @@
> > > > >  #define OPTEE_OS_UID2          0xaf630002
> > > > >  #define OPTEE_OS_UID3          0xa5d5c51b
> > > > >
> > > > > +#define OPTEE_MSG_ATTR_TYPE_NONE                0x0
> > > > > +#define OPTEE_MSG_ATTR_TYPE_VALUE_INPUT         0x1
> > > > > +#define OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT        0x2
> > > > > +#define OPTEE_MSG_ATTR_TYPE_VALUE_INOUT         0x3
> > > > > +#define OPTEE_MSG_ATTR_TYPE_MEM_INPUT           0x9
> > > > > +#define OPTEE_MSG_ATTR_TYPE_MEM_OUTPUT          0xa
> > > > > +#define OPTEE_MSG_ATTR_TYPE_MEM_INOUT           0xb
> > > > > +
> > > > > +#define OPTEE_MSG_ATTR_TYPE_MASK                0xff
> > > > > +
> > > > > +#define OPTEE_ORIGIN_COMMS                      0x00000002
> > > > > +#define OPTEE_ERROR_COMMS                       0xFFFF000E
> > > > > +
> > > > > +typedef struct {
> > > > > +  UINT64    BufPtr;
> > >
> > > If it's a pointer, it has a *.
> > > Otherwise it's an address.
> > >
> >
> > Will rather use BufferAddress.
> >
> > > > > +  UINT64    Size;
> > > > > +  UINT64    ShmRef;
> > >
> > > Abbreviations in function, variable or type names (other than the ones
> > > defined in [1] are not permitted unless they are explicitly added to a
> > > glossary section of the source file header. Where possible, just write
> > > out the name in full.
> > >
> > > [1] https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/v/release/2.20/4_naming_conventions/#table-2-efi-supported-abbreviations
> > >
> > > BufPtr (as a name) gets a pass since it's unambiguous and we already
> > > have a bunch of those in the codebase. ShmRef needs to be clear.
> > >
> >
> > Will replace it with SharedMemoryReference.
> >
> > > (This comment also applies to a lot of things below, I won't point
> > > them all out unless asked to.)
> > >
> >
> > I will try to replace most of them with full names. But I have
> > confusion regarding usage of following not included in [1]:
> >
> > 1. MSG/Msg
> > 2. MEM/Mem
> > 3. INFO/Info
> > 4. FUNC/Func
> > 5. RET/Ret
> > 6. ATTR/Attr
> > 7. CMD/Cmd
> >
> > But I do see their usage in the codebase. Please help to clarify.
>
> It's a horrible secret, but not all of the codebase conforms to the
> coding style. We're continuously trying to improve it.
>
> > Also can I use 1-2 letter variable or struct member names like Mp, Ip,
> > Op, U etc.?
>
> Ideally not. I'm not a huge fan of ReallyLongCamelCase coding styles,
> but the benefit it does bring is that you don't need to stop and think
> about what a variable may be for. And correspondingly, mixing between
> abbreviations an non-abbreviations make for hard reading.
>
> Another completely unscientific observation I would make is that once
> you force yourself to write a full variable name out, it becomes a lot
> more clear when you've left valuable semantic information out.
> The codebase ends up with a bunch of fewer variables called only
> 'Msg', 'Mem', 'Info', Func', 'Ret', 'Attr' 'Cmd' (, or 'Data' ...).
> (Alternatively, the mailing list ends up with a lot fewer emails
> asking "what message", "what memory", "what information" ...)
>

Ok then I will replace these abbreviations with full name too in v4.

> > > > > +} OPTEE_MSG_PARAM_MEM;
> > > > > +
> > > > > +typedef struct {
> > > > > +  UINT64    A;
> > > > > +  UINT64    B;
> > > > > +  UINT64    C;
> > > > > +} OPTEE_MSG_PARAM_VALUE;
> > > > > +
> > > > > +typedef struct {
> > > > > +  UINT64 Attr;
> > > > > +  union {
> > > > > +    OPTEE_MSG_PARAM_MEM      Mem;
> > > > > +    OPTEE_MSG_PARAM_VALUE    Value;
> > > > > +  } U;
> > > > > +} OPTEE_MSG_PARAM;
> > > > > +
> > > > > +#define MAX_PARAMS           4
> > >
> > > This is a very localised macro with a very globalised name.
> > > Suggest adding an OPTEE_ prefix, but also something describing what it
> > > is the maximum parameters for. CALL_?
> > >
> >
> > Ok will rename it to OPTEE_CALL_MAX_PARAMS.
>
> I'd take that or OPTEE_MAX_CALL_PARAMS.
>

OPTEE_MAX_CALL_PARAMS sounds more apt. Will use it instead.

> > > > > +
> > > > > +typedef struct {
> > > > > +        UINT32             Cmd;
> > > > > +        UINT32             Func;
> > > > > +        UINT32             Session;
> > > > > +        UINT32             CancelId;
> > > > > +        UINT32             Pad;
> > > > > +        UINT32             Ret;
> > > > > +        UINT32             RetOrigin;
> > > > > +        UINT32             NumParams;
> > > > > +
> > > > > +        // NumParams tells the actual number of element in Params
> > > > > +        OPTEE_MSG_PARAM    Params[MAX_PARAMS];
> > > > > +} OPTEE_MSG_ARG;
> > > > > +
> > > > > +#define OPTEE_UUID_LEN       16
> > >
> > > UUIDs are UUIDs. If optee decides on an incompatible format, we may
> > > have an interoperability issue. I assume this is not the case, so
> > > perhaps replace with sizeof (EFI_GUID) at each point of use?
> >
> > Agree will use sizeof (EFI_GUID) instead.
> >
> > > > > +
> > > > > +typedef struct {
> > > > > +        UINT8     Uuid[OPTEE_UUID_LEN]; // [in] UUID of the Trusted Application
> > >
> > > Is there a strong reason for not using EFI_GUID here?
> > >
> >
> > EFI_GUID can be used here as UUID are also known as GUID. But here we
> > pass UUID as 16 octets to OP-TEE. So will use EFI_GUID instead and add
> > an api to convert uuid to octets.
>
> Right. That may be tedious, but I think it will be an improvement.
> If OPTEE uses UUIDs without the Microsoft timestamp endianness variant
> (as described in the UEFI spec), then ideally those functions could
> byte reverse the timestamp as well.
>

Yes, OPTEE uses UUID variant as per RFC-4122 [1] which states that
each UUID field is encoded with the Most Significant Byte first (known
as network byte order). So api to convert UUID to octets would do
following operation:

+STATIC
+VOID
+UuidToOctets (
+  OUT UINT8              *UuidOctet,
+  IN EFI_GUID            *Uuid
+  )
+{
+  UuidOctet[0] = Uuid->Data1 >> 24;
+  UuidOctet[1] = Uuid->Data1 >> 16;
+  UuidOctet[2] = Uuid->Data1 >> 8;
+  UuidOctet[3] = Uuid->Data1;
+  UuidOctet[4] = Uuid->Data2 >> 8;
+  UuidOctet[5] = Uuid->Data2;
+  UuidOctet[6] = Uuid->Data3 >> 8;
+  UuidOctet[7] = Uuid->Data3;
+  CopyMem (UuidOctet + 8, Uuid->Data4, sizeof (Uuid->Data4));
+}

[1] https://tools.ietf.org/html/rfc4122

> > > > > +        UINT32    Session;              // [out] Session id
> > > > > +        UINT32    Ret;                  // [out] Return value
> > > > > +        UINT32    RetOrigin;            // [out] Origin of the return value
> > > > > +} OPTEE_OPEN_SESSION_ARG;
> > > > > +
> > > > > +typedef struct {
> > > > > +        UINT32             Func;    // [in] Trusted App func, specific to the TA
> > > > > +        UINT32             Session; // [in] Session id
> > > > > +        UINT32             Ret;     // [out] Return value
> > > > > +        UINT32             RetOrigin; // [out] Origin of the return value
> > > > > +        OPTEE_MSG_PARAM    Params[MAX_PARAMS]; // Params for func to be invoked
> > > > > +} OPTEE_INVOKE_FUNC_ARG;
> > > > > +
> > > > >  BOOLEAN
> > > > >  EFIAPI
> > > > >  IsOpteePresent (
> > > > >    VOID
> > > > >    );
> > > > >
> > > > > +EFI_STATUS
> > > > > +EFIAPI
> > > > > +OpteeInit (
> > > > > +  VOID
> > > > > +  );
> > > > > +
> > > > > +EFI_STATUS
> > > > > +EFIAPI
> > > > > +OpteeOpenSession (
> > > > > +  IN OUT OPTEE_OPEN_SESSION_ARG      *OpenSessionArg
> > > > > +  );
> > > > > +
> > > > > +EFI_STATUS
> > > > > +EFIAPI
> > > > > +OpteeCloseSession (
> > > > > +  IN UINT32                      Session
> > > > > +  );
> > > > > +
> > > > > +EFI_STATUS
> > > > > +EFIAPI
> > > > > +OpteeInvokeFunc (
> > > > > +  IN OUT OPTEE_INVOKE_FUNC_ARG       *InvokeFuncArg
> > > > > +  );
> > > > > +
> > > > >  #endif
> > > > > diff --git a/ArmPkg/Library/OpteeLib/Optee.c b/ArmPkg/Library/OpteeLib/Optee.c
> > > > > index 574527f8b5ea..bf7872cbbce0 100644
> > > > > --- a/ArmPkg/Library/OpteeLib/Optee.c
> > > > > +++ b/ArmPkg/Library/OpteeLib/Optee.c
> > > > > @@ -14,11 +14,18 @@
> > > > >
> > > > >  **/
> > > > >
> > > > > +#include <Library/ArmMmuLib.h>
> > > > >  #include <Library/ArmSmcLib.h>
> > > > > +#include <Library/BaseMemoryLib.h>
> > > > >  #include <Library/BaseLib.h>
> > > > > +#include <Library/DebugLib.h>
> > > > >  #include <Library/OpteeLib.h>
> > > > >
> > > > >  #include <IndustryStandard/ArmStdSmc.h>
> > > > > +#include <OpteeSmc.h>
> > > > > +#include <Uefi.h>
> > > > > +
> > > > > +STATIC OPTEE_SHARED_MEMORY_INFO OpteeShmInfo = { 0 };
> > > > >
> > > > >  /**
> > > > >    Check for OP-TEE presence.
> > > > > @@ -31,6 +38,7 @@ IsOpteePresent (
> > > > >  {
> > > > >    ARM_SMC_ARGS ArmSmcArgs;
> > > > >
> > > > > +  ZeroMem (&ArmSmcArgs, sizeof (ARM_SMC_ARGS));
> > > > >    // Send a Trusted OS Calls UID command
> > > > >    ArmSmcArgs.Arg0 = ARM_SMC_ID_TOS_UID;
> > > > >    ArmCallSmc (&ArmSmcArgs);
> > > > > @@ -44,3 +52,352 @@ IsOpteePresent (
> > > > >      return FALSE;
> > > > >    }
> > > > >  }
> > > > > +
> > > > > +STATIC
> > > > > +EFI_STATUS
> > > > > +OpteeShmMemRemap (
> > > > > +  VOID
> > > > > +  )
> > > > > +{
> > > > > +  ARM_SMC_ARGS                 ArmSmcArgs;
> > > > > +  EFI_PHYSICAL_ADDRESS         Paddr;
> > > > > +  EFI_PHYSICAL_ADDRESS         Start;
> > > > > +  EFI_PHYSICAL_ADDRESS         End;
> > > > > +  EFI_STATUS                   Status;
> > > > > +  UINTN                        Size;
> > > > > +
> > > > > +  ZeroMem (&ArmSmcArgs, sizeof (ARM_SMC_ARGS));
> > > > > +  ArmSmcArgs.Arg0 = OPTEE_SMC_GET_SHM_CONFIG;
> > > > > +
> > > > > +  ArmCallSmc (&ArmSmcArgs);
> > > > > +  if (ArmSmcArgs.Arg0 != OPTEE_SMC_RETURN_OK) {
> > > > > +    DEBUG ((DEBUG_WARN, "OP-TEE shared memory not supported\n"));
> > > > > +    return EFI_UNSUPPORTED;
> > > > > +  }
> > > > > +
> > > > > +  if (ArmSmcArgs.Arg3 != OPTEE_SMC_SHM_CACHED) {
> > > > > +    DEBUG ((DEBUG_WARN, "OP-TEE: Only normal cached shared memory supported\n"));
> > > > > +    return EFI_UNSUPPORTED;
> > > > > +  }
> > > > > +
> > > > > +  Start = (ArmSmcArgs.Arg1 + SIZE_4KB - 1) & ~(SIZE_4KB - 1);
> > > > > +  End = (ArmSmcArgs.Arg1 + ArmSmcArgs.Arg2) & ~(SIZE_4KB - 1);
> > > > > +  Paddr = Start;
> > > > > +  Size = End - Start;
> > > > > +
> > > > > +  if (Size < SIZE_4KB) {
> > > > > +    DEBUG ((DEBUG_WARN, "OP-TEE shared memory too small\n"));
> > > > > +    return EFI_BUFFER_TOO_SMALL;
> > > > > +  }
> > > > > +
> > > > > +  Status = ArmSetMemoryAttributes (Paddr, Size, EFI_MEMORY_WB);
> > > > > +  if (EFI_ERROR (Status)) {
> > > > > +    return Status;
> > > > > +  }
> > > > > +
> > > > > +  OpteeShmInfo.Base = (UINTN)Paddr;
> > >
> > > Would PHYSICAL_ADDRESS (always 64-bit) be more correct here, or is the
> > > OP-TEE end seeing this struct as always having fields of native width?
> >
> > Its a global struct not shared with OP-TEE. Used native width to have
> > compatibility with 32-bit system.
>
> OK, that is not a problem then.
>
> > > (This is where the ordefile comes in handy, because it means I see the
> > > struct definition first, not its use.)
> > >
> > > > > +  OpteeShmInfo.Size = Size;
> > > > > +
> > > > > +  return EFI_SUCCESS;
> > > > > +}
> > > > > +
> > > > > +EFI_STATUS
> > > > > +EFIAPI
> > > > > +OpteeInit (
> > > > > +  VOID
> > > > > +  )
> > > > > +{
> > > > > +  EFI_STATUS      Status;
> > > > > +
> > > > > +  if (!IsOpteePresent ()) {
> > > > > +    DEBUG ((DEBUG_WARN, "OP-TEE not present\n"));
> > > > > +    return EFI_UNSUPPORTED;
> > > > > +  }
> > > > > +
> > > > > +  Status = OpteeShmMemRemap ();
> > > > > +  if (EFI_ERROR (Status)) {
> > > > > +    DEBUG ((DEBUG_WARN, "OP-TEE shared memory remap failed\n"));
> > > > > +    return Status;
> > > > > +  }
> > > > > +
> > > > > +  return EFI_SUCCESS;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > +  Does Standard SMC to OP-TEE in secure world.
> > > > > +
> > > > > +  @param[in]  Parg   Physical address of message to pass to secure world
> > > > > +
> > > > > +  @return            0 on success, secure world return code otherwise
> > > > > +
> > > > > +**/
> > > > > +STATIC
> > > > > +UINT32
> > > > > +OpteeCallWithArg (
> > > > > +  IN EFI_PHYSICAL_ADDRESS Parg
> > > > > +  )
> > > > > +{
> > > > > +  ARM_SMC_ARGS ArmSmcArgs;
> > > > > +
> > > > > +  ZeroMem (&ArmSmcArgs, sizeof (ARM_SMC_ARGS));
> > > > > +  ArmSmcArgs.Arg0 = OPTEE_SMC_CALL_WITH_ARG;
> > > > > +  ArmSmcArgs.Arg1 = (UINT32)(Parg >> 32);
> > > > > +  ArmSmcArgs.Arg2 = (UINT32)Parg;
> > > > > +
> > > > > +  while (TRUE) {
> > > > > +    ArmCallSmc (&ArmSmcArgs);
> > > > > +
> > > > > +    if (ArmSmcArgs.Arg0 == OPTEE_SMC_RETURN_RPC_FOREIGN_INTR) {
> > > > > +      //
> > > > > +      // A foreign interrupt was raised while secure world was
> > > > > +      // executing, since they are handled in UEFI a dummy RPC is
> > > > > +      // performed to let UEFI take the interrupt through the normal
> > > > > +      // vector.
> > > > > +      //
> > > > > +      ArmSmcArgs.Arg0 = OPTEE_SMC_RETURN_FROM_RPC;
> > > > > +    } else {
> > > > > +      break;
> > > > > +    }
> > > > > +  }
> > > > > +
> > > > > +  return ArmSmcArgs.Arg0;
> > > > > +}
> > > > > +
> > > > > +EFI_STATUS
> > > > > +EFIAPI
> > > > > +OpteeOpenSession (
> > > > > +  IN OUT OPTEE_OPEN_SESSION_ARG      *OpenSessionArg
> > > > > +  )
> > > > > +{
> > > > > +  OPTEE_MSG_ARG    *MsgArg;
> > > > > +
> > > > > +  MsgArg = NULL;
> > > > > +
> > > > > +  if (OpteeShmInfo.Base == 0) {
> > > > > +    DEBUG ((DEBUG_WARN, "OP-TEE not initialized\n"));
> > > > > +    return EFI_NOT_STARTED;
> > > > > +  }
> > > > > +
> > > > > +  MsgArg = (OPTEE_MSG_ARG *)OpteeShmInfo.Base;
> > > > > +  ZeroMem (MsgArg, sizeof (OPTEE_MSG_ARG));
> > > > > +
> > > > > +  MsgArg->Cmd = OPTEE_MSG_CMD_OPEN_SESSION;
> > > > > +
> > > > > +  //
> > > > > +  // Initialize and add the meta parameters needed when opening a
> > > > > +  // session.
> > > > > +  //
> > > > > +  MsgArg->Params[0].Attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT |
> > > > > +                           OPTEE_MSG_ATTR_META;
> > > > > +  MsgArg->Params[1].Attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT |
> > > > > +                           OPTEE_MSG_ATTR_META;
> > > > > +  CopyMem (&MsgArg->Params[0].U.Value, OpenSessionArg->Uuid, OPTEE_UUID_LEN);
> > > > > +  ZeroMem (&MsgArg->Params[1].U.Value, OPTEE_UUID_LEN);
> > > > > +  MsgArg->Params[1].U.Value.C = TEE_LOGIN_PUBLIC;
> > > > > +
> > > > > +  MsgArg->NumParams = 2;
> > > > > +
> > > > > +  if (OpteeCallWithArg ((EFI_PHYSICAL_ADDRESS)MsgArg)) {
> > > > > +    MsgArg->Ret = OPTEE_ERROR_COMMS;
> > > > > +    MsgArg->RetOrigin = OPTEE_ORIGIN_COMMS;
> > > > > +  }
> > > > > +
> > > > > +  OpenSessionArg->Session = MsgArg->Session;
> > > > > +  OpenSessionArg->Ret = MsgArg->Ret;
> > > > > +  OpenSessionArg->RetOrigin = MsgArg->RetOrigin;
> > > > > +
> > > > > +  return EFI_SUCCESS;
> > > > > +}
> > > > > +
> > > > > +EFI_STATUS
> > > > > +EFIAPI
> > > > > +OpteeCloseSession (
> > > > > +  IN UINT32                      Session
> > > > > +  )
> > > > > +{
> > > > > +  OPTEE_MSG_ARG    *MsgArg;
> > > > > +
> > > > > +  MsgArg = NULL;
> > > > > +
> > > > > +  if (OpteeShmInfo.Base == 0) {
> > > > > +    DEBUG ((DEBUG_WARN, "OP-TEE not initialized\n"));
> > > > > +    return EFI_NOT_STARTED;
> > > > > +  }
> > > > > +
> > > > > +  MsgArg = (OPTEE_MSG_ARG *)OpteeShmInfo.Base;
> > > > > +  ZeroMem (MsgArg, sizeof (OPTEE_MSG_ARG));
> > > > > +
> > > > > +  MsgArg->Cmd = OPTEE_MSG_CMD_CLOSE_SESSION;
> > > > > +  MsgArg->Session = Session;
> > > > > +
> > > > > +  OpteeCallWithArg ((EFI_PHYSICAL_ADDRESS)MsgArg);
> > > > > +
> > > > > +  return EFI_SUCCESS;
> > > > > +}
> > > > > +
> > > > > +STATIC
> > > > > +EFI_STATUS
> > > > > +OpteeToMsgParam (
> > > > > +  OUT OPTEE_MSG_PARAM    *MsgParams,
> > > > > +  IN UINT32              NumParams,
> > > > > +  IN OPTEE_MSG_PARAM     *InParams
> > > > > +  )
> > > > > +{
> > > > > +  UINT32                  Idx;
> > > > > +  UINTN                   ParamShmAddr;
> > > > > +  UINTN                   ShmSize;
> > > > > +  UINTN                   Size;
> > > > > +
> > > > > +  Size = (sizeof (OPTEE_MSG_ARG) + sizeof (UINT64) - 1) & ~(sizeof (UINT64) - 1);
> > > > > +  ParamShmAddr = OpteeShmInfo.Base + Size;
> > > > > +  ShmSize = OpteeShmInfo.Size - Size;
> > > > > +
> > > > > +  for (Idx = 0; Idx < NumParams; Idx++) {
> > > > > +    CONST OPTEE_MSG_PARAM    *Ip;
> > > > > +    OPTEE_MSG_PARAM          *Mp;
> > > > > +    UINT32                   Attr;
> > > > > +
> > > > > +    Ip = InParams + Idx;
> > > > > +    Mp = MsgParams + Idx;
> > > > > +    Attr = Ip->Attr & OPTEE_MSG_ATTR_TYPE_MASK;
> > > > > +
> > > > > +    switch (Attr) {
> > > > > +    case OPTEE_MSG_ATTR_TYPE_NONE:
> > > > > +      Mp->Attr = OPTEE_MSG_ATTR_TYPE_NONE;
> > > > > +      ZeroMem (&Mp->U, sizeof (Mp->U));
> > > > > +      break;
> > > > > +
> > > > > +    case OPTEE_MSG_ATTR_TYPE_VALUE_INPUT:
> > > > > +    case OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT:
> > > > > +    case OPTEE_MSG_ATTR_TYPE_VALUE_INOUT:
> > > > > +      Mp->Attr = Attr;
> > > > > +      Mp->U.Value.A = Ip->U.Value.A;
> > > > > +      Mp->U.Value.B = Ip->U.Value.B;
> > > > > +      Mp->U.Value.C = Ip->U.Value.C;
> > > > > +      break;
> > > > > +
> > > > > +    case OPTEE_MSG_ATTR_TYPE_MEM_INPUT:
> > > > > +    case OPTEE_MSG_ATTR_TYPE_MEM_OUTPUT:
> > > > > +    case OPTEE_MSG_ATTR_TYPE_MEM_INOUT:
> > > > > +      Mp->Attr = Attr;
> > > > > +
> > > > > +      if (Ip->U.Mem.Size > ShmSize) {
> > > > > +        return EFI_OUT_OF_RESOURCES;
> > > > > +      }
> > > > > +
> > > > > +      CopyMem ((VOID *)ParamShmAddr, (VOID *)Ip->U.Mem.BufPtr, Ip->U.Mem.Size);
> > > > > +      Mp->U.Mem.BufPtr = (UINT64)ParamShmAddr;
> > >
> > > While I don't think it's possible for this to end up doing something
> > > unexpected, casting pointers to a higher alignment (which this would
> > > do on 32-bit) is a bad habit to get into.
> >
> > This is a shared message param structure with OP-TEE. So needed to cast it.
>
> Then you need to also ensure that you are not accidentally casting a
> less-than-8-byte aligned pointer here.
> I would suggest an explicit test with an error return and an
> assert. Then if that thing ever does come back to bite you, you will
> know why, rather than needing to spend a few days debugging.
>
> Alternatively, make ParamShmAddr an UINT64 * to begin with.
>

If you see above ParamShmAddr is computed taking care of 8-byte
alignment as follows:

+  Size = (sizeof (OPTEE_MSG_ARG) + sizeof (UINT64) - 1) & ~(sizeof
(UINT64) - 1);
+  ParamShmAddr = OpteeShmInfo.Base + Size;
+  ShmSize = OpteeShmInfo.Size - Size;

Here OpteeShmInfo.Base is already 4K aligned address.

-Sumit

> > > > > +      Mp->U.Mem.Size = Ip->U.Mem.Size;
> > > > > +
> > > > > +      Size = (Ip->U.Mem.Size + sizeof (UINT64) - 1) & ~(sizeof (UINT64) - 1);
> > > > > +      ParamShmAddr += Size;
> > > > > +      ShmSize -= Size;
> > > > > +      break;
> > > > > +
> > > > > +    default:
> > > > > +      return EFI_INVALID_PARAMETER;
> > > > > +    }
> > > > > +  }
> > > > > +
> > > > > +  return EFI_SUCCESS;
> > > > > +}
> > > > > +
> > > > > +STATIC
> > > > > +EFI_STATUS
> > > > > +OpteeFromMsgParam (
> > > > > +  OUT OPTEE_MSG_PARAM    *OutParams,
> > > > > +  IN UINT32              NumParams,
> > > > > +  IN OPTEE_MSG_PARAM     *MsgParams
> > > > > +  )
> > > > > +{
> > > > > +  UINT32                 Idx;
> > > > > +
> > > > > +  for (Idx = 0; Idx < NumParams; Idx++) {
> > > > > +    OPTEE_MSG_PARAM          *Op;
> > > > > +    CONST OPTEE_MSG_PARAM    *Mp;
> > > > > +    UINT32                   Attr;
> > > > > +
> > > > > +    Op = OutParams + Idx;
> > > > > +    Mp = MsgParams + Idx;
> > > > > +    Attr = Mp->Attr & OPTEE_MSG_ATTR_TYPE_MASK;
> > > > > +
> > > > > +    switch (Attr) {
> > > > > +    case OPTEE_MSG_ATTR_TYPE_NONE:
> > > > > +      Op->Attr = OPTEE_MSG_ATTR_TYPE_NONE;
> > > > > +      ZeroMem (&Op->U, sizeof (Op->U));
> > > > > +      break;
> > > > > +
> > > > > +    case OPTEE_MSG_ATTR_TYPE_VALUE_INPUT:
> > > > > +    case OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT:
> > > > > +    case OPTEE_MSG_ATTR_TYPE_VALUE_INOUT:
> > > > > +      Op->Attr = Attr;
> > > > > +      Op->U.Value.A = Mp->U.Value.A;
> > > > > +      Op->U.Value.B = Mp->U.Value.B;
> > > > > +      Op->U.Value.C = Mp->U.Value.C;
> > > > > +      break;
> > > > > +
> > > > > +    case OPTEE_MSG_ATTR_TYPE_MEM_INPUT:
> > > > > +    case OPTEE_MSG_ATTR_TYPE_MEM_OUTPUT:
> > > > > +    case OPTEE_MSG_ATTR_TYPE_MEM_INOUT:
> > > > > +      Op->Attr = Attr;
> > > > > +
> > > > > +      if (Mp->U.Mem.Size > Op->U.Mem.Size) {
> > > > > +        return EFI_BAD_BUFFER_SIZE;
> > > > > +      }
> > > > > +
> > > > > +      CopyMem ((VOID *)Op->U.Mem.BufPtr, (VOID *)Mp->U.Mem.BufPtr, Mp->U.Mem.Size);
> > > > > +      Op->U.Mem.Size = Mp->U.Mem.Size;
> > > > > +      break;
> > > > > +
> > > > > +    default:
> > > > > +      return EFI_INVALID_PARAMETER;
> > > > > +    }
> > > > > +  }
> > > > > +
> > > > > +  return EFI_SUCCESS;
> > > > > +}
> > > > > +
> > > > > +EFI_STATUS
> > > > > +EFIAPI
> > > > > +OpteeInvokeFunc (
> > > > > +  IN OUT OPTEE_INVOKE_FUNC_ARG       *InvokeFuncArg
> > > > > +  )
> > > > > +{
> > > > > +  EFI_STATUS       Status;
> > > > > +  OPTEE_MSG_ARG    *MsgArg;
> > > > > +
> > > > > +  MsgArg = NULL;
> > > > > +
> > > > > +  if (OpteeShmInfo.Base == 0) {
> > > > > +    DEBUG ((DEBUG_WARN, "OP-TEE not initialized\n"));
> > > > > +    return EFI_NOT_STARTED;
> > > > > +  }
> > > > > +
> > > > > +  MsgArg = (OPTEE_MSG_ARG *)OpteeShmInfo.Base;
> > > > > +  ZeroMem (MsgArg, sizeof (OPTEE_MSG_ARG));
> > > > > +
> > > > > +  MsgArg->Cmd = OPTEE_MSG_CMD_INVOKE_COMMAND;
> > > > > +  MsgArg->Func = InvokeFuncArg->Func;
> > > > > +  MsgArg->Session = InvokeFuncArg->Session;
> > > > > +
> > > > > +  Status = OpteeToMsgParam (MsgArg->Params, MAX_PARAMS, InvokeFuncArg->Params);
> > > > > +  if (Status)
> > > > > +    return Status;
> > >
> > > Always use {} (coding style requirement).
> > >
> >
> > Ok.
> >
> > > > > +
> > > > > +  MsgArg->NumParams = MAX_PARAMS;
> > > > > +
> > > > > +  if (OpteeCallWithArg ((EFI_PHYSICAL_ADDRESS)MsgArg)) {
> > > > > +    MsgArg->Ret = OPTEE_ERROR_COMMS;
> > > > > +    MsgArg->RetOrigin = OPTEE_ORIGIN_COMMS;
> > > > > +  }
> > > > > +
> > > > > +  if (OpteeFromMsgParam (InvokeFuncArg->Params, MAX_PARAMS, MsgArg->Params)) {
> > > > > +    MsgArg->Ret = OPTEE_ERROR_COMMS;
> > > > > +    MsgArg->RetOrigin = OPTEE_ORIGIN_COMMS;
> > > > > +  }
> > > > > +
> > > > > +  InvokeFuncArg->Ret = MsgArg->Ret;
> > > > > +  InvokeFuncArg->RetOrigin = MsgArg->RetOrigin;
> > > > > +
> > > > > +  return EFI_SUCCESS;
> > > > > +}
> > > > > diff --git a/ArmPkg/Library/OpteeLib/OpteeLib.inf b/ArmPkg/Library/OpteeLib/OpteeLib.inf
> > > > > index 5abd427379cc..e03054a7167d 100644
> > > > > --- a/ArmPkg/Library/OpteeLib/OpteeLib.inf
> > > > > +++ b/ArmPkg/Library/OpteeLib/OpteeLib.inf
> > > > > @@ -23,11 +23,13 @@ [Defines]
> > > > >
> > > > >  [Sources]
> > > > >    Optee.c
> > > > > +  OpteeSmc.h
> > > > >
> > > > >  [Packages]
> > > > >    ArmPkg/ArmPkg.dec
> > > > >    MdePkg/MdePkg.dec
> > > > >
> > > > >  [LibraryClasses]
> > > > > +  ArmMmuLib
> > > > >    ArmSmcLib
> > > > >    BaseLib
> > > > > diff --git a/ArmPkg/Library/OpteeLib/OpteeSmc.h b/ArmPkg/Library/OpteeLib/OpteeSmc.h
> > > > > new file mode 100644
> > > > > index 000000000000..e2ea35784a0a
> > > > > --- /dev/null
> > > > > +++ b/ArmPkg/Library/OpteeLib/OpteeSmc.h
> > > > > @@ -0,0 +1,43 @@
> > > > > +/** @file
> > > > > +  OP-TEE SMC header file.
> > > > > +
> > > > > +  Copyright (c) 2018, Linaro Ltd. All rights reserved.<BR>
> > > > > +
> > > > > +  This program and the accompanying materials
> > > > > +  are licensed and made available under the terms and conditions of the BSD License
> > > > > +  which accompanies this distribution.  The full text of the license may be found at
> > > > > +  http://opensource.org/licenses/bsd-license.php
> > > > > +
> > > > > +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> > > > > +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> > > > > +
> > > > > +**/
> > > > > +
> > > > > +#ifndef _OPTEE_SMC_H_
> > > > > +#define _OPTEE_SMC_H_
> > > > > +
> > > > > +/* Returned in Arg0 only from Trusted OS functions */
> > > > > +#define OPTEE_SMC_RETURN_OK                     0x0
> > > > > +
> > > > > +#define OPTEE_SMC_RETURN_FROM_RPC               0x32000003
> > > > > +#define OPTEE_SMC_CALL_WITH_ARG                 0x32000004
> > > > > +#define OPTEE_SMC_GET_SHM_CONFIG                0xb2000007
> > > > > +
> > > > > +#define OPTEE_SMC_SHM_CACHED                    1
> > > > > +
> > > > > +#define OPTEE_SMC_RETURN_RPC_FOREIGN_INTR       0xffff0004
> > > > > +
> > > > > +#define OPTEE_MSG_CMD_OPEN_SESSION              0
> > > > > +#define OPTEE_MSG_CMD_INVOKE_COMMAND            1
> > > > > +#define OPTEE_MSG_CMD_CLOSE_SESSION             2
> > > > > +
> > > > > +#define OPTEE_MSG_ATTR_META                     0x100
> > > > > +
> > > > > +#define TEE_LOGIN_PUBLIC                        0x0
> > >
> > > I'm a bit apprehensive about taking over the whole TEE namespace in an
> > > implementation-specific header file.
> > > Can/should this be OPTEE_?
> > >
> >
> > Ok will change this to OPTEE_.
>
> Thanks!
>
>
> /
>     Leif