IBM has published the latest ISA 3.1. This bugzilla will be used to track adding the new ISA 3.1 support to Valgrind. There are approximately 244 new instructions in ISA 3.1 that need to be supported in Valgrind.
Posted first 4 of about 36 patches for review.
Created attachment 130173 [details] Add check for isa 3.1 support 0001-Add-check-for-isa-3.1-support.patch Adds base support for detecting ISA 3.1
Created attachment 130174 [details] Adds the support for prefix instructions 0002-ISA-3.1-Instruction-Prefix-Support.patch Adds the support for prefix instructions
Created attachment 130175 [details] Add prefixed support for the following word 0003-ISA-3.1-Add-prefixed-support-for-the-following-word-.patch Add prefixed support for new load and store instructions
Created attachment 130176 [details] Initial load and store instruction tests 0004-Initial-ISA-3.1-instruction-tests.patch Testsuite for new instructions
This bugzilla covers the first four patches of the ISA 3.1 support for PPC.
(In reply to Carl Love from comment #2) > Created attachment 130173 [details] > Add check for isa 3.1 support > > 0001-Add-check-for-isa-3.1-support.patch > > Adds base support for detecting ISA 3.1 Looks OK to me. No comments.
(In reply to Carl Love from comment #3) > Created attachment 130174 [details] > Adds the support for prefix instructions > > 0002-ISA-3.1-Instruction-Prefix-Support.patch > > Adds the support for prefix instructions static Addr64 nextInsnAddr( void ) { - return guest_CIA_curr_instr + 4; + return guest_CIA_curr_instr + WORD_INST_SIZE; } Is this correct? What if this insn is an 8-byte insn? ---- +#define ENABLE_PREFIX_CHECK 1 + +#if ENABLE_PREFIX_CHECK +#define PREFIX_CHECK { vassert( !prefix_instruction( prefixInstr ) ); } +#else +#define PREFIX_CHECK { } +#endif and then used (eg) in: +static Bool dis_int_mult_add ( UInt prefixInstr, UInt theInstr ) { /* VA-Form */ UChar rD_addr = ifieldRegDS( theInstr ); @@ -5131,6 +5188,9 @@ static Bool dis_int_mult_add ( UInt theInstr ) assign( rB, getIReg( rB_addr ) ); assign( rC, getIReg( rC_addr ) ); + /* There is no prefixed version of these instructions. */ + PREFIX_CHECK I'm not sure whether you're intending to use PREFIX_CHECK just for debugging the implementation (meaning, it will fail if there are implementation bugs, but can't be made to fail regardless of the input) or whether it will fail if there is some kind of invalid input. The first use is OK, but for the second use we need to generate SIGILL instead of asserting. Can you say which it is?
(In reply to Carl Love from comment #3) > Created attachment 130174 [details] > Adds the support for prefix instructions > > 0002-ISA-3.1-Instruction-Prefix-Support.patch > > Adds the support for prefix instructions Oops, missed a comment for this patch: @@ -28401,11 +28764,47 @@ static UInt get_VSX60_opc2(UInt opc2_full, UInt theInstr) +static Bool dis_nop_prefix ( UInt prefixInstr, UInt theInstr ) +{ ... + return PREFIX_NOP_INVALID; This seems not-correct from a types point of view. dis_nop_prefix is declared to return a Bool, but PREFIX_NOP_INVALID is -1. I imagine this only compiles because of how lame C's type system is. Can you fix it so that Bool values are only True or False? @@ -28598,6 +29000,35 @@ DisResult disInstr_PPC_WRK ( + int ret; + ret = dis_nop_prefix( prefixInstr, theInstr); + if (ret == True) + goto decode_success; + else if (ret == PREFIX_NOP_INVALID) + goto decode_failure; Related problem here; pls fix (`ret` being sometimes True and sometimes PREFIX_NOP_INVALID given that they have different types)
(In reply to Carl Love from comment #4) > Created attachment 130175 [details] > Add prefixed support for the following word > > 0003-ISA-3.1-Add-prefixed-support-for-the-following-word-.patch > > Add prefixed support for new load and store instructions Looks OK. No comments.
(In reply to Carl Love from comment #5) > Created attachment 130176 [details] > Initial load and store instruction tests > > 0004-Initial-ISA-3.1-instruction-tests.patch > > Testsuite for new instructions diff --git a/none/tests/ppc64/isa_3_1_helpers.h b/none/tests/ppc64/isa_3_1_helpers.h new file mode 100644 index 000000000..d6f2f9878 --- /dev/null +++ b/none/tests/ppc64/isa_3_1_helpers.h This seems like a lot of code (functions) to put in a .h file. Should this stuff be in a .c file instead? ---- new files: none/tests/ppc64/test_isa_3_1_RT.c none/tests/ppc64/test_isa_3_1_XT.c none/tests/ppc64/test_isa_3_1_RT.c none/tests/ppc64/test_isa_3_1_XT.c These are GPL 2-only licensed. I'd prefer that they are licensed "2 or any later version" since that is what the project requests. See the top level README file.
(In reply to Julian Seward from comment #11) > (In reply to Carl Love from comment #5) > > Created attachment 130176 [details] > > Initial load and store instruction tests > > > > 0004-Initial-ISA-3.1-instruction-tests.patch > > > > Testsuite for new instructions > > diff --git a/none/tests/ppc64/isa_3_1_helpers.h > b/none/tests/ppc64/isa_3_1_helpers.h > new file mode 100644 > index 000000000..d6f2f9878 > --- /dev/null > +++ b/none/tests/ppc64/isa_3_1_helpers.h > > This seems like a lot of code (functions) to put in a .h file. Should this > stuff be in a .c > file instead? Probably yes. I'm prettu sure we have a similar scenario with the existing ppc64_helpers.h ; i'll do a lookover and see if that can be fixed up too. > > ---- > > new files: > none/tests/ppc64/test_isa_3_1_RT.c > none/tests/ppc64/test_isa_3_1_XT.c > none/tests/ppc64/test_isa_3_1_RT.c > none/tests/ppc64/test_isa_3_1_XT.c > > These are GPL 2-only licensed. I'd prefer that they are licensed "2 or any > later version" since > that is what the project requests. See the top level README file. That shouldn't be a problem. I believe I started with one of the existing tests for those, so we probably have something else existing with the GPL 2 license. I'll poke around a bit and see if I can confirm the origins of that one too. Thanks for the review.
Created attachment 130630 [details] Add check for isa 3.1 support Updated the patch per Julian's comments
Created attachment 130631 [details] Instruction prefix support Updated the patch per Julian's comments
Created attachment 130632 [details] Add prefixed support for the following word Updated the patch
I have uploaded the latest version of the three Valgrind ISA 3.1 support patches. A small error was found in the patch for "Add check for isa 3.1 support". The original line in coregrind/m_initimg/initimg-linux.c, setup_client_stack() was + /* ISA 3.1 */ + auxv_3_1 = (auxv->u.a_val & 0x01000000ULL) == 0x01000000ULL; + hw_caps_3_1 = (vex_archinfo->hwcaps & VEX_HWCAPS_PPC64_ISA3_1) + == VEX_HWCAPS_PPC64_ISA3_1; the hex values were changed to: + /* ISA 3.1 */ + auxv_3_1 = (auxv->u.a_val & 0x00040000ULL) == 0x00040000ULL; + hw_caps_3_1 = (vex_archinfo->hwcaps & VEX_HWCAPS_PPC64_ISA3_1) + == VEX_HWCAPS_PPC64_ISA3_1; The following are the fixes for the second patch, "Instruction prefix support" to address Julian's comments > 0002-ISA-3.1-Instruction-Prefix-Support.patch > > Adds the support for prefix instructions static Addr64 nextInsnAddr( void ) { - return guest_CIA_curr_instr + 4; + return guest_CIA_curr_instr + WORD_INST_SIZE; } Is this correct? What if this insn is an 8-byte insn? Yes it is correct. Added comment for clarity: static Addr64 nextInsnAddr( void ) { + /* Note in the case of a prefix instruction, delta has already been + incremented by WORD_INST_SIZE to move past the prefix part of the + instruction. So only need to increment by WORD_INST_SIZE to get to + the start of the next instruction. */ return guest_CIA_curr_instr + WORD_INST_SIZE; } The prefix check macro check +#define ENABLE_PREFIX_CHECK 1 + +#if ENABLE_PREFIX_CHECK +#define PREFIX_CHECK { vassert( !prefix_instruction( prefixInstr ) ); } +#else +#define PREFIX_CHECK { } +#endif I'm not sure whether you're intending to use PREFIX_CHECK just for debugging the implementation (meaning, it will fail if there are implementation bugs, but can't be made to fail regardless of the input) or whether it will fail if there is some kind of invalid input. The first use is OK, but for the second use we need to generate SIGILL instead of asserting. Can you say which it is? It is just a development check to make sure the instruction parsing is correct. There are no instruction inputs that would cause this to fail. I added the following comment for clarification: +#define ISA_3_1_PREFIX_CHECK if (prefixInstr) {if (!allow_isa_3_1) goto decode_noIsa3_1;} + +/* ENABLE_PREFIX_CHECK is for development purposes. Turn off for production + releases */ #define ENABLE_PREFIX_CHECK 1 I went ahead and turned it off for the commit. I will turn it on and off again as we do each set of patches. The issue with dis_nop_prefix: +static Bool dis_nop_prefix ( UInt prefixInstr, UInt theInstr ) +{ ... + return PREFIX_NOP_INVALID; This seems not-correct from a types point of view. dis_nop_prefix is declared to return a Bool, but PREFIX_NOP_INVALID is -1. I imagine this only compiles because of how lame C's type system is. Can you fix it so that Bool values are only True or False? @@ -28598,6 +29000,35 @@ DisResult disInstr_PPC_WRK ( + int ret; + ret = dis_nop_prefix( prefixInstr, theInstr); + if (ret == True) + goto decode_success; + else if (ret == PREFIX_NOP_INVALID) + goto decode_failure; Related problem here; pls fix (`ret` being sometimes True and sometimes PREFIX_NOP_INVALID given that they have different types) The function needs to return type Int. We need to know True, False or invalid. Changed the code to: static Int dis_nop_prefix ( UInt prefixInstr, UInt theInstr ) { ... }
Created attachment 130689 [details] Test suite foundation The testsuite patch is being reposted as a series of two patches. 1) Test suite foundation patch, adds all the needed infrastructure for doing the tests. 2) Load store test suite, adds tests for the Valgrind functionality added in the patch "Add prefiexed support for the following word..."
Created attachment 130690 [details] Testsuite load store word instructions Add tests for the load store word instructions.
The testsuite has been updated to address the concerns with the previous patch. The C code that was in the isa_3_1_helpers.h file has been put into a new file test_isa_3_1_common.c. The license for the test files was changed to GPL 2 or later.
Created bugzilla for second set of patches. https://bugs.kde.org/show_bug.cgi?id=425232
Created attachment 131014 [details] Test suite foundation Updating the Test suite foundation patch. Previous file had a couple of files, test_isa_3_1_RT.val-out and test_isa_3_1_XT.val-out,in the patch that were not supposed to be in the patch. These were outputs from testing valgrind to verify the results.
(In reply to Carl Love from comment #18) > Created attachment 130690 [details] > Testsuite load store word instructions Ok to land.
(In reply to Carl Love from comment #21) > Created attachment 131014 [details] > Test suite foundation ok to land.
(In reply to Carl Love from comment #15) > Created attachment 130632 [details] > Add prefixed support for the following word ok to land but please at least consider the following: > +/* Prefix instsruction effective address calc: (rA + simm) */ typo: instruction > + /* Get the EA */ > + if ( *R == 0 ) > + return mkexpr ( tmp ); At least in a simplistic reading of this function, it looks like *R is read on all paths, but only written on the `prefix` path. So is it defined before the call, in that case?
(In reply to Carl Love from comment #14) > Created attachment 130631 [details] > Instruction prefix support ok to land provided at least that the dis_nop_prefix return type is fixed properly. > + int ret; > + ret = dis_nop_prefix( prefixInstr, theInstr); > + if (ret == True) > + goto decode_success; > + else if (ret == PREFIX_NOP_INVALID) > + goto decode_failure; > > Related problem here; pls fix (`ret` being sometimes True and sometimes > PREFIX_NOP_INVALID given > that they have different types) > > The function needs to return type Int. We need to know True, > False or invalid. Changed the code to: > > static Int dis_nop_prefix ( UInt prefixInstr, UInt theInstr ) > { > ... > } We shouldn't be mixing up integral types like this. Please use a custom enum instead: typedef enum { Invalid, No, Yes } PrefixStatus; Some other comments to consider: > +/*-----------------------------------------------------------*/ > +/*--- Prefix instruction helpers ---*/ > +/*-----------------------------------------------------------*/ > +/* ENABLE_PREFIX_CHECK is for development purposes. Turn off for production > + releases to improve performance. */ > +#define ENABLE_PREFIX_CHECK 0 It won't make much perf difference in practice -- the insn decoders are not a hot path. If it would generate better test coverage, and if it will *only* fail if there's a bug in the decode logic (as opposed to an unknown insn), feel free to turn it on in production. > -static Bool dis_int_mult_add ( UInt theInstr ) > +static Bool dis_int_mult_add ( UInt prefixInstr, UInt theInstr ) Nit: isn't it a big misleading to call the first 32 bits `prefixInstr`? Really, it's just *part* of the instruction. Would you consider calling it instead just `prefix` ? > + prefixInstr = 0; /* Reset the prefix so instruction flag */ Comment doesn't make sense; are there words missing? > + A prefix instruction basically consists of a 4-byte pre-emble followed nit: "preamble"
(In reply to Carl Love from comment #13) > Created attachment 130630 [details] > Add check for isa 3.1 support Ok to land.
comment 25 1) return type for dis_nop_prefix - as requested changed to using an enum for the return value. 2) #define ENABLE_PREFIX_CHECK - This was added as an extra check during development. It was used to ensure that the parsing called the correct function with the code rearrangement. Have considered removing entirely once all of the support is added. Has no
oops effect on determining an unknown instruction. 3) Changed prefixInst to prefix and fixed up one conflict due to the name change. 4) Fixed comment and spelling error. Will update patch once I get thru all the patch changes.
comment 24 There is a comment in the caller of function calculate_prefix_EA() that R must be set to 0 for word instruction. But that is a bit dangerous. So, made sure *R gets set to 0 in the function for word instructions. Fixed the spelling error.
Created attachment 131848 [details] Add check for isa 3.1 support
Created attachment 131849 [details] Instruction prefix support
Created attachment 131850 [details] add isa 3.1 support for instructiions to guest_ppc_toIR.c
Created attachment 131851 [details] Test suite foundation
Created attachment 131852 [details] Testsuite load store word instructions
Updated all of the patches to make sure everything is consistent. All of the requested changes were made. Reran the regression test. No regressions found. Will commit the attached patches.
Patches committedcommit 336b8c514dbfc177283ee71b803b67f5145e8bf7 (HEAD -> master, origin/master, origin/HEAD) Author: Carl Love <cel@us.ibm.com> Date: Mon Sep 21 15:57:26 2020 -0500 Prefixed load-store support commit 2a88a98f5b69ac7cdd06e682b2158fd8a31399c9 Author: Carl Love <cel@us.ibm.com> Date: Mon Sep 21 15:56:22 2020 -0500 valgrind isa 3.1 foundation header files and other common parts associated with the initial isa v3.1 support commit 459d52ec1f744006dea8d695ea3e6968b04bff22 Author: Carl Love <carll@us.ibm.com> Date: Tue Jul 28 13:17:18 2020 -0500 Add prefixed support for the following word instructions. addi Add Immediate lbz Load Byte & Zero ld Load Doubleword lfd Load Floating Double lfs Load Floating Single lha Load Halfword Algebraic lhz Load Halfword & Zero lq Load Quadword lwa Load Word Algebraic lwz Load Word & Zero lxsd Load VSX Scalar Doubleword lxssp Load VSX Scalar Single-Precision lxv Load VSX Vector stb Store Byte std Store Doubleword stfd Store Floating Double stfs Store Floating Single sth Store Halfword stq Store Quadword stw Store Word stxsd Store VSX Scalar Doubleword stxssp Store VSX Scalar Single-Precision stxv Store VSX Vector commit ad293d5168c7966329681da62a952183b61d5a05 Author: Carl Love <cel@us.ibm.com> Date: Wed May 6 15:27:32 2020 -0500 Instruction Prefix Support commit ebcf62b047843bfb0a8c9141b7e272f693c3df28 Author: Carl Love <cel@us.ibm.com> Date: Wed May 6 15:13:42 2020 -0500 Add check for isa 3.1 support Closing this bug