Re: [PATCH v3 04/16] xen/riscv: implement vcpu_csr_init()

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

On 2/11/26 10:44 AM, Jan Beulich wrote:
> On 09.02.2026 17:52, Oleksii Kurochko wrote:
>> --- a/xen/arch/riscv/domain.c
>> +++ b/xen/arch/riscv/domain.c
>> @@ -5,6 +5,72 @@
>>   #include <xen/sched.h>
>>   #include <xen/vmap.h>
>>   
>> +#include <asm/cpufeature.h>
>> +#include <asm/csr.h>
>> +#include <asm/riscv_encoding.h>
>> +#include <asm/setup.h>
>> +
>> +#define HEDELEG_DEFAULT (BIT(CAUSE_MISALIGNED_FETCH, U) | \
>> +                         BIT(CAUSE_FETCH_ACCESS, U) | \
>> +                         BIT(CAUSE_ILLEGAL_INSTRUCTION, U) | \
>> +                         BIT(CAUSE_BREAKPOINT, U) | \
>> +                         BIT(CAUSE_MISALIGNED_LOAD, U) | \
>> +                         BIT(CAUSE_LOAD_ACCESS, U) | \
>> +                         BIT(CAUSE_MISALIGNED_STORE, U) | \
>> +                         BIT(CAUSE_STORE_ACCESS, U) | \
>> +                         BIT(CAUSE_USER_ECALL, U) | \
>> +                         BIT(CAUSE_FETCH_PAGE_FAULT, U) | \
>> +                         BIT(CAUSE_LOAD_PAGE_FAULT, U) | \
>> +                         BIT(CAUSE_STORE_PAGE_FAULT, U))
>> +
>> +#define HIDELEG_DEFAULT (MIP_VSSIP | MIP_VSTIP | MIP_VSEIP)
>> +
>> +static void vcpu_csr_init(struct vcpu *v)
>> +{
>> +    register_t hstateen0;
> There not being an initializer here, ...
>
>> +    v->arch.hedeleg = HEDELEG_DEFAULT & csr_masks.hedeleg;
>> +
>> +    vcpu_guest_cpu_user_regs(v)->hstatus = HSTATUS_SPV | HSTATUS_SPVP;
>> +
>> +    v->arch.hideleg = HIDELEG_DEFAULT & csr_masks.hideleg;
>> +
>> +    /*
>> +     * VS should access only the time counter directly.
>> +     * Everything else should trap.
>> +     */
>> +    v->arch.hcounteren = HCOUNTEREN_TM;
>> +
>> +    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svpbmt) )
>> +        v->arch.henvcfg = ENVCFG_PBMTE & csr_masks.henvcfg;
>> +
>> +    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_smstateen) )
>> +    {
>> +         if (riscv_isa_extension_available(NULL, RISCV_ISA_EXT_ssaia))
> (Nit: Style.)
>
>> +            /*
>> +             * If the hypervisor extension is implemented, the same three
>> +             * bitsare defined also in hypervisor CSR hstateen0 but concern
> (Nit: "bits are")
>
>> +             * only the state potentially accessible to a virtual machine
>> +             * executing in privilege modes VS and VU:
>> +             *      bit 60 CSRs siselect and sireg (really vsiselect and
>> +             *             vsireg)
>> +             *      bit 59 CSRs siph and sieh (RV32 only) and stopi (really
>> +             *             vsiph, vsieh, and vstopi)
>> +             *      bit 58 all state of IMSIC guest interrupt files, including
>> +             *             CSR stopei (really vstopei)
>> +             * If one of these bits is zero in hstateen0, and the same bit is
>> +             * one in mstateen0, then an attempt to access the corresponding
>> +             * state from VS or VU-mode raises a virtual instruction exception.
>> +             */
>> +            hstateen0 = SMSTATEEN0_AIA | SMSTATEEN0_IMSIC | SMSTATEEN0_SVSLCT;
>> +
>> +        /* Allow guest to access CSR_ENVCFG */
>> +        hstateen0 |= SMSTATEEN0_HSENVCFG;
> ... doesn't the compiler complain about the use of a possibly uninitialized
> variable? The variable also wants to move to the more narrow scope.

Hmm, for some reason it doesn't. Anyway, I agree that it would be better to move
it to a narrower scope, since `hstateen0` exists only when RISCV_ISA_EXT_smstateen
is supported.


>
>> @@ -32,6 +98,8 @@ int arch_vcpu_create(struct vcpu *v)
>>       v->arch.xen_saved_context.sp = (register_t)v->arch.cpu_info;
>>       v->arch.xen_saved_context.ra = (register_t)continue_new_vcpu;
>>   
>> +    vcpu_csr_init(v);
>> +
>>       /* Idle VCPUs don't need the rest of this setup */
>>       if ( is_idle_vcpu(v) )
>>           return rc;
> Do idle vCPU-s really need to have vcpu_csr_init() called for them?

Agree, there is no any sense. I will move the call of vcpu_csr_init() after
is_idle_vcpu() check.

Thanks.

~ Oleksii