[PATCH 0/3] S3 and S4 sleep for loongarch/virt & microvm

[PATCH 0/3] S3 and S4 sleep for loongarch/virt & microvm

[Jiaxun Yang]

Hi all,

This series implemented S3 and S4 sleep for loongarch virt machine
and microvm.

For loongarch/virt a kernel patch is requried [1].

[1]: https://lore.kernel.org/loongarch/20240613-loongarch64-sleep-v1-0-a245232af5e4@flygoat.com/

Please review.
Thanks

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
Jiaxun Yang (3):
      acpi/ged: Implement S3 and S4 sleep
      hw/loongarch/virt: Wire up S3 and S4 sleep
      hw/i386/microvm: Wire up S3 and S4 sleep

 hw/acpi/generic_event_device.c         | 70 ++++++++++++++++++++++++++++++----
 hw/i386/acpi-microvm.c                 | 18 +++++++++
 hw/i386/microvm.c                      |  3 ++
 hw/loongarch/acpi-build.c              | 18 +++++++++
 hw/loongarch/virt.c                    |  3 ++
 include/hw/acpi/generic_event_device.h | 12 +++++-
 6 files changed, 115 insertions(+), 9 deletions(-)
---
base-commit: f3e8cc47de2bc537d4991e883a85208e4e1c0f98
change-id: 20240613-loongarch64-sleep-37b2466b8d76

Best regards,
-- 
Jiaxun Yang <jiaxun.yang@flygoat.com>

[PATCH 1/3] acpi/ged: Implement S3 and S4 sleep

[Jiaxun Yang]

Implement S3 and S4 sleep with ACPI_GED_REG_SLEEP_CTL.SLP_TYP
writes.

Implement wakeup callback and WAK_STS register to inform guest
about current states.

All new functions are gated by "slp-typs" property, it is defaulted
to S5 only and machines can opt-in for S3 and S4.

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 hw/acpi/generic_event_device.c         | 70 ++++++++++++++++++++++++++++++----
 include/hw/acpi/generic_event_device.h | 12 +++++-
 2 files changed, 73 insertions(+), 9 deletions(-)

diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index 2d6e91b124e5..f1fc99c04011 100644
--- a/hw/acpi/generic_event_device.c
+++ b/hw/acpi/generic_event_device.c
@@ -11,6 +11,7 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qapi/qapi-events-run-state.h"
 #include "hw/acpi/acpi.h"
 #include "hw/acpi/generic_event_device.h"
 #include "hw/irq.h"
@@ -186,24 +187,53 @@ static const MemoryRegionOps ged_evt_ops = {
 
 static uint64_t ged_regs_read(void *opaque, hwaddr addr, unsigned size)
 {
+    GEDState *ged_st = opaque;
+
+    switch (addr) {
+    case ACPI_GED_REG_SLEEP_STS:
+        return ged_st->sleep_sts;
+    default:
+        break;
+    }
+
     return 0;
 }
 
 static void ged_regs_write(void *opaque, hwaddr addr, uint64_t data,
                            unsigned int size)
 {
-    bool slp_en;
-    int slp_typ;
+    GEDState *ged_st = opaque;
+    AcpiGedState *s = container_of(ged_st, AcpiGedState, ged_state);
 
     switch (addr) {
     case ACPI_GED_REG_SLEEP_CTL:
-        slp_typ = (data >> 2) & 0x07;
-        slp_en  = (data >> 5) & 0x01;
-        if (slp_en && slp_typ == 5) {
-            qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
+        if (data & ACPI_GED_SLP_EN) {
+            switch (extract8(data, 2, 3)) {
+            case ACPI_GED_SLP_TYP_S3:
+                if (s->slp_typs_bitmap & (1 << ACPI_GED_SLP_TYP_S3)) {
+                    qemu_system_suspend_request();
+                }
+                break;
+            case ACPI_GED_SLP_TYP_S4:
+                if (s->slp_typs_bitmap & (1 << ACPI_GED_SLP_TYP_S4)) {
+                    qapi_event_send_suspend_disk();
+                    qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
+                }
+                break;
+            case ACPI_GED_SLP_TYP_S5:
+                if (s->slp_typs_bitmap & (1 << ACPI_GED_SLP_TYP_S5)) {
+                    qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
+                }
+                break;
+            default:
+                break;
+            }
         }
         return;
     case ACPI_GED_REG_SLEEP_STS:
+        if (data & ACPI_GED_WAK_STS) {
+            ged_st->sleep_sts &= ~ACPI_GED_WAK_STS;
+        }
         return;
     case ACPI_GED_REG_RESET:
         if (data == ACPI_GED_RESET_VALUE) {
@@ -223,6 +253,14 @@ static const MemoryRegionOps ged_regs_ops = {
     },
 };
 
+static void acpi_ged_notify_wakeup(Notifier *notifier, void *data)
+{
+    GEDState *ged_st = container_of(notifier, GEDState, wakeup);
+
+    ged_st->sleep_sts |= ACPI_GED_WAK_STS;
+}
+
+
 static void acpi_ged_device_plug_cb(HotplugHandler *hotplug_dev,
                                     DeviceState *dev, Error **errp)
 {
@@ -305,6 +343,8 @@ static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
 
 static Property acpi_ged_properties[] = {
     DEFINE_PROP_UINT32("ged-event", AcpiGedState, ged_event_bitmap, 0),
+    DEFINE_PROP_UINT32("slp-typs", AcpiGedState, slp_typs_bitmap,
+                        (1 << ACPI_GED_SLP_TYP_S5)),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -320,10 +360,11 @@ static const VMStateDescription vmstate_memhp_state = {
 
 static const VMStateDescription vmstate_ged_state = {
     .name = "acpi-ged-state",
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (const VMStateField[]) {
         VMSTATE_UINT32(sel, GEDState),
+        VMSTATE_UINT8(sleep_sts, GEDState),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -371,6 +412,18 @@ static const VMStateDescription vmstate_acpi_ged = {
     }
 };
 
+static void acpi_ged_realize(DeviceState *dev, Error **errp)
+{
+    AcpiGedState *s = ACPI_GED(dev);
+    GEDState *ged_st = &s->ged_state;
+
+    if (s->slp_typs_bitmap & (1 << ACPI_GED_SLP_TYP_S3)) {
+        ged_st->wakeup.notify = acpi_ged_notify_wakeup;
+        qemu_register_wakeup_notifier(&ged_st->wakeup);
+        qemu_register_wakeup_support();
+    }
+}
+
 static void acpi_ged_initfn(Object *obj)
 {
     DeviceState *dev = DEVICE(obj);
@@ -409,6 +462,7 @@ static void acpi_ged_class_init(ObjectClass *class, void *data)
     AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_CLASS(class);
 
     dc->desc = "ACPI Generic Event Device";
+    dc->realize = acpi_ged_realize;
     device_class_set_props(dc, acpi_ged_properties);
     dc->vmsd = &vmstate_acpi_ged;
 
diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
index ba84ce021477..1ea3cb848679 100644
--- a/include/hw/acpi/generic_event_device.h
+++ b/include/hw/acpi/generic_event_device.h
@@ -80,9 +80,16 @@ OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
 /* ACPI_GED_REG_RESET value for reset*/
 #define ACPI_GED_RESET_VALUE       0x42
 
-/* ACPI_GED_REG_SLEEP_CTL.SLP_TYP value for S5 (aka poweroff) */
+/* ACPI_GED_REG_SLEEP_CTL.SLP_EN bit */
+#define ACPI_GED_SLP_EN            (1 << 5)
+
+/* ACPI_GED_REG_SLEEP_CTL.SLP_TYP values */
+#define ACPI_GED_SLP_TYP_S3        0x03
+#define ACPI_GED_SLP_TYP_S4        0x04
 #define ACPI_GED_SLP_TYP_S5        0x05
 
+#define ACPI_GED_WAK_STS           (1 << 7)
+
 #define GED_DEVICE      "GED"
 #define AML_GED_EVT_REG "EREG"
 #define AML_GED_EVT_SEL "ESEL"
@@ -99,7 +106,9 @@ OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
 typedef struct GEDState {
     MemoryRegion evt;
     MemoryRegion regs;
+    Notifier     wakeup;
     uint32_t     sel;
+    uint8_t      sleep_sts;
 } GEDState;
 
 struct AcpiGedState {
@@ -108,6 +117,7 @@ struct AcpiGedState {
     MemoryRegion container_memhp;
     GEDState ged_state;
     uint32_t ged_event_bitmap;
+    uint32_t slp_typs_bitmap;
     qemu_irq irq;
     AcpiGhesState ghes_state;
 };

-- 
2.43.0

Re: [PATCH 1/3] acpi/ged: Implement S3 and S4 sleep

[Michael S. Tsirkin]

On Thu, Jun 13, 2024 at 06:30:15PM +0100, Jiaxun Yang wrote:
> Implement S3 and S4 sleep with ACPI_GED_REG_SLEEP_CTL.SLP_TYP
> writes.
> 
> Implement wakeup callback and WAK_STS register to inform guest
> about current states.
> 
> All new functions are gated by "slp-typs" property, it is defaulted
> to S5 only and machines can opt-in for S3 and S4.
> 
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
>  hw/acpi/generic_event_device.c         | 70 ++++++++++++++++++++++++++++++----
>  include/hw/acpi/generic_event_device.h | 12 +++++-
>  2 files changed, 73 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> index 2d6e91b124e5..f1fc99c04011 100644
> --- a/hw/acpi/generic_event_device.c
> +++ b/hw/acpi/generic_event_device.c
> @@ -11,6 +11,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
> +#include "qapi/qapi-events-run-state.h"
>  #include "hw/acpi/acpi.h"
>  #include "hw/acpi/generic_event_device.h"
>  #include "hw/irq.h"
> @@ -186,24 +187,53 @@ static const MemoryRegionOps ged_evt_ops = {
>  
>  static uint64_t ged_regs_read(void *opaque, hwaddr addr, unsigned size)
>  {
> +    GEDState *ged_st = opaque;
> +
> +    switch (addr) {
> +    case ACPI_GED_REG_SLEEP_STS:
> +        return ged_st->sleep_sts;
> +    default:
> +        break;
> +    }
> +
>      return 0;
>  }
>  
>  static void ged_regs_write(void *opaque, hwaddr addr, uint64_t data,
>                             unsigned int size)
>  {
> -    bool slp_en;
> -    int slp_typ;
> +    GEDState *ged_st = opaque;
> +    AcpiGedState *s = container_of(ged_st, AcpiGedState, ged_state);
>  
>      switch (addr) {
>      case ACPI_GED_REG_SLEEP_CTL:
> -        slp_typ = (data >> 2) & 0x07;
> -        slp_en  = (data >> 5) & 0x01;
> -        if (slp_en && slp_typ == 5) {
> -            qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> +        if (data & ACPI_GED_SLP_EN) {
> +            switch (extract8(data, 2, 3)) {
> +            case ACPI_GED_SLP_TYP_S3:
> +                if (s->slp_typs_bitmap & (1 << ACPI_GED_SLP_TYP_S3)) {
> +                    qemu_system_suspend_request();
> +                }
> +                break;
> +            case ACPI_GED_SLP_TYP_S4:
> +                if (s->slp_typs_bitmap & (1 << ACPI_GED_SLP_TYP_S4)) {
> +                    qapi_event_send_suspend_disk();
> +                    qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> +                }
> +                break;
> +            case ACPI_GED_SLP_TYP_S5:
> +                if (s->slp_typs_bitmap & (1 << ACPI_GED_SLP_TYP_S5)) {
> +                    qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> +                }
> +                break;
> +            default:
> +                break;
> +            }
>          }
>          return;
>      case ACPI_GED_REG_SLEEP_STS:
> +        if (data & ACPI_GED_WAK_STS) {
> +            ged_st->sleep_sts &= ~ACPI_GED_WAK_STS;
> +        }
>          return;
>      case ACPI_GED_REG_RESET:
>          if (data == ACPI_GED_RESET_VALUE) {
> @@ -223,6 +253,14 @@ static const MemoryRegionOps ged_regs_ops = {
>      },
>  };
>  
> +static void acpi_ged_notify_wakeup(Notifier *notifier, void *data)
> +{
> +    GEDState *ged_st = container_of(notifier, GEDState, wakeup);
> +
> +    ged_st->sleep_sts |= ACPI_GED_WAK_STS;
> +}
> +
> +
>  static void acpi_ged_device_plug_cb(HotplugHandler *hotplug_dev,
>                                      DeviceState *dev, Error **errp)
>  {
> @@ -305,6 +343,8 @@ static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
>  
>  static Property acpi_ged_properties[] = {
>      DEFINE_PROP_UINT32("ged-event", AcpiGedState, ged_event_bitmap, 0),
> +    DEFINE_PROP_UINT32("slp-typs", AcpiGedState, slp_typs_bitmap,
> +                        (1 << ACPI_GED_SLP_TYP_S5)),


I don't see an immediate need for users to tweak this.
Accordingly, prefix this property with "x-" so users know that
if they do, this is unsupported.


>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -320,10 +360,11 @@ static const VMStateDescription vmstate_memhp_state = {
>  
>  static const VMStateDescription vmstate_ged_state = {
>      .name = "acpi-ged-state",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
>      .fields = (const VMStateField[]) {
>          VMSTATE_UINT32(sel, GEDState),
> +        VMSTATE_UINT8(sleep_sts, GEDState),
>          VMSTATE_END_OF_LIST()
>      }
>  };


No, avoid playing with versions please.
Use a conditional section instead, so format does not change
for existing machine types.


> @@ -371,6 +412,18 @@ static const VMStateDescription vmstate_acpi_ged = {
>      }
>  };
>  
> +static void acpi_ged_realize(DeviceState *dev, Error **errp)
> +{
> +    AcpiGedState *s = ACPI_GED(dev);
> +    GEDState *ged_st = &s->ged_state;
> +
> +    if (s->slp_typs_bitmap & (1 << ACPI_GED_SLP_TYP_S3)) {
> +        ged_st->wakeup.notify = acpi_ged_notify_wakeup;
> +        qemu_register_wakeup_notifier(&ged_st->wakeup);
> +        qemu_register_wakeup_support();
> +    }
> +}
> +
>  static void acpi_ged_initfn(Object *obj)
>  {
>      DeviceState *dev = DEVICE(obj);
> @@ -409,6 +462,7 @@ static void acpi_ged_class_init(ObjectClass *class, void *data)
>      AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_CLASS(class);
>  
>      dc->desc = "ACPI Generic Event Device";
> +    dc->realize = acpi_ged_realize;
>      device_class_set_props(dc, acpi_ged_properties);
>      dc->vmsd = &vmstate_acpi_ged;
>  
> diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
> index ba84ce021477..1ea3cb848679 100644
> --- a/include/hw/acpi/generic_event_device.h
> +++ b/include/hw/acpi/generic_event_device.h
> @@ -80,9 +80,16 @@ OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
>  /* ACPI_GED_REG_RESET value for reset*/
>  #define ACPI_GED_RESET_VALUE       0x42
>  
> -/* ACPI_GED_REG_SLEEP_CTL.SLP_TYP value for S5 (aka poweroff) */
> +/* ACPI_GED_REG_SLEEP_CTL.SLP_EN bit */
> +#define ACPI_GED_SLP_EN            (1 << 5)
> +
> +/* ACPI_GED_REG_SLEEP_CTL.SLP_TYP values */
> +#define ACPI_GED_SLP_TYP_S3        0x03
> +#define ACPI_GED_SLP_TYP_S4        0x04
>  #define ACPI_GED_SLP_TYP_S5        0x05
>  
> +#define ACPI_GED_WAK_STS           (1 << 7)
> +
>  #define GED_DEVICE      "GED"
>  #define AML_GED_EVT_REG "EREG"
>  #define AML_GED_EVT_SEL "ESEL"
> @@ -99,7 +106,9 @@ OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
>  typedef struct GEDState {
>      MemoryRegion evt;
>      MemoryRegion regs;
> +    Notifier     wakeup;
>      uint32_t     sel;
> +    uint8_t      sleep_sts;
>  } GEDState;
>  
>  struct AcpiGedState {
> @@ -108,6 +117,7 @@ struct AcpiGedState {
>      MemoryRegion container_memhp;
>      GEDState ged_state;
>      uint32_t ged_event_bitmap;
> +    uint32_t slp_typs_bitmap;
>      qemu_irq irq;
>      AcpiGhesState ghes_state;
>  };
> 
> -- 
> 2.43.0

Re: [PATCH 1/3] acpi/ged: Implement S3 and S4 sleep

[Igor Mammedov]

On Thu, 13 Jun 2024 18:30:15 +0100
Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:

> Implement S3 and S4 sleep with ACPI_GED_REG_SLEEP_CTL.SLP_TYP
> writes.
> 
> Implement wakeup callback and WAK_STS register to inform guest
> about current states.
> 
> All new functions are gated by "slp-typs" property, it is defaulted
> to S5 only and machines can opt-in for S3 and S4.

subject says S3 + S4 and don't mention S5
the same happens throughout the series, please fix it up

PS:
please reference relevant ACPI portions in the patch below,
so reader could easily find and understand what code does.

> 
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
>  hw/acpi/generic_event_device.c         | 70 ++++++++++++++++++++++++++++++----
>  include/hw/acpi/generic_event_device.h | 12 +++++-
>  2 files changed, 73 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> index 2d6e91b124e5..f1fc99c04011 100644
> --- a/hw/acpi/generic_event_device.c
> +++ b/hw/acpi/generic_event_device.c
> @@ -11,6 +11,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
> +#include "qapi/qapi-events-run-state.h"
>  #include "hw/acpi/acpi.h"
>  #include "hw/acpi/generic_event_device.h"
>  #include "hw/irq.h"
> @@ -186,24 +187,53 @@ static const MemoryRegionOps ged_evt_ops = {
>  
>  static uint64_t ged_regs_read(void *opaque, hwaddr addr, unsigned size)
>  {
> +    GEDState *ged_st = opaque;
> +
> +    switch (addr) {
> +    case ACPI_GED_REG_SLEEP_STS:
> +        return ged_st->sleep_sts;
> +    default:
> +        break;
> +    }
> +
>      return 0;
>  }
>  
>  static void ged_regs_write(void *opaque, hwaddr addr, uint64_t data,
>                             unsigned int size)
>  {
> -    bool slp_en;
> -    int slp_typ;
> +    GEDState *ged_st = opaque;
> +    AcpiGedState *s = container_of(ged_st, AcpiGedState, ged_state);
>  
>      switch (addr) {
>      case ACPI_GED_REG_SLEEP_CTL:
> -        slp_typ = (data >> 2) & 0x07;
> -        slp_en  = (data >> 5) & 0x01;
maybe use defines instead of magic numbers, it's also good to add
coments here referring to concrete chapter in APCI spec that describe
what these numbers are.

> -        if (slp_en && slp_typ == 5) {
                                    ^^^
ditto

> -            qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> +        if (data & ACPI_GED_SLP_EN) {
> +            switch (extract8(data, 2, 3)) {
> +            case ACPI_GED_SLP_TYP_S3:
> +                if (s->slp_typs_bitmap & (1 << ACPI_GED_SLP_TYP_S3)) {

why not use existing helpers like test_bit()
the same applies to following bit checks

> +                    qemu_system_suspend_request();
> +                }
> +                break;
> +            case ACPI_GED_SLP_TYP_S4:
> +                if (s->slp_typs_bitmap & (1 << ACPI_GED_SLP_TYP_S4)) {
> +                    qapi_event_send_suspend_disk();
> +                    qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> +                }
> +                break;
> +            case ACPI_GED_SLP_TYP_S5:
> +                if (s->slp_typs_bitmap & (1 << ACPI_GED_SLP_TYP_S5)) {
> +                    qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> +                }
> +                break;
> +            default:
> +                break;
> +            }
>          }
>          return;
>      case ACPI_GED_REG_SLEEP_STS:
> +        if (data & ACPI_GED_WAK_STS) {
> +            ged_st->sleep_sts &= ~ACPI_GED_WAK_STS;
> +        }
>          return;
>      case ACPI_GED_REG_RESET:
>          if (data == ACPI_GED_RESET_VALUE) {
> @@ -223,6 +253,14 @@ static const MemoryRegionOps ged_regs_ops = {
>      },
>  };
>  
> +static void acpi_ged_notify_wakeup(Notifier *notifier, void *data)
> +{
> +    GEDState *ged_st = container_of(notifier, GEDState, wakeup);
> +
> +    ged_st->sleep_sts |= ACPI_GED_WAK_STS;

describe somewhere workflow how it is supposed to work 
(commit message or add ged specific doc in docs/specs/
as the 1st patch)

> +}
> +
> +
>  static void acpi_ged_device_plug_cb(HotplugHandler *hotplug_dev,
>                                      DeviceState *dev, Error **errp)
>  {
> @@ -305,6 +343,8 @@ static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
>  
>  static Property acpi_ged_properties[] = {
>      DEFINE_PROP_UINT32("ged-event", AcpiGedState, ged_event_bitmap, 0),
> +    DEFINE_PROP_UINT32("slp-typs", AcpiGedState, slp_typs_bitmap,
> +                        (1 << ACPI_GED_SLP_TYP_S5)),

I'd default to everything enabled, and use compat mechanism
to disable it on older machine types.

You have to do this as ged is also used by versioned arm/virt machine

>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -320,10 +360,11 @@ static const VMStateDescription vmstate_memhp_state = {
>  
>  static const VMStateDescription vmstate_ged_state = {
>      .name = "acpi-ged-state",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
>      .fields = (const VMStateField[]) {
>          VMSTATE_UINT32(sel, GEDState),
> +        VMSTATE_UINT8(sleep_sts, GEDState),
>          VMSTATE_END_OF_LIST()
>      }
>  };

see for example
commit 829600a519386c7b188d5d813e78ba69bf0bd323
    hpet: recover timer offset correctly



> @@ -371,6 +412,18 @@ static const VMStateDescription vmstate_acpi_ged = {
>      }
>  };
>  
> +static void acpi_ged_realize(DeviceState *dev, Error **errp)
> +{
> +    AcpiGedState *s = ACPI_GED(dev);
> +    GEDState *ged_st = &s->ged_state;
> +
> +    if (s->slp_typs_bitmap & (1 << ACPI_GED_SLP_TYP_S3)) {
> +        ged_st->wakeup.notify = acpi_ged_notify_wakeup;
> +        qemu_register_wakeup_notifier(&ged_st->wakeup);
> +        qemu_register_wakeup_support();
> +    }
> +}
> +
>  static void acpi_ged_initfn(Object *obj)
>  {
>      DeviceState *dev = DEVICE(obj);
> @@ -409,6 +462,7 @@ static void acpi_ged_class_init(ObjectClass *class, void *data)
>      AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_CLASS(class);
>  
>      dc->desc = "ACPI Generic Event Device";
> +    dc->realize = acpi_ged_realize;

realize was added recently (currently in master),
please rebase on top of current master tree

>      device_class_set_props(dc, acpi_ged_properties);
>      dc->vmsd = &vmstate_acpi_ged;
>  
> diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
> index ba84ce021477..1ea3cb848679 100644
> --- a/include/hw/acpi/generic_event_device.h
> +++ b/include/hw/acpi/generic_event_device.h
> @@ -80,9 +80,16 @@ OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
>  /* ACPI_GED_REG_RESET value for reset*/
>  #define ACPI_GED_RESET_VALUE       0x42
>  
> -/* ACPI_GED_REG_SLEEP_CTL.SLP_TYP value for S5 (aka poweroff) */
> +/* ACPI_GED_REG_SLEEP_CTL.SLP_EN bit */
> +#define ACPI_GED_SLP_EN            (1 << 5)
> +
> +/* ACPI_GED_REG_SLEEP_CTL.SLP_TYP values */
> +#define ACPI_GED_SLP_TYP_S3        0x03
> +#define ACPI_GED_SLP_TYP_S4        0x04
>  #define ACPI_GED_SLP_TYP_S5        0x05
>  
> +#define ACPI_GED_WAK_STS           (1 << 7)
> +
>  #define GED_DEVICE      "GED"
>  #define AML_GED_EVT_REG "EREG"
>  #define AML_GED_EVT_SEL "ESEL"
> @@ -99,7 +106,9 @@ OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
>  typedef struct GEDState {
>      MemoryRegion evt;
>      MemoryRegion regs;
> +    Notifier     wakeup;
>      uint32_t     sel;
> +    uint8_t      sleep_sts;
>  } GEDState;
>  
>  struct AcpiGedState {
> @@ -108,6 +117,7 @@ struct AcpiGedState {
>      MemoryRegion container_memhp;
>      GEDState ged_state;
>      uint32_t ged_event_bitmap;
> +    uint32_t slp_typs_bitmap;
>      qemu_irq irq;
>      AcpiGhesState ghes_state;
>  };
> 

Re: [PATCH 1/3] acpi/ged: Implement S3 and S4 sleep

[Michael S. Tsirkin]

On Thu, Jul 25, 2024 at 10:22:15AM +0200, Igor Mammedov wrote:
> On Thu, 13 Jun 2024 18:30:15 +0100
> Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
> 
> > Implement S3 and S4 sleep with ACPI_GED_REG_SLEEP_CTL.SLP_TYP
> > writes.
> > 
> > Implement wakeup callback and WAK_STS register to inform guest
> > about current states.
> > 
> > All new functions are gated by "slp-typs" property, it is defaulted
> > to S5 only and machines can opt-in for S3 and S4.
> 
> subject says S3 + S4 and don't mention S5
> the same happens throughout the series, please fix it up
> 
> PS:
> please reference relevant ACPI portions in the patch below,
> so reader could easily find and understand what code does.
> 
> > 
> > Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> > ---
> >  hw/acpi/generic_event_device.c         | 70 ++++++++++++++++++++++++++++++----
> >  include/hw/acpi/generic_event_device.h | 12 +++++-
> >  2 files changed, 73 insertions(+), 9 deletions(-)
> > 
> > diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> > index 2d6e91b124e5..f1fc99c04011 100644
> > --- a/hw/acpi/generic_event_device.c
> > +++ b/hw/acpi/generic_event_device.c
> > @@ -11,6 +11,7 @@
> >  
> >  #include "qemu/osdep.h"
> >  #include "qapi/error.h"
> > +#include "qapi/qapi-events-run-state.h"
> >  #include "hw/acpi/acpi.h"
> >  #include "hw/acpi/generic_event_device.h"
> >  #include "hw/irq.h"
> > @@ -186,24 +187,53 @@ static const MemoryRegionOps ged_evt_ops = {
> >  
> >  static uint64_t ged_regs_read(void *opaque, hwaddr addr, unsigned size)
> >  {
> > +    GEDState *ged_st = opaque;
> > +
> > +    switch (addr) {
> > +    case ACPI_GED_REG_SLEEP_STS:
> > +        return ged_st->sleep_sts;
> > +    default:
> > +        break;
> > +    }
> > +
> >      return 0;
> >  }
> >  
> >  static void ged_regs_write(void *opaque, hwaddr addr, uint64_t data,
> >                             unsigned int size)
> >  {
> > -    bool slp_en;
> > -    int slp_typ;
> > +    GEDState *ged_st = opaque;
> > +    AcpiGedState *s = container_of(ged_st, AcpiGedState, ged_state);
> >  
> >      switch (addr) {
> >      case ACPI_GED_REG_SLEEP_CTL:
> > -        slp_typ = (data >> 2) & 0x07;
> > -        slp_en  = (data >> 5) & 0x01;
> maybe use defines instead of magic numbers, it's also good to add
> coments here referring to concrete chapter in APCI spec that describe
> what these numbers are.

In fact if you add comments you do not need defines.
One time use defines are a waste of time and hard to parse.

 slp_typ = (data >> 2 /* x Yz */ ) & 0x07 /* bits 0 to 3 */;

is clearer than

#define ACPI_S3_X_YZ 2
#define ACPI_S3_MASK 7

because that "x Yz" can match spec text *exactly*, you do
text search and you find where it is. With macros
you have to guess.

build_amd_iommu is a nice example I think.

> 
> > -        if (slp_en && slp_typ == 5) {
>                                     ^^^
> ditto

this is deleted code, isn't it?

> > -            qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> > +        if (data & ACPI_GED_SLP_EN) {
> > +            switch (extract8(data, 2, 3)) {
> > +            case ACPI_GED_SLP_TYP_S3:
> > +                if (s->slp_typs_bitmap & (1 << ACPI_GED_SLP_TYP_S3)) {
> 
> why not use existing helpers like test_bit()
> the same applies to following bit checks
> 
> > +                    qemu_system_suspend_request();
> > +                }
> > +                break;
> > +            case ACPI_GED_SLP_TYP_S4:
> > +                if (s->slp_typs_bitmap & (1 << ACPI_GED_SLP_TYP_S4)) {
> > +                    qapi_event_send_suspend_disk();
> > +                    qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> > +                }
> > +                break;
> > +            case ACPI_GED_SLP_TYP_S5:
> > +                if (s->slp_typs_bitmap & (1 << ACPI_GED_SLP_TYP_S5)) {
> > +                    qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> > +                }
> > +                break;
> > +            default:
> > +                break;
> > +            }
> >          }
> >          return;
> >      case ACPI_GED_REG_SLEEP_STS:
> > +        if (data & ACPI_GED_WAK_STS) {
> > +            ged_st->sleep_sts &= ~ACPI_GED_WAK_STS;
> > +        }
> >          return;
> >      case ACPI_GED_REG_RESET:
> >          if (data == ACPI_GED_RESET_VALUE) {
> > @@ -223,6 +253,14 @@ static const MemoryRegionOps ged_regs_ops = {
> >      },
> >  };
> >  
> > +static void acpi_ged_notify_wakeup(Notifier *notifier, void *data)
> > +{
> > +    GEDState *ged_st = container_of(notifier, GEDState, wakeup);
> > +
> > +    ged_st->sleep_sts |= ACPI_GED_WAK_STS;
> 
> describe somewhere workflow how it is supposed to work 
> (commit message or add ged specific doc in docs/specs/
> as the 1st patch)
> 
> > +}
> > +
> > +
> >  static void acpi_ged_device_plug_cb(HotplugHandler *hotplug_dev,
> >                                      DeviceState *dev, Error **errp)
> >  {
> > @@ -305,6 +343,8 @@ static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
> >  
> >  static Property acpi_ged_properties[] = {
> >      DEFINE_PROP_UINT32("ged-event", AcpiGedState, ged_event_bitmap, 0),
> > +    DEFINE_PROP_UINT32("slp-typs", AcpiGedState, slp_typs_bitmap,
> > +                        (1 << ACPI_GED_SLP_TYP_S5)),
> 
> I'd default to everything enabled, and use compat mechanism
> to disable it on older machine types.
> 
> You have to do this as ged is also used by versioned arm/virt machine
> 
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >  
> > @@ -320,10 +360,11 @@ static const VMStateDescription vmstate_memhp_state = {
> >  
> >  static const VMStateDescription vmstate_ged_state = {
> >      .name = "acpi-ged-state",
> > -    .version_id = 1,
> > -    .minimum_version_id = 1,
> > +    .version_id = 2,
> > +    .minimum_version_id = 2,
> >      .fields = (const VMStateField[]) {
> >          VMSTATE_UINT32(sel, GEDState),
> > +        VMSTATE_UINT8(sleep_sts, GEDState),
> >          VMSTATE_END_OF_LIST()
> >      }
> >  };
> 
> see for example
> commit 829600a519386c7b188d5d813e78ba69bf0bd323
>     hpet: recover timer offset correctly
> 
> 
> 
> > @@ -371,6 +412,18 @@ static const VMStateDescription vmstate_acpi_ged = {
> >      }
> >  };
> >  
> > +static void acpi_ged_realize(DeviceState *dev, Error **errp)
> > +{
> > +    AcpiGedState *s = ACPI_GED(dev);
> > +    GEDState *ged_st = &s->ged_state;
> > +
> > +    if (s->slp_typs_bitmap & (1 << ACPI_GED_SLP_TYP_S3)) {
> > +        ged_st->wakeup.notify = acpi_ged_notify_wakeup;
> > +        qemu_register_wakeup_notifier(&ged_st->wakeup);
> > +        qemu_register_wakeup_support();
> > +    }
> > +}
> > +
> >  static void acpi_ged_initfn(Object *obj)
> >  {
> >      DeviceState *dev = DEVICE(obj);
> > @@ -409,6 +462,7 @@ static void acpi_ged_class_init(ObjectClass *class, void *data)
> >      AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_CLASS(class);
> >  
> >      dc->desc = "ACPI Generic Event Device";
> > +    dc->realize = acpi_ged_realize;
> 
> realize was added recently (currently in master),
> please rebase on top of current master tree
> 
> >      device_class_set_props(dc, acpi_ged_properties);
> >      dc->vmsd = &vmstate_acpi_ged;
> >  
> > diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
> > index ba84ce021477..1ea3cb848679 100644
> > --- a/include/hw/acpi/generic_event_device.h
> > +++ b/include/hw/acpi/generic_event_device.h
> > @@ -80,9 +80,16 @@ OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
> >  /* ACPI_GED_REG_RESET value for reset*/
> >  #define ACPI_GED_RESET_VALUE       0x42
> >  
> > -/* ACPI_GED_REG_SLEEP_CTL.SLP_TYP value for S5 (aka poweroff) */
> > +/* ACPI_GED_REG_SLEEP_CTL.SLP_EN bit */
> > +#define ACPI_GED_SLP_EN            (1 << 5)
> > +
> > +/* ACPI_GED_REG_SLEEP_CTL.SLP_TYP values */
> > +#define ACPI_GED_SLP_TYP_S3        0x03
> > +#define ACPI_GED_SLP_TYP_S4        0x04
> >  #define ACPI_GED_SLP_TYP_S5        0x05
> >  
> > +#define ACPI_GED_WAK_STS           (1 << 7)
> > +
> >  #define GED_DEVICE      "GED"
> >  #define AML_GED_EVT_REG "EREG"
> >  #define AML_GED_EVT_SEL "ESEL"
> > @@ -99,7 +106,9 @@ OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
> >  typedef struct GEDState {
> >      MemoryRegion evt;
> >      MemoryRegion regs;
> > +    Notifier     wakeup;
> >      uint32_t     sel;
> > +    uint8_t      sleep_sts;
> >  } GEDState;
> >  
> >  struct AcpiGedState {
> > @@ -108,6 +117,7 @@ struct AcpiGedState {
> >      MemoryRegion container_memhp;
> >      GEDState ged_state;
> >      uint32_t ged_event_bitmap;
> > +    uint32_t slp_typs_bitmap;
> >      qemu_irq irq;
> >      AcpiGhesState ghes_state;
> >  };
> > 

Re: [PATCH 1/3] acpi/ged: Implement S3 and S4 sleep

[Igor Mammedov]

On Thu, 25 Jul 2024 04:30:42 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Jul 25, 2024 at 10:22:15AM +0200, Igor Mammedov wrote:
> > On Thu, 13 Jun 2024 18:30:15 +0100
> > Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
> >   
> > > Implement S3 and S4 sleep with ACPI_GED_REG_SLEEP_CTL.SLP_TYP
> > > writes.
> > > 
> > > Implement wakeup callback and WAK_STS register to inform guest
> > > about current states.
> > > 
> > > All new functions are gated by "slp-typs" property, it is defaulted
> > > to S5 only and machines can opt-in for S3 and S4.  
> > 
> > subject says S3 + S4 and don't mention S5
> > the same happens throughout the series, please fix it up
> > 
> > PS:
> > please reference relevant ACPI portions in the patch below,
> > so reader could easily find and understand what code does.
> >   
> > > 
> > > Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> > > ---
> > >  hw/acpi/generic_event_device.c         | 70 ++++++++++++++++++++++++++++++----
> > >  include/hw/acpi/generic_event_device.h | 12 +++++-
> > >  2 files changed, 73 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> > > index 2d6e91b124e5..f1fc99c04011 100644
> > > --- a/hw/acpi/generic_event_device.c
> > > +++ b/hw/acpi/generic_event_device.c
> > > @@ -11,6 +11,7 @@
> > >  
> > >  #include "qemu/osdep.h"
> > >  #include "qapi/error.h"
> > > +#include "qapi/qapi-events-run-state.h"
> > >  #include "hw/acpi/acpi.h"
> > >  #include "hw/acpi/generic_event_device.h"
> > >  #include "hw/irq.h"
> > > @@ -186,24 +187,53 @@ static const MemoryRegionOps ged_evt_ops = {
> > >  
> > >  static uint64_t ged_regs_read(void *opaque, hwaddr addr, unsigned size)
> > >  {
> > > +    GEDState *ged_st = opaque;
> > > +
> > > +    switch (addr) {
> > > +    case ACPI_GED_REG_SLEEP_STS:
> > > +        return ged_st->sleep_sts;
> > > +    default:
> > > +        break;
> > > +    }
> > > +
> > >      return 0;
> > >  }
> > >  
> > >  static void ged_regs_write(void *opaque, hwaddr addr, uint64_t data,
> > >                             unsigned int size)
> > >  {
> > > -    bool slp_en;
> > > -    int slp_typ;
> > > +    GEDState *ged_st = opaque;
> > > +    AcpiGedState *s = container_of(ged_st, AcpiGedState, ged_state);
> > >  
> > >      switch (addr) {
> > >      case ACPI_GED_REG_SLEEP_CTL:
> > > -        slp_typ = (data >> 2) & 0x07;
> > > -        slp_en  = (data >> 5) & 0x01;  
> > maybe use defines instead of magic numbers, it's also good to add
> > coments here referring to concrete chapter in APCI spec that describe
> > what these numbers are.  
> 
> In fact if you add comments you do not need defines.
> One time use defines are a waste of time and hard to parse.
> 
>  slp_typ = (data >> 2 /* x Yz */ ) & 0x07 /* bits 0 to 3 */;
> 
> is clearer than
> 
> #define ACPI_S3_X_YZ 2
> #define ACPI_S3_MASK 7
> 
> because that "x Yz" can match spec text *exactly*, you do
> text search and you find where it is. With macros
> you have to guess.

seconded, I'd also prefer comments (for 1-off use) that
match spec _exactly_ so reader could easily find relevant
place in spec

> build_amd_iommu is a nice example I think.
> 
> >   
> > > -        if (slp_en && slp_typ == 5) {  
> >                                     ^^^
> > ditto  
> 
> this is deleted code, isn't it?
yep, sorry.
(a cup of coffee wasn't enough)
 
> > > -            qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> > > +        if (data & ACPI_GED_SLP_EN) {
> > > +            switch (extract8(data, 2, 3)) {
> > > +            case ACPI_GED_SLP_TYP_S3:
> > > +                if (s->slp_typs_bitmap & (1 << ACPI_GED_SLP_TYP_S3)) {  
> > 
> > why not use existing helpers like test_bit()
> > the same applies to following bit checks
> >   
> > > +                    qemu_system_suspend_request();
> > > +                }
> > > +                break;
> > > +            case ACPI_GED_SLP_TYP_S4:
> > > +                if (s->slp_typs_bitmap & (1 << ACPI_GED_SLP_TYP_S4)) {
> > > +                    qapi_event_send_suspend_disk();
> > > +                    qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> > > +                }
> > > +                break;
> > > +            case ACPI_GED_SLP_TYP_S5:
> > > +                if (s->slp_typs_bitmap & (1 << ACPI_GED_SLP_TYP_S5)) {
> > > +                    qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> > > +                }
> > > +                break;
> > > +            default:
> > > +                break;
> > > +            }
> > >          }
> > >          return;
> > >      case ACPI_GED_REG_SLEEP_STS:
> > > +        if (data & ACPI_GED_WAK_STS) {
> > > +            ged_st->sleep_sts &= ~ACPI_GED_WAK_STS;
> > > +        }
> > >          return;
> > >      case ACPI_GED_REG_RESET:
> > >          if (data == ACPI_GED_RESET_VALUE) {
> > > @@ -223,6 +253,14 @@ static const MemoryRegionOps ged_regs_ops = {
> > >      },
> > >  };
> > >  
> > > +static void acpi_ged_notify_wakeup(Notifier *notifier, void *data)
> > > +{
> > > +    GEDState *ged_st = container_of(notifier, GEDState, wakeup);
> > > +
> > > +    ged_st->sleep_sts |= ACPI_GED_WAK_STS;  
> > 
> > describe somewhere workflow how it is supposed to work 
> > (commit message or add ged specific doc in docs/specs/
> > as the 1st patch)
> >   
> > > +}
> > > +
> > > +
> > >  static void acpi_ged_device_plug_cb(HotplugHandler *hotplug_dev,
> > >                                      DeviceState *dev, Error **errp)
> > >  {
> > > @@ -305,6 +343,8 @@ static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
> > >  
> > >  static Property acpi_ged_properties[] = {
> > >      DEFINE_PROP_UINT32("ged-event", AcpiGedState, ged_event_bitmap, 0),
> > > +    DEFINE_PROP_UINT32("slp-typs", AcpiGedState, slp_typs_bitmap,
> > > +                        (1 << ACPI_GED_SLP_TYP_S5)),  
> > 
> > I'd default to everything enabled, and use compat mechanism
> > to disable it on older machine types.
> > 
> > You have to do this as ged is also used by versioned arm/virt machine
> >   
> > >      DEFINE_PROP_END_OF_LIST(),
> > >  };
> > >  
> > > @@ -320,10 +360,11 @@ static const VMStateDescription vmstate_memhp_state = {
> > >  
> > >  static const VMStateDescription vmstate_ged_state = {
> > >      .name = "acpi-ged-state",
> > > -    .version_id = 1,
> > > -    .minimum_version_id = 1,
> > > +    .version_id = 2,
> > > +    .minimum_version_id = 2,
> > >      .fields = (const VMStateField[]) {
> > >          VMSTATE_UINT32(sel, GEDState),
> > > +        VMSTATE_UINT8(sleep_sts, GEDState),
> > >          VMSTATE_END_OF_LIST()
> > >      }
> > >  };  
> > 
> > see for example
> > commit 829600a519386c7b188d5d813e78ba69bf0bd323
> >     hpet: recover timer offset correctly
> > 
> > 
> >   
> > > @@ -371,6 +412,18 @@ static const VMStateDescription vmstate_acpi_ged = {
> > >      }
> > >  };
> > >  
> > > +static void acpi_ged_realize(DeviceState *dev, Error **errp)
> > > +{
> > > +    AcpiGedState *s = ACPI_GED(dev);
> > > +    GEDState *ged_st = &s->ged_state;
> > > +
> > > +    if (s->slp_typs_bitmap & (1 << ACPI_GED_SLP_TYP_S3)) {
> > > +        ged_st->wakeup.notify = acpi_ged_notify_wakeup;
> > > +        qemu_register_wakeup_notifier(&ged_st->wakeup);
> > > +        qemu_register_wakeup_support();
> > > +    }
> > > +}
> > > +
> > >  static void acpi_ged_initfn(Object *obj)
> > >  {
> > >      DeviceState *dev = DEVICE(obj);
> > > @@ -409,6 +462,7 @@ static void acpi_ged_class_init(ObjectClass *class, void *data)
> > >      AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_CLASS(class);
> > >  
> > >      dc->desc = "ACPI Generic Event Device";
> > > +    dc->realize = acpi_ged_realize;  
> > 
> > realize was added recently (currently in master),
> > please rebase on top of current master tree
> >   
> > >      device_class_set_props(dc, acpi_ged_properties);
> > >      dc->vmsd = &vmstate_acpi_ged;
> > >  
> > > diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
> > > index ba84ce021477..1ea3cb848679 100644
> > > --- a/include/hw/acpi/generic_event_device.h
> > > +++ b/include/hw/acpi/generic_event_device.h
> > > @@ -80,9 +80,16 @@ OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
> > >  /* ACPI_GED_REG_RESET value for reset*/
> > >  #define ACPI_GED_RESET_VALUE       0x42
> > >  
> > > -/* ACPI_GED_REG_SLEEP_CTL.SLP_TYP value for S5 (aka poweroff) */
> > > +/* ACPI_GED_REG_SLEEP_CTL.SLP_EN bit */
> > > +#define ACPI_GED_SLP_EN            (1 << 5)
> > > +
> > > +/* ACPI_GED_REG_SLEEP_CTL.SLP_TYP values */
> > > +#define ACPI_GED_SLP_TYP_S3        0x03
> > > +#define ACPI_GED_SLP_TYP_S4        0x04
> > >  #define ACPI_GED_SLP_TYP_S5        0x05
> > >  
> > > +#define ACPI_GED_WAK_STS           (1 << 7)
> > > +
> > >  #define GED_DEVICE      "GED"
> > >  #define AML_GED_EVT_REG "EREG"
> > >  #define AML_GED_EVT_SEL "ESEL"
> > > @@ -99,7 +106,9 @@ OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
> > >  typedef struct GEDState {
> > >      MemoryRegion evt;
> > >      MemoryRegion regs;
> > > +    Notifier     wakeup;
> > >      uint32_t     sel;
> > > +    uint8_t      sleep_sts;
> > >  } GEDState;
> > >  
> > >  struct AcpiGedState {
> > > @@ -108,6 +117,7 @@ struct AcpiGedState {
> > >      MemoryRegion container_memhp;
> > >      GEDState ged_state;
> > >      uint32_t ged_event_bitmap;
> > > +    uint32_t slp_typs_bitmap;
> > >      qemu_irq irq;
> > >      AcpiGhesState ghes_state;
> > >  };
> > >   
> 

[PATCH 2/3] hw/loongarch/virt: Wire up S3 and S4 sleep

[Jiaxun Yang]

Wire up S3 and S4 sleep by setting relevant slp-typs bits for
GED and generate _S3 and _S4 methods in acpi table.

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 hw/loongarch/acpi-build.c | 18 ++++++++++++++++++
 hw/loongarch/virt.c       |  3 +++
 2 files changed, 21 insertions(+)

diff --git a/hw/loongarch/acpi-build.c b/hw/loongarch/acpi-build.c
index af45ce526d6e..2cb1696b579b 100644
--- a/hw/loongarch/acpi-build.c
+++ b/hw/loongarch/acpi-build.c
@@ -416,6 +416,24 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, MachineState *machine)
     acpi_dsdt_add_tpm(dsdt, lvms);
 #endif
     /* System State Package */
+    scope = aml_scope("\\");
+    pkg = aml_package(4);
+    aml_append(pkg, aml_int(ACPI_GED_SLP_TYP_S3));
+    aml_append(pkg, aml_int(0)); /* ignored */
+    aml_append(pkg, aml_int(0)); /* reserved */
+    aml_append(pkg, aml_int(0)); /* reserved */
+    aml_append(scope, aml_name_decl("_S3", pkg));
+    aml_append(dsdt, scope);
+
+    scope = aml_scope("\\");
+    pkg = aml_package(4);
+    aml_append(pkg, aml_int(ACPI_GED_SLP_TYP_S4));
+    aml_append(pkg, aml_int(0)); /* ignored */
+    aml_append(pkg, aml_int(0)); /* reserved */
+    aml_append(pkg, aml_int(0)); /* reserved */
+    aml_append(scope, aml_name_decl("_S4", pkg));
+    aml_append(dsdt, scope);
+
     scope = aml_scope("\\");
     pkg = aml_package(4);
     aml_append(pkg, aml_int(ACPI_GED_SLP_TYP_S5));
diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index 66cef201abe0..a4b55f00a32b 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -615,6 +615,9 @@ static DeviceState *create_acpi_ged(DeviceState *pch_pic,
     }
     dev = qdev_new(TYPE_ACPI_GED);
     qdev_prop_set_uint32(dev, "ged-event", event);
+    qdev_prop_set_uint32(dev, "slp-typs", (1 << ACPI_GED_SLP_TYP_S3) |
+                         (1 << ACPI_GED_SLP_TYP_S4) |
+                         (1 << ACPI_GED_SLP_TYP_S5));
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
 
     /* ged event */

-- 
2.43.0

Re: [PATCH 2/3] hw/loongarch/virt: Wire up S3 and S4 sleep

[Igor Mammedov]

On Thu, 13 Jun 2024 18:30:16 +0100
Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:

> Wire up S3 and S4 sleep by setting relevant slp-typs bits for
> GED and generate _S3 and _S4 methods in acpi table.
> 
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
>  hw/loongarch/acpi-build.c | 18 ++++++++++++++++++
>  hw/loongarch/virt.c       |  3 +++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/hw/loongarch/acpi-build.c b/hw/loongarch/acpi-build.c
> index af45ce526d6e..2cb1696b579b 100644
> --- a/hw/loongarch/acpi-build.c
> +++ b/hw/loongarch/acpi-build.c
> @@ -416,6 +416,24 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, MachineState *machine)
>      acpi_dsdt_add_tpm(dsdt, lvms);
>  #endif
>      /* System State Package */
> +    scope = aml_scope("\\");
> +    pkg = aml_package(4);
> +    aml_append(pkg, aml_int(ACPI_GED_SLP_TYP_S3));
> +    aml_append(pkg, aml_int(0)); /* ignored */
> +    aml_append(pkg, aml_int(0)); /* reserved */
> +    aml_append(pkg, aml_int(0)); /* reserved */
> +    aml_append(scope, aml_name_decl("_S3", pkg));
> +    aml_append(dsdt, scope);
> +
> +    scope = aml_scope("\\");
> +    pkg = aml_package(4);
> +    aml_append(pkg, aml_int(ACPI_GED_SLP_TYP_S4));
> +    aml_append(pkg, aml_int(0)); /* ignored */
> +    aml_append(pkg, aml_int(0)); /* reserved */
> +    aml_append(pkg, aml_int(0)); /* reserved */
> +    aml_append(scope, aml_name_decl("_S4", pkg));
> +    aml_append(dsdt, scope);

if it's the same in both patches, I'd suggest to move it
in helper function in hw/acpi/generic_event_device.c
and call that here.

>      scope = aml_scope("\\");
>      pkg = aml_package(4);
>      aml_append(pkg, aml_int(ACPI_GED_SLP_TYP_S5));
> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
> index 66cef201abe0..a4b55f00a32b 100644
> --- a/hw/loongarch/virt.c
> +++ b/hw/loongarch/virt.c
> @@ -615,6 +615,9 @@ static DeviceState *create_acpi_ged(DeviceState *pch_pic,
>      }
>      dev = qdev_new(TYPE_ACPI_GED);
>      qdev_prop_set_uint32(dev, "ged-event", event);
> +    qdev_prop_set_uint32(dev, "slp-typs", (1 << ACPI_GED_SLP_TYP_S3) |
> +                         (1 << ACPI_GED_SLP_TYP_S4) |
> +                         (1 << ACPI_GED_SLP_TYP_S5));

drop this, it should be default,
and backward compatibility for versioned machine types,
should be handled by compat machinery
 
>      sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>  
>      /* ged event */
> 

[PATCH 3/3] hw/i386/microvm: Wire up S3 and S4 sleep

[Jiaxun Yang]

Wire up S3 and S4 sleep by setting relevant slp-typs bits for
GED and generate _S3 and _S4 methods in acpi table.

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 hw/i386/acpi-microvm.c | 18 ++++++++++++++++++
 hw/i386/microvm.c      |  3 +++
 2 files changed, 21 insertions(+)

diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
index 279da6b4aa2f..7564de2b343b 100644
--- a/hw/i386/acpi-microvm.c
+++ b/hw/i386/acpi-microvm.c
@@ -129,6 +129,24 @@ build_dsdt_microvm(GArray *table_data, BIOSLinker *linker,
     aml_append(dsdt, sb_scope);
 
     /* ACPI 5.0: Table 7-209 System State Package */
+    scope = aml_scope("\\");
+    pkg = aml_package(4);
+    aml_append(pkg, aml_int(ACPI_GED_SLP_TYP_S3));
+    aml_append(pkg, aml_int(0)); /* ignored */
+    aml_append(pkg, aml_int(0)); /* reserved */
+    aml_append(pkg, aml_int(0)); /* reserved */
+    aml_append(scope, aml_name_decl("_S3", pkg));
+    aml_append(dsdt, scope);
+
+    scope = aml_scope("\\");
+    pkg = aml_package(4);
+    aml_append(pkg, aml_int(ACPI_GED_SLP_TYP_S4));
+    aml_append(pkg, aml_int(0)); /* ignored */
+    aml_append(pkg, aml_int(0)); /* reserved */
+    aml_append(pkg, aml_int(0)); /* reserved */
+    aml_append(scope, aml_name_decl("_S4", pkg));
+    aml_append(dsdt, scope);
+
     scope = aml_scope("\\");
     pkg = aml_package(4);
     aml_append(pkg, aml_int(ACPI_GED_SLP_TYP_S5));
diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
index fec63cacfa88..b9fb2d28e570 100644
--- a/hw/i386/microvm.c
+++ b/hw/i386/microvm.c
@@ -206,6 +206,9 @@ static void microvm_devices_init(MicrovmMachineState *mms)
     if (x86_machine_is_acpi_enabled(x86ms)) {
         DeviceState *dev = qdev_new(TYPE_ACPI_GED);
         qdev_prop_set_uint32(dev, "ged-event", ACPI_GED_PWR_DOWN_EVT);
+        qdev_prop_set_uint32(dev, "slp-typs", (1 << ACPI_GED_SLP_TYP_S3) |
+                             (1 << ACPI_GED_SLP_TYP_S4) |
+                             (1 << ACPI_GED_SLP_TYP_S5));
         sysbus_realize(SYS_BUS_DEVICE(dev), &error_fatal);
         sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, GED_MMIO_BASE);
         /* sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, GED_MMIO_BASE_MEMHP); */

-- 
2.43.0

Re: [PATCH 0/3] S3 and S4 sleep for loongarch/virt & microvm

[maobibo]

It is interesting.

How to wakeup VM if it sleeps in S3/S4, from emulated keyboard or 
ethernet magic packet or qemu monitor command in background?

Regards
Bibo Mao


On 2024/6/14 上午1:30, Jiaxun Yang wrote:
> Hi all,
> 
> This series implemented S3 and S4 sleep for loongarch virt machine
> and microvm.
> 
> For loongarch/virt a kernel patch is requried [1].
> 
> [1]: https://lore.kernel.org/loongarch/20240613-loongarch64-sleep-v1-0-a245232af5e4@flygoat.com/
> 
> Please review.
> Thanks
> 
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
> Jiaxun Yang (3):
>        acpi/ged: Implement S3 and S4 sleep
>        hw/loongarch/virt: Wire up S3 and S4 sleep
>        hw/i386/microvm: Wire up S3 and S4 sleep
> 
>   hw/acpi/generic_event_device.c         | 70 ++++++++++++++++++++++++++++++----
>   hw/i386/acpi-microvm.c                 | 18 +++++++++
>   hw/i386/microvm.c                      |  3 ++
>   hw/loongarch/acpi-build.c              | 18 +++++++++
>   hw/loongarch/virt.c                    |  3 ++
>   include/hw/acpi/generic_event_device.h | 12 +++++-
>   6 files changed, 115 insertions(+), 9 deletions(-)
> ---
> base-commit: f3e8cc47de2bc537d4991e883a85208e4e1c0f98
> change-id: 20240613-loongarch64-sleep-37b2466b8d76
> 
> Best regards,
> 

Re: [PATCH 0/3] S3 and S4 sleep for loongarch/virt & microvm

[Jiaxun Yang]

在2024年6月14日六月 上午4:32，maobibo写道：
> It is interesting.
>
> How to wakeup VM if it sleeps in S3/S4, from emulated keyboard or 
> ethernet magic packet or qemu monitor command in background?

Hi Bibo,

The best way to wake the guest is system_wakeup command in monitor.

Thanks
- Jiaxun

>
> Regards
> Bibo Mao
>
>
> On 2024/6/14 上午1:30, Jiaxun Yang wrote:
>> Hi all,
>> 
>> This series implemented S3 and S4 sleep for loongarch virt machine
>> and microvm.
>> 
>> For loongarch/virt a kernel patch is requried [1].
>> 
>> [1]: https://lore.kernel.org/loongarch/20240613-loongarch64-sleep-v1-0-a245232af5e4@flygoat.com/
>> 
>> Please review.
>> Thanks
>> 
>> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>> ---
>> Jiaxun Yang (3):
>>        acpi/ged: Implement S3 and S4 sleep
>>        hw/loongarch/virt: Wire up S3 and S4 sleep
>>        hw/i386/microvm: Wire up S3 and S4 sleep
>> 
>>   hw/acpi/generic_event_device.c         | 70 ++++++++++++++++++++++++++++++----
>>   hw/i386/acpi-microvm.c                 | 18 +++++++++
>>   hw/i386/microvm.c                      |  3 ++
>>   hw/loongarch/acpi-build.c              | 18 +++++++++
>>   hw/loongarch/virt.c                    |  3 ++
>>   include/hw/acpi/generic_event_device.h | 12 +++++-
>>   6 files changed, 115 insertions(+), 9 deletions(-)
>> ---
>> base-commit: f3e8cc47de2bc537d4991e883a85208e4e1c0f98
>> change-id: 20240613-loongarch64-sleep-37b2466b8d76
>> 
>> Best regards,
>>

-- 
- Jiaxun

Re: [PATCH 0/3] S3 and S4 sleep for loongarch/virt & microvm

[maobibo]

On 2024/6/14 下午12:27, Jiaxun Yang wrote:
> 
> 
> 在2024年6月14日六月 上午4:32，maobibo写道：
>> It is interesting.
>>
>> How to wakeup VM if it sleeps in S3/S4, from emulated keyboard or
>> ethernet magic packet or qemu monitor command in background?
> 
> Hi Bibo,
> 
> The best way to wake the guest is system_wakeup command in monitor.
Ok, I see.

It is useful and it can be used to test S3/S4 in TCG mode at least.

Can we add feature capability, enabled in TCG mode, disabled in KVM mode 
by default? If vm deploys in cloud, users in general help it is power-on 
always.

Regards
Bibo Mao
> 
> Thanks
> - Jiaxun
> 
>>
>> Regards
>> Bibo Mao
>>
>>
>> On 2024/6/14 上午1:30, Jiaxun Yang wrote:
>>> Hi all,
>>>
>>> This series implemented S3 and S4 sleep for loongarch virt machine
>>> and microvm.
>>>
>>> For loongarch/virt a kernel patch is requried [1].
>>>
>>> [1]: https://lore.kernel.org/loongarch/20240613-loongarch64-sleep-v1-0-a245232af5e4@flygoat.com/
>>>
>>> Please review.
>>> Thanks
>>>
>>> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>>> ---
>>> Jiaxun Yang (3):
>>>         acpi/ged: Implement S3 and S4 sleep
>>>         hw/loongarch/virt: Wire up S3 and S4 sleep
>>>         hw/i386/microvm: Wire up S3 and S4 sleep
>>>
>>>    hw/acpi/generic_event_device.c         | 70 ++++++++++++++++++++++++++++++----
>>>    hw/i386/acpi-microvm.c                 | 18 +++++++++
>>>    hw/i386/microvm.c                      |  3 ++
>>>    hw/loongarch/acpi-build.c              | 18 +++++++++
>>>    hw/loongarch/virt.c                    |  3 ++
>>>    include/hw/acpi/generic_event_device.h | 12 +++++-
>>>    6 files changed, 115 insertions(+), 9 deletions(-)
>>> ---
>>> base-commit: f3e8cc47de2bc537d4991e883a85208e4e1c0f98
>>> change-id: 20240613-loongarch64-sleep-37b2466b8d76
>>>
>>> Best regards,
>>>
> 

Re: [PATCH 0/3] S3 and S4 sleep for loongarch/virt & microvm

[Jiaxun Yang]

在2024年6月14日六月 上午6:17，maobibo写道：
> On 2024/6/14 下午12:27, Jiaxun Yang wrote:
>> 
>> 
>> 在2024年6月14日六月 上午4:32，maobibo写道：
>>> It is interesting.
>>>
>>> How to wakeup VM if it sleeps in S3/S4, from emulated keyboard or
>>> ethernet magic packet or qemu monitor command in background?
>> 
>> Hi Bibo,
>> 
>> The best way to wake the guest is system_wakeup command in monitor.
> Ok, I see.
>
> It is useful and it can be used to test S3/S4 in TCG mode at least.
>
> Can we add feature capability, enabled in TCG mode, disabled in KVM mode 
> by default? If vm deploys in cloud, users in general help it is power-on 
> always.

Well, I think we should align both machines as much as possible.

It's also enabled for KVM on x86 piix4 and q35 machines.

Thanks
>
> Regards
> Bibo Mao
-- 
- Jiaxun

Re: [PATCH 0/3] S3 and S4 sleep for loongarch/virt & microvm

[Daniel P. Berrangé]

On Fri, Jun 14, 2024 at 01:17:39PM +0800, maobibo wrote:
> 
> 
> On 2024/6/14 下午12:27, Jiaxun Yang wrote:
> > 
> > 
> > 在2024年6月14日六月 上午4:32，maobibo写道：
> > > It is interesting.
> > > 
> > > How to wakeup VM if it sleeps in S3/S4, from emulated keyboard or
> > > ethernet magic packet or qemu monitor command in background?
> > 
> > Hi Bibo,
> > 
> > The best way to wake the guest is system_wakeup command in monitor.
> Ok, I see.
> 
> It is useful and it can be used to test S3/S4 in TCG mode at least.
> 
> Can we add feature capability, enabled in TCG mode, disabled in KVM mode by
> default? If vm deploys in cloud, users in general help it is power-on
> always.

Please avoid creating differing defaults between TCG and KVM where
practical.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/3] S3 and S4 sleep for loongarch/virt & microvm

[maobibo]

On 2024/6/14 下午10:03, Daniel P. Berrangé wrote:
> On Fri, Jun 14, 2024 at 01:17:39PM +0800, maobibo wrote:
>>
>>
>> On 2024/6/14 下午12:27, Jiaxun Yang wrote:
>>>
>>>
>>> 在2024年6月14日六月 上午4:32，maobibo写道：
>>>> It is interesting.
>>>>
>>>> How to wakeup VM if it sleeps in S3/S4, from emulated keyboard or
>>>> ethernet magic packet or qemu monitor command in background?
>>>
>>> Hi Bibo,
>>>
>>> The best way to wake the guest is system_wakeup command in monitor.
>> Ok, I see.
>>
>> It is useful and it can be used to test S3/S4 in TCG mode at least.
>>
>> Can we add feature capability, enabled in TCG mode, disabled in KVM mode by
>> default? If vm deploys in cloud, users in general help it is power-on
>> always.
> 
> Please avoid creating differing defaults between TCG and KVM where
> practical.
There is bad experience for me, remote VM suddenly freezes and all 
network connection are lost if virt-machine does not support S3/S4 on 
LoongArch machines.

However it does not happen on x86 machine, how does x86 KVM VM machine 
stop this?

Regards
Bibo Mao
> 
> With regards,
> Daniel
> 

Re: [PATCH 0/3] S3 and S4 sleep for loongarch/virt & microvm

[Igor Mammedov]

On Sat, 15 Jun 2024 09:45:48 +0800
maobibo <maobibo@loongson.cn> wrote:

> On 2024/6/14 下午10:03, Daniel P. Berrangé wrote:
> > On Fri, Jun 14, 2024 at 01:17:39PM +0800, maobibo wrote:  
> >>
> >>
> >> On 2024/6/14 下午12:27, Jiaxun Yang wrote:  
> >>>
> >>>
> >>> 在2024年6月14日六月 上午4:32，maobibo写道：  
> >>>> It is interesting.
> >>>>
> >>>> How to wakeup VM if it sleeps in S3/S4, from emulated keyboard or
> >>>> ethernet magic packet or qemu monitor command in background?  
> >>>
> >>> Hi Bibo,
> >>>
> >>> The best way to wake the guest is system_wakeup command in monitor.  
> >> Ok, I see.
> >>
> >> It is useful and it can be used to test S3/S4 in TCG mode at least.
> >>
> >> Can we add feature capability, enabled in TCG mode, disabled in KVM mode by
> >> default? If vm deploys in cloud, users in general help it is power-on
> >> always.  
> > 
> > Please avoid creating differing defaults between TCG and KVM where
> > practical.  
> There is bad experience for me, remote VM suddenly freezes and all 
> network connection are lost if virt-machine does not support S3/S4 on 
> LoongArch machines.
> 
> However it does not happen on x86 machine, how does x86 KVM VM machine 
> stop this?

S3 support in linux was rather unreliable (that's the reason why RHEL
disables it downstream even for x86), it might be even more so for
LoongArch machines.

The best approach would be to find the actual reason why your guest
freezes in KVM mode. (it might be be a bug on qemu/kvm side or
a guest bug that needs fixing)

> Regards
> Bibo Mao
> > 
> > With regards,
> > Daniel
> >   
> 

Re: [PATCH 0/3] S3 and S4 sleep for loongarch/virt & microvm

[Igor Mammedov]

On Thu, 13 Jun 2024 18:30:14 +0100
Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:

> Hi all,
> 
> This series implemented S3 and S4 sleep for loongarch virt machine
> and microvm.
> 
> For loongarch/virt a kernel patch is requried [1].
> 
> [1]: https://lore.kernel.org/loongarch/20240613-loongarch64-sleep-v1-0-a245232af5e4@flygoat.com/

series also should include arm/virt patch to enable this
as it's another user of GED

> 
> Please review.
> Thanks
> 
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
> Jiaxun Yang (3):
>       acpi/ged: Implement S3 and S4 sleep
>       hw/loongarch/virt: Wire up S3 and S4 sleep
>       hw/i386/microvm: Wire up S3 and S4 sleep
> 
>  hw/acpi/generic_event_device.c         | 70 ++++++++++++++++++++++++++++++----
>  hw/i386/acpi-microvm.c                 | 18 +++++++++
>  hw/i386/microvm.c                      |  3 ++
>  hw/loongarch/acpi-build.c              | 18 +++++++++
>  hw/loongarch/virt.c                    |  3 ++
>  include/hw/acpi/generic_event_device.h | 12 +++++-
>  6 files changed, 115 insertions(+), 9 deletions(-)
> ---
> base-commit: f3e8cc47de2bc537d4991e883a85208e4e1c0f98
> change-id: 20240613-loongarch64-sleep-37b2466b8d76
> 
> Best regards,