Message ID | alpine.DEB.2.00.1106271555540.12963@kaball-desktop |
---|---|
State | New |
Headers | show |
Am 27.06.2011 17:34, schrieb Stefano Stabellini: > On Mon, 27 Jun 2011, Kevin Wolf wrote: >> hw/ide/pci.h is just as internal as internal.h is. And even if you >> managed to access the same things without any IDE header file, I still >> think it's not the right level of abstraction because it relies on the >> implementation details of IDE. >> >> Just this line: pci_ide->bus[di->bus].ifs[di->unit].bs = NULL; Does this >> really look right to you to do anywhere outside IDE? >> >> I'm basically looking for the same as Michael who wanted to have network >> unplug handled through qdev, just that the IDE code doesn't support >> unplug yet. > > I understand. > > I created pci_piix3_xen_ide_init and moved the unplug code to > hw/ide/piix.c so that we don't have any internal knowledge of IDE code > in xen_platform.c any more, see below. > > One remaining problem is that the generic pci unplug function is not > what I need in this case so I have to setup my own; but I hope that in > general the changes are going in the right direction. Looks much better to me now. :-) Just a few comments inline: > diff --git a/hw/ide.h b/hw/ide.h > index 34d9394..a490cbb 100644 > --- a/hw/ide.h > +++ b/hw/ide.h > @@ -13,6 +13,7 @@ ISADevice *isa_ide_init(int iobase, int iobase2, int isairq, > /* ide-pci.c */ > void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table, > int secondary_ide_enabled); > +PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn); > PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn); > PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn); > void vt82c686b_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn); > diff --git a/hw/ide/piix.c b/hw/ide/piix.c > index c349644..8ae9ff0 100644 > --- a/hw/ide/piix.c > +++ b/hw/ide/piix.c > @@ -167,6 +167,41 @@ static int pci_piix4_ide_initfn(PCIDevice *dev) > return pci_piix_ide_initfn(d); > } > > +static int pci_piix3_xen_ide_unplug(DeviceState *dev) > +{ > + PCIDevice *pci_dev; > + PCIIDEState *pci_ide; > + DriveInfo *di; > + int i = 0; > + > + pci_dev = DO_UPCAST(PCIDevice, qdev, dev); > + pci_ide = DO_UPCAST(PCIIDEState, dev, pci_dev); > + > + for (; i < 3; i++) { > + di = drive_get_by_index(IF_IDE, i); > + if (di != NULL && di->bdrv != NULL && !di->bdrv->removable) { > + DeviceState *ds = bdrv_get_attached(di->bdrv); > + if (ds) > + bdrv_detach(di->bdrv, ds); Missing braces > + bdrv_close(di->bdrv); > + pci_ide->bus[di->bus].ifs[di->unit].bs = NULL; > + drive_put_ref(di); > + } > + } > + qdev_reset_all(&(pci_ide->dev.qdev)); > + return 0; > +} > + > +PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn) > +{ > + PCIDevice *dev; > + > + dev = pci_create_simple(bus, devfn, "piix3-ide"); > + dev->qdev.info->unplug = pci_piix3_xen_ide_unplug; Doesn't this modify the piix3-ide definition? Shouldn't matter in practice because you can't have additional IDE controllers anyway, but maybe worth a comment stating this. The other, probably cleaner way of doing it would be adding another PCIDeviceInfo for xen-ide. Kevin
On Mon, 27 Jun 2011, Kevin Wolf wrote: > Am 27.06.2011 17:34, schrieb Stefano Stabellini: > > On Mon, 27 Jun 2011, Kevin Wolf wrote: > >> hw/ide/pci.h is just as internal as internal.h is. And even if you > >> managed to access the same things without any IDE header file, I still > >> think it's not the right level of abstraction because it relies on the > >> implementation details of IDE. > >> > >> Just this line: pci_ide->bus[di->bus].ifs[di->unit].bs = NULL; Does this > >> really look right to you to do anywhere outside IDE? > >> > >> I'm basically looking for the same as Michael who wanted to have network > >> unplug handled through qdev, just that the IDE code doesn't support > >> unplug yet. > > > > I understand. > > > > I created pci_piix3_xen_ide_init and moved the unplug code to > > hw/ide/piix.c so that we don't have any internal knowledge of IDE code > > in xen_platform.c any more, see below. > > > > One remaining problem is that the generic pci unplug function is not > > what I need in this case so I have to setup my own; but I hope that in > > general the changes are going in the right direction. > > Looks much better to me now. :-) great :) > Just a few comments inline: > > > diff --git a/hw/ide.h b/hw/ide.h > > index 34d9394..a490cbb 100644 > > --- a/hw/ide.h > > +++ b/hw/ide.h > > @@ -13,6 +13,7 @@ ISADevice *isa_ide_init(int iobase, int iobase2, int isairq, > > /* ide-pci.c */ > > void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table, > > int secondary_ide_enabled); > > +PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn); > > PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn); > > PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn); > > void vt82c686b_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn); > > diff --git a/hw/ide/piix.c b/hw/ide/piix.c > > index c349644..8ae9ff0 100644 > > --- a/hw/ide/piix.c > > +++ b/hw/ide/piix.c > > @@ -167,6 +167,41 @@ static int pci_piix4_ide_initfn(PCIDevice *dev) > > return pci_piix_ide_initfn(d); > > } > > > > +static int pci_piix3_xen_ide_unplug(DeviceState *dev) > > +{ > > + PCIDevice *pci_dev; > > + PCIIDEState *pci_ide; > > + DriveInfo *di; > > + int i = 0; > > + > > + pci_dev = DO_UPCAST(PCIDevice, qdev, dev); > > + pci_ide = DO_UPCAST(PCIIDEState, dev, pci_dev); > > + > > + for (; i < 3; i++) { > > + di = drive_get_by_index(IF_IDE, i); > > + if (di != NULL && di->bdrv != NULL && !di->bdrv->removable) { > > + DeviceState *ds = bdrv_get_attached(di->bdrv); > > + if (ds) > > + bdrv_detach(di->bdrv, ds); > > Missing braces This time I run checkpatch.pl on it. > > > + bdrv_close(di->bdrv); > > + pci_ide->bus[di->bus].ifs[di->unit].bs = NULL; > > + drive_put_ref(di); > > + } > > + } > > + qdev_reset_all(&(pci_ide->dev.qdev)); > > + return 0; > > +} > > + > > +PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn) > > +{ > > + PCIDevice *dev; > > + > > + dev = pci_create_simple(bus, devfn, "piix3-ide"); > > + dev->qdev.info->unplug = pci_piix3_xen_ide_unplug; > > Doesn't this modify the piix3-ide definition? Shouldn't matter in > practice because you can't have additional IDE controllers anyway, but > maybe worth a comment stating this. > > The other, probably cleaner way of doing it would be adding another > PCIDeviceInfo for xen-ide. I have added piix3-ide-xen, that doesn't set no_hotplug = 1, so now the code should be more coherent. I'll send the new version of the patch separately.
diff --git a/hw/ide.h b/hw/ide.h index 34d9394..a490cbb 100644 --- a/hw/ide.h +++ b/hw/ide.h @@ -13,6 +13,7 @@ ISADevice *isa_ide_init(int iobase, int iobase2, int isairq, /* ide-pci.c */ void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table, int secondary_ide_enabled); +PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn); PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn); PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn); void vt82c686b_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn); diff --git a/hw/ide/piix.c b/hw/ide/piix.c index c349644..8ae9ff0 100644 --- a/hw/ide/piix.c +++ b/hw/ide/piix.c @@ -167,6 +167,41 @@ static int pci_piix4_ide_initfn(PCIDevice *dev) return pci_piix_ide_initfn(d); } +static int pci_piix3_xen_ide_unplug(DeviceState *dev) +{ + PCIDevice *pci_dev; + PCIIDEState *pci_ide; + DriveInfo *di; + int i = 0; + + pci_dev = DO_UPCAST(PCIDevice, qdev, dev); + pci_ide = DO_UPCAST(PCIIDEState, dev, pci_dev); + + for (; i < 3; i++) { + di = drive_get_by_index(IF_IDE, i); + if (di != NULL && di->bdrv != NULL && !di->bdrv->removable) { + DeviceState *ds = bdrv_get_attached(di->bdrv); + if (ds) + bdrv_detach(di->bdrv, ds); + bdrv_close(di->bdrv); + pci_ide->bus[di->bus].ifs[di->unit].bs = NULL; + drive_put_ref(di); + } + } + qdev_reset_all(&(pci_ide->dev.qdev)); + return 0; +} + +PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn) +{ + PCIDevice *dev; + + dev = pci_create_simple(bus, devfn, "piix3-ide"); + dev->qdev.info->unplug = pci_piix3_xen_ide_unplug; + pci_ide_create_devs(dev, hd_table); + return dev; +} + /* hd_table must contain 4 block drivers */ /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */ PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn) diff --git a/hw/pc_piix.c b/hw/pc_piix.c index 8dbeb0c..b59adcc 100644 --- a/hw/pc_piix.c +++ b/hw/pc_piix.c @@ -155,7 +155,11 @@ static void pc_init1(ram_addr_t ram_size, ide_drive_get(hd, MAX_IDE_BUS); if (pci_enabled) { PCIDevice *dev; - dev = pci_piix3_ide_init(pci_bus, hd, piix3_devfn + 1); + if (xen_enabled()) { + dev = pci_piix3_xen_ide_init(pci_bus, hd, piix3_devfn + 1); + } else { + dev = pci_piix3_ide_init(pci_bus, hd, piix3_devfn + 1); + } idebus[0] = qdev_get_child_bus(&dev->qdev, "ide.0"); idebus[1] = qdev_get_child_bus(&dev->qdev, "ide.1"); } else { diff --git a/hw/xen_platform.c b/hw/xen_platform.c index b167eee..a271369 100644 --- a/hw/xen_platform.c +++ b/hw/xen_platform.c @@ -76,6 +76,35 @@ static void log_writeb(PCIXenPlatformState *s, char val) } /* Xen Platform, Fixed IOPort */ +#define UNPLUG_ALL_IDE_DISKS 1 +#define UNPLUG_ALL_NICS 2 +#define UNPLUG_AUX_IDE_DISKS 4 + +static void unplug_nic(PCIBus *b, PCIDevice *d) +{ + if (pci_get_word(d->config + PCI_CLASS_DEVICE) == + PCI_CLASS_NETWORK_ETHERNET) { + qdev_unplug(&(d->qdev)); + } +} + +static void pci_unplug_nics(PCIBus *bus) +{ + pci_for_each_device(bus, 0, unplug_nic); +} + +static void unplug_disks(PCIBus *b, PCIDevice *d) +{ + if (pci_get_word(d->config + PCI_CLASS_DEVICE) == + PCI_CLASS_STORAGE_IDE) { + qdev_unplug(&(d->qdev)); + } +} + +static void pci_unplug_disks(PCIBus *bus) +{ + pci_for_each_device(bus, 0, unplug_disks); +} static void platform_fixed_ioport_writew(void *opaque, uint32_t addr, uint32_t val) { @@ -83,10 +112,22 @@ static void platform_fixed_ioport_writew(void *opaque, uint32_t addr, uint32_t v switch (addr - XEN_PLATFORM_IOPORT) { case 0: - /* TODO: */ /* Unplug devices. Value is a bitmask of which devices to unplug, with bit 0 the IDE devices, bit 1 the network devices, and bit 2 the non-primary-master IDE devices. */ + if (val & UNPLUG_ALL_IDE_DISKS) { + DPRINTF("unplug disks\n"); + qemu_aio_flush(); + bdrv_flush_all(); + pci_unplug_disks(s->pci_dev.bus); + } + if (val & UNPLUG_ALL_NICS) { + DPRINTF("unplug nics\n"); + pci_unplug_nics(s->pci_dev.bus); + } + if (val & UNPLUG_AUX_IDE_DISKS) { + DPRINTF("unplug auxiliary disks not supported\n"); + } break; case 2: switch (val) {