[PATCH v4 00/11] Add support for a few Zc* extensions as well as Zcmop

[PATCH v4 00/11] Add support for a few Zc* extensions as well as Zcmop

[Clément Léger]

Add support for (yet again) more RVA23U64 missing extensions. Add
support for Zcmop, Zca, Zcf, Zcd and Zcb extensions isa string parsing,
hwprobe and kvm support. Zce, Zcmt and Zcmp extensions have been left
out since they target microcontrollers/embedded CPUs and are not needed
by RVA23U64.

Since Zc* extensions states that C implies Zca, Zcf (if F and RV32), Zcd
(if D), this series modifies the way ISA string is parsed and now does
it in two phases. First one parses the string and the second one
validates it for the final ISA description.

This series is based on the Zimop one [1]. An additional fix [2] should
be applied to correctly test that series.

Link: https://lore.kernel.org/linux-riscv/20240404103254.1752834-1-cleger@rivosinc.com/ [1]
Link: https://lore.kernel.org/all/20240409143839.558784-1-cleger@rivosinc.com/ [2]

---

v4:
 - Modify validate() callbacks to return an 0, -EPROBEDEFER or another
   error.
 - v3: https://lore.kernel.org/all/20240423124326.2532796-1-cleger@rivosinc.com/

v3:
 - Fix typo "exists" -> "exist"
 - Remove C implies Zca, Zcd, Zcf, dt-bindings rules
 - Rework ISA string resolver to handle dependencies
 - v2: https://lore.kernel.org/all/20240418124300.1387978-1-cleger@rivosinc.com/

v2:
 - Add Zc* dependencies validation in dt-bindings
 - v1: https://lore.kernel.org/lkml/20240410091106.749233-1-cleger@rivosinc.com/

Clément Léger (11):
  dt-bindings: riscv: add Zca, Zcf, Zcd and Zcb ISA extension
    description
  riscv: add ISA extensions validation
  riscv: add ISA parsing for Zca, Zcf, Zcd and Zcb
  riscv: hwprobe: export Zca, Zcf, Zcd and Zcb ISA extensions
  RISC-V: KVM: Allow Zca, Zcf, Zcd and Zcb extensions for Guest/VM
  KVM: riscv: selftests: Add some Zc* extensions to get-reg-list test
  dt-bindings: riscv: add Zcmop ISA extension description
  riscv: add ISA extension parsing for Zcmop
  riscv: hwprobe: export Zcmop ISA extension
  RISC-V: KVM: Allow Zcmop extension for Guest/VM
  KVM: riscv: selftests: Add Zcmop extension to get-reg-list test

 Documentation/arch/riscv/hwprobe.rst          |  24 ++
 .../devicetree/bindings/riscv/extensions.yaml |  90 ++++++
 arch/riscv/include/asm/cpufeature.h           |   1 +
 arch/riscv/include/asm/hwcap.h                |   5 +
 arch/riscv/include/uapi/asm/hwprobe.h         |   5 +
 arch/riscv/include/uapi/asm/kvm.h             |   5 +
 arch/riscv/kernel/cpufeature.c                | 259 ++++++++++++------
 arch/riscv/kernel/sys_hwprobe.c               |   5 +
 arch/riscv/kvm/vcpu_onereg.c                  |  10 +
 .../selftests/kvm/riscv/get-reg-list.c        |  20 ++
 10 files changed, 337 insertions(+), 87 deletions(-)

-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v4 01/11] dt-bindings: riscv: add Zca, Zcf, Zcd and Zcb ISA extension description

[Clément Léger]

Add description for Zca, Zcf, Zcd and Zcb extensions which are part the
Zc* standard extensions for code size reduction. Additional validation
rules are added since Zcb depends on Zca, Zcf, depends on Zca and F, Zcd
depends on Zca and D and finally, Zcf can not be present on rv64.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../devicetree/bindings/riscv/extensions.yaml | 78 +++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
index 616370318a66..81bce4fa2424 100644
--- a/Documentation/devicetree/bindings/riscv/extensions.yaml
+++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
@@ -220,6 +220,38 @@ properties:
             instructions as ratified at commit 6d33919 ("Merge pull request #158
             from hirooih/clmul-fix-loop-end-condition") of riscv-bitmanip.
 
+        - const: zca
+          description: |
+            The Zca extension part of Zc* standard extensions for code size
+            reduction, as ratified in commit 8be3419c1c0 ("Zcf doesn't exist on
+            RV64 as it contains no instructions") of riscv-code-size-reduction,
+            merged in the riscv-isa-manual by commit dbc79cf28a2 ("Initial seed
+            of zc.adoc to src tree.").
+
+        - const: zcb
+          description: |
+            The Zcb extension part of Zc* standard extensions for code size
+            reduction, as ratified in commit 8be3419c1c0 ("Zcf doesn't exist on
+            RV64 as it contains no instructions") of riscv-code-size-reduction,
+            merged in the riscv-isa-manual by commit dbc79cf28a2 ("Initial seed
+            of zc.adoc to src tree.").
+
+        - const: zcd
+          description: |
+            The Zcd extension part of Zc* standard extensions for code size
+            reduction, as ratified in commit 8be3419c1c0 ("Zcf doesn't exist on
+            RV64 as it contains no instructions") of riscv-code-size-reduction,
+            merged in the riscv-isa-manual by commit dbc79cf28a2 ("Initial seed
+            of zc.adoc to src tree.").
+
+        - const: zcf
+          description: |
+            The Zcf extension part of Zc* standard extensions for code size
+            reduction, as ratified in commit 8be3419c1c0 ("Zcf doesn't exist on
+            RV64 as it contains no instructions") of riscv-code-size-reduction,
+            merged in the riscv-isa-manual by commit dbc79cf28a2 ("Initial seed
+            of zc.adoc to src tree.").
+
         - const: zfa
           description:
             The standard Zfa extension for additional floating point
@@ -489,5 +521,51 @@ properties:
             Registers in the AX45MP datasheet.
             https://www.andestech.com/wp-content/uploads/AX45MP-1C-Rev.-5.0.0-Datasheet.pdf
 
+    allOf:
+      # Zcb depends on Zca
+      - if:
+          contains:
+            const: zcb
+        then:
+          contains:
+            const: zca
+      # Zcd depends on Zca and D
+      - if:
+          contains:
+            const: zcd
+        then:
+          allOf:
+            - contains:
+                const: zca
+            - contains:
+                const: d
+      # Zcf depends on Zca and F
+      - if:
+          contains:
+            const: zcf
+        then:
+          allOf:
+            - contains:
+                const: zca
+            - contains:
+                const: f
+
+allOf:
+  # Zcf extension does not exist on rv64
+  - if:
+      properties:
+        riscv,isa-extensions:
+          contains:
+            const: zcf
+        riscv,isa-base:
+          contains:
+            const: rv64i
+    then:
+      properties:
+        riscv,isa-extensions:
+          not:
+            contains:
+              const: zcf
+
 additionalProperties: true
 ...
-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v4 02/11] riscv: add ISA extensions validation

[Clément Léger]

Since a few extensions (Zicbom/Zicboz) already needs validation and
future ones will need it as well (Zc*) add a validate() callback to
struct riscv_isa_ext_data. This require to rework the way extensions are
parsed and split it in two phases. First phase is isa string or isa
extension list parsing and consists in enabling all the extensions in a
temporary bitmask without any validation. The second step "resolves" the
final isa bitmap, handling potential missing dependencies. The mechanism
is quite simple and simply validate each extension described in the
temporary bitmap before enabling it in the final isa bitmap. validate()
callbacks can return either 0 for success, -EPROBEDEFER if extension
needs to be validated again at next loop. A previous ISA bitmap is kept
to avoid looping mutliple times if an extension dependencies are never
satisfied until we reach a stable state. In order to avoid any potential
infinite looping, allow looping a maximum of the number of extension we
handle. Zicboz and Zicbom extensions are modified to use this validation
mechanism.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 arch/riscv/include/asm/cpufeature.h |   1 +
 arch/riscv/kernel/cpufeature.c      | 211 ++++++++++++++++------------
 2 files changed, 126 insertions(+), 86 deletions(-)

diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
index 347805446151..000796c2d0b1 100644
--- a/arch/riscv/include/asm/cpufeature.h
+++ b/arch/riscv/include/asm/cpufeature.h
@@ -70,6 +70,7 @@ struct riscv_isa_ext_data {
 	const char *property;
 	const unsigned int *subset_ext_ids;
 	const unsigned int subset_ext_size;
+	int (*validate)(const struct riscv_isa_ext_data *data, const unsigned long *isa_bitmap);
 };
 
 extern const struct riscv_isa_ext_data riscv_isa_ext[];
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 115ba001f1bc..cb2ffa6c8c33 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -72,51 +72,58 @@ bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, unsigned i
 }
 EXPORT_SYMBOL_GPL(__riscv_isa_extension_available);
 
-static bool riscv_isa_extension_check(int id)
+static bool riscv_isa_extension_valid(int id)
 {
-	switch (id) {
-	case RISCV_ISA_EXT_ZICBOM:
-		if (!riscv_cbom_block_size) {
-			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("Zicbom disabled as cbom-block-size present, but is not a power-of-2\n");
-			return false;
-		}
-		return true;
-	case RISCV_ISA_EXT_ZICBOZ:
-		if (!riscv_cboz_block_size) {
-			pr_err("Zicboz detected in ISA string, disabling as no cboz-block-size found\n");
-			return false;
-		} else if (!is_power_of_2(riscv_cboz_block_size)) {
-			pr_err("Zicboz disabled as cboz-block-size present, but is not a power-of-2\n");
-			return false;
-		}
-		return true;
-	case RISCV_ISA_EXT_INVALID:
-		return false;
+	return id != RISCV_ISA_EXT_INVALID;
+}
+
+static int riscv_ext_zicbom_validate(const struct riscv_isa_ext_data *data,
+				     const unsigned long *isa_bitmap)
+{
+	if (!riscv_cbom_block_size) {
+		pr_err("Zicbom detected in ISA string, disabling as no cbom-block-size found\n");
+		return -EINVAL;
+	} else if (!is_power_of_2(riscv_cbom_block_size)) {
+		pr_err("Zicbom disabled as cbom-block-size present, but is not a power-of-2\n");
+		return -EINVAL;
 	}
+	return 0;
+}
 
-	return true;
+static int riscv_ext_zicboz_validate(const struct riscv_isa_ext_data *data,
+				     const unsigned long *isa_bitmap)
+{
+	if (!riscv_cboz_block_size) {
+		pr_err("Zicboz detected in ISA string, disabling as no cboz-block-size found\n");
+		return -EINVAL;
+	} else if (!is_power_of_2(riscv_cboz_block_size)) {
+		pr_err("Zicboz disabled as cboz-block-size present, but is not a power-of-2\n");
+		return -EINVAL;
+	}
+	return 0;
 }
 
-#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size) {	\
-	.name = #_name,								\
-	.property = #_name,							\
-	.id = _id,								\
-	.subset_ext_ids = _subset_exts,						\
-	.subset_ext_size = _subset_exts_size					\
+#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size, _validate) {	\
+	.name = #_name,									\
+	.property = #_name,								\
+	.id = _id,									\
+	.subset_ext_ids = _subset_exts,							\
+	.subset_ext_size = _subset_exts_size,						\
+	.validate = _validate								\
 }
 
-#define __RISCV_ISA_EXT_DATA(_name, _id) _RISCV_ISA_EXT_DATA(_name, _id, NULL, 0)
+#define __RISCV_ISA_EXT_DATA(_name, _id) _RISCV_ISA_EXT_DATA(_name, _id, NULL, 0, NULL)
 
 /* Used to declare pure "lasso" extension (Zk for instance) */
 #define __RISCV_ISA_EXT_BUNDLE(_name, _bundled_exts) \
-	_RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, _bundled_exts, ARRAY_SIZE(_bundled_exts))
+	_RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, _bundled_exts, \
+			    ARRAY_SIZE(_bundled_exts), NULL)
 
 /* Used to declare extensions that are a superset of other extensions (Zvbb for instance) */
 #define __RISCV_ISA_EXT_SUPERSET(_name, _id, _sub_exts) \
-	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts))
+	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts), NULL)
+#define __RISCV_ISA_EXT_SUPERSET_VALIDATE(_name, _id, _sub_exts, _validate) \
+	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts), _validate)
 
 static const unsigned int riscv_zk_bundled_exts[] = {
 	RISCV_ISA_EXT_ZBKB,
@@ -247,8 +254,10 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
 	__RISCV_ISA_EXT_DATA(c, RISCV_ISA_EXT_c),
 	__RISCV_ISA_EXT_DATA(v, RISCV_ISA_EXT_v),
 	__RISCV_ISA_EXT_DATA(h, RISCV_ISA_EXT_h),
-	__RISCV_ISA_EXT_SUPERSET(zicbom, RISCV_ISA_EXT_ZICBOM, riscv_xlinuxenvcfg_exts),
-	__RISCV_ISA_EXT_SUPERSET(zicboz, RISCV_ISA_EXT_ZICBOZ, riscv_xlinuxenvcfg_exts),
+	__RISCV_ISA_EXT_SUPERSET_VALIDATE(zicbom, RISCV_ISA_EXT_ZICBOM, riscv_xlinuxenvcfg_exts,
+					  riscv_ext_zicbom_validate),
+	__RISCV_ISA_EXT_SUPERSET_VALIDATE(zicboz, RISCV_ISA_EXT_ZICBOZ, riscv_xlinuxenvcfg_exts,
+					  riscv_ext_zicboz_validate),
 	__RISCV_ISA_EXT_DATA(zicntr, RISCV_ISA_EXT_ZICNTR),
 	__RISCV_ISA_EXT_DATA(zicond, RISCV_ISA_EXT_ZICOND),
 	__RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR),
@@ -310,33 +319,80 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
 
 const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext);
 
-static void __init match_isa_ext(const struct riscv_isa_ext_data *ext, const char *name,
-				 const char *name_end, struct riscv_isainfo *isainfo)
+static void riscv_isa_set_ext(const struct riscv_isa_ext_data *ext, unsigned long *bitmap)
 {
-	if ((name_end - name == strlen(ext->name)) &&
-	     !strncasecmp(name, ext->name, name_end - name)) {
-		/*
-		 * If this is a bundle, enable all the ISA extensions that
-		 * comprise the bundle.
-		 */
-		if (ext->subset_ext_size) {
-			for (int i = 0; i < ext->subset_ext_size; i++) {
-				if (riscv_isa_extension_check(ext->subset_ext_ids[i]))
-					set_bit(ext->subset_ext_ids[i], isainfo->isa);
-			}
+	/*
+	 * This is valid even for bundle extensions which uses the RISCV_ISA_EXT_INVALID id
+	 * (rejected by riscv_isa_extension_valid()).
+	 */
+	if (riscv_isa_extension_valid(ext->id))
+		set_bit(ext->id, bitmap);
+
+	for (int i = 0; i < ext->subset_ext_size; i++) {
+		if (riscv_isa_extension_valid(ext->subset_ext_ids[i]))
+			set_bit(ext->subset_ext_ids[i], bitmap);
+	}
+}
+
+static void __init riscv_resolve_isa(unsigned long *isa_bitmap, struct riscv_isainfo *isainfo,
+				     unsigned long *this_hwcap, unsigned long *isa2hwcap)
+{
+	bool loop;
+	const struct riscv_isa_ext_data *ext;
+	DECLARE_BITMAP(prev_bitmap, RISCV_ISA_EXT_MAX);
+	int max_loop_count = riscv_isa_ext_count, ret;
+
+	do {
+		loop = false;
+		if (max_loop_count-- < 0) {
+			pr_err("Failed to reach a stable ISA state\n");
+			return;
 		}
+		memcpy(prev_bitmap, isainfo->isa, sizeof(prev_bitmap));
+		for (int i = 0; i < riscv_isa_ext_count; i++) {
+			ext = &riscv_isa_ext[i];
+
+			/* Bundle extensions ids are invalid*/
+			if (!riscv_isa_extension_valid(ext->id))
+				continue;
+
+			if (!test_bit(ext->id, isa_bitmap) || test_bit(ext->id, isainfo->isa))
+				continue;
+
+			if (ext->validate) {
+				ret = ext->validate(ext, isainfo->isa);
+				if (ret) {
+					if (ret == -EPROBE_DEFER)
+						loop = true;
+					else
+						clear_bit(ext->id, isa_bitmap);
+					continue;
+				}
+			}
 
-		/*
-		 * This is valid even for bundle extensions which uses the RISCV_ISA_EXT_INVALID id
-		 * (rejected by riscv_isa_extension_check()).
-		 */
-		if (riscv_isa_extension_check(ext->id))
 			set_bit(ext->id, isainfo->isa);
+
+			/* Only single letter extensions get set in hwcap */
+			if (ext->id < RISCV_ISA_EXT_BASE)
+				*this_hwcap |= isa2hwcap[ext->id];
+		}
+	} while (loop && memcmp(prev_bitmap, isainfo->isa, sizeof(prev_bitmap)));
+}
+
+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)) {
+			riscv_isa_set_ext(ext, bitmap);
+			break;
+		}
 	}
 }
 
-static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct riscv_isainfo *isainfo,
-					  unsigned long *isa2hwcap, const char *isa)
+static void __init riscv_resolve_isa_string(const char *isa, unsigned long *bitmap)
 {
 	/*
 	 * For all possible cpus, we have already validated in
@@ -349,7 +405,7 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc
 	while (*isa) {
 		const char *ext = isa++;
 		const char *ext_end = isa;
-		bool ext_long = false, ext_err = false;
+		bool ext_err = false;
 
 		switch (*ext) {
 		case 's':
@@ -389,7 +445,6 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc
 			 * character itself while eliminating the extensions version number.
 			 * A simple re-increment solves this problem.
 			 */
-			ext_long = true;
 			for (; *isa && *isa != '_'; ++isa)
 				if (unlikely(!isalnum(*isa)))
 					ext_err = true;
@@ -469,17 +524,8 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc
 
 		if (unlikely(ext_err))
 			continue;
-		if (!ext_long) {
-			int nr = tolower(*ext) - 'a';
 
-			if (riscv_isa_extension_check(nr)) {
-				*this_hwcap |= isa2hwcap[nr];
-				set_bit(nr, isainfo->isa);
-			}
-		} else {
-			for (int i = 0; i < riscv_isa_ext_count; i++)
-				match_isa_ext(&riscv_isa_ext[i], ext, ext_end, isainfo);
-		}
+		match_isa_ext(ext, ext_end, bitmap);
 	}
 }
 
@@ -501,6 +547,7 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
 	for_each_possible_cpu(cpu) {
 		struct riscv_isainfo *isainfo = &hart_isa[cpu];
 		unsigned long this_hwcap = 0;
+		DECLARE_BITMAP(isa_bitmap, RISCV_ISA_EXT_MAX) = { 0 };
 
 		if (acpi_disabled) {
 			node = of_cpu_device_node_get(cpu);
@@ -523,7 +570,7 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
 			}
 		}
 
-		riscv_parse_isa_string(&this_hwcap, isainfo, isa2hwcap, isa);
+		riscv_resolve_isa_string(isa, isa_bitmap);
 
 		/*
 		 * These ones were as they were part of the base ISA when the
@@ -531,10 +578,10 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
 		 * unconditionally where `i` is in riscv,isa on DT systems.
 		 */
 		if (acpi_disabled) {
-			set_bit(RISCV_ISA_EXT_ZICSR, isainfo->isa);
-			set_bit(RISCV_ISA_EXT_ZIFENCEI, isainfo->isa);
-			set_bit(RISCV_ISA_EXT_ZICNTR, isainfo->isa);
-			set_bit(RISCV_ISA_EXT_ZIHPM, isainfo->isa);
+			set_bit(RISCV_ISA_EXT_ZICSR, isa_bitmap);
+			set_bit(RISCV_ISA_EXT_ZIFENCEI, isa_bitmap);
+			set_bit(RISCV_ISA_EXT_ZICNTR, isa_bitmap);
+			set_bit(RISCV_ISA_EXT_ZIHPM, isa_bitmap);
 		}
 
 		/*
@@ -548,9 +595,11 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
 		if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID &&
 		    riscv_cached_marchid(cpu) == 0x0) {
 			this_hwcap &= ~isa2hwcap[RISCV_ISA_EXT_v];
-			clear_bit(RISCV_ISA_EXT_v, isainfo->isa);
+			clear_bit(RISCV_ISA_EXT_v, isa_bitmap);
 		}
 
+		riscv_resolve_isa(isa_bitmap, isainfo, &this_hwcap, isa2hwcap);
+
 		/*
 		 * All "okay" hart should have same isa. Set HWCAP based on
 		 * common capabilities of every "okay" hart, in case they don't
@@ -579,6 +628,7 @@ static int __init riscv_fill_hwcap_from_ext_list(unsigned long *isa2hwcap)
 		unsigned long this_hwcap = 0;
 		struct device_node *cpu_node;
 		struct riscv_isainfo *isainfo = &hart_isa[cpu];
+		DECLARE_BITMAP(isa_bitmap, RISCV_ISA_EXT_MAX) = { 0 };
 
 		cpu_node = of_cpu_device_node_get(cpu);
 		if (!cpu_node) {
@@ -598,22 +648,11 @@ static int __init riscv_fill_hwcap_from_ext_list(unsigned long *isa2hwcap)
 						     ext->property) < 0)
 				continue;
 
-			if (ext->subset_ext_size) {
-				for (int j = 0; j < ext->subset_ext_size; j++) {
-					if (riscv_isa_extension_check(ext->subset_ext_ids[i]))
-						set_bit(ext->subset_ext_ids[j], isainfo->isa);
-				}
-			}
-
-			if (riscv_isa_extension_check(ext->id)) {
-				set_bit(ext->id, isainfo->isa);
-
-				/* Only single letter extensions get set in hwcap */
-				if (strnlen(riscv_isa_ext[i].name, 2) == 1)
-					this_hwcap |= isa2hwcap[riscv_isa_ext[i].id];
-			}
+			riscv_isa_set_ext(ext, isa_bitmap);
 		}
 
+		riscv_resolve_isa(isa_bitmap, isainfo, &this_hwcap, isa2hwcap);
+
 		of_node_put(cpu_node);
 
 		/*
-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v4 03/11] riscv: add ISA parsing for Zca, Zcf, Zcd and Zcb

[Clément Léger]

The Zc* standard extension for code reduction introduces new extensions.
This patch adds support for Zca, Zcf, Zcd and Zcb. Zce, Zcmt and Zcmp
are left out of this patch since they are targeting microcontrollers/
embedded CPUs instead of application processors.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 arch/riscv/include/asm/hwcap.h |  4 +++
 arch/riscv/kernel/cpufeature.c | 47 +++++++++++++++++++++++++++++++++-
 2 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index 543e3ea2da0e..b7551bad341b 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -82,6 +82,10 @@
 #define RISCV_ISA_EXT_ZACAS		73
 #define RISCV_ISA_EXT_XANDESPMU		74
 #define RISCV_ISA_EXT_ZIMOP		75
+#define RISCV_ISA_EXT_ZCA		76
+#define RISCV_ISA_EXT_ZCB		77
+#define RISCV_ISA_EXT_ZCD		78
+#define RISCV_ISA_EXT_ZCF		79
 
 #define RISCV_ISA_EXT_XLINUXENVCFG	127
 
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index cb2ffa6c8c33..c74bdb9c0a9f 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -103,6 +103,29 @@ static int riscv_ext_zicboz_validate(const struct riscv_isa_ext_data *data,
 	return 0;
 }
 
+static int riscv_ext_zca_depends(const struct riscv_isa_ext_data *data,
+				 const unsigned long *isa_bitmap)
+{
+	return __riscv_isa_extension_available(isa_bitmap, RISCV_ISA_EXT_ZCA) ? 0 : -EPROBE_DEFER;
+}
+static int riscv_ext_zcd_validate(const struct riscv_isa_ext_data *data,
+				  const unsigned long *isa_bitmap)
+{
+	return __riscv_isa_extension_available(isa_bitmap, RISCV_ISA_EXT_ZCA) &&
+	       __riscv_isa_extension_available(isa_bitmap, RISCV_ISA_EXT_d) ? 0 : -EPROBE_DEFER;
+}
+
+static int riscv_ext_zcf_validate(const struct riscv_isa_ext_data *data,
+				  const unsigned long *isa_bitmap)
+{
+#ifdef CONFIG_64BIT
+	return -EINVAL;
+#else
+	return __riscv_isa_extension_available(isa_bitmap, RISCV_ISA_EXT_ZCA) &&
+	       __riscv_isa_extension_available(isa_bitmap, RISCV_ISA_EXT_f) ? 0 : -EPROBE_DEFER;
+#endif
+}
+
 #define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size, _validate) {	\
 	.name = #_name,									\
 	.property = #_name,								\
@@ -114,6 +137,9 @@ static int riscv_ext_zicboz_validate(const struct riscv_isa_ext_data *data,
 
 #define __RISCV_ISA_EXT_DATA(_name, _id) _RISCV_ISA_EXT_DATA(_name, _id, NULL, 0, NULL)
 
+#define __RISCV_ISA_EXT_DATA_VALIDATE(_name, _id, _validate) \
+			_RISCV_ISA_EXT_DATA(_name, _id, NULL, 0, _validate)
+
 /* Used to declare pure "lasso" extension (Zk for instance) */
 #define __RISCV_ISA_EXT_BUNDLE(_name, _bundled_exts) \
 	_RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, _bundled_exts, \
@@ -205,6 +231,21 @@ static const unsigned int riscv_xlinuxenvcfg_exts[] = {
 	RISCV_ISA_EXT_XLINUXENVCFG
 };
 
+/*
+ * Zc* spec states that:
+ * - C always implies Zca
+ * - C+F implies Zcf (RV32 only)
+ * - C+D implies Zcd
+ *
+ * These extensions will be enabled and then validated depending on the
+ * availability of F/D RV32.
+ */
+static const unsigned int riscv_c_exts[] = {
+	RISCV_ISA_EXT_ZCA,
+	RISCV_ISA_EXT_ZCF,
+	RISCV_ISA_EXT_ZCD,
+};
+
 /*
  * The canonical order of ISA extension names in the ISA string is defined in
  * chapter 27 of the unprivileged specification.
@@ -251,7 +292,7 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
 	__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(c, RISCV_ISA_EXT_c),
+	__RISCV_ISA_EXT_SUPERSET(c, RISCV_ISA_EXT_c, riscv_c_exts),
 	__RISCV_ISA_EXT_DATA(v, RISCV_ISA_EXT_v),
 	__RISCV_ISA_EXT_DATA(h, RISCV_ISA_EXT_h),
 	__RISCV_ISA_EXT_SUPERSET_VALIDATE(zicbom, RISCV_ISA_EXT_ZICBOM, riscv_xlinuxenvcfg_exts,
@@ -270,6 +311,10 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
 	__RISCV_ISA_EXT_DATA(zfa, RISCV_ISA_EXT_ZFA),
 	__RISCV_ISA_EXT_DATA(zfh, RISCV_ISA_EXT_ZFH),
 	__RISCV_ISA_EXT_DATA(zfhmin, RISCV_ISA_EXT_ZFHMIN),
+	__RISCV_ISA_EXT_DATA(zca, RISCV_ISA_EXT_ZCA),
+	__RISCV_ISA_EXT_DATA_VALIDATE(zcb, RISCV_ISA_EXT_ZCB, riscv_ext_zca_depends),
+	__RISCV_ISA_EXT_DATA_VALIDATE(zcd, RISCV_ISA_EXT_ZCD, riscv_ext_zcd_validate),
+	__RISCV_ISA_EXT_DATA_VALIDATE(zcf, RISCV_ISA_EXT_ZCF, riscv_ext_zcf_validate),
 	__RISCV_ISA_EXT_DATA(zba, RISCV_ISA_EXT_ZBA),
 	__RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
 	__RISCV_ISA_EXT_DATA(zbc, RISCV_ISA_EXT_ZBC),
-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v4 04/11] riscv: hwprobe: export Zca, Zcf, Zcd and Zcb ISA extensions

[Clément Léger]

Export Zca, Zcf, Zcd and Zcb ISA extension through hwprobe.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 Documentation/arch/riscv/hwprobe.rst  | 20 ++++++++++++++++++++
 arch/riscv/include/uapi/asm/hwprobe.h |  4 ++++
 arch/riscv/kernel/sys_hwprobe.c       |  4 ++++
 3 files changed, 28 insertions(+)

diff --git a/Documentation/arch/riscv/hwprobe.rst b/Documentation/arch/riscv/hwprobe.rst
index 9ca5b093b6d5..bf96b4e8ba3b 100644
--- a/Documentation/arch/riscv/hwprobe.rst
+++ b/Documentation/arch/riscv/hwprobe.rst
@@ -192,6 +192,26 @@ The following keys are defined:
        supported as defined in the RISC-V ISA manual starting from commit
        58220614a5f ("Zimop is ratified/1.0").
 
+  * :c:macro:`RISCV_HWPROBE_EXT_ZCA`: The Zca extension part of Zc* standard
+       extensions for code size reduction, as ratified in commit 8be3419c1c0
+       ("Zcf doesn't exist on RV64 as it contains no instructions") of
+       riscv-code-size-reduction.
+
+  * :c:macro:`RISCV_HWPROBE_EXT_ZCB`: The Zcb extension part of Zc* standard
+       extensions for code size reduction, as ratified in commit 8be3419c1c0
+       ("Zcf doesn't exist on RV64 as it contains no instructions") of
+       riscv-code-size-reduction.
+
+  * :c:macro:`RISCV_HWPROBE_EXT_ZCD`: The Zcd extension part of Zc* standard
+       extensions for code size reduction, as ratified in commit 8be3419c1c0
+       ("Zcf doesn't exist on RV64 as it contains no instructions") of
+       riscv-code-size-reduction.
+
+  * :c:macro:`RISCV_HWPROBE_EXT_ZCF`: The Zcf extension part of Zc* standard
+       extensions for code size reduction, as ratified in commit 8be3419c1c0
+       ("Zcf doesn't exist on RV64 as it contains no instructions") of
+       riscv-code-size-reduction.
+
 * :c:macro:`RISCV_HWPROBE_KEY_CPUPERF_0`: A bitmask that contains performance
   information about the selected set of processors.
 
diff --git a/arch/riscv/include/uapi/asm/hwprobe.h b/arch/riscv/include/uapi/asm/hwprobe.h
index ac6874ab743a..dd4ad77faf49 100644
--- a/arch/riscv/include/uapi/asm/hwprobe.h
+++ b/arch/riscv/include/uapi/asm/hwprobe.h
@@ -60,6 +60,10 @@ struct riscv_hwprobe {
 #define		RISCV_HWPROBE_EXT_ZACAS		(1ULL << 34)
 #define		RISCV_HWPROBE_EXT_ZICOND	(1ULL << 35)
 #define		RISCV_HWPROBE_EXT_ZIMOP		(1ULL << 36)
+#define		RISCV_HWPROBE_EXT_ZCA		(1ULL << 37)
+#define		RISCV_HWPROBE_EXT_ZCB		(1ULL << 38)
+#define		RISCV_HWPROBE_EXT_ZCD		(1ULL << 39)
+#define		RISCV_HWPROBE_EXT_ZCF		(1ULL << 40)
 #define RISCV_HWPROBE_KEY_CPUPERF_0	5
 #define		RISCV_HWPROBE_MISALIGNED_UNKNOWN	(0 << 0)
 #define		RISCV_HWPROBE_MISALIGNED_EMULATED	(1 << 0)
diff --git a/arch/riscv/kernel/sys_hwprobe.c b/arch/riscv/kernel/sys_hwprobe.c
index c99a4cf231c5..2ffa0fe5101e 100644
--- a/arch/riscv/kernel/sys_hwprobe.c
+++ b/arch/riscv/kernel/sys_hwprobe.c
@@ -112,6 +112,8 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
 		EXT_KEY(ZACAS);
 		EXT_KEY(ZICOND);
 		EXT_KEY(ZIMOP);
+		EXT_KEY(ZCA);
+		EXT_KEY(ZCB);
 
 		if (has_vector()) {
 			EXT_KEY(ZVBB);
@@ -132,6 +134,8 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
 			EXT_KEY(ZFH);
 			EXT_KEY(ZFHMIN);
 			EXT_KEY(ZFA);
+			EXT_KEY(ZCD);
+			EXT_KEY(ZCF);
 		}
 #undef EXT_KEY
 	}
-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v4 05/11] RISC-V: KVM: Allow Zca, Zcf, Zcd and Zcb extensions for Guest/VM

[Clément Léger]

Extend the KVM ISA extension ONE_REG interface to allow KVM user space
to detect and enable Zca, Zcf, Zcd and Zcb extensions for Guest/VM.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
Acked-by: Anup Patel <anup@brainfault.org>
---
 arch/riscv/include/uapi/asm/kvm.h | 4 ++++
 arch/riscv/kvm/vcpu_onereg.c      | 8 ++++++++
 2 files changed, 12 insertions(+)

diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
index 35a12aa1953e..57db3fea679f 100644
--- a/arch/riscv/include/uapi/asm/kvm.h
+++ b/arch/riscv/include/uapi/asm/kvm.h
@@ -168,6 +168,10 @@ enum KVM_RISCV_ISA_EXT_ID {
 	KVM_RISCV_ISA_EXT_ZTSO,
 	KVM_RISCV_ISA_EXT_ZACAS,
 	KVM_RISCV_ISA_EXT_ZIMOP,
+	KVM_RISCV_ISA_EXT_ZCA,
+	KVM_RISCV_ISA_EXT_ZCB,
+	KVM_RISCV_ISA_EXT_ZCD,
+	KVM_RISCV_ISA_EXT_ZCF,
 	KVM_RISCV_ISA_EXT_MAX,
 };
 
diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c
index 12436f6f0d20..a2747a6dbdb6 100644
--- a/arch/riscv/kvm/vcpu_onereg.c
+++ b/arch/riscv/kvm/vcpu_onereg.c
@@ -48,6 +48,10 @@ static const unsigned long kvm_isa_ext_arr[] = {
 	KVM_ISA_EXT_ARR(ZBKC),
 	KVM_ISA_EXT_ARR(ZBKX),
 	KVM_ISA_EXT_ARR(ZBS),
+	KVM_ISA_EXT_ARR(ZCA),
+	KVM_ISA_EXT_ARR(ZCB),
+	KVM_ISA_EXT_ARR(ZCD),
+	KVM_ISA_EXT_ARR(ZCF),
 	KVM_ISA_EXT_ARR(ZFA),
 	KVM_ISA_EXT_ARR(ZFH),
 	KVM_ISA_EXT_ARR(ZFHMIN),
@@ -128,6 +132,10 @@ static bool kvm_riscv_vcpu_isa_disable_allowed(unsigned long ext)
 	case KVM_RISCV_ISA_EXT_ZBKC:
 	case KVM_RISCV_ISA_EXT_ZBKX:
 	case KVM_RISCV_ISA_EXT_ZBS:
+	case KVM_RISCV_ISA_EXT_ZCA:
+	case KVM_RISCV_ISA_EXT_ZCB:
+	case KVM_RISCV_ISA_EXT_ZCD:
+	case KVM_RISCV_ISA_EXT_ZCF:
 	case KVM_RISCV_ISA_EXT_ZFA:
 	case KVM_RISCV_ISA_EXT_ZFH:
 	case KVM_RISCV_ISA_EXT_ZFHMIN:
-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v4 06/11] KVM: riscv: selftests: Add some Zc* extensions to get-reg-list test

[Clément Léger]

The KVM RISC-V allows Zca, Zcf, Zcd and Zcb extensions for Guest/VM so
add these extensions to get-reg-list test.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
Acked-by: Anup Patel <anup@brainfault.org>
---
 tools/testing/selftests/kvm/riscv/get-reg-list.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c
index 40107bb61975..61cad4514197 100644
--- a/tools/testing/selftests/kvm/riscv/get-reg-list.c
+++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c
@@ -55,6 +55,10 @@ bool filter_reg(__u64 reg)
 	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_ZBKC:
 	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_ZBKX:
 	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_ZBS:
+	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_ZCA:
+	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_ZCB:
+	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_ZCD:
+	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_ZCF:
 	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_ZFA:
 	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_ZFH:
 	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_ZFHMIN:
@@ -421,6 +425,10 @@ static const char *isa_ext_single_id_to_str(__u64 reg_off)
 		KVM_ISA_EXT_ARR(ZBKC),
 		KVM_ISA_EXT_ARR(ZBKX),
 		KVM_ISA_EXT_ARR(ZBS),
+		KVM_ISA_EXT_ARR(ZCA),
+		KVM_ISA_EXT_ARR(ZCB),
+		KVM_ISA_EXT_ARR(ZCD),
+		KVM_ISA_EXT_ARR(ZCF),
 		KVM_ISA_EXT_ARR(ZFA),
 		KVM_ISA_EXT_ARR(ZFH),
 		KVM_ISA_EXT_ARR(ZFHMIN),
@@ -945,6 +953,10 @@ KVM_ISA_EXT_SIMPLE_CONFIG(zbkb, ZBKB);
 KVM_ISA_EXT_SIMPLE_CONFIG(zbkc, ZBKC);
 KVM_ISA_EXT_SIMPLE_CONFIG(zbkx, ZBKX);
 KVM_ISA_EXT_SIMPLE_CONFIG(zbs, ZBS);
+KVM_ISA_EXT_SIMPLE_CONFIG(zca, ZCA),
+KVM_ISA_EXT_SIMPLE_CONFIG(zcb, ZCB),
+KVM_ISA_EXT_SIMPLE_CONFIG(zcd, ZCD),
+KVM_ISA_EXT_SIMPLE_CONFIG(zcf, ZCF),
 KVM_ISA_EXT_SIMPLE_CONFIG(zfa, ZFA);
 KVM_ISA_EXT_SIMPLE_CONFIG(zfh, ZFH);
 KVM_ISA_EXT_SIMPLE_CONFIG(zfhmin, ZFHMIN);
@@ -1001,6 +1013,10 @@ struct vcpu_reg_list *vcpu_configs[] = {
 	&config_zbkc,
 	&config_zbkx,
 	&config_zbs,
+	&config_zca,
+	&config_zcb,
+	&config_zcd,
+	&config_zcf,
 	&config_zfa,
 	&config_zfh,
 	&config_zfhmin,
-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v4 07/11] dt-bindings: riscv: add Zcmop ISA extension description

[Clément Léger]

Add description for the Zcmop (Compressed May-Be-Operations) ISA
extension which was ratified in commit c732a4f39a4 ("Zcmop is
ratified/1.0") of the riscv-isa-manual.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../devicetree/bindings/riscv/extensions.yaml        | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
index 81bce4fa2424..1952d20b8996 100644
--- a/Documentation/devicetree/bindings/riscv/extensions.yaml
+++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
@@ -252,6 +252,11 @@ properties:
             merged in the riscv-isa-manual by commit dbc79cf28a2 ("Initial seed
             of zc.adoc to src tree.").
 
+        - const: zcmop
+          description:
+            The standard Zcmop extension version 1.0, as ratified in commit
+            c732a4f39a4 ("Zcmop is ratified/1.0") of the riscv-isa-manual.
+
         - const: zfa
           description:
             The standard Zfa extension for additional floating point
@@ -549,6 +554,13 @@ properties:
                 const: zca
             - contains:
                 const: f
+      # Zcmop depends on Zca
+      - if:
+          contains:
+            const: zcmop
+        then:
+          contains:
+            const: zca
 
 allOf:
   # Zcf extension does not exist on rv64
-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v4 08/11] riscv: add ISA extension parsing for Zcmop

[Clément Léger]

Add parsing for Zcmop ISA extension which was ratified in commit
b854a709c00 ("Zcmop is ratified/1.0") of the riscv-isa-manual.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 arch/riscv/include/asm/hwcap.h | 1 +
 arch/riscv/kernel/cpufeature.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index b7551bad341b..cff7660de268 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -86,6 +86,7 @@
 #define RISCV_ISA_EXT_ZCB		77
 #define RISCV_ISA_EXT_ZCD		78
 #define RISCV_ISA_EXT_ZCF		79
+#define RISCV_ISA_EXT_ZCMOP		80
 
 #define RISCV_ISA_EXT_XLINUXENVCFG	127
 
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index c74bdb9c0a9f..acd745bbf41f 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -315,6 +315,7 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
 	__RISCV_ISA_EXT_DATA_VALIDATE(zcb, RISCV_ISA_EXT_ZCB, riscv_ext_zca_depends),
 	__RISCV_ISA_EXT_DATA_VALIDATE(zcd, RISCV_ISA_EXT_ZCD, riscv_ext_zcd_validate),
 	__RISCV_ISA_EXT_DATA_VALIDATE(zcf, RISCV_ISA_EXT_ZCF, riscv_ext_zcf_validate),
+	__RISCV_ISA_EXT_DATA_VALIDATE(zcmop, RISCV_ISA_EXT_ZCMOP, riscv_ext_zca_depends),
 	__RISCV_ISA_EXT_DATA(zba, RISCV_ISA_EXT_ZBA),
 	__RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
 	__RISCV_ISA_EXT_DATA(zbc, RISCV_ISA_EXT_ZBC),
-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v4 09/11] riscv: hwprobe: export Zcmop ISA extension

[Clément Léger]

Export Zcmop ISA extension through hwprobe.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 Documentation/arch/riscv/hwprobe.rst  | 4 ++++
 arch/riscv/include/uapi/asm/hwprobe.h | 1 +
 arch/riscv/kernel/sys_hwprobe.c       | 1 +
 3 files changed, 6 insertions(+)

diff --git a/Documentation/arch/riscv/hwprobe.rst b/Documentation/arch/riscv/hwprobe.rst
index bf96b4e8ba3b..e3187659a077 100644
--- a/Documentation/arch/riscv/hwprobe.rst
+++ b/Documentation/arch/riscv/hwprobe.rst
@@ -212,6 +212,10 @@ The following keys are defined:
        ("Zcf doesn't exist on RV64 as it contains no instructions") of
        riscv-code-size-reduction.
 
+  * :c:macro:`RISCV_HWPROBE_EXT_ZCMOP`: The Zcmop May-Be-Operations extension is
+       supported as defined in the RISC-V ISA manual starting from commit
+       c732a4f39a4 ("Zcmop is ratified/1.0").
+
 * :c:macro:`RISCV_HWPROBE_KEY_CPUPERF_0`: A bitmask that contains performance
   information about the selected set of processors.
 
diff --git a/arch/riscv/include/uapi/asm/hwprobe.h b/arch/riscv/include/uapi/asm/hwprobe.h
index dd4ad77faf49..d97ac5436447 100644
--- a/arch/riscv/include/uapi/asm/hwprobe.h
+++ b/arch/riscv/include/uapi/asm/hwprobe.h
@@ -64,6 +64,7 @@ struct riscv_hwprobe {
 #define		RISCV_HWPROBE_EXT_ZCB		(1ULL << 38)
 #define		RISCV_HWPROBE_EXT_ZCD		(1ULL << 39)
 #define		RISCV_HWPROBE_EXT_ZCF		(1ULL << 40)
+#define		RISCV_HWPROBE_EXT_ZCMOP		(1ULL << 41)
 #define RISCV_HWPROBE_KEY_CPUPERF_0	5
 #define		RISCV_HWPROBE_MISALIGNED_UNKNOWN	(0 << 0)
 #define		RISCV_HWPROBE_MISALIGNED_EMULATED	(1 << 0)
diff --git a/arch/riscv/kernel/sys_hwprobe.c b/arch/riscv/kernel/sys_hwprobe.c
index 2ffa0fe5101e..9457231bd1c0 100644
--- a/arch/riscv/kernel/sys_hwprobe.c
+++ b/arch/riscv/kernel/sys_hwprobe.c
@@ -114,6 +114,7 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
 		EXT_KEY(ZIMOP);
 		EXT_KEY(ZCA);
 		EXT_KEY(ZCB);
+		EXT_KEY(ZCMOP);
 
 		if (has_vector()) {
 			EXT_KEY(ZVBB);
-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v4 10/11] RISC-V: KVM: Allow Zcmop extension for Guest/VM

[Clément Léger]

Extend the KVM ISA extension ONE_REG interface to allow KVM user space
to detect and enable Zcmop extension for Guest/VM.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
Acked-by: Anup Patel <anup@brainfault.org>
---
 arch/riscv/include/uapi/asm/kvm.h | 1 +
 arch/riscv/kvm/vcpu_onereg.c      | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
index 57db3fea679f..0366389a0bae 100644
--- a/arch/riscv/include/uapi/asm/kvm.h
+++ b/arch/riscv/include/uapi/asm/kvm.h
@@ -172,6 +172,7 @@ enum KVM_RISCV_ISA_EXT_ID {
 	KVM_RISCV_ISA_EXT_ZCB,
 	KVM_RISCV_ISA_EXT_ZCD,
 	KVM_RISCV_ISA_EXT_ZCF,
+	KVM_RISCV_ISA_EXT_ZCMOP,
 	KVM_RISCV_ISA_EXT_MAX,
 };
 
diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c
index a2747a6dbdb6..77a0d337faeb 100644
--- a/arch/riscv/kvm/vcpu_onereg.c
+++ b/arch/riscv/kvm/vcpu_onereg.c
@@ -52,6 +52,7 @@ static const unsigned long kvm_isa_ext_arr[] = {
 	KVM_ISA_EXT_ARR(ZCB),
 	KVM_ISA_EXT_ARR(ZCD),
 	KVM_ISA_EXT_ARR(ZCF),
+	KVM_ISA_EXT_ARR(ZCMOP),
 	KVM_ISA_EXT_ARR(ZFA),
 	KVM_ISA_EXT_ARR(ZFH),
 	KVM_ISA_EXT_ARR(ZFHMIN),
@@ -136,6 +137,7 @@ static bool kvm_riscv_vcpu_isa_disable_allowed(unsigned long ext)
 	case KVM_RISCV_ISA_EXT_ZCB:
 	case KVM_RISCV_ISA_EXT_ZCD:
 	case KVM_RISCV_ISA_EXT_ZCF:
+	case KVM_RISCV_ISA_EXT_ZCMOP:
 	case KVM_RISCV_ISA_EXT_ZFA:
 	case KVM_RISCV_ISA_EXT_ZFH:
 	case KVM_RISCV_ISA_EXT_ZFHMIN:
-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v4 11/11] KVM: riscv: selftests: Add Zcmop extension to get-reg-list test

[Clément Léger]

The KVM RISC-V allows Zcmop extension for Guest/VM so add this
extension to get-reg-list test.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
Acked-by: Anup Patel <anup@brainfault.org>
---
 tools/testing/selftests/kvm/riscv/get-reg-list.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c
index 61cad4514197..9604c8ece787 100644
--- a/tools/testing/selftests/kvm/riscv/get-reg-list.c
+++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c
@@ -59,6 +59,7 @@ bool filter_reg(__u64 reg)
 	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_ZCB:
 	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_ZCD:
 	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_ZCF:
+	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_ZCMOP:
 	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_ZFA:
 	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_ZFH:
 	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_ZFHMIN:
@@ -429,6 +430,7 @@ static const char *isa_ext_single_id_to_str(__u64 reg_off)
 		KVM_ISA_EXT_ARR(ZCB),
 		KVM_ISA_EXT_ARR(ZCD),
 		KVM_ISA_EXT_ARR(ZCF),
+		KVM_ISA_EXT_ARR(ZCMOP),
 		KVM_ISA_EXT_ARR(ZFA),
 		KVM_ISA_EXT_ARR(ZFH),
 		KVM_ISA_EXT_ARR(ZFHMIN),
@@ -957,6 +959,7 @@ KVM_ISA_EXT_SIMPLE_CONFIG(zca, ZCA),
 KVM_ISA_EXT_SIMPLE_CONFIG(zcb, ZCB),
 KVM_ISA_EXT_SIMPLE_CONFIG(zcd, ZCD),
 KVM_ISA_EXT_SIMPLE_CONFIG(zcf, ZCF),
+KVM_ISA_EXT_SIMPLE_CONFIG(zcmop, ZCMOP);
 KVM_ISA_EXT_SIMPLE_CONFIG(zfa, ZFA);
 KVM_ISA_EXT_SIMPLE_CONFIG(zfh, ZFH);
 KVM_ISA_EXT_SIMPLE_CONFIG(zfhmin, ZFHMIN);
@@ -1017,6 +1020,7 @@ struct vcpu_reg_list *vcpu_configs[] = {
 	&config_zcb,
 	&config_zcd,
 	&config_zcf,
+	&config_zcmop,
 	&config_zfa,
 	&config_zfh,
 	&config_zfhmin,
-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v4 02/11] riscv: add ISA extensions validation

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 15076 bytes --]

On Mon, Apr 29, 2024 at 05:04:55PM +0200, Clément Léger wrote:
> Since a few extensions (Zicbom/Zicboz) already needs validation and
> future ones will need it as well (Zc*) add a validate() callback to
> struct riscv_isa_ext_data. This require to rework the way extensions are
> parsed and split it in two phases. First phase is isa string or isa
> extension list parsing and consists in enabling all the extensions in a
> temporary bitmask without any validation. The second step "resolves" the
> final isa bitmap, handling potential missing dependencies. The mechanism
> is quite simple and simply validate each extension described in the
> temporary bitmap before enabling it in the final isa bitmap. validate()
> callbacks can return either 0 for success, -EPROBEDEFER if extension
> needs to be validated again at next loop. A previous ISA bitmap is kept
> to avoid looping mutliple times if an extension dependencies are never
> satisfied until we reach a stable state. In order to avoid any potential
> infinite looping, allow looping a maximum of the number of extension we
> handle. Zicboz and Zicbom extensions are modified to use this validation
> mechanism.

Your reply to my last review only talked about part of my comments,
which is usually what you do when you're gonna implement the rest, but
you haven't.
I like the change you've made to shorten looping, but I'd at least like
a response to why a split is not worth doing :)

Thanks,
Conor.

> 
> Signed-off-by: Clément Léger <cleger@rivosinc.com>
> ---
>  arch/riscv/include/asm/cpufeature.h |   1 +
>  arch/riscv/kernel/cpufeature.c      | 211 ++++++++++++++++------------
>  2 files changed, 126 insertions(+), 86 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
> index 347805446151..000796c2d0b1 100644
> --- a/arch/riscv/include/asm/cpufeature.h
> +++ b/arch/riscv/include/asm/cpufeature.h
> @@ -70,6 +70,7 @@ struct riscv_isa_ext_data {
>  	const char *property;
>  	const unsigned int *subset_ext_ids;
>  	const unsigned int subset_ext_size;
> +	int (*validate)(const struct riscv_isa_ext_data *data, const unsigned long *isa_bitmap);
>  };
>  
>  extern const struct riscv_isa_ext_data riscv_isa_ext[];
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 115ba001f1bc..cb2ffa6c8c33 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -72,51 +72,58 @@ bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, unsigned i
>  }
>  EXPORT_SYMBOL_GPL(__riscv_isa_extension_available);
>  
> -static bool riscv_isa_extension_check(int id)
> +static bool riscv_isa_extension_valid(int id)
>  {
> -	switch (id) {
> -	case RISCV_ISA_EXT_ZICBOM:
> -		if (!riscv_cbom_block_size) {
> -			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("Zicbom disabled as cbom-block-size present, but is not a power-of-2\n");
> -			return false;
> -		}
> -		return true;
> -	case RISCV_ISA_EXT_ZICBOZ:
> -		if (!riscv_cboz_block_size) {
> -			pr_err("Zicboz detected in ISA string, disabling as no cboz-block-size found\n");
> -			return false;
> -		} else if (!is_power_of_2(riscv_cboz_block_size)) {
> -			pr_err("Zicboz disabled as cboz-block-size present, but is not a power-of-2\n");
> -			return false;
> -		}
> -		return true;
> -	case RISCV_ISA_EXT_INVALID:
> -		return false;
> +	return id != RISCV_ISA_EXT_INVALID;
> +}
> +
> +static int riscv_ext_zicbom_validate(const struct riscv_isa_ext_data *data,
> +				     const unsigned long *isa_bitmap)
> +{
> +	if (!riscv_cbom_block_size) {
> +		pr_err("Zicbom detected in ISA string, disabling as no cbom-block-size found\n");
> +		return -EINVAL;
> +	} else if (!is_power_of_2(riscv_cbom_block_size)) {
> +		pr_err("Zicbom disabled as cbom-block-size present, but is not a power-of-2\n");
> +		return -EINVAL;
>  	}
> +	return 0;
> +}
>  
> -	return true;
> +static int riscv_ext_zicboz_validate(const struct riscv_isa_ext_data *data,
> +				     const unsigned long *isa_bitmap)
> +{
> +	if (!riscv_cboz_block_size) {
> +		pr_err("Zicboz detected in ISA string, disabling as no cboz-block-size found\n");
> +		return -EINVAL;
> +	} else if (!is_power_of_2(riscv_cboz_block_size)) {
> +		pr_err("Zicboz disabled as cboz-block-size present, but is not a power-of-2\n");
> +		return -EINVAL;
> +	}
> +	return 0;
>  }
>  
> -#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size) {	\
> -	.name = #_name,								\
> -	.property = #_name,							\
> -	.id = _id,								\
> -	.subset_ext_ids = _subset_exts,						\
> -	.subset_ext_size = _subset_exts_size					\
> +#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size, _validate) {	\
> +	.name = #_name,									\
> +	.property = #_name,								\
> +	.id = _id,									\
> +	.subset_ext_ids = _subset_exts,							\
> +	.subset_ext_size = _subset_exts_size,						\
> +	.validate = _validate								\
>  }
>  
> -#define __RISCV_ISA_EXT_DATA(_name, _id) _RISCV_ISA_EXT_DATA(_name, _id, NULL, 0)
> +#define __RISCV_ISA_EXT_DATA(_name, _id) _RISCV_ISA_EXT_DATA(_name, _id, NULL, 0, NULL)
>  
>  /* Used to declare pure "lasso" extension (Zk for instance) */
>  #define __RISCV_ISA_EXT_BUNDLE(_name, _bundled_exts) \
> -	_RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, _bundled_exts, ARRAY_SIZE(_bundled_exts))
> +	_RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, _bundled_exts, \
> +			    ARRAY_SIZE(_bundled_exts), NULL)
>  
>  /* Used to declare extensions that are a superset of other extensions (Zvbb for instance) */
>  #define __RISCV_ISA_EXT_SUPERSET(_name, _id, _sub_exts) \
> -	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts))
> +	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts), NULL)
> +#define __RISCV_ISA_EXT_SUPERSET_VALIDATE(_name, _id, _sub_exts, _validate) \
> +	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts), _validate)
>  
>  static const unsigned int riscv_zk_bundled_exts[] = {
>  	RISCV_ISA_EXT_ZBKB,
> @@ -247,8 +254,10 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
>  	__RISCV_ISA_EXT_DATA(c, RISCV_ISA_EXT_c),
>  	__RISCV_ISA_EXT_DATA(v, RISCV_ISA_EXT_v),
>  	__RISCV_ISA_EXT_DATA(h, RISCV_ISA_EXT_h),
> -	__RISCV_ISA_EXT_SUPERSET(zicbom, RISCV_ISA_EXT_ZICBOM, riscv_xlinuxenvcfg_exts),
> -	__RISCV_ISA_EXT_SUPERSET(zicboz, RISCV_ISA_EXT_ZICBOZ, riscv_xlinuxenvcfg_exts),
> +	__RISCV_ISA_EXT_SUPERSET_VALIDATE(zicbom, RISCV_ISA_EXT_ZICBOM, riscv_xlinuxenvcfg_exts,
> +					  riscv_ext_zicbom_validate),
> +	__RISCV_ISA_EXT_SUPERSET_VALIDATE(zicboz, RISCV_ISA_EXT_ZICBOZ, riscv_xlinuxenvcfg_exts,
> +					  riscv_ext_zicboz_validate),
>  	__RISCV_ISA_EXT_DATA(zicntr, RISCV_ISA_EXT_ZICNTR),
>  	__RISCV_ISA_EXT_DATA(zicond, RISCV_ISA_EXT_ZICOND),
>  	__RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR),
> @@ -310,33 +319,80 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
>  
>  const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext);
>  
> -static void __init match_isa_ext(const struct riscv_isa_ext_data *ext, const char *name,
> -				 const char *name_end, struct riscv_isainfo *isainfo)
> +static void riscv_isa_set_ext(const struct riscv_isa_ext_data *ext, unsigned long *bitmap)
>  {
> -	if ((name_end - name == strlen(ext->name)) &&
> -	     !strncasecmp(name, ext->name, name_end - name)) {
> -		/*
> -		 * If this is a bundle, enable all the ISA extensions that
> -		 * comprise the bundle.
> -		 */
> -		if (ext->subset_ext_size) {
> -			for (int i = 0; i < ext->subset_ext_size; i++) {
> -				if (riscv_isa_extension_check(ext->subset_ext_ids[i]))
> -					set_bit(ext->subset_ext_ids[i], isainfo->isa);
> -			}
> +	/*
> +	 * This is valid even for bundle extensions which uses the RISCV_ISA_EXT_INVALID id
> +	 * (rejected by riscv_isa_extension_valid()).
> +	 */
> +	if (riscv_isa_extension_valid(ext->id))
> +		set_bit(ext->id, bitmap);
> +
> +	for (int i = 0; i < ext->subset_ext_size; i++) {
> +		if (riscv_isa_extension_valid(ext->subset_ext_ids[i]))
> +			set_bit(ext->subset_ext_ids[i], bitmap);
> +	}
> +}
> +
> +static void __init riscv_resolve_isa(unsigned long *isa_bitmap, struct riscv_isainfo *isainfo,
> +				     unsigned long *this_hwcap, unsigned long *isa2hwcap)
> +{
> +	bool loop;
> +	const struct riscv_isa_ext_data *ext;
> +	DECLARE_BITMAP(prev_bitmap, RISCV_ISA_EXT_MAX);
> +	int max_loop_count = riscv_isa_ext_count, ret;
> +
> +	do {
> +		loop = false;
> +		if (max_loop_count-- < 0) {
> +			pr_err("Failed to reach a stable ISA state\n");
> +			return;
>  		}
> +		memcpy(prev_bitmap, isainfo->isa, sizeof(prev_bitmap));
> +		for (int i = 0; i < riscv_isa_ext_count; i++) {
> +			ext = &riscv_isa_ext[i];
> +
> +			/* Bundle extensions ids are invalid*/
> +			if (!riscv_isa_extension_valid(ext->id))
> +				continue;
> +
> +			if (!test_bit(ext->id, isa_bitmap) || test_bit(ext->id, isainfo->isa))
> +				continue;
> +
> +			if (ext->validate) {
> +				ret = ext->validate(ext, isainfo->isa);
> +				if (ret) {
> +					if (ret == -EPROBE_DEFER)
> +						loop = true;
> +					else
> +						clear_bit(ext->id, isa_bitmap);
> +					continue;
> +				}
> +			}
>  
> -		/*
> -		 * This is valid even for bundle extensions which uses the RISCV_ISA_EXT_INVALID id
> -		 * (rejected by riscv_isa_extension_check()).
> -		 */
> -		if (riscv_isa_extension_check(ext->id))
>  			set_bit(ext->id, isainfo->isa);
> +
> +			/* Only single letter extensions get set in hwcap */
> +			if (ext->id < RISCV_ISA_EXT_BASE)
> +				*this_hwcap |= isa2hwcap[ext->id];
> +		}
> +	} while (loop && memcmp(prev_bitmap, isainfo->isa, sizeof(prev_bitmap)));
> +}
> +
> +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)) {
> +			riscv_isa_set_ext(ext, bitmap);
> +			break;
> +		}
>  	}
>  }
>  
> -static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct riscv_isainfo *isainfo,
> -					  unsigned long *isa2hwcap, const char *isa)
> +static void __init riscv_resolve_isa_string(const char *isa, unsigned long *bitmap)
>  {
>  	/*
>  	 * For all possible cpus, we have already validated in
> @@ -349,7 +405,7 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc
>  	while (*isa) {
>  		const char *ext = isa++;
>  		const char *ext_end = isa;
> -		bool ext_long = false, ext_err = false;
> +		bool ext_err = false;
>  
>  		switch (*ext) {
>  		case 's':
> @@ -389,7 +445,6 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc
>  			 * character itself while eliminating the extensions version number.
>  			 * A simple re-increment solves this problem.
>  			 */
> -			ext_long = true;
>  			for (; *isa && *isa != '_'; ++isa)
>  				if (unlikely(!isalnum(*isa)))
>  					ext_err = true;
> @@ -469,17 +524,8 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc
>  
>  		if (unlikely(ext_err))
>  			continue;
> -		if (!ext_long) {
> -			int nr = tolower(*ext) - 'a';
>  
> -			if (riscv_isa_extension_check(nr)) {
> -				*this_hwcap |= isa2hwcap[nr];
> -				set_bit(nr, isainfo->isa);
> -			}
> -		} else {
> -			for (int i = 0; i < riscv_isa_ext_count; i++)
> -				match_isa_ext(&riscv_isa_ext[i], ext, ext_end, isainfo);
> -		}
> +		match_isa_ext(ext, ext_end, bitmap);
>  	}
>  }
>  
> @@ -501,6 +547,7 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
>  	for_each_possible_cpu(cpu) {
>  		struct riscv_isainfo *isainfo = &hart_isa[cpu];
>  		unsigned long this_hwcap = 0;
> +		DECLARE_BITMAP(isa_bitmap, RISCV_ISA_EXT_MAX) = { 0 };
>  
>  		if (acpi_disabled) {
>  			node = of_cpu_device_node_get(cpu);
> @@ -523,7 +570,7 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
>  			}
>  		}
>  
> -		riscv_parse_isa_string(&this_hwcap, isainfo, isa2hwcap, isa);
> +		riscv_resolve_isa_string(isa, isa_bitmap);
>  
>  		/*
>  		 * These ones were as they were part of the base ISA when the
> @@ -531,10 +578,10 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
>  		 * unconditionally where `i` is in riscv,isa on DT systems.
>  		 */
>  		if (acpi_disabled) {
> -			set_bit(RISCV_ISA_EXT_ZICSR, isainfo->isa);
> -			set_bit(RISCV_ISA_EXT_ZIFENCEI, isainfo->isa);
> -			set_bit(RISCV_ISA_EXT_ZICNTR, isainfo->isa);
> -			set_bit(RISCV_ISA_EXT_ZIHPM, isainfo->isa);
> +			set_bit(RISCV_ISA_EXT_ZICSR, isa_bitmap);
> +			set_bit(RISCV_ISA_EXT_ZIFENCEI, isa_bitmap);
> +			set_bit(RISCV_ISA_EXT_ZICNTR, isa_bitmap);
> +			set_bit(RISCV_ISA_EXT_ZIHPM, isa_bitmap);
>  		}
>  
>  		/*
> @@ -548,9 +595,11 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
>  		if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID &&
>  		    riscv_cached_marchid(cpu) == 0x0) {
>  			this_hwcap &= ~isa2hwcap[RISCV_ISA_EXT_v];
> -			clear_bit(RISCV_ISA_EXT_v, isainfo->isa);
> +			clear_bit(RISCV_ISA_EXT_v, isa_bitmap);
>  		}
>  
> +		riscv_resolve_isa(isa_bitmap, isainfo, &this_hwcap, isa2hwcap);
> +
>  		/*
>  		 * All "okay" hart should have same isa. Set HWCAP based on
>  		 * common capabilities of every "okay" hart, in case they don't
> @@ -579,6 +628,7 @@ static int __init riscv_fill_hwcap_from_ext_list(unsigned long *isa2hwcap)
>  		unsigned long this_hwcap = 0;
>  		struct device_node *cpu_node;
>  		struct riscv_isainfo *isainfo = &hart_isa[cpu];
> +		DECLARE_BITMAP(isa_bitmap, RISCV_ISA_EXT_MAX) = { 0 };
>  
>  		cpu_node = of_cpu_device_node_get(cpu);
>  		if (!cpu_node) {
> @@ -598,22 +648,11 @@ static int __init riscv_fill_hwcap_from_ext_list(unsigned long *isa2hwcap)
>  						     ext->property) < 0)
>  				continue;
>  
> -			if (ext->subset_ext_size) {
> -				for (int j = 0; j < ext->subset_ext_size; j++) {
> -					if (riscv_isa_extension_check(ext->subset_ext_ids[i]))
> -						set_bit(ext->subset_ext_ids[j], isainfo->isa);
> -				}
> -			}
> -
> -			if (riscv_isa_extension_check(ext->id)) {
> -				set_bit(ext->id, isainfo->isa);
> -
> -				/* Only single letter extensions get set in hwcap */
> -				if (strnlen(riscv_isa_ext[i].name, 2) == 1)
> -					this_hwcap |= isa2hwcap[riscv_isa_ext[i].id];
> -			}
> +			riscv_isa_set_ext(ext, isa_bitmap);
>  		}
>  
> +		riscv_resolve_isa(isa_bitmap, isainfo, &this_hwcap, isa2hwcap);
> +
>  		of_node_put(cpu_node);
>  
>  		/*
> -- 
> 2.43.0
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v4 02/11] riscv: add ISA extensions validation

[Clément Léger]

On 30/04/2024 00:15, Conor Dooley wrote:
> On Mon, Apr 29, 2024 at 05:04:55PM +0200, Clément Léger wrote:
>> Since a few extensions (Zicbom/Zicboz) already needs validation and
>> future ones will need it as well (Zc*) add a validate() callback to
>> struct riscv_isa_ext_data. This require to rework the way extensions are
>> parsed and split it in two phases. First phase is isa string or isa
>> extension list parsing and consists in enabling all the extensions in a
>> temporary bitmask without any validation. The second step "resolves" the
>> final isa bitmap, handling potential missing dependencies. The mechanism
>> is quite simple and simply validate each extension described in the
>> temporary bitmap before enabling it in the final isa bitmap. validate()
>> callbacks can return either 0 for success, -EPROBEDEFER if extension
>> needs to be validated again at next loop. A previous ISA bitmap is kept
>> to avoid looping mutliple times if an extension dependencies are never
>> satisfied until we reach a stable state. In order to avoid any potential
>> infinite looping, allow looping a maximum of the number of extension we
>> handle. Zicboz and Zicbom extensions are modified to use this validation
>> mechanism.
> 
> Your reply to my last review only talked about part of my comments,
> which is usually what you do when you're gonna implement the rest, but
> you haven't.
> I like the change you've made to shorten looping, but I'd at least like
> a response to why a split is not worth doing :)

Hi Conor,

Missed that point since I was feeling that my solution actually
addresses your concerns. Your argument was that there is no reason to
loop for Zicbom/Zicboz but that would also apply to Zcf in case we are
on RV64 as well (since zcf is not supported on RV64). So for Zcf, that
would lead to using both mecanism or additional ifdefery with little to
no added value since the current solution actually solves both cases:

- We don't have any extra looping if all validation callback returns 0
(except the initial one on riscv_isa_ext, which is kind of unavoidable).
- Zicbom, Zicboz callbacks will be called only once (which was one of
your concern).

Adding a second kind of callback for after loop validation would only
lead to a bunch of additional macros/ifdefery for extensions with
validate() callback, with validate_end() or with both (ie Zcf)). For
these reasons, I do not think there is a need for a separate mechanism
nor additional callback for such extensions except adding extra code
with no real added functionality.

AFAIK, the platform driver probing mechanism works the same, the probe()
callback is actually called even if for some reason properties are
missing from nodes for platform devices and thus the probe() returns
-EINVAL or whatever.

Hope this answers your question,

Thanks,

Clément

> 
> Thanks,
> Conor.
> 
>>
>> Signed-off-by: Clément Léger <cleger@rivosinc.com>
>> ---
>>  arch/riscv/include/asm/cpufeature.h |   1 +
>>  arch/riscv/kernel/cpufeature.c      | 211 ++++++++++++++++------------
>>  2 files changed, 126 insertions(+), 86 deletions(-)
>>
>> diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
>> index 347805446151..000796c2d0b1 100644
>> --- a/arch/riscv/include/asm/cpufeature.h
>> +++ b/arch/riscv/include/asm/cpufeature.h
>> @@ -70,6 +70,7 @@ struct riscv_isa_ext_data {
>>  	const char *property;
>>  	const unsigned int *subset_ext_ids;
>>  	const unsigned int subset_ext_size;
>> +	int (*validate)(const struct riscv_isa_ext_data *data, const unsigned long *isa_bitmap);
>>  };
>>  
>>  extern const struct riscv_isa_ext_data riscv_isa_ext[];
>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>> index 115ba001f1bc..cb2ffa6c8c33 100644
>> --- a/arch/riscv/kernel/cpufeature.c
>> +++ b/arch/riscv/kernel/cpufeature.c
>> @@ -72,51 +72,58 @@ bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, unsigned i
>>  }
>>  EXPORT_SYMBOL_GPL(__riscv_isa_extension_available);
>>  
>> -static bool riscv_isa_extension_check(int id)
>> +static bool riscv_isa_extension_valid(int id)
>>  {
>> -	switch (id) {
>> -	case RISCV_ISA_EXT_ZICBOM:
>> -		if (!riscv_cbom_block_size) {
>> -			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("Zicbom disabled as cbom-block-size present, but is not a power-of-2\n");
>> -			return false;
>> -		}
>> -		return true;
>> -	case RISCV_ISA_EXT_ZICBOZ:
>> -		if (!riscv_cboz_block_size) {
>> -			pr_err("Zicboz detected in ISA string, disabling as no cboz-block-size found\n");
>> -			return false;
>> -		} else if (!is_power_of_2(riscv_cboz_block_size)) {
>> -			pr_err("Zicboz disabled as cboz-block-size present, but is not a power-of-2\n");
>> -			return false;
>> -		}
>> -		return true;
>> -	case RISCV_ISA_EXT_INVALID:
>> -		return false;
>> +	return id != RISCV_ISA_EXT_INVALID;
>> +}
>> +
>> +static int riscv_ext_zicbom_validate(const struct riscv_isa_ext_data *data,
>> +				     const unsigned long *isa_bitmap)
>> +{
>> +	if (!riscv_cbom_block_size) {
>> +		pr_err("Zicbom detected in ISA string, disabling as no cbom-block-size found\n");
>> +		return -EINVAL;
>> +	} else if (!is_power_of_2(riscv_cbom_block_size)) {
>> +		pr_err("Zicbom disabled as cbom-block-size present, but is not a power-of-2\n");
>> +		return -EINVAL;
>>  	}
>> +	return 0;
>> +}
>>  
>> -	return true;
>> +static int riscv_ext_zicboz_validate(const struct riscv_isa_ext_data *data,
>> +				     const unsigned long *isa_bitmap)
>> +{
>> +	if (!riscv_cboz_block_size) {
>> +		pr_err("Zicboz detected in ISA string, disabling as no cboz-block-size found\n");
>> +		return -EINVAL;
>> +	} else if (!is_power_of_2(riscv_cboz_block_size)) {
>> +		pr_err("Zicboz disabled as cboz-block-size present, but is not a power-of-2\n");
>> +		return -EINVAL;
>> +	}
>> +	return 0;
>>  }
>>  
>> -#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size) {	\
>> -	.name = #_name,								\
>> -	.property = #_name,							\
>> -	.id = _id,								\
>> -	.subset_ext_ids = _subset_exts,						\
>> -	.subset_ext_size = _subset_exts_size					\
>> +#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size, _validate) {	\
>> +	.name = #_name,									\
>> +	.property = #_name,								\
>> +	.id = _id,									\
>> +	.subset_ext_ids = _subset_exts,							\
>> +	.subset_ext_size = _subset_exts_size,						\
>> +	.validate = _validate								\
>>  }
>>  
>> -#define __RISCV_ISA_EXT_DATA(_name, _id) _RISCV_ISA_EXT_DATA(_name, _id, NULL, 0)
>> +#define __RISCV_ISA_EXT_DATA(_name, _id) _RISCV_ISA_EXT_DATA(_name, _id, NULL, 0, NULL)
>>  
>>  /* Used to declare pure "lasso" extension (Zk for instance) */
>>  #define __RISCV_ISA_EXT_BUNDLE(_name, _bundled_exts) \
>> -	_RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, _bundled_exts, ARRAY_SIZE(_bundled_exts))
>> +	_RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, _bundled_exts, \
>> +			    ARRAY_SIZE(_bundled_exts), NULL)
>>  
>>  /* Used to declare extensions that are a superset of other extensions (Zvbb for instance) */
>>  #define __RISCV_ISA_EXT_SUPERSET(_name, _id, _sub_exts) \
>> -	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts))
>> +	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts), NULL)
>> +#define __RISCV_ISA_EXT_SUPERSET_VALIDATE(_name, _id, _sub_exts, _validate) \
>> +	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts), _validate)
>>  
>>  static const unsigned int riscv_zk_bundled_exts[] = {
>>  	RISCV_ISA_EXT_ZBKB,
>> @@ -247,8 +254,10 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
>>  	__RISCV_ISA_EXT_DATA(c, RISCV_ISA_EXT_c),
>>  	__RISCV_ISA_EXT_DATA(v, RISCV_ISA_EXT_v),
>>  	__RISCV_ISA_EXT_DATA(h, RISCV_ISA_EXT_h),
>> -	__RISCV_ISA_EXT_SUPERSET(zicbom, RISCV_ISA_EXT_ZICBOM, riscv_xlinuxenvcfg_exts),
>> -	__RISCV_ISA_EXT_SUPERSET(zicboz, RISCV_ISA_EXT_ZICBOZ, riscv_xlinuxenvcfg_exts),
>> +	__RISCV_ISA_EXT_SUPERSET_VALIDATE(zicbom, RISCV_ISA_EXT_ZICBOM, riscv_xlinuxenvcfg_exts,
>> +					  riscv_ext_zicbom_validate),
>> +	__RISCV_ISA_EXT_SUPERSET_VALIDATE(zicboz, RISCV_ISA_EXT_ZICBOZ, riscv_xlinuxenvcfg_exts,
>> +					  riscv_ext_zicboz_validate),
>>  	__RISCV_ISA_EXT_DATA(zicntr, RISCV_ISA_EXT_ZICNTR),
>>  	__RISCV_ISA_EXT_DATA(zicond, RISCV_ISA_EXT_ZICOND),
>>  	__RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR),
>> @@ -310,33 +319,80 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
>>  
>>  const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext);
>>  
>> -static void __init match_isa_ext(const struct riscv_isa_ext_data *ext, const char *name,
>> -				 const char *name_end, struct riscv_isainfo *isainfo)
>> +static void riscv_isa_set_ext(const struct riscv_isa_ext_data *ext, unsigned long *bitmap)
>>  {
>> -	if ((name_end - name == strlen(ext->name)) &&
>> -	     !strncasecmp(name, ext->name, name_end - name)) {
>> -		/*
>> -		 * If this is a bundle, enable all the ISA extensions that
>> -		 * comprise the bundle.
>> -		 */
>> -		if (ext->subset_ext_size) {
>> -			for (int i = 0; i < ext->subset_ext_size; i++) {
>> -				if (riscv_isa_extension_check(ext->subset_ext_ids[i]))
>> -					set_bit(ext->subset_ext_ids[i], isainfo->isa);
>> -			}
>> +	/*
>> +	 * This is valid even for bundle extensions which uses the RISCV_ISA_EXT_INVALID id
>> +	 * (rejected by riscv_isa_extension_valid()).
>> +	 */
>> +	if (riscv_isa_extension_valid(ext->id))
>> +		set_bit(ext->id, bitmap);
>> +
>> +	for (int i = 0; i < ext->subset_ext_size; i++) {
>> +		if (riscv_isa_extension_valid(ext->subset_ext_ids[i]))
>> +			set_bit(ext->subset_ext_ids[i], bitmap);
>> +	}
>> +}
>> +
>> +static void __init riscv_resolve_isa(unsigned long *isa_bitmap, struct riscv_isainfo *isainfo,
>> +				     unsigned long *this_hwcap, unsigned long *isa2hwcap)
>> +{
>> +	bool loop;
>> +	const struct riscv_isa_ext_data *ext;
>> +	DECLARE_BITMAP(prev_bitmap, RISCV_ISA_EXT_MAX);
>> +	int max_loop_count = riscv_isa_ext_count, ret;
>> +
>> +	do {
>> +		loop = false;
>> +		if (max_loop_count-- < 0) {
>> +			pr_err("Failed to reach a stable ISA state\n");
>> +			return;
>>  		}
>> +		memcpy(prev_bitmap, isainfo->isa, sizeof(prev_bitmap));
>> +		for (int i = 0; i < riscv_isa_ext_count; i++) {
>> +			ext = &riscv_isa_ext[i];
>> +
>> +			/* Bundle extensions ids are invalid*/
>> +			if (!riscv_isa_extension_valid(ext->id))
>> +				continue;
>> +
>> +			if (!test_bit(ext->id, isa_bitmap) || test_bit(ext->id, isainfo->isa))
>> +				continue;
>> +
>> +			if (ext->validate) {
>> +				ret = ext->validate(ext, isainfo->isa);
>> +				if (ret) {
>> +					if (ret == -EPROBE_DEFER)
>> +						loop = true;
>> +					else
>> +						clear_bit(ext->id, isa_bitmap);
>> +					continue;
>> +				}
>> +			}
>>  
>> -		/*
>> -		 * This is valid even for bundle extensions which uses the RISCV_ISA_EXT_INVALID id
>> -		 * (rejected by riscv_isa_extension_check()).
>> -		 */
>> -		if (riscv_isa_extension_check(ext->id))
>>  			set_bit(ext->id, isainfo->isa);
>> +
>> +			/* Only single letter extensions get set in hwcap */
>> +			if (ext->id < RISCV_ISA_EXT_BASE)
>> +				*this_hwcap |= isa2hwcap[ext->id];
>> +		}
>> +	} while (loop && memcmp(prev_bitmap, isainfo->isa, sizeof(prev_bitmap)));
>> +}
>> +
>> +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)) {
>> +			riscv_isa_set_ext(ext, bitmap);
>> +			break;
>> +		}
>>  	}
>>  }
>>  
>> -static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct riscv_isainfo *isainfo,
>> -					  unsigned long *isa2hwcap, const char *isa)
>> +static void __init riscv_resolve_isa_string(const char *isa, unsigned long *bitmap)
>>  {
>>  	/*
>>  	 * For all possible cpus, we have already validated in
>> @@ -349,7 +405,7 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc
>>  	while (*isa) {
>>  		const char *ext = isa++;
>>  		const char *ext_end = isa;
>> -		bool ext_long = false, ext_err = false;
>> +		bool ext_err = false;
>>  
>>  		switch (*ext) {
>>  		case 's':
>> @@ -389,7 +445,6 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc
>>  			 * character itself while eliminating the extensions version number.
>>  			 * A simple re-increment solves this problem.
>>  			 */
>> -			ext_long = true;
>>  			for (; *isa && *isa != '_'; ++isa)
>>  				if (unlikely(!isalnum(*isa)))
>>  					ext_err = true;
>> @@ -469,17 +524,8 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc
>>  
>>  		if (unlikely(ext_err))
>>  			continue;
>> -		if (!ext_long) {
>> -			int nr = tolower(*ext) - 'a';
>>  
>> -			if (riscv_isa_extension_check(nr)) {
>> -				*this_hwcap |= isa2hwcap[nr];
>> -				set_bit(nr, isainfo->isa);
>> -			}
>> -		} else {
>> -			for (int i = 0; i < riscv_isa_ext_count; i++)
>> -				match_isa_ext(&riscv_isa_ext[i], ext, ext_end, isainfo);
>> -		}
>> +		match_isa_ext(ext, ext_end, bitmap);
>>  	}
>>  }
>>  
>> @@ -501,6 +547,7 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
>>  	for_each_possible_cpu(cpu) {
>>  		struct riscv_isainfo *isainfo = &hart_isa[cpu];
>>  		unsigned long this_hwcap = 0;
>> +		DECLARE_BITMAP(isa_bitmap, RISCV_ISA_EXT_MAX) = { 0 };
>>  
>>  		if (acpi_disabled) {
>>  			node = of_cpu_device_node_get(cpu);
>> @@ -523,7 +570,7 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
>>  			}
>>  		}
>>  
>> -		riscv_parse_isa_string(&this_hwcap, isainfo, isa2hwcap, isa);
>> +		riscv_resolve_isa_string(isa, isa_bitmap);
>>  
>>  		/*
>>  		 * These ones were as they were part of the base ISA when the
>> @@ -531,10 +578,10 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
>>  		 * unconditionally where `i` is in riscv,isa on DT systems.
>>  		 */
>>  		if (acpi_disabled) {
>> -			set_bit(RISCV_ISA_EXT_ZICSR, isainfo->isa);
>> -			set_bit(RISCV_ISA_EXT_ZIFENCEI, isainfo->isa);
>> -			set_bit(RISCV_ISA_EXT_ZICNTR, isainfo->isa);
>> -			set_bit(RISCV_ISA_EXT_ZIHPM, isainfo->isa);
>> +			set_bit(RISCV_ISA_EXT_ZICSR, isa_bitmap);
>> +			set_bit(RISCV_ISA_EXT_ZIFENCEI, isa_bitmap);
>> +			set_bit(RISCV_ISA_EXT_ZICNTR, isa_bitmap);
>> +			set_bit(RISCV_ISA_EXT_ZIHPM, isa_bitmap);
>>  		}
>>  
>>  		/*
>> @@ -548,9 +595,11 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
>>  		if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID &&
>>  		    riscv_cached_marchid(cpu) == 0x0) {
>>  			this_hwcap &= ~isa2hwcap[RISCV_ISA_EXT_v];
>> -			clear_bit(RISCV_ISA_EXT_v, isainfo->isa);
>> +			clear_bit(RISCV_ISA_EXT_v, isa_bitmap);
>>  		}
>>  
>> +		riscv_resolve_isa(isa_bitmap, isainfo, &this_hwcap, isa2hwcap);
>> +
>>  		/*
>>  		 * All "okay" hart should have same isa. Set HWCAP based on
>>  		 * common capabilities of every "okay" hart, in case they don't
>> @@ -579,6 +628,7 @@ static int __init riscv_fill_hwcap_from_ext_list(unsigned long *isa2hwcap)
>>  		unsigned long this_hwcap = 0;
>>  		struct device_node *cpu_node;
>>  		struct riscv_isainfo *isainfo = &hart_isa[cpu];
>> +		DECLARE_BITMAP(isa_bitmap, RISCV_ISA_EXT_MAX) = { 0 };
>>  
>>  		cpu_node = of_cpu_device_node_get(cpu);
>>  		if (!cpu_node) {
>> @@ -598,22 +648,11 @@ static int __init riscv_fill_hwcap_from_ext_list(unsigned long *isa2hwcap)
>>  						     ext->property) < 0)
>>  				continue;
>>  
>> -			if (ext->subset_ext_size) {
>> -				for (int j = 0; j < ext->subset_ext_size; j++) {
>> -					if (riscv_isa_extension_check(ext->subset_ext_ids[i]))
>> -						set_bit(ext->subset_ext_ids[j], isainfo->isa);
>> -				}
>> -			}
>> -
>> -			if (riscv_isa_extension_check(ext->id)) {
>> -				set_bit(ext->id, isainfo->isa);
>> -
>> -				/* Only single letter extensions get set in hwcap */
>> -				if (strnlen(riscv_isa_ext[i].name, 2) == 1)
>> -					this_hwcap |= isa2hwcap[riscv_isa_ext[i].id];
>> -			}
>> +			riscv_isa_set_ext(ext, isa_bitmap);
>>  		}
>>  
>> +		riscv_resolve_isa(isa_bitmap, isainfo, &this_hwcap, isa2hwcap);
>> +
>>  		of_node_put(cpu_node);
>>  
>>  		/*
>> -- 
>> 2.43.0
>>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v4 02/11] riscv: add ISA extensions validation

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 4415 bytes --]

On Tue, Apr 30, 2024 at 09:18:47AM +0200, Clément Léger wrote:
> 
> 
> On 30/04/2024 00:15, Conor Dooley wrote:
> > On Mon, Apr 29, 2024 at 05:04:55PM +0200, Clément Léger wrote:
> >> Since a few extensions (Zicbom/Zicboz) already needs validation and
> >> future ones will need it as well (Zc*) add a validate() callback to
> >> struct riscv_isa_ext_data. This require to rework the way extensions are
> >> parsed and split it in two phases. First phase is isa string or isa
> >> extension list parsing and consists in enabling all the extensions in a
> >> temporary bitmask without any validation. The second step "resolves" the
> >> final isa bitmap, handling potential missing dependencies. The mechanism
> >> is quite simple and simply validate each extension described in the
> >> temporary bitmap before enabling it in the final isa bitmap. validate()
> >> callbacks can return either 0 for success, -EPROBEDEFER if extension
> >> needs to be validated again at next loop. A previous ISA bitmap is kept
> >> to avoid looping mutliple times if an extension dependencies are never
> >> satisfied until we reach a stable state. In order to avoid any potential
> >> infinite looping, allow looping a maximum of the number of extension we
> >> handle. Zicboz and Zicbom extensions are modified to use this validation
> >> mechanism.
> > 
> > Your reply to my last review only talked about part of my comments,
> > which is usually what you do when you're gonna implement the rest, but
> > you haven't.
> > I like the change you've made to shorten looping, but I'd at least like
> > a response to why a split is not worth doing :)
> 
> Hi Conor,
> 
> Missed that point since I was feeling that my solution actually
> addresses your concerns. Your argument was that there is no reason to
> loop for Zicbom/Zicboz but that would also apply to Zcf in case we are
> on RV64 as well (since zcf is not supported on RV64). So for Zcf, that
> would lead to using both mecanism or additional ifdefery with little to
> no added value since the current solution actually solves both cases:
> 
> - We don't have any extra looping if all validation callback returns 0
> (except the initial one on riscv_isa_ext, which is kind of unavoidable).
> - Zicbom, Zicboz callbacks will be called only once (which was one of
> your concern).
> 
> Adding a second kind of callback for after loop validation would only
> lead to a bunch of additional macros/ifdefery for extensions with
> validate() callback, with validate_end() or with both (ie Zcf)). For
> these reasons, I do not think there is a need for a separate mechanism
> nor additional callback for such extensions except adding extra code
> with no real added functionality.
> 
> AFAIK, the platform driver probing mechanism works the same, the probe()
> callback is actually called even if for some reason properties are
> missing from nodes for platform devices and thus the probe() returns
> -EINVAL or whatever.
> 
> Hope this answers your question,

Yeah, pretty much I am happy with just an "it's not worth doing it"
response. Given it wasn't your first choice, I doubt you're overly happy
with it either, but I really would like to avoid looping to closure to
sort out dependencies - particularly on the boot CPU before we bring
anyone else up, but if the code is now more proactive about breaking
out, I suppose that'll have to do :)
I kinda wish we didn't do this at all, but I think we've brought this
upon ourselves via hwprobe. I'm still on the fence as to whether things
that are implied need to be handled in this way. I think I'll bring this
up tomorrow at the weekly call, because so far it's only been you and I
discussing this really and it's a policy decision that hwprobe-ists
should be involved in I think.

Implied extensions aside, I think we will eventually need this stuff
anyway, for extensions that make no sense to consider if a config option
for a dependency is disabled.
From talking to Eric Biggers the other week about
riscv_isa_extension_available() I'm of the opinion that we need to do
better with that interface w.r.t. extension and config dependencies,
and what seems like a good idea to me at the moment is putting tests for
IS_ENABLED(RISCV_ISA_FOO) into these validate hooks.

I'll try to look at the actual implementation here tomorrow.

Cheers,
Conor.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v4 02/11] riscv: add ISA extensions validation

[Clément Léger]

On 30/04/2024 13:44, Conor Dooley wrote:
> On Tue, Apr 30, 2024 at 09:18:47AM +0200, Clément Léger wrote:
>>
>>
>> On 30/04/2024 00:15, Conor Dooley wrote:
>>> On Mon, Apr 29, 2024 at 05:04:55PM +0200, Clément Léger wrote:
>>>> Since a few extensions (Zicbom/Zicboz) already needs validation and
>>>> future ones will need it as well (Zc*) add a validate() callback to
>>>> struct riscv_isa_ext_data. This require to rework the way extensions are
>>>> parsed and split it in two phases. First phase is isa string or isa
>>>> extension list parsing and consists in enabling all the extensions in a
>>>> temporary bitmask without any validation. The second step "resolves" the
>>>> final isa bitmap, handling potential missing dependencies. The mechanism
>>>> is quite simple and simply validate each extension described in the
>>>> temporary bitmap before enabling it in the final isa bitmap. validate()
>>>> callbacks can return either 0 for success, -EPROBEDEFER if extension
>>>> needs to be validated again at next loop. A previous ISA bitmap is kept
>>>> to avoid looping mutliple times if an extension dependencies are never
>>>> satisfied until we reach a stable state. In order to avoid any potential
>>>> infinite looping, allow looping a maximum of the number of extension we
>>>> handle. Zicboz and Zicbom extensions are modified to use this validation
>>>> mechanism.
>>>
>>> Your reply to my last review only talked about part of my comments,
>>> which is usually what you do when you're gonna implement the rest, but
>>> you haven't.
>>> I like the change you've made to shorten looping, but I'd at least like
>>> a response to why a split is not worth doing :)
>>
>> Hi Conor,
>>
>> Missed that point since I was feeling that my solution actually
>> addresses your concerns. Your argument was that there is no reason to
>> loop for Zicbom/Zicboz but that would also apply to Zcf in case we are
>> on RV64 as well (since zcf is not supported on RV64). So for Zcf, that
>> would lead to using both mecanism or additional ifdefery with little to
>> no added value since the current solution actually solves both cases:
>>
>> - We don't have any extra looping if all validation callback returns 0
>> (except the initial one on riscv_isa_ext, which is kind of unavoidable).
>> - Zicbom, Zicboz callbacks will be called only once (which was one of
>> your concern).
>>
>> Adding a second kind of callback for after loop validation would only
>> lead to a bunch of additional macros/ifdefery for extensions with
>> validate() callback, with validate_end() or with both (ie Zcf)). For
>> these reasons, I do not think there is a need for a separate mechanism
>> nor additional callback for such extensions except adding extra code
>> with no real added functionality.
>>
>> AFAIK, the platform driver probing mechanism works the same, the probe()
>> callback is actually called even if for some reason properties are
>> missing from nodes for platform devices and thus the probe() returns
>> -EINVAL or whatever.
>>
>> Hope this answers your question,
> 
> Yeah, pretty much I am happy with just an "it's not worth doing it"
> response. Given it wasn't your first choice, I doubt you're overly happy
> with it either, but I really would like to avoid looping to closure to
> sort out dependencies - particularly on the boot CPU before we bring
> anyone else up, but if the code is now more proactive about breaking
> out, I suppose that'll have to do :)

Ahah, I would have done it if it would have make sense and add any
useful support. But AFAICT, the current implementation solves most of
the problems you raised.

While thinking about it, I can even simply it a bit more. Once a ISA
extension bit is set in the final mask, I can actually disable it in the
original mask to avoid looping over it again. I'll send a V5.

> I kinda wish we didn't do this at all, but I think we've brought this
> upon ourselves via hwprobe. I'm still on the fence as to whether things
> that are implied need to be handled in this way. I think I'll bring this
> up tomorrow at the weekly call, because so far it's only been you and I
> discussing this really and it's a policy decision that hwprobe-ists
> should be involved in I think.

Yeah sure.

> 
> Implied extensions aside, I think we will eventually need this stuff
> anyway, for extensions that make no sense to consider if a config option
> for a dependency is disabled.
> From talking to Eric Biggers the other week about
> riscv_isa_extension_available() I'm of the opinion that we need to do
> better with that interface w.r.t. extension and config dependencies,
> and what seems like a good idea to me at the moment is putting tests for
> IS_ENABLED(RISCV_ISA_FOO) into these validate hooks.

Yeah, see what you mean. I think we also need to define if we want to
expose all the ISA extensions in /proc/cpuinfo (ie no matter the config
of the kernel) or not. If so, additional validate() callback would make
sense. If we want to keep the full ISA string in /proc/info, then we
will need another way of doing so.

> 
> I'll try to look at the actual implementation here tomorrow.

Great, thanks.

Clément

> 
> Cheers,
> Conor.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v4 02/11] riscv: add ISA extensions validation

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 706 bytes --]

On Tue, Apr 30, 2024 at 01:58:11PM +0200, Clément Léger wrote:
> Yeah, see what you mean. I think we also need to define if we want to
> expose all the ISA extensions in /proc/cpuinfo (ie no matter the config
> of the kernel) or not. If so, additional validate() callback would make
> sense. If we want to keep the full ISA string in /proc/info, then we
> will need another way of doing so.

If extensions aren't usable, they shouldn't be in /proc/cpuinfo either
as there's programs that parse that to figure out what they can use,
possibly even only checking a single cpu and using that as gospel.
That's why there's that per-hart-isa thing that was added by one of your
colleagues last year.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v4 02/11] riscv: add ISA extensions validation

[Clément Léger]

On 30/04/2024 14:12, Conor Dooley wrote:
> On Tue, Apr 30, 2024 at 01:58:11PM +0200, Clément Léger wrote:
>> Yeah, see what you mean. I think we also need to define if we want to
>> expose all the ISA extensions in /proc/cpuinfo (ie no matter the config
>> of the kernel) or not. If so, additional validate() callback would make
>> sense. If we want to keep the full ISA string in /proc/info, then we
>> will need another way of doing so.
> 
> If extensions aren't usable, they shouldn't be in /proc/cpuinfo either
> as there's programs that parse that to figure out what they can use,
> possibly even only checking a single cpu and using that as gospel.
> That's why there's that per-hart-isa thing that was added by one of your
> colleagues last year.

Acked. So indeed, validate() callback for F/V dependent extensions makes
sense.

Clément


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v4 03/11] riscv: add ISA parsing for Zca, Zcf, Zcd and Zcb

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 454 bytes --]

On Mon, Apr 29, 2024 at 05:04:56PM +0200, Clément Léger wrote:
> The Zc* standard extension for code reduction introduces new extensions.
> This patch adds support for Zca, Zcf, Zcd and Zcb. Zce, Zcmt and Zcmp
> are left out of this patch since they are targeting microcontrollers/
> embedded CPUs instead of application processors.
> 
> Signed-off-by: Clément Léger <cleger@rivosinc.com>

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v4 08/11] riscv: add ISA extension parsing for Zcmop

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 335 bytes --]

On Mon, Apr 29, 2024 at 05:05:01PM +0200, Clément Léger wrote:
> Add parsing for Zcmop ISA extension which was ratified in commit
> b854a709c00 ("Zcmop is ratified/1.0") of the riscv-isa-manual.
> 
> Signed-off-by: Clément Léger <cleger@rivosinc.com>

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v4 02/11] riscv: add ISA extensions validation

[Clément Léger]

On 30/04/2024 13:44, Conor Dooley wrote:
> On Tue, Apr 30, 2024 at 09:18:47AM +0200, Clément Léger wrote:
>>
>>
>> On 30/04/2024 00:15, Conor Dooley wrote:
>>> On Mon, Apr 29, 2024 at 05:04:55PM +0200, Clément Léger wrote:
>>>> Since a few extensions (Zicbom/Zicboz) already needs validation and
>>>> future ones will need it as well (Zc*) add a validate() callback to
>>>> struct riscv_isa_ext_data. This require to rework the way extensions are
>>>> parsed and split it in two phases. First phase is isa string or isa
>>>> extension list parsing and consists in enabling all the extensions in a
>>>> temporary bitmask without any validation. The second step "resolves" the
>>>> final isa bitmap, handling potential missing dependencies. The mechanism
>>>> is quite simple and simply validate each extension described in the
>>>> temporary bitmap before enabling it in the final isa bitmap. validate()
>>>> callbacks can return either 0 for success, -EPROBEDEFER if extension
>>>> needs to be validated again at next loop. A previous ISA bitmap is kept
>>>> to avoid looping mutliple times if an extension dependencies are never
>>>> satisfied until we reach a stable state. In order to avoid any potential
>>>> infinite looping, allow looping a maximum of the number of extension we
>>>> handle. Zicboz and Zicbom extensions are modified to use this validation
>>>> mechanism.
>>>
>>> Your reply to my last review only talked about part of my comments,
>>> which is usually what you do when you're gonna implement the rest, but
>>> you haven't.
>>> I like the change you've made to shorten looping, but I'd at least like
>>> a response to why a split is not worth doing :)
>>
>> Hi Conor,
>>
>> Missed that point since I was feeling that my solution actually
>> addresses your concerns. Your argument was that there is no reason to
>> loop for Zicbom/Zicboz but that would also apply to Zcf in case we are
>> on RV64 as well (since zcf is not supported on RV64). So for Zcf, that
>> would lead to using both mecanism or additional ifdefery with little to
>> no added value since the current solution actually solves both cases:
>>
>> - We don't have any extra looping if all validation callback returns 0
>> (except the initial one on riscv_isa_ext, which is kind of unavoidable).
>> - Zicbom, Zicboz callbacks will be called only once (which was one of
>> your concern).
>>
>> Adding a second kind of callback for after loop validation would only
>> lead to a bunch of additional macros/ifdefery for extensions with
>> validate() callback, with validate_end() or with both (ie Zcf)). For
>> these reasons, I do not think there is a need for a separate mechanism
>> nor additional callback for such extensions except adding extra code
>> with no real added functionality.
>>
>> AFAIK, the platform driver probing mechanism works the same, the probe()
>> callback is actually called even if for some reason properties are
>> missing from nodes for platform devices and thus the probe() returns
>> -EINVAL or whatever.
>>
>> Hope this answers your question,
> 
> Yeah, pretty much I am happy with just an "it's not worth doing it"
> response. Given it wasn't your first choice, I doubt you're overly happy
> with it either, but I really would like to avoid looping to closure to
> sort out dependencies - particularly on the boot CPU before we bring
> anyone else up, but if the code is now more proactive about breaking
> out, I suppose that'll have to do :)
> I kinda wish we didn't do this at all, but I think we've brought this
> upon ourselves via hwprobe. I'm still on the fence as to whether things
> that are implied need to be handled in this way. I think I'll bring this
> up tomorrow at the weekly call, because so far it's only been you and I
> discussing this really and it's a policy decision that hwprobe-ists
> should be involved in I think.

Hi Conor,

Were you able to discuss that topic ?

> 
> Implied extensions aside, I think we will eventually need this stuff
> anyway, for extensions that make no sense to consider if a config option
> for a dependency is disabled.
> From talking to Eric Biggers the other week about
> riscv_isa_extension_available() I'm of the opinion that we need to do
> better with that interface w.r.t. extension and config dependencies,
> and what seems like a good idea to me at the moment is putting tests for
> IS_ENABLED(RISCV_ISA_FOO) into these validate hooks.
> 
> I'll try to look at the actual implementation here tomorrow.

Did you found time to look at the implementation ?

Thanks,

Clément

> 
> Cheers,
> Conor.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v4 02/11] riscv: add ISA extensions validation

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 5224 bytes --]

On Tue, May 14, 2024 at 09:53:08AM +0200, Clément Léger wrote:
> 
> 
> On 30/04/2024 13:44, Conor Dooley wrote:
> > On Tue, Apr 30, 2024 at 09:18:47AM +0200, Clément Léger wrote:
> >>
> >>
> >> On 30/04/2024 00:15, Conor Dooley wrote:
> >>> On Mon, Apr 29, 2024 at 05:04:55PM +0200, Clément Léger wrote:
> >>>> Since a few extensions (Zicbom/Zicboz) already needs validation and
> >>>> future ones will need it as well (Zc*) add a validate() callback to
> >>>> struct riscv_isa_ext_data. This require to rework the way extensions are
> >>>> parsed and split it in two phases. First phase is isa string or isa
> >>>> extension list parsing and consists in enabling all the extensions in a
> >>>> temporary bitmask without any validation. The second step "resolves" the
> >>>> final isa bitmap, handling potential missing dependencies. The mechanism
> >>>> is quite simple and simply validate each extension described in the
> >>>> temporary bitmap before enabling it in the final isa bitmap. validate()
> >>>> callbacks can return either 0 for success, -EPROBEDEFER if extension
> >>>> needs to be validated again at next loop. A previous ISA bitmap is kept
> >>>> to avoid looping mutliple times if an extension dependencies are never
> >>>> satisfied until we reach a stable state. In order to avoid any potential
> >>>> infinite looping, allow looping a maximum of the number of extension we
> >>>> handle. Zicboz and Zicbom extensions are modified to use this validation
> >>>> mechanism.
> >>>
> >>> Your reply to my last review only talked about part of my comments,
> >>> which is usually what you do when you're gonna implement the rest, but
> >>> you haven't.
> >>> I like the change you've made to shorten looping, but I'd at least like
> >>> a response to why a split is not worth doing :)
> >>
> >> Hi Conor,
> >>
> >> Missed that point since I was feeling that my solution actually
> >> addresses your concerns. Your argument was that there is no reason to
> >> loop for Zicbom/Zicboz but that would also apply to Zcf in case we are
> >> on RV64 as well (since zcf is not supported on RV64). So for Zcf, that
> >> would lead to using both mecanism or additional ifdefery with little to
> >> no added value since the current solution actually solves both cases:
> >>
> >> - We don't have any extra looping if all validation callback returns 0
> >> (except the initial one on riscv_isa_ext, which is kind of unavoidable).
> >> - Zicbom, Zicboz callbacks will be called only once (which was one of
> >> your concern).
> >>
> >> Adding a second kind of callback for after loop validation would only
> >> lead to a bunch of additional macros/ifdefery for extensions with
> >> validate() callback, with validate_end() or with both (ie Zcf)). For
> >> these reasons, I do not think there is a need for a separate mechanism
> >> nor additional callback for such extensions except adding extra code
> >> with no real added functionality.
> >>
> >> AFAIK, the platform driver probing mechanism works the same, the probe()
> >> callback is actually called even if for some reason properties are
> >> missing from nodes for platform devices and thus the probe() returns
> >> -EINVAL or whatever.
> >>
> >> Hope this answers your question,
> > 
> > Yeah, pretty much I am happy with just an "it's not worth doing it"
> > response. Given it wasn't your first choice, I doubt you're overly happy
> > with it either, but I really would like to avoid looping to closure to
> > sort out dependencies - particularly on the boot CPU before we bring
> > anyone else up, but if the code is now more proactive about breaking
> > out, I suppose that'll have to do :)
> > I kinda wish we didn't do this at all, but I think we've brought this
> > upon ourselves via hwprobe. I'm still on the fence as to whether things
> > that are implied need to be handled in this way. I think I'll bring this
> > up tomorrow at the weekly call, because so far it's only been you and I
> > discussing this really and it's a policy decision that hwprobe-ists
> > should be involved in I think.
> 
> Hi Conor,
> 
> Were you able to discuss that topic ?

I realised last night that I'd not got back to this thread and meant to
do that today (I had accidentally deleted it from my mailbox), but I had
a migraine this morning and so didn't.
I did bring it up and IIRC Palmer was of the opinion that we should try
our best to infer extensions.

> > Implied extensions aside, I think we will eventually need this stuff
> > anyway, for extensions that make no sense to consider if a config option
> > for a dependency is disabled.
> > From talking to Eric Biggers the other week about
> > riscv_isa_extension_available() I'm of the opinion that we need to do
> > better with that interface w.r.t. extension and config dependencies,
> > and what seems like a good idea to me at the moment is putting tests for
> > IS_ENABLED(RISCV_ISA_FOO) into these validate hooks.
> > 
> > I'll try to look at the actual implementation here tomorrow.
> 
> Did you found time to look at the implementation ?

No, with the above excuse. I'll try to get to it today or tomorrow...

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v4 02/11] riscv: add ISA extensions validation

[Clément Léger]

On 14/05/2024 14:43, Conor Dooley wrote:
> On Tue, May 14, 2024 at 09:53:08AM +0200, Clément Léger wrote:
>>
>>
>> On 30/04/2024 13:44, Conor Dooley wrote:
>>> On Tue, Apr 30, 2024 at 09:18:47AM +0200, Clément Léger wrote:
>>>>
>>>>
>>>> On 30/04/2024 00:15, Conor Dooley wrote:
>>>>> On Mon, Apr 29, 2024 at 05:04:55PM +0200, Clément Léger wrote:
>>>>>> Since a few extensions (Zicbom/Zicboz) already needs validation and
>>>>>> future ones will need it as well (Zc*) add a validate() callback to
>>>>>> struct riscv_isa_ext_data. This require to rework the way extensions are
>>>>>> parsed and split it in two phases. First phase is isa string or isa
>>>>>> extension list parsing and consists in enabling all the extensions in a
>>>>>> temporary bitmask without any validation. The second step "resolves" the
>>>>>> final isa bitmap, handling potential missing dependencies. The mechanism
>>>>>> is quite simple and simply validate each extension described in the
>>>>>> temporary bitmap before enabling it in the final isa bitmap. validate()
>>>>>> callbacks can return either 0 for success, -EPROBEDEFER if extension
>>>>>> needs to be validated again at next loop. A previous ISA bitmap is kept
>>>>>> to avoid looping mutliple times if an extension dependencies are never
>>>>>> satisfied until we reach a stable state. In order to avoid any potential
>>>>>> infinite looping, allow looping a maximum of the number of extension we
>>>>>> handle. Zicboz and Zicbom extensions are modified to use this validation
>>>>>> mechanism.
>>>>>
>>>>> Your reply to my last review only talked about part of my comments,
>>>>> which is usually what you do when you're gonna implement the rest, but
>>>>> you haven't.
>>>>> I like the change you've made to shorten looping, but I'd at least like
>>>>> a response to why a split is not worth doing :)
>>>>
>>>> Hi Conor,
>>>>
>>>> Missed that point since I was feeling that my solution actually
>>>> addresses your concerns. Your argument was that there is no reason to
>>>> loop for Zicbom/Zicboz but that would also apply to Zcf in case we are
>>>> on RV64 as well (since zcf is not supported on RV64). So for Zcf, that
>>>> would lead to using both mecanism or additional ifdefery with little to
>>>> no added value since the current solution actually solves both cases:
>>>>
>>>> - We don't have any extra looping if all validation callback returns 0
>>>> (except the initial one on riscv_isa_ext, which is kind of unavoidable).
>>>> - Zicbom, Zicboz callbacks will be called only once (which was one of
>>>> your concern).
>>>>
>>>> Adding a second kind of callback for after loop validation would only
>>>> lead to a bunch of additional macros/ifdefery for extensions with
>>>> validate() callback, with validate_end() or with both (ie Zcf)). For
>>>> these reasons, I do not think there is a need for a separate mechanism
>>>> nor additional callback for such extensions except adding extra code
>>>> with no real added functionality.
>>>>
>>>> AFAIK, the platform driver probing mechanism works the same, the probe()
>>>> callback is actually called even if for some reason properties are
>>>> missing from nodes for platform devices and thus the probe() returns
>>>> -EINVAL or whatever.
>>>>
>>>> Hope this answers your question,
>>>
>>> Yeah, pretty much I am happy with just an "it's not worth doing it"
>>> response. Given it wasn't your first choice, I doubt you're overly happy
>>> with it either, but I really would like to avoid looping to closure to
>>> sort out dependencies - particularly on the boot CPU before we bring
>>> anyone else up, but if the code is now more proactive about breaking
>>> out, I suppose that'll have to do :)
>>> I kinda wish we didn't do this at all, but I think we've brought this
>>> upon ourselves via hwprobe. I'm still on the fence as to whether things
>>> that are implied need to be handled in this way. I think I'll bring this
>>> up tomorrow at the weekly call, because so far it's only been you and I
>>> discussing this really and it's a policy decision that hwprobe-ists
>>> should be involved in I think.
>>
>> Hi Conor,
>>
>> Were you able to discuss that topic ?
> 
> I realised last night that I'd not got back to this thread and meant to
> do that today (I had accidentally deleted it from my mailbox), but I had
> a migraine this morning and so didn't.
> I did bring it up and IIRC Palmer was of the opinion that we should try
> our best to infer extensions.
> 
>>> Implied extensions aside, I think we will eventually need this stuff
>>> anyway, for extensions that make no sense to consider if a config option
>>> for a dependency is disabled.
>>> From talking to Eric Biggers the other week about
>>> riscv_isa_extension_available() I'm of the opinion that we need to do
>>> better with that interface w.r.t. extension and config dependencies,
>>> and what seems like a good idea to me at the moment is putting tests for
>>> IS_ENABLED(RISCV_ISA_FOO) into these validate hooks.
>>>
>>> I'll try to look at the actual implementation here tomorrow.
>>
>> Did you found time to look at the implementation ?
> 
> No, with the above excuse. I'll try to get to it today or tomorrow...

No worries, I was on vacation and was just checking if I hadn't missed
anything in the meantime. Take your time ;)

Thanks,

Clément


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v4 02/11] riscv: add ISA extensions validation

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 6299 bytes --]

On Tue, May 14, 2024 at 02:48:01PM +0200, Clément Léger wrote:
> 
> 
> On 14/05/2024 14:43, Conor Dooley wrote:
> > On Tue, May 14, 2024 at 09:53:08AM +0200, Clément Léger wrote:
> >>
> >>
> >> On 30/04/2024 13:44, Conor Dooley wrote:
> >>> On Tue, Apr 30, 2024 at 09:18:47AM +0200, Clément Léger wrote:
> >>>>
> >>>>
> >>>> On 30/04/2024 00:15, Conor Dooley wrote:
> >>>>> On Mon, Apr 29, 2024 at 05:04:55PM +0200, Clément Léger wrote:
> >>>>>> Since a few extensions (Zicbom/Zicboz) already needs validation and
> >>>>>> future ones will need it as well (Zc*) add a validate() callback to
> >>>>>> struct riscv_isa_ext_data. This require to rework the way extensions are
> >>>>>> parsed and split it in two phases. First phase is isa string or isa
> >>>>>> extension list parsing and consists in enabling all the extensions in a
> >>>>>> temporary bitmask without any validation. The second step "resolves" the
> >>>>>> final isa bitmap, handling potential missing dependencies. The mechanism
> >>>>>> is quite simple and simply validate each extension described in the
> >>>>>> temporary bitmap before enabling it in the final isa bitmap. validate()
> >>>>>> callbacks can return either 0 for success, -EPROBEDEFER if extension
> >>>>>> needs to be validated again at next loop. A previous ISA bitmap is kept
> >>>>>> to avoid looping mutliple times if an extension dependencies are never
> >>>>>> satisfied until we reach a stable state. In order to avoid any potential
> >>>>>> infinite looping, allow looping a maximum of the number of extension we
> >>>>>> handle. Zicboz and Zicbom extensions are modified to use this validation
> >>>>>> mechanism.
> >>>>>
> >>>>> Your reply to my last review only talked about part of my comments,
> >>>>> which is usually what you do when you're gonna implement the rest, but
> >>>>> you haven't.
> >>>>> I like the change you've made to shorten looping, but I'd at least like
> >>>>> a response to why a split is not worth doing :)
> >>>>
> >>>> Hi Conor,
> >>>>
> >>>> Missed that point since I was feeling that my solution actually
> >>>> addresses your concerns. Your argument was that there is no reason to
> >>>> loop for Zicbom/Zicboz but that would also apply to Zcf in case we are
> >>>> on RV64 as well (since zcf is not supported on RV64). So for Zcf, that
> >>>> would lead to using both mecanism or additional ifdefery with little to
> >>>> no added value since the current solution actually solves both cases:
> >>>>
> >>>> - We don't have any extra looping if all validation callback returns 0
> >>>> (except the initial one on riscv_isa_ext, which is kind of unavoidable).
> >>>> - Zicbom, Zicboz callbacks will be called only once (which was one of
> >>>> your concern).
> >>>>
> >>>> Adding a second kind of callback for after loop validation would only
> >>>> lead to a bunch of additional macros/ifdefery for extensions with
> >>>> validate() callback, with validate_end() or with both (ie Zcf)). For
> >>>> these reasons, I do not think there is a need for a separate mechanism
> >>>> nor additional callback for such extensions except adding extra code
> >>>> with no real added functionality.
> >>>>
> >>>> AFAIK, the platform driver probing mechanism works the same, the probe()
> >>>> callback is actually called even if for some reason properties are
> >>>> missing from nodes for platform devices and thus the probe() returns
> >>>> -EINVAL or whatever.
> >>>>
> >>>> Hope this answers your question,
> >>>
> >>> Yeah, pretty much I am happy with just an "it's not worth doing it"
> >>> response. Given it wasn't your first choice, I doubt you're overly happy
> >>> with it either, but I really would like to avoid looping to closure to
> >>> sort out dependencies - particularly on the boot CPU before we bring
> >>> anyone else up, but if the code is now more proactive about breaking
> >>> out, I suppose that'll have to do :)
> >>> I kinda wish we didn't do this at all, but I think we've brought this
> >>> upon ourselves via hwprobe. I'm still on the fence as to whether things
> >>> that are implied need to be handled in this way. I think I'll bring this
> >>> up tomorrow at the weekly call, because so far it's only been you and I
> >>> discussing this really and it's a policy decision that hwprobe-ists
> >>> should be involved in I think.
> >>
> >> Hi Conor,
> >>
> >> Were you able to discuss that topic ?
> > 
> > I realised last night that I'd not got back to this thread and meant to
> > do that today (I had accidentally deleted it from my mailbox), but I had
> > a migraine this morning and so didn't.
> > I did bring it up and IIRC Palmer was of the opinion that we should try
> > our best to infer extensions.
> > 
> >>> Implied extensions aside, I think we will eventually need this stuff
> >>> anyway, for extensions that make no sense to consider if a config option
> >>> for a dependency is disabled.
> >>> From talking to Eric Biggers the other week about
> >>> riscv_isa_extension_available() I'm of the opinion that we need to do
> >>> better with that interface w.r.t. extension and config dependencies,
> >>> and what seems like a good idea to me at the moment is putting tests for
> >>> IS_ENABLED(RISCV_ISA_FOO) into these validate hooks.
> >>>
> >>> I'll try to look at the actual implementation here tomorrow.
> >>
> >> Did you found time to look at the implementation ?
> > 
> > No, with the above excuse. I'll try to get to it today or tomorrow...
> 
> No worries, I was on vacation and was just checking if I hadn't missed
> anything in the meantime. Take your time ;)

I forget where we talked about validation for F/V, but I chucked this
together last week in response to another thread of Andy's that was
adding some of the vector subset stuff, because I realised we don't turn
off any of the stuff that depends on vector if vector gets disabled:
https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=riscv-check_vector&id=38050c6858143f43ce2fd04e9824727a7d7731d0

What I've got there doesn't actually work for the vector subsets though,
only for vector itself, because of the probe ordering. Your validate
callback stuff should solve that issue though.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v4 02/11] riscv: add ISA extensions validation

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 16801 bytes --]

On Mon, Apr 29, 2024 at 05:04:55PM +0200, Clément Léger wrote:
> Since a few extensions (Zicbom/Zicboz) already needs validation and
> future ones will need it as well (Zc*) add a validate() callback to
> struct riscv_isa_ext_data. This require to rework the way extensions are
> parsed and split it in two phases. First phase is isa string or isa
> extension list parsing and consists in enabling all the extensions in a
> temporary bitmask without any validation. The second step "resolves" the
> final isa bitmap, handling potential missing dependencies. The mechanism
> is quite simple and simply validate each extension described in the
> temporary bitmap before enabling it in the final isa bitmap. validate()
> callbacks can return either 0 for success, -EPROBEDEFER if extension
> needs to be validated again at next loop. A previous ISA bitmap is kept
> to avoid looping mutliple times if an extension dependencies are never
> satisfied until we reach a stable state. In order to avoid any potential
> infinite looping, allow looping a maximum of the number of extension we
> handle. Zicboz and Zicbom extensions are modified to use this validation
> mechanism.
> 
> Signed-off-by: Clément Léger <cleger@rivosinc.com>
> ---
>  arch/riscv/include/asm/cpufeature.h |   1 +
>  arch/riscv/kernel/cpufeature.c      | 211 ++++++++++++++++------------
>  2 files changed, 126 insertions(+), 86 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
> index 347805446151..000796c2d0b1 100644
> --- a/arch/riscv/include/asm/cpufeature.h
> +++ b/arch/riscv/include/asm/cpufeature.h
> @@ -70,6 +70,7 @@ struct riscv_isa_ext_data {
>  	const char *property;
>  	const unsigned int *subset_ext_ids;
>  	const unsigned int subset_ext_size;
> +	int (*validate)(const struct riscv_isa_ext_data *data, const unsigned long *isa_bitmap);
>  };
>  
>  extern const struct riscv_isa_ext_data riscv_isa_ext[];
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 115ba001f1bc..cb2ffa6c8c33 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -72,51 +72,58 @@ bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, unsigned i
>  }
>  EXPORT_SYMBOL_GPL(__riscv_isa_extension_available);
>  
> -static bool riscv_isa_extension_check(int id)
> +static bool riscv_isa_extension_valid(int id)
>  {
> -	switch (id) {
> -	case RISCV_ISA_EXT_ZICBOM:
> -		if (!riscv_cbom_block_size) {
> -			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("Zicbom disabled as cbom-block-size present, but is not a power-of-2\n");
> -			return false;
> -		}
> -		return true;
> -	case RISCV_ISA_EXT_ZICBOZ:
> -		if (!riscv_cboz_block_size) {
> -			pr_err("Zicboz detected in ISA string, disabling as no cboz-block-size found\n");
> -			return false;
> -		} else if (!is_power_of_2(riscv_cboz_block_size)) {
> -			pr_err("Zicboz disabled as cboz-block-size present, but is not a power-of-2\n");
> -			return false;
> -		}
> -		return true;
> -	case RISCV_ISA_EXT_INVALID:
> -		return false;
> +	return id != RISCV_ISA_EXT_INVALID;
> +}
> +
> +static int riscv_ext_zicbom_validate(const struct riscv_isa_ext_data *data,
> +				     const unsigned long *isa_bitmap)
> +{
> +	if (!riscv_cbom_block_size) {
> +		pr_err("Zicbom detected in ISA string, disabling as no cbom-block-size found\n");
> +		return -EINVAL;
> +	} else if (!is_power_of_2(riscv_cbom_block_size)) {

I guess the original code did this too, but as the branches return the
else here should go.

> +		pr_err("Zicbom disabled as cbom-block-size present, but is not a power-of-2\n");
> +		return -EINVAL;
>  	}
> +	return 0;
> +}
>  
> -	return true;
> +static int riscv_ext_zicboz_validate(const struct riscv_isa_ext_data *data,
> +				     const unsigned long *isa_bitmap)
> +{
> +	if (!riscv_cboz_block_size) {
> +		pr_err("Zicboz detected in ISA string, disabling as no cboz-block-size found\n");
> +		return -EINVAL;
> +	} else if (!is_power_of_2(riscv_cboz_block_size)) {
> +		pr_err("Zicboz disabled as cboz-block-size present, but is not a power-of-2\n");
> +		return -EINVAL;
> +	}
> +	return 0;
>  }
>  
> -#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size) {	\
> -	.name = #_name,								\
> -	.property = #_name,							\
> -	.id = _id,								\
> -	.subset_ext_ids = _subset_exts,						\
> -	.subset_ext_size = _subset_exts_size					\
> +#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size, _validate) {	\
> +	.name = #_name,									\
> +	.property = #_name,								\
> +	.id = _id,									\
> +	.subset_ext_ids = _subset_exts,							\
> +	.subset_ext_size = _subset_exts_size,						\
> +	.validate = _validate								\
>  }
>  
> -#define __RISCV_ISA_EXT_DATA(_name, _id) _RISCV_ISA_EXT_DATA(_name, _id, NULL, 0)
> +#define __RISCV_ISA_EXT_DATA(_name, _id) _RISCV_ISA_EXT_DATA(_name, _id, NULL, 0, NULL)
>  
>  /* Used to declare pure "lasso" extension (Zk for instance) */
>  #define __RISCV_ISA_EXT_BUNDLE(_name, _bundled_exts) \
> -	_RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, _bundled_exts, ARRAY_SIZE(_bundled_exts))
> +	_RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, _bundled_exts, \
> +			    ARRAY_SIZE(_bundled_exts), NULL)
>  
>  /* Used to declare extensions that are a superset of other extensions (Zvbb for instance) */
>  #define __RISCV_ISA_EXT_SUPERSET(_name, _id, _sub_exts) \
> -	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts))
> +	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts), NULL)
> +#define __RISCV_ISA_EXT_SUPERSET_VALIDATE(_name, _id, _sub_exts, _validate) \
> +	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts), _validate)
>  
>  static const unsigned int riscv_zk_bundled_exts[] = {
>  	RISCV_ISA_EXT_ZBKB,
> @@ -247,8 +254,10 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
>  	__RISCV_ISA_EXT_DATA(c, RISCV_ISA_EXT_c),
>  	__RISCV_ISA_EXT_DATA(v, RISCV_ISA_EXT_v),
>  	__RISCV_ISA_EXT_DATA(h, RISCV_ISA_EXT_h),
> -	__RISCV_ISA_EXT_SUPERSET(zicbom, RISCV_ISA_EXT_ZICBOM, riscv_xlinuxenvcfg_exts),
> -	__RISCV_ISA_EXT_SUPERSET(zicboz, RISCV_ISA_EXT_ZICBOZ, riscv_xlinuxenvcfg_exts),
> +	__RISCV_ISA_EXT_SUPERSET_VALIDATE(zicbom, RISCV_ISA_EXT_ZICBOM, riscv_xlinuxenvcfg_exts,
> +					  riscv_ext_zicbom_validate),
> +	__RISCV_ISA_EXT_SUPERSET_VALIDATE(zicboz, RISCV_ISA_EXT_ZICBOZ, riscv_xlinuxenvcfg_exts,
> +					  riscv_ext_zicboz_validate),
>  	__RISCV_ISA_EXT_DATA(zicntr, RISCV_ISA_EXT_ZICNTR),
>  	__RISCV_ISA_EXT_DATA(zicond, RISCV_ISA_EXT_ZICOND),
>  	__RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR),
> @@ -310,33 +319,80 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
>  
>  const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext);
>  
> -static void __init match_isa_ext(const struct riscv_isa_ext_data *ext, const char *name,
> -				 const char *name_end, struct riscv_isainfo *isainfo)
> +static void riscv_isa_set_ext(const struct riscv_isa_ext_data *ext, unsigned long *bitmap)
>  {
> -	if ((name_end - name == strlen(ext->name)) &&
> -	     !strncasecmp(name, ext->name, name_end - name)) {
> -		/*
> -		 * If this is a bundle, enable all the ISA extensions that
> -		 * comprise the bundle.
> -		 */
> -		if (ext->subset_ext_size) {
> -			for (int i = 0; i < ext->subset_ext_size; i++) {
> -				if (riscv_isa_extension_check(ext->subset_ext_ids[i]))
> -					set_bit(ext->subset_ext_ids[i], isainfo->isa);
> -			}
> +	/*
> +	 * This is valid even for bundle extensions which uses the RISCV_ISA_EXT_INVALID id
> +	 * (rejected by riscv_isa_extension_valid()).

I really don't understand what this comment is trying to say.
I think what you're trying to say is that it is safe to call
riscv_isa_extension_valid() for bundles, but wouldn't it just be clearer
to drop the function calls and do the comparison to ..._INVALID here
since riscv_isa_extension_valid() has been reduced to just that single
comparison?

I'd understand this function looking as it did if
riscv_isa_extension_valid() was more than a oneliner.

> +	 */
> +	if (riscv_isa_extension_valid(ext->id))
> +		set_bit(ext->id, bitmap);
> +
> +	for (int i = 0; i < ext->subset_ext_size; i++) {
> +		if (riscv_isa_extension_valid(ext->subset_ext_ids[i]))
> +			set_bit(ext->subset_ext_ids[i], bitmap);
> +	}
> +}
> +
> +static void __init riscv_resolve_isa(unsigned long *isa_bitmap, struct riscv_isainfo *isainfo,
> +				     unsigned long *this_hwcap, unsigned long *isa2hwcap)

This function is badly in need of some new variable names for the first
two parameters. It's hard to follow what each of them is meant to be
once you're inside this function and removed from their definitions.
The first parameter is the source bitmap that we've already filled from
the dt/acpi scan of that hart and the second is the per-hart data
structure that we're gonna assign it to and keep "forever", I think the
naming should reflect that.

> +{
> +	bool loop;
> +	const struct riscv_isa_ext_data *ext;
> +	DECLARE_BITMAP(prev_bitmap, RISCV_ISA_EXT_MAX);
> +	int max_loop_count = riscv_isa_ext_count, ret;
> +
> +	do {
> +		loop = false;
> +		if (max_loop_count-- < 0) {
> +			pr_err("Failed to reach a stable ISA state\n");
> +			return;
>  		}
> +		memcpy(prev_bitmap, isainfo->isa, sizeof(prev_bitmap));

Why not bitmap_copy()?

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

Why would we even be testing extensions that have been disabled rather
than iterating just over the set that has been turned on? IOW, does
for_each_set_bit() work here?

> +			ext = &riscv_isa_ext[i];
> +
> +			/* Bundle extensions ids are invalid*/
> +			if (!riscv_isa_extension_valid(ext->id))
> +				continue;
> +

> +			if (!test_bit(ext->id, isa_bitmap) || test_bit(ext->id, isainfo->isa))
> +				continue;

What's this test excluding? I think this deserves a comment.

> +
> +			if (ext->validate) {
> +				ret = ext->validate(ext, isainfo->isa);
> +				if (ret) {
> +					if (ret == -EPROBE_DEFER)
> +						loop = true;
> +					else
> +						clear_bit(ext->id, isa_bitmap);
> +					continue;
> +				}
> +			}
>  
> -		/*
> -		 * This is valid even for bundle extensions which uses the RISCV_ISA_EXT_INVALID id
> -		 * (rejected by riscv_isa_extension_check()).
> -		 */
> -		if (riscv_isa_extension_check(ext->id))
>  			set_bit(ext->id, isainfo->isa);
> +
> +			/* Only single letter extensions get set in hwcap */
> +			if (ext->id < RISCV_ISA_EXT_BASE)
> +				*this_hwcap |= isa2hwcap[ext->id];
> +		}
> +	} while (loop && memcmp(prev_bitmap, isainfo->isa, sizeof(prev_bitmap)));
> +}
> +
> +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)) {
> +			riscv_isa_set_ext(ext, bitmap);
> +			break;
> +		}
>  	}
>  }
>  
> -static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct riscv_isainfo *isainfo,
> -					  unsigned long *isa2hwcap, const char *isa)
> +static void __init riscv_resolve_isa_string(const char *isa, unsigned long *bitmap)

I don't see why this needs to be renamed, I think the original name here
was fine - and the new name makes the operation of the caller of this
function less clear to me.

>  {
>  	/*
>  	 * For all possible cpus, we have already validated in
> @@ -349,7 +405,7 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc
>  	while (*isa) {
>  		const char *ext = isa++;
>  		const char *ext_end = isa;
> -		bool ext_long = false, ext_err = false;
> +		bool ext_err = false;
>  
>  		switch (*ext) {
>  		case 's':
> @@ -389,7 +445,6 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc
>  			 * character itself while eliminating the extensions version number.
>  			 * A simple re-increment solves this problem.
>  			 */
> -			ext_long = true;
>  			for (; *isa && *isa != '_'; ++isa)
>  				if (unlikely(!isalnum(*isa)))
>  					ext_err = true;
> @@ -469,17 +524,8 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc
>  
>  		if (unlikely(ext_err))
>  			continue;
> -		if (!ext_long) {
> -			int nr = tolower(*ext) - 'a';
>  
> -			if (riscv_isa_extension_check(nr)) {
> -				*this_hwcap |= isa2hwcap[nr];
> -				set_bit(nr, isainfo->isa);
> -			}
> -		} else {
> -			for (int i = 0; i < riscv_isa_ext_count; i++)
> -				match_isa_ext(&riscv_isa_ext[i], ext, ext_end, isainfo);
> -		}
> +		match_isa_ext(ext, ext_end, bitmap);
>  	}
>  }
>  
> @@ -501,6 +547,7 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
>  	for_each_possible_cpu(cpu) {
>  		struct riscv_isainfo *isainfo = &hart_isa[cpu];

I think this code would, and the non-string variant below, benefit from
a similar renaming to make the "flow" of information clearer.

In general tho, this stuff looks sane to me. There's a bunch of moving
pieces at the moment with various extensions, so I hope that some of
them (the vector subsets & the non-vector parts (1-9) of Charlie's series
for vendor stuff maybe) get merged as 6.10 material so that we can
reduce what's in play while we try to add this stuff.

I'll suggest that to Palmer tomorrow I think..

Cheers,
Conor.

>  		unsigned long this_hwcap = 0;
> +		DECLARE_BITMAP(isa_bitmap, RISCV_ISA_EXT_MAX) = { 0 };
>  
>  		if (acpi_disabled) {
>  			node = of_cpu_device_node_get(cpu);
> @@ -523,7 +570,7 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
>  			}
>  		}
>  
> -		riscv_parse_isa_string(&this_hwcap, isainfo, isa2hwcap, isa);
> +		riscv_resolve_isa_string(isa, isa_bitmap);
>  
>  		/*
>  		 * These ones were as they were part of the base ISA when the
> @@ -531,10 +578,10 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
>  		 * unconditionally where `i` is in riscv,isa on DT systems.
>  		 */
>  		if (acpi_disabled) {
> -			set_bit(RISCV_ISA_EXT_ZICSR, isainfo->isa);
> -			set_bit(RISCV_ISA_EXT_ZIFENCEI, isainfo->isa);
> -			set_bit(RISCV_ISA_EXT_ZICNTR, isainfo->isa);
> -			set_bit(RISCV_ISA_EXT_ZIHPM, isainfo->isa);
> +			set_bit(RISCV_ISA_EXT_ZICSR, isa_bitmap);
> +			set_bit(RISCV_ISA_EXT_ZIFENCEI, isa_bitmap);
> +			set_bit(RISCV_ISA_EXT_ZICNTR, isa_bitmap);
> +			set_bit(RISCV_ISA_EXT_ZIHPM, isa_bitmap);
>  		}
>  
>  		/*

> @@ -548,9 +595,11 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
>  		if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID &&
>  		    riscv_cached_marchid(cpu) == 0x0) {
>  			this_hwcap &= ~isa2hwcap[RISCV_ISA_EXT_v];
> -			clear_bit(RISCV_ISA_EXT_v, isainfo->isa);
> +			clear_bit(RISCV_ISA_EXT_v, isa_bitmap);
>  		}
>  
> +		riscv_resolve_isa(isa_bitmap, isainfo, &this_hwcap, isa2hwcap);
> +
>  		/*
>  		 * All "okay" hart should have same isa. Set HWCAP based on
>  		 * common capabilities of every "okay" hart, in case they don't
> @@ -579,6 +628,7 @@ static int __init riscv_fill_hwcap_from_ext_list(unsigned long *isa2hwcap)
>  		unsigned long this_hwcap = 0;
>  		struct device_node *cpu_node;
>  		struct riscv_isainfo *isainfo = &hart_isa[cpu];
> +		DECLARE_BITMAP(isa_bitmap, RISCV_ISA_EXT_MAX) = { 0 };
>  
>  		cpu_node = of_cpu_device_node_get(cpu);
>  		if (!cpu_node) {
> @@ -598,22 +648,11 @@ static int __init riscv_fill_hwcap_from_ext_list(unsigned long *isa2hwcap)
>  						     ext->property) < 0)
>  				continue;
>  
> -			if (ext->subset_ext_size) {
> -				for (int j = 0; j < ext->subset_ext_size; j++) {
> -					if (riscv_isa_extension_check(ext->subset_ext_ids[i]))
> -						set_bit(ext->subset_ext_ids[j], isainfo->isa);
> -				}
> -			}
> -
> -			if (riscv_isa_extension_check(ext->id)) {
> -				set_bit(ext->id, isainfo->isa);
> -
> -				/* Only single letter extensions get set in hwcap */
> -				if (strnlen(riscv_isa_ext[i].name, 2) == 1)
> -					this_hwcap |= isa2hwcap[riscv_isa_ext[i].id];
> -			}
> +			riscv_isa_set_ext(ext, isa_bitmap);
>  		}
>  
> +		riscv_resolve_isa(isa_bitmap, isainfo, &this_hwcap, isa2hwcap);
> +
>  		of_node_put(cpu_node);
>  
>  		/*
> -- 
> 2.43.0
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v4 02/11] riscv: add ISA extensions validation

[Clément Léger]

On 14/05/2024 19:39, Conor Dooley wrote:
> On Mon, Apr 29, 2024 at 05:04:55PM +0200, Clément Léger wrote:
>> Since a few extensions (Zicbom/Zicboz) already needs validation and
>> future ones will need it as well (Zc*) add a validate() callback to
>> struct riscv_isa_ext_data. This require to rework the way extensions are
>> parsed and split it in two phases. First phase is isa string or isa
>> extension list parsing and consists in enabling all the extensions in a
>> temporary bitmask without any validation. The second step "resolves" the
>> final isa bitmap, handling potential missing dependencies. The mechanism
>> is quite simple and simply validate each extension described in the
>> temporary bitmap before enabling it in the final isa bitmap. validate()
>> callbacks can return either 0 for success, -EPROBEDEFER if extension
>> needs to be validated again at next loop. A previous ISA bitmap is kept
>> to avoid looping mutliple times if an extension dependencies are never
>> satisfied until we reach a stable state. In order to avoid any potential
>> infinite looping, allow looping a maximum of the number of extension we
>> handle. Zicboz and Zicbom extensions are modified to use this validation
>> mechanism.
>>
>> Signed-off-by: Clément Léger <cleger@rivosinc.com>
>> ---
>>  arch/riscv/include/asm/cpufeature.h |   1 +
>>  arch/riscv/kernel/cpufeature.c      | 211 ++++++++++++++++------------
>>  2 files changed, 126 insertions(+), 86 deletions(-)
>>
>> diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
>> index 347805446151..000796c2d0b1 100644
>> --- a/arch/riscv/include/asm/cpufeature.h
>> +++ b/arch/riscv/include/asm/cpufeature.h
>> @@ -70,6 +70,7 @@ struct riscv_isa_ext_data {
>>  	const char *property;
>>  	const unsigned int *subset_ext_ids;
>>  	const unsigned int subset_ext_size;
>> +	int (*validate)(const struct riscv_isa_ext_data *data, const unsigned long *isa_bitmap);
>>  };
>>  
>>  extern const struct riscv_isa_ext_data riscv_isa_ext[];
>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>> index 115ba001f1bc..cb2ffa6c8c33 100644
>> --- a/arch/riscv/kernel/cpufeature.c
>> +++ b/arch/riscv/kernel/cpufeature.c
>> @@ -72,51 +72,58 @@ bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, unsigned i
>>  }
>>  EXPORT_SYMBOL_GPL(__riscv_isa_extension_available);
>>  
>> -static bool riscv_isa_extension_check(int id)
>> +static bool riscv_isa_extension_valid(int id)
>>  {
>> -	switch (id) {
>> -	case RISCV_ISA_EXT_ZICBOM:
>> -		if (!riscv_cbom_block_size) {
>> -			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("Zicbom disabled as cbom-block-size present, but is not a power-of-2\n");
>> -			return false;
>> -		}
>> -		return true;
>> -	case RISCV_ISA_EXT_ZICBOZ:
>> -		if (!riscv_cboz_block_size) {
>> -			pr_err("Zicboz detected in ISA string, disabling as no cboz-block-size found\n");
>> -			return false;
>> -		} else if (!is_power_of_2(riscv_cboz_block_size)) {
>> -			pr_err("Zicboz disabled as cboz-block-size present, but is not a power-of-2\n");
>> -			return false;
>> -		}
>> -		return true;
>> -	case RISCV_ISA_EXT_INVALID:
>> -		return false;
>> +	return id != RISCV_ISA_EXT_INVALID;
>> +}
>> +
>> +static int riscv_ext_zicbom_validate(const struct riscv_isa_ext_data *data,
>> +				     const unsigned long *isa_bitmap)
>> +{
>> +	if (!riscv_cbom_block_size) {
>> +		pr_err("Zicbom detected in ISA string, disabling as no cbom-block-size found\n");
>> +		return -EINVAL;
>> +	} else if (!is_power_of_2(riscv_cbom_block_size)) {
> 
> I guess the original code did this too, but as the branches return the
> else here should go.

Indeed.

> 
>> +		pr_err("Zicbom disabled as cbom-block-size present, but is not a power-of-2\n");
>> +		return -EINVAL;
>>  	}
>> +	return 0;
>> +}
>>  
>> -	return true;
>> +static int riscv_ext_zicboz_validate(const struct riscv_isa_ext_data *data,
>> +				     const unsigned long *isa_bitmap)
>> +{
>> +	if (!riscv_cboz_block_size) {
>> +		pr_err("Zicboz detected in ISA string, disabling as no cboz-block-size found\n");
>> +		return -EINVAL;
>> +	} else if (!is_power_of_2(riscv_cboz_block_size)) {
>> +		pr_err("Zicboz disabled as cboz-block-size present, but is not a power-of-2\n");
>> +		return -EINVAL;
>> +	}
>> +	return 0;
>>  }
>>  
>> -#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size) {	\
>> -	.name = #_name,								\
>> -	.property = #_name,							\
>> -	.id = _id,								\
>> -	.subset_ext_ids = _subset_exts,						\
>> -	.subset_ext_size = _subset_exts_size					\
>> +#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size, _validate) {	\
>> +	.name = #_name,									\
>> +	.property = #_name,								\
>> +	.id = _id,									\
>> +	.subset_ext_ids = _subset_exts,							\
>> +	.subset_ext_size = _subset_exts_size,						\
>> +	.validate = _validate								\
>>  }
>>  
>> -#define __RISCV_ISA_EXT_DATA(_name, _id) _RISCV_ISA_EXT_DATA(_name, _id, NULL, 0)
>> +#define __RISCV_ISA_EXT_DATA(_name, _id) _RISCV_ISA_EXT_DATA(_name, _id, NULL, 0, NULL)
>>  
>>  /* Used to declare pure "lasso" extension (Zk for instance) */
>>  #define __RISCV_ISA_EXT_BUNDLE(_name, _bundled_exts) \
>> -	_RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, _bundled_exts, ARRAY_SIZE(_bundled_exts))
>> +	_RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, _bundled_exts, \
>> +			    ARRAY_SIZE(_bundled_exts), NULL)
>>  
>>  /* Used to declare extensions that are a superset of other extensions (Zvbb for instance) */
>>  #define __RISCV_ISA_EXT_SUPERSET(_name, _id, _sub_exts) \
>> -	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts))
>> +	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts), NULL)
>> +#define __RISCV_ISA_EXT_SUPERSET_VALIDATE(_name, _id, _sub_exts, _validate) \
>> +	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts), _validate)
>>  
>>  static const unsigned int riscv_zk_bundled_exts[] = {
>>  	RISCV_ISA_EXT_ZBKB,
>> @@ -247,8 +254,10 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
>>  	__RISCV_ISA_EXT_DATA(c, RISCV_ISA_EXT_c),
>>  	__RISCV_ISA_EXT_DATA(v, RISCV_ISA_EXT_v),
>>  	__RISCV_ISA_EXT_DATA(h, RISCV_ISA_EXT_h),
>> -	__RISCV_ISA_EXT_SUPERSET(zicbom, RISCV_ISA_EXT_ZICBOM, riscv_xlinuxenvcfg_exts),
>> -	__RISCV_ISA_EXT_SUPERSET(zicboz, RISCV_ISA_EXT_ZICBOZ, riscv_xlinuxenvcfg_exts),
>> +	__RISCV_ISA_EXT_SUPERSET_VALIDATE(zicbom, RISCV_ISA_EXT_ZICBOM, riscv_xlinuxenvcfg_exts,
>> +					  riscv_ext_zicbom_validate),
>> +	__RISCV_ISA_EXT_SUPERSET_VALIDATE(zicboz, RISCV_ISA_EXT_ZICBOZ, riscv_xlinuxenvcfg_exts,
>> +					  riscv_ext_zicboz_validate),
>>  	__RISCV_ISA_EXT_DATA(zicntr, RISCV_ISA_EXT_ZICNTR),
>>  	__RISCV_ISA_EXT_DATA(zicond, RISCV_ISA_EXT_ZICOND),
>>  	__RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR),
>> @@ -310,33 +319,80 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
>>  
>>  const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext);
>>  
>> -static void __init match_isa_ext(const struct riscv_isa_ext_data *ext, const char *name,
>> -				 const char *name_end, struct riscv_isainfo *isainfo)
>> +static void riscv_isa_set_ext(const struct riscv_isa_ext_data *ext, unsigned long *bitmap)
>>  {
>> -	if ((name_end - name == strlen(ext->name)) &&
>> -	     !strncasecmp(name, ext->name, name_end - name)) {
>> -		/*
>> -		 * If this is a bundle, enable all the ISA extensions that
>> -		 * comprise the bundle.
>> -		 */
>> -		if (ext->subset_ext_size) {
>> -			for (int i = 0; i < ext->subset_ext_size; i++) {
>> -				if (riscv_isa_extension_check(ext->subset_ext_ids[i]))
>> -					set_bit(ext->subset_ext_ids[i], isainfo->isa);
>> -			}
>> +	/*
>> +	 * This is valid even for bundle extensions which uses the RISCV_ISA_EXT_INVALID id
>> +	 * (rejected by riscv_isa_extension_valid()).
> 
> I really don't understand what this comment is trying to say.
> I think what you're trying to say is that it is safe to call
> riscv_isa_extension_valid() for bundles, but wouldn't it just be clearer
> to drop the function calls and do the comparison to ..._INVALID here
> since riscv_isa_extension_valid() has been reduced to just that single
> comparison?

Yeah, that comment is a remnant of the rpevious code but does not make
sense anymore. I'll remove it along with riscv_isa_extension_valid() and
just compare to RISCV_ISA_EXT_INVALID.

> 
> I'd understand this function looking as it did if
> riscv_isa_extension_valid() was more than a oneliner.
> 
>> +	 */
>> +	if (riscv_isa_extension_valid(ext->id))
>> +		set_bit(ext->id, bitmap);
>> +
>> +	for (int i = 0; i < ext->subset_ext_size; i++) {
>> +		if (riscv_isa_extension_valid(ext->subset_ext_ids[i]))
>> +			set_bit(ext->subset_ext_ids[i], bitmap);
>> +	}
>> +}
>> +
>> +static void __init riscv_resolve_isa(unsigned long *isa_bitmap, struct riscv_isainfo *isainfo,
>> +				     unsigned long *this_hwcap, unsigned long *isa2hwcap)
> 
> This function is badly in need of some new variable names for the first
> two parameters. It's hard to follow what each of them is meant to be
> once you're inside this function and removed from their definitions.
> The first parameter is the source bitmap that we've already filled from
> the dt/acpi scan of that hart and the second is the per-hart data
> structure that we're gonna assign it to and keep "forever", I think the
> naming should reflect that.

Yeah, wasn't sure of the naming at all. Would you be ok with the following:

- source_isa: Input ISA bitmap parsed from ISA string (DT/ACPI)
- resolved_isa: Output ISA bitmap resolved from the first one
(configuration and extension dependencies matching).

Since I'm a non-native english speaker, I'm not sure at all if it
correctly means what they do, feel free to tell me if you have some
better options.

> 
>> +{
>> +	bool loop;
>> +	const struct riscv_isa_ext_data *ext;
>> +	DECLARE_BITMAP(prev_bitmap, RISCV_ISA_EXT_MAX);
>> +	int max_loop_count = riscv_isa_ext_count, ret;
>> +
>> +	do {
>> +		loop = false;
>> +		if (max_loop_count-- < 0) {
>> +			pr_err("Failed to reach a stable ISA state\n");
>> +			return;
>>  		}
>> +		memcpy(prev_bitmap, isainfo->isa, sizeof(prev_bitmap));
> 
> Why not bitmap_copy()?

Not reason at all, just forgot it existed.

> 
>> +		for (int i = 0; i < riscv_isa_ext_count; i++) {
> 
> Why would we even be testing extensions that have been disabled rather
> than iterating just over the set that has been turned on? IOW, does
> for_each_set_bit() work here?

I think the loop can acutally be done the other way (not sure, need to
check thoug) and iterate on isa_bitmap first rather than on extension array.

> 
>> +			ext = &riscv_isa_ext[i];
>> +
>> +			/* Bundle extensions ids are invalid*/
>> +			if (!riscv_isa_extension_valid(ext->id))
>> +				continue;
>> +
> 
>> +			if (!test_bit(ext->id, isa_bitmap) || test_bit(ext->id, isainfo->isa))
>> +				continue;
> 
> What's this test excluding? I think this deserves a comment.

Skips non set extension id in isa bitmap or extensions already enabled
in resolved bitmap. Will be rendered useless if changing the loop order.

> 
>> +
>> +			if (ext->validate) {
>> +				ret = ext->validate(ext, isainfo->isa);
>> +				if (ret) {
>> +					if (ret == -EPROBE_DEFER)
>> +						loop = true;
>> +					else
>> +						clear_bit(ext->id, isa_bitmap);
>> +					continue;
>> +				}
>> +			}
>>  
>> -		/*
>> -		 * This is valid even for bundle extensions which uses the RISCV_ISA_EXT_INVALID id
>> -		 * (rejected by riscv_isa_extension_check()).
>> -		 */
>> -		if (riscv_isa_extension_check(ext->id))
>>  			set_bit(ext->id, isainfo->isa);
>> +
>> +			/* Only single letter extensions get set in hwcap */
>> +			if (ext->id < RISCV_ISA_EXT_BASE)
>> +				*this_hwcap |= isa2hwcap[ext->id];
>> +		}
>> +	} while (loop && memcmp(prev_bitmap, isainfo->isa, sizeof(prev_bitmap)));
>> +}
>> +
>> +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)) {
>> +			riscv_isa_set_ext(ext, bitmap);
>> +			break;
>> +		}
>>  	}
>>  }
>>  
>> -static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct riscv_isainfo *isainfo,
>> -					  unsigned long *isa2hwcap, const char *isa)
>> +static void __init riscv_resolve_isa_string(const char *isa, unsigned long *bitmap)
> 
> I don't see why this needs to be renamed, I think the original name here
> was fine - and the new name makes the operation of the caller of this
> function less clear to me.
> 

Bad renaming from a previous version where it conflicted with a new
function. No reason to keep it as is though, I'll revert that.

>>  {
>>  	/*
>>  	 * For all possible cpus, we have already validated in
>> @@ -349,7 +405,7 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc
>>  	while (*isa) {
>>  		const char *ext = isa++;
>>  		const char *ext_end = isa;
>> -		bool ext_long = false, ext_err = false;
>> +		bool ext_err = false;
>>  
>>  		switch (*ext) {
>>  		case 's':
>> @@ -389,7 +445,6 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc
>>  			 * character itself while eliminating the extensions version number.
>>  			 * A simple re-increment solves this problem.
>>  			 */
>> -			ext_long = true;
>>  			for (; *isa && *isa != '_'; ++isa)
>>  				if (unlikely(!isalnum(*isa)))
>>  					ext_err = true;
>> @@ -469,17 +524,8 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc
>>  
>>  		if (unlikely(ext_err))
>>  			continue;
>> -		if (!ext_long) {
>> -			int nr = tolower(*ext) - 'a';
>>  
>> -			if (riscv_isa_extension_check(nr)) {
>> -				*this_hwcap |= isa2hwcap[nr];
>> -				set_bit(nr, isainfo->isa);
>> -			}
>> -		} else {
>> -			for (int i = 0; i < riscv_isa_ext_count; i++)
>> -				match_isa_ext(&riscv_isa_ext[i], ext, ext_end, isainfo);
>> -		}
>> +		match_isa_ext(ext, ext_end, bitmap);
>>  	}
>>  }
>>  
>> @@ -501,6 +547,7 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
>>  	for_each_possible_cpu(cpu) {
>>  		struct riscv_isainfo *isainfo = &hart_isa[cpu];
> 
> I think this code would, and the non-string variant below, benefit from
> a similar renaming to make the "flow" of information clearer.
> 
> In general tho, this stuff looks sane to me. There's a bunch of moving
> pieces at the moment with various extensions, so I hope that some of
> them (the vector subsets & the non-vector parts (1-9) of Charlie's series
> for vendor stuff maybe) get merged as 6.10 material so that we can
> reduce what's in play while we try to add this stuff.

Yes sure.

Thanks,

Clément

> 
> I'll suggest that to Palmer tomorrow I think..
> 
> Cheers,
> Conor.
> 
>>  		unsigned long this_hwcap = 0;
>> +		DECLARE_BITMAP(isa_bitmap, RISCV_ISA_EXT_MAX) = { 0 };
>>  
>>  		if (acpi_disabled) {
>>  			node = of_cpu_device_node_get(cpu);
>> @@ -523,7 +570,7 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
>>  			}
>>  		}
>>  
>> -		riscv_parse_isa_string(&this_hwcap, isainfo, isa2hwcap, isa);
>> +		riscv_resolve_isa_string(isa, isa_bitmap);
>>  
>>  		/*
>>  		 * These ones were as they were part of the base ISA when the
>> @@ -531,10 +578,10 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
>>  		 * unconditionally where `i` is in riscv,isa on DT systems.
>>  		 */
>>  		if (acpi_disabled) {
>> -			set_bit(RISCV_ISA_EXT_ZICSR, isainfo->isa);
>> -			set_bit(RISCV_ISA_EXT_ZIFENCEI, isainfo->isa);
>> -			set_bit(RISCV_ISA_EXT_ZICNTR, isainfo->isa);
>> -			set_bit(RISCV_ISA_EXT_ZIHPM, isainfo->isa);
>> +			set_bit(RISCV_ISA_EXT_ZICSR, isa_bitmap);
>> +			set_bit(RISCV_ISA_EXT_ZIFENCEI, isa_bitmap);
>> +			set_bit(RISCV_ISA_EXT_ZICNTR, isa_bitmap);
>> +			set_bit(RISCV_ISA_EXT_ZIHPM, isa_bitmap);
>>  		}
>>  
>>  		/*
> 
>> @@ -548,9 +595,11 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
>>  		if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID &&
>>  		    riscv_cached_marchid(cpu) == 0x0) {
>>  			this_hwcap &= ~isa2hwcap[RISCV_ISA_EXT_v];
>> -			clear_bit(RISCV_ISA_EXT_v, isainfo->isa);
>> +			clear_bit(RISCV_ISA_EXT_v, isa_bitmap);
>>  		}
>>  
>> +		riscv_resolve_isa(isa_bitmap, isainfo, &this_hwcap, isa2hwcap);
>> +
>>  		/*
>>  		 * All "okay" hart should have same isa. Set HWCAP based on
>>  		 * common capabilities of every "okay" hart, in case they don't
>> @@ -579,6 +628,7 @@ static int __init riscv_fill_hwcap_from_ext_list(unsigned long *isa2hwcap)
>>  		unsigned long this_hwcap = 0;
>>  		struct device_node *cpu_node;
>>  		struct riscv_isainfo *isainfo = &hart_isa[cpu];
>> +		DECLARE_BITMAP(isa_bitmap, RISCV_ISA_EXT_MAX) = { 0 };
>>  
>>  		cpu_node = of_cpu_device_node_get(cpu);
>>  		if (!cpu_node) {
>> @@ -598,22 +648,11 @@ static int __init riscv_fill_hwcap_from_ext_list(unsigned long *isa2hwcap)
>>  						     ext->property) < 0)
>>  				continue;
>>  
>> -			if (ext->subset_ext_size) {
>> -				for (int j = 0; j < ext->subset_ext_size; j++) {
>> -					if (riscv_isa_extension_check(ext->subset_ext_ids[i]))
>> -						set_bit(ext->subset_ext_ids[j], isainfo->isa);
>> -				}
>> -			}
>> -
>> -			if (riscv_isa_extension_check(ext->id)) {
>> -				set_bit(ext->id, isainfo->isa);
>> -
>> -				/* Only single letter extensions get set in hwcap */
>> -				if (strnlen(riscv_isa_ext[i].name, 2) == 1)
>> -					this_hwcap |= isa2hwcap[riscv_isa_ext[i].id];
>> -			}
>> +			riscv_isa_set_ext(ext, isa_bitmap);
>>  		}
>>  
>> +		riscv_resolve_isa(isa_bitmap, isainfo, &this_hwcap, isa2hwcap);
>> +
>>  		of_node_put(cpu_node);
>>  
>>  		/*
>> -- 
>> 2.43.0
>>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v4 02/11] riscv: add ISA extensions validation

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 1020 bytes --]

On Wed, May 15, 2024 at 03:26:23PM +0200, Clément Léger wrote:
> > This function is badly in need of some new variable names for the first
> > two parameters. It's hard to follow what each of them is meant to be
> > once you're inside this function and removed from their definitions.
> > The first parameter is the source bitmap that we've already filled from
> > the dt/acpi scan of that hart and the second is the per-hart data
> > structure that we're gonna assign it to and keep "forever", I think the
> > naming should reflect that.
> 
> Yeah, wasn't sure of the naming at all. Would you be ok with the following:
> 
> - source_isa: Input ISA bitmap parsed from ISA string (DT/ACPI)
> - resolved_isa: Output ISA bitmap resolved from the first one
> (configuration and extension dependencies matching).
> 
> Since I'm a non-native english speaker, I'm not sure at all if it
> correctly means what they do, feel free to tell me if you have some
> better options.

I think those are fine, thanks.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv