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

[Alistair Francis]

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 :)

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
>
>