On Fri, 2013-12-20 at 00:18 +0200, Michael S. Tsirkin wrote:
On Thu, Dec 19, 2013 at 09:52:39PM +0100, Laszlo Ersek wrote:
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
One useful test to try is to dump full 256 byte device config before and after resume. It should match for all devices. I'll look into this next week if no one beats me to it.
I used a patch that modifies only the pm config registers, otherwise the os doesn't load and the pci configuration is not restored...
Before resume:
------------------------------------------------------ 0:0.0 0x00h: 0x12378086 0x00000103 0x06000002 0x00000000 [...] 0x20h: 0x00000000 0x00000000 0x00000000 0x11001af4 [...] 0x50h: 0x00000000 0xff000000 0x11111000 0x33111111 [...] 0x70h: 0x00020000 0x00000000 0x00000000 0x00000000 [...]
0:1.0 0x00h: 0x70008086 0x02000103 0x06010000 0x00800000 [...] 0x20h: 0x00000000 0x00000000 0x00000000 0x11001af4 [...] 0x40h: 0x00000000 0x00000000 0x00000000 0x0003004d [...] 0x60h: 0x0b0a0a8a 0x00000000 0x00000200 0x00000000 0x70h: 0x00000080 0x0c0c0000 0x00000002 0x00000000 0x80h: 0x00020000 0x00000000 0x00000000 0x00000000 [...] 0xa0h: 0x00000008 0x00000000 0x0000000f 0x00000000 [...]
0:1.1 0x00h: 0x70108086 0x02800107 0x01018000 0x00000000 [...] 0x20h: 0x0000c0a1 0x00000000 0x00000000 0x11001af4 [...] 0x40h: 0xe303c000 0x00000000 0x00000000 0x00000000 [...]
0:1.3 0x00h: 0x71138086 0x02800103 0x06800003 0x00000000 [...] 0x20h: 0x00000000 0x00000000 0x00000000 0x11001af4 0x30h: 0x00000000 0x00000000 0x00000000 0x00000109 0x40h: 0x0000b001 0x00000000 0x00000000 0x00000000 0x50h: 0x00000000 0x00000000 0x02000000 0x90000000 0x60h: 0x60000000 0x98000000 0x00000000 0x00000000 [...] 0x80h: 0x00000001 0x00000000 0x00000000 0x00000000 0x90h: 0x0000b101 0x00000000 0x00000000 0x00000000 [...] 0xd0h: 0x00090000 0x00000000 0x00000000 0x00000000 [...]
0:2.0 0x00h: 0x01001b36 0x00000103 0x03000004 0x00000000 0x10h: 0xf4000000 0xf8000000 0xfc070000 0x0000c081 0x20h: 0x00000000 0x00000000 0x00000000 0x11001af4 0x30h: 0xfc060000 0x00000000 0x00000000 0x0000010a [...]
0:3.0 0x00h: 0x100e8086 0x00000107 0x02000003 0x00000000 0x10h: 0xfc040000 0x0000c001 0x00000000 0x00000000 0x20h: 0x00000000 0x00000000 0x00000000 0x11001af4 0x30h: 0xfc000000 0x00000000 0x00000000 0x0000010b [...]
0:4.0 0x00h: 0x10011af4 0x00100507 0x01000000 0x00000000 0x10h: 0x0000c041 0xfc072000 0x00000000 0x00000000 0x20h: 0x00000000 0x00000000 0x00000000 0x00021af4 0x30h: 0x00000000 0x00000040 0x00000000 0x0000010b 0x40h: 0x80010011 0x00000001 0x00000801 0x00000000 [...]
------------------------------------------------------------
After resume: 0:0.0 0x00h: 0x12378086 0x00000103 0x06000002 0x00000000 [...] 0x20h: 0x00000000 0x00000000 0x00000000 0x11001af4 [...] 0x50h: 0x00000000 0xff000000 0x11111000 0x11111111 [...] 0x70h: 0x00020000 0x00000000 0x00000000 0x00000000 [...]
0:1.0 0x00h: 0x70008086 0x02000103 0x06010000 0x00800000 [...] 0x20h: 0x00000000 0x00000000 0x00000000 0x11001af4 [...] 0x40h: 0x00000000 0x00000000 0x00000000 0x0003004d [...] 0x60h: 0x0b0a0a80 0x00000000 0x00000200 0x00000000 0x70h: 0x00000080 0x0c0c0000 0x00000002 0x00000000 0x80h: 0x00020000 0x00000000 0x00000000 0x00000000 [...] 0xa0h: 0x00000008 0x00000000 0x0000000f 0x00000000 [...]
0:1.1 0x00h: 0x70108086 0x02800107 0x01018000 0x00000000 [...] 0x20h: 0x0000c0a1 0x00000000 0x00000000 0x11001af4 [...] 0x40h: 0xe303c000 0x00000000 0x00000000 0x00000000 [...]
0:1.3 0x00h: 0x71138086 0x02800103 0x06800003 0x00000000 [...] 0x20h: 0x00000000 0x00000000 0x00000000 0x11001af4 0x30h: 0x00000000 0x00000000 0x00000000 0x00000109 0x40h: 0x0000b001 0x00000000 0x00000000 0x00000000 0x50h: 0x00000000 0x00000000 0x02000000 0x90000000 0x60h: 0x60000000 0x98000000 0x00000000 0x00000000 [...] 0x80h: 0x00000001 0x00000000 0x00000000 0x00000000 0x90h: 0x0000b101 0x00000000 0x00000000 0x00000000 [...] 0xd0h: 0x00090000 0x00000000 0x00000000 0x00000000 [...]
0:2.0 0x00h: 0x01001b36 0x00000103 0x03000004 0x00000000 0x10h: 0xf4000000 0xf8000000 0xfc070000 0x0000c081 0x20h: 0x00000000 0x00000000 0x00000000 0x11001af4 0x30h: 0xfc060000 0x00000000 0x00000000 0x0000010a [...]
0:3.0 0x00h: 0x100e8086 0x00000107 0x02000003 0x00000000 0x10h: 0xfc040000 0x0000c001 0x00000000 0x00000000 0x20h: 0x00000000 0x00000000 0x00000000 0x11001af4 0x30h: 0xfc000000 0x00000000 0x00000000 0x0000010b [...]
0:4.0 0x00h: 0x10011af4 0x00100507 0x01000000 0x00000000 0x10h: 0x0000c041 0xfc072000 0x00000000 0x00000000 0x20h: 0x00000000 0x00000000 0x00000000 0x00021af4 0x30h: 0x00000000 0x00000040 0x00000000 0x0000010b 0x40h: 0x80010011 0x00000001 0x00000801 0x00000000 [...]
---------------------------------------------------------------
We have 2 differences (besides the obvious pm config register) 1. 0:0.0 0x50h: 0x00000000 0xff000000 0x11111000 0x33111111 => 0x50h: 0x00000000 0xff000000 0x11111000 0x11111111 Analysis: - From Intel 440FX spec: - PAM0 (59h) - PAM6 (5Fh): - Default value: 0x0 - PAM0[3:0] - reserved, PAM0[7:4] points to BIOS Area attribute bits - Before resume, BIOS Area had RW attribute - After resume, BIOS Area had RE attribute Outcome: I am not an expert on PAM, but it seems that the value after resume (RE) is ok for BIOS region, no idea why before it was RW. So everything OK here.
2. 0:1.0 0x60h: 0x0b0a0a8a 0x00000000 0x00000200 0x00000000 => 0x60h: 0x0b0a0a80 0x00000000 0x00000200 0x00000000 Analysis: - From PIIX3 spec: - 60h (PIRQRCA#)—63h (PIRQRCD#): PIRQx ROUTE CONTROL REGISTERS - Default value: 0x80 - Bit 7 Interrupt Routing Enable. 0=Enable; 1=Disable - The value is different, 8a -> 80, but bit 7 is set, so it remains disabled - After resume it receives the default value, but before this is dirty Outcome: Everything OK here
Thanks, Marcel