RISC-V: Apply Zicboz to clear_page

PATCH Superseded Andrew Jones 5 participants 14 messages Jan 30, 2023 – Feb 2, 2023

Subsystems (3)

Device tree bindings RISC-V KVM RISC-V

lore

Share & tools

lore

Andrew Jones

author

2023-01-30 12:01 UTC #0 lore raw

v3 cover RISC-V: Apply Zicboz to clear_page

When the Zicboz extension is available we can more rapidly zero naturally aligned Zicboz block sized chunks of memory. As pages are always page aligned and are larger than any Zicboz block size will be, then clear_page() appears to be a good candidate for the extension. While cycle count and energy consumption should also be considered, we can be pretty certain that implementing clear_page() with the Zicboz extension is a win by comparing the new dynamic instruction count with its current count[1]. Doing so we see that the new count is just over a quarter of the old count (see patch4's commit message for more details).

For those of you who reviewed v1[2], you may be looking for the memset() patches. As pointed out in v1, and a couple follow-up emails, it's not clear that patching memset() is a win yet. When I get a chance to test on real hardware with a comprehensive benchmark collection then I can post the memset() patches separately (assuming the benchmarks show it's worthwhile).

Dependencies:
- "[PATCH v5 00/13] riscv: improve boot time isa extensions handling"

    https://lore.kernel.org/all/20230128172856.3814-1-jszhang at kernel.org/
  - "[PATCH v1 0/3] Remove toolchain dependencies for Zicbom"
    https://lore.kernel.org/all/20230108163356.3063839-1-conor at kernel.org/

The patches are also available here
https://github.com/jones-drew/linux/commits/riscv/zicboz-v3

To test over QEMU this branch may be used to enable Zicboz
https://gitlab.com/jones-drew/qemu/-/commits/riscv/zicboz

To test running a KVM guest with Zicboz this kvmtool branch may be used
https://github.com/jones-drew/kvmtool/commits/riscv/zicboz

Thanks,
drew

[1] I ported the functions under test to userspace and linked them with a test program. Then, I ran them under gdb with a script[3] which counted instructions by single stepping.
[2] https://lore.kernel.org/all/20221027130247.31634-1-ajones at ventanamicro.com/
[3] https://gist.github.com/jones-drew/487791c956ceca8c18adc2847eec9c60

v3:
- CC'ed DT list
- Improved commit message of DT bindings patch to point out relationship with cbom-block-size
- Picked up an a-b from Conor

v2:
- s/blksz/block_size/, improved commit message for "RISC-V: Add Zicboz detection and block size parsing", isa ext sorting [Conor]
- Added dt binding patch [Heiko]
- Picked up r-b's from Conor, Heiko, and Anup
- Moved config symbol and CBO_zero() introduction to "RISC-V: Use Zicboz in clear_page when available" and improved its commit message and implementation (unrolled four times) [drew]
- Dropped memset() patches [drew]
- Rebased on ae4d39f75308 ("Merge patch "RISC-V: fix incorrect type of ARCH_CANAAN_K210_DTB_SOURCE"") plus the dependencies

Andrew Jones (6):
RISC-V: Factor out body of riscv_init_cbom_blocksize loop
dt-bindings: riscv: Document cboz-block-size
RISC-V: Add Zicboz detection and block size parsing
RISC-V: Use Zicboz in clear_page when available
RISC-V: KVM: Provide UAPI for Zicboz block size
RISC-V: KVM: Expose Zicboz to the guest

 .../devicetree/bindings/riscv/cpus.yaml       |  5 ++
 arch/riscv/Kconfig                            | 13 ++++
 arch/riscv/include/asm/cacheflush.h           |  3 +-
 arch/riscv/include/asm/hwcap.h                |  1 +
 arch/riscv/include/asm/insn-def.h             |  4 ++
 arch/riscv/include/asm/page.h                 |  6 +-
 arch/riscv/include/uapi/asm/kvm.h             |  2 +
 arch/riscv/kernel/cpu.c                       |  1 +
 arch/riscv/kernel/cpufeature.c                | 10 +++
 arch/riscv/kernel/setup.c                     |  2 +-
 arch/riscv/kvm/vcpu.c                         | 11 ++++
 arch/riscv/lib/Makefile                       |  1 +
 arch/riscv/lib/clear_page.S                   | 36 +++++++++++
 arch/riscv/mm/cacheflush.c                    | 64 +++++++++++--------
 14 files changed, 130 insertions(+), 29 deletions(-)
 create mode 100644 arch/riscv/lib/clear_page.S

Signature · 1 line

2.39.1

Andrew Jones

author

2023-01-30 12:01 UTC #1 lore raw

PATCH v3 1/6 RISC-V: Factor out body of riscv_init_cbom_blocksize loop

Refactor riscv_init_cbom_blocksize() to prepare for it to be used for both cbom block size and cboz block size.

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

Reviewed-by Heiko Stuebner <[email protected]>

Reviewed-by Conor Dooley <[email protected]>

---
 arch/riscv/mm/cacheflush.c | 45 +++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 20 deletions(-)

arch/riscv/mm/cacheflush.c +25 ~~-20~~

index 3cc07ed45aeb..eaf23fc14966 100644
@@ -98,34 +98,39 @@ void flush_icache_pte(pte_t pte)
 unsigned int riscv_cbom_block_size;
 EXPORT_SYMBOL_GPL(riscv_cbom_block_size);
 
+static void cbo_get_block_size(struct device_node *node,
+			       const char *name, u32 *block_size,
+			       unsigned long *first_hartid)
+{
+	unsigned long hartid;
+	u32 val;
+
+	if (riscv_of_processor_hartid(node, &hartid))
+		return;
+
+	if (of_property_read_u32(node, name, &val))
+		return;
+
+	if (!*block_size) {
+		*block_size = val;
+		*first_hartid = hartid;
+	} else if (*block_size != val) {
+		pr_warn("%s mismatched between harts %lu and %lu\n",
+			name, *first_hartid, hartid);
+	}
+}
+
 void riscv_init_cbom_blocksize(void)
 {
 	struct device_node *node;
 	unsigned long cbom_hartid;
-	u32 val, probed_block_size;
-	int ret;
+	u32 probed_block_size;
 
 	probed_block_size = 0;
 	for_each_of_cpu_node(node) {
-		unsigned long hartid;
-
-		ret = riscv_of_processor_hartid(node, &hartid);
-		if (ret)
-			continue;
-
 		/* set block-size for cbom extension if available */
-		ret = of_property_read_u32(node, "riscv,cbom-block-size", &val);
-		if (ret)
-			continue;
-
-		if (!probed_block_size) {
-			probed_block_size = val;
-			cbom_hartid = hartid;
-		} else {
-			if (probed_block_size != val)
-				pr_warn("cbom-block-size mismatched between harts %lu and %lu\n",
-					cbom_hartid, hartid);
-		}
+		cbo_get_block_size(node, "riscv,cbom-block-size",
+				   &probed_block_size, &cbom_hartid);
 	}
 
 	if (probed_block_size)

Signature · 1 line

2.39.1

Andrew Jones

author

2023-01-30 12:01 UTC #2 lore raw

PATCH v3 2/6 dt-bindings: riscv: Document cboz-block-size

In reply to Andrew Jones

The Zicboz operation (cbo.zero) operates on a block-size defined for the cpu-core. While we already have the riscv,cbom-block-size property, it only provides the block size for Zicbom operations. Even though it's likely Zicboz and Zicbom will use the same size, that's not specified. Create another property specifically for Zicboz.

Cc Rob Herring <[email protected]>

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

---
 Documentation/devicetree/bindings/riscv/cpus.yaml | 5 +++++
 1 file changed, 5 insertions(+)

Documentation/devicetree/bindings/riscv/cpus.yaml +5 -0

index c6720764e765..f4ee70f8e1cf 100644
@@ -72,6 +72,11 @@ properties:
     description:
       The blocksize in bytes for the Zicbom cache operations.
 
+  riscv,cboz-block-size:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      The blocksize in bytes for the Zicboz cache operations.
+
   riscv,isa:
     description:
       Identifies the specific RISC-V instruction set architecture

Signature · 1 line

2.39.1

Andrew Jones

author

2023-01-30 12:01 UTC #5 lore raw

PATCH v3 3/6 RISC-V: Add Zicboz detection and block size parsing

In reply to Andrew Jones

Parse "riscv,cboz-block-size" from the DT by piggybacking on Zicbom's riscv_init_cbom_blocksize(). Additionally check the DT for the presence of the "zicboz" extension and, when it's present, validate the parsed cboz block size as we do Zicbom's cbom block size with riscv_isa_extension_check().

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

Reviewed-by Heiko Stuebner <[email protected]>

Reviewed-by Conor Dooley <[email protected]>

---
 arch/riscv/include/asm/cacheflush.h |  3 ++-
 arch/riscv/include/asm/hwcap.h      |  1 +
 arch/riscv/kernel/cpu.c             |  1 +
 arch/riscv/kernel/cpufeature.c      | 10 ++++++++++
 arch/riscv/kernel/setup.c           |  2 +-
 arch/riscv/mm/cacheflush.c          | 23 +++++++++++++++--------
 6 files changed, 30 insertions(+), 10 deletions(-)

arch/riscv/include/asm/cacheflush.h +2 -1

index 03e3b95ae6da..8091b8bf4883 100644
@@ -50,7 +50,8 @@ void flush_icache_mm(struct mm_struct *mm, bool local);
 #endif /* CONFIG_SMP */
 
 extern unsigned int riscv_cbom_block_size;
-void riscv_init_cbom_blocksize(void);
+extern unsigned int riscv_cboz_block_size;
+void riscv_init_cbo_blocksizes(void);
 
 #ifdef CONFIG_RISCV_DMA_NONCOHERENT
 void riscv_noncoherent_supported(void);

arch/riscv/include/asm/hwcap.h +1 -0

index 7936ae6f7bdf..7d1e88d7320d 100644
@@ -46,6 +46,7 @@
 #define RISCV_ISA_EXT_SVPBMT		29
 #define RISCV_ISA_EXT_ZICBOM		30
 #define RISCV_ISA_EXT_ZIHINTPAUSE	31
+#define RISCV_ISA_EXT_ZICBOZ		32
 
 #ifndef __ASSEMBLY__
 #include <linux/jump_label.h>

arch/riscv/kernel/cpu.c +1 -0

index 0bf1c7f663fc..578c1093b839 100644
@@ -186,6 +186,7 @@ arch_initcall(riscv_cpuinfo_init);
  */
 static struct riscv_isa_ext_data isa_ext_arr[] = {
 	__RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM),
+	__RISCV_ISA_EXT_DATA(zicboz, RISCV_ISA_EXT_ZICBOZ),
 	__RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
 	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
 	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),

arch/riscv/kernel/cpufeature.c +10 -0

index a4f737bc7530..5d278d2c48fb 100644
@@ -78,6 +78,15 @@ static bool riscv_isa_extension_check(int id)
 			return false;
 		}
 		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");
+			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");
+			return false;
+		}
+		return true;
 	}
 
 	return true;

@@ -225,6 +234,7 @@ void __init riscv_fill_hwcap(void)
 				SET_ISA_EXT_MAP("svinval", RISCV_ISA_EXT_SVINVAL);
 				SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
 				SET_ISA_EXT_MAP("zicbom", RISCV_ISA_EXT_ZICBOM);
+				SET_ISA_EXT_MAP("zicboz", RISCV_ISA_EXT_ZICBOZ);
 				SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
 			}
 #undef SET_ISA_EXT_MAP

arch/riscv/kernel/setup.c +1 -1

index 376d2827e736..5d3184cbf518 100644
@@ -297,7 +297,7 @@ void __init setup_arch(char **cmdline_p)
 	setup_smp();
 #endif
 
-	riscv_init_cbom_blocksize();
+	riscv_init_cbo_blocksizes();
 	riscv_fill_hwcap();
 	apply_boot_alternatives();
 	if (IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM) &&

arch/riscv/mm/cacheflush.c +15 -8

index eaf23fc14966..ba4832bb949b 100644
@@ -98,6 +98,9 @@ void flush_icache_pte(pte_t pte)
 unsigned int riscv_cbom_block_size;
 EXPORT_SYMBOL_GPL(riscv_cbom_block_size);
 
+unsigned int riscv_cboz_block_size;
+EXPORT_SYMBOL_GPL(riscv_cboz_block_size);
+
 static void cbo_get_block_size(struct device_node *node,
 			       const char *name, u32 *block_size,
 			       unsigned long *first_hartid)

@@ -120,19 +123,23 @@ static void cbo_get_block_size(struct device_node *node,
 	}
 }
 
-void riscv_init_cbom_blocksize(void)
+void riscv_init_cbo_blocksizes(void)
 {
+	unsigned long cbom_hartid, cboz_hartid;
+	u32 cbom_block_size = 0, cboz_block_size = 0;
 	struct device_node *node;
-	unsigned long cbom_hartid;
-	u32 probed_block_size;
 
-	probed_block_size = 0;
 	for_each_of_cpu_node(node) {
-		/* set block-size for cbom extension if available */
+		/* set block-size for cbom and/or cboz extension if available */
 		cbo_get_block_size(node, "riscv,cbom-block-size",
-				   &probed_block_size, &cbom_hartid);
+				   &cbom_block_size, &cbom_hartid);
+		cbo_get_block_size(node, "riscv,cboz-block-size",
+				   &cboz_block_size, &cboz_hartid);
 	}
 
-	if (probed_block_size)
-		riscv_cbom_block_size = probed_block_size;
+	if (cbom_block_size)
+		riscv_cbom_block_size = cbom_block_size;
+
+	if (cboz_block_size)
+		riscv_cboz_block_size = cboz_block_size;
 }

Signature · 1 line

2.39.1

Andrew Jones

author

2023-01-30 12:01 UTC #6 lore raw

PATCH v3 4/6 RISC-V: Use Zicboz in clear_page when available

In reply to Andrew Jones

Using memset() to zero a 4K page takes 563 total instructions where 20 are branches. clear_page() with Zicboz takes 150 total instructions where 16 are branches. We could reduce the numbers by further unrolling, but, since the cboz block size isn't fixed, we'd need a Duff device to ensure we don't execute too many unrolled steps. Also, cbo.zero doesn't take an offset, so each unrolled step requires it and an add instruction. This increases the chance for icache misses if we unroll many times. For these reasons we only unroll four times. Unrolling four times should be safe as it supports cboz block sizes up to 1K when used with 4K pages and it's only 24 to 32 bytes of unrolled instructions.

Another note about the Duff device idea is that it would probably be best to store the number of steps needed at boot time and then load the value in clear_page(). Calculating it in clear_page(), particularly without the Zbb extension, would not be efficient.

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

Acked-by Conor Dooley <[email protected]>

---
 arch/riscv/Kconfig                | 13 +++++++++++
 arch/riscv/include/asm/insn-def.h |  4 ++++
 arch/riscv/include/asm/page.h     |  6 +++++-
 arch/riscv/lib/Makefile           |  1 +
 arch/riscv/lib/clear_page.S       | 36 +++++++++++++++++++++++++++++++
 5 files changed, 59 insertions(+), 1 deletion(-)
 create mode 100644 arch/riscv/lib/clear_page.S

arch/riscv/Kconfig +13 -0

index 33bbdc33cef8..3759a2f6edd5 100644
@@ -432,6 +432,19 @@ config RISCV_ISA_ZICBOM
 
 	   If you don't know what to do here, say Y.
 
+config RISCV_ISA_ZICBOZ
+	bool "Zicboz extension support for faster zeroing of memory"
+	depends on !XIP_KERNEL && MMU
+	select RISCV_ALTERNATIVE
+	default y
+	help
+	   Enable the use of the ZICBOZ extension (cbo.zero instruction)
+	   when available.
+
+	   The Zicboz extension is used for faster zeroing of memory.
+
+	   If you don't know what to do here, say Y.
+
 config TOOLCHAIN_HAS_ZIHINTPAUSE
 	bool
 	default y

arch/riscv/include/asm/insn-def.h +4 -0

index e01ab51f50d2..6960beb75f32 100644
@@ -192,4 +192,8 @@
 	INSN_I(OPCODE_MISC_MEM, FUNC3(2), __RD(0),		\
 	       RS1(base), SIMM12(2))
 
+#define CBO_zero(base)						\
+	INSN_I(OPCODE_MISC_MEM, FUNC3(2), __RD(0),		\
+	       RS1(base), SIMM12(4))
+
 #endif /* __ASM_INSN_DEF_H */

arch/riscv/include/asm/page.h +5 -1

index 9f432c1b5289..ccd168fe29d2 100644
@@ -49,10 +49,14 @@
 
 #ifndef __ASSEMBLY__
 
+#ifdef CONFIG_RISCV_ISA_ZICBOZ
+void clear_page(void *page);
+#else
 #define clear_page(pgaddr)			memset((pgaddr), 0, PAGE_SIZE)
+#endif
 #define copy_page(to, from)			memcpy((to), (from), PAGE_SIZE)
 
-#define clear_user_page(pgaddr, vaddr, page)	memset((pgaddr), 0, PAGE_SIZE)
+#define clear_user_page(pgaddr, vaddr, page)	clear_page(pgaddr)
 #define copy_user_page(vto, vfrom, vaddr, topg) \
 			memcpy((vto), (vfrom), PAGE_SIZE)

arch/riscv/lib/Makefile +1 -0

index 25d5c9664e57..9ee5e2ab5143 100644
@@ -5,5 +5,6 @@ lib-y			+= memset.o
 lib-y			+= memmove.o
 lib-$(CONFIG_MMU)	+= uaccess.o
 lib-$(CONFIG_64BIT)	+= tishift.o
+lib-$(CONFIG_RISCV_ISA_ZICBOZ)	+= clear_page.o
 
 obj-$(CONFIG_FUNCTION_ERROR_INJECTION) += error-inject.o

arch/riscv/lib/clear_page.S new file +36 -0

new file mode 100644
index 000000000000..49f29139a5b6
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2023 Ventana Micro Systems Inc.
+ */
+
+#include <linux/linkage.h>
+#include <asm/asm.h>
+#include <asm/alternative-macros.h>
+#include <asm/hwcap.h>
+#include <asm/insn-def.h>
+#include <asm/page.h>
+
+/* void clear_page(void *page) */
+ENTRY(__clear_page)
+WEAK(clear_page)
+	li	a2, PAGE_SIZE
+	ALTERNATIVE("j .Lno_zicboz", "nop",
+		    0, RISCV_ISA_EXT_ZICBOZ, CONFIG_RISCV_ISA_ZICBOZ)
+	la	a1, riscv_cboz_block_size
+	lw	a1, 0(a1)
+	add	a2, a0, a2
+.Lzero_loop:
+	CBO_zero(a0)
+	add	a0, a0, a1
+	CBO_zero(a0)
+	add	a0, a0, a1
+	CBO_zero(a0)
+	add	a0, a0, a1
+	CBO_zero(a0)
+	add	a0, a0, a1
+	bltu	a0, a2, .Lzero_loop
+	ret
+.Lno_zicboz:
+	li	a1, 0
+	tail	__memset
+END(__clear_page)

Signature · 1 line

2.39.1

Andrew Jones

author

2023-01-30 12:01 UTC #9 lore raw

PATCH v3 5/6 RISC-V: KVM: Provide UAPI for Zicboz block size

In reply to Andrew Jones

We're about to allow guests to use the Zicboz extension. KVM userspace needs to know the cache block size in order to properly advertise it to the guest. Provide a virtual config register for userspace to get it with the GET_ONE_REG API, but setting it cannot be supported, so disallow SET_ONE_REG.

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

Reviewed-by Conor Dooley <[email protected]>

Reviewed-by Anup Patel <[email protected]>

---
 arch/riscv/include/uapi/asm/kvm.h | 1 +
 arch/riscv/kvm/vcpu.c             | 7 +++++++
 2 files changed, 8 insertions(+)

arch/riscv/include/uapi/asm/kvm.h +1 -0

index 92af6f3f057c..c1a1bb0fa91c 100644
@@ -52,6 +52,7 @@ struct kvm_riscv_config {
 	unsigned long mvendorid;
 	unsigned long marchid;
 	unsigned long mimpid;
+	unsigned long zicboz_block_size;
 };
 
 /* CORE registers for KVM_GET_ONE_REG and KVM_SET_ONE_REG */

arch/riscv/kvm/vcpu.c +7 -0

index 7c08567097f0..e5126cefbc87 100644
@@ -276,6 +276,11 @@ static int kvm_riscv_vcpu_get_reg_config(struct kvm_vcpu *vcpu,
 			return -EINVAL;
 		reg_val = riscv_cbom_block_size;
 		break;
+	case KVM_REG_RISCV_CONFIG_REG(zicboz_block_size):
+		if (!riscv_isa_extension_available(vcpu->arch.isa, ZICBOZ))
+			return -EINVAL;
+		reg_val = riscv_cboz_block_size;
+		break;
 	case KVM_REG_RISCV_CONFIG_REG(mvendorid):
 		reg_val = vcpu->arch.mvendorid;
 		break;

@@ -347,6 +352,8 @@ static int kvm_riscv_vcpu_set_reg_config(struct kvm_vcpu *vcpu,
 		break;
 	case KVM_REG_RISCV_CONFIG_REG(zicbom_block_size):
 		return -EOPNOTSUPP;
+	case KVM_REG_RISCV_CONFIG_REG(zicboz_block_size):
+		return -EOPNOTSUPP;
 	case KVM_REG_RISCV_CONFIG_REG(mvendorid):
 		if (!vcpu->arch.ran_atleast_once)
 			vcpu->arch.mvendorid = reg_val;

Signature · 1 line

2.39.1

Andrew Jones

author

2023-01-30 12:01 UTC #10 lore raw

PATCH v3 6/6 RISC-V: KVM: Expose Zicboz to the guest

In reply to Andrew Jones

Guests may use the cbo.zero instruction when the CPU has the Zicboz extension and the hypervisor sets henvcfg.CBZE.

Add Zicboz support for KVM guests which may be enabled and disabled from KVM userspace using the ISA extension ONE_REG API.

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

Reviewed-by Conor Dooley <[email protected]>

Reviewed-by Anup Patel <[email protected]>

---
 arch/riscv/include/uapi/asm/kvm.h | 1 +
 arch/riscv/kvm/vcpu.c             | 4 ++++
 2 files changed, 5 insertions(+)

arch/riscv/include/uapi/asm/kvm.h +1 -0

index c1a1bb0fa91c..e44c1e90eaa7 100644
@@ -106,6 +106,7 @@ enum KVM_RISCV_ISA_EXT_ID {
 	KVM_RISCV_ISA_EXT_SVINVAL,
 	KVM_RISCV_ISA_EXT_ZIHINTPAUSE,
 	KVM_RISCV_ISA_EXT_ZICBOM,
+	KVM_RISCV_ISA_EXT_ZICBOZ,
 	KVM_RISCV_ISA_EXT_MAX,
 };

arch/riscv/kvm/vcpu.c +4 -0

index e5126cefbc87..198ee86cad38 100644
@@ -63,6 +63,7 @@ static const unsigned long kvm_isa_ext_arr[] = {
 	KVM_ISA_EXT_ARR(SVPBMT),
 	KVM_ISA_EXT_ARR(ZIHINTPAUSE),
 	KVM_ISA_EXT_ARR(ZICBOM),
+	KVM_ISA_EXT_ARR(ZICBOZ),
 };
 
 static unsigned long kvm_riscv_vcpu_base2isa_ext(unsigned long base_ext)

@@ -865,6 +866,9 @@ static void kvm_riscv_vcpu_update_config(const unsigned long *isa)
 	if (riscv_isa_extension_available(isa, ZICBOM))
 		henvcfg |= (ENVCFG_CBIE | ENVCFG_CBCFE);
 
+	if (riscv_isa_extension_available(isa, ZICBOZ))
+		henvcfg |= ENVCFG_CBZE;
+
 	csr_write(CSR_HENVCFG, henvcfg);
 #ifdef CONFIG_32BIT
 	csr_write(CSR_HENVCFGH, henvcfg >> 32);

Signature · 1 line

2.39.1

Conor Dooley

2023-01-30 12:25 UTC #3 lore raw

PATCH v3 2/6 dt-bindings: riscv: Document cboz-block-size

In reply to Andrew Jones

Hey!

Quoted 5 lines · Andrew Jones

The Zicboz operation (cbo.zero) operates on a block-size defined for the cpu-core. While we already have the riscv,cbom-block-size property, it only provides the block size for Zicbom operations. Even though it's likely Zicboz and Zicbom will use the same size, that's not specified.

If you end up respinning for some other reason, perhaps:
s/that's not specified/that's not required by the specification/ or some other wording to that effect?
I'm assuming by "specified" you're referring to the cmobase spec, but that wording I don't think is particular clear.

Quoted 14 lines

Create another property specifically for
Zicboz.
Cc Rob Herring <[email protected]>
Signed-off-by Andrew Jones <[email protected]>
---
 Documentation/devicetree/bindings/riscv/cpus.yaml | 5 +++++
 1 file changed, 5 insertions(+)
Documentation/devicetree/bindings/riscv/cpus.yaml +0 -0
index c6720764e765..f4ee70f8e1cf 100644

Documentation/devicetree/bindings/riscv/cpus.yaml properties: · line 79

Quoted 8 lines

Documentation/devicetree/bindings/riscv/cpus.yaml +5 -0

@@ -72,6 +72,11 @@ properties:
     description:
       The blocksize in bytes for the Zicbom cache operations.
 
+  riscv,cboz-block-size:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      The blocksize in bytes for the Zicboz cache operations.
+

I was happy with either keeping them apart entirely or a having this one default to the value of cbom-block-size, so:

Reviewed-by Conor Dooley <[email protected]>

Thanks,
Conor.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/kvm-riscv/attachments/20230130/7b887020/attachment-0001.sig>

Jeff Law

2023-01-30 18:31 UTC #11 lore raw

In reply to Andrew Jones

Quoted 10 lines · Andrew Jones

When the Zicboz extension is available we can more rapidly zero naturally aligned Zicboz block sized chunks of memory. As pages are always page aligned and are larger than any Zicboz block size will be, then clear_page() appears to be a good candidate for the extension. While cycle count and energy consumption should also be considered, we can be pretty certain that implementing clear_page() with the Zicboz extension is a win by comparing the new dynamic instruction count with its current count[1]. Doing so we see that the new count is just over a quarter of the old count (see patch4's commit message for more details).

Quoted 6 lines

For those of you who reviewed v1[2], you may be looking for the memset() patches. As pointed out in v1, and a couple follow-up emails, it's not clear that patching memset() is a win yet. When I get a chance to test on real hardware with a comprehensive benchmark collection then I can post the memset() patches separately (assuming the benchmarks show it's worthwhile).

So a note. On the userspace side we are using cboz for clearing memory in memset. While the data is intermixed with other changes, there's a very significant drop in stores and a host of related low level performance counters and a notable uptick in gcc #5 performance from spec2017 which is particularly sensitive to memory clearing. We haven't seen any performance regressions attributable to using cboz across spec2017's integer suite.

I believe our current threshold setting is to use cboz for chunks >= 128 bytes.

Jeff

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

Andrew Jones

author

2023-01-30 18:47 UTC #12 lore raw

Quoted 26 lines · Jeff Law

Quoted 10 lines · Andrew Jones
When the Zicboz extension is available we can more rapidly zero naturally aligned Zicboz block sized chunks of memory. As pages are always page aligned and are larger than any Zicboz block size will be, then clear_page() appears to be a good candidate for the extension. While cycle count and energy consumption should also be considered, we can be pretty certain that implementing clear_page() with the Zicboz extension is a win by comparing the new dynamic instruction count with its current count[1]. Doing so we see that the new count is just over a quarter of the old count (see patch4's commit message for more details).
Quoted 6 lines
For those of you who reviewed v1[2], you may be looking for the memset() patches. As pointed out in v1, and a couple follow-up emails, it's not clear that patching memset() is a win yet. When I get a chance to test on real hardware with a comprehensive benchmark collection then I can post the memset() patches separately (assuming the benchmarks show it's worthwhile).
So a note. On the userspace side we are using cboz for clearing memory in memset. While the data is intermixed with other changes, there's a very significant drop in stores and a host of related low level performance counters and a notable uptick in gcc #5 performance from spec2017 which is particularly sensitive to memory clearing. We haven't seen any performance regressions attributable to using cboz across spec2017's integer suite.

Quoted 2 lines

I believe our current threshold setting is to use cboz for chunks >= 128 bytes.

Thanks, Jeff! That's an encouraging report. I'll keep focused on clear_page() with this series, but once I can get some numbers with the memset patch, then I'll be happy to repost that as well.

Thanks,
drew

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

Jeff Law

2023-01-30 18:55 UTC #13 lore raw

v3 cover RISC-V: Apply Zicboz to clear_page

In reply to Andrew Jones

[ Sorry for the duplicate. Andrew indicated I'd used reply-list rather than reply-all. ]

Quoted 10 lines · Andrew Jones

When the Zicboz extension is available we can more rapidly zero naturally aligned Zicboz block sized chunks of memory. As pages are always page aligned and are larger than any Zicboz block size will be, then clear_page() appears to be a good candidate for the extension. While cycle count and energy consumption should also be considered, we can be pretty certain that implementing clear_page() with the Zicboz extension is a win by comparing the new dynamic instruction count with its current count[1]. Doing so we see that the new count is just over a quarter of the old count (see patch4's commit message for more details).

Quoted 7 lines

For those of you who reviewed v1[2], you may be looking for the memset() patches. As pointed out in v1, and a couple follow-up emails, it's not clear that patching memset() is a win yet. When I get a chance to test on real hardware with a comprehensive benchmark collection then I can post the memset() patches separately (assuming the benchmarks show it's worthwhile).

I believe our current threshold setting is to use cboz for chunks >= 128 bytes.

Jeff

Rob Herring

maintainer

2023-01-30 22:57 UTC #4 lore raw

PATCH v3 2/6 dt-bindings: riscv: Document cboz-block-size

In reply to Andrew Jones

Quoted 7 lines · Andrew Jones

The Zicboz operation (cbo.zero) operates on a block-size defined for the cpu-core. While we already have the riscv,cbom-block-size property, it only provides the block size for Zicbom operations. Even though it's likely Zicboz and Zicbom will use the same size, that's not specified. Create another property specifically for Zicboz.

Quoted 6 lines

Cc Rob Herring <[email protected]>
Signed-off-by Andrew Jones <[email protected]>
---
 Documentation/devicetree/bindings/riscv/cpus.yaml | 5 +++++
 1 file changed, 5 insertions(+)

Reviewed-by Rob Herring <[email protected]>

Palmer Dabbelt

maintainer

2023-02-02 04:35 UTC #7 lore raw

PATCH v3 4/6 RISC-V: Use Zicboz in clear_page when available

In reply to Andrew Jones

Quoted 112 lines · ajones at ventanamicro.com

Using memset() to zero a 4K page takes 563 total instructions where 20 are branches. clear_page() with Zicboz takes 150 total instructions where 16 are branches. We could reduce the numbers by further unrolling, but, since the cboz block size isn't fixed, we'd need a Duff device to ensure we don't execute too many unrolled steps. Also, cbo.zero doesn't take an offset, so each unrolled step requires it and an add instruction. This increases the chance for icache misses if we unroll many times. For these reasons we only unroll four times. Unrolling four times should be safe as it supports cboz block sizes up to 1K when used with 4K pages and it's only 24 to 32 bytes of unrolled instructions.
Another note about the Duff device idea is that it would probably be best to store the number of steps needed at boot time and then load the value in clear_page(). Calculating it in clear_page(), particularly without the Zbb extension, would not be efficient.
Signed-off-by Andrew Jones <[email protected]>
Acked-by Conor Dooley <[email protected]>
---
 arch/riscv/Kconfig                | 13 +++++++++++
 arch/riscv/include/asm/insn-def.h |  4 ++++
 arch/riscv/include/asm/page.h     |  6 +++++-
 arch/riscv/lib/Makefile           |  1 +
 arch/riscv/lib/clear_page.S       | 36 +++++++++++++++++++++++++++++++
 5 files changed, 59 insertions(+), 1 deletion(-)
 create mode 100644 arch/riscv/lib/clear_page.S
arch/riscv/Kconfig +13 -0
index 33bbdc33cef8..3759a2f6edd5 100644
@@ -432,6 +432,19 @@ config RISCV_ISA_ZICBOM
 
 	   If you don't know what to do here, say Y.
 
+config RISCV_ISA_ZICBOZ
+	bool "Zicboz extension support for faster zeroing of memory"
+	depends on !XIP_KERNEL && MMU
+	select RISCV_ALTERNATIVE
+	default y
+	help
+	   Enable the use of the ZICBOZ extension (cbo.zero instruction)
+	   when available.
+
+	   The Zicboz extension is used for faster zeroing of memory.
+
+	   If you don't know what to do here, say Y.
+
 config TOOLCHAIN_HAS_ZIHINTPAUSE
 	bool
 	default y
arch/riscv/include/asm/insn-def.h +4 -0
index e01ab51f50d2..6960beb75f32 100644
@@ -192,4 +192,8 @@
 	INSN_I(OPCODE_MISC_MEM, FUNC3(2), __RD(0),		\
 	       RS1(base), SIMM12(2))
 
+#define CBO_zero(base)						\
+	INSN_I(OPCODE_MISC_MEM, FUNC3(2), __RD(0),		\
+	       RS1(base), SIMM12(4))
+
 #endif /* __ASM_INSN_DEF_H */
arch/riscv/include/asm/page.h +5 -1
index 9f432c1b5289..ccd168fe29d2 100644
@@ -49,10 +49,14 @@
 
 #ifndef __ASSEMBLY__
 
+#ifdef CONFIG_RISCV_ISA_ZICBOZ
+void clear_page(void *page);
+#else
 #define clear_page(pgaddr)			memset((pgaddr), 0, PAGE_SIZE)
+#endif
 #define copy_page(to, from)			memcpy((to), (from), PAGE_SIZE)
 
-#define clear_user_page(pgaddr, vaddr, page)	memset((pgaddr), 0, PAGE_SIZE)
+#define clear_user_page(pgaddr, vaddr, page)	clear_page(pgaddr)
 #define copy_user_page(vto, vfrom, vaddr, topg) \
 			memcpy((vto), (vfrom), PAGE_SIZE)
 
arch/riscv/lib/Makefile +1 -0
index 25d5c9664e57..9ee5e2ab5143 100644
@@ -5,5 +5,6 @@ lib-y			+= memset.o
 lib-y			+= memmove.o
 lib-$(CONFIG_MMU)	+= uaccess.o
 lib-$(CONFIG_64BIT)	+= tishift.o
+lib-$(CONFIG_RISCV_ISA_ZICBOZ)	+= clear_page.o
 
 obj-$(CONFIG_FUNCTION_ERROR_INJECTION) += error-inject.o
arch/riscv/lib/clear_page.S new file +10 -0
new file mode 100644
index 000000000000..49f29139a5b6
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2023 Ventana Micro Systems Inc.
+ */
+
+#include <linux/linkage.h>
+#include <asm/asm.h>
+#include <asm/alternative-macros.h>
+#include <asm/hwcap.h>
+#include <asm/insn-def.h>

arch/riscv/lib/clear_page.S line 20

Quoted 10 lines

arch/riscv/lib/clear_page.S new file +10 -0

+#include <asm/page.h>
+
+/* void clear_page(void *page) */
+ENTRY(__clear_page)
+WEAK(clear_page)
+	li	a2, PAGE_SIZE
+	ALTERNATIVE("j .Lno_zicboz", "nop",
+		    0, RISCV_ISA_EXT_ZICBOZ, CONFIG_RISCV_ISA_ZICBOZ)
+	la	a1, riscv_cboz_block_size
+	lw	a1, 0(a1)

You should be able to just "lw a1, riscv_cboz_block_size", which can sometimes generate better code.

arch/riscv/lib/clear_page.S line 26

Quoted 6 lines

arch/riscv/lib/clear_page.S new file +6 -0

+	add	a2, a0, a2
+.Lzero_loop:
+	CBO_zero(a0)
+	add	a0, a0, a1
+	CBO_zero(a0)
+	add	a0, a0, a1

We were talking about this in the patchwork call: this risks overflow if the block size is bigger than a quarter of a page. That's probably pretty rare, but given that there's already an alternative for the jump it's easy to check.

Quoted 10 lines

arch/riscv/lib/clear_page.S new file +10 -0

+	CBO_zero(a0)
+	add	a0, a0, a1
+	CBO_zero(a0)
+	add	a0, a0, a1
+	bltu	a0, a2, .Lzero_loop
+	ret
+.Lno_zicboz:
+	li	a1, 0
+	tail	__memset
+END(__clear_page)

Andrew Jones

author

2023-02-02 07:41 UTC #8 lore raw

PATCH v3 4/6 RISC-V: Use Zicboz in clear_page when available

Quoted 127 lines · Palmer Dabbelt

Quoted 112 lines · ajones at ventanamicro.com
Using memset() to zero a 4K page takes 563 total instructions where 20 are branches. clear_page() with Zicboz takes 150 total instructions where 16 are branches. We could reduce the numbers by further unrolling, but, since the cboz block size isn't fixed, we'd need a Duff device to ensure we don't execute too many unrolled steps. Also, cbo.zero doesn't take an offset, so each unrolled step requires it and an add instruction. This increases the chance for icache misses if we unroll many times. For these reasons we only unroll four times. Unrolling four times should be safe as it supports cboz block sizes up to 1K when used with 4K pages and it's only 24 to 32 bytes of unrolled instructions.
Another note about the Duff device idea is that it would probably be best to store the number of steps needed at boot time and then load the value in clear_page(). Calculating it in clear_page(), particularly without the Zbb extension, would not be efficient.
Signed-off-by Andrew Jones <[email protected]>
Acked-by Conor Dooley <[email protected]>
---
 arch/riscv/Kconfig                | 13 +++++++++++
 arch/riscv/include/asm/insn-def.h |  4 ++++
 arch/riscv/include/asm/page.h     |  6 +++++-
 arch/riscv/lib/Makefile           |  1 +
 arch/riscv/lib/clear_page.S       | 36 +++++++++++++++++++++++++++++++
 5 files changed, 59 insertions(+), 1 deletion(-)
 create mode 100644 arch/riscv/lib/clear_page.S
arch/riscv/Kconfig +13 -0
index 33bbdc33cef8..3759a2f6edd5 100644
@@ -432,6 +432,19 @@ config RISCV_ISA_ZICBOM
 
 	   If you don't know what to do here, say Y.
 
+config RISCV_ISA_ZICBOZ
+	bool "Zicboz extension support for faster zeroing of memory"
+	depends on !XIP_KERNEL && MMU
+	select RISCV_ALTERNATIVE
+	default y
+	help
+	   Enable the use of the ZICBOZ extension (cbo.zero instruction)
+	   when available.
+
+	   The Zicboz extension is used for faster zeroing of memory.
+
+	   If you don't know what to do here, say Y.
+
 config TOOLCHAIN_HAS_ZIHINTPAUSE
 	bool
 	default y
arch/riscv/include/asm/insn-def.h +4 -0
index e01ab51f50d2..6960beb75f32 100644
@@ -192,4 +192,8 @@
 	INSN_I(OPCODE_MISC_MEM, FUNC3(2), __RD(0),		\
 	       RS1(base), SIMM12(2))
 
+#define CBO_zero(base)						\
+	INSN_I(OPCODE_MISC_MEM, FUNC3(2), __RD(0),		\
+	       RS1(base), SIMM12(4))
+
 #endif /* __ASM_INSN_DEF_H */
arch/riscv/include/asm/page.h +5 -1
index 9f432c1b5289..ccd168fe29d2 100644
@@ -49,10 +49,14 @@
 
 #ifndef __ASSEMBLY__
 
+#ifdef CONFIG_RISCV_ISA_ZICBOZ
+void clear_page(void *page);
+#else
 #define clear_page(pgaddr)			memset((pgaddr), 0, PAGE_SIZE)
+#endif
 #define copy_page(to, from)			memcpy((to), (from), PAGE_SIZE)
 
-#define clear_user_page(pgaddr, vaddr, page)	memset((pgaddr), 0, PAGE_SIZE)
+#define clear_user_page(pgaddr, vaddr, page)	clear_page(pgaddr)
 #define copy_user_page(vto, vfrom, vaddr, topg) \
 			memcpy((vto), (vfrom), PAGE_SIZE)
 
arch/riscv/lib/Makefile +1 -0
index 25d5c9664e57..9ee5e2ab5143 100644
@@ -5,5 +5,6 @@ lib-y			+= memset.o
 lib-y			+= memmove.o
 lib-$(CONFIG_MMU)	+= uaccess.o
 lib-$(CONFIG_64BIT)	+= tishift.o
+lib-$(CONFIG_RISCV_ISA_ZICBOZ)	+= clear_page.o
 
 obj-$(CONFIG_FUNCTION_ERROR_INJECTION) += error-inject.o
arch/riscv/lib/clear_page.S new file +10 -0
new file mode 100644
index 000000000000..49f29139a5b6
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2023 Ventana Micro Systems Inc.
+ */
+
+#include <linux/linkage.h>
+#include <asm/asm.h>
+#include <asm/alternative-macros.h>
+#include <asm/hwcap.h>
+#include <asm/insn-def.h>
arch/riscv/lib/clear_page.S line 20
Quoted 10 lines
arch/riscv/lib/clear_page.S new file +10 -0
+#include <asm/page.h>
+
+/* void clear_page(void *page) */
+ENTRY(__clear_page)
+WEAK(clear_page)
+	li	a2, PAGE_SIZE
+	ALTERNATIVE("j .Lno_zicboz", "nop",
+		    0, RISCV_ISA_EXT_ZICBOZ, CONFIG_RISCV_ISA_ZICBOZ)
+	la	a1, riscv_cboz_block_size
+	lw	a1, 0(a1)
You should be able to just "lw a1, riscv_cboz_block_size", which can sometimes generate better code.

Quoted 11 lines

arch/riscv/lib/clear_page.S line 26
Quoted 6 lines
arch/riscv/lib/clear_page.S new file +6 -0
+	add	a2, a0, a2
+.Lzero_loop:
+	CBO_zero(a0)
+	add	a0, a0, a1
+	CBO_zero(a0)
+	add	a0, a0, a1
We were talking about this in the patchwork call: this risks overflow if the block size is bigger than a quarter of a page. That's probably pretty rare, but given that there's already an alternative for the jump it's easy to check.

I'll pull v4 together with your fixes.

Thanks!

drew

Quoted 11 lines

arch/riscv/lib/clear_page.S new file +10 -0

+	CBO_zero(a0)
+	add	a0, a0, a1
+	CBO_zero(a0)
+	add	a0, a0, a1
+	bltu	a0, a2, .Lzero_loop
+	ret
+.Lno_zicboz:
+	li	a1, 0
+	tail	__memset
+END(__clear_page)