Re: [Qemu-devel] [PATCH for-3.1] tests/cpu-plug-test: check CPU hotplug on ppc64 with KVM

[Greg Kurz <groug@kaod.org>]

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

> 
>