Am 5. Juli 2023 10:01:21 UTC schrieb Olaf Hering <[email protected]>:
>Tue, 4 Jul 2023 08:38:33 +0200 Paolo Bonzini <[email protected]>:
>
>> I agree that calling pci_device_reset() would be a better match for
>> pci_xen_ide_unplug().
>
>This change works as well:
Nice!
>
>--- a/hw/i386/xen/xen_platform.c
>+++ b/hw/i386/xen/xen_platform.c
>@@ -164,8 +164,9 @@ static void pci_unplug_nics(PCIBus *bus)
> *
> * [1]
> https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/hvm-emulated-unplug.pandoc
> */
>-static void pci_xen_ide_unplug(DeviceState *dev, bool aux)
>+static void pci_xen_ide_unplug(PCIDevice *d, bool aux)
> {
>+ DeviceState *dev = DEVICE(d);
> PCIIDEState *pci_ide;
> int i;
> IDEDevice *idedev;
>@@ -195,7 +196,7 @@ static void pci_xen_ide_unplug(DeviceState *dev, bool aux)
> blk_unref(blk);
> }
> }
>- device_cold_reset(dev);
>+ pci_device_reset(d);
> }
>
> static void unplug_disks(PCIBus *b, PCIDevice *d, void *opaque)
>@@ -210,7 +211,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_xen_ide_unplug(DEVICE(d), aux);
>+ pci_xen_ide_unplug(d, aux);
> break;
>
> case PCI_CLASS_STORAGE_SCSI:
>--- a/hw/ide/piix.c
>+++ b/hw/ide/piix.c
>@@ -118,7 +118,6 @@ static void piix_ide_reset(DeviceState *dev)
> pci_set_word(pci_conf + PCI_COMMAND, 0x0000);
> pci_set_word(pci_conf + PCI_STATUS,
> PCI_STATUS_DEVSEL_MEDIUM | PCI_STATUS_FAST_BACK);
>- pci_set_byte(pci_conf + 0x20, 0x01); /* BMIBA: 20-23h */
I wonder if we should fix this line rather than dropping it. pci_device_reset()
calls pci_reset_regions() which unconditionally clears all BARs to zero. While
that works for PIIX IDE the VIA IDE device model intends to set BARs to the IDE
compatibility addresses during reset but pci_reset_regions() overwrites it with
zeroes again. So I wonder if pci_reset_regions() should be dropped such that
pci_update_mappings() resets the BARs to whatever they were set in reset.
Of course this won't be an easy change but I wonder if it was more correct,
especially since there seems to be no way to have the device model have the
last word. Any opinions/suggestions?
Thanks,
Bernhard
> }
>
> static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
>
>
>Olaf
