Re: [edk2] [PATCH v4 1/1] ArmPkg/OpteeLib: Add APIs to communicate with OP-TEE
On Thu, 18 Oct 2018 at 14:54, Leif Lindholm wrote:
>
> On Thu, Oct 18, 2018 at 02:43:37PM +0530, Sumit Garg wrote:
> > On Thu, 18 Oct 2018 at 14:04, Leif Lindholm
> > 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 {
UINTNSize;
} 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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v4 1/1] ArmPkg/OpteeLib: Add APIs to communicate with OP-TEE
On Thu, Oct 18, 2018 at 02:43:37PM +0530, Sumit Garg wrote: > On Thu, 18 Oct 2018 at 14:04, Leif Lindholm 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 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v4 1/1] ArmPkg/OpteeLib: Add APIs to communicate with OP-TEE
On Thu, 18 Oct 2018 at 14:04, Leif Lindholm 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 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v4 1/1] ArmPkg/OpteeLib: Add APIs to communicate with OP-TEE
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 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v4 1/1] ArmPkg/OpteeLib: Add APIs to communicate with OP-TEE
Hi Leif,
On Thu, 18 Oct 2018 at 11:53, Leif Lindholm 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
> > Cc: Leif Lindholm
> > Cc: Michael D Kinney
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Sumit Garg
> > ---
> > 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_NONE0x0
> > +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_VALUE_INPUT 0x1
> > +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_VALUE_OUTPUT0x2
> > +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_VALUE_INOUT 0x3
> > +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_MEMORY_INPUT0x9
> > +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_MEMORY_OUTPUT 0xa
> > +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_MEMORY_INOUT0xb
> > +
> > +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_MASK0xff
> > +
> > +#define OPTEE_ORIGIN_COMMUNICATION 0x0002
> > +#define OPTEE_ERROR_COMMUNICATION 0x000E
> > +
> > +typedef struct {
> > + UINT64BufferAddress;
> > + UINT64Size;
> > + UINT64SharedMemoryReference;
> > +} OPTEE_MESSAGE_PARAM_MEMORY;
> > +
> > +typedef struct {
> > + UINT64A;
> > + UINT64B;
> > + UINT64C;
> > +} OPTEE_MESSAGE_PARAM_VALUE;
> > +
> > +typedef struct {
> > + UINT64 Attribute;
> > + union {
> > +OPTEE_MESSAGE_PARAM_MEMORY Memory;
> > +OPTEE_MESSAGE_PARAM_VALUEValue;
> > + } Union;
> > +} OPTEE_MESSAGE_PARAM;
> > +
> > +#define OPTEE_MAX_CALL_PARAMS 4
> > +
> > +typedef struct {
> > + UINT32Command;
> > + UINT32Function;
> > + UINT32Session;
> > + UINT32CancelId;
> > + UINT32Pad;
> > + UINT32Return;
> > + UINT32ReturnOrigin;
> > + UINT32NumParams;
> > +
> > + // 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).
>
> > + UINT32Session;// [out] Session id
> > + UINT32Return; // [out] Return value
> > + UINT32ReturnOrigin; // [out] Origin of the return value
> > +} OPTEE_OPEN_SESSION_ARG;
> > +
> > +typedef struct {
> > + UINT32Function; // [in] Trusted Application function, specific
> > to the TA
> > + UINT32Session;// [in] Session id
> > + UINT32Return; // [out] Return value
> > + UINT32ReturnOrigin; // [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 ..21ff4b22ab92
> > --- /dev/null
> > +++ b/ArmPkg/Library/OpteeLib/OpteeSmc.h
> > @@ -0,0 +1,43 @@
>
Re: [edk2] [PATCH v4 1/1] ArmPkg/OpteeLib: Add APIs to communicate with OP-TEE
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
> Cc: Leif Lindholm
> Cc: Michael D Kinney
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Sumit Garg
> ---
> 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_NONE0x0
> +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_VALUE_INPUT 0x1
> +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_VALUE_OUTPUT0x2
> +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_VALUE_INOUT 0x3
> +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_MEMORY_INPUT0x9
> +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_MEMORY_OUTPUT 0xa
> +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_MEMORY_INOUT0xb
> +
> +#define OPTEE_MESSAGE_ATTRIBUTE_TYPE_MASK0xff
> +
> +#define OPTEE_ORIGIN_COMMUNICATION 0x0002
> +#define OPTEE_ERROR_COMMUNICATION 0x000E
> +
> +typedef struct {
> + UINT64BufferAddress;
> + UINT64Size;
> + UINT64SharedMemoryReference;
> +} OPTEE_MESSAGE_PARAM_MEMORY;
> +
> +typedef struct {
> + UINT64A;
> + UINT64B;
> + UINT64C;
> +} OPTEE_MESSAGE_PARAM_VALUE;
> +
> +typedef struct {
> + UINT64 Attribute;
> + union {
> +OPTEE_MESSAGE_PARAM_MEMORY Memory;
> +OPTEE_MESSAGE_PARAM_VALUEValue;
> + } Union;
> +} OPTEE_MESSAGE_PARAM;
> +
> +#define OPTEE_MAX_CALL_PARAMS 4
> +
> +typedef struct {
> + UINT32Command;
> + UINT32Function;
> + UINT32Session;
> + UINT32CancelId;
> + UINT32Pad;
> + UINT32Return;
> + UINT32ReturnOrigin;
> + UINT32NumParams;
> +
> + // 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).
> + UINT32Session;// [out] Session id
> + UINT32Return; // [out] Return value
> + UINT32ReturnOrigin; // [out] Origin of the return value
> +} OPTEE_OPEN_SESSION_ARG;
> +
> +typedef struct {
> + UINT32Function; // [in] Trusted Application function, specific
> to the TA
> + UINT32Session;// [in] Session id
> + UINT32Return; // [out] Return value
> + UINT32ReturnOrigin; // [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 ..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.
> +
> + 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/license
