[Qemu-devel] [PATCH v3] xen: implement unplug protocol in xen_platform

[Qemu-devel] [PATCH v3] xen: implement unplug protocol in xen_platform

[stefano.stabellini]

From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

The unplug protocol is necessary to support PV drivers in the guest: the
drivers expect to be able to "unplug" emulated disks and nics before
initializing the Xen PV interfaces.
It is responsibility of the guest to make sure that the unplug is done
before the emulated devices or the PV interface start to be used.

We use pci_for_each_device to walk the PCI bus, identify the devices and
disks that we want to disable and dynamically unplug them.


Changes in v2:

- use PCI_CLASS constants;

- replace pci_unplug_device with qdev_unplug;

- do not import hw/ide/internal.h in xen_platform.c;


Changes in v3:

- introduce piix3-ide-xen, that support hot-unplug;

- move the unplug code to hw/ide/piix.c;

- just call qdev_unplug from xen_platform.c to unplug the IDE disks;


Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/ide.h          |    1 +
 hw/ide/piix.c     |   41 +++++++++++++++++++++++++++++++++++++++++
 hw/pc_piix.c      |    6 +++++-
 hw/xen_platform.c |   43 ++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 89 insertions(+), 2 deletions(-)

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..50ab7c5 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -167,6 +167,42 @@ 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-xen");
+    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)
@@ -197,6 +233,11 @@ static PCIDeviceInfo piix_ide_info[] = {
         .no_hotplug   = 1,
         .init         = pci_piix3_ide_initfn,
     },{
+        .qdev.name    = "piix3-ide-xen",
+        .qdev.size    = sizeof(PCIIDEState),
+        .qdev.no_user = 1,
+        .init         = pci_piix3_ide_initfn,
+    },{
         .qdev.name    = "piix4-ide",
         .qdev.size    = sizeof(PCIIDEState),
         .qdev.no_user = 1,
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) {
-- 
1.7.2.3

[PATCH v3] xen: implement unplug protocol in xen_platform

[stefano.stabellini]

From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

The unplug protocol is necessary to support PV drivers in the guest: the
drivers expect to be able to "unplug" emulated disks and nics before
initializing the Xen PV interfaces.
It is responsibility of the guest to make sure that the unplug is done
before the emulated devices or the PV interface start to be used.

We use pci_for_each_device to walk the PCI bus, identify the devices and
disks that we want to disable and dynamically unplug them.


Changes in v2:

- use PCI_CLASS constants;

- replace pci_unplug_device with qdev_unplug;

- do not import hw/ide/internal.h in xen_platform.c;


Changes in v3:

- introduce piix3-ide-xen, that support hot-unplug;

- move the unplug code to hw/ide/piix.c;

- just call qdev_unplug from xen_platform.c to unplug the IDE disks;


Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/ide.h          |    1 +
 hw/ide/piix.c     |   41 +++++++++++++++++++++++++++++++++++++++++
 hw/pc_piix.c      |    6 +++++-
 hw/xen_platform.c |   43 ++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 89 insertions(+), 2 deletions(-)

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..50ab7c5 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -167,6 +167,42 @@ 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-xen");
+    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)
@@ -197,6 +233,11 @@ static PCIDeviceInfo piix_ide_info[] = {
         .no_hotplug   = 1,
         .init         = pci_piix3_ide_initfn,
     },{
+        .qdev.name    = "piix3-ide-xen",
+        .qdev.size    = sizeof(PCIIDEState),
+        .qdev.no_user = 1,
+        .init         = pci_piix3_ide_initfn,
+    },{
         .qdev.name    = "piix4-ide",
         .qdev.size    = sizeof(PCIIDEState),
         .qdev.no_user = 1,
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) {
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH v3] xen: implement unplug protocol in xen_platform

[Alexander Graf]

On 06/28/2011 01:29 PM, stefano.stabellini@eu.citrix.com wrote:
> From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>
> The unplug protocol is necessary to support PV drivers in the guest: the
> drivers expect to be able to "unplug" emulated disks and nics before
> initializing the Xen PV interfaces.
> It is responsibility of the guest to make sure that the unplug is done
> before the emulated devices or the PV interface start to be used.
>
> We use pci_for_each_device to walk the PCI bus, identify the devices and
> disks that we want to disable and dynamically unplug them.

$ ~/git/qemu/scripts/checkpatch.pl *
ERROR: space required after that ',' (ctx:VxV)
#158: FILE: hw/ide/piix.c:240:
+    },{
       ^

total: 1 errors, 0 warnings, 130 lines checked


I definitely want to see an ack from Kevin here first though.


Alex

Re: [PATCH v3] xen: implement unplug protocol in xen_platform

[Alexander Graf]

On 06/28/2011 01:29 PM, stefano.stabellini@eu.citrix.com wrote:
> From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>
> The unplug protocol is necessary to support PV drivers in the guest: the
> drivers expect to be able to "unplug" emulated disks and nics before
> initializing the Xen PV interfaces.
> It is responsibility of the guest to make sure that the unplug is done
> before the emulated devices or the PV interface start to be used.
>
> We use pci_for_each_device to walk the PCI bus, identify the devices and
> disks that we want to disable and dynamically unplug them.

$ ~/git/qemu/scripts/checkpatch.pl *
ERROR: space required after that ',' (ctx:VxV)
#158: FILE: hw/ide/piix.c:240:
+    },{
       ^

total: 1 errors, 0 warnings, 130 lines checked


I definitely want to see an ack from Kevin here first though.


Alex

Re: [Qemu-devel] [PATCH v3] xen: implement unplug protocol in xen_platform

[Kevin Wolf]

Am 28.06.2011 13:29, schrieb stefano.stabellini@eu.citrix.com:
> From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> The unplug protocol is necessary to support PV drivers in the guest: the
> drivers expect to be able to "unplug" emulated disks and nics before
> initializing the Xen PV interfaces.
> It is responsibility of the guest to make sure that the unplug is done
> before the emulated devices or the PV interface start to be used.
> 
> We use pci_for_each_device to walk the PCI bus, identify the devices and
> disks that we want to disable and dynamically unplug them.
> 
> 
> Changes in v2:
> 
> - use PCI_CLASS constants;
> 
> - replace pci_unplug_device with qdev_unplug;
> 
> - do not import hw/ide/internal.h in xen_platform.c;
> 
> 
> Changes in v3:
> 
> - introduce piix3-ide-xen, that support hot-unplug;
> 
> - move the unplug code to hw/ide/piix.c;
> 
> - just call qdev_unplug from xen_platform.c to unplug the IDE disks;
> 
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  hw/ide.h          |    1 +
>  hw/ide/piix.c     |   41 +++++++++++++++++++++++++++++++++++++++++
>  hw/pc_piix.c      |    6 +++++-
>  hw/xen_platform.c |   43 ++++++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 89 insertions(+), 2 deletions(-)
> 
> 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..50ab7c5 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -167,6 +167,42 @@ 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;

Have you tested if this is enough if the guest tries to continue using
the device? I don't know of any case where it's not sufficient, just
trying to make sure that it's really true in practice.

> +            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-xen");
> +    dev->qdev.info->unplug = pci_piix3_xen_ide_unplug;

Can't this be moved into the PCIDeviceInfo now that we have a separate
one for Xen?

Kevin

Re: [Qemu-devel] [PATCH v3] xen: implement unplug protocol in xen_platform

[Alexander Graf]

On 06/30/2011 02:16 PM, Kevin Wolf wrote:
> Am 30.06.2011 13:45, schrieb Alexander Graf:
>> On 06/28/2011 01:29 PM, stefano.stabellini@eu.citrix.com wrote:
>>> From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>>>
>>> The unplug protocol is necessary to support PV drivers in the guest: the
>>> drivers expect to be able to "unplug" emulated disks and nics before
>>> initializing the Xen PV interfaces.
>>> It is responsibility of the guest to make sure that the unplug is done
>>> before the emulated devices or the PV interface start to be used.
>>>
>>> We use pci_for_each_device to walk the PCI bus, identify the devices and
>>> disks that we want to disable and dynamically unplug them.
>> $ ~/git/qemu/scripts/checkpatch.pl *
>> ERROR: space required after that ',' (ctx:VxV)
>> #158: FILE: hw/ide/piix.c:240:
>> +    },{
>>         ^
>>
>> total: 1 errors, 0 warnings, 130 lines checked
> I think checkpatch.pl should be fixed in this case. We do have this
> pattern all over the place in qemu and I don't see why it's bad.

Hrm. I tend to agree. I'll post a patch :).


Alex

Re: [PATCH v3] xen: implement unplug protocol in xen_platform

[Alexander Graf]

On 06/30/2011 02:16 PM, Kevin Wolf wrote:
> Am 30.06.2011 13:45, schrieb Alexander Graf:
>> On 06/28/2011 01:29 PM, stefano.stabellini@eu.citrix.com wrote:
>>> From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>>>
>>> The unplug protocol is necessary to support PV drivers in the guest: the
>>> drivers expect to be able to "unplug" emulated disks and nics before
>>> initializing the Xen PV interfaces.
>>> It is responsibility of the guest to make sure that the unplug is done
>>> before the emulated devices or the PV interface start to be used.
>>>
>>> We use pci_for_each_device to walk the PCI bus, identify the devices and
>>> disks that we want to disable and dynamically unplug them.
>> $ ~/git/qemu/scripts/checkpatch.pl *
>> ERROR: space required after that ',' (ctx:VxV)
>> #158: FILE: hw/ide/piix.c:240:
>> +    },{
>>         ^
>>
>> total: 1 errors, 0 warnings, 130 lines checked
> I think checkpatch.pl should be fixed in this case. We do have this
> pattern all over the place in qemu and I don't see why it's bad.

Hrm. I tend to agree. I'll post a patch :).


Alex

Re: [Qemu-devel] [PATCH v3] xen: implement unplug protocol in xen_platform

[Kevin Wolf]

Am 30.06.2011 13:45, schrieb Alexander Graf:
> On 06/28/2011 01:29 PM, stefano.stabellini@eu.citrix.com wrote:
>> From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>>
>> The unplug protocol is necessary to support PV drivers in the guest: the
>> drivers expect to be able to "unplug" emulated disks and nics before
>> initializing the Xen PV interfaces.
>> It is responsibility of the guest to make sure that the unplug is done
>> before the emulated devices or the PV interface start to be used.
>>
>> We use pci_for_each_device to walk the PCI bus, identify the devices and
>> disks that we want to disable and dynamically unplug them.
> 
> $ ~/git/qemu/scripts/checkpatch.pl *
> ERROR: space required after that ',' (ctx:VxV)
> #158: FILE: hw/ide/piix.c:240:
> +    },{
>        ^
> 
> total: 1 errors, 0 warnings, 130 lines checked

I think checkpatch.pl should be fixed in this case. We do have this
pattern all over the place in qemu and I don't see why it's bad.

> I definitely want to see an ack from Kevin here first though.

The approach looks good enough for me, unless someone has a better idea
on how to do it cleanly. I commented on the implementation.

Kevin

Re: [PATCH v3] xen: implement unplug protocol in xen_platform

[Kevin Wolf]

Am 30.06.2011 13:45, schrieb Alexander Graf:
> On 06/28/2011 01:29 PM, stefano.stabellini@eu.citrix.com wrote:
>> From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>>
>> The unplug protocol is necessary to support PV drivers in the guest: the
>> drivers expect to be able to "unplug" emulated disks and nics before
>> initializing the Xen PV interfaces.
>> It is responsibility of the guest to make sure that the unplug is done
>> before the emulated devices or the PV interface start to be used.
>>
>> We use pci_for_each_device to walk the PCI bus, identify the devices and
>> disks that we want to disable and dynamically unplug them.
> 
> $ ~/git/qemu/scripts/checkpatch.pl *
> ERROR: space required after that ',' (ctx:VxV)
> #158: FILE: hw/ide/piix.c:240:
> +    },{
>        ^
> 
> total: 1 errors, 0 warnings, 130 lines checked

I think checkpatch.pl should be fixed in this case. We do have this
pattern all over the place in qemu and I don't see why it's bad.

> I definitely want to see an ack from Kevin here first though.

The approach looks good enough for me, unless someone has a better idea
on how to do it cleanly. I commented on the implementation.

Kevin

Re: [Qemu-devel] [PATCH v3] xen: implement unplug protocol in xen_platform

[Stefano Stabellini]

On Thu, 30 Jun 2011, Kevin Wolf wrote:
> > +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;
> 
> Have you tested if this is enough if the guest tries to continue using
> the device? I don't know of any case where it's not sufficient, just
> trying to make sure that it's really true in practice.

The purpose of this is to "hide" the disk from the guest. The unplug is
supposed to happen *before* the guest enumerates the IDE disks; it is
responsibility of the guest to make sure of it.
I tested it with Linux PV on HVM drivers, and Linux doesn't see the
emulated disk after the unplug, as it should be.


> > +            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-xen");
> > +    dev->qdev.info->unplug = pci_piix3_xen_ide_unplug;
> 
> Can't this be moved into the PCIDeviceInfo now that we have a separate
> one for Xen?
 
No because it would be overridden by the default pci unplug function,
that is not what we want in this case.

Re: [Qemu-devel] [PATCH v3] xen: implement unplug protocol in xen_platform

[Kevin Wolf]

Am 30.06.2011 16:16, schrieb Stefano Stabellini:
> On Thu, 30 Jun 2011, Kevin Wolf wrote:
>>> +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;
>>
>> Have you tested if this is enough if the guest tries to continue using
>> the device? I don't know of any case where it's not sufficient, just
>> trying to make sure that it's really true in practice.
> 
> The purpose of this is to "hide" the disk from the guest. The unplug is
> supposed to happen *before* the guest enumerates the IDE disks; it is
> responsibility of the guest to make sure of it.
> I tested it with Linux PV on HVM drivers, and Linux doesn't see the
> emulated disk after the unplug, as it should be.

Yeah. What I meant is that we should make sure that a misbehaving guest,
which just keeps on playing with the IDE ports anyway, can't crash qemu.
A quick review suggests that it is the case, but testing it anyway would
be better.

>>> +            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-xen");
>>> +    dev->qdev.info->unplug = pci_piix3_xen_ide_unplug;
>>
>> Can't this be moved into the PCIDeviceInfo now that we have a separate
>> one for Xen?
>  
> No because it would be overridden by the default pci unplug function,
> that is not what we want in this case.

Okay. I'm not familiar with that code, so I'll just trust you there.

Kevin

Re: [Qemu-devel] [PATCH v3] xen: implement unplug protocol in xen_platform

[Stefano Stabellini]

On Fri, 1 Jul 2011, Kevin Wolf wrote:
> Am 30.06.2011 16:16, schrieb Stefano Stabellini:
> > On Thu, 30 Jun 2011, Kevin Wolf wrote:
> >>> +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;
> >>
> >> Have you tested if this is enough if the guest tries to continue using
> >> the device? I don't know of any case where it's not sufficient, just
> >> trying to make sure that it's really true in practice.
> > 
> > The purpose of this is to "hide" the disk from the guest. The unplug is
> > supposed to happen *before* the guest enumerates the IDE disks; it is
> > responsibility of the guest to make sure of it.
> > I tested it with Linux PV on HVM drivers, and Linux doesn't see the
> > emulated disk after the unplug, as it should be.
> 
> Yeah. What I meant is that we should make sure that a misbehaving guest,
> which just keeps on playing with the IDE ports anyway, can't crash qemu.
> A quick review suggests that it is the case, but testing it anyway would
> be better.

I see what you mean: I tested it, a guest cannot crash Qemu.

Re: [Qemu-devel] [PATCH v3] xen: implement unplug protocol in xen_platform

[Stefano Stabellini]

On Fri, 1 Jul 2011, Stefano Stabellini wrote:
> On Fri, 1 Jul 2011, Kevin Wolf wrote:
> > Am 30.06.2011 16:16, schrieb Stefano Stabellini:
> > > On Thu, 30 Jun 2011, Kevin Wolf wrote:
> > >>> +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;
> > >>
> > >> Have you tested if this is enough if the guest tries to continue using
> > >> the device? I don't know of any case where it's not sufficient, just
> > >> trying to make sure that it's really true in practice.
> > > 
> > > The purpose of this is to "hide" the disk from the guest. The unplug is
> > > supposed to happen *before* the guest enumerates the IDE disks; it is
> > > responsibility of the guest to make sure of it.
> > > I tested it with Linux PV on HVM drivers, and Linux doesn't see the
> > > emulated disk after the unplug, as it should be.
> > 
> > Yeah. What I meant is that we should make sure that a misbehaving guest,
> > which just keeps on playing with the IDE ports anyway, can't crash qemu.
> > A quick review suggests that it is the case, but testing it anyway would
> > be better.
> 
> I see what you mean: I tested it, a guest cannot crash Qemu.
> 

ping?

Re: [Qemu-devel] [PATCH v3] xen: implement unplug protocol in xen_platform

[Kevin Wolf]

Am 15.07.2011 12:34, schrieb Stefano Stabellini:
> On Fri, 1 Jul 2011, Stefano Stabellini wrote:
>> On Fri, 1 Jul 2011, Kevin Wolf wrote:
>>> Am 30.06.2011 16:16, schrieb Stefano Stabellini:
>>>> On Thu, 30 Jun 2011, Kevin Wolf wrote:
>>>>>> +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;
>>>>>
>>>>> Have you tested if this is enough if the guest tries to continue using
>>>>> the device? I don't know of any case where it's not sufficient, just
>>>>> trying to make sure that it's really true in practice.
>>>>
>>>> The purpose of this is to "hide" the disk from the guest. The unplug is
>>>> supposed to happen *before* the guest enumerates the IDE disks; it is
>>>> responsibility of the guest to make sure of it.
>>>> I tested it with Linux PV on HVM drivers, and Linux doesn't see the
>>>> emulated disk after the unplug, as it should be.
>>>
>>> Yeah. What I meant is that we should make sure that a misbehaving guest,
>>> which just keeps on playing with the IDE ports anyway, can't crash qemu.
>>> A quick review suggests that it is the case, but testing it anyway would
>>> be better.
>>
>> I see what you mean: I tested it, a guest cannot crash Qemu.
>>
> 
> ping?

I thought Alex had already merged it. I'm pretty sure that I stated
somewhere that the patch is okay for me now. In case I didn't:

Acked-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH v3] xen: implement unplug protocol in xen_platform

[Alexander Graf]

On 15.07.2011, at 12:52, Kevin Wolf wrote:

> Am 15.07.2011 12:34, schrieb Stefano Stabellini:
>> On Fri, 1 Jul 2011, Stefano Stabellini wrote:
>>> On Fri, 1 Jul 2011, Kevin Wolf wrote:
>>>> Am 30.06.2011 16:16, schrieb Stefano Stabellini:
>>>>> On Thu, 30 Jun 2011, Kevin Wolf wrote:
>>>>>>> +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;
>>>>>> 
>>>>>> Have you tested if this is enough if the guest tries to continue using
>>>>>> the device? I don't know of any case where it's not sufficient, just
>>>>>> trying to make sure that it's really true in practice.
>>>>> 
>>>>> The purpose of this is to "hide" the disk from the guest. The unplug is
>>>>> supposed to happen *before* the guest enumerates the IDE disks; it is
>>>>> responsibility of the guest to make sure of it.
>>>>> I tested it with Linux PV on HVM drivers, and Linux doesn't see the
>>>>> emulated disk after the unplug, as it should be.
>>>> 
>>>> Yeah. What I meant is that we should make sure that a misbehaving guest,
>>>> which just keeps on playing with the IDE ports anyway, can't crash qemu.
>>>> A quick review suggests that it is the case, but testing it anyway would
>>>> be better.
>>> 
>>> I see what you mean: I tested it, a guest cannot crash Qemu.
>>> 
>> 
>> ping?
> 
> I thought Alex had already merged it. I'm pretty sure that I stated
> somewhere that the patch is okay for me now. In case I didn't:
> 
> Acked-by: Kevin Wolf <kwolf@redhat.com>

Ah, must have missed it :). Thanks.

Stefano, could you please rebase? The patch doesn't apply cleanly anymore.


Alex

Re: [Qemu-devel] [PATCH v3] xen: implement unplug protocol in xen_platform

[Stefano Stabellini]

On Mon, 18 Jul 2011, Alexander Graf wrote:
> On 15.07.2011, at 12:52, Kevin Wolf wrote:
> 
> > Am 15.07.2011 12:34, schrieb Stefano Stabellini:
> >> On Fri, 1 Jul 2011, Stefano Stabellini wrote:
> >>> On Fri, 1 Jul 2011, Kevin Wolf wrote:
> >>>> Am 30.06.2011 16:16, schrieb Stefano Stabellini:
> >>>>> On Thu, 30 Jun 2011, Kevin Wolf wrote:
> >>>>>>> +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;
> >>>>>> 
> >>>>>> Have you tested if this is enough if the guest tries to continue using
> >>>>>> the device? I don't know of any case where it's not sufficient, just
> >>>>>> trying to make sure that it's really true in practice.
> >>>>> 
> >>>>> The purpose of this is to "hide" the disk from the guest. The unplug is
> >>>>> supposed to happen *before* the guest enumerates the IDE disks; it is
> >>>>> responsibility of the guest to make sure of it.
> >>>>> I tested it with Linux PV on HVM drivers, and Linux doesn't see the
> >>>>> emulated disk after the unplug, as it should be.
> >>>> 
> >>>> Yeah. What I meant is that we should make sure that a misbehaving guest,
> >>>> which just keeps on playing with the IDE ports anyway, can't crash qemu.
> >>>> A quick review suggests that it is the case, but testing it anyway would
> >>>> be better.
> >>> 
> >>> I see what you mean: I tested it, a guest cannot crash Qemu.
> >>> 
> >> 
> >> ping?
> > 
> > I thought Alex had already merged it. I'm pretty sure that I stated
> > somewhere that the patch is okay for me now. In case I didn't:
> > 
> > Acked-by: Kevin Wolf <kwolf@redhat.com>
> 
> Ah, must have missed it :). Thanks.
> 
> Stefano, could you please rebase? The patch doesn't apply cleanly anymore.
 
OK, I'll send it again based on your latest xen-next branch.