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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.02.2026 17:52, 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:
>     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.

Hmm, I wonder in how far the wording there is precise. In a subsequent
paragraph there is:

"For both CSRRS and CSRRC, if rs1=x0, then the instruction will not write
 to the CSR at all, and so shall not cause any of the side effects that
 might otherwise occur on a CSR write, nor raise illegal-instruction
 exceptions on accesses to read-only CSRs."

To me, a read-only CSR is a CSR with all bits read-only. With this
interpretation, the two statements conflict with one another. Is this
interpretation ruled out somewhere?

> Masks are calculated at the moment only for hdeleg, henvcfg, hideleg,

Nit: First one is hedeleg.

> 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 V3:
>  - New patch.
> 
> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -32,6 +32,8 @@
>  unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
>      __aligned(STACK_SIZE);
>  
> +struct csr_masks __ro_after_init csr_masks;

setup.c would be nice to only have __init functions and __initdata data.
Really up to now that's the case, and I wonder why the makefile doesn't
leverage this by using setup.init.o in place of setup.o. This variable
would likely better live elsewhere anyway, imo: Somewhere it's actually
(going to be) used.

> @@ -70,6 +72,28 @@ static void * __init relocate_fdt(paddr_t dtb_paddr, 
> size_t dtb_size)
>      return fdt;
>  }
>  
> +void __init init_csr_masks(void)
> +{
> +    register_t old;
> +
> +#define X(csr, field) \
> +        old = csr_read(CSR_##csr); \
> +        csr_set(CSR_##csr, ULONG_MAX); \
> +        csr_masks.field = csr_read(CSR_##csr); \
> +        csr_write(CSR_##csr, old)

See my remark on the earlier patch regarding locally used macros. You
shouldn't ...

> +    X(HEDELEG, hedeleg);
> +    X(HENVCFG, henvcfg);
> +    X(HIDELEG, hideleg);
> +
> +    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_smstateen) )
> +    {
> +        X(HSTATEEN0, hstateen0);
> +    }

... be required to put braces here. (Then I'd further recommend to make "old"
local to the macro's scope.)

I'm also inclined to recommend to avoid an inflation of X() macros. Give
each such macro a somewhat sensible (yet still short) name. This way you'll
avoid Misra rule 5.4 ("Macro identifiers shall be distinct") concerns, in
combination with rule 20.5 ("#undef should not be used"). Note that we
didn't accept the latter rule, hence why I'm only saying "concerns", not
"violations".

Jan