On Mon, 2014-01-13 at 16:39 -0500, Kevin O'Connor wrote:
On Mon, Jan 13, 2014 at 07:46:33PM +0200, Marcel Apfelbaum wrote:
On Mon, 2014-01-13 at 11:31 -0500, Kevin O'Connor wrote:
Thanks. SeaBIOS isn't responsible for PCI setup on CSM/coreboot, so the patch must check for CONFIG_QEMU.
Sure thing, Sorry I missed that, I'll add it to V2.
Also, I think we can simplify this a bit - how about the patch below (untested)?
Hi Kevin, I followed the patch and indeed it is smaller and it does the job, but
- We also have a q35 chipset and we'll need a global variable also for it.
- If we will have other devices that need special attention on resume we will be ready for them (low chance, but you never know).
- Finally, the pci_resume will look a little strange and unclear with the new "if" statements (that means, this is the board and we saved the value)
Is this needed for q35? In general, the firmware is not responsible for restoring hardware state, so I don't think resume fixups will be common. The storing of PCI BDFs for use in resume is already done elsewhere in the code (see shadow.c and smm.c).
I checked and Q35 does not need this kind of hack, but it has other problems :( (Can't suspend/resume 2 times). I am going to find some time to look into it.
What do you think?
I'm open to alternatives. However, the code needs to be run only when CONFIG_QEMU is set, and I would ask that resume.c not be made more complicated - so lets have it just call a function (eg, pci_resume()) and put the rest of the logic in the fw/ directory.
Thank you for the review! I sent a V2 following your review.