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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 3/10/26 9:11 AM, Jan Beulich wrote:
> 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>
> > --- 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 signal that these bits are potentially available for s/w to be set.
If you want to suggest the better naming and can change that in the
follow-up patch.


>   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).
I thought it is always be good to use _UL() for such type of constants as
ULONG_MAX also uses UL, but not in form of _UL() macros. If it would be
better to drop, I can do that in follow-up patch.

> > +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.
Good point. Thanks a lot!

~ Oleksii