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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.03.2026 17:33, Oleksii Kurochko wrote:
> Some hypervisor CSRs expose optional functionality and may not implement
> all architectural bits. Writing unsupported bits can either be ignored
> or raise an exception depending on the platform.
> 
> Detect the set of writable bits for selected hypervisor CSRs at boot and
> store the resulting masks for later use. This allows safely programming
> these CSRs during vCPU context switching and avoids relying on hardcoded
> architectural assumptions.
> 
> Use csr_read()&csr_write() instead of csr_swap()+all ones mask as some
> CSR registers have WPRI fields which should be preserved during write
> operation.
> 
> Also, ro_one struct is introduced to cover the cases when a bit in CSR
> register (at the momemnt, it is only hstateen0) may be r/o-one to have
> hypervisor view of register seen by guest correct.
> 
> Masks are calculated at the moment only for hedeleg, henvcfg, hideleg,
> hstateen0 registers as only them are going to be used in the follow up
> patch.
> 
> If the Smstateen extension is not implemented, hstateen0 cannot be read
> because the register is considered non-existent. Instructions that attempt
> to access a CSR that is not implemented or not visible in the current mode
> are reserved and will raise an illegal-instruction exception.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>

Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

I'll commit as-is, yet still a couple of remarks:

> ---
> Changes in V7:
>  - Use csr_read_set() in INIT_CSR_MASK() instead of csr_read()+csr_write().
>  - Add undef of INIT_CSR_MASK().
>  - Move local variable old above INIT_CSR_MASK().

This contradicts ...

>  - Introduce INIT_RO_ONE_MASK() to init csr_masks.ro_one.* fields.
>  - Introduce defines for masks intead of constants.
>  - Move old variable inside macros INIT_CSR_MASK() and INIT_RO_ONE_MASK().

... this. You may want to prune revlog entries when making incremental
changes within one revision.

> --- a/xen/arch/riscv/domain.c
> +++ b/xen/arch/riscv/domain.c
> @@ -2,9 +2,66 @@
>  
>  #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;
> +
> +#define HEDELEG_AVAIL_MASK ULONG_MAX
> +#define HIDELEG_AVAIL_MASK ULONG_MAX
> +#define HENVCFG_AVAIL_MASK _UL(0xE0000003000000FF)
> +#define HSTATEEN0_AVAIL_MASK _UL(0xDE00000000000007)

It's not quite clear to me what AVAIL in here is to signal. It's also not
quite clear to me why you would use _UL() in #define-s sitting in a C file
(and hence not possibly being used in assembly code; even for asm() I'd
expect constants to be properly passed in as C operands).

> +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 { \
> +        register_t old = csr_read_set(CSR_##csr, mask); \
> +        csr_masks.field = csr_swap(CSR_##csr, old); \
> +    } while (0)
> +
> +#define INIT_RO_ONE_MASK(csr, field, mask) do { \
> +        register_t old = csr_read_clear(CSR_HSTATEEN0, mask); \
> +        csr_masks.ro_one.field = csr_swap(CSR_##csr, old) & mask; \
> +    } while (0)
> +
> +    INIT_CSR_MASK(HEDELEG, hedeleg, HEDELEG_AVAIL_MASK);
> +    INIT_CSR_MASK(HIDELEG, hideleg, HIDELEG_AVAIL_MASK);
> +
> +    INIT_CSR_MASK(HENVCFG, henvcfg, HENVCFG_AVAIL_MASK);
> +
> +    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_smstateen) )
> +    {
> +        INIT_CSR_MASK(HSTATEEN0, hstateen0, HSTATEEN0_AVAIL_MASK);
> +        INIT_RO_ONE_MASK(HSTATEEN0, hstateen0, HSTATEEN0_AVAIL_MASK);
> +    }

The 3rd macro parameters are now redundant. At the example of INIT_CSR_MASK(),
you could now have

#define INIT_CSR_MASK(csr, field) do { \
        register_t old = csr_read_set(CSR_ ## csr, csr ## _AVAIL_MASK); \
        csr_masks.field = csr_swap(CSR_ ## csr, old); \
    } while (0)

This would reduce the risk of incomplete editing after copy-and-paste, or
other typo-ing.

Note also that ## being a binary operator, ./CODING_STYLE wants us to put
blanks around it just like for non-pre-processor binary operators. I'll
try to remember to make that adjustment when committing.

Jan