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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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