{"action":"created","issue":{"url":"https://api.github.com/repos/hercules-390/hyperion/issues/90","labels_url":"https://api.github.com/repos/hercules-390/hyperion/issues/90/labels{/name}","comments_url":"https://api.github.com/repos/hercules-390/hyperion/issues/90/comments","events_url":"https://api.github.com/repos/hercules-390/hyperion/issues/90/events","html_url":"https://github.com/hercules-390/hyperion/issues/90","id":127075192,"number":90,"title":"s37x dynamic module MVCLE (and possible other?) instruction issue","user":{"login":"Fish-Git","id":1486786,"avatar_url":"https://avatars.githubusercontent.com/u/1486786?v=3","gravatar_id":"","url":"https://api.github.com/users/Fish-Git","html_url":"https://github.com/Fish-Git","followers_url":"https://api.github.com/users/Fish-Git/followers","following_url":"https://api.github.com/users/Fish-Git/following{/other_user}","gists_url":"https://api.github.com/users/Fish-Git/gists{/gist_id}","starred_url":"https://api.github.com/users/Fish-Git/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Fish-Git/subscriptions","organizations_url":"https://api.github.com/users/Fish-Git/orgs","repos_url":"https://api.github.com/users/Fish-Git/repos","events_url":"https://api.github.com/users/Fish-Git/events{/privacy}","received_events_url":"https://api.github.com/users/Fish-Git/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/hercules-390/hyperion/labels/BUG","name":"BUG","color":"FF3300"},{"url":"https://api.github.com/repos/hercules-390/hyperion/labels/More%20work%20remains","name":"More work remains","color":"FF4848"}],"state":"open","locked":false,"assignee":{"login":"ivan-w","id":1625179,"avatar_url":"https://avatars.githubusercontent.com/u/1625179?v=3","gravatar_id":"","url":"https://api.github.com/users/ivan-w","html_url":"https://github.com/ivan-w","followers_url":"https://api.github.com/users/ivan-w/followers","following_url":"https://api.github.com/users/ivan-w/following{/other_user}","gists_url":"https://api.github.com/users/ivan-w/gists{/gist_id}","starred_url":"https://api.github.com/users/ivan-w/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ivan-w/subscriptions","organizations_url":"https://api.github.com/users/ivan-w/orgs","repos_url":"https://api.github.com/users/ivan-w/repos","events_url":"https://api.github.com/users/ivan-w/events{/privacy}","received_events_url":"https://api.github.com/users/ivan-w/received_events","type":"User","site_admin":false},"milestone":null,"comments":1,"created_at":"2016-01-17T05:16:31Z","updated_at":"2016-01-17T11:18:07Z","closed_at":null,"body":"#### Background\r\n\r\nAs pointed out in the `hercules 3.12 MVCLE inconsistency` thread in the main Hercules-390 group, the MVCLE instruction is currently incorrectly implemented for S/370 mode builds of Hercules.\r\n\r\nThe MVCLE instruction (opcode A8) is not officially defined as being part of the System/370 architecture.  It does not appear in any of the S/370-related Principles of Operation manuals, as do any any of the other S/390 and/or z/Arch instructions either.\r\n\r\nSince some of the newer z/Arch instructions are quite handy to use (and since there is nothing in any of the Principles of Operations manuals disallowing it), the `s37x` module was written to allow certain instructions not normally/officially defined for the S/70 architecture to be optionally supported anyway via the `ldmod s37x` command.\r\n\r\n#### The bug\r\n\r\nWhether a given instruction is supported on a given architecture or not is controlled via the opcode tables' `GEN...` macros defined in source module opcode.c.  If `GENx370x390x900` is coded for an instruction it will be supported by all three architectures. If `GENx___x390x900` is coded instead, then the instruction will only be supported for the 390 and z architectures but not for the 370 architecture. If such an instruction were attempted to be executed in 370 mode, a Program Check Interruption would result.\r\n\r\nTo allow the `s37x` loadable module to dynamically enable/disable support for certain instructions for 370 mode, a new set of `GEN...` macros were defined to indicate *optional* 370 mode support for an instruction by coding `x37X` in the `GEN...` macro instead of the normal `x370`.  For example, coding `GENx37Xx390x900` indicates the instruction is an *optional* 370 instruction which can be dynamically activated (enabled) via the `ldmod s37x` command.\r\n\r\nUnfortunately however, the opcode.c `GEN...` macro for the MVCLE (opcode A8) instruction was incorrectly coded as `GENx370x390x900` instead of `GENx37Xx390x900`, causing the instruction to always be valid in 370 mode when technically is should not be. Furthermore, support for actually *providing* the optional support for it in 370 mode was missing altogether in the `s37x` module (source file `s37x.c`).\r\n\r\nThis issue was created to address this oversight/bug and to provide a means of tracking additional work that needs to be done to the `s37x` module.\r\n\r\n#### The quick fix\r\n\r\nThe quick fix (which I will be committing shortly) is to simply code `x37X` in the opcode.c's `GEN...` macro entry for opcode A8 (MVCLE), *and* to add the missing `INST37X` macro statement to the `s37x` module's instruction table in source file `s37x.c`.\r\n\r\nWith this simple fix in place the MVCLE (opcode A8) now properly Program Checks for the normal/default 370 mode case, and properly successfully executes *only* when the `ldmod s37x` command is used to activate/enable the optional set of 370-mode instructions.\r\n\r\n#### The additional work that should also be done (IMO)\r\n\r\nWhile the previously mentioned quick fix addresses (resolves) the immediate problem, it does not resolve the following issues (which in this author's opinion still need to be (*should* be) resolved):\r\n\r\n<ol>\r\n<li>First and foremost the s37x module should really be a dyninst module and not a normal module.</li>\r\n<p>\r\nThat is to say, the module should not be manually patching the instruction tables like it currently is.  Instead, it should use our existing dyninst technique. This is important form a maintainability / quality assurance point of view. If the layout of the instruction opcode tables or the technique for dynamically updating them ever changes, there should ideally be only once place where the change needs to be made. Always used the officially supported technique instead if one is available.\r\n<p>\r\nAdditionally, since it is not currently coded  as a dyninst module as it should be, doing an \"lsmod\" after having loaded the module (ldmod s37x) does not list all of the instructions/opcodes that the module is providing like it should be like other dyninst modules do (such as dyncrypt):\r\n<p>\r\n<pre>ldmod s37x\r\n\r\nHDL: loading module s37x...\r\nHDL: module s37x loaded\r\n\r\nlsmod\r\n\r\nHDL: dll type = load, name = s37x, flags = (unloadable, not forced)\r\nHDL: dll type = load, name = hdt1052c, flags = (unloadable, not forced)\r\nHDL:  symbol = panel_command, loadcount = 2, owner = hdt1052c\r\nHDL:  devtype(s) = 3215-C 1052-C\r\nHDL: dll type = load, name = dyngui, flags = (unloadable, not forced)\r\nHDL:  symbol = panel_command, loadcount = 0, owner = dyngui\r\nHDL:  symbol = debug_cd_cmd, loadcount = 1, owner = dyngui\r\nHDL:  symbol = debug_cpu_state, loadcount = 1, owner = dyngui\r\nHDL:  symbol = daemon_task, loadcount = 1, owner = dyngui\r\nHDL:  symbol = panel_display, loadcount = 1, owner = dyngui\r\nHDL: dll type = load, name = dyncrypt, flags = (unloadable, not forced)\r\nHDL:  instruction = perform_cryptographic_key_management_operation, opcode = B928, archmode = z/Arch\r\nHDL:  instruction = perform_cryptographic_key_management_operation, opcode = B928, archmode = ESA/390\r\nHDL:  instruction = perform_cryptographic_computation, opcode = B92C, archmode = z/Arch\r\nHDL:  instruction = perform_cryptographic_computation, opcode = B92C, archmode = ESA/390\r\nHDL:  instruction = cipher_message_with_output_feedback, opcode = B92B, archmode = z/Arch\r\nHDL:  instruction = cipher_message_with_output_feedback, opcode = B92B, archmode = ESA/390\r\nHDL:  instruction = cipher_message_with_cipher_feedback, opcode = B92A, archmode = z/Arch\r\nHDL:  instruction = cipher_message_with_cipher_feedback, opcode = B92A, archmode = ESA/390\r\nHDL:  instruction = cipher_message_with_counter, opcode = B92D, archmode = z/Arch\r\nHDL:  instruction = cipher_message_with_counter, opcode = B92D, archmode = ESA/390\r\nHDL:  instruction = cipher_message_with_chaining, opcode = B92F, archmode = z/Arch\r\nHDL:  instruction = cipher_message_with_chaining, opcode = B92F, archmode = ESA/390\r\nHDL:  instruction = compute_message_authentication_code, opcode = B91E, archmode = z/Arch\r\nHDL:  instruction = compute_message_authentication_code, opcode = B91E, archmode = ESA/390\r\nHDL:  instruction = cipher_message, opcode = B92E, archmode = z/Arch\r\nHDL:  instruction = cipher_message, opcode = B92E, archmode = ESA/390\r\nHDL:  instruction = compute_last_message_digest, opcode = B93F, archmode = z/Arch\r\nHDL:  instruction = compute_last_message_digest, opcode = B93F, archmode = ESA/390\r\nHDL:  instruction = compute_intermediate_message_digest, opcode = B93E, archmode = z/Arch\r\nHDL:  instruction = compute_intermediate_message_digest, opcode = B93E, archmode = ESA/390\r\nHDL: dll type = load, name = hdteq, flags = (unloadable, not forced)\r\nHDL:  symbol = hdl_device_type_equates, loadcount = 1, owner = hdteq\r\nHDL: dll type = main, name = *Hercules, flags = (not unloadable, not forced)\r\nHDL:  symbol = replace_opcode, loadcount = 1, owner = *Hercules\r\nHDL:  symbol = panel_display, loadcount = 0, owner = *Hercules\r\nHDL:  symbol = panel_command, loadcount = 0, owner = *Hercules\r\nHDL:  symbol = parse_args, loadcount = 0, owner = *Hercules\r\nHDL:  devtype(s) = 9336 9335 9332 9313 3370 3310 0671 9345 3390 3380 3375 3350 3340 3330 2314 2311 2305</pre>\r\n<p>\r\nSo that's the first thing that will still need to be fixed.\r\n\r\n<li>The second thing that will still need to be done is an audit of the instructions / features the module is supposedly providing to ensure there aren't any other instruction(s) which are also incorrectly implemented (e.g. opcode.c's \"GEN\" macro incorrectly coded with x370 instead of x37X like it was for MVCLE and/or completely missing support in the s37x module for an instruction which it *should* be providing support for (again like it was for MVCLE).</li>\r\n<p><p>\r\nIt also would help to add a few extra comments to the #if defined(OPTION_370_EXTENSION) code block in the feat370.h header (as well as come corresponding comments in the s37x module's source file(s) a well) explaining that should support for new instructions / feature sets be added to the #if defined(OPTION_370_EXTENSION) code block in the future, that one should take care to ensure all of the required pieces are properly in place (e.g. the all important coding of \"x37X\" in 'GEN' macros in opcode.c as well as the support code to call it in the s37x dyninst module itself).\r\n<p><p>\r\n</ol>\r\n<hr>\r\n##### Assignment\r\n\r\nI am assigning this issue to you @ivan-w since the `s37x` module was written by you. If you do not wish to take ownership/responsibility for this issue then please simply change the ownership for this issue back to \"unassigned\" and we'll try to find someone else to do it. Thanks."},"comment":{"url":"https://api.github.com/repos/hercules-390/hyperion/issues/comments/172314931","html_url":"https://github.com/hercules-390/hyperion/issues/90#issuecomment-172314931","issue_url":"https://api.github.com/repos/hercules-390/hyperion/issues/90","id":172314931,"user":{"login":"jphartmann","id":1812435,"avatar_url":"https://avatars.githubusercontent.com/u/1812435?v=3","gravatar_id":"","url":"https://api.github.com/users/jphartmann","html_url":"https://github.com/jphartmann","followers_url":"https://api.github.com/users/jphartmann/followers","following_url":"https://api.github.com/users/jphartmann/following{/other_user}","gists_url":"https://api.github.com/users/jphartmann/gists{/gist_id}","starred_url":"https://api.github.com/users/jphartmann/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jphartmann/subscriptions","organizations_url":"https://api.github.com/users/jphartmann/orgs","repos_url":"https://api.github.com/users/jphartmann/repos","events_url":"https://api.github.com/users/jphartmann/events{/privacy}","received_events_url":"https://api.github.com/users/jphartmann/received_events","type":"User","site_admin":false},"created_at":"2016-01-17T11:18:07Z","updated_at":"2016-01-17T11:18:07Z","body":"Don't forget to give CLCLE (A9) the same treatment"}}