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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

?

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

> +    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()).

> +    }

#undef INIT_CSR_MASK

Jan