Re: [PATCH v5 01/14] xen/riscv: detect and store supported hypervisor CSR bits at boot

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxx>
> 
> 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