Patchwork [3/3] qdev: convert watchdogs

login
register
mail settings
Submitter Markus Armbruster
Date Aug. 21, 2009, 8:31 a.m.
Message ID <1250843494-28326-4-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/31811/
State Superseded
Headers show

Comments

Markus Armbruster - Aug. 21, 2009, 8:31 a.m.
-watchdog NAME is now equivalent to -device NAME, except it treats
option argument '?' specially, and supports only one watchdog.

A side effect is that a device created with -watchdog may now receive
a different PCI address.

i6300esb is now available on any machine with a PCI bus, not just PCs.
ib700 is still PC only, but that could be changed easily.

The only remaining use of struct WatchdogTimerModel and
watchdog_add_model() is supporting '-watchdog ?'.  Should be replaced
by searching device_info_list for watchdog devices when we can
identify them there.

Also fixes ib700 not to use vm_clock before it is initialized: in
wdt_ib700_init(), called from register_watchdogs(), which runs before
init_timers().  The bug made ib700_write_enable_reg() crash in
qemu_del_timer().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 Makefile.target   |    6 +++---
 hw/pc.c           |    2 --
 hw/watchdog.c     |   26 ++++++--------------------
 hw/watchdog.h     |   11 -----------
 hw/wdt_i6300esb.c |   29 ++++++++++++++---------------
 hw/wdt_ib700.c    |   18 ++++++++++++------
 vl.c              |   18 +++++++++++++-----
 7 files changed, 48 insertions(+), 62 deletions(-)
Gerd Hoffmann - Aug. 21, 2009, 1:07 p.m.
> +static void wdt_ib700_init(ISADevice *dev)
>   {
> +    timer = qemu_new_timer(vm_clock, ib700_timer_expired, NULL);
>       register_savevm("ib700_wdt", -1, 0, ib700_save, ib700_load, NULL);
> -
>       register_ioport_write(0x441, 2, 1, ib700_write_disable_reg, NULL);
>       register_ioport_write(0x443, 2, 1, ib700_write_enable_reg, NULL);
>   }

One minor nit:  Setting dev->iobase[] to { 0x441, 0x443 } here would be 
nice as 'info qtree' will show the ports actually used by the device then.

Otherwise the patch series looks good.

cheers,
   Gerd
Markus Armbruster - Aug. 24, 2009, 9:05 a.m.
Gerd Hoffmann <kraxel@redhat.com> writes:

>> +static void wdt_ib700_init(ISADevice *dev)
>>   {
>> +    timer = qemu_new_timer(vm_clock, ib700_timer_expired, NULL);
>>       register_savevm("ib700_wdt", -1, 0, ib700_save, ib700_load, NULL);
>> -
>>       register_ioport_write(0x441, 2, 1, ib700_write_disable_reg, NULL);
>>       register_ioport_write(0x443, 2, 1, ib700_write_enable_reg, NULL);
>>   }
>
> One minor nit:  Setting dev->iobase[] to { 0x441, 0x443 } here would
> be nice as 'info qtree' will show the ports actually used by the
> device then.

Can do.

iobase[] will start as whatever the user specifies in -device,
defaulting to { -1, -1 }, then revert to { 0x441, 0x443 } when the
device is initialized.  Not sure whether this is a problem.

Taking a step back, the general problem is "immutable" qdev properties,
i.e. properties that aren't configurable.  I think it would be good to
agree on a common method there, and document it.

Your suggestion for this particular case is to have device
initialization overwrite whatever was configured.  Good enough for the
general case?

> Otherwise the patch series looks good.

Thanks for the review!
Gerd Hoffmann - Aug. 24, 2009, 12:25 p.m.
On 08/24/09 11:05, Markus Armbruster wrote:

> iobase[] will start as whatever the user specifies in -device,
> defaulting to { -1, -1 }, then revert to { 0x441, 0x443 } when the
> device is initialized.  Not sure whether this is a problem.

The ioport properties should reflect the values actually used, even in 
case they can not be configured.  So in case a ISA device finds '-1' aka 
'not specified by the user', it should fill in (and use) the default 
value.  See also below ...

> Taking a step back, the general problem is "immutable" qdev properties,
> i.e. properties that aren't configurable.  I think it would be good to
> agree on a common method there, and document it.

On a even broader view: how to handle invalid property values?

Having a device support one fixed I/O port is just a special case of 
that.  Other ISA devices usually can be configured to a few possible I/O 
bases using dip switches (sound cards for example).  But the possible 
values are usually fairly limited.

In case the user specifies a impossible iobase we can

   (1) ignore it and use defaults instead.
   (2) fix it (pick nearest possible value or so).
   (3) fail.

#3 is probably the most sane, but that depends on the 
init()-callback-can-fail patches ...

cheers,
   Gerd
Markus Armbruster - Aug. 24, 2009, 1:04 p.m.
Gerd Hoffmann <kraxel@redhat.com> writes:

> On 08/24/09 11:05, Markus Armbruster wrote:
>
>> iobase[] will start as whatever the user specifies in -device,
>> defaulting to { -1, -1 }, then revert to { 0x441, 0x443 } when the
>> device is initialized.  Not sure whether this is a problem.
>
> The ioport properties should reflect the values actually used, even in
> case they can not be configured.  So in case a ISA device finds '-1'
> aka 'not specified by the user', it should fill in (and use) the
> default value.  See also below ...

Works for me.

>> Taking a step back, the general problem is "immutable" qdev properties,
>> i.e. properties that aren't configurable.  I think it would be good to
>> agree on a common method there, and document it.
>
> On a even broader view: how to handle invalid property values?

Good point.

> Having a device support one fixed I/O port is just a special case of
> that.  Other ISA devices usually can be configured to a few possible
> I/O bases using dip switches (sound cards for example).  But the
> possible values are usually fairly limited.
>
> In case the user specifies a impossible iobase we can
>
>   (1) ignore it and use defaults instead.
>   (2) fix it (pick nearest possible value or so).
>   (3) fail.
>
> #3 is probably the most sane,

Agree.

>                               but that depends on the
> init()-callback-can-fail patches ...

Yes.

Patch

diff --git a/Makefile.target b/Makefile.target
index 066af8d..a3492b9 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -184,15 +184,15 @@  obj-y += pcnet.o
 obj-y += rtl8139.o
 obj-y += e1000.o
 
-# Generic watchdog support and some watchdog devices
-obj-y += wdt_ib700.o wdt_i6300esb.o
+# PCI watchdog devices
+obj-y += wdt_i6300esb.o
 
 # Hardware support
 obj-i386-y = ide.o pckbd.o vga.o $(sound-obj-y) dma.o isa-bus.o
 obj-i386-y += fdc.o mc146818rtc.o serial.o i8259.o i8254.o pcspk.o pc.o
 obj-i386-y += cirrus_vga.o apic.o ioapic.o parallel.o acpi.o piix_pci.o
 obj-i386-y += usb-uhci.o vmmouse.o vmport.o vmware_vga.o hpet.o
-obj-i386-y += device-hotplug.o pci-hotplug.o smbios.o
+obj-i386-y += device-hotplug.o pci-hotplug.o smbios.o wdt_ib700.o
 
 # shared objects
 obj-ppc-y = ppc.o ide.o vga.o $(sound-obj-y) dma.o isa-bus.o openpic.o
diff --git a/hw/pc.c b/hw/pc.c
index cc6e7e8..167ff0e 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -1331,8 +1331,6 @@  static void pc_init1(ram_addr_t ram_size,
         }
     }
 
-    watchdog_pc_init(pci_bus);
-
     for(i = 0; i < nb_nics; i++) {
         NICInfo *nd = &nd_table[i];
 
diff --git a/hw/watchdog.c b/hw/watchdog.c
index 359c318..adba872 100644
--- a/hw/watchdog.c
+++ b/hw/watchdog.c
@@ -20,6 +20,8 @@ 
  */
 
 #include "qemu-common.h"
+#include "qemu-option.h"
+#include "qemu-config.h"
 #include "sys-queue.h"
 #include "sysemu.h"
 #include "hw/watchdog.h"
@@ -32,7 +34,6 @@ 
 #define WDT_DEBUG        5	/* Prints a message and continues running. */
 #define WDT_NONE         6	/* Do nothing. */
 
-static WatchdogTimerModel *watchdog;
 static int watchdog_action = WDT_RESET;
 static LIST_HEAD(watchdog_list, WatchdogTimerModel) watchdog_list;
 
@@ -49,12 +50,7 @@  void watchdog_add_model(WatchdogTimerModel *model)
 int select_watchdog(const char *p)
 {
     WatchdogTimerModel *model;
-
-    if (watchdog) {
-        fprintf(stderr,
-                 "qemu: only one watchdog option may be given\n");
-        return 1;
-    }
+    QemuOpts *opts;
 
     /* -watchdog ? lists available devices and exits cleanly. */
     if (strcmp(p, "?") == 0) {
@@ -67,7 +63,9 @@  int select_watchdog(const char *p)
 
     LIST_FOREACH(model, &watchdog_list, entry) {
         if (strcasecmp(model->wdt_name, p) == 0) {
-            watchdog = model;
+            /* add the device */
+            opts = qemu_opts_create(&qemu_device_opts, NULL, 0);
+            qemu_opt_set(opts, "driver", p);
             return 0;
         }
     }
@@ -130,15 +128,3 @@  void watchdog_perform_action(void)
         break;
     }
 }
-
-void watchdog_pc_init(PCIBus *pci_bus)
-{
-    if (watchdog)
-        watchdog->wdt_pc_init(pci_bus);
-}
-
-void register_watchdogs(void)
-{
-    wdt_ib700_init();
-    wdt_i6300esb_init();
-}
diff --git a/hw/watchdog.h b/hw/watchdog.h
index bb81204..8c54fa4 100644
--- a/hw/watchdog.h
+++ b/hw/watchdog.h
@@ -22,10 +22,6 @@ 
 #ifndef QEMU_WATCHDOG_H
 #define QEMU_WATCHDOG_H
 
-extern void wdt_i6300esb_init(void);
-extern void wdt_ib700_init(void);
-
-
 struct WatchdogTimerModel {
     LIST_ENTRY(WatchdogTimerModel) entry;
 
@@ -33,11 +29,6 @@  struct WatchdogTimerModel {
     const char *wdt_name;
     /* Longer description (eg. manufacturer and full model number). */
     const char *wdt_description;
-
-    /* This callback should create/register the device.  It is called
-     * indirectly from hw/pc.c when the virtual PC is being set up.
-     */
-    void (*wdt_pc_init)(PCIBus *pci_bus);
 };
 typedef struct WatchdogTimerModel WatchdogTimerModel;
 
@@ -46,7 +37,5 @@  extern int select_watchdog(const char *p);
 extern int select_watchdog_action(const char *action);
 extern void watchdog_add_model(WatchdogTimerModel *model);
 extern void watchdog_perform_action(void);
-extern void watchdog_pc_init(PCIBus *pci_bus);
-extern void register_watchdogs(void);
 
 #endif /* QEMU_WATCHDOG_H */
diff --git a/hw/wdt_i6300esb.c b/hw/wdt_i6300esb.c
index 2227303..2318738 100644
--- a/hw/wdt_i6300esb.c
+++ b/hw/wdt_i6300esb.c
@@ -413,22 +413,11 @@  static int i6300esb_load(QEMUFile *f, void *vp, int version)
     return 0;
 }
 
-/* Create and initialize a virtual Intel 6300ESB during PC creation. */
-static void i6300esb_pc_init(PCIBus *pci_bus)
+static void i6300esb_init(PCIDevice *dev)
 {
-    I6300State *d;
+    I6300State *d = container_of(dev, I6300State, dev);
     uint8_t *pci_conf;
 
-    if (!pci_bus) {
-        fprintf(stderr, "wdt_i6300esb: no PCI bus in this machine\n");
-        return;
-    }
-
-    d = (I6300State *)
-        pci_register_device (pci_bus, "i6300esb_wdt", sizeof (I6300State),
-                             -1,
-                             i6300esb_config_read, i6300esb_config_write);
-
     d->reboot_enabled = 1;
     d->clock_scale = CLOCK_SCALE_1KHZ;
     d->int_type = INT_TYPE_IRQ;
@@ -458,10 +447,20 @@  static void i6300esb_pc_init(PCIBus *pci_bus)
 static WatchdogTimerModel model = {
     .wdt_name = "i6300esb",
     .wdt_description = "Intel 6300ESB",
-    .wdt_pc_init = i6300esb_pc_init,
 };
 
-void wdt_i6300esb_init(void)
+static PCIDeviceInfo i6300esb_info = {
+    .qdev.name    = "i6300esb",
+    .qdev.size    = sizeof(I6300State),
+    .config_read  = i6300esb_config_read,
+    .config_write = i6300esb_config_write,
+    .init         = i6300esb_init,
+};
+
+static void i6300esb_register_devices(void)
 {
     watchdog_add_model(&model);
+    pci_qdev_register(&i6300esb_info);
 }
+
+device_init(i6300esb_register_devices);
diff --git a/hw/wdt_ib700.c b/hw/wdt_ib700.c
index 7b54bde..5fc3d83 100644
--- a/hw/wdt_ib700.c
+++ b/hw/wdt_ib700.c
@@ -88,11 +88,10 @@  static int ib700_load(QEMUFile *f, void *vp, int version)
     return 0;
 }
 
-/* Create and initialize a virtual IB700 during PC creation. */
-static void ib700_pc_init(PCIBus *unused)
+static void wdt_ib700_init(ISADevice *dev)
 {
+    timer = qemu_new_timer(vm_clock, ib700_timer_expired, NULL);
     register_savevm("ib700_wdt", -1, 0, ib700_save, ib700_load, NULL);
-
     register_ioport_write(0x441, 2, 1, ib700_write_disable_reg, NULL);
     register_ioport_write(0x443, 2, 1, ib700_write_enable_reg, NULL);
 }
@@ -100,11 +99,18 @@  static void ib700_pc_init(PCIBus *unused)
 static WatchdogTimerModel model = {
     .wdt_name = "ib700",
     .wdt_description = "iBASE 700",
-    .wdt_pc_init = ib700_pc_init,
 };
 
-void wdt_ib700_init(void)
+static ISADeviceInfo wdt_ib700_info = {
+    .qdev.name = "ib700",
+    .qdev.size = sizeof(ISADevice),
+    .init      = wdt_ib700_init,
+};
+
+static void wdt_ib700_register_devices(void)
 {
     watchdog_add_model(&model);
-    timer = qemu_new_timer(vm_clock, ib700_timer_expired, NULL);
+    isa_qdev_register(&wdt_ib700_info);
 }
+
+device_init(wdt_ib700_register_devices);
diff --git a/vl.c b/vl.c
index 9b73ea3..1e2e0e6 100644
--- a/vl.c
+++ b/vl.c
@@ -233,6 +233,7 @@  int graphic_rotate = 0;
 #ifndef _WIN32
 int daemonize = 0;
 #endif
+const char *watchdog;
 const char *option_rom[MAX_OPTION_ROMS];
 int nb_option_roms;
 int semihosting_enabled = 0;
@@ -4884,8 +4885,6 @@  int main(int argc, char **argv, char **envp)
     tb_size = 0;
     autostart= 1;
 
-    register_watchdogs();
-
     optind = 1;
     for(;;) {
         if (optind >= argc)
@@ -5301,9 +5300,12 @@  int main(int argc, char **argv, char **envp)
                 serial_device_index++;
                 break;
             case QEMU_OPTION_watchdog:
-                i = select_watchdog(optarg);
-                if (i > 0)
-                    exit (i == 1 ? 1 : 0);
+                if (watchdog) {
+                    fprintf(stderr,
+                            "qemu: only one watchdog option may be given\n");
+                    return 1;
+                }
+                watchdog = optarg;
                 break;
             case QEMU_OPTION_watchdog_action:
                 if (select_watchdog_action(optarg) == -1) {
@@ -5911,6 +5913,12 @@  int main(int argc, char **argv, char **envp)
 
     module_call_init(MODULE_INIT_DEVICE);
 
+    if (watchdog) {
+        i = select_watchdog(watchdog);
+        if (i > 0)
+            exit (i == 1 ? 1 : 0);
+    }
+
     if (machine->compat_props) {
         qdev_prop_register_compat(machine->compat_props);
     }