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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.02.2025 16:28, Oleksii Kurochko wrote:
> 
> On 2/11/25 11:01 AM, Jan Beulich wrote:
>> 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.
> 
> Actually, I think that we should drop 'f' and 'd' from required_extensions[]
> array as they aren't really needed for Xen. Or make them conditional just to
> be sure that if "G" was used for compilation and the code with using of them
> was generated then they are really supported by a h/w.

As said, that's okay. But as also said you then need to also keep the
compiler from potentially using F or D insns / registers.

> Regarding #ifdef-ing with RISCV_ISA_RV64G, I think that we have to keep them
> mentioned unconditionally as 'i', 'm', 'a', 'zicsr' and 'zifencei' which are
> part of 'G' as all of them are needed by Xen to work.

Yet then why do we have CONFIG_RISCV_ISA_RV64G?

Jan