On Thu, Dec 19, 2013 at 02:39:45PM -0500, Kevin O'Connor wrote:
On Thu, Dec 19, 2013 at 08:40:23PM +0200, Michael S. Tsirkin wrote:
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.
If I recall correctly, the firmware is not required to restore device state. But, agreed, if the firmware restored some devices from the OS state and initialized other devices and that caused a conflict, then that would not be good.
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?
On CSM and coreboot, it's the responsibility of UEFI/coreboot to do pci init. Performing the qemu specific firmware init on real hardware would likely cause problems. The code is specific to qemu, so it should be wrapped in a "if (CONFIG_QEMU) ...".
I am not sure it's qemu specific. The hardware spec states explicitly that all PIIX registers are reset on suspend, and OS does not seem to restore them, so it seems that we have to. At this point it looks like the fact things worked without this is due to qemu specific behaviour of not reseting some registers on suspend.
That means either delaying the release, or implementing this change as part of a future stable release.
You decide :)
If ultimately we need a significant change in behavior, then I'd lean torwards going forward with the current release and then make a follow up stable release for this change once it is ready.
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.
But, could the resume code be targetted to just PCI_DEVICE_ID_INTEL_82371AB_3, or do you think an init needs to be done to all/many PCI devices?
I suspect we need to init more devices, but I need to look some more into this. Basically whatever OS does not restore, we have to. QEMU currently retains a lot of state across resets so we get away with not restoring everything neither from bios nor from OS, but I think it's a bug.
pci_config_writel(bdf, 0x40, PORT_ACPI_PM_BASE | 1); pci_config_writeb(bdf, 0x80, 0x01); /* enable PM io space */ pci_config_writel(bdf, 0x90, PORT_SMB_BASE | 1); pci_config_writeb(bdf, 0xd2, 0x09); /* enable SMBus io space */-Kevin