Re: [PATCH] xm reboot/shutdown/sysrq to HVM domain

[Tetsu Yamamoto <yamamoto.tetsu@jp.fujitsu.com>]

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

Hi Steven,

Thank you for your comments.

Steven Smith wrote:
>> This patch enhances 'xm reboot'/'xm shutdown' commands to
>> reboot/shutdown guest Linux on HVM domain as gracefully as para-Linux.
>> In addtion, sysrq key signal can be sent to HVM domain by 'xm sysrq'
>> command.
> Thanks, that's really useful.  I have a couple of comments about the
> patch, though:
>
> -- It looks like you had some problems with ctrl_alt_del(), and instead
>    used kill_proc(cad_pid, SIGINT, 1).  What was the reason for this?

The symbol ctrl_alt_del() can't be found when it is used in loadable
module.  On build, the warning message is shown that ctrl_alt_del() is
undefined, and on loading, the error message is shown that it is unknown
symbol.  I'm not sure why this happens, but I tried kill_proc(), which
is called in ctrl_alt_del(), it works correctly.

> -- You've introduced a lot of #ifdefs into reboot.c.  It might be
>    easier to just split the file in two; did you look at this at all?

reboot.c has common process to deal with reboot/shutdown/sysrq for
para-linux (built in kernel) and full-linux (loadable module), so I
think that it would be better to be one file in consideration of code
maintenance.

> -- You set reboot_module from within a xenbus transaction.  I don't
>    think that's necessary, since xenbus_writes are supposed to be
>    atomic anyway.

The reason why I use xenbus_write is that I could not find other
interface to write xenstore through xenbus module for HVM.  I'm not sure
which interface you suggest to use, but for example, xb_write() is not
exported, so it can not be called from reboot module.  If I should use
other interface, please let me know.

> -- Because of the way mkbuildtree works, you're going to create
>    symlinks from unmodified-drivers to all of the files in
>    linux-2.6-xen-sparse/drivers/core, rather than just to reboot.c.
>    It's a trivial aesthetic issue, but it'd be nice not to create lots
>    of useless symlinks.

The modified patch is attached.

Regards,

Tetsu Yamamoto

Signed-off-by: Tetsu Yamamoto <yamamoto.tetsu@jp.fujitsu.com>


>
> Apart from that, it looks pretty reasonable.
>
> Steven.



[-- Attachment #2: reboot.patch --]
[-- Type: text/plain, Size: 7461 bytes --]

diff -r 38f9bd7a4ce6 linux-2.6-xen-sparse/drivers/xen/core/reboot.c
--- a/linux-2.6-xen-sparse/drivers/xen/core/reboot.c	Tue Oct 03 11:39:22 2006 +0100
+++ b/linux-2.6-xen-sparse/drivers/xen/core/reboot.c	Tue Oct 10 15:06:27 2006 +0900
@@ -19,7 +19,13 @@
 #include <xen/xencons.h>
 #include <xen/cpu_hotplug.h>
 
+#ifdef CONFIG_XEN
 extern void ctrl_alt_del(void);
+#endif /* CONFIG_XEN */
+
+#ifndef CONFIG_XEN
+MODULE_LICENSE("Dual BSD/GPL");
+#endif /* !CONFIG_XEN */
 
 #define SHUTDOWN_INVALID  -1
 #define SHUTDOWN_POWEROFF  0
@@ -31,6 +37,7 @@ extern void ctrl_alt_del(void);
  */
 #define SHUTDOWN_HALT      4
 
+#ifdef CONFIG_XEN
 #if defined(__i386__) || defined(__x86_64__)
 
 /*
@@ -71,6 +78,7 @@ EXPORT_SYMBOL(machine_power_off);
 EXPORT_SYMBOL(machine_power_off);
 
 #endif /* defined(__i386__) || defined(__x86_64__) */
+#endif /* CONFIG_XEN */
 
 /******************************************************************************
  * Stop/pickle callback handling.
@@ -81,6 +89,7 @@ static void __shutdown_handler(void *unu
 static void __shutdown_handler(void *unused);
 static DECLARE_WORK(shutdown_work, __shutdown_handler, NULL);
 
+#ifdef CONFIG_XEN
 #if defined(__i386__) || defined(__x86_64__)
 
 /* Ensure we run on the idle task page tables so that we will
@@ -210,6 +219,7 @@ static int __do_suspend(void *ignore)
 
 	return err;
 }
+#endif /* CONFIG_XEN */
 
 static int shutdown_process(void *__unused)
 {
@@ -222,12 +232,17 @@ static int shutdown_process(void *__unus
 
 	if ((shutting_down == SHUTDOWN_POWEROFF) ||
 	    (shutting_down == SHUTDOWN_HALT)) {
+#ifdef CONFIG_XEN
 		if (execve("/sbin/poweroff", poweroff_argv, envp) < 0) {
 			sys_reboot(LINUX_REBOOT_MAGIC1,
 				   LINUX_REBOOT_MAGIC2,
 				   LINUX_REBOOT_CMD_POWER_OFF,
 				   NULL);
 		}
+#else /* !CONFIG_XEN */
+		call_usermodehelper_keys("/sbin/poweroff", poweroff_argv, envp, NULL, 0);
+
+#endif /* !CONFIG_XEN */
 	}
 
 	shutting_down = SHUTDOWN_INVALID; /* could try again */
@@ -235,6 +250,7 @@ static int shutdown_process(void *__unus
 	return 0;
 }
 
+#ifdef CONFIG_XEN
 static int kthread_create_on_cpu(int (*f)(void *arg),
 				 void *arg,
 				 const char *name,
@@ -248,17 +264,24 @@ static int kthread_create_on_cpu(int (*f
 	wake_up_process(p);
 	return 0;
 }
+#endif /* CONFIG_XEN */
 
 static void __shutdown_handler(void *unused)
 {
 	int err;
 
+#ifdef CONFIG_XEN
 	if (shutting_down != SHUTDOWN_SUSPEND)
 		err = kernel_thread(shutdown_process, NULL,
 				    CLONE_FS | CLONE_FILES);
 	else
 		err = kthread_create_on_cpu(__do_suspend, NULL, "suspend", 0);
 
+#else /* !CONFIG_XEN */
+	err = kernel_thread(shutdown_process, NULL,
+			    CLONE_FS | CLONE_FILES);
+#endif /* !CONFIG_XEN */
+
 	if (err < 0) {
 		printk(KERN_WARNING "Error creating shutdown process (%d): "
 		       "retrying...\n", -err);
@@ -272,6 +295,9 @@ static void shutdown_handler(struct xenb
 	char *str;
 	struct xenbus_transaction xbt;
 	int err;
+#ifndef CONFIG_XEN
+	int cad_pid = 1; 
+#endif /* !CONFIG_XEN */
 
 	if (shutting_down != SHUTDOWN_INVALID)
 		return;
@@ -298,7 +324,11 @@ static void shutdown_handler(struct xenb
 	if (strcmp(str, "poweroff") == 0)
 		shutting_down = SHUTDOWN_POWEROFF;
 	else if (strcmp(str, "reboot") == 0)
+#ifdef CONFIG_XEN  
 		ctrl_alt_del();
+#else /* !CONFIG_XEN */
+		kill_proc(cad_pid, SIGINT, 1);
+#endif /* !CONFIG_XEN */
 	else if (strcmp(str, "suspend") == 0)
 		shutting_down = SHUTDOWN_SUSPEND;
 	else if (strcmp(str, "halt") == 0)
@@ -374,10 +404,27 @@ static int setup_shutdown_watcher(struct
 
 static int __init setup_shutdown_event(void)
 {
+#ifndef CONFIG_XEN
+	int err;
+	struct xenbus_transaction xbt;
+#endif /* !CONFIG_XEN */
+
 	static struct notifier_block xenstore_notifier = {
 		.notifier_call = setup_shutdown_watcher
 	};
 	register_xenstore_notifier(&xenstore_notifier);
+#ifndef CONFIG_XEN
+ again:
+	err = xenbus_transaction_start(&xbt);
+	if (err)
+		return -1;
+	xenbus_write(xbt, "control", "reboot_module", "installed");
+
+	err = xenbus_transaction_end(xbt, 0);
+	if (err == -EAGAIN) {
+		goto again;
+	}
+#endif /* !CONFIG_XEN */
 	return 0;
 }
 
diff -r 38f9bd7a4ce6 tools/python/xen/xend/image.py
--- a/tools/python/xen/xend/image.py	Tue Oct 03 11:39:22 2006 +0100
+++ b/tools/python/xen/xend/image.py	Tue Oct 10 15:06:27 2006 +0900
@@ -281,6 +281,7 @@ class HVMImageHandler(ImageHandler):
         log.debug("apic           = %d", self.apic)
 
         self.register_shutdown_watch()
+        self.register_reboot_module_watch()
 
         return xc.hvm_build(dom            = self.vm.getDomid(),
                             image          = self.kernel,
@@ -383,6 +384,7 @@ class HVMImageHandler(ImageHandler):
 
     def destroy(self):
         self.unregister_shutdown_watch();
+        self.unregister_reboot_module_watch();
         import signal
         if not self.pid:
             return
@@ -425,6 +427,39 @@ class HVMImageHandler(ImageHandler):
                 vm.refreshShutdown(vm.info)
 
         return 1 # Keep watching
+
+    def register_reboot_module_watch(self):
+        """ add xen store watch on control/reboot_module """
+        self.rebootModuleWatch = xswatch(self.vm.dompath + "/control/reboot_module", \
+                                    self.hvm_reboot_module)
+        log.debug("hvm reboot module watch registered")
+
+    def unregister_reboot_module_watch(self):
+        """Remove the watch on the control/reboot_module, if any. Nothrow
+        guarantee."""
+
+        try:
+            if self.rebootModuleWatch:
+                self.rebootModuleWatch.unwatch()
+        except:
+            log.exception("Unwatching hvm reboot module watch failed.")
+        self.rebootModuleWatch = None
+        log.debug("hvm reboot module watch unregistered")
+
+    def hvm_reboot_module(self, _):
+        """ watch call back on node control/reboot_module,
+            if node changed, this function will be called
+        """
+        xd = xen.xend.XendDomain.instance()
+        vm = xd.domain_lookup( self.vm.getDomid() )
+
+        reboot_module_status = vm.readDom('control/reboot_module')
+        log.debug("hvm_reboot_module fired, module status=%s", reboot_module_status)
+        if reboot_module_status == 'installed':
+            self.unregister_shutdown_watch()
+
+        return 1 # Keep watching
+
 
 class IA64_HVM_ImageHandler(HVMImageHandler):
 
diff -r 38f9bd7a4ce6 unmodified_drivers/linux-2.6/Makefile
--- a/unmodified_drivers/linux-2.6/Makefile	Tue Oct 03 11:39:22 2006 +0100
+++ b/unmodified_drivers/linux-2.6/Makefile	Tue Oct 10 15:06:27 2006 +0900
@@ -4,3 +4,4 @@ obj-m += xenbus/
 obj-m += xenbus/
 obj-m += blkfront/
 obj-m += netfront/
+obj-m += util/
diff -r 38f9bd7a4ce6 unmodified_drivers/linux-2.6/mkbuildtree
--- a/unmodified_drivers/linux-2.6/mkbuildtree	Tue Oct 03 11:39:22 2006 +0100
+++ b/unmodified_drivers/linux-2.6/mkbuildtree	Tue Oct 10 15:06:27 2006 +0900
@@ -14,6 +14,7 @@ ln -sf ${XL}/drivers/xen/core/gnttab.c p
 ln -sf ${XL}/drivers/xen/core/gnttab.c platform-pci
 ln -sf ${XL}/drivers/xen/core/features.c platform-pci
 ln -sf ${XL}/drivers/xen/core/xen_proc.c xenbus
+ln -sf ${XL}/drivers/xen/core/reboot.c util
 
 mkdir -p include
 mkdir -p include/xen
diff -r 38f9bd7a4ce6 unmodified_drivers/linux-2.6/util/Kbuild
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/unmodified_drivers/linux-2.6/util/Kbuild	Tue Oct 10 15:11:29 2006 +0900
@@ -0,0 +1,3 @@
+include $(M)/overrides.mk
+
+obj-m := reboot.o

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

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