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?
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
>>>>
