[PATCH 0/3] PIIX3-IDE XEN cleanup

[PATCH 0/3] PIIX3-IDE XEN cleanup

[Bernhard Beschow]

This patch series first removes the redundant "piix3-ide-xen" device class and
then moves a XEN-specific helper function from PIIX3 code to XEN code. The idea
is to decouple PIIX3-IDE and XEN and to compile XEN-specific bits only if XEN
support is enabled.

Testing done:
'qemu-system-x86_64 -M pc -m 1G -cdrom archlinux-2022.05.01-x86_64.iso" boots
successfully and a 'poweroff' inside the VM also shuts it down correctly.

XEN mode wasn't tested for the time being since its setup procedure seems quite
sophisticated. Please let me know in case this is an obstacle.

Bernhard Beschow (3):
  hw/ide/piix: Remove redundant "piix3-ide-xen" device class
  hw/ide/piix: Add some documentation to pci_piix3_xen_ide_unplug()
  include/hw/ide: Unexport pci_piix3_xen_ide_unplug()

 hw/i386/pc_piix.c          |  3 +--
 hw/i386/xen/xen_platform.c | 49 +++++++++++++++++++++++++++++++++++++-
 hw/ide/piix.c              | 42 --------------------------------
 include/hw/ide.h           |  3 ---
 4 files changed, 49 insertions(+), 48 deletions(-)

-- 
2.36.1

[PATCH 1/3] hw/ide/piix: Remove redundant "piix3-ide-xen" device class

[Bernhard Beschow]

Commit 0f8445820f11a69154309863960328dda3dc1ad4 'xen: piix reuse pci
generic class init function' already resolved redundant code which in
turn rendered piix3-ide-xen redundant.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/pc_piix.c | 3 +--
 hw/ide/piix.c     | 7 -------
 2 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 4c185c72d0..27dfde4917 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -244,8 +244,7 @@ static void pc_init1(MachineState *machine,
     if (pcmc->pci_enabled) {
         PCIDevice *dev;
 
-        dev = pci_create_simple(pci_bus, piix3_devfn + 1,
-                                xen_enabled() ? "piix3-ide-xen" : "piix3-ide");
+        dev = pci_create_simple(pci_bus, piix3_devfn + 1, "piix3-ide");
         pci_ide_create_devs(dev);
         idebus[0] = qdev_get_child_bus(&dev->qdev, "ide.0");
         idebus[1] = qdev_get_child_bus(&dev->qdev, "ide.1");
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index ce89fd0aa3..2345fe9e1d 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -241,12 +241,6 @@ static const TypeInfo piix3_ide_info = {
     .class_init    = piix3_ide_class_init,
 };
 
-static const TypeInfo piix3_ide_xen_info = {
-    .name          = "piix3-ide-xen",
-    .parent        = TYPE_PCI_IDE,
-    .class_init    = piix3_ide_class_init,
-};
-
 /* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
 static void piix4_ide_class_init(ObjectClass *klass, void *data)
 {
@@ -272,7 +266,6 @@ static const TypeInfo piix4_ide_info = {
 static void piix_ide_register_types(void)
 {
     type_register_static(&piix3_ide_info);
-    type_register_static(&piix3_ide_xen_info);
     type_register_static(&piix4_ide_info);
 }
 
-- 
2.36.1

Re: [PATCH 1/3] hw/ide/piix: Remove redundant "piix3-ide-xen" device class

[Michael S. Tsirkin]

On Sun, May 08, 2022 at 12:34:30PM +0200, Bernhard Beschow wrote:
> Commit 0f8445820f11a69154309863960328dda3dc1ad4 'xen: piix reuse pci
> generic class init function' already resolved redundant code which in
> turn rendered piix3-ide-xen redundant.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

Cc xen maintainers for review please.

> ---
>  hw/i386/pc_piix.c | 3 +--
>  hw/ide/piix.c     | 7 -------
>  2 files changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 4c185c72d0..27dfde4917 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -244,8 +244,7 @@ static void pc_init1(MachineState *machine,
>      if (pcmc->pci_enabled) {
>          PCIDevice *dev;
>  
> -        dev = pci_create_simple(pci_bus, piix3_devfn + 1,
> -                                xen_enabled() ? "piix3-ide-xen" : "piix3-ide");
> +        dev = pci_create_simple(pci_bus, piix3_devfn + 1, "piix3-ide");
>          pci_ide_create_devs(dev);
>          idebus[0] = qdev_get_child_bus(&dev->qdev, "ide.0");
>          idebus[1] = qdev_get_child_bus(&dev->qdev, "ide.1");
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index ce89fd0aa3..2345fe9e1d 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -241,12 +241,6 @@ static const TypeInfo piix3_ide_info = {
>      .class_init    = piix3_ide_class_init,
>  };
>  
> -static const TypeInfo piix3_ide_xen_info = {
> -    .name          = "piix3-ide-xen",
> -    .parent        = TYPE_PCI_IDE,
> -    .class_init    = piix3_ide_class_init,
> -};
> -
>  /* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
>  static void piix4_ide_class_init(ObjectClass *klass, void *data)
>  {
> @@ -272,7 +266,6 @@ static const TypeInfo piix4_ide_info = {
>  static void piix_ide_register_types(void)
>  {
>      type_register_static(&piix3_ide_info);
> -    type_register_static(&piix3_ide_xen_info);
>      type_register_static(&piix4_ide_info);
>  }
>  
> -- 
> 2.36.1

[PATCH 2/3] hw/ide/piix: Add some documentation to pci_piix3_xen_ide_unplug()

[Bernhard Beschow]

The comment is based on commit message
ae4d2eb273b167dad748ea4249720319240b1ac2 'xen-platform: add missing disk
unplug option'. Since it seems to describe design decisions and
limitations that still apply it seems worth having.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/ide/piix.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 2345fe9e1d..bc1b37512a 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -173,6 +173,17 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
     }
 }
 
+/*
+ * The Xen HVM unplug protocol [1] specifies a mechanism to allow guests to
+ * request unplug of 'aux' disks (which is stated to mean all IDE disks,
+ * except the primary master).
+ *
+ * NOTE: The semantics of what happens if unplug of all disks and 'aux' disks
+ *       is simultaneously requested is not clear. The implementation assumes
+ *       that an 'all' request overrides an 'aux' request.
+ *
+ * [1] https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/hvm-emulated-unplug.pandoc
+ */
 int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux)
 {
     PCIIDEState *pci_ide;
-- 
2.36.1

[PATCH 3/3] include/hw/ide: Unexport pci_piix3_xen_ide_unplug()

[Bernhard Beschow]

This function was declared in a generic and public header, implemented
in a device-specific source file but only used in xen_platform. Given its
'aux' parameter, this function is more xen-specific than piix-specific.
Also, the hardcoded magic constants seem to be generic and related to
PCIIDEState and IDEBus rather than piix.

Therefore, move this function to xen_platform, unexport it, and drop the
"piix3" in the function name as well.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/xen/xen_platform.c | 49 +++++++++++++++++++++++++++++++++++++-
 hw/ide/piix.c              | 46 -----------------------------------
 include/hw/ide.h           |  3 ---
 3 files changed, 48 insertions(+), 50 deletions(-)

diff --git a/hw/i386/xen/xen_platform.c b/hw/i386/xen/xen_platform.c
index 72028449ba..124ffeae35 100644
--- a/hw/i386/xen/xen_platform.c
+++ b/hw/i386/xen/xen_platform.c
@@ -26,6 +26,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "hw/ide.h"
+#include "hw/ide/pci.h"
 #include "hw/pci/pci.h"
 #include "hw/xen/xen_common.h"
 #include "migration/vmstate.h"
@@ -134,6 +135,52 @@ static void pci_unplug_nics(PCIBus *bus)
     pci_for_each_device(bus, 0, unplug_nic, NULL);
 }
 
+/*
+ * The Xen HVM unplug protocol [1] specifies a mechanism to allow guests to
+ * request unplug of 'aux' disks (which is stated to mean all IDE disks,
+ * except the primary master).
+ *
+ * NOTE: The semantics of what happens if unplug of all disks and 'aux' disks
+ *       is simultaneously requested is not clear. The implementation assumes
+ *       that an 'all' request overrides an 'aux' request.
+ *
+ * [1] https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/hvm-emulated-unplug.pandoc
+ */
+static int pci_xen_ide_unplug(DeviceState *dev, bool aux)
+{
+    PCIIDEState *pci_ide;
+    int i;
+    IDEDevice *idedev;
+    IDEBus *idebus;
+    BlockBackend *blk;
+
+    pci_ide = PCI_IDE(dev);
+
+    for (i = aux ? 1 : 0; i < 4; i++) {
+        idebus = &pci_ide->bus[i / 2];
+        blk = idebus->ifs[i % 2].blk;
+
+        if (blk && idebus->ifs[i % 2].drive_kind != IDE_CD) {
+            if (!(i % 2)) {
+                idedev = idebus->master;
+            } else {
+                idedev = idebus->slave;
+            }
+
+            blk_drain(blk);
+            blk_flush(blk);
+
+            blk_detach_dev(blk, DEVICE(idedev));
+            idebus->ifs[i % 2].blk = NULL;
+            idedev->conf.blk = NULL;
+            monitor_remove_blk(blk);
+            blk_unref(blk);
+        }
+    }
+    qdev_reset_all(dev);
+    return 0;
+}
+
 static void unplug_disks(PCIBus *b, PCIDevice *d, void *opaque)
 {
     uint32_t flags = *(uint32_t *)opaque;
@@ -147,7 +194,7 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void *opaque)
 
     switch (pci_get_word(d->config + PCI_CLASS_DEVICE)) {
     case PCI_CLASS_STORAGE_IDE:
-        pci_piix3_xen_ide_unplug(DEVICE(d), aux);
+        pci_xen_ide_unplug(DEVICE(d), aux);
         break;
 
     case PCI_CLASS_STORAGE_SCSI:
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index bc1b37512a..9a9b28078e 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -173,52 +173,6 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
     }
 }
 
-/*
- * The Xen HVM unplug protocol [1] specifies a mechanism to allow guests to
- * request unplug of 'aux' disks (which is stated to mean all IDE disks,
- * except the primary master).
- *
- * NOTE: The semantics of what happens if unplug of all disks and 'aux' disks
- *       is simultaneously requested is not clear. The implementation assumes
- *       that an 'all' request overrides an 'aux' request.
- *
- * [1] https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/hvm-emulated-unplug.pandoc
- */
-int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux)
-{
-    PCIIDEState *pci_ide;
-    int i;
-    IDEDevice *idedev;
-    IDEBus *idebus;
-    BlockBackend *blk;
-
-    pci_ide = PCI_IDE(dev);
-
-    for (i = aux ? 1 : 0; i < 4; i++) {
-        idebus = &pci_ide->bus[i / 2];
-        blk = idebus->ifs[i % 2].blk;
-
-        if (blk && idebus->ifs[i % 2].drive_kind != IDE_CD) {
-            if (!(i % 2)) {
-                idedev = idebus->master;
-            } else {
-                idedev = idebus->slave;
-            }
-
-            blk_drain(blk);
-            blk_flush(blk);
-
-            blk_detach_dev(blk, DEVICE(idedev));
-            idebus->ifs[i % 2].blk = NULL;
-            idedev->conf.blk = NULL;
-            monitor_remove_blk(blk);
-            blk_unref(blk);
-        }
-    }
-    qdev_reset_all(dev);
-    return 0;
-}
-
 static void pci_piix_ide_exitfn(PCIDevice *dev)
 {
     PCIIDEState *d = PCI_IDE(dev);
diff --git a/include/hw/ide.h b/include/hw/ide.h
index c5ce5da4f4..60f1f4f714 100644
--- a/include/hw/ide.h
+++ b/include/hw/ide.h
@@ -8,9 +8,6 @@
 ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int isairq,
                         DriveInfo *hd0, DriveInfo *hd1);
 
-/* ide-pci.c */
-int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux);
-
 /* ide-mmio.c */
 void mmio_ide_init_drives(DeviceState *dev, DriveInfo *hd0, DriveInfo *hd1);
 
-- 
2.36.1

Re: [PATCH 3/3] include/hw/ide: Unexport pci_piix3_xen_ide_unplug()

[Durrant, Paul]

On 08/05/2022 11:34, Bernhard Beschow wrote:
> This function was declared in a generic and public header, implemented
> in a device-specific source file but only used in xen_platform. Given its
> 'aux' parameter, this function is more xen-specific than piix-specific.
> Also, the hardcoded magic constants seem to be generic and related to
> PCIIDEState and IDEBus rather than piix.
> 
> Therefore, move this function to xen_platform, unexport it, and drop the
> "piix3" in the function name as well.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

Reviewed-by: Paul Durrant <paul@xen.org>

... with one suggestion...

> ---
>   hw/i386/xen/xen_platform.c | 49 +++++++++++++++++++++++++++++++++++++-
>   hw/ide/piix.c              | 46 -----------------------------------
>   include/hw/ide.h           |  3 ---
>   3 files changed, 48 insertions(+), 50 deletions(-)
> 
> diff --git a/hw/i386/xen/xen_platform.c b/hw/i386/xen/xen_platform.c
> index 72028449ba..124ffeae35 100644
> --- a/hw/i386/xen/xen_platform.c
> +++ b/hw/i386/xen/xen_platform.c
> @@ -26,6 +26,7 @@
>   #include "qemu/osdep.h"
>   #include "qapi/error.h"
>   #include "hw/ide.h"
> +#include "hw/ide/pci.h"
>   #include "hw/pci/pci.h"
>   #include "hw/xen/xen_common.h"
>   #include "migration/vmstate.h"
> @@ -134,6 +135,52 @@ static void pci_unplug_nics(PCIBus *bus)
>       pci_for_each_device(bus, 0, unplug_nic, NULL);
>   }
>   
> +/*
> + * The Xen HVM unplug protocol [1] specifies a mechanism to allow guests to
> + * request unplug of 'aux' disks (which is stated to mean all IDE disks,
> + * except the primary master).
> + *
> + * NOTE: The semantics of what happens if unplug of all disks and 'aux' disks
> + *       is simultaneously requested is not clear. The implementation assumes
> + *       that an 'all' request overrides an 'aux' request.
> + *
> + * [1] https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/hvm-emulated-unplug.pandoc
> + */
> +static int pci_xen_ide_unplug(DeviceState *dev, bool aux)
> +{
> +    PCIIDEState *pci_ide;
> +    int i;
> +    IDEDevice *idedev;
> +    IDEBus *idebus;
> +    BlockBackend *blk;
> +
> +    pci_ide = PCI_IDE(dev);
> +
> +    for (i = aux ? 1 : 0; i < 4; i++) {
> +        idebus = &pci_ide->bus[i / 2];
> +        blk = idebus->ifs[i % 2].blk;
> +
> +        if (blk && idebus->ifs[i % 2].drive_kind != IDE_CD) {
> +            if (!(i % 2)) {
> +                idedev = idebus->master;
> +            } else {
> +                idedev = idebus->slave;
> +            }
> +
> +            blk_drain(blk);
> +            blk_flush(blk);
> +
> +            blk_detach_dev(blk, DEVICE(idedev));
> +            idebus->ifs[i % 2].blk = NULL;
> +            idedev->conf.blk = NULL;
> +            monitor_remove_blk(blk);
> +            blk_unref(blk);
> +        }
> +    }
> +    qdev_reset_all(dev);
> +    return 0;

The return value is ignored so you may as well make this a static void 
function.

   Paul

> +}
> +
>   static void unplug_disks(PCIBus *b, PCIDevice *d, void *opaque)
>   {
>       uint32_t flags = *(uint32_t *)opaque;
> @@ -147,7 +194,7 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void *opaque)
>   
>       switch (pci_get_word(d->config + PCI_CLASS_DEVICE)) {
>       case PCI_CLASS_STORAGE_IDE:
> -        pci_piix3_xen_ide_unplug(DEVICE(d), aux);
> +        pci_xen_ide_unplug(DEVICE(d), aux);
>           break;
>   
>       case PCI_CLASS_STORAGE_SCSI:
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index bc1b37512a..9a9b28078e 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -173,52 +173,6 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
>       }
>   }
>   
> -/*
> - * The Xen HVM unplug protocol [1] specifies a mechanism to allow guests to
> - * request unplug of 'aux' disks (which is stated to mean all IDE disks,
> - * except the primary master).
> - *
> - * NOTE: The semantics of what happens if unplug of all disks and 'aux' disks
> - *       is simultaneously requested is not clear. The implementation assumes
> - *       that an 'all' request overrides an 'aux' request.
> - *
> - * [1] https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/hvm-emulated-unplug.pandoc
> - */
> -int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux)
> -{
> -    PCIIDEState *pci_ide;
> -    int i;
> -    IDEDevice *idedev;
> -    IDEBus *idebus;
> -    BlockBackend *blk;
> -
> -    pci_ide = PCI_IDE(dev);
> -
> -    for (i = aux ? 1 : 0; i < 4; i++) {
> -        idebus = &pci_ide->bus[i / 2];
> -        blk = idebus->ifs[i % 2].blk;
> -
> -        if (blk && idebus->ifs[i % 2].drive_kind != IDE_CD) {
> -            if (!(i % 2)) {
> -                idedev = idebus->master;
> -            } else {
> -                idedev = idebus->slave;
> -            }
> -
> -            blk_drain(blk);
> -            blk_flush(blk);
> -
> -            blk_detach_dev(blk, DEVICE(idedev));
> -            idebus->ifs[i % 2].blk = NULL;
> -            idedev->conf.blk = NULL;
> -            monitor_remove_blk(blk);
> -            blk_unref(blk);
> -        }
> -    }
> -    qdev_reset_all(dev);
> -    return 0;
> -}
> -
>   static void pci_piix_ide_exitfn(PCIDevice *dev)
>   {
>       PCIIDEState *d = PCI_IDE(dev);
> diff --git a/include/hw/ide.h b/include/hw/ide.h
> index c5ce5da4f4..60f1f4f714 100644
> --- a/include/hw/ide.h
> +++ b/include/hw/ide.h
> @@ -8,9 +8,6 @@
>   ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int isairq,
>                           DriveInfo *hd0, DriveInfo *hd1);
>   
> -/* ide-pci.c */
> -int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux);
> -
>   /* ide-mmio.c */
>   void mmio_ide_init_drives(DeviceState *dev, DriveInfo *hd0, DriveInfo *hd1);
>   

Re: [PATCH 3/3] include/hw/ide: Unexport pci_piix3_xen_ide_unplug()

[Bernhard Beschow]

Am 9. Mai 2022 08:02:13 UTC schrieb "Durrant, Paul" <xadimgnik@gmail.com>:
>On 08/05/2022 11:34, Bernhard Beschow wrote:
>> This function was declared in a generic and public header, implemented
>> in a device-specific source file but only used in xen_platform. Given its
>> 'aux' parameter, this function is more xen-specific than piix-specific.
>> Also, the hardcoded magic constants seem to be generic and related to
>> PCIIDEState and IDEBus rather than piix.
>> 
>> Therefore, move this function to xen_platform, unexport it, and drop the
>> "piix3" in the function name as well.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>
>Reviewed-by: Paul Durrant <paul@xen.org>
>
>... with one suggestion...
>
>> ---
>>   hw/i386/xen/xen_platform.c | 49 +++++++++++++++++++++++++++++++++++++-
>>   hw/ide/piix.c              | 46 -----------------------------------
>>   include/hw/ide.h           |  3 ---
>>   3 files changed, 48 insertions(+), 50 deletions(-)
>> 
>> diff --git a/hw/i386/xen/xen_platform.c b/hw/i386/xen/xen_platform.c
>> index 72028449ba..124ffeae35 100644
>> --- a/hw/i386/xen/xen_platform.c
>> +++ b/hw/i386/xen/xen_platform.c
>> @@ -26,6 +26,7 @@
>>   #include "qemu/osdep.h"
>>   #include "qapi/error.h"
>>   #include "hw/ide.h"
>> +#include "hw/ide/pci.h"
>>   #include "hw/pci/pci.h"
>>   #include "hw/xen/xen_common.h"
>>   #include "migration/vmstate.h"
>> @@ -134,6 +135,52 @@ static void pci_unplug_nics(PCIBus *bus)
>>       pci_for_each_device(bus, 0, unplug_nic, NULL);
>>   }
>>   +/*
>> + * The Xen HVM unplug protocol [1] specifies a mechanism to allow guests to
>> + * request unplug of 'aux' disks (which is stated to mean all IDE disks,
>> + * except the primary master).
>> + *
>> + * NOTE: The semantics of what happens if unplug of all disks and 'aux' disks
>> + *       is simultaneously requested is not clear. The implementation assumes
>> + *       that an 'all' request overrides an 'aux' request.
>> + *
>> + * [1] https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/hvm-emulated-unplug.pandoc
>> + */
>> +static int pci_xen_ide_unplug(DeviceState *dev, bool aux)
>> +{
>> +    PCIIDEState *pci_ide;
>> +    int i;
>> +    IDEDevice *idedev;
>> +    IDEBus *idebus;
>> +    BlockBackend *blk;
>> +
>> +    pci_ide = PCI_IDE(dev);
>> +
>> +    for (i = aux ? 1 : 0; i < 4; i++) {
>> +        idebus = &pci_ide->bus[i / 2];
>> +        blk = idebus->ifs[i % 2].blk;
>> +
>> +        if (blk && idebus->ifs[i % 2].drive_kind != IDE_CD) {
>> +            if (!(i % 2)) {
>> +                idedev = idebus->master;
>> +            } else {
>> +                idedev = idebus->slave;
>> +            }
>> +
>> +            blk_drain(blk);
>> +            blk_flush(blk);
>> +
>> +            blk_detach_dev(blk, DEVICE(idedev));
>> +            idebus->ifs[i % 2].blk = NULL;
>> +            idedev->conf.blk = NULL;
>> +            monitor_remove_blk(blk);
>> +            blk_unref(blk);
>> +        }
>> +    }
>> +    qdev_reset_all(dev);
>> +    return 0;
>
>The return value is ignored so you may as well make this a static void function.

Good catch! I'll prepare a v2. Meanwhile, I'm looking forward to comments on the other patches as well.

Thanks,
Bernhard

>  Paul
>
>> +}
>> +
>>   static void unplug_disks(PCIBus *b, PCIDevice *d, void *opaque)
>>   {
>>       uint32_t flags = *(uint32_t *)opaque;
>> @@ -147,7 +194,7 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void *opaque)
>>         switch (pci_get_word(d->config + PCI_CLASS_DEVICE)) {
>>       case PCI_CLASS_STORAGE_IDE:
>> -        pci_piix3_xen_ide_unplug(DEVICE(d), aux);
>> +        pci_xen_ide_unplug(DEVICE(d), aux);
>>           break;
>>         case PCI_CLASS_STORAGE_SCSI:
>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>> index bc1b37512a..9a9b28078e 100644
>> --- a/hw/ide/piix.c
>> +++ b/hw/ide/piix.c
>> @@ -173,52 +173,6 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
>>       }
>>   }
>>   -/*
>> - * The Xen HVM unplug protocol [1] specifies a mechanism to allow guests to
>> - * request unplug of 'aux' disks (which is stated to mean all IDE disks,
>> - * except the primary master).
>> - *
>> - * NOTE: The semantics of what happens if unplug of all disks and 'aux' disks
>> - *       is simultaneously requested is not clear. The implementation assumes
>> - *       that an 'all' request overrides an 'aux' request.
>> - *
>> - * [1] https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/hvm-emulated-unplug.pandoc
>> - */
>> -int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux)
>> -{
>> -    PCIIDEState *pci_ide;
>> -    int i;
>> -    IDEDevice *idedev;
>> -    IDEBus *idebus;
>> -    BlockBackend *blk;
>> -
>> -    pci_ide = PCI_IDE(dev);
>> -
>> -    for (i = aux ? 1 : 0; i < 4; i++) {
>> -        idebus = &pci_ide->bus[i / 2];
>> -        blk = idebus->ifs[i % 2].blk;
>> -
>> -        if (blk && idebus->ifs[i % 2].drive_kind != IDE_CD) {
>> -            if (!(i % 2)) {
>> -                idedev = idebus->master;
>> -            } else {
>> -                idedev = idebus->slave;
>> -            }
>> -
>> -            blk_drain(blk);
>> -            blk_flush(blk);
>> -
>> -            blk_detach_dev(blk, DEVICE(idedev));
>> -            idebus->ifs[i % 2].blk = NULL;
>> -            idedev->conf.blk = NULL;
>> -            monitor_remove_blk(blk);
>> -            blk_unref(blk);
>> -        }
>> -    }
>> -    qdev_reset_all(dev);
>> -    return 0;
>> -}
>> -
>>   static void pci_piix_ide_exitfn(PCIDevice *dev)
>>   {
>>       PCIIDEState *d = PCI_IDE(dev);
>> diff --git a/include/hw/ide.h b/include/hw/ide.h
>> index c5ce5da4f4..60f1f4f714 100644
>> --- a/include/hw/ide.h
>> +++ b/include/hw/ide.h
>> @@ -8,9 +8,6 @@
>>   ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int isairq,
>>                           DriveInfo *hd0, DriveInfo *hd1);
>>   -/* ide-pci.c */
>> -int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux);
>> -
>>   /* ide-mmio.c */
>>   void mmio_ide_init_drives(DeviceState *dev, DriveInfo *hd0, DriveInfo *hd1);
>