Set machine other than "q35" if not on X86. #143
Conversation
Introduce a new variable, arch.
| @@ -1509,7 +1511,11 @@ prepare_qcmd() | |||
| if [[ $_arg_kvm = "off" ]]; then | |||
| accel="tcg" # the default | |||
| fi | |||
| machine_args=("q35" "accel=$accel") | |||
| if [[ $(arch) != "aarch64" ]]; then | |||
There was a problem hiding this comment.
The negative check is awkward to read, it should be flipped around, but additionally, I'm thinking instead of a situation where we do something for arm vs something else for anything else, it might be clearer to make it more explicit?
i.e. something like:
case $arch in
x86_64)
<stuff>
;;
aarch64)
<stuff>
;;
*)
fail "Running on $arch is not supported
;;
esac
It does depend a bit on how many of these we have to sprinkle around. It could be worth seeing if the places that need arm specific changes can be consolidated together a bit more first, and then we have arch-specific functions to do those.
There was a problem hiding this comment.
Looking ahead, the negative if pattern follows in all the other PRs too, but I think a lot of that can be consolidated into an arch specific setup function which is called early on, and sets up variables for say things like where to find the kernel image etc. @marc-hb Thoughts on this arch specific 'setup' function approach vs something else?
One other approach I can think of would be to have a set of arch_do_thing() type helpers, and they could live in separate arch-specific files, and at the start, $arch decides which of the files to source. That can be cleaner overall, but could also make it a bit harder to follow what's coming from where for future debugging/changes.
There was a problem hiding this comment.
Even if most combinations do not work yet, let's please separate host_arch and guest_arch from the very beginning. I suspect only "homogeneous" configurations will only work initially and be better tested and maintained going forward but QEMU is... an emulator after all! So, run_qemu.sh should really not get in the way of someone wanting to try some "heterogenous" configuration where host_arch and guest_arch are different.
host_arch should be initialized from uname or similar, while guest_arch can default to host_arch for now. Later, guest_arch could be configurable somehow.
but I think a lot of that can be consolidated into an arch specific setup function which is called early on, and sets up variables for say things like where to find the kernel image etc
@marc-hb Thoughts on this arch specific 'setup' function approach vs something else?
Agreed, let's try to have all the arch-specific logic and configuration centralized in a small number of setup functions and/or files in order to keep the rest of the code simple, only referencing already initialized variables like for instance guest_arch_machine=virt,highmem=on.
but could also make it a bit harder to follow what's coming from where for future debugging/changes.
I think it's easier to have arch-specific configuration more centralized.
PS: in #140 (comment), @ikitayama you wrote that you are testing ARM64 on an M1 mac but you didn't say whether the M1 is running Linux or macOS? I'd be amazed if run_qemu works on macOS :-)
There was a problem hiding this comment.
Agreed on the host_arch vs guest_arch distinction.
I think I'd slightly prefer the separate files that are sourced route for this instead of making the main script even longer with copies of arch specific setup functions.
There was a problem hiding this comment.
@marc-hb I'm running the run_qemu.sh script on an Ubuntu 24.10 VM on an M1 powered by macOS Sequoia 15.4. I never tested run_qemu on macOS.
There was a problem hiding this comment.
I think I'd slightly prefer the separate files that are sourced
How about this middle ground: inside the script for host_arch stuff and outside for guest_arch stuff? host_arch is logically more "hardcoded" / not configurable.
There was a problem hiding this comment.
I'm running the run_qemu.sh script on an Ubuntu 24.10 VM on an M1 powered by macOS Sequoia 15.4.
So you got two, "stacked" layers of emulation! Which means it should be possible to run some tests in GitHub Actions...
PS: I know people have been raving about M1 performance but I would really recommend a server-class host to test run_qemu.sh changes if you can find one... "Plain" users can leverage incremental build tricks but making changes to run_qemu.sh itself very often requires build from scratch.
There was a problem hiding this comment.
@marc-hb @stellarhopper do you guys want to come up with a unified framework and I need to restart work from the refined run_qemu.sh?
There was a problem hiding this comment.
I'll get a skeleton started.
There was a problem hiding this comment.
I'll get a skeleton started.
Done and tested in #160. @stellarhopper can you please review?
|
Superseded by #160 |
| if [[ $(arch) != "aarch64" ]]; then | ||
| machine_args=("q35" "accel=$accel") | ||
| else | ||
| machine_args=("virt,highmem=on" "accel=$acel") |
There was a problem hiding this comment.
This PR has been superseded by #160 but I forgot highmem=on, sorry.
Architecture is checked by looking at the arch variable which is set at the top of run_qemu.sh.