Skip to contentSkip to conversation

RISC-V: Enable cbo.zero in usermode

PATCH Applied Andrew Jones 5 participants 23 messages Aug 9, 2023 – Aug 30, 2023

Subsystems (3)
Documentation Kselftest RISC-V

v1 of 4 Applied 6/6 Latest is v4 →

lore
Share & tools
lore

Andrew Jones

author
#0 lore raw

cover RISC-V: Enable cbo.zero in usermode

In order for usermode to issue cbo.zero, it needs privilege granted to issue the extension instruction (patch 2) and to know that the extension is available and its block size (patch 3). Patch 1 could be separate from this series (it just fixes up some error messages), patches 4-5 convert the hwprobe selftest to a statically-linked, TAP test and patch 6 adds a new hwprobe test for the new information as well as testing CBO instructions can or cannot be issued as appropriate.

Thanks,
drew

Andrew Jones (6):
RISC-V: Make zicbom/zicboz errors consistent
RISC-V: Enable cbo.zero in usermode
RISC-V: hwprobe: Expose Zicboz extension and its block size
RISC-V: selftests: Statically link hwprobe test
RISC-V: selftests: Convert hwprobe test to kselftest API
RISC-V: selftests: Add CBO tests

 Documentation/riscv/hwprobe.rst               |   6 +
 arch/riscv/include/asm/cpufeature.h           |   2 +
 arch/riscv/include/asm/csr.h                  |   1 +
 arch/riscv/include/asm/hwcap.h                |  16 ++
 arch/riscv/include/asm/hwprobe.h              |   2 +-
 arch/riscv/include/uapi/asm/hwprobe.h         |   2 +
 arch/riscv/kernel/cpufeature.c                |  10 +-
 arch/riscv/kernel/setup.c                     |   4 +
 arch/riscv/kernel/smpboot.c                   |   4 +
 arch/riscv/kernel/sys_riscv.c                 |  41 +++--
 tools/testing/selftests/riscv/Makefile        |   2 +-
 .../testing/selftests/riscv/hwprobe/Makefile  |   7 +-
 tools/testing/selftests/riscv/hwprobe/cbo.c   | 160 ++++++++++++++++++
 .../testing/selftests/riscv/hwprobe/hwprobe.c |  64 +++----
 .../testing/selftests/riscv/hwprobe/hwprobe.h |  15 ++
 15 files changed, 272 insertions(+), 64 deletions(-)
 create mode 100644 tools/testing/selftests/riscv/hwprobe/cbo.c
 create mode 100644 tools/testing/selftests/riscv/hwprobe/hwprobe.h
Signature · 7 lines
2.41.0


_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
author
#1 lore raw

PATCH 1/6 RISC-V: Make zicbom/zicboz errors consistent

commit c818fea83de4 ("riscv: say disabling zicbom if no or bad riscv,cbom-block-size found") improved the error messages for zicbom but zicboz was missed since its patches were in flight at the same time. Get 'em now.

Signed-off-by Andrew Jones <[email protected]>
---
 arch/riscv/kernel/cpufeature.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
arch/riscv/kernel/cpufeature.c +2 -2
index a8f66c015229..31843e9cc80c 100644
@@ -87,10 +87,10 @@ static bool riscv_isa_extension_check(int id)
 		return true;
 	case RISCV_ISA_EXT_ZICBOZ:
 		if (!riscv_cboz_block_size) {
-			pr_err("Zicboz detected in ISA string, but no cboz-block-size found\n");
+			pr_err("Zicboz detected in ISA string, disabling as no cboz-block-size found\n");
 			return false;
 		} else if (!is_power_of_2(riscv_cboz_block_size)) {
-			pr_err("cboz-block-size present, but is not a power-of-2\n");
+			pr_err("Zicboz disabled as cboz-block-size present, but is not a power-of-2\n");
 			return false;
 		}
 		return true;
Signature · 7 lines
2.41.0


_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
Reviewed-by on v1 1/6
#2 lore raw

Re: [PATCH 1/6] RISC-V: Make zicbom/zicboz errors consistent

Quoted 6 lines · Andrew Jones

commit c818fea83de4 ("riscv: say disabling zicbom if no or bad riscv,cbom-block-size found") improved the error messages for zicbom but zicboz was missed since its patches were in flight at the same time. Get 'em now.

Signed-off-by Andrew Jones <[email protected]>
Reviewed-by Conor Dooley <[email protected]>

Thanks,
Conor.

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

author
#3 lore raw

PATCH 2/6 RISC-V: Enable cbo.zero in usermode

In reply to Andrew Jones

When Zicboz is present, enable its instruction (cbo.zero) in usermode by setting its respective senvcfg bit. We don't bother trying to set this bit per-task, which would also require an interface for tasks to request enabling and/or disabling. Instead, permanently set the bit for each hart which has the extension when bringing it online.

Signed-off-by Andrew Jones <[email protected]>
---
 arch/riscv/include/asm/cpufeature.h |  2 ++
 arch/riscv/include/asm/csr.h        |  1 +
 arch/riscv/include/asm/hwcap.h      | 16 ++++++++++++++++
 arch/riscv/kernel/cpufeature.c      |  6 ++++++
 arch/riscv/kernel/setup.c           |  4 ++++
 arch/riscv/kernel/smpboot.c         |  4 ++++
 6 files changed, 33 insertions(+)
arch/riscv/include/asm/cpufeature.h +2 -0
index 23fed53b8815..788fd575c21a 100644
@@ -30,4 +30,6 @@ DECLARE_PER_CPU(long, misaligned_access_speed);
 /* Per-cpu ISA extensions. */
 extern struct riscv_isainfo hart_isa[NR_CPUS];
 
+void riscv_user_isa_enable(void);
+
 #endif
arch/riscv/include/asm/csr.h +1 -0
index 7bac43a3176e..e187e76e3df4 100644
@@ -273,6 +273,7 @@
 #define CSR_SIE			0x104
 #define CSR_STVEC		0x105
 #define CSR_SCOUNTEREN		0x106
+#define CSR_SENVCFG		0x10a
 #define CSR_SSCRATCH		0x140
 #define CSR_SEPC		0x141
 #define CSR_SCAUSE		0x142
arch/riscv/include/asm/hwcap.h +16 -0
index f041bfa7f6a0..4929faecb75f 100644
@@ -66,6 +66,7 @@
 #ifndef __ASSEMBLY__
 
 #include <linux/jump_label.h>
+#include <asm/cpufeature.h>
 
 unsigned long riscv_get_elf_hwcap(void);
 

@@ -130,6 +131,21 @@ riscv_has_extension_unlikely(const unsigned long ext)
 	return true;
 }
 
+static __always_inline bool riscv_this_cpu_has_extension_likely(const unsigned long ext)
+{
+	if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_likely(ext))
+		return true;
+
+	return __riscv_isa_extension_available(hart_isa[smp_processor_id()].isa, ext);
+}
+
+static __always_inline bool riscv_this_cpu_has_extension_unlikely(const unsigned long ext)
+{
+	if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_unlikely(ext))
+		return true;
+
+	return __riscv_isa_extension_available(hart_isa[smp_processor_id()].isa, ext);
+}
 #endif
 
 #endif /* _ASM_RISCV_HWCAP_H */
arch/riscv/kernel/cpufeature.c +6 -0
index 31843e9cc80c..fc0bf300acc7 100644
@@ -391,6 +391,12 @@ unsigned long riscv_get_elf_hwcap(void)
 	return hwcap;
 }
 
+void riscv_user_isa_enable(void)
+{
+	if (riscv_this_cpu_has_extension_unlikely(RISCV_ISA_EXT_ZICBOZ))
+		csr_set(CSR_SENVCFG, ENVCFG_CBZE);
+}
+
 #ifdef CONFIG_RISCV_ALTERNATIVE
 /*
  * Alternative patch sites consider 48 bits when determining when to patch
arch/riscv/kernel/setup.c +4 -0
index 971fe776e2f8..2f053f0763a1 100644
@@ -25,6 +25,7 @@
 #include <asm/acpi.h>
 #include <asm/alternative.h>
 #include <asm/cacheflush.h>
+#include <asm/cpufeature.h>
 #include <asm/cpu_ops.h>
 #include <asm/early_ioremap.h>
 #include <asm/pgtable.h>

@@ -308,9 +309,12 @@ void __init setup_arch(char **cmdline_p)
 	riscv_fill_hwcap();
 	init_rt_signal_env();
 	apply_boot_alternatives();
+
 	if (IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM) &&
 	    riscv_isa_extension_available(NULL, ZICBOM))
 		riscv_noncoherent_supported();
+
+	riscv_user_isa_enable();
 }
 
 static int __init topology_init(void)
arch/riscv/kernel/smpboot.c +4 -0
index f4d6acb38dd0..502b04abda0b 100644
@@ -25,6 +25,8 @@
 #include <linux/of.h>
 #include <linux/sched/task_stack.h>
 #include <linux/sched/mm.h>
+
+#include <asm/cpufeature.h>
 #include <asm/cpu_ops.h>
 #include <asm/irq.h>
 #include <asm/mmu_context.h>

@@ -252,6 +254,8 @@ asmlinkage __visible void smp_callin(void)
 			elf_hwcap &= ~COMPAT_HWCAP_ISA_V;
 	}
 
+	riscv_user_isa_enable();
+
 	/*
 	 * Remote TLB flushes are ignored while the CPU is offline, so emit
 	 * a local TLB flush right now just in case.
Signature · 7 lines
2.41.0


_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
#4 lore raw
Quoted 62 lines · Andrew Jones

When Zicboz is present, enable its instruction (cbo.zero) in usermode by setting its respective senvcfg bit. We don't bother trying to set this bit per-task, which would also require an interface for tasks to request enabling and/or disabling. Instead, permanently set the bit for each hart which has the extension when bringing it online.

Signed-off-by Andrew Jones <[email protected]>
---
 arch/riscv/include/asm/cpufeature.h |  2 ++
 arch/riscv/include/asm/csr.h        |  1 +
 arch/riscv/include/asm/hwcap.h      | 16 ++++++++++++++++
 arch/riscv/kernel/cpufeature.c      |  6 ++++++
 arch/riscv/kernel/setup.c           |  4 ++++
 arch/riscv/kernel/smpboot.c         |  4 ++++
 6 files changed, 33 insertions(+)
arch/riscv/include/asm/cpufeature.h +2 -0
index 23fed53b8815..788fd575c21a 100644
@@ -30,4 +30,6 @@ DECLARE_PER_CPU(long, misaligned_access_speed);
 /* Per-cpu ISA extensions. */
 extern struct riscv_isainfo hart_isa[NR_CPUS];
 
+void riscv_user_isa_enable(void);
+
 #endif
arch/riscv/include/asm/csr.h +1 -0
index 7bac43a3176e..e187e76e3df4 100644
@@ -273,6 +273,7 @@
 #define CSR_SIE                        0x104
 #define CSR_STVEC              0x105
 #define CSR_SCOUNTEREN         0x106
+#define CSR_SENVCFG            0x10a
 #define CSR_SSCRATCH           0x140
 #define CSR_SEPC               0x141
 #define CSR_SCAUSE             0x142
arch/riscv/include/asm/hwcap.h +6 -0
index f041bfa7f6a0..4929faecb75f 100644
@@ -66,6 +66,7 @@
 #ifndef __ASSEMBLY__
 
 #include <linux/jump_label.h>
+#include <asm/cpufeature.h>
 
 unsigned long riscv_get_elf_hwcap(void);
 

@@ -130,6 +131,21 @@ riscv_has_extension_unlikely(const unsigned long ext)
        return true;
 }
 
+static __always_inline bool riscv_this_cpu_has_extension_likely(const unsigned long ext)
+{
+       if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_likely(ext))
+               return true;
+

arch/riscv/include/asm/hwcap.h riscv_has_extension_unlikely(const unsigned long ext) · line 148

Quoted 10 lines
arch/riscv/include/asm/hwcap.h +10 -0
+       return __riscv_isa_extension_available(hart_isa[smp_processor_id()].isa, ext);
+}
+
+static __always_inline bool riscv_this_cpu_has_extension_unlikely(const unsigned long ext)
+{
+       if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_unlikely(ext))
+               return true;
+
+       return __riscv_isa_extension_available(hart_isa[smp_processor_id()].isa, ext);
+}

Another way to do this would be to add a parameter to riscv_has_extension_*() (as there are very few users), then these new functions can turn around and call those with the new parameter set to hart_isa[smp_processor_id()].isa. It's a tossup, so up to you. The only advantage to it I can argue is it keeps the code flows more unified.

-Evan

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

author
#5 lore raw
Quoted 1 line · Evan Green

On Wed, Aug 9, 2023 at 4:55 AM Andrew Jones <[email protected]> wrote:

...

Quoted 14 lines
Quoted 14 lines
arch/riscv/include/asm/hwcap.h +14 -0
+static __always_inline bool riscv_this_cpu_has_extension_likely(const unsigned long ext)
+{
+       if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_likely(ext))
+               return true;
+
+       return __riscv_isa_extension_available(hart_isa[smp_processor_id()].isa, ext);
+}
+
+static __always_inline bool riscv_this_cpu_has_extension_unlikely(const unsigned long ext)
+{
+       if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_unlikely(ext))
+               return true;
+
+       return __riscv_isa_extension_available(hart_isa[smp_processor_id()].isa, ext);
Quoted 9 lines
Quoted 1 line

+}

Another way to do this would be to add a parameter to riscv_has_extension_*() (as there are very few users), then these new functions can turn around and call those with the new parameter set to hart_isa[smp_processor_id()].isa. It's a tossup, so up to you. The only advantage to it I can argue is it keeps the code flows more unified.

I like unification, but I think I'd prefer we create wrappers and try to avoid callers needing to construct hart_isa[].isa parameters themselves. I'm also not a big fan of the NULL parameter needed when riscv_isa_extension_available() is invoked for the riscv_isa bitmap. So we need:

1. check if an extension is in riscv_isa
2. check if an extension is in a bitmap provided by the caller
3. check if an extension is in this cpu's isa bitmap
4. check if an extension is in the isa bitmap of a cpu provided by the caller

The only one we can optimize with alternatives is (1), so it definitely gets wrappers (riscv_has_extension_likely/unlikely()). (3) and (4) can also get wrappers which first try the optimized (1), like I have above. Actually (3)'s wrapper could be based on (4)'s, or only provide wrappers for (4)

static __always_inline bool riscv_cpu_has_extension_likely(int cpu, const unsigned long ext)

 {
     if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_likely(ext))
         return true;

     return __riscv_isa_extension_available(hart_isa[cpu].isa, ext);

}

static __always_inline bool riscv_cpu_has_extension_unlikely(int cpu, const unsigned long ext)

 {
     if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_unlikely(ext))
         return true;

     return __riscv_isa_extension_available(hart_isa[cpu].isa, ext);

}

and then use smp_processor_id() directly in the callers that need to check this_cpu's extensions.

For case (2), I'd advocate we rename __riscv_isa_extension_available() to riscv_has_extension() and drop the riscv_isa_extension_available() macro in order to avoid having some calls with RISCV_ISA_EXT_* spelled out and others that rely on the pasting. And, ideally, we'd convert most riscv_has_extension(NULL, ext) calls to riscv_has_extension_[un]likely().

Thanks,
drew

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

#6 lore raw
Quoted 51 lines · Andrew Jones
Quoted 1 line · Evan Green

On Wed, Aug 9, 2023 at 4:55 AM Andrew Jones <[email protected]> wrote:

...

Quoted 14 lines
Quoted 14 lines
arch/riscv/include/asm/hwcap.h +14 -0
+static __always_inline bool riscv_this_cpu_has_extension_likely(const unsigned long ext)
+{
+       if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_likely(ext))
+               return true;
+
+       return __riscv_isa_extension_available(hart_isa[smp_processor_id()].isa, ext);
+}
+
+static __always_inline bool riscv_this_cpu_has_extension_unlikely(const unsigned long ext)
+{
+       if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_unlikely(ext))
+               return true;
+
+       return __riscv_isa_extension_available(hart_isa[smp_processor_id()].isa, ext);
Quoted 9 lines
Quoted 1 line

+}

Another way to do this would be to add a parameter to riscv_has_extension_*() (as there are very few users), then these new functions can turn around and call those with the new parameter set to hart_isa[smp_processor_id()].isa. It's a tossup, so up to you. The only advantage to it I can argue is it keeps the code flows more unified.

I like unification, but I think I'd prefer we create wrappers and try to avoid callers needing to construct hart_isa[].isa parameters themselves. I'm also not a big fan of the NULL parameter needed when riscv_isa_extension_available() is invoked for the riscv_isa bitmap. So we need:

1. check if an extension is in riscv_isa
2. check if an extension is in a bitmap provided by the caller
3. check if an extension is in this cpu's isa bitmap
4. check if an extension is in the isa bitmap of a cpu provided by the caller

The only one we can optimize with alternatives is (1), so it definitely gets wrappers (riscv_has_extension_likely/unlikely()). (3) and (4) can also get wrappers which first try the optimized (1), like I have above. Actually (3)'s wrapper could be based on (4)'s, or only provide wrappers for (4)

static __always_inline bool riscv_cpu_has_extension_likely(int cpu, const unsigned long ext)

 {
     if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_likely(ext))
         return true;

     return __riscv_isa_extension_available(hart_isa[cpu].isa, ext);
Quoted 5 lines

}

static __always_inline bool riscv_cpu_has_extension_unlikely(int cpu, const unsigned long ext)

 {
     if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_unlikely(ext))

Why are you gating on CONFIG_RISCV_ALTERNATIVE here?

Quoted 13 lines
         return true;

     return __riscv_isa_extension_available(hart_isa[cpu].isa, ext);

}

and then use smp_processor_id() directly in the callers that need to check this_cpu's extensions.

For case (2), I'd advocate we rename __riscv_isa_extension_available() to riscv_has_extension() and drop the riscv_isa_extension_available() macro in order to avoid having some calls with RISCV_ISA_EXT_* spelled out and others that rely on the pasting.

Quoted 5 lines

And, ideally, we'd convert most
riscv_has_extension(NULL, ext) calls to riscv_has_extension_[un]likely().

I'm also not a big fan of the NULL parameter needed when riscv_isa_extension_available() is invoked for the riscv_isa bitmap

Rather than actually act on my concerns about __riscv_isa_extension_available(), I've been busy devoting my spare time to playing MMOs with the excuse of not wanting to fiddle further with cpufeature.c et al until Palmer merged the new DT property stuff, but splitting out your case 1 above seems like it would really help there. The NULL argument case is the one I think has the potential to be a footgun in the face of config options.
Split out we can document that purpose of each function & hopefully have one set of functions that deals with "this extension was detected to be present in the hardware" and one that does "this extension was detected & supported by this particular kernel".

I'll try to take a proper look at this series tomorrow :)

Cheers,
Conor.

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

author
#7 lore raw
Quoted 51 lines · Conor Dooley
Quoted 50 lines · Andrew Jones
Quoted 1 line · Evan Green

On Wed, Aug 9, 2023 at 4:55 AM Andrew Jones <[email protected]> wrote:

...

Quoted 14 lines
Quoted 14 lines
arch/riscv/include/asm/hwcap.h +14 -0
+static __always_inline bool riscv_this_cpu_has_extension_likely(const unsigned long ext)
+{
+       if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_likely(ext))
+               return true;
+
+       return __riscv_isa_extension_available(hart_isa[smp_processor_id()].isa, ext);
+}
+
+static __always_inline bool riscv_this_cpu_has_extension_unlikely(const unsigned long ext)
+{
+       if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_unlikely(ext))
+               return true;
+
+       return __riscv_isa_extension_available(hart_isa[smp_processor_id()].isa, ext);
Quoted 9 lines
Quoted 1 line

+}

Another way to do this would be to add a parameter to riscv_has_extension_*() (as there are very few users), then these new functions can turn around and call those with the new parameter set to hart_isa[smp_processor_id()].isa. It's a tossup, so up to you. The only advantage to it I can argue is it keeps the code flows more unified.

I like unification, but I think I'd prefer we create wrappers and try to avoid callers needing to construct hart_isa[].isa parameters themselves. I'm also not a big fan of the NULL parameter needed when riscv_isa_extension_available() is invoked for the riscv_isa bitmap. So we need:

1. check if an extension is in riscv_isa
2. check if an extension is in a bitmap provided by the caller
3. check if an extension is in this cpu's isa bitmap
4. check if an extension is in the isa bitmap of a cpu provided by the caller

The only one we can optimize with alternatives is (1), so it definitely gets wrappers (riscv_has_extension_likely/unlikely()). (3) and (4) can also get wrappers which first try the optimized (1), like I have above. Actually (3)'s wrapper could be based on (4)'s, or only provide wrappers for (4)

static __always_inline bool riscv_cpu_has_extension_likely(int cpu, const unsigned long ext)

 {
     if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_likely(ext))
         return true;
Quoted 8 lines
Quoted 6 lines
     return __riscv_isa_extension_available(hart_isa[cpu].isa, ext);

}

static __always_inline bool riscv_cpu_has_extension_unlikely(int cpu, const unsigned long ext)

 {
     if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_unlikely(ext))

Why are you gating on CONFIG_RISCV_ALTERNATIVE here?

This ensures we remove the riscv_has_extension_[un]likely() call when that call would end up using its __riscv_isa_extension_available(NULL, ext) fallback. If that fallback where to return false, then we'd still need to make the __riscv_isa_extension_available(hart_isa[cpu].isa, ext) call, doubling the cost. Whereas, when we gate on CONFIG_RISCV_ALTERNATIVE, we know that riscv_has_extension_[un]likely() will use an alternative to check the global set of extensions. When the extension is there, the compiler ensures that everything vanishes. When it's not, we'll fallback to a single search of the cpu's isa bitmap.

Quoted 20 lines
Quoted 14 lines
         return true;

     return __riscv_isa_extension_available(hart_isa[cpu].isa, ext);

}

and then use smp_processor_id() directly in the callers that need to check this_cpu's extensions.

For case (2), I'd advocate we rename __riscv_isa_extension_available() to riscv_has_extension() and drop the riscv_isa_extension_available() macro in order to avoid having some calls with RISCV_ISA_EXT_* spelled out and others that rely on the pasting.

Quoted 5 lines

And, ideally, we'd convert most
riscv_has_extension(NULL, ext) calls to riscv_has_extension_[un]likely().

I'm also not a big fan of the NULL parameter needed when riscv_isa_extension_available() is invoked for the riscv_isa bitmap

Quoted 11 lines

Rather than actually act on my concerns about __riscv_isa_extension_available(), I've been busy devoting my spare time to playing MMOs with the excuse of not wanting to fiddle further with cpufeature.c et al until Palmer merged the new DT property stuff, but splitting out your case 1 above seems like it would really help there. The NULL argument case is the one I think has the potential to be a footgun in the face of config options.
Split out we can document that purpose of each function & hopefully have one set of functions that deals with "this extension was detected to be present in the hardware" and one that does "this extension was detected & supported by this particular kernel".

Sounds good to me!

Quoted 3 lines

I'll try to take a proper look at this series tomorrow :)

Thanks!
drew

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

#8 lore raw
Quoted 61 lines · Andrew Jones
Quoted 51 lines · Conor Dooley
Quoted 50 lines · Andrew Jones
Quoted 1 line · Evan Green

On Wed, Aug 9, 2023 at 4:55 AM Andrew Jones <[email protected]> wrote:

...

Quoted 14 lines
Quoted 14 lines
arch/riscv/include/asm/hwcap.h +14 -0
+static __always_inline bool riscv_this_cpu_has_extension_likely(const unsigned long ext)
+{
+       if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_likely(ext))
+               return true;
+
+       return __riscv_isa_extension_available(hart_isa[smp_processor_id()].isa, ext);
+}
+
+static __always_inline bool riscv_this_cpu_has_extension_unlikely(const unsigned long ext)
+{
+       if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_unlikely(ext))
+               return true;
+
+       return __riscv_isa_extension_available(hart_isa[smp_processor_id()].isa, ext);
Quoted 9 lines
Quoted 1 line

+}

Another way to do this would be to add a parameter to riscv_has_extension_*() (as there are very few users), then these new functions can turn around and call those with the new parameter set to hart_isa[smp_processor_id()].isa. It's a tossup, so up to you. The only advantage to it I can argue is it keeps the code flows more unified.

I like unification, but I think I'd prefer we create wrappers and try to avoid callers needing to construct hart_isa[].isa parameters themselves. I'm also not a big fan of the NULL parameter needed when riscv_isa_extension_available() is invoked for the riscv_isa bitmap. So we need:

1. check if an extension is in riscv_isa
2. check if an extension is in a bitmap provided by the caller
3. check if an extension is in this cpu's isa bitmap
4. check if an extension is in the isa bitmap of a cpu provided by the caller

The only one we can optimize with alternatives is (1), so it definitely gets wrappers (riscv_has_extension_likely/unlikely()). (3) and (4) can also get wrappers which first try the optimized (1), like I have above. Actually (3)'s wrapper could be based on (4)'s, or only provide wrappers for (4)

static __always_inline bool riscv_cpu_has_extension_likely(int cpu, const unsigned long ext)

 {
     if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_likely(ext))
         return true;
Quoted 8 lines
Quoted 6 lines
     return __riscv_isa_extension_available(hart_isa[cpu].isa, ext);

}

static __always_inline bool riscv_cpu_has_extension_unlikely(int cpu, const unsigned long ext)

 {
     if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_unlikely(ext))

Why are you gating on CONFIG_RISCV_ALTERNATIVE here?

Quoted 10 lines

This ensures we remove the riscv_has_extension_[un]likely() call when that call would end up using its __riscv_isa_extension_available(NULL, ext) fallback. If that fallback where to return false, then we'd still need to make the __riscv_isa_extension_available(hart_isa[cpu].isa, ext) call, doubling the cost. Whereas, when we gate on CONFIG_RISCV_ALTERNATIVE, we know that riscv_has_extension_[un]likely() will use an alternative to check the global set of extensions. When the extension is there, the compiler ensures that everything vanishes. When it's not, we'll fallback to a single search of the cpu's isa bitmap.

Right, that is what I suspected that you were trying to accomplish here. I was not just not entirely sure whether it was or you'd just missed the fallback path. In my original mail I was just going to say "Please add a comment here as to why you want to avoid the fallback", but figured I should figure out your intent first!

Just to note, alternatives are available on all !XIP kernels now, so it's only in the case that the fallback path will be activated.

Quoted 31 lines
Quoted 19 lines
Quoted 13 lines
         return true;

     return __riscv_isa_extension_available(hart_isa[cpu].isa, ext);

}

and then use smp_processor_id() directly in the callers that need to check this_cpu's extensions.

For case (2), I'd advocate we rename __riscv_isa_extension_available() to riscv_has_extension() and drop the riscv_isa_extension_available() macro in order to avoid having some calls with RISCV_ISA_EXT_* spelled out and others that rely on the pasting.

Quoted 5 lines

And, ideally, we'd convert most
riscv_has_extension(NULL, ext) calls to riscv_has_extension_[un]likely().

I'm also not a big fan of the NULL parameter needed when riscv_isa_extension_available() is invoked for the riscv_isa bitmap

Quoted 11 lines

Rather than actually act on my concerns about __riscv_isa_extension_available(), I've been busy devoting my spare time to playing MMOs with the excuse of not wanting to fiddle further with cpufeature.c et al until Palmer merged the new DT property stuff, but splitting out your case 1 above seems like it would really help there. The NULL argument case is the one I think has the potential to be a footgun in the face of config options.
Split out we can document that purpose of each function & hopefully have one set of functions that deals with "this extension was detected to be present in the hardware" and one that does "this extension was detected & supported by this particular kernel".

Quoted 1 line

Sounds good to me!

I figure said change should be independent of what's going on in this series?

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

author
#9 lore raw
Quoted 73 lines · Conor Dooley
Quoted 61 lines · Andrew Jones
Quoted 51 lines · Conor Dooley
Quoted 50 lines · Andrew Jones
Quoted 1 line · Evan Green

On Wed, Aug 9, 2023 at 4:55 AM Andrew Jones <[email protected]> wrote:

...

Quoted 14 lines
Quoted 14 lines
arch/riscv/include/asm/hwcap.h +14 -0
+static __always_inline bool riscv_this_cpu_has_extension_likely(const unsigned long ext)
+{
+       if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_likely(ext))
+               return true;
+
+       return __riscv_isa_extension_available(hart_isa[smp_processor_id()].isa, ext);
+}
+
+static __always_inline bool riscv_this_cpu_has_extension_unlikely(const unsigned long ext)
+{
+       if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_unlikely(ext))
+               return true;
+
+       return __riscv_isa_extension_available(hart_isa[smp_processor_id()].isa, ext);
Quoted 9 lines
Quoted 1 line

+}

Another way to do this would be to add a parameter to riscv_has_extension_*() (as there are very few users), then these new functions can turn around and call those with the new parameter set to hart_isa[smp_processor_id()].isa. It's a tossup, so up to you. The only advantage to it I can argue is it keeps the code flows more unified.

I like unification, but I think I'd prefer we create wrappers and try to avoid callers needing to construct hart_isa[].isa parameters themselves. I'm also not a big fan of the NULL parameter needed when riscv_isa_extension_available() is invoked for the riscv_isa bitmap. So we need:

1. check if an extension is in riscv_isa
2. check if an extension is in a bitmap provided by the caller
3. check if an extension is in this cpu's isa bitmap
4. check if an extension is in the isa bitmap of a cpu provided by the caller

The only one we can optimize with alternatives is (1), so it definitely gets wrappers (riscv_has_extension_likely/unlikely()). (3) and (4) can also get wrappers which first try the optimized (1), like I have above. Actually (3)'s wrapper could be based on (4)'s, or only provide wrappers for (4)

static __always_inline bool riscv_cpu_has_extension_likely(int cpu, const unsigned long ext)

 {
     if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_likely(ext))
         return true;
Quoted 8 lines
Quoted 6 lines
     return __riscv_isa_extension_available(hart_isa[cpu].isa, ext);

}

static __always_inline bool riscv_cpu_has_extension_unlikely(int cpu, const unsigned long ext)

 {
     if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_unlikely(ext))

Why are you gating on CONFIG_RISCV_ALTERNATIVE here?

Quoted 10 lines

This ensures we remove the riscv_has_extension_[un]likely() call when that call would end up using its __riscv_isa_extension_available(NULL, ext) fallback. If that fallback where to return false, then we'd still need to make the __riscv_isa_extension_available(hart_isa[cpu].isa, ext) call, doubling the cost. Whereas, when we gate on CONFIG_RISCV_ALTERNATIVE, we know that riscv_has_extension_[un]likely() will use an alternative to check the global set of extensions. When the extension is there, the compiler ensures that everything vanishes. When it's not, we'll fallback to a single search of the cpu's isa bitmap.

Quoted 5 lines

Right, that is what I suspected that you were trying to accomplish here. I was not just not entirely sure whether it was or you'd just missed the fallback path. In my original mail I was just going to say "Please add a comment here as to why you want to avoid the fallback", but figured I should figure out your intent first!

Thanks, I'll add a comment for v2.

Quoted 35 lines

Just to note, alternatives are available on all !XIP kernels now, so it's only in the case that the fallback path will be activated.

Quoted 31 lines
Quoted 30 lines
Quoted 13 lines
         return true;

     return __riscv_isa_extension_available(hart_isa[cpu].isa, ext);

}

and then use smp_processor_id() directly in the callers that need to check this_cpu's extensions.

For case (2), I'd advocate we rename __riscv_isa_extension_available() to riscv_has_extension() and drop the riscv_isa_extension_available() macro in order to avoid having some calls with RISCV_ISA_EXT_* spelled out and others that rely on the pasting.

Quoted 5 lines

And, ideally, we'd convert most
riscv_has_extension(NULL, ext) calls to riscv_has_extension_[un]likely().

I'm also not a big fan of the NULL parameter needed when riscv_isa_extension_available() is invoked for the riscv_isa bitmap

Rather than actually act on my concerns about __riscv_isa_extension_available(), I've been busy devoting my spare time to playing MMOs with the excuse of not wanting to fiddle further with cpufeature.c et al until Palmer merged the new DT property stuff, but splitting out your case 1 above seems like it would really help there. The NULL argument case is the one I think has the potential to be a footgun in the face of config options.
Split out we can document that purpose of each function & hopefully have one set of functions that deals with "this extension was detected to be present in the hardware" and one that does "this extension was detected & supported by this particular kernel".

Quoted 4 lines
Quoted 1 line

Sounds good to me!

I figure said change should be independent of what's going on in this series?

Yeah, I considered doing it as part of this series, but then decided to only start down the path with two new wrappers, which, for v2, I'll change to the cpu parameter taking variant. The rest of the rework would probably be best to do as a separate series, which I can start soon.

Thanks,
drew

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

#10 lore raw
Quoted 11 lines · Andrew Jones
Quoted 2 lines · Conor Dooley

I figure said change should be independent of what's going on in this series?

Yeah, I considered doing it as part of this series, but then decided to only start down the path with two new wrappers, which, for v2, I'll change to the cpu parameter taking variant. The rest of the rework would probably be best to do as a separate series, which I can start soon.

If you're offering, who am I to refuse!

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

#11 lore raw
In reply to Andrew Jones
Quoted 62 lines · Andrew Jones
Quoted 1 line · Evan Green

On Wed, Aug 9, 2023 at 4:55 AM Andrew Jones <[email protected]> wrote:

...

Quoted 14 lines
Quoted 14 lines
arch/riscv/include/asm/hwcap.h +14 -0
+static __always_inline bool riscv_this_cpu_has_extension_likely(const unsigned long ext)
+{
+       if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_likely(ext))
+               return true;
+
+       return __riscv_isa_extension_available(hart_isa[smp_processor_id()].isa, ext);
+}
+
+static __always_inline bool riscv_this_cpu_has_extension_unlikely(const unsigned long ext)
+{
+       if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_unlikely(ext))
+               return true;
+
+       return __riscv_isa_extension_available(hart_isa[smp_processor_id()].isa, ext);
Quoted 9 lines
Quoted 1 line

+}

Another way to do this would be to add a parameter to riscv_has_extension_*() (as there are very few users), then these new functions can turn around and call those with the new parameter set to hart_isa[smp_processor_id()].isa. It's a tossup, so up to you. The only advantage to it I can argue is it keeps the code flows more unified.

I like unification, but I think I'd prefer we create wrappers and try to avoid callers needing to construct hart_isa[].isa parameters themselves. I'm also not a big fan of the NULL parameter needed when riscv_isa_extension_available() is invoked for the riscv_isa bitmap. So we need:

1. check if an extension is in riscv_isa
2. check if an extension is in a bitmap provided by the caller
3. check if an extension is in this cpu's isa bitmap
4. check if an extension is in the isa bitmap of a cpu provided by the caller

The only one we can optimize with alternatives is (1), so it definitely gets wrappers (riscv_has_extension_likely/unlikely()). (3) and (4) can also get wrappers which first try the optimized (1), like I have above. Actually (3)'s wrapper could be based on (4)'s, or only provide wrappers for (4)

static __always_inline bool riscv_cpu_has_extension_likely(int cpu, const unsigned long ext)

 {
     if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_likely(ext))
         return true;

     return __riscv_isa_extension_available(hart_isa[cpu].isa, ext);

}

static __always_inline bool riscv_cpu_has_extension_unlikely(int cpu, const unsigned long ext)

 {
     if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_unlikely(ext))
         return true;

     return __riscv_isa_extension_available(hart_isa[cpu].isa, ext);

}

Quoted 8 lines

and then use smp_processor_id() directly in the callers that need to check this_cpu's extensions.

For case (2), I'd advocate we rename __riscv_isa_extension_available() to riscv_has_extension() and drop the riscv_isa_extension_available() macro in order to avoid having some calls with RISCV_ISA_EXT_* spelled out and others that rely on the pasting. And, ideally, we'd convert most riscv_has_extension(NULL, ext) calls to riscv_has_extension_[un]likely().

Sounds ok to me!
-Evan

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

author
#12 lore raw

PATCH 3/6 RISC-V: hwprobe: Expose Zicboz extension and its block size

In reply to Andrew Jones

Expose Zicboz through hwprobe and also provide a key to extract its respective block size. Opportunistically add a macro and apply it to current extensions in order to avoid duplicating code.

Signed-off-by Andrew Jones <[email protected]>
---
 Documentation/riscv/hwprobe.rst       |  6 ++++
 arch/riscv/include/asm/hwprobe.h      |  2 +-
 arch/riscv/include/uapi/asm/hwprobe.h |  2 ++
 arch/riscv/kernel/sys_riscv.c         | 41 ++++++++++++++++++---------
 4 files changed, 36 insertions(+), 15 deletions(-)
Documentation/riscv/hwprobe.rst +6 -0
index 933c715065d6..6a17c2872660 100644
@@ -77,6 +77,9 @@ The following keys are defined:
   * :c:macro:`RISCV_HWPROBE_EXT_ZBS`: The Zbs extension is supported, as defined
        in version 1.0 of the Bit-Manipulation ISA extensions.
 
+  * :c:macro:`RISCV_HWPROBE_EXT_ZICBOZ`: The Zicboz extension is supported, as
+       ratified in commit 3dd606f ("Create cmobase-v1.0.pdf") of riscv-CMOs.
+
 * :c:macro:`RISCV_HWPROBE_KEY_CPUPERF_0`: A bitmask that contains performance
   information about the selected set of processors.
 

@@ -97,3 +100,6 @@ The following keys are defined:
 
   * :c:macro:`RISCV_HWPROBE_MISALIGNED_UNSUPPORTED`: Misaligned accesses are
     not supported at all and will generate a misaligned address fault.
+
+* :c:macro:`RISCV_HWPROBE_KEY_ZICBOZ_BLOCK_SIZE`: An unsigned int which
+  represents the size of the Zicboz block in bytes.
arch/riscv/include/asm/hwprobe.h +1 -1
index 78936f4ff513..39df8604fea1 100644
@@ -8,6 +8,6 @@
 
 #include <uapi/asm/hwprobe.h>
 
-#define RISCV_HWPROBE_MAX_KEY 5
+#define RISCV_HWPROBE_MAX_KEY 6
 
 #endif
arch/riscv/include/uapi/asm/hwprobe.h +2 -0
index 006bfb48343d..86d08a0e617b 100644
@@ -29,6 +29,7 @@ struct riscv_hwprobe {
 #define		RISCV_HWPROBE_EXT_ZBA		(1 << 3)
 #define		RISCV_HWPROBE_EXT_ZBB		(1 << 4)
 #define		RISCV_HWPROBE_EXT_ZBS		(1 << 5)
+#define		RISCV_HWPROBE_EXT_ZICBOZ	(1 << 6)
 #define RISCV_HWPROBE_KEY_CPUPERF_0	5
 #define		RISCV_HWPROBE_MISALIGNED_UNKNOWN	(0 << 0)
 #define		RISCV_HWPROBE_MISALIGNED_EMULATED	(1 << 0)

@@ -36,6 +37,7 @@ struct riscv_hwprobe {
 #define		RISCV_HWPROBE_MISALIGNED_FAST		(3 << 0)
 #define		RISCV_HWPROBE_MISALIGNED_UNSUPPORTED	(4 << 0)
 #define		RISCV_HWPROBE_MISALIGNED_MASK		(7 << 0)
+#define RISCV_HWPROBE_KEY_ZICBOZ_BLOCK_SIZE	6
 /* Increase RISCV_HWPROBE_MAX_KEY when adding items. */
 
 #endif
arch/riscv/kernel/sys_riscv.c +27 -14

59 lines not on this page · raw message

Signature · 7 lines
2.41.0


_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
Reviewed-by on v1 3/6
#13 lore raw

Re: [PATCH 3/6] RISC-V: hwprobe: Expose Zicboz extension and its block size

Quoted 6 lines · Andrew Jones

Expose Zicboz through hwprobe and also provide a key to extract its respective block size. Opportunistically add a macro and apply it to current extensions in order to avoid duplicating code.

Signed-off-by Andrew Jones <[email protected]>
Reviewed-by Evan Green <[email protected]>

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

#14 lore raw

Re: [PATCH 3/6] RISC-V: hwprobe: Expose Zicboz extension and its block size

In reply to Andrew Jones
Quoted 84 lines · Andrew Jones

Expose Zicboz through hwprobe and also provide a key to extract its respective block size. Opportunistically add a macro and apply it to current extensions in order to avoid duplicating code.

Signed-off-by Andrew Jones <[email protected]>
---
 Documentation/riscv/hwprobe.rst       |  6 ++++
 arch/riscv/include/asm/hwprobe.h      |  2 +-
 arch/riscv/include/uapi/asm/hwprobe.h |  2 ++
 arch/riscv/kernel/sys_riscv.c         | 41 ++++++++++++++++++---------
 4 files changed, 36 insertions(+), 15 deletions(-)
Documentation/riscv/hwprobe.rst +6 -0
index 933c715065d6..6a17c2872660 100644
@@ -77,6 +77,9 @@ The following keys are defined:
   * :c:macro:`RISCV_HWPROBE_EXT_ZBS`: The Zbs extension is supported, as defined
        in version 1.0 of the Bit-Manipulation ISA extensions.
 
+  * :c:macro:`RISCV_HWPROBE_EXT_ZICBOZ`: The Zicboz extension is supported, as
+       ratified in commit 3dd606f ("Create cmobase-v1.0.pdf") of riscv-CMOs.
+
 * :c:macro:`RISCV_HWPROBE_KEY_CPUPERF_0`: A bitmask that contains performance
   information about the selected set of processors.
 

@@ -97,3 +100,6 @@ The following keys are defined:
 
   * :c:macro:`RISCV_HWPROBE_MISALIGNED_UNSUPPORTED`: Misaligned accesses are
     not supported at all and will generate a misaligned address fault.
+
+* :c:macro:`RISCV_HWPROBE_KEY_ZICBOZ_BLOCK_SIZE`: An unsigned int which
+  represents the size of the Zicboz block in bytes.
arch/riscv/include/asm/hwprobe.h +1 -1
index 78936f4ff513..39df8604fea1 100644
@@ -8,6 +8,6 @@
 
 #include <uapi/asm/hwprobe.h>
 
-#define RISCV_HWPROBE_MAX_KEY 5
+#define RISCV_HWPROBE_MAX_KEY 6
 
 #endif
arch/riscv/include/uapi/asm/hwprobe.h +2 -0
index 006bfb48343d..86d08a0e617b 100644
@@ -29,6 +29,7 @@ struct riscv_hwprobe {
 #define		RISCV_HWPROBE_EXT_ZBA		(1 << 3)
 #define		RISCV_HWPROBE_EXT_ZBB		(1 << 4)
 #define		RISCV_HWPROBE_EXT_ZBS		(1 << 5)
+#define		RISCV_HWPROBE_EXT_ZICBOZ	(1 << 6)
 #define RISCV_HWPROBE_KEY_CPUPERF_0	5
 #define		RISCV_HWPROBE_MISALIGNED_UNKNOWN	(0 << 0)
 #define		RISCV_HWPROBE_MISALIGNED_EMULATED	(1 << 0)

@@ -36,6 +37,7 @@ struct riscv_hwprobe {
 #define		RISCV_HWPROBE_MISALIGNED_FAST		(3 << 0)
 #define		RISCV_HWPROBE_MISALIGNED_UNSUPPORTED	(4 << 0)
 #define		RISCV_HWPROBE_MISALIGNED_MASK		(7 << 0)
+#define RISCV_HWPROBE_KEY_ZICBOZ_BLOCK_SIZE	6
 /* Increase RISCV_HWPROBE_MAX_KEY when adding items. */
 
 #endif
arch/riscv/kernel/sys_riscv.c +0 -11
index 26ef5526bfb4..7d970358597b 100644
@@ -145,26 +145,33 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
 	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))

arch/riscv/kernel/sys_riscv.c static void hwprobe_isa_ext0(struct riscv_hwprobe *pair, · line 154

Quoted 10 lines
arch/riscv/kernel/sys_riscv.c +7 -3
-			pair->value |= RISCV_HWPROBE_EXT_ZBS;
-		else
-			missing |= RISCV_HWPROBE_EXT_ZBS;
+#define EXT_KEY(ext)									\
+	do {										\
+		if (__riscv_isa_extension_available(isainfo->isa, RISCV_ISA_EXT_##ext))	\
+			pair->value |= RISCV_HWPROBE_EXT_##ext;				\
+		else									\
+			missing |= RISCV_HWPROBE_EXT_##ext;				\
+	} while (false)

Would you mind adding a comment here that using this macro is only permitted for extensions that can be used by userspace regardless of kernel config options?

arch/riscv/kernel/sys_riscv.c static void hwprobe_isa_ext0(struct riscv_hwprobe *pair, · line 165

Quoted 10 lines
arch/riscv/kernel/sys_riscv.c +5 -0
+		EXT_KEY(ZBA);
+		EXT_KEY(ZBB);
+		EXT_KEY(ZBS);
+		EXT_KEY(ZICBOZ);
+#undef EXT_KEY
 	}
 
 	/* Now turn off reporting features if any CPU is missing it. */
 	pair->value &= ~missing;
 }

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

author
#15 lore raw

Re: [PATCH 3/6] RISC-V: hwprobe: Expose Zicboz extension and its block size

Quoted 91 lines · Conor Dooley
Quoted 90 lines · Andrew Jones

Expose Zicboz through hwprobe and also provide a key to extract its respective block size. Opportunistically add a macro and apply it to current extensions in order to avoid duplicating code.

Signed-off-by Andrew Jones <[email protected]>
---
 Documentation/riscv/hwprobe.rst       |  6 ++++
 arch/riscv/include/asm/hwprobe.h      |  2 +-
 arch/riscv/include/uapi/asm/hwprobe.h |  2 ++
 arch/riscv/kernel/sys_riscv.c         | 41 ++++++++++++++++++---------
 4 files changed, 36 insertions(+), 15 deletions(-)
Documentation/riscv/hwprobe.rst +6 -0
index 933c715065d6..6a17c2872660 100644
@@ -77,6 +77,9 @@ The following keys are defined:
   * :c:macro:`RISCV_HWPROBE_EXT_ZBS`: The Zbs extension is supported, as defined
        in version 1.0 of the Bit-Manipulation ISA extensions.
 
+  * :c:macro:`RISCV_HWPROBE_EXT_ZICBOZ`: The Zicboz extension is supported, as
+       ratified in commit 3dd606f ("Create cmobase-v1.0.pdf") of riscv-CMOs.
+
 * :c:macro:`RISCV_HWPROBE_KEY_CPUPERF_0`: A bitmask that contains performance
   information about the selected set of processors.
 

@@ -97,3 +100,6 @@ The following keys are defined:
 
   * :c:macro:`RISCV_HWPROBE_MISALIGNED_UNSUPPORTED`: Misaligned accesses are
     not supported at all and will generate a misaligned address fault.
+
+* :c:macro:`RISCV_HWPROBE_KEY_ZICBOZ_BLOCK_SIZE`: An unsigned int which
+  represents the size of the Zicboz block in bytes.
arch/riscv/include/asm/hwprobe.h +1 -1
index 78936f4ff513..39df8604fea1 100644
@@ -8,6 +8,6 @@
 
 #include <uapi/asm/hwprobe.h>
 
-#define RISCV_HWPROBE_MAX_KEY 5
+#define RISCV_HWPROBE_MAX_KEY 6
 
 #endif
arch/riscv/include/uapi/asm/hwprobe.h +2 -0
index 006bfb48343d..86d08a0e617b 100644
@@ -29,6 +29,7 @@ struct riscv_hwprobe {
 #define		RISCV_HWPROBE_EXT_ZBA		(1 << 3)
 #define		RISCV_HWPROBE_EXT_ZBB		(1 << 4)
 #define		RISCV_HWPROBE_EXT_ZBS		(1 << 5)
+#define		RISCV_HWPROBE_EXT_ZICBOZ	(1 << 6)
 #define RISCV_HWPROBE_KEY_CPUPERF_0	5
 #define		RISCV_HWPROBE_MISALIGNED_UNKNOWN	(0 << 0)
 #define		RISCV_HWPROBE_MISALIGNED_EMULATED	(1 << 0)

@@ -36,6 +37,7 @@ struct riscv_hwprobe {
 #define		RISCV_HWPROBE_MISALIGNED_FAST		(3 << 0)
 #define		RISCV_HWPROBE_MISALIGNED_UNSUPPORTED	(4 << 0)
 #define		RISCV_HWPROBE_MISALIGNED_MASK		(7 << 0)
+#define RISCV_HWPROBE_KEY_ZICBOZ_BLOCK_SIZE	6
 /* Increase RISCV_HWPROBE_MAX_KEY when adding items. */
 
 #endif
arch/riscv/kernel/sys_riscv.c +3 -14
index 26ef5526bfb4..7d970358597b 100644
@@ -145,26 +145,33 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
 	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;
+#define EXT_KEY(ext)									\
+	do {										\
+		if (__riscv_isa_extension_available(isainfo->isa, RISCV_ISA_EXT_##ext))	\
Quoted 8 lines

arch/riscv/kernel/sys_riscv.c static void hwprobe_isa_ext0(struct riscv_hwprobe *pair, · line 154

Quoted 4 lines
arch/riscv/kernel/sys_riscv.c +4 -0
+			pair->value |= RISCV_HWPROBE_EXT_##ext;				\
+		else									\
+			missing |= RISCV_HWPROBE_EXT_##ext;				\
+	} while (false)

Would you mind adding a comment here that using this macro is only permitted for extensions that can be used by userspace regardless of kernel config options?

I can do that. Then, as part of the "extension available" API rework, we can maybe drop the comment when we use the new call, which will be documented to say that as well.

Thanks,
drew

Quoted 11 lines

arch/riscv/kernel/sys_riscv.c static void hwprobe_isa_ext0(struct riscv_hwprobe *pair, · line 165

Quoted 11 lines
arch/riscv/kernel/sys_riscv.c +5 -0
+		EXT_KEY(ZBA);
+		EXT_KEY(ZBB);
+		EXT_KEY(ZBS);
+		EXT_KEY(ZICBOZ);
+#undef EXT_KEY
 	}
 
 	/* Now turn off reporting features if any CPU is missing it. */
 	pair->value &= ~missing;
 }

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

#16 lore raw

Re: [PATCH 3/6] RISC-V: hwprobe: Expose Zicboz extension and its block size

Quoted 15 lines · Andrew Jones
Quoted 13 lines · Conor Dooley

arch/riscv/kernel/sys_riscv.c static void hwprobe_isa_ext0(struct riscv_hwprobe *pair, · line 154

Quoted 7 lines · Andrew Jones
arch/riscv/kernel/sys_riscv.c +7 -0
+#define EXT_KEY(ext)									\
+	do {										\
+		if (__riscv_isa_extension_available(isainfo->isa, RISCV_ISA_EXT_##ext))	\
+			pair->value |= RISCV_HWPROBE_EXT_##ext;				\
+		else									\
+			missing |= RISCV_HWPROBE_EXT_##ext;				\
+	} while (false)

Would you mind adding a comment here that using this macro is only permitted for extensions that can be used by userspace regardless of kernel config options?

Quoted 3 lines

I can do that. Then, as part of the "extension available" API rework, we can maybe drop the comment when we use the new call, which will be documented to say that as well.

Yeah, exactly.

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

author
#17 lore raw

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

In reply to Andrew Jones

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

Signed-off-by Andrew Jones <[email protected]>
---
 tools/testing/selftests/riscv/hwprobe/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/testing/selftests/riscv/hwprobe/Makefile +1 -1
index ebdbb3c22e54..5f614c3ba598 100644
@@ -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) $^
Signature · 7 lines
2.41.0


_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
Reviewed-by on v1 4/6
#18 lore raw

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

Quoted 2 lines · Andrew Jones

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

As that's mainly what I used for quick testing of stuff, this seems helpful to me!

Reviewed-by Conor Dooley <[email protected]>

Thanks,
Conor.

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

author
#19 lore raw

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

In reply to 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 <[email protected]>
---
 .../testing/selftests/riscv/hwprobe/hwprobe.c | 54 +++++++------------
 1 file changed, 20 insertions(+), 34 deletions(-)
tools/testing/selftests/riscv/hwprobe/hwprobe.c +20 -34
index 09f290a67420..4f15f1f3b4c3 100644
@@ -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 count\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();
 }
Signature · 7 lines
2.41.0


_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
author
#20 lore raw

PATCH 6/6 RISC-V: selftests: Add CBO tests

In reply to Andrew Jones

Add hwprobe test for Zicboz and its block size. Also, when Zicboz is present, test that cbo.zero may be issued and works. Additionally test that the Zicbom instructions cause SIGILL and also that cbo.zero causes SIGILL when Zicboz is not present. Pinning the test to a subset of cpus with taskset will also restrict the hwprobe calls to that set.

Signed-off-by Andrew Jones <[email protected]>
---
 tools/testing/selftests/riscv/Makefile        |   2 +-
 .../testing/selftests/riscv/hwprobe/Makefile  |   5 +-
 tools/testing/selftests/riscv/hwprobe/cbo.c   | 160 ++++++++++++++++++
 .../testing/selftests/riscv/hwprobe/hwprobe.c |  12 +-
 .../testing/selftests/riscv/hwprobe/hwprobe.h |  15 ++
 5 files changed, 181 insertions(+), 13 deletions(-)
 create mode 100644 tools/testing/selftests/riscv/hwprobe/cbo.c
 create mode 100644 tools/testing/selftests/riscv/hwprobe/hwprobe.h
tools/testing/selftests/riscv/Makefile +1 -1
index f4b3d5c9af5b..7bbeec48292d 100644
@@ -16,7 +16,7 @@ CFLAGS := -Wall -O2 -g
 top_srcdir = $(realpath ../../../../)
 
 # Additional include paths needed by kselftest.h and local headers
-CFLAGS += -I$(top_srcdir)/tools/testing/selftests/
+CFLAGS += -I$(top_srcdir)/tools/testing/selftests/ -I$(top_srcdir)/tools/include
 
 CFLAGS += $(KHDR_INCLUDES)
tools/testing/selftests/riscv/hwprobe/Makefile +4 -1
index 5f614c3ba598..56e54d66df7e 100644
@@ -2,9 +2,12 @@
 # Copyright (C) 2021 ARM Limited
 # Originally tools/testing/arm64/abi/Makefile
 
-TEST_GEN_PROGS := hwprobe
+TEST_GEN_PROGS := hwprobe cbo
 
 include ../../lib.mk
 
 $(OUTPUT)/hwprobe: hwprobe.c sys_hwprobe.S
 	$(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^
+
+$(OUTPUT)/cbo: cbo.c sys_hwprobe.S
+	$(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^
tools/testing/selftests/riscv/hwprobe/cbo.c new file +160 -0

160 lines not on this page · raw message

tools/testing/selftests/riscv/hwprobe/hwprobe.c +1 -11
index 4f15f1f3b4c3..c474891df307 100644
@@ -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];
tools/testing/selftests/riscv/hwprobe/hwprobe.h new file +15 -0
new file mode 100644
index 000000000000..721b0ce73a56
@@ -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 cpu_count, unsigned long *cpus, unsigned int flags);
+
+#endif
Signature · 7 lines
2.41.0


_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
CI Applied
#21 lore raw
In reply to Andrew Jones
This series was applied to riscv/linux.git (for-next) by Palmer Dabbelt <[email protected]>:

Hello:

This series was applied to riscv/linux.git (for-next) by Palmer Dabbelt <[email protected]>:

Quoted 9 lines · On Wed, 9 Aug 2023 13:55:17 +0200 you wrote:

In order for usermode to issue cbo.zero, it needs privilege granted to issue the extension instruction (patch 2) and to know that the extension is available and its block size (patch 3). Patch 1 could be separate from this series (it just fixes up some error messages), patches 4-5 convert the hwprobe selftest to a statically-linked, TAP test and patch 6 adds a new hwprobe test for the new information as well as testing CBO instructions can or cannot be issued as appropriate.

[...]

Here is the summary with links:
- [1/6] RISC-V: Make zicbom/zicboz errors consistent

    https://git.kernel.org/riscv/c/66e02b1d6430
  - [2/6] RISC-V: Enable cbo.zero in usermode
    https://git.kernel.org/riscv/c/090925374690
  - [3/6] RISC-V: hwprobe: Expose Zicboz extension and its block size
    https://git.kernel.org/riscv/c/d45243f8c239
  - [4/6] RISC-V: selftests: Statically link hwprobe test
    https://git.kernel.org/riscv/c/0ca410a80172
  - [5/6] RISC-V: selftests: Convert hwprobe test to kselftest API
    https://git.kernel.org/riscv/c/d79a646354a9
  - [6/6] RISC-V: selftests: Add CBO tests
    https://git.kernel.org/riscv/c/026645838c86

You are awesome, thank you!

Signature · 9 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
author
#22 lore raw
Quoted 30 lines · On Wed, Aug 30, 2023 at 01:20:34PM +0000, [email protected]…

Hello:

This series was applied to riscv/linux.git (for-next) by Palmer Dabbelt <[email protected]>:

Quoted 9 lines · On Wed, 9 Aug 2023 13:55:17 +0200 you wrote:

In order for usermode to issue cbo.zero, it needs privilege granted to issue the extension instruction (patch 2) and to know that the extension is available and its block size (patch 3). Patch 1 could be separate from this series (it just fixes up some error messages), patches 4-5 convert the hwprobe selftest to a statically-linked, TAP test and patch 6 adds a new hwprobe test for the new information as well as testing CBO instructions can or cannot be issued as appropriate.

[...]

Here is the summary with links:
- [1/6] RISC-V: Make zicbom/zicboz errors consistent

    https://git.kernel.org/riscv/c/66e02b1d6430
  - [2/6] RISC-V: Enable cbo.zero in usermode
    https://git.kernel.org/riscv/c/090925374690
  - [3/6] RISC-V: hwprobe: Expose Zicboz extension and its block size
    https://git.kernel.org/riscv/c/d45243f8c239
  - [4/6] RISC-V: selftests: Statically link hwprobe test
    https://git.kernel.org/riscv/c/0ca410a80172
  - [5/6] RISC-V: selftests: Convert hwprobe test to kselftest API
    https://git.kernel.org/riscv/c/d79a646354a9
  - [6/6] RISC-V: selftests: Add CBO tests
    https://git.kernel.org/riscv/c/026645838c86
Quoted 6 lines

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Hi Palmer,

This series needs a v2. There were couple minor comments on v1, and I just found out that I broke the compiling of the nolibc vector selftest.

I'll post v2 in the next few minutes. Sorry for the hassle!

Thanks,
drew

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv