Re: [PATCH v4 02/16] xen/riscv: detect and store supported hypervisor CSR bits at boot

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.02.2026 17:28, 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.
> 
> Note that csr_set() is used instead of csr_write() to write all ones to
> the mask, as the CSRRS instruction, according to the RISC-V specification,
> sets only those bits that are writable (note that the quote consider only
> non-read-only CSRs as writing to read-only CSRs according to the spec.
> will raise an exception):
>     Any bit that is high in rs1 will cause the corresponding bit to be set
>     in the CSR, if that CSR bit is writable.
> In contrast, the CSRRW instruction does not take CSR bit writability into
> account, which could lead to unintended side effects when writing all ones
> to a CSR.
> 
> 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>
> ---
> Changes in V4:
>  - Move csr_masks defintion to domain.c. Make it static as at the moment
>    it is going to be used only in domain.c.

Except that ...

> --- a/xen/arch/riscv/domain.c
> +++ b/xen/arch/riscv/domain.c
> @@ -2,9 +2,14 @@
>  
>  #include <xen/init.h>
>  #include <xen/mm.h>
> +#include <xen/sections.h>
>  #include <xen/sched.h>
>  #include <xen/vmap.h>
>  
> +#include <asm/setup.h>
> +
> +struct csr_masks __ro_after_init csr_masks;

... it's not static here and even has ...

> --- a/xen/arch/riscv/include/asm/setup.h
> +++ b/xen/arch/riscv/include/asm/setup.h
> @@ -5,6 +5,15 @@
>  
>  #include <xen/types.h>
>  
> +struct csr_masks {
> +    register_t hedeleg;
> +    register_t henvcfg;
> +    register_t hideleg;
> +    register_t hstateen0;
> +};
> +
> +extern struct csr_masks csr_masks;

... a declaration here. If you want to keep it non-static, it (and the struct
decl) likely wants moving elsewhere. Whereas if you truly want it to be static,
the struct decl would want moving to domain.c as well.

> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -70,6 +70,25 @@ static void * __init relocate_fdt(paddr_t dtb_paddr, 
> size_t dtb_size)
>      return fdt;
>  }
>  
> +void __init init_csr_masks(void)
> +{
> +    register_t old;

As indicated before, this would better be ...

> +#define INIT_CSR_MASK(csr, field) do { \
> +        old = csr_read(CSR_##csr); \
> +        csr_set(CSR_##csr, ULONG_MAX); \
> +        csr_masks.field = csr_read(CSR_##csr); \
> +        csr_write(CSR_##csr, old); \
> +} while (0)

... local to the scope the macro introduces. IOW with both this and the
earlier remark, let's try to strive to have scope and exposure of variables
as narrow as possible (unless of course there are clear reasons not to).

And btw, wouldn't you again better use csr_swap() here?

Jan