From: Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
To: Bjorn Andersson <andersson@kernel.org>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>,
<linux-kernel@vger.kernel.org>,
<linux-remoteproc@vger.kernel.org>
Subject: Re: [PATCH v15 2/8] remoteproc: Add TEE support
Date: Tue, 25 Mar 2025 12:05:12 +0100 [thread overview]
Message-ID: <a2c8655c-48e8-49e5-8460-175f73729f54@foss.st.com> (raw)
In-Reply-To: <6fufphs3ajlc7htj74qus6gifdd4yd64l5yjn2zyjrtdezoe4f@cqbbzg63acv4>
On 12/6/24 23:07, Bjorn Andersson wrote:
> On Thu, Nov 28, 2024 at 09:42:09AM GMT, Arnaud Pouliquen wrote:
>> Add a remoteproc TEE (Trusted Execution Environment) driver
>> that will be probed by the TEE bus. If the associated Trusted
>> application is supported on secure part this driver offers a client
>> interface to load a firmware by the secure part.
>
> If...else?
>
>> This firmware could be authenticated by the secure trusted application.
>>
>
> I would like for this to fully describe how this fits with the bus and
> how it is expected to be used by a specific remoteproc driver.
>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>> Updates vs version v13:
>> - define REMOTEPROC_TEE as bool instead of tristate,
>> - remove the load of the firmware in rproc_tee_parse_fw as we will ensure
>> that the firmware is loaded using the load_fw() operation.
>> ---
>> drivers/remoteproc/Kconfig | 10 +
>> drivers/remoteproc/Makefile | 1 +
>> drivers/remoteproc/remoteproc_tee.c | 508 ++++++++++++++++++++++++++++
>> include/linux/remoteproc.h | 4 +
>> include/linux/remoteproc_tee.h | 105 ++++++
>> 5 files changed, 628 insertions(+)
>> create mode 100644 drivers/remoteproc/remoteproc_tee.c
>> create mode 100644 include/linux/remoteproc_tee.h
[...]
>> +
>> +MODULE_DEVICE_TABLE(tee, rproc_tee_id_table);
>> +
>> +static struct tee_client_driver rproc_tee_fw_driver = {
>> + .id_table = rproc_tee_id_table,
>> + .driver = {
>> + .name = KBUILD_MODNAME,
>> + .bus = &tee_bus_type,
>> + .probe = rproc_tee_probe,
>> + .remove = rproc_tee_remove,
>> + },
>> +};
>> +
>> +static int __init rproc_tee_fw_mod_init(void)
>> +{
>> + return driver_register(&rproc_tee_fw_driver.driver);
>> +}
>> +
>> +static void __exit rproc_tee_fw_mod_exit(void)
>> +{
>> + driver_unregister(&rproc_tee_fw_driver.driver);
>> +}
>> +
>> +module_init(rproc_tee_fw_mod_init);
>> +module_exit(rproc_tee_fw_mod_exit);
>
> Please add an equivalent of the module_platform_driver() macro to tee
> framework instead of open-coding this.
>
It is not possible to use equivalent of the module_platform_driver() macro
as the device is on the TEE bus.
I followed recommendation provided in Documentation/driver-api/tee.rst[1]
Or do you have an alternative in mind?
Thanks,
Arnaud
[1]https://elixir.bootlin.com/linux/v6.14-rc6/source/Documentation/driver-api/tee.rst
>> +
>> +MODULE_DESCRIPTION(" remote processor TEE module");
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index 8fd0d7f63c8e..2e0ddcb2d792 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -503,6 +503,8 @@ enum rproc_features {
>> RPROC_MAX_FEATURES,
>> };
>>
>> +struct rproc_tee;
>> +
>> /**
>> * struct rproc - represents a physical remote processor device
>> * @node: list node of this rproc object
>> @@ -545,6 +547,7 @@ enum rproc_features {
>> * @cdev: character device of the rproc
>> * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
>> * @features: indicate remoteproc features
>> + * @rproc_tee_itf: pointer to the remoteproc tee context
>> */
>> struct rproc {
>> struct list_head node;
>> @@ -586,6 +589,7 @@ struct rproc {
>> struct cdev cdev;
>> bool cdev_put_on_release;
>> DECLARE_BITMAP(features, RPROC_MAX_FEATURES);
>> + struct rproc_tee *rproc_tee_itf;
>
> TEE is just one specific remoteproc implementation, why does it need to
> infest the core data structure? Do you want a stm32_rproc here as well?
>
>> };
>>
>> /**
>> diff --git a/include/linux/remoteproc_tee.h b/include/linux/remoteproc_tee.h
>> new file mode 100644
>> index 000000000000..9b498a8eff4d
>> --- /dev/null
>> +++ b/include/linux/remoteproc_tee.h
>> @@ -0,0 +1,105 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright(c) 2024 STMicroelectronics
>> + */
>> +
>> +#ifndef REMOTEPROC_TEE_H
>> +#define REMOTEPROC_TEE_H
>> +
>> +#include <linux/tee_drv.h>
>> +#include <linux/firmware.h>
>> +#include <linux/remoteproc.h>
>> +
>> +struct rproc;
>> +
>> +/**
>> + * struct rproc_tee - TEE remoteproc structure
>> + * @node: Reference in list
>> + * @rproc: Remoteproc reference
>> + * @parent: Parent device
>
> Isn't that rproc->dev->parent?
>
>> + * @rproc_id: Identifier of the target firmware
>> + * @session_id: TEE session identifier
>> + */
>> +struct rproc_tee {
>
> As far as I can tell this isn't dereferenced outside remoteproc_tee.c,
> can we hide it therein?
>
>> + struct list_head node;
>> + struct rproc *rproc;
>> + struct device *parent;
>> + u32 rproc_id;
>> + u32 session_id;
>> +};
>> +
>> +#if IS_REACHABLE(CONFIG_REMOTEPROC_TEE)
>> +
>> +int rproc_tee_register(struct device *dev, struct rproc *rproc, unsigned int rproc_id);
>> +int rproc_tee_unregister(struct rproc *rproc);
>> +int rproc_tee_parse_fw(struct rproc *rproc, const struct firmware *fw);
>> +int rproc_tee_load_fw(struct rproc *rproc, const struct firmware *fw);
>> +void rproc_tee_release_fw(struct rproc *rproc);
>> +struct resource_table *rproc_tee_find_loaded_rsc_table(struct rproc *rproc,
>> + const struct firmware *fw);
>> +int rproc_tee_start(struct rproc *rproc);
>> +int rproc_tee_stop(struct rproc *rproc);
>> +
>> +#else
>> +
>
> Do we really need yet another bunch of stubs? Can't we just leave
> CONFIG_REMOTEPROC_TEE non-user-selectable and have the drivers that rely
> on it do "select REMOTEPROC_TEE"?
>
> If my measurements are correct, it's 3.1kB of code...
>
> Regards,
> Bjorn
>
>> +static inline int rproc_tee_register(struct device *dev, struct rproc *rproc, unsigned int rproc_id)
>> +{
>> + return -ENODEV;
>> +}
>> +
>> +static inline int rproc_tee_parse_fw(struct rproc *rproc, const struct firmware *fw)
>> +{
>> + /* This shouldn't be possible */
>> + WARN_ON(1);
>> +
>> + return 0;
>> +}
>> +
>> +static inline int rproc_tee_unregister(struct rproc *rproc)
>> +{
>> + /* This shouldn't be possible */
>> + WARN_ON(1);
>> +
>> + return 0;
>> +}
>> +
>> +static inline int rproc_tee_load_fw(struct rproc *rproc, const struct firmware *fw)
>> +{
>> + /* This shouldn't be possible */
>> + WARN_ON(1);
>> +
>> + return 0;
>> +}
>> +
>> +static inline int rproc_tee_start(struct rproc *rproc)
>> +{
>> + /* This shouldn't be possible */
>> + WARN_ON(1);
>> +
>> + return 0;
>> +}
>> +
>> +static inline int rproc_tee_stop(struct rproc *rproc)
>> +{
>> + /* This shouldn't be possible */
>> + WARN_ON(1);
>> +
>> + return 0;
>> +}
>> +
>> +static inline void rproc_tee_release_fw(struct rproc *rproc)
>> +{
>> + /* This shouldn't be possible */
>> + WARN_ON(1);
>> +}
>> +
>> +static inline struct resource_table *
>> +rproc_tee_find_loaded_rsc_table(struct rproc *rproc, const struct firmware *fw)
>> +{
>> + /* This shouldn't be possible */
>> + WARN_ON(1);
>> +
>> + return NULL;
>> +}
>> +#endif /* CONFIG_REMOTEPROC_TEE */
>> +#endif /* REMOTEPROC_TEE_H */
>> --
>> 2.25.1
>>
>
next prev parent reply other threads:[~2025-03-25 11:07 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-11-28 8:42 [PATCH v15 0/8] Introduction of a remoteproc tee to load signed firmware Arnaud Pouliquen
2024-11-28 8:42 ` [PATCH v15 1/8] remoteproc: core: Introduce rproc_pa_to_va helper Arnaud Pouliquen
2024-11-28 8:42 ` [PATCH v15 2/8] remoteproc: Add TEE support Arnaud Pouliquen
2024-12-03 17:04 ` Mathieu Poirier
2024-12-06 22:07 ` Bjorn Andersson
2024-12-10 8:57 ` Arnaud POULIQUEN
2025-01-10 8:51 ` Arnaud POULIQUEN
2025-02-12 3:18 ` Bjorn Andersson
2025-02-12 13:42 ` Arnaud POULIQUEN
2025-03-04 15:58 ` Bjorn Andersson
2025-03-05 12:48 ` Arnaud POULIQUEN
2025-03-25 11:05 ` Arnaud POULIQUEN [this message]
2024-11-28 8:42 ` [PATCH v15 3/8] remoteproc: Introduce load_fw and release_fw optional operation Arnaud Pouliquen
2024-12-03 17:22 ` Mathieu Poirier
2024-12-05 18:20 ` Arnaud POULIQUEN
2024-12-06 17:05 ` Mathieu Poirier
2024-12-06 17:07 ` Mathieu Poirier
2024-12-06 18:09 ` Arnaud POULIQUEN
2024-12-04 17:39 ` Mathieu Poirier
2024-12-09 23:14 ` Bjorn Andersson
2024-12-10 10:33 ` Arnaud POULIQUEN
2025-02-12 3:54 ` Bjorn Andersson
2025-02-12 13:48 ` Arnaud POULIQUEN
2025-03-04 15:23 ` Bjorn Andersson
2025-03-05 12:50 ` Arnaud POULIQUEN
2024-11-28 8:42 ` [PATCH v15 4/8] remoteproc: Rename load() operation to load_segments() in rproc_ops struct Arnaud Pouliquen
2024-12-04 18:02 ` Mathieu Poirier
2024-11-28 8:42 ` [PATCH v15 5/8] remoteproc: Make load_segments and load_fw ops exclusive and optional Arnaud Pouliquen
2024-12-04 17:53 ` Mathieu Poirier
2024-11-28 8:42 ` [PATCH v15 6/8] dt-bindings: remoteproc: Add compatibility for TEE support Arnaud Pouliquen
2024-11-28 8:42 ` [PATCH v15 7/8] remoteproc: stm32: Create sub-functions to request shutdown and release Arnaud Pouliquen
2024-11-28 8:42 ` [PATCH v15 8/8] remoteproc: stm32: Add support of an OP-TEE TA to load the firmware Arnaud Pouliquen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a2c8655c-48e8-49e5-8460-175f73729f54@foss.st.com \
--to=arnaud.pouliquen@foss.st.com \
--cc=andersson@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-remoteproc@vger.kernel.org \
--cc=mathieu.poirier@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).