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

Commit Message

Philippe Mathieu-Daudé Feb. 20, 2024, 3:08 p.m. UTC

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é <[e-mail]>
---
 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(-)

Comments

Peter Maydell March 20, 2024, 1:34 p.m. UTC | #1

On Tue, 20 Feb 2024 at 15:09, Philippe Mathieu-Daudé <[e-mail]> 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é <[e-mail]>
> ---
>  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

Patch

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.
+ *
+ * On success, return %true.
+ * On failure, store an error through @errp and return %false.
+ */
+bool nmi_trigger(Error **errp);
 
 #endif /* NMI_H */
diff --git a/hw/core/nmi.c b/hw/core/nmi.c
index 409164d445..a740f39c98 100644
--- a/hw/core/nmi.c
+++ b/hw/core/nmi.c
@@ -22,11 +22,8 @@ 
 #include "qemu/osdep.h"
 #include "hw/nmi.h"
 #include "qapi/error.h"
-#include "qemu/module.h"
-#include "monitor/monitor.h"
 
 struct do_nmi_s {
-    int cpu_index;
     Error *err;
     bool handled;
 };
@@ -53,19 +50,21 @@  static int nmi_children(Object *o, struct do_nmi_s *ns)
     return object_child_foreach_recursive(o, do_nmi, ns);
 }
 
-void nmi_monitor_handle(int cpu_index, Error **errp)
+bool nmi_trigger(Error **errp)
 {
     struct do_nmi_s ns = {
-        .cpu_index = cpu_index,
         .err = NULL,
         .handled = false
     };
 
     if (nmi_children(object_get_root(), &ns)) {
         error_propagate(errp, ns.err);
+        return false;
     } else if (!ns.handled) {
         error_setg(errp, "machine does not provide NMIs");
+        return false;
     }
+    return true;
 }
 
 static const TypeInfo nmi_info = {
diff --git a/hw/ipmi/ipmi.c b/hw/ipmi/ipmi.c
index bbb07b151e..45e36a7492 100644
--- a/hw/ipmi/ipmi.c
+++ b/hw/ipmi/ipmi.c
@@ -59,8 +59,7 @@  static int ipmi_do_hw_op(IPMIInterface *s, enum ipmi_op op, int checkonly)
         if (checkonly) {
             return 0;
         }
-        /* We don't care what CPU we use. */
-        nmi_monitor_handle(0, NULL);
+        nmi_trigger(NULL);
         return 0;
 
     case IPMI_SHUTDOWN_VIA_ACPI_OVERTEMP:
diff --git a/hw/watchdog/watchdog.c b/hw/watchdog/watchdog.c
index 955046161b..d324c761aa 100644
--- a/hw/watchdog/watchdog.c
+++ b/hw/watchdog/watchdog.c
@@ -81,7 +81,7 @@  void watchdog_perform_action(void)
 
     case WATCHDOG_ACTION_INJECT_NMI:
         qapi_event_send_watchdog(WATCHDOG_ACTION_INJECT_NMI);
-        nmi_monitor_handle(0, NULL);
+        nmi_trigger(NULL);
         break;
 
     default:
diff --git a/system/cpus.c b/system/cpus.c
index 68d161d96b..f11ee3d404 100644
--- a/system/cpus.c
+++ b/system/cpus.c
@@ -856,6 +856,6 @@  exit:
 
 void qmp_inject_nmi(Error **errp)
 {
-    nmi_monitor_handle(monitor_get_cpu_index(monitor_cur()), errp);
+    nmi_trigger(errp);
 }