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

[Marcel Apfelbaum <marcel.apfelbaum@gmail.com>]

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