Message ID | 20230701111341.25500-1-olaf@aepfle.de |
---|---|
State | New |
Headers | show |
Series | [v1] hw/ide/piix: properly initialize the BMIBA register | expand |
On Sat, 1 Jul 2023, Olaf Hering wrote: > 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 #1 "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(-) > > diff --git a/hw/ide/piix.c b/hw/ide/piix.c > index 41d60921e3..6449ba8b6b 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_word(pci_conf + 0x20, 0x01); /* BMIBA: 20-23h */ Commit message is way longer than the patch itself and very detailed but I may have lost in the details. If all 32 bits should be writtern does this need pci_set_long instead of pci_set_word? Regards, BALATON Zoltan > } > > static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp) > > base-commit: d145c0da22cde391d8c6672d33146ce306e8bf75 > >
Sat, 1 Jul 2023 15:34:40 +0200 (CEST) BALATON Zoltan <balaton@eik.bme.hu>:
> If all 32 bits should be writtern does this need pci_set_long instead of pci_set_word?
Thanks for spotting. After a number of experiments I used the wrong variant.
Olaf
diff --git a/hw/ide/piix.c b/hw/ide/piix.c index 41d60921e3..6449ba8b6b 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_word(pci_conf + 0x20, 0x01); /* BMIBA: 20-23h */ } static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
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 #1 "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