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

[Bernhard Beschow]

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
>