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.

Sign up for GitHub

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

Jump to bottom

cmd/asm: aes related tests fail with SIGILL on z13 #63387

Closed
jcajka opened this issue Oct 5, 2023 · 13 comments
Closed

cmd/asm: aes related tests fail with SIGILL on z13 #63387

jcajka opened this issue Oct 5, 2023 · 13 comments
Assignees
@srinivas-pokala
Labels
arch-s390x Issues solely affecting the s390x architecture. compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@jcajka
Copy link
Contributor

jcajka commented Oct 5, 2023

What version of Go are you using (go version)?

master branch

Does this issue reproduce with the latest release?

No

What operating system and processor architecture are you using (go env)?

linux/s390x

What did you do?

GOROOT_BOOTSTRAP=... ./all.bash

What did you expect to see?

All tests passing

What did you see instead?

Example of one of the failures.

SIGILL: illegal instruction
PC=0x14a960 m=0 sigcode=2
instruction bytes: 0xa7 0x14 0xff 0xfe 0x7 0xfe 0xe5 0x48 0x0 0x0 0x0 0x0 0x7 0xfe 0x0 0x0

goroutine 14 [running]:
crypto/aes.cryptBlocksGCM(0x12, {0xc00007cea0, 0x10, 0x20}, {0xc00001a3e0, 0x10, 0x20}, {0xc000134c10, 0x10, 0x800}, ...)
	/home/jcajka/upstream/go/src/crypto/aes/asm_s390x.s:131 +0xe0 fp=0xc000134b88 sp=0xc000134b88 pc=0x14a960
crypto/aes.(*gcmAsm).counterCrypt(0xc000078660, {0xc00001a3e0, 0x20, 0x20}, {0xc00001a3c0, 0x10, 0x20}, 0xc000135cd0)
	/home/jcajka/upstream/go/src/crypto/aes/gcm_s390x.go:151 +0x228 fp=0xc000135c50 sp=0xc000134b88 pc=0x149328
crypto/aes.(*gcmAsm).Seal(0xc000078660, {0x0, 0x0, 0x0}, {0xc000014198, 0xc, 0x18}, {0xc00001a3c0, 0x10, 0x20}, ...)
	/home/jcajka/upstream/go/src/crypto/aes/gcm_s390x.go:225 +0x228 fp=0xc000135d18 sp=0xc000135c50 pc=0x1499a8
crypto/cipher_test.TestAESGCM(0xc000126820)
	/home/jcajka/upstream/go/src/crypto/cipher/gcm_test.go:273 +0x5c6 fp=0xc000135f60 sp=0xc000135d18 pc=0x1527c6
testing.tRunner(0xc000126820, 0x1a7f18)
	/home/jcajka/upstream/go/src/testing/testing.go:1593 +0x13e fp=0xc000135fc0 sp=0xc000135f60 pc=0xfe35e
testing.(*T).Run.gowrap1()
	/home/jcajka/upstream/go/src/testing/testing.go:1646 +0x64 fp=0xc000135fd8 sp=0xc000135fc0 pc=0xff684
runtime.goexit({})
	/home/jcajka/upstream/go/src/runtime/asm_s390x.s:774 +0x2 fp=0xc000135fd8 sp=0xc000135fd8 pc=0x91882
created by testing.(*T).Run in goroutine 1
	/home/jcajka/upstream/go/src/testing/testing.go:1646 +0x47c

goroutine 1 [chan receive]:
runtime.gopark(0x1a8128, 0xc00001c528, 0xe, 0x17, 0x2)
	/home/jcajka/upstream/go/src/runtime/proc.go:400 +0x136 fp=0xc0001049e8 sp=0xc0001049d0 pc=0x57816
runtime.chanrecv(0xc00001c4d0, 0xc000104ad7, 0x1)
	/home/jcajka/upstream/go/src/runtime/chan.go:583 +0x5ac fp=0xc000104a60 sp=0xc0001049e8 pc=0x17c2c
runtime.chanrecv1(0xc00001c4d0, 0xc000104ad7)
	/home/jcajka/upstream/go/src/runtime/chan.go:442 +0x2e fp=0xc000104a88 sp=0xc000104a60 pc=0x1766e
testing.(*T).Run(0xc0000036c0, {0x198b02, 0xa}, 0x1a7f18)
	/home/jcajka/upstream/go/src/testing/testing.go:1647 +0x49c fp=0xc000104b40 sp=0xc000104a88 pc=0xff4cc
testing.runTests.func1(0xc0000036c0)
	/home/jcajka/upstream/go/src/testing/testing.go:2052 +0x62 fp=0xc000104b80 sp=0xc000104b40 pc=0x1019e2
testing.tRunner(0xc0000036c0, 0xc000104c90)
	/home/jcajka/upstream/go/src/testing/testing.go:1593 +0x13e fp=0xc000104be0 sp=0xc000104b80 pc=0xfe35e
testing.runTests(0xc00000c048, {0x29b360, 0xe, 0xe}, {0xc13f1650377a9793, 0x7dba847778, 0x2a0800})
	/home/jcajka/upstream/go/src/testing/testing.go:2050 +0x4ba fp=0xc000104cb0 sp=0xc000104be0 pc=0x1018aa
testing.(*M).Run(0xc0000740a0)
	/home/jcajka/upstream/go/src/testing/testing.go:1923 +0x66e fp=0xc000104ee8 sp=0xc000104cb0 pc=0xfff8e
main.main()
	_testmain.go:107 +0x1a2 fp=0xc000104f68 sp=0xc000104ee8 pc=0x1559a2
runtime.main()
	/home/jcajka/upstream/go/src/runtime/proc.go:269 +0x2f2 fp=0xc000104fd8 sp=0xc000104f68 pc=0x57262
runtime.goexit({})
	/home/jcajka/upstream/go/src/runtime/asm_s390x.s:774 +0x2 fp=0xc000104fd8 sp=0xc000104fd8 pc=0x91882

goroutine 2 [force gc (idle)]:
runtime.gopark(0x1a8408, 0x2a03d0, 0x11, 0x14, 0x1)
	/home/jcajka/upstream/go/src/runtime/proc.go:400 +0x136 fp=0xc00003efb0 sp=0xc00003ef98 pc=0x57816
runtime.goparkunlock(...)
	/home/jcajka/upstream/go/src/runtime/proc.go:406
runtime.forcegchelper()
	/home/jcajka/upstream/go/src/runtime/proc.go:324 +0xd2 fp=0xc00003efd8 sp=0xc00003efb0 pc=0x575e2
runtime.goexit({})
	/home/jcajka/upstream/go/src/runtime/asm_s390x.s:774 +0x2 fp=0xc00003efd8 sp=0xc00003efd8 pc=0x91882
created by runtime.init.5 in goroutine 1
	/home/jcajka/upstream/go/src/runtime/proc.go:312 +0x30

goroutine 3 [GC sweep wait]:
runtime.gopark(0x1a8408, 0x2a0700, 0xc, 0x14, 0x1)
	/home/jcajka/upstream/go/src/runtime/proc.go:400 +0x136 fp=0xc00003f790 sp=0xc00003f778 pc=0x57816
runtime.goparkunlock(...)
	/home/jcajka/upstream/go/src/runtime/proc.go:406
runtime.bgsweep(0xc00001c070)
	/home/jcajka/upstream/go/src/runtime/mgcsweep.go:280 +0xaa fp=0xc00003f7c8 sp=0xc00003f790 pc=0x3d64a
runtime.gcenable.gowrap1()
	/home/jcajka/upstream/go/src/runtime/mgc.go:203 +0x5e fp=0xc00003f7d8 sp=0xc00003f7c8 pc=0x2fd5e
runtime.goexit({})
	/home/jcajka/upstream/go/src/runtime/asm_s390x.s:774 +0x2 fp=0xc00003f7d8 sp=0xc00003f7d8 pc=0x91882
created by runtime.gcenable in goroutine 1
	/home/jcajka/upstream/go/src/runtime/mgc.go:203 +0xa8

goroutine 4 [GC scavenge wait]:
runtime.gopark(0x1a8408, 0x2a0880, 0xd, 0x14, 0x2)
	/home/jcajka/upstream/go/src/runtime/proc.go:400 +0x136 fp=0xc00003ff80 sp=0xc00003ff68 pc=0x57816
runtime.goparkunlock(...)
	/home/jcajka/upstream/go/src/runtime/proc.go:406
runtime.(*scavengerState).park(0x2a0880)
	/home/jcajka/upstream/go/src/runtime/mgcscavenge.go:425 +0x72 fp=0xc00003ffa8 sp=0xc00003ff80 pc=0x3a8c2
runtime.bgscavenge(0xc00001c070)
	/home/jcajka/upstream/go/src/runtime/mgcscavenge.go:653 +0x5a fp=0xc00003ffc8 sp=0xc00003ffa8 pc=0x3af7a
runtime.gcenable.gowrap2()
	/home/jcajka/upstream/go/src/runtime/mgc.go:204 +0x5e fp=0xc00003ffd8 sp=0xc00003ffc8 pc=0x2fcde
runtime.goexit({})
	/home/jcajka/upstream/go/src/runtime/asm_s390x.s:774 +0x2 fp=0xc00003ffd8 sp=0xc00003ffd8 pc=0x91882
created by runtime.gcenable in goroutine 1
	/home/jcajka/upstream/go/src/runtime/mgc.go:204 +0x10e

goroutine 5 [runnable]:
runtime.runfinq()
	/home/jcajka/upstream/go/src/runtime/mfinal.go:176 fp=0xc0000407d8 sp=0xc0000407d8 pc=0x2e7e0
runtime.goexit({})
	/home/jcajka/upstream/go/src/runtime/asm_s390x.s:774 +0x2 fp=0xc0000407d8 sp=0xc0000407d8 pc=0x91882
created by runtime.createfing in goroutine 1
	/home/jcajka/upstream/go/src/runtime/mfinal.go:163 +0x64

r0   0x12	r1   0xc00007cea0
r2   0xc000135410	r3   0x3efd488700000003
r4   0xc00001a3e0	r5   0x3efd488700000003
r6   0xc000134c10	r7   0x10
r8   0xee283a3fc75575e3	r9   0x3efd488700000005
r10  0xf	r11  0x10
r12  0xc000135cd0	r13  0xc000003a00
r14  0x149328	r15  0xc000134b88
pc   0x14a960	link 0x149328
FAIL	crypto/cipher	0.006s

Tests crypto/cipher, crypto/tls, net/http, net/http/httptest, net/smtp and cmd/go started to fail with SIGILL on z13 since commit a40404da74. It seems to me as some part of the aes optimizations is using features that are present only on z14 and newer now, but I can't find, missed any reference in the Principles of Operation.
@randall77
Copy link
Contributor

@Vishwanatha-HD

@cherrymui cherrymui added the arch-s390x Issues solely affecting the s390x architecture. label Oct 5, 2023
@laboger
Copy link
Contributor

laboger commented Oct 6, 2023

@billotosyr

@prattmic
Copy link
Member

prattmic commented Oct 6, 2023

cc @golang/s390x

@prattmic prattmic added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. compiler/runtime Issues related to the Go compiler and/or runtime. labels Oct 6, 2023
@Vishwanatha-HD
Copy link
Contributor

Hi All..
I am trying to get hold of a Z13 system.. I will start investigating on this sooner.. Thanks..

@mknyszek mknyszek added this to the Backlog milestone Oct 11, 2023
@mknyszek mknyszek moved this from Todo to In Progress in Go Compiler / Runtime Oct 11, 2023
@srinivas-pokala srinivas-pokala self-assigned this Oct 12, 2023
@srinivas-pokala
Copy link
Contributor

@jcajka Thanks for identifying the bug on z13.. I went through the code and I have identified the fix.

As part of adding assembly instruction mnemonics to the s390x.s files,"KMCTR" instruction was added.

But, it looks like the instruction encoding for operands was not handled properly. The operands were encoded in a reverse order when compared to what was expected. I have fixed the issue and tested it both on z13 and latest z15 machines. Its working fine. I will raise a CL to incorporate my changes. Thanks..

@jcajka
Copy link
Contributor Author

jcajka commented Oct 13, 2023

@srinivas-pokala thank you for quick response. Just for record, did it had any adverse effect on the z14+ or did the encoding changed between z13 and z14?

@srinivas-pokala
Copy link
Contributor

@jcajka , for the AES-GCM test "TestAESGCM" which contain's two implementations. First implementation (gcmAsm) uses KMCTR instruction to encrypt using AES in counter mode and the KIMD instruction for GHASH.
In the second implementation (gcmKMA) uses the newer KMA instruction which performs both operations.
Till z13 which uses first method,later(>z13) onwards which uses 2nd method.
As the issue is in KMCTR instruction encoding which using in 1st method, there is no effect on the z14++ as AES-GCM test's uses gcmKMA method. Hope this clarifies.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/535675 mentions this issue: cmd/asm: fix the KMCTR instruction encoding and argument passing

@jcajka
Copy link
Contributor Author

jcajka commented Oct 16, 2023

For the record just tested the CL, it fixes the issue for me. Thank you for quick fix.

@jcajka
Copy link
Contributor Author

jcajka commented Nov 9, 2023

As the freeze is approaching IMHO this seems to me as rather serious issue(effectively dropping z13) to make it in to the 1.22. If fix can't make it in to the 1.22, could the breaking change be reverted? CC'ing for visibility @srinivas-pokala @laboger @alexsaezm

@srinivas-pokala
Copy link
Contributor

@jcajka thank's for bringing the note, we are working on code review to get it merged asap before the freeze else will revert the breaking change.

@Vishwanatha-HD
Copy link
Contributor

@jcajka @randall77
From s390x perspective, I have reviewed the code changes done by Srinivas. I have worked on adding the instructions for s390x in the past and I fairly have an idea about this. And also, I have tested the changes on z13 machine as well. The changes are working fine.

So, from s390x arch perspective, I approve the changes. Request you to consider this and merge this CL asap before the code freeze happens, since its breaking the z13 build.
Thanks.

@randall77
Copy link
Contributor

@Vishwanatha-HD Probably better to comment on the CL, there are a few unresolved things over there. I think we'd all want this fixed once that CL is ready.

@github-project-automation github-project-automation bot moved this from In Progress to Done in Go Compiler / Runtime Nov 22, 2023
@golang golang locked and limited conversation to collaborators Nov 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@srinivas-pokala srinivas-pokala

Labels
arch-s390x Issues solely affecting the s390x architecture. compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
9 participants
@mknyszek @prattmic @randall77 @jcajka @gopherbot @laboger @cherrymui @srinivas-pokala @Vishwanatha-HD