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

Michael S. Tsirkin mst at redhat.com
Thu Dec 19 23:16:48 CET 2013


On Thu, Dec 19, 2013 at 02:39:45PM -0500, 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) ...".

I am not sure it's qemu specific. The hardware spec states
explicitly that all PIIX registers are reset on suspend, and
OS does not seem to restore them, so it seems that we have to.
At this point it looks like the fact things worked without this is due to
qemu specific behaviour of not reseting some registers on suspend.

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

I suspect we need to init more devices, but I need to look some
more into this. Basically whatever OS does not restore, we have to.
QEMU currently retains a lot of state across resets so we get away
with not restoring everything neither from bios nor from OS,
but I think it's a bug.

>     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



More information about the SeaBIOS mailing list