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

2018-08-02 Thread Greg Kurz
On Thu, 2 Aug 2018 11:36:19 +0200
Igor Mammedov  wrote:

> On Wed, 1 Aug 2018 15:24:30 +0200
> Greg Kurz  wrote:
> 
> > On Tue, 31 Jul 2018 13:25:59 +1000
> > David Gibson  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

> 
> 




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

2018-08-02 Thread Igor Mammedov
On Wed, 1 Aug 2018 15:24:30 +0200
Greg Kurz  wrote:

> On Tue, 31 Jul 2018 13:25:59 +1000
> David Gibson  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.

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.





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

2018-08-02 Thread David Gibson
On Wed, Aug 01, 2018 at 03:24:30PM +0200, Greg Kurz wrote:
> On Tue, 31 Jul 2018 13:25:59 +1000
> David Gibson  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.

Right, so I'm suggesting this be changed to a non-qtest test.  Those
are fine to have -machine accel=kvm:tcg.  Of course that doesn't
prevent later adding a more detailed cpu hotplug qtest that *does* do
actions on the guest side.

> 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.

It makes sense in the context of what qtests are for.  They're
designed to test the emulated peripherals in isolation from the core
emulation (whether that's via tcg or kvm).

> > 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...

Yup.


-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature


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

2018-08-01 Thread Greg Kurz
On Tue, 31 Jul 2018 13:27:15 +1000
David Gibson  wrote:

> On Mon, Jul 30, 2018 at 11:59:00AM +0200, Greg Kurz wrote:
> > On Mon, 30 Jul 2018 10:41:45 +0200
> > Greg Kurz  wrote:
> >   
> > > On Mon, 30 Jul 2018 15:57:15 +1000
> > > David Gibson  wrote:  
> > [...]  
> > > > > > I'm pretty sure trying to change the accelerator on a qtest test 
> > > > > > just
> > > > > > doesn't make sense.  We'd need a different approach for testing cpu
> > > > > > hotplug against kvm & tcg backends.
> > > > > >   
> > > > > 
> > > > > The test starts QEMU, triggers the CPU hotplug code with a QMP command
> > > > > and checks the command didn't fail (or QEMU didn't crash, as it would
> > > > > have before commit b585395b655a)... I really don't understand what
> > > > > is wrong with that... Please elaborate.  
> > > > 
> > > > Well, ok, let me turn that around.  A test that doesn't rely on
> > > > controlling the guest side behaviour at all probably shouldn't be a
> > > > qtest based test, since that's what qtest is all about.
> > > > 
> > > 
> > > 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.
> > > 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.
> > > 
> > > Your remark seems to be more general though... are you meaning that
> > > doing something like qtest_start("-machine accel=kvm:tcg") is just
> > > wrong ?  
> > 
> > The purpose of this test is simply to exercise a path in QEMU that
> > is only used with KVM, but it can also be achieved the other way
> > around:
> > 
> > @@ -189,7 +190,7 @@ static void xics_system_init(MachineState *machine, int 
> > nr_irqs, Error **errp)
> >  sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> >  Error *local_err = NULL;
> >  
> > -if (kvm_enabled()) {
> > +if (kvm_enabled() || qtest_enabled()) {
> >  if (machine_kernel_irqchip_allowed(machine) &&
> >  !xics_kvm_init(spapr, _err)) {
> > 
> > This will test the setup of the in-kernel XICS when run on a book3s host,
> > and fallback to emulated XICS otherwise (eg, travis).
> > 
> > Would this be more acceptable ?  
> 
> No, I don't think that will work.  With this we call into kvm related
> code via machine_kernel_irqchip_allowed() and xics_kvm_init() even in
> the qtest case.  If they work on a host which doesn't have KVM (say
> x86) it will only be by sheer accident.
> 

It's the other way around actually. The expected behaviour would be
for machine_kernel_irqchip_allowed(machine) and/or xics_kvm_init()
to fail and to fallback to emulated XICS if run without a proper KVM.
This means no behavior change for this test when run on a x86 host.

The issue is when we run this with KVM actually, because the XICS KVM
code obviously needs... KVM to be initialized, which won't happen
with qtest :)


pgpX4s_4uO4ix.pgp
Description: OpenPGP digital signature


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

2018-08-01 Thread Greg Kurz
On Tue, 31 Jul 2018 13:25:59 +1000
David Gibson  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...


pgpvWiXg8OY8X.pgp
Description: OpenPGP digital signature


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

2018-07-30 Thread David Gibson
On Mon, Jul 30, 2018 at 10:41:45AM +0200, Greg Kurz wrote:
> On Mon, 30 Jul 2018 15:57:15 +1000
> David Gibson  wrote:
> 
> > On Fri, Jul 27, 2018 at 09:54:52AM +0200, Greg Kurz wrote:
> > > On Fri, 27 Jul 2018 15:27:24 +1000
> > > David Gibson  wrote:
> > >   
> > > > On Wed, Jul 25, 2018 at 04:45:26PM +0200, Greg Kurz wrote:  
> > > > > Commit b585395b655 fixed a regression introduced by some recent 
> > > > > changes
> > > > > in the XICS code, that was causing QEMU to crash instantly during CPU
> > > > > hotplug with KVM. This is typically the kind of bug we'd like our
> > > > > test suite to detect before it gets merged. Unfortunately, the current
> > > > > tests run with '-machine accel=qtest' and don't exercise KVM specific
> > > > > paths in QEMU.
> > > > > 
> > > > > This patch hence changes add_pseries_test_case() to launch QEMU with
> > > > > '-machine accel=kvm' if KVM is available.
> > > > > 
> > > > > A notable consequence is that the guest will execute SLOF, but for 
> > > > > some
> > > > > reasons SLOF sometimes hits a program exception. This causes the guest
> > > > > to loop forever and the test to be stuck.  Since we don't need the 
> > > > > guest
> > > > > to be truely running, let's pass -S to QEMU to avoid that.
> > > > > 
> > > > > Also disable machine capabilities that could be unavailable in KVM, 
> > > > > eg,
> > > > > when using PR KVM.
> > > > > 
> > > > > Signed-off-by: Greg Kurz 
> > > > 
> > > > I'm pretty sure trying to change the accelerator on a qtest test just
> > > > doesn't make sense.  We'd need a different approach for testing cpu
> > > > hotplug against kvm & tcg backends.
> > > >   
> > > 
> > > The test starts QEMU, triggers the CPU hotplug code with a QMP command
> > > and checks the command didn't fail (or QEMU didn't crash, as it would
> > > have before commit b585395b655a)... I really don't understand what
> > > is wrong with that... Please elaborate.  
> > 
> > Well, ok, let me turn that around.  A test that doesn't rely on
> > controlling the guest side behaviour at all probably shouldn't be a
> > qtest based test, since that's what qtest is all about.
> > 
> 
> 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 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.

> 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.

> 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.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature


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

2018-07-30 Thread David Gibson
On Mon, Jul 30, 2018 at 11:59:00AM +0200, Greg Kurz wrote:
> On Mon, 30 Jul 2018 10:41:45 +0200
> Greg Kurz  wrote:
> 
> > On Mon, 30 Jul 2018 15:57:15 +1000
> > David Gibson  wrote:
> [...]
> > > > > I'm pretty sure trying to change the accelerator on a qtest test just
> > > > > doesn't make sense.  We'd need a different approach for testing cpu
> > > > > hotplug against kvm & tcg backends.
> > > > > 
> > > > 
> > > > The test starts QEMU, triggers the CPU hotplug code with a QMP command
> > > > and checks the command didn't fail (or QEMU didn't crash, as it would
> > > > have before commit b585395b655a)... I really don't understand what
> > > > is wrong with that... Please elaborate.
> > > 
> > > Well, ok, let me turn that around.  A test that doesn't rely on
> > > controlling the guest side behaviour at all probably shouldn't be a
> > > qtest based test, since that's what qtest is all about.
> > >   
> > 
> > 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.
> > 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.
> > 
> > Your remark seems to be more general though... are you meaning that
> > doing something like qtest_start("-machine accel=kvm:tcg") is just
> > wrong ?
> 
> The purpose of this test is simply to exercise a path in QEMU that
> is only used with KVM, but it can also be achieved the other way
> around:
> 
> @@ -189,7 +190,7 @@ static void xics_system_init(MachineState *machine, int 
> nr_irqs, Error **errp)
>  sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
>  Error *local_err = NULL;
>  
> -if (kvm_enabled()) {
> +if (kvm_enabled() || qtest_enabled()) {
>  if (machine_kernel_irqchip_allowed(machine) &&
>  !xics_kvm_init(spapr, _err)) {
> 
> This will test the setup of the in-kernel XICS when run on a book3s host,
> and fallback to emulated XICS otherwise (eg, travis).
> 
> Would this be more acceptable ?

No, I don't think that will work.  With this we call into kvm related
code via machine_kernel_irqchip_allowed() and xics_kvm_init() even in
the qtest case.  If they work on a host which doesn't have KVM (say
x86) it will only be by sheer accident.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature


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

2018-07-30 Thread Greg Kurz
On Mon, 30 Jul 2018 10:41:45 +0200
Greg Kurz  wrote:

> On Mon, 30 Jul 2018 15:57:15 +1000
> David Gibson  wrote:
[...]
> > > > I'm pretty sure trying to change the accelerator on a qtest test just
> > > > doesn't make sense.  We'd need a different approach for testing cpu
> > > > hotplug against kvm & tcg backends.
> > > > 
> > > 
> > > The test starts QEMU, triggers the CPU hotplug code with a QMP command
> > > and checks the command didn't fail (or QEMU didn't crash, as it would
> > > have before commit b585395b655a)... I really don't understand what
> > > is wrong with that... Please elaborate.
> > 
> > Well, ok, let me turn that around.  A test that doesn't rely on
> > controlling the guest side behaviour at all probably shouldn't be a
> > qtest based test, since that's what qtest is all about.
> >   
> 
> 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.
> 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.
> 
> Your remark seems to be more general though... are you meaning that
> doing something like qtest_start("-machine accel=kvm:tcg") is just
> wrong ?

The purpose of this test is simply to exercise a path in QEMU that
is only used with KVM, but it can also be achieved the other way
around:

@@ -189,7 +190,7 @@ static void xics_system_init(MachineState *machine, int 
nr_irqs, Error **errp)
 sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
 Error *local_err = NULL;
 
-if (kvm_enabled()) {
+if (kvm_enabled() || qtest_enabled()) {
 if (machine_kernel_irqchip_allowed(machine) &&
 !xics_kvm_init(spapr, _err)) {

This will test the setup of the in-kernel XICS when run on a book3s host,
and fallback to emulated XICS otherwise (eg, travis).

Would this be more acceptable ?


pgpWLCihI6AQI.pgp
Description: OpenPGP digital signature


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

2018-07-30 Thread Greg Kurz
On Mon, 30 Jul 2018 15:57:15 +1000
David Gibson  wrote:

> On Fri, Jul 27, 2018 at 09:54:52AM +0200, Greg Kurz wrote:
> > On Fri, 27 Jul 2018 15:27:24 +1000
> > David Gibson  wrote:
> >   
> > > On Wed, Jul 25, 2018 at 04:45:26PM +0200, Greg Kurz wrote:  
> > > > Commit b585395b655 fixed a regression introduced by some recent changes
> > > > in the XICS code, that was causing QEMU to crash instantly during CPU
> > > > hotplug with KVM. This is typically the kind of bug we'd like our
> > > > test suite to detect before it gets merged. Unfortunately, the current
> > > > tests run with '-machine accel=qtest' and don't exercise KVM specific
> > > > paths in QEMU.
> > > > 
> > > > This patch hence changes add_pseries_test_case() to launch QEMU with
> > > > '-machine accel=kvm' if KVM is available.
> > > > 
> > > > A notable consequence is that the guest will execute SLOF, but for some
> > > > reasons SLOF sometimes hits a program exception. This causes the guest
> > > > to loop forever and the test to be stuck.  Since we don't need the guest
> > > > to be truely running, let's pass -S to QEMU to avoid that.
> > > > 
> > > > Also disable machine capabilities that could be unavailable in KVM, eg,
> > > > when using PR KVM.
> > > > 
> > > > Signed-off-by: Greg Kurz 
> > > 
> > > I'm pretty sure trying to change the accelerator on a qtest test just
> > > doesn't make sense.  We'd need a different approach for testing cpu
> > > hotplug against kvm & tcg backends.
> > >   
> > 
> > The test starts QEMU, triggers the CPU hotplug code with a QMP command
> > and checks the command didn't fail (or QEMU didn't crash, as it would
> > have before commit b585395b655a)... I really don't understand what
> > is wrong with that... Please elaborate.  
> 
> Well, ok, let me turn that around.  A test that doesn't rely on
> controlling the guest side behaviour at all probably shouldn't be a
> qtest based test, since that's what qtest is all about.
> 

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.
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.

Your remark seems to be more general though... are you meaning that
doing something like qtest_start("-machine accel=kvm:tcg") is just
wrong ?


pgpAxFUrKL5Uy.pgp
Description: OpenPGP digital signature


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

2018-07-29 Thread David Gibson
On Fri, Jul 27, 2018 at 09:54:52AM +0200, Greg Kurz wrote:
> On Fri, 27 Jul 2018 15:27:24 +1000
> David Gibson  wrote:
> 
> > On Wed, Jul 25, 2018 at 04:45:26PM +0200, Greg Kurz wrote:
> > > Commit b585395b655 fixed a regression introduced by some recent changes
> > > in the XICS code, that was causing QEMU to crash instantly during CPU
> > > hotplug with KVM. This is typically the kind of bug we'd like our
> > > test suite to detect before it gets merged. Unfortunately, the current
> > > tests run with '-machine accel=qtest' and don't exercise KVM specific
> > > paths in QEMU.
> > > 
> > > This patch hence changes add_pseries_test_case() to launch QEMU with
> > > '-machine accel=kvm' if KVM is available.
> > > 
> > > A notable consequence is that the guest will execute SLOF, but for some
> > > reasons SLOF sometimes hits a program exception. This causes the guest
> > > to loop forever and the test to be stuck.  Since we don't need the guest
> > > to be truely running, let's pass -S to QEMU to avoid that.
> > > 
> > > Also disable machine capabilities that could be unavailable in KVM, eg,
> > > when using PR KVM.
> > > 
> > > Signed-off-by: Greg Kurz   
> > 
> > I'm pretty sure trying to change the accelerator on a qtest test just
> > doesn't make sense.  We'd need a different approach for testing cpu
> > hotplug against kvm & tcg backends.
> > 
> 
> The test starts QEMU, triggers the CPU hotplug code with a QMP command
> and checks the command didn't fail (or QEMU didn't crash, as it would
> have before commit b585395b655a)... I really don't understand what
> is wrong with that... Please elaborate.

Well, ok, let me turn that around.  A test that doesn't rely on
controlling the guest side behaviour at all probably shouldn't be a
qtest based test, since that's what qtest is all about.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature


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

2018-07-27 Thread Greg Kurz
On Fri, 27 Jul 2018 13:25:33 +0200
Thomas Huth  wrote:

> On 07/27/2018 11:00 AM, Greg Kurz wrote:
> > On Fri, 27 Jul 2018 10:18:14 +0200
> > Thomas Huth  wrote:
> >   
> >> On 07/27/2018 09:54 AM, Greg Kurz wrote:  
> >>> On Fri, 27 Jul 2018 15:27:24 +1000
> >>> David Gibson  wrote:
> >>> 
>  On Wed, Jul 25, 2018 at 04:45:26PM +0200, Greg Kurz wrote:
> > Commit b585395b655 fixed a regression introduced by some recent changes
> > in the XICS code, that was causing QEMU to crash instantly during CPU
> > hotplug with KVM. This is typically the kind of bug we'd like our
> > test suite to detect before it gets merged. Unfortunately, the current
> > tests run with '-machine accel=qtest' and don't exercise KVM specific
> > paths in QEMU.
> >
> > This patch hence changes add_pseries_test_case() to launch QEMU with
> > '-machine accel=kvm' if KVM is available.
> >
> > A notable consequence is that the guest will execute SLOF, but for some
> > reasons SLOF sometimes hits a program exception. This causes the guest
> > to loop forever and the test to be stuck.  Since we don't need the guest
> > to be truely running, let's pass -S to QEMU to avoid that.
> >
> > Also disable machine capabilities that could be unavailable in KVM, eg,
> > when using PR KVM.
> >
> > Signed-off-by: Greg Kurz   
> 
>  I'm pretty sure trying to change the accelerator on a qtest test just
>  doesn't make sense.  We'd need a different approach for testing cpu
>  hotplug against kvm & tcg backends.
> 
> >>>
> >>> The test starts QEMU, triggers the CPU hotplug code with a QMP command
> >>> and checks the command didn't fail (or QEMU didn't crash, as it would
> >>> have before commit b585395b655a)... I really don't understand what
> >>> is wrong with that... Please elaborate.
> >>
> >> For a "real" test, I think you'd need a guest OS that is reacting to the
> >> hot plug events. So maybe this should rather be done in the avocado
> >> framework instead?
> >>  
> > 
> > The intent isn't a "real" test actually, but just to exercise the XICS-KVM
> > paths in QEMU that get called during CPU hotplug. This cannot be achieved
> > with the qtest accelerator.
> > 
> > This patch would have revealed the regression in b585395b655a right away,
> > with the simple 'make check' developers are expected to run before posting. 
> >  
> 
> OK, that's fair. I think what rather bugs me here is that you start QEMU
> with -S to work around something that sounds like real bug in SLOF ...
> any chance that you could fix that bug in SLOF instead of using -S here?
> 

Yeah, -S is a workaround indeed. I've chosen to go that way because I couldn't
find any impact on the hotplug path, but I may have overlooked something, so
I'm not surprised by your request :)

I guess that's fair too and I'll investigate.

Cheers,

--
Greg

>  Thomas




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

2018-07-27 Thread Thomas Huth
On 07/27/2018 11:00 AM, Greg Kurz wrote:
> On Fri, 27 Jul 2018 10:18:14 +0200
> Thomas Huth  wrote:
> 
>> On 07/27/2018 09:54 AM, Greg Kurz wrote:
>>> On Fri, 27 Jul 2018 15:27:24 +1000
>>> David Gibson  wrote:
>>>   
 On Wed, Jul 25, 2018 at 04:45:26PM +0200, Greg Kurz wrote:  
> Commit b585395b655 fixed a regression introduced by some recent changes
> in the XICS code, that was causing QEMU to crash instantly during CPU
> hotplug with KVM. This is typically the kind of bug we'd like our
> test suite to detect before it gets merged. Unfortunately, the current
> tests run with '-machine accel=qtest' and don't exercise KVM specific
> paths in QEMU.
>
> This patch hence changes add_pseries_test_case() to launch QEMU with
> '-machine accel=kvm' if KVM is available.
>
> A notable consequence is that the guest will execute SLOF, but for some
> reasons SLOF sometimes hits a program exception. This causes the guest
> to loop forever and the test to be stuck.  Since we don't need the guest
> to be truely running, let's pass -S to QEMU to avoid that.
>
> Also disable machine capabilities that could be unavailable in KVM, eg,
> when using PR KVM.
>
> Signed-off-by: Greg Kurz 

 I'm pretty sure trying to change the accelerator on a qtest test just
 doesn't make sense.  We'd need a different approach for testing cpu
 hotplug against kvm & tcg backends.
  
>>>
>>> The test starts QEMU, triggers the CPU hotplug code with a QMP command
>>> and checks the command didn't fail (or QEMU didn't crash, as it would
>>> have before commit b585395b655a)... I really don't understand what
>>> is wrong with that... Please elaborate.  
>>
>> For a "real" test, I think you'd need a guest OS that is reacting to the
>> hot plug events. So maybe this should rather be done in the avocado
>> framework instead?
>>
> 
> The intent isn't a "real" test actually, but just to exercise the XICS-KVM
> paths in QEMU that get called during CPU hotplug. This cannot be achieved
> with the qtest accelerator.
> 
> This patch would have revealed the regression in b585395b655a right away,
> with the simple 'make check' developers are expected to run before posting.

OK, that's fair. I think what rather bugs me here is that you start QEMU
with -S to work around something that sounds like real bug in SLOF ...
any chance that you could fix that bug in SLOF instead of using -S here?

 Thomas



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

2018-07-27 Thread Greg Kurz
On Fri, 27 Jul 2018 10:18:14 +0200
Thomas Huth  wrote:

> On 07/27/2018 09:54 AM, Greg Kurz wrote:
> > On Fri, 27 Jul 2018 15:27:24 +1000
> > David Gibson  wrote:
> >   
> >> On Wed, Jul 25, 2018 at 04:45:26PM +0200, Greg Kurz wrote:  
> >>> Commit b585395b655 fixed a regression introduced by some recent changes
> >>> in the XICS code, that was causing QEMU to crash instantly during CPU
> >>> hotplug with KVM. This is typically the kind of bug we'd like our
> >>> test suite to detect before it gets merged. Unfortunately, the current
> >>> tests run with '-machine accel=qtest' and don't exercise KVM specific
> >>> paths in QEMU.
> >>>
> >>> This patch hence changes add_pseries_test_case() to launch QEMU with
> >>> '-machine accel=kvm' if KVM is available.
> >>>
> >>> A notable consequence is that the guest will execute SLOF, but for some
> >>> reasons SLOF sometimes hits a program exception. This causes the guest
> >>> to loop forever and the test to be stuck.  Since we don't need the guest
> >>> to be truely running, let's pass -S to QEMU to avoid that.
> >>>
> >>> Also disable machine capabilities that could be unavailable in KVM, eg,
> >>> when using PR KVM.
> >>>
> >>> Signed-off-by: Greg Kurz 
> >>
> >> I'm pretty sure trying to change the accelerator on a qtest test just
> >> doesn't make sense.  We'd need a different approach for testing cpu
> >> hotplug against kvm & tcg backends.
> >>  
> > 
> > The test starts QEMU, triggers the CPU hotplug code with a QMP command
> > and checks the command didn't fail (or QEMU didn't crash, as it would
> > have before commit b585395b655a)... I really don't understand what
> > is wrong with that... Please elaborate.  
> 
> For a "real" test, I think you'd need a guest OS that is reacting to the
> hot plug events. So maybe this should rather be done in the avocado
> framework instead?
> 

The intent isn't a "real" test actually, but just to exercise the XICS-KVM
paths in QEMU that get called during CPU hotplug. This cannot be achieved
with the qtest accelerator.

This patch would have revealed the regression in b585395b655a right away,
with the simple 'make check' developers are expected to run before posting.

Autotest has CPU hotplug tests, I don't know if avocado also has some, but
in any case, this is a different story IMHO.

Cheers,

--
Greg

>  Thomas
> 
> 




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

2018-07-27 Thread Thomas Huth
On 07/27/2018 09:54 AM, Greg Kurz wrote:
> On Fri, 27 Jul 2018 15:27:24 +1000
> David Gibson  wrote:
> 
>> On Wed, Jul 25, 2018 at 04:45:26PM +0200, Greg Kurz wrote:
>>> Commit b585395b655 fixed a regression introduced by some recent changes
>>> in the XICS code, that was causing QEMU to crash instantly during CPU
>>> hotplug with KVM. This is typically the kind of bug we'd like our
>>> test suite to detect before it gets merged. Unfortunately, the current
>>> tests run with '-machine accel=qtest' and don't exercise KVM specific
>>> paths in QEMU.
>>>
>>> This patch hence changes add_pseries_test_case() to launch QEMU with
>>> '-machine accel=kvm' if KVM is available.
>>>
>>> A notable consequence is that the guest will execute SLOF, but for some
>>> reasons SLOF sometimes hits a program exception. This causes the guest
>>> to loop forever and the test to be stuck.  Since we don't need the guest
>>> to be truely running, let's pass -S to QEMU to avoid that.
>>>
>>> Also disable machine capabilities that could be unavailable in KVM, eg,
>>> when using PR KVM.
>>>
>>> Signed-off-by: Greg Kurz   
>>
>> I'm pretty sure trying to change the accelerator on a qtest test just
>> doesn't make sense.  We'd need a different approach for testing cpu
>> hotplug against kvm & tcg backends.
>>
> 
> The test starts QEMU, triggers the CPU hotplug code with a QMP command
> and checks the command didn't fail (or QEMU didn't crash, as it would
> have before commit b585395b655a)... I really don't understand what
> is wrong with that... Please elaborate.

For a "real" test, I think you'd need a guest OS that is reacting to the
hot plug events. So maybe this should rather be done in the avocado
framework instead?

 Thomas





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

2018-07-27 Thread Greg Kurz
On Fri, 27 Jul 2018 15:27:24 +1000
David Gibson  wrote:

> On Wed, Jul 25, 2018 at 04:45:26PM +0200, Greg Kurz wrote:
> > Commit b585395b655 fixed a regression introduced by some recent changes
> > in the XICS code, that was causing QEMU to crash instantly during CPU
> > hotplug with KVM. This is typically the kind of bug we'd like our
> > test suite to detect before it gets merged. Unfortunately, the current
> > tests run with '-machine accel=qtest' and don't exercise KVM specific
> > paths in QEMU.
> > 
> > This patch hence changes add_pseries_test_case() to launch QEMU with
> > '-machine accel=kvm' if KVM is available.
> > 
> > A notable consequence is that the guest will execute SLOF, but for some
> > reasons SLOF sometimes hits a program exception. This causes the guest
> > to loop forever and the test to be stuck.  Since we don't need the guest
> > to be truely running, let's pass -S to QEMU to avoid that.
> > 
> > Also disable machine capabilities that could be unavailable in KVM, eg,
> > when using PR KVM.
> > 
> > Signed-off-by: Greg Kurz   
> 
> I'm pretty sure trying to change the accelerator on a qtest test just
> doesn't make sense.  We'd need a different approach for testing cpu
> hotplug against kvm & tcg backends.
> 

The test starts QEMU, triggers the CPU hotplug code with a QMP command
and checks the command didn't fail (or QEMU didn't crash, as it would
have before commit b585395b655a)... I really don't understand what
is wrong with that... Please elaborate.

> > ---
> >  tests/cpu-plug-test.c |   20 
> >  1 file changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tests/cpu-plug-test.c b/tests/cpu-plug-test.c
> > index 5f39ba0df394..2107557809b7 100644
> > --- a/tests/cpu-plug-test.c
> > +++ b/tests/cpu-plug-test.c
> > @@ -21,6 +21,7 @@ struct PlugTestData {
> >  unsigned cores;
> >  unsigned threads;
> >  unsigned maxcpus;
> > +const char *extra_args;
> >  };
> >  typedef struct PlugTestData PlugTestData;
> >  
> > @@ -106,9 +107,10 @@ static void 
> > test_plug_with_device_add_coreid(gconstpointer data)
> >  char *args;
> >  unsigned int c;
> >  
> > -args = g_strdup_printf("-machine %s -cpu %s "
> > +args = g_strdup_printf("-machine %s -cpu %s %s "
> > "-smp 
> > 1,sockets=%u,cores=%u,threads=%u,maxcpus=%u",
> > td->machine, td->cpu_model,
> > +   td->extra_args ? td->extra_args : "",
> > td->sockets, td->cores, td->threads, 
> > td->maxcpus);
> >  qtest_start(args);
> >  
> > @@ -195,10 +197,20 @@ static void add_pseries_test_case(const char *mname)
> >  (g_str_has_prefix(mname, "pseries-2.") && atoi([10]) < 7)) {
> >  return;
> >  }
> > -data = g_new(PlugTestData, 1);
> > +data = g_new0(PlugTestData, 1);
> >  data->machine = g_strdup(mname);
> >  data->cpu_model = "power8_v2.0";
> > -data->device_model = g_strdup("power8_v2.0-spapr-cpu-core");
> > +if (!access("/sys/module/kvm_hv", F_OK) ||
> > +!access("/sys/module/kvm_pr", F_OK)) {
> > +data->cpu_model = "host";
> > +data->extra_args =
> > +"-machine accel=kvm "
> > +"-machine cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken "
> > +"-machine cap-htm=off "
> > +"-machine cap-hpt-max-page-size=64k "
> > +"-S";
> > +}
> > +data->device_model = g_strdup_printf("%s-spapr-cpu-core", 
> > data->cpu_model);
> >  data->sockets = 2;
> >  data->cores = 3;
> >  data->threads = 1;
> > @@ -221,7 +233,7 @@ static void add_s390x_test_case(const char *mname)
> >  return;
> >  }
> >  
> > -data = g_new(PlugTestData, 1);
> > +data = g_new0(PlugTestData, 1);
> >  data->machine = g_strdup(mname);
> >  data->cpu_model = "qemu";
> >  data->device_model = g_strdup("qemu-s390x-cpu");
> >   
> 



pgpL9_yKIwBoQ.pgp
Description: OpenPGP digital signature


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

2018-07-26 Thread David Gibson
On Wed, Jul 25, 2018 at 04:45:26PM +0200, Greg Kurz wrote:
> Commit b585395b655 fixed a regression introduced by some recent changes
> in the XICS code, that was causing QEMU to crash instantly during CPU
> hotplug with KVM. This is typically the kind of bug we'd like our
> test suite to detect before it gets merged. Unfortunately, the current
> tests run with '-machine accel=qtest' and don't exercise KVM specific
> paths in QEMU.
> 
> This patch hence changes add_pseries_test_case() to launch QEMU with
> '-machine accel=kvm' if KVM is available.
> 
> A notable consequence is that the guest will execute SLOF, but for some
> reasons SLOF sometimes hits a program exception. This causes the guest
> to loop forever and the test to be stuck.  Since we don't need the guest
> to be truely running, let's pass -S to QEMU to avoid that.
> 
> Also disable machine capabilities that could be unavailable in KVM, eg,
> when using PR KVM.
> 
> Signed-off-by: Greg Kurz 

I'm pretty sure trying to change the accelerator on a qtest test just
doesn't make sense.  We'd need a different approach for testing cpu
hotplug against kvm & tcg backends.

> ---
>  tests/cpu-plug-test.c |   20 
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/cpu-plug-test.c b/tests/cpu-plug-test.c
> index 5f39ba0df394..2107557809b7 100644
> --- a/tests/cpu-plug-test.c
> +++ b/tests/cpu-plug-test.c
> @@ -21,6 +21,7 @@ struct PlugTestData {
>  unsigned cores;
>  unsigned threads;
>  unsigned maxcpus;
> +const char *extra_args;
>  };
>  typedef struct PlugTestData PlugTestData;
>  
> @@ -106,9 +107,10 @@ static void 
> test_plug_with_device_add_coreid(gconstpointer data)
>  char *args;
>  unsigned int c;
>  
> -args = g_strdup_printf("-machine %s -cpu %s "
> +args = g_strdup_printf("-machine %s -cpu %s %s "
> "-smp 
> 1,sockets=%u,cores=%u,threads=%u,maxcpus=%u",
> td->machine, td->cpu_model,
> +   td->extra_args ? td->extra_args : "",
> td->sockets, td->cores, td->threads, td->maxcpus);
>  qtest_start(args);
>  
> @@ -195,10 +197,20 @@ static void add_pseries_test_case(const char *mname)
>  (g_str_has_prefix(mname, "pseries-2.") && atoi([10]) < 7)) {
>  return;
>  }
> -data = g_new(PlugTestData, 1);
> +data = g_new0(PlugTestData, 1);
>  data->machine = g_strdup(mname);
>  data->cpu_model = "power8_v2.0";
> -data->device_model = g_strdup("power8_v2.0-spapr-cpu-core");
> +if (!access("/sys/module/kvm_hv", F_OK) ||
> +!access("/sys/module/kvm_pr", F_OK)) {
> +data->cpu_model = "host";
> +data->extra_args =
> +"-machine accel=kvm "
> +"-machine cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken "
> +"-machine cap-htm=off "
> +"-machine cap-hpt-max-page-size=64k "
> +"-S";
> +}
> +data->device_model = g_strdup_printf("%s-spapr-cpu-core", 
> data->cpu_model);
>  data->sockets = 2;
>  data->cores = 3;
>  data->threads = 1;
> @@ -221,7 +233,7 @@ static void add_s390x_test_case(const char *mname)
>  return;
>  }
>  
> -data = g_new(PlugTestData, 1);
> +data = g_new0(PlugTestData, 1);
>  data->machine = g_strdup(mname);
>  data->cpu_model = "qemu";
>  data->device_model = g_strdup("qemu-s390x-cpu");
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature