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(-)
--
2.25.1
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 <[email protected]>
---
.../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>;
};
};
--
2.25.1
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 <[email protected]>
Signed-off-by: Pascal Paillet <[email protected]>
Co-developed-by: Fabrice Gasnier <[email protected]>
Signed-off-by: Fabrice Gasnier <[email protected]>
Signed-off-by: Etienne Carriere <[email protected]>
---
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");
}
--
2.25.1
On Thu, 12 Jan 2023 14:54:24 +0000,
Etienne Carriere <[email protected]> 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 <[email protected]>
> Signed-off-by: Pascal Paillet <[email protected]>
> Co-developed-by: Fabrice Gasnier <[email protected]>
> Signed-off-by: Fabrice Gasnier <[email protected]>
> Signed-off-by: Etienne Carriere <[email protected]>
> ---
> 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)
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);
> + 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.
M.
--
Without deviation from the norm, progress is not possible.
Hello Marc,
Thanks for the review.
On Fri, 13 Jan 2023 at 10:22, Marc Zyngier <[email protected]> wrote:
>
> On Thu, 12 Jan 2023 14:54:24 +0000,
> Etienne Carriere <[email protected]> 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 <[email protected]>
> > Signed-off-by: Pascal Paillet <[email protected]>
> > Co-developed-by: Fabrice Gasnier <[email protected]>
> > Signed-off-by: Fabrice Gasnier <[email protected]>
> > Signed-off-by: Etienne Carriere <[email protected]>
> > ---
> > 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)
> > * 5. 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 <[email protected]>
> ---
> .../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 <[email protected]> wrote:
>
> Hello Marc,
>
> Thanks for the review.
>
>
> On Fri, 13 Jan 2023 at 10:22, Marc Zyngier <[email protected]> wrote:
> >
> > On Thu, 12 Jan 2023 14:54:24 +0000,
> > Etienne Carriere <[email protected]> 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 <[email protected]>
> > > Signed-off-by: Pascal Paillet <[email protected]>
> > > Co-developed-by: Fabrice Gasnier <[email protected]>
> > > Signed-off-by: Fabrice Gasnier <[email protected]>
> > > Signed-off-by: Etienne Carriere <[email protected]>
> > > ---
> > > 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)
> > > * 5. 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.
--
Without deviation from the norm, progress is not possible.
On Sun, 15 Jan 2023 at 11:14, Marc Zyngier <[email protected]> wrote:
>
> On Fri, 13 Jan 2023 15:27:22 +0000,
> Etienne Carriere <[email protected]> wrote:
> >
> > Hello Marc,
> >
> > Thanks for the review.
> >
> >
> > On Fri, 13 Jan 2023 at 10:22, Marc Zyngier <[email protected]> wrote:
> > >
> > > On Thu, 12 Jan 2023 14:54:24 +0000,
> > > Etienne Carriere <[email protected]> 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 <[email protected]>
> > > > Signed-off-by: Pascal Paillet <[email protected]>
> > > > Co-developed-by: Fabrice Gasnier <[email protected]>
> > > > Signed-off-by: Fabrice Gasnier <[email protected]>
> > > > Signed-off-by: Etienne Carriere <[email protected]>
> > > > ---
> > > > 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)
> > > > * 5. 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 <[email protected]> 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 <[email protected]>
> > ---
> > .../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