[Qemu-devel] [PATCH 0/8] ide: convert to qdev.

[Qemu-devel] [PATCH 0/8] ide: convert to qdev.

[Gerd Hoffmann]

  Hi,

Here is a patch series which starts the conversion of ide to qdev.
It brings the core infrastructure and converts pci+isa ide adapters
to qdev.  Also some minor preparatory patches and bugfixes.

cheers,
  Gerd

[Qemu-devel] [PATCH 1/8] qdev/pci: add pci_create_noinit()

[Gerd Hoffmann]

Like pci_create_simple() but doesn't call qdev_init(), so one can
set properties before initializing the device.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci.c |   11 ++++++++---
 hw/pci.h |    1 +
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index c12b0be..64d70ed 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -923,15 +923,20 @@ void pci_qdev_register_many(PCIDeviceInfo *info)
     }
 }
 
-PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name)
+PCIDevice *pci_create_noinit(PCIBus *bus, int devfn, const char *name)
 {
     DeviceState *dev;
 
     dev = qdev_create(&bus->qbus, name);
     qdev_prop_set_uint32(dev, "addr", devfn);
-    qdev_init(dev);
+    return DO_UPCAST(PCIDevice, qdev, dev);
+}
 
-    return (PCIDevice *)dev;
+PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name)
+{
+    PCIDevice *dev = pci_create_noinit(bus, devfn, name);
+    qdev_init(&dev->qdev);
+    return dev;
 }
 
 static int pci_find_space(PCIDevice *pdev, uint8_t size)
diff --git a/hw/pci.h b/hw/pci.h
index 6196b6a..e7bf33a 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -328,6 +328,7 @@ void pci_qdev_register(PCIDeviceInfo *info);
 void pci_qdev_register_many(PCIDeviceInfo *info);
 
 PCIDevice *pci_create(const char *name, const char *devaddr);
+PCIDevice *pci_create_noinit(PCIBus *bus, int devfn, const char *name);
 PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name);
 
 /* lsi53c895a.c */
-- 
1.6.2.5

[Qemu-devel] [PATCH 2/8] support media=cdrom for if=none

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 vl.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/vl.c b/vl.c
index c6c6a6b..316dc49 100644
--- a/vl.c
+++ b/vl.c
@@ -2187,6 +2187,7 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
     case IF_IDE:
     case IF_SCSI:
     case IF_XEN:
+    case IF_NONE:
         switch(media) {
 	case MEDIA_DISK:
             if (cyls != 0) {
@@ -2207,7 +2208,6 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
         break;
     case IF_PFLASH:
     case IF_MTD:
-    case IF_NONE:
         break;
     case IF_VIRTIO:
         /* add virtio block device */
-- 
1.6.2.5

[Qemu-devel] [PATCH 3/8] split away drive init from ide_init2()

[Gerd Hoffmann]

This allows the ide bus being initialized without drives attached
and the drives being attached and initialization later on as
separate step.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/ide/core.c     |   71 +++++++++++++++++++++++++++++------------------------
 hw/ide/internal.h |    1 +
 2 files changed, 40 insertions(+), 32 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index fe5bd17..bac2d5e 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2505,51 +2505,58 @@ void ide_reset(IDEState *s)
     s->media_changed = 0;
 }
 
+void ide_init_drive(IDEState *s, DriveInfo *dinfo)
+{
+    int cylinders, heads, secs;
+    uint64_t nb_sectors;
+
+    if (dinfo && dinfo->bdrv) {
+        s->bs = dinfo->bdrv;
+        bdrv_get_geometry(s->bs, &nb_sectors);
+        bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
+        s->cylinders = cylinders;
+        s->heads = heads;
+        s->sectors = secs;
+        s->nb_sectors = nb_sectors;
+        /* The SMART values should be preserved across power cycles
+           but they aren't.  */
+        s->smart_enabled = 1;
+        s->smart_autosave = 1;
+        s->smart_errors = 0;
+        s->smart_selftest_count = 0;
+        if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM) {
+            s->is_cdrom = 1;
+            bdrv_set_change_cb(s->bs, cdrom_change_cb, s);
+        }
+        strncpy(s->drive_serial_str, drive_get_serial(s->bs),
+                sizeof(s->drive_serial_str));
+    }
+    if (strlen(s->drive_serial_str) == 0)
+        snprintf(s->drive_serial_str, sizeof(s->drive_serial_str),
+                 "QM%05d", s->drive_serial);
+    ide_reset(s);
+}
+
 void ide_init2(IDEBus *bus, DriveInfo *hd0, DriveInfo *hd1,
                qemu_irq irq)
 {
     IDEState *s;
     static int drive_serial = 1;
-    int i, cylinders, heads, secs;
-    uint64_t nb_sectors;
+    int i;
 
     for(i = 0; i < 2; i++) {
         s = bus->ifs + i;
         s->bus = bus;
         s->unit = i;
-        if (i == 0 && hd0)
-            s->bs = hd0->bdrv;
-        if (i == 1 && hd1)
-            s->bs = hd1->bdrv;
-        s->io_buffer = qemu_blockalign(s->bs, IDE_DMA_BUF_SECTORS*512 + 4);
-        if (s->bs) {
-            bdrv_get_geometry(s->bs, &nb_sectors);
-            bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
-            s->cylinders = cylinders;
-            s->heads = heads;
-            s->sectors = secs;
-            s->nb_sectors = nb_sectors;
-	    /* The SMART values should be preserved across power cycles
-	       but they aren't.  */
-	    s->smart_enabled = 1;
-	    s->smart_autosave = 1;
-	    s->smart_errors = 0;
-	    s->smart_selftest_count = 0;
-	    s->smart_selftest_data = qemu_blockalign(s->bs, 512);
-            if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM) {
-                s->is_cdrom = 1;
-		bdrv_set_change_cb(s->bs, cdrom_change_cb, s);
-            }
-        }
         s->drive_serial = drive_serial++;
-        strncpy(s->drive_serial_str, drive_get_serial(s->bs),
-                sizeof(s->drive_serial_str));
-        if (strlen(s->drive_serial_str) == 0)
-            snprintf(s->drive_serial_str, sizeof(s->drive_serial_str),
-                    "QM%05d", s->drive_serial);
+        s->io_buffer = qemu_blockalign(s->bs, IDE_DMA_BUF_SECTORS*512 + 4);
+        s->smart_selftest_data = qemu_blockalign(s->bs, 512);
         s->sector_write_timer = qemu_new_timer(vm_clock,
                                                ide_sector_write_timer_cb, s);
-        ide_reset(s);
+        if (i == 0)
+            ide_init_drive(s, hd0);
+        if (i == 1)
+            ide_init_drive(s, hd1);
     }
     bus->irq = irq;
 }
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index b9a7c72..62aef1c 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -527,6 +527,7 @@ uint32_t ide_data_readw(void *opaque, uint32_t addr);
 void ide_data_writel(void *opaque, uint32_t addr, uint32_t val);
 uint32_t ide_data_readl(void *opaque, uint32_t addr);
 
+void ide_init_drive(IDEState *s, DriveInfo *dinfo);
 void ide_init2(IDEBus *bus, DriveInfo *hd0, DriveInfo *hd1,
                qemu_irq irq);
 void ide_init_ioport(IDEBus *bus, int iobase, int iobase2);
-- 
1.6.2.5

[Qemu-devel] [PATCH 4/8] ide/qdev: add ide bus.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/ide/internal.h |   24 ++++++++++-
 hw/ide/qdev.c     |  123 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 146 insertions(+), 1 deletions(-)
 create mode 100644 hw/ide/qdev.c

diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 62aef1c..9df759d 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -15,6 +15,8 @@
 #define USE_DMA_CDROM
 
 typedef struct IDEBus IDEBus;
+typedef struct IDEDevice IDEDevice;
+typedef struct IDEDeviceInfo IDEDeviceInfo;
 typedef struct IDEState IDEState;
 typedef struct BMDMAState BMDMAState;
 
@@ -440,6 +442,8 @@ struct IDEState {
 
 struct IDEBus {
     BusState qbus;
+    IDEDevice *master;
+    IDEDevice *slave;
     BMDMAState *bmdma;
     IDEState ifs[2];
     uint8_t unit;
@@ -447,6 +451,20 @@ struct IDEBus {
     qemu_irq irq;
 };
 
+struct IDEDevice {
+    DeviceState qdev;
+    uint32_t unit;
+    DriveInfo *dinfo;
+};
+
+typedef int (*ide_qdev_initfn)(IDEDevice *dev);
+struct IDEDeviceInfo {
+    DeviceInfo qdev;
+    ide_qdev_initfn init;
+    uint32_t unit;
+    DriveInfo *drive;
+};
+
 #define BM_STATUS_DMAING 0x01
 #define BM_STATUS_ERROR  0x02
 #define BM_STATUS_INT    0x04
@@ -500,7 +518,7 @@ static inline void ide_set_irq(IDEBus *bus)
     }
 }
 
-/* ide.c */
+/* hw/ide/core.c */
 void ide_save(QEMUFile* f, IDEState *s);
 void ide_load(QEMUFile* f, IDEState *s, int version_id);
 void idebus_save(QEMUFile* f, IDEBus *bus);
@@ -532,4 +550,8 @@ void ide_init2(IDEBus *bus, DriveInfo *hd0, DriveInfo *hd1,
                qemu_irq irq);
 void ide_init_ioport(IDEBus *bus, int iobase, int iobase2);
 
+/* hw/ide/qdev.c */
+IDEBus *ide_bus_new(DeviceState *dev);
+IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive);
+
 #endif /* HW_IDE_INTERNAL_H */
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
new file mode 100644
index 0000000..6026c28
--- /dev/null
+++ b/hw/ide/qdev.c
@@ -0,0 +1,123 @@
+/*
+ * ide bus support for qdev.
+ *
+ * Copyright (c) 2009 Gerd Hoffmann <kraxel@redhat.com>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+#include <hw/hw.h>
+#include "sysemu.h"
+#include "dma.h"
+
+#include <hw/ide/internal.h>
+
+/* --------------------------------- */
+
+static struct BusInfo ide_bus_info = {
+    .name  = "IDE",
+    .size  = sizeof(IDEBus),
+};
+
+IDEBus *ide_bus_new(DeviceState *dev)
+{
+    IDEBus *idebus;
+
+    idebus = FROM_QBUS(IDEBus, qbus_create(&ide_bus_info, dev, NULL));
+    return idebus;
+}
+
+static int ide_qdev_init(DeviceState *qdev, DeviceInfo *base)
+{
+    IDEDevice *dev = DO_UPCAST(IDEDevice, qdev, qdev);
+    IDEDeviceInfo *info = DO_UPCAST(IDEDeviceInfo, qdev, base);
+    IDEBus *bus = DO_UPCAST(IDEBus, qbus, qdev->parent_bus);
+
+    if (!dev->dinfo) {
+        fprintf(stderr, "%s: no drive specified\n", qdev->info->name);
+        goto err;
+    }
+    if (dev->unit == -1) {
+        dev->unit = bus->master ? 1 : 0;
+    }
+    switch (dev->unit) {
+    case 0:
+        if (bus->master) {
+            fprintf(stderr, "ide: tried to assign master twice\n");
+            goto err;
+        }
+        bus->master = dev;
+        break;
+    case 1:
+        if (bus->slave) {
+            fprintf(stderr, "ide: tried to assign slave twice\n");
+            goto err;
+        }
+        bus->slave = dev;
+        break;
+    default:
+        goto err;
+    }
+    return info->init(dev);
+
+err:
+    return -1;
+}
+
+static void ide_qdev_register(IDEDeviceInfo *info)
+{
+    info->qdev.init = ide_qdev_init;
+    info->qdev.bus_info = &ide_bus_info;
+    qdev_register(&info->qdev);
+}
+
+IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive)
+{
+    DeviceState *dev;
+
+    dev = qdev_create(&bus->qbus, "ide-drive");
+    qdev_prop_set_uint32(dev, "unit", unit);
+    qdev_prop_set_drive(dev, "drive", drive);
+    qdev_init(dev);
+    return DO_UPCAST(IDEDevice, qdev, dev);
+}
+
+/* --------------------------------- */
+
+typedef struct IDEDrive {
+    IDEDevice dev;
+} IDEDrive;
+
+static int ide_drive_initfn(IDEDevice *dev)
+{
+    IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
+    ide_init_drive(bus->ifs + dev->unit, dev->dinfo);
+    return 0;
+}
+
+static IDEDeviceInfo ide_drive_info = {
+    .qdev.name  = "ide-drive",
+    .qdev.size  = sizeof(IDEDrive),
+    .init       = ide_drive_initfn,
+    .qdev.props = (Property[]) {
+        DEFINE_PROP_UINT32("unit", IDEDrive, dev.unit, -1),
+        DEFINE_PROP_DRIVE("drive", IDEDrive, dev.dinfo),
+        DEFINE_PROP_END_OF_LIST(),
+    }
+};
+
+static void ide_drive_register(void)
+{
+    ide_qdev_register(&ide_drive_info);
+}
+device_init(ide_drive_register);
-- 
1.6.2.5

[Qemu-devel] [PATCH 5/8] ide/pci: convert to qdev.

[Gerd Hoffmann]

With this patch applied ide drives (when attached to a pci adapter) can
be created via -device, like this:

  -drive if=none,id=mydisk,file=/path/to/disk.img
  -device ide-drive,drive=mydisk,bus=ide.0,unit=0

Note that creating a master on ide1 doesn't work that way.  That is a
side effect of qemu creating a cdrom automagically even if you don't
ask for it.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 Makefile.target |   10 ++-
 hw/ide/pci.c    |  205 ++++++++++++++++++++++++++++++++++---------------------
 2 files changed, 133 insertions(+), 82 deletions(-)

diff --git a/Makefile.target b/Makefile.target
index 0fe8b6a..3c71736 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -183,7 +183,8 @@ obj-y += e1000.o
 obj-y += wdt_i6300esb.o
 
 # Hardware support
-obj-i386-y = ide/core.o ide/isa.o ide/pci.o pckbd.o $(sound-obj-y) dma.o isa-bus.o
+obj-i386-y = ide/core.o ide/qdev.o ide/isa.o ide/pci.o
+obj-i386-y += pckbd.o $(sound-obj-y) dma.o isa-bus.o
 obj-i386-y += vga.o vga-pci.o vga-isa.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
@@ -192,7 +193,7 @@ obj-i386-y += device-hotplug.o pci-hotplug.o smbios.o wdt_ib700.o
 obj-i386-y += ne2000-isa.o
 
 # shared objects
-obj-ppc-y = ppc.o ide/core.o ide/isa.o ide/pci.o ide/macio.o
+obj-ppc-y = ppc.o ide/core.o ide/qdev.o ide/isa.o ide/pci.o ide/macio.o
 obj-ppc-y += vga.o vga-pci.o $(sound-obj-y) dma.o isa-bus.o openpic.o
 # PREP target
 obj-ppc-y += pckbd.o serial.o i8259.o i8254.o fdc.o mc146818rtc.o
@@ -215,7 +216,7 @@ obj-mips-y = mips_r4k.o mips_jazz.o mips_malta.o mips_mipssim.o
 obj-mips-y += mips_timer.o mips_int.o dma.o vga.o serial.o i8254.o i8259.o rc4030.o
 obj-mips-y += vga-pci.o vga-isa.o vga-isa-mm.o
 obj-mips-y += g364fb.o jazz_led.o dp8393x.o
-obj-mips-y += ide/core.o ide/isa.o ide/pci.o
+obj-mips-y += ide/core.o ide/qdev.o ide/isa.o ide/pci.o
 obj-mips-y += gt64xxx.o pckbd.o fdc.o mc146818rtc.o usb-uhci.o acpi.o ds1225y.o
 obj-mips-y += piix4.o parallel.o cirrus_vga.o isa-bus.o pcspk.o $(sound-obj-y)
 obj-mips-y += mipsnet.o ne2000-isa.o
@@ -247,7 +248,8 @@ obj-cris-y += etraxfs_ser.o
 obj-cris-y += pflash_cfi02.o
 
 ifeq ($(TARGET_ARCH), sparc64)
-obj-sparc-y = sun4u.o ide/core.o ide/pci.o isa-bus.o pckbd.o apb_pci.o
+obj-sparc-y = sun4u.o isa-bus.o pckbd.o apb_pci.o
+obj-sparc-y += ide/core.o ide/qdev.o ide/pci.o
 obj-sparc-y += vga.o vga-pci.o
 obj-sparc-y += fdc.o mc146818rtc.o serial.o
 obj-sparc-y += cirrus_vga.o parallel.o
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 607472b..aa6daf2 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -25,6 +25,7 @@
 #include <hw/hw.h>
 #include <hw/pc.h>
 #include <hw/pci.h>
+#include <hw/isa.h>
 #include "block.h"
 #include "block_int.h"
 #include "sysemu.h"
@@ -50,9 +51,10 @@
 
 typedef struct PCIIDEState {
     PCIDevice dev;
-    IDEBus bus[2];
+    IDEBus *bus[2];
     BMDMAState bmdma[2];
     int type; /* see IDE_TYPE_xxx */
+    uint32_t secondary;
 } PCIIDEState;
 
 static void cmd646_update_irq(PCIIDEState *d);
@@ -64,7 +66,7 @@ static void ide_map(PCIDevice *pci_dev, int region_num,
     IDEBus *bus;
 
     if (region_num <= 3) {
-        bus = &d->bus[(region_num >> 1)];
+        bus = d->bus[(region_num >> 1)];
         if (region_num & 1) {
             register_ioport_read(addr + 2, 1, 1, ide_status_read, bus);
             register_ioport_write(addr + 2, 1, 1, ide_cmd_write, bus);
@@ -250,9 +252,9 @@ static void bmdma_map(PCIDevice *pci_dev, int region_num,
 
     for(i = 0;i < 2; i++) {
         BMDMAState *bm = &d->bmdma[i];
-        d->bus[i].bmdma = bm;
+        d->bus[i]->bmdma = bm;
         bm->pci_dev = DO_UPCAST(PCIIDEState, dev, pci_dev);
-        bm->bus = d->bus+i;
+        bm->bus = d->bus[i];
         qemu_add_vm_change_state_handler(ide_dma_restart_cb, bm);
 
         register_ioport_write(addr, 1, 1, bmdma_cmd_writeb, bm);
@@ -292,13 +294,13 @@ static void pci_ide_save(QEMUFile* f, void *opaque)
 
     /* per IDE interface data */
     for(i = 0; i < 2; i++) {
-        idebus_save(f, &d->bus[i]);
+        idebus_save(f, d->bus[i]);
     }
 
     /* per IDE drive data */
     for(i = 0; i < 2; i++) {
-        ide_save(f, &d->bus[i].ifs[0]);
-        ide_save(f, &d->bus[i].ifs[1]);
+        ide_save(f, &d->bus[i]->ifs[0]);
+        ide_save(f, &d->bus[i]->ifs[1]);
     }
 }
 
@@ -328,17 +330,31 @@ static int pci_ide_load(QEMUFile* f, void *opaque, int version_id)
 
     /* per IDE interface data */
     for(i = 0; i < 2; i++) {
-        idebus_load(f, &d->bus[i], version_id);
+        idebus_load(f, d->bus[i], version_id);
     }
 
     /* per IDE drive data */
     for(i = 0; i < 2; i++) {
-        ide_load(f, &d->bus[i].ifs[0], version_id);
-        ide_load(f, &d->bus[i].ifs[1], version_id);
+        ide_load(f, &d->bus[i]->ifs[0], version_id);
+        ide_load(f, &d->bus[i]->ifs[1], version_id);
     }
     return 0;
 }
 
+static void pci_ide_create_devs(PCIDevice *dev, DriveInfo **hd_table)
+{
+    PCIIDEState *d = DO_UPCAST(PCIIDEState, dev, dev);
+    static const int bus[4]  = { 0, 0, 1, 1 };
+    static const int unit[4] = { 0, 1, 0, 1 };
+    int i;
+
+    for (i = 0; i < 4; i++) {
+        if (hd_table[i] == NULL)
+            continue;
+        ide_create_drive(d->bus[bus[i]], unit[i], hd_table[i]);
+    }
+}
+
 /* XXX: call it also when the MRDMODE is changed from the PCI config
    registers */
 static void cmd646_update_irq(PCIIDEState *d)
@@ -375,19 +391,13 @@ static void cmd646_reset(void *opaque)
 }
 
 /* CMD646 PCI IDE controller */
-void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
-                         int secondary_ide_enabled)
+static int pci_cmd646_ide_initfn(PCIDevice *dev)
 {
-    PCIIDEState *d;
-    uint8_t *pci_conf;
+    PCIIDEState *d = DO_UPCAST(PCIIDEState, dev, dev);
+    uint8_t *pci_conf = d->dev.config;
     qemu_irq *irq;
 
-    d = (PCIIDEState *)pci_register_device(bus, "CMD646 IDE",
-                                           sizeof(PCIIDEState),
-                                           -1,
-                                           NULL, NULL);
     d->type = IDE_TYPE_CMD646;
-    pci_conf = d->dev.config;
     pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_CMD);
     pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_CMD_646);
 
@@ -398,31 +408,47 @@ void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
     pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
 
     pci_conf[0x51] = 0x04; // enable IDE0
-    if (secondary_ide_enabled) {
+    if (d->secondary) {
         /* XXX: if not enabled, really disable the seconday IDE controller */
         pci_conf[0x51] |= 0x08; /* enable IDE1 */
     }
+    pci_conf[0x3d] = 0x01; // interrupt on pin 1
 
     pci_register_bar((PCIDevice *)d, 0, 0x8,
-                           PCI_ADDRESS_SPACE_IO, ide_map);
+                     PCI_ADDRESS_SPACE_IO, ide_map);
     pci_register_bar((PCIDevice *)d, 1, 0x4,
-                           PCI_ADDRESS_SPACE_IO, ide_map);
+                     PCI_ADDRESS_SPACE_IO, ide_map);
     pci_register_bar((PCIDevice *)d, 2, 0x8,
-                           PCI_ADDRESS_SPACE_IO, ide_map);
+                     PCI_ADDRESS_SPACE_IO, ide_map);
     pci_register_bar((PCIDevice *)d, 3, 0x4,
-                           PCI_ADDRESS_SPACE_IO, ide_map);
+                     PCI_ADDRESS_SPACE_IO, ide_map);
     pci_register_bar((PCIDevice *)d, 4, 0x10,
-                           PCI_ADDRESS_SPACE_IO, bmdma_map);
+                     PCI_ADDRESS_SPACE_IO, bmdma_map);
 
-    pci_conf[0x3d] = 0x01; // interrupt on pin 1
+    register_savevm("ide", 0, 3, pci_ide_save, pci_ide_load, d);
 
-    irq = qemu_allocate_irqs(cmd646_set_irq, d, 2);
-    ide_init2(&d->bus[0], hd_table[0], hd_table[1], irq[0]);
-    ide_init2(&d->bus[1], hd_table[2], hd_table[3], irq[1]);
+    d->bus[0] = ide_bus_new(&d->dev.qdev);
+    d->bus[1] = ide_bus_new(&d->dev.qdev);
 
-    register_savevm("ide", 0, 3, pci_ide_save, pci_ide_load, d);
     qemu_register_reset(cmd646_reset, d);
     cmd646_reset(d);
+
+    irq = qemu_allocate_irqs(cmd646_set_irq, d, 2);
+    ide_init2(d->bus[0], NULL, NULL, irq[0]);
+    ide_init2(d->bus[1], NULL, NULL, irq[1]);
+    return 0;
+}
+
+void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
+                         int secondary_ide_enabled)
+{
+    PCIDevice *dev;
+
+    dev = pci_create_noinit(bus, -1, "CMD646 IDE");
+    qdev_prop_set_uint32(&dev->qdev, "secondary", secondary_ide_enabled);
+    qdev_init(&dev->qdev);
+
+    pci_ide_create_devs(dev, hd_table);
 }
 
 static void piix3_reset(void *opaque)
@@ -441,23 +467,10 @@ static void piix3_reset(void *opaque)
     pci_conf[0x20] = 0x01; /* BMIBA: 20-23h */
 }
 
-/* hd_table must contain 4 block drivers */
-/* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
-void pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
+static int pci_piix_ide_initfn(PCIIDEState *d)
 {
-    PCIIDEState *d;
-    uint8_t *pci_conf;
-
-    /* register a function 1 of PIIX3 */
-    d = (PCIIDEState *)pci_register_device(bus, "PIIX3 IDE",
-                                           sizeof(PCIIDEState),
-                                           devfn,
-                                           NULL, NULL);
-    d->type = IDE_TYPE_PIIX3;
+    uint8_t *pci_conf = d->dev.config;
 
-    pci_conf = d->dev.config;
-    pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
-    pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82371SB_1);
     pci_conf[0x09] = 0x80; // legacy ATA mode
     pci_config_set_class(pci_conf, PCI_CLASS_STORAGE_IDE);
     pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
@@ -466,48 +479,84 @@ void pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
     piix3_reset(d);
 
     pci_register_bar((PCIDevice *)d, 4, 0x10,
-                           PCI_ADDRESS_SPACE_IO, bmdma_map);
-
-    ide_init2(&d->bus[0], hd_table[0], hd_table[1], isa_reserve_irq(14));
-    ide_init2(&d->bus[1], hd_table[2], hd_table[3], isa_reserve_irq(15));
-    ide_init_ioport(&d->bus[0], 0x1f0, 0x3f6);
-    ide_init_ioport(&d->bus[1], 0x170, 0x376);
+                     PCI_ADDRESS_SPACE_IO, bmdma_map);
 
     register_savevm("ide", 0, 3, pci_ide_save, pci_ide_load, d);
+
+    d->bus[0] = ide_bus_new(&d->dev.qdev);
+    d->bus[1] = ide_bus_new(&d->dev.qdev);
+    ide_init_ioport(d->bus[0], 0x1f0, 0x3f6);
+    ide_init_ioport(d->bus[1], 0x170, 0x376);
+
+    ide_init2(d->bus[0], NULL, NULL, isa_reserve_irq(14));
+    ide_init2(d->bus[1], NULL, NULL, isa_reserve_irq(15));
+    return 0;
 }
 
-/* hd_table must contain 4 block drivers */
-/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
-void pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
+static int pci_piix3_ide_initfn(PCIDevice *dev)
 {
-    PCIIDEState *d;
-    uint8_t *pci_conf;
-
-    /* register a function 1 of PIIX4 */
-    d = (PCIIDEState *)pci_register_device(bus, "PIIX4 IDE",
-                                           sizeof(PCIIDEState),
-                                           devfn,
-                                           NULL, NULL);
-    d->type = IDE_TYPE_PIIX4;
+    PCIIDEState *d = DO_UPCAST(PCIIDEState, dev, dev);
 
-    pci_conf = d->dev.config;
-    pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
-    pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82371AB);
-    pci_conf[0x09] = 0x80; // legacy ATA mode
-    pci_config_set_class(pci_conf, PCI_CLASS_STORAGE_IDE);
-    pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
+    d->type = IDE_TYPE_PIIX3;
+    pci_config_set_vendor_id(d->dev.config, PCI_VENDOR_ID_INTEL);
+    pci_config_set_device_id(d->dev.config, PCI_DEVICE_ID_INTEL_82371SB_1);
+    return pci_piix_ide_initfn(d);
+}
 
-    qemu_register_reset(piix3_reset, d);
-    piix3_reset(d);
+static int pci_piix4_ide_initfn(PCIDevice *dev)
+{
+    PCIIDEState *d = DO_UPCAST(PCIIDEState, dev, dev);
 
-    pci_register_bar((PCIDevice *)d, 4, 0x10,
-                           PCI_ADDRESS_SPACE_IO, bmdma_map);
+    d->type = IDE_TYPE_PIIX4;
+    pci_config_set_vendor_id(d->dev.config, PCI_VENDOR_ID_INTEL);
+    pci_config_set_device_id(d->dev.config, PCI_DEVICE_ID_INTEL_82371AB);
+    return pci_piix_ide_initfn(d);
+}
 
-    ide_init2(&d->bus[0], hd_table[0], hd_table[1], isa_reserve_irq(14));
-    ide_init2(&d->bus[1], hd_table[2], hd_table[3], isa_reserve_irq(15));
-    ide_init_ioport(&d->bus[0], 0x1f0, 0x3f6);
-    ide_init_ioport(&d->bus[1], 0x170, 0x376);
+/* hd_table must contain 4 block drivers */
+/* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
+void pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
+{
+    PCIDevice *dev;
 
-    register_savevm("ide", 0, 3, pci_ide_save, pci_ide_load, d);
+    dev = pci_create_simple(bus, devfn, "PIIX3 IDE");
+    pci_ide_create_devs(dev, hd_table);
+}
+
+/* hd_table must contain 4 block drivers */
+/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
+void pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
+{
+    PCIDevice *dev;
+
+    dev = pci_create_simple(bus, devfn, "PIIX4 IDE");
+    pci_ide_create_devs(dev, hd_table);
 }
 
+static PCIDeviceInfo piix_ide_info[] = {
+    {
+        .qdev.name    = "PIIX3 IDE",
+        .qdev.size    = sizeof(PCIIDEState),
+        .init         = pci_piix3_ide_initfn,
+    },{
+        .qdev.name    = "PIIX4 IDE",
+        .qdev.size    = sizeof(PCIIDEState),
+        .init         = pci_piix4_ide_initfn,
+    },{
+        .qdev.name    = "CMD646 IDE",
+        .qdev.size    = sizeof(PCIIDEState),
+        .init         = pci_cmd646_ide_initfn,
+        .qdev.props   = (Property[]) {
+            DEFINE_PROP_UINT32("secondary", PCIIDEState, secondary, 0),
+            DEFINE_PROP_END_OF_LIST(),
+        },
+    },{
+        /* end of list */
+    }
+};
+
+static void piix_ide_register(void)
+{
+    pci_qdev_register_many(piix_ide_info);
+}
+device_init(piix_ide_register);
-- 
1.6.2.5

[Qemu-devel] [PATCH 6/8] ide/isa: convert to qdev.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/ide.h      |    4 +-
 hw/ide/isa.c  |   61 +++++++++++++++++++++++++++++++++++++++++++++++++-------
 hw/mips_r4k.c |    2 +-
 hw/pc.c       |    3 +-
 hw/ppc_prep.c |    2 +-
 5 files changed, 58 insertions(+), 14 deletions(-)

diff --git a/hw/ide.h b/hw/ide.h
index e0a508b..0e7d540 100644
--- a/hw/ide.h
+++ b/hw/ide.h
@@ -4,8 +4,8 @@
 #include "qdev.h"
 
 /* ide-isa.c */
-void isa_ide_init(int iobase, int iobase2, qemu_irq irq,
-                  DriveInfo *hd0, DriveInfo *hd1);
+int isa_ide_init(int iobase, int iobase2, int isairq,
+                 DriveInfo *hd0, DriveInfo *hd1);
 
 /* ide-pci.c */
 void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
diff --git a/hw/ide/isa.c b/hw/ide/isa.c
index ec2d6fb..d2fe0c0 100644
--- a/hw/ide/isa.c
+++ b/hw/ide/isa.c
@@ -24,6 +24,7 @@
  */
 #include <hw/hw.h>
 #include <hw/pc.h>
+#include <hw/isa.h>
 #include "block.h"
 #include "block_int.h"
 #include "sysemu.h"
@@ -35,7 +36,12 @@
 /* ISA IDE definitions */
 
 typedef struct ISAIDEState {
-    IDEBus *bus;
+    ISADevice dev;
+    IDEBus    *bus;
+    uint32_t  iobase;
+    uint32_t  iobase2;
+    uint32_t  isairq;
+    qemu_irq  irq;
 } ISAIDEState;
 
 static void isa_ide_save(QEMUFile* f, void *opaque)
@@ -57,15 +63,54 @@ static int isa_ide_load(QEMUFile* f, void *opaque, int version_id)
     return 0;
 }
 
-void isa_ide_init(int iobase, int iobase2, qemu_irq irq,
-                  DriveInfo *hd0, DriveInfo *hd1)
+static int isa_ide_initfn(ISADevice *dev)
 {
+    ISAIDEState *s = DO_UPCAST(ISAIDEState, dev, dev);
+
+    s->bus = ide_bus_new(&s->dev.qdev);
+    ide_init_ioport(s->bus, s->iobase, s->iobase2);
+    isa_init_irq(dev, &s->irq, s->isairq);
+    ide_init2(s->bus, NULL, NULL, s->irq);
+    register_savevm("isa-ide", 0, 3, isa_ide_save, isa_ide_load, s);
+    return 0;
+};
+
+int isa_ide_init(int iobase, int iobase2, int isairq,
+                 DriveInfo *hd0, DriveInfo *hd1)
+{
+    ISADevice *dev;
     ISAIDEState *s;
 
-    s = qemu_mallocz(sizeof(*s));
-    s->bus = qemu_mallocz(sizeof(IDEBus));
+    dev = isa_create("isa-ide");
+    qdev_prop_set_uint32(&dev->qdev, "iobase",  iobase);
+    qdev_prop_set_uint32(&dev->qdev, "iobase2", iobase2);
+    qdev_prop_set_uint32(&dev->qdev, "irq",     isairq);
+    if (qdev_init(&dev->qdev) != 0)
+        return -1;
 
-    ide_init2(s->bus, hd0, hd1, irq);
-    ide_init_ioport(s->bus, iobase, iobase2);
-    register_savevm("isa-ide", 0, 3, isa_ide_save, isa_ide_load, s);
+    s = DO_UPCAST(ISAIDEState, dev, dev);
+    if (hd0)
+        ide_create_drive(s->bus, 0, hd0);
+    if (hd1)
+        ide_create_drive(s->bus, 1, hd1);
+    return 0;
+}
+
+static ISADeviceInfo isa_ide_info = {
+    .qdev.name  = "isa-ide",
+    .qdev.size  = sizeof(ISAIDEState),
+    .init       = isa_ide_initfn,
+    .qdev.props = (Property[]) {
+        DEFINE_PROP_HEX32("iobase",  ISAIDEState, iobase,  0x1f0),
+        DEFINE_PROP_HEX32("iobase2", ISAIDEState, iobase2, 0x3f6),
+        DEFINE_PROP_UINT32("irq",    ISAIDEState, isairq,  14),
+        DEFINE_PROP_END_OF_LIST(),
+    },
+};
+
+static void isa_ide_register_devices(void)
+{
+    isa_qdev_register(&isa_ide_info);
 }
+
+device_init(isa_ide_register_devices)
diff --git a/hw/mips_r4k.c b/hw/mips_r4k.c
index d801417..fcc7fed 100644
--- a/hw/mips_r4k.c
+++ b/hw/mips_r4k.c
@@ -274,7 +274,7 @@ void mips_r4k_init (ram_addr_t ram_size,
     }
 
     for(i = 0; i < MAX_IDE_BUS; i++)
-        isa_ide_init(ide_iobase[i], ide_iobase2[i], i8259[ide_irq[i]],
+        isa_ide_init(ide_iobase[i], ide_iobase2[i], ide_irq[i],
                      hd[MAX_IDE_DEVS * i],
 		     hd[MAX_IDE_DEVS * i + 1]);
 
diff --git a/hw/pc.c b/hw/pc.c
index d96d756..96907b2 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -1367,8 +1367,7 @@ static void pc_init1(ram_addr_t ram_size,
         pci_piix3_ide_init(pci_bus, hd, piix3_devfn + 1);
     } else {
         for(i = 0; i < MAX_IDE_BUS; i++) {
-            isa_ide_init(ide_iobase[i], ide_iobase2[i],
-                         isa_reserve_irq(ide_irq[i]),
+            isa_ide_init(ide_iobase[i], ide_iobase2[i], ide_irq[i],
 	                 hd[MAX_IDE_DEVS * i], hd[MAX_IDE_DEVS * i + 1]);
         }
     }
diff --git a/hw/ppc_prep.c b/hw/ppc_prep.c
index 1930146..6001c86 100644
--- a/hw/ppc_prep.c
+++ b/hw/ppc_prep.c
@@ -707,7 +707,7 @@ static void ppc_prep_init (ram_addr_t ram_size,
     }
 
     for(i = 0; i < MAX_IDE_BUS; i++) {
-        isa_ide_init(ide_iobase[i], ide_iobase2[i], i8259[ide_irq[i]],
+        isa_ide_init(ide_iobase[i], ide_iobase2[i], ide_irq[i],
                      hd[2 * i],
 		     hd[2 * i + 1]);
     }
-- 
1.6.2.5

[Qemu-devel] [PATCH 7/8] isa: refine irq reservations

[Gerd Hoffmann]

There are a few cases where IRQ sharing on the ISA bus is used and
possible.  In general only devices of the same kind can do that.
A few use cases:

  * serial lines 1+3 share irq 4
  * serial lines 2+4 share irq 3
  * parallel ports share irq 7
  * ppc/prep: ide ports share irq 13

This patch refines the irq reservation mechanism for the isa bus to
handle those cases.  It keeps track of the driver which owns the IRQ in
question and allows irq sharing for devices handled by the same driver.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/isa-bus.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/hw/isa-bus.c b/hw/isa-bus.c
index 4ecc0f8..1d2ca90 100644
--- a/hw/isa-bus.c
+++ b/hw/isa-bus.c
@@ -26,6 +26,7 @@ struct ISABus {
     BusState qbus;
     qemu_irq *irqs;
     uint32_t assigned;
+    DeviceInfo *irq_owner[16];
 };
 static ISABus *isabus;
 
@@ -71,7 +72,9 @@ qemu_irq isa_reserve_irq(int isairq)
         exit(1);
     }
     if (isabus->assigned & (1 << isairq)) {
-        fprintf(stderr, "isa irq %d already assigned\n", isairq);
+        DeviceInfo *owner = isabus->irq_owner[isairq];
+        fprintf(stderr, "isa irq %d already assigned (%s)\n",
+                isairq, owner ? owner->name : "unknown");
         exit(1);
     }
     isabus->assigned |= (1 << isairq);
@@ -82,10 +85,17 @@ void isa_init_irq(ISADevice *dev, qemu_irq *p, int isairq)
 {
     assert(dev->nirqs < ARRAY_SIZE(dev->isairq));
     if (isabus->assigned & (1 << isairq)) {
-        fprintf(stderr, "isa irq %d already assigned\n", isairq);
-        exit(1);
+        DeviceInfo *owner = isabus->irq_owner[isairq];
+        if (owner == dev->qdev.info) {
+            /* irq sharing is ok in case the same driver handles both */;
+        } else {
+            fprintf(stderr, "isa irq %d already assigned (%s)\n",
+                    isairq, owner ? owner->name : "unknown");
+            exit(1);
+        }
     }
     isabus->assigned |= (1 << isairq);
+    isabus->irq_owner[isairq] = dev->qdev.info;
     dev->isairq[dev->nirqs] = isairq;
     *p = isabus->irqs[isairq];
     dev->nirqs++;
-- 
1.6.2.5

[Qemu-devel] [PATCH 8/8] unbreak ppc/prep

[Gerd Hoffmann]

Changes:
  * added isa bus, hooked up to the system bus. Not sure this is correct,
    but 'info pci' lists lists no pci-isa bridge in the machine ...).
  * switches the default cpu to one which actually works.

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

diff --git a/hw/ppc_prep.c b/hw/ppc_prep.c
index 6001c86..5392072 100644
--- a/hw/ppc_prep.c
+++ b/hw/ppc_prep.c
@@ -572,7 +572,7 @@ static void ppc_prep_init (ram_addr_t ram_size,
 
     /* init CPUs */
     if (cpu_model == NULL)
-        cpu_model = "default";
+        cpu_model = "602";
     for (i = 0; i < smp_cpus; i++) {
         env = cpu_init(cpu_model);
         if (!env) {
@@ -670,6 +670,9 @@ static void ppc_prep_init (ram_addr_t ram_size,
     }
     i8259 = i8259_init(first_cpu->irq_inputs[PPC6xx_INPUT_INT]);
     pci_bus = pci_prep_init(i8259);
+    /* Hmm, prep has no pci-isa bridge ??? */
+    isa_bus_new(NULL);
+    isa_bus_irqs(i8259);
     //    pci_bus = i440fx_init();
     /* Register 8 MB of ISA IO space (needed for non-contiguous map) */
     PPC_io_memory = cpu_register_io_memory(PPC_prep_io_read,
-- 
1.6.2.5

[Qemu-devel] Re: [PATCH 4/8] ide/qdev: add ide bus.

[Gerd Hoffmann]

>>   struct IDEBus {
>>       BusState qbus;
>> +    IDEDevice *master;
>> +    IDEDevice *slave;
>
> Can this ones be embedded, not pointers, please.

No.

Also note that vmstate doesn't has to follow these pointers, so you have 
no reason in the first place ;)

cheers,
   Gerd

[Qemu-devel] Re: [PATCH 5/8] ide/pci: convert to qdev.

[Gerd Hoffmann]

On 09/11/09 15:56, Juan Quintela wrote:
> Gerd Hoffmann<kraxel@redhat.com>  wrote:
>> With this patch applied ide drives (when attached to a pci adapter) can
>> be created via -device, like this:
>> -    IDEBus bus[2];
>> +    IDEBus *bus[2];
>
> You change a nice static array for a pointer to one array.

I can't spot a pointer here.

> VMState is happier with embedded arrays and not pointers to arrays.

There also is no pointer to an array.  It is an embedded array holding 
pointers.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 1/8] qdev/pci: add pci_create_noinit()

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> Like pci_create_simple() but doesn't call qdev_init(), so one can
> set properties before initializing the device.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/pci.c |   11 ++++++++---
>  hw/pci.h |    1 +
>  2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/hw/pci.c b/hw/pci.c
> index c12b0be..64d70ed 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -923,15 +923,20 @@ void pci_qdev_register_many(PCIDeviceInfo *info)
>      }
>  }
>  
> -PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name)
> +PCIDevice *pci_create_noinit(PCIBus *bus, int devfn, const char *name)
>  {
>      DeviceState *dev;
>  
>      dev = qdev_create(&bus->qbus, name);
>      qdev_prop_set_uint32(dev, "addr", devfn);
> -    qdev_init(dev);
> +    return DO_UPCAST(PCIDevice, qdev, dev);
> +}

Okay, this is qdev_create() specialized for PCI.  What about calling it
just pci_create()?

>  
> -    return (PCIDevice *)dev;
> +PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name)
> +{
> +    PCIDevice *dev = pci_create_noinit(bus, devfn, name);
> +    qdev_init(&dev->qdev);
> +    return dev;
>  }
>  
>  static int pci_find_space(PCIDevice *pdev, uint8_t size)
> diff --git a/hw/pci.h b/hw/pci.h
> index 6196b6a..e7bf33a 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -328,6 +328,7 @@ void pci_qdev_register(PCIDeviceInfo *info);
>  void pci_qdev_register_many(PCIDeviceInfo *info);
>  
>  PCIDevice *pci_create(const char *name, const char *devaddr);
> +PCIDevice *pci_create_noinit(PCIBus *bus, int devfn, const char *name);
>  PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name);
>  
>  /* lsi53c895a.c */

Re: [Qemu-devel] [PATCH 3/8] split away drive init from ide_init2()

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> This allows the ide bus being initialized without drives attached
> and the drives being attached and initialization later on as
> separate step.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/ide/core.c     |   71 +++++++++++++++++++++++++++++------------------------
>  hw/ide/internal.h |    1 +
>  2 files changed, 40 insertions(+), 32 deletions(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index fe5bd17..bac2d5e 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -2505,51 +2505,58 @@ void ide_reset(IDEState *s)
>      s->media_changed = 0;
>  }
>  
> +void ide_init_drive(IDEState *s, DriveInfo *dinfo)
> +{
> +    int cylinders, heads, secs;
> +    uint64_t nb_sectors;
> +
> +    if (dinfo && dinfo->bdrv) {
> +        s->bs = dinfo->bdrv;

Before, this assignement was executed even when !dinfo->bdrv.  Can this
happen?  Does it matter?

> +        bdrv_get_geometry(s->bs, &nb_sectors);
> +        bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
> +        s->cylinders = cylinders;
> +        s->heads = heads;
> +        s->sectors = secs;
> +        s->nb_sectors = nb_sectors;
> +        /* The SMART values should be preserved across power cycles
> +           but they aren't.  */
> +        s->smart_enabled = 1;
> +        s->smart_autosave = 1;
> +        s->smart_errors = 0;
> +        s->smart_selftest_count = 0;
> +        if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM) {
> +            s->is_cdrom = 1;
> +            bdrv_set_change_cb(s->bs, cdrom_change_cb, s);
> +        }
> +        strncpy(s->drive_serial_str, drive_get_serial(s->bs),
> +                sizeof(s->drive_serial_str));

Same for this assignment.

> +    }
> +    if (strlen(s->drive_serial_str) == 0)
> +        snprintf(s->drive_serial_str, sizeof(s->drive_serial_str),
> +                 "QM%05d", s->drive_serial);
> +    ide_reset(s);
> +}
> +
>  void ide_init2(IDEBus *bus, DriveInfo *hd0, DriveInfo *hd1,
>                 qemu_irq irq)
>  {
>      IDEState *s;
>      static int drive_serial = 1;
> -    int i, cylinders, heads, secs;
> -    uint64_t nb_sectors;
> +    int i;
>  
>      for(i = 0; i < 2; i++) {
>          s = bus->ifs + i;
>          s->bus = bus;
>          s->unit = i;
> -        if (i == 0 && hd0)
> -            s->bs = hd0->bdrv;
> -        if (i == 1 && hd1)
> -            s->bs = hd1->bdrv;
> -        s->io_buffer = qemu_blockalign(s->bs, IDE_DMA_BUF_SECTORS*512 + 4);
> -        if (s->bs) {
> -            bdrv_get_geometry(s->bs, &nb_sectors);
> -            bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
> -            s->cylinders = cylinders;
> -            s->heads = heads;
> -            s->sectors = secs;
> -            s->nb_sectors = nb_sectors;
> -	    /* The SMART values should be preserved across power cycles
> -	       but they aren't.  */
> -	    s->smart_enabled = 1;
> -	    s->smart_autosave = 1;
> -	    s->smart_errors = 0;
> -	    s->smart_selftest_count = 0;
> -	    s->smart_selftest_data = qemu_blockalign(s->bs, 512);
> -            if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM) {
> -                s->is_cdrom = 1;
> -		bdrv_set_change_cb(s->bs, cdrom_change_cb, s);
> -            }
> -        }
>          s->drive_serial = drive_serial++;
> -        strncpy(s->drive_serial_str, drive_get_serial(s->bs),
> -                sizeof(s->drive_serial_str));
> -        if (strlen(s->drive_serial_str) == 0)
> -            snprintf(s->drive_serial_str, sizeof(s->drive_serial_str),
> -                    "QM%05d", s->drive_serial);
> +        s->io_buffer = qemu_blockalign(s->bs, IDE_DMA_BUF_SECTORS*512 + 4);
> +        s->smart_selftest_data = qemu_blockalign(s->bs, 512);
>          s->sector_write_timer = qemu_new_timer(vm_clock,
>                                                 ide_sector_write_timer_cb, s);
> -        ide_reset(s);
> +        if (i == 0)
> +            ide_init_drive(s, hd0);
> +        if (i == 1)
> +            ide_init_drive(s, hd1);

I'd write this as

        ide_init_drive(i == 0 ? hd0 : hd1);

because I find it clearer, but it's a matter of taste.

>      }
>      bus->irq = irq;
>  }
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index b9a7c72..62aef1c 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -527,6 +527,7 @@ uint32_t ide_data_readw(void *opaque, uint32_t addr);
>  void ide_data_writel(void *opaque, uint32_t addr, uint32_t val);
>  uint32_t ide_data_readl(void *opaque, uint32_t addr);
>  
> +void ide_init_drive(IDEState *s, DriveInfo *dinfo);
>  void ide_init2(IDEBus *bus, DriveInfo *hd0, DriveInfo *hd1,
>                 qemu_irq irq);
>  void ide_init_ioport(IDEBus *bus, int iobase, int iobase2);

Re: [Qemu-devel] [PATCH 1/8] qdev/pci: add pci_create_noinit()

[Gerd Hoffmann]

>> -PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name)
>> +PCIDevice *pci_create_noinit(PCIBus *bus, int devfn, const char *name)
>>   {
>>       DeviceState *dev;
>>
>>       dev = qdev_create(&bus->qbus, name);
>>       qdev_prop_set_uint32(dev, "addr", devfn);
>> -    qdev_init(dev);
>> +    return DO_UPCAST(PCIDevice, qdev, dev);
>> +}
>
> Okay, this is qdev_create() specialized for PCI.  What about calling it
> just pci_create()?

pci_create() should go away once it has no more users.  It doesn't 
accept a pcibus parameter as it should.

cheers,
   Gerd

[Qemu-devel] Re: [PATCH 0/8] ide: convert to qdev.

[Juan Quintela]

Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
> Here is a patch series which starts the conversion of ide to qdev.
> It brings the core infrastructure and converts pci+isa ide adapters
> to qdev.  Also some minor preparatory patches and bugfixes.

I like the cleanup.  My reservations are with the qdev allocation of
memory,  not with this series.

Later, Juan.

Re: [Qemu-devel] [PATCH 3/8] split away drive init from ide_init2()

[Gerd Hoffmann]

>> +void ide_init_drive(IDEState *s, DriveInfo *dinfo)
>> +{
>> +    int cylinders, heads, secs;
>> +    uint64_t nb_sectors;
>> +
>> +    if (dinfo&&  dinfo->bdrv) {
>> +        s->bs = dinfo->bdrv;
>
> Before, this assignement was executed even when !dinfo->bdrv.  Can this
> happen?

Running this once with dinfo->bdrv = something, then with dinfo->bdrv = 
NULL (which is the only case where this matters) can't happen.  You 
can't hot-unplut ide drives ;)

The code used to be called once for initialization.  Now it might be 
called twice for ide busses converted to qdev: Once with dinfo = NULL 
(when setting up the bus) and once with dinfo != NULL (when attaching 
the drive).

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 4/8] ide/qdev: add ide bus.

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/ide/internal.h |   24 ++++++++++-
>  hw/ide/qdev.c     |  123 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 146 insertions(+), 1 deletions(-)
>  create mode 100644 hw/ide/qdev.c
>
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index 62aef1c..9df759d 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -15,6 +15,8 @@
>  #define USE_DMA_CDROM
>  
>  typedef struct IDEBus IDEBus;
> +typedef struct IDEDevice IDEDevice;
> +typedef struct IDEDeviceInfo IDEDeviceInfo;
>  typedef struct IDEState IDEState;
>  typedef struct BMDMAState BMDMAState;
>  
> @@ -440,6 +442,8 @@ struct IDEState {
>  
>  struct IDEBus {
>      BusState qbus;
> +    IDEDevice *master;
> +    IDEDevice *slave;
>      BMDMAState *bmdma;
>      IDEState ifs[2];
>      uint8_t unit;

master corresponds to ifs[0], and slave to ifs[1].  Document?

> @@ -447,6 +451,20 @@ struct IDEBus {
>      qemu_irq irq;
>  };
>  
> +struct IDEDevice {
> +    DeviceState qdev;
> +    uint32_t unit;
> +    DriveInfo *dinfo;
> +};
> +
> +typedef int (*ide_qdev_initfn)(IDEDevice *dev);
> +struct IDEDeviceInfo {
> +    DeviceInfo qdev;
> +    ide_qdev_initfn init;
> +    uint32_t unit;
> +    DriveInfo *drive;
> +};
> +
>  #define BM_STATUS_DMAING 0x01
>  #define BM_STATUS_ERROR  0x02
>  #define BM_STATUS_INT    0x04
> @@ -500,7 +518,7 @@ static inline void ide_set_irq(IDEBus *bus)
>      }
>  }
>  
> -/* ide.c */
> +/* hw/ide/core.c */
>  void ide_save(QEMUFile* f, IDEState *s);
>  void ide_load(QEMUFile* f, IDEState *s, int version_id);
>  void idebus_save(QEMUFile* f, IDEBus *bus);
> @@ -532,4 +550,8 @@ void ide_init2(IDEBus *bus, DriveInfo *hd0, DriveInfo *hd1,
>                 qemu_irq irq);
>  void ide_init_ioport(IDEBus *bus, int iobase, int iobase2);
>  
> +/* hw/ide/qdev.c */
> +IDEBus *ide_bus_new(DeviceState *dev);
> +IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive);
> +
>  #endif /* HW_IDE_INTERNAL_H */
> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
> new file mode 100644
> index 0000000..6026c28
> --- /dev/null
> +++ b/hw/ide/qdev.c
> @@ -0,0 +1,123 @@
> +/*
> + * ide bus support for qdev.
> + *
> + * Copyright (c) 2009 Gerd Hoffmann <kraxel@redhat.com>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + */
> +#include <hw/hw.h>
> +#include "sysemu.h"
> +#include "dma.h"
> +
> +#include <hw/ide/internal.h>
> +
> +/* --------------------------------- */
> +
> +static struct BusInfo ide_bus_info = {
> +    .name  = "IDE",
> +    .size  = sizeof(IDEBus),
> +};
> +
> +IDEBus *ide_bus_new(DeviceState *dev)
> +{
> +    IDEBus *idebus;
> +
> +    idebus = FROM_QBUS(IDEBus, qbus_create(&ide_bus_info, dev, NULL));
> +    return idebus;
> +}
> +
> +static int ide_qdev_init(DeviceState *qdev, DeviceInfo *base)
> +{
> +    IDEDevice *dev = DO_UPCAST(IDEDevice, qdev, qdev);
> +    IDEDeviceInfo *info = DO_UPCAST(IDEDeviceInfo, qdev, base);
> +    IDEBus *bus = DO_UPCAST(IDEBus, qbus, qdev->parent_bus);
> +
> +    if (!dev->dinfo) {
> +        fprintf(stderr, "%s: no drive specified\n", qdev->info->name);
> +        goto err;
> +    }
> +    if (dev->unit == -1) {
> +        dev->unit = bus->master ? 1 : 0;
> +    }
> +    switch (dev->unit) {
> +    case 0:
> +        if (bus->master) {
> +            fprintf(stderr, "ide: tried to assign master twice\n");
> +            goto err;
> +        }
> +        bus->master = dev;
> +        break;
> +    case 1:
> +        if (bus->slave) {
> +            fprintf(stderr, "ide: tried to assign slave twice\n");
> +            goto err;
> +        }
> +        bus->slave = dev;
> +        break;
> +    default:
> +        goto err;
> +    }
> +    return info->init(dev);

Is always ide_drive_initfn() so far, but more callbacks come in later in
the series.

> +
> +err:
> +    return -1;
> +}
> +
> +static void ide_qdev_register(IDEDeviceInfo *info)
> +{
> +    info->qdev.init = ide_qdev_init;
> +    info->qdev.bus_info = &ide_bus_info;
> +    qdev_register(&info->qdev);
> +}
> +
> +IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive)
> +{
> +    DeviceState *dev;
> +
> +    dev = qdev_create(&bus->qbus, "ide-drive");
> +    qdev_prop_set_uint32(dev, "unit", unit);
> +    qdev_prop_set_drive(dev, "drive", drive);
> +    qdev_init(dev);

Callback dev->info is ide_qdev_init(), which can fail, so qdev_init()
can fail, can't it?

> +    return DO_UPCAST(IDEDevice, qdev, dev);
> +}
> +
> +/* --------------------------------- */
> +
> +typedef struct IDEDrive {
> +    IDEDevice dev;
> +} IDEDrive;
> +
> +static int ide_drive_initfn(IDEDevice *dev)
> +{
> +    IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
> +    ide_init_drive(bus->ifs + dev->unit, dev->dinfo);
> +    return 0;
> +}
> +
> +static IDEDeviceInfo ide_drive_info = {
> +    .qdev.name  = "ide-drive",
> +    .qdev.size  = sizeof(IDEDrive),
> +    .init       = ide_drive_initfn,
> +    .qdev.props = (Property[]) {
> +        DEFINE_PROP_UINT32("unit", IDEDrive, dev.unit, -1),
> +        DEFINE_PROP_DRIVE("drive", IDEDrive, dev.dinfo),
> +        DEFINE_PROP_END_OF_LIST(),
> +    }
> +};
> +
> +static void ide_drive_register(void)
> +{
> +    ide_qdev_register(&ide_drive_info);
> +}
> +device_init(ide_drive_register);

Re: [Qemu-devel] [PATCH 1/8] qdev/pci: add pci_create_noinit()

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

>>> -PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name)
>>> +PCIDevice *pci_create_noinit(PCIBus *bus, int devfn, const char *name)
>>>   {
>>>       DeviceState *dev;
>>>
>>>       dev = qdev_create(&bus->qbus, name);
>>>       qdev_prop_set_uint32(dev, "addr", devfn);
>>> -    qdev_init(dev);
>>> +    return DO_UPCAST(PCIDevice, qdev, dev);
>>> +}
>>
>> Okay, this is qdev_create() specialized for PCI.  What about calling it
>> just pci_create()?
>
> pci_create() should go away once it has no more users.  It doesn't
> accept a pcibus parameter as it should.

I missed the fact that pci_create() already exists.  That's a pity.

[Qemu-devel] Re: [PATCH 5/8] ide/pci: convert to qdev.

[Gerd Hoffmann]

On 09/11/09 16:35, Juan Quintela wrote:
>>>> -    IDEBus bus[2];
>>>> +    IDEBus *bus[2];
>
> You notice this little thing "*" that changed?
> VMState preffers pretty much embedded arrays than pointers (arrays of
> pointers have the same problem, basically).
>
> Notice the "preffers" part.  VMState can follow pointers, just that each
> VMStateDescription is a series of offsets in one structure.  Having
> pointers brings to you all the problems associated with ioctl's with
> pointers in the middle of the payload.  It is doable, just not nice.

I don't see the problem here.

You'll need a separate VMStateDescription for a IDEBus anyway, then have 
all the ide drivers use that, just to avoid duplication.

ide will just be
   vmstate_ide_bus(bus)

pci will be something like
   vmstate_pci()
   /* busmaster stuff goes here */
   vmstate_ide_bus(bus[0])
   vmstate_ide_bus(bus[1])

I fail to see how it makes a big difference whenever the ide bus is 
referenced or embedded ...

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 4/8] ide/qdev: add ide bus.

[Gerd Hoffmann]

>> +    qdev_init(dev);
>
> Callback dev->info is ide_qdev_init(), which can fail, so qdev_init()
> can fail, can't it?

Indeed, will fix.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 5/8] ide/pci: convert to qdev.

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> With this patch applied ide drives (when attached to a pci adapter) can
> be created via -device, like this:
>
>   -drive if=none,id=mydisk,file=/path/to/disk.img
>   -device ide-drive,drive=mydisk,bus=ide.0,unit=0
>
> Note that creating a master on ide1 doesn't work that way.  That is a
> side effect of qemu creating a cdrom automagically even if you don't
> ask for it.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  Makefile.target |   10 ++-
>  hw/ide/pci.c    |  205 ++++++++++++++++++++++++++++++++++---------------------
>  2 files changed, 133 insertions(+), 82 deletions(-)
>
[...]
> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> index 607472b..aa6daf2 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
[...]
> @@ -375,19 +391,13 @@ static void cmd646_reset(void *opaque)
>  }
>  
>  /* CMD646 PCI IDE controller */
> -void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
> -                         int secondary_ide_enabled)
> +static int pci_cmd646_ide_initfn(PCIDevice *dev)
>  {
> -    PCIIDEState *d;
> -    uint8_t *pci_conf;
> +    PCIIDEState *d = DO_UPCAST(PCIIDEState, dev, dev);
> +    uint8_t *pci_conf = d->dev.config;
>      qemu_irq *irq;
>  
> -    d = (PCIIDEState *)pci_register_device(bus, "CMD646 IDE",
> -                                           sizeof(PCIIDEState),
> -                                           -1,
> -                                           NULL, NULL);
>      d->type = IDE_TYPE_CMD646;
> -    pci_conf = d->dev.config;
>      pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_CMD);
>      pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_CMD_646);
>  
> @@ -398,31 +408,47 @@ void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
>      pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
>  
>      pci_conf[0x51] = 0x04; // enable IDE0
> -    if (secondary_ide_enabled) {
> +    if (d->secondary) {
>          /* XXX: if not enabled, really disable the seconday IDE controller */
>          pci_conf[0x51] |= 0x08; /* enable IDE1 */
>      }
> +    pci_conf[0x3d] = 0x01; // interrupt on pin 1
>  
>      pci_register_bar((PCIDevice *)d, 0, 0x8,
> -                           PCI_ADDRESS_SPACE_IO, ide_map);
> +                     PCI_ADDRESS_SPACE_IO, ide_map);
>      pci_register_bar((PCIDevice *)d, 1, 0x4,
> -                           PCI_ADDRESS_SPACE_IO, ide_map);
> +                     PCI_ADDRESS_SPACE_IO, ide_map);
>      pci_register_bar((PCIDevice *)d, 2, 0x8,
> -                           PCI_ADDRESS_SPACE_IO, ide_map);
> +                     PCI_ADDRESS_SPACE_IO, ide_map);
>      pci_register_bar((PCIDevice *)d, 3, 0x4,
> -                           PCI_ADDRESS_SPACE_IO, ide_map);
> +                     PCI_ADDRESS_SPACE_IO, ide_map);
>      pci_register_bar((PCIDevice *)d, 4, 0x10,
> -                           PCI_ADDRESS_SPACE_IO, bmdma_map);
> +                     PCI_ADDRESS_SPACE_IO, bmdma_map);

Please don't mix white-space cleanups with complex changes.

>  
> -    pci_conf[0x3d] = 0x01; // interrupt on pin 1

Any particular reason for moving this assignment up?

> +    register_savevm("ide", 0, 3, pci_ide_save, pci_ide_load, d);
>  
> -    irq = qemu_allocate_irqs(cmd646_set_irq, d, 2);
> -    ide_init2(&d->bus[0], hd_table[0], hd_table[1], irq[0]);
> -    ide_init2(&d->bus[1], hd_table[2], hd_table[3], irq[1]);
> +    d->bus[0] = ide_bus_new(&d->dev.qdev);
> +    d->bus[1] = ide_bus_new(&d->dev.qdev);
>  
> -    register_savevm("ide", 0, 3, pci_ide_save, pci_ide_load, d);
>      qemu_register_reset(cmd646_reset, d);
>      cmd646_reset(d);
> +
> +    irq = qemu_allocate_irqs(cmd646_set_irq, d, 2);
> +    ide_init2(d->bus[0], NULL, NULL, irq[0]);
> +    ide_init2(d->bus[1], NULL, NULL, irq[1]);
> +    return 0;
> +}
> +
> +void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
> +                         int secondary_ide_enabled)
> +{
> +    PCIDevice *dev;
> +
> +    dev = pci_create_noinit(bus, -1, "CMD646 IDE");
> +    qdev_prop_set_uint32(&dev->qdev, "secondary", secondary_ide_enabled);
> +    qdev_init(&dev->qdev);
> +
> +    pci_ide_create_devs(dev, hd_table);
>  }
>  
>  static void piix3_reset(void *opaque)
> @@ -441,23 +467,10 @@ static void piix3_reset(void *opaque)
>      pci_conf[0x20] = 0x01; /* BMIBA: 20-23h */
>  }
>  
> -/* hd_table must contain 4 block drivers */
> -/* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
> -void pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
> +static int pci_piix_ide_initfn(PCIIDEState *d)
>  {
> -    PCIIDEState *d;
> -    uint8_t *pci_conf;
> -
> -    /* register a function 1 of PIIX3 */
> -    d = (PCIIDEState *)pci_register_device(bus, "PIIX3 IDE",
> -                                           sizeof(PCIIDEState),
> -                                           devfn,
> -                                           NULL, NULL);
> -    d->type = IDE_TYPE_PIIX3;
> +    uint8_t *pci_conf = d->dev.config;
>  
> -    pci_conf = d->dev.config;
> -    pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
> -    pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82371SB_1);
>      pci_conf[0x09] = 0x80; // legacy ATA mode
>      pci_config_set_class(pci_conf, PCI_CLASS_STORAGE_IDE);
>      pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
> @@ -466,48 +479,84 @@ void pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
>      piix3_reset(d);
>  
>      pci_register_bar((PCIDevice *)d, 4, 0x10,
> -                           PCI_ADDRESS_SPACE_IO, bmdma_map);
> -
> -    ide_init2(&d->bus[0], hd_table[0], hd_table[1], isa_reserve_irq(14));
> -    ide_init2(&d->bus[1], hd_table[2], hd_table[3], isa_reserve_irq(15));
> -    ide_init_ioport(&d->bus[0], 0x1f0, 0x3f6);
> -    ide_init_ioport(&d->bus[1], 0x170, 0x376);
> +                     PCI_ADDRESS_SPACE_IO, bmdma_map);
>  
>      register_savevm("ide", 0, 3, pci_ide_save, pci_ide_load, d);
> +
> +    d->bus[0] = ide_bus_new(&d->dev.qdev);
> +    d->bus[1] = ide_bus_new(&d->dev.qdev);
> +    ide_init_ioport(d->bus[0], 0x1f0, 0x3f6);
> +    ide_init_ioport(d->bus[1], 0x170, 0x376);
> +
> +    ide_init2(d->bus[0], NULL, NULL, isa_reserve_irq(14));
> +    ide_init2(d->bus[1], NULL, NULL, isa_reserve_irq(15));
> +    return 0;
>  }
>  
> -/* hd_table must contain 4 block drivers */
> -/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
> -void pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
> +static int pci_piix3_ide_initfn(PCIDevice *dev)
>  {
> -    PCIIDEState *d;
> -    uint8_t *pci_conf;
> -
> -    /* register a function 1 of PIIX4 */
> -    d = (PCIIDEState *)pci_register_device(bus, "PIIX4 IDE",
> -                                           sizeof(PCIIDEState),
> -                                           devfn,
> -                                           NULL, NULL);
> -    d->type = IDE_TYPE_PIIX4;
> +    PCIIDEState *d = DO_UPCAST(PCIIDEState, dev, dev);
>  
> -    pci_conf = d->dev.config;
> -    pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
> -    pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82371AB);
> -    pci_conf[0x09] = 0x80; // legacy ATA mode
> -    pci_config_set_class(pci_conf, PCI_CLASS_STORAGE_IDE);
> -    pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
> +    d->type = IDE_TYPE_PIIX3;
> +    pci_config_set_vendor_id(d->dev.config, PCI_VENDOR_ID_INTEL);
> +    pci_config_set_device_id(d->dev.config, PCI_DEVICE_ID_INTEL_82371SB_1);
> +    return pci_piix_ide_initfn(d);
> +}
>  
> -    qemu_register_reset(piix3_reset, d);
> -    piix3_reset(d);
> +static int pci_piix4_ide_initfn(PCIDevice *dev)
> +{
> +    PCIIDEState *d = DO_UPCAST(PCIIDEState, dev, dev);
>  
> -    pci_register_bar((PCIDevice *)d, 4, 0x10,
> -                           PCI_ADDRESS_SPACE_IO, bmdma_map);
> +    d->type = IDE_TYPE_PIIX4;
> +    pci_config_set_vendor_id(d->dev.config, PCI_VENDOR_ID_INTEL);
> +    pci_config_set_device_id(d->dev.config, PCI_DEVICE_ID_INTEL_82371AB);
> +    return pci_piix_ide_initfn(d);
> +}
>  
> -    ide_init2(&d->bus[0], hd_table[0], hd_table[1], isa_reserve_irq(14));
> -    ide_init2(&d->bus[1], hd_table[2], hd_table[3], isa_reserve_irq(15));
> -    ide_init_ioport(&d->bus[0], 0x1f0, 0x3f6);
> -    ide_init_ioport(&d->bus[1], 0x170, 0x376);

This part of the diff is a bit confusing because besides qdevification
it also factors out common parts of piix3 and piix4 into
pci_piix_ide_initfn().  Good move, but would be easier to review as a
separate commit.

> +/* hd_table must contain 4 block drivers */
> +/* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
> +void pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
> +{
> +    PCIDevice *dev;
>  
> -    register_savevm("ide", 0, 3, pci_ide_save, pci_ide_load, d);
> +    dev = pci_create_simple(bus, devfn, "PIIX3 IDE");
> +    pci_ide_create_devs(dev, hd_table);
> +}
> +
> +/* hd_table must contain 4 block drivers */
> +/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
> +void pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
> +{
> +    PCIDevice *dev;
> +
> +    dev = pci_create_simple(bus, devfn, "PIIX4 IDE");
> +    pci_ide_create_devs(dev, hd_table);
>  }
>  
> +static PCIDeviceInfo piix_ide_info[] = {
> +    {
> +        .qdev.name    = "PIIX3 IDE",
> +        .qdev.size    = sizeof(PCIIDEState),
> +        .init         = pci_piix3_ide_initfn,
> +    },{
> +        .qdev.name    = "PIIX4 IDE",
> +        .qdev.size    = sizeof(PCIIDEState),
> +        .init         = pci_piix4_ide_initfn,
> +    },{
> +        .qdev.name    = "CMD646 IDE",
> +        .qdev.size    = sizeof(PCIIDEState),
> +        .init         = pci_cmd646_ide_initfn,
> +        .qdev.props   = (Property[]) {
> +            DEFINE_PROP_UINT32("secondary", PCIIDEState, secondary, 0),
> +            DEFINE_PROP_END_OF_LIST(),
> +        },
> +    },{
> +        /* end of list */
> +    }
> +};
> +
> +static void piix_ide_register(void)
> +{
> +    pci_qdev_register_many(piix_ide_info);
> +}
> +device_init(piix_ide_register);

[Qemu-devel] Re: [PATCH 5/8] ide/pci: convert to qdev.

[Gerd Hoffmann]

On 09/11/09 17:16, Juan Quintela wrote:
> But now we look at savevm.c::vmstate_load_state()
>
>         if (field->flags&  VMS_POINTER) {
>              base_addr = *(void **)base_addr;
>         }
>
> And you see this really nice piece of code.  Each time that we follow a
> pointer, we have to read something for a table, hope that value is
> right, and follow it.
>
> Do you see know why I want to have the minimal amount of pointers
> possible to follow?

No.

Sure, you have to dereference the pointer.  I still don't see a problem 
here.  You seem to think this is fragile.  Why do you think so? 
Typechecking missing somewhere?

> And yes, I understand why you don't want qdev_create_here() idea, I am
> pointing this out to make sure that everybody agrees that not having
> qdev_create_here() and having rest of code use more
> pointers/malloc/... is the right compromise.

I'm sure you'll need VMS_POINTER anyway.  There will be corner cases 
which don't work without.  I think the floppy fifo is one of them.

It is perfectly fine to avoid the pointer indirection if possible.
It isn't the most important thing on earth though.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 5/8] ide/pci: convert to qdev.

[Gerd Hoffmann]

   Hi,

>> -    pci_conf[0x3d] = 0x01; // interrupt on pin 1
>
> Any particular reason for moving this assignment up?

Probably just because the patch went through a number of revisions ...

cheers,
   Gerd

[Qemu-devel] Re: [PATCH 5/8] ide/pci: convert to qdev.

[Gerd Hoffmann]

   Hi,

>> Sure, you have to dereference the pointer.  I still don't see a
>> problem here.  You seem to think this is fragile.  Why do you think
>> so?
>
> Pointer not initialized, it is NULL, point nowhere, etc, etc.
> No pointer, you don't need to even think about all that kind of porblems/checks.

NULL can easily be checked for, and that should also catch the "not 
initialized" case.  Pointing into nowhere is a clear bug which needs 
fixing anyway.

> uint8_t *foo[4]
> is a different beast ... how do you declare a pointer to one array of 4
> uint8_t?

This isn't a pointer to an array, it is a array of pointers ...

cheers,
   Gerd