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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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@xxxxxxxx>
If what I wrote above make sense then it seems that I have to send a new
version of this patch.

Thanks.

~ Oleksii