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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

> +    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.

> +static bool is_lowercase_extension_name(const char *str)

__init ?

> +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"
> +#endif
> +
> +    /*
> +     * In unpriv. specification (*_20240411) is mentioned the following:
> +     * (1) A RISC-V ISA is defined as a base integer ISA, which must be
> +     *     present in any implementation, plus optional extensions to
> +     *     the base ISA.
> +     * (2) Chapter 6 describes the RV32E and RV64E subset variants of
> +     *     the RV32I or RV64I base instruction sets respectively, which
> +     *     have been added to support small microcontrollers, and which
> +     *     have half the number of integer registers.
> +     *
> +     * What means that isa should contain, at least, I or E.
> +     *
> +     * As Xen isn't expected to be run on microcontrollers and according
> +     * to device tree binding the first extension should be "i".
> +     */
> +    if ( isa[4] != 'i' )
> +        return -EINVAL;
> +
> +    isa += 4;
> +
> +    while ( *isa )
> +    {
> +        const char *ext = isa++;
> +        const char *ext_end = isa;
> +
> +        switch ( *ext )
> +        {
> +        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 )
> +                if ( unlikely(!isalnum(*isa)) )
> +                    goto riscv_isa_parse_string_err;
> +
> +            ext_end = NULL;
> +            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_end = NULL;
> +                break;
> +            }
> +            fallthrough;
> +        case 'z':
> +            /*
> +             * Before attempting to parse the extension itself, 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.
> +             *
> +             * Next, as the extensions version is currently ignored, we
> +             * eliminate that portion. This is done by parsing backwards from
> +             * the end of the extension, removing any numbers. This may be a
> +             * major or minor number however, so the process is repeated if a
> +             * minor number was found.
> +             *
> +             * ext_end is intended to represent the first character *after* 
> the
> +             * name portion of an extension, but will be decremented to the 
> last
> +             * character itself while eliminating the extensions version 
> number.
> +             * A simple re-increment solves this problem.
> +             */
> +            for ( ; *isa && *isa != '_'; ++isa )
> +                if ( unlikely(!isalnum(*isa)) )
> +                    goto riscv_isa_parse_string_err;
> +
> +            ext_end = isa;
> +
> +            if ( !isdigit(ext_end[-1]) )
> +                break;
> +
> +            while ( isdigit(*--ext_end) )
> +                ;
> +
> +            if ( ext_end[0] != 'p' || !isdigit(ext_end[-1]) )
> +            {
> +                ++ext_end;
> +                break;
> +            }
> +
> +            while ( isdigit(*--ext_end) )
> +                ;
> +
> +            ++ext_end;
> +            break;
> +
> +        default:
> +            /*
> +             * Things are a little easier for single-letter extensions, as 
> they
> +             * are parsed forwards.
> +             *
> +             * After checking that our starting position is valid, we need to
> +             * ensure that, when isa was incremented at the start of the 
> loop,
> +             * that it arrived at the start of the next extension.
> +             *
> +             * If we are already on a non-digit, there is nothing to do. 
> Either
> +             * we have a multi-letter extension's _, or the start of an
> +             * extension.
> +             *
> +             * Otherwise we have found the current extension's major version
> +             * number. Parse past it, and a subsequent p/minor version number
> +             * if present. The `p` extension must not appear immediately 
> after
> +             * a number, so there is no fear of missing it.
> +             */
> +            if ( unlikely(!isalpha(*ext)) )
> +                goto riscv_isa_parse_string_err;
> +
> +            if ( !isdigit(*isa) )
> +                break;
> +
> +            while ( isdigit(*++isa) )
> +                ;
> +
> +            if ( *isa != 'p' )
> +                break;
> +
> +            if ( !isdigit(*++isa) )
> +            {
> +                --isa;
> +                break;
> +            }
> +
> +            while ( isdigit(*++isa) )
> +                ;
> +
> +            break;
> +        }
> +
> +        /*
> +         * The parser expects that at the start of an iteration isa points 
> to the
> +         * first character of the next extension. As we stop parsing an 
> extension
> +         * on meeting a non-alphanumeric character, an extra increment is 
> needed
> +         * where the succeeding extension is a multi-letter prefixed with an 
> "_".
> +         */
> +        if ( *isa == '_' )
> +            ++isa;
> +
> +        if ( unlikely(!ext_end) )
> +            continue;
> +
> +        match_isa_ext(ext, ext_end, out_bitmap);
> +    }
> +
> +    return 0;
> +
> +riscv_isa_parse_string_err:

Nit: Labels indented by at least one blank, please. See ./CODING_STYLE.

> +    printk("illegal symbol %c in riscv,isa string\n", *isa);

You may want to consider to include %c in quotes, such that even for e.g.
a blank it'll be clear to the observer of the log message what is meant.

> --- /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).

Jan