On 9/23/18 8:38 PM, Fredrik Noring wrote: > Hi Aleksandar, > > Thank your for your review comments. Please find clarifications and > questions below: > >>> 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"? > > That would be programs not compiled by GCC, as explained in the first > sentence of the body text. The subject line is very brief by necessity > since it is limited to 72 or so characters. It was an attempt to qualify > the subject line "initial support for MIPS R5900", which you previously > rejected. I agree that there are probably better wordings.
What about "linux-user support for MIPS R5900"? >> 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? > > Well, no, your implication and conclusion are invalid because the commit > message does not say anything about programs that are not compiled by GCC. > Nevertheless, it may be helpful to add such a statement. Perhaps that was > your point? > >> 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 main use case and motivation for this change is to run R5900 Linux > distributions compiled by GCC. Other use cases, such as running programs > that contain unsupported machine code, or using the FPU in system mode, > are secondary. As Maciej noted, it is probably wise to assert exceptions > in unsupported cases. > > GCC, in its current version, by itself, by inspection of the GCC source > code and inspection of the generated machine code, for the R5900 target, > only emits two instructions that are specific to the R5900: the three- > operand MULT and MULTU. GCC and libc also emit certain MIPS III and IV > instructions that are not implemented in R5900 hardware. Those are normally > trapped and emulated by the Linux kernel, and therefore need to be treated > accordingly by QEMU. This is addressed, in turn, by the patch series. > > "Programs compiled by GCC" was taken to mean source code compiled by GCC > under the restrictions above. One can, even with the apparent limitations, > with a bit of effort obtain a fully functioning operating system such as > R5900 Gentoo. Strictly speaking, programs need not be compiled by GCC under > these restrictions, although GCC is very much the target of this change due > to its practical importance. > >>> 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? > > Problems with cross-compilation may be related to host and target > differences in integer sizes, pointer sizes, machine code, ABI, etc. > Sometimes cross-compilation is not even supported by the build script for > a given package. One effective way to sidestep ("avoid") these problems > is to replace the cross-compiler with a native compiler. This change of > methods does not "resolve" the inherent problems with cross-compilation. > >>> 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? > > Well, the native compiler naturally replaces the cross-compiler, because > one typically uses one or the other, and preferably the native compiler when > circumstances admit this. The native compiler is also a rather good test > case for the R5900 QEMU user mode. Additionally, Gentoo is well-known for > compiling its packages from sources. > >> 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? > > I will make a try for v6 of the patch series. > >> 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? > > I used the Gentoo sys-devel/crossdev package > > https://wiki.gentoo.org/wiki/Crossdev > > with a couple of patches mainly to simplify the handling of LL/SC and > floating point support, avoiding complications with additional configure and > compiler flags. I plan to submit these patches, in some form, for GAS and > GCC inclusion. However, building Gentoo for R5900 is quite a bit more than > the necessities for R5900 QEMU user mode testing purposes. We could manage to build a docker mips-r5900 cross-compiler image, such: https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg06908.html But we'll need your patches. >> 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. > > I think Busybox > > https://busybox.net/ > > could serve as a simplified test. I can recommend it if you have not tried > it. Buildroot > > https://buildroot.org/ > > seems to be popular although I have not used it myself. > >>> 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. > > Sure. > >> 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. > > Right. GCC accepts it since static inline functions are exempted. > >> 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. > > To be clear, would you mind indicating which single instance you are > referring to? > >> Don't forget to run checkpatch.pl on all your patches. > > Sure. > > Fredrik >