Re: [PATCH v4 08/25] xen/riscv: introduce guest riscv,isa string

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.06.2026 18:06, Oleksii Kurochko wrote:
> On 6/29/26 4:46 PM, Jan Beulich wrote:
>> On 26.06.2026 17:46, Oleksii Kurochko wrote:
>>> +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);
>>> +}
>>
>> Wouldn't riscv_isa_ext[] better get a prominent reminder that additions there
>> may need mirroring here (unless guest support is implemented at the same 
>> time)?
>> (As before, yet better would of course be to make sure this is consistent
>> right from build time, i.e. without the need to have this separate function.
>> Or minimally have the info right in that array, so that while adding one 
>> needs
>> to think how to set that separate field.)
> 
> How about making the field mandatory at the call site instead, so it 
> can't be silently forgotten:
> 
> #define RISCV_ISA_EXT_DATA(ext_name, guest_supp)    \
> {                                                    \
>      .id = RISCV_ISA_EXT_ ## ext_name,                \
>      .name = #ext_name,                               \
>      .guest_supported = guest_supp,                   \
> }
> 
> Every entry in riscv_isa_ext[] would then need an explicit true/false 
> argument, e.g. RISCV_ISA_EXT_DATA(f, false). That forces whoever adds a 
> new extension to make the decision right there, rather than relying on a 
> separate init_guest_unsupp() to be remembered. We'd drop guest_unsupp 
> and init_guest_unsupp(), and build d->arch.isa directly from the array 
> in init_guest_isa().

This is what I meant with "Or minimally ...".

>>> @@ -94,6 +95,9 @@ struct arch_domain {
>>>       struct p2m_domain p2m;
>>>   
>>>       struct paging_domain paging;
>>> +
>>> +    DECLARE_BITMAP(isa, RISCV_ISA_EXT_MAX);
>>> +    char *isa_str;
>>>   };
>>
>> Why is it again that both the bitmap and its string representation need
>> storing? In the end they provide two different sources of truth, as there's
>> no guarantee that they'll remain in sync.
> 
> isa_str is needed to guest device tree to tell which extensions are 
> supported.

Sure, but does that need storing over the lifetime of the domain? Can't
that string be allocated, built, used, and then freed while DT is being
built? Would that string be used at all for toolstack-created domains?

Jan