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

[Gleb Natapov <gleb@redhat.com>]

On Wed, May 23, 2012 at 08:27:01AM -0500, Anthony Liguori wrote:
> 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.
> 
Hmm, need to check that we have fw_cfg handy when the device is created.
Or can I add it through global?

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

--
			Gleb.