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 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            | 30 ++++++++++++++++++++++++++++++
 xen/arch/riscv/include/asm/setup.h |  2 ++
 xen/arch/riscv/setup.c             |  2 ++
 3 files changed, 34 insertions(+)

diff --git a/xen/arch/riscv/domain.c b/xen/arch/riscv/domain.c
index b60320b90def..8b05f0f23b77 100644
--- a/xen/arch/riscv/domain.c
+++ b/xen/arch/riscv/domain.c
@@ -2,9 +2,39 @@
 
 #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;
+};
+
+static struct csr_masks __ro_after_init csr_masks;
+
+void __init init_csr_masks(void)
+{
+#define INIT_CSR_MASK(csr, field) do { \
+    register_t old; \
+    old = csr_read(CSR_##csr); \
+    csr_set(CSR_##csr, ULONG_MAX); \
+    csr_masks.field = csr_swap(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);
+}
+
 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 20.02.2026 17:18, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/domain.c
> +++ b/xen/arch/riscv/domain.c
> @@ -2,9 +2,39 @@
>  
>  #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;
> +};
> +
> +static struct csr_masks __ro_after_init csr_masks;
> +
> +void __init init_csr_masks(void)
> +{
> +#define INIT_CSR_MASK(csr, field) do { \
> +    register_t old; \
> +    old = csr_read(CSR_##csr); \

Can't this be the initializer of the variable? Can't ...

> +    csr_set(CSR_##csr, ULONG_MAX); \

... csr_swap() be used here, too?

> +    csr_masks.field = csr_swap(CSR_##csr, old); \
> +} while (0)

This whole macro body would also better be indented by one level, to not leave
in particular this closing brace as a misleading one.

Happy to make adjustments while committing, provided you agree. With the
adjustments (or clarification why any of them shouldn't be done):
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

On 2/24/26 9:07 AM, Jan Beulich wrote:
> On 20.02.2026 17:18, Oleksii Kurochko wrote:
>> --- a/xen/arch/riscv/domain.c
>> +++ b/xen/arch/riscv/domain.c
>> @@ -2,9 +2,39 @@
>>   
>>   #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;
>> +};
>> +
>> +static struct csr_masks __ro_after_init csr_masks;
>> +
>> +void __init init_csr_masks(void)
>> +{
>> +#define INIT_CSR_MASK(csr, field) do { \
>> +    register_t old; \
>> +    old = csr_read(CSR_##csr); \
> Can't this be the initializer of the variable? Can't ...

I agree that this is change is okay to be done but I am not sure about ...

>
>> +    csr_set(CSR_##csr, ULONG_MAX); \
> ... csr_swap() be used here, too?

... as after re-reading spec again I am not sure that we can do in this way
at all.

Initially I used csr_set() instead of csr_swap() or csr_write() as csr_set() to
take into account a writability of the bit, so it won't touch a bit if it
is r/o.
But it seems like this approach won't work with**WLRL or WPRI fields as these
fields aren't r/o, but they only support specific value and for example:

- Implementations are permitted but not required to raise an
   illegal-instruction exception if an instruction attempts to write a
   non-supported value to a WLRL field.
- For these reserved fields, software is required to preserve the existing
   values when writing to other fields in the same register. Overwriting them
   with 1s could be considered non-compliant behavior.

So it seems like we can't just do csr_swap() with all 1s and it is needed
to pass a mask to INIT_CSR_MASK() which will tell which bits should be
ignored and don't touched.
For example, HENVCFG and HSTATEEN0 has WPRI fields.

reserved_bits_mask = 0x1FFFFFFCFFFFFF00;
tmp = csr_read(HENVCFG);
tmp=(~reserved_bits_mask) |(tmp&reserved_bits_mask); csr_set(HENVCFG, tmp);


>> +    csr_masks.field = csr_swap(CSR_##csr, old); \
>> +} while (0)
> This whole macro body would also better be indented by one level, to not leave
> in particular this closing brace as a misleading one.

Do you mean that it should be:

+void __init init_csr_masks(void)
+{
+#define INIT_CSR_MASK(csr, field) \
     do {
          ....
     } while (0)
#endif

I will update it.

>
> Happy to make adjustments while committing, provided you agree. With the
> adjustments (or clarification why any of them shouldn't be done):
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

If what I wrote above make sense then it seems that I have to send a new
version of this patch.

Thanks.

~ Oleksii

On 24.02.2026 10:44, Oleksii Kurochko wrote:
> 
> On 2/24/26 9:07 AM, Jan Beulich wrote:
>> On 20.02.2026 17:18, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/domain.c
>>> +++ b/xen/arch/riscv/domain.c
>>> @@ -2,9 +2,39 @@
>>>   
>>>   #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;
>>> +};
>>> +
>>> +static struct csr_masks __ro_after_init csr_masks;
>>> +
>>> +void __init init_csr_masks(void)
>>> +{
>>> +#define INIT_CSR_MASK(csr, field) do { \
>>> +    register_t old; \
>>> +    old = csr_read(CSR_##csr); \
>> Can't this be the initializer of the variable? Can't ...
> 
> I agree that this is change is okay to be done but I am not sure about ...
> 
>>
>>> +    csr_set(CSR_##csr, ULONG_MAX); \
>> ... csr_swap() be used here, too?
> 
> ... as after re-reading spec again I am not sure that we can do in this way
> at all.
> 
> Initially I used csr_set() instead of csr_swap() or csr_write() as csr_set() to
> take into account a writability of the bit, so it won't touch a bit if it
> is r/o.

To me the spec isn't quite clear enough in this regard.

> But it seems like this approach won't work with**WLRL or WPRI fields as these
> fields aren't r/o,

In the CSRs presently dealt with, are there any WLRL fields at all? (I don't
think WPRI fields represent much of an issue for the purpose here.)

> but they only support specific value and for example:
> 
> - Implementations are permitted but not required to raise an
>    illegal-instruction exception if an instruction attempts to write a
>    non-supported value to a WLRL field.
> - For these reserved fields, software is required to preserve the existing
>    values when writing to other fields in the same register. Overwriting them
>    with 1s could be considered non-compliant behavior.
> 
> So it seems like we can't just do csr_swap() with all 1s and it is needed
> to pass a mask to INIT_CSR_MASK() which will tell which bits should be
> ignored and don't touched.
> For example, HENVCFG and HSTATEEN0 has WPRI fields.
> 
> reserved_bits_mask = 0x1FFFFFFCFFFFFF00;
> tmp = csr_read(HENVCFG);
> tmp=(~reserved_bits_mask) |(tmp&reserved_bits_mask); csr_set(HENVCFG, tmp);

What I find further concerning is that HSTATEEN0 also may have read-only-1
fields. You don't currently cope with that, I think.

>>> +    csr_masks.field = csr_swap(CSR_##csr, old); \
>>> +} while (0)
>> This whole macro body would also better be indented by one level, to not leave
>> in particular this closing brace as a misleading one.
> 
> Do you mean that it should be:
> 
> +void __init init_csr_masks(void)
> +{
> +#define INIT_CSR_MASK(csr, field) \
>      do {
>           ....
>      } while (0)
> #endif
> 
> I will update it.

Yes, with no #endif of course. The "do {" could also stay where you had it.

>> Happy to make adjustments while committing, provided you agree. With the
>> adjustments (or clarification why any of them shouldn't be done):
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> If what I wrote above make sense then it seems that I have to send a new
> version of this patch.

Not sure there, but the hstateen0 aspect needs dealing with, I think.

Jan

On 2/24/26 11:16 AM, Jan Beulich wrote:
> On 24.02.2026 10:44, Oleksii Kurochko wrote:
>> On 2/24/26 9:07 AM, Jan Beulich wrote:
>>> On 20.02.2026 17:18, Oleksii Kurochko wrote:
>>>> --- a/xen/arch/riscv/domain.c
>>>> +++ b/xen/arch/riscv/domain.c
>>>> @@ -2,9 +2,39 @@
>>>>    
>>>>    #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;
>>>> +};
>>>> +
>>>> +static struct csr_masks __ro_after_init csr_masks;
>>>> +
>>>> +void __init init_csr_masks(void)
>>>> +{
>>>> +#define INIT_CSR_MASK(csr, field) do { \
>>>> +    register_t old; \
>>>> +    old = csr_read(CSR_##csr); \
>>> Can't this be the initializer of the variable? Can't ...
>> I agree that this is change is okay to be done but I am not sure about ...
>>
>>>> +    csr_set(CSR_##csr, ULONG_MAX); \
>>> ... csr_swap() be used here, too?
>> ... as after re-reading spec again I am not sure that we can do in this way
>> at all.
>>
>> Initially I used csr_set() instead of csr_swap() or csr_write() as csr_set() to
>> take into account a writability of the bit, so it won't touch a bit if it
>> is r/o.
> To me the spec isn't quite clear enough in this regard.
>
>> But it seems like this approach won't work with**WLRL or WPRI fields as these
>> fields aren't r/o,
> In the CSRs presently dealt with, are there any WLRL fields at all? (I don't
> think WPRI fields represent much of an issue for the purpose here.)

Agree, currently used CSRs in this function don't have WLRL feilds, but I suppose
we want to have the similar treatment (read WLRL fields and reuse what was read)
for WLRL fields as these fields expect only valid value and so all 1s for this
fields can be wrong to use.

>
>> but they only support specific value and for example:
>>
>> - Implementations are permitted but not required to raise an
>>     illegal-instruction exception if an instruction attempts to write a
>>     non-supported value to a WLRL field.
>> - For these reserved fields, software is required to preserve the existing
>>     values when writing to other fields in the same register. Overwriting them
>>     with 1s could be considered non-compliant behavior.
>>
>> So it seems like we can't just do csr_swap() with all 1s and it is needed
>> to pass a mask to INIT_CSR_MASK() which will tell which bits should be
>> ignored and don't touched.
>> For example, HENVCFG and HSTATEEN0 has WPRI fields.
>>
>> reserved_bits_mask = 0x1FFFFFFCFFFFFF00;
>> tmp = csr_read(HENVCFG);
>> tmp=(~reserved_bits_mask) |(tmp&reserved_bits_mask); csr_set(HENVCFG, tmp);
> What I find further concerning is that HSTATEEN0 also may have read-only-1
> fields. You don't currently cope with that, I think.

Because of this:
   A bit in an hstateen CSR cannot be read-only one unless the same bit is
   read-only one in the matching mstateen CSR.
?

I expect that it will be covered by csr_set() which will touch only writable
bits and ignore read-only. So doesn't matter if a bit is read only 1 or 0
it still shouldn't be in the final mask.

~ Oleksii

On 24.02.2026 11:42, Oleksii Kurochko wrote:
> 
> On 2/24/26 11:16 AM, Jan Beulich wrote:
>> On 24.02.2026 10:44, Oleksii Kurochko wrote:
>>> On 2/24/26 9:07 AM, Jan Beulich wrote:
>>>> On 20.02.2026 17:18, Oleksii Kurochko wrote:
>>>>> --- a/xen/arch/riscv/domain.c
>>>>> +++ b/xen/arch/riscv/domain.c
>>>>> @@ -2,9 +2,39 @@
>>>>>    
>>>>>    #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;
>>>>> +};
>>>>> +
>>>>> +static struct csr_masks __ro_after_init csr_masks;
>>>>> +
>>>>> +void __init init_csr_masks(void)
>>>>> +{
>>>>> +#define INIT_CSR_MASK(csr, field) do { \
>>>>> +    register_t old; \
>>>>> +    old = csr_read(CSR_##csr); \
>>>> Can't this be the initializer of the variable? Can't ...
>>> I agree that this is change is okay to be done but I am not sure about ...
>>>
>>>>> +    csr_set(CSR_##csr, ULONG_MAX); \
>>>> ... csr_swap() be used here, too?
>>> ... as after re-reading spec again I am not sure that we can do in this way
>>> at all.
>>>
>>> Initially I used csr_set() instead of csr_swap() or csr_write() as csr_set() to
>>> take into account a writability of the bit, so it won't touch a bit if it
>>> is r/o.
>> To me the spec isn't quite clear enough in this regard.
>>
>>> But it seems like this approach won't work with**WLRL or WPRI fields as these
>>> fields aren't r/o,
>> In the CSRs presently dealt with, are there any WLRL fields at all? (I don't
>> think WPRI fields represent much of an issue for the purpose here.)
> 
> Agree, currently used CSRs in this function don't have WLRL feilds, but I suppose
> we want to have the similar treatment (read WLRL fields and reuse what was read)
> for WLRL fields as these fields expect only valid value and so all 1s for this
> fields can be wrong to use.
> 
>>
>>> but they only support specific value and for example:
>>>
>>> - Implementations are permitted but not required to raise an
>>>     illegal-instruction exception if an instruction attempts to write a
>>>     non-supported value to a WLRL field.
>>> - For these reserved fields, software is required to preserve the existing
>>>     values when writing to other fields in the same register. Overwriting them
>>>     with 1s could be considered non-compliant behavior.
>>>
>>> So it seems like we can't just do csr_swap() with all 1s and it is needed
>>> to pass a mask to INIT_CSR_MASK() which will tell which bits should be
>>> ignored and don't touched.
>>> For example, HENVCFG and HSTATEEN0 has WPRI fields.
>>>
>>> reserved_bits_mask = 0x1FFFFFFCFFFFFF00;
>>> tmp = csr_read(HENVCFG);
>>> tmp=(~reserved_bits_mask) |(tmp&reserved_bits_mask); csr_set(HENVCFG, tmp);
>> What I find further concerning is that HSTATEEN0 also may have read-only-1
>> fields. You don't currently cope with that, I think.
> 
> Because of this:
>    A bit in an hstateen CSR cannot be read-only one unless the same bit is
>    read-only one in the matching mstateen CSR.
> ?
> 
> I expect that it will be covered by csr_set() which will touch only writable
> bits and ignore read-only. So doesn't matter if a bit is read only 1 or 0
> it still shouldn't be in the final mask.

But the hypervisor view of the register value seen by guests won't be correct.
Recall that you moved to probing to make sure that the cached values we have
in the hypervisor properly match the values seen by the guest?

Jan

On 2/24/26 11:47 AM, Jan Beulich wrote:
> On 24.02.2026 11:42, Oleksii Kurochko wrote:
>> On 2/24/26 11:16 AM, Jan Beulich wrote:
>>> On 24.02.2026 10:44, Oleksii Kurochko wrote:
>>>> On 2/24/26 9:07 AM, Jan Beulich wrote:
>>>>> On 20.02.2026 17:18, Oleksii Kurochko wrote:
>>>>>> --- a/xen/arch/riscv/domain.c
>>>>>> +++ b/xen/arch/riscv/domain.c
>>>>>> @@ -2,9 +2,39 @@
>>>>>>     
>>>>>>     #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;
>>>>>> +};
>>>>>> +
>>>>>> +static struct csr_masks __ro_after_init csr_masks;
>>>>>> +
>>>>>> +void __init init_csr_masks(void)
>>>>>> +{
>>>>>> +#define INIT_CSR_MASK(csr, field) do { \
>>>>>> +    register_t old; \
>>>>>> +    old = csr_read(CSR_##csr); \
>>>>> Can't this be the initializer of the variable? Can't ...
>>>> I agree that this is change is okay to be done but I am not sure about ...
>>>>
>>>>>> +    csr_set(CSR_##csr, ULONG_MAX); \
>>>>> ... csr_swap() be used here, too?
>>>> ... as after re-reading spec again I am not sure that we can do in this way
>>>> at all.
>>>>
>>>> Initially I used csr_set() instead of csr_swap() or csr_write() as csr_set() to
>>>> take into account a writability of the bit, so it won't touch a bit if it
>>>> is r/o.
>>> To me the spec isn't quite clear enough in this regard.
>>>
>>>> But it seems like this approach won't work with**WLRL or WPRI fields as these
>>>> fields aren't r/o,
>>> In the CSRs presently dealt with, are there any WLRL fields at all? (I don't
>>> think WPRI fields represent much of an issue for the purpose here.)
>> Agree, currently used CSRs in this function don't have WLRL feilds, but I suppose
>> we want to have the similar treatment (read WLRL fields and reuse what was read)
>> for WLRL fields as these fields expect only valid value and so all 1s for this
>> fields can be wrong to use.
>>
>>>> but they only support specific value and for example:
>>>>
>>>> - Implementations are permitted but not required to raise an
>>>>      illegal-instruction exception if an instruction attempts to write a
>>>>      non-supported value to a WLRL field.
>>>> - For these reserved fields, software is required to preserve the existing
>>>>      values when writing to other fields in the same register. Overwriting them
>>>>      with 1s could be considered non-compliant behavior.
>>>>
>>>> So it seems like we can't just do csr_swap() with all 1s and it is needed
>>>> to pass a mask to INIT_CSR_MASK() which will tell which bits should be
>>>> ignored and don't touched.
>>>> For example, HENVCFG and HSTATEEN0 has WPRI fields.
>>>>
>>>> reserved_bits_mask = 0x1FFFFFFCFFFFFF00;
>>>> tmp = csr_read(HENVCFG);
>>>> tmp=(~reserved_bits_mask) |(tmp&reserved_bits_mask); csr_set(HENVCFG, tmp);
>>> What I find further concerning is that HSTATEEN0 also may have read-only-1
>>> fields. You don't currently cope with that, I think.
>> Because of this:
>>     A bit in an hstateen CSR cannot be read-only one unless the same bit is
>>     read-only one in the matching mstateen CSR.
>> ?
>>
>> I expect that it will be covered by csr_set() which will touch only writable
>> bits and ignore read-only. So doesn't matter if a bit is read only 1 or 0
>> it still shouldn't be in the final mask.
> But the hypervisor view of the register value seen by guests won't be correct.
> Recall that you moved to probing to make sure that the cached values we have
> in the hypervisor properly match the values seen by the guest?

Then we have to store what csr_read(hstateen0) returns in struct csr_masks in
new field hstateen0_ro_ones. And then in the next patch apply that new field
in vcpu_csr_init():
   v->arch.hstateen0 = hstateen0 & csr_masks.hstateen0 |
                       csr_masks.hstateen0_ro_ones;

Are you okay with such changes?

~ Oleksii

On 24.02.2026 12:25, Oleksii Kurochko wrote:
> 
> On 2/24/26 11:47 AM, Jan Beulich wrote:
>> On 24.02.2026 11:42, Oleksii Kurochko wrote:
>>> On 2/24/26 11:16 AM, Jan Beulich wrote:
>>>> On 24.02.2026 10:44, Oleksii Kurochko wrote:
>>>>> On 2/24/26 9:07 AM, Jan Beulich wrote:
>>>>>> On 20.02.2026 17:18, Oleksii Kurochko wrote:
>>>>>>> --- a/xen/arch/riscv/domain.c
>>>>>>> +++ b/xen/arch/riscv/domain.c
>>>>>>> @@ -2,9 +2,39 @@
>>>>>>>     
>>>>>>>     #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;
>>>>>>> +};
>>>>>>> +
>>>>>>> +static struct csr_masks __ro_after_init csr_masks;
>>>>>>> +
>>>>>>> +void __init init_csr_masks(void)
>>>>>>> +{
>>>>>>> +#define INIT_CSR_MASK(csr, field) do { \
>>>>>>> +    register_t old; \
>>>>>>> +    old = csr_read(CSR_##csr); \
>>>>>> Can't this be the initializer of the variable? Can't ...
>>>>> I agree that this is change is okay to be done but I am not sure about ...
>>>>>
>>>>>>> +    csr_set(CSR_##csr, ULONG_MAX); \
>>>>>> ... csr_swap() be used here, too?
>>>>> ... as after re-reading spec again I am not sure that we can do in this way
>>>>> at all.
>>>>>
>>>>> Initially I used csr_set() instead of csr_swap() or csr_write() as csr_set() to
>>>>> take into account a writability of the bit, so it won't touch a bit if it
>>>>> is r/o.
>>>> To me the spec isn't quite clear enough in this regard.
>>>>
>>>>> But it seems like this approach won't work with**WLRL or WPRI fields as these
>>>>> fields aren't r/o,
>>>> In the CSRs presently dealt with, are there any WLRL fields at all? (I don't
>>>> think WPRI fields represent much of an issue for the purpose here.)
>>> Agree, currently used CSRs in this function don't have WLRL feilds, but I suppose
>>> we want to have the similar treatment (read WLRL fields and reuse what was read)
>>> for WLRL fields as these fields expect only valid value and so all 1s for this
>>> fields can be wrong to use.
>>>
>>>>> but they only support specific value and for example:
>>>>>
>>>>> - Implementations are permitted but not required to raise an
>>>>>      illegal-instruction exception if an instruction attempts to write a
>>>>>      non-supported value to a WLRL field.
>>>>> - For these reserved fields, software is required to preserve the existing
>>>>>      values when writing to other fields in the same register. Overwriting them
>>>>>      with 1s could be considered non-compliant behavior.
>>>>>
>>>>> So it seems like we can't just do csr_swap() with all 1s and it is needed
>>>>> to pass a mask to INIT_CSR_MASK() which will tell which bits should be
>>>>> ignored and don't touched.
>>>>> For example, HENVCFG and HSTATEEN0 has WPRI fields.
>>>>>
>>>>> reserved_bits_mask = 0x1FFFFFFCFFFFFF00;
>>>>> tmp = csr_read(HENVCFG);
>>>>> tmp=(~reserved_bits_mask) |(tmp&reserved_bits_mask); csr_set(HENVCFG, tmp);
>>>> What I find further concerning is that HSTATEEN0 also may have read-only-1
>>>> fields. You don't currently cope with that, I think.
>>> Because of this:
>>>     A bit in an hstateen CSR cannot be read-only one unless the same bit is
>>>     read-only one in the matching mstateen CSR.
>>> ?
>>>
>>> I expect that it will be covered by csr_set() which will touch only writable
>>> bits and ignore read-only. So doesn't matter if a bit is read only 1 or 0
>>> it still shouldn't be in the final mask.
>> But the hypervisor view of the register value seen by guests won't be correct.
>> Recall that you moved to probing to make sure that the cached values we have
>> in the hypervisor properly match the values seen by the guest?
> 
> Then we have to store what csr_read(hstateen0) returns in struct csr_masks in
> new field hstateen0_ro_ones. And then in the next patch apply that new field
> in vcpu_csr_init():
>    v->arch.hstateen0 = hstateen0 & csr_masks.hstateen0 |
>                        csr_masks.hstateen0_ro_ones;
> 
> Are you okay with such changes?

Properly structured, sure. That's pretty much unavoidable, isn't it?

As to "structured", for example I wonder whether hstateen0_ro_ones isn't
going to lead to redundancies once further registers appear which may have
r/o-1 fields. Maybe there should be "ro_one" sub-struct right away?

And of course "structured" also touches on proper parenthesization of the
statement above.

Jan

On 2/24/26 12:32 PM, Jan Beulich wrote:
> On 24.02.2026 12:25, Oleksii Kurochko wrote:
>> On 2/24/26 11:47 AM, Jan Beulich wrote:
>>> On 24.02.2026 11:42, Oleksii Kurochko wrote:
>>>> On 2/24/26 11:16 AM, Jan Beulich wrote:
>>>>> On 24.02.2026 10:44, Oleksii Kurochko wrote:
>>>>>> On 2/24/26 9:07 AM, Jan Beulich wrote:
>>>>>>> On 20.02.2026 17:18, Oleksii Kurochko wrote:
>>>>>>>> --- a/xen/arch/riscv/domain.c
>>>>>>>> +++ b/xen/arch/riscv/domain.c
>>>>>>>> @@ -2,9 +2,39 @@
>>>>>>>>      
>>>>>>>>      #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;
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +static struct csr_masks __ro_after_init csr_masks;
>>>>>>>> +
>>>>>>>> +void __init init_csr_masks(void)
>>>>>>>> +{
>>>>>>>> +#define INIT_CSR_MASK(csr, field) do { \
>>>>>>>> +    register_t old; \
>>>>>>>> +    old = csr_read(CSR_##csr); \
>>>>>>> Can't this be the initializer of the variable? Can't ...
>>>>>> I agree that this is change is okay to be done but I am not sure about ...
>>>>>>
>>>>>>>> +    csr_set(CSR_##csr, ULONG_MAX); \
>>>>>>> ... csr_swap() be used here, too?
>>>>>> ... as after re-reading spec again I am not sure that we can do in this way
>>>>>> at all.
>>>>>>
>>>>>> Initially I used csr_set() instead of csr_swap() or csr_write() as csr_set() to
>>>>>> take into account a writability of the bit, so it won't touch a bit if it
>>>>>> is r/o.
>>>>> To me the spec isn't quite clear enough in this regard.
>>>>>
>>>>>> But it seems like this approach won't work with**WLRL or WPRI fields as these
>>>>>> fields aren't r/o,
>>>>> In the CSRs presently dealt with, are there any WLRL fields at all? (I don't
>>>>> think WPRI fields represent much of an issue for the purpose here.)
>>>> Agree, currently used CSRs in this function don't have WLRL feilds, but I suppose
>>>> we want to have the similar treatment (read WLRL fields and reuse what was read)
>>>> for WLRL fields as these fields expect only valid value and so all 1s for this
>>>> fields can be wrong to use.
>>>>
>>>>>> but they only support specific value and for example:
>>>>>>
>>>>>> - Implementations are permitted but not required to raise an
>>>>>>       illegal-instruction exception if an instruction attempts to write a
>>>>>>       non-supported value to a WLRL field.
>>>>>> - For these reserved fields, software is required to preserve the existing
>>>>>>       values when writing to other fields in the same register. Overwriting them
>>>>>>       with 1s could be considered non-compliant behavior.
>>>>>>
>>>>>> So it seems like we can't just do csr_swap() with all 1s and it is needed
>>>>>> to pass a mask to INIT_CSR_MASK() which will tell which bits should be
>>>>>> ignored and don't touched.
>>>>>> For example, HENVCFG and HSTATEEN0 has WPRI fields.
>>>>>>
>>>>>> reserved_bits_mask = 0x1FFFFFFCFFFFFF00;
>>>>>> tmp = csr_read(HENVCFG);
>>>>>> tmp=(~reserved_bits_mask) |(tmp&reserved_bits_mask); csr_set(HENVCFG, tmp);
>>>>> What I find further concerning is that HSTATEEN0 also may have read-only-1
>>>>> fields. You don't currently cope with that, I think.
>>>> Because of this:
>>>>      A bit in an hstateen CSR cannot be read-only one unless the same bit is
>>>>      read-only one in the matching mstateen CSR.
>>>> ?
>>>>
>>>> I expect that it will be covered by csr_set() which will touch only writable
>>>> bits and ignore read-only. So doesn't matter if a bit is read only 1 or 0
>>>> it still shouldn't be in the final mask.
>>> But the hypervisor view of the register value seen by guests won't be correct.
>>> Recall that you moved to probing to make sure that the cached values we have
>>> in the hypervisor properly match the values seen by the guest?
>> Then we have to store what csr_read(hstateen0) returns in struct csr_masks in
>> new field hstateen0_ro_ones. And then in the next patch apply that new field
>> in vcpu_csr_init():
>>     v->arch.hstateen0 = hstateen0 & csr_masks.hstateen0 |
>>                         csr_masks.hstateen0_ro_ones;
>>
>> Are you okay with such changes?
> Properly structured, sure. That's pretty much unavoidable, isn't it?
>
> As to "structured", for example I wonder whether hstateen0_ro_ones isn't
> going to lead to redundancies once further registers appear which may have
> r/o-1 fields. Maybe there should be "ro_one" sub-struct right away?

Maybe, it makes sense. I will then update the csr_masks structure in the
following way:

struct csr_masks {
     register_t hedeleg;
     register_t henvcfg;
     register_t hideleg;
     register_t hstateen0;
     
     struct {
         hstateen0;
     } ro_one;
};

>
> And of course "structured" also touches on proper parenthesization of the
> statement above.

According to https://en.cppreference.com/w/c/language/operator_precedence.html
& has bigger priority, so I haven't put parenthesizes.

~ Oleksii

On 24.02.2026 13:02, Oleksii Kurochko wrote:
> On 2/24/26 12:32 PM, Jan Beulich wrote:
>> And of course "structured" also touches on proper parenthesization of the
>> statement above.
> 
> According to https://en.cppreference.com/w/c/language/operator_precedence.html
> & has bigger priority, so I haven't put parenthesizes.

We tend to put them everywhere where rules from school maths don't apply.

Jan