Re: [PATCH v5 5/7] remoteproc: core: support of the tee interface

[Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>]

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@foss.st.com>
>> ---
>>  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
     => 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
>>