[Qemu-devel] [PATCH for-2.4] s390x: diag288 reset fix

[Qemu-devel] [PATCH for-2.4] s390x: diag288 reset fix

[Cornelia Huck]

One more fix for s390x: The newly introduced diag288 watchdog driver
is not on the sysbus but bus-less (as feedback suggested). Unfortunately,
this also means we need to wire up any resets by hand.

Xu Wang (1):
  watchdog/diag288: correctly register for system reset requests

 hw/s390x/s390-virtio-ccw.c | 6 +++++-
 hw/watchdog/wdt_diag288.c  | 8 ++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

-- 
2.4.5

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

[Cornelia Huck]

From: Xu Wang <gesaint@linux.vnet.ibm.com>

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 <gesaint@linux.vnet.ibm.com>
Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 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);
     diag288->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, diag288_timer_expired,
                                   dev);
 }
-- 
2.4.5

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

[Christian Borntraeger]

Am 07.07.2015 um 10:53 schrieb Cornelia Huck:
> From: Xu Wang <gesaint@linux.vnet.ibm.com>
> 
> 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 <gesaint@linux.vnet.ibm.com>
> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>

kdump/kexec and reboot disable the watchdog.


Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>





> ---
>  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);
>      diag288->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, diag288_timer_expired,
>                                    dev);
>  }
> 

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

[Peter Crosthwaite]

On Tue, Jul 7, 2015 at 1:53 AM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> From: Xu Wang <gesaint@linux.vnet.ibm.com>
>
> 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 <gesaint@linux.vnet.ibm.com>
> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
>  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?

I.E. What string do you get with an object_get_canonical_path() of the
obj after machine init?

Regards,
Peter

>      diag288->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, diag288_timer_expired,
>                                    dev);
>  }
> --
> 2.4.5
>
>

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

[Christian Borntraeger]

Am 07.07.2015 um 12:51 schrieb Peter Crosthwaite:
>>      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?

Hmm, to me it seems that qemu_devices_reset does only work for devices
on a bus. This is a busless-device so the reset function gets not triggered.


> 
> I.E. What string do you get with an object_get_canonical_path() of the
> obj after machine init?


path /machine/peripheral/watchdog0

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

[Peter Crosthwaite]

On Tue, Jul 7, 2015 at 7:22 AM, Christian Borntraeger
<borntraeger@de.ibm.com> wrote:
> Am 07.07.2015 um 12:51 schrieb Peter Crosthwaite:
>>>      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?
>
> Hmm, to me it seems that qemu_devices_reset does only work for devices
> on a bus. This is a busless-device so the reset function gets not triggered.
>

Yes I see. I think it is a core code bug though and we want to avoid
having to patch individual devs based on their system level
connectivity. I'm looking at qbus_realize, and there, there is code to
register a reset for orphaned busses. So we have precedent for lazily
setting up a reset for an orphaned bus at realize time, just not for
indiv. devs. We can do the same.

I think this can be added to device_set_realized(). If a devices
parent is not a bus, then register its reset individually to catch-all
these. There are other devs that will qualify as well. I remember a
similar issue for NAND (hw/block/nand.c) where we lost it from the
qtree because we removed it from the sysbus. Looking at the code, I
fear NAND may be susceptible to this same missing-reset bug.

Regards,
Peter

>
>>
>> I.E. What string do you get with an object_get_canonical_path() of the
>> obj after machine init?
>
>
> path /machine/peripheral/watchdog0
>
>

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

[Cornelia Huck]

On Tue, 7 Jul 2015 14:11:18 -0700
Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:

> On Tue, Jul 7, 2015 at 7:22 AM, Christian Borntraeger
> <borntraeger@de.ibm.com> wrote:
> > Am 07.07.2015 um 12:51 schrieb Peter Crosthwaite:
> >>>      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?
> >
> > Hmm, to me it seems that qemu_devices_reset does only work for devices
> > on a bus. This is a busless-device so the reset function gets not triggered.
> >
> 
> Yes I see. I think it is a core code bug though and we want to avoid
> having to patch individual devs based on their system level
> connectivity. I'm looking at qbus_realize, and there, there is code to
> register a reset for orphaned busses. So we have precedent for lazily
> setting up a reset for an orphaned bus at realize time, just not for
> indiv. devs. We can do the same.
> 
> I think this can be added to device_set_realized(). If a devices
> parent is not a bus, then register its reset individually to catch-all
> these. 

Solving this in the core sounds good, but do you already have some kind
of patch ready? :) As we're pretty late in the cycle, it might make
sense to merge the existing fix for diag288 first, and switch to a
generic solution later on.

> There are other devs that will qualify as well. I remember a
> similar issue for NAND (hw/block/nand.c) where we lost it from the
> qtree because we removed it from the sysbus. Looking at the code, I
> fear NAND may be susceptible to this same missing-reset bug.

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

[Cornelia Huck]

On Wed, 8 Jul 2015 09:45:05 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Tue, 7 Jul 2015 14:11:18 -0700
> Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:

> > Yes I see. I think it is a core code bug though and we want to avoid
> > having to patch individual devs based on their system level
> > connectivity. I'm looking at qbus_realize, and there, there is code to
> > register a reset for orphaned busses. So we have precedent for lazily
> > setting up a reset for an orphaned bus at realize time, just not for
> > indiv. devs. We can do the same.
> > 
> > I think this can be added to device_set_realized(). If a devices
> > parent is not a bus, then register its reset individually to catch-all
> > these. 
> 
> Solving this in the core sounds good, but do you already have some kind
> of patch ready? :) As we're pretty late in the cycle, it might make
> sense to merge the existing fix for diag288 first, and switch to a
> generic solution later on.

OTOH, this is less code than I expected. With the following code, I see
the diag288 reset callback called on system reset. If this looks good,
I can resend as a proper patch; we can reduce Xu's patch to the
io_subsystem_reset() part in that case. Opinions?


diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index b2f404a..5c7c27b 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -1018,6 +1018,13 @@ static bool device_get_realized(Object *obj, Error **errp)
     return dev->realized;
 }
 
+static void do_device_reset(void *opaque)
+{
+    DeviceState *dev = opaque;
+
+    device_reset(dev);
+}
+
 static void device_set_realized(Object *obj, bool value, Error **errp)
 {
     DeviceState *dev = DEVICE(obj);
@@ -1061,6 +1068,11 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
             goto post_realize_fail;
         }
 
+        if (!dev->parent_bus) {
+            /* Make sure that reset is called for bus-less devices. */
+            qemu_register_reset(do_device_reset, dev);
+        }
+
         if (qdev_get_vmsd(dev)) {
             vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
                                            dev->instance_id_alias,
@@ -1094,6 +1106,9 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
         }
         dev->pending_deleted_event = true;
         DEVICE_LISTENER_CALL(unrealize, Reverse, dev);
+        if (!dev->parent_bus) {
+            qemu_unregister_reset(do_device_reset, dev);
+        }
     }
 
     if (local_err != NULL) {
-- 
2.4.5

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

[Cornelia Huck]

On Wed, 8 Jul 2015 12:31:40 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Wed, 8 Jul 2015 09:45:05 +0200
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> 
> > On Tue, 7 Jul 2015 14:11:18 -0700
> > Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> 
> > > Yes I see. I think it is a core code bug though and we want to avoid
> > > having to patch individual devs based on their system level
> > > connectivity. I'm looking at qbus_realize, and there, there is code to
> > > register a reset for orphaned busses. So we have precedent for lazily
> > > setting up a reset for an orphaned bus at realize time, just not for
> > > indiv. devs. We can do the same.
> > > 
> > > I think this can be added to device_set_realized(). If a devices
> > > parent is not a bus, then register its reset individually to catch-all
> > > these. 
> > 
> > Solving this in the core sounds good, but do you already have some kind
> > of patch ready? :) As we're pretty late in the cycle, it might make
> > sense to merge the existing fix for diag288 first, and switch to a
> > generic solution later on.
> 
> OTOH, this is less code than I expected. With the following code, I see
> the diag288 reset callback called on system reset. If this looks good,
> I can resend as a proper patch; we can reduce Xu's patch to the
> io_subsystem_reset() part in that case. Opinions?

Ping? Does this make sense?

> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index b2f404a..5c7c27b 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -1018,6 +1018,13 @@ static bool device_get_realized(Object *obj, Error **errp)
>      return dev->realized;
>  }
>  
> +static void do_device_reset(void *opaque)
> +{
> +    DeviceState *dev = opaque;
> +
> +    device_reset(dev);
> +}
> +
>  static void device_set_realized(Object *obj, bool value, Error **errp)
>  {
>      DeviceState *dev = DEVICE(obj);
> @@ -1061,6 +1068,11 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>              goto post_realize_fail;
>          }
>  
> +        if (!dev->parent_bus) {
> +            /* Make sure that reset is called for bus-less devices. */
> +            qemu_register_reset(do_device_reset, dev);
> +        }
> +
>          if (qdev_get_vmsd(dev)) {
>              vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
>                                             dev->instance_id_alias,
> @@ -1094,6 +1106,9 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>          }
>          dev->pending_deleted_event = true;
>          DEVICE_LISTENER_CALL(unrealize, Reverse, dev);
> +        if (!dev->parent_bus) {
> +            qemu_unregister_reset(do_device_reset, dev);
> +        }
>      }
>  
>      if (local_err != NULL) {

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

[Peter Crosthwaite]

On Thu, Jul 9, 2015 at 5:41 AM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> On Wed, 8 Jul 2015 12:31:40 +0200
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>
>> On Wed, 8 Jul 2015 09:45:05 +0200
>> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>>
>> > On Tue, 7 Jul 2015 14:11:18 -0700
>> > Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>>
>> > > Yes I see. I think it is a core code bug though and we want to avoid
>> > > having to patch individual devs based on their system level
>> > > connectivity. I'm looking at qbus_realize, and there, there is code to
>> > > register a reset for orphaned busses. So we have precedent for lazily
>> > > setting up a reset for an orphaned bus at realize time, just not for
>> > > indiv. devs. We can do the same.
>> > >
>> > > I think this can be added to device_set_realized(). If a devices
>> > > parent is not a bus, then register its reset individually to catch-all
>> > > these.
>> >
>> > Solving this in the core sounds good, but do you already have some kind
>> > of patch ready? :) As we're pretty late in the cycle, it might make
>> > sense to merge the existing fix for diag288 first, and switch to a
>> > generic solution later on.
>>
>> OTOH, this is less code than I expected. With the following code, I see
>> the diag288 reset callback called on system reset. If this looks good,
>> I can resend as a proper patch; we can reduce Xu's patch to the
>> io_subsystem_reset() part in that case. Opinions?
>

I'm for sending that new core patch, as I'm suspicious I can make it
fail in other places than your diag case. Runtime reset is poorly
exercised code so I think you are going to pickup half-a-dozen
bugfixes here.

Regards,
Peter

> Ping? Does this make sense?
>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index b2f404a..5c7c27b 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -1018,6 +1018,13 @@ static bool device_get_realized(Object *obj, Error **errp)
>>      return dev->realized;
>>  }
>>
>> +static void do_device_reset(void *opaque)
>> +{
>> +    DeviceState *dev = opaque;
>> +
>> +    device_reset(dev);
>> +}
>> +
>>  static void device_set_realized(Object *obj, bool value, Error **errp)
>>  {
>>      DeviceState *dev = DEVICE(obj);
>> @@ -1061,6 +1068,11 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>>              goto post_realize_fail;
>>          }
>>
>> +        if (!dev->parent_bus) {
>> +            /* Make sure that reset is called for bus-less devices. */
>> +            qemu_register_reset(do_device_reset, dev);
>> +        }
>> +
>>          if (qdev_get_vmsd(dev)) {
>>              vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
>>                                             dev->instance_id_alias,
>> @@ -1094,6 +1106,9 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>>          }
>>          dev->pending_deleted_event = true;
>>          DEVICE_LISTENER_CALL(unrealize, Reverse, dev);
>> +        if (!dev->parent_bus) {
>> +            qemu_unregister_reset(do_device_reset, dev);
>> +        }
>>      }
>>
>>      if (local_err != NULL) {
>
>

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

[Peter Maydell]

On 9 July 2015 at 17:16, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> On Thu, Jul 9, 2015 at 5:41 AM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>> On Wed, 8 Jul 2015 12:31:40 +0200
>> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>>> OTOH, this is less code than I expected. With the following code, I see
>>> the diag288 reset callback called on system reset. If this looks good,
>>> I can resend as a proper patch; we can reduce Xu's patch to the
>>> io_subsystem_reset() part in that case. Opinions?
>>
>
> I'm for sending that new core patch, as I'm suspicious I can make it
> fail in other places than your diag case. Runtime reset is poorly
> exercised code so I think you are going to pickup half-a-dozen
> bugfixes here.

FWIW this patch would mean we would try to call dc->reset on
the ARM CPU devices; the only reason this doesn't cause a
problem is that those devices don't happen to register a
dc->reset function pointer. (It's this sort of "now we start
calling a reset method on a device that was probably doing its
own reset via another path" that meant I wasn't too keen on it
for 2.4, though quite possibly it would work out better than
the ad-hoc reset we have currently in the longer term...)

-- PMM

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 <peter.crosthwaite@xilinx.com> wrote:

> On Tue, Jul 7, 2015 at 1:53 AM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> > From: Xu Wang <gesaint@linux.vnet.ibm.com>
> >
> > 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 <gesaint@linux.vnet.ibm.com>
> > Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > ---
> >  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.

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

[Paolo Bonzini]

On 07/07/2015 12:51, Peter Crosthwaite wrote:
> On Tue, Jul 7, 2015 at 1:53 AM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>> From: Xu Wang <gesaint@linux.vnet.ibm.com>
>>
>> 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 <gesaint@linux.vnet.ibm.com>
>> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
>> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>> ---
>>  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?

Reset doesn't follow the QOM tree.

In fact this is the main reason why I dislike TYPE_DEVICE as a superclass...

Paolo

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

[Christian Borntraeger]

Am 08.07.2015 um 09:54 schrieb Paolo Bonzini:
> 
> 
> On 07/07/2015 12:51, Peter Crosthwaite wrote:
>> On Tue, Jul 7, 2015 at 1:53 AM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>>> From: Xu Wang <gesaint@linux.vnet.ibm.com>
>>>
>>> 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 <gesaint@linux.vnet.ibm.com>
>>> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
>>> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>>> ---
>>>  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?
> 
> Reset doesn't follow the QOM tree.
> 
> In fact this is the main reason why I dislike TYPE_DEVICE as a superclass...

Oh, thats why we were asked to use that instead of sysbus device? ;-)

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

[Paolo Bonzini]

On 08/07/2015 10:01, Christian Borntraeger wrote:
> > > 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?
> > 
> > Reset doesn't follow the QOM tree.
> > 
> > In fact this is the main reason why I dislike TYPE_DEVICE as a superclass...
>
> Oh, thats why we were asked to use that instead of sysbus device? ;-)

Did I?  I've always liked sysbus... :)

Paolo

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

[Christian Borntraeger]

Am 08.07.2015 um 10:44 schrieb Paolo Bonzini:
> 
> 
> On 08/07/2015 10:01, Christian Borntraeger wrote:
>>>> 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?
>>>
>>> Reset doesn't follow the QOM tree.
>>>
>>> In fact this is the main reason why I dislike TYPE_DEVICE as a superclass...
>>
>> Oh, thats why we were asked to use that instead of sysbus device? ;-)
> 
> Did I?  I've always liked sysbus... :)

Not you.
But its a pattern that I have seen 3 or 4 times: "Oh no, please do not
use xxx any more, we have fancy new stuff that nobody else is using in the tree,
but its the way to go..." But  most of the time the hickups can be handled. :-)

Christian

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

[Paolo Bonzini]

On 08/07/2015 10:48, Christian Borntraeger wrote:
> > > In fact this is the main reason why I dislike TYPE_DEVICE as a superclass...
> > >
> > > Oh, thats why we were asked to use that instead of sysbus device? ;-)
> > 
> > Did I?  I've always liked sysbus... :)
>
> Not you.
> But its a pattern that I have seen 3 or 4 times: "Oh no, please do not
> use xxx any more, we have fancy new stuff that nobody else is using in the tree,
> but its the way to go..." But  most of the time the hickups can be handled. :-)

I know, and it's a problem. :(

Paolo

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

[Andreas Färber]

Am 07.07.2015 um 10:53 schrieb Cornelia Huck:
> From: Xu Wang <gesaint@linux.vnet.ibm.com>
> 
> 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 <gesaint@linux.vnet.ibm.com>
> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Reviewed-by: Andreas Färber <afaerber@suse.de>

Thanks,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)