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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 5/20/26 8:13 AM, Jan Beulich wrote:
> On 19.05.2026 18:21, Oleksii Kurochko wrote:
> > On 5/19/26 5:56 PM, Jan Beulich wrote:
> > > On 19.05.2026 17:17, Oleksii Kurochko wrote:
> > > > On 5/19/26 4:53 PM, Jan Beulich wrote:
> > > > > On 19.05.2026 16:49, Oleksii Kurochko wrote:
> > > > > > 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);
> > > Ehem. Please check how ASSERT() works (and the difference to BUG_ON()).
> > Condition itself looks correct for ASSERT(). If build_guest_isa_str()
> > returns value equal to len then assert_failed() shouldn't be called.
> >
> > Maybe do you mean that it will never fire in release build then yes it
> > should be changed to BUG_ON() and the condition should be inverted:
> >    BUG_ON(build_guest_isa_str(d->arch.isa_str, len + 1, d->arch.isa) != len);
> No. Unlike in BUG_ON(), you can't use expressions with side effects (i.e.
> also function calls, unless they're const/pure) in ASSERT(). That's
> true for standard C's assert() as well, i.e. not Xen specific at all.
> (We do, however, diverge from assert() in another aspect.)
Got you.

Also it could be in release build that build_guest_isa_str() won't be 
called at all because of how ASSERT() is open-coded:  if ( 0 && (p) ...
Then probably it would be better to do in the following way:

if ( build_guest_isa_str(d->arch.isa_str, len + 1, d->arch.isa) != len )
{
    XVFREE(d->arch.isa_str);
    return -EINVAL;
}

return 0;

I am not sure that -EINVAL is the best one option but I don't see any 
better now.
~ Oleksii