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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 5/19/26 3:40 PM, Jan Beulich wrote:
> 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.
I will do than in the following way:

static int build_guest_isa_str(char *buf, size_t size,
                               const unsigned long *isa_bitmap)
{
    int total = 0;
    int ret;

#if defined(CONFIG_RISCV_32)
    ret = snprintf(buf, size, "rv32");
#elif defined(CONFIG_RISCV_64)
    ret = snprintf(buf, size, "rv64");
#else
#   error "Unsupported RISC-V bitness"
#endif
    if ( ret < 0 )
        return ret;
    total += ret;

    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;

        ret = snprintf(buf ? buf + total : NULL,
                       buf ? size - total : 0, "%s%s",
                       ext->id >= RISCV_ISA_EXT_BASE ? "_" : "",
                       ext->name);
        if ( ret < 0 )
            return ret;
        total += ret;
    }

    return total;
}

int init_guest_isa(struct domain *d)
{
    int len;

    bitmap_andnot(d->arch.isa, riscv_isa, guest_unsupp,
                  RISCV_ISA_EXT_MAX);

    len = build_guest_isa_str(NULL, 0, d->arch.isa);
    if ( len < 0 )
        return len;

    d->arch.isa_str = xmalloc_array(char, len + 1);
    if ( !d->arch.isa_str )
        return -ENOMEM;

    build_guest_isa_str(d->arch.isa_str, len + 1, d->arch.isa);

    return 0;
}

Thanks for suggestion.

~ Oleksii