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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> }
> 
> 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().

Jan