On Tue, 20 Feb 2024 at 15:09, Philippe Mathieu-Daudé <phi...@linaro.org> wrote:
>
> nmi_monitor_handle() is not related to the monitor,
> rename it as nmi_trigger().
> Return boolean value
> indicating success / failure. The 'cpu_index' argument
> is not used, remove it.
>
> Signed-off-by: Philippe Mathieu-Daudé <phi...@linaro.org>
> ---
> include/hw/nmi.h | 13 ++++++++++++-
> hw/core/nmi.c | 9 ++++-----
> hw/ipmi/ipmi.c | 3 +--
> hw/watchdog/watchdog.c | 2 +-
> system/cpus.c | 2 +-
> 5 files changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/include/hw/nmi.h b/include/hw/nmi.h
> index c70db941c9..32b27067f2 100644
> --- a/include/hw/nmi.h
> +++ b/include/hw/nmi.h
> @@ -49,6 +49,17 @@ struct NMIClass {
> bool (*nmi_handler)(NMIState *n, Error **errp);
> };
>
> -void nmi_monitor_handle(int cpu_index, Error **errp);
> +/**
> + * nmi_trigger: Trigger a NMI.
> + *
> + * @errp: pointer to error object
> + *
> + * Iterate over all objects implementing the TYPE_NMI interface
> + * and deliver NMI to them.
I think I would document this something like;
* nmi_trigger: Trigger an NMI, in a machine-specific way
*
* This function triggers an NMI, in a machine-specific way. The
* intention is that this should typically trigger a guest kernel
* dump or reboot, and might happen as a result of user request
* from the monitor, watchdog timeouts, and similar events.
* (For example on the x86 PC it triggers an NMI on all CPUs,
* and on s390 it triggers the RESTART interrupt on the first CPU.)
*
* The NMI is triggered by looking for QOM objects which
* implement the TYPE_NMI interface, and calling their nmi_handler
* method. Usually it is the machine model class that implements
* this interface.
*
* Not all machines implement NMI handling; this function
* will return an error if used on a machine which does not
* implement NMIs.
(In an ideal world we would also document per-board what
the NMI handling is, in the user-facing board docs...)
> + *
> + * On success, return %true.
> + * On failure, store an error through @errp and return %false.
> + */
> +bool nmi_trigger(Error **errp);
Why return a bool here? None of the callsites looks at the
return value.
thanks
-- PMM
