Re: [Qemu-devel] [PATCH 2/6] watchdog: Add watchdog device diag288 to the sysbus

[Markus Armbruster]

Cornelia Huck <address@hidden> writes:

> From: Xu Wang <address@hidden>
>
> A new sysbus device for diag288 watchdog, it monitors the kvm guest
> status, and take corresponding actions when it detects that the guest
> is not responding.
>
> Signed-off-by: Xu Wang <address@hidden>
> Reviewed-by: David Hildenbrand <address@hidden>
> Signed-off-by: Cornelia Huck <address@hidden>
> ---
>  default-configs/s390x-softmmu.mak |   1 +
>  hw/watchdog/Makefile.objs         |   1 +
>  hw/watchdog/wdt_diag288.c         | 110 
> ++++++++++++++++++++++++++++++++++++++
>  include/hw/watchdog/wdt_diag288.h |  36 +++++++++++++
>  qemu-options.hx                   |   6 ++-
>  5 files changed, 152 insertions(+), 2 deletions(-)
>  create mode 100644 hw/watchdog/wdt_diag288.c
>  create mode 100644 include/hw/watchdog/wdt_diag288.h
>
> diff --git a/default-configs/s390x-softmmu.mak 
> b/default-configs/s390x-softmmu.mak
> index f9e13f1..36e15de 100644
> --- a/default-configs/s390x-softmmu.mak
> +++ b/default-configs/s390x-softmmu.mak
> @@ -4,3 +4,4 @@ CONFIG_VIRTIO=y
>  CONFIG_SCLPCONSOLE=y
>  CONFIG_S390_FLIC=y
>  CONFIG_S390_FLIC_KVM=$(CONFIG_KVM)
> +CONFIG_WDT_DIAG288=y
> diff --git a/hw/watchdog/Makefile.objs b/hw/watchdog/Makefile.objs
> index 4b0374a..72e3ffd 100644
> --- a/hw/watchdog/Makefile.objs
> +++ b/hw/watchdog/Makefile.objs
> @@ -1,3 +1,4 @@
>  common-obj-y += watchdog.o
>  common-obj-$(CONFIG_WDT_IB6300ESB) += wdt_i6300esb.o
>  common-obj-$(CONFIG_WDT_IB700) += wdt_ib700.o
> +common-obj-$(CONFIG_WDT_DIAG288) += wdt_diag288.o
> diff --git a/hw/watchdog/wdt_diag288.c b/hw/watchdog/wdt_diag288.c
> new file mode 100644
> index 0000000..81a0e30
> --- /dev/null
> +++ b/hw/watchdog/wdt_diag288.c
> @@ -0,0 +1,110 @@
> +/*
> + * watchdog device diag288 support
> + *
> + * Copyright IBM, Corp. 2015
> + *
> + * Authors:
> + *  Xu Wang <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at 
> your
> + * option) any later version.  See the COPYING file in the top-level 
> directory.
> + *
> + */
> +
> +#include "sysemu/watchdog.h"
> +#include "hw/sysbus.h"
> +#include "qemu/timer.h"
> +#include "hw/watchdog/wdt_diag288.h"
> +
> +static WatchdogTimerModel model = {
> +    .wdt_name = TYPE_WDT_DIAG288,
> +    .wdt_description = "diag288 device for s390x platform",
> +};
> +
> +static void wdt_diag288_reset(DeviceState *dev)
> +{
> +    DIAG288State *diag288 = DIAG288(dev);
> +
> +    diag288->enabled = false;
> +    timer_del(diag288->timer);
> +}
> +
> +static void diag288_timer_expired(void *dev)
> +{
> +    qemu_log_mask(CPU_LOG_RESET, "Watchdog timer expired.\n");
> +    watchdog_perform_action();
> +    wdt_diag288_reset(dev);
> +}
> +
> +static int wdt_diag288_handle_timer(DIAG288State *diag288,
> +                                     uint64_t func, uint64_t timeout)
> +{
> +    switch (func) {
> +    case WDT_DIAG288_INIT:
> +        diag288->enabled = true;
> +        /* fall through */
> +    case WDT_DIAG288_CHANGE:
> +        if (!diag288->enabled) {
> +            return -1;
> +        }
> +        timer_mod(diag288->timer,
> +                  qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> +                  timeout * get_ticks_per_sec());
> +        break;
> +    case WDT_DIAG288_CANCEL:
> +        if (!diag288->enabled) {
> +            return -1;
> +        }
> +        diag288->enabled = false;
> +        timer_del(diag288->timer);
> +        break;
> +    default:
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +static void wdt_diag288_realize(DeviceState *dev, Error **errp)
> +{
> +    DIAG288State *diag288 = DIAG288(dev);
> +
> +    diag288->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, diag288_timer_expired,
> +                                  dev);
> +}
> +
> +static void wdt_diag288_unrealize(DeviceState *dev, Error **errp)
> +{
> +    DIAG288State *diag288 = DIAG288(dev);
> +
> +    timer_del(diag288->timer);
> +    timer_free(diag288->timer);
> +}
> +
> +static void wdt_diag288_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    DIAG288Class *diag288 = DIAG288_CLASS(klass);
> +
> +    dc->realize = wdt_diag288_realize;
> +    dc->unrealize = wdt_diag288_unrealize;
> +    dc->reset = wdt_diag288_reset;
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +    diag288->handle_timer = wdt_diag288_handle_timer;
> +}
> +
> +static const TypeInfo wdt_diag288_info = {
> +    .class_init = wdt_diag288_class_init,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .name  = TYPE_WDT_DIAG288,
> +    .instance_size  = sizeof(DIAG288State),
> +    .class_size = sizeof(DIAG288Class),
> +};
> +
> +static void wdt_diag288_register_types(void)
> +{
> +    watchdog_add_model(&model);
> +    type_register_static(&wdt_diag288_info);
> +}
> +
> +type_init(wdt_diag288_register_types)
> diff --git a/include/hw/watchdog/wdt_diag288.h 
> b/include/hw/watchdog/wdt_diag288.h
> new file mode 100644
> index 0000000..3d076b0
> --- /dev/null
> +++ b/include/hw/watchdog/wdt_diag288.h
> @@ -0,0 +1,36 @@
> +#ifndef WDT_DIAG288_H
> +#define WDT_DIAG288_H
> +
> +#include "hw/sysbus.h"
> +
> +#define TYPE_WDT_DIAG288 "diag288"
> +#define DIAG288(obj) \
> +    OBJECT_CHECK(DIAG288State, (obj), TYPE_WDT_DIAG288)
> +#define DIAG288_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(DIAG288Class, (klass), TYPE_WDT_DIAG288)
> +#define DIAG288_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(DIAG288Class, (obj), TYPE_WDT_DIAG288)
> +
> +#define WDT_DIAG288_INIT      0
> +#define WDT_DIAG288_CHANGE    1
> +#define WDT_DIAG288_CANCEL    2
> +
> +typedef struct DIAG288State {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +    QEMUTimer *timer;
> +    bool enabled;
> +
> +    /*< public >*/
> +} DIAG288State;
> +
> +typedef struct DIAG288Class {
> +    /*< private >*/
> +    SysBusDeviceClass parent_class;
> +
> +    /*< public >*/
> +    int (*handle_timer)(DIAG288State *dev,
> +                        uint64_t func, uint64_t timeout);
> +} DIAG288Class;
> +
> +#endif  /* WDT_DIAG288_H */
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 319d971..7ef8f50 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3115,7 +3115,7 @@ when the shift value is high (how high depends on the 
> host machine).
>  ETEXI
>  
>  DEF("watchdog", HAS_ARG, QEMU_OPTION_watchdog, \
> -    "-watchdog i6300esb|ib700\n" \
> +    "-watchdog i6300esb|ib700|diag288\n" \
>      "                enable virtual hardware watchdog [default=none]\n",
>      QEMU_ARCH_ALL)
>  STEXI

Preexisting: the help text assumes that all these watchdogs are actually
compiled in.  Generally not the case.  Instead of adding another one
that's generally not available, please change the help text in a
preliminary patch to something like

DEF("watchdog", HAS_ARG, QEMU_OPTION_watchdog, \
    "-watchdog model\n" \
    "                enable virtual hardware watchdog [default none]\n",
    QEMU_ARCH_ALL)
STEXI
@item -watchdog @var{model}
@findex -watchdog
Create a virtual hardware watchdog device.  Once enabled (by a guest
action), the watchdog must be periodically polled by an agent inside
the guest or else the guest will be restarted.

The @var{model} is the model of hardware watchdog to emulate.  Use
@code{-watchdog help} to list available hardware models.  Only one
watchdog can be enabled for a guest.

The following models may be available:
@table @option
@item ib700
iBASE 700 is a very simple ISA watchdog with a single timer.
@item i6300esb
Intel 6300ESB I/O controller hub is a much more featureful PCI-based
dual-timer watchdog.
@end table
ETEXI

> @@ -3129,7 +3129,9 @@ The @var{model} is the model of hardware watchdog to 
> emulate.  Choices
>  for model are: @code{ib700} (iBASE 700) which is a very simple ISA
>  watchdog with a single timer, or @code{i6300esb} (Intel 6300ESB I/O
>  controller hub) which is a much more featureful PCI-based dual-timer
> -watchdog.  Choose a model for which your guest has drivers.
> +watchdog. @code{diag288} is a watchdog device only available on s390x
> +(for now only supported by KVM). Choose a model for which your guest
> +has drivers.
>  
>  Use @code{-watchdog help} to list available hardware models.  Only one
>  watchdog can be enabled for a guest.

Recommend to split this patch: one patch to add the device model, and
another one to wire up -watchdog.