Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1460238 > unrolled thread

Re: [PATCH 6/9] remoteproc: core: Add function to append a new resource table entry

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 6/9] remoteproc: core: Add function to append a new  resource table entry loic pallardy <loic.pallardy@st.com> - 2016-08-11 10:00 +0200
    Re: [PATCH 6/9] remoteproc: core: Add function to append a new  resource table entry Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-08-11 22:30 +0200
      Re: [PATCH 6/9] remoteproc: core: Add function to append a new  resource table entry loic pallardy <loic.pallardy@st.com> - 2016-08-12 14:00 +0200

#1460238 — Re: [PATCH 6/9] remoteproc: core: Add function to append a new resource table entry

Hi Lee,

I just tested your series and found issue with append mechanism.
There is no problem to add resources when working on Linux side, but the 
resource table is growing and when copying it at loaded location (ie 
overwriting existing prebuilt resource table of firmware), you have an 
overflow corrupting part of firmware code.

Moreover firmware code is in general tuned to a feature set. Resource 
table is created according to supported features. In most of the cases, 
new resource won't be handled by firmware.

Regards,
Loic


On 08/04/2016 11:21 AM, Lee Jones wrote:
> A new function now exists to pull in and amend and existing resource
> table entry.  But what if we wish to provide a new resource?  This
> function provides functionality to append a brand new resource entry
> onto the resource table.  All complexity related to shuffling parts
> of the table around, providing new offsets and incriminating number
> of entries in the resource table's top-level header is taken care of
> here.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>   drivers/remoteproc/remoteproc_core.c | 55 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 55 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 3318ebd..111350e 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -980,6 +980,61 @@ static int rproc_update_resource_table_entry(struct rproc *rproc,
>   	return !updated;
>   }
>
> +static struct resource_table*
> +rproc_add_resource_table_entry(struct rproc *rproc,
> +			       struct rproc_request_resource *request,
> +			       struct resource_table *old_table, int *tablesz)
> +{
> +	struct resource_table *table;
> +	struct fw_rsc_hdr h;
> +	void *new_rsc_loc;
> +	void *fw_header_loc;
> +	void *start_of_rscs;
> +	int new_rsc_offset;
> +	int size = *tablesz;
> +	int i;
> +
> +	h.type = request->type;
> +
> +	new_rsc_offset = size;
> +
> +	/*
> +	 * Allocate another contiguous chunk of memory, large enough to
> +	 * contain the new, expanded resource table.
> +	 *
> +	 * The +4 is for the extra offset[] element in the top level header
> +	 */
> +	size += sizeof(struct fw_rsc_hdr) + request->size + 4;
> +	table = devm_kmemdup(&rproc->dev, old_table, size, GFP_KERNEL);
> +	if (!table)
> +		return ERR_PTR(-ENOMEM);
> +
> +	/* Shunt table by 4 Bytes to account for the extra offset[] element */
> +	start_of_rscs = (void *)table + table->offset[0];
> +	memmove(start_of_rscs + 4,
> +		start_of_rscs, new_rsc_offset - table->offset[0]);
> +	new_rsc_offset += 4;
> +
> +	/* Update existing resource entry's offsets */
> +	for (i = 0; i < table->num; i++)
> +		table->offset[i] += 4;
> +
> +	/* Update the top level 'resource table' header */
> +	table->offset[table->num] = new_rsc_offset;
> +	table->num++;
> +
> +	/* Copy new firmware header into table */
> +	fw_header_loc = (void *)table + new_rsc_offset;
> +	memcpy(fw_header_loc, &h, sizeof(h));
> +
> +	/* Copy new resource entry into table */
> +	new_rsc_loc = (void *)fw_header_loc + sizeof(h);
> +	memcpy(new_rsc_loc, request->resource, request->size);
> +
> +	*tablesz = size;
> +	return table;
> +}
> +
>   /*
>    * take a firmware and boot a remote processor with it.
>    */
>

[toc] | [next] | [standalone]

#1460778

On Thu 11 Aug 00:51 PDT 2016, loic pallardy wrote:

> Hi Lee,
> 

Loic, please don't top-post.

> I just tested your series and found issue with append mechanism.
> There is no problem to add resources when working on Linux side, but the
> resource table is growing and when copying it at loaded location (ie
> overwriting existing prebuilt resource table of firmware), you have an
> overflow corrupting part of firmware code.
> 

Suman brought up the same concern. For one it shows that we must check
the size of the .resource_table to know if we can fit an expanded table
before installing it.

> Moreover firmware code is in general tuned to a feature set. Resource table
> is created according to supported features. In most of the cases, new
> resource won't be handled by firmware.
> 

For the case behind this implementation, where you have resource
information from e.g. DT and build up a resource table (to be installed)
from that, how would you deal with this? Would you build your firmware
with room for some amount of resources?



As my (not the maintainer-me) need for this is purely on the Linux side
I originally envisioned something where we during firmware load parse
the resource_table into some Linux side data structures; we would allow
for these to be merged with additional data or new ones added and Linux
would handle these.

At the end we would have modified the referenced resource_table (through
references as it isn't the primary data structure) and could copy this.

Or alternatively, in the case you described with an empty start and
resources only from DT, we could have a resource-table-installer that
would make up a resource table from these Linux-side lists of resources.


This path would solve the case that we would not automatically grow the
table with new resources, but for the case where we generate a resource
table at the end we would still have the same issues to conclude on.

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

#1461131

On 08/11/2016 10:20 PM, Bjorn Andersson wrote:
> On Thu 11 Aug 00:51 PDT 2016, loic pallardy wrote:
>
>> Hi Lee,
>>
>
> Loic, please don't top-post.

Hi Bjorn,

Thanks for the advice, I'll.

>
>> I just tested your series and found issue with append mechanism.
>> There is no problem to add resources when working on Linux side, but the
>> resource table is growing and when copying it at loaded location (ie
>> overwriting existing prebuilt resource table of firmware), you have an
>> overflow corrupting part of firmware code.
>>
>
> Suman brought up the same concern. For one it shows that we must check
> the size of the .resource_table to know if we can fit an expanded table
> before installing it.
>
Yes I saw Suman's reply just after my answer.

>> Moreover firmware code is in general tuned to a feature set. Resource table
>> is created according to supported features. In most of the cases, new
>> resource won't be handled by firmware.
>>
>
> For the case behind this implementation, where you have resource
> information from e.g. DT and build up a resource table (to be installed)
> from that, how would you deal with this? Would you build your firmware
> with room for some amount of resources?
>
In general host is filling existing resources defined in firmware 
resource table (using DT information).
Resource table is built according to firmware capabilities. No room with 
current firmware (current status on ST side).

>
>
> As my (not the maintainer-me) need for this is purely on the Linux side
> I originally envisioned something where we during firmware load parse
> the resource_table into some Linux side data structures; we would allow
> for these to be merged with additional data or new ones added and Linux
> would handle these.
>
Yes agree, working on this point to differentiate:
- resource we want to modify in firmware resource table,
- resource we want to handle on Linux side,
- resource we want to append is possible to existing resource table

> At the end we would have modified the referenced resource_table (through
> references as it isn't the primary data structure) and could copy this.

Copy is an issue if not spare bytes are reserved on firmware side.
Moreover firmware should be able to handle new resources on it side.
In general, resource table fits firmware capabilities. If a resource is 
not present, that means firmware won't be able to handle it.
(I'm speaking about basic firmware with limited feature set)

>
> Or alternatively, in the case you described with an empty start and
> resources only from DT, we could have a resource-table-installer that
> would make up a resource table from these Linux-side lists of resources.
>
I had long discussion with Lee about firmware resource table extension. 
Indeed we can create some "free" resource slots that host can 
dynamically fill, or a new resource type (like RSV_SPARE) providing 
information about spare bytes which can be used by host to append 
resource table.
Lee should work on a proposal...

Regards,
Loic
>
> This path would solve the case that we would not automatically grow the
> table with new resources, but for the case where we generate a resource
> table at the end we would still have the same issues to conclude on.
>
> Regards,
> Bjorn
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web