Patchwork [4/3] wakeup: only reset the CPU

login
register
mail settings
Submitter Paolo Bonzini
Date March 5, 2013, 4:03 p.m.
Message ID <1362499403-16514-1-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/225069/
State New
Headers show

Comments

Paolo Bonzini - March 5, 2013, 4:03 p.m.
Resuming from suspend-to-RAM should not reset all devices.  Only the CPU
should get a reset signal.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 vl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
David Woodhouse - March 5, 2013, 4:59 p.m.
On Tue, 2013-03-05 at 17:03 +0100, Paolo Bonzini wrote:
> Resuming from suspend-to-RAM should not reset all devices.  Only the
> CPU should get a reset signal.

Hm... on reflection, I don't actually know if this is true.

Perhaps we *should* reset all devices. After all, in a real machine
they'll all have been turned off and the RAM will have been in
self-refresh. Surely they have to be reset?

So maybe we should *let* the i440FX PAM registers get reset to point to
ROM. And fix the firmware to *cope* with that, check to see if the
shadow RAM already holds an image of a started-up firmware with the
correct checksum, and jump back to it.

That is: perhaps it's a *SeaBIOS* bug that suspend/resume doesn't work
if the PAM configuration is reset?
Paolo Bonzini - March 5, 2013, 5:10 p.m.
Il 05/03/2013 17:59, David Woodhouse ha scritto:
> On Tue, 2013-03-05 at 17:03 +0100, Paolo Bonzini wrote:
>> > Resuming from suspend-to-RAM should not reset all devices.  Only the
>> > CPU should get a reset signal.
> Hm... on reflection, I don't actually know if this is true.
> 
> Perhaps we *should* reset all devices. After all, in a real machine
> they'll all have been turned off and the RAM will have been in
> self-refresh. Surely they have to be reset?
> 
> So maybe we should *let* the i440FX PAM registers get reset to point to
> ROM. And fix the firmware to *cope* with that, check to see if the
> shadow RAM already holds an image of a started-up firmware with the
> correct checksum, and jump back to it.
> 
> That is: perhaps it's a *SeaBIOS* bug that suspend/resume doesn't work
> if the PAM configuration is reset?

Yeah, it sounded a bit weird when I wrote that commit message.  This
could be the case.  How does it work on Coreboot?

Paolo
Peter Stuge - March 5, 2013, 5:26 p.m.
Paolo Bonzini wrote:
> > That is: perhaps it's a *SeaBIOS* bug that suspend/resume doesn't
> > work if the PAM configuration is reset?
> 
> Yeah, it sounded a bit weird when I wrote that commit message. 
> This could be the case.  How does it work on Coreboot?

Yes all hardware except RAM in self-refresh and registers explicitly
hooked to battery sleep power rail has reset state on resume from S3.
Those registers seem to grow over time, the i440 probably doesn't
have very many. The memory controller configuration would be stored
there.

Power on and resume are identical except for memory controller init
and what should happen once init is finished.

On power on jump to payload.

On resume jump to S3 resume vector set by OSPM.

This is another case where coreboot has infrastructure since a long
time, which can be used by hypervisors. Please don't replicate that
into SeaBIOS. In the end you will have what is essentially a fork of
coreboot, which seems like a tremendous waste of effort.

See if there is a 440 board in coreboot where suspend works. If yes,
then making that work for the QEMU board should take only moderate
effort.


//Peter
Paolo Bonzini - March 5, 2013, 5:42 p.m.
Il 05/03/2013 18:26, Peter Stuge ha scritto:
>>> > > That is: perhaps it's a *SeaBIOS* bug that suspend/resume doesn't
>>> > > work if the PAM configuration is reset?
>> > 
>> > Yeah, it sounded a bit weird when I wrote that commit message. 
>> > This could be the case.  How does it work on Coreboot?
> Yes all hardware except RAM in self-refresh and registers explicitly
> hooked to battery sleep power rail has reset state on resume from S3.
> Those registers seem to grow over time, the i440 probably doesn't
> have very many. The memory controller configuration would be stored
> there.

Ok, so it's not correct either with this patch or without. :)

But without this patch it is more correct; things such as USB queue
heads must be reset, or you'll corrupt random memory when the VM is
restarted.

Paolo
Laszlo Ersek - March 5, 2013, 7:12 p.m.
On 03/05/13 17:59, David Woodhouse wrote:> On Tue, 2013-03-05 at 17:03 +0100, Paolo Bonzini wrote:
>> Resuming from suspend-to-RAM should not reset all devices.  Only the
>> CPU should get a reset signal.
>
> Hm... on reflection, I don't actually know if this is true.
>
> Perhaps we *should* reset all devices. After all, in a real machine
> they'll all have been turned off and the RAM will have been in
> self-refresh. Surely they have to be reset?
>
> So maybe we should *let* the i440FX PAM registers get reset to point to
> ROM. And fix the firmware to *cope* with that, check to see if the
> shadow RAM already holds an image of a started-up firmware with the
> correct checksum, and jump back to it.
>
> That is: perhaps it's a *SeaBIOS* bug that suspend/resume doesn't work
> if the PAM configuration is reset?

I think it is indeed a problem with SeaBIOS. (git.seabios.org
(80.81.252.135) seems to be down ATM, so I can only check at f465e1ec.)
Open romlayout.S:

   622  reset_vector:
   623          ljmpw $SEG_BIOS, $entry_post

   537  entry_post:
   538          cmpl $0, %cs:HaveRunPost                // Check for resume/reboot
   539          jnz entry_resume
   540          ENTRY_INTO32 _cfunc32flat_handle_post   // Normal entry point

   239  entry_resume:
...
   248          // Call handler.
   249          jmp handle_resume

handle_resume() [src/resume.c]
  inb_cmos(CMOS_RESET_CODE)

It checks the CMOS only after looking at HaveRunPost. The value of
HaveRunPost depends on the PAM settings. It's always 0 in ROM, in which
case we continue at handle_post() [src/post.c].

Laszlo
Paolo Bonzini - March 5, 2013, 7:25 p.m.
Il 05/03/2013 20:12, Laszlo Ersek ha scritto:
> On 03/05/13 17:59, David Woodhouse wrote:> On Tue, 2013-03-05 at 17:03 +0100, Paolo Bonzini wrote:
>>> Resuming from suspend-to-RAM should not reset all devices.  Only the
>>> CPU should get a reset signal.
>>
>> Hm... on reflection, I don't actually know if this is true.
>>
>> Perhaps we *should* reset all devices. After all, in a real machine
>> they'll all have been turned off and the RAM will have been in
>> self-refresh. Surely they have to be reset?
>>
>> So maybe we should *let* the i440FX PAM registers get reset to point to
>> ROM. And fix the firmware to *cope* with that, check to see if the
>> shadow RAM already holds an image of a started-up firmware with the
>> correct checksum, and jump back to it.
>>
>> That is: perhaps it's a *SeaBIOS* bug that suspend/resume doesn't work
>> if the PAM configuration is reset?
> 
> I think it is indeed a problem with SeaBIOS. Open romlayout.S: [...]
>
> It checks the CMOS only after looking at HaveRunPost. The value of
> HaveRunPost depends on the PAM settings. It's always 0 in ROM, in which
> case we continue at handle_post() [src/post.c].

Actually, Peter explained that it is okay.  S3 doesn't clear the PAM
configuration, but S4 does.  The PAM registers are attached to the same
power line as the RAM.

Paolo
Laszlo Ersek - March 5, 2013, 7:51 p.m.
On 03/05/13 20:25, Paolo Bonzini wrote:
> Il 05/03/2013 20:12, Laszlo Ersek ha scritto:
>> On 03/05/13 17:59, David Woodhouse wrote:> On Tue, 2013-03-05 at 17:03 +0100, Paolo Bonzini wrote:
>>>> Resuming from suspend-to-RAM should not reset all devices.  Only the
>>>> CPU should get a reset signal.
>>>
>>> Hm... on reflection, I don't actually know if this is true.
>>>
>>> Perhaps we *should* reset all devices. After all, in a real machine
>>> they'll all have been turned off and the RAM will have been in
>>> self-refresh. Surely they have to be reset?
>>>
>>> So maybe we should *let* the i440FX PAM registers get reset to point to
>>> ROM. And fix the firmware to *cope* with that, check to see if the
>>> shadow RAM already holds an image of a started-up firmware with the
>>> correct checksum, and jump back to it.
>>>
>>> That is: perhaps it's a *SeaBIOS* bug that suspend/resume doesn't work
>>> if the PAM configuration is reset?
>>
>> I think it is indeed a problem with SeaBIOS. Open romlayout.S: [...]
>>
>> It checks the CMOS only after looking at HaveRunPost. The value of
>> HaveRunPost depends on the PAM settings. It's always 0 in ROM, in which
>> case we continue at handle_post() [src/post.c].
> 
> Actually, Peter explained that it is okay.  S3 doesn't clear the PAM
> configuration, but S4 does.  The PAM registers are attached to the same
> power line as the RAM.

Ah, you expect me to understand that "memory controller configuration"
is a superset of the PAM registers :)

Laszlo
Kevin O'Connor - March 6, 2013, 1:45 a.m.
On Tue, Mar 05, 2013 at 04:59:51PM +0000, David Woodhouse wrote:
> On Tue, 2013-03-05 at 17:03 +0100, Paolo Bonzini wrote:
> > Resuming from suspend-to-RAM should not reset all devices.  Only the
> > CPU should get a reset signal.
> 
> Hm... on reflection, I don't actually know if this is true.
> 
> Perhaps we *should* reset all devices. After all, in a real machine
> they'll all have been turned off and the RAM will have been in
> self-refresh. Surely they have to be reset?
> 
> So maybe we should *let* the i440FX PAM registers get reset to point to
> ROM. And fix the firmware to *cope* with that, check to see if the
> shadow RAM already holds an image of a started-up firmware with the
> correct checksum, and jump back to it.
> 
> That is: perhaps it's a *SeaBIOS* bug that suspend/resume doesn't work
> if the PAM configuration is reset?

On a real machine the firmware would query the available memory and
its settings via the smbus and then program the memory controller.
Programming the memory controller without accessing any memory is a
non-trivial task.  QEMU doesn't emulate this low-level hardware - nor
does it even fully emulate the PAM registers.

So, I think the question isn't what does real-hardware do (there's no
gain to be had in emulating all of this).  Instead, I think the
question is - what makes the most sense.  Given that the objective is
to restore the memory image and reset everything else, I think it
would be best for QEMU's S3resume to completely restore the memory
image - including the PAM settings.

-Kevin
David Woodhouse - March 6, 2013, 8:32 a.m.
On Tue, 2013-03-05 at 20:45 -0500, Kevin O'Connor wrote:
> 
> So, I think the question isn't what does real-hardware do (there's no
> gain to be had in emulating all of this).  Instead, I think the
> question is - what makes the most sense.  

Well,... what we implement for the 'native SeaBIOS on qemu' case doesn't
need to work on real hardware, it's true.

But a bunch of other combinations *do* need to work on real hardware, so
it's worth bearing it in mind to a certain extent.

We wouldn't want Coreboot and OVMF to have to have workarounds for qemu
behaving differently, just because we've done something simpler for
SeaBIOS. But it looks like the PAM configuration is considered part of
the memory setup and isn't lost in S3, so for the case I was wondering
about it's fine. Qemu *is* behaving like real hardware, which *is* the
simple option that makes most sense.

Patch

diff --git a/vl.c b/vl.c
index c03edf1..2c93478 100644
--- a/vl.c
+++ b/vl.c
@@ -1973,8 +1973,8 @@  static bool main_loop_should_exit(void)
     }
     if (qemu_wakeup_requested()) {
         pause_all_vcpus();
+        cpu_soft_reset();
         cpu_synchronize_all_states();
-        qemu_system_reset(VMRESET_SILENT);
         resume_all_vcpus();
         monitor_protocol_event(QEVENT_WAKEUP, NULL);
     }