Re: [PATCH v1 05/27] xen/riscv: introduce guest riscv,isa string

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 4/1/26 3:49 PM, Jan Beulich wrote:
> On 10.03.2026 18:08, Oleksii Kurochko wrote:
> > Introduce generation of the riscv,isa string passed to the guest via the
> > Device Tree riscv,isa property.
> Title as well as this suggest this is all about guest properties. Then ...
>
> > The following changes are introduced:
> >
> > - According to the RISC-V privileged specification, M-mode software controls
> >    the XLEN value used by S-mode and U-mode. For RV64 harts, the SXL and UXL
> >    fields of the mstatus register are WARL fields that define the XLEN for
> >    S-mode and U-mode.
> >
> >    The XLEN value is provided by M-mode software (OpenSBI in the case of Xen)
> >    via the riscv,isa DT property. Introduce and initialize an xlen variable
> >    when parsing the host riscv,isa string in riscv_isa_parse_string().
> ... suddenly talk is of host aspects? (See below as to what "xlen" really
> is meant to hold.)
I just used it to show that xlen could be different and based on what 
prev. mode put into one of it register to encode xlen. And the same is 
applied for guest domains. I will rephrase it in more proper way.
> > --- a/xen/arch/riscv/cpufeature.c
> > +++ b/xen/arch/riscv/cpufeature.c
> > @@ -38,6 +38,8 @@ struct riscv_isa_ext_data {
> >   /* Host ISA bitmap */
> >   static __ro_after_init DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX);
> >   
> > +static __ro_after_init unsigned int xlen;
> Nit: Attribute between type and identifier please, whenever possible (it
> isn't neatly possible in riscv_isa above, due to DECLARE_BITMAP()).
>
> > @@ -160,6 +162,19 @@ static const struct riscv_isa_ext_data __initconst 
> > required_extensions[] = {
> >       RISCV_ISA_EXT_DATA(svpbmt),
> >   };
> >   
> > +static const unsigned int __initconst guest_unsupp_exts[] = {
> > +    RISCV_ISA_EXT_f,
> > +    RISCV_ISA_EXT_d,
> > +    RISCV_ISA_EXT_h,
> > +    RISCV_ISA_EXT_q,
> > +    RISCV_ISA_EXT_v,
> > +};
> This could do with a comment clarifying what needs (and what doesn't need)
> putting here. My expectation would have been that everything in
> riscv_isa_ext[] which shouldn't be exposed to guests should appear here.
> Yet then there is V (which riscv_isa_ext[] doesn't have), while e.g. Svade
> and Svpbmt (which iirc won't be available to guests right away) aren't
> there.
>
> > +static __ro_after_init DECLARE_BITMAP(guest_unsupp_bmp, RISCV_ISA_EXT_MAX);
> Is the _bmp suffix really needed? riscv_isa, for example, doesn't have it.
It makes sense to drop.

> > +#define MAX_GUEST_ISA_STR_LEN 256
> > +char guest_isa_str[MAX_GUEST_ISA_STR_LEN];
> __ro_after_init?
>
> Yet then - can this really be a global? Isn't the set of extensions
> available to a guest a per-guest property, i.e. a global could at best
> represent an upper bound on features?
At least, for now I think it is okay just to use global guest riscv,isa 
for all the domains. From my point of view if Xen doesn't support some 
extension to be used by guest so then should be responsible to generate 
proper riscv,isa.
It probably make sense to have riscv,isa per guest but then we still 
will want to parse this passed riscv,isa and check what Xen supports and 
what not, what looks like a double work a little bit. Probably it is the 
best one option and if riscv,isa isn't provided then just use that one 
generated by Xen.
> > @@ -193,6 +208,15 @@ static void __init match_isa_ext(const char *name, const 
> > char *name_end,
> >                !memcmp(name, ext->name, name_end - name) )
> >           {
> >               __set_bit(ext->id, bitmap);
> > +
> > +            if ( riscv_isa_extension_available(guest_unsupp_bmp, ext->id) )
> > +                break;
> > +
> > +            if ( ext->id >= RISCV_ISA_EXT_BASE )
> > +                safe_strcat(guest_isa_str, "_");
> > +
> > +            safe_strcat(guest_isa_str, ext->name);
> Shouldn't you check the (kind-of-)return value? (Yet better would be a build-
> time check, but I can't think of a way to achieve that.)
Yes, it would be nice. But it doesn't clear what should be reaction - 
just panic() with the message that we don't have enough space to cover 
all extenstion in riscv,isa or just continue with what was put to 
guest_isa_str. I think the second option could be fine.

> > @@ -207,13 +231,17 @@ static int __init riscv_isa_parse_string(const char *isa,
> >   #if defined(CONFIG_RISCV_32)
> >       if ( isa[2] != '3' && isa[3] != '2' )
> >           return -EINVAL;
> > +    xlen = 32;
> >   #elif defined(CONFIG_RISCV_64)
> >       if ( isa[2] != '6' && isa[3] != '4' )
> >           return -EINVAL;
> > +    xlen = 64;
> >   #else
> >   # error "unsupported RISC-V bitness"
> >   #endif
> This can be had with an initializer of "xlen". Then the (kind-of-)variable
> could be const unsigned int. Seeing the use below, is the variable
> correctly named, though? I.e. shouldn't it be guest_xlen or some such?
guest_xlen would be better.

> Independently I expect you will want to support 32-bit guests on 64-bit Xen
> at some point, in which case encoding this value into a global string won't
> work very well.
Yes, then it will be needed to move it to domain-specific structure.
I don't know if it makes sense to do now. (depends on what we will 
decide to do with per-domain riscv,isa or it is fine to go with global one)
Btw, it isn't always be possible to have 32-bit guest on 64-bit Xen as 
when HSXLEN=64, VSXL is a WARL field that is encoded the same as the MXL 
field of misa. In particular, an implementation may make VSXL be a 
read-only field whose value always ensures that VSXLEN=HSXLEN.
> > +    snprintf(guest_isa_str, sizeof(guest_isa_str), "rv%d", xlen);
> %u please with unsigned int.
>
> This being the only use of the variable (afaics), why is it not function-
> scope?
With the current implmentation, yes, it should be in function-scope.

> > @@ -487,6 +515,11 @@ void __init riscv_fill_hwcap(void)
> >       bool all_extns_available = true;
> >       struct trap_info trap;
> >   
> > +    for ( i = 0; i < ARRAY_SIZE(guest_unsupp_exts); i++ )
> > +    {
> > +        __set_bit(guest_unsupp_exts[i], guest_unsupp_bmp);
> > +    }
> Nit: No need for braces here. And anyway - can't this be had with an
> initializer for guest_unsupp_bmp?
It could be. I will use an initializer.

~ Oleksii