From fac862ffa605f6fa41f52033b27346d26a96bea5 Mon Sep 17 00:00:00 2001 From: Eduardo Habkost Date: Thu, 12 Nov 2015 15:29:54 -0200 Subject: osdep: Change default value of qemu_hw_version() to "2.5+" There are two issues with qemu_hw_version() today: 1) If a machine has hw_version set, the value returned by it is not very useful, because it is not the actual QEMU version. 2) If a machine does't set hw_version, the return value of qemu_hw_version() is broken, because it will change when upgrading QEMU. For those reasons, using qemu_hw_version() is strongly discouraged, and should be used only in code that used QEMU_VERSION in the past and needs to keep compatibility. To fix (2), instead of making every machine broken by default unless they set hw_version, make qemu_hw_version() simply return "2.5+" if qemu_set_hw_version() is not called. Suggested-by: Michael S. Tsirkin Signed-off-by: Eduardo Habkost Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- include/hw/boards.h | 5 +++++ include/qemu/osdep.h | 4 ++++ util/osdep.c | 9 ++++++++- 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/include/hw/boards.h b/include/hw/boards.h index 24eb6f0..5da4fb0 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -51,6 +51,11 @@ bool machine_mem_merge(MachineState *machine); * used to provide @cpu_index to socket number mapping, allowing * a machine to group CPU threads belonging to the same socket/package * Returns: socket number given cpu_index belongs to. + * @hw_version: + * Value of QEMU_VERSION when the machine was added to QEMU. + * Set only by old machines because they need to keep + * compatibility on code that exposed QEMU_VERSION to guests in + * the past (and now use qemu_hw_version()). */ struct MachineClass { /*< private >*/ diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h index 861d84b..84e84ac 100644 --- a/include/qemu/osdep.h +++ b/include/qemu/osdep.h @@ -256,6 +256,10 @@ static inline void qemu_timersub(const struct timeval *val1, void qemu_set_cloexec(int fd); +/* QEMU "hardware version" setting. Used to replace code that exposed + * QEMU_VERSION to guests in the past and need to keep compatibilty. + * Do not use qemu_hw_version() in new code. + */ void qemu_set_hw_version(const char *); const char *qemu_hw_version(void); diff --git a/util/osdep.c b/util/osdep.c index 80c6bfe..534b511 100644 --- a/util/osdep.c +++ b/util/osdep.c @@ -52,7 +52,14 @@ extern int madvise(caddr_t, size_t, int); static bool fips_enabled = false; -static const char *hw_version = QEMU_VERSION; +/* Starting on QEMU 2.5, qemu_hw_version() returns "2.5+" by default + * instead of QEMU_VERSION, so setting hw_version on MachineClass + * is no longer mandatory. + * + * Do NOT change this string, or it will break compatibility on all + * machine classes that don't set hw_version. + */ +static const char *hw_version = "2.5+"; int socket_set_cork(int fd, int v) { -- cgit v1.1 From 463b52f285164ec3dc0649763e06e40cea9e8a1f Mon Sep 17 00:00:00 2001 From: Eduardo Habkost Date: Thu, 12 Nov 2015 15:29:55 -0200 Subject: pc: Don't set hw_version on pc-*-2.5 Now that qemu_hw_version() returns a fixed "2.5+" string instead of QEMU_VERSION, we don't need to set hw_version on pc-*-2.5 explicitly. Suggested-by: Michael S. Tsirkin Signed-off-by: Eduardo Habkost Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/i386/pc_piix.c | 1 - hw/i386/pc_q35.c | 1 - 2 files changed, 2 deletions(-) diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index 07d0baa..2e41efe 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -472,7 +472,6 @@ static void pc_i440fx_machine_options(MachineClass *m) static void pc_i440fx_2_5_machine_options(MachineClass *m) { pc_i440fx_machine_options(m); - m->hw_version = QEMU_VERSION; m->alias = "pc"; m->is_default = 1; } diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index 0fdae09..133bc68 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -373,7 +373,6 @@ static void pc_q35_machine_options(MachineClass *m) static void pc_q35_2_5_machine_options(MachineClass *m) { pc_q35_machine_options(m); - m->hw_version = QEMU_VERSION; m->alias = "q35"; } -- cgit v1.1 From d39c87d70763f2755d1d7a719817b06f0281fb01 Mon Sep 17 00:00:00 2001 From: Wen Congyang Date: Wed, 11 Nov 2015 14:53:29 +0800 Subject: vhost-user: set link down when the char device is closed Signed-off-by: Wen Congyang Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Yuanhan Liu --- net/vhost-user.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/vhost-user.c b/net/vhost-user.c index 0ebd7df..5071602 100644 --- a/net/vhost-user.c +++ b/net/vhost-user.c @@ -188,7 +188,7 @@ static void net_vhost_user_event(void *opaque, int event) qmp_set_link(name, true, &err); break; case CHR_EVENT_CLOSED: - qmp_set_link(name, true, &err); + qmp_set_link(name, false, &err); vhost_user_stop(queues, ncs); break; } -- cgit v1.1 From c61f09ed855b5009f816242ce281fd01586d4646 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Mon, 23 Nov 2015 12:48:52 +0200 Subject: vhost-user: clarify start and enable It seems that we currently have some duplication between started and enabled states. The actual reason is that enable is not documented correctly: what it does is connecting ring to the backend. This is important for MQ, because a Linux guest expects TX packets to be completed even if it disables some queues temporarily. Cc: Yuanhan Liu Cc: Victor Kaplansky Signed-off-by: Michael S. Tsirkin --- docs/specs/vhost-user.txt | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt index 7b9cd6d..0312d40 100644 --- a/docs/specs/vhost-user.txt +++ b/docs/specs/vhost-user.txt @@ -148,13 +148,23 @@ a feature bit was dedicated for this purpose: Starting and stopping rings ---------------------- -Client must only process each ring when it is both started and enabled. +Client must only process each ring when it is started. + +Client must only pass data between the ring and the +backend, when the ring is enabled. + +If ring is started but disabled, client must process the +ring without talking to the backend. + +For example, for a networking device, in the disabled state +client must not supply any new RX packets, but must process +and discard any TX packets. If VHOST_USER_F_PROTOCOL_FEATURES has not been negotiated, the ring is initialized in an enabled state. If VHOST_USER_F_PROTOCOL_FEATURES has been negotiated, the ring is initialized -in a disabled state. Client must not process it until ring is enabled by +in a disabled state. Client must not pass data to/from the backend until ring is enabled by VHOST_USER_SET_VRING_ENABLE with parameter 1, or after it has been disabled by VHOST_USER_SET_VRING_ENABLE with parameter 0. @@ -166,7 +176,7 @@ descriptor is readable) on the descriptor specified by VHOST_USER_SET_VRING_KICK, and stop ring upon receiving VHOST_USER_GET_VRING_BASE. -While processing the rings (when they are started and enabled), client must +While processing the rings (whether they are enabled or not), client must support changing some configuration aspects on the fly. Multiple queue support @@ -309,11 +319,11 @@ Message types Id: 4 Master payload: N/A - This is no longer used. Used to be sent to request stopping + This is no longer used. Used to be sent to request disabling all rings, but some clients interpreted it to also discard connection state (this interpretation would lead to bugs). It is recommended that clients either ignore this message, - or use it to stop all rings. + or use it to disable all rings. * VHOST_USER_SET_MEM_TABLE -- cgit v1.1 From 85ea9da5b8d8c0b2ab77b493d5ce62599279bf33 Mon Sep 17 00:00:00 2001 From: Victor Kaplansky Date: Tue, 24 Nov 2015 12:55:56 +0200 Subject: tests/vhost-user-bridge: propose GUEST_ANNOUNCE feature The backend has to know whether VIRTIO_NET_F_GUEST_ANNOUNCE was negotiated, so, as a hack we propose the feature by vhost-user-bridge during the feature negotiation. Signed-off-by: Victor Kaplansky Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- tests/vhost-user-bridge.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c index 7bdfc98..784f15f 100644 --- a/tests/vhost-user-bridge.c +++ b/tests/vhost-user-bridge.c @@ -747,6 +747,7 @@ vubr_get_features_exec(VubrDev *dev, VhostUserMsg *vmsg) vmsg->payload.u64 = ((1ULL << VIRTIO_NET_F_MRG_RXBUF) | (1ULL << VHOST_F_LOG_ALL) | + (1ULL << VIRTIO_NET_F_GUEST_ANNOUNCE) | (1ULL << VHOST_USER_F_PROTOCOL_FEATURES)); vmsg->size = sizeof(vmsg->payload.u64); -- cgit v1.1 From 7cf32491eac9dc32fd8ca7933f3edc9d42f884ee Mon Sep 17 00:00:00 2001 From: Victor Kaplansky Date: Tue, 24 Nov 2015 12:56:00 +0200 Subject: tests/vhost-user-bridge: read command line arguments Now some vhost-user-bridge parameters can be passed from the command line: Usage: prog [-u ud_socket_path] [-l lhost:lport] [-r rhost:rport] -u path to unix doman socket. default: /tmp/vubr.sock -l local host and port. default: 127.0.0.1:4444 -r remote host and port. default: 127.0.0.1:5555 Signed-off-by: Victor Kaplansky Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- tests/vhost-user-bridge.c | 129 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 111 insertions(+), 18 deletions(-) diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c index 784f15f..85c4c8a 100644 --- a/tests/vhost-user-bridge.c +++ b/tests/vhost-user-bridge.c @@ -45,6 +45,8 @@ #include #include #include +#include +#include #include @@ -1211,32 +1213,61 @@ vubr_new(const char *path) } static void +vubr_set_host(struct sockaddr_in *saddr, const char *host) +{ + if (isdigit(host[0])) { + if (!inet_aton(host, &saddr->sin_addr)) { + fprintf(stderr, "inet_aton() failed.\n"); + exit(1); + } + } else { + struct hostent *he = gethostbyname(host); + + if (!he) { + fprintf(stderr, "gethostbyname() failed.\n"); + exit(1); + } + saddr->sin_addr = *(struct in_addr *)he->h_addr; + } +} + +static void vubr_backend_udp_setup(VubrDev *dev, const char *local_host, - uint16_t local_port, - const char *dest_host, - uint16_t dest_port) + const char *local_port, + const char *remote_host, + const char *remote_port) { int sock; + const char *r; + + int lport, rport; + + lport = strtol(local_port, (char **)&r, 0); + if (r == local_port) { + fprintf(stderr, "lport parsing failed.\n"); + exit(1); + } + + rport = strtol(remote_port, (char **)&r, 0); + if (r == remote_port) { + fprintf(stderr, "rport parsing failed.\n"); + exit(1); + } + struct sockaddr_in si_local = { .sin_family = AF_INET, - .sin_port = htons(local_port), + .sin_port = htons(lport), }; - if (inet_aton(local_host, &si_local.sin_addr) == 0) { - fprintf(stderr, "inet_aton() failed.\n"); - exit(1); - } + vubr_set_host(&si_local, local_host); /* setup destination for sends */ dev->backend_udp_dest = (struct sockaddr_in) { .sin_family = AF_INET, - .sin_port = htons(dest_port), + .sin_port = htons(rport), }; - if (inet_aton(dest_host, &dev->backend_udp_dest.sin_addr) == 0) { - fprintf(stderr, "inet_aton() failed.\n"); - exit(1); - } + vubr_set_host(&dev->backend_udp_dest, remote_host); sock = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP); if (sock == -1) { @@ -1250,7 +1281,7 @@ vubr_backend_udp_setup(VubrDev *dev, dev->backend_udp_sock = sock; dispatcher_add(&dev->dispatcher, sock, dev, vubr_backend_recv_cb); DPRINT("Waiting for data from udp backend on %s:%d...\n", - local_host, local_port); + local_host, lport); } static void @@ -1263,19 +1294,81 @@ vubr_run(VubrDev *dev) } } +static int +vubr_parse_host_port(const char **host, const char **port, const char *buf) +{ + char *p = strchr(buf, ':'); + + if (!p) { + return -1; + } + *p = '\0'; + *host = strdup(buf); + *port = strdup(p + 1); + return 0; +} + +#define DEFAULT_UD_SOCKET "/tmp/vubr.sock" +#define DEFAULT_LHOST "127.0.0.1" +#define DEFAULT_LPORT "4444" +#define DEFAULT_RHOST "127.0.0.1" +#define DEFAULT_RPORT "5555" + +static const char *ud_socket_path = DEFAULT_UD_SOCKET; +static const char *lhost = DEFAULT_LHOST; +static const char *lport = DEFAULT_LPORT; +static const char *rhost = DEFAULT_RHOST; +static const char *rport = DEFAULT_RPORT; + int main(int argc, char *argv[]) { VubrDev *dev; + int opt; - dev = vubr_new("/tmp/vubr.sock"); + while ((opt = getopt(argc, argv, "l:r:u:")) != -1) { + + switch (opt) { + case 'l': + if (vubr_parse_host_port(&lhost, &lport, optarg) < 0) { + goto out; + } + break; + case 'r': + if (vubr_parse_host_port(&rhost, &rport, optarg) < 0) { + goto out; + } + break; + case 'u': + ud_socket_path = strdup(optarg); + break; + default: + goto out; + } + } + + DPRINT("ud socket: %s\n", ud_socket_path); + DPRINT("local: %s:%s\n", lhost, lport); + DPRINT("remote: %s:%s\n", rhost, rport); + + dev = vubr_new(ud_socket_path); if (!dev) { return 1; } - vubr_backend_udp_setup(dev, - "127.0.0.1", 4444, - "127.0.0.1", 5555); + vubr_backend_udp_setup(dev, lhost, lport, rhost, rport); vubr_run(dev); return 0; + +out: + fprintf(stderr, "Usage: %s ", argv[0]); + fprintf(stderr, "[-u ud_socket_path] [-l lhost:lport] [-r rhost:rport]\n"); + fprintf(stderr, "\t-u path to unix doman socket. default: %s\n", + DEFAULT_UD_SOCKET); + fprintf(stderr, "\t-l local host and port. default: %s:%s\n", + DEFAULT_LHOST, DEFAULT_LPORT); + fprintf(stderr, "\t-r remote host and port. default: %s:%s\n", + DEFAULT_RHOST, DEFAULT_RPORT); + + return 1; } -- cgit v1.1 From 449e3578107799817a81249b189f6f82aa9e787d Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 25 Nov 2015 13:39:57 +0200 Subject: Revert "vhost: send SET_VRING_ENABLE at start/stop" This reverts commit 3a12f32229a046f4d4ab0a3a52fb01d2d5a1ab76. In case of live migration several queues can be enabled and not only the first one. So informing backend that only the first queue is enabled is wrong. Reported-by: Thibaut Collet Cc: Yuanhan Liu Signed-off-by: Michael S. Tsirkin Reviewed-by: Yuanhan Liu --- hw/virtio/vhost.c | 9 --------- 1 file changed, 9 deletions(-) diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 1794f0d..de29968 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -1226,11 +1226,6 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev) } } - if (hdev->vhost_ops->vhost_set_vring_enable) { - /* only enable first vq pair by default */ - hdev->vhost_ops->vhost_set_vring_enable(hdev, hdev->vq_index == 0); - } - return 0; fail_log: vhost_log_put(hdev, false); @@ -1261,10 +1256,6 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev) hdev->vq_index + i); } - if (hdev->vhost_ops->vhost_set_vring_enable) { - hdev->vhost_ops->vhost_set_vring_enable(hdev, 0); - } - vhost_log_put(hdev, true); hdev->started = false; hdev->log = NULL; -- cgit v1.1 From 903a41d3415960240cb3b9f1d66f3707b27010d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefano=20Dong=20=28=E8=91=A3=E5=85=B4=E6=B0=B4=29?= Date: Thu, 26 Nov 2015 12:00:12 +0000 Subject: Fix memory leak on error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit hw/ppc/spapr.c: Fix memory leak on error, it was introduced in bc09e0611 hw/acpi/memory_hotplug.c: Fix memory leak on error, it was introduced in 34f2af3d Signed-off-by: Stefano Dong (董兴水) Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/acpi/memory_hotplug.c | 1 + hw/ppc/spapr.c | 1 + 2 files changed, 2 insertions(+) diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c index ce428df..e4b9a01 100644 --- a/hw/acpi/memory_hotplug.c +++ b/hw/acpi/memory_hotplug.c @@ -155,6 +155,7 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data, qapi_event_send_mem_unplug_error(dev->id, error_get_pretty(local_err), &error_abort); + error_free(local_err); break; } trace_mhp_acpi_pc_dimm_deleted(mem_st->selector); diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 030ee35..3bb8bcd 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -125,6 +125,7 @@ static XICSState *xics_system_init(MachineState *machine, error_report("kernel_irqchip requested but unavailable: %s", error_get_pretty(err)); } + error_free(err); } if (!icp) { -- cgit v1.1 From d08e42a1125d384cb53423f5810b0c7ea52dc6c9 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 26 Nov 2015 15:14:02 +0200 Subject: vhost-user-test: fix migration overlap test During migration, source does GET_BASE, destination does SET_BASE. Use that as opposed to fds being configured to detect vhost user running on both source and destination. Signed-off-by: Michael S. Tsirkin --- tests/vhost-user-test.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c index 022223b..e4c36af 100644 --- a/tests/vhost-user-test.c +++ b/tests/vhost-user-test.c @@ -131,6 +131,7 @@ typedef struct TestServer { GMutex data_mutex; GCond data_cond; int log_fd; + uint64_t rings; } TestServer; #if !GLIB_CHECK_VERSION(2, 32, 0) @@ -279,6 +280,9 @@ static void chr_read(void *opaque, const uint8_t *buf, int size) msg.payload.state.num = 0; p = (uint8_t *) &msg; qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size); + + assert(msg.payload.state.index < 2); + s->rings &= ~(0x1ULL << msg.payload.state.index); break; case VHOST_USER_SET_MEM_TABLE: @@ -316,10 +320,9 @@ static void chr_read(void *opaque, const uint8_t *buf, int size) g_cond_signal(&s->data_cond); break; - case VHOST_USER_SET_VRING_ENABLE: - if (!msg.payload.state.num) { - s->fds_num = 0; - } + case VHOST_USER_SET_VRING_BASE: + assert(msg.payload.state.index < 2); + s->rings |= 0x1ULL << msg.payload.state.index; break; default: @@ -486,7 +489,7 @@ static gboolean test_migrate_source_check(GSource *source) { TestMigrateSource *t = (TestMigrateSource *)source; - gboolean overlap = t->src->fds_num > 0 && t->dest->fds_num > 0; + gboolean overlap = t->src->rings && t->dest->rings; g_assert(!overlap); -- cgit v1.1