[SeaBIOS] [PATCH] seabios: call pci_init_device on resume

[Kevin O'Connor]

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 */

-Kevin