All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH for-3.1] tests/cpu-plug-test: check CPU hotplug on ppc64 with KVM
@ 2018-07-25 14:45 Greg Kurz
  2018-07-27  5:27 ` David Gibson
  0 siblings, 1 reply; 17+ messages in thread
From: Greg Kurz @ 2018-07-25 14:45 UTC (permalink / raw)
  To: qemu-devel; +Cc: David Gibson, Thomas Huth, Cédric Le Goater

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 <groug@kaod.org>
---
 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(&mname[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");

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH for-3.1] tests/cpu-plug-test: check CPU hotplug on ppc64 with KVM
  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
  0 siblings, 1 reply; 17+ messages in thread
From: David Gibson @ 2018-07-27  5:27 UTC (permalink / raw)
  To: Greg Kurz; +Cc: qemu-devel, Thomas Huth, Cédric Le Goater

[-- Attachment #1: Type: text/plain, Size: 3763 bytes --]

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 <groug@kaod.org>

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(&mname[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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH for-3.1] tests/cpu-plug-test: check CPU hotplug on ppc64 with KVM
  2018-07-27  5:27 ` David Gibson
@ 2018-07-27  7:54   ` Greg Kurz
  2018-07-27  8:18     ` Thomas Huth
  2018-07-30  5:57     ` David Gibson
  0 siblings, 2 replies; 17+ messages in thread
From: Greg Kurz @ 2018-07-27  7:54 UTC (permalink / raw)
  To: David Gibson; +Cc: qemu-devel, Thomas Huth, Cédric Le Goater

[-- Attachment #1: Type: text/plain, Size: 4094 bytes --]

On Fri, 27 Jul 2018 15:27:24 +1000
David Gibson <david@gibson.dropbear.id.au> 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 <groug@kaod.org>  
> 
> 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(&mname[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");
> >   
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH for-3.1] tests/cpu-plug-test: check CPU hotplug on ppc64 with KVM
  2018-07-27  7:54   ` Greg Kurz
@ 2018-07-27  8:18     ` Thomas Huth
  2018-07-27  9:00       ` Greg Kurz
  2018-07-30  5:57     ` David Gibson
  1 sibling, 1 reply; 17+ messages in thread
From: Thomas Huth @ 2018-07-27  8:18 UTC (permalink / raw)
  To: Greg Kurz, David Gibson; +Cc: qemu-devel, Cédric Le Goater

On 07/27/2018 09:54 AM, Greg Kurz wrote:
> On Fri, 27 Jul 2018 15:27:24 +1000
> David Gibson <david@gibson.dropbear.id.au> 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 <groug@kaod.org>  
>>
>> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH for-3.1] tests/cpu-plug-test: check CPU hotplug on ppc64 with KVM
  2018-07-27  8:18     ` Thomas Huth
@ 2018-07-27  9:00       ` Greg Kurz
  2018-07-27 11:25         ` Thomas Huth
  0 siblings, 1 reply; 17+ messages in thread
From: Greg Kurz @ 2018-07-27  9:00 UTC (permalink / raw)
  To: Thomas Huth; +Cc: David Gibson, qemu-devel, Cédric Le Goater

On Fri, 27 Jul 2018 10:18:14 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 07/27/2018 09:54 AM, Greg Kurz wrote:
> > On Fri, 27 Jul 2018 15:27:24 +1000
> > David Gibson <david@gibson.dropbear.id.au> 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 <groug@kaod.org>    
> >>
> >> 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
> 
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH for-3.1] tests/cpu-plug-test: check CPU hotplug on ppc64 with KVM
  2018-07-27  9:00       ` Greg Kurz
@ 2018-07-27 11:25         ` Thomas Huth
  2018-07-27 12:03           ` Greg Kurz
  0 siblings, 1 reply; 17+ messages in thread
From: Thomas Huth @ 2018-07-27 11:25 UTC (permalink / raw)
  To: Greg Kurz; +Cc: Cédric Le Goater, qemu-devel, David Gibson

On 07/27/2018 11:00 AM, Greg Kurz wrote:
> On Fri, 27 Jul 2018 10:18:14 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 07/27/2018 09:54 AM, Greg Kurz wrote:
>>> On Fri, 27 Jul 2018 15:27:24 +1000
>>> David Gibson <david@gibson.dropbear.id.au> 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 <groug@kaod.org>    
>>>>
>>>> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH for-3.1] tests/cpu-plug-test: check CPU hotplug on ppc64 with KVM
  2018-07-27 11:25         ` Thomas Huth
@ 2018-07-27 12:03           ` Greg Kurz
  0 siblings, 0 replies; 17+ messages in thread
From: Greg Kurz @ 2018-07-27 12:03 UTC (permalink / raw)
  To: Thomas Huth; +Cc: Cédric Le Goater, qemu-devel, David Gibson

On Fri, 27 Jul 2018 13:25:33 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 07/27/2018 11:00 AM, Greg Kurz wrote:
> > On Fri, 27 Jul 2018 10:18:14 +0200
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> >> On 07/27/2018 09:54 AM, Greg Kurz wrote:  
> >>> On Fri, 27 Jul 2018 15:27:24 +1000
> >>> David Gibson <david@gibson.dropbear.id.au> 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 <groug@kaod.org>      
> >>>>
> >>>> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH for-3.1] tests/cpu-plug-test: check CPU hotplug on ppc64 with KVM
  2018-07-27  7:54   ` Greg Kurz
  2018-07-27  8:18     ` Thomas Huth
@ 2018-07-30  5:57     ` David Gibson
  2018-07-30  8:41       ` Greg Kurz
  1 sibling, 1 reply; 17+ messages in thread
From: David Gibson @ 2018-07-30  5:57 UTC (permalink / raw)
  To: Greg Kurz; +Cc: qemu-devel, Thomas Huth, Cédric Le Goater

[-- Attachment #1: Type: text/plain, Size: 2095 bytes --]

On Fri, Jul 27, 2018 at 09:54:52AM +0200, Greg Kurz wrote:
> On Fri, 27 Jul 2018 15:27:24 +1000
> David Gibson <david@gibson.dropbear.id.au> 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 <groug@kaod.org>  
> > 
> > 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH for-3.1] tests/cpu-plug-test: check CPU hotplug on ppc64 with KVM
  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:25         ` David Gibson
  0 siblings, 2 replies; 17+ messages in thread
From: Greg Kurz @ 2018-07-30  8:41 UTC (permalink / raw)
  To: David Gibson; +Cc: qemu-devel, Thomas Huth, Cédric Le Goater

[-- Attachment #1: Type: text/plain, Size: 2579 bytes --]

On Mon, 30 Jul 2018 15:57:15 +1000
David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au> 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 <groug@kaod.org>    
> > > 
> > > 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 ?

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH for-3.1] tests/cpu-plug-test: check CPU hotplug on ppc64 with KVM
  2018-07-30  8:41       ` Greg Kurz
@ 2018-07-30  9:59         ` Greg Kurz
  2018-07-31  3:27           ` David Gibson
  2018-07-31  3:25         ` David Gibson
  1 sibling, 1 reply; 17+ messages in thread
From: Greg Kurz @ 2018-07-30  9:59 UTC (permalink / raw)
  To: David Gibson; +Cc: Thomas Huth, qemu-devel, Cédric Le Goater

[-- Attachment #1: Type: text/plain, Size: 2098 bytes --]

On Mon, 30 Jul 2018 10:41:45 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Mon, 30 Jul 2018 15:57:15 +1000
> David Gibson <david@gibson.dropbear.id.au> 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, &local_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 ?

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH for-3.1] tests/cpu-plug-test: check CPU hotplug on ppc64 with KVM
  2018-07-30  8:41       ` Greg Kurz
  2018-07-30  9:59         ` Greg Kurz
@ 2018-07-31  3:25         ` David Gibson
  2018-08-01 13:24           ` Greg Kurz
  1 sibling, 1 reply; 17+ messages in thread
From: David Gibson @ 2018-07-31  3:25 UTC (permalink / raw)
  To: Greg Kurz; +Cc: qemu-devel, Thomas Huth, Cédric Le Goater

[-- Attachment #1: Type: text/plain, Size: 3955 bytes --]

On Mon, Jul 30, 2018 at 10:41:45AM +0200, Greg Kurz wrote:
> On Mon, 30 Jul 2018 15:57:15 +1000
> David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au> 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 <groug@kaod.org>    
> > > > 
> > > > 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH for-3.1] tests/cpu-plug-test: check CPU hotplug on ppc64 with KVM
  2018-07-30  9:59         ` Greg Kurz
@ 2018-07-31  3:27           ` David Gibson
  2018-08-01 13:35             ` Greg Kurz
  0 siblings, 1 reply; 17+ messages in thread
From: David Gibson @ 2018-07-31  3:27 UTC (permalink / raw)
  To: Greg Kurz; +Cc: Thomas Huth, qemu-devel, Cédric Le Goater

[-- Attachment #1: Type: text/plain, Size: 2704 bytes --]

On Mon, Jul 30, 2018 at 11:59:00AM +0200, Greg Kurz wrote:
> On Mon, 30 Jul 2018 10:41:45 +0200
> Greg Kurz <groug@kaod.org> wrote:
> 
> > On Mon, 30 Jul 2018 15:57:15 +1000
> > David Gibson <david@gibson.dropbear.id.au> 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, &local_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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH for-3.1] tests/cpu-plug-test: check CPU hotplug on ppc64 with KVM
  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
  0 siblings, 2 replies; 17+ messages in thread
From: Greg Kurz @ 2018-08-01 13:24 UTC (permalink / raw)
  To: David Gibson; +Cc: qemu-devel, Thomas Huth, Cédric Le Goater

[-- Attachment #1: Type: text/plain, Size: 2775 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH for-3.1] tests/cpu-plug-test: check CPU hotplug on ppc64 with KVM
  2018-07-31  3:27           ` David Gibson
@ 2018-08-01 13:35             ` Greg Kurz
  0 siblings, 0 replies; 17+ messages in thread
From: Greg Kurz @ 2018-08-01 13:35 UTC (permalink / raw)
  To: David Gibson; +Cc: Thomas Huth, qemu-devel, Cédric Le Goater

[-- Attachment #1: Type: text/plain, Size: 3159 bytes --]

On Tue, 31 Jul 2018 13:27:15 +1000
David Gibson <david@gibson.dropbear.id.au> 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 <groug@kaod.org> wrote:
> >   
> > > On Mon, 30 Jul 2018 15:57:15 +1000
> > > David Gibson <david@gibson.dropbear.id.au> 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, &local_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 :)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH for-3.1] tests/cpu-plug-test: check CPU hotplug on ppc64 with KVM
  2018-08-01 13:24           ` Greg Kurz
@ 2018-08-02  4:08             ` David Gibson
  2018-08-02  9:36             ` Igor Mammedov
  1 sibling, 0 replies; 17+ messages in thread
From: David Gibson @ 2018-08-02  4:08 UTC (permalink / raw)
  To: Greg Kurz; +Cc: qemu-devel, Thomas Huth, Cédric Le Goater

[-- Attachment #1: Type: text/plain, Size: 3576 bytes --]

On Wed, Aug 01, 2018 at 03:24:30PM +0200, Greg Kurz 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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH for-3.1] tests/cpu-plug-test: check CPU hotplug on ppc64 with KVM
  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
  1 sibling, 1 reply; 17+ messages in thread
From: Igor Mammedov @ 2018-08-02  9:36 UTC (permalink / raw)
  To: Greg Kurz; +Cc: David Gibson, Thomas Huth, qemu-devel, Cédric Le Goater

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.

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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH for-3.1] tests/cpu-plug-test: check CPU hotplug on ppc64 with KVM
  2018-08-02  9:36             ` Igor Mammedov
@ 2018-08-02 10:52               ` Greg Kurz
  0 siblings, 0 replies; 17+ messages in thread
From: Greg Kurz @ 2018-08-02 10:52 UTC (permalink / raw)
  To: Igor Mammedov
  Cc: David Gibson, Thomas Huth, qemu-devel, Cédric Le Goater

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

> 
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2018-08-02 10:52 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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 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.