On 12/19/13 20:39, 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) ...". > >>> 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? > > 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 */
(Sorry for butting in.) Currently OVMF cares about the former only, and it seems to suffice. Thanks, Laszlo _______________________________________________ SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios