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