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

Oleksii Kurochko posted 14 patches 1 month, 1 week ago
[PATCH v7 14/14] xen/riscv: Disable SSTC extension and add trap-based CSR probing
Posted by Oleksii Kurochko 1 month, 1 week ago
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.

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*).

Also reorder header inclusion in asm-offsets.c to follow Xen coding
style: <xen/types.h> should be included before <asm/*> headers as there
is no any specific reason to remain the current order.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v7:
 - new patch.
   IMO, it is okay to have this patch separetely as at the moment it won't be
   an issue if Xen will use CSR_STIMECMP to setup its timer. The issue will
   start to occur when a guest will run.
---
 automation/scripts/qemu-smoke-riscv64.sh |  2 +-
 xen/arch/riscv/cpufeature.c              |  8 ++++++
 xen/arch/riscv/entry.S                   | 24 ++++++++++++++++++
 xen/arch/riscv/include/asm/csr.h         | 32 ++++++++++++++++++++++++
 xen/arch/riscv/include/asm/traps.h       |  7 ++++++
 xen/arch/riscv/riscv64/asm-offsets.c     |  7 +++++-
 6 files changed, 78 insertions(+), 2 deletions(-)

diff --git a/automation/scripts/qemu-smoke-riscv64.sh b/automation/scripts/qemu-smoke-riscv64.sh
index c0b1082a08fd..1909abb7af32 100755
--- 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 \
diff --git a/xen/arch/riscv/cpufeature.c b/xen/arch/riscv/cpufeature.c
index 03e27b037be0..987d36dc7eee 100644
--- 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);
+
+    if ( !trap.scause )
+        panic("SSTC isn't supported\n");
 }
diff --git a/xen/arch/riscv/entry.S b/xen/arch/riscv/entry.S
index 202a35fb03a8..b434948da3a4 100644
--- 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
+        addi    a4, a4, 4
+        csrw    CSR_SEPC, a4
+        sret
+END(do_expected_trap)
diff --git a/xen/arch/riscv/include/asm/csr.h b/xen/arch/riscv/include/asm/csr.h
index 01876f828981..b318cbdf35c3 100644
--- 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; \
+    register unsigned long ttmp asm("a4"); \
+    register unsigned long stvec = (unsigned long)&do_expected_trap; \
+    register unsigned long ret = 0; \
+    unsigned long flags; \
+    ((struct trap_info *)(trap))->scause = 0; \
+    local_irq_save(flags); \
+    asm volatile ( \
+        ".option push\n" \
+        ".option norvc\n" \
+        "add %[ttmp], %[tinfo], zero\n" \
+        "csrrw %[stvec], " STR(CSR_STVEC) ", %[stvec]\n" \
+        "csrr %[ret], %[csr]\n" \
+        "csrw " STR(CSR_STVEC) ", %[stvec]\n" \
+        ".option pop" \
+        : [stvec] "+&r" (stvec), [tinfo] "+&r" (tinfo), \
+          [ttmp] "+&r" (ttmp), [ret] "=&r" (ret) \
+        : [csr] "i" (csr_num) \
+        : "memory" ); \
+    local_irq_restore(flags); \
+    ret; \
+})
+
 #endif /* __ASSEMBLER__ */
 
 #endif /* ASM__RISCV__CSR_H */
diff --git a/xen/arch/riscv/include/asm/traps.h b/xen/arch/riscv/include/asm/traps.h
index 21fa3c3259b3..194d9a72f3ed 100644
--- a/xen/arch/riscv/include/asm/traps.h
+++ b/xen/arch/riscv/include/asm/traps.h
@@ -7,10 +7,17 @@
 
 #ifndef __ASSEMBLER__
 
+struct trap_info {
+    register_t sepc;
+    register_t scause;
+};
+
 void do_trap(struct cpu_user_regs *cpu_regs);
 void handle_trap(void);
 void trap_init(void);
 
+void do_expected_trap(void);
+
 #endif /* __ASSEMBLER__ */
 
 #endif /* ASM__RISCV__TRAPS_H */
diff --git a/xen/arch/riscv/riscv64/asm-offsets.c b/xen/arch/riscv/riscv64/asm-offsets.c
index 472cced4f8af..b0e2a4d86bd3 100644
--- a/xen/arch/riscv/riscv64/asm-offsets.c
+++ b/xen/arch/riscv/riscv64/asm-offsets.c
@@ -1,8 +1,10 @@
 #define COMPILE_OFFSETS
 
+#include <xen/types.h>
+
 #include <asm/current.h>
 #include <asm/processor.h>
-#include <xen/types.h>
+#include <asm/traps.h>
 
 #define DEFINE(_sym, _val)                                                 \
     asm volatile ( "\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\""\
@@ -54,4 +56,7 @@ void asm_offsets(void)
     BLANK();
     DEFINE(PCPU_INFO_SIZE, sizeof(struct pcpu_info));
     BLANK();
+    OFFSET(RISCV_TRAP_SEPC, struct trap_info, sepc);
+    OFFSET(RISCV_TRAP_SCAUSE, struct trap_info, scause);
+    BLANK();
 }
-- 
2.53.0
Re: [PATCH v7 14/14] xen/riscv: Disable SSTC extension and add trap-based CSR probing
Posted by Jan Beulich 1 month ago
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
Re: [PATCH v7 14/14] xen/riscv: Disable SSTC extension and add trap-based CSR probing
Posted by Oleksii Kurochko 1 month ago
On 3/10/26 10:15 AM, Jan Beulich wrote:
> 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.

At the moment this approach works for me and I haven't had a need for more
generic approach. I will look at _ASM_EXTABLE(). I haven't checked yet but
I assume it will require some extra fixup code in trap handler what looks
like over complication for the current case, at least.

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

When SSTC isn't available my panic() won't occur and then will continue to
be executed. Otherwise, when SSTC is enabled (it is enabled by QEMU by default)
my panic will occur.

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

Because a4 was changed, so it should be re-read, but we can setup CSR_SEPC before a4
being changed.


> Yet further, what's the point of storing the value
> in the first place? The sole present user doesn't care.

I needed that initially for debug. And also it would be useful for trap redirection
for example, but it isn't a case now. So for now I can drop that.

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

Good point it could be an option.

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

Do I understand correct that your concern is about that if NMIs will be handled
in HS-mode that switching stvec in this way could be dangerous as do_expected_trap()
doesn't know how to handle NMIs?

If yes, then NMIs should be handled by M-mode as:
   Non-maskable interrupts (NMIs) are only used for hardware error conditions, and
   cause an immediate jump to an implementation-defined NMI vector running in M-mode
   regardless of the state of a hart’s interrupt enable bits
and:
   The non-maskable interrupt is not made visible via the mip register as its
   presence is implicitly known when executing the NMI trap handler.

So standard delegation registers like mideleg do not apply to NMIs because NMIs
are not visible in the mip register.

I haven't found in OpenSBI how they are explicitly handling NMIs, but it looks
like if they happen in (H)S-mode or (V)U-mode then they will be just redirected
to (H)S-mode or V(U)-mode:
   https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_trap.c#L361
And then do_expected_trap() will fail to handle them...

Interesting that other hypervisors are using the similar approarch (with temporary
updating of stvec) and they haven't faced such issue with NMIs yet...

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

I don't see that such an invalid value exist for scause. I think we have to reserved
a value from region 24-31 or 48-63 as they are designated for custom use.


>
>> +    local_irq_save(flags); \
>> +    asm volatile ( \
>> +        ".option push\n" \
>> +        ".option norvc\n" \
> Shouldn't this come later?

Do you mean before where SSTC csr is really tried to be read ("csrr %[ret], %[csr]\n")?
Does it really matter in such small inline assembler?

>
>> +        "add %[ttmp], %[tinfo], zero\n" \
> Why "add", when you really mean "mv"?

I think it could be "mv".

> And why set ttmp in the first place, when
> that's what do_expected_trap() writes to?

To force the compiler to materialize tinfo in register a4 (ttmp) before the
trap handler runs as handler will use a4 as temporary register.

>   Don't you really mean to specify "a4"
> as a clobber?

Good point. It makes sense. Likely it can updated to:
   ...
   mv a4, %[tinfo] ... : ... : ... : "memory", "a4"

>
>> +        "csrrw %[stvec], " STR(CSR_STVEC) ", %[stvec]\n" \
> The assembler does understand "stvec" as an operand, doesn't it?

I haven't tried... I'll check that.

>
>> +        "csrr %[ret], %[csr]\n" \
>> +        "csrw " STR(CSR_STVEC) ", %[stvec]\n" \
>> +        ".option pop" \
>> +        : [stvec] "+&r" (stvec), [tinfo] "+&r" (tinfo), \
> tinfo isn't modified, is it?

It is modified by handler.

>
>> +          [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).

ttmp is modified by handler too.

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

It was done in that way as when csrr will lead to a fault, handler will jump
over the csrr instruction and so ret won't be set at all. For that case it was
set to 0.

>
>> +        : [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).

With the current one use case it doesn't care but generally I think that someone
will want to use this macro just to get CSR value. I don't have a speicifc example
but still it could be used in this way.

> Ideally such would also be an inline function.

I thought about that but I had difficulties with csr* instruction and their second
operand which expects to have immediate. But if I will have inline function that
csr_num will be in register.

Thanks.

~ Oleksii


Re: [PATCH v7 14/14] xen/riscv: Disable SSTC extension and add trap-based CSR probing
Posted by Jan Beulich 1 month ago
On 11.03.2026 10:54, Oleksii Kurochko wrote:
> On 3/10/26 10:15 AM, Jan Beulich wrote:
>> On 06.03.2026 17:33, Oleksii Kurochko wrote:
>>> --- 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.
> 
> When SSTC isn't available my panic() won't occur and then will continue to
> be executed. Otherwise, when SSTC is enabled (it is enabled by QEMU by default)
> my panic will occur.

Oh, I notice I misread the condition around the panic(), mainly because of
the misleading / ambiguous message passed to it: "SSTC isn't supported\n"
can mean unsupported by Xen or unsupported by the platform.

Anyway, to me this is entirely bogus: Why would we panic() because there is
a certain extension available?

>>> --- 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.
> 
> Good point it could be an option.
> 
>>> +    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.
> 
> Do I understand correct that your concern is about that if NMIs will be handled
> in HS-mode that switching stvec in this way could be dangerous as do_expected_trap()
> doesn't know how to handle NMIs?

Yes.

> If yes, then NMIs should be handled by M-mode as:
>    Non-maskable interrupts (NMIs) are only used for hardware error conditions, and
>    cause an immediate jump to an implementation-defined NMI vector running in M-mode
>    regardless of the state of a hart’s interrupt enable bits
> and:
>    The non-maskable interrupt is not made visible via the mip register as its
>    presence is implicitly known when executing the NMI trap handler.
> 
> So standard delegation registers like mideleg do not apply to NMIs because NMIs
> are not visible in the mip register.
> 
> I haven't found in OpenSBI how they are explicitly handling NMIs, but it looks
> like if they happen in (H)S-mode or (V)U-mode then they will be just redirected
> to (H)S-mode or V(U)-mode:
>    https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_trap.c#L361
> And then do_expected_trap() will fail to handle them...
> 
> Interesting that other hypervisors are using the similar approarch (with temporary
> updating of stvec) and they haven't faced such issue with NMIs yet...

Well, NMIs may be rare to occur? And hence very unlikely to occur in this small
a window?

>>> +    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.
> 
> I don't see that such an invalid value exist for scause. I think we have to reserved
> a value from region 24-31 or 48-63 as they are designated for custom use.

Not sure that's possible. "Custom use" may mean "custom" from hw perspective.
I was rather thinking of picking something pretty high in the reserved range,
like (1 << (MXLEN-1)) - 1 or 1 << (MXLEN-2).

>>> +    local_irq_save(flags); \
>>> +    asm volatile ( \
>>> +        ".option push\n" \
>>> +        ".option norvc\n" \
>> Shouldn't this come later?
> 
> Do you mean before where SSTC csr is really tried to be read ("csrr %[ret], %[csr]\n")?

Yes.

> Does it really matter in such small inline assembler?

Yes, if nothing else then to not raise questions. Plus (depending on the
specific operands used), the ADD (MV) could e.g. be representable by a C insn.

>> And why set ttmp in the first place, when
>> that's what do_expected_trap() writes to?
> 
> To force the compiler to materialize tinfo in register a4 (ttmp) before the
> trap handler runs as handler will use a4 as temporary register.

??? I don't understand what you mean with "materialize".

>>> +        "csrr %[ret], %[csr]\n" \
>>> +        "csrw " STR(CSR_STVEC) ", %[stvec]\n" \
>>> +        ".option pop" \
>>> +        : [stvec] "+&r" (stvec), [tinfo] "+&r" (tinfo), \
>> tinfo isn't modified, is it?
> 
> It is modified by handler.

Where? It's only used as the address of the two stores.

>>> +          [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).
> 
> ttmp is modified by handler too.

Of course, but just to repeat - you mean "=&r" there.

>> 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).
> 
> It was done in that way as when csrr will lead to a fault, handler will jump
> over the csrr instruction and so ret won't be set at all. For that case it was
> set to 0.

And again - this is meaningless if the constraint is "=&r".

>>> +        : [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).
> 
> With the current one use case it doesn't care but generally I think that someone
> will want to use this macro just to get CSR value. I don't have a speicifc example
> but still it could be used in this way.

Well, if you want to keep it doing so, make the name match what it does (and
in particular what it returns).

>> Ideally such would also be an inline function.
> 
> I thought about that but I had difficulties with csr* instruction and their second
> operand which expects to have immediate. But if I will have inline function that
> csr_num will be in register.

Only if the function wouldn't be inlined, I expect? Which hence you may need
to force, by using always_inline.

Jan

Re: [PATCH v7 14/14] xen/riscv: Disable SSTC extension and add trap-based CSR probing
Posted by Oleksii Kurochko 1 month ago
On 3/11/26 11:58 AM, Jan Beulich wrote:
> On 11.03.2026 10:54, Oleksii Kurochko wrote:
>> On 3/10/26 10:15 AM, Jan Beulich wrote:
>>> On 06.03.2026 17:33, Oleksii Kurochko wrote:
>>>> --- 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.
>> When SSTC isn't available my panic() won't occur and then will continue to
>> be executed. Otherwise, when SSTC is enabled (it is enabled by QEMU by default)
>> my panic will occur.
> Oh, I notice I misread the condition around the panic(), mainly because of
> the misleading / ambiguous message passed to it: "SSTC isn't supported\n"
> can mean unsupported by Xen or unsupported by the platform.
>
> Anyway, to me this is entirely bogus: Why would we panic() because there is
> a certain extension available?

It is bogus because then we need also add support of SSTC for a guest what isn't
done now thereby if it is detected that SSTC is available that it is dangerous
to continue about full support (guest part) of it.

I thought about the case to let Xen use SSTC and just don't tell Linux that SSTC
is available by dropping from riscv,isa property the mentioning of SSTC, so then
Linux will still continue to use SBI set timer call to Xen and the will just
safely reprogram (if it is needed) a timer using SSTC instructions. But if to do
in this way still nothing will prevent a guest to test if SSTC is available by
reading CSR_STIMECMP and nothing will prevent to access CSR_VSTIMECMP by guest
what could also lead to some misleading behavior.
Likely we could set henvcfg.STCE to zero and it will forbid guest to access SSTC
registers but I am not sure that we really want such behavior when Xen is using
SSTC to setup a timer, but guest isn't allowed. It seems it will be better just
support SSTC extension fully and not to support it only for now.

>
>>>> --- 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.
>> Good point it could be an option.
>>
>>>> +    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.
>> Do I understand correct that your concern is about that if NMIs will be handled
>> in HS-mode that switching stvec in this way could be dangerous as do_expected_trap()
>> doesn't know how to handle NMIs?
> Yes.
>
>> If yes, then NMIs should be handled by M-mode as:
>>     Non-maskable interrupts (NMIs) are only used for hardware error conditions, and
>>     cause an immediate jump to an implementation-defined NMI vector running in M-mode
>>     regardless of the state of a hart’s interrupt enable bits
>> and:
>>     The non-maskable interrupt is not made visible via the mip register as its
>>     presence is implicitly known when executing the NMI trap handler.
>>
>> So standard delegation registers like mideleg do not apply to NMIs because NMIs
>> are not visible in the mip register.
>>
>> I haven't found in OpenSBI how they are explicitly handling NMIs, but it looks
>> like if they happen in (H)S-mode or (V)U-mode then they will be just redirected
>> to (H)S-mode or V(U)-mode:
>>     https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_trap.c#L361
>> And then do_expected_trap() will fail to handle them...
>>
>> Interesting that other hypervisors are using the similar approarch (with temporary
>> updating of stvec) and they haven't faced such issue with NMIs yet...
> Well, NMIs may be rare to occur? And hence very unlikely to occur in this small
> a window?

It is still sound risky and ASM_EXTABLE approach sounds more safe particular in this
case.

>
>>>> +    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.
>> I don't see that such an invalid value exist for scause. I think we have to reserved
>> a value from region 24-31 or 48-63 as they are designated for custom use.
> Not sure that's possible. "Custom use" may mean "custom" from hw perspective.
> I was rather thinking of picking something pretty high in the reserved range,
> like (1 << (MXLEN-1)) - 1 or 1 << (MXLEN-2).

Agree, it could be an option.

>
>>>> +    local_irq_save(flags); \
>>>> +    asm volatile ( \
>>>> +        ".option push\n" \
>>>> +        ".option norvc\n" \
>>> Shouldn't this come later?
>> Do you mean before where SSTC csr is really tried to be read ("csrr %[ret], %[csr]\n")?
> Yes.
>
>> Does it really matter in such small inline assembler?
> Yes, if nothing else then to not raise questions. Plus (depending on the
> specific operands used), the ADD (MV) could e.g. be representable by a C insn.
>
>>> And why set ttmp in the first place, when
>>> that's what do_expected_trap() writes to?
>> To force the compiler to materialize tinfo in register a4 (ttmp) before the
>> trap handler runs as handler will use a4 as temporary register.
> ??? I don't understand what you mean with "materialize".

Mean forcing the compiler to load the variable into the specific hardware
register (a4) before the potentially trapping instruction executes, so the
trap handler can safely use that register.

>
>>>> +        "csrr %[ret], %[csr]\n" \
>>>> +        "csrw " STR(CSR_STVEC) ", %[stvec]\n" \
>>>> +        ".option pop" \
>>>> +        : [stvec] "+&r" (stvec), [tinfo] "+&r" (tinfo), \
>>> tinfo isn't modified, is it?
>> It is modified by handler.
> Where? It's only used as the address of the two stores.

There are to updates of tinfo in the do_expected_trap():

FUNC(do_expected_trap)
         ...
         REG_S   a4, RISCV_TRAP_SEPC(a3)
         ...
         REG_S   a4, RISCV_TRAP_SCAUSE(a3)
         ...
END(do_expected_trap)


>
>>>> +          [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).
>> ttmp is modified by handler too.
> Of course, but just to repeat - you mean "=&r" there.
>
>>> 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).
>> It was done in that way as when csrr will lead to a fault, handler will jump
>> over the csrr instruction and so ret won't be set at all. For that case it was
>> set to 0.
> And again - this is meaningless if the constraint is "=&r".

Make sense...

>
>>>> +        : [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).
>> With the current one use case it doesn't care but generally I think that someone
>> will want to use this macro just to get CSR value. I don't have a speicifc example
>> but still it could be used in this way.
> Well, if you want to keep it doing so, make the name match what it does (and
> in particular what it returns).
>
>>> Ideally such would also be an inline function.
>> I thought about that but I had difficulties with csr* instruction and their second
>> operand which expects to have immediate. But if I will have inline function that
>> csr_num will be in register.
> Only if the function wouldn't be inlined, I expect? Which hence you may need
> to force, by using always_inline.

It could work.

Thanks.

~ Oleksii


Re: [PATCH v7 14/14] xen/riscv: Disable SSTC extension and add trap-based CSR probing
Posted by Jan Beulich 1 month ago
On 11.03.2026 12:38, Oleksii Kurochko wrote:
> On 3/11/26 11:58 AM, Jan Beulich wrote:
>> On 11.03.2026 10:54, Oleksii Kurochko wrote:
>>> On 3/10/26 10:15 AM, Jan Beulich wrote:
>>>> On 06.03.2026 17:33, Oleksii Kurochko wrote:
>>>>> --- 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.
>>> When SSTC isn't available my panic() won't occur and then will continue to
>>> be executed. Otherwise, when SSTC is enabled (it is enabled by QEMU by default)
>>> my panic will occur.
>> Oh, I notice I misread the condition around the panic(), mainly because of
>> the misleading / ambiguous message passed to it: "SSTC isn't supported\n"
>> can mean unsupported by Xen or unsupported by the platform.
>>
>> Anyway, to me this is entirely bogus: Why would we panic() because there is
>> a certain extension available?
> 
> It is bogus because then we need also add support of SSTC for a guest what isn't
> done now thereby if it is detected that SSTC is available that it is dangerous
> to continue about full support (guest part) of it.
> 
> I thought about the case to let Xen use SSTC and just don't tell Linux that SSTC
> is available by dropping from riscv,isa property the mentioning of SSTC, so then
> Linux will still continue to use SBI set timer call to Xen and the will just
> safely reprogram (if it is needed) a timer using SSTC instructions. But if to do
> in this way still nothing will prevent a guest to test if SSTC is available by
> reading CSR_STIMECMP and nothing will prevent to access CSR_VSTIMECMP by guest
> what could also lead to some misleading behavior.
> Likely we could set henvcfg.STCE to zero and it will forbid guest to access SSTC
> registers but I am not sure that we really want such behavior when Xen is using
> SSTC to setup a timer, but guest isn't allowed.

I don't see what's wrong with Xen using an extension that isn't made available
to guests.

> It seems it will be better just
> support SSTC extension fully and not to support it only for now.

If at all, an ack for such from me would be pretty reluctantly given.

>>>>> +    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.
>>> I don't see that such an invalid value exist for scause. I think we have to reserved
>>> a value from region 24-31 or 48-63 as they are designated for custom use.
>> Not sure that's possible. "Custom use" may mean "custom" from hw perspective.
>> I was rather thinking of picking something pretty high in the reserved range,
>> like (1 << (MXLEN-1)) - 1 or 1 << (MXLEN-2).
> 
> Agree, it could be an option.
> 
>>
>>>>> +    local_irq_save(flags); \
>>>>> +    asm volatile ( \
>>>>> +        ".option push\n" \
>>>>> +        ".option norvc\n" \
>>>> Shouldn't this come later?
>>> Do you mean before where SSTC csr is really tried to be read ("csrr %[ret], %[csr]\n")?
>> Yes.
>>
>>> Does it really matter in such small inline assembler?
>> Yes, if nothing else then to not raise questions. Plus (depending on the
>> specific operands used), the ADD (MV) could e.g. be representable by a C insn.
>>
>>>> And why set ttmp in the first place, when
>>>> that's what do_expected_trap() writes to?
>>> To force the compiler to materialize tinfo in register a4 (ttmp) before the
>>> trap handler runs as handler will use a4 as temporary register.
>> ??? I don't understand what you mean with "materialize".
> 
> Mean forcing the compiler to load the variable into the specific hardware
> register (a4) before the potentially trapping instruction executes, so the
> trap handler can safely use that register.

I fear there's some misunderstanding on inline assembly here. An output-only
variable doesn't need "pre-loading". But anyway, all of this is gong to be
moot here (but potentially relevant elsewhere in the future) when this
becomes a mere clobber.

>>>>> +        "csrr %[ret], %[csr]\n" \
>>>>> +        "csrw " STR(CSR_STVEC) ", %[stvec]\n" \
>>>>> +        ".option pop" \
>>>>> +        : [stvec] "+&r" (stvec), [tinfo] "+&r" (tinfo), \
>>>> tinfo isn't modified, is it?
>>> It is modified by handler.
>> Where? It's only used as the address of the two stores.
> 
> There are to updates of tinfo in the do_expected_trap():
> 
> FUNC(do_expected_trap)
>          ...
>          REG_S   a4, RISCV_TRAP_SEPC(a3)
>          ...
>          REG_S   a4, RISCV_TRAP_SCAUSE(a3)
>          ...
> END(do_expected_trap)

a3 is used there twice, yes, but neither use modifies the register. If the
first use modified it, the 2nd use would be broken.

Jan
Re: [PATCH v7 14/14] xen/riscv: Disable SSTC extension and add trap-based CSR probing
Posted by Oleksii Kurochko 1 month ago
On 3/11/26 10:54 AM, Oleksii Kurochko wrote:
> On 3/10/26 10:15 AM, Jan Beulich wrote:
>> 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.
>
> At the moment this approach works for me and I haven't had a need for 
> more
> generic approach. I will look at _ASM_EXTABLE(). I haven't checked yet 
> but
> I assume it will require some extra fixup code in trap handler what looks
> like over complication for the current case, at least.

I checked _ASM_EXTABLE() implementation and so I will need basically provide only
for now implementation of EX_TYPE_FIXUP what doesn't look too much and is comparable
with the current suggested solution.

~ Oleksii