Re: [PATCH v1 05/27] xen/riscv: introduce guest riscv,isa string

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.03.2026 18:08, Oleksii Kurochko wrote:
> Introduce generation of the riscv,isa string passed to the guest via the
> Device Tree riscv,isa property.

Title as well as this suggest this is all about guest properties. Then ...

> The following changes are introduced:
> 
> - According to the RISC-V privileged specification, M-mode software controls
>   the XLEN value used by S-mode and U-mode. For RV64 harts, the SXL and UXL
>   fields of the mstatus register are WARL fields that define the XLEN for
>   S-mode and U-mode.
> 
>   The XLEN value is provided by M-mode software (OpenSBI in the case of Xen)
>   via the riscv,isa DT property. Introduce and initialize an xlen variable
>   when parsing the host riscv,isa string in riscv_isa_parse_string().

... suddenly talk is of host aspects? (See below as to what "xlen" really
is meant to hold.)

> --- a/xen/arch/riscv/cpufeature.c
> +++ b/xen/arch/riscv/cpufeature.c
> @@ -38,6 +38,8 @@ struct riscv_isa_ext_data {
>  /* Host ISA bitmap */
>  static __ro_after_init DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX);
>  
> +static __ro_after_init unsigned int xlen;

Nit: Attribute between type and identifier please, whenever possible (it
isn't neatly possible in riscv_isa above, due to DECLARE_BITMAP()).

> @@ -160,6 +162,19 @@ static const struct riscv_isa_ext_data __initconst 
> required_extensions[] = {
>      RISCV_ISA_EXT_DATA(svpbmt),
>  };
>  
> +static const unsigned int __initconst guest_unsupp_exts[] = {
> +    RISCV_ISA_EXT_f,
> +    RISCV_ISA_EXT_d,
> +    RISCV_ISA_EXT_h,
> +    RISCV_ISA_EXT_q,
> +    RISCV_ISA_EXT_v,
> +};

This could do with a comment clarifying what needs (and what doesn't need)
putting here. My expectation would have been that everything in
riscv_isa_ext[] which shouldn't be exposed to guests should appear here.
Yet then there is V (which riscv_isa_ext[] doesn't have), while e.g. Svade
and Svpbmt (which iirc won't be available to guests right away) aren't
there.

> +static __ro_after_init DECLARE_BITMAP(guest_unsupp_bmp, RISCV_ISA_EXT_MAX);

Is the _bmp suffix really needed? riscv_isa, for example, doesn't have it.

> +#define MAX_GUEST_ISA_STR_LEN 256
> +char guest_isa_str[MAX_GUEST_ISA_STR_LEN];

__ro_after_init?

Yet then - can this really be a global? Isn't the set of extensions
available to a guest a per-guest property, i.e. a global could at best
represent an upper bound on features?

> @@ -193,6 +208,15 @@ static void __init match_isa_ext(const char *name, const 
> char *name_end,
>               !memcmp(name, ext->name, name_end - name) )
>          {
>              __set_bit(ext->id, bitmap);
> +
> +            if ( riscv_isa_extension_available(guest_unsupp_bmp, ext->id) )
> +                break;
> +
> +            if ( ext->id >= RISCV_ISA_EXT_BASE )
> +                safe_strcat(guest_isa_str, "_");
> +
> +            safe_strcat(guest_isa_str, ext->name);

Shouldn't you check the (kind-of-)return value? (Yet better would be a build-
time check, but I can't think of a way to achieve that.)

> @@ -207,13 +231,17 @@ static int __init riscv_isa_parse_string(const char 
> *isa,
>  #if defined(CONFIG_RISCV_32)
>      if ( isa[2] != '3' && isa[3] != '2' )
>          return -EINVAL;
> +    xlen = 32;
>  #elif defined(CONFIG_RISCV_64)
>      if ( isa[2] != '6' && isa[3] != '4' )
>          return -EINVAL;
> +    xlen = 64;
>  #else
>  # error "unsupported RISC-V bitness"
>  #endif

This can be had with an initializer of "xlen". Then the (kind-of-)variable
could be const unsigned int. Seeing the use below, is the variable
correctly named, though? I.e. shouldn't it be guest_xlen or some such?

Independently I expect you will want to support 32-bit guests on 64-bit Xen
at some point, in which case encoding this value into a global string won't
work very well.

> +    snprintf(guest_isa_str, sizeof(guest_isa_str), "rv%d", xlen);

%u please with unsigned int.

This being the only use of the variable (afaics), why is it not function-
scope?

> @@ -487,6 +515,11 @@ void __init riscv_fill_hwcap(void)
>      bool all_extns_available = true;
>      struct trap_info trap;
>  
> +    for ( i = 0; i < ARRAY_SIZE(guest_unsupp_exts); i++ )
> +    {
> +        __set_bit(guest_unsupp_exts[i], guest_unsupp_bmp);
> +    }

Nit: No need for braces here. And anyway - can't this be had with an
initializer for guest_unsupp_bmp?

Jan