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

[Laszlo Ersek]

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