[Qemu-devel] [PATCH 0/2] spapr: Fix CPU unplug tests

[Qemu-devel] [PATCH 0/2] spapr: Fix CPU unplug tests

[Greg Kurz]

This series follows another approach suggested by Mike Roth in
order to fix the recent breakage of CPU unplug tests caused by
commit b8165118f52c, as discussed here:

https://patchwork.ozlabs.org/patch/1050227/

--
Greg

---

Greg Kurz (2):
      Revert "spapr: support memory unplug for qtest"
      spapr: Simulate CAS for qtest


 hw/ppc/spapr.c      |   11 +++++++++++
 hw/ppc/spapr_ovec.c |    6 ------
 2 files changed, 11 insertions(+), 6 deletions(-)

[Qemu-devel] [PATCH 1/2] Revert "spapr: support memory unplug for qtest"

[Greg Kurz]

Commit b8165118f52c broke CPU hotplug tests for old machine types:

$ QTEST_QEMU_BINARY=ppc64-softmmu/qemu-system-ppc64 ./tests/cpu-plug-test -m=slow
/ppc64/cpu-plug/pseries-3.1/device-add/2x3x1&maxcpus=6: OK
/ppc64/cpu-plug/pseries-2.12-sxxm/device-add/2x3x1&maxcpus=6: OK
/ppc64/cpu-plug/pseries-3.0/device-add/2x3x1&maxcpus=6: OK
/ppc64/cpu-plug/pseries-2.10/device-add/2x3x1&maxcpus=6: OK
/ppc64/cpu-plug/pseries-2.11/device-add/2x3x1&maxcpus=6: OK
/ppc64/cpu-plug/pseries-2.12/device-add/2x3x1&maxcpus=6: OK
/ppc64/cpu-plug/pseries-2.9/device-add/2x3x1&maxcpus=6: OK
/ppc64/cpu-plug/pseries-2.7/device-add/2x3x1&maxcpus=6: **
ERROR:/home/thuth/devel/qemu/hw/ppc/spapr_events.c:313:rtas_event_log_to_source: assertion failed: (source->enabled)
Broken pipe
/home/thuth/devel/qemu/tests/libqtest.c:143: kill_qemu() detected QEMU death from signal 6 (Aborted) (core dumped)
Aborted (core dumped)

The approach of faking the availability of OV5_HP_EVT causes the
code to assume the hotplug event source is enabled, which is wrong
for older machines.

This reverts commit b8165118f52ce5ee88565d3cec83d30374efdc96.

A subsequent patch will address the problem of CAS under qtest from
a different angle.

Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_ovec.c |    6 ------
 1 file changed, 6 deletions(-)

diff --git a/hw/ppc/spapr_ovec.c b/hw/ppc/spapr_ovec.c
index 12510b236a95..318bf33de4b1 100644
--- a/hw/ppc/spapr_ovec.c
+++ b/hw/ppc/spapr_ovec.c
@@ -16,7 +16,6 @@
 #include "qemu/bitmap.h"
 #include "exec/address-spaces.h"
 #include "qemu/error-report.h"
-#include "sysemu/qtest.h"
 #include "trace.h"
 #include <libfdt.h>
 
@@ -132,11 +131,6 @@ bool spapr_ovec_test(sPAPROptionVector *ov, long bitnr)
     g_assert(ov);
     g_assert(bitnr < OV_MAXBITS);
 
-    /* support memory unplug for qtest */
-    if (qtest_enabled() && bitnr == OV5_HP_EVT) {
-        return true;
-    }
-
     return test_bit(bitnr, ov->bitmap) ? true : false;
 }
 

[Qemu-devel] [PATCH 2/2] spapr: Simulate CAS for qtest

[Greg Kurz]

The RTAS event hotplug code for machine types 2.8 and newer depends on
the CAS negotiated ov5 in order to work properly. However, there's no
CAS when running under qtest. There has been a tentative to trick the
code by faking the OV5_HP_EVT bit, but it turned out to break other
assumptions in the code and the change got reverted.

Go for a more general approach and simulate a CAS when running under
qtest. For simplicity, this pseudo CAS simple simulates the case where
the guest supports the same features as the machine. It is done at
reset time, just before we reset the DRCs, which could potentially
exercise the unplug code.

This allows to test unplug on spapr with both older and newer machine
types.

Suggested-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index b6a571b6f184..6da64ef7ee2b 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -29,6 +29,7 @@
 #include "qapi/visitor.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/numa.h"
+#include "sysemu/qtest.h"
 #include "hw/hw.h"
 #include "qemu/log.h"
 #include "hw/fw-path-provider.h"
@@ -1711,6 +1712,16 @@ static void spapr_machine_reset(void)
      */
     spapr_irq_reset(spapr, &error_fatal);
 
+    /*
+     * There is no CAS under qtest. Simulate one to please the code that
+     * depends on spapr->ov5_cas. This is especially needed to test device
+     * unplug, so we do that before resetting the DRCs.
+     */
+    if (qtest_enabled()) {
+        spapr_ovec_cleanup(spapr->ov5_cas);
+        spapr->ov5_cas = spapr_ovec_clone(spapr->ov5);
+    }
+
     /* DRC reset may cause a device to be unplugged. This will cause troubles
      * if this device is used by another device (eg, a running vhost backend
      * will crash QEMU if the DIMM holding the vring goes away). To avoid such

Re: [Qemu-devel] [PATCH 2/2] spapr: Simulate CAS for qtest

[Michael Roth]

Quoting Greg Kurz (2019-03-01 13:32:37)
> The RTAS event hotplug code for machine types 2.8 and newer depends on
> the CAS negotiated ov5 in order to work properly. However, there's no
> CAS when running under qtest. There has been a tentative to trick the
> code by faking the OV5_HP_EVT bit, but it turned out to break other
> assumptions in the code and the change got reverted.
> 
> Go for a more general approach and simulate a CAS when running under
> qtest. For simplicity, this pseudo CAS simple simulates the case where
> the guest supports the same features as the machine. It is done at
> reset time, just before we reset the DRCs, which could potentially
> exercise the unplug code.
> 
> This allows to test unplug on spapr with both older and newer machine
> types.
> 
> Suggested-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Greg Kurz <groug@kaod.org>

Tested-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

Thanks for sending this!

Just now realizing we should probably apply the revert after this patch
however, since the commit we're reverting fixes a `make check` test that
is run by default, whereas this patch fixes one that only gets run if we
run the tests with -m=slow specified.

Maybe David can do that on his end?

> ---
>  hw/ppc/spapr.c |   11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b6a571b6f184..6da64ef7ee2b 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -29,6 +29,7 @@
>  #include "qapi/visitor.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/numa.h"
> +#include "sysemu/qtest.h"
>  #include "hw/hw.h"
>  #include "qemu/log.h"
>  #include "hw/fw-path-provider.h"
> @@ -1711,6 +1712,16 @@ static void spapr_machine_reset(void)
>       */
>      spapr_irq_reset(spapr, &error_fatal);
> 
> +    /*
> +     * There is no CAS under qtest. Simulate one to please the code that
> +     * depends on spapr->ov5_cas. This is especially needed to test device
> +     * unplug, so we do that before resetting the DRCs.
> +     */
> +    if (qtest_enabled()) {
> +        spapr_ovec_cleanup(spapr->ov5_cas);
> +        spapr->ov5_cas = spapr_ovec_clone(spapr->ov5);
> +    }
> +
>      /* DRC reset may cause a device to be unplugged. This will cause troubles
>       * if this device is used by another device (eg, a running vhost backend
>       * will crash QEMU if the DIMM holding the vring goes away). To avoid such
> 

Re: [Qemu-devel] [PATCH 2/2] spapr: Simulate CAS for qtest

[David Gibson]

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

On Fri, Mar 01, 2019 at 04:33:38PM -0600, Michael Roth wrote:
> Quoting Greg Kurz (2019-03-01 13:32:37)
> > The RTAS event hotplug code for machine types 2.8 and newer depends on
> > the CAS negotiated ov5 in order to work properly. However, there's no
> > CAS when running under qtest. There has been a tentative to trick the
> > code by faking the OV5_HP_EVT bit, but it turned out to break other
> > assumptions in the code and the change got reverted.
> > 
> > Go for a more general approach and simulate a CAS when running under
> > qtest. For simplicity, this pseudo CAS simple simulates the case where
> > the guest supports the same features as the machine. It is done at
> > reset time, just before we reset the DRCs, which could potentially
> > exercise the unplug code.
> > 
> > This allows to test unplug on spapr with both older and newer machine
> > types.
> > 
> > Suggested-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> 
> Tested-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> 
> Thanks for sending this!
> 
> Just now realizing we should probably apply the revert after this patch
> however, since the commit we're reverting fixes a `make check` test that
> is run by default, whereas this patch fixes one that only gets run if we
> run the tests with -m=slow specified.
> 
> Maybe David can do that on his end?

Applied in reverse order as suggested to ppc-for-4.0.  Thanks.

> 
> > ---
> >  hw/ppc/spapr.c |   11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index b6a571b6f184..6da64ef7ee2b 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -29,6 +29,7 @@
> >  #include "qapi/visitor.h"
> >  #include "sysemu/sysemu.h"
> >  #include "sysemu/numa.h"
> > +#include "sysemu/qtest.h"
> >  #include "hw/hw.h"
> >  #include "qemu/log.h"
> >  #include "hw/fw-path-provider.h"
> > @@ -1711,6 +1712,16 @@ static void spapr_machine_reset(void)
> >       */
> >      spapr_irq_reset(spapr, &error_fatal);
> > 
> > +    /*
> > +     * There is no CAS under qtest. Simulate one to please the code that
> > +     * depends on spapr->ov5_cas. This is especially needed to test device
> > +     * unplug, so we do that before resetting the DRCs.
> > +     */
> > +    if (qtest_enabled()) {
> > +        spapr_ovec_cleanup(spapr->ov5_cas);
> > +        spapr->ov5_cas = spapr_ovec_clone(spapr->ov5);
> > +    }
> > +
> >      /* DRC reset may cause a device to be unplugged. This will cause troubles
> >       * if this device is used by another device (eg, a running vhost backend
> >       * will crash QEMU if the DIMM holding the vring goes away). To avoid such
> > 
> 

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