diff mbox series

[PULL,04/21] acpi: ged: add control regs

Message ID 20200917135323.18022-5-kraxel@redhat.com
State Superseded
Headers show
Series Microvm 20200917 patches | expand

Commit Message

Gerd Hoffmann Sept. 17, 2020, 1:53 p.m. UTC
Add control regs (sleep, reset) for hw-reduced acpi.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Message-id: 20200915120909.20838-5-kraxel@redhat.com
---
 include/hw/acpi/generic_event_device.h | 12 +++++++
 hw/acpi/generic_event_device.c         | 44 ++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)

Comments

Peter Maydell June 24, 2021, 9:17 a.m. UTC | #1
On Thu, 17 Sept 2020 at 14:53, Gerd Hoffmann <kraxel@redhat.com> wrote:
>

> Add control regs (sleep, reset) for hw-reduced acpi.

>

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

> Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> Message-id: 20200915120909.20838-5-kraxel@redhat.com

> ---

>  include/hw/acpi/generic_event_device.h | 12 +++++++

>  hw/acpi/generic_event_device.c         | 44 ++++++++++++++++++++++++++

>  2 files changed, 56 insertions(+)


Hi; I've just run across this code because I found a bug in
a different device and was doing a grep to see if anybody
else had made the same mistake...

> +static void ged_regs_write(void *opaque, hwaddr addr, uint64_t data,

> +                           unsigned int size)

> +{

> +    bool slp_en;

> +    int slp_typ;

> +

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

> +        }

> +        return;

> +    case ACPI_GED_REG_SLEEP_STS:

> +        return;

> +    case ACPI_GED_REG_RESET:

> +        if (data == ACPI_GED_RESET_VALUE) {

> +            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);


Here we call qemu_system_reset_request(), but we pass it a cause
value of GUEST_SHUTDOWN. Is this trying to do a reset (in which
case it should probably be SHUTDOWN_CAUSE_GUEST_RESET) or a shutdown
(in which case it needs to call qemu_system_shutdown_request()) ?

> +        }

> +        return;

> +    }

> +}


thanks
-- PMM
Gerd Hoffmann June 24, 2021, 9:42 a.m. UTC | #2
Hi,

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

> > +        }

> > +        return;

> > +    case ACPI_GED_REG_SLEEP_STS:

> > +        return;

> > +    case ACPI_GED_REG_RESET:

> > +        if (data == ACPI_GED_RESET_VALUE) {

> > +            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);

> 

> Here we call qemu_system_reset_request(), but we pass it a cause

> value of GUEST_SHUTDOWN. Is this trying to do a reset (in which

> case it should probably be SHUTDOWN_CAUSE_GUEST_RESET) or a shutdown

> (in which case it needs to call qemu_system_shutdown_request()) ?


It is reset (shutdown is a few lines above and the cause was probably
just copy & pasted ...).

take care,
  Gerd
diff mbox series

Patch

diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
index 1be05a3c0f8c..38aec526f944 100644
--- a/include/hw/acpi/generic_event_device.h
+++ b/include/hw/acpi/generic_event_device.h
@@ -74,6 +74,17 @@  DECLARE_INSTANCE_CHECKER(AcpiGedState, ACPI_GED,
 #define ACPI_GED_EVT_SEL_OFFSET    0x0
 #define ACPI_GED_EVT_SEL_LEN       0x4
 
+#define ACPI_GED_REG_SLEEP_CTL     0x00
+#define ACPI_GED_REG_SLEEP_STS     0x01
+#define ACPI_GED_REG_RESET         0x02
+#define ACPI_GED_REG_COUNT         0x03
+
+/* 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) */
+#define ACPI_GED_SLP_TYP_S5        0x05
+
 #define GED_DEVICE      "GED"
 #define AML_GED_EVT_REG "EREG"
 #define AML_GED_EVT_SEL "ESEL"
@@ -89,6 +100,7 @@  DECLARE_INSTANCE_CHECKER(AcpiGedState, ACPI_GED,
 
 typedef struct GEDState {
     MemoryRegion evt;
+    MemoryRegion regs;
     uint32_t     sel;
 } GEDState;
 
diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index b8abdefa1c77..491df80a5cc7 100644
--- a/hw/acpi/generic_event_device.c
+++ b/hw/acpi/generic_event_device.c
@@ -20,6 +20,7 @@ 
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
 #include "qemu/error-report.h"
+#include "sysemu/runstate.h"
 
 static const uint32_t ged_supported_events[] = {
     ACPI_GED_MEM_HOTPLUG_EVT,
@@ -176,6 +177,45 @@  static const MemoryRegionOps ged_evt_ops = {
     },
 };
 
+static uint64_t ged_regs_read(void *opaque, hwaddr addr, unsigned size)
+{
+    return 0;
+}
+
+static void ged_regs_write(void *opaque, hwaddr addr, uint64_t data,
+                           unsigned int size)
+{
+    bool slp_en;
+    int slp_typ;
+
+    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);
+        }
+        return;
+    case ACPI_GED_REG_SLEEP_STS:
+        return;
+    case ACPI_GED_REG_RESET:
+        if (data == ACPI_GED_RESET_VALUE) {
+            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
+        }
+        return;
+    }
+}
+
+static const MemoryRegionOps ged_regs_ops = {
+    .read = ged_regs_read,
+    .write = ged_regs_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
 static void acpi_ged_device_plug_cb(HotplugHandler *hotplug_dev,
                                     DeviceState *dev, Error **errp)
 {
@@ -332,6 +372,10 @@  static void acpi_ged_initfn(Object *obj)
      sysbus_init_mmio(sbd, &s->container_memhp);
      acpi_memory_hotplug_init(&s->container_memhp, OBJECT(dev),
                               &s->memhp_state, 0);
+
+    memory_region_init_io(&ged_st->regs, obj, &ged_regs_ops, ged_st,
+                          TYPE_ACPI_GED "-regs", ACPI_GED_REG_COUNT);
+    sysbus_init_mmio(sbd, &ged_st->regs);
 }
 
 static void acpi_ged_class_init(ObjectClass *class, void *data)