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.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home