[PATCH] xenstored: allow guests to shutdown all of its watches using XS_RESET

[PATCH] xenstored: allow guests to shutdown all of its watches using XS_RESET

[Olaf Hering]

# HG changeset patch
# User Olaf Hering <olaf@aepfle.de>
# Date 1313090526 -7200
# Node ID 31687c9a9e3edfae7cf23483c8457b011dc4d843
# Parent  1f08b380d4386cdd6714786a9163e5f51aecab5d
xenstored: allow guests to shutdown all of its watches using XS_RESET

During kexec all old watches have to be removed, otherwise the new
kernel will receive unexpected events. Allow a guest to reset itself and
cleanup all of its watches.

Add a new XS_RESET command to do the reset on behalf of the guest.

Signed-off-by: Olaf Hering <olaf@aepfle.de>

diff -r 1f08b380d438 -r 31687c9a9e3e tools/xenstore/xenstored_core.c
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -120,6 +120,7 @@ static char *sockmsg_string(enum xsd_soc
 	case XS_IS_DOMAIN_INTRODUCED: return "XS_IS_DOMAIN_INTRODUCED";
 	case XS_RESUME: return "RESUME";
 	case XS_SET_TARGET: return "SET_TARGET";
+	case XS_RESET: return "RESET";
 	default:
 		return "**UNKNOWN**";
 	}
@@ -1242,6 +1243,10 @@ static void process_message(struct conne
 		do_set_target(conn, in);
 		break;
 
+	case XS_RESET:
+		do_reset(conn, in);
+		break;
+
 	default:
 		eprintf("Client unknown operation %i", in->hdr.msg.type);
 		send_error(conn, ENOSYS);
diff -r 1f08b380d438 -r 31687c9a9e3e tools/xenstore/xenstored_domain.c
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -537,6 +537,57 @@ void do_is_domain_introduced(struct conn
 	send_reply(conn, XS_IS_DOMAIN_INTRODUCED, result ? "T" : "F", 2);
 }
 
+/* domid, mfn, evtchn, path */
+void do_reset(struct connection *conn, struct buffered_data *in)
+{
+	struct domain *domain;
+	char *vec[3];
+	unsigned int domid;
+	unsigned long mfn;
+	evtchn_port_t port;
+
+	if (!conn->can_write) {
+		send_error(conn, EACCES);
+		return;
+	}
+
+	if (get_strings(in, vec, ARRAY_SIZE(vec)) < ARRAY_SIZE(vec)) {
+		send_error(conn, EINVAL);
+		return;
+	}
+
+	domid = atoi(vec[0]);
+	mfn = atol(vec[1]);
+	port = atoi(vec[2]);
+
+	/* Sanity check args. */
+	if (port <= 0) {
+		send_error(conn, EINVAL);
+		return;
+	}
+	/* Allow guest to reset all watches */
+	if (domid != DOMID_SELF && conn->id != 0) {
+		send_error(conn, EACCES);
+		return;
+	}
+
+	domain = find_domain_by_domid(conn->id);
+
+	if (domain == NULL) {
+		send_error(conn, EINVAL);
+		return;
+	}
+
+	if (domain->mfn != mfn || domain->conn != conn || domain->port != port) {
+		send_error(conn, EACCES);
+		return;
+	}
+
+	domain_conn_reset(domain);
+
+	send_ack(conn, XS_RESET);
+}
+
 static int close_xc_handle(void *_handle)
 {
 	xc_interface_close(*(xc_interface**)_handle);
diff -r 1f08b380d438 -r 31687c9a9e3e tools/xenstore/xenstored_domain.h
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -40,6 +40,9 @@ void do_set_target(struct connection *co
 /* domid */
 void do_get_domain_path(struct connection *conn, const char *domid_str);
 
+/* domid, mfn, eventchn, path */
+void do_reset(struct connection *conn, struct buffered_data *in);
+
 void domain_init(void);
 
 /* Returns the implicit path of a connection (only domains have this) */
diff -r 1f08b380d438 -r 31687c9a9e3e xen/include/public/io/xs_wire.h
--- a/xen/include/public/io/xs_wire.h
+++ b/xen/include/public/io/xs_wire.h
@@ -48,7 +48,8 @@ enum xsd_sockmsg_type
     XS_IS_DOMAIN_INTRODUCED,
     XS_RESUME,
     XS_SET_TARGET,
-    XS_RESTRICT
+    XS_RESTRICT,
+    XS_RESET
 };
 
 #define XS_WRITE_NONE "NONE"

[PATCH] xen/pv-on-hvm kexec: add xs_reset to shutdown watches from old kernel

[Olaf Hering]

Add new xs_reset function to shutdown watches from old kernel after
kexec boot.  The old kernel does not unregister all watches in the
shutdown path.  They are still active, the double registration can not
be detected by the new kernel.  When the watches fire, unexpected events
will arrive and the xenwatch thread will crash (jumps to NULL).  An
orderly reboot of a hvm guest will destroy the entire guest with all its
resources (including the watches) before it is rebuilt from scratch, so
the missing unregister is not an issue in that case.

With this change the xenstored is instructed to wipe all active watches
for the guest.  However, a patch for xenstored is required so that it
accepts the XS_RESET request from a guest.

http://lists.xensource.com/archives/html/xen-devel/2011-08/ <tbd.html>

Also sync the XS_* enum with xen-unstable version.


v3:
  - use XS_RESET instead of XS_INTRODUCE
  - sync XS_* enum from xen-unstable

v2:
  - move all code which deals with XS_INTRODUCE into xs_introduce()
    (based on feedback from Ian Campbell)
  - remove casts from kvec assignment

Signed-off-by: Olaf Hering <olaf@aepfle.de>

---
 drivers/xen/xenbus/xenbus_comms.h  |    2 +-
 drivers/xen/xenbus/xenbus_probe.c  |    2 +-
 drivers/xen/xenbus/xenbus_xs.c     |   29 ++++++++++++++++++++++++++++-
 include/xen/interface/io/xs_wire.h |    6 +++++-
 4 files changed, 35 insertions(+), 4 deletions(-)

Index: linux-3.0/drivers/xen/xenbus/xenbus_comms.h
===================================================================
--- linux-3.0.orig/drivers/xen/xenbus/xenbus_comms.h
+++ linux-3.0/drivers/xen/xenbus/xenbus_comms.h
@@ -31,7 +31,7 @@
 #ifndef _XENBUS_COMMS_H
 #define _XENBUS_COMMS_H
 
-int xs_init(void);
+int xs_init(unsigned long xen_store_mfn);
 int xb_init_comms(void);
 
 /* Low level routines. */
Index: linux-3.0/drivers/xen/xenbus/xenbus_probe.c
===================================================================
--- linux-3.0.orig/drivers/xen/xenbus/xenbus_probe.c
+++ linux-3.0/drivers/xen/xenbus/xenbus_probe.c
@@ -757,7 +757,7 @@ static int __init xenbus_init(void)
 	}
 
 	/* Initialize the interface to xenstore. */
-	err = xs_init();
+	err = xs_init(xen_store_mfn);
 	if (err) {
 		printk(KERN_WARNING
 		       "XENBUS: Error initializing xenstore comms: %i\n", err);
Index: linux-3.0/drivers/xen/xenbus/xenbus_xs.c
===================================================================
--- linux-3.0.orig/drivers/xen/xenbus/xenbus_xs.c
+++ linux-3.0/drivers/xen/xenbus/xenbus_xs.c
@@ -620,6 +620,29 @@ static struct xenbus_watch *find_watch(c
 	return NULL;
 }
 
+static void xs_reset(unsigned long xen_store_mfn)
+{
+	struct kvec iov[3];
+	char domid[12], mfn[24], port[24];
+	int err;
+
+	snprintf(domid, sizeof(domid), "%u", DOMID_SELF);
+	snprintf(mfn, sizeof(mfn), "%lu", xen_store_mfn);
+	snprintf(port, sizeof(port), "%d", xen_store_evtchn);
+
+	iov[0].iov_base = domid;
+	iov[0].iov_len = strlen(domid) + 1;
+	iov[1].iov_base = mfn;
+	iov[1].iov_len = strlen(mfn) + 1;
+	iov[2].iov_base = port;
+	iov[2].iov_len = strlen(port) + 1;
+
+	err = xs_error(xs_talkv(XBT_NIL, XS_RESET, iov,
+				 ARRAY_SIZE(iov), NULL));
+	if (err)
+		printk(KERN_WARNING "xs_reset failed: %d\n", err);
+}
+
 /* Register callback to watch this node. */
 int register_xenbus_watch(struct xenbus_watch *watch)
 {
@@ -867,7 +890,7 @@ static int xenbus_thread(void *unused)
 	return 0;
 }
 
-int xs_init(void)
+int xs_init(unsigned long xen_store_mfn)
 {
 	int err;
 	struct task_struct *task;
@@ -897,5 +920,9 @@ int xs_init(void)
 	if (IS_ERR(task))
 		return PTR_ERR(task);
 
+	/* shutdown watches for kexec boot */
+	if (xen_hvm_domain())
+		xs_reset(xen_store_mfn);
+
 	return 0;
 }
Index: linux-3.0/include/xen/interface/io/xs_wire.h
===================================================================
--- linux-3.0.orig/include/xen/interface/io/xs_wire.h
+++ linux-3.0/include/xen/interface/io/xs_wire.h
@@ -26,7 +26,11 @@ enum xsd_sockmsg_type
     XS_SET_PERMS,
     XS_WATCH_EVENT,
     XS_ERROR,
-    XS_IS_DOMAIN_INTRODUCED
+    XS_IS_DOMAIN_INTRODUCED,
+    XS_RESUME,
+    XS_SET_TARGET,
+    XS_RESTRICT,
+    XS_RESET
 };
 
 #define XS_WRITE_NONE "NONE"

Re: [PATCH] xenstored: allow guests to shutdown all of its watches using XS_RESET

[Ian Jackson]

Olaf Hering writes ("[Xen-devel] [PATCH] xenstored: allow guests to shutdown all of its watches using XS_RESET"):
> Add a new XS_RESET command to do the reset on behalf of the guest.

I'm afraid I still think this is wrong.

* The XS_RESET command as you have it takes too many arguments.  It
  should probably not take any arguments at all.

* You have failed to update the specification document which describes
  the xenstored protocol (in docs/misc/)

* Your XS_RESET command doesn't just delete watches and transactions.
  It also:
     - discards outstanding responses to earlier commands
     - discards potential incoming partial commands
     - resets the ring pointers
  I don't think any of these things are necessary.  If they are,
  please explain.  I think you should have your do_reset simply call
  conn_delete_all_watches and conn_delete_all_transactions.

Thanks,
Ian.

Re: [PATCH] xenstored: allow guests to shutdown all of its watches using XS_RESET

[Olaf Hering]

On Thu, Aug 25, Ian Jackson wrote:

> Olaf Hering writes ("[Xen-devel] [PATCH] xenstored: allow guests to shutdown all of its watches using XS_RESET"):
> > Add a new XS_RESET command to do the reset on behalf of the guest.
> 
> I'm afraid I still think this is wrong.

There is a later patch XS_RESET_WATCHES.
http://lists.xensource.com/archives/html/xen-devel/2011-08/msg00529.html

> * The XS_RESET command as you have it takes too many arguments.  It
>   should probably not take any arguments at all.

The later patch does not take arguments.

> * You have failed to update the specification document which describes
>   the xenstored protocol (in docs/misc/)

The later patch adds the XS_RESET_WATCHES part to the xenstored.txt.

> * Your XS_RESET command doesn't just delete watches and transactions.
>   It also:
>      - discards outstanding responses to earlier commands
>      - discards potential incoming partial commands
>      - resets the ring pointers
>   I don't think any of these things are necessary.  If they are,
>   please explain.  I think you should have your do_reset simply call
>   conn_delete_all_watches and conn_delete_all_transactions.

Thats what the later patch does.

Olaf

Re: [PATCH] xenstored: allow guests to shutdown all of its watches using XS_RESET

[Ian Jackson]

Olaf Hering writes ("Re: [Xen-devel] [PATCH] xenstored: allow guests to shutdown all of its watches using XS_RESET"):
> Thats what the later patch does.

Marvellous.  Sorry, I'm a bit behind as you can tell.  I'll get to
that one soon I hope ...

Ian.