| Message ID | 1252672351-12937-6-git-send-email-kraxel@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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);
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
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
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
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);
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(-)