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

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

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

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

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