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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.02.2025 17:50, Oleksii Kurochko wrote:
> On 2/11/25 4:47 PM, Jan Beulich wrote:
>> 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?
> 
> Several reasons come to my mind:
> 1. We still need to specify the architecture's bitness (rv32, rv64, etc.) 
> in|march|.

Of course.

> 2. It was more convenient to write just "G" instead of "ima_zicsr_zifencei".

Convenience can only matter when correctness isn't affected. Allowing
floating point registers to be used (when they're not aliases of the
integer ones) can end in crashes or even data corruption.

> Perhaps it would be better to have separate configs (similar to what Linux 
> uses):
> |1. ||riscv-march-$(CONFIG_ARCH_RV32I) := rv32ima 2. 
> riscv-march-$(CONFIG_ARCH_RV64I) := rv64ima 3. riscv-march-$(CONFIG_FPU) 
> := $(riscv-march-y)fd |
> 
> For now, we can skip option 3 until|CONFIG_FPU| is actually needed.
> 
> Introduce only options 1 and 2, probably reusing|CONFIG_RISCV_32| 
> and|CONFIG_RISCV_64|:
> 1.|riscv-march-$(CONFIG_RISCV_32) := rv32ima 
> 2.riscv-march-$(CONFIG_RISCV_64) := rv64ima |
> 
> Then, explicitly add|_zicsr| and|_zifencei|:
> |||riscv-march-y := $(riscv-march-y)_zicsr_zifencei|

Sounds reasonable, albeit the formatting of your email was somewhat screwed,
hampering readability (which may well be an effect of the MUI I'm using).

Jan