[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 0/8] target/mips: Support R5900 GCC programs
Re: [Qemu-devel] [PATCH v5 0/8] target/mips: Support R5900 GCC programs in user mode
Thu, 20 Sep 2018 19:03:05 +0000
> 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
> 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.
Re: [Qemu-devel] [PATCH v5 6/8] target/mips: Define the R5900 CPU, Philippe Mathieu-Daudé, 2018/09/24
[Qemu-devel] [PATCH v5 5/8] target/mips: R5900 DMULT[U], DDIV[U], LL[D] and SC[D] are user only, Fredrik Noring, 2018/09/19
[Qemu-devel] [PATCH v5 7/8] linux-user/mips: Recognise the R5900 CPU model, Fredrik Noring, 2018/09/19
[Qemu-devel] [PATCH v5 8/8] elf: Toshiba/Sony rather than MIPS are the implementors of the R5900, Fredrik Noring, 2018/09/19
Re: [Qemu-devel] [PATCH v5 0/8] target/mips: Support R5900 GCC programs in user mode,
Aleksandar Markovic <=
Re: [Qemu-devel] [PATCH v5 0/8] target/mips: Support R5900 GCC programs in user mode, Philippe Mathieu-Daudé, 2018/09/28