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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 2/11/26 8:49 AM, Jan Beulich wrote:
> 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?
Good question. Actually by read-only CSRs RISC-V spec means that a CSR is
read-only by its design:
  The standard RISC-V ISA sets aside a 12-bit encoding space (csr[11:0])
  for up to 4,096 CSRs. By convention, the upper 4 bits of the CSR address
  (csr[11:8]) are used to encode the read and write accessibility of the
  CSRs according to privilege level as shown in Table 3. The top two bits
  (csr[11:10]) indicate whether the register is read/write (00,01, or 10)
  or read-only (11).
But logically it seems like if CSR is read-only then technically all CSR
bits are read-only as anyway an exception will occur if CSR is read-only.

So CSRRW* can't be used for such read-only CSRs as write always happen
and it will lead to an exception.

I can add in the commit message that the quote about CSRRS considers only
not read-only CSRs if it makes sense as if CSR is read-only then we won't
calculate a mask for it.

> > 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.
I put it here as I wasn't able to find better place. If it is okay to have
it in domain.c then I'm okay to move this variable there.


> > @@ -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".
Thanks for explanation MISRA stuff.

I will rename X() here to INIT_CSR_MASK() and add do {...} while(0) to deal
with if()'s brackets.

~ Oleksii