[Top][All Lists]

[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

From: Philippe Mathieu-Daudé
Subject: Re: [Qemu-devel] [PATCH v5 0/8] target/mips: Support R5900 GCC programs in user mode
Date: Mon, 24 Sep 2018 02:27:46 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0

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:

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]