Skip to contentSkip to conversation

RISC-V: Apply Zicboz to clear_page

PATCH Superseded Andrew Jones 5 participants 15 messages Jan 22, 2023 – Jan 24, 2023

Subsystems (3)

v2 of 6 Superseded Latest is v6 →

lore
Share & tools
lore
#0 lore raw

v2 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 v4 00/13] riscv: improve boot time isa extensions handling"

    https://lore.kernel.org/all/20230115154953.831-1-jszhang at kernel.org/
    (Plus a fix for it, "riscv: module: Add ADD16 and SUB16 rela types",
     https://lore.kernel.org/all/20230120183418.ngdppppvwzysqtcr at orel/)
  - "[PATCH v1 0/3] Remove toolchain dependencies for Zicbom"
    https://lore.kernel.org/all/20230108163356.3063839-1-conor at kernel.org/
  - "[PATCH v2 0/3] Putting some basic order on isa extension lists"
    https://lore.kernel.org/all/20221205144525.2148448-1-conor.dooley at microchip.com/

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

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

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.0
#1 lore raw

PATCH v2 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.

---
 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.0
#2 lore raw

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

In reply to Andrew Jones

The Zicboz operates on a block-size defined for the cpu-core, which does not necessarily match other cache-sizes used.

So add the necessary property for the system to know the core's block-size.

---
 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.0
#3 lore raw

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

Hey,

Quoted 7 lines · Andrew Jones

The Zicboz operates on a block-size defined for the cpu-core, which does not necessarily match other cache-sizes used.

So add the necessary property for the system to know the core's block-size.

Cc Rob Herring <[email protected]>

FYI bindings need to be CC Krzysztof & the devicetree list too.

Quoted 10 lines
---
 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 78

Quoted 7 lines
Documentation/devicetree/bindings/riscv/cpus.yaml +4 -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.

Do you think hardware that has different Zicboz versus Zicbom block sizes is going to be common, or would we benefit from just defaulting the Zicboz size to the Zicbom one?

Either way,

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/20230123/6a7fc562/attachment.sig>

#4 lore raw

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

Quoted 6 lines · Conor Dooley

Hey,

Quoted 3 lines · Andrew Jones

The Zicboz operates on a block-size defined for the cpu-core, which does not necessarily match other cache-sizes used.

Quoted 6 lines
Quoted 4 lines

So add the necessary property for the system to know the core's block-size.

Cc Rob Herring <[email protected]>

FYI bindings need to be CC Krzysztof & the devicetree list too.

Thanks, hopefully CC'ing them now is OK (I just added them). If not, I can repost.

Quoted 14 lines
Quoted 14 lines
---
 Documentation/devicetree/bindings/riscv/cpus.yaml | 5 +++++
 1 file changed, 5 insertions(+)
Documentation/devicetree/bindings/riscv/cpus.yaml +0 -0
index c6720764e765..f4ee70f8e1cf 100644
@@ -72,6 +72,11 @@ properties:
     description:
       The blocksize in bytes for the Zicbom cache operations.
 
Quoted 8 lines

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

Quoted 4 lines
Documentation/devicetree/bindings/riscv/cpus.yaml +4 -0
+  riscv,cboz-block-size:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      The blocksize in bytes for the Zicboz cache operations.

Do you think hardware that has different Zicboz versus Zicbom block sizes is going to be common, or would we benefit from just defaulting the Zicboz size to the Zicbom one?

I'm not sure. If it turns out the block size will be the same in most cases, then we could add another property named cbo-block-size, which, when present, would be parsed first and used to populate all CBO-related block sizes. Then, these specific properties would only serve to override that general size for their respective block sizes when they're present.

Quoted 3 lines

Either way,

Thanks,
drew

Quoted 4 lines

Thanks,
Conor.

#5 lore raw

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

Quoted 10 lines · Andrew Jones
Quoted 9 lines · Conor Dooley

Hey,

Quoted 6 lines · Andrew Jones

The Zicboz operates on a block-size defined for the cpu-core, which does not necessarily match other cache-sizes used.

So add the necessary property for the system to know the core's block-size.

Quoted 7 lines
Quoted 3 lines
Quoted 1 line
Cc Rob Herring <[email protected]>

FYI bindings need to be CC Krzysztof & the devicetree list too.

Thanks, hopefully CC'ing them now is OK (I just added them). If not, I can repost.

It does not work like this. I don't have the patch in my inbox. How it should magically appear there?

Also how do you expect the bot to get it?

You need to use get_maintainers.pl *always*. Please resend.

Best regards,
Krzysztof

maintainer
#6 lore raw

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

In reply to Andrew Jones
Quoted 3 lines · Andrew Jones

The Zicboz operates on a block-size defined for the cpu-core, which does not necessarily match other cache-sizes used.

Please use get_maintainers.pl and send patches to the correct lists.

I have no idea what Zicboz is. How does it relate to Zicbom for which we already have a block size property? I really hate one by one property additions because they lead to poorly designed bindings. So what's next? What other information might be needed?

Rob

#7 lore raw

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

Quoted 6 lines · Rob Herring
Quoted 3 lines · Andrew Jones

The Zicboz operates on a block-size defined for the cpu-core, which does not necessarily match other cache-sizes used.

Please use get_maintainers.pl and send patches to the correct lists.

Yup, Conor also pointed out that I forgot to update the CC list when adding this patch to the series.

Quoted 5 lines

I have no idea what Zicboz is. How does it relate to Zicbom for which we already have a block size property? I really hate one by one property additions because they lead to poorly designed bindings. So what's next? What other information might be needed?

Zicbom and Zicboz are both RISC-V ISA extensions for cache-block operation (CBO) instructions. Zicbom defines the instructions cbo.inval, cbo.clean, and cbo.flush while Zicboz only defines cbo.zero. While it's probably likely that the cache block sizes which Zicbom and Zicboz use will be the same when both are implemented, the specification does not require it. With that in mind, it makes sense to me that Zicbom has its own property and that Zicboz could follow Zicbom's pattern with its own property as well.

That said, having a generic block size property which is used in the absence of the per-extension block size properties would allow DTs to only specify the size once when they're the same. In my reply to Conor, I suggested introducing a cbo-block-size property for this purpose, but Anup suggests we just expand the purpose of cbom-block-size. Expanding cbom- block-size's purpose would allow its size to be used with cbo.zero in the absence of a cboz-block-size property. Additionally, we could defer the introduction of the cboz-block-size property until some system needs it, which may be never.

As far as to what's coming next, I'm not aware of a plan for more of these types of properties at this time, but the CMO spec also describes prefetch instructions, which are defined under the Zicbop extension. If Zicbop support is added, then it should follow the same pattern as we agree for Zicboz, which is either

a. Add cboz-block-size and require it (as this series currently does) b. Add cboz-block-size, expand the function of cbom-block-size to be
a fallback, and fallback to cbom-block-size when cboz-block-size is absent
c. Don't add cboz-block-size, only expand the function of cbom-block-size
and use it. If a need arises for cboz-block-size some day, then it can be added at that time.
d. ??

I'm not aware of any additional information needed for these extensions beyond the block sizes.

Thanks,
drew

#8 lore raw

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

Hey Drew, Rob,

Quoted 36 lines · Andrew Jones
Quoted 6 lines · Rob Herring
Quoted 3 lines · Andrew Jones

The Zicboz operates on a block-size defined for the cpu-core, which does not necessarily match other cache-sizes used.

Please use get_maintainers.pl and send patches to the correct lists.

Yup, Conor also pointed out that I forgot to update the CC list when adding this patch to the series.

Quoted 5 lines

I have no idea what Zicboz is. How does it relate to Zicbom for which we already have a block size property? I really hate one by one property additions because they lead to poorly designed bindings. So what's next? What other information might be needed?

Zicbom and Zicboz are both RISC-V ISA extensions for cache-block operation (CBO) instructions. Zicbom defines the instructions cbo.inval, cbo.clean, and cbo.flush while Zicboz only defines cbo.zero. While it's probably likely that the cache block sizes which Zicbom and Zicboz use will be the same when both are implemented, the specification does not require it. With that in mind, it makes sense to me that Zicbom has its own property and that Zicboz could follow Zicbom's pattern with its own property as well.

That said, having a generic block size property which is used in the absence of the per-extension block size properties would allow DTs to only specify the size once when they're the same. In my reply to Conor, I suggested introducing a cbo-block-size property for this purpose, but Anup suggests we just expand the purpose of cbom-block-size. Expanding cbom- block-size's purpose would allow its size to be used with cbo.zero in the absence of a cboz-block-size property. Additionally, we could defer the introduction of the cboz-block-size property until some system needs it, which may be never.

Quoted 7 lines

As far as to what's coming next, I'm not aware of a plan for more of these types of properties at this time, but the CMO spec also describes prefetch instructions, which are defined under the Zicbop extension. If Zicbop support is added, then it should follow the same pattern as we agree for Zicboz, which is either

a. Add cboz-block-size and require it (as this series currently does)

heh, be careful with the word "require", in dt-binding terms it's not required - we just get a pr_err() and the feature is disabled, right?

This is most clear of the options though, even if it will likely be superfluous most of the time...

Quoted 3 lines

c. Don't add cboz-block-size, only expand the function of cbom-block-size
and use it. If a need arises for cboz-block-size some day, then it can be added at that time.

I don't really think that this one makes much sense. It'd be perfectly okay to have Zicboz without Zicbom, even if that scenario may be unlikely.
Having a system with Zicboz in the isa string, a cbom-block-size just sounds like a source of confusion.
IMO, there's enough potential crud that "random" extensions may bring going forward that I'd rather not make decisions here that complicate matters more.

Quoted 3 lines

b. Add cboz-block-size, expand the function of cbom-block-size to be
a fallback, and fallback to cbom-block-size when cboz-block-size is absent

I personally think that this one is pretty fair. I won't even try to claim knowledge of what decisions hardware folk will make, but I think it is likely that cbo.zero will share its block size with the other three cbo instructions that we already support.

idk if you can refer to other properties in a binding, but effectively I am suggesting:
riscv,cboz-block-size:
$ref: /schemas/types.yaml#/definitions/uint32
default: riscv,cbom-block-size
description:
The blocksize in bytes for the Zicboz cache operations.

Quoted 1 line

d. ??

Have both properties and default them to the regular old cache block sizes, thereby allowing Zicbom/Zicboz in the ISA string without either property at all?
Or is that one an ABI break? And if it is, do we care since there doesn't appear to be a linux-capable, Zicbo* compatible SoC yet?

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/20230123/80e3c33a/attachment-0001.sig>

#9 lore raw

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

Quoted 45 lines · Conor Dooley

Hey Drew, Rob,

Quoted 42 lines · Andrew Jones
Quoted 6 lines · Rob Herring
Quoted 3 lines · Andrew Jones

The Zicboz operates on a block-size defined for the cpu-core, which does not necessarily match other cache-sizes used.

Please use get_maintainers.pl and send patches to the correct lists.

Yup, Conor also pointed out that I forgot to update the CC list when adding this patch to the series.

Quoted 5 lines

I have no idea what Zicboz is. How does it relate to Zicbom for which we already have a block size property? I really hate one by one property additions because they lead to poorly designed bindings. So what's next? What other information might be needed?

Zicbom and Zicboz are both RISC-V ISA extensions for cache-block operation (CBO) instructions. Zicbom defines the instructions cbo.inval, cbo.clean, and cbo.flush while Zicboz only defines cbo.zero. While it's probably likely that the cache block sizes which Zicbom and Zicboz use will be the same when both are implemented, the specification does not require it. With that in mind, it makes sense to me that Zicbom has its own property and that Zicboz could follow Zicbom's pattern with its own property as well.

That said, having a generic block size property which is used in the absence of the per-extension block size properties would allow DTs to only specify the size once when they're the same. In my reply to Conor, I suggested introducing a cbo-block-size property for this purpose, but Anup suggests we just expand the purpose of cbom-block-size. Expanding cbom- block-size's purpose would allow its size to be used with cbo.zero in the absence of a cboz-block-size property. Additionally, we could defer the introduction of the cboz-block-size property until some system needs it, which may be never.

As far as to what's coming next, I'm not aware of a plan for more of these types of properties at this time, but the CMO spec also describes prefetch instructions, which are defined under the Zicbop extension. If Zicbop support is added, then it should follow the same pattern as we agree for Zicboz, which is either

Quoted 4 lines
Quoted 1 line

a. Add cboz-block-size and require it (as this series currently does)

heh, be careful with the word "require", in dt-binding terms it's not required - we just get a pr_err() and the feature is disabled, right?

Correct. Here "require" means that without this property Zicboz won't work, not that the DT won't validate. I'll use "need" for this purpose to avoid confusion below :-)

Quoted 3 lines

This is most clear of the options though, even if it will likely be superfluous most of the time...

I'm leaning more towards this approach (and not just because it's already done). While this property may potentially be redundant with cbom-block-size and cache-block-size, one should be sure they know Zicboz's cache block size before they use it. Having to explicitly assign it to a property in the DT to get Zicboz to work should ensure they've double checked their manuals. Otherwise, potentially difficult to debug issues may emerge.

Quoted 14 lines
Quoted 4 lines

c. Don't add cboz-block-size, only expand the function of cbom-block-size
and use it. If a need arises for cboz-block-size some day, then it can be added at that time.

I don't really think that this one makes much sense. It'd be perfectly okay to have Zicboz without Zicbom, even if that scenario may be unlikely.
Having a system with Zicboz in the isa string, a cbom-block-size just sounds like a source of confusion.
IMO, there's enough potential crud that "random" extensions may bring going forward that I'd rather not make decisions here that complicate matters more.

Quoted 8 lines
Quoted 3 lines

b. Add cboz-block-size, expand the function of cbom-block-size to be
a fallback, and fallback to cbom-block-size when cboz-block-size is absent

I personally think that this one is pretty fair. I won't even try to claim knowledge of what decisions hardware folk will make, but I think it is likely that cbo.zero will share its block size with the other three cbo instructions that we already support.

While I think we're all in agreement on the likeliness of these block sizes being the same, I think the fact that we have to use the word 'likely' implies we should just stick to explicit properties.

Quoted 9 lines

idk if you can refer to other properties in a binding, but effectively I am suggesting:
riscv,cboz-block-size:
$ref: /schemas/types.yaml#/definitions/uint32
default: riscv,cbom-block-size
description:
The blocksize in bytes for the Zicboz cache operations.

Quoted 7 lines
Quoted 1 line

d. ??

Have both properties and default them to the regular old cache block sizes, thereby allowing Zicbom/Zicboz in the ISA string without either property at all?
Or is that one an ABI break? And if it is, do we care since there doesn't appear to be a linux-capable, Zicbo* compatible SoC yet?

I don't think it would be ABI breakage unless we need to preserve failures in the absence of cbom-block-size and/or cannot expand the scope of cache-block-size to include Zicbom and Zicboz. IMO, those should be safe changes, but I'm still leaning towards keeping all these sizes explicit and needed for their respective extensions, particularly if we believe we should add the properties anyway.

Thanks,
drew

#10 lore raw

PATCH v2 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().

---
 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 a6e28363e0ef..9a138c140efa 100644
@@ -46,6 +46,7 @@
 #define RISCV_ISA_EXT_ZIHINTPAUSE	29
 #define RISCV_ISA_EXT_SSTC		30
 #define RISCV_ISA_EXT_SVINVAL		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 c1274f8b97b0..bc5a7d6a2160 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.0
#11 lore raw

PATCH v2 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.

---
 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.0
#12 lore raw

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

Quoted 56 lines · 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.

---
 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 +0 -0
index e01ab51f50d2..6960beb75f32 100644

arch/riscv/include/asm/insn-def.h line 198

Quoted 7 lines
arch/riscv/include/asm/insn-def.h +4 -0
@@ -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))
+

Since most of the patch is the clear page implementation that I have no comments on:

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/20230123/3f8f6643/attachment.sig>

#13 lore raw

PATCH v2 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.

---
 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.0
#14 lore raw

PATCH v2 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.

---
 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.0