diff mbox series

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

Message ID 20220508103432.14874-4-shentey@gmail.com (mailing list archive)
State New
Headers show
Series None | expand

Commit Message

BB May 8, 2022, 10:34 a.m. UTC
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(-)

Comments

Durrant, Paul May 9, 2022, 8:02 a.m. UTC | #1
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);
>
BB May 9, 2022, 10:01 a.m. UTC | #2
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);
>
diff mbox series

Patch

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