Re: [PATCH v2] hw/ide/piix: properly initialize the BMIBA register

[Bernhard Beschow]

Am 5. Juli 2023 10:01:21 UTC schrieb Olaf Hering <olaf@aepfle.de>:
>Tue, 4 Jul 2023 08:38:33 +0200 Paolo Bonzini <pbonzini@redhat.com>:
>
>> 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