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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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