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