stnolting / neorv32

:desktop_computer: A small, customizable and extensible MCU-class 32-bit RISC-V soft-core CPU and microcontroller-like SoC written in platform-independent VHDL.
https://neorv32.org
BSD 3-Clause "New" or "Revised" License
1.58k stars 221 forks source link

Illegal compressed instruction reports 2 traps. #770

Closed mikaelsky closed 8 months ago

mikaelsky commented 8 months ago

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_10101010 -> 011 _ _10 -> seems to fall into c.lwsp but the func is 011 and not 010 so illegal // 0000_0000_00000000 -> 000 _ _00 -> "ADD Imm 4 + SP" -> addi X0, sp, 40 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:

Hardware:

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 commented 8 months ago

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 commented 8 months ago

Hey @mikaelsky. Thank you so much for providing so much verification information! :+1:

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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.

mikaelsky commented 8 months ago

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

stnolting commented 8 months ago

Passed our internal randomized instruction sim and my dedicated test.

🚀

Fix for issue: https://github.com/stnolting/neorv32/issues/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).