Re: [Qemu-devel] [PATCH] piix: do not reset APIC base address (0x80) on piix4_reset.

[Laszlo Ersek]

On 12/18/13 17:34, Paolo Bonzini wrote:
> 
> 
> ----- Messaggio originale -----
>> Da: "Michael S. Tsirkin" <address@hidden>
>> A: "marcel a" <address@hidden>
>> Cc: "Paolo Bonzini" <address@hidden>, "Gal Hammer" <address@hidden>, 
>> address@hidden,
>> address@hidden
>> Inviato: Mercoledì, 18 dicembre 2013 17:33:06
>> Oggetto: Re: [Qemu-devel] [PATCH] piix: do not reset APIC base address 
>> (0x80) on piix4_reset.
>>
>> On Wed, Dec 18, 2013 at 06:27:12PM +0200, Marcel Apfelbaum wrote:
>>> On Wed, 2013-12-18 at 17:22 +0200, Michael S. Tsirkin wrote:
>>>> On Wed, Dec 18, 2013 at 03:22:59PM +0100, Paolo Bonzini wrote:
>>>>> Il 11/12/2013 10:21, Gal Hammer ha scritto:
>>>>>> Fix a bug that was introduced in commit c046e8c4. QEMU fails to
>>>>>> resume from suspend mode (S3).
>>>>>>
>>>>>> Signed-off-by: Gal Hammer <address@hidden>
>>>>>> ---
>>>>>>  hw/acpi/piix4.c | 1 -
>>>>>>  1 file changed, 1 deletion(-)
>>>>>>
>>>>>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>>>>>> index 93849c8..5c736a4 100644
>>>>>> --- a/hw/acpi/piix4.c
>>>>>> +++ b/hw/acpi/piix4.c
>>>>>> @@ -376,7 +376,6 @@ static void piix4_reset(void *opaque)
>>>>>>      pci_conf[0x5b] = 0;
>>>>>>  
>>>>>>      pci_conf[0x40] = 0x01; /* PM io base read only bit */
>>>>>> -    pci_conf[0x80] = 0;
>>>>>>  
>>>>>>      if (s->kvm_enabled) {
>>>>>>          /* Mark SMM as already inited (until KVM supports SMM). */
>>>>>
>>>>> Note this is not the APIC base address, that one is 80h on the ISA
>>>>> bridge (function 0).  You're changing the behavior for 80h on the power
>>>>> management function, which is function 3.  The register is "PMBA—POWER
>>>>> MANAGEMENT BASE ADDRESS" and it is indeed initialized by SeaBIOS in
>>>>> piix4_pm_setup (src/fw/pciinit.c).
>>>>>
>>>>> Michael, perhaps a part of pci_setup (same file) should run on S3
>>>>> resume?
>>>>>
>>>>> Paolo
>>>>
>>>> Seems reasonable: either seabios or guest OS must do it, and
>>>> guest does not seem to.
>>> I was looking into this today, but it seems that we have a problem.
>>> We cannot run pci_setup() in init section:
>>> .data.varinit.seabios/src/hw/pci.h.66 is VARVERIFY32INIT but used from
>>> ['.text.runtime.seabios/src/resume.c.150', '.text.pci_setup']
>>>
>>> Any thoughts how to get around this?
>>> Thanks,
>>> Marcel
>>
>> We defintely don't want to do full pci enumeration.
>> Just pci_bios_init_platform or even less.
> 
> Or put an array of (bdf, offset, size, value) tuples somewhere in low memory,
> fill it at startup, and reproduce it blindly at S3 resume time.  This is 
> similar
> to what UEFI does.

What UEFI does is kind of a mess :)

PEI runs both during cold boot and S3 resume.

DXE runs only after cold boot, it is not reached during S3 resume. (The
DXE initial program loader is the last PEI module, and dependent on the
boot mode, it either loads the DXE core, or invokes the S3 resume vector.)

So, PEI itself can reinitialize whatever it wants. The S3 boot script
that you refer to above is a way for DXE drivers to stash a bunch of IO
/ PCI etc writes for the *next* PEI phase, ie. the one that runs when
resuming from S3. QemuVideoDxe (the driver in OVMF that configures
produces the GOP on top of Cirrus) is such a DXE driver.

In a nutshell,

1. SEC after cold boot
2. PEI after cold boot
2.5 DXE IPL PEIM loads DXE core
3. DXE after cold boot
4. BDS after cold boot
5. runtime (OSPM), normal entry
6. PEI after S3 resume
6.5 DXE IPL PEIM branches to S3 resume PEIM
7. runtime (OSPM), entry via S3 resume vector

Steps 2 and 6 are implemented by the same PEI code (it can branch
internally based on boot mode of course), and this PEI code contains the
excerpt that I posted a minute ago.

The S3 boot script is prepared during step 3 by various DXE drivers, and
it is saved into reserved / AcpiNVS RAM no later than entering 5. This
script is then replayed / interpreted by step 6.

So, if PEI must do something after S3 resume that is independent of any
DXE drivers, it can simply do it. The boot script is only necessary when
the S3 resume PEI actions (in step 6) need to depend on earlier actions
during DXE (step 3).

Thanks,
Laszlo