Re: [PATCH v7 14/14] xen/riscv: Disable SSTC extension and add trap-based CSR probing

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.03.2026 17:33, Oleksii Kurochko wrote:
> Some RISC-V platforms expose the SSTC extension, but its CSRs are not
> properly saved and restored by Xen. Using SSTC in Xen could therefore
> lead to unexpected behaviour.

And what's wrong with (or what gets in the way of) adding proper
saving/restoring? Also, wouldn't a guest use vstimecmp anyway? I.e. what
saving/restoring are you talking about here?

> To avoid this in QEMU, disable SSTC by passing "sstc=off". On real
> hardware, OpenSBI does not provide a mechanism to disable SSTC via the
> DTS (riscv,isa or similar property), as it does not rely on that
> property to determine extension availability. Instead, it directly
> probes the CSR_STIMECMP register.
> 
> Introduce struct trap_info together with the do_expected_trap() handler
> to safely probe CSRs. The helper csr_read_allowed() attempts to read a
> CSR while catching traps, allowing Xen to detect whether the register
> is accessible. This mechanism is used at boot to verify SSTC support and
> panic if the CSR is not available.
> 
> The trap handling infrastructure may also be reused for other cases
> where controlled trap handling is required (e.g. probing instructions
> such as HLV*).

Hmm, won't you need a more generic way of dealing with traps anyway? See
Linux'es _ASM_EXTABLE(). See also comments further down.

> --- a/automation/scripts/qemu-smoke-riscv64.sh
> +++ b/automation/scripts/qemu-smoke-riscv64.sh
> @@ -7,7 +7,7 @@ rm -f smoke.serial
>  
>  export TEST_CMD="qemu-system-riscv64 \
>      -M virt,aia=aplic-imsic \
> -    -cpu rv64,svpbmt=on \
> +    -cpu rv64,svpbmt=on,sstc=off \
>      -smp 1 \
>      -nographic \
>      -m 2g \

How does this fit with you panic()ing when SSTC isn't available (i.e. the
register cannot be read)? I must be missing something, likely a result of
me not being able to really understand the description.

> --- a/xen/arch/riscv/cpufeature.c
> +++ b/xen/arch/riscv/cpufeature.c
> @@ -17,6 +17,8 @@
>  #include <xen/sections.h>
>  
>  #include <asm/cpufeature.h>
> +#include <asm/csr.h>
> +#include <asm/traps.h>
>  
>  #ifdef CONFIG_ACPI
>  # error "cpufeature.c functions should be updated to support ACPI"
> @@ -483,6 +485,7 @@ void __init riscv_fill_hwcap(void)
>      unsigned int i;
>      const size_t req_extns_amount = ARRAY_SIZE(required_extensions);
>      bool all_extns_available = true;
> +    struct trap_info trap;
>  
>      riscv_fill_hwcap_from_isa_string();
>  
> @@ -509,4 +512,9 @@ void __init riscv_fill_hwcap(void)
>      if ( !all_extns_available )
>          panic("Look why the extensions above are needed in "
>                
> "https://xenbits.xenproject.org/docs/unstable/misc/riscv/booting.txt\n";);
> +
> +    csr_read_allowed(CSR_STIMECMP, (unsigned long)&trap);

Please avoid such casts; see also below.

> --- a/xen/arch/riscv/entry.S
> +++ b/xen/arch/riscv/entry.S
> @@ -99,3 +99,27 @@ restore_registers:
>  
>          sret
>  END(handle_trap)
> +
> +        /*
> +         * We assume that the faulting instruction is 4 bytes long and 
> blindly
> +         * increment SEPC by 4.
> +         *
> +         * This should be safe because all places that may trigger this 
> handler
> +         * use ".option norvc" around the instruction that could cause the 
> trap,
> +         * or the instruction is not available in the RVC instruction set.
> +         *
> +         * do_expected_trap(a3, a4):
> +         *   a3 <- pointer to struct trap_info
> +         *   a4 <- temporary register
> +         */
> +FUNC(do_expected_trap)
> +        csrr    a4, CSR_SEPC
> +        REG_S   a4, RISCV_TRAP_SEPC(a3)
> +        csrr    a4, CSR_SCAUSE
> +        REG_S   a4, RISCV_TRAP_SCAUSE(a3)
> +
> +        csrr    a4, CSR_SEPC

Why read sepc a 2nd time? Yet further, what's the point of storing the value
in the first place? The sole present user doesn't care.

> --- a/xen/arch/riscv/include/asm/csr.h
> +++ b/xen/arch/riscv/include/asm/csr.h
> @@ -9,6 +9,7 @@
>  #include <asm/asm.h>
>  #include <xen/const.h>
>  #include <asm/riscv_encoding.h>
> +#include <asm/traps.h>
>  
>  #ifndef __ASSEMBLER__
>  
> @@ -78,6 +79,37 @@
>                             : "memory" );                        \
>  })
>  
> +/*
> + * Some functions inside asm/system.h requires some of the macros above,
> + * so this header should be included after the macros above are introduced.
> + */
> +#include <asm/system.h>
> +
> +#define csr_read_allowed(csr_num, trap) \
> +({ \
> +    register unsigned long tinfo asm("a3") = (unsigned long)trap; \

Why can't this variable be of the correct (pointer) type? This would then
at the same time serve as a compile-time check for the caller to have
passed an argument of the correct type.

> +    register unsigned long ttmp asm("a4"); \
> +    register unsigned long stvec = (unsigned long)&do_expected_trap; \

Fiddling with stvec may be okay-ish very early during boot. NMIs, for
example, do exist in principle on RISC-V, aiui. There must be a way for them
to be dealt with by other than just M-mode. 

> +    register unsigned long ret = 0; \
> +    unsigned long flags; \
> +    ((struct trap_info *)(trap))->scause = 0; \

"trap" would better be of the correct type. Don't use casts like this, please.

Further, wouldn't you better set the field to a guaranteed invalid value? 0 is
CAUSE_MISALIGNED_FETCH, after all.

> +    local_irq_save(flags); \
> +    asm volatile ( \
> +        ".option push\n" \
> +        ".option norvc\n" \

Shouldn't this come later?

> +        "add %[ttmp], %[tinfo], zero\n" \

Why "add", when you really mean "mv"? And why set ttmp in the first place, when
that's what do_expected_trap() writes to? Don't you really mean to specify "a4"
as a clobber?

> +        "csrrw %[stvec], " STR(CSR_STVEC) ", %[stvec]\n" \

The assembler does understand "stvec" as an operand, doesn't it?

> +        "csrr %[ret], %[csr]\n" \
> +        "csrw " STR(CSR_STVEC) ", %[stvec]\n" \
> +        ".option pop" \
> +        : [stvec] "+&r" (stvec), [tinfo] "+&r" (tinfo), \

tinfo isn't modified, is it?

> +          [ttmp] "+&r" (ttmp), [ret] "=&r" (ret) \

ttmp isn't initialized (in C), so the compiler could legitimately complain
about the use of an uninitialized variable here (due to the use of + where
= is meant).

Whereas for ret the situation is the other way around - you initialize the
variable, just to then tell the compiler that it can drop this
initialization, as - supposedly - the asm() always sets it (which it doesn't
when the csrr faults).

> +        : [csr] "i" (csr_num) \
> +        : "memory" ); \
> +    local_irq_restore(flags); \
> +    ret; \
> +})

A macro of this name would better return an indicator of what it is checking,
rather than the CSR value (which the sole user of this macro doesn't even
care about). Ideally such would also be an inline function.

Jan