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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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.
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).
> > +    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(zbb, RISCV_ISA_EXT_ZBB),
> > +};
> >       
> Despite earlier answers to the same question, looking at just what the
> patch adds I still can't conclude how a system offering the B extension
> (under that name) will satisfy our requirement of Zbb. It's okay if you
> don't want to make adjustments to the code for now, but this at the very
> least wants clarifying in the patch description. Better would be by way
> of a code comment here.
>     
I think that it will be better then just do the following:
  --- a/xen/arch/riscv/cpufeature.c
  +++ b/xen/arch/riscv/cpufeature.c
  @@ -171,6 +171,14 @@ static void __init match_isa_ext(const char *name, const char *name_end,
   {
       const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext);
 
  +    if ( *name == 'b' )
  +    {
  +        __set_bit(RISCV_ISA_EXT_ZBA, bitmap);
  +        __set_bit(RISCV_ISA_EXT_ZBB, bitmap);
  +        __set_bit(RISCV_ISA_EXT_ZBS, bitmap);
  +        return;
  +    }
  +
       for ( unsigned int i = 0; i < riscv_isa_ext_count; i++ )

> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/cpufeature.h
> > @@ -0,0 +1,57 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef ASM__RISCV__CPUFEATURE_H
> > +#define ASM__RISCV__CPUFEATURE_H
> > +
> > +#ifndef __ASSEMBLY__
> > +
> > +#include <xen/stdbool.h>
> > +
> > +/*
> > + * These macros represent the logical IDs of each multi-letter RISC-V ISA
> > + * extension and are used in the ISA bitmap. The logical IDs start from
> > + * RISCV_ISA_EXT_BASE, which allows the 0-25 range to be reserved for single
> > + * letter extensions and are used in enum riscv_isa_ext_id.
> > + *
> > + * New extensions should just be added to the bottom, rather than added
> > + * alphabetically, in order to avoid unnecessary shuffling.
> > + */
> > +#define RISCV_ISA_EXT_BASE  26
> > +
> > +enum riscv_isa_ext_id {
> > +    RISCV_ISA_EXT_a,
> > +    RISCV_ISA_EXT_c,
> > +    RISCV_ISA_EXT_d,
> > +    RISCV_ISA_EXT_f,
> > +    RISCV_ISA_EXT_h,
> > +    RISCV_ISA_EXT_i,
> > +    RISCV_ISA_EXT_m,
> > +    RISCV_ISA_EXT_q,
> > +    RISCV_ISA_EXT_v,
> >       
> I'm sorry to be picky, but: If you use lower case letters here, why would
> ...
>
> > +    RISCV_ISA_EXT_ZICNTR = RISCV_ISA_EXT_BASE,
> >       
> ... e.g. this not be RISCV_ISA_EXT_Zicntr or RISCV_ISA_EXT_zicntr? In the
> latter case this could even be leveraged to simplify populating of the two
> arrays (RISCV_ISA_EXT_DATA() could then get away with just a single
> parameter).
>     
It makes sense. I will use the lower case and update the macro RISCV_ISA_EXT_DATA().

Thanks.

~ Oleksii