| Message ID | 1429257161-29597-3-git-send-email-cornelia.huck@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
Cornelia Huck <cornelia.huck@de.ibm.com> writes: > From: Xu Wang <gesaint@linux.vnet.ibm.com> > > 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 <gesaint@linux.vnet.ibm.com> > Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > --- > 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 <gesaint@linux.vnet.ibm.com> > + * > + * 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.
On Wed, 29 Apr 2015 14:43:56 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Cornelia Huck <cornelia.huck@de.ibm.com> writes: > > > From: Xu Wang <gesaint@linux.vnet.ibm.com> > > > > 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 <gesaint@linux.vnet.ibm.com> > > Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com> > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > > --- > > 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/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. Maybe keep "Choose a watchdog for which your guest has drivers."? > > 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 I like that. > > > @@ -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. Hm... let's make that @item diag288 A virtual watchdog for s390x backed by the diagnose 288 hypercall (currently KVM only). ? > > > > 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. So we end up with three patches: - clean up help text - add device (probably as a plain device) - wire up -watchdog Sounds sensible.
On Fri, 17 Apr 2015 09:52:37 +0200 Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > From: Xu Wang <gesaint@linux.vnet.ibm.com> > > 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 <gesaint@linux.vnet.ibm.com> > Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > --- > 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 OK, after some deliberation, I think it is best to model the diag288 watchdog as a bus-less device: The other watchdogs are modelled as devices as well, and it is probably a good idea to stay with this model as other parties may have made assumptions based on this. Xu, can you please switch the diag288 device to TYPE_DEVICE and address Markus' comments regarding the help text as well?
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 <gesaint@linux.vnet.ibm.com> + * + * 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 @@ -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.