SpinalHDL / NaxRiscv

MIT License
269 stars 40 forks source link

[BUG] CSR write verification error in the Verilator wrapper #78

Open Bill94l opened 10 months ago

Bill94l commented 10 months ago

Hi Charles,

Following on from my previous issue https://github.com/SpinalHDL/NaxRiscv/issues/70#issuecomment-1910747595, where I had an error with the double and float tests using RVLS.

I didn't understand why these tests didn't work. They are used in NaxRiscvRegression.scala and run without errors.

I finally found out what the problem was by comparing main.cpp and hart.cpp from RVLS.

https://github.com/SpinalHDL/NaxRiscv/blob/3fa251104e4bc3cb5fda8e784838fd94200875d6/src/test/cpp/naxriscv/src/main.cpp#L2186

hart.cpp

         if((inst & 0x7F) == 0x73 && (inst & 0x3000) != 0){

In an if statement that compares the CSR addresses (for write) between DUT and Spike. The if statement in main.cpp needs some parentheses and CSR writing is never checked when using NaxRiscvRegression.

By adding the parentheses in main.cpp, we have the same error as RVLS. (A different address when writing in PMP csr)

./obj_dir/VNaxRiscv --load-elf /media/big_dd/nanotrust/bi262934/naxriscv_tuleap/test-jenkins-tmp/NaxRiscv/ext/NaxSoftware/riscv-tests/rv32uf-p-fmadd --start-symbol _start --pass-symbol pass --fail-symbol fail --trace-ref --spike-debug --timeout 10000 

*** CSR WRITE ADDRESS DUT=3 REF=2 ***

TIME=768
LAST PC COMMIT=80000170
INCOMING SPIKE PC=ffffffff80000174
ROB_ID=x28
FAILURE ???

Best regards

Dolu1990 commented 9 months ago

Hoo damned, thanks ^^ missed those parenthesis

So, the issue is more that Spike does things by writing to frm while fcsr is accessed (shadow register) I think this specific missmatch should be ignored, so for hardware CSR access to address 3, SPIKE access to 1 and 2 are ok, For the data, the remapping could be done via this table : image

Sound good ?

Best regards Charles

Bill94l commented 9 months ago

I ignored the missmatch by making a few changes to "hart.cpp" and adding a patch to spike "riscv/csrs.cc" to log the write operation in the FCSR register rather than in the FRM and FFLAGS registers.

hart.cpp

if((inst & 0x7F) == 0x73 && (inst & 0x3000) != 0){
    if((inst >> 20)==CSR_FCSR || (inst >> 20)==CSR_FRM || (inst >> 20)==CSR_FFLAGS){
        if(rd!=CSR_MSTATUS){
            assertTrue("CSR WRITE MISSING", csrWrite);
            assertEq("CSR WRITE ADDRESS", (u32)(csrAddress & 0xCFF), (u32)(rd & 0xCFF));
         }
         break;
     }
     assertTrue("CSR WRITE MISSING", csrWrite);
     assertEq("CSR WRITE ADDRESS", (u32)(csrAddress & 0xCFF), (u32)(rd & 0xCFF));
 }
break;

csrs.cc

Subject: [PATCH] Patch logging FCSR write register
===================================================================

diff --git a/riscv/csrs.cc b/riscv/csrs.cc
--- a/riscv/csrs.cc (revision 020ad5ac424c3c1ba4ed8e77458e2fec084b8cf6)
+++ b/riscv/csrs.cc (date 1706715209703)
@@ -1180,9 +1180,9 @@
 }

 bool composite_csr_t::unlogged_write(const reg_t val) noexcept {
-  upper_csr->write(val >> upper_lsb);
-  lower_csr->write(val);
-  return false;  // logging is done only by the underlying CSRs
+  upper_csr->unlogged_write(val >> upper_lsb);
+  lower_csr->unlogged_write(val);
+  return true;  // logging is done only by the underlying CSRs
 }

Currently the 32bit float and double tests work perfectly, but I have a MISSMATCH PC for 64bit float and double tests.

LOG (rv64ud-p-ldst)

sbt "runMain naxriscv.platform.tilelinkdemo.SocSim --xlen 64 --nax-count 1 --withRvc --withFloat --withDouble --dual-sim ${ARGS} --start-symbol _start --pass-symbol pass --start-add 0 --load-elf NaxRiscv/ext/NaxSoftware/riscv-tests/rv64ud-p-ldst"

[info] Sim starting <3
[info] PC COMMIT dut=80000000 ref=80000000
[info] PC COMMIT dut=8000004c ref=8000004c
[info] PC COMMIT dut=80000050 ref=80000050
[info] PC COMMIT dut=80000054 ref=80000054
[info] PC COMMIT dut=80000058 ref=80000058
[info] PC COMMIT dut=8000005c ref=8000005c
[info] PC COMMIT dut=80000060 ref=80000060
[info] PC COMMIT dut=80000064 ref=80000064
[info] PC COMMIT dut=80000068 ref=80000068
[info] PC COMMIT dut=8000006c ref=8000006c
[info] PC COMMIT dut=80000070 ref=80000070
[info] PC COMMIT dut=80000074 ref=80000074
[info] PC COMMIT dut=80000078 ref=80000078
[info] PC COMMIT dut=8000007c ref=8000007c
[info] PC COMMIT dut=80000080 ref=80000080
[info] PC COMMIT dut=80000084 ref=80000084
[info] PC COMMIT dut=80000088 ref=80000088
[info] PC COMMIT dut=8000008c ref=8000008c
[info] PC COMMIT dut=80000090 ref=80000090
[info] PC COMMIT dut=80000094 ref=80000094
[info] PC COMMIT dut=80000098 ref=80000098
[info] PC COMMIT dut=8000009c ref=8000009c
[info] PC COMMIT dut=800000a0 ref=800000a0
[info] PC COMMIT dut=800000a4 ref=800000a4
[info] PC COMMIT dut=800000a8 ref=800000a8
[info] PC COMMIT dut=800000ac ref=800000ac
[info] PC COMMIT dut=800000b0 ref=800000b0
[info] PC COMMIT dut=800000b4 ref=800000b4
[info] PC COMMIT dut=800000b8 ref=800000b8
[info] PC COMMIT dut=800000bc ref=800000bc
[info] PC COMMIT dut=800000c0 ref=800000c0
[info] PC COMMIT dut=800000c4 ref=800000c4
[info] PC COMMIT dut=800000c8 ref=800000c8
[info] PC COMMIT dut=800000cc ref=800000cc
[info] PC COMMIT dut=800000d0 ref=800000d0
[info] PC COMMIT dut=800000d4 ref=800000d4
[info] PC COMMIT dut=800000d8 ref=800000d8
[info] PC COMMIT dut=800000dc ref=800000dc
[info] PC COMMIT dut=800000e0 ref=800000e0
[info] PC COMMIT dut=800000e4 ref=800000e4
[info] PC COMMIT dut=800000e8 ref=800000e8
[info] PC COMMIT dut=800000ec ref=800000ec
[info] PC COMMIT dut=800000f0 ref=800000f0
[info] PC COMMIT dut=800000f4 ref=800000f4
[info] PC COMMIT dut=80000104 ref=ffffffff80000104
[info] PC COMMIT dut=80000104 ref=ffffffff80000104
[info] PC MISSMATCH dut=80000104 ref=ffffffff80000104
[info] commit error
[info] - std::exception

LOG (rv32ud-p-ldst)

sbt "runMain naxriscv.platform.tilelinkdemo.SocSim --xlen 32 --nax-count 1 --withRvc --withFloat --withDouble --dual-sim ${ARGS} --start-symbol _start --pass-symbol pass --start-add 0 --load-elf NaxRiscv/ext/NaxSoftware/riscv-tests/rv32ud-p-ldst"


[info] Sim starting <3
[info] PC COMMIT dut=ffffffff80000000 ref=ffffffff80000000
[info] PC COMMIT dut=ffffffff8000004c ref=ffffffff8000004c
[info] PC COMMIT dut=ffffffff80000050 ref=ffffffff80000050
[info] PC COMMIT dut=ffffffff80000054 ref=ffffffff80000054
[info] PC COMMIT dut=ffffffff80000058 ref=ffffffff80000058
[info] PC COMMIT dut=ffffffff8000005c ref=ffffffff8000005c
[info] PC COMMIT dut=ffffffff80000060 ref=ffffffff80000060
[info] PC COMMIT dut=ffffffff80000064 ref=ffffffff80000064
[info] PC COMMIT dut=ffffffff80000068 ref=ffffffff80000068
[info] PC COMMIT dut=ffffffff8000006c ref=ffffffff8000006c
[info] PC COMMIT dut=ffffffff80000070 ref=ffffffff80000070
[info] PC COMMIT dut=ffffffff80000074 ref=ffffffff80000074
[info] PC COMMIT dut=ffffffff80000078 ref=ffffffff80000078
[info] PC COMMIT dut=ffffffff8000007c ref=ffffffff8000007c
[info] PC COMMIT dut=ffffffff80000080 ref=ffffffff80000080
[info] PC COMMIT dut=ffffffff80000084 ref=ffffffff80000084
[info] PC COMMIT dut=ffffffff80000088 ref=ffffffff80000088
[info] PC COMMIT dut=ffffffff8000008c ref=ffffffff8000008c
[info] PC COMMIT dut=ffffffff80000090 ref=ffffffff80000090
[info] PC COMMIT dut=ffffffff80000094 ref=ffffffff80000094
[info] PC COMMIT dut=ffffffff80000098 ref=ffffffff80000098
[info] PC COMMIT dut=ffffffff8000009c ref=ffffffff8000009c
[info] PC COMMIT dut=ffffffff800000a0 ref=ffffffff800000a0
[info] PC COMMIT dut=ffffffff800000a4 ref=ffffffff800000a4
[info] PC COMMIT dut=ffffffff800000a8 ref=ffffffff800000a8
[info] PC COMMIT dut=ffffffff800000ac ref=ffffffff800000ac
[info] PC COMMIT dut=ffffffff800000b0 ref=ffffffff800000b0
[info] PC COMMIT dut=ffffffff800000b4 ref=ffffffff800000b4
[info] PC COMMIT dut=ffffffff800000b8 ref=ffffffff800000b8
[info] PC COMMIT dut=ffffffff800000bc ref=ffffffff800000bc
[info] PC COMMIT dut=ffffffff800000c0 ref=ffffffff800000c0
[info] PC COMMIT dut=ffffffff800000c4 ref=ffffffff800000c4
[info] PC COMMIT dut=ffffffff800000c8 ref=ffffffff800000c8
[info] PC COMMIT dut=ffffffff800000cc ref=ffffffff800000cc
[info] PC COMMIT dut=ffffffff800000d0 ref=ffffffff800000d0
[info] PC COMMIT dut=ffffffff800000d4 ref=ffffffff800000d4
[info] PC COMMIT dut=ffffffff800000d8 ref=ffffffff800000d8
[info] PC COMMIT dut=ffffffff800000dc ref=ffffffff800000dc
[info] PC COMMIT dut=ffffffff800000e0 ref=ffffffff800000e0
[info] PC COMMIT dut=ffffffff800000e4 ref=ffffffff800000e4
[info] PC COMMIT dut=ffffffff800000e8 ref=ffffffff800000e8
[info] PC COMMIT dut=ffffffff800000ec ref=ffffffff800000ec
[info] PC COMMIT dut=ffffffff800000f0 ref=ffffffff800000f0
[info] PC COMMIT dut=ffffffff80000100 ref=ffffffff80000100
[info] PC COMMIT dut=ffffffff80000104 ref=ffffffff80000104
[info] PC COMMIT dut=ffffffff80000108 ref=ffffffff80000108
[info] PC COMMIT dut=ffffffff8000010c ref=ffffffff8000010c
[info] PC COMMIT dut=ffffffff80000110 ref=ffffffff80000110
[info] PC COMMIT dut=ffffffff80000114 ref=ffffffff80000114
[info] PC COMMIT dut=ffffffff80000118 ref=ffffffff80000118
[info] PC COMMIT dut=ffffffff8000011c ref=ffffffff8000011c
[info] PC COMMIT dut=ffffffff80000120 ref=ffffffff80000120
[info] PC COMMIT dut=ffffffff80000124 ref=ffffffff80000124
[info] PC COMMIT dut=ffffffff80000128 ref=ffffffff80000128
[info] PC COMMIT dut=ffffffff8000012c ref=ffffffff8000012c
[info] PC COMMIT dut=ffffffff80000130 ref=ffffffff80000130
[info] PC COMMIT dut=ffffffff80000148 ref=ffffffff80000148
[info] PC COMMIT dut=ffffffff8000014c ref=ffffffff8000014c
[info] PC COMMIT dut=ffffffff80000150 ref=ffffffff80000150
[info] PC COMMIT dut=ffffffff80000164 ref=ffffffff80000164
[info] PC COMMIT dut=ffffffff80000168 ref=ffffffff80000168
[info] PC COMMIT dut=ffffffff8000016c ref=ffffffff8000016c
[info] PC COMMIT dut=ffffffff80000170 ref=ffffffff80000170
[info] PC COMMIT dut=ffffffff80000174 ref=ffffffff80000174
[info] PC COMMIT dut=ffffffff80000178 ref=ffffffff80000178
[info] PC COMMIT dut=ffffffff8000017c ref=ffffffff8000017c
[info] PC COMMIT dut=ffffffff80000180 ref=ffffffff80000180
[info] PC COMMIT dut=ffffffff80000184 ref=ffffffff80000184
[info] PC COMMIT dut=ffffffff80000188 ref=ffffffff80000188
[info] PC COMMIT dut=ffffffff8000018c ref=ffffffff8000018c
[info] PC COMMIT dut=ffffffff80000190 ref=ffffffff80000190
[info] PC COMMIT dut=ffffffff80000194 ref=ffffffff80000194
[info] PC COMMIT dut=ffffffff80000198 ref=ffffffff80000198
[info] PC COMMIT dut=ffffffff8000019c ref=ffffffff8000019c
[info] PC COMMIT dut=ffffffff800001a0 ref=ffffffff800001a0
[info] PC COMMIT dut=ffffffff800001a4 ref=ffffffff800001a4
[info] PC COMMIT dut=ffffffff800001a8 ref=ffffffff800001a8
[info] PC COMMIT dut=ffffffff800001ac ref=ffffffff800001ac
[info] PC COMMIT dut=ffffffff800001b0 ref=fffffHart 0
[info] - i$ refill = 11
[info] - d$ refill = 2
[info] TEST PASS
Dolu1990 commented 9 months ago

Hi,

I think your pc missmatch is because the risc-v test is trying to access the PMP, which isn't implemented. 80000100: 3a029073 csrw pmpcfg0,t0

Probably a simple workaround would be to implement the pmp in naxriscv as hardwired to zero ?

Bill94l commented 9 months ago

Hi,

The code execution log using NaxRiscvRegression shows an exception during PMP initialization, but the code is executed successfully.

hart.cpp

void Hart::physExtends(u64 &v){
    v = (u64)(((s64)v<<(64-physWidth)) >> (64-physWidth));
}

Executing the code with SocSim also presents an exception during the initialization of the PMP which is normal, but in "hart.cpp" during the exception the "physExtends" method triggers an extension of the sign which gives the PC missmatch. Why use physExtends?

I made the following changes to avoid PC mismatches :

void Hart::physExtends(u64 &v){
    auto xlen = proc->get_xlen();
    if(xlen == 32){
        v = (u64)(((s64)v<<(64-physWidth)) >> (64-physWidth));
    }
    else{
        v = (s64)v;
    }
}

But normally it does not depend on xlen, because physWidth is a constant, which is equal to 32 for (xlen = 32 or 64) ?

Thank you !

Dolu1990 commented 9 months ago

Ahhh those address sign extends are realy kinda hellish XD

Why use physExtends?

I think the idea was to force spike to behave with a limited number of bits on the PC. The missmatch you get should be handled on the PC check assertion, by only checking the 32 bits LSB when RV32 is used i would say, instead of doing a full 64 bits comparison ?