[PATCH v4 04/16] xen/riscv: implement vcpu_csr_init()

Oleksii Kurochko posted 16 patches 1 month, 4 weeks ago
There is a newer version of this series
[PATCH v4 04/16] xen/riscv: implement vcpu_csr_init()
Posted by Oleksii Kurochko 1 month, 4 weeks ago
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>
---
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                     | 66 +++++++++++++++++++++
 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, 76 insertions(+)

diff --git a/xen/arch/riscv/domain.c b/xen/arch/riscv/domain.c
index 5572e10bfaa9..6c8a6269d791 100644
--- a/xen/arch/riscv/domain.c
+++ b/xen/arch/riscv/domain.c
@@ -6,10 +6,74 @@
 #include <xen/sched.h>
 #include <xen/vmap.h>
 
+#include <asm/cpufeature.h>
+#include <asm/csr.h>
+#include <asm/riscv_encoding.h>
 #include <asm/setup.h>
 
 struct csr_masks __ro_after_init csr_masks;
 
+#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 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) )
+    {
+        register_t hstateen0 = 0;
+
+        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;
+
+        /* Allow guest to access CSR_ENVCFG */
+        hstateen0 |= SMSTATEEN0_HSENVCFG;
+
+        v->arch.hstateen0 = hstateen0 & csr_masks.hstateen0;
+    }
+}
+
 static void continue_new_vcpu(struct vcpu *prev)
 {
     BUG_ON("unimplemented\n");
@@ -41,6 +105,8 @@ int arch_vcpu_create(struct vcpu *v)
     if ( is_idle_vcpu(v) )
         return rc;
 
+    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 f78f145258d6..6bb06a50c6ab 100644
--- a/xen/arch/riscv/include/asm/domain.h
+++ b/xen/arch/riscv/include/asm/domain.h
@@ -48,6 +48,12 @@ struct arch_vcpu {
     } xen_saved_context;
 
     struct cpu_info *cpu_info;
+
+    register_t hcounteren;
+    register_t hedeleg;
+    register_t henvcfg;
+    register_t hideleg;
+    register_t hstateen0;
 };
 
 struct paging_domain {
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.52.0
Re: [PATCH v4 04/16] xen/riscv: implement vcpu_csr_init()
Posted by Jan Beulich 1 month, 3 weeks ago
On 13.02.2026 17:28, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/domain.c
> +++ b/xen/arch/riscv/domain.c
> @@ -6,10 +6,74 @@
>  #include <xen/sched.h>
>  #include <xen/vmap.h>
>  
> +#include <asm/cpufeature.h>
> +#include <asm/csr.h>
> +#include <asm/riscv_encoding.h>
>  #include <asm/setup.h>
>  
>  struct csr_masks __ro_after_init csr_masks;
>  
> +#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 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) )
> +    {
> +        register_t hstateen0 = 0;
> +
> +        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;
> +
> +        /* Allow guest to access CSR_ENVCFG */
> +        hstateen0 |= SMSTATEEN0_HSENVCFG;

I continue to be puzzled by the use of = vs |=. If you use |=, best do so
uniformly. Then inserting new code ahead of the one you have now is not a
problem. I wonder anyway why you don't do (omitting commentary):

        register_t hstateen0 = SMSTATEEN0_HSENVCFG;

        if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_ssaia) )
            hstateen0 |= SMSTATEEN0_AIA | SMSTATEEN0_IMSIC | SMSTATEEN0_SVSLCT;

As to CSR_ENVCFG - what's that referring to? I'm aware of menvcfg, henvcfg,
and senvcfg. But I'm unaware of plain envcfg, and there's also no CSR_ENVCFG
constant in riscv_encoding.h afaics. I assume it's senvcfg that you mean
here. And then - is this CSR unconditionally available? The "Supervisor ISA"
isn't called an extension, yet at the same time it's also part of the
separate "privileged" specification, not the general one.

> --- a/xen/arch/riscv/include/asm/domain.h
> +++ b/xen/arch/riscv/include/asm/domain.h
> @@ -48,6 +48,12 @@ struct arch_vcpu {
>      } xen_saved_context;
>  
>      struct cpu_info *cpu_info;
> +
> +    register_t hcounteren;
> +    register_t hedeleg;
> +    register_t henvcfg;
> +    register_t hideleg;
> +    register_t hstateen0;
>  };

One question about the ordering here: It looks to be alphabetically sorted
right now, yet I wonder whether that's optimal. Some CSRs might typically
be used together, in which case they may best live close together (for
chances to be good that they end up in the same cache line).

Jan
Re: [PATCH v4 04/16] xen/riscv: implement vcpu_csr_init()
Posted by Oleksii Kurochko 1 month, 3 weeks ago
On 2/17/26 3:23 PM, Jan Beulich wrote:
> On 13.02.2026 17:28, Oleksii Kurochko wrote:
>> --- a/xen/arch/riscv/domain.c
>> +++ b/xen/arch/riscv/domain.c
>> @@ -6,10 +6,74 @@
>>   #include <xen/sched.h>
>>   #include <xen/vmap.h>
>>   
>> +#include <asm/cpufeature.h>
>> +#include <asm/csr.h>
>> +#include <asm/riscv_encoding.h>
>>   #include <asm/setup.h>
>>   
>>   struct csr_masks __ro_after_init csr_masks;
>>   
>> +#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 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) )
>> +    {
>> +        register_t hstateen0 = 0;
>> +
>> +        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;
>> +
>> +        /* Allow guest to access CSR_ENVCFG */
>> +        hstateen0 |= SMSTATEEN0_HSENVCFG;
> I continue to be puzzled by the use of = vs |=. If you use |=, best do so
> uniformly. Then inserting new code ahead of the one you have now is not a
> problem. I wonder anyway why you don't do (omitting commentary):
>
>          register_t hstateen0 = SMSTATEEN0_HSENVCFG;
>
>          if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_ssaia) )
>              hstateen0 |= SMSTATEEN0_AIA | SMSTATEEN0_IMSIC | SMSTATEEN0_SVSLCT;
>
> As to CSR_ENVCFG - what's that referring to? I'm aware of menvcfg, henvcfg,
> and senvcfg. But I'm unaware of plain envcfg, and there's also no CSR_ENVCFG
> constant in riscv_encoding.h afaics. I assume it's senvcfg that you mean
> here.

I referred to CSR_SENVCFG, I just automatically applied the way how it is defined
in Linux kernel and Linux kernel abstracts it as CSR_ENVCFG as it could be booted
in M-mode or S-mode. There is no such definition in Xen as we don't use it.


>   And then - is this CSR unconditionally available? The "Supervisor ISA"
> isn't called an extension, yet at the same time it's also part of the
> separate "privileged" specification, not the general one.

I don't know, the available specs aren't precise here. Considering that
OpenSBI(menvcfg) or Linux(menvcfg or senvcfg) uses them unconditionally
then an assumption that CSR_*ENVCFG unconditionally exist could be done.

Anyway, a ENVCFG bit in hstateen0 depends only on RISCV_ISA_EXT_smstateen
(or Ssstateen extension) then it is okay to set that bit and even
CSR_*ENVCFG isn't implemented a trap to Xen will happen and it should be
handled somehow separately as I mentioned above Linux kernel uses them
unconditionally.

>
>> --- a/xen/arch/riscv/include/asm/domain.h
>> +++ b/xen/arch/riscv/include/asm/domain.h
>> @@ -48,6 +48,12 @@ struct arch_vcpu {
>>       } xen_saved_context;
>>   
>>       struct cpu_info *cpu_info;
>> +
>> +    register_t hcounteren;
>> +    register_t hedeleg;
>> +    register_t henvcfg;
>> +    register_t hideleg;
>> +    register_t hstateen0;
>>   };
> One question about the ordering here: It looks to be alphabetically sorted
> right now, yet I wonder whether that's optimal. Some CSRs might typically
> be used together, in which case they may best live close together (for
> chances to be good that they end up in the same cache line).

Make sense, I will group then hedeleg and hideleg.

Thanks.

~ Oleksii