> From: Fredrik Noring <nor...@nocrew.org> > Sent: Wednesday, September 19, 2018 7:48 PM
Fredrik, first of all, many thanks for your efforts. There is a visible progress in the way you create, organize, and present the changes you devised. However, I will be mainly expressing criticism in this mail, but this should not discourage you - this is a normal part of the review process. Hope you are going to understand this in a positive, friendly way. > > Subject: [PATCH v5 0/8] target/mips: Support R5900 GCC programs in user mode The expression "GCC programs" will raise many eyebrows. What R5900 programs are not "GCC programs"? How come (as it is really implied by the title) QEMU suddenly becomes aware whether it emulates executables compiled by GCC, if its basic design/architecture principles include being agnostic on the tools used for compiling executables? Please clarify what is supported by your changes and what is not. I suspect you actually meant something slightly different than "GCC programs" or "programs compiled by GCC". > The primary purpose of this change is to support programs compiled by GCC for the R5900 target and thereby run R5900 Linux distributions, for example Gentoo. In particular, this avoids issues with cross compilation. What issues with cross compilation are avoided by your changes? How are they avoided? Are they avoided or resolved? > This change has been tested with Gentoo compiled for R5900, including native compilation of several packages under QEMU. In the preceding paragraph, you mention issues with cross compilation. Now you mention native compilation. In both cases, the circumstances are vague. Why such confusion, incompleteness, and unclarity? Can you rewrite these couple of paragraphs in a clear way, not omitting any relevant info that will prevent reader from understanding them, but at the same time not making explanations too long and complex? Before your changes are accepted, other people in the community must be able to test them. In that light, can you provide the repro procedure for the scenario with Gentoo? And also some other illustrative scenarios, not related to Gentoo. Link to toolchain used would be useful. If you have prebuilt Gentoo binaries, links to them would be good too. We must be able to test scenarios in question. > The R5900 implements the 64-bit MIPS III instruction set except DMULT, DMULTU, DDIV, DDIVU, LL, SC, LLD and SCD. The MIPS IV instructions MOVN, MOVZ and PREF are implemented. It has the R5900 specific three-operand instructions MADD, MADDU, MULT and MULTU as well as pipeline 1 versions MULT1, MULTU1, DIV1, DIVU1, MADD1, MADDU1, MFHI1, MFLO1, MTHI1 and MTLO1. A set of 93 128-bit multimedia instructions specific to the R5900 is also implemented. > The Toshiba TX System RISC TX79 Core Architecture manual describes the R5900 processor: > http://www.lukasz.dk/files/tx79architecture.pdf > Changes in v5: > - Reorder check_insn_opc_user_only calls > - Call check_insn_opc_removed in check_insn_opc_user_only You should include history for v2, v3, and v4 as well. Patch 4 will break bisect on clang builds. The reason for this is that clang treats unused functions as errors. Therefore, patch 4 must be merged with some of subsequent patches that contain first invocation of the function currently defined in patch 4. I know this is in some way illogical, but not breaking the bisect takes precedence. For all patches you should review commit messages, and rewrite some of them so that they are clear and right on the money. The same for code comments. In one instance, the code comment is more complicated than the code itself. Don't forget to run checkpatch.pl on all your patches. Sincerely, Aleksandar
