Re: [Qemu-devel] [PATCH 3/3] tests/acceptance: Add boot linux with kvm test

From: Eduardo Habkost <ehabkost@redhat.com>
To: Cleber Rosa <crosa@redhat.com>
Cc: philmd@redhat.com, jsnow@redhat.com, qemu-devel@nongnu.org,
	Wainer dos Santos Moschetta <wainersm@redhat.com>
Subject: Re: [Qemu-devel] [PATCH 3/3] tests/acceptance: Add boot linux with kvm test
Date: Mon, 1 Jul 2019 15:34:51 -0300	[thread overview]
Message-ID: <20190701183451.GF5198@habkost.net> (raw)
In-Reply-To: <20190630173933.GB2820@localhost.localdomain>

On Sun, Jun 30, 2019 at 01:39:33PM -0400, Cleber Rosa wrote:
> On Fri, Jun 28, 2019 at 05:18:46PM -0300, Eduardo Habkost wrote:
> > On Fri, Jun 28, 2019 at 11:02:17AM -0400, Wainer dos Santos Moschetta wrote:
> > > Until now the suite of acceptance tests doesn't exercise
> > > QEMU with kvm enabled. So this introduces a simple test
> > > that boots the Linux kernel and checks it boots on the
> > > accelerator correctly.
> > > 
> > > Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> > 
> > Why not just change the existing test_x86_64_pc() test case to
> > use KVM by default?  We can use "accel=kvm:tcg" to allow it to
> > fall back to TCG if KVM is not available.
> > 
> > -- 
> > Eduardo
> 
> I though of something similar, but not exactly the same.  An example
> can be seen here:
> 
>   https://travis-ci.org/clebergnu/qemu/jobs/551437429#L3350
> 
> IMO, it's a good practice to be able to briefly describe what a test
> does, given its name.  It's also very important for the test to
> attempt to exercise the same behavior across executions.
> 
> I'm saying that because I don't think we should fallback to TCG if KVM
> is not available, but instead, have two different tests that do each a
> simpler and more predictable set of checks.  This would make it
> simpler to find KVM issues when a given test fails but the TCG
> continues to pass.  The tags (and other mechanisms) can be used to
> select the tests that a given job should run though.

Agreed that kvm:tcg fallback I suggested isn't a good idea.
However, do we really want to require a separate test method to
be written just because we want to use a different accelerator or
other QEMU option?

This patch may be the simplest solution short term, but can we
have something that doesn't require so much code duplication and
boilerplate code in the future?

-- 
Eduardo