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