LinuxLists.cc - [PATCHv8 0/3] optee: register drivers on optee bus

[permalink] [raw]

Subject: [PATCHv8 1/3] optee: use uuid for sysfs driver entry

With the evolving use-cases for TEE bus, now it's required to support
multi-stage enumeration process. But using a simple index doesn't
suffice this requirement and instead leads to duplicate sysfs entries.
So instead switch to use more informative device UUID for sysfs entry
like:
/sys/bus/tee/devices/optee-ta-<uuid>

Signed-off-by: Maxim Uvarov <[email protected]>
Reviewed-by: Sumit Garg <[email protected]>
---
Documentation/ABI/testing/sysfs-bus-optee-devices | 8 ++++++++
MAINTAINERS | 1 +
drivers/tee/optee/device.c | 9 ++++++---
3 files changed, 15 insertions(+), 3 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-bus-optee-devices

diff --git a/Documentation/ABI/testing/sysfs-bus-optee-devices b/Documentation/ABI/testing/sysfs-bus-optee-devices
new file mode 100644
index 000000000000..0ae04ae5374a
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-optee-devices
@@ -0,0 +1,8 @@
+What: /sys/bus/tee/devices/optee-ta-<uuid>/
+Date: May 2020
+KernelVersion 5.7
+Contact: [email protected]
+Description:
+ OP-TEE bus provides reference to registered drivers under this directory. The <uuid>
+ matches Trusted Application (TA) driver and corresponding TA in secure OS. Drivers
+ are free to create needed API under optee-ta-<uuid> directory.
diff --git a/MAINTAINERS b/MAINTAINERS
index ecc0749810b0..6717afef2de3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12516,6 +12516,7 @@ OP-TEE DRIVER
M: Jens Wiklander <[email protected]>
L: [email protected]
S: Maintained
+F: Documentation/ABI/testing/sysfs-bus-optee-devices
F: drivers/tee/optee/

OP-TEE RANDOM NUMBER GENERATOR (RNG) DRIVER
diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
index e3a148521ec1..23d264c8146e 100644
--- a/drivers/tee/optee/device.c
+++ b/drivers/tee/optee/device.c
@@ -65,7 +65,7 @@ static int get_devices(struct tee_context *ctx, u32 session,
return 0;
}

-static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
+static int optee_register_device(const uuid_t *device_uuid)
{
struct tee_client_device *optee_device = NULL;
int rc;
@@ -75,7 +75,10 @@ static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
return -ENOMEM;

optee_device->dev.bus = &tee_bus_type;
- dev_set_name(&optee_device->dev, "optee-clnt%u", device_id);
+ if (dev_set_name(&optee_device->dev, "optee-ta-%pUl", device_uuid)) {
+ kfree(optee_device);
+ return -ENOMEM;
+ }
uuid_copy(&optee_device->id.uuid, device_uuid);

rc = device_register(&optee_device->dev);
@@ -144,7 +147,7 @@ int optee_enumerate_devices(void)
num_devices = shm_size / sizeof(uuid_t);

for (idx = 0; idx < num_devices; idx++) {
- rc = optee_register_device(&device_uuid[idx], idx);
+ rc = optee_register_device(&device_uuid[idx]);
if (rc)
goto out_shm;
}
--
2.17.1

2020-06-04 18:03:45

[permalink] [raw]

Subject: [PATCHv8 2/3] optee: enable support for multi-stage bus enumeration

Some drivers (like ftpm) can operate only after tee-supplicant
runs because of tee-supplicant provides things like storage
services (rpmb, shm). This patch splits probe of non tee-supplicant
dependable drivers to the early stage, and after tee-supplicant run
probe other drivers.

Signed-off-by: Maxim Uvarov <[email protected]>
Suggested-by: Sumit Garg <[email protected]>
Suggested-by: Arnd Bergmann <[email protected]>
Reviewed-by: Sumit Garg <[email protected]>
---
drivers/tee/optee/core.c | 27 ++++++++++++++++++++++++---
drivers/tee/optee/device.c | 29 +++++++++++------------------
drivers/tee/optee/optee_private.h | 10 +++++++++-
3 files changed, 44 insertions(+), 22 deletions(-)

diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index 99698b8a3a74..b373b1b08b6d 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -17,6 +17,7 @@
#include <linux/tee_drv.h>
#include <linux/types.h>
#include <linux/uaccess.h>
+#include <linux/workqueue.h>
#include "optee_private.h"
#include "optee_smc.h"
#include "shm_pool.h"
@@ -218,6 +219,11 @@ static void optee_get_version(struct tee_device *teedev,
*vers = v;
}

+static void optee_bus_scan(struct work_struct *work)
+{
+ WARN_ON(optee_enumerate_devices(PTA_CMD_GET_DEVICES_SUPP));
+}
+
static int optee_open(struct tee_context *ctx)
{
struct optee_context_data *ctxdata;
@@ -241,8 +247,18 @@ static int optee_open(struct tee_context *ctx)
kfree(ctxdata);
return -EBUSY;
}
- }

+ if (!optee->scan_bus_done) {
+ INIT_WORK(&optee->scan_bus_work, optee_bus_scan);
+ optee->scan_bus_wq = create_workqueue("optee_bus_scan");
+ if (!optee->scan_bus_wq) {
+ kfree(ctxdata);
+ return -ECHILD;
+ }
+ queue_work(optee->scan_bus_wq, &optee->scan_bus_work);
+ optee->scan_bus_done = true;
+ }
+ }
mutex_init(&ctxdata->mutex);
INIT_LIST_HEAD(&ctxdata->sess_list);

@@ -296,8 +312,13 @@ static void optee_release(struct tee_context *ctx)

ctx->data = NULL;

- if (teedev == optee->supp_teedev)
+ if (teedev == optee->supp_teedev) {
+ if (optee->scan_bus_wq) {
+ destroy_workqueue(optee->scan_bus_wq);
+ optee->scan_bus_wq = NULL;
+ }
optee_supp_release(&optee->supp);
+ }
}

static const struct tee_driver_ops optee_ops = {
@@ -675,7 +696,7 @@ static int optee_probe(struct platform_device *pdev)

platform_set_drvdata(pdev, optee);

- rc = optee_enumerate_devices();
+ rc = optee_enumerate_devices(PTA_CMD_GET_DEVICES);
if (rc) {
optee_remove(pdev);
return rc;
diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
index 23d264c8146e..19260c5740df 100644
--- a/drivers/tee/optee/device.c
+++ b/drivers/tee/optee/device.c
@@ -11,18 +11,6 @@
#include <linux/uuid.h>
#include "optee_private.h"

-/*
- * Get device UUIDs
- *
- * [out] memref[0] Array of device UUIDs
- *
- * Return codes:
- * TEE_SUCCESS - Invoke command success
- * TEE_ERROR_BAD_PARAMETERS - Incorrect input param
- * TEE_ERROR_SHORT_BUFFER - Output buffer size less than required
- */
-#define PTA_CMD_GET_DEVICES 0x0
-
static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
{
if (ver->impl_id == TEE_IMPL_ID_OPTEE)
@@ -32,7 +20,8 @@ static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
}

static int get_devices(struct tee_context *ctx, u32 session,
- struct tee_shm *device_shm, u32 *shm_size)
+ struct tee_shm *device_shm, u32 *shm_size,
+ u32 func)
{
int ret = 0;
struct tee_ioctl_invoke_arg inv_arg;
@@ -41,8 +30,7 @@ static int get_devices(struct tee_context *ctx, u32 session,
memset(&inv_arg, 0, sizeof(inv_arg));
memset(&param, 0, sizeof(param));

- /* Invoke PTA_CMD_GET_DEVICES function */
- inv_arg.func = PTA_CMD_GET_DEVICES;
+ inv_arg.func = func;
inv_arg.session = session;
inv_arg.num_params = 4;

@@ -90,7 +78,7 @@ static int optee_register_device(const uuid_t *device_uuid)
return rc;
}

-int optee_enumerate_devices(void)
+static int __optee_enumerate_devices(u32 func)
{
const uuid_t pta_uuid =
UUID_INIT(0x7011a688, 0xddde, 0x4053,
@@ -121,7 +109,7 @@ int optee_enumerate_devices(void)
goto out_ctx;
}

- rc = get_devices(ctx, sess_arg.session, NULL, &shm_size);
+ rc = get_devices(ctx, sess_arg.session, NULL, &shm_size, func);
if (rc < 0 || !shm_size)
goto out_sess;

@@ -133,7 +121,7 @@ int optee_enumerate_devices(void)
goto out_sess;
}

- rc = get_devices(ctx, sess_arg.session, device_shm, &shm_size);
+ rc = get_devices(ctx, sess_arg.session, device_shm, &shm_size, func);
if (rc < 0)
goto out_shm;

@@ -161,3 +149,8 @@ int optee_enumerate_devices(void)

return rc;
}
+
+int optee_enumerate_devices(u32 func)
+{
+ return __optee_enumerate_devices(func);
+}
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
index d9c5037b4e03..8b71839a357e 100644
--- a/drivers/tee/optee/optee_private.h
+++ b/drivers/tee/optee/optee_private.h
@@ -78,6 +78,9 @@ struct optee_supp {
* @memremaped_shm virtual address of memory in shared memory pool
* @sec_caps: secure world capabilities defined by
* OPTEE_SMC_SEC_CAP_* in optee_smc.h
+ * @scan_bus_done flag if device registation was already done.
+ * @scan_bus_wq workqueue to scan optee bus and register optee drivers
+ * @scan_bus_work workq to scan optee bus and register optee drivers
*/
struct optee {
struct tee_device *supp_teedev;
@@ -89,6 +92,9 @@ struct optee {
struct tee_shm_pool *pool;
void *memremaped_shm;
u32 sec_caps;
+ bool scan_bus_done;
+ struct workqueue_struct *scan_bus_wq;
+ struct work_struct scan_bus_work;
};

struct optee_session {
@@ -173,7 +179,9 @@ void optee_free_pages_list(void *array, size_t num_entries);
void optee_fill_pages_list(u64 *dst, struct page **pages, int num_pages,
size_t page_offset);

-int optee_enumerate_devices(void);
+#define PTA_CMD_GET_DEVICES 0x0
+#define PTA_CMD_GET_DEVICES_SUPP 0x1
+int optee_enumerate_devices(u32 func);

/*
* Small helpers
--
2.17.1

2020-06-04 18:04:30

[permalink] [raw]

Subject: [PATCHv8 3/3] tpm_ftpm_tee: register driver on TEE bus

OP-TEE based fTPM Trusted Application depends on tee-supplicant to
provide NV RAM implementation based on RPMB secure storage. So this
dependency can be resolved via TEE bus where we only invoke fTPM
driver probe once fTPM device is registered on the bus which is only
true after the tee-supplicant is up and running. Additionally, TEE bus
provides auto device enumeration.

Signed-off-by: Maxim Uvarov <[email protected]>
Suggested-by: Sumit Garg <[email protected]>
Suggested-by: Arnd Bergmann <[email protected]>
Reviewed-by: Sumit Garg <[email protected]>
---
drivers/char/tpm/tpm_ftpm_tee.c | 70 ++++++++++++++++++++++++++++-----
1 file changed, 60 insertions(+), 10 deletions(-)

diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
index 22bf553ccf9d..28da638360d8 100644
--- a/drivers/char/tpm/tpm_ftpm_tee.c
+++ b/drivers/char/tpm/tpm_ftpm_tee.c
@@ -214,11 +214,10 @@ static int ftpm_tee_match(struct tee_ioctl_version_data *ver, const void *data)
* Return:
* On success, 0. On failure, -errno.
*/
-static int ftpm_tee_probe(struct platform_device *pdev)
+static int ftpm_tee_probe(struct device *dev)
{
int rc;
struct tpm_chip *chip;
- struct device *dev = &pdev->dev;
struct ftpm_tee_private *pvt_data = NULL;
struct tee_ioctl_open_session_arg sess_arg;

@@ -297,6 +296,13 @@ static int ftpm_tee_probe(struct platform_device *pdev)
return rc;
}

+static int ftpm_plat_tee_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+
+ return ftpm_tee_probe(dev);
+}
+
/**
* ftpm_tee_remove() - remove the TPM device
* @pdev: the platform_device description.
@@ -304,9 +310,9 @@ static int ftpm_tee_probe(struct platform_device *pdev)
* Return:
* 0 always.
*/
-static int ftpm_tee_remove(struct platform_device *pdev)
+static int ftpm_tee_remove(struct device *dev)
{
- struct ftpm_tee_private *pvt_data = dev_get_drvdata(&pdev->dev);
+ struct ftpm_tee_private *pvt_data = dev_get_drvdata(dev);

/* Release the chip */
tpm_chip_unregister(pvt_data->chip);
@@ -328,11 +334,18 @@ static int ftpm_tee_remove(struct platform_device *pdev)
return 0;
}

+static int ftpm_plat_tee_remove(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+
+ return ftpm_tee_remove(dev);
+}
+
/**
* ftpm_tee_shutdown() - shutdown the TPM device
* @pdev: the platform_device description.
*/
-static void ftpm_tee_shutdown(struct platform_device *pdev)
+static void ftpm_plat_tee_shutdown(struct platform_device *pdev)
{
struct ftpm_tee_private *pvt_data = dev_get_drvdata(&pdev->dev);

@@ -347,17 +360,54 @@ static const struct of_device_id of_ftpm_tee_ids[] = {
};
MODULE_DEVICE_TABLE(of, of_ftpm_tee_ids);

-static struct platform_driver ftpm_tee_driver = {
+static struct platform_driver ftpm_tee_plat_driver = {
.driver = {
.name = "ftpm-tee",
.of_match_table = of_match_ptr(of_ftpm_tee_ids),
},
- .probe = ftpm_tee_probe,
- .remove = ftpm_tee_remove,
- .shutdown = ftpm_tee_shutdown,
+ .shutdown = ftpm_plat_tee_shutdown,
+ .probe = ftpm_plat_tee_probe,
+ .remove = ftpm_plat_tee_remove,
+};
+
+/* UUID of the fTPM TA */
+static const struct tee_client_device_id optee_ftpm_id_table[] = {
+ {UUID_INIT(0xbc50d971, 0xd4c9, 0x42c4,
+ 0x82, 0xcb, 0x34, 0x3f, 0xb7, 0xf3, 0x78, 0x96)},
+ {}
};

-module_platform_driver(ftpm_tee_driver);
+MODULE_DEVICE_TABLE(tee, optee_ftpm_id_table);
+
+static struct tee_client_driver ftpm_tee_driver = {
+ .id_table = optee_ftpm_id_table,
+ .driver = {
+ .name = "optee-ftpm",
+ .bus = &tee_bus_type,
+ .probe = ftpm_tee_probe,
+ .remove = ftpm_tee_remove,
+ },
+};
+
+static int __init ftpm_mod_init(void)
+{
+ int rc;
+
+ rc = platform_driver_register(&ftpm_tee_plat_driver);
+ if (rc)
+ return rc;
+
+ return driver_register(&ftpm_tee_driver.driver);
+}
+
+static void __exit ftpm_mod_exit(void)
+{
+ platform_driver_unregister(&ftpm_tee_plat_driver);
+ driver_unregister(&ftpm_tee_driver.driver);
+}
+
+module_init(ftpm_mod_init);
+module_exit(ftpm_mod_exit);

MODULE_AUTHOR("Thirupathaiah Annapureddy <[email protected]>");
MODULE_DESCRIPTION("TPM Driver for fTPM TA in TEE");
--
2.17.1

2020-06-15 14:37:50

[permalink] [raw]

Subject: Re: [PATCHv8 0/3] optee: register drivers on optee bus

ping.
Patchset was reviewed and all comments are codeverd. Optee-os patches
were merged. These kernel patches look like they are hanging
somewhere...

Thanks,
Maxim.

On Thu, 4 Jun 2020 at 20:58, Maxim Uvarov <[email protected]> wrote:
>
> v8: - fix v7 check.
> v7: - check return value of dev_set_name() (Jarkko Sakkinen)
> v6: - description, comments, patches reorder and destroy workqueue (Sumit Garg)
> v5: - removed pr_err and fix typos in description (Jarkko Sakkinen)
> - added missed kfree in optee_open()
> v4: - sysfs entry is optee-ta-uuid (Jerome Forissier, Sumit Garg)
> - added Documentation/ABI/testing/sysfs-bus-optee-devices (Greg Kroah-Hartman)
> v3: - support tee-suppicant restart (Jens Wiklander)
> - description and comments (Jarkko Sakkinen)
> - do not name optee drivers by index in sysfs (Sumit Garg)
> v2: - write TEE with capital letters.
> - declare __optee_enumerate_device() as static.
>
> Maxim Uvarov (3):
> optee: use uuid for sysfs driver entry
> optee: enable support for multi-stage bus enumeration
> tpm_ftpm_tee: register driver on TEE bus
>
> .../ABI/testing/sysfs-bus-optee-devices | 8 +++
> MAINTAINERS | 1 +
> drivers/char/tpm/tpm_ftpm_tee.c | 70 ++++++++++++++++---
> drivers/tee/optee/core.c | 27 ++++++-
> drivers/tee/optee/device.c | 38 +++++-----
> drivers/tee/optee/optee_private.h | 10 ++-
> 6 files changed, 119 insertions(+), 35 deletions(-)
> create mode 100644 Documentation/ABI/testing/sysfs-bus-optee-devices
>
> --
> 2.17.1
>

2020-06-16 08:32:07

[permalink] [raw]

Subject: Re: [PATCHv8 0/3] optee: register drivers on optee bus

Hi Maxim and Jarkko,

On Mon, Jun 15, 2020 at 05:32:40PM +0300, Maxim Uvarov wrote:
> ping.
> Patchset was reviewed and all comments are codeverd. Optee-os patches
> were merged. These kernel patches look like they are hanging
> somewhere...

I'm almost OK with this patchset, except that
Documentation/ABI/testing/sysfs-bus-optee-devices needs to be updated
for the new kernel version and TEE mailing list which we're changing right
now.

The last patch touches files I'm not maintainer of. That patch depends
on the previous patches so it makes sense to keep them together. If a
TPM device driver maintainer would ack that patch I can take it via my
tree. Or we can do it the other way around (with a v9 patchset),
whichever is preferred.

Cheers,
Jens

>
> Thanks,
> Maxim.
>
> On Thu, 4 Jun 2020 at 20:58, Maxim Uvarov <[email protected]> wrote:
> >
> > v8: - fix v7 check.
> > v7: - check return value of dev_set_name() (Jarkko Sakkinen)
> > v6: - description, comments, patches reorder and destroy workqueue (Sumit Garg)
> > v5: - removed pr_err and fix typos in description (Jarkko Sakkinen)
> > - added missed kfree in optee_open()
> > v4: - sysfs entry is optee-ta-uuid (Jerome Forissier, Sumit Garg)
> > - added Documentation/ABI/testing/sysfs-bus-optee-devices (Greg Kroah-Hartman)
> > v3: - support tee-suppicant restart (Jens Wiklander)
> > - description and comments (Jarkko Sakkinen)
> > - do not name optee drivers by index in sysfs (Sumit Garg)
> > v2: - write TEE with capital letters.
> > - declare __optee_enumerate_device() as static.
> >
> > Maxim Uvarov (3):
> > optee: use uuid for sysfs driver entry
> > optee: enable support for multi-stage bus enumeration
> > tpm_ftpm_tee: register driver on TEE bus
> >
> > .../ABI/testing/sysfs-bus-optee-devices | 8 +++
> > MAINTAINERS | 1 +
> > drivers/char/tpm/tpm_ftpm_tee.c | 70 ++++++++++++++++---
> > drivers/tee/optee/core.c | 27 ++++++-
> > drivers/tee/optee/device.c | 38 +++++-----
> > drivers/tee/optee/optee_private.h | 10 ++-
> > 6 files changed, 119 insertions(+), 35 deletions(-)
> > create mode 100644 Documentation/ABI/testing/sysfs-bus-optee-devices
> >
> > --
> > 2.17.1
> >

2020-06-16 20:52:57

[permalink] [raw]

Subject: Re: [PATCHv8 0/3] optee: register drivers on optee bus

On Mon, Jun 15, 2020 at 05:32:40PM +0300, Maxim Uvarov wrote:
> ping.
> Patchset was reviewed and all comments are codeverd. Optee-os patches
> were merged. These kernel patches look like they are hanging
> somewhere...
>
> Thanks,
> Maxim.

I'm checking them atm.

/Jarkko

2020-06-16 20:55:17

[permalink] [raw]

Subject: Re: [PATCHv8 1/3] optee: use uuid for sysfs driver entry

On Thu, Jun 04, 2020 at 08:58:49PM +0300, Maxim Uvarov wrote:
> With the evolving use-cases for TEE bus, now it's required to support
> multi-stage enumeration process. But using a simple index doesn't
> suffice this requirement and instead leads to duplicate sysfs entries.
> So instead switch to use more informative device UUID for sysfs entry
> like:
> /sys/bus/tee/devices/optee-ta-<uuid>
>
> Signed-off-by: Maxim Uvarov <[email protected]>
> Reviewed-by: Sumit Garg <[email protected]>

Reviewed-by: Jarkko Sakkinen <[email protected]>

Sumit, are you able to test these easily?

/Jarkko

2020-06-16 20:57:13

[permalink] [raw]

Subject: Re: [PATCHv8 0/3] optee: register drivers on optee bus

On Tue, Jun 16, 2020 at 11:49:13PM +0300, Jarkko Sakkinen wrote:
> On Mon, Jun 15, 2020 at 05:32:40PM +0300, Maxim Uvarov wrote:
> > ping.
> > Patchset was reviewed and all comments are codeverd. Optee-os patches
> > were merged. These kernel patches look like they are hanging
> > somewhere...
> >
> > Thanks,
> > Maxim.
>
> I'm checking them atm.

I'm a bit confused about the optee patches:

? scripts/get_maintainer.pl drivers/tee/
Jens Wiklander <[email protected]> (maintainer:TEE SUBSYSTEM)
[email protected] (open list:TEE SUBSYSTEM)
[email protected] (open list)

Before I can pick the two optee patches they both need ack/reviewed-by
from Jens and also ack from him for me picking them, instead of him.

/Jarkko

2020-06-16 21:01:35

[permalink] [raw]

Subject: Re: [PATCHv8 2/3] optee: enable support for multi-stage bus enumeration

On Thu, Jun 04, 2020 at 08:58:50PM +0300, Maxim Uvarov wrote:
> Some drivers (like ftpm) can operate only after tee-supplicant
> runs because of tee-supplicant provides things like storage
> services (rpmb, shm). ?This patch splits probe of non tee-supplicant
> dependable drivers to the early stage, and after tee-supplicant run
> probe other drivers.
>
> Signed-off-by: Maxim Uvarov <[email protected]>
> Suggested-by: Sumit Garg <[email protected]>
> Suggested-by: Arnd Bergmann <[email protected]>
> Reviewed-by: Sumit Garg <[email protected]>

Acked-by: Jarkko Sakkinen <[email protected]>

Looks good to me but Jens should really go this through in detail. I'm
not the decision maker on this one.

/Jarkko

2020-06-16 21:02:51

[permalink] [raw]

Subject: Re: [PATCHv8 3/3] tpm_ftpm_tee: register driver on TEE bus

On Thu, Jun 04, 2020 at 08:58:51PM +0300, Maxim Uvarov wrote:
> OP-TEE based fTPM Trusted Application depends on tee-supplicant to
> provide NV RAM implementation based on RPMB secure storage. So this
> dependency can be resolved via TEE bus where we only invoke fTPM
> driver probe once fTPM device is registered on the bus which is only
> true after the tee-supplicant is up and running. Additionally, TEE bus
> provides auto device enumeration.
>
> Signed-off-by: Maxim Uvarov <[email protected]>
> Suggested-by: Sumit Garg <[email protected]>
> Suggested-by: Arnd Bergmann <[email protected]>
> Reviewed-by: Sumit Garg <[email protected]>

Reviewed-by: Jarkko Sakkinen <[email protected]>

/Jarkko

2020-06-17 06:11:41

[permalink] [raw]

Subject: Re: [PATCHv8 1/3] optee: use uuid for sysfs driver entry

On Wed, 17 Jun 2020 at 02:20, Jarkko Sakkinen
<[email protected]> wrote:
>
> On Thu, Jun 04, 2020 at 08:58:49PM +0300, Maxim Uvarov wrote:
> > With the evolving use-cases for TEE bus, now it's required to support
> > multi-stage enumeration process. But using a simple index doesn't
> > suffice this requirement and instead leads to duplicate sysfs entries.
> > So instead switch to use more informative device UUID for sysfs entry
> > like:
> > /sys/bus/tee/devices/optee-ta-<uuid>
> >
> > Signed-off-by: Maxim Uvarov <[email protected]>
> > Reviewed-by: Sumit Garg <[email protected]>
>
> Reviewed-by: Jarkko Sakkinen <[email protected]>
>
> Sumit, are you able to test these easily?

Yes, I could give them a try.

Maxim,

Could you share fTPM TA tree which I should use for testing?

-Sumit

>
> /Jarkko

2020-06-17 06:48:01

[permalink] [raw]

Subject: Re: [PATCHv8 1/3] optee: use uuid for sysfs driver entry

On Wed, 17 Jun 2020 at 09:07, Sumit Garg <[email protected]> wrote:
>
> On Wed, 17 Jun 2020 at 02:20, Jarkko Sakkinen
> <[email protected]> wrote:
> >
> > On Thu, Jun 04, 2020 at 08:58:49PM +0300, Maxim Uvarov wrote:
> > > With the evolving use-cases for TEE bus, now it's required to support
> > > multi-stage enumeration process. But using a simple index doesn't
> > > suffice this requirement and instead leads to duplicate sysfs entries.
> > > So instead switch to use more informative device UUID for sysfs entry
> > > like:
> > > /sys/bus/tee/devices/optee-ta-<uuid>
> > >
> > > Signed-off-by: Maxim Uvarov <[email protected]>
> > > Reviewed-by: Sumit Garg <[email protected]>
> >
> > Reviewed-by: Jarkko Sakkinen <[email protected]>
> >
> > Sumit, are you able to test these easily?
>
> Yes, I could give them a try.
>
> Maxim,
>
> Could you share fTPM TA tree which I should use for testing?
>
> -Sumit

Yes, I will send you an email.

>
> >
> > /Jarkko

2020-06-17 14:01:07

[permalink] [raw]

Subject: Re: [PATCHv8 1/3] optee: use uuid for sysfs driver entry

Hi Maxim,

On Thu, 4 Jun 2020 at 23:28, Maxim Uvarov <[email protected]> wrote:
>
> With the evolving use-cases for TEE bus, now it's required to support
> multi-stage enumeration process. But using a simple index doesn't
> suffice this requirement and instead leads to duplicate sysfs entries.
> So instead switch to use more informative device UUID for sysfs entry
> like:
> /sys/bus/tee/devices/optee-ta-<uuid>
>
> Signed-off-by: Maxim Uvarov <[email protected]>
> Reviewed-by: Sumit Garg <[email protected]>
> ---
> Documentation/ABI/testing/sysfs-bus-optee-devices | 8 ++++++++
> MAINTAINERS | 1 +
> drivers/tee/optee/device.c | 9 ++++++---
> 3 files changed, 15 insertions(+), 3 deletions(-)
> create mode 100644 Documentation/ABI/testing/sysfs-bus-optee-devices
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-optee-devices b/Documentation/ABI/testing/sysfs-bus-optee-devices
> new file mode 100644
> index 000000000000..0ae04ae5374a
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-optee-devices
> @@ -0,0 +1,8 @@
> +What: /sys/bus/tee/devices/optee-ta-<uuid>/
> +Date: May 2020
> +KernelVersion 5.7
> +Contact: [email protected]
> +Description:
> + OP-TEE bus provides reference to registered drivers under this directory. The <uuid>
> + matches Trusted Application (TA) driver and corresponding TA in secure OS. Drivers
> + are free to create needed API under optee-ta-<uuid> directory.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ecc0749810b0..6717afef2de3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12516,6 +12516,7 @@ OP-TEE DRIVER
> M: Jens Wiklander <[email protected]>
> L: [email protected]
> S: Maintained
> +F: Documentation/ABI/testing/sysfs-bus-optee-devices
> F: drivers/tee/optee/
>
> OP-TEE RANDOM NUMBER GENERATOR (RNG) DRIVER
> diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
> index e3a148521ec1..23d264c8146e 100644
> --- a/drivers/tee/optee/device.c
> +++ b/drivers/tee/optee/device.c
> @@ -65,7 +65,7 @@ static int get_devices(struct tee_context *ctx, u32 session,
> return 0;
> }
>
> -static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
> +static int optee_register_device(const uuid_t *device_uuid)
> {
> struct tee_client_device *optee_device = NULL;
> int rc;
> @@ -75,7 +75,10 @@ static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
> return -ENOMEM;
>
> optee_device->dev.bus = &tee_bus_type;
> - dev_set_name(&optee_device->dev, "optee-clnt%u", device_id);
> + if (dev_set_name(&optee_device->dev, "optee-ta-%pUl", device_uuid)) {

You should be using format specifier as: "%pUb" instead of "%pUl" as
UUID representation for TAs is in big endian format. See below:

# ls /sys/bus/tee/devices/
optee-ta-405b6ad9-e5c3-e321-8794-1002a5d5c61b
optee-ta-71d950bc-c9d4-c442-82cb-343fb7f37896
optee-ta-e70f4af0-5d1f-9b4b-abf7-619b85b4ce8c

While UUID for fTPM TA is in big endian format:
bc50d971-d4c9-42c4-82cb-343fb7f37896

Sorry that I missed it during review and noticed this while testing.

With the above fix included, I tested this series using fTPM early TA
on Qemu for aarch64 and used basic random number generation test using
tpm2-tools. So feel free to add:

Tested-by: Sumit Garg <[email protected]>

-Sumit

> + kfree(optee_device);
> + return -ENOMEM;
> + }
> uuid_copy(&optee_device->id.uuid, device_uuid);
>
> rc = device_register(&optee_device->dev);
> @@ -144,7 +147,7 @@ int optee_enumerate_devices(void)
> num_devices = shm_size / sizeof(uuid_t);
>
> for (idx = 0; idx < num_devices; idx++) {
> - rc = optee_register_device(&device_uuid[idx], idx);
> + rc = optee_register_device(&device_uuid[idx]);
> if (rc)
> goto out_shm;
> }
> --
> 2.17.1
>

2020-06-17 14:16:51

[permalink] [raw]

Subject: Re: [PATCHv8 1/3] optee: use uuid for sysfs driver entry

On Wed, 17 Jun 2020 at 16:58, Sumit Garg <[email protected]> wrote:
>
> Hi Maxim,
>
> On Thu, 4 Jun 2020 at 23:28, Maxim Uvarov <[email protected]> wrote:
> >
> > With the evolving use-cases for TEE bus, now it's required to support
> > multi-stage enumeration process. But using a simple index doesn't
> > suffice this requirement and instead leads to duplicate sysfs entries.
> > So instead switch to use more informative device UUID for sysfs entry
> > like:
> > /sys/bus/tee/devices/optee-ta-<uuid>
> >
> > Signed-off-by: Maxim Uvarov <[email protected]>
> > Reviewed-by: Sumit Garg <[email protected]>
> > ---
> > Documentation/ABI/testing/sysfs-bus-optee-devices | 8 ++++++++
> > MAINTAINERS | 1 +
> > drivers/tee/optee/device.c | 9 ++++++---
> > 3 files changed, 15 insertions(+), 3 deletions(-)
> > create mode 100644 Documentation/ABI/testing/sysfs-bus-optee-devices
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-optee-devices b/Documentation/ABI/testing/sysfs-bus-optee-devices
> > new file mode 100644
> > index 000000000000..0ae04ae5374a
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-optee-devices
> > @@ -0,0 +1,8 @@
> > +What: /sys/bus/tee/devices/optee-ta-<uuid>/
> > +Date: May 2020
> > +KernelVersion 5.7
> > +Contact: [email protected]
> > +Description:
> > + OP-TEE bus provides reference to registered drivers under this directory. The <uuid>
> > + matches Trusted Application (TA) driver and corresponding TA in secure OS. Drivers
> > + are free to create needed API under optee-ta-<uuid> directory.
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index ecc0749810b0..6717afef2de3 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12516,6 +12516,7 @@ OP-TEE DRIVER
> > M: Jens Wiklander <[email protected]>
> > L: [email protected]
> > S: Maintained
> > +F: Documentation/ABI/testing/sysfs-bus-optee-devices
> > F: drivers/tee/optee/
> >
> > OP-TEE RANDOM NUMBER GENERATOR (RNG) DRIVER
> > diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
> > index e3a148521ec1..23d264c8146e 100644
> > --- a/drivers/tee/optee/device.c
> > +++ b/drivers/tee/optee/device.c
> > @@ -65,7 +65,7 @@ static int get_devices(struct tee_context *ctx, u32 session,
> > return 0;
> > }
> >
> > -static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
> > +static int optee_register_device(const uuid_t *device_uuid)
> > {
> > struct tee_client_device *optee_device = NULL;
> > int rc;
> > @@ -75,7 +75,10 @@ static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
> > return -ENOMEM;
> >
> > optee_device->dev.bus = &tee_bus_type;
> > - dev_set_name(&optee_device->dev, "optee-clnt%u", device_id);
> > + if (dev_set_name(&optee_device->dev, "optee-ta-%pUl", device_uuid)) {
>
> You should be using format specifier as: "%pUb" instead of "%pUl" as
> UUID representation for TAs is in big endian format. See below:
>
> # ls /sys/bus/tee/devices/
> optee-ta-405b6ad9-e5c3-e321-8794-1002a5d5c61b
> optee-ta-71d950bc-c9d4-c442-82cb-343fb7f37896
> optee-ta-e70f4af0-5d1f-9b4b-abf7-619b85b4ce8c
>
> While UUID for fTPM TA is in big endian format:
> bc50d971-d4c9-42c4-82cb-343fb7f37896
>
> Sorry that I missed it during review and noticed this while testing.
>
> With the above fix included, I tested this series using fTPM early TA
> on Qemu for aarch64 and used basic random number generation test using
> tpm2-tools. So feel free to add:
>
> Tested-by: Sumit Garg <[email protected]>
>
> -Sumit
>
Oh, thanks. I will do v9 with this change and mailing list change.
Probalby you also need to check which prints are inside optee-os. I
think I copypasted this print from somewhere.

Maxim.

> > + kfree(optee_device);
> > + return -ENOMEM;
> > + }
> > uuid_copy(&optee_device->id.uuid, device_uuid);
> >
> > rc = device_register(&optee_device->dev);
> > @@ -144,7 +147,7 @@ int optee_enumerate_devices(void)
> > num_devices = shm_size / sizeof(uuid_t);
> >
> > for (idx = 0; idx < num_devices; idx++) {
> > - rc = optee_register_device(&device_uuid[idx], idx);
> > + rc = optee_register_device(&device_uuid[idx]);
> > if (rc)
> > goto out_shm;
> > }
> > --
> > 2.17.1
> >

2020-06-17 14:31:38

[permalink] [raw]

Subject: Re: [PATCHv8 0/3] optee: register drivers on optee bus

On Tue, 16 Jun 2020 at 11:29, Jens Wiklander <[email protected]> wrote:
>
> Hi Maxim and Jarkko,
>
> On Mon, Jun 15, 2020 at 05:32:40PM +0300, Maxim Uvarov wrote:
> > ping.
> > Patchset was reviewed and all comments are codeverd. Optee-os patches
> > were merged. These kernel patches look like they are hanging
> > somewhere...
>
> I'm almost OK with this patchset, except that
> Documentation/ABI/testing/sysfs-bus-optee-devices needs to be updated
> for the new kernel version and TEE mailing list which we're changing right
> now.
>

What is the new mailing list address? I did git pull the latest
master and get_maintainer.pl still points to
[email protected].

Maxim.

> The last patch touches files I'm not maintainer of. That patch depends
> on the previous patches so it makes sense to keep them together. If a
> TPM device driver maintainer would ack that patch I can take it via my
> tree. Or we can do it the other way around (with a v9 patchset),
> whichever is preferred.
>
> Cheers,
> Jens
>
> >
> > Thanks,
> > Maxim.
> >
> > On Thu, 4 Jun 2020 at 20:58, Maxim Uvarov <[email protected]> wrote:
> > >
> > > v8: - fix v7 check.
> > > v7: - check return value of dev_set_name() (Jarkko Sakkinen)
> > > v6: - description, comments, patches reorder and destroy workqueue (Sumit Garg)
> > > v5: - removed pr_err and fix typos in description (Jarkko Sakkinen)
> > > - added missed kfree in optee_open()
> > > v4: - sysfs entry is optee-ta-uuid (Jerome Forissier, Sumit Garg)
> > > - added Documentation/ABI/testing/sysfs-bus-optee-devices (Greg Kroah-Hartman)
> > > v3: - support tee-suppicant restart (Jens Wiklander)
> > > - description and comments (Jarkko Sakkinen)
> > > - do not name optee drivers by index in sysfs (Sumit Garg)
> > > v2: - write TEE with capital letters.
> > > - declare __optee_enumerate_device() as static.
> > >
> > > Maxim Uvarov (3):
> > > optee: use uuid for sysfs driver entry
> > > optee: enable support for multi-stage bus enumeration
> > > tpm_ftpm_tee: register driver on TEE bus
> > >
> > > .../ABI/testing/sysfs-bus-optee-devices | 8 +++
> > > MAINTAINERS | 1 +
> > > drivers/char/tpm/tpm_ftpm_tee.c | 70 ++++++++++++++++---
> > > drivers/tee/optee/core.c | 27 ++++++-
> > > drivers/tee/optee/device.c | 38 +++++-----
> > > drivers/tee/optee/optee_private.h | 10 ++-
> > > 6 files changed, 119 insertions(+), 35 deletions(-)
> > > create mode 100644 Documentation/ABI/testing/sysfs-bus-optee-devices
> > >
> > > --
> > > 2.17.1
> > >

2020-06-17 15:20:48

[permalink] [raw]

Subject: Re: [Tee-dev] [PATCHv8 1/3] optee: use uuid for sysfs driver entry

On 6/17/20 3:58 PM, Sumit Garg wrote:
> Hi Maxim,
>
> On Thu, 4 Jun 2020 at 23:28, Maxim Uvarov <[email protected]> wrote:
>>
>> With the evolving use-cases for TEE bus, now it's required to support
>> multi-stage enumeration process. But using a simple index doesn't
>> suffice this requirement and instead leads to duplicate sysfs entries.
>> So instead switch to use more informative device UUID for sysfs entry
>> like:
>> /sys/bus/tee/devices/optee-ta-<uuid>
>>
>> Signed-off-by: Maxim Uvarov <[email protected]>
>> Reviewed-by: Sumit Garg <[email protected]>
>> ---
>> Documentation/ABI/testing/sysfs-bus-optee-devices | 8 ++++++++
>> MAINTAINERS | 1 +
>> drivers/tee/optee/device.c | 9 ++++++---
>> 3 files changed, 15 insertions(+), 3 deletions(-)
>> create mode 100644 Documentation/ABI/testing/sysfs-bus-optee-devices
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-optee-devices b/Documentation/ABI/testing/sysfs-bus-optee-devices
>> new file mode 100644
>> index 000000000000..0ae04ae5374a
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-bus-optee-devices
>> @@ -0,0 +1,8 @@
>> +What: /sys/bus/tee/devices/optee-ta-<uuid>/
>> +Date: May 2020
>> +KernelVersion 5.7
>> +Contact: [email protected]
>> +Description:
>> + OP-TEE bus provides reference to registered drivers under this directory. The <uuid>
>> + matches Trusted Application (TA) driver and corresponding TA in secure OS. Drivers
>> + are free to create needed API under optee-ta-<uuid> directory.
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index ecc0749810b0..6717afef2de3 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -12516,6 +12516,7 @@ OP-TEE DRIVER
>> M: Jens Wiklander <[email protected]>
>> L: [email protected]
>> S: Maintained
>> +F: Documentation/ABI/testing/sysfs-bus-optee-devices
>> F: drivers/tee/optee/
>>
>> OP-TEE RANDOM NUMBER GENERATOR (RNG) DRIVER
>> diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
>> index e3a148521ec1..23d264c8146e 100644
>> --- a/drivers/tee/optee/device.c
>> +++ b/drivers/tee/optee/device.c
>> @@ -65,7 +65,7 @@ static int get_devices(struct tee_context *ctx, u32 session,
>> return 0;
>> }
>>
>> -static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
>> +static int optee_register_device(const uuid_t *device_uuid)
>> {
>> struct tee_client_device *optee_device = NULL;
>> int rc;
>> @@ -75,7 +75,10 @@ static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
>> return -ENOMEM;
>>
>> optee_device->dev.bus = &tee_bus_type;
>> - dev_set_name(&optee_device->dev, "optee-clnt%u", device_id);
>> + if (dev_set_name(&optee_device->dev, "optee-ta-%pUl", device_uuid)) {
>
> You should be using format specifier as: "%pUb" instead of "%pUl" as
> UUID representation for TAs is in big endian format. See below:

Where does device_uuid come from? If it comes directly from OP-TEE, then
it should be a pointer to the following struct:

typedef struct
{
uint32_t timeLow;
uint16_t timeMid;
uint16_t timeHiAndVersion;
uint8_t clockSeqAndNode[8];
} TEE_UUID;

(GlobalPlatform TEE Internal Core API spec v1.2.1 section 3.2.4)

- The spec does not mandate any particular endianness and simply warns
about possible issues if secure and non-secure worlds differ in endianness.
- OP-TEE uses %pUl assuming that host order is little endian (that is
true for the Arm platforms that run OP-TEE currently). By the same logic
%pUl should be fine in the kernel.
- On the other hand, the UUID in a Trusted App header is always encoded
big endian by the Python script that signs and optionally encrypts the
TA. This should not have any visible impact on UUIDs exchanged between
the secure and non-secure world though.

So I am wondering why you had to use %pUb. There must be some
inconsistency somewhere :-/

--
Jerome

>
> # ls /sys/bus/tee/devices/
> optee-ta-405b6ad9-e5c3-e321-8794-1002a5d5c61b
> optee-ta-71d950bc-c9d4-c442-82cb-343fb7f37896
> optee-ta-e70f4af0-5d1f-9b4b-abf7-619b85b4ce8c
>
> While UUID for fTPM TA is in big endian format:
> bc50d971-d4c9-42c4-82cb-343fb7f37896
>
> Sorry that I missed it during review and noticed this while testing.
>
> With the above fix included, I tested this series using fTPM early TA
> on Qemu for aarch64 and used basic random number generation test using
> tpm2-tools. So feel free to add:
>
> Tested-by: Sumit Garg <[email protected]>
>
> -Sumit
>
>> + kfree(optee_device);
>> + return -ENOMEM;
>> + }
>> uuid_copy(&optee_device->id.uuid, device_uuid);
>>
>> rc = device_register(&optee_device->dev);
>> @@ -144,7 +147,7 @@ int optee_enumerate_devices(void)
>> num_devices = shm_size / sizeof(uuid_t);
>>
>> for (idx = 0; idx < num_devices; idx++) {
>> - rc = optee_register_device(&device_uuid[idx], idx);
>> + rc = optee_register_device(&device_uuid[idx]);
>> if (rc)
>> goto out_shm;
>> }
>> --
>> 2.17.1
>>
> _______________________________________________
> Tee-dev mailing list
> [email protected]
> https://lists.linaro.org/mailman/listinfo/tee-dev
>

2020-06-17 19:55:09

[permalink] [raw]

Subject: Re: [Tee-dev] [PATCHv8 1/3] optee: use uuid for sysfs driver entry

On Wed, 17 Jun 2020 at 18:16, Jerome Forissier <[email protected]> wrote:
>
>
>
> On 6/17/20 3:58 PM, Sumit Garg wrote:
> > Hi Maxim,
> >
> > On Thu, 4 Jun 2020 at 23:28, Maxim Uvarov <[email protected]> wrote:
> >>
> >> With the evolving use-cases for TEE bus, now it's required to support
> >> multi-stage enumeration process. But using a simple index doesn't
> >> suffice this requirement and instead leads to duplicate sysfs entries.
> >> So instead switch to use more informative device UUID for sysfs entry
> >> like:
> >> /sys/bus/tee/devices/optee-ta-<uuid>
> >>
> >> Signed-off-by: Maxim Uvarov <[email protected]>
> >> Reviewed-by: Sumit Garg <[email protected]>
> >> ---
> >> Documentation/ABI/testing/sysfs-bus-optee-devices | 8 ++++++++
> >> MAINTAINERS | 1 +
> >> drivers/tee/optee/device.c | 9 ++++++---
> >> 3 files changed, 15 insertions(+), 3 deletions(-)
> >> create mode 100644 Documentation/ABI/testing/sysfs-bus-optee-devices
> >>
> >> diff --git a/Documentation/ABI/testing/sysfs-bus-optee-devices b/Documentation/ABI/testing/sysfs-bus-optee-devices
> >> new file mode 100644
> >> index 000000000000..0ae04ae5374a
> >> --- /dev/null
> >> +++ b/Documentation/ABI/testing/sysfs-bus-optee-devices
> >> @@ -0,0 +1,8 @@
> >> +What: /sys/bus/tee/devices/optee-ta-<uuid>/
> >> +Date: May 2020
> >> +KernelVersion 5.7
> >> +Contact: [email protected]
> >> +Description:
> >> + OP-TEE bus provides reference to registered drivers under this directory. The <uuid>
> >> + matches Trusted Application (TA) driver and corresponding TA in secure OS. Drivers
> >> + are free to create needed API under optee-ta-<uuid> directory.
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index ecc0749810b0..6717afef2de3 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -12516,6 +12516,7 @@ OP-TEE DRIVER
> >> M: Jens Wiklander <[email protected]>
> >> L: [email protected]
> >> S: Maintained
> >> +F: Documentation/ABI/testing/sysfs-bus-optee-devices
> >> F: drivers/tee/optee/
> >>
> >> OP-TEE RANDOM NUMBER GENERATOR (RNG) DRIVER
> >> diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
> >> index e3a148521ec1..23d264c8146e 100644
> >> --- a/drivers/tee/optee/device.c
> >> +++ b/drivers/tee/optee/device.c
> >> @@ -65,7 +65,7 @@ static int get_devices(struct tee_context *ctx, u32 session,
> >> return 0;
> >> }
> >>
> >> -static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
> >> +static int optee_register_device(const uuid_t *device_uuid)
> >> {
> >> struct tee_client_device *optee_device = NULL;
> >> int rc;
> >> @@ -75,7 +75,10 @@ static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
> >> return -ENOMEM;
> >>
> >> optee_device->dev.bus = &tee_bus_type;
> >> - dev_set_name(&optee_device->dev, "optee-clnt%u", device_id);
> >> + if (dev_set_name(&optee_device->dev, "optee-ta-%pUl", device_uuid)) {
> >
> > You should be using format specifier as: "%pUb" instead of "%pUl" as
> > UUID representation for TAs is in big endian format. See below:
>
> Where does device_uuid come from? If it comes directly from OP-TEE, then
> it should be a pointer to the following struct:
>
> typedef struct
> {
> uint32_t timeLow;
> uint16_t timeMid;
> uint16_t timeHiAndVersion;
> uint8_t clockSeqAndNode[8];
> } TEE_UUID;
>
> (GlobalPlatform TEE Internal Core API spec v1.2.1 section 3.2.4)
>
> - The spec does not mandate any particular endianness and simply warns
> about possible issues if secure and non-secure worlds differ in endianness.
> - OP-TEE uses %pUl assuming that host order is little endian (that is
> true for the Arm platforms that run OP-TEE currently). By the same logic
> %pUl should be fine in the kernel.
> - On the other hand, the UUID in a Trusted App header is always encoded
> big endian by the Python script that signs and optionally encrypts the
> TA. This should not have any visible impact on UUIDs exchanged between
> the secure and non-secure world though.
>
> So I am wondering why you had to use %pUb. There must be some
> inconsistency somewhere :-/
>
> --
> Jerome

From linux side it is for example:

static const struct tee_client_device_id optee_ftpm_id_table[] = {
{UUID_INIT(0xbc50d971, 0xd4c9, 0x42c4,
0x82, 0xcb, 0x34, 0x3f, 0xb7, 0xf3, 0x78, 0x96)},
{}
};

static struct tee_client_driver ftpm_tee_driver = {
.id_table = optee_ftpm_id_table,
.driver = {

So sysfs name has to be the same as the driver has. And UUD is simple
16 bytes:#define UUID_SIZE 16
typedef struct {
__u8 b[UUID_SIZE];
} uuid_t;

From TA it also:
#define TA_UUID { 0xBC50D971, 0xD4C9, 0x42C4, \
{0x82, 0xCB, 0x34, 0x3F, 0xB7, 0xF3, 0x78, 0x96}}

Compare uuid from optee and kernel driver version is simple:
static inline bool uuid_equal(const uuid_t *u1, const uuid_t *u2)
{
return memcmp(u1, u2, sizeof(uuid_t)) == 0;
}

So to support better code navigation. For example grep sources for
0xBC50D971, or find in sysfs "*bc50d971-*" I would say we need to use
BE format.
optee might also need to switch to BE prints for the same reason.

Maxim.

>
> >
> > # ls /sys/bus/tee/devices/
> > optee-ta-405b6ad9-e5c3-e321-8794-1002a5d5c61b
> > optee-ta-71d950bc-c9d4-c442-82cb-343fb7f37896
> > optee-ta-e70f4af0-5d1f-9b4b-abf7-619b85b4ce8c
> >
> > While UUID for fTPM TA is in big endian format:
> > bc50d971-d4c9-42c4-82cb-343fb7f37896
> >
> > Sorry that I missed it during review and noticed this while testing.
> >
> > With the above fix included, I tested this series using fTPM early TA
> > on Qemu for aarch64 and used basic random number generation test using
> > tpm2-tools. So feel free to add:
> >
> > Tested-by: Sumit Garg <[email protected]>
> >
> > -Sumit
> >
> >> + kfree(optee_device);
> >> + return -ENOMEM;
> >> + }
> >> uuid_copy(&optee_device->id.uuid, device_uuid);
> >>
> >> rc = device_register(&optee_device->dev);
> >> @@ -144,7 +147,7 @@ int optee_enumerate_devices(void)
> >> num_devices = shm_size / sizeof(uuid_t);
> >>
> >> for (idx = 0; idx < num_devices; idx++) {
> >> - rc = optee_register_device(&device_uuid[idx], idx);
> >> + rc = optee_register_device(&device_uuid[idx]);
> >> if (rc)
> >> goto out_shm;
> >> }
> >> --
> >> 2.17.1
> >>
> > _______________________________________________
> > Tee-dev mailing list
> > [email protected]
> > https://lists.linaro.org/mailman/listinfo/tee-dev
> >

2020-06-17 20:47:57

[permalink] [raw]

Subject: Re: [Tee-dev] [PATCHv8 1/3] optee: use uuid for sysfs driver entry

On 6/17/20 9:52 PM, Maxim Uvarov wrote:
> On Wed, 17 Jun 2020 at 18:16, Jerome Forissier <[email protected]> wrote:
>>
>> On 6/17/20 3:58 PM, Sumit Garg wrote:
>>> Hi Maxim,
>>>
>>> On Thu, 4 Jun 2020 at 23:28, Maxim Uvarov <[email protected]> wrote:
>>>>
>>>> With the evolving use-cases for TEE bus, now it's required to support
>>>> multi-stage enumeration process. But using a simple index doesn't
>>>> suffice this requirement and instead leads to duplicate sysfs entries.
>>>> So instead switch to use more informative device UUID for sysfs entry
>>>> like:
>>>> /sys/bus/tee/devices/optee-ta-<uuid>
>>>>
>>>> Signed-off-by: Maxim Uvarov <[email protected]>
>>>> Reviewed-by: Sumit Garg <[email protected]>
>>>> ---
>>>> Documentation/ABI/testing/sysfs-bus-optee-devices | 8 ++++++++
>>>> MAINTAINERS | 1 +
>>>> drivers/tee/optee/device.c | 9 ++++++---
>>>> 3 files changed, 15 insertions(+), 3 deletions(-)
>>>> create mode 100644 Documentation/ABI/testing/sysfs-bus-optee-devices
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-bus-optee-devices b/Documentation/ABI/testing/sysfs-bus-optee-devices
>>>> new file mode 100644
>>>> index 000000000000..0ae04ae5374a
>>>> --- /dev/null
>>>> +++ b/Documentation/ABI/testing/sysfs-bus-optee-devices
>>>> @@ -0,0 +1,8 @@
>>>> +What: /sys/bus/tee/devices/optee-ta-<uuid>/
>>>> +Date: May 2020
>>>> +KernelVersion 5.7
>>>> +Contact: [email protected]
>>>> +Description:
>>>> + OP-TEE bus provides reference to registered drivers under this directory. The <uuid>
>>>> + matches Trusted Application (TA) driver and corresponding TA in secure OS. Drivers
>>>> + are free to create needed API under optee-ta-<uuid> directory.
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index ecc0749810b0..6717afef2de3 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -12516,6 +12516,7 @@ OP-TEE DRIVER
>>>> M: Jens Wiklander <[email protected]>
>>>> L: [email protected]
>>>> S: Maintained
>>>> +F: Documentation/ABI/testing/sysfs-bus-optee-devices
>>>> F: drivers/tee/optee/
>>>>
>>>> OP-TEE RANDOM NUMBER GENERATOR (RNG) DRIVER
>>>> diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
>>>> index e3a148521ec1..23d264c8146e 100644
>>>> --- a/drivers/tee/optee/device.c
>>>> +++ b/drivers/tee/optee/device.c
>>>> @@ -65,7 +65,7 @@ static int get_devices(struct tee_context *ctx, u32 session,
>>>> return 0;
>>>> }
>>>>
>>>> -static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
>>>> +static int optee_register_device(const uuid_t *device_uuid)
>>>> {
>>>> struct tee_client_device *optee_device = NULL;
>>>> int rc;
>>>> @@ -75,7 +75,10 @@ static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
>>>> return -ENOMEM;
>>>>
>>>> optee_device->dev.bus = &tee_bus_type;
>>>> - dev_set_name(&optee_device->dev, "optee-clnt%u", device_id);
>>>> + if (dev_set_name(&optee_device->dev, "optee-ta-%pUl", device_uuid)) {
>>>
>>> You should be using format specifier as: "%pUb" instead of "%pUl" as
>>> UUID representation for TAs is in big endian format. See below:
>>
>> Where does device_uuid come from? If it comes directly from OP-TEE, then
>> it should be a pointer to the following struct:
>>
>> typedef struct
>> {
>> uint32_t timeLow;
>> uint16_t timeMid;
>> uint16_t timeHiAndVersion;
>> uint8_t clockSeqAndNode[8];
>> } TEE_UUID;
>>
>> (GlobalPlatform TEE Internal Core API spec v1.2.1 section 3.2.4)
>>
>> - The spec does not mandate any particular endianness and simply warns
>> about possible issues if secure and non-secure worlds differ in endianness.
>> - OP-TEE uses %pUl assuming that host order is little endian (that is
>> true for the Arm platforms that run OP-TEE currently). By the same logic
>> %pUl should be fine in the kernel.
>> - On the other hand, the UUID in a Trusted App header is always encoded
>> big endian by the Python script that signs and optionally encrypts the
>> TA. This should not have any visible impact on UUIDs exchanged between
>> the secure and non-secure world though.
>>
>> So I am wondering why you had to use %pUb. There must be some
>> inconsistency somewhere :-/
>>
>> --
>> Jerome
>
> From linux side it is for example:
>
> static const struct tee_client_device_id optee_ftpm_id_table[] = {
> {UUID_INIT(0xbc50d971, 0xd4c9, 0x42c4,
> 0x82, 0xcb, 0x34, 0x3f, 0xb7, 0xf3, 0x78, 0x96)},
> {}
> };
>
> static struct tee_client_driver ftpm_tee_driver = {
> .id_table = optee_ftpm_id_table,
> .driver = {
>
> So sysfs name has to be the same as the driver has. And UUD is simple
> 16 bytes:#define UUID_SIZE 16
> typedef struct {
> __u8 b[UUID_SIZE];
> } uuid_t;
>
> From TA it also:
> #define TA_UUID { 0xBC50D971, 0xD4C9, 0x42C4, \
> {0x82, 0xCB, 0x34, 0x3F, 0xB7, 0xF3, 0x78, 0x96}}
>
> Compare uuid from optee and kernel driver version is simple:
> static inline bool uuid_equal(const uuid_t *u1, const uuid_t *u2)
> {
> return memcmp(u1, u2, sizeof(uuid_t)) == 0;
> }
>
> So to support better code navigation. For example grep sources for
> 0xBC50D971, or find in sysfs "*bc50d971-*" I would say we need to use
> BE format.
> optee might also need to switch to BE prints for the same reason.

Sorry but this does not make much sense to me :-/

All I want to say is, if you ever need to use %pUb for things to work as
expected then it is *very* suspect and you should try to understand why,
because as I said and as far as I can tell OP-TEE stores all it's UUIDs
in memory in little endian format (more precisely, host endian with all
platforms being little endian currently), and %pUb is not even
implemented in OP-TEE.

--
Jerome

2020-06-17 23:40:12

[permalink] [raw]

Subject: Re: [PATCHv8 0/3] optee: register drivers on optee bus

On Tue, Jun 16, 2020 at 10:29:07AM +0200, Jens Wiklander wrote:
> Hi Maxim and Jarkko,
>
> On Mon, Jun 15, 2020 at 05:32:40PM +0300, Maxim Uvarov wrote:
> > ping.
> > Patchset was reviewed and all comments are codeverd. Optee-os patches
> > were merged. These kernel patches look like they are hanging
> > somewhere...
>
> I'm almost OK with this patchset, except that
> Documentation/ABI/testing/sysfs-bus-optee-devices needs to be updated
> for the new kernel version and TEE mailing list which we're changing right
> now.
>
> The last patch touches files I'm not maintainer of. That patch depends
> on the previous patches so it makes sense to keep them together. If a
> TPM device driver maintainer would ack that patch I can take it via my
> tree. Or we can do it the other way around (with a v9 patchset),
> whichever is preferred.
>
> Cheers,
> Jens

Probably easier if you pick all three and I ack the one touching TPM.

/Jarkko

2020-06-18 05:07:01

[permalink] [raw]

Subject: Re: [Tee-dev] [PATCHv8 1/3] optee: use uuid for sysfs driver entry

Hi Jerome,

On Wed, 17 Jun 2020 at 20:46, Jerome Forissier <[email protected]> wrote:
>
>
>
> On 6/17/20 3:58 PM, Sumit Garg wrote:
> > Hi Maxim,
> >
> > On Thu, 4 Jun 2020 at 23:28, Maxim Uvarov <[email protected]> wrote:
> >>
> >> With the evolving use-cases for TEE bus, now it's required to support
> >> multi-stage enumeration process. But using a simple index doesn't
> >> suffice this requirement and instead leads to duplicate sysfs entries.
> >> So instead switch to use more informative device UUID for sysfs entry
> >> like:
> >> /sys/bus/tee/devices/optee-ta-<uuid>
> >>
> >> Signed-off-by: Maxim Uvarov <[email protected]>
> >> Reviewed-by: Sumit Garg <[email protected]>
> >> ---
> >> Documentation/ABI/testing/sysfs-bus-optee-devices | 8 ++++++++
> >> MAINTAINERS | 1 +
> >> drivers/tee/optee/device.c | 9 ++++++---
> >> 3 files changed, 15 insertions(+), 3 deletions(-)
> >> create mode 100644 Documentation/ABI/testing/sysfs-bus-optee-devices
> >>
> >> diff --git a/Documentation/ABI/testing/sysfs-bus-optee-devices b/Documentation/ABI/testing/sysfs-bus-optee-devices
> >> new file mode 100644
> >> index 000000000000..0ae04ae5374a
> >> --- /dev/null
> >> +++ b/Documentation/ABI/testing/sysfs-bus-optee-devices
> >> @@ -0,0 +1,8 @@
> >> +What: /sys/bus/tee/devices/optee-ta-<uuid>/
> >> +Date: May 2020
> >> +KernelVersion 5.7
> >> +Contact: [email protected]
> >> +Description:
> >> + OP-TEE bus provides reference to registered drivers under this directory. The <uuid>
> >> + matches Trusted Application (TA) driver and corresponding TA in secure OS. Drivers
> >> + are free to create needed API under optee-ta-<uuid> directory.
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index ecc0749810b0..6717afef2de3 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -12516,6 +12516,7 @@ OP-TEE DRIVER
> >> M: Jens Wiklander <[email protected]>
> >> L: [email protected]
> >> S: Maintained
> >> +F: Documentation/ABI/testing/sysfs-bus-optee-devices
> >> F: drivers/tee/optee/
> >>
> >> OP-TEE RANDOM NUMBER GENERATOR (RNG) DRIVER
> >> diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
> >> index e3a148521ec1..23d264c8146e 100644
> >> --- a/drivers/tee/optee/device.c
> >> +++ b/drivers/tee/optee/device.c
> >> @@ -65,7 +65,7 @@ static int get_devices(struct tee_context *ctx, u32 session,
> >> return 0;
> >> }
> >>
> >> -static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
> >> +static int optee_register_device(const uuid_t *device_uuid)
> >> {
> >> struct tee_client_device *optee_device = NULL;
> >> int rc;
> >> @@ -75,7 +75,10 @@ static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
> >> return -ENOMEM;
> >>
> >> optee_device->dev.bus = &tee_bus_type;
> >> - dev_set_name(&optee_device->dev, "optee-clnt%u", device_id);
> >> + if (dev_set_name(&optee_device->dev, "optee-ta-%pUl", device_uuid)) {
> >
> > You should be using format specifier as: "%pUb" instead of "%pUl" as
> > UUID representation for TAs is in big endian format. See below:
>
> Where does device_uuid come from? If it comes directly from OP-TEE, then
> it should be a pointer to the following struct:
>
> typedef struct
> {
> uint32_t timeLow;
> uint16_t timeMid;
> uint16_t timeHiAndVersion;
> uint8_t clockSeqAndNode[8];
> } TEE_UUID;
>
> (GlobalPlatform TEE Internal Core API spec v1.2.1 section 3.2.4)
>
> - The spec does not mandate any particular endianness and simply warns
> about possible issues if secure and non-secure worlds differ in endianness.
> - OP-TEE uses %pUl assuming that host order is little endian (that is
> true for the Arm platforms that run OP-TEE currently). By the same logic
> %pUl should be fine in the kernel.
> - On the other hand, the UUID in a Trusted App header is always encoded
> big endian by the Python script that signs and optionally encrypts the
> TA. This should not have any visible impact on UUIDs exchanged between
> the secure and non-secure world though.
>
> So I am wondering why you had to use %pUb. There must be some
> inconsistency somewhere :-/

Yes there is. Linux stores UUID in big endian format (16 byte octets)
and OP-TEE stores UUID in little endian format (in form of struct you
referenced above).

And format conversion APIs [1] in OP-TEE OS are used while passing
UUID among Linux and OP-TEE.

So we need to use %pUb in case of Linux and %pUl in case of OP-TEE.

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

-Sumit

>
> --
> Jerome
>
> >
> > # ls /sys/bus/tee/devices/
> > optee-ta-405b6ad9-e5c3-e321-8794-1002a5d5c61b
> > optee-ta-71d950bc-c9d4-c442-82cb-343fb7f37896
> > optee-ta-e70f4af0-5d1f-9b4b-abf7-619b85b4ce8c
> >
> > While UUID for fTPM TA is in big endian format:
> > bc50d971-d4c9-42c4-82cb-343fb7f37896
> >
> > Sorry that I missed it during review and noticed this while testing.
> >
> > With the above fix included, I tested this series using fTPM early TA
> > on Qemu for aarch64 and used basic random number generation test using
> > tpm2-tools. So feel free to add:
> >
> > Tested-by: Sumit Garg <[email protected]>
> >
> > -Sumit
> >
> >> + kfree(optee_device);
> >> + return -ENOMEM;
> >> + }
> >> uuid_copy(&optee_device->id.uuid, device_uuid);
> >>
> >> rc = device_register(&optee_device->dev);
> >> @@ -144,7 +147,7 @@ int optee_enumerate_devices(void)
> >> num_devices = shm_size / sizeof(uuid_t);
> >>
> >> for (idx = 0; idx < num_devices; idx++) {
> >> - rc = optee_register_device(&device_uuid[idx], idx);
> >> + rc = optee_register_device(&device_uuid[idx]);
> >> if (rc)
> >> goto out_shm;
> >> }
> >> --
> >> 2.17.1
> >>
> > _______________________________________________
> > Tee-dev mailing list
> > [email protected]
> > https://lists.linaro.org/mailman/listinfo/tee-dev
> >

2020-06-18 05:15:37

[permalink] [raw]

Subject: Re: [Tee-dev] [PATCHv8 1/3] optee: use uuid for sysfs driver entry

On Thu, 18 Jun 2020 at 10:29, Sumit Garg <[email protected]> wrote:
>
> Hi Jerome,
>
> On Wed, 17 Jun 2020 at 20:46, Jerome Forissier <[email protected]> wrote:
> >
> >
> >
> > On 6/17/20 3:58 PM, Sumit Garg wrote:
> > > Hi Maxim,
> > >
> > > On Thu, 4 Jun 2020 at 23:28, Maxim Uvarov <[email protected]> wrote:
> > >>
> > >> With the evolving use-cases for TEE bus, now it's required to support
> > >> multi-stage enumeration process. But using a simple index doesn't
> > >> suffice this requirement and instead leads to duplicate sysfs entries.
> > >> So instead switch to use more informative device UUID for sysfs entry
> > >> like:
> > >> /sys/bus/tee/devices/optee-ta-<uuid>
> > >>
> > >> Signed-off-by: Maxim Uvarov <[email protected]>
> > >> Reviewed-by: Sumit Garg <[email protected]>
> > >> ---
> > >> Documentation/ABI/testing/sysfs-bus-optee-devices | 8 ++++++++
> > >> MAINTAINERS | 1 +
> > >> drivers/tee/optee/device.c | 9 ++++++---
> > >> 3 files changed, 15 insertions(+), 3 deletions(-)
> > >> create mode 100644 Documentation/ABI/testing/sysfs-bus-optee-devices
> > >>
> > >> diff --git a/Documentation/ABI/testing/sysfs-bus-optee-devices b/Documentation/ABI/testing/sysfs-bus-optee-devices
> > >> new file mode 100644
> > >> index 000000000000..0ae04ae5374a
> > >> --- /dev/null
> > >> +++ b/Documentation/ABI/testing/sysfs-bus-optee-devices
> > >> @@ -0,0 +1,8 @@
> > >> +What: /sys/bus/tee/devices/optee-ta-<uuid>/
> > >> +Date: May 2020
> > >> +KernelVersion 5.7
> > >> +Contact: [email protected]
> > >> +Description:
> > >> + OP-TEE bus provides reference to registered drivers under this directory. The <uuid>
> > >> + matches Trusted Application (TA) driver and corresponding TA in secure OS. Drivers
> > >> + are free to create needed API under optee-ta-<uuid> directory.
> > >> diff --git a/MAINTAINERS b/MAINTAINERS
> > >> index ecc0749810b0..6717afef2de3 100644
> > >> --- a/MAINTAINERS
> > >> +++ b/MAINTAINERS
> > >> @@ -12516,6 +12516,7 @@ OP-TEE DRIVER
> > >> M: Jens Wiklander <[email protected]>
> > >> L: [email protected]
> > >> S: Maintained
> > >> +F: Documentation/ABI/testing/sysfs-bus-optee-devices
> > >> F: drivers/tee/optee/
> > >>
> > >> OP-TEE RANDOM NUMBER GENERATOR (RNG) DRIVER
> > >> diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
> > >> index e3a148521ec1..23d264c8146e 100644
> > >> --- a/drivers/tee/optee/device.c
> > >> +++ b/drivers/tee/optee/device.c
> > >> @@ -65,7 +65,7 @@ static int get_devices(struct tee_context *ctx, u32 session,
> > >> return 0;
> > >> }
> > >>
> > >> -static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
> > >> +static int optee_register_device(const uuid_t *device_uuid)
> > >> {
> > >> struct tee_client_device *optee_device = NULL;
> > >> int rc;
> > >> @@ -75,7 +75,10 @@ static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
> > >> return -ENOMEM;
> > >>
> > >> optee_device->dev.bus = &tee_bus_type;
> > >> - dev_set_name(&optee_device->dev, "optee-clnt%u", device_id);
> > >> + if (dev_set_name(&optee_device->dev, "optee-ta-%pUl", device_uuid)) {
> > >
> > > You should be using format specifier as: "%pUb" instead of "%pUl" as
> > > UUID representation for TAs is in big endian format. See below:
> >
> > Where does device_uuid come from? If it comes directly from OP-TEE, then
> > it should be a pointer to the following struct:
> >
> > typedef struct
> > {
> > uint32_t timeLow;
> > uint16_t timeMid;
> > uint16_t timeHiAndVersion;
> > uint8_t clockSeqAndNode[8];
> > } TEE_UUID;
> >
> > (GlobalPlatform TEE Internal Core API spec v1.2.1 section 3.2.4)
> >
> > - The spec does not mandate any particular endianness and simply warns
> > about possible issues if secure and non-secure worlds differ in endianness.
> > - OP-TEE uses %pUl assuming that host order is little endian (that is
> > true for the Arm platforms that run OP-TEE currently). By the same logic
> > %pUl should be fine in the kernel.

I think Linux adheres to this RFC [1] for UUID byte order. See below
snippet from section: "Layout and Byte Order":

The fields are encoded as 16 octets, with the sizes and order of the
fields defined above, and with each field encoded with the Most
Significant Byte first (known as network byte order). Note that the
field names, particularly for multiplexed fields, follow historical
practice.

-Sumit

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

> > - On the other hand, the UUID in a Trusted App header is always encoded
> > big endian by the Python script that signs and optionally encrypts the
> > TA. This should not have any visible impact on UUIDs exchanged between
> > the secure and non-secure world though.
> >
> > So I am wondering why you had to use %pUb. There must be some
> > inconsistency somewhere :-/
>
> Yes there is. Linux stores UUID in big endian format (16 byte octets)
> and OP-TEE stores UUID in little endian format (in form of struct you
> referenced above).
>
> And format conversion APIs [1] in OP-TEE OS are used while passing
> UUID among Linux and OP-TEE.
>
> So we need to use %pUb in case of Linux and %pUl in case of OP-TEE.
>
> [1] https://github.com/OP-TEE/optee_os/blob/master/core/tee/uuid.c
>
> -Sumit
>
> >
> > --
> > Jerome
> >
> > >
> > > # ls /sys/bus/tee/devices/
> > > optee-ta-405b6ad9-e5c3-e321-8794-1002a5d5c61b
> > > optee-ta-71d950bc-c9d4-c442-82cb-343fb7f37896
> > > optee-ta-e70f4af0-5d1f-9b4b-abf7-619b85b4ce8c
> > >
> > > While UUID for fTPM TA is in big endian format:
> > > bc50d971-d4c9-42c4-82cb-343fb7f37896
> > >
> > > Sorry that I missed it during review and noticed this while testing.
> > >
> > > With the above fix included, I tested this series using fTPM early TA
> > > on Qemu for aarch64 and used basic random number generation test using
> > > tpm2-tools. So feel free to add:
> > >
> > > Tested-by: Sumit Garg <[email protected]>
> > >
> > > -Sumit
> > >
> > >> + kfree(optee_device);
> > >> + return -ENOMEM;
> > >> + }
> > >> uuid_copy(&optee_device->id.uuid, device_uuid);
> > >>
> > >> rc = device_register(&optee_device->dev);
> > >> @@ -144,7 +147,7 @@ int optee_enumerate_devices(void)
> > >> num_devices = shm_size / sizeof(uuid_t);
> > >>
> > >> for (idx = 0; idx < num_devices; idx++) {
> > >> - rc = optee_register_device(&device_uuid[idx], idx);
> > >> + rc = optee_register_device(&device_uuid[idx]);
> > >> if (rc)
> > >> goto out_shm;
> > >> }
> > >> --
> > >> 2.17.1
> > >>
> > > _______________________________________________
> > > Tee-dev mailing list
> > > [email protected]
> > > https://lists.linaro.org/mailman/listinfo/tee-dev
> > >

2020-06-18 07:03:15

[permalink] [raw]

Subject: Re: [Tee-dev] [PATCHv8 1/3] optee: use uuid for sysfs driver entry

On 6/18/20 6:59 AM, Sumit Garg wrote:
> Hi Jerome,
>
> On Wed, 17 Jun 2020 at 20:46, Jerome Forissier <[email protected]> wrote:
>>
>>
>>
>> On 6/17/20 3:58 PM, Sumit Garg wrote:
>>> Hi Maxim,
>>>
>>> On Thu, 4 Jun 2020 at 23:28, Maxim Uvarov <[email protected]> wrote:
>>>>
>>>> With the evolving use-cases for TEE bus, now it's required to support
>>>> multi-stage enumeration process. But using a simple index doesn't
>>>> suffice this requirement and instead leads to duplicate sysfs entries.
>>>> So instead switch to use more informative device UUID for sysfs entry
>>>> like:
>>>> /sys/bus/tee/devices/optee-ta-<uuid>
>>>>
>>>> Signed-off-by: Maxim Uvarov <[email protected]>
>>>> Reviewed-by: Sumit Garg <[email protected]>
>>>> ---
>>>> Documentation/ABI/testing/sysfs-bus-optee-devices | 8 ++++++++
>>>> MAINTAINERS | 1 +
>>>> drivers/tee/optee/device.c | 9 ++++++---
>>>> 3 files changed, 15 insertions(+), 3 deletions(-)
>>>> create mode 100644 Documentation/ABI/testing/sysfs-bus-optee-devices
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-bus-optee-devices b/Documentation/ABI/testing/sysfs-bus-optee-devices
>>>> new file mode 100644
>>>> index 000000000000..0ae04ae5374a
>>>> --- /dev/null
>>>> +++ b/Documentation/ABI/testing/sysfs-bus-optee-devices
>>>> @@ -0,0 +1,8 @@
>>>> +What: /sys/bus/tee/devices/optee-ta-<uuid>/
>>>> +Date: May 2020
>>>> +KernelVersion 5.7
>>>> +Contact: [email protected]
>>>> +Description:
>>>> + OP-TEE bus provides reference to registered drivers under this directory. The <uuid>
>>>> + matches Trusted Application (TA) driver and corresponding TA in secure OS. Drivers
>>>> + are free to create needed API under optee-ta-<uuid> directory.
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index ecc0749810b0..6717afef2de3 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -12516,6 +12516,7 @@ OP-TEE DRIVER
>>>> M: Jens Wiklander <[email protected]>
>>>> L: [email protected]
>>>> S: Maintained
>>>> +F: Documentation/ABI/testing/sysfs-bus-optee-devices
>>>> F: drivers/tee/optee/
>>>>
>>>> OP-TEE RANDOM NUMBER GENERATOR (RNG) DRIVER
>>>> diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
>>>> index e3a148521ec1..23d264c8146e 100644
>>>> --- a/drivers/tee/optee/device.c
>>>> +++ b/drivers/tee/optee/device.c
>>>> @@ -65,7 +65,7 @@ static int get_devices(struct tee_context *ctx, u32 session,
>>>> return 0;
>>>> }
>>>>
>>>> -static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
>>>> +static int optee_register_device(const uuid_t *device_uuid)
>>>> {
>>>> struct tee_client_device *optee_device = NULL;
>>>> int rc;
>>>> @@ -75,7 +75,10 @@ static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
>>>> return -ENOMEM;
>>>>
>>>> optee_device->dev.bus = &tee_bus_type;
>>>> - dev_set_name(&optee_device->dev, "optee-clnt%u", device_id);
>>>> + if (dev_set_name(&optee_device->dev, "optee-ta-%pUl", device_uuid)) {
>>>
>>> You should be using format specifier as: "%pUb" instead of "%pUl" as
>>> UUID representation for TAs is in big endian format. See below:
>>
>> Where does device_uuid come from? If it comes directly from OP-TEE, then
>> it should be a pointer to the following struct:
>>
>> typedef struct
>> {
>> uint32_t timeLow;
>> uint16_t timeMid;
>> uint16_t timeHiAndVersion;
>> uint8_t clockSeqAndNode[8];
>> } TEE_UUID;
>>
>> (GlobalPlatform TEE Internal Core API spec v1.2.1 section 3.2.4)
>>
>> - The spec does not mandate any particular endianness and simply warns
>> about possible issues if secure and non-secure worlds differ in endianness.
>> - OP-TEE uses %pUl assuming that host order is little endian (that is
>> true for the Arm platforms that run OP-TEE currently). By the same logic
>> %pUl should be fine in the kernel.
>> - On the other hand, the UUID in a Trusted App header is always encoded
>> big endian by the Python script that signs and optionally encrypts the
>> TA. This should not have any visible impact on UUIDs exchanged between
>> the secure and non-secure world though.
>>
>> So I am wondering why you had to use %pUb. There must be some
>> inconsistency somewhere :-/
>
> Yes there is. Linux stores UUID in big endian format (16 byte octets)
> and OP-TEE stores UUID in little endian format (in form of struct you
> referenced above).
>
> And format conversion APIs [1] in OP-TEE OS are used while passing
> UUID among Linux and OP-TEE.
>
> So we need to use %pUb in case of Linux and %pUl in case of OP-TEE.
>
> [1] https://github.com/OP-TEE/optee_os/blob/master/core/tee/uuid.c

Got it now. The TA enumeration function in OP-TEE performs the
conversion here:
https://github.com/OP-TEE/optee_os/blob/3.9.0/core/pta/device.c#L34

Thanks for clarifying.

--
Jerome

2020-06-18 07:47:15

[permalink] [raw]

Subject: Re: [Tee-dev] [PATCHv8 1/3] optee: use uuid for sysfs driver entry

There was a comment about a new mailing list address in Documentation.
Which one I should specify now?

On Thu, 18 Jun 2020 at 09:57, Jerome Forissier <[email protected]> wrote:
>
> On 6/18/20 6:59 AM, Sumit Garg wrote:
> > Hi Jerome,
> >
> > On Wed, 17 Jun 2020 at 20:46, Jerome Forissier <[email protected]> wrote:
> >>
> >>
> >>
> >> On 6/17/20 3:58 PM, Sumit Garg wrote:
> >>> Hi Maxim,
> >>>
> >>> On Thu, 4 Jun 2020 at 23:28, Maxim Uvarov <[email protected]> wrote:
> >>>>
> >>>> With the evolving use-cases for TEE bus, now it's required to support
> >>>> multi-stage enumeration process. But using a simple index doesn't
> >>>> suffice this requirement and instead leads to duplicate sysfs entries.
> >>>> So instead switch to use more informative device UUID for sysfs entry
> >>>> like:
> >>>> /sys/bus/tee/devices/optee-ta-<uuid>
> >>>>
> >>>> Signed-off-by: Maxim Uvarov <[email protected]>
> >>>> Reviewed-by: Sumit Garg <[email protected]>
> >>>> ---
> >>>> Documentation/ABI/testing/sysfs-bus-optee-devices | 8 ++++++++
> >>>> MAINTAINERS | 1 +
> >>>> drivers/tee/optee/device.c | 9 ++++++---
> >>>> 3 files changed, 15 insertions(+), 3 deletions(-)
> >>>> create mode 100644 Documentation/ABI/testing/sysfs-bus-optee-devices
> >>>>
> >>>> diff --git a/Documentation/ABI/testing/sysfs-bus-optee-devices b/Documentation/ABI/testing/sysfs-bus-optee-devices
> >>>> new file mode 100644
> >>>> index 000000000000..0ae04ae5374a
> >>>> --- /dev/null
> >>>> +++ b/Documentation/ABI/testing/sysfs-bus-optee-devices
> >>>> @@ -0,0 +1,8 @@
> >>>> +What: /sys/bus/tee/devices/optee-ta-<uuid>/
> >>>> +Date: May 2020
> >>>> +KernelVersion 5.7
> >>>> +Contact: [email protected]
> >>>> +Description:
> >>>> + OP-TEE bus provides reference to registered drivers under this directory. The <uuid>
> >>>> + matches Trusted Application (TA) driver and corresponding TA in secure OS. Drivers
> >>>> + are free to create needed API under optee-ta-<uuid> directory.
> >>>> diff --git a/MAINTAINERS b/MAINTAINERS
> >>>> index ecc0749810b0..6717afef2de3 100644
> >>>> --- a/MAINTAINERS
> >>>> +++ b/MAINTAINERS
> >>>> @@ -12516,6 +12516,7 @@ OP-TEE DRIVER
> >>>> M: Jens Wiklander <[email protected]>
> >>>> L: [email protected]
> >>>> S: Maintained
> >>>> +F: Documentation/ABI/testing/sysfs-bus-optee-devices
> >>>> F: drivers/tee/optee/
> >>>>
> >>>> OP-TEE RANDOM NUMBER GENERATOR (RNG) DRIVER
> >>>> diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
> >>>> index e3a148521ec1..23d264c8146e 100644
> >>>> --- a/drivers/tee/optee/device.c
> >>>> +++ b/drivers/tee/optee/device.c
> >>>> @@ -65,7 +65,7 @@ static int get_devices(struct tee_context *ctx, u32 session,
> >>>> return 0;
> >>>> }
> >>>>
> >>>> -static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
> >>>> +static int optee_register_device(const uuid_t *device_uuid)
> >>>> {
> >>>> struct tee_client_device *optee_device = NULL;
> >>>> int rc;
> >>>> @@ -75,7 +75,10 @@ static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
> >>>> return -ENOMEM;
> >>>>
> >>>> optee_device->dev.bus = &tee_bus_type;
> >>>> - dev_set_name(&optee_device->dev, "optee-clnt%u", device_id);
> >>>> + if (dev_set_name(&optee_device->dev, "optee-ta-%pUl", device_uuid)) {
> >>>
> >>> You should be using format specifier as: "%pUb" instead of "%pUl" as
> >>> UUID representation for TAs is in big endian format. See below:
> >>
> >> Where does device_uuid come from? If it comes directly from OP-TEE, then
> >> it should be a pointer to the following struct:
> >>
> >> typedef struct
> >> {
> >> uint32_t timeLow;
> >> uint16_t timeMid;
> >> uint16_t timeHiAndVersion;
> >> uint8_t clockSeqAndNode[8];
> >> } TEE_UUID;
> >>
> >> (GlobalPlatform TEE Internal Core API spec v1.2.1 section 3.2.4)
> >>
> >> - The spec does not mandate any particular endianness and simply warns
> >> about possible issues if secure and non-secure worlds differ in endianness.
> >> - OP-TEE uses %pUl assuming that host order is little endian (that is
> >> true for the Arm platforms that run OP-TEE currently). By the same logic
> >> %pUl should be fine in the kernel.
> >> - On the other hand, the UUID in a Trusted App header is always encoded
> >> big endian by the Python script that signs and optionally encrypts the
> >> TA. This should not have any visible impact on UUIDs exchanged between
> >> the secure and non-secure world though.
> >>
> >> So I am wondering why you had to use %pUb. There must be some
> >> inconsistency somewhere :-/
> >
> > Yes there is. Linux stores UUID in big endian format (16 byte octets)
> > and OP-TEE stores UUID in little endian format (in form of struct you
> > referenced above).
> >
> > And format conversion APIs [1] in OP-TEE OS are used while passing
> > UUID among Linux and OP-TEE.
> >
> > So we need to use %pUb in case of Linux and %pUl in case of OP-TEE.
> >
> > [1] https://github.com/OP-TEE/optee_os/blob/master/core/tee/uuid.c
>
>
> Got it now. The TA enumeration function in OP-TEE performs the
> conversion here:
> https://github.com/OP-TEE/optee_os/blob/3.9.0/core/pta/device.c#L34
>
> Thanks for clarifying.
>
> --
> Jerome

2020-06-18 07:58:45

[permalink] [raw]

Subject: Re: [PATCHv8 0/3] optee: register drivers on optee bus

On Thu, Jun 18, 2020 at 02:37:55AM +0300, Jarkko Sakkinen wrote:
> On Tue, Jun 16, 2020 at 10:29:07AM +0200, Jens Wiklander wrote:
> > Hi Maxim and Jarkko,
> >
> > On Mon, Jun 15, 2020 at 05:32:40PM +0300, Maxim Uvarov wrote:
> > > ping.
> > > Patchset was reviewed and all comments are codeverd. Optee-os patches
> > > were merged. These kernel patches look like they are hanging
> > > somewhere...
> >
> > I'm almost OK with this patchset, except that
> > Documentation/ABI/testing/sysfs-bus-optee-devices needs to be updated
> > for the new kernel version and TEE mailing list which we're changing right
> > now.
> >
> > The last patch touches files I'm not maintainer of. That patch depends
> > on the previous patches so it makes sense to keep them together. If a
> > TPM device driver maintainer would ack that patch I can take it via my
> > tree. Or we can do it the other way around (with a v9 patchset),
> > whichever is preferred.
> >
> > Cheers,
> > Jens
>
> Probably easier if you pick all three and I ack the one touching TPM.

Makes sense, let's do that.

Cheers,
Jens

2020-06-18 08:02:58

[permalink] [raw]

Subject: Re: [PATCHv8 0/3] optee: register drivers on optee bus

On Wed, Jun 17, 2020 at 05:26:42PM +0300, Maxim Uvarov wrote:
> On Tue, 16 Jun 2020 at 11:29, Jens Wiklander <[email protected]> wrote:
> >
> > Hi Maxim and Jarkko,
> >
> > On Mon, Jun 15, 2020 at 05:32:40PM +0300, Maxim Uvarov wrote:
> > > ping.
> > > Patchset was reviewed and all comments are codeverd. Optee-os patches
> > > were merged. These kernel patches look like they are hanging
> > > somewhere...
> >
> > I'm almost OK with this patchset, except that
> > Documentation/ABI/testing/sysfs-bus-optee-devices needs to be updated
> > for the new kernel version and TEE mailing list which we're changing right
> > now.
> >
>
> What is the new mailing list address? I did git pull the latest
> master and get_maintainer.pl still points to
> [email protected].

I'm sorry, I should have mentioned that. I just sent a pull request
to update it so it will have changed by the time these patches gets
merged. The new address is [email protected]

Cheers,
Jens

>
> Maxim.
>
> > The last patch touches files I'm not maintainer of. That patch depends
> > on the previous patches so it makes sense to keep them together. If a
> > TPM device driver maintainer would ack that patch I can take it via my
> > tree. Or we can do it the other way around (with a v9 patchset),
> > whichever is preferred.
> >
> > Cheers,
> > Jens
> >
> > >
> > > Thanks,
> > > Maxim.
> > >
> > > On Thu, 4 Jun 2020 at 20:58, Maxim Uvarov <[email protected]> wrote:
> > > >
> > > > v8: - fix v7 check.
> > > > v7: - check return value of dev_set_name() (Jarkko Sakkinen)
> > > > v6: - description, comments, patches reorder and destroy workqueue (Sumit Garg)
> > > > v5: - removed pr_err and fix typos in description (Jarkko Sakkinen)
> > > > - added missed kfree in optee_open()
> > > > v4: - sysfs entry is optee-ta-uuid (Jerome Forissier, Sumit Garg)
> > > > - added Documentation/ABI/testing/sysfs-bus-optee-devices (Greg Kroah-Hartman)
> > > > v3: - support tee-suppicant restart (Jens Wiklander)
> > > > - description and comments (Jarkko Sakkinen)
> > > > - do not name optee drivers by index in sysfs (Sumit Garg)
> > > > v2: - write TEE with capital letters.
> > > > - declare __optee_enumerate_device() as static.
> > > >
> > > > Maxim Uvarov (3):
> > > > optee: use uuid for sysfs driver entry
> > > > optee: enable support for multi-stage bus enumeration
> > > > tpm_ftpm_tee: register driver on TEE bus
> > > >
> > > > .../ABI/testing/sysfs-bus-optee-devices | 8 +++
> > > > MAINTAINERS | 1 +
> > > > drivers/char/tpm/tpm_ftpm_tee.c | 70 ++++++++++++++++---
> > > > drivers/tee/optee/core.c | 27 ++++++-
> > > > drivers/tee/optee/device.c | 38 +++++-----
> > > > drivers/tee/optee/optee_private.h | 10 ++-
> > > > 6 files changed, 119 insertions(+), 35 deletions(-)
> > > > create mode 100644 Documentation/ABI/testing/sysfs-bus-optee-devices
> > > >
> > > > --
> > > > 2.17.1
> > > >

2020-06-18 19:24:15

[permalink] [raw]

Subject: Re: [Tee-dev] [PATCHv8 1/3] optee: use uuid for sysfs driver entry

On Thu, 2020-06-18 at 10:42 +0530, Sumit Garg wrote:
> On Thu, 18 Jun 2020 at 10:29, Sumit Garg <[email protected]>
> wrote:
[...]
> > > typedef struct
> > > {
> > > uint32_t timeLow;
> > > uint16_t timeMid;
> > > uint16_t timeHiAndVersion;
> > > uint8_t clockSeqAndNode[8];
> > > } TEE_UUID;
> > >
> > > (GlobalPlatform TEE Internal Core API spec v1.2.1 section 3.2.4)
> > >
> > > - The spec does not mandate any particular endianness and simply
> > > warnsabout possible issues if secure and non-secure worlds differ
> > > in endianness.
> > > - OP-TEE uses %pUl assuming that host order is little endian
> > > (that is true for the Arm platforms that run OP-TEE currently).
> > > By the same logic %pUl should be fine in the kernel.
>
> I think Linux adheres to this RFC [1] for UUID byte order. See below
> snippet from section: "Layout and Byte Order":
>
> The fields are encoded as 16 octets, with the sizes and order of
> the
> fields defined above, and with each field encoded with the Most
> Significant Byte first (known as network byte order). Note that
> the
> field names, particularly for multiplexed fields, follow
> historical
> practice.

Actually, that's not quite true. We used to support both little and
big endian uuids until we realised it was basically microsoft vs
everyone else (as codified by RFC 4122). Now we support UUIDs which
are big endian and GUIDs which are little endian. This was the commit
that sorted out the confusion:

commit f9727a17db9bab71ddae91f74f11a8a2f9a0ece6
Author: Christoph Hellwig <[email protected]>
Date: Wed May 17 10:02:48 2017 +0200

uuid: rename uuid types

so if you're using a little endian uuid, you should probably be using
GUID for TEE_UUID.

James

2020-06-19 08:18:33

[permalink] [raw]

Subject: Re: [Tee-dev] [PATCHv8 1/3] optee: use uuid for sysfs driver entry

On Fri, 19 Jun 2020 at 00:49, James Bottomley
<[email protected]> wrote:
>
> On Thu, 2020-06-18 at 10:42 +0530, Sumit Garg wrote:
> > On Thu, 18 Jun 2020 at 10:29, Sumit Garg <[email protected]>
> > wrote:
> [...]
> > > > typedef struct
> > > > {
> > > > uint32_t timeLow;
> > > > uint16_t timeMid;
> > > > uint16_t timeHiAndVersion;
> > > > uint8_t clockSeqAndNode[8];
> > > > } TEE_UUID;
> > > >
> > > > (GlobalPlatform TEE Internal Core API spec v1.2.1 section 3.2.4)
> > > >
> > > > - The spec does not mandate any particular endianness and simply
> > > > warnsabout possible issues if secure and non-secure worlds differ
> > > > in endianness.
> > > > - OP-TEE uses %pUl assuming that host order is little endian
> > > > (that is true for the Arm platforms that run OP-TEE currently).
> > > > By the same logic %pUl should be fine in the kernel.
> >
> > I think Linux adheres to this RFC [1] for UUID byte order. See below
> > snippet from section: "Layout and Byte Order":
> >
> > The fields are encoded as 16 octets, with the sizes and order of
> > the
> > fields defined above, and with each field encoded with the Most
> > Significant Byte first (known as network byte order). Note that
> > the
> > field names, particularly for multiplexed fields, follow
> > historical
> > practice.
>
> Actually, that's not quite true. We used to support both little and
> big endian uuids until we realised it was basically microsoft vs
> everyone else (as codified by RFC 4122). Now we support UUIDs which
> are big endian and GUIDs which are little endian. This was the commit
> that sorted out the confusion:
>
> commit f9727a17db9bab71ddae91f74f11a8a2f9a0ece6
> Author: Christoph Hellwig <[email protected]>
> Date: Wed May 17 10:02:48 2017 +0200
>
> uuid: rename uuid types
>

Thanks for providing the background here.

> so if you're using a little endian uuid, you should probably be using
> GUID for TEE_UUID.

IMO, using GUID in kernel for TEE_UUID in OP-TEE OS will lead to
deviation from GlobalPlatform TEE client spec [1] as the spec only
references it as UUID and we would like to keep kernel TEE client
interface to be compatible with GP specs.

[1] https://globalplatform.org/specs-library/tee-client-api-specification/

-Sumit

>
> James
>

2020-06-19 16:24:32

[permalink] [raw]

Subject: Re: [Tee-dev] [PATCHv8 1/3] optee: use uuid for sysfs driver entry

On Fri, 2020-06-19 at 13:42 +0530, Sumit Garg wrote:
> On Fri, 19 Jun 2020 at 00:49, James Bottomley
> <[email protected]> wrote:
> >
> > On Thu, 2020-06-18 at 10:42 +0530, Sumit Garg wrote:
> > > On Thu, 18 Jun 2020 at 10:29, Sumit Garg <[email protected]>
> > > wrote:
> >
> > [...]
> > > > > typedef struct
> > > > > {
> > > > > uint32_t timeLow;
> > > > > uint16_t timeMid;
> > > > > uint16_t timeHiAndVersion;
> > > > > uint8_t clockSeqAndNode[8];
> > > > > } TEE_UUID;
> > > > >
> > > > > (GlobalPlatform TEE Internal Core API spec v1.2.1 section
> > > > > 3.2.4)
> > > > >
> > > > > - The spec does not mandate any particular endianness and
> > > > > simply
> > > > > warnsabout possible issues if secure and non-secure worlds
> > > > > differ
> > > > > in endianness.
> > > > > - OP-TEE uses %pUl assuming that host order is little endian
> > > > > (that is true for the Arm platforms that run OP-TEE
> > > > > currently).
> > > > > By the same logic %pUl should be fine in the kernel.
> > >
> > > I think Linux adheres to this RFC [1] for UUID byte order. See
> > > below
> > > snippet from section: "Layout and Byte Order":
> > >
> > > The fields are encoded as 16 octets, with the sizes and order
> > > of
> > > the
> > > fields defined above, and with each field encoded with the
> > > Most
> > > Significant Byte first (known as network byte order). Note
> > > that
> > > the
> > > field names, particularly for multiplexed fields, follow
> > > historical
> > > practice.
> >
> > Actually, that's not quite true. We used to support both little
> > and
> > big endian uuids until we realised it was basically microsoft vs
> > everyone else (as codified by RFC 4122). Now we support UUIDs
> > which
> > are big endian and GUIDs which are little endian. This was the
> > commit
> > that sorted out the confusion:
> >
> > commit f9727a17db9bab71ddae91f74f11a8a2f9a0ece6
> > Author: Christoph Hellwig <[email protected]>
> > Date: Wed May 17 10:02:48 2017 +0200
> >
> > uuid: rename uuid types
> >
>
> Thanks for providing the background here.
>
> > so if you're using a little endian uuid, you should probably be
> > using GUID for TEE_UUID.
>
> IMO, using GUID in kernel for TEE_UUID in OP-TEE OS will lead to
> deviation from GlobalPlatform TEE client spec [1] as the spec only
> references it as UUID and we would like to keep kernel TEE client
> interface to be compatible with GP specs.
>
> [1] https://globalplatform.org/specs-library/tee-client-api-specifica
> tion/

So having read the above, you know uuid_t is for big endian and guid_t
for little endian. However in your patch:

> -static int optee_register_device(const uuid_t *device_uuid, u32
> device_id)
> +static int optee_register_device(const uuid_t *device_uuid)
>

You're using uuid_t for little endian, you should be using guid_t.
It's not about consistency with the OP-TEE docs (although I'm pretty
sure they don't mandate what kernel type to use), it's about
consistency with what the kernel types mean. When some checker detects
your using little endian operations on a big endian structure (like in
the prink for instance) they're going to keep emailing you about it.

James

2020-06-23 00:54:54

[permalink] [raw]

Subject: Re: [PATCHv8 0/3] optee: register drivers on optee bus

On Thu, Jun 18, 2020 at 09:56:13AM +0200, Jens Wiklander wrote:
> On Thu, Jun 18, 2020 at 02:37:55AM +0300, Jarkko Sakkinen wrote:
> > On Tue, Jun 16, 2020 at 10:29:07AM +0200, Jens Wiklander wrote:
> > > Hi Maxim and Jarkko,
> > >
> > > On Mon, Jun 15, 2020 at 05:32:40PM +0300, Maxim Uvarov wrote:
> > > > ping.
> > > > Patchset was reviewed and all comments are codeverd. Optee-os patches
> > > > were merged. These kernel patches look like they are hanging
> > > > somewhere...
> > >
> > > I'm almost OK with this patchset, except that
> > > Documentation/ABI/testing/sysfs-bus-optee-devices needs to be updated
> > > for the new kernel version and TEE mailing list which we're changing right
> > > now.
> > >
> > > The last patch touches files I'm not maintainer of. That patch depends
> > > on the previous patches so it makes sense to keep them together. If a
> > > TPM device driver maintainer would ack that patch I can take it via my
> > > tree. Or we can do it the other way around (with a v9 patchset),
> > > whichever is preferred.
> > >
> > > Cheers,
> > > Jens
> >
> > Probably easier if you pick all three and I ack the one touching TPM.
>
> Makes sense, let's do that.

Great, thanks!

/Jarkko

2020-06-24 09:55:25

[permalink] [raw]

Subject: Re: [Tee-dev] [PATCHv8 1/3] optee: use uuid for sysfs driver entry

On Fri, Jun 19, 2020 at 08:00:44AM -0700, James Bottomley wrote:
> On Fri, 2020-06-19 at 13:42 +0530, Sumit Garg wrote:
> > On Fri, 19 Jun 2020 at 00:49, James Bottomley
> > <[email protected]> wrote:
> > >
> > > On Thu, 2020-06-18 at 10:42 +0530, Sumit Garg wrote:
> > > > On Thu, 18 Jun 2020 at 10:29, Sumit Garg <[email protected]>
> > > > wrote:
> > >
> > > [...]
> > > > > > typedef struct
> > > > > > {
> > > > > > uint32_t timeLow;
> > > > > > uint16_t timeMid;
> > > > > > uint16_t timeHiAndVersion;
> > > > > > uint8_t clockSeqAndNode[8];
> > > > > > } TEE_UUID;
> > > > > >
> > > > > > (GlobalPlatform TEE Internal Core API spec v1.2.1 section
> > > > > > 3.2.4)
> > > > > >
> > > > > > - The spec does not mandate any particular endianness and
> > > > > > simply
> > > > > > warnsabout possible issues if secure and non-secure worlds
> > > > > > differ
> > > > > > in endianness.
> > > > > > - OP-TEE uses %pUl assuming that host order is little endian
> > > > > > (that is true for the Arm platforms that run OP-TEE
> > > > > > currently).
> > > > > > By the same logic %pUl should be fine in the kernel.
> > > >
> > > > I think Linux adheres to this RFC [1] for UUID byte order. See
> > > > below
> > > > snippet from section: "Layout and Byte Order":
> > > >
> > > > The fields are encoded as 16 octets, with the sizes and order
> > > > of
> > > > the
> > > > fields defined above, and with each field encoded with the
> > > > Most
> > > > Significant Byte first (known as network byte order). Note
> > > > that
> > > > the
> > > > field names, particularly for multiplexed fields, follow
> > > > historical
> > > > practice.
> > >
> > > Actually, that's not quite true. We used to support both little
> > > and
> > > big endian uuids until we realised it was basically microsoft vs
> > > everyone else (as codified by RFC 4122). Now we support UUIDs
> > > which
> > > are big endian and GUIDs which are little endian. This was the
> > > commit
> > > that sorted out the confusion:
> > >
> > > commit f9727a17db9bab71ddae91f74f11a8a2f9a0ece6
> > > Author: Christoph Hellwig <[email protected]>
> > > Date: Wed May 17 10:02:48 2017 +0200
> > >
> > > uuid: rename uuid types
> > >
> >
> > Thanks for providing the background here.
> >
> > > so if you're using a little endian uuid, you should probably be
> > > using GUID for TEE_UUID.
> >
> > IMO, using GUID in kernel for TEE_UUID in OP-TEE OS will lead to
> > deviation from GlobalPlatform TEE client spec [1] as the spec only
> > references it as UUID and we would like to keep kernel TEE client
> > interface to be compatible with GP specs.
> >
> > [1] https://globalplatform.org/specs-library/tee-client-api-specifica
> > tion/
>
> So having read the above, you know uuid_t is for big endian and guid_t
> for little endian. However in your patch:
>
> > -static int optee_register_device(const uuid_t *device_uuid, u32
> > device_id)
> > +static int optee_register_device(const uuid_t *device_uuid)
> >
>
> You're using uuid_t for little endian, you should be using guid_t.
> It's not about consistency with the OP-TEE docs (although I'm pretty
> sure they don't mandate what kernel type to use), it's about
> consistency with what the kernel types mean. When some checker detects
> your using little endian operations on a big endian structure (like in
> the prink for instance) they're going to keep emailing you about it.

Thanks for the clarification. Sumit, Maxim, please take care of this.

Cheers,
Jens

>
> James
>

2020-06-24 10:50:59

[permalink] [raw]

Subject: Re: [Tee-dev] [PATCHv8 1/3] optee: use uuid for sysfs driver entry

Apologies for delay in my reply as I was busy with some other stuff.

On Fri, 19 Jun 2020 at 20:30, James Bottomley
<[email protected]> wrote:
>
> On Fri, 2020-06-19 at 13:42 +0530, Sumit Garg wrote:
> > On Fri, 19 Jun 2020 at 00:49, James Bottomley
> > <[email protected]> wrote:
> > >
> > > On Thu, 2020-06-18 at 10:42 +0530, Sumit Garg wrote:
> > > > On Thu, 18 Jun 2020 at 10:29, Sumit Garg <[email protected]>
> > > > wrote:
> > >
> > > [...]
> > > > > > typedef struct
> > > > > > {
> > > > > > uint32_t timeLow;
> > > > > > uint16_t timeMid;
> > > > > > uint16_t timeHiAndVersion;
> > > > > > uint8_t clockSeqAndNode[8];
> > > > > > } TEE_UUID;
> > > > > >
> > > > > > (GlobalPlatform TEE Internal Core API spec v1.2.1 section
> > > > > > 3.2.4)
> > > > > >
> > > > > > - The spec does not mandate any particular endianness and
> > > > > > simply
> > > > > > warnsabout possible issues if secure and non-secure worlds
> > > > > > differ
> > > > > > in endianness.
> > > > > > - OP-TEE uses %pUl assuming that host order is little endian
> > > > > > (that is true for the Arm platforms that run OP-TEE
> > > > > > currently).
> > > > > > By the same logic %pUl should be fine in the kernel.
> > > >
> > > > I think Linux adheres to this RFC [1] for UUID byte order. See
> > > > below
> > > > snippet from section: "Layout and Byte Order":
> > > >
> > > > The fields are encoded as 16 octets, with the sizes and order
> > > > of
> > > > the
> > > > fields defined above, and with each field encoded with the
> > > > Most
> > > > Significant Byte first (known as network byte order). Note
> > > > that
> > > > the
> > > > field names, particularly for multiplexed fields, follow
> > > > historical
> > > > practice.
> > >
> > > Actually, that's not quite true. We used to support both little
> > > and
> > > big endian uuids until we realised it was basically microsoft vs
> > > everyone else (as codified by RFC 4122). Now we support UUIDs
> > > which
> > > are big endian and GUIDs which are little endian. This was the
> > > commit
> > > that sorted out the confusion:
> > >
> > > commit f9727a17db9bab71ddae91f74f11a8a2f9a0ece6
> > > Author: Christoph Hellwig <[email protected]>
> > > Date: Wed May 17 10:02:48 2017 +0200
> > >
> > > uuid: rename uuid types
> > >
> >
> > Thanks for providing the background here.
> >
> > > so if you're using a little endian uuid, you should probably be
> > > using GUID for TEE_UUID.
> >
> > IMO, using GUID in kernel for TEE_UUID in OP-TEE OS will lead to
> > deviation from GlobalPlatform TEE client spec [1] as the spec only
> > references it as UUID and we would like to keep kernel TEE client
> > interface to be compatible with GP specs.
> >
> > [1] https://globalplatform.org/specs-library/tee-client-api-specifica
> > tion/
>
> So having read the above, you know uuid_t is for big endian and guid_t
> for little endian. However in your patch:
>
> > -static int optee_register_device(const uuid_t *device_uuid, u32
> > device_id)
> > +static int optee_register_device(const uuid_t *device_uuid)
> >
>
> You're using uuid_t for little endian, you should be using guid_t.

It's not just about implementation differences but about terminology
as well. AFAIK about GUID, it's been typically used in Microsoft
centric softwares as compared to UUID which is generically defined by
RFC 4122.

AFAIU about the differences [1] among UUID and GUID, it seems like
UUID is a subset of GUID. IOW, we can't say that every GUID can be
represented as UUID as per RFC 4122.

So by using different terminology in the kernel with respect to a TEE
implementation is meant to cause more confusion among users than
difference in implementation details (like endianness).

[1] https://stackoverflow.com/questions/246930/is-there-any-difference-between-a-guid-and-a-uuid

> It's not about consistency with the OP-TEE docs (although I'm pretty
> sure they don't mandate what kernel type to use),

The document which I shared wasn't OP-TEE specific but a generic TEE
specification defined by GlobalPlatform. And that spec doesn't put any
restrictions on UUID endianness. So it is very much possible that
another TEE implementation could implement UUID in big endian format
as the kernel does.

> it's about
> consistency with what the kernel types mean. When some checker detects
> your using little endian operations on a big endian structure (like in
> the prink for instance) they're going to keep emailing you about it.

As mentioned above, using different terminology is meant to cause more
confusion than just difference in endianness which is manageable
inside TEE.

And I think it's safe to say that the kernel implements UUID in big
endian format and thus uses %pUb whereas OP-TEE implements UUID in
little endian format and thus uses %pUl.

-Sumit

>
> James
>

2020-06-24 15:21:57

[permalink] [raw]

Subject: Re: [Tee-dev] [PATCHv8 1/3] optee: use uuid for sysfs driver entry

On Wed, 2020-06-24 at 16:17 +0530, Sumit Garg wrote:
> Apologies for delay in my reply as I was busy with some other stuff.
>
> On Fri, 19 Jun 2020 at 20:30, James Bottomley
> <[email protected]> wrote:
[...]
> > it's about consistency with what the kernel types mean. When some
> > checker detects your using little endian operations on a big endian
> > structure (like in the prink for instance) they're going to keep
> > emailing you about it.
>
> As mentioned above, using different terminology is meant to cause
> more confusion than just difference in endianness which is manageable
> inside TEE.
>
> And I think it's safe to say that the kernel implements UUID in big
> endian format and thus uses %pUb whereas OP-TEE implements UUID in
> little endian format and thus uses %pUl.

So what I think you're saying is that if we still had uuid_be and
uuid_le you'd use uuid_le, because that's exactly the structure
described in the docs. But because we renamed

uuid_be -> uuid_t
uuid_le -> guid_t

You can't use guid_t as a kernel type because it has the wrong name?

James

2020-06-24 15:46:13

[permalink] [raw]

Subject: Re: [Tee-dev] [PATCHv8 1/3] optee: use uuid for sysfs driver entry

On 6/24/20 5:21 PM, James Bottomley wrote:
> On Wed, 2020-06-24 at 16:17 +0530, Sumit Garg wrote:
>> Apologies for delay in my reply as I was busy with some other stuff.
>>
>> On Fri, 19 Jun 2020 at 20:30, James Bottomley
>> <[email protected]> wrote:
> [...]
>>> it's about consistency with what the kernel types mean. When some
>>> checker detects your using little endian operations on a big endian
>>> structure (like in the prink for instance) they're going to keep
>>> emailing you about it.
>>
>> As mentioned above, using different terminology is meant to cause
>> more confusion than just difference in endianness which is manageable
>> inside TEE.
>>
>> And I think it's safe to say that the kernel implements UUID in big
>> endian format and thus uses %pUb whereas OP-TEE implements UUID in
>> little endian format and thus uses %pUl.
>
> So what I think you're saying is that if we still had uuid_be and
> uuid_le you'd use uuid_le, because that's exactly the structure
> described in the docs. But because we renamed
>
> uuid_be -> uuid_t
> uuid_le -> guid_t
>
> You can't use guid_t as a kernel type because it has the wrong name?

Let me try to clear the confusion that I introduce myself I believe :-/
IMO:

- optee_register_device(const uuid_t *device_uuid) *is* the correct
prototype.
- device_uuid is *guaranteed* to be BE because OP-TEE makes this
guarantee (it converts from its internal LE representation to BE when
enumerating the devices, but it doesn't matter to the kernel).
- Therefore %pUb is the correct format.

I'm sorry for doubting the BE order initially. I am so used to OP-TEE
using LE internally, that I missed the fact that we have an explicit
conversion...

Does this sound good?

Thanks,
--
Jerome

2020-06-25 12:55:49

[permalink] [raw]

Subject: Re: [Tee-dev] [PATCHv8 1/3] optee: use uuid for sysfs driver entry

On Wed, 24 Jun 2020 at 18:44, Jerome Forissier <[email protected]> wrote:
>
>
>
> On 6/24/20 5:21 PM, James Bottomley wrote:
> > On Wed, 2020-06-24 at 16:17 +0530, Sumit Garg wrote:
> >> Apologies for delay in my reply as I was busy with some other stuff.
> >>
> >> On Fri, 19 Jun 2020 at 20:30, James Bottomley
> >> <[email protected]> wrote:
> > [...]
> >>> it's about consistency with what the kernel types mean. When some
> >>> checker detects your using little endian operations on a big endian
> >>> structure (like in the prink for instance) they're going to keep
> >>> emailing you about it.
> >>
> >> As mentioned above, using different terminology is meant to cause
> >> more confusion than just difference in endianness which is manageable
> >> inside TEE.
> >>
> >> And I think it's safe to say that the kernel implements UUID in big
> >> endian format and thus uses %pUb whereas OP-TEE implements UUID in
> >> little endian format and thus uses %pUl.
> >
> > So what I think you're saying is that if we still had uuid_be and
> > uuid_le you'd use uuid_le, because that's exactly the structure
> > described in the docs. But because we renamed
> >
> > uuid_be -> uuid_t
> > uuid_le -> guid_t
> >
> > You can't use guid_t as a kernel type because it has the wrong name?
>
> Let me try to clear the confusion that I introduce myself I believe :-/
> IMO:
>
> - optee_register_device(const uuid_t *device_uuid) *is* the correct
> prototype.
> - device_uuid is *guaranteed* to be BE because OP-TEE makes this
> guarantee (it converts from its internal LE representation to BE when
> enumerating the devices, but it doesn't matter to the kernel).
> - Therefore %pUb is the correct format.
>
> I'm sorry for doubting the BE order initially. I am so used to OP-TEE
> using LE internally, that I missed the fact that we have an explicit
> conversion...
>
> Does this sound good?
>
> Thanks,
> --
> Jerome

I think your description is correct. But I think this problem would
be solved outside of the current patchset.
All places should use one single format (LE):
- internal optee representation;
- device enumeration pta;
- this kernel driver which creates sysfs entry and sets
uid_copy(&optee_device->id.uuid, device_uuid);
- matching function;
- drivers use UUID_INIT();

In that way everything will be consistent. But it will require
changing other pieces, not just the kernel. While
these patches add functionality to support current device enumeration
in optee os.
So I think this version is ok to be applied.

Regards,
Maxim.

2020-06-25 14:25:01

[permalink] [raw]

Subject: Re: [Tee-dev] [PATCHv8 1/3] optee: use uuid for sysfs driver entry

On Wed, 24 Jun 2020 at 20:51, James Bottomley
<[email protected]> wrote:
>
> On Wed, 2020-06-24 at 16:17 +0530, Sumit Garg wrote:
> > Apologies for delay in my reply as I was busy with some other stuff.
> >
> > On Fri, 19 Jun 2020 at 20:30, James Bottomley
> > <[email protected]> wrote:
> [...]
> > > it's about consistency with what the kernel types mean. When some
> > > checker detects your using little endian operations on a big endian
> > > structure (like in the prink for instance) they're going to keep
> > > emailing you about it.
> >
> > As mentioned above, using different terminology is meant to cause
> > more confusion than just difference in endianness which is manageable
> > inside TEE.
> >
> > And I think it's safe to say that the kernel implements UUID in big
> > endian format and thus uses %pUb whereas OP-TEE implements UUID in
> > little endian format and thus uses %pUl.
>
> So what I think you're saying is that if we still had uuid_be and
> uuid_le you'd use uuid_le, because that's exactly the structure
> described in the docs. But because we renamed
>
> uuid_be -> uuid_t
> uuid_le -> guid_t
>
> You can't use guid_t as a kernel type because it has the wrong name?

Isn't the rename commit description [1] pretty clear about which is
the true UUID type from Linux point of view?

[1]
commit f9727a17db9bab71ddae91f74f11a8a2f9a0ece6
Author: Christoph Hellwig <[email protected]>
Date: Wed May 17 10:02:48 2017 +0200

uuid: rename uuid types

Our "little endian" UUID really is a Wintel GUID, so rename it and its
helpers such (guid_t). The big endian UUID is the **only true** one, so
give it the name uuid_t. The uuid_le and uuid_be names are retained for
now, but will hopefully go away soon. The exception to that are the _cmp
helpers that will be replaced by better primitives ASAP and thus don't
get the new names.

Also the _to_bin helpers are named to match the better named uuid_parse
routine in userspace.

Also remove the existing typedef in XFS that's now been superceeded by
the generic type name.

Signed-off-by: Christoph Hellwig <[email protected]>
[andy: also update the UUID_LE/UUID_BE macros including fallout]
Signed-off-by: Andy Shevchenko <[email protected]>
Reviewed-by: Amir Goldstein <[email protected]>
Reviewed-by: Darrick J. Wong <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>

Signed-off-by: Christoph Hellwig <[email protected]>

-Sumit

>
> James
>

2020-06-26 00:02:30

[permalink] [raw]

Subject: Re: [Tee-dev] [PATCHv8 1/3] optee: use uuid for sysfs driver entry

On Thu, 2020-06-25 at 19:54 +0530, Sumit Garg wrote:
> On Wed, 24 Jun 2020 at 20:51, James Bottomley
> <[email protected]> wrote:
> >
> > On Wed, 2020-06-24 at 16:17 +0530, Sumit Garg wrote:
> > > Apologies for delay in my reply as I was busy with some other
> > > stuff.
> > >
> > > On Fri, 19 Jun 2020 at 20:30, James Bottomley
> > > <[email protected]> wrote:
> >
> > [...]
> > > > it's about consistency with what the kernel types mean. When
> > > > some checker detects your using little endian operations on a
> > > > big endian structure (like in the prink for instance) they're
> > > > going to keep emailing you about it.
> > >
> > > As mentioned above, using different terminology is meant to cause
> > > more confusion than just difference in endianness which is
> > > manageable inside TEE.
> > >
> > > And I think it's safe to say that the kernel implements UUID in
> > > big endian format and thus uses %pUb whereas OP-TEE implements
> > > UUID in little endian format and thus uses %pUl.
> >
> > So what I think you're saying is that if we still had uuid_be and
> > uuid_le you'd use uuid_le, because that's exactly the structure
> > described in the docs. But because we renamed
> >
> > uuid_be -> uuid_t
> > uuid_le -> guid_t
> >
> > You can't use guid_t as a kernel type because it has the wrong
> > name?
>
> Isn't the rename commit description [1] pretty clear about which is
> the true UUID type from Linux point of view?

I don't think the kernel code takes a position on eternal verity, just
on logical or arithmetic truth. We just have to deal with both LE and
BE UUIDs so we have appropriate types for them and the LE type is now
named guid_t. They're both equally correct to use provided the use
case matches the designed one. So does the name really matter? If we
did

#define uuid_le_t guid_t

would you be happy? (not that the kernel cares about karmic emotional
states either ...)

James

2020-06-26 05:14:11

[permalink] [raw]

Subject: Re: [Tee-dev] [PATCHv8 1/3] optee: use uuid for sysfs driver entry

On Fri, 26 Jun 2020 at 05:01, James Bottomley
<[email protected]> wrote:
>
> On Thu, 2020-06-25 at 19:54 +0530, Sumit Garg wrote:
> > On Wed, 24 Jun 2020 at 20:51, James Bottomley
> > <[email protected]> wrote:
> > >
> > > On Wed, 2020-06-24 at 16:17 +0530, Sumit Garg wrote:
> > > > Apologies for delay in my reply as I was busy with some other
> > > > stuff.
> > > >
> > > > On Fri, 19 Jun 2020 at 20:30, James Bottomley
> > > > <[email protected]> wrote:
> > >
> > > [...]
> > > > > it's about consistency with what the kernel types mean. When
> > > > > some checker detects your using little endian operations on a
> > > > > big endian structure (like in the prink for instance) they're
> > > > > going to keep emailing you about it.
> > > >
> > > > As mentioned above, using different terminology is meant to cause
> > > > more confusion than just difference in endianness which is
> > > > manageable inside TEE.
> > > >
> > > > And I think it's safe to say that the kernel implements UUID in
> > > > big endian format and thus uses %pUb whereas OP-TEE implements
> > > > UUID in little endian format and thus uses %pUl.
> > >
> > > So what I think you're saying is that if we still had uuid_be and
> > > uuid_le you'd use uuid_le, because that's exactly the structure
> > > described in the docs. But because we renamed
> > >
> > > uuid_be -> uuid_t
> > > uuid_le -> guid_t
> > >
> > > You can't use guid_t as a kernel type because it has the wrong
> > > name?
> >
> > Isn't the rename commit description [1] pretty clear about which is
> > the true UUID type from Linux point of view?
>
> I don't think the kernel code takes a position on eternal verity, just
> on logical or arithmetic truth. We just have to deal with both LE and
> BE UUIDs so we have appropriate types for them and the LE type is now
> named guid_t. They're both equally correct to use provided the use
> case matches the designed one. So does the name really matter?

Yes it does. I guess I have provided enough reasoning for that. Also,
the rename commit itself illustrates its importance and clarifies the
use case for which they are meant to be used.

> If we
> did
>
> #define uuid_le_t guid_t
>
> would you be happy? (not that the kernel cares about karmic emotional
> states either ...)

It's not about me being happy but more about confusion and
inconsistency it will bring.

IMO, either kernel should be opinionated about UUID endianness like
currently it is:

uuid_t and its corresponding helpers (eg. UUID_INIT) follows BE format.

or support both endianness for UUID (no common type: uuid_t) like we
had earlier prior to rename commit:

uuid_be_t and its corresponding helpers (eg. UUID_BE_INIT) follow BE format.
uuid_le_t and its corresponding helpers (eg. UUID_LE_INIT) follow LE format.

But even if we consider later case as well, I am still not sure if we
can switch to uuid_le_t as it's been part of TEE core ABI
(open_session) where UUID is passed in BE format (see LE to BE
conversion in TEE client [1] and vice-versa in OP-TEE OS [2]) and
won't be a backwards compatible change.

[1] https://github.com/OP-TEE/optee_client/blob/master/libteec/src/tee_client_api.c#L595
[2] https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/kernel/ree_fs_ta.c#L92

-Sumit

>
> James
>

2020-06-26 05:18:06

[permalink] [raw]

Subject: Re: [Tee-dev] [PATCHv8 1/3] optee: use uuid for sysfs driver entry

On Thu, 25 Jun 2020 at 18:22, Maxim Uvarov <[email protected]> wrote:
>
> On Wed, 24 Jun 2020 at 18:44, Jerome Forissier <[email protected]> wrote:
> >
> >
> >
> > On 6/24/20 5:21 PM, James Bottomley wrote:
> > > On Wed, 2020-06-24 at 16:17 +0530, Sumit Garg wrote:
> > >> Apologies for delay in my reply as I was busy with some other stuff.
> > >>
> > >> On Fri, 19 Jun 2020 at 20:30, James Bottomley
> > >> <[email protected]> wrote:
> > > [...]
> > >>> it's about consistency with what the kernel types mean. When some
> > >>> checker detects your using little endian operations on a big endian
> > >>> structure (like in the prink for instance) they're going to keep
> > >>> emailing you about it.
> > >>
> > >> As mentioned above, using different terminology is meant to cause
> > >> more confusion than just difference in endianness which is manageable
> > >> inside TEE.
> > >>
> > >> And I think it's safe to say that the kernel implements UUID in big
> > >> endian format and thus uses %pUb whereas OP-TEE implements UUID in
> > >> little endian format and thus uses %pUl.
> > >
> > > So what I think you're saying is that if we still had uuid_be and
> > > uuid_le you'd use uuid_le, because that's exactly the structure
> > > described in the docs. But because we renamed
> > >
> > > uuid_be -> uuid_t
> > > uuid_le -> guid_t
> > >
> > > You can't use guid_t as a kernel type because it has the wrong name?
> >
> > Let me try to clear the confusion that I introduce myself I believe :-/
> > IMO:
> >
> > - optee_register_device(const uuid_t *device_uuid) *is* the correct
> > prototype.
> > - device_uuid is *guaranteed* to be BE because OP-TEE makes this
> > guarantee (it converts from its internal LE representation to BE when
> > enumerating the devices, but it doesn't matter to the kernel).
> > - Therefore %pUb is the correct format.
> >
> > I'm sorry for doubting the BE order initially. I am so used to OP-TEE
> > using LE internally, that I missed the fact that we have an explicit
> > conversion...
> >
> > Does this sound good?
> >
> > Thanks,
> > --
> > Jerome
>
> I think your description is correct. But I think this problem would
> be solved outside of the current patchset.
> All places should use one single format (LE):

I guess you missed the point that uuid_t is implemented in BE format
in the kernel which is compliant as per RFC 4122.

> - internal optee representation;
> - device enumeration pta;
> - this kernel driver which creates sysfs entry and sets
> uid_copy(&optee_device->id.uuid, device_uuid);
> - matching function;
> - drivers use UUID_INIT();

See carefully the implementation of UUID_INIT() which is in BE format.

-Sumit

>
> In that way everything will be consistent. But it will require
> changing other pieces, not just the kernel. While
> these patches add functionality to support current device enumeration
> in optee os.
> So I think this version is ok to be applied.
>
> Regards,
> Maxim.

2020-06-26 05:28:06

[permalink] [raw]

Subject: Re: [Tee-dev] [PATCHv8 1/3] optee: use uuid for sysfs driver entry

On Thu, 25 Jun 2020 at 18:22, Maxim Uvarov <[email protected]> wrote:
>
> On Wed, 24 Jun 2020 at 18:44, Jerome Forissier <[email protected]> wrote:
> >
> >
> >
> > On 6/24/20 5:21 PM, James Bottomley wrote:
> > > On Wed, 2020-06-24 at 16:17 +0530, Sumit Garg wrote:
> > >> Apologies for delay in my reply as I was busy with some other stuff.
> > >>
> > >> On Fri, 19 Jun 2020 at 20:30, James Bottomley
> > >> <[email protected]> wrote:
> > > [...]
> > >>> it's about consistency with what the kernel types mean. When some
> > >>> checker detects your using little endian operations on a big endian
> > >>> structure (like in the prink for instance) they're going to keep
> > >>> emailing you about it.
> > >>
> > >> As mentioned above, using different terminology is meant to cause
> > >> more confusion than just difference in endianness which is manageable
> > >> inside TEE.
> > >>
> > >> And I think it's safe to say that the kernel implements UUID in big
> > >> endian format and thus uses %pUb whereas OP-TEE implements UUID in
> > >> little endian format and thus uses %pUl.
> > >
> > > So what I think you're saying is that if we still had uuid_be and
> > > uuid_le you'd use uuid_le, because that's exactly the structure
> > > described in the docs. But because we renamed
> > >
> > > uuid_be -> uuid_t
> > > uuid_le -> guid_t
> > >
> > > You can't use guid_t as a kernel type because it has the wrong name?
> >
> > Let me try to clear the confusion that I introduce myself I believe :-/
> > IMO:
> >
> > - optee_register_device(const uuid_t *device_uuid) *is* the correct
> > prototype.
> > - device_uuid is *guaranteed* to be BE because OP-TEE makes this
> > guarantee (it converts from its internal LE representation to BE when
> > enumerating the devices, but it doesn't matter to the kernel).
> > - Therefore %pUb is the correct format.
> >
> > I'm sorry for doubting the BE order initially. I am so used to OP-TEE
> > using LE internally, that I missed the fact that we have an explicit
> > conversion...
> >
> > Does this sound good?
> >
> > Thanks,
> > --
> > Jerome
>
> I think your description is correct. But I think this problem would
> be solved outside of the current patchset.
> All places should use one single format (LE):
> - internal optee representation;
> - device enumeration pta;
> - this kernel driver which creates sysfs entry and sets
> uid_copy(&optee_device->id.uuid, device_uuid);
> - matching function;
> - drivers use UUID_INIT();
>
> In that way everything will be consistent. But it will require
> changing other pieces, not just the kernel. While
> these patches add functionality to support current device enumeration
> in optee os.
> So I think this version is ok to be applied.

I guess here you meant v9 patch-set. If yes then it's fine with me as well.

-Sumit

>
> Regards,
> Maxim.

2020-06-26 07:27:49

[permalink] [raw]

Subject: Re: [Tee-dev] [PATCHv8 1/3] optee: use uuid for sysfs driver entry

On Fri, 26 Jun 2020 at 10:40, Sumit Garg <[email protected]> wrote:
>
> On Fri, 26 Jun 2020 at 05:01, James Bottomley
> <[email protected]> wrote:
> >
> > On Thu, 2020-06-25 at 19:54 +0530, Sumit Garg wrote:
> > > On Wed, 24 Jun 2020 at 20:51, James Bottomley
> > > <[email protected]> wrote:
> > > >
> > > > On Wed, 2020-06-24 at 16:17 +0530, Sumit Garg wrote:
> > > > > Apologies for delay in my reply as I was busy with some other
> > > > > stuff.
> > > > >
> > > > > On Fri, 19 Jun 2020 at 20:30, James Bottomley
> > > > > <[email protected]> wrote:
> > > >
> > > > [...]
> > > > > > it's about consistency with what the kernel types mean. When
> > > > > > some checker detects your using little endian operations on a
> > > > > > big endian structure (like in the prink for instance) they're
> > > > > > going to keep emailing you about it.
> > > > >
> > > > > As mentioned above, using different terminology is meant to cause
> > > > > more confusion than just difference in endianness which is
> > > > > manageable inside TEE.
> > > > >
> > > > > And I think it's safe to say that the kernel implements UUID in
> > > > > big endian format and thus uses %pUb whereas OP-TEE implements
> > > > > UUID in little endian format and thus uses %pUl.
> > > >
> > > > So what I think you're saying is that if we still had uuid_be and
> > > > uuid_le you'd use uuid_le, because that's exactly the structure
> > > > described in the docs. But because we renamed
> > > >
> > > > uuid_be -> uuid_t
> > > > uuid_le -> guid_t
> > > >
> > > > You can't use guid_t as a kernel type because it has the wrong
> > > > name?
> > >
> > > Isn't the rename commit description [1] pretty clear about which is
> > > the true UUID type from Linux point of view?
> >
> > I don't think the kernel code takes a position on eternal verity, just
> > on logical or arithmetic truth. We just have to deal with both LE and
> > BE UUIDs so we have appropriate types for them and the LE type is now
> > named guid_t. They're both equally correct to use provided the use
> > case matches the designed one. So does the name really matter?
>
> Yes it does. I guess I have provided enough reasoning for that. Also,
> the rename commit itself illustrates its importance and clarifies the
> use case for which they are meant to be used.
>
> > If we
> > did
> >
> > #define uuid_le_t guid_t
> >
> > would you be happy? (not that the kernel cares about karmic emotional
> > states either ...)
>
> It's not about me being happy but more about confusion and
> inconsistency it will bring.
>
> IMO, either kernel should be opinionated about UUID endianness like
> currently it is:
>
> uuid_t and its corresponding helpers (eg. UUID_INIT) follows BE format.
>
> or support both endianness for UUID (no common type: uuid_t) like we
> had earlier prior to rename commit:
>
> uuid_be_t and its corresponding helpers (eg. UUID_BE_INIT) follow BE format.
> uuid_le_t and its corresponding helpers (eg. UUID_LE_INIT) follow LE format.
>
> But even if we consider later case as well, I am still not sure if we
> can switch to uuid_le_t as it's been part of TEE core ABI
> (open_session) where UUID is passed in BE format (see LE to BE
> conversion in TEE client [1] and vice-versa in OP-TEE OS [2]) and
> won't be a backwards compatible change.
>
> [1] https://github.com/OP-TEE/optee_client/blob/master/libteec/src/tee_client_api.c#L595
> [2] https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/kernel/ree_fs_ta.c#L92

Sorry, the second reference is incorrect, correct one is:

[2] https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/tee/entry_std.c#L279

-Sumit

>
> -Sumit
>
> >
> > James
> >

2020-06-26 08:48:33

by Andy Shevchenko

[permalink] [raw]

Subject: Re: [Tee-dev] [PATCHv8 1/3] optee: use uuid for sysfs driver entry

On Fri, Jun 26, 2020 at 8:18 AM Sumit Garg <[email protected]> wrote:
> On Thu, 25 Jun 2020 at 18:22, Maxim Uvarov <[email protected]> wrote:

...

> I guess you missed the point that uuid_t is implemented in BE format
> in the kernel which is compliant as per RFC 4122.

I guess you missed the point. Kernel doesn't have anything special
about these types and does NOT compliant as per RFC.
The only things the kernel distinguishes are a) byte order (always),
and b) version bits when you get a random UUID (whatever you call it).

I guess this discussion takes too much time. The idea is that kernel
types are just for kernel use with as little intrusion as possible.
The main principle, we get something, we carry it w/o modifications
inside the kernel.

So, you may use whatever you want and LE/BE is purely based on a) and b) above.

--
With Best Regards,
Andy Shevchenko

2020-06-26 12:23:12

by Daniel Thompson

[permalink] [raw]

Subject: Re: [Tee-dev] [PATCHv8 1/3] optee: use uuid for sysfs driver entry

On Fri, Jun 26, 2020 at 10:40:41AM +0530, Sumit Garg wrote:
> On Fri, 26 Jun 2020 at 05:01, James Bottomley
> <[email protected]> wrote:
> >
> > On Thu, 2020-06-25 at 19:54 +0530, Sumit Garg wrote:
> > > On Wed, 24 Jun 2020 at 20:51, James Bottomley
> > > <[email protected]> wrote:
> > > >
> > > > On Wed, 2020-06-24 at 16:17 +0530, Sumit Garg wrote:
> > > > > Apologies for delay in my reply as I was busy with some other
> > > > > stuff.
> > > > >
> > > > > On Fri, 19 Jun 2020 at 20:30, James Bottomley
> > > > > <[email protected]> wrote:
> > > >
> > > > [...]
> > > > > > it's about consistency with what the kernel types mean. When
> > > > > > some checker detects your using little endian operations on a
> > > > > > big endian structure (like in the prink for instance) they're
> > > > > > going to keep emailing you about it.
> > > > >
> > > > > As mentioned above, using different terminology is meant to cause
> > > > > more confusion than just difference in endianness which is
> > > > > manageable inside TEE.
> > > > >
> > > > > And I think it's safe to say that the kernel implements UUID in
> > > > > big endian format and thus uses %pUb whereas OP-TEE implements
> > > > > UUID in little endian format and thus uses %pUl.
> > > >
> > > > So what I think you're saying is that if we still had uuid_be and
> > > > uuid_le you'd use uuid_le, because that's exactly the structure
> > > > described in the docs. But because we renamed
> > > >
> > > > uuid_be -> uuid_t
> > > > uuid_le -> guid_t
> > > >
> > > > You can't use guid_t as a kernel type because it has the wrong
> > > > name?
> > >
> > > Isn't the rename commit description [1] pretty clear about which is
> > > the true UUID type from Linux point of view?
> >
> > I don't think the kernel code takes a position on eternal verity, just
> > on logical or arithmetic truth. We just have to deal with both LE and
> > BE UUIDs so we have appropriate types for them and the LE type is now
> > named guid_t. They're both equally correct to use provided the use
> > case matches the designed one. So does the name really matter?
>
> Yes it does. I guess I have provided enough reasoning for that. Also,
> the rename commit itself illustrates its importance and clarifies the
> use case for which they are meant to be used.
>
> > If we
> > did
> >
> > #define uuid_le_t guid_t
> >
> > would you be happy? (not that the kernel cares about karmic emotional
> > states either ...)
>
> It's not about me being happy but more about confusion and
> inconsistency it will bring.
>
> IMO, either kernel should be opinionated about UUID endianness like
> currently it is:
>
> uuid_t and its corresponding helpers (eg. UUID_INIT) follows BE format.
>
> or support both endianness for UUID (no common type: uuid_t) like we
> had earlier prior to rename commit:
>
> uuid_be_t and its corresponding helpers (eg. UUID_BE_INIT) follow BE format.
> uuid_le_t and its corresponding helpers (eg. UUID_LE_INIT) follow LE format.
>
> But even if we consider later case as well, I am still not sure if we
> can switch to uuid_le_t as it's been part of TEE core ABI
> (open_session) where UUID is passed in BE format (see LE to BE
> conversion in TEE client [1] and vice-versa in OP-TEE OS [2]) and
> won't be a backwards compatible change.
>
> [1] https://github.com/OP-TEE/optee_client/blob/master/libteec/src/tee_client_api.c#L595
> [2] https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/kernel/ree_fs_ta.c#L92

I'm struck that all references here are to code that does not run in
kernel space. Frankly on LKML I don't know if we should even *care* what
format UUIDs are stored in other address spaces.

We care about is the endianness of the UUID on the interface boundaries
into and out of the kernel[1] and we care that we use the correct kernel
type to describe it.

I understood from Jerome's post that the UUID that the kernel
manipulates are, in fact, big endian and that they should be called
uuid_t.

Is there more going on here or is that it?

Daniel.

[1] I guess we'd also like those interface boundaries to be reasonably
sane but if they already exist and are the interface is consistent
about endianness then I find it hard to see no reason for a
gratuitous endianness change.

2020-06-26 16:43:53

[permalink] [raw]

Subject: Re: [Tee-dev] [PATCHv8 1/3] optee: use uuid for sysfs driver entry

On Fri, 2020-06-26 at 12:29 +0100, Daniel Thompson wrote:
> On Fri, Jun 26, 2020 at 10:40:41AM +0530, Sumit Garg wrote:
> > On Fri, 26 Jun 2020 at 05:01, James Bottomley
> > <[email protected]> wrote:
> > >
> > > On Thu, 2020-06-25 at 19:54 +0530, Sumit Garg wrote:
> > > > On Wed, 24 Jun 2020 at 20:51, James Bottomley
> > > > <[email protected]> wrote:
> > > > >
> > > > > On Wed, 2020-06-24 at 16:17 +0530, Sumit Garg wrote:
> > > > > > Apologies for delay in my reply as I was busy with some
> > > > > > other stuff.
> > > > > >
> > > > > > On Fri, 19 Jun 2020 at 20:30, James Bottomley
> > > > > > <[email protected]> wrote:
> > > > >
> > > > > [...]
> > > > > > > it's about consistency with what the kernel types
> > > > > > > mean. When some checker detects your using little endian
> > > > > > > operations on a big endian structure (like in the prink
> > > > > > > for instance) they're going to keep emailing you about
> > > > > > > it.
> > > > > >
> > > > > > As mentioned above, using different terminology is meant to
> > > > > > cause more confusion than just difference in endianness
> > > > > > which is manageable inside TEE.
> > > > > >
> > > > > > And I think it's safe to say that the kernel implements
> > > > > > UUID in big endian format and thus uses %pUb whereas OP-TEE
> > > > > > implements UUID in little endian format and thus uses %pUl.
> > > > >
> > > > > So what I think you're saying is that if we still had uuid_be
> > > > > and uuid_le you'd use uuid_le, because that's exactly the
> > > > > structure described in the docs. But because we renamed
> > > > >
> > > > > uuid_be -> uuid_t
> > > > > uuid_le -> guid_t
> > > > >
> > > > > You can't use guid_t as a kernel type because it has the
> > > > > wrong name?
> > > >
> > > > Isn't the rename commit description [1] pretty clear about
> > > > which is the true UUID type from Linux point of view?
> > >
> > > I don't think the kernel code takes a position on eternal verity,
> > > just on logical or arithmetic truth. We just have to deal with
> > > both LE and BE UUIDs so we have appropriate types for them and
> > > the LE type is now named guid_t. They're both equally correct to
> > > use provided the use case matches the designed one. So does the
> > > name really matter?
> >
> > Yes it does. I guess I have provided enough reasoning for that.
> > Also, the rename commit itself illustrates its importance and
> > clarifies the use case for which they are meant to be used.
> >
> > > If we did
> > >
> > > #define uuid_le_t guid_t
> > >
> > > would you be happy? (not that the kernel cares about karmic
> > > emotional states either ...)
> >
> > It's not about me being happy but more about confusion and
> > inconsistency it will bring.
> >
> > IMO, either kernel should be opinionated about UUID endianness like
> > currently it is:
> >
> > uuid_t and its corresponding helpers (eg. UUID_INIT) follows BE
> > format.
> >
> > or support both endianness for UUID (no common type: uuid_t) like
> > we had earlier prior to rename commit:
> >
> > uuid_be_t and its corresponding helpers (eg. UUID_BE_INIT) follow
> > BE format. uuid_le_t and its corresponding helpers (eg.
> > UUID_LE_INIT) follow LE format.
> >
> > But even if we consider later case as well, I am still not sure if
> > we can switch to uuid_le_t as it's been part of TEE core ABI
> > (open_session) where UUID is passed in BE format (see LE to BE
> > conversion in TEE client [1] and vice-versa in OP-TEE OS [2]) and
> > won't be a backwards compatible change.
> >
> > [1] https://github.com/OP-TEE/optee_client/blob/master/libteec/src/
> > tee_client_api.c#L595
> > [2] https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/ke
> > rnel/ree_fs_ta.c#L92
>
> I'm struck that all references here are to code that does not run in
> kernel space. Frankly on LKML I don't know if we should even *care*
> what format UUIDs are stored in other address spaces.
>
> We care about is the endianness of the UUID on the interface
> boundaries into and out of the kernel[1] and we care that we use the
> correct kernel type to describe it.
>
> I understood from Jerome's post that the UUID that the kernel
> manipulates are, in fact, big endian and that they should be called
> uuid_t.
>
> Is there more going on here or is that it?

As you say, a UUID to the kernel is a binary blob except for input,
which to the kernel is INIT_UUID or INIT_GUID and output, which is
either printk %Ub for uuid_t or %Ul for guid_t.

The bit I objected to was doing a %Ul on a uuid_t because it's going to
trip the static checkers. That shows me there's a confusion in the
code between little and big endian UUID types, but I haven't looked
further than that.

James

2020-06-29 20:45:40

[permalink] [raw]

Subject: Re: [Tee-dev] [PATCHv8 1/3] optee: use uuid for sysfs driver entry

On Fri, Jun 26, 2020 at 08:11:21AM -0700, James Bottomley wrote:
> On Fri, 2020-06-26 at 12:29 +0100, Daniel Thompson wrote:
> > On Fri, Jun 26, 2020 at 10:40:41AM +0530, Sumit Garg wrote:
> > > On Fri, 26 Jun 2020 at 05:01, James Bottomley
> > > <[email protected]> wrote:
> > > >
> > > > On Thu, 2020-06-25 at 19:54 +0530, Sumit Garg wrote:
> > > > > On Wed, 24 Jun 2020 at 20:51, James Bottomley
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > On Wed, 2020-06-24 at 16:17 +0530, Sumit Garg wrote:
> > > > > > > Apologies for delay in my reply as I was busy with some
> > > > > > > other stuff.
> > > > > > >
> > > > > > > On Fri, 19 Jun 2020 at 20:30, James Bottomley
> > > > > > > <[email protected]> wrote:
> > > > > >
> > > > > > [...]
> > > > > > > > it's about consistency with what the kernel types
> > > > > > > > mean. When some checker detects your using little endian
> > > > > > > > operations on a big endian structure (like in the prink
> > > > > > > > for instance) they're going to keep emailing you about
> > > > > > > > it.
> > > > > > >
> > > > > > > As mentioned above, using different terminology is meant to
> > > > > > > cause more confusion than just difference in endianness
> > > > > > > which is manageable inside TEE.
> > > > > > >
> > > > > > > And I think it's safe to say that the kernel implements
> > > > > > > UUID in big endian format and thus uses %pUb whereas OP-TEE
> > > > > > > implements UUID in little endian format and thus uses %pUl.
> > > > > >
> > > > > > So what I think you're saying is that if we still had uuid_be
> > > > > > and uuid_le you'd use uuid_le, because that's exactly the
> > > > > > structure described in the docs. But because we renamed
> > > > > >
> > > > > > uuid_be -> uuid_t
> > > > > > uuid_le -> guid_t
> > > > > >
> > > > > > You can't use guid_t as a kernel type because it has the
> > > > > > wrong name?
> > > > >
> > > > > Isn't the rename commit description [1] pretty clear about
> > > > > which is the true UUID type from Linux point of view?
> > > >
> > > > I don't think the kernel code takes a position on eternal verity,
> > > > just on logical or arithmetic truth. We just have to deal with
> > > > both LE and BE UUIDs so we have appropriate types for them and
> > > > the LE type is now named guid_t. They're both equally correct to
> > > > use provided the use case matches the designed one. So does the
> > > > name really matter?
> > >
> > > Yes it does. I guess I have provided enough reasoning for that.
> > > Also, the rename commit itself illustrates its importance and
> > > clarifies the use case for which they are meant to be used.
> > >
> > > > If we did
> > > >
> > > > #define uuid_le_t guid_t
> > > >
> > > > would you be happy? (not that the kernel cares about karmic
> > > > emotional states either ...)
> > >
> > > It's not about me being happy but more about confusion and
> > > inconsistency it will bring.
> > >
> > > IMO, either kernel should be opinionated about UUID endianness like
> > > currently it is:
> > >
> > > uuid_t and its corresponding helpers (eg. UUID_INIT) follows BE
> > > format.
> > >
> > > or support both endianness for UUID (no common type: uuid_t) like
> > > we had earlier prior to rename commit:
> > >
> > > uuid_be_t and its corresponding helpers (eg. UUID_BE_INIT) follow
> > > BE format. uuid_le_t and its corresponding helpers (eg.
> > > UUID_LE_INIT) follow LE format.
> > >
> > > But even if we consider later case as well, I am still not sure if
> > > we can switch to uuid_le_t as it's been part of TEE core ABI
> > > (open_session) where UUID is passed in BE format (see LE to BE
> > > conversion in TEE client [1] and vice-versa in OP-TEE OS [2]) and
> > > won't be a backwards compatible change.
> > >
> > > [1] https://github.com/OP-TEE/optee_client/blob/master/libteec/src/
> > > tee_client_api.c#L595
> > > [2] https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/ke
> > > rnel/ree_fs_ta.c#L92
> >
> > I'm struck that all references here are to code that does not run in
> > kernel space. Frankly on LKML I don't know if we should even *care*
> > what format UUIDs are stored in other address spaces.
> >
> > We care about is the endianness of the UUID on the interface
> > boundaries into and out of the kernel[1] and we care that we use the
> > correct kernel type to describe it.
> >
> > I understood from Jerome's post that the UUID that the kernel
> > manipulates are, in fact, big endian and that they should be called
> > uuid_t.
> >
> > Is there more going on here or is that it?
>
> As you say, a UUID to the kernel is a binary blob except for input,
> which to the kernel is INIT_UUID or INIT_GUID and output, which is
> either printk %Ub for uuid_t or %Ul for guid_t.
>
> The bit I objected to was doing a %Ul on a uuid_t because it's going to
> trip the static checkers. That shows me there's a confusion in the
> code between little and big endian UUID types, but I haven't looked
> further than that.

Thanks for bringing our attention to this, it was educating for me at
least. It seems the only problem was just the %Ul that should have been
a %Ub. The OP-TEE driver is only dealing with BE UUIDs internallay and
in the ABIs to user space and secure world.

I agree with Daniel, what happens in user space and in secure world
isn't of interest here as long as the ABIs are OK.

Cheers,
Jens

2020-06-29 20:47:24