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

[Anthony Liguori <anthony@codemonkey.ws>]

On 05/23/2012 08:34 AM, Gleb Natapov wrote:
> 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.

Yes, the pc.c/pc_piix.c split needs to die...

For now, just add another output parameter to pc_memory_init to return fw_cfg. 
Then you can pass it to piix4_pm_init().

Regards,

Anthony Liguori

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