Skip to content
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.

Sign up for GitHub

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

run_qemu: make git-qemu argument a path #17

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

run_qemu: make git-qemu argument a path #17

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion parser_generator.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ exit 11 #)Created by argbash-init v2.9.0
# ARG_OPTIONAL_BOOLEAN([qmp], , [Open a QMP socket at unix:./run_qemu_qmp], )
# ARG_OPTIONAL_BOOLEAN([rw], , [Make guest image writeable (remove -snapshot)\n (Note that an image rebuild will lose any changes made via --rw)], )
# ARG_OPTIONAL_BOOLEAN([curses], , [Default display is -nographic. switch to -curses instead with this option.\n Use Esc+1, Esc+2, etc. to switch between the different screens.\n 'q' in the monitor screen to quit], )
# ARG_OPTIONAL_BOOLEAN([git-qemu], [g], [Use a qemu tree at '~/git/qemu/' for qemu binaries.\n This overrides any qemu=<foo> setting from the env.\n This is required and forced for any cxl related options], )
# ARG_OPTIONAL_SINGLE([git-qemu], , [Use specified qemu tree for qemu binaries.\n This overrides any qemu=<foo> setting from the env.\n This is required and forced for any cxl related options], )
# ARG_OPTIONAL_BOOLEAN([nfit-test], , [Setup an environment for unit tests\n - include libnvdimm 'extra' modules\n - add some memmap reserved memory\n Note: --rebuild=img or higher required when switching either to or away from nfit-test.\n This overrides any supplied 'preset' or topology options and forces preset=med], )
# ARG_OPTIONAL_BOOLEAN([defconfig], , [Run 'make olddefconfig' before the kernel build], )
# ARG_OPTIONAL_BOOLEAN([cmdline], , [Print the final qemu command line, but don't start qemu], )
Expand Down
16 changes: 9 additions & 7 deletions run_qemu.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,16 +227,18 @@ process_options_logic()
_arg_cxl="on"
fi
if [[ $_arg_cxl_legacy == "on" ]] || [[ $_arg_cxl == "on" ]]; then
_arg_git_qemu="on"
if [[ ! $_arg_git_qemu ]]; then
_arg_git_qemu=~/git/qemu/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This puts us in a weird situation where ~/git/qemu is default for CXL, but there's no default outside of CXL.
Also with CXL support soon to start appearing in upstream releases, and distros thereafter, I wonder if it is time to drop the CXL special case requirement of git-qemu altogether.

fi
fi
if [[ $_arg_git_qemu == "on" ]]; then
qemu=~/git/qemu/x86_64-softmmu/qemu-system-x86_64
qemu_img=~/git/qemu/qemu-img
qmp=~/git/qemu/scripts/qmp/qmp-shell
if [[ $_arg_git_qemu ]]; then
qemu=${_arg_git_qemu}/x86_64-softmmu/qemu-system-x86_64
qemu_img=${_arg_git_qemu}/qemu-img
qmp=${_arg_git_qemu}/scripts/qmp/qmp-shell
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this essentially duplicates the env way of setting qemu's location -

qemu=/path/to/qemu/binary run_qemu.sh ...

--git-qemu was supposed to be a shortcut to make that default to ~/git/qemu which happened to be the path in my case :)

Now that the user base is slightly bigger, I think it makes sense to clean it all up a bit:

  1. We can convert to --git=qemu= going forward, and make ~git/qemu the default in parser_generator.m4
  2. make qemu=<path> run_qemu.sh ... behave the same way (probably already the case)
  3. Remove the special case behavior for --cxl

Thoughts on this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get rid of all of these special cases and just use the env way of setting qemu? Let's assume cxl support moving forward, and figure out a way to add a check for cxl support if --cxl is set, maybe check qemu version?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather drop the environment way because environments variables are sneaky and evil :-)

While CXL support in QEMU is mainstream now, I think it would be good to keep just ONE way to override it, at the very least for testing purposes.

# upstream changed where binaries go recently
if [ ! -f "$qemu_img" ]; then
qemu=~/git/qemu/build/qemu-system-x86_64
qemu_img=~/git/qemu/build/qemu-img
qemu=${_arg_git_qemu}/build/qemu-system-x86_64
qemu_img=${_arg_git_qemu}/build/qemu-img
fi
if [ ! -x "$qemu" ]; then
fail "expected to find $qemu"
Expand Down