OpenHW Group core cv32e40p privilege group tests failure due to mtvec not fully writable

[dd-vaibhavjain]

There are older tickets for this issue #149 #145

Creating this issue as a continuation of those tickets as I am not the author of those tickets to be able to reopen.

I see the README is updated as mentioned in one of these older tickets, and as per its requirements I see for the target DUT core of cv32e40p the test setup is meeting 1 of the 2 criteria listed in the README , (2nd criteria is met):
- i.) The target must have implemented mtvec which is completely writable by the test in machine mode.
-ii.) The target has initialized mtvec, before entering the test (via RVMODEL_BOOT), to point to a memory location which has both read and write permissions.

But still these tests continue to fail for this core.

Just to refresh the issue, that in this OpenHW Group's CV32E40P implementation the mtvec[31:8] are writable and [7:2] are Read-Only - 0s. Thus this core expects the Trap Handler address to be 256-Byte aligned.

And I notice that the current privilege tests try to workaround this issue by copying the original trap handler to the User Configured mtvec location if MTVEC is not fully writeable.
But this method implemented in these riscv-arch-test privilege tests still doesn't seem to work as expected at the moment.
After an exception the execution jumps to this user macro defined MTVEC location for trap handler ,where the trap handler was copied by the test and then test execution proceeds successfully from there with the copied over code, but this only works until this code runs into uninitialised memory locations which were never copied over and overwritten by the Test.

So this issue still prevents the tests to pass. And seems like a bug in the test code where this method of copying over doesn't fully work.
I noticed some tests passing for us (by chance) as the trap handler happened to be placed at 256 byte aligned location.

Maybe this issue can be resolved by updating this file:
riscof_arch_test_suite/riscv-test-suite/env/arch_test.h

---

.macro RVTEST_TRAP_HANDLER MODE
.option push
.option rvc // temporarily allow compress to allow c.nop alignment
.align 6 // ensure that a trampoline is on a typical cacheline boundary, just in case
.option pop

---

By changing this directive " .align 6" for trap handler "trampoline" to " .align 8" , this will resolve the issue in case of this core of cv32e40p. Or if possible we can make it parameterized, to provide a mechanism to overwrite this by the User could be helpful, that would also work without affecting anyother cores running this suite.

Otherwise if this solution is not acceptable probably tests need to be fixed so the intended mechanism of moving the trap handler to new location works.

[pawks]

@allenjbaum Would it be a useful thing to let the models declare a macro called MTVEC_ALIGN and control the alignment of the trap trampoline table? Incase it is not defined, we can assign a default of 6. I dont think there is any loss of coverage/testability in the general case where a trap handler needs to be instantiated.

[allenjbaum]

That might work in many cases, until some other restriction breaks it. If we (I) could push out a new spec that required that kind of thing, I’d feel a lot better about it. Why don’t you file a PR with that change and tell them to use it. Fixing all the bugs I wrote might also remove the problem, and there’s no reason we can’t do both.

…

On Monday, May 15, 2023, S Pawan Kumar ***@***.***> wrote: @allenjbaum <https://github.com/allenjbaum> Would it be a useful thing to let the models declare a macro called MTVEC_ALIGN and control the alignment of the trap trampoline table? Incase it is not defined, we can assign a default of 6. I dont think there is any loss of coverage/testability in the general case where a trap handler needs to be instantiated. — Reply to this email directly, view it on GitHub <#351 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPXVJTNJLGF3BI4M5NPVADXGII47ANCNFSM6AAAAAAX5ZW4L4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[MikeOpenHWGroup]

Thanks for looking into this @pawks and @allenjbaum. I am not clear on Allen's statement:

Why don’t you file a PR with that change and tell them to use it.

Who should file the PR (@pawks or @dd-vaibhavjain)?

[allenjbaum]

I wasn’t clear, was I. Open HW can file the PR, and whoever is running the tests tests should be told to set the variable

…

On Wednesday, May 17, 2023, Mike Thompson ***@***.***> wrote: Thanks for looking into this @pawks <https://github.com/pawks> and @allenjbaum <https://github.com/allenjbaum>. I am not clear on Allen's statement: Why don’t you file a PR with that change and tell them to use it. Who should file the PR ***@***.*** <https://github.com/pawks> or @dd-vaibhavjain <https://github.com/dd-vaibhavjain>)? — Reply to this email directly, view it on GitHub <#351 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPXVJTJAOCWWPTY3EP5PHTXGTDW3ANCNFSM6AAAAAAX5ZW4L4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[allenjbaum]

Thanks for looking into this.
I think I see the bug and it was a change that wasn't properly tested
I'll get a proper fix made and a proper test.

[dd-vaibhavjain]

Opened a PR #355 based on last week's discussion.
Since i had already started worked on this before Allen's last comment, so just went ahead with PR just in case it is still useful.

[dd-vaibhavjain]

Thanks to everyone for helping to close the PR for this issue. This provides a way for these failures to be resolve for us now.

Regarding this ticket, I believe @allenjbaum you mentioned about fixing the bug you see for this in test as well, so I can keep this ticket open while thats being done, or otherwise in case you prefer to close this ticket now while the other fix is being worked on, do let me know.

[allenjbaum]

I believe I fixed the bug; in any case, anyone that uses your fix shouldn't run into it, so you can close this.