Re: [Qemu-devel] [PATCH for-2.4] watchdog/diag288: correctly register for system reset requests

[Cornelia Huck]

On Tue, 7 Jul 2015 03:51:59 -0700
Peter Crosthwaite <address@hidden> wrote:

> On Tue, Jul 7, 2015 at 1:53 AM, Cornelia Huck <address@hidden> wrote:
> > From: Xu Wang <address@hidden>
> >
> > The diag288 watchdog is no sysbus device, therefore it doesn't get
> > triggered on resets automatically using dc->reset.
> >
> > Let's register the reset handler manually, so we get correctly notified
> > again when a system reset was requested. Also reset the watchdog on
> > subsystem resets that don't trigger a full system reset.
> >
> > Signed-off-by: Xu Wang <address@hidden>
> > Reviewed-by: David Hildenbrand <address@hidden>
> > Signed-off-by: Cornelia Huck <address@hidden>
> > ---
> >  hw/s390x/s390-virtio-ccw.c | 6 +++++-
> >  hw/watchdog/wdt_diag288.c  | 8 ++++++++
> >  2 files changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> > index 3d20d6a..4c51d1a 100644
> > --- a/hw/s390x/s390-virtio-ccw.c
> > +++ b/hw/s390x/s390-virtio-ccw.c
> > @@ -36,7 +36,7 @@ typedef struct S390CcwMachineState {
> >
> >  void io_subsystem_reset(void)
> >  {
> > -    DeviceState *css, *sclp, *flic;
> > +    DeviceState *css, *sclp, *flic, *diag288;
> >
> >      css = DEVICE(object_resolve_path_type("", "virtual-css-bridge", NULL));
> >      if (css) {
> > @@ -51,6 +51,10 @@ void io_subsystem_reset(void)
> >      if (flic) {
> >          qdev_reset_all(flic);
> >      }
> > +    diag288 = DEVICE(object_resolve_path_type("", "diag288", NULL));
> > +    if (diag288) {
> > +        qdev_reset_all(diag288);
> > +    }
> >  }
> >
> >  static int virtio_ccw_hcall_notify(const uint64_t *args)
> > diff --git a/hw/watchdog/wdt_diag288.c b/hw/watchdog/wdt_diag288.c
> > index 1185e06..2a885a4 100644
> > --- a/hw/watchdog/wdt_diag288.c
> > +++ b/hw/watchdog/wdt_diag288.c
> > @@ -40,6 +40,13 @@ static void wdt_diag288_reset(DeviceState *dev)
> >      timer_del(diag288->timer);
> >  }
> >
> > +static void diag288_reset(void *opaque)
> > +{
> > +    DeviceState *diag288 = opaque;
> > +
> > +    wdt_diag288_reset(diag288);
> > +}
> > +
> >  static void diag288_timer_expired(void *dev)
> >  {
> >      qemu_log_mask(CPU_LOG_RESET, "Watchdog timer expired.\n");
> > @@ -80,6 +87,7 @@ static void wdt_diag288_realize(DeviceState *dev, Error 
> > **errp)
> >  {
> >      DIAG288State *diag288 = DIAG288(dev);
> >
> > +    qemu_register_reset(diag288_reset, diag288);
> 
> Doesn't seem right. Even if it is not a SBD it should still sit in the
> QOM tree in a place where the reset is reached. Where is this device
> in the QOM tree?

It will show up as /machine/peripheral-anon/device[].

But is just showing up really enough? For the sysbus, qbus_reset_all_fn
is registered as a reset handler, and that called our reset handler
when diag288 was still a sysbus device in a previous patch version.