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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.01.2025 17:36, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/cpufeature.c
> @@ -0,0 +1,506 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Taken for Linux kernel v6.12-rc3 and modified by
> + * Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>:
> + *
> + * - Drop unconditional setting of {RISCV_ISA_EXT_ZICSR,
> + *   RISCV_ISA_EXT_ZIFENCEI, RISCV_ISA_EXT_ZICNTR, RISCV_ISA_EXT_ZIHPM} as 
> they
> + *   are now part of the riscv,isa string.
> + * - Remove saving of the ISA for each CPU, only the common available ISA is
> + *   saved.
> + * - Remove ACPI-related code as ACPI is not supported by Xen.
> + * - Drop handling of elf_hwcap, since Xen does not provide hwcap to
> + *   userspace.
> + * - Replace of_cpu_device_node_get() API, which is not available in Xen,
> + *   with a combination of dt_for_each_child_node(), 
> dt_device_type_is_equal(),
> + *   and dt_get_cpuid_from_node() to retrieve cpuid and riscv,isa in
> + *   riscv_fill_hwcap_from_isa_string().
> + * - Rename arguments of __RISCV_ISA_EXT_DATA() from _name to ext_name, and
> + *   _id to ext_id for clarity.
> + * - Replace instances of __RISCV_ISA_EXT_DATA with RISCV_ISA_EXT_DATA.
> + * - Replace instances of __riscv_isa_extension_available with
> + *   riscv_isa_extension_available for consistency. Also, update the type of
> + *   `bit` argument of riscv_isa_extension_available().
> + * - Redefine RISCV_ISA_EXT_DATA() to work only with ext_name and ext_id,
> + *   as other fields are not used in Xen currently.
> + * - Add check of first 4 letters of riscv,isa string to
> + *   riscv_isa_parse_string() as Xen doesn't do this check before so it is
> + *   necessary to check correctness of riscv,isa string. ( it should start 
> with
> + *   rv{32,64} with taking into account lower case of "rv").
> + * - Drop an argument of riscv_fill_hwcap() and 
> riscv_fill_hwcap_from_isa_string()
> + *   as it isn't used, at the moment.
> + * - Update the comment message about QEMU workaround.
> + * - s/pr_info/printk.

As said before - having this in the commit message is likely enough. Putting
it here as well is only risking for this to go stale (perhaps rather sooner
than later).

> + * 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, ext_id)    \
> +{                                               \
> +    .id = ext_id,                               \
> +    .name = #ext_name,                          \
> +}
> +
> +/* Host ISA bitmap */
> +static __ro_after_init DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX);
> +
> +static int __init dt_get_cpuid_from_node(const struct dt_device_node *cpu)
> +{
> +    const __be32 *prop;
> +    unsigned int reg_len;
> +
> +    if ( dt_n_size_cells(cpu) != 0 )
> +        printk("cpu node `%s`: #size-cells %d\n",
> +               dt_node_full_name(cpu), dt_n_size_cells(cpu));

The call to dt_n_size_cells() is here really just for the printk()?

> +    prop = dt_get_property(cpu, "reg", &reg_len);
> +    if ( !prop )
> +    {
> +        printk("cpu node `%s`: has no reg property\n", 
> dt_node_full_name(cpu));
> +        return -EINVAL;
> +    }
> +
> +    if ( reg_len < dt_cells_to_size(dt_n_addr_cells(cpu)) )
> +    {
> +        printk("cpu node `%s`: reg property too short\n",
> +               dt_node_full_name(cpu));
> +        return -EINVAL;
> +    }
> +
> +    return dt_read_paddr(prop, dt_n_addr_cells(cpu));

How come it is okay to (silently) truncate here from paddr_t to int?

> +}
> +
> +/*
> + * 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),
> +};
> +
> +static const struct riscv_isa_ext_data __initconst required_extensions[] = {
> +    RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
> +    RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
> +};

No Zicsr here?

> +static bool is_lowercase_extension_name(const char *str)
> +{
> +    if ( !str )
> +        return false;

This path doesn't look like it can actually be taken. Imo such checks
may make sense in non-static functions, but in static ones it is usually
clear enough that all callers pass in good pointers.

> +    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[]

Nit: There appears to be a missing closing parenthesis here. Plus in text
may I advise to omit blanks after the opening and before the closing
parenthesis? Imo this just makes it harder to read, even if only slightly.

> +         * elements must be all in lowercase.
> +         *
> +         * Just to be sure that it is true, ASSERT() are added.
> +         */
> +        ASSERT(is_lowercase_extension_name(name) &&
> +               is_lowercase_extension_name(ext->name));
> +
> +        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)
> +{
> +    /*
> +     * According to RISC-V device tree binding isa string must be all
> +     * lowercase.
> +     * To be sure that this is true, ASSERT below is added.
> +     */
> +    ASSERT(islower(isa[0]) && islower(isa[1]));

This looks a little odd to me when you have ...

> +    if ( (isa[0] != 'r') && (isa[1] != 'v') )
> +        return -EINVAL;

... this anyway.

> +#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
> +
> +    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."
> +                        "Use riscv,isa-extensions instead\n");

Interesting suggestion considering that doing so will end in a panic().

> +static int __init riscv_fill_hwcap_from_ext_list(void)
> +{
> +    const struct dt_device_node *cpus = dt_find_node_by_path("/cpus");
> +    const struct dt_device_node *cpu;
> +    int res = -EINVAL;
> +
> +    if ( !cpus )
> +    {
> +        printk("Missing /cpus node in the device tree?\n");
> +        return -EINVAL;
> +    }
> +
> +    dt_for_each_child_node(cpus, cpu)
> +    {
> +        const char *isa;
> +        int cpuid;
> +
> +        if ( !dt_device_type_is_equal(cpu, "cpu") )
> +            continue;
> +
> +        cpuid = dt_get_cpuid_from_node(cpu);
> +        if ( cpuid < 0 )
> +            continue;
> +
> +        if ( dt_property_read_string(cpu, "riscv,isa-extensions", &isa) )
> +        {
> +            printk("Unable to find \"riscv,isa-extensions\" devicetree entry 
> "
> +                   "for cpu%d\n", cpuid);

This is DT's number space for CPUs, isn't it? Any use of cpu%d (or CPU%d) or
alike generally suggests the number is Xen's. This may want disambiguating
here.

> +static void __init riscv_fill_hwcap_from_isa_string(void)
> +{
> +    const struct dt_device_node *cpus = dt_find_node_by_path("/cpus");
> +    const struct dt_device_node *cpu;
> +
> +    if ( !cpus )
> +    {
> +        printk("Missing /cpus node in the device tree?\n");
> +        return;
> +    }
> +
> +    dt_for_each_child_node(cpus, cpu)
> +    {
> +        DECLARE_BITMAP(this_isa, RISCV_ISA_EXT_MAX);
> +        const char *isa;
> +        int cpuid;
> +
> +        if ( !dt_device_type_is_equal(cpu, "cpu") )
> +            continue;
> +
> +        cpuid = dt_get_cpuid_from_node(cpu);
> +        if ( cpuid < 0 )
> +            continue;
> +
> +        if ( dt_property_read_string(cpu, "riscv,isa", &isa) )
> +        {
> +            printk("Unable to find \"riscv,isa\" devicetree entry\n");
> +            continue;
> +        }

Interestingly you don't log the CPU number here. What's the deal?

> +bool riscv_isa_extension_available(const unsigned long *isa_bitmap,
> +                                   enum riscv_isa_ext_id bit)
> +{
> +    const unsigned long *bmap = (isa_bitmap) ? isa_bitmap : riscv_isa;

Since it helps readability, may I suggest

    const unsigned long *bmap = isa_bitmap ?: riscv_isa;

or even getting away without the local var altogether:

    if ( !isa_bitmap )
        isa_bitmap = riscv_isa;

?

> +void __init riscv_fill_hwcap(void)
> +{
> +    unsigned int i;
> +    size_t req_extns_amount = ARRAY_SIZE(required_extensions);
> +    bool all_extns_available = true;
> +
> +    int ret = riscv_fill_hwcap_from_ext_list();

I don't think there's a reason here to have a blank line in the middle
of declarations.

> +    if ( ret )
> +    {
> +        printk("Falling back to deprecated \"riscv,isa\"\n");
> +        riscv_fill_hwcap_from_isa_string();
> +    }

I continue to find it irritating that you first try a function you
know won't succeed (and will panic() if the DT item is actually there),
in order to the log yet another message about using something that's
deprecated. If this is deprecated, why don't we prefer (and hence
support) the mor modern approach?

> +    for ( i = 0; i < req_extns_amount; i++ )
> +    {
> +        const struct riscv_isa_ext_data ext = required_extensions[i];
> +
> +        if ( !riscv_isa_extension_available(NULL, ext.id) )
> +        {
> +            printk("Xen requires extension: %s\n", ext.name);
> +            all_extns_available = false;
> +        }
> +    }
> +
> +    if ( !all_extns_available )
> +        panic("Look why the extensions above are needed in booting.txt\n");

Where's this booting.txt? I don't think people should be made hunt it
down ...

> --- /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,
> +    RISCV_ISA_EXT_ZICNTR = RISCV_ISA_EXT_BASE,
> +    RISCV_ISA_EXT_ZICSR,
> +    RISCV_ISA_EXT_ZIFENCEI,
> +    RISCV_ISA_EXT_ZIHINTPAUSE,
> +    RISCV_ISA_EXT_ZIHPM,
> +    RISCV_ISA_EXT_ZBB,
> +    RISCV_ISA_EXT_SMAIA,
> +    RISCV_ISA_EXT_SSAIA,
> +    RISCV_ISA_EXT_MAX
> +};
> +
> +void riscv_fill_hwcap(void);
> +
> +bool riscv_isa_extension_available(const unsigned long *isa_bitmap,
> +                                   enum riscv_isa_ext_id bit);

Nit: "bit" is an implementation detail. Imo "id" would be more natural
to use for people considering whether to call this function in a given
situation.

Jan