* [RFC PATCH v1 01/26] chardev: tcp: allow to change the reconnect timer
2020-04-15 0:59 [RFC PATCH v1 00/26] VM introspection Adalbert Lazăr
@ 2020-04-15 0:59 ` Adalbert Lazăr
2020-04-15 0:59 ` [RFC PATCH v1 02/26] char-socket: allow vsock parameters (cid, port) Adalbert Lazăr
` (26 subsequent siblings)
27 siblings, 0 replies; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 0:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Adalbert Lazăr, Marc-André Lureau, Paolo Bonzini
When the introspected VM is paused/suspended/migrated, the introspection
tool removes its hooks from the guest and closes the connection.
This is detected by KVM, which in turn will clean the introspection
structures. Thanks to the reconnect parameter, the chardev will reconnect
with the introspection tool, which will try to hook the VM again,
assuming that the pause/suspend/migration operation has ended.
With this new feature, we can suspend the reconnection.
CC: "Marc-André Lureau" <marcandre.lureau@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
---
chardev/char-fe.c | 11 +++++++++++
chardev/char-socket.c | 14 ++++++++++++++
include/chardev/char-fe.h | 7 +++++++
include/chardev/char.h | 1 +
4 files changed, 33 insertions(+)
diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index f3530a90e6..ac83528078 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -384,3 +384,14 @@ void qemu_chr_fe_disconnect(CharBackend *be)
CHARDEV_GET_CLASS(chr)->chr_disconnect(chr);
}
}
+
+int qemu_chr_fe_reconnect_time(CharBackend *be, int secs)
+{
+ Chardev *chr = be->chr;
+
+ if (chr && CHARDEV_GET_CLASS(chr)->chr_reconnect_time) {
+ return CHARDEV_GET_CLASS(chr)->chr_reconnect_time(chr, secs);
+ }
+
+ return -1;
+}
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 185fe38dda..bd966aace1 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1471,6 +1471,19 @@ char_socket_get_connected(Object *obj, Error **errp)
return s->state == TCP_CHARDEV_STATE_CONNECTED;
}
+static int tcp_chr_reconnect_time(Chardev *chr, int secs)
+{
+ SocketChardev *s = SOCKET_CHARDEV(chr);
+
+ int old = s->reconnect_time;
+
+ if (secs >= 0) {
+ s->reconnect_time = secs;
+ }
+
+ return old;
+}
+
static void char_socket_class_init(ObjectClass *oc, void *data)
{
ChardevClass *cc = CHARDEV_CLASS(oc);
@@ -1481,6 +1494,7 @@ static void char_socket_class_init(ObjectClass *oc, void *data)
cc->chr_write = tcp_chr_write;
cc->chr_sync_read = tcp_chr_sync_read;
cc->chr_disconnect = tcp_chr_disconnect;
+ cc->chr_reconnect_time = tcp_chr_reconnect_time;
cc->get_msgfds = tcp_get_msgfds;
cc->set_msgfds = tcp_set_msgfds;
cc->chr_add_client = tcp_chr_add_client;
diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
index a553843364..ff1897040a 100644
--- a/include/chardev/char-fe.h
+++ b/include/chardev/char-fe.h
@@ -135,6 +135,13 @@ void qemu_chr_fe_accept_input(CharBackend *be);
*/
void qemu_chr_fe_disconnect(CharBackend *be);
+/**
+ * qemu_chr_fe_reconnect_time:
+ *
+ * Change the reconnect time and return the old value.
+ */
+int qemu_chr_fe_reconnect_time(CharBackend *be, int secs);
+
/**
* qemu_chr_fe_wait_connected:
*
diff --git a/include/chardev/char.h b/include/chardev/char.h
index 00589a6025..80204d43ae 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -270,6 +270,7 @@ typedef struct ChardevClass {
int (*chr_add_client)(Chardev *chr, int fd);
int (*chr_wait_connected)(Chardev *chr, Error **errp);
void (*chr_disconnect)(Chardev *chr);
+ int (*chr_reconnect_time)(Chardev *be, int secs);
void (*chr_accept_input)(Chardev *chr);
void (*chr_set_echo)(Chardev *chr, bool echo);
void (*chr_set_fe_open)(Chardev *chr, int fe_open);
^ permalink raw reply related [flat|nested] 46+ messages in thread* [RFC PATCH v1 02/26] char-socket: allow vsock parameters (cid, port)
2020-04-15 0:59 [RFC PATCH v1 00/26] VM introspection Adalbert Lazăr
2020-04-15 0:59 ` [RFC PATCH v1 01/26] chardev: tcp: allow to change the reconnect timer Adalbert Lazăr
@ 2020-04-15 0:59 ` Adalbert Lazăr
2020-04-15 10:43 ` Marc-André Lureau
2020-04-15 0:59 ` [RFC PATCH v1 03/26] char-socket: fix the client mode when created through QMP Adalbert Lazăr
` (25 subsequent siblings)
27 siblings, 1 reply; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 0:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Adalbert Lazăr, Marc-André Lureau, Paolo Bonzini
The introspection tool can run in a separate VM and the introspected
VM will establish a connection using a virtual socket.
CC: "Marc-André Lureau" <marcandre.lureau@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
---
chardev/char-socket.c | 27 ++++++++++++++++++++++++---
chardev/char.c | 3 +++
2 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index bd966aace1..9b2deb0125 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -23,6 +23,11 @@
*/
#include "qemu/osdep.h"
+
+#ifdef CONFIG_AF_VSOCK
+#include <linux/vm_sockets.h>
+#endif /* CONFIG_AF_VSOCK */
+
#include "chardev/char.h"
#include "io/channel-socket.h"
#include "io/channel-tls.h"
@@ -590,6 +595,14 @@ static char *qemu_chr_compute_filename(SocketChardev *s)
s->is_listen ? ",server" : "",
left, phost, right, pserv);
+#ifdef CONFIG_AF_VSOCK
+ case AF_VSOCK:
+ return g_strdup_printf("vsock:%d:%d%s",
+ ((struct sockaddr_vm *)(ss))->svm_cid,
+ ((struct sockaddr_vm *)(ss))->svm_port,
+ s->is_listen ? ",server" : "");
+#endif
+
default:
return g_strdup_printf("unknown");
}
@@ -1378,18 +1391,19 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
{
const char *path = qemu_opt_get(opts, "path");
const char *host = qemu_opt_get(opts, "host");
+ const char *cid = qemu_opt_get(opts, "cid");
const char *port = qemu_opt_get(opts, "port");
const char *fd = qemu_opt_get(opts, "fd");
SocketAddressLegacy *addr;
ChardevSocket *sock;
- if ((!!path + !!fd + !!host) != 1) {
+ if ((!!path + !!fd + !!host + !!cid) != 1) {
error_setg(errp,
- "Exactly one of 'path', 'fd' or 'host' required");
+ "Exactly one of 'path', 'fd', 'cid' or 'host' required");
return;
}
- if (host && !port) {
+ if ((host || cid) && !port) {
error_setg(errp, "chardev: socket: no port given");
return;
}
@@ -1444,6 +1458,13 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
.has_ipv6 = qemu_opt_get(opts, "ipv6"),
.ipv6 = qemu_opt_get_bool(opts, "ipv6", 0),
};
+ } else if (cid) {
+ addr->type = SOCKET_ADDRESS_LEGACY_KIND_VSOCK;
+ addr->u.vsock.data = g_new0(VsockSocketAddress, 1);
+ *addr->u.vsock.data = (VsockSocketAddress) {
+ .cid = g_strdup(cid),
+ .port = g_strdup(port),
+ };
} else if (fd) {
addr->type = SOCKET_ADDRESS_LEGACY_KIND_FD;
addr->u.fd.data = g_new(String, 1);
diff --git a/chardev/char.c b/chardev/char.c
index e77564060d..39e36ceb97 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -852,6 +852,9 @@ QemuOptsList qemu_chardev_opts = {
},{
.name = "host",
.type = QEMU_OPT_STRING,
+ },{
+ .name = "cid",
+ .type = QEMU_OPT_STRING,
},{
.name = "port",
.type = QEMU_OPT_STRING,
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [RFC PATCH v1 02/26] char-socket: allow vsock parameters (cid, port)
2020-04-15 0:59 ` [RFC PATCH v1 02/26] char-socket: allow vsock parameters (cid, port) Adalbert Lazăr
@ 2020-04-15 10:43 ` Marc-André Lureau
2020-04-15 12:09 ` Adalbert Lazăr
0 siblings, 1 reply; 46+ messages in thread
From: Marc-André Lureau @ 2020-04-15 10:43 UTC (permalink / raw)
To: Adalbert Lazăr; +Cc: Paolo Bonzini, QEMU
Hi
On Wed, Apr 15, 2020 at 3:00 AM Adalbert Lazăr <alazar@bitdefender.com> wrote:
>
> The introspection tool can run in a separate VM and the introspected
> VM will establish a connection using a virtual socket.
>
> CC: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
We should also add QMP support.
Please add some tests in tests/test-char.c.
> ---
> chardev/char-socket.c | 27 ++++++++++++++++++++++++---
> chardev/char.c | 3 +++
> 2 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index bd966aace1..9b2deb0125 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -23,6 +23,11 @@
> */
>
> #include "qemu/osdep.h"
> +
> +#ifdef CONFIG_AF_VSOCK
> +#include <linux/vm_sockets.h>
> +#endif /* CONFIG_AF_VSOCK */
> +
> #include "chardev/char.h"
> #include "io/channel-socket.h"
> #include "io/channel-tls.h"
> @@ -590,6 +595,14 @@ static char *qemu_chr_compute_filename(SocketChardev *s)
> s->is_listen ? ",server" : "",
> left, phost, right, pserv);
>
> +#ifdef CONFIG_AF_VSOCK
> + case AF_VSOCK:
> + return g_strdup_printf("vsock:%d:%d%s",
> + ((struct sockaddr_vm *)(ss))->svm_cid,
> + ((struct sockaddr_vm *)(ss))->svm_port,
> + s->is_listen ? ",server" : "");
> +#endif
> +
> default:
> return g_strdup_printf("unknown");
> }
> @@ -1378,18 +1391,19 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
> {
> const char *path = qemu_opt_get(opts, "path");
> const char *host = qemu_opt_get(opts, "host");
> + const char *cid = qemu_opt_get(opts, "cid");
> const char *port = qemu_opt_get(opts, "port");
> const char *fd = qemu_opt_get(opts, "fd");
> SocketAddressLegacy *addr;
> ChardevSocket *sock;
>
> - if ((!!path + !!fd + !!host) != 1) {
> + if ((!!path + !!fd + !!host + !!cid) != 1) {
> error_setg(errp,
> - "Exactly one of 'path', 'fd' or 'host' required");
> + "Exactly one of 'path', 'fd', 'cid' or 'host' required");
> return;
> }
>
> - if (host && !port) {
> + if ((host || cid) && !port) {
> error_setg(errp, "chardev: socket: no port given");
> return;
> }
> @@ -1444,6 +1458,13 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
> .has_ipv6 = qemu_opt_get(opts, "ipv6"),
> .ipv6 = qemu_opt_get_bool(opts, "ipv6", 0),
> };
> + } else if (cid) {
> + addr->type = SOCKET_ADDRESS_LEGACY_KIND_VSOCK;
> + addr->u.vsock.data = g_new0(VsockSocketAddress, 1);
> + *addr->u.vsock.data = (VsockSocketAddress) {
> + .cid = g_strdup(cid),
> + .port = g_strdup(port),
> + };
> } else if (fd) {
> addr->type = SOCKET_ADDRESS_LEGACY_KIND_FD;
> addr->u.fd.data = g_new(String, 1);
> diff --git a/chardev/char.c b/chardev/char.c
> index e77564060d..39e36ceb97 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -852,6 +852,9 @@ QemuOptsList qemu_chardev_opts = {
> },{
> .name = "host",
> .type = QEMU_OPT_STRING,
> + },{
> + .name = "cid",
> + .type = QEMU_OPT_STRING,
> },{
> .name = "port",
> .type = QEMU_OPT_STRING,
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [RFC PATCH v1 02/26] char-socket: allow vsock parameters (cid, port)
2020-04-15 10:43 ` Marc-André Lureau
@ 2020-04-15 12:09 ` Adalbert Lazăr
0 siblings, 0 replies; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 12:09 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: Paolo Bonzini, QEMU
On Wed, 15 Apr 2020 12:43:31 +0200, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> Hi
>
> On Wed, Apr 15, 2020 at 3:00 AM Adalbert Lazăr <alazar@bitdefender.com> wrote:
> >
> > The introspection tool can run in a separate VM and the introspected
> > VM will establish a connection using a virtual socket.
> >
> > CC: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> > CC: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
>
> We should also add QMP support.
>
The virtual socket seems to be created with the next QMP command:
{
"execute" : "chardev-add", "arguments" :
{
"id" : "id1", "backend" :
{
"type" : "socket", "data" :
{
"reconnect" : 10, "addr" :
{
"type" : "vsock", "data" : { "cid" : "321", "port" : "1234" }
}
}
}
}
}
From what I remember, only the creation from command line was missing.
> Please add some tests in tests/test-char.c.
>
Sure.
Thanks.
> > ---
> > chardev/char-socket.c | 27 ++++++++++++++++++++++++---
> > chardev/char.c | 3 +++
> > 2 files changed, 27 insertions(+), 3 deletions(-)
> >
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index bd966aace1..9b2deb0125 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -23,6 +23,11 @@
> > */
> >
> > #include "qemu/osdep.h"
> > +
> > +#ifdef CONFIG_AF_VSOCK
> > +#include <linux/vm_sockets.h>
> > +#endif /* CONFIG_AF_VSOCK */
> > +
> > #include "chardev/char.h"
> > #include "io/channel-socket.h"
> > #include "io/channel-tls.h"
> > @@ -590,6 +595,14 @@ static char *qemu_chr_compute_filename(SocketChardev *s)
> > s->is_listen ? ",server" : "",
> > left, phost, right, pserv);
> >
> > +#ifdef CONFIG_AF_VSOCK
> > + case AF_VSOCK:
> > + return g_strdup_printf("vsock:%d:%d%s",
> > + ((struct sockaddr_vm *)(ss))->svm_cid,
> > + ((struct sockaddr_vm *)(ss))->svm_port,
> > + s->is_listen ? ",server" : "");
> > +#endif
> > +
> > default:
> > return g_strdup_printf("unknown");
> > }
> > @@ -1378,18 +1391,19 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
> > {
> > const char *path = qemu_opt_get(opts, "path");
> > const char *host = qemu_opt_get(opts, "host");
> > + const char *cid = qemu_opt_get(opts, "cid");
> > const char *port = qemu_opt_get(opts, "port");
> > const char *fd = qemu_opt_get(opts, "fd");
> > SocketAddressLegacy *addr;
> > ChardevSocket *sock;
> >
> > - if ((!!path + !!fd + !!host) != 1) {
> > + if ((!!path + !!fd + !!host + !!cid) != 1) {
> > error_setg(errp,
> > - "Exactly one of 'path', 'fd' or 'host' required");
> > + "Exactly one of 'path', 'fd', 'cid' or 'host' required");
> > return;
> > }
> >
> > - if (host && !port) {
> > + if ((host || cid) && !port) {
> > error_setg(errp, "chardev: socket: no port given");
> > return;
> > }
> > @@ -1444,6 +1458,13 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
> > .has_ipv6 = qemu_opt_get(opts, "ipv6"),
> > .ipv6 = qemu_opt_get_bool(opts, "ipv6", 0),
> > };
> > + } else if (cid) {
> > + addr->type = SOCKET_ADDRESS_LEGACY_KIND_VSOCK;
> > + addr->u.vsock.data = g_new0(VsockSocketAddress, 1);
> > + *addr->u.vsock.data = (VsockSocketAddress) {
> > + .cid = g_strdup(cid),
> > + .port = g_strdup(port),
> > + };
> > } else if (fd) {
> > addr->type = SOCKET_ADDRESS_LEGACY_KIND_FD;
> > addr->u.fd.data = g_new(String, 1);
> > diff --git a/chardev/char.c b/chardev/char.c
> > index e77564060d..39e36ceb97 100644
> > --- a/chardev/char.c
> > +++ b/chardev/char.c
> > @@ -852,6 +852,9 @@ QemuOptsList qemu_chardev_opts = {
> > },{
> > .name = "host",
> > .type = QEMU_OPT_STRING,
> > + },{
> > + .name = "cid",
> > + .type = QEMU_OPT_STRING,
> > },{
> > .name = "port",
> > .type = QEMU_OPT_STRING,
> >
>
>
> --
> Marc-André Lureau
^ permalink raw reply [flat|nested] 46+ messages in thread
* [RFC PATCH v1 03/26] char-socket: fix the client mode when created through QMP
2020-04-15 0:59 [RFC PATCH v1 00/26] VM introspection Adalbert Lazăr
2020-04-15 0:59 ` [RFC PATCH v1 01/26] chardev: tcp: allow to change the reconnect timer Adalbert Lazăr
2020-04-15 0:59 ` [RFC PATCH v1 02/26] char-socket: allow vsock parameters (cid, port) Adalbert Lazăr
@ 2020-04-15 0:59 ` Adalbert Lazăr
2020-04-15 10:37 ` Marc-André Lureau
2020-04-15 0:59 ` [RFC PATCH v1 04/26] char-socket: add 'reconnecting' property Adalbert Lazăr
` (24 subsequent siblings)
27 siblings, 1 reply; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 0:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Adalbert Lazăr, Marc-André Lureau, Paolo Bonzini
qmp_chardev_open_socket() ignores the absence of the 'server' argument
and always switches to listen/server mode.
CC: "Marc-André Lureau" <marcandre.lureau@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
---
chardev/char-socket.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 9b2deb0125..fd0106ab85 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1310,7 +1310,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
SocketChardev *s = SOCKET_CHARDEV(chr);
ChardevSocket *sock = backend->u.socket.data;
bool do_nodelay = sock->has_nodelay ? sock->nodelay : false;
- bool is_listen = sock->has_server ? sock->server : true;
+ bool is_listen = sock->has_server ? sock->server : false;
bool is_telnet = sock->has_telnet ? sock->telnet : false;
bool is_tn3270 = sock->has_tn3270 ? sock->tn3270 : false;
bool is_waitconnect = sock->has_wait ? sock->wait : false;
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [RFC PATCH v1 03/26] char-socket: fix the client mode when created through QMP
2020-04-15 0:59 ` [RFC PATCH v1 03/26] char-socket: fix the client mode when created through QMP Adalbert Lazăr
@ 2020-04-15 10:37 ` Marc-André Lureau
2020-04-15 11:47 ` Adalbert Lazăr
0 siblings, 1 reply; 46+ messages in thread
From: Marc-André Lureau @ 2020-04-15 10:37 UTC (permalink / raw)
To: Adalbert Lazăr; +Cc: Paolo Bonzini, QEMU
Hi
On Wed, Apr 15, 2020 at 3:00 AM Adalbert Lazăr <alazar@bitdefender.com> wrote:
>
> qmp_chardev_open_socket() ignores the absence of the 'server' argument
> and always switches to listen/server mode.
>
> CC: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
> ---
> chardev/char-socket.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 9b2deb0125..fd0106ab85 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -1310,7 +1310,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
> SocketChardev *s = SOCKET_CHARDEV(chr);
> ChardevSocket *sock = backend->u.socket.data;
> bool do_nodelay = sock->has_nodelay ? sock->nodelay : false;
> - bool is_listen = sock->has_server ? sock->server : true;
> + bool is_listen = sock->has_server ? sock->server : false;
I don't understand what you mean. It defaults to server mode. We can't
change that.
> bool is_telnet = sock->has_telnet ? sock->telnet : false;
> bool is_tn3270 = sock->has_tn3270 ? sock->tn3270 : false;
> bool is_waitconnect = sock->has_wait ? sock->wait : false;
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH v1 03/26] char-socket: fix the client mode when created through QMP
2020-04-15 10:37 ` Marc-André Lureau
@ 2020-04-15 11:47 ` Adalbert Lazăr
2020-04-15 14:11 ` Markus Armbruster
0 siblings, 1 reply; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 11:47 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: Paolo Bonzini, QEMU
On Wed, 15 Apr 2020 12:37:34 +0200, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> Hi
>
> On Wed, Apr 15, 2020 at 3:00 AM Adalbert Lazăr <alazar@bitdefender.com> wrote:
> >
> > qmp_chardev_open_socket() ignores the absence of the 'server' argument
> > and always switches to listen/server mode.
> >
> > CC: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> > CC: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
> > ---
> > chardev/char-socket.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index 9b2deb0125..fd0106ab85 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -1310,7 +1310,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
> > SocketChardev *s = SOCKET_CHARDEV(chr);
> > ChardevSocket *sock = backend->u.socket.data;
> > bool do_nodelay = sock->has_nodelay ? sock->nodelay : false;
> > - bool is_listen = sock->has_server ? sock->server : true;
> > + bool is_listen = sock->has_server ? sock->server : false;
>
> I don't understand what you mean. It defaults to server mode. We can't
> change that.
First of all, thanks for your comments.
I understand that a chardev socket is either in client mode or in server
mode. If the 'server' parameter is not used, the socket is put in client
mode. At least this is the behavior when the socket is created by parsing
the command line. But, when created through QMP, without the 'server' parameter,
the socket is put in server mode.
Until this moment, I did not think that we can use "server=no" through QMP :))
So, yes. We may create the socket in client mode through QMP without this patch.
>
> > bool is_telnet = sock->has_telnet ? sock->telnet : false;
> > bool is_tn3270 = sock->has_tn3270 ? sock->tn3270 : false;
> > bool is_waitconnect = sock->has_wait ? sock->wait : false;
> >
>
>
> --
> Marc-André Lureau
>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH v1 03/26] char-socket: fix the client mode when created through QMP
2020-04-15 11:47 ` Adalbert Lazăr
@ 2020-04-15 14:11 ` Markus Armbruster
2020-04-15 17:53 ` Adalbert Lazăr
0 siblings, 1 reply; 46+ messages in thread
From: Markus Armbruster @ 2020-04-15 14:11 UTC (permalink / raw)
To: Adalbert Lazãr; +Cc: Paolo Bonzini, Marc-André Lureau, QEMU
Adalbert Lazãr <alazar@bitdefender.com> writes:
> On Wed, 15 Apr 2020 12:37:34 +0200, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
>> Hi
>>
>> On Wed, Apr 15, 2020 at 3:00 AM Adalbert Lazăr <alazar@bitdefender.com> wrote:
>> >
>> > qmp_chardev_open_socket() ignores the absence of the 'server' argument
>> > and always switches to listen/server mode.
>> >
>> > CC: "Marc-André Lureau" <marcandre.lureau@redhat.com>
>> > CC: Paolo Bonzini <pbonzini@redhat.com>
>> > Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
>> > ---
>> > chardev/char-socket.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
>> > index 9b2deb0125..fd0106ab85 100644
>> > --- a/chardev/char-socket.c
>> > +++ b/chardev/char-socket.c
>> > @@ -1310,7 +1310,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
>> > SocketChardev *s = SOCKET_CHARDEV(chr);
>> > ChardevSocket *sock = backend->u.socket.data;
>> > bool do_nodelay = sock->has_nodelay ? sock->nodelay : false;
>> > - bool is_listen = sock->has_server ? sock->server : true;
>> > + bool is_listen = sock->has_server ? sock->server : false;
>>
>> I don't understand what you mean. It defaults to server mode. We can't
>> change that.
>
> First of all, thanks for your comments.
>
> I understand that a chardev socket is either in client mode or in server
> mode. If the 'server' parameter is not used, the socket is put in client
> mode. At least this is the behavior when the socket is created by parsing
> the command line. But, when created through QMP, without the 'server' parameter,
> the socket is put in server mode.
>
> Until this moment, I did not think that we can use "server=no" through QMP :))
Start here:
$ socat "READLINE,history=$HOME/.qmp_history,prompt=QMP>" UNIX-CONNECT:$HOME/work/images/test-qmp
{"QMP": {"version": {"qemu": {"micro": 92, "minor": 2, "major": 4}, "package": "v5.0.0-rc2-30-g25b0509e28"}, "capabilities": ["oob"]}}
QMP>{"execute": "qmp_capabilities"}
{"return": {}}
QMP>{"execute":"chardev-add", "arguments": {"id":"foo", "backend": {"type": "socket", "data": {"addr": {"type": "inet", "data": {"host": "0.0.0.0", "port": "2445"}}, "server": false}}}}
{"error": {"class": "GenericError", "desc": "Failed to connect socket: Connection refused"}}
> So, yes. We may create the socket in client mode through QMP without this patch.
>
>>
>> > bool is_telnet = sock->has_telnet ? sock->telnet : false;
>> > bool is_tn3270 = sock->has_tn3270 ? sock->tn3270 : false;
>> > bool is_waitconnect = sock->has_wait ? sock->wait : false;
>> >
>>
>>
>> --
>> Marc-André Lureau
>>
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [RFC PATCH v1 03/26] char-socket: fix the client mode when created through QMP
2020-04-15 14:11 ` Markus Armbruster
@ 2020-04-15 17:53 ` Adalbert Lazăr
2020-04-16 6:03 ` Markus Armbruster
0 siblings, 1 reply; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 17:53 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Paolo
Bonzini, Marc-André Lureau, QEMU
On Wed, 15 Apr 2020 16:11:14 +0200, Markus Armbruster <armbru@redhat.com> wrote:
> Adalbert Lazãr <alazar@bitdefender.com> writes:
>
> > On Wed, 15 Apr 2020 12:37:34 +0200, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> >> Hi
> >>
> >> On Wed, Apr 15, 2020 at 3:00 AM Adalbert Lazăr <alazar@bitdefender.com> wrote:
> >> >
> >> > qmp_chardev_open_socket() ignores the absence of the 'server' argument
> >> > and always switches to listen/server mode.
> >> >
> >> > CC: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> >> > CC: Paolo Bonzini <pbonzini@redhat.com>
> >> > Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
> >> > ---
> >> > chardev/char-socket.c | 2 +-
> >> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> >> > index 9b2deb0125..fd0106ab85 100644
> >> > --- a/chardev/char-socket.c
> >> > +++ b/chardev/char-socket.c
> >> > @@ -1310,7 +1310,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
> >> > SocketChardev *s = SOCKET_CHARDEV(chr);
> >> > ChardevSocket *sock = backend->u.socket.data;
> >> > bool do_nodelay = sock->has_nodelay ? sock->nodelay : false;
> >> > - bool is_listen = sock->has_server ? sock->server : true;
> >> > + bool is_listen = sock->has_server ? sock->server : false;
> >>
> >> I don't understand what you mean. It defaults to server mode. We can't
> >> change that.
> >
> > First of all, thanks for your comments.
> >
> > I understand that a chardev socket is either in client mode or in server
> > mode. If the 'server' parameter is not used, the socket is put in client
> > mode. At least this is the behavior when the socket is created by parsing
> > the command line. But, when created through QMP, without the 'server' parameter,
> > the socket is put in server mode.
> >
> > Until this moment, I did not think that we can use "server=no" through QMP :))
>
> Start here:
>
> $ socat "READLINE,history=$HOME/.qmp_history,prompt=QMP>" UNIX-CONNECT:$HOME/work/images/test-qmp
> {"QMP": {"version": {"qemu": {"micro": 92, "minor": 2, "major": 4}, "package": "v5.0.0-rc2-30-g25b0509e28"}, "capabilities": ["oob"]}}
> QMP>{"execute": "qmp_capabilities"}
> {"return": {}}
> QMP>{"execute":"chardev-add", "arguments": {"id":"foo", "backend": {"type": "socket", "data": {"addr": {"type": "inet", "data": {"host": "0.0.0.0", "port": "2445"}}, "server": false}}}}
> {"error": {"class": "GenericError", "desc": "Failed to connect socket: Connection refused"}}
>
Thank you, Markus.
I wanted to say that while I was writing the reply, I had an aha! moment and I was
amused that I have not thought to use server=no/false and I used the wrong verb tense.
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [RFC PATCH v1 03/26] char-socket: fix the client mode when created through QMP
2020-04-15 17:53 ` Adalbert Lazăr
@ 2020-04-16 6:03 ` Markus Armbruster
0 siblings, 0 replies; 46+ messages in thread
From: Markus Armbruster @ 2020-04-16 6:03 UTC (permalink / raw)
To: Adalbert Lazãr; +Cc: Paolo Bonzini, Marc-André Lureau, QEMU
Adalbert Lazãr <alazar@bitdefender.com> writes:
> On Wed, 15 Apr 2020 16:11:14 +0200, Markus Armbruster <armbru@redhat.com> wrote:
>> Adalbert Lazãr <alazar@bitdefender.com> writes:
[...]
>> > Until this moment, I did not think that we can use "server=no" through QMP :))
>>
>> Start here:
>>
>> $ socat "READLINE,history=$HOME/.qmp_history,prompt=QMP>" UNIX-CONNECT:$HOME/work/images/test-qmp
>> {"QMP": {"version": {"qemu": {"micro": 92, "minor": 2, "major": 4}, "package": "v5.0.0-rc2-30-g25b0509e28"}, "capabilities": ["oob"]}}
>> QMP>{"execute": "qmp_capabilities"}
>> {"return": {}}
>> QMP>{"execute":"chardev-add", "arguments": {"id":"foo", "backend": {"type": "socket", "data": {"addr": {"type": "inet", "data": {"host": "0.0.0.0", "port": "2445"}}, "server": false}}}}
>> {"error": {"class": "GenericError", "desc": "Failed to connect socket: Connection refused"}}
>>
>
> Thank you, Markus.
>
> I wanted to say that while I was writing the reply, I had an aha! moment and I was
> amused that I have not thought to use server=no/false and I used the wrong verb tense.
Happy to help! Enjoy :)
^ permalink raw reply [flat|nested] 46+ messages in thread
* [RFC PATCH v1 04/26] char-socket: add 'reconnecting' property
2020-04-15 0:59 [RFC PATCH v1 00/26] VM introspection Adalbert Lazăr
` (2 preceding siblings ...)
2020-04-15 0:59 ` [RFC PATCH v1 03/26] char-socket: fix the client mode when created through QMP Adalbert Lazăr
@ 2020-04-15 0:59 ` Adalbert Lazăr
2020-04-15 10:46 ` Marc-André Lureau
2020-04-15 0:59 ` [RFC PATCH v1 05/26] char-socket: add 'fd' property Adalbert Lazăr
` (23 subsequent siblings)
27 siblings, 1 reply; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 0:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Adalbert Lazăr, Marc-André Lureau, Paolo Bonzini
This is used by the VM introspection object to check if the connection
will be reestablished in case it disconnects from some reason.
The closing of the socket is used by any of the three parties involved,
KVM, the introspection tool and QEMU (eg. on force-reset), to signal
the other parties that the session is over. As such, it is very important
that the socket will reconnect.
CC: "Marc-André Lureau" <marcandre.lureau@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
---
chardev/char-socket.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index fd0106ab85..22ab242748 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1492,6 +1492,13 @@ char_socket_get_connected(Object *obj, Error **errp)
return s->state == TCP_CHARDEV_STATE_CONNECTED;
}
+static bool char_socket_get_reconnecting(Object *obj, Error **errp)
+{
+ SocketChardev *s = SOCKET_CHARDEV(obj);
+
+ return s->reconnect_time > 0;
+}
+
static int tcp_chr_reconnect_time(Chardev *chr, int secs)
{
SocketChardev *s = SOCKET_CHARDEV(chr);
@@ -1528,6 +1535,10 @@ static void char_socket_class_init(ObjectClass *oc, void *data)
object_class_property_add_bool(oc, "connected", char_socket_get_connected,
NULL, &error_abort);
+
+ object_class_property_add_bool(oc, "reconnecting",
+ char_socket_get_reconnecting,
+ NULL, &error_abort);
}
static const TypeInfo char_socket_type_info = {
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [RFC PATCH v1 04/26] char-socket: add 'reconnecting' property
2020-04-15 0:59 ` [RFC PATCH v1 04/26] char-socket: add 'reconnecting' property Adalbert Lazăr
@ 2020-04-15 10:46 ` Marc-André Lureau
2020-04-15 12:28 ` Adalbert Lazăr
0 siblings, 1 reply; 46+ messages in thread
From: Marc-André Lureau @ 2020-04-15 10:46 UTC (permalink / raw)
To: Adalbert Lazăr; +Cc: Paolo Bonzini, QEMU
Hi
On Wed, Apr 15, 2020 at 3:03 AM Adalbert Lazăr <alazar@bitdefender.com> wrote:
>
> This is used by the VM introspection object to check if the connection
> will be reestablished in case it disconnects from some reason.
>
> The closing of the socket is used by any of the three parties involved,
> KVM, the introspection tool and QEMU (eg. on force-reset), to signal
> the other parties that the session is over. As such, it is very important
> that the socket will reconnect.
>
> CC: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
> ---
> chardev/char-socket.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index fd0106ab85..22ab242748 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -1492,6 +1492,13 @@ char_socket_get_connected(Object *obj, Error **errp)
> return s->state == TCP_CHARDEV_STATE_CONNECTED;
> }
>
> +static bool char_socket_get_reconnecting(Object *obj, Error **errp)
> +{
> + SocketChardev *s = SOCKET_CHARDEV(obj);
> +
> + return s->reconnect_time > 0;
> +}
> +
> static int tcp_chr_reconnect_time(Chardev *chr, int secs)
> {
> SocketChardev *s = SOCKET_CHARDEV(chr);
> @@ -1528,6 +1535,10 @@ static void char_socket_class_init(ObjectClass *oc, void *data)
>
> object_class_property_add_bool(oc, "connected", char_socket_get_connected,
> NULL, &error_abort);
> +
> + object_class_property_add_bool(oc, "reconnecting",
> + char_socket_get_reconnecting,
> + NULL, &error_abort);
That may be misleading, as the socket connection may be established
and this will return true if reconnect_time > 0. Why not have a
"reconnect-time" property instead?
> }
>
> static const TypeInfo char_socket_type_info = {
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [RFC PATCH v1 04/26] char-socket: add 'reconnecting' property
2020-04-15 10:46 ` Marc-André Lureau
@ 2020-04-15 12:28 ` Adalbert Lazăr
0 siblings, 0 replies; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 12:28 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: Paolo Bonzini, QEMU
On Wed, 15 Apr 2020 12:46:57 +0200, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> Hi
>
> On Wed, Apr 15, 2020 at 3:03 AM Adalbert Lazăr <alazar@bitdefender.com> wrote:
> >
> > This is used by the VM introspection object to check if the connection
> > will be reestablished in case it disconnects from some reason.
> >
> > The closing of the socket is used by any of the three parties involved,
> > KVM, the introspection tool and QEMU (eg. on force-reset), to signal
> > the other parties that the session is over. As such, it is very important
> > that the socket will reconnect.
> >
> > CC: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> > CC: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
> > ---
> > chardev/char-socket.c | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index fd0106ab85..22ab242748 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -1492,6 +1492,13 @@ char_socket_get_connected(Object *obj, Error **errp)
> > return s->state == TCP_CHARDEV_STATE_CONNECTED;
> > }
> >
> > +static bool char_socket_get_reconnecting(Object *obj, Error **errp)
> > +{
> > + SocketChardev *s = SOCKET_CHARDEV(obj);
> > +
> > + return s->reconnect_time > 0;
> > +}
> > +
> > static int tcp_chr_reconnect_time(Chardev *chr, int secs)
> > {
> > SocketChardev *s = SOCKET_CHARDEV(chr);
> > @@ -1528,6 +1535,10 @@ static void char_socket_class_init(ObjectClass *oc, void *data)
> >
> > object_class_property_add_bool(oc, "connected", char_socket_get_connected,
> > NULL, &error_abort);
> > +
> > + object_class_property_add_bool(oc, "reconnecting",
> > + char_socket_get_reconnecting,
> > + NULL, &error_abort);
>
> That may be misleading, as the socket connection may be established
> and this will return true if reconnect_time > 0. Why not have a
> "reconnect-time" property instead?
>
Sure.
Thanks.
^ permalink raw reply [flat|nested] 46+ messages in thread
* [RFC PATCH v1 05/26] char-socket: add 'fd' property
2020-04-15 0:59 [RFC PATCH v1 00/26] VM introspection Adalbert Lazăr
` (3 preceding siblings ...)
2020-04-15 0:59 ` [RFC PATCH v1 04/26] char-socket: add 'reconnecting' property Adalbert Lazăr
@ 2020-04-15 0:59 ` Adalbert Lazăr
2020-04-15 10:56 ` Marc-André Lureau
2020-04-15 0:59 ` [RFC PATCH v1 06/26] E820: extend the table access interface Adalbert Lazăr
` (22 subsequent siblings)
27 siblings, 1 reply; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 0:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Adalbert Lazăr, Marc-André Lureau, Paolo Bonzini
This is used by the VM introspection object, after handshake, to hand
over the file descriptor to KVM.
CC: "Marc-André Lureau" <marcandre.lureau@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
---
chardev/char-socket.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 22ab242748..76d0fb8839 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1499,6 +1499,21 @@ static bool char_socket_get_reconnecting(Object *obj, Error **errp)
return s->reconnect_time > 0;
}
+static void
+char_socket_get_fd(Object *obj, Visitor *v, const char *name, void *opaque,
+ Error **errp)
+{
+ int fd = -1;
+ SocketChardev *s = SOCKET_CHARDEV(obj);
+ QIOChannelSocket *sock = QIO_CHANNEL_SOCKET(s->sioc);
+
+ if (sock) {
+ fd = sock->fd;
+ }
+
+ visit_type_int32(v, name, &fd, errp);
+}
+
static int tcp_chr_reconnect_time(Chardev *chr, int secs)
{
SocketChardev *s = SOCKET_CHARDEV(chr);
@@ -1539,6 +1554,9 @@ static void char_socket_class_init(ObjectClass *oc, void *data)
object_class_property_add_bool(oc, "reconnecting",
char_socket_get_reconnecting,
NULL, &error_abort);
+
+ object_class_property_add(oc, "fd", "int32", char_socket_get_fd,
+ NULL, NULL, NULL, &error_abort);
}
static const TypeInfo char_socket_type_info = {
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [RFC PATCH v1 05/26] char-socket: add 'fd' property
2020-04-15 0:59 ` [RFC PATCH v1 05/26] char-socket: add 'fd' property Adalbert Lazăr
@ 2020-04-15 10:56 ` Marc-André Lureau
2020-04-15 12:55 ` Adalbert Lazăr
0 siblings, 1 reply; 46+ messages in thread
From: Marc-André Lureau @ 2020-04-15 10:56 UTC (permalink / raw)
To: Adalbert Lazăr; +Cc: Paolo Bonzini, QEMU
On Wed, Apr 15, 2020 at 3:07 AM Adalbert Lazăr <alazar@bitdefender.com> wrote:
>
> This is used by the VM introspection object, after handshake, to hand
> over the file descriptor to KVM.
>
> CC: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
patch looks ok,
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Though I wonder about potential conflicts of fd usage here..
> ---
> chardev/char-socket.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 22ab242748..76d0fb8839 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -1499,6 +1499,21 @@ static bool char_socket_get_reconnecting(Object *obj, Error **errp)
> return s->reconnect_time > 0;
> }
>
> +static void
> +char_socket_get_fd(Object *obj, Visitor *v, const char *name, void *opaque,
> + Error **errp)
> +{
> + int fd = -1;
> + SocketChardev *s = SOCKET_CHARDEV(obj);
> + QIOChannelSocket *sock = QIO_CHANNEL_SOCKET(s->sioc);
> +
> + if (sock) {
> + fd = sock->fd;
> + }
> +
> + visit_type_int32(v, name, &fd, errp);
> +}
> +
> static int tcp_chr_reconnect_time(Chardev *chr, int secs)
> {
> SocketChardev *s = SOCKET_CHARDEV(chr);
> @@ -1539,6 +1554,9 @@ static void char_socket_class_init(ObjectClass *oc, void *data)
> object_class_property_add_bool(oc, "reconnecting",
> char_socket_get_reconnecting,
> NULL, &error_abort);
> +
> + object_class_property_add(oc, "fd", "int32", char_socket_get_fd,
> + NULL, NULL, NULL, &error_abort);
> }
>
> static const TypeInfo char_socket_type_info = {
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [RFC PATCH v1 05/26] char-socket: add 'fd' property
2020-04-15 10:56 ` Marc-André Lureau
@ 2020-04-15 12:55 ` Adalbert Lazăr
0 siblings, 0 replies; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 12:55 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: Paolo Bonzini, QEMU
On Wed, 15 Apr 2020 12:56:18 +0200, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> On Wed, Apr 15, 2020 at 3:07 AM Adalbert Lazăr <alazar@bitdefender.com> wrote:
> >
> > This is used by the VM introspection object, after handshake, to hand
> > over the file descriptor to KVM.
> >
> > CC: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> > CC: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
>
>
> patch looks ok,
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
>
> Though I wonder about potential conflicts of fd usage here..
>
The 'plan' was to pass the file descriptor to KVM, disable socket
reconnect and forget about this file descriptor. But, we need a way
to signal from KVM to QEMU that the file descriptor was closed and to
reconnect the socket.
There were some vsock issues in kernel (shutdown on the socket was
not detected in some cases or something else) and having the fd checked
for POLLHUP in QEMU helped, but these issues might be already fixed,
because I've seen a lot of improvements made on vsock code.
>
> > ---
> > chardev/char-socket.c | 18 ++++++++++++++++++
> > 1 file changed, 18 insertions(+)
> >
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index 22ab242748..76d0fb8839 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -1499,6 +1499,21 @@ static bool char_socket_get_reconnecting(Object *obj, Error **errp)
> > return s->reconnect_time > 0;
> > }
> >
> > +static void
> > +char_socket_get_fd(Object *obj, Visitor *v, const char *name, void *opaque,
> > + Error **errp)
> > +{
> > + int fd = -1;
> > + SocketChardev *s = SOCKET_CHARDEV(obj);
> > + QIOChannelSocket *sock = QIO_CHANNEL_SOCKET(s->sioc);
> > +
> > + if (sock) {
> > + fd = sock->fd;
> > + }
> > +
> > + visit_type_int32(v, name, &fd, errp);
> > +}
> > +
> > static int tcp_chr_reconnect_time(Chardev *chr, int secs)
> > {
> > SocketChardev *s = SOCKET_CHARDEV(chr);
> > @@ -1539,6 +1554,9 @@ static void char_socket_class_init(ObjectClass *oc, void *data)
> > object_class_property_add_bool(oc, "reconnecting",
> > char_socket_get_reconnecting,
> > NULL, &error_abort);
> > +
> > + object_class_property_add(oc, "fd", "int32", char_socket_get_fd,
> > + NULL, NULL, NULL, &error_abort);
> > }
> >
> > static const TypeInfo char_socket_type_info = {
> >
>
>
> --
> Marc-André Lureau
>
^ permalink raw reply [flat|nested] 46+ messages in thread
* [RFC PATCH v1 06/26] E820: extend the table access interface
2020-04-15 0:59 [RFC PATCH v1 00/26] VM introspection Adalbert Lazăr
` (4 preceding siblings ...)
2020-04-15 0:59 ` [RFC PATCH v1 05/26] char-socket: add 'fd' property Adalbert Lazăr
@ 2020-04-15 0:59 ` Adalbert Lazăr
2020-04-15 0:59 ` [RFC PATCH v1 07/26] linux-headers: update with VM introspection interface Adalbert Lazăr
` (21 subsequent siblings)
27 siblings, 0 replies; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 0:59 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Michael S. Tsirkin, Adalbert Lazăr,
Paolo Bonzini, Richard Henderson
This new function is necessary for the VM introspection object.
By sending all e820 entries, not just the RAM ones,
the introspection tool can differentiate between
an invalid address and a reserved one.
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Richard Henderson <rth@twiddle.net>
CC: Eduardo Habkost <ehabkost@redhat.com>
CC: "Michael S. Tsirkin" <mst@redhat.com>
CC: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
---
hw/i386/e820_memory_layout.c | 12 ++++++++++++
hw/i386/e820_memory_layout.h | 2 ++
2 files changed, 14 insertions(+)
diff --git a/hw/i386/e820_memory_layout.c b/hw/i386/e820_memory_layout.c
index bcf9eaf837..a875e9e326 100644
--- a/hw/i386/e820_memory_layout.c
+++ b/hw/i386/e820_memory_layout.c
@@ -57,3 +57,15 @@ bool e820_get_entry(int idx, uint32_t type, uint64_t *address, uint64_t *length)
}
return false;
}
+
+bool e820_get_entry2(int idx, uint32_t *type, uint64_t *address,
+ uint64_t *length)
+{
+ if (idx < e820_entries) {
+ *type = le32_to_cpu(e820_table[idx].type);
+ *address = le64_to_cpu(e820_table[idx].address);
+ *length = le64_to_cpu(e820_table[idx].length);
+ return true;
+ }
+ return false;
+}
diff --git a/hw/i386/e820_memory_layout.h b/hw/i386/e820_memory_layout.h
index 2a0ceb8b9c..a4555c21fb 100644
--- a/hw/i386/e820_memory_layout.h
+++ b/hw/i386/e820_memory_layout.h
@@ -36,6 +36,8 @@ int e820_add_entry(uint64_t address, uint64_t length, uint32_t type);
int e820_get_num_entries(void);
bool e820_get_entry(int index, uint32_t type,
uint64_t *address, uint64_t *length);
+bool e820_get_entry2(int index, uint32_t *type,
+ uint64_t *address, uint64_t *length);
^ permalink raw reply related [flat|nested] 46+ messages in thread* [RFC PATCH v1 07/26] linux-headers: update with VM introspection interface
2020-04-15 0:59 [RFC PATCH v1 00/26] VM introspection Adalbert Lazăr
` (5 preceding siblings ...)
2020-04-15 0:59 ` [RFC PATCH v1 06/26] E820: extend the table access interface Adalbert Lazăr
@ 2020-04-15 0:59 ` Adalbert Lazăr
2020-04-15 0:59 ` [RFC PATCH v1 08/26] kvm: add VM introspection usage documentation Adalbert Lazăr
` (20 subsequent siblings)
27 siblings, 0 replies; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 0:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Adalbert Lazăr
Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
---
| 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
--git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 265099100e..4e5d390640 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -1010,6 +1010,7 @@ struct kvm_ppc_resize_hpt {
#define KVM_CAP_ARM_NISV_TO_USER 177
#define KVM_CAP_ARM_INJECT_EXT_DABT 178
#define KVM_CAP_S390_VCPU_RESETS 179
+#define KVM_CAP_INTROSPECTION 180
#ifdef KVM_CAP_IRQ_ROUTING
@@ -1558,6 +1559,25 @@ struct kvm_sev_dbg {
__u32 len;
};
+struct kvm_introspection_hook {
+ __s32 fd;
+ __u32 padding;
+ __u8 uuid[16];
+};
+
+#define KVM_INTROSPECTION_HOOK _IOW(KVMIO, 0xc3, struct kvm_introspection_hook)
+#define KVM_INTROSPECTION_UNHOOK _IO(KVMIO, 0xc4)
+
+struct kvm_introspection_feature {
+ __u32 allow;
+ __s32 id;
+};
+
+#define KVM_INTROSPECTION_COMMAND _IOW(KVMIO, 0xc5, struct kvm_introspection_feature)
+#define KVM_INTROSPECTION_EVENT _IOW(KVMIO, 0xc6, struct kvm_introspection_feature)
+
+#define KVM_INTROSPECTION_PREUNHOOK _IO(KVMIO, 0xc7)
+
#define KVM_DEV_ASSIGN_ENABLE_IOMMU (1 << 0)
#define KVM_DEV_ASSIGN_PCI_2_3 (1 << 1)
#define KVM_DEV_ASSIGN_MASK_INTX (1 << 2)
^ permalink raw reply related [flat|nested] 46+ messages in thread* [RFC PATCH v1 08/26] kvm: add VM introspection usage documentation
2020-04-15 0:59 [RFC PATCH v1 00/26] VM introspection Adalbert Lazăr
` (6 preceding siblings ...)
2020-04-15 0:59 ` [RFC PATCH v1 07/26] linux-headers: update with VM introspection interface Adalbert Lazăr
@ 2020-04-15 0:59 ` Adalbert Lazăr
2020-04-15 0:59 ` [RFC PATCH v1 09/26] kvm: introduce the VM introspection object Adalbert Lazăr
` (19 subsequent siblings)
27 siblings, 0 replies; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 0:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Adalbert Lazăr, Marian Rotariu
From: Marian Rotariu <marian.c.rotariu@gmail.com>
Signed-off-by: Marian Rotariu <marian.c.rotariu@gmail.com>
Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
---
qemu-options.hx | 76 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 76 insertions(+)
diff --git a/qemu-options.hx b/qemu-options.hx
index 16debd03cb..6c5618e310 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -5005,6 +5005,82 @@ SRST
::
(qemu) qom-set /objects/iothread1 poll-max-ns 100000
+
+ ``-object introspection,id=id,chardev=id[,key=id][,handshake_timeout=seconds][,unhook_timeout=seconds][,command=id[,...]][,event=id[,...]]``
+ Defines a VM Introspection (VMI) object that will connect to
+ an introspection tool, initiate the handshake and hand over the connection
+ file descriptor to KVM. The introspection channel will be used by
+ the introspection tool to talk directly with KVM. If the VM is paused
+ or migrated, QEMU will delay the action, signal KVM, which in turn will
+ signal the introspection tool to remove its hooks (e.g. breakpoints
+ placed inside the guest).
+
+ The ``chardev`` parameter provides the introspection channel.
+ This is the id of a previously created chardev socket,
+ with a non-zero reconnect parameter.
+
+ The ``key`` parameter is an optional secret object used to
+ authenticate the instrospection tool.
+
+ The ``handshake_timeout`` parameter specify how long will QEMU
+ wait for the introspection tool during handshake (default is
+ 10 seconds).
+
+ The ``unhook_timeout` parameter specify how long will QEMU
+ wait for the introspection tool on pause/migrate (default is
+ 60 seconds).
+
+ The ``command`` parameter specify an allowed introspection command.
+ It can be used multiple times. If omitted, all commands are
+ allowed. For example, ``command=10,command=8`` will allow the
+ introspection tool to use two commands, KVMI_VCPU_PAUSE(10) and
+ KVMI_VM_WRITE_PHYSICAL(8), in addition to those that are used
+ to query the API, which are always allowed (KVMI_GET_VERSION,
+ KVMI_VM_CHECK_COMMAND and KVMI_VM_CHECK_EVENT).
+
+ The ``event` parameter specify an allowed introspection event.
+ It can be used multiple times. If omitted, all events
+ are allowed. For example, ``event=1,event=3`` will
+ allow the introspection tool to receive only two events,
+ KVMI_EVENT_PAUSE_VCPU(1) and KVMI_EVENT_BREAKPOINT(3).
+
+ VM introspected through a unix socket:
+
+ .. parsed-literal::
+
+ # |qemu_system_x86| \
+ ......
+ -chardev socket,id=vmi_chardev,type=unix,path=/tmp/vmi-guest1.sock,reconnect=10 \
+ -object introspection,id=vmi,chardev=vmi_chardev
+
+ VM introspected by an authenticated introspection tool:
+
+ .. parsed-literal::
+
+ # |qemu_system_x86| \
+ ......
+ -chardev socket,id=vmi_chardev,type=unix,path=/tmp/vmi-guest1.sock,reconnect=10 \
+ -object secret,id=vmi_key,file=/etc/secret \
+ -object introspection,id=vmi,chardev=vmi_chardev,key=vmi_key
+
+ VM introspected through a virtual socket, with the introspection tool
+ listening on port 4321 from another VM started with cid=1234:
+
+ .. parsed-literal::
+
+ # |qemu_system_x86| \
+ ......
+ -chardev socket,id=vmi_chardev,type=vsock,cid=1234,port=4321,reconnect=10 \
+ -object introspection,id=vmi,chardev=vmi_chardev
+
+ VM running the introspection tool:
+
+ .. parsed-literal::
+
+ # |qemu_system_x86| \
+ ......
+ -device vhost-vsock-pci,id=vhost-vsock-pci0,guest-cid=1234
+
ERST
^ permalink raw reply related [flat|nested] 46+ messages in thread* [RFC PATCH v1 09/26] kvm: introduce the VM introspection object
2020-04-15 0:59 [RFC PATCH v1 00/26] VM introspection Adalbert Lazăr
` (7 preceding siblings ...)
2020-04-15 0:59 ` [RFC PATCH v1 08/26] kvm: add VM introspection usage documentation Adalbert Lazăr
@ 2020-04-15 0:59 ` Adalbert Lazăr
2020-04-15 0:59 ` [RFC PATCH v1 10/26] kvm: vmi: add the handshake with the introspection tool Adalbert Lazăr
` (18 subsequent siblings)
27 siblings, 0 replies; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 0:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Adalbert Lazăr, Paolo Bonzini, Marian Rotariu
This is used to initiate the connection with the introspection tool and
hand over the file descriptor to KVM. The object needs a chardev socket
(in client mode) created with the 'reconnect' property set.
CC: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Marian Rotariu <marian.c.rotariu@gmail.com>
Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
---
accel/kvm/Makefile.objs | 1 +
accel/kvm/vmi.c | 168 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 169 insertions(+)
create mode 100644 accel/kvm/vmi.c
diff --git a/accel/kvm/Makefile.objs b/accel/kvm/Makefile.objs
index fdfa481578..5e85294eb3 100644
--- a/accel/kvm/Makefile.objs
+++ b/accel/kvm/Makefile.objs
@@ -1,2 +1,3 @@
obj-y += kvm-all.o
+obj-y += vmi.o
obj-$(call lnot,$(CONFIG_SEV)) += sev-stub.o
diff --git a/accel/kvm/vmi.c b/accel/kvm/vmi.c
new file mode 100644
index 0000000000..883c666a2a
--- /dev/null
+++ b/accel/kvm/vmi.c
@@ -0,0 +1,168 @@
+/*
+ * VM Introspection
+ *
+ * Copyright (C) 2017-2020 Bitdefender S.R.L.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+#include "qom/object_interfaces.h"
+#include "sysemu/sysemu.h"
+#include "sysemu/kvm.h"
+#include "chardev/char.h"
+#include "chardev/char-fe.h"
+
+typedef struct VMIntrospection {
+ Object parent_obj;
+
+ Error *init_error;
+
+ char *chardevid;
+ Chardev *chr;
+
+ Notifier machine_ready;
+ bool created_from_command_line;
+} VMIntrospection;
+
+#define TYPE_VM_INTROSPECTION "introspection"
+
+#define VM_INTROSPECTION(obj) \
+ OBJECT_CHECK(VMIntrospection, (obj), TYPE_VM_INTROSPECTION)
+
+static Error *vm_introspection_init(VMIntrospection *i);
+
+static void machine_ready(Notifier *notifier, void *data)
+{
+ VMIntrospection *i = container_of(notifier, VMIntrospection, machine_ready);
+
+ i->init_error = vm_introspection_init(i);
+ if (i->init_error) {
+ Error *err = error_copy(i->init_error);
+
+ error_report_err(err);
+ if (i->created_from_command_line) {
+ exit(1);
+ }
+ }
+}
+
+static void complete(UserCreatable *uc, Error **errp)
+{
+ VMIntrospection *i = VM_INTROSPECTION(uc);
+
+ if (!i->chardevid) {
+ error_setg(errp, "VMI: chardev is not set");
+ return;
+ }
+
+ i->machine_ready.notify = machine_ready;
+ qemu_add_machine_init_done_notifier(&i->machine_ready);
+
+ /*
+ * If the introspection object is created while parsing the command line,
+ * the machine_ready callback will be called later. At that time,
+ * it vm_introspection_init() fails, exit() will be called.
+ *
+ * If the introspection object is created through QMP, machine_init_done
+ * is already set and qemu_add_machine_init_done_notifier() will
+ * call our machine_done() callback. If vm_introspection_init() fails,
+ * we don't call exit() and report the error back to the user.
+ */
+ if (i->init_error) {
+ *errp = i->init_error;
+ i->init_error = NULL;
+ return;
+ }
+}
+
+static void prop_set_chardev(Object *obj, const char *value, Error **errp)
+{
+ VMIntrospection *i = VM_INTROSPECTION(obj);
+
+ g_free(i->chardevid);
+ i->chardevid = g_strdup(value);
+}
+
+static void class_init(ObjectClass *oc, void *data)
+{
+ UserCreatableClass *uc = USER_CREATABLE_CLASS(oc);
+
+ uc->complete = complete;
+}
+
+static void instance_init(Object *obj)
+{
+ VMIntrospection *i = VM_INTROSPECTION(obj);
+
+ i->created_from_command_line = (qdev_hotplug == false);
+
+ object_property_add_str(obj, "chardev", NULL, prop_set_chardev, NULL);
+}
+
+static void instance_finalize(Object *obj)
+{
+ VMIntrospection *i = VM_INTROSPECTION(obj);
+
+ g_free(i->chardevid);
+
+ error_free(i->init_error);
+}
+
+static const TypeInfo info = {
+ .name = TYPE_VM_INTROSPECTION,
+ .parent = TYPE_OBJECT,
+ .class_init = class_init,
+ .instance_size = sizeof(VMIntrospection),
+ .instance_finalize = instance_finalize,
+ .instance_init = instance_init,
+ .interfaces = (InterfaceInfo[]){
+ {TYPE_USER_CREATABLE},
+ {}
+ }
+};
+
+static void register_types(void)
+{
+ type_register_static(&info);
+}
+
+type_init(register_types);
+
+static Error *vm_introspection_init(VMIntrospection *i)
+{
+ Error *err = NULL;
+ int kvmi_version;
+ Chardev *chr;
+
+ if (!kvm_enabled()) {
+ error_setg(&err, "VMI: missing KVM support");
+ return err;
+ }
+
+ kvmi_version = kvm_check_extension(kvm_state, KVM_CAP_INTROSPECTION);
+ if (kvmi_version == 0) {
+ error_setg(&err,
+ "VMI: missing kernel built with CONFIG_KVM_INTROSPECTION");
+ return err;
+ }
+
+ chr = qemu_chr_find(i->chardevid);
+ if (!chr) {
+ error_setg(&err, "VMI: device '%s' not found", i->chardevid);
+ return err;
+ }
+
+ if (!object_property_get_bool(OBJECT(chr), "reconnecting", &err)) {
+ error_append_hint(&err, "VMI: missing reconnect=N for '%s'",
+ i->chardevid);
+ return err;
+ }
+
+ i->chr = chr;
+
+ return NULL;
+}
^ permalink raw reply related [flat|nested] 46+ messages in thread* [RFC PATCH v1 10/26] kvm: vmi: add the handshake with the introspection tool
2020-04-15 0:59 [RFC PATCH v1 00/26] VM introspection Adalbert Lazăr
` (8 preceding siblings ...)
2020-04-15 0:59 ` [RFC PATCH v1 09/26] kvm: introduce the VM introspection object Adalbert Lazăr
@ 2020-04-15 0:59 ` Adalbert Lazăr
2020-04-15 0:59 ` [RFC PATCH v1 11/26] kvm: vmi: add 'handshake_timeout' property Adalbert Lazăr
` (17 subsequent siblings)
27 siblings, 0 replies; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 0:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Adalbert Lazăr
QEMU sends the name, the UUID and the VM start time and expects the
hash of a secret shared with the introspection tool that can be used to
authenticate it.
Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
---
accel/kvm/vmi.c | 290 +++++++++++++++++++++++++++++++++
include/sysemu/vmi-handshake.h | 45 +++++
2 files changed, 335 insertions(+)
create mode 100644 include/sysemu/vmi-handshake.h
diff --git a/accel/kvm/vmi.c b/accel/kvm/vmi.c
index 883c666a2a..57ded2f69c 100644
--- a/accel/kvm/vmi.c
+++ b/accel/kvm/vmi.c
@@ -8,6 +8,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu-common.h"
#include "qapi/error.h"
#include "qemu/error-report.h"
#include "qom/object_interfaces.h"
@@ -16,6 +17,8 @@
#include "chardev/char.h"
#include "chardev/char-fe.h"
+#include "sysemu/vmi-handshake.h"
+
typedef struct VMIntrospection {
Object parent_obj;
@@ -23,9 +26,19 @@ typedef struct VMIntrospection {
char *chardevid;
Chardev *chr;
+ CharBackend sock;
+ int sock_fd;
+
+ qemu_vmi_from_introspector hsk_in;
+ uint64_t hsk_in_read_pos;
+ uint64_t hsk_in_read_size;
+
+ int64_t vm_start_time;
Notifier machine_ready;
bool created_from_command_line;
+
+ bool kvmi_hooked;
} VMIntrospection;
#define TYPE_VM_INTROSPECTION "introspection"
@@ -50,6 +63,11 @@ static void machine_ready(Notifier *notifier, void *data)
}
}
+static void update_vm_start_time(VMIntrospection *i)
+{
+ i->vm_start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+}
+
static void complete(UserCreatable *uc, Error **errp)
{
VMIntrospection *i = VM_INTROSPECTION(uc);
@@ -87,6 +105,13 @@ static void prop_set_chardev(Object *obj, const char *value, Error **errp)
i->chardevid = g_strdup(value);
}
+static bool chardev_is_connected(VMIntrospection *i, Error **errp)
+{
+ Object *obj = OBJECT(i->chr);
+
+ return obj && object_property_get_bool(obj, "connected", errp);
+}
+
static void class_init(ObjectClass *oc, void *data)
{
UserCreatableClass *uc = USER_CREATABLE_CLASS(oc);
@@ -98,17 +123,60 @@ static void instance_init(Object *obj)
{
VMIntrospection *i = VM_INTROSPECTION(obj);
+ i->sock_fd = -1;
i->created_from_command_line = (qdev_hotplug == false);
+ update_vm_start_time(i);
+
object_property_add_str(obj, "chardev", NULL, prop_set_chardev, NULL);
}
+static void disconnect_chardev(VMIntrospection *i)
+{
+ if (chardev_is_connected(i, NULL)) {
+ qemu_chr_fe_disconnect(&i->sock);
+ }
+}
+
+static void unhook_kvmi(VMIntrospection *i)
+{
+ if (i->kvmi_hooked) {
+ if (kvm_vm_ioctl(kvm_state, KVM_INTROSPECTION_UNHOOK, NULL)) {
+ error_report("VMI: ioctl/KVM_INTROSPECTION_UNHOOK failed, errno %d",
+ errno);
+ }
+ i->kvmi_hooked = false;
+ }
+}
+
+static void shutdown_socket_fd(VMIntrospection *i)
+{
+ /* signal both ends (kernel, introspector) */
+ if (i->sock_fd != -1) {
+ shutdown(i->sock_fd, SHUT_RDWR);
+ i->sock_fd = -1;
+ }
+}
+
+static void disconnect_and_unhook_kvmi(VMIntrospection *i)
+{
+ shutdown_socket_fd(i);
+ disconnect_chardev(i);
+ unhook_kvmi(i);
+}
+
static void instance_finalize(Object *obj)
{
VMIntrospection *i = VM_INTROSPECTION(obj);
g_free(i->chardevid);
+ if (i->chr) {
+ shutdown_socket_fd(i);
+ qemu_chr_fe_deinit(&i->sock, true);
+ unhook_kvmi(i);
+ }
+
error_free(i->init_error);
}
@@ -132,6 +200,210 @@ static void register_types(void)
type_init(register_types);
+static bool send_handshake_info(VMIntrospection *i, Error **errp)
+{
+ qemu_vmi_to_introspector send = {};
+ const char *vm_name;
+ int r;
+
+ send.struct_size = sizeof(send);
+ send.start_time = i->vm_start_time;
+ memcpy(&send.uuid, &qemu_uuid, sizeof(send.uuid));
+ vm_name = qemu_get_vm_name();
+ if (vm_name) {
+ snprintf(send.name, sizeof(send.name), "%s", vm_name);
+ send.name[sizeof(send.name) - 1] = 0;
+ }
+
+ r = qemu_chr_fe_write_all(&i->sock, (uint8_t *)&send, sizeof(send));
+ if (r != sizeof(send)) {
+ error_setg_errno(errp, errno, "VMI: error writing to '%s'",
+ i->chardevid);
+ return false;
+ }
+
+ /* tcp_chr_write may call tcp_chr_disconnect/CHR_EVENT_CLOSED */
+ if (!chardev_is_connected(i, errp)) {
+ error_append_hint(errp, "VMI: qemu_chr_fe_write_all() failed");
+ return false;
+ }
+
+ return true;
+}
+
+static bool validate_handshake(VMIntrospection *i, Error **errp)
+{
+ uint32_t min_accepted_size;
+
+ min_accepted_size = offsetof(qemu_vmi_from_introspector, cookie_hash)
+ + QEMU_VMI_COOKIE_HASH_SIZE;
+
+ if (i->hsk_in.struct_size < min_accepted_size) {
+ error_setg(errp, "VMI: not enough or invalid handshake data");
+ return false;
+ }
+
+ /*
+ * Check hsk_in.struct_size and sizeof(hsk_in) before accessing any
+ * other fields. We might get fewer bytes from applications using
+ * old versions if we extended the qemu_vmi_from_introspector structure.
+ */
+
+ return true;
+}
+
+static bool connect_kernel(VMIntrospection *i, Error **errp)
+{
+ struct kvm_introspection_feature commands, events;
+ struct kvm_introspection_hook kernel;
+ const __s32 all_ids = -1;
+
+ memset(&kernel, 0, sizeof(kernel));
+ memcpy(kernel.uuid, &qemu_uuid, sizeof(kernel.uuid));
+ kernel.fd = i->sock_fd;
+
+ if (kvm_vm_ioctl(kvm_state, KVM_INTROSPECTION_HOOK, &kernel)) {
+ error_setg_errno(errp, -errno,
+ "VMI: ioctl/KVM_INTROSPECTION_HOOK failed");
+ if (errno == -EPERM) {
+ error_append_hint(errp,
+ "Reload the kvm module with kvm.introspection=on");
+ }
+ return false;
+ }
+
+ i->kvmi_hooked = true;
+
+ commands.allow = 1;
+ commands.id = all_ids;
+ if (kvm_vm_ioctl(kvm_state, KVM_INTROSPECTION_COMMAND, &commands)) {
+ error_setg_errno(errp, -errno,
+ "VMI: ioctl/KVM_INTROSPECTION_COMMAND failed");
+ unhook_kvmi(i);
+ return false;
+ }
+
+ events.allow = 1;
+ events.id = all_ids;
+ if (kvm_vm_ioctl(kvm_state, KVM_INTROSPECTION_EVENT, &events)) {
+ error_setg_errno(errp, -errno,
+ "VMI: ioctl/KVM_INTROSPECTION_EVENT failed");
+ unhook_kvmi(i);
+ return false;
+ }
+
+ return true;
+}
+
+/*
+ * We should read only the handshake structure,
+ * which might have a different size than what we expect.
+ */
+static int chr_can_read(void *opaque)
+{
+ VMIntrospection *i = opaque;
+
+ if (i->sock_fd == -1) {
+ return 0;
+ }
+
+ /* first, we read the incoming structure size */
+ if (i->hsk_in_read_pos == 0) {
+ return sizeof(i->hsk_in.struct_size);
+ }
+
+ /* validate the incoming structure size */
+ if (i->hsk_in.struct_size < sizeof(i->hsk_in.struct_size)) {
+ return 0;
+ }
+
+ /* read the rest of the incoming structure */
+ return i->hsk_in.struct_size - i->hsk_in_read_pos;
+}
+
+static bool enough_bytes_for_handshake(VMIntrospection *i)
+{
+ return i->hsk_in_read_pos >= sizeof(i->hsk_in.struct_size)
+ && i->hsk_in_read_size == i->hsk_in.struct_size;
+}
+
+static void validate_and_connect(VMIntrospection *i)
+{
+ Error *local_err = NULL;
+
+ if (!validate_handshake(i, &local_err) || !connect_kernel(i, &local_err)) {
+ error_append_hint(&local_err, "reconnecting\n");
+ warn_report_err(local_err);
+ disconnect_chardev(i);
+ }
+}
+
+static void chr_read(void *opaque, const uint8_t *buf, int size)
+{
+ VMIntrospection *i = opaque;
+ size_t to_read;
+
+ i->hsk_in_read_size += size;
+
+ to_read = sizeof(i->hsk_in) - i->hsk_in_read_pos;
+ if (to_read > size) {
+ to_read = size;
+ }
+
+ if (to_read) {
+ memcpy((uint8_t *)&i->hsk_in + i->hsk_in_read_pos, buf, to_read);
+ i->hsk_in_read_pos += to_read;
+ }
+
+ if (enough_bytes_for_handshake(i)) {
+ validate_and_connect(i);
+ }
+}
+
+static void chr_event_open(VMIntrospection *i)
+{
+ Error *local_err = NULL;
+
+ if (!send_handshake_info(i, &local_err)) {
+ error_append_hint(&local_err, "reconnecting\n");
+ warn_report_err(local_err);
+ disconnect_chardev(i);
+ return;
+ }
+
+ info_report("VMI: introspection tool connected");
+
+ i->sock_fd = object_property_get_int(OBJECT(i->chr), "fd", NULL);
+
+ memset(&i->hsk_in, 0, sizeof(i->hsk_in));
+ i->hsk_in_read_pos = 0;
+ i->hsk_in_read_size = 0;
+}
+
+static void chr_event_close(VMIntrospection *i)
+{
+ if (i->sock_fd != -1) {
+ warn_report("VMI: introspection tool disconnected");
+ disconnect_and_unhook_kvmi(i);
+ }
+}
+
+static void chr_event(void *opaque, QEMUChrEvent event)
+{
+ VMIntrospection *i = opaque;
+
+ switch (event) {
+ case CHR_EVENT_OPENED:
+ chr_event_open(i);
+ break;
+ case CHR_EVENT_CLOSED:
+ chr_event_close(i);
+ break;
+ default:
+ break;
+ }
+}
+
static Error *vm_introspection_init(VMIntrospection *i)
{
Error *err = NULL;
@@ -162,7 +434,25 @@ static Error *vm_introspection_init(VMIntrospection *i)
return err;
}
+ if (!qemu_chr_fe_init(&i->sock, chr, &err)) {
+ error_append_hint(&err, "VMI: device '%s' not initialized",
+ i->chardevid);
+ return err;
+ }
+
i->chr = chr;
+ qemu_chr_fe_set_handlers(&i->sock, chr_can_read, chr_read, chr_event,
+ NULL, i, NULL, true);
+
+ /*
+ * The reconnect timer is triggered by either machine init or by a chardev
+ * disconnect. For the QMP creation, when the machine is already started,
+ * use an artificial disconnect just to restart the timer.
+ */
+ if (!i->created_from_command_line) {
+ qemu_chr_fe_disconnect(&i->sock);
+ }
+
return NULL;
}
diff --git a/include/sysemu/vmi-handshake.h b/include/sysemu/vmi-handshake.h
new file mode 100644
index 0000000000..19bdfb6740
--- /dev/null
+++ b/include/sysemu/vmi-handshake.h
@@ -0,0 +1,45 @@
+/*
+ * QEMU VM Introspection Handshake
+ *
+ */
+
+#ifndef QEMU_VMI_HANDSHAKE_H
+#define QEMU_VMI_HANDSHAKE_H
+
+enum { QEMU_VMI_NAME_SIZE = 64 };
+enum { QEMU_VMI_COOKIE_HASH_SIZE = 20};
+
+/**
+ * qemu_vmi_to_introspector:
+ *
+ * This structure is passed to the introspection tool during the handshake.
+ *
+ * @struct_size: the structure size
+ * @uuid: the UUID
+ * @start_time: the VM start time
+ * @name: the VM name
+ */
+typedef struct qemu_vmi_to_introspector {
+ uint32_t struct_size;
+ uint8_t uuid[16];
+ uint32_t padding;
+ int64_t start_time;
+ char name[QEMU_VMI_NAME_SIZE];
+ /* ... */
+} qemu_vmi_to_introspector;
+
+/**
+ * qemu_vmi_from_introspector:
+ *
+ * This structure is passed by the introspection tool during the handshake.
+ *
+ * @struct_size: the structure size
+ * @cookie_hash: the hash of the cookie know by the introspection tool
+ */
+typedef struct qemu_vmi_from_introspector {
+ uint32_t struct_size;
+ uint8_t cookie_hash[QEMU_VMI_COOKIE_HASH_SIZE];
+ /* ... */
+} qemu_vmi_from_introspector;
+
+#endif /* QEMU_VMI_HANDSHAKE_H */
^ permalink raw reply related [flat|nested] 46+ messages in thread* [RFC PATCH v1 11/26] kvm: vmi: add 'handshake_timeout' property
2020-04-15 0:59 [RFC PATCH v1 00/26] VM introspection Adalbert Lazăr
` (9 preceding siblings ...)
2020-04-15 0:59 ` [RFC PATCH v1 10/26] kvm: vmi: add the handshake with the introspection tool Adalbert Lazăr
@ 2020-04-15 0:59 ` Adalbert Lazăr
2020-04-15 0:59 ` [RFC PATCH v1 12/26] kvm: vmi: add 'key' property Adalbert Lazăr
` (16 subsequent siblings)
27 siblings, 0 replies; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 0:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Adalbert Lazăr
By having a timer during handshake, the blocked connections can be
restored.
Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
---
accel/kvm/vmi.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 65 insertions(+), 1 deletion(-)
diff --git a/accel/kvm/vmi.c b/accel/kvm/vmi.c
index 57ded2f69c..5659663caa 100644
--- a/accel/kvm/vmi.c
+++ b/accel/kvm/vmi.c
@@ -19,6 +19,8 @@
#include "sysemu/vmi-handshake.h"
+#define HANDSHAKE_TIMEOUT_SEC 10
+
typedef struct VMIntrospection {
Object parent_obj;
@@ -32,6 +34,8 @@ typedef struct VMIntrospection {
qemu_vmi_from_introspector hsk_in;
uint64_t hsk_in_read_pos;
uint64_t hsk_in_read_size;
+ GSource *hsk_timer;
+ uint32_t handshake_timeout;
int64_t vm_start_time;
@@ -105,6 +109,26 @@ static void prop_set_chardev(Object *obj, const char *value, Error **errp)
i->chardevid = g_strdup(value);
}
+static void prop_get_uint32(Object *obj, Visitor *v, const char *name,
+ void *opaque, Error **errp)
+{
+ uint32_t *value = opaque;
+
+ visit_type_uint32(v, name, value, errp);
+}
+
+static void prop_set_uint32(Object *obj, Visitor *v, const char *name,
+ void *opaque, Error **errp)
+{
+ uint32_t *value = opaque;
+ Error *local_err = NULL;
+
+ visit_type_uint32(v, name, value, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ }
+}
+
static bool chardev_is_connected(VMIntrospection *i, Error **errp)
{
Object *obj = OBJECT(i->chr);
@@ -129,6 +153,11 @@ static void instance_init(Object *obj)
update_vm_start_time(i);
object_property_add_str(obj, "chardev", NULL, prop_set_chardev, NULL);
+
+ i->handshake_timeout = HANDSHAKE_TIMEOUT_SEC;
+ object_property_add(obj, "handshake_timeout", "uint32",
+ prop_set_uint32, prop_get_uint32,
+ NULL, &i->handshake_timeout, NULL);
}
static void disconnect_chardev(VMIntrospection *i)
@@ -165,12 +194,28 @@ static void disconnect_and_unhook_kvmi(VMIntrospection *i)
unhook_kvmi(i);
}
+static void cancel_timer(GSource *timer)
+{
+ if (timer) {
+ g_source_destroy(timer);
+ g_source_unref(timer);
+ }
+}
+
+static void cancel_handshake_timer(VMIntrospection *i)
+{
+ cancel_timer(i->hsk_timer);
+ i->hsk_timer = NULL;
+}
+
static void instance_finalize(Object *obj)
{
VMIntrospection *i = VM_INTROSPECTION(obj);
g_free(i->chardevid);
+ cancel_handshake_timer(i);
+
if (i->chr) {
shutdown_socket_fd(i);
qemu_chr_fe_deinit(&i->sock, true);
@@ -303,7 +348,7 @@ static int chr_can_read(void *opaque)
{
VMIntrospection *i = opaque;
- if (i->sock_fd == -1) {
+ if (i->hsk_timer == NULL || i->sock_fd == -1) {
return 0;
}
@@ -356,10 +401,24 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
}
if (enough_bytes_for_handshake(i)) {
+ cancel_handshake_timer(i);
validate_and_connect(i);
}
}
+static gboolean chr_timeout(gpointer opaque)
+{
+ VMIntrospection *i = opaque;
+
+ warn_report("VMI: the handshake takes too long");
+
+ g_source_unref(i->hsk_timer);
+ i->hsk_timer = NULL;
+
+ disconnect_and_unhook_kvmi(i);
+ return FALSE;
+}
+
static void chr_event_open(VMIntrospection *i)
{
Error *local_err = NULL;
@@ -378,6 +437,9 @@ static void chr_event_open(VMIntrospection *i)
memset(&i->hsk_in, 0, sizeof(i->hsk_in));
i->hsk_in_read_pos = 0;
i->hsk_in_read_size = 0;
+ i->hsk_timer = qemu_chr_timeout_add_ms(i->chr,
+ i->handshake_timeout * 1000,
+ chr_timeout, i);
}
static void chr_event_close(VMIntrospection *i)
@@ -386,6 +448,8 @@ static void chr_event_close(VMIntrospection *i)
warn_report("VMI: introspection tool disconnected");
disconnect_and_unhook_kvmi(i);
}
+
+ cancel_handshake_timer(i);
}
static void chr_event(void *opaque, QEMUChrEvent event)
^ permalink raw reply related [flat|nested] 46+ messages in thread* [RFC PATCH v1 12/26] kvm: vmi: add 'key' property
2020-04-15 0:59 [RFC PATCH v1 00/26] VM introspection Adalbert Lazăr
` (10 preceding siblings ...)
2020-04-15 0:59 ` [RFC PATCH v1 11/26] kvm: vmi: add 'handshake_timeout' property Adalbert Lazăr
@ 2020-04-15 0:59 ` Adalbert Lazăr
2020-04-15 0:59 ` [RFC PATCH v1 13/26] kvm: vmi: block the object destruction if the chardev is connected Adalbert Lazăr
` (15 subsequent siblings)
27 siblings, 0 replies; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 0:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Adalbert Lazăr
The introspection tool can be authenticated if the 'key' parameter is
set with the ID of a secret object holding a shared secret between the
introspection tool and QEMU of the introspected VM.
Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
---
accel/kvm/vmi.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 66 insertions(+)
diff --git a/accel/kvm/vmi.c b/accel/kvm/vmi.c
index 5659663caa..f456ca56ef 100644
--- a/accel/kvm/vmi.c
+++ b/accel/kvm/vmi.c
@@ -14,6 +14,8 @@
#include "qom/object_interfaces.h"
#include "sysemu/sysemu.h"
#include "sysemu/kvm.h"
+#include "crypto/secret.h"
+#include "crypto/hash.h"
#include "chardev/char.h"
#include "chardev/char-fe.h"
@@ -31,6 +33,11 @@ typedef struct VMIntrospection {
CharBackend sock;
int sock_fd;
+ char *keyid;
+ Object *key;
+ uint8_t cookie_hash[QEMU_VMI_COOKIE_HASH_SIZE];
+ bool key_with_cookie;
+
qemu_vmi_from_introspector hsk_in;
uint64_t hsk_in_read_pos;
uint64_t hsk_in_read_size;
@@ -109,6 +116,14 @@ static void prop_set_chardev(Object *obj, const char *value, Error **errp)
i->chardevid = g_strdup(value);
}
+static void prop_set_key(Object *obj, const char *value, Error **errp)
+{
+ VMIntrospection *i = VM_INTROSPECTION(obj);
+
+ g_free(i->keyid);
+ i->keyid = g_strdup(value);
+}
+
static void prop_get_uint32(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
@@ -153,6 +168,7 @@ static void instance_init(Object *obj)
update_vm_start_time(i);
object_property_add_str(obj, "chardev", NULL, prop_set_chardev, NULL);
+ object_property_add_str(obj, "key", NULL, prop_set_key, NULL);
i->handshake_timeout = HANDSHAKE_TIMEOUT_SEC;
object_property_add(obj, "handshake_timeout", "uint32",
@@ -213,6 +229,7 @@ static void instance_finalize(Object *obj)
VMIntrospection *i = VM_INTROSPECTION(obj);
g_free(i->chardevid);
+ g_free(i->keyid);
cancel_handshake_timer(i);
@@ -276,6 +293,16 @@ static bool send_handshake_info(VMIntrospection *i, Error **errp)
return true;
}
+static bool validate_handshake_cookie(VMIntrospection *i)
+{
+ if (!i->key_with_cookie) {
+ return true;
+ }
+
+ return 0 == memcmp(&i->cookie_hash, &i->hsk_in.cookie_hash,
+ sizeof(i->cookie_hash));
+}
+
static bool validate_handshake(VMIntrospection *i, Error **errp)
{
uint32_t min_accepted_size;
@@ -288,6 +315,11 @@ static bool validate_handshake(VMIntrospection *i, Error **errp)
return false;
}
+ if (!validate_handshake_cookie(i)) {
+ error_setg(errp, "VMI: received cookie doesn't match");
+ return false;
+ }
+
/*
* Check hsk_in.struct_size and sizeof(hsk_in) before accessing any
* other fields. We might get fewer bytes from applications using
@@ -468,6 +500,31 @@ static void chr_event(void *opaque, QEMUChrEvent event)
}
}
+static bool make_cookie_hash(const char *key_id, uint8_t *cookie_hash,
+ Error **errp)
+{
+ uint8_t *cookie = NULL, *hash = NULL;
+ size_t cookie_size, hash_size = 0;
+ bool done = false;
+
+ if (qcrypto_secret_lookup(key_id, &cookie, &cookie_size, errp) == 0
+ && qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA1,
+ (const char *)cookie, cookie_size,
+ &hash, &hash_size, errp) == 0) {
+ if (hash_size == QEMU_VMI_COOKIE_HASH_SIZE) {
+ memcpy(cookie_hash, hash, QEMU_VMI_COOKIE_HASH_SIZE);
+ done = true;
+ } else {
+ error_setg(errp, "VMI: hash algorithm size mismatch");
+ }
+ }
+
+ g_free(cookie);
+ g_free(hash);
+
+ return done;
+}
+
static Error *vm_introspection_init(VMIntrospection *i)
{
Error *err = NULL;
@@ -486,6 +543,15 @@ static Error *vm_introspection_init(VMIntrospection *i)
return err;
}
+ if (i->keyid) {
+ if (!make_cookie_hash(i->keyid, i->cookie_hash, &err)) {
+ return err;
+ }
+ i->key_with_cookie = true;
+ } else {
+ warn_report("VMI: the introspection tool won't be 'authenticated'");
+ }
+
chr = qemu_chr_find(i->chardevid);
if (!chr) {
error_setg(&err, "VMI: device '%s' not found", i->chardevid);
^ permalink raw reply related [flat|nested] 46+ messages in thread* [RFC PATCH v1 13/26] kvm: vmi: block the object destruction if the chardev is connected
2020-04-15 0:59 [RFC PATCH v1 00/26] VM introspection Adalbert Lazăr
` (11 preceding siblings ...)
2020-04-15 0:59 ` [RFC PATCH v1 12/26] kvm: vmi: add 'key' property Adalbert Lazăr
@ 2020-04-15 0:59 ` Adalbert Lazăr
2020-04-15 0:59 ` [RFC PATCH v1 14/26] kvm: vmi: allow only one instance of the introspection object Adalbert Lazăr
` (14 subsequent siblings)
27 siblings, 0 replies; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 0:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Adalbert Lazăr
The introspection tool can modify the VM while it is running
(e.g. it can set breakpoints), and when the VM is no longer introspected
these changes need to be removed. Until then, we block the destruction of
the introspection object that would lead to the unexpected shutdown
of the introspection channel.
Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
---
accel/kvm/vmi.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/accel/kvm/vmi.c b/accel/kvm/vmi.c
index f456ca56ef..2ce8a60565 100644
--- a/accel/kvm/vmi.c
+++ b/accel/kvm/vmi.c
@@ -151,11 +151,19 @@ static bool chardev_is_connected(VMIntrospection *i, Error **errp)
return obj && object_property_get_bool(obj, "connected", errp);
}
+static bool introspection_can_be_deleted(UserCreatable *uc)
+{
+ VMIntrospection *i = VM_INTROSPECTION(uc);
+
+ return !chardev_is_connected(i, NULL);
+}
+
static void class_init(ObjectClass *oc, void *data)
{
UserCreatableClass *uc = USER_CREATABLE_CLASS(oc);
uc->complete = complete;
+ uc->can_be_deleted = introspection_can_be_deleted;
}
static void instance_init(Object *obj)
^ permalink raw reply related [flat|nested] 46+ messages in thread* [RFC PATCH v1 14/26] kvm: vmi: allow only one instance of the introspection object
2020-04-15 0:59 [RFC PATCH v1 00/26] VM introspection Adalbert Lazăr
` (12 preceding siblings ...)
2020-04-15 0:59 ` [RFC PATCH v1 13/26] kvm: vmi: block the object destruction if the chardev is connected Adalbert Lazăr
@ 2020-04-15 0:59 ` Adalbert Lazăr
2020-04-15 0:59 ` [RFC PATCH v1 15/26] kvm: vmi: reconnect the socket on reset Adalbert Lazăr
` (13 subsequent siblings)
27 siblings, 0 replies; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 0:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Adalbert Lazăr
Because only one introspection tool must introspect a VM at a given time,
we block the completion of the second instance.
Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
---
accel/kvm/vmi.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/accel/kvm/vmi.c b/accel/kvm/vmi.c
index 2ce8a60565..54c56c6e13 100644
--- a/accel/kvm/vmi.c
+++ b/accel/kvm/vmi.c
@@ -52,10 +52,18 @@ typedef struct VMIntrospection {
bool kvmi_hooked;
} VMIntrospection;
+typedef struct VMIntrospectionClass {
+ ObjectClass parent_class;
+ uint32_t instance_counter;
+ VMIntrospection *uniq;
+} VMIntrospectionClass;
+
#define TYPE_VM_INTROSPECTION "introspection"
#define VM_INTROSPECTION(obj) \
OBJECT_CHECK(VMIntrospection, (obj), TYPE_VM_INTROSPECTION)
+#define VM_INTROSPECTION_CLASS(class) \
+ OBJECT_CLASS_CHECK(VMIntrospectionClass, (class), TYPE_VM_INTROSPECTION)
static Error *vm_introspection_init(VMIntrospection *i);
@@ -81,8 +89,14 @@ static void update_vm_start_time(VMIntrospection *i)
static void complete(UserCreatable *uc, Error **errp)
{
+ VMIntrospectionClass *ic = VM_INTROSPECTION_CLASS(OBJECT(uc)->class);
VMIntrospection *i = VM_INTROSPECTION(uc);
+ if (ic->instance_counter > 1) {
+ error_setg(errp, "VMI: only one introspection object can be created");
+ return;
+ }
+
if (!i->chardevid) {
error_setg(errp, "VMI: chardev is not set");
return;
@@ -106,6 +120,8 @@ static void complete(UserCreatable *uc, Error **errp)
i->init_error = NULL;
return;
}
+
+ ic->uniq = i;
}
static void prop_set_chardev(Object *obj, const char *value, Error **errp)
@@ -168,8 +184,11 @@ static void class_init(ObjectClass *oc, void *data)
static void instance_init(Object *obj)
{
+ VMIntrospectionClass *ic = VM_INTROSPECTION_CLASS(obj->class);
VMIntrospection *i = VM_INTROSPECTION(obj);
+ ic->instance_counter++;
+
i->sock_fd = -1;
i->created_from_command_line = (qdev_hotplug == false);
@@ -234,6 +253,7 @@ static void cancel_handshake_timer(VMIntrospection *i)
static void instance_finalize(Object *obj)
{
+ VMIntrospectionClass *ic = VM_INTROSPECTION_CLASS(obj->class);
VMIntrospection *i = VM_INTROSPECTION(obj);
g_free(i->chardevid);
@@ -248,12 +268,18 @@ static void instance_finalize(Object *obj)
}
error_free(i->init_error);
+
+ ic->instance_counter--;
+ if (!ic->instance_counter) {
+ ic->uniq = NULL;
+ }
}
static const TypeInfo info = {
.name = TYPE_VM_INTROSPECTION,
.parent = TYPE_OBJECT,
.class_init = class_init,
+ .class_size = sizeof(VMIntrospectionClass),
.instance_size = sizeof(VMIntrospection),
.instance_finalize = instance_finalize,
.instance_init = instance_init,
^ permalink raw reply related [flat|nested] 46+ messages in thread* [RFC PATCH v1 15/26] kvm: vmi: reconnect the socket on reset
2020-04-15 0:59 [RFC PATCH v1 00/26] VM introspection Adalbert Lazăr
` (13 preceding siblings ...)
2020-04-15 0:59 ` [RFC PATCH v1 14/26] kvm: vmi: allow only one instance of the introspection object Adalbert Lazăr
@ 2020-04-15 0:59 ` Adalbert Lazăr
2020-04-15 0:59 ` [RFC PATCH v1 16/26] kvm: vmi: intercept pause/resume Adalbert Lazăr
` (12 subsequent siblings)
27 siblings, 0 replies; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 0:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Adalbert Lazăr, Marian Rotariu
From: Marian Rotariu <marian.c.rotariu@gmail.com>
The guest could be reset from various reasons and by disconnecting the
socket (which would reconnect), KVM and the introspection tool will be
notified and can clean up the introspection structures.
Signed-off-by: Marian Rotariu <marian.c.rotariu@gmail.com>
Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
---
accel/kvm/vmi.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/accel/kvm/vmi.c b/accel/kvm/vmi.c
index 54c56c6e13..5beec2b091 100644
--- a/accel/kvm/vmi.c
+++ b/accel/kvm/vmi.c
@@ -13,6 +13,7 @@
#include "qemu/error-report.h"
#include "qom/object_interfaces.h"
#include "sysemu/sysemu.h"
+#include "sysemu/reset.h"
#include "sysemu/kvm.h"
#include "crypto/secret.h"
#include "crypto/hash.h"
@@ -66,6 +67,7 @@ typedef struct VMIntrospectionClass {
OBJECT_CLASS_CHECK(VMIntrospectionClass, (class), TYPE_VM_INTROSPECTION)
static Error *vm_introspection_init(VMIntrospection *i);
+static void vm_introspection_reset(void *opaque);
static void machine_ready(Notifier *notifier, void *data)
{
@@ -122,6 +124,8 @@ static void complete(UserCreatable *uc, Error **errp)
}
ic->uniq = i;
+
+ qemu_register_reset(vm_introspection_reset, i);
}
static void prop_set_chardev(Object *obj, const char *value, Error **errp)
@@ -273,6 +277,8 @@ static void instance_finalize(Object *obj)
if (!ic->instance_counter) {
ic->uniq = NULL;
}
+
+ qemu_unregister_reset(vm_introspection_reset, i);
}
static const TypeInfo info = {
@@ -534,6 +540,18 @@ static void chr_event(void *opaque, QEMUChrEvent event)
}
}
+static void vm_introspection_reset(void *opaque)
+{
+ VMIntrospection *i = opaque;
+
+ if (i->sock_fd != -1) {
+ info_report("VMI: Reset detected. Closing the socket...");
+ disconnect_and_unhook_kvmi(i);
+ }
+
+ update_vm_start_time(i);
+}
+
static bool make_cookie_hash(const char *key_id, uint8_t *cookie_hash,
Error **errp)
{
^ permalink raw reply related [flat|nested] 46+ messages in thread* [RFC PATCH v1 16/26] kvm: vmi: intercept pause/resume
2020-04-15 0:59 [RFC PATCH v1 00/26] VM introspection Adalbert Lazăr
` (14 preceding siblings ...)
2020-04-15 0:59 ` [RFC PATCH v1 15/26] kvm: vmi: reconnect the socket on reset Adalbert Lazăr
@ 2020-04-15 0:59 ` Adalbert Lazăr
2020-04-15 0:59 ` [RFC PATCH v1 17/26] kvm: vmi: add 'unhook_timeout' property Adalbert Lazăr
` (11 subsequent siblings)
27 siblings, 0 replies; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 0:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Adalbert Lazăr, Marian Rotariu, Markus Armbruster
From: Marian Rotariu <marian.c.rotariu@gmail.com>
Because the introspection tool can run on another VM, suspending either
of these two VMs requires signaling the introspection tool to remove
any changes made to the introspected VM. This is done through the
KVM_INTROSPECTION_PREUNHOOK ioctl. KVM will send an event through the
introspection socket, if active. QEMU will wait for the introspection tool
to let the VM run without being introspected and close the socket.
While the guest is suspended, the socket reconnection is disabled.
CC: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Marian Rotariu <marian.c.rotariu@gmail.com>
Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
---
accel/kvm/vmi.c | 147 +++++++++++++++++++++++++++++++++
accel/stubs/Makefile.objs | 1 +
accel/stubs/vmi-stubs.c | 7 ++
include/sysemu/vmi-intercept.h | 21 +++++
monitor/qmp-cmds.c | 10 +++
5 files changed, 186 insertions(+)
create mode 100644 accel/stubs/vmi-stubs.c
create mode 100644 include/sysemu/vmi-intercept.h
diff --git a/accel/kvm/vmi.c b/accel/kvm/vmi.c
index 5beec2b091..151e27265a 100644
--- a/accel/kvm/vmi.c
+++ b/accel/kvm/vmi.c
@@ -14,12 +14,14 @@
#include "qom/object_interfaces.h"
#include "sysemu/sysemu.h"
#include "sysemu/reset.h"
+#include "sysemu/runstate.h"
#include "sysemu/kvm.h"
#include "crypto/secret.h"
#include "crypto/hash.h"
#include "chardev/char.h"
#include "chardev/char-fe.h"
+#include "sysemu/vmi-intercept.h"
#include "sysemu/vmi-handshake.h"
#define HANDSHAKE_TIMEOUT_SEC 10
@@ -45,6 +47,10 @@ typedef struct VMIntrospection {
GSource *hsk_timer;
uint32_t handshake_timeout;
+ int intercepted_action;
+
+ int reconnect_time;
+
int64_t vm_start_time;
Notifier machine_ready;
@@ -59,6 +65,14 @@ typedef struct VMIntrospectionClass {
VMIntrospection *uniq;
} VMIntrospectionClass;
+static const char *action_string[] = {
+ "none",
+ "suspend",
+ "resume",
+};
+
+static bool suspend_pending;
+
#define TYPE_VM_INTROSPECTION "introspection"
#define VM_INTROSPECTION(obj) \
@@ -412,6 +426,39 @@ static bool connect_kernel(VMIntrospection *i, Error **errp)
return true;
}
+static void enable_socket_reconnect(VMIntrospection *i)
+{
+ if (i->sock_fd == -1 && i->reconnect_time) {
+ qemu_chr_fe_reconnect_time(&i->sock, i->reconnect_time);
+ qemu_chr_fe_disconnect(&i->sock);
+ i->reconnect_time = 0;
+ }
+}
+
+static void maybe_disable_socket_reconnect(VMIntrospection *i)
+{
+ if (i->reconnect_time == 0) {
+ info_report("VMI: disable socket reconnect");
+ i->reconnect_time = qemu_chr_fe_reconnect_time(&i->sock, 0);
+ }
+}
+
+static void continue_with_the_intercepted_action(VMIntrospection *i)
+{
+ switch (i->intercepted_action) {
+ case VMI_INTERCEPT_SUSPEND:
+ vm_stop(RUN_STATE_PAUSED);
+ break;
+ default:
+ error_report("VMI: %s: unexpected action %d",
+ __func__, i->intercepted_action);
+ break;
+ }
+
+ info_report("VMI: continue with '%s'",
+ action_string[i->intercepted_action]);
+}
+
/*
* We should read only the handshake structure,
* which might have a different size than what we expect.
@@ -495,6 +542,14 @@ static void chr_event_open(VMIntrospection *i)
{
Error *local_err = NULL;
+ if (suspend_pending) {
+ info_report("VMI: %s: too soon (suspend=%d)",
+ __func__, suspend_pending);
+ maybe_disable_socket_reconnect(i);
+ qemu_chr_fe_disconnect(&i->sock);
+ return;
+ }
+
if (!send_handshake_info(i, &local_err)) {
error_append_hint(&local_err, "reconnecting\n");
warn_report_err(local_err);
@@ -522,6 +577,15 @@ static void chr_event_close(VMIntrospection *i)
}
cancel_handshake_timer(i);
+
+ if (suspend_pending) {
+ maybe_disable_socket_reconnect(i);
+
+ if (i->intercepted_action != VMI_INTERCEPT_NONE) {
+ continue_with_the_intercepted_action(i);
+ i->intercepted_action = VMI_INTERCEPT_NONE;
+ }
+ }
}
static void chr_event(void *opaque, QEMUChrEvent event)
@@ -540,6 +604,89 @@ static void chr_event(void *opaque, QEMUChrEvent event)
}
}
+static VMIntrospection *vm_introspection_object(void)
+{
+ VMIntrospectionClass *ic;
+
+ ic = VM_INTROSPECTION_CLASS(object_class_by_name(TYPE_VM_INTROSPECTION));
+
+ return ic ? ic->uniq : NULL;
+}
+
+/*
+ * This ioctl succeeds only when KVM signals the introspection tool.
+ * (the socket is connected and the event was sent without error).
+ */
+static bool signal_introspection_tool_to_unhook(VMIntrospection *i)
+{
+ int err;
+
+ err = kvm_vm_ioctl(kvm_state, KVM_INTROSPECTION_PREUNHOOK, NULL);
+
+ return !err;
+}
+
+static bool record_intercept_action(VMI_intercept_command action)
+{
+ switch (action) {
+ case VMI_INTERCEPT_SUSPEND:
+ suspend_pending = true;
+ break;
+ case VMI_INTERCEPT_RESUME:
+ suspend_pending = false;
+ break;
+ default:
+ return false;
+ }
+
+ return true;
+}
+
+static bool intercept_action(VMIntrospection *i,
+ VMI_intercept_command action, Error **errp)
+{
+ if (i->intercepted_action != VMI_INTERCEPT_NONE) {
+ error_report("VMI: unhook in progress");
+ return false;
+ }
+
+ switch (action) {
+ case VMI_INTERCEPT_RESUME:
+ enable_socket_reconnect(i);
+ return false;
+ default:
+ break;
+ }
+
+ if (!signal_introspection_tool_to_unhook(i)) {
+ disconnect_and_unhook_kvmi(i);
+ return false;
+ }
+
+ i->intercepted_action = action;
+ return true;
+}
+
+bool vm_introspection_intercept(VMI_intercept_command action, Error **errp)
+{
+ VMIntrospection *i = vm_introspection_object();
+ bool intercepted = false;
+
+ info_report("VMI: intercept command: %s",
+ action < ARRAY_SIZE(action_string)
+ ? action_string[action]
+ : "unknown");
+
+ if (record_intercept_action(action) && i) {
+ intercepted = intercept_action(i, action, errp);
+ }
+
+ info_report("VMI: intercept action: %s",
+ intercepted ? "delayed" : "continue");
+
+ return intercepted;
+}
+
static void vm_introspection_reset(void *opaque)
{
VMIntrospection *i = opaque;
diff --git a/accel/stubs/Makefile.objs b/accel/stubs/Makefile.objs
index 3894caf95d..fcec6edf0f 100644
--- a/accel/stubs/Makefile.objs
+++ b/accel/stubs/Makefile.objs
@@ -2,4 +2,5 @@ obj-$(call lnot,$(CONFIG_HAX)) += hax-stub.o
obj-$(call lnot,$(CONFIG_HVF)) += hvf-stub.o
obj-$(call lnot,$(CONFIG_WHPX)) += whpx-stub.o
obj-$(call lnot,$(CONFIG_KVM)) += kvm-stub.o
+obj-$(call lnot,$(CONFIG_KVM)) += vmi-stubs.o
obj-$(call lnot,$(CONFIG_TCG)) += tcg-stub.o
diff --git a/accel/stubs/vmi-stubs.c b/accel/stubs/vmi-stubs.c
new file mode 100644
index 0000000000..1bd93b2ca5
--- /dev/null
+++ b/accel/stubs/vmi-stubs.c
@@ -0,0 +1,7 @@
+#include "qemu/osdep.h"
+#include "sysemu/vmi-intercept.h"
+
+bool vm_introspection_intercept(VMI_intercept_command ic, Error **errp)
+{
+ return false;
+}
diff --git a/include/sysemu/vmi-intercept.h b/include/sysemu/vmi-intercept.h
new file mode 100644
index 0000000000..06998ff18a
--- /dev/null
+++ b/include/sysemu/vmi-intercept.h
@@ -0,0 +1,21 @@
+/*
+ * QEMU VM Introspection
+ *
+ * Copyright (C) 2018-2020 Bitdefender S.R.L.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef QEMU_VMI_INTERCEPT_H
+#define QEMU_VMI_INTERCEPT_H
+
+typedef enum {
+ VMI_INTERCEPT_NONE = 0,
+ VMI_INTERCEPT_SUSPEND,
+ VMI_INTERCEPT_RESUME,
+} VMI_intercept_command;
+
+bool vm_introspection_intercept(VMI_intercept_command ic, Error **errp);
+
+#endif /* QEMU_VMI_INTERCEPT_H */
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 864cbfa32e..eabd20fca3 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -39,6 +39,8 @@
#include "hw/mem/memory-device.h"
#include "hw/acpi/acpi_dev_interface.h"
+#include "sysemu/vmi-intercept.h"
+
NameInfo *qmp_query_name(Error **errp)
{
NameInfo *info = g_malloc0(sizeof(*info));
@@ -87,6 +89,9 @@ void qmp_stop(Error **errp)
if (runstate_check(RUN_STATE_INMIGRATE)) {
autostart = 0;
} else {
+ if (vm_introspection_intercept(VMI_INTERCEPT_SUSPEND, errp)) {
+ return;
+ }
vm_stop(RUN_STATE_PAUSED);
}
}
@@ -158,6 +163,11 @@ void qmp_cont(Error **errp)
autostart = 1;
} else {
vm_start();
+ /*
+ * this interception is post-event as we might need the vm to run before
+ * doing the interception, therefore we do not need the return value.
+ */
+ vm_introspection_intercept(VMI_INTERCEPT_RESUME, errp);
}
}
^ permalink raw reply related [flat|nested] 46+ messages in thread* [RFC PATCH v1 17/26] kvm: vmi: add 'unhook_timeout' property
2020-04-15 0:59 [RFC PATCH v1 00/26] VM introspection Adalbert Lazăr
` (15 preceding siblings ...)
2020-04-15 0:59 ` [RFC PATCH v1 16/26] kvm: vmi: intercept pause/resume Adalbert Lazăr
@ 2020-04-15 0:59 ` Adalbert Lazăr
2020-04-15 0:59 ` [RFC PATCH v1 18/26] kvm: vmi: store/restore 'vm_start_time' on migrate/snapshot Adalbert Lazăr
` (10 subsequent siblings)
27 siblings, 0 replies; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 0:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Adalbert Lazăr
When the introspection tool has to remove all changes made to the
introspected VM, the guest must run because some hooks can be removed only
in certain conditions. But this shouldn't take too long even with a host
under heavy load. So, if the socket is not closed by the introspection
tool at the end of this unhook process in the time specified by the
unhook_timeout property, QEMU will shutdown the socket.
Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
---
accel/kvm/vmi.c | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/accel/kvm/vmi.c b/accel/kvm/vmi.c
index 151e27265a..1f3aff3bfe 100644
--- a/accel/kvm/vmi.c
+++ b/accel/kvm/vmi.c
@@ -25,6 +25,7 @@
#include "sysemu/vmi-handshake.h"
#define HANDSHAKE_TIMEOUT_SEC 10
+#define UNHOOK_TIMEOUT_SEC 60
typedef struct VMIntrospection {
Object parent_obj;
@@ -48,6 +49,8 @@ typedef struct VMIntrospection {
uint32_t handshake_timeout;
int intercepted_action;
+ GSource *unhook_timer;
+ uint32_t unhook_timeout;
int reconnect_time;
@@ -219,6 +222,11 @@ static void instance_init(Object *obj)
object_property_add(obj, "handshake_timeout", "uint32",
prop_set_uint32, prop_get_uint32,
NULL, &i->handshake_timeout, NULL);
+
+ i->unhook_timeout = UNHOOK_TIMEOUT_SEC;
+ object_property_add(obj, "unhook_timeout", "uint32",
+ prop_set_uint32, prop_get_uint32,
+ NULL, &i->unhook_timeout, NULL);
}
static void disconnect_chardev(VMIntrospection *i)
@@ -269,6 +277,12 @@ static void cancel_handshake_timer(VMIntrospection *i)
i->hsk_timer = NULL;
}
+static void cancel_unhook_timer(VMIntrospection *i)
+{
+ cancel_timer(i->unhook_timer);
+ i->unhook_timer = NULL;
+}
+
static void instance_finalize(Object *obj)
{
VMIntrospectionClass *ic = VM_INTROSPECTION_CLASS(obj->class);
@@ -277,6 +291,7 @@ static void instance_finalize(Object *obj)
g_free(i->chardevid);
g_free(i->keyid);
+ cancel_unhook_timer(i);
cancel_handshake_timer(i);
if (i->chr) {
@@ -576,6 +591,7 @@ static void chr_event_close(VMIntrospection *i)
disconnect_and_unhook_kvmi(i);
}
+ cancel_unhook_timer(i);
cancel_handshake_timer(i);
if (suspend_pending) {
@@ -604,6 +620,19 @@ static void chr_event(void *opaque, QEMUChrEvent event)
}
}
+static gboolean unhook_timeout_cbk(gpointer opaque)
+{
+ VMIntrospection *i = opaque;
+
+ warn_report("VMI: the introspection tool is too slow");
+
+ g_source_unref(i->unhook_timer);
+ i->unhook_timer = NULL;
+
+ disconnect_and_unhook_kvmi(i);
+ return FALSE;
+}
+
static VMIntrospection *vm_introspection_object(void)
{
VMIntrospectionClass *ic;
@@ -663,6 +692,10 @@ static bool intercept_action(VMIntrospection *i,
return false;
}
+ i->unhook_timer = qemu_chr_timeout_add_ms(i->chr,
+ i->unhook_timeout * 1000,
+ unhook_timeout_cbk, i);
+
i->intercepted_action = action;
return true;
}
^ permalink raw reply related [flat|nested] 46+ messages in thread* [RFC PATCH v1 18/26] kvm: vmi: store/restore 'vm_start_time' on migrate/snapshot
2020-04-15 0:59 [RFC PATCH v1 00/26] VM introspection Adalbert Lazăr
` (16 preceding siblings ...)
2020-04-15 0:59 ` [RFC PATCH v1 17/26] kvm: vmi: add 'unhook_timeout' property Adalbert Lazăr
@ 2020-04-15 0:59 ` Adalbert Lazăr
2020-04-15 0:59 ` [RFC PATCH v1 19/26] kvm: vmi: intercept force-reset Adalbert Lazăr
` (9 subsequent siblings)
27 siblings, 0 replies; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 0:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Adalbert Lazăr
The VM start time sent during handshake can be used by the introspection
tool as a session id.
We save this 'VM start time' with the snapshot in order to be sent again
to the introspection tool when the VM is restored from snapshot and the
introspection connection is reestablished.
Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
---
accel/kvm/vmi.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/accel/kvm/vmi.c b/accel/kvm/vmi.c
index 1f3aff3bfe..e511558f3d 100644
--- a/accel/kvm/vmi.c
+++ b/accel/kvm/vmi.c
@@ -20,6 +20,7 @@
#include "crypto/hash.h"
#include "chardev/char.h"
#include "chardev/char-fe.h"
+#include "migration/vmstate.h"
#include "sysemu/vmi-intercept.h"
#include "sysemu/vmi-handshake.h"
@@ -203,6 +204,16 @@ static void class_init(ObjectClass *oc, void *data)
uc->can_be_deleted = introspection_can_be_deleted;
}
+static const VMStateDescription vmstate_introspection = {
+ .name = "vm_introspection",
+ .minimum_version_id = 1,
+ .version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_INT64(vm_start_time, VMIntrospection),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
static void instance_init(Object *obj)
{
VMIntrospectionClass *ic = VM_INTROSPECTION_CLASS(obj->class);
@@ -227,6 +238,8 @@ static void instance_init(Object *obj)
object_property_add(obj, "unhook_timeout", "uint32",
prop_set_uint32, prop_get_uint32,
NULL, &i->unhook_timeout, NULL);
+
+ vmstate_register(NULL, 0, &vmstate_introspection, i);
}
static void disconnect_chardev(VMIntrospection *i)
^ permalink raw reply related [flat|nested] 46+ messages in thread* [RFC PATCH v1 19/26] kvm: vmi: intercept force-reset
2020-04-15 0:59 [RFC PATCH v1 00/26] VM introspection Adalbert Lazăr
` (17 preceding siblings ...)
2020-04-15 0:59 ` [RFC PATCH v1 18/26] kvm: vmi: store/restore 'vm_start_time' on migrate/snapshot Adalbert Lazăr
@ 2020-04-15 0:59 ` Adalbert Lazăr
2020-04-15 0:59 ` [RFC PATCH v1 20/26] kvm: vmi: intercept live migration Adalbert Lazăr
` (8 subsequent siblings)
27 siblings, 0 replies; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 0:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Adalbert Lazăr, Marian Rotariu, Markus Armbruster
From: Marian Rotariu <marian.c.rotariu@gmail.com>
On forced reset, KVM and the instrospection tool must clean-up the
introspection structures. An important thing that must by done by KVM
is to unlink the shared memory pages (the introspection tool
can map memory pages from the introspected VM in its own process/VM).
CC: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Marian Rotariu <marian.c.rotariu@gmail.com>
Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
---
accel/kvm/vmi.c | 6 ++++++
include/sysemu/vmi-intercept.h | 2 ++
monitor/qmp-cmds.c | 4 ++++
3 files changed, 12 insertions(+)
diff --git a/accel/kvm/vmi.c b/accel/kvm/vmi.c
index e511558f3d..90906478b4 100644
--- a/accel/kvm/vmi.c
+++ b/accel/kvm/vmi.c
@@ -73,6 +73,7 @@ static const char *action_string[] = {
"none",
"suspend",
"resume",
+ "force-reset",
};
static bool suspend_pending;
@@ -677,6 +678,8 @@ static bool record_intercept_action(VMI_intercept_command action)
case VMI_INTERCEPT_RESUME:
suspend_pending = false;
break;
+ case VMI_INTERCEPT_FORCE_RESET:
+ break;
default:
return false;
}
@@ -693,6 +696,9 @@ static bool intercept_action(VMIntrospection *i,
}
switch (action) {
+ case VMI_INTERCEPT_FORCE_RESET:
+ disconnect_and_unhook_kvmi(i);
+ return false;
case VMI_INTERCEPT_RESUME:
enable_socket_reconnect(i);
return false;
diff --git a/include/sysemu/vmi-intercept.h b/include/sysemu/vmi-intercept.h
index 06998ff18a..ef591b49e7 100644
--- a/include/sysemu/vmi-intercept.h
+++ b/include/sysemu/vmi-intercept.h
@@ -14,8 +14,10 @@ typedef enum {
VMI_INTERCEPT_NONE = 0,
VMI_INTERCEPT_SUSPEND,
VMI_INTERCEPT_RESUME,
+ VMI_INTERCEPT_FORCE_RESET,
} VMI_intercept_command;
bool vm_introspection_intercept(VMI_intercept_command ic, Error **errp);
+bool vm_introspection_qmp_delay(void *mon, QObject *id, bool resume);
#endif /* QEMU_VMI_INTERCEPT_H */
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index eabd20fca3..d164635b5f 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -98,6 +98,10 @@ void qmp_stop(Error **errp)
void qmp_system_reset(Error **errp)
{
+ if (vm_introspection_intercept(VMI_INTERCEPT_FORCE_RESET, errp)) {
+ return;
+ }
+
qemu_system_reset_request(SHUTDOWN_CAUSE_HOST_QMP_SYSTEM_RESET);
}
^ permalink raw reply related [flat|nested] 46+ messages in thread* [RFC PATCH v1 20/26] kvm: vmi: intercept live migration
2020-04-15 0:59 [RFC PATCH v1 00/26] VM introspection Adalbert Lazăr
` (18 preceding siblings ...)
2020-04-15 0:59 ` [RFC PATCH v1 19/26] kvm: vmi: intercept force-reset Adalbert Lazăr
@ 2020-04-15 0:59 ` Adalbert Lazăr
2020-04-27 19:08 ` Dr. David Alan Gilbert
2020-04-15 0:59 ` [RFC PATCH v1 21/26] kvm: vmi: postpone the OK response from qmp_stop() Adalbert Lazăr
` (7 subsequent siblings)
27 siblings, 1 reply; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 0:59 UTC (permalink / raw)
To: qemu-devel
Cc: Adalbert Lazăr, Marian Rotariu, Dr. David Alan Gilbert,
Juan Quintela
From: Marian Rotariu <marian.c.rotariu@gmail.com>
It is possible that the introspection tool has made some changes inside
the introspected VM which can make the guest crash if the introspection
connection is suddenly closed.
When the live migration starts, for now, the introspection tool is
signaled to remove its hooks from the introspected VM.
CC: Juan Quintela <quintela@redhat.com>
CC: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Signed-off-by: Marian Rotariu <marian.c.rotariu@gmail.com>
Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
---
accel/kvm/vmi.c | 31 +++++++++++++++++++++++++++----
include/sysemu/vmi-intercept.h | 1 +
migration/migration.c | 18 +++++++++++++++---
migration/migration.h | 2 ++
4 files changed, 45 insertions(+), 7 deletions(-)
diff --git a/accel/kvm/vmi.c b/accel/kvm/vmi.c
index 90906478b4..ea7191e48d 100644
--- a/accel/kvm/vmi.c
+++ b/accel/kvm/vmi.c
@@ -21,6 +21,8 @@
#include "chardev/char.h"
#include "chardev/char-fe.h"
#include "migration/vmstate.h"
+#include "migration/migration.h"
+#include "migration/misc.h"
#include "sysemu/vmi-intercept.h"
#include "sysemu/vmi-handshake.h"
@@ -58,6 +60,7 @@ typedef struct VMIntrospection {
int64_t vm_start_time;
Notifier machine_ready;
+ Notifier migration_state_change;
bool created_from_command_line;
bool kvmi_hooked;
@@ -74,9 +77,11 @@ static const char *action_string[] = {
"suspend",
"resume",
"force-reset",
+ "migrate",
};
static bool suspend_pending;
+static bool migrate_pending;
#define TYPE_VM_INTROSPECTION "introspection"
@@ -88,6 +93,15 @@ static bool suspend_pending;
static Error *vm_introspection_init(VMIntrospection *i);
static void vm_introspection_reset(void *opaque);
+static void migration_state_notifier(Notifier *notifier, void *data)
+{
+ MigrationState *s = data;
+
+ if (migration_has_failed(s)) {
+ migrate_pending = false;
+ }
+}
+
static void machine_ready(Notifier *notifier, void *data)
{
VMIntrospection *i = container_of(notifier, VMIntrospection, machine_ready);
@@ -144,6 +158,9 @@ static void complete(UserCreatable *uc, Error **errp)
ic->uniq = i;
+ i->migration_state_change.notify = migration_state_notifier;
+ add_migration_state_change_notifier(&i->migration_state_change);
+
qemu_register_reset(vm_introspection_reset, i);
}
@@ -478,6 +495,9 @@ static void continue_with_the_intercepted_action(VMIntrospection *i)
case VMI_INTERCEPT_SUSPEND:
vm_stop(RUN_STATE_PAUSED);
break;
+ case VMI_INTERCEPT_MIGRATE:
+ start_live_migration_thread(migrate_get_current());
+ break;
default:
error_report("VMI: %s: unexpected action %d",
__func__, i->intercepted_action);
@@ -571,9 +591,9 @@ static void chr_event_open(VMIntrospection *i)
{
Error *local_err = NULL;
- if (suspend_pending) {
- info_report("VMI: %s: too soon (suspend=%d)",
- __func__, suspend_pending);
+ if (suspend_pending || migrate_pending) {
+ info_report("VMI: %s: too soon (suspend=%d, migrate=%d)",
+ __func__, suspend_pending, migrate_pending);
maybe_disable_socket_reconnect(i);
qemu_chr_fe_disconnect(&i->sock);
return;
@@ -608,7 +628,7 @@ static void chr_event_close(VMIntrospection *i)
cancel_unhook_timer(i);
cancel_handshake_timer(i);
- if (suspend_pending) {
+ if (suspend_pending || migrate_pending) {
maybe_disable_socket_reconnect(i);
if (i->intercepted_action != VMI_INTERCEPT_NONE) {
@@ -680,6 +700,9 @@ static bool record_intercept_action(VMI_intercept_command action)
break;
case VMI_INTERCEPT_FORCE_RESET:
break;
+ case VMI_INTERCEPT_MIGRATE:
+ migrate_pending = true;
+ break;
default:
return false;
}
diff --git a/include/sysemu/vmi-intercept.h b/include/sysemu/vmi-intercept.h
index ef591b49e7..b4a9a3faa7 100644
--- a/include/sysemu/vmi-intercept.h
+++ b/include/sysemu/vmi-intercept.h
@@ -15,6 +15,7 @@ typedef enum {
VMI_INTERCEPT_SUSPEND,
VMI_INTERCEPT_RESUME,
VMI_INTERCEPT_FORCE_RESET,
+ VMI_INTERCEPT_MIGRATE,
} VMI_intercept_command;
bool vm_introspection_intercept(VMI_intercept_command ic, Error **errp);
diff --git a/migration/migration.c b/migration/migration.c
index 187ac0410c..222037d739 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -55,6 +55,8 @@
#include "qemu/queue.h"
#include "multifd.h"
+#include "sysemu/vmi-intercept.h"
+
#define MAX_THROTTLE (32 << 20) /* Migration transfer speed throttling */
/* Amount of time to allocate to each "chunk" of bandwidth-throttled
@@ -3471,6 +3473,13 @@ static void *migration_thread(void *opaque)
return NULL;
}
+void start_live_migration_thread(MigrationState *s)
+{
+ qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
+ QEMU_THREAD_JOINABLE);
+ s->migration_thread_running = true;
+}
+
void migrate_fd_connect(MigrationState *s, Error *error_in)
{
Error *local_err = NULL;
@@ -3534,9 +3543,12 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
migrate_fd_cleanup(s);
return;
}
- qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
- QEMU_THREAD_JOINABLE);
- s->migration_thread_running = true;
+
+ if (vm_introspection_intercept(VMI_INTERCEPT_MIGRATE, &error_in)) {
+ return;
+ }
+
+ start_live_migration_thread(s);
}
void migration_global_dump(Monitor *mon)
diff --git a/migration/migration.h b/migration/migration.h
index 507284e563..eb5668e1f2 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -263,6 +263,8 @@ struct MigrationState
uint8_t clear_bitmap_shift;
};
+void start_live_migration_thread(MigrationState *s);
+
void migrate_set_state(int *state, int old_state, int new_state);
void migration_fd_process_incoming(QEMUFile *f, Error **errp);
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [RFC PATCH v1 20/26] kvm: vmi: intercept live migration
2020-04-15 0:59 ` [RFC PATCH v1 20/26] kvm: vmi: intercept live migration Adalbert Lazăr
@ 2020-04-27 19:08 ` Dr. David Alan Gilbert
2020-04-28 12:14 ` Adalbert Lazăr
0 siblings, 1 reply; 46+ messages in thread
From: Dr. David Alan Gilbert @ 2020-04-27 19:08 UTC (permalink / raw)
To: Adalbert Lazăr; +Cc: Marian Rotariu, qemu-devel, Juan Quintela
* Adalbert Lazăr (alazar@bitdefender.com) wrote:
> From: Marian Rotariu <marian.c.rotariu@gmail.com>
>
> It is possible that the introspection tool has made some changes inside
> the introspected VM which can make the guest crash if the introspection
> connection is suddenly closed.
>
> When the live migration starts, for now, the introspection tool is
> signaled to remove its hooks from the introspected VM.
>
> CC: Juan Quintela <quintela@redhat.com>
> CC: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> Signed-off-by: Marian Rotariu <marian.c.rotariu@gmail.com>
> Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
OK, so this isn't too intrusive to the migration code; and other than
renaming 'start_live_migration_thread' to
'start_outgoing_migration_thread' I think I'd be OK with this,
but it might depend what your overall aim is.
For example, you might be better intercepting each migration_state
change in your notifier, that's much finer grain than just the start of
migration.
The other thing I worry about is that there doesn't seem to be much
guard against odd orderings of things - for example, what happens
if the introspection client was to issue the INTERCEPT_MIGRATE command
twice while a migration was already running? Or before an actual
incoming channel connetion had happened?
Dave
> ---
> accel/kvm/vmi.c | 31 +++++++++++++++++++++++++++----
> include/sysemu/vmi-intercept.h | 1 +
> migration/migration.c | 18 +++++++++++++++---
> migration/migration.h | 2 ++
> 4 files changed, 45 insertions(+), 7 deletions(-)
>
> diff --git a/accel/kvm/vmi.c b/accel/kvm/vmi.c
> index 90906478b4..ea7191e48d 100644
> --- a/accel/kvm/vmi.c
> +++ b/accel/kvm/vmi.c
> @@ -21,6 +21,8 @@
> #include "chardev/char.h"
> #include "chardev/char-fe.h"
> #include "migration/vmstate.h"
> +#include "migration/migration.h"
> +#include "migration/misc.h"
>
> #include "sysemu/vmi-intercept.h"
> #include "sysemu/vmi-handshake.h"
> @@ -58,6 +60,7 @@ typedef struct VMIntrospection {
> int64_t vm_start_time;
>
> Notifier machine_ready;
> + Notifier migration_state_change;
> bool created_from_command_line;
>
> bool kvmi_hooked;
> @@ -74,9 +77,11 @@ static const char *action_string[] = {
> "suspend",
> "resume",
> "force-reset",
> + "migrate",
> };
>
> static bool suspend_pending;
> +static bool migrate_pending;
>
> #define TYPE_VM_INTROSPECTION "introspection"
>
> @@ -88,6 +93,15 @@ static bool suspend_pending;
> static Error *vm_introspection_init(VMIntrospection *i);
> static void vm_introspection_reset(void *opaque);
>
> +static void migration_state_notifier(Notifier *notifier, void *data)
> +{
> + MigrationState *s = data;
> +
> + if (migration_has_failed(s)) {
> + migrate_pending = false;
> + }
> +}
> +
> static void machine_ready(Notifier *notifier, void *data)
> {
> VMIntrospection *i = container_of(notifier, VMIntrospection, machine_ready);
> @@ -144,6 +158,9 @@ static void complete(UserCreatable *uc, Error **errp)
>
> ic->uniq = i;
>
> + i->migration_state_change.notify = migration_state_notifier;
> + add_migration_state_change_notifier(&i->migration_state_change);
> +
> qemu_register_reset(vm_introspection_reset, i);
> }
>
> @@ -478,6 +495,9 @@ static void continue_with_the_intercepted_action(VMIntrospection *i)
> case VMI_INTERCEPT_SUSPEND:
> vm_stop(RUN_STATE_PAUSED);
> break;
> + case VMI_INTERCEPT_MIGRATE:
> + start_live_migration_thread(migrate_get_current());
> + break;
> default:
> error_report("VMI: %s: unexpected action %d",
> __func__, i->intercepted_action);
> @@ -571,9 +591,9 @@ static void chr_event_open(VMIntrospection *i)
> {
> Error *local_err = NULL;
>
> - if (suspend_pending) {
> - info_report("VMI: %s: too soon (suspend=%d)",
> - __func__, suspend_pending);
> + if (suspend_pending || migrate_pending) {
> + info_report("VMI: %s: too soon (suspend=%d, migrate=%d)",
> + __func__, suspend_pending, migrate_pending);
> maybe_disable_socket_reconnect(i);
> qemu_chr_fe_disconnect(&i->sock);
> return;
> @@ -608,7 +628,7 @@ static void chr_event_close(VMIntrospection *i)
> cancel_unhook_timer(i);
> cancel_handshake_timer(i);
>
> - if (suspend_pending) {
> + if (suspend_pending || migrate_pending) {
> maybe_disable_socket_reconnect(i);
>
> if (i->intercepted_action != VMI_INTERCEPT_NONE) {
> @@ -680,6 +700,9 @@ static bool record_intercept_action(VMI_intercept_command action)
> break;
> case VMI_INTERCEPT_FORCE_RESET:
> break;
> + case VMI_INTERCEPT_MIGRATE:
> + migrate_pending = true;
> + break;
> default:
> return false;
> }
> diff --git a/include/sysemu/vmi-intercept.h b/include/sysemu/vmi-intercept.h
> index ef591b49e7..b4a9a3faa7 100644
> --- a/include/sysemu/vmi-intercept.h
> +++ b/include/sysemu/vmi-intercept.h
> @@ -15,6 +15,7 @@ typedef enum {
> VMI_INTERCEPT_SUSPEND,
> VMI_INTERCEPT_RESUME,
> VMI_INTERCEPT_FORCE_RESET,
> + VMI_INTERCEPT_MIGRATE,
> } VMI_intercept_command;
>
> bool vm_introspection_intercept(VMI_intercept_command ic, Error **errp);
> diff --git a/migration/migration.c b/migration/migration.c
> index 187ac0410c..222037d739 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -55,6 +55,8 @@
> #include "qemu/queue.h"
> #include "multifd.h"
>
> +#include "sysemu/vmi-intercept.h"
> +
> #define MAX_THROTTLE (32 << 20) /* Migration transfer speed throttling */
>
> /* Amount of time to allocate to each "chunk" of bandwidth-throttled
> @@ -3471,6 +3473,13 @@ static void *migration_thread(void *opaque)
> return NULL;
> }
>
> +void start_live_migration_thread(MigrationState *s)
> +{
> + qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
> + QEMU_THREAD_JOINABLE);
> + s->migration_thread_running = true;
> +}
> +
> void migrate_fd_connect(MigrationState *s, Error *error_in)
> {
> Error *local_err = NULL;
> @@ -3534,9 +3543,12 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
> migrate_fd_cleanup(s);
> return;
> }
> - qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
> - QEMU_THREAD_JOINABLE);
> - s->migration_thread_running = true;
> +
> + if (vm_introspection_intercept(VMI_INTERCEPT_MIGRATE, &error_in)) {
> + return;
> + }
> +
> + start_live_migration_thread(s);
> }
>
> void migration_global_dump(Monitor *mon)
> diff --git a/migration/migration.h b/migration/migration.h
> index 507284e563..eb5668e1f2 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -263,6 +263,8 @@ struct MigrationState
> uint8_t clear_bitmap_shift;
> };
>
> +void start_live_migration_thread(MigrationState *s);
> +
> void migrate_set_state(int *state, int old_state, int new_state);
>
> void migration_fd_process_incoming(QEMUFile *f, Error **errp);
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [RFC PATCH v1 20/26] kvm: vmi: intercept live migration
2020-04-27 19:08 ` Dr. David Alan Gilbert
@ 2020-04-28 12:14 ` Adalbert Lazăr
2020-04-28 12:24 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-28 12:14 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Marian Rotariu, qemu-devel, Juan Quintela
On Mon, 27 Apr 2020 20:08:55 +0100, "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Adalbert Lazăr (alazar@bitdefender.com) wrote:
> > From: Marian Rotariu <marian.c.rotariu@gmail.com>
> >
> > It is possible that the introspection tool has made some changes inside
> > the introspected VM which can make the guest crash if the introspection
> > connection is suddenly closed.
> >
> > When the live migration starts, for now, the introspection tool is
> > signaled to remove its hooks from the introspected VM.
> >
> > CC: Juan Quintela <quintela@redhat.com>
> > CC: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > Signed-off-by: Marian Rotariu <marian.c.rotariu@gmail.com>
> > Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
>
> OK, so this isn't too intrusive to the migration code; and other than
> renaming 'start_live_migration_thread' to
> 'start_outgoing_migration_thread' I think I'd be OK with this,
>
> but it might depend what your overall aim is.
>
> For example, you might be better intercepting each migration_state
> change in your notifier, that's much finer grain than just the start of
> migration.
Thank you, Dave.
We want to intercept the live migration and 'block' it while the guest
is running (some changes made to the guest by the introspection app has
to be undone while the vCPUs are in certain states).
I'm not sure what is the best way to block these kind of events
(including the pause/shutdown commands). If calling main_loop_wait()
is enough (patch [22/26] kvm: vmi: add 'async_unhook' property [1])
then we can drop a lot of code.
The use of a notifier will be nice, but from what I understand, we can't
block the migration from a notification callback.
> The other thing I worry about is that there doesn't seem to be much
> guard against odd orderings of things - for example, what happens
> if the introspection client was to issue the INTERCEPT_MIGRATE command
> twice while a migration was already running? Or before an actual
> incoming channel connetion had happened?
>
> Dave
Sorry that I haven't described the interception. When we intercept
an action that we want to 'block', we set a static variable first,
regardless if the introspection channel is connected or not, and :
- if the introspection channel is not connected we don't block the
action, but this (variable) will prevent the activation of this
channel until the action (ie. migrate) is completed (a). I assume
that there could be only one migrate (or suspend/pause) user command
at any given time (b).
- if the introspection channel is connected, the introspection app
is signaled to start its unhook/undo process. We let the code flow
continue, but the action (migrate/suspend/pause) is delayed until
the introspection channel is closed. Meanwhile, any other intercepted
action will not be blocked/delayed (c), but the fact that these actions
are in progress is saved to static variables and the introspecton
channel won't be reactivated.
Indeed, there are cases that are not handled well:
a) if the migration is started and canceled before the introspection
object is created (through QMP), the introspection channel will be
disabled until the next migration starts and finishes.
b) if a migration command has been delayed, a following migrate command
(if this is possible) won't be delayed and we will have two migration
threads started.
c) if a migration command has been delayed, a following suspend/pause
command won't be delayed and the introspection app might not have
enough time to finish its unhook/undo process.
[1]: https://lore.kernel.org/qemu-devel/20200415005938.23895-23-alazar@bitdefender.com/
> > ---
> > accel/kvm/vmi.c | 31 +++++++++++++++++++++++++++----
> > include/sysemu/vmi-intercept.h | 1 +
> > migration/migration.c | 18 +++++++++++++++---
> > migration/migration.h | 2 ++
> > 4 files changed, 45 insertions(+), 7 deletions(-)
> >
> > diff --git a/accel/kvm/vmi.c b/accel/kvm/vmi.c
> > index 90906478b4..ea7191e48d 100644
> > --- a/accel/kvm/vmi.c
> > +++ b/accel/kvm/vmi.c
> > @@ -21,6 +21,8 @@
> > #include "chardev/char.h"
> > #include "chardev/char-fe.h"
> > #include "migration/vmstate.h"
> > +#include "migration/migration.h"
> > +#include "migration/misc.h"
> >
> > #include "sysemu/vmi-intercept.h"
> > #include "sysemu/vmi-handshake.h"
> > @@ -58,6 +60,7 @@ typedef struct VMIntrospection {
> > int64_t vm_start_time;
> >
> > Notifier machine_ready;
> > + Notifier migration_state_change;
> > bool created_from_command_line;
> >
> > bool kvmi_hooked;
> > @@ -74,9 +77,11 @@ static const char *action_string[] = {
> > "suspend",
> > "resume",
> > "force-reset",
> > + "migrate",
> > };
> >
> > static bool suspend_pending;
> > +static bool migrate_pending;
> >
> > #define TYPE_VM_INTROSPECTION "introspection"
> >
> > @@ -88,6 +93,15 @@ static bool suspend_pending;
> > static Error *vm_introspection_init(VMIntrospection *i);
> > static void vm_introspection_reset(void *opaque);
> >
> > +static void migration_state_notifier(Notifier *notifier, void *data)
> > +{
> > + MigrationState *s = data;
> > +
> > + if (migration_has_failed(s)) {
> > + migrate_pending = false;
> > + }
> > +}
> > +
> > static void machine_ready(Notifier *notifier, void *data)
> > {
> > VMIntrospection *i = container_of(notifier, VMIntrospection, machine_ready);
> > @@ -144,6 +158,9 @@ static void complete(UserCreatable *uc, Error **errp)
> >
> > ic->uniq = i;
> >
> > + i->migration_state_change.notify = migration_state_notifier;
> > + add_migration_state_change_notifier(&i->migration_state_change);
> > +
> > qemu_register_reset(vm_introspection_reset, i);
> > }
> >
> > @@ -478,6 +495,9 @@ static void continue_with_the_intercepted_action(VMIntrospection *i)
> > case VMI_INTERCEPT_SUSPEND:
> > vm_stop(RUN_STATE_PAUSED);
> > break;
> > + case VMI_INTERCEPT_MIGRATE:
> > + start_live_migration_thread(migrate_get_current());
> > + break;
> > default:
> > error_report("VMI: %s: unexpected action %d",
> > __func__, i->intercepted_action);
> > @@ -571,9 +591,9 @@ static void chr_event_open(VMIntrospection *i)
> > {
> > Error *local_err = NULL;
> >
> > - if (suspend_pending) {
> > - info_report("VMI: %s: too soon (suspend=%d)",
> > - __func__, suspend_pending);
> > + if (suspend_pending || migrate_pending) {
> > + info_report("VMI: %s: too soon (suspend=%d, migrate=%d)",
> > + __func__, suspend_pending, migrate_pending);
> > maybe_disable_socket_reconnect(i);
> > qemu_chr_fe_disconnect(&i->sock);
> > return;
> > @@ -608,7 +628,7 @@ static void chr_event_close(VMIntrospection *i)
> > cancel_unhook_timer(i);
> > cancel_handshake_timer(i);
> >
> > - if (suspend_pending) {
> > + if (suspend_pending || migrate_pending) {
> > maybe_disable_socket_reconnect(i);
> >
> > if (i->intercepted_action != VMI_INTERCEPT_NONE) {
> > @@ -680,6 +700,9 @@ static bool record_intercept_action(VMI_intercept_command action)
> > break;
> > case VMI_INTERCEPT_FORCE_RESET:
> > break;
> > + case VMI_INTERCEPT_MIGRATE:
> > + migrate_pending = true;
> > + break;
> > default:
> > return false;
> > }
> > diff --git a/include/sysemu/vmi-intercept.h b/include/sysemu/vmi-intercept.h
> > index ef591b49e7..b4a9a3faa7 100644
> > --- a/include/sysemu/vmi-intercept.h
> > +++ b/include/sysemu/vmi-intercept.h
> > @@ -15,6 +15,7 @@ typedef enum {
> > VMI_INTERCEPT_SUSPEND,
> > VMI_INTERCEPT_RESUME,
> > VMI_INTERCEPT_FORCE_RESET,
> > + VMI_INTERCEPT_MIGRATE,
> > } VMI_intercept_command;
> >
> > bool vm_introspection_intercept(VMI_intercept_command ic, Error **errp);
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 187ac0410c..222037d739 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -55,6 +55,8 @@
> > #include "qemu/queue.h"
> > #include "multifd.h"
> >
> > +#include "sysemu/vmi-intercept.h"
> > +
> > #define MAX_THROTTLE (32 << 20) /* Migration transfer speed throttling */
> >
> > /* Amount of time to allocate to each "chunk" of bandwidth-throttled
> > @@ -3471,6 +3473,13 @@ static void *migration_thread(void *opaque)
> > return NULL;
> > }
> >
> > +void start_live_migration_thread(MigrationState *s)
> > +{
> > + qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
> > + QEMU_THREAD_JOINABLE);
> > + s->migration_thread_running = true;
> > +}
> > +
> > void migrate_fd_connect(MigrationState *s, Error *error_in)
> > {
> > Error *local_err = NULL;
> > @@ -3534,9 +3543,12 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
> > migrate_fd_cleanup(s);
> > return;
> > }
> > - qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
> > - QEMU_THREAD_JOINABLE);
> > - s->migration_thread_running = true;
> > +
> > + if (vm_introspection_intercept(VMI_INTERCEPT_MIGRATE, &error_in)) {
> > + return;
> > + }
> > +
> > + start_live_migration_thread(s);
> > }
> >
> > void migration_global_dump(Monitor *mon)
> > diff --git a/migration/migration.h b/migration/migration.h
> > index 507284e563..eb5668e1f2 100644
> > --- a/migration/migration.h
> > +++ b/migration/migration.h
> > @@ -263,6 +263,8 @@ struct MigrationState
> > uint8_t clear_bitmap_shift;
> > };
> >
> > +void start_live_migration_thread(MigrationState *s);
> > +
> > void migrate_set_state(int *state, int old_state, int new_state);
> >
> > void migration_fd_process_incoming(QEMUFile *f, Error **errp);
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [RFC PATCH v1 20/26] kvm: vmi: intercept live migration
2020-04-28 12:14 ` Adalbert Lazăr
@ 2020-04-28 12:24 ` Dr. David Alan Gilbert
2020-04-28 13:16 ` Adalbert Lazăr
0 siblings, 1 reply; 46+ messages in thread
From: Dr. David Alan Gilbert @ 2020-04-28 12:24 UTC (permalink / raw)
To: Adalbert Lazăr; +Cc: Marian Rotariu, qemu-devel, Juan Quintela
* Adalbert Lazăr (alazar@bitdefender.com) wrote:
> On Mon, 27 Apr 2020 20:08:55 +0100, "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > * Adalbert Lazăr (alazar@bitdefender.com) wrote:
> > > From: Marian Rotariu <marian.c.rotariu@gmail.com>
> > >
> > > It is possible that the introspection tool has made some changes inside
> > > the introspected VM which can make the guest crash if the introspection
> > > connection is suddenly closed.
> > >
> > > When the live migration starts, for now, the introspection tool is
> > > signaled to remove its hooks from the introspected VM.
> > >
> > > CC: Juan Quintela <quintela@redhat.com>
> > > CC: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > Signed-off-by: Marian Rotariu <marian.c.rotariu@gmail.com>
> > > Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
> >
> > OK, so this isn't too intrusive to the migration code; and other than
> > renaming 'start_live_migration_thread' to
> > 'start_outgoing_migration_thread' I think I'd be OK with this,
> >
> > but it might depend what your overall aim is.
> >
> > For example, you might be better intercepting each migration_state
> > change in your notifier, that's much finer grain than just the start of
> > migration.
>
> Thank you, Dave.
>
> We want to intercept the live migration and 'block' it while the guest
> is running (some changes made to the guest by the introspection app has
> to be undone while the vCPUs are in certain states).
>
> I'm not sure what is the best way to block these kind of events
> (including the pause/shutdown commands). If calling main_loop_wait()
> is enough (patch [22/26] kvm: vmi: add 'async_unhook' property [1])
> then we can drop a lot of code.
>
> The use of a notifier will be nice, but from what I understand, we can't
> block the migration from a notification callback.
Oh, if your intention is *just* to block a migration starting then you
can use 'migrate_add_blocker' - see hw/9pfs/9p.c for an example where
it's used and then removed; they use it to stop migration while the fs
is mounted. That causes an attempt to start a migration to give an
error (of your choosing).
> > The other thing I worry about is that there doesn't seem to be much
> > guard against odd orderings of things - for example, what happens
> > if the introspection client was to issue the INTERCEPT_MIGRATE command
> > twice while a migration was already running? Or before an actual
> > incoming channel connetion had happened?
> >
> > Dave
>
> Sorry that I haven't described the interception. When we intercept
> an action that we want to 'block', we set a static variable first,
> regardless if the introspection channel is connected or not, and :
>
> - if the introspection channel is not connected we don't block the
> action, but this (variable) will prevent the activation of this
> channel until the action (ie. migrate) is completed (a). I assume
> that there could be only one migrate (or suspend/pause) user command
> at any given time (b).
>
> - if the introspection channel is connected, the introspection app
> is signaled to start its unhook/undo process. We let the code flow
> continue, but the action (migrate/suspend/pause) is delayed until
> the introspection channel is closed. Meanwhile, any other intercepted
> action will not be blocked/delayed (c), but the fact that these actions
> are in progress is saved to static variables and the introspecton
> channel won't be reactivated.
>
> Indeed, there are cases that are not handled well:
>
> a) if the migration is started and canceled before the introspection
> object is created (through QMP), the introspection channel will be
> disabled until the next migration starts and finishes.
>
> b) if a migration command has been delayed, a following migrate command
> (if this is possible) won't be delayed and we will have two migration
> threads started.
>
> c) if a migration command has been delayed, a following suspend/pause
> command won't be delayed and the introspection app might not have
> enough time to finish its unhook/undo process.
Yeh that sounds a bit messy.
Dave
> [1]: https://lore.kernel.org/qemu-devel/20200415005938.23895-23-alazar@bitdefender.com/
>
> > > ---
> > > accel/kvm/vmi.c | 31 +++++++++++++++++++++++++++----
> > > include/sysemu/vmi-intercept.h | 1 +
> > > migration/migration.c | 18 +++++++++++++++---
> > > migration/migration.h | 2 ++
> > > 4 files changed, 45 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/accel/kvm/vmi.c b/accel/kvm/vmi.c
> > > index 90906478b4..ea7191e48d 100644
> > > --- a/accel/kvm/vmi.c
> > > +++ b/accel/kvm/vmi.c
> > > @@ -21,6 +21,8 @@
> > > #include "chardev/char.h"
> > > #include "chardev/char-fe.h"
> > > #include "migration/vmstate.h"
> > > +#include "migration/migration.h"
> > > +#include "migration/misc.h"
> > >
> > > #include "sysemu/vmi-intercept.h"
> > > #include "sysemu/vmi-handshake.h"
> > > @@ -58,6 +60,7 @@ typedef struct VMIntrospection {
> > > int64_t vm_start_time;
> > >
> > > Notifier machine_ready;
> > > + Notifier migration_state_change;
> > > bool created_from_command_line;
> > >
> > > bool kvmi_hooked;
> > > @@ -74,9 +77,11 @@ static const char *action_string[] = {
> > > "suspend",
> > > "resume",
> > > "force-reset",
> > > + "migrate",
> > > };
> > >
> > > static bool suspend_pending;
> > > +static bool migrate_pending;
> > >
> > > #define TYPE_VM_INTROSPECTION "introspection"
> > >
> > > @@ -88,6 +93,15 @@ static bool suspend_pending;
> > > static Error *vm_introspection_init(VMIntrospection *i);
> > > static void vm_introspection_reset(void *opaque);
> > >
> > > +static void migration_state_notifier(Notifier *notifier, void *data)
> > > +{
> > > + MigrationState *s = data;
> > > +
> > > + if (migration_has_failed(s)) {
> > > + migrate_pending = false;
> > > + }
> > > +}
> > > +
> > > static void machine_ready(Notifier *notifier, void *data)
> > > {
> > > VMIntrospection *i = container_of(notifier, VMIntrospection, machine_ready);
> > > @@ -144,6 +158,9 @@ static void complete(UserCreatable *uc, Error **errp)
> > >
> > > ic->uniq = i;
> > >
> > > + i->migration_state_change.notify = migration_state_notifier;
> > > + add_migration_state_change_notifier(&i->migration_state_change);
> > > +
> > > qemu_register_reset(vm_introspection_reset, i);
> > > }
> > >
> > > @@ -478,6 +495,9 @@ static void continue_with_the_intercepted_action(VMIntrospection *i)
> > > case VMI_INTERCEPT_SUSPEND:
> > > vm_stop(RUN_STATE_PAUSED);
> > > break;
> > > + case VMI_INTERCEPT_MIGRATE:
> > > + start_live_migration_thread(migrate_get_current());
> > > + break;
> > > default:
> > > error_report("VMI: %s: unexpected action %d",
> > > __func__, i->intercepted_action);
> > > @@ -571,9 +591,9 @@ static void chr_event_open(VMIntrospection *i)
> > > {
> > > Error *local_err = NULL;
> > >
> > > - if (suspend_pending) {
> > > - info_report("VMI: %s: too soon (suspend=%d)",
> > > - __func__, suspend_pending);
> > > + if (suspend_pending || migrate_pending) {
> > > + info_report("VMI: %s: too soon (suspend=%d, migrate=%d)",
> > > + __func__, suspend_pending, migrate_pending);
> > > maybe_disable_socket_reconnect(i);
> > > qemu_chr_fe_disconnect(&i->sock);
> > > return;
> > > @@ -608,7 +628,7 @@ static void chr_event_close(VMIntrospection *i)
> > > cancel_unhook_timer(i);
> > > cancel_handshake_timer(i);
> > >
> > > - if (suspend_pending) {
> > > + if (suspend_pending || migrate_pending) {
> > > maybe_disable_socket_reconnect(i);
> > >
> > > if (i->intercepted_action != VMI_INTERCEPT_NONE) {
> > > @@ -680,6 +700,9 @@ static bool record_intercept_action(VMI_intercept_command action)
> > > break;
> > > case VMI_INTERCEPT_FORCE_RESET:
> > > break;
> > > + case VMI_INTERCEPT_MIGRATE:
> > > + migrate_pending = true;
> > > + break;
> > > default:
> > > return false;
> > > }
> > > diff --git a/include/sysemu/vmi-intercept.h b/include/sysemu/vmi-intercept.h
> > > index ef591b49e7..b4a9a3faa7 100644
> > > --- a/include/sysemu/vmi-intercept.h
> > > +++ b/include/sysemu/vmi-intercept.h
> > > @@ -15,6 +15,7 @@ typedef enum {
> > > VMI_INTERCEPT_SUSPEND,
> > > VMI_INTERCEPT_RESUME,
> > > VMI_INTERCEPT_FORCE_RESET,
> > > + VMI_INTERCEPT_MIGRATE,
> > > } VMI_intercept_command;
> > >
> > > bool vm_introspection_intercept(VMI_intercept_command ic, Error **errp);
> > > diff --git a/migration/migration.c b/migration/migration.c
> > > index 187ac0410c..222037d739 100644
> > > --- a/migration/migration.c
> > > +++ b/migration/migration.c
> > > @@ -55,6 +55,8 @@
> > > #include "qemu/queue.h"
> > > #include "multifd.h"
> > >
> > > +#include "sysemu/vmi-intercept.h"
> > > +
> > > #define MAX_THROTTLE (32 << 20) /* Migration transfer speed throttling */
> > >
> > > /* Amount of time to allocate to each "chunk" of bandwidth-throttled
> > > @@ -3471,6 +3473,13 @@ static void *migration_thread(void *opaque)
> > > return NULL;
> > > }
> > >
> > > +void start_live_migration_thread(MigrationState *s)
> > > +{
> > > + qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
> > > + QEMU_THREAD_JOINABLE);
> > > + s->migration_thread_running = true;
> > > +}
> > > +
> > > void migrate_fd_connect(MigrationState *s, Error *error_in)
> > > {
> > > Error *local_err = NULL;
> > > @@ -3534,9 +3543,12 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
> > > migrate_fd_cleanup(s);
> > > return;
> > > }
> > > - qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
> > > - QEMU_THREAD_JOINABLE);
> > > - s->migration_thread_running = true;
> > > +
> > > + if (vm_introspection_intercept(VMI_INTERCEPT_MIGRATE, &error_in)) {
> > > + return;
> > > + }
> > > +
> > > + start_live_migration_thread(s);
> > > }
> > >
> > > void migration_global_dump(Monitor *mon)
> > > diff --git a/migration/migration.h b/migration/migration.h
> > > index 507284e563..eb5668e1f2 100644
> > > --- a/migration/migration.h
> > > +++ b/migration/migration.h
> > > @@ -263,6 +263,8 @@ struct MigrationState
> > > uint8_t clear_bitmap_shift;
> > > };
> > >
> > > +void start_live_migration_thread(MigrationState *s);
> > > +
> > > void migrate_set_state(int *state, int old_state, int new_state);
> > >
> > > void migration_fd_process_incoming(QEMUFile *f, Error **errp);
> > >
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [RFC PATCH v1 20/26] kvm: vmi: intercept live migration
2020-04-28 12:24 ` Dr. David Alan Gilbert
@ 2020-04-28 13:16 ` Adalbert Lazăr
2020-04-28 13:43 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-28 13:16 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Marian Rotariu, qemu-devel, Juan Quintela
On Tue, 28 Apr 2020 13:24:39 +0100, "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Adalbert Lazăr (alazar@bitdefender.com) wrote:
> > On Mon, 27 Apr 2020 20:08:55 +0100, "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > > * Adalbert Lazăr (alazar@bitdefender.com) wrote:
> > > > From: Marian Rotariu <marian.c.rotariu@gmail.com>
> > > >
> > > > It is possible that the introspection tool has made some changes inside
> > > > the introspected VM which can make the guest crash if the introspection
> > > > connection is suddenly closed.
> > > >
> > > > When the live migration starts, for now, the introspection tool is
> > > > signaled to remove its hooks from the introspected VM.
> > > >
> > > > CC: Juan Quintela <quintela@redhat.com>
> > > > CC: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > > Signed-off-by: Marian Rotariu <marian.c.rotariu@gmail.com>
> > > > Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
> > >
> > > OK, so this isn't too intrusive to the migration code; and other than
> > > renaming 'start_live_migration_thread' to
> > > 'start_outgoing_migration_thread' I think I'd be OK with this,
> > >
> > > but it might depend what your overall aim is.
> > >
> > > For example, you might be better intercepting each migration_state
> > > change in your notifier, that's much finer grain than just the start of
> > > migration.
> >
> > Thank you, Dave.
> >
> > We want to intercept the live migration and 'block' it while the guest
> > is running (some changes made to the guest by the introspection app has
> > to be undone while the vCPUs are in certain states).
> >
> > I'm not sure what is the best way to block these kind of events
> > (including the pause/shutdown commands). If calling main_loop_wait()
> > is enough (patch [22/26] kvm: vmi: add 'async_unhook' property [1])
> > then we can drop a lot of code.
> >
> > The use of a notifier will be nice, but from what I understand, we can't
> > block the migration from a notification callback.
>
> Oh, if your intention is *just* to block a migration starting then you
> can use 'migrate_add_blocker' - see hw/9pfs/9p.c for an example where
> it's used and then removed; they use it to stop migration while the fs
> is mounted. That causes an attempt to start a migration to give an
> error (of your choosing).
One use case is to do VM introspection all the time the guest is running.
From the user perspective, the pause/suspend/shutdown/snapshot/migrate
commands should work regardless if the VM is currently introspected
or not. Our first option was to delay these commands for a couple of
seconds when the VM is introspected, while the introspection app reverts
its changes, without blocking the vCPUs.
I'll see if we can mix the migrate notifier with migrate_add_blocker(),
or add a new migration state. To block the migration (with an error)
is our second option, because the user doing this might not be allowed
to stop the VM introspection.
Thank you,
Adalbert
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH v1 20/26] kvm: vmi: intercept live migration
2020-04-28 13:16 ` Adalbert Lazăr
@ 2020-04-28 13:43 ` Dr. David Alan Gilbert
2020-04-28 14:38 ` Adalbert Lazăr
0 siblings, 1 reply; 46+ messages in thread
From: Dr. David Alan Gilbert @ 2020-04-28 13:43 UTC (permalink / raw)
To: Adalbert Lazăr; +Cc: Marian Rotariu, qemu-devel, Juan Quintela
* Adalbert Lazăr (alazar@bitdefender.com) wrote:
> On Tue, 28 Apr 2020 13:24:39 +0100, "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > * Adalbert Lazăr (alazar@bitdefender.com) wrote:
> > > On Mon, 27 Apr 2020 20:08:55 +0100, "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > > > * Adalbert Lazăr (alazar@bitdefender.com) wrote:
> > > > > From: Marian Rotariu <marian.c.rotariu@gmail.com>
> > > > >
> > > > > It is possible that the introspection tool has made some changes inside
> > > > > the introspected VM which can make the guest crash if the introspection
> > > > > connection is suddenly closed.
> > > > >
> > > > > When the live migration starts, for now, the introspection tool is
> > > > > signaled to remove its hooks from the introspected VM.
> > > > >
> > > > > CC: Juan Quintela <quintela@redhat.com>
> > > > > CC: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > > > Signed-off-by: Marian Rotariu <marian.c.rotariu@gmail.com>
> > > > > Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
> > > >
> > > > OK, so this isn't too intrusive to the migration code; and other than
> > > > renaming 'start_live_migration_thread' to
> > > > 'start_outgoing_migration_thread' I think I'd be OK with this,
> > > >
> > > > but it might depend what your overall aim is.
> > > >
> > > > For example, you might be better intercepting each migration_state
> > > > change in your notifier, that's much finer grain than just the start of
> > > > migration.
> > >
> > > Thank you, Dave.
> > >
> > > We want to intercept the live migration and 'block' it while the guest
> > > is running (some changes made to the guest by the introspection app has
> > > to be undone while the vCPUs are in certain states).
> > >
> > > I'm not sure what is the best way to block these kind of events
> > > (including the pause/shutdown commands). If calling main_loop_wait()
> > > is enough (patch [22/26] kvm: vmi: add 'async_unhook' property [1])
> > > then we can drop a lot of code.
> > >
> > > The use of a notifier will be nice, but from what I understand, we can't
> > > block the migration from a notification callback.
> >
> > Oh, if your intention is *just* to block a migration starting then you
> > can use 'migrate_add_blocker' - see hw/9pfs/9p.c for an example where
> > it's used and then removed; they use it to stop migration while the fs
> > is mounted. That causes an attempt to start a migration to give an
> > error (of your choosing).
>
> One use case is to do VM introspection all the time the guest is running.
> From the user perspective, the pause/suspend/shutdown/snapshot/migrate
> commands should work regardless if the VM is currently introspected
> or not. Our first option was to delay these commands for a couple of
> seconds when the VM is introspected, while the introspection app reverts
> its changes, without blocking the vCPUs.
Ah OK, so it's not really about blocking it completely; just delaying it
a bit; in that case add_blocker is the wrong thing.
> I'll see if we can mix the migrate notifier with migrate_add_blocker(),
> or add a new migration state. To block the migration (with an error)
> is our second option, because the user doing this might not be allowed
> to stop the VM introspection.
Maybe the right thing is to do something just like
MIGRATION_STATUS_WAIT_UNPLUG, it's right near the start of the thread.
Again it's job is just to make the migration wait while it does some
stuff before it can let migration continue.
Dave
> Thank you,
> Adalbert
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH v1 20/26] kvm: vmi: intercept live migration
2020-04-28 13:43 ` Dr. David Alan Gilbert
@ 2020-04-28 14:38 ` Adalbert Lazăr
0 siblings, 0 replies; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-28 14:38 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Marian Rotariu, qemu-devel, Juan Quintela
On Tue, 28 Apr 2020 14:43:20 +0100, "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Adalbert Lazăr (alazar@bitdefender.com) wrote:
> > One use case is to do VM introspection all the time the guest is running.
> > From the user perspective, the pause/suspend/shutdown/snapshot/migrate
> > commands should work regardless if the VM is currently introspected
> > or not. Our first option was to delay these commands for a couple of
> > seconds when the VM is introspected, while the introspection app reverts
> > its changes, without blocking the vCPUs.
>
> Ah OK, so it's not really about blocking it completely; just delaying it
> a bit; in that case add_blocker is the wrong thing.
>
> > I'll see if we can mix the migrate notifier with migrate_add_blocker(),
> > or add a new migration state. To block the migration (with an error)
> > is our second option, because the user doing this might not be allowed
> > to stop the VM introspection.
>
> Maybe the right thing is to do something just like
> MIGRATION_STATUS_WAIT_UNPLUG, it's right near the start of the thread.
> Again it's job is just to make the migration wait while it does some
> stuff before it can let migration continue.
>
This is it! Thank you, Dave.
We already register a VMStateDescription structure to save the VM start time
([18/26] kvm: vmi: store/restore 'vm_start_time' on migrate/snapshot [1]).
All we have to do is setup the dev_unplug_pending callback and
return true when the introspection channel is still active.
[1]: https://lore.kernel.org/qemu-devel/20200415005938.23895-19-alazar@bitdefender.com/
^ permalink raw reply [flat|nested] 46+ messages in thread
* [RFC PATCH v1 21/26] kvm: vmi: postpone the OK response from qmp_stop()
2020-04-15 0:59 [RFC PATCH v1 00/26] VM introspection Adalbert Lazăr
` (19 preceding siblings ...)
2020-04-15 0:59 ` [RFC PATCH v1 20/26] kvm: vmi: intercept live migration Adalbert Lazăr
@ 2020-04-15 0:59 ` Adalbert Lazăr
2020-04-15 0:59 ` [RFC PATCH v1 22/26] kvm: vmi: add 'async_unhook' property Adalbert Lazăr
` (6 subsequent siblings)
27 siblings, 0 replies; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 0:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Adalbert Lazăr, Markus Armbruster
The method to postpone the intercepted command (pause/suspend/migrate)
until the introspection tool has the chance to remove its hooks
(e.g. breakpoints) from guest doesn't work on snapshot+memory (at
least as it is done by libvirt/virt-manager 1.3.1). The sequence
qmp_stop()+save_vm+qmp_cont() doesn't wait for the STOP event. save_vm()
is called right after qmp_stop() returns OK. What we do is postpone
this OK response until the introspection tools finishes the unhook
process.
CC: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
---
accel/kvm/vmi.c | 29 +++++++++++++++++++++++++++++
accel/stubs/vmi-stubs.c | 7 +++++++
include/monitor/monitor.h | 1 +
include/sysemu/vmi-intercept.h | 2 +-
monitor/Makefile.objs | 2 +-
monitor/qmp.c | 11 +++++++++++
monitor/stubs.c | 9 +++++++++
7 files changed, 59 insertions(+), 2 deletions(-)
create mode 100644 monitor/stubs.c
diff --git a/accel/kvm/vmi.c b/accel/kvm/vmi.c
index ea7191e48d..01034d460e 100644
--- a/accel/kvm/vmi.c
+++ b/accel/kvm/vmi.c
@@ -10,6 +10,7 @@
#include "qemu/osdep.h"
#include "qemu-common.h"
#include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
#include "qemu/error-report.h"
#include "qom/object_interfaces.h"
#include "sysemu/sysemu.h"
@@ -23,6 +24,8 @@
#include "migration/vmstate.h"
#include "migration/migration.h"
#include "migration/misc.h"
+#include "qapi/qmp/qobject.h"
+#include "monitor/monitor.h"
#include "sysemu/vmi-intercept.h"
#include "sysemu/vmi-handshake.h"
@@ -63,6 +66,9 @@ typedef struct VMIntrospection {
Notifier migration_state_change;
bool created_from_command_line;
+ void *qmp_monitor;
+ QDict *qmp_rsp;
+
bool kvmi_hooked;
} VMIntrospection;
@@ -333,6 +339,8 @@ static void instance_finalize(Object *obj)
error_free(i->init_error);
+ qobject_unref(i->qmp_rsp);
+
ic->instance_counter--;
if (!ic->instance_counter) {
ic->uniq = NULL;
@@ -506,6 +514,12 @@ static void continue_with_the_intercepted_action(VMIntrospection *i)
info_report("VMI: continue with '%s'",
action_string[i->intercepted_action]);
+
+ if (i->qmp_rsp) {
+ monitor_qmp_respond_later(i->qmp_monitor, i->qmp_rsp);
+ i->qmp_monitor = NULL;
+ i->qmp_rsp = NULL;
+ }
}
/*
@@ -676,6 +690,21 @@ static VMIntrospection *vm_introspection_object(void)
return ic ? ic->uniq : NULL;
}
+bool vm_introspection_qmp_delay(void *mon, QDict *rsp)
+{
+ VMIntrospection *i = vm_introspection_object();
+ bool intercepted;
+
+ intercepted = i && i->intercepted_action == VMI_INTERCEPT_SUSPEND;
+
+ if (intercepted) {
+ i->qmp_monitor = mon;
+ i->qmp_rsp = rsp;
+ }
+
+ return intercepted;
+}
+
/*
* This ioctl succeeds only when KVM signals the introspection tool.
* (the socket is connected and the event was sent without error).
diff --git a/accel/stubs/vmi-stubs.c b/accel/stubs/vmi-stubs.c
index 1bd93b2ca5..0cb1d6572b 100644
--- a/accel/stubs/vmi-stubs.c
+++ b/accel/stubs/vmi-stubs.c
@@ -1,7 +1,14 @@
#include "qemu/osdep.h"
+#include "qapi/qmp/qdict.h"
+
#include "sysemu/vmi-intercept.h"
bool vm_introspection_intercept(VMI_intercept_command ic, Error **errp)
{
return false;
}
+
+bool vm_introspection_qmp_delay(void *mon, QDict *rsp)
+{
+ return false;
+}
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 1018d754a6..1b3debc635 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -47,5 +47,6 @@ int monitor_fdset_get_fd(int64_t fdset_id, int flags);
int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
void monitor_fdset_dup_fd_remove(int dup_fd);
int64_t monitor_fdset_dup_fd_find(int dup_fd);
+void monitor_qmp_respond_later(void *_mon, QDict *rsp);
#endif /* MONITOR_H */
diff --git a/include/sysemu/vmi-intercept.h b/include/sysemu/vmi-intercept.h
index b4a9a3faa7..4b93d17f2b 100644
--- a/include/sysemu/vmi-intercept.h
+++ b/include/sysemu/vmi-intercept.h
@@ -19,6 +19,6 @@ typedef enum {
} VMI_intercept_command;
bool vm_introspection_intercept(VMI_intercept_command ic, Error **errp);
-bool vm_introspection_qmp_delay(void *mon, QObject *id, bool resume);
+bool vm_introspection_qmp_delay(void *mon, QDict *rsp);
#endif /* QEMU_VMI_INTERCEPT_H */
diff --git a/monitor/Makefile.objs b/monitor/Makefile.objs
index a8533c9dd7..16652ed162 100644
--- a/monitor/Makefile.objs
+++ b/monitor/Makefile.objs
@@ -3,4 +3,4 @@ common-obj-y += monitor.o qmp.o hmp.o
common-obj-y += qmp-cmds.o qmp-cmds-control.o
common-obj-y += hmp-cmds.o
-storage-daemon-obj-y += monitor.o qmp.o qmp-cmds-control.o
+storage-daemon-obj-y += monitor.o qmp.o qmp-cmds-control.o stubs.o
diff --git a/monitor/qmp.c b/monitor/qmp.c
index f89e7daf27..fc9ea7eafa 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -32,6 +32,7 @@
#include "qapi/qmp/qjson.h"
#include "qapi/qmp/qlist.h"
#include "qapi/qmp/qstring.h"
+#include "sysemu/vmi-intercept.h"
#include "trace.h"
struct QMPRequest {
@@ -158,6 +159,16 @@ static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req)
}
}
+ if (!vm_introspection_qmp_delay(mon, rsp)) {
+ monitor_qmp_respond(mon, rsp);
+ qobject_unref(rsp);
+ }
+}
+
+void monitor_qmp_respond_later(void *_mon, QDict *rsp)
+{
+ MonitorQMP *mon = _mon;
+
monitor_qmp_respond(mon, rsp);
qobject_unref(rsp);
}
diff --git a/monitor/stubs.c b/monitor/stubs.c
new file mode 100644
index 0000000000..fc5707ae13
--- /dev/null
+++ b/monitor/stubs.c
@@ -0,0 +1,9 @@
+#include "qemu/osdep.h"
+#include "qapi/qmp/qdict.h"
+
+#include "sysemu/vmi-intercept.h"
+
+bool vm_introspection_qmp_delay(void *mon, QDict *rsp)
+{
+ return false;
+}
^ permalink raw reply related [flat|nested] 46+ messages in thread* [RFC PATCH v1 22/26] kvm: vmi: add 'async_unhook' property
2020-04-15 0:59 [RFC PATCH v1 00/26] VM introspection Adalbert Lazăr
` (20 preceding siblings ...)
2020-04-15 0:59 ` [RFC PATCH v1 21/26] kvm: vmi: postpone the OK response from qmp_stop() Adalbert Lazăr
@ 2020-04-15 0:59 ` Adalbert Lazăr
2020-04-15 0:59 ` [RFC PATCH v1 23/26] kvm: vmi: intercept shutdown Adalbert Lazăr
` (5 subsequent siblings)
27 siblings, 0 replies; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 0:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Adalbert Lazăr
The default method to handle the intercepted commands
(pause/suspend/migrate) might not be the simplest method. We add an
alternative method, used when async_unhook is set to false, that runs
the main loop until the introspection tool finish the unhook process
and closes the introspection socket.
Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
---
accel/kvm/vmi.c | 38 ++++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/accel/kvm/vmi.c b/accel/kvm/vmi.c
index 01034d460e..bee9798e54 100644
--- a/accel/kvm/vmi.c
+++ b/accel/kvm/vmi.c
@@ -57,6 +57,7 @@ typedef struct VMIntrospection {
int intercepted_action;
GSource *unhook_timer;
uint32_t unhook_timeout;
+ bool async_unhook;
int reconnect_time;
@@ -186,6 +187,20 @@ static void prop_set_key(Object *obj, const char *value, Error **errp)
i->keyid = g_strdup(value);
}
+static bool prop_get_async_unhook(Object *obj, Error **errp)
+{
+ VMIntrospection *i = VM_INTROSPECTION(obj);
+
+ return i->async_unhook;
+}
+
+static void prop_set_async_unhook(Object *obj, bool value, Error **errp)
+{
+ VMIntrospection *i = VM_INTROSPECTION(obj);
+
+ i->async_unhook = value;
+}
+
static void prop_get_uint32(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
@@ -263,6 +278,11 @@ static void instance_init(Object *obj)
prop_set_uint32, prop_get_uint32,
NULL, &i->unhook_timeout, NULL);
+ i->async_unhook = true;
+ object_property_add_bool(obj, "async_unhook",
+ prop_get_async_unhook,
+ prop_set_async_unhook, NULL);
+
vmstate_register(NULL, 0, &vmstate_introspection, i);
}
@@ -739,6 +759,19 @@ static bool record_intercept_action(VMI_intercept_command action)
return true;
}
+static void wait_until_the_socket_is_closed(VMIntrospection *i)
+{
+ info_report("VMI: start waiting until fd=%d is closed", i->sock_fd);
+
+ while (i->sock_fd != -1) {
+ main_loop_wait(false);
+ }
+
+ info_report("VMI: continue with the intercepted action fd=%d", i->sock_fd);
+
+ maybe_disable_socket_reconnect(i);
+}
+
static bool intercept_action(VMIntrospection *i,
VMI_intercept_command action, Error **errp)
{
@@ -767,6 +800,11 @@ static bool intercept_action(VMIntrospection *i,
i->unhook_timeout * 1000,
unhook_timeout_cbk, i);
+ if (!i->async_unhook) {
+ wait_until_the_socket_is_closed(i);
+ return false;
+ }
+
i->intercepted_action = action;
return true;
}
^ permalink raw reply related [flat|nested] 46+ messages in thread* [RFC PATCH v1 23/26] kvm: vmi: intercept shutdown
2020-04-15 0:59 [RFC PATCH v1 00/26] VM introspection Adalbert Lazăr
` (21 preceding siblings ...)
2020-04-15 0:59 ` [RFC PATCH v1 22/26] kvm: vmi: add 'async_unhook' property Adalbert Lazăr
@ 2020-04-15 0:59 ` Adalbert Lazăr
2020-04-15 0:59 ` [RFC PATCH v1 24/26] kvm: vmi: add 'unhook_on_shutdown' property Adalbert Lazăr
` (4 subsequent siblings)
27 siblings, 0 replies; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 0:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Adalbert Lazăr, Marian Rotariu, Markus Armbruster
From: Marian Rotariu <marian.c.rotariu@gmail.com>
On shutdown, it is desirable that the introspection tool removes
its changes from the introspected VM, so that they don't reach the
hibernation file.
CC: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Marian Rotariu <marian.c.rotariu@gmail.com>
Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
---
accel/kvm/vmi.c | 31 +++++++++++++++++++++++++++----
include/sysemu/vmi-intercept.h | 1 +
monitor/qmp-cmds.c | 4 ++++
3 files changed, 32 insertions(+), 4 deletions(-)
diff --git a/accel/kvm/vmi.c b/accel/kvm/vmi.c
index bee9798e54..2c6981a4bf 100644
--- a/accel/kvm/vmi.c
+++ b/accel/kvm/vmi.c
@@ -85,10 +85,12 @@ static const char *action_string[] = {
"resume",
"force-reset",
"migrate",
+ "shutdown",
};
static bool suspend_pending;
static bool migrate_pending;
+static bool shutdown_pending;
#define TYPE_VM_INTROSPECTION "introspection"
@@ -511,6 +513,17 @@ static void enable_socket_reconnect(VMIntrospection *i)
static void maybe_disable_socket_reconnect(VMIntrospection *i)
{
+ if (shutdown_pending) {
+ /*
+ * We've got the shutdown notification, but the guest might not stop.
+ * We already caused the introspection tool to unhook
+ * because shutdown_pending was set.
+ * Let the socket connect again just in case the guest doesn't stop.
+ */
+ shutdown_pending = false;
+ return;
+ }
+
if (i->reconnect_time == 0) {
info_report("VMI: disable socket reconnect");
i->reconnect_time = qemu_chr_fe_reconnect_time(&i->sock, 0);
@@ -526,6 +539,9 @@ static void continue_with_the_intercepted_action(VMIntrospection *i)
case VMI_INTERCEPT_MIGRATE:
start_live_migration_thread(migrate_get_current());
break;
+ case VMI_INTERCEPT_SHUTDOWN:
+ qemu_system_powerdown_request();
+ break;
default:
error_report("VMI: %s: unexpected action %d",
__func__, i->intercepted_action);
@@ -625,9 +641,10 @@ static void chr_event_open(VMIntrospection *i)
{
Error *local_err = NULL;
- if (suspend_pending || migrate_pending) {
- info_report("VMI: %s: too soon (suspend=%d, migrate=%d)",
- __func__, suspend_pending, migrate_pending);
+ if (suspend_pending || migrate_pending || shutdown_pending) {
+ info_report("VMI: %s: too soon (suspend=%d, migrate=%d, shutdown=%d)",
+ __func__, suspend_pending, migrate_pending,
+ shutdown_pending);
maybe_disable_socket_reconnect(i);
qemu_chr_fe_disconnect(&i->sock);
return;
@@ -662,7 +679,7 @@ static void chr_event_close(VMIntrospection *i)
cancel_unhook_timer(i);
cancel_handshake_timer(i);
- if (suspend_pending || migrate_pending) {
+ if (suspend_pending || migrate_pending || shutdown_pending) {
maybe_disable_socket_reconnect(i);
if (i->intercepted_action != VMI_INTERCEPT_NONE) {
@@ -752,6 +769,9 @@ static bool record_intercept_action(VMI_intercept_command action)
case VMI_INTERCEPT_MIGRATE:
migrate_pending = true;
break;
+ case VMI_INTERCEPT_SHUTDOWN:
+ shutdown_pending = true;
+ break;
default:
return false;
}
@@ -839,6 +859,9 @@ static void vm_introspection_reset(void *opaque)
}
update_vm_start_time(i);
+
+ /* warm reset triggered by user */
+ shutdown_pending = false;
}
static bool make_cookie_hash(const char *key_id, uint8_t *cookie_hash,
diff --git a/include/sysemu/vmi-intercept.h b/include/sysemu/vmi-intercept.h
index 4b93d17f2b..da086d7a04 100644
--- a/include/sysemu/vmi-intercept.h
+++ b/include/sysemu/vmi-intercept.h
@@ -16,6 +16,7 @@ typedef enum {
VMI_INTERCEPT_RESUME,
VMI_INTERCEPT_FORCE_RESET,
VMI_INTERCEPT_MIGRATE,
+ VMI_INTERCEPT_SHUTDOWN,
} VMI_intercept_command;
bool vm_introspection_intercept(VMI_intercept_command ic, Error **errp);
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index d164635b5f..333a4a0ecc 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -107,6 +107,10 @@ void qmp_system_reset(Error **errp)
void qmp_system_powerdown(Error **errp)
{
+ if (vm_introspection_intercept(VMI_INTERCEPT_SHUTDOWN, errp)) {
+ return;
+ }
+
qemu_system_powerdown_request();
}
^ permalink raw reply related [flat|nested] 46+ messages in thread* [RFC PATCH v1 24/26] kvm: vmi: add 'unhook_on_shutdown' property
2020-04-15 0:59 [RFC PATCH v1 00/26] VM introspection Adalbert Lazăr
` (22 preceding siblings ...)
2020-04-15 0:59 ` [RFC PATCH v1 23/26] kvm: vmi: intercept shutdown Adalbert Lazăr
@ 2020-04-15 0:59 ` Adalbert Lazăr
2020-04-15 0:59 ` [RFC PATCH v1 25/26] kvm: vmi: extend handshake to include the e820 table Adalbert Lazăr
` (3 subsequent siblings)
27 siblings, 0 replies; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 0:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Adalbert Lazăr
Some introspection tools can detect when the guest is shutting down.
This new option, 'unhook_on_shutdown' controls if QEMU will notify the
introspection tool on a shutdown command at its level.
Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
---
accel/kvm/vmi.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/accel/kvm/vmi.c b/accel/kvm/vmi.c
index 2c6981a4bf..02877eec06 100644
--- a/accel/kvm/vmi.c
+++ b/accel/kvm/vmi.c
@@ -58,6 +58,7 @@ typedef struct VMIntrospection {
GSource *unhook_timer;
uint32_t unhook_timeout;
bool async_unhook;
+ bool unhook_on_shutdown;
int reconnect_time;
@@ -203,6 +204,20 @@ static void prop_set_async_unhook(Object *obj, bool value, Error **errp)
i->async_unhook = value;
}
+static bool prop_get_unhook_on_shutdown(Object *obj, Error **errp)
+{
+ VMIntrospection *i = VM_INTROSPECTION(obj);
+
+ return i->unhook_on_shutdown;
+}
+
+static void prop_set_unhook_on_shutdown(Object *obj, bool value, Error **errp)
+{
+ VMIntrospection *i = VM_INTROSPECTION(obj);
+
+ i->unhook_on_shutdown = value;
+}
+
static void prop_get_uint32(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
@@ -285,6 +300,11 @@ static void instance_init(Object *obj)
prop_get_async_unhook,
prop_set_async_unhook, NULL);
+ i->unhook_on_shutdown = true;
+ object_property_add_bool(obj, "unhook_on_shutdown",
+ prop_get_unhook_on_shutdown,
+ prop_set_unhook_on_shutdown, NULL);
+
vmstate_register(NULL, 0, &vmstate_introspection, i);
}
@@ -801,6 +821,11 @@ static bool intercept_action(VMIntrospection *i,
}
switch (action) {
+ case VMI_INTERCEPT_SHUTDOWN:
+ if (!i->unhook_on_shutdown) {
+ return false;
+ }
+ break;
case VMI_INTERCEPT_FORCE_RESET:
disconnect_and_unhook_kvmi(i);
return false;
^ permalink raw reply related [flat|nested] 46+ messages in thread* [RFC PATCH v1 25/26] kvm: vmi: extend handshake to include the e820 table
2020-04-15 0:59 [RFC PATCH v1 00/26] VM introspection Adalbert Lazăr
` (23 preceding siblings ...)
2020-04-15 0:59 ` [RFC PATCH v1 24/26] kvm: vmi: add 'unhook_on_shutdown' property Adalbert Lazăr
@ 2020-04-15 0:59 ` Adalbert Lazăr
2020-04-15 0:59 ` [RFC PATCH v1 26/26] kvm: vmi: add 'command' and 'event' properties Adalbert Lazăr
` (2 subsequent siblings)
27 siblings, 0 replies; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 0:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Adalbert Lazăr
The introspection tool can use the e820 table to avoid accessing
(read/write) or modifying access (rwx) for reserved memory pages.
Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
---
accel/kvm/vmi.c | 68 ++++++++++++++++++++++++++++++----
include/sysemu/vmi-handshake.h | 23 +++++++++++-
2 files changed, 82 insertions(+), 9 deletions(-)
diff --git a/accel/kvm/vmi.c b/accel/kvm/vmi.c
index 02877eec06..f70d78848a 100644
--- a/accel/kvm/vmi.c
+++ b/accel/kvm/vmi.c
@@ -26,6 +26,7 @@
#include "migration/misc.h"
#include "qapi/qmp/qobject.h"
#include "monitor/monitor.h"
+#include "hw/i386/e820_memory_layout.h"
#include "sysemu/vmi-intercept.h"
#include "sysemu/vmi-handshake.h"
@@ -412,23 +413,74 @@ static void register_types(void)
type_init(register_types);
+static uint8_t handshake_cpu_type(void)
+{
+#ifdef TARGET_X86_64
+ return QEMU_VMI_CPU_TYPE_X86_64;
+#elif TARGET_I386
+ return QEMU_VMI_CPU_TYPE_I386;
+#else
+ return QEMU_VMI_CPU_TYPE_UNKNOWN;
+#endif
+}
+
+static int cmp_address(const void *a, const void *b)
+{
+ uint64_t addr_a = ((qemu_vmi_e820_entry *)a)->address;
+ uint64_t addr_b = ((qemu_vmi_e820_entry *)b)->address;
+
+ return (addr_a > addr_b) - (addr_a < addr_b);
+}
+
+static void fill_e820_info(qemu_vmi_e820_entry *dest, int n)
+{
+ int idx;
+
+ for (idx = 0; idx < n; idx++)
+ e820_get_entry2(idx, &dest[idx].type, &dest[idx].address,
+ &dest[idx].length);
+
+ qsort(dest, n, sizeof(*dest), cmp_address);
+}
+
static bool send_handshake_info(VMIntrospection *i, Error **errp)
{
- qemu_vmi_to_introspector send = {};
+ qemu_vmi_to_introspector *send;
+ int max_n_e820, n_e820;
const char *vm_name;
+ size_t send_sz;
int r;
- send.struct_size = sizeof(send);
- send.start_time = i->vm_start_time;
- memcpy(&send.uuid, &qemu_uuid, sizeof(send.uuid));
+ max_n_e820 = 8 * sizeof(((qemu_vmi_to_introspector *)0)->arch.e820_count);
+ n_e820 = e820_get_num_entries();
+
+ if (n_e820 < 0 || n_e820 > max_n_e820) {
+ warn_report("VMI: discard e820 info (size %d, max %d)",
+ n_e820, max_n_e820);
+ n_e820 = 0;
+ }
+
+ send_sz = sizeof(*send) + n_e820 * sizeof(qemu_vmi_e820_entry);
+
+ send = g_malloc0(send_sz);
+
+ send->struct_size = send_sz;
+ send->start_time = i->vm_start_time;
+ send->cpu_type = handshake_cpu_type();
+ memcpy(&send->uuid, &qemu_uuid, sizeof(send->uuid));
vm_name = qemu_get_vm_name();
if (vm_name) {
- snprintf(send.name, sizeof(send.name), "%s", vm_name);
- send.name[sizeof(send.name) - 1] = 0;
+ snprintf(send->name, sizeof(send->name), "%s", vm_name);
+ send->name[sizeof(send->name) - 1] = 0;
+ }
+ send->arch.e820_count = n_e820;
+ if (n_e820) {
+ fill_e820_info(send->arch.e820_entries, n_e820);
}
- r = qemu_chr_fe_write_all(&i->sock, (uint8_t *)&send, sizeof(send));
- if (r != sizeof(send)) {
+ r = qemu_chr_fe_write_all(&i->sock, (uint8_t *)send, send_sz);
+ g_free(send);
+ if (r != send_sz) {
error_setg_errno(errp, errno, "VMI: error writing to '%s'",
i->chardevid);
return false;
diff --git a/include/sysemu/vmi-handshake.h b/include/sysemu/vmi-handshake.h
index 19bdfb6740..3c5201d37b 100644
--- a/include/sysemu/vmi-handshake.h
+++ b/include/sysemu/vmi-handshake.h
@@ -9,6 +9,25 @@
enum { QEMU_VMI_NAME_SIZE = 64 };
enum { QEMU_VMI_COOKIE_HASH_SIZE = 20};
+enum {
+ QEMU_VMI_CPU_TYPE_I386 = 0,
+ QEMU_VMI_CPU_TYPE_X86_64 = 1,
+ QEMU_VMI_CPU_TYPE_UNKNOWN = 255
+};
+
+typedef struct qemu_vmi_e820_entry {
+ uint64_t address;
+ uint64_t length;
+ uint32_t type;
+ uint32_t padding;
+} qemu_vmi_e820_entry;
+
+typedef struct qemu_vmi_to_introspector_x86 {
+ uint8_t e820_count;
+ uint8_t padding[3];
+ qemu_vmi_e820_entry e820_entries[0];
+} qemu_vmi_to_introspector_x86;
+
/**
* qemu_vmi_to_introspector:
*
@@ -22,9 +41,11 @@ enum { QEMU_VMI_COOKIE_HASH_SIZE = 20};
typedef struct qemu_vmi_to_introspector {
uint32_t struct_size;
uint8_t uuid[16];
- uint32_t padding;
+ uint8_t cpu_type;
+ uint8_t padding[3];
int64_t start_time;
char name[QEMU_VMI_NAME_SIZE];
+ qemu_vmi_to_introspector_x86 arch;
/* ... */
} qemu_vmi_to_introspector;
^ permalink raw reply related [flat|nested] 46+ messages in thread* [RFC PATCH v1 26/26] kvm: vmi: add 'command' and 'event' properties
2020-04-15 0:59 [RFC PATCH v1 00/26] VM introspection Adalbert Lazăr
` (24 preceding siblings ...)
2020-04-15 0:59 ` [RFC PATCH v1 25/26] kvm: vmi: extend handshake to include the e820 table Adalbert Lazăr
@ 2020-04-15 0:59 ` Adalbert Lazăr
2020-04-15 2:02 ` [RFC PATCH v1 00/26] VM introspection no-reply
2020-04-15 2:26 ` no-reply
27 siblings, 0 replies; 46+ messages in thread
From: Adalbert Lazăr @ 2020-04-15 0:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Adalbert Lazăr
There are cases when the access to an introspected VM must be limited
to certain introspection commands/events.
Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
---
accel/kvm/vmi.c | 86 ++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 74 insertions(+), 12 deletions(-)
diff --git a/accel/kvm/vmi.c b/accel/kvm/vmi.c
index f70d78848a..1574a643c4 100644
--- a/accel/kvm/vmi.c
+++ b/accel/kvm/vmi.c
@@ -73,6 +73,9 @@ typedef struct VMIntrospection {
QDict *qmp_rsp;
bool kvmi_hooked;
+
+ GArray *allowed_commands;
+ GArray *allowed_events;
} VMIntrospection;
typedef struct VMIntrospectionClass {
@@ -94,6 +97,8 @@ static bool suspend_pending;
static bool migrate_pending;
static bool shutdown_pending;
+static __s32 all_IDs = -1;
+
#define TYPE_VM_INTROSPECTION "introspection"
#define VM_INTROSPECTION(obj) \
@@ -239,6 +244,25 @@ static void prop_set_uint32(Object *obj, Visitor *v, const char *name,
}
}
+static void prop_add_to_array(Object *obj, Visitor *v,
+ const char *name, void *opaque,
+ Error **errp)
+{
+ Error *local_err = NULL;
+ GArray *arr = opaque;
+ uint32_t value;
+
+ visit_type_uint32(v, name, &value, &local_err);
+ if (!local_err && value == (uint32_t)all_IDs) {
+ error_setg(&local_err, "VMI: add %s: invalid id %d", name, value);
+ }
+ if (local_err) {
+ error_propagate(errp, local_err);
+ } else {
+ g_array_append_val(arr, value);
+ }
+}
+
static bool chardev_is_connected(VMIntrospection *i, Error **errp)
{
Object *obj = OBJECT(i->chr);
@@ -286,6 +310,15 @@ static void instance_init(Object *obj)
object_property_add_str(obj, "chardev", NULL, prop_set_chardev, NULL);
object_property_add_str(obj, "key", NULL, prop_set_key, NULL);
+ i->allowed_commands = g_array_new(FALSE, FALSE, sizeof(uint32_t));
+ object_property_add(obj, "command", "uint32",
+ prop_add_to_array, NULL,
+ NULL, i->allowed_commands, NULL);
+ i->allowed_events = g_array_new(FALSE, FALSE, sizeof(uint32_t));
+ object_property_add(obj, "event", "uint32",
+ prop_add_to_array, NULL,
+ NULL, i->allowed_events, NULL);
+
i->handshake_timeout = HANDSHAKE_TIMEOUT_SEC;
object_property_add(obj, "handshake_timeout", "uint32",
prop_set_uint32, prop_get_uint32,
@@ -368,6 +401,13 @@ static void instance_finalize(Object *obj)
VMIntrospectionClass *ic = VM_INTROSPECTION_CLASS(obj->class);
VMIntrospection *i = VM_INTROSPECTION(obj);
+ if (i->allowed_commands) {
+ g_array_free(i->allowed_commands, TRUE);
+ }
+ if (i->allowed_events) {
+ g_array_free(i->allowed_events, TRUE);
+ }
+
g_free(i->chardevid);
g_free(i->keyid);
@@ -531,11 +571,39 @@ static bool validate_handshake(VMIntrospection *i, Error **errp)
return true;
}
+static bool set_allowed_features(int ioctl, GArray *allowed, Error **errp)
+{
+ struct kvm_introspection_feature feature;
+ gint i;
+
+ feature.allow = 1;
+
+ if (allowed->len == 0) {
+ feature.id = all_IDs;
+ if (kvm_vm_ioctl(kvm_state, ioctl, &feature)) {
+ goto out_err;
+ }
+ } else {
+ for (i = 0; i < allowed->len; i++) {
+ feature.id = g_array_index(allowed, uint32_t, i);
+ if (kvm_vm_ioctl(kvm_state, ioctl, &feature)) {
+ goto out_err;
+ }
+ }
+ }
+
+ return true;
+
+out_err:
+ error_setg_errno(errp, -errno,
+ "VMI: feature %d with id %d failed",
+ ioctl, feature.id);
+ return false;
+}
+
static bool connect_kernel(VMIntrospection *i, Error **errp)
{
- struct kvm_introspection_feature commands, events;
struct kvm_introspection_hook kernel;
- const __s32 all_ids = -1;
memset(&kernel, 0, sizeof(kernel));
memcpy(kernel.uuid, &qemu_uuid, sizeof(kernel.uuid));
@@ -553,20 +621,14 @@ static bool connect_kernel(VMIntrospection *i, Error **errp)
i->kvmi_hooked = true;
- commands.allow = 1;
- commands.id = all_ids;
- if (kvm_vm_ioctl(kvm_state, KVM_INTROSPECTION_COMMAND, &commands)) {
- error_setg_errno(errp, -errno,
- "VMI: ioctl/KVM_INTROSPECTION_COMMAND failed");
+ if (!set_allowed_features(KVM_INTROSPECTION_COMMAND,
+ i->allowed_commands, errp)) {
unhook_kvmi(i);
return false;
}
- events.allow = 1;
- events.id = all_ids;
- if (kvm_vm_ioctl(kvm_state, KVM_INTROSPECTION_EVENT, &events)) {
- error_setg_errno(errp, -errno,
- "VMI: ioctl/KVM_INTROSPECTION_EVENT failed");
+ if (!set_allowed_features(KVM_INTROSPECTION_EVENT,
+ i->allowed_events, errp)) {
unhook_kvmi(i);
return false;
}
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [RFC PATCH v1 00/26] VM introspection
2020-04-15 0:59 [RFC PATCH v1 00/26] VM introspection Adalbert Lazăr
` (25 preceding siblings ...)
2020-04-15 0:59 ` [RFC PATCH v1 26/26] kvm: vmi: add 'command' and 'event' properties Adalbert Lazăr
@ 2020-04-15 2:02 ` no-reply
2020-04-15 2:26 ` no-reply
27 siblings, 0 replies; 46+ messages in thread
From: no-reply @ 2020-04-15 2:02 UTC (permalink / raw)
To: alazar
Cc: tamas, ehabkost, konrad.wilk, jan.kiszka, samuel.lauren, mst,
qemu-devel, armbru, alazar, quintela, patrick.colp,
mathieu.tarral, stefanha, pbonzini, marcandre.lureau, rth,
mdontu, dgilbert, marian.c.rotariu
Patchew URL: https://patchew.org/QEMU/20200415005938.23895-1-alazar@bitdefender.com/
Hi,
This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===
CC chardev/char-mux.o
CC chardev/char-null.o
Warning, treated as error:
/tmp/qemu-test/src/docs/../qemu-options.hx:5041:Inline literal start-string without end-string.
make: *** [Makefile:1115: .docs_system_qemu.1_docs_system_qemu-block-drivers.7_docs_system_qemu-cpu-models.7.sentinel.] Error 2
make: *** Deleting file '.docs_system_qemu.1_docs_system_qemu-block-drivers.7_docs_system_qemu-cpu-models.7.sentinel.'
make: *** Waiting for unfinished jobs....
Warning, treated as error:
/tmp/qemu-test/src/docs/../qemu-options.hx:5041:Inline literal start-string without end-string.
make: *** [Makefile:1104: docs/system/index.html] Error 2
Traceback (most recent call last):
File "./tests/docker/docker.py", line 664, in <module>
sys.exit(main())
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=14969c1b7a7f4cb589d53f5ee4a705f4', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=x86_64-softmmu', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-a4iu7vy2/src/docker-src.2020-04-14-21.58.48.26010:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-debug']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=14969c1b7a7f4cb589d53f5ee4a705f4
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-a4iu7vy2/src'
make: *** [docker-run-test-debug@fedora] Error 2
real 3m22.713s
user 0m7.733s
The full log is available at
http://patchew.org/logs/20200415005938.23895-1-alazar@bitdefender.com/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [RFC PATCH v1 00/26] VM introspection
2020-04-15 0:59 [RFC PATCH v1 00/26] VM introspection Adalbert Lazăr
` (26 preceding siblings ...)
2020-04-15 2:02 ` [RFC PATCH v1 00/26] VM introspection no-reply
@ 2020-04-15 2:26 ` no-reply
27 siblings, 0 replies; 46+ messages in thread
From: no-reply @ 2020-04-15 2:26 UTC (permalink / raw)
To: alazar
Cc: tamas, ehabkost, konrad.wilk, jan.kiszka, samuel.lauren, mst,
qemu-devel, armbru, alazar, quintela, patrick.colp,
mathieu.tarral, stefanha, pbonzini, marcandre.lureau, rth,
mdontu, dgilbert, marian.c.rotariu
Patchew URL: https://patchew.org/QEMU/20200415005938.23895-1-alazar@bitdefender.com/
Hi,
This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===
CC block/blklogwrites.o
CC block/block-backend.o
Warning, treated as error:
/tmp/qemu-test/src/docs/../qemu-options.hx:5041:Inline literal start-string without end-string.
Warning, treated as error:
/tmp/qemu-test/src/docs/../qemu-options.hx:5041:Inline literal start-string without end-string.
CC block/snapshot.o
CC block/qapi.o
---
CC block/file-win32.o
CC block/null.o
CC block/mirror.o
make: *** [Makefile:1115: .docs_system_qemu.1_docs_system_qemu-block-drivers.7_docs_system_qemu-cpu-models.7.sentinel.] Error 2
make: *** Deleting file '.docs_system_qemu.1_docs_system_qemu-block-drivers.7_docs_system_qemu-cpu-models.7.sentinel.'
make: *** Waiting for unfinished jobs....
make: *** [Makefile:1104: docs/system/index.html] Error 2
Traceback (most recent call last):
File "./tests/docker/docker.py", line 664, in <module>
sys.exit(main())
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=9999c583cfe64588a174af36ef6d29c3', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-6xohf4_4/src/docker-src.2020-04-14-22.23.28.25626:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=9999c583cfe64588a174af36ef6d29c3
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-6xohf4_4/src'
make: *** [docker-run-test-mingw@fedora] Error 2
real 2m43.146s
user 0m6.958s
The full log is available at
http://patchew.org/logs/20200415005938.23895-1-alazar@bitdefender.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 46+ messages in thread