[Qemu-devel] [PATCH 0/3] qdev: clean up & convert watchdogs

[Qemu-devel] [PATCH 0/3] qdev: clean up & convert watchdogs

[Markus Armbruster]

This series convers the watchdog devices to qdev.

Markus Armbruster (3):
  Move watchdog, watchdog_action, give them internal linkage
  Clean up upcast from PCIDevice to I6300State
  qdev: convert watchdogs

 Makefile.target   |    6 +++---
 hw/pc.c           |    2 --
 hw/watchdog.c     |   34 +++++++++++++++-------------------
 hw/watchdog.h     |   22 ----------------------
 hw/wdt_i6300esb.c |   38 ++++++++++++++++++--------------------
 hw/wdt_ib700.c    |   18 ++++++++++++------
 vl.c              |   20 +++++++++++++-------
 7 files changed, 61 insertions(+), 79 deletions(-)

[Qemu-devel] [PATCH 1/3] Move watchdog, watchdog_action, give them internal linkage

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/watchdog.c |   10 ++++++++++
 hw/watchdog.h |   11 -----------
 vl.c          |    2 --
 3 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/hw/watchdog.c b/hw/watchdog.c
index fde2f1b..359c318 100644
--- a/hw/watchdog.c
+++ b/hw/watchdog.c
@@ -24,6 +24,16 @@
 #include "sysemu.h"
 #include "hw/watchdog.h"
 
+/* Possible values for action parameter. */
+#define WDT_RESET        1	/* Hard reset. */
+#define WDT_SHUTDOWN     2	/* Shutdown. */
+#define WDT_POWEROFF     3	/* Quit. */
+#define WDT_PAUSE        4	/* Pause. */
+#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;
 
 void watchdog_add_model(WatchdogTimerModel *model)
diff --git a/hw/watchdog.h b/hw/watchdog.h
index ad1fcfc..bb81204 100644
--- a/hw/watchdog.h
+++ b/hw/watchdog.h
@@ -25,13 +25,6 @@
 extern void wdt_i6300esb_init(void);
 extern void wdt_ib700_init(void);
 
-/* Possible values for action parameter. */
-#define WDT_RESET        1	/* Hard reset. */
-#define WDT_SHUTDOWN     2	/* Shutdown. */
-#define WDT_POWEROFF     3	/* Quit. */
-#define WDT_PAUSE        4	/* Pause. */
-#define WDT_DEBUG        5	/* Prints a message and continues running. */
-#define WDT_NONE         6	/* Do nothing. */
 
 struct WatchdogTimerModel {
     LIST_ENTRY(WatchdogTimerModel) entry;
@@ -48,10 +41,6 @@ struct WatchdogTimerModel {
 };
 typedef struct WatchdogTimerModel WatchdogTimerModel;
 
-/* in vl.c */
-extern WatchdogTimerModel *watchdog;
-extern int watchdog_action;
-
 /* in hw/watchdog.c */
 extern int select_watchdog(const char *p);
 extern int select_watchdog_action(const char *action);
diff --git a/vl.c b/vl.c
index 8b2b289..9b73ea3 100644
--- a/vl.c
+++ b/vl.c
@@ -233,8 +233,6 @@ int graphic_rotate = 0;
 #ifndef _WIN32
 int daemonize = 0;
 #endif
-WatchdogTimerModel *watchdog = NULL;
-int watchdog_action = WDT_RESET;
 const char *option_rom[MAX_OPTION_ROMS];
 int nb_option_roms;
 int semihosting_enabled = 0;
-- 
1.6.0.6

[Qemu-devel] [PATCH 2/3] Clean up upcast from PCIDevice to I6300State

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/wdt_i6300esb.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/wdt_i6300esb.c b/hw/wdt_i6300esb.c
index 5e9fd7c..2227303 100644
--- a/hw/wdt_i6300esb.c
+++ b/hw/wdt_i6300esb.c
@@ -25,7 +25,6 @@
 #include "qemu-timer.h"
 #include "watchdog.h"
 #include "hw.h"
-#include "isa.h"
 #include "pc.h"
 #include "pci.h"
 
@@ -71,7 +70,7 @@
 
 /* Device state. */
 struct I6300State {
-    PCIDevice dev;              /* PCI device state, must be first field. */
+    PCIDevice dev;
 
     int reboot_enabled;         /* "Reboot" on timer expiry.  The real action
                                  * performed depends on the -watchdog-action
@@ -199,7 +198,7 @@ static void i6300esb_timer_expired(void *vp)
 static void i6300esb_config_write(PCIDevice *dev, uint32_t addr,
                                   uint32_t data, int len)
 {
-    I6300State *d = (I6300State *) dev;
+    I6300State *d = container_of(dev, I6300State, dev);
     int old;
 
     i6300esb_debug("addr = %x, data = %x, len = %d\n", addr, data, len);
@@ -227,7 +226,7 @@ static void i6300esb_config_write(PCIDevice *dev, uint32_t addr,
 
 static uint32_t i6300esb_config_read(PCIDevice *dev, uint32_t addr, int len)
 {
-    I6300State *d = (I6300State *) dev;
+    I6300State *d = container_of(dev, I6300State, dev);
     uint32_t data;
 
     i6300esb_debug ("addr = %x, len = %d\n", addr, len);
@@ -361,7 +360,7 @@ static void i6300esb_map(PCIDevice *dev, int region_num,
         i6300esb_mem_writew,
         i6300esb_mem_writel,
     };
-    I6300State *d = (I6300State *) dev;
+    I6300State *d = container_of(dev, I6300State, dev);
     int io_mem;
 
     i6300esb_debug("addr = %x, size = %x, type = %d\n", addr, size, type);
-- 
1.6.0.6

[Qemu-devel] [PATCH 3/3] qdev: convert watchdogs

[Markus Armbruster]

-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(-)

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);
     }
-- 
1.6.0.6

Re: [Qemu-devel] [PATCH 3/3] qdev: convert watchdogs

[Gerd Hoffmann]

> +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

[Qemu-devel] container_of() vs DO_UPCAST() (was: [PATCH 2/3] Clean up upcast from PCIDevice to I6300State)

[Markus Armbruster]

We got container_of() in osdep.h.

We also got DO_UPCAST() in qdev.h.  Odd place, as it's not really
specific to qdev (except for the naming of the last parameter).  It
takes the same parameters as container_of(), but in a different order,
which I find needlessly confusing.  The last parameter is insufficiently
parenthesized in the macro expansion.  Finally, I don't really like the
name --- I find container_of() much clearer --- but that's just me.

Why do we have two macros to do essentially the same thing?

When should container_of() be used, and when DO_UPCAST()?

Immutable qdev properties (was: [Qemu-devel] [PATCH 3/3] qdev: convert watchdogs)

[Markus Armbruster]

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!

Re: [Qemu-devel] container_of() vs DO_UPCAST() (was: [PATCH 2/3] Clean up upcast from PCIDevice to I6300State)

[Paul Brook]

On Monday 24 August 2009, Markus Armbruster wrote:
> We got container_of() in osdep.h.
>
> We also got DO_UPCAST() in qdev.h.  Odd place, as it's not really
> specific to qdev (except for the naming of the last parameter).  It
> takes the same parameters as container_of(), but in a different order,
> which I find needlessly confusing.  The last parameter is insufficiently
> parenthesized in the macro expansion.  Finally, I don't really like the
> name --- I find container_of() much clearer --- but that's just me.
>
> Why do we have two macros to do essentially the same thing?
>
> When should container_of() be used, and when DO_UPCAST()?

DO_UPCAST requires that the child object be at offset zero.

Paul

Re: [Qemu-devel] [PATCH 2/3] Clean up upcast from PCIDevice to I6300State

[Paul Brook]

>  /* Device state. */
>  struct I6300State {
> -    PCIDevice dev;             /* PCI device state, must be first field. */
> +    PCIDevice dev;
>...
> -    I6300State *d = (I6300State *) dev;
> +    I6300State *d = container_of(dev, I6300State, dev);

I'm pretty sure this is wrong, and code elsewhere still requires the PCIDevice 
be the first field in I6300State. e.g. i6300esb_pc_init. Did you actually try 
putting dev at a nonzero offset?

Paul

[Qemu-devel] Re: Immutable qdev properties

[Gerd Hoffmann]

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

[Qemu-devel] Re: Immutable qdev properties

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH 2/3] Clean up upcast from PCIDevice to I6300State

[Markus Armbruster]

Paul Brook <paul@codesourcery.com> writes:

>>  /* Device state. */
>>  struct I6300State {
>> -    PCIDevice dev;             /* PCI device state, must be first field. */
>> +    PCIDevice dev;
>>...
>> -    I6300State *d = (I6300State *) dev;
>> +    I6300State *d = container_of(dev, I6300State, dev);
>
> I'm pretty sure this is wrong, and code elsewhere still requires the PCIDevice 
> be the first field in I6300State. e.g. i6300esb_pc_init. Did you actually try 
> putting dev at a nonzero offset?

I believe this commit is correct, because no code remains in
wdt_i6300esb.c that relies on the zero offset, and qdev is not yet in
play.  However, the *next* commit (conversion to qdev) is wrong if qdev
requires the PCIDevice at offset zero (not obvious to me, document it?).

I'll use DO_UPCAST() when I respin the patch series, to match usage
elsewhere.

Thanks!

Re: [Qemu-devel] [PATCH 2/3] Clean up upcast from PCIDevice to I6300State

[Gerd Hoffmann]

   Hi,

> play.  However, the *next* commit (conversion to qdev) is wrong if qdev
> requires the PCIDevice at offset zero (not obvious to me, document it?).

qdev_create() allocates DeviceInfo->size bytes for you and assumes 
DeviceState starts at offset zero of your FooDeviceState.  I think that 
is the only place though.

Could probably be relaxed by adding a offset field to DeviceInfo, but I 
don't think that buys us much, so unless there is some real need I'd 
just go with the "DeviceState must be first" convention.

It also could be that there is some old code which does casts instead of 
using DO_UPCAST, especially for PCI because PCIDevice predates qdev. 
That I'd consider a bug though.

cheers,
   Gerd