From fed14be476f075a523fd4addfee07cb2f8dc1971 Mon Sep 17 00:00:00 2001
From: Conor Dooley <conor.dooley@microchip.com>
Date: Wed, 7 Jun 2023 21:28:25 +0100
Subject: [PATCH 1/7] RISC-V: simplify register width check in ISA string
parsing
Saving off the `isa` pointer to a temp variable, followed by checking if
it has been incremented is a bit of an odd pattern. Perhaps it was done
to avoid a funky looking if statement mixed with the ifdeffery.
Now that we use IS_ENABLED() here just return from the parser as soon as
we detect a mismatch between the string and the currently running
kernel.
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
Reviewed-by: Sunil V L <sunilvl@ventanamicro.com>
Link: https://lore.kernel.org/r/20230607-splatter-bacterium-a75bb9f0d0b7@spud
Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
---
arch/riscv/kernel/cpufeature.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index e3324d661fb96..c8635211fc180 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -126,7 +126,6 @@ void __init riscv_fill_hwcap(void)
for_each_possible_cpu(cpu) {
unsigned long this_hwcap = 0;
DECLARE_BITMAP(this_isa, RISCV_ISA_EXT_MAX);
- const char *temp;
if (acpi_disabled) {
node = of_cpu_device_node_get(cpu);
@@ -149,14 +148,14 @@ 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)
+ if (IS_ENABLED(CONFIG_32BIT) && strncasecmp(isa, "rv32", 4))
continue;
+
+ if (IS_ENABLED(CONFIG_64BIT) && strncasecmp(isa, "rv64", 4))
+ continue;
+
+ isa += 4;
+
bitmap_zero(this_isa, RISCV_ISA_EXT_MAX);
for (; *isa; ++isa) {
const char *ext = isa++;
From 2ac874343749b76e069cff5fea09c49e0bd365a0 Mon Sep 17 00:00:00 2001
From: Conor Dooley <conor.dooley@microchip.com>
Date: Wed, 7 Jun 2023 21:28:26 +0100
Subject: [PATCH 2/7] RISC-V: split early & late of_node to hartid mapping
Some back and forth with Drew [1] about riscv_fill_hwcap() resulted in
the realisation that it is not very useful to parse the DT & perform
validation of riscv,isa every time we would like to get the id for a
hart.
Although it is no longer called in riscv_fill_hwcap(),
riscv_of_processor_hartid() is called in several other places.
Notably in setup_smp() it forms part of the logic for filling the mask
of possible CPUs. Since a possible CPU must have passed this basic
validation of riscv,isa, a repeat validation is not required.
Rename riscv_of_processor_id() to riscv_early_of_processor_id(),
which will be called from setup_smp() & introduce a new
riscv_of_processor_id() which makes use of the pre-populated mask of
possible cpus.
Link: https://lore.kernel.org/linux-riscv/xvdswl3iyikwvamny7ikrxo2ncuixshtg3f6uucjahpe3xpc5c@ud4cz4fkg5dj/ [1]
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
Reviewed-by: Sunil V L <sunilvl@ventanamicro.com>
Link: https://lore.kernel.org/r/20230607-glade-pastel-d8cbd9d9f3c6@spud
Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
---
arch/riscv/include/asm/processor.h | 1 +
arch/riscv/kernel/cpu.c | 22 +++++++++++++++++++++-
arch/riscv/kernel/smpboot.c | 2 +-
3 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
index 94a0590c69710..3479f9fca4b09 100644
--- a/arch/riscv/include/asm/processor.h
+++ b/arch/riscv/include/asm/processor.h
@@ -75,6 +75,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 637263f9a7b94..8025de06edb76 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -22,6 +22,26 @@
* isn't an enabled and valid RISC-V hart node.
*/
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;
@@ -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;
diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
index 67bc5ef3e8b24..3f42331c89120 100644
--- a/arch/riscv/kernel/smpboot.c
+++ b/arch/riscv/kernel/smpboot.c
@@ -148,7 +148,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;
From 069b0d51707721d5ab2001df866b66b82e4c1c35 Mon Sep 17 00:00:00 2001
From: Conor Dooley <conor.dooley@microchip.com>
Date: Wed, 7 Jun 2023 21:28:27 +0100
Subject: [PATCH 3/7] RISC-V: validate riscv,isa at boot, not during ISA string
parsing
Since riscv_fill_hwcap() now only iterates over possible cpus, the
basic validation of whether riscv,isa contains "rv<width>" can be moved
to riscv_early_of_processor_hartid().
Further, "ima" support is required by the kernel, so reject any CPU not
fitting the bill.
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
Reviewed-by: Sunil V L <sunilvl@ventanamicro.com>
Link: https://lore.kernel.org/r/20230607-guts-blurry-67e711acf328@spud
Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
---
arch/riscv/kernel/cpu.c | 8 +++++---
arch/riscv/kernel/cpufeature.c | 12 ++++++------
2 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index 8025de06edb76..dfb4a2a61050e 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -65,10 +65,12 @@ int riscv_early_of_processor_hartid(struct device_node *node, unsigned long *har
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;
}
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index c8635211fc180..c3851c8cfa9c9 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -148,12 +148,12 @@ void __init riscv_fill_hwcap(void)
}
}
- if (IS_ENABLED(CONFIG_32BIT) && strncasecmp(isa, "rv32", 4))
- continue;
-
- if (IS_ENABLED(CONFIG_64BIT) && strncasecmp(isa, "rv64", 4))
- continue;
-
+ /*
+ * 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);
From 6b913e3da87da1be57096c068b4d2e7d4b31f457 Mon Sep 17 00:00:00 2001
From: Conor Dooley <conor.dooley@microchip.com>
Date: Wed, 7 Jun 2023 21:28:28 +0100
Subject: [PATCH 4/7] RISC-V: rework comments in ISA string parser
I have found these comments to not be at all helpful whenever I look at
the parser. Further, the comments in the default case (single letter
parser) are not quite right either.
Group the comments into a larger one at the start of each case, that
attempts to explain things at a higher level.
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
Link: https://lore.kernel.org/r/20230607-headpiece-tannery-83ed5cc4856a@spud
Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
---
arch/riscv/kernel/cpufeature.c | 70 ++++++++++++++++++++++++++++------
1 file changed, 59 insertions(+), 11 deletions(-)
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index c3851c8cfa9c9..7dd4589e79a4b 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -164,7 +164,7 @@ void __init riscv_fill_hwcap(void)
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
@@ -181,53 +181,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;
}
+
+ /*
+ * The parser expects that at the start of an iteration isa points to the
+ * character before the start of the next extension. This will not be the
+ * case if we have just parsed a single-letter extension and the next
+ * extension is not a multi-letter extension prefixed with an "_". It is
+ * also not the case at the end of the string, where it will point to the
+ * terminating null character.
+ */
if (*isa != '_')
--isa;
From 7816ebc1ddd16b5cc95febb75f778bf88411a365 Mon Sep 17 00:00:00 2001
From: Conor Dooley <conor.dooley@microchip.com>
Date: Wed, 7 Jun 2023 21:28:29 +0100
Subject: [PATCH 5/7] RISC-V: remove decrement/increment dance in ISA string
parser
While expanding on the comments in the ISA string parsing code, I
noticed that the conditional decrement of `isa` at the end of the loop
was a bit odd.
The parsing code expects that at the start of the for loop, `isa` will
point to the first character of the next unparsed extension.
However, depending on what the next extension is, this may not be true.
Unless the next extension is a multi-letter extension preceded by an
underscore, `isa` will either point to the string's null-terminator or
to the first character of the next extension, once the switch statement
has been evaluated.
Obviously incrementing `isa` at the end of the loop could cause it to
increment past the null terminator or miss a single letter extension, so
`isa` is conditionally decremented, just so that the loop can increment
it again.
It's easier to understand the code if, instead of this decrement +
increment dance, we instead use a while loop & rely on the handling of
individual extension types to leave `isa` pointing to the first
character of the next extension.
As already mentioned, this won't be the case where the following
extension is multi-letter & preceded by an underscore. To handle that,
invert the check and increment rather than decrement.
Hopefully this eliminates a "huh?!?" moment the next time somebody tries
to understand this code.
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
Reviewed-by: Sunil V L <sunilvl@ventanamicro.com>
Link: https://lore.kernel.org/r/20230607-estate-left-f20faabefb89@spud
Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
---
arch/riscv/kernel/cpufeature.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 7dd4589e79a4b..84dc44a3e6e5c 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -157,7 +157,7 @@ void __init riscv_fill_hwcap(void)
isa += 4;
bitmap_zero(this_isa, RISCV_ISA_EXT_MAX);
- for (; *isa; ++isa) {
+ while (*isa) {
const char *ext = isa++;
const char *ext_end = isa;
bool ext_long = false, ext_err = false;
@@ -270,14 +270,12 @@ void __init riscv_fill_hwcap(void)
/*
* The parser expects that at the start of an iteration isa points to the
- * character before the start of the next extension. This will not be the
- * case if we have just parsed a single-letter extension and the next
- * extension is not a multi-letter extension prefixed with an "_". It is
- * also not the case at the end of the string, where it will point to the
- * terminating null character.
+ * 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 (*isa == '_')
+ ++isa;
#define SET_ISA_EXT_MAP(name, bit) \
do { \
From 1e5cae98e46d15f4dc7c675e1bd0ed2172ea181c Mon Sep 17 00:00:00 2001
From: Conor Dooley <conor.dooley@microchip.com>
Date: Wed, 7 Jun 2023 21:28:30 +0100
Subject: [PATCH 6/7] dt-bindings: riscv: explicitly mention assumption of
Zicntr & Zihpm support
Similar to commit 41ebfc91f785 ("dt-bindings: riscv: explicitly mention
assumption of Zicsr & Zifencei support"), the Zicntr and Zihpm
extensions also used to be part of the base ISA but were removed after
the bindings were merged. Document the assumption of their presence in
the base ISA.
Suggested-by: Palmer Dabbelt <palmer@rivosinc.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
Acked-by: Rob Herring <robh@kernel.org>
Link: https://lore.kernel.org/r/20230607-rerun-retinal-5e8ba89e98f1@spud
Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
---
Documentation/devicetree/bindings/riscv/cpus.yaml | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml
index db5253a2a74ab..d5208881a1fb8 100644
--- 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
From 07edc32779e3dfe164970fc254291258277219c9 Mon Sep 17 00:00:00 2001
From: Conor Dooley <conor.dooley@microchip.com>
Date: Wed, 7 Jun 2023 21:28:31 +0100
Subject: [PATCH 7/7] RISC-V: always report presence of extensions formerly
part of the base ISA
Of these four extensions, two were part of the base ISA when the port was
written and are required by the kernel. The other two are implied when
`i` is in riscv,isa on DT systems.
There's not much that userspace can do with this extra information, but
there is no harm in reporting an ISA string that closer resembles the
current versions of the specifications either.
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
Link: https://lore.kernel.org/r/20230607-nest-collision-5796b6be8be6@spud
Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
---
arch/riscv/include/asm/hwcap.h | 4 ++++
arch/riscv/kernel/cpu.c | 4 ++++
arch/riscv/kernel/cpufeature.c | 17 +++++++++++++++++
3 files changed, 25 insertions(+)
diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index e0c40a4c63d51..e0eb9ad068052 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -46,6 +46,10 @@
#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_MAX 64
#define RISCV_ISA_EXT_NAME_LEN_MAX 32
diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index dfb4a2a61050e..6aea6412cf658 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -208,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(zbb, RISCV_ISA_EXT_ZBB),
__RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
__RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 84dc44a3e6e5c..d21f7e8a33efa 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -311,6 +311,23 @@ void __init riscv_fill_hwcap(void)
#undef SET_ISA_EXT_MAP
}
+ /*
+ * Linux requires the following extensions, so we may as well
+ * always set them.
+ */
+ set_bit(RISCV_ISA_EXT_ZICSR, this_isa);
+ set_bit(RISCV_ISA_EXT_ZIFENCEI, this_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, this_isa);
+ set_bit(RISCV_ISA_EXT_ZIHPM, this_isa);
+ }
+
/*
* All "okay" hart should have same isa. Set HWCAP based on
* common capabilities of every "okay" hart, in case they don't