[Qemu-devel] [PATCH 0/5] qdev: device capabilities

[Qemu-devel] [PATCH 0/5] qdev: device capabilities

[Gerd Hoffmann]

  Hi,

This patch series adds device capabilities to qdev devices.

First patch adds the infrastructure, next patches add the capabilities.
New member of this patch series is #5 which adds a watchdog capability
and uses it to kill the superfluous private driver list in watchdog.c.

Individual patches have more detailed descriptions.

cheers,
  Gerd

[Qemu-devel] [PATCH 1/5] qdev: device capabilities

[Gerd Hoffmann]

This patch adds device capabilities to qdev devices.  This is the
core code, following patches will add the individual capabilities
and tag drivers.

The capabilities will be printed by '-device ?' and 'info qdm", so
users and management apps can use it.

Future plans:  I plan to use them to get rid off some hard-coded
lists in qemu by using capabilities instead: pci nic list, watchdog
list, maybe more.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/qdev.c |   19 ++++++++++++++++++-
 hw/qdev.h |    5 +++++
 2 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index ff2f096..0d21152 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -107,8 +107,11 @@ DeviceState *qdev_create(BusState *bus, const char *name)
 
 static int qdev_print_devinfo(DeviceInfo *info, char *dest, int len)
 {
+    static const char *capname[] = {
+    };
+    const char *sep;
     int pos = 0;
-    int ret;
+    int ret,i;
 
     ret = snprintf(dest+pos, len-pos, "name \"%s\", bus %s",
                    info->name, info->bus_info->name);
@@ -125,6 +128,20 @@ static int qdev_print_devinfo(DeviceInfo *info, char *dest, int len)
         ret = snprintf(dest+pos, len-pos, ", no-user");
         pos += MIN(len-pos,ret);
     }
+    if (info->caps) {
+        ret = snprintf(dest+pos, len-pos, ", caps \"");
+        pos += MIN(len-pos,ret);
+        sep = "";
+        for (i = 0; i < ARRAY_SIZE(capname); i++) {
+            if (!(info->caps & (1 << i)))
+                continue;
+            ret = snprintf(dest+pos, len-pos, "%s%s", sep, capname[i]);
+            pos += MIN(len-pos,ret);
+            sep = ",";
+        }
+        ret = snprintf(dest+pos, len-pos, "\"");
+        pos += MIN(len-pos,ret);
+    }
     return pos;
 }
 
diff --git a/hw/qdev.h b/hw/qdev.h
index af735d7..3d07a24 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -102,6 +102,10 @@ typedef int (*qdev_initfn)(DeviceState *dev, DeviceInfo *info);
 typedef void (*SCSIAttachFn)(DeviceState *host, BlockDriverState *bdrv,
               int unit);
 
+enum DeviceCapBits {
+    dummy
+};
+
 struct DeviceInfo {
     const char *name;
     const char *alias;
@@ -109,6 +113,7 @@ struct DeviceInfo {
     size_t size;
     Property *props;
     int no_user;
+    uint32_t caps;
 
     /* Private to qdev / bus.  */
     qdev_initfn init;
-- 
1.6.2.5

[Qemu-devel] [PATCH 2/5] qdev: add audio capability

[Gerd Hoffmann]

... and tag sound drivers (well, only the pci ones as the others are not
yet converted to qdev).

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/ac97.c   |    1 +
 hw/es1370.c |    1 +
 hw/qdev.c   |    1 +
 hw/qdev.h   |    4 +++-
 4 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/hw/ac97.c b/hw/ac97.c
index 610ca60..cceb66b 100644
--- a/hw/ac97.c
+++ b/hw/ac97.c
@@ -1350,6 +1350,7 @@ static PCIDeviceInfo ac97_info = {
     .qdev.name    = "AC97",
     .qdev.desc    = "Intel 82801AA AC97 Audio",
     .qdev.size    = sizeof (AC97LinkState),
+    .qdev.caps    = DEV_CAP_AUDIO,
     .init         = ac97_initfn,
 };
 
diff --git a/hw/es1370.c b/hw/es1370.c
index 9071a48..2dfec15 100644
--- a/hw/es1370.c
+++ b/hw/es1370.c
@@ -1046,6 +1046,7 @@ static PCIDeviceInfo es1370_info = {
     .qdev.name    = "ES1370",
     .qdev.desc    = "ENSONIQ AudioPCI ES1370",
     .qdev.size    = sizeof (ES1370State),
+    .qdev.caps    = DEV_CAP_AUDIO,
     .init         = es1370_initfn,
 };
 
diff --git a/hw/qdev.c b/hw/qdev.c
index 0d21152..760efd4 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -108,6 +108,7 @@ DeviceState *qdev_create(BusState *bus, const char *name)
 static int qdev_print_devinfo(DeviceInfo *info, char *dest, int len)
 {
     static const char *capname[] = {
+        [ DEV_CAP_BIT_AUDIO       ] = "audio",
     };
     const char *sep;
     int pos = 0;
diff --git a/hw/qdev.h b/hw/qdev.h
index 3d07a24..ef05903 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -103,9 +103,11 @@ typedef void (*SCSIAttachFn)(DeviceState *host, BlockDriverState *bdrv,
               int unit);
 
 enum DeviceCapBits {
-    dummy
+    DEV_CAP_BIT_AUDIO      = 0,
 };
 
+#define DEV_CAP_AUDIO      (1 << DEV_CAP_BIT_AUDIO)
+
 struct DeviceInfo {
     const char *name;
     const char *alias;
-- 
1.6.2.5

[Qemu-devel] [PATCH 3/5] qdev: add ethernet capability

[Gerd Hoffmann]

... and tag ethernet drivers.  Also add some descriptions while being
at it.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/e1000.c      |    2 ++
 hw/eepro100.c   |    6 ++++++
 hw/ne2000.c     |    2 ++
 hw/pcnet.c      |    3 +++
 hw/qdev.c       |    1 +
 hw/qdev.h       |    2 ++
 hw/rtl8139.c    |    2 ++
 hw/virtio-pci.c |    1 +
 8 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/hw/e1000.c b/hw/e1000.c
index 95c471c..5f7744d 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -1137,7 +1137,9 @@ static int pci_e1000_init(PCIDevice *pci_dev)
 
 static PCIDeviceInfo e1000_info = {
     .qdev.name = "e1000",
+    .qdev.desc = "Intel Gigabit Ethernet",
     .qdev.size = sizeof(E1000State),
+    .qdev.caps = DEV_CAP_ETHERNET,
     .init      = pci_e1000_init,
 };
 
diff --git a/hw/eepro100.c b/hw/eepro100.c
index c374931..bc7b625 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -1788,15 +1788,21 @@ static int pci_i82559er_init(PCIDevice *dev)
 static PCIDeviceInfo eepro100_info[] = {
     {
         .qdev.name = "i82551",
+        .qdev.desc = "Intel EtherExpress PRO 100",
         .qdev.size = sizeof(EEPRO100State),
+        .qdev.caps = DEV_CAP_ETHERNET,
         .init      = pci_i82551_init,
     },{
         .qdev.name = "i82557b",
+        .qdev.desc = "Intel EtherExpress PRO 100",
         .qdev.size = sizeof(EEPRO100State),
+        .qdev.caps = DEV_CAP_ETHERNET,
         .init      = pci_i82557b_init,
     },{
         .qdev.name = "i82559er",
+        .qdev.desc = "Intel EtherExpress PRO 100",
         .qdev.size = sizeof(EEPRO100State),
+        .qdev.caps = DEV_CAP_ETHERNET,
         .init      = pci_i82559er_init,
     },{
         /* end of list */
diff --git a/hw/ne2000.c b/hw/ne2000.c
index bdfc9ed..ee9f293 100644
--- a/hw/ne2000.c
+++ b/hw/ne2000.c
@@ -848,7 +848,9 @@ static int pci_ne2000_init(PCIDevice *pci_dev)
 
 static PCIDeviceInfo ne2000_info = {
     .qdev.name = "ne2k_pci",
+    .qdev.desc = "NE2000 PCI Ethernet",
     .qdev.size = sizeof(PCINE2000State),
+    .qdev.caps = DEV_CAP_ETHERNET,
     .init      = pci_ne2000_init,
 };
 
diff --git a/hw/pcnet.c b/hw/pcnet.c
index 8c352d2..803a196 100644
--- a/hw/pcnet.c
+++ b/hw/pcnet.c
@@ -2158,6 +2158,7 @@ static SysBusDeviceInfo lance_info = {
     .init = lance_init,
     .qdev.name  = "lance",
     .qdev.size  = sizeof(SysBusPCNetState),
+    .qdev.caps  = DEV_CAP_ETHERNET,
     .qdev.props = (Property[]) {
         DEFINE_PROP_PTR("dma", SysBusPCNetState, state.dma_opaque),
         DEFINE_PROP_END_OF_LIST(),
@@ -2168,7 +2169,9 @@ static SysBusDeviceInfo lance_info = {
 
 static PCIDeviceInfo pcnet_info = {
     .qdev.name = "pcnet",
+    .qdev.desc = "PCnet Lance Ethernet",
     .qdev.size = sizeof(PCIPCNetState),
+    .qdev.caps = DEV_CAP_ETHERNET,
     .init      = pci_pcnet_init,
 };
 
diff --git a/hw/qdev.c b/hw/qdev.c
index 760efd4..ff7da96 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -109,6 +109,7 @@ static int qdev_print_devinfo(DeviceInfo *info, char *dest, int len)
 {
     static const char *capname[] = {
         [ DEV_CAP_BIT_AUDIO       ] = "audio",
+        [ DEV_CAP_BIT_ETHERNET    ] = "ethernet",
     };
     const char *sep;
     int pos = 0;
diff --git a/hw/qdev.h b/hw/qdev.h
index ef05903..45808fa 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -104,9 +104,11 @@ typedef void (*SCSIAttachFn)(DeviceState *host, BlockDriverState *bdrv,
 
 enum DeviceCapBits {
     DEV_CAP_BIT_AUDIO      = 0,
+    DEV_CAP_BIT_ETHERNET   = 1,
 };
 
 #define DEV_CAP_AUDIO      (1 << DEV_CAP_BIT_AUDIO)
+#define DEV_CAP_ETHERNET   (1 << DEV_CAP_BIT_ETHERNET)
 
 struct DeviceInfo {
     const char *name;
diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index b3542a3..1dc39be 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -3490,7 +3490,9 @@ static int pci_rtl8139_init(PCIDevice *dev)
 
 static PCIDeviceInfo rtl8139_info = {
     .qdev.name = "rtl8139",
+    .qdev.desc = "Realtek RTL8139 Fast Ethernet",
     .qdev.size = sizeof(RTL8139State),
+    .qdev.caps = DEV_CAP_ETHERNET,
     .init      = pci_rtl8139_init,
 };
 
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index f812ab7..20c89d7 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -518,6 +518,7 @@ static PCIDeviceInfo virtio_info[] = {
     },{
         .qdev.name  = "virtio-net-pci",
         .qdev.size  = sizeof(VirtIOPCIProxy),
+        .qdev.caps  = DEV_CAP_ETHERNET,
         .init       = virtio_net_init_pci,
         .qdev.props = (Property[]) {
             DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors,
-- 
1.6.2.5

[Qemu-devel] [PATCH 4/5] qdev: add display capability

[Gerd Hoffmann]

... and tag devices.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/cirrus_vga.c |    1 +
 hw/qdev.c       |    1 +
 hw/qdev.h       |    2 ++
 hw/syborg_fb.c  |    1 +
 hw/tcx.c        |    1 +
 hw/vga.c        |    1 +
 hw/vmware_vga.c |    1 +
 7 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index 991d1da..d530305 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -3361,6 +3361,7 @@ void pci_cirrus_vga_init(PCIBus *bus)
 static PCIDeviceInfo cirrus_vga_info = {
     .qdev.name    = "Cirrus VGA",
     .qdev.size    = sizeof(PCICirrusVGAState),
+    .qdev.caps    = DEV_CAP_DISPLAY,
     .init         = pci_cirrus_vga_initfn,
     .config_write = pci_cirrus_write_config,
 };
diff --git a/hw/qdev.c b/hw/qdev.c
index ff7da96..b6a1a1d 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -110,6 +110,7 @@ static int qdev_print_devinfo(DeviceInfo *info, char *dest, int len)
     static const char *capname[] = {
         [ DEV_CAP_BIT_AUDIO       ] = "audio",
         [ DEV_CAP_BIT_ETHERNET    ] = "ethernet",
+        [ DEV_CAP_BIT_DISPLAY     ] = "display",
     };
     const char *sep;
     int pos = 0;
diff --git a/hw/qdev.h b/hw/qdev.h
index 45808fa..2df6ad3 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -105,10 +105,12 @@ typedef void (*SCSIAttachFn)(DeviceState *host, BlockDriverState *bdrv,
 enum DeviceCapBits {
     DEV_CAP_BIT_AUDIO      = 0,
     DEV_CAP_BIT_ETHERNET   = 1,
+    DEV_CAP_BIT_DISPLAY    = 2,
 };
 
 #define DEV_CAP_AUDIO      (1 << DEV_CAP_BIT_AUDIO)
 #define DEV_CAP_ETHERNET   (1 << DEV_CAP_BIT_ETHERNET)
+#define DEV_CAP_DISPLAY    (1 << DEV_CAP_BIT_DISPLAY)
 
 struct DeviceInfo {
     const char *name;
diff --git a/hw/syborg_fb.c b/hw/syborg_fb.c
index 7be04a3..c4a2a15 100644
--- a/hw/syborg_fb.c
+++ b/hw/syborg_fb.c
@@ -535,6 +535,7 @@ static SysBusDeviceInfo syborg_fb_info = {
     .init = syborg_fb_init,
     .qdev.name  = "syborg,framebuffer",
     .qdev.size  = sizeof(SyborgFBState),
+    .qdev.caps  = DEV_CAP_DISPLAY,
     .qdev.props = (Property[]) {
         DEFINE_PROP_UINT32("width",  SyborgFBState, cols, 0),
         DEFINE_PROP_UINT32("height", SyborgFBState, rows, 0),
diff --git a/hw/tcx.c b/hw/tcx.c
index d1e5851..b809e51 100644
--- a/hw/tcx.c
+++ b/hw/tcx.c
@@ -634,6 +634,7 @@ static SysBusDeviceInfo tcx_info = {
     .init = tcx_init1,
     .qdev.name  = "SUNW,tcx",
     .qdev.size  = sizeof(TCXState),
+    .qdev.caps  = DEV_CAP_DISPLAY,
     .qdev.props = (Property[]) {
         DEFINE_PROP_TADDR("addr",      TCXState, addr,      -1),
         DEFINE_PROP_HEX32("vram_size", TCXState, vram_size, -1),
diff --git a/hw/vga.c b/hw/vga.c
index 6b5070a..4898b7a 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -2539,6 +2539,7 @@ int pci_vga_init(PCIBus *bus,
 static PCIDeviceInfo vga_info = {
     .qdev.name    = "VGA",
     .qdev.size    = sizeof(PCIVGAState),
+    .qdev.caps    = DEV_CAP_DISPLAY,
     .init         = pci_vga_initfn,
     .config_write = pci_vga_write_config,
     .qdev.props   = (Property[]) {
diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index c6bce5a..151edf1 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -1247,6 +1247,7 @@ void pci_vmsvga_init(PCIBus *bus)
 static PCIDeviceInfo vmsvga_info = {
     .qdev.name    = "QEMUware SVGA",
     .qdev.size    = sizeof(struct pci_vmsvga_state_s),
+    .qdev.caps    = DEV_CAP_DISPLAY,
     .init         = pci_vmsvga_initfn,
 };
 
-- 
1.6.2.5

[Qemu-devel] [PATCH 5/5] qdev: add watchdog capability

[Gerd Hoffmann]

This patch add watchdog capability and tags the two watchdog drivers.
It also kill the private register functions and driver list in
watchdog.c, instead the qdev driver list and the watchdog capability
bit is used.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/qdev.c         |    2 +-
 hw/qdev.h         |    3 +++
 hw/watchdog.c     |   53 +++++++++++++++++++++++++++++------------------------
 hw/watchdog.h     |   11 -----------
 hw/wdt_i6300esb.c |    8 ++------
 hw/wdt_ib700.c    |    8 ++------
 6 files changed, 37 insertions(+), 48 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index b6a1a1d..a26bd46 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -33,7 +33,7 @@
 /* This is a nasty hack to allow passing a NULL bus to qdev_create.  */
 static BusState *main_system_bus;
 
-static DeviceInfo *device_info_list;
+DeviceInfo *device_info_list;
 
 static BusState *qbus_find_recursive(BusState *bus, const char *name,
                                      const BusInfo *info);
diff --git a/hw/qdev.h b/hw/qdev.h
index 2df6ad3..d443534 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -106,11 +106,13 @@ enum DeviceCapBits {
     DEV_CAP_BIT_AUDIO      = 0,
     DEV_CAP_BIT_ETHERNET   = 1,
     DEV_CAP_BIT_DISPLAY    = 2,
+    DEV_CAP_BIT_WATCHDOG   = 3,
 };
 
 #define DEV_CAP_AUDIO      (1 << DEV_CAP_BIT_AUDIO)
 #define DEV_CAP_ETHERNET   (1 << DEV_CAP_BIT_ETHERNET)
 #define DEV_CAP_DISPLAY    (1 << DEV_CAP_BIT_DISPLAY)
+#define DEV_CAP_WATCHDOG   (1 << DEV_CAP_BIT_WATCHDOG)
 
 struct DeviceInfo {
     const char *name;
@@ -126,6 +128,7 @@ struct DeviceInfo {
     BusInfo *bus_info;
     struct DeviceInfo *next;
 };
+extern DeviceInfo *device_info_list;
 
 void qdev_register(DeviceInfo *info);
 
diff --git a/hw/watchdog.c b/hw/watchdog.c
index adba872..5c6bb8a 100644
--- a/hw/watchdog.c
+++ b/hw/watchdog.c
@@ -24,6 +24,7 @@
 #include "qemu-config.h"
 #include "sys-queue.h"
 #include "sysemu.h"
+#include "qdev.h"
 #include "hw/watchdog.h"
 
 /* Possible values for action parameter. */
@@ -35,47 +36,51 @@
 #define WDT_NONE         6	/* Do nothing. */
 
 static int watchdog_action = WDT_RESET;
-static LIST_HEAD(watchdog_list, WatchdogTimerModel) watchdog_list;
-
-void watchdog_add_model(WatchdogTimerModel *model)
-{
-    LIST_INSERT_HEAD(&watchdog_list, model, entry);
-}
 
 /* Returns:
  *   0 = continue
  *   1 = exit program with error
  *   2 = exit program without error
  */
+
+static void print_watchdog_list(FILE *fp)
+{
+    DeviceInfo *info;
+
+    for (info = device_info_list; info != NULL; info = info->next) {
+        if (!(info->caps & DEV_CAP_WATCHDOG))
+            continue;
+        fprintf(fp, "    %-10s %s (%s)\n", info->name, info->desc, info->bus_info->name);
+    }
+}
+
 int select_watchdog(const char *p)
 {
-    WatchdogTimerModel *model;
+    DeviceInfo *info;
     QemuOpts *opts;
 
     /* -watchdog ? lists available devices and exits cleanly. */
     if (strcmp(p, "?") == 0) {
-        LIST_FOREACH(model, &watchdog_list, entry) {
-            fprintf(stderr, "\t%s\t%s\n",
-                     model->wdt_name, model->wdt_description);
-        }
+        print_watchdog_list(stderr);
         return 2;
     }
 
-    LIST_FOREACH(model, &watchdog_list, entry) {
-        if (strcasecmp(model->wdt_name, p) == 0) {
-            /* add the device */
-            opts = qemu_opts_create(&qemu_device_opts, NULL, 0);
-            qemu_opt_set(opts, "driver", p);
-            return 0;
-        }
+    for (info = device_info_list; info != NULL; info = info->next) {
+        if (!(info->caps & DEV_CAP_WATCHDOG))
+            continue;
+        if (strcmp(p, info->name) == 0)
+            break;
     }
-
-    fprintf(stderr, "Unknown -watchdog device. Supported devices are:\n");
-    LIST_FOREACH(model, &watchdog_list, entry) {
-        fprintf(stderr, "\t%s\t%s\n",
-                 model->wdt_name, model->wdt_description);
+    if (info == NULL) {
+        fprintf(stderr, "Unknown -watchdog device. Supported devices are:\n");
+        print_watchdog_list(stderr);
+        return 1;
     }
-    return 1;
+
+    /* add the device */
+    opts = qemu_opts_create(&qemu_device_opts, NULL, 0);
+    qemu_opt_set(opts, "driver", p);
+    return 0;
 }
 
 int select_watchdog_action(const char *p)
diff --git a/hw/watchdog.h b/hw/watchdog.h
index 8c54fa4..2c042af 100644
--- a/hw/watchdog.h
+++ b/hw/watchdog.h
@@ -22,20 +22,9 @@
 #ifndef QEMU_WATCHDOG_H
 #define QEMU_WATCHDOG_H
 
-struct WatchdogTimerModel {
-    LIST_ENTRY(WatchdogTimerModel) entry;
-
-    /* Short name of the device - used to select it on the command line. */
-    const char *wdt_name;
-    /* Longer description (eg. manufacturer and full model number). */
-    const char *wdt_description;
-};
-typedef struct WatchdogTimerModel WatchdogTimerModel;
-
 /* in hw/watchdog.c */
 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);
 
 #endif /* QEMU_WATCHDOG_H */
diff --git a/hw/wdt_i6300esb.c b/hw/wdt_i6300esb.c
index 6927d43..1b46cbf 100644
--- a/hw/wdt_i6300esb.c
+++ b/hw/wdt_i6300esb.c
@@ -446,14 +446,11 @@ static int i6300esb_init(PCIDevice *dev)
     return 0;
 }
 
-static WatchdogTimerModel model = {
-    .wdt_name = "i6300esb",
-    .wdt_description = "Intel 6300ESB",
-};
-
 static PCIDeviceInfo i6300esb_info = {
     .qdev.name    = "i6300esb",
+    .qdev.desc    = "Intel 6300ESB",
     .qdev.size    = sizeof(I6300State),
+    .qdev.caps    = DEV_CAP_WATCHDOG,
     .config_read  = i6300esb_config_read,
     .config_write = i6300esb_config_write,
     .init         = i6300esb_init,
@@ -461,7 +458,6 @@ static PCIDeviceInfo i6300esb_info = {
 
 static void i6300esb_register_devices(void)
 {
-    watchdog_add_model(&model);
     pci_qdev_register(&i6300esb_info);
 }
 
diff --git a/hw/wdt_ib700.c b/hw/wdt_ib700.c
index e0ee65a..c1570a4 100644
--- a/hw/wdt_ib700.c
+++ b/hw/wdt_ib700.c
@@ -98,20 +98,16 @@ static int wdt_ib700_init(ISADevice *dev)
     return 0;
 }
 
-static WatchdogTimerModel model = {
-    .wdt_name = "ib700",
-    .wdt_description = "iBASE 700",
-};
-
 static ISADeviceInfo wdt_ib700_info = {
     .qdev.name = "ib700",
+    .qdev.desc = "iBASE 700",
     .qdev.size = sizeof(ISADevice),
+    .qdev.caps = DEV_CAP_WATCHDOG,
     .init      = wdt_ib700_init,
 };
 
 static void wdt_ib700_register_devices(void)
 {
-    watchdog_add_model(&model);
     isa_qdev_register(&wdt_ib700_info);
 }
 
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 0/5] qdev: device capabilities

[Anthony Liguori]

Gerd Hoffmann wrote:
>   Hi,
>
> This patch series adds device capabilities to qdev devices.
>
> First patch adds the infrastructure, next patches add the capabilities.
> New member of this patch series is #5 which adds a watchdog capability
> and uses it to kill the superfluous private driver list in watchdog.c.
>
> Individual patches have more detailed descriptions.
>   

I still don't understand why this is needed.  It's duplicating data that 
should already be present in the device model.

A device has a capability because it supports a particular interface.  
Doesn't it make more sense to tie that interface to qdev instead of 
having to explicitly state the device has that interface?

Regards,

Anthony Liguori

> cheers,
>   Gerd
>
>
>
>   

Re: [Qemu-devel] [PATCH 0/5] qdev: device capabilities

[Gerd Hoffmann]

On 09/04/09 17:08, Anthony Liguori wrote:
> Gerd Hoffmann wrote:
>> Hi,
>>
>> This patch series adds device capabilities to qdev devices.
>>
>> First patch adds the infrastructure, next patches add the capabilities.
>> New member of this patch series is #5 which adds a watchdog capability
>> and uses it to kill the superfluous private driver list in watchdog.c.
>>
>> Individual patches have more detailed descriptions.
>
> I still don't understand why this is needed. It's duplicating data that
> should already be present in the device model.

Look at patch #5 (watchdogs).  What else do you suggest here?
There is nothing which can be used to identify the device as watchdog.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 0/5] qdev: device capabilities

[Anthony Liguori]

Gerd Hoffmann wrote:
> Look at patch #5 (watchdogs).  What else do you suggest here?
> There is nothing which can be used to identify the device as watchdog.

That's a problem with the watchdog timers.  The WatchdogTimerModel stuff 
really replicates qdev functionality.

Ideally, the watchdog_list would go away and we would just make 
-watchdog an alias for -device.  Instead of having a global watchdog 
action, we should have a watchdog frontend/backend and allow a user to 
specify the action for a watchdog backend.  This is basically what 
-watchdog-action does.

The current command line options assume one global watchdog.  I don't 
see why we shouldn't support two though.  Even if we limited ourselves 
to one, it should use the same infrastructure as everything else.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 0/5] qdev: device capabilities

[Gerd Hoffmann]

On 09/05/09 15:39, Anthony Liguori wrote:
> Gerd Hoffmann wrote:
>> Look at patch #5 (watchdogs). What else do you suggest here?
>> There is nothing which can be used to identify the device as watchdog.
>
> That's a problem with the watchdog timers. The WatchdogTimerModel stuff
> really replicates qdev functionality.

Good point, I fully agree.
Patch #5 kills WatchdogTimerModel.

> Ideally, the watchdog_list would go away and we would just make
> -watchdog an alias for -device.

Patch #5 does just that.  Almost.  -watchdog continues to accept 
watchdog devices only.  And '-watchdog ?' lists watchdog devices only.

watchdog_list is gone, the qdev list is used instead.  To identify the 
watchdog devices in the qdev device model list the capability bit is 
used.  The patch description says so, doesn't it?

> Instead of having a global watchdog
> action, we should have a watchdog frontend/backend and allow a user to
> specify the action for a watchdog backend. This is basically what
> -watchdog-action does.

Sounds complicated.  Isn't that over-engineering it a bit?  The 
-watchdog-action implementation is just two little functions in 
watchdog.c right now ...

> The current command line options assume one global watchdog. I don't see
> why we shouldn't support two though. Even if we limited ourselves to
> one, it should use the same infrastructure as everything else.

After applying patch #5 you can have two watchdogs, no problem ;)

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 0/5] qdev: device capabilities

[Anthony Liguori]

Gerd Hoffmann wrote:
> Patch #5 does just that.  Almost.  -watchdog continues to accept 
> watchdog devices only.  And '-watchdog ?' lists watchdog devices only.
>
> watchdog_list is gone, the qdev list is used instead.  To identify the 
> watchdog devices in the qdev device model list the capability bit is 
> used.  The patch description says so, doesn't it?

Instead of checking the capability bit, why not look for a property with 
a type that's unique to the watch dog timer.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 0/5] qdev: device capabilities

[Gerd Hoffmann]

On 09/07/09 22:36, Anthony Liguori wrote:
> Gerd Hoffmann wrote:
>> Patch #5 does just that. Almost. -watchdog continues to accept
>> watchdog devices only. And '-watchdog ?' lists watchdog devices only.
>>
>> watchdog_list is gone, the qdev list is used instead. To identify the
>> watchdog devices in the qdev device model list the capability bit is
>> used. The patch description says so, doesn't it?
>
> Instead of checking the capability bit, why not look for a property with
> a type that's unique to the watch dog timer.

*There is no such property.*

We are running in circles for months now.  You are suggesting to look 
for a unique property.  I point out that it simply doesn't work for 
certain devices due to lack of such a property.  Repeat.

Can we stop that please?

Do you suggest to create some unused dummy property to tag devices?

thanks,
   Gerd

Re: [Qemu-devel] [PATCH 0/5] qdev: device capabilities

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> On 09/07/09 22:36, Anthony Liguori wrote:
>> Gerd Hoffmann wrote:
>>> Patch #5 does just that. Almost. -watchdog continues to accept
>>> watchdog devices only. And '-watchdog ?' lists watchdog devices only.
>>>
>>> watchdog_list is gone, the qdev list is used instead. To identify the
>>> watchdog devices in the qdev device model list the capability bit is
>>> used. The patch description says so, doesn't it?
>>
>> Instead of checking the capability bit, why not look for a property with
>> a type that's unique to the watch dog timer.
>
> *There is no such property.*
>
> We are running in circles for months now.  You are suggesting to look
> for a unique property.  I point out that it simply doesn't work for
> certain devices due to lack of such a property.  Repeat.
>
> Can we stop that please?
>
> Do you suggest to create some unused dummy property to tag devices?

We could make it a set of well-known bits, say bit#0 audio, bit#1
ethernet, bit#2 display, bit#3 watchdog, ..., and call it "caps" ;)

I think we all agree that we want watchdog_list replaced by filtering
watchdog devices from the qdev list.  I figure there will be more
applications for that elsewhere (-net nic,model=?  comes to mind).

Filtering the qdev list requires a way to ask "what kind of device is
this?", or at least "is this a FOO device?", for the various FOOs.

Capabilities support that in a simple, general way.

We can answer either question by detecting characteristic properties
instead.  But it'll require artificial characteristic properties, as the
watchdog example demonstrates.  And I doubt it'll be easier or cleaner
in the end.