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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

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@gmail.com>
>> --- 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