| Message ID | 20240521081001.2989417-6-arnaud.pouliquen@foss.st.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduction of a remoteproc tee to load signed firmware | expand |
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? 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. 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 >
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 >>
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@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 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 > >>
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@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 > > 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@foss.st.com/ 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 >>>>
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@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 > > > > 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? Let me think a little - I'll get back to you. > 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@foss.st.com/ > > 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 > >>>>
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@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 > > > > 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@foss.st.com/ > > 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 > >>>>
Hello Mathieu, On 5/31/24 19:28, Mathieu Poirier wrote: > 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@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 >>> >>> 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. Remapping the resource table in rproc_find_loaded_rsc_table will require that we unmap it on rproc_stop before updating rproc->table_ptr to rproc->cached_table. On the other hand, I wonder if declaring the memory region in the stm32-rproc DT node would address this second mapping and avoid a map in rproc_find_loaded_rsc_table(). I will do the V6 integrating your suggestions and having a deeper look on the resource table map/unmap. > > 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. That seems to me reasonable and part of the normal upstream process :) Thanks, Arnaud > > 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@foss.st.com/ >> >> 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 >>>>>>
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); + + 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; }
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(-)