-
Notifications
You must be signed in to change notification settings - Fork 316
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
target/riscv: support log memory access128 #819
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @zqb-all, thank you for submittng this change.
My recommendation would be to implement this differently: extend log_memory_access()
function to accept 128-bit values instead of calling it multiple times with smaller values. That's because one 128-bit memory access is a different thing than four 32-bit accesses - and the log messages should clearly reflect that.
Note: I understand it can be tricky to extend log_memory_access()
to accept 128-bit inputs since support of 128-bit integer data types is not required by the C standard (1, 2). So you can create a second function log_memory_access128()
that would accept an array as an argument.
Also, how have you tested these changes?
Thanks,I will modify patch in this way
I have a riscv board with a cpu that supports sbaccess128,debug it with openocd and do some memory writes can trigger this assert |
3e10369
to
78ea7cf
Compare
log show like this:
|
I assume that function If you do not wish to do it in this merge request, that is OK. In that case please create an issue so that this "TODO" item is recorded. The changes made so far are already nice improvement over the current state. |
Change-Id: I6b22c97f81fac26703b66d3dbd8b6d41aaea4875 Signed-off-by: Mark Zhuang <mark.zhuang@spacemit.com>
read/write is system function Change-Id: I75db4dd5a1c60e9cff8a58a863a887beffc37cab Signed-off-by: Mark Zhuang <mark.zhuang@spacemit.com>
78ea7cf
to
dfce1d2
Compare
Yes, thank you for pointing that out
After this patch is merged, I will submit another patch to update read_memory_bus_v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zqb-all Thank you for the changes (and I'm sorry for my not so prompt reply).
It looks all right - approved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of renaming a bunch of trigger variables in a commit that's about logging 128-bit memory accesses.
But the code looks OK.
Would it make more sense to split it into two discrete PRs? |
Thanks, next time there is a similar change, I will split it into a separate PR |
|
log_memory_access only support size 1/2/4/8, let write_memory_bus_v1 call log_memory_access multiple times to fix size assert fail.
Change-Id: I6b22c97f81fac26703b66d3dbd8b6d41aaea4875