Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix detection of IBM Power8 machines (ISA 2.07) #14576

Merged
merged 1 commit into from
Mar 7, 2023

Conversation

mcmilk
Copy link
Contributor

@mcmilk mcmilk commented Mar 5, 2023

An IBM POWER7 system with Power ISA 2.06 tried to execute zfs_sha256_power8() - which should only be run on ISA 2.07 machines.

The detection is implemented via the zfs_isa207_available() call, but this check was not used.

This pull request will fix this.

Motivation and Context

#13741 (comment)

Description

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@Low-power
Copy link
Contributor

Please add the followings to fix #13741 (comment):

diff --git a/module/icp/algs/sha2/sha512_impl.c b/module/icp/algs/sha2/sha512_impl.c
index b6341f6..8e1dfab 100644
--- a/module/icp/algs/sha2/sha512_impl.c
+++ b/module/icp/algs/sha2/sha512_impl.c
@@ -131,14 +131,14 @@ const sha512_ops_t sha512_ppc_impl = {
 	.name = "ppc"
 };
 
-static boolean_t sha512_have_vsx(void)
+static boolean_t sha512_have_isa207(void)
 {
-	return (kfpu_allowed() && zfs_vsx_available());
+	return (kfpu_allowed() && zfs_isa207_available());
 }
 
 TF(zfs_sha512_power8, tf_sha512_power8);
 const sha512_ops_t sha512_power8_impl = {
-	.is_supported = sha512_have_vsx,
+	.is_supported = sha512_have_isa207,
 	.transform = tf_sha512_power8,
 	.name = "power8"
 };

An IBM POWER7 system with Power ISA 2.06 tried to execute
zfs_sha256_power8() - which should only be run on ISA 2.07
machines.

The detection is implemented via the zfs_isa207_available() call,
but this check was not used.

This pull request will fix this.

Signed-off-by: Tino Reichardt <milky-zfs@mcmilk.de>
Signed-off-by: Low-power <msl0000023508@gmail.com>
Copy link
Contributor

@Low-power Low-power left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed to work on both POWER7 and POWER8 machines in my environment.

@mcmilk
Copy link
Contributor Author

mcmilk commented Mar 6, 2023

Confirmed to work on both POWER7 and POWER8 machines in my environment.

Could you post the output of cat /proc/spl/kstat/zfs/chksum_bench ?

@Low-power
Copy link
Contributor

POWER7 machine:

root@debian:~# modinfo zfs | grep ^version:
version:        2.1.99-1780_g620a977
root@debian:~# lscpu
Architecture:            ppc64
  CPU op-mode(s):        32-bit, 64-bit
  Byte Order:            Big Endian
CPU(s):                  4
  On-line CPU(s) list:   0-3
Model name:              POWER7 (architected), altivec supported
  Model:                 2.3 (pvr 003f 0203)
  Thread(s) per core:    4
  Core(s) per socket:    1
  Socket(s):             1
Virtualization features: 
  Hypervisor vendor:     pHyp
  Virtualization type:   para
Caches (sum of all):     
  L1d:                   32 KiB (1 instance)
  L1i:                   32 KiB (1 instance)
  L2:                    256 KiB (1 instance)
  L3:                    4 MiB (1 instance)
NUMA:                    
  NUMA node(s):          1
  NUMA node0 CPU(s):     0-3
root@debian:~# cat /proc/spl/kstat/zfs/chksum_bench
15 0 0x01 -1 0 302165441347 9745603564008
implementation               1k      4k     16k     64k    256k      1m      4m     16m
edonr-generic               336     439     475     472     480     486     782     786
skein-generic               194     213     223     222     222     222     102     212
sha256-generic               87      92      94      95      61      98      99      86
sha256-ppc                  177     192     189     195     196     197     196     179
sha512-generic              121     134     141     143     143     146     232     235
sha512-ppc                  287     325     333     339     343     344     345     347
blake3-generic              275     287     281     286     287     286     332     334

POWER8 machine:

root@debiansid-be:~# modinfo zfs | grep ^version:
version:        2.1.99-1780_g620a977f2
root@debiansid-be:~# lscpu
Architecture:          ppc64
  CPU op-mode(s):      32-bit, 64-bit
  Byte Order:          Big Endian
CPU(s):                176
  On-line CPU(s) list: 0-175
Model name:            POWER8 (raw), altivec supported
  Model:               2.0 (pvr 004d 0200)
  Thread(s) per core:  8
  Core(s) per socket:  11
  Socket(s):           2
  CPU(s) scaling MHz:  100%
  CPU max MHz:         3225.0000
  CPU min MHz:         2061.0000
Caches (sum of all):   
  L1d:                 1.4 MiB (22 instances)
  L1i:                 704 KiB (22 instances)
  L2:                  11 MiB (22 instances)
  L3:                  176 MiB (22 instances)
NUMA:                  
  NUMA node(s):        2
  NUMA node0 CPU(s):   0-87
  NUMA node8 CPU(s):   88-175
Vulnerabilities:       
  Itlb multihit:       Not affected
  L1tf:                Mitigation; RFI Flush
  Mds:                 Not affected
  Meltdown:            Mitigation; RFI Flush
  Mmio stale data:     Not affected
  Retbleed:            Not affected
  Spec store bypass:   Mitigation; Kernel entry/exit barrier (hwsync)
  Spectre v1:          Mitigation; __user pointer sanitization
  Spectre v2:          Vulnerable
  Srbds:               Not affected
  Tsx async abort:     Not affected
root@debiansid-be:~# cat /proc/spl/kstat/zfs/chksum_bench 
15 0 0x01 -1 0 21176320767 5428882179462
implementation               1k      4k     16k     64k    256k      1m      4m     16m
edonr-generic               871    1074    1138    1160    1168    1166    1169    1170
skein-generic               263     288     295     297     298     298     298     298
sha256-generic              121     129     131     132     132     132     132     132
sha256-ppc                  178     192     196     197     198     197     197     197
sha256-power8               284     310     317     319     319     319     319     319
sha512-generic              175     197     203     204     205     205     205     205
sha512-ppc                  261     296     308     311     312     312     312     312
sha512-power8               417     484     504     509     510     511     510     511
blake3-generic              183     181     182     182     182     182     182     182

@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Mar 7, 2023
@behlendorf behlendorf merged commit 84a1c48 into openzfs:master Mar 7, 2023
@mcmilk mcmilk deleted the ppc-isa-fix branch March 8, 2023 09:36
mcmilk added a commit to mcmilk/zfs that referenced this pull request Mar 13, 2023
An IBM POWER7 system with Power ISA 2.06 tried to execute
zfs_sha256_power8() - which should only be run on ISA 2.07
machines.

The detection is implemented via the zfs_isa207_available() call,
but this check was not used.

This pull request will fix this.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Richard Yao <richard.yao@alumni.stonybrook.edu>
Signed-off-by: Tino Reichardt <milky-zfs@mcmilk.de>
Signed-off-by: Low-power <msl0000023508@gmail.com>
Closes openzfs#14576
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 16, 2023
An IBM POWER7 system with Power ISA 2.06 tried to execute
zfs_sha256_power8() - which should only be run on ISA 2.07
machines.

The detection is implemented via the zfs_isa207_available() call,
but this check was not used.

This pull request will fix this.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Richard Yao <richard.yao@alumni.stonybrook.edu>
Signed-off-by: Tino Reichardt <milky-zfs@mcmilk.de>
Signed-off-by: Low-power <msl0000023508@gmail.com>
Closes openzfs#14576
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants