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

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

[Olaf Hering]

Add new xs_reset_watches 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_WATCHES request from a guest.

v3:
  - use XS_RESET_WATCHES instead of XS_INTRODUCE

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_xs.c     |   20 ++++++++++++++++++++
 include/xen/interface/io/xs_wire.h |    6 +++++-
 2 files changed, 25 insertions(+), 1 deletion(-)

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,22 @@ static struct xenbus_watch *find_watch(c
 	return NULL;
 }
 
+static void xs_reset_watches(void)
+{
+	/* dummy, empty iov not handled by xenstored */
+	struct kvec iov[1];
+	int err;
+
+
+	iov[0].iov_base = "dummy";
+	iov[0].iov_len = strlen(iov[0].iov_base) + 1;
+
+	err = xs_error(xs_talkv(XBT_NIL, XS_RESET_WATCHES, iov,
+				 ARRAY_SIZE(iov), NULL));
+	if (err)
+		printk(KERN_WARNING "xs_reset_watches failed: %d\n", err);
+}
+
 /* Register callback to watch this node. */
 int register_xenbus_watch(struct xenbus_watch *watch)
 {
@@ -897,5 +913,9 @@ int xs_init(void)
 	if (IS_ERR(task))
 		return PTR_ERR(task);
 
+	/* shutdown watches for kexec boot */
+	if (xen_hvm_domain())
+		xs_reset_watches();
+
 	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_WATCHES
 };
 
 #define XS_WRITE_NONE "NONE"

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

[Konrad Rzeszutek Wilk]

On Mon, Aug 15, 2011 at 11:25:30AM +0200, Olaf Hering wrote:
> Add new xs_reset_watches 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.

So this patch replaces the big patch series you sent some while ago?
[I've one of your patches in my tree, but I wasn't sure about the other
ones]
> 
> 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_WATCHES request from a guest.

Ok, make sure to note the c/s when that is accepted.
> 
> v3:
>   - use XS_RESET_WATCHES instead of XS_INTRODUCE
> 
> 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_xs.c     |   20 ++++++++++++++++++++
>  include/xen/interface/io/xs_wire.h |    6 +++++-
>  2 files changed, 25 insertions(+), 1 deletion(-)
> 
> 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,22 @@ static struct xenbus_watch *find_watch(c
>  	return NULL;
>  }
>  
> +static void xs_reset_watches(void)
> +{
> +	/* dummy, empty iov not handled by xenstored */
> +	struct kvec iov[1];
> +	int err;
> +
> +
> +	iov[0].iov_base = "dummy";
> +	iov[0].iov_len = strlen(iov[0].iov_base) + 1;
> +
> +	err = xs_error(xs_talkv(XBT_NIL, XS_RESET_WATCHES, iov,
> +				 ARRAY_SIZE(iov), NULL));
> +	if (err)
> +		printk(KERN_WARNING "xs_reset_watches failed: %d\n", err);

If the xenstore does not have the patch for this, what is the
error code? Is it ENOSYS? If we get that can we not print this message?
Or perhaps print:
"Yikes! We can't reset the watches. Potential crash immienient"
or something similar.
> +}
> +
>  /* Register callback to watch this node. */
>  int register_xenbus_watch(struct xenbus_watch *watch)
>  {
> @@ -897,5 +913,9 @@ int xs_init(void)
>  	if (IS_ERR(task))
>  		return PTR_ERR(task);
>  
> +	/* shutdown watches for kexec boot */
> +	if (xen_hvm_domain())
> +		xs_reset_watches();
> +
>  	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_WATCHES
>  };
>  
>  #define XS_WRITE_NONE "NONE"
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

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

[Olaf Hering]

On Mon, Aug 15, Konrad Rzeszutek Wilk wrote:

> On Mon, Aug 15, 2011 at 11:25:30AM +0200, Olaf Hering wrote:
> > Add new xs_reset_watches 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.
> 
> So this patch replaces the big patch series you sent some while ago?
> [I've one of your patches in my tree, but I wasn't sure about the other
> ones]

No, there are 3 other patches required:

xen/pv-on-hvm kexec+kdump: reset PV devices in kexec or crash kernel
xen/pv-on-hvm kexec: rebind virqs to existing eventchannel ports
xen/pv-on-hvm kexec: prevent crash in xenwatch_thread() when stale watch events arrive

I will send them once we settled on a way to reset the watches.

Which one did you already apply?

> If the xenstore does not have the patch for this, what is the
> error code? Is it ENOSYS? If we get that can we not print this message?
> Or perhaps print:
> "Yikes! We can't reset the watches. Potential crash immienient"
> or something similar.

Now that you mention it, the return code should be checked and ENOSYS
should be filtered to not print the warning on a host without the
updated xenstored.  I will update that part of the patch.

Olaf

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

[Konrad Rzeszutek Wilk]

On Mon, Aug 15, 2011 at 02:59:58PM +0200, Olaf Hering wrote:
> On Mon, Aug 15, Konrad Rzeszutek Wilk wrote:
> 
> > On Mon, Aug 15, 2011 at 11:25:30AM +0200, Olaf Hering wrote:
> > > Add new xs_reset_watches 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.
> > 
> > So this patch replaces the big patch series you sent some while ago?
> > [I've one of your patches in my tree, but I wasn't sure about the other
> > ones]
> 
> No, there are 3 other patches required:
> 
> xen/pv-on-hvm kexec+kdump: reset PV devices in kexec or crash kernel
> xen/pv-on-hvm kexec: rebind virqs to existing eventchannel ports
> xen/pv-on-hvm kexec: prevent crash in xenwatch_thread() when stale watch events arrive
> 
> I will send them once we settled on a way to reset the watches.

Ok.
> 
> Which one did you already apply?

commit 6b71c52e7f848e2c9f804e175215e5965ea90d32
Author: Olaf Hering <olaf@aepfle.de>
Date:   Thu Jul 28 15:23:03 2011 +0200

    xen: use static initializers in xen-balloon.c
    
    There is no need to use dynamic initializaion, it just confuses the reader.
    Switch to static initializers like its used in other files.
    
    Signed-off-by: Olaf Hering <olaf@aepfle.de>
    [v2: Rebased on v3.0]

> 
> > If the xenstore does not have the patch for this, what is the
> > error code? Is it ENOSYS? If we get that can we not print this message?
> > Or perhaps print:
> > "Yikes! We can't reset the watches. Potential crash immienient"
> > or something similar.
> 
> Now that you mention it, the return code should be checked and ENOSYS
> should be filtered to not print the warning on a host without the
> updated xenstored.  I will update that part of the patch.

ok.
> 
> Olaf

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

[Olaf Hering]

Add xs_reset_watches 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_WATCHES request from a client (see changeset
23839:42a45baf037d in xen-unstable.hg). Without the patch for xenstored
the registration of watches will fail and some features of a PVonHVM
guest are not available. The guest is still able to boot, but repeated
kexec boots will fail.

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

---

Note: the patch will be available via git://... once it was reviewed

 drivers/xen/xenbus/xenbus_xs.c     |   21 +++++++++++++++++++++
 include/xen/interface/io/xs_wire.h |    3 ++-
 2 files changed, 23 insertions(+), 1 deletion(-)

Index: linux-3.2/drivers/xen/xenbus/xenbus_xs.c
===================================================================
--- linux-3.2.orig/drivers/xen/xenbus/xenbus_xs.c
+++ linux-3.2/drivers/xen/xenbus/xenbus_xs.c
@@ -621,6 +621,23 @@ static struct xenbus_watch *find_watch(c
 	return NULL;
 }
 
+static void xs_reset_watches(void)
+{
+	int err, supported = 0;
+
+	if (!xen_hvm_domain())
+		return;
+
+	err = xenbus_scanf(XBT_NIL, "control",
+			"platform-feature-xs_reset_watches", "%d", &supported);
+	if (err != 1 || !supported)
+		return;
+
+	err = xs_error(xs_single(XBT_NIL, XS_RESET_WATCHES, "", NULL));
+	if (err && err != -EEXIST)
+		printk(KERN_WARNING "xs_reset_watches failed: %d\n", err);
+}
+
 /* Register callback to watch this node. */
 int register_xenbus_watch(struct xenbus_watch *watch)
 {
@@ -897,5 +915,8 @@ int xs_init(void)
 	if (IS_ERR(task))
 		return PTR_ERR(task);
 
+	/* shutdown watches for kexec boot */
+	xs_reset_watches();
+
 	return 0;
 }
Index: linux-3.2/include/xen/interface/io/xs_wire.h
===================================================================
--- linux-3.2.orig/include/xen/interface/io/xs_wire.h
+++ linux-3.2/include/xen/interface/io/xs_wire.h
@@ -29,7 +29,8 @@ enum xsd_sockmsg_type
     XS_IS_DOMAIN_INTRODUCED,
     XS_RESUME,
     XS_SET_TARGET,
-    XS_RESTRICT
+    XS_RESTRICT,
+    XS_RESET_WATCHES
 };
 
 #define XS_WRITE_NONE "NONE"

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

[Konrad Rzeszutek Wilk]

On Thu, Jan 05, 2012 at 09:41:26PM +0100, Olaf Hering wrote:
> Add xs_reset_watches 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_WATCHES request from a client (see changeset
> 23839:42a45baf037d in xen-unstable.hg). Without the patch for xenstored
> the registration of watches will fail and some features of a PVonHVM
> guest are not available. The guest is still able to boot, but repeated
> kexec boots will fail.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>

Please resend and CC the maintainer and as well LKML.

> 
> ---
> 
> Note: the patch will be available via git://... once it was reviewed

huh?

> 
>  drivers/xen/xenbus/xenbus_xs.c     |   21 +++++++++++++++++++++
>  include/xen/interface/io/xs_wire.h |    3 ++-
>  2 files changed, 23 insertions(+), 1 deletion(-)
> 
> Index: linux-3.2/drivers/xen/xenbus/xenbus_xs.c
> ===================================================================
> --- linux-3.2.orig/drivers/xen/xenbus/xenbus_xs.c
> +++ linux-3.2/drivers/xen/xenbus/xenbus_xs.c
> @@ -621,6 +621,23 @@ static struct xenbus_watch *find_watch(c
>  	return NULL;
>  }
>  
> +static void xs_reset_watches(void)
> +{
> +	int err, supported = 0;
> +
> +	if (!xen_hvm_domain())
> +		return;
> +
> +	err = xenbus_scanf(XBT_NIL, "control",
> +			"platform-feature-xs_reset_watches", "%d", &supported);
> +	if (err != 1 || !supported)
> +		return;
> +
> +	err = xs_error(xs_single(XBT_NIL, XS_RESET_WATCHES, "", NULL));
> +	if (err && err != -EEXIST)
> +		printk(KERN_WARNING "xs_reset_watches failed: %d\n", err);
> +}
> +
>  /* Register callback to watch this node. */
>  int register_xenbus_watch(struct xenbus_watch *watch)
>  {
> @@ -897,5 +915,8 @@ int xs_init(void)
>  	if (IS_ERR(task))
>  		return PTR_ERR(task);
>  
> +	/* shutdown watches for kexec boot */
> +	xs_reset_watches();
> +
>  	return 0;
>  }
> Index: linux-3.2/include/xen/interface/io/xs_wire.h
> ===================================================================
> --- linux-3.2.orig/include/xen/interface/io/xs_wire.h
> +++ linux-3.2/include/xen/interface/io/xs_wire.h
> @@ -29,7 +29,8 @@ enum xsd_sockmsg_type
>      XS_IS_DOMAIN_INTRODUCED,
>      XS_RESUME,
>      XS_SET_TARGET,
> -    XS_RESTRICT
> +    XS_RESTRICT,
> +    XS_RESET_WATCHES
>  };
>  
>  #define XS_WRITE_NONE "NONE"
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel