[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 3/3] include/hw/ide: Unexport pci_piix3_xen_ide_unplug()
From: |
Bernhard Beschow |
Subject: |
Re: [PATCH 3/3] include/hw/ide: Unexport pci_piix3_xen_ide_unplug() |
Date: |
Mon, 09 May 2022 10:01:28 +0000 |
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);
>