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