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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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.
2. It was more convenient to write just "G" instead of "ima_zicsr_zifencei".

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
~ Oleksii