Re: [PATCH v4 08/25] xen/riscv: introduce guest riscv,isa string

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

On 6/29/26 4:46 PM, Jan Beulich wrote:
> On 26.06.2026 17:46, Oleksii Kurochko wrote:
>> Changes in v4:
>>   - Add an explicit overflow guard in build_guest_isa_str(): return
>>     -ENOSPC when buf is non-NULL and total >= size, to avoid the
>>     size - total underflow being passed to snprintf().
> 
> How does ...
> 
>> @@ -480,6 +489,81 @@ bool riscv_isa_extension_available(const unsigned long *isa_bitmap,
>>       return test_bit(id, isa_bitmap);
>>   }
>>   
>> +static int build_guest_isa_str(char *buf, size_t size,
>> +                               const unsigned long *isa_bitmap)
>> +{
>> +    int total;
>> +
>> +#if defined(CONFIG_RISCV_32)
>> +    total = snprintf(buf, size, "rv32");
>> +#elif defined(CONFIG_RISCV_64)
>> +    total = snprintf(buf, size, "rv64");
>> +#else
>> +#   error "Unsupported RISC-V bitness"
>> +#endif
>> +
>> +    if ( total < 0 )
>> +        return total;
>> +
>> +    if ( buf && ((size_t)total >= size) )
>> +        return -ENOSPC;
> 
> ... this help an underflow ...
> 
>> +    for ( unsigned int i = 0; i < ARRAY_SIZE(riscv_isa_ext); i++ )
>> +    {
>> +        const struct riscv_isa_ext_data *ext = &riscv_isa_ext[i];
>> +        int ret;
>> +
>> +        if ( !riscv_isa_extension_available(isa_bitmap, ext->id) )
>> +            continue;
>> +
>> +        ret = snprintf(buf ? buf + total : NULL,
>> +                       buf ? size - total : 0, "%s%s",
> 
> ... on any but the first iteration here?

Agree, overflow still could happen. I will update that part to:

char *p = buf;
size_t left = size;

...

            ret = snprintf(p, left, ...);

            if ( ret < 0 )
                return ret;

            total += ret;

            if ( buf )
            {
                if ( (size_t)ret >= left )
                    return -ENOSPC;

                p += ret;
                left -= ret;
            }

Then size - total never compute, so unsigned underflow simply cannot occur.

> 
>> +static void __init init_guest_unsupp(void)
>> +{
>> +    __set_bit(RISCV_ISA_EXT_f, guest_unsupp);
>> +    __set_bit(RISCV_ISA_EXT_d, guest_unsupp);
>> +    __set_bit(RISCV_ISA_EXT_q, guest_unsupp);
>> +    __set_bit(RISCV_ISA_EXT_v, guest_unsupp);
>> +    __set_bit(RISCV_ISA_EXT_h, guest_unsupp);
>> +    __set_bit(RISCV_ISA_EXT_sstc, guest_unsupp);
>> +    __set_bit(RISCV_ISA_EXT_svade, guest_unsupp);
>> +    __set_bit(RISCV_ISA_EXT_svpbmt, guest_unsupp);
>> +}
> 
> Wouldn't riscv_isa_ext[] better get a prominent reminder that additions there
> may need mirroring here (unless guest support is implemented at the same time)?
> (As before, yet better would of course be to make sure this is consistent
> right from build time, i.e. without the need to have this separate function.
> Or minimally have the info right in that array, so that while adding one needs
> to think how to set that separate field.)

How about making the field mandatory at the call site instead, so it 
can't be silently forgotten:

#define RISCV_ISA_EXT_DATA(ext_name, guest_supp)    \
{                                                    \
     .id = RISCV_ISA_EXT_ ## ext_name,                \
     .name = #ext_name,                               \
     .guest_supported = guest_supp,                   \
}

Every entry in riscv_isa_ext[] would then need an explicit true/false 
argument, e.g. RISCV_ISA_EXT_DATA(f, false). That forces whoever adds a 
new extension to make the decision right there, rather than relying on a 
separate init_guest_unsupp() to be remembered. We'd drop guest_unsupp 
and init_guest_unsupp(), and build d->arch.isa directly from the array 
in init_guest_isa().

Maybe it would be better to introduce separate structure which will 
embed .guest_supported + struct riscv_isa_ext_data.

> 
>> --- a/xen/arch/riscv/include/asm/cpufeature.h
>> +++ b/xen/arch/riscv/include/asm/cpufeature.h
>> @@ -17,6 +17,7 @@
>>    */
>>   #define RISCV_ISA_EXT_BASE  26
>>   
>> +
>>   enum riscv_isa_ext_id {
>>       RISCV_ISA_EXT_a,
>>       RISCV_ISA_EXT_c,
> 
> ???

I will drop unnecessary empty line.

> 
>> @@ -94,6 +95,9 @@ struct arch_domain {
>>       struct p2m_domain p2m;
>>   
>>       struct paging_domain paging;
>> +
>> +    DECLARE_BITMAP(isa, RISCV_ISA_EXT_MAX);
>> +    char *isa_str;
>>   };
> 
> Why is it again that both the bitmap and its string representation need
> storing? In the end they provide two different sources of truth, as there's
> no guarantee that they'll remain in sync.

isa_str is needed to guest device tree to tell which extensions are 
supported.

isa bitmap is needed to check in runtime if extension is available and 
it will be faster then search in a string. It could be a cases when code 
is generic enough and some things shouldn't be touched when some 
extension is available or not.

Considering the way how they are built they should be always in sync.

Thanks.

~ Oleksii