Re: [PATCH for 4.21 v5] xen/riscv: identify specific ISA supported by cpu

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.02.2025 10:53, Oleksii Kurochko wrote:
> On 2/10/25 5:19 PM, Jan Beulich wrote:
>> On 07.02.2025 21:07, Oleksii Kurochko wrote:
>>> +/*
>>> + * The canonical order of ISA extension names in the ISA string is defined 
>>> in
>>> + * chapter 27 of the unprivileged specification.
>>> + *
>>> + * The specification uses vague wording, such as should, when it comes to
>>> + * ordering, so for our purposes the following rules apply:
>>> + *
>>> + * 1. All multi-letter extensions must be separated from other extensions 
>>> by an
>>> + *    underscore.
>>> + *
>>> + * 2. Additional standard extensions (starting with 'Z') must be sorted 
>>> after
>>> + *    single-letter extensions and before any higher-privileged extensions.
>>> + *
>>> + * 3. The first letter following the 'Z' conventionally indicates the most
>>> + *    closely related alphabetical extension category, IMAFDQLCBKJTPVH.
>>> + *    If multiple 'Z' extensions are named, they must be ordered first by
>>> + *    category, then alphabetically within a category.
>>> + *
>>> + * 4. Standard supervisor-level extensions (starting with 'S') must be 
>>> listed
>>> + *    after standard unprivileged extensions.  If multiple supervisor-level
>>> + *    extensions are listed, they must be ordered alphabetically.
>>> + *
>>> + * 5. Standard machine-level extensions (starting with 'Zxm') must be 
>>> listed
>>> + *    after any lower-privileged, standard extensions.  If multiple
>>> + *    machine-level extensions are listed, they must be ordered
>>> + *    alphabetically.
>>> + *
>>> + * 6. Non-standard extensions (starting with 'X') must be listed after all
>>> + *    standard extensions. If multiple non-standard extensions are listed, 
>>> they
>>> + *    must be ordered alphabetically.
>>> + *
>>> + * An example string following the order is:
>>> + *    rv64imadc_zifoo_zigoo_zafoo_sbar_scar_zxmbaz_xqux_xrux
>>> + *
>>> + * New entries to this struct should follow the ordering rules described 
>>> above.
>>> + *
>>> + * Extension name must be all lowercase (according to device-tree binding)
>>> + * and strncmp() is used in match_isa_ext() to compare extension names 
>>> instead
>>> + * of strncasecmp().
>>> + */
>>> +const struct riscv_isa_ext_data __initconst riscv_isa_ext[] = {
>>> +    RISCV_ISA_EXT_DATA(i, RISCV_ISA_EXT_i),
>>> +    RISCV_ISA_EXT_DATA(m, RISCV_ISA_EXT_m),
>>> +    RISCV_ISA_EXT_DATA(a, RISCV_ISA_EXT_a),
>>> +    RISCV_ISA_EXT_DATA(f, RISCV_ISA_EXT_f),
>>> +    RISCV_ISA_EXT_DATA(d, RISCV_ISA_EXT_d),
>>> +    RISCV_ISA_EXT_DATA(q, RISCV_ISA_EXT_q),
>>> +    RISCV_ISA_EXT_DATA(c, RISCV_ISA_EXT_c),
>>> +    RISCV_ISA_EXT_DATA(h, RISCV_ISA_EXT_h),
>>> +    RISCV_ISA_EXT_DATA(zicntr, RISCV_ISA_EXT_ZICNTR),
>>> +    RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR),
>>> +    RISCV_ISA_EXT_DATA(zifencei, RISCV_ISA_EXT_ZIFENCEI),
>>> +    RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
>>> +    RISCV_ISA_EXT_DATA(zihpm, RISCV_ISA_EXT_ZIHPM),
>>> +    RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
>>> +    RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
>>> +    RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
>>> +};
>>> +
>>> +static const struct riscv_isa_ext_data __initconst required_extensions[] = 
>>> {
>>> +    RISCV_ISA_EXT_DATA(i, RISCV_ISA_EXT_i),
>>> +    RISCV_ISA_EXT_DATA(m, RISCV_ISA_EXT_m),
>>> +    RISCV_ISA_EXT_DATA(a, RISCV_ISA_EXT_a),
>>> +    RISCV_ISA_EXT_DATA(f, RISCV_ISA_EXT_f),
>>> +    RISCV_ISA_EXT_DATA(d, RISCV_ISA_EXT_d),
>> Why would these last four (Zifencei below) not be included in #ifdef
>> CONFIG_RISCV_ISA_RV64G, just like ...
>>
>>> +#ifdef CONFIG_RISCV_ISA_C
>>> +    RISCV_ISA_EXT_DATA(c, RISCV_ISA_EXT_c),
>>> +#endif
>> .. this one is?
> 
> I'm not sure if it was the best decision, but I did it this way because
> I believe other bitnesses (32, 128) will also need G. So, I left them
> without|#ifdef| to avoid adding|#ifdef CONFIG_RV{32,128}G| in the future.

That's fine, but then imo RISCV_ISA_RV64G ought to be dropped, and we use
G unconditionally. Whether that's a good move I don't know. I could
imagine embedded use cases which want to run an very simple processors.

> I also spent some time considering whether 'f' and 'd' are necessary
> for Xen. In the end, I decided that if they aren't needed for Xen,
> it might be better not to use "G" for compilation and instead explicitly
> specify "ima". But it will be better to do as a separate patch (if it
> makes sense).

That's certainly an option; use of floating point registers / insns will
need suppressing one way or another anyway, sooner or later. And yes, I
agree this wants to be a separate change. Even their relative order is
not important, as long as things remain consistent at any point in time.

Jan