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

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

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
>