-
Notifications
You must be signed in to change notification settings - Fork 385
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support to use qemu-system to run linux aarch64 binaries #153
Conversation
@japaric If you consider this approach viable, we can write some docs add support for other archs in another PR. |
|
||
COPY linux-runner / | ||
|
||
ENV CROSS_RUNNER="qemu-user" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default runner is qemu-user, but the user can set CROSS_RUNNER=qemu-system to change the runner.
docker/linux-runner
Outdated
-kernel /qemu/vmlinuz \ | ||
-initrd /qemu/initrd.gz \ | ||
-serial tcp:127.0.0.1:4444,server,nowait \ | ||
-virtfs local,path=/target,security_model=mapped,mount_tag=target \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use virtfs to share /target with the guest running in qemu. The files do not need to be explicit copied!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In stdsimd
we use objdump
from a procedural macro to check the assembly generated by SIMD intrinsics. objdump
isn't typically installed by default in the distros that are usually loaded into qemu-sytem
.
I think we could hack around this by copying whichever objdump
we want to use into target/
and passing its path in an environment variable to qemu system, but maybe there would be a better way of sharing resources and binaries with the code running in qemu-system
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qemu-system is used to run the target binaries, the build happens in the docker container, which has objdump
. Does this makes sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so. So what happens if my target binary tries to start a new process that runs objdump
? Does the one from the docker container get picked up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case there would be no objdump
... The qemu-system image is really small, it is basically a busybox rootfs... If this is necessary (sorry if I misunderstood you), I think you could put extra binaries in /target and use passthrough to pass env variable to inform where the binaries are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, that's exactly what I meant in my first comment. Once this is merged I'll try to get it to work.
docker/linux-runner
Outdated
begin=no | ||
|
||
# execute the program and then print the exit code | ||
echo "echo $begin_tag; $prog; echo $end_tag \$?" | nc -q -1 127.0.0.1 4444 | while read LINE; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can run $prog directly on the guest!
Using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really impressive, @malbarbo. 👏
One of the concerns I had about using qemu-system was the wait time required for the VM boot process. I was expecting that a new VM would have to be booted per executed binary. But with your approach the VM is booted once per Cross invocation even if several binaries are going to be executed (e.g. if there are several integration tests in the tests
folder) during that invoctaion. Very good.
Regarding the UI, I'm wondering if it would be better to have a per target setting in Cross.toml instead of a single CROSS_RUNNER variable. Something like this:
# Cross.toml
[target.aarch64-unknown-linux-gnu]
runner = "qemu-system"
Eventually we want to support changing the settings through environment variables (cf. #147) so something like CROSS_TARGET_AARCH64_UNKNOWN_LINUX_GNU_RUNNER=qemu-system
would also work.
An advantage of doing this is that we can error earlier when an invalid runner is selected (before starting a Docker container), rather than erroring on each binary invocation -- which I think is what happens with the current implementation.
I'm also wondering if runner
is the right term for this. That sounds like we are directly executing the binary using $runner
(because that's how Cargo runners work and they existed first) but there's more logic involved when, for example, qemu-system is used. Perhaps emulation
would be better? At least it won't cause confusion with the Cargo runner feature, I think.
Before rubber stamping this I'd like to see how it would integrate with #131 which I have yet to review.
Finally, before landing this we should document the downsides and advantages of using qemu-system instead of qemu-user. For example with qemu-system you can't modify the host filesystem (that's probably a good thing though) or invoke binaries in the host $PATH (what @gnzlbg mentioned about objdump) but threading support is better.
P.S. Any idea of what other architectures could get qemu-system support? I think the main requirement is getting our hands on a compiled kernel and root filesystem for the target so it's up to what we can find and what's provided by distributions.
docker/linux-runner
Outdated
# execute the program and then print the exit code | ||
echo "echo $begin_tag; $prog; echo $end_tag \$?" | nc -q -1 127.0.0.1 4444 | while read LINE; do | ||
LINE=$(echo "$LINE" | tr -d '\r') | ||
if [[ "$LINE" == "oach8aobiePheew1kahl-start" ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could this be $begin_tag instead of the whole string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
docker/qemu.sh
Outdated
nice make -j$(nproc) | ||
make install | ||
|
||
# HACK the binfmt_misc interpreter we'll use expects the QEMU binary to be | ||
# in /usr/bin. Create an appropriate symlink | ||
ln -s /usr/local/bin/qemu-$arch /usr/bin/qemu-$arch-static | ||
ln -s /usr/local/bin/qemu-system-$arch /usr/bin/qemu-system-$arch || true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: is this necessary? /usr/local/bin should be in $PATH.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not necessary. Thanks!
docker/linux-runner
Outdated
-virtfs local,path=/target,security_model=mapped,mount_tag=target \ | ||
-append init=/init-alt > /dev/null & | ||
|
||
sleep 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add a rationale for the sleep here (probably "waiting for OS to boot")?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think this could be improved in the future (not necessarily in this PR) to wait until the OS finishes booting instead of waiting for a fixed amount of time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also I think it would helpful to at least print "booting QEMU virtual machine..." before sleep
to let the user know what's going on. Right now Cross kind of silently "hangs" for like 10 seconds before the binary is executed, which is a bit unsettling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point it would be useful to add an option to Cross (-vv
?) that lets you see the QEMU boot process on the console, for debugging purposes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a mechanism to wait until the OS finishes to boot. Also add boot messages including the time to boot.
docker/qemu.sh
Outdated
true | ||
;; | ||
*) | ||
echo "Invalid option $softmmu=$softmmu" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this branch contain an exit 1
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also the message will probably be rendered as Invalid option foo=foo
. It should probably look like Invalid softmmu option: foo
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
d83b621
to
6ede842
Compare
I tried to address all pointed issues. For now I continue to use the runner term. I made each target have at least one runner, witch simplified the code but also brings the possibility of adding new generic runners (like a ssh remote runner, for example). I do not have preference for the runner term, but I think using node, native, etc as a kind of emulator is strange.
I will need help with this. I have some difficulties writing in english, so it will take a lot of my time.
I've tried s390x, sparc64 and arm. I think Debian provides kernel and initrd for all rust supported archs, so we can offers qemu-system for many targets! |
If you just write "the contents" in schematic way I can go over it; feel free to use words in your mother tongue if you don't know the english ones.
This is more like a wishlist for
|
3772cf1
to
24f6239
Compare
I rewrote linux-image.sh and linux-runner to be more robust and make it easier to add other archs (I did preliminary test with 10 archs!). Instead of using debian installer kernel and initrd, we are using the normal debian packages and creating an initrd with busybox and some libs (this initrd is smaller and the boot is faster). Color is missing (adding ncurses-term package in the qemu image did not help). @gnzlbg Thanks for your help. When the PR is ready to merge I will try to write some docs. I will add the other targets after that. @japaric After this rewrite I think a new review from zero is need... but it is for better! |
See the reason for this change in the last commit message. This is a prerequisite for #166 |
Timed out |
bors r+ |
Canceled |
@japaric thank you! |
bd2a46e
to
0291f42
Compare
@malbarbo resolve the conflict so we can move ahead with this 👍 |
…e starting docker
This avoids hacks to get the exit code of the executed programs and also allows the correct color for the host terminal. It increases the time to start the qemu-system emulator and to start the execution of a program on it. But the overall time to execute ripgrep tests does not changes. We use dropbear because the openssh client refuses to execute in docker with an unknown user id.
The linux-runner hangs on travis, instead of using tailf to wait for qemu, we grep the log directly. This stopped the hang.
Rebased and adapted to #217. We are ready to go. |
bors: try |
tryBuild succeeded |
bors: r+ |
bors: r- |
Canceled |
bors: r+ |
Build succeeded |
Fixes #146