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

[Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>]

> [ ... 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
> +    }
> +
> +    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?
> [ ... 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.
     */

-- 
Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>