[Qemu-devel] [PATCH v3 11/32] migration: pass MigrationState to migrate_init()
Let the callers take the object, then pass it to migrate_init().
Reviewed-by: Dr. David Alan Gilbert
Signed-off-by: Peter Xu
---
migration/migration.c | 7 ++-
migration/migration.h | 2 +-
migration/savevm.c| 5 -
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 059c9cc334..3a734e9dda 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1220,10 +1220,8 @@ bool migration_is_idle(void)
return false;
}
-MigrationState *migrate_init(void)
+void migrate_init(MigrationState *s)
{
-MigrationState *s = migrate_get_current();
-
/*
* Reinitialise all migration state, except
* parameters/capabilities that the user set, and
@@ -1251,7 +1249,6 @@ MigrationState *migrate_init(void)
migrate_set_state(&s->state, MIGRATION_STATUS_NONE,
MIGRATION_STATUS_SETUP);
s->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
-return s;
}
static GSList *migration_blockers;
@@ -1359,7 +1356,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
migrate_set_block_incremental(s, true);
}
-s = migrate_init();
+migrate_init(s);
if (strstart(uri, "tcp:", &p)) {
tcp_start_outgoing_migration(s, p, &local_err);
diff --git a/migration/migration.h b/migration/migration.h
index b9bc617175..9502b2aad6 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -173,7 +173,7 @@ void migrate_fd_error(MigrationState *s, const Error
*error);
void migrate_fd_connect(MigrationState *s);
-MigrationState *migrate_init(void);
+void migrate_init(MigrationState *s);
bool migration_is_blocked(Error **errp);
/* True if outgoing migration has entered postcopy phase */
bool migration_in_postcopy(void);
diff --git a/migration/savevm.c b/migration/savevm.c
index 86ada6d0e7..6d6f8ee3e4 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1256,8 +1256,11 @@ void qemu_savevm_state_cleanup(void)
static int qemu_savevm_state(QEMUFile *f, Error **errp)
{
int ret;
-MigrationState *ms = migrate_init();
+MigrationState *ms = migrate_get_current();
MigrationStatus status;
+
+migrate_init(ms);
+
ms->to_dst_file = f;
if (migration_is_blocked(errp)) {
--
2.13.5
[Qemu-devel] [PATCH v3 14/32] migration: wakeup dst ram-load-thread for recover
On the destination side, we cannot wake up all the threads when we got
reconnected. The first thing to do is to wake up the main load thread,
so that we can continue to receive valid messages from source again and
reply when needed.
At this point, we switch the destination VM state from postcopy-paused
back to postcopy-recover.
Now we are finally ready to do the resume logic.
Reviewed-by: Dr. David Alan Gilbert
Signed-off-by: Peter Xu
---
migration/migration.c | 30 --
1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 76fdad9a72..c869d6ceb5 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -420,8 +420,34 @@ static void migration_incoming_process(void)
void migration_fd_process_incoming(QEMUFile *f)
{
-migration_incoming_setup(f);
-migration_incoming_process();
+MigrationIncomingState *mis = migration_incoming_get_current();
+
+if (mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
+/* Resumed from a paused postcopy migration */
+
+mis->from_src_file = f;
+/* Postcopy has standalone thread to do vm load */
+qemu_file_set_blocking(f, true);
+
+/* Re-configure the return path */
+mis->to_src_file = qemu_file_get_return_path(f);
+
+migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_PAUSED,
+ MIGRATION_STATUS_POSTCOPY_RECOVER);
+
+/*
+ * Here, we only wake up the main loading thread (while the
+ * fault thread will still be waiting), so that we can receive
+ * commands from source now, and answer it if needed. The
+ * fault thread will be woken up afterwards until we are sure
+ * that source is ready to reply to page requests.
+ */
+qemu_sem_post(&mis->postcopy_pause_sem_dst);
+} else {
+/* New incoming migration */
+migration_incoming_setup(f);
+migration_incoming_process();
+}
}
void migration_ioc_process_incoming(QIOChannel *ioc)
--
2.13.5
[Qemu-devel] [PATCH v3 24/32] migration: return incoming task tag for sockets
For socket based incoming migration, we attached a background task onto
main loop to handle the acception of connections. We never had a way to
destroy it before, only if we finished the migration.
Let's allow socket_start_incoming_migration() to return the source tag
of the listening async work, so that we may be able to clean it up in
the future.
Reviewed-by: Dr. David Alan Gilbert
Signed-off-by: Peter Xu
---
migration/socket.c | 36
migration/socket.h | 4 ++--
2 files changed, 26 insertions(+), 14 deletions(-)
diff --git a/migration/socket.c b/migration/socket.c
index 4879f11e0f..e8f3325155 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -162,8 +162,12 @@ out:
}
-static void socket_start_incoming_migration(SocketAddress *saddr,
-Error **errp)
+/*
+ * Returns the tag ID of the watch that is attached to global main
+ * loop (>0), or zero if failure detected.
+ */
+static guint socket_start_incoming_migration(SocketAddress *saddr,
+ Error **errp)
{
QIOChannelSocket *listen_ioc = qio_channel_socket_new();
@@ -172,30 +176,38 @@ static void socket_start_incoming_migration(SocketAddress
*saddr,
if (qio_channel_socket_listen_sync(listen_ioc, saddr, errp) < 0) {
object_unref(OBJECT(listen_ioc));
-return;
+return 0;
}
-qio_channel_add_watch(QIO_CHANNEL(listen_ioc),
- G_IO_IN,
- socket_accept_incoming_migration,
- listen_ioc,
- (GDestroyNotify)object_unref);
+return qio_channel_add_watch(QIO_CHANNEL(listen_ioc),
+ G_IO_IN,
+ socket_accept_incoming_migration,
+ listen_ioc,
+ (GDestroyNotify)object_unref);
}
-void tcp_start_incoming_migration(const char *host_port, Error **errp)
+guint tcp_start_incoming_migration(const char *host_port, Error **errp)
{
Error *err = NULL;
SocketAddress *saddr = tcp_build_address(host_port, &err);
+guint tag = 0;
+
if (!err) {
-socket_start_incoming_migration(saddr, &err);
+tag = socket_start_incoming_migration(saddr, &err);
}
error_propagate(errp, err);
qapi_free_SocketAddress(saddr);
+
+return tag;
}
-void unix_start_incoming_migration(const char *path, Error **errp)
+guint unix_start_incoming_migration(const char *path, Error **errp)
{
SocketAddress *saddr = unix_build_address(path);
-socket_start_incoming_migration(saddr, errp);
+guint tag;
+
+tag = socket_start_incoming_migration(saddr, errp);
qapi_free_SocketAddress(saddr);
+
+return tag;
}
diff --git a/migration/socket.h b/migration/socket.h
index 6b91e9db38..bc8a59aee4 100644
--- a/migration/socket.h
+++ b/migration/socket.h
@@ -16,12 +16,12 @@
#ifndef QEMU_MIGRATION_SOCKET_H
#define QEMU_MIGRATION_SOCKET_H
-void tcp_start_incoming_migration(const char *host_port, Error **errp);
+guint tcp_start_incoming_migration(const char *host_port, Error **errp);
void tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
Error **errp);
-void unix_start_incoming_migration(const char *path, Error **errp);
+guint unix_start_incoming_migration(const char *path, Error **errp);
void unix_start_outgoing_migration(MigrationState *s, const char *path,
Error **errp);
--
2.13.5
[Qemu-devel] [PATCH v3 13/32] migration: new state "postcopy-recover"
Introducing new migration state "postcopy-recover". If a migration
procedure is paused and the connection is rebuilt afterward
successfully, we'll switch the source VM state from "postcopy-paused" to
the new state "postcopy-recover", then we'll do the resume logic in the
migration thread (along with the return path thread).
This patch only do the state switch on source side. Another following up
patch will handle the state switching on destination side using the same
status bit.
Reviewed-by: Dr. David Alan Gilbert
Signed-off-by: Peter Xu
---
migration/migration.c | 76 ++-
qapi/migration.json | 4 ++-
2 files changed, 60 insertions(+), 20 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 56ba011007..76fdad9a72 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -550,6 +550,7 @@ static bool migration_is_setup_or_active(int state)
case MIGRATION_STATUS_ACTIVE:
case MIGRATION_STATUS_POSTCOPY_ACTIVE:
case MIGRATION_STATUS_POSTCOPY_PAUSED:
+case MIGRATION_STATUS_POSTCOPY_RECOVER:
case MIGRATION_STATUS_SETUP:
return true;
@@ -626,6 +627,7 @@ MigrationInfo *qmp_query_migrate(Error **errp)
case MIGRATION_STATUS_CANCELLING:
case MIGRATION_STATUS_POSTCOPY_ACTIVE:
case MIGRATION_STATUS_POSTCOPY_PAUSED:
+case MIGRATION_STATUS_POSTCOPY_RECOVER:
/* TODO add some postcopy stats */
info->has_status = true;
info->has_total_time = true;
@@ -2157,6 +2159,13 @@ typedef enum MigThrError {
MIG_THR_ERR_FATAL = 2,
} MigThrError;
+/* Return zero if success, or <0 for error */
+static int postcopy_do_resume(MigrationState *s)
+{
+/* TODO: do the resume logic */
+return 0;
+}
+
/*
* We don't return until we are in a safe state to continue current
* postcopy migration. Returns MIG_THR_ERR_RECOVERED if recovered, or
@@ -2165,29 +2174,55 @@ typedef enum MigThrError {
static MigThrError postcopy_pause(MigrationState *s)
{
assert(s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE);
-migrate_set_state(&s->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
- MIGRATION_STATUS_POSTCOPY_PAUSED);
-/* Current channel is possibly broken. Release it. */
-assert(s->to_dst_file);
-qemu_file_shutdown(s->to_dst_file);
-qemu_fclose(s->to_dst_file);
-s->to_dst_file = NULL;
+while (true) {
+migrate_set_state(&s->state, s->state,
+ MIGRATION_STATUS_POSTCOPY_PAUSED);
-error_report("Detected IO failure for postcopy. "
- "Migration paused.");
+/* Current channel is possibly broken. Release it. */
+assert(s->to_dst_file);
+qemu_file_shutdown(s->to_dst_file);
+qemu_fclose(s->to_dst_file);
+s->to_dst_file = NULL;
-/*
- * We wait until things fixed up. Then someone will setup the
- * status back for us.
- */
-while (s->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
-qemu_sem_wait(&s->postcopy_pause_sem);
-}
+error_report("Detected IO failure for postcopy. "
+ "Migration paused.");
+
+/*
+ * We wait until things fixed up. Then someone will setup the
+ * status back for us.
+ */
+while (s->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
+qemu_sem_wait(&s->postcopy_pause_sem);
+}
-trace_postcopy_pause_continued();
+if (s->state == MIGRATION_STATUS_POSTCOPY_RECOVER) {
+/* Woken up by a recover procedure. Give it a shot */
+
+/*
+ * Firstly, let's wake up the return path now, with a new
+ * return path channel.
+ */
+qemu_sem_post(&s->postcopy_pause_rp_sem);
-return MIG_THR_ERR_RECOVERED;
+/* Do the resume logic */
+if (postcopy_do_resume(s) == 0) {
+/* Let's continue! */
+trace_postcopy_pause_continued();
+return MIG_THR_ERR_RECOVERED;
+} else {
+/*
+ * Something wrong happened during the recovery, let's
+ * pause again. Pause is always better than throwing
+ * data away.
+ */
+continue;
+}
+} else {
+/* This is not right... Time to quit. */
+return MIG_THR_ERR_FATAL;
+}
+}
}
static MigThrError migration_detect_error(MigrationState *s)
@@ -2452,7 +2487,10 @@ void migrate_fd_connect(MigrationState *s)
}
if (resume) {
-/* TODO: do the resume logic */
+/* Wakeup the main migration thread to do the recovery */
+migrate_set_state(&s->state, MIGRATION_STATUS_POSTCOPY_PAUSED,
+ MIGRATION_STATUS_POSTCOPY_RECOVER);
+qemu_sem_post(&s->postcopy_pause_sem);
return;
}
diff --git a/qapi/migration.json b/qapi/migration.jso
[Qemu-devel] [PATCH v3 28/32] migration: allow migrate_incoming for paused VM
migrate_incoming command is previously only used when we were providing
"-incoming defer" in the command line, to defer the incoming migration
channel creation.
However there is similar requirement when we are paused during postcopy
migration. The old incoming channel might have been destroyed already.
We may need another new channel for the recovery to happen.
This patch leveraged the same interface, but allows the user to specify
incoming migration channel even for paused postcopy.
Meanwhile, now migration listening ports are always detached manually
using the tag, rather than using return values of dispatchers.
Signed-off-by: Peter Xu
---
migration/exec.c | 2 +-
migration/fd.c | 2 +-
migration/migration.c | 50 +-
migration/socket.c | 4 +---
migration/trace-events | 2 ++
5 files changed, 46 insertions(+), 14 deletions(-)
diff --git a/migration/exec.c b/migration/exec.c
index a0796c2c70..9d20d10899 100644
--- a/migration/exec.c
+++ b/migration/exec.c
@@ -49,7 +49,7 @@ static gboolean exec_accept_incoming_migration(QIOChannel
*ioc,
{
migration_channel_process_incoming(ioc);
object_unref(OBJECT(ioc));
-return G_SOURCE_REMOVE;
+return G_SOURCE_CONTINUE;
}
/*
diff --git a/migration/fd.c b/migration/fd.c
index 7ead2f26cc..54b36888e2 100644
--- a/migration/fd.c
+++ b/migration/fd.c
@@ -49,7 +49,7 @@ static gboolean fd_accept_incoming_migration(QIOChannel *ioc,
{
migration_channel_process_incoming(ioc);
object_unref(OBJECT(ioc));
-return G_SOURCE_REMOVE;
+return G_SOURCE_CONTINUE;
}
/*
diff --git a/migration/migration.c b/migration/migration.c
index 0baa09ada3..387fbefad4 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -176,6 +176,17 @@ void migration_incoming_state_destroy(void)
qemu_event_reset(&mis->main_thread_load_event);
}
+static bool migrate_incoming_detach_listen(MigrationIncomingState *mis)
+{
+if (mis->listen_task_tag) {
+/* Never fail */
+g_source_remove(mis->listen_task_tag);
+mis->listen_task_tag = 0;
+return true;
+}
+return false;
+}
+
static void migrate_generate_event(int new_state)
{
if (migrate_use_events()) {
@@ -460,10 +471,9 @@ void migration_fd_process_incoming(QEMUFile *f)
/*
* When reach here, we should not need the listening port any
- * more. We'll detach the listening task soon, let's reset the
- * listen task tag.
+ * more. Detach the listening port explicitly.
*/
-mis->listen_task_tag = 0;
+migrate_incoming_detach_listen(mis);
}
void migration_ioc_process_incoming(QIOChannel *ioc)
@@ -1373,13 +1383,35 @@ void qmp_migrate_incoming(const char *uri, Error **errp)
{
Error *local_err = NULL;
static bool once = true;
+MigrationIncomingState *mis = migration_incoming_get_current();
-if (!deferred_incoming) {
-error_setg(errp, "For use with '-incoming defer'");
-return;
-}
-if (!once) {
-error_setg(errp, "The incoming migration has already been started");
+if (mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED &&
+mis->listen_task_tag == 0) {
+/*
+ * We are in postcopy-paused state, and we don't have
+ * listening port. It's very possible that the old listening
+ * port is already gone, so we allow to create a new one.
+ *
+ * NOTE: RDMA migration currently does not really use
+ * listen_task_tag for now, so even if listen_task_tag is
+ * zero, RDMA can still have its accept port listening.
+ * However, RDMA is not supported by postcopy at all (yet), so
+ * we are safe here.
+ */
+trace_migrate_incoming_recover();
+} else if (deferred_incoming) {
+/*
+ * We don't need recovery, but we possibly has a deferred
+ * incoming parameter, this allows us to manually specify
+ * incoming port once.
+ */
+if (!once) {
+error_setg(errp, "The incoming migration has already been
started");
+return;
+} else {
+/* PASS */
+trace_migrate_incoming_deferred();
+}
}
qemu_start_incoming_migration(uri, &local_err);
diff --git a/migration/socket.c b/migration/socket.c
index e8f3325155..54095a80a0 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -155,10 +155,8 @@ out:
if (migration_has_all_channels()) {
/* Close listening socket as its no longer needed */
qio_channel_close(ioc, NULL);
-return G_SOURCE_REMOVE;
-} else {
-return G_SOURCE_CONTINUE;
}
+return G_SOURCE_CONTINUE;
}
diff --git a/migration/trace-events b/migration/trace-events
index 98c2e4de58..65b1c7e459 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -136,6 +136,8 @@ process_incoming_migration_co_end(int ret, int ps) "ret=%d
postcopy-st
[Qemu-devel] [PATCH v3 17/32] migration: new cmd MIG_CMD_POSTCOPY_RESUME
Introducing this new command to be sent when the source VM is ready to
resume the paused migration. What the destination does here is
basically release the fault thread to continue service page faults.
Reviewed-by: Dr. David Alan Gilbert
Signed-off-by: Peter Xu
---
migration/savevm.c | 35 +++
migration/savevm.h | 1 +
migration/trace-events | 2 ++
3 files changed, 38 insertions(+)
diff --git a/migration/savevm.c b/migration/savevm.c
index 2148b198c7..bb6639812b 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -77,6 +77,7 @@ enum qemu_vm_cmd {
MIG_CMD_POSTCOPY_RAM_DISCARD, /* A list of pages to discard that
were previously sent during
precopy but are dirty. */
+MIG_CMD_POSTCOPY_RESUME, /* resume postcopy on dest */
MIG_CMD_PACKAGED, /* Send a wrapped stream within this stream */
MIG_CMD_RECV_BITMAP, /* Request for recved bitmap on dst */
MIG_CMD_MAX
@@ -95,6 +96,7 @@ static struct mig_cmd_args {
[MIG_CMD_POSTCOPY_RUN] = { .len = 0, .name = "POSTCOPY_RUN" },
[MIG_CMD_POSTCOPY_RAM_DISCARD] = {
.len = -1, .name = "POSTCOPY_RAM_DISCARD" },
+[MIG_CMD_POSTCOPY_RESUME] = { .len = 0, .name = "POSTCOPY_RESUME" },
[MIG_CMD_PACKAGED] = { .len = 4, .name = "PACKAGED" },
[MIG_CMD_RECV_BITMAP] = { .len = -1, .name = "RECV_BITMAP" },
[MIG_CMD_MAX] = { .len = -1, .name = "MAX" },
@@ -955,6 +957,12 @@ void qemu_savevm_send_postcopy_run(QEMUFile *f)
qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_RUN, 0, NULL);
}
+void qemu_savevm_send_postcopy_resume(QEMUFile *f)
+{
+trace_savevm_send_postcopy_resume();
+qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_RESUME, 0, NULL);
+}
+
void qemu_savevm_send_recv_bitmap(QEMUFile *f, char *block_name)
{
size_t len;
@@ -1727,6 +1735,30 @@ static int
loadvm_postcopy_handle_run(MigrationIncomingState *mis)
return LOADVM_QUIT;
}
+static int loadvm_postcopy_handle_resume(MigrationIncomingState *mis)
+{
+if (mis->state != MIGRATION_STATUS_POSTCOPY_RECOVER) {
+error_report("%s: illegal resume received", __func__);
+/* Don't fail the load, only for this. */
+return 0;
+}
+
+/*
+ * This means source VM is ready to resume the postcopy migration.
+ * It's time to switch state and release the fault thread to
+ * continue service page faults.
+ */
+migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_RECOVER,
+ MIGRATION_STATUS_POSTCOPY_ACTIVE);
+qemu_sem_post(&mis->postcopy_pause_sem_fault);
+
+trace_loadvm_postcopy_handle_resume();
+
+/* TODO: Tell source that "we are ready" */
+
+return 0;
+}
+
/**
* Immediately following this command is a blob of data containing an embedded
* chunk of migration stream; read it and load it.
@@ -1892,6 +1924,9 @@ static int loadvm_process_command(QEMUFile *f)
case MIG_CMD_POSTCOPY_RAM_DISCARD:
return loadvm_postcopy_ram_handle_discard(mis, len);
+case MIG_CMD_POSTCOPY_RESUME:
+return loadvm_postcopy_handle_resume(mis);
+
case MIG_CMD_RECV_BITMAP:
return loadvm_handle_recv_bitmap(mis, len);
}
diff --git a/migration/savevm.h b/migration/savevm.h
index 8126b1cc14..a5f3879191 100644
--- a/migration/savevm.h
+++ b/migration/savevm.h
@@ -46,6 +46,7 @@ int qemu_savevm_send_packaged(QEMUFile *f, const uint8_t
*buf, size_t len);
void qemu_savevm_send_postcopy_advise(QEMUFile *f);
void qemu_savevm_send_postcopy_listen(QEMUFile *f);
void qemu_savevm_send_postcopy_run(QEMUFile *f);
+void qemu_savevm_send_postcopy_resume(QEMUFile *f);
void qemu_savevm_send_recv_bitmap(QEMUFile *f, char *block_name);
void qemu_savevm_send_postcopy_ram_discard(QEMUFile *f, const char *name,
diff --git a/migration/trace-events b/migration/trace-events
index 3dcf8a93d9..4b60865194 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -18,6 +18,7 @@ loadvm_postcopy_handle_listen(void) ""
loadvm_postcopy_handle_run(void) ""
loadvm_postcopy_handle_run_cpu_sync(void) ""
loadvm_postcopy_handle_run_vmstart(void) ""
+loadvm_postcopy_handle_resume(void) ""
loadvm_postcopy_ram_handle_discard(void) ""
loadvm_postcopy_ram_handle_discard_end(void) ""
loadvm_postcopy_ram_handle_discard_header(const char *ramid, uint16_t len)
"%s: %ud"
@@ -35,6 +36,7 @@ savevm_send_open_return_path(void) ""
savevm_send_ping(uint32_t val) "0x%x"
savevm_send_postcopy_listen(void) ""
savevm_send_postcopy_run(void) ""
+savevm_send_postcopy_resume(void) ""
savevm_send_recv_bitmap(char *name) "%s"
savevm_state_setup(void) ""
savevm_state_header(void) ""
--
2.13.5
[Qemu-devel] [PATCH v3 15/32] migration: new cmd MIG_CMD_RECV_BITMAP
Add a new vm command MIG_CMD_RECV_BITMAP to request received bitmap for
one ramblock.
Reviewed-by: Dr. David Alan Gilbert
Signed-off-by: Peter Xu
---
migration/savevm.c | 61 ++
migration/savevm.h | 1 +
migration/trace-events | 2 ++
3 files changed, 64 insertions(+)
diff --git a/migration/savevm.c b/migration/savevm.c
index 6d6f8ee3e4..0f61da3ebb 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -78,6 +78,7 @@ enum qemu_vm_cmd {
were previously sent during
precopy but are dirty. */
MIG_CMD_PACKAGED, /* Send a wrapped stream within this stream */
+MIG_CMD_RECV_BITMAP, /* Request for recved bitmap on dst */
MIG_CMD_MAX
};
@@ -95,6 +96,7 @@ static struct mig_cmd_args {
[MIG_CMD_POSTCOPY_RAM_DISCARD] = {
.len = -1, .name = "POSTCOPY_RAM_DISCARD" },
[MIG_CMD_PACKAGED] = { .len = 4, .name = "PACKAGED" },
+[MIG_CMD_RECV_BITMAP] = { .len = -1, .name = "RECV_BITMAP" },
[MIG_CMD_MAX] = { .len = -1, .name = "MAX" },
};
@@ -953,6 +955,19 @@ void qemu_savevm_send_postcopy_run(QEMUFile *f)
qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_RUN, 0, NULL);
}
+void qemu_savevm_send_recv_bitmap(QEMUFile *f, char *block_name)
+{
+size_t len;
+char buf[256];
+
+trace_savevm_send_recv_bitmap(block_name);
+
+buf[0] = len = strlen(block_name);
+memcpy(buf + 1, block_name, len);
+
+qemu_savevm_command_send(f, MIG_CMD_RECV_BITMAP, len + 1, (uint8_t *)buf);
+}
+
bool qemu_savevm_state_blocked(Error **errp)
{
SaveStateEntry *se;
@@ -1761,6 +1776,49 @@ static int
loadvm_handle_cmd_packaged(MigrationIncomingState *mis)
}
/*
+ * Handle request that source requests for recved_bitmap on
+ * destination. Payload format:
+ *
+ * len (1 byte) + ramblock_name (<255 bytes)
+ */
+static int loadvm_handle_recv_bitmap(MigrationIncomingState *mis,
+ uint16_t len)
+{
+QEMUFile *file = mis->from_src_file;
+RAMBlock *rb;
+char block_name[256];
+size_t cnt;
+
+cnt = qemu_get_counted_string(file, block_name);
+if (!cnt) {
+error_report("%s: failed to read block name", __func__);
+return -EINVAL;
+}
+
+/* Validate before using the data */
+if (qemu_file_get_error(file)) {
+return qemu_file_get_error(file);
+}
+
+if (len != cnt + 1) {
+error_report("%s: invalid payload length (%d)", __func__, len);
+return -EINVAL;
+}
+
+rb = qemu_ram_block_by_name(block_name);
+if (!rb) {
+error_report("%s: block '%s' not found", __func__, block_name);
+return -EINVAL;
+}
+
+/* TODO: send the bitmap back to source */
+
+trace_loadvm_handle_recv_bitmap(block_name);
+
+return 0;
+}
+
+/*
* Process an incoming 'QEMU_VM_COMMAND'
* 0 just a normal return
* LOADVM_QUIT All good, but exit the loop
@@ -1833,6 +1891,9 @@ static int loadvm_process_command(QEMUFile *f)
case MIG_CMD_POSTCOPY_RAM_DISCARD:
return loadvm_postcopy_ram_handle_discard(mis, len);
+
+case MIG_CMD_RECV_BITMAP:
+return loadvm_handle_recv_bitmap(mis, len);
}
return 0;
diff --git a/migration/savevm.h b/migration/savevm.h
index 295c4a1f2c..8126b1cc14 100644
--- a/migration/savevm.h
+++ b/migration/savevm.h
@@ -46,6 +46,7 @@ int qemu_savevm_send_packaged(QEMUFile *f, const uint8_t
*buf, size_t len);
void qemu_savevm_send_postcopy_advise(QEMUFile *f);
void qemu_savevm_send_postcopy_listen(QEMUFile *f);
void qemu_savevm_send_postcopy_run(QEMUFile *f);
+void qemu_savevm_send_recv_bitmap(QEMUFile *f, char *block_name);
void qemu_savevm_send_postcopy_ram_discard(QEMUFile *f, const char *name,
uint16_t len,
diff --git a/migration/trace-events b/migration/trace-events
index 32f02cbdcc..55c0412aaa 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -12,6 +12,7 @@ loadvm_state_cleanup(void) ""
loadvm_handle_cmd_packaged(unsigned int length) "%u"
loadvm_handle_cmd_packaged_main(int ret) "%d"
loadvm_handle_cmd_packaged_received(int ret) "%d"
+loadvm_handle_recv_bitmap(char *s) "%s"
loadvm_postcopy_handle_advise(void) ""
loadvm_postcopy_handle_listen(void) ""
loadvm_postcopy_handle_run(void) ""
@@ -34,6 +35,7 @@ savevm_send_open_return_path(void) ""
savevm_send_ping(uint32_t val) "0x%x"
savevm_send_postcopy_listen(void) ""
savevm_send_postcopy_run(void) ""
+savevm_send_recv_bitmap(char *name) "%s"
savevm_state_setup(void) ""
savevm_state_header(void) ""
savevm_state_iterate(void) ""
--
2.13.5
[Qemu-devel] [PATCH v3 16/32] migration: new message MIG_RP_MSG_RECV_BITMAP
Introducing new return path message MIG_RP_MSG_RECV_BITMAP to send
received bitmap of ramblock back to source.
This is the reply message of MIG_CMD_RECV_BITMAP, it contains not only
the header (including the ramblock name), and it was appended with the
whole ramblock received bitmap on the destination side.
When the source receives such a reply message (MIG_RP_MSG_RECV_BITMAP),
it parses it, convert it to the dirty bitmap by inverting the bits.
One thing to mention is that, when we send the recv bitmap, we are doing
these things in extra:
- converting the bitmap to little endian, to support when hosts are
using different endianess on src/dst.
- do proper alignment for 8 bytes, to support when hosts are using
different word size (32/64 bits) on src/dst.
Signed-off-by: Peter Xu
---
migration/migration.c | 68 +++
migration/migration.h | 2 +
migration/ram.c| 144 +
migration/ram.h| 3 ++
migration/savevm.c | 2 +-
migration/trace-events | 3 ++
6 files changed, 221 insertions(+), 1 deletion(-)
diff --git a/migration/migration.c b/migration/migration.c
index c869d6ceb5..2e992e891e 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -93,6 +93,7 @@ enum mig_rp_message_type {
MIG_RP_MSG_REQ_PAGES_ID, /* data (start: be64, len: be32, id: string) */
MIG_RP_MSG_REQ_PAGES,/* data (start: be64, len: be32) */
+MIG_RP_MSG_RECV_BITMAP, /* send recved_bitmap back to source */
MIG_RP_MSG_MAX
};
@@ -499,6 +500,45 @@ void migrate_send_rp_pong(MigrationIncomingState *mis,
migrate_send_rp_message(mis, MIG_RP_MSG_PONG, sizeof(buf), &buf);
}
+void migrate_send_rp_recv_bitmap(MigrationIncomingState *mis,
+ char *block_name)
+{
+char buf[512];
+int len;
+int64_t res;
+
+/*
+ * First, we send the header part. It contains only the len of
+ * idstr, and the idstr itself.
+ */
+len = strlen(block_name);
+buf[0] = len;
+memcpy(buf + 1, block_name, len);
+
+if (mis->state != MIGRATION_STATUS_POSTCOPY_RECOVER) {
+error_report("%s: MSG_RP_RECV_BITMAP only used for recovery",
+ __func__);
+return;
+}
+
+migrate_send_rp_message(mis, MIG_RP_MSG_RECV_BITMAP, len + 1, buf);
+
+/*
+ * Next, we dump the received bitmap to the stream.
+ *
+ * TODO: currently we are safe since we are the only one that is
+ * using the to_src_file handle (fault thread is still paused),
+ * and it's ok even not taking the mutex. However the best way is
+ * to take the lock before sending the message header, and release
+ * the lock after sending the bitmap.
+ */
+qemu_mutex_lock(&mis->rp_mutex);
+res = ramblock_recv_bitmap_send(mis->to_src_file, block_name);
+qemu_mutex_unlock(&mis->rp_mutex);
+
+trace_migrate_send_rp_recv_bitmap(block_name, res);
+}
+
MigrationCapabilityStatusList *qmp_query_migrate_capabilities(Error **errp)
{
MigrationCapabilityStatusList *head = NULL;
@@ -1685,6 +1725,7 @@ static struct rp_cmd_args {
[MIG_RP_MSG_PONG] = { .len = 4, .name = "PONG" },
[MIG_RP_MSG_REQ_PAGES] = { .len = 12, .name = "REQ_PAGES" },
[MIG_RP_MSG_REQ_PAGES_ID] = { .len = -1, .name = "REQ_PAGES_ID" },
+[MIG_RP_MSG_RECV_BITMAP]= { .len = -1, .name = "RECV_BITMAP" },
[MIG_RP_MSG_MAX]= { .len = -1, .name = "MAX" },
};
@@ -1729,6 +1770,19 @@ static bool
postcopy_pause_return_path_thread(MigrationState *s)
return true;
}
+static int migrate_handle_rp_recv_bitmap(MigrationState *s, char *block_name)
+{
+RAMBlock *block = qemu_ram_block_by_name(block_name);
+
+if (!block) {
+error_report("%s: invalid block name '%s'", __func__, block_name);
+return -EINVAL;
+}
+
+/* Fetch the received bitmap and refresh the dirty bitmap */
+return ram_dirty_bitmap_reload(s, block);
+}
+
/*
* Handles messages sent on the return path towards the source VM
*
@@ -1834,6 +1888,20 @@ retry:
migrate_handle_rp_req_pages(ms, (char *)&buf[13], start, len);
break;
+case MIG_RP_MSG_RECV_BITMAP:
+if (header_len < 1) {
+error_report("%s: missing block name", __func__);
+mark_source_rp_bad(ms);
+goto out;
+}
+/* Format: len (1B) + idstr (<255B). This ends the idstr. */
+buf[buf[0] + 1] = '\0';
+if (migrate_handle_rp_recv_bitmap(ms, (char *)(buf + 1))) {
+mark_source_rp_bad(ms);
+goto out;
+}
+break;
+
default:
break;
}
diff --git a/migration/migration.h b/migration/migration.h
index 9502b2aad6..56ac50839e 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -211,5 +211,7 @@ void migrate_send_rp_pong(MigrationIncomingState *mi
[Qemu-devel] [PATCH v3 32/32] migration, hmp: new command "migrate_pause"
HMP version of QMP "migrate-pause".
Signed-off-by: Peter Xu
---
hmp-commands.hx | 14 ++
hmp.c | 9 +
hmp.h | 1 +
3 files changed, 24 insertions(+)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 7adb029b34..bfba3ea977 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -980,6 +980,20 @@ as the -incoming option).
ETEXI
{
+.name = "migrate_pause",
+.args_type = "",
+.params = "",
+.help = "Pause a migration stream (only supported by postcopy)",
+.cmd= hmp_migrate_pause,
+},
+
+STEXI
+@item migrate_pause
+@findex migrate_pause
+Pause an existing migration manually. Currently it only support postcopy.
+ETEXI
+
+{
.name = "migrate_set_cache_size",
.args_type = "value:o",
.params = "value",
diff --git a/hmp.c b/hmp.c
index 7b1abcd535..689092f534 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1500,6 +1500,15 @@ void hmp_migrate_incoming(Monitor *mon, const QDict
*qdict)
hmp_handle_error(mon, &err);
}
+void hmp_migrate_pause(Monitor *mon, const QDict *qdict)
+{
+Error *err = NULL;
+
+qmp_migrate_pause(&err);
+
+hmp_handle_error(mon, &err);
+}
+
/* Kept for backwards compatibility */
void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict)
{
diff --git a/hmp.h b/hmp.h
index 3605003e4c..2c1dab6b8f 100644
--- a/hmp.h
+++ b/hmp.h
@@ -69,6 +69,7 @@ void hmp_delvm(Monitor *mon, const QDict *qdict);
void hmp_info_snapshots(Monitor *mon, const QDict *qdict);
void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
void hmp_migrate_incoming(Monitor *mon, const QDict *qdict);
+void hmp_migrate_pause(Monitor *mon, const QDict *qdict);
void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict);
void hmp_migrate_set_capability(Monitor *mon, const QDict *qdict);
--
2.13.5
[Qemu-devel] [PATCH v3 19/32] migration: introduce SaveVMHandlers.resume_prepare
This is hook function to be called when a postcopy migration wants to
resume from a failure. For each module, it should provide its own
recovery logic before we switch to the postcopy-active state.
Reviewed-by: Dr. David Alan Gilbert
Signed-off-by: Peter Xu
---
include/migration/register.h | 2 ++
migration/migration.c| 20 +++-
migration/savevm.c | 25 +
migration/savevm.h | 1 +
migration/trace-events | 1 +
5 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/include/migration/register.h b/include/migration/register.h
index f4f7bdc177..128124f008 100644
--- a/include/migration/register.h
+++ b/include/migration/register.h
@@ -42,6 +42,8 @@ typedef struct SaveVMHandlers {
LoadStateHandler *load_state;
int (*load_setup)(QEMUFile *f, void *opaque);
int (*load_cleanup)(void *opaque);
+/* Called when postcopy migration wants to resume from failure */
+int (*resume_prepare)(MigrationState *s, void *opaque);
} SaveVMHandlers;
int register_savevm_live(DeviceState *dev,
diff --git a/migration/migration.c b/migration/migration.c
index 49117111ae..3762e69eb3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2293,7 +2293,25 @@ typedef enum MigThrError {
/* Return zero if success, or <0 for error */
static int postcopy_do_resume(MigrationState *s)
{
-/* TODO: do the resume logic */
+int ret;
+
+/*
+ * Call all the resume_prepare() hooks, so that modules can be
+ * ready for the migration resume.
+ */
+ret = qemu_savevm_state_resume_prepare(s);
+if (ret) {
+error_report("%s: resume_prepare() failure detected: %d",
+ __func__, ret);
+return ret;
+}
+
+/*
+ * TODO: handshake with dest using MIG_CMD_RESUME,
+ * MIG_RP_MSG_RESUME_ACK, then switch source state to
+ * "postcopy-active"
+ */
+
return 0;
}
diff --git a/migration/savevm.c b/migration/savevm.c
index 611b3f1a09..bc87b0e5b1 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1028,6 +1028,31 @@ void qemu_savevm_state_setup(QEMUFile *f)
}
}
+int qemu_savevm_state_resume_prepare(MigrationState *s)
+{
+SaveStateEntry *se;
+int ret;
+
+trace_savevm_state_resume_prepare();
+
+QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
+if (!se->ops || !se->ops->resume_prepare) {
+continue;
+}
+if (se->ops && se->ops->is_active) {
+if (!se->ops->is_active(se->opaque)) {
+continue;
+}
+}
+ret = se->ops->resume_prepare(s, se->opaque);
+if (ret < 0) {
+return ret;
+}
+}
+
+return 0;
+}
+
/*
* this function has three return values:
* negative: there was one error, and we have -errno.
diff --git a/migration/savevm.h b/migration/savevm.h
index a5f3879191..3193f04cca 100644
--- a/migration/savevm.h
+++ b/migration/savevm.h
@@ -31,6 +31,7 @@
bool qemu_savevm_state_blocked(Error **errp);
void qemu_savevm_state_setup(QEMUFile *f);
+int qemu_savevm_state_resume_prepare(MigrationState *s);
void qemu_savevm_state_header(QEMUFile *f);
int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy);
void qemu_savevm_state_cleanup(void);
diff --git a/migration/trace-events b/migration/trace-events
index 2bf8301293..eadabf03e8 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -39,6 +39,7 @@ savevm_send_postcopy_run(void) ""
savevm_send_postcopy_resume(void) ""
savevm_send_recv_bitmap(char *name) "%s"
savevm_state_setup(void) ""
+savevm_state_resume_prepare(void) ""
savevm_state_header(void) ""
savevm_state_iterate(void) ""
savevm_state_cleanup(void) ""
--
2.13.5
[Qemu-devel] [PATCH v3 18/32] migration: new message MIG_RP_MSG_RESUME_ACK
Creating new message to reply for MIG_CMD_POSTCOPY_RESUME. One uint32_t
is used as payload to let the source know whether destination is ready
to continue the migration.
Reviewed-by: Dr. David Alan Gilbert
Signed-off-by: Peter Xu
---
migration/migration.c | 37 +
migration/migration.h | 3 +++
migration/savevm.c | 3 ++-
migration/trace-events | 1 +
4 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/migration/migration.c b/migration/migration.c
index 2e992e891e..49117111ae 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -94,6 +94,7 @@ enum mig_rp_message_type {
MIG_RP_MSG_REQ_PAGES_ID, /* data (start: be64, len: be32, id: string) */
MIG_RP_MSG_REQ_PAGES,/* data (start: be64, len: be32) */
MIG_RP_MSG_RECV_BITMAP, /* send recved_bitmap back to source */
+MIG_RP_MSG_RESUME_ACK, /* tell source that we are ready to resume */
MIG_RP_MSG_MAX
};
@@ -539,6 +540,14 @@ void migrate_send_rp_recv_bitmap(MigrationIncomingState
*mis,
trace_migrate_send_rp_recv_bitmap(block_name, res);
}
+void migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value)
+{
+uint32_t buf;
+
+buf = cpu_to_be32(value);
+migrate_send_rp_message(mis, MIG_RP_MSG_RESUME_ACK, sizeof(buf), &buf);
+}
+
MigrationCapabilityStatusList *qmp_query_migrate_capabilities(Error **errp)
{
MigrationCapabilityStatusList *head = NULL;
@@ -1726,6 +1735,7 @@ static struct rp_cmd_args {
[MIG_RP_MSG_REQ_PAGES] = { .len = 12, .name = "REQ_PAGES" },
[MIG_RP_MSG_REQ_PAGES_ID] = { .len = -1, .name = "REQ_PAGES_ID" },
[MIG_RP_MSG_RECV_BITMAP]= { .len = -1, .name = "RECV_BITMAP" },
+[MIG_RP_MSG_RESUME_ACK] = { .len = 4, .name = "RESUME_ACK" },
[MIG_RP_MSG_MAX]= { .len = -1, .name = "MAX" },
};
@@ -1783,6 +1793,25 @@ static int migrate_handle_rp_recv_bitmap(MigrationState
*s, char *block_name)
return ram_dirty_bitmap_reload(s, block);
}
+static int migrate_handle_rp_resume_ack(MigrationState *s, uint32_t value)
+{
+trace_source_return_path_thread_resume_ack(value);
+
+if (value != MIGRATION_RESUME_ACK_VALUE) {
+error_report("%s: illegal resume_ack value %"PRIu32,
+ __func__, value);
+return -1;
+}
+
+/* Now both sides are active. */
+migrate_set_state(&s->state, MIGRATION_STATUS_POSTCOPY_RECOVER,
+ MIGRATION_STATUS_POSTCOPY_ACTIVE);
+
+/* TODO: notify send thread that time to continue send pages */
+
+return 0;
+}
+
/*
* Handles messages sent on the return path towards the source VM
*
@@ -1902,6 +1931,14 @@ retry:
}
break;
+case MIG_RP_MSG_RESUME_ACK:
+tmp32 = ldl_be_p(buf);
+if (migrate_handle_rp_resume_ack(ms, tmp32)) {
+mark_source_rp_bad(ms);
+goto out;
+}
+break;
+
default:
break;
}
diff --git a/migration/migration.h b/migration/migration.h
index 56ac50839e..feeb1997a2 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -22,6 +22,8 @@
#include "hw/qdev.h"
#include "io/channel.h"
+#define MIGRATION_RESUME_ACK_VALUE (1)
+
/* State for the incoming migration */
struct MigrationIncomingState {
QEMUFile *from_src_file;
@@ -213,5 +215,6 @@ int migrate_send_rp_req_pages(MigrationIncomingState *mis,
const char* rbname,
ram_addr_t start, size_t len);
void migrate_send_rp_recv_bitmap(MigrationIncomingState *mis,
char *block_name);
+void migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value);
#endif
diff --git a/migration/savevm.c b/migration/savevm.c
index bb6639812b..611b3f1a09 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1754,7 +1754,8 @@ static int
loadvm_postcopy_handle_resume(MigrationIncomingState *mis)
trace_loadvm_postcopy_handle_resume();
-/* TODO: Tell source that "we are ready" */
+/* Tell source that "we are ready" */
+migrate_send_rp_resume_ack(mis, MIGRATION_RESUME_ACK_VALUE);
return 0;
}
diff --git a/migration/trace-events b/migration/trace-events
index 4b60865194..2bf8301293 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -120,6 +120,7 @@ source_return_path_thread_entry(void) ""
source_return_path_thread_loop_top(void) ""
source_return_path_thread_pong(uint32_t val) "0x%x"
source_return_path_thread_shut(uint32_t val) "0x%x"
+source_return_path_thread_resume_ack(uint32_t v) "%"PRIu32
migrate_global_state_post_load(const char *state) "loaded state: %s"
migrate_global_state_pre_save(const char *state) "saved state: %s"
migration_thread_low_pending(uint64_t pending) "%" PRIu64
--
2.13.5
[Qemu-devel] [PATCH v3 22/32] migration: final handshake for the resume
Finish the last step to do the final handshake for the recovery.
First source sends one MIG_CMD_RESUME to dst, telling that source is
ready to resume.
Then, dest replies with MIG_RP_MSG_RESUME_ACK to source, telling that
dest is ready to resume (after switch to postcopy-active state).
When source received the RESUME_ACK, it switches its state to
postcopy-active, and finally the recovery is completed.
Reviewed-by: Dr. David Alan Gilbert
Signed-off-by: Peter Xu
---
migration/migration.c | 28
1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 52f81c3add..edf7365b69 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1807,7 +1807,8 @@ static int migrate_handle_rp_resume_ack(MigrationState
*s, uint32_t value)
migrate_set_state(&s->state, MIGRATION_STATUS_POSTCOPY_RECOVER,
MIGRATION_STATUS_POSTCOPY_ACTIVE);
-/* TODO: notify send thread that time to continue send pages */
+/* Notify send thread that time to continue send pages */
+qemu_sem_post(&s->rp_state.rp_sem);
return 0;
}
@@ -2290,6 +2291,21 @@ typedef enum MigThrError {
MIG_THR_ERR_FATAL = 2,
} MigThrError;
+static int postcopy_resume_handshake(MigrationState *s)
+{
+qemu_savevm_send_postcopy_resume(s->to_dst_file);
+
+while (s->state == MIGRATION_STATUS_POSTCOPY_RECOVER) {
+qemu_sem_wait(&s->rp_state.rp_sem);
+}
+
+if (s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) {
+return 0;
+}
+
+return -1;
+}
+
/* Return zero if success, or <0 for error */
static int postcopy_do_resume(MigrationState *s)
{
@@ -2307,10 +2323,14 @@ static int postcopy_do_resume(MigrationState *s)
}
/*
- * TODO: handshake with dest using MIG_CMD_RESUME,
- * MIG_RP_MSG_RESUME_ACK, then switch source state to
- * "postcopy-active"
+ * Last handshake with destination on the resume (destination will
+ * switch to postcopy-active afterwards)
*/
+ret = postcopy_resume_handshake(s);
+if (ret) {
+error_report("%s: handshake failed: %d", __func__, ret);
+return ret;
+}
return 0;
}
--
2.13.5
[Qemu-devel] [PATCH v3 20/32] migration: synchronize dirty bitmap for resume
This patch implements the first part of core RAM resume logic for
postcopy. ram_resume_prepare() is provided for the work.
When the migration is interrupted by network failure, the dirty bitmap
on the source side will be meaningless, because even the dirty bit is
cleared, it is still possible that the sent page was lost along the way
to destination. Here instead of continue the migration with the old
dirty bitmap on source, we ask the destination side to send back its
received bitmap, then invert it to be our initial dirty bitmap.
The source side send thread will issue the MIG_CMD_RECV_BITMAP requests,
once per ramblock, to ask for the received bitmap. On destination side,
MIG_RP_MSG_RECV_BITMAP will be issued, along with the requested bitmap.
Data will be received on the return-path thread of source, and the main
migration thread will be notified when all the ramblock bitmaps are
synchronized.
Signed-off-by: Peter Xu
---
migration/migration.c | 4
migration/migration.h | 1 +
migration/ram.c| 47 +++
migration/trace-events | 4
4 files changed, 56 insertions(+)
diff --git a/migration/migration.c b/migration/migration.c
index 3762e69eb3..52f81c3add 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2739,6 +2739,8 @@ static void migration_instance_finalize(Object *obj)
g_free(params->tls_hostname);
g_free(params->tls_creds);
+
+qemu_sem_destroy(&ms->rp_state.rp_sem);
}
static void migration_instance_init(Object *obj)
@@ -2765,6 +2767,8 @@ static void migration_instance_init(Object *obj)
params->has_block_incremental = true;
params->has_x_multifd_channels = true;
params->has_x_multifd_page_count = true;
+
+qemu_sem_init(&ms->rp_state.rp_sem, 0);
}
/*
diff --git a/migration/migration.h b/migration/migration.h
index feeb1997a2..c8d5939d43 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -108,6 +108,7 @@ struct MigrationState
QEMUFile *from_dst_file;
QemuThreadrp_thread;
bool error;
+QemuSemaphore rp_sem;
} rp_state;
double mbps;
diff --git a/migration/ram.c b/migration/ram.c
index fa2b4ed207..a7431a802e 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -48,6 +48,7 @@
#include "qemu/rcu_queue.h"
#include "migration/colo.h"
#include "migration/block.h"
+#include "savevm.h"
/***/
/* ram save/restore */
@@ -2984,6 +2985,38 @@ static bool ram_has_postcopy(void *opaque)
return migrate_postcopy_ram();
}
+/* Sync all the dirty bitmap with destination VM. */
+static int ram_dirty_bitmap_sync_all(MigrationState *s, RAMState *rs)
+{
+RAMBlock *block;
+QEMUFile *file = s->to_dst_file;
+int ramblock_count = 0;
+
+trace_ram_dirty_bitmap_sync_start();
+
+RAMBLOCK_FOREACH(block) {
+qemu_savevm_send_recv_bitmap(file, block->idstr);
+trace_ram_dirty_bitmap_request(block->idstr);
+ramblock_count++;
+}
+
+trace_ram_dirty_bitmap_sync_wait();
+
+/* Wait until all the ramblocks' dirty bitmap synced */
+while (ramblock_count--) {
+qemu_sem_wait(&s->rp_state.rp_sem);
+}
+
+trace_ram_dirty_bitmap_sync_complete();
+
+return 0;
+}
+
+static void ram_dirty_bitmap_reload_notify(MigrationState *s)
+{
+qemu_sem_post(&s->rp_state.rp_sem);
+}
+
/*
* Read the received bitmap, revert it as the initial dirty bitmap.
* This is only used when the postcopy migration is paused but wants
@@ -3058,12 +3091,25 @@ int ram_dirty_bitmap_reload(MigrationState *s, RAMBlock
*block)
trace_ram_dirty_bitmap_reload_complete(block->idstr);
+/*
+ * We succeeded to sync bitmap for current ramblock. If this is
+ * the last one to sync, we need to notify the main send thread.
+ */
+ram_dirty_bitmap_reload_notify(s);
+
ret = 0;
out:
free(le_bitmap);
return ret;
}
+static int ram_resume_prepare(MigrationState *s, void *opaque)
+{
+RAMState *rs = *(RAMState **)opaque;
+
+return ram_dirty_bitmap_sync_all(s, rs);
+}
+
static SaveVMHandlers savevm_ram_handlers = {
.save_setup = ram_save_setup,
.save_live_iterate = ram_save_iterate,
@@ -3075,6 +3121,7 @@ static SaveVMHandlers savevm_ram_handlers = {
.save_cleanup = ram_save_cleanup,
.load_setup = ram_load_setup,
.load_cleanup = ram_load_cleanup,
+.resume_prepare = ram_resume_prepare,
};
void ram_mig_init(void)
diff --git a/migration/trace-events b/migration/trace-events
index eadabf03e8..804f18d492 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -82,8 +82,12 @@ ram_load_postcopy_loop(uint64_t addr, int flags) "@%" PRIx64
" %x"
ram_postcopy_send_discard_bitmap(void) ""
ram_save_page(const char *rbname, uint64_t offset, void *host) "%s: offset:
0x%" PRIx64 " host: %p"
ram_save_queue_pages(const char *rbname, size_t start, size_t len) "%s: sta
[Qemu-devel] [PATCH v3 25/32] migration: return incoming task tag for exec
Return the async task tag for exec typed incoming migration in
exec_start_incoming_migration().
Reviewed-by: Dr. David Alan Gilbert
Signed-off-by: Peter Xu
---
migration/exec.c | 18 +++---
migration/exec.h | 2 +-
2 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/migration/exec.c b/migration/exec.c
index f3be1baf2e..a0796c2c70 100644
--- a/migration/exec.c
+++ b/migration/exec.c
@@ -52,7 +52,11 @@ static gboolean exec_accept_incoming_migration(QIOChannel
*ioc,
return G_SOURCE_REMOVE;
}
-void exec_start_incoming_migration(const char *command, Error **errp)
+/*
+ * Returns the tag ID of the watch that is attached to global main
+ * loop (>0), or zero if failure detected.
+ */
+guint exec_start_incoming_migration(const char *command, Error **errp)
{
QIOChannel *ioc;
const char *argv[] = { "/bin/sh", "-c", command, NULL };
@@ -62,13 +66,13 @@ void exec_start_incoming_migration(const char *command,
Error **errp)
O_RDWR,
errp));
if (!ioc) {
-return;
+return 0;
}
qio_channel_set_name(ioc, "migration-exec-incoming");
-qio_channel_add_watch(ioc,
- G_IO_IN,
- exec_accept_incoming_migration,
- NULL,
- NULL);
+return qio_channel_add_watch(ioc,
+ G_IO_IN,
+ exec_accept_incoming_migration,
+ NULL,
+ NULL);
}
diff --git a/migration/exec.h b/migration/exec.h
index b210ffde7a..0a7aadacd3 100644
--- a/migration/exec.h
+++ b/migration/exec.h
@@ -19,7 +19,7 @@
#ifndef QEMU_MIGRATION_EXEC_H
#define QEMU_MIGRATION_EXEC_H
-void exec_start_incoming_migration(const char *host_port, Error **errp);
+guint exec_start_incoming_migration(const char *host_port, Error **errp);
void exec_start_outgoing_migration(MigrationState *s, const char *host_port,
Error **errp);
--
2.13.5
Re: [Qemu-devel] [PATCH v3 31/32] migration, qmp: new command "migrate-pause"
On Mon, Oct 16, 2017 at 02:52:15PM +0800, Peter Xu wrote: [...] > +# @migrate-pause: > +# > +# Pause an migration. Currently it can only pause a postcopy > +# migration. Pausing a precopy migration is not supported yet. > +# > +# It is mostly used as a manual way to trigger the postcopy paused > +# state when the network sockets hang due to some reason, so that we > +# can try a recovery afterward. > +# > +# Returns: nothing on success > +# > +# Since: 2.10 Ouch! I believe this should be for 2.11. -- Peter Xu
[Qemu-devel] [PATCH v3 23/32] migration: free SocketAddress where allocated
Freeing the SocketAddress struct in socket_start_incoming_migration is
slightly confusing. Let's free the address in the same context where we
allocated it.
Reviewed-by: Dr. David Alan Gilbert
Signed-off-by: Peter Xu
---
migration/socket.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/migration/socket.c b/migration/socket.c
index dee869044a..4879f11e0f 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -172,7 +172,6 @@ static void socket_start_incoming_migration(SocketAddress
*saddr,
if (qio_channel_socket_listen_sync(listen_ioc, saddr, errp) < 0) {
object_unref(OBJECT(listen_ioc));
-qapi_free_SocketAddress(saddr);
return;
}
@@ -181,7 +180,6 @@ static void socket_start_incoming_migration(SocketAddress
*saddr,
socket_accept_incoming_migration,
listen_ioc,
(GDestroyNotify)object_unref);
-qapi_free_SocketAddress(saddr);
}
void tcp_start_incoming_migration(const char *host_port, Error **errp)
@@ -192,10 +190,12 @@ void tcp_start_incoming_migration(const char *host_port,
Error **errp)
socket_start_incoming_migration(saddr, &err);
}
error_propagate(errp, err);
+qapi_free_SocketAddress(saddr);
}
void unix_start_incoming_migration(const char *path, Error **errp)
{
SocketAddress *saddr = unix_build_address(path);
socket_start_incoming_migration(saddr, errp);
+qapi_free_SocketAddress(saddr);
}
--
2.13.5
[Qemu-devel] [PATCH v3 26/32] migration: return incoming task tag for fd
Allow to return the task tag in fd_start_incoming_migration().
Reviewed-by: Dr. David Alan Gilbert
Signed-off-by: Peter Xu
---
migration/fd.c | 18 +++---
migration/fd.h | 2 +-
2 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/migration/fd.c b/migration/fd.c
index 30de4b9847..7ead2f26cc 100644
--- a/migration/fd.c
+++ b/migration/fd.c
@@ -52,7 +52,11 @@ static gboolean fd_accept_incoming_migration(QIOChannel *ioc,
return G_SOURCE_REMOVE;
}
-void fd_start_incoming_migration(const char *infd, Error **errp)
+/*
+ * Returns the tag ID of the watch that is attached to global main
+ * loop (>0), or zero if failure detected.
+ */
+guint fd_start_incoming_migration(const char *infd, Error **errp)
{
QIOChannel *ioc;
int fd;
@@ -63,13 +67,13 @@ void fd_start_incoming_migration(const char *infd, Error
**errp)
ioc = qio_channel_new_fd(fd, errp);
if (!ioc) {
close(fd);
-return;
+return 0;
}
qio_channel_set_name(QIO_CHANNEL(ioc), "migration-fd-incoming");
-qio_channel_add_watch(ioc,
- G_IO_IN,
- fd_accept_incoming_migration,
- NULL,
- NULL);
+return qio_channel_add_watch(ioc,
+ G_IO_IN,
+ fd_accept_incoming_migration,
+ NULL,
+ NULL);
}
diff --git a/migration/fd.h b/migration/fd.h
index a14a63ce2e..94cdea87d8 100644
--- a/migration/fd.h
+++ b/migration/fd.h
@@ -16,7 +16,7 @@
#ifndef QEMU_MIGRATION_FD_H
#define QEMU_MIGRATION_FD_H
-void fd_start_incoming_migration(const char *path, Error **errp);
+guint fd_start_incoming_migration(const char *path, Error **errp);
void fd_start_outgoing_migration(MigrationState *s, const char *fdname,
Error **errp);
--
2.13.5
[Qemu-devel] [PATCH v3 21/32] migration: setup ramstate for resume
After we updated the dirty bitmaps of ramblocks, we also need to update
the critical fields in RAMState to make sure it is ready for a resume.
Reviewed-by: Dr. David Alan Gilbert
Signed-off-by: Peter Xu
---
migration/ram.c| 37 -
migration/trace-events | 1 +
2 files changed, 37 insertions(+), 1 deletion(-)
diff --git a/migration/ram.c b/migration/ram.c
index a7431a802e..79c52631b9 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2185,6 +2185,33 @@ static int ram_state_init(RAMState **rsp)
return 0;
}
+static void ram_state_resume_prepare(RAMState *rs)
+{
+RAMBlock *block;
+long pages = 0;
+
+/*
+ * Postcopy is not using xbzrle/compression, so no need for that.
+ * Also, since source are already halted, we don't need to care
+ * about dirty page logging as well.
+ */
+
+RAMBLOCK_FOREACH(block) {
+pages += bitmap_count_one(block->bmap,
+ block->used_length >> TARGET_PAGE_BITS);
+}
+
+/* This may not be aligned with current bitmaps. Recalculate. */
+rs->migration_dirty_pages = pages;
+
+rs->last_seen_block = NULL;
+rs->last_sent_block = NULL;
+rs->last_page = 0;
+rs->last_version = ram_list.version;
+
+trace_ram_state_resume_prepare(pages);
+}
+
/*
* Each of ram_save_setup, ram_save_iterate and ram_save_complete has
* long-running RCU critical section. When rcu-reclaims in the code
@@ -3106,8 +3133,16 @@ out:
static int ram_resume_prepare(MigrationState *s, void *opaque)
{
RAMState *rs = *(RAMState **)opaque;
+int ret;
-return ram_dirty_bitmap_sync_all(s, rs);
+ret = ram_dirty_bitmap_sync_all(s, rs);
+if (ret) {
+return ret;
+}
+
+ram_state_resume_prepare(rs);
+
+return 0;
}
static SaveVMHandlers savevm_ram_handlers = {
diff --git a/migration/trace-events b/migration/trace-events
index 804f18d492..98c2e4de58 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -88,6 +88,7 @@ ram_dirty_bitmap_reload_complete(char *str) "%s"
ram_dirty_bitmap_sync_start(void) ""
ram_dirty_bitmap_sync_wait(void) ""
ram_dirty_bitmap_sync_complete(void) ""
+ram_state_resume_prepare(long v) "%ld"
# migration/migration.c
await_return_path_close_on_source_close(void) ""
--
2.13.5
[Qemu-devel] [PATCH v3 27/32] migration: store listen task tag
Store the task tag for migration types: tcp/unix/fd/exec in current
MigrationIncomingState struct.
For defered migration, no need to store task tag since there is no task
running in the main loop at all. For RDMA, let's mark it as todo.
Reviewed-by: Dr. David Alan Gilbert
Signed-off-by: Peter Xu
---
migration/migration.c | 22 ++
migration/migration.h | 2 ++
2 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index edf7365b69..0baa09ada3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -172,6 +172,7 @@ void migration_incoming_state_destroy(void)
mis->from_src_file = NULL;
}
+mis->listen_task_tag = 0;
qemu_event_reset(&mis->main_thread_load_event);
}
@@ -266,25 +267,31 @@ int migrate_send_rp_req_pages(MigrationIncomingState
*mis, const char *rbname,
void qemu_start_incoming_migration(const char *uri, Error **errp)
{
const char *p;
+guint task_tag = 0;
+MigrationIncomingState *mis = migration_incoming_get_current();
qapi_event_send_migration(MIGRATION_STATUS_SETUP, &error_abort);
if (!strcmp(uri, "defer")) {
deferred_incoming_migration(errp);
} else if (strstart(uri, "tcp:", &p)) {
-tcp_start_incoming_migration(p, errp);
+task_tag = tcp_start_incoming_migration(p, errp);
#ifdef CONFIG_RDMA
} else if (strstart(uri, "rdma:", &p)) {
+/* TODO: store task tag for RDMA migrations */
rdma_start_incoming_migration(p, errp);
#endif
} else if (strstart(uri, "exec:", &p)) {
-exec_start_incoming_migration(p, errp);
+task_tag = exec_start_incoming_migration(p, errp);
} else if (strstart(uri, "unix:", &p)) {
-unix_start_incoming_migration(p, errp);
+task_tag = unix_start_incoming_migration(p, errp);
} else if (strstart(uri, "fd:", &p)) {
-fd_start_incoming_migration(p, errp);
+task_tag = fd_start_incoming_migration(p, errp);
} else {
error_setg(errp, "unknown migration protocol: %s", uri);
+return;
}
+
+mis->listen_task_tag = task_tag;
}
static void process_incoming_migration_bh(void *opaque)
@@ -450,6 +457,13 @@ void migration_fd_process_incoming(QEMUFile *f)
migration_incoming_setup(f);
migration_incoming_process();
}
+
+/*
+ * When reach here, we should not need the listening port any
+ * more. We'll detach the listening task soon, let's reset the
+ * listen task tag.
+ */
+mis->listen_task_tag = 0;
}
void migration_ioc_process_incoming(QIOChannel *ioc)
diff --git a/migration/migration.h b/migration/migration.h
index c8d5939d43..d89a0387cf 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -27,6 +27,8 @@
/* State for the incoming migration */
struct MigrationIncomingState {
QEMUFile *from_src_file;
+/* Task tag for incoming listen port. Valid when >0. */
+guint listen_task_tag;
/*
* Free at the start of the main state load, set as the main thread
finishes
--
2.13.5
[Qemu-devel] [FIX PATCH] monitor: Don't return CPU marked for unplug as monitor CPU
The following sequence of steps kill the QEMU:
- Hotplug a CPU
- Change the default CPU to the newly hotplugged cpu using "cpu" HMP command.
- Hot unplug the CPU
- Run "info cpus"
Fix this by not letting monitor_get_cpu() to return a CPU which is marked
for unplug.
Reported-by: Satheesh Rajendran
Signed-off-by: Bharata B Rao
---
monitor.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/monitor.c b/monitor.c
index fe0d1bd..8d60e57 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1053,7 +1053,7 @@ int monitor_set_cpu(int cpu_index)
CPUState *mon_get_cpu(void)
{
-if (!cur_mon->mon_cpu) {
+if (!cur_mon->mon_cpu || cur_mon->mon_cpu->unplug) {
if (!first_cpu) {
return NULL;
}
--
2.7.4
Re: [Qemu-devel] [PATCH v2 00/30] s390x: SMP for TCG
On 11.10.2017 14:07, Cornelia Huck wrote: > On Thu, 28 Sep 2017 22:36:38 +0200 > David Hildenbrand wrote: > >> This series is based on a couple of other patches floating around on >> the mailing list (e.g. cleanups and LAP). The fist patches of v1 have >> been sent as a separate series. >> >> As I am working on kvm-unit-tests for SIGP, I discovered some bugs that >> have been fixed in this series. >> >> == >> I'll be on vacation until 15. October. >> == >> >> This series contains: >> - properly implement local external interrupts for TCG. Take care of >> interrupt masks. Cleanup service interrupt handling. >> - factor out KVM SIGP handling code into common code >> - implement missing SIGP orders for TCG handled by the kernel for KVM >> (including STOP and RESTART interrupts) >> - make TCG use the new SIGP code - experimental SMP support for s390x TCG >> - refactor STFL(E) implementation for TCG >> - bunch of cleanups >> >> Basically all SIGP instructions are fully supported. >> >> Thanks to Aurelien Jarno for the initital prototype and showcasing that >> supporting experimental SMP code can be implemented quite easily. >> > > Thanks, applied (with some minor changes, see the individual patches). > Thanks, all looks sane. >> CPU hotplug does still not work correctly. > > Do you have an idea what still needs to be done? > Nope, no idea yet. As the hotplugged CPU starts running but somehow produces a deadlock, however ordinary onlining/offlining of CPUs works, this could be a common code problem. If I find further problems using my kvm-unit-tests/testing, I'll send fixes. -- Thanks, David
[Qemu-devel] [PATCH v3 29/32] migration: init dst in migration_object_init too
Though we may not need it, now we init both the src/dst migration
objects in migration_object_init() so that even incoming migration
object would be thread safe (it was not).
Reviewed-by: Dr. David Alan Gilbert
Signed-off-by: Peter Xu
---
migration/migration.c | 28 +++-
1 file changed, 15 insertions(+), 13 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 387fbefad4..51e771685c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -104,6 +104,7 @@ enum mig_rp_message_type {
dynamic creation of migration */
static MigrationState *current_migration;
+static MigrationIncomingState *current_incoming;
static bool migration_object_check(MigrationState *ms, Error **errp);
@@ -116,6 +117,18 @@ void migration_object_init(void)
assert(!current_migration);
current_migration = MIGRATION_OBJ(object_new(TYPE_MIGRATION));
+/*
+ * Init the migrate incoming object as well no matter whether
+ * we'll use it or not.
+ */
+assert(!current_incoming);
+current_incoming = g_new0(MigrationIncomingState, 1);
+current_incoming->state = MIGRATION_STATUS_NONE;
+qemu_mutex_init(¤t_incoming->rp_mutex);
+qemu_event_init(¤t_incoming->main_thread_load_event, false);
+qemu_sem_init(¤t_incoming->postcopy_pause_sem_dst, 0);
+qemu_sem_init(¤t_incoming->postcopy_pause_sem_fault, 0);
+
if (!migration_object_check(current_migration, &err)) {
error_report_err(err);
exit(1);
@@ -141,19 +154,8 @@ MigrationState *migrate_get_current(void)
MigrationIncomingState *migration_incoming_get_current(void)
{
-static bool once;
-static MigrationIncomingState mis_current;
-
-if (!once) {
-mis_current.state = MIGRATION_STATUS_NONE;
-memset(&mis_current, 0, sizeof(MigrationIncomingState));
-qemu_mutex_init(&mis_current.rp_mutex);
-qemu_event_init(&mis_current.main_thread_load_event, false);
-qemu_sem_init(&mis_current.postcopy_pause_sem_dst, 0);
-qemu_sem_init(&mis_current.postcopy_pause_sem_fault, 0);
-once = true;
-}
-return &mis_current;
+assert(current_incoming);
+return current_incoming;
}
void migration_incoming_state_destroy(void)
--
2.13.5
[Qemu-devel] [PATCH v3 30/32] migration: delay the postcopy-active state switch
Switch the state until we try to start the VM on destination side. The problem is that without doing this we may have a very small window that we'll be in such a state: - dst VM is in postcopy-active state, - main thread is handling MIG_CMD_PACKAGED message, which loads all the device states, - ram load thread is reading memory data from source. Then if we failed at this point when reading the migration stream we'll also switch to postcopy-paused state, but that is not what we want. If device states failed to load, we should fail the migration directly instead of pause. Postponing the state switch to the point when we have already loaded the devices' states and been ready to start running destination VM. Signed-off-by: Peter Xu --- migration/savevm.c | 10 -- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/migration/savevm.c b/migration/savevm.c index bc87b0e5b1..3bc792e320 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -1584,8 +1584,6 @@ static void *postcopy_ram_listen_thread(void *opaque) QEMUFile *f = mis->from_src_file; int load_res; -migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE, - MIGRATION_STATUS_POSTCOPY_ACTIVE); qemu_sem_post(&mis->listen_thread_sem); trace_postcopy_ram_listen_thread_start(); @@ -1748,6 +1746,14 @@ static int loadvm_postcopy_handle_run(MigrationIncomingState *mis) return -1; } +/* + * Declare that we are in postcopy now. We should already have + * all the device states loaded ready when reach here, and also + * the ram load thread running. + */ +migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE, + MIGRATION_STATUS_POSTCOPY_ACTIVE); + data = g_new(HandleRunBhData, 1); data->bh = qemu_bh_new(loadvm_postcopy_handle_run_bh, data); qemu_bh_schedule(data->bh); -- 2.13.5
[Qemu-devel] [PATCH v3 31/32] migration, qmp: new command "migrate-pause"
It is used to manually trigger the postcopy pause state. It works just
like when we found the migration stream failed during postcopy, but
provide an explicit way for user in case of misterious socket hangs.
Signed-off-by: Peter Xu
---
migration/migration.c | 18 ++
qapi/migration.json | 22 ++
2 files changed, 40 insertions(+)
diff --git a/migration/migration.c b/migration/migration.c
index 51e771685c..3d852d233f 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1426,6 +1426,24 @@ void qmp_migrate_incoming(const char *uri, Error **errp)
once = false;
}
+void qmp_migrate_pause(Error **errp)
+{
+int ret;
+MigrationState *ms = migrate_get_current();
+
+if (ms->state != MIGRATION_STATUS_POSTCOPY_ACTIVE) {
+error_setg(errp, "Migration pause is currently only allowed during"
+ " an active postcopy phase.");
+return;
+}
+
+ret = qemu_file_shutdown(ms->to_dst_file);
+
+if (ret) {
+error_setg(errp, "Failed to pause migration stream.");
+}
+}
+
bool migration_is_blocked(Error **errp)
{
if (qemu_savevm_state_blocked(errp)) {
diff --git a/qapi/migration.json b/qapi/migration.json
index f8132e683a..095aeb4dd2 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1028,6 +1028,28 @@
{ 'command': 'migrate-incoming', 'data': {'uri': 'str' } }
##
+# @migrate-pause:
+#
+# Pause an migration. Currently it can only pause a postcopy
+# migration. Pausing a precopy migration is not supported yet.
+#
+# It is mostly used as a manual way to trigger the postcopy paused
+# state when the network sockets hang due to some reason, so that we
+# can try a recovery afterward.
+#
+# Returns: nothing on success
+#
+# Since: 2.10
+#
+# Example:
+#
+# -> { "execute": "migrate-pause" }
+# <- { "return": {} }
+#
+##
+{ 'command': 'migrate-pause' }
+
+##
# @xen-save-devices-state:
#
# Save the state of all devices to file. The RAM and the block devices
--
2.13.5
Re: [Qemu-devel] [RFC v2 09/22] monitor: create monitor dedicate iothread
On Thu, Oct 12, 2017 at 01:29:11PM +0100, Stefan Hajnoczi wrote:
> On Fri, Sep 29, 2017 at 11:38:31AM +0800, Peter Xu wrote:
> > @@ -4126,10 +4150,23 @@ void monitor_init(Chardev *chr, int flags)
> > qemu_mutex_unlock(&monitor_lock);
> > }
> >
> > +static void monitor_io_thread_destroy(void)
> > +{
> > +iothread_destroy(mon_global.mon_io_thread);
> > +mon_global.mon_io_thread = NULL;
> > +}
> > +
> > void monitor_cleanup(void)
> > {
> > Monitor *mon, *next;
> >
> > +/*
> > + * We need to explicitly stop the iothread (but not destroy it),
> > + * cleanup the monitor resources, then destroy the iothread. See
> > + * again on the glib bug mentioned in 2b316774f6 for a reason.
> > + */
> > +iothread_stop(mon_global.mon_io_thread);
> > +
> > qemu_mutex_lock(&monitor_lock);
> > QLIST_FOREACH_SAFE(mon, &mon_list, entry, next) {
> > QLIST_REMOVE(mon, entry);
> > @@ -4137,6 +4174,8 @@ void monitor_cleanup(void)
> > g_free(mon);
> > }
> > qemu_mutex_unlock(&monitor_lock);
> > +
> > +monitor_io_thread_destroy();
> > }
>
> Minor style comment, I would inline monitor_io_thread_destroy() into
> monitor_cleanup() instead of making it a function.
>
> monitor_io_thread_destroy() relies on iothread_stop() being called
> first. Defining a function with no doc comment creates a risk that
> someone else will call it in the future without first calling
> iothread_stop(). It's safer to inline the code where it cannot be
> misused by accident.
There will be some more lines added to monitor_io_thread_destroy() in
follow-up patches. I was trying to put iothread things all into this
function but I cannot really do that since the glib bug (then we'll
need explicit iothread_stop() above). But sure, I can inline them all.
>
> Also, please name things "iothread" instead of "io_thread" for
> consistency.
Will do. Thanks,
--
Peter Xu
Re: [Qemu-devel] [PATCH v1 5/6] kvm: kvm_log_start/stop are only called with known sections
On 10.10.2017 15:29, Paolo Bonzini wrote:
> On 10/10/2017 11:06, Thomas Huth wrote:
>> On 11.09.2017 19:49, David Hildenbrand wrote:
>>> Let's properly align the sections first and bail out if we would ever
>>> get called with a memory section we don't know yet.
>>>
>>> Signed-off-by: David Hildenbrand
>>> ---
>>> accel/kvm/kvm-all.c | 18 --
>>> 1 file changed, 12 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>>> index b677d1b13e..2ae459453d 100644
>>> --- a/accel/kvm/kvm-all.c
>>> +++ b/accel/kvm/kvm-all.c
>>> @@ -411,15 +411,21 @@ static int kvm_slot_update_flags(KVMMemoryListener
>>> *kml, KVMSlot *mem,
>>> static int kvm_section_update_flags(KVMMemoryListener *kml,
>>> MemoryRegionSection *section)
>>> {
>>> -hwaddr phys_addr = section->offset_within_address_space;
>>> -ram_addr_t size = int128_get64(section->size);
>>> -KVMSlot *mem = kvm_lookup_matching_slot(kml, phys_addr, size);
>>> +hwaddr start_addr, size;
>>> +KVMSlot *mem;
>>>
>>> -if (mem == NULL) {
>>> +size = kvm_align_section(section, &start_addr);
>>> +if (!size) {
>>> return 0;
>>> -} else {
>>> -return kvm_slot_update_flags(kml, mem, section->mr);
>>> }
>>> +
>>> +mem = kvm_lookup_matching_slot(kml, start_addr, size);
>>> +if (!mem) {
>>> +fprintf(stderr, "%s: error finding slot\n", __func__);
>>> +abort();
>>> +}
>>
>> FYI, this abort now triggers with the "isa-vga" device:
>>
>> $ x86_64-softmmu/qemu-system-x86_64 -S -accel kvm -device isa-vga
>> kvm_section_update_flags: error finding slot
>> Aborted (core dumped)
>>
>> Any ideas how to fix this?
>
> Reverting, unless David has some time to look into it.
>
> Paolo
>
Just returned from vacation, I'll have a look this week.
Thanks for that nice reproducer Thomas, that should help a lot!
--
Thanks,
David
Re: [Qemu-devel] [PATCH RFC 2/3] s390x/tcg: low-address protection support
On 12.10.2017 10:41, Thomas Huth wrote:
> On 29.09.2017 13:27, Cornelia Huck wrote:
>> On Thu, 28 Sep 2017 15:08:11 +0200
>> David Hildenbrand wrote:
>>
>>> On 28.09.2017 06:50, Thomas Huth wrote:
On 27.09.2017 19:00, David Hildenbrand wrote:
> This is a neat way to implement low address protection, whereby
> only the first 512 bytes of the first two pages (each 4096 bytes) of
> every address space are protected.
>
> Store a tec of 0 for the access exception, this is what is defined by
> Enhanced Suppression on Protection in case of a low address protection
> (Bit 61 set to 0, rest undefined).
>
> We have to make sure to to pass the access address, not the masked page
> address into mmu_translate*().
>
> Drop the check from testblock. So we can properly test this via
> kvm-unit-tests.
>
> This will check every access going through one of the MMUs.
>
> Signed-off-by: David Hildenbrand
> ---
> target/s390x/excp_helper.c | 3 +-
> target/s390x/mem_helper.c | 8
> target/s390x/mmu_helper.c | 96
> +-
> 3 files changed, 62 insertions(+), 45 deletions(-)
[...]
> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
> index 9daa0fd8e2..44a15449d2 100644
> --- a/target/s390x/mmu_helper.c
> +++ b/target/s390x/mmu_helper.c
> @@ -106,6 +106,37 @@ static void trigger_page_fault(CPUS390XState *env,
> target_ulong vaddr,
> trigger_access_exception(env, type, ilen, tec);
> }
>
> +/* check whether the address would be proteted by Low-Address Protection
> */
> +static bool is_low_address(uint64_t addr)
> +{
> +return addr < 512 || (addr >= 4096 && addr < 4607);
> +}
I like the check from the kernel sources better:
static inline int is_low_address(unsigned long ga)
{
/* Check for address ranges 0..511 and 4096..4607 */
return (ga & ~0x11fful) == 0;
}
... that might result in slightly faster code (depending on the
compiler, of course).
>>>
>>> I think that lim (readability) -> 0. Without that comment you're at
>>> first sight really clueless what this is about.
>>>
>>> My check exactly corresponds to the wording in the PoP (and smart
>>> compilers should be able to optimize).
>>>
>>> But I don't have a strong opinion on this micro optimization.
>>
>> FWIW, I'd be happy with both, but has anyone actually looked at the
>> generated code?
>
> This is what I get for David's original code:
>
> 8510: c4 18 00 00 0d a4 lgrl%r1,80002058
> 8516: a7 29 01 ff lghi%r2,511
> 851a: ec 12 00 4f c0 65 clgrjnh %r1,%r2,85b8
> 8520: a7 1b f0 00 aghi%r1,-4096
> 8524: c2 1e 00 00 01 fe clgfi %r1,510
> 852a: a7 18 00 00 lhi %r1,0
> 852e: b9 99 00 11 slbr%r1,%r1
> 8532: 13 11 lcr %r1,%r1
> 8534: c4 1f 00 00 0d 96 strl%r1,80002060
>
> And this is the optimized kernel version:
>
> 854a: c4 18 00 00 0d 7f lgrl%r1,80002048
> 8550: a5 17 ee 00 nill%r1,60928
> 8554: b9 00 00 11 lpgr%r1,%r1
> 8558: a7 1b ff ff aghi%r1,-1
> 855c: eb 11 00 3f 00 0c srlg%r1,%r1,63
> 8562: c4 1f 00 00 0d 77 strl%r1,80002050
>
> So that's indeed a little bit better :-)
> (I was using GCC 4.8.5 from RHEL7, with -O2)
>
> By the way, I think there's a bug in David's code: It should either be
> "addr <= 4607" or "addr < 4608" instead of "addr < 4607".
>
> With that bug fixed, David's version get's optimized even more:
>
> 8510: c4 18 00 00 0d a4 lgrl%r1,80002058
> 8516: a5 17 ef ff nill%r1,61439
> 851a: c2 1e 00 00 01 ff clgfi %r1,511
> 8520: a7 18 00 00 lhi %r1,0
> 8524: b9 99 00 11 slbr%r1,%r1
> 8528: 13 11 lcr %r1,%r1
> 852a: c4 1f 00 00 0d 9b strl%r1,80002060
>
> ... so the difference is really very minimal in that case --> We could
> really use the more readable version, I think.
>
> Thomas
>
Very right, I'll fix that.
Nice way to find BUGs - comparing generated code :)
Thanks!
--
Thanks,
David
Re: [Qemu-devel] [PATCH RFC 1/3] accel/tcg: allow to invalidate a write TLB entry immediately
On 27.09.2017 19:48, Richard Henderson wrote: > On 09/27/2017 10:00 AM, David Hildenbrand wrote: >> Background: s390x implements Low-Address Protection (LAP). If LAP is >> enabled, writing to effective addresses (before any transaltion) >> 0-511 and 4096-4607 triggers a protection exception. >> >> So we have subpage protection on the first two pages of every address >> space (where the lowcore - the CPU private data resides). >> >> By immediately invalidating the write entry but allowing the caller to >> continue, we force every write access onto these first two pages into >> the slow path. we will get a tlb fault with the specific accessed >> addresses and can then evaluate if protection applies or not. >> >> We have to make sure to ignore the invalid bit if tlb_fill() succeeds. > > This is similar to a scheme I proposed to PMM wrt handling ARM v8M > translation. > Reusing TLB_INVALID_MASK would appear to work, but I wonder if it wouldn't be > clearer to use another bit. I believe I had proposed a TLB_FORCE_SLOW_MASK. > > Thoughts, Peter? As two weeks have passed: Any further opinions? Richard, how do you want me to continue with this? Thanks! -- Thanks, David
Re: [Qemu-devel] [RFC v2 10/22] monitor: allow to use IO thread for parsing
On Thu, Oct 12, 2017 at 01:35:08PM +0100, Stefan Hajnoczi wrote:
> On Fri, Sep 29, 2017 at 11:38:32AM +0800, Peter Xu wrote:
> > For each Monitor, add one field "use_io_thr" to show whether it will be
> > using the dedicated monitor IO thread to handle input/output. When set,
> > monitor IO parsing work will be offloaded to dedicated monitor IO
> > thread, rather than the original main loop thread.
>
> Why make use_io_thr optional? We'd have to maintain and test both code
> paths.
Because of MUXed typed chardevs. I will add the explanation into this
patch's commit message, and also into the codes where we used it (in a
follow-up patch).
>
> > @@ -4135,19 +4139,37 @@ void monitor_init(Chardev *chr, int flags)
> > monitor_read_command(mon, 0);
> > }
> >
> > +if (mon->use_io_thr) {
> > +/*
> > + * When use_io_thr is set, we use the global shared dedicated
> > + * IO thread for this monitor to handle input/output.
> > + */
> > +context = monitor_io_context_get();
> > +/* We should have inited globals before reaching here. */
> > +assert(context);
> > +} else {
> > +/* The default main loop, which is the main thread */
> > +context = NULL;
> > +}
> > +
> > +/*
> > + * Hang the monitor before running it (which is triggered by
>
> s/Hang/Add/
>
> "Hang" isn't used as a verb for adding items to collections. People
> would probably think about deadlocking the thread instead :-).
Thanks for explaining this! Fixed.
(I am always happy to improve my English, though in a slow way :-)
>
> > + * qemu_chr_fe_set_handlers). Otherwise one monitor may run while
> > + * find itself not on the mon_list.
> > + */
> > +qemu_mutex_lock(&monitor_lock);
> > +QTAILQ_INSERT_HEAD(&mon_list, mon, entry);
> > +qemu_mutex_unlock(&monitor_lock);
--
Peter Xu
Re: [Qemu-devel] [PATCH] configure: pick the right compiler for OpenBSD by default
Hi, > By the way, looks like OpenBSD is also switching to clang by default > soon: > > https://www.phoronix.com/scan.php?page=news_item&px=OpenBSD-Default- > Clang It did happen already. On openbsd 6.2 (released a week ago) cc is clang: $ cc --version OpenBSD clang version 4.0.0 (tags/RELEASE_400/final) (based on LLVM 4.0.0) cheers, Gerd
Re: [Qemu-devel] [RFC v2 13/22] monitor: separate QMP parser and dispatcher
On Thu, Oct 12, 2017 at 01:50:45PM +0100, Stefan Hajnoczi wrote:
> On Fri, Sep 29, 2017 at 11:38:35AM +0800, Peter Xu wrote:
> > Originally QMP is going throw these steps:
>
> s/is going throw/goes through/
Fixed.
>
> >
> > JSON Parser --> QMP Dispatcher --> Respond
> > /|\(2)(3) |
> >(1) | \|/ (4)
> >+- main thread +
> >
> > This patch does this:
> >
> > JSON Parser QMP Dispatcher --> Respond
> > /|\ | /|\ (4) |
> >| | (2)| (3)| (5)
> >(1) | +-> | \|/
> >+- main thread <---+
> >
> > So the parsing job and the dispatching job is isolated now. It gives us
> > a chance in following up patches to totally move the parser outside.
> >
> > The isloation is done using one QEMUBH. Only one dispatcher QEMUBH is
> > used for all the monitors.
> >
> > Signed-off-by: Peter Xu
> > ---
> > monitor.c | 156
> > +-
> > 1 file changed, 133 insertions(+), 23 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 7b76dff5ad..1e9a6cb6a5 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -208,10 +208,14 @@ struct Monitor {
> > mon_cmd_t *cmd_table;
> > QLIST_HEAD(,mon_fd_t) fds;
> > QTAILQ_ENTRY(Monitor) entry;
> > +/* Input queue that hangs all the parsed QMP requests */
>
> s/hangs/holds/
Fixed.
>
> > +static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens,
> > + void *opaque)
> > +{
> > +QObject *req, *id = NULL;
> > +QDict *qdict = NULL;
> > +Monitor *mon = opaque;
> > +Error *err = NULL;
> > +QMPRequest *req_obj;
> > +
> > +req = json_parser_parse_err(tokens, NULL, &err);
> > +if (!req && !err) {
> > +/* json_parser_parse_err() sucks: can fail without setting @err */
> > +error_setg(&err, QERR_JSON_PARSING);
> > +}
> > +if (err) {
> > +monitor_qmp_respond(mon, NULL, err, NULL);
> > +qobject_decref(req);
>
> Is there a return statement missing here?
Hmm... Very possible!
Fixed.
>
> > +}
> > +
> > +qdict = qobject_to_qdict(req);
> > +if (qdict) {
> > +id = qdict_get(qdict, "id");
> > +qobject_incref(id);
> > +qdict_del(qdict, "id");
> > +} /* else will fail qmp_dispatch() */
> > +
> > +req_obj = g_new0(QMPRequest, 1);
> > +req_obj->mon = mon;
> > +req_obj->id = id;
> > +req_obj->req = req;
> > +
> > +/*
> > + * Put the request to the end of queue so that requests will be
> > + * handled in time order. Ownership for req_obj, req, id,
> > + * etc. will be delivered to the handler side.
> > + */
> > +g_queue_push_tail(mon->qmp_requests, req_obj);
> > +
> > +/* Kick the dispatcher routine */
> > +qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
>
> How is thread-safety ensured when accessing qmp_requests?
It's a GQueue. I assume GQueue is thread safe itself as long as
g_thread_init() is called?
Thanks,
--
Peter Xu
Re: [Qemu-devel] [PATCH v2 00/40] generalize parsing of cpu_model (part 2)
On Fri, 13 Oct 2017 16:13:16 -0300 Eduardo Habkost wrote: > On Thu, Oct 12, 2017 at 06:27:46PM +0200, Igor Mammedov wrote: > > On Thu, 5 Oct 2017 15:50:34 +0200 > > Igor Mammedov wrote: > > > > Eduardo, > > > > Could you merge series via machine tree, pls? > > I just queued it on machine-next. Pull request will be sent only > next week, though. > Thanks!
Re: [Qemu-devel] [PATCH 2/2] monitor: add proper reference counting of the current CPU
On Fri, 13 Oct 2017 12:05:46 +0200
Greg Kurz wrote:
> On Fri, 13 Oct 2017 11:24:59 +0200
> Igor Mammedov wrote:
>
> > On Fri, 13 Oct 2017 10:35:31 +0200
> > Greg Kurz wrote:
> >
> > > If a CPU selected with the "cpu" command is hot-unplugged then "info cpus"
> > > causes QEMU to exit:
> > >
> > > (qemu) device_del cpu1
> > > (qemu) info cpus
> > > qemu:qemu_cpu_kick_thread: No such process
> > >
> > > This happens because "cpu" stores the pointer to the selected CPU into
> > > the monitor structure. When the CPU is hot-unplugged, we end up with a
> > > dangling pointer. The "info cpus" command then does:
> > >
> > > hmp_info_cpus()
> > > monitor_get_cpu_index()
> > > mon_get_cpu()
> > >cpu_synchronize_state() <--- called with dangling pointer
> > >
> > > This could cause a QEMU crash as well.
> > >
> > > This patch switches the monitor to use object_ref() to ensure the
> > > CPU object doesn't vanish unexpectedly. The reference is dropped
> > > either when "cpu" is used to switch to another CPU, or when the
> > > selected CPU is unrealized and cpu_list_remove() sets its cpu_index
> > > back to UNASSIGNED_CPU_INDEX.
> > I don't really like an idea of leaving dangling cpu around.
> > Is it possible to store QOM path on set_cpu in monitor and
> > resolving it each to instance each time it's needed?
> >
>
> It sounds workable. Also it would allow the fix to not depend on patch 1
> for ppc.
yep,
also if you'd touch these cpu_index based monitor commands,
it might make sense to convert them to use qmo path directly
instead if cpu_index (with command completion it would be
easy to use)
>
> Thanks for the suggestion!
>
> >
> > > Signed-off-by: Greg Kurz
> > > ---
> > > monitor.c | 12
> > > 1 file changed, 12 insertions(+)
> > >
> > > diff --git a/monitor.c b/monitor.c
> > > index fe0d1bdbb461..1c0b9a2c3ad3 100644
> > > --- a/monitor.c
> > > +++ b/monitor.c
> > > @@ -579,6 +579,9 @@ static void monitor_data_init(Monitor *mon)
> > >
> > > static void monitor_data_destroy(Monitor *mon)
> > > {
> > > +if (mon->mon_cpu) {
> > > +object_unref((Object *) mon->mon_cpu);
> > > +}
> > > qemu_chr_fe_deinit(&mon->chr, false);
> > > if (monitor_is_qmp(mon)) {
> > > json_message_parser_destroy(&mon->qmp.parser);
> > > @@ -1047,12 +1050,21 @@ int monitor_set_cpu(int cpu_index)
> > > if (cpu == NULL) {
> > > return -1;
> > > }
> > > +if (cur_mon->mon_cpu) {
> > > +object_unref((Object *) cur_mon->mon_cpu);
> > > +}
> > > cur_mon->mon_cpu = cpu;
> > > +object_ref((Object *) cpu);
> > > return 0;
> > > }
> > >
> > > CPUState *mon_get_cpu(void)
> > > {
> > > +if (cur_mon->mon_cpu &&
> > > +cur_mon->mon_cpu->cpu_index == UNASSIGNED_CPU_INDEX) {
> > > +object_unref((Object *) cur_mon->mon_cpu);
> > > +cur_mon->mon_cpu = NULL;
> > > +}
> > > if (!cur_mon->mon_cpu) {
> > > if (!first_cpu) {
> > > return NULL;
> > >
> > >
> >
>
Re: [Qemu-devel] [RFC v2 15/22] monitor: send event when request queue full
On Thu, Oct 12, 2017 at 01:56:20PM +0100, Stefan Hajnoczi wrote:
> On Fri, Sep 29, 2017 at 11:38:37AM +0800, Peter Xu wrote:
> > Set maximum QMP request queue length to 8. If queue full, instead of
> > queue the command, we directly return a "request-dropped" event, telling
> > client that specific command is dropped.
> >
> > Signed-off-by: Peter Xu
> > ---
> > monitor.c | 15 +++
> > 1 file changed, 15 insertions(+)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 1e9a6cb6a5..d9bed31248 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -3971,6 +3971,8 @@ static void monitor_qmp_bh_dispatcher(void *data)
> > }
> > }
> >
> > +#define QMP_ASYNC_QUEUE_LEN_MAX (8)
>
> Why 8?
I proposed this in previous discussion and no one objected, so I just
used it. It's here:
https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg03989.html
(please don't go over the thread; I'll copy the related paragraphs)
"""
...
Regarding to queue size: I am afraid max_size=1 may not suffice?
Otherwise a simple batch of:
{"execute": "query-status"} {"execute": "query-status"}
Will trigger the failure. But I definitely agree it should not be
something very large. The total memory will be this:
json limit * queue length limit * monitor count limit
(X)(Y)(Z)
Now we have (X) already (in form of a few tunables for JSON token
counts, etc.), we don't have (Z), and we definitely need (Y).
How about we add limits on Y=16 and Z=8?
We can do some math if we want some more exact number though.
...
"""
Oops, I proposed "16", but I used "8"; I hope 8 is good enough, but I
am definitely not sure whether "1" is good.
>
> My understanding is that this patch series is not about asynchronous QMP
> commands. Instead it's about executing certain commands immediately in
> the parser thread.
Indeed, but IMHO the series does something further than that - we do
have async queues for QMP requests/responses now. IMHO that's real
async, though totally different from the idea of "async QMP commands"
for sure.
>
> Therefore, I suggest hardcoding length 1 for now and not calling it
> "async". You may also be able to simplify the code since a queue isn't
> actually needed.
For the queue length: discussed above, I'm not sure whether queue=1 is
really what we want. Again, I may be wrong.
For the naming: how about QMP_REQ_QUEUE_LEN_MAX?
Thanks,
--
Peter Xu
Re: [Qemu-devel] [RFC v2 16/22] monitor: enable IO thread for (qmp & !mux) typed
On Thu, Oct 12, 2017 at 01:57:55PM +0100, Stefan Hajnoczi wrote: > On Fri, Sep 29, 2017 at 11:38:38AM +0800, Peter Xu wrote: > > Start to use dedicate IO thread for QMP monitors that are not using > > MUXed chardev. > > > > We excluded MUXed chardev because when mux is used, frontend can be the > > monitor plus something else. The only thing we know would be safe to be > > run outside main thread is the monitor frontend, all the rest of the > > frontends should still be run in main thread only. > > Please move this explanation into a comment so it's immediately visible > when reading the code. Will do. Thanks, -- Peter Xu
Re: [Qemu-devel] [RFC PATCH 14/30] softfloat: 16 bit helpers for shr, clz and rounding and packing
Richard Henderson writes:
> On 10/13/2017 09:24 AM, Alex Bennée wrote:
>> Half-precision helpers for float16 maths. I didn't bother hand-coding
>> the count leading zeros as we could always fall-back to host-utils if
>> we needed to.
>>
>> Signed-off-by: Alex Bennée
>> ---
>> fpu/softfloat-macros.h | 39 +++
>> fpu/softfloat.c| 21 +
>> 2 files changed, 60 insertions(+)
>>
>> diff --git a/fpu/softfloat-macros.h b/fpu/softfloat-macros.h
>> index 9cc6158cb4..73091a88a8 100644
>> --- a/fpu/softfloat-macros.h
>> +++ b/fpu/softfloat-macros.h
>> @@ -89,6 +89,31 @@ this code that are retained.
>> # define SOFTFLOAT_GNUC_PREREQ(maj, min) 0
>> #endif
>>
>> +/*
>> +| Shifts `a' right by the number of bits given in `count'. If any nonzero
>> +| bits are shifted off, they are ``jammed'' into the least significant bit
>> of
>> +| the result by setting the least significant bit to 1. The value of
>> `count'
>> +| can be arbitrarily large; in particular, if `count' is greater than 16,
>> the
>> +| result will be either 0 or 1, depending on whether `a' is zero or nonzero.
>> +| The result is stored in the location pointed to by `zPtr'.
>> +**/
>> +
>> +static inline void shift16RightJamming(uint16_t a, int count, uint16_t
>> *zPtr)
>> +{
>> +uint16_t z;
>> +
>> +if ( count == 0 ) {
>> +z = a;
>> +}
>> +else if ( count < 16 ) {
>> +z = ( a>>count ) | ( ( a<<( ( - count ) & 16 ) ) != 0 );
>> +}
>> +else {
>> +z = ( a != 0 );
>> +}
>> +*zPtr = z;
>> +
>> +}
>
> When are you going to use a SRJ of a uint16_t? Isn't most of your actual
> arithmetic actually done on uint32_t?
The add/sub stuff currently uses it. Arguably it could do what it needs
with 32 bit as well but the spare exponent bits are enough for operating
on the significand. That said I'm fairly sure it all ends up 32 bit in
the generated code.
>
>> +/*
>> +| Returns the number of leading 0 bits before the most-significant 1 bit of
>> +| `a'. If `a' is zero, 16 is returned.
>> +**/
>> +
>> +static int8_t countLeadingZeros16( uint16_t a )
>> +{
>> +if (a) {
>> +return __builtin_clz(a);
>> +} else {
>> +return 16;
>> +}
>> +}
>
> __builtin_clz works on "int". You need to use clz32(a) - 16.
Ahh my mistake - I'd assumed it had the same smarts as the gcc atomics.
Maybe I should just use our utils functions afterall.
>
>> +/*
>> +| Takes an abstract floating-point value having sign `zSign', exponent
>> `zExp',
>> +| and significand `zSig', and returns the proper single-precision floating-
>
> s/single/half/
>
>> +| point value corresponding to the abstract input. This routine is just
>> like
>> +| `roundAndPackFloat32' except that `zSig' does not have to be normalized.
>> +| Bit 15 of `zSig' must be zero, and `zExp' must be 1 less than the ``true''
>> +| floating-point exponent.
>> +**/
>> +
>> +static float16
>> + normalizeRoundAndPackFloat16(flag zSign, int zExp, uint16_t zSig,
>> + float_status *status)
>> +{
>> +int8_t shiftCount;
>> +
>> +shiftCount = countLeadingZeros16( zSig ) - 1;
>> +return roundAndPackFloat16(zSign, zExp - shiftCount, zSig<> + true, status);
>
> Do I recall correctly that your lsb is between bits 7:6, like
> roundAndPackFloat32? You've got 11 bits of sig. Plus 7 bits of extra equals
> 18 bits. Which doesn't fit in uint16_t.
No it takes a 32 bit sig in and deals with it internally.
>
> So, the reason that roundAndPackFloat32 uses 7 bits is that 7 + 24 == 31.
>
> We can either use a split at (15 - 11 =) 4 bits, and still fit in a uint16_t,
> or we can drop uint16_t and admit that the compiler is going to promote to
> int,
> or uint32_t, anyway. If we do that, we have options of a split between 4 and
> (31 - 11 =) 20 bits.
>
> We talked this week re fp->int conversion, it did seem Really Useful when we
> noted that sig << exp is representable in a uint32_t. Which does suggest a
> choice at or below (32 - 11 - 14 =) 7.
>
>
> r~
--
Alex Bennée
Re: [Qemu-devel] [PATCH] spapr_cpu_core: instantiate CPUs separately
On Sat, 14 Oct 2017 20:33:37 +1100
David Gibson wrote:
> On Fri, Oct 13, 2017 at 01:31:44PM +0200, Greg Kurz wrote:
> > The current code assumes that only the CPU core object holds a
> > reference on each individual CPU object, and happily frees their
> > allocated memory when the core is unrealized. This is dangerous
> > as some other code can legitimely keep a pointer to a CPU if it
> > calls object_ref(), but it would end up with a dangling pointer.
> >
> > Let's allocate all CPUs with object_new() and let QOM frees them
> > when their reference count reaches zero. This greatly simplify the
> > code as we don't have to fiddle with the instance size anymore.
> >
> > Signed-off-by: Greg Kurz
>
> So, I'm pretty sure my first drafts of the core stuff did things this
> waym and it got nacked, for QOM lifetime reasons that I never really
> understood.
From what I remember, Andreas would like to see composite CPU object
allocated in one go and then its children initialized with object_initialize()
so that no more allocation were needed.
That potentially would benefit hotplug, since we could gracefully
fail object creation early if there is not enough memory.
But the way it's implemented currently doesn't really match that initial
goal as array for threads is dynamically allocated later
and then we need to dance around it with pointer arithmetic.
BTW: almost any allocation failure in qemu currently
is fatal so whether we fail on array alloc or on individual
object_new() won't make any difference.
I'd rather see this clean up merged as it simplifies code
in these case.
>
> > ---
> > v2: - mention code simplification in changelog
> > - use PowerPCCPU * and Object * instead of void *
> > ---
> > hw/ppc/spapr.c | 11 +++
> > hw/ppc/spapr_cpu_core.c | 19 +++
> > include/hw/ppc/spapr_cpu_core.h |2 +-
> > 3 files changed, 11 insertions(+), 21 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index fd9813bde82f..d9555a3677be 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -3153,12 +3153,10 @@ void spapr_core_release(DeviceState *dev)
> >
> > if (smc->pre_2_10_has_unused_icps) {
> > sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> > -sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc));
> > -size_t size = object_type_get_instance_size(scc->cpu_type);
> > int i;
> >
> > for (i = 0; i < cc->nr_threads; i++) {
> > -CPUState *cs = CPU(sc->threads + i * size);
> > +CPUState *cs = CPU(sc->threads[i]);
> >
> > pre_2_10_vmstate_register_dummy_icp(cs->cpu_index);
> > }
> > @@ -3204,7 +3202,7 @@ static void spapr_core_plug(HotplugHandler
> > *hotplug_dev, DeviceState *dev,
> > sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> > sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> > CPUCore *cc = CPU_CORE(dev);
> > -CPUState *cs = CPU(core->threads);
> > +CPUState *cs = CPU(core->threads[0]);
> > sPAPRDRConnector *drc;
> > Error *local_err = NULL;
> > int smt = kvmppc_smt_threads();
> > @@ -3249,15 +3247,12 @@ static void spapr_core_plug(HotplugHandler
> > *hotplug_dev, DeviceState *dev,
> > core_slot->cpu = OBJECT(dev);
> >
> > if (smc->pre_2_10_has_unused_icps) {
> > -sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc));
> > -size_t size = object_type_get_instance_size(scc->cpu_type);
> > int i;
> >
> > for (i = 0; i < cc->nr_threads; i++) {
> > sPAPRCPUCore *sc = SPAPR_CPU_CORE(dev);
> > -void *obj = sc->threads + i * size;
> >
> > -cs = CPU(obj);
> > +cs = CPU(sc->threads[i]);
> > pre_2_10_vmstate_unregister_dummy_icp(cs->cpu_index);
> > }
> > }
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 3a4c17401226..588f9b45714a 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -79,13 +79,11 @@ const char *spapr_get_cpu_core_type(const char
> > *cpu_type)
> > static void spapr_cpu_core_unrealizefn(DeviceState *dev, Error **errp)
> > {
> > sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> > -sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(dev));
> > -size_t size = object_type_get_instance_size(scc->cpu_type);
> > CPUCore *cc = CPU_CORE(dev);
> > int i;
> >
> > for (i = 0; i < cc->nr_threads; i++) {
> > -void *obj = sc->threads + i * size;
> > +Object *obj = OBJECT(sc->threads[i]);
> > DeviceState *dev = DEVICE(obj);
> > CPUState *cs = CPU(dev);
> > PowerPCCPU *cpu = POWERPC_CPU(cs);
> > @@ -146,9 +144,8 @@ static void spapr_cpu_core_realize(DeviceState *dev,
> > Error **errp)
> > sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> > sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(dev)
Re: [Qemu-devel] [PATCH 2/4] multiboot: load any machine type of ELF
Am 14.10.2017 um 15:41 hat Peter Maydell geschrieben: > On 14 October 2017 at 00:21, Eduardo Habkost wrote: > > I don't believe the spec restricts that, but I don't see why it > > would be useful to load an ELF file that doesn't match the target > > architecture (e.g. loading non-x86 ELF files on a x86 machine > > like PC). > > Agreed. If we have non i386 boards that want to use multiboot > we should probably move the common code out of hw/i386... Impossible with Multiboot 1, it's a spec that is really made for i386. The spec isn't really explicit about it being a requirement, but it does say that its target are 32-bit OSes on PCs, and it defines the boot state in terms of i386 registers, so it doesn't make sense for non-x86. >From my interpretation of the spec, even support for 64-bit ELFs seems to be (implicitly) out of spec (there is one place where it even says "refer to the i386 ELF documentation for details"), but if GRUB implements it... Kevin
Re: [Qemu-devel] [PATCH v4 1/8] nbd: Include error names in trace messages
15.10.2017 04:01, Eric Blake wrote: NBD errors were originally sent over the wire based on Linux errno values; but not all the world is Linux, and not all platforms share the same values. Since a number isn't very easy to decipher on all platforms, update the trace messages to include the name of NBD errors being sent/received over the wire. Tweak the trace messages to be at the point where we are using the NBD error, not the translation to the host errno values. Signed-off-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
Re: [Qemu-devel] [RFC v2 00/32] postcopy+vhost-user/shared ram
Hello Maxime On 09/01/2017 04:42 PM, Maxime Coquelin wrote: Hello Alexey, On 09/01/2017 03:34 PM, Alexey Perevalov wrote: Hello David, You wrote in previous version: We've had a postcopy migrate work now, with a few hacks we're still cleaning up, both on vhost-user-bridge and dpdk; so I'll get this updated and reposted. I want to know more about DPDK work, do you know, is somebody assigned to that task? I did the DPDK (rough) prototype, you may find it here: https://gitlab.com/mcoquelin/dpdk-next-virtio/commits/postcopy_proto_v1 I found it is for previous version of the patchset. Do you have any updates? Cheers, Maxime -- Best regards, Alexey Perevalov
Re: [Qemu-devel] [PATCH v4 2/8] nbd: Move nbd_errno_to_system_errno() to public header
15.10.2017 04:01, Eric Blake wrote:
This is needed in preparation for structured reply handling,
as we will be performing the translation from NBD error to
system errno value higher in the stack at block/nbd-client.c.
you've forget to sign-off.
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
include/block/nbd.h | 13 +
nbd/nbd-internal.h | 12
nbd/client.c| 32
nbd/common.c| 34 ++
nbd/trace-events| 4 +++-
5 files changed, 50 insertions(+), 45 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index a6df5ce8b5..dc62b5cd19 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -149,6 +149,18 @@ enum {
* aren't overflowing some other buffer. */
#define NBD_MAX_NAME_SIZE 256
+/* NBD errors are based on errno numbers, so there is a 1:1 mapping,
+ * but only a limited set of errno values is specified in the protocol.
+ * Everything else is squashed to EINVAL.
+ */
+#define NBD_SUCCESS0
+#define NBD_EPERM 1
+#define NBD_EIO5
+#define NBD_ENOMEM 12
+#define NBD_EINVAL 22
+#define NBD_ENOSPC 28
+#define NBD_ESHUTDOWN 108
+
/* Details collected by NBD_OPT_EXPORT_NAME and NBD_OPT_GO */
struct NBDExportInfo {
/* Set by client before nbd_receive_negotiate() */
@@ -172,6 +184,7 @@ int nbd_send_request(QIOChannel *ioc, NBDRequest *request);
int nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp);
int nbd_client(int fd);
int nbd_disconnect(int fd);
+int nbd_errno_to_system_errno(int err);
typedef struct NBDExport NBDExport;
typedef struct NBDClient NBDClient;
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index 4bfe5be884..df6c8b2f24 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -64,18 +64,6 @@
#define NBD_SET_TIMEOUT _IO(0xab, 9)
#define NBD_SET_FLAGS _IO(0xab, 10)
-/* NBD errors are based on errno numbers, so there is a 1:1 mapping,
- * but only a limited set of errno values is specified in the protocol.
- * Everything else is squashed to EINVAL.
- */
-#define NBD_SUCCESS0
-#define NBD_EPERM 1
-#define NBD_EIO5
-#define NBD_ENOMEM 12
-#define NBD_EINVAL 22
-#define NBD_ENOSPC 28
-#define NBD_ESHUTDOWN 108
-
/* nbd_read_eof
* Tries to read @size bytes from @ioc.
* Returns 1 on success
diff --git a/nbd/client.c b/nbd/client.c
index 59d7c9d49f..50f36b511e 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -22,38 +22,6 @@
#include "trace.h"
#include "nbd-internal.h"
-static int nbd_errno_to_system_errno(int err)
-{
-int ret;
-switch (err) {
-case NBD_SUCCESS:
-ret = 0;
-break;
-case NBD_EPERM:
-ret = EPERM;
-break;
-case NBD_EIO:
-ret = EIO;
-break;
-case NBD_ENOMEM:
-ret = ENOMEM;
-break;
-case NBD_ENOSPC:
-ret = ENOSPC;
-break;
-case NBD_ESHUTDOWN:
-ret = ESHUTDOWN;
-break;
-default:
-trace_nbd_unknown_error(err);
-/* fallthrough */
-case NBD_EINVAL:
-ret = EINVAL;
-break;
-}
-return ret;
-}
-
/* Definitions for opaque data types */
static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
diff --git a/nbd/common.c b/nbd/common.c
index 7456021f7e..593904f148 100644
--- a/nbd/common.c
+++ b/nbd/common.c
@@ -18,6 +18,7 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
+#include "trace.h"
#include "nbd-internal.h"
/* Discard length bytes from channel. Return -errno on failure and 0 on
@@ -171,3 +172,36 @@ const char *nbd_err_lookup(int err)
return "";
}
}
+
+
+int nbd_errno_to_system_errno(int err)
+{
+int ret;
+switch (err) {
+case NBD_SUCCESS:
+ret = 0;
+break;
+case NBD_EPERM:
+ret = EPERM;
+break;
+case NBD_EIO:
+ret = EIO;
+break;
+case NBD_ENOMEM:
+ret = ENOMEM;
+break;
+case NBD_ENOSPC:
+ret = ENOSPC;
+break;
+case NBD_ESHUTDOWN:
+ret = ESHUTDOWN;
+break;
+default:
+trace_nbd_unknown_error(err);
+/* fallthrough */
+case NBD_EINVAL:
+ret = EINVAL;
+break;
+}
+return ret;
+}
diff --git a/nbd/trace-events b/nbd/trace-events
index 9a72f458b2..d3b702dd9a 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -1,5 +1,4 @@
# nbd/client.c
-nbd_unknown_error(int err) "Squashing unexpected error %d to EINVAL"
nbd_send_option_request(uint32_t opt, const char *name, uint32_t len) "Sending option
request %" PRIu32" (%s), len %" PRIu32
nbd_receive_option_reply(uint32_t option, const char *optname, uint32_t type, const char *typename,
uint32_t length) "Received option reply 0x%" PRIx32" (%s), type 0x%" PRIx32" (%s),
len %" PRIu32
nbd_reply_err_unsup(uint32_t option, const char *name) "server doesn't understand reque
Re: [Qemu-devel] [PATCH] monitor: fix dangling CPU pointer
On Fri, 13 Oct 2017 13:47:51 +0200
Greg Kurz wrote:
[...]
> Note that the resolution should really return a CPU object, otherwise
> we have a bug. This is achieved by relying on object_resolve_path()
> and CPU() instead of calling object_resolve_path_type(path, TYPE_CPU).
I'm not sure what you are trying to say here, could you explain
a bit more why CPU(object_resolve_path()) is chosen vs
object_resolve_path_type(path, TYPE_CPU)
> Suggested-by: Igor Mammedov
> Signed-off-by: Greg Kurz
> ---
> monitor.c | 25 -
> 1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index fe0d1bdbb461..8489b2ad99c0 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -200,7 +200,7 @@ struct Monitor {
>
> ReadLineState *rs;
> MonitorQMP qmp;
> -CPUState *mon_cpu;
> +gchar *mon_cpu_path;
> BlockCompletionFunc *password_completion_cb;
> void *password_opaque;
> mon_cmd_t *cmd_table;
> @@ -579,6 +579,7 @@ static void monitor_data_init(Monitor *mon)
>
> static void monitor_data_destroy(Monitor *mon)
> {
> +g_free(mon->mon_cpu_path);
> qemu_chr_fe_deinit(&mon->chr, false);
> if (monitor_is_qmp(mon)) {
> json_message_parser_destroy(&mon->qmp.parser);
> @@ -1047,20 +1048,34 @@ int monitor_set_cpu(int cpu_index)
> if (cpu == NULL) {
> return -1;
> }
> -cur_mon->mon_cpu = cpu;
> +g_free(cur_mon->mon_cpu_path);
> +cur_mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
> return 0;
> }
>
> CPUState *mon_get_cpu(void)
> {
> -if (!cur_mon->mon_cpu) {
> +CPUState *cpu;
> +
> +if (cur_mon->mon_cpu_path) {
> +Object *obj = object_resolve_path(cur_mon->mon_cpu_path, NULL);
> +
> +if (!obj) {
> +g_free(cur_mon->mon_cpu_path);
> +cur_mon->mon_cpu_path = NULL;
> +} else {
> +cpu = CPU(obj);
this potentially might abort if obj couldn't be cast to TYPE_CPU
> +}
> +}
> +if (!cur_mon->mon_cpu_path) {
> if (!first_cpu) {
> return NULL;
> }
> monitor_set_cpu(first_cpu->cpu_index);
> +cpu = first_cpu;
> }
> -cpu_synchronize_state(cur_mon->mon_cpu);
> -return cur_mon->mon_cpu;
> +cpu_synchronize_state(cpu);
> +return cpu;
> }
>
> CPUArchState *mon_get_cpu_env(void)
>
>
Re: [Qemu-devel] [PATCH v4 3/8] nbd: Expose constants and structs for structured read
15.10.2017 04:01, Eric Blake wrote: Upcoming patches will implement the NBD structured reply extension [1] for both client and server roles. Declare the constants, structs, and lookup routines that will be valuable whether the server or client code is backported in isolation. This includes moving one constant from an internal header to the public header, as part of the structured read processing will be done in block/nbd-client.c rather than nbd/client.c. [1]https://github.com/NetworkBlockDevice/nbd/blob/extension-structured-reply/doc/proto.md this link may become dead in future, after merging extension spec into master... Based on patches from Vladimir Sementsov-Ogievskiy. Signed-off-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
Re: [Qemu-devel] [PATCH v1 5/6] kvm: kvm_log_start/stop are only called with known sections
On 16.10.2017 09:16, David Hildenbrand wrote:
> On 10.10.2017 15:29, Paolo Bonzini wrote:
>> On 10/10/2017 11:06, Thomas Huth wrote:
>>> On 11.09.2017 19:49, David Hildenbrand wrote:
Let's properly align the sections first and bail out if we would ever
get called with a memory section we don't know yet.
Signed-off-by: David Hildenbrand
---
accel/kvm/kvm-all.c | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index b677d1b13e..2ae459453d 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -411,15 +411,21 @@ static int kvm_slot_update_flags(KVMMemoryListener
*kml, KVMSlot *mem,
static int kvm_section_update_flags(KVMMemoryListener *kml,
MemoryRegionSection *section)
{
-hwaddr phys_addr = section->offset_within_address_space;
-ram_addr_t size = int128_get64(section->size);
-KVMSlot *mem = kvm_lookup_matching_slot(kml, phys_addr, size);
+hwaddr start_addr, size;
+KVMSlot *mem;
-if (mem == NULL) {
+size = kvm_align_section(section, &start_addr);
+if (!size) {
return 0;
-} else {
-return kvm_slot_update_flags(kml, mem, section->mr);
}
+
+mem = kvm_lookup_matching_slot(kml, start_addr, size);
+if (!mem) {
+fprintf(stderr, "%s: error finding slot\n", __func__);
+abort();
+}
>>>
>>> FYI, this abort now triggers with the "isa-vga" device:
>>>
>>> $ x86_64-softmmu/qemu-system-x86_64 -S -accel kvm -device isa-vga
>>> kvm_section_update_flags: error finding slot
>>> Aborted (core dumped)
>>>
>>> Any ideas how to fix this?
>>
>> Reverting, unless David has some time to look into it.
>>
>> Paolo
>>
>
> Just returned from vacation, I'll have a look this week.
>
> Thanks for that nice reproducer Thomas, that should help a lot!
FWIW, I've found the problem with the scripts/device-crash-test script,
so you might want to run that, too, before submitting a new version.
Thomas
[Qemu-devel] [PATCH 0/2] add "nopin" option in the memory-backend-file
Qemu does not need pin NVDIMM memory for VFIO device during VFIO hotplug, what's more, if there is no NVDIMM hw in the test machine, the VFIO hotplug operation will need at least 10 minutes to pin RAM as the NVDIMM, this time is not accepted. So we add "nopin=on" option in the memory-backed-file, which can avoid to pin RAM memory for NVDIMM. The new command like below: -object memory-backend-file,id=mem0,share,nopin=on,mem-path=kvm.img,size=9161408512 The default of "nopin" still "off" value, which is same with previous value. Yang Zhong (2): hostmem-file: Add "nopin" option for memory-backend-file nvdimm: Add "nopin" for related documents backends/hostmem-file.c | 23 +++ docs/nvdimm.txt | 10 -- hw/vfio/common.c| 12 +++- qemu-options.hx | 6 +- 4 files changed, 47 insertions(+), 4 deletions(-) -- 1.9.1
[Qemu-devel] [PATCH 1/2] hostmem-file: Add "nopin" option for memory-backend-file
Since qemu does not need pin nvdimm memory during the VFIO
hotplug, the new option can be used to avoid pin whole nvdimm
memory. The default value is still "nopin=off" as previous.
Signed-off-by: Yang Zhong
---
backends/hostmem-file.c | 23 +++
hw/vfio/common.c| 12 +++-
2 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
index e44c319..e402077 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -33,6 +33,7 @@ struct HostMemoryBackendFile {
bool share;
bool discard_data;
+bool nopin;
char *mem_path;
};
@@ -128,6 +129,25 @@ static void file_backend_unparent(Object *obj)
}
}
+static bool file_memory_backend_get_nopin(Object *o, Error **errp)
+{
+HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(o);
+
+return fb->nopin;
+}
+
+static void file_memory_backend_set_nopin(Object *o, bool value, Error **errp)
+{
+HostMemoryBackend *backend = MEMORY_BACKEND(o);
+HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(o);
+
+if (memory_region_size(&backend->mr)) {
+error_setg(errp, "cannot change property value");
+return;
+}
+fb->nopin = value;
+}
+
static void
file_backend_class_init(ObjectClass *oc, void *data)
{
@@ -142,6 +162,9 @@ file_backend_class_init(ObjectClass *oc, void *data)
object_class_property_add_bool(oc, "discard-data",
file_memory_backend_get_discard_data,
file_memory_backend_set_discard_data,
&error_abort);
+object_class_property_add_bool(oc, "nopin",
+file_memory_backend_get_nopin, file_memory_backend_set_nopin,
+&error_abort);
object_class_property_add_str(oc, "mem-path",
get_mem_path, set_mem_path,
&error_abort);
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 7b2924c..f36ff24 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -408,7 +408,8 @@ static void vfio_listener_region_add(MemoryListener
*listener,
void *vaddr;
int ret;
VFIOHostDMAWindow *hostwin;
-bool hostwin_found;
+bool hostwin_found, nopin;
+Object *obj = section->mr->owner;
if (vfio_listener_skipped_section(section)) {
trace_vfio_listener_region_add_skip(
@@ -424,6 +425,15 @@ static void vfio_listener_region_add(MemoryListener
*listener,
return;
}
+if (obj && object_dynamic_cast(obj, "memory-backend-file")) {
+nopin = object_property_get_bool(obj, "nopin", NULL);
+if (nopin) {
+error_report("warning: If VFIO DMA still map to NVDIMM memory, "
+ "the VM will crash");
+return;
+}
+}
+
iova = TARGET_PAGE_ALIGN(section->offset_within_address_space);
llend = int128_make64(section->offset_within_address_space);
llend = int128_add(llend, section->size);
--
1.9.1
[Qemu-devel] [PATCH 2/2] nvdimm: Add "nopin" for related documents
Added the "nopin" related changes in nvdimm.txt and
qemu-options.hx.
Signed-off-by: Yang Zhong
---
docs/nvdimm.txt | 10 --
qemu-options.hx | 6 +-
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/docs/nvdimm.txt b/docs/nvdimm.txt
index 2d9f8c0..41ac1c2 100644
--- a/docs/nvdimm.txt
+++ b/docs/nvdimm.txt
@@ -17,7 +17,7 @@ following command line options:
-machine pc,nvdimm
-m $RAM_SIZE,slots=$N,maxmem=$MAX_SIZE
- -object memory-backend-file,id=mem1,share=on,mem-path=$PATH,size=$NVDIMM_SIZE
+ -object
memory-backend-file,id=mem1,share=on,nopin=on,mem-path=$PATH,size=$NVDIMM_SIZE
-device nvdimm,id=nvdimm1,memdev=mem1
Where,
@@ -31,7 +31,7 @@ Where,
of normal RAM devices and vNVDIMM devices, e.g. $MAX_SIZE should be
>= $RAM_SIZE + $NVDIMM_SIZE here.
- - "object
memory-backend-file,id=mem1,share=on,mem-path=$PATH,size=$NVDIMM_SIZE"
+ - "object
memory-backend-file,id=mem1,share=on,nopin=on,mem-path=$PATH,size=$NVDIMM_SIZE"
creates a backend storage of size $NVDIMM_SIZE on a file $PATH. All
accesses to the virtual NVDIMM device go to the file $PATH.
@@ -42,6 +42,12 @@ Where,
"share=off", then guest writes won't be applied to the backend
file and thus will be invisible to other guests.
+ "nopin=on/off" controls the memory pining of memory backend file
+during the VFIO device hotplug. If "nopin=on", then VFIO device
+hotplug can skip the NVDIMM memory pining because qemu does not
+need pining NVDIMM memory for devices. If "nopin=off", the VFIO
+device hotplug need NVDIMM memory pining.
+
- "device nvdimm,id=nvdimm1,memdev=mem1" creates a virtual NVDIMM
device whose storage is provided by above memory backend device.
diff --git a/qemu-options.hx b/qemu-options.hx
index 981742d..d21ce2e 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4173,7 +4173,7 @@ property must be set. These objects are placed in the
@table @option
-@item -object
memory-backend-file,id=@var{id},size=@var{size},mem-path=@var{dir},share=@var{on|off},discard-data=@var{on|off}
+@item -object
memory-backend-file,id=@var{id},size=@var{size},mem-path=@var{dir},share=@var{on|off},discard-data=@var{on|off},nopin=@var{on|off}
Creates a memory file backend object, which can be used to back
the guest RAM with huge pages. The @option{id} parameter is a
@@ -4191,6 +4191,10 @@ to avoid unnecessarily flushing data to the backing
file. Note
that @option{discard-data} is only an optimization, and QEMU
might not discard file contents if it aborts unexpectedly or is
terminated using SIGKILL.
+Setting the @option{nopin} boolean option to @var{on} indicates
+that if the memory-backend-file is nvdimm or else, the memory
+pining can be disable during VFIO device hotplug. The default
+nopin is still off, which is same with previous value.
@item -object rng-random,id=@var{id},filename=@var{/dev/random}
--
1.9.1
[Qemu-devel] [PATCH RESEND 0/2] x86_iommu: Fix segfault when starting on non-PCI machines
Starting qemu with
qemu-system-x86_64 -S -M isapc -device {amd|intel}-iommu
leads to a segfault. The code assume PCI bus is present and
tries to access the bus structure without checking.
The patch series moves the error checks from vtd_realize()
and amdvi_realize() to the generic x86_iommu_realize() and
adds a check for PCI bus presence.
Mohammed Gamal (2):
x86_iommu: Move machine check to x86_iommu_realize()
x86_iommu: check if machine has PCI bus
hw/i386/amd_iommu.c | 13 ++---
hw/i386/intel_iommu.c | 13 ++---
hw/i386/x86-iommu.c | 13 +
3 files changed, 17 insertions(+), 22 deletions(-)
--
1.8.3.1
[Qemu-devel] [PATCH RESEND 1/2] x86_iommu: Move machine check to x86_iommu_realize()
Instead of having the same error checks in vtd_realize()
and amdvi_realize(), move that over to the generic
x86_iommu_realize().
Reviewed-by: Peter Xu
Reviewed-by: Eduardo Habkost
Signed-off-by: Mohammed Gamal
---
hw/i386/amd_iommu.c | 13 ++---
hw/i386/intel_iommu.c | 13 ++---
hw/i386/x86-iommu.c | 13 +
3 files changed, 17 insertions(+), 22 deletions(-)
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 334938a..f2e1868 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1141,18 +1141,9 @@ static void amdvi_realize(DeviceState *dev, Error **err)
AMDVIState *s = AMD_IOMMU_DEVICE(dev);
X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev);
MachineState *ms = MACHINE(qdev_get_machine());
-MachineClass *mc = MACHINE_GET_CLASS(ms);
-PCMachineState *pcms =
-PC_MACHINE(object_dynamic_cast(OBJECT(ms), TYPE_PC_MACHINE));
-PCIBus *bus;
-
-if (!pcms) {
-error_setg(err, "Machine-type '%s' not supported by amd-iommu",
- mc->name);
-return;
-}
+PCMachineState *pcms = PC_MACHINE(ms);
+PCIBus *bus = pcms->bus;
-bus = pcms->bus;
s->iotlb = g_hash_table_new_full(amdvi_uint64_hash,
amdvi_uint64_equal, g_free, g_free);
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 3a5bb0b..0138b3b 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3027,20 +3027,11 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error
**errp)
static void vtd_realize(DeviceState *dev, Error **errp)
{
MachineState *ms = MACHINE(qdev_get_machine());
-MachineClass *mc = MACHINE_GET_CLASS(ms);
-PCMachineState *pcms =
-PC_MACHINE(object_dynamic_cast(OBJECT(ms), TYPE_PC_MACHINE));
-PCIBus *bus;
+PCMachineState *pcms = PC_MACHINE(ms);
+PCIBus *bus = pcms->bus;
IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev);
X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev);
-if (!pcms) {
-error_setg(errp, "Machine-type '%s' not supported by intel-iommu",
- mc->name);
-return;
-}
-
-bus = pcms->bus;
x86_iommu->type = TYPE_INTEL;
if (!vtd_decide_config(s, errp)) {
diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c
index 293caf8..51de519 100644
--- a/hw/i386/x86-iommu.c
+++ b/hw/i386/x86-iommu.c
@@ -21,6 +21,8 @@
#include "hw/sysbus.h"
#include "hw/boards.h"
#include "hw/i386/x86-iommu.h"
+#include "hw/i386/pc.h"
+#include "qapi/error.h"
#include "qemu/error-report.h"
#include "trace.h"
@@ -80,7 +82,18 @@ static void x86_iommu_realize(DeviceState *dev, Error **errp)
{
X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev);
X86IOMMUClass *x86_class = X86_IOMMU_GET_CLASS(dev);
+MachineState *ms = MACHINE(qdev_get_machine());
+MachineClass *mc = MACHINE_GET_CLASS(ms);
+PCMachineState *pcms =
+PC_MACHINE(object_dynamic_cast(OBJECT(ms), TYPE_PC_MACHINE));
QLIST_INIT(&x86_iommu->iec_notifiers);
+
+if (!pcms) {
+error_setg(errp, "Machine-type '%s' not supported by IOMMU",
+ mc->name);
+return;
+}
+
if (x86_class->realize) {
x86_class->realize(dev, errp);
}
--
1.8.3.1
[Qemu-devel] [PATCH RESEND 2/2] x86_iommu: check if machine has PCI bus
Starting qemu with
qemu-system-x86_64 -S -M isapc -device {amd|intel}-iommu
leads to a segfault. The code assume PCI bus is present and
tries to access the bus structure without checking.
Since Intel VT-d and AMDVI should only work with PCI, add a
check for PCI bus and return error if not present.
Reviewed-by: Peter Xu
Reviewed-by: Eduardo Habkost
Signed-off-by: Mohammed Gamal
---
hw/i386/x86-iommu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c
index 51de519..8a01a2d 100644
--- a/hw/i386/x86-iommu.c
+++ b/hw/i386/x86-iommu.c
@@ -88,7 +88,7 @@ static void x86_iommu_realize(DeviceState *dev, Error **errp)
PC_MACHINE(object_dynamic_cast(OBJECT(ms), TYPE_PC_MACHINE));
QLIST_INIT(&x86_iommu->iec_notifiers);
-if (!pcms) {
+if (!pcms || !pcms->bus) {
error_setg(errp, "Machine-type '%s' not supported by IOMMU",
mc->name);
return;
--
1.8.3.1
Re: [Qemu-devel] [FIX PATCH] monitor: Don't return CPU marked for unplug as monitor CPU
On Mon, 16 Oct 2017 12:33:26 +0530
Bharata B Rao wrote:
> The following sequence of steps kill the QEMU:
>
> - Hotplug a CPU
> - Change the default CPU to the newly hotplugged cpu using "cpu" HMP command.
> - Hot unplug the CPU
> - Run "info cpus"
>
> Fix this by not letting monitor_get_cpu() to return a CPU which is marked
> for unplug.
>
> Reported-by: Satheesh Rajendran
> Signed-off-by: Bharata B Rao
> ---
> monitor.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/monitor.c b/monitor.c
> index fe0d1bd..8d60e57 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1053,7 +1053,7 @@ int monitor_set_cpu(int cpu_index)
>
> CPUState *mon_get_cpu(void)
> {
> -if (!cur_mon->mon_cpu) {
> +if (!cur_mon->mon_cpu || cur_mon->mon_cpu->unplug) {
mon_cpu could be freed so it would cause use after free,
Greg was looking into the same issue see his patch:
"PATCH] monitor: fix dangling CPU pointer"
> if (!first_cpu) {
> return NULL;
> }
[Qemu-devel] [PATCH v1] memory: call log_start after region_add
It might be confusing for some listener implementations that implement
both, region_add and log_start (e.g. KVM) if we call log_start before an
actual region was added using region_add.
This makes current KVM code trigger an assertion
("kvm_section_update_flags: error finding slot"). So let's just reverse
the order instead of tolerating log_start on yet unknown regions.
Reported-by: Thomas Huth
Signed-off-by: David Hildenbrand
---
memory.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/memory.c b/memory.c
index 5e6351a6c1..f39b8592bf 100644
--- a/memory.c
+++ b/memory.c
@@ -2607,12 +2607,12 @@ static void listener_add_address_space(MemoryListener
*listener,
.offset_within_address_space = int128_get64(fr->addr.start),
.readonly = fr->readonly,
};
-if (fr->dirty_log_mask && listener->log_start) {
-listener->log_start(listener, §ion, 0, fr->dirty_log_mask);
-}
if (listener->region_add) {
listener->region_add(listener, §ion);
}
+if (fr->dirty_log_mask && listener->log_start) {
+listener->log_start(listener, §ion, 0, fr->dirty_log_mask);
+}
}
if (listener->commit) {
listener->commit(listener);
--
2.13.5
Re: [Qemu-devel] [PATCH v1 5/6] kvm: kvm_log_start/stop are only called with known sections
On 16.10.2017 10:52, Thomas Huth wrote:
> On 16.10.2017 09:16, David Hildenbrand wrote:
>> On 10.10.2017 15:29, Paolo Bonzini wrote:
>>> On 10/10/2017 11:06, Thomas Huth wrote:
On 11.09.2017 19:49, David Hildenbrand wrote:
> Let's properly align the sections first and bail out if we would ever
> get called with a memory section we don't know yet.
>
> Signed-off-by: David Hildenbrand
> ---
> accel/kvm/kvm-all.c | 18 --
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index b677d1b13e..2ae459453d 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -411,15 +411,21 @@ static int kvm_slot_update_flags(KVMMemoryListener
> *kml, KVMSlot *mem,
> static int kvm_section_update_flags(KVMMemoryListener *kml,
> MemoryRegionSection *section)
> {
> -hwaddr phys_addr = section->offset_within_address_space;
> -ram_addr_t size = int128_get64(section->size);
> -KVMSlot *mem = kvm_lookup_matching_slot(kml, phys_addr, size);
> +hwaddr start_addr, size;
> +KVMSlot *mem;
>
> -if (mem == NULL) {
> +size = kvm_align_section(section, &start_addr);
> +if (!size) {
> return 0;
> -} else {
> -return kvm_slot_update_flags(kml, mem, section->mr);
> }
> +
> +mem = kvm_lookup_matching_slot(kml, start_addr, size);
> +if (!mem) {
> +fprintf(stderr, "%s: error finding slot\n", __func__);
> +abort();
> +}
FYI, this abort now triggers with the "isa-vga" device:
$ x86_64-softmmu/qemu-system-x86_64 -S -accel kvm -device isa-vga
kvm_section_update_flags: error finding slot
Aborted (core dumped)
Any ideas how to fix this?
>>>
>>> Reverting, unless David has some time to look into it.
>>>
>>> Paolo
>>>
>>
>> Just returned from vacation, I'll have a look this week.
>>
>> Thanks for that nice reproducer Thomas, that should help a lot!
>
> FWIW, I've found the problem with the scripts/device-crash-test script,
> so you might want to run that, too, before submitting a new version.
>
> Thomas
>
This one was easy to fix, there was just one place where the asssumption
"only called with known sections" was wrong.
See "memory: call log_start after region_add"
--
Thanks,
David
Re: [Qemu-devel] [PULL 00/11] Ui 20171013 patches
Hi, > I'm afraid this is quite likely to fail build smoke test again. I've > just tried a build on OpenBSD with the bash -> sh fix in it, and I > found that it still tried to build the keycodemap files in parallel > with checking out the GIT submodules. It also tried to run the > mkdir -p dtc/libfdt in parallel and this caused git to refuse to > checkout the dtc module due to that empty dir existing. So there's > still some deps problems in here I think that let make build in > the wrong order :-( > In all my debugging the one thing I've seen work correctly is the > re-running of configure (via config.status), which always happens > earlier and is serialized wrt everything else. So I wonder if we > should change direction slightly, and have configure checkout the > submodules. Then just make sure that config.status is triggered > when submodules are out of date. I've noticed configure running *in parallel* to other stuff. Reproducer: (1) Apply patch #1 (2) run normal build (3) make -C dtc clean (4) touch configure (5) make Watch configure and dtc build running in parallel. I think the added Makefile dependency breaks it. Incremental fix (also pushed to queue/ui): --- a/Makefile +++ b/Makefile @@ -35,8 +35,6 @@ endif .git-submodule-status: git-submodule-update config-host.mak -Makefile: .git-submodule-status - # Check that we're not trying to do an out-of-tree build from # a tree that's been used for an in-tree build. ifneq ($(realpath $(SRC_PATH)),$(realpath .)) @@ -107,6 +105,7 @@ endif GENERATED_FILES += $(TRACE_HEADERS) GENERATED_FILES += $(TRACE_SOURCES) GENERATED_FILES += $(BUILD_DIR)/trace-events-all +GENERATED_FILES += .git-submodule-status trace-group-name = $(shell dirname $1 | sed -e 's/[^a-zA-Z0-9]/_/g') cheers, Gerd
Re: [Qemu-devel] [PULL 00/13] target-arm queue
On 12 October 2017 at 17:03, Peter Maydell wrote: > target-arm queue: > * mostly my latest v8M stuff, plus a couple of minor patches > > The following changes since commit a0b261db8c030813e30a39eae47359ac2a37f7e2: > > Merge remote-tracking branch > 'remotes/ehabkost/tags/python-next-pull-request' into staging (2017-10-12 > 10:02:09 +0100) > > are available in the git repository at: > > git://git.linaro.org/people/pmaydell/qemu-arm.git > tags/pull-target-arm-20171012 > > for you to fetch changes up to cf5f7937b05c84d5565134f058c00cd48304a117: > > nvic: Fix miscalculation of offsets into ITNS array (2017-10-12 16:33:16 > +0100) > > > target-arm queue: > * v8M: SG, BLXNS, secure-return > * v8M: fixes for coverity issues in previous patches > * arm: fix armv7m_init() declaration to match definition > * watchdog/aspeed: fix variable type to store reload value > Applied, thanks. -- PMM
Re: [Qemu-devel] [PATCH RESEND 0/2] x86_iommu: Fix segfault when starting on non-PCI machines
On Mon, Oct 16, 2017 at 11:06:21AM +0200, Mohammed Gamal wrote:
> Starting qemu with
> qemu-system-x86_64 -S -M isapc -device {amd|intel}-iommu
> leads to a segfault. The code assume PCI bus is present and
> tries to access the bus structure without checking.
>
> The patch series moves the error checks from vtd_realize()
> and amdvi_realize() to the generic x86_iommu_realize() and
> adds a check for PCI bus presence.
Hi, Michael,
Would you like to pick Mohammed's two patches in your next pull
request?
Each of the patches has got 2 acks, and it does fix a problem.
Thanks,
>
>
> Mohammed Gamal (2):
> x86_iommu: Move machine check to x86_iommu_realize()
> x86_iommu: check if machine has PCI bus
>
> hw/i386/amd_iommu.c | 13 ++---
> hw/i386/intel_iommu.c | 13 ++---
> hw/i386/x86-iommu.c | 13 +
> 3 files changed, 17 insertions(+), 22 deletions(-)
>
> --
> 1.8.3.1
>
--
Peter Xu
Re: [Qemu-devel] using which notification for guest about GHES error
On Mon, 16 Oct 2017 14:10:05 +0800
gengdongjiu wrote:
> Hi Igor/Michael,
>I am very sorry to disturb you again.
> Now we use Qemu to create APEI table and record CPER for guest,
> After QEMU recorded a asynchronous CPER error, we needs to notify guest using
> interrupt or Polled notification.
> For the asynchronous error. I think using GPIO-signaled notification may be
> better in the Qemu, and also which is suggested by APEI spec.
> James worried that old guest OS may not support GPIO or GSIV notification for
> GHES, because GPIO or GSIV notification is supported in OS since about kernel
> version 4.10.
How APEI support is fairly new on ARM (kernel), isn't it still in state of
development?
Do we really care about old guests in this case?
We'd like to stick to ACPI spec as much as possible and also to
http://infocenter.arm.com/help/topic/com.arm.doc.den0044b/DEN0044B_Server_Base_Boot_Requirements.pdf
which mandates GPIO in platform (QEMU)
"
4.5 Hardware Requirements Imposed on the Platform by ACPI
...
Platforms compliant with this specification must provide the following
GPIO-Signaled platform events:
...
"
> and suggested using Polled notification. About above two notifications, do
> you think which is better? and could you give us some suggestion? thanks.
how polling is supposed to be implemented in QEMU?
>
>
> Hi James,
> Below is APEI spec, From the spec, it suggested using GPIO interrupt or
> GPIO-signaled events in ARM64 [1]. If using Polled notification for GHES, I
> do not sure whether it is reasonable.
> In the Qemu, X86 does not using Polled notification. it mainly use SCI. Until
> now, I do not found there is peopled using Polled notification in qemu. if
> implemented polled notification, I
> do not know how much work effort need to do. Now I have already implemented
> the GPIO-Signal notification using GPIO pin.
>
>
> [1]
> HW-reduced ACPI platforms signal the error using a GPIO interrupt or another
> interrupt declared under a generic event device (Section 5.6.9). In the case
> of GPIO-signaled events,
> an _AEI object lists the appropriate GPIO pin, while for Interrupt-signaled
> events a _CRS object is used to list the interrupt:
> • The OSPM evaluates the control method associated with this event as
> indicated in Section 5.6.5.3 and Section 5.6.9.3.
> • OSPM responds to this notification by checking the error status block
> of all generic error sources with the GPIO-Signal notification or
> Interrupt-signaled notification types to identify the
> source reporting the error.
>
>
>
>
>
> On 2017/10/1 11:30, Michael S. Tsirkin wrote:
> > On Thu, Sep 28, 2017 at 06:10:05PM +0800, gengdongjiu wrote:
> >>
> >>
> >> On 2017/9/28 6:15, Michael S. Tsirkin wrote:
> >>> On Wed, Sep 27, 2017 at 07:32:35PM +0800, gengdongjiu wrote:
> Hi Igor/Laszlo
>
> I am very sorry to disturb you. I have a question that want to
> consult with you.
> Now In ARM64 platform I need to send a IRQ notification to guest OS
> through Qemu, so that guest OS can receive a IRQ notification and parse
> the APEI table.
> but Now I do not find a proper API to send the IRQ notification to
> guest, seems X86 mainly uses the API "acpi_send_gpe_event"
> in the ARM64 platform, whether we have existed API that can send IRQ
> notification to guest? thank you very much in advance.
> >>>
> >>> ACPI events fundamentally go through the GPE mechanism. As the spec
> >>> says:
> >>>
> >>> ACPI Event Programming Model
> >>> The ACPI event programming model is based on the SCI interrupt and
> >>> General-Purpose Event
> >>> (GPE) register.
> >>
> >> got it, thanks Michael
> >
> > As Igor pointed out, the next spec sentence states that reduced HW
> > configurations use GPIO.
> >
> >>>
> >>>
>
> [1]
> void acpi_send_gpe_event(ACPIREGS *ar, qemu_irq irq,
> AcpiEventStatusBits status)
> {
> ar->gpe.sts[0] |= status;
> acpi_update_sci(ar, irq);
> }
>
> void acpi_update_sci(ACPIREGS *regs, qemu_irq irq)
> {
> int sci_level, pm1a_sts;
>
> pm1a_sts = acpi_pm1_evt_get_sts(regs);
>
> sci_level = ((pm1a_sts &
> regs->pm1.evt.en & ACPI_BITMASK_PM1_COMMON_ENABLED) !=
> 0) ||
> ((regs->gpe.sts[0] & regs->gpe.en[0]) != 0);
>
> qemu_set_irq(irq, sci_level);
>
> /* schedule a timer interruption if needed */
> acpi_pm_tmr_update(regs,
> (regs->pm1.evt.en & ACPI_BITMASK_TIMER_ENABLE) &&
> !(pm1a_sts & ACPI_BITMASK_TIMER_STATUS));
> }
> >>>
> >>> .
> >>>
> >
> > .
> >
>
>
Re: [Qemu-devel] [PATCH v1] memory: call log_start after region_add
On 16.10.2017 11:18, David Hildenbrand wrote:
> It might be confusing for some listener implementations that implement
> both, region_add and log_start (e.g. KVM) if we call log_start before an
> actual region was added using region_add.
>
> This makes current KVM code trigger an assertion
> ("kvm_section_update_flags: error finding slot"). So let's just reverse
> the order instead of tolerating log_start on yet unknown regions.
>
> Reported-by: Thomas Huth
> Signed-off-by: David Hildenbrand
> ---
> memory.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/memory.c b/memory.c
> index 5e6351a6c1..f39b8592bf 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -2607,12 +2607,12 @@ static void listener_add_address_space(MemoryListener
> *listener,
> .offset_within_address_space = int128_get64(fr->addr.start),
> .readonly = fr->readonly,
> };
> -if (fr->dirty_log_mask && listener->log_start) {
> -listener->log_start(listener, §ion, 0, fr->dirty_log_mask);
> -}
> if (listener->region_add) {
> listener->region_add(listener, §ion);
> }
> +if (fr->dirty_log_mask && listener->log_start) {
> +listener->log_start(listener, §ion, 0, fr->dirty_log_mask);
> +}
> }
> if (listener->commit) {
> listener->commit(listener);
>
... deciding to tolerate it in KVM instead (just as it used to be), so
please ignore this patch.
--
Thanks,
David
Re: [Qemu-devel] [PATCH qemu v3] RFC: ppc/spapr: Receive and store device tree blob from SLOF
On Mon, Oct 16, 2017 at 04:20:04PM +1100, Alexey Kardashevskiy wrote:
> SLOF receives a device tree and updates it with various properties
> before switching to the guest kernel and QEMU is not aware of any changes
> made by SLOF. Since there is no real RTAS and QEMU implements it,
> it makes sense to pass the SLOF device tree to QEMU so the latter could
> implement RTAS related tasks better.
>
> Specifially, now QEMU can find out the actual XICS phandle (for PHB
> hotplug) and the RTAS linux,rtas-entry/base properties (for firmware
> assisted NMI - FWNMI).
>
> This stores the initial DT blob in the sPAPR machine and replaces it
> in the KVMPPC_H_UPDATE_DT (new private hypercall) handler.
>
> This implements a basic FDT header validity check of the new blob;
> the new blob size should not increase more than twice since the reset.
>
> This adds a machine property - update_dt_enabled - to allow backward
> migration.
>
> This requires SLOF update: "fdt: Pass the resulting device tree to QEMU".
>
> Signed-off-by: Alexey Kardashevskiy
> ---
> Changes:
> v3:
> * store reset-time FDT blob size and compare the update size against it;
> this disallows more than 2x increase between resets
> * added some FDT header sanity checks
> * implemented migration
>
> ---
> I could store just a size of the QEMU's blob, or a tree, not sure
> which one makes more sense here.
>
> This allows up to 2 times blob increase. Not 1.5 just to avoid
> float/double, just looks a bit ugly imho.
> ---
> include/hw/ppc/spapr.h | 7 ++-
> hw/ppc/spapr.c | 31 ++-
> hw/ppc/spapr_hcall.c | 42 ++
> hw/ppc/trace-events| 2 ++
> 4 files changed, 80 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index c1b365f564..a9ccc14823 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -60,6 +60,7 @@ struct sPAPRMachineClass {
> /*< public >*/
> bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of LMBs */
> bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */
> +bool update_dt_enabled;/* enable KVMPPC_H_UPDATE_DT */
> const char *tcg_default_cpu; /* which (TCG) CPU to simulate by default */
> bool pre_2_10_has_unused_icps;
> void (*phb_placement)(sPAPRMachineState *spapr, uint32_t index,
> @@ -92,6 +93,9 @@ struct sPAPRMachineState {
> int vrma_adjust;
> ssize_t rtas_size;
> void *rtas_blob;
> +uint32_t fdt_size;
> +uint32_t fdt_initial_size;
> +void *fdt_blob;
> long kernel_size;
> bool kernel_le;
> uint32_t initrd_base;
> @@ -400,7 +404,8 @@ struct sPAPRMachineState {
> #define KVMPPC_H_LOGICAL_MEMOP (KVMPPC_HCALL_BASE + 0x1)
> /* Client Architecture support */
> #define KVMPPC_H_CAS(KVMPPC_HCALL_BASE + 0x2)
> -#define KVMPPC_HCALL_MAXKVMPPC_H_CAS
> +#define KVMPPC_H_UPDATE_DT (KVMPPC_HCALL_BASE + 0x3)
> +#define KVMPPC_HCALL_MAXKVMPPC_H_UPDATE_DT
>
> typedef struct sPAPRDeviceTreeUpdateHeader {
> uint32_t version_id;
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index ff87f155d5..cb7bcc851e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1467,7 +1467,10 @@ static void ppc_spapr_reset(void)
> /* Load the fdt */
> qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
> cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
> -g_free(fdt);
> +g_free(spapr->fdt_blob);
> +spapr->fdt_size = fdt_totalsize(fdt);
> +spapr->fdt_initial_size = spapr->fdt_size;
> +spapr->fdt_blob = fdt;
>
> /* Set up the entry state */
> first_ppc_cpu = POWERPC_CPU(first_cpu);
> @@ -1675,6 +1678,27 @@ static const VMStateDescription
> vmstate_spapr_patb_entry = {
> },
> };
>
> +static bool spapr_dtb_needed(void *opaque)
> +{
> +sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(opaque);
> +
> +return smc->update_dt_enabled;
> +}
> +
> +static const VMStateDescription vmstate_spapr_dtb = {
> +.name = "spapr_dtb",
> +.version_id = 1,
> +.minimum_version_id = 1,
> +.needed = spapr_dtb_needed,
> +.fields = (VMStateField[]) {
> +VMSTATE_UINT32(fdt_initial_size, sPAPRMachineState),
I'm not sure you need to migrate initial size. The destination can
work this out itself.. unless we skip the reset when we have an
incoming migration, I'm not sure.
> +VMSTATE_UINT32(fdt_size, sPAPRMachineState),
> +VMSTATE_VBUFFER_ALLOC_UINT32(fdt_blob, sPAPRMachineState, 0, NULL,
> + fdt_size),
> +VMSTATE_END_OF_LIST()
> +},
> +};
> +
> static const VMStateDescription vmstate_spapr = {
> .name = "spapr",
> .version_id = 3,
> @@ -1694,6 +1718,7 @@ static const VMStateDescription vmstate_spapr = {
> &vmstate_spapr_ov5_cas,
> &vmstate_spapr_patb_entry,
> &vmstate_spapr_pending_events,
> +&vmst
[Qemu-devel] [PATCH v1] kvm: tolerate non-existing slot for log_start and log_stop
log_start might be called by memory.c just before registering the
section. So we can actually get a log_start without a region_add, which
we can silently ignore.
This makes current KVM code trigger an assertion
("kvm_section_update_flags: error finding slot").
Also, if we want to trap every access to a section, we might not have a
slot. So let's just tolerate if we don't have a slot.
Fixes: 343562e8fa22 ("kvm: kvm_log_start/stop are only called with known
sections")
Reported-by: Thomas Huth
Signed-off-by: David Hildenbrand
---
accel/kvm/kvm-all.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 90c88b517d..64de8461e0 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -394,8 +394,11 @@ static int kvm_section_update_flags(KVMMemoryListener *kml,
mem = kvm_lookup_matching_slot(kml, start_addr, size);
if (!mem) {
-fprintf(stderr, "%s: error finding slot\n", __func__);
-abort();
+/*
+ * log_start() might be called before region_add(), and sometimes
+ * we don't have a slot as we want to trap every access.
+ */
+return 0;
}
return kvm_slot_update_flags(kml, mem, section->mr);
--
2.13.5
Re: [Qemu-devel] [PATCH v3 00/32] Migration: postcopy failure recovery
On Mon, Oct 16, 2017 at 12:32:10AM -0700, no-re...@patchew.org wrote: > Hi, > > This series failed build test on s390x host. Please find the details below. > > Type: series > Subject: [Qemu-devel] [PATCH v3 00/32] Migration: postcopy failure recovery > Message-id: 20171016065216.18162-1-pet...@redhat.com > > === TEST SCRIPT BEGIN === > #!/bin/bash > # Testing script will be invoked under the git checkout with > # HEAD pointing to a commit that has the patches applied on top of "base" > # branch > set -e > echo "=== ENV ===" > env > echo "=== PACKAGES ===" > rpm -qa > echo "=== TEST BEGIN ===" > CC=$HOME/bin/cc > INSTALL=$PWD/install > BUILD=$PWD/build > echo -n "Using CC: " > realpath $CC > mkdir -p $BUILD $INSTALL > SRC=$PWD > cd $BUILD > $SRC/configure --cc=$CC --prefix=$INSTALL > make -j4 > # XXX: we need reliable clean up > # make check -j4 V=1 > make install > === TEST SCRIPT END === [...] > CC migration/postcopy-ram.o > CC migration/qjson.o > CC migration/block.o > CC net/net.o > CC net/queue.o > /var/tmp/patchew-tester-tmp-nm4b3qpv/src/migration/postcopy-ram.c: In > function ‘postcopy_ram_fault_thread’: > /var/tmp/patchew-tester-tmp-nm4b3qpv/src/migration/postcopy-ram.c:553:13: > error: ignoring return value of ‘read’, declared with attribute > warn_unused_result [-Werror=unused-result] > read(mis->userfault_event_fd, &tmp64, 8); > ^~~~ Need to capture read() return code. Fixed. I captured this as well using "make docker-test-build@fedora". Anyone knows why I didn't encounter this on my Fedora 26 host? My gcc version: 7.2.1 20170915 (Red Hat 7.2.1-2), which seems to be quite new as well. Would also appreciate if anyone can provide a minimum docker test subset (TEST & IMAGE) that I'd run before posting patches. TIA. > cc1: all warnings being treated as errors > /var/tmp/patchew-tester-tmp-nm4b3qpv/src/rules.mak:66: recipe for target > 'migration/postcopy-ram.o' failed > make: *** [migration/postcopy-ram.o] Error 1 > make: *** Waiting for unfinished jobs > === OUTPUT END === > > Test command exited with code: 2 > > > --- > Email generated automatically by Patchew [http://patchew.org/]. > Please send your feedback to patchew-de...@freelists.org -- Peter Xu
Re: [Qemu-devel] [PATCH v11 3/5] msf2: Add Smartfusion2 SPI controller
Hi Peter,
On Tue, Oct 10, 2017 at 6:24 PM, Peter Maydell
wrote:
> On 20 September 2017 at 21:17, Philippe Mathieu-Daudé
> wrote:
> > From: Subbaraya Sundeep
> >
> > Modelled Microsemi's Smartfusion2 SPI controller.
> >
> > Signed-off-by: Subbaraya Sundeep
> > Reviewed-by: Alistair Francis
> > Tested-by: Philippe Mathieu-Daudé
>
> > +#define FRAMESZ_MASK 0x1F
>
> > +static void set_fifodepth(MSSSpiState *s)
> > +{
> > +unsigned int size = s->regs[R_SPI_DFSIZE] & FRAMESZ_MASK;
> > +
> > +if (size <= 8) {
> > +s->fifo_depth = 32;
> > +} else if (size <= 16) {
> > +s->fifo_depth = 16;
> > +} else if (size <= 32) {
> > +s->fifo_depth = 8;
> > +} else {
> > +s->fifo_depth = 4;
> > +}
> > +}
>
> Hi. Coverity points out (CID 1381483) that the "else" case here
> is dead code, because the FRAMESZ_MASK of 0x1F means that size
> cannot be 32 or more.
>
> Paolo kindly checked up with the spec at
> https://www.eecs.umich.edu/courses/eecs373/readings/Actel_SmartFusion_MSS_
> UserGuide.pdf
> which says that this register's field is bits [5:0] which
> would imply an 0x3f mask is needed. On the other hand it also
> says that "maximum value is 32", so what is the else clause
> doing anyway?
>
I will remove the else, change mask to 0x3F and add check for max 32 in
spi_write:
case R_SPI_DFSIZE:
if (s->enabled || (value & FRAMESZ_MASK) > 32) {
break;
}
s->regs[R_SPI_DFSIZE] = value;
break;
Thanks for pointing out.
Sundeep
>
> thanks
> -- PMM
>
Re: [Qemu-devel] [PATCH v3 00/32] Migration: postcopy failure recovery
On Mon, Oct 16, 2017 at 01:05:04AM -0700, no-re...@patchew.org wrote: > Hi, > > This series failed automatic build test. Please find the testing commands and > their output below. If you have docker installed, you can probably reproduce > it > locally. > > Type: series > Message-id: 20171016065216.18162-1-pet...@redhat.com > Subject: [Qemu-devel] [PATCH v3 00/32] Migration: postcopy failure recovery > > === TEST SCRIPT BEGIN === > #!/bin/bash > set -e > git submodule update --init dtc > # Let docker tests dump environment info > export SHOW_ENV=1 > export J=8 > time make docker-test-quick@centos6 > time make docker-test-build@min-glib > time make docker-test-mingw@fedora > time make docker-test-block@fedora > === TEST SCRIPT END === [...] > GEN trace/generated-helpers.c > CC aarch64-softmmu/trace/control-target.o > CC aarch64-softmmu/gdbstub-xml.o > CC aarch64-softmmu/trace/generated-helpers.o > LINKaarch64-softmmu/qemu-system-aarch64.exe > ../migration/savevm.o: In function `postcopy_pause_incoming': > /tmp/qemu-test/src/migration/savevm.c:2174: undefined reference to > `postcopy_fault_thread_notify' I'm moving postcopy_fault_thread_notify() after the #endif of: #if defined(__linux__) && defined(__NR_userfaultfd) && defined(CONFIG_EVENTFD) Then problem solved. Will hold before reposting until I got more comments. Thanks, -- Peter Xu
Re: [Qemu-devel] [PATCH v4 4/8] nbd: Minimal structured read for server
15.10.2017 04:01, Eric Blake wrote:
From: Vladimir Sementsov-Ogievskiy
Minimal implementation of structured read: one structured reply chunk,
no segmentation.
Minimal structured error implementation: no text message.
Support DF flag, but just ignore it, as there is no segmentation any
way.
Signed-off-by: Vladimir Sementsov-Ogievskiy
Signed-off-by: Eric Blake
---
v4: better _DF flag handling, convert errno to wire format, add
comments and tracing, rework structured error for less churn when adding
text message later, don't kill connection on redundant client option
---
nbd/server.c | 106 +--
nbd/trace-events | 2 ++
2 files changed, 98 insertions(+), 10 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index efb6003364..23dc6be708 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -100,6 +100,8 @@ struct NBDClient {
QTAILQ_ENTRY(NBDClient) next;
int nb_requests;
bool closing;
+
+bool structured_reply;
};
/* That's all folks */
@@ -762,6 +764,23 @@ static int nbd_negotiate_options(NBDClient *client,
uint16_t myflags,
return ret;
}
break;
+
+case NBD_OPT_STRUCTURED_REPLY:
+if (client->structured_reply) {
+ret = nbd_negotiate_send_rep_err(
+client->ioc, NBD_REP_ERR_INVALID, option, errp,
+"structured reply already negotiated");
You were going to send a patch to spec for this..
+} else {
+ret = nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
+ option, errp);
+}
+if (ret < 0) {
+return ret;
+}
+client->structured_reply = true;
+myflags |= NBD_CMD_FLAG_DF;
it should be NBD_FLAG_SEND_DF
[...]
the following looks ok.
--
Best regards,
Vladimir
Re: [Qemu-devel] [PATCH] checkpatch: replace ERROR with WARN forextern checking.
> On 11 October 2017 at 05:33, Jiang Biao wrote: > > I'd rather not drop this to a warning for the sake of a single > use case that's already in the tree (and which if you really > cared about you could work around by putting the link_error() > declaration in a header file I suppose, though I wouldn't > bother personally.) Neither would I :). Thanks for the reply. Regards, Jiang
Re: [Qemu-devel] [PATCH v1] kvm: tolerate non-existing slot for log_start and log_stop
On 16.10.2017 11:41, David Hildenbrand wrote:
> log_start might be called by memory.c just before registering the
> section. So we can actually get a log_start without a region_add, which
> we can silently ignore.
>
> This makes current KVM code trigger an assertion
> ("kvm_section_update_flags: error finding slot").
>
> Also, if we want to trap every access to a section, we might not have a
> slot. So let's just tolerate if we don't have a slot.
>
> Fixes: 343562e8fa22 ("kvm: kvm_log_start/stop are only called with known
> sections")
> Reported-by: Thomas Huth
> Signed-off-by: David Hildenbrand
> ---
> accel/kvm/kvm-all.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 90c88b517d..64de8461e0 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -394,8 +394,11 @@ static int kvm_section_update_flags(KVMMemoryListener
> *kml,
>
> mem = kvm_lookup_matching_slot(kml, start_addr, size);
> if (!mem) {
> -fprintf(stderr, "%s: error finding slot\n", __func__);
> -abort();
> +/*
> + * log_start() might be called before region_add(), and sometimes
> + * we don't have a slot as we want to trap every access.
> + */
> +return 0;
> }
>
> return kvm_slot_update_flags(kml, mem, section->mr);
>
I'll also send a patch for log_sync(), dropping the same assert.
Looks like adding these assertions was counter productive :)
--
Thanks,
David
Re: [Qemu-devel] QEMU terminates during reboot after memory unplug with vhost=on
On Thu, Sep 14, 2017 at 10:59:05AM +0200, Igor Mammedov wrote:
> On Thu, 14 Sep 2017 13:48:26 +0530
> Bharata B Rao wrote:
>
> > On Thu, Sep 14, 2017 at 10:00:11AM +0200, Igor Mammedov wrote:
> > > On Thu, 14 Sep 2017 12:31:18 +0530
> > > Bharata B Rao wrote:
> > >
> > > > Hi,
> > > >
> > > > QEMU hits the below assert
> > > >
> > > > qemu-system-ppc64: used ring relocated for ring 2
> > > > qemu-system-ppc64: qemu/hw/virtio/vhost.c:649: vhost_commit: Assertion
> > > > `r >= 0' failed.
> > > >
> > > > in the following scenario:
> > > >
> > > > 1. Boot guest with vhost=on
> > > > -netdev
> > > > tap,id=mynet0,script=qemu-ifup,downscript=qemu-ifdown,vhost=on -device
> > > > virtio-net-pci,netdev=mynet0
> > > > 2. Hot add a DIMM device
> > > > 3. Reboot
> > > >When the guest reboots, we can see
> > > >vhost_virtqueue_start:vq->used_phys getting assigned an address that
> > > >falls in the hotplugged memory range.
> > > > 4. Remove the DIMM device
> > > >Guest refuses the removal as the hotplugged memory is under use.
> > > > 5. Reboot
> > >
> > > >QEMU forces the removal of the DIMM device during reset and that's
> > > >when we hit the above assert.
> > > I don't recall implementing forced removal om DIMM,
> > > could you point out to the related code, pls?
> >
> > This is ppc specific. We have DR Connector objects for each LMB (multiple
> > LMBs make up one DIMM device) and during reset we invoke the
> > release routine for these LMBs which will further invoke
> > pc_dimm_memory_unplug().
> >
> > See hw/ppc/spapr_drc.c: spapr_drc_reset()
> > hw/ppc/spapr.c: spapr_lmb_release()
> >
> > >
> > > > Any pointers on why we are hitting this assert ? Shouldn't vhost be
> > > > done with using the hotplugged memory when we hit reset ?
> > >
> > > >From another point of view,
> > > DIMM shouldn't be removed unless guest explicitly ejects it
> > > (at least that should be so in x86 case).
> >
> > While that is true for ppc also, shouldn't we start fresh from reset ?
> we should.
>
> when it aborts vhost should print out error from vhost_verify_ring_mappings()
>
>if (r == -ENOMEM) {
>error_report("Unable to map %s for ring %d", part_name[j], i);
>
>} else if (r == -EBUSY) {
>
>error_report("%s relocated for ring %d", part_name[j], i);
>
> that might give a clue where that memory stuck in.
>
> Michael might point out where to start look at, but he's on vacation
> so ...
Michael (or anyone else) - Any pointers on this problem ?
Regards,
Bharata.
Re: [Qemu-devel] [PULL 00/11] Ui 20171013 patches
On Mon, Oct 16, 2017 at 11:19:13AM +0200, Gerd Hoffmann wrote: > Hi, > > > I'm afraid this is quite likely to fail build smoke test again. I've > > just tried a build on OpenBSD with the bash -> sh fix in it, and I > > found that it still tried to build the keycodemap files in parallel > > with checking out the GIT submodules. It also tried to run the > > mkdir -p dtc/libfdt in parallel and this caused git to refuse to > > checkout the dtc module due to that empty dir existing. So there's > > still some deps problems in here I think that let make build in > > the wrong order :-( > > > In all my debugging the one thing I've seen work correctly is the > > re-running of configure (via config.status), which always happens > > earlier and is serialized wrt everything else. So I wonder if we > > should change direction slightly, and have configure checkout the > > submodules. Then just make sure that config.status is triggered > > when submodules are out of date. > > I've noticed configure running *in parallel* to other stuff. > Reproducer: > > (1) Apply patch #1 > (2) run normal build > (3) make -C dtc clean > (4) touch configure > (5) make > > Watch configure and dtc build running in parallel. > I think the added Makefile dependency breaks it. > > Incremental fix (also pushed to queue/ui): > > --- a/Makefile > +++ b/Makefile > @@ -35,8 +35,6 @@ endif > > .git-submodule-status: git-submodule-update config-host.mak > > -Makefile: .git-submodule-status > - > # Check that we're not trying to do an out-of-tree build from > # a tree that's been used for an in-tree build. > ifneq ($(realpath $(SRC_PATH)),$(realpath .)) > @@ -107,6 +105,7 @@ endif > GENERATED_FILES += $(TRACE_HEADERS) > GENERATED_FILES += $(TRACE_SOURCES) > GENERATED_FILES += $(BUILD_DIR)/trace-events-all > +GENERATED_FILES += .git-submodule-status > > trace-group-name = $(shell dirname $1 | sed -e 's/[^a-zA-Z0-9]/_/g') Yep, that looks to have fixed the races I was able to reproduce Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH] configure: pick the right compiler for OpenBSD by default
On Mon, Oct 16, 2017 at 09:44:42AM +0200, Gerd Hoffmann wrote: > Hi, > > > By the way, looks like OpenBSD is also switching to clang by default > > soon: > > > > https://www.phoronix.com/scan.php?page=news_item&px=OpenBSD-Default- > > Clang > > It did happen already. On openbsd 6.2 (released a week ago) cc is > clang: > > $ cc --version > OpenBSD clang version 4.0.0 (tags/RELEASE_400/final) (based on LLVM > 4.0.0) Oh, I didn't notice that was out already. Lets drop my patch and I'll update the BSD page on the wiki Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 0/2] add "nopin" option in the memory-backend-file
On 10/16/2017 04:56 PM, Yang Zhong wrote: Qemu does not need pin NVDIMM memory for VFIO device during VFIO hotplug, what's more, if there is no NVDIMM hw in the test machine, the VFIO hotplug operation will need at least 10 minutes to pin RAM as the NVDIMM, this time is not accepted. So we add "nopin=on" option in the memory-backed-file, which can avoid to pin RAM memory for NVDIMM. No. memory-backed-file does not dedicate for nvdimm only, it can be mapped as normal memory as well. Rather more, this is no way to stop guest to use it as DMA.
Re: [Qemu-devel] [PATCH v4 5/8] nbd/server: Include human-readable message in structured errors
15.10.2017 04:01, Eric Blake wrote:
The NBD spec permits including a human-readable error string if
structured replies are in force, so we might as well send the
client the message that we logged on any error.
Signed-off-by: Eric Blake
---
nbd/server.c | 22 --
nbd/trace-events | 2 +-
2 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index 23dc6be708..8085d79076 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1289,23 +1289,25 @@ static int coroutine_fn
nbd_co_send_structured_read(NBDClient *client,
static int coroutine_fn nbd_co_send_structured_error(NBDClient *client,
uint64_t handle,
uint32_t error,
+ char *msg,
it's not const because we want to put it into iov..
may be it is better to make it const and convert to (char *) like in
qio_channel_write_all, to
1: more native message parameter type
2: allow constat strings like nbd_co_send_structured_error(..., "some
error")
Error **errp)
{
NBDStructuredError chunk;
int nbd_err = system_errno_to_nbd_errno(error);
struct iovec iov[] = {
{.iov_base = &chunk, .iov_len = sizeof(chunk)},
-/* FIXME: Support human-readable error message */
+{.iov_base = msg, .iov_len = strlen(msg)},
};
msg is always non-zero? so we don't want to send errors without
message.. (1)
assert(nbd_err);
-trace_nbd_co_send_structured_error(handle, nbd_err);
+trace_nbd_co_send_structured_error(handle, nbd_err,
+ nbd_err_lookup(nbd_err), msg);
it doesn't really matter, but "nbd_err_lookup(nbd_err)" is a bit
unrelated and looks like part of previous patch
set_be_chunk(&chunk.h, NBD_REPLY_FLAG_DONE, NBD_REPLY_TYPE_ERROR, handle,
sizeof(chunk) - sizeof(chunk.h));
stl_be_p(&chunk.error, nbd_err);
-stw_be_p(&chunk.message_length, 0);
+stw_be_p(&chunk.message_length, iov[1].iov_len);
-return nbd_co_send_iov(client, iov, 1, errp);
+return nbd_co_send_iov(client, iov, 1 + !!iov[1].iov_len, errp);
but you allow messages of zero length..
it's ok, but looks a bit strange in connection with (1)
}
/* nbd_co_receive_request
@@ -1406,6 +1408,7 @@ static coroutine_fn void nbd_trip(void *opaque)
int flags;
int reply_data_len = 0;
Error *local_err = NULL;
+char *msg = NULL;
trace_nbd_trip();
if (client->closing) {
@@ -1521,14 +1524,20 @@ reply:
if (local_err) {
/* If we get here, local_err was not a fatal error, and should be sent
* to the client. */
+assert(ret < 0);
+msg = g_strdup(error_get_pretty(local_err));
error_report_err(local_err);
local_err = NULL;
}
-if (client->structured_reply && request.type == NBD_CMD_READ) {
+if (client->structured_reply &&
+(ret < 0 || request.type == NBD_CMD_READ)) {
if (ret < 0) {
+if (!msg) {
+msg = g_strdup("");
I'd prefer to handle msg=NULL in nbd_co_send_structured_error instead of
this.
+}
ret = nbd_co_send_structured_error(req->client, request.handle,
- -ret, &local_err);
+ -ret, msg, &local_err);
} else {
ret = nbd_co_send_structured_read(req->client, request.handle,
request.from, req->data,
@@ -1539,6 +1548,7 @@ reply:
ret < 0 ? -ret : 0,
req->data, reply_data_len, &local_err);
}
+g_free(msg);
if (ret < 0) {
error_prepend(&local_err, "Failed to send reply: ");
goto disconnect;
diff --git a/nbd/trace-events b/nbd/trace-events
index 15a9294445..880f211c07 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -57,7 +57,7 @@ nbd_blk_aio_attached(const char *name, void *ctx) "Export %s:
Attaching clients
nbd_blk_aio_detach(const char *name, void *ctx) "Export %s: Detaching clients from
AIO context %p\n"
nbd_co_send_simple_reply(uint64_t handle, uint32_t error, const char *errname, int len) "Send simple
reply: handle = %" PRIu64 ", error = %" PRIu32 " (%s), len = %d"
nbd_co_send_structured_read(uint64_t handle, uint64_t offset, void *data, size_t size) "Send
structured read data reply: handle = %" PRIu64 ", offset = %" PRIu64 ", data = %p, len =
%zu"
-nbd_co_send_structured_error(uint64_t handle, int err) "Send structured error reply: handle =
%" PRIu64 ", error = %d"
+nbd_co_send_structured_error(uint64_t handle, int err, const char *errname, const char *msg)
"Send structured error reply: handle = %" PRIu64 ", error = %d (%
Re: [Qemu-devel] [PATCH v4 6/8] nbd/client: refactor nbd_receive_starttls
15.10.2017 04:01, Eric Blake wrote: From: Vladimir Sementsov-Ogievskiy Split out nbd_request_simple_option to be reused for structured reply option. Signed-off-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Eric Blake ok for me. -- Best regards, Vladimir
Re: [Qemu-devel] qemu-img convert to VMDK
On 10/14/2017 10:45 AM, Fam Zheng wrote: On Fri, 10/13 15:20, Guilherme Moro wrote: Hi, I'm trying to convert some images from raw to vmdk to run in a ESXi6.5 server but I need to rectify the image before being able to run. The scenario goes like that: qemu-img convert -f raw image.raw -O vmdk -o compat6 image.vmdk ESXi complains with "Failed - File system specific implementation of LookupAndOpen"[file] failed running vmkfstools -x repair on the image make it bootable I checked the changes, vmkfstools is basically removing the second descriptor from the end of the file, I'm not familiar enough with the code and the format to know what this means, maybe compat6 should make qemu-img avoid to put that in the end of the file (again, mostly guessing). Anyway we would need an option to make sure that this second descriptor does not end in the final image. What do you mean by "second descriptor"? The one at offset 0x200? But other than this descriptor I don't see a second one. Maybe I'm missing something? Fam Hi, maybe there's something else happening on my side, I will double check, but my converted image have descriptors in the end and in the beginning of the file: linux-kv33:/data/NFS # tail -c 512 SLES15-JeOS-for-VMware.x86_64-1.3.0-Build.vmdk encoding="UTF-8" # Disk DescriptorFile version=1 CID=f7105bc0 parentCID= createType="monolithicSparse" # Extent description RW 50331648 SPARSE "SLES15-JeOS-for-VMware.x86_64-1.3.0.vmdk" # The Disk Data Base #DDB ddb.virtualHWVersion = "4" ddb.geometry.cylinders = "49932" ddb.geometry.heads = "16" ddb.geometry.sectors = "63" ddb.adapterType = "ide" ddb.toolsInstallType = "4" ddb.toolsVersion = "10282" linux-kv33:/data/NFS # head -c 1024 SLES15-JeOS-for-VMware.x86_64-1.3.0-Build.vmdk KDMV� � # Disk DescriptorFile version=1 CID=f7105bc0 parentCID= createType="monolithicSparse" # Extent description RW 50331648 SPARSE "SLES15-JeOS-for-VMware.x86_64-1.3.0.vmdk" # The Disk Data Base #DDB ddb.virtualHWVersion = "4" ddb.geometry.cylinders = "49932" ddb.geometry.heads = "16" ddb.geometry.sectors = "63" ddb.adapterType = "ide" Again, not acquainted with the code as much, but the code seems to indicate VMDK4 files having a footer. Specifically the function vmdk_open_vmdk4 inside block/vmdk.c . I couldn't trace further if there's any other code *writing* a footer. Guilherme
Re: [Qemu-devel] [PATCH v3 00/32] Migration: postcopy failure recovery
Hi, This series failed build test on s390x host. Please find the details below. Type: series Subject: [Qemu-devel] [PATCH v3 00/32] Migration: postcopy failure recovery Message-id: 20171016065216.18162-1-pet...@redhat.com === TEST SCRIPT BEGIN === #!/bin/bash # Testing script will be invoked under the git checkout with # HEAD pointing to a commit that has the patches applied on top of "base" # branch set -e echo "=== ENV ===" env echo "=== PACKAGES ===" rpm -qa echo "=== TEST BEGIN ===" CC=$HOME/bin/cc INSTALL=$PWD/install BUILD=$PWD/build echo -n "Using CC: " realpath $CC mkdir -p $BUILD $INSTALL SRC=$PWD cd $BUILD $SRC/configure --cc=$CC --prefix=$INSTALL make -j4 # XXX: we need reliable clean up # make check -j4 V=1 make install === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/20171016065216.18162-1-pet...@redhat.com -> patchew/20171016065216.18162-1-pet...@redhat.com Switched to a new branch 'test' 862beab migration, hmp: new command "migrate_pause" e1bc9dd migration, qmp: new command "migrate-pause" 26a11ab migration: delay the postcopy-active state switch f314bea migration: init dst in migration_object_init too ddf5991 migration: allow migrate_incoming for paused VM 26b00b0 migration: store listen task tag 7a0f9a1 migration: return incoming task tag for fd 7be5fe0 migration: return incoming task tag for exec 24bfe17 migration: return incoming task tag for sockets 3a6b903 migration: free SocketAddress where allocated aac8464 migration: final handshake for the resume d4ee206 migration: setup ramstate for resume 88fd506 migration: synchronize dirty bitmap for resume e88ab5b6 migration: introduce SaveVMHandlers.resume_prepare f61a07b migration: new message MIG_RP_MSG_RESUME_ACK 5e27997 migration: new cmd MIG_CMD_POSTCOPY_RESUME 1115dcc migration: new message MIG_RP_MSG_RECV_BITMAP 6f7310d migration: new cmd MIG_CMD_RECV_BITMAP f49a912 migration: wakeup dst ram-load-thread for recover d697f5e migration: new state "postcopy-recover" 713a12f migration: rebuild channel on source 97d5141 migration: pass MigrationState to migrate_init() 74a905e qmp: hmp: add migrate "resume" option 280cf7c migration: allow fault thread to pause 4ce2f13 migration: allow send_rq to fail fc9dec7 migration: allow src return path to pause 927c0b3 migration: allow dst vm pause on postcopy 13a2534 migration: implement "postcopy-pause" src logic 6ee50f4 migration: new postcopy-pause state af824fe migration: provide postcopy_fault_thread_notify() 309d9e9 migration: reuse mis->userfault_quit_fd f5b1d03 migration: better error handling with QEMUFile === OUTPUT BEGIN === === ENV === XDG_SESSION_ID=48831 SHELL=/bin/sh USER=fam PATCHEW=/home/fam/patchew/patchew-cli -s http://patchew.org --nodebug PATH=/usr/bin:/bin PWD=/var/tmp/patchew-tester-tmp-nm4b3qpv/src LANG=en_US.UTF-8 HOME=/home/fam SHLVL=2 LOGNAME=fam DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/1012/bus XDG_RUNTIME_DIR=/run/user/1012 _=/usr/bin/env === PACKAGES === gpg-pubkey-873529b8-54e386ff xz-libs-5.2.2-2.fc24.s390x libxshmfence-1.2-3.fc24.s390x giflib-4.1.6-15.fc24.s390x trousers-lib-0.3.13-6.fc24.s390x ncurses-base-6.0-6.20160709.fc25.noarch gmp-6.1.1-1.fc25.s390x libidn-1.33-1.fc25.s390x slang-2.3.0-7.fc25.s390x pkgconfig-0.29.1-1.fc25.s390x alsa-lib-1.1.1-2.fc25.s390x yum-metadata-parser-1.1.4-17.fc25.s390x python3-slip-dbus-0.6.4-4.fc25.noarch python2-cssselect-0.9.2-1.fc25.noarch createrepo_c-libs-0.10.0-6.fc25.s390x initscripts-9.69-1.fc25.s390x parted-3.2-21.fc25.s390x flex-2.6.0-3.fc25.s390x colord-libs-1.3.4-1.fc25.s390x python-osbs-client-0.33-3.fc25.noarch perl-Pod-Simple-3.35-1.fc25.noarch python2-simplejson-3.10.0-1.fc25.s390x brltty-5.4-2.fc25.s390x librados2-10.2.4-2.fc25.s390x tcp_wrappers-7.6-83.fc25.s390x libcephfs_jni1-10.2.4-2.fc25.s390x nettle-devel-3.3-1.fc25.s390x bzip2-devel-1.0.6-21.fc25.s390x libuuid-2.28.2-2.fc25.s390x python3-dnf-1.1.10-6.fc25.noarch texlive-kpathsea-doc-svn41139-33.fc25.1.noarch openssh-7.4p1-4.fc25.s390x texlive-kpathsea-bin-svn40473-33.20160520.fc25.1.s390x texlive-graphics-svn41015-33.fc25.1.noarch texlive-dvipdfmx-def-svn40328-33.fc25.1.noarch texlive-mfware-svn40768-33.fc25.1.noarch texlive-texlive-scripts-svn41433-33.fc25.1.noarch texlive-euro-svn22191.1.1-33.fc25.1.noarch texlive-etex-svn37057.0-33.fc25.1.noarch texlive-iftex-svn29654.0.2-33.fc25.1.noarch texlive-palatino-svn31835.0-33.fc25.1.noarch texlive-texlive-docindex-svn41430-33.fc25.1.noarch texlive-xunicode-svn30466.0.981-33.fc25.1.noarch texlive-koma-script-svn41508-33.fc25.1.noarch texlive-pst-grad-svn15878.1.06-33.fc25.1.noarch texlive-pst-blur-svn15878.2.0-33.fc25.1.noarch texlive-jknapltx-svn19440.0-33.fc25.1.noarch texinfo-6.1-4.fc25.s390x openssl-devel-1.0.2k-1.fc25.s390x jansson-2.10-2.fc25.s390x fedora-repos-25-4.noarch perl-Errno-1.25-387.fc25.s390x acl-2.2.52-13.fc25.s390x systemd-pam-231-17.fc25.s390x NetworkManager-libnm-1.4.4-5.fc25.s390x poppler-0
Re: [Qemu-devel] [PATCH] monitor: fix dangling CPU pointer
On Mon, 16 Oct 2017 10:41:39 +0200
Igor Mammedov wrote:
> On Fri, 13 Oct 2017 13:47:51 +0200
> Greg Kurz wrote:
>
> [...]
>
> > Note that the resolution should really return a CPU object, otherwise
> > we have a bug. This is achieved by relying on object_resolve_path()
> > and CPU() instead of calling object_resolve_path_type(path, TYPE_CPU).
> I'm not sure what you are trying to say here, could you explain
> a bit more why CPU(object_resolve_path()) is chosen vs
> object_resolve_path_type(path, TYPE_CPU)
>
IIUC, if we use object_resolve_path_type(path, TYPE_CPU) and path doesn't
point to a CPU object, it will return NULL (see object_resolve_abs_path())
just like if the CPU got hot-unplugged.
My point is that the path we got from object_get_canonical_path() did
point to a CPU: if later on this path resolves to an object that isn't
a CPU, then some code somewhere used the same QOM path for some unrelated
object. I tend to think this is a bug in QEMU and we shouldn't silently
ignore it.
Makes sense ?
>
> > Suggested-by: Igor Mammedov
> > Signed-off-by: Greg Kurz
> > ---
> > monitor.c | 25 -
> > 1 file changed, 20 insertions(+), 5 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index fe0d1bdbb461..8489b2ad99c0 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -200,7 +200,7 @@ struct Monitor {
> >
> > ReadLineState *rs;
> > MonitorQMP qmp;
> > -CPUState *mon_cpu;
> > +gchar *mon_cpu_path;
> > BlockCompletionFunc *password_completion_cb;
> > void *password_opaque;
> > mon_cmd_t *cmd_table;
> > @@ -579,6 +579,7 @@ static void monitor_data_init(Monitor *mon)
> >
> > static void monitor_data_destroy(Monitor *mon)
> > {
> > +g_free(mon->mon_cpu_path);
> > qemu_chr_fe_deinit(&mon->chr, false);
> > if (monitor_is_qmp(mon)) {
> > json_message_parser_destroy(&mon->qmp.parser);
> > @@ -1047,20 +1048,34 @@ int monitor_set_cpu(int cpu_index)
> > if (cpu == NULL) {
> > return -1;
> > }
> > -cur_mon->mon_cpu = cpu;
> > +g_free(cur_mon->mon_cpu_path);
> > +cur_mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
> > return 0;
> > }
> >
> > CPUState *mon_get_cpu(void)
> > {
> > -if (!cur_mon->mon_cpu) {
> > +CPUState *cpu;
> > +
> > +if (cur_mon->mon_cpu_path) {
> > +Object *obj = object_resolve_path(cur_mon->mon_cpu_path, NULL);
> > +
> > +if (!obj) {
> > +g_free(cur_mon->mon_cpu_path);
> > +cur_mon->mon_cpu_path = NULL;
> > +} else {
> > +cpu = CPU(obj);
> this potentially might abort if obj couldn't be cast to TYPE_CPU
>
This is deliberate... is there a case where cur_mon->mon_cpu_path would
legitimately point to something that isn't of type TYPE_CPU ?
>
> > +}
> > +}
> > +if (!cur_mon->mon_cpu_path) {
> > if (!first_cpu) {
> > return NULL;
> > }
> > monitor_set_cpu(first_cpu->cpu_index);
> > +cpu = first_cpu;
> > }
> > -cpu_synchronize_state(cur_mon->mon_cpu);
> > -return cur_mon->mon_cpu;
> > +cpu_synchronize_state(cpu);
> > +return cpu;
> > }
> >
> > CPUArchState *mon_get_cpu_env(void)
> >
> >
>
Re: [Qemu-devel] kvm_set_phys_mem: assertion failed
On 04.10.2017 17:48, Laszlo Ersek wrote: > Hi, > > On 09/21/17 16:28, Auger Eric wrote: >> Hi David, >> On 20/09/2017 16:34, David Hildenbrand wrote: >>> On 20.09.2017 16:31, Gerd Hoffmann wrote: Hi, > Dropping from os section: > > type="pflash">/usr/share/edk2.git/ovmf-x64/OVMF_CODE-pure- > efi.fd > template="/usr/share/edk2.git/ovmf-x64/OVMF_VARS-pure- > efi.fd">/var/lib/libvirt/qemu/nvram/fedora-org-drm-qxl- > base_VARS.fd Bad idea I guess. > 1) Does the assert trigger right at startup? Or how is it triggered? Yes, right at startup, before OVMF is done initializing. So probably something in OVMF triggers it. >>> >>> I'll try to include ovmf to reproduce it. >>> > 2) Does your setup work when dopping the assertion? Can try tomorrow. >> I encounter the problem on ARM too. My setup works when dropping the >> assertion. >> >> Thanks >> >> Eric >>> >>> I think that assertion might not be stable, because properties (romd >>> mode) might not be stable and can change. So if it works without the >>> assert, dropping it is the right thing to do. > > There seems to be a new issue report on LaunchPad about the same > (f357f564be0b) commit, stating that the removal of the assertion does > not help: > > https://bugs.launchpad.net/qemu/+bug/1721221 > > (Just trying to connect the dots here; I have no comments on the commit > otherwise. CC'ing Joe who has filed the LP report.) > > Thanks > Laszlo > Hi Joe, do you have some time to test with the following branch? https://github.com/davidhildenbrand/qemu/tree/kvm_slot If it works - very good. If not, can you give me what QEMU prints (I added some debug output). Looks something like this: set_phys_mem(0): start: 0x0, size: 0x800, add: 1 Add(0): start: 0x0, size: 0x800, ram: 0x0x7f6697e0 set_phys_mem(0): start: 0xfffc, size: 0x4, add: 1 Add(0): start: 0xfffc, size: 0x4, ram: 0x0x7f66a720 set_phys_mem(0): start: 0x0, size: 0x800, add: 0 ... -- Thanks, David
Re: [Qemu-devel] [PATCH v2 22/47] hw/m68k: Replace fprintf(stderr, "*\n" with error_report()
On 30.09.2017 02:16, Alistair Francis wrote:
> Replace a large number of the fprintf(stderr, "*\n" calls with
> error_report(). The functions were renamed with these commands and then
> compiler issues where manually fixed.
[...]
> @@ -83,7 +83,7 @@ static void an5206_init(MachineState *machine)
> entry = KERNEL_LOAD_ADDR;
> }
> if (kernel_size < 0) {
> -fprintf(stderr, "qemu: could not load kernel '%s'\n",
> kernel_filename);
> +error_report("qemu: could not load kernel '%s'", kernel_filename);
I just noticed: The "qemu:" prefix should also be dropped when using
error_report() now. I can change that while applying the patch.
> exit(1);
> }
>
[...]
> diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c
> index b9dde75106..2fb5037b72 100644
> --- a/hw/m68k/mcf5208.c
> +++ b/hw/m68k/mcf5208.c
> @@ -6,6 +6,7 @@
> * This code is licensed under the GPL
> */
> #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> #include "qapi/error.h"
> #include "qemu-common.h"
> #include "cpu.h"
> @@ -257,7 +258,7 @@ static void mcf5208evb_init(MachineState *machine)
> mcf5208_sys_init(address_space_mem, pic);
>
> if (nb_nics > 1) {
> -fprintf(stderr, "Too many NICs\n");
> +error_report("Too many NICs");
> exit(1);
> }
> if (nd_table[0].used) {
> @@ -292,7 +293,7 @@ static void mcf5208evb_init(MachineState *machine)
> if (qtest_enabled()) {
> return;
> }
> -fprintf(stderr, "Kernel image must be specified\n");
> +error_report("Kernel image must be specified");
> exit(1);
> }
>
> @@ -309,7 +310,7 @@ static void mcf5208evb_init(MachineState *machine)
> entry = 0x4000;
> }
> if (kernel_size < 0) {
> -fprintf(stderr, "qemu: could not load kernel '%s'\n",
> kernel_filename);
> +error_report("qemu: could not load kernel '%s'", kernel_filename);
> exit(1);
> }
dito.
Thomas
Re: [Qemu-devel] [PATCH v4 7/8] nbd/client: prepare nbd_receive_reply for structured reply
15.10.2017 04:01, Eric Blake wrote: From: Vladimir Sementsov-Ogievskiy In following patch nbd_receive_reply will be used both for simple and structured reply header receiving. NBDReply is altered into union of simple reply header and structured reply chunk header, simple error translation moved to block/nbd-client to be consistent with further structured reply error translation. Signed-off-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Eric Blake looks ok. -- Best regards, Vladimir
Re: [Qemu-devel] [PATCH v4 8/8] nbd: Move nbd_read() to common header
15.10.2017 04:01, Eric Blake wrote: An upcoming change to block/nbd-client.c will want to read the tail of a structured reply chunk directly from the wire. Move this function to make it easier. Based on a patch from Vladimir Sementsov-Ogievskiy. Signed-off-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
[Qemu-devel] [Bug 1723927] [NEW] Linux or windows guest boot failed by uefi on CPU of Intel Xeon X5675
Public bug reported: Hi, I started windows server 2012 DC or redhat7.0, but boot failed by UEFI, and start process stop on "TianoCore" image by looking at VNCviewer. VM using the command:(redhat7.0) /usr/bin/kvm -name guest=ytest,debug-threads=on -S -object secret,id=masterKey0,format=raw,file=/run/lib/libvirt/qemu/domain-40-ytest/master-key.aes -machine pc-i440fx-2.7,accel=kvm,usb=off,system=windows,dump-guest-core=off -bios /usr/share/qemu-kvm/OVMF_CODE.fd -m size=8388608k,slots=10,maxmem=34359738368k -realtime mlock=off -smp 1,maxcpus=24,sockets=24,cores=1,threads=1 -numa node,nodeid=0,cpus=0-23,mem=8192 -uuid 8cf40bd6-258a-4550-ba4e-b38230547a11 -no-user-config -nodefaults -chardev socket,id=charmonitor,path=/run/lib/libvirt/qemu/domain-40-ytest/monitor.sock,server,nowait -mon chardev=charmonitor,id=monitor,mode=control -chardev socket,id=charmonitor_cas,path=/run/lib/libvirt/qemu/domain-40-ytest/monitor.sock.cas,server,nowait -mon chardev=charmonitor_cas,id=monitor_cas,mode=control -rtc base=utc -no-shutdown -global PIIX4_PM.disable_s3=1 -global PIIX4_PM.disable_s4=1 -boot strict=on -device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 -device usb-ehci,id=usb1,bus=pci.0,addr=0x3 -device nec-usb-xhci,id=usb2,bus=pci.0,addr=0x4 -device virtio-scsi-pci,id=scsi1,bus=pci.0,addr=0x6 -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x7 -device usb-hub,id=hub0,bus=usb.0,port=1 -drive file=/vms/hw235/ytest,format=qcow2,if=none,id=drive-virtio-disk0,cache=directsync,aio=native -device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x8,pci_hotpluggable=on,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 -drive if=none,id=drive-fdc0-0-0,readonly=on -global isa-fdc.driveA=drive-fdc0-0-0 -global isa-fdc.bootindexA=2 -netdev tap,fd=48,id=hostnet0,vhost=on,vhostfd=50 -device virtio-net-pci,pci_hotpluggable=on,netdev=hostnet0,id=net0,mac=0c:da:41:1d:67:6f,bus=pci.0,addr=0x5,bootindex=4 -chardev pty,id=charserial0 -device isa-serial,chardev=charserial0,id=serial0 -chardev socket,id=charchannel0,path=/var/lib/libvirt/qemu/ytest.agent,server,nowait -device virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=org.qemu.guest_agent.0 -vnc 0.0.0.0:9 -device cirrus-vga,id=video0,bus=pci.0,addr=0x2 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x9 -msg timestamp=on qemu version: 2.7.1 edk2 version: git://git.code.sf.net/p/tianocore/edk2.git, commit: cc0b456a05f8dd1ebfb9be485465be37e96999e7 server: ProLiant BL460c G7, CPU: Intel(R) Xeon(R) CPU X5675 @ 3.07GHz Another, last version of edk2(compiled by myself) start guest is failed, too. But r15214 of edk2 start guest is ok(Download from http://sourceforge.net/projects/edk2/files/OVMF/, OVMF-X64-r15214.zip) Thanks in Advance ** Affects: qemu Importance: Undecided Status: New -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1723927 Title: Linux or windows guest boot failed by uefi on CPU of Intel Xeon X5675 Status in QEMU: New Bug description: Hi, I started windows server 2012 DC or redhat7.0, but boot failed by UEFI, and start process stop on "TianoCore" image by looking at VNCviewer. VM using the command:(redhat7.0) /usr/bin/kvm -name guest=ytest,debug-threads=on -S -object secret,id=masterKey0,format=raw,file=/run/lib/libvirt/qemu/domain-40-ytest/master-key.aes -machine pc-i440fx-2.7,accel=kvm,usb=off,system=windows,dump-guest-core=off -bios /usr/share/qemu-kvm/OVMF_CODE.fd -m size=8388608k,slots=10,maxmem=34359738368k -realtime mlock=off -smp 1,maxcpus=24,sockets=24,cores=1,threads=1 -numa node,nodeid=0,cpus=0-23,mem=8192 -uuid 8cf40bd6-258a-4550-ba4e-b38230547a11 -no-user-config -nodefaults -chardev socket,id=charmonitor,path=/run/lib/libvirt/qemu/domain-40-ytest/monitor.sock,server,nowait -mon chardev=charmonitor,id=monitor,mode=control -chardev socket,id=charmonitor_cas,path=/run/lib/libvirt/qemu/domain-40-ytest/monitor.sock.cas,server,nowait -mon chardev=charmonitor_cas,id=monitor_cas,mode=control -rtc base=utc -no-shutdown -global PIIX4_PM.disable_s3=1 -global PIIX4_PM.disable_s4=1 -boot strict=on -device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 -device usb-ehci,id=usb1,bus=pci.0,addr=0x3 -device nec-usb-xhci,id=usb2,bus=pci.0,addr=0x4 -device virtio-scsi-pci,id=scsi1,bus=pci.0,addr=0x6 -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x7 -device usb-hub,id=hub0,bus=usb.0,port=1 -drive file=/vms/hw235/ytest,format=qcow2,if=none,id=drive-virtio-disk0,cache=directsync,aio=native -device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x8,pci_hotpluggable=on,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 -drive if=none,id=drive-fdc0-0-0,readonly=on -global isa-fdc.driveA=drive-fdc0-0-0 -global isa-fdc.bootindexA=2 -netdev tap,fd=48,id=hostnet0,vhost=on,vhostfd=50 -device virtio-net-pci,pci_hotpluggable=on,netdev=hostnet0,id=net0
Re: [Qemu-devel] [PATCH v2 05/47] hw/arm: Replace fprintf(stderr, "*\n" with error_report()
On 30.09.2017 02:15, Alistair Francis wrote:
> Replace a large number of the fprintf(stderr, "*\n" calls with
> error_report(). The functions were renamed with these commands and then
> compiler issues where manually fixed.
>
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}'
> \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
>
> Some lines where then manually tweaked to pass checkpatch.
>
> Signed-off-by: Alistair Francis
> Cc: qemu-...@nongnu.org
> ---
> V2:
> - Split hw patch into individual directories
[...]
> @@ -459,8 +460,7 @@ static int load_dtb(hwaddr addr, const struct
> arm_boot_info *binfo,
> /* This is user error so deserves a friendlier error message
> * than the failure of setprop_sized_cells would provide
> */
> -fprintf(stderr, "qemu: dtb file not compatible with "
> -"RAM size > 4GB\n");
> +error_report("qemu: dtb file not compatible with RAM size > 4GB");
Please drop the "qemu:" prefix when using error_report() now
(here and also in all the other affected strings in this patch).
Thomas
Re: [Qemu-devel] [PATCH v2 10/47] hw/cris: Replace fprintf(stderr, "*\n" with error_report()
On 01.10.2017 16:13, Thomas Huth wrote:
> On 30.09.2017 02:15, Alistair Francis wrote:
>> Replace a large number of the fprintf(stderr, "*\n" calls with
>> error_report(). The functions were renamed with these commands and then
>> compiler issues where manually fixed.
> [...]
>> diff --git a/hw/cris/boot.c b/hw/cris/boot.c
>> index f896ed7f86..f199a13a82 100644
>> --- a/hw/cris/boot.c
>> +++ b/hw/cris/boot.c
>> @@ -23,6 +23,7 @@
>> */
>>
>> #include "qemu/osdep.h"
>> +#include "qemu/error-report.h"
>> #include "qemu-common.h"
>> #include "cpu.h"
>> #include "hw/hw.h"
>> @@ -86,14 +87,14 @@ void cris_load_image(CRISCPU *cpu, struct cris_load_info
>> *li)
>> }
>>
>> if (image_size < 0) {
>> -fprintf(stderr, "qemu: could not load kernel '%s'\n",
>> +error_report("qemu: could not load kernel '%s'",
>> li->image_filename);
>
> Put it on one line, please.
... and remove the "qemu:" prefix, please.
Thomas
Re: [Qemu-devel] [PATCH v2 17/47] hw/input: Replace fprintf(stderr, "*\n" with error_report()
On 30.09.2017 02:15, Alistair Francis wrote:
> Replace a large number of the fprintf(stderr, "*\n" calls with
> error_report(). The functions were renamed with these commands and then
> compiler issues where manually fixed.
[...]
> diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
> index c479f827b6..ff4f03e4c3 100644
> --- a/hw/input/pckbd.c
> +++ b/hw/input/pckbd.c
> @@ -22,6 +22,7 @@
> * THE SOFTWARE.
> */
> #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> #include "hw/hw.h"
> #include "hw/isa/isa.h"
> #include "hw/i386/pc.h"
> @@ -307,7 +308,7 @@ static void kbd_write_command(void *opaque, hwaddr addr,
> /* ignore that */
> break;
> default:
> -fprintf(stderr, "qemu: unsupported keyboard cmd=0x%02x\n", (int)val);
> +error_report("qemu: unsupported keyboard cmd=0x%02x", (int)val);
> break;
Please drop the "qemu:" prefix from the string now here, too.
Thomas
Re: [Qemu-devel] [PATCH v2 21/47] hw/lm32: Replace fprintf(stderr, "*\n" with error_report()
On 30.09.2017 02:16, Alistair Francis wrote:
> Replace a large number of the fprintf(stderr, "*\n" calls with
> error_report(). The functions were renamed with these commands and then
> compiler issues where manually fixed.
>
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}'
> \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
>
> Some lines where then manually tweaked to pass checkpatch.
>
> Signed-off-by: Alistair Francis
> Cc: Michael Walle
> ---
> V2:
> - Split hw patch into individual directories
>
> hw/lm32/lm32_boards.c | 5 +++--
> hw/lm32/milkymist.c | 5 +++--
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/hw/lm32/lm32_boards.c b/hw/lm32/lm32_boards.c
> index b0bb3ef58a..d739acf476 100644
> --- a/hw/lm32/lm32_boards.c
> +++ b/hw/lm32/lm32_boards.c
> @@ -18,6 +18,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> #include "qemu-common.h"
> #include "cpu.h"
> #include "hw/sysbus.h"
> @@ -152,7 +153,7 @@ static void lm32_evr_init(MachineState *machine)
> }
>
> if (kernel_size < 0) {
> -fprintf(stderr, "qemu: could not load kernel '%s'\n",
> +error_report("qemu: could not load kernel '%s'",
> kernel_filename);
> exit(1);
> }
> @@ -250,7 +251,7 @@ static void lm32_uclinux_init(MachineState *machine)
> }
>
> if (kernel_size < 0) {
> -fprintf(stderr, "qemu: could not load kernel '%s'\n",
> +error_report("qemu: could not load kernel '%s'",
> kernel_filename);
> exit(1);
> }
> diff --git a/hw/lm32/milkymist.c b/hw/lm32/milkymist.c
> index 4db4d2d533..caa6cf4e0f 100644
> --- a/hw/lm32/milkymist.c
> +++ b/hw/lm32/milkymist.c
> @@ -18,6 +18,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> #include "qemu-common.h"
> #include "cpu.h"
> #include "hw/sysbus.h"
> @@ -149,7 +150,7 @@ milkymist_init(MachineState *machine)
>
> /* if no kernel is given no valid bios rom is a fatal error */
> if (!kernel_filename && !dinfo && !bios_filename && !qtest_enabled()) {
> -fprintf(stderr, "qemu: could not load Milkymist One bios '%s'\n",
> +error_report("qemu: could not load Milkymist One bios '%s'",
> bios_name);
> exit(1);
> }
> @@ -188,7 +189,7 @@ milkymist_init(MachineState *machine)
> }
>
> if (kernel_size < 0) {
> -fprintf(stderr, "qemu: could not load kernel '%s'\n",
> +error_report("qemu: could not load kernel '%s'",
> kernel_filename);
> exit(1);
> }
>
Please remove the "qemu:" prefixes here, too.
Thomas
Re: [Qemu-devel] [PATCH v2 24/47] hw/mips: Replace fprintf(stderr, "*\n" with error_report()
On 30.09.2017 02:16, Alistair Francis wrote:
> Replace a large number of the fprintf(stderr, "*\n" calls with
> error_report(). The functions were renamed with these commands and then
> compiler issues where manually fixed.
>
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}'
> \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
>
> Some lines where then manually tweaked to pass checkpatch.
>
> Signed-off-by: Alistair Francis
> Cc: Paul Burton
> Cc: Aurelien Jarno
> Cc: Yongbok Kim
> Cc: "Hervé Poussineau"
> ---
> V2:
> - Split hw patch into individual directories
>
> hw/mips/boston.c| 4 ++--
> hw/mips/mips_fulong2e.c | 4 ++--
> hw/mips/mips_jazz.c | 4 ++--
> hw/mips/mips_malta.c| 4 ++--
> hw/mips/mips_mipssim.c | 4 ++--
> hw/mips/mips_r4k.c | 6 +++---
> 6 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/hw/mips/boston.c b/hw/mips/boston.c
> index 776ee283e1..ee82968ea7 100644
> --- a/hw/mips/boston.c
> +++ b/hw/mips/boston.c
> @@ -353,7 +353,7 @@ static const void *boston_fdt_filter(void *opaque, const
> void *fdt_orig,
>
> err = fdt_open_into(fdt_orig, fdt, fdt_sz);
> if (err) {
> -fprintf(stderr, "unable to open FDT\n");
> +error_report("unable to open FDT");
> return NULL;
> }
>
> @@ -361,7 +361,7 @@ static const void *boston_fdt_filter(void *opaque, const
> void *fdt_orig,
> ? machine->kernel_cmdline : " ";
> err = qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
> if (err < 0) {
> -fprintf(stderr, "couldn't set /chosen/bootargs\n");
> +error_report("couldn't set /chosen/bootargs");
> return NULL;
> }
>
> diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
> index 75318680e1..34710a466c 100644
> --- a/hw/mips/mips_fulong2e.c
> +++ b/hw/mips/mips_fulong2e.c
> @@ -143,7 +143,7 @@ static int64_t load_kernel (CPUMIPSState *env)
> initrd_offset, ram_size -
> initrd_offset);
> }
> if (initrd_size == (target_ulong) -1) {
> -fprintf(stderr, "qemu: could not load initial ram disk '%s'\n",
> +error_report("qemu: could not load initial ram disk '%s'",
> loaderparams.initrd_filename);
> exit(1);
> }
> @@ -342,7 +342,7 @@ static void mips_fulong2e_init(MachineState *machine)
>
> isa_bus = vt82c686b_init(pci_bus, PCI_DEVFN(FULONG2E_VIA_SLOT, 0));
> if (!isa_bus) {
> -fprintf(stderr, "vt82c686b_init error\n");
> +error_report("vt82c686b_init error");
> exit(1);
> }
>
> diff --git a/hw/mips/mips_jazz.c b/hw/mips/mips_jazz.c
> index 7e6626dc88..6c91a940be 100644
> --- a/hw/mips/mips_jazz.c
> +++ b/hw/mips/mips_jazz.c
> @@ -271,10 +271,10 @@ static void mips_jazz_init(MachineState *machine,
> sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(rc4030, 4));
> break;
> } else if (is_help_option(nd->model)) {
> -fprintf(stderr, "qemu: Supported NICs: dp83932\n");
> +error_report("qemu: Supported NICs: dp83932");
> exit(1);
> } else {
> -fprintf(stderr, "qemu: Unsupported NIC: %s\n", nd->model);
> +error_report("qemu: Unsupported NIC: %s", nd->model);
> exit(1);
> }
> }
> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> index 2adb9bcf89..d457adfa69 100644
> --- a/hw/mips/mips_malta.c
> +++ b/hw/mips/mips_malta.c
> @@ -856,8 +856,8 @@ static int64_t load_kernel (void)
>
Re: [Qemu-devel] [PATCH v2 26/47] hw/moxie: Replace fprintf(stderr, "*\n" with error_report()
On 30.09.2017 02:16, Alistair Francis wrote:
> Replace a large number of the fprintf(stderr, "*\n" calls with
> error_report(). The functions were renamed with these commands and then
> compiler issues where manually fixed.
>
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}'
> \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
>
> Some lines where then manually tweaked to pass checkpatch.
>
> Signed-off-by: Alistair Francis
> Cc: Anthony Green
> ---
> V2:
> - Split hw patch into individual directories
>
> hw/moxie/moxiesim.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/moxie/moxiesim.c b/hw/moxie/moxiesim.c
> index 5ea8dd3a93..5916f86c68 100644
> --- a/hw/moxie/moxiesim.c
> +++ b/hw/moxie/moxiesim.c
> @@ -25,6 +25,7 @@
> * THE SOFTWARE.
> */
> #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> #include "qapi/error.h"
> #include "qemu-common.h"
> #include "cpu.h"
> @@ -61,7 +62,7 @@ static void load_kernel(MoxieCPU *cpu, LoaderParams
> *loader_params)
> 0, 0);
>
> if (kernel_size <= 0) {
> -fprintf(stderr, "qemu: could not load kernel '%s'\n",
> +error_report("qemu: could not load kernel '%s'",
> loader_params->kernel_filename);
> exit(1);
> }
> @@ -85,8 +86,8 @@ static void load_kernel(MoxieCPU *cpu, LoaderParams
> *loader_params)
>ram_size);
> }
> if (initrd_size == (target_ulong)-1) {
> -fprintf(stderr, "qemu: could not load initial ram disk '%s'\n",
> -loader_params->initrd_filename);
> +error_report("qemu: could not load initial ram disk '%s'",
> + loader_params->initrd_filename);
> exit(1);
> }
> }
Please remove the "qemu:" prefixes from the strings here, too.
Thomas
Re: [Qemu-devel] [PATCH v2 30/47] hw/openrisc: Replace fprintf(stderr, "*\n" with error_report()
On 30.09.2017 02:16, Alistair Francis wrote:
> Replace a large number of the fprintf(stderr, "*\n" calls with
> error_report(). The functions were renamed with these commands and then
> compiler issues where manually fixed.
>
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}'
> \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
>
> Some lines where then manually tweaked to pass checkpatch.
>
> Signed-off-by: Alistair Francis
> Cc: Jia Liu
> Cc: Stafford Horne
> ---
> V2:
> - Split hw patch into individual directories
>
> hw/openrisc/openrisc_sim.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/openrisc/openrisc_sim.c b/hw/openrisc/openrisc_sim.c
> index 86bf2849c4..44a6d115dd 100644
> --- a/hw/openrisc/openrisc_sim.c
> +++ b/hw/openrisc/openrisc_sim.c
> @@ -19,6 +19,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> #include "qapi/error.h"
> #include "qemu-common.h"
> #include "cpu.h"
> @@ -87,7 +88,7 @@ static void cpu_openrisc_load_kernel(ram_addr_t ram_size,
> }
>
> if (kernel_size < 0) {
> -fprintf(stderr, "QEMU: couldn't load the kernel '%s'\n",
> +error_report("QEMU: couldn't load the kernel '%s'",
> kernel_filename);
Please remove the "QEMU:" prefix here as well.
Thomas
Re: [Qemu-devel] [PATCH v2 32/47] hw/ppc: Replace fprintf(stderr, "*\n" with error_report()
On 30.09.2017 02:16, Alistair Francis wrote:
> Replace a large number of the fprintf(stderr, "*\n" calls with
> error_report(). The functions were renamed with these commands and then
> compiler issues where manually fixed.
[...]
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index db0e49ab8f..8a5350161f 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
[...]
> @@ -967,7 +967,7 @@ void ppce500_init(MachineState *machine, PPCE500Params
> *params)
>cur_base,
>ram_size - cur_base);
> if (kernel_size < 0) {
> -fprintf(stderr, "qemu: could not load kernel '%s'\n",
> +error_report("qemu: could not load kernel '%s'",
> machine->kernel_filename);
> exit(1);
> }
> @@ -982,8 +982,8 @@ void ppce500_init(MachineState *machine, PPCE500Params
> *params)
>ram_size - initrd_base);
>
> if (initrd_size < 0) {
> -fprintf(stderr, "qemu: could not load initial ram disk '%s'\n",
> -machine->initrd_filename);
> +error_report("qemu: could not load initial ram disk '%s'",
> + machine->initrd_filename);
> exit(1);
> }
>
> @@ -1024,7 +1024,7 @@ void ppce500_init(MachineState *machine, PPCE500Params
> *params)
> kernel_size = load_uimage(filename, &bios_entry, &loadaddr, NULL,
>NULL, NULL);
> if (kernel_size < 0) {
> -fprintf(stderr, "qemu: could not load firmware '%s'\n",
> filename);
> +error_report("qemu: could not load firmware '%s'", filename);
> exit(1);
> }
> }
Please remove the "qemu:" prefix from the strings here, too.
Thomas
Re: [Qemu-devel] kvm_set_phys_mem: assertion failed
On 16/10/17 12:23, David Hildenbrand wrote: On 04.10.2017 17:48, Laszlo Ersek wrote: Hi, On 09/21/17 16:28, Auger Eric wrote: Hi David, On 20/09/2017 16:34, David Hildenbrand wrote: On 20.09.2017 16:31, Gerd Hoffmann wrote: Hi, Dropping from os section: /usr/share/edk2.git/ovmf-x64/OVMF_CODE-pure- efi.fd /var/lib/libvirt/qemu/nvram/fedora-org-drm-qxl- base_VARS.fd Bad idea I guess. 1) Does the assert trigger right at startup? Or how is it triggered? Yes, right at startup, before OVMF is done initializing. So probably something in OVMF triggers it. I'll try to include ovmf to reproduce it. 2) Does your setup work when dopping the assertion? Can try tomorrow. I encounter the problem on ARM too. My setup works when dropping the assertion. Thanks Eric I think that assertion might not be stable, because properties (romd mode) might not be stable and can change. So if it works without the assert, dropping it is the right thing to do. There seems to be a new issue report on LaunchPad about the same (f357f564be0b) commit, stating that the removal of the assertion does not help: https://bugs.launchpad.net/qemu/+bug/1721221 (Just trying to connect the dots here; I have no comments on the commit otherwise. CC'ing Joe who has filed the LP report.) Thanks Laszlo Hi Joe, do you have some time to test with the following branch? https://github.com/davidhildenbrand/qemu/tree/kvm_slot If it works - very good. If not, can you give me what QEMU prints (I added some debug output). Looks something like this: set_phys_mem(0): start: 0x0, size: 0x800, add: 1 Add(0): start: 0x0, size: 0x800, ram: 0x0x7f6697e0 set_phys_mem(0): start: 0xfffc, size: 0x4, add: 1 Add(0): start: 0xfffc, size: 0x4, ram: 0x0x7f66a720 set_phys_mem(0): start: 0x0, size: 0x800, add: 0 ... ...and apologies for top posting! :S Joe
Re: [Qemu-devel] [PATCH v2 36/47] hw/sh4: Replace fprintf(stderr, "*\n" with error_report()
On 30.09.2017 02:16, Alistair Francis wrote:
> Replace a large number of the fprintf(stderr, "*\n" calls with
> error_report(). The functions were renamed with these commands and then
> compiler issues where manually fixed.
>
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}'
> \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
>
> Some lines where then manually tweaked to pass checkpatch.
>
> Signed-off-by: Alistair Francis
> Cc: Magnus Damm
> Cc: Aurelien Jarno
> ---
> V2:
> - Split hw patch into individual directories
>
> hw/sh4/r2d.c| 9
> hw/sh4/sh7750.c | 64
> +
> 2 files changed, 38 insertions(+), 35 deletions(-)
>
> diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
> index 16b9ed2db2..a010f559e2 100644
> --- a/hw/sh4/r2d.c
> +++ b/hw/sh4/r2d.c
> @@ -24,6 +24,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> #include "qapi/error.h"
> #include "qemu-common.h"
> #include "cpu.h"
> @@ -319,8 +320,8 @@ static void r2d_init(MachineState *machine)
>SDRAM_BASE + LINUX_LOAD_OFFSET,
>INITRD_LOAD_OFFSET -
> LINUX_LOAD_OFFSET);
> if (kernel_size < 0) {
> - fprintf(stderr, "qemu: could not load kernel '%s'\n",
> kernel_filename);
> - exit(1);
> +error_report("qemu: could not load kernel '%s'",
> kernel_filename);
> +exit(1);
> }
>
> /* initialization which should be done by firmware */
> @@ -339,8 +340,8 @@ static void r2d_init(MachineState *machine)
>SDRAM_SIZE - INITRD_LOAD_OFFSET);
>
> if (initrd_size < 0) {
> - fprintf(stderr, "qemu: could not load initrd '%s'\n",
> initrd_filename);
> - exit(1);
> +error_report("qemu: could not load initrd '%s'",
> initrd_filename);
> +exit(1);
> }
Please remove the "qemu:" prefix here as well.
Thomas
Re: [Qemu-devel] [PATCH v2 33/47] hw/s390x: Replace fprintf(stderr, "*\n" with error_report()
On 30.09.2017 02:16, Alistair Francis wrote:
> Replace a large number of the fprintf(stderr, "*\n" calls with
> error_report(). The functions were renamed with these commands and then
> compiler issues where manually fixed.
>
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}'
> \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
>
> Some lines where then manually tweaked to pass checkpatch.
>
> Signed-off-by: Alistair Francis
> ---
> V2:
> - Split hw patch into individual directories
>
> hw/s390x/virtio-ccw.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 62d69aa30b..e5b3cc0005 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -468,7 +468,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> * passes us zeroes for those we don't support.
> */
> if (features.features) {
> -fprintf(stderr, "Guest bug: features[%i]=%x (expected
> 0)\n",
> +error_report("Guest bug: features[%i]=%x (expected 0)",
> features.index, features.features);
> /* XXX: do a unit check here? */
> }
Sounds like this should rather be a qemu_log_mask(LOG_GUEST_ERROR, ...)
instead?
Thomas
Re: [Qemu-devel] [PATCH 0/2] add "nopin" option in the memory-backend-file
On 16/10/2017 12:20, Xiao Guangrong wrote: > >> Qemu does not need pin NVDIMM memory for VFIO device during VFIO >> hotplug, what's more, if there is no NVDIMM hw in the test machine, >> the VFIO hotplug operation will need at least 10 minutes to pin RAM >> as the NVDIMM, this time is not accepted. So we add "nopin=on" option >> in the memory-backed-file, which can avoid to pin RAM memory for NVDIMM. > > No. > > memory-backed-file does not dedicate for nvdimm only, it can be mapped > as normal memory as well. Rather more, this is no way to stop guest to > use it as DMA. Right, so a better name for the object property could be "dma" rather than "nopin". I'll let others comment on whether MemoryBackend (not just memory-backend-file) is the right place for the option. I am also not sure whether VFIO is not the right place for the "other side" of the hook. If you add the memory region to the CPU address space and not the PCI address space, you can hide it from all PCI devices. Paolo
Re: [Qemu-devel] [PATCH 1/6] tests: Add basic migration precopy test
* Juan Quintela (quint...@redhat.com) wrote:
> Signed-off-by: Juan Quintela
1) I think I agree with the bits about needing to share this code with
postcopy; lets see how it goes.
2) I do have a bug I can reproduce in the postcopy test where it fails
under heavy load (e.g. ~10 TCG'd guests on one CPU) - not investigated
in detail yet, but watch out.
> +static void test_precopy(const char *uri)
> +{
> +QTestState *from, *to;
> +
> +test_migrate_start(&from, &to, uri);
> +
> +/* We want to pick a speed slow enough that the test completes
> + * quickly, but that it doesn't complete precopy even on a slow
> + * machine, so also set the downtime.
> + */
> +/* 100 ms */
> +migrate_set_parameter(from, "downtime-limit", "100");
> +/* 1MB/s slow*/
> +migrate_set_parameter(from, "max-bandwidth", "1");
> +
> +/* Wait for the first serial output from the source */
> +wait_for_serial("src_serial");
> +
> +migrate(from, uri);
> +
> +wait_for_migration_pass(from);
> +
> +/* 1GB/s now it should converge */
> +migrate_set_parameter(from, "max-bandwidth", "10");
why is 1GB/s enough for it to converge? I'd add another 0 and
make the downtime larger as well.
> +if (!got_stop) {
> +qtest_qmp_eventwait(from, "STOP");
> +}
> +qtest_qmp_eventwait(to, "RESUME");
> +
> +wait_for_serial("dest_serial");
> +wait_for_migration_complete(from);
> +
> +test_migrate_end(from, to);
> +}
> +
> +static void test_precopy_unix(void)
> +{
> +char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
> +
> +test_precopy(uri);
> +g_free(uri);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +char template[] = "/tmp/migration-test-XX";
> +int ret;
> +
> +g_test_init(&argc, &argv, NULL);
> +
> +tmpfs = mkdtemp(template);
> +if (!tmpfs) {
> +g_test_message("mkdtemp on path (%s): %s\n", template,
> strerror(errno));
> +}
> +g_assert(tmpfs);
We've started using g_dir_make_tmp recently; it's certianly simpler.
Dave
> +module_call_init(MODULE_INIT_QOM);
> +
> +qtest_add_func("/migration/precopy/unix", test_precopy_unix);
> +
> +ret = g_test_run();
> +
> +g_assert_cmpint(ret, ==, 0);
> +
> +ret = rmdir(tmpfs);
> +if (ret != 0) {
> +g_test_message("unable to rmdir: path (%s): %s\n",
> + tmpfs, strerror(errno));
> +}
> +
> +return ret;
> +}
> --
> 2.13.5
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PULL 0/4] vhost-user patches
On 12 October 2017 at 21:53, Marc-André Lureau wrote: > The following changes since commit a0b261db8c030813e30a39eae47359ac2a37f7e2: > > Merge remote-tracking branch > 'remotes/ehabkost/tags/python-next-pull-request' into staging (2017-10-12 > 10:02:09 +0100) > > are available in the Git repository at: > > https://github.com/elmarco/qemu.git tags/vu-pull-request > > for you to fetch changes up to 13384f158ce043744cf7542162a34953c7e05a70: > > libvhost-user: Support VHOST_USER_SET_SLAVE_REQ_FD (2017-10-12 16:57:50 > +0200) > > > > > > Dr. David Alan Gilbert (4): > libvhost-user: vu_queue_started > vhost-user-bridge: Only process received packets on started queues > libvhost-user: Update and fix feature and request lists > libvhost-user: Support VHOST_USER_SET_SLAVE_REQ_FD > > contrib/libvhost-user/libvhost-user.h | 19 +++- > contrib/libvhost-user/libvhost-user.c | 43 > ++- > tests/vhost-user-bridge.c | 1 + > 3 files changed, 56 insertions(+), 7 deletions(-) Applied, thanks. -- PMM
Re: [Qemu-devel] [PATCH 2/6] tests: Add basic migration precopy tcp test
* Juan Quintela (quint...@redhat.com) wrote:
> Signed-off-by: Juan Quintela
> ---
> tests/migration-test.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/tests/migration-test.c b/tests/migration-test.c
> index cd954caee4..8ef2b72459 100644
> --- a/tests/migration-test.c
> +++ b/tests/migration-test.c
> @@ -466,6 +466,13 @@ static void test_precopy_unix(void)
> g_free(uri);
> }
>
> +static void test_precopy_tcp(void)
> +{
> +const char *uri = "tcp:0:4";
> +
> +test_precopy(uri);
> +}
The problem is that this will fail if you make check -j and two
tests use at the same time.
That's pretty common especially when you've built multiple architectures
as targets and all the same tests run at the same time.
Dave
> int main(int argc, char **argv)
> {
> char template[] = "/tmp/migration-test-XX";
> @@ -482,6 +489,7 @@ int main(int argc, char **argv)
> module_call_init(MODULE_INIT_QOM);
>
> qtest_add_func("/migration/precopy/unix", test_precopy_unix);
> +qtest_add_func("/migration/precopy/tcp", test_precopy_tcp);
>
> ret = g_test_run();
>
> --
> 2.13.5
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH v4 2/8] nbd: Move nbd_errno_to_system_errno() to public header
On 10/16/2017 03:33 AM, Vladimir Sementsov-Ogievskiy wrote: > 15.10.2017 04:01, Eric Blake wrote: >> This is needed in preparation for structured reply handling, >> as we will be performing the translation from NBD error to >> system errno value higher in the stack at block/nbd-client.c. > > you've forget to sign-off. D'oh. Bad patch split on my part. Signed-off-by: Eric Blake > > Reviewed-by: Vladimir Sementsov-Ogievskiy > >> --- >> include/block/nbd.h | 13 + >> nbd/nbd-internal.h | 12 >> nbd/client.c | 32 >> nbd/common.c | 34 ++ >> nbd/trace-events | 4 +++- >> 5 files changed, 50 insertions(+), 45 deletions(-) >> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v1] kvm: tolerate non-existing slot for log_start and log_stop
On 16/10/2017 11:41, David Hildenbrand wrote:
> log_start might be called by memory.c just before registering the
> section. So we can actually get a log_start without a region_add, which
> we can silently ignore.
This is really a bug in memory.c, I think. When you put together
everything as a single patch series, can you include the memory.c change
instead, and drop this reference in the commit message and the kvm-all.c
comment?
Thanks,
Paolo
> This makes current KVM code trigger an assertion
> ("kvm_section_update_flags: error finding slot").
>
> Also, if we want to trap every access to a section, we might not have a
> slot. So let's just tolerate if we don't have a slot.
Re: [Qemu-devel] [PATCH v1] kvm: tolerate non-existing slot for log_start and log_stop
On 16/10/2017 11:53, David Hildenbrand wrote: > > Looks like adding these assertions was counter productive :) Or productive. Bugs happen, and if they are a good occasion to add new tests to the testsuite or at least a comment to the code, all the better. Paolo
Re: [Qemu-devel] [PATCH v4 3/8] nbd: Expose constants and structs for structured read
On 10/16/2017 03:49 AM, Vladimir Sementsov-Ogievskiy wrote: > 15.10.2017 04:01, Eric Blake wrote: >> Upcoming patches will implement the NBD structured reply >> extension [1] for both client and server roles. Declare the >> constants, structs, and lookup routines that will be valuable >> whether the server or client code is backported in isolation. >> >> This includes moving one constant from an internal header to >> the public header, as part of the structured read processing >> will be done in block/nbd-client.c rather than nbd/client.c. >> >> [1]https://github.com/NetworkBlockDevice/nbd/blob/extension-structured-reply/doc/proto.md >> > > this link may become dead in future, after merging extension spec into > master... Perhaps; but it's a git branch, so it's not going to disappear, even when it is no longer actively maintained. For comparison, here's the (now-stale) link for NBD_CMD_WRITE_ZEROES: https://github.com/NetworkBlockDevice/nbd/blob/extension-write-zeroes/doc/proto.md So I'm not too worried about having the URL in the commit message. > >> >> Based on patches from Vladimir Sementsov-Ogievskiy. >> >> Signed-off-by: Eric Blake > > Reviewed-by: Vladimir Sementsov-Ogievskiy > > > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v4 4/8] nbd: Minimal structured read for server
On 10/16/2017 04:49 AM, Vladimir Sementsov-Ogievskiy wrote:
> 15.10.2017 04:01, Eric Blake wrote:
>> From: Vladimir Sementsov-Ogievskiy
>>
>> Minimal implementation of structured read: one structured reply chunk,
>> no segmentation.
>> Minimal structured error implementation: no text message.
>> Support DF flag, but just ignore it, as there is no segmentation any
>> way.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> Signed-off-by: Eric Blake
>>
>> +
>> + case NBD_OPT_STRUCTURED_REPLY:
>> + if (client->structured_reply) {
>> + ret = nbd_negotiate_send_rep_err(
>> + client->ioc, NBD_REP_ERR_INVALID, option, errp,
>> + "structured reply already negotiated");
>
> You were going to send a patch to spec for this..
Still am; it's on my queue for tasks to do today. :)
>
>> + } else {
>> + ret = nbd_negotiate_send_rep(client->ioc,
>> NBD_REP_ACK,
>> + option, errp);
>> + }
>> + if (ret < 0) {
>> + return ret;
>> + }
>> + client->structured_reply = true;
>> + myflags |= NBD_CMD_FLAG_DF;
>
> it should be NBD_FLAG_SEND_DF
Ouch, you're right. NBD_CMD_FLAG_DF has the same value as
NBD_FLAG_SEND_FLUSH (which we always advertise). If nothing else, you
proved that my weekend hacking was shoved out the door quickly, rather
than fully tested. :)
>
> [...]
>
> the following looks ok.
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
