Skip to content
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

Illegal compressed instruction reports 2 traps. #770

Closed
mikaelsky opened this issue Jan 22, 2024 · 7 comments · Fixed by #771
Closed

Illegal compressed instruction reports 2 traps. #770

mikaelsky opened this issue Jan 22, 2024 · 7 comments · Fixed by #771
Assignees
Labels
bug Something isn't working as expected HW Hardware-related risc-v compliance Modification to comply with official RISC-V specs.

Comments

@mikaelsky
Copy link
Collaborator

Describe the bug
When issuing the following instruction generated by the riscvDV verification tool
28e0: 000066aa .word 0x000066aa
.4byte 0x66aa # kIllegalCompressedOpcode

From my directed testcase C file.
// Opcode
// 0000_0000_0000_0000_0110_0110_1010_1010
// 0110_0110_1010_1010 -> 011_ _ 10 -> seems to fall into c.lwsp but the func is 011 and not 010 so illegal
// 0000_0000_0000_0000 -> 000
_ _00 -> "ADD Imm * 4 + SP" -> addi X0, sp, 4*0
asm volatile (".word 0x000066aa");

The core issues 2 traps:
xcelium> run
VIRTUAL_UART: [Issuing Illegal Instruction]
VIRTUAL_UART: [Load Access Fault, 00000005]
VIRTUAL_UART: [Illegal instruction, 00000002]
VIRTUAL_UART: [Illegal Instruction issued]

From our imperas compare we get a different mcause error: (not the imperas decoder identifies this as an FLW instruction, which it is not.
Warning (RISCV_ILL) CPU 'refRoot/cpu' 0x000028e0 66aa flw f13,136(x2): Illegal instruction - Zcf absent
Info (IDV) Instruction executed prior to mismatch '0xf04a(mtvec_handler+146): 34202573 csrr x10,mcause'
Error (IDV) GPR register value mismatch (HartId:0, PC:0x0000f04a mtvec_handler+146):
Error (IDV) Mismatch 0> GPR x10
Error (IDV) . dut:0x00000004
Error (IDV) . ref:0x00000002
Error (IDV) tb.riscv_tb.idv_trace2api.state_compare @ 550660.00 ns: MISMATCH

Basically it seems that the core issues 2 traps for the one instruction. The actual trap is supposed to be "illegal instruction" but prior to that we also get an "Load Access Fault" or "Misaligned Address". Depending on the content of the registers prior to the instruction call.
This hints to the core decoder for compressed instructions doesn't correctly capture "0x66AA" as an illegal instruction

To Reproduce
I have a main.c test case that I will probably need to port to your system. The gist of it though is to install appropriate trap handlers and basically call the instruction in question per the code given above.
Let me take an action to upload a C test case for this one.

Expected behavior
The core is supposed to issue an illegal instruction trap.

Screenshots
NA

Environment:

  • RHEL7
  • GCC Version: riscv32-unknown-elf-gcc (g) 12.2.0
  • Libraries used: newlib

Hardware:

  • Hardware version (1.9.3.0)
  • Implemented CPU extensions: NA
  • Hardware modifications: RVVI trace an other feature enhancements

Additional context
This is part of the extended NEORV32 core validation where we are "punishing" the core with large sets of randomized instruction sequences. We then compare the NEORV32 with an Imperas core mode via the RVVI trace interface.

@mikaelsky
Copy link
Collaborator Author

Seems the fundamental issue is tied to the compressed instruction decompressor that falls into this "others" bucket.
Line 162:
"when others => -- "000": Illegal_instruction, C.ADDI4SPN; others: illegal"

@stnolting
Copy link
Owner

Hey @mikaelsky. Thank you so much for providing so much verification information! 👍

Basically it seems that the core issues 2 traps for the one instruction. The actual trap is supposed to be "illegal instruction" but prior to that we also get an "Load Access Fault" or "Misaligned Address". Depending on the content of the registers prior to the instruction call.

I think this is correct (at least from my point of understanding the RISC-V specs). You are feeding 0x000066aa to the core. These are two compressed instructions.

The first one (0x0000) comes from the "C0" quadrant and encodes the "official compressed illegal instruction":

grafik

So raising an illegal instruction exception here seems to be fine.

The second one (0x66aa) comes from the "C2" quadrant and encodes a FPU float load using the stack pointer as index register:

grafik

This is a legal instruction when the FPU is implemented. As the FPU implements the Zfinx ISA the "load-float" instruction gets transformed into a "load-integer" instruction. So the instruction seems valid. However, depending on the value inside the instruction's index register a bus error exception or misaligned address exception might be raised.

@stnolting stnolting self-assigned this Jan 22, 2024
@stnolting stnolting added the risc-v compliance Modification to comply with official RISC-V specs. label Jan 22, 2024
@stnolting
Copy link
Owner

Oh dear, I think I got the Zfinx ISA extension wrong... 🙈 I just had a look at the RISC-V / Zfinx spec again and this it what it says:

grafik

So, C.FLWSP, C.FSWSP, C.FLW and C.FSW should all raise an illegal instruction exception regardless of the Zfinx FPU being enabled or not, right?

@mikaelsky
Copy link
Collaborator Author

That would be my read as well. Basically for Zfinx any load/store and register moves are defined as illegal. This also explains why we see a load misalign/access fault erroneously.

TIL all 0's was illegal for the C extension. I thought it just mapped into the addi4spn... except I see the immediate is defined as a non-zero immediate.

Let me see if I can quickly port my test case from our "fancy" test environment to yours :)

@stnolting
Copy link
Owner

Seems like you've found a bug in the core's decompressor. Thank you very much for identifying this! I will fix that as soon as possible.

@stnolting stnolting added bug Something isn't working as expected HW Hardware-related labels Jan 22, 2024
mikaelsky added a commit to mikaelsky/neorv32 that referenced this issue Jan 23, 2024
Updated the decompressor decoder to not recognize compressed float load/stores as instructions and instead flag them as illegal instructions.
The fix passes our randomized instruction test.

Signed-off-by: Mikael Mortensen <119539842+mikaelsky@users.noreply.github.com>
@mikaelsky
Copy link
Collaborator Author

Bug fix pull request submitted. Passed our internal randomized instruction sim and my dedicated test.

@stnolting
Copy link
Owner

stnolting commented Jan 23, 2024

Passed our internal randomized instruction sim and my dedicated test.

🚀

Fix for issue: #770

Looks good! I'll copy that and open a PR over here.

edit

Basically, I have copied your changes. I just re-arranged some parts of the RTL code to save some LUTs (just about a dozen LUTs according to Quartus).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as expected HW Hardware-related risc-v compliance Modification to comply with official RISC-V specs.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants