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

[Philippe Mathieu-Daudé]

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
>