| Message ID | 20200917135323.18022-5-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/21] microvm: name qboot binary qboot.rom | expand |
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
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 --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)