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@xxxxxxxxx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
