Re: [Qemu-devel] [PATCH 2/2] Add PIIX4 properties to control PM system states.

[Anthony Liguori <anthony@codemonkey.ws>]

On 05/20/2012 04:02 AM, Gleb Natapov wrote:
> This patch adds two things. First it allows QEMU to distinguish between
> regular powerdown and S4 powerdown. Later separate QMP notification will
> be added for S4 powerdown. Second it allows S3/S4 states to be disabled
> from QEMU command line. Some guests known to be broken with regards to
> power management, but allow to use it anyway. Using new properties
> management will be able to disable S3/S4 for such guests.
>
> Supported system state are passed to a firmware using new fw_cfg file.
> The file contains  6 byte array. Each byte represents one system
> state. If byte at offset X has its MSB set it means that system state
> X is supported and to enter it guest should use the value from lowest 3
> bits.
>
> Signed-off-by: Gleb Natapov<gleb@redhat.com>

I see nothing wrong in principle here except that you should use a PTR property 
to pass the fw_cfg object to the ACPI PM device.

Regards,

Anthony Liguori

> ---
>   hw/acpi.c       |    5 ++++-
>   hw/acpi.h       |    2 +-
>   hw/acpi_piix4.c |   16 +++++++++++++++-
>   hw/vt82c686.c   |    2 +-
>   4 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/hw/acpi.c b/hw/acpi.c
> index 5d521e5..effc7ec 100644
> --- a/hw/acpi.c
> +++ b/hw/acpi.c
> @@ -370,7 +370,7 @@ void acpi_pm1_cnt_init(ACPIREGS *ar)
>       qemu_register_wakeup_notifier(&ar->wakeup);
>   }
>
> -void acpi_pm1_cnt_write(ACPIREGS *ar, uint16_t val)
> +void acpi_pm1_cnt_write(ACPIREGS *ar, uint16_t val, char s4)
>   {
>       ar->pm1.cnt.cnt = val&  ~(ACPI_BITMASK_SLEEP_ENABLE);
>
> @@ -385,6 +385,9 @@ void acpi_pm1_cnt_write(ACPIREGS *ar, uint16_t val)
>               qemu_system_suspend_request();
>               break;
>           default:
> +            if (sus_typ == s4) { /* S4 request */
> +                qemu_system_shutdown_request();
> +            }
>               break;
>           }
>       }
> diff --git a/hw/acpi.h b/hw/acpi.h
> index fe8cdb4..7337f41 100644
> --- a/hw/acpi.h
> +++ b/hw/acpi.h
> @@ -139,7 +139,7 @@ void acpi_pm1_evt_reset(ACPIREGS *ar);
>
>   /* PM1a_CNT: piix and ich9 don't implement PM1b CNT. */
>   void acpi_pm1_cnt_init(ACPIREGS *ar);
> -void acpi_pm1_cnt_write(ACPIREGS *ar, uint16_t val);
> +void acpi_pm1_cnt_write(ACPIREGS *ar, uint16_t val, char s4);
>   void acpi_pm1_cnt_update(ACPIREGS *ar,
>                            bool sci_enable, bool sci_disable);
>   void acpi_pm1_cnt_reset(ACPIREGS *ar);
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 585da4e..883314d 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -27,6 +27,7 @@
>   #include "sysemu.h"
>   #include "range.h"
>   #include "ioport.h"
> +#include "fw_cfg.h"
>
>   //#define DEBUG
>
> @@ -71,6 +72,10 @@ typedef struct PIIX4PMState {
>       struct pci_status pci0_status;
>       uint32_t pci0_hotplug_enable;
>       uint32_t pci0_slot_device_present;
> +
> +    uint8_t disable_s3;
> +    uint8_t disable_s4;
> +    uint8_t s4_val;
>   } PIIX4PMState;
>
>   static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s);
> @@ -123,7 +128,7 @@ static void pm_ioport_write(IORange *ioport, uint64_t addr, unsigned width,
>           pm_update_sci(s);
>           break;
>       case 0x04:
> -        acpi_pm1_cnt_write(&s->ar, val);
> +        acpi_pm1_cnt_write(&s->ar, val, s->s4_val);
>           break;
>       default:
>           break;
> @@ -425,6 +430,7 @@ i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
>   {
>       PCIDevice *dev;
>       PIIX4PMState *s;
> +    uint8_t suspend[6] = {128, 0, 0, 129, 128, 128};
>
>       dev = pci_create(bus, devfn, "PIIX4_PM");
>       qdev_prop_set_uint32(&dev->qdev, "smb_io_base", smb_io_base);
> @@ -437,11 +443,19 @@ i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
>
>       qdev_init_nofail(&dev->qdev);
>
> +    suspend[3] = 1 | ((!s->disable_s3)<<  7);
> +    suspend[4] = s->s4_val | ((!s->disable_s4)<<  7);
> +
> +    fw_cfg_add_file("etc/system-states", g_memdup(suspend, 6), 6);
> +
>       return s->smb.smbus;
>   }
>
>   static Property piix4_pm_properties[] = {
>       DEFINE_PROP_UINT32("smb_io_base", PIIX4PMState, smb_io_base, 0),
> +    DEFINE_PROP_UINT8("disable_s3", PIIX4PMState, disable_s3, 0),
> +    DEFINE_PROP_UINT8("disable_s4", PIIX4PMState, disable_s4, 0),
> +    DEFINE_PROP_UINT8("s4_val", PIIX4PMState, s4_val, 2),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>
> diff --git a/hw/vt82c686.c b/hw/vt82c686.c
> index 6fb7950..5d7c00c 100644
> --- a/hw/vt82c686.c
> +++ b/hw/vt82c686.c
> @@ -210,7 +210,7 @@ static void pm_ioport_writew(void *opaque, uint32_t addr, uint32_t val)
>           pm_update_sci(s);
>           break;
>       case 0x04:
> -        acpi_pm1_cnt_write(&s->ar, val);
> +        acpi_pm1_cnt_write(&s->ar, val, 0);
>           break;
>       default:
>           break;