On Thu, Mar 12, 2015 at 03:00:56PM +0100, Joakim Bech wrote: [...]
+struct tee_version {
- uint32_t gendrv_major_version;
- uint32_t gendrv_minor_version;
- uint32_t specdrv_major_version;
- uint32_t specdrv_minor_version;
- uint32_t tee_api_major_version;
- uint32_t tee_api_minor_version;
- uint32_t tee_os_major_version;
- uint32_t tee_os_minor_version;
- uint8_t tee_api_uuid[16];
- uint8_t tee_os_uuid[16];
+};
Seems like a lot of different version numbers here. I wonder if we want to expose much more than the TEE API version and eventually the Generic driver version? Can't the other versions be something that the specific driver etc provides themselves?
Also, even though it is convenient to get all this information in one go maybe we should consider reducing the struct to only contain one major and one minor value and then instead add a flag/type that tells which kind of version you are asking for.
struct tee_version { uint32_t major_version; uint32_t minor_version; uint64_t type; /* This is huge, but it's 64bits to make alignment of the struct good */ };
In type, a certain range could be defined (by us when creating this generic driver) and another range could be implementation defined. This would of course require the caller to do several calls instead of a single call to figure out misc versions, that is the drawback.
What about reducing it to: struct tee_version { uint32_t gen_version; uint32_t spec_version; uint8_t spec_uuid[16]; };
This way the client can still identify the specific driver. The reason I'd like to keep the uuid is because that's what the specific driver will be looking for when identifying the secure OS, at least in the OP-TEE case. Another benefit is that we don't need to keep track of different types of specific drivers in a number since that's handled by the secure OS.
Version and UUID are related, but it would like to separate them apart. UUID is nothing TEE specific, that is something generic stated in RFC4122, which means that the struct we have been using so far is a good choice. I.e,
struct tee_uuid { uint32_t time_low; uint16_t time_mid; uint16_t time_hi_and_version; uint8_t clockseq_and_node[8]; };
Maybe we should consider splitting up clockseq_and_node into clk_seq_hi_res, clk_seq_low and node as in RFC4122?
I did a grep for uuid in the linux kernel, it seems what's used there if not just having it as I wrote above is the two types uuid_le and uuid_be from <linux/uuid.h>. The interface we're defining here is supposed to be a transport layer on which the client lib builds the high level stuff. I think it's overkill to go beyond what the kernel normally uses/does.
A benefit of keeping the uuid as I've defined it (no conversion between byte orders) is that the client lib can see if there's an endian missmatch between linux user space and secure OS. The client lib can then indicate that to the user space application if needed.
-- Regards, Jens