[v1] Misc: Make watchdog devices using qemu_system_reset_request() use watchdog_perfom_action()

[PATCH 1/3] misc: m48t59: replace qemu_system_reset_request() call with watchdog_perform_action()

Posted by Abhiram Tilak 8 months, 2 weeks ago

A few watchdog devices use qemu_system_reset_request(). This is not ideal since
behaviour of watchdog-expiry can't be changed by QMP using `watchdog_action`.
As stated in BiteSizedTasks wiki page, instead of using qemu_system_reset_request()
to reset when a watchdog timer expires, let watchdog_perform_action() decide
what to do.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2124
Signed-off-by: Abhiram Tilak <atp.exp@gmail.com>
---
 hw/rtc/m48t59.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/rtc/m48t59.c b/hw/rtc/m48t59.c
index aa44c4b20c..ebda084478 100644
--- a/hw/rtc/m48t59.c
+++ b/hw/rtc/m48t59.c
@@ -36,6 +36,7 @@
 #include "qemu/bcd.h"
 #include "qemu/module.h"
 #include "trace.h"
+#include "sysemu/watchdog.h"
 
 #include "m48t59-internal.h"
 #include "migration/vmstate.h"
@@ -163,8 +164,7 @@ static void watchdog_cb (void *opaque)
     if (NVRAM->buffer[0x1FF7] & 0x80) {
         NVRAM->buffer[0x1FF7] = 0x00;
         NVRAM->buffer[0x1FFC] &= ~0x40;
-        /* May it be a hw CPU Reset instead ? */
-        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+        watchdog_perform_action(); /* watchdog-expired action */
     } else {
         qemu_set_irq(NVRAM->IRQ, 1);
         qemu_set_irq(NVRAM->IRQ, 0);
-- 
2.42.1

Re: [PATCH 1/3] misc: m48t59: replace qemu_system_reset_request() call with watchdog_perform_action()

Posted by Peter Maydell 8 months, 2 weeks ago

On Fri, 16 Feb 2024 at 13:56, Abhiram Tilak <atp.exp@gmail.com> wrote:
>
> A few watchdog devices use qemu_system_reset_request(). This is not ideal since
> behaviour of watchdog-expiry can't be changed by QMP using `watchdog_action`.
> As stated in BiteSizedTasks wiki page, instead of using qemu_system_reset_request()
> to reset when a watchdog timer expires, let watchdog_perform_action() decide
> what to do.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2124
> Signed-off-by: Abhiram Tilak <atp.exp@gmail.com>
> ---
>  hw/rtc/m48t59.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/rtc/m48t59.c b/hw/rtc/m48t59.c
> index aa44c4b20c..ebda084478 100644
> --- a/hw/rtc/m48t59.c
> +++ b/hw/rtc/m48t59.c
> @@ -36,6 +36,7 @@
>  #include "qemu/bcd.h"
>  #include "qemu/module.h"
>  #include "trace.h"
> +#include "sysemu/watchdog.h"
>
>  #include "m48t59-internal.h"
>  #include "migration/vmstate.h"
> @@ -163,8 +164,7 @@ static void watchdog_cb (void *opaque)
>      if (NVRAM->buffer[0x1FF7] & 0x80) {
>          NVRAM->buffer[0x1FF7] = 0x00;
>          NVRAM->buffer[0x1FFC] &= ~0x40;
> -        /* May it be a hw CPU Reset instead ? */
> -        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +        watchdog_perform_action(); /* watchdog-expired action */
>      } else {

I don't think we need the comment, it's clear enough from the
name of the function.

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 1/3] misc: m48t59: replace qemu_system_reset_request() call with watchdog_perform_action()

Posted by atp exp 8 months, 2 weeks ago

I agree, comment here is redundant, i will fix
it in the next patch.

Abhiram

On Fri, 16 Feb 2024 at 20:19, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Fri, 16 Feb 2024 at 13:56, Abhiram Tilak <atp.exp@gmail.com> wrote:
> >
> > A few watchdog devices use qemu_system_reset_request(). This is not
> ideal since
> > behaviour of watchdog-expiry can't be changed by QMP using
> `watchdog_action`.
> > As stated in BiteSizedTasks wiki page, instead of using
> qemu_system_reset_request()
> > to reset when a watchdog timer expires, let watchdog_perform_action()
> decide
> > what to do.
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2124
> > Signed-off-by: Abhiram Tilak <atp.exp@gmail.com>
> > ---
> >  hw/rtc/m48t59.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/rtc/m48t59.c b/hw/rtc/m48t59.c
> > index aa44c4b20c..ebda084478 100644
> > --- a/hw/rtc/m48t59.c
> > +++ b/hw/rtc/m48t59.c
> > @@ -36,6 +36,7 @@
> >  #include "qemu/bcd.h"
> >  #include "qemu/module.h"
> >  #include "trace.h"
> > +#include "sysemu/watchdog.h"
> >
> >  #include "m48t59-internal.h"
> >  #include "migration/vmstate.h"
> > @@ -163,8 +164,7 @@ static void watchdog_cb (void *opaque)
> >      if (NVRAM->buffer[0x1FF7] & 0x80) {
> >          NVRAM->buffer[0x1FF7] = 0x00;
> >          NVRAM->buffer[0x1FFC] &= ~0x40;
> > -        /* May it be a hw CPU Reset instead ? */
> > -        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> > +        watchdog_perform_action(); /* watchdog-expired action */
> >      } else {
>
> I don't think we need the comment, it's clear enough from the
> name of the function.
>
> Otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> thanks
> -- PMM
>

[PATCH 1/3] misc: m48t59: replace qemu_system_reset_request() call with watchdog_perform_action()
[PATCH 2/3] misc: pxa2xx_timer: replace qemu_system_reset_request() call with watchdog_perform_action()
[PATCH 3/3] misc: ppc/spapr: replace qemu_system_reset_request() calls with watchdog_perform_action()