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

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

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)

• 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 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)

• 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.