[PATCH 0/3] [v5] kexec and kdump for Xen PVonHVM guests

[PATCH 0/3] [v5] kexec and kdump for Xen PVonHVM guests

[Olaf Hering]

The following series implements kexec and kdump in a Xen PVonHVM guest.

It is available via git:

 git://github.com/olafhering/linux.git xen-kexec-3.0

changes since v4:
- drop patch which uses XS_INTRODUCE, a different way to reset watches
  has to be found
- find_virq(): use NR_EVENT_CHANNELS instead of private MAX_EVTCHNS
- reconnect devices:
  add timeout when waiting for backend state change
  extent printk message to include backend string
  add comment to fall-through case in xenbus_reset_frontend
- new patch which prevents crash in xenwatch_thread() during repeated
  kexec boots


The kexec or kdump kernel has to take care of already allocated virqs,
PV devices in Closed or Connected state, and of registered watches in
the old kernel. With the three patches these conditions are checked
during boot of the new kernel rather than in the reboot/crash path.

A fixed kexec-tools-2.0.2 package is required:
http://lists.infradead.org/pipermail/kexec/2011-May/005026.html
http://lists.infradead.org/pipermail/kexec/2011-August/005339.html

Another fix is for xenstored, it has to accept the XS_RESET_WATCHES from
a guest. Since the exact interface to xenstored is not yet final, the
patch to deal with the reset was removed from this series.
http://lists.xensource.com/archives/html/xen-devel/2011-08/msg00529.html
http://lists.xensource.com/archives/html/xen-devel/2011-08/msg00533.html

One open issue is the balloon driver. It removes pages from the guest
and gives them back to the hypervisor. The kexec kernel is not aware of
the fact that some pages are unavailable, and hangs or crashes.
The workaround for the time being is:

if test -f /sys/devices/system/xen_memory/xen_memory0/target -a \
        -f /sys/devices/system/xen_memory/xen_memory0/target_kb
then
    cat /sys/devices/system/xen_memory/xen_memory0/target > \
        /sys/devices/system/xen_memory/xen_memory0/target_kb
fi
kexec -e

This balloon issue has to be resolved with another series of changes.

Olaf

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

 drivers/xen/events.c                       |   37 +++++++-
 drivers/xen/xenbus/xenbus_comms.c          |    4 +-
 drivers/xen/xenbus/xenbus_probe_frontend.c |  121 ++++++++++++++++++++++++++++
 drivers/xen/xenbus/xenbus_xs.c             |    2 +-
 4 files changed, 157 insertions(+), 7 deletions(-)

-- 
1.7.3.4

[PATCH 1/3] xen/pv-on-hvm kexec: prevent crash in xenwatch_thread() when stale watch events arrive

[Olaf Hering]

During repeated kexec boots xenwatch_thread() can crash because
xenbus_watch->callback is cleared by xenbus_watch_path() if a node/token
combo for a new watch happens to match an already registered watch from
an old kernel.  In this case xs_watch returns -EEXISTS, then
register_xenbus_watch() does not remove the to-be-registered watch from
the list of active watches but returns the -EEXISTS to the caller
anyway.

Because the watch is still active in xenstored it will cause an event
which will arrive in the new kernel. process_msg() will find the
encapsulated struct xenbus_watch in its list of registered watches and
puts the "empty" watch handle in the queue for xenwatch_thread().
xenwatch_thread() then calls ->callback which was cleared earlier by
xenbus_watch_path().

To prevent that crash in a guest running on an old xen toolstack, add a
check wether xenbus_watch->callback is active.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 drivers/xen/xenbus/xenbus_xs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c
index 5534690..64248b2 100644
--- a/drivers/xen/xenbus/xenbus_xs.c
+++ b/drivers/xen/xenbus/xenbus_xs.c
@@ -828,7 +828,7 @@ static int process_msg(void)
 		spin_lock(&watches_lock);
 		msg->u.watch.handle = find_watch(
 			msg->u.watch.vec[XS_WATCH_TOKEN]);
-		if (msg->u.watch.handle != NULL) {
+		if (msg->u.watch.handle && msg->u.watch.handle->callback) {
 			spin_lock(&watch_events_lock);
 			list_add_tail(&msg->list, &watch_events);
 			wake_up(&watch_events_waitq);
-- 
1.7.3.4

Re: [Xen-devel] [PATCH 1/3] xen/pv-on-hvm kexec: prevent crash in xenwatch_thread() when stale watch events arrive

[Ian Campbell]

On Tue, 2011-08-16 at 14:16 +0100, Olaf Hering wrote:
> During repeated kexec boots xenwatch_thread() can crash because
> xenbus_watch->callback is cleared by xenbus_watch_path() if a node/token
> combo for a new watch happens to match an already registered watch from
> an old kernel.  In this case xs_watch returns -EEXISTS, then
> register_xenbus_watch() does not remove the to-be-registered watch from
> the list of active watches but returns the -EEXISTS to the caller
> anyway.

Isn't this behaviour the root cause of the issue (which should be fixed)
rather than papering over it during watch processing. IOW should't
register_xenbus_watch cleanup after itself if xs_watch fails.

> 
> Because the watch is still active in xenstored it will cause an event
> which will arrive in the new kernel. process_msg() will find the
> encapsulated struct xenbus_watch in its list of registered watches and
> puts the "empty" watch handle in the queue for xenwatch_thread().
> xenwatch_thread() then calls ->callback which was cleared earlier by
> xenbus_watch_path().
> 
> To prevent that crash in a guest running on an old xen toolstack, add a
> check wether xenbus_watch->callback is active.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
>  drivers/xen/xenbus/xenbus_xs.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c
> index 5534690..64248b2 100644
> --- a/drivers/xen/xenbus/xenbus_xs.c
> +++ b/drivers/xen/xenbus/xenbus_xs.c
> @@ -828,7 +828,7 @@ static int process_msg(void)
>  		spin_lock(&watches_lock);
>  		msg->u.watch.handle = find_watch(
>  			msg->u.watch.vec[XS_WATCH_TOKEN]);
> -		if (msg->u.watch.handle != NULL) {
> +		if (msg->u.watch.handle && msg->u.watch.handle->callback) {
>  			spin_lock(&watch_events_lock);
>  			list_add_tail(&msg->list, &watch_events);
>  			wake_up(&watch_events_waitq);

Re: [Xen-devel] [PATCH 1/3] xen/pv-on-hvm kexec: prevent crash in xenwatch_thread() when stale watch events arrive

[Olaf Hering]

On Tue, Aug 16, Ian Campbell wrote:

> On Tue, 2011-08-16 at 14:16 +0100, Olaf Hering wrote:
> > During repeated kexec boots xenwatch_thread() can crash because
> > xenbus_watch->callback is cleared by xenbus_watch_path() if a node/token
> > combo for a new watch happens to match an already registered watch from
> > an old kernel.  In this case xs_watch returns -EEXISTS, then
> > register_xenbus_watch() does not remove the to-be-registered watch from
> > the list of active watches but returns the -EEXISTS to the caller
> > anyway.
> 
> Isn't this behaviour the root cause of the issue (which should be fixed)
> rather than papering over it during watch processing. IOW should't
> register_xenbus_watch cleanup after itself if xs_watch fails.

Keir, the EEXISTS case in register_xenbus_watch() was added by you 6
years ago. Do you happen to know what it tried to solve, and do these
conditions still apply today?  Perhaps the EEXISTS can be removed now.

http://xenbits.xen.org/hg/xen-unstable.hg/diff/8016551fde98/linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_xs.c

Olaf

Re: [PATCH 1/3] xen/pv-on-hvm kexec: prevent crash in xenwatch_thread() when stale watch events arrive

[Keir Fraser]

On 17/08/2011 13:51, "Olaf Hering" <olaf@aepfle.de> wrote:

> On Tue, Aug 16, Ian Campbell wrote:
> 
>> On Tue, 2011-08-16 at 14:16 +0100, Olaf Hering wrote:
>>> During repeated kexec boots xenwatch_thread() can crash because
>>> xenbus_watch->callback is cleared by xenbus_watch_path() if a node/token
>>> combo for a new watch happens to match an already registered watch from
>>> an old kernel.  In this case xs_watch returns -EEXISTS, then
>>> register_xenbus_watch() does not remove the to-be-registered watch from
>>> the list of active watches but returns the -EEXISTS to the caller
>>> anyway.
>> 
>> Isn't this behaviour the root cause of the issue (which should be fixed)
>> rather than papering over it during watch processing. IOW should't
>> register_xenbus_watch cleanup after itself if xs_watch fails.
> 
> Keir, the EEXISTS case in register_xenbus_watch() was added by you 6
> years ago. Do you happen to know what it tried to solve, and do these
> conditions still apply today?  Perhaps the EEXISTS can be removed now.
> 
> http://xenbits.xen.org/hg/xen-unstable.hg/diff/8016551fde98/linux-2.6-xen-spar
> se/drivers/xen/xenbus/xenbus_xs.c

Bad me. Either remove the EEXIST check, or convert EEXIST to return code 0
in register_xenbus_watch(). You could do either, since I'm sure I added the
EEXIST check only as an attempt to theoretically robustify that function,
and looks like I got it wrong.

 K.

> Olaf

[PATCH 2/3] xen/pv-on-hvm kexec: rebind virqs to existing eventchannel ports

[Olaf Hering]

During a kexec boot some virqs such as timer and debugirq were already
registered by the old kernel.  The hypervisor will return -EEXISTS from
the new EVTCHNOP_bind_virq request and the BUG in bind_virq_to_irq()
triggers.  Catch the -EEXISTS error and loop through all possible ports to find
what port belongs to the virq/cpu combo.

v2:
  - use NR_EVENT_CHANNELS instead of private MAX_EVTCHNS

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 drivers/xen/events.c |   37 ++++++++++++++++++++++++++++++++-----
 1 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 30df85d..31493e9 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -877,11 +877,32 @@ static int bind_interdomain_evtchn_to_irq(unsigned int remote_domain,
 	return err ? : bind_evtchn_to_irq(bind_interdomain.local_port);
 }
 
+static int find_virq(unsigned int virq, unsigned int cpu)
+{
+	struct evtchn_status status;
+	int port, rc = -ENOENT;
+
+	memset(&status, 0, sizeof(status));
+	for (port = 0; port <= NR_EVENT_CHANNELS; port++) {
+		status.dom = DOMID_SELF;
+		status.port = port;
+		rc = HYPERVISOR_event_channel_op(EVTCHNOP_status, &status);
+		if (rc < 0)
+			continue;
+		if (status.status != EVTCHNSTAT_virq)
+			continue;
+		if (status.u.virq == virq && status.vcpu == cpu) {
+			rc = port;
+			break;
+		}
+	}
+	return rc;
+}
 
 int bind_virq_to_irq(unsigned int virq, unsigned int cpu)
 {
 	struct evtchn_bind_virq bind_virq;
-	int evtchn, irq;
+	int evtchn, irq, ret;
 
 	spin_lock(&irq_mapping_update_lock);
 
@@ -897,10 +918,16 @@ int bind_virq_to_irq(unsigned int virq, unsigned int cpu)
 
 		bind_virq.virq = virq;
 		bind_virq.vcpu = cpu;
-		if (HYPERVISOR_event_channel_op(EVTCHNOP_bind_virq,
-						&bind_virq) != 0)
-			BUG();
-		evtchn = bind_virq.port;
+		ret = HYPERVISOR_event_channel_op(EVTCHNOP_bind_virq,
+						&bind_virq);
+		if (ret == 0)
+			evtchn = bind_virq.port;
+		else {
+			if (ret == -EEXIST)
+				ret = find_virq(virq, cpu);
+			BUG_ON(ret < 0);
+			evtchn = ret;
+		}
 
 		xen_irq_info_virq_init(cpu, irq, evtchn, virq);
 
-- 
1.7.3.4

[PATCH 3/3] xen/pv-on-hvm kexec+kdump: reset PV devices in kexec or crash kernel

[Olaf Hering]

After triggering a crash dump in a HVM guest, the PV backend drivers
will remain in Connected state. When the kdump kernel starts the PV
drivers will skip such devices. As a result, no root device is found and
the vmcore cant be saved.

A similar situation happens after a kexec boot, here the devices will be
in the Closed state.

With this change all frontend devices with state XenbusStateConnected or
XenbusStateClosed will be reset by changing the state file to Closing ->
Closed -> Initializing.  This will trigger a disconnect in the backend
drivers. Now the frontend drivers will find the backend drivers in state
Initwait and can connect.

v2:
  - add timeout when waiting for backend state change
  (based on feedback from Ian Campell)
  - extent printk message to include backend string
  - add comment to fall-through case in xenbus_reset_frontend

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 drivers/xen/xenbus/xenbus_comms.c          |    4 +-
 drivers/xen/xenbus/xenbus_probe_frontend.c |  121 ++++++++++++++++++++++++++++
 2 files changed, 124 insertions(+), 1 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_comms.c b/drivers/xen/xenbus/xenbus_comms.c
index 090c61e..2eff7a6 100644
--- a/drivers/xen/xenbus/xenbus_comms.c
+++ b/drivers/xen/xenbus/xenbus_comms.c
@@ -212,7 +212,9 @@ int xb_init_comms(void)
 		printk(KERN_WARNING "XENBUS response ring is not quiescent "
 		       "(%08x:%08x): fixing up\n",
 		       intf->rsp_cons, intf->rsp_prod);
-		intf->rsp_cons = intf->rsp_prod;
+		/* breaks kdump */
+		if (!reset_devices)
+			intf->rsp_cons = intf->rsp_prod;
 	}
 
 	if (xenbus_irq) {
diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c b/drivers/xen/xenbus/xenbus_probe_frontend.c
index b6a2690..b521ce4 100644
--- a/drivers/xen/xenbus/xenbus_probe_frontend.c
+++ b/drivers/xen/xenbus/xenbus_probe_frontend.c
@@ -252,10 +252,131 @@ int __xenbus_register_frontend(struct xenbus_driver *drv,
 }
 EXPORT_SYMBOL_GPL(__xenbus_register_frontend);
 
+static DECLARE_WAIT_QUEUE_HEAD(backend_state_wq);
+static int backend_state;
+
+static void xenbus_reset_backend_state_changed(struct xenbus_watch *w,
+					const char **v, unsigned int l)
+{
+	xenbus_scanf(XBT_NIL, v[XS_WATCH_PATH], "", "%i", &backend_state);
+	printk(KERN_DEBUG "XENBUS: backend %s %s\n",
+			v[XS_WATCH_PATH], xenbus_strstate(backend_state));
+	wake_up(&backend_state_wq);
+}
+
+static void xenbus_reset_wait_for_backend(char *be, int expected)
+{
+	long timeout;
+	timeout = wait_event_interruptible_timeout(backend_state_wq,
+			backend_state == expected, 5 * HZ);
+	if (timeout <= 0)
+		printk(KERN_INFO "XENBUS: backend %s timed out.\n", be);
+}
+
+/*
+ * Reset frontend if it is in Connected or Closed state.
+ * Wait for backend to catch up.
+ * State Connected happens during kdump, Closed after kexec.
+ */
+static void xenbus_reset_frontend(char *fe, char *be, int be_state)
+{
+	struct xenbus_watch be_watch;
+
+	printk(KERN_DEBUG "XENBUS: backend %s %s\n",
+			be, xenbus_strstate(be_state));
+
+	memset(&be_watch, 0, sizeof(be_watch));
+	be_watch.node = kasprintf(GFP_NOIO | __GFP_HIGH, "%s/state", be);
+	if (!be_watch.node)
+		return;
+
+	be_watch.callback = xenbus_reset_backend_state_changed;
+	backend_state = XenbusStateUnknown;
+
+	printk(KERN_INFO "XENBUS: triggering reconnect on %s\n", be);
+	register_xenbus_watch(&be_watch);
+
+	/* fall through to forward backend to state XenbusStateInitialising */
+	switch (be_state) {
+	case XenbusStateConnected:
+		xenbus_printf(XBT_NIL, fe, "state", "%d", XenbusStateClosing);
+		xenbus_reset_wait_for_backend(be, XenbusStateClosing);
+
+	case XenbusStateClosing:
+		xenbus_printf(XBT_NIL, fe, "state", "%d", XenbusStateClosed);
+		xenbus_reset_wait_for_backend(be, XenbusStateClosed);
+
+	case XenbusStateClosed:
+		xenbus_printf(XBT_NIL, fe, "state", "%d", XenbusStateInitialising);
+		xenbus_reset_wait_for_backend(be, XenbusStateInitWait);
+	}
+
+	unregister_xenbus_watch(&be_watch);
+	printk(KERN_INFO "XENBUS: reconnect done on %s\n", be);
+	kfree(be_watch.node);
+}
+
+static void xenbus_check_frontend(char *class, char *dev)
+{
+	int be_state, fe_state, err;
+	char *backend, *frontend;
+
+	frontend = kasprintf(GFP_NOIO | __GFP_HIGH, "device/%s/%s", class, dev);
+	if (!frontend)
+		return;
+
+	err = xenbus_scanf(XBT_NIL, frontend, "state", "%i", &fe_state);
+	if (err != 1)
+		goto out;
+
+	switch (fe_state) {
+	case XenbusStateConnected:
+	case XenbusStateClosed:
+		printk(KERN_DEBUG "XENBUS: frontend %s %s\n",
+				frontend, xenbus_strstate(fe_state));
+		backend = xenbus_read(XBT_NIL, frontend, "backend", NULL);
+		if (!backend || IS_ERR(backend))
+			goto out;
+		err = xenbus_scanf(XBT_NIL, backend, "state", "%i", &be_state);
+		if (err == 1)
+			xenbus_reset_frontend(frontend, backend, be_state);
+		kfree(backend);
+		break;
+	default:
+		break;
+	}
+out:
+	kfree(frontend);
+}
+
+static void xenbus_reset_state(void)
+{
+	char **devclass, **dev;
+	int devclass_n, dev_n;
+	int i, j;
+
+	devclass = xenbus_directory(XBT_NIL, "device", "", &devclass_n);
+	if (IS_ERR(devclass))
+		return;
+
+	for (i = 0; i < devclass_n; i++) {
+		dev = xenbus_directory(XBT_NIL, "device", devclass[i], &dev_n);
+		if (IS_ERR(dev))
+			continue;
+		for (j = 0; j < dev_n; j++)
+			xenbus_check_frontend(devclass[i], dev[j]);
+		kfree(dev);
+	}
+	kfree(devclass);
+}
+
 static int frontend_probe_and_watch(struct notifier_block *notifier,
 				   unsigned long event,
 				   void *data)
 {
+	/* reset devices in Connected or Closed state */
+	if (xen_hvm_domain())
+		xenbus_reset_state();
 	/* Enumerate devices in xenstore and watch for changes. */
 	xenbus_probe_devices(&xenbus_frontend);
 	register_xenbus_watch(&fe_watch);
-- 
1.7.3.4