Re: [Qemu-devel] [PATCH 1/1] ich9: add disable_s3, disable_s4, s4_val properties

[Amit Shah]

On (Mon) 12 Jan 2015 [13:16:46], Michael S. Tsirkin wrote:
> On Mon, Jan 12, 2015 at 04:41:03PM +0530, Amit Shah wrote:
> > On (Mon) 12 Jan 2015 [13:01:28], Michael S. Tsirkin wrote:
> > > On Mon, Jan 12, 2015 at 04:25:01PM +0530, Amit Shah wrote:
> > > > On (Mon) 12 Jan 2015 [12:26:08], Marcel Apfelbaum wrote:
> > > > > On 12/16/2014 01:23 PM, Amit Shah wrote:
> > > > > >PIIX4 has disable_s3 and disable_s4 properties to enable or disable 
> > > > > >PM
> > > > > >functions.  Add such properties to the ICH9 chipset as well for the 
> > > > > >Q35
> > > > > >machine type.
> > > > > >
> > > > > >S3 / S4 are not guaranteed to always work (needs work in the guest as
> > > > > >well as QEMU for things to work properly), and disabling advertising 
> > > > > >of
> > > > > >these features ensures guests don't go into zombie state if something
> > > > > >isn't working right.
> > > > > >
> > > > > >The defaults are kept the same as in PIIX4: both S3 and S4 are 
> > > > > >enabled
> > > > > >by default.
> > > > > >
> > > > > >These can be disabled via the cmdline:
> > > > > >
> > > > > >   ... -global ICH9-LPC,disable_s3=1 -global ICH9-LPC,disable_s4=1
> > > > >                         ^^^                           ^^^
> > > > > Should be -global ICH9-LPC.disable_s3=1 -global ICH9-LPC.disable_s4=1
> > > > 
> > > > Indeed, thanks.
> > > > 
> > > > > Hi Amit, thanks for answering my prev question.
> > > > > I have one more:)
> > > > > 
> > > > > I didn't see how the properties are connected to the ACPI mechanism.
> > > > > I tested it with your suggested command line and it didn't work from 
> > > > > some reason.
> > > > >    - I used ... -M Q35 -global ICH9-LPC.disable_s3=1 -global 
> > > > > ICH9-LPC.disable_s4=1
> > > > >    - On guest: pm-is-supported --hibernate && echo $? => 0 (enabled)
> > > > >    - Furthermore, pm-hibernate worked
> > > > > 
> > > > > Maybe I am missing something or maybe this is not in the scope of 
> > > > > this patch.
> > > > 
> > > > Hibernate is special for Linux guests.  If acpi-based hibernate isn't
> > > > available, Linux simulates it by writing a hibernate image and doing a
> > > > shutdown of the guest instead of entering the S4 state.
> > > >
> > > > To test, there are two ways: check if s3 works after passing this
> > > > parm, or check the acpi blobs inside the guest for the advertisement
> > > > of the params.
> > > > 
> > > >                 Amit
> > > 
> > > Interesting. So this isn't for the benefit of linux guests then?
> > > Which guests do actually benefit? It might be a good idea to
> > > put this info in the commit log.
> > 
> > No, this does disable the ACPI-based s4 advertisement, so it does
> > affect Linux too.
> > 
> > Linux, though, has a way of doing hibernate even when acpi-s4 isn't
> > available.  It's a convenience(?) feature offered by Linux, and isn't
> > related to anything else.  No need for mentioning it in the commit
> > message, and this behaviour is not dependent on anything that qemu can
> > or cannot do.
> 
> Yes but the implication is that your patch will not prevent Linux
> from "go into zombie state".

Nothing can - it's a guest thing; nothing the host can do about it.

> While I don't have issues with workarounds for guest bugs,
> the following text in the commit log:
> " ensures guests don't go into zombie state if something isn't working right"
> seems unnecessarily vague.
> 
> What does zombie state refer to, with which guests was this observed,
> and what was the root cause?

s3 and s4 are known to be unreliable even on physical systems.
The bugs are hardware-dependent, firmware dependent, and
OS-dependent.  In our case, bugs in qemu can cause badness, bugs in
guests can cause badness, bugs in seabios can cause badness, etc.
Unless some combination is tested thoroughly, one doesn't know what
can break.

There are several examples over the years detailing how guest s3/s4
keeps breaking.  E.g. virtio queue state between guest and host can go
out of sync, and the infamous 'virtio: guest moved head from X to Y'
messages.  kvmclock, if not re-synced after resume, can cause guest
hangs.  And so on.

If you have a better message for the commit log, pls go ahead and edit
it.


                Amit