Merge patch series "ISA string parser cleanups"

Conor Dooley <conor@kernel.org> says:

From: Conor Dooley <conor.dooley@microchip.com>

Here are some bits that were discussed with Drew on the "should we
allow caps" threads that I have now created patches for:
- splitting of riscv_of_processor_hartid() into two distinct functions,
  one for use purely during early boot, prior to the establishment of
  the possible-cpus mask & another to fit the other current use-cases
- that then allows us to then completely skip some validation of the
  hartid in the parser
- the biggest diff in the series is a rework of the comments in the
  parser, as I have mostly found the existing (sparse) ones to not be
  all that helpful whenever I have to go back and look at it
- from writing the comments, I found a conditional doing a bit of a
  dance that I found counter-intuitive, so I've had a go at making that
  match what I would expect a little better
- `i` implies 4 other extensions, so add them as extensions and set
  them for the craic. Sure why not like...

* b4-shazam-merge:
  RISC-V: always report presence of extensions formerly part of the base ISA
  dt-bindings: riscv: explicitly mention assumption of Zicntr & Zihpm support
  RISC-V: remove decrement/increment dance in ISA string parser
  RISC-V: rework comments in ISA string parser
  RISC-V: validate riscv,isa at boot, not during ISA string parsing
  RISC-V: split early & late of_node to hartid mapping
  RISC-V: simplify register width check in ISA string parsing

Link: https://lore.kernel.org/r/20230607-audacity-overhaul-82bb867a825f@spud
Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>

diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml
index 8a56473..c2ed979 100644 (file)
--- a/Documentation/devicetree/bindings/riscv/cpus.yaml
+++ b/Documentation/devicetree/bindings/riscv/cpus.yaml
@@ -89,8 +89,8 @@ properties:
       Due to revisions of the ISA specification, some deviations
       have arisen over time.
       Notably, riscv,isa was defined prior to the creation of the
-      Zicsr and Zifencei extensions and thus "i" implies
-      "zicsr_zifencei".
+      Zicntr, Zicsr, Zifencei and Zihpm extensions and thus "i"
+      implies "zicntr_zicsr_zifencei_zihpm".
 
       While the isa strings in ISA specification are case
       insensitive, letters in the riscv,isa string must be all

diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index bdd614d..f041bfa 100644 (file)
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -49,6 +49,10 @@
 #define RISCV_ISA_EXT_SSAIA            36
 #define RISCV_ISA_EXT_ZBA              37
 #define RISCV_ISA_EXT_ZBS              38
+#define RISCV_ISA_EXT_ZICNTR           39
+#define RISCV_ISA_EXT_ZICSR            40
+#define RISCV_ISA_EXT_ZIFENCEI         41
+#define RISCV_ISA_EXT_ZIHPM            42
 
 #define RISCV_ISA_EXT_MAX              64
 #define RISCV_ISA_EXT_NAME_LEN_MAX     32

diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
index e82af10..c950a8d 100644 (file)
--- a/arch/riscv/include/asm/processor.h
+++ b/arch/riscv/include/asm/processor.h
@@ -78,6 +78,7 @@ static inline void wait_for_interrupt(void)
 
 struct device_node;
 int riscv_of_processor_hartid(struct device_node *node, unsigned long *hartid);
+int riscv_early_of_processor_hartid(struct device_node *node, unsigned long *hartid);
 int riscv_of_parent_hartid(struct device_node *node, unsigned long *hartid);
 
 extern void riscv_fill_hwcap(void);

diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index e58e93d..a2fc952 100644 (file)
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -23,6 +23,26 @@
  */
 int riscv_of_processor_hartid(struct device_node *node, unsigned long *hart)
 {
+       int cpu;
+
+       *hart = (unsigned long)of_get_cpu_hwid(node, 0);
+       if (*hart == ~0UL) {
+               pr_warn("Found CPU without hart ID\n");
+               return -ENODEV;
+       }
+
+       cpu = riscv_hartid_to_cpuid(*hart);
+       if (cpu < 0)
+               return cpu;
+
+       if (!cpu_possible(cpu))
+               return -ENODEV;
+
+       return 0;
+}
+
+int riscv_early_of_processor_hartid(struct device_node *node, unsigned long *hart)
+{
        const char *isa;
 
        if (!of_device_is_compatible(node, "riscv")) {
@@ -30,7 +50,7 @@ int riscv_of_processor_hartid(struct device_node *node, unsigned long *hart)
                return -ENODEV;
        }
 
-       *hart = (unsigned long) of_get_cpu_hwid(node, 0);
+       *hart = (unsigned long)of_get_cpu_hwid(node, 0);
        if (*hart == ~0UL) {
                pr_warn("Found CPU without hart ID\n");
                return -ENODEV;
@@ -45,10 +65,12 @@ int riscv_of_processor_hartid(struct device_node *node, unsigned long *hart)
                pr_warn("CPU with hartid=%lu has no \"riscv,isa\" property\n", *hart);
                return -ENODEV;
        }
-       if (tolower(isa[0]) != 'r' || tolower(isa[1]) != 'v') {
-               pr_warn("CPU with hartid=%lu has an invalid ISA of \"%s\"\n", *hart, isa);
+
+       if (IS_ENABLED(CONFIG_32BIT) && strncasecmp(isa, "rv32ima", 7))
+               return -ENODEV;
+
+       if (IS_ENABLED(CONFIG_64BIT) && strncasecmp(isa, "rv64ima", 7))
                return -ENODEV;
-       }
 
        return 0;
 }
@@ -186,7 +208,11 @@ arch_initcall(riscv_cpuinfo_init);
 static struct riscv_isa_ext_data isa_ext_arr[] = {
        __RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM),
        __RISCV_ISA_EXT_DATA(zicboz, RISCV_ISA_EXT_ZICBOZ),
+       __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(zba, RISCV_ISA_EXT_ZBA),
        __RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
        __RISCV_ISA_EXT_DATA(zbs, RISCV_ISA_EXT_ZBS),

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index f8dc577..bdcf460 100644 (file)
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -131,7 +131,6 @@ void __init riscv_fill_hwcap(void)
        for_each_possible_cpu(cpu) {
                struct riscv_isainfo *isainfo = &hart_isa[cpu];
                unsigned long this_hwcap = 0;
-               const char *temp;
 
                if (acpi_disabled) {
                        node = of_cpu_device_node_get(cpu);
@@ -154,22 +153,22 @@ void __init riscv_fill_hwcap(void)
                        }
                }
 
-               temp = isa;
-               if (IS_ENABLED(CONFIG_32BIT) && !strncasecmp(isa, "rv32", 4))
-                       isa += 4;
-               else if (IS_ENABLED(CONFIG_64BIT) && !strncasecmp(isa, "rv64", 4))
-                       isa += 4;
-               /* The riscv,isa DT property must start with rv64 or rv32 */
-               if (temp == isa)
-                       continue;
-               for (; *isa; ++isa) {
+               /*
+                * For all possible cpus, we have already validated in
+                * the boot process that they at least contain "rv" and
+                * whichever of "32"/"64" this kernel supports, and so this
+                * section can be skipped.
+                */
+               isa += 4;
+
+               while (*isa) {
                        const char *ext = isa++;
                        const char *ext_end = isa;
                        bool ext_long = false, ext_err = false;
 
                        switch (*ext) {
                        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 until multi-letter
@@ -186,55 +185,101 @@ void __init riscv_fill_hwcap(void)
                        case 'X':
                        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.
+                                */
                                ext_long = true;
-                               /* Multi-letter extension must be delimited */
                                for (; *isa && *isa != '_'; ++isa)
                                        if (unlikely(!isalnum(*isa)))
                                                ext_err = true;
-                               /* Parse backwards */
+
                                ext_end = isa;
                                if (unlikely(ext_err))
                                        break;
+
                                if (!isdigit(ext_end[-1]))
                                        break;
-                               /* Skip the minor version */
+
                                while (isdigit(*--ext_end))
                                        ;
-                               if (tolower(ext_end[0]) != 'p'
-                                   || !isdigit(ext_end[-1])) {
-                                       /* Advance it to offset the pre-decrement */
+
+                               if (tolower(ext_end[0]) != 'p' || !isdigit(ext_end[-1])) {
                                        ++ext_end;
                                        break;
                                }
-                               /* Skip the major version */
+
                                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;
                                }
-                               /* Find next extension */
+
                                if (!isdigit(*isa))
                                        break;
-                               /* Skip the minor version */
+
                                while (isdigit(*++isa))
                                        ;
+
                                if (tolower(*isa) != 'p')
                                        break;
+
                                if (!isdigit(*++isa)) {
                                        --isa;
                                        break;
                                }
-                               /* Skip the major version */
+
                                while (isdigit(*++isa))
                                        ;
+
                                break;
                        }
-                       if (*isa != '_')
-                               --isa;
+
+                       /*
+                        * 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;
 
 #define SET_ISA_EXT_MAP(name, bit)                                                     \
                        do {                                                            \
@@ -273,6 +318,23 @@ void __init riscv_fill_hwcap(void)
                }
 
                /*
+                * Linux requires the following extensions, so we may as well
+                * always set them.
+                */
+               set_bit(RISCV_ISA_EXT_ZICSR, isainfo->isa);
+               set_bit(RISCV_ISA_EXT_ZIFENCEI, isainfo->isa);
+
+               /*
+                * These ones were as they were part of the base ISA when the
+                * port & dt-bindings were upstreamed, and so can be set
+                * unconditionally where `i` is in riscv,isa on DT systems.
+                */
+               if (acpi_disabled) {
+                       set_bit(RISCV_ISA_EXT_ZICNTR, isainfo->isa);
+                       set_bit(RISCV_ISA_EXT_ZIHPM, isainfo->isa);
+               }
+
+               /*
                 * All "okay" hart should have same isa. Set HWCAP based on
                 * common capabilities of every "okay" hart, in case they don't
                 * have.

diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
index 6ca2b53..bb0b76e 100644 (file)
--- a/arch/riscv/kernel/smpboot.c
+++ b/arch/riscv/kernel/smpboot.c
@@ -150,7 +150,7 @@ static void __init of_parse_and_init_cpus(void)
        cpu_set_ops(0);
 
        for_each_of_cpu_node(dn) {
-               rc = riscv_of_processor_hartid(dn, &hart);
+               rc = riscv_early_of_processor_hartid(dn, &hart);
                if (rc < 0)
                        continue;