1. Patchew
  2. Xen
  3. [PATCH v4 00/16] xen/riscv: introduce vtimer related things
  4. View patch

[PATCH v4 02/16] xen/riscv: detect and store supported hypervisor CSR bits at boot

Oleksii Kurochko posted 16 patches 1 month, 4 weeks ago
There is a newer version of this series
[PATCH v4 02/16] xen/riscv: detect and store supported hypervisor CSR bits at boot
Posted by Oleksii Kurochko 1 month, 4 weeks ago 
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.

Note that csr_set() is used instead of csr_write() to write all ones to
the mask, as the CSRRS instruction, according to the RISC-V specification,
sets only those bits that are writable (note that the quote consider only
non-read-only CSRs as writing to read-only CSRs according to the spec.
will raise an exception):
    Any bit that is high in rs1 will cause the corresponding bit to be set
    in the CSR, if that CSR bit is writable.
In contrast, the CSRRW instruction does not take CSR bit writability into
account, which could lead to unintended side effects when writing all ones
to a CSR.

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 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            |  5 +++++
 xen/arch/riscv/include/asm/setup.h |  9 +++++++++
 xen/arch/riscv/setup.c             | 21 +++++++++++++++++++++
 3 files changed, 35 insertions(+)

diff --git a/xen/arch/riscv/domain.c b/xen/arch/riscv/domain.c
index f5c624ac92c7..5572e10bfaa9 100644
--- a/xen/arch/riscv/domain.c
+++ b/xen/arch/riscv/domain.c
@@ -2,9 +2,14 @@
 
 #include <xen/init.h>
 #include <xen/mm.h>
+#include <xen/sections.h>
 #include <xen/sched.h>
 #include <xen/vmap.h>
 
+#include <asm/setup.h>
+
+struct csr_masks __ro_after_init csr_masks;
+
 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..d54f6a2d1d29 100644
--- a/xen/arch/riscv/include/asm/setup.h
+++ b/xen/arch/riscv/include/asm/setup.h
@@ -5,6 +5,15 @@
 
 #include <xen/types.h>
 
+struct csr_masks {
+    register_t hedeleg;
+    register_t henvcfg;
+    register_t hideleg;
+    register_t hstateen0;
+};
+
+extern struct csr_masks csr_masks;
+
 #define max_init_domid (0)
 
 void setup_mm(void);
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 9b4835960d20..dc469b49623f 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -70,6 +70,25 @@ static void * __init relocate_fdt(paddr_t dtb_paddr, size_t dtb_size)
     return fdt;
 }
 
+void __init init_csr_masks(void)
+{
+    register_t old;
+
+#define INIT_CSR_MASK(csr, field) do { \
+        old = csr_read(CSR_##csr); \
+        csr_set(CSR_##csr, ULONG_MAX); \
+        csr_masks.field = csr_read(CSR_##csr); \
+        csr_write(CSR_##csr, old); \
+} while (0)
+
+    INIT_CSR_MASK(HEDELEG, hedeleg);
+    INIT_CSR_MASK(HENVCFG, henvcfg);
+    INIT_CSR_MASK(HIDELEG, hideleg);
+
+    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_smstateen) )
+        INIT_CSR_MASK(HSTATEEN0, hstateen0);
+}
+
 void __init noreturn start_xen(unsigned long bootcpu_id,
                                paddr_t dtb_addr)
 {
@@ -137,6 +156,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
 
     riscv_fill_hwcap();
 
+    init_csr_masks();
+
     preinit_xen_time();
 
     intc_preinit();
-- 
2.52.0
Re: [PATCH v4 02/16] xen/riscv: detect and store supported hypervisor CSR bits at boot
Posted by Jan Beulich 1 month, 3 weeks ago 
On 13.02.2026 17:28, 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.
> 
> Note that csr_set() is used instead of csr_write() to write all ones to
> the mask, as the CSRRS instruction, according to the RISC-V specification,
> sets only those bits that are writable (note that the quote consider only
> non-read-only CSRs as writing to read-only CSRs according to the spec.
> will raise an exception):
>     Any bit that is high in rs1 will cause the corresponding bit to be set
>     in the CSR, if that CSR bit is writable.
> In contrast, the CSRRW instruction does not take CSR bit writability into
> account, which could lead to unintended side effects when writing all ones
> to a CSR.
> 
> 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 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.

Except that ...

> --- a/xen/arch/riscv/domain.c
> +++ b/xen/arch/riscv/domain.c
> @@ -2,9 +2,14 @@
>  
>  #include <xen/init.h>
>  #include <xen/mm.h>
> +#include <xen/sections.h>
>  #include <xen/sched.h>
>  #include <xen/vmap.h>
>  
> +#include <asm/setup.h>
> +
> +struct csr_masks __ro_after_init csr_masks;

... it's not static here and even has ...

> --- a/xen/arch/riscv/include/asm/setup.h
> +++ b/xen/arch/riscv/include/asm/setup.h
> @@ -5,6 +5,15 @@
>  
>  #include <xen/types.h>
>  
> +struct csr_masks {
> +    register_t hedeleg;
> +    register_t henvcfg;
> +    register_t hideleg;
> +    register_t hstateen0;
> +};
> +
> +extern struct csr_masks csr_masks;

... a declaration here. If you want to keep it non-static, it (and the struct
decl) likely wants moving elsewhere. Whereas if you truly want it to be static,
the struct decl would want moving to domain.c as well.

> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -70,6 +70,25 @@ static void * __init relocate_fdt(paddr_t dtb_paddr, size_t dtb_size)
>      return fdt;
>  }
>  
> +void __init init_csr_masks(void)
> +{
> +    register_t old;

As indicated before, this would better be ...

> +#define INIT_CSR_MASK(csr, field) do { \
> +        old = csr_read(CSR_##csr); \
> +        csr_set(CSR_##csr, ULONG_MAX); \
> +        csr_masks.field = csr_read(CSR_##csr); \
> +        csr_write(CSR_##csr, old); \
> +} while (0)

... local to the scope the macro introduces. IOW with both this and the
earlier remark, let's try to strive to have scope and exposure of variables
as narrow as possible (unless of course there are clear reasons not to).

And btw, wouldn't you again better use csr_swap() here?

Jan
Re: [PATCH v4 02/16] xen/riscv: detect and store supported hypervisor CSR bits at boot
Posted by Oleksii Kurochko 1 month, 3 weeks ago 
On 2/17/26 12:37 PM, Jan Beulich wrote:
> On 13.02.2026 17:28, 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.
>>
>> Note that csr_set() is used instead of csr_write() to write all ones to
>> the mask, as the CSRRS instruction, according to the RISC-V specification,
>> sets only those bits that are writable (note that the quote consider only
>> non-read-only CSRs as writing to read-only CSRs according to the spec.
>> will raise an exception):
>>      Any bit that is high in rs1 will cause the corresponding bit to be set
>>      in the CSR, if that CSR bit is writable.
>> In contrast, the CSRRW instruction does not take CSR bit writability into
>> account, which could lead to unintended side effects when writing all ones
>> to a CSR.
>>
>> 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 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.
> Except that ...
>
>> --- a/xen/arch/riscv/domain.c
>> +++ b/xen/arch/riscv/domain.c
>> @@ -2,9 +2,14 @@
>>   
>>   #include <xen/init.h>
>>   #include <xen/mm.h>
>> +#include <xen/sections.h>
>>   #include <xen/sched.h>
>>   #include <xen/vmap.h>
>>   
>> +#include <asm/setup.h>
>> +
>> +struct csr_masks __ro_after_init csr_masks;
> ... it's not static here and even has ...
>
>> --- a/xen/arch/riscv/include/asm/setup.h
>> +++ b/xen/arch/riscv/include/asm/setup.h
>> @@ -5,6 +5,15 @@
>>   
>>   #include <xen/types.h>
>>   
>> +struct csr_masks {
>> +    register_t hedeleg;
>> +    register_t henvcfg;
>> +    register_t hideleg;
>> +    register_t hstateen0;
>> +};
>> +
>> +extern struct csr_masks csr_masks;
> ... a declaration here. If you want to keep it non-static, it (and the struct
> decl) likely wants moving elsewhere. Whereas if you truly want it to be static,
> the struct decl would want moving to domain.c as well.

Wrong patch version. I made it static so csr_masks declaration and struct csr_masks
were in domain.c. Also, init_csr_masks() was moved to domain.c too.

I will update the patch version next time.

>
>> --- a/xen/arch/riscv/setup.c
>> +++ b/xen/arch/riscv/setup.c
>> @@ -70,6 +70,25 @@ static void * __init relocate_fdt(paddr_t dtb_paddr, size_t dtb_size)
>>       return fdt;
>>   }
>>   
>> +void __init init_csr_masks(void)
>> +{
>> +    register_t old;
> As indicated before, this would better be ...
>
>> +#define INIT_CSR_MASK(csr, field) do { \
>> +        old = csr_read(CSR_##csr); \
>> +        csr_set(CSR_##csr, ULONG_MAX); \
>> +        csr_masks.field = csr_read(CSR_##csr); \
>> +        csr_write(CSR_##csr, old); \
>> +} while (0)
> ... local to the scope the macro introduces. IOW with both this and the
> earlier remark, let's try to strive to have scope and exposure of variables
> as narrow as possible (unless of course there are clear reasons not to).
>
> And btw, wouldn't you again better use csr_swap() here?

It makes sense, I'll use csr_swap() instead of pair csr_read() and csr_write().

Thanks.

~ Oleksii

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.