Re: [PATCH v5 3/3] ivshmem: add check on protocol version in QEMU
On Fri, Sep 05, 2014 at 02:25:05PM +0200, David Marchand wrote:
> Hello Stefan,
>
> On 09/05/2014 12:29 PM, Stefan Hajnoczi wrote:
> >On Thu, Sep 04, 2014 at 02:51:01PM +0200, David Marchand wrote:
> >>+"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, finish init and switch to real
> >>chardev "
> >>+"handler\n");
> >>+
> >>+pci_register_bar(dev, 2, s->ivshmem_attr, &s->bar);
> >
> >Not sure if it is okay to delay PCI initialization to a fd hander
> >callback.
> >
> >If the version message is too slow the guest could see the PCI adapter
> >without the BAR!
> >
> >Did you move this code in order to prevent the guest from accessing the
> >device before it has connected to the server? Perhaps the device needs
> >a state field that tracks whether or not it is ready for operation. Any
> >access before RUNNING state is reached will be ignored (?).
>
> Yes, exactly.
>
> There already is a synchronisation mechanism described in the documentation:
> "When using the server, since the server is a separate process, the VM ID
> will only be set when the device is ready (shared memory is received from
> the server and accessible via the device). If the device is not ready, the
> IVPosition will return -1.
> Applications should ensure that they have a valid VM ID before accessing the
> shared memory."
>
> So actually, this move is unneeded if ivshmem users comply to this.
>
> I will let the init stuff (pci_register_bar + gmalloc) where it was before,
> ivshmem_check_version will only switch the chardev handler.
>
> What do you think about this ?
Sounds good.
pgp0HzrORqFD9.pgp
Description: PGP signature
Re: [PATCH v5 3/3] ivshmem: add check on protocol version in QEMU
Hello Stefan,
On 09/05/2014 12:29 PM, Stefan Hajnoczi wrote:
On Thu, Sep 04, 2014 at 02:51:01PM +0200, David Marchand wrote:
diff --git a/contrib/ivshmem-client/ivshmem-client.c
b/contrib/ivshmem-client/ivshmem-client.c
index ad210c8..0c4e016 100644
--- a/contrib/ivshmem-client/ivshmem-client.c
+++ b/contrib/ivshmem-client/ivshmem-client.c
@@ -184,10 +184,18 @@ ivshmem_client_connect(IvshmemClient *client)
goto err_close;
}
-/* first, we expect our index + a fd == -1 */
+/* first, we expect a protocol version */
+if (read_one_msg(client, &tmp, &fd) < 0 ||
+(tmp != IVSHMEM_PROTOCOL_VERSION) || fd != -1) {
+debug_log(client, "cannot read from server\n");
+goto err_close;
+}
+debug_log(client, "our_id=%ld\n", client->local.id);
This debug_log() is probably not intentional. local.id will always be
-1 here so the output is not useful.
Yes, this is most likely a merge/rebase issue.
Will remove this.
+static void ivshmem_check_version(void *opaque, const uint8_t * buf, int flags)
+{
+IVShmemState *s = opaque;
+PCIDevice *dev = PCI_DEVICE(s);
+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
ivhsmem-"
Hum, typo: ivhs -> ivsh.
+"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, finish init and switch to real chardev "
+"handler\n");
+
+pci_register_bar(dev, 2, s->ivshmem_attr, &s->bar);
Not sure if it is okay to delay PCI initialization to a fd hander
callback.
If the version message is too slow the guest could see the PCI adapter
without the BAR!
Did you move this code in order to prevent the guest from accessing the
device before it has connected to the server? Perhaps the device needs
a state field that tracks whether or not it is ready for operation. Any
access before RUNNING state is reached will be ignored (?).
Yes, exactly.
There already is a synchronisation mechanism described in the documentation:
"When using the server, since the server is a separate process, the VM
ID will only be set when the device is ready (shared memory is received
from the server and accessible via the device). If the device is not
ready, the IVPosition will return -1.
Applications should ensure that they have a valid VM ID before accessing
the shared memory."
So actually, this move is unneeded if ivshmem users comply to this.
I will let the init stuff (pci_register_bar + gmalloc) where it was
before, ivshmem_check_version will only switch the chardev handler.
What do you think about this ?
Thanks.
--
David Marchand
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/3] ivshmem: add check on protocol version in QEMU
On Thu, Sep 04, 2014 at 02:51:01PM +0200, David Marchand wrote:
> diff --git a/contrib/ivshmem-client/ivshmem-client.c
> b/contrib/ivshmem-client/ivshmem-client.c
> index ad210c8..0c4e016 100644
> --- a/contrib/ivshmem-client/ivshmem-client.c
> +++ b/contrib/ivshmem-client/ivshmem-client.c
> @@ -184,10 +184,18 @@ ivshmem_client_connect(IvshmemClient *client)
> goto err_close;
> }
>
> -/* first, we expect our index + a fd == -1 */
> +/* first, we expect a protocol version */
> +if (read_one_msg(client, &tmp, &fd) < 0 ||
> +(tmp != IVSHMEM_PROTOCOL_VERSION) || fd != -1) {
> +debug_log(client, "cannot read from server\n");
> +goto err_close;
> +}
> +debug_log(client, "our_id=%ld\n", client->local.id);
This debug_log() is probably not intentional. local.id will always be
-1 here so the output is not useful.
> +static void ivshmem_check_version(void *opaque, const uint8_t * buf, int
> flags)
> +{
> +IVShmemState *s = opaque;
> +PCIDevice *dev = PCI_DEVICE(s);
> +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
> ivhsmem-"
> +"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, finish init and switch to real
> chardev "
> +"handler\n");
> +
> +pci_register_bar(dev, 2, s->ivshmem_attr, &s->bar);
Not sure if it is okay to delay PCI initialization to a fd hander
callback.
If the version message is too slow the guest could see the PCI adapter
without the BAR!
Did you move this code in order to prevent the guest from accessing the
device before it has connected to the server? Perhaps the device needs
a state field that tracks whether or not it is ready for operation. Any
access before RUNNING state is reached will be ignored (?).
pgpiIFphsdeyj.pgp
Description: PGP signature
[PATCH v5 3/3] ivshmem: add check on protocol version in QEMU
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
---
contrib/ivshmem-client/ivshmem-client.c | 14 +++---
contrib/ivshmem-client/ivshmem-client.h |1 +
contrib/ivshmem-server/ivshmem-server.c |7 +
contrib/ivshmem-server/ivshmem-server.h |1 +
docs/specs/ivshmem_device_spec.txt |9 ---
hw/misc/ivshmem.c | 43 ---
include/hw/misc/ivshmem.h | 17
7 files changed, 77 insertions(+), 15 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 ad210c8..0c4e016 100644
--- a/contrib/ivshmem-client/ivshmem-client.c
+++ b/contrib/ivshmem-client/ivshmem-client.c
@@ -184,10 +184,18 @@ ivshmem_client_connect(IvshmemClient *client)
goto err_close;
}
-/* first, we expect our index + a fd == -1 */
+/* first, we expect a protocol version */
+if (read_one_msg(client, &tmp, &fd) < 0 ||
+(tmp != IVSHMEM_PROTOCOL_VERSION) || fd != -1) {
+debug_log(client, "cannot read from server\n");
+goto err_close;
+}
+debug_log(client, "our_id=%ld\n", client->local.id);
+
+/* then, we expect our index + a fd == -1 */
if (read_one_msg(client, &client->local.id, &fd) < 0 ||
client->local.id < 0 || fd != -1) {
-debug_log(client, "cannot read from server\n");
+debug_log(client, "cannot read from server (2)\n");
goto err_close;
}
debug_log(client, "our_id=%ld\n", client->local.id);
@@ -196,7 +204,7 @@ ivshmem_client_connect(IvshmemClient *client)
* is not used */
if (read_one_msg(client, &tmp, &fd) < 0 ||
tmp != -1 || fd < 0) {
-debug_log(client, "cannot read from server (2)\n");
+debug_log(client, "cannot read from server (3)\n");
goto err_close;
}
debug_log(client, "shm_fd=%d\n", fd);
diff --git a/contrib/ivshmem-client/ivshmem-client.h
b/contrib/ivshmem-client/ivshmem-client.h
index 45f2b64..8d6ab35 100644
--- a/contrib/ivshmem-client/ivshmem-client.h
+++ b/contrib/ivshmem-client/ivshmem-client.h
@@ -23,6 +23,7 @@
#include
#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 f441da7..670c58c 100644
--- a/contrib/ivshmem-server/ivshmem-server.c
+++ b/contrib/ivshmem-server/ivshmem-server.c
@@ -99,6 +99,13 @@ send_initial_info(IvshmemServer *server, IvshmemServerPeer
*peer)
{
int ret;
+/* send our protool version first */
+ret = send_one_msg(peer->sock_fd, IVSHMEM_PROTOCOL_VERSION, -1);
+if (ret < 0) {
+debug_log(server, "cannot send version: %s\n", strerror(errno));
+return -1;
+}
+
/* send the peer id to the client */
ret = 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 5ccc7af..e76e4fe 100644
--- a/contrib/ivshmem-server/ivshmem-server.h
+++ b/contrib/ivshmem-server/ivshmem-server.h
@@ -30,6 +30,7 @@
#include
#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.
eventf
