v2: - write TEE with capital letters. - declare __optee_enumerate_device() as static.
Hello,
This patchset fixes issues with probing() tee, optee and optee driver if they were compiled into kernel, built as modules or any mixed combination. These changes require optee-os changes which already were merged. Main corresponding commits are: https://github.com/OP-TEE/optee_os/commit/9389d8030ef198c9d7b8ab7ea8e877e0ac... https://github.com/OP-TEE/optee_os/commit/bc5921cdab538c8ae48422f5ffd600f1cb...
optee_enumerate_devices() which discovers Trusted Applications on tee bus is split up on 2 changes. Do probe of drivers which do not require userspace support of tee-supplicant and stage two to run drivers with support of tee-supplicant only after tee supplicant run.
Best regards, Maxim.
Maxim Uvarov (2): optee: do drivers initialization before and after tee-supplicant run tpm_ftpm_tee: register driver on TEE bus
drivers/char/tpm/tpm_ftpm_tee.c | 69 ++++++++++++++++++++++++++----- drivers/tee/optee/core.c | 25 +++++++++-- drivers/tee/optee/device.c | 17 +++++--- drivers/tee/optee/optee_private.h | 8 +++- 4 files changed, 99 insertions(+), 20 deletions(-)
Some drivers (like ftpm) can operate only after tee-supplicant runs becase of tee-supplicant provides things like storage services. This patch splits probe of non tee-supplicant dependable drivers to early stage, and after tee-supplicant run probe other drivers.
Signed-off-by: Maxim Uvarov maxim.uvarov@linaro.org Suggested-by: Sumit Garg sumit.garg@linaro.org Suggested-by: Arnd Bergmann arnd@linaro.org --- drivers/tee/optee/core.c | 25 ++++++++++++++++++++++--- drivers/tee/optee/device.c | 17 +++++++++++------ drivers/tee/optee/optee_private.h | 8 +++++++- 3 files changed, 40 insertions(+), 10 deletions(-)
diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c index 99698b8a3a74..dd2265c44907 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,15 @@ static void optee_get_version(struct tee_device *teedev, *vers = v; }
+static void optee_bus_scan(struct work_struct *work) +{ + int rc; + + rc = optee_enumerate_devices(PTA_CMD_GET_DEVICES_SUPP); + if (rc) + pr_err("optee_enumerate_devices failed %d\n", rc); +} + static int optee_open(struct tee_context *ctx) { struct optee_context_data *ctxdata; @@ -241,8 +251,15 @@ static int optee_open(struct tee_context *ctx) kfree(ctxdata); return -EBUSY; } - }
+ INIT_WORK(&optee->scan_bus_work, optee_bus_scan); + optee->scan_bus_wq = create_workqueue("optee_bus_scan"); + if (!optee->scan_bus_wq) { + pr_err("optee: couldn't create workqueue\n"); + return -ECHILD; + } + queue_work(optee->scan_bus_wq, &optee->scan_bus_work); + } mutex_init(&ctxdata->mutex); INIT_LIST_HEAD(&ctxdata->sess_list);
@@ -296,8 +313,10 @@ static void optee_release(struct tee_context *ctx)
ctx->data = NULL;
- if (teedev == optee->supp_teedev) + if (teedev == optee->supp_teedev) { + destroy_workqueue(optee->scan_bus_wq); optee_supp_release(&optee->supp); + } }
static const struct tee_driver_ops optee_ops = { @@ -675,7 +694,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 e3a148521ec1..d4931dad07aa 100644 --- a/drivers/tee/optee/device.c +++ b/drivers/tee/optee/device.c @@ -21,7 +21,6 @@ * 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) { @@ -32,7 +31,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; @@ -42,7 +42,7 @@ static int get_devices(struct tee_context *ctx, u32 session, memset(¶m, 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;
@@ -87,7 +87,7 @@ static int optee_register_device(const uuid_t *device_uuid, u32 device_id) return rc; }
-int optee_enumerate_devices(void) +static int __optee_enumerate_devices(u32 func) { const uuid_t pta_uuid = UUID_INIT(0x7011a688, 0xddde, 0x4053, @@ -118,7 +118,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;
@@ -130,7 +130,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;
@@ -158,3 +158,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..6cdac4bb7253 100644 --- a/drivers/tee/optee/optee_private.h +++ b/drivers/tee/optee/optee_private.h @@ -78,6 +78,8 @@ 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_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 +91,8 @@ struct optee { struct tee_shm_pool *pool; void *memremaped_shm; u32 sec_caps; + struct workqueue_struct *scan_bus_wq; + struct work_struct scan_bus_work; };
struct optee_session { @@ -173,7 +177,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
Register driver on tee bus. module tee registers bus, and module optee calls optee_enumerate_devices() to scan all devices on the bus. This TA can be Early TA's ( can be compiled into optee-os). In that case it will be on optee bus before linux booting. Also optee-suplicant application is needed to be loaded between optee module and ftpm module to to maintain functionality for ftpm driver.
Signed-off-by: Maxim Uvarov maxim.uvarov@linaro.org Suggested-by: Sumit Garg sumit.garg@linaro.org Suggested-by: Arnd Bergmann arnd@linaro.org --- drivers/char/tpm/tpm_ftpm_tee.c | 69 ++++++++++++++++++++++++++++----- 1 file changed, 59 insertions(+), 10 deletions(-)
diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c index 22bf553ccf9d..7bb4ce281050 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,53 @@ 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, +}; + +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 thiruan@microsoft.com"); MODULE_DESCRIPTION("TPM Driver for fTPM TA in TEE");
Register driver on TEE bus. module tee registers bus, and module optee calls optee_enumerate_devices() to scan all devices on the bus. Trusted Application for this driver can be Early TA's (can be compiled into optee-os). In that case it will be on OPTEE bus before linux booting. Also optee-suplicant application is needed to be loaded between OPTEE module and ftpm module to maintain functionality for fTPM driver.
Signed-off-by: Maxim Uvarov maxim.uvarov@linaro.org Suggested-by: Sumit Garg sumit.garg@linaro.org Suggested-by: Arnd Bergmann arnd@linaro.org --- drivers/char/tpm/tpm_ftpm_tee.c | 69 ++++++++++++++++++++++++++++----- 1 file changed, 59 insertions(+), 10 deletions(-)
diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c index 22bf553ccf9d..7bb4ce281050 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,53 @@ 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, +}; + +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 thiruan@microsoft.com"); MODULE_DESCRIPTION("TPM Driver for fTPM TA in TEE");
Hi Maxim,
On Thu, 21 May 2020 at 12:17, Maxim Uvarov maxim.uvarov@linaro.org wrote:
Some drivers (like ftpm) can operate only after tee-supplicant runs becase of tee-supplicant provides things like storage services. This patch splits probe of non tee-supplicant dependable drivers to early stage, and after tee-supplicant run probe other drivers.
Signed-off-by: Maxim Uvarov maxim.uvarov@linaro.org Suggested-by: Sumit Garg sumit.garg@linaro.org Suggested-by: Arnd Bergmann arnd@linaro.org
drivers/tee/optee/core.c | 25 ++++++++++++++++++++++--- drivers/tee/optee/device.c | 17 +++++++++++------ drivers/tee/optee/optee_private.h | 8 +++++++- 3 files changed, 40 insertions(+), 10 deletions(-)
This patch still doesn't seem to address the issue (duplicate devices leading to sysfs registration failure) that was pointed during OP-TEE patch review [1].
You could reproduce it simply with one TA having TA_FLAG_DEVICE_ENUM flag and other TA having TA_FLAG_DEVICE_ENUM_SUPP flag.
[1] https://github.com/OP-TEE/optee_os/pull/3840#discussion_r425144487
-Sumit
diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c index 99698b8a3a74..dd2265c44907 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,15 @@ static void optee_get_version(struct tee_device *teedev, *vers = v; }
+static void optee_bus_scan(struct work_struct *work) +{
int rc;rc = optee_enumerate_devices(PTA_CMD_GET_DEVICES_SUPP);if (rc)pr_err("optee_enumerate_devices failed %d\n", rc);+}
static int optee_open(struct tee_context *ctx) { struct optee_context_data *ctxdata; @@ -241,8 +251,15 @@ static int optee_open(struct tee_context *ctx) kfree(ctxdata); return -EBUSY; }
}
INIT_WORK(&optee->scan_bus_work, optee_bus_scan);optee->scan_bus_wq = create_workqueue("optee_bus_scan");if (!optee->scan_bus_wq) {pr_err("optee: couldn't create workqueue\n");return -ECHILD;}queue_work(optee->scan_bus_wq, &optee->scan_bus_work);} mutex_init(&ctxdata->mutex); INIT_LIST_HEAD(&ctxdata->sess_list);@@ -296,8 +313,10 @@ static void optee_release(struct tee_context *ctx)
ctx->data = NULL;
if (teedev == optee->supp_teedev)
if (teedev == optee->supp_teedev) {destroy_workqueue(optee->scan_bus_wq); optee_supp_release(&optee->supp);}}
static const struct tee_driver_ops optee_ops = { @@ -675,7 +694,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 e3a148521ec1..d4931dad07aa 100644 --- a/drivers/tee/optee/device.c +++ b/drivers/tee/optee/device.c @@ -21,7 +21,6 @@
- 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) { @@ -32,7 +31,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; @@ -42,7 +42,7 @@ static int get_devices(struct tee_context *ctx, u32 session, memset(¶m, 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;@@ -87,7 +87,7 @@ static int optee_register_device(const uuid_t *device_uuid, u32 device_id) return rc; }
-int optee_enumerate_devices(void) +static int __optee_enumerate_devices(u32 func) { const uuid_t pta_uuid = UUID_INIT(0x7011a688, 0xddde, 0x4053, @@ -118,7 +118,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;@@ -130,7 +130,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;@@ -158,3 +158,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..6cdac4bb7253 100644 --- a/drivers/tee/optee/optee_private.h +++ b/drivers/tee/optee/optee_private.h @@ -78,6 +78,8 @@ 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_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 +91,8 @@ struct optee { struct tee_shm_pool *pool; void *memremaped_shm; u32 sec_caps;
struct workqueue_struct *scan_bus_wq;struct work_struct scan_bus_work;};
struct optee_session { @@ -173,7 +177,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
On Fri, 22 May 2020 at 14:40, Sumit Garg sumit.garg@linaro.org wrote:
Hi Maxim,
On Thu, 21 May 2020 at 12:17, Maxim Uvarov maxim.uvarov@linaro.org wrote:
Some drivers (like ftpm) can operate only after tee-supplicant runs becase of tee-supplicant provides things like storage services. This patch splits probe of non tee-supplicant dependable drivers to early stage, and after tee-supplicant run probe other drivers.
Signed-off-by: Maxim Uvarov maxim.uvarov@linaro.org Suggested-by: Sumit Garg sumit.garg@linaro.org Suggested-by: Arnd Bergmann arnd@linaro.org
drivers/tee/optee/core.c | 25 ++++++++++++++++++++++--- drivers/tee/optee/device.c | 17 +++++++++++------ drivers/tee/optee/optee_private.h | 8 +++++++- 3 files changed, 40 insertions(+), 10 deletions(-)
This patch still doesn't seem to address the issue (duplicate devices leading to sysfs registration failure) that was pointed during OP-TEE patch review [1].
You could reproduce it simply with one TA having TA_FLAG_DEVICE_ENUM flag and other TA having TA_FLAG_DEVICE_ENUM_SUPP flag.
[1] https://github.com/OP-TEE/optee_os/pull/3840#discussion_r425144487
-Sumit
Ah, yes. I will send an updated version.
Maxim.
diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c index 99698b8a3a74..dd2265c44907 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,15 @@ static void optee_get_version(struct tee_device *teedev, *vers = v; }
+static void optee_bus_scan(struct work_struct *work) +{
int rc;rc = optee_enumerate_devices(PTA_CMD_GET_DEVICES_SUPP);if (rc)pr_err("optee_enumerate_devices failed %d\n", rc);+}
static int optee_open(struct tee_context *ctx) { struct optee_context_data *ctxdata; @@ -241,8 +251,15 @@ static int optee_open(struct tee_context *ctx) kfree(ctxdata); return -EBUSY; }
}
INIT_WORK(&optee->scan_bus_work, optee_bus_scan);optee->scan_bus_wq = create_workqueue("optee_bus_scan");if (!optee->scan_bus_wq) {pr_err("optee: couldn't create workqueue\n");return -ECHILD;}queue_work(optee->scan_bus_wq, &optee->scan_bus_work);} mutex_init(&ctxdata->mutex); INIT_LIST_HEAD(&ctxdata->sess_list);@@ -296,8 +313,10 @@ static void optee_release(struct tee_context *ctx)
ctx->data = NULL;
if (teedev == optee->supp_teedev)
if (teedev == optee->supp_teedev) {destroy_workqueue(optee->scan_bus_wq); optee_supp_release(&optee->supp);}}
static const struct tee_driver_ops optee_ops = { @@ -675,7 +694,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 e3a148521ec1..d4931dad07aa 100644 --- a/drivers/tee/optee/device.c +++ b/drivers/tee/optee/device.c @@ -21,7 +21,6 @@
- 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) { @@ -32,7 +31,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; @@ -42,7 +42,7 @@ static int get_devices(struct tee_context *ctx, u32 session, memset(¶m, 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;@@ -87,7 +87,7 @@ static int optee_register_device(const uuid_t *device_uuid, u32 device_id) return rc; }
-int optee_enumerate_devices(void) +static int __optee_enumerate_devices(u32 func) { const uuid_t pta_uuid = UUID_INIT(0x7011a688, 0xddde, 0x4053, @@ -118,7 +118,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;@@ -130,7 +130,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;@@ -158,3 +158,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..6cdac4bb7253 100644 --- a/drivers/tee/optee/optee_private.h +++ b/drivers/tee/optee/optee_private.h @@ -78,6 +78,8 @@ 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_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 +91,8 @@ struct optee { struct tee_shm_pool *pool; void *memremaped_shm; u32 sec_caps;
struct workqueue_struct *scan_bus_wq;struct work_struct scan_bus_work;};
struct optee_session { @@ -173,7 +177,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
On Thu, May 21, 2020 at 09:47:43AM +0300, Maxim Uvarov wrote:
Register driver on TEE bus. module tee registers bus,
"on the TEE bus"
"The module tee"
and module optee calls optee_enumerate_devices() to scan all devices on the bus. Trusted Application for this driver
Looking at drivers/tee, it shows that tee and optee are in fact the same module as opposed to what your commit message says.
can be Early TA's (can be compiled into optee-os). In that case it will be on OPTEE bus before linux booting. Also optee-suplicant application is needed to be loaded between OPTEE module and ftpm module to maintain functionality for fTPM driver.
Why is this needed and why things worked before having this?
Signed-off-by: Maxim Uvarov maxim.uvarov@linaro.org Suggested-by: Sumit Garg sumit.garg@linaro.org Suggested-by: Arnd Bergmann arnd@linaro.org
drivers/char/tpm/tpm_ftpm_tee.c | 69 ++++++++++++++++++++++++++++----- 1 file changed, 59 insertions(+), 10 deletions(-)
diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c index 22bf553ccf9d..7bb4ce281050 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,53 @@ 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,
+};
+static const struct tee_client_device_id optee_ftpm_id_table[] = {
- {UUID_INIT(0xbc50d971, 0xd4c9, 0x42c4,
0x82, 0xcb, 0x34, 0x3f, 0xb7, 0xf3, 0x78, 0x96)},- {}
Please put a comment describing what this is.
}; -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 thiruan@microsoft.com"); MODULE_DESCRIPTION("TPM Driver for fTPM TA in TEE"); -- 2.17.1
Wondering if MODULE_AUTHOR() is still equired given that the GIT log has objectively better log of authorship.
/Jarkko
On Fri, 22 May 2020 at 20:15, Jarkko Sakkinen jarkko.sakkinen@linux.intel.com wrote:
On Thu, May 21, 2020 at 09:47:43AM +0300, Maxim Uvarov wrote:
Register driver on TEE bus. module tee registers bus,
"on the TEE bus"
"The module tee"
and module optee calls optee_enumerate_devices() to scan all devices on the bus. Trusted Application for this driver
Looking at drivers/tee, it shows that tee and optee are in fact the same module as opposed to what your commit message says.
In the current kernel it's 2 different modules.
can be Early TA's (can be compiled into optee-os). In that case it will be on OPTEE bus before linux booting. Also optee-suplicant application is needed to be loaded between OPTEE module and ftpm module to maintain functionality for fTPM driver.
Why is this needed and why things worked before having this?
Before these changes user space has to drive boot sequence. I.e. tee and optee modules loads, then application tee-supplicant has to start and only then module ftpm can be loaded. The reason for that is storage services are implemented in userspace and driver needs them. To make it work needed some tricky systemd scenario with black list driver mode. That really did not look well. From the over side ftpm might be used by uboot so it can be compiled inside firmware (bl32/optee-os). On the other hand there are drivers like tee-rng which do not need tee-supplicant and can run immediately after tee and optee modules. So the solution as I described in the commit is to make tee bus work for both types of drivers.
Also in this driver I specially did not remove the old way of registration using DT entry. To not break existence things. And added new registration from the platform driver.
Signed-off-by: Maxim Uvarov maxim.uvarov@linaro.org Suggested-by: Sumit Garg sumit.garg@linaro.org Suggested-by: Arnd Bergmann arnd@linaro.org
drivers/char/tpm/tpm_ftpm_tee.c | 69 ++++++++++++++++++++++++++++----- 1 file changed, 59 insertions(+), 10 deletions(-)
diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c index 22bf553ccf9d..7bb4ce281050 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,53 @@ 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,+};
+static const struct tee_client_device_id optee_ftpm_id_table[] = {
{UUID_INIT(0xbc50d971, 0xd4c9, 0x42c4,0x82, 0xcb, 0x34, 0x3f, 0xb7, 0xf3, 0x78, 0x96)},{}Please put a comment describing what this is.
};
-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 thiruan@microsoft.com"); MODULE_DESCRIPTION("TPM Driver for fTPM TA in TEE"); -- 2.17.1
Wondering if MODULE_AUTHOR() is still equired given that the GIT log has objectively better log of authorship.
/Jarkko
Yea, but I prefer to not touch such lines. It's up to maintainers how they support the driver.
Best regards, Maxim.
On Fri, May 22, 2020 at 10:29:44PM +0300, Maxim Uvarov wrote:
On Fri, 22 May 2020 at 20:15, Jarkko Sakkinen jarkko.sakkinen@linux.intel.com wrote:
On Thu, May 21, 2020 at 09:47:43AM +0300, Maxim Uvarov wrote:
Register driver on TEE bus. module tee registers bus,
"on the TEE bus"
"The module tee"
and module optee calls optee_enumerate_devices() to scan all devices on the bus. Trusted Application for this driver
Looking at drivers/tee, it shows that tee and optee are in fact the same module as opposed to what your commit message says.
In the current kernel it's 2 different modules.
can be Early TA's (can be compiled into optee-os). In that case it will be on OPTEE bus before linux booting. Also optee-suplicant application is needed to be loaded between OPTEE module and ftpm module to maintain functionality for fTPM driver.
Why is this needed and why things worked before having this?
Before these changes user space has to drive boot sequence. I.e. tee and optee modules loads, then application tee-supplicant has to start and only then module ftpm can be loaded. The reason for that is storage services are implemented in userspace and driver needs them.
Is the TPM implementation uploaded to TEE from user space and or what storage are we talking about? Not sure how these storage services connect to the TPM.
/Jarkko
Hi Maxim,
On Thu, May 21, 2020 at 8:47 AM Maxim Uvarov maxim.uvarov@linaro.org wrote:
Some drivers (like ftpm) can operate only after tee-supplicant runs becase of tee-supplicant provides things like storage services. This patch splits probe of non tee-supplicant dependable drivers to early stage, and after tee-supplicant run probe other drivers.
Signed-off-by: Maxim Uvarov maxim.uvarov@linaro.org Suggested-by: Sumit Garg sumit.garg@linaro.org Suggested-by: Arnd Bergmann arnd@linaro.org
drivers/tee/optee/core.c | 25 ++++++++++++++++++++++--- drivers/tee/optee/device.c | 17 +++++++++++------ drivers/tee/optee/optee_private.h | 8 +++++++- 3 files changed, 40 insertions(+), 10 deletions(-)
diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c index 99698b8a3a74..dd2265c44907 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,15 @@ static void optee_get_version(struct tee_device *teedev, *vers = v; }
+static void optee_bus_scan(struct work_struct *work) +{
int rc;rc = optee_enumerate_devices(PTA_CMD_GET_DEVICES_SUPP);if (rc)pr_err("optee_enumerate_devices failed %d\n", rc);+}
static int optee_open(struct tee_context *ctx) { struct optee_context_data *ctxdata; @@ -241,8 +251,15 @@ static int optee_open(struct tee_context *ctx) kfree(ctxdata); return -EBUSY; }
}
INIT_WORK(&optee->scan_bus_work, optee_bus_scan);optee->scan_bus_wq = create_workqueue("optee_bus_scan");if (!optee->scan_bus_wq) {pr_err("optee: couldn't create workqueue\n");return -ECHILD;}queue_work(optee->scan_bus_wq, &optee->scan_bus_work);
Shouldn't this be done only the first time tee-supplicant opens the device? Normally tee-supplicant only does this once, but it might get restarted for some reason.
} mutex_init(&ctxdata->mutex); INIT_LIST_HEAD(&ctxdata->sess_list);@@ -296,8 +313,10 @@ static void optee_release(struct tee_context *ctx)
ctx->data = NULL;
if (teedev == optee->supp_teedev)
if (teedev == optee->supp_teedev) {destroy_workqueue(optee->scan_bus_wq); optee_supp_release(&optee->supp);}}
static const struct tee_driver_ops optee_ops = { @@ -675,7 +694,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 e3a148521ec1..d4931dad07aa 100644 --- a/drivers/tee/optee/device.c +++ b/drivers/tee/optee/device.c @@ -21,7 +21,6 @@
- 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) { @@ -32,7 +31,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; @@ -42,7 +42,7 @@ static int get_devices(struct tee_context *ctx, u32 session, memset(¶m, 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;@@ -87,7 +87,7 @@ static int optee_register_device(const uuid_t *device_uuid, u32 device_id) return rc; }
-int optee_enumerate_devices(void) +static int __optee_enumerate_devices(u32 func) { const uuid_t pta_uuid = UUID_INIT(0x7011a688, 0xddde, 0x4053, @@ -118,7 +118,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;@@ -130,7 +130,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;@@ -158,3 +158,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..6cdac4bb7253 100644 --- a/drivers/tee/optee/optee_private.h +++ b/drivers/tee/optee/optee_private.h @@ -78,6 +78,8 @@ 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_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 +91,8 @@ struct optee { struct tee_shm_pool *pool; void *memremaped_shm; u32 sec_caps;
struct workqueue_struct *scan_bus_wq;struct work_struct scan_bus_work;};
struct optee_session { @@ -173,7 +177,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
On Fri, 22 May 2020 at 23:03, Jarkko Sakkinen jarkko.sakkinen@linux.intel.com wrote:
On Fri, May 22, 2020 at 10:29:44PM +0300, Maxim Uvarov wrote:
On Fri, 22 May 2020 at 20:15, Jarkko Sakkinen jarkko.sakkinen@linux.intel.com wrote:
On Thu, May 21, 2020 at 09:47:43AM +0300, Maxim Uvarov wrote:
Register driver on TEE bus. module tee registers bus,
"on the TEE bus"
"The module tee"
and module optee calls optee_enumerate_devices() to scan all devices on the bus. Trusted Application for this driver
Looking at drivers/tee, it shows that tee and optee are in fact the same module as opposed to what your commit message says.
In the current kernel it's 2 different modules.
can be Early TA's (can be compiled into optee-os). In that case it will be on OPTEE bus before linux booting. Also optee-suplicant application is needed to be loaded between OPTEE module and ftpm module to maintain functionality for fTPM driver.
Why is this needed and why things worked before having this?
Before these changes user space has to drive boot sequence. I.e. tee and optee modules loads, then application tee-supplicant has to start and only then module ftpm can be loaded. The reason for that is storage services are implemented in userspace and driver needs them.
Is the TPM implementation uploaded to TEE from user space and or what storage are we talking about? Not sure how these storage services connect to the TPM.
/Jarkko
Jakko, tee-supplicant application provides state machine over callbacks with RPC messages. https://github.com/OP-TEE/optee_client/blob/master/tee-supplicant/src/tee_su... It also allocates shm. Without running tee-supplicant tee_client_open_session() will fail. optee_open_session()->get_msg_arg()->tee_shm_alloc()->... Optee team wanted to remove some dependencies from tee-supplicant with moving code to the kernel. But for now I think that should be out of the scope of current patches due to they fix driver initialization on tee bus without breaking current functionality.
Maxim.
On Sun, 24 May 2020 at 09:22, Jens Wiklander jens.wiklander@linaro.org wrote:
Hi Maxim,
On Thu, May 21, 2020 at 8:47 AM Maxim Uvarov maxim.uvarov@linaro.org wrote:
Some drivers (like ftpm) can operate only after tee-supplicant runs becase of tee-supplicant provides things like storage services. This patch splits probe of non tee-supplicant dependable drivers to early stage, and after tee-supplicant run probe other drivers.
Signed-off-by: Maxim Uvarov maxim.uvarov@linaro.org Suggested-by: Sumit Garg sumit.garg@linaro.org Suggested-by: Arnd Bergmann arnd@linaro.org
drivers/tee/optee/core.c | 25 ++++++++++++++++++++++--- drivers/tee/optee/device.c | 17 +++++++++++------ drivers/tee/optee/optee_private.h | 8 +++++++- 3 files changed, 40 insertions(+), 10 deletions(-)
diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c index 99698b8a3a74..dd2265c44907 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,15 @@ static void optee_get_version(struct tee_device *teedev, *vers = v; }
+static void optee_bus_scan(struct work_struct *work) +{
int rc;rc = optee_enumerate_devices(PTA_CMD_GET_DEVICES_SUPP);if (rc)pr_err("optee_enumerate_devices failed %d\n", rc);+}
static int optee_open(struct tee_context *ctx) { struct optee_context_data *ctxdata; @@ -241,8 +251,15 @@ static int optee_open(struct tee_context *ctx) kfree(ctxdata); return -EBUSY; }
}
INIT_WORK(&optee->scan_bus_work, optee_bus_scan);optee->scan_bus_wq = create_workqueue("optee_bus_scan");if (!optee->scan_bus_wq) {pr_err("optee: couldn't create workqueue\n");return -ECHILD;}queue_work(optee->scan_bus_wq, &optee->scan_bus_work);Shouldn't this be done only the first time tee-supplicant opens the device? Normally tee-supplicant only does this once, but it might get restarted for some reason.
Yes, thanks. I will add this to the next version.
Maxim.
} mutex_init(&ctxdata->mutex); INIT_LIST_HEAD(&ctxdata->sess_list);@@ -296,8 +313,10 @@ static void optee_release(struct tee_context *ctx)
ctx->data = NULL;
if (teedev == optee->supp_teedev)
if (teedev == optee->supp_teedev) {destroy_workqueue(optee->scan_bus_wq); optee_supp_release(&optee->supp);}}
static const struct tee_driver_ops optee_ops = { @@ -675,7 +694,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 e3a148521ec1..d4931dad07aa 100644 --- a/drivers/tee/optee/device.c +++ b/drivers/tee/optee/device.c @@ -21,7 +21,6 @@
- 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) { @@ -32,7 +31,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; @@ -42,7 +42,7 @@ static int get_devices(struct tee_context *ctx, u32 session, memset(¶m, 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;@@ -87,7 +87,7 @@ static int optee_register_device(const uuid_t *device_uuid, u32 device_id) return rc; }
-int optee_enumerate_devices(void) +static int __optee_enumerate_devices(u32 func) { const uuid_t pta_uuid = UUID_INIT(0x7011a688, 0xddde, 0x4053, @@ -118,7 +118,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;@@ -130,7 +130,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;@@ -158,3 +158,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..6cdac4bb7253 100644 --- a/drivers/tee/optee/optee_private.h +++ b/drivers/tee/optee/optee_private.h @@ -78,6 +78,8 @@ 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_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 +91,8 @@ struct optee { struct tee_shm_pool *pool; void *memremaped_shm; u32 sec_caps;
struct workqueue_struct *scan_bus_wq;struct work_struct scan_bus_work;};
struct optee_session { @@ -173,7 +177,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
+ Thiru (original author), Ilias, Joakim
Hi Jarkko,
On Mon, 25 May 2020 at 12:20, Maxim Uvarov maxim.uvarov@linaro.org wrote:
On Fri, 22 May 2020 at 23:03, Jarkko Sakkinen jarkko.sakkinen@linux.intel.com wrote:
On Fri, May 22, 2020 at 10:29:44PM +0300, Maxim Uvarov wrote:
On Fri, 22 May 2020 at 20:15, Jarkko Sakkinen jarkko.sakkinen@linux.intel.com wrote:
On Thu, May 21, 2020 at 09:47:43AM +0300, Maxim Uvarov wrote:
Register driver on TEE bus. module tee registers bus,
"on the TEE bus"
"The module tee"
and module optee calls optee_enumerate_devices() to scan all devices on the bus. Trusted Application for this driver
Looking at drivers/tee, it shows that tee and optee are in fact the same module as opposed to what your commit message says.
In the current kernel it's 2 different modules.
can be Early TA's (can be compiled into optee-os). In that case it will be on OPTEE bus before linux booting. Also optee-suplicant application is needed to be loaded between OPTEE module and ftpm module to maintain functionality for fTPM driver.
Why is this needed and why things worked before having this?
If you remembered earlier discussions when this driver was added, tee-supplicant dependency concern was raised at that time too. But the response from author [1] was to use a workaround in firmware to overcome the initialization issue of fTPM driver.
Basically while using upstream OP-TEE, fTPM NV RAM is implemented via RPMB secure storage [2]. So any fTPM operation dependent on NV RAM may fail without access to RPMB secure storage.
And during kernel boot, RPMB access isn't available (there were some prior efforts [3] around this but unfortunately didn't land in upstream) due to which we need to rely on user mode tee-supplicant for corresponding access.
So we should only register fTPM device when the underlying infrastructure is working, which is something this patch-set addresses via registering fTPM device only when the tee-supplicant is up and running.
[1] https://lkml.org/lkml/2019/7/4/779 [2] https://optee.readthedocs.io/en/latest/architecture/secure_storage.html#rpmb... [3] https://lwn.net/Articles/682276/
Before these changes user space has to drive boot sequence. I.e. tee and optee modules loads, then application tee-supplicant has to start and only then module ftpm can be loaded. The reason for that is storage services are implemented in userspace and driver needs them.
Is the TPM implementation uploaded to TEE from user space and or what storage are we talking about? Not sure how these storage services connect to the TPM.
Please see my response above.
-Sumit
/Jarkko
Jakko, tee-supplicant application provides state machine over callbacks with RPC messages. https://github.com/OP-TEE/optee_client/blob/master/tee-supplicant/src/tee_su... It also allocates shm. Without running tee-supplicant tee_client_open_session() will fail. optee_open_session()->get_msg_arg()->tee_shm_alloc()->... Optee team wanted to remove some dependencies from tee-supplicant with moving code to the kernel. But for now I think that should be out of the scope of current patches due to they fix driver initialization on tee bus without breaking current functionality.
Maxim.
On Mon, 2020-05-25 at 09:50 +0300, Maxim Uvarov wrote:
Jakko, tee-supplicant application provides state machine over callbacks with RPC messages. https://github.com/OP-TEE/optee_client/blob/master/tee-supplicant/src/tee_su... It also allocates shm. Without running tee-supplicant tee_client_open_session() will fail. optee_open_session()->get_msg_arg()->tee_shm_alloc()->... Optee team wanted to remove some dependencies from tee-supplicant with moving code to the kernel. But for now I think that should be out of the scope of current patches due to they fix driver initialization on tee bus without breaking current functionality.
So what is the role in high-level for tee-supplicant? Why does it exist? No time to dive into code unfortunately.
These kernel commits do not explain in simple terms enough how all of these entities connect with each other, if you don't have that understanding beforehand.
/Jarkko
On Wed, 27 May 2020 at 22:42, Jarkko Sakkinen jarkko.sakkinen@linux.intel.com wrote:
On Mon, 2020-05-25 at 09:50 +0300, Maxim Uvarov wrote:
Jakko, tee-supplicant application provides state machine over callbacks with RPC messages. https://github.com/OP-TEE/optee_client/blob/master/tee-supplicant/src/tee_su... It also allocates shm. Without running tee-supplicant tee_client_open_session() will fail. optee_open_session()->get_msg_arg()->tee_shm_alloc()->... Optee team wanted to remove some dependencies from tee-supplicant with moving code to the kernel. But for now I think that should be out of the scope of current patches due to they fix driver initialization on tee bus without breaking current functionality.
So what is the role in high-level for tee-supplicant? Why does it exist? No time to dive into code unfortunately.
Original implementation for tee-supplicant does several things: 1. allocate shm 2. load ta from user space (fs file) 3. emulate rpmb 4. also there are some ftrace and socket functions which I did not use.
As I I understand, current implementation uses tee-supplicant and it's library as API from user land to Trusted OS.
Some docs can be found here: https://optee.readthedocs.io/en/latest/architecture/index.html
These kernel commits do not explain in simple terms enough how all of these entities connect with each other, if you don't have that understanding beforehand.
Yes, that is true. But I think it's something new and good docs will be some time later.
/Jarkko
Regards, Maxim.
On Thu, May 28, 2020 at 11:08:18AM +0300, Maxim Uvarov wrote:
On Wed, 27 May 2020 at 22:42, Jarkko Sakkinen jarkko.sakkinen@linux.intel.com wrote:
On Mon, 2020-05-25 at 09:50 +0300, Maxim Uvarov wrote:
Jakko, tee-supplicant application provides state machine over callbacks with RPC messages. https://github.com/OP-TEE/optee_client/blob/master/tee-supplicant/src/tee_su... It also allocates shm. Without running tee-supplicant tee_client_open_session() will fail. optee_open_session()->get_msg_arg()->tee_shm_alloc()->... Optee team wanted to remove some dependencies from tee-supplicant with moving code to the kernel. But for now I think that should be out of the scope of current patches due to they fix driver initialization on tee bus without breaking current functionality.
So what is the role in high-level for tee-supplicant? Why does it exist? No time to dive into code unfortunately.
Original implementation for tee-supplicant does several things:
- allocate shm
- load ta from user space (fs file)
- emulate rpmb
- also there are some ftrace and socket functions which I did not use.
As I I understand, current implementation uses tee-supplicant and it's library as API from user land to Trusted OS.
Some docs can be found here: https://optee.readthedocs.io/en/latest/architecture/index.html
These kernel commits do not explain in simple terms enough how all of these entities connect with each other, if you don't have that understanding beforehand.
Yes, that is true. But I think it's something new and good docs will be some time later.
There's already some in Documentation/tee.txt, but it will get outdated if we don't update it when we architectural changes like this. It's a pity we missed updating it with the introduction of the bus. It seems a good time to do it now so it easier to follow what's done.
Cheers, Jens
/Jarkko
Regards, Maxim.
On Thu, 28 May 2020 at 15:41, Jens Wiklander jens.wiklander@linaro.org wrote:
On Thu, May 28, 2020 at 11:08:18AM +0300, Maxim Uvarov wrote:
On Wed, 27 May 2020 at 22:42, Jarkko Sakkinen jarkko.sakkinen@linux.intel.com wrote:
On Mon, 2020-05-25 at 09:50 +0300, Maxim Uvarov wrote:
Jakko, tee-supplicant application provides state machine over callbacks with RPC messages. https://github.com/OP-TEE/optee_client/blob/master/tee-supplicant/src/tee_su... It also allocates shm. Without running tee-supplicant tee_client_open_session() will fail. optee_open_session()->get_msg_arg()->tee_shm_alloc()->... Optee team wanted to remove some dependencies from tee-supplicant with moving code to the kernel. But for now I think that should be out of the scope of current patches due to they fix driver initialization on tee bus without breaking current functionality.
So what is the role in high-level for tee-supplicant? Why does it exist? No time to dive into code unfortunately.
Original implementation for tee-supplicant does several things:
- allocate shm
- load ta from user space (fs file)
- emulate rpmb
- also there are some ftrace and socket functions which I did not use.
As I I understand, current implementation uses tee-supplicant and it's library as API from user land to Trusted OS.
Some docs can be found here: https://optee.readthedocs.io/en/latest/architecture/index.html
These kernel commits do not explain in simple terms enough how all of these entities connect with each other, if you don't have that understanding beforehand.
Yes, that is true. But I think it's something new and good docs will be some time later.
There's already some in Documentation/tee.txt, but it will get outdated if we don't update it when we architectural changes like this. It's a pity we missed updating it with the introduction of the bus. It seems a good time to do it now so it easier to follow what's done.
Agree, let me try to update documentation for TEE bus. Will share it as a separate patch.
-Sumit
Cheers, Jens
/Jarkko
Regards, Maxim.
-
Jarkko Sakkinen -
Jens Wiklander -
Maxim Uvarov -
Sumit Garg