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

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

On 11/4/26 3:37 PM, Baptiste Le Duc wrote:
>> [ ... 97 lines skipped ... ]
>> --- a/xen/arch/riscv/domain.c
>> +++ b/xen/arch/riscv/domain.c
>> @@ -51,6 +51,119 @@ static struct csr_masks __ro_after_init csr_masks;
>>   #define HENVCFG_VALID_MASK 0xe0000003000000ffUL
>>   #define HSTATEEN0_VALID_MASK 0xde00000000000007UL
>>   
>> +/*
>> + * Return the hstatus.VSXL value encoding the guest's XLEN. Whether the
>> + * hardware can actually provide that width is a separate question, answered
>> + * by hstatus_vsxl_settable().
>> + *
>> + * 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.
>> + */
>> +unsigned int domain_vsxl(const struct domain *d)
>> +{
>> +    switch ( d->type )
>> +    {
>> +    case DOMAIN_32BIT:
>> +        return XLEN_FIELD_32;
>> +
>> +    case DOMAIN_64BIT:
>> +        return XLEN_FIELD_64;
> Couldn't we add to avoid having to explain why there is no default in
> the comment above, and to respect MISRA rules?
> default:
>      break

I'd rather not: d->type is an enum, and for those Xen deliberately omits
the default label, as -Wall enables -Wswitch, which breaks the build
(-Werror) if one of the enum's values isn't handled. A default label
would silence exactly that check. MISRA-wise this is covered by the
R16.4 deviation for switches on enums.

I'll move the explanation into the switch() itself, though, and mention 
-Wswitch there, which should make the intent clearer:

...
     case DOMAIN_64BIT:
         return XLEN_FIELD_64;

     /*
      * No default case: -Wswitch then fails the build should a new
      * domain_type (a 128-bit one, in particular) be added without
      * updating this mapping.
      */
     }
...

>> +    }
>> +
>> +    ASSERT_UNREACHABLE();
>> +
>> +    /* A reserved encoding, which hstatus_vsxl_settable() never accepts. */
>> +    return 0;
> Could this get a name next to XLEN_FIELD_{32,64,128} in riscv_encoding.h,
> e.g.:
> 
> #define XLEN_FIELD_RESERVED     _UL(0)
> so the comment isn't the only thing explaining the 0?

Sure, I'll add XLEN_FIELD_RESERVED and return it there.

>> [ ... 88 lines skipped ... ]
>> +
>>   void __init init_csr_masks(void)
>>   {
>>       /*
>> @@ -91,6 +204,10 @@ static void vcpu_csr_init(struct vcpu *v)
>>   {
>>       v->arch.hedeleg = HEDELEG_DEFAULT & csr_masks.hedeleg;
>>   
>> +    /*
>> +     * hstatus.VSXL is left zero here: the domain's type, and hence the
>> +     * guest's XLEN, is only known once its kernel image has been probed.
>> +     */
> 
> "Probe" refers to two different things in this series: the hardware
> probe of the settable hstatus.VSXL values (init_hstatus_vsxl_settable_mask())
> and the guest kernel header parsing (kernel_probe()). This comment is
> about the latter, but it is easy to read it as the former, which already
> ran at boot. Could you name it explicitly and state the ordering that
> actually matters? I'd suggest:
> 
>      /*
>       * hstatus.VSXL is left zero here: vCPU0 is created before
>       * kernel_probe() reads the guest kernel header, so the domain's type,
>       * and hence the guest's XLEN, is not known yet.
>       */
> 
Your suggested rewording looks good to me. I'll take your wording, and 
name kernel_probe() in the commit message as well, which has the same 
ambiguity.

Thanks.

~ Oleksii