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

[Michael S. Tsirkin]

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.


> 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?

> That means either delaying the release, or
> implementing this change as part of a future stable release.

You decide :)

> 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.

>             ...
>             return;
>         }
>     }
> }
> 
> -Kevin