Merge patch series "ISA string parser cleanups"
authorPalmer Dabbelt <palmer@rivosinc.com>
Wed, 21 Jun 2023 14:49:09 +0000 (07:49 -0700)
committerPalmer Dabbelt <palmer@rivosinc.com>
Fri, 23 Jun 2023 17:06:20 +0000 (10:06 -0700)
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>
1  2 
Documentation/devicetree/bindings/riscv/cpus.yaml
arch/riscv/include/asm/hwcap.h
arch/riscv/include/asm/processor.h
arch/riscv/kernel/cpu.c
arch/riscv/kernel/cpufeature.c
arch/riscv/kernel/smpboot.c

@@@ -61,7 -61,7 +61,7 @@@ properties
        hart.  These values originate from the RISC-V Privileged
        Specification document, available from
        https://riscv.org/specifications/
 -    $ref: "/schemas/types.yaml#/definitions/string"
 +    $ref: /schemas/types.yaml#/definitions/string
      enum:
        - riscv,sv32
        - riscv,sv39
        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
        lowercase.
 -    $ref: "/schemas/types.yaml#/definitions/string"
 +    $ref: /schemas/types.yaml#/definitions/string
      pattern: ^rv(?:64|32)imaf?d?q?c?b?k?j?p?v?h?(?:[hsxz](?:[a-z])+)?(?:_[hsxz](?:[a-z])+)*$
  
    # RISC-V requires 'timebase-frequency' in /cpus, so disallow it here
        - interrupt-controller
  
    cpu-idle-states:
 -    $ref: '/schemas/types.yaml#/definitions/phandle-array'
 +    $ref: /schemas/types.yaml#/definitions/phandle-array
      items:
        maxItems: 1
      description: |
@@@ -22,7 -22,6 +22,7 @@@
  #define RISCV_ISA_EXT_m               ('m' - 'a')
  #define RISCV_ISA_EXT_s               ('s' - 'a')
  #define RISCV_ISA_EXT_u               ('u' - 'a')
 +#define RISCV_ISA_EXT_v               ('v' - 'a')
  
  /*
   * These macros represent the logical IDs of each multi-letter RISC-V ISA
  #define RISCV_ISA_EXT_ZICBOZ          34
  #define RISCV_ISA_EXT_SMAIA           35
  #define RISCV_ISA_EXT_SSAIA           36
 -#define RISCV_ISA_EXT_ZICNTR          37
 -#define RISCV_ISA_EXT_ZICSR           38
 -#define RISCV_ISA_EXT_ZIFENCEI                39
 -#define RISCV_ISA_EXT_ZIHPM           40
 +#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
@@@ -63,8 -64,6 +67,8 @@@
  
  #include <linux/jump_label.h>
  
 +unsigned long riscv_get_elf_hwcap(void);
 +
  struct riscv_isa_ext_data {
        /* Name of the extension displayed to userspace via /proc/cpuinfo */
        char uprop[RISCV_ISA_EXT_NAME_LEN_MAX];
@@@ -7,7 -7,6 +7,7 @@@
  #define _ASM_RISCV_PROCESSOR_H
  
  #include <linux/const.h>
 +#include <linux/cache.h>
  
  #include <vdso/processor.h>
  
@@@ -40,8 -39,6 +40,8 @@@ struct thread_struct 
        unsigned long s[12];    /* s[0]: frame pointer */
        struct __riscv_d_ext_state fstate;
        unsigned long bad_cause;
 +      unsigned long vstate_ctrl;
 +      struct __riscv_v_ext_state vstate;
  };
  
  /* Whitelist the fstate from the task_struct for hardened usercopy */
@@@ -78,21 -75,12 +78,22 @@@ static inline void wait_for_interrupt(v
  
  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);
  extern int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src);
  
 +extern unsigned long signal_minsigstksz __ro_after_init;
 +
 +#ifdef CONFIG_RISCV_ISA_V
 +/* Userspace interface for PR_RISCV_V_{SET,GET}_VS prctl()s: */
 +#define RISCV_V_SET_CONTROL(arg)      riscv_v_vstate_ctrl_set_current(arg)
 +#define RISCV_V_GET_CONTROL()         riscv_v_vstate_ctrl_get_current()
 +extern long riscv_v_vstate_ctrl_set_current(unsigned long arg);
 +extern long riscv_v_vstate_ctrl_get_current(void);
 +#endif /* CONFIG_RISCV_ISA_V */
 +
  #endif /* __ASSEMBLY__ */
  
  #endif /* _ASM_RISCV_PROCESSOR_H */
diff --combined arch/riscv/kernel/cpu.c
   */
  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 +50,7 @@@
                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;
                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,10 -208,12 +208,14 @@@ 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),
        __RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
        __RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
        __RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
@@@ -21,7 -21,6 +21,7 @@@
  #include <asm/hwcap.h>
  #include <asm/patch.h>
  #include <asm/processor.h>
 +#include <asm/vector.h>
  
  #define NUM_ALPHA_EXTS ('z' - 'a' + 1)
  
@@@ -30,9 -29,6 +30,9 @@@ unsigned long elf_hwcap __read_mostly
  /* Host ISA bitmap */
  static DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX) __read_mostly;
  
 +/* Per-cpu ISA extensions. */
 +struct riscv_isainfo hart_isa[NR_CPUS];
 +
  /* Performance information */
  DEFINE_PER_CPU(long, misaligned_access_speed);
  
@@@ -78,10 -74,10 +78,10 @@@ static bool riscv_isa_extension_check(i
        switch (id) {
        case RISCV_ISA_EXT_ZICBOM:
                if (!riscv_cbom_block_size) {
 -                      pr_err("Zicbom detected in ISA string, but no cbom-block-size found\n");
 +                      pr_err("Zicbom detected in ISA string, disabling as no cbom-block-size found\n");
                        return false;
                } else if (!is_power_of_2(riscv_cbom_block_size)) {
 -                      pr_err("cbom-block-size present, but is not a power-of-2\n");
 +                      pr_err("Zicbom disabled as cbom-block-size present, but is not a power-of-2\n");
                        return false;
                }
                return true;
@@@ -116,7 -112,6 +116,7 @@@ void __init riscv_fill_hwcap(void
        isa2hwcap['f' - 'a'] = COMPAT_HWCAP_ISA_F;
        isa2hwcap['d' - 'a'] = COMPAT_HWCAP_ISA_D;
        isa2hwcap['c' - 'a'] = COMPAT_HWCAP_ISA_C;
 +      isa2hwcap['v' - 'a'] = COMPAT_HWCAP_ISA_V;
  
        elf_hwcap = 0;
  
        }
  
        for_each_possible_cpu(cpu) {
 +              struct riscv_isainfo *isainfo = &hart_isa[cpu];
                unsigned long this_hwcap = 0;
-               const char *temp;
 -              DECLARE_BITMAP(this_isa, RISCV_ISA_EXT_MAX);
  
                if (acpi_disabled) {
                        node = of_cpu_device_node_get(cpu);
                        }
                }
  
-               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;
 -              bitmap_zero(this_isa, RISCV_ISA_EXT_MAX);
+               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
                        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 {                                                            \
                                if ((ext_end - ext == sizeof(name) - 1) &&              \
                                     !strncasecmp(ext, name, sizeof(name) - 1) &&       \
                                     riscv_isa_extension_check(bit))                    \
 -                                      set_bit(bit, this_isa);                         \
 +                                      set_bit(bit, isainfo->isa);                     \
                        } while (false)                                                 \
  
                        if (unlikely(ext_err))
  
                                if (riscv_isa_extension_check(nr)) {
                                        this_hwcap |= isa2hwcap[nr];
 -                                      set_bit(nr, this_isa);
 +                                      set_bit(nr, isainfo->isa);
                                }
                        } else {
                                /* sorted alphabetically */
                                SET_ISA_EXT_MAP("svinval", RISCV_ISA_EXT_SVINVAL);
                                SET_ISA_EXT_MAP("svnapot", RISCV_ISA_EXT_SVNAPOT);
                                SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
 +                              SET_ISA_EXT_MAP("zba", RISCV_ISA_EXT_ZBA);
                                SET_ISA_EXT_MAP("zbb", RISCV_ISA_EXT_ZBB);
 +                              SET_ISA_EXT_MAP("zbs", RISCV_ISA_EXT_ZBS);
                                SET_ISA_EXT_MAP("zicbom", RISCV_ISA_EXT_ZICBOM);
                                SET_ISA_EXT_MAP("zicboz", RISCV_ISA_EXT_ZICBOZ);
                                SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
                }
  
                /*
 -              set_bit(RISCV_ISA_EXT_ZICSR, this_isa);
 -              set_bit(RISCV_ISA_EXT_ZIFENCEI, this_isa);
+                * Linux requires the following extensions, so we may as well
+                * always set them.
+                */
 -                      set_bit(RISCV_ISA_EXT_ZICNTR, this_isa);
 -                      set_bit(RISCV_ISA_EXT_ZIHPM, this_isa);
++              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.
                        elf_hwcap = this_hwcap;
  
                if (bitmap_empty(riscv_isa, RISCV_ISA_EXT_MAX))
 -                      bitmap_copy(riscv_isa, this_isa, RISCV_ISA_EXT_MAX);
 +                      bitmap_copy(riscv_isa, isainfo->isa, RISCV_ISA_EXT_MAX);
                else
 -                      bitmap_and(riscv_isa, riscv_isa, this_isa, RISCV_ISA_EXT_MAX);
 +                      bitmap_and(riscv_isa, riscv_isa, isainfo->isa, RISCV_ISA_EXT_MAX);
        }
  
        if (!acpi_disabled && rhct)
                elf_hwcap &= ~COMPAT_HWCAP_ISA_F;
        }
  
 +      if (elf_hwcap & COMPAT_HWCAP_ISA_V) {
 +              riscv_v_setup_vsize();
 +              /*
 +               * ISA string in device tree might have 'v' flag, but
 +               * CONFIG_RISCV_ISA_V is disabled in kernel.
 +               * Clear V flag in elf_hwcap if CONFIG_RISCV_ISA_V is disabled.
 +               */
 +              if (!IS_ENABLED(CONFIG_RISCV_ISA_V))
 +                      elf_hwcap &= ~COMPAT_HWCAP_ISA_V;
 +      }
 +
        memset(print_str, 0, sizeof(print_str));
        for (i = 0, j = 0; i < NUM_ALPHA_EXTS; i++)
                if (riscv_isa[0] & BIT_MASK(i))
        pr_info("riscv: ELF capabilities %s\n", print_str);
  }
  
 +unsigned long riscv_get_elf_hwcap(void)
 +{
 +      unsigned long hwcap;
 +
 +      hwcap = (elf_hwcap & ((1UL << RISCV_ISA_EXT_BASE) - 1));
 +
 +      if (!riscv_v_vstate_ctrl_user_allowed())
 +              hwcap &= ~COMPAT_HWCAP_ISA_V;
 +
 +      return hwcap;
 +}
 +
  #ifdef CONFIG_RISCV_ALTERNATIVE
  /*
   * Alternative patch sites consider 48 bits when determining when to patch
@@@ -32,8 -32,6 +32,8 @@@
  #include <asm/tlbflush.h>
  #include <asm/sections.h>
  #include <asm/smp.h>
 +#include <uapi/asm/hwcap.h>
 +#include <asm/vector.h>
  
  #include "head.h"
  
@@@ -150,7 -148,7 +150,7 @@@ static void __init of_parse_and_init_cp
        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;
  
@@@ -246,11 -244,6 +246,11 @@@ asmlinkage __visible void smp_callin(vo
        set_cpu_online(curr_cpuid, 1);
        probe_vendor_features(curr_cpuid);
  
 +      if (has_vector()) {
 +              if (riscv_v_setup_vsize())
 +                      elf_hwcap &= ~COMPAT_HWCAP_ISA_V;
 +      }
 +
        /*
         * Remote TLB flushes are ignored while the CPU is offline, so emit
         * a local TLB flush right now just in case.