Patchwork [v3,2/6] suspend: switch acpi s3 to new infrastructure.

login
register
mail settings
Submitter Gerd Hoffmann
Date Feb. 8, 2012, 11 a.m.
Message ID <1328698819-31269-3-git-send-email-kraxel@redhat.com>
Download mbox | patch
Permalink /patch/140117/
State New
Headers show

Comments

Gerd Hoffmann - Feb. 8, 2012, 11 a.m.
This patch switches pc s3 suspend over to the new infrastructure.
The cmos_s3 qemu_irq is killed, the new notifier is used instead.
The xen hack goes away with that too, the hypercall can simply be
done in a notifier function now.

This patch also makes the guest actually stay suspended instead
of leaving suspend instantly, so it is useful for more than just
testing whenever the suspend/resume cycle actually works.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/acpi.c        |   11 +----------
 hw/acpi.h        |    2 --
 hw/acpi_piix4.c  |    3 +--
 hw/mc146818rtc.c |   12 ++++++++++++
 hw/mips_malta.c  |    2 +-
 hw/pc.c          |   11 -----------
 hw/pc.h          |    3 +--
 hw/pc_piix.c     |    8 +-------
 hw/vt82c686.c    |    1 -
 xen-all.c        |   11 ++++++-----
 10 files changed, 23 insertions(+), 41 deletions(-)
Gleb Natapov - Feb. 9, 2012, 8:53 a.m.
On Wed, Feb 08, 2012 at 12:00:15PM +0100, Gerd Hoffmann wrote:
> This patch switches pc s3 suspend over to the new infrastructure.
> The cmos_s3 qemu_irq is killed, the new notifier is used instead.
> The xen hack goes away with that too, the hypercall can simply be
> done in a notifier function now.
> 
> This patch also makes the guest actually stay suspended instead
> of leaving suspend instantly, so it is useful for more than just
> testing whenever the suspend/resume cycle actually works.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/acpi.c        |   11 +----------
>  hw/acpi.h        |    2 --
>  hw/acpi_piix4.c  |    3 +--
>  hw/mc146818rtc.c |   12 ++++++++++++
>  hw/mips_malta.c  |    2 +-
>  hw/pc.c          |   11 -----------
>  hw/pc.h          |    3 +--
>  hw/pc_piix.c     |    8 +-------
>  hw/vt82c686.c    |    1 -
>  xen-all.c        |   11 ++++++-----
>  10 files changed, 23 insertions(+), 41 deletions(-)
> 
> diff --git a/hw/acpi.c b/hw/acpi.c
> index 79b179b..67dcd43 100644
> --- a/hw/acpi.c
> +++ b/hw/acpi.c
> @@ -330,11 +330,6 @@ void acpi_pm_tmr_reset(ACPIPMTimer *tmr)
>  }
>  
>  /* ACPI PM1aCNT */
> -void acpi_pm1_cnt_init(ACPIPM1CNT *pm1_cnt, qemu_irq cmos_s3)
> -{
> -    pm1_cnt->cmos_s3 = cmos_s3;
> -}
> -
>  void acpi_pm1_cnt_write(ACPIPM1EVT *pm1a, ACPIPM1CNT *pm1_cnt, uint16_t val)
>  {
>      pm1_cnt->cnt = val & ~(ACPI_BITMASK_SLEEP_ENABLE);
> @@ -351,8 +346,7 @@ void acpi_pm1_cnt_write(ACPIPM1EVT *pm1a, ACPIPM1CNT *pm1_cnt, uint16_t val)
>                 Pretend that resume was caused by power button */
>              pm1a->sts |=
>                  (ACPI_BITMASK_WAKE_STATUS | ACPI_BITMASK_POWER_BUTTON_STATUS);
Here we should report real reason for a wakeup (if it can be reported in
mp1sts that is).

> -            qemu_system_reset_request();
> -            qemu_irq_raise(pm1_cnt->cmos_s3);
> +            qemu_system_suspend_request();
>          default:
>              break;
>          }
> @@ -373,9 +367,6 @@ void acpi_pm1_cnt_update(ACPIPM1CNT *pm1_cnt,
>  void acpi_pm1_cnt_reset(ACPIPM1CNT *pm1_cnt)
>  {
>      pm1_cnt->cnt = 0;
> -    if (pm1_cnt->cmos_s3) {
> -        qemu_irq_lower(pm1_cnt->cmos_s3);
> -    }
>  }
>  
>  /* ACPI GPE */
> diff --git a/hw/acpi.h b/hw/acpi.h
> index c141e65..e0c7dbe 100644
> --- a/hw/acpi.h
> +++ b/hw/acpi.h
> @@ -115,8 +115,6 @@ void acpi_pm1_evt_reset(ACPIPM1EVT *pm1);
>  /* PM1a_CNT: piix and ich9 don't implement PM1b CNT. */
>  struct ACPIPM1CNT {
>      uint16_t cnt;
> -
> -    qemu_irq cmos_s3;
>  };
>  typedef struct ACPIPM1CNT ACPIPM1CNT;
>  
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 21484ae..bc77dc5 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -376,7 +376,7 @@ static int piix4_pm_initfn(PCIDevice *dev)
>  }
>  
>  i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
> -                       qemu_irq sci_irq, qemu_irq cmos_s3, qemu_irq smi_irq,
> +                       qemu_irq sci_irq, qemu_irq smi_irq,
>                         int kvm_enabled)
>  {
>      PCIDevice *dev;
> @@ -387,7 +387,6 @@ i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
>  
>      s = DO_UPCAST(PIIX4PMState, dev, dev);
>      s->irq = sci_irq;
> -    acpi_pm1_cnt_init(&s->pm1_cnt, cmos_s3);
>      s->smi_irq = smi_irq;
>      s->kvm_enabled = kvm_enabled;
>  
> diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
> index 4a43225..314ed52 100644
> --- a/hw/mc146818rtc.c
> +++ b/hw/mc146818rtc.c
> @@ -102,6 +102,7 @@ typedef struct RTCState {
>      QEMUTimer *second_timer2;
>      Notifier clock_reset_notifier;
>      LostTickPolicy lost_tick_policy;
> +    Notifier suspend_notifier;
>  } RTCState;
>  
>  static void rtc_set_time(RTCState *s);
> @@ -596,6 +597,14 @@ static void rtc_notify_clock_reset(Notifier *notifier, void *data)
>  #endif
>  }
>  
> +/* set CMOS shutdown status register (index 0xF) as S3_resume(0xFE)
> +   BIOS will read it and start S3 resume at POST Entry */
> +static void rtc_notify_suspend(Notifier *notifier, void *data)
> +{
> +    RTCState *s = container_of(notifier, RTCState, suspend_notifier);
> +    rtc_set_memory(&s->dev, 0xF, 0xFE);
> +}
> +
>  static void rtc_reset(void *opaque)
>  {
>      RTCState *s = opaque;
> @@ -676,6 +685,9 @@ static int rtc_initfn(ISADevice *dev)
>      s->clock_reset_notifier.notify = rtc_notify_clock_reset;
>      qemu_register_clock_reset_notifier(rtc_clock, &s->clock_reset_notifier);
>  
> +    s->suspend_notifier.notify = rtc_notify_suspend;
> +    qemu_register_suspend_notifier(&s->suspend_notifier);
> +
>      s->next_second_time =
>          qemu_get_clock_ns(rtc_clock) + (get_ticks_per_sec() * 99) / 100;
>      qemu_mod_timer(s->second_timer2, s->next_second_time);
> diff --git a/hw/mips_malta.c b/hw/mips_malta.c
> index d232630..fe02c93 100644
> --- a/hw/mips_malta.c
> +++ b/hw/mips_malta.c
> @@ -966,7 +966,7 @@ void mips_malta_init (ram_addr_t ram_size,
>      pci_piix4_ide_init(pci_bus, hd, piix4_devfn + 1);
>      usb_uhci_piix4_init(pci_bus, piix4_devfn + 2);
>      smbus = piix4_pm_init(pci_bus, piix4_devfn + 3, 0x1100,
> -                          isa_get_irq(NULL, 9), NULL, NULL, 0);
> +                          isa_get_irq(NULL, 9), NULL, 0);
>      /* TODO: Populate SPD eeprom data.  */
>      smbus_eeprom_init(smbus, 8, NULL, 0);
>      pit = pit_init(isa_bus, 0x40, 0);
> diff --git a/hw/pc.c b/hw/pc.c
> index 7f3aa65..7b93aee 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -915,17 +915,6 @@ static DeviceState *apic_init(void *env, uint8_t apic_id)
>      return dev;
>  }
>  
> -/* set CMOS shutdown status register (index 0xF) as S3_resume(0xFE)
> -   BIOS will read it and start S3 resume at POST Entry */
> -void pc_cmos_set_s3_resume(void *opaque, int irq, int level)
> -{
> -    ISADevice *s = opaque;
> -
> -    if (level) {
> -        rtc_set_memory(s, 0xF, 0xFE);
> -    }
> -}
> -
>  void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
>  {
>      CPUState *s = opaque;
> diff --git a/hw/pc.h b/hw/pc.h
> index c666ec9..571c915 100644
> --- a/hw/pc.h
> +++ b/hw/pc.h
> @@ -128,7 +128,6 @@ void i8042_setup_a20_line(ISADevice *dev, qemu_irq *a20_out);
>  extern int fd_bootchk;
>  
>  void pc_register_ferr_irq(qemu_irq irq);
> -void pc_cmos_set_s3_resume(void *opaque, int irq, int level);
>  void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
>  
>  void pc_cpus_init(const char *cpu_model);
> @@ -167,7 +166,7 @@ int acpi_table_add(const char *table_desc);
>  /* acpi_piix.c */
>  
>  i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
> -                       qemu_irq sci_irq, qemu_irq cmos_s3, qemu_irq smi_irq,
> +                       qemu_irq sci_irq, qemu_irq smi_irq,
>                         int kvm_enabled);
>  void piix4_smbus_register_device(SMBusDevice *dev, uint8_t addr);
>  
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index c06f1b5..918f5ac 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -139,7 +139,6 @@ static void pc_init1(MemoryRegion *system_memory,
>      qemu_irq *cpu_irq;
>      qemu_irq *gsi;
>      qemu_irq *i8259;
> -    qemu_irq *cmos_s3;
>      qemu_irq *smi_irq;
>      GSIState *gsi_state;
>      DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
> @@ -291,15 +290,10 @@ static void pc_init1(MemoryRegion *system_memory,
>      if (pci_enabled && acpi_enabled) {
>          i2c_bus *smbus;
>  
> -        if (!xen_enabled()) {
> -            cmos_s3 = qemu_allocate_irqs(pc_cmos_set_s3_resume, rtc_state, 1);
> -        } else {
> -            cmos_s3 = qemu_allocate_irqs(xen_cmos_set_s3_resume, rtc_state, 1);
> -        }
>          smi_irq = qemu_allocate_irqs(pc_acpi_smi_interrupt, first_cpu, 1);
>          /* TODO: Populate SPD eeprom data.  */
>          smbus = piix4_pm_init(pci_bus, piix3_devfn + 3, 0xb100,
> -                              gsi[9], *cmos_s3, *smi_irq,
> +                              gsi[9], *smi_irq,
>                                kvm_enabled());
>          smbus_eeprom_init(smbus, 8, NULL, 0);
>      }
> diff --git a/hw/vt82c686.c b/hw/vt82c686.c
> index aa0954f..8beb6c5 100644
> --- a/hw/vt82c686.c
> +++ b/hw/vt82c686.c
> @@ -446,7 +446,6 @@ static int vt82c686b_pm_initfn(PCIDevice *dev)
>      apm_init(&s->apm, NULL, s);
>  
>      acpi_pm_tmr_init(&s->tmr, pm_tmr_timer);
> -    acpi_pm1_cnt_init(&s->pm1_cnt, NULL);
>  
>      pm_smbus_init(&s->dev.qdev, &s->smb);
>  
> diff --git a/xen-all.c b/xen-all.c
> index fd39168..b0ed1ed 100644
> --- a/xen-all.c
> +++ b/xen-all.c
> @@ -89,6 +89,7 @@ typedef struct XenIOState {
>      const XenPhysmap *log_for_dirtybit;
>  
>      Notifier exit;
> +    Notifier suspend;
>  } XenIOState;
>  
>  /* Xen specific function for piix pci */
> @@ -121,12 +122,9 @@ void xen_piix_pci_write_config_client(uint32_t address, uint32_t val, int len)
>      }
>  }
>  
> -void xen_cmos_set_s3_resume(void *opaque, int irq, int level)
> +static void xen_suspend_notifier(Notifier *notifier, void *data)
>  {
> -    pc_cmos_set_s3_resume(opaque, irq, level);
> -    if (level) {
> -        xc_set_hvm_param(xen_xc, xen_domid, HVM_PARAM_ACPI_S_STATE, 3);
> -    }
> +    xc_set_hvm_param(xen_xc, xen_domid, HVM_PARAM_ACPI_S_STATE, 3);
>  }
>  
>  /* Xen Interrupt Controller */
> @@ -936,6 +934,9 @@ int xen_hvm_init(void)
>      state->exit.notify = xen_exit_notifier;
>      qemu_add_exit_notifier(&state->exit);
>  
> +    state->suspend.notify = xen_suspend_notifier;
> +    qemu_register_suspend_notifier(&state->suspend);
> +
>      xc_get_hvm_param(xen_xc, xen_domid, HVM_PARAM_IOREQ_PFN, &ioreq_pfn);
>      DPRINTF("shared page at pfn %lx\n", ioreq_pfn);
>      state->shared_page = xc_map_foreign_range(xen_xc, xen_domid, XC_PAGE_SIZE,
> -- 
> 1.7.1
> 
> 

--
			Gleb.
Gerd Hoffmann - Feb. 9, 2012, 10:51 a.m.
Hi,

>>                 Pretend that resume was caused by power button */
>>              pm1a->sts |=
>>                  (ACPI_BITMASK_WAKE_STATUS | ACPI_BITMASK_POWER_BUTTON_STATUS);
> Here we should report real reason for a wakeup (if it can be reported in
> mp1sts that is).

These are available I guess?

/* PM1x_STS */
#define ACPI_BITMASK_TIMER_STATUS               0x0001
#define ACPI_BITMASK_BUS_MASTER_STATUS          0x0010
#define ACPI_BITMASK_GLOBAL_LOCK_STATUS         0x0020
#define ACPI_BITMASK_POWER_BUTTON_STATUS        0x0100
#define ACPI_BITMASK_SLEEP_BUTTON_STATUS        0x0200
#define ACPI_BITMASK_RT_CLOCK_STATUS            0x0400
#define ACPI_BITMASK_PCIEXP_WAKE_STATUS         0x4000  /* ACPI 3.0 */
#define ACPI_BITMASK_WAKE_STATUS                0x8000

What do they mean?  How would the rtc wakeup be tagged?  Set
ACPI_BITMASK_RT_CLOCK_STATUS?  Anything I can use for ps/2 kbd/mouse
wakeup?  What do you suggest to do when there is nothing usable (such as
qemu monitor command which simply doesn't exist on real hardware)?

thanks,
  Gerd
Stefano Stabellini - Feb. 9, 2012, 11:13 a.m.
On Wed, 8 Feb 2012, Gerd Hoffmann wrote:
> This patch switches pc s3 suspend over to the new infrastructure.
> The cmos_s3 qemu_irq is killed, the new notifier is used instead.
> The xen hack goes away with that too, the hypercall can simply be
> done in a notifier function now.

nice cleanup
Gleb Natapov - Feb. 9, 2012, 11:14 a.m.
On Thu, Feb 09, 2012 at 11:51:45AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> >>                 Pretend that resume was caused by power button */
> >>              pm1a->sts |=
> >>                  (ACPI_BITMASK_WAKE_STATUS | ACPI_BITMASK_POWER_BUTTON_STATUS);
> > Here we should report real reason for a wakeup (if it can be reported in
> > mp1sts that is).
> 
> These are available I guess?
> 
Yes. Once those defines had the same names as ACPI spec, but some kind soul
renamed them to be more "descriptive". So forgive me if I will use names
that you actually can lookup in ACPI spec.

> /* PM1x_STS */
> #define ACPI_BITMASK_TIMER_STATUS               0x0001
> #define ACPI_BITMASK_BUS_MASTER_STATUS          0x0010
> #define ACPI_BITMASK_GLOBAL_LOCK_STATUS         0x0020
> #define ACPI_BITMASK_POWER_BUTTON_STATUS        0x0100
> #define ACPI_BITMASK_SLEEP_BUTTON_STATUS        0x0200
> #define ACPI_BITMASK_RT_CLOCK_STATUS            0x0400
> #define ACPI_BITMASK_PCIEXP_WAKE_STATUS         0x4000  /* ACPI 3.0 */
> #define ACPI_BITMASK_WAKE_STATUS                0x8000
> 
Only three of those are actually wakeup source related:
PWRBTN_STS (bit 8)
RTC_STS (bit 10)
PCIEXP_WAKE_STS (bit 14)

And of course if system was awoken WAK_STS (bit 15) should be set too.

> What do they mean?  How would the rtc wakeup be tagged?  Set
> ACPI_BITMASK_RT_CLOCK_STATUS?  Anything I can use for ps/2 kbd/mouse
> wakeup?  What do you suggest to do when there is nothing usable (such as
> qemu monitor command which simply doesn't exist on real hardware)?
> 
RTC will set RTC_STS. In other reply I suggested to use PWRBTN_STS for
monitor command wakeup. Other devices are more complicated :( They have
to provide _PRW (Power Resources for Wake) method in the device
description in DSDT. This method, among other things, specifies which
bit in GPE (and which GPE) correspond to the device.

--
			Gleb.
Paolo Bonzini - Feb. 9, 2012, 11:17 a.m.
On 02/08/2012 12:00 PM, Gerd Hoffmann wrote:
> +/* set CMOS shutdown status register (index 0xF) as S3_resume(0xFE)
> +   BIOS will read it and start S3 resume at POST Entry */
> +static void rtc_notify_suspend(Notifier *notifier, void *data)
> +{
> +    RTCState *s = container_of(notifier, RTCState, suspend_notifier);
> +    rtc_set_memory(&s->dev, 0xF, 0xFE);
> +}
> +

Out of curiosity, who would set this on real hardware?  And since RTC 
memory is nvram, what happens if I remove the mains plug while the 
system is in S3?

Paolo
Gleb Natapov - Feb. 9, 2012, 12:31 p.m.
On Thu, Feb 09, 2012 at 12:17:33PM +0100, Paolo Bonzini wrote:
> On 02/08/2012 12:00 PM, Gerd Hoffmann wrote:
> >+/* set CMOS shutdown status register (index 0xF) as S3_resume(0xFE)
> >+   BIOS will read it and start S3 resume at POST Entry */
> >+static void rtc_notify_suspend(Notifier *notifier, void *data)
> >+{
> >+    RTCState *s = container_of(notifier, RTCState, suspend_notifier);
> >+    rtc_set_memory(&s->dev, 0xF, 0xFE);
> >+}
> >+
> 
> Out of curiosity, who would set this on real hardware?  And since
Real HW may have other ways to notify BIOS that system was S3 suspended
(special chipset register for instance). I think we can write DSDT magic
to write RTC for us, but I prefer to do it from inside QEMU.

> RTC memory is nvram, what happens if I remove the mains plug while
> the system is in S3?
> 
RTC is battery backed, but what do you expect will happen to main memory
where all your data resides during S3 in this case anyway?

--
			Gleb.
Paolo Bonzini - Feb. 9, 2012, 12:47 p.m.
On 02/09/2012 01:31 PM, Gleb Natapov wrote:
> Real HW may have other ways to notify BIOS that system was S3 suspended
> (special chipset register for instance).  I think we can write DSDT magic
> to write RTC for us, but I prefer to do it from inside QEMU.

Heh, I would prefer DSDT magic, but I wouldn't want to write it. ;)

> >  RTC memory is nvram, what happens if I remove the mains plug while
> >  the system is in S3?
>
> RTC is battery backed, but what do you expect will happen to main memory
> where all your data resides during S3 in this case anyway?

Ah, I confused the ACPI resume vector with the BIOS vector at 0040:0067. 
  No ACPI tables -> no resume vector, and S3 resume will fail.

Paolo
Gleb Natapov - Feb. 9, 2012, 12:53 p.m.
On Thu, Feb 09, 2012 at 01:47:30PM +0100, Paolo Bonzini wrote:
> On 02/09/2012 01:31 PM, Gleb Natapov wrote:
> >Real HW may have other ways to notify BIOS that system was S3 suspended
> >(special chipset register for instance).  I think we can write DSDT magic
> >to write RTC for us, but I prefer to do it from inside QEMU.
> 
> Heh, I would prefer DSDT magic, but I wouldn't want to write it. ;)
> 
The only reason I prefer QEMU doing it is because I have S3 unit test
and writing AML interpreter for it would be too much. But on the second
thought the test can write to RTC by itself, so if you'll write DSDT
code I'll ack it :)

--
			Gleb.

Patch

diff --git a/hw/acpi.c b/hw/acpi.c
index 79b179b..67dcd43 100644
--- a/hw/acpi.c
+++ b/hw/acpi.c
@@ -330,11 +330,6 @@  void acpi_pm_tmr_reset(ACPIPMTimer *tmr)
 }
 
 /* ACPI PM1aCNT */
-void acpi_pm1_cnt_init(ACPIPM1CNT *pm1_cnt, qemu_irq cmos_s3)
-{
-    pm1_cnt->cmos_s3 = cmos_s3;
-}
-
 void acpi_pm1_cnt_write(ACPIPM1EVT *pm1a, ACPIPM1CNT *pm1_cnt, uint16_t val)
 {
     pm1_cnt->cnt = val & ~(ACPI_BITMASK_SLEEP_ENABLE);
@@ -351,8 +346,7 @@  void acpi_pm1_cnt_write(ACPIPM1EVT *pm1a, ACPIPM1CNT *pm1_cnt, uint16_t val)
                Pretend that resume was caused by power button */
             pm1a->sts |=
                 (ACPI_BITMASK_WAKE_STATUS | ACPI_BITMASK_POWER_BUTTON_STATUS);
-            qemu_system_reset_request();
-            qemu_irq_raise(pm1_cnt->cmos_s3);
+            qemu_system_suspend_request();
         default:
             break;
         }
@@ -373,9 +367,6 @@  void acpi_pm1_cnt_update(ACPIPM1CNT *pm1_cnt,
 void acpi_pm1_cnt_reset(ACPIPM1CNT *pm1_cnt)
 {
     pm1_cnt->cnt = 0;
-    if (pm1_cnt->cmos_s3) {
-        qemu_irq_lower(pm1_cnt->cmos_s3);
-    }
 }
 
 /* ACPI GPE */
diff --git a/hw/acpi.h b/hw/acpi.h
index c141e65..e0c7dbe 100644
--- a/hw/acpi.h
+++ b/hw/acpi.h
@@ -115,8 +115,6 @@  void acpi_pm1_evt_reset(ACPIPM1EVT *pm1);
 /* PM1a_CNT: piix and ich9 don't implement PM1b CNT. */
 struct ACPIPM1CNT {
     uint16_t cnt;
-
-    qemu_irq cmos_s3;
 };
 typedef struct ACPIPM1CNT ACPIPM1CNT;
 
diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 21484ae..bc77dc5 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -376,7 +376,7 @@  static int piix4_pm_initfn(PCIDevice *dev)
 }
 
 i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
-                       qemu_irq sci_irq, qemu_irq cmos_s3, qemu_irq smi_irq,
+                       qemu_irq sci_irq, qemu_irq smi_irq,
                        int kvm_enabled)
 {
     PCIDevice *dev;
@@ -387,7 +387,6 @@  i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
 
     s = DO_UPCAST(PIIX4PMState, dev, dev);
     s->irq = sci_irq;
-    acpi_pm1_cnt_init(&s->pm1_cnt, cmos_s3);
     s->smi_irq = smi_irq;
     s->kvm_enabled = kvm_enabled;
 
diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index 4a43225..314ed52 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -102,6 +102,7 @@  typedef struct RTCState {
     QEMUTimer *second_timer2;
     Notifier clock_reset_notifier;
     LostTickPolicy lost_tick_policy;
+    Notifier suspend_notifier;
 } RTCState;
 
 static void rtc_set_time(RTCState *s);
@@ -596,6 +597,14 @@  static void rtc_notify_clock_reset(Notifier *notifier, void *data)
 #endif
 }
 
+/* set CMOS shutdown status register (index 0xF) as S3_resume(0xFE)
+   BIOS will read it and start S3 resume at POST Entry */
+static void rtc_notify_suspend(Notifier *notifier, void *data)
+{
+    RTCState *s = container_of(notifier, RTCState, suspend_notifier);
+    rtc_set_memory(&s->dev, 0xF, 0xFE);
+}
+
 static void rtc_reset(void *opaque)
 {
     RTCState *s = opaque;
@@ -676,6 +685,9 @@  static int rtc_initfn(ISADevice *dev)
     s->clock_reset_notifier.notify = rtc_notify_clock_reset;
     qemu_register_clock_reset_notifier(rtc_clock, &s->clock_reset_notifier);
 
+    s->suspend_notifier.notify = rtc_notify_suspend;
+    qemu_register_suspend_notifier(&s->suspend_notifier);
+
     s->next_second_time =
         qemu_get_clock_ns(rtc_clock) + (get_ticks_per_sec() * 99) / 100;
     qemu_mod_timer(s->second_timer2, s->next_second_time);
diff --git a/hw/mips_malta.c b/hw/mips_malta.c
index d232630..fe02c93 100644
--- a/hw/mips_malta.c
+++ b/hw/mips_malta.c
@@ -966,7 +966,7 @@  void mips_malta_init (ram_addr_t ram_size,
     pci_piix4_ide_init(pci_bus, hd, piix4_devfn + 1);
     usb_uhci_piix4_init(pci_bus, piix4_devfn + 2);
     smbus = piix4_pm_init(pci_bus, piix4_devfn + 3, 0x1100,
-                          isa_get_irq(NULL, 9), NULL, NULL, 0);
+                          isa_get_irq(NULL, 9), NULL, 0);
     /* TODO: Populate SPD eeprom data.  */
     smbus_eeprom_init(smbus, 8, NULL, 0);
     pit = pit_init(isa_bus, 0x40, 0);
diff --git a/hw/pc.c b/hw/pc.c
index 7f3aa65..7b93aee 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -915,17 +915,6 @@  static DeviceState *apic_init(void *env, uint8_t apic_id)
     return dev;
 }
 
-/* set CMOS shutdown status register (index 0xF) as S3_resume(0xFE)
-   BIOS will read it and start S3 resume at POST Entry */
-void pc_cmos_set_s3_resume(void *opaque, int irq, int level)
-{
-    ISADevice *s = opaque;
-
-    if (level) {
-        rtc_set_memory(s, 0xF, 0xFE);
-    }
-}
-
 void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
 {
     CPUState *s = opaque;
diff --git a/hw/pc.h b/hw/pc.h
index c666ec9..571c915 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -128,7 +128,6 @@  void i8042_setup_a20_line(ISADevice *dev, qemu_irq *a20_out);
 extern int fd_bootchk;
 
 void pc_register_ferr_irq(qemu_irq irq);
-void pc_cmos_set_s3_resume(void *opaque, int irq, int level);
 void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
 
 void pc_cpus_init(const char *cpu_model);
@@ -167,7 +166,7 @@  int acpi_table_add(const char *table_desc);
 /* acpi_piix.c */
 
 i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
-                       qemu_irq sci_irq, qemu_irq cmos_s3, qemu_irq smi_irq,
+                       qemu_irq sci_irq, qemu_irq smi_irq,
                        int kvm_enabled);
 void piix4_smbus_register_device(SMBusDevice *dev, uint8_t addr);
 
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index c06f1b5..918f5ac 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -139,7 +139,6 @@  static void pc_init1(MemoryRegion *system_memory,
     qemu_irq *cpu_irq;
     qemu_irq *gsi;
     qemu_irq *i8259;
-    qemu_irq *cmos_s3;
     qemu_irq *smi_irq;
     GSIState *gsi_state;
     DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
@@ -291,15 +290,10 @@  static void pc_init1(MemoryRegion *system_memory,
     if (pci_enabled && acpi_enabled) {
         i2c_bus *smbus;
 
-        if (!xen_enabled()) {
-            cmos_s3 = qemu_allocate_irqs(pc_cmos_set_s3_resume, rtc_state, 1);
-        } else {
-            cmos_s3 = qemu_allocate_irqs(xen_cmos_set_s3_resume, rtc_state, 1);
-        }
         smi_irq = qemu_allocate_irqs(pc_acpi_smi_interrupt, first_cpu, 1);
         /* TODO: Populate SPD eeprom data.  */
         smbus = piix4_pm_init(pci_bus, piix3_devfn + 3, 0xb100,
-                              gsi[9], *cmos_s3, *smi_irq,
+                              gsi[9], *smi_irq,
                               kvm_enabled());
         smbus_eeprom_init(smbus, 8, NULL, 0);
     }
diff --git a/hw/vt82c686.c b/hw/vt82c686.c
index aa0954f..8beb6c5 100644
--- a/hw/vt82c686.c
+++ b/hw/vt82c686.c
@@ -446,7 +446,6 @@  static int vt82c686b_pm_initfn(PCIDevice *dev)
     apm_init(&s->apm, NULL, s);
 
     acpi_pm_tmr_init(&s->tmr, pm_tmr_timer);
-    acpi_pm1_cnt_init(&s->pm1_cnt, NULL);
 
     pm_smbus_init(&s->dev.qdev, &s->smb);
 
diff --git a/xen-all.c b/xen-all.c
index fd39168..b0ed1ed 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -89,6 +89,7 @@  typedef struct XenIOState {
     const XenPhysmap *log_for_dirtybit;
 
     Notifier exit;
+    Notifier suspend;
 } XenIOState;
 
 /* Xen specific function for piix pci */
@@ -121,12 +122,9 @@  void xen_piix_pci_write_config_client(uint32_t address, uint32_t val, int len)
     }
 }
 
-void xen_cmos_set_s3_resume(void *opaque, int irq, int level)
+static void xen_suspend_notifier(Notifier *notifier, void *data)
 {
-    pc_cmos_set_s3_resume(opaque, irq, level);
-    if (level) {
-        xc_set_hvm_param(xen_xc, xen_domid, HVM_PARAM_ACPI_S_STATE, 3);
-    }
+    xc_set_hvm_param(xen_xc, xen_domid, HVM_PARAM_ACPI_S_STATE, 3);
 }
 
 /* Xen Interrupt Controller */
@@ -936,6 +934,9 @@  int xen_hvm_init(void)
     state->exit.notify = xen_exit_notifier;
     qemu_add_exit_notifier(&state->exit);
 
+    state->suspend.notify = xen_suspend_notifier;
+    qemu_register_suspend_notifier(&state->suspend);
+
     xc_get_hvm_param(xen_xc, xen_domid, HVM_PARAM_IOREQ_PFN, &ioreq_pfn);
     DPRINTF("shared page at pfn %lx\n", ioreq_pfn);
     state->shared_page = xc_map_foreign_range(xen_xc, xen_domid, XC_PAGE_SIZE,