[PATCH v1 0/6] RISC-V: hwprobe: Introduce which-cpus

[PATCH v1 0/6] RISC-V: hwprobe: Introduce which-cpus

[Andrew Jones]

This series introduces a flag for the hwprobe syscall which
effectively reverses its behavior from getting the values of
keys for a set of cpus to getting the cpus for a set of key-value
pairs. The series was originally posted as an RFC[1] and based on
another series. v1 is now only based on a single patch pointed out
with the tag below.

Based-on: https://lore.kernel.org/all/20231010165101.14942-2-ajones@ventanamicro.com/

[1] https://lore.kernel.org/all/20230921125518.175428-7-ajones@ventanamicro.com/

Changes since the RFC:
 - Split hwprobe out of sys_riscv.c into its own file [Palmer]
 - Split the which-cpus functionality out of do_riscv_hwprobe() [Palmer]
 - Rename hwprobe_key_is_map to hwprobe_key_is_bitmask [Evan]
 - Move the homogeneous_cpus logic into the vDSO function [Evan]
 - Rework logic to not need to allocate any memory
 - Honor cpu affinity in the which-cpus selftests utility
 - Picked up some r-b's


Andrew Jones (6):
  RISC-V: hwprobe: Clarify cpus size parameter
  RISC-V: Move the hwprobe syscall to its own file
  RISC-V: hwprobe: Introduce which-cpus flag
  RISC-V: selftests: Statically link hwprobe test
  RISC-V: selftests: Convert hwprobe test to kselftest API
  RISC-V: selftests: Add which-cpus hwprobe test

 Documentation/riscv/hwprobe.rst               |  27 +-
 arch/riscv/include/asm/hwprobe.h              |  24 ++
 arch/riscv/include/uapi/asm/hwprobe.h         |   3 +
 arch/riscv/kernel/Makefile                    |   1 +
 arch/riscv/kernel/sys_hwprobe.c               | 361 ++++++++++++++++++
 arch/riscv/kernel/sys_riscv.c                 | 267 -------------
 arch/riscv/kernel/vdso/hwprobe.c              |  74 +++-
 .../testing/selftests/riscv/hwprobe/Makefile  |   7 +-
 .../testing/selftests/riscv/hwprobe/hwprobe.c |  64 +---
 .../testing/selftests/riscv/hwprobe/hwprobe.h |  15 +
 .../selftests/riscv/hwprobe/which-cpus.c      | 154 ++++++++
 .../selftests/riscv/vector/vstate_prctl.c     |  10 +-
 12 files changed, 667 insertions(+), 340 deletions(-)
 create mode 100644 arch/riscv/kernel/sys_hwprobe.c
 create mode 100644 tools/testing/selftests/riscv/hwprobe/hwprobe.h
 create mode 100644 tools/testing/selftests/riscv/hwprobe/which-cpus.c

-- 
2.41.0


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

[PATCH v1 1/6] RISC-V: hwprobe: Clarify cpus size parameter

[Andrew Jones]

The "count" parameter associated with the 'cpus' parameter of the
hwprobe syscall is the size in bytes of 'cpus'. Naming it 'cpu_count'
may mislead users (it did me) to think it's the number of CPUs that
are or can be represented by 'cpus' instead. This is particularly
easy (IMO) to get wrong since 'cpus' is documented to be defined by
CPU_SET(3) and CPU_SET(3) also documents a CPU_COUNT() (the number
of CPUs in set) macro. CPU_SET(3) refers to the size of cpu sets
with 'setsize'. Adopt 'cpusetsize' for the hwprobe parameter and
specifically state it is in bytes in Documentation/riscv/hwprobe.rst
to clarify.

Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>
Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 Documentation/riscv/hwprobe.rst  | 15 ++++++++-------
 arch/riscv/kernel/sys_riscv.c    | 14 +++++++-------
 arch/riscv/kernel/vdso/hwprobe.c | 10 +++++-----
 3 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/Documentation/riscv/hwprobe.rst b/Documentation/riscv/hwprobe.rst
index a52996b22f75..c57437e40ffb 100644
--- a/Documentation/riscv/hwprobe.rst
+++ b/Documentation/riscv/hwprobe.rst
@@ -12,7 +12,7 @@ is defined in <asm/hwprobe.h>::
     };
 
     long sys_riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
-                           size_t cpu_count, cpu_set_t *cpus,
+                           size_t cpusetsize, cpu_set_t *cpus,
                            unsigned int flags);
 
 The arguments are split into three groups: an array of key-value pairs, a CPU
@@ -20,12 +20,13 @@ set, and some flags. The key-value pairs are supplied with a count. Userspace
 must prepopulate the key field for each element, and the kernel will fill in the
 value if the key is recognized. If a key is unknown to the kernel, its key field
 will be cleared to -1, and its value set to 0. The CPU set is defined by
-CPU_SET(3). For value-like keys (eg. vendor/arch/impl), the returned value will
-be only be valid if all CPUs in the given set have the same value. Otherwise -1
-will be returned. For boolean-like keys, the value returned will be a logical
-AND of the values for the specified CPUs. Usermode can supply NULL for cpus and
-0 for cpu_count as a shortcut for all online CPUs. There are currently no flags,
-this value must be zero for future compatibility.
+CPU_SET(3) with size ``cpusetsize`` bytes. For value-like keys (eg. vendor,
+arch, impl), the returned value will only be valid if all CPUs in the given set
+have the same value. Otherwise -1 will be returned. For boolean-like keys, the
+value returned will be a logical AND of the values for the specified CPUs.
+Usermode can supply NULL for ``cpus`` and 0 for ``cpusetsize`` as a shortcut for
+all online CPUs. There are currently no flags, this value must be zero for
+future compatibility.
 
 On success 0 is returned, on failure a negative error code is returned.
 
diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
index 473159b5f303..ed3545eb1b2b 100644
--- a/arch/riscv/kernel/sys_riscv.c
+++ b/arch/riscv/kernel/sys_riscv.c
@@ -228,7 +228,7 @@ static void hwprobe_one_pair(struct riscv_hwprobe *pair,
 }
 
 static int do_riscv_hwprobe(struct riscv_hwprobe __user *pairs,
-			    size_t pair_count, size_t cpu_count,
+			    size_t pair_count, size_t cpusetsize,
 			    unsigned long __user *cpus_user,
 			    unsigned int flags)
 {
@@ -246,13 +246,13 @@ static int do_riscv_hwprobe(struct riscv_hwprobe __user *pairs,
 	 * 0 as a shortcut to all online CPUs.
 	 */
 	cpumask_clear(&cpus);
-	if (!cpu_count && !cpus_user) {
+	if (!cpusetsize && !cpus_user) {
 		cpumask_copy(&cpus, cpu_online_mask);
 	} else {
-		if (cpu_count > cpumask_size())
-			cpu_count = cpumask_size();
+		if (cpusetsize > cpumask_size())
+			cpusetsize = cpumask_size();
 
-		ret = copy_from_user(&cpus, cpus_user, cpu_count);
+		ret = copy_from_user(&cpus, cpus_user, cpusetsize);
 		if (ret)
 			return -EFAULT;
 
@@ -329,10 +329,10 @@ arch_initcall_sync(init_hwprobe_vdso_data);
 #endif /* CONFIG_MMU */
 
 SYSCALL_DEFINE5(riscv_hwprobe, struct riscv_hwprobe __user *, pairs,
-		size_t, pair_count, size_t, cpu_count, unsigned long __user *,
+		size_t, pair_count, size_t, cpusetsize, unsigned long __user *,
 		cpus, unsigned int, flags)
 {
-	return do_riscv_hwprobe(pairs, pair_count, cpu_count,
+	return do_riscv_hwprobe(pairs, pair_count, cpusetsize,
 				cpus, flags);
 }
 
diff --git a/arch/riscv/kernel/vdso/hwprobe.c b/arch/riscv/kernel/vdso/hwprobe.c
index cadf725ef798..026b7645c5ab 100644
--- a/arch/riscv/kernel/vdso/hwprobe.c
+++ b/arch/riscv/kernel/vdso/hwprobe.c
@@ -8,21 +8,21 @@
 #include <vdso/helpers.h>
 
 extern int riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
-			 size_t cpu_count, unsigned long *cpus,
+			 size_t cpusetsize, unsigned long *cpus,
 			 unsigned int flags);
 
 /* Add a prototype to avoid -Wmissing-prototypes warning. */
 int __vdso_riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
-			 size_t cpu_count, unsigned long *cpus,
+			 size_t cpusetsize, unsigned long *cpus,
 			 unsigned int flags);
 
 int __vdso_riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
-			 size_t cpu_count, unsigned long *cpus,
+			 size_t cpusetsize, unsigned long *cpus,
 			 unsigned int flags)
 {
 	const struct vdso_data *vd = __arch_get_vdso_data();
 	const struct arch_vdso_data *avd = &vd->arch_data;
-	bool all_cpus = !cpu_count && !cpus;
+	bool all_cpus = !cpusetsize && !cpus;
 	struct riscv_hwprobe *p = pairs;
 	struct riscv_hwprobe *end = pairs + pair_count;
 
@@ -33,7 +33,7 @@ int __vdso_riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
 	 * masks.
 	 */
 	if ((flags != 0) || (!all_cpus && !avd->homogeneous_cpus))
-		return riscv_hwprobe(pairs, pair_count, cpu_count, cpus, flags);
+		return riscv_hwprobe(pairs, pair_count, cpusetsize, cpus, flags);
 
 	/* This is something we can handle, fill out the pairs. */
 	while (p < end) {
-- 
2.41.0


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

[PATCH v1 2/6] RISC-V: Move the hwprobe syscall to its own file

[Andrew Jones]

As Palmer says, hwprobe is "sort of its own thing now, and it's only
going to get bigger..."

Suggested-by: Palmer Dabbelt <palmer@dabbelt.com>
Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 arch/riscv/kernel/Makefile      |   1 +
 arch/riscv/kernel/sys_hwprobe.c | 276 ++++++++++++++++++++++++++++++++
 arch/riscv/kernel/sys_riscv.c   | 267 ------------------------------
 3 files changed, 277 insertions(+), 267 deletions(-)
 create mode 100644 arch/riscv/kernel/sys_hwprobe.c

diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
index 95cf25d48405..db7a43211b30 100644
--- a/arch/riscv/kernel/Makefile
+++ b/arch/riscv/kernel/Makefile
@@ -50,6 +50,7 @@ obj-y	+= setup.o
 obj-y	+= signal.o
 obj-y	+= syscall_table.o
 obj-y	+= sys_riscv.o
+obj-y	+= sys_hwprobe.o
 obj-y	+= time.o
 obj-y	+= traps.o
 obj-y	+= riscv_ksyms.o
diff --git a/arch/riscv/kernel/sys_hwprobe.c b/arch/riscv/kernel/sys_hwprobe.c
new file mode 100644
index 000000000000..69ad5f793374
--- /dev/null
+++ b/arch/riscv/kernel/sys_hwprobe.c
@@ -0,0 +1,276 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * The hwprobe interface, for allowing userspace to probe to see which features
+ * are supported by the hardware.  See Documentation/riscv/hwprobe.rst for more
+ * details.
+ *
+ * Copyright (C) 2012 Regents of the University of California
+ * Copyright (C) 2014 Darius Rad <darius@bluespec.com>
+ * Copyright (C) 2017 SiFive
+ */
+
+#include <linux/syscalls.h>
+#include <asm/cacheflush.h>
+#include <asm/cpufeature.h>
+#include <asm/hwprobe.h>
+#include <asm/sbi.h>
+#include <asm/switch_to.h>
+#include <asm/uaccess.h>
+#include <asm/unistd.h>
+#include <asm/vector.h>
+#include <vdso/vsyscall.h>
+
+
+static void hwprobe_arch_id(struct riscv_hwprobe *pair,
+			    const struct cpumask *cpus)
+{
+	u64 id = -1ULL;
+	bool first = true;
+	int cpu;
+
+	for_each_cpu(cpu, cpus) {
+		u64 cpu_id;
+
+		switch (pair->key) {
+		case RISCV_HWPROBE_KEY_MVENDORID:
+			cpu_id = riscv_cached_mvendorid(cpu);
+			break;
+		case RISCV_HWPROBE_KEY_MIMPID:
+			cpu_id = riscv_cached_mimpid(cpu);
+			break;
+		case RISCV_HWPROBE_KEY_MARCHID:
+			cpu_id = riscv_cached_marchid(cpu);
+			break;
+		}
+
+		if (first) {
+			id = cpu_id;
+			first = false;
+		}
+
+		/*
+		 * If there's a mismatch for the given set, return -1 in the
+		 * value.
+		 */
+		if (id != cpu_id) {
+			id = -1ULL;
+			break;
+		}
+	}
+
+	pair->value = id;
+}
+
+static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
+			     const struct cpumask *cpus)
+{
+	int cpu;
+	u64 missing = 0;
+
+	pair->value = 0;
+	if (has_fpu())
+		pair->value |= RISCV_HWPROBE_IMA_FD;
+
+	if (riscv_isa_extension_available(NULL, c))
+		pair->value |= RISCV_HWPROBE_IMA_C;
+
+	if (has_vector())
+		pair->value |= RISCV_HWPROBE_IMA_V;
+
+	/*
+	 * Loop through and record extensions that 1) anyone has, and 2) anyone
+	 * doesn't have.
+	 */
+	for_each_cpu(cpu, cpus) {
+		struct riscv_isainfo *isainfo = &hart_isa[cpu];
+
+		if (riscv_isa_extension_available(isainfo->isa, ZBA))
+			pair->value |= RISCV_HWPROBE_EXT_ZBA;
+		else
+			missing |= RISCV_HWPROBE_EXT_ZBA;
+
+		if (riscv_isa_extension_available(isainfo->isa, ZBB))
+			pair->value |= RISCV_HWPROBE_EXT_ZBB;
+		else
+			missing |= RISCV_HWPROBE_EXT_ZBB;
+
+		if (riscv_isa_extension_available(isainfo->isa, ZBS))
+			pair->value |= RISCV_HWPROBE_EXT_ZBS;
+		else
+			missing |= RISCV_HWPROBE_EXT_ZBS;
+	}
+
+	/* Now turn off reporting features if any CPU is missing it. */
+	pair->value &= ~missing;
+}
+
+static u64 hwprobe_misaligned(const struct cpumask *cpus)
+{
+	int cpu;
+	u64 perf = -1ULL;
+
+	for_each_cpu(cpu, cpus) {
+		int this_perf = per_cpu(misaligned_access_speed, cpu);
+
+		if (perf == -1ULL)
+			perf = this_perf;
+
+		if (perf != this_perf) {
+			perf = RISCV_HWPROBE_MISALIGNED_UNKNOWN;
+			break;
+		}
+	}
+
+	if (perf == -1ULL)
+		return RISCV_HWPROBE_MISALIGNED_UNKNOWN;
+
+	return perf;
+}
+
+static void hwprobe_one_pair(struct riscv_hwprobe *pair,
+			     const struct cpumask *cpus)
+{
+	switch (pair->key) {
+	case RISCV_HWPROBE_KEY_MVENDORID:
+	case RISCV_HWPROBE_KEY_MARCHID:
+	case RISCV_HWPROBE_KEY_MIMPID:
+		hwprobe_arch_id(pair, cpus);
+		break;
+	/*
+	 * The kernel already assumes that the base single-letter ISA
+	 * extensions are supported on all harts, and only supports the
+	 * IMA base, so just cheat a bit here and tell that to
+	 * userspace.
+	 */
+	case RISCV_HWPROBE_KEY_BASE_BEHAVIOR:
+		pair->value = RISCV_HWPROBE_BASE_BEHAVIOR_IMA;
+		break;
+
+	case RISCV_HWPROBE_KEY_IMA_EXT_0:
+		hwprobe_isa_ext0(pair, cpus);
+		break;
+
+	case RISCV_HWPROBE_KEY_CPUPERF_0:
+		pair->value = hwprobe_misaligned(cpus);
+		break;
+
+	/*
+	 * For forward compatibility, unknown keys don't fail the whole
+	 * call, but get their element key set to -1 and value set to 0
+	 * indicating they're unrecognized.
+	 */
+	default:
+		pair->key = -1;
+		pair->value = 0;
+		break;
+	}
+}
+
+static int do_riscv_hwprobe(struct riscv_hwprobe __user *pairs,
+			    size_t pair_count, size_t cpusetsize,
+			    unsigned long __user *cpus_user,
+			    unsigned int flags)
+{
+	size_t out;
+	int ret;
+	cpumask_t cpus;
+
+	/* Check the reserved flags. */
+	if (flags != 0)
+		return -EINVAL;
+
+	/*
+	 * The interface supports taking in a CPU mask, and returns values that
+	 * are consistent across that mask. Allow userspace to specify NULL and
+	 * 0 as a shortcut to all online CPUs.
+	 */
+	cpumask_clear(&cpus);
+	if (!cpusetsize && !cpus_user) {
+		cpumask_copy(&cpus, cpu_online_mask);
+	} else {
+		if (cpusetsize > cpumask_size())
+			cpusetsize = cpumask_size();
+
+		ret = copy_from_user(&cpus, cpus_user, cpusetsize);
+		if (ret)
+			return -EFAULT;
+
+		/*
+		 * Userspace must provide at least one online CPU, without that
+		 * there's no way to define what is supported.
+		 */
+		cpumask_and(&cpus, &cpus, cpu_online_mask);
+		if (cpumask_empty(&cpus))
+			return -EINVAL;
+	}
+
+	for (out = 0; out < pair_count; out++, pairs++) {
+		struct riscv_hwprobe pair;
+
+		if (get_user(pair.key, &pairs->key))
+			return -EFAULT;
+
+		pair.value = 0;
+		hwprobe_one_pair(&pair, &cpus);
+		ret = put_user(pair.key, &pairs->key);
+		if (ret == 0)
+			ret = put_user(pair.value, &pairs->value);
+
+		if (ret)
+			return -EFAULT;
+	}
+
+	return 0;
+}
+
+#ifdef CONFIG_MMU
+
+static int __init init_hwprobe_vdso_data(void)
+{
+	struct vdso_data *vd = __arch_get_k_vdso_data();
+	struct arch_vdso_data *avd = &vd->arch_data;
+	u64 id_bitsmash = 0;
+	struct riscv_hwprobe pair;
+	int key;
+
+	/*
+	 * Initialize vDSO data with the answers for the "all CPUs" case, to
+	 * save a syscall in the common case.
+	 */
+	for (key = 0; key <= RISCV_HWPROBE_MAX_KEY; key++) {
+		pair.key = key;
+		hwprobe_one_pair(&pair, cpu_online_mask);
+
+		WARN_ON_ONCE(pair.key < 0);
+
+		avd->all_cpu_hwprobe_values[key] = pair.value;
+		/*
+		 * Smash together the vendor, arch, and impl IDs to see if
+		 * they're all 0 or any negative.
+		 */
+		if (key <= RISCV_HWPROBE_KEY_MIMPID)
+			id_bitsmash |= pair.value;
+	}
+
+	/*
+	 * If the arch, vendor, and implementation ID are all the same across
+	 * all harts, then assume all CPUs are the same, and allow the vDSO to
+	 * answer queries for arbitrary masks. However if all values are 0 (not
+	 * populated) or any value returns -1 (varies across CPUs), then the
+	 * vDSO should defer to the kernel for exotic cpu masks.
+	 */
+	avd->homogeneous_cpus = id_bitsmash != 0 && id_bitsmash != -1;
+	return 0;
+}
+
+arch_initcall_sync(init_hwprobe_vdso_data);
+
+#endif /* CONFIG_MMU */
+
+SYSCALL_DEFINE5(riscv_hwprobe, struct riscv_hwprobe __user *, pairs,
+		size_t, pair_count, size_t, cpusetsize, unsigned long __user *,
+		cpus, unsigned int, flags)
+{
+	return do_riscv_hwprobe(pairs, pair_count, cpusetsize,
+				cpus, flags);
+}
diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
index ed3545eb1b2b..f1c1416a9f1e 100644
--- a/arch/riscv/kernel/sys_riscv.c
+++ b/arch/riscv/kernel/sys_riscv.c
@@ -7,15 +7,7 @@
 
 #include <linux/syscalls.h>
 #include <asm/cacheflush.h>
-#include <asm/cpufeature.h>
-#include <asm/hwprobe.h>
-#include <asm/sbi.h>
-#include <asm/vector.h>
-#include <asm/switch_to.h>
-#include <asm/uaccess.h>
-#include <asm/unistd.h>
 #include <asm-generic/mman-common.h>
-#include <vdso/vsyscall.h>
 
 static long riscv_sys_mmap(unsigned long addr, unsigned long len,
 			   unsigned long prot, unsigned long flags,
@@ -77,265 +69,6 @@ SYSCALL_DEFINE3(riscv_flush_icache, uintptr_t, start, uintptr_t, end,
 	return 0;
 }
 
-/*
- * The hwprobe interface, for allowing userspace to probe to see which features
- * are supported by the hardware.  See Documentation/riscv/hwprobe.rst for more
- * details.
- */
-static void hwprobe_arch_id(struct riscv_hwprobe *pair,
-			    const struct cpumask *cpus)
-{
-	u64 id = -1ULL;
-	bool first = true;
-	int cpu;
-
-	for_each_cpu(cpu, cpus) {
-		u64 cpu_id;
-
-		switch (pair->key) {
-		case RISCV_HWPROBE_KEY_MVENDORID:
-			cpu_id = riscv_cached_mvendorid(cpu);
-			break;
-		case RISCV_HWPROBE_KEY_MIMPID:
-			cpu_id = riscv_cached_mimpid(cpu);
-			break;
-		case RISCV_HWPROBE_KEY_MARCHID:
-			cpu_id = riscv_cached_marchid(cpu);
-			break;
-		}
-
-		if (first) {
-			id = cpu_id;
-			first = false;
-		}
-
-		/*
-		 * If there's a mismatch for the given set, return -1 in the
-		 * value.
-		 */
-		if (id != cpu_id) {
-			id = -1ULL;
-			break;
-		}
-	}
-
-	pair->value = id;
-}
-
-static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
-			     const struct cpumask *cpus)
-{
-	int cpu;
-	u64 missing = 0;
-
-	pair->value = 0;
-	if (has_fpu())
-		pair->value |= RISCV_HWPROBE_IMA_FD;
-
-	if (riscv_isa_extension_available(NULL, c))
-		pair->value |= RISCV_HWPROBE_IMA_C;
-
-	if (has_vector())
-		pair->value |= RISCV_HWPROBE_IMA_V;
-
-	/*
-	 * Loop through and record extensions that 1) anyone has, and 2) anyone
-	 * doesn't have.
-	 */
-	for_each_cpu(cpu, cpus) {
-		struct riscv_isainfo *isainfo = &hart_isa[cpu];
-
-		if (riscv_isa_extension_available(isainfo->isa, ZBA))
-			pair->value |= RISCV_HWPROBE_EXT_ZBA;
-		else
-			missing |= RISCV_HWPROBE_EXT_ZBA;
-
-		if (riscv_isa_extension_available(isainfo->isa, ZBB))
-			pair->value |= RISCV_HWPROBE_EXT_ZBB;
-		else
-			missing |= RISCV_HWPROBE_EXT_ZBB;
-
-		if (riscv_isa_extension_available(isainfo->isa, ZBS))
-			pair->value |= RISCV_HWPROBE_EXT_ZBS;
-		else
-			missing |= RISCV_HWPROBE_EXT_ZBS;
-	}
-
-	/* Now turn off reporting features if any CPU is missing it. */
-	pair->value &= ~missing;
-}
-
-static u64 hwprobe_misaligned(const struct cpumask *cpus)
-{
-	int cpu;
-	u64 perf = -1ULL;
-
-	for_each_cpu(cpu, cpus) {
-		int this_perf = per_cpu(misaligned_access_speed, cpu);
-
-		if (perf == -1ULL)
-			perf = this_perf;
-
-		if (perf != this_perf) {
-			perf = RISCV_HWPROBE_MISALIGNED_UNKNOWN;
-			break;
-		}
-	}
-
-	if (perf == -1ULL)
-		return RISCV_HWPROBE_MISALIGNED_UNKNOWN;
-
-	return perf;
-}
-
-static void hwprobe_one_pair(struct riscv_hwprobe *pair,
-			     const struct cpumask *cpus)
-{
-	switch (pair->key) {
-	case RISCV_HWPROBE_KEY_MVENDORID:
-	case RISCV_HWPROBE_KEY_MARCHID:
-	case RISCV_HWPROBE_KEY_MIMPID:
-		hwprobe_arch_id(pair, cpus);
-		break;
-	/*
-	 * The kernel already assumes that the base single-letter ISA
-	 * extensions are supported on all harts, and only supports the
-	 * IMA base, so just cheat a bit here and tell that to
-	 * userspace.
-	 */
-	case RISCV_HWPROBE_KEY_BASE_BEHAVIOR:
-		pair->value = RISCV_HWPROBE_BASE_BEHAVIOR_IMA;
-		break;
-
-	case RISCV_HWPROBE_KEY_IMA_EXT_0:
-		hwprobe_isa_ext0(pair, cpus);
-		break;
-
-	case RISCV_HWPROBE_KEY_CPUPERF_0:
-		pair->value = hwprobe_misaligned(cpus);
-		break;
-
-	/*
-	 * For forward compatibility, unknown keys don't fail the whole
-	 * call, but get their element key set to -1 and value set to 0
-	 * indicating they're unrecognized.
-	 */
-	default:
-		pair->key = -1;
-		pair->value = 0;
-		break;
-	}
-}
-
-static int do_riscv_hwprobe(struct riscv_hwprobe __user *pairs,
-			    size_t pair_count, size_t cpusetsize,
-			    unsigned long __user *cpus_user,
-			    unsigned int flags)
-{
-	size_t out;
-	int ret;
-	cpumask_t cpus;
-
-	/* Check the reserved flags. */
-	if (flags != 0)
-		return -EINVAL;
-
-	/*
-	 * The interface supports taking in a CPU mask, and returns values that
-	 * are consistent across that mask. Allow userspace to specify NULL and
-	 * 0 as a shortcut to all online CPUs.
-	 */
-	cpumask_clear(&cpus);
-	if (!cpusetsize && !cpus_user) {
-		cpumask_copy(&cpus, cpu_online_mask);
-	} else {
-		if (cpusetsize > cpumask_size())
-			cpusetsize = cpumask_size();
-
-		ret = copy_from_user(&cpus, cpus_user, cpusetsize);
-		if (ret)
-			return -EFAULT;
-
-		/*
-		 * Userspace must provide at least one online CPU, without that
-		 * there's no way to define what is supported.
-		 */
-		cpumask_and(&cpus, &cpus, cpu_online_mask);
-		if (cpumask_empty(&cpus))
-			return -EINVAL;
-	}
-
-	for (out = 0; out < pair_count; out++, pairs++) {
-		struct riscv_hwprobe pair;
-
-		if (get_user(pair.key, &pairs->key))
-			return -EFAULT;
-
-		pair.value = 0;
-		hwprobe_one_pair(&pair, &cpus);
-		ret = put_user(pair.key, &pairs->key);
-		if (ret == 0)
-			ret = put_user(pair.value, &pairs->value);
-
-		if (ret)
-			return -EFAULT;
-	}
-
-	return 0;
-}
-
-#ifdef CONFIG_MMU
-
-static int __init init_hwprobe_vdso_data(void)
-{
-	struct vdso_data *vd = __arch_get_k_vdso_data();
-	struct arch_vdso_data *avd = &vd->arch_data;
-	u64 id_bitsmash = 0;
-	struct riscv_hwprobe pair;
-	int key;
-
-	/*
-	 * Initialize vDSO data with the answers for the "all CPUs" case, to
-	 * save a syscall in the common case.
-	 */
-	for (key = 0; key <= RISCV_HWPROBE_MAX_KEY; key++) {
-		pair.key = key;
-		hwprobe_one_pair(&pair, cpu_online_mask);
-
-		WARN_ON_ONCE(pair.key < 0);
-
-		avd->all_cpu_hwprobe_values[key] = pair.value;
-		/*
-		 * Smash together the vendor, arch, and impl IDs to see if
-		 * they're all 0 or any negative.
-		 */
-		if (key <= RISCV_HWPROBE_KEY_MIMPID)
-			id_bitsmash |= pair.value;
-	}
-
-	/*
-	 * If the arch, vendor, and implementation ID are all the same across
-	 * all harts, then assume all CPUs are the same, and allow the vDSO to
-	 * answer queries for arbitrary masks. However if all values are 0 (not
-	 * populated) or any value returns -1 (varies across CPUs), then the
-	 * vDSO should defer to the kernel for exotic cpu masks.
-	 */
-	avd->homogeneous_cpus = id_bitsmash != 0 && id_bitsmash != -1;
-	return 0;
-}
-
-arch_initcall_sync(init_hwprobe_vdso_data);
-
-#endif /* CONFIG_MMU */
-
-SYSCALL_DEFINE5(riscv_hwprobe, struct riscv_hwprobe __user *, pairs,
-		size_t, pair_count, size_t, cpusetsize, unsigned long __user *,
-		cpus, unsigned int, flags)
-{
-	return do_riscv_hwprobe(pairs, pair_count, cpusetsize,
-				cpus, flags);
-}
-
 /* Not defined using SYSCALL_DEFINE0 to avoid error injection */
 asmlinkage long __riscv_sys_ni_syscall(const struct pt_regs *__unused)
 {
-- 
2.41.0


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

[PATCH v1 3/6] RISC-V: hwprobe: Introduce which-cpus flag

[Andrew Jones]

Introduce the first flag for the hwprobe syscall. The flag basically
reverses its behavior, i.e. instead of populating the values of keys
for a given set of cpus, the set of cpus after the call is the result
of finding a set which supports the values of the keys. In order to
do this, we implement a pair compare function which takes the type of
value (a single value vs. a bitmask of booleans) into consideration.
We also implement vdso support for the new flag.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 Documentation/riscv/hwprobe.rst       | 16 ++++-
 arch/riscv/include/asm/hwprobe.h      | 24 +++++++
 arch/riscv/include/uapi/asm/hwprobe.h |  3 +
 arch/riscv/kernel/sys_hwprobe.c       | 93 +++++++++++++++++++++++++--
 arch/riscv/kernel/vdso/hwprobe.c      | 68 +++++++++++++++++---
 5 files changed, 190 insertions(+), 14 deletions(-)

diff --git a/Documentation/riscv/hwprobe.rst b/Documentation/riscv/hwprobe.rst
index c57437e40ffb..576aa03f56bb 100644
--- a/Documentation/riscv/hwprobe.rst
+++ b/Documentation/riscv/hwprobe.rst
@@ -25,8 +25,20 @@ arch, impl), the returned value will only be valid if all CPUs in the given set
 have the same value. Otherwise -1 will be returned. For boolean-like keys, the
 value returned will be a logical AND of the values for the specified CPUs.
 Usermode can supply NULL for ``cpus`` and 0 for ``cpusetsize`` as a shortcut for
-all online CPUs. There are currently no flags, this value must be zero for
-future compatibility.
+all online CPUs. The currently supported flags are:
+
+* :c:macro:`RISCV_HWPROBE_WHICH_CPUS`: This flag basically reverses the behavior
+  of sys_riscv_hwprobe().  Instead of populating the values of keys for a given
+  set of CPUs, the set of CPUs is initially all unset and the values of each key
+  are given.  Upon return, the CPUs which all match each of the given key-value
+  pairs are set in ``cpus``.  How matching is done depends on the key type.  For
+  value-like keys, matching means to be the exact same as the value.  For
+  boolean-like keys, matching means the result of a logical AND of the pair's
+  value with the CPU's value is exactly the same as the pair's value.  ``cpus``
+  may also initially have set bits, in which case the bits of any CPUs which do
+  not match the pairs will be cleared, but no other bits will be set.
+
+All other flags are reserved for future compatibility and must be zero.
 
 On success 0 is returned, on failure a negative error code is returned.
 
diff --git a/arch/riscv/include/asm/hwprobe.h b/arch/riscv/include/asm/hwprobe.h
index 7cad513538d8..a68764149e51 100644
--- a/arch/riscv/include/asm/hwprobe.h
+++ b/arch/riscv/include/asm/hwprobe.h
@@ -15,4 +15,28 @@ static inline bool riscv_hwprobe_key_is_valid(__s64 key)
 	return key >= 0 && key <= RISCV_HWPROBE_MAX_KEY;
 }
 
+static inline bool hwprobe_key_is_bitmask(__s64 key)
+{
+	switch (key) {
+	case RISCV_HWPROBE_KEY_BASE_BEHAVIOR:
+	case RISCV_HWPROBE_KEY_IMA_EXT_0:
+	case RISCV_HWPROBE_KEY_CPUPERF_0:
+		return true;
+	}
+
+	return false;
+}
+
+static inline bool riscv_hwprobe_pair_cmp(struct riscv_hwprobe *pair,
+					  struct riscv_hwprobe *other_pair)
+{
+	if (pair->key != other_pair->key)
+		return false;
+
+	if (hwprobe_key_is_bitmask(pair->key))
+		return (pair->value & other_pair->value) == other_pair->value;
+
+	return pair->value == other_pair->value;
+}
+
 #endif
diff --git a/arch/riscv/include/uapi/asm/hwprobe.h b/arch/riscv/include/uapi/asm/hwprobe.h
index 006bfb48343d..1d4134befc48 100644
--- a/arch/riscv/include/uapi/asm/hwprobe.h
+++ b/arch/riscv/include/uapi/asm/hwprobe.h
@@ -38,4 +38,7 @@ struct riscv_hwprobe {
 #define		RISCV_HWPROBE_MISALIGNED_MASK		(7 << 0)
 /* Increase RISCV_HWPROBE_MAX_KEY when adding items. */
 
+/* Flags */
+#define RISCV_HWPROBE_WHICH_CPUS	(1 << 0)
+
 #endif
diff --git a/arch/riscv/kernel/sys_hwprobe.c b/arch/riscv/kernel/sys_hwprobe.c
index 69ad5f793374..de294538ca25 100644
--- a/arch/riscv/kernel/sys_hwprobe.c
+++ b/arch/riscv/kernel/sys_hwprobe.c
@@ -166,10 +166,10 @@ static void hwprobe_one_pair(struct riscv_hwprobe *pair,
 	}
 }
 
-static int do_riscv_hwprobe(struct riscv_hwprobe __user *pairs,
-			    size_t pair_count, size_t cpusetsize,
-			    unsigned long __user *cpus_user,
-			    unsigned int flags)
+static int hwprobe_get_values(struct riscv_hwprobe __user *pairs,
+			      size_t pair_count, size_t cpusetsize,
+			      unsigned long __user *cpus_user,
+			      unsigned int flags)
 {
 	size_t out;
 	int ret;
@@ -223,6 +223,91 @@ static int do_riscv_hwprobe(struct riscv_hwprobe __user *pairs,
 	return 0;
 }
 
+static int hwprobe_get_cpus(struct riscv_hwprobe __user *pairs,
+			    size_t pair_count, size_t cpusetsize,
+			    unsigned long __user *cpus_user,
+			    unsigned int flags)
+{
+	cpumask_t cpus, one_cpu;
+	bool clear_all = false;
+	size_t i;
+	int ret;
+
+	if (flags != RISCV_HWPROBE_WHICH_CPUS)
+		return -EINVAL;
+
+	if (!cpusetsize || !cpus_user)
+		return -EINVAL;
+
+	if (cpusetsize > cpumask_size())
+		cpusetsize = cpumask_size();
+
+	ret = copy_from_user(&cpus, cpus_user, cpusetsize);
+	if (ret)
+		return -EFAULT;
+
+	cpumask_and(&cpus, &cpus, cpu_online_mask);
+	if (cpumask_empty(&cpus))
+		cpumask_copy(&cpus, cpu_online_mask);
+
+	cpumask_clear(&one_cpu);
+
+	for (i = 0; i < pair_count; i++) {
+		struct riscv_hwprobe pair, tmp;
+		int cpu;
+
+		ret = copy_from_user(&pair, &pairs[i], sizeof(pair));
+		if (ret)
+			return -EFAULT;
+
+		if (!riscv_hwprobe_key_is_valid(pair.key)) {
+			clear_all = true;
+			pair = (struct riscv_hwprobe){ .key = -1, };
+			ret = copy_to_user(&pairs[i], &pair, sizeof(pair));
+			if (ret)
+				return -EFAULT;
+		}
+
+		if (clear_all)
+			continue;
+
+		tmp = (struct riscv_hwprobe){ .key = pair.key, };
+
+		for_each_cpu(cpu, &cpus) {
+			cpumask_set_cpu(cpu, &one_cpu);
+
+			hwprobe_one_pair(&tmp, &one_cpu);
+
+			if (!riscv_hwprobe_pair_cmp(&tmp, &pair))
+				cpumask_clear_cpu(cpu, &cpus);
+
+			cpumask_clear_cpu(cpu, &one_cpu);
+		}
+	}
+
+	if (clear_all)
+		cpumask_clear(&cpus);
+
+	ret = copy_to_user(cpus_user, &cpus, cpusetsize);
+	if (ret)
+		return -EFAULT;
+
+	return 0;
+}
+
+static int do_riscv_hwprobe(struct riscv_hwprobe __user *pairs,
+			    size_t pair_count, size_t cpusetsize,
+			    unsigned long __user *cpus_user,
+			    unsigned int flags)
+{
+	if (flags & RISCV_HWPROBE_WHICH_CPUS)
+		return hwprobe_get_cpus(pairs, pair_count, cpusetsize,
+					cpus_user, flags);
+
+	return hwprobe_get_values(pairs, pair_count, cpusetsize,
+				  cpus_user, flags);
+}
+
 #ifdef CONFIG_MMU
 
 static int __init init_hwprobe_vdso_data(void)
diff --git a/arch/riscv/kernel/vdso/hwprobe.c b/arch/riscv/kernel/vdso/hwprobe.c
index 026b7645c5ab..e6c324d64544 100644
--- a/arch/riscv/kernel/vdso/hwprobe.c
+++ b/arch/riscv/kernel/vdso/hwprobe.c
@@ -3,6 +3,7 @@
  * Copyright 2023 Rivos, Inc
  */
 
+#include <linux/string.h>
 #include <linux/types.h>
 #include <vdso/datapage.h>
 #include <vdso/helpers.h>
@@ -11,14 +12,9 @@ extern int riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
 			 size_t cpusetsize, unsigned long *cpus,
 			 unsigned int flags);
 
-/* Add a prototype to avoid -Wmissing-prototypes warning. */
-int __vdso_riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
-			 size_t cpusetsize, unsigned long *cpus,
-			 unsigned int flags);
-
-int __vdso_riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
-			 size_t cpusetsize, unsigned long *cpus,
-			 unsigned int flags)
+static int riscv_vdso_get_values(struct riscv_hwprobe *pairs, size_t pair_count,
+				 size_t cpusetsize, unsigned long *cpus,
+				 unsigned int flags)
 {
 	const struct vdso_data *vd = __arch_get_vdso_data();
 	const struct arch_vdso_data *avd = &vd->arch_data;
@@ -50,3 +46,59 @@ int __vdso_riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
 
 	return 0;
 }
+
+static int riscv_vdso_get_cpus(struct riscv_hwprobe *pairs, size_t pair_count,
+			       size_t cpusetsize, unsigned long *cpus,
+			       unsigned int flags)
+{
+	const struct vdso_data *vd = __arch_get_vdso_data();
+	const struct arch_vdso_data *avd = &vd->arch_data;
+	struct riscv_hwprobe *p = pairs;
+	struct riscv_hwprobe *end = pairs + pair_count;
+	bool clear_all = false;
+
+	if (!cpusetsize || !cpus)
+		return -EINVAL;
+
+	if (flags != RISCV_HWPROBE_WHICH_CPUS || !avd->homogeneous_cpus)
+		return riscv_hwprobe(pairs, pair_count, cpusetsize, cpus, flags);
+
+	while (p < end) {
+		if (riscv_hwprobe_key_is_valid(p->key)) {
+			struct riscv_hwprobe t = {
+				.key = p->key,
+				.value = avd->all_cpu_hwprobe_values[p->key],
+			};
+
+			if (!riscv_hwprobe_pair_cmp(&t, p))
+				clear_all = true;
+		} else {
+			clear_all = true;
+			p->key = -1;
+			p->value = 0;
+		}
+		p++;
+	}
+
+	if (clear_all)
+		memset(cpus, 0, cpusetsize);
+
+	return 0;
+}
+
+/* Add a prototype to avoid -Wmissing-prototypes warning. */
+int __vdso_riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
+			 size_t cpusetsize, unsigned long *cpus,
+			 unsigned int flags);
+
+int __vdso_riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
+			 size_t cpusetsize, unsigned long *cpus,
+			 unsigned int flags)
+{
+	if (flags & RISCV_HWPROBE_WHICH_CPUS)
+		return riscv_vdso_get_cpus(pairs, pair_count, cpusetsize,
+					   cpus, flags);
+
+	return riscv_vdso_get_values(pairs, pair_count, cpusetsize,
+				     cpus, flags);
+}
-- 
2.41.0


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

[PATCH v1 4/6] RISC-V: selftests: Statically link hwprobe test

[Andrew Jones]

Statically linking makes it more convenient to copy the test to a
minimal busybox environment.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
---
 tools/testing/selftests/riscv/hwprobe/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/riscv/hwprobe/Makefile b/tools/testing/selftests/riscv/hwprobe/Makefile
index ebdbb3c22e54..5f614c3ba598 100644
--- a/tools/testing/selftests/riscv/hwprobe/Makefile
+++ b/tools/testing/selftests/riscv/hwprobe/Makefile
@@ -7,4 +7,4 @@ TEST_GEN_PROGS := hwprobe
 include ../../lib.mk
 
 $(OUTPUT)/hwprobe: hwprobe.c sys_hwprobe.S
-	$(CC) -o$@ $(CFLAGS) $(LDFLAGS) $^
+	$(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^
-- 
2.41.0


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

[PATCH v1 5/6] RISC-V: selftests: Convert hwprobe test to kselftest API

[Andrew Jones]

Returning (exiting with) negative exit codes isn't user friendly,
because the user must output the exit code with the shell, convert it
from its unsigned 8-bit value back to the negative value, and then
look up where that comes from in the code (which may be multiple
places). Use the kselftests TAP interface, instead.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 .../testing/selftests/riscv/hwprobe/hwprobe.c | 54 +++++++------------
 1 file changed, 20 insertions(+), 34 deletions(-)

diff --git a/tools/testing/selftests/riscv/hwprobe/hwprobe.c b/tools/testing/selftests/riscv/hwprobe/hwprobe.c
index 09f290a67420..56cdca53f54a 100644
--- a/tools/testing/selftests/riscv/hwprobe/hwprobe.c
+++ b/tools/testing/selftests/riscv/hwprobe/hwprobe.c
@@ -2,6 +2,8 @@
 #include <stddef.h>
 #include <asm/hwprobe.h>
 
+#include "../../kselftest.h"
+
 /*
  * Rather than relying on having a new enough libc to define this, just do it
  * ourselves.  This way we don't need to be coupled to a new-enough libc to
@@ -16,6 +18,9 @@ int main(int argc, char **argv)
 	unsigned long cpus;
 	long out;
 
+	ksft_print_header();
+	ksft_set_plan(5);
+
 	/* Fake the CPU_SET ops. */
 	cpus = -1;
 
@@ -25,13 +30,16 @@ int main(int argc, char **argv)
 	 */
 	for (long i = 0; i < 8; i++)
 		pairs[i].key = i;
+
 	out = riscv_hwprobe(pairs, 8, 1, &cpus, 0);
 	if (out != 0)
-		return -1;
+		ksft_exit_fail_msg("hwprobe() failed with %ld\n", out);
+
 	for (long i = 0; i < 4; ++i) {
 		/* Fail if the kernel claims not to recognize a base key. */
 		if ((i < 4) && (pairs[i].key != i))
-			return -2;
+			ksft_exit_fail_msg("Failed to recognize base key: key != i, "
+					   "key=%ld, i=%ld\n", pairs[i].key, i);
 
 		if (pairs[i].key != RISCV_HWPROBE_KEY_BASE_BEHAVIOR)
 			continue;
@@ -39,52 +47,30 @@ int main(int argc, char **argv)
 		if (pairs[i].value & RISCV_HWPROBE_BASE_BEHAVIOR_IMA)
 			continue;
 
-		return -3;
+		ksft_exit_fail_msg("Unexpected pair: (%ld, %ld)\n", pairs[i].key, pairs[i].value);
 	}
 
-	/*
-	 * This should also work with a NULL CPU set, but should not work
-	 * with an improperly supplied CPU set.
-	 */
 	out = riscv_hwprobe(pairs, 8, 0, 0, 0);
-	if (out != 0)
-		return -4;
+	ksft_test_result(out == 0, "NULL CPU set\n");
 
 	out = riscv_hwprobe(pairs, 8, 0, &cpus, 0);
-	if (out == 0)
-		return -5;
+	ksft_test_result(out != 0, "Bad CPU set\n");
 
 	out = riscv_hwprobe(pairs, 8, 1, 0, 0);
-	if (out == 0)
-		return -6;
+	ksft_test_result(out != 0, "NULL CPU set with non-zero size\n");
 
-	/*
-	 * Check that keys work by providing one that we know exists, and
-	 * checking to make sure the resultig pair is what we asked for.
-	 */
 	pairs[0].key = RISCV_HWPROBE_KEY_BASE_BEHAVIOR;
 	out = riscv_hwprobe(pairs, 1, 1, &cpus, 0);
-	if (out != 0)
-		return -7;
-	if (pairs[0].key != RISCV_HWPROBE_KEY_BASE_BEHAVIOR)
-		return -8;
+	ksft_test_result(out == 0 && pairs[0].key == RISCV_HWPROBE_KEY_BASE_BEHAVIOR,
+			 "Existing key is maintained\n");
 
-	/*
-	 * Check that an unknown key gets overwritten with -1,
-	 * but doesn't block elements after it.
-	 */
 	pairs[0].key = 0x5555;
 	pairs[1].key = 1;
 	pairs[1].value = 0xAAAA;
 	out = riscv_hwprobe(pairs, 2, 0, 0, 0);
-	if (out != 0)
-		return -9;
-
-	if (pairs[0].key != -1)
-		return -10;
-
-	if ((pairs[1].key != 1) || (pairs[1].value == 0xAAAA))
-		return -11;
+	ksft_test_result(out == 0 && pairs[0].key == -1 &&
+			 pairs[1].key == 1 && pairs[1].value != 0xAAAA,
+			 "Unknown key overwritten with -1 and doesn't block other elements\n");
 
-	return 0;
+	ksft_finished();
 }
-- 
2.41.0


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

[PATCH v1 6/6] RISC-V: selftests: Add which-cpus hwprobe test

[Andrew Jones]

Test the RISCV_HWPROBE_WHICH_CPUS flag of hwprobe. The test also
has a command line interface in order to get the cpu list for
arbitrary hwprobe pairs.

Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>
Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 .../testing/selftests/riscv/hwprobe/Makefile  |   5 +-
 .../testing/selftests/riscv/hwprobe/hwprobe.c |  12 +-
 .../testing/selftests/riscv/hwprobe/hwprobe.h |  15 ++
 .../selftests/riscv/hwprobe/which-cpus.c      | 154 ++++++++++++++++++
 .../selftests/riscv/vector/vstate_prctl.c     |  10 +-
 5 files changed, 175 insertions(+), 21 deletions(-)
 create mode 100644 tools/testing/selftests/riscv/hwprobe/hwprobe.h
 create mode 100644 tools/testing/selftests/riscv/hwprobe/which-cpus.c

diff --git a/tools/testing/selftests/riscv/hwprobe/Makefile b/tools/testing/selftests/riscv/hwprobe/Makefile
index 5f614c3ba598..210a8d7325b0 100644
--- a/tools/testing/selftests/riscv/hwprobe/Makefile
+++ b/tools/testing/selftests/riscv/hwprobe/Makefile
@@ -2,9 +2,12 @@
 # Copyright (C) 2021 ARM Limited
 # Originally tools/testing/arm64/abi/Makefile
 
-TEST_GEN_PROGS := hwprobe
+TEST_GEN_PROGS := hwprobe which-cpus
 
 include ../../lib.mk
 
 $(OUTPUT)/hwprobe: hwprobe.c sys_hwprobe.S
 	$(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^
+
+$(OUTPUT)/which-cpus: which-cpus.c sys_hwprobe.S
+	$(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^
diff --git a/tools/testing/selftests/riscv/hwprobe/hwprobe.c b/tools/testing/selftests/riscv/hwprobe/hwprobe.c
index 56cdca53f54a..d53e0889b59e 100644
--- a/tools/testing/selftests/riscv/hwprobe/hwprobe.c
+++ b/tools/testing/selftests/riscv/hwprobe/hwprobe.c
@@ -1,17 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
-#include <stddef.h>
-#include <asm/hwprobe.h>
-
+#include "hwprobe.h"
 #include "../../kselftest.h"
 
-/*
- * Rather than relying on having a new enough libc to define this, just do it
- * ourselves.  This way we don't need to be coupled to a new-enough libc to
- * contain the call.
- */
-long riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
-		   size_t cpu_count, unsigned long *cpus, unsigned int flags);
-
 int main(int argc, char **argv)
 {
 	struct riscv_hwprobe pairs[8];
diff --git a/tools/testing/selftests/riscv/hwprobe/hwprobe.h b/tools/testing/selftests/riscv/hwprobe/hwprobe.h
new file mode 100644
index 000000000000..e3fccb390c4d
--- /dev/null
+++ b/tools/testing/selftests/riscv/hwprobe/hwprobe.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef SELFTEST_RISCV_HWPROBE_H
+#define SELFTEST_RISCV_HWPROBE_H
+#include <stddef.h>
+#include <asm/hwprobe.h>
+
+/*
+ * Rather than relying on having a new enough libc to define this, just do it
+ * ourselves.  This way we don't need to be coupled to a new-enough libc to
+ * contain the call.
+ */
+long riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
+		   size_t cpusetsize, unsigned long *cpus, unsigned int flags);
+
+#endif
diff --git a/tools/testing/selftests/riscv/hwprobe/which-cpus.c b/tools/testing/selftests/riscv/hwprobe/which-cpus.c
new file mode 100644
index 000000000000..82c121412dfc
--- /dev/null
+++ b/tools/testing/selftests/riscv/hwprobe/which-cpus.c
@@ -0,0 +1,154 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023 Ventana Micro Systems Inc.
+ *
+ * Test the RISCV_HWPROBE_WHICH_CPUS flag of hwprobe. Also provides a command
+ * line interface to get the cpu list for arbitrary hwprobe pairs.
+ */
+#define _GNU_SOURCE
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sched.h>
+#include <unistd.h>
+#include <assert.h>
+
+#include "hwprobe.h"
+#include "../../kselftest.h"
+
+static void help(void)
+{
+	printf("\n"
+	       "which-cpus: [-h] [<key=value> [<key=value> ...]]\n\n"
+	       "   Without parameters, tests the RISCV_HWPROBE_WHICH_CPUS flag of hwprobe.\n"
+	       "   With parameters, where each parameter is a hwprobe pair written as\n"
+	       "   <key=value>, outputs the cpulist for cpus which all match the given set\n"
+	       "   of pairs.  'key' and 'value' should be in numeric form, e.g. 4=0x3b\n");
+}
+
+static void print_cpulist(cpu_set_t *cpus)
+{
+	int start = 0, end = 0;
+
+	if (!CPU_COUNT(cpus)) {
+		printf("cpus: None\n");
+		return;
+	}
+
+	printf("cpus:");
+	for (int i = 0, c = 0; i < CPU_COUNT(cpus); i++, c++) {
+		if (start != end && !CPU_ISSET(c, cpus))
+			printf("-%d", end);
+
+		while (!CPU_ISSET(c, cpus))
+			++c;
+
+		if (i != 0 && c == end + 1) {
+			end = c;
+			continue;
+		}
+
+		printf("%c%d", i == 0 ? ' ' : ',', c);
+		start = end = c;
+	}
+	if (start != end)
+		printf("-%d", end);
+	printf("\n");
+}
+
+static void do_which_cpus(int argc, char **argv, cpu_set_t *cpus)
+{
+	struct riscv_hwprobe *pairs;
+	int nr_pairs = argc - 1;
+	char *start, *end;
+	int rc;
+
+	pairs = malloc(nr_pairs * sizeof(struct riscv_hwprobe));
+	assert(pairs);
+
+	for (int i = 0; i < nr_pairs; i++) {
+		start = argv[i + 1];
+		pairs[i].key = strtol(start, &end, 0);
+		assert(end != start && *end == '=');
+		start = end + 1;
+		pairs[i].value = strtoul(start, &end, 0);
+		assert(end != start && *end == '\0');
+	}
+
+	rc = riscv_hwprobe(pairs, nr_pairs, sizeof(cpu_set_t), (unsigned long *)cpus, RISCV_HWPROBE_WHICH_CPUS);
+	assert(rc == 0);
+	print_cpulist(cpus);
+	free(pairs);
+}
+
+int main(int argc, char **argv)
+{
+	struct riscv_hwprobe pairs[2];
+	cpu_set_t cpus_aff, cpus;
+	__u64 ext0_all;
+	long rc;
+
+	rc = sched_getaffinity(0, sizeof(cpu_set_t), &cpus_aff);
+	assert(rc == 0);
+
+	if (argc > 1) {
+		if (!strcmp(argv[1], "-h"))
+			help();
+		else
+			do_which_cpus(argc, argv, &cpus_aff);
+		return 0;
+	}
+
+	ksft_print_header();
+	ksft_set_plan(7);
+
+	pairs[0] = (struct riscv_hwprobe){ .key = RISCV_HWPROBE_KEY_BASE_BEHAVIOR, };
+	rc = riscv_hwprobe(pairs, 1, 0, NULL, 0);
+	assert(rc == 0 && pairs[0].key == RISCV_HWPROBE_KEY_BASE_BEHAVIOR &&
+	       pairs[0].value == RISCV_HWPROBE_BASE_BEHAVIOR_IMA);
+
+	pairs[0] = (struct riscv_hwprobe){ .key = RISCV_HWPROBE_KEY_IMA_EXT_0, };
+	rc = riscv_hwprobe(pairs, 1, 0, NULL, 0);
+	assert(rc == 0 && pairs[0].key == RISCV_HWPROBE_KEY_IMA_EXT_0);
+	ext0_all = pairs[0].value;
+
+	pairs[0] = (struct riscv_hwprobe){ .key = RISCV_HWPROBE_KEY_BASE_BEHAVIOR, .value = RISCV_HWPROBE_BASE_BEHAVIOR_IMA, };
+	CPU_ZERO(&cpus);
+	rc = riscv_hwprobe(pairs, 1, 0, (unsigned long *)&cpus, RISCV_HWPROBE_WHICH_CPUS);
+	ksft_test_result(rc == -EINVAL, "no cpusetsize\n");
+
+	pairs[0] = (struct riscv_hwprobe){ .key = RISCV_HWPROBE_KEY_BASE_BEHAVIOR, .value = RISCV_HWPROBE_BASE_BEHAVIOR_IMA, };
+	rc = riscv_hwprobe(pairs, 1, sizeof(cpu_set_t), NULL, RISCV_HWPROBE_WHICH_CPUS);
+	ksft_test_result(rc == -EINVAL, "NULL cpus\n");
+
+	pairs[0] = (struct riscv_hwprobe){ .key = 0xbadc0de, };
+	CPU_ZERO(&cpus);
+	rc = riscv_hwprobe(pairs, 1, sizeof(cpu_set_t), (unsigned long *)&cpus, RISCV_HWPROBE_WHICH_CPUS);
+	ksft_test_result(rc == 0 && CPU_COUNT(&cpus) == 0, "unknown key\n");
+
+	pairs[0] = (struct riscv_hwprobe){ .key = RISCV_HWPROBE_KEY_BASE_BEHAVIOR, .value = RISCV_HWPROBE_BASE_BEHAVIOR_IMA, };
+	pairs[1] = (struct riscv_hwprobe){ .key = RISCV_HWPROBE_KEY_BASE_BEHAVIOR, .value = RISCV_HWPROBE_BASE_BEHAVIOR_IMA, };
+	CPU_ZERO(&cpus);
+	rc = riscv_hwprobe(pairs, 2, sizeof(cpu_set_t), (unsigned long *)&cpus, RISCV_HWPROBE_WHICH_CPUS);
+	ksft_test_result(rc == 0, "duplicate keys\n");
+
+	pairs[0] = (struct riscv_hwprobe){ .key = RISCV_HWPROBE_KEY_BASE_BEHAVIOR, .value = RISCV_HWPROBE_BASE_BEHAVIOR_IMA, };
+	pairs[1] = (struct riscv_hwprobe){ .key = RISCV_HWPROBE_KEY_IMA_EXT_0, .value = ext0_all, };
+	CPU_ZERO(&cpus);
+	rc = riscv_hwprobe(pairs, 2, sizeof(cpu_set_t), (unsigned long *)&cpus, RISCV_HWPROBE_WHICH_CPUS);
+	ksft_test_result(rc == 0 && CPU_COUNT(&cpus) == sysconf(_SC_NPROCESSORS_ONLN), "set all cpus\n");
+
+	pairs[0] = (struct riscv_hwprobe){ .key = RISCV_HWPROBE_KEY_BASE_BEHAVIOR, .value = RISCV_HWPROBE_BASE_BEHAVIOR_IMA, };
+	pairs[1] = (struct riscv_hwprobe){ .key = RISCV_HWPROBE_KEY_IMA_EXT_0, .value = ext0_all, };
+	memcpy(&cpus, &cpus_aff, sizeof(cpu_set_t));
+	rc = riscv_hwprobe(pairs, 2, sizeof(cpu_set_t), (unsigned long *)&cpus, RISCV_HWPROBE_WHICH_CPUS);
+	ksft_test_result(rc == 0 && CPU_EQUAL(&cpus, &cpus_aff), "set all affinity cpus\n");
+
+	pairs[0] = (struct riscv_hwprobe){ .key = RISCV_HWPROBE_KEY_BASE_BEHAVIOR, .value = RISCV_HWPROBE_BASE_BEHAVIOR_IMA, };
+	pairs[1] = (struct riscv_hwprobe){ .key = RISCV_HWPROBE_KEY_IMA_EXT_0, .value = ~ext0_all, };
+	memcpy(&cpus, &cpus_aff, sizeof(cpu_set_t));
+	rc = riscv_hwprobe(pairs, 2, sizeof(cpu_set_t), (unsigned long *)&cpus, RISCV_HWPROBE_WHICH_CPUS);
+	ksft_test_result(rc == 0 && CPU_COUNT(&cpus) == 0, "clear all cpus\n");
+
+	ksft_finished();
+}
diff --git a/tools/testing/selftests/riscv/vector/vstate_prctl.c b/tools/testing/selftests/riscv/vector/vstate_prctl.c
index b348b475be57..8dcd399ef7fc 100644
--- a/tools/testing/selftests/riscv/vector/vstate_prctl.c
+++ b/tools/testing/selftests/riscv/vector/vstate_prctl.c
@@ -1,20 +1,12 @@
 // SPDX-License-Identifier: GPL-2.0-only
 #include <sys/prctl.h>
 #include <unistd.h>
-#include <asm/hwprobe.h>
 #include <errno.h>
 #include <sys/wait.h>
 
+#include "../hwprobe/hwprobe.h"
 #include "../../kselftest.h"
 
-/*
- * Rather than relying on having a new enough libc to define this, just do it
- * ourselves.  This way we don't need to be coupled to a new-enough libc to
- * contain the call.
- */
-long riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
-		   size_t cpu_count, unsigned long *cpus, unsigned int flags);
-
 #define NEXT_PROGRAM "./vstate_exec_nolibc"
 static int launch_test(int test_inherit)
 {
-- 
2.41.0


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

Re: [PATCH v1 1/6] RISC-V: hwprobe: Clarify cpus size parameter

[Conor Dooley]

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

On Wed, Oct 11, 2023 at 03:56:12PM +0200, Andrew Jones wrote:
> The "count" parameter associated with the 'cpus' parameter of the
> hwprobe syscall is the size in bytes of 'cpus'. Naming it 'cpu_count'
> may mislead users (it did me) to think it's the number of CPUs that
> are or can be represented by 'cpus' instead. This is particularly
> easy (IMO) to get wrong since 'cpus' is documented to be defined by
> CPU_SET(3) and CPU_SET(3) also documents a CPU_COUNT() (the number
> of CPUs in set) macro. CPU_SET(3) refers to the size of cpu sets
> with 'setsize'. Adopt 'cpusetsize' for the hwprobe parameter and
> specifically state it is in bytes in Documentation/riscv/hwprobe.rst
> to clarify.
> 
> Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>

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

Thanks,
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 v1 2/6] RISC-V: Move the hwprobe syscall to its own file

[Conor Dooley]

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

On Wed, Oct 11, 2023 at 03:56:13PM +0200, Andrew Jones wrote:

> +++ b/arch/riscv/kernel/sys_hwprobe.c
> @@ -0,0 +1,276 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * The hwprobe interface, for allowing userspace to probe to see which features
> + * are supported by the hardware.  See Documentation/riscv/hwprobe.rst for more
> + * details.
> + *
> + * Copyright (C) 2012 Regents of the University of California
> + * Copyright (C) 2014 Darius Rad <darius@bluespec.com>
> + * Copyright (C) 2017 SiFive

So uh, this is all new(ish) code, originally written last September,
that is being moved, right?

[-- 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 v1 2/6] RISC-V: Move the hwprobe syscall to its own file

[Andrew Jones]

On Thu, Oct 12, 2023 at 02:45:14PM +0100, Conor Dooley wrote:
> On Wed, Oct 11, 2023 at 03:56:13PM +0200, Andrew Jones wrote:
> 
> > +++ b/arch/riscv/kernel/sys_hwprobe.c
> > @@ -0,0 +1,276 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * The hwprobe interface, for allowing userspace to probe to see which features
> > + * are supported by the hardware.  See Documentation/riscv/hwprobe.rst for more
> > + * details.
> > + *
> > + * Copyright (C) 2012 Regents of the University of California
> > + * Copyright (C) 2014 Darius Rad <darius@bluespec.com>
> > + * Copyright (C) 2017 SiFive
> 
> So uh, this is all new(ish) code, originally written last September,
> that is being moved, right?

Yeah, I just pulled the Copyrights over by standard practice of code
movement, but I agree they don't make much sense for the code I moved.
I suck at copyright management and would be happy for suggestions here.
If Rivos would like to put one here for the work Evan did, then I'll be
happy to add it. Or, if people prefer, I could add a Ventana copyright.

For me, whatever protects the open source code best is best :-)

Thanks,
drew

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

Re: [PATCH v1 2/6] RISC-V: Move the hwprobe syscall to its own file

[Evan Green]

On Thu, Oct 12, 2023 at 9:11 AM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Thu, Oct 12, 2023 at 02:45:14PM +0100, Conor Dooley wrote:
> > On Wed, Oct 11, 2023 at 03:56:13PM +0200, Andrew Jones wrote:
> >
> > > +++ b/arch/riscv/kernel/sys_hwprobe.c
> > > @@ -0,0 +1,276 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * The hwprobe interface, for allowing userspace to probe to see which features
> > > + * are supported by the hardware.  See Documentation/riscv/hwprobe.rst for more
> > > + * details.
> > > + *
> > > + * Copyright (C) 2012 Regents of the University of California
> > > + * Copyright (C) 2014 Darius Rad <darius@bluespec.com>
> > > + * Copyright (C) 2017 SiFive
> >
> > So uh, this is all new(ish) code, originally written last September,
> > that is being moved, right?
>
> Yeah, I just pulled the Copyrights over by standard practice of code
> movement, but I agree they don't make much sense for the code I moved.
> I suck at copyright management and would be happy for suggestions here.
> If Rivos would like to put one here for the work Evan did, then I'll be
> happy to add it. Or, if people prefer, I could add a Ventana copyright.

I probably would have stuck a Rivos banner on it if I had written it
as a new file myself, as I think they "own" the work I do. But it all feels
a bit like vanity license plates given it's GPL, so I imagine whatever
you do will get no real complaints.


-Evan

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

Re: [PATCH v1 2/6] RISC-V: Move the hwprobe syscall to its own file

[Conor Dooley]

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

On Thu, Oct 12, 2023 at 09:42:39AM -0700, Evan Green wrote:
> On Thu, Oct 12, 2023 at 9:11 AM Andrew Jones <ajones@ventanamicro.com> wrote:
> >
> > On Thu, Oct 12, 2023 at 02:45:14PM +0100, Conor Dooley wrote:
> > > On Wed, Oct 11, 2023 at 03:56:13PM +0200, Andrew Jones wrote:
> > >
> > > > +++ b/arch/riscv/kernel/sys_hwprobe.c
> > > > @@ -0,0 +1,276 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > +/*
> > > > + * The hwprobe interface, for allowing userspace to probe to see which features
> > > > + * are supported by the hardware.  See Documentation/riscv/hwprobe.rst for more
> > > > + * details.
> > > > + *
> > > > + * Copyright (C) 2012 Regents of the University of California
> > > > + * Copyright (C) 2014 Darius Rad <darius@bluespec.com>
> > > > + * Copyright (C) 2017 SiFive
> > >
> > > So uh, this is all new(ish) code, originally written last September,
> > > that is being moved, right?
> >
> > Yeah, I just pulled the Copyrights over by standard practice of code
> > movement, but I agree they don't make much sense for the code I moved.
> > I suck at copyright management and would be happy for suggestions here.
> > If Rivos would like to put one here for the work Evan did, then I'll be
> > happy to add it. Or, if people prefer, I could add a Ventana copyright.
> 
> I probably would have stuck a Rivos banner on it if I had written it
> as a new file myself, as I think they "own" the work I do. But it all feels
> a bit like vanity license plates given it's GPL, so I imagine whatever
> you do will get no real complaints.

Aye, that's how I see it too. I've not bothered adding that stuff where
I could help it. The git history shows who made copyrightable changes to
the file anyway, but obv. I am no lawyer.
Just in this case, since as far as I could tell this code was written
from LPC onwards, it made little sense to copy over some 2012 era
copyright information.

Thanks,
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 v1 3/6] RISC-V: hwprobe: Introduce which-cpus flag

[Evan Green]

On Wed, Oct 11, 2023 at 6:56 AM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> Introduce the first flag for the hwprobe syscall. The flag basically
> reverses its behavior, i.e. instead of populating the values of keys
> for a given set of cpus, the set of cpus after the call is the result
> of finding a set which supports the values of the keys. In order to
> do this, we implement a pair compare function which takes the type of
> value (a single value vs. a bitmask of booleans) into consideration.
> We also implement vdso support for the new flag.
>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>  Documentation/riscv/hwprobe.rst       | 16 ++++-
>  arch/riscv/include/asm/hwprobe.h      | 24 +++++++
>  arch/riscv/include/uapi/asm/hwprobe.h |  3 +
>  arch/riscv/kernel/sys_hwprobe.c       | 93 +++++++++++++++++++++++++--
>  arch/riscv/kernel/vdso/hwprobe.c      | 68 +++++++++++++++++---
>  5 files changed, 190 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/riscv/hwprobe.rst b/Documentation/riscv/hwprobe.rst
> index c57437e40ffb..576aa03f56bb 100644
> --- a/Documentation/riscv/hwprobe.rst
> +++ b/Documentation/riscv/hwprobe.rst
> @@ -25,8 +25,20 @@ arch, impl), the returned value will only be valid if all CPUs in the given set
>  have the same value. Otherwise -1 will be returned. For boolean-like keys, the
>  value returned will be a logical AND of the values for the specified CPUs.
>  Usermode can supply NULL for ``cpus`` and 0 for ``cpusetsize`` as a shortcut for
> -all online CPUs. There are currently no flags, this value must be zero for
> -future compatibility.
> +all online CPUs. The currently supported flags are:
> +
> +* :c:macro:`RISCV_HWPROBE_WHICH_CPUS`: This flag basically reverses the behavior
> +  of sys_riscv_hwprobe().  Instead of populating the values of keys for a given
> +  set of CPUs, the set of CPUs is initially all unset and the values of each key

This isn't quite right anymore, I reckon. The cpuset passed in is used
as an initial set, and this function removes CPUs from the set that
have differing values from those given in the array of pairs. If an
empty cpuset is passed in, then the initial set used is all online
cpus.

> +  are given.  Upon return, the CPUs which all match each of the given key-value
> +  pairs are set in ``cpus``.  How matching is done depends on the key type.  For
> +  value-like keys, matching means to be the exact same as the value.  For
> +  boolean-like keys, matching means the result of a logical AND of the pair's
> +  value with the CPU's value is exactly the same as the pair's value.  ``cpus``
> +  may also initially have set bits, in which case the bits of any CPUs which do
> +  not match the pairs will be cleared, but no other bits will be set.
> +
> +All other flags are reserved for future compatibility and must be zero.
>
>  On success 0 is returned, on failure a negative error code is returned.
>
> diff --git a/arch/riscv/include/asm/hwprobe.h b/arch/riscv/include/asm/hwprobe.h
> index 7cad513538d8..a68764149e51 100644
> --- a/arch/riscv/include/asm/hwprobe.h
> +++ b/arch/riscv/include/asm/hwprobe.h
> @@ -15,4 +15,28 @@ static inline bool riscv_hwprobe_key_is_valid(__s64 key)
>         return key >= 0 && key <= RISCV_HWPROBE_MAX_KEY;
>  }
>
> +static inline bool hwprobe_key_is_bitmask(__s64 key)
> +{
> +       switch (key) {
> +       case RISCV_HWPROBE_KEY_BASE_BEHAVIOR:
> +       case RISCV_HWPROBE_KEY_IMA_EXT_0:
> +       case RISCV_HWPROBE_KEY_CPUPERF_0:
> +               return true;
> +       }
> +
> +       return false;
> +}
> +
> +static inline bool riscv_hwprobe_pair_cmp(struct riscv_hwprobe *pair,
> +                                         struct riscv_hwprobe *other_pair)
> +{
> +       if (pair->key != other_pair->key)
> +               return false;
> +
> +       if (hwprobe_key_is_bitmask(pair->key))
> +               return (pair->value & other_pair->value) == other_pair->value;
> +
> +       return pair->value == other_pair->value;
> +}
> +
>  #endif
> diff --git a/arch/riscv/include/uapi/asm/hwprobe.h b/arch/riscv/include/uapi/asm/hwprobe.h
> index 006bfb48343d..1d4134befc48 100644
> --- a/arch/riscv/include/uapi/asm/hwprobe.h
> +++ b/arch/riscv/include/uapi/asm/hwprobe.h
> @@ -38,4 +38,7 @@ struct riscv_hwprobe {
>  #define                RISCV_HWPROBE_MISALIGNED_MASK           (7 << 0)
>  /* Increase RISCV_HWPROBE_MAX_KEY when adding items. */
>
> +/* Flags */
> +#define RISCV_HWPROBE_WHICH_CPUS       (1 << 0)
> +
>  #endif
> diff --git a/arch/riscv/kernel/sys_hwprobe.c b/arch/riscv/kernel/sys_hwprobe.c
> index 69ad5f793374..de294538ca25 100644
> --- a/arch/riscv/kernel/sys_hwprobe.c
> +++ b/arch/riscv/kernel/sys_hwprobe.c
> @@ -166,10 +166,10 @@ static void hwprobe_one_pair(struct riscv_hwprobe *pair,
>         }
>  }
>
> -static int do_riscv_hwprobe(struct riscv_hwprobe __user *pairs,
> -                           size_t pair_count, size_t cpusetsize,
> -                           unsigned long __user *cpus_user,
> -                           unsigned int flags)
> +static int hwprobe_get_values(struct riscv_hwprobe __user *pairs,
> +                             size_t pair_count, size_t cpusetsize,
> +                             unsigned long __user *cpus_user,
> +                             unsigned int flags)
>  {
>         size_t out;
>         int ret;
> @@ -223,6 +223,91 @@ static int do_riscv_hwprobe(struct riscv_hwprobe __user *pairs,
>         return 0;
>  }
>
> +static int hwprobe_get_cpus(struct riscv_hwprobe __user *pairs,
> +                           size_t pair_count, size_t cpusetsize,
> +                           unsigned long __user *cpus_user,
> +                           unsigned int flags)
> +{
> +       cpumask_t cpus, one_cpu;
> +       bool clear_all = false;
> +       size_t i;
> +       int ret;
> +
> +       if (flags != RISCV_HWPROBE_WHICH_CPUS)
> +               return -EINVAL;

We'll have to be careful if we add another flag to deal with how it
behaves here too. I think the choice you made here is correct as it's
the most defensive. It's (usually) easy to change a failure to a
non-failure, but an ABI compatibility issue to change a
weird-but-nonfailing behavior to a different behavior. This might be
worth a comment, but I also tend to love comments more than most, so
up to you.

> +
> +       if (!cpusetsize || !cpus_user)
> +               return -EINVAL;
> +
> +       if (cpusetsize > cpumask_size())
> +               cpusetsize = cpumask_size();
> +
> +       ret = copy_from_user(&cpus, cpus_user, cpusetsize);
> +       if (ret)
> +               return -EFAULT;
> +
> +       cpumask_and(&cpus, &cpus, cpu_online_mask);
> +       if (cpumask_empty(&cpus))
> +               cpumask_copy(&cpus, cpu_online_mask);

I worry this is accident prone. If the caller asks for some set of
CPUs that don't happen to be online right now, they get an answer for
a completely different set of CPUs. I'm all for having a shorthand for
"all online CPUs", but I think it should be more explicit. If you
moved the cpumask_and() below the if(cpumask_empty()), then IMO it
would be a clean shorthand: pass in an empty set to get all online
CPUs.

> +
> +       cpumask_clear(&one_cpu);
> +
> +       for (i = 0; i < pair_count; i++) {
> +               struct riscv_hwprobe pair, tmp;
> +               int cpu;
> +
> +               ret = copy_from_user(&pair, &pairs[i], sizeof(pair));
> +               if (ret)
> +                       return -EFAULT;
> +
> +               if (!riscv_hwprobe_key_is_valid(pair.key)) {
> +                       clear_all = true;
> +                       pair = (struct riscv_hwprobe){ .key = -1, };
> +                       ret = copy_to_user(&pairs[i], &pair, sizeof(pair));
> +                       if (ret)
> +                               return -EFAULT;
> +               }
> +
> +               if (clear_all)
> +                       continue;
> +
> +               tmp = (struct riscv_hwprobe){ .key = pair.key, };
> +
> +               for_each_cpu(cpu, &cpus) {
> +                       cpumask_set_cpu(cpu, &one_cpu);
> +
> +                       hwprobe_one_pair(&tmp, &one_cpu);
> +
> +                       if (!riscv_hwprobe_pair_cmp(&tmp, &pair))
> +                               cpumask_clear_cpu(cpu, &cpus);
> +
> +                       cpumask_clear_cpu(cpu, &one_cpu);
> +               }
> +       }
> +
> +       if (clear_all)
> +               cpumask_clear(&cpus);
> +
> +       ret = copy_to_user(cpus_user, &cpus, cpusetsize);
> +       if (ret)
> +               return -EFAULT;
> +
> +       return 0;
> +}
> +
> +static int do_riscv_hwprobe(struct riscv_hwprobe __user *pairs,
> +                           size_t pair_count, size_t cpusetsize,
> +                           unsigned long __user *cpus_user,
> +                           unsigned int flags)
> +{
> +       if (flags & RISCV_HWPROBE_WHICH_CPUS)
> +               return hwprobe_get_cpus(pairs, pair_count, cpusetsize,
> +                                       cpus_user, flags);
> +
> +       return hwprobe_get_values(pairs, pair_count, cpusetsize,
> +                                 cpus_user, flags);
> +}
> +
>  #ifdef CONFIG_MMU
>
>  static int __init init_hwprobe_vdso_data(void)
> diff --git a/arch/riscv/kernel/vdso/hwprobe.c b/arch/riscv/kernel/vdso/hwprobe.c
> index 026b7645c5ab..e6c324d64544 100644
> --- a/arch/riscv/kernel/vdso/hwprobe.c
> +++ b/arch/riscv/kernel/vdso/hwprobe.c
> @@ -3,6 +3,7 @@
>   * Copyright 2023 Rivos, Inc
>   */
>
> +#include <linux/string.h>
>  #include <linux/types.h>
>  #include <vdso/datapage.h>
>  #include <vdso/helpers.h>
> @@ -11,14 +12,9 @@ extern int riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
>                          size_t cpusetsize, unsigned long *cpus,
>                          unsigned int flags);
>
> -/* Add a prototype to avoid -Wmissing-prototypes warning. */
> -int __vdso_riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
> -                        size_t cpusetsize, unsigned long *cpus,
> -                        unsigned int flags);
> -
> -int __vdso_riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
> -                        size_t cpusetsize, unsigned long *cpus,
> -                        unsigned int flags)
> +static int riscv_vdso_get_values(struct riscv_hwprobe *pairs, size_t pair_count,
> +                                size_t cpusetsize, unsigned long *cpus,
> +                                unsigned int flags)
>  {
>         const struct vdso_data *vd = __arch_get_vdso_data();
>         const struct arch_vdso_data *avd = &vd->arch_data;
> @@ -50,3 +46,59 @@ int __vdso_riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
>
>         return 0;
>  }
> +
> +static int riscv_vdso_get_cpus(struct riscv_hwprobe *pairs, size_t pair_count,
> +                              size_t cpusetsize, unsigned long *cpus,
> +                              unsigned int flags)
> +{
> +       const struct vdso_data *vd = __arch_get_vdso_data();
> +       const struct arch_vdso_data *avd = &vd->arch_data;
> +       struct riscv_hwprobe *p = pairs;
> +       struct riscv_hwprobe *end = pairs + pair_count;
> +       bool clear_all = false;
> +
> +       if (!cpusetsize || !cpus)
> +               return -EINVAL;
> +
> +       if (flags != RISCV_HWPROBE_WHICH_CPUS || !avd->homogeneous_cpus)
> +               return riscv_hwprobe(pairs, pair_count, cpusetsize, cpus, flags);
> +

Hm, I realize now that doing the shorthand of "empty set == all online
cpus" leaves the VDSO in a slight lurch, as we now have to figure out
how to come up with the cpu_online_mask in usermode. It'd be a bummer
if the shorthand always bounced to the system call, as I think this
will be the most common case. Is there a way we could stash the
cpu_online_mask in the vDSO data without it going stale? Any other
ideas?

> +       while (p < end) {
> +               if (riscv_hwprobe_key_is_valid(p->key)) {
> +                       struct riscv_hwprobe t = {
> +                               .key = p->key,
> +                               .value = avd->all_cpu_hwprobe_values[p->key],
> +                       };
> +
> +                       if (!riscv_hwprobe_pair_cmp(&t, p))
> +                               clear_all = true;
> +               } else {
> +                       clear_all = true;
> +                       p->key = -1;
> +                       p->value = 0;
> +               }
> +               p++;
> +       }
> +
> +       if (clear_all)
> +               memset(cpus, 0, cpusetsize);
> +
> +       return 0;
> +}
> +
> +/* Add a prototype to avoid -Wmissing-prototypes warning. */
> +int __vdso_riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
> +                        size_t cpusetsize, unsigned long *cpus,
> +                        unsigned int flags);
> +
> +int __vdso_riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
> +                        size_t cpusetsize, unsigned long *cpus,
> +                        unsigned int flags)
> +{
> +       if (flags & RISCV_HWPROBE_WHICH_CPUS)
> +               return riscv_vdso_get_cpus(pairs, pair_count, cpusetsize,
> +                                          cpus, flags);
> +
> +       return riscv_vdso_get_values(pairs, pair_count, cpusetsize,
> +                                    cpus, flags);
> +}
> --
> 2.41.0
>

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

Re: [PATCH v1 2/6] RISC-V: Move the hwprobe syscall to its own file

[Andrew Jones]

On Thu, Oct 12, 2023 at 06:02:38PM +0100, Conor Dooley wrote:
> On Thu, Oct 12, 2023 at 09:42:39AM -0700, Evan Green wrote:
> > On Thu, Oct 12, 2023 at 9:11 AM Andrew Jones <ajones@ventanamicro.com> wrote:
> > >
> > > On Thu, Oct 12, 2023 at 02:45:14PM +0100, Conor Dooley wrote:
> > > > On Wed, Oct 11, 2023 at 03:56:13PM +0200, Andrew Jones wrote:
> > > >
> > > > > +++ b/arch/riscv/kernel/sys_hwprobe.c
> > > > > @@ -0,0 +1,276 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > > +/*
> > > > > + * The hwprobe interface, for allowing userspace to probe to see which features
> > > > > + * are supported by the hardware.  See Documentation/riscv/hwprobe.rst for more
> > > > > + * details.
> > > > > + *
> > > > > + * Copyright (C) 2012 Regents of the University of California
> > > > > + * Copyright (C) 2014 Darius Rad <darius@bluespec.com>
> > > > > + * Copyright (C) 2017 SiFive
> > > >
> > > > So uh, this is all new(ish) code, originally written last September,
> > > > that is being moved, right?
> > >
> > > Yeah, I just pulled the Copyrights over by standard practice of code
> > > movement, but I agree they don't make much sense for the code I moved.
> > > I suck at copyright management and would be happy for suggestions here.
> > > If Rivos would like to put one here for the work Evan did, then I'll be
> > > happy to add it. Or, if people prefer, I could add a Ventana copyright.
> > 
> > I probably would have stuck a Rivos banner on it if I had written it
> > as a new file myself, as I think they "own" the work I do. But it all feels
> > a bit like vanity license plates given it's GPL, so I imagine whatever
> > you do will get no real complaints.
> 
> Aye, that's how I see it too. I've not bothered adding that stuff where
> I could help it. The git history shows who made copyrightable changes to
> the file anyway, but obv. I am no lawyer.
> Just in this case, since as far as I could tell this code was written
> from LPC onwards, it made little sense to copy over some 2012 era
> copyright information.
>

So what's the final plan? I feel a bit strange adding a Rivos copyright
while copying the Rivos code over since I don't consider myself
"authorized" to do so. I also don't want to add a Ventana one for just
code movement. Do we need a copyright on this file at all? Should I move
it without anything and then add a Ventana copyright when adding the
which-cpus stuff?

Thanks,
drew

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

Re: [PATCH v1 3/6] RISC-V: hwprobe: Introduce which-cpus flag

[Andrew Jones]

On Thu, Oct 12, 2023 at 10:40:21AM -0700, Evan Green wrote:
> On Wed, Oct 11, 2023 at 6:56 AM Andrew Jones <ajones@ventanamicro.com> wrote:
> >
> > Introduce the first flag for the hwprobe syscall. The flag basically
> > reverses its behavior, i.e. instead of populating the values of keys
> > for a given set of cpus, the set of cpus after the call is the result
> > of finding a set which supports the values of the keys. In order to
> > do this, we implement a pair compare function which takes the type of
> > value (a single value vs. a bitmask of booleans) into consideration.
> > We also implement vdso support for the new flag.
> >
> > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > ---
> >  Documentation/riscv/hwprobe.rst       | 16 ++++-
> >  arch/riscv/include/asm/hwprobe.h      | 24 +++++++
> >  arch/riscv/include/uapi/asm/hwprobe.h |  3 +
> >  arch/riscv/kernel/sys_hwprobe.c       | 93 +++++++++++++++++++++++++--
> >  arch/riscv/kernel/vdso/hwprobe.c      | 68 +++++++++++++++++---
> >  5 files changed, 190 insertions(+), 14 deletions(-)
> >
> > diff --git a/Documentation/riscv/hwprobe.rst b/Documentation/riscv/hwprobe.rst
> > index c57437e40ffb..576aa03f56bb 100644
> > --- a/Documentation/riscv/hwprobe.rst
> > +++ b/Documentation/riscv/hwprobe.rst
> > @@ -25,8 +25,20 @@ arch, impl), the returned value will only be valid if all CPUs in the given set
> >  have the same value. Otherwise -1 will be returned. For boolean-like keys, the
> >  value returned will be a logical AND of the values for the specified CPUs.
> >  Usermode can supply NULL for ``cpus`` and 0 for ``cpusetsize`` as a shortcut for
> > -all online CPUs. There are currently no flags, this value must be zero for
> > -future compatibility.
> > +all online CPUs. The currently supported flags are:
> > +
> > +* :c:macro:`RISCV_HWPROBE_WHICH_CPUS`: This flag basically reverses the behavior
> > +  of sys_riscv_hwprobe().  Instead of populating the values of keys for a given
> > +  set of CPUs, the set of CPUs is initially all unset and the values of each key
> 
> This isn't quite right anymore, I reckon. The cpuset passed in is used
> as an initial set, and this function removes CPUs from the set that
> have differing values from those given in the array of pairs. If an
> empty cpuset is passed in, then the initial set used is all online
> cpus.

Indeed. I'll add more text describing the "which cpus of this cpuset"
behavior for v2.

> 
> > +  are given.  Upon return, the CPUs which all match each of the given key-value
> > +  pairs are set in ``cpus``.  How matching is done depends on the key type.  For
> > +  value-like keys, matching means to be the exact same as the value.  For
> > +  boolean-like keys, matching means the result of a logical AND of the pair's
> > +  value with the CPU's value is exactly the same as the pair's value.  ``cpus``
> > +  may also initially have set bits, in which case the bits of any CPUs which do
> > +  not match the pairs will be cleared, but no other bits will be set.
> > +
> > +All other flags are reserved for future compatibility and must be zero.
> >
> >  On success 0 is returned, on failure a negative error code is returned.
> >
> > diff --git a/arch/riscv/include/asm/hwprobe.h b/arch/riscv/include/asm/hwprobe.h
> > index 7cad513538d8..a68764149e51 100644
> > --- a/arch/riscv/include/asm/hwprobe.h
> > +++ b/arch/riscv/include/asm/hwprobe.h
> > @@ -15,4 +15,28 @@ static inline bool riscv_hwprobe_key_is_valid(__s64 key)
> >         return key >= 0 && key <= RISCV_HWPROBE_MAX_KEY;
> >  }
> >
> > +static inline bool hwprobe_key_is_bitmask(__s64 key)
> > +{
> > +       switch (key) {
> > +       case RISCV_HWPROBE_KEY_BASE_BEHAVIOR:
> > +       case RISCV_HWPROBE_KEY_IMA_EXT_0:
> > +       case RISCV_HWPROBE_KEY_CPUPERF_0:
> > +               return true;
> > +       }
> > +
> > +       return false;
> > +}
> > +
> > +static inline bool riscv_hwprobe_pair_cmp(struct riscv_hwprobe *pair,
> > +                                         struct riscv_hwprobe *other_pair)
> > +{
> > +       if (pair->key != other_pair->key)
> > +               return false;
> > +
> > +       if (hwprobe_key_is_bitmask(pair->key))
> > +               return (pair->value & other_pair->value) == other_pair->value;
> > +
> > +       return pair->value == other_pair->value;
> > +}
> > +
> >  #endif
> > diff --git a/arch/riscv/include/uapi/asm/hwprobe.h b/arch/riscv/include/uapi/asm/hwprobe.h
> > index 006bfb48343d..1d4134befc48 100644
> > --- a/arch/riscv/include/uapi/asm/hwprobe.h
> > +++ b/arch/riscv/include/uapi/asm/hwprobe.h
> > @@ -38,4 +38,7 @@ struct riscv_hwprobe {
> >  #define                RISCV_HWPROBE_MISALIGNED_MASK           (7 << 0)
> >  /* Increase RISCV_HWPROBE_MAX_KEY when adding items. */
> >
> > +/* Flags */
> > +#define RISCV_HWPROBE_WHICH_CPUS       (1 << 0)
> > +
> >  #endif
> > diff --git a/arch/riscv/kernel/sys_hwprobe.c b/arch/riscv/kernel/sys_hwprobe.c
> > index 69ad5f793374..de294538ca25 100644
> > --- a/arch/riscv/kernel/sys_hwprobe.c
> > +++ b/arch/riscv/kernel/sys_hwprobe.c
> > @@ -166,10 +166,10 @@ static void hwprobe_one_pair(struct riscv_hwprobe *pair,
> >         }
> >  }
> >
> > -static int do_riscv_hwprobe(struct riscv_hwprobe __user *pairs,
> > -                           size_t pair_count, size_t cpusetsize,
> > -                           unsigned long __user *cpus_user,
> > -                           unsigned int flags)
> > +static int hwprobe_get_values(struct riscv_hwprobe __user *pairs,
> > +                             size_t pair_count, size_t cpusetsize,
> > +                             unsigned long __user *cpus_user,
> > +                             unsigned int flags)
> >  {
> >         size_t out;
> >         int ret;
> > @@ -223,6 +223,91 @@ static int do_riscv_hwprobe(struct riscv_hwprobe __user *pairs,
> >         return 0;
> >  }
> >
> > +static int hwprobe_get_cpus(struct riscv_hwprobe __user *pairs,
> > +                           size_t pair_count, size_t cpusetsize,
> > +                           unsigned long __user *cpus_user,
> > +                           unsigned int flags)
> > +{
> > +       cpumask_t cpus, one_cpu;
> > +       bool clear_all = false;
> > +       size_t i;
> > +       int ret;
> > +
> > +       if (flags != RISCV_HWPROBE_WHICH_CPUS)
> > +               return -EINVAL;
> 
> We'll have to be careful if we add another flag to deal with how it
> behaves here too. I think the choice you made here is correct as it's
> the most defensive. It's (usually) easy to change a failure to a
> non-failure, but an ABI compatibility issue to change a
> weird-but-nonfailing behavior to a different behavior. This might be
> worth a comment, but I also tend to love comments more than most, so
> up to you.

I'll probably leave it without a comment. My thinking is that if we
add another flag which applies to which-cpus, then the selftests,
which should get updated with new tests for the new flag, should
quickly point this out. I prefer avoiding comments when possible,
since neither the compiler nor test cases test them, allowing them
to easily go stale.

> 
> > +
> > +       if (!cpusetsize || !cpus_user)
> > +               return -EINVAL;
> > +
> > +       if (cpusetsize > cpumask_size())
> > +               cpusetsize = cpumask_size();
> > +
> > +       ret = copy_from_user(&cpus, cpus_user, cpusetsize);
> > +       if (ret)
> > +               return -EFAULT;
> > +
> > +       cpumask_and(&cpus, &cpus, cpu_online_mask);
> > +       if (cpumask_empty(&cpus))
> > +               cpumask_copy(&cpus, cpu_online_mask);
> 
> I worry this is accident prone. If the caller asks for some set of
> CPUs that don't happen to be online right now, they get an answer for
> a completely different set of CPUs. I'm all for having a shorthand for
> "all online CPUs", but I think it should be more explicit. If you
> moved the cpumask_and() below the if(cpumask_empty()), then IMO it
> would be a clean shorthand: pass in an empty set to get all online
> CPUs.

Good catch. The way it is now is indeed a bug and your suggestion is
the fix. Will do for v2.

> 
> > +
> > +       cpumask_clear(&one_cpu);
> > +
> > +       for (i = 0; i < pair_count; i++) {
> > +               struct riscv_hwprobe pair, tmp;
> > +               int cpu;
> > +
> > +               ret = copy_from_user(&pair, &pairs[i], sizeof(pair));
> > +               if (ret)
> > +                       return -EFAULT;
> > +
> > +               if (!riscv_hwprobe_key_is_valid(pair.key)) {
> > +                       clear_all = true;
> > +                       pair = (struct riscv_hwprobe){ .key = -1, };
> > +                       ret = copy_to_user(&pairs[i], &pair, sizeof(pair));
> > +                       if (ret)
> > +                               return -EFAULT;
> > +               }
> > +
> > +               if (clear_all)
> > +                       continue;
> > +
> > +               tmp = (struct riscv_hwprobe){ .key = pair.key, };
> > +
> > +               for_each_cpu(cpu, &cpus) {
> > +                       cpumask_set_cpu(cpu, &one_cpu);
> > +
> > +                       hwprobe_one_pair(&tmp, &one_cpu);
> > +
> > +                       if (!riscv_hwprobe_pair_cmp(&tmp, &pair))
> > +                               cpumask_clear_cpu(cpu, &cpus);
> > +
> > +                       cpumask_clear_cpu(cpu, &one_cpu);
> > +               }
> > +       }
> > +
> > +       if (clear_all)
> > +               cpumask_clear(&cpus);
> > +
> > +       ret = copy_to_user(cpus_user, &cpus, cpusetsize);
> > +       if (ret)
> > +               return -EFAULT;
> > +
> > +       return 0;
> > +}
> > +
> > +static int do_riscv_hwprobe(struct riscv_hwprobe __user *pairs,
> > +                           size_t pair_count, size_t cpusetsize,
> > +                           unsigned long __user *cpus_user,
> > +                           unsigned int flags)
> > +{
> > +       if (flags & RISCV_HWPROBE_WHICH_CPUS)
> > +               return hwprobe_get_cpus(pairs, pair_count, cpusetsize,
> > +                                       cpus_user, flags);
> > +
> > +       return hwprobe_get_values(pairs, pair_count, cpusetsize,
> > +                                 cpus_user, flags);
> > +}
> > +
> >  #ifdef CONFIG_MMU
> >
> >  static int __init init_hwprobe_vdso_data(void)
> > diff --git a/arch/riscv/kernel/vdso/hwprobe.c b/arch/riscv/kernel/vdso/hwprobe.c
> > index 026b7645c5ab..e6c324d64544 100644
> > --- a/arch/riscv/kernel/vdso/hwprobe.c
> > +++ b/arch/riscv/kernel/vdso/hwprobe.c
> > @@ -3,6 +3,7 @@
> >   * Copyright 2023 Rivos, Inc
> >   */
> >
> > +#include <linux/string.h>
> >  #include <linux/types.h>
> >  #include <vdso/datapage.h>
> >  #include <vdso/helpers.h>
> > @@ -11,14 +12,9 @@ extern int riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
> >                          size_t cpusetsize, unsigned long *cpus,
> >                          unsigned int flags);
> >
> > -/* Add a prototype to avoid -Wmissing-prototypes warning. */
> > -int __vdso_riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
> > -                        size_t cpusetsize, unsigned long *cpus,
> > -                        unsigned int flags);
> > -
> > -int __vdso_riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
> > -                        size_t cpusetsize, unsigned long *cpus,
> > -                        unsigned int flags)
> > +static int riscv_vdso_get_values(struct riscv_hwprobe *pairs, size_t pair_count,
> > +                                size_t cpusetsize, unsigned long *cpus,
> > +                                unsigned int flags)
> >  {
> >         const struct vdso_data *vd = __arch_get_vdso_data();
> >         const struct arch_vdso_data *avd = &vd->arch_data;
> > @@ -50,3 +46,59 @@ int __vdso_riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
> >
> >         return 0;
> >  }
> > +
> > +static int riscv_vdso_get_cpus(struct riscv_hwprobe *pairs, size_t pair_count,
> > +                              size_t cpusetsize, unsigned long *cpus,
> > +                              unsigned int flags)
> > +{
> > +       const struct vdso_data *vd = __arch_get_vdso_data();
> > +       const struct arch_vdso_data *avd = &vd->arch_data;
> > +       struct riscv_hwprobe *p = pairs;
> > +       struct riscv_hwprobe *end = pairs + pair_count;
> > +       bool clear_all = false;
> > +
> > +       if (!cpusetsize || !cpus)
> > +               return -EINVAL;
> > +
> > +       if (flags != RISCV_HWPROBE_WHICH_CPUS || !avd->homogeneous_cpus)
> > +               return riscv_hwprobe(pairs, pair_count, cpusetsize, cpus, flags);
> > +
> 
> Hm, I realize now that doing the shorthand of "empty set == all online
> cpus" leaves the VDSO in a slight lurch, as we now have to figure out
> how to come up with the cpu_online_mask in usermode. It'd be a bummer
> if the shorthand always bounced to the system call,

Huh, I recall thinking about this while working on it, but now, looking at
the code I wrote, I see that I apparently forgot to address it. I had
intended to bounce to the system call to handle the empty set case, but
I'm missing the check for that!

> as I think this
> will be the most common case. Is there a way we could stash the
> cpu_online_mask in the vDSO data without it going stale? Any other
> ideas?

I don't think we can stash the data and keep it synchronized and, while
the empty set case may be useful for platform description utilities, I
actually suspect applications which want to set the affinity of their
threads based on cpu features will want to start by setting their set
to the result of sched_getaffinity(). This is because it wouldn't be
useful for an application to know that cpu A has the extension they need
if their process has been confined to a set excluding cpu A with cgroups.

I'll fix this lack of empty set check for v2.

Thanks,
drew

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

Re: [PATCH v1 2/6] RISC-V: Move the hwprobe syscall to its own file

[Conor Dooley]

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

On Fri, Oct 13, 2023 at 08:45:39AM +0200, Andrew Jones wrote:
> On Thu, Oct 12, 2023 at 06:02:38PM +0100, Conor Dooley wrote:
> > On Thu, Oct 12, 2023 at 09:42:39AM -0700, Evan Green wrote:
> > > On Thu, Oct 12, 2023 at 9:11 AM Andrew Jones <ajones@ventanamicro.com> wrote:
> > > >
> > > > On Thu, Oct 12, 2023 at 02:45:14PM +0100, Conor Dooley wrote:
> > > > > On Wed, Oct 11, 2023 at 03:56:13PM +0200, Andrew Jones wrote:
> > > > >
> > > > > > +++ b/arch/riscv/kernel/sys_hwprobe.c
> > > > > > @@ -0,0 +1,276 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > > > +/*
> > > > > > + * The hwprobe interface, for allowing userspace to probe to see which features
> > > > > > + * are supported by the hardware.  See Documentation/riscv/hwprobe.rst for more
> > > > > > + * details.
> > > > > > + *
> > > > > > + * Copyright (C) 2012 Regents of the University of California
> > > > > > + * Copyright (C) 2014 Darius Rad <darius@bluespec.com>
> > > > > > + * Copyright (C) 2017 SiFive
> > > > >
> > > > > So uh, this is all new(ish) code, originally written last September,
> > > > > that is being moved, right?
> > > >
> > > > Yeah, I just pulled the Copyrights over by standard practice of code
> > > > movement, but I agree they don't make much sense for the code I moved.
> > > > I suck at copyright management and would be happy for suggestions here.
> > > > If Rivos would like to put one here for the work Evan did, then I'll be
> > > > happy to add it. Or, if people prefer, I could add a Ventana copyright.
> > > 
> > > I probably would have stuck a Rivos banner on it if I had written it
> > > as a new file myself, as I think they "own" the work I do. But it all feels
> > > a bit like vanity license plates given it's GPL, so I imagine whatever
> > > you do will get no real complaints.
> > 
> > Aye, that's how I see it too. I've not bothered adding that stuff where
> > I could help it. The git history shows who made copyrightable changes to
> > the file anyway, but obv. I am no lawyer.
> > Just in this case, since as far as I could tell this code was written
> > from LPC onwards, it made little sense to copy over some 2012 era
> > copyright information.
> >
> 
> So what's the final plan? I feel a bit strange adding a Rivos copyright
> while copying the Rivos code over since I don't consider myself
> "authorized" to do so.

They didn't add one when they made the changes, so I wouldn't.

> I also don't want to add a Ventana one for just
> code movement. Do we need a copyright on this file at all? Should I move
> it without anything and then add a Ventana copyright when adding the
> which-cpus stuff?

I would move it without anything.
Apologies for derailing things with copyright notice crap :/

[-- 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 v1 2/6] RISC-V: Move the hwprobe syscall to its own file

[Evan Green]

On Fri, Oct 13, 2023 at 12:39 AM Conor Dooley <conor@kernel.org> wrote:
>
> On Fri, Oct 13, 2023 at 08:45:39AM +0200, Andrew Jones wrote:
> > On Thu, Oct 12, 2023 at 06:02:38PM +0100, Conor Dooley wrote:
> > > On Thu, Oct 12, 2023 at 09:42:39AM -0700, Evan Green wrote:
> > > > On Thu, Oct 12, 2023 at 9:11 AM Andrew Jones <ajones@ventanamicro.com> wrote:
> > > > >
> > > > > On Thu, Oct 12, 2023 at 02:45:14PM +0100, Conor Dooley wrote:
> > > > > > On Wed, Oct 11, 2023 at 03:56:13PM +0200, Andrew Jones wrote:
> > > > > >
> > > > > > > +++ b/arch/riscv/kernel/sys_hwprobe.c
> > > > > > > @@ -0,0 +1,276 @@
> > > > > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > > > > +/*
> > > > > > > + * The hwprobe interface, for allowing userspace to probe to see which features
> > > > > > > + * are supported by the hardware.  See Documentation/riscv/hwprobe.rst for more
> > > > > > > + * details.
> > > > > > > + *
> > > > > > > + * Copyright (C) 2012 Regents of the University of California
> > > > > > > + * Copyright (C) 2014 Darius Rad <darius@bluespec.com>
> > > > > > > + * Copyright (C) 2017 SiFive
> > > > > >
> > > > > > So uh, this is all new(ish) code, originally written last September,
> > > > > > that is being moved, right?
> > > > >
> > > > > Yeah, I just pulled the Copyrights over by standard practice of code
> > > > > movement, but I agree they don't make much sense for the code I moved.
> > > > > I suck at copyright management and would be happy for suggestions here.
> > > > > If Rivos would like to put one here for the work Evan did, then I'll be
> > > > > happy to add it. Or, if people prefer, I could add a Ventana copyright.
> > > >
> > > > I probably would have stuck a Rivos banner on it if I had written it
> > > > as a new file myself, as I think they "own" the work I do. But it all feels
> > > > a bit like vanity license plates given it's GPL, so I imagine whatever
> > > > you do will get no real complaints.
> > >
> > > Aye, that's how I see it too. I've not bothered adding that stuff where
> > > I could help it. The git history shows who made copyrightable changes to
> > > the file anyway, but obv. I am no lawyer.
> > > Just in this case, since as far as I could tell this code was written
> > > from LPC onwards, it made little sense to copy over some 2012 era
> > > copyright information.
> > >
> >
> > So what's the final plan? I feel a bit strange adding a Rivos copyright
> > while copying the Rivos code over since I don't consider myself
> > "authorized" to do so.
>
> They didn't add one when they made the changes, so I wouldn't.
>
> > I also don't want to add a Ventana one for just
> > code movement. Do we need a copyright on this file at all? Should I move
> > it without anything and then add a Ventana copyright when adding the
> > which-cpus stuff?
>
> I would move it without anything.
> Apologies for derailing things with copyright notice crap :/

Works for me, too.
-Evan

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

Re: [PATCH v1 3/6] RISC-V: hwprobe: Introduce which-cpus flag

[Andrew Jones]

On Wed, Oct 11, 2023 at 03:56:14PM +0200, Andrew Jones wrote:
...
> diff --git a/arch/riscv/kernel/vdso/hwprobe.c b/arch/riscv/kernel/vdso/hwprobe.c
> index 026b7645c5ab..e6c324d64544 100644
> --- a/arch/riscv/kernel/vdso/hwprobe.c
> +++ b/arch/riscv/kernel/vdso/hwprobe.c
> @@ -3,6 +3,7 @@
>   * Copyright 2023 Rivos, Inc
>   */
>  
> +#include <linux/string.h>
>  #include <linux/types.h>
>  #include <vdso/datapage.h>
>  #include <vdso/helpers.h>
> @@ -11,14 +12,9 @@ extern int riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
>  			 size_t cpusetsize, unsigned long *cpus,
>  			 unsigned int flags);
>  
> -/* Add a prototype to avoid -Wmissing-prototypes warning. */
> -int __vdso_riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
> -			 size_t cpusetsize, unsigned long *cpus,
> -			 unsigned int flags);
> -
> -int __vdso_riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
> -			 size_t cpusetsize, unsigned long *cpus,
> -			 unsigned int flags)
> +static int riscv_vdso_get_values(struct riscv_hwprobe *pairs, size_t pair_count,
> +				 size_t cpusetsize, unsigned long *cpus,
> +				 unsigned int flags)
>  {
>  	const struct vdso_data *vd = __arch_get_vdso_data();
>  	const struct arch_vdso_data *avd = &vd->arch_data;
> @@ -50,3 +46,59 @@ int __vdso_riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
>  
>  	return 0;
>  }
> +
> +static int riscv_vdso_get_cpus(struct riscv_hwprobe *pairs, size_t pair_count,
> +			       size_t cpusetsize, unsigned long *cpus,
> +			       unsigned int flags)
> +{
> +	const struct vdso_data *vd = __arch_get_vdso_data();
> +	const struct arch_vdso_data *avd = &vd->arch_data;
> +	struct riscv_hwprobe *p = pairs;
> +	struct riscv_hwprobe *end = pairs + pair_count;
> +	bool clear_all = false;
> +
> +	if (!cpusetsize || !cpus)
> +		return -EINVAL;
> +
> +	if (flags != RISCV_HWPROBE_WHICH_CPUS || !avd->homogeneous_cpus)
> +		return riscv_hwprobe(pairs, pair_count, cpusetsize, cpus, flags);
> +
> +	while (p < end) {
> +		if (riscv_hwprobe_key_is_valid(p->key)) {
> +			struct riscv_hwprobe t = {
> +				.key = p->key,
> +				.value = avd->all_cpu_hwprobe_values[p->key],
> +			};
> +
> +			if (!riscv_hwprobe_pair_cmp(&t, p))
> +				clear_all = true;
> +		} else {
> +			clear_all = true;
> +			p->key = -1;
> +			p->value = 0;
> +		}
> +		p++;
> +	}
> +
> +	if (clear_all)
> +		memset(cpus, 0, cpusetsize);

This memset will go away in v2. It wasn't very smart of me to put it there
in the first place as there's no memset available to vdso code, which
means it would segfault when attempting to use it. My initial testing
failed to find this, because I had forgotten that avd->homogeneous_cpus
would be false on a default QEMU machine since it doesn't set mvendorid to
anything. I actually found this problem when trying to add a memcmp which
wasn't skipped with !avd->homogeneous_cpus, and that promptly segfaulted.
(I won't be adding that memcmp either :-)

Thanks,
drew

> +
> +	return 0;
> +}
> +
> +/* Add a prototype to avoid -Wmissing-prototypes warning. */
> +int __vdso_riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
> +			 size_t cpusetsize, unsigned long *cpus,
> +			 unsigned int flags);
> +
> +int __vdso_riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
> +			 size_t cpusetsize, unsigned long *cpus,
> +			 unsigned int flags)
> +{
> +	if (flags & RISCV_HWPROBE_WHICH_CPUS)
> +		return riscv_vdso_get_cpus(pairs, pair_count, cpusetsize,
> +					   cpus, flags);
> +
> +	return riscv_vdso_get_values(pairs, pair_count, cpusetsize,
> +				     cpus, flags);
> +}
> -- 
> 2.41.0
> 

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