Fix read value of CSR mip.

[NewPaulWalker]

The read value of mip should not depend on the value of hvip.

hvip.VSSIP is an alias of mip.VSSIP, so when reading hvip, the VSSIP bit of mip needs to be OR-ed. Similarly, when writing to hvip, the VSSIP bit of mip also needs to be updated.

However, there is no reason for the value of mip to depend on the value of hvip when being read.

[demin-han]

#1590 add the hvip read.

[NewPaulWalker]

#1590 add the hvip read.

Yes, the read value of hvip should depend on the value of mip, but the read value of mip should not depend on the value of hvip.

[demin-han]

#1590 add the hvip read.

Yes, the read value of hvip should depend on the value of mip, but the read value of mip should not depend on the value of hvip.

without hvip read, the MIP_VSEIP and MIP_VSTIP will miss when reading mip.
These two bits are stored in hvip.

bool hvip_csr_t::unlogged_write(const reg_t val) noexcept {
state->mip->write_with_mask(MIP_VSSIP, val); // hvip.VSSIP is an alias of mip.VSSIP
const reg_t lscof_int = proc->extension_enabled(EXT_SSCOFPMF) ? MIP_LCOFIP : 0;
return basic_csr_t::unlogged_write(val & (lscof_int | MIP_VSEIP | MIP_VSTIP));
}

[NewPaulWalker]

without hvip read, the MIP_VSEIP and MIP_VSTIP will miss when reading mip.
These two bits are stored in hvip.

Thank you for the reminder!

Yes, the read value of mip should depend on hvip, but only on the hvip.VSEIP and hvip.VSTIP bits.

Previously, directly OR-ing the read value of mip with the read value of hvip was fine. However, after I modified the write behavior of hvip: #1862 — specifically, after implementing the Sscofpmf extension, hvip.LCOFIP became writable. This caused an issue where the entire value of hvip was OR-ed when reading mip.

I will revise the mip read logic to make it depend only on the hvip.VSEIP and hvip.VSTIP bits.
Thank you again for the reminder.

[JJ-Gaisler]

I have some questions about this. I mostly agree with what has changed but I don't quite get how an LCOF interrupt is ever to become pending through hvip. Spike currently reads mip to get knowledge of which interrupts are pending:

void take_pending_interrupt() { take_interrupt(state.mip->read() & state.mie->read()); }

If hvip has its own set of bits then mip.lcof must either be an alias of hvip.lcof or the interrupt pending logic has to also look at hvip.lcof. I am not sure if the spec is clear on what relation hvip.lcof has to mip and the other xIP CSRs.

In any case, mip_csr_t::ip_read() only allows VSEIP and VSTIP bits of hvip to be read in mip. If this is the case I can't see any way for the IRQ to actually happen?
I added MIP_LCOFIP to the read mask and that causes the IRQ to be taken.

Anything I am missing here?

edit: The interrupt logic also has to be updated for this to work.
The cause reported in vscause is always subtracted with 1 right now. Since LCOF is supposed to be reported as 13 in vs mode too the logic could be ajdusted to something like this:

diff --git a/riscv/processor.cc b/riscv/processor.cc                                                                                                                                                                                                                                                                         
index 26af4a81..aadd987c 100644                                                                                                                                                                                                                                                                                              
--- a/riscv/processor.cc                                                                                                                                                                                                                                                                                                     
+++ b/riscv/processor.cc  
@@ -495,9 +495,22 @@ void processor_t::take_trap(trap_t& t, reg_t epc)                                                                                                                                                                                                                                                       
-    reg_t adjusted_cause = interrupt ? bit - 1 : bit;  // VSSIP -> SSIP, etc                                                                                                                                                                                                                                                
+    int adjust_factor = 0;                                                                                                                                                                                                                                                                                                      
+    if (interrupt){                                                                                                                                                                                                                                                                                                         
+      switch (t.cause() & ~interrupt_bit){                                                                                                                                                                                                                                                                                  
+        // Reported as SEI when in VS mode                                                                                                                                                                                                                                                                                  
+        case IRQ_S_GEXT:                                                                                                                                                                                                                                                                                                    
+          adjust_factor = IRQ_S_GEXT - IRQ_S_EXT;                                                                                                                                                                                                                                                                           
+          break;                                                                                                                                                                                                                                                                                                            
+        // Reported as is in VS mode                                                                                                                                                                                                                                                                                        
+        case IRQ_LCOF:                                                                                                                                                                                                                                                                                                      
+          adjust_factor = 0;                                                                                                                                                                                                                                                                                                
+          break;                                                                                                                                                                                                                                                                                                            
+        default:                                                                                                                                                                                                                                                                                                            
+          adjust_factor = 1;                                                                                                                                                                                                                                                                                                
+      }                                                                                                                                                                                                                                                                                                                     
+    }                                                                                                                                                                                                                                                                                                                       
+    reg_t adjusted_cause = interrupt ? bit - adjust_factor : bit;  // VSSIP -> SSIP, etc 

[NewPaulWalker]

I have some questions about this. I mostly agree with what has changed but I don't quite get how an LCOF interrupt is ever to become pending through hvip. Spike currently reads mip to get knowledge of which interrupts are pending:

void take_pending_interrupt() { take_interrupt(state.mip->read() & state.mie->read()); }

If hvip has its own set of bits then mip.lcof must either be an alias of hvip.lcof or the interrupt pending logic has to also look at hvip.lcof. I am not sure if the spec is clear on what relation hvip.lcof has to mip and the other xIP CSRs.

Maybe I made some mistakes. This is what I thought at first:
Spike currently does not support the AIA extension, and hvien is a CSR introduced by the AIA extension, used for injecting interrupts numbered 13-63 into the VS-level, with LCOFI being exactly the 13th bit.
On the other hand, hvip is a CSR introduced by the H extension, which is only used for injecting VSSIP, VSTIP, and VSEIP interrupts into the VS-level.
Additionally, when the Sscofpmf extension is implemented, LCOF (bit 13) becomes writable. Thus, writing 1 to hvip.LCOFIP is allowed, but this injection will not succeed, and the interrupt will not be raised.
For now, before Spike implements the AIA extension, there is no need to modify the take_interrupt function, as all interrupts can be obtained by reading mip.

However, the manual states is:

This should mean that when only Sscofpmf is implemented but the AIA extension is not, the LCOFI bit of hvip/hvien remains non-writable, right?

edit: The interrupt logic also has to be updated for this to work.
The cause reported in vscause is always subtracted with 1 right now. Since LCOF is supposed to be reported as 13 in vs mode too the logic could be ajdusted to something like this:

Here, I don’t quite understand why SGEI is treated as SEI. Did I miss something in the spec manual?
Additionally, I believe the modification for LCOF is correct.

Also, could there be some issues with how interrupt priorities are determined in Spike?

However, the manual requires that the priority of LCOFIP is the lowest.