| Message ID | 1410167870-680-4-git-send-email-david.marchand@6wind.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Sep 08, 2014 at 11:17:50AM +0200, David Marchand wrote: > Send a protocol version as the first message from server, clients must close > communication if they don't support this protocol version. What's the motivation here? This is at best a way to break all clients if an incompatible change in the server is made. Would not it be better to send a bitmap, or a list of supported versions, so it's possible to write servers compatible with multiple clients? > Older QEMUs should be fine with this change in the protocol since they overrides they override > their own vm_id on reception of an id associated to no eventfd. associated with? I don't understand how compatibility works exactly, a bit more explanation in code comments would be nice. In particular new client won't work with old server, will it? > > Signed-off-by: David Marchand <david.marchand@6wind.com> > --- > contrib/ivshmem-client/ivshmem-client.c | 13 ++++++++++--- > contrib/ivshmem-client/ivshmem-client.h | 1 + > contrib/ivshmem-server/ivshmem-server.c | 9 +++++++++ > contrib/ivshmem-server/ivshmem-server.h | 1 + > docs/specs/ivshmem_device_spec.txt | 9 ++++++--- > hw/misc/ivshmem.c | 27 +++++++++++++++++++++++++-- > include/hw/misc/ivshmem.h | 17 +++++++++++++++++ > 7 files changed, 69 insertions(+), 8 deletions(-) > create mode 100644 include/hw/misc/ivshmem.h > > diff --git a/contrib/ivshmem-client/ivshmem-client.c b/contrib/ivshmem-client/ivshmem-client.c > index 11c805c..4413e26 100644 > --- a/contrib/ivshmem-client/ivshmem-client.c > +++ b/contrib/ivshmem-client/ivshmem-client.c > @@ -201,10 +201,17 @@ ivshmem_client_connect(IvshmemClient *client) > goto err_close; > } > > - /* first, we expect our index + a fd == -1 */ > + /* first, we expect a protocol version */ > + if (ivshmem_client_read_one_msg(client, &tmp, &fd) < 0 || > + (tmp != IVSHMEM_PROTOCOL_VERSION) || fd != -1) { > + IVSHMEM_CLIENT_DEBUG(client, "cannot read from server\n"); > + goto err_close; > + } > + > + /* then, we expect our index + a fd == -1 */ > if (ivshmem_client_read_one_msg(client, &client->local.id, &fd) < 0 || > client->local.id < 0 || fd != -1) { > - IVSHMEM_CLIENT_DEBUG(client, "cannot read from server\n"); > + IVSHMEM_CLIENT_DEBUG(client, "cannot read from server (2)\n"); > goto err_close; > } > IVSHMEM_CLIENT_DEBUG(client, "our_id=%ld\n", client->local.id); > @@ -216,7 +223,7 @@ ivshmem_client_connect(IvshmemClient *client) > if (fd >= 0) { > close(fd); > } > - IVSHMEM_CLIENT_DEBUG(client, "cannot read from server (2)\n"); > + IVSHMEM_CLIENT_DEBUG(client, "cannot read from server (3)\n"); > goto err_close; > } > client->shm_fd = fd; > diff --git a/contrib/ivshmem-client/ivshmem-client.h b/contrib/ivshmem-client/ivshmem-client.h > index 284c4a3..9215f34 100644 > --- a/contrib/ivshmem-client/ivshmem-client.h > +++ b/contrib/ivshmem-client/ivshmem-client.h > @@ -23,6 +23,7 @@ > #include <sys/select.h> > > #include "qemu/queue.h" > +#include "hw/misc/ivshmem.h" > > /** > * Maximum number of notification vectors supported by the client > diff --git a/contrib/ivshmem-server/ivshmem-server.c b/contrib/ivshmem-server/ivshmem-server.c > index 91dc208..ea290f6 100644 > --- a/contrib/ivshmem-server/ivshmem-server.c > +++ b/contrib/ivshmem-server/ivshmem-server.c > @@ -99,6 +99,15 @@ ivshmem_server_send_initial_info(IvshmemServer *server, IvshmemServerPeer *peer) > { > int ret; > > + /* send our protocol version first */ > + ret = ivshmem_server_send_one_msg(peer->sock_fd, IVSHMEM_PROTOCOL_VERSION, > + -1); > + if (ret < 0) { > + IVSHMEM_SERVER_DEBUG(server, "cannot send version: %s\n", > + strerror(errno)); > + return -1; > + } > + > /* send the peer id to the client */ > ret = ivshmem_server_send_one_msg(peer->sock_fd, peer->id, -1); > if (ret < 0) { > diff --git a/contrib/ivshmem-server/ivshmem-server.h b/contrib/ivshmem-server/ivshmem-server.h > index f85dcd2..38df7bb 100644 > --- a/contrib/ivshmem-server/ivshmem-server.h > +++ b/contrib/ivshmem-server/ivshmem-server.h > @@ -30,6 +30,7 @@ > #include <sys/select.h> > > #include "qemu/queue.h" > +#include "hw/misc/ivshmem.h" > > /** > * Maximum number of notification vectors supported by the server > diff --git a/docs/specs/ivshmem_device_spec.txt b/docs/specs/ivshmem_device_spec.txt > index 12f338e..3435116 100644 > --- a/docs/specs/ivshmem_device_spec.txt > +++ b/docs/specs/ivshmem_device_spec.txt > @@ -64,6 +64,8 @@ It creates a shared memory object then waits for clients to connect on a unix > socket. > > For each client (QEMU process) that connects to the server: > +- the server sends a protocol version, if client does not support it, the client > + closes the communication, > - the server assigns an ID for this client and sends this ID to him as the first > message, > - the server sends a fd to the shared memory object to this client, > @@ -86,9 +88,10 @@ been provided in qemu.git/contrib/ivshmem-client for debug. > > *QEMU as an ivshmem client* > > -At initialisation, when creating the ivshmem device, QEMU gets its ID from the > -server then makes it available through BAR0 IVPosition register for the VM to > -use (see 'PCI device registers' subsection). > +At initialisation, when creating the ivshmem device, QEMU first receives a > +protocol version and closes communication with server if it does not match. > +Then, QEMU gets its ID from the server then makes it available through BAR0 > +IVPosition register for the VM to use (see 'PCI device registers' subsection). > QEMU then uses the fd to the shared memory to map it to BAR2. > eventfds for all other clients received from the server are stored to implement > BAR0 Doorbell register (see 'PCI device registers' subsection). > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c > index bd9d718..e08632b 100644 > --- a/hw/misc/ivshmem.c > +++ b/hw/misc/ivshmem.c > @@ -26,6 +26,8 @@ > #include "qemu/event_notifier.h" > #include "sysemu/char.h" > > +#include "hw/misc/ivshmem.h" > + > #include <sys/mman.h> > #include <sys/types.h> > > @@ -528,6 +530,27 @@ static void ivshmem_read(void *opaque, const uint8_t * buf, int flags) > } > } > > +static void ivshmem_check_version(void *opaque, const uint8_t * buf, int flags) > +{ > + IVShmemState *s = opaque; > + int tmp; > + long version; > + > + memcpy(&version, buf, sizeof(long)); > + tmp = qemu_chr_fe_get_msgfd(s->server_chr); > + if (tmp != -1 || version != IVSHMEM_PROTOCOL_VERSION) { > + fprintf(stderr, "incompatible version, you are connecting to a ivshmem-" > + "server using a different protocol please check your setup\n"); > + qemu_chr_delete(s->server_chr); > + s->server_chr = NULL; > + return; > + } > + > + IVSHMEM_DPRINTF("version check ok, switch to real chardev handler\n"); > + qemu_chr_add_handlers(s->server_chr, ivshmem_can_receive, ivshmem_read, > + ivshmem_event, s); > +} > + > /* Select the MSI-X vectors used by device. > * ivshmem maps events to vectors statically, so > * we just enable all vectors on init and after reset. */ > @@ -742,8 +765,8 @@ static int pci_ivshmem_init(PCIDevice *dev) > > s->eventfd_chr = g_malloc0(s->vectors * sizeof(CharDriverState *)); > > - qemu_chr_add_handlers(s->server_chr, ivshmem_can_receive, ivshmem_read, > - ivshmem_event, s); > + qemu_chr_add_handlers(s->server_chr, ivshmem_can_receive, > + ivshmem_check_version, ivshmem_event, s); > } else { > /* just map the file immediately, we're not using a server */ > int fd; > diff --git a/include/hw/misc/ivshmem.h b/include/hw/misc/ivshmem.h > new file mode 100644 > index 0000000..22c9f1f > --- /dev/null > +++ b/include/hw/misc/ivshmem.h > @@ -0,0 +1,17 @@ > +/* > + * Copyright 6WIND S.A., 2014 > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or > + * (at your option) any later version. See the COPYING file in the > + * top-level directory. > + */ > + > +#ifndef _IVSHMEM_H_ > +#define _IVSHMEM_H_ > + > +/** > + * Protocol version > + */ > +#define IVSHMEM_PROTOCOL_VERSION 0x0 > + > +#endif > -- > 1.7.10.4
On Mon, Sep 08, 2014 at 12:49:48PM +0300, Michael S. Tsirkin wrote: > On Mon, Sep 08, 2014 at 11:17:50AM +0200, David Marchand wrote: > > Send a protocol version as the first message from server, clients must close > > communication if they don't support this protocol version. > > What's the motivation here? > This is at best a way to break all clients if an incompatible > change in the server is made. > Would not it be better to send a bitmap, or a list of supported > versions, so it's possible to write servers compatible > with multiple clients? I'm not sure a full-fledged feature negotiation system is needed. The ivshmem protocol is local to the host and all participants are under control of the administrator. I suggested a protocol version to protect against misconfiguration. For example, building QEMU from source but talking to an outdated ivhsmem server that is still running from before. Remember that ivshmem-server and QEMU are shipped together by the distro. So in 99% of the cases they will have the same version anyway. But we want to protect against rare misconfiguration that break things (user mixing and matching incompatible software). The only reason I can see for fancy negotiation is to make life easier for proprietary third-party software, which I don't care about or like. Stefan
On 09/23/2014 05:58 PM, Stefan Hajnoczi wrote: > I'm not sure a full-fledged feature negotiation system is needed. The > ivshmem protocol is local to the host and all participants are under > control of the administrator. > > I suggested a protocol version to protect against misconfiguration. For > example, building QEMU from source but talking to an outdated ivhsmem > server that is still running from before. > > Remember that ivshmem-server and QEMU are shipped together by the > distro. So in 99% of the cases they will have the same version anyway. > But we want to protect against rare misconfiguration that break things > (user mixing and matching incompatible software). Ok, so how about keeping with this simple mechanism at the moment ? If this versionning system is too limited in the future, I think we will need a global rework on the protocol, anyway.
diff --git a/contrib/ivshmem-client/ivshmem-client.c b/contrib/ivshmem-client/ivshmem-client.c index 11c805c..4413e26 100644 --- a/contrib/ivshmem-client/ivshmem-client.c +++ b/contrib/ivshmem-client/ivshmem-client.c @@ -201,10 +201,17 @@ ivshmem_client_connect(IvshmemClient *client) goto err_close; } - /* first, we expect our index + a fd == -1 */ + /* first, we expect a protocol version */ + if (ivshmem_client_read_one_msg(client, &tmp, &fd) < 0 || + (tmp != IVSHMEM_PROTOCOL_VERSION) || fd != -1) { + IVSHMEM_CLIENT_DEBUG(client, "cannot read from server\n"); + goto err_close; + } + + /* then, we expect our index + a fd == -1 */ if (ivshmem_client_read_one_msg(client, &client->local.id, &fd) < 0 || client->local.id < 0 || fd != -1) { - IVSHMEM_CLIENT_DEBUG(client, "cannot read from server\n"); + IVSHMEM_CLIENT_DEBUG(client, "cannot read from server (2)\n"); goto err_close; } IVSHMEM_CLIENT_DEBUG(client, "our_id=%ld\n", client->local.id); @@ -216,7 +223,7 @@ ivshmem_client_connect(IvshmemClient *client) if (fd >= 0) { close(fd); } - IVSHMEM_CLIENT_DEBUG(client, "cannot read from server (2)\n"); + IVSHMEM_CLIENT_DEBUG(client, "cannot read from server (3)\n"); goto err_close; } client->shm_fd = fd; diff --git a/contrib/ivshmem-client/ivshmem-client.h b/contrib/ivshmem-client/ivshmem-client.h index 284c4a3..9215f34 100644 --- a/contrib/ivshmem-client/ivshmem-client.h +++ b/contrib/ivshmem-client/ivshmem-client.h @@ -23,6 +23,7 @@ #include <sys/select.h> #include "qemu/queue.h" +#include "hw/misc/ivshmem.h" /** * Maximum number of notification vectors supported by the client diff --git a/contrib/ivshmem-server/ivshmem-server.c b/contrib/ivshmem-server/ivshmem-server.c index 91dc208..ea290f6 100644 --- a/contrib/ivshmem-server/ivshmem-server.c +++ b/contrib/ivshmem-server/ivshmem-server.c @@ -99,6 +99,15 @@ ivshmem_server_send_initial_info(IvshmemServer *server, IvshmemServerPeer *peer) { int ret; + /* send our protocol version first */ + ret = ivshmem_server_send_one_msg(peer->sock_fd, IVSHMEM_PROTOCOL_VERSION, + -1); + if (ret < 0) { + IVSHMEM_SERVER_DEBUG(server, "cannot send version: %s\n", + strerror(errno)); + return -1; + } + /* send the peer id to the client */ ret = ivshmem_server_send_one_msg(peer->sock_fd, peer->id, -1); if (ret < 0) { diff --git a/contrib/ivshmem-server/ivshmem-server.h b/contrib/ivshmem-server/ivshmem-server.h index f85dcd2..38df7bb 100644 --- a/contrib/ivshmem-server/ivshmem-server.h +++ b/contrib/ivshmem-server/ivshmem-server.h @@ -30,6 +30,7 @@ #include <sys/select.h> #include "qemu/queue.h" +#include "hw/misc/ivshmem.h" /** * Maximum number of notification vectors supported by the server diff --git a/docs/specs/ivshmem_device_spec.txt b/docs/specs/ivshmem_device_spec.txt index 12f338e..3435116 100644 --- a/docs/specs/ivshmem_device_spec.txt +++ b/docs/specs/ivshmem_device_spec.txt @@ -64,6 +64,8 @@ It creates a shared memory object then waits for clients to connect on a unix socket. For each client (QEMU process) that connects to the server: +- the server sends a protocol version, if client does not support it, the client + closes the communication, - the server assigns an ID for this client and sends this ID to him as the first message, - the server sends a fd to the shared memory object to this client, @@ -86,9 +88,10 @@ been provided in qemu.git/contrib/ivshmem-client for debug. *QEMU as an ivshmem client* -At initialisation, when creating the ivshmem device, QEMU gets its ID from the -server then makes it available through BAR0 IVPosition register for the VM to -use (see 'PCI device registers' subsection). +At initialisation, when creating the ivshmem device, QEMU first receives a +protocol version and closes communication with server if it does not match. +Then, QEMU gets its ID from the server then makes it available through BAR0 +IVPosition register for the VM to use (see 'PCI device registers' subsection). QEMU then uses the fd to the shared memory to map it to BAR2. eventfds for all other clients received from the server are stored to implement BAR0 Doorbell register (see 'PCI device registers' subsection). diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c index bd9d718..e08632b 100644 --- a/hw/misc/ivshmem.c +++ b/hw/misc/ivshmem.c @@ -26,6 +26,8 @@ #include "qemu/event_notifier.h" #include "sysemu/char.h" +#include "hw/misc/ivshmem.h" + #include <sys/mman.h> #include <sys/types.h> @@ -528,6 +530,27 @@ static void ivshmem_read(void *opaque, const uint8_t * buf, int flags) } } +static void ivshmem_check_version(void *opaque, const uint8_t * buf, int flags) +{ + IVShmemState *s = opaque; + int tmp; + long version; + + memcpy(&version, buf, sizeof(long)); + tmp = qemu_chr_fe_get_msgfd(s->server_chr); + if (tmp != -1 || version != IVSHMEM_PROTOCOL_VERSION) { + fprintf(stderr, "incompatible version, you are connecting to a ivshmem-" + "server using a different protocol please check your setup\n"); + qemu_chr_delete(s->server_chr); + s->server_chr = NULL; + return; + } + + IVSHMEM_DPRINTF("version check ok, switch to real chardev handler\n"); + qemu_chr_add_handlers(s->server_chr, ivshmem_can_receive, ivshmem_read, + ivshmem_event, s); +} + /* Select the MSI-X vectors used by device. * ivshmem maps events to vectors statically, so * we just enable all vectors on init and after reset. */ @@ -742,8 +765,8 @@ static int pci_ivshmem_init(PCIDevice *dev) s->eventfd_chr = g_malloc0(s->vectors * sizeof(CharDriverState *)); - qemu_chr_add_handlers(s->server_chr, ivshmem_can_receive, ivshmem_read, - ivshmem_event, s); + qemu_chr_add_handlers(s->server_chr, ivshmem_can_receive, + ivshmem_check_version, ivshmem_event, s); } else { /* just map the file immediately, we're not using a server */ int fd; diff --git a/include/hw/misc/ivshmem.h b/include/hw/misc/ivshmem.h new file mode 100644 index 0000000..22c9f1f --- /dev/null +++ b/include/hw/misc/ivshmem.h @@ -0,0 +1,17 @@ +/* + * Copyright 6WIND S.A., 2014 + * + * This work is licensed under the terms of the GNU GPL, version 2 or + * (at your option) any later version. See the COPYING file in the + * top-level directory. + */ + +#ifndef _IVSHMEM_H_ +#define _IVSHMEM_H_ + +/** + * Protocol version + */ +#define IVSHMEM_PROTOCOL_VERSION 0x0 + +#endif
Send a protocol version as the first message from server, clients must close communication if they don't support this protocol version. Older QEMUs should be fine with this change in the protocol since they overrides their own vm_id on reception of an id associated to no eventfd. Signed-off-by: David Marchand <david.marchand@6wind.com> --- contrib/ivshmem-client/ivshmem-client.c | 13 ++++++++++--- contrib/ivshmem-client/ivshmem-client.h | 1 + contrib/ivshmem-server/ivshmem-server.c | 9 +++++++++ contrib/ivshmem-server/ivshmem-server.h | 1 + docs/specs/ivshmem_device_spec.txt | 9 ++++++--- hw/misc/ivshmem.c | 27 +++++++++++++++++++++++++-- include/hw/misc/ivshmem.h | 17 +++++++++++++++++ 7 files changed, 69 insertions(+), 8 deletions(-) create mode 100644 include/hw/misc/ivshmem.h