----- On 22 May, 2020, at 20:03, Clément Leger cleger@kalray.eu wrote:

Hi Suman,

----- On 22 May, 2020, at 19:33, Bjorn Andersson bjorn.andersson@linaro.org
wrote:

quoted

On Fri 22 May 09:54 PDT 2020, Suman Anna wrote:

quoted

On 5/21/20 2:42 PM, Suman Anna wrote:

quoted

Hi Bjorn,

On 5/21/20 1:04 PM, Bjorn Andersson wrote:

quoted

On Wed 25 Mar 13:47 PDT 2020, Suman Anna wrote:

[..]

quoted

quoted

quoted

quoted

diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h

[..]

quoted

quoted

quoted

quoted

+struct fw_rsc_trace2 {

Sounds more like fw_rsc_trace64 to me - in particular since the version
of trace2 is 1...

Yeah, will rename this.

quoted

quoted

+    u32 padding;
+    u64 da;
+    u32 len;
+    u32 reserved;

What's the purpose of this reserved field?

Partly to make sure the entire resource is aligned on an 8-byte, and
partly copied over from fw_rsc_trace entry. I guess 32-bits is already
large enough of a size for trace entries irrespective of 32-bit or
64-bit traces, so I doubt if we want to make the len field also a u64.

Looking at this again, I can drop both padding and reserved fields, if I
move the len field before da. Any preferences/comments?

Sorry, my message went a bit too fast... So as I was saying:

Not only the in-structure alignment matters but also in the resource table.
Since the resource table is often packed (see [1] for instance), if a trace
resource is embedded in the resource table after another resource aligned
on 32 bits only, your 64 bits trace field will potentially end up
misaligned.

To overcome this, there is multiple solutions:

- Split the 64 bits fields into 32bits low and high parts:
Since all resources are aligned on 32bits, it will be ok

- Use memcpy_from/to_io when reading/writing such fields
As I said in a previous message this should probably be used since
the memories that are accessed by rproc are io mem (ioremap in almost
all drivers).

Regards,

Clément

[1]  https://github.com/OpenAMP/open-amp/blob/master/apps/machine/zynqmp_r5/rsc_table.h

quoted

quoted

Sounds good to me.

Thanks,
Bjorn