Re: [PATCH v7 0/9] target/riscv: Add support for Smdbltrp and Ssdbltrp extensions

[Clément Léger]

On 10/01/2025 03:12, Alistair Francis wrote:
> On Tue, Dec 17, 2024 at 7:10 PM Clément Léger <cleger@rivosinc.com> wrote:
>>
>> A double trap typically arises during a sensitive phase in trap handling
>> operations — when an exception or interrupt occurs while the trap
>> handler (the component responsible for managing these events) is in a
>> non-reentrant state. This non-reentrancy usually occurs in the early
>> phase of trap handling, wherein the trap handler has not yet preserved
>> the necessary state to handle and resume from the trap. The occurrence
>> of such event is unlikely but can happen when dealing with hardware
>> errors.
>>
>> This series adds support for Ssdbltrp and Smdbltrp ratified ISA
>> extensions [1]. It is based on the Smrnmi series [5].
>>
>> Ssdbltrp can be tested using qemu[2], opensbi (master branch), linux[3] and
>> kvm-unit-tests[4]. Assuming you have a riscv environment available and
>> configured (CROSS_COMPILE), it can be built for riscv64 using the
>> following instructions:
>>
>> Qemu:
>>   $ git clone https://github.com/rivosinc/qemu.git
>>   $ cd qemu
>>   $ git switch -C dbltrp_v6 dev/cleger/dbltrp_v6
>>   $ mkdir build && cd build
>>   $ ../configure --target-list=riscv64-softmmu
>>   $ make
>>
>> OpenSBI:
>>   $ git clone https://github.com/rivosinc/opensbi.git
>>   $ cd opensbi
>>   $ make O=build PLATFORM_RISCV_XLEN=64 PLATFORM=generic
>>
>> Linux:
>>   $ git clone https://github.com/rivosinc/linux.git
>>   $ cd linux
>>   $ git switch -C dbltrp_v1 dev/cleger/dbltrp_v1
>>   $ export ARCH=riscv
>>   $ make O=build defconfig
>>   $ ./script/config --file build/.config --enable RISCV_DBLTRP
>>   $ make O=build
>>
>> kvm-unit-tests:
>>   $ git clone https://github.com/clementleger/kvm-unit-tests.git
>>   $ cd kvm-unit-tests
>>   $ git switch -C dbltrp_v1 dev/cleger/dbltrp_v1
>>   $ ./configure --arch=riscv64 --cross-prefix=$CROSS_COMPILE
>>   $ make
>>
>> You will also need kvmtool in your rootfs.
>>
>> Run with kvm-unit-test test as kernel:
>>   $ qemu-system-riscv64 \
>>     -M virt \
>>     -cpu rv64,ssdbltrp=true,smdbltrp=true \
>>     -nographic \
>>     -serial mon:stdio \
>>     -bios opensbi/build/platform/generic/firmware/fw_jump.bin \
>>     -kernel kvm-unit-tests-dbltrp/riscv/sbi_dbltrp.flat
>>   ...
>>   [OpenSBI boot partially elided]
>>   Boot HART ISA Extensions  : 
>> sscofpmf,sstc,zicntr,zihpm,zicboz,zicbom,sdtrig,svadu,ssdbltrp
>>   ...
>>   ##########################################################################
>>   #    kvm-unit-tests
>>   ##########################################################################
>>
>>   PASS: sbi: fwft: FWFT extension probing no error
>>   PASS: sbi: fwft: FWFT extension is present
>>   PASS: sbi: fwft: dbltrp: Get double trap enable feature value
>>   PASS: sbi: fwft: dbltrp: Set double trap enable feature value == 0
>>   PASS: sbi: fwft: dbltrp: Get double trap enable feature value == 0
>>   PASS: sbi: fwft: dbltrp: Double trap disabled, trap first time ok
>>   PASS: sbi: fwft: dbltrp: Set double trap enable feature value == 1
>>   PASS: sbi: fwft: dbltrp: Get double trap enable feature value == 1
>>   PASS: sbi: fwft: dbltrp: Trapped twice allowed ok
>>   INFO: sbi: fwft: dbltrp: Should generate a double trap and crash !
>>
>>   sbi_trap_error: hart0: trap0: double trap handler failed (error -10)
>>
>>   sbi_trap_error: hart0: trap0: mcause=0x0000000000000010 
>> mtval=0x0000000000000000
>>   sbi_trap_error: hart0: trap0: mtval2=0x0000000000000003 
>> mtinst=0x0000000000000000
>>   sbi_trap_error: hart0: trap0: mepc=0x00000000802000d8 
>> mstatus=0x8000000a01006900
>>   sbi_trap_error: hart0: trap0: ra=0x00000000802001fc sp=0x0000000080213e70
>>   sbi_trap_error: hart0: trap0: gp=0x0000000000000000 tp=0x0000000080088000
>>   sbi_trap_error: hart0: trap0: s0=0x0000000080213e80 s1=0x0000000000000001
>>   sbi_trap_error: hart0: trap0: a0=0x0000000080213e80 a1=0x0000000080208193
>>   sbi_trap_error: hart0: trap0: a2=0x000000008020dc20 a3=0x000000000000000f
>>   sbi_trap_error: hart0: trap0: a4=0x0000000080210cd8 a5=0x00000000802110d0
>>   sbi_trap_error: hart0: trap0: a6=0x00000000802136e4 a7=0x0000000046574654
>>   sbi_trap_error: hart0: trap0: s2=0x0000000080210cd9 s3=0x0000000000000000
>>   sbi_trap_error: hart0: trap0: s4=0x0000000000000000 s5=0x0000000000000000
>>   sbi_trap_error: hart0: trap0: s6=0x0000000000000000 s7=0x0000000000000001
>>   sbi_trap_error: hart0: trap0: s8=0x0000000000002000 s9=0x0000000080083700
>>   sbi_trap_error: hart0: trap0: s10=0x0000000000000000 s11=0x0000000000000000
>>   sbi_trap_error: hart0: trap0: t0=0x0000000000000000 t1=0x0000000080213ed8
>>   sbi_trap_error: hart0: trap0: t2=0x0000000000001000 t3=0x0000000080213ee0
>>   sbi_trap_error: hart0: trap0: t4=0x0000000000000000 t5=0x000000008020f8d0
>>   sbi_trap_error: hart0: trap0: t6=0x0000000000000000
>>
>> Run with linux and kvm-unit-test test in kvm (testing VS-mode):
>>   $ qemu-system-riscv64 \
>>     -M virt \
>>     -cpu rv64,ssdbltrp=true,smdbltrp=true \
>>     -nographic \
>>     -serial mon:stdio \
>>     -bios opensbi/build/platform/generic/firmware/fw_jump.bin \
>>     -kernel linux/build/arch/riscv/boot/Image
>>   ...
>>   [Linux boot partially elided]
>>   [    0.735079] riscv-dbltrp: Double trap handling registered
>>   ...
>>
>>   $ lkvm run -k sbi_dbltrp.flat -m 128 -c 2
>>   ##########################################################################
>>   #    kvm-unit-tests
>>   ##########################################################################
>>
>>   PASS: sbi: fwft: FWFT extension probing no error
>>   PASS: sbi: fwft: FWFT extension is present
>>   PASS: sbi: fwft: dbltrp: Get double trap enable feature value
>>   PASS: sbi: fwft: dbltrp: Set double trap enable feature value == 0
>>   PASS: sbi: fwft: dbltrp: Get double trap enable feature value == 0
>>   PASS: sbi: fwft: dbltrp: Double trap disabled, trap first time ok
>>   PASS: sbi: fwft: dbltrp: Set double trap enable feature value == 1
>>   PASS: sbi: fwft: dbltrp: Get double trap enable feature value == 1
>>   PASS: sbi: fwft: dbltrp: Trapped twice allowed ok
>>   INFO: sbi: fwft: dbltrp: Should generate a double trap and crash !
>>   [   51.939077] Guest double trap
>>   [   51.939323] kvm [93]: VCPU exit error -95
>>   [   51.939683] kvm [93]: SEPC=0x802000d8 SSTATUS=0x200004520 
>> HSTATUS=0x200200180
>>   [   51.939947] kvm [93]: SCAUSE=0x10 STVAL=0x0 HTVAL=0x3 HTINST=0x0
>>   KVM_RUN failed: Operation not supported
>>   $
>>
>> Testing Smbdbltrp can be done using gdb and trigger some trap. For
>> instance, interrupt M-mode firmware at some point, set mstatus.mdt = 1
>> and corrupt some register to generate a NULL pointer exception.
>>
>> Link: 
>> https://github.com/riscv/riscv-isa-manual/commit/52a5742d5ab5a0792019033631b2035a493ad981
>>  [1]
>> Link: https://github.com/rivosinc/qemu/tree/dev/cleger/dbltrp_v7 [2]
>> Link: https://github.com/rivosinc/linux/tree/dev/cleger/dbltrp_v1 [3]
>> Link: 
>> https://github.com/clementleger/kvm-unit-tests/tree/dev/cleger/dbltrp_v1 [4]
>> Link: 
>> https://lore.kernel.org/qemu-riscv/20241217062440.884261-1-frank.chang@sifive.com/
>>  [5]
>>
>> ---
>>
>> V7:
>>  - Rebased on riscv-to-apply.next, on top of snrmi v10 series
> 
> Sorry for asking again, but can you do another rebase on
> https://github.com/alistair23/qemu/tree/riscv-to-apply.next ?
> 
> All these new extensions are conflicting a lot unfortunately, but at
> least snrmi has been merged :)

Ahah no worries, I'll do that !

Thanks,

Clément

> 
> Alistair
> 
>>
>> V6:
>>  - Simplify and fix write_henvcfg() masking by assigning the written
>>    value to henvcfg and mask the value to be written as well as clearing
>>    the upper part of henvcfgh upon writing.
>>  - Rebased on RNMI v9 series.
>>
>> V5:
>>  - Use 0 instead of false to set MSTATUS_MDT in helper_mnret()
>>  - Added explicit comments about henvcfg write mask being tricky.
>>  - Fixed a invalid menvcfg_mask in write_henvcfgh
>>
>> V4:
>>  - Remove DTE from sstatus_v1_10_mask variable and add specific if for
>>    DTE masking where it's used.
>>  - Use mstatus_hs.sdt field rather than setting DTE to 0 in
>>    riscv_do_cpu_interrupt().
>>  - Add a fix for henvcfg value which was incorrectly set after changing
>>    menvcfg
>>  - Remove useless ext_ssdbltrp check in
>>    riscv_env_smode_dbltrp_enabled().
>>  - Remove useless mstatus clear in write_mstatus().
>>  - Add proper handling of SDT writing to vsstatus.
>>  - Add clearing of vsstatus//mstatus SDT field when DTE is disabled.
>>  - Fix wrong value being written for MDT/MIE in write_mstatush().
>>  - Rebased on Frank Snrnmi v7
>>
>> V3:
>>  - Fix spec version from 1.12 to 1.13 for Smdbltrp and Ssdbltrp
>>  - Add better comments for dte/sdt computation in
>>    riscv_cpu_do_interrupt().
>>  - Move some CSR related changes to the CSRs related commits.
>>
>> V2:
>>  - Squashed commits that added ext_s{s|m}dbltrp as suggested by Daniel
>>
>> Clément Léger (9):
>>   target/riscv: Fix henvcfg potentially containing stale bits
>>   target/riscv: Add Ssdbltrp CSRs handling
>>   target/riscv: Implement Ssdbltrp sret, mret and mnret behavior
>>   target/riscv: Implement Ssdbltrp exception handling
>>   target/riscv: Add Ssdbltrp ISA extension enable switch
>>   target/riscv: Add Smdbltrp CSRs handling
>>   target/riscv: Implement Smdbltrp sret, mret and mnret behavior
>>   target/riscv: Implement Smdbltrp behavior
>>   target/riscv: Add Smdbltrp ISA extension enable switch
>>
>>  target/riscv/cpu.c        |   9 ++-
>>  target/riscv/cpu.h        |   1 +
>>  target/riscv/cpu_bits.h   |   8 +++
>>  target/riscv/cpu_cfg.h    |   2 +
>>  target/riscv/cpu_helper.c | 115 +++++++++++++++++++++++++++++++-------
>>  target/riscv/csr.c        |  95 ++++++++++++++++++++++++++-----
>>  target/riscv/op_helper.c  |  47 +++++++++++++++-
>>  7 files changed, 239 insertions(+), 38 deletions(-)
>>
>> --
>> 2.45.2
>>
>>