[PATCH for 4.21 v1 1/1] xen/riscv: identify specific ISA supported by cpu

[PATCH for 4.21 v1 1/1] xen/riscv: identify specific ISA supported by cpu

[Oleksii Kurochko]

Supported ISA extensions are specified in the device tree within the CPU
node, using two properties: `riscv,isa-extensions` and `riscv,isa`.

Currently, Xen does not support the `riscv,isa-extensions` property, as
all available device tree source (DTS) files in the Linux kernel (v6.12-rc3)
and DTBs generated by QEMU use only the `riscv,isa` property.
Therefore, only `riscv,isa` parsing is implemented.

The `riscv,isa` property is parsed for each CPU, and the common extensions
are stored in the `host_riscv_isa` bitmap.
This bitmap is then used by `riscv_isa_extension_available()` to check
if a specific extension is supported.

The current implementation is based on Linux kernel v6.12-rc3
implementation with the following changes:
 - 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.
 - 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 upper and 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.
 - Apply Xen coding style.
 - s/pr_info/printk.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/Makefile                 |   1 +
 xen/arch/riscv/cpufeature.c             | 466 ++++++++++++++++++++++++
 xen/arch/riscv/include/asm/cpufeature.h |  63 ++++
 xen/arch/riscv/setup.c                  |   3 +
 4 files changed, 533 insertions(+)
 create mode 100644 xen/arch/riscv/cpufeature.c
 create mode 100644 xen/arch/riscv/include/asm/cpufeature.h

diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
index a5eb2aed4b..b0c8270a99 100644
--- a/xen/arch/riscv/Makefile
+++ b/xen/arch/riscv/Makefile
@@ -1,3 +1,4 @@
+obj-y += cpufeature.o
 obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
 obj-y += entry.o
 obj-y += mm.o
diff --git a/xen/arch/riscv/cpufeature.c b/xen/arch/riscv/cpufeature.c
new file mode 100644
index 0000000000..52ec05bd03
--- /dev/null
+++ b/xen/arch/riscv/cpufeature.c
@@ -0,0 +1,466 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Taken for Linux kernel v6.12-rc3 and modified by
+ * Oleksii Kurochko <oleksii.kurochko@gmail.com>:
+ *
+ * - 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.
+ * - 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 upper and 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.
+ * - s/pr_info/printk.
+ * - Apply Xen coding style.
+ *
+ * Copyright (C) 2015 ARM Ltd.
+ * Copyright (C) 2017 SiFive
+ * Copyright (C) 2024 Vates
+ */
+
+#include <xen/acpi.h>
+#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>
+
+struct riscv_isa_ext_data {
+    const 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 __read_mostly 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));
+
+    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));
+}
+
+/*
+ * The canonical order of ISA extension names in the ISA string is defined in
+ * chapter 27 of the unprivileged specification.
+ *
+ * Ordinarily, for in-kernel data structures, this order is unimportant but
+ * isa_ext_arr defines the order of the ISA string in /proc/cpuinfo.
+ *
+ * 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.
+ *
+ * 3. 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.
+ *
+ * 4. 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.
+ *
+ * 5. 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.
+ */
+const struct riscv_isa_ext_data 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 required_extensions[] = {
+    RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
+    RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
+};
+
+static const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext);
+
+static void __init match_isa_ext(const char *name, const char *name_end, unsigned long *bitmap)
+{
+    for ( int i = 0; i < riscv_isa_ext_count; i++ )
+    {
+        const struct riscv_isa_ext_data *ext = &riscv_isa_ext[i];
+
+        if ( (name_end - name == strlen(ext->name)) &&
+             !strncasecmp(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[0] != 'R' )
+        return -EINVAL;
+
+    if ( isa[1] != 'v' && isa[1] != 'V' )
+        return -EINVAL;
+
+    if ( isa[2] != '3' && isa[3] != '2' &&
+         isa[2] != '6' && isa[3] != '4' )
+        return -EINVAL;
+
+    isa += 4;
+
+    while ( *isa )
+    {
+        const char *ext = isa++;
+        const char *ext_end = isa;
+        bool ext_err = false;
+
+        switch ( *ext )
+        {
+        case 'x':
+        case 'X':
+            if ( acpi_disabled )
+                printk_once("Vendor extensions are ignored in riscv,isa."
+                            "Use riscv,isa-extensions instead\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).
+             * 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':
+            /*
+             * 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)) )
+                    ext_err = true;
+
+            ext_end = isa;
+            if ( unlikely(ext_err) )
+                break;
+
+            if ( !isdigit(ext_end[-1]) )
+                break;
+
+            while ( isdigit(*--ext_end) )
+                ;
+
+            if ( tolower(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)) )
+            {
+                ext_err = true;
+                break;
+            }
+
+            if ( !isdigit(*isa) )
+                break;
+
+            while ( isdigit(*++isa) )
+                ;
+
+            if ( tolower(*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_err) )
+            continue;
+
+        match_isa_ext(ext, ext_end, out_bitmap);
+    }
+
+    return 0;
+}
+
+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;
+
+    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 = 0;
+
+        if ( !dt_device_type_is_equal(cpu, "cpu") )
+            continue;
+
+        cpuid = dt_get_cpuid_from_node(cpu);
+        if ( cpuid < 0 )
+            continue;
+
+        if ( cpuid >= NR_CPUS )
+        {
+            printk_once("%s: dts has more cpu than NR_CPUs\n", __func__);
+            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);
+            continue;
+        }
+        else
+            printk("riscv,isa-extensions isnt supported\n");
+    }
+
+    return -EOPNOTSUPP;
+}
+
+static void __init riscv_fill_hwcap_from_isa_string(void)
+{
+    const char *isa;
+    const struct dt_device_node *cpus = dt_find_node_by_path("/cpus");
+    const struct dt_device_node *cpu;
+    int cpuid = 0;
+
+    if ( !acpi_disabled )
+        panic("%s should be updated correspondingly to support ACPI\n", __func__);
+
+    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);
+
+        if ( !dt_device_type_is_equal(cpu, "cpu") )
+            continue;
+
+        cpuid = dt_get_cpuid_from_node(cpu);
+        if ( cpuid < 0 )
+            continue;
+
+        if ( cpuid >= NR_CPUS )
+        {
+            printk_once("%s: dts has more cpu than NR_CPUs\n", __func__);
+            continue;
+        }
+
+        if ( acpi_disabled )
+        {
+            if ( dt_property_read_string(cpu, "riscv,isa", &isa) )
+            {
+                printk("Unable to find \"riscv,isa\" devicetree entry\n");
+                continue;
+            }
+        } else
+            panic("there is no support for ACPI\n");
+
+        riscv_isa_parse_string(isa, this_isa);
+
+        if ( bitmap_empty(riscv_isa, RISCV_ISA_EXT_MAX) )
+            bitmap_copy(riscv_isa, this_isa, RISCV_ISA_EXT_MAX);
+        else
+            bitmap_and(riscv_isa, riscv_isa, this_isa, RISCV_ISA_EXT_MAX);
+    }
+}
+
+bool riscv_isa_extension_available(const unsigned long *isa_bitmap, int bit)
+{
+    const unsigned long *bmap = (isa_bitmap) ? isa_bitmap : riscv_isa;
+
+    if (bit >= RISCV_ISA_EXT_MAX)
+        return false;
+
+    return test_bit(bit, bmap) ? true : false;
+}
+
+void __init riscv_fill_hwcap(void)
+{
+    unsigned int i;
+    size_t req_extns_amount = ARRAY_SIZE(required_extensions);
+    bool all_extns_available = true;
+
+    if ( !acpi_disabled )
+        riscv_fill_hwcap_from_isa_string();
+    else {
+        int ret = riscv_fill_hwcap_from_ext_list();
+
+        if ( ret )
+        {
+            printk("Falling back to deprecated \"riscv,isa\"\n");
+            riscv_fill_hwcap_from_isa_string();
+        }
+    }
+
+    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 extenstion: %s\n", ext.name);
+            all_extns_available = false;
+        }
+    }
+
+    if ( !all_extns_available )
+        panic("Look why the extenstions above are needed in booting.txt\n");
+}
diff --git a/xen/arch/riscv/include/asm/cpufeature.h b/xen/arch/riscv/include/asm/cpufeature.h
new file mode 100644
index 0000000000..9b2ed1e914
--- /dev/null
+++ b/xen/arch/riscv/include/asm/cpufeature.h
@@ -0,0 +1,63 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef ASM__RISCV__CPUFEATURE_H
+#define ASM__RISCV__CPUFEATURE_H
+
+#ifndef __ASSEMBLY__
+
+#define RISCV_ISA_EXT_a     ('a' - 'a')
+#define RISCV_ISA_EXT_c     ('c' - 'a')
+#define RISCV_ISA_EXT_d     ('d' - 'a')
+#define RISCV_ISA_EXT_f     ('f' - 'a')
+#define RISCV_ISA_EXT_h     ('h' - 'a')
+#define RISCV_ISA_EXT_i     ('i' - 'a')
+#define RISCV_ISA_EXT_m     ('m' - 'a')
+#define RISCV_ISA_EXT_q     ('q' - 'a')
+#define RISCV_ISA_EXT_v     ('v' - 'a')
+
+/*
+ * Increse this to higher value as kernel support more ISA extensions.
+ */
+#define RISCV_ISA_EXT_MAX   128
+
+#define RISCV_ISA_EXT_SxAIA     RISCV_ISA_EXT_SSAIA
+
+/*
+ * 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. The maximum, RISCV_ISA_EXT_MAX, is defined in order
+ * to allocate the bitmap and may be increased when necessary.
+ *
+ * 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_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_ID_MAX = RISCV_ISA_EXT_MAX,
+};
+
+void riscv_fill_hwcap(void);
+
+bool riscv_isa_extension_available(const unsigned long *isa_bitmap, int bit);
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* ASM__RISCV__CPUFEATURE_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 38ca4f3baa..380461a054 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -13,6 +13,7 @@
 
 #include <public/version.h>
 
+#include <asm/cpufeature.h>
 #include <asm/early_printk.h>
 #include <asm/fixmap.h>
 #include <asm/sbi.h>
@@ -121,6 +122,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
         panic("Booting using ACPI isn't supported\n");
     }
 
+    riscv_fill_hwcap();
+
     printk("All set up\n");
 
     machine_halt();
-- 
2.47.1

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

[Jan Beulich]

On 23.12.2024 13:55, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/cpufeature.c
> @@ -0,0 +1,466 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Taken for Linux kernel v6.12-rc3 and modified by
> + * Oleksii Kurochko <oleksii.kurochko@gmail.com>:
> + *
> + * - 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.
> + * - 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 upper and 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.
> + * - s/pr_info/printk.
> + * - Apply Xen coding style.

Having this in the patch description is sufficient imo.

> + * Copyright (C) 2015 ARM Ltd.
> + * Copyright (C) 2017 SiFive
> + * Copyright (C) 2024 Vates
> + */
> +
> +#include <xen/acpi.h>

Didn't you say you dropped the ACPI pieces?

> +#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>
> +
> +struct riscv_isa_ext_data {
> +    const unsigned int id;
> +    const char *name;
> +};

This is odd - why would the id be const, but not the name? Thus you
require all instances of the struct to have an initializer. The more
conventional approach is to apply the const on the instances of the
structure (e.g. as you already have it for riscv_isa_ext[]). 

> +#define RISCV_ISA_EXT_DATA(ext_name, ext_id)    \
> +{                                               \
> +    .id = ext_id,                               \
> +    .name = #ext_name,                          \
> +}
> +
> +/* Host ISA bitmap */
> +static __read_mostly DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX);

Not __ro_after_init?

> +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));
> +
> +    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));
> +}
> +
> +/*
> + * The canonical order of ISA extension names in the ISA string is defined in
> + * chapter 27 of the unprivileged specification.
> + *
> + * Ordinarily, for in-kernel data structures, this order is unimportant but
> + * isa_ext_arr defines the order of the ISA string in /proc/cpuinfo.

Inapplicable Linux detail? (If you want to keep it, you'll want to add
"Linux'es" and avoid mentioning something that looks like a variable
but then doesn't exist anywhere.)

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

Two times "3."?

> + * 4. 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.
> + *
> + * 5. 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.
> + */
> +const struct riscv_isa_ext_data riscv_isa_ext[] = {

__initconst?

> +    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),

Isn't it kind of implied that with the presence of Zbb, B should also be
present?

> +    RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
> +    RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
> +};

Wouldn't the Z and S prefixes better be recorded in upper case here?

> +static const struct riscv_isa_ext_data required_extensions[] = {

__initconst again?

> +    RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
> +    RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
> +};
> +
> +static const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext);

Is this variable really useful to have?

> +static void __init match_isa_ext(const char *name, const char *name_end, unsigned long *bitmap)

Overly long line.

> +{
> +    for ( int i = 0; i < riscv_isa_ext_count; i++ )

unsigned int

> +    {
> +        const struct riscv_isa_ext_data *ext = &riscv_isa_ext[i];
> +
> +        if ( (name_end - name == strlen(ext->name)) &&
> +             !strncasecmp(name, ext->name, name_end - name) )

Does this really need to be a case-insensitive comparison?

> +        {
> +            set_bit(ext->id, bitmap);

No need for atomicity, I suppose (i.e. __set_bit()).

> +            break;
> +        }
> +    }
> +}
> +
> +static int __init riscv_isa_parse_string(const char *isa, unsigned long *out_bitmap)
> +{
> +    if ( isa[0] != 'r' && isa[0] != 'R' )
> +        return -EINVAL;
> +
> +    if ( isa[1] != 'v' && isa[1] != 'V' )
> +        return -EINVAL;
> +
> +    if ( isa[2] != '3' && isa[3] != '2' &&
> +         isa[2] != '6' && isa[3] != '4' )
> +        return -EINVAL;

Any reason to accept the (respectively, depending on configuration) wrong
bitness? Or to accept e.g. RV34?

> +    isa += 4;
> +
> +    while ( *isa )
> +    {
> +        const char *ext = isa++;
> +        const char *ext_end = isa;
> +        bool ext_err = false;
> +
> +        switch ( *ext )
> +        {
> +        case 'x':
> +        case 'X':
> +            if ( acpi_disabled )
> +                printk_once("Vendor extensions are ignored in riscv,isa."
> +                            "Use riscv,isa-extensions instead\n");

How's this connected to ACPI? The more that you said there's nothing
ACPI-ish left.

> +            /*
> +             * 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':

Blank lines please between non-fall-through case blocks.

> +            /*
> +             * Workaround for invalid single-letter 's' & 'u' (QEMU).
> +             * 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;
> +            }

I'm afraid I don't understand this; the comment raises more questions
than it answers.

> +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;
> +
> +    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 = 0;

Pointless initializer.

> +        if ( !dt_device_type_is_equal(cpu, "cpu") )
> +            continue;
> +
> +        cpuid = dt_get_cpuid_from_node(cpu);
> +        if ( cpuid < 0 )
> +            continue;
> +
> +        if ( cpuid >= NR_CPUS )
> +        {
> +            printk_once("%s: dts has more cpu than NR_CPUs\n", __func__);
> +            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);
> +            continue;
> +        }
> +        else
> +            printk("riscv,isa-extensions isnt supported\n");

IOW no matter what, a message will be logged. Odd.

Also: Preferably no "else" after an if() ending in "continue".

> +    }
> +
> +    return -EOPNOTSUPP;
> +}

Hmm, this function then has no way of succeeding? Certainly requires a
comment then.

> +static void __init riscv_fill_hwcap_from_isa_string(void)
> +{
> +    const char *isa;

Like in the earlier function better limit this and ...

> +    const struct dt_device_node *cpus = dt_find_node_by_path("/cpus");
> +    const struct dt_device_node *cpu;
> +    int cpuid = 0;

... this to ...

> +    if ( !acpi_disabled )
> +        panic("%s should be updated correspondingly to support ACPI\n", __func__);
> +
> +    if ( !cpus )
> +    {
> +        printk("Missing /cpus node in the device tree?\n");
> +        return;
> +    }
> +
> +    dt_for_each_child_node(cpus, cpu)
> +    {

... the scope of this loop.

> +        DECLARE_BITMAP(this_isa, RISCV_ISA_EXT_MAX);
> +
> +        if ( !dt_device_type_is_equal(cpu, "cpu") )
> +            continue;
> +
> +        cpuid = dt_get_cpuid_from_node(cpu);
> +        if ( cpuid < 0 )
> +            continue;
> +
> +        if ( cpuid >= NR_CPUS )
> +        {
> +            printk_once("%s: dts has more cpu than NR_CPUs\n", __func__);

What's "dts"? Did the 's' mean to be in "cpus" instead? Also NR_CPUS
to avoid confusion.

> +            continue;
> +        }
> +
> +        if ( acpi_disabled )
> +        {
> +            if ( dt_property_read_string(cpu, "riscv,isa", &isa) )
> +            {
> +                printk("Unable to find \"riscv,isa\" devicetree entry\n");
> +                continue;
> +            }
> +        } else

Nit: Stlye.

> +            panic("there is no support for ACPI\n");
> +
> +        riscv_isa_parse_string(isa, this_isa);
> +
> +        if ( bitmap_empty(riscv_isa, RISCV_ISA_EXT_MAX) )
> +            bitmap_copy(riscv_isa, this_isa, RISCV_ISA_EXT_MAX);
> +        else
> +            bitmap_and(riscv_isa, riscv_isa, this_isa, RISCV_ISA_EXT_MAX);

What if the first instance had no extensions at all? You'll then copy what
the second instance say, ending up with extensions not supported by one of
the CPUs.

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

Nit: Style.

> +        return false;
> +
> +    return test_bit(bit, bmap) ? true : false;

No conditional operator like this is needed when the return type is bool
anyway.

> +void __init riscv_fill_hwcap(void)
> +{
> +    unsigned int i;
> +    size_t req_extns_amount = ARRAY_SIZE(required_extensions);
> +    bool all_extns_available = true;
> +
> +    if ( !acpi_disabled )
> +        riscv_fill_hwcap_from_isa_string();
> +    else {

Style again.

> +        int ret = riscv_fill_hwcap_from_ext_list();
> +
> +        if ( ret )
> +        {
> +            printk("Falling back to deprecated \"riscv,isa\"\n");
> +            riscv_fill_hwcap_from_isa_string();
> +        }
> +    }
> +
> +    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 extenstion: %s\n", ext.name);

extension

> +            all_extns_available = false;
> +        }
> +    }
> +
> +    if ( !all_extns_available )
> +        panic("Look why the extenstions above are needed in booting.txt\n");

extensions

> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/cpufeature.h
> @@ -0,0 +1,63 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef ASM__RISCV__CPUFEATURE_H
> +#define ASM__RISCV__CPUFEATURE_H
> +
> +#ifndef __ASSEMBLY__
> +
> +#define RISCV_ISA_EXT_a     ('a' - 'a')
> +#define RISCV_ISA_EXT_c     ('c' - 'a')
> +#define RISCV_ISA_EXT_d     ('d' - 'a')
> +#define RISCV_ISA_EXT_f     ('f' - 'a')
> +#define RISCV_ISA_EXT_h     ('h' - 'a')
> +#define RISCV_ISA_EXT_i     ('i' - 'a')
> +#define RISCV_ISA_EXT_m     ('m' - 'a')
> +#define RISCV_ISA_EXT_q     ('q' - 'a')
> +#define RISCV_ISA_EXT_v     ('v' - 'a')
> +
> +/*
> + * Increse this to higher value as kernel support more ISA extensions.
> + */
> +#define RISCV_ISA_EXT_MAX   128

What's this about? Why can't the last element of the enum below go without
this, thus not needing manual bumping here?

> +#define RISCV_ISA_EXT_SxAIA     RISCV_ISA_EXT_SSAIA

Why does this expand to RISCV_ISA_EXT_SSAIA and not RISCV_ISA_EXT_SMAIA?
(Easiest way to address: remove the #define, as it's unused. Yet if it
is to be kept, the question needs addressing, perhaps by way of a code
comment.)

> +/*
> + * 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. The maximum, RISCV_ISA_EXT_MAX, is defined in order
> + * to allocate the bitmap and may be increased when necessary.
> + *
> + * 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

The comment living above this #define, it also wants wording to match
this. Specifically the text starts with describing ...

> +enum riscv_isa_ext_id {

... this enum instead (which doesn't consist of any macros).

> +    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_ID_MAX = RISCV_ISA_EXT_MAX,
> +};

Why can't the single-letter RISCV_ISA_EXT_? be part of this enum as well?

> +void riscv_fill_hwcap(void);
> +
> +bool riscv_isa_extension_available(const unsigned long *isa_bitmap, int bit);

A signed bit position? Can negative values be passed in? Actually - can't
this be enum riscv_isa_ext_id anyway?

Jan

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

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 19037 bytes --]

On 1/8/25 4:21 PM, Jan Beulich wrote:

> On 23.12.2024 13:55, Oleksii Kurochko wrote:
>> --- /dev/null
>> +++ b/xen/arch/riscv/cpufeature.c
>> @@ -0,0 +1,466 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Taken for Linux kernel v6.12-rc3 and modified by
>> + * Oleksii Kurochko<oleksii.kurochko@gmail.com>:
>> + *
>> + * - 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.
>> + * - 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 upper and 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.
>> + * - s/pr_info/printk.
>> + * - Apply Xen coding style.
> Having this in the patch description is sufficient imo.
>
>> + * Copyright (C) 2015 ARM Ltd.
>> + * Copyright (C) 2017 SiFive
>> + * Copyright (C) 2024 Vates
>> + */
>> +
>> +#include <xen/acpi.h>
> Didn't you say you dropped the ACPI pieces?

Yes, they are dropped but my intention was to add "panic("%s should be updated correspondingly to support ACPI\n", __func__)"
for the places where ACPI changes should be done in case of ACPI support will be added. And these places are detected by
checking acpi_disabled variable which is in <xen/acpi.h>.

>
>> +#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>
>> +
>> +struct riscv_isa_ext_data {
>> +    const unsigned int id;
>> +    const char *name;
>> +};
> This is odd - why would the id be const, but not the name? Thus you
> require all instances of the struct to have an initializer. The more
> conventional approach is to apply the const on the instances of the
> structure (e.g. as you already have it for riscv_isa_ext[]).

Agree, not too much sense to have const id, but not the name. Then it should be also "const char * const name".

Lets follow conventional approach and declare riscv_isa_ext_data structure as:
   struct riscv_isa_ext_data {
       unsigned int id;
       const char *name;
   };
name member of riscv_isa_ext_data structure still should be "const char *" to avoid compilation error:
   discarding of `const' qualifier from pointer target type.

An option could be to have a case in macros RISCV_ISA_EXT_DATA():
   #define RISCV_ISA_EXT_DATA(ext_name, ext_id)    \
   {                                               \
       .id = ext_id,                               \
       .name = (char *)#ext_name,                  \
   }
But IMO it is better just to declare riscv_isa_ext_data as suggested above.

>> +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));
>> +
>> +    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));
>> +}
>> +
>> +/*
>> + * The canonical order of ISA extension names in the ISA string is defined in
>> + * chapter 27 of the unprivileged specification.
>> + *
>> + * Ordinarily, for in-kernel data structures, this order is unimportant but
>> + * isa_ext_arr defines the order of the ISA string in /proc/cpuinfo.
> Inapplicable Linux detail? (If you want to keep it, you'll want to add
> "Linux'es" and avoid mentioning something that looks like a variable
> but then doesn't exist anywhere.)

Agree, no need for this Linux detail, it is not really useful in our case. I will drop it.

>> +    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),
> Isn't it kind of implied that with the presence of Zbb, B should also be
> present?

My interpretation of the RISC-V Bitmanip Extension spec is that the 'B' extension is essentially a collection of
the Zba, Zbb, Zbs, and other extensions, but it isn't an extension by itself.
The following is mentioned in the spec:
   The bit-manipulation (bitmanip) extension collection is comprised of several component extensions to the base
   RISC-V architecture that are intended to provide some combination of code size reduction, performance
   improvement, and energy reduction. While the instructions are intended to have general use, some instructions
   are more useful in some domains than others. Hence, several smaller bitmanip extensions are provided, rather
   than one large extension. Each of these smaller extensions is grouped by common function and use case, and
   each of which has its own Zb*-extension name.

>> +    RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
>> +    RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
>> +};
> Wouldn't the Z and S prefixes better be recorded in upper case here?

I decided that it is always mentioned in lower case in the spec, but it is wrong and Z and S are
used in upper case (https://github.com/riscv/riscv-isa-manual/blob/main/src/naming.adoc#subset-naming-convention ),
and also there is another one reason mentioned in an answer to another your question below. ( it was marked as [1] )

>> +    RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
>> +    RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
>> +};
>> +
>> +static const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext);
> Is this variable really useful to have?

Not really. ( at least, at the moment ) It could be dropped.

>> +    {
>> +        const struct riscv_isa_ext_data *ext = &riscv_isa_ext[i];
>> +
>> +        if ( (name_end - name == strlen(ext->name)) &&
>> +             !strncasecmp(name, ext->name, name_end - name) )
> Does this really need to be a case-insensitive comparison?

[1]:
Considering that we are discussing to use Z and S in upper case then we need to use strncasecmp()
because `name` comes from riscv, isa property of device tree and it means that letters in
the riscv,isa string must be all lowercase (https://elixir.bootlin.com/linux/v6.12.6/source/Documentation/devicetree/bindings/riscv/extensions.yaml#L34 )
so we have to avoid comparison of extension names which could start in upper case with the same extension name which starts
from lower case:
While the isa <https://elixir.bootlin.com/linux/v6.12.6/B/ident/isa> 
strings in ISA specification are case insensitive, letters in the 
riscv,isa string must be all lowercase. And so to avoid comparison of a 
name from riscv_isa_ext[] which could be theoretically starts from upper 
case we have to use strncasecmp() to transform everything to lower case 
before comparison. And it seems it is better to be on a safe side for 
the cases when some accidentally will use upper case or in 
riscv_isa_ext[] or in riscv,isa property of device-tree.

>> +            break;
>> +        }
>> +    }
>> +}
>> +
>> +static int __init riscv_isa_parse_string(const char *isa, unsigned long *out_bitmap)
>> +{
>> +    if ( isa[0] != 'r' && isa[0] != 'R' )
>> +        return -EINVAL;
>> +
>> +    if ( isa[1] != 'v' && isa[1] != 'V' )
>> +        return -EINVAL;
>> +
>> +    if ( isa[2] != '3' && isa[3] != '2' &&
>> +         isa[2] != '6' && isa[3] != '4' )
>> +        return -EINVAL;
> Any reason to accept the (respectively, depending on configuration) wrong
> bitness? Or to accept e.g. RV34?

Good question. I think there is no any reason to accept the wrong bitness and
it is a bug. I'll re-write the last if-condition.

>
>> +    isa += 4;
>> +
>> +    while ( *isa )
>> +    {
>> +        const char *ext = isa++;
>> +        const char *ext_end = isa;
>> +        bool ext_err = false;
>> +
>> +        switch ( *ext )
>> +        {
>> +        case 'x':
>> +        case 'X':
>> +            if ( acpi_disabled )
>> +                printk_once("Vendor extensions are ignored in riscv,isa."
>> +                            "Use riscv,isa-extensions instead\n");
> How's this connected to ACPI? The more that you said there's nothing
> ACPI-ish left.

The same as above just left that for the case if one day someone will add ACPI support.
( but, at the moment, we could drop it or I will update the comment when I wrote about
dropping of ACPI-connected things to something like: "ACPI is almost fully drop and leave
only places where a code should be updated to have ACPI support".

>> +            /*
>> +             * Workaround for invalid single-letter 's' & 'u' (QEMU).
>> +             * 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;
>> +            }
> I'm afraid I don't understand this; the comment raises more questions
> than it answers.

Some details could be found here about these QEMU workaround from LK view:
https://lore.kernel.org/linux-riscv/ae93358e-e117-b43d-faad-772c529f846c@irq.a4lg.com/#t

This leads to the following fix in QEMU:
https://patchwork.kernel.org/project/qemu-devel/patch/dee09d708405075420b29115c1e9e87910b8da55.1648270894.git.research_trasio@irq.a4lg.com/#24792587

Considering QEMU's patch, these workaround isn't needed anymore since QEMU 7.1 ( it has been released30 Aug 2022 ) probably we could update the
QEMU version on our CI and just drop these changes.
Or, at least, update the comment with the links mentioned above and add a message that these changes are needed only for QEMU < 7.1.
Am I right that we don't have something like GCC_VERSION in Xen but for QEMU?

>> +        if ( !dt_device_type_is_equal(cpu, "cpu") )
>> +            continue;
>> +
>> +        cpuid = dt_get_cpuid_from_node(cpu);
>> +        if ( cpuid < 0 )
>> +            continue;
>> +
>> +        if ( cpuid >= NR_CPUS )
>> +        {
>> +            printk_once("%s: dts has more cpu than NR_CPUs\n", __func__);
>> +            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);
>> +            continue;
>> +        }
>> +        else
>> +            printk("riscv,isa-extensions isnt supported\n");
> IOW no matter what, a message will be logged. Odd.

I think it should be panic but I confused it with printk...

>
> Also: Preferably no "else" after an if() ending in "continue".

If I understand you correctly, we really can drop here "else" and just
panic("riscv,isa-extensions isnt supported\n"), and it would be enough.

>> +        DECLARE_BITMAP(this_isa, RISCV_ISA_EXT_MAX);
>> +
>> +        if ( !dt_device_type_is_equal(cpu, "cpu") )
>> +            continue;
>> +
>> +        cpuid = dt_get_cpuid_from_node(cpu);
>> +        if ( cpuid < 0 )
>> +            continue;
>> +
>> +        if ( cpuid >= NR_CPUS )
>> +        {
>> +            printk_once("%s: dts has more cpu than NR_CPUs\n", __func__);
> What's "dts"? Did the 's' mean to be in "cpus" instead? Also NR_CPUS
> to avoid confusion.

DTS here means Device Tree Source. It would be more clear to write in the following way:
"CPU id is higher then maximum number of CPUs in Xen."
But I am not sure that his check is correct as it is not clearly defined that CPU ids are
from the range from 0 to NR_CPUS. The following is mentioned in the spec:
   Hart IDs might not necessarily be numbered contiguously in a multiprocessor system,
   but at least one hart must have a hart ID of zero. Hart IDs must be unique within the
   execution environment.
All that it guarantees it is that one of them should be zero and IDs are unique.

I think it would be better just to drop this check.

>> +            panic("there is no support for ACPI\n");
>> +
>> +        riscv_isa_parse_string(isa, this_isa);
>> +
>> +        if ( bitmap_empty(riscv_isa, RISCV_ISA_EXT_MAX) )
>> +            bitmap_copy(riscv_isa, this_isa, RISCV_ISA_EXT_MAX);
>> +        else
>> +            bitmap_and(riscv_isa, riscv_isa, this_isa, RISCV_ISA_EXT_MAX);
> What if the first instance had no extensions at all? You'll then copy what
> the second instance say, ending up with extensions not supported by one of
> the CPUs.

I think that it's impossible that there is no extensions at all and it should be
considered as a bug of provided riscv,isa property. Thereby it should be enough to
add BUG_ON(!bitmap_empty(this_isa, RISCV_ISA_EXT_MAX)) before if-condition.

>> --- /dev/null
>> +++ b/xen/arch/riscv/include/asm/cpufeature.h
>> @@ -0,0 +1,63 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +#ifndef ASM__RISCV__CPUFEATURE_H
>> +#define ASM__RISCV__CPUFEATURE_H
>> +
>> +#ifndef __ASSEMBLY__
>> +
>> +#define RISCV_ISA_EXT_a     ('a' - 'a')
>> +#define RISCV_ISA_EXT_c     ('c' - 'a')
>> +#define RISCV_ISA_EXT_d     ('d' - 'a')
>> +#define RISCV_ISA_EXT_f     ('f' - 'a')
>> +#define RISCV_ISA_EXT_h     ('h' - 'a')
>> +#define RISCV_ISA_EXT_i     ('i' - 'a')
>> +#define RISCV_ISA_EXT_m     ('m' - 'a')
>> +#define RISCV_ISA_EXT_q     ('q' - 'a')
>> +#define RISCV_ISA_EXT_v     ('v' - 'a')
>> +
>> +/*
>> + * Increse this to higher value as kernel support more ISA extensions.
>> + */
>> +#define RISCV_ISA_EXT_MAX   128
> What's this about? Why can't the last element of the enum below go without
> this, thus not needing manual bumping here?

RISCV_ISA_EXT_ID_MAX could be used instead of RISCV_ISA_EXT_MAX. RISCV_ISA_EXT_MAX it
is rudiment from Linux kernel which initially I decided to leave to have more close to
Linux kernel code.

>
>> +#define RISCV_ISA_EXT_SxAIA     RISCV_ISA_EXT_SSAIA
> Why does this expand to RISCV_ISA_EXT_SSAIA and not RISCV_ISA_EXT_SMAIA?
> (Easiest way to address: remove the #define, as it's unused. Yet if it
> is to be kept, the question needs addressing, perhaps by way of a code
> comment.)

I agree that it is better just to drop and I will do in that way.
Initial idea was close to Linux kernel. As Linux kernel could run in M mode
then these define should be *_EXT_SMAIA and if it is run in S mode then *_EXT_SSAIA:
#ifdef CONFIG_RISCV_M_MODE 
<https://elixir.bootlin.com/linux/v6.12.6/K/ident/CONFIG_RISCV_M_MODE> 
#define RISCV_ISA_EXT_SxAIA 
<https://elixir.bootlin.com/linux/v6.12.6/C/ident/RISCV_ISA_EXT_SxAIA> 
RISCV_ISA_EXT_SMAIA 
<https://elixir.bootlin.com/linux/v6.12.6/C/ident/RISCV_ISA_EXT_SMAIA> 
#else #define RISCV_ISA_EXT_SxAIA 
<https://elixir.bootlin.com/linux/v6.12.6/C/ident/RISCV_ISA_EXT_SxAIA> 
RISCV_ISA_EXT_SSAIA 
<https://elixir.bootlin.com/linux/v6.12.6/C/ident/RISCV_ISA_EXT_SSAIA> 
#endif And I thought to something similar with hypervisor ( before 
decided that hypervisor mode is mandatory ).

>
>> +/*
>> + * 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. The maximum, RISCV_ISA_EXT_MAX, is defined in order
>> + * to allocate the bitmap and may be increased when necessary.
>> + *
>> + * 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
> The comment living above this #define, it also wants wording to match
> this. Specifically the text starts with describing ...
>
>> +enum riscv_isa_ext_id {
> ... this enum instead (which doesn't consist of any macros).
>
>> +    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_ID_MAX = RISCV_ISA_EXT_MAX,
>> +};
> Why can't the single-letter RISCV_ISA_EXT_? be part of this enum as well?

Good point. It could and would be better. Thereby I will refactor that.


>
>> +void riscv_fill_hwcap(void);
>> +
>> +bool riscv_isa_extension_available(const unsigned long *isa_bitmap, int bit);
> A signed bit position? Can negative values be passed in? Actually - can't
> this be enum riscv_isa_ext_id anyway?

No, negative values can't be passed. Extension IDs are always positive.
If we will move everything ( single-letter extensions too ) to enum
riscv_isa_ext_id then we can use enum riscv_isa_ext_id
instead of int here. I think that it would be really better so I will refactor that
accordingly.

Thanks for review!

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 29527 bytes --]

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

[Jan Beulich]

On 13.01.2025 18:11, Oleksii Kurochko wrote:
> On 1/8/25 4:21 PM, Jan Beulich wrote:
>> On 23.12.2024 13:55, Oleksii Kurochko wrote:
>>> +struct riscv_isa_ext_data {
>>> +    const unsigned int id;
>>> +    const char *name;
>>> +};
>> This is odd - why would the id be const, but not the name? Thus you
>> require all instances of the struct to have an initializer. The more
>> conventional approach is to apply the const on the instances of the
>> structure (e.g. as you already have it for riscv_isa_ext[]).
> 
> Agree, not too much sense to have const id, but not the name. Then it should be also "const char * const name".
> 
> Lets follow conventional approach and declare riscv_isa_ext_data structure as:
>    struct riscv_isa_ext_data {
>        unsigned int id;
>        const char *name;
>    };

Yes, this is what I'd have expected. I'm afraid ...

> name member of riscv_isa_ext_data structure still should be "const char *" to avoid compilation error:
>    discarding of `const' qualifier from pointer target type.
> 
> An option could be to have a case in macros RISCV_ISA_EXT_DATA():
>    #define RISCV_ISA_EXT_DATA(ext_name, ext_id)    \
>    {                                               \
>        .id = ext_id,                               \
>        .name = (char *)#ext_name,                  \
>    }
> But IMO it is better just to declare riscv_isa_ext_data as suggested above.

... I don't really follow all of this. It's clear though that the (char *)
cast is a Misra violation, and hence anything like this is out of question
anyway.

>>> +    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),
>> Isn't it kind of implied that with the presence of Zbb, B should also be
>> present?
> 
> My interpretation of the RISC-V Bitmanip Extension spec is that the 'B' extension is essentially a collection of
> the Zba, Zbb, Zbs, and other extensions, but it isn't an extension by itself.
> The following is mentioned in the spec:
>    The bit-manipulation (bitmanip) extension collection is comprised of several component extensions to the base
>    RISC-V architecture that are intended to provide some combination of code size reduction, performance
>    improvement, and energy reduction. While the instructions are intended to have general use, some instructions
>    are more useful in some domains than others. Hence, several smaller bitmanip extensions are provided, rather
>    than one large extension. Each of these smaller extensions is grouped by common function and use case, and
>    each of which has its own Zb*-extension name.

Still the doc has '"B" Extension for Bit Manipulation' as the title of the
chapter. And gas accepts B as an extension (e.g. ".option arch, +b").

>>> +            /*
>>> +             * Workaround for invalid single-letter 's' & 'u' (QEMU).
>>> +             * 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;
>>> +            }
>> I'm afraid I don't understand this; the comment raises more questions
>> than it answers.
> 
> Some details could be found here about these QEMU workaround from LK view:
> https://lore.kernel.org/linux-riscv/ae93358e-e117-b43d-faad-772c529f846c@irq.a4lg.com/#t
> 
> This leads to the following fix in QEMU:
> https://patchwork.kernel.org/project/qemu-devel/patch/dee09d708405075420b29115c1e9e87910b8da55.1648270894.git.research_trasio@irq.a4lg.com/#24792587
> 
> Considering QEMU's patch, these workaround isn't needed anymore since QEMU 7.1 ( it has been released30 Aug 2022 ) probably we could update the
> QEMU version on our CI and just drop these changes.
> Or, at least, update the comment with the links mentioned above and add a message that these changes are needed only for QEMU < 7.1.
> Am I right that we don't have something like GCC_VERSION in Xen but for QEMU?

How could there be? At the time of building Xen we know what compiler
version is in use, but we clearly don't know under what qemu versions
it might later be run.

>>> +        riscv_isa_parse_string(isa, this_isa);
>>> +
>>> +        if ( bitmap_empty(riscv_isa, RISCV_ISA_EXT_MAX) )
>>> +            bitmap_copy(riscv_isa, this_isa, RISCV_ISA_EXT_MAX);
>>> +        else
>>> +            bitmap_and(riscv_isa, riscv_isa, this_isa, RISCV_ISA_EXT_MAX);
>> What if the first instance had no extensions at all? You'll then copy what
>> the second instance say, ending up with extensions not supported by one of
>> the CPUs.
> 
> I think that it's impossible that there is no extensions at all and it should be
> considered as a bug of provided riscv,isa property. Thereby it should be enough to
> add BUG_ON(!bitmap_empty(this_isa, RISCV_ISA_EXT_MAX)) before if-condition.

Well, you can of course make such an assumption. I don't think though that
it's technically impossible to have an extension-less environment. Xen
won't be able to run there, though (we'll require H at the very least aiui,
and I'm sure we really also require Zicsr).

Jan

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

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 6057 bytes --]


On 1/14/25 8:33 AM, Jan Beulich wrote:
>>>> +    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),
>>> Isn't it kind of implied that with the presence of Zbb, B should also be
>>> present?
>> My interpretation of the RISC-V Bitmanip Extension spec is that the 'B' extension is essentially a collection of
>> the Zba, Zbb, Zbs, and other extensions, but it isn't an extension by itself.
>> The following is mentioned in the spec:
>>     The bit-manipulation (bitmanip) extension collection is comprised of several component extensions to the base
>>     RISC-V architecture that are intended to provide some combination of code size reduction, performance
>>     improvement, and energy reduction. While the instructions are intended to have general use, some instructions
>>     are more useful in some domains than others. Hence, several smaller bitmanip extensions are provided, rather
>>     than one large extension. Each of these smaller extensions is grouped by common function and use case, and
>>     each of which has its own Zb*-extension name.
> Still the doc has '"B" Extension for Bit Manipulation' as the title of the
> chapter.
> And gas accepts B as an extension (e.g. ".option arch, +b").

I think it is fine.
B, in this case, just represents Zba, Zbb, Zbc, Zbs and that all of them are supported at the same time.
But I see chips that doesn't have B because it doesn't have one of those extensions.

>
>>>> +            /*
>>>> +             * Workaround for invalid single-letter 's' & 'u' (QEMU).
>>>> +             * 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;
>>>> +            }
>>> I'm afraid I don't understand this; the comment raises more questions
>>> than it answers.
>> Some details could be found here about these QEMU workaround from LK view:
>> https://lore.kernel.org/linux-riscv/ae93358e-e117-b43d-faad-772c529f846c@irq.a4lg.com/#t
>>
>> This leads to the following fix in QEMU:
>> https://patchwork.kernel.org/project/qemu-devel/patch/dee09d708405075420b29115c1e9e87910b8da55.1648270894.git.research_trasio@irq.a4lg.com/#24792587
>>
>> Considering QEMU's patch, these workaround isn't needed anymore since QEMU 7.1 ( it has been released30 Aug 2022 ) probably we could update the
>> QEMU version on our CI and just drop these changes.
>> Or, at least, update the comment with the links mentioned above and add a message that these changes are needed only for QEMU < 7.1.
>> Am I right that we don't have something like GCC_VERSION in Xen but for QEMU?
> How could there be? At the time of building Xen we know what compiler
> version is in use, but we clearly don't know under what qemu versions
> it might later be run.

Agree with that, there is no any sense for having something similar as GCC_VERSIOB but
for QEMU. Then I will just update the comment around this workaround with some clarifications.

>
>>>> +        riscv_isa_parse_string(isa, this_isa);
>>>> +
>>>> +        if ( bitmap_empty(riscv_isa, RISCV_ISA_EXT_MAX) )
>>>> +            bitmap_copy(riscv_isa, this_isa, RISCV_ISA_EXT_MAX);
>>>> +        else
>>>> +            bitmap_and(riscv_isa, riscv_isa, this_isa, RISCV_ISA_EXT_MAX);
>>> What if the first instance had no extensions at all? You'll then copy what
>>> the second instance say, ending up with extensions not supported by one of
>>> the CPUs.
>> I think that it's impossible that there is no extensions at all and it should be
>> considered as a bug of provided riscv,isa property. Thereby it should be enough to
>> add BUG_ON(!bitmap_empty(this_isa, RISCV_ISA_EXT_MAX)) before if-condition.
> Well, you can of course make such an assumption. I don't think though that
> it's technically impossible to have an extension-less environment. Xen
> won't be able to run there, though (we'll require H at the very least aiui,
> and I'm sure we really also require Zicsr).

I would like to clarify some things. I think we are counting by word 'extension' different things.
I am including to this `Base ISA` ( and likely it is incorrect to do so ( or,at least, confusing )
and I will try not to do that in the future. I am using it in this manner because `Base ISA` is
included to the table 74. Standard ISA extension names in Unpriv spec ) then it is impossible to
have an extension-less environment because the spec mentions the following:

     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.

So, at least, r{32,64,128}i should written in riscv,isa property of DTS file and that is the reason why
this_isa can't be empty, and thereby riscv_isa will be initialized with, at least, `i` ending up with only `i`
supported by all CPUs.

But if not count `I` as an extension and just as base ISA then it is really technically possible to come up with
extension-less environment. But anyway as you mentioned we still need for Xen some extensions. ( btw, thanks, I missed to
add Zicsr to required_extensions[] ).

Thanks.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 8190 bytes --]