spapr watchdog vs watchdog_perform_action() / QMP watchdog-set-action

spapr watchdog vs watchdog_perform_action() / QMP watchdog-set-action

[Peter Maydell]

Hi; one of the "bitesized tasks" we have listed is to convert
watchdog timers which directly call qemu_system_reset_request() on
watchdog timeout to call watchdog_perform_action() instead. This
means they honour the QMP commands that let the user specifiy
the behaviour on watchdog expiry:
https://www.qemu.org/docs/master/interop/qemu-qmp-ref.html#qapidoc-141
https://www.qemu.org/docs/master/interop/qemu-qmp-ref.html#qapidoc-129
(choices include reset, power off the system, do nothing, etc).

There are only a few remaining watchdogs that don't use the
watchdog_perform_action() function. In most cases the change
is obvious and easy: just make them do that instead of calling
qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET).

However, the hw/watchdog/spapr_watchdog.c case is trickier. As
far as I can tell from the sources, this is a watchdog set up via
a hypercall, and the guest makes a choice of "power off, restart,
or dump and restart" for its on-expiry action.

What should this watchdog's interaction with the watchdog-set-action
QMP command be? If the user says "do X" and the guest says "do Y",
which do we do? (With the current code, we always honour what
the guest asks for and ignore what the user asks for.)

(The bitesized task for watchdog_perform_action() is
https://gitlab.com/qemu-project/qemu/-/issues/2124 . For the
purposes of this email thread I'm only after a concrete decision
about what we think the right thing is, not for any code. Then
I can write that up in the bug for potential new contributors.)

thanks
-- PMM

Re: spapr watchdog vs watchdog_perform_action() / QMP watchdog-set-action

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> Hi; one of the "bitesized tasks" we have listed is to convert
> watchdog timers which directly call qemu_system_reset_request() on
> watchdog timeout to call watchdog_perform_action() instead. This
> means they honour the QMP commands that let the user specifiy
> the behaviour on watchdog expiry:
> https://www.qemu.org/docs/master/interop/qemu-qmp-ref.html#qapidoc-141
> https://www.qemu.org/docs/master/interop/qemu-qmp-ref.html#qapidoc-129
> (choices include reset, power off the system, do nothing, etc).
>
> There are only a few remaining watchdogs that don't use the
> watchdog_perform_action() function. In most cases the change
> is obvious and easy: just make them do that instead of calling
> qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET).
>
> However, the hw/watchdog/spapr_watchdog.c case is trickier. As
> far as I can tell from the sources, this is a watchdog set up via
> a hypercall, and the guest makes a choice of "power off, restart,
> or dump and restart" for its on-expiry action.
>
> What should this watchdog's interaction with the watchdog-set-action
> QMP command be? If the user says "do X" and the guest says "do Y",
> which do we do? (With the current code, we always honour what
> the guest asks for and ignore what the user asks for.)

Gut reaction: when the user says "do X", the guest should not get a say.
But one of the values of X could be "whatever the guest says".

> (The bitesized task for watchdog_perform_action() is
> https://gitlab.com/qemu-project/qemu/-/issues/2124 . For the
> purposes of this email thread I'm only after a concrete decision
> about what we think the right thing is, not for any code. Then
> I can write that up in the bug for potential new contributors.)
>
> thanks
> -- PMM

Re: spapr watchdog vs watchdog_perform_action() / QMP watchdog-set-action

[Peter Maydell]

On Fri, 26 Jan 2024 at 20:49, Markus Armbruster <armbru@redhat.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > Hi; one of the "bitesized tasks" we have listed is to convert
> > watchdog timers which directly call qemu_system_reset_request() on
> > watchdog timeout to call watchdog_perform_action() instead. This
> > means they honour the QMP commands that let the user specifiy
> > the behaviour on watchdog expiry:
> > https://www.qemu.org/docs/master/interop/qemu-qmp-ref.html#qapidoc-141
> > https://www.qemu.org/docs/master/interop/qemu-qmp-ref.html#qapidoc-129
> > (choices include reset, power off the system, do nothing, etc).
> >
> > There are only a few remaining watchdogs that don't use the
> > watchdog_perform_action() function. In most cases the change
> > is obvious and easy: just make them do that instead of calling
> > qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET).
> >
> > However, the hw/watchdog/spapr_watchdog.c case is trickier. As
> > far as I can tell from the sources, this is a watchdog set up via
> > a hypercall, and the guest makes a choice of "power off, restart,
> > or dump and restart" for its on-expiry action.
> >
> > What should this watchdog's interaction with the watchdog-set-action
> > QMP command be? If the user says "do X" and the guest says "do Y",
> > which do we do? (With the current code, we always honour what
> > the guest asks for and ignore what the user asks for.)
>
> Gut reaction: when the user says "do X", the guest should not get a say.
> But one of the values of X could be "whatever the guest says".

Mmm. Slightly awkwardly, we don't currently distinguish between
"action is reset because the user never expressed a preference"
and "action is reset because the user specifically asked for that",
but I guess in theory we could make that distinction. (Conveniently
there is no QMP action for "query current watchdog-action state",
so we don't need to worry about reflecting that distinction in the
QMP interface if we make it.)

-- PMM

Re: spapr watchdog vs watchdog_perform_action() / QMP watchdog-set-action

[David Gibson]

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

On Sat, Jan 27, 2024 at 01:08:02PM +0000, Peter Maydell wrote:
> On Fri, 26 Jan 2024 at 20:49, Markus Armbruster <armbru@redhat.com> wrote:
> >
> > Peter Maydell <peter.maydell@linaro.org> writes:
> >
> > > Hi; one of the "bitesized tasks" we have listed is to convert
> > > watchdog timers which directly call qemu_system_reset_request() on
> > > watchdog timeout to call watchdog_perform_action() instead. This
> > > means they honour the QMP commands that let the user specifiy
> > > the behaviour on watchdog expiry:
> > > https://www.qemu.org/docs/master/interop/qemu-qmp-ref.html#qapidoc-141
> > > https://www.qemu.org/docs/master/interop/qemu-qmp-ref.html#qapidoc-129
> > > (choices include reset, power off the system, do nothing, etc).
> > >
> > > There are only a few remaining watchdogs that don't use the
> > > watchdog_perform_action() function. In most cases the change
> > > is obvious and easy: just make them do that instead of calling
> > > qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET).
> > >
> > > However, the hw/watchdog/spapr_watchdog.c case is trickier. As
> > > far as I can tell from the sources, this is a watchdog set up via
> > > a hypercall, and the guest makes a choice of "power off, restart,
> > > or dump and restart" for its on-expiry action.
> > >
> > > What should this watchdog's interaction with the watchdog-set-action
> > > QMP command be? If the user says "do X" and the guest says "do Y",
> > > which do we do? (With the current code, we always honour what
> > > the guest asks for and ignore what the user asks for.)
> >
> > Gut reaction: when the user says "do X", the guest should not get a say.
> > But one of the values of X could be "whatever the guest says".

That would also be my inclination.

> Mmm. Slightly awkwardly, we don't currently distinguish between
> "action is reset because the user never expressed a preference"
> and "action is reset because the user specifically asked for that",
> but I guess in theory we could make that distinction. (Conveniently
> there is no QMP action for "query current watchdog-action state",
> so we don't need to worry about reflecting that distinction in the
> QMP interface if we make it.)

I think that change is necessary in order to accomodate this sort of
watchdog with guest-progammable behaviour (which is part of the PAPR
spec, so we shouldn't just ignore it).

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: spapr watchdog vs watchdog_perform_action() / QMP watchdog-set-action

[Peter Maydell]

On Mon, 29 Jan 2024 at 04:02, David Gibson <david@gibson.dropbear.id.au> wrote:
>
> On Sat, Jan 27, 2024 at 01:08:02PM +0000, Peter Maydell wrote:
> > On Fri, 26 Jan 2024 at 20:49, Markus Armbruster <armbru@redhat.com> wrote:
> > >
> > > Peter Maydell <peter.maydell@linaro.org> writes:
> > >
> > > > Hi; one of the "bitesized tasks" we have listed is to convert
> > > > watchdog timers which directly call qemu_system_reset_request() on
> > > > watchdog timeout to call watchdog_perform_action() instead. This
> > > > means they honour the QMP commands that let the user specifiy
> > > > the behaviour on watchdog expiry:
> > > > https://www.qemu.org/docs/master/interop/qemu-qmp-ref.html#qapidoc-141
> > > > https://www.qemu.org/docs/master/interop/qemu-qmp-ref.html#qapidoc-129
> > > > (choices include reset, power off the system, do nothing, etc).
> > > >
> > > > There are only a few remaining watchdogs that don't use the
> > > > watchdog_perform_action() function. In most cases the change
> > > > is obvious and easy: just make them do that instead of calling
> > > > qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET).
> > > >
> > > > However, the hw/watchdog/spapr_watchdog.c case is trickier. As
> > > > far as I can tell from the sources, this is a watchdog set up via
> > > > a hypercall, and the guest makes a choice of "power off, restart,
> > > > or dump and restart" for its on-expiry action.
> > > >
> > > > What should this watchdog's interaction with the watchdog-set-action
> > > > QMP command be? If the user says "do X" and the guest says "do Y",
> > > > which do we do? (With the current code, we always honour what
> > > > the guest asks for and ignore what the user asks for.)
> > >
> > > Gut reaction: when the user says "do X", the guest should not get a say.
> > > But one of the values of X could be "whatever the guest says".
>
> That would also be my inclination.
>
> > Mmm. Slightly awkwardly, we don't currently distinguish between
> > "action is reset because the user never expressed a preference"
> > and "action is reset because the user specifically asked for that",
> > but I guess in theory we could make that distinction. (Conveniently
> > there is no QMP action for "query current watchdog-action state",
> > so we don't need to worry about reflecting that distinction in the
> > QMP interface if we make it.)
>
> I think that change is necessary in order to accomodate this sort of
> watchdog with guest-progammable behaviour (which is part of the PAPR
> spec, so we shouldn't just ignore it).

I've now filed https://gitlab.com/qemu-project/qemu/-/issues/2185
to track the spapr-watchdog vs watchdog-set-action missing feature
that we've discussed in this thread. (I haven't marked it as a
"bite-sized feature", though I think it probably could be one if
somebody familiar with at least one of spapr or QMP wanted to
flesh out the required changes a bit.)

-- PMM