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

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

[Amit Shah]

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

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/acpi/ich9.c         | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/acpi/ich9.h |  4 +++
 2 files changed, 100 insertions(+)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index ea991a3..98828f5 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -269,10 +269,94 @@ static void ich9_pm_set_memory_hotplug_support(Object *obj, bool value,
     s->pm.acpi_memory_hotplug.is_enabled = value;
 }
 
+static void ich9_pm_get_disable_s3(Object *obj, Visitor *v,
+                                   void *opaque, const char *name,
+                                   Error **errp)
+{
+    ICH9LPCPMRegs *pm = opaque;
+    uint8_t value = pm->disable_s3;
+
+    visit_type_uint8(v, &value, name, errp);
+}
+
+static void ich9_pm_set_disable_s3(Object *obj, Visitor *v,
+                                   void *opaque, const char *name,
+                                   Error **errp)
+{
+    ICH9LPCPMRegs *pm = opaque;
+    Error *local_err = NULL;
+    uint8_t value;
+
+    visit_type_uint8(v, &value, name, &local_err);
+    if (local_err) {
+        goto out;
+    }
+    pm->disable_s3 = value;
+out:
+    error_propagate(errp, local_err);
+}
+
+static void ich9_pm_get_disable_s4(Object *obj, Visitor *v,
+                                   void *opaque, const char *name,
+                                   Error **errp)
+{
+    ICH9LPCPMRegs *pm = opaque;
+    uint8_t value = pm->disable_s4;
+
+    visit_type_uint8(v, &value, name, errp);
+}
+
+static void ich9_pm_set_disable_s4(Object *obj, Visitor *v,
+                                   void *opaque, const char *name,
+                                   Error **errp)
+{
+    ICH9LPCPMRegs *pm = opaque;
+    Error *local_err = NULL;
+    uint8_t value;
+
+    visit_type_uint8(v, &value, name, &local_err);
+    if (local_err) {
+        goto out;
+    }
+    pm->disable_s4 = value;
+out:
+    error_propagate(errp, local_err);
+}
+
+static void ich9_pm_get_s4_val(Object *obj, Visitor *v,
+                               void *opaque, const char *name,
+                               Error **errp)
+{
+    ICH9LPCPMRegs *pm = opaque;
+    uint8_t value = pm->s4_val;
+
+    visit_type_uint8(v, &value, name, errp);
+}
+
+static void ich9_pm_set_s4_val(Object *obj, Visitor *v,
+                               void *opaque, const char *name,
+                               Error **errp)
+{
+    ICH9LPCPMRegs *pm = opaque;
+    Error *local_err = NULL;
+    uint8_t value;
+
+    visit_type_uint8(v, &value, name, &local_err);
+    if (local_err) {
+        goto out;
+    }
+    pm->s4_val = value;
+out:
+    error_propagate(errp, local_err);
+}
+
 void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
 {
     static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;
     pm->acpi_memory_hotplug.is_enabled = true;
+    pm->disable_s3 = 0;
+    pm->disable_s4 = 0;
+    pm->s4_val = 2;
 
     object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
                                    &pm->pm_io_base, errp);
@@ -285,6 +369,18 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
                              ich9_pm_get_memory_hotplug_support,
                              ich9_pm_set_memory_hotplug_support,
                              NULL);
+    object_property_add(obj, ACPI_PM_PROP_S3_DISABLED, "uint8",
+                        ich9_pm_get_disable_s3,
+                        ich9_pm_set_disable_s3,
+                        NULL, pm, NULL);
+    object_property_add(obj, ACPI_PM_PROP_S4_DISABLED, "uint8",
+                        ich9_pm_get_disable_s4,
+                        ich9_pm_set_disable_s4,
+                        NULL, pm, NULL);
+    object_property_add(obj, ACPI_PM_PROP_S4_VAL, "uint8",
+                        ich9_pm_get_s4_val,
+                        ich9_pm_set_s4_val,
+                        NULL, pm, NULL);
 }
 
 void ich9_pm_device_plug_cb(ICH9LPCPMRegs *pm, DeviceState *dev, Error **errp)
diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
index fe975e6..12d7a7a 100644
--- a/include/hw/acpi/ich9.h
+++ b/include/hw/acpi/ich9.h
@@ -49,6 +49,10 @@ typedef struct ICH9LPCPMRegs {
     AcpiCpuHotplug gpe_cpu;
 
     MemHotplugState acpi_memory_hotplug;
+
+    uint8_t disable_s3;
+    uint8_t disable_s4;
+    uint8_t s4_val;
 } ICH9LPCPMRegs;
 
 void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
-- 
2.1.0

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

[Marcel Apfelbaum]

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
Maybe we can add them to Q35 Machines instead?
- machine q35,disable_s3=1,disable_s4=1

We have built-in support now.

Thanks,
Marcel

>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>   hw/acpi/ich9.c         | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   include/hw/acpi/ich9.h |  4 +++
>   2 files changed, 100 insertions(+)
>
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index ea991a3..98828f5 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -269,10 +269,94 @@ static void ich9_pm_set_memory_hotplug_support(Object *obj, bool value,
>       s->pm.acpi_memory_hotplug.is_enabled = value;
>   }
>
> +static void ich9_pm_get_disable_s3(Object *obj, Visitor *v,
> +                                   void *opaque, const char *name,
> +                                   Error **errp)
> +{
> +    ICH9LPCPMRegs *pm = opaque;
> +    uint8_t value = pm->disable_s3;
> +
> +    visit_type_uint8(v, &value, name, errp);
> +}
> +
> +static void ich9_pm_set_disable_s3(Object *obj, Visitor *v,
> +                                   void *opaque, const char *name,
> +                                   Error **errp)
> +{
> +    ICH9LPCPMRegs *pm = opaque;
> +    Error *local_err = NULL;
> +    uint8_t value;
> +
> +    visit_type_uint8(v, &value, name, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +    pm->disable_s3 = value;
> +out:
> +    error_propagate(errp, local_err);
> +}
> +
> +static void ich9_pm_get_disable_s4(Object *obj, Visitor *v,
> +                                   void *opaque, const char *name,
> +                                   Error **errp)
> +{
> +    ICH9LPCPMRegs *pm = opaque;
> +    uint8_t value = pm->disable_s4;
> +
> +    visit_type_uint8(v, &value, name, errp);
> +}
> +
> +static void ich9_pm_set_disable_s4(Object *obj, Visitor *v,
> +                                   void *opaque, const char *name,
> +                                   Error **errp)
> +{
> +    ICH9LPCPMRegs *pm = opaque;
> +    Error *local_err = NULL;
> +    uint8_t value;
> +
> +    visit_type_uint8(v, &value, name, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +    pm->disable_s4 = value;
> +out:
> +    error_propagate(errp, local_err);
> +}
> +
> +static void ich9_pm_get_s4_val(Object *obj, Visitor *v,
> +                               void *opaque, const char *name,
> +                               Error **errp)
> +{
> +    ICH9LPCPMRegs *pm = opaque;
> +    uint8_t value = pm->s4_val;
> +
> +    visit_type_uint8(v, &value, name, errp);
> +}
> +
> +static void ich9_pm_set_s4_val(Object *obj, Visitor *v,
> +                               void *opaque, const char *name,
> +                               Error **errp)
> +{
> +    ICH9LPCPMRegs *pm = opaque;
> +    Error *local_err = NULL;
> +    uint8_t value;
> +
> +    visit_type_uint8(v, &value, name, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +    pm->s4_val = value;
> +out:
> +    error_propagate(errp, local_err);
> +}
> +
>   void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
>   {
>       static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;
>       pm->acpi_memory_hotplug.is_enabled = true;
> +    pm->disable_s3 = 0;
> +    pm->disable_s4 = 0;
> +    pm->s4_val = 2;
>
>       object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
>                                      &pm->pm_io_base, errp);
> @@ -285,6 +369,18 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
>                                ich9_pm_get_memory_hotplug_support,
>                                ich9_pm_set_memory_hotplug_support,
>                                NULL);
> +    object_property_add(obj, ACPI_PM_PROP_S3_DISABLED, "uint8",
> +                        ich9_pm_get_disable_s3,
> +                        ich9_pm_set_disable_s3,
> +                        NULL, pm, NULL);
> +    object_property_add(obj, ACPI_PM_PROP_S4_DISABLED, "uint8",
> +                        ich9_pm_get_disable_s4,
> +                        ich9_pm_set_disable_s4,
> +                        NULL, pm, NULL);
> +    object_property_add(obj, ACPI_PM_PROP_S4_VAL, "uint8",
> +                        ich9_pm_get_s4_val,
> +                        ich9_pm_set_s4_val,
> +                        NULL, pm, NULL);
>   }
>
>   void ich9_pm_device_plug_cb(ICH9LPCPMRegs *pm, DeviceState *dev, Error **errp)
> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> index fe975e6..12d7a7a 100644
> --- a/include/hw/acpi/ich9.h
> +++ b/include/hw/acpi/ich9.h
> @@ -49,6 +49,10 @@ typedef struct ICH9LPCPMRegs {
>       AcpiCpuHotplug gpe_cpu;
>
>       MemHotplugState acpi_memory_hotplug;
> +
> +    uint8_t disable_s3;
> +    uint8_t disable_s4;
> +    uint8_t s4_val;
>   } ICH9LPCPMRegs;
>
>   void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
>

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

[Amit Shah]

On (Wed) 17 Dec 2014 [15:08:36], 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
> Maybe we can add them to Q35 Machines instead?
> - machine q35,disable_s3=1,disable_s4=1

The PM properties are chipset properties, aren't they?  To make the
change you requested, q35 will have to query the chipsets it supports,
and set the properties for the chipset in use - not sure how that
happens.

I modeled this based on how i440fx works, so there's also the case for
keeping things similar to an existing implementation...

		Amit

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

[Paolo Bonzini]

On 05/01/2015 13:23, Amit Shah wrote:
> I modeled this based on how i440fx works, so there's also the case for
> keeping things similar to an existing implementation...

Yes, I agree.

Paolo

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

[Marcel Apfelbaum]

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

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.
Thanks,
Marcel

>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>   hw/acpi/ich9.c         | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   include/hw/acpi/ich9.h |  4 +++
>   2 files changed, 100 insertions(+)
>
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index ea991a3..98828f5 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -269,10 +269,94 @@ static void ich9_pm_set_memory_hotplug_support(Object *obj, bool value,
>       s->pm.acpi_memory_hotplug.is_enabled = value;
>   }
>
> +static void ich9_pm_get_disable_s3(Object *obj, Visitor *v,
> +                                   void *opaque, const char *name,
> +                                   Error **errp)
> +{
> +    ICH9LPCPMRegs *pm = opaque;
> +    uint8_t value = pm->disable_s3;
> +
> +    visit_type_uint8(v, &value, name, errp);
> +}
> +
> +static void ich9_pm_set_disable_s3(Object *obj, Visitor *v,
> +                                   void *opaque, const char *name,
> +                                   Error **errp)
> +{
> +    ICH9LPCPMRegs *pm = opaque;
> +    Error *local_err = NULL;
> +    uint8_t value;
> +
> +    visit_type_uint8(v, &value, name, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +    pm->disable_s3 = value;
> +out:
> +    error_propagate(errp, local_err);
> +}
> +
> +static void ich9_pm_get_disable_s4(Object *obj, Visitor *v,
> +                                   void *opaque, const char *name,
> +                                   Error **errp)
> +{
> +    ICH9LPCPMRegs *pm = opaque;
> +    uint8_t value = pm->disable_s4;
> +
> +    visit_type_uint8(v, &value, name, errp);
> +}
> +
> +static void ich9_pm_set_disable_s4(Object *obj, Visitor *v,
> +                                   void *opaque, const char *name,
> +                                   Error **errp)
> +{
> +    ICH9LPCPMRegs *pm = opaque;
> +    Error *local_err = NULL;
> +    uint8_t value;
> +
> +    visit_type_uint8(v, &value, name, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +    pm->disable_s4 = value;
> +out:
> +    error_propagate(errp, local_err);
> +}
> +
> +static void ich9_pm_get_s4_val(Object *obj, Visitor *v,
> +                               void *opaque, const char *name,
> +                               Error **errp)
> +{
> +    ICH9LPCPMRegs *pm = opaque;
> +    uint8_t value = pm->s4_val;
> +
> +    visit_type_uint8(v, &value, name, errp);
> +}
> +
> +static void ich9_pm_set_s4_val(Object *obj, Visitor *v,
> +                               void *opaque, const char *name,
> +                               Error **errp)
> +{
> +    ICH9LPCPMRegs *pm = opaque;
> +    Error *local_err = NULL;
> +    uint8_t value;
> +
> +    visit_type_uint8(v, &value, name, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +    pm->s4_val = value;
> +out:
> +    error_propagate(errp, local_err);
> +}
> +
>   void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
>   {
>       static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;
>       pm->acpi_memory_hotplug.is_enabled = true;
> +    pm->disable_s3 = 0;
> +    pm->disable_s4 = 0;
> +    pm->s4_val = 2;
>
>       object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
>                                      &pm->pm_io_base, errp);
> @@ -285,6 +369,18 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
>                                ich9_pm_get_memory_hotplug_support,
>                                ich9_pm_set_memory_hotplug_support,
>                                NULL);
> +    object_property_add(obj, ACPI_PM_PROP_S3_DISABLED, "uint8",
> +                        ich9_pm_get_disable_s3,
> +                        ich9_pm_set_disable_s3,
> +                        NULL, pm, NULL);
> +    object_property_add(obj, ACPI_PM_PROP_S4_DISABLED, "uint8",
> +                        ich9_pm_get_disable_s4,
> +                        ich9_pm_set_disable_s4,
> +                        NULL, pm, NULL);
> +    object_property_add(obj, ACPI_PM_PROP_S4_VAL, "uint8",
> +                        ich9_pm_get_s4_val,
> +                        ich9_pm_set_s4_val,
> +                        NULL, pm, NULL);
>   }
>
>   void ich9_pm_device_plug_cb(ICH9LPCPMRegs *pm, DeviceState *dev, Error **errp)
> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> index fe975e6..12d7a7a 100644
> --- a/include/hw/acpi/ich9.h
> +++ b/include/hw/acpi/ich9.h
> @@ -49,6 +49,10 @@ typedef struct ICH9LPCPMRegs {
>       AcpiCpuHotplug gpe_cpu;
>
>       MemHotplugState acpi_memory_hotplug;
> +
> +    uint8_t disable_s3;
> +    uint8_t disable_s4;
> +    uint8_t s4_val;
>   } ICH9LPCPMRegs;
>
>   void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
>

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

[Amit Shah]

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

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

[Michael S. Tsirkin]

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.

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

[Amit Shah]

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.

(I think Windows since some version too does this, but don't remember
the details..)

		Amit

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

[Michael S. Tsirkin]

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".

> (I think Windows since some version too does this, but don't remember
> the details..)
> 
> 		Amit

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?

-- 
MST

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

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

[Marcel Apfelbaum]

On 01/12/2015 12:55 PM, 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 finally found it, acpi_get_pm_info in hw/i386/acpi-build.c
access the object's properties for both pc/q35.

[discussed off-list]
Last thing that is missing is:
   - in ich9_pm_init we have acpi_pm1_cnt_init(&pm->acpi_regs, &pm->io, 2);
   - while in piix4_pm_initfn we have acpi_pm1_cnt_init(&s->ar, &s->io, s->s4_val);

So ich9_pm_init can override the actual object property value, better if we update it
accordingly.

Thanks,
Marcel

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

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

[Amit Shah]

On (Mon) 12 Jan 2015 [13:51:00], Marcel Apfelbaum wrote:
> On 01/12/2015 12:55 PM, 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 finally found it, acpi_get_pm_info in hw/i386/acpi-build.c
> access the object's properties for both pc/q35.
> 
> [discussed off-list]
> Last thing that is missing is:
>   - in ich9_pm_init we have acpi_pm1_cnt_init(&pm->acpi_regs, &pm->io, 2);
>   - while in piix4_pm_initfn we have acpi_pm1_cnt_init(&s->ar, &s->io, s->s4_val);
> 
> So ich9_pm_init can override the actual object property value, better if we update it
> accordingly.

Thanks, v2 sent.

		Amit