Re: [PATCH v2 05/26] xen/riscv: introduce guest riscv,isa string

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

On 5/18/26 5:51 PM, Jan Beulich wrote:
> On 08.05.2026 16:43, Oleksii Kurochko wrote:
>> @@ -480,6 +488,53 @@ bool riscv_isa_extension_available(const unsigned long *isa_bitmap,
>>       return test_bit(id, isa_bitmap);
>>   }
>>   
>> +int init_guest_isa(struct domain *d)
>> +{
>> +    char *buf = d->arch.guest_isa_str;
>> +    size_t len = sizeof(d->arch.guest_isa_str);
> 
> Seeing these uses: Is the "guest" prefix really of much use here?
> 
>> +    bitmap_andnot(d->arch.guest_isa, riscv_isa, guest_unsupp,
>> +                  RISCV_ISA_EXT_MAX);
> 
> Same question here, clearly.

I will drop "guest" prefix for "d->arch.guest_isa_str".


> 
>> +#if defined(CONFIG_RISCV_32)
>> +    if ( snprintf(buf, len, "rv32") >= len )
>> +        return -ENOBUFS;
>> +#elif defined(CONFIG_RISCV_64)
>> +    if ( snprintf(buf, len, "rv64") >= len )
>> +        return -ENOBUFS;
>> +#else
>> +#   error "Unsupported RISC-V bitness"
>> +#endif
>> +
>> +    for ( unsigned int i = 0; i < ARRAY_SIZE(riscv_isa_ext); i++ )
>> +    {
>> +        const struct riscv_isa_ext_data *ext = &riscv_isa_ext[i];
>> +
>> +        if ( !riscv_isa_extension_available(d->arch.guest_isa, ext->id) )
>> +            continue;
>> +
>> +        if ( ext->id >= RISCV_ISA_EXT_BASE && strlcat(buf, "_", len) >= len )
>> +            return -ENOBUFS;
>> +
>> +        if ( strlcat(buf, ext->name, len) >= len )
>> +            return -ENOBUFS;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +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);
>> +}
> 
> These don't need to be atomic, do they? I.e. __set_bit() would suffice.

Agree, __set_bit() would be enough.


> 
>> --- a/xen/arch/riscv/include/asm/cpufeature.h
>> +++ b/xen/arch/riscv/include/asm/cpufeature.h
>> @@ -17,6 +17,8 @@
>>    */
>>   #define RISCV_ISA_EXT_BASE  26
>>   
>> +#define RISCV_GUEST_ISA_STR_MAX 256
> 
> This looks like it won't be good for very long, seeing how long ISA strings can
> get. I wonder anyway whether ...
> 
>> @@ -94,6 +95,9 @@ struct arch_domain {
>>       struct p2m_domain p2m;
>>   
>>       struct paging_domain paging;
>> +
>> +    DECLARE_BITMAP(guest_isa, RISCV_ISA_EXT_MAX);
>> +    char guest_isa_str[RISCV_GUEST_ISA_STR_MAX];
> 
> ... a compile-time sized buffer is suitable here. Can't you allocate a buffer
> just large enough to hold the string?

It could be allocated dynamically.

Does it make sense to evaluate in run-time what should be a buffer size? 
For this case I can't find analogue of realloc() in Xen. Or it would be 
fine just to take something bigger as a const (lets say 2048) and use it 
for dynamic allocation?

Thanks.

~ Oleksii