Re: [PATCH 4/4] hw/nmi: Remove @cpu_index argument from nmi_trigger()

[Peter Maydell]

On Tue, 20 Feb 2024 at 15:09, Philippe Mathieu-Daudé <philmd@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é <philmd@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