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

["Philippe Mathieu-Daudé" <philmd@oss.qualcomm.com>]

On 20/3/24 14:34, Peter Maydell wrote:
> 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.

Thanks!

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

Indeed, but again this is the style *recommended* by the Error API
since commit e3fe3988d7 ("error: Document Error API usage rules").

Callers providing a non-NULL errp should check the return value,
but the QMP handlers generated by gen_command_decl() like
qmp_inject_nmi() here don't return anything.

Markus, what would be your preference?