From acbcd06e5d2c181afd87b413e92a92f4594ec084 Mon Sep 17 00:00:00 2001 From: "Dr. David Alan Gilbert" Date: Tue, 11 Jan 2022 10:19:34 +0000 Subject: [PATCH 01/18] clock-vmstate: Add missing END_OF_LIST MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add the missing VMSTATE_END_OF_LIST to vmstate_muldiv Fixes: 99abcbc7600 ("clock: Provide builtin multiplier/divider") Signed-off-by: Dr. David Alan Gilbert Message-Id: <20220111101934.115028-1-dgilbert@redhat.com> Reviewed-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Alex Bennée Reviewed-by: Luc Michel Cc: qemu-stable@nongnu.org Signed-off-by: Dr. David Alan Gilbert --- hw/core/clock-vmstate.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/core/clock-vmstate.c b/hw/core/clock-vmstate.c index 9d9174ffbd..7eccb6d4ea 100644 --- a/hw/core/clock-vmstate.c +++ b/hw/core/clock-vmstate.c @@ -44,6 +44,7 @@ const VMStateDescription vmstate_muldiv = { .fields = (VMStateField[]) { VMSTATE_UINT32(multiplier, Clock), VMSTATE_UINT32(divider, Clock), + VMSTATE_END_OF_LIST() }, }; From 4ce7a08d3e3a869f89c2af622fa77e967ade7cbf Mon Sep 17 00:00:00 2001 From: Hanna Reitz Date: Wed, 23 Feb 2022 10:23:40 +0100 Subject: [PATCH 02/18] virtiofsd: Let meson check for statx.stx_mnt_id In virtiofsd, we assume that the presence of the STATX_MNT_ID macro implies existence of the statx.stx_mnt_id field. Unfortunately, that is not necessarily the case: glibc has introduced the macro in its commit 88a2cf6c4bab6e94a65e9c0db8813709372e9180, but the statx.stx_mnt_id field is still missing from its own headers. Let meson.build actually chek for both STATX_MNT_ID and statx.stx_mnt_id, and set CONFIG_STATX_MNT_ID if both are present. Then, use this config macro in virtiofsd. Closes: https://gitlab.com/qemu-project/qemu/-/issues/882 Signed-off-by: Hanna Reitz Message-Id: <20220223092340.9043-1-hreitz@redhat.com> Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Dr. David Alan Gilbert --- meson.build | 13 +++++++++++++ tools/virtiofsd/passthrough_ll.c | 2 +- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/meson.build b/meson.build index 8df40bfac4..a5b63e62cd 100644 --- a/meson.build +++ b/meson.build @@ -1306,6 +1306,18 @@ statx_test = gnu_source_prefix + ''' has_statx = cc.links(statx_test) +# Check whether statx() provides mount ID information + +statx_mnt_id_test = gnu_source_prefix + ''' + #include + int main(void) { + struct statx statxbuf; + statx(0, "", 0, STATX_BASIC_STATS | STATX_MNT_ID, &statxbuf); + return statxbuf.stx_mnt_id; + }''' + +has_statx_mnt_id = cc.links(statx_mnt_id_test) + have_vhost_user_blk_server = get_option('vhost_user_blk_server') \ .require(targetos == 'linux', error_message: 'vhost_user_blk_server requires linux') \ @@ -1553,6 +1565,7 @@ config_host_data.set('CONFIG_NETTLE', nettle.found()) config_host_data.set('CONFIG_QEMU_PRIVATE_XTS', xts == 'private') config_host_data.set('CONFIG_MALLOC_TRIM', has_malloc_trim) config_host_data.set('CONFIG_STATX', has_statx) +config_host_data.set('CONFIG_STATX_MNT_ID', has_statx_mnt_id) config_host_data.set('CONFIG_ZSTD', zstd.found()) config_host_data.set('CONFIG_FUSE', fuse.found()) config_host_data.set('CONFIG_FUSE_LSEEK', fuse_lseek.found()) diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c index dfa2fc250d..028dacdd8f 100644 --- a/tools/virtiofsd/passthrough_ll.c +++ b/tools/virtiofsd/passthrough_ll.c @@ -1039,7 +1039,7 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname, { int res; -#if defined(CONFIG_STATX) && defined(STATX_MNT_ID) +#if defined(CONFIG_STATX) && defined(CONFIG_STATX_MNT_ID) if (lo->use_statx) { struct statx statxbuf; From 26fcd766179e8a27c4c6c702201bdb0fe60eb091 Mon Sep 17 00:00:00 2001 From: Stefan Reiter Date: Fri, 25 Feb 2022 09:49:47 +0100 Subject: [PATCH 03/18] monitor/hmp: add support for flag argument with value Adds support for the "-xs" parameter type, where "-x" denotes a flag name and the "s" suffix indicates that this flag is supposed to take an arbitrary string parameter. These parameters are always optional, the entry in the qdict will be omitted if the flag is not given. Reviewed-by: Dr. David Alan Gilbert Reviewed-by: Eric Blake Signed-off-by: Stefan Reiter [FE: fixed typo pointed out by Eric Blake use s instead of V to indicate string parameter] Signed-off-by: Fabian Ebner Message-Id: <20220225084949.35746-2-f.ebner@proxmox.com> Signed-off-by: Dr. David Alan Gilbert --- monitor/hmp.c | 19 ++++++++++++++++++- monitor/monitor-internal.h | 3 ++- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/monitor/hmp.c b/monitor/hmp.c index b20737e63c..569066036d 100644 --- a/monitor/hmp.c +++ b/monitor/hmp.c @@ -981,6 +981,7 @@ static QDict *monitor_parse_arguments(Monitor *mon, { const char *tmp = p; int skip_key = 0; + int ret; /* option */ c = *typestr++; @@ -1003,11 +1004,27 @@ static QDict *monitor_parse_arguments(Monitor *mon, } if (skip_key) { p = tmp; + } else if (*typestr == 's') { + /* has option with string value */ + typestr++; + tmp = p++; + while (qemu_isspace(*p)) { + p++; + } + ret = get_str(buf, sizeof(buf), &p); + if (ret < 0) { + monitor_printf(mon, "%s: value expected for -%c\n", + cmd->name, *tmp); + goto fail; + } + qdict_put_str(qdict, key, buf); } else { - /* has option */ + /* has boolean option */ p++; qdict_put_bool(qdict, key, true); } + } else if (*typestr == 's') { + typestr++; } } break; diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h index 3da3f86c6a..caa2e90ef2 100644 --- a/monitor/monitor-internal.h +++ b/monitor/monitor-internal.h @@ -63,7 +63,8 @@ * '.' other form of optional type (for 'i' and 'l') * 'b' boolean * user mode accepts "on" or "off" - * '-' optional parameter (eg. '-f') + * '-' optional parameter (eg. '-f'); if followed by a 's', it + * specifies an optional string param (e.g. '-fs' allows '-f foo') * */ From 7277db9103bc82a79cdd5db633560d2638fbfc33 Mon Sep 17 00:00:00 2001 From: Stefan Reiter Date: Fri, 25 Feb 2022 09:49:48 +0100 Subject: [PATCH 04/18] qapi/monitor: refactor set/expire_password with enums 'protocol' and 'connected' are better suited as enums than as strings, make use of that. No functional change intended. Suggested-by: Markus Armbruster Reviewed-by: Markus Armbruster Signed-off-by: Stefan Reiter [FE: update "Since: " from 6.2 to 7.0 put 'keep' first in enum to ease use as a default] Signed-off-by: Fabian Ebner Message-Id: <20220225084949.35746-3-f.ebner@proxmox.com> Signed-off-by: Dr. David Alan Gilbert --- monitor/hmp-cmds.c | 29 +++++++++++++++++++++++++++-- monitor/qmp-cmds.c | 37 ++++++++++++------------------------- qapi/ui.json | 36 ++++++++++++++++++++++++++++++++++-- 3 files changed, 73 insertions(+), 29 deletions(-) diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index 8c384dc1b2..ff78741b75 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -1398,8 +1398,24 @@ void hmp_set_password(Monitor *mon, const QDict *qdict) const char *password = qdict_get_str(qdict, "password"); const char *connected = qdict_get_try_str(qdict, "connected"); Error *err = NULL; + DisplayProtocol proto; + SetPasswordAction conn; - qmp_set_password(protocol, password, !!connected, connected, &err); + proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol, + DISPLAY_PROTOCOL_VNC, &err); + if (err) { + goto out; + } + + conn = qapi_enum_parse(&SetPasswordAction_lookup, connected, + SET_PASSWORD_ACTION_KEEP, &err); + if (err) { + goto out; + } + + qmp_set_password(proto, password, !!connected, conn, &err); + +out: hmp_handle_error(mon, err); } @@ -1408,8 +1424,17 @@ void hmp_expire_password(Monitor *mon, const QDict *qdict) const char *protocol = qdict_get_str(qdict, "protocol"); const char *whenstr = qdict_get_str(qdict, "time"); Error *err = NULL; + DisplayProtocol proto; - qmp_expire_password(protocol, whenstr, &err); + proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol, + DISPLAY_PROTOCOL_VNC, &err); + if (err) { + goto out; + } + + qmp_expire_password(proto, whenstr, &err); + +out: hmp_handle_error(mon, err); } diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c index db4d186448..b6e8b57fcc 100644 --- a/monitor/qmp-cmds.c +++ b/monitor/qmp-cmds.c @@ -168,33 +168,27 @@ void qmp_system_wakeup(Error **errp) qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, errp); } -void qmp_set_password(const char *protocol, const char *password, - bool has_connected, const char *connected, Error **errp) +void qmp_set_password(DisplayProtocol protocol, const char *password, + bool has_connected, SetPasswordAction connected, + Error **errp) { int disconnect_if_connected = 0; int fail_if_connected = 0; int rc; if (has_connected) { - if (strcmp(connected, "fail") == 0) { - fail_if_connected = 1; - } else if (strcmp(connected, "disconnect") == 0) { - disconnect_if_connected = 1; - } else if (strcmp(connected, "keep") == 0) { - /* nothing */ - } else { - error_setg(errp, QERR_INVALID_PARAMETER, "connected"); - return; - } + fail_if_connected = connected == SET_PASSWORD_ACTION_FAIL; + disconnect_if_connected = connected == SET_PASSWORD_ACTION_DISCONNECT; } - if (strcmp(protocol, "spice") == 0) { + if (protocol == DISPLAY_PROTOCOL_SPICE) { if (!qemu_using_spice(errp)) { return; } rc = qemu_spice.set_passwd(password, fail_if_connected, disconnect_if_connected); - } else if (strcmp(protocol, "vnc") == 0) { + } else { + assert(protocol == DISPLAY_PROTOCOL_VNC); if (fail_if_connected || disconnect_if_connected) { /* vnc supports "connected=keep" only */ error_setg(errp, QERR_INVALID_PARAMETER, "connected"); @@ -203,10 +197,6 @@ void qmp_set_password(const char *protocol, const char *password, /* Note that setting an empty password will not disable login through * this interface. */ rc = vnc_display_password(NULL, password); - } else { - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "protocol", - "'vnc' or 'spice'"); - return; } if (rc != 0) { @@ -214,7 +204,7 @@ void qmp_set_password(const char *protocol, const char *password, } } -void qmp_expire_password(const char *protocol, const char *whenstr, +void qmp_expire_password(DisplayProtocol protocol, const char *whenstr, Error **errp) { time_t when; @@ -230,17 +220,14 @@ void qmp_expire_password(const char *protocol, const char *whenstr, when = strtoull(whenstr, NULL, 10); } - if (strcmp(protocol, "spice") == 0) { + if (protocol == DISPLAY_PROTOCOL_SPICE) { if (!qemu_using_spice(errp)) { return; } rc = qemu_spice.set_pw_expire(when); - } else if (strcmp(protocol, "vnc") == 0) { - rc = vnc_display_pw_expire(NULL, when); } else { - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "protocol", - "'vnc' or 'spice'"); - return; + assert(protocol == DISPLAY_PROTOCOL_VNC); + rc = vnc_display_pw_expire(NULL, when); } if (rc != 0) { diff --git a/qapi/ui.json b/qapi/ui.json index 9354f4c467..e112409211 100644 --- a/qapi/ui.json +++ b/qapi/ui.json @@ -9,6 +9,34 @@ { 'include': 'common.json' } { 'include': 'sockets.json' } +## +# @DisplayProtocol: +# +# Display protocols which support changing password options. +# +# Since: 7.0 +# +## +{ 'enum': 'DisplayProtocol', + 'data': [ 'vnc', 'spice' ] } + +## +# @SetPasswordAction: +# +# An action to take on changing a password on a connection with active clients. +# +# @keep: maintain existing clients +# +# @fail: fail the command if clients are connected +# +# @disconnect: disconnect existing clients +# +# Since: 7.0 +# +## +{ 'enum': 'SetPasswordAction', + 'data': [ 'keep', 'fail', 'disconnect' ] } + ## # @set_password: # @@ -38,7 +66,9 @@ # ## { 'command': 'set_password', - 'data': {'protocol': 'str', 'password': 'str', '*connected': 'str'} } + 'data': { 'protocol': 'DisplayProtocol', + 'password': 'str', + '*connected': 'SetPasswordAction' } } ## # @expire_password: @@ -71,7 +101,9 @@ # <- { "return": {} } # ## -{ 'command': 'expire_password', 'data': {'protocol': 'str', 'time': 'str'} } +{ 'command': 'expire_password', + 'data': { 'protocol': 'DisplayProtocol', + 'time': 'str' } } ## # @screendump: From 675fd3c96b93abd50a3856089d832c0666dfab52 Mon Sep 17 00:00:00 2001 From: Stefan Reiter Date: Fri, 25 Feb 2022 09:49:49 +0100 Subject: [PATCH 05/18] qapi/monitor: allow VNC display id in set/expire_password MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It is possible to specify more than one VNC server on the command line, either with an explicit ID or the auto-generated ones à la "default", "vnc2", "vnc3", ... It is not possible to change the password on one of these extra VNC displays though. Fix this by adding a "display" parameter to the "set_password" and "expire_password" QMP and HMP commands. For HMP, the display is specified using the "-d" value flag. For QMP, the schema is updated to explicitly express the supported variants of the commands with protocol-discriminated unions. Signed-off-by: Stefan Reiter [FE: update "Since: " from 6.2 to 7.0 make @connected a common member of @SetPasswordOptions] Signed-off-by: Fabian Ebner Message-Id: <20220225084949.35746-4-f.ebner@proxmox.com> Acked-by: Markus Armbruster Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Dr. David Alan Gilbert --- hmp-commands.hx | 24 ++++++------ monitor/hmp-cmds.c | 40 +++++++++++++------ monitor/qmp-cmds.c | 34 +++++++--------- qapi/ui.json | 96 +++++++++++++++++++++++++++++++++++----------- 4 files changed, 129 insertions(+), 65 deletions(-) diff --git a/hmp-commands.hx b/hmp-commands.hx index 70a9136ac2..8476277aa9 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -1514,33 +1514,35 @@ ERST { .name = "set_password", - .args_type = "protocol:s,password:s,connected:s?", - .params = "protocol password action-if-connected", + .args_type = "protocol:s,password:s,display:-ds,connected:s?", + .params = "protocol password [-d display] [action-if-connected]", .help = "set spice/vnc password", .cmd = hmp_set_password, }, SRST -``set_password [ vnc | spice ] password [ action-if-connected ]`` - Change spice/vnc password. *action-if-connected* specifies what - should happen in case a connection is established: *fail* makes the - password change fail. *disconnect* changes the password and +``set_password [ vnc | spice ] password [ -d display ] [ action-if-connected ]`` + Change spice/vnc password. *display* can be used with 'vnc' to specify + which display to set the password on. *action-if-connected* specifies + what should happen in case a connection is established: *fail* makes + the password change fail. *disconnect* changes the password and disconnects the client. *keep* changes the password and keeps the connection up. *keep* is the default. ERST { .name = "expire_password", - .args_type = "protocol:s,time:s", - .params = "protocol time", + .args_type = "protocol:s,time:s,display:-ds", + .params = "protocol time [-d display]", .help = "set spice/vnc password expire-time", .cmd = hmp_expire_password, }, SRST -``expire_password [ vnc | spice ]`` *expire-time* - Specify when a password for spice/vnc becomes - invalid. *expire-time* accepts: +``expire_password [ vnc | spice ] expire-time [ -d display ]`` + Specify when a password for spice/vnc becomes invalid. + *display* behaves the same as in ``set_password``. + *expire-time* accepts: ``now`` Invalidate password instantly. diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index ff78741b75..634968498b 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -1396,24 +1396,33 @@ void hmp_set_password(Monitor *mon, const QDict *qdict) { const char *protocol = qdict_get_str(qdict, "protocol"); const char *password = qdict_get_str(qdict, "password"); + const char *display = qdict_get_try_str(qdict, "display"); const char *connected = qdict_get_try_str(qdict, "connected"); Error *err = NULL; - DisplayProtocol proto; - SetPasswordAction conn; - proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol, - DISPLAY_PROTOCOL_VNC, &err); + SetPasswordOptions opts = { + .password = (char *)password, + .has_connected = !!connected, + }; + + opts.connected = qapi_enum_parse(&SetPasswordAction_lookup, connected, + SET_PASSWORD_ACTION_KEEP, &err); if (err) { goto out; } - conn = qapi_enum_parse(&SetPasswordAction_lookup, connected, - SET_PASSWORD_ACTION_KEEP, &err); + opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol, + DISPLAY_PROTOCOL_VNC, &err); if (err) { goto out; } - qmp_set_password(proto, password, !!connected, conn, &err); + if (opts.protocol == DISPLAY_PROTOCOL_VNC) { + opts.u.vnc.has_display = !!display; + opts.u.vnc.display = (char *)display; + } + + qmp_set_password(&opts, &err); out: hmp_handle_error(mon, err); @@ -1423,16 +1432,25 @@ void hmp_expire_password(Monitor *mon, const QDict *qdict) { const char *protocol = qdict_get_str(qdict, "protocol"); const char *whenstr = qdict_get_str(qdict, "time"); + const char *display = qdict_get_try_str(qdict, "display"); Error *err = NULL; - DisplayProtocol proto; - proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol, - DISPLAY_PROTOCOL_VNC, &err); + ExpirePasswordOptions opts = { + .time = (char *)whenstr, + }; + + opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol, + DISPLAY_PROTOCOL_VNC, &err); if (err) { goto out; } - qmp_expire_password(proto, whenstr, &err); + if (opts.protocol == DISPLAY_PROTOCOL_VNC) { + opts.u.vnc.has_display = !!display; + opts.u.vnc.display = (char *)display; + } + + qmp_expire_password(&opts, &err); out: hmp_handle_error(mon, err); diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c index b6e8b57fcc..df97582dd4 100644 --- a/monitor/qmp-cmds.c +++ b/monitor/qmp-cmds.c @@ -168,35 +168,27 @@ void qmp_system_wakeup(Error **errp) qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, errp); } -void qmp_set_password(DisplayProtocol protocol, const char *password, - bool has_connected, SetPasswordAction connected, - Error **errp) +void qmp_set_password(SetPasswordOptions *opts, Error **errp) { - int disconnect_if_connected = 0; - int fail_if_connected = 0; int rc; - if (has_connected) { - fail_if_connected = connected == SET_PASSWORD_ACTION_FAIL; - disconnect_if_connected = connected == SET_PASSWORD_ACTION_DISCONNECT; - } - - if (protocol == DISPLAY_PROTOCOL_SPICE) { + if (opts->protocol == DISPLAY_PROTOCOL_SPICE) { if (!qemu_using_spice(errp)) { return; } - rc = qemu_spice.set_passwd(password, fail_if_connected, - disconnect_if_connected); + rc = qemu_spice.set_passwd(opts->password, + opts->connected == SET_PASSWORD_ACTION_FAIL, + opts->connected == SET_PASSWORD_ACTION_DISCONNECT); } else { - assert(protocol == DISPLAY_PROTOCOL_VNC); - if (fail_if_connected || disconnect_if_connected) { + assert(opts->protocol == DISPLAY_PROTOCOL_VNC); + if (opts->connected != SET_PASSWORD_ACTION_KEEP) { /* vnc supports "connected=keep" only */ error_setg(errp, QERR_INVALID_PARAMETER, "connected"); return; } /* Note that setting an empty password will not disable login through * this interface. */ - rc = vnc_display_password(NULL, password); + rc = vnc_display_password(opts->u.vnc.display, opts->password); } if (rc != 0) { @@ -204,11 +196,11 @@ void qmp_set_password(DisplayProtocol protocol, const char *password, } } -void qmp_expire_password(DisplayProtocol protocol, const char *whenstr, - Error **errp) +void qmp_expire_password(ExpirePasswordOptions *opts, Error **errp) { time_t when; int rc; + const char *whenstr = opts->time; if (strcmp(whenstr, "now") == 0) { when = 0; @@ -220,14 +212,14 @@ void qmp_expire_password(DisplayProtocol protocol, const char *whenstr, when = strtoull(whenstr, NULL, 10); } - if (protocol == DISPLAY_PROTOCOL_SPICE) { + if (opts->protocol == DISPLAY_PROTOCOL_SPICE) { if (!qemu_using_spice(errp)) { return; } rc = qemu_spice.set_pw_expire(when); } else { - assert(protocol == DISPLAY_PROTOCOL_VNC); - rc = vnc_display_pw_expire(NULL, when); + assert(opts->protocol == DISPLAY_PROTOCOL_VNC); + rc = vnc_display_pw_expire(opts->u.vnc.display, when); } if (rc != 0) { diff --git a/qapi/ui.json b/qapi/ui.json index e112409211..4a13f883a3 100644 --- a/qapi/ui.json +++ b/qapi/ui.json @@ -38,20 +38,47 @@ 'data': [ 'keep', 'fail', 'disconnect' ] } ## -# @set_password: +# @SetPasswordOptions: # -# Sets the password of a remote display session. +# Options for set_password. # # @protocol: - 'vnc' to modify the VNC server password # - 'spice' to modify the Spice server password # # @password: the new password # -# @connected: how to handle existing clients when changing the -# password. If nothing is specified, defaults to 'keep' -# 'fail' to fail the command if clients are connected -# 'disconnect' to disconnect existing clients -# 'keep' to maintain existing clients +# @connected: How to handle existing clients when changing the +# password. If nothing is specified, defaults to 'keep'. +# For VNC, only 'keep' is currently implemented. +# +# Since: 7.0 +# +## +{ 'union': 'SetPasswordOptions', + 'base': { 'protocol': 'DisplayProtocol', + 'password': 'str', + '*connected': 'SetPasswordAction' }, + 'discriminator': 'protocol', + 'data': { 'vnc': 'SetPasswordOptionsVnc' } } + +## +# @SetPasswordOptionsVnc: +# +# Options for set_password specific to the VNC procotol. +# +# @display: The id of the display where the password should be changed. +# Defaults to the first. +# +# Since: 7.0 +# +## +{ 'struct': 'SetPasswordOptionsVnc', + 'data': { '*display': 'str' } } + +## +# @set_password: +# +# Set the password of a remote display server. # # Returns: - Nothing on success # - If Spice is not enabled, DeviceNotFound @@ -65,17 +92,15 @@ # <- { "return": {} } # ## -{ 'command': 'set_password', - 'data': { 'protocol': 'DisplayProtocol', - 'password': 'str', - '*connected': 'SetPasswordAction' } } +{ 'command': 'set_password', 'boxed': true, 'data': 'SetPasswordOptions' } ## -# @expire_password: +# @ExpirePasswordOptions: # -# Expire the password of a remote display server. +# General options for expire_password. # -# @protocol: the name of the remote display protocol 'vnc' or 'spice' +# @protocol: - 'vnc' to modify the VNC server expiration +# - 'spice' to modify the Spice server expiration # # @time: when to expire the password. # @@ -84,16 +109,45 @@ # - '+INT' where INT is the number of seconds from now (integer) # - 'INT' where INT is the absolute time in seconds # -# Returns: - Nothing on success -# - If @protocol is 'spice' and Spice is not active, DeviceNotFound -# -# Since: 0.14 -# # Notes: Time is relative to the server and currently there is no way to # coordinate server time with client time. It is not recommended to # use the absolute time version of the @time parameter unless you're # sure you are on the same machine as the QEMU instance. # +# Since: 7.0 +# +## +{ 'union': 'ExpirePasswordOptions', + 'base': { 'protocol': 'DisplayProtocol', + 'time': 'str' }, + 'discriminator': 'protocol', + 'data': { 'vnc': 'ExpirePasswordOptionsVnc' } } + +## +# @ExpirePasswordOptionsVnc: +# +# Options for expire_password specific to the VNC procotol. +# +# @display: The id of the display where the expiration should be changed. +# Defaults to the first. +# +# Since: 7.0 +# +## + +{ 'struct': 'ExpirePasswordOptionsVnc', + 'data': { '*display': 'str' } } + +## +# @expire_password: +# +# Expire the password of a remote display server. +# +# Returns: - Nothing on success +# - If @protocol is 'spice' and Spice is not active, DeviceNotFound +# +# Since: 0.14 +# # Example: # # -> { "execute": "expire_password", "arguments": { "protocol": "vnc", @@ -101,9 +155,7 @@ # <- { "return": {} } # ## -{ 'command': 'expire_password', - 'data': { 'protocol': 'DisplayProtocol', - 'time': 'str' } } +{ 'command': 'expire_password', 'boxed': true, 'data': 'ExpirePasswordOptions' } ## # @screendump: From f736e414ee16d7a515e755bf7f4a022b5851844e Mon Sep 17 00:00:00 2001 From: Jack Wang Date: Tue, 8 Feb 2022 09:56:40 +0100 Subject: [PATCH 06/18] migration/rdma: set the REUSEADDR option for destination We hit following error during testing RDMA transport: in case of migration error, mgmt daemon pick one migration port, incoming rdma:[::]:8089: RDMA ERROR: Error: could not rdma_bind_addr Then try another -incoming rdma:[::]:8103, sometime it worked, sometimes need another try with other ports number. Set the REUSEADDR option for destination, This allow address could be reused to avoid rdma_bind_addr error out. Signed-off-by: Jack Wang Message-Id: <20220208085640.19702-1-jinpu.wang@ionos.com> Reviewed-by: Dr. David Alan Gilbert Reviewed-by: Pankaj Gupta Signed-off-by: Dr. David Alan Gilbert dgilbert: Fixed up some tabs --- migration/rdma.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/migration/rdma.c b/migration/rdma.c index c7c7a38487..ef1e65ec36 100644 --- a/migration/rdma.c +++ b/migration/rdma.c @@ -2705,6 +2705,7 @@ static int qemu_rdma_dest_init(RDMAContext *rdma, Error **errp) char ip[40] = "unknown"; struct rdma_addrinfo *res, *e; char port_str[16]; + int reuse = 1; for (idx = 0; idx < RDMA_WRID_MAX; idx++) { rdma->wr_data[idx].control_len = 0; @@ -2740,6 +2741,12 @@ static int qemu_rdma_dest_init(RDMAContext *rdma, Error **errp) goto err_dest_init_bind_addr; } + ret = rdma_set_option(listen_id, RDMA_OPTION_ID, RDMA_OPTION_ID_REUSEADDR, + &reuse, sizeof reuse); + if (ret) { + ERROR(errp, "Error: could not set REUSEADDR option"); + goto err_dest_init_bind_addr; + } for (e = res; e != NULL; e = e->ai_next) { inet_ntop(e->ai_family, &((struct sockaddr_in *) e->ai_dst_addr)->sin_addr, ip, sizeof ip); From a7060ba3ccc0ec6b8674988b46fc131d2f299458 Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Tue, 1 Mar 2022 16:39:01 +0800 Subject: [PATCH 07/18] migration: Dump sub-cmd name in loadvm_process_command tp It'll be easier to read the name rather than index of sub-cmd when debugging. Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Peter Xu Message-Id: <20220301083925.33483-2-peterx@redhat.com> Signed-off-by: Dr. David Alan Gilbert --- migration/savevm.c | 3 ++- migration/trace-events | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/migration/savevm.c b/migration/savevm.c index 1599b02fbc..7bb65e1d61 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -2273,12 +2273,13 @@ static int loadvm_process_command(QEMUFile *f) return qemu_file_get_error(f); } - trace_loadvm_process_command(cmd, len); if (cmd >= MIG_CMD_MAX || cmd == MIG_CMD_INVALID) { error_report("MIG_CMD 0x%x unknown (len 0x%x)", cmd, len); return -EINVAL; } + trace_loadvm_process_command(mig_cmd_args[cmd].name, len); + if (mig_cmd_args[cmd].len != -1 && mig_cmd_args[cmd].len != len) { error_report("%s received with bad length - expecting %zu, got %d", mig_cmd_args[cmd].name, diff --git a/migration/trace-events b/migration/trace-events index 48aa7b10ee..123cfe79d7 100644 --- a/migration/trace-events +++ b/migration/trace-events @@ -22,7 +22,7 @@ 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" -loadvm_process_command(uint16_t com, uint16_t len) "com=0x%x len=%d" +loadvm_process_command(const char *s, uint16_t len) "com=%s len=%d" loadvm_process_command_ping(uint32_t val) "0x%x" postcopy_ram_listen_thread_exit(void) "" postcopy_ram_listen_thread_start(void) "" From c84f976e913187eecdd9db5e45cae2ea6dd1a82b Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Tue, 1 Mar 2022 16:39:02 +0800 Subject: [PATCH 08/18] migration: Finer grained tracepoints for POSTCOPY_LISTEN The enablement of postcopy listening has a few steps, add a few tracepoints to be there ready for some basic measurements for them. Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Peter Xu Message-Id: <20220301083925.33483-3-peterx@redhat.com> Signed-off-by: Dr. David Alan Gilbert --- migration/savevm.c | 9 ++++++++- migration/trace-events | 2 +- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/migration/savevm.c b/migration/savevm.c index 7bb65e1d61..190cc5fc42 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -1948,9 +1948,10 @@ static void *postcopy_ram_listen_thread(void *opaque) static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis) { PostcopyState ps = postcopy_state_set(POSTCOPY_INCOMING_LISTENING); - trace_loadvm_postcopy_handle_listen(); Error *local_err = NULL; + trace_loadvm_postcopy_handle_listen("enter"); + if (ps != POSTCOPY_INCOMING_ADVISE && ps != POSTCOPY_INCOMING_DISCARD) { error_report("CMD_POSTCOPY_LISTEN in wrong postcopy state (%d)", ps); return -1; @@ -1965,6 +1966,8 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis) } } + trace_loadvm_postcopy_handle_listen("after discard"); + /* * Sensitise RAM - can now generate requests for blocks that don't exist * However, at this point the CPU shouldn't be running, and the IO @@ -1977,6 +1980,8 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis) } } + trace_loadvm_postcopy_handle_listen("after uffd"); + if (postcopy_notify(POSTCOPY_NOTIFY_INBOUND_LISTEN, &local_err)) { error_report_err(local_err); return -1; @@ -1991,6 +1996,8 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis) qemu_sem_wait(&mis->listen_thread_sem); qemu_sem_destroy(&mis->listen_thread_sem); + trace_loadvm_postcopy_handle_listen("return"); + return 0; } diff --git a/migration/trace-events b/migration/trace-events index 123cfe79d7..92596c00d8 100644 --- a/migration/trace-events +++ b/migration/trace-events @@ -14,7 +14,7 @@ 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_listen(const char *str) "%s" loadvm_postcopy_handle_run(void) "" loadvm_postcopy_handle_run_cpu_sync(void) "" loadvm_postcopy_handle_run_vmstart(void) "" From b9a040b93536b6b89a569b577ff22317c0287879 Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Tue, 1 Mar 2022 16:39:03 +0800 Subject: [PATCH 09/18] migration: Tracepoint change in postcopy-run bottom half Remove the old two tracepoints and they're even near each other: trace_loadvm_postcopy_handle_run_cpu_sync() trace_loadvm_postcopy_handle_run_vmstart() Add trace_loadvm_postcopy_handle_run_bh() with a finer granule trace. Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Peter Xu Message-Id: <20220301083925.33483-4-peterx@redhat.com> Signed-off-by: Dr. David Alan Gilbert --- migration/savevm.c | 12 +++++++++--- migration/trace-events | 3 +-- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/migration/savevm.c b/migration/savevm.c index 190cc5fc42..41e3238798 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -2006,13 +2006,19 @@ static void loadvm_postcopy_handle_run_bh(void *opaque) Error *local_err = NULL; MigrationIncomingState *mis = opaque; + trace_loadvm_postcopy_handle_run_bh("enter"); + /* TODO we should move all of this lot into postcopy_ram.c or a shared code * in migration.c */ cpu_synchronize_all_post_init(); + trace_loadvm_postcopy_handle_run_bh("after cpu sync"); + qemu_announce_self(&mis->announce_timer, migrate_announce_params()); + trace_loadvm_postcopy_handle_run_bh("after announce"); + /* Make sure all file formats flush their mutable metadata. * If we get an error here, just don't restart the VM yet. */ bdrv_invalidate_cache_all(&local_err); @@ -2022,9 +2028,7 @@ static void loadvm_postcopy_handle_run_bh(void *opaque) autostart = false; } - trace_loadvm_postcopy_handle_run_cpu_sync(); - - trace_loadvm_postcopy_handle_run_vmstart(); + trace_loadvm_postcopy_handle_run_bh("after invalidate cache"); dirty_bitmap_mig_before_vm_start(); @@ -2037,6 +2041,8 @@ static void loadvm_postcopy_handle_run_bh(void *opaque) } qemu_bh_delete(mis->bh); + + trace_loadvm_postcopy_handle_run_bh("return"); } /* After all discards we can start running and asking for pages */ diff --git a/migration/trace-events b/migration/trace-events index 92596c00d8..1aec580e92 100644 --- a/migration/trace-events +++ b/migration/trace-events @@ -16,8 +16,7 @@ loadvm_handle_recv_bitmap(char *s) "%s" loadvm_postcopy_handle_advise(void) "" loadvm_postcopy_handle_listen(const char *str) "%s" loadvm_postcopy_handle_run(void) "" -loadvm_postcopy_handle_run_cpu_sync(void) "" -loadvm_postcopy_handle_run_vmstart(void) "" +loadvm_postcopy_handle_run_bh(const char *str) "%s" loadvm_postcopy_handle_resume(void) "" loadvm_postcopy_ram_handle_discard(void) "" loadvm_postcopy_ram_handle_discard_end(void) "" From 77dadc3f83745cbdc56500efd4384950415a2158 Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Tue, 1 Mar 2022 16:39:04 +0800 Subject: [PATCH 10/18] migration: Introduce postcopy channels on dest node Postcopy handles huge pages in a special way that currently we can only have one "channel" to transfer the page. It's because when we install pages using UFFDIO_COPY, we need to have the whole huge page ready, it also means we need to have a temp huge page when trying to receive the whole content of the page. Currently all maintainance around this tmp page is global: firstly we'll allocate a temp huge page, then we maintain its status mostly within ram_load_postcopy(). To enable multiple channels for postcopy, the first thing we need to do is to prepare N temp huge pages as caching, one for each channel. Meanwhile we need to maintain the tmp huge page status per-channel too. To give some example, some local variables maintained in ram_load_postcopy() are listed; they are responsible for maintaining temp huge page status: - all_zero: this keeps whether this huge page contains all zeros - target_pages: this counts how many target pages have been copied - host_page: this keeps the host ptr for the page to install Move all these fields to be together with the temp huge pages to form a new structure called PostcopyTmpPage. Then for each (future) postcopy channel, we need one structure to keep the state around. For vanilla postcopy, obviously there's only one channel. It contains both precopy and postcopy pages. This patch teaches the dest migration node to start realize the possible number of postcopy channels by introducing the "postcopy_channels" variable. Its value is calculated when setup postcopy on dest node (during POSTCOPY_LISTEN phase). Vanilla postcopy will have channels=1, but when postcopy-preempt capability is enabled (in the future), we will boost it to 2 because even during partial sending of a precopy huge page we still want to preempt it and start sending the postcopy requested page right away (so we start to keep two temp huge pages; more if we want to enable multifd). In this patch there's a TODO marked for that; so far the channels is always set to 1. We need to send one "host huge page" on one channel only and we cannot split them, because otherwise the data upon the same huge page can locate on more than one channel so we need more complicated logic to manage. One temp host huge page for each channel will be enough for us for now. Postcopy will still always use the index=0 huge page even after this patch. However it prepares for the latter patches where it can start to use multiple channels (which needs src intervention, because only src knows which channel we should use). Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Peter Xu Message-Id: <20220301083925.33483-5-peterx@redhat.com> Signed-off-by: Dr. David Alan Gilbert dgilbert: Fixed up long line --- migration/migration.h | 36 ++++++++++++++++++++++++- migration/postcopy-ram.c | 58 +++++++++++++++++++++++++++++++--------- migration/ram.c | 45 +++++++++++++++---------------- migration/savevm.c | 12 +++++++++ 4 files changed, 113 insertions(+), 38 deletions(-) diff --git a/migration/migration.h b/migration/migration.h index 8130b703eb..42c7395094 100644 --- a/migration/migration.h +++ b/migration/migration.h @@ -45,6 +45,24 @@ struct PostcopyBlocktimeContext; */ #define CLEAR_BITMAP_SHIFT_MAX 31 +/* This is an abstraction of a "temp huge page" for postcopy's purpose */ +typedef struct { + /* + * This points to a temporary huge page as a buffer for UFFDIO_COPY. It's + * mmap()ed and needs to be freed when cleanup. + */ + void *tmp_huge_page; + /* + * This points to the host page we're going to install for this temp page. + * It tells us after we've received the whole page, where we should put it. + */ + void *host_addr; + /* Number of small pages copied (in size of TARGET_PAGE_SIZE) */ + unsigned int target_pages; + /* Whether this page contains all zeros */ + bool all_zero; +} PostcopyTmpPage; + /* State for the incoming migration */ struct MigrationIncomingState { QEMUFile *from_src_file; @@ -81,7 +99,22 @@ struct MigrationIncomingState { QemuMutex rp_mutex; /* We send replies from multiple threads */ /* RAMBlock of last request sent to source */ RAMBlock *last_rb; - void *postcopy_tmp_page; + /* + * Number of postcopy channels including the default precopy channel, so + * vanilla postcopy will only contain one channel which contain both + * precopy and postcopy streams. + * + * This is calculated when the src requests to enable postcopy but before + * it starts. Its value can depend on e.g. whether postcopy preemption is + * enabled. + */ + unsigned int postcopy_channels; + /* + * An array of temp host huge pages to be used, one for each postcopy + * channel. + */ + PostcopyTmpPage *postcopy_tmp_pages; + /* This is shared for all postcopy channels */ void *postcopy_tmp_zero_page; /* PostCopyFD's for external userfaultfds & handlers of shared memory */ GArray *postcopy_remote_fds; @@ -391,5 +424,6 @@ bool migration_rate_limit(void); void migration_cancel(const Error *error); void populate_vfio_info(MigrationInfo *info); +void postcopy_temp_page_reset(PostcopyTmpPage *tmp_page); #endif diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c index 2a2cc5faf8..30c3508f44 100644 --- a/migration/postcopy-ram.c +++ b/migration/postcopy-ram.c @@ -526,9 +526,18 @@ int postcopy_ram_incoming_init(MigrationIncomingState *mis) static void postcopy_temp_pages_cleanup(MigrationIncomingState *mis) { - if (mis->postcopy_tmp_page) { - munmap(mis->postcopy_tmp_page, mis->largest_page_size); - mis->postcopy_tmp_page = NULL; + int i; + + if (mis->postcopy_tmp_pages) { + for (i = 0; i < mis->postcopy_channels; i++) { + if (mis->postcopy_tmp_pages[i].tmp_huge_page) { + munmap(mis->postcopy_tmp_pages[i].tmp_huge_page, + mis->largest_page_size); + mis->postcopy_tmp_pages[i].tmp_huge_page = NULL; + } + } + g_free(mis->postcopy_tmp_pages); + mis->postcopy_tmp_pages = NULL; } if (mis->postcopy_tmp_zero_page) { @@ -1092,17 +1101,30 @@ retry: static int postcopy_temp_pages_setup(MigrationIncomingState *mis) { - int err; + PostcopyTmpPage *tmp_page; + int err, i, channels; + void *temp_page; - mis->postcopy_tmp_page = mmap(NULL, mis->largest_page_size, - PROT_READ | PROT_WRITE, - MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); - if (mis->postcopy_tmp_page == MAP_FAILED) { - err = errno; - mis->postcopy_tmp_page = NULL; - error_report("%s: Failed to map postcopy_tmp_page %s", - __func__, strerror(err)); - return -err; + /* TODO: will be boosted when enable postcopy preemption */ + mis->postcopy_channels = 1; + + channels = mis->postcopy_channels; + mis->postcopy_tmp_pages = g_malloc0_n(sizeof(PostcopyTmpPage), channels); + + for (i = 0; i < channels; i++) { + tmp_page = &mis->postcopy_tmp_pages[i]; + temp_page = mmap(NULL, mis->largest_page_size, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + if (temp_page == MAP_FAILED) { + err = errno; + error_report("%s: Failed to map postcopy_tmp_pages[%d]: %s", + __func__, i, strerror(err)); + /* Clean up will be done later */ + return -err; + } + tmp_page->tmp_huge_page = temp_page; + /* Initialize default states for each tmp page */ + postcopy_temp_page_reset(tmp_page); } /* @@ -1352,6 +1374,16 @@ int postcopy_wake_shared(struct PostCopyFD *pcfd, #endif /* ------------------------------------------------------------------------- */ +void postcopy_temp_page_reset(PostcopyTmpPage *tmp_page) +{ + tmp_page->target_pages = 0; + tmp_page->host_addr = NULL; + /* + * This is set to true when reset, and cleared as long as we received any + * of the non-zero small page within this huge page. + */ + tmp_page->all_zero = true; +} void postcopy_fault_thread_notify(MigrationIncomingState *mis) { diff --git a/migration/ram.c b/migration/ram.c index 781f0745dc..fe3de84856 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -3641,11 +3641,8 @@ static int ram_load_postcopy(QEMUFile *f) bool place_needed = false; bool matches_target_page_size = false; MigrationIncomingState *mis = migration_incoming_get_current(); - /* Temporary page that is later 'placed' */ - void *postcopy_host_page = mis->postcopy_tmp_page; - void *host_page = NULL; - bool all_zero = true; - int target_pages = 0; + /* Currently we only use channel 0. TODO: use all the channels */ + PostcopyTmpPage *tmp_page = &mis->postcopy_tmp_pages[0]; while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) { ram_addr_t addr; @@ -3689,7 +3686,7 @@ static int ram_load_postcopy(QEMUFile *f) ret = -EINVAL; break; } - target_pages++; + tmp_page->target_pages++; matches_target_page_size = block->page_size == TARGET_PAGE_SIZE; /* * Postcopy requires that we place whole host pages atomically; @@ -3701,15 +3698,16 @@ static int ram_load_postcopy(QEMUFile *f) * however the source ensures it always sends all the components * of a host page in one chunk. */ - page_buffer = postcopy_host_page + + page_buffer = tmp_page->tmp_huge_page + host_page_offset_from_ram_block_offset(block, addr); /* If all TP are zero then we can optimise the place */ - if (target_pages == 1) { - host_page = host_page_from_ram_block_offset(block, addr); - } else if (host_page != host_page_from_ram_block_offset(block, - addr)) { + if (tmp_page->target_pages == 1) { + tmp_page->host_addr = + host_page_from_ram_block_offset(block, addr); + } else if (tmp_page->host_addr != + host_page_from_ram_block_offset(block, addr)) { /* not the 1st TP within the HP */ - error_report("Non-same host page %p/%p", host_page, + error_report("Non-same host page %p/%p", tmp_page->host_addr, host_page_from_ram_block_offset(block, addr)); ret = -EINVAL; break; @@ -3719,10 +3717,11 @@ static int ram_load_postcopy(QEMUFile *f) * If it's the last part of a host page then we place the host * page */ - if (target_pages == (block->page_size / TARGET_PAGE_SIZE)) { + if (tmp_page->target_pages == + (block->page_size / TARGET_PAGE_SIZE)) { place_needed = true; } - place_source = postcopy_host_page; + place_source = tmp_page->tmp_huge_page; } switch (flags & ~RAM_SAVE_FLAG_CONTINUE) { @@ -3736,12 +3735,12 @@ static int ram_load_postcopy(QEMUFile *f) memset(page_buffer, ch, TARGET_PAGE_SIZE); } if (ch) { - all_zero = false; + tmp_page->all_zero = false; } break; case RAM_SAVE_FLAG_PAGE: - all_zero = false; + tmp_page->all_zero = false; if (!matches_target_page_size) { /* For huge pages, we always use temporary buffer */ qemu_get_buffer(f, page_buffer, TARGET_PAGE_SIZE); @@ -3759,7 +3758,7 @@ static int ram_load_postcopy(QEMUFile *f) } break; case RAM_SAVE_FLAG_COMPRESS_PAGE: - all_zero = false; + tmp_page->all_zero = false; len = qemu_get_be32(f); if (len < 0 || len > compressBound(TARGET_PAGE_SIZE)) { error_report("Invalid compressed data length: %d", len); @@ -3791,16 +3790,14 @@ static int ram_load_postcopy(QEMUFile *f) } if (!ret && place_needed) { - if (all_zero) { - ret = postcopy_place_page_zero(mis, host_page, block); + if (tmp_page->all_zero) { + ret = postcopy_place_page_zero(mis, tmp_page->host_addr, block); } else { - ret = postcopy_place_page(mis, host_page, place_source, - block); + ret = postcopy_place_page(mis, tmp_page->host_addr, + place_source, block); } place_needed = false; - target_pages = 0; - /* Assume we have a zero page until we detect something different */ - all_zero = true; + postcopy_temp_page_reset(tmp_page); } } diff --git a/migration/savevm.c b/migration/savevm.c index 41e3238798..0ccd7e5e3f 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -2579,6 +2579,18 @@ void qemu_loadvm_state_cleanup(void) /* Return true if we should continue the migration, or false. */ static bool postcopy_pause_incoming(MigrationIncomingState *mis) { + int i; + + /* + * If network is interrupted, any temp page we received will be useless + * because we didn't mark them as "received" in receivedmap. After a + * proper recovery later (which will sync src dirty bitmap with receivedmap + * on dest) these cached small pages will be resent again. + */ + for (i = 0; i < mis->postcopy_channels; i++) { + postcopy_temp_page_reset(&mis->postcopy_tmp_pages[i]); + } + trace_postcopy_pause_incoming(); assert(migrate_postcopy_ram()); From cfc7dc8abfdb71e39e48bdb297b5e86fe2121d97 Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Tue, 1 Mar 2022 16:39:05 +0800 Subject: [PATCH 11/18] migration: Dump ramblock and offset too when non-same-page detected In ram_load_postcopy() we'll try to detect non-same-page case and dump error. This error is very helpful for debugging. Adding ramblock & offset into the error log too. Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Peter Xu Message-Id: <20220301083925.33483-6-peterx@redhat.com> Signed-off-by: Dr. David Alan Gilbert dgilbert: Fix up long line --- migration/ram.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/migration/ram.c b/migration/ram.c index fe3de84856..a9d0d100bd 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -3707,8 +3707,12 @@ static int ram_load_postcopy(QEMUFile *f) } else if (tmp_page->host_addr != host_page_from_ram_block_offset(block, addr)) { /* not the 1st TP within the HP */ - error_report("Non-same host page %p/%p", tmp_page->host_addr, - host_page_from_ram_block_offset(block, addr)); + error_report("Non-same host page detected. " + "Target host page %p, received host page %p " + "(rb %s offset 0x"RAM_ADDR_FMT" target_pages %d)", + tmp_page->host_addr, + host_page_from_ram_block_offset(block, addr), + block->idstr, addr, tmp_page->target_pages); ret = -EINVAL; break; } From 095c12a4a2df4bb45e8d1b1f74dc7ea80fef9ca4 Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Tue, 1 Mar 2022 16:39:06 +0800 Subject: [PATCH 12/18] migration: Add postcopy_thread_create() Postcopy create threads. A common manner is we init a sem and use it to sync with the thread. Namely, we have fault_thread_sem and listen_thread_sem and they're only used for this. Make it a shared infrastructure so it's easier to create yet another thread. Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Peter Xu Message-Id: <20220301083925.33483-7-peterx@redhat.com> Signed-off-by: Dr. David Alan Gilbert --- migration/migration.h | 8 +++++--- migration/postcopy-ram.c | 23 +++++++++++++++++------ migration/postcopy-ram.h | 4 ++++ migration/savevm.c | 12 +++--------- 4 files changed, 29 insertions(+), 18 deletions(-) diff --git a/migration/migration.h b/migration/migration.h index 42c7395094..8445e1d14a 100644 --- a/migration/migration.h +++ b/migration/migration.h @@ -70,7 +70,11 @@ struct MigrationIncomingState { /* A hook to allow cleanup at the end of incoming migration */ void *transport_data; void (*transport_cleanup)(void *data); - + /* + * Used to sync thread creations. Note that we can't create threads in + * parallel with this sem. + */ + QemuSemaphore thread_sync_sem; /* * Free at the start of the main state load, set as the main thread finishes * loading state. @@ -83,13 +87,11 @@ struct MigrationIncomingState { size_t largest_page_size; bool have_fault_thread; QemuThread fault_thread; - QemuSemaphore fault_thread_sem; /* Set this when we want the fault thread to quit */ bool fault_thread_quit; bool have_listen_thread; QemuThread listen_thread; - QemuSemaphore listen_thread_sem; /* For the kernel to send us notifications */ int userfault_fd; diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c index 30c3508f44..d08d396c63 100644 --- a/migration/postcopy-ram.c +++ b/migration/postcopy-ram.c @@ -78,6 +78,20 @@ int postcopy_notify(enum PostcopyNotifyReason reason, Error **errp) &pnd); } +/* + * NOTE: this routine is not thread safe, we can't call it concurrently. But it + * should be good enough for migration's purposes. + */ +void postcopy_thread_create(MigrationIncomingState *mis, + QemuThread *thread, const char *name, + void *(*fn)(void *), int joinable) +{ + qemu_sem_init(&mis->thread_sync_sem, 0); + qemu_thread_create(thread, name, fn, mis, joinable); + qemu_sem_wait(&mis->thread_sync_sem); + qemu_sem_destroy(&mis->thread_sync_sem); +} + /* Postcopy needs to detect accesses to pages that haven't yet been copied * across, and efficiently map new pages in, the techniques for doing this * are target OS specific. @@ -902,7 +916,7 @@ static void *postcopy_ram_fault_thread(void *opaque) trace_postcopy_ram_fault_thread_entry(); rcu_register_thread(); mis->last_rb = NULL; /* last RAMBlock we sent part of */ - qemu_sem_post(&mis->fault_thread_sem); + qemu_sem_post(&mis->thread_sync_sem); struct pollfd *pfd; size_t pfd_len = 2 + mis->postcopy_remote_fds->len; @@ -1173,11 +1187,8 @@ int postcopy_ram_incoming_setup(MigrationIncomingState *mis) return -1; } - qemu_sem_init(&mis->fault_thread_sem, 0); - qemu_thread_create(&mis->fault_thread, "postcopy/fault", - postcopy_ram_fault_thread, mis, QEMU_THREAD_JOINABLE); - qemu_sem_wait(&mis->fault_thread_sem); - qemu_sem_destroy(&mis->fault_thread_sem); + postcopy_thread_create(mis, &mis->fault_thread, "postcopy/fault", + postcopy_ram_fault_thread, QEMU_THREAD_JOINABLE); mis->have_fault_thread = true; /* Mark so that we get notified of accesses to unwritten areas */ diff --git a/migration/postcopy-ram.h b/migration/postcopy-ram.h index 6d2b3cf124..07684c0e1d 100644 --- a/migration/postcopy-ram.h +++ b/migration/postcopy-ram.h @@ -135,6 +135,10 @@ void postcopy_remove_notifier(NotifierWithReturn *n); /* Call the notifier list set by postcopy_add_start_notifier */ int postcopy_notify(enum PostcopyNotifyReason reason, Error **errp); +void postcopy_thread_create(MigrationIncomingState *mis, + QemuThread *thread, const char *name, + void *(*fn)(void *), int joinable); + struct PostCopyFD; /* ufd is a pointer to the struct uffd_msg *TODO: more Portable! */ diff --git a/migration/savevm.c b/migration/savevm.c index 0ccd7e5e3f..967ff80547 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -1863,7 +1863,7 @@ static void *postcopy_ram_listen_thread(void *opaque) migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE, MIGRATION_STATUS_POSTCOPY_ACTIVE); - qemu_sem_post(&mis->listen_thread_sem); + qemu_sem_post(&mis->thread_sync_sem); trace_postcopy_ram_listen_thread_start(); rcu_register_thread(); @@ -1988,14 +1988,8 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis) } mis->have_listen_thread = true; - /* Start up the listening thread and wait for it to signal ready */ - qemu_sem_init(&mis->listen_thread_sem, 0); - qemu_thread_create(&mis->listen_thread, "postcopy/listen", - postcopy_ram_listen_thread, NULL, - QEMU_THREAD_DETACHED); - qemu_sem_wait(&mis->listen_thread_sem); - qemu_sem_destroy(&mis->listen_thread_sem); - + postcopy_thread_create(mis, &mis->listen_thread, "postcopy/listen", + postcopy_ram_listen_thread, QEMU_THREAD_DETACHED); trace_loadvm_postcopy_handle_listen("return"); return 0; From 755e8d7cb6ce2ba62d282ffbb367de391fe0cc3d Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Tue, 1 Mar 2022 16:39:07 +0800 Subject: [PATCH 13/18] migration: Move static var in ram_block_from_stream() into global Static variable is very unfriendly to threading of ram_block_from_stream(). Move it into MigrationIncomingState. Make the incoming state pointer to be passed over to ram_block_from_stream() on both caller sites. Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Peter Xu Message-Id: <20220301083925.33483-8-peterx@redhat.com> Signed-off-by: Dr. David Alan Gilbert --- migration/migration.h | 3 ++- migration/ram.c | 13 +++++++++---- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/migration/migration.h b/migration/migration.h index 8445e1d14a..d8b9850eae 100644 --- a/migration/migration.h +++ b/migration/migration.h @@ -66,7 +66,8 @@ typedef struct { /* State for the incoming migration */ struct MigrationIncomingState { QEMUFile *from_src_file; - + /* Previously received RAM's RAMBlock pointer */ + RAMBlock *last_recv_block; /* A hook to allow cleanup at the end of incoming migration */ void *transport_data; void (*transport_cleanup)(void *data); diff --git a/migration/ram.c b/migration/ram.c index a9d0d100bd..170e522a1f 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -3185,12 +3185,14 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host) * * Returns a pointer from within the RCU-protected ram_list. * + * @mis: the migration incoming state pointer * @f: QEMUFile where to read the data from * @flags: Page flags (mostly to see if it's a continuation of previous block) */ -static inline RAMBlock *ram_block_from_stream(QEMUFile *f, int flags) +static inline RAMBlock *ram_block_from_stream(MigrationIncomingState *mis, + QEMUFile *f, int flags) { - static RAMBlock *block; + RAMBlock *block = mis->last_recv_block; char id[256]; uint8_t len; @@ -3217,6 +3219,8 @@ static inline RAMBlock *ram_block_from_stream(QEMUFile *f, int flags) return NULL; } + mis->last_recv_block = block; + return block; } @@ -3669,7 +3673,7 @@ static int ram_load_postcopy(QEMUFile *f) trace_ram_load_postcopy_loop((uint64_t)addr, flags); if (flags & (RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE | RAM_SAVE_FLAG_COMPRESS_PAGE)) { - block = ram_block_from_stream(f, flags); + block = ram_block_from_stream(mis, f, flags); if (!block) { ret = -EINVAL; break; @@ -3881,6 +3885,7 @@ void colo_flush_ram_cache(void) */ static int ram_load_precopy(QEMUFile *f) { + MigrationIncomingState *mis = migration_incoming_get_current(); int flags = 0, ret = 0, invalid_flags = 0, len = 0, i = 0; /* ADVISE is earlier, it shows the source has the postcopy capability on */ bool postcopy_advised = postcopy_is_advised(); @@ -3919,7 +3924,7 @@ static int ram_load_precopy(QEMUFile *f) if (flags & (RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE | RAM_SAVE_FLAG_COMPRESS_PAGE | RAM_SAVE_FLAG_XBZRLE)) { - RAMBlock *block = ram_block_from_stream(f, flags); + RAMBlock *block = ram_block_from_stream(mis, f, flags); host = host_from_ram_block_offset(block, addr); /* From d5c8f2afe866b84ce9372d5c52c130d1e457fc95 Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Tue, 1 Mar 2022 16:39:10 +0800 Subject: [PATCH 14/18] migration: Enlarge postcopy recovery to capture !-EIO too We used to have quite a few places making sure -EIO happened and that's the only way to trigger postcopy recovery. That's based on the assumption that we'll only return -EIO for channel issues. It'll work in 99.99% cases but logically that won't cover some corner cases. One example is e.g. ram_block_from_stream() could fail with an interrupted network, then -EINVAL will be returned instead of -EIO. I remembered Dave Gilbert pointed that out before, but somehow this is overlooked. Neither did I encounter anything outside the -EIO error. However we'd better touch that up before it triggers a rare VM data loss during live migrating. To cover as much those cases as possible, remove the -EIO restriction on triggering the postcopy recovery, because even if it's not a channel failure, we can't do anything better than halting QEMU anyway - the corpse of the process may even be used by a good hand to dig out useful memory regions, or the admin could simply kill the process later on. Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Peter Xu Message-Id: <20220301083925.33483-11-peterx@redhat.com> Signed-off-by: Dr. David Alan Gilbert --- migration/migration.c | 4 ++-- migration/postcopy-ram.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/migration/migration.c b/migration/migration.c index bcc385b94b..306e2ac60e 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -2865,7 +2865,7 @@ retry: out: res = qemu_file_get_error(rp); if (res) { - if (res == -EIO && migration_in_postcopy()) { + if (res && migration_in_postcopy()) { /* * Maybe there is something we can do: it looks like a * network down issue, and we pause for a recovery. @@ -3466,7 +3466,7 @@ static MigThrError migration_detect_error(MigrationState *s) error_free(local_error); } - if (state == MIGRATION_STATUS_POSTCOPY_ACTIVE && ret == -EIO) { + if (state == MIGRATION_STATUS_POSTCOPY_ACTIVE && ret) { /* * For postcopy, we allow the network to be down for a * while. After that, it can be continued by a diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c index d08d396c63..b0d12d5053 100644 --- a/migration/postcopy-ram.c +++ b/migration/postcopy-ram.c @@ -1039,7 +1039,7 @@ retry: msg.arg.pagefault.address); if (ret) { /* May be network failure, try to wait for recovery */ - if (ret == -EIO && postcopy_pause_fault_thread(mis)) { + if (postcopy_pause_fault_thread(mis)) { /* We got reconnected somehow, try to continue */ goto retry; } else { From 27dd21b461569f3b85a9db409f9117ae9d976548 Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Tue, 1 Mar 2022 16:39:11 +0800 Subject: [PATCH 15/18] migration: postcopy_pause_fault_thread() never fails Per the title, remove the return code and simplify the callers as the errors will never be triggered. No functional change intended. Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Peter Xu Message-Id: <20220301083925.33483-12-peterx@redhat.com> Signed-off-by: Dr. David Alan Gilbert --- migration/postcopy-ram.c | 25 ++++--------------------- 1 file changed, 4 insertions(+), 21 deletions(-) diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c index b0d12d5053..32c52f4b1d 100644 --- a/migration/postcopy-ram.c +++ b/migration/postcopy-ram.c @@ -891,15 +891,11 @@ static void mark_postcopy_blocktime_end(uintptr_t addr) affected_cpu); } -static bool postcopy_pause_fault_thread(MigrationIncomingState *mis) +static void postcopy_pause_fault_thread(MigrationIncomingState *mis) { trace_postcopy_pause_fault_thread(); - qemu_sem_wait(&mis->postcopy_pause_sem_fault); - trace_postcopy_pause_fault_thread_continued(); - - return true; } /* @@ -959,13 +955,7 @@ static void *postcopy_ram_fault_thread(void *opaque) * broken already using the event. We should hold until * the channel is rebuilt. */ - if (postcopy_pause_fault_thread(mis)) { - /* Continue to read the userfaultfd */ - } else { - error_report("%s: paused but don't allow to continue", - __func__); - break; - } + postcopy_pause_fault_thread(mis); } if (pfd[1].revents) { @@ -1039,15 +1029,8 @@ retry: msg.arg.pagefault.address); if (ret) { /* May be network failure, try to wait for recovery */ - if (postcopy_pause_fault_thread(mis)) { - /* We got reconnected somehow, try to continue */ - goto retry; - } else { - /* This is a unavoidable fault */ - error_report("%s: postcopy_request_page() get %d", - __func__, ret); - break; - } + postcopy_pause_fault_thread(mis); + goto retry; } } From e031149c78489413038e934eec9f54ac699cf322 Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Tue, 1 Mar 2022 16:39:14 +0800 Subject: [PATCH 16/18] migration: Add migration_incoming_transport_cleanup() Add a helper to cleanup the transport listener. When do it, we should also null-ify the cleanup hook and the data, then it's even safe to call it multiple times. Move the socket_address_list cleanup altogether, because that's a mirror of the listener channels and only for the purpose of query-migrate. Hence when someone wants to cleanup the listener transport, it should also want to cleanup the socket list too, always. No functional change intended. Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Peter Xu Message-Id: <20220301083925.33483-15-peterx@redhat.com> Signed-off-by: Dr. David Alan Gilbert --- migration/migration.c | 22 ++++++++++++++-------- migration/migration.h | 1 + 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/migration/migration.c b/migration/migration.c index 306e2ac60e..9cc344514b 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -267,6 +267,19 @@ MigrationIncomingState *migration_incoming_get_current(void) return current_incoming; } +void migration_incoming_transport_cleanup(MigrationIncomingState *mis) +{ + if (mis->socket_address_list) { + qapi_free_SocketAddressList(mis->socket_address_list); + mis->socket_address_list = NULL; + } + + if (mis->transport_cleanup) { + mis->transport_cleanup(mis->transport_data); + mis->transport_data = mis->transport_cleanup = NULL; + } +} + void migration_incoming_state_destroy(void) { struct MigrationIncomingState *mis = migration_incoming_get_current(); @@ -287,10 +300,8 @@ void migration_incoming_state_destroy(void) g_array_free(mis->postcopy_remote_fds, TRUE); mis->postcopy_remote_fds = NULL; } - if (mis->transport_cleanup) { - mis->transport_cleanup(mis->transport_data); - } + migration_incoming_transport_cleanup(mis); qemu_event_reset(&mis->main_thread_load_event); if (mis->page_requested) { @@ -298,11 +309,6 @@ void migration_incoming_state_destroy(void) mis->page_requested = NULL; } - if (mis->socket_address_list) { - qapi_free_SocketAddressList(mis->socket_address_list); - mis->socket_address_list = NULL; - } - yank_unregister_instance(MIGRATION_YANK_INSTANCE); } diff --git a/migration/migration.h b/migration/migration.h index d8b9850eae..2de861df01 100644 --- a/migration/migration.h +++ b/migration/migration.h @@ -166,6 +166,7 @@ struct MigrationIncomingState { MigrationIncomingState *migration_incoming_get_current(void); void migration_incoming_state_destroy(void); +void migration_incoming_transport_cleanup(MigrationIncomingState *mis); /* * Functions to work with blocktime context */ From 3ff57401870481825c50d17afd393f64fa64a0b0 Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Tue, 1 Mar 2022 16:39:25 +0800 Subject: [PATCH 17/18] tests: Pass in MigrateStart** into test_migrate_start() test_migrate_start() will release the MigrateStart structure that passed in, however that's not super clear to the caller because after the call returned the pointer can still be referenced by the callers. It can easily be a source of use-after-free. Let's pass in a double pointer of that, then we can safely clear the pointer for the caller after the struct is released. Signed-off-by: Peter Xu Message-Id: <20220301083925.33483-26-peterx@redhat.com> Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Dr. David Alan Gilbert dgilbert: Fixup apply since I didn't take 24/25 --- tests/qtest/migration-test.c | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c index 7b42f6fd90..0870656d82 100644 --- a/tests/qtest/migration-test.c +++ b/tests/qtest/migration-test.c @@ -495,7 +495,7 @@ static void migrate_start_destroy(MigrateStart *args) } static int test_migrate_start(QTestState **from, QTestState **to, - const char *uri, MigrateStart *args) + const char *uri, MigrateStart **pargs) { g_autofree gchar *arch_source = NULL; g_autofree gchar *arch_target = NULL; @@ -507,6 +507,7 @@ static int test_migrate_start(QTestState **from, QTestState **to, g_autofree char *shmem_path = NULL; const char *arch = qtest_get_arch(); const char *machine_opts = NULL; + MigrateStart *args = *pargs; const char *memory_size; int ret = 0; @@ -621,6 +622,8 @@ static int test_migrate_start(QTestState **from, QTestState **to, out: migrate_start_destroy(args); + /* This tells the caller that this structure is gone */ + *pargs = NULL; return ret; } @@ -665,7 +668,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr, g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs); QTestState *from, *to; - if (test_migrate_start(&from, &to, uri, args)) { + if (test_migrate_start(&from, &to, uri, &args)) { return -1; } @@ -788,7 +791,7 @@ static void test_baddest(void) args->hide_stderr = true; - if (test_migrate_start(&from, &to, "tcp:127.0.0.1:0", args)) { + if (test_migrate_start(&from, &to, "tcp:127.0.0.1:0", &args)) { return; } migrate_qmp(from, "tcp:127.0.0.1:0", "{}"); @@ -804,7 +807,7 @@ static void test_precopy_unix_common(bool dirty_ring) args->use_dirty_ring = dirty_ring; - if (test_migrate_start(&from, &to, uri, args)) { + if (test_migrate_start(&from, &to, uri, &args)) { return; } @@ -892,7 +895,7 @@ static void test_xbzrle(const char *uri) MigrateStart *args = migrate_start_new(); QTestState *from, *to; - if (test_migrate_start(&from, &to, uri, args)) { + if (test_migrate_start(&from, &to, uri, &args)) { return; } @@ -946,7 +949,7 @@ static void test_precopy_tcp(void) g_autofree char *uri = NULL; QTestState *from, *to; - if (test_migrate_start(&from, &to, "tcp:127.0.0.1:0", args)) { + if (test_migrate_start(&from, &to, "tcp:127.0.0.1:0", &args)) { return; } @@ -991,7 +994,7 @@ static void test_migrate_fd_proto(void) QDict *rsp; const char *error_desc; - if (test_migrate_start(&from, &to, "defer", args)) { + if (test_migrate_start(&from, &to, "defer", &args)) { return; } @@ -1071,7 +1074,7 @@ static void do_test_validate_uuid(MigrateStart *args, bool should_fail) g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs); QTestState *from, *to; - if (test_migrate_start(&from, &to, uri, args)) { + if (test_migrate_start(&from, &to, uri, &args)) { return; } @@ -1163,7 +1166,7 @@ static void test_migrate_auto_converge(void) */ const int64_t expected_threshold = max_bandwidth * downtime_limit / 1000; - if (test_migrate_start(&from, &to, uri, args)) { + if (test_migrate_start(&from, &to, uri, &args)) { return; } @@ -1232,7 +1235,7 @@ static void test_multifd_tcp(const char *method) QDict *rsp; g_autofree char *uri = NULL; - if (test_migrate_start(&from, &to, "defer", args)) { + if (test_migrate_start(&from, &to, "defer", &args)) { return; } @@ -1318,7 +1321,7 @@ static void test_multifd_tcp_cancel(void) args->hide_stderr = true; - if (test_migrate_start(&from, &to, "defer", args)) { + if (test_migrate_start(&from, &to, "defer", &args)) { return; } @@ -1357,7 +1360,7 @@ static void test_multifd_tcp_cancel(void) args = migrate_start_new(); args->only_target = true; - if (test_migrate_start(&from, &to2, "defer", args)) { + if (test_migrate_start(&from, &to2, "defer", &args)) { return; } From 18621987027b1800f315fb9e29967e7b5398ef6f Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 15 Feb 2022 17:57:05 +0000 Subject: [PATCH 18/18] migration: Remove load_state_old and minimum_version_id_old There are no longer any VMStateDescription structs in the tree which use the load_state_old support for custom handling of incoming migration from very old QEMU. Remove the mechanism entirely. This includes removing one stray useless setting of minimum_version_id_old in a VMStateDescription with no load_state_old function, which crept in after the global weeding-out of them in commit 17e313406126. Signed-off-by: Peter Maydell Message-Id: <20220215175705.3846411-1-peter.maydell@linaro.org> Reviewed-by: Dr. David Alan Gilbert Reviewed-by: Francisco Iglesias Reviewed-by: Alistair Francis Signed-off-by: Dr. David Alan Gilbert --- docs/devel/migration.rst | 10 ++-------- hw/ssi/xlnx-versal-ospi.c | 1 - include/migration/vmstate.h | 2 -- migration/vmstate.c | 6 ------ 4 files changed, 2 insertions(+), 17 deletions(-) diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst index 2401253482..3e9656d8e0 100644 --- a/docs/devel/migration.rst +++ b/docs/devel/migration.rst @@ -389,19 +389,13 @@ Each version is associated with a series of fields saved. The ``save_state`` al the state as the newer version. But ``load_state`` sometimes is able to load state from an older version. -You can see that there are several version fields: +You can see that there are two version fields: - ``version_id``: the maximum version_id supported by VMState for that device. - ``minimum_version_id``: the minimum version_id that VMState is able to understand for that device. -- ``minimum_version_id_old``: For devices that were not able to port to vmstate, we can - assign a function that knows how to read this old state. This field is - ignored if there is no ``load_state_old`` handler. -VMState is able to read versions from minimum_version_id to -version_id. And the function ``load_state_old()`` (if present) is able to -load state from minimum_version_id_old to minimum_version_id. This -function is deprecated and will be removed when no more users are left. +VMState is able to read versions from minimum_version_id to version_id. There are *_V* forms of many ``VMSTATE_`` macros to load fields for version dependent fields, e.g. diff --git a/hw/ssi/xlnx-versal-ospi.c b/hw/ssi/xlnx-versal-ospi.c index 7ecd148fdf..c762e0b367 100644 --- a/hw/ssi/xlnx-versal-ospi.c +++ b/hw/ssi/xlnx-versal-ospi.c @@ -1800,7 +1800,6 @@ static const VMStateDescription vmstate_xlnx_versal_ospi = { .name = TYPE_XILINX_VERSAL_OSPI, .version_id = 1, .minimum_version_id = 1, - .minimum_version_id_old = 1, .fields = (VMStateField[]) { VMSTATE_FIFO8(rx_fifo, XlnxVersalOspi), VMSTATE_FIFO8(tx_fifo, XlnxVersalOspi), diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h index 017c03675c..ad24aa1934 100644 --- a/include/migration/vmstate.h +++ b/include/migration/vmstate.h @@ -181,9 +181,7 @@ struct VMStateDescription { int unmigratable; int version_id; int minimum_version_id; - int minimum_version_id_old; MigrationPriority priority; - LoadStateHandler *load_state_old; int (*pre_load)(void *opaque); int (*post_load)(void *opaque, int version_id); int (*pre_save)(void *opaque); diff --git a/migration/vmstate.c b/migration/vmstate.c index 05f87cdddc..36ae8b9e19 100644 --- a/migration/vmstate.c +++ b/migration/vmstate.c @@ -90,12 +90,6 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, return -EINVAL; } if (version_id < vmsd->minimum_version_id) { - if (vmsd->load_state_old && - version_id >= vmsd->minimum_version_id_old) { - ret = vmsd->load_state_old(f, opaque, version_id); - trace_vmstate_load_state_end(vmsd->name, "old path", ret); - return ret; - } error_report("%s: incoming version_id %d is too old " "for local minimum version_id %d", vmsd->name, version_id, vmsd->minimum_version_id);