public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v4 0/1] Add ArmPkg/Optee library APIs
@ 2018-10-10  5:18 Sumit Garg
  2018-10-10  5:18 ` [PATCH v4 1/1] ArmPkg/OpteeLib: Add APIs to communicate with OP-TEE Sumit Garg
  2018-10-17 12:00 ` [PATCH v4 0/1] Add ArmPkg/Optee library APIs Sumit Garg
  0 siblings, 2 replies; 9+ messages in thread
From: Sumit Garg @ 2018-10-10  5:18 UTC (permalink / raw)
  To: edk2-devel
  Cc: ard.biesheuvel, leif.lindholm, michael.d.kinney, tee-dev,
	daniel.thompson, joakim.bech, Matteo.Carlini, Achin.Gupta,
	udit.kumar, Sumit Garg

Changes in v4:
Replaced abbreviations with full name which are not defined in [1]. Also
used EFI_GUID for Trusted Application UUIDs.

[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

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/Library/OpteeLib/OpteeLib.inf |   2 +
 ArmPkg/Include/Library/OpteeLib.h    |  88 +++++
 ArmPkg/Library/OpteeLib/OpteeSmc.h   |  43 +++
 ArmPkg/Library/OpteeLib/Optee.c      | 397 ++++++++++++++++++++
 4 files changed, 530 insertions(+)
 create mode 100644 ArmPkg/Library/OpteeLib/OpteeSmc.h

-- 
2.7.4



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v4 1/1] ArmPkg/OpteeLib: Add APIs to communicate with OP-TEE
  2018-10-10  5:18 [PATCH v4 0/1] Add ArmPkg/Optee library APIs Sumit Garg
@ 2018-10-10  5:18 ` Sumit Garg
  2018-10-18  6:23   ` Leif Lindholm
  2018-10-17 12:00 ` [PATCH v4 0/1] Add ArmPkg/Optee library APIs Sumit Garg
  1 sibling, 1 reply; 9+ messages in thread
From: Sumit Garg @ 2018-10-10  5:18 UTC (permalink / raw)
  To: edk2-devel
  Cc: ard.biesheuvel, leif.lindholm, michael.d.kinney, tee-dev,
	daniel.thompson, joakim.bech, Matteo.Carlini, Achin.Gupta,
	udit.kumar, 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/Library/OpteeLib/OpteeLib.inf |   2 +
 ArmPkg/Include/Library/OpteeLib.h    |  88 +++++
 ArmPkg/Library/OpteeLib/OpteeSmc.h   |  43 +++
 ArmPkg/Library/OpteeLib/Optee.c      | 397 ++++++++++++++++++++
 4 files changed, 530 insertions(+)

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/Include/Library/OpteeLib.h b/ArmPkg/Include/Library/OpteeLib.h
index f65d8674d9b8..6884d5681831 100644
--- a/ArmPkg/Include/Library/OpteeLib.h
+++ b/ArmPkg/Include/Library/OpteeLib.h
@@ -25,10 +25,98 @@
 #define OPTEE_OS_UID2          0xaf630002
 #define OPTEE_OS_UID3          0xa5d5c51b
 
+#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_NONE                0x0
+#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_VALUE_INPUT         0x1
+#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_VALUE_OUTPUT        0x2
+#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_VALUE_INOUT         0x3
+#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_MEMORY_INPUT        0x9
+#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_MEMORY_OUTPUT       0xa
+#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_MEMORY_INOUT        0xb
+
+#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_MASK                0xff
+
+#define OPTEE_ORIGIN_COMMUNICATION              0x00000002
+#define OPTEE_ERROR_COMMUNICATION               0xFFFF000E
+
+typedef struct {
+  UINT64    BufferAddress;
+  UINT64    Size;
+  UINT64    SharedMemoryReference;
+} OPTEE_MESSAGE_PARAM_MEMORY;
+
+typedef struct {
+  UINT64    A;
+  UINT64    B;
+  UINT64    C;
+} OPTEE_MESSAGE_PARAM_VALUE;
+
+typedef struct {
+  UINT64 Attribute;
+  union {
+    OPTEE_MESSAGE_PARAM_MEMORY   Memory;
+    OPTEE_MESSAGE_PARAM_VALUE    Value;
+  } Union;
+} OPTEE_MESSAGE_PARAM;
+
+#define OPTEE_MAX_CALL_PARAMS       4
+
+typedef struct {
+  UINT32    Command;
+  UINT32    Function;
+  UINT32    Session;
+  UINT32    CancelId;
+  UINT32    Pad;
+  UINT32    Return;
+  UINT32    ReturnOrigin;
+  UINT32    NumParams;
+
+  // NumParams tells the actual number of element in Params
+  OPTEE_MESSAGE_PARAM  Params[OPTEE_MAX_CALL_PARAMS];
+} OPTEE_MESSAGE_ARG;
+
+typedef struct {
+  EFI_GUID  Uuid;           // [in] GUID/UUID of the Trusted Application
+  UINT32    Session;        // [out] Session id
+  UINT32    Return;         // [out] Return value
+  UINT32    ReturnOrigin;   // [out] Origin of the return value
+} OPTEE_OPEN_SESSION_ARG;
+
+typedef struct {
+  UINT32    Function;       // [in] Trusted Application function, specific to the TA
+  UINT32    Session;        // [in] Session id
+  UINT32    Return;         // [out] Return value
+  UINT32    ReturnOrigin;   // [out] Origin of the return value
+  OPTEE_MESSAGE_PARAM  Params[OPTEE_MAX_CALL_PARAMS]; // Params for function to be invoked
+} OPTEE_INVOKE_FUNCTION_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
+OpteeInvokeFunction (
+  IN OUT OPTEE_INVOKE_FUNCTION_ARG       *InvokeFunctionArg
+  );
+
 #endif
diff --git a/ArmPkg/Library/OpteeLib/OpteeSmc.h b/ArmPkg/Library/OpteeLib/OpteeSmc.h
new file mode 100644
index 000000000000..21ff4b22ab92
--- /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_SHARED_MEMORY_CONFIG      0xb2000007
+
+#define OPTEE_SMC_SHARED_MEMORY_CACHED          1
+
+#define OPTEE_SMC_RETURN_RPC_FOREIGN_INTERRUPT  0xffff0004
+
+#define OPTEE_MESSAGE_COMMAND_OPEN_SESSION      0
+#define OPTEE_MESSAGE_COMMAND_INVOKE_FUNCTION   1
+#define OPTEE_MESSAGE_COMMAND_CLOSE_SESSION     2
+
+#define OPTEE_MESSAGE_ATTRIBUTE_META            0x100
+
+#define OPTEE_LOGIN_PUBLIC                      0x0
+
+typedef struct {
+  UINTN    Base;
+  UINTN    Size;
+} OPTEE_SHARED_MEMORY_INFORMATION;
+
+#endif
diff --git a/ArmPkg/Library/OpteeLib/Optee.c b/ArmPkg/Library/OpteeLib/Optee.c
index 574527f8b5ea..6617126e8bdb 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_INFORMATION OpteeSharedMemoryInformation = { 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,392 @@ IsOpteePresent (
     return FALSE;
   }
 }
+
+STATIC
+EFI_STATUS
+OpteeSharedMemoryRemap (
+  VOID
+  )
+{
+  ARM_SMC_ARGS                 ArmSmcArgs;
+  EFI_PHYSICAL_ADDRESS         PhysicalAddress;
+  EFI_PHYSICAL_ADDRESS         Start;
+  EFI_PHYSICAL_ADDRESS         End;
+  EFI_STATUS                   Status;
+  UINTN                        Size;
+
+  ZeroMem (&ArmSmcArgs, sizeof (ARM_SMC_ARGS));
+  ArmSmcArgs.Arg0 = OPTEE_SMC_GET_SHARED_MEMORY_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_SHARED_MEMORY_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);
+  PhysicalAddress = 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 (PhysicalAddress, Size, EFI_MEMORY_WB);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  OpteeSharedMemoryInformation.Base = (UINTN)PhysicalAddress;
+  OpteeSharedMemoryInformation.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 = OpteeSharedMemoryRemap ();
+  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]  PhysicalArg   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 PhysicalArg
+  )
+{
+  ARM_SMC_ARGS ArmSmcArgs;
+
+  ZeroMem (&ArmSmcArgs, sizeof (ARM_SMC_ARGS));
+  ArmSmcArgs.Arg0 = OPTEE_SMC_CALL_WITH_ARG;
+  ArmSmcArgs.Arg1 = (UINT32)(PhysicalArg >> 32);
+  ArmSmcArgs.Arg2 = (UINT32)PhysicalArg;
+
+  while (TRUE) {
+    ArmCallSmc (&ArmSmcArgs);
+
+    if (ArmSmcArgs.Arg0 == OPTEE_SMC_RETURN_RPC_FOREIGN_INTERRUPT) {
+      //
+      // 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;
+}
+
+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));
+}
+
+EFI_STATUS
+EFIAPI
+OpteeOpenSession (
+  IN OUT OPTEE_OPEN_SESSION_ARG      *OpenSessionArg
+  )
+{
+  OPTEE_MESSAGE_ARG    *MessageArg;
+
+  MessageArg = NULL;
+
+  if (OpteeSharedMemoryInformation.Base == 0) {
+    DEBUG ((DEBUG_WARN, "OP-TEE not initialized\n"));
+    return EFI_NOT_STARTED;
+  }
+
+  MessageArg = (OPTEE_MESSAGE_ARG *)OpteeSharedMemoryInformation.Base;
+  ZeroMem (MessageArg, sizeof (OPTEE_MESSAGE_ARG));
+
+  MessageArg->Command = OPTEE_MESSAGE_COMMAND_OPEN_SESSION;
+
+  //
+  // Initialize and add the meta parameters needed when opening a
+  // session.
+  //
+  MessageArg->Params[0].Attribute = OPTEE_MESSAGE_ATTRIBUTE_TYPE_VALUE_INPUT |
+                                    OPTEE_MESSAGE_ATTRIBUTE_META;
+  MessageArg->Params[1].Attribute = OPTEE_MESSAGE_ATTRIBUTE_TYPE_VALUE_INPUT |
+                                    OPTEE_MESSAGE_ATTRIBUTE_META;
+  UuidToOctets (
+    (UINT8 *)&MessageArg->Params[0].Union.Value,
+    &OpenSessionArg->Uuid
+    );
+  ZeroMem (&MessageArg->Params[1].Union.Value, sizeof (EFI_GUID));
+  MessageArg->Params[1].Union.Value.C = OPTEE_LOGIN_PUBLIC;
+
+  MessageArg->NumParams = 2;
+
+  if (OpteeCallWithArg ((EFI_PHYSICAL_ADDRESS)MessageArg)) {
+    MessageArg->Return = OPTEE_ERROR_COMMUNICATION;
+    MessageArg->ReturnOrigin = OPTEE_ORIGIN_COMMUNICATION;
+  }
+
+  OpenSessionArg->Session = MessageArg->Session;
+  OpenSessionArg->Return = MessageArg->Return;
+  OpenSessionArg->ReturnOrigin = MessageArg->ReturnOrigin;
+
+  return EFI_SUCCESS;
+}
+
+EFI_STATUS
+EFIAPI
+OpteeCloseSession (
+  IN UINT32                  Session
+  )
+{
+  OPTEE_MESSAGE_ARG    *MessageArg;
+
+  MessageArg = NULL;
+
+  if (OpteeSharedMemoryInformation.Base == 0) {
+    DEBUG ((DEBUG_WARN, "OP-TEE not initialized\n"));
+    return EFI_NOT_STARTED;
+  }
+
+  MessageArg = (OPTEE_MESSAGE_ARG *)OpteeSharedMemoryInformation.Base;
+  ZeroMem (MessageArg, sizeof (OPTEE_MESSAGE_ARG));
+
+  MessageArg->Command = OPTEE_MESSAGE_COMMAND_CLOSE_SESSION;
+  MessageArg->Session = Session;
+
+  OpteeCallWithArg ((EFI_PHYSICAL_ADDRESS)MessageArg);
+
+  return EFI_SUCCESS;
+}
+
+STATIC
+EFI_STATUS
+OpteeToMessageParam (
+  OUT OPTEE_MESSAGE_PARAM    *MessageParams,
+  IN UINT32                  NumParams,
+  IN OPTEE_MESSAGE_PARAM     *InParams
+  )
+{
+  UINT32                  Idx;
+  UINTN                   ParamSharedMemoryAddress;
+  UINTN                   SharedMemorySize;
+  UINTN                   Size;
+
+  Size = (sizeof (OPTEE_MESSAGE_ARG) + sizeof (UINT64) - 1) &
+          ~(sizeof (UINT64) - 1);
+  ParamSharedMemoryAddress = OpteeSharedMemoryInformation.Base + Size;
+  SharedMemorySize = OpteeSharedMemoryInformation.Size - Size;
+
+  for (Idx = 0; Idx < NumParams; Idx++) {
+    CONST OPTEE_MESSAGE_PARAM    *InParam;
+    OPTEE_MESSAGE_PARAM          *MessageParam;
+    UINT32                       Attribute;
+
+    InParam = InParams + Idx;
+    MessageParam = MessageParams + Idx;
+    Attribute = InParam->Attribute & OPTEE_MESSAGE_ATTRIBUTE_TYPE_MASK;
+
+    switch (Attribute) {
+    case OPTEE_MESSAGE_ATTRIBUTE_TYPE_NONE:
+      MessageParam->Attribute = OPTEE_MESSAGE_ATTRIBUTE_TYPE_NONE;
+      ZeroMem (&MessageParam->Union, sizeof (MessageParam->Union));
+      break;
+
+    case OPTEE_MESSAGE_ATTRIBUTE_TYPE_VALUE_INPUT:
+    case OPTEE_MESSAGE_ATTRIBUTE_TYPE_VALUE_OUTPUT:
+    case OPTEE_MESSAGE_ATTRIBUTE_TYPE_VALUE_INOUT:
+      MessageParam->Attribute = Attribute;
+      MessageParam->Union.Value.A = InParam->Union.Value.A;
+      MessageParam->Union.Value.B = InParam->Union.Value.B;
+      MessageParam->Union.Value.C = InParam->Union.Value.C;
+      break;
+
+    case OPTEE_MESSAGE_ATTRIBUTE_TYPE_MEMORY_INPUT:
+    case OPTEE_MESSAGE_ATTRIBUTE_TYPE_MEMORY_OUTPUT:
+    case OPTEE_MESSAGE_ATTRIBUTE_TYPE_MEMORY_INOUT:
+      MessageParam->Attribute = Attribute;
+
+      if (InParam->Union.Memory.Size > SharedMemorySize) {
+        return EFI_OUT_OF_RESOURCES;
+      }
+
+      CopyMem (
+        (VOID *)ParamSharedMemoryAddress,
+        (VOID *)InParam->Union.Memory.BufferAddress,
+        InParam->Union.Memory.Size
+        );
+      MessageParam->Union.Memory.BufferAddress = (UINT64)ParamSharedMemoryAddress;
+      MessageParam->Union.Memory.Size = InParam->Union.Memory.Size;
+
+      Size = (InParam->Union.Memory.Size + sizeof (UINT64) - 1) &
+              ~(sizeof (UINT64) - 1);
+      ParamSharedMemoryAddress += Size;
+      SharedMemorySize -= Size;
+      break;
+
+    default:
+      return EFI_INVALID_PARAMETER;
+    }
+  }
+
+  return EFI_SUCCESS;
+}
+
+STATIC
+EFI_STATUS
+OpteeFromMessageParam (
+  OUT OPTEE_MESSAGE_PARAM    *OutParams,
+  IN UINT32                  NumParams,
+  IN OPTEE_MESSAGE_PARAM     *MessageParams
+  )
+{
+  UINT32                 Idx;
+
+  for (Idx = 0; Idx < NumParams; Idx++) {
+    OPTEE_MESSAGE_PARAM          *OutParam;
+    CONST OPTEE_MESSAGE_PARAM    *MessageParam;
+    UINT32                   Attribute;
+
+    OutParam = OutParams + Idx;
+    MessageParam = MessageParams + Idx;
+    Attribute = MessageParam->Attribute & OPTEE_MESSAGE_ATTRIBUTE_TYPE_MASK;
+
+    switch (Attribute) {
+    case OPTEE_MESSAGE_ATTRIBUTE_TYPE_NONE:
+      OutParam->Attribute = OPTEE_MESSAGE_ATTRIBUTE_TYPE_NONE;
+      ZeroMem (&OutParam->Union, sizeof (OutParam->Union));
+      break;
+
+    case OPTEE_MESSAGE_ATTRIBUTE_TYPE_VALUE_INPUT:
+    case OPTEE_MESSAGE_ATTRIBUTE_TYPE_VALUE_OUTPUT:
+    case OPTEE_MESSAGE_ATTRIBUTE_TYPE_VALUE_INOUT:
+      OutParam->Attribute = Attribute;
+      OutParam->Union.Value.A = MessageParam->Union.Value.A;
+      OutParam->Union.Value.B = MessageParam->Union.Value.B;
+      OutParam->Union.Value.C = MessageParam->Union.Value.C;
+      break;
+
+    case OPTEE_MESSAGE_ATTRIBUTE_TYPE_MEMORY_INPUT:
+    case OPTEE_MESSAGE_ATTRIBUTE_TYPE_MEMORY_OUTPUT:
+    case OPTEE_MESSAGE_ATTRIBUTE_TYPE_MEMORY_INOUT:
+      OutParam->Attribute = Attribute;
+
+      if (MessageParam->Union.Memory.Size > OutParam->Union.Memory.Size) {
+        return EFI_BAD_BUFFER_SIZE;
+      }
+
+      CopyMem (
+        (VOID *)OutParam->Union.Memory.BufferAddress,
+        (VOID *)MessageParam->Union.Memory.BufferAddress,
+        MessageParam->Union.Memory.Size
+        );
+      OutParam->Union.Memory.Size = MessageParam->Union.Memory.Size;
+      break;
+
+    default:
+      return EFI_INVALID_PARAMETER;
+    }
+  }
+
+  return EFI_SUCCESS;
+}
+
+EFI_STATUS
+EFIAPI
+OpteeInvokeFunction (
+  IN OUT OPTEE_INVOKE_FUNCTION_ARG       *InvokeFunctionArg
+  )
+{
+  EFI_STATUS       Status;
+  OPTEE_MESSAGE_ARG    *MessageArg;
+
+  MessageArg = NULL;
+
+  if (OpteeSharedMemoryInformation.Base == 0) {
+    DEBUG ((DEBUG_WARN, "OP-TEE not initialized\n"));
+    return EFI_NOT_STARTED;
+  }
+
+  MessageArg = (OPTEE_MESSAGE_ARG *)OpteeSharedMemoryInformation.Base;
+  ZeroMem (MessageArg, sizeof (OPTEE_MESSAGE_ARG));
+
+  MessageArg->Command = OPTEE_MESSAGE_COMMAND_INVOKE_FUNCTION;
+  MessageArg->Function = InvokeFunctionArg->Function;
+  MessageArg->Session = InvokeFunctionArg->Session;
+
+  Status = OpteeToMessageParam (
+             MessageArg->Params,
+             OPTEE_MAX_CALL_PARAMS,
+             InvokeFunctionArg->Params
+             );
+  if (Status) {
+    return Status;
+  }
+
+  MessageArg->NumParams = OPTEE_MAX_CALL_PARAMS;
+
+  if (OpteeCallWithArg ((EFI_PHYSICAL_ADDRESS)MessageArg)) {
+    MessageArg->Return = OPTEE_ERROR_COMMUNICATION;
+    MessageArg->ReturnOrigin = OPTEE_ORIGIN_COMMUNICATION;
+  }
+
+  if (OpteeFromMessageParam (
+        InvokeFunctionArg->Params,
+        OPTEE_MAX_CALL_PARAMS,
+        MessageArg->Params
+        )) {
+    MessageArg->Return = OPTEE_ERROR_COMMUNICATION;
+    MessageArg->ReturnOrigin = OPTEE_ORIGIN_COMMUNICATION;
+  }
+
+  InvokeFunctionArg->Return = MessageArg->Return;
+  InvokeFunctionArg->ReturnOrigin = MessageArg->ReturnOrigin;
+
+  return EFI_SUCCESS;
+}
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v4 0/1] Add ArmPkg/Optee library APIs
  2018-10-10  5:18 [PATCH v4 0/1] Add ArmPkg/Optee library APIs Sumit Garg
  2018-10-10  5:18 ` [PATCH v4 1/1] ArmPkg/OpteeLib: Add APIs to communicate with OP-TEE Sumit Garg
@ 2018-10-17 12:00 ` Sumit Garg
  1 sibling, 0 replies; 9+ messages in thread
From: Sumit Garg @ 2018-10-17 12:00 UTC (permalink / raw)
  To: Ard Biesheuvel, Leif Lindholm
  Cc: edk2-devel, Michael D Kinney, tee-dev, Daniel Thompson,
	Joakim Bech, Matteo.Carlini, Achin.Gupta, udit.kumar

Gentle reminder.

Please let me know if you have any further review comments.

-Sumit

On Wed, 10 Oct 2018 at 10:49, Sumit Garg <sumit.garg@linaro.org> wrote:
>
> Changes in v4:
> Replaced abbreviations with full name which are not defined in [1]. Also
> used EFI_GUID for Trusted Application UUIDs.
>
> [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
>
> 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/Library/OpteeLib/OpteeLib.inf |   2 +
>  ArmPkg/Include/Library/OpteeLib.h    |  88 +++++
>  ArmPkg/Library/OpteeLib/OpteeSmc.h   |  43 +++
>  ArmPkg/Library/OpteeLib/Optee.c      | 397 ++++++++++++++++++++
>  4 files changed, 530 insertions(+)
>  create mode 100644 ArmPkg/Library/OpteeLib/OpteeSmc.h
>
> --
> 2.7.4
>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4 1/1] ArmPkg/OpteeLib: Add APIs to communicate with OP-TEE
  2018-10-10  5:18 ` [PATCH v4 1/1] ArmPkg/OpteeLib: Add APIs to communicate with OP-TEE Sumit Garg
@ 2018-10-18  6:23   ` Leif Lindholm
  2018-10-18  7:29     ` Sumit Garg
  0 siblings, 1 reply; 9+ messages in thread
From: Leif Lindholm @ 2018-10-18  6:23 UTC (permalink / raw)
  To: Sumit Garg
  Cc: edk2-devel, ard.biesheuvel, michael.d.kinney, tee-dev,
	daniel.thompson, joakim.bech, Matteo.Carlini, Achin.Gupta,
	udit.kumar

Hi Sumit,

I have some further comments/suggestions on UUID/GUID handling below.

On Wed, Oct 10, 2018 at 10:48:53AM +0530, Sumit Garg 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>
> ---
>  ArmPkg/Library/OpteeLib/OpteeLib.inf |   2 +
>  ArmPkg/Include/Library/OpteeLib.h    |  88 +++++
>  ArmPkg/Library/OpteeLib/OpteeSmc.h   |  43 +++
>  ArmPkg/Library/OpteeLib/Optee.c      | 397 ++++++++++++++++++++
>  4 files changed, 530 insertions(+)
> 
> 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/Include/Library/OpteeLib.h b/ArmPkg/Include/Library/OpteeLib.h
> index f65d8674d9b8..6884d5681831 100644
> --- a/ArmPkg/Include/Library/OpteeLib.h
> +++ b/ArmPkg/Include/Library/OpteeLib.h
> @@ -25,10 +25,98 @@
>  #define OPTEE_OS_UID2          0xaf630002
>  #define OPTEE_OS_UID3          0xa5d5c51b
>  
> +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_NONE                0x0
> +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_VALUE_INPUT         0x1
> +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_VALUE_OUTPUT        0x2
> +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_VALUE_INOUT         0x3
> +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_MEMORY_INPUT        0x9
> +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_MEMORY_OUTPUT       0xa
> +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_MEMORY_INOUT        0xb
> +
> +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_MASK                0xff
> +
> +#define OPTEE_ORIGIN_COMMUNICATION              0x00000002
> +#define OPTEE_ERROR_COMMUNICATION               0xFFFF000E
> +
> +typedef struct {
> +  UINT64    BufferAddress;
> +  UINT64    Size;
> +  UINT64    SharedMemoryReference;
> +} OPTEE_MESSAGE_PARAM_MEMORY;
> +
> +typedef struct {
> +  UINT64    A;
> +  UINT64    B;
> +  UINT64    C;
> +} OPTEE_MESSAGE_PARAM_VALUE;
> +
> +typedef struct {
> +  UINT64 Attribute;
> +  union {
> +    OPTEE_MESSAGE_PARAM_MEMORY   Memory;
> +    OPTEE_MESSAGE_PARAM_VALUE    Value;
> +  } Union;
> +} OPTEE_MESSAGE_PARAM;
> +
> +#define OPTEE_MAX_CALL_PARAMS       4
> +
> +typedef struct {
> +  UINT32    Command;
> +  UINT32    Function;
> +  UINT32    Session;
> +  UINT32    CancelId;
> +  UINT32    Pad;
> +  UINT32    Return;
> +  UINT32    ReturnOrigin;
> +  UINT32    NumParams;
> +
> +  // NumParams tells the actual number of element in Params
> +  OPTEE_MESSAGE_PARAM  Params[OPTEE_MAX_CALL_PARAMS];
> +} OPTEE_MESSAGE_ARG;
> +
> +typedef struct {
> +  EFI_GUID  Uuid;           // [in] GUID/UUID of the Trusted Application

Forward reference: I have a longer comment on UUID/GUID struct usage
below. I believe this field describes a UUID held in the TEE_UUID
struct format on the OpTee side. If so, can we change this to an
OPTEE_UUID (mentioned below).

> +  UINT32    Session;        // [out] Session id
> +  UINT32    Return;         // [out] Return value
> +  UINT32    ReturnOrigin;   // [out] Origin of the return value
> +} OPTEE_OPEN_SESSION_ARG;
> +
> +typedef struct {
> +  UINT32    Function;       // [in] Trusted Application function, specific to the TA
> +  UINT32    Session;        // [in] Session id
> +  UINT32    Return;         // [out] Return value
> +  UINT32    ReturnOrigin;   // [out] Origin of the return value
> +  OPTEE_MESSAGE_PARAM  Params[OPTEE_MAX_CALL_PARAMS]; // Params for function to be invoked
> +} OPTEE_INVOKE_FUNCTION_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
> +OpteeInvokeFunction (
> +  IN OUT OPTEE_INVOKE_FUNCTION_ARG       *InvokeFunctionArg
> +  );
> +
>  #endif
> diff --git a/ArmPkg/Library/OpteeLib/OpteeSmc.h b/ArmPkg/Library/OpteeLib/OpteeSmc.h
> new file mode 100644
> index 000000000000..21ff4b22ab92
> --- /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_SHARED_MEMORY_CONFIG      0xb2000007
> +
> +#define OPTEE_SMC_SHARED_MEMORY_CACHED          1
> +
> +#define OPTEE_SMC_RETURN_RPC_FOREIGN_INTERRUPT  0xffff0004
> +
> +#define OPTEE_MESSAGE_COMMAND_OPEN_SESSION      0
> +#define OPTEE_MESSAGE_COMMAND_INVOKE_FUNCTION   1
> +#define OPTEE_MESSAGE_COMMAND_CLOSE_SESSION     2
> +
> +#define OPTEE_MESSAGE_ATTRIBUTE_META            0x100
> +
> +#define OPTEE_LOGIN_PUBLIC                      0x0
> +
> +typedef struct {
> +  UINTN    Base;
> +  UINTN    Size;
> +} OPTEE_SHARED_MEMORY_INFORMATION;
> +
> +#endif
> diff --git a/ArmPkg/Library/OpteeLib/Optee.c b/ArmPkg/Library/OpteeLib/Optee.c
> index 574527f8b5ea..6617126e8bdb 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_INFORMATION OpteeSharedMemoryInformation = { 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,392 @@ IsOpteePresent (
>      return FALSE;
>    }
>  }
> +
> +STATIC
> +EFI_STATUS
> +OpteeSharedMemoryRemap (
> +  VOID
> +  )
> +{
> +  ARM_SMC_ARGS                 ArmSmcArgs;
> +  EFI_PHYSICAL_ADDRESS         PhysicalAddress;
> +  EFI_PHYSICAL_ADDRESS         Start;
> +  EFI_PHYSICAL_ADDRESS         End;
> +  EFI_STATUS                   Status;
> +  UINTN                        Size;
> +
> +  ZeroMem (&ArmSmcArgs, sizeof (ARM_SMC_ARGS));
> +  ArmSmcArgs.Arg0 = OPTEE_SMC_GET_SHARED_MEMORY_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_SHARED_MEMORY_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);
> +  PhysicalAddress = 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 (PhysicalAddress, Size, EFI_MEMORY_WB);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  OpteeSharedMemoryInformation.Base = (UINTN)PhysicalAddress;
> +  OpteeSharedMemoryInformation.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 = OpteeSharedMemoryRemap ();
> +  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]  PhysicalArg   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 PhysicalArg
> +  )
> +{
> +  ARM_SMC_ARGS ArmSmcArgs;
> +
> +  ZeroMem (&ArmSmcArgs, sizeof (ARM_SMC_ARGS));
> +  ArmSmcArgs.Arg0 = OPTEE_SMC_CALL_WITH_ARG;
> +  ArmSmcArgs.Arg1 = (UINT32)(PhysicalArg >> 32);
> +  ArmSmcArgs.Arg2 = (UINT32)PhysicalArg;
> +
> +  while (TRUE) {
> +    ArmCallSmc (&ArmSmcArgs);
> +
> +    if (ArmSmcArgs.Arg0 == OPTEE_SMC_RETURN_RPC_FOREIGN_INTERRUPT) {
> +      //
> +      // 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;
> +}
> +

So, looking at the OpTee sources, TEE_UUID is defined as a struct, to
exactly the same layout as the EFI_GUID type (which is a typedef of
the GUID struct). Could we add a OPTEE_UUID typedef for the same
struct in OpteeLib.h?

Since it comes in as an OPTEE_MESSAGE_PARAM_VALUE, alignment is
already guaranteed to be 64-bit.

(This also deserves a comment explaining how EFI_GUID basically
follows rfc4122, but uses little-endian for the timestamp fields.)

> +STATIC
> +VOID
> +UuidToOctets (

EfiGuidToOpteeUuid would be a better.

> +  OUT UINT8              *UuidOctet,

Rename Uuid?

> +  IN EFI_GUID            *Uuid

Guid?

> +  )
> +{
> +  UuidOctet[0] = Uuid->Data1 >> 24;
> +  UuidOctet[1] = Uuid->Data1 >> 16;
> +  UuidOctet[2] = Uuid->Data1 >> 8;
> +  UuidOctet[3] = Uuid->Data1;

Then we could do
  Uuid->Data1 = SwapBytes32 (Guid->Data1);

> +  UuidOctet[4] = Uuid->Data2 >> 8;
> +  UuidOctet[5] = Uuid->Data2;

  Uuid->Data2 = SwapBytes16 (Guid->Data2);

> +  UuidOctet[6] = Uuid->Data3 >> 8;
> +  UuidOctet[7] = Uuid->Data3;

  Uuid->Data3 = SwapBytes16 (Guid->Data3);

> +  CopyMem (UuidOctet + 8, Uuid->Data4, sizeof (Uuid->Data4));

  CopyMem (Uuid->Data4, Guid->Data4, sizeof (Uuid->Data4));

/
    Leif


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4 1/1] ArmPkg/OpteeLib: Add APIs to communicate with OP-TEE
  2018-10-18  6:23   ` Leif Lindholm
@ 2018-10-18  7:29     ` Sumit Garg
  2018-10-18  8:34       ` Leif Lindholm
  0 siblings, 1 reply; 9+ messages in thread
From: Sumit Garg @ 2018-10-18  7:29 UTC (permalink / raw)
  To: Leif Lindholm
  Cc: edk2-devel, Ard Biesheuvel, Michael D Kinney, tee-dev,
	Daniel Thompson, Joakim Bech, Matteo.Carlini, Achin.Gupta,
	udit.kumar

Hi Leif,

On Thu, 18 Oct 2018 at 11:53, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> Hi Sumit,
>
> I have some further comments/suggestions on UUID/GUID handling below.
>
> On Wed, Oct 10, 2018 at 10:48:53AM +0530, Sumit Garg 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>
> > ---
> >  ArmPkg/Library/OpteeLib/OpteeLib.inf |   2 +
> >  ArmPkg/Include/Library/OpteeLib.h    |  88 +++++
> >  ArmPkg/Library/OpteeLib/OpteeSmc.h   |  43 +++
> >  ArmPkg/Library/OpteeLib/Optee.c      | 397 ++++++++++++++++++++
> >  4 files changed, 530 insertions(+)
> >
> > 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/Include/Library/OpteeLib.h b/ArmPkg/Include/Library/OpteeLib.h
> > index f65d8674d9b8..6884d5681831 100644
> > --- a/ArmPkg/Include/Library/OpteeLib.h
> > +++ b/ArmPkg/Include/Library/OpteeLib.h
> > @@ -25,10 +25,98 @@
> >  #define OPTEE_OS_UID2          0xaf630002
> >  #define OPTEE_OS_UID3          0xa5d5c51b
> >
> > +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_NONE                0x0
> > +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_VALUE_INPUT         0x1
> > +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_VALUE_OUTPUT        0x2
> > +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_VALUE_INOUT         0x3
> > +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_MEMORY_INPUT        0x9
> > +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_MEMORY_OUTPUT       0xa
> > +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_MEMORY_INOUT        0xb
> > +
> > +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_MASK                0xff
> > +
> > +#define OPTEE_ORIGIN_COMMUNICATION              0x00000002
> > +#define OPTEE_ERROR_COMMUNICATION               0xFFFF000E
> > +
> > +typedef struct {
> > +  UINT64    BufferAddress;
> > +  UINT64    Size;
> > +  UINT64    SharedMemoryReference;
> > +} OPTEE_MESSAGE_PARAM_MEMORY;
> > +
> > +typedef struct {
> > +  UINT64    A;
> > +  UINT64    B;
> > +  UINT64    C;
> > +} OPTEE_MESSAGE_PARAM_VALUE;
> > +
> > +typedef struct {
> > +  UINT64 Attribute;
> > +  union {
> > +    OPTEE_MESSAGE_PARAM_MEMORY   Memory;
> > +    OPTEE_MESSAGE_PARAM_VALUE    Value;
> > +  } Union;
> > +} OPTEE_MESSAGE_PARAM;
> > +
> > +#define OPTEE_MAX_CALL_PARAMS       4
> > +
> > +typedef struct {
> > +  UINT32    Command;
> > +  UINT32    Function;
> > +  UINT32    Session;
> > +  UINT32    CancelId;
> > +  UINT32    Pad;
> > +  UINT32    Return;
> > +  UINT32    ReturnOrigin;
> > +  UINT32    NumParams;
> > +
> > +  // NumParams tells the actual number of element in Params
> > +  OPTEE_MESSAGE_PARAM  Params[OPTEE_MAX_CALL_PARAMS];
> > +} OPTEE_MESSAGE_ARG;
> > +
> > +typedef struct {
> > +  EFI_GUID  Uuid;           // [in] GUID/UUID of the Trusted Application
>
> Forward reference: I have a longer comment on UUID/GUID struct usage
> below. I believe this field describes a UUID held in the TEE_UUID
> struct format on the OpTee side. If so, can we change this to an
> OPTEE_UUID (mentioned below).
>
> > +  UINT32    Session;        // [out] Session id
> > +  UINT32    Return;         // [out] Return value
> > +  UINT32    ReturnOrigin;   // [out] Origin of the return value
> > +} OPTEE_OPEN_SESSION_ARG;
> > +
> > +typedef struct {
> > +  UINT32    Function;       // [in] Trusted Application function, specific to the TA
> > +  UINT32    Session;        // [in] Session id
> > +  UINT32    Return;         // [out] Return value
> > +  UINT32    ReturnOrigin;   // [out] Origin of the return value
> > +  OPTEE_MESSAGE_PARAM  Params[OPTEE_MAX_CALL_PARAMS]; // Params for function to be invoked
> > +} OPTEE_INVOKE_FUNCTION_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
> > +OpteeInvokeFunction (
> > +  IN OUT OPTEE_INVOKE_FUNCTION_ARG       *InvokeFunctionArg
> > +  );
> > +
> >  #endif
> > diff --git a/ArmPkg/Library/OpteeLib/OpteeSmc.h b/ArmPkg/Library/OpteeLib/OpteeSmc.h
> > new file mode 100644
> > index 000000000000..21ff4b22ab92
> > --- /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_SHARED_MEMORY_CONFIG      0xb2000007
> > +
> > +#define OPTEE_SMC_SHARED_MEMORY_CACHED          1
> > +
> > +#define OPTEE_SMC_RETURN_RPC_FOREIGN_INTERRUPT  0xffff0004
> > +
> > +#define OPTEE_MESSAGE_COMMAND_OPEN_SESSION      0
> > +#define OPTEE_MESSAGE_COMMAND_INVOKE_FUNCTION   1
> > +#define OPTEE_MESSAGE_COMMAND_CLOSE_SESSION     2
> > +
> > +#define OPTEE_MESSAGE_ATTRIBUTE_META            0x100
> > +
> > +#define OPTEE_LOGIN_PUBLIC                      0x0
> > +
> > +typedef struct {
> > +  UINTN    Base;
> > +  UINTN    Size;
> > +} OPTEE_SHARED_MEMORY_INFORMATION;
> > +
> > +#endif
> > diff --git a/ArmPkg/Library/OpteeLib/Optee.c b/ArmPkg/Library/OpteeLib/Optee.c
> > index 574527f8b5ea..6617126e8bdb 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_INFORMATION OpteeSharedMemoryInformation = { 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,392 @@ IsOpteePresent (
> >      return FALSE;
> >    }
> >  }
> > +
> > +STATIC
> > +EFI_STATUS
> > +OpteeSharedMemoryRemap (
> > +  VOID
> > +  )
> > +{
> > +  ARM_SMC_ARGS                 ArmSmcArgs;
> > +  EFI_PHYSICAL_ADDRESS         PhysicalAddress;
> > +  EFI_PHYSICAL_ADDRESS         Start;
> > +  EFI_PHYSICAL_ADDRESS         End;
> > +  EFI_STATUS                   Status;
> > +  UINTN                        Size;
> > +
> > +  ZeroMem (&ArmSmcArgs, sizeof (ARM_SMC_ARGS));
> > +  ArmSmcArgs.Arg0 = OPTEE_SMC_GET_SHARED_MEMORY_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_SHARED_MEMORY_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);
> > +  PhysicalAddress = 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 (PhysicalAddress, Size, EFI_MEMORY_WB);
> > +  if (EFI_ERROR (Status)) {
> > +    return Status;
> > +  }
> > +
> > +  OpteeSharedMemoryInformation.Base = (UINTN)PhysicalAddress;
> > +  OpteeSharedMemoryInformation.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 = OpteeSharedMemoryRemap ();
> > +  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]  PhysicalArg   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 PhysicalArg
> > +  )
> > +{
> > +  ARM_SMC_ARGS ArmSmcArgs;
> > +
> > +  ZeroMem (&ArmSmcArgs, sizeof (ARM_SMC_ARGS));
> > +  ArmSmcArgs.Arg0 = OPTEE_SMC_CALL_WITH_ARG;
> > +  ArmSmcArgs.Arg1 = (UINT32)(PhysicalArg >> 32);
> > +  ArmSmcArgs.Arg2 = (UINT32)PhysicalArg;
> > +
> > +  while (TRUE) {
> > +    ArmCallSmc (&ArmSmcArgs);
> > +
> > +    if (ArmSmcArgs.Arg0 == OPTEE_SMC_RETURN_RPC_FOREIGN_INTERRUPT) {
> > +      //
> > +      // 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;
> > +}
> > +
>
> So, looking at the OpTee sources, TEE_UUID is defined as a struct, to
> exactly the same layout as the EFI_GUID type (which is a typedef of
> the GUID struct). Could we add a OPTEE_UUID typedef for the same
> struct in OpteeLib.h?
>
> Since it comes in as an OPTEE_MESSAGE_PARAM_VALUE, alignment is
> already guaranteed to be 64-bit.
>
> (This also deserves a comment explaining how EFI_GUID basically
> follows rfc4122, but uses little-endian for the timestamp fields.)
>

Actually, OP-TEE also uses little-endian format for timestamp fields.
You can refer to [1] for conversion from network byte order (octets)
to little-endian and vice-versa.

So for communications among secure world and non-secure world it uses
network byte order for UUID/GUID to comply with rfc4122.

[1] https://github.com/OP-TEE/optee_os/blob/master/core/tee/uuid.c

-Sumit

> > +STATIC
> > +VOID
> > +UuidToOctets (
>
> EfiGuidToOpteeUuid would be a better.
>
> > +  OUT UINT8              *UuidOctet,
>
> Rename Uuid?
>
> > +  IN EFI_GUID            *Uuid
>
> Guid?
>
> > +  )
> > +{
> > +  UuidOctet[0] = Uuid->Data1 >> 24;
> > +  UuidOctet[1] = Uuid->Data1 >> 16;
> > +  UuidOctet[2] = Uuid->Data1 >> 8;
> > +  UuidOctet[3] = Uuid->Data1;
>
> Then we could do
>   Uuid->Data1 = SwapBytes32 (Guid->Data1);
>
> > +  UuidOctet[4] = Uuid->Data2 >> 8;
> > +  UuidOctet[5] = Uuid->Data2;
>
>   Uuid->Data2 = SwapBytes16 (Guid->Data2);
>
> > +  UuidOctet[6] = Uuid->Data3 >> 8;
> > +  UuidOctet[7] = Uuid->Data3;
>
>   Uuid->Data3 = SwapBytes16 (Guid->Data3);
>
> > +  CopyMem (UuidOctet + 8, Uuid->Data4, sizeof (Uuid->Data4));
>
>   CopyMem (Uuid->Data4, Guid->Data4, sizeof (Uuid->Data4));
>
> /
>     Leif


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4 1/1] ArmPkg/OpteeLib: Add APIs to communicate with OP-TEE
  2018-10-18  7:29     ` Sumit Garg
@ 2018-10-18  8:34       ` Leif Lindholm
  2018-10-18  9:13         ` Sumit Garg
  0 siblings, 1 reply; 9+ messages in thread
From: Leif Lindholm @ 2018-10-18  8:34 UTC (permalink / raw)
  To: Sumit Garg
  Cc: edk2-devel, Ard Biesheuvel, Michael D Kinney, tee-dev,
	Daniel Thompson, Joakim Bech, Matteo.Carlini, Achin.Gupta,
	udit.kumar

On Thu, Oct 18, 2018 at 12:59:32PM +0530, Sumit Garg wrote:
> > So, looking at the OpTee sources, TEE_UUID is defined as a struct, to
> > exactly the same layout as the EFI_GUID type (which is a typedef of
> > the GUID struct). Could we add a OPTEE_UUID typedef for the same
> > struct in OpteeLib.h?
> >
> > Since it comes in as an OPTEE_MESSAGE_PARAM_VALUE, alignment is
> > already guaranteed to be 64-bit.
> >
> > (This also deserves a comment explaining how EFI_GUID basically
> > follows rfc4122, but uses little-endian for the timestamp fields.)
> 
> Actually, OP-TEE also uses little-endian format for timestamp fields.
> You can refer to [1] for conversion from network byte order (octets)
> to little-endian and vice-versa.
> 
> So for communications among secure world and non-secure world it uses
> network byte order for UUID/GUID to comply with rfc4122.
> 
> [1] https://github.com/OP-TEE/optee_os/blob/master/core/tee/uuid.c

Huh, ok. That's good to know.
It does however not change my comments. Since we're dealing with data
structures of a known layout, I am not a fan of treating them as byte
arrays.

/
    Leif


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4 1/1] ArmPkg/OpteeLib: Add APIs to communicate with OP-TEE
  2018-10-18  8:34       ` Leif Lindholm
@ 2018-10-18  9:13         ` Sumit Garg
  2018-10-18  9:24           ` Leif Lindholm
  0 siblings, 1 reply; 9+ messages in thread
From: Sumit Garg @ 2018-10-18  9:13 UTC (permalink / raw)
  To: Leif Lindholm
  Cc: edk2-devel, Ard Biesheuvel, Michael D Kinney, tee-dev,
	Daniel Thompson, Joakim Bech, Matteo.Carlini, Achin.Gupta,
	udit.kumar

On Thu, 18 Oct 2018 at 14:04, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Thu, Oct 18, 2018 at 12:59:32PM +0530, Sumit Garg wrote:
> > > So, looking at the OpTee sources, TEE_UUID is defined as a struct, to
> > > exactly the same layout as the EFI_GUID type (which is a typedef of
> > > the GUID struct). Could we add a OPTEE_UUID typedef for the same
> > > struct in OpteeLib.h?
> > >
> > > Since it comes in as an OPTEE_MESSAGE_PARAM_VALUE, alignment is
> > > already guaranteed to be 64-bit.
> > >
> > > (This also deserves a comment explaining how EFI_GUID basically
> > > follows rfc4122, but uses little-endian for the timestamp fields.)
> >
> > Actually, OP-TEE also uses little-endian format for timestamp fields.
> > You can refer to [1] for conversion from network byte order (octets)
> > to little-endian and vice-versa.
> >
> > So for communications among secure world and non-secure world it uses
> > network byte order for UUID/GUID to comply with rfc4122.
> >
> > [1] https://github.com/OP-TEE/optee_os/blob/master/core/tee/uuid.c
>
> Huh, ok. That's good to know.
> It does however not change my comments. Since we're dealing with data
> structures of a known layout, I am not a fan of treating them as byte
> arrays.
>

But calling UUID struct with swapped timestamp as OPTEE_UUID would
also be misnomer. I am not sure regarding appropriate naming for that
struct.

On the other hand, we have byte array of 16 octets as per network byte
order complying with rfc4122 which also doesn't imply swapped
timestamp.

-Sumit

> /
>     Leif


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4 1/1] ArmPkg/OpteeLib: Add APIs to communicate with OP-TEE
  2018-10-18  9:13         ` Sumit Garg
@ 2018-10-18  9:24           ` Leif Lindholm
  2018-10-18 10:12             ` Sumit Garg
  0 siblings, 1 reply; 9+ messages in thread
From: Leif Lindholm @ 2018-10-18  9:24 UTC (permalink / raw)
  To: Sumit Garg
  Cc: edk2-devel, Ard Biesheuvel, Michael D Kinney, tee-dev,
	Daniel Thompson, Joakim Bech, Matteo.Carlini, Achin.Gupta,
	udit.kumar

On Thu, Oct 18, 2018 at 02:43:37PM +0530, Sumit Garg wrote:
> On Thu, 18 Oct 2018 at 14:04, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >
> > On Thu, Oct 18, 2018 at 12:59:32PM +0530, Sumit Garg wrote:
> > > > So, looking at the OpTee sources, TEE_UUID is defined as a struct, to
> > > > exactly the same layout as the EFI_GUID type (which is a typedef of
> > > > the GUID struct). Could we add a OPTEE_UUID typedef for the same
> > > > struct in OpteeLib.h?
> > > >
> > > > Since it comes in as an OPTEE_MESSAGE_PARAM_VALUE, alignment is
> > > > already guaranteed to be 64-bit.
> > > >
> > > > (This also deserves a comment explaining how EFI_GUID basically
> > > > follows rfc4122, but uses little-endian for the timestamp fields.)
> > >
> > > Actually, OP-TEE also uses little-endian format for timestamp fields.
> > > You can refer to [1] for conversion from network byte order (octets)
> > > to little-endian and vice-versa.
> > >
> > > So for communications among secure world and non-secure world it uses
> > > network byte order for UUID/GUID to comply with rfc4122.
> > >
> > > [1] https://github.com/OP-TEE/optee_os/blob/master/core/tee/uuid.c
> >
> > Huh, ok. That's good to know.
> > It does however not change my comments. Since we're dealing with data
> > structures of a known layout, I am not a fan of treating them as byte
> > arrays.
> >
> 
> But calling UUID struct with swapped timestamp as OPTEE_UUID would
> also be misnomer. I am not sure regarding appropriate naming for that
> struct.

That's a fair point. We could call it RFC4122_UUID for now.

There could even be a case to add that to BaseLib at some point (but
probably not while there is only one user).

Regards,

Leif

> On the other hand, we have byte array of 16 octets as per network byte
> order complying with rfc4122 which also doesn't imply swapped
> timestamp.
> 
> -Sumit
> 
> > /
> >     Leif


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4 1/1] ArmPkg/OpteeLib: Add APIs to communicate with OP-TEE
  2018-10-18  9:24           ` Leif Lindholm
@ 2018-10-18 10:12             ` Sumit Garg
  0 siblings, 0 replies; 9+ messages in thread
From: Sumit Garg @ 2018-10-18 10:12 UTC (permalink / raw)
  To: Leif Lindholm
  Cc: edk2-devel, Ard Biesheuvel, Michael D Kinney, tee-dev,
	Daniel Thompson, Joakim Bech, Matteo.Carlini, Achin.Gupta,
	udit.kumar

On Thu, 18 Oct 2018 at 14:54, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Thu, Oct 18, 2018 at 02:43:37PM +0530, Sumit Garg wrote:
> > On Thu, 18 Oct 2018 at 14:04, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > >
> > > On Thu, Oct 18, 2018 at 12:59:32PM +0530, Sumit Garg wrote:
> > > > > So, looking at the OpTee sources, TEE_UUID is defined as a struct, to
> > > > > exactly the same layout as the EFI_GUID type (which is a typedef of
> > > > > the GUID struct). Could we add a OPTEE_UUID typedef for the same
> > > > > struct in OpteeLib.h?
> > > > >
> > > > > Since it comes in as an OPTEE_MESSAGE_PARAM_VALUE, alignment is
> > > > > already guaranteed to be 64-bit.
> > > > >
> > > > > (This also deserves a comment explaining how EFI_GUID basically
> > > > > follows rfc4122, but uses little-endian for the timestamp fields.)
> > > >
> > > > Actually, OP-TEE also uses little-endian format for timestamp fields.
> > > > You can refer to [1] for conversion from network byte order (octets)
> > > > to little-endian and vice-versa.
> > > >
> > > > So for communications among secure world and non-secure world it uses
> > > > network byte order for UUID/GUID to comply with rfc4122.
> > > >
> > > > [1] https://github.com/OP-TEE/optee_os/blob/master/core/tee/uuid.c
> > >
> > > Huh, ok. That's good to know.
> > > It does however not change my comments. Since we're dealing with data
> > > structures of a known layout, I am not a fan of treating them as byte
> > > arrays.
> > >
> >
> > But calling UUID struct with swapped timestamp as OPTEE_UUID would
> > also be misnomer. I am not sure regarding appropriate naming for that
> > struct.
>
> That's a fair point. We could call it RFC4122_UUID for now.
>

Ok then in v5 I will define this as internal communication structure
in ArmPkg/Library/OpteeLib/OpteeSmc.h and use it instead in following
manner. Please review it.

diff --git a/ArmPkg/Library/OpteeLib/OpteeSmc.h
b/ArmPkg/Library/OpteeLib/OpteeSmc.h
index 21ff4b22ab92..9cccd81810c9 100644
--- a/ArmPkg/Library/OpteeLib/OpteeSmc.h
+++ b/ArmPkg/Library/OpteeLib/OpteeSmc.h
@@ -40,4 +40,14 @@ typedef struct {
   UINTN    Size;
 } OPTEE_SHARED_MEMORY_INFORMATION;

+//
+// UUID struct compliant with RFC4122 (network byte order).
+//
+typedef struct {
+  UINT32  Data1;
+  UINT16  Data2;
+  UINT16  Data3;
+  UINT8   Data4[8];
+} RFC4122_UUID;
+
 #endif
diff --git a/ArmPkg/Library/OpteeLib/Optee.c b/ArmPkg/Library/OpteeLib/Optee.c
index 6617126e8bdb..8ac31cb28266 100644
--- a/ArmPkg/Library/OpteeLib/Optee.c
+++ b/ArmPkg/Library/OpteeLib/Optee.c
@@ -165,20 +165,15 @@ OpteeCallWithArg (

 STATIC
 VOID
-UuidToOctets (
-  OUT UINT8              *UuidOctet,
-  IN EFI_GUID            *Uuid
+EfiGuidToRfc4122Uuid (
+  OUT RFC4122_UUID       *Rfc4122Uuid,
+  IN EFI_GUID            *Guid
   )
 {
-  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));
+  Rfc4122Uuid->Data1 = SwapBytes32 (Guid->Data1);
+  Rfc4122Uuid->Data2 = SwapBytes16 (Guid->Data2);
+  Rfc4122Uuid->Data3 = SwapBytes16 (Guid->Data3);
+  CopyMem (Rfc4122Uuid->Data4, Guid->Data4, sizeof (Rfc4122Uuid->Data4));
 }

 EFI_STATUS
@@ -209,8 +204,8 @@ OpteeOpenSession (
                                     OPTEE_MESSAGE_ATTRIBUTE_META;
   MessageArg->Params[1].Attribute = OPTEE_MESSAGE_ATTRIBUTE_TYPE_VALUE_INPUT |
                                     OPTEE_MESSAGE_ATTRIBUTE_META;
-  UuidToOctets (
-    (UINT8 *)&MessageArg->Params[0].Union.Value,
+  EfiGuidToRfc4122Uuid (
+    (RFC4122_UUID *)&MessageArg->Params[0].Union.Value,
     &OpenSessionArg->Uuid
     );
   ZeroMem (&MessageArg->Params[1].Union.Value, sizeof (EFI_GUID));

-Sumit

> There could even be a case to add that to BaseLib at some point (but
> probably not while there is only one user).
>
> Regards,
>
> Leif
>
> > On the other hand, we have byte array of 16 octets as per network byte
> > order complying with rfc4122 which also doesn't imply swapped
> > timestamp.
> >
> > -Sumit
> >
> > > /
> > >     Leif


^ permalink raw reply related	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-10-18 10:12 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-10-10  5:18 [PATCH v4 0/1] Add ArmPkg/Optee library APIs Sumit Garg
2018-10-10  5:18 ` [PATCH v4 1/1] ArmPkg/OpteeLib: Add APIs to communicate with OP-TEE Sumit Garg
2018-10-18  6:23   ` Leif Lindholm
2018-10-18  7:29     ` Sumit Garg
2018-10-18  8:34       ` Leif Lindholm
2018-10-18  9:13         ` Sumit Garg
2018-10-18  9:24           ` Leif Lindholm
2018-10-18 10:12             ` Sumit Garg
2018-10-17 12:00 ` [PATCH v4 0/1] Add ArmPkg/Optee library APIs Sumit Garg

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox