[PATCH 0/6] misc changes for kexec in pv-on-hvm guests

[PATCH 0/6] misc changes for kexec in pv-on-hvm guests

[Olaf Hering]

The following series partly fixes kexec in a pv-on-hvm guest. After a few
iterations of kexec boots the guest will panic with memory corruption.

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

Olaf

[PATCH 1/6] xen: use static initializers in xen-balloon.c

[Olaf Hering]

[-- Attachment #1: xen.balloon.static_initializer.patch --]
[-- Type: text/plain, Size: 1619 bytes --]

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

---
 drivers/xen/xen-balloon.c |   18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

Index: linux-3.0/drivers/xen/xen-balloon.c
===================================================================
--- linux-3.0.orig/drivers/xen/xen-balloon.c
+++ linux-3.0/drivers/xen/xen-balloon.c
@@ -50,11 +50,6 @@ static struct sys_device balloon_sysdev;
 
 static int register_balloon(struct sys_device *sysdev);
 
-static struct xenbus_watch target_watch =
-{
-	.node = "memory/target"
-};
-
 /* React to a change in the target key */
 static void watch_target(struct xenbus_watch *watch,
 			 const char **vec, unsigned int len)
@@ -74,6 +69,11 @@ static void watch_target(struct xenbus_w
 	balloon_set_new_target(new_target >> (PAGE_SHIFT - 10));
 }
 
+static struct xenbus_watch target_watch = {
+	.node = "memory/target",
+	.callback = watch_target,
+};
+
 static int balloon_init_watcher(struct notifier_block *notifier,
 				unsigned long event,
 				void *data)
@@ -87,7 +87,9 @@ static int balloon_init_watcher(struct n
 	return NOTIFY_DONE;
 }
 
-static struct notifier_block xenstore_notifier;
+static struct notifier_block xenstore_notifier = {
+	.notifier_call = balloon_init_watcher,
+};
 
 static int __init balloon_init(void)
 {
@@ -97,10 +99,6 @@ static int __init balloon_init(void)
 	pr_info("xen-balloon: Initialising balloon driver.\n");
 
 	register_balloon(&balloon_sysdev);
-
-	target_watch.callback = watch_target;
-	xenstore_notifier.notifier_call = balloon_init_watcher;
-
 	register_xenstore_notifier(&xenstore_notifier);
 
 	return 0;

Re: [PATCH 1/6] xen: use static initializers in xen-balloon.c

[Konrad Rzeszutek Wilk]

On Tue, Jul 26, 2011 at 01:52:10PM +0200, Olaf Hering wrote:

You need a description of why you are doing this.

And also make sure you use git, not patches.

Lastly, please also CC the LKML mailing list and
whoever shows up as you run scripts/get_maintainer.pl
for the patches.

> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> 
> ---
>  drivers/xen/xen-balloon.c |   18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> Index: linux-3.0/drivers/xen/xen-balloon.c
> ===================================================================
> --- linux-3.0.orig/drivers/xen/xen-balloon.c
> +++ linux-3.0/drivers/xen/xen-balloon.c
> @@ -50,11 +50,6 @@ static struct sys_device balloon_sysdev;
>  
>  static int register_balloon(struct sys_device *sysdev);
>  
> -static struct xenbus_watch target_watch =
> -{
> -	.node = "memory/target"
> -};
> -
>  /* React to a change in the target key */
>  static void watch_target(struct xenbus_watch *watch,
>  			 const char **vec, unsigned int len)
> @@ -74,6 +69,11 @@ static void watch_target(struct xenbus_w
>  	balloon_set_new_target(new_target >> (PAGE_SHIFT - 10));
>  }
>  
> +static struct xenbus_watch target_watch = {
> +	.node = "memory/target",
> +	.callback = watch_target,
> +};
> +
>  static int balloon_init_watcher(struct notifier_block *notifier,
>  				unsigned long event,
>  				void *data)
> @@ -87,7 +87,9 @@ static int balloon_init_watcher(struct n
>  	return NOTIFY_DONE;
>  }
>  
> -static struct notifier_block xenstore_notifier;
> +static struct notifier_block xenstore_notifier = {
> +	.notifier_call = balloon_init_watcher,
> +};
>  
>  static int __init balloon_init(void)
>  {
> @@ -97,10 +99,6 @@ static int __init balloon_init(void)
>  	pr_info("xen-balloon: Initialising balloon driver.\n");
>  
>  	register_balloon(&balloon_sysdev);
> -
> -	target_watch.callback = watch_target;
> -	xenstore_notifier.notifier_call = balloon_init_watcher;
> -
>  	register_xenstore_notifier(&xenstore_notifier);
>  
>  	return 0;
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

[PATCH 2/6] xen/hvm kexec: unregister shutdown+sysrq watches during reboot

[Olaf Hering]

[-- Attachment #1: xen.syscore_ops.manage.shutdown_event.patch --]
[-- Type: text/plain, Size: 1156 bytes --]

Unregister the shutdown and sysrq watch during kexec.  The watches can
not be re-registered in the kexec kernel because they are still seen as
busy by xenstore.

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

---
 drivers/xen/manage.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

Index: linux-3.0/drivers/xen/manage.c
===================================================================
--- linux-3.0.orig/drivers/xen/manage.c
+++ linux-3.0/drivers/xen/manage.c
@@ -320,6 +320,18 @@ static int shutdown_event(struct notifie
 	return NOTIFY_DONE;
 }
 
+static void xenbus_disable_shutdown_watcher(void)
+{
+	unregister_xenbus_watch(&shutdown_watch);
+#ifdef CONFIG_MAGIC_SYSRQ
+	unregister_xenbus_watch(&sysrq_watch);
+#endif
+}
+
+static struct syscore_ops xenbus_watcher_syscore_ops = {
+	.shutdown = xenbus_disable_shutdown_watcher,
+};
+
 int xen_setup_shutdown_event(void)
 {
 	static struct notifier_block xenstore_notifier = {
@@ -329,6 +341,7 @@ int xen_setup_shutdown_event(void)
 	if (!xen_domain())
 		return -ENODEV;
 	register_xenstore_notifier(&xenstore_notifier);
+	register_syscore_ops(&xenbus_watcher_syscore_ops);
 
 	return 0;
 }

Re: [PATCH 2/6] xen/hvm kexec: unregister shutdown+sysrq watches during reboot

[Konrad Rzeszutek Wilk]

On Tue, Jul 26, 2011 at 01:52:11PM +0200, Olaf Hering wrote:
> Unregister the shutdown and sysrq watch during kexec.  The watches can
> not be re-registered in the kexec kernel because they are still seen as
> busy by xenstore.

So this is the PV or HVM guest doing the kexec?

> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> 
> ---
>  drivers/xen/manage.c |   13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> Index: linux-3.0/drivers/xen/manage.c
> ===================================================================
> --- linux-3.0.orig/drivers/xen/manage.c
> +++ linux-3.0/drivers/xen/manage.c
> @@ -320,6 +320,18 @@ static int shutdown_event(struct notifie
>  	return NOTIFY_DONE;
>  }
>  
> +static void xenbus_disable_shutdown_watcher(void)
> +{
> +	unregister_xenbus_watch(&shutdown_watch);
> +#ifdef CONFIG_MAGIC_SYSRQ
> +	unregister_xenbus_watch(&sysrq_watch);
> +#endif
> +}
> +
> +static struct syscore_ops xenbus_watcher_syscore_ops = {
> +	.shutdown = xenbus_disable_shutdown_watcher,
> +};
> +
>  int xen_setup_shutdown_event(void)
>  {
>  	static struct notifier_block xenstore_notifier = {
> @@ -329,6 +341,7 @@ int xen_setup_shutdown_event(void)
>  	if (!xen_domain())
>  		return -ENODEV;
>  	register_xenstore_notifier(&xenstore_notifier);
> +	register_syscore_ops(&xenbus_watcher_syscore_ops);
>  
>  	return 0;
>  }
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH 2/6] xen/hvm kexec: unregister shutdown+sysrq watches during reboot

[Olaf Hering]

On Tue, Jul 26, Konrad Rzeszutek Wilk wrote:

> On Tue, Jul 26, 2011 at 01:52:11PM +0200, Olaf Hering wrote:
> > Unregister the shutdown and sysrq watch during kexec.  The watches can
> > not be re-registered in the kexec kernel because they are still seen as
> > busy by xenstore.
> 
> So this is the PV or HVM guest doing the kexec?

Its for HVM guests with PV drivers.

Olaf

[PATCH 3/6] xen/hvm kexec: unregister memory/target watch in xen-balloon.c

[Olaf Hering]

[-- Attachment #1: xen.syscore_ops.xen-balloon.unregister_watch.patch --]
[-- Type: text/plain, Size: 1270 bytes --]

Unregister the memory/target watch during kexec. The watche can not be
re-registered in the kexec kernel because it is still seen as busy by
xenstore.

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

---
 drivers/xen/xen-balloon.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

Index: linux-3.0/drivers/xen/xen-balloon.c
===================================================================
--- linux-3.0.orig/drivers/xen/xen-balloon.c
+++ linux-3.0/drivers/xen/xen-balloon.c
@@ -34,6 +34,7 @@
 #include <linux/module.h>
 #include <linux/sysdev.h>
 #include <linux/capability.h>
+#include <linux/syscore_ops.h>
 
 #include <xen/xen.h>
 #include <xen/interface/xen.h>
@@ -91,6 +92,15 @@ static struct notifier_block xenstore_no
 	.notifier_call = balloon_init_watcher,
 };
 
+static void xen_balloon_shutdown_watcher(void)
+{
+	unregister_xenbus_watch(&target_watch);
+}
+
+static struct syscore_ops xen_balloon_watcher_syscore_ops = {
+	.shutdown = xen_balloon_shutdown_watcher,
+};
+
 static int __init balloon_init(void)
 {
 	if (!xen_domain())
@@ -100,6 +110,7 @@ static int __init balloon_init(void)
 
 	register_balloon(&balloon_sysdev);
 	register_xenstore_notifier(&xenstore_notifier);
+	register_syscore_ops(&xen_balloon_watcher_syscore_ops);
 
 	return 0;
 }

Re: [PATCH 3/6] xen/hvm kexec: unregister memory/target watch in xen-balloon.c

[Konrad Rzeszutek Wilk]

On Tue, Jul 26, 2011 at 01:52:12PM +0200, Olaf Hering wrote:
> Unregister the memory/target watch during kexec. The watche can not be

watche? watcher I think?

> re-registered in the kexec kernel because it is still seen as busy by
> xenstore.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> 
> ---
>  drivers/xen/xen-balloon.c |   11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> Index: linux-3.0/drivers/xen/xen-balloon.c
> ===================================================================
> --- linux-3.0.orig/drivers/xen/xen-balloon.c
> +++ linux-3.0/drivers/xen/xen-balloon.c
> @@ -34,6 +34,7 @@
>  #include <linux/module.h>
>  #include <linux/sysdev.h>
>  #include <linux/capability.h>
> +#include <linux/syscore_ops.h>
>  
>  #include <xen/xen.h>
>  #include <xen/interface/xen.h>
> @@ -91,6 +92,15 @@ static struct notifier_block xenstore_no
>  	.notifier_call = balloon_init_watcher,
>  };
>  
> +static void xen_balloon_shutdown_watcher(void)
> +{
> +	unregister_xenbus_watch(&target_watch);
> +}
> +
> +static struct syscore_ops xen_balloon_watcher_syscore_ops = {
> +	.shutdown = xen_balloon_shutdown_watcher,
> +};
> +
>  static int __init balloon_init(void)
>  {
>  	if (!xen_domain())
> @@ -100,6 +110,7 @@ static int __init balloon_init(void)
>  
>  	register_balloon(&balloon_sysdev);
>  	register_xenstore_notifier(&xenstore_notifier);
> +	register_syscore_ops(&xen_balloon_watcher_syscore_ops);
>  
>  	return 0;
>  }
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

[PATCH 4/6] xen/hvm kexec: unbind debugirq during reboot

[Olaf Hering]

[-- Attachment #1: xen.syscore_ops.smp.unbind-xen_debug_irq.patch --]
[-- Type: text/plain, Size: 1408 bytes --]

Unregister the debugirq during kexec, otherwise the kexec kernel can not
bind to the still registered virq.

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

---
 arch/x86/xen/smp.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Index: linux-3.0/arch/x86/xen/smp.c
===================================================================
--- linux-3.0.orig/arch/x86/xen/smp.c
+++ linux-3.0/arch/x86/xen/smp.c
@@ -16,6 +16,7 @@
 #include <linux/err.h>
 #include <linux/slab.h>
 #include <linux/smp.h>
+#include <linux/syscore_ops.h>
 
 #include <asm/paravirt.h>
 #include <asm/desc.h>
@@ -45,6 +46,21 @@ static DEFINE_PER_CPU(int, xen_debug_irq
 static irqreturn_t xen_call_function_interrupt(int irq, void *dev_id);
 static irqreturn_t xen_call_function_single_interrupt(int irq, void *dev_id);
 
+static void xen_hvn_smp_shutdown(void)
+{
+	int cpu;
+	for_each_online_cpu(cpu) {
+		if (per_cpu(xen_debug_irq, cpu) < 0)
+			continue;
+		unbind_from_irqhandler(per_cpu(xen_debug_irq, cpu), NULL);
+		per_cpu(xen_debug_irq, cpu) = -1;
+	}
+}
+
+static struct syscore_ops xen_hvn_smp_syscore_ops = {
+	.shutdown = xen_hvn_smp_shutdown,
+};
+
 /*
  * Reschedule call back.
  */
@@ -525,6 +541,7 @@ static void __init xen_hvm_smp_prepare_c
 		return;
 	xen_init_lock_cpu(0);
 	xen_init_spinlocks();
+	register_syscore_ops(&xen_hvn_smp_syscore_ops);
 }
 
 static int __cpuinit xen_hvm_cpu_up(unsigned int cpu)

Re: [PATCH 4/6] xen/hvm kexec: unbind debugirq during reboot

[Konrad Rzeszutek Wilk]

On Tue, Jul 26, 2011 at 01:52:13PM +0200, Olaf Hering wrote:
> Unregister the debugirq during kexec, otherwise the kexec kernel can not
> bind to the still registered virq.

What about the other ones? Say spinlock and the timer ones?

> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> 
> ---
>  arch/x86/xen/smp.c |   17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> Index: linux-3.0/arch/x86/xen/smp.c
> ===================================================================
> --- linux-3.0.orig/arch/x86/xen/smp.c
> +++ linux-3.0/arch/x86/xen/smp.c
> @@ -16,6 +16,7 @@
>  #include <linux/err.h>
>  #include <linux/slab.h>
>  #include <linux/smp.h>
> +#include <linux/syscore_ops.h>
>  
>  #include <asm/paravirt.h>
>  #include <asm/desc.h>
> @@ -45,6 +46,21 @@ static DEFINE_PER_CPU(int, xen_debug_irq
>  static irqreturn_t xen_call_function_interrupt(int irq, void *dev_id);
>  static irqreturn_t xen_call_function_single_interrupt(int irq, void *dev_id);
>  
> +static void xen_hvn_smp_shutdown(void)
> +{
> +	int cpu;
> +	for_each_online_cpu(cpu) {
> +		if (per_cpu(xen_debug_irq, cpu) < 0)
> +			continue;
> +		unbind_from_irqhandler(per_cpu(xen_debug_irq, cpu), NULL);
> +		per_cpu(xen_debug_irq, cpu) = -1;
> +	}
> +}
> +
> +static struct syscore_ops xen_hvn_smp_syscore_ops = {
> +	.shutdown = xen_hvn_smp_shutdown,
> +};
> +
>  /*
>   * Reschedule call back.
>   */
> @@ -525,6 +541,7 @@ static void __init xen_hvm_smp_prepare_c
>  		return;
>  	xen_init_lock_cpu(0);
>  	xen_init_spinlocks();
> +	register_syscore_ops(&xen_hvn_smp_syscore_ops);
>  }
>  
>  static int __cpuinit xen_hvm_cpu_up(unsigned int cpu)
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

[PATCH 5/6] xen/hvm kexec: unregister timer interrupt during reboot

[Olaf Hering]

[-- Attachment #1: xen.syscore_ops.time.unbind_timer-irq.patch --]
[-- Type: text/plain, Size: 1883 bytes --]

The kexec kernel will crash because the timer interrupt is already
registerd with EVTCHNOP_bind_virq.  Unbind the event channel during
shutdown so that the kexec kernel can reregister the interrupt.

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

---
 arch/x86/xen/time.c |   27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

Index: linux-3.0/arch/x86/xen/time.c
===================================================================
--- linux-3.0.orig/arch/x86/xen/time.c
+++ linux-3.0/arch/x86/xen/time.c
@@ -14,6 +14,7 @@
 #include <linux/kernel_stat.h>
 #include <linux/math64.h>
 #include <linux/gfp.h>
+#include <linux/syscore_ops.h>
 
 #include <asm/pvclock.h>
 #include <asm/xen/hypervisor.h>
@@ -405,12 +406,20 @@ void xen_setup_timer(int cpu)
 	evt->irq = irq;
 }
 
-void xen_teardown_timer(int cpu)
+static void xen_unbind_timer(int cpu)
 {
 	struct clock_event_device *evt;
-	BUG_ON(cpu == 0);
 	evt = &per_cpu(xen_clock_events, cpu);
-	unbind_from_irqhandler(evt->irq, NULL);
+	if (evt->irq >= 0) {
+		unbind_from_irqhandler(evt->irq, NULL);
+		evt->irq = -1;
+	}
+}
+
+void xen_teardown_timer(int cpu)
+{
+	BUG_ON(cpu == 0);
+	xen_unbind_timer(cpu);
 }
 
 void xen_setup_cpu_clockevents(void)
@@ -478,6 +487,17 @@ void __init xen_init_time_ops(void)
 }
 
 #ifdef CONFIG_XEN_PVHVM
+static void xen_hvmtimer_shutdown(void)
+{
+	int cpu;
+	for_each_online_cpu(cpu)
+		xen_unbind_timer(cpu);
+}
+
+static struct syscore_ops xen_hvmtimer_syscore_ops = {
+	.shutdown = xen_hvmtimer_shutdown,
+};
+
 static void xen_hvm_setup_cpu_clockevents(void)
 {
 	int cpu = smp_processor_id();
@@ -506,5 +526,6 @@ void __init xen_hvm_init_time_ops(void)
 	x86_platform.calibrate_tsc = xen_tsc_khz;
 	x86_platform.get_wallclock = xen_get_wallclock;
 	x86_platform.set_wallclock = xen_set_wallclock;
+	register_syscore_ops(&xen_hvmtimer_syscore_ops);
 }
 #endif

Re: [PATCH 5/6] xen/hvm kexec: unregister timer interrupt during reboot

[Konrad Rzeszutek Wilk]

On Tue, Jul 26, 2011 at 01:52:14PM +0200, Olaf Hering wrote:
> The kexec kernel will crash because the timer interrupt is already
> registerd with EVTCHNOP_bind_virq.  Unbind the event channel during
> shutdown so that the kexec kernel can reregister the interrupt.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> 
> ---
>  arch/x86/xen/time.c |   27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
> Index: linux-3.0/arch/x86/xen/time.c
> ===================================================================
> --- linux-3.0.orig/arch/x86/xen/time.c
> +++ linux-3.0/arch/x86/xen/time.c
> @@ -14,6 +14,7 @@
>  #include <linux/kernel_stat.h>
>  #include <linux/math64.h>
>  #include <linux/gfp.h>
> +#include <linux/syscore_ops.h>
>  
>  #include <asm/pvclock.h>
>  #include <asm/xen/hypervisor.h>
> @@ -405,12 +406,20 @@ void xen_setup_timer(int cpu)
>  	evt->irq = irq;
>  }
>  
> -void xen_teardown_timer(int cpu)
> +static void xen_unbind_timer(int cpu)
>  {
>  	struct clock_event_device *evt;
> -	BUG_ON(cpu == 0);
>  	evt = &per_cpu(xen_clock_events, cpu);
> -	unbind_from_irqhandler(evt->irq, NULL);
> +	if (evt->irq >= 0) {
> +		unbind_from_irqhandler(evt->irq, NULL);
> +		evt->irq = -1;
> +	}
> +}
> +
> +void xen_teardown_timer(int cpu)
> +{
> +	BUG_ON(cpu == 0);

Why the BUG? Ah you just copied it from xen_unbind_timer.
Hm, I wonder if we actually need that BUG_ON.

> +	xen_unbind_timer(cpu);
>  }
>  
>  void xen_setup_cpu_clockevents(void)
> @@ -478,6 +487,17 @@ void __init xen_init_time_ops(void)
>  }
>  
>  #ifdef CONFIG_XEN_PVHVM
> +static void xen_hvmtimer_shutdown(void)
> +{
> +	int cpu;
> +	for_each_online_cpu(cpu)
> +		xen_unbind_timer(cpu);
> +}
> +
> +static struct syscore_ops xen_hvmtimer_syscore_ops = {
> +	.shutdown = xen_hvmtimer_shutdown,
> +};
> +
>  static void xen_hvm_setup_cpu_clockevents(void)
>  {
>  	int cpu = smp_processor_id();
> @@ -506,5 +526,6 @@ void __init xen_hvm_init_time_ops(void)
>  	x86_platform.calibrate_tsc = xen_tsc_khz;
>  	x86_platform.get_wallclock = xen_get_wallclock;
>  	x86_platform.set_wallclock = xen_set_wallclock;
> +	register_syscore_ops(&xen_hvmtimer_syscore_ops);
>  }
>  #endif
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH 5/6] xen/hvm kexec: unregister timer interrupt during reboot

[Olaf Hering]

On Tue, Jul 26, Konrad Rzeszutek Wilk wrote:

> On Tue, Jul 26, 2011 at 01:52:14PM +0200, Olaf Hering wrote:
> > The kexec kernel will crash because the timer interrupt is already
> > registerd with EVTCHNOP_bind_virq.  Unbind the event channel during
> > shutdown so that the kexec kernel can reregister the interrupt.
> > 
> > Signed-off-by: Olaf Hering <olaf@aepfle.de>
> > 
> > ---
> >  arch/x86/xen/time.c |   27 ++++++++++++++++++++++++---
> >  1 file changed, 24 insertions(+), 3 deletions(-)
> > 
> > Index: linux-3.0/arch/x86/xen/time.c
> > ===================================================================
> > --- linux-3.0.orig/arch/x86/xen/time.c
> > +++ linux-3.0/arch/x86/xen/time.c
> > @@ -14,6 +14,7 @@
> >  #include <linux/kernel_stat.h>
> >  #include <linux/math64.h>
> >  #include <linux/gfp.h>
> > +#include <linux/syscore_ops.h>
> >  
> >  #include <asm/pvclock.h>
> >  #include <asm/xen/hypervisor.h>
> > @@ -405,12 +406,20 @@ void xen_setup_timer(int cpu)
> >  	evt->irq = irq;
> >  }
> >  
> > -void xen_teardown_timer(int cpu)
> > +static void xen_unbind_timer(int cpu)
> >  {
> >  	struct clock_event_device *evt;
> > -	BUG_ON(cpu == 0);
> >  	evt = &per_cpu(xen_clock_events, cpu);
> > -	unbind_from_irqhandler(evt->irq, NULL);
> > +	if (evt->irq >= 0) {
> > +		unbind_from_irqhandler(evt->irq, NULL);
> > +		evt->irq = -1;
> > +	}
> > +}
> > +
> > +void xen_teardown_timer(int cpu)
> > +{
> > +	BUG_ON(cpu == 0);
> 
> Why the BUG? Ah you just copied it from xen_unbind_timer.
> Hm, I wonder if we actually need that BUG_ON.

A quick grep did not show up the place where
/sys/devices/system/cpu/cpu0/online would have been created. Since that
file is missing in my guest I think cpu0 can not be shutdown, So that
BUG_ON() can probably go.

Olaf

Re: [PATCH 5/6] xen/hvm kexec: unregister timer interrupt during reboot

[Konrad Rzeszutek Wilk]

On Wed, Jul 27, 2011 at 04:05:12PM +0200, Olaf Hering wrote:
> On Tue, Jul 26, Konrad Rzeszutek Wilk wrote:
> 
> > On Tue, Jul 26, 2011 at 01:52:14PM +0200, Olaf Hering wrote:
> > > The kexec kernel will crash because the timer interrupt is already
> > > registerd with EVTCHNOP_bind_virq.  Unbind the event channel during
> > > shutdown so that the kexec kernel can reregister the interrupt.
> > > 
> > > Signed-off-by: Olaf Hering <olaf@aepfle.de>
> > > 
> > > ---
> > >  arch/x86/xen/time.c |   27 ++++++++++++++++++++++++---
> > >  1 file changed, 24 insertions(+), 3 deletions(-)
> > > 
> > > Index: linux-3.0/arch/x86/xen/time.c
> > > ===================================================================
> > > --- linux-3.0.orig/arch/x86/xen/time.c
> > > +++ linux-3.0/arch/x86/xen/time.c
> > > @@ -14,6 +14,7 @@
> > >  #include <linux/kernel_stat.h>
> > >  #include <linux/math64.h>
> > >  #include <linux/gfp.h>
> > > +#include <linux/syscore_ops.h>
> > >  
> > >  #include <asm/pvclock.h>
> > >  #include <asm/xen/hypervisor.h>
> > > @@ -405,12 +406,20 @@ void xen_setup_timer(int cpu)
> > >  	evt->irq = irq;
> > >  }
> > >  
> > > -void xen_teardown_timer(int cpu)
> > > +static void xen_unbind_timer(int cpu)
> > >  {
> > >  	struct clock_event_device *evt;
> > > -	BUG_ON(cpu == 0);
> > >  	evt = &per_cpu(xen_clock_events, cpu);
> > > -	unbind_from_irqhandler(evt->irq, NULL);
> > > +	if (evt->irq >= 0) {
> > > +		unbind_from_irqhandler(evt->irq, NULL);
> > > +		evt->irq = -1;
> > > +	}
> > > +}
> > > +
> > > +void xen_teardown_timer(int cpu)
> > > +{
> > > +	BUG_ON(cpu == 0);
> > 
> > Why the BUG? Ah you just copied it from xen_unbind_timer.
> > Hm, I wonder if we actually need that BUG_ON.
> 
> A quick grep did not show up the place where
> /sys/devices/system/cpu/cpu0/online would have been created. Since that
> file is missing in my guest I think cpu0 can not be shutdown, So that
> BUG_ON() can probably go.

ok, Thanks for looking around for that. If you can just send that
BUG_ON() remove check as a seperate cleanup patch it would be much appreciated.
(either before this patch or after it).

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

[PATCH 6/6] xen kexec: reset device state to Initializing during reboot

[Olaf Hering]

[-- Attachment #1: xen.xenbus_dev_shutdown.XenbusStateInitialising.patch --]
[-- Type: text/plain, Size: 1827 bytes --]

During kexec all devices will be shutdown, the backend drivers enter the
Closed state. But in this state the kexec kernel can not connect to the
backend because it expects the devices in InitWait state.
After triggering the Closing event, trigger also the Initializing event
and wait until the backend has changed its state. Without this waiting
the kexec kernel may find a device where a state change is still in
progress.

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

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
@@ -192,8 +192,19 @@ void xenbus_otherend_changed(struct xenb
 	 * work that can fail e.g., when the rootfs is gone.
 	 */
 	if (system_state > SYSTEM_RUNNING) {
-		if (ignore_on_shutdown && (state == XenbusStateClosing))
-			xenbus_frontend_closed(dev);
+		if (ignore_on_shutdown) {
+			switch (state) {
+			case XenbusStateClosing:
+				xenbus_frontend_closed(dev);
+				break;
+			case XenbusStateInitialising:
+			case XenbusStateInitWait:
+				complete(&dev->down);
+				break;
+			default:
+				break;
+			}
+		}
 		return;
 	}
 
@@ -284,6 +295,14 @@ void xenbus_dev_shutdown(struct device *
 	if (!timeout)
 		printk(KERN_INFO "%s: %s timeout closing device\n",
 		       __func__, dev->nodename);
+
+	if (system_state > SYSTEM_RUNNING) {
+		xenbus_switch_state(dev, XenbusStateInitialising);
+		timeout = wait_for_completion_timeout(&dev->down, timeout);
+		if (!timeout)
+			printk(KERN_INFO "%s: %s timeout initializing device\n",
+			       __func__, dev->nodename);
+	}
  out:
 	put_device(&dev->dev);
 }

Re: [PATCH 6/6] xen kexec: reset device state to Initializing during reboot

[Konrad Rzeszutek Wilk]

On Tue, Jul 26, 2011 at 01:52:15PM +0200, Olaf Hering wrote:
> During kexec all devices will be shutdown, the backend drivers enter the
                                ^^ - get rid of that.

> Closed state. But in this state the kexec kernel can not connect to the
> backend because it expects the devices in InitWait state.
                                        ^- add "to be"
> After triggering the Closing event, trigger also the Initializing event
> and wait until the backend has changed its state. Without this waiting

> the kexec kernel may find a device where a state change is still in
> progress.

Uhh, say that again? Are you saying that after moving to Initializing state
we should allow the allow the state changes to proceed as they would do under
normal circumstances?

> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
>  drivers/xen/xenbus/xenbus_probe.c |   23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> 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
> @@ -192,8 +192,19 @@ void xenbus_otherend_changed(struct xenb
>  	 * work that can fail e.g., when the rootfs is gone.
>  	 */
>  	if (system_state > SYSTEM_RUNNING) {
> -		if (ignore_on_shutdown && (state == XenbusStateClosing))
> -			xenbus_frontend_closed(dev);
> +		if (ignore_on_shutdown) {
> +			switch (state) {
> +			case XenbusStateClosing:
> +				xenbus_frontend_closed(dev);
> +				break;
> +			case XenbusStateInitialising:
> +			case XenbusStateInitWait:
> +				complete(&dev->down);
> +				break;
> +			default:
> +				break;
> +			}
> +		}
>  		return;
>  	}
>  
> @@ -284,6 +295,14 @@ void xenbus_dev_shutdown(struct device *
>  	if (!timeout)
>  		printk(KERN_INFO "%s: %s timeout closing device\n",
>  		       __func__, dev->nodename);
> +
> +	if (system_state > SYSTEM_RUNNING) {
> +		xenbus_switch_state(dev, XenbusStateInitialising);
> +		timeout = wait_for_completion_timeout(&dev->down, timeout);
> +		if (!timeout)
> +			printk(KERN_INFO "%s: %s timeout initializing device\n",
> +			       __func__, dev->nodename);
> +	}
>   out:
>  	put_device(&dev->dev);
>  }
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH 6/6] xen kexec: reset device state to Initializing during reboot

[Stefano Stabellini]

On Tue, 26 Jul 2011, Olaf Hering wrote:
> During kexec all devices will be shutdown, the backend drivers enter the
> Closed state. But in this state the kexec kernel can not connect to the
> backend because it expects the devices in InitWait state.
> After triggering the Closing event, trigger also the Initializing event
> and wait until the backend has changed its state. Without this waiting
> the kexec kernel may find a device where a state change is still in
> progress.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
>  drivers/xen/xenbus/xenbus_probe.c |   23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> 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
> @@ -192,8 +192,19 @@ void xenbus_otherend_changed(struct xenb
>  	 * work that can fail e.g., when the rootfs is gone.
>  	 */
>  	if (system_state > SYSTEM_RUNNING) {
> -		if (ignore_on_shutdown && (state == XenbusStateClosing))
> -			xenbus_frontend_closed(dev);
> +		if (ignore_on_shutdown) {
> +			switch (state) {
> +			case XenbusStateClosing:
> +				xenbus_frontend_closed(dev);
> +				break;
> +			case XenbusStateInitialising:
> +			case XenbusStateInitWait:
> +				complete(&dev->down);
> +				break;
> +			default:
> +				break;
> +			}
> +		}
>  		return;
>  	}
>  
> @@ -284,6 +295,14 @@ void xenbus_dev_shutdown(struct device *
>  	if (!timeout)
>  		printk(KERN_INFO "%s: %s timeout closing device\n",
>  		       __func__, dev->nodename);
> +
> +	if (system_state > SYSTEM_RUNNING) {
> +		xenbus_switch_state(dev, XenbusStateInitialising);
> +		timeout = wait_for_completion_timeout(&dev->down, timeout);
> +		if (!timeout)
> +			printk(KERN_INFO "%s: %s timeout initializing device\n",
> +			       __func__, dev->nodename);
> +	}
>   out:
>  	put_device(&dev->dev);
>  }

It looks like this code path is going to be triggered every time we shut
down a PV on HVM guest. I think we should avoid going into
XenbusStateInitialising on normal shut down or reboot.

Re: [PATCH 6/6] xen kexec: reset device state to Initializing during reboot

[Olaf Hering]

On Wed, Jul 27, Stefano Stabellini wrote:

> It looks like this code path is going to be triggered every time we shut
> down a PV on HVM guest. I think we should avoid going into
> XenbusStateInitialising on normal shut down or reboot.

There is currently no way to know wether its a reboot via kexec.
That would be a different patch.

Olaf

Re: [PATCH 6/6] xen kexec: reset device state to Initializing during reboot

[Stefano Stabellini]

On Wed, 27 Jul 2011, Olaf Hering wrote:
> On Wed, Jul 27, Stefano Stabellini wrote:
> 
> > It looks like this code path is going to be triggered every time we shut
> > down a PV on HVM guest. I think we should avoid going into
> > XenbusStateInitialising on normal shut down or reboot.
> 
> There is currently no way to know wether its a reboot via kexec.
> That would be a different patch.

Then we need a different patch :)

Re: [PATCH 2/6] xen/hvm kexec: unregister shutdown+sysrq watches during reboot

[Jan Beulich]

[-- Attachment #1.1: Type: text/plain, Size: 1965 bytes --]

>>> On 26.07.11 at 13:52, Olaf Hering <olaf@aepfle.de> wrote:
> Unregister the shutdown and sysrq watch during kexec.  The watches can
> not be re-registered in the kexec kernel because they are still seen as
> busy by xenstore.

This and subsequent patches don't look right to me from a conceptual
pov: If the kexec attempt is due to a crash, the dying kernel should be
doing as little as possible, and the new kernel should really do the
cleanup. The more logic gets added to the shutdown path of the old
kernel, the more likely it'll become that the kexec attempt will fail.

If this requires changes outside the kernel (e.g. state reset helpers
in hypervisor or tools) - so be it.

Jan

> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> 
> ---
>  drivers/xen/manage.c |   13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> Index: linux-3.0/drivers/xen/manage.c
> ===================================================================
> --- linux-3.0.orig/drivers/xen/manage.c
> +++ linux-3.0/drivers/xen/manage.c
> @@ -320,6 +320,18 @@ static int shutdown_event(struct notifie
>  return NOTIFY_DONE;
>  }
>  
> +static void xenbus_disable_shutdown_watcher(void)
> +{
> +unregister_xenbus_watch(&shutdown_watch);
> +#ifdef CONFIG_MAGIC_SYSRQ
> +unregister_xenbus_watch(&sysrq_watch);
> +#endif
> +}
> +
> +static struct syscore_ops xenbus_watcher_syscore_ops = {
> +.shutdown = xenbus_disable_shutdown_watcher,
> +};
> +
>  int xen_setup_shutdown_event(void)
>  {
>  static struct notifier_block xenstore_notifier = {
> @@ -329,6 +341,7 @@ int xen_setup_shutdown_event(void)
>  if (!xen_domain())
>  return -ENODEV;
>  register_xenstore_notifier(&xenstore_notifier);
> +register_syscore_ops(&xenbus_watcher_syscore_ops);
>  
>  return 0;
>  }
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com 
> http://lists.xensource.com/xen-devel 



[-- Attachment #1.2: HTML --]
[-- Type: text/html, Size: 3447 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH 2/6] xen/hvm kexec: unregister shutdown+sysrq watches during reboot

[Olaf Hering]

On Thu, Jul 28, Jan Beulich wrote:

> >>> On 26.07.11 at 13:52, Olaf Hering <olaf@aepfle.de> wrote:
> > Unregister the shutdown and sysrq watch during kexec.  The watches can
> > not be re-registered in the kexec kernel because they are still seen as
> > busy by xenstore.
> 
> This and subsequent patches don't look right to me from a conceptual
> pov: If the kexec attempt is due to a crash, the dying kernel should be
> doing as little as possible, and the new kernel should really do the
> cleanup. The more logic gets added to the shutdown path of the old
> kernel, the more likely it'll become that the kexec attempt will fail.

kexec is about reboot, kdump is about crash handling. Both use different
code paths.
The kexec code path is like a reboot without going through the firmware.
The kdump kernel runs in its own memory range, so memory corruption does
not appear to happen (with the sles11sp1 kernel + my kdump patch).

> 
> If this requires changes outside the kernel (e.g. state reset helpers
> in hypervisor or tools) - so be it.

Are you suggesting that there have to be ways for a domU to query the
state of its registered watches and shut them all down during very early
boot? And what about the event/irq handling? There is currently no way
to check what virq is bound to what port, other than looping through all
possible ports and see if one matches the requested virq.

Olaf

Re: [PATCH 2/6] xen/hvm kexec: unregister shutdown+sysrq watches during reboot

[Ian Campbell]

On Thu, 2011-07-28 at 01:25 -0400, Olaf Hering wrote:
> On Thu, Jul 28, Jan Beulich wrote:
> 
> > >>> On 26.07.11 at 13:52, Olaf Hering <olaf@aepfle.de> wrote:
> > > Unregister the shutdown and sysrq watch during kexec.  The watches can
> > > not be re-registered in the kexec kernel because they are still seen as
> > > busy by xenstore.
> > 
> > This and subsequent patches don't look right to me from a conceptual
> > pov: If the kexec attempt is due to a crash, the dying kernel should be
> > doing as little as possible, and the new kernel should really do the
> > cleanup. The more logic gets added to the shutdown path of the old
> > kernel, the more likely it'll become that the kexec attempt will fail.
> 
> kexec is about reboot, kdump is about crash handling. Both use different
> code paths.
> The kexec code path is like a reboot without going through the firmware.
> The kdump kernel runs in its own memory range, so memory corruption does
> not appear to happen (with the sles11sp1 kernel + my kdump patch).

Getting into the kdump kernel is a kexec like operation though and
shares many of the code paths, doesn't it?

> > If this requires changes outside the kernel (e.g. state reset helpers
> > in hypervisor or tools) - so be it.
> 
> Are you suggesting that there have to be ways for a domU to query the
> state of its registered watches and shut them all down during very early
> boot?

Perhaps the xenstore protocol could be enhanced with a mechanism to
clear all existing watches? A kernel could call that at start of day.

>  And what about the event/irq handling? There is currently no way
> to check what virq is bound to what port, other than looping through all
> possible ports and see if one matches the requested virq.

There is EVTCHNOP_reset but I'm not sure if it does too much. For
example I'm not sure how the guest could recover event channels which
are setup by the domain builder -- such as the xenstore event channel.

IIRC EVTCHNOP_reset was added to aid with kexec though -- so I must be
missing something.

Ian.

Re: [PATCH 2/6] xen/hvm kexec: unregister shutdown+sysrq watches during reboot

[Keir Fraser]

On 28/07/2011 11:52, "Ian Campbell" <Ian.Campbell@citrix.com> wrote:

>>  And what about the event/irq handling? There is currently no way
>> to check what virq is bound to what port, other than looping through all
>> possible ports and see if one matches the requested virq.
> 
> There is EVTCHNOP_reset but I'm not sure if it does too much. For
> example I'm not sure how the guest could recover event channels which
> are setup by the domain builder -- such as the xenstore event channel.

EVTCHNOP_reset is currently only called from the toolstack, which is then
able to re-setup things like the xenstore event channel for the guest.

 -- Keir

Re: [PATCH 2/6] xen/hvm kexec: unregister shutdown+sysrq watches during reboot

[Ian Campbell]

On Thu, 2011-07-28 at 07:00 -0400, Keir Fraser wrote:
> On 28/07/2011 11:52, "Ian Campbell" <Ian.Campbell@citrix.com> wrote:
> 
> >>  And what about the event/irq handling? There is currently no way
> >> to check what virq is bound to what port, other than looping through all
> >> possible ports and see if one matches the requested virq.
> > 
> > There is EVTCHNOP_reset but I'm not sure if it does too much. For
> > example I'm not sure how the guest could recover event channels which
> > are setup by the domain builder -- such as the xenstore event channel.
> 
> EVTCHNOP_reset is currently only called from the toolstack, which is then
> able to re-setup things like the xenstore event channel for the guest.

Oh, right. And toolstack isn't involved in kexec so that's not terribly
helpful.

Ian.

Re: [PATCH 2/6] xen/hvm kexec: unregister shutdown+sysrq watches during reboot

[Olaf Hering]

On Thu, Jul 28, Ian Campbell wrote:

> Getting into the kdump kernel is a kexec like operation though and
> shares many of the code paths, doesn't it?

The big difference is that kdump is entered in an unreliable state,
while kexec is a controlled reboot.

> > > If this requires changes outside the kernel (e.g. state reset helpers
> > > in hypervisor or tools) - so be it.
> > 
> > Are you suggesting that there have to be ways for a domU to query the
> > state of its registered watches and shut them all down during very early
> > boot?
> 
> Perhaps the xenstore protocol could be enhanced with a mechanism to
> clear all existing watches? A kernel could call that at start of day.

I wonder why xenstore knows that sysrq and shutdown nodes are busy,
while the device, backend and state files can be watched twice.

Olaf

Re: [PATCH 2/6] xen/hvm kexec: unregister shutdown+sysrq watches during reboot

[Ian Campbell]

On Thu, 2011-07-28 at 07:02 -0400, Olaf Hering wrote:
> On Thu, Jul 28, Ian Campbell wrote:
> 
> > Getting into the kdump kernel is a kexec like operation though and
> > shares many of the code paths, doesn't it?
> 
> The big difference is that kdump is entered in an unreliable state,
> while kexec is a controlled reboot.

Sure, but by handling the former you also solve the later, while the
opposite is not true.

> > > > If this requires changes outside the kernel (e.g. state reset helpers
> > > > in hypervisor or tools) - so be it.
> > > 
> > > Are you suggesting that there have to be ways for a domU to query the
> > > state of its registered watches and shut them all down during very early
> > > boot?
> > 
> > Perhaps the xenstore protocol could be enhanced with a mechanism to
> > clear all existing watches? A kernel could call that at start of day.
> 
> I wonder why xenstore knows that sysrq and shutdown nodes are busy,
> while the device, backend and state files can be watched twice.

Do the precise path's watched differ subtly?

Oh, I see, xenstored only calls a watch a duplicate if both the path
_and_ the token match. In the case of backend paths the token is a
reference to the dynamically allocated per-device data structure. In the
sysrq and shutdown case the token is a static global variable -- I
suppose you are kexec'ing an identical kernel? I expect that if you
kexec'd a subtly different kernel where these datastructures ended up at
a different address you wouldn't get the EEXIST.

Ian.

Re: [PATCH 2/6] xen/hvm kexec: unregister shutdown+sysrq watches during reboot

[Olaf Hering]

On Thu, Jul 28, Ian Campbell wrote:

> > Are you suggesting that there have to be ways for a domU to query the
> > state of its registered watches and shut them all down during very early
> > boot?
> 
> Perhaps the xenstore protocol could be enhanced with a mechanism to
> clear all existing watches? A kernel could call that at start of day.

I poked around in the xenstore sources, there is appearently nothing
that can be used to shutdown all. Or would calling XS_RELEASE do the trick?

Olaf

Re: [PATCH 2/6] xen/hvm kexec: unregister shutdown+sysrq watches during reboot

[Keir Fraser]

On 28/07/2011 15:07, "Olaf Hering" <olaf@aepfle.de> wrote:

> On Thu, Jul 28, Ian Campbell wrote:
> 
>>> Are you suggesting that there have to be ways for a domU to query the
>>> state of its registered watches and shut them all down during very early
>>> boot?
>> 
>> Perhaps the xenstore protocol could be enhanced with a mechanism to
>> clear all existing watches? A kernel could call that at start of day.
> 
> I poked around in the xenstore sources, there is appearently nothing
> that can be used to shutdown all. Or would calling XS_RELEASE do the trick?

XS_INTRODUCE

 K.

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

Re: [PATCH 2/6] xen/hvm kexec: unregister shutdown+sysrq watches during reboot

[Olaf Hering]

On Thu, Jul 28, Keir Fraser wrote:

> On 28/07/2011 15:07, "Olaf Hering" <olaf@aepfle.de> wrote:
> 
> > On Thu, Jul 28, Ian Campbell wrote:
> > 
> >>> Are you suggesting that there have to be ways for a domU to query the
> >>> state of its registered watches and shut them all down during very early
> >>> boot?
> >> 
> >> Perhaps the xenstore protocol could be enhanced with a mechanism to
> >> clear all existing watches? A kernel could call that at start of day.
> > 
> > I poked around in the xenstore sources, there is appearently nothing
> > that can be used to shutdown all. Or would calling XS_RELEASE do the trick?
> 
> XS_INTRODUCE

Unfortunately do_introduce() is not preprared for that.

On enter, conn->id contains the domain_id, so it errors out early.
Later it compares domain->conn != conn and does not enter the correct path.

If I remove both checks the kexec appears to work ok.
Is it save to remove both checks?

The only issue I havent figured out:
How to get the domid from within the kernel?

Olaf

diff -r 42edf1481c57 tools/xenstore/xenstored_domain.c
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -326,7 +326,7 @@ void do_introduce(struct connection *con
 		return;
 	}
 
-	if (conn->id != 0 || !conn->can_write) {
+	if (!conn->can_write) {
 		send_error(conn, EACCES);
 		return;
 	}
@@ -365,7 +365,7 @@ void do_introduce(struct connection *con
 		talloc_steal(domain->conn, domain);
 
 		fire_watches(NULL, "@introduceDomain", false);
-	} else if ((domain->mfn == mfn) && (domain->conn != conn)) {
+	} else if (domain->mfn == mfn) {
 		/* Use XS_INTRODUCE for recreating the xenbus event-channel. */
 		if (domain->port)
 			xc_evtchn_unbind(xce_handle, domain->port);

Re: [PATCH 2/6] xen/hvm kexec: unregister shutdown+sysrq watches during reboot

[Keir Fraser]

[-- Attachment #1: Type: text/plain, Size: 2280 bytes --]

On 28/07/2011 20:50, "Olaf Hering" <olaf@aepfle.de> wrote:

> On Thu, Jul 28, Keir Fraser wrote:
> 
>> On 28/07/2011 15:07, "Olaf Hering" <olaf@aepfle.de> wrote:
>> 
>>> On Thu, Jul 28, Ian Campbell wrote:
>>> 
>>>>> Are you suggesting that there have to be ways for a domU to query the
>>>>> state of its registered watches and shut them all down during very early
>>>>> boot?
>>>> 
>>>> Perhaps the xenstore protocol could be enhanced with a mechanism to
>>>> clear all existing watches? A kernel could call that at start of day.
>>> 
>>> I poked around in the xenstore sources, there is appearently nothing
>>> that can be used to shutdown all. Or would calling XS_RELEASE do the trick?
>> 
>> XS_INTRODUCE
> 
> Unfortunately do_introduce() is not preprared for that.
> 
> On enter, conn->id contains the domain_id, so it errors out early.
> Later it compares domain->conn != conn and does not enter the correct path.

Oh of course you want to do it from *inside* the guest...

> If I remove both checks the kexec appears to work ok.
> Is it save to remove both checks?

Hmm, no. Attached patch would be safer, give it a try.

And note that it is *dangerous* to reset the domain xenstore connection if
there could be any other concurrent activity on the connection that could
confuse the guest kernel. So doing the reset during kernel bringup might be
safest, there you can do it in the kernel before the whole xenbus subsystem
is fully up.

 -- Keir

> The only issue I havent figured out:
> How to get the domid from within the kernel?
> 
> Olaf
> 
> diff -r 42edf1481c57 tools/xenstore/xenstored_domain.c
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -326,7 +326,7 @@ void do_introduce(struct connection *con
> return;
> }
>  
> - if (conn->id != 0 || !conn->can_write) {
> + if (!conn->can_write) {
> send_error(conn, EACCES);
> return;
> }
> @@ -365,7 +365,7 @@ void do_introduce(struct connection *con
> talloc_steal(domain->conn, domain);
>  
> fire_watches(NULL, "@introduceDomain", false);
> - } else if ((domain->mfn == mfn) && (domain->conn != conn)) {
> + } else if (domain->mfn == mfn) {
> /* Use XS_INTRODUCE for recreating the xenbus event-channel. */
> if (domain->port)
> xc_evtchn_unbind(xce_handle, domain->port);


[-- Attachment #2: 00-xenstore-introduce --]
[-- Type: application/octet-stream, Size: 890 bytes --]

diff -r 0f36c2eec2e1 tools/xenstore/xenstored_domain.c
--- a/tools/xenstore/xenstored_domain.c	Thu Jul 28 15:40:54 2011 +0100
+++ b/tools/xenstore/xenstored_domain.c	Thu Jul 28 21:26:49 2011 +0100
@@ -326,7 +326,7 @@ void do_introduce(struct connection *con
 		return;
 	}
 
-	if (conn->id != 0 || !conn->can_write) {
+	if (!conn->can_write) {
 		send_error(conn, EACCES);
 		return;
 	}
@@ -343,7 +343,16 @@ void do_introduce(struct connection *con
 
 	domain = find_domain_by_domid(domid);
 
-	if (domain == NULL) {
+	if (conn->id != 0) {
+		if ((domain == NULL) || (domain->conn != conn)) {
+			send_error(conn, EACCES);
+			return;
+		}
+		if ((domain->mfn != mfn) || (domain->remote_port != port)) {
+			send_error(conn, EINVAL);
+			return;
+		}
+	} else if (domain == NULL) {
 		interface = xc_map_foreign_range(
 			*xc_handle, domid,
 			getpagesize(), PROT_READ|PROT_WRITE, mfn);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH 2/6] xen/hvm kexec: unregister shutdown+sysrq watches during reboot

[Olaf Hering]

On Thu, Jul 28, Keir Fraser wrote:

> > Unfortunately do_introduce() is not preprared for that.
> > 
> > On enter, conn->id contains the domain_id, so it errors out early.
> > Later it compares domain->conn != conn and does not enter the correct path.
> 
> Oh of course you want to do it from *inside* the guest...
> 
> > If I remove both checks the kexec appears to work ok.
> > Is it save to remove both checks?
> 
> Hmm, no. Attached patch would be safer, give it a try.
> 
> And note that it is *dangerous* to reset the domain xenstore connection if
> there could be any other concurrent activity on the connection that could
> confuse the guest kernel. So doing the reset during kernel bringup might be
> safest, there you can do it in the kernel before the whole xenbus subsystem
> is fully up.

I sent a different xenstored patch which takes DOMID_SELF into account.

Olaf

Re: [PATCH 2/6] xen/hvm kexec: unregister shutdown+sysrq watches during reboot

[Keir Fraser]

On 01/08/2011 06:01, "Olaf Hering" <olaf@aepfle.de> wrote:

>> Hmm, no. Attached patch would be safer, give it a try.
>> 
>> And note that it is *dangerous* to reset the domain xenstore connection if
>> there could be any other concurrent activity on the connection that could
>> confuse the guest kernel. So doing the reset during kernel bringup might be
>> safest, there you can do it in the kernel before the whole xenbus subsystem
>> is fully up.
> 
> I sent a different xenstored patch which takes DOMID_SELF into account.

Saw it, I'll give it a think over. I think it looks okay, I wonder whether
DOMID_SELF handling should be pushed into find_domain_by_domid, but it's a
minor detail really. I certainly support the approach. Hopefully we can get
someone to similarly modify ocaml/xenstored too.

 -- Keir

Re: [PATCH 2/6] xen/hvm kexec: unregister shutdown+sysrq watches during reboot

[Jan Beulich]

>>> On 28.07.11 at 07:25, Olaf Hering <olaf@aepfle.de> wrote:
> On Thu, Jul 28, Jan Beulich wrote:
> 
>> >>> On 26.07.11 at 13:52, Olaf Hering <olaf@aepfle.de> wrote:
>> > Unregister the shutdown and sysrq watch during kexec.  The watches can
>> > not be re-registered in the kexec kernel because they are still seen as
>> > busy by xenstore.
>> 
>> This and subsequent patches don't look right to me from a conceptual
>> pov: If the kexec attempt is due to a crash, the dying kernel should be
>> doing as little as possible, and the new kernel should really do the
>> cleanup. The more logic gets added to the shutdown path of the old
>> kernel, the more likely it'll become that the kexec attempt will fail.
> 
> kexec is about reboot, kdump is about crash handling. Both use different
> code paths.
> The kexec code path is like a reboot without going through the firmware.

Oh, I implied it would be kdump.

But then again xenstore state shouldn't matter wrt the purpose of
the secondary kernel.

> The kdump kernel runs in its own memory range, so memory corruption does
> not appear to happen (with the sles11sp1 kernel + my kdump patch).

It's also not clear to me what corruption there is - this would seem to
imply that there should be information on certain addresses that were
used in the old kernel to get passed to the new kernel, or get used to
access guest memory from outside the guest. All of which sounds
wrong.

Jan

>> If this requires changes outside the kernel (e.g. state reset helpers
>> in hypervisor or tools) - so be it.
> 
> Are you suggesting that there have to be ways for a domU to query the
> state of its registered watches and shut them all down during very early
> boot? And what about the event/irq handling? There is currently no way
> to check what virq is bound to what port, other than looping through all
> possible ports and see if one matches the requested virq.
> 
> Olaf

Re: [PATCH 2/6] xen/hvm kexec: unregister shutdown+sysrq watches during reboot

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 761 bytes --]

On Thu, 28 Jul 2011, Jan Beulich wrote:
> >>> On 26.07.11 at 13:52, Olaf Hering <olaf@aepfle.de> wrote:
> > Unregister the shutdown and sysrq watch during kexec.  The watches can
> > not be re-registered in the kexec kernel because they are still seen as
> > busy by xenstore.
> 
> This and subsequent patches don't look right to me from a conceptual
> pov: If the kexec attempt is due to a crash, the dying kernel should be
> doing as little as possible, and the new kernel should really do the
> cleanup. The more logic gets added to the shutdown path of the old
> kernel, the more likely it'll become that the kexec attempt will fail.
> 
> If this requires changes outside the kernel (e.g. state reset helpers
> in hypervisor or tools) - so be it.

I agree.

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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