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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.05.2026 15:24, Oleksii Kurochko wrote:
> 
> 
> On 5/19/26 2:12 PM, Jan Beulich wrote:
>> On 19.05.2026 13:59, Oleksii Kurochko wrote:
>>> On 5/18/26 5:51 PM, Jan Beulich wrote:
>>>> On 08.05.2026 16:43, Oleksii Kurochko wrote:
>>>>> --- 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.
>>
>> Hmm, I see xrealloc_array(), and surely we could gain xvrealloc_array()
>> which we'll need anyway once xrealloc_array() uses get converted. (I also
>> see x{,v}realloc_flex_struct(), but that's of no use here as it looks.)
> 
> Oh, I missed to turned off "full match" during search...
> 
>>
>>> Or it would be
>>> fine just to take something bigger as a const (lets say 2048) and use it
>>> for dynamic allocation?
>>
>> I'd rather not. Can't you determine how much space the string is going to
>> occupy?
> 
> I thought about two options as alternatives:
> 
> 1. Take as a length host RISC-V ISA string but theoretically we can 
> emulate some extensions which aren't mentioned in host RISC-V ISA string 
> so it could be longer. So not a good option.
> 
> 2. Having two walks in init_guest_isa():
>     Introduce the following function:
> static size_t guest_isa_str_len(const unsigned long *isa_bitmap)
> {
>      size_t len = 4; /* rvX prefix */
> 
>      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(isa_bitmap, ext->id) )
>              continue;
> 
>          if ( ext->id >= RISCV_ISA_EXT_BASE )
>              len++; /* '_' separator */
> 
>          len += strlen(ext->name);
>      }
> 
>      return len + 1; /* NUL terminator */
> }
> 
>    and then:
> 
> int init_guest_isa(struct domain *d)
> {
>      bitmap_andnot(d->arch.guest_isa, riscv_isa, guest_unsupp,
>                    RISCV_ISA_EXT_MAX);
> 
>      size_t len = guest_isa_str_len(d->arch.guest_isa);
>      d->arch.guest_isa_str = xzalloc_array(char, len);
>      if ( !d->arch.guest_isa_str )
>          return -ENOMEM;
> 
>      /* ... existing snprintf + strlcat loop unchanged ... */
> }
> 
> If approach 2 is a good one I can follow it.

This might be yet better with only a single function. Otherwise the two are
always at risk of going out of sync. After all you can use snprintf() to
determine just the size needed; if you go look, there may even be an
example or two in the tree.

Jan