)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":1001661,"name":"Daniel Goehring","email":"dgoehrin@os.amperecomputing.com","username":"dgoehrin"},"change_message_id":"1d0c17d2d5f9ef5c0dcbbc913cb8e52f63d12537","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":3,"id":"0728e3d7_b1661ec7","updated":"2025-06-07 03:11:54.000000000","message":"Thanks for the review. Please see comment responses.","commit_id":"9a2ef9a80a2754999f0499701c73ab24817b724c"},{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"0e57300d4ea4fb07e9d1f5e10601bb9f6cc80f84","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"a814e791_354ec54a","in_reply_to":"0728e3d7_b1661ec7","updated":"2025-06-07 07:59:27.000000000","message":"Thanks!","commit_id":"9a2ef9a80a2754999f0499701c73ab24817b724c"}],"src/target/armv8.c":[{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"09c690672a39f01ed001b0478a95e35ed2db29b8","unresolved":true,"context_lines":[{"line_number":647,"context_line":"\t\t\treturn retval;"},{"line_number":648,"context_line":"\t}"},{"line_number":649,"context_line":""},{"line_number":650,"context_line":"\tretval \u003d dpm-\u003einstr_read_data_r0_64(dpm, ARMV8_MRS(SYSTEM_MPIDR, 0), \u0026mpidr);"},{"line_number":651,"context_line":"\tif (retval !\u003d ERROR_OK)"},{"line_number":652,"context_line":"\t\tgoto done;"},{"line_number":653,"context_line":"\tif (mpidr \u0026 1U\u003c\u003c31) {"}],"source_content_type":"text/x-csrc","patch_set":1,"id":"a11364a8_f0399e66","line":650,"updated":"2025-06-04 21:50:56.000000000","message":"I think this will be an illegal instruction on systems that run EL1 in AArch32 execution state.\nLooking at the implementation of `dpmv8_instr_read_data_r0()` and `dpmv8_instr_read_data_r0_64()`, it should work if you use\n```\nuint64_t mpidr;\nretval \u003d dpm-\u003einstr_read_data_r0_64(dpm, armv8_opcode(armv8, READ_REG_MPIDR), \u0026mpidr);\n```\n\nNevertheless, I think there is a bug in case of core halted in EL0 AArch32 while EL1 is AArch64.\nThe modeswitch above moves to EL1, but there is no re-evaluation of `dpm-\u003earm-\u003ecore_state`, so this code considers the system still in AArch32 while it is already AArch64!\nThe read of MPIDR would select the instruction based on the old `core_state`!\nWould you mind adding a comment `/* TODO: bla bla bla */` before this line?","commit_id":"65a40804815ebce902ff3d34736ee787f3c2619e"},{"author":{"_account_id":1001661,"name":"Daniel Goehring","email":"dgoehrin@os.amperecomputing.com","username":"dgoehrin"},"change_message_id":"68bf39b8ddab8bed42596e577a338e51e6055177","unresolved":true,"context_lines":[{"line_number":647,"context_line":"\t\t\treturn retval;"},{"line_number":648,"context_line":"\t}"},{"line_number":649,"context_line":""},{"line_number":650,"context_line":"\tretval \u003d dpm-\u003einstr_read_data_r0_64(dpm, ARMV8_MRS(SYSTEM_MPIDR, 0), \u0026mpidr);"},{"line_number":651,"context_line":"\tif (retval !\u003d ERROR_OK)"},{"line_number":652,"context_line":"\t\tgoto done;"},{"line_number":653,"context_line":"\tif (mpidr \u0026 1U\u003c\u003c31) {"}],"source_content_type":"text/x-csrc","patch_set":1,"id":"be99f92d_6f781791","line":650,"in_reply_to":"11c65500_a0c8e854","updated":"2025-06-07 03:24:37.000000000","message":"The patch prints the socket, cluster, core, and thread identifiers in hexadecimal. Let me know if you prefer decimal instead.","commit_id":"65a40804815ebce902ff3d34736ee787f3c2619e"},{"author":{"_account_id":1001661,"name":"Daniel Goehring","email":"dgoehrin@os.amperecomputing.com","username":"dgoehrin"},"change_message_id":"11da2b614fe0f658a170c9176b27a66657c9abae","unresolved":false,"context_lines":[{"line_number":647,"context_line":"\t\t\treturn retval;"},{"line_number":648,"context_line":"\t}"},{"line_number":649,"context_line":""},{"line_number":650,"context_line":"\tretval \u003d dpm-\u003einstr_read_data_r0_64(dpm, ARMV8_MRS(SYSTEM_MPIDR, 0), \u0026mpidr);"},{"line_number":651,"context_line":"\tif (retval !\u003d ERROR_OK)"},{"line_number":652,"context_line":"\t\tgoto done;"},{"line_number":653,"context_line":"\tif (mpidr \u0026 1U\u003c\u003c31) {"}],"source_content_type":"text/x-csrc","patch_set":1,"id":"5f102c6b_0812726d","line":650,"in_reply_to":"1898246a_3147510b","updated":"2025-06-10 02:34:19.000000000","message":"If both are ok, I\u0027ll go with decimal.","commit_id":"65a40804815ebce902ff3d34736ee787f3c2619e"},{"author":{"_account_id":1001661,"name":"Daniel Goehring","email":"dgoehrin@os.amperecomputing.com","username":"dgoehrin"},"change_message_id":"1d0c17d2d5f9ef5c0dcbbc913cb8e52f63d12537","unresolved":true,"context_lines":[{"line_number":647,"context_line":"\t\t\treturn retval;"},{"line_number":648,"context_line":"\t}"},{"line_number":649,"context_line":""},{"line_number":650,"context_line":"\tretval \u003d dpm-\u003einstr_read_data_r0_64(dpm, ARMV8_MRS(SYSTEM_MPIDR, 0), \u0026mpidr);"},{"line_number":651,"context_line":"\tif (retval !\u003d ERROR_OK)"},{"line_number":652,"context_line":"\t\tgoto done;"},{"line_number":653,"context_line":"\tif (mpidr \u0026 1U\u003c\u003c31) {"}],"source_content_type":"text/x-csrc","patch_set":1,"id":"11c65500_a0c8e854","line":650,"in_reply_to":"a11364a8_f0399e66","updated":"2025-06-07 03:11:54.000000000","message":"Yes, good point about the illegal instruction wrt AArch32 execution state. The updated code should work in both AArch32 and AArch64 execution states for Armv8.\n\nI tested the updated patch with both MT\u003d0 and MT\u003d1 paths on systems in the AArch64 execution state. I don\u0027t have easy access to a system with AArch32 execution state support, so that code path is untested for both MT\u003d0 and MT\u003d1.\n\nI agree with your analysis of the code bug. I added a comment to document it.","commit_id":"65a40804815ebce902ff3d34736ee787f3c2619e"},{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"0e57300d4ea4fb07e9d1f5e10601bb9f6cc80f84","unresolved":false,"context_lines":[{"line_number":647,"context_line":"\t\t\treturn retval;"},{"line_number":648,"context_line":"\t}"},{"line_number":649,"context_line":""},{"line_number":650,"context_line":"\tretval \u003d dpm-\u003einstr_read_data_r0_64(dpm, ARMV8_MRS(SYSTEM_MPIDR, 0), \u0026mpidr);"},{"line_number":651,"context_line":"\tif (retval !\u003d ERROR_OK)"},{"line_number":652,"context_line":"\t\tgoto done;"},{"line_number":653,"context_line":"\tif (mpidr \u0026 1U\u003c\u003c31) {"}],"source_content_type":"text/x-csrc","patch_set":1,"id":"1898246a_3147510b","line":650,"in_reply_to":"be99f92d_6f781791","updated":"2025-06-07 07:59:27.000000000","message":"This is informative only. Hex or dec are both ok, the important is having 0x in front of hex values to avoid misunderstanding.","commit_id":"65a40804815ebce902ff3d34736ee787f3c2619e"},{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"0e57300d4ea4fb07e9d1f5e10601bb9f6cc80f84","unresolved":true,"context_lines":[{"line_number":900,"context_line":"\t/* to EL1, but there is no re-evaluation of dpm-\u003earm-\u003ecore_state. As a result, while the core */"},{"line_number":901,"context_line":"\t/* is in AArch64, the code considers the system still in AArch32. The read of MPIDR would */"},{"line_number":902,"context_line":"\t/* select the instruction based on the old core_state. The call to \u0027armv8_dpm_get_core_state()\u0027 */"},{"line_number":903,"context_line":"\t/* below could also potentially return the incorrect execution state for the current EL. */"},{"line_number":904,"context_line":""},{"line_number":905,"context_line":"\t/* check if we\u0027re in an unprivileged mode */"},{"line_number":906,"context_line":"\tif (armv8_curel_from_core_mode(arm-\u003ecore_mode) \u003c SYSTEM_CUREL_EL1) {"}],"source_content_type":"text/x-csrc","patch_set":3,"id":"2ebfdf3e_d7222951","line":903,"updated":"2025-06-07 07:59:27.000000000","message":"The coding style requires the multiline comments to be inside a single `/* ... */` block.\nBut I would accept it as is because I would like to address this TODO asap, even during the code freeze, as it is a bug.","commit_id":"9a2ef9a80a2754999f0499701c73ab24817b724c"},{"author":{"_account_id":1001661,"name":"Daniel Goehring","email":"dgoehrin@os.amperecomputing.com","username":"dgoehrin"},"change_message_id":"11da2b614fe0f658a170c9176b27a66657c9abae","unresolved":true,"context_lines":[{"line_number":900,"context_line":"\t/* to EL1, but there is no re-evaluation of dpm-\u003earm-\u003ecore_state. As a result, while the core */"},{"line_number":901,"context_line":"\t/* is in AArch64, the code considers the system still in AArch32. The read of MPIDR would */"},{"line_number":902,"context_line":"\t/* select the instruction based on the old core_state. The call to \u0027armv8_dpm_get_core_state()\u0027 */"},{"line_number":903,"context_line":"\t/* below could also potentially return the incorrect execution state for the current EL. */"},{"line_number":904,"context_line":""},{"line_number":905,"context_line":"\t/* check if we\u0027re in an unprivileged mode */"},{"line_number":906,"context_line":"\tif (armv8_curel_from_core_mode(arm-\u003ecore_mode) \u003c SYSTEM_CUREL_EL1) {"}],"source_content_type":"text/x-csrc","patch_set":3,"id":"ab07a8f4_7313366b","line":903,"in_reply_to":"2ebfdf3e_d7222951","updated":"2025-06-10 02:34:19.000000000","message":"Fixed.","commit_id":"9a2ef9a80a2754999f0499701c73ab24817b724c"},{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"8bdd06e8ebd97164b272f4a56c1d6d9195e22fab","unresolved":false,"context_lines":[{"line_number":900,"context_line":"\t/* to EL1, but there is no re-evaluation of dpm-\u003earm-\u003ecore_state. As a result, while the core */"},{"line_number":901,"context_line":"\t/* is in AArch64, the code considers the system still in AArch32. The read of MPIDR would */"},{"line_number":902,"context_line":"\t/* select the instruction based on the old core_state. The call to \u0027armv8_dpm_get_core_state()\u0027 */"},{"line_number":903,"context_line":"\t/* below could also potentially return the incorrect execution state for the current EL. */"},{"line_number":904,"context_line":""},{"line_number":905,"context_line":"\t/* check if we\u0027re in an unprivileged mode */"},{"line_number":906,"context_line":"\tif (armv8_curel_from_core_mode(arm-\u003ecore_mode) \u003c SYSTEM_CUREL_EL1) {"}],"source_content_type":"text/x-csrc","patch_set":3,"id":"8beca576_c046b9a3","line":903,"in_reply_to":"ab07a8f4_7313366b","updated":"2025-06-10 08:21:41.000000000","message":"Thanks!","commit_id":"9a2ef9a80a2754999f0499701c73ab24817b724c"}],"src/target/armv8.h":[{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"09c690672a39f01ed001b0478a95e35ed2db29b8","unresolved":true,"context_lines":[{"line_number":192,"context_line":""},{"line_number":193,"context_line":"\t/* mdir */"},{"line_number":194,"context_line":"\tuint8_t multi_processor_system;"},{"line_number":195,"context_line":"\tuint8_t aff3;"},{"line_number":196,"context_line":"\tuint8_t aff2;"},{"line_number":197,"context_line":"\tuint8_t aff1;"},{"line_number":198,"context_line":"\tuint8_t aff0;"}],"source_content_type":"text/x-csrc","patch_set":1,"id":"fa41ef98_5db1188b","line":195,"updated":"2025-06-04 21:50:56.000000000","message":"I\u0027m questioning myself if we really need to keep all of these info. We do not reuse them in the rest of the code.\nProbably keeping only `uint64_t mpidr;` is enough.\nAnd the incorrect comment above `/* mdir */` could be also dropped.","commit_id":"65a40804815ebce902ff3d34736ee787f3c2619e"},{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"0e57300d4ea4fb07e9d1f5e10601bb9f6cc80f84","unresolved":false,"context_lines":[{"line_number":192,"context_line":""},{"line_number":193,"context_line":"\t/* mdir */"},{"line_number":194,"context_line":"\tuint8_t multi_processor_system;"},{"line_number":195,"context_line":"\tuint8_t aff3;"},{"line_number":196,"context_line":"\tuint8_t aff2;"},{"line_number":197,"context_line":"\tuint8_t aff1;"},{"line_number":198,"context_line":"\tuint8_t aff0;"}],"source_content_type":"text/x-csrc","patch_set":1,"id":"e33a6f52_17c08b24","line":195,"in_reply_to":"a1894e72_591d0837","updated":"2025-06-07 07:59:27.000000000","message":"That\u0027s fine for me too.\nIt would be easy to add it again, if we need it.","commit_id":"65a40804815ebce902ff3d34736ee787f3c2619e"},{"author":{"_account_id":1001661,"name":"Daniel Goehring","email":"dgoehrin@os.amperecomputing.com","username":"dgoehrin"},"change_message_id":"1d0c17d2d5f9ef5c0dcbbc913cb8e52f63d12537","unresolved":true,"context_lines":[{"line_number":192,"context_line":""},{"line_number":193,"context_line":"\t/* mdir */"},{"line_number":194,"context_line":"\tuint8_t multi_processor_system;"},{"line_number":195,"context_line":"\tuint8_t aff3;"},{"line_number":196,"context_line":"\tuint8_t aff2;"},{"line_number":197,"context_line":"\tuint8_t aff1;"},{"line_number":198,"context_line":"\tuint8_t aff0;"}],"source_content_type":"text/x-csrc","patch_set":1,"id":"a1894e72_591d0837","line":195,"in_reply_to":"fa41ef98_5db1188b","updated":"2025-06-07 03:11:54.000000000","message":"Since the structure fields aren\u0027t used in the rest of the code, I removed them from the structure definition, including the \u0027uint64_t mpidr;\u0027 statement. Let me know if you want me to add the \u0027mpidr\u0027 declaration back into the structure. I also removed the \u0027/* mdir */\u0027 comment.","commit_id":"65a40804815ebce902ff3d34736ee787f3c2619e"}]}