On Thu, Dec 19, 2013 at 12:04:25PM -0500, Kevin O'Connor wrote:
On Thu, Dec 19, 2013 at 01:49:02PM +0200, Michael S. Tsirkin wrote:
PIIX spec says that most its registers are reset on suspend. However, QEMU didn't reset most of them properly. Now that it started doing that, resume is broken. Fix it up by probing system devices on bus 0 and re-initializing them.
Thanks. This patch runs part of the "pciinit" code and not all of it. What's the reasoning behind which code to run and which code not to run? Re-running parts of pciinit on resume is a change in behaviour and I'm concerned about unintended consequences. Is this definitely what should be setup for PCI on resume - nothing more should be done and nothing less?
I'm not sure about nothing less. It is possible that there are some registers here that we don't need to touch.
Nothing more, I am pretty sure.
We must not touch anything that OS might have changed, or there will be a conflict. This is why we don't touch interrupt registers and BARs - OS can change these and it must put them back exactly the way they were, if we enable some BARs there will be a conflict.
Also, we were just about to release SeaBIOS v1.7.4. I don't think this behaviour change would be good to add so late in the release cycle. (As a side note, the patch would not be correct for coreboot and CSM users.)
Interesting. Why won't it?
That means either delaying the release, or implementing this change as part of a future stable release.
You decide :)
Finally, if the issue is just on the PIIX PM device, would a more targeted change be more appropriate? For example, something like:
void qemu_piix_resume_fixup(void) { if (!CONFIG_QEMU) return; int bdf; foreachbdf(bdf, 0) { u32 vendev = pci_config_readl(bdf, PCI_VENDOR_ID); u16 vendor = vendev & 0xffff, device = vendev >> 16; if (vendor == PCI_VENDOR_ID_INTEL && device == PCI_DEVICE_ID_INTEL_82371AB_3) { pci_config_writeb(bdf, 0x80, 0x01);
IMO this is definitely not enough. E.g. I'm pretty sure we should set the IO address before we enable IO.
... return; } }
}
-Kevin