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

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

On 7/9/26 3:05 PM, Jan Beulich wrote:
> 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.

I checked downstream changes and it also local to cpufeature.c file so 
it should be static.

> 
>> @@ -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.

pointer to const struct domain looks really better I chnage prototype of 
build_guest_isa_str().

> 
>> +    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?

At the moment, we don't have a use case where domain has unique bitmap. 
But IIRC correctly we agreed that generally it is good idea to have isa 
bitmap per domain and let the use to chose.

  Else
> why not calculate that bitmap once, taking the same shortcut as you take
> elsewhere for the time being?

static __ro_after_init DECLARE_BITMAP(guest_isa, RISCV_ISA_EXT_MAX);

/* called once from riscv_fill_hwcap() or similar */
void __init init_guest_isa_mask(void)
{
     for ( unsigned int i = 0; i < ARRAY_SIZE(riscv_isa_ext); i++ )
         if ( riscv_isa_ext[i].guest_supported &&
              riscv_isa_extension_available(NULL, riscv_isa_ext[i].id) )
             __set_bit(riscv_isa_ext[i].id, guest_isa);
}

void init_guest_isa(struct domain *d)   /* called per-domain */
{
     bitmap_copy(d->arch.isa, guest_isa, RISCV_ISA_EXT_MAX);
}

Do you mean something like that?

  Then allowing riscv_isa_ext[] to remain
> __initconst (should really have been __initconstrel).

I think you explained me already why but I forgot. Could you please 
remind me again why __initconstrel should be here? Is it because of 
pointers used inside struct riscv_isa_ext_entry?

Thanks.

~ Oleksii