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

[Fredrik Noring]

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.

> 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.

> 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