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

[Cornelia Huck]

On Wed, 29 Apr 2015 14:43:56 +0200
Markus Armbruster <address@hidden> wrote:

> 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/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.