Patchwork [REPOST] Watchdog: disable watchdog timer when hard-rebooting a guest

login
register
mail settings
Submitter Richard W.M. Jones
Date Oct. 5, 2010, 8:11 a.m.
Message ID <20101005081147.GA14199@amd.home.annexia.org>
Download mbox | patch
Permalink /patch/66769/
State New
Headers show

Comments

Richard W.M. Jones - Oct. 5, 2010, 8:11 a.m.
[Repost: I've attached the patch and put the commit message
in the main email this time]

This commit causes the watchdog timer to be reset when a guest is
hard-rebooted.

The failure case previously was as follows:

  (a) guest boots, watchdog is enabled

  (b) guest does a reset eg:
        echo 'b' > /proc/sysrq-trigger
    (note that an ordinary /sbin/reboot wouldn't hit this case
    since as the watchdog daemon is shut down, the daemon would
    properly disable the watchdog device)

  (c) the reboot takes longer than the remaining time on the
    watchdog

  (d) the watchdog therefore fires during the reboot

  (e) probably the VM would just reboot again at this point which
    is pretty benign, but it could depend on the action that the
    user had selected for the watchdog

Now we use the qdev reset function to register a reset handler
which disables the timer.  Note the handler is called _either_
just after init _or_ when the guest reboots.

In the i6300esb case there is a small refactoring of the code so
that the device's internal state is now fully restored to defaults
on a reboot.

Signed-off-by: Richard W.M. Jones <rjones@redhat.com>

Patch

diff --git a/hw/wdt_i6300esb.c b/hw/wdt_i6300esb.c
index 46e1df8..dbd05ba 100644
--- a/hw/wdt_i6300esb.c
+++ b/hw/wdt_i6300esb.c
@@ -102,6 +102,8 @@  struct I6300State {
 
 typedef struct I6300State I6300State;
 
+static void i6300esb_reset(DeviceState *dev);
+
 /* This function is called when the watchdog has either been enabled
  * (hence it starts counting down) or has been keep-alived.
  */
@@ -140,16 +142,6 @@  static void i6300esb_disable_timer(I6300State *d)
     qemu_del_timer(d->timer);
 }
 
-static void i6300esb_reset(I6300State *d)
-{
-    /* XXX We should probably reset other parts of the state here,
-     * but we should also reset our state on general machine reset
-     * too.  For now just disable the timer so it doesn't fire
-     * again after the reboot.
-     */
-    i6300esb_disable_timer(d);
-}
-
 /* This function is called when the watchdog expires.  Note that
  * the hardware has two timers, and so expiry happens in two stages.
  * If d->stage == 1 then we perform the first stage action (usually,
@@ -181,7 +173,7 @@  static void i6300esb_timer_expired(void *vp)
         if (d->reboot_enabled) {
             d->previous_reboot_flag = 1;
             watchdog_perform_action(); /* This reboots, exits, etc */
-            i6300esb_reset(d);
+            i6300esb_reset(&d->dev.qdev);
         }
 
         /* In "free running mode" we start stage 1 again. */
@@ -394,17 +386,9 @@  static int i6300esb_init(PCIDevice *dev)
     I6300State *d = DO_UPCAST(I6300State, dev, dev);
     uint8_t *pci_conf;
 
-    d->reboot_enabled = 1;
-    d->clock_scale = CLOCK_SCALE_1KHZ;
-    d->int_type = INT_TYPE_IRQ;
-    d->free_run = 0;
-    d->locked = 0;
-    d->enabled = 0;
+    i6300esb_debug("watchdog init\n");
+
     d->timer = qemu_new_timer(vm_clock, i6300esb_timer_expired, d);
-    d->timer1_preload = 0xfffff;
-    d->timer2_preload = 0xfffff;
-    d->stage = 1;
-    d->unlock_state = 0;
     d->previous_reboot_flag = 0;
 
     pci_conf = d->dev.config;
@@ -413,11 +397,33 @@  static int i6300esb_init(PCIDevice *dev)
     pci_config_set_class(pci_conf, PCI_CLASS_SYSTEM_OTHER);
 
     pci_register_bar(&d->dev, 0, 0x10,
-                            PCI_BASE_ADDRESS_SPACE_MEMORY, i6300esb_map);
+                     PCI_BASE_ADDRESS_SPACE_MEMORY, i6300esb_map);
 
     return 0;
 }
 
+static void i6300esb_reset(DeviceState *dev)
+{
+    I6300State *d = DO_UPCAST(I6300State, dev.qdev, dev);
+
+    i6300esb_debug("watchdog reset\n");
+
+    /* NB: Don't change d->previous_reboot_flag in this function. */
+
+    d->reboot_enabled = 1;
+    d->clock_scale = CLOCK_SCALE_1KHZ;
+    d->int_type = INT_TYPE_IRQ;
+    d->free_run = 0;
+    d->locked = 0;
+    d->enabled = 0;
+    d->timer1_preload = 0xfffff;
+    d->timer2_preload = 0xfffff;
+    d->stage = 1;
+    d->unlock_state = 0;
+
+    i6300esb_disable_timer(d);
+}
+
 static WatchdogTimerModel model = {
     .wdt_name = "i6300esb",
     .wdt_description = "Intel 6300ESB",
@@ -427,6 +433,7 @@  static PCIDeviceInfo i6300esb_info = {
     .qdev.name    = "i6300esb",
     .qdev.size    = sizeof(I6300State),
     .qdev.vmsd    = &vmstate_i6300esb,
+    .qdev.reset   = i6300esb_reset,
     .config_read  = i6300esb_config_read,
     .config_write = i6300esb_config_write,
     .init         = i6300esb_init,
diff --git a/hw/wdt_ib700.c b/hw/wdt_ib700.c
index c34687b..b6235eb 100644
--- a/hw/wdt_ib700.c
+++ b/hw/wdt_ib700.c
@@ -97,6 +97,8 @@  static int wdt_ib700_init(ISADevice *dev)
 {
     IB700State *s = DO_UPCAST(IB700State, dev, dev);
 
+    ib700_debug("watchdog init\n");
+
     s->timer = qemu_new_timer(vm_clock, ib700_timer_expired, s);
     register_ioport_write(0x441, 2, 1, ib700_write_disable_reg, s);
     register_ioport_write(0x443, 2, 1, ib700_write_enable_reg, s);
@@ -104,16 +106,26 @@  static int wdt_ib700_init(ISADevice *dev)
     return 0;
 }
 
+static void wdt_ib700_reset(DeviceState *dev)
+{
+    IB700State *s = DO_UPCAST(IB700State, dev.qdev, dev);
+
+    ib700_debug("watchdog reset\n");
+
+    qemu_del_timer(s->timer);
+}
+
 static WatchdogTimerModel model = {
     .wdt_name = "ib700",
     .wdt_description = "iBASE 700",
 };
 
 static ISADeviceInfo wdt_ib700_info = {
-    .qdev.name = "ib700",
-    .qdev.size = sizeof(IB700State),
-    .qdev.vmsd = &vmstate_ib700,
-    .init      = wdt_ib700_init,
+    .qdev.name  = "ib700",
+    .qdev.size  = sizeof(IB700State),
+    .qdev.vmsd  = &vmstate_ib700,
+    .qdev.reset = wdt_ib700_reset,
+    .init       = wdt_ib700_init,
 };
 
 static void wdt_ib700_register_devices(void)