Trap and exit code includes non-RV32E compliant registers

Question

Trap and exit code includes non-RV32E compliant registers

Closed this issue a year ago · 9 comments

The trap code in arch_test.h contains t3, 4, and 5 which are registers x28-x30, and do not exist for RV32E architectures

RVTEST_CODE_END uses t4 in abort_tests

Also, the init register code uses RVTEST_E, but the gpr save register code uses RV32E

Answer 1 · 2023-05-01T19:04:00.000Z

I know about that - I thought it was mentioned in some other issue. I have a fix, but I don't know the ABI for an RV32E to nail down the details.

…

On Mon, May 1, 2023 at 11:08 AM ssecatchseagate ***@***.***> wrote: The trap code in arch_test.h contains t3, 4, and 5 which are registers x28-x30, and do not exist for RV32E architectures RVTEST_CODE_END uses t4 in abort_tests Also, the init register code uses RVTEST_E, but the gpr save register code uses *RV32E* — Reply to this email directly, view it on GitHub <#342>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPXVJTES4A5S2B7PRHOA23XD732TANCNFSM6AAAAAAXSAT6IQ> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

Answer 2 · 2023-05-20T18:04:18.000Z

Also sw x1,tohost,t5 in model_test.h not compatible for rv32e.

Answer 3 · 2023-05-20T18:31:56.000Z

not sure where you're getting model_test.h, but it is intended to be implementation specific. If someone is testing an RV32E, then they should provide a version that doesn't use t5.

…

On Sat, May 20, 2023 at 11:04 AM animesh211 ***@***.***> wrote: Also sw x1,tohost,t5 in model_test.h not compatible for rv32e. — Reply to this email directly, view it on GitHub <#342 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPXVJRO4YKNZZY4HIRJQ7LXHEBSZANCNFSM6AAAAAAXSAT6IQ> . You are receiving this because you commented.Message ID: ***@***.***>

Answer 4 · 2023-05-24T06:18:07.000Z

Ok, So Is there a version for model_test.h that is rv32e compatible?
I am trying RISCOF framework, and getting model_test.h generated in that.
It comes from here ->
model_test.h

Answer 5 · 2023-05-25T17:54:55.000Z

If you have an RV32E implementation, you are responsible for generating the model_test.h file, just as anyone that has an implementation of an version of the architecture is.

…

On Wednesday, May 24, 2023, animesh211 ***@***.***> wrote: Ok, So Is there a version for model_test.h that is rv32e compatible? I am trying RISCOF framework, and getting model_test.h generated in that. It comes from here -> model_test.h <https://github.com/riscv-software-src/riscof/blob/a25e3155d3a62a042a2c5d976b386e3df5874d70/riscof/Templates/setup/model/env/model_test.h#L17> — Reply to this email directly, view it on GitHub <#342 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPXVJVIIMQCVQ2ASCDMCD3XHWR2TANCNFSM6AAAAAAXSAT6IQ> . You are receiving this because you commented.Message ID: ***@***.***>

Answer 6 · 2023-05-25T18:31:59.000Z

But if there is an autogenerated template that is suggested to be adapted for use, shouldn't it be the lowest common denominator and be compliant to rv32e, no Zicsr, etc..? However, I think this requires a new issue for the riscof repo, rather than merging this into the request for riscv-arch-test, since as Allen points out, model_test.h is not provided by riscv-arch-test

Answer 7 · 2023-05-30T13:37:17.000Z

I was suggesting the same. Also asked for same here in riscof repo. - issue-85.
I think it'll be good if that one line in model_test.h has rv32e compliant register as the tests are present in repo.

Answer 8 · 2023-07-14T15:39:16.000Z

Thanks! I only have one concern about the maintenance and readability of this code:

#define T1 x6
...
LREG T1, code_bgn_off + 2*sv_area_sz(sp)

T1 looks really like the ABI register t1, and I can see where the code has all been changed to reflect this "local temporary", but I'm confused why a full self-documenting rename like "TEMP1" or "LTEMP1" or even something more specific like "ACT1" or "EXCEPTHAND1" wasn't used. T1 seems to imply t1, and it clearly shouldn't.

Additionally, it seems dangerous to be combining both ABI and gpr names for registers. You can see in that LREG code that the stack pointer register sp (x2) is used. But the redefinition for "T1" uses x6 (which in this case IS actually t1, but of course, is NOT true for t4, the original problem register). I would think our code would use one or the other, but mixing them could get us into trouble in the future.

Answer 9 · 2023-07-14T21:21:46.000Z

If there are tools that don't distinguish between upper and lower case names, then we have a problem. Otherwise we don't technically have a problem, but we will clean that up in the next rev. Since this is the trap handler, I think TRP1..6 would be slightly more descriptive than TEMP1..6

…

On Fri, Jul 14, 2023 at 8:39 AM ssecatchseagate ***@***.***> wrote: Thanks! I only have one concern about the maintenance and readability of this code: #define T1 x6 ... LREG T1, code_bgn_off + 2*sv_area_sz(sp) T1 looks really like the ABI register t1, and I can see where the code has all been changed to reflect this "local temporary", but I'm confused why a full self-documenting rename like "TEMP1" or "LTEMP1" or even something more specific like "ACT1" or "EXCEPTHAND1" wasn't used. T1 seems to imply t1, and it clearly shouldn't. Additionally, it seems dangerous to be combining both ABI and gpr names for registers. You can see in that LREG code that the stack pointer register sp (x2) is used. But the redefinition for "T1" uses x6 (which in this case IS actually t1, but of course, is NOT true for t4, the original problem register). I would think our code would use one or the other, but mixing them could get us into trouble in the future. — Reply to this email directly, view it on GitHub <#342 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPXVJWSCX27ZIMPP4X4HYTXQFR25ANCNFSM6AAAAAAXSAT6IQ> . You are receiving this because you commented.Message ID: ***@***.***>