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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 5/19/26 4:53 PM, Jan Beulich wrote:
> On 19.05.2026 16:49, Oleksii Kurochko wrote:
> > On 5/19/26 3:40 PM, Jan Beulich wrote:
> > > On 19.05.2026 15:24, Oleksii Kurochko wrote:
> > > > 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;
> You can use total here right away, and limit ...
>
> >       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];
> ... ret's scope to this loop. This then also justifies total to be of a
> signed type.
It makes sense.

> >           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);
> At least ASSERT() the success of this?
I will add:

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

but I expect that we won't be here if len is incorrect or 
d->arch.isa_str wasn't allocated properly.
Thanks.

~ Oleksii