Patchwork [3/3] xen: implement unplug protocol in xen_platform

login
register
mail settings
Submitter Stefano Stabellini
Date June 27, 2011, 3:34 p.m.
Message ID <alpine.DEB.2.00.1106271555540.12963@kaball-desktop>
Download mbox | patch
Permalink /patch/102221/
State New
Headers show

Comments

Stefano Stabellini - June 27, 2011, 3:34 p.m.
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.

---
Kevin Wolf - June 27, 2011, 3:46 p.m.
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
Stefano Stabellini - June 28, 2011, 11:27 a.m.
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.

Patch

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) {