Apart from instructions with vector operands, Valgrind does not implement the additional z/Architecture instructions introduced with z13. These are: - load and zero rightmost byte (LZRF, LZRG); - load logical and zero rightmost byte (LLZRGF); - load halfword high immediate on condition (LOCHHI); - load halfword immediate on condition (LOCHI, LOCGHI); - load high on condition (LOCFHR, LOCFH); - store high on condition (STOCFH); - perform pseudorandom number operation (PPNO), with the functions PPNO-Query and PPNO-SHA-512-DRNG; - load count to block boundary (LCBB).
Note that work on this bug is done. I'll submit code right after patch for "z13 vector "support" instructions" (https://bugs.kde.org/show_bug.cgi?id=385408) is merged into main repo.
Created attachment 110193 [details] Non-vector z13 support
It can be applied on master. Please take a look.
Created attachment 110203 [details] Non-vector z13 support Changes: Structured into sequence of patches.
(In reply to Vadim Barkov from comment #4) > Created attachment 110203 [details] > Non-vector z13 support Looks mostly OK to me. One thing I noticed is that the "z13" test case isn't memcheck-clean: $ ../../../vg-in-place --tool=memcheck ./z13 -----(snip)----- lcbb0_cc : PASSED ppno_query : PASSED ==34390== Conditional jump or move depends on uninitialised value(s) ==34390== at 0x483C47A: bcmp (vg_replace_strmem.c:1100) ==34390== by 0x1003B1F: test_ppno_sha512_seed (z13.c:467) ==34390== by 0x1003D31: test_all_ppno (z13.c:530) ==34390== by 0x100513B: test_short_all (z13.c:598) ==34390== by 0x10051D3: main (z13.c:614) ==34390== -----(snip)----- This message occurs four times. Since I don't see a problem in the test case, maybe something is wrong with the PPNO implementation...? Also, the test case has weird formatting. I'll attach a patch for fixing that.
Created attachment 110849 [details] s390x tests: formatting fixes for z13.c
Julian, if you don't have any further comments, I think we can push this (including my cleanup). I wouldn't hold up the item just because PPNO is potentially not memcheck-clean. If necessary, we can open another Bug for that. Vadim, do you have any news regarding the PPNO issue?
Andreas, I'll look into this issue on this week. Could you check vector integer&string patch out?
Created attachment 112403 [details] PPNO fix & refactoring Changes: Fixed PPNO issue. Split z13 test into "ppno" and "lsc2" (load and store on condition 2) ones. I think this structure of tests is more natural and simplier to support. PS This patch is supposed to be applied after the previous two ones.
Andreas, I fixed the PPNO implementation so the tests are memcheck clean. I think it's ready to merge. Julian, Could you review and merge it?
(In reply to Vadim Barkov from comment #10) > Andreas, > > I fixed the PPNO implementation so the tests are memcheck clean. I think > it's ready to merge. Thanks! > Julian, > Could you review and merge it? Julian, can we merge this now?
> > Julian, > > Could you review and merge it? > > Julian, can we merge this now? Ping?
I have not checked if these patches still apply against todays git, but consider the patch acked by me.
I looked at the patches. They all seem fine to me, except for one thing I wasn't happy about: the use of the three dirty helpers to implement PPNO. In particular I don't think s390x_dirtyhelper_PPNO_sha512_load_param_block will do what you want, because it merely says memory is loaded. But it doesn't show, at the IR level, any connection betweeen the loaded values and the rest of the computation -- they are simply ignored. I assume that they are really inputs somehow. Because of this, any undefinedness in the loaded memory will be ignored by Memcheck. I understand that PPNO is difficult to implement because it has so many inputs and outputs. We have had similar problems implementing crypto instructions on Intel and ARM before now. Can you tell me briefly what PPNO does? Specifically, what inputs and what outputs (registers and memory) does it have? Then I can maybe suggest an alternative implementation.
The full PDF is at http://publibfp.dhe.ibm.com/epubs/pdf/dz9zr011.pdf look at 7-346 PERFORM RANDOM NUMBER OPERATION for a description of that instruction
in fact that was the z14 one, the z13 one is at http://publibfi.boulder.ibm.com/epubs/pdf/dz9zr010.pdf which has the "simpler" version of that instruction that only deals with pseudorandom (z14 added a true random mode)
(In reply to Julian Seward from comment #14) > Can you tell me briefly what PPNO does? Specifically, what inputs and > what outputs (registers and memory) does it have? Then I can maybe suggest > an alternative implementation. There is some pseudocode: #define gpr0 0 #define gpr1 1 // r1 and r2 are even void ppno(register r1, register r2) { addressOfParameterBlock = valueOf(gpr1) switch(valueOf(gpr0)) { case 0x0: // Query mode, ignore r1 and r2 // Save 16 bytes to addressOfParameterBlock break; case 0x3: // SHA-512 generate mode, ignore r2 value // Read 240 bytes from addressOfParameterBlock addressOfResult = valueOf(r1); resultLength = valueOf(r1 + 1); // Write resultLength bytes to addressOfResult // Overwrite some of 240 bytes starting from addressOfParameterBlock break; case 0x83: // SHA-512 seed mode, ignore r1 value // Read some of 240 bytes starting from addressOfParameterBlock argumentAddress = valueOf(r2); argumentLength = valueOf(r2 + 1); // Read argumentLength bytes from argumentAddress // Write 240 bytes to addressOfParameterBlock break; default: // Specification exception, abort execution. } }
I think this is equivalent: // getReg(RegisterNumber n) returns the value of GPR number 'n' // reg1 and reg2 are even void ppno(RegisterNumber reg1, RegisterNumber reg2) { switch(getReg(0)) { case 0x0: // Query mode, ignore reg1 and reg2 // Write 16 bytes at getReg(1) break; case 0x3: // SHA-512 generate mode, ignore reg2 // Read 240 bytes at getReg(1) // Write getReg(reg1 + 1) bytes at getReg(reg1) // Write some of 240 bytes starting at getReg(1) break; case 0x83: // SHA-512 seed mode, ignore reg1 // Read some of 240 bytes starting at getReg(1) // Read getReg(reg2 + 1) bytes at getReg(reg2) // Write 240 bytes at getReg(1) break; default: // Specification exception, abort execution. } }
(In reply to Julian Seward from comment #18) > I think this is equivalent: > > // getReg(RegisterNumber n) returns the value of GPR number 'n' > > // reg1 and reg2 are even > void ppno(RegisterNumber reg1, RegisterNumber reg2) { > > switch(getReg(0)) { > case 0x0: > // Query mode, ignore reg1 and reg2 > // Write 16 bytes at getReg(1) > break; > > case 0x3: > // SHA-512 generate mode, ignore reg2 > > // Read 240 bytes at getReg(1) > // Write getReg(reg1 + 1) bytes at getReg(reg1) > // Write some of 240 bytes starting at getReg(1) > break; > > case 0x83: > // SHA-512 seed mode, ignore reg1 > > // Read some of 240 bytes starting at getReg(1) > // Read getReg(reg2 + 1) bytes at getReg(reg2) > // Write 240 bytes at getReg(1) > break; > > default: > // Specification exception, abort execution. > } > } Yes, this is equivalent.
(In reply to Vadim Barkov from comment #19) > (In reply to Julian Seward from comment #18) > > I think this is equivalent: Unfortunately I can't think of a way to clean this up without extending the IRDirty machinery to allow two different memory-effects descriptors, as you said. That's a whole bunch of work, which I don't think anybody has the time for right now. So let's just land this as-is :-/
(In reply to Julian Seward from comment #20) > So let's just land this as-is :-/ Sounds good to me. As long as there are no false positives, this should be OK for now. And even that may not be too critical for PPNO. The patch set is certainly more important than this issue.
Hmm, the first two patches apply without problem. But the third one fails as follows. Vadim, can you please check they actually apply to the trunk, for you, and if not, update them so they do? Also please can you clarify what order they should be applied it. I guessed this, but I am not sure: bug385412_1_Non-vector_z13_support.diff bug385412_2_s390x_tests_formatting_fixes_for_z13.c bug385412_3_PPNO_fix_and_refactoring.diff _1_ and _2_ apply OK. But _3_ fails thusly: $ patch -p1 --dry-run < ../trunk/bug385412_3_PPNO_fix_and_refactoring.diff checking file .gitignore checking file VEX/priv/guest_s390_toIR.c checking file none/tests/s390x/Makefile.am checking file none/tests/s390x/ppno.c checking file none/tests/s390x/ppno.stderr.exp checking file none/tests/s390x/ppno.stdout.exp checking file none/tests/s390x/ppno.vgtest checking file none/tests/s390x/z13.c checking file none/tests/s390x/z13.stdout.exp checking file none/tests/s390x/z13.vgtest checking file .gitignore Hunk #1 FAILED at 1875. 1 out of 1 hunk FAILED checking file none/tests/s390x/Makefile.am Hunk #1 FAILED at 18. 1 out of 2 hunks FAILED checking file none/tests/s390x/lsc2.c (renamed from none/tests/s390x/z13.c) checking file none/tests/s390x/lsc2.stderr.exp (renamed from none/tests/s390x/z13.stderr.exp) checking file none/tests/s390x/lsc2.stdout.exp (renamed from none/tests/s390x/z13.stdout.exp) checking file none/tests/s390x/lsc2.vgtest checking file none/tests/s390x/z13.vgtest Not deleting file none/tests/s390x/z13.vgtest as content differs from patch checking file VEX/priv/guest_s390_defs.h checking file VEX/priv/guest_s390_helpers.c checking file VEX/priv/guest_s390_toIR.c Hunk #2 FAILED at 15871. Hunk #5 FAILED at 15944. Hunk #6 FAILED at 15954. 3 out of 6 hunks FAILED
Andreas, I've just reproduced the problem you get. I had tried to apply this patches with "--dry-run" and got the same error. But without "--dry-run" everything is okay. I am not sure why it is so and suggest to apply patches on master without "--dry-run" mode. The order of patches is correct: - Non-vector z13 support - s390x tests: formatting fixes for z13.c - PPNO fix & refactoring Please let me know if you are running any issue with them.
(In reply to Vadim Barkov from comment #23) > I've just reproduced the problem you get. I had tried to apply this patches > with "--dry-run" and got the same error. But without "--dry-run" everything > is okay. Maybe related: https://savannah.gnu.org/bugs/?25860 GNU patch - Bugs: bug #25860, Patch --dry-run fails when a patch... When a patch modifies the same file more than once and the changes conflict, patch --dry-run will fail even for patches which would apply without --dry-run.
Landed, d44563c49e55f47016e23591f708c7aa57f7a098. Andreas, could you please checkout and test, when convenient?
(In reply to Julian Seward from comment #25) > Landed, d44563c49e55f47016e23591f708c7aa57f7a098. Andreas, could > you please checkout and test, when convenient? Sure, checked out and tested. As far as I can tell, everything looks OK, so let's close this. I ran into the unrelated Bug 396839 while testing PPNO, but after fixing that, the test case ran without problems. (I'll try to provide a patch for that issue soon.) Next up would be Bug 385409 (vector integer & string z13 support).