On Thu, May 30, 2024 at 09:42:26AM +0200, Arnaud POULIQUEN wrote:
> Hello Mathieu,
>
> On 5/29/24 22:35, Mathieu Poirier wrote:
> > On Wed, May 29, 2024 at 09:13:26AM +0200, Arnaud POULIQUEN wrote:
> >> Hello Mathieu,
> >>
> >> On 5/28/24 23:30, Mathieu Poirier wrote:
> >>> On Tue, May 21, 2024 at 10:09:59AM +0200, Arnaud Pouliquen wrote:
> >>>> 1) on start:
> >>>> - Using the TEE loader, the resource table is loaded by an external entity.
> >>>> In such case the resource table address is not find from the firmware but
> >>>> provided by the TEE remoteproc framework.
> >>>> Use the rproc_get_loaded_rsc_table instead of rproc_find_loaded_rsc_table
> >>>> - test that rproc->cached_table is not null before performing the memcpy
> >>>>
> >>>> 2)on stop
> >>>> The use of the cached_table seems mandatory:
> >>>> - during recovery sequence to have a snapshot of the resource table
> >>>> resources used,
> >>>> - on stop to allow for the deinitialization of resources after the
> >>>> the remote processor has been shutdown.
> >>>> However if the TEE interface is being used, we first need to unmap the
> >>>> table_ptr before setting it to rproc->cached_table.
> >>>> The update of rproc->table_ptr to rproc->cached_table is performed in
> >>>> tee_remoteproc.
> >>>>
> >>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@xxxxxxxxxxx>
> >>>> ---
> >>>> drivers/remoteproc/remoteproc_core.c | 31 +++++++++++++++++++++-------
> >>>> 1 file changed, 23 insertions(+), 8 deletions(-)
> >>>>
> >>>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> >>>> index 42bca01f3bde..3a642151c983 100644
> >>>> --- a/drivers/remoteproc/remoteproc_core.c
> >>>> +++ b/drivers/remoteproc/remoteproc_core.c
> >>>> @@ -1267,6 +1267,7 @@ EXPORT_SYMBOL(rproc_resource_cleanup);
> >>>> static int rproc_set_rsc_table_on_start(struct rproc *rproc, const struct firmware *fw)
> >>>> {
> >>>> struct resource_table *loaded_table;
> >>>> + struct device *dev = &rproc->dev;
> >>>>
> >>>> /*
> >>>> * The starting device has been given the rproc->cached_table as the
> >>>> @@ -1276,12 +1277,21 @@ static int rproc_set_rsc_table_on_start(struct rproc *rproc, const struct firmwa
> >>>> * this information to device memory. We also update the table_ptr so
> >>>> * that any subsequent changes will be applied to the loaded version.
> >>>> */
> >>>> - loaded_table = rproc_find_loaded_rsc_table(rproc, fw);
> >>>> - if (loaded_table) {
> >>>> - memcpy(loaded_table, rproc->cached_table, rproc->table_sz);
> >>>> - rproc->table_ptr = loaded_table;
> >>>> + if (rproc->tee_interface) {
> >>>> + loaded_table = rproc_get_loaded_rsc_table(rproc, &rproc->table_sz);
> >>>> + if (IS_ERR(loaded_table)) {
> >>>> + dev_err(dev, "can't get resource table\n");
> >>>> + return PTR_ERR(loaded_table);
> >>>> + }
> >>>> + } else {
> >>>> + loaded_table = rproc_find_loaded_rsc_table(rproc, fw);
> >>>> }
> >>>>
> >>>> + if (loaded_table && rproc->cached_table)
> >>>> + memcpy(loaded_table, rproc->cached_table, rproc->table_sz);
> >>>> +
> >>>
> >>> Why is this not part of the else {} above as it was the case before this patch?
> >>> And why was an extra check for ->cached_table added?
> >>
> >> Here we have to cover 2 use cases if rproc->tee_interface is set.
> >> 1) The remote processor is in stop state
> >> - loaded_table points to the resource table in the remote memory and
> >> - rproc->cached_table is null
> >> => no memcopy
> >> 2) crash recovery
> >> - loaded_table points to the resource table in the remote memory
> >> - rproc-cached_table point to a copy of the resource table
> >
> > A cached_table exists because it was created in rproc_reset_rsc_table_on_stop().
> > But as the comment says [1], that part of the code was meant to be used for the
> > attach()/detach() use case. Mixing both will become extremely confusing and
> > impossible to maintain.
>
> i am not sure to understand your point here... the cached_table table was
> already existing for the "normal" case[2]. Seems to me that the cache table is
> needed on stop in all scenarios.
>
> [2]
> https://elixir.bootlin.com/linux/v4.20.17/source/drivers/remoteproc/remoteproc_core.c#L1402
>
> >
> > I think the TEE scenario should be as similar as the "normal" one where TEE is
> > not involved. To that end, I suggest to create a cached_table in
> > tee_rproc_parse_fw(), exactly the same way it is done in
> > rproc_elf_load_rsc_table(). That way the code path in
> > rproc_set_rsc_table_on_start() become very similar and we have a cached_table to
> > work with when the remote processor is recovered. In fact we may not need
> > rproc_set_rsc_table_on_start() at all but that needs to be asserted.
>
> This is was I proposed in my V4 [3]. Could you please confirm that this aligns
> with what you have in mind?
After spending more time on this I have the following 3 observations:
1) We need a ->cached_table, otherwise the crash recovery path gets really
messy.
2) It _might_ be a good idea to rename tee_rproc_get_loaded_rsc_table() to
tee_rproc_find_loaded_rsc_table() to be aligned with the scenario where the
firmware is loaded by the remoteproc core. I think you had
tee_rproc_find_loaded_rsc_table() in the first place and I asked you to change
it. If so, apologies - reviewing patches isn't an exact science.
3) The same way ->cached_table is created in rproc_elf_load_rsc_table(), which
is essentially ops::parse_fw(), we should create one in tee_rproc_parse_fw()
with a kmemdup(). Exactly the same as in rproc_elf_load_rsc_table(). In
tee_rproc_parse_fw(), @rsc_table should be iounmap'ed right away so that we
don't need to keep a local variable to free it later. In rproc_start() the call
to rproc_find_loaded_rsc_table() will get another mapped handle to the resource
table in memory. It might be a little unefficient but it sure beats doing a lot
of modifications in the core.
As I said above this isn't an exact science and we may need to changes more
things but at least it should take us a little further.
Thanks,
Mathieu
> In such a case, should I keep the updates below in
> rproc_reset_rsc_table_on_stop(), or should I revert to using rproc->rsc_table to
> store the pointer to the resource table in tee_remoteproc for the associated
> memory map/unmap?"
>
> [3]
> https://patchwork.kernel.org/project/linux-remoteproc/patch/20240308144708.62362-2-arnaud.pouliquen@xxxxxxxxxxx/
>
> Thanks,
> Arnaud
>
> >
> > [1]. https://elixir.bootlin.com/linux/v6.10-rc1/source/drivers/remoteproc/remoteproc_core.c#L1565
> >
> >> => need to perform the memcpy to reapply settings in the resource table
> >>
> >> I can duplicate the memcpy in if{} and else{} but this will be similar code
> >> as needed in both case.
> >> Adding rproc->cached_table test if proc->tee_interface=NULL seems also
> >> reasonable as a memcpy from 0 should not be performed.
> >>
> >>
> >>>
> >>> This should be a simple change, i.e introduce an if {} else {} block to take
> >>> care of the two scenarios. Plus the comment is misplaced now.
> >>
> >> What about split it in 2 patches?
> >> - one adding the test on rproc->cached_table for the memcpy
> >> - one adding the if {} else {}?
> >>
> >> Thanks,
> >> Arnaud
> >>
> >>
> >>>
> >>> More comments tomorrow.
> >>>
> >>> Thanks,
> >>> Mathieu
> >>>
> >>>> + rproc->table_ptr = loaded_table;
> >>>> +
> >>>> return 0;
> >>>> }
> >>>>
> >>>> @@ -1318,11 +1328,16 @@ static int rproc_reset_rsc_table_on_stop(struct rproc *rproc)
> >>>> kfree(rproc->clean_table);
> >>>>
> >>>> out:
> >>>> - /*
> >>>> - * Use a copy of the resource table for the remainder of the
> >>>> - * shutdown process.
> >>>> + /* If the remoteproc_tee interface is used, then we have first to unmap the resource table
> >>>> + * before updating the proc->table_ptr reference.
> >>>> */
> >>>> - rproc->table_ptr = rproc->cached_table;
> >>>> + if (!rproc->tee_interface) {
> >>>> + /*
> >>>> + * Use a copy of the resource table for the remainder of the
> >>>> + * shutdown process.
> >>>> + */
> >>>> + rproc->table_ptr = rproc->cached_table;
> >>>> + }
> >>>> return 0;
> >>>> }
> >>>>
> >>>> --
> >>>> 2.25.1
> >>>>
