Xen devel - Aug 2005 - Re: [Xen-changelog] New console transport and update xenconsoled.

Xen patchbot -unstable wrote:
># HG changeset patch
># User cl349@firebug.cl.cam.ac.uk
># Node ID 8fe8a99b1c2a6ea88624546ab625eaa0758e3a17
># Parent  e69cbfee4011da1648718f1f5cbe8dabb956e72a
>New console transport and update xenconsoled.
>Add a new console interface using a seperate shared page and event channel
>instead of passing the console input/output over control messages.
>Update xenconsoled to use the new console interface.
>Make xenconsoled garbage collect dying domains by subscribing to
>the domain exception virq.
>  
>
Why are we listening for virq?  The polling method used before is 
considerably more robust.  The virq''s aren''t always delivered
on domain
destruction (they are only delivered if a domain crashes or is shutdown).

Moreover, I thought the overriding goal of this was to get rid of xcs?  
Why are we still using it?

Also, there''s a number of errors that this code introduces (changing 
things to use asprintf and not checking for OOM conditions).  It would 
have been nice if this went to the list first so we could comment on it.

I''ll submit some cleanup patches later today...

Regards,

Anthony Liguori
>Signed-off-by: Robert Read <robert@xensource.com>
>Signed-off-by: Christian Limpach <Christian.Limpach@cl.cam.ac.uk>
>
>diff -r e69cbfee4011 -r 8fe8a99b1c2a
linux-2.6-xen-sparse/drivers/xen/console/Makefile
>--- a/linux-2.6-xen-sparse/drivers/xen/console/Makefile	Tue Aug 30 08:47:51
2005
>+++ b/linux-2.6-xen-sparse/drivers/xen/console/Makefile	Tue Aug 30 16:14:53
2005
>@@ -1,2 +1,2 @@
> 
>-obj-y	:= console.o
>+obj-y	:= console.o xencons_ring.o
>diff -r e69cbfee4011 -r 8fe8a99b1c2a
linux-2.6-xen-sparse/drivers/xen/console/console.c
>--- a/linux-2.6-xen-sparse/drivers/xen/console/console.c	Tue Aug 30 08:47:51
2005
>+++ b/linux-2.6-xen-sparse/drivers/xen/console/console.c	Tue Aug 30 16:14:53
2005
>@@ -51,8 +51,8 @@
> #include <asm-xen/xen-public/event_channel.h>
> #include <asm-xen/hypervisor.h>
> #include <asm-xen/evtchn.h>
>-#include <asm-xen/ctrl_if.h>
>-
>+
>+#include "xencons_ring.h"
> /*
>  * Modes:
>  *  ''xencons=off''  [XC_OFF]:     Console is disabled.
>@@ -118,13 +118,6 @@
> /* Common transmit-kick routine. */
> static void __xencons_tx_flush(void);
> 
>-/* This task is used to defer sending console data until there is space. */
>-static void xencons_tx_flush_task_routine(void *data);
>-
>-static DECLARE_TQUEUE(xencons_tx_flush_task, 
>-                      xencons_tx_flush_task_routine,
>-                      NULL);
>-
> #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,0)
> static struct tty_driver *xencons_driver;
> #else
>@@ -264,39 +257,22 @@
> /*** Forcibly flush console data before dying. ***/
> void xencons_force_flush(void)
> {
>-    ctrl_msg_t msg;
>     int        sz;
> 
>     /* Emergency console is synchronous, so there''s nothing to
flush. */
>     if ( xen_start_info.flags & SIF_INITDOMAIN )
>         return;
> 
>-    /*
>-     * We use dangerous control-interface functions that require a
quiescent
>-     * system and no interrupts. Try to ensure this with a global cli().
>-     */
>-    local_irq_disable(); /* XXXsmp */
> 
>     /* Spin until console data is flushed through to the domain controller.
*/
>-    while ( (wc != wp) && !ctrl_if_transmitter_empty() )
>-    {
>-        /* Interrupts are disabled -- we must manually reap responses. */
>-        ctrl_if_discard_responses();
>-
>+    while ( (wc != wp) )
>+    {
>+	int sent = 0;
>         if ( (sz = wp - wc) == 0 )
>             continue;
>-        if ( sz > sizeof(msg.msg) )
>-            sz = sizeof(msg.msg);
>-        if ( sz > (wbuf_size - WBUF_MASK(wc)) )
>-            sz = wbuf_size - WBUF_MASK(wc);
>-
>-        msg.type    = CMSG_CONSOLE;
>-        msg.subtype = CMSG_CONSOLE_DATA;
>-        msg.length  = sz;
>-        memcpy(msg.msg, &wbuf[WBUF_MASK(wc)], sz);
>-            
>-        if ( ctrl_if_send_message_noblock(&msg, NULL, 0) == 0 )
>-            wc += sz;
>+	sent = xencons_ring_send(&wbuf[WBUF_MASK(wc)], sz);
>+	if (sent > 0)
>+	    wc += sent;
>     }
> }
> 
>@@ -320,7 +296,7 @@
> static char x_char;
> 
> /* Non-privileged receive callback. */
>-static void xencons_rx(ctrl_msg_t *msg, unsigned long id)
>+static void xencons_rx(char *buf, unsigned len)
> {
>     int           i;
>     unsigned long flags;
>@@ -328,21 +304,18 @@
>     spin_lock_irqsave(&xencons_lock, flags);
>     if ( xencons_tty != NULL )
>     {
>-        for ( i = 0; i < msg->length; i++ )
>-            tty_insert_flip_char(xencons_tty, msg->msg[i], 0);
>+        for ( i = 0; i < len; i++ )
>+            tty_insert_flip_char(xencons_tty, buf[i], 0);
>         tty_flip_buffer_push(xencons_tty);
>     }
>     spin_unlock_irqrestore(&xencons_lock, flags);
> 
>-    msg->length = 0;
>-    ctrl_if_send_response(msg);
> }
> 
> /* Privileged and non-privileged transmit worker. */
> static void __xencons_tx_flush(void)
> {
>     int        sz, work_done = 0;
>-    ctrl_msg_t msg;
> 
>     if ( xen_start_info.flags & SIF_INITDOMAIN )
>     {
>@@ -367,38 +340,23 @@
>     {
>         while ( x_char )
>         {
>-            msg.type    = CMSG_CONSOLE;
>-            msg.subtype = CMSG_CONSOLE_DATA;
>-            msg.length  = 1;
>-            msg.msg[0]  = x_char;
>-
>-            if ( ctrl_if_send_message_noblock(&msg, NULL, 0) == 0 )
>-                x_char = 0;
>-            else if (
ctrl_if_enqueue_space_callback(&xencons_tx_flush_task) )
>-                break;
>-
>-            work_done = 1;
>+	    if (xencons_ring_send(&x_char, 1) == 1) {
>+		x_char = 0;
>+		work_done = 1;
>+	    }
>         }
> 
>         while ( wc != wp )
>         {
>+	    int sent;
>             sz = wp - wc;
>-            if ( sz > sizeof(msg.msg) )
>-                sz = sizeof(msg.msg);
>-            if ( sz > (wbuf_size - WBUF_MASK(wc)) )
>-                sz = wbuf_size - WBUF_MASK(wc);
>-
>-            msg.type    = CMSG_CONSOLE;
>-            msg.subtype = CMSG_CONSOLE_DATA;
>-            msg.length  = sz;
>-            memcpy(msg.msg, &wbuf[WBUF_MASK(wc)], sz);
>-            
>-            if ( ctrl_if_send_message_noblock(&msg, NULL, 0) == 0 )
>-                wc += sz;
>-            else if (
ctrl_if_enqueue_space_callback(&xencons_tx_flush_task) )
>-                break;
>-
>-            work_done = 1;
>+	    if ( sz > (wbuf_size - WBUF_MASK(wc)) )
>+		sz = wbuf_size - WBUF_MASK(wc);
>+	    sent = xencons_ring_send(&wbuf[WBUF_MASK(wc)], sz);
>+	    if ( sent > 0 ) {
>+		wc += sent;
>+		work_done = 1;
>+	    }
>         }
>     }
> 
>@@ -409,15 +367,6 @@
>              (xencons_tty->ldisc.write_wakeup != NULL) )
>             (xencons_tty->ldisc.write_wakeup)(xencons_tty);
>     }
>-}
>-
>-/* Non-privileged transmit kicker. */
>-static void xencons_tx_flush_task_routine(void *data)
>-{
>-    unsigned long flags;
>-    spin_lock_irqsave(&xencons_lock, flags);
>-    __xencons_tx_flush();
>-    spin_unlock_irqrestore(&xencons_lock, flags);
> }
> 
> /* Privileged receive callback and transmit kicker. */
>@@ -726,6 +675,8 @@
>     if ( xc_mode == XC_OFF )
>         return 0;
> 
>+    xencons_ring_init();
>+
> #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,0)
>     xencons_driver = alloc_tty_driver((xc_mode == XC_SERIAL) ? 
>                                       1 : MAX_NR_CONSOLES);
>@@ -802,7 +753,8 @@
>     }
>     else
>     {
>-        (void)ctrl_if_register_receiver(CMSG_CONSOLE, xencons_rx, 0);
>+	
>+	xencons_ring_register_receiver(xencons_rx);
>     }
> 
>     printk("Xen virtual console successfully installed as
%s%d\n",
>diff -r e69cbfee4011 -r 8fe8a99b1c2a tools/console/daemon/io.c
>--- a/tools/console/daemon/io.c	Tue Aug 30 08:47:51 2005
>+++ b/tools/console/daemon/io.c	Tue Aug 30 16:14:53 2005
>@@ -36,6 +36,9 @@
> #include <fcntl.h>
> #include <unistd.h>
> #include <termios.h>
>+#include <stdarg.h>
>+#include <sys/ioctl.h>
>+#include <sys/mman.h>
> 
> #define MAX(a, b) (((a) > (b)) ? (a) : (b))
> #define MIN(a, b) (((a) < (b)) ? (a) : (b))
>@@ -48,41 +51,6 @@
> 	size_t max_capacity;
> };
> 
>-static void buffer_append(struct buffer *buffer, const void *data, size_t
size)
>-{
>-	if ((buffer->capacity - buffer->size) < size) {
>-		buffer->capacity += (size + 1024);
>-		buffer->data = realloc(buffer->data, buffer->capacity);
>-		if (buffer->data == NULL) {
>-			dolog(LOG_ERR, "Memory allocation failed");
>-			exit(ENOMEM);
>-		}
>-	}
>-
>-	memcpy(buffer->data + buffer->size, data, size);
>-	buffer->size += size;
>-
>-	if (buffer->max_capacity &&
>-	    buffer->size > buffer->max_capacity) {
>-		memmove(buffer->data + (buffer->size - buffer->max_capacity),
>-			buffer->data, buffer->max_capacity);
>-		buffer->data = realloc(buffer->data, buffer->max_capacity);
>-		buffer->capacity = buffer->max_capacity;
>-	}
>-}
>-
>-static bool buffer_empty(struct buffer *buffer)
>-{
>-	return buffer->size == 0;
>-}
>-
>-static void buffer_advance(struct buffer *buffer, size_t size)
>-{
>-	size = MIN(size, buffer->size);
>-	memmove(buffer->data, buffer + size, buffer->size - size);
>-	buffer->size -= size;
>-}
>-
> struct domain
> {
> 	int domid;
>@@ -90,9 +58,74 @@
> 	bool is_dead;
> 	struct buffer buffer;
> 	struct domain *next;
>+	unsigned long mfn;
>+	int local_port;
>+	int remote_port;
>+	char *page;
>+	int evtchn_fd;
> };
> 
> static struct domain *dom_head;
>+
>+struct ring_head
>+{
>+	u32 cons;
>+	u32 prod;
>+	char buf[0];
>+} __attribute__((packed));
>+
>+#define PAGE_SIZE (getpagesize())
>+#define XENCONS_RING_SIZE (PAGE_SIZE/2 - sizeof (struct ring_head))
>+#define XENCONS_IDX(cnt) ((cnt) % XENCONS_RING_SIZE)
>+#define XENCONS_FULL(ring) (((ring)->prod - (ring)->cons) ==
XENCONS_RING_SIZE)
>+#define XENCONS_SPACE(ring) (XENCONS_RING_SIZE - ((ring)->prod -
(ring)->cons))
>+
>+static void buffer_append(struct domain *dom)
>+{
>+	struct buffer *buffer = &dom->buffer;
>+	struct ring_head *ring = (struct ring_head *)dom->page;
>+	size_t size;
>+
>+	while ((size = ring->prod - ring->cons) != 0) {
>+		if ((buffer->capacity - buffer->size) < size) {
>+			buffer->capacity += (size + 1024);
>+			buffer->data = realloc(buffer->data, buffer->capacity);
>+			if (buffer->data == NULL) {
>+				dolog(LOG_ERR, "Memory allocation failed");
>+				exit(ENOMEM);
>+			}
>+		}
>+
>+		while (ring->cons < ring->prod) {
>+			buffer->data[buffer->size] >+			
ring->buf[XENCONS_IDX(ring->cons)];
>+			buffer->size++;
>+			ring->cons++;
>+		}
>+
>+		if (buffer->max_capacity &&
>+		    buffer->size > buffer->max_capacity) {
>+			memmove(buffer->data + (buffer->size -
>+						buffer->max_capacity),
>+				buffer->data, buffer->max_capacity);
>+			buffer->data = realloc(buffer->data,
>+					       buffer->max_capacity);
>+			buffer->capacity = buffer->max_capacity;
>+		}
>+	}
>+}
>+
>+static bool buffer_empty(struct buffer *buffer)
>+{
>+	return buffer->size == 0;
>+}
>+
>+static void buffer_advance(struct buffer *buffer, size_t size)
>+{
>+	size = MIN(size, buffer->size);
>+	memmove(buffer->data, buffer + size, buffer->size - size);
>+	buffer->size -= size;
>+}
> 
> static bool domain_is_valid(int domid)
> {
>@@ -107,7 +140,7 @@
> 
> static int domain_create_tty(struct domain *dom)
> {
>-	char path[1024];
>+	char *path;
> 	int master;
> 
> 	if ((master = getpt()) == -1 ||
>@@ -126,22 +159,106 @@
> 			tcsetattr(master, TCSAFLUSH, &term);
> 		}
> 
>-		xs_mkdir(xs, "/console");
>-		snprintf(path, sizeof(path), "/console/%d", dom->domid);
>-		xs_mkdir(xs, path);
>-		strcat(path, "/tty");
>-
>+		asprintf(&path, "/console/%d/tty", dom->domid);
> 		xs_write(xs, path, slave, strlen(slave), O_CREAT);
>-
>-		snprintf(path, sizeof(path), "/console/%d/limit",
dom->domid);
>+		free(path);
>+
>+		asprintf(&path, "/console/%d/limit", dom->domid);
> 		data = xs_read(xs, path, &len);
> 		if (data) {
> 			dom->buffer.max_capacity = strtoul(data, 0, 0);
> 			free(data);
> 		}
>+		free(path);
> 	}
> 
> 	return master;
>+}
>+
>+/* Takes tuples of names, scanf-style args, and void **, NULL terminated.
*/
>+int xs_gather(struct xs_handle *xs, const char *dir, ...)
>+{
>+	va_list ap;
>+	const char *name;
>+	char *path;
>+	int ret = 0;
>+
>+	va_start(ap, dir);
>+	while (ret == 0 && (name = va_arg(ap, char *)) != NULL) {
>+		const char *fmt = va_arg(ap, char *);
>+		void *result = va_arg(ap, void *);
>+		char *p;
>+
>+		asprintf(&path, "%s/%s", dir, name);
>+		p = xs_read(xs, path, NULL);
>+		free(path);
>+		if (p == NULL) {
>+			ret = ENOENT;
>+			break;
>+		}
>+		if (fmt) {
>+			if (sscanf(p, fmt, result) == 0)
>+				ret = EINVAL;
>+			free(p);
>+		} else
>+			*(char **)result = p;
>+	}
>+	va_end(ap);
>+	return ret;
>+}
>+
>+#define EVENTCHN_BIND		_IO(''E'', 2)
>+#define EVENTCHN_UNBIND 	_IO(''E'', 3)
>+
>+static int domain_create_ring(struct domain *dom)
>+{
>+	char *dompath, *path;
>+	int err;
>+
>+	dom->page = NULL;
>+	dom->evtchn_fd = -1;
>+
>+	asprintf(&path, "/console/%d/domain", dom->domid);
>+	dompath = xs_read(xs, path, NULL);
>+	free(path);
>+	if (!dompath)
>+		return ENOENT;
>+
>+	err = xs_gather(xs, dompath,
>+			"console_mfn", "%li", &dom->mfn,
>+			"console_channel/port1", "%i",
&dom->local_port,
>+			"console_channel/port2", "%i",
&dom->remote_port,
>+			NULL);
>+	if (err)
>+		goto out;
>+
>+	dom->page = xc_map_foreign_range(xc, dom->domid, getpagesize(),
>+					 PROT_READ|PROT_WRITE, dom->mfn);
>+	if (dom->page == NULL) {
>+		err = EINVAL;
>+		goto out;
>+	}
>+
>+	/* Opening evtchn independently for each console is a bit
>+	 * wastefule, but that''s how the code is structured... */
>+	err = open("/dev/xen/evtchn", O_RDWR);
>+	if (err == -1) {
>+		err = errno;
>+		goto out;
>+	}
>+	dom->evtchn_fd = err;
>+
>+	if (ioctl(dom->evtchn_fd, EVENTCHN_BIND, dom->local_port) == -1) {
>+		err = errno;
>+		munmap(dom->page, getpagesize());
>+		close(dom->evtchn_fd);
>+		dom->evtchn_fd = -1;
>+		goto out;
>+	}
>+
>+ out:
>+	free(dompath);
>+	return err;
> }
> 
> static struct domain *create_domain(int domid)
>@@ -162,7 +279,9 @@
> 	dom->buffer.size = 0;
> 	dom->buffer.capacity = 0;
> 	dom->buffer.max_capacity = 0;
>-	dom->next = 0;
>+	dom->next = NULL;
>+
>+	domain_create_ring(dom);
> 
> 	dolog(LOG_DEBUG, "New domain %d", domid);
> 
>@@ -200,9 +319,14 @@
> 
> 		if (dom->domid == d->domid) {
> 			*pp = d->next;
>-			if (d->buffer.data) {
>+			if (d->buffer.data)
> 				free(d->buffer.data);
>-			}
>+			if (d->page)
>+				munmap(d->page, getpagesize());
>+			if (d->evtchn_fd != -1)
>+				close(d->evtchn_fd);
>+			if (d->tty_fd != -1)
>+				close(d->tty_fd);
> 			free(d);
> 			break;
> 		}
>@@ -211,28 +335,28 @@
> 
> static void remove_dead_domains(struct domain *dom)
> {
>-	if (dom == NULL) return;
>-	remove_dead_domains(dom->next);
>-
>-	if (dom->is_dead) {
>-		remove_domain(dom);
>+	struct domain *n;
>+
>+	while (dom != NULL) {
>+		n = dom->next;
>+		if (dom->is_dead)
>+			remove_domain(dom);
>+		dom = n;
> 	}
> }
> 
> static void handle_tty_read(struct domain *dom)
> {
> 	ssize_t len;
>-	xcs_msg_t msg;
>-
>-	msg.type = XCS_REQUEST;
>-	msg.u.control.remote_dom = dom->domid;
>-	msg.u.control.msg.type = CMSG_CONSOLE;
>-	msg.u.control.msg.subtype = CMSG_CONSOLE_DATA;
>-	msg.u.control.msg.id = 1;
>-
>-	len = read(dom->tty_fd, msg.u.control.msg.msg, 60);
>+	char msg[80];
>+	struct ring_head *inring >+		(struct ring_head *)(dom->page +
PAGE_SIZE/2);
>+	int i;
>+
>+	len = read(dom->tty_fd, msg, MAX(XENCONS_SPACE(inring), sizeof(msg)));
> 	if (len < 1) {
> 		close(dom->tty_fd);
>+		dom->tty_fd = -1;
> 
> 		if (domain_is_valid(dom->domid)) {
> 			dom->tty_fd = domain_create_tty(dom);
>@@ -240,14 +364,14 @@
> 			dom->is_dead = true;
> 		}
> 	} else if (domain_is_valid(dom->domid)) {
>-		msg.u.control.msg.length = len;
>-
>-		if (!write_sync(xcs_data_fd, &msg, sizeof(msg))) {
>-			dolog(LOG_ERR, "Write to xcs failed: %m");
>-			exit(1);
>-		}
>+		for (i = 0; i < len; i++) {
>+			inring->buf[XENCONS_IDX(inring->prod)] = msg[i];
>+			inring->prod++;
>+		}
>+		xc_evtchn_send(xc, dom->local_port);
> 	} else {
> 		close(dom->tty_fd);
>+		dom->tty_fd = -1;
> 		dom->is_dead = true;
> 	}
> }
>@@ -259,6 +383,7 @@
> 	len = write(dom->tty_fd, dom->buffer.data, dom->buffer.size);
> 	if (len < 1) {
> 		close(dom->tty_fd);
>+		dom->tty_fd = -1;
> 
> 		if (domain_is_valid(dom->domid)) {
> 			dom->tty_fd = domain_create_tty(dom);
>@@ -270,6 +395,18 @@
> 	}
> }
> 
>+static void handle_ring_read(struct domain *dom)
>+{
>+	u16 v;
>+
>+	if (!read_sync(dom->evtchn_fd, &v, sizeof(v)))
>+		return;
>+
>+	buffer_append(dom);
>+
>+	(void)write_sync(dom->evtchn_fd, &v, sizeof(v));
>+}
>+
> static void handle_xcs_msg(int fd)
> {
> 	xcs_msg_t msg;
>@@ -277,13 +414,6 @@
> 	if (!read_sync(fd, &msg, sizeof(msg))) {
> 		dolog(LOG_ERR, "read from xcs failed! %m");
> 		exit(1);
>-	} else if (msg.type == XCS_REQUEST) {
>-		struct domain *dom;
>-
>-		dom = lookup_domain(msg.u.control.remote_dom);
>-		buffer_append(&dom->buffer,
>-			      msg.u.control.msg.msg,
>-			      msg.u.control.msg.length);
> 	}
> }
> 
>@@ -291,9 +421,12 @@
> {
> 	int domid = 0;
> 	xc_dominfo_t dominfo;
>+	struct domain *dom;
> 
> 	while (xc_domain_getinfo(xc, domid, 1, &dominfo) == 1) {
>-		lookup_domain(dominfo.domid);
>+		dom = lookup_domain(dominfo.domid);
>+		if (dominfo.dying || dominfo.crashed || dominfo.shutdown)
>+			dom->is_dead = true;
> 		domid = dominfo.domid + 1;
> 	}
> }
>@@ -302,12 +435,11 @@
> {
> 	fd_set readfds, writefds;
> 	int ret;
>-	int max_fd = -1;
>-	int num_of_writes = 0;
> 
> 	do {
> 		struct domain *d;
> 		struct timeval tv = { 1, 0 };
>+		int max_fd = -1;
> 
> 		FD_ZERO(&readfds);
> 		FD_ZERO(&writefds);
>@@ -319,42 +451,36 @@
> 			if (d->tty_fd != -1) {
> 				FD_SET(d->tty_fd, &readfds);
> 			}
>+			if (d->evtchn_fd != -1)
>+				FD_SET(d->evtchn_fd, &readfds);
> 
> 			if (d->tty_fd != -1 && !buffer_empty(&d->buffer)) {
> 				FD_SET(d->tty_fd, &writefds);
> 			}
> 
> 			max_fd = MAX(d->tty_fd, max_fd);
>+			max_fd = MAX(d->evtchn_fd, max_fd);
> 		}
> 
> 		ret = select(max_fd + 1, &readfds, &writefds, 0, &tv);
>-		if (tv.tv_sec == 1 && (++num_of_writes % 100) == 0) {
>-#if 0
>-			/* FIXME */
>-			/* This is a nasty hack.  xcs does not handle the
>-			   control channels filling up well at all.  We''ll
>-			   throttle ourselves here since we do proper
>-			   queueing to give the domains a shot at pulling out
>-			   the data.  Fixing xcs is not worth it as it''s
>-			   going away */
>-			tv.tv_usec = 1000;
>-			select(0, 0, 0, 0, &tv);
>-#endif
>-		}
> 		enum_domains();
> 
>-		if (FD_ISSET(xcs_data_fd, &readfds)) {
>+		if (FD_ISSET(xcs_data_fd, &readfds))
> 			handle_xcs_msg(xcs_data_fd);
>-		}
> 
> 		for (d = dom_head; d; d = d->next) {
>-			if (!d->is_dead && FD_ISSET(d->tty_fd, &readfds)) {
>+			if (d->is_dead || d->tty_fd == -1 ||
>+			    d->evtchn_fd == -1)
>+				continue;
>+
>+			if (FD_ISSET(d->tty_fd, &readfds))
> 				handle_tty_read(d);
>-			}
>-
>-			if (!d->is_dead && FD_ISSET(d->tty_fd, &writefds)) {
>+
>+			if (FD_ISSET(d->evtchn_fd, &readfds))
>+				handle_ring_read(d);
>+
>+			if (FD_ISSET(d->tty_fd, &writefds))
> 				handle_tty_write(d);
>-			}
> 		}
> 
> 		remove_dead_domains(dom_head);
>diff -r e69cbfee4011 -r 8fe8a99b1c2a tools/console/daemon/utils.c
>--- a/tools/console/daemon/utils.c	Tue Aug 30 08:47:51 2005
>+++ b/tools/console/daemon/utils.c	Tue Aug 30 16:14:53 2005
>@@ -226,14 +226,10 @@
> 		goto out_close_data;
> 	}
> 
>-	/* Since the vast majority of control messages are console messages
>-	   it''s just easier to ignore other messages that try to bind to 
>-	   a specific type. */
>-	msg.type = XCS_MSG_BIND;
>-	msg.u.bind.port = PORT_WILDCARD;
>-	msg.u.bind.type = TYPE_WILDCARD;
>+	msg.type = XCS_VIRQ_BIND;
>+	msg.u.virq.virq = VIRQ_DOM_EXC;
> 	if (!xcs_send_recv(xcs_ctrl_fd, &msg) || msg.result != XCS_RSLT_OK) {
>-		dolog(LOG_ERR, "xcs vind failed.  Possible bug.");
>+		dolog(LOG_ERR, "xcs virq bind failed.  Possible bug.");
> 		goto out_close_data;
> 	}
> 	
>diff -r e69cbfee4011 -r 8fe8a99b1c2a tools/libxc/xc_linux_build.c
>--- a/tools/libxc/xc_linux_build.c	Tue Aug 30 08:47:51 2005
>+++ b/tools/libxc/xc_linux_build.c	Tue Aug 30 16:14:53 2005
>@@ -335,7 +335,8 @@
>                        unsigned int control_evtchn,
>                        unsigned long flags,
>                        unsigned int vcpus,
>-		       unsigned int store_evtchn, unsigned long *store_mfn)
>+		       unsigned int store_evtchn, unsigned long *store_mfn,
>+		       unsigned int console_evtchn, unsigned long *console_mfn)
> {
>     unsigned long *page_array = NULL;
>     unsigned long count, i;
>@@ -358,6 +359,8 @@
>     unsigned long vstartinfo_end;
>     unsigned long vstoreinfo_start;
>     unsigned long vstoreinfo_end;
>+    unsigned long vconsole_start;
>+    unsigned long vconsole_end;
>     unsigned long vstack_start;
>     unsigned long vstack_end;
>     unsigned long vpt_start;
>@@ -393,7 +396,9 @@
>     vphysmap_end     = vphysmap_start + (nr_pages * sizeof(unsigned long));
>     vstoreinfo_start = round_pgup(vphysmap_end);
>     vstoreinfo_end   = vstoreinfo_start + PAGE_SIZE;
>-    vpt_start        = vstoreinfo_end; 
>+    vconsole_start   = vstoreinfo_end;
>+    vconsole_end     = vstoreinfo_end + PAGE_SIZE;
>+    vpt_start        = vconsole_end; 
> 
>     for ( nr_pt_pages = 2; ; nr_pt_pages++ )
>     {
>@@ -437,6 +442,7 @@
>            " Init. ramdisk: %p->%p\n"
>            " Phys-Mach map: %p->%p\n"
>            " Store page:    %p->%p\n"
>+           " Console page:  %p->%p\n"
>            " Page tables:   %p->%p\n"
>            " Start info:    %p->%p\n"
>            " Boot stack:    %p->%p\n"
>@@ -445,6 +451,7 @@
>            _p(vinitrd_start), _p(vinitrd_end),
>            _p(vphysmap_start), _p(vphysmap_end),
>            _p(vstoreinfo_start), _p(vstoreinfo_end),
>+           _p(vconsole_start), _p(vconsole_end),
>            _p(vpt_start), _p(vpt_end),
>            _p(vstartinfo_start), _p(vstartinfo_end),
>            _p(vstack_start), _p(vstack_end),
>@@ -566,6 +573,8 @@
> #endif
> 
>     *store_mfn = page_array[(vstoreinfo_start-dsi.v_start) >>
PAGE_SHIFT];
>+    *console_mfn = page_array[(vconsole_start-dsi.v_start) >>
PAGE_SHIFT];
>+
> 
>     start_info = xc_map_foreign_range(
>         xc_handle, dom, PAGE_SIZE, PROT_READ|PROT_WRITE,
>@@ -580,6 +589,8 @@
>     start_info->domain_controller_evtchn = control_evtchn;
>     start_info->store_mfn    = *store_mfn;
>     start_info->store_evtchn = store_evtchn;
>+    start_info->console_mfn   = *console_mfn;
>+    start_info->console_evtchn = console_evtchn;
>     if ( initrd_len != 0 )
>     {
>         start_info->mod_start    = vinitrd_start;
>@@ -631,7 +642,9 @@
>                    unsigned long flags,
>                    unsigned int vcpus,
>                    unsigned int store_evtchn,
>-                   unsigned long *store_mfn)
>+                   unsigned long *store_mfn,
>+                   unsigned int console_evtchn,
>+                   unsigned long *console_mfn)
> {
>     dom0_op_t launch_op, op;
>     int initrd_fd = -1;
>@@ -707,7 +720,8 @@
>                      &vstack_start, ctxt, cmdline,
>                      op.u.getdomaininfo.shared_info_frame,
>                      control_evtchn, flags, vcpus,
>-                     store_evtchn, store_mfn) < 0 )
>+                     store_evtchn, store_mfn,
>+		     console_evtchn, console_mfn) < 0 )
>     {
>         ERROR("Error constructing guest OS");
>         goto error_out;
>diff -r e69cbfee4011 -r 8fe8a99b1c2a tools/libxc/xenguest.h
>--- a/tools/libxc/xenguest.h	Tue Aug 30 08:47:51 2005
>+++ b/tools/libxc/xenguest.h	Tue Aug 30 16:14:53 2005
>@@ -47,7 +47,9 @@
>                    unsigned long flags,
>                    unsigned int vcpus,
>                    unsigned int store_evtchn,
>-                   unsigned long *store_mfn);
>+                   unsigned long *store_mfn,
>+                   unsigned int console_evtchn,
>+                   unsigned long *console_mfn);
> 
> struct mem_map;
> int xc_vmx_build(int xc_handle,
>diff -r e69cbfee4011 -r 8fe8a99b1c2a tools/python/xen/lowlevel/xc/xc.c
>--- a/tools/python/xen/lowlevel/xc/xc.c	Tue Aug 30 08:47:51 2005
>+++ b/tools/python/xen/lowlevel/xc/xc.c	Tue Aug 30 16:14:53 2005
>@@ -268,25 +268,33 @@
>     u32 dom;
>     char *image, *ramdisk = NULL, *cmdline = "";
>     int flags = 0, vcpus = 1;
>-    int control_evtchn, store_evtchn;
>+    int control_evtchn, store_evtchn, console_evtchn;
>     unsigned long store_mfn = 0;
>+    unsigned long console_mfn = 0;
> 
>     static char *kwd_list[] = { "dom",
"control_evtchn", "store_evtchn",
>-                                "image", "ramdisk",
"cmdline", "flags",
>+                                "console_evtchn",
"image",
>+				/* optional */
>+				"ramdisk", "cmdline", "flags",
> 				"vcpus", NULL };
> 
>-    if ( !PyArg_ParseTupleAndKeywords(args, kwds, "iiis|ssii",
kwd_list,
>+    if ( !PyArg_ParseTupleAndKeywords(args, kwds, "iiiis|ssii",
kwd_list,
>                                       &dom, &control_evtchn,
&store_evtchn,
>-                                      &image, &ramdisk,
&cmdline, &flags,
>+				      &console_evtchn, &image, 
>+				      /* optional */
>+				      &ramdisk, &cmdline, &flags,
>                                       &vcpus) )
>         return NULL;
> 
>     if ( xc_linux_build(xc->xc_handle, dom, image,
>                         ramdisk, cmdline, control_evtchn, flags, vcpus,
>-                        store_evtchn, &store_mfn) != 0 )
>-        return PyErr_SetFromErrno(xc_error);
>-    
>-    return Py_BuildValue("{s:i}", "store_mfn",
store_mfn);
>+                        store_evtchn, &store_mfn, 
>+			console_evtchn, &console_mfn) != 0 )
>+        return PyErr_SetFromErrno(xc_error);
>+    
>+    return Py_BuildValue("{s:i,s:i}", 
>+			 "store_mfn", store_mfn,
>+			 "console_mfn", console_mfn);
> }
> 
> static PyObject *pyxc_vmx_build(PyObject *self,
>diff -r e69cbfee4011 -r 8fe8a99b1c2a tools/python/xen/xend/XendDomainInfo.py
>--- a/tools/python/xen/xend/XendDomainInfo.py	Tue Aug 30 08:47:51 2005
>+++ b/tools/python/xen/xend/XendDomainInfo.py	Tue Aug 30 16:14:53 2005
>@@ -47,7 +47,7 @@
> from xen.xend.XendRoot import get_component
> 
> from xen.xend.uuid import getUuid
>-from xen.xend.xenstore import DBVar
>+from xen.xend.xenstore import DBVar, XenNode, DBMap
> 
> """Shutdown code for poweroff."""
> DOMAIN_POWEROFF = 0
>@@ -231,6 +231,7 @@
>         DBVar(''start_time'',   
ty=''float''),
>         DBVar(''state'',        
ty=''str''),
>         DBVar(''store_mfn'',    
ty=''long''),
>+        DBVar(''console_mfn'',    
ty=''long''),
>         DBVar(''restart_mode'', 
ty=''str''),
>         DBVar(''restart_state'',
ty=''str''),
>         DBVar(''restart_time'', 
ty=''float''),
>@@ -260,6 +261,8 @@
>         self.channel = None
>         self.store_channel = None
>         self.store_mfn = None
>+        self.console_channel = None
>+        self.console_mfn = None
>         self.controllers = {}
>         
>         self.info = None
>@@ -297,6 +300,9 @@
>         if self.store_channel:
>            
self.store_channel.saveToDB(self.db.addChild("store_channel"),
>                                         save=save)
>+        if self.console_channel:
>+           
self.console_channel.saveToDB(self.db.addChild("console_channel"),
>+                                        save=save)
>         if self.image:
>             self.image.exportToDB(save=save, sync=sync)
>         self.db.exportToDB(self, fields=self.__exports__, save=save,
sync=sync)
>@@ -328,6 +334,9 @@
> 
>     def getStoreChannel(self):
>         return self.store_channel
>+
>+    def getConsoleChannel(self):
>+        return self.console_channel
> 
>     def update(self, info):
>         """Update with  info from xc.domain_getinfo().
>@@ -518,6 +527,14 @@
>             sxpr.append(self.store_channel.sxpr())
>         if self.store_mfn:
>             sxpr.append([''store_mfn'', self.store_mfn])
>+        if self.console_channel:
>+            sxpr.append([''console_channel'',
self.console_channel.sxpr()])
>+        if self.console_mfn:
>+            sxpr.append([''console_mfn'',
self.console_mfn])
>+# already in (devices)
>+#        console = self.getConsole()
>+#        if console:
>+#            sxpr.append(console.sxpr())
> 
>         if self.restart_count:
>             sxpr.append([''restart_count'',
self.restart_count])
>@@ -712,6 +729,13 @@
>             except Exception, ex:
>                 log.warning("error in domain release on xenstore:
%s", ex)
>                 pass
>+        if self.console_channel:
>+            # notify processes using this cosole?
>+            try:
>+                self.console_channel.close()
>+                self.console_channel = None
>+            except:
>+                pass
>         if self.image:
>             try:
>                 self.device_model_pid = 0
>@@ -808,6 +832,7 @@
>         """
>         self.channel = self.openChannel("channel", 0, 1)
>         self.store_channel = self.eventChannel("store_channel")
>+        self.console_channel =
self.eventChannel("console_channel")
> 
>     def create_configured_devices(self):
>         devices = sxp.children(self.config, ''device'')
>@@ -1003,6 +1028,7 @@
>         self.configure_fields()
>         self.create_devices()
>         self.create_blkif()
>+        self.publish_console()
> 
>     def create_blkif(self):
>         """Create the block device interface (blkif) for the
vm.
>@@ -1017,6 +1043,12 @@
>             backend = blkif.getBackend(0)
>             backend.connect(recreate=self.recreate)
> 
>+    def publish_console(self):
>+        db = DBMap(db=XenNode("/console/%d" % self.id))
>+        db.clear()
>+        db[''domain''] = self.db.getPath()
>+        db.saveDB(save=True)
>+        
>     def configure_fields(self):
>         """Process the vm configuration fields using the
registered handlers.
>         """
>diff -r e69cbfee4011 -r 8fe8a99b1c2a tools/python/xen/xend/image.py
>--- a/tools/python/xen/xend/image.py	Tue Aug 30 08:47:51 2005
>+++ b/tools/python/xen/xend/image.py	Tue Aug 30 16:14:53 2005
>@@ -238,16 +238,33 @@
>             store_evtchn = self.vm.store_channel.port2
>         else:
>             store_evtchn = 0
>+        if self.vm.console_channel:
>+            console_evtchn = self.vm.console_channel.port2
>+        else:
>+            console_evtchn = 0
>+
>+        log.debug("dom            = %d", self.vm.getDomain())
>+        log.debug("image          = %s", self.kernel)
>+        log.debug("control_evtchn = %s",
self.vm.channel.getRemotePort())
>+        log.debug("store_evtchn   = %d", store_evtchn)
>+        log.debug("console_evtchn = %d", console_evtchn)
>+        log.debug("cmdline        = %s", self.cmdline)
>+        log.debug("ramdisk        = %s", self.ramdisk)
>+        log.debug("flags          = %d", self.flags)
>+        log.debug("vcpus          = %d", self.vm.vcpus)
>+
>         ret = xc.linux_build(dom            = self.vm.getDomain(),
>                              image          = self.kernel,
>                              control_evtchn =
self.vm.channel.getRemotePort(),
>                              store_evtchn   = store_evtchn,
>+                             console_evtchn = console_evtchn,
>                              cmdline        = self.cmdline,
>                              ramdisk        = self.ramdisk,
>                              flags          = self.flags,
>                              vcpus          = self.vm.vcpus)
>         if isinstance(ret, dict):
>             self.vm.store_mfn = ret.get(''store_mfn'')
>+            self.vm.console_mfn = ret.get(''console_mfn'')
>             return 0
>         return ret
> 
>diff -r e69cbfee4011 -r 8fe8a99b1c2a xen/include/public/xen.h
>--- a/xen/include/public/xen.h	Tue Aug 30 08:47:51 2005
>+++ b/xen/include/public/xen.h	Tue Aug 30 16:14:53 2005
>@@ -438,19 +438,21 @@
> #define MAX_GUEST_CMDLINE 1024
> typedef struct start_info {
>     /* THE FOLLOWING ARE FILLED IN BOTH ON INITIAL BOOT AND ON RESUME.   
*/
>-    unsigned long nr_pages;   /* Total pages allocated to this domain.   
*/
>-    unsigned long shared_info;/* MACHINE address of shared info struct.  
*/
>-    u32      flags;           /* SIF_xxx flags.                          
*/
>+    unsigned long nr_pages;     /* Total pages allocated to this domain. 
*/
>+    unsigned long shared_info;  /* MACHINE address of shared info struct.
*/
>+    u32      flags;             /* SIF_xxx flags.                        
*/
>     u16      domain_controller_evtchn;
>     /* THE FOLLOWING ARE ONLY FILLED IN ON INITIAL BOOT (NOT RESUME).    
*/
>-    unsigned long pt_base;    /* VIRTUAL address of page directory.      
*/
>-    unsigned long nr_pt_frames;/* Number of bootstrap p.t. frames.       
*/
>-    unsigned long mfn_list;   /* VIRTUAL address of page-frame list.     
*/
>-    unsigned long mod_start;  /* VIRTUAL address of pre-loaded module.   
*/
>-    unsigned long mod_len;    /* Size (bytes) of pre-loaded module.      
*/
>+    unsigned long pt_base;      /* VIRTUAL address of page directory.    
*/
>+    unsigned long nr_pt_frames; /* Number of bootstrap p.t. frames.      
*/
>+    unsigned long mfn_list;     /* VIRTUAL address of page-frame list.   
*/
>+    unsigned long mod_start;    /* VIRTUAL address of pre-loaded module. 
*/
>+    unsigned long mod_len;      /* Size (bytes) of pre-loaded module.    
*/
>     s8 cmd_line[MAX_GUEST_CMDLINE];
>-    unsigned long store_mfn;  /* MACHINE page number of shared page.     
*/
>-    u16      store_evtchn;    /* Event channel for store communication.  
*/
>+    unsigned long store_mfn;    /* MACHINE page number of shared page.   
*/
>+    u16      store_evtchn;      /* Event channel for store communication.
*/
>+    unsigned long console_mfn;  /* MACHINE address of console page.      
*/
>+    u16      console_evtchn;    /* Event channel for console messages.   
*/
> } start_info_t;
> 
> /* These flags are passed in the ''flags'' field of
start_info_t. */
>diff -r e69cbfee4011 -r 8fe8a99b1c2a
linux-2.6-xen-sparse/drivers/xen/console/xencons_ring.c
>--- /dev/null	Tue Aug 30 08:47:51 2005
>+++ b/linux-2.6-xen-sparse/drivers/xen/console/xencons_ring.c	Tue Aug 30
16:14:53 2005
>@@ -0,0 +1,124 @@
>+#include <linux/version.h>
>+#include <linux/module.h>
>+#include <linux/errno.h>
>+#include <linux/signal.h>
>+#include <linux/sched.h>
>+#include <linux/interrupt.h>
>+#include <linux/tty.h>
>+#include <linux/tty_flip.h>
>+#include <linux/serial.h>
>+#include <linux/major.h>
>+#include <linux/ptrace.h>
>+#include <linux/ioport.h>
>+#include <linux/mm.h>
>+#include <linux/slab.h>
>+
>+#include <asm-xen/hypervisor.h>
>+#include <asm-xen/evtchn.h>
>+#include <linux/wait.h>
>+#include <linux/interrupt.h>
>+#include <linux/sched.h>
>+#include <linux/err.h>
>+#include "xencons_ring.h"
>+
>+
>+struct ring_head
>+{
>+	u32 cons;
>+	u32 prod;
>+	char buf[0];
>+} __attribute__((packed));
>+
>+
>+#define XENCONS_RING_SIZE (PAGE_SIZE/2 - sizeof (struct ring_head))
>+#define XENCONS_IDX(cnt) ((cnt) % XENCONS_RING_SIZE)
>+#define XENCONS_FULL(ring) (((ring)->prod - (ring)->cons) ==
XENCONS_RING_SIZE)
>+
>+static inline struct ring_head *outring(void)
>+{
>+	return machine_to_virt(xen_start_info.console_mfn << PAGE_SHIFT);
>+}
>+
>+static inline struct ring_head *inring(void)
>+{
>+	return machine_to_virt(xen_start_info.console_mfn << PAGE_SHIFT)
>+		+ PAGE_SIZE/2;
>+}
>+
>+
>+/* don''t block -  write as much as possible and return */
>+static int __xencons_ring_send(struct ring_head *ring, const char *data,
unsigned len)
>+{
>+	int copied = 0;
>+
>+	mb();
>+	while (copied < len && !XENCONS_FULL(ring)) {
>+		ring->buf[XENCONS_IDX(ring->prod)] = data[copied];
>+		ring->prod++;
>+		copied++;
>+	}
>+	mb();
>+
>+	return copied;
>+}
>+
>+int xencons_ring_send(const char *data, unsigned len)
>+{
>+	struct ring_head *out = outring();
>+	int sent = 0;
>+	
>+	sent = __xencons_ring_send(out, data, len);
>+	notify_via_evtchn(xen_start_info.console_evtchn);
>+	return sent;
>+
>+}	
>+
>+
>+static xencons_receiver_func *xencons_receiver;
>+
>+static irqreturn_t handle_input(int irq, void *unused, struct pt_regs
*regs)
>+{
>+	struct ring_head *ring = inring();
>+	while (ring->cons < ring->prod) {
>+		if (xencons_receiver != NULL) {
>+			xencons_receiver(ring->buf + XENCONS_IDX(ring->cons),
>+					 1);
>+		}
>+		ring->cons++;
>+	}
>+	return IRQ_HANDLED;
>+}
>+
>+void xencons_ring_register_receiver(xencons_receiver_func *f)
>+{
>+	xencons_receiver = f;
>+}
>+
>+int xencons_ring_init(void)
>+{
>+	int err;
>+
>+	if (!xen_start_info.console_evtchn)
>+		return 0;
>+
>+	err = bind_evtchn_to_irqhandler(
>+		xen_start_info.console_evtchn, handle_input,
>+		0, "xencons", inring());
>+	if (err) {
>+		xprintk(KERN_ERR "XEN console request irq failed %i\n", err);
>+		unbind_evtchn_from_irq(xen_start_info.console_evtchn);
>+		return err;
>+	}
>+
>+	return 0;
>+}
>+
>+void xencons_suspend_comms(void)
>+{
>+
>+	if (!xen_start_info.console_evtchn)
>+		return;
>+
>+	unbind_evtchn_from_irqhandler(xen_start_info.console_evtchn, inring());
>+}
>+
>diff -r e69cbfee4011 -r 8fe8a99b1c2a
linux-2.6-xen-sparse/drivers/xen/console/xencons_ring.h
>--- /dev/null	Tue Aug 30 08:47:51 2005
>+++ b/linux-2.6-xen-sparse/drivers/xen/console/xencons_ring.h	Tue Aug 30
16:14:53 2005
>@@ -0,0 +1,13 @@
>+#ifndef _XENCONS_RING_H
>+#define _XENCONS_RING_H
>+
>+asmlinkage int xprintk(const char *fmt, ...);
>+
>+
>+int xencons_ring_init(void);
>+int xencons_ring_send(const char *data, unsigned len);
>+
>+typedef void (xencons_receiver_func)(char *buf, unsigned len);
>+void xencons_ring_register_receiver(xencons_receiver_func *f);
>+
>+#endif /* _XENCONS_RING_H */
>
>_______________________________________________
>Xen-changelog mailing list
>Xen-changelog@lists.xensource.com
>http://lists.xensource.com/xen-changelog
>
>  
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Tue, Aug 30, 2005 at 12:01:08PM -0500, Anthony Liguori
wrote:
> Why are we listening for virq?  The polling method used before is 
> considerably more robust.  The virq''s aren''t always
delivered on domain
> destruction (they are only delivered if a domain crashes or is shutdown).
That would be a bug then, when does it happen?
> Moreover, I thought the overriding goal of this was to get rid of xcs?  
> Why are we still using it?
Because we haven''t switched virq delivery over to use the store.
> Also, there''s a number of errors that this code introduces
(changing
> things to use asprintf and not checking for OOM conditions).  It would 
> have been nice if this went to the list first so we could comment on it.
> I''ll submit some cleanup patches later today...
Thanks, looking forward to those patches!

    christian


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Anthony Liguori wrote:
> Also, there''s a number of errors that this code introduces
(changing
> things to use asprintf and not checking for OOM conditions).  It would 
> have been nice if this went to the list first so we could comment on it.
I''d also like to add another request.

I know the Xen mode of operation is "if you''re using xen-unstable,
follow the commits-list for changes."

However, this is turning out to be somewhat painful. I don''t want
to add further burden on developers who are putting out this code,
but it would really, really help us to get a line or two on xen-devel
as a head''s up that some code has gone in and changed the way some
user-visible piece works. Sometimes, this isn''t obvious from the list
and
sometimes, it''s not possible for all the users (quite a few who
are on xen-unstable) to track check-ins.

Documentation would be good, too :).

thanks,
Nivedita


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Christian Limpach wrote:
>On Tue, Aug 30, 2005 at 12:01:08PM -0500, Anthony Liguori wrote:
>  
>
>>Why are we listening for virq?  The polling method used before is 
>>considerably more robust.  The virq''s aren''t always
delivered on domain
>>destruction (they are only delivered if a domain crashes or is
shutdown).
>>    
>>
>
>That would be a bug then, when does it happen?
>  
>
I see from your latest checkins that you found out what I was talking about.

Although, I''m a bit confused at the semantics now.  Previously, a 
DOM_EXC VIRQ was delivered on shutdown or crash.  In both circumstances, 
there''s always going to be a destroy that occurs after it (when Xend 
decides to clean up).

Now it appears that a VIRQ is delivered on shutdown, crash, and then 
again for destroy?  This means that for some domains it''s delivered 
twice (when shutdown or crashed) and some domains it''s delivered only 
once (if they are manually xm destroyed)?

This is why I''ve not treated this as a bug previously.  I
don''t think
the new behavior is much more useful.
>>Moreover, I thought the overriding goal of this was to get rid of xcs?  
>>Why are we still using it?
>>    
>>
>
>Because we haven''t switched virq delivery over to use the store.
>  
>
With VM-Tools and consoled, I''ve convinced myself that polling is the 
right way to go for detecting events.  It''s a bunch more code but
it''s
got a lot of advantages.

I think I understand what you guys were trying to do, in consoled today 
domains are reaped only when 1) they are dead (or dying) and 2) their 
output buffers are empty.  This is to ensure that if you''re connected
to
a console and the domain crashes, you still see all the console output.

For the domain to completely reap (in the new console code), you need to 
unmap the shared frame.  I think the right way to do this is when the 
domain destroy is detected (in the polling loop), to unmap the shared 
frame but not actually reap until the above conditions are satisified.

There''s a bit of a memory leak here (sort of) if noone ever reads the 
final data for a domain.  I''ve been thinking that having a timeout is 
the right way to handle that.  That''s just not been all that much of a 
priority.
>>Also, there''s a number of errors that this code introduces
(changing
>>things to use asprintf and not checking for OOM conditions).  It would 
>>have been nice if this went to the list first so we could comment on it.
>>I''ll submit some cleanup patches later today...
>>    
>>
>
>Thanks, looking forward to those patches!
>  
>
Writing them as we speak..

Regards,

Anthony Liguori
>    christian
>
>
>  
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 8/30/05, Anthony Liguori <aliguori@us.ibm.com>
wrote:
> >>considerably more robust.  The virq''s aren''t
always delivered on domain
> >>destruction (they are only delivered if a domain crashes or is
shutdown).
> I see from your latest checkins that you found out what I was talking
about.
Yes, I hope that all cases are now covered.
> Although, I''m a bit confused at the semantics now.  Previously, a
> DOM_EXC VIRQ was delivered on shutdown or crash.  In both circumstances,
> there''s always going to be a destroy that occurs after it (when
Xend
> decides to clean up).
> 
> Now it appears that a VIRQ is delivered on shutdown, crash, and then
> again for destroy?  This means that for some domains it''s
delivered
> twice (when shutdown or crashed) and some domains it''s delivered
only
> once (if they are manually xm destroyed)?
Yes, I guess you can actually get upto 3 VIRQs now when a domain dies:
- shutdown/crash
- destroy
- final cleanup

This is ok though, anybody who listens for the VIRQ needs to handle
spurious VIRQs anyway since multiple domains ending at the same time
can raise an arbitrary number of VIRQs.  The VIRQ only tells you that
something has changed.
> I think I understand what you guys were trying to do, in consoled today
> domains are reaped only when 1) they are dead (or dying) and 2) their
> output buffers are empty.  This is to ensure that if you''re
connected to
> a console and the domain crashes, you still see all the console output.
> 
> For the domain to completely reap (in the new console code), you need to
> unmap the shared frame.  I think the right way to do this is when the
> domain destroy is detected (in the polling loop), to unmap the shared
> frame but not actually reap until the above conditions are satisified.
Yeah, at the moment we''re still missing the code which lets the output
buffer get drained after we detect that a domain has ended.

    christian

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel