From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:59337) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SXBaq-0005lS-Jq for qemu-devel@nongnu.org; Wed, 23 May 2012 09:27:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SXBak-00081t-1R for qemu-devel@nongnu.org; Wed, 23 May 2012 09:27:12 -0400 Received: from mail-yw0-f45.google.com ([209.85.213.45]:41138) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SXBaj-00081c-SK for qemu-devel@nongnu.org; Wed, 23 May 2012 09:27:05 -0400 Received: by yhoo21 with SMTP id o21so8111940yho.4 for ; Wed, 23 May 2012 06:27:04 -0700 (PDT) Message-ID: <4FBCE5A5.7050308@codemonkey.ws> Date: Wed, 23 May 2012 08:27:01 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1337504561-20297-1-git-send-email-gleb@redhat.com> <1337504561-20297-2-git-send-email-gleb@redhat.com> In-Reply-To: <1337504561-20297-2-git-send-email-gleb@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] Add PIIX4 properties to control PM system states. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gleb Natapov Cc: qemu-devel@nongnu.org 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 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;