Re: [PATCH RFC v2 7/8] tee: optee: bind RPMI services and negotiate backend capabilities

[Amirreza Zarrabi <amirreza.zarrabi@oss.qualcomm.com>]

Hi jens,

On 10/8/2026 7:20 PM, Jens Wiklander wrote:
> Hi Amir,
> 
> On Tue, Oct 6, 2026 at 2:40 AM Amirreza Zarrabi
> <amirreza.zarrabi@oss.qualcomm.com> wrote:
>>
>> Register an RPMI service driver matching the OP-TEE service UUID and
>> integrate it with OP-TEE module initialization and removal.
>>
>> Check the service API version, query the trusted OS revision, and
>> obtain the RPC parameter and logical notification counts. Initialize
>> shared-memory tracking, the call queue, supplicant state and internal
>> context before publishing the client and supplicant TEE devices.
>>
>> Connect the RPMI backend to the common OP-TEE operations and enumerate
>> trusted application devices. Enable in-kernel RPMB routing when the
>> RPMB subsystem is reachable.
>>
>> Add removal and probe failure cleanup for the backend resources.
>>
>> Signed-off-by: Amirreza Zarrabi <amirreza.zarrabi@oss.qualcomm.com>
>> ---
>>  drivers/tee/optee/core.c          |  10 +-
>>  drivers/tee/optee/optee_private.h |  21 ++-
>>  drivers/tee/optee/rpmi_abi.c      | 286 ++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 312 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
>> index a52c1f498b99..8a44a25ebc66 100644
>> --- a/drivers/tee/optee/core.c
>> +++ b/drivers/tee/optee/core.c
>> @@ -220,6 +220,7 @@ void optee_remove_common(struct optee *optee)
>>
>>  static int smc_abi_rc;
>>  static int ffa_abi_rc;
>> +static int rpmi_abi_rc;
>>  static bool intf_is_regged;
>>
>>  static int __init optee_core_init(void)
>> @@ -245,14 +246,15 @@ static int __init optee_core_init(void)
>>
>>         smc_abi_rc = optee_smc_abi_register();
>>         ffa_abi_rc = optee_ffa_abi_register();
>> +       rpmi_abi_rc = optee_rpmi_abi_register();
>>
>> -       /* If both failed there's no point with this module */
>> -       if (smc_abi_rc && ffa_abi_rc) {
>> +       /* Keep the module if any supported transport registered successfully. */
>> +       if (smc_abi_rc && ffa_abi_rc && rpmi_abi_rc) {
>>                 if (IS_REACHABLE(CONFIG_RPMB)) {
>>                         rpmb_interface_unregister(&rpmb_class_intf);
>>                         intf_is_regged = false;
>>                 }
>> -               return smc_abi_rc;
>> +               return -EOPNOTSUPP;
>>         }
>>
>>         return 0;
>> @@ -270,6 +272,8 @@ static void __exit optee_core_exit(void)
>>                 optee_smc_abi_unregister();
>>         if (!ffa_abi_rc)
>>                 optee_ffa_abi_unregister();
>> +       if (!rpmi_abi_rc)
>> +               optee_rpmi_abi_unregister();
>>  }
>>  module_exit(optee_core_exit);
>>
>> diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
>> index 2422caf3c883..07c27e322a71 100644
>> --- a/drivers/tee/optee/optee_private.h
>> +++ b/drivers/tee/optee/optee_private.h
>> @@ -188,6 +188,8 @@ struct rpmi_tee_device;
>>   * @rdev: owning RPMI service device
>>   * @shm_rht_lock: protects parcel lookup, insertion, removal and publication
>>   * @shm_rht: lookup by the host-endian parcel ID and nonce pair
>> + * @sec_caps: negotiated optional OPTEE_RPMI_CAP_* features
>> + * @notification_count: negotiated nonzero number of logical notification keys
>>   *
>>   * Callers keep their tee_shm alive while using its registration. Lookup
>>   * returns a raw pointer; the mutex does not protect its lifetime after
>> @@ -199,6 +201,8 @@ struct optee_rpmi {
>>         /* Protects parcel lookup, insertion, removal and publication. */
>>         struct mutex shm_rht_lock;
>>         struct rhashtable shm_rht;
>> +       u32 sec_caps;
>> +       u32 notification_count;
> 
> Why are these two needed?
> 

Neither needs to be retained in the instance state: sec_caps is currently unused,
and notification_count is consumed during probe. I'll remove both fields.
I thought it would be nice to keep them.

>>  };
>>  #endif
>>
>> @@ -211,8 +215,8 @@ struct optee;
>>   * @os_build_id:       OP-TEE OS build identifier (0 if unspecified)
>>   *
>>   * Values come from OPTEE_SMC_CALL_GET_OS_REVISION (SMC ABI) or
>> - * OPTEE_FFA_GET_OS_VERSION (FF-A ABI); this is the trusted OS revision, not an
>> - * FF-A ABI version.
>> + * OPTEE_FFA_GET_OS_VERSION (FF-A ABI) or OPTEE_RPMI_GET_OS_VERSION (RPMI ABI).
>> + * This is the trusted OS revision, not a transport ABI version.
>>   */
>>  struct optee_revision {
>>         u32 os_major;
>> @@ -490,5 +494,18 @@ static inline void optee_ffa_abi_unregister(void)
>>  }
>>  #endif
>>
>> +#if IS_REACHABLE(CONFIG_RISCV_RPMI_TEE_TRANSPORT)
>> +int optee_rpmi_abi_register(void);
>> +void optee_rpmi_abi_unregister(void);
>> +#else
>> +static inline int optee_rpmi_abi_register(void)
>> +{
>> +       return -EOPNOTSUPP;
>> +}
>> +
>> +static inline void optee_rpmi_abi_unregister(void)
>> +{
>> +}
>> +#endif
>>
>>  #endif /*OPTEE_PRIVATE_H*/
>> diff --git a/drivers/tee/optee/rpmi_abi.c b/drivers/tee/optee/rpmi_abi.c
>> index 6e76316794c1..db541f3de425 100644
>> --- a/drivers/tee/optee/rpmi_abi.c
>> +++ b/drivers/tee/optee/rpmi_abi.c
>> @@ -684,3 +684,289 @@ static int optee_rpmi_do_call_with_arg(struct tee_context *ctx,
>>
>>         return optee_rpmi_yielding_call(ctx, &req, rpc_arg, system_thread);
>>  }
>> +
>> +/* Query and store the trusted OS revision. */
>> +static int optee_rpmi_get_os_version(struct optee *optee)
>> +{
>> +       struct optee_rpmi_probe_req req = {
>> +               .op = cpu_to_le32(OPTEE_RPMI_GET_OS_VERSION),
>> +       };
>> +       struct optee_rpmi_os_resp os;
>> +       int ret;
>> +
>> +       ret = optee_rpmi_call(optee, &req, sizeof(req), &os, sizeof(os));
>> +       if (ret)
>> +               return ret;
>> +
>> +       optee->revision.os_major = get_unaligned_le32(&os.major);
>> +       optee->revision.os_minor = get_unaligned_le32(&os.minor);
>> +       optee->revision.os_build_id = get_unaligned_le64(&os.build_id);
>> +
>> +       if (optee->revision.os_build_id)
>> +               pr_info("revision %u.%u (%016llx)\n",
>> +                       optee->revision.os_major, optee->revision.os_minor,
>> +                       optee->revision.os_build_id);
>> +       else
>> +               pr_info("revision %u.%u\n", optee->revision.os_major,
>> +                       optee->revision.os_minor);
>> +
>> +       return 0;
>> +}
>> +
>> +/* Query and store secure-world capabilities and buffer limits. */
>> +static int optee_rpmi_exchange_caps(struct optee *optee)
>> +{
>> +       struct optee_rpmi_probe_req req = {
>> +               .op = cpu_to_le32(OPTEE_RPMI_EXCHANGE_CAPABILITIES),
>> +       };
>> +       struct optee_rpmi_caps_resp caps;
>> +       u32 rpc_count, sec_caps, notif_count;
>> +       int ret;
>> +
>> +       ret = optee_rpmi_call(optee, &req, sizeof(req), &caps, sizeof(caps));
>> +       if (ret)
>> +               return ret;
>> +
>> +       sec_caps = get_unaligned_le32(&caps.secure_caps);
>> +       rpc_count = get_unaligned_le32(&caps.rpc_param_count);
>> +       notif_count = get_unaligned_le32(&caps.notification_count);
>> +       if (!notif_count || !rpc_count)
>> +               return -EPROTO;
>> +
>> +       optee->rpc_param_count = rpc_count;
>> +       optee->rpmi.sec_caps = sec_caps;
>> +       optee->rpmi.notification_count = notif_count;
>> +       optee->in_kernel_rpmb_routing = IS_REACHABLE(CONFIG_RPMB);
> 
> What if OP-TEE is built without RPMB support?
> 

I discussed this with Sumit, and the intention was to make RPMB probing
part of the baseline for the new ABI rather than negotiate it separately.
I can enable the capability bit if it should remain optional.

>> +
>> +       return 0;
>> +}
>> +
>> +static int optee_rpmi_api_is_compatible(struct optee *optee)
>> +{
>> +       struct optee_rpmi_probe_req req = {
>> +               .op = cpu_to_le32(OPTEE_RPMI_GET_API_VERSION),
>> +       };
>> +       struct optee_rpmi_api_resp api;
>> +       int ret;
>> +
>> +       ret = optee_rpmi_call(optee, &req, sizeof(req), &api, sizeof(api));
>> +       if (ret)
>> +               return ret;
>> +
>> +       if (get_unaligned_le32(&api.major) != OPTEE_RPMI_VERSION_MAJOR)
>> +               return -EPROTONOSUPPORT;
>> +
>> +       /* Version 1.0 has no minimum minor revision beyond zero. */
>> +       return 0;
>> +}
>> +
>> +static void optee_rpmi_get_version(struct tee_device *teedev,
>> +                                  struct tee_ioctl_version_data *vers)
>> +{
>> +       *vers = (struct tee_ioctl_version_data) {
>> +               .impl_id = TEE_IMPL_ID_OPTEE,
>> +               .gen_caps = TEE_GEN_CAP_GP | TEE_GEN_CAP_REG_MEM |
>> +                           TEE_GEN_CAP_MEMREF_NULL,
>> +       };
>> +}
>> +
>> +static int optee_rpmi_open(struct tee_context *ctx)
>> +{
>> +       return optee_open(ctx, true);
>> +}
>> +
>> +static const struct tee_driver_ops optee_rpmi_clnt_ops = {
>> +       .get_version = optee_rpmi_get_version,
>> +       .get_tee_revision = optee_get_revision,
>> +       .open = optee_rpmi_open,
>> +       .release = optee_release,
>> +       .open_session = optee_open_session,
>> +       .close_session = optee_close_session,
>> +       .invoke_func = optee_invoke_func,
>> +       .cancel_req = optee_cancel_req,
>> +       .shm_register = optee_rpmi_shm_register,
>> +       .shm_unregister = optee_rpmi_shm_unregister,
>> +};
>> +
>> +static const struct tee_driver_ops optee_rpmi_supp_ops = {
>> +       .get_version = optee_rpmi_get_version,
>> +       .get_tee_revision = optee_get_revision,
>> +       .open = optee_rpmi_open,
>> +       .release = optee_release_supp,
>> +       .supp_recv = optee_supp_recv,
>> +       .supp_send = optee_supp_send,
>> +       .shm_register = optee_rpmi_shm_register,
>> +       .shm_unregister = optee_rpmi_shm_unregister_supp,
>> +};
>> +
>> +static const struct tee_desc optee_rpmi_clnt_desc = {
>> +       .name = DRIVER_NAME "-rpmi-clnt",
>> +       .ops = &optee_rpmi_clnt_ops,
>> +       .owner = THIS_MODULE,
>> +};
>> +
>> +static const struct tee_desc optee_rpmi_supp_desc = {
>> +       .name = DRIVER_NAME "-rpmi-supp",
>> +       .ops = &optee_rpmi_supp_ops,
>> +       .owner = THIS_MODULE,
>> +       .flags = TEE_DESC_PRIVILEGED,
>> +};
>> +
>> +static const struct optee_ops optee_rpmi_ops = {
>> +       .do_call_with_arg = optee_rpmi_do_call_with_arg,
>> +       .to_msg_param = optee_rpmi_to_msg_param,
>> +       .from_msg_param = optee_rpmi_from_msg_param,
>> +};
>> +
>> +/* Keep callback state and memory tables alive until all TEE users release. */
>> +static void optee_rpmi_remove(struct rpmi_tee_device *rdev)
>> +{
>> +       struct optee *optee = dev_get_drvdata(&rdev->dev);
>> +
>> +       optee_remove_common(optee);
>> +       optee_rpmi_shm_rht_uninit(optee);
>> +       kfree(optee);
>> +}
>> +
>> +static int optee_rpmi_probe(struct rpmi_tee_device *rdev)
>> +{
>> +       struct tee_device *teedev;
>> +       struct tee_context *ctx;
>> +       int ret;
>> +
>> +       struct optee *optee __free(kfree) = kzalloc_obj(*optee);
> 
> The cleanup macros should, if I understand it correctly, not be used
> in functions using gotos for cleanup.

I'll remove all cleanup.h related macros as requested.

> 
>> +       if (!optee)
>> +               return -ENOMEM;
>> +
>> +       optee->rpmi.rdev = rdev;
>> +       optee->ops = &optee_rpmi_ops;
>> +
>> +       ret = optee_rpmi_api_is_compatible(optee);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = optee_rpmi_get_os_version(optee);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = optee_rpmi_exchange_caps(optee);
>> +       if (ret)
>> +               return ret;
>> +
>> +       optee->pool = optee_rpmi_shm_pool_alloc();
> 
> Perhaps it's just me, but it seems a bit odd to store an err pointer
> in a struct like this.
> 

Ack.

>> +       if (IS_ERR(optee->pool))
>> +               return PTR_ERR(optee->pool);
>> +
>> +       ret = optee_rpmi_shm_rht_init(optee);
>> +       if (ret)
>> +               goto err_pool;
>> +
>> +       optee_cq_init(&optee->call_queue, 0);
>> +       optee_supp_init(&optee->supp);
>> +       optee_shm_arg_cache_init(optee, OPTEE_SHM_ARG_SHARED);
>> +       mutex_init(&optee->rpmb_dev_mutex);
>> +       INIT_WORK(&optee->rpmb_scan_bus_work, optee_bus_scan_rpmb);
>> +       optee->rpmb_intf.notifier_call = optee_rpmb_intf_rdev;
>> +       ret = optee_notif_init(optee, optee->rpmi.notification_count);
>> +       if (ret)
>> +               goto err_common;
>> +
>> +       /* Allocate all keys, then restrict the inclusive bound to the last key. */
>> +       optee->notif.max_key = optee->rpmi.notification_count - 1;
> 
> Why? Do you have any plans for that?
> 

The comment is misleading; there is no additional restriction intended.
The ABI currently reports a key count, while the common notification code uses an
inclusive maximum key. I'll change the ABI to report the maximum key instead
and remove this adjustment.

Thanks Jens for the review.

Best Regards,
Amir

> Cheers,
> Jens
> 
>> +
>> +       teedev = tee_device_alloc(&optee_rpmi_clnt_desc, &rdev->dev,
>> +                                 optee->pool, optee);
>> +       if (IS_ERR(teedev)) {
>> +               ret = PTR_ERR(teedev);
>> +               goto err_notif;
>> +       }
>> +       optee->teedev = teedev;
>> +
>> +       teedev = tee_device_alloc(&optee_rpmi_supp_desc, &rdev->dev,
>> +                                 optee->pool, optee);
>> +       if (IS_ERR(teedev)) {
>> +               ret = PTR_ERR(teedev);
>> +               goto err_devices;
>> +       }
>> +       optee->supp_teedev = teedev;
>> +
>> +       optee_set_dev_group(optee);
>> +
>> +       /* Internal RPC allocation must be ready before userspace can enter. */
>> +       ctx = teedev_open(optee->teedev);
>> +       if (IS_ERR(ctx)) {
>> +               ret = PTR_ERR(ctx);
>> +               goto err_devices;
>> +       }
>> +
>> +       optee->ctx = ctx;
>> +       dev_set_drvdata(&rdev->dev, optee);
>> +       if (optee->in_kernel_rpmb_routing)
>> +               blocking_notifier_chain_register(&optee_rpmb_intf_added,
>> +                                                &optee->rpmb_intf);
>> +
>> +       ret = tee_device_register(optee->teedev);
>> +       if (ret)
>> +               goto err_initialized;
>> +
>> +       ret = tee_device_register(optee->supp_teedev);
>> +       if (ret)
>> +               goto err_initialized;
>> +
>> +       ret = optee_enumerate_devices(PTA_CMD_GET_DEVICES);
>> +       if (ret)
>> +               goto err_initialized;
>> +
>> +       dev_info(&rdev->dev, "OP-TEE RPMI %u.%u initialized\n",
>> +                optee->revision.os_major, optee->revision.os_minor);
>> +       retain_and_null_ptr(optee);
>> +
>> +       return 0;
>> +
>> +err_initialized:
>> +       /* The remove path owns and frees the published backend state. */
>> +       retain_and_null_ptr(optee);
>> +       optee_rpmi_remove(rdev);
>> +
>> +       return ret;
>> +err_devices:
>> +       tee_device_unregister(optee->supp_teedev);
>> +       tee_device_unregister(optee->teedev);
>> +       optee_shm_arg_cache_uninit(optee);
>> +err_notif:
>> +       optee_notif_uninit(optee);
>> +err_common:
>> +       optee_supp_uninit(&optee->supp);
>> +       mutex_destroy(&optee->call_queue.mutex);
>> +       rpmb_dev_put(optee->rpmb_dev);
>> +       mutex_destroy(&optee->rpmb_dev_mutex);
>> +       optee_rpmi_shm_rht_uninit(optee);
>> +err_pool:
>> +       tee_shm_pool_free(optee->pool);
>> +
>> +       return ret;
>> +}
>> +
>> +static const struct rpmi_tee_device_id optee_rpmi_device_ids[] = {
>> +       { OPTEE_RPMI_SERVICE_UUID },
>> +       {}
>> +};
>> +
>> +static struct rpmi_tee_driver optee_rpmi_driver = {
>> +       .name = DRIVER_NAME "-rpmi",
>> +       .probe = optee_rpmi_probe,
>> +       .remove = optee_rpmi_remove,
>> +       .id_table = optee_rpmi_device_ids,
>> +};
>> +
>> +int optee_rpmi_abi_register(void)
>> +{
>> +       return rpmi_tee_register(&optee_rpmi_driver);
>> +}
>> +
>> +void optee_rpmi_abi_unregister(void)
>> +{
>> +       rpmi_tee_unregister(&optee_rpmi_driver);
>> +}
>> +
>> +MODULE_ALIAS("rpmi_tee:486178e0-e7f8-11e3-bc5e-0002a5d5c51b");
>>
>> --
>> 2.34.1
>>