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

According to the 82371FB documentation (82371FB.pdf, 2.3.9. BMIBA—BUS
MASTER INTERFACE BASE ADDRESS REGISTER, April 1997), the register is
32bit wide. To properly reset it to default values, all 32bit need to be
cleared. Bit #0 "Resource Type Indicator (RTE)" needs to be enabled.

The initial change wrote just the lower 8 bit, leaving parts of the "Bus
Master Interface Base Address" address at bit 15:4 unchanged.

This bug went unnoticed until commit ee358e919e38 ("hw/ide/piix: Convert
reset handler to DeviceReset"). After this change, piix_ide_reset is
exercised after the "unplug" command from a Xen HVM domU, which was not
the case prior that commit. This function resets the command register.
As a result the ata_piix driver inside the domU will see a disabled PCI
device. The generic PCI code will reenable the PCI device. On the qemu
side, this runs pci_default_write_config/pci_update_mappings. Here a
changed address is returned by pci_bar_address, this is the address
which was truncated in piix_ide_reset. In case of a Xen HVM domU, the
address changes from 0xc120 to 0xc100.

While the unplug is supposed to hide the IDE disks, the changed BMIBA
address breaks the UHCI device. In case the domU has an USB tablet
configured, to recive absolute pointer coordinates for the GUI, it will
cause a hang during device discovery of the partly discovered USB hid
device. Reading the USBSTS word size register will fail. The access ends
up in the QEMU piix-bmdma device, instead of the expected uhci device.
Here a byte size request is expected, and a value of ~0 is returned. As
a result the UCHI driver sees an error state in the register, and turns
off the UHCI controller.

Fixes: e6a71ae327 ("Add support for 82371FB (Step A1) and Improved support for 82371SB (Function 1)")

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 hw/ide/piix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


base-commit: d145c0da22cde391d8c6672d33146ce306e8bf75

Am 1. Juli 2023 17:46:59 UTC schrieb Olaf Hering <olaf@aepfle.de>:
>According to the 82371FB documentation (82371FB.pdf, 2.3.9. BMIBA—BUS
>MASTER INTERFACE BASE ADDRESS REGISTER, April 1997), the register is
>32bit wide. To properly reset it to default values, all 32bit need to be
>cleared. Bit #0 "Resource Type Indicator (RTE)" needs to be enabled.
>
>The initial change wrote just the lower 8 bit, leaving parts of the "Bus
>Master Interface Base Address" address at bit 15:4 unchanged.
>
>This bug went unnoticed until commit ee358e919e38 ("hw/ide/piix: Convert
>reset handler to DeviceReset"). After this change, piix_ide_reset is
>exercised after the "unplug" command from a Xen HVM domU, 

Do you know if that command calls pci_device_reset() (which would eventually call piix_ide_reset())? Or does it call the DeviceReset handler directly? I'm asking because pci_device_reset() would take care of resetting the BMIBA register as well as disabling the BAR. IOW I'm wondering if the Xen command does the right thing.

Best regards,
Bernhard

>which was not
>the case prior that commit. This function resets the command register.
>As a result the ata_piix driver inside the domU will see a disabled PCI
>device. The generic PCI code will reenable the PCI device. On the qemu
>side, this runs pci_default_write_config/pci_update_mappings. Here a
>changed address is returned by pci_bar_address, this is the address
>which was truncated in piix_ide_reset. In case of a Xen HVM domU, the
>address changes from 0xc120 to 0xc100.
>
>While the unplug is supposed to hide the IDE disks, the changed BMIBA
>address breaks the UHCI device. In case the domU has an USB tablet
>configured, to recive absolute pointer coordinates for the GUI, it will
>cause a hang during device discovery of the partly discovered USB hid
>device. Reading the USBSTS word size register will fail. The access ends
>up in the QEMU piix-bmdma device, instead of the expected uhci device.
>Here a byte size request is expected, and a value of ~0 is returned. As
>a result the UCHI driver sees an error state in the register, and turns
>off the UHCI controller.
>
>Fixes: e6a71ae327 ("Add support for 82371FB (Step A1) and Improved support for 82371SB (Function 1)")
>
>Signed-off-by: Olaf Hering <olaf@aepfle.de>
>---
> hw/ide/piix.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>index 41d60921e3..1e346b1b1d 100644
>--- a/hw/ide/piix.c
>+++ b/hw/ide/piix.c
>@@ -118,7 +118,7 @@ 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 */
>+    pci_set_long(pci_conf + 0x20, 0x01);  /* BMIBA: 20-23h */
> }
> 
> static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
>
>base-commit: d145c0da22cde391d8c6672d33146ce306e8bf75
>

Sun, 02 Jul 2023 22:18:50 +0000 Bernhard Beschow <shentey@gmail.com>:

> Do you know if that command calls pci_device_reset() (which would eventually call piix_ide_reset())?

The function is pci_xen_ide_unplug, which calls device_cold_reset.


Olaf

Am 3. Juli 2023 07:59:29 UTC schrieb Olaf Hering <olaf@aepfle.de>:
>Sun, 02 Jul 2023 22:18:50 +0000 Bernhard Beschow <shentey@gmail.com>:
>
>> Do you know if that command calls pci_device_reset() (which would eventually call piix_ide_reset())?
>
>The function is pci_xen_ide_unplug, which calls device_cold_reset.

I think this explains why the BAR isn't reset: Unlike pci_device_reset(), device_cold_reset() lacks calling pci_do_device_reset() which would take care of the BARs.

Paolo, Peter: Should we switch to pci_device_reset() in pci_xen_ide_unplug()? Or is device_cold_reset() supposed to do everything?

Best regards,
Bernhard

>
>
>Olaf

On 7/3/23 22:33, Bernhard Beschow wrote:
> Paolo, Peter: Should we switch to pci_device_reset() in
> pci_xen_ide_unplug()? Or is device_cold_reset() supposed to do
> everything?

device_cold_reset() does not reset state that is part of the bus, so I 
think it's consistent that it doesn't call pci_do_device_reset().

I agree that calling pci_device_reset() would be a better match for 
pci_xen_ide_unplug().

Paolo

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:

--- 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 */
 }
 
 static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)


Olaf

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

Wed, 05 Jul 2023 21:52:05 +0000 Bernhard Beschow <shentey@gmail.com>:

> I wonder if we should fix this line rather than dropping it.

I think this needs to be fixed, just to fix the initial commit which
added this bug. This will allow backporting this oneliner.

Further changes to pci_xen_ide_unplug will be done in a separate patch.


Olaf

Am 1. Juli 2023 17:46:59 UTC schrieb Olaf Hering <olaf@aepfle.de>:
>According to the 82371FB documentation (82371FB.pdf, 2.3.9. BMIBA—BUS
>MASTER INTERFACE BASE ADDRESS REGISTER, April 1997), the register is
>32bit wide. To properly reset it to default values, all 32bit need to be
>cleared. Bit #0 "Resource Type Indicator (RTE)" needs to be enabled.
>
>The initial change wrote just the lower 8 bit, leaving parts of the "Bus
>Master Interface Base Address" address at bit 15:4 unchanged.

For v3 you could cut the following paragraphs from this commit message ...

>
>This bug went unnoticed until commit ee358e919e38 ("hw/ide/piix: Convert
>reset handler to DeviceReset"). After this change, piix_ide_reset is
>exercised after the "unplug" command from a Xen HVM domU, which was not
>the case prior that commit. This function resets the command register.
>As a result the ata_piix driver inside the domU will see a disabled PCI
>device. The generic PCI code will reenable the PCI device. On the qemu
>side, this runs pci_default_write_config/pci_update_mappings. Here a
>changed address is returned by pci_bar_address, this is the address
>which was truncated in piix_ide_reset. In case of a Xen HVM domU, the
>address changes from 0xc120 to 0xc100.
>
>While the unplug is supposed to hide the IDE disks, the changed BMIBA
>address breaks the UHCI device. In case the domU has an USB tablet
>configured, to recive absolute pointer coordinates for the GUI, it will
>cause a hang during device discovery of the partly discovered USB hid
>device. Reading the USBSTS word size register will fail. The access ends
>up in the QEMU piix-bmdma device, instead of the expected uhci device.
>Here a byte size request is expected, and a value of ~0 is returned. As
>a result the UCHI driver sees an error state in the register, and turns
>off the UHCI controller.

... until here and paste them into the patch with the Xen fix.

>
>Fixes: e6a71ae327 ("Add support for 82371FB (Step A1) and Improved support for 82371SB (Function 1)")
>
>Signed-off-by: Olaf Hering <olaf@aepfle.de>

With the changed commit message:

Reviewed-by: Bernhard Beschow <shentey@gmail.com>

>---
> hw/ide/piix.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>index 41d60921e3..1e346b1b1d 100644
>--- a/hw/ide/piix.c
>+++ b/hw/ide/piix.c
>@@ -118,7 +118,7 @@ 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 */
>+    pci_set_long(pci_conf + 0x20, 0x01);  /* BMIBA: 20-23h */
> }
> 
> static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
>
>base-commit: d145c0da22cde391d8c6672d33146ce306e8bf75
>

Am 11. Juli 2023 09:11:33 UTC schrieb Olaf Hering <olaf@aepfle.de>:
>Wed, 05 Jul 2023 21:52:05 +0000 Bernhard Beschow <shentey@gmail.com>:
>
>> I wonder if we should fix this line rather than dropping it.
>
>I think this needs to be fixed, just to fix the initial commit which
>added this bug. This will allow backporting this oneliner.
>
>Further changes to pci_xen_ide_unplug will be done in a separate patch.

Sounds good to me. You've got my R-b for this patch with small changes in the commit message.

Best regards,
Bernhard

>
>
>Olaf

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:
>
>--- 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:

Hi Olaf,

Would you mind sending this patch as well? The PIIX fix alone just fixes the syptom, not the underlying problem. The underlying problem is that the BAR isn't deactivated, and with the PIIX patch it will stay at address zero rather than in the USB function address range.

I think this patch should ideally land in 8.1, and IIUC bug fixes are still accepted for it.

Best regards,
Bernhard

>--- 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 */
> }
> 
> static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
>
>
>Olaf

Mon, 17 Jul 2023 08:46:16 +0000 Bernhard Beschow <shentey@gmail.com>:

> Would you mind sending this patch as well? 

Sure, I was waiting for the other change to appear in the master branch,
so I can reference it in the new commit message.


Olaf

Mon, 17 Jul 2023 08:46:16 +0000 Bernhard Beschow <shentey@gmail.com>:

> The PIIX fix alone just fixes the syptom, not the underlying problem. The underlying problem is that the BAR isn't deactivated, and with the PIIX patch it will stay at address zero rather than in the USB function address range.

Did you mean to say USB will not work now? It actually does with just 230dfd9257 backported to v4.2+.

Either way, I will test and send the additional change for pci_xen_ide_unplug in the next days.


Olaf

Am 17. Juli 2023 11:03:38 UTC schrieb Olaf Hering <olaf@aepfle.de>:
>Mon, 17 Jul 2023 08:46:16 +0000 Bernhard Beschow <shentey@gmail.com>:
>
>> The PIIX fix alone just fixes the syptom, not the underlying problem. The underlying problem is that the BAR isn't deactivated, and with the PIIX patch it will stay at address zero rather than in the USB function address range.
>
>Did you mean to say USB will not work now? It actually does with just 230dfd9257 backported to v4.2+.

What I'm saying is that the PIIX-IDE BAR is still a zombie, possibly threatening other innocent devices instead of USB.

Best regards,
Bernhard

>
>Either way, I will test and send the additional change for pci_xen_ide_unplug in the next days.
>
>
>Olaf