[PATCH v2 0/4] RISCV: Intrdouce SSTC support in Xen

[PATCH v2 0/4] RISCV: Intrdouce SSTC support in Xen

[Oleksii Kurochko]

This patch series is created based on [1] and the aim is to make Xen properly
works when SSTC extension is available.

It is needed to do in this way as OpenSBI doesn't pass that it supports SSTC
by DTS to Xen and there is no easy way to turn off support of SSTC support.

Also, as a part of this patch series intrdouce suggested [2] by Jan B.
improvements of init_csr_masks().

[1] https://lore.kernel.org/xen-devel/cover.1772814110.git.oleksii.kurochko@gmail.com/T/#mc14576ef43a83b344c5f31626005b995e2ccbeaa
[2] https://lore.kernel.org/xen-devel/cover.1772814110.git.oleksii.kurochko@gmail.com/T/#m9c18d2d7a98958befec16419f5deccc40f6c8c3e

CI tests: https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/2417456208

Also, I've manually ran QEMU with SSTC=on/off and built Xen with a newer RISC-V
compileer (in comparison with the version used by CI's docker container) to
verify CONFIG_CC_HAS_ASM_GOTO_OUTPUT branch inside csr_read_safe().

---
Changes in v2:
 - Address the comments from ML.
---

Oleksii Kurochko (4):
  xen/riscv: add exception table support
  xen/riscv: add csr_allowed_read() helper
  xen/riscv: allow Xen to use SSTC while hiding it from guests
  xen/riscv: init_csr_masks()-related improvements

 xen/arch/riscv/Kconfig                      |  1 +
 xen/arch/riscv/Makefile                     |  1 +
 xen/arch/riscv/cpufeature.c                 | 33 ++++++++
 xen/arch/riscv/domain.c                     | 29 +++----
 xen/arch/riscv/extable.c                    | 85 +++++++++++++++++++++
 xen/arch/riscv/include/asm/bug.h            |  2 +
 xen/arch/riscv/include/asm/cpufeature.h     |  1 +
 xen/arch/riscv/include/asm/csr.h            | 36 ++++++++-
 xen/arch/riscv/include/asm/extable.h        | 58 ++++++++++++++
 xen/arch/riscv/include/asm/riscv_encoding.h |  2 +
 xen/arch/riscv/setup.c                      |  3 +
 xen/arch/riscv/time.c                       | 34 +++++----
 xen/arch/riscv/traps.c                      |  5 ++
 xen/arch/riscv/vtimer.c                     |  7 +-
 xen/arch/riscv/xen.lds.S                    |  3 +
 xen/arch/x86/xen.lds.S                      |  6 +-
 xen/include/xen/xen.lds.h                   |  6 ++
 17 files changed, 277 insertions(+), 35 deletions(-)
 create mode 100644 xen/arch/riscv/extable.c
 create mode 100644 xen/arch/riscv/include/asm/extable.h

-- 
2.53.0

[PATCH v2 1/4] xen/riscv: add exception table support

[Oleksii Kurochko]

Introduce exception table handling for RISC-V so faults from selected
instructions can be recovered via fixup handlers instead of being
treated as fatal.

Add the RISC-V exception table format, sorting at boot to allow binary
search used furthuer, and lookup from the trap handler. Update the
linker script to emit the .ex_table section using introduced common
EX_TABLE macro shared with other architectures.

Also, reduce __start___ex_table alignment from 8 to 4 bytes to
match the natural alignment of struct exception_table_entry,
which contains two int32_t fields.

Add inclusion of asm/extable.h to asm/bug.h to deal with compilation
issue of common/virtual_region.c, which require declaration of
__start___ex_table and __stop___ex_table.

This implementation is based on Linux 6.16.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v2:
 - Corrected the name for __start_ex_table identifier in the commit message.
 - Droped plural where extables is used.
 - Added inclusuion of <asm/extable.h> to deal with compilation (nothing
   declares __start___ex_table and __stop___ex_table) of common/virtual_region.c.
 - Use long for delta variable inside swap_ex in extable.c.
 - To take into acoount live-patching code:
   - s/sort_extable/sort_exception_tables.
   - Introduce sort_exception_table() as liveaptch code requires it and
     re-use it inside sort_exception_tables().
- Drop cmp_ex_search() and rename cmp_ex_sort() to cmp_ex().
  Rename local variable l and r inside cmp_ex().
- Identation fixes.
- prefer sizeof(<expression>) over sizeof(<type>) in calls of bsearch() and
  sort().
- Return back defintion of asm_extable() for __ASSEMBLER__ case.
- Correct the comment above declaration of struct exception_table_entry.
- Drop else in do_trap() before "if ( fixup_exception() )" to visually separate
  the set of checks.
- Align start of exception table section by 4-bytes as exception table struct
  contains two 4 bytes integers.
- Make extable.o compile unconditionally.
- Drop ifdef HAS_EX_TABLE in extable.h as extable.o is always compiled.
- Drop ifdef around defintion of EX_TABLE.
- Drop __init for cmp_ex as it is now used in fixup_exception() which isn't
  marked as __init.
- Return void instead of bool for ex_handler_fixup() as this function always
  returns true.
- Update the comment above defintion of struct exception_table_entry() to be
  more accurate.
- Add inclusion of asm/extable.h to asm/bug.h to deal with compilation issue
  of common/virtual_region.c, which require declaration of __start___ex_table
  and __stop___ex_table.
---
 xen/arch/riscv/Kconfig               |  1 +
 xen/arch/riscv/Makefile              |  1 +
 xen/arch/riscv/extable.c             | 85 ++++++++++++++++++++++++++++
 xen/arch/riscv/include/asm/bug.h     |  2 +
 xen/arch/riscv/include/asm/extable.h | 58 +++++++++++++++++++
 xen/arch/riscv/setup.c               |  3 +
 xen/arch/riscv/traps.c               |  5 ++
 xen/arch/riscv/xen.lds.S             |  3 +
 xen/arch/x86/xen.lds.S               |  6 +-
 xen/include/xen/xen.lds.h            |  6 ++
 10 files changed, 165 insertions(+), 5 deletions(-)
 create mode 100644 xen/arch/riscv/extable.c
 create mode 100644 xen/arch/riscv/include/asm/extable.h

diff --git a/xen/arch/riscv/Kconfig b/xen/arch/riscv/Kconfig
index 89876b32175d..a5e87c1757f7 100644
--- a/xen/arch/riscv/Kconfig
+++ b/xen/arch/riscv/Kconfig
@@ -4,6 +4,7 @@ config RISCV
 	select GENERIC_BUG_FRAME
 	select GENERIC_UART_INIT
 	select HAS_DEVICE_TREE_DISCOVERY
+	select HAS_EX_TABLE
 	select HAS_PMAP
 	select HAS_UBSAN
 	select HAS_VMAP
diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
index ffbd7062e214..04f02ad89cba 100644
--- a/xen/arch/riscv/Makefile
+++ b/xen/arch/riscv/Makefile
@@ -3,6 +3,7 @@ obj-y += cpufeature.o
 obj-y += domain.o
 obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
 obj-y += entry.o
+obj-y += extable.o
 obj-y += imsic.o
 obj-y += intc.o
 obj-y += irq.o
diff --git a/xen/arch/riscv/extable.c b/xen/arch/riscv/extable.c
new file mode 100644
index 000000000000..882ae9508d19
--- /dev/null
+++ b/xen/arch/riscv/extable.c
@@ -0,0 +1,85 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include <xen/init.h>
+#include <xen/bsearch.h>
+#include <xen/lib.h>
+#include <xen/livepatch.h>
+#include <xen/sort.h>
+#include <xen/virtual_region.h>
+
+#include <asm/extable.h>
+#include <asm/processor.h>
+
+#define EX_FIELD(ptr, field) ((unsigned long)&(ptr)->field + (ptr)->field)
+
+static inline unsigned long ex_insn(const struct exception_table_entry *ex)
+{
+    return EX_FIELD(ex, insn);
+}
+
+static inline unsigned long ex_fixup(const struct exception_table_entry *ex)
+{
+    return EX_FIELD(ex, fixup);
+}
+
+static void __init cf_check swap_ex(void *a, void *b)
+{
+    struct exception_table_entry *x = a, *y = b, tmp;
+    long delta = b - a;
+
+    tmp = *x;
+    x->insn = y->insn + delta;
+    y->insn = tmp.insn - delta;
+
+    x->fixup = y->fixup + delta;
+    y->fixup = tmp.fixup - delta;
+}
+
+static int cf_check cmp_ex(const void *a, const void *b)
+{
+    const unsigned long insn_a = ex_insn(a);
+    const unsigned long insn_b = ex_insn(b);
+
+    /* avoid overflow */
+    return (insn_a > insn_b) - (insn_a < insn_b);
+}
+
+void init_or_livepatch sort_exception_table(struct exception_table_entry *start,
+                                 const struct exception_table_entry *stop)
+{
+    sort(start, stop - start, sizeof(*start), cmp_ex, swap_ex);
+}
+
+void __init sort_exception_tables(void)
+{
+    sort_exception_table(__start___ex_table, __stop___ex_table);
+}
+
+static void ex_handler_fixup(const struct exception_table_entry *ex,
+                             struct cpu_user_regs *regs)
+{
+    regs->sepc = ex_fixup(ex);
+}
+
+bool fixup_exception(struct cpu_user_regs *regs)
+{
+    unsigned long pc = regs->sepc;
+    const struct virtual_region *region = find_text_region(pc);
+    const struct exception_table_entry *ex;
+    struct exception_table_entry key;
+
+    if ( !region || !region->ex )
+        return false;
+
+    key.insn = pc - (unsigned long)&key.insn;
+
+    ex = bsearch(&key, region->ex, region->ex_end - region->ex, sizeof(key),
+                 cmp_ex);
+
+    if ( !ex )
+        return false;
+
+    ex_handler_fixup(ex, regs);
+
+    return true;
+}
diff --git a/xen/arch/riscv/include/asm/bug.h b/xen/arch/riscv/include/asm/bug.h
index 6ec8adc528a9..e6f286881662 100644
--- a/xen/arch/riscv/include/asm/bug.h
+++ b/xen/arch/riscv/include/asm/bug.h
@@ -9,6 +9,8 @@
 
 #ifndef __ASSEMBLER__
 
+#include <asm/extable.h>
+
 #define BUG_INSTR "unimp"
 
 /*
diff --git a/xen/arch/riscv/include/asm/extable.h b/xen/arch/riscv/include/asm/extable.h
new file mode 100644
index 000000000000..4f50f84e69f2
--- /dev/null
+++ b/xen/arch/riscv/include/asm/extable.h
@@ -0,0 +1,58 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef ASM__RISCV__ASM_EXTABLE_H
+#define ASM__RISCV__ASM_EXTABLE_H
+
+#ifdef __ASSEMBLER__
+
+#define ASM_EXTABLE(insn, fixup)    \
+    .pushsection .ex_table, "a";    \
+    .balign     4;                  \
+    .long       ((insn) - .);       \
+    .long       ((fixup) - .);      \
+    .popsection;
+
+.macro asm_extable, insn, fixup
+    ASM_EXTABLE(\insn, \fixup)
+.endm
+
+#else /* __ASSEMBLER__ */
+
+#include <xen/stringify.h>
+#include <xen/types.h>
+
+struct cpu_user_regs;
+
+#define ASM_EXTABLE(insn, fixup)        \
+    ".pushsection .ex_table, \"a\"\n"   \
+    ".balign    4\n"                    \
+    ".long      ((" #insn ") - .)\n"    \
+    ".long      ((" #fixup ") - .)\n"   \
+    ".popsection\n"
+
+/*
+ * The exception table consists of pairs of relative offsets: the first
+ * is the relative offset to an instruction that is allowed to fault,
+ * and the second is the relative offset at which the program should
+ * continue. No general-purpose registers are modified by the exception
+ * handling mechanism itself, so it is up to the fixup code to handle
+ * any necessary state cleanup.
+ *
+ * The exception table and fixup code live out of line with the main
+ * instruction path. This means when everything is well, we don't even
+ * have to jump over them. Further, they do not intrude on our cache or
+ * tlb entries.
+ */
+struct exception_table_entry {
+    int32_t insn, fixup;
+};
+
+extern struct exception_table_entry __start___ex_table[];
+extern struct exception_table_entry __stop___ex_table[];
+
+void sort_exception_tables(void);
+bool fixup_exception(struct cpu_user_regs *regs);
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* ASM__RISCV__ASM_EXTABLE_H */
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index cae49bb29626..56a0907a855f 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -19,6 +19,7 @@
 
 #include <public/version.h>
 
+#include <asm/extable.h>
 #include <asm/cpufeature.h>
 #include <asm/early_printk.h>
 #include <asm/fixmap.h>
@@ -81,6 +82,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
 
     smp_prepare_boot_cpu();
 
+    sort_exception_tables();
+
     set_cpuid_to_hartid(0, bootcpu_id);
 
     trap_init();
diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index 326f2be62823..d35c013e1399 100644
--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -12,6 +12,7 @@
 #include <xen/sched.h>
 #include <xen/softirq.h>
 
+#include <asm/extable.h>
 #include <asm/cpufeature.h>
 #include <asm/intc.h>
 #include <asm/processor.h>
@@ -217,6 +218,10 @@ void do_trap(struct cpu_user_regs *cpu_regs)
 
             break;
         }
+
+        if ( fixup_exception(cpu_regs) )
+            break;
+
         fallthrough;
     default:
         if ( cause & CAUSE_IRQ_FLAG )
diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S
index 331a7d63d3c9..65f136dce9f7 100644
--- a/xen/arch/riscv/xen.lds.S
+++ b/xen/arch/riscv/xen.lds.S
@@ -74,6 +74,9 @@ SECTIONS
     .data.ro_after_init : {
         __ro_after_init_start = .;
         *(.data.ro_after_init)
+
+        EX_TABLE
+
         . = ALIGN(PAGE_SIZE);
         __ro_after_init_end = .;
     } : text
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index c326538ebbb2..b9e888e5962f 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -113,11 +113,7 @@ SECTIONS
        __ro_after_init_start = .;
        *(.data.ro_after_init)
 
-       . = ALIGN(8);
-       /* Exception table */
-       __start___ex_table = .;
-       *(.ex_table)
-       __stop___ex_table = .;
+       EX_TABLE
 
        . = ALIGN(PAGE_SIZE);
        __ro_after_init_end = .;
diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h
index 136849ecd515..ea11e3fb6213 100644
--- a/xen/include/xen/xen.lds.h
+++ b/xen/include/xen/xen.lds.h
@@ -219,4 +219,10 @@
 #define VPCI_ARRAY
 #endif
 
+#define EX_TABLE                  \
+        . = ALIGN(4);             \
+        __start___ex_table = .;   \
+        *(.ex_table)              \
+        __stop___ex_table = .;
+
 #endif /* __XEN_LDS_H__ */
-- 
2.53.0

[PATCH v2 2/4] xen/riscv: add csr_read_safe() helper

[Oleksii Kurochko]

Accessing some CSRs may trap when the corresponding extension is not
implemented or enabled. Introduce csr_read_safe() which attempts to
read a CSR and relies on the exception table mechanism to safely recover
if the access faults.

This helper allows Xen to probe CSR availability without taking a fatal
trap and will be used for feature detection during early boot as we
can't always rely on what is in riscv,isa string in DTS.

While touching the header, reorder the include directives to follow the
usual Xen style.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v2:
 - s/extables.h/extable.h.
 - s/csr_allowed_read/csr_read_safe() to follow the common nomenclature.
 - asm/asm_inline in csr_allowed_safe().
 - Drop the comment inside csr_allowed_safe().
 - Add ifdef CONFIG_CC_HAS_ASM_GOTO_OUTPUT
 - Drop memory clobber as a memory which is going to be changed is explicitly
   mentioned in output list.
 - Rename local variable error to allowed.
---
 xen/arch/riscv/include/asm/csr.h | 36 +++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/xen/arch/riscv/include/asm/csr.h b/xen/arch/riscv/include/asm/csr.h
index 01876f828981..1d47f70f7251 100644
--- a/xen/arch/riscv/include/asm/csr.h
+++ b/xen/arch/riscv/include/asm/csr.h
@@ -6,8 +6,10 @@
 #ifndef ASM__RISCV__CSR_H
 #define ASM__RISCV__CSR_H
 
-#include <asm/asm.h>
 #include <xen/const.h>
+
+#include <asm/asm.h>
+#include <asm/extable.h>
 #include <asm/riscv_encoding.h>
 
 #ifndef __ASSEMBLER__
@@ -78,6 +80,38 @@
                            : "memory" );                        \
 })
 
+static always_inline bool csr_read_safe(unsigned long csr,
+                                        unsigned long *val)
+{
+#ifdef CONFIG_CC_HAS_ASM_GOTO_OUTPUT
+    asm_inline goto (
+        "1: csrr %[val], %[csr]\n"
+        ASM_EXTABLE(1b, %l[fault])
+        : [val] "=&r" (*val)
+        : [csr] "i" (csr)
+        :
+        : fault );
+
+    return true;
+
+ fault:
+    return false;
+#else
+    bool allowed = false;
+
+    asm_inline volatile (
+        "1: csrr %[val], %[csr]\n"
+        "   li %[allowed], 1\n"
+        "2:\n"
+        ASM_EXTABLE(1b, 2b)
+        : [val] "=&r" (*val), [allowed] "+r" (allowed)
+        : [csr] "i" (csr)
+        : );
+
+    return allowed;
+#endif
+}
+
 #endif /* __ASSEMBLER__ */
 
 #endif /* ASM__RISCV__CSR_H */
-- 
2.53.0

[PATCH v2 3/4] xen/riscv: allow Xen to use SSTC while hiding it from guests

[Oleksii Kurochko]

OpenSBI currently does not advertise the SSTC extension via the device
tree. Additionally, SSTC can no longer be reliably disabled by removing
the "sstc" string from riscv,isa, as 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.

To prevent guests from using SSTC, RISCV_ISA_EXT_sstc is cleared from the
riscv_isa bitmap and in future patches from riscv_isa DTS property.
As a result, the corresponding HENVCFG bit is not set and 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. This also reduces the
number of if statements in reprogram_timer().

The set_xen_timer function pointer is selected based on
csr_read_safe() rather than riscv_isa_extension(). The latter
reflects features supported by both Xen and the guest, while SSTC is
currently only supported for Xen. Therefore, relying solely on
riscv_isa_extension() would not reliably determine whether SSTC can be
used.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/cpufeature.c                 | 33 ++++++++++++++++++++
 xen/arch/riscv/include/asm/cpufeature.h     |  1 +
 xen/arch/riscv/include/asm/riscv_encoding.h |  2 ++
 xen/arch/riscv/time.c                       | 34 ++++++++++++---------
 xen/arch/riscv/vtimer.c                     |  7 ++++-
 5 files changed, 62 insertions(+), 15 deletions(-)

diff --git a/xen/arch/riscv/cpufeature.c b/xen/arch/riscv/cpufeature.c
index 03e27b037be0..823af53ca18e 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,36 @@ void __init riscv_fill_hwcap(void)
         panic("HW capabilities parsing failed: %s\n", failure_msg);
     }
 
+    if ( csr_read_safe(CSR_STIMECMP, &tmp) )
+    {
+        printk("SSTC is detected but is supported only for Xen usage not for "
+               "a guest\n");
+
+        /*
+         * As SSTC for guest isn't supported it is needed temprorary to:
+         *
+         * 1. Clear bit RISCV_ISA_EXT_sstc in riscv_isa as theoretuically it
+         *    could be that OpenSBI (it doesn't pass it now) or whatever ran
+         *    before Xen will add SSTC to riscv,isa string. This bit clear
+         *    won't allow guest to use SSTC extension as vtimer context
+         *    switch and restore isn't ready for that.
+         */
+        __clear_bit(RISCV_ISA_EXT_sstc, riscv_isa);
+
+        /*
+         * 2. 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.
+         *
+         * It should be dropped when SSTC for guests will be supported.
+         */
+        csr_write(CSR_VSTIMECMP, ULONG_MAX);
+#ifdef CONFIG_RISCV_32
+        csr_write(CSR_VSTIMECMPH, ULONG_MAX);
+#endif
+    }
+
     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/riscv_encoding.h b/xen/arch/riscv/include/asm/riscv_encoding.h
index dd15731a86fa..d0d60ba15e62 100644
--- a/xen/arch/riscv/include/asm/riscv_encoding.h
+++ b/xen/arch/riscv/include/asm/riscv_encoding.h
@@ -396,6 +396,8 @@
 #define CSR_VSTVAL			0x243
 #define CSR_VSIP			0x244
 #define CSR_VSATP			0x280
+#define CSR_VSTIMECMP		0x24d
+#define CSR_VSTIMECMPH		0x25d
 
 /* Virtual Interrupts and Interrupt Priorities (H-extension with AIA) */
 #define CSR_HVIEN			0x608
diff --git a/xen/arch/riscv/time.c b/xen/arch/riscv/time.c
index 7efa76fdbcb1..42d547a03e0f 100644
--- a/xen/arch/riscv/time.c
+++ b/xen/arch/riscv/time.c
@@ -13,6 +13,18 @@
 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_write(CSR_STIMECMP, deadline);
+#ifdef CONFIG_RISCV_32
+    csr_write(CSR_STIMECMPH, deadline >> 32);
+#endif
+
+    return 0;
+}
+
+static 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 +73,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 */
@@ -85,10 +84,17 @@ int reprogram_timer(s_time_t timeout)
 
 void __init preinit_xen_time(void)
 {
+    unsigned long tmp;
+
     if ( acpi_disabled )
         preinit_dt_xen_time();
     else
         panic("%s: ACPI isn't supported\n", __func__);
 
     boot_clock_cycles = get_cycles();
+
+    if ( csr_read_safe(CSR_STIMECMP, &tmp) )
+        set_xen_timer = sstc_set_xen_timer;
+    else
+        set_xen_timer = sbi_set_timer;
 }
diff --git a/xen/arch/riscv/vtimer.c b/xen/arch/riscv/vtimer.c
index afd8a53a7387..c065052afeb7 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)
@@ -75,12 +76,16 @@ void vtimer_ctxt_switch_from(struct vcpu *p)
 {
     ASSERT(!is_idle_vcpu(p));
 
-    /* Nothing to do at the moment as SSTC isn't supported now. */
+    BUG_ON(riscv_isa_extension_available(NULL, RISCV_ISA_EXT_sstc));
+
+    /* Nothing to do at the moment as SSTC for guests isn't supported now */
 }
 
 void vtimer_ctxt_switch_to(struct vcpu *n)
 {
     ASSERT(!is_idle_vcpu(n));
 
+    BUG_ON(riscv_isa_extension_available(NULL, RISCV_ISA_EXT_sstc));
+
     migrate_timer(&n->arch.vtimer.timer, n->processor);
 }
-- 
2.53.0

[PATCH v2 4/4] xen/riscv: init_csr_masks()-related improvements

[Oleksii Kurochko]

There is no reason to use _UL() in define-s sitting in C file hence use UL
suffix instead.

Drop 3d argument of INIT_CSR_MASK() and INIT_RO_ONE_MASK() to reduce risk
of incomplete editing after copy-and-paste, or other typo-ing.

Use _VALID_ infix instead of _AVAIL_ as the mask identifies architecturally
defined bits, not bits available for software use.

Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/riscv/domain.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/xen/arch/riscv/domain.c b/xen/arch/riscv/domain.c
index c327f44d07ca..c77be3b827eb 100644
--- a/xen/arch/riscv/domain.c
+++ b/xen/arch/riscv/domain.c
@@ -42,10 +42,10 @@ struct csr_masks {
 
 static struct csr_masks __ro_after_init csr_masks;
 
-#define HEDELEG_AVAIL_MASK ULONG_MAX
-#define HIDELEG_AVAIL_MASK ULONG_MAX
-#define HENVCFG_AVAIL_MASK _UL(0xE0000003000000FF)
-#define HSTATEEN0_AVAIL_MASK _UL(0xDE00000000000007)
+#define HEDELEG_VALID_MASK ULONG_MAX
+#define HIDELEG_VALID_MASK ULONG_MAX
+#define HENVCFG_VALID_MASK 0xe0000003000000ffUL
+#define HSTATEEN0_VALID_MASK 0xde00000000000007UL
 
 void __init init_csr_masks(void)
 {
@@ -57,25 +57,26 @@ void __init init_csr_masks(void)
      * fields that must be preserved. Any write to the full register must
      * therefore retain the original values of those fields.
      */
-#define INIT_CSR_MASK(csr, field, mask) do { \
-        register_t old = csr_read_set(CSR_ ## csr, mask); \
+#define INIT_CSR_MASK(csr, field) do { \
+        register_t old = csr_read_set(CSR_ ## csr, csr ## _VALID_MASK); \
         csr_masks.field = csr_swap(CSR_ ## csr, old); \
     } while (0)
 
-#define INIT_RO_ONE_MASK(csr, field, mask) do { \
-        register_t old = csr_read_clear(CSR_ ## csr, mask); \
-        csr_masks.ro_one.field = csr_swap(CSR_ ## csr, old) & mask; \
+#define INIT_RO_ONE_MASK(csr, field) do { \
+        register_t old = csr_read_clear(CSR_ ## csr, csr ## _VALID_MASK); \
+        csr_masks.ro_one.field = csr_swap(CSR_ ## csr, old) & \
+                                 csr ## _VALID_MASK; \
     } while (0)
 
-    INIT_CSR_MASK(HEDELEG, hedeleg, HEDELEG_AVAIL_MASK);
-    INIT_CSR_MASK(HIDELEG, hideleg, HIDELEG_AVAIL_MASK);
+    INIT_CSR_MASK(HEDELEG, hedeleg);
+    INIT_CSR_MASK(HIDELEG, hideleg);
 
-    INIT_CSR_MASK(HENVCFG, henvcfg, HENVCFG_AVAIL_MASK);
+    INIT_CSR_MASK(HENVCFG, henvcfg);
 
     if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_smstateen) )
     {
-        INIT_CSR_MASK(HSTATEEN0, hstateen0, HSTATEEN0_AVAIL_MASK);
-        INIT_RO_ONE_MASK(HSTATEEN0, hstateen0, HSTATEEN0_AVAIL_MASK);
+        INIT_CSR_MASK(HSTATEEN0, hstateen0);
+        INIT_RO_ONE_MASK(HSTATEEN0, hstateen0);
     }
 
 #undef INIT_CSR_MASK
-- 
2.53.0

Re: [PATCH v2 4/4] xen/riscv: init_csr_masks()-related improvements

[Jan Beulich]

On 31.03.2026 21:04, Oleksii Kurochko wrote:
> There is no reason to use _UL() in define-s sitting in C file hence use UL
> suffix instead.
> 
> Drop 3d argument of INIT_CSR_MASK() and INIT_RO_ONE_MASK() to reduce risk
> of incomplete editing after copy-and-paste, or other typo-ing.
> 
> Use _VALID_ infix instead of _AVAIL_ as the mask identifies architecturally
> defined bits, not bits available for software use.
> 
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Seeing this is ready to go in, am I overlooking any dependency on earlier
patches, or could this indeed go in right away?

Jan

Re: [PATCH v2 4/4] xen/riscv: init_csr_masks()-related improvements

[Oleksii Kurochko]

On 4/1/26 8:19 AM, Jan Beulich wrote:
> On 31.03.2026 21:04, Oleksii Kurochko wrote:
>> There is no reason to use _UL() in define-s sitting in C file hence use UL
>> suffix instead.
>>
>> Drop 3d argument of INIT_CSR_MASK() and INIT_RO_ONE_MASK() to reduce risk
>> of incomplete editing after copy-and-paste, or other typo-ing.
>>
>> Use _VALID_ infix instead of _AVAIL_ as the mask identifies architecturally
>> defined bits, not bits available for software use.
>>
>> Suggested-by: Jan Beulich <jbeulich@suse.com>
>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Seeing this is ready to go in, am I overlooking any dependency on earlier
> patches, or could this indeed go in right away?

No, there is no any dependency, it could go earlier then other patches 
of this patch series.

Thanks.

~ Oleksii

Re: [PATCH v2 1/4] xen/riscv: add exception table support

[Jan Beulich]

On 31.03.2026 21:04, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/extable.c
> @@ -0,0 +1,85 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#include <xen/init.h>
> +#include <xen/bsearch.h>
> +#include <xen/lib.h>
> +#include <xen/livepatch.h>
> +#include <xen/sort.h>
> +#include <xen/virtual_region.h>
> +
> +#include <asm/extable.h>
> +#include <asm/processor.h>
> +
> +#define EX_FIELD(ptr, field) ((unsigned long)&(ptr)->field + (ptr)->field)
> +
> +static inline unsigned long ex_insn(const struct exception_table_entry *ex)
> +{
> +    return EX_FIELD(ex, insn);
> +}
> +
> +static inline unsigned long ex_fixup(const struct exception_table_entry *ex)
> +{
> +    return EX_FIELD(ex, fixup);
> +}
> +
> +static void __init cf_check swap_ex(void *a, void *b)
> +{
> +    struct exception_table_entry *x = a, *y = b, tmp;
> +    long delta = b - a;
> +
> +    tmp = *x;
> +    x->insn = y->insn + delta;
> +    y->insn = tmp.insn - delta;
> +
> +    x->fixup = y->fixup + delta;
> +    y->fixup = tmp.fixup - delta;
> +}
> +
> +static int cf_check cmp_ex(const void *a, const void *b)
> +{
> +    const unsigned long insn_a = ex_insn(a);
> +    const unsigned long insn_b = ex_insn(b);
> +
> +    /* avoid overflow */
> +    return (insn_a > insn_b) - (insn_a < insn_b);

What is the (slightly malformed) comment about? I don't see anything close
to possibly causing overflow here.

> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/extable.h
> @@ -0,0 +1,58 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef ASM__RISCV__ASM_EXTABLE_H
> +#define ASM__RISCV__ASM_EXTABLE_H
> +
> +#ifdef __ASSEMBLER__
> +
> +#define ASM_EXTABLE(insn, fixup)    \
> +    .pushsection .ex_table, "a";    \
> +    .balign     4;                  \
> +    .long       ((insn) - .);       \
> +    .long       ((fixup) - .);      \

For readability's sake I'm generally advocating for having enough, but
not more parentheses than necessary. What's the purpose of the outer pair
here and ...

> +    .popsection;
> +
> +.macro asm_extable, insn, fixup
> +    ASM_EXTABLE(\insn, \fixup)
> +.endm
> +
> +#else /* __ASSEMBLER__ */
> +
> +#include <xen/stringify.h>
> +#include <xen/types.h>
> +
> +struct cpu_user_regs;
> +
> +#define ASM_EXTABLE(insn, fixup)        \
> +    ".pushsection .ex_table, \"a\"\n"   \
> +    ".balign    4\n"                    \
> +    ".long      ((" #insn ") - .)\n"    \
> +    ".long      ((" #fixup ") - .)\n"   \

... here?

I'm also uncertain about the use of .long (generally in RISC-V code, and
really also in some other architectures). Imo, considering suffixes used
in the instruction set (e.g. load/store insns or OP-32 ones in RV64) .word
may be the more expressive directive.

Preferably with the adjustments:
Acked-by: Jan Beulich <jbeulich@suse.com>
Happy to carry out while committing, provided you agree.

Jan

Re: [PATCH v2 2/4] xen/riscv: add csr_read_safe() helper

[Jan Beulich]

On 31.03.2026 21:04, Oleksii Kurochko wrote:
> @@ -78,6 +80,38 @@
>                             : "memory" );                        \
>  })
>  
> +static always_inline bool csr_read_safe(unsigned long csr,
> +                                        unsigned long *val)
> +{
> +#ifdef CONFIG_CC_HAS_ASM_GOTO_OUTPUT
> +    asm_inline goto (
> +        "1: csrr %[val], %[csr]\n"
> +        ASM_EXTABLE(1b, %l[fault])
> +        : [val] "=&r" (*val)

Why the & when there's only a single insn?

> +        : [csr] "i" (csr)
> +        :
> +        : fault );
> +
> +    return true;
> +
> + fault:
> +    return false;
> +#else
> +    bool allowed = false;
> +
> +    asm_inline volatile (
> +        "1: csrr %[val], %[csr]\n"
> +        "   li %[allowed], 1\n"
> +        "2:\n"
> +        ASM_EXTABLE(1b, 2b)
> +        : [val] "=&r" (*val), [allowed] "+r" (allowed)
> +        : [csr] "i" (csr)
> +        : );

Why the excess colon?

With these adjusted (again happy to do so while committing, so long
as you agree):
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v2 3/4] xen/riscv: allow Xen to use SSTC while hiding it from guests

[Jan Beulich]

On 31.03.2026 21:04, Oleksii Kurochko wrote:
> @@ -495,6 +498,36 @@ void __init riscv_fill_hwcap(void)
>          panic("HW capabilities parsing failed: %s\n", failure_msg);
>      }
>  
> +    if ( csr_read_safe(CSR_STIMECMP, &tmp) )
> +    {
> +        printk("SSTC is detected but is supported only for Xen usage not for "
> +               "a guest\n");

Please don't wrap format strings. Them going slightly beyond 80 columns is okay.
Them going much beyond that is a good indication that you want to consider re-
wording. (Here e.g. "SSTC detected; supported for Xen use, but not for guests".)

I question though whether something like this needs logging.

> +        /*
> +         * As SSTC for guest isn't supported it is needed temprorary to:

Nit: temporary

> +         *
> +         * 1. Clear bit RISCV_ISA_EXT_sstc in riscv_isa as theoretuically it

Nit: theoretically

> +         *    could be that OpenSBI (it doesn't pass it now) or whatever ran
> +         *    before Xen will add SSTC to riscv,isa string. This bit clear
> +         *    won't allow guest to use SSTC extension as vtimer context
> +         *    switch and restore isn't ready for that.
> +         */
> +        __clear_bit(RISCV_ISA_EXT_sstc, riscv_isa);

Seeing your other series, shouldn't this instead be done without affecting
riscv_isa? The BUG_ON()s in vtimer.x therefore also look inappropriate.

> @@ -61,20 +73,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 */
> @@ -85,10 +84,17 @@ int reprogram_timer(s_time_t timeout)
>  
>  void __init preinit_xen_time(void)
>  {
> +    unsigned long tmp;
> +
>      if ( acpi_disabled )
>          preinit_dt_xen_time();
>      else
>          panic("%s: ACPI isn't supported\n", __func__);
>  
>      boot_clock_cycles = get_cycles();
> +
> +    if ( csr_read_safe(CSR_STIMECMP, &tmp) )
> +        set_xen_timer = sstc_set_xen_timer;
> +    else
> +        set_xen_timer = sbi_set_timer;
>  }

Doesn't all of this together eliminate the need for sbi_set_timer as a
separate global variable?

Jan

Re: [PATCH v2 1/4] xen/riscv: add exception table support

[Oleksii Kurochko]

On 4/2/26 8:24 AM, Jan Beulich wrote:
> On 31.03.2026 21:04, Oleksii Kurochko wrote:
>> --- /dev/null
>> +++ b/xen/arch/riscv/extable.c
>> @@ -0,0 +1,85 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +#include <xen/init.h>
>> +#include <xen/bsearch.h>
>> +#include <xen/lib.h>
>> +#include <xen/livepatch.h>
>> +#include <xen/sort.h>
>> +#include <xen/virtual_region.h>
>> +
>> +#include <asm/extable.h>
>> +#include <asm/processor.h>
>> +
>> +#define EX_FIELD(ptr, field) ((unsigned long)&(ptr)->field + (ptr)->field)
>> +
>> +static inline unsigned long ex_insn(const struct exception_table_entry *ex)
>> +{
>> +    return EX_FIELD(ex, insn);
>> +}
>> +
>> +static inline unsigned long ex_fixup(const struct exception_table_entry *ex)
>> +{
>> +    return EX_FIELD(ex, fixup);
>> +}
>> +
>> +static void __init cf_check swap_ex(void *a, void *b)
>> +{
>> +    struct exception_table_entry *x = a, *y = b, tmp;
>> +    long delta = b - a;
>> +
>> +    tmp = *x;
>> +    x->insn = y->insn + delta;
>> +    y->insn = tmp.insn - delta;
>> +
>> +    x->fixup = y->fixup + delta;
>> +    y->fixup = tmp.fixup - delta;
>> +}
>> +
>> +static int cf_check cmp_ex(const void *a, const void *b)
>> +{
>> +    const unsigned long insn_a = ex_insn(a);
>> +    const unsigned long insn_b = ex_insn(b);
>> +
>> +    /* avoid overflow */
>> +    return (insn_a > insn_b) - (insn_a < insn_b);
> 
> What is the (slightly malformed) comment about? I don't see anything close
> to possibly causing overflow here.

Originally, I thought to imeplement this function something like:
   return insn_a - insn_b;

It'd get integer overflow when insn_a is a very small number and insn_b 
is very large.

It could drop the comment to avoid confusion.


> 
>> --- /dev/null
>> +++ b/xen/arch/riscv/include/asm/extable.h
>> @@ -0,0 +1,58 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +#ifndef ASM__RISCV__ASM_EXTABLE_H
>> +#define ASM__RISCV__ASM_EXTABLE_H
>> +
>> +#ifdef __ASSEMBLER__
>> +
>> +#define ASM_EXTABLE(insn, fixup)    \
>> +    .pushsection .ex_table, "a";    \
>> +    .balign     4;                  \
>> +    .long       ((insn) - .);       \
>> +    .long       ((fixup) - .);      \
> 
> For readability's sake I'm generally advocating for having enough, but
> not more parentheses than necessary. What's the purpose of the outer pair
> here and ...
> 
>> +    .popsection;
>> +
>> +.macro asm_extable, insn, fixup
>> +    ASM_EXTABLE(\insn, \fixup)
>> +.endm
>> +
>> +#else /* __ASSEMBLER__ */
>> +
>> +#include <xen/stringify.h>
>> +#include <xen/types.h>
>> +
>> +struct cpu_user_regs;
>> +
>> +#define ASM_EXTABLE(insn, fixup)        \
>> +    ".pushsection .ex_table, \"a\"\n"   \
>> +    ".balign    4\n"                    \
>> +    ".long      ((" #insn ") - .)\n"    \
>> +    ".long      ((" #fixup ") - .)\n"   \
> 
> ... here?

It looked visually better to me but I am okay to drop them.

> 
> I'm also uncertain about the use of .long (generally in RISC-V code, and
> really also in some other architectures). Imo, considering suffixes used
> in the instruction set (e.g. load/store insns or OP-32 ones in RV64) .word
> may be the more expressive directive.

Agree, we could use .word instead of .long.

> 
> Preferably with the adjustments:
> Acked-by: Jan Beulich <jbeulich@suse.com>

Thanks a lot.

> Happy to carry out while committing, provided you agree.

I would be happy with that.

~ Oleksii

Re: [PATCH v2 2/4] xen/riscv: add csr_read_safe() helper

[Oleksii Kurochko]

On 4/2/26 8:30 AM, Jan Beulich wrote:
> On 31.03.2026 21:04, Oleksii Kurochko wrote:
>> @@ -78,6 +80,38 @@
>>                              : "memory" );                        \
>>   })
>>   
>> +static always_inline bool csr_read_safe(unsigned long csr,
>> +                                        unsigned long *val)
>> +{
>> +#ifdef CONFIG_CC_HAS_ASM_GOTO_OUTPUT
>> +    asm_inline goto (
>> +        "1: csrr %[val], %[csr]\n"
>> +        ASM_EXTABLE(1b, %l[fault])
>> +        : [val] "=&r" (*val)
> 
> Why the & when there's only a single insn?

Agree, the & isn't needed here.

> 
>> +        : [csr] "i" (csr)
>> +        :
>> +        : fault );
>> +
>> +    return true;
>> +
>> + fault:
>> +    return false;
>> +#else
>> +    bool allowed = false;
>> +
>> +    asm_inline volatile (
>> +        "1: csrr %[val], %[csr]\n"
>> +        "   li %[allowed], 1\n"
>> +        "2:\n"
>> +        ASM_EXTABLE(1b, 2b)
>> +        : [val] "=&r" (*val), [allowed] "+r" (allowed)
>> +        : [csr] "i" (csr)
>> +        : );
> 
> Why the excess colon?

Missed to proper cleanup. It could be also dropped.

> 
> With these adjusted (again happy to do so while committing, so long
> as you agree):

I would be happy with that.

> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks!

~ Oleksii

Re: [PATCH v2 1/4] xen/riscv: add exception table support

[Andrew Cooper]

On 08/04/2026 10:29 am, Oleksii Kurochko wrote:
>
>
> On 4/2/26 8:24 AM, Jan Beulich wrote:
>> On 31.03.2026 21:04, Oleksii Kurochko wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/riscv/extable.c
>>> @@ -0,0 +1,85 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +
>>> +#include <xen/init.h>
>>> +#include <xen/bsearch.h>
>>> +#include <xen/lib.h>
>>> +#include <xen/livepatch.h>
>>> +#include <xen/sort.h>
>>> +#include <xen/virtual_region.h>
>>> +
>>> +#include <asm/extable.h>
>>> +#include <asm/processor.h>
>>> +
>>> +#define EX_FIELD(ptr, field) ((unsigned long)&(ptr)->field +
>>> (ptr)->field)
>>> +
>>> +static inline unsigned long ex_insn(const struct
>>> exception_table_entry *ex)
>>> +{
>>> +    return EX_FIELD(ex, insn);
>>> +}
>>> +
>>> +static inline unsigned long ex_fixup(const struct
>>> exception_table_entry *ex)
>>> +{
>>> +    return EX_FIELD(ex, fixup);
>>> +}
>>> +
>>> +static void __init cf_check swap_ex(void *a, void *b)
>>> +{
>>> +    struct exception_table_entry *x = a, *y = b, tmp;
>>> +    long delta = b - a;
>>> +
>>> +    tmp = *x;
>>> +    x->insn = y->insn + delta;
>>> +    y->insn = tmp.insn - delta;
>>> +
>>> +    x->fixup = y->fixup + delta;
>>> +    y->fixup = tmp.fixup - delta;
>>> +}
>>> +
>>> +static int cf_check cmp_ex(const void *a, const void *b)
>>> +{
>>> +    const unsigned long insn_a = ex_insn(a);
>>> +    const unsigned long insn_b = ex_insn(b);
>>> +
>>> +    /* avoid overflow */
>>> +    return (insn_a > insn_b) - (insn_a < insn_b);
>>
>> What is the (slightly malformed) comment about? I don't see anything
>> close
>> to possibly causing overflow here.
>
> Originally, I thought to imeplement this function something like:
>   return insn_a - insn_b;
>
> It'd get integer overflow when insn_a is a very small number and
> insn_b is very large.
>
> It could drop the comment to avoid confusion.

"insn_a - insn_b" is a very common bug in cmp() functions.  It does
cause the sort/search to malfunction when the subtraction overflows.

However, the form you've got (a > b) - (b > a) is a very common correct
form.  I'd drop the comment.

~Andrew

Re: [PATCH v2 3/4] xen/riscv: allow Xen to use SSTC while hiding it from guests

[Oleksii Kurochko]

On 4/2/26 8:41 AM, Jan Beulich wrote:
> On 31.03.2026 21:04, Oleksii Kurochko wrote:
>> @@ -495,6 +498,36 @@ void __init riscv_fill_hwcap(void)
>>           panic("HW capabilities parsing failed: %s\n", failure_msg);
>>       }
>>   
>> +    if ( csr_read_safe(CSR_STIMECMP, &tmp) )
>> +    {
>> +        printk("SSTC is detected but is supported only for Xen usage not for "
>> +               "a guest\n");
> 
> Please don't wrap format strings. Them going slightly beyond 80 columns is okay.
> Them going much beyond that is a good indication that you want to consider re-
> wording. (Here e.g. "SSTC detected; supported for Xen use, but not for guests".)

I will reword log message in the form you suggested.

> 
> I question though whether something like this needs logging.

It is just a debug reminder that something should be additionally done. 
I will do it dprintk() so it won't appear in release builds.

> 
>> +        /*
>> +         * As SSTC for guest isn't supported it is needed temprorary to:
> 
> Nit: temporary
> 
>> +         *
>> +         * 1. Clear bit RISCV_ISA_EXT_sstc in riscv_isa as theoretuically it
> 
> Nit: theoretically
> 
>> +         *    could be that OpenSBI (it doesn't pass it now) or whatever ran
>> +         *    before Xen will add SSTC to riscv,isa string. This bit clear
>> +         *    won't allow guest to use SSTC extension as vtimer context
>> +         *    switch and restore isn't ready for that.
>> +         */
>> +        __clear_bit(RISCV_ISA_EXT_sstc, riscv_isa);
> 
> Seeing your other series, shouldn't this instead be done without affecting
> riscv_isa? The BUG_ON()s in vtimer.x therefore also look inappropriate.

It is incorrect to use __clear_bit(). What should be used instead is 
__set_bit(), because in the current boot process OpenSBI does not add 
"sstc" to the riscv,isa string. Therefore, we need to set the 
RISCV_ISA_EXT_sstc bit manually.

This change will affect BUG_ON() checks in vtimer.c, which I plan to 
remove. If SSTC is not supported for the guest, there is nothing we can 
do anyway. It might make sense to reintroduce these BUG_ON() checks once 
an unsupported bitmap is implemented [1]. At that point, we could have 
something like the following in the vtimer save and restore context 
functions in vtimer.c:

BUG_ON(!riscv_isa_extension_available(guest_unsupp_bmp, 
RISCV_ISA_EXT_sstc));

On the other hand, using BUG_ON() in the vtimer save and restore 
functions is of limited value. If SSTC support for guests is added in 
the future, these functions will need to be updated anyway, so such 
checks may become redundant.

[1] 
https://lore.kernel.org/xen-devel/007c0a0243ac7ff1d1ab3faa4ebcdd6fcd14e485.1773157782.git.oleksii.kurochko@gmail.com/


> 
>> @@ -61,20 +73,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 */
>> @@ -85,10 +84,17 @@ int reprogram_timer(s_time_t timeout)
>>   
>>   void __init preinit_xen_time(void)
>>   {
>> +    unsigned long tmp;
>> +
>>       if ( acpi_disabled )
>>           preinit_dt_xen_time();
>>       else
>>           panic("%s: ACPI isn't supported\n", __func__);
>>   
>>       boot_clock_cycles = get_cycles();
>> +
>> +    if ( csr_read_safe(CSR_STIMECMP, &tmp) )
>> +        set_xen_timer = sstc_set_xen_timer;
>> +    else
>> +        set_xen_timer = sbi_set_timer;
>>   }
> 
> Doesn't all of this together eliminate the need for sbi_set_timer as a
> separate global variable?
There's still a need for that SBI-level dispatch. However, sbi_set_timer 
doesn't need to be a global variable (exported from sbi.h). Since the 
only external user after this patch is the time.c, sbi_set_timer could 
be refactored into a plain static internal pointer with a non-static 
wrapper function:

// sbi.c — keep dispatch internal
static int (* __ro_after_init sbi_set_timer_fn)(uint64_t) = 
sbi_set_timer_v01;

int cf_check sbi_set_timer(uint64_t stime_value)
{
     return sbi_set_timer_fn(stime_value);
}

Do you mean this?

Thanks.

~ Oleksii

Re: [PATCH v2 3/4] xen/riscv: allow Xen to use SSTC while hiding it from guests

[Jan Beulich]

On 08.04.2026 12:58, Oleksii Kurochko wrote:
> On 4/2/26 8:41 AM, Jan Beulich wrote:
>> On 31.03.2026 21:04, Oleksii Kurochko wrote:
>>> @@ -61,20 +73,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 */
>>> @@ -85,10 +84,17 @@ int reprogram_timer(s_time_t timeout)
>>>   
>>>   void __init preinit_xen_time(void)
>>>   {
>>> +    unsigned long tmp;
>>> +
>>>       if ( acpi_disabled )
>>>           preinit_dt_xen_time();
>>>       else
>>>           panic("%s: ACPI isn't supported\n", __func__);
>>>   
>>>       boot_clock_cycles = get_cycles();
>>> +
>>> +    if ( csr_read_safe(CSR_STIMECMP, &tmp) )
>>> +        set_xen_timer = sstc_set_xen_timer;
>>> +    else
>>> +        set_xen_timer = sbi_set_timer;
>>>   }
>>
>> Doesn't all of this together eliminate the need for sbi_set_timer as a
>> separate global variable?
> There's still a need for that SBI-level dispatch. However, sbi_set_timer 
> doesn't need to be a global variable (exported from sbi.h). Since the 
> only external user after this patch is the time.c, sbi_set_timer could 
> be refactored into a plain static internal pointer with a non-static 
> wrapper function:
> 
> // sbi.c — keep dispatch internal
> static int (* __ro_after_init sbi_set_timer_fn)(uint64_t) = 
> sbi_set_timer_v01;
> 
> int cf_check sbi_set_timer(uint64_t stime_value)
> {
>      return sbi_set_timer_fn(stime_value);
> }
> 
> Do you mean this?

No. Why is it that we'd still need both set_xen_timer and sbi_set_timer
as distinct variables?

Jan

Re: [PATCH v2 3/4] xen/riscv: allow Xen to use SSTC while hiding it from guests

[Oleksii Kurochko]

On 4/8/26 1:25 PM, Jan Beulich wrote:
> On 08.04.2026 12:58, Oleksii Kurochko wrote:
>> On 4/2/26 8:41 AM, Jan Beulich wrote:
>>> On 31.03.2026 21:04, Oleksii Kurochko wrote:
>>>> @@ -61,20 +73,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 */
>>>> @@ -85,10 +84,17 @@ int reprogram_timer(s_time_t timeout)
>>>>    
>>>>    void __init preinit_xen_time(void)
>>>>    {
>>>> +    unsigned long tmp;
>>>> +
>>>>        if ( acpi_disabled )
>>>>            preinit_dt_xen_time();
>>>>        else
>>>>            panic("%s: ACPI isn't supported\n", __func__);
>>>>    
>>>>        boot_clock_cycles = get_cycles();
>>>> +
>>>> +    if ( csr_read_safe(CSR_STIMECMP, &tmp) )
>>>> +        set_xen_timer = sstc_set_xen_timer;
>>>> +    else
>>>> +        set_xen_timer = sbi_set_timer;
>>>>    }
>>>
>>> Doesn't all of this together eliminate the need for sbi_set_timer as a
>>> separate global variable?
>> There's still a need for that SBI-level dispatch. However, sbi_set_timer
>> doesn't need to be a global variable (exported from sbi.h). Since the
>> only external user after this patch is the time.c, sbi_set_timer could
>> be refactored into a plain static internal pointer with a non-static
>> wrapper function:
>>
>> // sbi.c — keep dispatch internal
>> static int (* __ro_after_init sbi_set_timer_fn)(uint64_t) =
>> sbi_set_timer_v01;
>>
>> int cf_check sbi_set_timer(uint64_t stime_value)
>> {
>>       return sbi_set_timer_fn(stime_value);
>> }
>>
>> Do you mean this?
> 
> No. Why is it that we'd still need both set_xen_timer and sbi_set_timer
> as distinct variables?

We don't need, it just a question of design I think.

extern int (* __ro_after_init sbi_set_timer)(uint64_t stime_value) could 
be renamed to set_xen_timer.

And then re-init set_xen_timer here in preinit_xen_time() if SSTC is 
available:
   if ( csr_read_safe(CSR_STIMECMP, &tmp) )
      set_xen_timer = sstc_set_xen_timer;

~ Oleksii