Dear all,
This small series aggregates 2 change proposals related to OP-TEE async notif. I've made a single series since the 2 patches hit the same portions of optee driver source file and I think this will help the review. If you prefer having independent post and deal with the conflicts afterward, please tell me.
Patch "optee: add per cpu asynchronous notification" aims at allowing optee to use a per-cpu interrupt (PPI on Arm CPUs) for async notif instead of a shared peripheral interrupt.
The 2 next patches implement a new feature in OP-TEE, based on optee async notif. The allow optee driver to behave as an interrupt controller, for when a secure OP-TEE event is to be delivered to the Linux kernel as a interrupt event.
Regards, Etienne
Etienne Carriere (3): optee: add per cpu asynchronous notification dt-bindings: arm: optee: add interrupt controller properties optee core: add irq chip using optee async notification
.../arm/firmware/linaro,optee-tz.yaml | 19 +- drivers/tee/optee/optee_private.h | 24 ++ drivers/tee/optee/optee_smc.h | 78 +++++- drivers/tee/optee/smc_abi.c | 249 +++++++++++++++++- 4 files changed, 358 insertions(+), 12 deletions(-)
Implements use of per CPU irq for asynchronous notification next to existing standard irq support. This change allows for example to use GIC_PPI on platforms where no GIC_SPI is provisioned for OP-TEE asynchronous notification.
Co-developed-by: Alexandre Torgue alexandre.torgue@foss.st.com Signed-off-by: Alexandre Torgue alexandre.torgue@foss.st.com Signed-off-by: Etienne Carriere etienne.carriere@linaro.org --- drivers/tee/optee/optee_private.h | 22 ++++++ drivers/tee/optee/smc_abi.c | 107 ++++++++++++++++++++++++++++-- 2 files changed, 124 insertions(+), 5 deletions(-)
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h index 04ae58892608..e5bd3548691f 100644 --- a/drivers/tee/optee/optee_private.h +++ b/drivers/tee/optee/optee_private.h @@ -94,11 +94,33 @@ struct optee_supp { struct completion reqs_c; };
+/* + * struct optee_pcpu - per cpu notif private struct passed to work functions + * @optee optee device reference + */ +struct optee_pcpu { + struct optee *optee; +}; + +/* + * struct optee_smc - optee smc communication struct + * @invoke_fn handler function to invoke secure monitor + * @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 + * @notif_irq interrupt used as async notification by OP-TEE or 0 + * @optee_pcpu per_cpu optee instance for per cpu work or NULL + * @notif_pcpu_wq workqueue for per cpu aynchronous notification or NULL + * @notif_pcpu_work work for per cpu asynchronous notification + */ struct optee_smc { optee_invoke_fn *invoke_fn; void *memremaped_shm; u32 sec_caps; unsigned int notif_irq; + struct optee_pcpu __percpu *optee_pcpu; + struct workqueue_struct *notif_pcpu_wq; + struct work_struct notif_pcpu_work; };
/** diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c index a1c1fa1a9c28..8c2d58d605ac 100644 --- a/drivers/tee/optee/smc_abi.c +++ b/drivers/tee/optee/smc_abi.c @@ -993,12 +993,20 @@ static u32 get_async_notif_value(optee_invoke_fn *invoke_fn, bool *value_valid,
static irqreturn_t notif_irq_handler(int irq, void *dev_id) { - struct optee *optee = dev_id; + struct optee *optee; bool do_bottom_half = false; bool value_valid; bool value_pending; u32 value;
+ if (irq_is_percpu_devid(irq)) { + struct optee_pcpu *pcpu = (struct optee_pcpu *)dev_id; + + optee = pcpu->optee; + } else { + optee = dev_id; + } + do { value = get_async_notif_value(optee->smc.invoke_fn, &value_valid, &value_pending); @@ -1011,8 +1019,13 @@ static irqreturn_t notif_irq_handler(int irq, void *dev_id) optee_notif_send(optee, value); } while (value_pending);
- if (do_bottom_half) - return IRQ_WAKE_THREAD; + if (do_bottom_half) { + if (irq_is_percpu_devid(irq)) + queue_work(optee->smc.notif_pcpu_wq, &optee->smc.notif_pcpu_work); + else + return IRQ_WAKE_THREAD; + } + return IRQ_HANDLED; }
@@ -1025,7 +1038,7 @@ static irqreturn_t notif_irq_thread_fn(int irq, void *dev_id) return IRQ_HANDLED; }
-static int optee_smc_notif_init_irq(struct optee *optee, u_int irq) +static int init_irq(struct optee *optee, u_int irq) { int rc;
@@ -1040,12 +1053,96 @@ static int optee_smc_notif_init_irq(struct optee *optee, u_int irq) return 0; }
+static void notif_pcpu_irq_work_fn(struct work_struct *work) +{ + struct optee_smc *optee_smc = container_of(work, struct optee_smc, notif_pcpu_work); + struct optee *optee = container_of(optee_smc, struct optee, smc); + + optee_smc_do_bottom_half(optee->ctx); +} + +static int init_pcpu_irq(struct optee *optee, u_int irq) +{ + struct optee_pcpu *optee_pcpu; + spinlock_t lock; + int cpu; + int rc; + + optee_pcpu = alloc_percpu(struct optee_pcpu); + if (!optee_pcpu) + return -ENOMEM; + + for_each_present_cpu(cpu) { + struct optee_pcpu *p = per_cpu_ptr(optee_pcpu, cpu); + + p->optee = optee; + } + + rc = request_percpu_irq(irq, notif_irq_handler, + "optee_pcpu_notification", optee_pcpu); + if (rc) + goto err_free_pcpu; + + spin_lock_init(&lock); + + spin_lock(&lock); + enable_percpu_irq(irq, 0); + spin_unlock(&lock); + + INIT_WORK(&optee->smc.notif_pcpu_work, notif_pcpu_irq_work_fn); + optee->smc.notif_pcpu_wq = create_workqueue("optee_pcpu_notification"); + if (!optee->smc.notif_pcpu_wq) { + rc = -EINVAL; + goto err_free_pcpu_irq; + } + + optee->smc.optee_pcpu = optee_pcpu; + optee->smc.notif_irq = irq; + + return 0; + +err_free_pcpu_irq: + spin_lock(&lock); + disable_percpu_irq(irq); + spin_unlock(&lock); + free_percpu_irq(irq, optee_pcpu); +err_free_pcpu: + free_percpu(optee_pcpu); + + return rc; +} + +static int optee_smc_notif_init_irq(struct optee *optee, u_int irq) +{ + if (irq_is_percpu_devid(irq)) + return init_pcpu_irq(optee, irq); + else + return init_irq(optee, irq); +} + +static void uninit_pcpu_irq(struct optee *optee) +{ + spinlock_t lock; + + spin_lock_init(&lock); + spin_lock(&lock); + disable_percpu_irq(optee->smc.notif_irq); + spin_unlock(&lock); + + free_percpu_irq(optee->smc.notif_irq, optee->smc.optee_pcpu); + free_percpu(optee->smc.optee_pcpu); +} + static void optee_smc_notif_uninit_irq(struct optee *optee) { if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_ASYNC_NOTIF) { optee_smc_stop_async_notif(optee->ctx); if (optee->smc.notif_irq) { - free_irq(optee->smc.notif_irq, optee); + if (irq_is_percpu_devid(optee->smc.notif_irq)) + uninit_pcpu_irq(optee); + else + free_irq(optee->smc.notif_irq, optee); + irq_dispose_mapping(optee->smc.notif_irq); } }
Adds optional interrupt controller properties used when OP-TEE generates interrupt events optee driver shall notified to its registered interrupt consumer. The example shows how OP-TEE can trigger a wakeup interrupt event consumed by a gpio-keys compatible device.
Signed-off-by: Etienne Carriere etienne.carriere@linaro.org --- .../arm/firmware/linaro,optee-tz.yaml | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml index d4dc0749f9fd..42874ca21b7e 100644 --- a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml +++ b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml @@ -40,6 +40,11 @@ properties: HVC #0, register assignments register assignments are specified in drivers/tee/optee/optee_smc.h
+ interrupt-controller: true + + "#interrupt-cells": + const: 1 + required: - compatible - method @@ -48,12 +53,24 @@ additionalProperties: false
examples: - | + #include <dt-bindings/input/input.h> #include <dt-bindings/interrupt-controller/arm-gic.h> firmware { - optee { + optee: optee { compatible = "linaro,optee-tz"; method = "smc"; interrupts = <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>; + interrupt-controller; + #interrupt-cells = <1>; + }; + }; + + wake_up { + compatible = "gpio-keys"; + + button { + linux,code = <KEY_WAKEUP>; + interrupts-extended = <&optee 0>; }; };
Adds an irq chip in optee driver to generate interrupts from OP-TEE notified interrupt events based on optee async notification. Upon such notification, optee driver invokes OP-TEE to query a pending interrupt event. If an interrupt notification is pending the invocation return OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_IT and optee driver can get the pending interrupt number with SMC function ID OPTEE_SMC_FUNCID_GET_IT_VALUE.
SMC function ID OPTEE_SMC_FUNCID_SET_IT_MASK allows Linux to mask/unmask an interrupt notification services.
The optee irq_chip if flagged IRQCHIP_SKIP_SET_WAKE to skip set_wake as optee interrupt notifications doesn't support the set_wake option. In case a device is using the optee irq and is marked as wakeup source, this result in an "Unbalanced IRQ xx wake disable" backtrace, since: - in irq_set_irq_wake(ON), wake_depth gets incremented, then reset due to set_irq_wake_real() returns an error (irq_set_wake() isn't implemented) - in irq_set_irq_wake(OFF), wake_depth is always 0, hence the warning
Co-developed-by: Pascal Paillet p.paillet@foss.st.com Signed-off-by: Pascal Paillet p.paillet@foss.st.com Co-developed-by: Fabrice Gasnier fabrice.gasnier@foss.st.com Signed-off-by: Fabrice Gasnier fabrice.gasnier@foss.st.com Signed-off-by: Etienne Carriere etienne.carriere@linaro.org --- drivers/tee/optee/optee_private.h | 2 + drivers/tee/optee/optee_smc.h | 78 +++++++++++++++- drivers/tee/optee/smc_abi.c | 142 ++++++++++++++++++++++++++++-- 3 files changed, 216 insertions(+), 6 deletions(-)
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h index e5bd3548691f..2a146d884d27 100644 --- a/drivers/tee/optee/optee_private.h +++ b/drivers/tee/optee/optee_private.h @@ -112,6 +112,7 @@ struct optee_pcpu { * @optee_pcpu per_cpu optee instance for per cpu work or NULL * @notif_pcpu_wq workqueue for per cpu aynchronous notification or NULL * @notif_pcpu_work work for per cpu asynchronous notification + * @domain interrupt domain registered by OP-TEE driver */ struct optee_smc { optee_invoke_fn *invoke_fn; @@ -121,6 +122,7 @@ struct optee_smc { struct optee_pcpu __percpu *optee_pcpu; struct workqueue_struct *notif_pcpu_wq; struct work_struct notif_pcpu_work; + struct irq_domain *domain; };
/** diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h index 73b5e7760d10..0cf83d5a2931 100644 --- a/drivers/tee/optee/optee_smc.h +++ b/drivers/tee/optee/optee_smc.h @@ -226,7 +226,8 @@ struct optee_smc_get_shm_config_result { * a3 Bit[7:0]: Number of parameters needed for RPC to be supplied * as the second MSG arg struct for * OPTEE_SMC_CALL_WITH_ARG - * Bit[31:8]: Reserved (MBZ) + * Bit[23:8]: The maximum interrupt event notification number + * Bit[31:24]: Reserved (MBZ) * a4-7 Preserved * * Error return register usage: @@ -254,6 +255,11 @@ struct optee_smc_get_shm_config_result { #define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF BIT(5) /* Secure world supports pre-allocating RPC arg struct */ #define OPTEE_SMC_SEC_CAP_RPC_ARG BIT(6) +/* Secure world supports interrupt events notification to normal world */ +#define OPTEE_SMC_SEC_CAP_IT_NOTIF BIT(7) + +#define OPTEE_SMC_SEC_CAP_MAX_NOTIF_IT_MASK GENMASK(23, 8) +#define OPTEE_SMC_SEC_CAP_MAX_NOTIF_IT_SHIFT 8
#define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES 9 #define OPTEE_SMC_EXCHANGE_CAPABILITIES \ @@ -416,6 +422,12 @@ struct optee_smc_disable_shm_cache_result { */ #define OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF 0
+/* + * Notification that OP-TEE triggers an interrupt event to Linux kernel + * for an interrupt consumer. + */ +#define OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_IT 1 + #define OPTEE_SMC_FUNCID_GET_ASYNC_NOTIF_VALUE 17 #define OPTEE_SMC_GET_ASYNC_NOTIF_VALUE \ OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_ASYNC_NOTIF_VALUE) @@ -426,6 +438,70 @@ struct optee_smc_disable_shm_cache_result { /* See OPTEE_SMC_CALL_WITH_REGD_ARG above */ #define OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG 19
+/* + * Retrieve the interrupt number of the pending interrupt event notified to + * non-secure world since the last call of this function. + * + * OP-TEE keeps a record of all posted interrupt notification events. When the + * async notif interrupt is received by non-secure world, this function should + * be called until all pended interrupt events have been retrieved. When an + * interrupt event is retrieved it is cleared from the record in secure world. + * + * It is expected that this function is called from an interrupt handler + * in normal world. + * + * Call requests usage: + * a0 SMC Function ID, OPTEE_SMC_GET_IT_NOTIF_VALUE + * a1-6 Not used + * a7 Hypervisor Client ID register + * + * Normal return register usage: + * a0 OPTEE_SMC_RETURN_OK + * a1 IT_NOTIF interrupt identifier value + * a2 Bit[0]: OPTEE_SMC_IT_NOTIF_VALID if the value in a1 is + * valid, else 0 if no interrupt event were pending + * a2 Bit[1]: OPTEE_SMC_IT_NOTIF_PENDING if another interrupt event + * value is pending, else 0. + * Bit[31:2]: MBZ + * a3-7 Preserved + * + * Not supported return register usage: + * a0 OPTEE_SMC_RETURN_ENOTAVAIL + * a1-7 Preserved + */ +#define OPTEE_SMC_IT_NOTIF_VALID BIT(0) +#define OPTEE_SMC_IT_NOTIF_PENDING BIT(1) + +#define OPTEE_SMC_FUNCID_GET_IT_NOTIF_VALUE 20 +#define OPTEE_SMC_GET_IT_NOTIF_VALUE \ + OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_IT_NOTIF_VALUE) + +/* + * Mask or unmask an interrupt notification event. + * + * It is expected that this function is called from an interrupt handler + * in normal world. + * + * Call requests usage: + * a0 SMC Function ID, OPTEE_SMC_SET_IT_NOTIF_MASK + * a1 Interrupt identifier value + * a2 Bit[0]: 1 if interrupt event is to be masked, 0 if it is to be unmasked + * a2 Bit[31:1] MBZ + * a3-6 Not used + * a7 Hypervisor Client ID register + * + * Normal return register usage: + * a0 OPTEE_SMC_RETURN_OK + * a1-7 Preserved + * + * Not supported return register usage: + * a0 OPTEE_SMC_RETURN_ENOTAVAIL + * a1-7 Preserved + */ +#define OPTEE_SMC_FUNCID_SET_IT_NOTIF_MASK 21 +#define OPTEE_SMC_SET_IT_NOTIF_MASK \ + OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_SET_IT_NOTIF_MASK) + /* * Resume from RPC (for example after processing a foreign interrupt) * diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c index 8c2d58d605ac..0360afde119f 100644 --- a/drivers/tee/optee/smc_abi.c +++ b/drivers/tee/optee/smc_abi.c @@ -977,6 +977,112 @@ static int optee_smc_stop_async_notif(struct tee_context *ctx) * 5. Asynchronous notification */
+static u32 get_it_value(optee_invoke_fn *invoke_fn, bool *value_valid, + bool *value_pending) +{ + struct arm_smccc_res res; + + invoke_fn(OPTEE_SMC_GET_IT_NOTIF_VALUE, 0, 0, 0, 0, 0, 0, 0, &res); + + if (res.a0) + return 0; + + *value_valid = res.a2 & OPTEE_SMC_IT_NOTIF_VALID; + *value_pending = res.a2 & OPTEE_SMC_IT_NOTIF_PENDING; + return res.a1; +} + +static u32 set_it_mask(optee_invoke_fn *invoke_fn, u32 it_value, bool mask) +{ + struct arm_smccc_res res; + + invoke_fn(OPTEE_SMC_SET_IT_NOTIF_MASK, it_value, mask, 0, 0, 0, 0, 0, &res); + + if (res.a0) + return 0; + + return res.a1; +} + +static int handle_optee_it(struct optee *optee) +{ + bool value_valid; + bool value_pending; + u32 it; + + do { + struct irq_desc *desc; + + it = get_it_value(optee->smc.invoke_fn, &value_valid, &value_pending); + if (!value_valid) + break; + + desc = irq_to_desc(irq_find_mapping(optee->smc.domain, it)); + if (!desc) { + pr_err("no desc for optee IT:%d\n", it); + return -EIO; + } + + handle_simple_irq(desc); + + } while (value_pending); + + return 0; +} + +static void optee_it_irq_mask(struct irq_data *d) +{ + struct optee *optee = d->domain->host_data; + + set_it_mask(optee->smc.invoke_fn, d->hwirq, true); +} + +static void optee_it_irq_unmask(struct irq_data *d) +{ + struct optee *optee = d->domain->host_data; + + set_it_mask(optee->smc.invoke_fn, d->hwirq, false); +} + +static struct irq_chip optee_it_irq_chip = { + .name = "optee-it", + .irq_disable = optee_it_irq_mask, + .irq_enable = optee_it_irq_unmask, + .flags = IRQCHIP_SKIP_SET_WAKE, +}; + +static int optee_it_alloc(struct irq_domain *d, unsigned int virq, + unsigned int nr_irqs, void *data) +{ + struct irq_fwspec *fwspec = data; + irq_hw_number_t hwirq; + + hwirq = fwspec->param[0]; + + irq_domain_set_hwirq_and_chip(d, virq, hwirq, &optee_it_irq_chip, d->host_data); + + return 0; +} + +static const struct irq_domain_ops optee_it_irq_domain_ops = { + .alloc = optee_it_alloc, + .free = irq_domain_free_irqs_common, +}; + +static int optee_irq_domain_init(struct platform_device *pdev, struct optee *optee, u_int max_it) +{ + struct device *dev = &pdev->dev; + struct device_node *np = dev->of_node; + + optee->smc.domain = irq_domain_add_linear(np, max_it, &optee_it_irq_domain_ops, optee); + if (!optee->smc.domain) { + dev_err(dev, "Unable to add irq domain\n"); + return -ENOMEM; + } + + return 0; +} + static u32 get_async_notif_value(optee_invoke_fn *invoke_fn, bool *value_valid, bool *value_pending) { @@ -1008,13 +1114,15 @@ static irqreturn_t notif_irq_handler(int irq, void *dev_id) }
do { - value = get_async_notif_value(optee->smc.invoke_fn, - &value_valid, &value_pending); + value = get_async_notif_value(optee->smc.invoke_fn, &value_valid, &value_pending); if (!value_valid) break;
if (value == OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF) do_bottom_half = true; + else if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_IT_NOTIF && + value == OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_IT) + handle_optee_it(optee); else optee_notif_send(optee, value); } while (value_pending); @@ -1042,8 +1150,7 @@ static int init_irq(struct optee *optee, u_int irq) { int rc;
- rc = request_threaded_irq(irq, notif_irq_handler, - notif_irq_thread_fn, + rc = request_threaded_irq(irq, notif_irq_handler, notif_irq_thread_fn, 0, "optee_notification", optee); if (rc) return rc; @@ -1145,6 +1252,9 @@ static void optee_smc_notif_uninit_irq(struct optee *optee)
irq_dispose_mapping(optee->smc.notif_irq); } + + if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_IT_NOTIF) + irq_domain_remove(optee->smc.domain); } }
@@ -1284,6 +1394,7 @@ static bool optee_msg_api_revision_is_compatible(optee_invoke_fn *invoke_fn)
static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn, u32 *sec_caps, u32 *max_notif_value, + u32 *max_notif_it, unsigned int *rpc_param_count) { union { @@ -1316,6 +1427,12 @@ static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn, else *rpc_param_count = 0;
+ if (*sec_caps & OPTEE_SMC_SEC_CAP_IT_NOTIF) + *max_notif_it = (res.result.data & OPTEE_SMC_SEC_CAP_MAX_NOTIF_IT_MASK) >> + OPTEE_SMC_SEC_CAP_MAX_NOTIF_IT_SHIFT; + else + *max_notif_it = 0; + return true; }
@@ -1461,6 +1578,7 @@ static int optee_probe(struct platform_device *pdev) struct tee_device *teedev; struct tee_context *ctx; u32 max_notif_value; + u32 max_notif_it; u32 arg_cache_flags; u32 sec_caps; int rc; @@ -1482,7 +1600,7 @@ static int optee_probe(struct platform_device *pdev) }
if (!optee_msg_exchange_capabilities(invoke_fn, &sec_caps, - &max_notif_value, + &max_notif_value, &max_notif_it, &rpc_param_count)) { pr_warn("capabilities mismatch\n"); return -EINVAL; @@ -1603,6 +1721,20 @@ static int optee_probe(struct platform_device *pdev) irq_dispose_mapping(irq); goto err_notif_uninit; } + + if (sec_caps & OPTEE_SMC_SEC_CAP_IT_NOTIF) { + rc = optee_irq_domain_init(pdev, optee, max_notif_it); + if (rc) { + if (irq_is_percpu_devid(optee->smc.notif_irq)) + uninit_pcpu_irq(optee); + else + free_irq(optee->smc.notif_irq, optee); + + irq_dispose_mapping(irq); + goto err_notif_uninit; + } + } + enable_async_notif(optee->smc.invoke_fn); pr_info("Asynchronous notifications enabled\n"); }
On Thu, 12 Jan 2023 14:54:24 +0000, Etienne Carriere etienne.carriere@linaro.org wrote:
Adds an irq chip in optee driver to generate interrupts from OP-TEE notified interrupt events based on optee async notification. Upon such notification, optee driver invokes OP-TEE to query a pending interrupt event. If an interrupt notification is pending the invocation return OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_IT and optee driver can get the pending interrupt number with SMC function ID OPTEE_SMC_FUNCID_GET_IT_VALUE.
SMC function ID OPTEE_SMC_FUNCID_SET_IT_MASK allows Linux to mask/unmask an interrupt notification services.
The optee irq_chip if flagged IRQCHIP_SKIP_SET_WAKE to skip set_wake as optee interrupt notifications doesn't support the set_wake option. In case a device is using the optee irq and is marked as wakeup source, this result in an "Unbalanced IRQ xx wake disable" backtrace, since:
- in irq_set_irq_wake(ON), wake_depth gets incremented, then reset due to set_irq_wake_real() returns an error (irq_set_wake() isn't implemented)
- in irq_set_irq_wake(OFF), wake_depth is always 0, hence the warning
Is this relevant information?
Co-developed-by: Pascal Paillet p.paillet@foss.st.com Signed-off-by: Pascal Paillet p.paillet@foss.st.com Co-developed-by: Fabrice Gasnier fabrice.gasnier@foss.st.com Signed-off-by: Fabrice Gasnier fabrice.gasnier@foss.st.com Signed-off-by: Etienne Carriere etienne.carriere@linaro.org
drivers/tee/optee/optee_private.h | 2 + drivers/tee/optee/optee_smc.h | 78 +++++++++++++++- drivers/tee/optee/smc_abi.c | 142 ++++++++++++++++++++++++++++-- 3 files changed, 216 insertions(+), 6 deletions(-)
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h index e5bd3548691f..2a146d884d27 100644 --- a/drivers/tee/optee/optee_private.h +++ b/drivers/tee/optee/optee_private.h @@ -112,6 +112,7 @@ struct optee_pcpu {
- @optee_pcpu per_cpu optee instance for per cpu work or NULL
- @notif_pcpu_wq workqueue for per cpu aynchronous notification or NULL
- @notif_pcpu_work work for per cpu asynchronous notification
*/
- @domain interrupt domain registered by OP-TEE driver
struct optee_smc { optee_invoke_fn *invoke_fn; @@ -121,6 +122,7 @@ struct optee_smc { struct optee_pcpu __percpu *optee_pcpu; struct workqueue_struct *notif_pcpu_wq; struct work_struct notif_pcpu_work;
- struct irq_domain *domain;
}; /** diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h index 73b5e7760d10..0cf83d5a2931 100644 --- a/drivers/tee/optee/optee_smc.h +++ b/drivers/tee/optee/optee_smc.h @@ -226,7 +226,8 @@ struct optee_smc_get_shm_config_result {
- a3 Bit[7:0]: Number of parameters needed for RPC to be supplied
as the second MSG arg struct for
OPTEE_SMC_CALL_WITH_ARG
- Bit[31:8]: Reserved (MBZ)
- Bit[23:8]: The maximum interrupt event notification number
- Bit[31:24]: Reserved (MBZ)
- a4-7 Preserved
- Error return register usage:
@@ -254,6 +255,11 @@ struct optee_smc_get_shm_config_result { #define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF BIT(5) /* Secure world supports pre-allocating RPC arg struct */ #define OPTEE_SMC_SEC_CAP_RPC_ARG BIT(6) +/* Secure world supports interrupt events notification to normal world */ +#define OPTEE_SMC_SEC_CAP_IT_NOTIF BIT(7)
+#define OPTEE_SMC_SEC_CAP_MAX_NOTIF_IT_MASK GENMASK(23, 8) +#define OPTEE_SMC_SEC_CAP_MAX_NOTIF_IT_SHIFT 8 #define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES 9 #define OPTEE_SMC_EXCHANGE_CAPABILITIES \ @@ -416,6 +422,12 @@ struct optee_smc_disable_shm_cache_result { */ #define OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF 0 +/*
- Notification that OP-TEE triggers an interrupt event to Linux kernel
- for an interrupt consumer.
- */
+#define OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_IT 1
#define OPTEE_SMC_FUNCID_GET_ASYNC_NOTIF_VALUE 17 #define OPTEE_SMC_GET_ASYNC_NOTIF_VALUE \ OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_ASYNC_NOTIF_VALUE) @@ -426,6 +438,70 @@ struct optee_smc_disable_shm_cache_result { /* See OPTEE_SMC_CALL_WITH_REGD_ARG above */ #define OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG 19 +/*
- Retrieve the interrupt number of the pending interrupt event notified to
- non-secure world since the last call of this function.
- OP-TEE keeps a record of all posted interrupt notification events. When the
- async notif interrupt is received by non-secure world, this function should
- be called until all pended interrupt events have been retrieved. When an
- interrupt event is retrieved it is cleared from the record in secure world.
- It is expected that this function is called from an interrupt handler
- in normal world.
- Call requests usage:
- a0 SMC Function ID, OPTEE_SMC_GET_IT_NOTIF_VALUE
- a1-6 Not used
- a7 Hypervisor Client ID register
- Normal return register usage:
- a0 OPTEE_SMC_RETURN_OK
- a1 IT_NOTIF interrupt identifier value
- a2 Bit[0]: OPTEE_SMC_IT_NOTIF_VALID if the value in a1 is
valid, else 0 if no interrupt event were pending
- a2 Bit[1]: OPTEE_SMC_IT_NOTIF_PENDING if another interrupt event
value is pending, else 0.
- Bit[31:2]: MBZ
- a3-7 Preserved
- Not supported return register usage:
- a0 OPTEE_SMC_RETURN_ENOTAVAIL
- a1-7 Preserved
- */
+#define OPTEE_SMC_IT_NOTIF_VALID BIT(0) +#define OPTEE_SMC_IT_NOTIF_PENDING BIT(1)
+#define OPTEE_SMC_FUNCID_GET_IT_NOTIF_VALUE 20 +#define OPTEE_SMC_GET_IT_NOTIF_VALUE \
- OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_IT_NOTIF_VALUE)
+/*
- Mask or unmask an interrupt notification event.
- It is expected that this function is called from an interrupt handler
- in normal world.
- Call requests usage:
- a0 SMC Function ID, OPTEE_SMC_SET_IT_NOTIF_MASK
- a1 Interrupt identifier value
- a2 Bit[0]: 1 if interrupt event is to be masked, 0 if it is to be unmasked
- a2 Bit[31:1] MBZ
- a3-6 Not used
- a7 Hypervisor Client ID register
- Normal return register usage:
- a0 OPTEE_SMC_RETURN_OK
- a1-7 Preserved
- Not supported return register usage:
- a0 OPTEE_SMC_RETURN_ENOTAVAIL
- a1-7 Preserved
- */
+#define OPTEE_SMC_FUNCID_SET_IT_NOTIF_MASK 21 +#define OPTEE_SMC_SET_IT_NOTIF_MASK \
- OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_SET_IT_NOTIF_MASK)
/*
- Resume from RPC (for example after processing a foreign interrupt)
diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c index 8c2d58d605ac..0360afde119f 100644 --- a/drivers/tee/optee/smc_abi.c +++ b/drivers/tee/optee/smc_abi.c @@ -977,6 +977,112 @@ static int optee_smc_stop_async_notif(struct tee_context *ctx)
- Asynchronous notification
*/ +static u32 get_it_value(optee_invoke_fn *invoke_fn, bool *value_valid,
bool *value_pending)
What value? If this is supposed to return a set of pending bits, just name the function to reflect that.
Also, at no point do you explain that each PPI is only a mux interrupt for a bunch of chained interrupts.
+{
- struct arm_smccc_res res;
- invoke_fn(OPTEE_SMC_GET_IT_NOTIF_VALUE, 0, 0, 0, 0, 0, 0, 0, &res);
- if (res.a0)
return 0;
- *value_valid = res.a2 & OPTEE_SMC_IT_NOTIF_VALID;
- *value_pending = res.a2 & OPTEE_SMC_IT_NOTIF_PENDING;
- return res.a1;
+}
+static u32 set_it_mask(optee_invoke_fn *invoke_fn, u32 it_value, bool mask) +{
- struct arm_smccc_res res;
- invoke_fn(OPTEE_SMC_SET_IT_NOTIF_MASK, it_value, mask, 0, 0, 0, 0, 0, &res);
- if (res.a0)
return 0;
- return res.a1;
+}
+static int handle_optee_it(struct optee *optee) +{
- bool value_valid;
- bool value_pending;
- u32 it;
- do {
struct irq_desc *desc;
it = get_it_value(optee->smc.invoke_fn, &value_valid, &value_pending);
if (!value_valid)
break;
desc = irq_to_desc(irq_find_mapping(optee->smc.domain, it));
if (!desc) {
pr_err("no desc for optee IT:%d\n", it);
return -EIO;
}
handle_simple_irq(desc);
What is this? Please use generic_handle_domain_irq(), like any other driver. Why is the flow handler handle_simple_irq()? You need to explain what the signalling is for the secure-provided interrupts.
- } while (value_pending);
- return 0;
+}
+static void optee_it_irq_mask(struct irq_data *d) +{
- struct optee *optee = d->domain->host_data;
- set_it_mask(optee->smc.invoke_fn, d->hwirq, true);
+}
+static void optee_it_irq_unmask(struct irq_data *d) +{
- struct optee *optee = d->domain->host_data;
- set_it_mask(optee->smc.invoke_fn, d->hwirq, false);
+}
+static struct irq_chip optee_it_irq_chip = {
- .name = "optee-it",
- .irq_disable = optee_it_irq_mask,
- .irq_enable = optee_it_irq_unmask,
- .flags = IRQCHIP_SKIP_SET_WAKE,
Is it a mask or a disable? These are different beasts.
+};
+static int optee_it_alloc(struct irq_domain *d, unsigned int virq,
unsigned int nr_irqs, void *data)
+{
- struct irq_fwspec *fwspec = data;
- irq_hw_number_t hwirq;
- hwirq = fwspec->param[0];
- irq_domain_set_hwirq_and_chip(d, virq, hwirq, &optee_it_irq_chip, d->host_data);
- return 0;
+}
+static const struct irq_domain_ops optee_it_irq_domain_ops = {
- .alloc = optee_it_alloc,
- .free = irq_domain_free_irqs_common,
+};
+static int optee_irq_domain_init(struct platform_device *pdev, struct optee *optee, u_int max_it) +{
- struct device *dev = &pdev->dev;
- struct device_node *np = dev->of_node;
- optee->smc.domain = irq_domain_add_linear(np, max_it, &optee_it_irq_domain_ops, optee);
- if (!optee->smc.domain) {
dev_err(dev, "Unable to add irq domain\n");
return -ENOMEM;
- }
- return 0;
+}
static u32 get_async_notif_value(optee_invoke_fn *invoke_fn, bool *value_valid, bool *value_pending) { @@ -1008,13 +1114,15 @@ static irqreturn_t notif_irq_handler(int irq, void *dev_id) } do {
value = get_async_notif_value(optee->smc.invoke_fn,
&value_valid, &value_pending);
if (!value_valid) break;value = get_async_notif_value(optee->smc.invoke_fn, &value_valid, &value_pending);
if (value == OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF) do_bottom_half = true;
else if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_IT_NOTIF &&
value == OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_IT)
handle_optee_it(optee);
NAK. This isn't how we deal with chained interrupts. Definitely not in an interrupt handler.
M.
Hello Marc,
Thanks for the review.
On Fri, 13 Jan 2023 at 10:22, Marc Zyngier maz@kernel.org wrote:
On Thu, 12 Jan 2023 14:54:24 +0000, Etienne Carriere etienne.carriere@linaro.org wrote:
Adds an irq chip in optee driver to generate interrupts from OP-TEE notified interrupt events based on optee async notification. Upon such notification, optee driver invokes OP-TEE to query a pending interrupt event. If an interrupt notification is pending the invocation return OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_IT and optee driver can get the pending interrupt number with SMC function ID OPTEE_SMC_FUNCID_GET_IT_VALUE.
SMC function ID OPTEE_SMC_FUNCID_SET_IT_MASK allows Linux to mask/unmask an interrupt notification services.
The optee irq_chip if flagged IRQCHIP_SKIP_SET_WAKE to skip set_wake as optee interrupt notifications doesn't support the set_wake option. In case a device is using the optee irq and is marked as wakeup source, this result in an "Unbalanced IRQ xx wake disable" backtrace, since:
- in irq_set_irq_wake(ON), wake_depth gets incremented, then reset due to set_irq_wake_real() returns an error (irq_set_wake() isn't implemented)
- in irq_set_irq_wake(OFF), wake_depth is always 0, hence the warning
Is this relevant information?
The description is maybe too specific to the setup used for this feature. I'll rephrase that, unless IRQCHIP_SKIP_SET_WAKE flag is not relevant here.
Co-developed-by: Pascal Paillet p.paillet@foss.st.com Signed-off-by: Pascal Paillet p.paillet@foss.st.com Co-developed-by: Fabrice Gasnier fabrice.gasnier@foss.st.com Signed-off-by: Fabrice Gasnier fabrice.gasnier@foss.st.com Signed-off-by: Etienne Carriere etienne.carriere@linaro.org
drivers/tee/optee/optee_private.h | 2 + drivers/tee/optee/optee_smc.h | 78 +++++++++++++++- drivers/tee/optee/smc_abi.c | 142 ++++++++++++++++++++++++++++-- 3 files changed, 216 insertions(+), 6 deletions(-)
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h index e5bd3548691f..2a146d884d27 100644 --- a/drivers/tee/optee/optee_private.h +++ b/drivers/tee/optee/optee_private.h @@ -112,6 +112,7 @@ struct optee_pcpu {
- @optee_pcpu per_cpu optee instance for per cpu work or NULL
- @notif_pcpu_wq workqueue for per cpu aynchronous notification or NULL
- @notif_pcpu_work work for per cpu asynchronous notification
*/
- @domain interrupt domain registered by OP-TEE driver
struct optee_smc { optee_invoke_fn *invoke_fn; @@ -121,6 +122,7 @@ struct optee_smc { struct optee_pcpu __percpu *optee_pcpu; struct workqueue_struct *notif_pcpu_wq; struct work_struct notif_pcpu_work;
struct irq_domain *domain;
};
/** diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h index 73b5e7760d10..0cf83d5a2931 100644 --- a/drivers/tee/optee/optee_smc.h +++ b/drivers/tee/optee/optee_smc.h @@ -226,7 +226,8 @@ struct optee_smc_get_shm_config_result {
- a3 Bit[7:0]: Number of parameters needed for RPC to be supplied
as the second MSG arg struct for
OPTEE_SMC_CALL_WITH_ARG
- Bit[31:8]: Reserved (MBZ)
- Bit[23:8]: The maximum interrupt event notification number
- Bit[31:24]: Reserved (MBZ)
- a4-7 Preserved
- Error return register usage:
@@ -254,6 +255,11 @@ struct optee_smc_get_shm_config_result { #define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF BIT(5) /* Secure world supports pre-allocating RPC arg struct */ #define OPTEE_SMC_SEC_CAP_RPC_ARG BIT(6) +/* Secure world supports interrupt events notification to normal world */ +#define OPTEE_SMC_SEC_CAP_IT_NOTIF BIT(7)
+#define OPTEE_SMC_SEC_CAP_MAX_NOTIF_IT_MASK GENMASK(23, 8) +#define OPTEE_SMC_SEC_CAP_MAX_NOTIF_IT_SHIFT 8
#define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES 9 #define OPTEE_SMC_EXCHANGE_CAPABILITIES \ @@ -416,6 +422,12 @@ struct optee_smc_disable_shm_cache_result { */ #define OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF 0
+/*
- Notification that OP-TEE triggers an interrupt event to Linux kernel
- for an interrupt consumer.
- */
+#define OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_IT 1
#define OPTEE_SMC_FUNCID_GET_ASYNC_NOTIF_VALUE 17 #define OPTEE_SMC_GET_ASYNC_NOTIF_VALUE \ OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_ASYNC_NOTIF_VALUE) @@ -426,6 +438,70 @@ struct optee_smc_disable_shm_cache_result { /* See OPTEE_SMC_CALL_WITH_REGD_ARG above */ #define OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG 19
+/*
- Retrieve the interrupt number of the pending interrupt event notified to
- non-secure world since the last call of this function.
- OP-TEE keeps a record of all posted interrupt notification events. When the
- async notif interrupt is received by non-secure world, this function should
- be called until all pended interrupt events have been retrieved. When an
- interrupt event is retrieved it is cleared from the record in secure world.
- It is expected that this function is called from an interrupt handler
- in normal world.
- Call requests usage:
- a0 SMC Function ID, OPTEE_SMC_GET_IT_NOTIF_VALUE
- a1-6 Not used
- a7 Hypervisor Client ID register
- Normal return register usage:
- a0 OPTEE_SMC_RETURN_OK
- a1 IT_NOTIF interrupt identifier value
- a2 Bit[0]: OPTEE_SMC_IT_NOTIF_VALID if the value in a1 is
valid, else 0 if no interrupt event were pending
- a2 Bit[1]: OPTEE_SMC_IT_NOTIF_PENDING if another interrupt event
value is pending, else 0.
- Bit[31:2]: MBZ
- a3-7 Preserved
- Not supported return register usage:
- a0 OPTEE_SMC_RETURN_ENOTAVAIL
- a1-7 Preserved
- */
+#define OPTEE_SMC_IT_NOTIF_VALID BIT(0) +#define OPTEE_SMC_IT_NOTIF_PENDING BIT(1)
+#define OPTEE_SMC_FUNCID_GET_IT_NOTIF_VALUE 20 +#define OPTEE_SMC_GET_IT_NOTIF_VALUE \
OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_IT_NOTIF_VALUE)
+/*
- Mask or unmask an interrupt notification event.
- It is expected that this function is called from an interrupt handler
- in normal world.
- Call requests usage:
- a0 SMC Function ID, OPTEE_SMC_SET_IT_NOTIF_MASK
- a1 Interrupt identifier value
- a2 Bit[0]: 1 if interrupt event is to be masked, 0 if it is to be unmasked
- a2 Bit[31:1] MBZ
- a3-6 Not used
- a7 Hypervisor Client ID register
- Normal return register usage:
- a0 OPTEE_SMC_RETURN_OK
- a1-7 Preserved
- Not supdealed ported return register usage:
- a0 OPTEE_SMC_RETURN_ENOTAVAIL
- a1-7 Preserved
- */
+#define OPTEE_SMC_FUNCID_SET_IT_NOTIF_MASK 21 +#define OPTEE_SMC_SET_IT_NOTIF_MASK \
OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_SET_IT_NOTIF_MASK)
/*
- Resume from RPC (for example after processing a foreign interrupt)
diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c index 8c2d58d605ac..0360afde119f 100644 --- a/drivers/tee/optee/smc_abi.c +++ b/drivers/tee/optee/smc_abi.c @@ -977,6 +977,112 @@ static int optee_smc_stop_async_notif(struct tee_context *ctx)
- Asynchronous notification
*/
+static u32 get_it_value(optee_invoke_fn *invoke_fn, bool *value_valid,
bool *value_pending)
What value? If this is supposed to return a set of pending bits, just name the function to reflect that.
Communication between Linux kernel and OP-TEE is this case is rather basic here, no shared memory used, only few CPU registers can be used to shared information. So Linux invokes OP-TEE for each pending optee interrupt event: each invocation returns a interrupt event number (an integer value) and a status whether another interrupt event is pending and shall be retrieved invoking OP-TEE again. In case Linux invoke OP-TEE for a pending interrupt and none is pending, a last status is also provided: 'value_valid'.
I could maybe change the prototype to something like: static u32 get_pending_it_number(optee_invoke_fn *invoke_fn, bool *it_number_valid, bool *more_pending_it)
Also, at no point do you explain that each PPI is only a mux interrupt for a bunch of chained interrupts.
Sorry, I don't understand your question. The "it notif" feature proposed in this change is not straight related a PPI. Previous patch in this series is indeed related to PPI: it propose optee driver can use a single PPI instead of a signle SPI for the OP-TEE "asyn notification" feature. This change allows to mux interrupts events (from OP-TEE to Linux optee driver) over "OP-TEE async notif" means, which is using a single SPI or PPI. Maybe I should have submitted both changes separately :(
+{
struct arm_smccc_res res;
invoke_fn(OPTEE_SMC_GET_IT_NOTIF_VALUE, 0, 0, 0, 0, 0, 0, 0, &res);
if (res.a0)
return 0;
*value_valid = res.a2 & OPTEE_SMC_IT_NOTIF_VALID;
*value_pending = res.a2 & OPTEE_SMC_IT_NOTIF_PENDING;
return res.a1;
+}
+static u32 set_it_mask(optee_invoke_fn *invoke_fn, u32 it_value, bool mask) +{
struct arm_smccc_res res;
invoke_fn(OPTEE_SMC_SET_IT_NOTIF_MASK, it_value, mask, 0, 0, 0, 0, 0, &res);
if (res.a0)
return 0;
return res.a1;
+}
+static int handle_optee_it(struct optee *optee) +{
bool value_valid;
bool value_pending;
u32 it;
do {
struct irq_desc *desc;
it = get_it_value(optee->smc.invoke_fn, &value_valid, &value_pending);
if (!value_valid)
break;
desc = irq_to_desc(irq_find_mapping(optee->smc.domain, it));
if (!desc) {
pr_err("no desc for optee IT:%d\n", it);
return -EIO;
}
handle_simple_irq(desc);
What is this? Please use generic_handle_domain_irq(), like any other driver. Why is the flow handler handle_simple_irq()? You need to explain what the signalling is for the secure-provided interrupts.
My fault. I thought handle_simple_irq() would better apply here since its description: * Simple interrupts are either sent from a demultiplexing interrupt * handler or come from hardware, where no interrupt hardware control * is necessary.
OP-TEE secure world has already dealt with the HW interrupt resources (ack/etc...) before it notifies Linux kernel of the event.
} while (value_pending);
return 0;
+}
+static void optee_it_irq_mask(struct irq_data *d) +{
struct optee *optee = d->domain->host_data;
set_it_mask(optee->smc.invoke_fn, d->hwirq, true);
+}
+static void optee_it_irq_unmask(struct irq_data *d) +{
struct optee *optee = d->domain->host_data;
set_it_mask(optee->smc.invoke_fn, d->hwirq, false);
+}
+static struct irq_chip optee_it_irq_chip = {
.name = "optee-it",
.irq_disable = optee_it_irq_mask,
.irq_enable = optee_it_irq_unmask,
.flags = IRQCHIP_SKIP_SET_WAKE,
Is it a mask or a disable? These are different beasts.
Indeed, thanks for catching that. I think we need to 4 (enable/disable/mask/unmask). I'll fix.
+};
+static int optee_it_alloc(struct irq_domain *d, unsigned int virq,
unsigned int nr_irqs, void *data)
+{
struct irq_fwspec *fwspec = data;
irq_hw_number_t hwirq;
hwirq = fwspec->param[0];
irq_domain_set_hwirq_and_chip(d, virq, hwirq, &optee_it_irq_chip, d->host_data);
return 0;
+}
+static const struct irq_domain_ops optee_it_irq_domain_ops = {
.alloc = optee_it_alloc,
.free = irq_domain_free_irqs_common,
+};
+static int optee_irq_domain_init(struct platform_device *pdev, struct optee *optee, u_int max_it) +{
struct device *dev = &pdev->dev;
struct device_node *np = dev->of_node;
optee->smc.domain = irq_domain_add_linear(np, max_it, &optee_it_irq_domain_ops, optee);
if (!optee->smc.domain) {
dev_err(dev, "Unable to add irq domain\n");
return -ENOMEM;
}
return 0;
+}
static u32 get_async_notif_value(optee_invoke_fn *invoke_fn, bool *value_valid, bool *value_pending) { @@ -1008,13 +1114,15 @@ static irqreturn_t notif_irq_handler(int irq, void *dev_id) }
do {
value = get_async_notif_value(optee->smc.invoke_fn,
&value_valid, &value_pending);
value = get_async_notif_value(optee->smc.invoke_fn, &value_valid, &value_pending); if (!value_valid) break; if (value == OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF) do_bottom_half = true;
else if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_IT_NOTIF &&
value == OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_IT)
handle_optee_it(optee);
NAK. This isn't how we deal with chained interrupts. Definitely not in an interrupt handler.
My apologies, what is wrong in this sequence. My expectations are: 1- Something happens on OP-TEE secure side that should be reported to Linux as a software interrupt event 2- OP-TEE (secure world) raises an HW interrupt to notify Linux optee driver that event(s) are pending 3- optee driver threade_irq handler is called and asks OP-TEE which are the pending events. This is done event per event until all pending event are consumed. 4- Each each pending events: 4a- (existing code) wake or spawn a thread when a pending event is related to a threaded context execution, 4b- (this patch) call the interrupt handler of the optee interrupt consumers when event is related to an interrupt event signalling,
Regards, Etienne
M.
-- Without deviation from the norm, progress is not possible.
On Thu, Jan 12, 2023 at 03:54:23PM +0100, Etienne Carriere wrote:
Adds optional interrupt controller properties used when OP-TEE generates interrupt events optee driver shall notified to its registered interrupt consumer. The example shows how OP-TEE can trigger a wakeup interrupt event consumed by a gpio-keys compatible device.
Why do we need this in DT? It's not a GPIO key, but an abuse of the binding. It looks like unnecessary abstraction to me.
Signed-off-by: Etienne Carriere etienne.carriere@linaro.org
.../arm/firmware/linaro,optee-tz.yaml | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml index d4dc0749f9fd..42874ca21b7e 100644 --- a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml +++ b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml @@ -40,6 +40,11 @@ properties: HVC #0, register assignments register assignments are specified in drivers/tee/optee/optee_smc.h
- interrupt-controller: true
- "#interrupt-cells":
- const: 1
required:
- compatible
- method
@@ -48,12 +53,24 @@ additionalProperties: false examples:
- |
- #include <dt-bindings/input/input.h> #include <dt-bindings/interrupt-controller/arm-gic.h> firmware {
optee {
optee: optee { compatible = "linaro,optee-tz"; method = "smc"; interrupts = <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>;
interrupt-controller;
#interrupt-cells = <1>;
};
- };
- wake_up {
compatible = "gpio-keys";
button {
linux,code = <KEY_WAKEUP>;
interrupts-extended = <&optee 0>;
In the end, you just need optee IRQ #0 to generate KEY_WAKEUP. Does either the optee interrupt number or the key code need to be configurable? If so, why? Why isn't #0 just wakeup and the driver can send KEY_WAKEUP?
DT is for non-discoverable hardware that we can't fix. Why repeat that for software interfaces to firmware?
Rob
On Fri, 13 Jan 2023 15:27:22 +0000, Etienne Carriere etienne.carriere@linaro.org wrote:
Hello Marc,
Thanks for the review.
On Fri, 13 Jan 2023 at 10:22, Marc Zyngier maz@kernel.org wrote:
On Thu, 12 Jan 2023 14:54:24 +0000, Etienne Carriere etienne.carriere@linaro.org wrote:
Adds an irq chip in optee driver to generate interrupts from OP-TEE notified interrupt events based on optee async notification. Upon such notification, optee driver invokes OP-TEE to query a pending interrupt event. If an interrupt notification is pending the invocation return OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_IT and optee driver can get the pending interrupt number with SMC function ID OPTEE_SMC_FUNCID_GET_IT_VALUE.
SMC function ID OPTEE_SMC_FUNCID_SET_IT_MASK allows Linux to mask/unmask an interrupt notification services.
The optee irq_chip if flagged IRQCHIP_SKIP_SET_WAKE to skip set_wake as optee interrupt notifications doesn't support the set_wake option. In case a device is using the optee irq and is marked as wakeup source, this result in an "Unbalanced IRQ xx wake disable" backtrace, since:
- in irq_set_irq_wake(ON), wake_depth gets incremented, then reset due to set_irq_wake_real() returns an error (irq_set_wake() isn't implemented)
- in irq_set_irq_wake(OFF), wake_depth is always 0, hence the warning
Is this relevant information?
The description is maybe too specific to the setup used for this feature. I'll rephrase that, unless IRQCHIP_SKIP_SET_WAKE flag is not relevant here.
IRQCHIP_SKIP_SET_FLAG is used when the irqchip doesn't provide any wake-up mechanism, but also isn't *denying* its use (there is a separate way to wake up from such an interrupt). I don't think you need to document that something goes wrong when you're doing something wrong.
Co-developed-by: Pascal Paillet p.paillet@foss.st.com Signed-off-by: Pascal Paillet p.paillet@foss.st.com Co-developed-by: Fabrice Gasnier fabrice.gasnier@foss.st.com Signed-off-by: Fabrice Gasnier fabrice.gasnier@foss.st.com Signed-off-by: Etienne Carriere etienne.carriere@linaro.org
drivers/tee/optee/optee_private.h | 2 + drivers/tee/optee/optee_smc.h | 78 +++++++++++++++- drivers/tee/optee/smc_abi.c | 142 ++++++++++++++++++++++++++++-- 3 files changed, 216 insertions(+), 6 deletions(-)
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h index e5bd3548691f..2a146d884d27 100644 --- a/drivers/tee/optee/optee_private.h +++ b/drivers/tee/optee/optee_private.h @@ -112,6 +112,7 @@ struct optee_pcpu {
- @optee_pcpu per_cpu optee instance for per cpu work or NULL
- @notif_pcpu_wq workqueue for per cpu aynchronous notification or NULL
- @notif_pcpu_work work for per cpu asynchronous notification
*/
- @domain interrupt domain registered by OP-TEE driver
struct optee_smc { optee_invoke_fn *invoke_fn; @@ -121,6 +122,7 @@ struct optee_smc { struct optee_pcpu __percpu *optee_pcpu; struct workqueue_struct *notif_pcpu_wq; struct work_struct notif_pcpu_work;
struct irq_domain *domain;
};
/** diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h index 73b5e7760d10..0cf83d5a2931 100644 --- a/drivers/tee/optee/optee_smc.h +++ b/drivers/tee/optee/optee_smc.h @@ -226,7 +226,8 @@ struct optee_smc_get_shm_config_result {
- a3 Bit[7:0]: Number of parameters needed for RPC to be supplied
as the second MSG arg struct for
OPTEE_SMC_CALL_WITH_ARG
- Bit[31:8]: Reserved (MBZ)
- Bit[23:8]: The maximum interrupt event notification number
- Bit[31:24]: Reserved (MBZ)
- a4-7 Preserved
- Error return register usage:
@@ -254,6 +255,11 @@ struct optee_smc_get_shm_config_result { #define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF BIT(5) /* Secure world supports pre-allocating RPC arg struct */ #define OPTEE_SMC_SEC_CAP_RPC_ARG BIT(6) +/* Secure world supports interrupt events notification to normal world */ +#define OPTEE_SMC_SEC_CAP_IT_NOTIF BIT(7)
+#define OPTEE_SMC_SEC_CAP_MAX_NOTIF_IT_MASK GENMASK(23, 8) +#define OPTEE_SMC_SEC_CAP_MAX_NOTIF_IT_SHIFT 8
#define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES 9 #define OPTEE_SMC_EXCHANGE_CAPABILITIES \ @@ -416,6 +422,12 @@ struct optee_smc_disable_shm_cache_result { */ #define OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF 0
+/*
- Notification that OP-TEE triggers an interrupt event to Linux kernel
- for an interrupt consumer.
- */
+#define OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_IT 1
#define OPTEE_SMC_FUNCID_GET_ASYNC_NOTIF_VALUE 17 #define OPTEE_SMC_GET_ASYNC_NOTIF_VALUE \ OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_ASYNC_NOTIF_VALUE) @@ -426,6 +438,70 @@ struct optee_smc_disable_shm_cache_result { /* See OPTEE_SMC_CALL_WITH_REGD_ARG above */ #define OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG 19
+/*
- Retrieve the interrupt number of the pending interrupt event notified to
- non-secure world since the last call of this function.
- OP-TEE keeps a record of all posted interrupt notification events. When the
- async notif interrupt is received by non-secure world, this function should
- be called until all pended interrupt events have been retrieved. When an
- interrupt event is retrieved it is cleared from the record in secure world.
- It is expected that this function is called from an interrupt handler
- in normal world.
- Call requests usage:
- a0 SMC Function ID, OPTEE_SMC_GET_IT_NOTIF_VALUE
- a1-6 Not used
- a7 Hypervisor Client ID register
- Normal return register usage:
- a0 OPTEE_SMC_RETURN_OK
- a1 IT_NOTIF interrupt identifier value
- a2 Bit[0]: OPTEE_SMC_IT_NOTIF_VALID if the value in a1 is
valid, else 0 if no interrupt event were pending
- a2 Bit[1]: OPTEE_SMC_IT_NOTIF_PENDING if another interrupt event
value is pending, else 0.
- Bit[31:2]: MBZ
- a3-7 Preserved
- Not supported return register usage:
- a0 OPTEE_SMC_RETURN_ENOTAVAIL
- a1-7 Preserved
- */
+#define OPTEE_SMC_IT_NOTIF_VALID BIT(0) +#define OPTEE_SMC_IT_NOTIF_PENDING BIT(1)
+#define OPTEE_SMC_FUNCID_GET_IT_NOTIF_VALUE 20 +#define OPTEE_SMC_GET_IT_NOTIF_VALUE \
OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_IT_NOTIF_VALUE)
+/*
- Mask or unmask an interrupt notification event.
- It is expected that this function is called from an interrupt handler
- in normal world.
- Call requests usage:
- a0 SMC Function ID, OPTEE_SMC_SET_IT_NOTIF_MASK
- a1 Interrupt identifier value
- a2 Bit[0]: 1 if interrupt event is to be masked, 0 if it is to be unmasked
- a2 Bit[31:1] MBZ
- a3-6 Not used
- a7 Hypervisor Client ID register
- Normal return register usage:
- a0 OPTEE_SMC_RETURN_OK
- a1-7 Preserved
- Not supdealed ported return register usage:
- a0 OPTEE_SMC_RETURN_ENOTAVAIL
- a1-7 Preserved
- */
+#define OPTEE_SMC_FUNCID_SET_IT_NOTIF_MASK 21 +#define OPTEE_SMC_SET_IT_NOTIF_MASK \
OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_SET_IT_NOTIF_MASK)
/*
- Resume from RPC (for example after processing a foreign interrupt)
diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c index 8c2d58d605ac..0360afde119f 100644 --- a/drivers/tee/optee/smc_abi.c +++ b/drivers/tee/optee/smc_abi.c @@ -977,6 +977,112 @@ static int optee_smc_stop_async_notif(struct tee_context *ctx)
- Asynchronous notification
*/
+static u32 get_it_value(optee_invoke_fn *invoke_fn, bool *value_valid,
bool *value_pending)
What value? If this is supposed to return a set of pending bits, just name the function to reflect that.
Communication between Linux kernel and OP-TEE is this case is rather basic here, no shared memory used, only few CPU registers can be used to shared information. So Linux invokes OP-TEE for each pending optee interrupt event: each invocation returns a interrupt event number (an integer value) and a status whether another interrupt event is pending and shall be retrieved invoking OP-TEE again. In case Linux invoke OP-TEE for a pending interrupt and none is pending, a last status is also provided: 'value_valid'.
I could maybe change the prototype to something like: static u32 get_pending_it_number(optee_invoke_fn *invoke_fn, bool *it_number_valid, bool *more_pending_it)
What does 'it' mean? Interrupt? Spell it out. Really, this should read as:
get_pending_irq()
Also, at no point do you explain that each PPI is only a mux interrupt for a bunch of chained interrupts.
Sorry, I don't understand your question. The "it notif" feature proposed in this change is not straight related a PPI. Previous patch in this series is indeed related to PPI: it propose optee driver can use a single PPI instead of a signle SPI for the OP-TEE "asyn notification" feature. This change allows to mux interrupts events (from OP-TEE to Linux optee driver) over "OP-TEE async notif" means, which is using a single SPI or PPI. Maybe I should have submitted both changes separately :(
Surely a less cryptic explaination would have helped.
+{
struct arm_smccc_res res;
invoke_fn(OPTEE_SMC_GET_IT_NOTIF_VALUE, 0, 0, 0, 0, 0, 0, 0, &res);
if (res.a0)
return 0;
*value_valid = res.a2 & OPTEE_SMC_IT_NOTIF_VALID;
*value_pending = res.a2 & OPTEE_SMC_IT_NOTIF_PENDING;
return res.a1;
+}
+static u32 set_it_mask(optee_invoke_fn *invoke_fn, u32 it_value, bool mask) +{
struct arm_smccc_res res;
invoke_fn(OPTEE_SMC_SET_IT_NOTIF_MASK, it_value, mask, 0, 0, 0, 0, 0, &res);
if (res.a0)
return 0;
return res.a1;
+}
+static int handle_optee_it(struct optee *optee) +{
bool value_valid;
bool value_pending;
u32 it;
do {
struct irq_desc *desc;
it = get_it_value(optee->smc.invoke_fn, &value_valid, &value_pending);
if (!value_valid)
break;
desc = irq_to_desc(irq_find_mapping(optee->smc.domain, it));
if (!desc) {
pr_err("no desc for optee IT:%d\n", it);
return -EIO;
}
handle_simple_irq(desc);
What is this? Please use generic_handle_domain_irq(), like any other driver. Why is the flow handler handle_simple_irq()? You need to explain what the signalling is for the secure-provided interrupts.
My fault. I thought handle_simple_irq() would better apply here since its description:
- Simple interrupts are either sent from a demultiplexing interrupt
- handler or come from hardware, where no interrupt hardware control
- is necessary.
Why isn't masking necessary? What is the signalling between OPTEE and NS? Does the "get_it_value" function *consume* the pending bits? You need to answer and document all of that, and only then pick the flow that matches these requirements.
OP-TEE secure world has already dealt with the HW interrupt resources (ack/etc...) before it notifies Linux kernel of the event.
} while (value_pending);
return 0;
+}
+static void optee_it_irq_mask(struct irq_data *d) +{
struct optee *optee = d->domain->host_data;
set_it_mask(optee->smc.invoke_fn, d->hwirq, true);
+}
+static void optee_it_irq_unmask(struct irq_data *d) +{
struct optee *optee = d->domain->host_data;
set_it_mask(optee->smc.invoke_fn, d->hwirq, false);
+}
+static struct irq_chip optee_it_irq_chip = {
.name = "optee-it",
.irq_disable = optee_it_irq_mask,
.irq_enable = optee_it_irq_unmask,
.flags = IRQCHIP_SKIP_SET_WAKE,
Is it a mask or a disable? These are different beasts.
Indeed, thanks for catching that. I think we need to 4 (enable/disable/mask/unmask). I'll fix.
What does enable do differently from unmask?
+};
+static int optee_it_alloc(struct irq_domain *d, unsigned int virq,
unsigned int nr_irqs, void *data)
+{
struct irq_fwspec *fwspec = data;
irq_hw_number_t hwirq;
hwirq = fwspec->param[0];
irq_domain_set_hwirq_and_chip(d, virq, hwirq, &optee_it_irq_chip, d->host_data);
return 0;
+}
+static const struct irq_domain_ops optee_it_irq_domain_ops = {
.alloc = optee_it_alloc,
.free = irq_domain_free_irqs_common,
+};
+static int optee_irq_domain_init(struct platform_device *pdev, struct optee *optee, u_int max_it) +{
struct device *dev = &pdev->dev;
struct device_node *np = dev->of_node;
optee->smc.domain = irq_domain_add_linear(np, max_it, &optee_it_irq_domain_ops, optee);
if (!optee->smc.domain) {
dev_err(dev, "Unable to add irq domain\n");
return -ENOMEM;
}
return 0;
+}
static u32 get_async_notif_value(optee_invoke_fn *invoke_fn, bool *value_valid, bool *value_pending) { @@ -1008,13 +1114,15 @@ static irqreturn_t notif_irq_handler(int irq, void *dev_id) }
do {
value = get_async_notif_value(optee->smc.invoke_fn,
&value_valid, &value_pending);
value = get_async_notif_value(optee->smc.invoke_fn, &value_valid, &value_pending); if (!value_valid) break; if (value == OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF) do_bottom_half = true;
else if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_IT_NOTIF &&
value == OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_IT)
handle_optee_it(optee);
NAK. This isn't how we deal with chained interrupts. Definitely not in an interrupt handler.
My apologies, what is wrong in this sequence. My expectations are: 1- Something happens on OP-TEE secure side that should be reported to Linux as a software interrupt event
Why? There is nothing Linux-specific in OPTEE. Why should OPTEE be prescriptive of the way this is handled?
2- OP-TEE (secure world) raises an HW interrupt to notify Linux optee driver that event(s) are pending 3- optee driver threade_irq handler is called and asks OP-TEE which are the pending events. This is done event per event until all pending event are consumed.
Why is this done in a thread? I'd expect the *handlers* to be threaded, but not the irqchip part of it (which is crucially missing here).
M.
On Sun, 15 Jan 2023 at 11:14, Marc Zyngier maz@kernel.org wrote:
On Fri, 13 Jan 2023 15:27:22 +0000, Etienne Carriere etienne.carriere@linaro.org wrote:
Hello Marc,
Thanks for the review.
On Fri, 13 Jan 2023 at 10:22, Marc Zyngier maz@kernel.org wrote:
On Thu, 12 Jan 2023 14:54:24 +0000, Etienne Carriere etienne.carriere@linaro.org wrote:
Adds an irq chip in optee driver to generate interrupts from OP-TEE notified interrupt events based on optee async notification. Upon such notification, optee driver invokes OP-TEE to query a pending interrupt event. If an interrupt notification is pending the invocation return OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_IT and optee driver can get the pending interrupt number with SMC function ID OPTEE_SMC_FUNCID_GET_IT_VALUE.
SMC function ID OPTEE_SMC_FUNCID_SET_IT_MASK allows Linux to mask/unmask an interrupt notification services.
The optee irq_chip if flagged IRQCHIP_SKIP_SET_WAKE to skip set_wake as optee interrupt notifications doesn't support the set_wake option. In case a device is using the optee irq and is marked as wakeup source, this result in an "Unbalanced IRQ xx wake disable" backtrace, since:
- in irq_set_irq_wake(ON), wake_depth gets incremented, then reset due to set_irq_wake_real() returns an error (irq_set_wake() isn't implemented)
- in irq_set_irq_wake(OFF), wake_depth is always 0, hence the warning
Is this relevant information?
The description is maybe too specific to the setup used for this feature. I'll rephrase that, unless IRQCHIP_SKIP_SET_WAKE flag is not relevant here.
IRQCHIP_SKIP_SET_FLAG is used when the irqchip doesn't provide any wake-up mechanism, but also isn't *denying* its use (there is a separate way to wake up from such an interrupt). I don't think you need to document that something goes wrong when you're doing something wrong.
:) fair.
Co-developed-by: Pascal Paillet p.paillet@foss.st.com Signed-off-by: Pascal Paillet p.paillet@foss.st.com Co-developed-by: Fabrice Gasnier fabrice.gasnier@foss.st.com Signed-off-by: Fabrice Gasnier fabrice.gasnier@foss.st.com Signed-off-by: Etienne Carriere etienne.carriere@linaro.org
drivers/tee/optee/optee_private.h | 2 + drivers/tee/optee/optee_smc.h | 78 +++++++++++++++- drivers/tee/optee/smc_abi.c | 142 ++++++++++++++++++++++++++++-- 3 files changed, 216 insertions(+), 6 deletions(-)
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h index e5bd3548691f..2a146d884d27 100644 --- a/drivers/tee/optee/optee_private.h +++ b/drivers/tee/optee/optee_private.h @@ -112,6 +112,7 @@ struct optee_pcpu {
- @optee_pcpu per_cpu optee instance for per cpu work or NULL
- @notif_pcpu_wq workqueue for per cpu aynchronous notification or NULL
- @notif_pcpu_work work for per cpu asynchronous notification
*/
- @domain interrupt domain registered by OP-TEE driver
struct optee_smc { optee_invoke_fn *invoke_fn; @@ -121,6 +122,7 @@ struct optee_smc { struct optee_pcpu __percpu *optee_pcpu; struct workqueue_struct *notif_pcpu_wq; struct work_struct notif_pcpu_work;
struct irq_domain *domain;
};
/** diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h index 73b5e7760d10..0cf83d5a2931 100644 --- a/drivers/tee/optee/optee_smc.h +++ b/drivers/tee/optee/optee_smc.h @@ -226,7 +226,8 @@ struct optee_smc_get_shm_config_result {
- a3 Bit[7:0]: Number of parameters needed for RPC to be supplied
as the second MSG arg struct for
OPTEE_SMC_CALL_WITH_ARG
- Bit[31:8]: Reserved (MBZ)
- Bit[23:8]: The maximum interrupt event notification number
- Bit[31:24]: Reserved (MBZ)
- a4-7 Preserved
- Error return register usage:
@@ -254,6 +255,11 @@ struct optee_smc_get_shm_config_result { #define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF BIT(5) /* Secure world supports pre-allocating RPC arg struct */ #define OPTEE_SMC_SEC_CAP_RPC_ARG BIT(6) +/* Secure world supports interrupt events notification to normal world */ +#define OPTEE_SMC_SEC_CAP_IT_NOTIF BIT(7)
+#define OPTEE_SMC_SEC_CAP_MAX_NOTIF_IT_MASK GENMASK(23, 8) +#define OPTEE_SMC_SEC_CAP_MAX_NOTIF_IT_SHIFT 8
#define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES 9 #define OPTEE_SMC_EXCHANGE_CAPABILITIES \ @@ -416,6 +422,12 @@ struct optee_smc_disable_shm_cache_result { */ #define OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF 0
+/*
- Notification that OP-TEE triggers an interrupt event to Linux kernel
- for an interrupt consumer.
- */
+#define OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_IT 1
#define OPTEE_SMC_FUNCID_GET_ASYNC_NOTIF_VALUE 17 #define OPTEE_SMC_GET_ASYNC_NOTIF_VALUE \ OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_ASYNC_NOTIF_VALUE) @@ -426,6 +438,70 @@ struct optee_smc_disable_shm_cache_result { /* See OPTEE_SMC_CALL_WITH_REGD_ARG above */ #define OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG 19
+/*
- Retrieve the interrupt number of the pending interrupt event notified to
- non-secure world since the last call of this function.
- OP-TEE keeps a record of all posted interrupt notification events. When the
- async notif interrupt is received by non-secure world, this function should
- be called until all pended interrupt events have been retrieved. When an
- interrupt event is retrieved it is cleared from the record in secure world.
- It is expected that this function is called from an interrupt handler
- in normal world.
- Call requests usage:
- a0 SMC Function ID, OPTEE_SMC_GET_IT_NOTIF_VALUE
- a1-6 Not used
- a7 Hypervisor Client ID register
- Normal return register usage:
- a0 OPTEE_SMC_RETURN_OK
- a1 IT_NOTIF interrupt identifier value
- a2 Bit[0]: OPTEE_SMC_IT_NOTIF_VALID if the value in a1 is
valid, else 0 if no interrupt event were pending
- a2 Bit[1]: OPTEE_SMC_IT_NOTIF_PENDING if another interrupt event
value is pending, else 0.
- Bit[31:2]: MBZ
- a3-7 Preserved
- Not supported return register usage:
- a0 OPTEE_SMC_RETURN_ENOTAVAIL
- a1-7 Preserved
- */
+#define OPTEE_SMC_IT_NOTIF_VALID BIT(0) +#define OPTEE_SMC_IT_NOTIF_PENDING BIT(1)
+#define OPTEE_SMC_FUNCID_GET_IT_NOTIF_VALUE 20 +#define OPTEE_SMC_GET_IT_NOTIF_VALUE \
OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_IT_NOTIF_VALUE)
+/*
- Mask or unmask an interrupt notification event.
- It is expected that this function is called from an interrupt handler
- in normal world.
- Call requests usage:
- a0 SMC Function ID, OPTEE_SMC_SET_IT_NOTIF_MASK
- a1 Interrupt identifier value
- a2 Bit[0]: 1 if interrupt event is to be masked, 0 if it is to be unmasked
- a2 Bit[31:1] MBZ
- a3-6 Not used
- a7 Hypervisor Client ID register
- Normal return register usage:
- a0 OPTEE_SMC_RETURN_OK
- a1-7 Preserved
- Not supdealed ported return register usage:
- a0 OPTEE_SMC_RETURN_ENOTAVAIL
- a1-7 Preserved
- */
+#define OPTEE_SMC_FUNCID_SET_IT_NOTIF_MASK 21 +#define OPTEE_SMC_SET_IT_NOTIF_MASK \
OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_SET_IT_NOTIF_MASK)
/*
- Resume from RPC (for example after processing a foreign interrupt)
diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c index 8c2d58d605ac..0360afde119f 100644 --- a/drivers/tee/optee/smc_abi.c +++ b/drivers/tee/optee/smc_abi.c @@ -977,6 +977,112 @@ static int optee_smc_stop_async_notif(struct tee_context *ctx)
- Asynchronous notification
*/
+static u32 get_it_value(optee_invoke_fn *invoke_fn, bool *value_valid,
bool *value_pending)
What value? If this is supposed to return a set of pending bits, just name the function to reflect that.
Communication between Linux kernel and OP-TEE is this case is rather basic here, no shared memory used, only few CPU registers can be used to shared information. So Linux invokes OP-TEE for each pending optee interrupt event: each invocation returns a interrupt event number (an integer value) and a status whether another interrupt event is pending and shall be retrieved invoking OP-TEE again. In case Linux invoke OP-TEE for a pending interrupt and none is pending, a last status is also provided: 'value_valid'.
I could maybe change the prototype to something like: static u32 get_pending_it_number(optee_invoke_fn *invoke_fn, bool *it_number_valid, bool *more_pending_it)
What does 'it' mean? Interrupt? Spell it out. Really, this should read as:
get_pending_irq()
Ok, understood. Thanks.
Also, at no point do you explain that each PPI is only a mux interrupt for a bunch of chained interrupts.
Sorry, I don't understand your question. The "it notif" feature proposed in this change is not straight related a PPI. Previous patch in this series is indeed related to PPI: it propose optee driver can use a single PPI instead of a signle SPI for the OP-TEE "asyn notification" feature. This change allows to mux interrupts events (from OP-TEE to Linux optee driver) over "OP-TEE async notif" means, which is using a single SPI or PPI. Maybe I should have submitted both changes separately :(
Surely a less cryptic explaination would have helped.
Indeed. You're not the first to tell :( Sorry I'll try to better use Linux wordings in Linux kernel changes. Thanks for being frank.
+{
struct arm_smccc_res res;
invoke_fn(OPTEE_SMC_GET_IT_NOTIF_VALUE, 0, 0, 0, 0, 0, 0, 0, &res);
if (res.a0)
return 0;
*value_valid = res.a2 & OPTEE_SMC_IT_NOTIF_VALID;
*value_pending = res.a2 & OPTEE_SMC_IT_NOTIF_PENDING;
return res.a1;
+}
+static u32 set_it_mask(optee_invoke_fn *invoke_fn, u32 it_value, bool mask) +{
struct arm_smccc_res res;
invoke_fn(OPTEE_SMC_SET_IT_NOTIF_MASK, it_value, mask, 0, 0, 0, 0, 0, &res);
if (res.a0)
return 0;
return res.a1;
+}
+static int handle_optee_it(struct optee *optee) +{
bool value_valid;
bool value_pending;
u32 it;
do {
struct irq_desc *desc;
it = get_it_value(optee->smc.invoke_fn, &value_valid, &value_pending);
if (!value_valid)
break;
desc = irq_to_desc(irq_find_mapping(optee->smc.domain, it));
if (!desc) {
pr_err("no desc for optee IT:%d\n", it);
return -EIO;
}
handle_simple_irq(desc);
What is this? Please use generic_handle_domain_irq(), like any other driver. Why is the flow handler handle_simple_irq()? You need to explain what the signalling is for the secure-provided interrupts.
My fault. I thought handle_simple_irq() would better apply here since its description:
- Simple interrupts are either sent from a demultiplexing interrupt
- handler or come from hardware, where no interrupt hardware control
- is necessary.
Why isn't masking necessary? What is the signalling between OPTEE and NS? Does the "get_it_value" function *consume* the pending bits? You need to answer and document all of that, and only then pick the flow that matches these requirements.
Yes, thet get_pending_irq() function is expect to consume the pending irq. As soon as it is consumed, any occurence of HW event (in OP-TEE world) that results in this optee irq be raised, will set again that irq number as pending and fire the irq leading to this get_pending_irq() function be called.
Thanks for the feedback. Maksing, enabling etc... are indeed needed. I'll come up with a (hopefully) better proposal.
OP-TEE secure world has already dealt with the HW interrupt resources (ack/etc...) before it notifies Linux kernel of the event.
} while (value_pending);
return 0;
+}
+static void optee_it_irq_mask(struct irq_data *d) +{
struct optee *optee = d->domain->host_data;
set_it_mask(optee->smc.invoke_fn, d->hwirq, true);
+}
+static void optee_it_irq_unmask(struct irq_data *d) +{
struct optee *optee = d->domain->host_data;
set_it_mask(optee->smc.invoke_fn, d->hwirq, false);
+}
+static struct irq_chip optee_it_irq_chip = {
.name = "optee-it",
.irq_disable = optee_it_irq_mask,
.irq_enable = optee_it_irq_unmask,
.flags = IRQCHIP_SKIP_SET_WAKE,
Is it a mask or a disable? These are different beasts.
Indeed, thanks for catching that. I think we need to 4 (enable/disable/mask/unmask). I'll fix.
What does enable do differently from unmask?
I think I saw it the wrong way. I understand implementing .irq_mask & .irq_unmask operations should be enough to notify OP-TEE world and let it manage the interrupt detection on its side.
+};
+static int optee_it_alloc(struct irq_domain *d, unsigned int virq,
unsigned int nr_irqs, void *data)
+{
struct irq_fwspec *fwspec = data;
irq_hw_number_t hwirq;
hwirq = fwspec->param[0];
irq_domain_set_hwirq_and_chip(d, virq, hwirq, &optee_it_irq_chip, d->host_data);
return 0;
+}
+static const struct irq_domain_ops optee_it_irq_domain_ops = {
.alloc = optee_it_alloc,
.free = irq_domain_free_irqs_common,
+};
+static int optee_irq_domain_init(struct platform_device *pdev, struct optee *optee, u_int max_it) +{
struct device *dev = &pdev->dev;
struct device_node *np = dev->of_node;
optee->smc.domain = irq_domain_add_linear(np, max_it, &optee_it_irq_domain_ops, optee);
if (!optee->smc.domain) {
dev_err(dev, "Unable to add irq domain\n");
return -ENOMEM;
}
return 0;
+}
static u32 get_async_notif_value(optee_invoke_fn *invoke_fn, bool *value_valid, bool *value_pending) { @@ -1008,13 +1114,15 @@ static irqreturn_t notif_irq_handler(int irq, void *dev_id) }
do {
value = get_async_notif_value(optee->smc.invoke_fn,
&value_valid, &value_pending);
value = get_async_notif_value(optee->smc.invoke_fn, &value_valid, &value_pending); if (!value_valid) break; if (value == OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF) do_bottom_half = true;
else if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_IT_NOTIF &&
value == OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_IT)
handle_optee_it(optee);
NAK. This isn't how we deal with chained interrupts. Definitely not in an interrupt handler.
My apologies, what is wrong in this sequence. My expectations are: 1- Something happens on OP-TEE secure side that should be reported to Linux as a software interrupt event
Why? There is nothing Linux-specific in OPTEE. Why should OPTEE be prescriptive of the way this is handled?
In embedded system, we can have for example a unqiue GPIO expander to get some input data, where some IOs are related to OP-TEE world side services and other IOs for so-called normal world/Linux services. Because OP-TEE is paranoïd, it controls the expenander and relays Linux related interrupt to it Linux consumer driver through optee driver, acting as a irq controller. Wake up irqs can also need the same paths. The wake up controller is under OP-TEE world control, by device implementation, but some wakeup irqs should hit Linux as wakeup source.
2- OP-TEE (secure world) raises an HW interrupt to notify Linux optee driver that event(s) are pending 3- optee driver threade_irq handler is called and asks OP-TEE which are the pending events. This is done event per event until all pending event are consumed.
Why is this done in a thread? I'd expect the *handlers* to be threaded, but not the irqchip part of it (which is crucially missing here).
I do understand the optee irq implementation should be handled from primary handler of the optee async notif irq. I'll come with a better proposal, I hope.
Thanks for all the feedback and time spent for the explanations
Regards, Etienne
M.
-- Without deviation from the norm, progress is not possible.
On Fri, 13 Jan 2023 at 21:42, Rob Herring robh@kernel.org wrote:
On Thu, Jan 12, 2023 at 03:54:23PM +0100, Etienne Carriere wrote:
Adds optional interrupt controller properties used when OP-TEE generates interrupt events optee driver shall notified to its registered interrupt consumer. The example shows how OP-TEE can trigger a wakeup interrupt event consumed by a gpio-keys compatible device.
Why do we need this in DT? It's not a GPIO key, but an abuse of the binding. It looks like unnecessary abstraction to me.
This is when for example OP-TEE world controller a IOs controller device. When some IOs are relate to OP-TEE feature, the controller route to OP-TEE handler. When the IO detection relates to Linux irqs it is routed to Linux, using optee driver irqchip. As Linux uses DT for device drivers to get their interrupt (controler phandle + arg), defining the irqchip in the DT of the platform running that OP-TEE firmware make sense to me.
The same way OP-TEE can be in charge of the wakeup source controllers and notify Linxu of event for the wakeup that relate to Linux services.
Signed-off-by: Etienne Carriere etienne.carriere@linaro.org
.../arm/firmware/linaro,optee-tz.yaml | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml index d4dc0749f9fd..42874ca21b7e 100644 --- a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml +++ b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml @@ -40,6 +40,11 @@ properties: HVC #0, register assignments register assignments are specified in drivers/tee/optee/optee_smc.h
- interrupt-controller: true
- "#interrupt-cells":
- const: 1
required:
- compatible
- method
@@ -48,12 +53,24 @@ additionalProperties: false
examples:
- |
- #include <dt-bindings/input/input.h> #include <dt-bindings/interrupt-controller/arm-gic.h> firmware {
optee {
optee: optee { compatible = "linaro,optee-tz"; method = "smc"; interrupts = <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>;
interrupt-controller;
#interrupt-cells = <1>;
};
- };
- wake_up {
compatible = "gpio-keys";
button {
linux,code = <KEY_WAKEUP>;
interrupts-extended = <&optee 0>;
In the end, you just need optee IRQ #0 to generate KEY_WAKEUP. Does either the optee interrupt number or the key code need to be configurable? If so, why? Why isn't #0 just wakeup and the driver can send KEY_WAKEUP?
The OP-TEE driver is a generic firmware driver. Platforms do not have specific hooks in it. A generic DT definition of the irqs exposed by opte driver irqchip allows consumers to get their irq resource. I even think 'allows' above could be replaced by is-required-by.
Here, binding KEY_WAKEUP to the OP-TEE firmware related irq line from the platform DT reuses existing drivers and bindings to get a irq wkaeup source, signaling KEY_WAKEUP even, when wakeup stouce controller is assigned to (controller by) OP-TEE world. This is an example. Maybe the binding are miss used, but I don't see why. Another example I plan to post is building an mailbox for SMCI notification from a SCMI service host in OP-TEE. OP-TEE would use this optee irqchip to get the interrupt related to the SCMI notification channel. In embedded system, limited resources can be shared by subsystems.
DT is for non-discoverable hardware that we can't fix. Why repeat that for software interfaces to firmware?
Do you mean the optee driver should enumerate the interrupt lines exposed by OP-TEE and register each line accordingly? This is doable I guess. But that would not prevent Linux kernel DT to define a interrupt controller consumer device nodes can refer to for their need.
BR, Etienne
Rob
Hi Etienne,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on robh/for-next] [also build test WARNING on krzk/for-next krzk-dt/for-next krzk-mem-ctrl/for-next linus/master v6.2-rc4] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Etienne-Carriere/optee-add-pe... base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next patch link: https://lore.kernel.org/r/20230112145424.3791276-2-etienne.carriere%40linaro... patch subject: [PATCH 1/3] optee: add per cpu asynchronous notification config: arm64-randconfig-s043-20230116 compiler: aarch64-linux-gcc (GCC) 12.1.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # apt-get install sparse # sparse version: v0.6.4-39-gce1a6720-dirty # https://github.com/intel-lab-lkp/linux/commit/3ae0dd52e3ae0cb2b3847b7c1e8338... git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Etienne-Carriere/optee-add-per-cpu-asynchronous-notification/20230112-232957 git checkout 3ae0dd52e3ae0cb2b3847b7c1e83381563751041 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=arm64 olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/tee/optee/
If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot lkp@intel.com
sparse warnings: (new ones prefixed by >>)
drivers/tee/optee/smc_abi.c:1071:20: sparse: sparse: incorrect type in assignment (different address spaces) @@ expected struct optee_pcpu *optee_pcpu @@ got struct optee_pcpu [noderef] __percpu * @@
drivers/tee/optee/smc_abi.c:1071:20: sparse: expected struct optee_pcpu *optee_pcpu drivers/tee/optee/smc_abi.c:1071:20: sparse: got struct optee_pcpu [noderef] __percpu *
drivers/tee/optee/smc_abi.c:1076:40: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected void const [noderef] __percpu *__vpp_verify @@ got struct optee_pcpu * @@
drivers/tee/optee/smc_abi.c:1076:40: sparse: expected void const [noderef] __percpu *__vpp_verify drivers/tee/optee/smc_abi.c:1076:40: sparse: got struct optee_pcpu *
drivers/tee/optee/smc_abi.c:1082:60: sparse: sparse: incorrect type in argument 4 (different address spaces) @@ expected void [noderef] __percpu *percpu_dev_id @@ got struct optee_pcpu *optee_pcpu @@
drivers/tee/optee/smc_abi.c:1082:60: sparse: expected void [noderef] __percpu *percpu_dev_id drivers/tee/optee/smc_abi.c:1082:60: sparse: got struct optee_pcpu *optee_pcpu
drivers/tee/optee/smc_abi.c:1099:31: sparse: sparse: incorrect type in assignment (different address spaces) @@ expected struct optee_pcpu [noderef] __percpu *optee_pcpu @@ got struct optee_pcpu *optee_pcpu @@
drivers/tee/optee/smc_abi.c:1099:31: sparse: expected struct optee_pcpu [noderef] __percpu *optee_pcpu drivers/tee/optee/smc_abi.c:1099:31: sparse: got struct optee_pcpu *optee_pcpu
drivers/tee/optee/smc_abi.c:1108:30: sparse: sparse: incorrect type in argument 2 (different address spaces) @@ expected void [noderef] __percpu * @@ got struct optee_pcpu *optee_pcpu @@
drivers/tee/optee/smc_abi.c:1108:30: sparse: expected void [noderef] __percpu * drivers/tee/optee/smc_abi.c:1108:30: sparse: got struct optee_pcpu *optee_pcpu
drivers/tee/optee/smc_abi.c:1110:21: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected void [noderef] __percpu *__pdata @@ got struct optee_pcpu *optee_pcpu @@
drivers/tee/optee/smc_abi.c:1110:21: sparse: expected void [noderef] __percpu *__pdata drivers/tee/optee/smc_abi.c:1110:21: sparse: got struct optee_pcpu *optee_pcpu
vim +1071 drivers/tee/optee/smc_abi.c
1063 1064 static int init_pcpu_irq(struct optee *optee, u_int irq) 1065 { 1066 struct optee_pcpu *optee_pcpu; 1067 spinlock_t lock; 1068 int cpu; 1069 int rc; 1070
1071 optee_pcpu = alloc_percpu(struct optee_pcpu);
1072 if (!optee_pcpu) 1073 return -ENOMEM; 1074 1075 for_each_present_cpu(cpu) {
1076 struct optee_pcpu *p = per_cpu_ptr(optee_pcpu, cpu);
1077 1078 p->optee = optee; 1079 } 1080 1081 rc = request_percpu_irq(irq, notif_irq_handler,
1082 "optee_pcpu_notification", optee_pcpu);
1083 if (rc) 1084 goto err_free_pcpu; 1085 1086 spin_lock_init(&lock); 1087 1088 spin_lock(&lock); 1089 enable_percpu_irq(irq, 0); 1090 spin_unlock(&lock); 1091 1092 INIT_WORK(&optee->smc.notif_pcpu_work, notif_pcpu_irq_work_fn); 1093 optee->smc.notif_pcpu_wq = create_workqueue("optee_pcpu_notification"); 1094 if (!optee->smc.notif_pcpu_wq) { 1095 rc = -EINVAL; 1096 goto err_free_pcpu_irq; 1097 } 1098
1099 optee->smc.optee_pcpu = optee_pcpu;
1100 optee->smc.notif_irq = irq; 1101 1102 return 0; 1103 1104 err_free_pcpu_irq: 1105 spin_lock(&lock); 1106 disable_percpu_irq(irq); 1107 spin_unlock(&lock);
1108 free_percpu_irq(irq, optee_pcpu);
1109 err_free_pcpu:
1110 free_percpu(optee_pcpu);
1111 1112 return rc; 1113 } 1114
op-tee@lists.trustedfirmware.org