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 <[email protected]> > >> --- > >> 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 > >>
