Re: [PATCH v2 03/39] xen/riscv: set the guest's XLEN explicitly in hstatus.VSXL

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 9/2/26 3:07 PM, Jan Beulich wrote:
> On 02.09.2026 13:42, Oleksii Kurochko wrote:
> > On 9/1/26 5:20 PM, Jan Beulich wrote:
> > > On 27.08.2026 17:20, Oleksii Kurochko wrote:
> > > > --- a/xen/arch/riscv/domain.c
> > > > +++ b/xen/arch/riscv/domain.c
> > > > @@ -88,7 +88,13 @@ static void vcpu_csr_init(struct vcpu *v)
> > > >    {
> > > >        v->arch.hedeleg = HEDELEG_DEFAULT & csr_masks.hedeleg;
> > > >    
> > > > -    vcpu_guest_cpu_user_regs(v)->hstatus = HSTATUS_SPV | HSTATUS_SPVP;
> > > > +    /*
> > > > +     * Xen supports 64-bit guests only, so set the guest's XLEN explicitly
> > > > +     * rather than leaving it to the WARL behaviour of hstatus.VSXL, which the
> > > > +     * decoding of a trapped instruction depends on.
> > > > +     */
> > > > +    vcpu_guest_cpu_user_regs(v)->hstatus =
> > > > +        HSTATUS_SPV | HSTATUS_SPVP | MASK_INSR(HSTATUS_VSXL_64, HSTATUS_VSXL);
> > > The comment is correct right now, but the situation better would change
> > > at some point. Can't you arrange for things to be correct here also for
> > > a future where 32- and 128-bit guests would also be supported?
> > I am not sure about 128-bit guests as H extension is dependent on RV32
> > or RV64 but probably it will be changed:
> > ```
> > The hypervisor extension depends on an "I" base integer ISA with 32 x
> > registers (RV32I or RV64I), not RV32E or RV64E, which have only 16 x
> > registers.
> > ```
> Lots of updates like this likely will be needed for RV128 to actually become
> a thing.
>
> > I will introduce the following (also it will be needed also to check if
> > we could VSXL set at all as implmentation can make that field read-only
> > and do VSXLLEN=HSXLEN):
> >
> > /*
> >    * Return the hstatus.VSXL value encoding the guest's XLEN. The switch()
> >    * deliberately has no default case, so that adding a new domain_type (a
> >    * 128-bit one, in particular) fails to build until this mapping is
> > updated.
> >    */
> > static unsigned int domain_vsxl(const struct domain *d)
> > {
> >       switch ( d->type )
> >       {
> >       case DOMAIN_32BIT:
> >           return HSTATUS_VSXL_32;
> >
> >       case DOMAIN_64BIT:
> >           return HSTATUS_VSXL_64;
> >       }
> >
> >       ASSERT_UNREACHABLE();
> >
> >       return HSTATUS_VSXL_64;
> Why not simply return 0 here? You genuinely don't know the size.
Agree, just 0 will be better.

> > }
> >
> > It will also affect then common code as vcpu_csr_init() could be then
> > called before domain type is set:
> >
> > +++ b/xen/common/device-tree/dom0less-build.c
> > @@ -812,17 +812,18 @@ static int __init construct_domU(struct
> > kernel_info *kinfo,
> >        else if ( rc == 0 && !strcmp(dom0less_enhanced, "no-xenstore") )
> >            kinfo->dom0less_feature = DOM0LESS_ENHANCED_NO_XS;
> >
> > -    if ( vcpu_create(d, 0) == NULL )
> > -        return -ENOMEM;
> > -
> >        d->max_pages = ((paddr_t)mem * SZ_1K) >> PAGE_SHIFT;
> >
> >        rc = kernel_probe(kinfo, node);
> >        if ( rc < 0 )
> >            return rc;
> >
> > +    /* The domain type needs to be known before the first vCPU is
> > created. */
> >        set_domain_type(d, kinfo);
> >
> > +    if ( vcpu_create(d, 0) == NULL )
> > +        return -ENOMEM;
> I don't understand the need for this, likely because I don't see why
> domain_vsxl() would need calling from underneath vcpu_create().
The call trace will be the following:
  vcpu_create() -> arch_vcpu_create() -> vcpu_csr_init() -> 
domain_vsxldomain_vsxl()
    unsigned int vsxl = domain_vsxl(v->domain);

    ...

    vcpu_guest_cpu_user_regs(v)->hstatus =
        HSTATUS_SPV | HSTATUS_SPVP | MASK_INSR(vsxl, HSTATUS_VSXL);

Without moving vcpu_create(d, 0) after set_domain_type(), domain_vsxl() 
will return something wrong.
I also thought about updating of VSXL for each vCPU in 
construct_domain() where d->type is already known and then no changes in 
common code are needed. But I think it is a little bit better just have 
a change in common code.
~ Oleksii