OpenSBI currently does not advertise the SSTC extension via the device
tree, so if SSTC support is detected by Xen the riscv_isa bitmap is updated
manually. Furthermore, removing the "sstc" string from riscv,isa is not
a reliable way to disable SSTC, because OpenSBI probes support by
attempting to access CSR_STIMECMP.

Introduce a runtime probe in Xen to determine whether SSTC is available.
The probe attempts to read CSR_STIMECMP using csr_read_safe(). If the
access succeeds, SSTC is considered available; if a trap occurs, it is
treated as unsupported.

When SSTC is detected, Xen may use it internally to program timers.
However, the extension is not exposed to guests because the required
context switch handling for the SSTC CSRs is not yet implemented.

Note: clearing RISCV_ISA_EXT_sstc from the DTS riscv,isa property is
deferred to a follow-up patch. Also, the corresponding HENVCFG bit is
not set so guests fall back to the SBI timer interface. Timer requests
are then handled by Xen via the usual SBI interception path.

Introduce set_xen_timer() to abstract how the timer is programmed,
either via the SSTC extension or an SBI call.

Drop sbi_set_timer() as it is more than enough to have only introduced
set_xen_timer().

Drop "SBI v0.2 TIME extension detected" message to avoid confusion
which set timer function is really used.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
----
Changes in v4:
 - Introduce csr_write64() and __csr_write32h().
 - Sort numericaly definitions of CSR_VSTIMECMP{H}.
---
Changes in v3:
 - Reword print message when SSTC extension is detected.
 - s/__clear_bit/__set_bit() for the case when SSTC is detected in
   riscv_fill_hwcap().
   Update also the comment above __set_bit().
 - Drop BUG_ON()s in vtimer.c.
 - s/printk/dprintk for the message: SSTC detected...
 - Drop sbi_set_timer global variable, it is enough just to have set_xen_timer.
 - As we set bit in riscv_isa bitmap there is no need to use csr_read_safe(CSR_STIMECMP) second time.
 - Move init of CSR_VSTIMECMP in preinit_xen_time as it looks more correct place.
 - Update the commit message.
---
Changes in v2:
 - Minor style fixes.
 - Drop from vcpu_csr_init() setting of SSTC bit in HENVCFG register. Add it
   back when SSTC for guests will be available.
 - Add static to set_xen_timer function pointer.
 - Refactor sstc_set_xen_timer().
 - s/csr_allowed_read/csr_read_safe()
---
 xen/arch/riscv/cpufeature.c                 | 18 +++++++++
 xen/arch/riscv/include/asm/cpufeature.h     |  1 +
 xen/arch/riscv/include/asm/csr.h            | 14 +++++++
 xen/arch/riscv/include/asm/riscv_encoding.h |  2 +
 xen/arch/riscv/include/asm/sbi.h            | 18 ---------
 xen/arch/riscv/include/asm/time.h           |  3 ++
 xen/arch/riscv/sbi.c                        | 29 +++++++++++---
 xen/arch/riscv/time.c                       | 42 +++++++++++++--------
 xen/arch/riscv/vtimer.c                     |  1 +
 9 files changed, 89 insertions(+), 39 deletions(-)

diff --git a/xen/arch/riscv/cpufeature.c b/xen/arch/riscv/cpufeature.c
index 03e27b037be0..92235fdfd5ab 100644
--- a/xen/arch/riscv/cpufeature.c
+++ b/xen/arch/riscv/cpufeature.c
@@ -17,6 +17,7 @@
 #include <xen/sections.h>
 
 #include <asm/cpufeature.h>
+#include <asm/csr.h>
 
 #ifdef CONFIG_ACPI
 # error "cpufeature.c functions should be updated to support ACPI"
@@ -139,6 +140,7 @@ const struct riscv_isa_ext_data __initconst riscv_isa_ext[] = {
     RISCV_ISA_EXT_DATA(smaia),
     RISCV_ISA_EXT_DATA(smstateen),
     RISCV_ISA_EXT_DATA(ssaia),
+    RISCV_ISA_EXT_DATA(sstc),
     RISCV_ISA_EXT_DATA(svade),
     RISCV_ISA_EXT_DATA(svpbmt),
 };
@@ -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;
+    unsigned long tmp;
 
     riscv_fill_hwcap_from_isa_string();
 
@@ -495,6 +498,21 @@ void __init riscv_fill_hwcap(void)
         panic("HW capabilities parsing failed: %s\n", failure_msg);
     }
 
+    if ( csr_read_safe(CSR_STIMECMP, &tmp) )
+    {
+        dprintk(XENLOG_DEBUG,
+                "SSTC detected; supported for Xen use, but not for guests\n");
+
+        /*
+         * As there is no any guarantee that SSTC will be added to riscv,isa
+         * property by OpenSBI(it doesn't add it now) or whatever ran before
+         * Xen, it is needed to set this bit manually.
+         *
+         * Guest isolation is maintained by not setting ENVCFG_STCE in henvcfg.
+         */
+        __set_bit(RISCV_ISA_EXT_sstc, riscv_isa);
+    }
+
     for ( i = 0; i < req_extns_amount; i++ )
     {
         const struct riscv_isa_ext_data ext = required_extensions[i];
diff --git a/xen/arch/riscv/include/asm/cpufeature.h b/xen/arch/riscv/include/asm/cpufeature.h
index ef02a3e26d2c..0c48d57a03bb 100644
--- a/xen/arch/riscv/include/asm/cpufeature.h
+++ b/xen/arch/riscv/include/asm/cpufeature.h
@@ -38,6 +38,7 @@ enum riscv_isa_ext_id {
     RISCV_ISA_EXT_smaia,
     RISCV_ISA_EXT_smstateen,
     RISCV_ISA_EXT_ssaia,
+    RISCV_ISA_EXT_sstc,
     RISCV_ISA_EXT_svade,
     RISCV_ISA_EXT_svpbmt,
     RISCV_ISA_EXT_MAX
diff --git a/xen/arch/riscv/include/asm/csr.h b/xen/arch/riscv/include/asm/csr.h
index 27d4b7942f6b..9dc5f8767508 100644
--- a/xen/arch/riscv/include/asm/csr.h
+++ b/xen/arch/riscv/include/asm/csr.h
@@ -32,6 +32,20 @@
                            : "memory" );                        \
 })
 
+#ifdef CONFIG_RISCV_32
+# define __csr_write32h(csr, val) csr_write(csr ## H, (val) >> 32)
+#else
+# define __csr_write32h(csr, val) ((void)(csr), (void)(val))
+#endif
+
+#define csr_write64(csr, val)   \
+({                              \
+    uint64_t _v = (val);        \
+                                \
+    csr_write(csr, _v);         \
+    __csr_write32h(csr, _v);    \
+})
+
 #define csr_swap(csr, val)                                      \
 ({                                                              \
     unsigned long __v = (unsigned long)(val);                   \
diff --git a/xen/arch/riscv/include/asm/riscv_encoding.h b/xen/arch/riscv/include/asm/riscv_encoding.h
index dd15731a86fa..03e186bcdb8c 100644
--- a/xen/arch/riscv/include/asm/riscv_encoding.h
+++ b/xen/arch/riscv/include/asm/riscv_encoding.h
@@ -395,6 +395,8 @@
 #define CSR_VSCAUSE			0x242
 #define CSR_VSTVAL			0x243
 #define CSR_VSIP			0x244
+#define CSR_VSTIMECMP		0x24d
+#define CSR_VSTIMECMPH		0x25d
 #define CSR_VSATP			0x280
 
 /* Virtual Interrupts and Interrupt Priorities (H-extension with AIA) */
diff --git a/xen/arch/riscv/include/asm/sbi.h b/xen/arch/riscv/include/asm/sbi.h
index ed7af200288f..1952868e963c 100644
--- a/xen/arch/riscv/include/asm/sbi.h
+++ b/xen/arch/riscv/include/asm/sbi.h
@@ -13,7 +13,6 @@
 #define ASM__RISCV__SBI_H
 
 #include <xen/cpumask.h>
-#include <xen/sections.h>
 
 /* SBI-defined implementation ID */
 #define SBI_XEN_IMPID 7
@@ -139,23 +138,6 @@ int sbi_remote_hfence_gvma(const cpumask_t *cpu_mask, vaddr_t start,
 int sbi_remote_hfence_gvma_vmid(const cpumask_t *cpu_mask, vaddr_t start,
                                 size_t size, unsigned long vmid);
 
-/*
- * Programs the clock for next event at (or after) stime_value. stime_value is
- * in absolute time. This function must clear the pending timer interrupt bit
- * as well.
- *
- * If the supervisor wishes to clear the timer interrupt without scheduling the
- * next timer event, it can either request a timer interrupt infinitely far
- * into the future (i.e., (uint64_t)-1), or it can instead mask the timer
- * interrupt by clearing sie.STIE CSR bit.
- *
- * The stime_value parameter represents absolute time measured in ticks.
- *
- * This SBI call returns 0 upon success or an implementation specific negative
- * error code.
- */
-extern int (* __ro_after_init sbi_set_timer)(uint64_t stime_value);
-
 /*
  * Initialize SBI library
  *
diff --git a/xen/arch/riscv/include/asm/time.h b/xen/arch/riscv/include/asm/time.h
index be3875b9984e..4d68900151a7 100644
--- a/xen/arch/riscv/include/asm/time.h
+++ b/xen/arch/riscv/include/asm/time.h
@@ -4,6 +4,7 @@
 
 #include <xen/bug.h>
 #include <xen/muldiv64.h>
+#include <xen/sections.h>
 
 #include <asm/csr.h>
 
@@ -26,6 +27,8 @@ static inline cycles_t get_cycles(void)
 
 void preinit_xen_time(void);
 
+extern int (* __ro_after_init set_xen_timer)(uint64_t deadline);
+
 #endif /* ASM__RISCV__TIME_H */
 
 /*
diff --git a/xen/arch/riscv/sbi.c b/xen/arch/riscv/sbi.c
index b4a7ae6940c1..3576e26033a5 100644
--- a/xen/arch/riscv/sbi.c
+++ b/xen/arch/riscv/sbi.c
@@ -22,6 +22,7 @@
 
 #include <asm/processor.h>
 #include <asm/sbi.h>
+#include <asm/time.h>
 
 static unsigned long __ro_after_init sbi_spec_version = SBI_SPEC_VERSION_DEFAULT;
 
@@ -249,6 +250,21 @@ static int (* __ro_after_init sbi_rfence)(unsigned long fid,
                                           unsigned long arg4,
                                           unsigned long arg5);
 
+/*
+ * Programs the clock for next event at (or after) stime_value. stime_value is
+ * in absolute time. This function must clear the pending timer interrupt bit
+ * as well.
+ *
+ * If the supervisor wishes to clear the timer interrupt without scheduling the
+ * next timer event, it can either request a timer interrupt infinitely far
+ * into the future (i.e., (uint64_t)-1), or it can instead mask the timer
+ * interrupt by clearing sie.STIE CSR bit.
+ *
+ * The stime_value parameter represents absolute time measured in ticks.
+ *
+ * This SBI call returns 0 upon success or an implementation specific negative
+ * error code.
+ */
 static int cf_check sbi_set_timer_v02(uint64_t stime_value)
 {
     struct sbiret ret;
@@ -264,6 +280,10 @@ static int cf_check sbi_set_timer_v02(uint64_t stime_value)
     return sbi_err_map_xen_errno(ret.error);
 }
 
+/*
+ * Legacy SBI v0.1 SET_TIMER; functionally equivalent to sbi_set_timer_v02
+ * from Xen's perspective.
+ */
 static int cf_check sbi_set_timer_v01(uint64_t stime_value)
 {
     struct sbiret ret;
@@ -279,8 +299,6 @@ static int cf_check sbi_set_timer_v01(uint64_t stime_value)
     return sbi_err_map_xen_errno(ret.error);
 }
 
-int (* __ro_after_init sbi_set_timer)(uint64_t stime_value) = sbi_set_timer_v01;
-
 int sbi_remote_sfence_vma(const cpumask_t *cpu_mask, vaddr_t start,
                           size_t size)
 {
@@ -360,10 +378,9 @@ int __init sbi_init(void)
         }
 
         if ( sbi_probe_extension(SBI_EXT_TIME) > 0 )
-        {
-            sbi_set_timer = sbi_set_timer_v02;
-            dprintk(XENLOG_INFO, "SBI v0.2 TIME extension detected\n");
-        }
+            set_xen_timer = sbi_set_timer_v02;
+        else
+            set_xen_timer = sbi_set_timer_v01;
     }
     else
         panic("Ooops. SBI spec version 0.1 detected. Need to add support");
diff --git a/xen/arch/riscv/time.c b/xen/arch/riscv/time.c
index 698ab49d1292..8769709e5227 100644
--- a/xen/arch/riscv/time.c
+++ b/xen/arch/riscv/time.c
@@ -7,12 +7,21 @@
 #include <xen/time.h>
 #include <xen/types.h>
 
+#include <asm/cpufeature.h>
 #include <asm/csr.h>
-#include <asm/sbi.h>
 
 unsigned long __ro_after_init cpu_khz; /* CPU clock frequency in kHz. */
 uint64_t __ro_after_init boot_clock_cycles;
 
+static int cf_check sstc_set_xen_timer(uint64_t deadline)
+{
+    csr_write64(CSR_STIMECMP, deadline);
+
+    return 0;
+}
+
+int (* __ro_after_init set_xen_timer)(uint64_t deadline);
+
 s_time_t get_s_time(void)
 {
     uint64_t ticks = get_cycles() - boot_clock_cycles;
@@ -61,20 +70,7 @@ int reprogram_timer(s_time_t timeout)
     if ( deadline <= now )
         return 0;
 
-    /*
-     * TODO: When the SSTC extension is supported, it would be preferable to
-     *       use the supervisor timer registers directly here for better
-     *       performance, since an SBI call and mode switch would no longer
-     *       be required.
-     *
-     *       This would also reduce reliance on a specific SBI implementation.
-     *       For example, it is not ideal to panic() if sbi_set_timer() returns
-     *       a non-zero value. Currently it can return 0 or -ENOSUPP, and
-     *       without SSTC we still need an implementation because only the
-     *       M-mode timer is available, and it can only be programmed in
-     *       M-mode.
-     */
-    if ( (rc = sbi_set_timer(deadline)) )
+    if ( (rc = set_xen_timer(deadline)) )
         panic("%s: timer wasn't set because: %d\n", __func__, rc);
 
     /* Enable timer interrupt */
@@ -91,4 +87,20 @@ void __init preinit_xen_time(void)
         panic("%s: ACPI isn't supported\n", __func__);
 
     boot_clock_cycles = get_cycles();
+
+    /* set_xen_timer must have been set by sbi_init() already */
+    ASSERT(set_xen_timer);
+
+    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_sstc) )
+    {
+        set_xen_timer = sstc_set_xen_timer;
+
+        /*
+         * A VS-timer interrupt becomes pending whenever the value of
+         * (time + htimedelta) is greater than or equal to vstimecmp CSR.
+         * Thereby to avoid spurious VS-timer irqs set vstimecmp CSR to
+         * ULONG_MAX.
+         */
+        csr_write64(CSR_STIMECMP, ULONG_MAX);
+    }
 }
diff --git a/xen/arch/riscv/vtimer.c b/xen/arch/riscv/vtimer.c
index afd8a53a7387..d5a8dfcb2edb 100644
--- a/xen/arch/riscv/vtimer.c
+++ b/xen/arch/riscv/vtimer.c
@@ -4,6 +4,7 @@
 #include <xen/sched.h>
 #include <xen/timer.h>
 
+#include <asm/cpufeature.h>
 #include <asm/vtimer.h>
 
 static void vtimer_expired(void *data)
-- 
2.53.0

On 17.04.2026 09:24, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/include/asm/csr.h
> +++ b/xen/arch/riscv/include/asm/csr.h
> @@ -32,6 +32,20 @@
>                             : "memory" );                        \
>  })
>  
> +#ifdef CONFIG_RISCV_32
> +# define __csr_write32h(csr, val) csr_write(csr ## H, (val) >> 32)
> +#else
> +# define __csr_write32h(csr, val) ((void)(csr), (void)(val))

In order to be able to spot issues in 64-bit builds, how about

# define __csr_write32h(csr, val) ((void)csr ## H, (void)(val))

?

Apart from this, no matter that it was Andrew to suggest this, I'd like to
(once again) point out that identifiers starting with two underscores are
reserved. I don't see why a single underscore wouldn't do here. Or
alternatively csr__write32h().

> @@ -279,8 +299,6 @@ static int cf_check sbi_set_timer_v01(uint64_t stime_value)
>      return sbi_err_map_xen_errno(ret.error);
>  }
>  
> -int (* __ro_after_init sbi_set_timer)(uint64_t stime_value) = sbi_set_timer_v01;
> -
>  int sbi_remote_sfence_vma(const cpumask_t *cpu_mask, vaddr_t start,
>                            size_t size)
>  {
> @@ -360,10 +378,9 @@ int __init sbi_init(void)
>          }
>  
>          if ( sbi_probe_extension(SBI_EXT_TIME) > 0 )
> -        {
> -            sbi_set_timer = sbi_set_timer_v02;
> -            dprintk(XENLOG_INFO, "SBI v0.2 TIME extension detected\n");
> -        }
> +            set_xen_timer = sbi_set_timer_v02;
> +        else
> +            set_xen_timer = sbi_set_timer_v01;
>      }

Sadly this isn't quite equivalent to sbi_set_timer having had an initializer.
I would have wanted to suggest to use a constructor function, but we call
init_constructors() even later than do_initcalls() on both Arm and x86 (we
don't call the latter at all on RISC-V so far). Might it be necessary to
introduce sbi_early_init(), called very early during boot? Else how do you
guarantee no accidental use of the variable before it is first set?

Jan

On 4/20/26 9:56 AM, Jan Beulich wrote:
> On 17.04.2026 09:24, Oleksii Kurochko wrote:
>> --- a/xen/arch/riscv/include/asm/csr.h
>> +++ b/xen/arch/riscv/include/asm/csr.h
>> @@ -32,6 +32,20 @@
>>                              : "memory" );                        \
>>   })
>>   
>> +#ifdef CONFIG_RISCV_32
>> +# define __csr_write32h(csr, val) csr_write(csr ## H, (val) >> 32)
>> +#else
>> +# define __csr_write32h(csr, val) ((void)(csr), (void)(val))
> 
> In order to be able to spot issues in 64-bit builds, how about
> 
> # define __csr_write32h(csr, val) ((void)csr ## H, (void)(val))
> 
> ?

But this will cause a build issue in 64-bit builds.

csr_write64(CSR_STIMECMP, ...)
   └─ __csr_write32h(csr, _v)   ← csr is NOT ##-adjacent here
                                   so preprocessor expands it FIRST
                                   CSR_STIMECMP → 0x14D
        └─ (void)csr ## H       ← csr is already 0x14D here
                                   0x14D ## H → 0x14DH  ERROR

Probably, it would be better to do in the following way:

#ifdef CONFIG_RISCV_32
#define csr_write64(csr, val)       \
({                                  \
     uint64_t v_ = (val);            \
     csr_write(csr, v_);             \
     csr_write(csr ## H, v_ >> 32);  \
})
#else
#define csr_write64(csr, val)       \
({                                  \
     uint64_t v_ = (val);            \
     csr_write(csr, v_);             \
})
#endif

Am I missing something?

> 
> Apart from this, no matter that it was Andrew to suggest this, I'd like to
> (once again) point out that identifiers starting with two underscores are
> reserved. I don't see why a single underscore wouldn't do here. Or
> alternatively csr__write32h().

I will apply your suggestion.

> 
>> @@ -279,8 +299,6 @@ static int cf_check sbi_set_timer_v01(uint64_t stime_value)
>>       return sbi_err_map_xen_errno(ret.error);
>>   }
>>   
>> -int (* __ro_after_init sbi_set_timer)(uint64_t stime_value) = sbi_set_timer_v01;
>> -
>>   int sbi_remote_sfence_vma(const cpumask_t *cpu_mask, vaddr_t start,
>>                             size_t size)
>>   {
>> @@ -360,10 +378,9 @@ int __init sbi_init(void)
>>           }
>>   
>>           if ( sbi_probe_extension(SBI_EXT_TIME) > 0 )
>> -        {
>> -            sbi_set_timer = sbi_set_timer_v02;
>> -            dprintk(XENLOG_INFO, "SBI v0.2 TIME extension detected\n");
>> -        }
>> +            set_xen_timer = sbi_set_timer_v02;
>> +        else
>> +            set_xen_timer = sbi_set_timer_v01;
>>       }
> 
> Sadly this isn't quite equivalent to sbi_set_timer having had an initializer.
> I would have wanted to suggest to use a constructor function, but we call
> init_constructors() even later than do_initcalls() on both Arm and x86 (we
> don't call the latter at all on RISC-V so far). Might it be necessary to
> introduce sbi_early_init(), called very early during boot? Else how do you
> guarantee no accidental use of the variable before it is first set?

I thought about an introduction of sbi_early_init() but then decided 
that set_xen_timer() won't be used earlier than at lest timer_init() + 
local_irq_enable().
Also, sbi_init() is executed pretty early.

Thanks.


~ Oleksii

On 21.04.2026 11:01, Oleksii Kurochko wrote:
> On 4/20/26 9:56 AM, Jan Beulich wrote:
>> On 17.04.2026 09:24, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/include/asm/csr.h
>>> +++ b/xen/arch/riscv/include/asm/csr.h
>>> @@ -32,6 +32,20 @@
>>>                              : "memory" );                        \
>>>   })
>>>   
>>> +#ifdef CONFIG_RISCV_32
>>> +# define __csr_write32h(csr, val) csr_write(csr ## H, (val) >> 32)

In my reply I followed this. If this compiled, then ...

>>> +#else
>>> +# define __csr_write32h(csr, val) ((void)(csr), (void)(val))
>>
>> In order to be able to spot issues in 64-bit builds, how about
>>
>> # define __csr_write32h(csr, val) ((void)csr ## H, (void)(val))
>>
>> ?

... aiui this would compile as well. Looks like the RV32 case then is in
need of adjustment as well.

> But this will cause a build issue in 64-bit builds.
> 
> csr_write64(CSR_STIMECMP, ...)
>    └─ __csr_write32h(csr, _v)   ← csr is NOT ##-adjacent here
>                                    so preprocessor expands it FIRST
>                                    CSR_STIMECMP → 0x14D
>         └─ (void)csr ## H       ← csr is already 0x14D here
>                                    0x14D ## H → 0x14DH  ERROR
> 
> Probably, it would be better to do in the following way:
> 
> #ifdef CONFIG_RISCV_32
> #define csr_write64(csr, val)       \
> ({                                  \
>      uint64_t v_ = (val);            \
>      csr_write(csr, v_);             \
>      csr_write(csr ## H, v_ >> 32);  \
> })
> #else
> #define csr_write64(csr, val)       \
> ({                                  \
>      uint64_t v_ = (val);            \
>      csr_write(csr, v_);             \
> })
> #endif

E.g. like this, albeit in the RV64 case the local v_ isn't needed. Instead,
again to be able to spot issues in RV64 builds, (void)csr ## H may want
adding.

A clear downside to all of this is that this helper can only be used with
CSR_* constants, not with runtime-calculated CSR numbers.

>>> @@ -279,8 +299,6 @@ static int cf_check sbi_set_timer_v01(uint64_t stime_value)
>>>       return sbi_err_map_xen_errno(ret.error);
>>>   }
>>>   
>>> -int (* __ro_after_init sbi_set_timer)(uint64_t stime_value) = sbi_set_timer_v01;
>>> -
>>>   int sbi_remote_sfence_vma(const cpumask_t *cpu_mask, vaddr_t start,
>>>                             size_t size)
>>>   {
>>> @@ -360,10 +378,9 @@ int __init sbi_init(void)
>>>           }
>>>   
>>>           if ( sbi_probe_extension(SBI_EXT_TIME) > 0 )
>>> -        {
>>> -            sbi_set_timer = sbi_set_timer_v02;
>>> -            dprintk(XENLOG_INFO, "SBI v0.2 TIME extension detected\n");
>>> -        }
>>> +            set_xen_timer = sbi_set_timer_v02;
>>> +        else
>>> +            set_xen_timer = sbi_set_timer_v01;
>>>       }
>>
>> Sadly this isn't quite equivalent to sbi_set_timer having had an initializer.
>> I would have wanted to suggest to use a constructor function, but we call
>> init_constructors() even later than do_initcalls() on both Arm and x86 (we
>> don't call the latter at all on RISC-V so far). Might it be necessary to
>> introduce sbi_early_init(), called very early during boot? Else how do you
>> guarantee no accidental use of the variable before it is first set?
> 
> I thought about an introduction of sbi_early_init() but then decided 
> that set_xen_timer() won't be used earlier than at lest timer_init() + 
> local_irq_enable().
> Also, sbi_init() is executed pretty early.

Many more additions to setup.c are to be expected. Are you sure hardly any will
go ahead of the call to sbi_init()?

Jan

On 4/21/26 11:10 AM, Jan Beulich wrote:
> On 21.04.2026 11:01, Oleksii Kurochko wrote:
>> On 4/20/26 9:56 AM, Jan Beulich wrote:
>>> On 17.04.2026 09:24, Oleksii Kurochko wrote:
>>>> --- a/xen/arch/riscv/include/asm/csr.h
>>>> +++ b/xen/arch/riscv/include/asm/csr.h
>>>> @@ -32,6 +32,20 @@
>>>>                               : "memory" );                        \
>>>>    })
>>>>    
>>>> +#ifdef CONFIG_RISCV_32
>>>> +# define __csr_write32h(csr, val) csr_write(csr ## H, (val) >> 32)
> 
> In my reply I followed this. If this compiled, then ...
> 
>>>> +#else
>>>> +# define __csr_write32h(csr, val) ((void)(csr), (void)(val))
>>>
>>> In order to be able to spot issues in 64-bit builds, how about
>>>
>>> # define __csr_write32h(csr, val) ((void)csr ## H, (void)(val))
>>>
>>> ?
> 
> ... aiui this would compile as well. Looks like the RV32 case then is in
> need of adjustment as well.
> 
>> But this will cause a build issue in 64-bit builds.
>>
>> csr_write64(CSR_STIMECMP, ...)
>>     └─ __csr_write32h(csr, _v)   ← csr is NOT ##-adjacent here
>>                                     so preprocessor expands it FIRST
>>                                     CSR_STIMECMP → 0x14D
>>          └─ (void)csr ## H       ← csr is already 0x14D here
>>                                     0x14D ## H → 0x14DH  ERROR
>>
>> Probably, it would be better to do in the following way:
>>
>> #ifdef CONFIG_RISCV_32
>> #define csr_write64(csr, val)       \
>> ({                                  \
>>       uint64_t v_ = (val);            \
>>       csr_write(csr, v_);             \
>>       csr_write(csr ## H, v_ >> 32);  \
>> })
>> #else
>> #define csr_write64(csr, val)       \
>> ({                                  \
>>       uint64_t v_ = (val);            \
>>       csr_write(csr, v_);             \
>> })
>> #endif
> 
> E.g. like this, albeit in the RV64 case the local v_ isn't needed. Instead,
> again to be able to spot issues in RV64 builds, (void)csr ## H may want
> adding.
> 
> A clear downside to all of this is that this helper can only be used with
> CSR_* constants, not with runtime-calculated CSR numbers.

Yes, but it isn't critical downside as I don't see cases where it will 
be useful to have runtime-calculated CSR numbers.

> 
>>>> @@ -279,8 +299,6 @@ static int cf_check sbi_set_timer_v01(uint64_t stime_value)
>>>>        return sbi_err_map_xen_errno(ret.error);
>>>>    }
>>>>    
>>>> -int (* __ro_after_init sbi_set_timer)(uint64_t stime_value) = sbi_set_timer_v01;
>>>> -
>>>>    int sbi_remote_sfence_vma(const cpumask_t *cpu_mask, vaddr_t start,
>>>>                              size_t size)
>>>>    {
>>>> @@ -360,10 +378,9 @@ int __init sbi_init(void)
>>>>            }
>>>>    
>>>>            if ( sbi_probe_extension(SBI_EXT_TIME) > 0 )
>>>> -        {
>>>> -            sbi_set_timer = sbi_set_timer_v02;
>>>> -            dprintk(XENLOG_INFO, "SBI v0.2 TIME extension detected\n");
>>>> -        }
>>>> +            set_xen_timer = sbi_set_timer_v02;
>>>> +        else
>>>> +            set_xen_timer = sbi_set_timer_v01;
>>>>        }
>>>
>>> Sadly this isn't quite equivalent to sbi_set_timer having had an initializer.
>>> I would have wanted to suggest to use a constructor function, but we call
>>> init_constructors() even later than do_initcalls() on both Arm and x86 (we
>>> don't call the latter at all on RISC-V so far). Might it be necessary to
>>> introduce sbi_early_init(), called very early during boot? Else how do you
>>> guarantee no accidental use of the variable before it is first set?
>>
>> I thought about an introduction of sbi_early_init() but then decided
>> that set_xen_timer() won't be used earlier than at lest timer_init() +
>> local_irq_enable().
>> Also, sbi_init() is executed pretty early.
> 
> Many more additions to setup.c are to be expected. Are you sure hardly any will
> go ahead of the call to sbi_init()?

Looking at the current state, I don't see something new what will added 
before sbi_init() except percpu_init_areas().

I am okay to introduce sbi_early_init() if it will be really better:

--- a/xen/arch/riscv/sbi.c
+++ b/xen/arch/riscv/sbi.c
@@ -387,3 +387,8 @@ int __init sbi_init(void)

      return 0;
  }
+
+void __init sbi_early_init(void)
+{
+    set_xen_timer = sbi_set_timer_v01;
+}
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 56a0907a855f..b187a84cd28d 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -78,6 +78,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
      const char *cmdline;
      size_t fdt_size;

+    sbi_early_init();

But it looks to me that is fine to have what we have now as even someone 
will try to use set_xen_timer earlier a trap will occur and thereby it 
will be need to put the code which start to use set_xen_timer after 
sbi_init().

Best regards,
  Oleksii

On 21.04.2026 11:33, Oleksii Kurochko wrote:
> On 4/21/26 11:10 AM, Jan Beulich wrote:
>> On 21.04.2026 11:01, Oleksii Kurochko wrote:
>>> On 4/20/26 9:56 AM, Jan Beulich wrote:
>>>> On 17.04.2026 09:24, Oleksii Kurochko wrote:
>>>>> @@ -279,8 +299,6 @@ static int cf_check sbi_set_timer_v01(uint64_t stime_value)
>>>>>        return sbi_err_map_xen_errno(ret.error);
>>>>>    }
>>>>>    
>>>>> -int (* __ro_after_init sbi_set_timer)(uint64_t stime_value) = sbi_set_timer_v01;
>>>>> -
>>>>>    int sbi_remote_sfence_vma(const cpumask_t *cpu_mask, vaddr_t start,
>>>>>                              size_t size)
>>>>>    {
>>>>> @@ -360,10 +378,9 @@ int __init sbi_init(void)
>>>>>            }
>>>>>    
>>>>>            if ( sbi_probe_extension(SBI_EXT_TIME) > 0 )
>>>>> -        {
>>>>> -            sbi_set_timer = sbi_set_timer_v02;
>>>>> -            dprintk(XENLOG_INFO, "SBI v0.2 TIME extension detected\n");
>>>>> -        }
>>>>> +            set_xen_timer = sbi_set_timer_v02;
>>>>> +        else
>>>>> +            set_xen_timer = sbi_set_timer_v01;
>>>>>        }
>>>>
>>>> Sadly this isn't quite equivalent to sbi_set_timer having had an initializer.
>>>> I would have wanted to suggest to use a constructor function, but we call
>>>> init_constructors() even later than do_initcalls() on both Arm and x86 (we
>>>> don't call the latter at all on RISC-V so far). Might it be necessary to
>>>> introduce sbi_early_init(), called very early during boot? Else how do you
>>>> guarantee no accidental use of the variable before it is first set?
>>>
>>> I thought about an introduction of sbi_early_init() but then decided
>>> that set_xen_timer() won't be used earlier than at lest timer_init() +
>>> local_irq_enable().
>>> Also, sbi_init() is executed pretty early.
>>
>> Many more additions to setup.c are to be expected. Are you sure hardly any will
>> go ahead of the call to sbi_init()?
> 
> Looking at the current state, I don't see something new what will added 
> before sbi_init() except percpu_init_areas().
> 
> I am okay to introduce sbi_early_init() if it will be really better:
> 
> --- a/xen/arch/riscv/sbi.c
> +++ b/xen/arch/riscv/sbi.c
> @@ -387,3 +387,8 @@ int __init sbi_init(void)
> 
>       return 0;
>   }
> +
> +void __init sbi_early_init(void)
> +{
> +    set_xen_timer = sbi_set_timer_v01;
> +}
> diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
> index 56a0907a855f..b187a84cd28d 100644
> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -78,6 +78,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
>       const char *cmdline;
>       size_t fdt_size;
> 
> +    sbi_early_init();
> 
> But it looks to me that is fine to have what we have now as even someone 
> will try to use set_xen_timer earlier a trap will occur and thereby it 
> will be need to put the code which start to use set_xen_timer after 
> sbi_init().

It's your call really.

Jan