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 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 | 47 ++++++++++++++++++++++++++++++
xen/arch/riscv/include/asm/setup.h | 2 ++
xen/arch/riscv/setup.c | 2 ++
3 files changed, 51 insertions(+)
diff --git a/xen/arch/riscv/domain.c b/xen/arch/riscv/domain.c
index b60320b90def..902aaac74290 100644
--- a/xen/arch/riscv/domain.c
+++ b/xen/arch/riscv/domain.c
@@ -2,9 +2,56 @@
#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;
+
+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 { \
+ old = csr_read(CSR_##csr); \
+ csr_write(CSR_##csr, (old & ~(mask)) | (mask)); \
+ csr_masks.field = csr_swap(CSR_##csr, old); \
+ } while (0)
+
+ register_t old;
+
+ INIT_CSR_MASK(HEDELEG, hedeleg, ULONG_MAX);
+ INIT_CSR_MASK(HIDELEG, hideleg, ULONG_MAX);
+
+ INIT_CSR_MASK(HENVCFG, henvcfg, _UL(0xE0000003000000FF));
+
+ if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_smstateen) )
+ {
+ INIT_CSR_MASK(HSTATEEN0, hstateen0, _UL(0xDE00000000000007));
+ csr_masks.ro_one.hstateen0 = old;
+ }
+}
+
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.0On 26.02.2026 12:51, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/domain.c
> +++ b/xen/arch/riscv/domain.c
> @@ -2,9 +2,56 @@
>
> #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;
> +
> +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 { \
> + old = csr_read(CSR_##csr); \
> + csr_write(CSR_##csr, (old & ~(mask)) | (mask)); \
I (now) agree csr_swap() can't be used here, but isn't the above
old = csr_read_set(CSR_##csr, mask);
?
> + csr_masks.field = csr_swap(CSR_##csr, old); \
> + } while (0)
> +
> + register_t old;
Since the macro uses the variable, this decl may better move up.
> + INIT_CSR_MASK(HEDELEG, hedeleg, ULONG_MAX);
> + INIT_CSR_MASK(HIDELEG, hideleg, ULONG_MAX);
> +
> + INIT_CSR_MASK(HENVCFG, henvcfg, _UL(0xE0000003000000FF));
This raw hex number (also the other one below) isn't quite nice. Can we have
a #define for this, please? It doesn't need to live in a header file if it's
not going to be used anywhere else.
> + if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_smstateen) )
> + {
> + INIT_CSR_MASK(HSTATEEN0, hstateen0, _UL(0xDE00000000000007));
> + csr_masks.ro_one.hstateen0 = old;
What guarantees that only r/o-one bits are set in the incoming hstateen0? I
can't help thinking that to determine those bits you want to use
csr_read_clear() (or csr_clear()).
> + }
#undef INIT_CSR_MASK
Jan
On 3/3/26 1:23 PM, Jan Beulich wrote:
> On 26.02.2026 12:51, Oleksii Kurochko wrote:
>> --- a/xen/arch/riscv/domain.c
>> +++ b/xen/arch/riscv/domain.c
>> @@ -2,9 +2,56 @@
>>
>> #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;
>> +
>> +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 { \
>> + old = csr_read(CSR_##csr); \
>> + csr_write(CSR_##csr, (old & ~(mask)) | (mask)); \
> I (now) agree csr_swap() can't be used here, but isn't the above
>
> old = csr_read_set(CSR_##csr, mask);
>
> ?
Agree, csr_read_set() could be used.
>
>> + csr_masks.field = csr_swap(CSR_##csr, old); \
>> + } while (0)
>> +
>> + register_t old;
> Since the macro uses the variable, this decl may better move up.
>
>> + INIT_CSR_MASK(HEDELEG, hedeleg, ULONG_MAX);
>> + INIT_CSR_MASK(HIDELEG, hideleg, ULONG_MAX);
>> +
>> + INIT_CSR_MASK(HENVCFG, henvcfg, _UL(0xE0000003000000FF));
> This raw hex number (also the other one below) isn't quite nice. Can we have
> a #define for this, please? It doesn't need to live in a header file if it's
> not going to be used anywhere else.
Sure, would it be okay to define them inside this init_csr_masks() or at the top
of the file would be better?
>
>> + if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_smstateen) )
>> + {
>> + INIT_CSR_MASK(HSTATEEN0, hstateen0, _UL(0xDE00000000000007));
>> + csr_masks.ro_one.hstateen0 = old;
> What guarantees that only r/o-one bits are set in the incoming hstateen0? I
> can't help thinking that to determine those bits you want to use
> csr_read_clear() (or csr_clear()).
Good point, then after INIT_CSR_MASK() it will be needed to do:
csr_masks.ro_one.hstateen0 = csr_read_clear(CSR_HSTATEEN0, _UL(0xDE00000000000007));
csr_swap(CSR_HSTATEEN0, old);
Probably, csr_swap() isn't needed as it would be good to have all writable bits by
default 0. Of course, except r/o-one bits.
Thanks.
~ Oleksii
On 3/4/26 3:54 PM, Oleksii Kurochko wrote:
>>
>>> + if ( riscv_isa_extension_available(NULL,
>>> RISCV_ISA_EXT_smstateen) )
>>> + {
>>> + INIT_CSR_MASK(HSTATEEN0, hstateen0, _UL(0xDE00000000000007));
>>> + csr_masks.ro_one.hstateen0 = old;
>> What guarantees that only r/o-one bits are set in the incoming
>> hstateen0? I
>> can't help thinking that to determine those bits you want to use
>> csr_read_clear() (or csr_clear()).
>
> Good point, then after INIT_CSR_MASK() it will be needed to do:
> csr_masks.ro_one.hstateen0 = csr_read_clear(CSR_HSTATEEN0,
> _UL(0xDE00000000000007));
> csr_swap(CSR_HSTATEEN0, old);
>
> Probably, csr_swap() isn't needed as it would be good to have all
> writable bits by
> default 0. Of course, except r/o-one bits.
I came up with the macros declared inside init_csr_masks():
#define INIT_RO_ONE_MASK(csr, field, mask) do { \
old = csr_read_clear(CSR_HSTATEEN0, mask); \
csr_masks.ro_one.field = csr_swap(CSR_##csr, old) & mask; \
} while (0)
~ Oleksii
On 04.03.2026 15:54, Oleksii Kurochko wrote:
> On 3/3/26 1:23 PM, Jan Beulich wrote:
>> On 26.02.2026 12:51, 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 { \
>>> + old = csr_read(CSR_##csr); \
>>> + csr_write(CSR_##csr, (old & ~(mask)) | (mask)); \
>> I (now) agree csr_swap() can't be used here, but isn't the above
>>
>> old = csr_read_set(CSR_##csr, mask);
>>
>> ?
>
> Agree, csr_read_set() could be used.
>
>>> + csr_masks.field = csr_swap(CSR_##csr, old); \
>>> + } while (0)
>>> +
>>> + register_t old;
>> Since the macro uses the variable, this decl may better move up.
>>
>>> + INIT_CSR_MASK(HEDELEG, hedeleg, ULONG_MAX);
>>> + INIT_CSR_MASK(HIDELEG, hideleg, ULONG_MAX);
>>> +
>>> + INIT_CSR_MASK(HENVCFG, henvcfg, _UL(0xE0000003000000FF));
>> This raw hex number (also the other one below) isn't quite nice. Can we have
>> a #define for this, please? It doesn't need to live in a header file if it's
>> not going to be used anywhere else.
>
> Sure, would it be okay to define them inside this init_csr_masks() or at the top
> of the file would be better?
Either would work imo, as long as you don't expect other uses of these
constants. Personally I'd likely put them not at the top of the file, but
immediately ahead of the function.
Jan
© 2016 - 2026 Red Hat, Inc.