RISC-V: Apply Zicboz to clear_page
Subsystems (3)
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
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
@@ -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
PATCH v2 2/6 dt-bindings: riscv: Document cboz-block-size
In reply to Andrew JonesThe 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
@@ -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
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
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
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>
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
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@@ -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,
Reviewed-by Conor Dooley <[email protected]>
Thanks,
drew
Quoted 4 lines
Thanks,
Conor.
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
Rob Herring
maintainerPATCH v2 2/6 dt-bindings: riscv: Document cboz-block-size
In reply to Andrew JonesQuoted 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
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
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>
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 absentI 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
PATCH v2 3/6 RISC-V: Add Zicboz detection and block size parsing
In reply to Andrew JonesParse "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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
PATCH v2 4/6 RISC-V: Use Zicboz in clear_page when available
In reply to Andrew JonesUsing 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
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 inclear_page(), particularly without the Zbb extension, would not be efficient.Signed-off-by Andrew Jones <[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.Sarch/riscv/Kconfig +13
-0@@ -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 yarch/riscv/include/asm/insn-def.h +0
-0
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>
PATCH v2 5/6 RISC-V: KVM: Provide UAPI for Zicboz block size
In reply to Andrew JonesWe'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
@@ -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
@@ -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
PATCH v2 6/6 RISC-V: KVM: Expose Zicboz to the guest
In reply to Andrew JonesGuests 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
@@ -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
@@ -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