[Qemu-devel] [PATCH 0/2] RfC: xenner (aka xen emulation) bits.

[Qemu-devel] [PATCH 0/2] RfC: xenner (aka xen emulation) bits.

[Gerd Hoffmann]

  Hi folks,

These are the very first bits of the xen emulation, codenamed xenner.
It brings an xen event channel implementation for qemu.  It isn't very
useful alone.  This post is just for an early review of the way I'm
planing to implement this.

The first patch implements the event channels and the switch between
libxenctl / qemu internal using function pointers.  The second patch
makes the backends go through the function pointers.

Comments are welcome.

cheers,
  Gerd

[Qemu-devel] [PATCH 1/2] xenner: add event channel implementation.

[Gerd Hoffmann]

This adds a xen event channel implementation to qemu, intented to be
used by xenner (aka xen emulation).

The patch also adds a XenEvtOps struct with function pointers for the
xc_evtchn_* family, which is used to switch between libxenctrl and the
qemu implementation at runtime.  By default libxenctrl is used.
---
 Makefile.target          |    1 +
 hw/xen_interfaces.h      |   27 +++
 hw/xen_machine_pv.c      |    2 +
 hw/xenner_libxc_evtchn.c |  396 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 426 insertions(+), 0 deletions(-)
 create mode 100644 hw/xen_interfaces.h
 create mode 100644 hw/xenner_libxc_evtchn.c

diff --git a/Makefile.target b/Makefile.target
index 5c97874..b88fd8f 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -521,6 +521,7 @@ endif
 # xen backend driver support
 XEN_OBJS := xen_machine_pv.o xen_backend.o xen_devconfig.o xen_domainbuild.o
 XEN_OBJS += xen_console.o xen_framebuffer.o xen_disk.o xen_nic.o
+XEN_OBJS += xenner_libxc_evtchn.o
 ifeq ($(CONFIG_XEN), yes)
   OBJS += $(XEN_OBJS)
   LIBS += $(XEN_LIBS)
diff --git a/hw/xen_interfaces.h b/hw/xen_interfaces.h
new file mode 100644
index 0000000..869b382
--- /dev/null
+++ b/hw/xen_interfaces.h
@@ -0,0 +1,27 @@
+#ifndef QEMU_XEN_INTERFACES_H
+#define QEMU_XEN_INTERFACES_H 1
+
+#include "xen_common.h"
+
+/* ------------------------------------------------------------- */
+/* xen event channel interface                                   */
+
+struct XenEvtOps {
+    int (*open)(void);
+    int (*domid)(int xce_handle, int domid);
+    int (*close)(int xce_handle);
+    int (*fd)(int xce_handle);
+    int (*notify)(int xce_handle, evtchn_port_t port);
+    evtchn_port_or_error_t (*bind_unbound_port)(int xce_handle, int domid);
+    evtchn_port_or_error_t (*bind_interdomain)(int xce_handle, int domid,
+					       evtchn_port_t remote_port);
+    evtchn_port_or_error_t (*bind_virq)(int xce_handle, unsigned int virq);
+    int (*unbind)(int xce_handle, evtchn_port_t port);
+    evtchn_port_or_error_t (*pending)(int xce_handle);
+    int (*unmask)(int xce_handle, evtchn_port_t port);
+};
+extern struct XenEvtOps xc_evtchn;
+
+void xenner_evtchn_init(void);
+
+#endif /* QEMU_XEN_INTERFACES_H */
diff --git a/hw/xen_machine_pv.c b/hw/xen_machine_pv.c
index 9c67848..5d755f5 100644
--- a/hw/xen_machine_pv.c
+++ b/hw/xen_machine_pv.c
@@ -26,6 +26,7 @@
 #include "boards.h"
 
 #include "xen_backend.h"
+#include "xen_interfaces.h"
 #include "xen_domainbuild.h"
 
 /* -------------------------------------------------------------------- */
@@ -80,6 +81,7 @@ static int xen_init(void)
     if (!xen_detect()) {
         fprintf(stderr, "%s: emulating Xen\n", __FUNCTION__);
         xen_emulate = 1;
+        xenner_evtchn_init();
     }
 
     if (-1 == xen_be_init()) {
diff --git a/hw/xenner_libxc_evtchn.c b/hw/xenner_libxc_evtchn.c
new file mode 100644
index 0000000..f0c179d
--- /dev/null
+++ b/hw/xenner_libxc_evtchn.c
@@ -0,0 +1,396 @@
+#include <xenctrl.h>
+
+#include "hw.h"
+#include "xen_interfaces.h"
+
+/* ------------------------------------------------------------- */
+
+struct evtpriv;
+
+struct port {
+    struct evtpriv   *priv;
+    struct port      *peer;
+    int              port;
+};
+
+struct domain {
+    int              domid;
+    int              refcount;
+    struct port      p[NR_EVENT_CHANNELS];
+    TAILQ_ENTRY(domain) list;
+};
+static TAILQ_HEAD(domain_head, domain) domains = TAILQ_HEAD_INITIALIZER(domains);
+
+struct evtpriv {
+    int                      fd_read, fd_write;
+    struct domain            *domain;
+    int                      ports;
+    TAILQ_ENTRY(evtpriv)     list;
+};
+static TAILQ_HEAD(evtpriv_head, evtpriv) privs = TAILQ_HEAD_INITIALIZER(privs);
+
+static int debug = 0;
+
+/* ------------------------------------------------------------- */
+
+static struct evtpriv *getpriv(int handle)
+{
+    struct evtpriv *priv;
+
+    TAILQ_FOREACH(priv, &privs, list) {
+	if (priv->fd_read == handle)
+	    return priv;
+    }
+    return NULL;
+}
+
+static struct domain *get_domain(int domid)
+{
+    struct domain *domain;
+
+    TAILQ_FOREACH(domain, &domains, list) {
+	if (domain->domid == domid)
+	    goto done;
+    }
+
+    domain = qemu_mallocz(sizeof(*domain));
+    if (NULL == domain)
+	return NULL;
+    if (domid)
+	domain->domid = domid;
+    TAILQ_INSERT_TAIL(&domains, domain, list);
+    if (debug)
+	fprintf(stderr, "xen ev:  ?: new domain id %d\n", domain->domid);
+
+done:
+    domain->refcount++;
+    return domain;
+}
+
+static void put_domain(struct domain *domain)
+{
+    domain->refcount--;
+    if (domain->refcount)
+	return;
+    if (debug)
+	fprintf(stderr, "xen ev:  ?: del domain id %d\n", domain->domid);
+    TAILQ_REMOVE(&domains, domain, list);
+    qemu_free(domain);
+}
+
+static struct port *alloc_port(struct evtpriv *priv, char *reason)
+{
+    struct port *p = NULL;
+    int i;
+
+    for (i = 1; i < NR_EVENT_CHANNELS; i++) {
+	if (NULL != priv->domain->p[i].priv)
+	    continue;
+	p = priv->domain->p+i;
+	p->port = i;
+	p->priv = priv;
+	priv->ports++;
+	if (debug)
+	    fprintf(stderr, "xen ev:%3d: alloc port %d, domain %d (%s)\n",
+		    priv->fd_read, p->port, priv->domain->domid, reason);
+	return p;
+    }
+    return NULL;
+}
+
+static void bind_port_peer(struct port *p, int domid, int port)
+{
+    struct domain *domain;
+    struct port *o;
+    char *msg = "ok";
+
+    domain = get_domain(domid);
+    o = domain->p+port;
+    if (!o->priv) {
+	msg = "peer not allocated";
+    } else if (o->peer) {
+	msg = "peer already bound";
+    } else if (p->peer) {
+	msg = "port already bound";
+    } else {
+	o->peer = p;
+	p->peer = o;
+    }
+    if (debug)
+	fprintf(stderr, "xen ev:%3d: bind port %d domain %d  <->  port %d domain %d : %s\n",
+		p->priv->fd_read,
+		p->port, p->priv->domain->domid,
+		port, domid, msg);
+    put_domain(domain);
+}
+
+static void unbind_port(struct port *p)
+{
+    struct port *o;
+
+    o = p->peer;
+    if (o) {
+	if (debug)
+	    fprintf(stderr,"xen ev:%3d: unbind port %d domain %d  <->  port %d domain %d\n",
+		    p->priv->fd_read,
+		    p->port, p->priv->domain->domid,
+		    o->port, o->priv->domain->domid);
+	o->peer = NULL;
+	p->peer = NULL;
+    }
+}
+
+static void notify_send_peer(struct port *peer)
+{
+    uint32_t evtchn = peer->port;
+    write(peer->priv->fd_write, &evtchn, sizeof(evtchn));
+}
+
+static void notify_port(struct port *p)
+{
+    if (p->peer) {
+	notify_send_peer(p->peer);
+	if (debug > 1)
+	    fprintf(stderr, "xen ev:%3d: notify port %d domain %d  ->  port %d domain %d\n",
+		    p->priv->fd_read, p->port, p->priv->domain->domid,
+		    p->peer->port, p->peer->priv->domain->domid);
+    } else {
+	if (debug)
+	    fprintf(stderr,"xen ev:%3d: notify port %d domain %d  ->  unconnected\n",
+		    p->priv->fd_read, p->port, p->priv->domain->domid);
+    }
+}
+
+static void unmask_port(struct port *p)
+{
+    /* nothing to do */
+}
+
+static void release_port(struct port *p)
+{
+    if (debug)
+	fprintf(stderr,"xen ev:%3d: release port %d, domain %d\n",
+		p->priv->fd_read, p->port, p->priv->domain->domid);
+    unbind_port(p);
+    p->priv->ports--;
+    p->port = 0;
+    p->priv = 0;
+}
+
+/* ------------------------------------------------------------- */
+
+static int qemu_open(void)
+{
+    struct evtpriv *priv;
+    int fd[2];
+
+    priv = qemu_mallocz(sizeof(*priv));
+    if (NULL == priv)
+	goto err;
+    TAILQ_INSERT_TAIL(&privs, priv, list);
+
+    if (-1 == pipe(fd))
+	goto err;
+    priv->fd_read  = fd[0];
+    priv->fd_write = fd[1];
+    fcntl(priv->fd_read,F_SETFL,O_NONBLOCK);
+
+    priv->domain = get_domain(0);
+    return priv->fd_read;
+
+err:
+    qemu_free(priv);
+    return -1;
+}
+
+static int qemu_close(int handle)
+{
+    struct evtpriv *priv = getpriv(handle);
+    struct port *p;
+    int i;
+
+    if (!priv)
+	return -1;
+
+    for (i = 1; i < NR_EVENT_CHANNELS; i++) {
+	p = priv->domain->p+i;
+	if (priv != p->priv)
+	    continue;
+	release_port(p);
+    }
+    put_domain(priv->domain);
+
+    close(priv->fd_read);
+    close(priv->fd_write);
+    TAILQ_REMOVE(&privs, priv, list);
+    qemu_free(priv);
+    return 0;
+}
+
+static int qemu_fd(int handle)
+{
+    struct evtpriv *priv = getpriv(handle);
+
+    if (!priv)
+	return -1;
+    return priv->fd_read;
+}
+
+static int qemu_notify(int handle, evtchn_port_t port)
+{
+    struct evtpriv *priv = getpriv(handle);
+    struct port *p;
+
+    if (!priv)
+	return -1;
+    if (port >= NR_EVENT_CHANNELS)
+	return -1;
+    p = priv->domain->p + port;
+    notify_port(p);
+    return -1;
+}
+
+static evtchn_port_or_error_t qemu_bind_unbound_port(int handle, int domid)
+{
+    struct evtpriv *priv = getpriv(handle);
+    struct port *p;
+
+    if (!priv)
+	return -1;
+    p = alloc_port(priv, "unbound");
+    if (!p)
+	return -1;
+    return p->port;
+}
+
+static evtchn_port_or_error_t qemu_bind_interdomain(int handle, int domid,
+						    evtchn_port_t remote_port)
+{
+    struct evtpriv *priv = getpriv(handle);
+    struct port *p;
+
+    if (!priv)
+	return -1;
+    if (remote_port >= NR_EVENT_CHANNELS)
+	return -1;
+    p = alloc_port(priv, "interdomain");
+    if (!p)
+	return -1;
+    bind_port_peer(p, domid, remote_port);
+    return p->port;
+}
+
+static evtchn_port_or_error_t qemu_bind_virq(int handle, unsigned int virq)
+{
+    struct evtpriv *priv = getpriv(handle);
+    struct port *p;
+
+    if (!priv)
+	return -1;
+    p = alloc_port(priv, "virq");
+    if (!p)
+	return -1;
+    /*
+     * FIXME: port not linked
+     * (but virq doesn't go this route anyway I think)
+     */
+    return p->port;
+}
+
+static int qemu_unbind(int handle, evtchn_port_t port)
+{
+    struct evtpriv *priv = getpriv(handle);
+    struct port *p;
+
+    if (!priv)
+	return -1;
+    if (port >= NR_EVENT_CHANNELS)
+	return -1;
+    p = priv->domain->p + port;
+    unbind_port(p);
+    release_port(p);
+    return 0;
+}
+
+static evtchn_port_or_error_t qemu_pending(int handle)
+{
+    struct evtpriv *priv = getpriv(handle);
+    uint32_t evtchn;
+    int rc;
+
+    if (!priv)
+	return -1;
+    rc = read(priv->fd_read, &evtchn, sizeof(evtchn));
+    if (rc != sizeof(evtchn))
+	return -1;
+    return evtchn;
+}
+
+static int qemu_unmask(int handle, evtchn_port_t port)
+{
+    struct evtpriv *priv = getpriv(handle);
+    struct port *p;
+
+    if (!priv)
+	return -1;
+    if (port >= NR_EVENT_CHANNELS)
+	return -1;
+    p = priv->domain->p + port;
+    unmask_port(p);
+    return 0;
+}
+
+static int qemu_domid(int handle, int domid)
+{
+    struct evtpriv *priv = getpriv(handle);
+
+    if (!priv)
+	return -1;
+    if (priv->ports)
+	return -1;
+    put_domain(priv->domain);
+    priv->domain = get_domain(domid);
+    return 0;
+}
+
+static struct XenEvtOps xenner_evtchn = {
+    .open               = qemu_open,
+    .domid              = qemu_domid,
+    .close              = qemu_close,
+    .fd                 = qemu_fd,
+    .notify             = qemu_notify,
+    .bind_unbound_port  = qemu_bind_unbound_port,
+    .bind_interdomain   = qemu_bind_interdomain,
+    .bind_virq          = qemu_bind_virq,
+    .unbind             = qemu_unbind,
+    .pending            = qemu_pending,
+    .unmask             = qemu_unmask,
+};
+
+/* ------------------------------------------------------------- */
+
+static int xc_evtchn_domid(int handle, int domid)
+{
+    return -1;
+}
+
+struct XenEvtOps xc_evtchn = {
+    .open               = xc_evtchn_open,
+    .domid              = xc_evtchn_domid,
+    .close              = xc_evtchn_close,
+    .fd                 = xc_evtchn_fd,
+    .notify             = xc_evtchn_notify,
+    .bind_unbound_port  = xc_evtchn_bind_unbound_port,
+    .bind_interdomain   = xc_evtchn_bind_interdomain,
+    .bind_virq          = xc_evtchn_bind_virq,
+    .unbind             = xc_evtchn_unbind,
+    .pending            = xc_evtchn_pending,
+    .unmask             = xc_evtchn_unmask,
+};
+
+/* ------------------------------------------------------------- */
+
+void xenner_evtchn_init(void)
+{
+    xc_evtchn = xenner_evtchn;
+}
-- 
1.5.5.1

[Qemu-devel] [PATCH 2/2] xenner: use evtchn function pointers in the backends.

[Gerd Hoffmann]

---
 hw/xen_backend.c |   27 ++++++++++++++-------------
 1 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/hw/xen_backend.c b/hw/xen_backend.c
index e926a01..fd07c03 100644
--- a/hw/xen_backend.c
+++ b/hw/xen_backend.c
@@ -39,6 +39,7 @@
 #include "hw.h"
 #include "qemu-char.h"
 #include "xen_backend.h"
+#include "xen_interfaces.h"
 
 /* ------------------------------------------------------------- */
 
@@ -200,19 +201,19 @@ static struct XenDevice *xen_be_get_xendev(char *type, int dom, int dev,
     xendev->debug      = debug;
     xendev->local_port = -1;
 
-    xendev->evtchndev = xc_evtchn_open();
+    xendev->evtchndev = xc_evtchn.open();
     if (xendev->evtchndev < 0) {
 	fprintf(stderr, "can't open evtchn device\n");
 	qemu_free(xendev);
 	return NULL;
     }
-    fcntl(xc_evtchn_fd(xendev->evtchndev), F_SETFD, FD_CLOEXEC);
+    fcntl(xc_evtchn.fd(xendev->evtchndev), F_SETFD, FD_CLOEXEC);
 
     if (ops->flags & DEVOPS_FLAG_NEED_GNTDEV) {
 	xendev->gnttabdev = xc_gnttab_open();
 	if (xendev->gnttabdev < 0) {
 	    fprintf(stderr, "can't open gnttab device\n");
-	    xc_evtchn_close(xendev->evtchndev);
+	    xc_evtchn.close(xendev->evtchndev);
 	    qemu_free(xendev);
 	    return NULL;
 	}
@@ -260,7 +261,7 @@ static struct XenDevice *xen_be_del_xendev(int dom, int dev)
 	}
 
 	if (xendev->evtchndev >= 0)
-	    xc_evtchn_close(xendev->evtchndev);
+	    xc_evtchn.close(xendev->evtchndev);
 	if (xendev->gnttabdev >= 0)
 	    xc_gnttab_close(xendev->gnttabdev);
 
@@ -595,13 +596,13 @@ static void xen_be_evtchn_event(void *opaque)
     struct XenDevice *xendev = opaque;
     evtchn_port_t port;
 
-    port = xc_evtchn_pending(xendev->evtchndev);
+    port = xc_evtchn.pending(xendev->evtchndev);
     if (port != xendev->local_port) {
-	xen_be_printf(xendev, 0, "xc_evtchn_pending returned %d (expected %d)\n",
+	xen_be_printf(xendev, 0, "xc_evtchn.pending returned %d (expected %d)\n",
 		      port, xendev->local_port);
 	return;
     }
-    xc_evtchn_unmask(xendev->evtchndev, port);
+    xc_evtchn.unmask(xendev->evtchndev, port);
 
     if (xendev->ops->event)
 	xendev->ops->event(xendev);
@@ -644,14 +645,14 @@ int xen_be_bind_evtchn(struct XenDevice *xendev)
 {
     if (xendev->local_port != -1)
 	return 0;
-    xendev->local_port = xc_evtchn_bind_interdomain
+    xendev->local_port = xc_evtchn.bind_interdomain
 	(xendev->evtchndev, xendev->dom, xendev->remote_port);
     if (-1 == xendev->local_port) {
-	xen_be_printf(xendev, 0, "xc_evtchn_bind_interdomain failed\n");
+	xen_be_printf(xendev, 0, "xc_evtchn.bind_interdomain failed\n");
 	return -1;
     }
     xen_be_printf(xendev, 2, "bind evtchn port %d\n", xendev->local_port);
-    qemu_set_fd_handler(xc_evtchn_fd(xendev->evtchndev),
+    qemu_set_fd_handler(xc_evtchn.fd(xendev->evtchndev),
 			xen_be_evtchn_event, NULL, xendev);
     return 0;
 }
@@ -660,15 +661,15 @@ void xen_be_unbind_evtchn(struct XenDevice *xendev)
 {
     if (xendev->local_port == -1)
 	return;
-    qemu_set_fd_handler(xc_evtchn_fd(xendev->evtchndev), NULL, NULL, NULL);
-    xc_evtchn_unbind(xendev->evtchndev, xendev->local_port);
+    qemu_set_fd_handler(xc_evtchn.fd(xendev->evtchndev), NULL, NULL, NULL);
+    xc_evtchn.unbind(xendev->evtchndev, xendev->local_port);
     xen_be_printf(xendev, 2, "unbind evtchn port %d\n", xendev->local_port);
     xendev->local_port = -1;
 }
 
 int xen_be_send_notify(struct XenDevice *xendev)
 {
-    return xc_evtchn_notify(xendev->evtchndev, xendev->local_port);
+    return xc_evtchn.notify(xendev->evtchndev, xendev->local_port);
 }
 
 /*
-- 
1.5.5.1

Re: [Qemu-devel] [PATCH 1/2] xenner: add event channel implementation.

[Anthony Liguori]

Gerd Hoffmann wrote:
> This adds a xen event channel implementation to qemu, intented to be
> used by xenner (aka xen emulation).
>
> The patch also adds a XenEvtOps struct with function pointers for the
> xc_evtchn_* family, which is used to switch between libxenctrl and the
> qemu implementation at runtime.  By default libxenctrl is used.
>   

I suppose the QEMU implementation is to eventually eliminate the need 
for libxc?  Do you also plan on doing a XenStore implementation within QEMU?

> ---
>  Makefile.target          |    1 +
>  hw/xen_interfaces.h      |   27 +++
>  hw/xen_machine_pv.c      |    2 +
>  hw/xenner_libxc_evtchn.c |  396 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 426 insertions(+), 0 deletions(-)
>  create mode 100644 hw/xen_interfaces.h
>  create mode 100644 hw/xenner_libxc_evtchn.c
>
> diff --git a/Makefile.target b/Makefile.target
> index 5c97874..b88fd8f 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -521,6 +521,7 @@ endif
>  # xen backend driver support
>  XEN_OBJS := xen_machine_pv.o xen_backend.o xen_devconfig.o xen_domainbuild.o
>  XEN_OBJS += xen_console.o xen_framebuffer.o xen_disk.o xen_nic.o
> +XEN_OBJS += xenner_libxc_evtchn.o
>   

Historically, we didn't have directories in QEMU because we used CVS and 
directories are a nightmare.  With the shear number of files being added 
for Xen, it probably makes sense to stick them all in a directory.

>  ifeq ($(CONFIG_XEN), yes)
>    OBJS += $(XEN_OBJS)
>    LIBS += $(XEN_LIBS)
> diff --git a/hw/xen_interfaces.h b/hw/xen_interfaces.h
> new file mode 100644
> index 0000000..869b382
> --- /dev/null
> +++ b/hw/xen_interfaces.h
> @@ -0,0 +1,27 @@
> +#ifndef QEMU_XEN_INTERFACES_H
> +#define QEMU_XEN_INTERFACES_H 1
>   

Minor nit, make sure to have copyrights in all of your files.

> +static void bind_port_peer(struct port *p, int domid, int port)
> +{
> +    struct domain *domain;
> +    struct port *o;
> +    char *msg = "ok";
>   

Should be const char *.

> +    domain = get_domain(domid);
> +    o = domain->p+port;
> +    if (!o->priv) {
> +	msg = "peer not allocated";
> +    } else if (o->peer) {
> +	msg = "peer already bound";
> +    } else if (p->peer) {
> +	msg = "port already bound";
> +    } else {
> +	o->peer = p;
> +	p->peer = o;
> +    }
>   

Watch the whitespace (you've got tabs all over).

> +    if (debug)
> +	fprintf(stderr, "xen ev:%3d: bind port %d domain %d  <->  port %d domain %d : %s\n",
> +		p->priv->fd_read,
> +		p->port, p->priv->domain->domid,
> +		port, domid, msg);
> +    put_domain(domain);
> +}
> +
> +static void unbind_port(struct port *p)
> +{
> +    struct port *o;
> +
> +    o = p->peer;
> +    if (o) {
> +	if (debug)
> +	    fprintf(stderr,"xen ev:%3d: unbind port %d domain %d  <->  port %d domain %d\n",
> +		    p->priv->fd_read,
> +		    p->port, p->priv->domain->domid,
> +		    o->port, o->priv->domain->domid);
> +	o->peer = NULL;
> +	p->peer = NULL;
> +    }
> +}
> +
> +static void notify_send_peer(struct port *peer)
> +{
> +    uint32_t evtchn = peer->port;
> +    write(peer->priv->fd_write, &evtchn, sizeof(evtchn));
>   

Should be unix_write and should deal with errors.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 1/2] xenner: add event channel implementation.

[Gerd Hoffmann]

Anthony Liguori wrote:
> Gerd Hoffmann wrote:
>> The patch also adds a XenEvtOps struct with function pointers for the
>> xc_evtchn_* family, which is used to switch between libxenctrl and the
>> qemu implementation at runtime.  By default libxenctrl is used.
> 
> I suppose the QEMU implementation is to eventually eliminate the need
> for libxc?

Yes, for xen emulation (aka xenner).
When running on Xen libxc is still needed of course.

> Do you also plan on doing a XenStore implementation within QEMU?

Yes.

>> @@ -521,6 +521,7 @@ endif
>>  # xen backend driver support
>>  XEN_OBJS := xen_machine_pv.o xen_backend.o xen_devconfig.o
>> xen_domainbuild.o
>>  XEN_OBJS += xen_console.o xen_framebuffer.o xen_disk.o xen_nic.o
>> +XEN_OBJS += xenner_libxc_evtchn.o
> 
> Historically, we didn't have directories in QEMU because we used CVS and
> directories are a nightmare.  With the shear number of files being added
> for Xen, it probably makes sense to stick them all in a directory.

Hmm, the xen guys vetoed file renames, so I don't feel like sticking
stuff used by xen somewhere else.  For the emulation bits aka
xenner*.[ch] it should be no problem though.  New xenner/ toplevel
directory?

> Minor nit, make sure to have copyrights in all of your files.
> [ ... more review comments ... ]

Thanks, I'll fix it up.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/2] xenner: add event channel implementation.

[Gerd Hoffmann]

  Hi,

>>>  XEN_OBJS += xen_console.o xen_framebuffer.o xen_disk.o xen_nic.o
>>> +XEN_OBJS += xenner_libxc_evtchn.o
>> Historically, we didn't have directories in QEMU because we used CVS and
>> directories are a nightmare.  With the shear number of files being added
>> for Xen, it probably makes sense to stick them all in a directory.
> 
> Hmm, the xen guys vetoed file renames, so I don't feel like sticking
> stuff used by xen somewhere else.  For the emulation bits aka
> xenner*.[ch] it should be no problem though.  New xenner/ toplevel
> directory?

Comments on this one?

Another idea:  Do the initial merge in hw/, and later on, after the
qemu-xen tree catched up, move all xen (and xen emulation) bits to
another directory.  That would keep the scm revision history intact for
the Xen guys.

I also think we need to organize the source -- especially the header
files -- a bit better if we start to separate the code into more
subdirectories.  And I'd prefer to have some agreement on how the source
tree should look like before starting to shuffle around files, so I have
to do it only once ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/2] xenner: add event channel implementation.

[Anthony Liguori]

Gerd Hoffmann wrote:
>   Hi,
>   
> Comments on this one?
>
> Another idea:  Do the initial merge in hw/, and later on, after the
> qemu-xen tree catched up, move all xen (and xen emulation) bits to
> another directory.  That would keep the scm revision history intact for
> the Xen guys.
>
> I also think we need to organize the source -- especially the header
> files -- a bit better if we start to separate the code into more
> subdirectories.  And I'd prefer to have some agreement on how the source
> tree should look like before starting to shuffle around files, so I have
> to do it only once ...
>   

Let's focus on merging and then once it's merged, we can reorganize 
files.  I still haven't seen any comments from the Xen guys wrt this 
patch series.  Has there been discussion on xen-devel that I missed?

Regards,

Anthony Liguori

> cheers,
>   Gerd
>   

Re: [Xen-devel] Re: [Qemu-devel] [PATCH 1/2] xenner: add event channel implementation.

[Keir Fraser]

On 25/8/08 15:15, "Anthony Liguori" <anthony@codemonkey.ws> wrote:

>> Another idea:  Do the initial merge in hw/, and later on, after the
>> qemu-xen tree catched up, move all xen (and xen emulation) bits to
>> another directory.  That would keep the scm revision history intact for
>> the Xen guys.
>> 
>> I also think we need to organize the source -- especially the header
>> files -- a bit better if we start to separate the code into more
>> subdirectories.  And I'd prefer to have some agreement on how the source
>> tree should look like before starting to shuffle around files, so I have
>> to do it only once ...
>>   
> 
> Let's focus on merging and then once it's merged, we can reorganize
> files.  I still haven't seen any comments from the Xen guys wrt this
> patch series.  Has there been discussion on xen-devel that I missed?

With regard to this Xenner patch series, I don't think we have a strong
opinion. The changes are not particularly relevant to us. As long as the
orthogonal qemu-on-xen modes that we *do* care about can coexist happily
alongside, we'll be happy.

 -- Keir

Re: [Xen-devel] Re: [Qemu-devel] [PATCH 1/2] xenner: add event channel implementation.

[Anthony Liguori]

Hi Keir,

Keir Fraser wrote:
> On 25/8/08 15:15, "Anthony Liguori" <anthony@codemonkey.ws> wrote:
>
> With regard to this Xenner patch series, I don't think we have a strong
> opinion. The changes are not particularly relevant to us. As long as the
> orthogonal qemu-on-xen modes that we *do* care about can coexist happily
> alongside, we'll be happy.
>   

Sorry, I was actually referring to the other qemu-dm PV domain support 
patch series.  That's the one that I'm looking for input from ya'll on.  
Now that 3.3.0 has been released, it should be possible for you guys to 
consider merging the patches into qemu-dm, right?

Regards,

Anthony Liguori

>  -- Keir
>
>
>   

Re: [Xen-devel] Re: [Qemu-devel] [PATCH 1/2] xenner: add event channel implementation.

[Keir Fraser]

On 25/8/08 18:57, "Anthony Liguori" <anthony@codemonkey.ws> wrote:

> Keir Fraser wrote:
>> On 25/8/08 15:15, "Anthony Liguori" <anthony@codemonkey.ws> wrote:
>> 
>> With regard to this Xenner patch series, I don't think we have a strong
>> opinion. The changes are not particularly relevant to us. As long as the
>> orthogonal qemu-on-xen modes that we *do* care about can coexist happily
>> alongside, we'll be happy.
> 
> Sorry, I was actually referring to the other qemu-dm PV domain support
> patch series.  That's the one that I'm looking for input from ya'll on.
> Now that 3.3.0 has been released, it should be possible for you guys to
> consider merging the patches into qemu-dm, right?

We'll certainly be sync'ing with upstream qemu now, yes. I'm not sure what
Ian Jackson's plans are with regard to Gerd's patches. I think he was hoping
that Gerd would post patches to xen-devel against his tree (being an
existing and actively maintained and tested Xen patchset) and then we would
from there submit to upstream. Afaics there's some workflow or patchflow to
be worked out here: I can't see why we would take Gerd's patches wholesale
when we have a working patchset already. It's a bank holiday here in the UK
today, but I expect Ian will give you his opinion tomorrow!

 -- Keir

Re: [Xen-devel] Re: [Qemu-devel] [PATCH 1/2] xenner: add event channel implementation.

[Gerd Hoffmann]

Keir Fraser wrote:
> We'll certainly be sync'ing with upstream qemu now, yes. I'm not sure what
> Ian Jackson's plans are with regard to Gerd's patches. I think he was hoping
> that Gerd would post patches to xen-devel against his tree (being an
> existing and actively maintained and tested Xen patchset) and then we would
> from there submit to upstream.

There are two patchsets out there:

#1 for upstream qemu:
   http://kraxel.fedorapeople.org/patches/qemu-upstream/

#2 for qemu-xen:
   http://kraxel.fedorapeople.org/patches/qemu-xen/

The patchsets are largely identical, thats why I don't post both.
I usually post only the upstream version and include a pointer to the
qemu-xen patches in the intro text for the patchset.

Differences come (a) from the fact that qemu-xen is old (these should go
away once Ian synced with upstream) and (b) the slightly different
ordering to make the qemu-xen patchset bisection-friendly.

> Afaics there's some workflow or patchflow to
> be worked out here:

Workflow could look like this:

 (1) Ian merges upstream into qemu-xen.
 (2) I'll rebase my patches to the resulting tree.
 (3) Merge both patchsets, into the trees.

That should result in almost identical hw/xen* files in both trees, so
we don't end up with a big mess when Ian merges again.

> I can't see why we would take Gerd's patches wholesale
> when we have a working patchset already.

Which patchset you are refering to?  As far I know Ian & Samuel are
focusing on getting the changes to generic qemu code upstream (such as
serial and ide fixes which ran over the qemu-devel list already), not on
the xen support bits.

cheers,
  Gerd

Re: [Xen-devel] Re: [Qemu-devel] [PATCH 1/2] xenner: add event channel implementation.

[Daniel P. Berrange]

On Mon, Aug 25, 2008 at 07:42:36PM +0100, Keir Fraser wrote:
> On 25/8/08 18:57, "Anthony Liguori" <anthony@codemonkey.ws> wrote:
> 
> > Keir Fraser wrote:
> >> On 25/8/08 15:15, "Anthony Liguori" <anthony@codemonkey.ws> wrote:
> >> 
> >> With regard to this Xenner patch series, I don't think we have a strong
> >> opinion. The changes are not particularly relevant to us. As long as the
> >> orthogonal qemu-on-xen modes that we *do* care about can coexist happily
> >> alongside, we'll be happy.
> > 
> > Sorry, I was actually referring to the other qemu-dm PV domain support
> > patch series.  That's the one that I'm looking for input from ya'll on.
> > Now that 3.3.0 has been released, it should be possible for you guys to
> > consider merging the patches into qemu-dm, right?
> 
> We'll certainly be sync'ing with upstream qemu now, yes. I'm not sure what
> Ian Jackson's plans are with regard to Gerd's patches. I think he was hoping
> that Gerd would post patches to xen-devel against his tree (being an
> existing and actively maintained and tested Xen patchset) and then we would
> from there submit to upstream. Afaics there's some workflow or patchflow to
> be worked out here: I can't see why we would take Gerd's patches wholesale
> when we have a working patchset already.

The core benefit of this new patchset is increased featureset. The current
Xen fork of QEMU only works for running Xen VMs ontop of the Xen hypervisor,
when invoked by XenD. The new patchset should  support this existing use 
case, but in addition allow running of Xen VMs ontop of the Xen hypervisor,
*without* needing XenD, and also allow running of Xen VMs ontop of KVM 
using Xenner. Supporting all three of those use cases with a codebase that
is in mainline QEMU would be a significant improvement over the existing
patchset both in terms of features & reduced support burden for people
developing and distributing Xen/QEMU.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Xen-devel] Re: [Qemu-devel] [PATCH 1/2] xenner: add event channel implementation.

[Keir Fraser]

On 26/8/08 09:27, "Gerd Hoffmann" <kraxel@redhat.com> wrote:

>> Afaics there's some workflow or patchflow to
>> be worked out here:
> 
> Workflow could look like this:
> 
>  (1) Ian merges upstream into qemu-xen.
>  (2) I'll rebase my patches to the resulting tree.
>  (3) Merge both patchsets, into the trees.
> 
> That should result in almost identical hw/xen* files in both trees, so
> we don't end up with a big mess when Ian merges again.

That sounds like a plausible avenue. I don't want to speak for Ian though:
he'll be the one doing most of the work on the qemu-xen side.

>> I can't see why we would take Gerd's patches wholesale
>> when we have a working patchset already.
> 
> Which patchset you are refering to?  As far I know Ian & Samuel are
> focusing on getting the changes to generic qemu code upstream (such as
> serial and ide fixes which ran over the qemu-devel list already), not on
> the xen support bits.

Sorry, by patchset I meant qemu-xen. Bad confusing terminology on my part.

 -- Keir

Re: [Qemu-devel] [PATCH 1/2] xenner: add event channel implementation.

[Ian Jackson]

Gerd Hoffmann writes ("[Qemu-devel] [PATCH 1/2] xenner: add event channel implementation."):
> The patch also adds a XenEvtOps struct with function pointers for the
> xc_evtchn_* family, which is used to switch between libxenctrl and the
> qemu implementation at runtime.  By default libxenctrl is used.

I don't understand why it's necessary to switch between these two
modes at runtime.  That adds a lot of extra boilerplate and
indirection code and probably has some tiny performance effect too.

Why not make it a compile-time option ?

Even if the xen qemu becomes nearly identical to upstream, version
skew issues with the hypervisor, dom0 kernel, and so on will mean that
there will have to be a separate qemu branch for xen just for release
management purposes and so on.

Ian.

Re: [Qemu-devel] [PATCH 1/2] xenner: add event channel implementation.

[Gerd Hoffmann]

Ian Jackson wrote:
> Gerd Hoffmann writes ("[Qemu-devel] [PATCH 1/2] xenner: add event channel implementation."):
>> The patch also adds a XenEvtOps struct with function pointers for the
>> xc_evtchn_* family, which is used to switch between libxenctrl and the
>> qemu implementation at runtime.  By default libxenctrl is used.
> 
> I don't understand why it's necessary to switch between these two
> modes at runtime.  That adds a lot of extra boilerplate and
> indirection code and probably has some tiny performance effect too.
> 
> Why not make it a compile-time option ?

IMHO compile time options are a usability nightmare in most cases.

Note that in another mail you've asked for a command line switch for
qemu to enable xen emulation mode even when running on xen.  That
wouldn't work any more if we would make this a compile time option.  You
would have to go and rebuild qemu first.

Also distrubutions would have to ship two binaries instead of one.  And
there are already way to much different qemu binaries today.  I want to
get that number down, not up.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/2] xenner: add event channel implementation.

[Ian Jackson]

Gerd Hoffmann writes ("Re: [Qemu-devel] [PATCH 1/2] xenner: add event channel implementation."):
> Ian Jackson wrote:
> > Why not make it a compile-time option ?
> 
> IMHO compile time options are a usability nightmare in most cases.

They're not a problem if you need different versions in the two cases
anyway.  That is going to be the case for the foreseeable future.  As
I explained: there is code in qemu-xen which is essential for
operation in the current Xen use model but which upstream don't think
suitable; and of course because of interface skew in the Xen world, it
is necessary to have a compatible version of qemu.

So an OS installation which is both a Xen/xend dom0 and which also has
the ability to run qemu for ordinary emulations, will need two copies
of qemu: one for the Xen domUs invoked by xend, and one for
command-line invocations.

> Also distrubutions would have to ship two binaries instead of one.  And
> there are already way to much different qemu binaries today.  I want to
> get that number down, not up.

The different binaries are with us for at least the next several
years.

Ian.

Re: [Qemu-devel] [PATCH 1/2] xenner: add event channel implementation.

[Gerd Hoffmann]

Ian Jackson wrote:
> Gerd Hoffmann writes ("Re: [Qemu-devel] [PATCH 1/2] xenner: add event channel implementation."):
>> Ian Jackson wrote:
>>> Why not make it a compile-time option ?
>> IMHO compile time options are a usability nightmare in most cases.
> 
> They're not a problem if you need different versions in the two cases
> anyway.  That is going to be the case for the foreseeable future.

I'd like to see the very same binary being used some day.  I know the
day is most likely a few years away, and it wouldn't work with todays
Xen versions because XenD will need changes too to make that happen (see
monitor vs xenstore discussion for example).  But IMHO that is no reason
to give up on that right from the start.

cheers,
  Gerd