vsetvli yields illegal instruction exception

[gsauthof]

Executing a RISC-V program with Vector extension instructions yields a trap_illegal_instruction on vsetvli in Spike.

Setup:

• I've compiled Spike from the master branch (bb1cd8f, i.e. head commit from 2020-01-09). The README says that this version supports RISC-V Vector V 0.8 draft extension
• I've compiled pk with the current riscv/riscv-gnu-toolchain.git@rvv-0.8.x branch cross compiler

What does not work:

$ ~/local/riscv-spike/bin/spike --isa=RV64IMAFDCV \
    -d  ~/local/riscv-pk/riscv64-unknown-elf/bin/pk myvhello
: until pc 0 101b4
bbl loader
: 
core   0: 0x00000000000101b4 (0x01000813) li      a6, 16
: 
core   0: 0x00000000000101b8 (0x003872d7) vsetvli t0, a6, e8,m8
core   0: exception trap_illegal_instruction, epc 0x00000000000101b8
core   0:           tval 0x0000000000000000

Another test program from #358 fails at the same instruction:

$ ~/local/riscv-spike/bin/spike --isa=RV64IMAFDCV  \
    -d  ~/local/riscv-pk/riscv64-unknown-elf/bin/pk vtest
: until pc 0 1016e
bbl loader
: 
core   0: 0x000000000001016e (0x00b075d7) vsetvli a1, zero, e32,m8
core   0: exception trap_illegal_instruction, epc 0x000000000001016e
core   0:           tval 0x0000000000000000

Expected behavior: vsetvli is executed successfully

Cross-Check - What does work: executing a simple hello world program which yields the expected hello world output in the terminal.

[aswaterman]

I think the issue is that pk doesn't know about the vector extension so doesn't enable it. Can you see if this commit fixes the issue? riscv-software-src/riscv-pk@77a5df5

[gsauthof]

I've rebuilt pk with that commit and now an assertion is hit in pk:

$ ~/local/riscv-spike/bin/spike --isa=RV64IMAFDCV  \
         ~/local/riscv-pk/riscv64-unknown-elf/bin/pk vtest 
bbl loader
../machine/minit.c:74: assertion failed: read_csr(mstatus) & MSTATUS_FS
$ echo $?
0

If I disable the F extension in Spike the vsetvli triggers the illegal instruction trap, again:

$ ~/local/riscv-spike/bin/spike --isa=RV64IMACV -d  \
        ~/local/riscv-pk/riscv64-unknown-elf/bin/pk vtest
: until pc 0 1016e
bbl loader
: 
core   0: 0x000000000001016e (0x00b075d7) vsetvli a1, zero, e32,m8
core   0: exception trap_illegal_instruction, epc 0x000000000001016e
core   0:           tval 0x0000000000000000

[gsauthof]

So I've debugged this a bit and I noticed that the MSTATUS_VS bits get reset after the pk sets them in riscv-software-src/riscv-pk@77a5df5.

1. In pk, in the bootloader - my local fix for this issue:

diff --git a/machine/encoding.h b/machine/encoding.h
index 20c44b9..1953f56 100644
--- a/machine/encoding.h
+++ b/machine/encoding.h
@@ -33,6 +33,7 @@
 #define SSTATUS_UPIE        0x00000010
 #define SSTATUS_SPIE        0x00000020
 #define SSTATUS_SPP         0x00000100
+#define SSTATUS_VS          0x01800000
 #define SSTATUS_FS          0x00006000
 #define SSTATUS_XS          0x00018000
 #define SSTATUS_SUM         0x00040000
diff --git a/pk/pk.c b/pk/pk.c
index a2be365..bce11a5 100644
--- a/pk/pk.c
+++ b/pk/pk.c
@@ -187,7 +187,7 @@ void boot_loader(uintptr_t dtb)
   write_csr(stvec, &trap_entry);
   write_csr(sscratch, 0);
   write_csr(sie, 0);
-  set_csr(sstatus, SSTATUS_SUM | SSTATUS_FS);
+  set_csr(sstatus, SSTATUS_SUM | SSTATUS_FS | SSTATUS_VS);
 
   file_init();
   enter_supervisor_mode(rest_of_boot_loader, pk_vm_init(), 0);

2. In the pk trap handlers start_user - the thing is that trap_entry stores the sstatus register without the SSTATUS_VS bits (cf. save_tf macro in that file). This is because of the mask in processor_t::get_csr() misses the SSTATUS_VS bits.

My local fix for this Spike issue:

diff --git a/riscv/processor.cc b/riscv/processor.cc
index 0dd0abf..a2433d9 100644
--- a/riscv/processor.cc
+++ b/riscv/processor.cc
@@ -822,6 +822,7 @@ reg_t processor_t::get_csr(int which)
     case CSR_MCOUNTEREN: return state.mcounteren;
     case CSR_SSTATUS: {
       reg_t mask = SSTATUS_SIE | SSTATUS_SPIE | SSTATUS_SPP | SSTATUS_FS
+                 | (supports_extension('V') ? SSTATUS_VS : 0)
                  | SSTATUS_XS | SSTATUS_SUM | SSTATUS_MXR | SSTATUS_UXL;
       reg_t sstatus = state.mstatus & mask;
       if ((sstatus & SSTATUS_FS) == SSTATUS_FS ||

With that the example from #358 (named vtest in my comments) executes without illegal instruction exception.

However, I still have to use --isa=RV64IMACV to disable FD extensions because otherwise I still get:

../machine/minit.c:74: assertion failed: read_csr(mstatus) & MSTATUS_FS

My local fix for this:

--- a/machine/minit.c
+++ b/machine/minit.c
@@ -23,13 +23,15 @@ void* kernel_end;
 
 static void mstatus_init()
 {
+  unsigned long fresh_mstatus = 0;
   // Enable FPU
   if (supports_extension('F'))
-    write_csr(mstatus, MSTATUS_FS);
-
+    fresh_mstatus = MSTATUS_FS;
   // Enable vector extension
   if (supports_extension('V'))
-    write_csr(mstatus, MSTATUS_VS);
+    fresh_mstatus |= MSTATUS_VS;
+  if (fresh_mstatus)
+    write_csr(mstatus, fresh_mstatus);
 
   // Enable user/supervisor use of perf counters
   if (supports_extension('S'))

Regarding setting the status - I'm not sure if the flag in the pk boot_loader() function should be guarded by a feature check.

Also, I noticed that supports_extension('V') is already used elsewhere in Spike to guard SSTATUS_VS, but in contrast to that SSTATUS_FS isn't guarded with supports_extension('F').

[aswaterman]

As you can see, the VS feature is nascent and the kinks haven't all been worked out yet. Thanks for pointing out the Spike bug; it's been fixed. I've also adopted your proposed changes to pk. I'll leave the issue open until you confirm everything upstream appears to be working.

[gsauthof]

I've just pulled the latest Spike/pk changes and with that I can confirm that everything works as expected.