[PATCH 0/3] Misc: Make watchdog devices using qemu_system_reset_request() use watchdog_perfom_action()

[PATCH 0/3] Misc: Make watchdog devices using qemu_system_reset_request() use watchdog_perfom_action()

[Abhiram Tilak]

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.

I am unsure about the changes in `spapr_watchdog.c` in patch 3, it would be great
if any of the maintainers review it.

Abhiram Tilak (3):
  misc: m48t59: replace qemu_system_reset_request() call with
    watchdog_perform_action()
  misc: pxa2xx_timer: replace qemu_system_reset_request() call with
    watchdog_perform_action()
  misc: ppc/spapr: replace qemu_system_reset_request() calls with
    watchdog_perform_action()

 hw/rtc/m48t59.c              | 4 ++--
 hw/timer/pxa2xx_timer.c      | 3 ++-
 hw/watchdog/spapr_watchdog.c | 3 ++-
 3 files changed, 6 insertions(+), 4 deletions(-)

--
2.42.1

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

[Abhiram Tilak]

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()

[Peter Maydell]

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()

[atp exp]

[-- Attachment #1: Type: text/plain, Size: 1753 bytes --]

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
>

[-- Attachment #2: Type: text/html, Size: 2704 bytes --]

[PATCH 2/3] misc: pxa2xx_timer: replace qemu_system_reset_request() call with watchdog_perform_action()

[Abhiram Tilak]

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/timer/pxa2xx_timer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/timer/pxa2xx_timer.c b/hw/timer/pxa2xx_timer.c
index 6a7d5551f4..6479ab1a8b 100644
--- a/hw/timer/pxa2xx_timer.c
+++ b/hw/timer/pxa2xx_timer.c
@@ -18,6 +18,7 @@
 #include "qemu/log.h"
 #include "qemu/module.h"
 #include "qom/object.h"
+#include "sysemu/watchdog.h"
 
 #define OSMR0	0x00
 #define OSMR1	0x04
@@ -417,7 +418,7 @@ static void pxa2xx_timer_tick(void *opaque)
     if (t->num == 3)
         if (i->reset3 & 1) {
             i->reset3 = 0;
-            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+            watchdog_perform_action();
         }
 }
 
-- 
2.42.1

Re: [PATCH 2/3] misc: pxa2xx_timer: replace qemu_system_reset_request() call with watchdog_perform_action()

[Peter Maydell]

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/timer/pxa2xx_timer.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/timer/pxa2xx_timer.c b/hw/timer/pxa2xx_timer.c
> index 6a7d5551f4..6479ab1a8b 100644
> --- a/hw/timer/pxa2xx_timer.c
> +++ b/hw/timer/pxa2xx_timer.c
> @@ -18,6 +18,7 @@
>  #include "qemu/log.h"
>  #include "qemu/module.h"
>  #include "qom/object.h"
> +#include "sysemu/watchdog.h"
>
>  #define OSMR0  0x00
>  #define OSMR1  0x04
> @@ -417,7 +418,7 @@ static void pxa2xx_timer_tick(void *opaque)
>      if (t->num == 3)
>          if (i->reset3 & 1) {
>              i->reset3 = 0;
> -            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +            watchdog_perform_action();
>          }
>  }

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

thanks
-- PMM

[PATCH 3/3] misc: ppc/spapr: replace qemu_system_reset_request() calls with watchdog_perform_action()

[Abhiram Tilak]

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.

I am unsure about the changes in `spapr_watchdog.c` in patch 3, it would be great
if any of the maintainers review it.

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

diff --git a/hw/watchdog/spapr_watchdog.c b/hw/watchdog/spapr_watchdog.c
index 2bb1d3c532..9751b19506 100644
--- a/hw/watchdog/spapr_watchdog.c
+++ b/hw/watchdog/spapr_watchdog.c
@@ -18,6 +18,7 @@
 #include "target/ppc/cpu.h"
 #include "migration/vmstate.h"
 #include "trace.h"
+#include "sysemu/watchdog.h"
 
 #include "hw/ppc/spapr.h"
 
@@ -114,7 +115,7 @@ static void watchdog_expired(void *pw)
         qemu_system_vmstop_request(RUN_STATE_SHUTDOWN);
         break;
     case PSERIES_WDTF_ACTION_HARD_RESTART:
-        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+        watchdog_perform_action();
         break;
     case PSERIES_WDTF_ACTION_DUMP_RESTART:
         CPU_FOREACH(cs) {
-- 
2.42.1

Re: [PATCH 3/3] misc: ppc/spapr: replace qemu_system_reset_request() calls with watchdog_perform_action()

[Peter Maydell]

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.
>
> I am unsure about the changes in `spapr_watchdog.c` in patch 3, it would be great
> if any of the maintainers review it.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2124
> Signed-off-by: Abhiram Tilak <atp.exp@gmail.com>
> ---
>  hw/watchdog/spapr_watchdog.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/watchdog/spapr_watchdog.c b/hw/watchdog/spapr_watchdog.c
> index 2bb1d3c532..9751b19506 100644
> --- a/hw/watchdog/spapr_watchdog.c
> +++ b/hw/watchdog/spapr_watchdog.c
> @@ -18,6 +18,7 @@
>  #include "target/ppc/cpu.h"
>  #include "migration/vmstate.h"
>  #include "trace.h"
> +#include "sysemu/watchdog.h"
>
>  #include "hw/ppc/spapr.h"
>
> @@ -114,7 +115,7 @@ static void watchdog_expired(void *pw)
>          qemu_system_vmstop_request(RUN_STATE_SHUTDOWN);
>          break;
>      case PSERIES_WDTF_ACTION_HARD_RESTART:
> -        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +        watchdog_perform_action();
>          break;
>      case PSERIES_WDTF_ACTION_DUMP_RESTART:
>          CPU_FOREACH(cs) {

This one is more complicated, because the spapr watchdog
has multiple possible behaviours which the guest can ask for.

We had a discussion on the mailing list about this a little while back:
https://lore.kernel.org/qemu-devel/CAFEAcA_KjSgt-oC=d2m6WAdqoRsUcs1W_ji7Ng2fgVjxAWLZEw@mail.gmail.com/

The conclusion was that the watchdog-behaviour QAPI API
needs to be enhanced to be able to handle this kind of
"the guest picks an action" watchdog, so that the user can
either override the guest's choice, or request that the
behaviour be what the guest wants it to be.

thanks
-- PMM

Re: [PATCH 3/3] misc: ppc/spapr: replace qemu_system_reset_request() calls with watchdog_perform_action()

[atp exp]

[-- Attachment #1: Type: text/plain, Size: 2466 bytes --]

I will exclude this patch from the series for now.
According to the discussions, the current code honours the
guest's preference.
Will wait for the enhancements needed in watchdog QAPI.

Abhiram

On Fri, 16 Feb 2024 at 20:24, 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.
> >
> > I am unsure about the changes in `spapr_watchdog.c` in patch 3, it would
> be great
> > if any of the maintainers review it.
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2124
> > Signed-off-by: Abhiram Tilak <atp.exp@gmail.com>
> > ---
> >  hw/watchdog/spapr_watchdog.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/watchdog/spapr_watchdog.c b/hw/watchdog/spapr_watchdog.c
> > index 2bb1d3c532..9751b19506 100644
> > --- a/hw/watchdog/spapr_watchdog.c
> > +++ b/hw/watchdog/spapr_watchdog.c
> > @@ -18,6 +18,7 @@
> >  #include "target/ppc/cpu.h"
> >  #include "migration/vmstate.h"
> >  #include "trace.h"
> > +#include "sysemu/watchdog.h"
> >
> >  #include "hw/ppc/spapr.h"
> >
> > @@ -114,7 +115,7 @@ static void watchdog_expired(void *pw)
> >          qemu_system_vmstop_request(RUN_STATE_SHUTDOWN);
> >          break;
> >      case PSERIES_WDTF_ACTION_HARD_RESTART:
> > -        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> > +        watchdog_perform_action();
> >          break;
> >      case PSERIES_WDTF_ACTION_DUMP_RESTART:
> >          CPU_FOREACH(cs) {
>
> This one is more complicated, because the spapr watchdog
> has multiple possible behaviours which the guest can ask for.
>
> We had a discussion on the mailing list about this a little while back:
>
> https://lore.kernel.org/qemu-devel/CAFEAcA_KjSgt-oC=d2m6WAdqoRsUcs1W_ji7Ng2fgVjxAWLZEw@mail.gmail.com/
>
> The conclusion was that the watchdog-behaviour QAPI API
> needs to be enhanced to be able to handle this kind of
> "the guest picks an action" watchdog, so that the user can
> either override the guest's choice, or request that the
> behaviour be what the guest wants it to be.
>
> thanks
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 3531 bytes --]