Re: [PATCH v5 04/20] acpi: ged: add control regs

From: Igor Mammedov <imammedo@redhat.com>
To: Gerd Hoffmann <kraxel@redhat.com>
Cc: Laurent Vivier <lvivier@redhat.com>,
	Peter Maydell <peter.maydell@linaro.org>,
	Thomas Huth <thuth@redhat.com>,
	Eduardo Habkost <ehabkost@redhat.com>,
	Sergio Lopez <slp@redhat.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	qemu-devel@nongnu.org, Shannon Zhao <shannon.zhaosl@gmail.com>,
	qemu-arm@nongnu.org, Paolo Bonzini <pbonzini@redhat.com>,
	Richard Henderson <rth@twiddle.net>
Subject: Re: [PATCH v5 04/20] acpi: ged: add control regs
Date: Fri, 10 Jul 2020 21:07:49 +0200	[thread overview]
Message-ID: <20200710210749.1da2e663@redhat.com> (raw)
In-Reply-To: <20200707125356.32450-5-kraxel@redhat.com>

On Tue,  7 Jul 2020 14:53:40 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

> Add control regs (sleep, reset) for hw-reduced acpi.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

with below comments addressed:

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

> ---
>  include/hw/acpi/generic_event_device.h |  7 ++++
>  hw/acpi/generic_event_device.c         | 44 ++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+)
> 
> diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
> index 90a9180db572..474c92198080 100644
> --- a/include/hw/acpi/generic_event_device.h
> +++ b/include/hw/acpi/generic_event_device.h
> @@ -72,6 +72,12 @@
>  #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_RESET_VALUE     0x42
          ^^ too many ' '

also it would be nice to point out where vaule comes from
(if it's from spec then ref to spec pls)

> +#define ACPI_GED_REG_COUNT         0x03
> +
>  #define GED_DEVICE      "GED"
>  #define AML_GED_EVT_REG "EREG"
>  #define AML_GED_EVT_SEL "ESEL"
> @@ -87,6 +93,7 @@
>  
>  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) {

replace magic 5 with something more descriptive and use it also in 8/20
during initializing _S5 package

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