Re: [PATCH v3] xen/riscv: identify specific ISA supported by cpu

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 1/27/25 3:47 PM, Jan Beulich wrote:

> > +    *dt_cpuid = dt_read_paddr(prop, dt_n_addr_cells(cpu));
> > +
> > +    return 0;
> > +}
> > +
> > +/*
> > + * 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(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),
> > +};
> >       
> Just to clarify: There's no particular sorting intended for this table,
> while ...
>
> > +static const struct riscv_isa_ext_data __initconst required_extensions[] = {
> > +    RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
> > +    RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR),
> > +    RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
> > +};
> >       
> ... this one looks to mean to be alphabetically sorted?
>     
I will sync the sorting between the tables and will use the rule 3 mentioned above where it
is mentioned how they should be ordered:
  + * 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.

Thereby final version will be:
  +    RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR),
  +    RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
  +    RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
> > +static bool is_lowercase_extension_name(const char *str)
> > +{
> > +    /*
> > +     * `str` could contain full riscv,isa string from device tree so one
> > +     * of the stop condionitions is checking for '_' as extensions are
> > +     * separated by '_'.
> > +     */
> > +    for ( unsigned int i = 0; (str[i] != '\0') && (str[i] != '_'); i++ )
> > +        if ( !islower(str[i]) )
> > +            return false;
> > +
> > +    return true;
> > +}
> > +
> > +static void __init match_isa_ext(const char *name, const char *name_end,
> > +                                 unsigned long *bitmap)
> > +{
> > +    const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext);
> > +
> > +    for ( unsigned int i = 0; i < riscv_isa_ext_count; i++ )
> > +    {
> > +        const struct riscv_isa_ext_data *ext = &riscv_isa_ext[i];
> > +
> > +        /*
> > +         * `name` (according to device tree binding) and
> > +         * `ext->name` (according to initialization of riscv_isa_ext[]
> > +         * elements) must be all in lowercase.
> > +         *
> > +         * Just to be sure that it is true, ASSERT() is added.
> > +         */
> > +        ASSERT(is_lowercase_extension_name(name) &&
> > +               is_lowercase_extension_name(ext->name));
> >       
> More general remark: While asserting on ext->name is okay, for it being
> our own data, asserting on data coming from the outside is generally not
> correct. For now I'm not going to insist on this being changed, but
> sooner or later it will want revisiting
>     
IIUC it would be better to leave ASSERT(is_lowercase_extension_name(ext->name)) in match_isa_ext()
and put ASSERT(is_lowercase_extension_name(ext) in riscv_isa_parse_string() before match_isa_ext()
is called:
  static int __init riscv_isa_parse_string(const char *isa,
                                           unsigned long *out_bitmap)
  {
    ...
    while ( *isa )
    {
      const char *ext = isa++;
    ...
    ASSERT(is_lowercase_extension_name(ext));
    match_isa_ext(ext, ext_end, out_bitmap);
  }

Is my understanding correct?

> > +        if ( (name_end - name == strlen(ext->name)) &&
> > +             !strncmp(name, ext->name, name_end - name) )
> > +        {
> > +            __set_bit(ext->id, bitmap);
> > +            break;
> > +        }
> > +    }
> > +}
> > +
> > +static int __init riscv_isa_parse_string(const char *isa,
> > +                                         unsigned long *out_bitmap)
> > +{
> > +    if ( (isa[0] != 'r') && (isa[1] != 'v') )
> > +        return -EINVAL;
> > +
> > +#if defined(CONFIG_RISCV_32)
> > +    if ( isa[2] != '3' && isa[3] != '2' )
> > +        return -EINVAL;
> > +#elif defined(CONFIG_RISCV_64)
> > +    if ( isa[2] != '6' && isa[3] != '4' )
> > +        return -EINVAL;
> > +#else
> > +    #error "unsupported RISC-V bitness"
> >       
> Nit: We generally like to have the # in the first column, and - if
> so desired - blank padding afterwards.
>     
Should it be done only when "#if defined" used inside function or blank padding is needed only in
case when "#if defined" is used and, for example, for "#ifdef" such padding isn't needed?

> > +#endif
> > +
> > +    isa += 4;
> > +
> > +    while ( *isa )
> > +    {
> > +        const char *ext = isa++;
> > +        const char *ext_end = isa;
> > +        bool ext_err = false;
> > +
> > +        switch ( *ext )
> > +        {
> > +        case 'x':
> > +        case 'X':
> > +            printk_once("Vendor extensions are ignored in riscv,isa\n");
> > +            /*
> > +             * To skip an extension, we find its end.
> > +             * As multi-letter extensions must be split from other multi-letter
> > +             * extensions with an "_", the end of a multi-letter extension will
> > +             * either be the null character or the "_" at the start of the next
> > +             * multi-letter extension.
> > +             */
> > +            for ( ; *isa && *isa != '_'; ++isa )
> > +                ;
> > +            ext_err = true;
> > +            break;
> > +
> > +        case 's':
> > +            /*
> > +             * Workaround for invalid single-letter 's' & 'u' (QEMU):
> > +             *   Before QEMU 7.1 it was an issue with misa to ISA string
> > +             *   conversion:
> > +             *     https://patchwork.kernel.org/project/qemu-devel/patch/dee09d708405075420b29115c1e9e87910b8da55.1648270894.git.research_trasio@xxxxxxxxxxxx/#24792587
> > +             *   Additional details of the workaround on Linux kernel side:
> > +             *     https://lore.kernel.org/linux-riscv/ae93358e-e117-b43d-faad-772c529f846c@xxxxxxxxxxxx/#t
> > +             *
> > +             * No need to set the bit in riscv_isa as 's' & 'u' are
> > +             * not valid ISA extensions. It works unless the first
> > +             * multi-letter extension in the ISA string begins with
> > +             * "Su" and is not prefixed with an underscore.
> > +             */
> > +            if ( ext[-1] != '_' && ext[1] == 'u' )
> > +            {
> > +                ++isa;
> > +                ext_err = true;
> > +                break;
> > +            }
> > +            fallthrough;
> > +        case 'S':
> > +        case 'z':
> > +        case 'Z':
> >       
> With match_isa_ext() insisting on ISA strings being all lowercase, what's
> the point of permitting 'S' and 'Z' here?
>     
There is no need for this anymore; it was necessary before when the requirement didn't exist.
I will cleanup that.

Thanks.

~ Oleksii