fix(csr): add support virtual interrupt for hvictl csr injection #457
Conversation
src/test/csrc/difftest/refproxy.h
Outdated
| @@ -144,6 +144,7 @@ class RefProxyConfig { | |||
| f(ref_skip_one, difftest_skip_one, void, bool, bool, uint32_t, uint64_t) \ | |||
| f(ref_guided_exec, difftest_guided_exec, void, void*) \ | |||
| f(raise_nmi_intr, difftest_raise_nmi_intr, void, bool) \ | |||
| f(ref_interrupt_injection, difftest_interrupt_injection, void, bool) \ | |||
There was a problem hiding this comment.
Is the name "interrupt injection" specifically used for this hvictl? This name looks like injecting interrupts to the REF to me, including the normal interrupts (by raise_interrupt)
There was a problem hiding this comment.
Yes, I'll modify the name for easier look.
|
Format check failed. |
src/test/csrc/difftest/difftest.cpp
Outdated
| @@ -398,6 +398,7 @@ int Difftest::step() { | |||
|
|
|||
| void Difftest::do_interrupt() { | |||
| state->record_interrupt(dut->event.exceptionPC, dut->event.exceptionInst, dut->event.interrupt); | |||
| proxy->hvictl_interrupt_injection(dut->event.intrInj); | |||
There was a problem hiding this comment.
Should we implement them as:
if (dut->event.intrInj) {
// call hvictl_...
}
else if (dut->event.hasNMI) {
// call nmi
}
else {
// call raise_intr
}The bool arguments seem useless (and may bring minor performance degradation)
src/main/scala/Bundles.scala
Outdated
| @@ -54,6 +54,7 @@ class ArchEvent extends DifftestBaseBundle with HasValid { | |||
| val exceptionPC = UInt(64.W) | |||
| val exceptionInst = UInt(32.W) | |||
| val hasNMI = Bool() | |||
| val intrInj = Bool() | |||
There was a problem hiding this comment.
This name also seems generally applicable for interrupts. I don't know much about hvictl but may we need a more specific name
sinceforYy
force-pushed
the
fix-vi-hvictl
branch
from
c8ac8ff
to
ddad4f0
Compare
src/test/csrc/difftest/difftest.cpp
Outdated
| } else if (dut->event.virtualInterruptIsHvictlInject) { | ||
| proxy->virtual_interrupt_is_hvictl_inject(dut->event.virtualInterruptIsHvictlInject); | ||
| } else { | ||
| printf("No NMI interrupt and virtual interrupt is not hvictl inject\n"); |
There was a problem hiding this comment.
Why is the printf here? For normal interrupts (should be the most common case), it will print this.
There was a problem hiding this comment.
The code looks good to me except for the printf. If there's the printf, there will be too many outputs because every normal interrupt (external interrupt, timer interrupt, software interrupt, ...) will cause a printf output.
I don't know much about the modern RISC-V interrupts. But I have a general concern on the implementation here. Previously without NMI/virtualization, there's only the normal interrupts and different interrupt sources (causes) can be determined by the interrupt cause number. Is there any similar approach to determine the NMI/virt interrupts? The current code adds a bool for each type of interrupt. But I'm thinking of a more general approach to cover all interrupt types to reduce the cosim overhead, including normal interrupts, NMI, virtual, .... I'm just raising this concern for discussions. I'm glad for approving and merging after the printf is removed.
sinceforYy
force-pushed
the
fix-vi-hvictl
branch
from
ddad4f0
to
1f2e878
Compare
No description provided.