[PATCH v2] Add configuration option to allow build qemu binary with

[PATCH v2] Add configuration option to allow build qemu binary with

[fan.ni]

From: Fan Ni <fan.ni@gmx.us>

Changes from v1 [1]:
1. Polished the help information;
2. Renamed the configuration name to emphasize it is for building qemu (Luis)


[1]: https://lore.kernel.org/kdevops/ZOfNoYfypKNijxoe@debian/T/#mf1c2a34860038dd672aee9203be223be136fd601

Fan Ni (1):
  qemu-build: Add a qemu build option for enabling debug

 Makefile.build_qemu                          | 4 ++++
 playbooks/roles/build_qemu/defaults/main.yml | 1 +
 playbooks/roles/build_qemu/tasks/main.yml    | 2 +-
 vagrant/Kconfig                              | 8 ++++++++
 4 files changed, 14 insertions(+), 1 deletion(-)

--
2.40.1

[PATCH v2 1/1] qemu-build: Add a qemu build option for enabling debug

[fan.ni]

From: Fan Ni <fan.ni@gmx.us>

Currently, qemu will always be compiled without enabling debug, which
means we will not be able to debug qemu source code with gdb. However,
building qemu with debug information included can be useful for qemu
development purpose. A new Kconfig option is added to allow user to enable
debug when build qemu. By default, it is disabled.

Signed-off-by: Fan Ni <fan.ni@samsung.com>
---
 Makefile.build_qemu                          | 4 ++++
 playbooks/roles/build_qemu/defaults/main.yml | 1 +
 playbooks/roles/build_qemu/tasks/main.yml    | 2 +-
 vagrant/Kconfig                              | 8 ++++++++
 4 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/Makefile.build_qemu b/Makefile.build_qemu
index 7addd2f0..f2dad0ae 100644
--- a/Makefile.build_qemu
+++ b/Makefile.build_qemu
@@ -19,6 +19,10 @@ ifeq (y,$(CONFIG_TARGET_ARCH_PPC64LE))
 QEMU_BUILD_SETUP_ARGS += qemu_target="ppc64-softmmu"
 endif

+ifeq (y,$(CONFIG_QEMU_BUILD_WITH_DEBUG_ENABLED))
+QEMU_BUILD_SETUP_ARGS += qemu_build_debug="--enable-debug"
+endif
+
 qemu: $(KDEVOPS_EXTRA_VARS)
 	@$(Q)ansible-playbook $(ANSIBLE_VERBOSE) --connection=local \
 		--inventory localhost, \
diff --git a/playbooks/roles/build_qemu/defaults/main.yml b/playbooks/roles/build_qemu/defaults/main.yml
index 006d669c..794cbd5f 100644
--- a/playbooks/roles/build_qemu/defaults/main.yml
+++ b/playbooks/roles/build_qemu/defaults/main.yml
@@ -12,3 +12,4 @@ qemu_git: "https://github.com/qemu/qemu.git"
 qemu_version: "v7.2.0-rc4"
 qemu_build_dir: "{{ qemu_data }}/build"
 qemu_target: "x86_64-softmmu"
+qemu_build_debug: False
diff --git a/playbooks/roles/build_qemu/tasks/main.yml b/playbooks/roles/build_qemu/tasks/main.yml
index c826c9cf..d3427c56 100644
--- a/playbooks/roles/build_qemu/tasks/main.yml
+++ b/playbooks/roles/build_qemu/tasks/main.yml
@@ -74,7 +74,7 @@
     - build_qemu_now|bool

 - name: Run configure for QEMU
-  command: "./configure --target-list={{ qemu_target }} --disable-download"
+  command: "./configure --target-list={{ qemu_target }} --disable-download {{qemu_build_debug}}"
   tags: [ 'qemu', 'configure' ]
   args:
     chdir: "{{ qemu_data }}"
diff --git a/vagrant/Kconfig b/vagrant/Kconfig
index 7f13791f..3bf096fc 100644
--- a/vagrant/Kconfig
+++ b/vagrant/Kconfig
@@ -266,6 +266,14 @@ endif # !QEMU_BUILD

 if QEMU_BUILD

+config QEMU_BUILD_WITH_DEBUG_ENABLED
+	bool "Build qemu with debug information enabled"
+	default n
+	help
+	  Enable this will add "--enable-debug" option when building qemu. With the
+	  option selected, users can use debug tools (like gdb) to debug qemu code,
+	  helping qemu code debugging.
+
 choice
 	prompt "QEMU git URL to use"
 	default QEMU_BUILD_JIC23
--
2.40.1

Re: [PATCH v2 1/1] qemu-build: Add a qemu build option for enabling debug

[Luis Chamberlain]

On Fri, Aug 25, 2023 at 11:17:44AM -0700, fan.ni@gmx.us wrote:
> From: Fan Ni <fan.ni@gmx.us>
> 
> Currently, qemu will always be compiled without enabling debug, which
> means we will not be able to debug qemu source code with gdb. However,
> building qemu with debug information included can be useful for qemu
> development purpose. A new Kconfig option is added to allow user to enable
> debug when build qemu. By default, it is disabled.

Commit log looks good now.

> 
> Signed-off-by: Fan Ni <fan.ni@samsung.com>
> ---
>  Makefile.build_qemu                          | 4 ++++
>  playbooks/roles/build_qemu/defaults/main.yml | 1 +
>  playbooks/roles/build_qemu/tasks/main.yml    | 2 +-
>  vagrant/Kconfig                              | 8 ++++++++
>  4 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile.build_qemu b/Makefile.build_qemu
> index 7addd2f0..f2dad0ae 100644
> --- a/Makefile.build_qemu
> +++ b/Makefile.build_qemu
> @@ -19,6 +19,10 @@ ifeq (y,$(CONFIG_TARGET_ARCH_PPC64LE))
>  QEMU_BUILD_SETUP_ARGS += qemu_target="ppc64-softmmu"
>  endif
> 
> +ifeq (y,$(CONFIG_QEMU_BUILD_WITH_DEBUG_ENABLED))
> +QEMU_BUILD_SETUP_ARGS += qemu_build_debug="--enable-debug"
> +endif
> +

The variable can be qemu_build_debug_enable instead, and this
should be replaced with a bool as well so 'True'.

>  qemu: $(KDEVOPS_EXTRA_VARS)
>  	@$(Q)ansible-playbook $(ANSIBLE_VERBOSE) --connection=local \
>  		--inventory localhost, \
> diff --git a/playbooks/roles/build_qemu/defaults/main.yml b/playbooks/roles/build_qemu/defaults/main.yml
> index 006d669c..794cbd5f 100644
> --- a/playbooks/roles/build_qemu/defaults/main.yml
> +++ b/playbooks/roles/build_qemu/defaults/main.yml
> @@ -12,3 +12,4 @@ qemu_git: "https://github.com/qemu/qemu.git"
>  qemu_version: "v7.2.0-rc4"
>  qemu_build_dir: "{{ qemu_data }}/build"
>  qemu_target: "x86_64-softmmu"
> +qemu_build_debug: False

Note this is a bool, please rename it and add something like:

qemu_build_debug_enable: False
qemu_build_debug_enable_string: "--enable-debug"
qemu_build_extra_strings: ""

> diff --git a/playbooks/roles/build_qemu/tasks/main.yml b/playbooks/roles/build_qemu/tasks/main.yml
> index c826c9cf..d3427c56 100644
> --- a/playbooks/roles/build_qemu/tasks/main.yml
> +++ b/playbooks/roles/build_qemu/tasks/main.yml
> @@ -74,7 +74,7 @@
>      - build_qemu_now|bool
> 
>  - name: Run configure for QEMU
> -  command: "./configure --target-list={{ qemu_target }} --disable-download"
> +  command: "./configure --target-list={{ qemu_target }} --disable-download {{qemu_build_debug}}"
>    tags: [ 'qemu', 'configure' ]
>    args:
>      chdir: "{{ qemu_data }}"

The way you have it this would run the following by default:

./configure --target-list=x86_64-softmmu --disable-download False

Where I think you want:

./configure --target-list=x86_64-softmmu --disable-download

A future goal is to modify kconfig to support outputting the yaml file
so we don't have to muck around with the Makefiles to exand on the
variables to let Makefile magic create our extra_vars.yaml file.
When that happens we won't ever have to edit Makefiles to check
for CONFIG variables and expand on things like QEMU_BUILD_SETUP_ARGS
which get appended to in the end ANSIBLE_EXTRA_ARGS. This is now
documented here:

https://github.com/linux-kdevops/kdevops/blob/master/docs/how-extra-vars-generated.md

Since our goal is to modify kconfig [0] to support outputting yaml file in
addition to the .config file, the question would be for us:

  what do we do today to minimize the amount of changes *today* so that when
  this support does come through we won't have to go and remove all the
  Makefile magic, or reduce that delta for new code?

I think the answer for the above would be for you to add an ansible
set_fact task which does the appending of the newly suggested default
empty string I made qemu_build_extra_strings with qemu_build_debug_enable_string
if qemu_build_debug_enable. For an example of a set_fact task which depends on
a bool you can check commit ed8c04809df15c ("bootlinux: add support for
"config-kdevops").

With this, in the future, if you find the need, you could even
dynamically expand the build arguments, so that if you run something
like 'make qemu-configure' you could add arguments at the end to pass to
ansible, like:

make qemu-configure EXTRA_CONFIGURE_ARGS="--enable-foo"

You can see how we made use of that on fstest with DYNAMIC_RUNTIME_VARS
on workflows/fstests/Makefile. To support this you could add an empty
dynamic string by default and the command line would override it, you'd
then just always append this dynamic default string to the task with
something like.

- name: Run configure for QEMU
  vars:
    extra_configure_args="{{ qemu_build_extra_strings + ' ' + qemu_build_dynamic_extra_strings }}
  command: "./configure --target-list={{ qemu_target }} {{ extra_configure_args }}"
  tags: [ 'qemu', 'configure' ]
    args:
      chdir: "{{ qemu_data }}"

[0] https://github.com/linux-kdevops/kconfig

  Luis

Re: [PATCH v2 1/1] qemu-build: Add a qemu build option for enabling debug

[Fan Ni]

On Fri, Aug 25, 2023 at 12:11:50PM -0700, Luis Chamberlain wrote:
> On Fri, Aug 25, 2023 at 11:17:44AM -0700, fan.ni@gmx.us wrote:
> > From: Fan Ni <fan.ni@gmx.us>
> >
> > Currently, qemu will always be compiled without enabling debug, which
> > means we will not be able to debug qemu source code with gdb. However,
> > building qemu with debug information included can be useful for qemu
> > development purpose. A new Kconfig option is added to allow user to enable
> > debug when build qemu. By default, it is disabled.
>
> Commit log looks good now.
>
> >
> > Signed-off-by: Fan Ni <fan.ni@samsung.com>
> > ---
> >  Makefile.build_qemu                          | 4 ++++
> >  playbooks/roles/build_qemu/defaults/main.yml | 1 +
> >  playbooks/roles/build_qemu/tasks/main.yml    | 2 +-
> >  vagrant/Kconfig                              | 8 ++++++++
> >  4 files changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/Makefile.build_qemu b/Makefile.build_qemu
> > index 7addd2f0..f2dad0ae 100644
> > --- a/Makefile.build_qemu
> > +++ b/Makefile.build_qemu
> > @@ -19,6 +19,10 @@ ifeq (y,$(CONFIG_TARGET_ARCH_PPC64LE))
> >  QEMU_BUILD_SETUP_ARGS += qemu_target="ppc64-softmmu"
> >  endif
> >
> > +ifeq (y,$(CONFIG_QEMU_BUILD_WITH_DEBUG_ENABLED))
> > +QEMU_BUILD_SETUP_ARGS += qemu_build_debug="--enable-debug"
> > +endif
> > +
>
> The variable can be qemu_build_debug_enable instead, and this
> should be replaced with a bool as well so 'True'.
>
> >  qemu: $(KDEVOPS_EXTRA_VARS)
> >  	@$(Q)ansible-playbook $(ANSIBLE_VERBOSE) --connection=local \
> >  		--inventory localhost, \
> > diff --git a/playbooks/roles/build_qemu/defaults/main.yml b/playbooks/roles/build_qemu/defaults/main.yml
> > index 006d669c..794cbd5f 100644
> > --- a/playbooks/roles/build_qemu/defaults/main.yml
> > +++ b/playbooks/roles/build_qemu/defaults/main.yml
> > @@ -12,3 +12,4 @@ qemu_git: "https://github.com/qemu/qemu.git"
> >  qemu_version: "v7.2.0-rc4"
> >  qemu_build_dir: "{{ qemu_data }}/build"
> >  qemu_target: "x86_64-softmmu"
> > +qemu_build_debug: False
>
> Note this is a bool, please rename it and add something like:
>
> qemu_build_debug_enable: False
> qemu_build_debug_enable_string: "--enable-debug"
> qemu_build_extra_strings: ""

Do we really need to add two variables for that? It seems we add an
unnecessary layer of check with qemu_build_debug_enable. I think we just need
a string like qemu_build_debug_enable_string. And leave it as "" here.
And when the configuration is turned on, set qemu_build_debug_enable_string to
"--enable-debug" and add build_debug_enable_string to the below
./configure command.

Fan

>
> > diff --git a/playbooks/roles/build_qemu/tasks/main.yml b/playbooks/roles/build_qemu/tasks/main.yml
> > index c826c9cf..d3427c56 100644
> > --- a/playbooks/roles/build_qemu/tasks/main.yml
> > +++ b/playbooks/roles/build_qemu/tasks/main.yml
> > @@ -74,7 +74,7 @@
> >      - build_qemu_now|bool
> >
> >  - name: Run configure for QEMU
> > -  command: "./configure --target-list={{ qemu_target }} --disable-download"
> > +  command: "./configure --target-list={{ qemu_target }} --disable-download {{qemu_build_debug}}"
> >    tags: [ 'qemu', 'configure' ]
> >    args:
> >      chdir: "{{ qemu_data }}"
>
> The way you have it this would run the following by default:
>
> ./configure --target-list=x86_64-softmmu --disable-download False
>
> Where I think you want:
>
> ./configure --target-list=x86_64-softmmu --disable-download
>
> A future goal is to modify kconfig to support outputting the yaml file
> so we don't have to muck around with the Makefiles to exand on the
> variables to let Makefile magic create our extra_vars.yaml file.
> When that happens we won't ever have to edit Makefiles to check
> for CONFIG variables and expand on things like QEMU_BUILD_SETUP_ARGS
> which get appended to in the end ANSIBLE_EXTRA_ARGS. This is now
> documented here:
>
> https://github.com/linux-kdevops/kdevops/blob/master/docs/how-extra-vars-generated.md
>
> Since our goal is to modify kconfig [0] to support outputting yaml file in
> addition to the .config file, the question would be for us:
>
>   what do we do today to minimize the amount of changes *today* so that when
>   this support does come through we won't have to go and remove all the
>   Makefile magic, or reduce that delta for new code?
>
> I think the answer for the above would be for you to add an ansible
> set_fact task which does the appending of the newly suggested default
> empty string I made qemu_build_extra_strings with qemu_build_debug_enable_string
> if qemu_build_debug_enable. For an example of a set_fact task which depends on
> a bool you can check commit ed8c04809df15c ("bootlinux: add support for
> "config-kdevops").
>
> With this, in the future, if you find the need, you could even
> dynamically expand the build arguments, so that if you run something
> like 'make qemu-configure' you could add arguments at the end to pass to
> ansible, like:
>
> make qemu-configure EXTRA_CONFIGURE_ARGS="--enable-foo"
>
> You can see how we made use of that on fstest with DYNAMIC_RUNTIME_VARS
> on workflows/fstests/Makefile. To support this you could add an empty
> dynamic string by default and the command line would override it, you'd
> then just always append this dynamic default string to the task with
> something like.
>
> - name: Run configure for QEMU
>   vars:
>     extra_configure_args="{{ qemu_build_extra_strings + ' ' + qemu_build_dynamic_extra_strings }}
>   command: "./configure --target-list={{ qemu_target }} {{ extra_configure_args }}"
>   tags: [ 'qemu', 'configure' ]
>     args:
>       chdir: "{{ qemu_data }}"
>
> [0] https://github.com/linux-kdevops/kconfig
>
>   Luis

Re: [PATCH v2 1/1] qemu-build: Add a qemu build option for enabling debug

[Luis Chamberlain]

On Fri, Aug 25, 2023 at 01:25:06PM -0700, Fan Ni wrote:
> > Note this is a bool, please rename it and add something like:
> >
> > qemu_build_debug_enable: False
> > qemu_build_debug_enable_string: "--enable-debug"
> > qemu_build_extra_strings: ""
> 
> Do we really need to add two variables for that? It seems we add an
> unnecessary layer of check with qemu_build_debug_enable.

It is not needed.

The bool was more of a suggestion to minimize the effort required later
once we get kconfig to support outputting extra_vars.yaml for us.

If you do the string in the yaml default but not in Kconfig, it means we
have to keep around more clutter in the Makefiles. Where as if you do
it as I suggested, once we add support to kconfig to ouput both .config
and extra_vars.yaml automatically for us, then the entire set of Makefile
changes you add could be removed.

But on is just trading off Makefile edits to an ansible tasks so that is
also not as optimal.

Come to think of it, from an optimization point of view I think we would want
this to be in Kconfig as a string *if* the bool is set, because an ansible
task task that is exucted at runtime which means we're slowing things down,
whereas a Kconfig would be just at initial 'make' time.

So you could just add something like:


config FSTESTS_FSTYP
 	string
	default "" if !DEBUG_BUILD_STUFF
	default "--foo" if DEBUG_BUILD_STUFF

The Makefile then would set the string variable if its not empty.
Then later once kconfig gets "extra_vars.yaml" output support we just
remove the Makefile check as kconfig would do the setting for us. The
variable names just need to match for Kconfig and ansible.

So this is just *forward* thinking about the future of minizing changes
like these.

  Luis