All of lore.kernel.org
 help / color / mirror / Atom feed
From: Greg Kurz <groug@kaod.org>
To: Igor Mammedov <imammedo@redhat.com>
Cc: "David Gibson" <david@gibson.dropbear.id.au>,
	"Thomas Huth" <thuth@redhat.com>,
	qemu-devel@nongnu.org, "Cédric Le Goater" <clg@kaod.org>
Subject: Re: [Qemu-devel] [PATCH for-3.1] tests/cpu-plug-test: check CPU hotplug on ppc64 with KVM
Date: Thu, 2 Aug 2018 12:52:24 +0200	[thread overview]
Message-ID: <20180802125224.7b8eca8d@bahia.lan> (raw)
In-Reply-To: <20180802113619.308800a3@redhat.com>

On Thu, 2 Aug 2018 11:36:19 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Wed, 1 Aug 2018 15:24:30 +0200
> Greg Kurz <groug@kaod.org> wrote:
> 
> > On Tue, 31 Jul 2018 13:25:59 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> > [...]  
> > > > 
> > > > The CPU hotplug test doesn't seem to do anything on the guest side: it
> > > > just checks that 'device_add' returns a response that isn't an
> > > > error.      
> > > 
> > > Right, which makes this ill suited to being a qtest test.  The whole
> > > point of qtest is making it easier to test qemu peripherals without
> > > having to have specific test code within the guest, by essentially
> > > replacing the guest's cpu with a stub controlled by the test harness.
> > > That's what the qtest accel is all about.
> > >     
> > 
> > I agree with what a qtest test should be, but cpu-plug-test doesn't
> > do anything like that obviously, ie, the guest CPU does nothing at
> > all. Only the hotplug path of the QEMU device model that don't need
> > the guest to run is tested here.
> > 
> > The more general issue is that paths guarded with kvm_enabled() cannot
> > be tested with a genuine qtest test. That's really unfortunate since
> > these paths are sometimes the one that are mostly used on the field,
> > eg, in-kernel XICS versus emulated XICS.
> >   
> > > I think it's confusing to have a test which tries things with both
> > > qtest and kvm accelerators.  Looking like a qtest test, people might
> > > reasonably think they can extend/refine the test to check behaviour
> > > when the guest does respond to the hotplug events.  But such an
> > > extension won't work with the kvm accel, because the qtest code used
> > > to simulate that guest response won't have any effect with kvm.
> > >     
> > 
> > If the motivation is to let the test be a true qtest in case someone
> > wants to emulate some guest behavior, I agree the kvm change is wrong.
> >   
> > > > I'm not aware that the guest is expected to have a specific behavior
> > > > during 'device_add', apart from not crashing or hanging. That was the
> > > > initial idea behind passing '-S' to ensure the guest doesn't run.      
> > > 
> > > Note that with qtest (or -S) we don't even test that minimal
> > > condition.  We only test that *qemu* doesn't crash - it could fatally
> > > compromise the guest and the test would never know.
> > >     
> > 
> > True.
> >   
> > > > Your remark seems to be more general though... are you meaning that
> > > > doing something like qtest_start("-machine accel=kvm:tcg") is just
> > > > wrong ?      
> > > 
> > > Pretty much, yes.  A non-qtest test which does that is reasonable, but
> > > not a qtest test.
> > >     
> > 
> > So, instead of hijacking the current qtest, we may add a non-qtest test
> > that would start QEMU with "-machine accel=kvm:tcg -S". This would allow
> > at least to test that QEMU doesn't crash right away. And, as suggested
> > by Thomas, the coverage could include SLOF as well if we don't pass -S.
> > But I would need to understand why SLOF sometimes hits a 0x700 when
> > running cpu-plug-test with this patch applied...  
> Is cpu-plug-test a qtest one?
> I was under impression it was using tcg accelerator.
> 

It starts QEMU with the qtest accelerator, but it doesn't do anything else
to simulate guest behavior. So I don't think it qualifies as a genuine
qtest test.

> we can switch it to accel=kvm:tcg like bios-tables-test did and
> probably reuse boot_sector_init() to make sure that firmware part
> at boot is exercised. Trying to do functional hotplug test on top
> of that (guest side) probably is out of scope of this unit test (too complex)
> and should be left to avocado or likes.

I agree. I'll do that.

Cheers,

--
Greg

> 
> 

      reply	other threads:[~2018-08-02 10:52 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-25 14:45 [Qemu-devel] [PATCH for-3.1] tests/cpu-plug-test: check CPU hotplug on ppc64 with KVM Greg Kurz
2018-07-27  5:27 ` David Gibson
2018-07-27  7:54   ` Greg Kurz
2018-07-27  8:18     ` Thomas Huth
2018-07-27  9:00       ` Greg Kurz
2018-07-27 11:25         ` Thomas Huth
2018-07-27 12:03           ` Greg Kurz
2018-07-30  5:57     ` David Gibson
2018-07-30  8:41       ` Greg Kurz
2018-07-30  9:59         ` Greg Kurz
2018-07-31  3:27           ` David Gibson
2018-08-01 13:35             ` Greg Kurz
2018-07-31  3:25         ` David Gibson
2018-08-01 13:24           ` Greg Kurz
2018-08-02  4:08             ` David Gibson
2018-08-02  9:36             ` Igor Mammedov
2018-08-02 10:52               ` Greg Kurz [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180802125224.7b8eca8d@bahia.lan \
    --to=groug@kaod.org \
    --cc=clg@kaod.org \
    --cc=david@gibson.dropbear.id.au \
    --cc=imammedo@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=thuth@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.