From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout.gmx.net (mout.gmx.net [212.227.15.15]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id E802219397 for ; Fri, 25 Aug 2023 20:25:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.us; s=s31663417; t=1692995115; x=1693599915; i=fan.ni@gmx.us; bh=7M+VdA7QZFpbdVMLbKLBTX5es0icthj83pYfHDVBA7c=; h=X-UI-Sender-Class:Date:From:To:Cc:Subject:References:In-Reply-To; b=N94frOvNNNU7WgPCJ4NO7COqqLkT8LIMu6OlzujLme5OxzDFAE8PXIjwmvssNvdpRtvqTWk vb33BoaGxqgXEcSnvvgig0Rc6mnsK4fw5d+y21Z8zpT7Af6+5xhPCtyokuTHVSgo7fhNPYUuP /+3wNORfVuW8YnyNgKF5UDkxNm1+BWZK7A77Y3HuD/nTTQk2SE6vdxBf/rKEEP2PF38Kq4Khk lD/MHAfjioqlBVz3i34Nh5a1VVhLO4SnvX65bzXEVuIW/vj2YI5Zm/yDHvg67fNq6gC9wbXMd e+xJc2s6nnQA7Q9U1s7vLVRQIGaf0PEem/4wNpha1wNb3gvQRjIg== X-UI-Sender-Class: 724b4f7f-cbec-4199-ad4e-598c01a50d3a Received: from debian ([99.13.228.231]) by mail.gmx.net (mrgmx005 [212.227.17.184]) with ESMTPSA (Nemesis) id 1MtwZ4-1phvol0Ywu-00uF5x; Fri, 25 Aug 2023 22:25:15 +0200 Date: Fri, 25 Aug 2023 13:25:06 -0700 From: Fan Ni To: Luis Chamberlain Cc: nmtadam.samsung@gmail.com, fan.ni@samsung.com, kdevops@lists.linux.dev, jlayton@kernel.org Subject: Re: [PATCH v2 1/1] qemu-build: Add a qemu build option for enabling debug Message-ID: References: <20230825181744.2303180-1-fan.ni@gmx.us> <20230825181744.2303180-2-fan.ni@gmx.us> Precedence: bulk X-Mailing-List: kdevops@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Provags-ID: V03:K1:tw1nzpszOC5+bVx+goBMwak694JzFLtMA5OpB4uY1l7A1ZOXQ/5 hRuqmyV/Z2ClqilrPLnB87/fO1Cac7XxEB7ZLY0Uq5wO7EQfWMLlFlOVnfifwsdZcvWV81r gLayFTb8TRQs5nNBXFVgTjcnMK4TqHsmZHeOngeR0f8yp240bsJSDS5T0V4AmFzt/oQ4G9l nDk6CXBXij3tXq9MOO03w== X-Spam-Flag: NO UI-OutboundReport: notjunk:1;M01:P0:eHLi5gBm5tA=;xIOwfFMVGcFb3H4uQ2098FzEaF0 WxM3yfWW+26czVJRxCR/2rCwqaBVnqtNwGjvmepWQo2Zg+xsTPnNuB2RaL+jPh7MXyQT9zonT kN9sNhlBF/5c+qVfuJnaGN6YVlZDxr2rjujQG0Hwk9/oA/a6+mbZAW/XVZaC0U2XCg4NuenG7 T6q9dmwf6vR6red121y6LXfaGfG/DpkRonZeLoa+B4m6H5XbDbPcizm+4lYQ30Z/0meNSIU/n w92CwMrr+TsdpJiCXy+tuaLn4OjFG6MQynyE36X5gPlonsveiqaQLWMhYrwerp/eRK31P+L+I Q+fpmeAnnNFCV2COJtwGlyxBTrWd+r28zVqyOreoNqFF+b/5eD4o+yD9aqK/q62aYKnygiokI Syw/8vluo9I/CPDV6oRnxKnJ0Xek61/Aqlv+uVYFvLIBhLOu6sGuBLrcfBiG8mUxCGhPjPrhf 8WGoco5Ls67roDHK6JOzpmNaVbusIE855RPpz9D3SFhw7CMAGcwhimdx6I0gqAx+4rx1LNTnd 48HKsQsSydvILZN4l/wBZIdQFeqpO5n6cQ28O+JuQyxxCFS/BHn+ROfeBJVCWAr4xPxOUrErE 1zI6N1MClifcZVZcp3RUTNulUWLY+e5s9E81iHcvUsKEmIcXpRPe2+SUJWuA1YRBjIN+d3nOD lUb5X42aTyN53Fcv5IHSeHA/iSNAsTgjM1ccvKmhtHZ6fqTDjuRwlHE6huT6P+YSPL5VfGhVg iB2CT6lzQO6ased4RNa7mDJCQSiUkZoKc5Y0+F64cbfdG74LPYxUqHqcPbLOyZm3tQm5kwOpw pGPe5P1HKSwlmJvl/7oWWzu98cm35H8ekuKt/IkNccj+Y1La12NumH44aTnwav4f4yFyxQHRB 3n017+MQ7F76aRAmVX9lnd9WkLyQl6Q0wNNbFBo7u4bWvLvzUXWXVpHh9/kuY+Ba0XWP+y7uh nJpffsSpmqRUSf0QICtCof9ocP0= Content-Transfer-Encoding: quoted-printable 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 > > > > 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 en= able > > debug when build qemu. By default, it is disabled. > > Commit log looks good now. > > > > > Signed-off-by: Fan Ni > > --- > > 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 +=3D qemu_target=3D"ppc64-softmmu" > > endif > > > > +ifeq (y,$(CONFIG_QEMU_BUILD_WITH_DEBUG_ENABLED)) > > +QEMU_BUILD_SETUP_ARGS +=3D qemu_build_debug=3D"--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=3Dlocal \ > > --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 n= eed 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_strin= g 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/rol= es/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=3D{{ qemu_target }} --disable-d= ownload" > > + command: "./configure --target-list=3D{{ qemu_target }} --disable-d= ownload {{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=3Dx86_64-softmmu --disable-download False > > Where I think you want: > > ./configure --target-list=3Dx86_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_enabl= e_string > if qemu_build_debug_enable. For an example of a set_fact task which depe= nds 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=3D"--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=3D"{{ qemu_build_extra_strings + ' ' + qemu_bui= ld_dynamic_extra_strings }} > command: "./configure --target-list=3D{{ qemu_target }} {{ extra_confi= gure_args }}" > tags: [ 'qemu', 'configure' ] > args: > chdir: "{{ qemu_data }}" > > [0] https://github.com/linux-kdevops/kconfig > > Luis