[Qemu-devel] [PULL 0/6] Migration pull

[Qemu-devel] [PULL 0/6] Migration pull

[Juan Quintela]

Hi

This are the current migration pull:
- fix long id's for migration (David)
- several postcopy fixes (David)
- create the incomming state in a single place for Migraition (me)

Please, apply.

The following changes since commit 7d2c6c95511e42dffe2b263275e09957723d0ff4:

  Merge remote-tracking branch 'remotes/kraxel/tags/pull-usb-20170206-1' into staging (2017-02-06 11:44:08 +0000)

are available in the git repository at:

  git://github.com/juanquintela/qemu.git tags/migration/20170206

for you to fetch changes up to ef8d6488d2767fe81bb4bb9bcdc52af5ff718b56:

  postcopy: Recover block devices on early failure (2017-02-06 13:36:49 +0100)

----------------------------------------------------------------
migration/next for 20170206

----------------------------------------------------------------
Dr. David Alan Gilbert (5):
      vmstate_register_with_alias_id: Take an Error **
      migration: Check for ID length
      vmstate registration: check return values
      Postcopy: Reset state to avoid cleanup assert
      postcopy: Recover block devices on early failure

Juan Quintela (1):
      migration: create Migration Incoming State at init time

 hw/core/qdev.c                |  9 ++++---
 hw/intc/apic_common.c         |  2 +-
 include/migration/migration.h |  1 -
 include/migration/vmstate.h   |  7 +++--
 migration/migration.c         | 63 ++++++++++++++++++++++++++++---------------
 migration/savevm.c            | 29 ++++++++++++++------
 stubs/vmstate.c               |  3 ++-
 7 files changed, 77 insertions(+), 37 deletions(-)

[Qemu-devel] [PULL 1/6] migration: create Migration Incoming State at init time

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Message-Id: <1485207141-1941-3-git-send-email-quintela@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 include/migration/migration.h |  1 -
 migration/migration.c         | 38 +++++++++++++++++---------------------
 migration/savevm.c            |  4 ++--
 3 files changed, 19 insertions(+), 24 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index af9135f..7528cc2 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -122,7 +122,6 @@ struct MigrationIncomingState {
 };

 MigrationIncomingState *migration_incoming_get_current(void);
-MigrationIncomingState *migration_incoming_state_new(QEMUFile *f);
 void migration_incoming_state_destroy(void);

 /*
diff --git a/migration/migration.c b/migration/migration.c
index 2766d2f..619ccc4 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -111,32 +111,28 @@ MigrationState *migrate_get_current(void)
     return &current_migration;
 }

-/* For incoming */
-static MigrationIncomingState *mis_current;
-
 MigrationIncomingState *migration_incoming_get_current(void)
 {
-    return mis_current;
-}
+    static bool once;
+    static MigrationIncomingState mis_current;

-MigrationIncomingState *migration_incoming_state_new(QEMUFile* f)
-{
-    mis_current = g_new0(MigrationIncomingState, 1);
-    mis_current->from_src_file = f;
-    mis_current->state = MIGRATION_STATUS_NONE;
-    QLIST_INIT(&mis_current->loadvm_handlers);
-    qemu_mutex_init(&mis_current->rp_mutex);
-    qemu_event_init(&mis_current->main_thread_load_event, false);
-
-    return mis_current;
+    if (!once) {
+        mis_current.state = MIGRATION_STATUS_NONE;
+        memset(&mis_current, 0, sizeof(MigrationIncomingState));
+        QLIST_INIT(&mis_current.loadvm_handlers);
+        qemu_mutex_init(&mis_current.rp_mutex);
+        qemu_event_init(&mis_current.main_thread_load_event, false);
+        once = true;
+    }
+    return &mis_current;
 }

 void migration_incoming_state_destroy(void)
 {
-    qemu_event_destroy(&mis_current->main_thread_load_event);
-    loadvm_free_handlers(mis_current);
-    g_free(mis_current);
-    mis_current = NULL;
+    struct MigrationIncomingState *mis = migration_incoming_get_current();
+
+    qemu_event_destroy(&mis->main_thread_load_event);
+    loadvm_free_handlers(mis);
 }


@@ -382,11 +378,11 @@ static void process_incoming_migration_bh(void *opaque)
 static void process_incoming_migration_co(void *opaque)
 {
     QEMUFile *f = opaque;
-    MigrationIncomingState *mis;
+    MigrationIncomingState *mis = migration_incoming_get_current();
     PostcopyState ps;
     int ret;

-    mis = migration_incoming_state_new(f);
+    mis->from_src_file = f;
     postcopy_state_set(POSTCOPY_INCOMING_NONE);
     migrate_set_state(&mis->state, MIGRATION_STATUS_NONE,
                       MIGRATION_STATUS_ACTIVE);
diff --git a/migration/savevm.c b/migration/savevm.c
index 204012e..f3644ca 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2199,7 +2199,6 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp)
     qio_channel_set_name(QIO_CHANNEL(ioc), "migration-xen-load-state");
     f = qemu_fopen_channel_input(QIO_CHANNEL(ioc));

-    migration_incoming_state_new(f);
     ret = qemu_loadvm_state(f);
     qemu_fclose(f);
     if (ret < 0) {
@@ -2215,6 +2214,7 @@ int load_vmstate(const char *name)
     QEMUFile *f;
     int ret;
     AioContext *aio_context;
+    MigrationIncomingState *mis = migration_incoming_get_current();

     if (!bdrv_all_can_snapshot(&bs)) {
         error_report("Device '%s' is writable but does not support snapshots.",
@@ -2265,7 +2265,7 @@ int load_vmstate(const char *name)
     }

     qemu_system_reset(VMRESET_SILENT);
-    migration_incoming_state_new(f);
+    mis->from_src_file = f;

     aio_context_acquire(aio_context);
     ret = qemu_loadvm_state(f);
-- 
2.7.4

[Qemu-devel] [PULL 2/6] vmstate_register_with_alias_id: Take an Error **

[Juan Quintela]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

I'll be adding an error to it in a subsequent patch.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Message-Id: <20170202125956.21942-2-dgilbert@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/core/qdev.c              | 3 ++-
 hw/intc/apic_common.c       | 2 +-
 include/migration/vmstate.h | 5 +++--
 migration/savevm.c          | 3 ++-
 stubs/vmstate.c             | 3 ++-
 5 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 5783442..ea97b15 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -935,7 +935,8 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
         if (qdev_get_vmsd(dev)) {
             vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
                                            dev->instance_id_alias,
-                                           dev->alias_required_for_version);
+                                           dev->alias_required_for_version,
+                                           NULL);
         }

         QLIST_FOREACH(bus, &dev->child_bus, sibling) {
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 17df24c..6ce8ef7 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -329,7 +329,7 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
         instance_id = -1;
     }
     vmstate_register_with_alias_id(NULL, instance_id, &vmstate_apic_common,
-                                   s, -1, 0);
+                                   s, -1, 0, NULL);
 }

 static void apic_common_unrealize(DeviceState *dev, Error **errp)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 3bbe3ed..c38b00a 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -988,14 +988,15 @@ bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque);
 int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
                                    const VMStateDescription *vmsd,
                                    void *base, int alias_id,
-                                   int required_for_version);
+                                   int required_for_version,
+                                   Error **errp);

 static inline int vmstate_register(DeviceState *dev, int instance_id,
                                    const VMStateDescription *vmsd,
                                    void *opaque)
 {
     return vmstate_register_with_alias_id(dev, instance_id, vmsd,
-                                          opaque, -1, 0);
+                                          opaque, -1, 0, NULL);
 }

 void vmstate_unregister(DeviceState *dev, const VMStateDescription *vmsd,
diff --git a/migration/savevm.c b/migration/savevm.c
index f3644ca..b11b4a2 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -656,7 +656,8 @@ void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque)
 int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
                                    const VMStateDescription *vmsd,
                                    void *opaque, int alias_id,
-                                   int required_for_version)
+                                   int required_for_version,
+                                   Error **errp)
 {
     SaveStateEntry *se;

diff --git a/stubs/vmstate.c b/stubs/vmstate.c
index 6590627..bbe158f 100644
--- a/stubs/vmstate.c
+++ b/stubs/vmstate.c
@@ -8,7 +8,8 @@ int vmstate_register_with_alias_id(DeviceState *dev,
                                    int instance_id,
                                    const VMStateDescription *vmsd,
                                    void *base, int alias_id,
-                                   int required_for_version)
+                                   int required_for_version,
+                                   Error **errp)
 {
     return 0;
 }
-- 
2.7.4

[Qemu-devel] [PULL 3/6] migration: Check for ID length

[Juan Quintela]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

The qdev id of a device can be huge if it's on the end of a chain
of bridges; in reality such chains shouldn't occur but they can
be made to by chaining PCIe bridges together.

The migration format has a number of 256 character long format
limits; check we don't hit them (we already use pstrcat/cpy but
that just protects us from buffer overruns, we fairly quickly
hit an assert).

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Message-Id: <20170202125956.21942-3-dgilbert@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 include/migration/vmstate.h |  2 ++
 migration/savevm.c          | 21 ++++++++++++++++-----
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index c38b00a..6233fe2 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -985,12 +985,14 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,

 bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque);

+/* Returns: 0 on success, -1 on failure */
 int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
                                    const VMStateDescription *vmsd,
                                    void *base, int alias_id,
                                    int required_for_version,
                                    Error **errp);

+/* Returns: 0 on success, -1 on failure */
 static inline int vmstate_register(DeviceState *dev, int instance_id,
                                    const VMStateDescription *vmsd,
                                    void *opaque)
diff --git a/migration/savevm.c b/migration/savevm.c
index b11b4a2..8b8c74d 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -590,8 +590,14 @@ int register_savevm_live(DeviceState *dev,
     if (dev) {
         char *id = qdev_get_dev_path(dev);
         if (id) {
-            pstrcpy(se->idstr, sizeof(se->idstr), id);
-            pstrcat(se->idstr, sizeof(se->idstr), "/");
+            if (snprintf(se->idstr, sizeof(se->idstr), "%s/", id) >=
+                sizeof(se->idstr)) {
+                error_report("Path too long for VMState (%s)", id);
+                g_free(id);
+                g_free(se);
+
+                return -1;
+            }
             g_free(id);

             se->compat = g_new0(CompatEntry, 1);
@@ -674,9 +680,14 @@ int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
     if (dev) {
         char *id = qdev_get_dev_path(dev);
         if (id) {
-            pstrcpy(se->idstr, sizeof(se->idstr), id);
-            pstrcat(se->idstr, sizeof(se->idstr), "/");
-            g_free(id);
+            if (snprintf(se->idstr, sizeof(se->idstr), "%s/", id) >=
+                sizeof(se->idstr)) {
+                error_setg(errp, "Path too long for VMState (%s)", id);
+                g_free(id);
+                g_free(se);
+
+                return -1;
+            }

             se->compat = g_new0(CompatEntry, 1);
             pstrcpy(se->compat->idstr, sizeof(se->compat->idstr), vmsd->name);
-- 
2.7.4

[Qemu-devel] [PULL 4/6] vmstate registration: check return values

[Juan Quintela]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Check qdev's call to vmstate_register_with_alias_id; that gets
most of the common uses; there's hundreds of calls via vmstate_register
which could get fixed over time.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Message-Id: <20170202125956.21942-4-dgilbert@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/core/qdev.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index ea97b15..06ba02e 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -933,10 +933,12 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
         }

         if (qdev_get_vmsd(dev)) {
-            vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
-                                           dev->instance_id_alias,
-                                           dev->alias_required_for_version,
-                                           NULL);
+            if (vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
+                                               dev->instance_id_alias,
+                                               dev->alias_required_for_version,
+                                               &local_err) < 0) {
+                goto post_realize_fail;
+            }
         }

         QLIST_FOREACH(bus, &dev->child_bus, sibling) {
-- 
2.7.4

[Qemu-devel] [PULL 5/6] Postcopy: Reset state to avoid cleanup assert

[Juan Quintela]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

On a destination host with no userfault support an incoming
postcopy would cause the state to enter ADVISE before
it realised there was no support, and because it was in ADVISE
state it would perform a cleanup at the end.  Since there
was no support the cleanup function should be unreachable,
but ends up being called and asserting.

Reset the state when we realise we have no support, thus the
cleanup doesn't happen.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Message-Id: <20170202155909.31784-2-dgilbert@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/savevm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/migration/savevm.c b/migration/savevm.c
index 8b8c74d..0199768 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1355,6 +1355,7 @@ static int loadvm_postcopy_handle_advise(MigrationIncomingState *mis)
     }

     if (!postcopy_ram_supported_by_host()) {
+        postcopy_state_set(POSTCOPY_INCOMING_NONE);
         return -1;
     }

-- 
2.7.4

[Qemu-devel] [PULL 6/6] postcopy: Recover block devices on early failure

[Juan Quintela]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

An early postcopy failure can be recovered from as long as we know
we haven't sent the command to run the destination.
We have to undo the bdrv_inactivate_all by calling
bdrv_invalidate_cache_all

Note that I'm not using ms->block_inactive because once we've
sent the postcopy package we dont want anything else to try
and recover the block storage on the source; the destination
might have started writing to it.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Message-Id: <20170202155909.31784-3-dgilbert@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index 619ccc4..2b179c6 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1601,6 +1601,7 @@ static int postcopy_start(MigrationState *ms, bool *old_vm_running)
     QIOChannelBuffer *bioc;
     QEMUFile *fb;
     int64_t time_at_stop = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+    bool restart_block = false;
     migrate_set_state(&ms->state, MIGRATION_STATUS_ACTIVE,
                       MIGRATION_STATUS_POSTCOPY_ACTIVE);

@@ -1620,6 +1621,7 @@ static int postcopy_start(MigrationState *ms, bool *old_vm_running)
     if (ret < 0) {
         goto fail;
     }
+    restart_block = true;

     /*
      * Cause any non-postcopiable, but iterative devices to
@@ -1676,6 +1678,18 @@ static int postcopy_start(MigrationState *ms, bool *old_vm_running)

     /* <><> end of stuff going into the package */

+    /* Last point of recovery; as soon as we send the package the destination
+     * can open devices and potentially start running.
+     * Lets just check again we've not got any errors.
+     */
+    ret = qemu_file_get_error(ms->to_dst_file);
+    if (ret) {
+        error_report("postcopy_start: Migration stream errored (pre package)");
+        goto fail_closefb;
+    }
+
+    restart_block = false;
+
     /* Now send that blob */
     if (qemu_savevm_send_packaged(ms->to_dst_file, bioc->data, bioc->usage)) {
         goto fail_closefb;
@@ -1713,6 +1727,17 @@ fail_closefb:
 fail:
     migrate_set_state(&ms->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
                           MIGRATION_STATUS_FAILED);
+    if (restart_block) {
+        /* A failure happened early enough that we know the destination hasn't
+         * accessed block devices, so we're safe to recover.
+         */
+        Error *local_err = NULL;
+
+        bdrv_invalidate_cache_all(&local_err);
+        if (local_err) {
+            error_report_err(local_err);
+        }
+    }
     qemu_mutex_unlock_iothread();
     return -1;
 }
-- 
2.7.4

Re: [Qemu-devel] [PULL 0/6] Migration pull

[Peter Maydell]

On 6 February 2017 at 16:51, Juan Quintela <quintela@redhat.com> wrote:
> Hi
>
> This are the current migration pull:
> - fix long id's for migration (David)
> - several postcopy fixes (David)
> - create the incomming state in a single place for Migraition (me)
>
> Please, apply.
>
> The following changes since commit 7d2c6c95511e42dffe2b263275e09957723d0ff4:
>
>   Merge remote-tracking branch 'remotes/kraxel/tags/pull-usb-20170206-1' into staging (2017-02-06 11:44:08 +0000)
>
> are available in the git repository at:
>
>   git://github.com/juanquintela/qemu.git tags/migration/20170206
>
> for you to fetch changes up to ef8d6488d2767fe81bb4bb9bcdc52af5ff718b56:
>
>   postcopy: Recover block devices on early failure (2017-02-06 13:36:49 +0100)
>
> ----------------------------------------------------------------
> migration/next for 20170206
>
> ----------------------------------------------------------------
> Dr. David Alan Gilbert (5):
>       vmstate_register_with_alias_id: Take an Error **
>       migration: Check for ID length
>       vmstate registration: check return values
>       Postcopy: Reset state to avoid cleanup assert
>       postcopy: Recover block devices on early failure
>
> Juan Quintela (1):
>       migration: create Migration Incoming State at init time

I get a test failure on aarch64 host on one of the ppc guest tests:

TEST: tests/prom-env-test... (pid=26435)
  /ppc64/prom-env/mac99:                                               OK
  /ppc64/prom-env/g3beige:                                             OK
  /ppc64/prom-env/pseries:                                             **
ERROR:/home/petmay01/qemu/tests/prom-env-test.c:42:check_guest_memory:
assertion failed (signature == MAGIC): (0x7c7f1b78 == 0xcafec0de)
FAIL
GTester: last random seed: R02Sbd97cf9617f5dd55ba90b08765cfa0d0
(pid=26445)
FAIL: tests/prom-env-test


This might just be the test having an overzealous timeout (I
think I've seen this before a while back, when I was trying
to do a 'make -j2 check' on this slow board); but on the other
hand it's been fine recently but it failed twice trying to test
this pullreq.

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/6] Migration pull

[Juan Quintela]

Peter Maydell <peter.maydell@linaro.org> wrote:
> On 6 February 2017 at 16:51, Juan Quintela <quintela@redhat.com> wrote:


> I get a test failure on aarch64 host on one of the ppc guest tests:
>
> TEST: tests/prom-env-test... (pid=26435)
>   /ppc64/prom-env/mac99:                                               OK
>   /ppc64/prom-env/g3beige:                                             OK
>   /ppc64/prom-env/pseries:                                             **
> ERROR:/home/petmay01/qemu/tests/prom-env-test.c:42:check_guest_memory:
> assertion failed (signature == MAGIC): (0x7c7f1b78 == 0xcafec0de)
> FAIL
> GTester: last random seed: R02Sbd97cf9617f5dd55ba90b08765cfa0d0
> (pid=26445)
> FAIL: tests/prom-env-test
>
>
> This might just be the test having an overzealous timeout (I
> think I've seen this before a while back, when I was trying
> to do a 'make -j2 check' on this slow board); but on the other
> hand it's been fine recently but it failed twice trying to test
> this pullreq.

I don't have n aarch64 host, all I can say is that make check worked on
x86_64.

I would expect that this didn't change anything for prom enviroment, but
one never knows :p

I will wait to see if ppc folks can take a look at this.

Thanks, Juan.

Re: [Qemu-devel] [PULL 0/6] Migration pull

[Peter Maydell]

On 7 February 2017 at 07:50, Juan Quintela <quintela@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 6 February 2017 at 16:51, Juan Quintela <quintela@redhat.com> wrote:
>
>
>> I get a test failure on aarch64 host on one of the ppc guest tests:
>>
>> TEST: tests/prom-env-test... (pid=26435)
>>   /ppc64/prom-env/mac99:                                               OK
>>   /ppc64/prom-env/g3beige:                                             OK
>>   /ppc64/prom-env/pseries:                                             **
>> ERROR:/home/petmay01/qemu/tests/prom-env-test.c:42:check_guest_memory:
>> assertion failed (signature == MAGIC): (0x7c7f1b78 == 0xcafec0de)
>> FAIL
>> GTester: last random seed: R02Sbd97cf9617f5dd55ba90b08765cfa0d0
>> (pid=26445)
>> FAIL: tests/prom-env-test
>>
>>
>> This might just be the test having an overzealous timeout (I
>> think I've seen this before a while back, when I was trying
>> to do a 'make -j2 check' on this slow board); but on the other
>> hand it's been fine recently but it failed twice trying to test
>> this pullreq.
>
> I don't have n aarch64 host, all I can say is that make check worked on
> x86_64.
>
> I would expect that this didn't change anything for prom enviroment, but
> one never knows :p
>
> I will wait to see if ppc folks can take a look at this.

I tried this pull again, and got two good test runs in a row,
so I suspect this is just the host being a bit slow and the
timeout being a bit enthusiastic.

Applied to master.

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/6] Migration pull

[Thomas Huth]

On 07.02.2017 18:47, Peter Maydell wrote:
> On 7 February 2017 at 07:50, Juan Quintela <quintela@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 6 February 2017 at 16:51, Juan Quintela <quintela@redhat.com> wrote:
>>
>>> I get a test failure on aarch64 host on one of the ppc guest tests:
>>>
>>> TEST: tests/prom-env-test... (pid=26435)
>>>   /ppc64/prom-env/mac99:                                               OK
>>>   /ppc64/prom-env/g3beige:                                             OK
>>>   /ppc64/prom-env/pseries:                                             **
>>> ERROR:/home/petmay01/qemu/tests/prom-env-test.c:42:check_guest_memory:
>>> assertion failed (signature == MAGIC): (0x7c7f1b78 == 0xcafec0de)
>>> FAIL
>>> GTester: last random seed: R02Sbd97cf9617f5dd55ba90b08765cfa0d0
>>> (pid=26445)
>>> FAIL: tests/prom-env-test
>>>
>>>
>>> This might just be the test having an overzealous timeout (I
>>> think I've seen this before a while back, when I was trying
>>> to do a 'make -j2 check' on this slow board); but on the other
>>> hand it's been fine recently but it failed twice trying to test
>>> this pullreq.
>>
>> I don't have n aarch64 host, all I can say is that make check worked on
>> x86_64.
>>
>> I would expect that this didn't change anything for prom enviroment, but
>> one never knows :p
>>
>> I will wait to see if ppc folks can take a look at this.
> 
> I tried this pull again, and got two good test runs in a row,
> so I suspect this is just the host being a bit slow and the
> timeout being a bit enthusiastic.

How fast is that aarch64 machine? The timeout of the prom-env test is
already 100 seconds, so that seems to be plenty to me (the test normally
finishes here on my laptop within 15 seconds).
OTOH, if it helps with the aarch64 machine, I'm also fine if we increase
the timeout further - but what value would be appropriate then? Could you
please run the following on that slow machine:

 time QTEST_QEMU_BINARY=ppc64-softmmu/qemu-system-ppc64 tests/prom-env-test

Thomas

Re: [Qemu-devel] [PULL 0/6] Migration pull

[Peter Maydell]

On 8 February 2017 at 08:47, Thomas Huth <thuth@redhat.com> wrote:
> How fast is that aarch64 machine?

It's a juno board, so it's not very fast, but I wouldn't
have expected it to hit a 100 second timeout assuming
the test isn't trying to do a ludicrous amount of work.

> The timeout of the prom-env test is
> already 100 seconds, so that seems to be plenty to me (the test normally
> finishes here on my laptop within 15 seconds).
> OTOH, if it helps with the aarch64 machine, I'm also fine if we increase
> the timeout further - but what value would be appropriate then? Could you
> please run the following on that slow machine:
>
>  time QTEST_QEMU_BINARY=ppc64-softmmu/qemu-system-ppc64 tests/prom-env-test

/ppc64/prom-env/mac99: OK
/ppc64/prom-env/g3beige: OK
/ppc64/prom-env/pseries: OK

real    1m37.657s
user    1m36.748s
sys     0m1.440s

Breaking that down per testcase:

<tmmu/qemu-system-ppc64 tests/prom-env-test -v -p /ppc64/prom-env/mac99
/ppc64/prom-env/mac99: OK

real    0m9.836s
user    0m9.584s
sys     0m0.424s
<u/qemu-system-ppc64 tests/prom-env-test -v -p /ppc64/prom-env/g3beige
/ppc64/prom-env/g3beige: OK

real    0m9.099s
user    0m8.988s
sys     0m0.144s
<qemu-system-ppc64 tests/prom-env-test -v -p /ppc64/prom-env/pseries
/ppc64/prom-env/pseries: OK

real    1m24.362s
user    1m23.572s
sys     0m1.144s

pseries is much slower than the other two and at 85 seconds is
not all that much below the 100s. Something's odd here.

thanks
-- PMM

Re: [Qemu-devel] [Qemu-ppc] [PULL 0/6] Migration pull

[Thomas Huth]

On 09.02.2017 11:43, Peter Maydell wrote:
> On 8 February 2017 at 08:47, Thomas Huth <thuth@redhat.com> wrote:
>> How fast is that aarch64 machine?
> 
> It's a juno board, so it's not very fast, but I wouldn't
> have expected it to hit a 100 second timeout assuming
> the test isn't trying to do a ludicrous amount of work.
> 
>> The timeout of the prom-env test is
>> already 100 seconds, so that seems to be plenty to me (the test normally
>> finishes here on my laptop within 15 seconds).
>> OTOH, if it helps with the aarch64 machine, I'm also fine if we increase
>> the timeout further - but what value would be appropriate then? Could you
>> please run the following on that slow machine:
>>
>>  time QTEST_QEMU_BINARY=ppc64-softmmu/qemu-system-ppc64 tests/prom-env-test
[....]
> <qemu-system-ppc64 tests/prom-env-test -v -p /ppc64/prom-env/pseries
> /ppc64/prom-env/pseries: OK
> 
> real    1m24.362s
> user    1m23.572s
> sys     0m1.144s
> 
> pseries is much slower than the other two and at 85 seconds is
> not all that much below the 100s. Something's odd here.

SLOF (the firmware of the pseries machine) is known to perform quite bad
when running under TCG ... maybe it is even worse with the ARM version
of TCG? Or did you by any chance accidentially compile that version of
QEMU with "--enable-debug"?

Anyway, I've got some ideas how to speed up the test... and to be sure,
I'll also increase the timeout to 120 seconds instead. I'll send a patch
when it is ready...

 Thomas