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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.02.2025 17:50, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/cpufeature.c
> @@ -0,0 +1,502 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Originally taken for Linux kernel v6.12-rc3.
> + *
> + * Copyright (C) 2015 ARM Ltd.
> + * Copyright (C) 2017 SiFive
> + * Copyright (C) 2024 Vates
> + */
> +
> +#include <xen/bitmap.h>
> +#include <xen/ctype.h>
> +#include <xen/device_tree.h>
> +#include <xen/errno.h>
> +#include <xen/init.h>
> +#include <xen/lib.h>
> +#include <xen/sections.h>
> +
> +#include <asm/cpufeature.h>
> +
> +#ifdef CONFIG_ACPI
> +# error "cpufeature.c functions should be updated to support ACPI"
> +#endif
> +
> +struct riscv_isa_ext_data {
> +    unsigned int id;
> +    const char *name;
> +};
> +
> +#define RISCV_ISA_EXT_DATA(ext_name)            \
> +{                                               \
> +    .id = RISCV_ISA_EXT_##ext_name,             \

Nit: ## being a binary operator (just for the pre-processor) we prefer
it, too, to be framed by blanks.

> +/*
> + * 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_DATA(m),
> +    RISCV_ISA_EXT_DATA(a),
> +    RISCV_ISA_EXT_DATA(f),
> +    RISCV_ISA_EXT_DATA(d),
> +    RISCV_ISA_EXT_DATA(q),
> +    RISCV_ISA_EXT_DATA(c),
> +    RISCV_ISA_EXT_DATA(h),
> +    RISCV_ISA_EXT_DATA(zicntr),
> +    RISCV_ISA_EXT_DATA(zicsr),
> +    RISCV_ISA_EXT_DATA(zifencei),
> +    RISCV_ISA_EXT_DATA(zihintpause),
> +    RISCV_ISA_EXT_DATA(zihpm),
> +    RISCV_ISA_EXT_DATA(zbb),

No Zba and Zbs here, despite there now being enumerators for them?

> +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;
> +
> +        /*
> +         * if someone mentioned `b` extension in riscv,isa instead of 
> Zb{a,b,s}
> +         * explicitly then set bits exlicitly in out_bitmap to satisfy
> +         * requirement of Zbb (mentioned in required_extensions[]).
> +         */

Nit (style): Comments want to start with a captial letter.

With the two nits addressed and the Zba/Zbs question sorted (all
adjustments could be done while committing, albeit the disposition of
patch 1 isn't clear yet, so a v7 may be needed anyway):
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan