[PATCH v7 00/14] xen/riscv: introduce vtimer related things

[PATCH v7 00/14] xen/riscv: introduce vtimer related things

[Oleksii Kurochko]

This patch series introduces the components necessary to implement a virtual
timer (vtimer).

Since the SSTC extension is not supported by Xen, an emulated (SBI-based)
timer is required. To address this, a virtual timer built on Xen’s timer
infrastructure is introduced, with save/restore support and SBI-based
programming.

To provide full guest software–based timer support, the following components
are also introduced:
- arch_vcpu_{create,destroy}() to initialize the virtual timer and other
  vCPU-related state not directly tied to timer functionality. As part of this
  work, struct arch_vcpu is introduced to describe the internal state of a
  virtual CPU, along with vcpu_csr_init() to initialize the relevant CSR state.
- Support functions required by the virtual timer, including:
  - vcpu_kick(), and a stub implementation of smp_send_event_check_mask()
    (since SMP is not yet supported in Xen), which is used by vcpu_kick().
  - Support for guest timer programming via interception of the SBI legacy
    SET_TIMER call from guest.
  - Implement reprogram_timer() using introduced sbi_set_timer().
  - Initial lockless tracking of pending vCPU interrupts using atomic bitmaps.
- Handling of hypervisor timer interrupts and dispatch into Xen’s generic timer
  softirq.

CI tests: https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/2368929819
---
Changes in v7:
 - Merged to upstream/staging:
   - xen/time: move ticks<->ns helpers to common code
 - Add Acked-by for Patch 2: xen/riscv: implement vcpu_csr_init().
 - Address comments from ML for patch 1 and 3.
 - Add new patch to detect availabilty of SSTC. 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.
---
Changes in v6:
 - Address comments from ML for patch 1 and 3, and minor update of patch 2
   because of introduced ro_one sub-struct in patch 1.
---
Changes in v5:
 - Merged to upstream/staging:
     xen/riscv: implement arch_vcpu_{create,destroy}()
     xen/riscv: build setup code as .init
 - Rebase this patch series on top of:
     https://lore.kernel.org/xen-devel/5756356294dc066d835269334d0f3347fe24cec4.1771504676.git.oleksii.kurochko@gmail.com/T/#u
   There is no logical dependency, only the potential merge conflict question.
 - Address comments from ML.
---
Changes in v4:
 - Merged to upstream/staging:
     xen/riscv: avoid reading hstateen0 when Smstateen is not implemented
 - Address other comments from ML.
---
Changes in v3:
 - Squash patch "xen/riscv: introduce struct arch_vcpu" into other
   patches of the patch series.
 - Merged to staging:
   - xen/riscv: implement stub for smp_send_event_check_mask()
 - Address other comments from ML.
---
Changes in v2:
 - Add consumer part of tracking of pending vCPU interrupts.
 - Split patch "xen/riscv: init tasklet subsystem" to two.
 - Patches were acked:
   - xen/riscv: introduce vcpu_kick() implementation
   - xen/riscv: implement SBI legacy SET_TIMER support for guests
 - All other changes are patch-specific. Please check them.
---

Oleksii Kurochko (14):
  xen/riscv: detect and store supported hypervisor CSR bits at boot
  xen/riscv: implement vcpu_csr_init()
  xen/riscv: introduce tracking of pending vCPU interrupts, part 1
  xen/riscv: introduce tracking of pending vCPU interrupts, part 2
  xen/riscv: introduce basic vtimer infrastructure for guests
  xen/riscv: introduce vcpu_kick() implementation
  xen/riscv: add vtimer context switch helpers
  xen/riscv: implement SBI legacy SET_TIMER support for guests
  xen/riscv: introduce sbi_set_timer()
  xen/riscv: implement reprogram_timer() via SBI
  xen/riscv: handle hypervisor timer interrupts
  xen/riscv: init tasklet subsystem
  xen/riscv: implement sync_vcpu_execstate()
  xen/riscv: Disable SSTC extension and add trap-based CSR probing

 automation/scripts/qemu-smoke-riscv64.sh    |   2 +-
 xen/arch/riscv/Makefile                     |   1 +
 xen/arch/riscv/cpufeature.c                 |   8 +
 xen/arch/riscv/domain.c                     | 244 +++++++++++++++++++-
 xen/arch/riscv/entry.S                      |  24 ++
 xen/arch/riscv/include/asm/Makefile         |   1 -
 xen/arch/riscv/include/asm/csr.h            |  32 +++
 xen/arch/riscv/include/asm/current.h        |   2 +
 xen/arch/riscv/include/asm/domain.h         |  32 +++
 xen/arch/riscv/include/asm/perfc_defn.h     |   3 +
 xen/arch/riscv/include/asm/riscv_encoding.h |   2 +
 xen/arch/riscv/include/asm/sbi.h            |  22 ++
 xen/arch/riscv/include/asm/setup.h          |   2 +
 xen/arch/riscv/include/asm/traps.h          |   7 +
 xen/arch/riscv/include/asm/vtimer.h         |  20 ++
 xen/arch/riscv/riscv64/asm-offsets.c        |   7 +-
 xen/arch/riscv/sbi.c                        |  40 +++-
 xen/arch/riscv/setup.c                      |   5 +
 xen/arch/riscv/stubs.c                      |  20 --
 xen/arch/riscv/time.c                       |  43 ++++
 xen/arch/riscv/traps.c                      |  20 ++
 xen/arch/riscv/vsbi/legacy-extension.c      |   6 +
 xen/arch/riscv/vtimer.c                     |  86 +++++++
 23 files changed, 603 insertions(+), 26 deletions(-)
 create mode 100644 xen/arch/riscv/include/asm/perfc_defn.h
 create mode 100644 xen/arch/riscv/include/asm/vtimer.h
 create mode 100644 xen/arch/riscv/vtimer.c

-- 
2.53.0

[PATCH v7 01/14] xen/riscv: detect and store supported hypervisor CSR bits at boot

[Oleksii Kurochko]

Some hypervisor CSRs expose optional functionality and may not implement
all architectural bits. Writing unsupported bits can either be ignored
or raise an exception depending on the platform.

Detect the set of writable bits for selected hypervisor CSRs at boot and
store the resulting masks for later use. This allows safely programming
these CSRs during vCPU context switching and avoids relying on hardcoded
architectural assumptions.

Use csr_read()&csr_write() instead of csr_swap()+all ones mask as some
CSR registers have WPRI fields which should be preserved during write
operation.

Also, ro_one struct is introduced to cover the cases when a bit in CSR
register (at the momemnt, it is only hstateen0) may be r/o-one to have
hypervisor view of register seen by guest correct.

Masks are calculated at the moment only for hedeleg, henvcfg, hideleg,
hstateen0 registers as only them are going to be used in the follow up
patch.

If the Smstateen extension is not implemented, hstateen0 cannot be read
because the register is considered non-existent. Instructions that attempt
to access a CSR that is not implemented or not visible in the current mode
are reserved and will raise an illegal-instruction exception.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V7:
 - Use csr_read_set() in INIT_CSR_MASK() instead of csr_read()+csr_write().
 - Add undef of INIT_CSR_MASK().
 - Move local variable old above INIT_CSR_MASK().
 - Introduce INIT_RO_ONE_MASK() to init csr_masks.ro_one.* fields.
 - Introduce defines for masks intead of constants.
 - Move old variable inside macros INIT_CSR_MASK() and INIT_RO_ONE_MASK().
---
Changes in V6:
 - Introduce sub-struct ro_one inside csr_masks to cover the case that
   hstateen0 could have read-only-one bits.
 - Refacotr init_csr_masks() to handle hstateen0 case when a bit is r/o-one
   and handle WPRI fields properly.
 - Update the commit message.
---
Changes in V5:
 - Move everything related to csr_masks to domain.c and make it static.
 - Move declaration of old variable in init_csr_masks() inside INIT_CSR_MASK.
 - Use csr_swap() in INIT_CSR_MASK().
---
Changes in V4:
 - Move csr_masks defintion to domain.c. Make it static as at the moment
   it is going to be used only in domain.c.
 - Rename and refactor X macros inside init_csr_masks().
---
Changes in V3:
 - New patch.
---
 xen/arch/riscv/domain.c            | 57 ++++++++++++++++++++++++++++++
 xen/arch/riscv/include/asm/setup.h |  2 ++
 xen/arch/riscv/setup.c             |  2 ++
 3 files changed, 61 insertions(+)

diff --git a/xen/arch/riscv/domain.c b/xen/arch/riscv/domain.c
index b60320b90def..32974cb48929 100644
--- a/xen/arch/riscv/domain.c
+++ b/xen/arch/riscv/domain.c
@@ -2,9 +2,66 @@
 
 #include <xen/init.h>
 #include <xen/mm.h>
+#include <xen/sections.h>
 #include <xen/sched.h>
 #include <xen/vmap.h>
 
+#include <asm/cpufeature.h>
+#include <asm/csr.h>
+
+struct csr_masks {
+    register_t hedeleg;
+    register_t henvcfg;
+    register_t hideleg;
+    register_t hstateen0;
+
+    struct {
+        register_t hstateen0;
+    } ro_one;
+};
+
+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)
+
+void __init init_csr_masks(void)
+{
+    /*
+     * The mask specifies the bits that may be safely modified without
+     * causing side effects.
+     *
+     * For example, registers such as henvcfg or hstateen0 contain WPRI
+     * 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); \
+        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_HSTATEEN0, mask); \
+        csr_masks.ro_one.field = csr_swap(CSR_##csr, old) & mask; \
+    } while (0)
+
+    INIT_CSR_MASK(HEDELEG, hedeleg, HEDELEG_AVAIL_MASK);
+    INIT_CSR_MASK(HIDELEG, hideleg, HIDELEG_AVAIL_MASK);
+
+    INIT_CSR_MASK(HENVCFG, henvcfg, HENVCFG_AVAIL_MASK);
+
+    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);
+    }
+
+#undef INIT_CSR_MASK
+#undef INIT_RO_ONE_MASK
+}
+
 static void continue_new_vcpu(struct vcpu *prev)
 {
     BUG_ON("unimplemented\n");
diff --git a/xen/arch/riscv/include/asm/setup.h b/xen/arch/riscv/include/asm/setup.h
index c9d69cdf5166..2215894cfbb1 100644
--- a/xen/arch/riscv/include/asm/setup.h
+++ b/xen/arch/riscv/include/asm/setup.h
@@ -11,6 +11,8 @@ void setup_mm(void);
 
 void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
 
+void init_csr_masks(void);
+
 #endif /* ASM__RISCV__SETUP_H */
 
 /*
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 9b4835960d20..bca6ca09eddd 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -137,6 +137,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
 
     riscv_fill_hwcap();
 
+    init_csr_masks();
+
     preinit_xen_time();
 
     intc_preinit();
-- 
2.53.0

[PATCH v7 02/14] xen/riscv: implement vcpu_csr_init()

[Oleksii Kurochko]

Introduce vcpu_csr_init() to initialise hypervisor CSRs that control
vCPU execution and virtualization behaviour before the vCPU is first
scheduled.
The function configures trap and interrupt delegation to VS-mode by
setting the appropriate bits in the hedeleg and hideleg registers,
initializes hstatus so that execution enters VS-mode when control is
passed to the guest, and restricts guest access to hardware performance
counters by initializing hcounteren, as unrestricted access would
require additional handling in Xen.
When the Smstateen and SSAIA extensions are available, access to AIA
CSRs and IMSIC guest interrupt files is enabled by setting the
corresponding bits in hstateen0, avoiding unnecessary traps into Xen
(note that SVSLCT(Supervisor Virtual Select) name is used intead of
CSRIND as OpenSBI uses such name and riscv_encoding.h is mostly based
on it).
If the Svpbmt extension is supported, the PBMTE bit is set in
henvcfg to allow its use for VS-stage address translation. Guest
access to the ENVCFG CSR is also enabled by setting ENVCFG bit in
hstateen0, as a guest may need to control certain characteristics of
the U-mode (VU-mode when V=1) execution environment.

For CSRs that may contain read-only bits (e.g. hedeleg, hideleg,
hstateen0), to the written value a correspondent mask is applied to
avoid divergence between the software state and the actual CSR
contents.

As hstatus is not part of struct arch_vcpu (it already resides in
struct cpu_user_regs), introduce vcpu_guest_cpu_user_regs() to provide
a uniform way to access hstatus and other guest CPU user registers.

This establishes a consistent and well-defined initial CSR state for
vCPUs prior to their first context switch.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in V7:
 - Add Acked-by: Jan Beulich <jbeulich@suse.com>.
---
Changes in V6:
 - Apply introduced in prev. patch csr_masks.ro_one.hstaten0 in vcpu_csr_init().
---
Changes in v5:
 - Initialize of hstateen0 with SMSTATEEN0_HSENVCFG when a variable is
   defined.
 - Use |= for a code inside if (*_ssaia) case.
 - Put declaration of the registers hedeleg and hideleg together in arch_vcpu
   structure as they are typically used together so better chances to
   be in the same cache line.
---
Changes in v4:
 - Move local variable hstateen0 into narrower scope.
 - Code style fixes.
 - Move the call of vcpu_csr_init(v) after if ( is_idle_vcpu() ) check in
   arcg_vcpu_create().
---
Changes in v3:
 - Add hypervisor register used to initalize vCPU state.
 - Apply masks introduced before instead of csr_write()/csr_read() pattern.
---
Changes in v2:
 - As hstatus isn't a part of arch_vcpu structure (as it is already a part of
   cpu_user_regs) introduce vcpu_guest_cpu_user_regs() to be able to access
   hstatus and other CPU user regs.
 - Sort hideleg bit setting by value. Drop a stray blank.
 - Drop | when the first initialization of hcounteren and hennvcfg happen.
 - Introduce HEDELEG_DEFAULT. Sort set bits by value and use BIT() macros
   instead of open-coding it.
 - Apply pattern csr_write() -> csr_read() for hedeleg and hideleg instead
   of direct bit setting in v->arch.h{i,e}deleg as it could be that for some
   reason some bits of hedeleg and hideleg are r/o.
   The similar patter is used for hstateen0 as some of the bits could be r/o.
 - Add check that SSAIA is avaialable before setting of SMSTATEEN0_AIA |
   SMSTATEEN0_IMSIC | SMSTATEEN0_SVSLCT bits.
 - Drop local variables hstatus, hideleg and hedeleg as they aren't used
   anymore.
---
 xen/arch/riscv/domain.c                     | 63 +++++++++++++++++++++
 xen/arch/riscv/include/asm/current.h        |  2 +
 xen/arch/riscv/include/asm/domain.h         |  6 ++
 xen/arch/riscv/include/asm/riscv_encoding.h |  2 +
 4 files changed, 73 insertions(+)

diff --git a/xen/arch/riscv/domain.c b/xen/arch/riscv/domain.c
index 32974cb48929..08b990f7b9f6 100644
--- a/xen/arch/riscv/domain.c
+++ b/xen/arch/riscv/domain.c
@@ -8,6 +8,7 @@
 
 #include <asm/cpufeature.h>
 #include <asm/csr.h>
+#include <asm/riscv_encoding.h>
 
 struct csr_masks {
     register_t hedeleg;
@@ -20,6 +21,21 @@ struct csr_masks {
     } ro_one;
 };
 
+#define HEDELEG_DEFAULT (BIT(CAUSE_MISALIGNED_FETCH, U) | \
+                         BIT(CAUSE_FETCH_ACCESS, U) | \
+                         BIT(CAUSE_ILLEGAL_INSTRUCTION, U) | \
+                         BIT(CAUSE_BREAKPOINT, U) | \
+                         BIT(CAUSE_MISALIGNED_LOAD, U) | \
+                         BIT(CAUSE_LOAD_ACCESS, U) | \
+                         BIT(CAUSE_MISALIGNED_STORE, U) | \
+                         BIT(CAUSE_STORE_ACCESS, U) | \
+                         BIT(CAUSE_USER_ECALL, U) | \
+                         BIT(CAUSE_FETCH_PAGE_FAULT, U) | \
+                         BIT(CAUSE_LOAD_PAGE_FAULT, U) | \
+                         BIT(CAUSE_STORE_PAGE_FAULT, U))
+
+#define HIDELEG_DEFAULT (MIP_VSSIP | MIP_VSTIP | MIP_VSEIP)
+
 static struct csr_masks __ro_after_init csr_masks;
 
 #define HEDELEG_AVAIL_MASK ULONG_MAX
@@ -62,6 +78,51 @@ void __init init_csr_masks(void)
 #undef INIT_RO_ONE_MASK
 }
 
+static void vcpu_csr_init(struct vcpu *v)
+{
+    v->arch.hedeleg = HEDELEG_DEFAULT & csr_masks.hedeleg;
+
+    vcpu_guest_cpu_user_regs(v)->hstatus = HSTATUS_SPV | HSTATUS_SPVP;
+
+    v->arch.hideleg = HIDELEG_DEFAULT & csr_masks.hideleg;
+
+    /*
+     * VS should access only the time counter directly.
+     * Everything else should trap.
+     */
+    v->arch.hcounteren = HCOUNTEREN_TM;
+
+    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svpbmt) )
+        v->arch.henvcfg = ENVCFG_PBMTE & csr_masks.henvcfg;
+
+    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_smstateen) )
+    {
+        /* Allow guest to access CSR_SENVCFG */
+        register_t hstateen0 = SMSTATEEN0_HSENVCFG;
+
+        if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_ssaia) )
+            /*
+             * If the hypervisor extension is implemented, the same three
+             * bits are defined also in hypervisor CSR hstateen0 but concern
+             * only the state potentially accessible to a virtual machine
+             * executing in privilege modes VS and VU:
+             *      bit 60 CSRs siselect and sireg (really vsiselect and
+             *             vsireg)
+             *      bit 59 CSRs siph and sieh (RV32 only) and stopi (really
+             *             vsiph, vsieh, and vstopi)
+             *      bit 58 all state of IMSIC guest interrupt files, including
+             *             CSR stopei (really vstopei)
+             * If one of these bits is zero in hstateen0, and the same bit is
+             * one in mstateen0, then an attempt to access the corresponding
+             * state from VS or VU-mode raises a virtual instruction exception.
+             */
+            hstateen0 |= SMSTATEEN0_AIA | SMSTATEEN0_IMSIC | SMSTATEEN0_SVSLCT;
+
+        v->arch.hstateen0 = (hstateen0 & csr_masks.hstateen0) |
+                            csr_masks.ro_one.hstateen0;
+    }
+}
+
 static void continue_new_vcpu(struct vcpu *prev)
 {
     BUG_ON("unimplemented\n");
@@ -84,6 +145,8 @@ int arch_vcpu_create(struct vcpu *v)
     if ( is_idle_vcpu(v) )
         return 0;
 
+    vcpu_csr_init(v);
+
     /*
      * As the vtimer and interrupt controller (IC) are not yet implemented,
      * return an error.
diff --git a/xen/arch/riscv/include/asm/current.h b/xen/arch/riscv/include/asm/current.h
index 58c9f1506b7c..5fbee8182caa 100644
--- a/xen/arch/riscv/include/asm/current.h
+++ b/xen/arch/riscv/include/asm/current.h
@@ -48,6 +48,8 @@ DECLARE_PER_CPU(struct vcpu *, curr_vcpu);
 #define get_cpu_current(cpu)  per_cpu(curr_vcpu, cpu)
 
 #define guest_cpu_user_regs() ({ BUG_ON("unimplemented"); NULL; })
+#define vcpu_guest_cpu_user_regs(vcpu) \
+    (&(vcpu)->arch.cpu_info->guest_cpu_user_regs)
 
 #define switch_stack_and_jump(stack, fn) do {               \
     asm volatile (                                          \
diff --git a/xen/arch/riscv/include/asm/domain.h b/xen/arch/riscv/include/asm/domain.h
index 5aec627a7adb..17be792afe7d 100644
--- a/xen/arch/riscv/include/asm/domain.h
+++ b/xen/arch/riscv/include/asm/domain.h
@@ -49,6 +49,12 @@ struct arch_vcpu {
 
     struct cpu_info *cpu_info;
 
+    register_t hcounteren;
+    register_t hedeleg;
+    register_t hideleg;
+    register_t henvcfg;
+    register_t hstateen0;
+
     register_t vsatp;
 };
 
diff --git a/xen/arch/riscv/include/asm/riscv_encoding.h b/xen/arch/riscv/include/asm/riscv_encoding.h
index 1f7e612366f8..dd15731a86fa 100644
--- a/xen/arch/riscv/include/asm/riscv_encoding.h
+++ b/xen/arch/riscv/include/asm/riscv_encoding.h
@@ -228,6 +228,8 @@
 #define ENVCFG_CBIE_INV			_UL(0x3)
 #define ENVCFG_FIOM			_UL(0x1)
 
+#define HCOUNTEREN_TM BIT(1, U)
+
 /* ===== User-level CSRs ===== */
 
 /* User Trap Setup (N-extension) */
-- 
2.53.0

[PATCH v7 03/14] xen/riscv: introduce tracking of pending vCPU interrupts, part 1

[Oleksii Kurochko]

Based on Linux kernel v6.16.0.
Note that smp_wmb() is used instead of smp_mb__before_atomic() as what
we want to guarantee that if a bit in irqs_pending_mask is obversable
that the correspondent bit in irqs_pending is observable too.

Add lockless tracking of pending vCPU interrupts using atomic bitops.
Two bitmaps are introduced:
 - irqs_pending — interrupts currently pending for the vCPU
 - irqs_pending_mask — bits that have changed in irqs_pending

The design follows a multi-producer, single-consumer model, where the
consumer is the vCPU itself. Producers may set bits in
irqs_pending_mask without a lock. Clearing bits in irqs_pending_mask is
performed only by the consumer via xchg(). The consumer must not write
to irqs_pending and must not act on bits that are not set in the mask.
Otherwise, extra synchronization should be provided.

On RISC-V interrupts are not injected via guest registers, so pending
interrupts must be recorded in irqs_pending (using the new
vcpu_{un}set_interrupt() helpers) and flushed to the guest by updating
HVIP before returning control to the guest. The consumer side is
implemented in a follow-up patch.

A barrier between updating irqs_pending and setting the corresponding
mask bit in vcpu_set_interrupt()/vcpu_unset_interrupt() guarantees
that if the consumer observes a mask bit set, the corresponding pending
bit is also visible. This prevents missed interrupts during the flush.

It is possible that a guest could have pending bit in the hardware
register without being marked pending in irq_pending bitmap as:
  According to the RISC-V ISA specification:
    Bits hip.VSSIP and hie.VSSIE are the interrupt-pending and
    interrupt-enable  bits for VS-level software interrupts. VSSIP in hip
    is an alias (writable) of the same bit in hvip.
  Additionally:
    When bit 2 of hideleg is zero, vsip.SSIP and vsie.SSIE are read-only
    zeros. Else, vsip.SSIP and vsie.SSIE are aliases of hip.VSSIP and
    hie.VSSIE.
This means the guest may modify vsip.SSIP, which implicitly updates
hip.VSSIP and the bit being written with 1 would also trigger an interrupt
as according to the RISC-V spec:
  These conditions for an interrupt trap to occur must be evaluated in a
  bounded   amount of time from when an interrupt becomes, or ceases to be,
  pending in sip,  and must also be evaluated immediately following the
  execution of an SRET  instruction or an explicit write to a CSR on which
  these interrupt trap conditions expressly depend (including sip, sie and
  sstatus).
What means that IRQ_VS_SOFT must be synchronized separately, what is done
in vcpu_sync_interrupts(). Note, also, that IRQ_PMU_OVF would want to be
synced for the similar reason as IRQ_VS_SOFT, but isn't sync-ed now as
PMU isn't supported now.

For the remaining VS-level interrupt types (IRQ_VS_TIMER and
IRQ_VS_EXT), the specification states they cannot be modified by the guest
and are read-only because of:
  Bits hip.VSEIP and hie.VSEIE are the interrupt-pending and interrupt-enable
  bits for VS-level external interrupts. VSEIP is read-only in hip, and is
  the logical-OR of these interrupt sources:
    • bit VSEIP of hvip;
    • the bit of hgeip selected by hstatus.VGEIN; and
    • any other platform-specific external interrupt signal directed to
      VS-level.
  Bits hip.VSTIP and hie.VSTIE are the interrupt-pending and interrupt-enable
  bits for VS-level timer interrupts. VSTIP is read-only in hip, and is the
  logical-OR of hvip.VSTIP and any other platform-specific timer interrupt
  signal directed to VS-level.
and
  When bit 10 of hideleg is zero, vsip.SEIP and vsie.SEIE are read-only zeros.
  Else, vsip.SEIP and vsie.SEIE are aliases of hip.VSEIP and hie.VSEIE.

  When bit 6 of hideleg is zero, vsip.STIP and vsie.STIE are read-only zeros.
  Else, vsip.STIP and vsie.STIE are aliases of hip.VSTIP and hie.VSTIE.
and also,
  Bits sip.SEIP and sie.SEIE are the interrupt-pending and interrupt-enable
  bits for supervisor-level external interrupts. If implemented, SEIP is
  read-only in sip, and is set and cleared by the execution environment,
  typically through a platform-specific interrupt controller.

  Bits sip.STIP and sie.STIE are the interrupt-pending and interrupt-enable
  bits for supervisor-level timer interrupts. If implemented, STIP is
  read-only in sip, and is set and cleared by the execution environment
Thus, for these interrupt types, it is sufficient to use vcpu_set_interrupt()
and vcpu_unset_interrupt(), and flush them during the call of
vcpu_flush_interrupts() (which is introduced in follow up patch).

vcpu_sync_interrupts(), which is called just before entering the VM,
slightly bends the rule that the irqs_pending bit must be written
first, followed by updating the corresponding bit in irqs_pending_mask.
However, it still respects the core guarantee that the producer never
clears the mask and only writes to irqs_pending if it is the one that
flipped the corresponding mask bit from 0 to 1.
Moreover, since the consumer won't run concurrently because
vcpu_sync_interrupts() and the consumer path are going to be invoked
sequentially immediately before VM entry, it is safe to slightly relax
this ordering rule in vcpu_sync_interrupts().

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v7:
 - Drop ifdef RV32 in vcpu_sync_interrupts(). It will be introduced later when
   it will be more clear from the context why it is needed in this function.
 - Rename vcpu_sync_interrups()'s argument to curr and add
   ASSERT(curr == current).
---
Changes in v6:
 - Drop for the moment:
       /* Read current HVIP and VSIE CSRs */
       v->arch.vsie = csr_read(CSR_VSIE);
   from vcpu_sync_interrupts() as it isn't used at the moment and will
   be introduced when a usage will be more clear.
---
Changes in v5:
 - Update the commit message().
 - Rename c to curr.
 - Update vcpu_set_interrupt() to use test_and_set_bit() for irqs_pending
   bitmask too.
 - Move #ifdef CONFIG_RISCV_32 above the comment in vcpu_sync_interrupts().
---
Changes in v4:
 - Update the commit message.
 - Update the comments in vcpu_(un)set_interrupt() and add the the  comment
   above smp_wmb() barrier.
 - call vcpu_kick() only if the pending_mask bit going from 0 to 1.
 - Code style fixes.
 - Update defintion of RISCV_VCPU_NR_IRQS to cover potential RV128 case and
   the case if AIA isn't used.
 - latch current into a local variable in check_for_pcpu_work().
---
Changes in v3:
 - Use smp_wb() instead of smp_mb__before_atomic().
 - Add explanation of the change above in the commit message.
 - Move vcpu_sync_interrupts() here to producers side.
 - Introduce check_for_pcpu_work() to be clear from where vcpu_sync_interrupts()
   is called.
---
Changes in V2:
 - Move the patch before an introduction of vtimer.
 - Drop bitmap_zero() of irqs_pending and irqs_pending_mask bitmaps as
   vcpu structure starts out all zeros.
 - Drop const for irq argument of vcpu_{un}set_interrupt().
 - Drop check "irq < IRQ_LOCAL_MAX" in vcpu_{un}set_interrupt() as it
   could lead to overrun of irqs_pending and irqs_pending_mask bitmaps.
 - Drop IRQ_LOCAL_MAX as there is no usage for it now.
---
 xen/arch/riscv/domain.c             | 63 +++++++++++++++++++++++++++++
 xen/arch/riscv/include/asm/domain.h | 22 ++++++++++
 xen/arch/riscv/traps.c              |  4 ++
 3 files changed, 89 insertions(+)

diff --git a/xen/arch/riscv/domain.c b/xen/arch/riscv/domain.c
index 08b990f7b9f6..5447c17402dd 100644
--- a/xen/arch/riscv/domain.c
+++ b/xen/arch/riscv/domain.c
@@ -6,6 +6,7 @@
 #include <xen/sched.h>
 #include <xen/vmap.h>
 
+#include <asm/bitops.h>
 #include <asm/cpufeature.h>
 #include <asm/csr.h>
 #include <asm/riscv_encoding.h>
@@ -168,6 +169,68 @@ void arch_vcpu_destroy(struct vcpu *v)
     vfree((void *)&v->arch.cpu_info[1] - STACK_SIZE);
 }
 
+int vcpu_set_interrupt(struct vcpu *v, unsigned int irq)
+{
+    bool kick_vcpu;
+
+    /* We only allow VS-mode software, timer, and external interrupts */
+    if ( irq != IRQ_VS_SOFT &&
+         irq != IRQ_VS_TIMER &&
+         irq != IRQ_VS_EXT )
+        return -EINVAL;
+
+    kick_vcpu = !test_and_set_bit(irq, v->arch.irqs_pending);
+
+    /*
+     * The counterpart of this barrier is the one encoded implicitly in xchg()
+     * which is used in consumer part (vcpu_flush_interrupts()).
+     */
+    smp_wmb();
+
+    kick_vcpu |= !test_and_set_bit(irq, v->arch.irqs_pending_mask);
+
+    if ( kick_vcpu )
+        vcpu_kick(v);
+
+    return 0;
+}
+
+int vcpu_unset_interrupt(struct vcpu *v, unsigned int irq)
+{
+    /* We only allow VS-mode software, timer, external interrupts */
+    if ( irq != IRQ_VS_SOFT &&
+         irq != IRQ_VS_TIMER &&
+         irq != IRQ_VS_EXT )
+        return -EINVAL;
+
+    clear_bit(irq, v->arch.irqs_pending);
+    /*
+     * The counterpart of this barrier is the one encoded implicitly in xchg()
+     * which is used in consumer part (vcpu_flush_interrupts()).
+     */
+    smp_wmb();
+    set_bit(irq, v->arch.irqs_pending_mask);
+
+    return 0;
+}
+
+void vcpu_sync_interrupts(struct vcpu *curr)
+{
+    unsigned long hvip = csr_read(CSR_HVIP);
+
+    ASSERT(curr == current);
+
+    /* Sync-up HVIP.VSSIP bit changes done by Guest */
+    if ( ((curr->arch.hvip ^ hvip) & BIT(IRQ_VS_SOFT, UL)) &&
+         !test_and_set_bit(IRQ_VS_SOFT, &curr->arch.irqs_pending_mask) )
+    {
+        if ( hvip & BIT(IRQ_VS_SOFT, UL) )
+            set_bit(IRQ_VS_SOFT, &curr->arch.irqs_pending);
+        else
+            clear_bit(IRQ_VS_SOFT, &curr->arch.irqs_pending);
+    }
+}
+
 static void __init __maybe_unused build_assertions(void)
 {
     /*
diff --git a/xen/arch/riscv/include/asm/domain.h b/xen/arch/riscv/include/asm/domain.h
index 17be792afe7d..1ecfe18c8519 100644
--- a/xen/arch/riscv/include/asm/domain.h
+++ b/xen/arch/riscv/include/asm/domain.h
@@ -54,8 +54,25 @@ struct arch_vcpu {
     register_t hideleg;
     register_t henvcfg;
     register_t hstateen0;
+    register_t hvip;
 
     register_t vsatp;
+
+    /*
+     * VCPU interrupts
+     *
+     * We have a lockless approach for tracking pending VCPU interrupts
+     * implemented using atomic bitops. The irqs_pending bitmap represent
+     * pending interrupts whereas irqs_pending_mask represent bits changed
+     * in irqs_pending. Our approach is modeled around multiple producer
+     * and single consumer problem where the consumer is the VCPU itself.
+     *
+     * DECLARE_BITMAP() is needed here to support 64 vCPU local interrupts
+     * on RV32 host.
+     */
+#define RISCV_VCPU_NR_IRQS MAX(BITS_PER_LONG, 64)
+    DECLARE_BITMAP(irqs_pending, RISCV_VCPU_NR_IRQS);
+    DECLARE_BITMAP(irqs_pending_mask, RISCV_VCPU_NR_IRQS);
 };
 
 struct paging_domain {
@@ -94,6 +111,11 @@ static inline void update_guest_memory_policy(struct vcpu *v,
 
 static inline void arch_vcpu_block(struct vcpu *v) {}
 
+int vcpu_set_interrupt(struct vcpu *v, unsigned int irq);
+int vcpu_unset_interrupt(struct vcpu *v, unsigned int irq);
+
+void vcpu_sync_interrupts(struct vcpu *curr);
+
 #endif /* ASM__RISCV__DOMAIN_H */
 
 /*
diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index 9fca941526f6..551f886e3a69 100644
--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -171,6 +171,10 @@ static void do_unexpected_trap(const struct cpu_user_regs *regs)
 
 static void check_for_pcpu_work(void)
 {
+    struct vcpu *curr = current;
+
+    vcpu_sync_interrupts(curr);
+
     p2m_handle_vmenter();
 }
 
-- 
2.53.0

[PATCH v7 04/14] xen/riscv: introduce tracking of pending vCPU interrupts, part 2

[Oleksii Kurochko]

This patch is based on Linux kernel 6.16.0.

Add the consumer side (vcpu_flush_interrupts()) of the lockless pending
interrupt tracking introduced in part 1 (for producers). According, to the
design only one consumer is possible, and it is vCPU itself.
vcpu_flush_interrupts() is expected to be ran (as guests aren't ran now due
to the lack of functionality) before the hypervisor returns control to the
guest.

Producers may set bits in irqs_pending_mask without a lock. Clearing bits in
irqs_pending_mask is performed only by the consumer via xchg() (with aquire
semantics). The consumer must not write to irqs_pending and must not act on
bits that are not set in the mask. Otherwise, extra synchronization should
be provided.
The worst thing which could happen with such approach is that a new pending
bit will be set to irqs_pending bitmap during update of hvip variable in
vcpu_flush_interrupt() but it isn't problem as the new pending bit won't
be lost and just be proceded during the next flush.

As AIA specs introduced hviph register which would want to be updated when
guest related AIA code vcpu_update_hvip() is introduced instead of just
open-code it in vcpu_flush_interrupts().

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in v7:
 - Rename argument v of vcpu_flush_interrupts() to curr.
 - Add ASSERT(curr == current) to be sure that this functions is called only
   for current vCPU as the whole vcpu_flush_interrupts() implemented to work 
   with current (at the moment, at least).
 - Drop vcpu_update_hvip() function and open code it in vcpu_flush_interrupt()
   as there is no more update of hvip unconditionally and hviph will be
   updated separately. (originally it was planned to update unconditionally
   both hvip and hviph inside vcpu_update_hvip() after if() conditions in
   vcpu_flush_interrupt())
---
Changes in v6:
 - Nothing changed. Only rebase.
---
Changes in v5:
 - Reorder the defintions of local variables (mask, val, hvip) in
   vcpu_flush_interrupts(). Also, drop a blank line between them.
 - Move #ifdef CONFIG_RISCV_32 above the comment in vcpu_flush_interrupts()
   and align the comment properly.
 - Add Acked-by: Jan Beulich <jbeulich@suse.com>.
---
Changes in v4:
 - Move defintion of hvip local variable to narrower space in
   vcpu_flush_interrupts().
 - Use initializers for mask and val variables.
 - Use local variable c as an argument of vcpu_flush_interrupts() in
   check_for_pcpu_work().
---
Changes in v3:
 - Update the error message in case of RV32 from "hviph" to v->arch.hviph.
 - Make const argument of vcpu_update_hvip.
 - Move local variables mask and val inside if() in vcpu_flush_interrupts().
 - Call vcpu_flush_interrupts() in check_pcpu_work().
 - Move vcpu_update_hvip() inside if() in vcpu_flush_interrupts().
---
Changes in v2:
 - New patch.
---
 xen/arch/riscv/domain.c             | 27 +++++++++++++++++++++++++++
 xen/arch/riscv/include/asm/domain.h |  1 +
 xen/arch/riscv/traps.c              |  2 ++
 3 files changed, 30 insertions(+)

diff --git a/xen/arch/riscv/domain.c b/xen/arch/riscv/domain.c
index 5447c17402dd..f3e3ad149453 100644
--- a/xen/arch/riscv/domain.c
+++ b/xen/arch/riscv/domain.c
@@ -231,6 +231,33 @@ void vcpu_sync_interrupts(struct vcpu *curr)
     }
 }
 
+void vcpu_flush_interrupts(struct vcpu *curr)
+{
+    ASSERT(curr == current);
+
+    if ( ACCESS_ONCE(curr->arch.irqs_pending_mask[0]) )
+    {
+        unsigned long mask = xchg(&curr->arch.irqs_pending_mask[0], 0UL);
+        unsigned long val = ACCESS_ONCE(curr->arch.irqs_pending[0]) & mask;
+        register_t *hvip = &curr->arch.hvip;
+
+        *hvip &= ~mask;
+        *hvip |= val;
+
+        csr_write(CSR_HVIP, *hvip);
+    }
+
+#ifdef CONFIG_RISCV_32
+    /*
+     * Flush AIA high interrupts.
+     *
+     * It is necessary to do only for CONFIG_RISCV_32 which isn't
+     * supported now.
+     */
+#   error "Update v->arch.hviph"
+#endif
+}
+
 static void __init __maybe_unused build_assertions(void)
 {
     /*
diff --git a/xen/arch/riscv/include/asm/domain.h b/xen/arch/riscv/include/asm/domain.h
index 1ecfe18c8519..59d23e4f9247 100644
--- a/xen/arch/riscv/include/asm/domain.h
+++ b/xen/arch/riscv/include/asm/domain.h
@@ -115,6 +115,7 @@ int vcpu_set_interrupt(struct vcpu *v, unsigned int irq);
 int vcpu_unset_interrupt(struct vcpu *v, unsigned int irq);
 
 void vcpu_sync_interrupts(struct vcpu *curr);
+void vcpu_flush_interrupts(struct vcpu *curr);
 
 #endif /* ASM__RISCV__DOMAIN_H */
 
diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index 551f886e3a69..244264c92a79 100644
--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -175,6 +175,8 @@ static void check_for_pcpu_work(void)
 
     vcpu_sync_interrupts(curr);
 
+    vcpu_flush_interrupts(curr);
+
     p2m_handle_vmenter();
 }
 
-- 
2.53.0

[PATCH v7 05/14] xen/riscv: introduce basic vtimer infrastructure for guests

[Oleksii Kurochko]

Lay the groundwork for guest timer support by introducing a per-vCPU
virtual timer backed by Xen’s common timer infrastructure.

The virtual timer is programmed in response to the guest SBI
sbi_set_timer() call and injects a virtual supervisor timer interrupt
into the vCPU when it expires.

While a dedicated struct vtimer is not strictly required at present,
it is expected to become necessary once SSTC support is introduced.
In particular, it will need to carry additional state such as whether
SSTC is enabled, the next compare value (e.g. for the VSTIMECMP CSR)
to be saved and restored across context switches, and time delta state
(e.g. HTIMEDELTA) required for use cases such as migration. Introducing
struct vtimer now avoids a later refactoring.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in v6-v7:
 - Nothing changed. Only rebase.
---
Changes in v5:
 - Drop copyright line from asm/vtimer.h.
 - Add Acked-by: Jan Beulich <jbeulich@suse.com>.
---
Changes in v4:
 - Add vcpu_timer_destroy() to void arch_vcpu_destroy().
---
Changes in v3:
 - use one container_of() to get vcpu instead of two container_of()s.
---
Changes in v2:
 - Drop domain_vtimer_init() as it does nothing.
 - Drop "struct vcpu *v;" from struct vtimer as it could be taken
   from arch_vcpu using container_of().
 - Drop vtimer_initialized, use t->status == TIMER_STATUS_invalid
   instead to understand if timer was or wasn't initialized.
 - Drop inclusion of xen/domain.h as xen/sched.h already includes it.
 - s/ xen/time.h/ xen.timer.h in vtimer.c.
 - Drop ULL in if-conidtion in vtimer_set_timer() as with the cast
   it isn't necessary to have suffix ULL.
 - Add migrate timer to vtimer_set_timer() to be sure that vtimer
   will occur on pCPU it was ran, so the signalling to that vCPU
   will (commonly) be cheaper.
 - Check if the timeout has already expired and just inject the event
   in vtimer_vtimer_set_timer().
 - Drop const for ticks argument of vtimer_set_timer().
 - Merge two patches to one:
   - xen/riscv: introduce vtimer
   - xen/riscv: introduce vtimer_set_timer() and vtimer_expired()
---
 xen/arch/riscv/Makefile             |  1 +
 xen/arch/riscv/domain.c             | 10 +++-
 xen/arch/riscv/include/asm/domain.h |  3 ++
 xen/arch/riscv/include/asm/vtimer.h | 17 +++++++
 xen/arch/riscv/vtimer.c             | 71 +++++++++++++++++++++++++++++
 5 files changed, 100 insertions(+), 2 deletions(-)
 create mode 100644 xen/arch/riscv/include/asm/vtimer.h
 create mode 100644 xen/arch/riscv/vtimer.c

diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
index bc47e83b26d7..ffbd7062e214 100644
--- a/xen/arch/riscv/Makefile
+++ b/xen/arch/riscv/Makefile
@@ -22,6 +22,7 @@ obj-y += traps.o
 obj-y += vmid.o
 obj-y += vm_event.o
 obj-y += vsbi/
+obj-y += vtimer.o
 
 $(TARGET): $(TARGET)-syms
 	$(OBJCOPY) -O binary -S $< $@
diff --git a/xen/arch/riscv/domain.c b/xen/arch/riscv/domain.c
index f3e3ad149453..b59e026a9635 100644
--- a/xen/arch/riscv/domain.c
+++ b/xen/arch/riscv/domain.c
@@ -10,6 +10,7 @@
 #include <asm/cpufeature.h>
 #include <asm/csr.h>
 #include <asm/riscv_encoding.h>
+#include <asm/vtimer.h>
 
 struct csr_masks {
     register_t hedeleg;
@@ -148,11 +149,14 @@ int arch_vcpu_create(struct vcpu *v)
 
     vcpu_csr_init(v);
 
+    if ( (rc = vcpu_vtimer_init(v)) )
+        goto fail;
+
     /*
-     * As the vtimer and interrupt controller (IC) are not yet implemented,
+     * As interrupt controller (IC) is not yet implemented,
      * return an error.
      *
-     * TODO: Drop this once the vtimer and IC are implemented.
+     * TODO: Drop this once IC is implemented.
      */
     rc = -EOPNOTSUPP;
     goto fail;
@@ -166,6 +170,8 @@ int arch_vcpu_create(struct vcpu *v)
 
 void arch_vcpu_destroy(struct vcpu *v)
 {
+    vcpu_timer_destroy(v);
+
     vfree((void *)&v->arch.cpu_info[1] - STACK_SIZE);
 }
 
diff --git a/xen/arch/riscv/include/asm/domain.h b/xen/arch/riscv/include/asm/domain.h
index 59d23e4f9247..6c48bf13111d 100644
--- a/xen/arch/riscv/include/asm/domain.h
+++ b/xen/arch/riscv/include/asm/domain.h
@@ -8,6 +8,7 @@
 #include <public/hvm/params.h>
 
 #include <asm/p2m.h>
+#include <asm/vtimer.h>
 
 struct vcpu_vmid {
     uint64_t generation;
@@ -49,6 +50,8 @@ struct arch_vcpu {
 
     struct cpu_info *cpu_info;
 
+    struct vtimer vtimer;
+
     register_t hcounteren;
     register_t hedeleg;
     register_t hideleg;
diff --git a/xen/arch/riscv/include/asm/vtimer.h b/xen/arch/riscv/include/asm/vtimer.h
new file mode 100644
index 000000000000..111863610a92
--- /dev/null
+++ b/xen/arch/riscv/include/asm/vtimer.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef ASM__RISCV__VTIMER_H
+#define ASM__RISCV__VTIMER_H
+
+#include <xen/timer.h>
+
+struct vtimer {
+    struct timer timer;
+};
+
+int vcpu_vtimer_init(struct vcpu *v);
+void vcpu_timer_destroy(struct vcpu *v);
+
+void vtimer_set_timer(struct vtimer *t, uint64_t ticks);
+
+#endif /* ASM__RISCV__VTIMER_H */
diff --git a/xen/arch/riscv/vtimer.c b/xen/arch/riscv/vtimer.c
new file mode 100644
index 000000000000..32d142bcdfcd
--- /dev/null
+++ b/xen/arch/riscv/vtimer.c
@@ -0,0 +1,71 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include <xen/sched.h>
+#include <xen/timer.h>
+
+#include <asm/vtimer.h>
+
+static void vtimer_expired(void *data)
+{
+    struct vtimer *t = data;
+    struct vcpu *v = container_of(t, struct vcpu, arch.vtimer);
+
+    vcpu_set_interrupt(v, IRQ_VS_TIMER);
+}
+
+int vcpu_vtimer_init(struct vcpu *v)
+{
+    struct vtimer *t = &v->arch.vtimer;
+
+    init_timer(&t->timer, vtimer_expired, t, v->processor);
+
+    return 0;
+}
+
+void vcpu_timer_destroy(struct vcpu *v)
+{
+    struct vtimer *t = &v->arch.vtimer;
+
+    if ( t->timer.status == TIMER_STATUS_invalid )
+        return;
+
+    kill_timer(&v->arch.vtimer.timer);
+}
+
+void vtimer_set_timer(struct vtimer *t, const uint64_t ticks)
+{
+    struct vcpu *v = container_of(t, struct vcpu, arch.vtimer);
+    s_time_t expires = ticks_to_ns(ticks - boot_clock_cycles);
+
+    vcpu_unset_interrupt(v, IRQ_VS_TIMER);
+
+    /*
+     * According to the RISC-V sbi spec:
+     *   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.
+     */
+    if ( ticks == ((uint64_t)~0) )
+    {
+        stop_timer(&t->timer);
+
+        return;
+    }
+
+    if ( expires < NOW() )
+    {
+        /*
+         * Simplify the logic if the timeout has already expired and just
+         * inject the event.
+         */
+        stop_timer(&t->timer);
+        vcpu_set_interrupt(v, IRQ_VS_TIMER);
+
+        return;
+    }
+
+    migrate_timer(&t->timer, smp_processor_id());
+    set_timer(&t->timer, expires);
+}
-- 
2.53.0

[PATCH v7 06/14] xen/riscv: introduce vcpu_kick() implementation

[Oleksii Kurochko]

Add a RISC-V implementation of vcpu_kick(), which unblocks the target
vCPU and sends an event check IPI if the vCPU was running on another
processor. This mirrors the behavior of Arm and enables proper vCPU
wakeup handling on RISC-V.

Remove the stub implementation from stubs.c, as it is now provided by
arch/riscv/domain.c.

Since vcpu_kick() calls perfc_incr(vcpu_kick), add perfcounter for
vcpu_kick to handle the case when CONFIG_PERF_COUNTERS=y. Although
CONFIG_PERF_COUNTERS is not enabled by default, it can be enabled,
for example, by randconfig what will lead to CI build issues.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in v4-v7:
 - Nothing changed. Only rebase.
---
Changes in v3:
 - Add asm/perfc_defn.h to provide vcpu_kick perfcoounter to cover
   the case when CONFIG_PERF_COUNTERS=y.
---
Changes in v2:
 - Add Acked-by: Jan Beulich <jbeulich@suse.com>.
---
 xen/arch/riscv/domain.c                 | 14 ++++++++++++++
 xen/arch/riscv/include/asm/Makefile     |  1 -
 xen/arch/riscv/include/asm/perfc_defn.h |  3 +++
 xen/arch/riscv/stubs.c                  |  5 -----
 4 files changed, 17 insertions(+), 6 deletions(-)
 create mode 100644 xen/arch/riscv/include/asm/perfc_defn.h

diff --git a/xen/arch/riscv/domain.c b/xen/arch/riscv/domain.c
index b59e026a9635..c8ce1efa884d 100644
--- a/xen/arch/riscv/domain.c
+++ b/xen/arch/riscv/domain.c
@@ -1,9 +1,11 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 
+#include <xen/cpumask.h>
 #include <xen/init.h>
 #include <xen/mm.h>
 #include <xen/sections.h>
 #include <xen/sched.h>
+#include <xen/smp.h>
 #include <xen/vmap.h>
 
 #include <asm/bitops.h>
@@ -264,6 +266,18 @@ void vcpu_flush_interrupts(struct vcpu *curr)
 #endif
 }
 
+void vcpu_kick(struct vcpu *v)
+{
+    bool running = v->is_running;
+
+    vcpu_unblock(v);
+    if ( running && v != current )
+    {
+        perfc_incr(vcpu_kick);
+        smp_send_event_check_mask(cpumask_of(v->processor));
+    }
+}
+
 static void __init __maybe_unused build_assertions(void)
 {
     /*
diff --git a/xen/arch/riscv/include/asm/Makefile b/xen/arch/riscv/include/asm/Makefile
index 3824f31c395c..86c56251d5d7 100644
--- a/xen/arch/riscv/include/asm/Makefile
+++ b/xen/arch/riscv/include/asm/Makefile
@@ -7,7 +7,6 @@ generic-y += hypercall.h
 generic-y += iocap.h
 generic-y += irq-dt.h
 generic-y += percpu.h
-generic-y += perfc_defn.h
 generic-y += random.h
 generic-y += softirq.h
 generic-y += vm_event.h
diff --git a/xen/arch/riscv/include/asm/perfc_defn.h b/xen/arch/riscv/include/asm/perfc_defn.h
new file mode 100644
index 000000000000..8a4b945df662
--- /dev/null
+++ b/xen/arch/riscv/include/asm/perfc_defn.h
@@ -0,0 +1,3 @@
+/* This file is intended to be included multiple times. */
+
+PERFCOUNTER(vcpu_kick, "vcpu: notify other vcpu")
diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
index daadff0138e4..eedf8bf9350a 100644
--- a/xen/arch/riscv/stubs.c
+++ b/xen/arch/riscv/stubs.c
@@ -203,11 +203,6 @@ void vcpu_block_unless_event_pending(struct vcpu *v)
     BUG_ON("unimplemented");
 }
 
-void vcpu_kick(struct vcpu *v)
-{
-    BUG_ON("unimplemented");
-}
-
 unsigned long
 hypercall_create_continuation(unsigned int op, const char *format, ...)
 {
-- 
2.53.0

[PATCH v7 07/14] xen/riscv: add vtimer context switch helpers

[Oleksii Kurochko]

Introduce vtimer_ctxt_switch_from() and vtimer_ctxt_switch_to() to handle
virtual timer state across vCPU context switches.

At present, vtimer_ctxt_switch_from() is a no-op because the RISC-V SSTC
extension, which provides a virtualization-aware timer, is not yet
supported. Xen therefore relies the virtual (SBI-based) timer.

The virtual timer uses Xen's internal timer infrastructure and must be
associated with the pCPU on which the vCPU is currently running so that
timer events can be delivered efficiently. As a result, vtimer_ctxt_switch_to()
migrates the timer to the target pCPU when a vCPU is scheduled in.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in v4-v7:
 - Nothing changed. Only rebase.
---
Changes in v3:
 - s/vtimer_ctx_switch_to/vtimer_ctxt_switch_to
 - s/vtimer_ctx_switch_from/vtimer_ctxt_switch_from
 - Add Acked-by: Jan Beulich <jbeulich@suse.com>.
---
Changes in v2:
 - Align the parameters names for  vtimer_ctx_switch_from() and vtimer_ctx_switch_to() in
   declarations to match the ones in the defintions to make Misra happy.
 - s/vtimer_save/vtimer_ctx_switch_from.
 - s/vtimer_restore/vtimer_ctx_switch_to.
 - Update the commit message.
---
 xen/arch/riscv/include/asm/vtimer.h |  3 +++
 xen/arch/riscv/vtimer.c             | 15 +++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/xen/arch/riscv/include/asm/vtimer.h b/xen/arch/riscv/include/asm/vtimer.h
index 111863610a92..b4d48d1a1732 100644
--- a/xen/arch/riscv/include/asm/vtimer.h
+++ b/xen/arch/riscv/include/asm/vtimer.h
@@ -14,4 +14,7 @@ void vcpu_timer_destroy(struct vcpu *v);
 
 void vtimer_set_timer(struct vtimer *t, uint64_t ticks);
 
+void vtimer_ctxt_switch_from(struct vcpu *p);
+void vtimer_ctxt_switch_to(struct vcpu *n);
+
 #endif /* ASM__RISCV__VTIMER_H */
diff --git a/xen/arch/riscv/vtimer.c b/xen/arch/riscv/vtimer.c
index 32d142bcdfcd..afd8a53a7387 100644
--- a/xen/arch/riscv/vtimer.c
+++ b/xen/arch/riscv/vtimer.c
@@ -1,5 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 
+#include <xen/bug.h>
 #include <xen/sched.h>
 #include <xen/timer.h>
 
@@ -69,3 +70,17 @@ void vtimer_set_timer(struct vtimer *t, const uint64_t ticks)
     migrate_timer(&t->timer, smp_processor_id());
     set_timer(&t->timer, expires);
 }
+
+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. */
+}
+
+void vtimer_ctxt_switch_to(struct vcpu *n)
+{
+    ASSERT(!is_idle_vcpu(n));
+
+    migrate_timer(&n->arch.vtimer.timer, n->processor);
+}
-- 
2.53.0

[PATCH v7 08/14] xen/riscv: implement SBI legacy SET_TIMER support for guests

[Oleksii Kurochko]

Add handling of the SBI_EXT_0_1_SET_TIMER function ID to the legacy
extension ecall handler. The handler now programs the vCPU’s virtual
timer via vtimer_set_timer() and returns SBI_SUCCESS.

This enables guests using the legacy SBI timer interface to schedule
timer events correctly.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in v3-v7:
 - Nothing changed. Only rebase.
---
Changes in v2:
 - Add Acked-by: Jan Beulich <jbeulich@suse.com>.
---
 xen/arch/riscv/vsbi/legacy-extension.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/arch/riscv/vsbi/legacy-extension.c b/xen/arch/riscv/vsbi/legacy-extension.c
index 2e8df191c295..090c23440cea 100644
--- a/xen/arch/riscv/vsbi/legacy-extension.c
+++ b/xen/arch/riscv/vsbi/legacy-extension.c
@@ -7,6 +7,7 @@
 
 #include <asm/processor.h>
 #include <asm/vsbi.h>
+#include <asm/vtimer.h>
 
 static void vsbi_print_char(char c)
 {
@@ -44,6 +45,11 @@ static int vsbi_legacy_ecall_handler(unsigned long eid, unsigned long fid,
         ret = SBI_ERR_NOT_SUPPORTED;
         break;
 
+    case SBI_EXT_0_1_SET_TIMER:
+        vtimer_set_timer(&current->arch.vtimer, regs->a0);
+        regs->a0 = SBI_SUCCESS;
+        break;
+
     default:
         /*
          * TODO: domain_crash() is acceptable here while things are still under
-- 
2.53.0

[PATCH v7 09/14] xen/riscv: introduce sbi_set_timer()

[Oleksii Kurochko]

Introduce a function pointer for sbi_set_timer(), since different OpenSBI
versions may implement the TIME extension with different extension IDs
and/or function IDs.

If the TIME extension is not available, fall back to the legacy timer
mechanism. This is useful when Xen runs as a guest under another Xen,
because the TIME extension is not currently virtualised and therefore
will not appear as available.
Despite of the fact that sbi_set_timer_v01 is introduced and used as
fall back, SBI v0.1 still isn't fully supported (with the current SBI
calls usage, sbi_rfence_v01 should be introduced too), so panic()
in sbi_init() isn't removed.

The sbi_set_timer() pointer will be used by reprogram_timer() to program
Xen’s physical timer as without SSTC extension there is no any other
option except SBI call to do that as only M-timer is available for us.

Use dprintk() for all the cases to print that a speicifc SBI extension
is available as it isn't really necessary in case of release builds.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in v6-v7:
 - Nothing changed. Only rebase.
---
Changes in v5:
- Add inclusion of <xen/sections.h> to <asm/sbi.h> to deal with the compilation issue:
    ./arch/riscv/include/asm/sbi.h:156:30: error: expected ')' before 'sbi_set_timer'
    156 | extern int (* __ro_after_init sbi_set_timer)(uint64_t stime_value);
- Rephrase the first sentence of the comment above declaration of sbi_set_timer
  pointer to function.
---
Changes in v4:
 - Add "stime_value is in absolute time" to the comment above declaration
   of sbi_set_timer() function pointer.
 - Add Acked-by: Jan Beulich <jbeulich@suse.com>.
---
Changes in v3:
 - Init sbi_set_timer with sbi_set_timer_v01 as fallback value.
 - Sort SBI IDs in the same way as SBI EXT IDs are declared.
 - Add __ro_after_init for sbi_set_timer variable.
 - use dprintk instead of printk to print information if SBI ext is available.
---
Changes in v2:
 - Move up defintion of SBI_EXT_TIME_SET_TIMER and use the same padding as
   defintions around it.
 - Add an extra comment about stime_value granuality above declaration of
   sbi_set_timer function pointer.
 - Refactor implemetation of sbi_set_timer_v02().
 - Provide fallback for sbi_set_timer_v01().
 - Update the commit message.
---
 xen/arch/riscv/include/asm/sbi.h | 22 ++++++++++++++++++
 xen/arch/riscv/sbi.c             | 40 +++++++++++++++++++++++++++++++-
 2 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/xen/arch/riscv/include/asm/sbi.h b/xen/arch/riscv/include/asm/sbi.h
index 79f7ff5c5501..ed7af200288f 100644
--- a/xen/arch/riscv/include/asm/sbi.h
+++ b/xen/arch/riscv/include/asm/sbi.h
@@ -13,6 +13,7 @@
 #define ASM__RISCV__SBI_H
 
 #include <xen/cpumask.h>
+#include <xen/sections.h>
 
 /* SBI-defined implementation ID */
 #define SBI_XEN_IMPID 7
@@ -29,6 +30,7 @@
 
 #define SBI_EXT_BASE                    0x10
 #define SBI_EXT_RFENCE                  0x52464E43
+#define SBI_EXT_TIME                    0x54494D45
 
 /* SBI function IDs for BASE extension */
 #define SBI_EXT_BASE_GET_SPEC_VERSION   0x0
@@ -48,6 +50,9 @@
 #define SBI_EXT_RFENCE_REMOTE_HFENCE_VVMA       0x5
 #define SBI_EXT_RFENCE_REMOTE_HFENCE_VVMA_ASID  0x6
 
+/* SBI function IDs for TIME extension */
+#define SBI_EXT_TIME_SET_TIMER          0x0
+
 #define SBI_SPEC_VERSION_MAJOR_MASK     0x7f000000
 #define SBI_SPEC_VERSION_MINOR_MASK     0x00ffffff
 
@@ -134,6 +139,23 @@ 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/sbi.c b/xen/arch/riscv/sbi.c
index 425dce44c679..b4a7ae6940c1 100644
--- a/xen/arch/riscv/sbi.c
+++ b/xen/arch/riscv/sbi.c
@@ -249,6 +249,38 @@ static int (* __ro_after_init sbi_rfence)(unsigned long fid,
                                           unsigned long arg4,
                                           unsigned long arg5);
 
+static int cf_check sbi_set_timer_v02(uint64_t stime_value)
+{
+    struct sbiret ret;
+
+    ret = sbi_ecall(SBI_EXT_TIME, SBI_EXT_TIME_SET_TIMER, stime_value,
+#ifdef CONFIG_RISCV_32
+                    stime_value >> 32,
+#else
+                    0,
+#endif
+                    0, 0, 0, 0);
+
+    return sbi_err_map_xen_errno(ret.error);
+}
+
+static int cf_check sbi_set_timer_v01(uint64_t stime_value)
+{
+    struct sbiret ret;
+
+    ret = sbi_ecall(SBI_EXT_0_1_SET_TIMER, 0, stime_value,
+#ifdef CONFIG_RISCV_32
+                    stime_value >> 32,
+#else
+                    0,
+#endif
+                    0, 0, 0, 0);
+
+    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)
 {
@@ -324,7 +356,13 @@ int __init sbi_init(void)
         if ( sbi_probe_extension(SBI_EXT_RFENCE) > 0 )
         {
             sbi_rfence = sbi_rfence_v02;
-            printk("SBI v0.2 RFENCE extension detected\n");
+            dprintk(XENLOG_INFO, "SBI v0.2 RFENCE extension detected\n");
+        }
+
+        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");
         }
     }
     else
-- 
2.53.0

[PATCH v7 10/14] xen/riscv: implement reprogram_timer() via SBI

[Oleksii Kurochko]

Implement reprogram_timer() on RISC-V using the standard SBI timer call.

The privileged architecture only defines machine-mode timer interrupts
(using mtime/mtimecmp). Therefore, timer services for S/HS/VS mode must
be provided by M-mode via SBI calls. SSTC (Supervisor-mode Timer Control)
is optional and is not supported on the boards available to me, so the
only viable approach today is to program the timer through SBI.

reprogram_timer() enables/disables the supervisor timer interrupt and
programs the next timer deadline using sbi_set_timer(). If the SBI call
fails, the code panics, because sbi_set_timer() is expected to return
either 0 or -ENOSUPP (this has been stable from early OpenSBI versions to
the latest ones). The SBI spec does not define a standard negative error
code for this call, and without SSTC there is no alternative method to
program the timer, so the SBI timer call must be available.

reprogram_timer() currently returns int for compatibility with the
existing prototype. While it might be cleaner to return bool, keeping the
existing signature avoids premature changes in case sbi_set_timer() ever
needs to return other values (based on which we could try to avoid
panic-ing) in the future.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in v5 - v7:
 - Nothing changed. Only rebase.
---
Changes in v4:
 - Add Acked-by: Jan Beulich <jbeulich@suse.com>.
---
Changes in v3:
 - Correct the comments in reprogram_timer().
 - Move enablement of timer interrupt after sbi_set_timer() to avoid
   potentially receiving a timer interrupt between these 2 operations.
---
Changes in v2:
 - Add TODO comment above sbi_set_timer() call.
 - Update the commit message.
---
 xen/arch/riscv/stubs.c |  5 -----
 xen/arch/riscv/time.c  | 43 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
index eedf8bf9350a..2f3a0ce76af9 100644
--- a/xen/arch/riscv/stubs.c
+++ b/xen/arch/riscv/stubs.c
@@ -21,11 +21,6 @@ nodemask_t __read_mostly node_online_map = { { [0] = 1UL } };
 
 /* time.c */
 
-int reprogram_timer(s_time_t timeout)
-{
-    BUG_ON("unimplemented");
-}
-
 void send_timer_event(struct vcpu *v)
 {
     BUG_ON("unimplemented");
diff --git a/xen/arch/riscv/time.c b/xen/arch/riscv/time.c
index 2c7af0a5d63b..7efa76fdbcb1 100644
--- a/xen/arch/riscv/time.c
+++ b/xen/arch/riscv/time.c
@@ -7,6 +7,9 @@
 #include <xen/time.h>
 #include <xen/types.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;
 
@@ -40,6 +43,46 @@ static void __init preinit_dt_xen_time(void)
     cpu_khz = rate / 1000;
 }
 
+int reprogram_timer(s_time_t timeout)
+{
+    uint64_t deadline, now;
+    int rc;
+
+    if ( timeout == 0 )
+    {
+        /* Disable timer interrupt */
+        csr_clear(CSR_SIE, BIT(IRQ_S_TIMER, UL));
+
+        return 1;
+    }
+
+    deadline = ns_to_ticks(timeout) + boot_clock_cycles;
+    now = get_cycles();
+    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)) )
+        panic("%s: timer wasn't set because: %d\n", __func__, rc);
+
+    /* Enable timer interrupt */
+    csr_set(CSR_SIE, BIT(IRQ_S_TIMER, UL));
+
+    return 1;
+}
+
 void __init preinit_xen_time(void)
 {
     if ( acpi_disabled )
-- 
2.53.0

[PATCH v7 11/14] xen/riscv: handle hypervisor timer interrupts

[Oleksii Kurochko]

Introduce timer_interrupt() to process IRQ_S_TIMER interrupts.
The handler disables further timer interrupts by clearing
SIE.STIE and raises TIMER_SOFTIRQ so the generic timer subsystem
can perform its processing.

Update do_trap() to dispatch IRQ_S_TIMER to this new handler.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in v4-v7:
 - Nothing changed. Only rebase.
---
Changes in v3:
 - add Acked-by: Jan Beulich <jbeulich@suse.com>.
---
Changes in v2:
 - Drop cause argument of timer_interrupt() as it isn't used inside
   the function and anyway it is pretty clear what is the cause inside
   timer_interrupt().
---
 xen/arch/riscv/traps.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index 244264c92a79..326f2be62823 100644
--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -10,6 +10,7 @@
 #include <xen/lib.h>
 #include <xen/nospec.h>
 #include <xen/sched.h>
+#include <xen/softirq.h>
 
 #include <asm/cpufeature.h>
 #include <asm/intc.h>
@@ -180,6 +181,15 @@ static void check_for_pcpu_work(void)
     p2m_handle_vmenter();
 }
 
+static void timer_interrupt(void)
+{
+    /* Disable the timer to avoid more interrupts */
+    csr_clear(CSR_SIE, BIT(IRQ_S_TIMER, UL));
+
+    /* Signal the generic timer code to do its work */
+    raise_softirq(TIMER_SOFTIRQ);
+}
+
 void do_trap(struct cpu_user_regs *cpu_regs)
 {
     register_t pc = cpu_regs->sepc;
@@ -221,6 +231,10 @@ void do_trap(struct cpu_user_regs *cpu_regs)
                 intc_handle_external_irqs(cpu_regs);
                 break;
 
+            case IRQ_S_TIMER:
+                timer_interrupt();
+                break;
+
             default:
                 intr_handled = false;
                 break;
-- 
2.53.0

[PATCH v7 12/14] xen/riscv: init tasklet subsystem

[Oleksii Kurochko]

As the tasklet subsystem is now initialized, it is necessary to implement
sync_local_execstate(), since it is invoked when something calls
tasklet_softirq_action(), which is registered in tasklet_subsys_init().

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in v6-v7:
 - Nothing changed. Only rebase.
---
Changes in v5:
 - It was something wrong with prev. rebase. So fix that and move
   removing of sync_local_execstate() and sync_vcpu_execstate() to the
   next patch.
---
Changes in v4:
 - Nothing changed. Only rebase.
---
Changes in v3:
 - add Acked-by: Jan Beulich <jbeulich@suse.com>.
---
Changes in v2:
 - Update the commit message.
 - Move implementation of sync_vcpu_execstate() to separate commit
   as it doesn't connect to tasklet subsystem.
---
 xen/arch/riscv/setup.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index bca6ca09eddd..cae49bb29626 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -12,6 +12,7 @@
 #include <xen/serial.h>
 #include <xen/shutdown.h>
 #include <xen/smp.h>
+#include <xen/tasklet.h>
 #include <xen/timer.h>
 #include <xen/vmap.h>
 #include <xen/xvmalloc.h>
@@ -133,6 +134,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
         panic("Booting using ACPI isn't supported\n");
     }
 
+    tasklet_subsys_init();
+
     init_IRQ();
 
     riscv_fill_hwcap();
-- 
2.53.0

[PATCH v7 13/14] xen/riscv: implement sync_vcpu_execstate()

[Oleksii Kurochko]

The scheduler may call this function to force synchronization of given
vCPU's state. RISC-V does not support lazy context switching, so nothing
is done in sync_vcpu_execstate() and sync_local_execstate().

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in v6-v7:
 - Nothing changed. Only rebase.
---
Changes in v5:
 - It was something wrong with prev. rebase. Drop stubs for
   sync_local_execstate() and sync_vcpu_execstate() in this patch.
---
Changes in v4:
 - Drop footer as [PATCH] sched: move vCPU exec state barriers
   is merged to upstream/staging.
 - Add Acked-by: Jan Beulich <jbeulich@suse.com>.
---
Changes in v3:
 - Align sync_vcpu_execstate() with patch:
     [PATCH] sched: move vCPU exec state barriers
---
Changes in v2:
 - New patch.
---
 xen/arch/riscv/domain.c | 10 ++++++++++
 xen/arch/riscv/stubs.c  | 10 ----------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/xen/arch/riscv/domain.c b/xen/arch/riscv/domain.c
index c8ce1efa884d..7e3070101714 100644
--- a/xen/arch/riscv/domain.c
+++ b/xen/arch/riscv/domain.c
@@ -278,6 +278,16 @@ void vcpu_kick(struct vcpu *v)
     }
 }
 
+void sync_local_execstate(void)
+{
+    /* Nothing to do -- no lazy switching */
+}
+
+void sync_vcpu_execstate(struct vcpu *v)
+{
+    /* Nothing to do -- no lazy switching */
+}
+
 static void __init __maybe_unused build_assertions(void)
 {
     /*
diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
index 2f3a0ce76af9..acbb5b9123ea 100644
--- a/xen/arch/riscv/stubs.c
+++ b/xen/arch/riscv/stubs.c
@@ -91,16 +91,6 @@ void continue_running(struct vcpu *same)
     BUG_ON("unimplemented");
 }
 
-void sync_local_execstate(void)
-{
-    BUG_ON("unimplemented");
-}
-
-void sync_vcpu_execstate(struct vcpu *v)
-{
-    BUG_ON("unimplemented");
-}
-
 void startup_cpu_idle_loop(void)
 {
     BUG_ON("unimplemented");
-- 
2.53.0

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

[Oleksii Kurochko]

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 01/14] xen/riscv: detect and store supported hypervisor CSR bits at boot

[Jan Beulich]

On 06.03.2026 17:33, Oleksii Kurochko wrote:
> Some hypervisor CSRs expose optional functionality and may not implement
> all architectural bits. Writing unsupported bits can either be ignored
> or raise an exception depending on the platform.
> 
> Detect the set of writable bits for selected hypervisor CSRs at boot and
> store the resulting masks for later use. This allows safely programming
> these CSRs during vCPU context switching and avoids relying on hardcoded
> architectural assumptions.
> 
> Use csr_read()&csr_write() instead of csr_swap()+all ones mask as some
> CSR registers have WPRI fields which should be preserved during write
> operation.
> 
> Also, ro_one struct is introduced to cover the cases when a bit in CSR
> register (at the momemnt, it is only hstateen0) may be r/o-one to have
> hypervisor view of register seen by guest correct.
> 
> Masks are calculated at the moment only for hedeleg, henvcfg, hideleg,
> hstateen0 registers as only them are going to be used in the follow up
> patch.
> 
> If the Smstateen extension is not implemented, hstateen0 cannot be read
> because the register is considered non-existent. Instructions that attempt
> to access a CSR that is not implemented or not visible in the current mode
> are reserved and will raise an illegal-instruction exception.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

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

I'll commit as-is, yet still a couple of remarks:

> ---
> Changes in V7:
>  - Use csr_read_set() in INIT_CSR_MASK() instead of csr_read()+csr_write().
>  - Add undef of INIT_CSR_MASK().
>  - Move local variable old above INIT_CSR_MASK().

This contradicts ...

>  - Introduce INIT_RO_ONE_MASK() to init csr_masks.ro_one.* fields.
>  - Introduce defines for masks intead of constants.
>  - Move old variable inside macros INIT_CSR_MASK() and INIT_RO_ONE_MASK().

... this. You may want to prune revlog entries when making incremental
changes within one revision.

> --- a/xen/arch/riscv/domain.c
> +++ b/xen/arch/riscv/domain.c
> @@ -2,9 +2,66 @@
>  
>  #include <xen/init.h>
>  #include <xen/mm.h>
> +#include <xen/sections.h>
>  #include <xen/sched.h>
>  #include <xen/vmap.h>
>  
> +#include <asm/cpufeature.h>
> +#include <asm/csr.h>
> +
> +struct csr_masks {
> +    register_t hedeleg;
> +    register_t henvcfg;
> +    register_t hideleg;
> +    register_t hstateen0;
> +
> +    struct {
> +        register_t hstateen0;
> +    } ro_one;
> +};
> +
> +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)

It's not quite clear to me what AVAIL in here is to signal. It's also not
quite clear to me why you would use _UL() in #define-s sitting in a C file
(and hence not possibly being used in assembly code; even for asm() I'd
expect constants to be properly passed in as C operands).

> +void __init init_csr_masks(void)
> +{
> +    /*
> +     * The mask specifies the bits that may be safely modified without
> +     * causing side effects.
> +     *
> +     * For example, registers such as henvcfg or hstateen0 contain WPRI
> +     * 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); \
> +        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_HSTATEEN0, mask); \
> +        csr_masks.ro_one.field = csr_swap(CSR_##csr, old) & mask; \
> +    } while (0)
> +
> +    INIT_CSR_MASK(HEDELEG, hedeleg, HEDELEG_AVAIL_MASK);
> +    INIT_CSR_MASK(HIDELEG, hideleg, HIDELEG_AVAIL_MASK);
> +
> +    INIT_CSR_MASK(HENVCFG, henvcfg, HENVCFG_AVAIL_MASK);
> +
> +    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);
> +    }

The 3rd macro parameters are now redundant. At the example of INIT_CSR_MASK(),
you could now have

#define INIT_CSR_MASK(csr, field) do { \
        register_t old = csr_read_set(CSR_ ## csr, csr ## _AVAIL_MASK); \
        csr_masks.field = csr_swap(CSR_ ## csr, old); \
    } while (0)

This would reduce the risk of incomplete editing after copy-and-paste, or
other typo-ing.

Note also that ## being a binary operator, ./CODING_STYLE wants us to put
blanks around it just like for non-pre-processor binary operators. I'll
try to remember to make that adjustment when committing.

Jan

Re: [PATCH v7 03/14] xen/riscv: introduce tracking of pending vCPU interrupts, part 1

[Jan Beulich]

On 06.03.2026 17:33, Oleksii Kurochko wrote:
> Based on Linux kernel v6.16.0.
> Note that smp_wmb() is used instead of smp_mb__before_atomic() as what
> we want to guarantee that if a bit in irqs_pending_mask is obversable
> that the correspondent bit in irqs_pending is observable too.
> 
> Add lockless tracking of pending vCPU interrupts using atomic bitops.
> Two bitmaps are introduced:
>  - irqs_pending — interrupts currently pending for the vCPU
>  - irqs_pending_mask — bits that have changed in irqs_pending
> 
> The design follows a multi-producer, single-consumer model, where the
> consumer is the vCPU itself. Producers may set bits in
> irqs_pending_mask without a lock. Clearing bits in irqs_pending_mask is
> performed only by the consumer via xchg(). The consumer must not write
> to irqs_pending and must not act on bits that are not set in the mask.
> Otherwise, extra synchronization should be provided.
> 
> On RISC-V interrupts are not injected via guest registers, so pending
> interrupts must be recorded in irqs_pending (using the new
> vcpu_{un}set_interrupt() helpers) and flushed to the guest by updating
> HVIP before returning control to the guest. The consumer side is
> implemented in a follow-up patch.
> 
> A barrier between updating irqs_pending and setting the corresponding
> mask bit in vcpu_set_interrupt()/vcpu_unset_interrupt() guarantees
> that if the consumer observes a mask bit set, the corresponding pending
> bit is also visible. This prevents missed interrupts during the flush.
> 
> It is possible that a guest could have pending bit in the hardware
> register without being marked pending in irq_pending bitmap as:
>   According to the RISC-V ISA specification:
>     Bits hip.VSSIP and hie.VSSIE are the interrupt-pending and
>     interrupt-enable  bits for VS-level software interrupts. VSSIP in hip
>     is an alias (writable) of the same bit in hvip.
>   Additionally:
>     When bit 2 of hideleg is zero, vsip.SSIP and vsie.SSIE are read-only
>     zeros. Else, vsip.SSIP and vsie.SSIE are aliases of hip.VSSIP and
>     hie.VSSIE.
> This means the guest may modify vsip.SSIP, which implicitly updates
> hip.VSSIP and the bit being written with 1 would also trigger an interrupt
> as according to the RISC-V spec:
>   These conditions for an interrupt trap to occur must be evaluated in a
>   bounded   amount of time from when an interrupt becomes, or ceases to be,
>   pending in sip,  and must also be evaluated immediately following the
>   execution of an SRET  instruction or an explicit write to a CSR on which
>   these interrupt trap conditions expressly depend (including sip, sie and
>   sstatus).
> What means that IRQ_VS_SOFT must be synchronized separately, what is done
> in vcpu_sync_interrupts(). Note, also, that IRQ_PMU_OVF would want to be
> synced for the similar reason as IRQ_VS_SOFT, but isn't sync-ed now as
> PMU isn't supported now.
> 
> For the remaining VS-level interrupt types (IRQ_VS_TIMER and
> IRQ_VS_EXT), the specification states they cannot be modified by the guest
> and are read-only because of:
>   Bits hip.VSEIP and hie.VSEIE are the interrupt-pending and interrupt-enable
>   bits for VS-level external interrupts. VSEIP is read-only in hip, and is
>   the logical-OR of these interrupt sources:
>     • bit VSEIP of hvip;
>     • the bit of hgeip selected by hstatus.VGEIN; and
>     • any other platform-specific external interrupt signal directed to
>       VS-level.
>   Bits hip.VSTIP and hie.VSTIE are the interrupt-pending and interrupt-enable
>   bits for VS-level timer interrupts. VSTIP is read-only in hip, and is the
>   logical-OR of hvip.VSTIP and any other platform-specific timer interrupt
>   signal directed to VS-level.
> and
>   When bit 10 of hideleg is zero, vsip.SEIP and vsie.SEIE are read-only zeros.
>   Else, vsip.SEIP and vsie.SEIE are aliases of hip.VSEIP and hie.VSEIE.
> 
>   When bit 6 of hideleg is zero, vsip.STIP and vsie.STIE are read-only zeros.
>   Else, vsip.STIP and vsie.STIE are aliases of hip.VSTIP and hie.VSTIE.
> and also,
>   Bits sip.SEIP and sie.SEIE are the interrupt-pending and interrupt-enable
>   bits for supervisor-level external interrupts. If implemented, SEIP is
>   read-only in sip, and is set and cleared by the execution environment,
>   typically through a platform-specific interrupt controller.
> 
>   Bits sip.STIP and sie.STIE are the interrupt-pending and interrupt-enable
>   bits for supervisor-level timer interrupts. If implemented, STIP is
>   read-only in sip, and is set and cleared by the execution environment
> Thus, for these interrupt types, it is sufficient to use vcpu_set_interrupt()
> and vcpu_unset_interrupt(), and flush them during the call of
> vcpu_flush_interrupts() (which is introduced in follow up patch).
> 
> vcpu_sync_interrupts(), which is called just before entering the VM,
> slightly bends the rule that the irqs_pending bit must be written
> first, followed by updating the corresponding bit in irqs_pending_mask.
> However, it still respects the core guarantee that the producer never
> clears the mask and only writes to irqs_pending if it is the one that
> flipped the corresponding mask bit from 0 to 1.
> Moreover, since the consumer won't run concurrently because
> vcpu_sync_interrupts() and the consumer path are going to be invoked
> sequentially immediately before VM entry, it is safe to slightly relax
> this ordering rule in vcpu_sync_interrupts().
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

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

Re: [PATCH v7 01/14] xen/riscv: detect and store supported hypervisor CSR bits at boot

[Jan Beulich]

On 10.03.2026 09:11, Jan Beulich wrote:
> On 06.03.2026 17:33, Oleksii Kurochko wrote:
>> +void __init init_csr_masks(void)
>> +{
>> +    /*
>> +     * The mask specifies the bits that may be safely modified without
>> +     * causing side effects.
>> +     *
>> +     * For example, registers such as henvcfg or hstateen0 contain WPRI
>> +     * 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); \
>> +        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_HSTATEEN0, mask); \
>> +        csr_masks.ro_one.field = csr_swap(CSR_##csr, old) & mask; \
>> +    } while (0)
>> +
>> +    INIT_CSR_MASK(HEDELEG, hedeleg, HEDELEG_AVAIL_MASK);
>> +    INIT_CSR_MASK(HIDELEG, hideleg, HIDELEG_AVAIL_MASK);
>> +
>> +    INIT_CSR_MASK(HENVCFG, henvcfg, HENVCFG_AVAIL_MASK);
>> +
>> +    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);
>> +    }
> 
> The 3rd macro parameters are now redundant. At the example of INIT_CSR_MASK(),
> you could now have
> 
> #define INIT_CSR_MASK(csr, field) do { \
>         register_t old = csr_read_set(CSR_ ## csr, csr ## _AVAIL_MASK); \
>         csr_masks.field = csr_swap(CSR_ ## csr, old); \
>     } while (0)
> 
> This would reduce the risk of incomplete editing after copy-and-paste, or
> other typo-ing.
> 
> Note also that ## being a binary operator, ./CODING_STYLE wants us to put
> blanks around it just like for non-pre-processor binary operators. I'll
> try to remember to make that adjustment when committing.

Oh, I'm also going to replace the bogus CSR_HSTATEEN0 inside the
INIT_RO_ONE_MASK() macro definition.

Jan

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

[Jan Beulich]

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 01/14] xen/riscv: detect and store supported hypervisor CSR bits at boot

[Oleksii Kurochko]

On 3/10/26 9:11 AM, Jan Beulich wrote:
> On 06.03.2026 17:33, Oleksii Kurochko wrote:
>> Some hypervisor CSRs expose optional functionality and may not implement
>> all architectural bits. Writing unsupported bits can either be ignored
>> or raise an exception depending on the platform.
>>
>> Detect the set of writable bits for selected hypervisor CSRs at boot and
>> store the resulting masks for later use. This allows safely programming
>> these CSRs during vCPU context switching and avoids relying on hardcoded
>> architectural assumptions.
>>
>> Use csr_read()&csr_write() instead of csr_swap()+all ones mask as some
>> CSR registers have WPRI fields which should be preserved during write
>> operation.
>>
>> Also, ro_one struct is introduced to cover the cases when a bit in CSR
>> register (at the momemnt, it is only hstateen0) may be r/o-one to have
>> hypervisor view of register seen by guest correct.
>>
>> Masks are calculated at the moment only for hedeleg, henvcfg, hideleg,
>> hstateen0 registers as only them are going to be used in the follow up
>> patch.
>>
>> If the Smstateen extension is not implemented, hstateen0 cannot be read
>> because the register is considered non-existent. Instructions that attempt
>> to access a CSR that is not implemented or not visible in the current mode
>> are reserved and will raise an illegal-instruction exception.
>>
>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>> --- a/xen/arch/riscv/domain.c
>> +++ b/xen/arch/riscv/domain.c
>> @@ -2,9 +2,66 @@
>>   
>>   #include <xen/init.h>
>>   #include <xen/mm.h>
>> +#include <xen/sections.h>
>>   #include <xen/sched.h>
>>   #include <xen/vmap.h>
>>   
>> +#include <asm/cpufeature.h>
>> +#include <asm/csr.h>
>> +
>> +struct csr_masks {
>> +    register_t hedeleg;
>> +    register_t henvcfg;
>> +    register_t hideleg;
>> +    register_t hstateen0;
>> +
>> +    struct {
>> +        register_t hstateen0;
>> +    } ro_one;
>> +};
>> +
>> +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)
> It's not quite clear to me what AVAIL in here is to signal.

It signal that these bits are potentially available for s/w to be set.
If you want to suggest the better naming and can change that in the
follow-up patch.


>   It's also not
> quite clear to me why you would use _UL() in #define-s sitting in a C file
> (and hence not possibly being used in assembly code; even for asm() I'd
> expect constants to be properly passed in as C operands).

I thought it is always be good to use _UL() for such type of constants as
ULONG_MAX also uses UL, but not in form of _UL() macros. If it would be
better to drop, I can do that in follow-up patch.

>
>> +void __init init_csr_masks(void)
>> +{
>> +    /*
>> +     * The mask specifies the bits that may be safely modified without
>> +     * causing side effects.
>> +     *
>> +     * For example, registers such as henvcfg or hstateen0 contain WPRI
>> +     * 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); \
>> +        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_HSTATEEN0, mask); \
>> +        csr_masks.ro_one.field = csr_swap(CSR_##csr, old) & mask; \
>> +    } while (0)
>> +
>> +    INIT_CSR_MASK(HEDELEG, hedeleg, HEDELEG_AVAIL_MASK);
>> +    INIT_CSR_MASK(HIDELEG, hideleg, HIDELEG_AVAIL_MASK);
>> +
>> +    INIT_CSR_MASK(HENVCFG, henvcfg, HENVCFG_AVAIL_MASK);
>> +
>> +    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);
>> +    }
> The 3rd macro parameters are now redundant. At the example of INIT_CSR_MASK(),
> you could now have
>
> #define INIT_CSR_MASK(csr, field) do { \
>          register_t old = csr_read_set(CSR_ ## csr, csr ## _AVAIL_MASK); \
>          csr_masks.field = csr_swap(CSR_ ## csr, old); \
>      } while (0)
>
> This would reduce the risk of incomplete editing after copy-and-paste, or
> other typo-ing.
>
> Note also that ## being a binary operator, ./CODING_STYLE wants us to put
> blanks around it just like for non-pre-processor binary operators. I'll
> try to remember to make that adjustment when committing.

Good point. Thanks a lot!

~ Oleksii

Re: [PATCH v7 01/14] xen/riscv: detect and store supported hypervisor CSR bits at boot

[Jan Beulich]

On 10.03.2026 17:00, Oleksii Kurochko wrote:
> On 3/10/26 9:11 AM, Jan Beulich wrote:
>> On 06.03.2026 17:33, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/domain.c
>>> +++ b/xen/arch/riscv/domain.c
>>> @@ -2,9 +2,66 @@
>>>   
>>>   #include <xen/init.h>
>>>   #include <xen/mm.h>
>>> +#include <xen/sections.h>
>>>   #include <xen/sched.h>
>>>   #include <xen/vmap.h>
>>>   
>>> +#include <asm/cpufeature.h>
>>> +#include <asm/csr.h>
>>> +
>>> +struct csr_masks {
>>> +    register_t hedeleg;
>>> +    register_t henvcfg;
>>> +    register_t hideleg;
>>> +    register_t hstateen0;
>>> +
>>> +    struct {
>>> +        register_t hstateen0;
>>> +    } ro_one;
>>> +};
>>> +
>>> +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)
>> It's not quite clear to me what AVAIL in here is to signal.
> 
> It signal that these bits are potentially available for s/w to be set.
> If you want to suggest the better naming and can change that in the
> follow-up patch.

I'd either omit the infix altogether ("avail" after all often means
"available for software use"), or use "valid" or (less desirable)
"defined".

>>   It's also not
>> quite clear to me why you would use _UL() in #define-s sitting in a C file
>> (and hence not possibly being used in assembly code; even for asm() I'd
>> expect constants to be properly passed in as C operands).
> 
> I thought it is always be good to use _UL() for such type of constants as
> ULONG_MAX also uses UL, but not in form of _UL() macros. If it would be
> better to drop, I can do that in follow-up patch.

The suffixes want to be there, at the very least for Misra's sake. But
you can just write 0xabcdUL, there's no need to involve a macro there.
That's only needed when the appending of the suffix needs to be
conditional upon is being C or assembler code that includes a header.

Jan

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

[Oleksii Kurochko]

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

[Oleksii Kurochko]

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

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

[Jan Beulich]

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

[Oleksii Kurochko]

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

[Jan Beulich]

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