Re: [kvm-unit-tests PATCH v4 05/13] scripts: Add 'kvmtool_params' to test definition

From: Alexandru Elisei <alexandru.elisei@arm.com>
To: Andrew Jones <andrew.jones@linux.dev>
Cc: eric.auger@redhat.com, lvivier@redhat.com, thuth@redhat.com,
	frankja@linux.ibm.com, imbrenda@linux.ibm.com, nrb@linux.ibm.com,
	david@redhat.com, pbonzini@redhat.com, kvm@vger.kernel.org,
	kvmarm@lists.linux.dev, linuxppc-dev@lists.ozlabs.org,
	kvm-riscv@lists.infradead.org, linux-s390@vger.kernel.org,
	will@kernel.org, julien.thierry.kdev@gmail.com, maz@kernel.org,
	oliver.upton@linux.dev, suzuki.poulose@arm.com,
	yuzenghui@huawei.com, joey.gouly@arm.com, andre.przywara@arm.com,
	shahuang@redhat.com
Subject: Re: [kvm-unit-tests PATCH v4 05/13] scripts: Add 'kvmtool_params' to test definition
Date: Thu, 26 Jun 2025 17:41:54 +0100	[thread overview]
Message-ID: <aF14UnQYBR9Knv4-@raptor> (raw)
In-Reply-To: <20250626-536c0af00aa655d6e647df44@orel>

Hi Drew,

On Thu, Jun 26, 2025 at 05:34:05PM +0200, Andrew Jones wrote:
> On Wed, Jun 25, 2025 at 04:48:05PM +0100, Alexandru Elisei wrote:
> > arm/arm64 supports running tests under kvmtool, but kvmtool's syntax for
> > running and configuring a virtual machine is different to qemu. To run
> > tests using the automated test infrastructure, add a new test parameter,
> > 'kvmtool_params'. The parameter serves the exact purpose as 'qemu_params',
> > but using kvmtool's syntax.
> > 
> > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > ---
> > 
> > Changes v3->v4:
> > 
> > * Added params_name in scripts/common.bash::for_each_unittest() to avoid
> > checking for $TARGET when deciding to parse kvmtool_params or
> > {qemu,extra}_params.
> > * Dropped factoring out parse_opts() in for_each_unittest().
> > 
> >  arm/unittests.cfg   | 24 ++++++++++++++++++++++++
> >  docs/unittests.txt  |  8 ++++++++
> >  scripts/common.bash | 11 +++++++----
> >  scripts/vmm.bash    | 16 ++++++++++++++++
> >  4 files changed, 55 insertions(+), 4 deletions(-)
[..]
> > +function vmm_unittest_params_name()
> > +{
> > +	# shellcheck disable=SC2155
> > +	local target=$(vmm_get_target)
> > +
> > +	case "$target" in
> > +	qemu)
> > +		echo "extra_params|qemu_params"
> > +		;;
> > +	*)
> > +		echo "$0 does not support '$target'"
> > +		exit 2
> > +		;;
> 
> It seems a bit odd that we've introduced kvmtool_params and applied it to
> arm in this patch, but we still don't support it. Not a huge deal, though.

Originally it was part of a huge patch that added everything in one go, it
was this patch and the next 6 or 7 patches combined. The feedback I got at
the time was to split it into more manageable chunks, which is very
understandable. So this is how I ended up with this patch, to make the
series easier to digest.

> 
> Reviewed-by: Andrew Jones <andrew.jones@linux.dev>

Thanks for the review!

Alex

-- 
kvm-riscv mailing list
kvm-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kvm-riscv