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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 3/3/26 1:23 PM, Jan Beulich wrote:
> On 26.02.2026 12:51, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/domain.c
> > +++ b/xen/arch/riscv/domain.c
> > @@ -2,9 +2,56 @@
> >   
> >   #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;
> > +
> > +    struct {
> > +        register_t hstateen0;
> > +    } ro_one;
> > +};
> > +
> > +static struct csr_masks __ro_after_init csr_masks;
> > +
> > +void __init init_csr_masks(void)
> > +{
> > +    /*
> > +     * The mask specifies the bits that may be safely modified without
> > +     * causing side effects.
> > +     *
> > +     * For example, registers such as henvcfg or hstateen0 contain WPRI
> > +     * fields that must be preserved. Any write to the full register must
> > +     * therefore retain the original values of those fields.
> > +     */
> > +#define INIT_CSR_MASK(csr, field, mask) do { \
> > +        old = csr_read(CSR_##csr); \
> > +        csr_write(CSR_##csr, (old & ~(mask)) | (mask)); \
> I (now) agree csr_swap() can't be used here, but isn't the above
>
>      old = csr_read_set(CSR_##csr, mask);
>
> ?
Agree, csr_read_set() could be used.


> > +        csr_masks.field = csr_swap(CSR_##csr, old); \
> > +    } while (0)
> > +
> > +    register_t old;
> Since the macro uses the variable, this decl may better move up.
>
> > +    INIT_CSR_MASK(HEDELEG, hedeleg, ULONG_MAX);
> > +    INIT_CSR_MASK(HIDELEG, hideleg, ULONG_MAX);
> > +
> > +    INIT_CSR_MASK(HENVCFG, henvcfg, _UL(0xE0000003000000FF));
> This raw hex number (also the other one below) isn't quite nice. Can we have
> a #define for this, please? It doesn't need to live in a header file if it's
> not going to be used anywhere else.
Sure, would it be okay to define them inside this init_csr_masks() or at the top
of the file would be better?

> > +    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_smstateen) )
> > +    {
> > +        INIT_CSR_MASK(HSTATEEN0, hstateen0, _UL(0xDE00000000000007));
> > +        csr_masks.ro_one.hstateen0 = old;
> What guarantees that only r/o-one bits are set in the incoming hstateen0? I
> can't help thinking that to determine those bits you want to use
> csr_read_clear() (or csr_clear()).
Good point, then after INIT_CSR_MASK() it will be needed to do:
  csr_masks.ro_one.hstateen0 = csr_read_clear(CSR_HSTATEEN0, 
_UL(0xDE00000000000007));
  csr_swap(CSR_HSTATEEN0, old);

Probably, csr_swap() isn't needed as it would be good to have all writable bits 
by
default 0. Of course, except r/o-one bits.

Thanks.

~ Oleksii