Re: [Qemu-devel] [PATCH v5 0/8] target/mips: Support R5900 GCC programs in user mode

[Aleksandar Markovic]

> From: Fredrik Noring <address@hidden>
> 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