Re: [PATCH v5 09/26] xen/riscv: introduce guest riscv,isa string

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.07.2026 17:57, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/cpufeature.c
> +++ b/xen/arch/riscv/cpufeature.c
> @@ -14,6 +14,7 @@
>  #include <xen/errno.h>
>  #include <xen/init.h>
>  #include <xen/lib.h>
> +#include <xen/sched.h>
>  #include <xen/sections.h>
>  
>  #include <asm/cpufeature.h>
> @@ -34,6 +35,19 @@ struct riscv_isa_ext_data {
>      .name = #ext_name,                          \
>  }
>  
> +struct riscv_isa_ext_entry {
> +    unsigned int id;
> +    const char *name;
> +    bool guest_supported;
> +};
> +
> +#define RISCV_ISA_EXT_ENTRY(ext_name, guest_supp)       \
> +{                                                       \
> +    .id              = RISCV_ISA_EXT_ ## ext_name,      \
> +    .name            = #ext_name,                       \
> +    .guest_supported = guest_supp,                      \
> +}
> +
>  /* Host ISA bitmap */
>  static __ro_after_init DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX);
>  
> @@ -120,29 +134,30 @@ static int __init dt_get_cpuid_from_node(const struct 
> dt_device_node *cpu,
>   * and strncmp() is used in match_isa_ext() to compare extension names 
> instead
>   * of strncasecmp().
>   */
> -const struct riscv_isa_ext_data __initconst riscv_isa_ext[] = {

I realize it has been this way before, but ...

> -    RISCV_ISA_EXT_DATA(i),
> -    RISCV_ISA_EXT_DATA(m),
> -    RISCV_ISA_EXT_DATA(a),
> -    RISCV_ISA_EXT_DATA(f),
> -    RISCV_ISA_EXT_DATA(d),
> -    RISCV_ISA_EXT_DATA(q),
> -    RISCV_ISA_EXT_DATA(c),
> -    RISCV_ISA_EXT_DATA(h),
> -    RISCV_ISA_EXT_DATA(zicntr),
> -    RISCV_ISA_EXT_DATA(zicsr),
> -    RISCV_ISA_EXT_DATA(zifencei),
> -    RISCV_ISA_EXT_DATA(zihintpause),
> -    RISCV_ISA_EXT_DATA(zihpm),
> -    RISCV_ISA_EXT_DATA(zba),
> -    RISCV_ISA_EXT_DATA(zbb),
> -    RISCV_ISA_EXT_DATA(zbs),
> -    RISCV_ISA_EXT_DATA(smaia),
> -    RISCV_ISA_EXT_DATA(smstateen),
> -    RISCV_ISA_EXT_DATA(ssaia),
> -    RISCV_ISA_EXT_DATA(sstc),
> -    RISCV_ISA_EXT_DATA(svade),
> -    RISCV_ISA_EXT_DATA(svpbmt),
> +const struct riscv_isa_ext_entry riscv_isa_ext[] = {

... is there a reason for this to be non-static? Its type (struct
riscv_isa_ext_entry) is local to this file, and I also can't spot any
declaration elsewhere.

> @@ -480,6 +495,74 @@ bool riscv_isa_extension_available(const unsigned long 
> *isa_bitmap,
>      return test_bit(id, isa_bitmap);
>  }
>  
> +int build_guest_isa_str(char *buf, size_t size,
> +                        const unsigned long *isa_bitmap)
> +{
> +    char *p = buf;
> +    size_t left = size;
> +    int total;
> +
> +#if defined(CONFIG_RISCV_32)
> +    total = snprintf(p, left, "rv32");
> +#elif defined(CONFIG_RISCV_64)
> +    total = snprintf(p, left, "rv64");
> +#else
> +# error "Unsupported RISC-V bitness"
> +#endif

For the longer-term future of this, passing in const struct domain * may
help.

> +    if ( total < 0 )
> +        return total;
> +
> +    if ( buf )
> +    {
> +        if ( (size_t)total >= left )
> +            return -ENOSPC;
> +
> +        p += total;
> +        left -= total;
> +    }
> +
> +    for ( unsigned int i = 0; i < ARRAY_SIZE(riscv_isa_ext); i++ )
> +    {
> +        const struct riscv_isa_ext_entry *ext = &riscv_isa_ext[i];
> +        int ret;
> +
> +        if ( !riscv_isa_extension_available(isa_bitmap, ext->id) )
> +            continue;
> +
> +        ret = snprintf(p, left, "%s%s",
> +                       ext->id >= RISCV_ISA_EXT_BASE ? "_" : "",
> +                       ext->name);
> +        if ( ret < 0 )
> +            return ret;
> +
> +        total += ret;
> +
> +        if ( buf )
> +        {
> +            if ( (size_t)ret >= left )
> +                return -ENOSPC;
> +
> +            p += ret;
> +            left -= ret;
> +        }
> +    }
> +
> +    return total;
> +}
> +
> +void init_guest_isa(struct domain *d)
> +{
> +    for ( unsigned int i = 0; i < ARRAY_SIZE(riscv_isa_ext); i++ )
> +    {
> +        const struct riscv_isa_ext_entry *ext = &riscv_isa_ext[i];
> +
> +        if ( ext->guest_supported &&
> +             riscv_isa_extension_available(NULL, ext->id) )
> +            __set_bit(ext->id, d->arch.isa);
> +    }
> +}

Right now what this function does is dependent on only global variables.
IOW each guest gets the same bitmap. Is this going to change soon? Else
why not calculate that bitmap once, taking the same shortcut as you take
elsewhere for the time being? Then allowing riscv_isa_ext[] to remain
__initconst (should really have been __initconstrel).

> @@ -527,4 +610,5 @@ void __init riscv_fill_hwcap(void)
>      if ( !all_extns_available )
>          panic("Look why the extensions above are needed in "
>                
> "https://xenbits.xenproject.org/docs/unstable/misc/riscv/booting.txt\n";);
> +
>  }

Stray change once again?

Jan