[PATCH 0/4] hw/nmi: Remove @cpu_index argument

[PATCH 0/4] hw/nmi: Remove @cpu_index argument

[Philippe Mathieu-Daudé]

Have s390x always deliver NMI to the first CPU,
remove the @cpu_index argument from handler,
rename API as nmi_trigger() (not monitor specific).

Philippe Mathieu-Daudé (4):
  hw/nmi: Use object_child_foreach_recursive() in nmi_children()
  hw/s390x/virtio-ccw: Always deliver NMI to first CPU
  hw/nmi: Remove @cpu_index argument from NMIClass::nmi_handler()
  hw/nmi: Remove @cpu_index argument from nmi_trigger()

 qapi/run-state.json        |  5 +++--
 include/hw/nmi.h           | 24 ++++++++++++++++++++++--
 hw/core/nmi.c              | 24 +++++++++---------------
 hw/hppa/machine.c          |  8 +++++---
 hw/i386/x86.c              |  7 ++++---
 hw/intc/m68k_irqc.c        |  6 ++++--
 hw/ipmi/ipmi.c             |  3 +--
 hw/m68k/q800-glue.c        |  6 ++++--
 hw/misc/macio/gpio.c       |  6 ++++--
 hw/ppc/pnv.c               |  6 ++++--
 hw/ppc/spapr.c             |  6 ++++--
 hw/s390x/s390-virtio-ccw.c |  8 ++++----
 hw/watchdog/watchdog.c     |  2 +-
 system/cpus.c              |  2 +-
 hmp-commands.hx            |  2 +-
 15 files changed, 71 insertions(+), 44 deletions(-)

-- 
2.41.0

[PATCH 1/4] hw/nmi: Use object_child_foreach_recursive() in nmi_children()

[Philippe Mathieu-Daudé]

Replace object_child_foreach() and recursion by a single
object_child_foreach_recursive() call.
Propagate the returned value so callers can check it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/core/nmi.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/hw/core/nmi.c b/hw/core/nmi.c
index a7bce8a04a..f5220111c1 100644
--- a/hw/core/nmi.c
+++ b/hw/core/nmi.c
@@ -31,8 +31,6 @@ struct do_nmi_s {
     bool handled;
 };
 
-static void nmi_children(Object *o, struct do_nmi_s *ns);
-
 static int do_nmi(Object *o, void *opaque)
 {
     struct do_nmi_s *ns = opaque;
@@ -47,14 +45,13 @@ static int do_nmi(Object *o, void *opaque)
             return -1;
         }
     }
-    nmi_children(o, ns);
 
     return 0;
 }
 
-static void nmi_children(Object *o, struct do_nmi_s *ns)
+static int nmi_children(Object *o, struct do_nmi_s *ns)
 {
-    object_child_foreach(o, do_nmi, ns);
+    return object_child_foreach_recursive(o, do_nmi, ns);
 }
 
 void nmi_monitor_handle(int cpu_index, Error **errp)
@@ -65,10 +62,9 @@ void nmi_monitor_handle(int cpu_index, Error **errp)
         .handled = false
     };
 
-    nmi_children(object_get_root(), &ns);
-    if (ns.handled) {
+    if (nmi_children(object_get_root(), &ns)) {
         error_propagate(errp, ns.err);
-    } else {
+    } else if (!ns.handled) {
         error_setg(errp, "machine does not provide NMIs");
     }
 }
-- 
2.41.0

Re: [PATCH 1/4] hw/nmi: Use object_child_foreach_recursive() in nmi_children()

[Peter Maydell]

On Tue, 20 Feb 2024 at 15:08, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Replace object_child_foreach() and recursion by a single
> object_child_foreach_recursive() call.
> Propagate the returned value so callers can check it.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 2/4] hw/s390x/virtio-ccw: Always deliver NMI to first CPU

[Philippe Mathieu-Daudé]

We can trigger NMI from HMP or QMP.

QEMU maps the NMI to the s390x per-CPU 'RESTART' interrupt.
Linux guests usually setup this interrupt to trigger kdump
or crash. Such crashdump can be triggered in QEMU by HMP
"nmi" or QMP "inject-nmi" commands.

Using QMP, since we can not select a particular CPU, the first
CPU is used (CPU#0). See the documentation from commit 795dc6e4
("watchdog: Add new Virtual Watchdog action INJECT-NMI"):

  @inject-nmi: a non-maskable interrupt is injected into the
               first VCPU (all VCPUS on x86) (since 2.4)

While we can select a particular CPU on HMP, the guest behavior
is expected to be the same if using CPU #N or CPU #0. Since
always using CPU#0 simplifies API maintainance, update s390_nmi()
to deliver NMI to the first CPU.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 qapi/run-state.json        | 5 +++--
 hw/s390x/s390-virtio-ccw.c | 4 +---
 hmp-commands.hx            | 2 +-
 3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/qapi/run-state.json b/qapi/run-state.json
index 08bc99cb85..a2542f1a50 100644
--- a/qapi/run-state.json
+++ b/qapi/run-state.json
@@ -320,8 +320,9 @@
 #
 # @none: nothing is done
 #
-# @inject-nmi: a non-maskable interrupt is injected into the first
-#     VCPU (all VCPUS on x86) (since 2.4)
+# @inject-nmi: a non-maskable interrupt is injected (architecture
+#     specific: on s390x only the first vCPU receive the NMI, on
+#     other architectures all vCPUs receive it). (since 2.4)
 #
 # Since: 2.1
 ##
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 62804cc228..ba1fa6472f 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -605,9 +605,7 @@ static HotplugHandler *s390_get_hotplug_handler(MachineState *machine,
 
 static void s390_nmi(NMIState *n, int cpu_index, Error **errp)
 {
-    CPUState *cs = qemu_get_cpu(cpu_index);
-
-    s390_cpu_restart(S390_CPU(cs));
+    s390_cpu_restart(S390_CPU(first_cpu));
 }
 
 static ram_addr_t s390_fixup_ram_size(ram_addr_t sz)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 17b5ea839d..2b01bb5926 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -851,7 +851,7 @@ ERST
     },
 SRST
 ``nmi`` *cpu*
-  Inject an NMI on the default CPU (x86/s390) or all CPUs (ppc64).
+  Inject an NMI.
 ERST
 
     {
-- 
2.41.0

Re: [PATCH 2/4] hw/s390x/virtio-ccw: Always deliver NMI to first CPU

[Peter Maydell]

On Tue, 20 Feb 2024 at 15:09, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> We can trigger NMI from HMP or QMP.
>
> QEMU maps the NMI to the s390x per-CPU 'RESTART' interrupt.
> Linux guests usually setup this interrupt to trigger kdump
> or crash. Such crashdump can be triggered in QEMU by HMP
> "nmi" or QMP "inject-nmi" commands.
>
> Using QMP, since we can not select a particular CPU, the first
> CPU is used (CPU#0). See the documentation from commit 795dc6e4
> ("watchdog: Add new Virtual Watchdog action INJECT-NMI"):
>
>   @inject-nmi: a non-maskable interrupt is injected into the
>                first VCPU (all VCPUS on x86) (since 2.4)
>
> While we can select a particular CPU on HMP, the guest behavior
> is expected to be the same if using CPU #N or CPU #0. Since
> always using CPU#0 simplifies API maintainance, update s390_nmi()
> to deliver NMI to the first CPU.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  qapi/run-state.json        | 5 +++--
>  hw/s390x/s390-virtio-ccw.c | 4 +---
>  hmp-commands.hx            | 2 +-
>  3 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/qapi/run-state.json b/qapi/run-state.json
> index 08bc99cb85..a2542f1a50 100644
> --- a/qapi/run-state.json
> +++ b/qapi/run-state.json
> @@ -320,8 +320,9 @@
>  #
>  # @none: nothing is done
>  #
> -# @inject-nmi: a non-maskable interrupt is injected into the first
> -#     VCPU (all VCPUS on x86) (since 2.4)
> +# @inject-nmi: a non-maskable interrupt is injected (architecture
> +#     specific: on s390x only the first vCPU receive the NMI, on

"receives"


> +#     other architectures all vCPUs receive it). (since 2.4)

This part isn't really true, because it's machine-specific rather
than architecture-specific (and many architectures don't implement
it at all). But I don't think we want to introduce that doc cleanup into
this patch. Maybe we should leave it as only saying "all vCPUs on x86",
though.

>  #
>  # Since: 2.1
>  ##
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 62804cc228..ba1fa6472f 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -605,9 +605,7 @@ static HotplugHandler *s390_get_hotplug_handler(MachineState *machine,
>
>  static void s390_nmi(NMIState *n, int cpu_index, Error **errp)
>  {
> -    CPUState *cs = qemu_get_cpu(cpu_index);
> -
> -    s390_cpu_restart(S390_CPU(cs));
> +    s390_cpu_restart(S390_CPU(first_cpu));
>  }
>
>  static ram_addr_t s390_fixup_ram_size(ram_addr_t sz)
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 17b5ea839d..2b01bb5926 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -851,7 +851,7 @@ ERST
>      },
>  SRST
>  ``nmi`` *cpu*
> -  Inject an NMI on the default CPU (x86/s390) or all CPUs (ppc64).
> +  Inject an NMI.
>  ERST
>
>      {

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 2/4] hw/s390x/virtio-ccw: Always deliver NMI to first CPU

[David Hildenbrand]

On 20.02.24 16:08, Philippe Mathieu-Daudé wrote:
> We can trigger NMI from HMP or QMP.
> 
> QEMU maps the NMI to the s390x per-CPU 'RESTART' interrupt.
> Linux guests usually setup this interrupt to trigger kdump
> or crash. Such crashdump can be triggered in QEMU by HMP
> "nmi" or QMP "inject-nmi" commands.
> 
> Using QMP, since we can not select a particular CPU, the first
> CPU is used (CPU#0). See the documentation from commit 795dc6e4
> ("watchdog: Add new Virtual Watchdog action INJECT-NMI"):
> 
>    @inject-nmi: a non-maskable interrupt is injected into the
>                 first VCPU (all VCPUS on x86) (since 2.4)
> 
> While we can select a particular CPU on HMP, the guest behavior
> is expected to be the same if using CPU #N or CPU #0. Since
> always using CPU#0 simplifies API maintainance, update s390_nmi()
> to deliver NMI to the first CPU.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

[PATCH 3/4] hw/nmi: Remove @cpu_index argument from NMIClass::nmi_handler()

[Philippe Mathieu-Daudé]

Only s390x was using the 'cpu_index' argument, but since the
previous commit it isn't anymore (it use the first cpu).
Since this argument is now completely unused, remove it. Have
the callback return a boolean indicating failure.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/nmi.h           | 11 ++++++++++-
 hw/core/nmi.c              |  3 +--
 hw/hppa/machine.c          |  8 +++++---
 hw/i386/x86.c              |  7 ++++---
 hw/intc/m68k_irqc.c        |  6 ++++--
 hw/m68k/q800-glue.c        |  6 ++++--
 hw/misc/macio/gpio.c       |  6 ++++--
 hw/ppc/pnv.c               |  6 ++++--
 hw/ppc/spapr.c             |  6 ++++--
 hw/s390x/s390-virtio-ccw.c |  6 ++++--
 10 files changed, 44 insertions(+), 21 deletions(-)

diff --git a/include/hw/nmi.h b/include/hw/nmi.h
index fff41bebc6..c70db941c9 100644
--- a/include/hw/nmi.h
+++ b/include/hw/nmi.h
@@ -37,7 +37,16 @@ typedef struct NMIState NMIState;
 struct NMIClass {
     InterfaceClass parent_class;
 
-    void (*nmi_monitor_handler)(NMIState *n, int cpu_index, Error **errp);
+    /**
+     * nmi_handler: Callback to handle NMI notifications.
+     *
+     * @n: Class #NMIState state
+     * @errp: pointer to error object
+     *
+     * On success, return %true.
+     * On failure, store an error through @errp and return %false.
+     */
+    bool (*nmi_handler)(NMIState *n, Error **errp);
 };
 
 void nmi_monitor_handle(int cpu_index, Error **errp);
diff --git a/hw/core/nmi.c b/hw/core/nmi.c
index f5220111c1..409164d445 100644
--- a/hw/core/nmi.c
+++ b/hw/core/nmi.c
@@ -40,8 +40,7 @@ static int do_nmi(Object *o, void *opaque)
         NMIClass *nc = NMI_GET_CLASS(n);
 
         ns->handled = true;
-        nc->nmi_monitor_handler(n, ns->cpu_index, &ns->err);
-        if (ns->err) {
+        if (!nc->nmi_handler(n, &ns->err)) {
             return -1;
         }
     }
diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index 5fcaf5884b..75b61a0683 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -673,13 +673,15 @@ static void hppa_machine_reset(MachineState *ms, ShutdownCause reason)
     cpu[0]->env.gr[19] = FW_CFG_IO_BASE;
 }
 
-static void hppa_nmi(NMIState *n, int cpu_index, Error **errp)
+static bool hppa_nmi(NMIState *n, Error **errp)
 {
     CPUState *cs;
 
     CPU_FOREACH(cs) {
         cpu_interrupt(cs, CPU_INTERRUPT_NMI);
     }
+
+    return true;
 }
 
 static void HP_B160L_machine_init_class_init(ObjectClass *oc, void *data)
@@ -705,7 +707,7 @@ static void HP_B160L_machine_init_class_init(ObjectClass *oc, void *data)
     mc->default_ram_id = "ram";
     mc->default_nic = "tulip";
 
-    nc->nmi_monitor_handler = hppa_nmi;
+    nc->nmi_handler = hppa_nmi;
 }
 
 static const TypeInfo HP_B160L_machine_init_typeinfo = {
@@ -741,7 +743,7 @@ static void HP_C3700_machine_init_class_init(ObjectClass *oc, void *data)
     mc->default_ram_id = "ram";
     mc->default_nic = "tulip";
 
-    nc->nmi_monitor_handler = hppa_nmi;
+    nc->nmi_handler = hppa_nmi;
 }
 
 static const TypeInfo HP_C3700_machine_init_typeinfo = {
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 684dce90e9..0d756c4857 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -512,9 +512,8 @@ const CPUArchIdList *x86_possible_cpu_arch_ids(MachineState *ms)
     return ms->possible_cpus;
 }
 
-static void x86_nmi(NMIState *n, int cpu_index, Error **errp)
+static bool x86_nmi(NMIState *n, Error **errp)
 {
-    /* cpu index isn't used */
     CPUState *cs;
 
     CPU_FOREACH(cs) {
@@ -526,6 +525,8 @@ static void x86_nmi(NMIState *n, int cpu_index, Error **errp)
             cpu_interrupt(cs, CPU_INTERRUPT_NMI);
         }
     }
+
+    return true;
 }
 
 static long get_file_size(FILE *f)
@@ -1416,7 +1417,7 @@ static void x86_machine_class_init(ObjectClass *oc, void *data)
     mc->possible_cpu_arch_ids = x86_possible_cpu_arch_ids;
     x86mc->save_tsc_khz = true;
     x86mc->fwcfg_dma_enabled = true;
-    nc->nmi_monitor_handler = x86_nmi;
+    nc->nmi_handler = x86_nmi;
 
     object_class_property_add(oc, X86_MACHINE_SMM, "OnOffAuto",
         x86_machine_get_smm, x86_machine_set_smm,
diff --git a/hw/intc/m68k_irqc.c b/hw/intc/m68k_irqc.c
index 4b11fb9f72..acc9348218 100644
--- a/hw/intc/m68k_irqc.c
+++ b/hw/intc/m68k_irqc.c
@@ -71,9 +71,11 @@ static void m68k_irqc_instance_init(Object *obj)
     qdev_init_gpio_in(DEVICE(obj), m68k_set_irq, M68K_IRQC_LEVEL_NUM);
 }
 
-static void m68k_nmi(NMIState *n, int cpu_index, Error **errp)
+static bool m68k_nmi(NMIState *n, Error **errp)
 {
     m68k_set_irq(n, M68K_IRQC_LEVEL_7, 1);
+
+    return true;
 }
 
 static const VMStateDescription vmstate_m68k_irqc = {
@@ -99,7 +101,7 @@ static void m68k_irqc_class_init(ObjectClass *oc, void *data)
     InterruptStatsProviderClass *ic = INTERRUPT_STATS_PROVIDER_CLASS(oc);
 
     device_class_set_props(dc, m68k_irqc_properties);
-    nc->nmi_monitor_handler = m68k_nmi;
+    nc->nmi_handler = m68k_nmi;
     dc->reset = m68k_irqc_reset;
     dc->vmsd = &vmstate_m68k_irqc;
     ic->get_statistics = m68k_irqc_get_statistics;
diff --git a/hw/m68k/q800-glue.c b/hw/m68k/q800-glue.c
index b5a7713863..f92bd5064a 100644
--- a/hw/m68k/q800-glue.c
+++ b/hw/m68k/q800-glue.c
@@ -159,13 +159,15 @@ static void glue_auxmode_set_irq(void *opaque, int irq, int level)
     s->auxmode = level;
 }
 
-static void glue_nmi(NMIState *n, int cpu_index, Error **errp)
+static bool glue_nmi(NMIState *n, Error **errp)
 {
     GLUEState *s = GLUE(n);
 
     /* Hold NMI active for 100ms */
     GLUE_set_irq(s, GLUE_IRQ_IN_NMI, 1);
     timer_mod(s->nmi_release, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 100);
+
+    return true;
 }
 
 static void glue_nmi_release(void *opaque)
@@ -238,7 +240,7 @@ static void glue_class_init(ObjectClass *klass, void *data)
     dc->vmsd = &vmstate_glue;
     device_class_set_props(dc, glue_properties);
     rc->phases.hold = glue_reset_hold;
-    nc->nmi_monitor_handler = glue_nmi;
+    nc->nmi_handler = glue_nmi;
 }
 
 static const TypeInfo glue_info_types[] = {
diff --git a/hw/misc/macio/gpio.c b/hw/misc/macio/gpio.c
index 549563747d..9ac93d9ed5 100644
--- a/hw/misc/macio/gpio.c
+++ b/hw/misc/macio/gpio.c
@@ -183,10 +183,12 @@ static void macio_gpio_reset(DeviceState *dev)
     macio_set_gpio(s, 1, true);
 }
 
-static void macio_gpio_nmi(NMIState *n, int cpu_index, Error **errp)
+static bool macio_gpio_nmi(NMIState *n, Error **errp)
 {
     macio_set_gpio(MACIO_GPIO(n), 9, true);
     macio_set_gpio(MACIO_GPIO(n), 9, false);
+
+    return true;
 }
 
 static void macio_gpio_class_init(ObjectClass *oc, void *data)
@@ -196,7 +198,7 @@ static void macio_gpio_class_init(ObjectClass *oc, void *data)
 
     dc->reset = macio_gpio_reset;
     dc->vmsd = &vmstate_macio_gpio;
-    nc->nmi_monitor_handler = macio_gpio_nmi;
+    nc->nmi_handler = macio_gpio_nmi;
 }
 
 static const TypeInfo macio_gpio_init_info = {
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 0297871bdd..f572f8d0ce 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -2321,13 +2321,15 @@ static void pnv_cpu_do_nmi_on_cpu(CPUState *cs, run_on_cpu_data arg)
     }
 }
 
-static void pnv_nmi(NMIState *n, int cpu_index, Error **errp)
+static bool pnv_nmi(NMIState *n, Error **errp)
 {
     CPUState *cs;
 
     CPU_FOREACH(cs) {
         async_run_on_cpu(cs, pnv_cpu_do_nmi_on_cpu, RUN_ON_CPU_NULL);
     }
+
+    return true;
 }
 
 static void pnv_machine_class_init(ObjectClass *oc, void *data)
@@ -2351,7 +2353,7 @@ static void pnv_machine_class_init(ObjectClass *oc, void *data)
     mc->default_ram_size = 1 * GiB;
     mc->default_ram_id = "pnv.ram";
     ispc->print_info = pnv_pic_print_info;
-    nc->nmi_monitor_handler = pnv_nmi;
+    nc->nmi_handler = pnv_nmi;
 
     object_class_property_add_bool(oc, "hb-mode",
                                    pnv_machine_get_hb, pnv_machine_set_hb);
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 0d72d286d8..7327bf3429 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3505,13 +3505,15 @@ void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg)
     }
 }
 
-static void spapr_nmi(NMIState *n, int cpu_index, Error **errp)
+static bool spapr_nmi(NMIState *n, Error **errp)
 {
     CPUState *cs;
 
     CPU_FOREACH(cs) {
         async_run_on_cpu(cs, spapr_do_system_reset_on_cpu, RUN_ON_CPU_NULL);
     }
+
+    return true;
 }
 
 int spapr_lmb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
@@ -4672,7 +4674,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->nvdimm_supported = true;
     smc->resize_hpt_default = SPAPR_RESIZE_HPT_ENABLED;
     fwc->get_dev_path = spapr_get_fw_dev_path;
-    nc->nmi_monitor_handler = spapr_nmi;
+    nc->nmi_handler = spapr_nmi;
     smc->phb_placement = spapr_phb_placement;
     vhc->cpu_in_nested = spapr_cpu_in_nested;
     vhc->deliver_hv_excp = spapr_exit_nested;
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index ba1fa6472f..817f414767 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -603,9 +603,11 @@ static HotplugHandler *s390_get_hotplug_handler(MachineState *machine,
     return NULL;
 }
 
-static void s390_nmi(NMIState *n, int cpu_index, Error **errp)
+static bool s390_nmi(NMIState *n, Error **errp)
 {
     s390_cpu_restart(S390_CPU(first_cpu));
+
+    return true;
 }
 
 static ram_addr_t s390_fixup_ram_size(ram_addr_t sz)
@@ -774,7 +776,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
     mc->default_cpu_type = S390_CPU_TYPE_NAME("qemu");
     hc->plug = s390_machine_device_plug;
     hc->unplug_request = s390_machine_device_unplug_request;
-    nc->nmi_monitor_handler = s390_nmi;
+    nc->nmi_handler = s390_nmi;
     mc->default_ram_id = "s390.ram";
     mc->default_nic = "virtio-net-ccw";
 
-- 
2.41.0

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

[Peter Maydell]

On Tue, 20 Feb 2024 at 15:09, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Only s390x was using the 'cpu_index' argument, but since the
> previous commit it isn't anymore (it use the first cpu).
> Since this argument is now completely unused, remove it. Have
> the callback return a boolean indicating failure.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/hw/nmi.h           | 11 ++++++++++-
>  hw/core/nmi.c              |  3 +--
>  hw/hppa/machine.c          |  8 +++++---
>  hw/i386/x86.c              |  7 ++++---
>  hw/intc/m68k_irqc.c        |  6 ++++--
>  hw/m68k/q800-glue.c        |  6 ++++--
>  hw/misc/macio/gpio.c       |  6 ++++--
>  hw/ppc/pnv.c               |  6 ++++--
>  hw/ppc/spapr.c             |  6 ++++--
>  hw/s390x/s390-virtio-ccw.c |  6 ++++--
>  10 files changed, 44 insertions(+), 21 deletions(-)
>
> diff --git a/include/hw/nmi.h b/include/hw/nmi.h
> index fff41bebc6..c70db941c9 100644
> --- a/include/hw/nmi.h
> +++ b/include/hw/nmi.h
> @@ -37,7 +37,16 @@ typedef struct NMIState NMIState;
>  struct NMIClass {
>      InterfaceClass parent_class;
>
> -    void (*nmi_monitor_handler)(NMIState *n, int cpu_index, Error **errp);
> +    /**
> +     * nmi_handler: Callback to handle NMI notifications.
> +     *
> +     * @n: Class #NMIState state
> +     * @errp: pointer to error object
> +     *
> +     * On success, return %true.
> +     * On failure, store an error through @errp and return %false.
> +     */
> +    bool (*nmi_handler)(NMIState *n, Error **errp);

Any particular reason to change the method name here?

Do we really need to indicate failure both through the bool return
and the Error** ?

thanks
-- PMM

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

[Philippe Mathieu-Daudé]

On 20/3/24 14:23, Peter Maydell wrote:
> On Tue, 20 Feb 2024 at 15:09, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Only s390x was using the 'cpu_index' argument, but since the
>> previous commit it isn't anymore (it use the first cpu).
>> Since this argument is now completely unused, remove it. Have
>> the callback return a boolean indicating failure.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   include/hw/nmi.h           | 11 ++++++++++-
>>   hw/core/nmi.c              |  3 +--
>>   hw/hppa/machine.c          |  8 +++++---
>>   hw/i386/x86.c              |  7 ++++---
>>   hw/intc/m68k_irqc.c        |  6 ++++--
>>   hw/m68k/q800-glue.c        |  6 ++++--
>>   hw/misc/macio/gpio.c       |  6 ++++--
>>   hw/ppc/pnv.c               |  6 ++++--
>>   hw/ppc/spapr.c             |  6 ++++--
>>   hw/s390x/s390-virtio-ccw.c |  6 ++++--
>>   10 files changed, 44 insertions(+), 21 deletions(-)
>>
>> diff --git a/include/hw/nmi.h b/include/hw/nmi.h
>> index fff41bebc6..c70db941c9 100644
>> --- a/include/hw/nmi.h
>> +++ b/include/hw/nmi.h
>> @@ -37,7 +37,16 @@ typedef struct NMIState NMIState;
>>   struct NMIClass {
>>       InterfaceClass parent_class;
>>
>> -    void (*nmi_monitor_handler)(NMIState *n, int cpu_index, Error **errp);
>> +    /**
>> +     * nmi_handler: Callback to handle NMI notifications.
>> +     *
>> +     * @n: Class #NMIState state
>> +     * @errp: pointer to error object
>> +     *
>> +     * On success, return %true.
>> +     * On failure, store an error through @errp and return %false.
>> +     */
>> +    bool (*nmi_handler)(NMIState *n, Error **errp);
> 
> Any particular reason to change the method name here?
> 
> Do we really need to indicate failure both through the bool return
> and the Error** ?

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

     error: Document Error API usage rules

     This merely codifies existing practice, with one exception: the rule
     advising against returning void, where existing practice is mixed.

     When the Error API was created, we adopted the (unwritten) rule to
     return void when the function returns no useful value on success,
     unlike GError, which recommends to return true on success and false
     on error then.

     [...]

     Make the rule advising against returning void official by putting it
     in writing.  This will hopefully reduce confusion.

   * - Whenever practical, also return a value that indicates success /
   *   failure.  This can make the error checking more concise, and can
   *   avoid useless error object creation and destruction.  Note that
   *   we still have many functions returning void.  We recommend
   *   • bool-valued functions return true on success / false on failure,
   *   • pointer-valued functions return non-null / null pointer, and
   *   • integer-valued functions return non-negative / negative.

Anyway I'll respin removing @cpu_index as a single change :)

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

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 20/3/24 14:23, Peter Maydell wrote:
>> On Tue, 20 Feb 2024 at 15:09, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>>
>>> Only s390x was using the 'cpu_index' argument, but since the
>>> previous commit it isn't anymore (it use the first cpu).
>>> Since this argument is now completely unused, remove it. Have
>>> the callback return a boolean indicating failure.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   include/hw/nmi.h           | 11 ++++++++++-
>>>   hw/core/nmi.c              |  3 +--
>>>   hw/hppa/machine.c          |  8 +++++---
>>>   hw/i386/x86.c              |  7 ++++---
>>>   hw/intc/m68k_irqc.c        |  6 ++++--
>>>   hw/m68k/q800-glue.c        |  6 ++++--
>>>   hw/misc/macio/gpio.c       |  6 ++++--
>>>   hw/ppc/pnv.c               |  6 ++++--
>>>   hw/ppc/spapr.c             |  6 ++++--
>>>   hw/s390x/s390-virtio-ccw.c |  6 ++++--
>>>   10 files changed, 44 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/include/hw/nmi.h b/include/hw/nmi.h
>>> index fff41bebc6..c70db941c9 100644
>>> --- a/include/hw/nmi.h
>>> +++ b/include/hw/nmi.h
>>> @@ -37,7 +37,16 @@ typedef struct NMIState NMIState;
>>>   struct NMIClass {
>>>       InterfaceClass parent_class;
>>>
>>> -    void (*nmi_monitor_handler)(NMIState *n, int cpu_index, Error **errp);
>>> +    /**
>>> +     * nmi_handler: Callback to handle NMI notifications.
>>> +     *
>>> +     * @n: Class #NMIState state
>>> +     * @errp: pointer to error object
>>> +     *
>>> +     * On success, return %true.
>>> +     * On failure, store an error through @errp and return %false.
>>> +     */
>>> +    bool (*nmi_handler)(NMIState *n, Error **errp);
>> Any particular reason to change the method name here?
>> Do we really need to indicate failure both through the bool return
>> and the Error** ?
>
> No, but this is the style *recommended* by the Error API since
> commit e3fe3988d7 ("error: Document Error API usage rules"):
>
>     error: Document Error API usage rules
>
>     This merely codifies existing practice, with one exception: the rule
>     advising against returning void, where existing practice is mixed.
>
>     When the Error API was created, we adopted the (unwritten) rule to
>     return void when the function returns no useful value on success,
>     unlike GError, which recommends to return true on success and false
>     on error then.
>
>     [...]
>
>     Make the rule advising against returning void official by putting it
>     in writing.  This will hopefully reduce confusion.
>
>   * - Whenever practical, also return a value that indicates success /
>   *   failure.  This can make the error checking more concise, and can
>   *   avoid useless error object creation and destruction.  Note that

It's the difference between

    if (!frobnicate(arg, errp)) {
        return;
    }

and

    frobnicate(arg, &err);
    if (err) {
        error_propagate(errp, err);
        return;
    }

Readabilty dies by a thousand cuts.

GError got this right.  We deviated from it for Error, until we
understood why it's right.

Another win: &error_abort gives you a backtrace into frobnicate() with
the former, and into error_propagate() with the latter.

>   *   we still have many functions returning void.  We recommend
>   *   • bool-valued functions return true on success / false on failure,
>   *   • pointer-valued functions return non-null / null pointer, and
>   *   • integer-valued functions return non-negative / negative.
>
> Anyway I'll respin removing @cpu_index as a single change :)

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

[Peter Maydell]

On Wed, 20 Mar 2024 at 19:05, Markus Armbruster <armbru@redhat.com> wrote:
>
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>
> > On 20/3/24 14:23, Peter Maydell wrote:
> >> On Tue, 20 Feb 2024 at 15:09, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >>>
> >>> Only s390x was using the 'cpu_index' argument, but since the
> >>> previous commit it isn't anymore (it use the first cpu).
> >>> Since this argument is now completely unused, remove it. Have
> >>> the callback return a boolean indicating failure.
> >>>
> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >>> ---
> >>>   include/hw/nmi.h           | 11 ++++++++++-
> >>>   hw/core/nmi.c              |  3 +--
> >>>   hw/hppa/machine.c          |  8 +++++---
> >>>   hw/i386/x86.c              |  7 ++++---
> >>>   hw/intc/m68k_irqc.c        |  6 ++++--
> >>>   hw/m68k/q800-glue.c        |  6 ++++--
> >>>   hw/misc/macio/gpio.c       |  6 ++++--
> >>>   hw/ppc/pnv.c               |  6 ++++--
> >>>   hw/ppc/spapr.c             |  6 ++++--
> >>>   hw/s390x/s390-virtio-ccw.c |  6 ++++--
> >>>   10 files changed, 44 insertions(+), 21 deletions(-)
> >>>
> >>> diff --git a/include/hw/nmi.h b/include/hw/nmi.h
> >>> index fff41bebc6..c70db941c9 100644
> >>> --- a/include/hw/nmi.h
> >>> +++ b/include/hw/nmi.h
> >>> @@ -37,7 +37,16 @@ typedef struct NMIState NMIState;
> >>>   struct NMIClass {
> >>>       InterfaceClass parent_class;
> >>>
> >>> -    void (*nmi_monitor_handler)(NMIState *n, int cpu_index, Error **errp);
> >>> +    /**
> >>> +     * nmi_handler: Callback to handle NMI notifications.
> >>> +     *
> >>> +     * @n: Class #NMIState state
> >>> +     * @errp: pointer to error object
> >>> +     *
> >>> +     * On success, return %true.
> >>> +     * On failure, store an error through @errp and return %false.
> >>> +     */
> >>> +    bool (*nmi_handler)(NMIState *n, Error **errp);
> >> Any particular reason to change the method name here?
> >> Do we really need to indicate failure both through the bool return
> >> and the Error** ?
> >
> > No, but this is the style *recommended* by the Error API since
> > commit e3fe3988d7 ("error: Document Error API usage rules"):
> >
> >     error: Document Error API usage rules
> >
> >     This merely codifies existing practice, with one exception: the rule
> >     advising against returning void, where existing practice is mixed.
> >
> >     When the Error API was created, we adopted the (unwritten) rule to
> >     return void when the function returns no useful value on success,
> >     unlike GError, which recommends to return true on success and false
> >     on error then.
> >
> >     [...]
> >
> >     Make the rule advising against returning void official by putting it
> >     in writing.  This will hopefully reduce confusion.
> >
> >   * - Whenever practical, also return a value that indicates success /
> >   *   failure.  This can make the error checking more concise, and can
> >   *   avoid useless error object creation and destruction.  Note that
>
> It's the difference between
>
>     if (!frobnicate(arg, errp)) {
>         return;
>     }
>
> and
>
>     frobnicate(arg, &err);
>     if (err) {
>         error_propagate(errp, err);
>         return;
>     }
>
> Readabilty dies by a thousand cuts.
>
> GError got this right.  We deviated from it for Error, until we
> understood why it's right.
>
> Another win: &error_abort gives you a backtrace into frobnicate() with
> the former, and into error_propagate() with the latter.

Fair enough. (When I made the comment I was vaguely wondering
if we wanted to keep the return value available to distinguish
"this hook has handled the NMI, don't keep iterating" from
"no error, but you should keep iterating through other handlers".
But I think in the end my feeling is we should always stop
after the first NMI handler we find regardless.

-- PMM

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

[Philippe Mathieu-Daudé]

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.
+ *
+ * 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);
 }
 
-- 
2.41.0

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

Re: [PATCH 0/4] hw/nmi: Remove @cpu_index argument

[Thomas Huth]

On 20/02/2024 16.08, Philippe Mathieu-Daudé wrote:
> Have s390x always deliver NMI to the first CPU,
> remove the @cpu_index argument from handler,
> rename API as nmi_trigger() (not monitor specific).

Could you please add some rationale here why this is needed / desired?

Thanks,
  Thomas


> Philippe Mathieu-Daudé (4):
>    hw/nmi: Use object_child_foreach_recursive() in nmi_children()
>    hw/s390x/virtio-ccw: Always deliver NMI to first CPU
>    hw/nmi: Remove @cpu_index argument from NMIClass::nmi_handler()
>    hw/nmi: Remove @cpu_index argument from nmi_trigger()
> 
>   qapi/run-state.json        |  5 +++--
>   include/hw/nmi.h           | 24 ++++++++++++++++++++++--
>   hw/core/nmi.c              | 24 +++++++++---------------
>   hw/hppa/machine.c          |  8 +++++---
>   hw/i386/x86.c              |  7 ++++---
>   hw/intc/m68k_irqc.c        |  6 ++++--
>   hw/ipmi/ipmi.c             |  3 +--
>   hw/m68k/q800-glue.c        |  6 ++++--
>   hw/misc/macio/gpio.c       |  6 ++++--
>   hw/ppc/pnv.c               |  6 ++++--
>   hw/ppc/spapr.c             |  6 ++++--
>   hw/s390x/s390-virtio-ccw.c |  8 ++++----
>   hw/watchdog/watchdog.c     |  2 +-
>   system/cpus.c              |  2 +-
>   hmp-commands.hx            |  2 +-
>   15 files changed, 71 insertions(+), 44 deletions(-)
> 

Re: [PATCH 0/4] hw/nmi: Remove @cpu_index argument

[Philippe Mathieu-Daudé]

On 20/2/24 16:19, Thomas Huth wrote:
> On 20/02/2024 16.08, Philippe Mathieu-Daudé wrote:
>> Have s390x always deliver NMI to the first CPU,
>> remove the @cpu_index argument from handler,
>> rename API as nmi_trigger() (not monitor specific).
> 
> Could you please add some rationale here why this is needed / desired?

Heterogeneous machines, a NMI have to reach all NMI-aware HW.

See also "Remove cpu_interrupt() from hw/":
https://lore.kernel.org/qemu-devel/20240220192625.17944-1-philmd@linaro.org/

Re: [PATCH 0/4] hw/nmi: Remove @cpu_index argument

[Philippe Mathieu-Daudé]

On 20/2/24 16:19, Thomas Huth wrote:
> On 20/02/2024 16.08, Philippe Mathieu-Daudé wrote:
>> Have s390x always deliver NMI to the first CPU,
>> remove the @cpu_index argument from handler,
>> rename API as nmi_trigger() (not monitor specific).
> 
> Could you please add some rationale here why this is needed / desired?

I'm not sure it is desired... I'm trying to get the NMI delivery
working in heterogeneous machine, but now I'm wondering whether
hw/core/nmi.c was designed with that in mind or likely not.

I suppose in a complex machine you explicitly wire IRQ lines such
NMI, so they are delivered to a particular INTC or CPU core, and
there is no "broadcast this signal to all listeners registered
for NMI events".

> 
> Thanks,
>   Thomas
> 
> 
>> Philippe Mathieu-Daudé (4):
>>    hw/nmi: Use object_child_foreach_recursive() in nmi_children()
>>    hw/s390x/virtio-ccw: Always deliver NMI to first CPU
>>    hw/nmi: Remove @cpu_index argument from NMIClass::nmi_handler()
>>    hw/nmi: Remove @cpu_index argument from nmi_trigger()

Re: [PATCH 0/4] hw/nmi: Remove @cpu_index argument

[Mark Burton]

> On 20 Mar 2024, at 12:19, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.
> 
> On 20/2/24 16:19, Thomas Huth wrote:
>> On 20/02/2024 16.08, Philippe Mathieu-Daudé wrote:
>>> Have s390x always deliver NMI to the first CPU,
>>> remove the @cpu_index argument from handler,
>>> rename API as nmi_trigger() (not monitor specific).
>> 
>> Could you please add some rationale here why this is needed / desired?
> 
> I'm not sure it is desired... I'm trying to get the NMI delivery
> working in heterogeneous machine, but now I'm wondering whether
> hw/core/nmi.c was designed with that in mind or likely not.
> 
> I suppose in a complex machine you explicitly wire IRQ lines such
> NMI, so they are delivered to a particular INTC or CPU core, and
> there is no "broadcast this signal to all listeners registered
> for NMI events".
> 

I think those two things are sort of similar. e.g. we could have a machine in which many of the components all receive a ‘reset’ signal, but that would indeed be wired up explicitly from the thing generating that reset signal and all the components wanting to receive it. And, yes, there may be components that are not reset by that signal….

Cheers
Mark.


>> 
>> Thanks,
>>  Thomas
>> 
>> 
>>> Philippe Mathieu-Daudé (4):
>>>   hw/nmi: Use object_child_foreach_recursive() in nmi_children()
>>>   hw/s390x/virtio-ccw: Always deliver NMI to first CPU
>>>   hw/nmi: Remove @cpu_index argument from NMIClass::nmi_handler()
>>>   hw/nmi: Remove @cpu_index argument from nmi_trigger()
> 

Re: [PATCH 0/4] hw/nmi: Remove @cpu_index argument

[Peter Maydell]

On Wed, 20 Mar 2024 at 11:20, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 20/2/24 16:19, Thomas Huth wrote:
> > On 20/02/2024 16.08, Philippe Mathieu-Daudé wrote:
> >> Have s390x always deliver NMI to the first CPU,
> >> remove the @cpu_index argument from handler,
> >> rename API as nmi_trigger() (not monitor specific).
> >
> > Could you please add some rationale here why this is needed / desired?
>
> I'm not sure it is desired... I'm trying to get the NMI delivery
> working in heterogeneous machine, but now I'm wondering whether
> hw/core/nmi.c was designed with that in mind or likely not.
>
> I suppose in a complex machine you explicitly wire IRQ lines such
> NMI, so they are delivered to a particular INTC or CPU core, and
> there is no "broadcast this signal to all listeners registered
> for NMI events".

I think in a complex heterogenous machine you do want the
monitor NMI command to do something sensible, but the
definition of "sensible" is going to be machine-specific:
probably it will be "raise NMI in some way on some core in
the main application processor cluster", and it's the machine
model that's going to know what "sensible" is for that machine.

The current hw/core/nmi.c code is a bit odd because it's partly
working with a cpu_index and partly not: the code passes cpu_index
around, but in practice for the QMP command the user can't set
which CPU to operate on, and for everything except s390 the
implementation doesn't care anyway. My impression from the IRC
discussion is that it's not really necessary for the S390 that
the monitor user be able to specify which CPU to NMI (and in any
case you can only do that from the HMP command, not the QMP
command, AIUI), so getting rid of that weird inconsistency makes
sense to me: and that's what this patchset is doing.

What NMI probably ought to be is board-specific: so it's like
having some notional front panel switch labeled "NMI", and the
board gets to decide what that means (which is usually going to be
"send some NMI like interrupt to the first CPU in the main cluster",
but could be something else). It doesn't need to be like a
front panel switch with a rotary-selector for 'pick a CPU'
plus a button for "send NMI to that CPU". In fact we're quite
close to "it's a board thing" already, because almost every
implementation of the TYPE_NMI interface is actually a machine
model. (The exceptions are hw/intc/m68k_irqc.c,
hw/m68k/q800-glue.c and hw/misc/macio/gpio.c.)

So I think that:
 * we should indeed drop the cpu_index stuff, per this patch:
   it's unnecessary cruft we don't really use
 * we should look at whether the three classes listed above
   which implement TYPE_NMI on a non-machine-model are really
   the right way to do that, i.e. whether it would be a lot of
   effort to effectively switch to having nmi_monitor_handler
   be a simple method on MachineClass. Not walking the QOM
   tree would make the NMI infrastructure rather simpler.
   (But I just looked at the macio case, and it's inside a
   PCI device, so at best that's a bunch of clunky plumbing.)
 * failing that, we should look at whether we should really
   continue to walk the whole QOM tree calling methods on every
   TYPE_NMI object, or whether we can say "once we've found one
   implementation we're done". This also depends on those three
   non-MachineClass implementations, because obviously there's
   only ever one MachineClass object in the system. This is
   kind of useful for heterogenous boards which use the m68k
   or ppc devices listed above (seems highly unlikely), but it
   would mean you can override the default "those objects handle
   NMI" by having your heterogenous board implement TYPE_NMI,
   and then since it's earlier in the QOM tree that will be
   the method called, not the ones on specific devices.
   (This one I think we can easily do -- my quick check suggests
   that TYPE_M68K_IRQ is only used in the m68k virt board,
   TYPE_GLUE is only used in the m68k q800 board, and
   TYPE_MACIO_GPIO is only used in the ppc mac99 board. So in
   fact in all cases there's only ever one TYPE_NMI interface
   present in the system.)

The last two aren't blockers for heterogenous-system work,
though: they just seem to me like nice cleanup of this interface.

thanks
-- PMM

Re: [PATCH 0/4] hw/nmi: Remove @cpu_index argument

[Mark Burton]

> On 20 Mar 2024, at 13:00, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.
> 
> On Wed, 20 Mar 2024 at 11:20, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>> 
>> On 20/2/24 16:19, Thomas Huth wrote:
>>> On 20/02/2024 16.08, Philippe Mathieu-Daudé wrote:
>>>> Have s390x always deliver NMI to the first CPU,
>>>> remove the @cpu_index argument from handler,
>>>> rename API as nmi_trigger() (not monitor specific).
>>> 
>>> Could you please add some rationale here why this is needed / desired?
>> 
>> I'm not sure it is desired... I'm trying to get the NMI delivery
>> working in heterogeneous machine, but now I'm wondering whether
>> hw/core/nmi.c was designed with that in mind or likely not.
>> 
>> I suppose in a complex machine you explicitly wire IRQ lines such
>> NMI, so they are delivered to a particular INTC or CPU core, and
>> there is no "broadcast this signal to all listeners registered
>> for NMI events".
> 
> I think in a complex heterogenous machine you do want the
> monitor NMI command to do something sensible, but the
> definition of "sensible" is going to be machine-specific:
> probably it will be "raise NMI in some way on some core in
> the main application processor cluster", and it's the machine
> model that's going to know what "sensible" is for that machine.
> 
> The current hw/core/nmi.c code is a bit odd because it's partly
> working with a cpu_index and partly not: the code passes cpu_index
> around, but in practice for the QMP command the user can't set
> which CPU to operate on, and for everything except s390 the
> implementation doesn't care anyway. My impression from the IRC
> discussion is that it's not really necessary for the S390 that
> the monitor user be able to specify which CPU to NMI (and in any
> case you can only do that from the HMP command, not the QMP
> command, AIUI), so getting rid of that weird inconsistency makes
> sense to me: and that's what this patchset is doing.
> 
> What NMI probably ought to be is board-specific: so it's like
> having some notional front panel switch labeled "NMI", and the

Do the youngsters of today know what one of those is ?
	:-)


Is there a reason for not using a GPIO interface for ’NMI’ - wiring it up like any other wire?

Cheers
Mark.

> board gets to decide what that means (which is usually going to be
> "send some NMI like interrupt to the first CPU in the main cluster",
> but could be something else). It doesn't need to be like a
> front panel switch with a rotary-selector for 'pick a CPU'
> plus a button for "send NMI to that CPU". In fact we're quite
> close to "it's a board thing" already, because almost every
> implementation of the TYPE_NMI interface is actually a machine
> model. (The exceptions are hw/intc/m68k_irqc.c,
> hw/m68k/q800-glue.c and hw/misc/macio/gpio.c.)
> 
> So I think that:
> * we should indeed drop the cpu_index stuff, per this patch:
>   it's unnecessary cruft we don't really use
> * we should look at whether the three classes listed above
>   which implement TYPE_NMI on a non-machine-model are really
>   the right way to do that, i.e. whether it would be a lot of
>   effort to effectively switch to having nmi_monitor_handler
>   be a simple method on MachineClass. Not walking the QOM
>   tree would make the NMI infrastructure rather simpler.
>   (But I just looked at the macio case, and it's inside a
>   PCI device, so at best that's a bunch of clunky plumbing.)
> * failing that, we should look at whether we should really
>   continue to walk the whole QOM tree calling methods on every
>   TYPE_NMI object, or whether we can say "once we've found one
>   implementation we're done". This also depends on those three
>   non-MachineClass implementations, because obviously there's
>   only ever one MachineClass object in the system. This is
>   kind of useful for heterogenous boards which use the m68k
>   or ppc devices listed above (seems highly unlikely), but it
>   would mean you can override the default "those objects handle
>   NMI" by having your heterogenous board implement TYPE_NMI,
>   and then since it's earlier in the QOM tree that will be
>   the method called, not the ones on specific devices.
>   (This one I think we can easily do -- my quick check suggests
>   that TYPE_M68K_IRQ is only used in the m68k virt board,
>   TYPE_GLUE is only used in the m68k q800 board, and
>   TYPE_MACIO_GPIO is only used in the ppc mac99 board. So in
>   fact in all cases there's only ever one TYPE_NMI interface
>   present in the system.)
> 
> The last two aren't blockers for heterogenous-system work,
> though: they just seem to me like nice cleanup of this interface.
> 
> thanks
> -- PMM

Re: [PATCH 0/4] hw/nmi: Remove @cpu_index argument

[Peter Maydell]

On Wed, 20 Mar 2024 at 12:31, Mark Burton <mburton@qti.qualcomm.com> wrote:
> > On 20 Mar 2024, at 13:00, Peter Maydell <peter.maydell@linaro.org> wrote:
> > What NMI probably ought to be is board-specific: so it's like
> > having some notional front panel switch labeled "NMI", and the
>
> Do the youngsters of today know what one of those is ?
>         :-)
>
>
> Is there a reason for not using a GPIO interface for ’NMI’ - wiring it up like any other wire?

The places we want to generate 'NMI' are:
 * when the user uses the 'nmi' command in the monitor
 * in the generic "a watchdog timer expired" handling code (in the
   case where the user used a monitor command to say "trigger an NMI
   if the watchdog times out")
 * when the user requested to send the VM an NMI via IPMI

In all those cases we don't have a pointer to the board or
any kind of idea of what a GPIO wire would be, and because at
the moment TYPE_MACHINE is not a subclass of TYPE_DEVICE a
machine model can't have external GPIO lines anyway. From
a convenience point of view all those callsites simply want
to be able to call a function to say "trigger an NMI please"
(which is what nmi_monitor_handle() does). Beyond that the
implementation of that function is just whatever is convenient.

If in your particular machine model it makes sense to have NMI
be something you deal with via GPIO wiring, you can implement the
TYPE_NMI interface on your machine class to work by raising a
GPIO line.

thanks
-- PMM

Re: [PATCH 0/4] hw/nmi: Remove @cpu_index argument

[Mark Burton]

> On 20 Mar 2024, at 14:55, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.
> 
> On Wed, 20 Mar 2024 at 12:31, Mark Burton <mburton@qti.qualcomm.com> wrote:
>>> On 20 Mar 2024, at 13:00, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> What NMI probably ought to be is board-specific: so it's like
>>> having some notional front panel switch labeled "NMI", and the
>> 
>> Do the youngsters of today know what one of those is ?
>>        :-)
>> 
>> 
>> Is there a reason for not using a GPIO interface for ’NMI’ - wiring it up like any other wire?
> 
> The places we want to generate 'NMI' are:
> * when the user uses the 'nmi' command in the monitor
> * in the generic "a watchdog timer expired" handling code (in the
>   case where the user used a monitor command to say "trigger an NMI
>   if the watchdog times out")
> * when the user requested to send the VM an NMI via IPMI
> 
> In all those cases we don't have a pointer to the board or
> any kind of idea of what a GPIO wire would be, and because at
> the moment TYPE_MACHINE is not a subclass of TYPE_DEVICE a
> machine model can't have external GPIO lines anyway. From
> a convenience point of view all those callsites simply want
> to be able to call a function to say "trigger an NMI please"
> (which is what nmi_monitor_handle() does). Beyond that the
> implementation of that function is just whatever is convenient.
> 
> If in your particular machine model it makes sense to have NMI
> be something you deal with via GPIO wiring, you can implement the
> TYPE_NMI interface on your machine class to work by raising a
> GPIO line.


I’d broaden this to all ’signals’ (IRQ, Reset etc) - and I guess similar statements apply, with the “bridge” between the function and the GPIO mechanism moved closer or further from the originator(s) of the activity.

The issue isn’t my “machine” model, rather the compose-ability of (any) such machine.  A-priori, a model writer doesn’t know if they should respond directly to an NMI or not - Hence they dont know if they should implement the TYPE_NMI or not. That’s a decision only the machine composer knows.
My suggestion would be to use a GPIO interface to models, which can then be appropriately wired. (And, hence, to have a single place that implements the TYPE_NMI interface and provides the GPIO wire ready for wiring to appropriate devices). 

Cheers
Mark.


> 
> thanks
> -- PMM

Re: [PATCH 0/4] hw/nmi: Remove @cpu_index argument

[Peter Maydell]

On Wed, 20 Mar 2024 at 14:10, Mark Burton <mburton@qti.qualcomm.com> wrote:
> I’d broaden this to all ’signals’ (IRQ, Reset etc) - and I guess
> similar statements apply, with the “bridge” between the function
> and the GPIO mechanism moved closer or further from the originator(s)
> of the activity.
>
> The issue isn’t my “machine” model, rather the compose-ability of
> (any) such machine.  A-priori, a model writer doesn’t know if they
> should respond directly to an NMI or not - Hence they dont know if
> they should implement the TYPE_NMI or not. That’s a decision only
> the machine composer knows.
> My suggestion would be to use a GPIO interface to models, which can
> then be appropriately wired. (And, hence, to have a single place
> that implements the TYPE_NMI interface and provides the GPIO wire
> ready for wiring to appropriate devices).

I feel like that's a long way in the future, but my back-of-the-envelope
design sketch of that is that the TYPE_MACHINE class that's implementing
the "I am just a container for all the devices that the user has
specified and wired together" machine would itself implement TYPE_NMI and
when an NMI came in it would assert a GPIO line that the user could
wire up, or not wire up, as they chose.

Right now we can't do that though, because, among other reasons,
TYPE_MACHINE isn't a TYPE_DEVICE. (I do want to fix that, though:
I'm hoping it won't be too difficult.)

thanks
-- PMM

Re: [PATCH 0/4] hw/nmi: Remove @cpu_index argument

[Mark Burton]

> On 20 Mar 2024, at 16:00, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.
> 
> On Wed, 20 Mar 2024 at 14:10, Mark Burton <mburton@qti.qualcomm.com> wrote:
>> I’d broaden this to all ’signals’ (IRQ, Reset etc) - and I guess
>> similar statements apply, with the “bridge” between the function
>> and the GPIO mechanism moved closer or further from the originator(s)
>> of the activity.
>> 
>> The issue isn’t my “machine” model, rather the compose-ability of
>> (any) such machine.  A-priori, a model writer doesn’t know if they
>> should respond directly to an NMI or not - Hence they dont know if
>> they should implement the TYPE_NMI or not. That’s a decision only
>> the machine composer knows.
>> My suggestion would be to use a GPIO interface to models, which can
>> then be appropriately wired. (And, hence, to have a single place
>> that implements the TYPE_NMI interface and provides the GPIO wire
>> ready for wiring to appropriate devices).
> 
> I feel like that's a long way in the future, but my back-of-the-envelope
> design sketch of that is that the TYPE_MACHINE class that's implementing
> the "I am just a container for all the devices that the user has
> specified and wired together" machine would itself implement TYPE_NMI and
> when an NMI came in it would assert a GPIO line that the user could
> wire up, or not wire up, as they chose.
> 

Yeah - makes sense.
Cheers
Mark.



> Right now we can't do that though, because, among other reasons,
> TYPE_MACHINE isn't a TYPE_DEVICE. (I do want to fix that, though:
> I'm hoping it won't be too difficult.)
> 
> thanks
> -- PMM

Re: [PATCH 0/4] hw/nmi: Remove @cpu_index argument

[Cédric Le Goater]

On 3/20/24 16:00, Peter Maydell wrote:
> On Wed, 20 Mar 2024 at 14:10, Mark Burton <mburton@qti.qualcomm.com> wrote:
>> I’d broaden this to all ’signals’ (IRQ, Reset etc) - and I guess
>> similar statements apply, with the “bridge” between the function
>> and the GPIO mechanism moved closer or further from the originator(s)
>> of the activity.
>>
>> The issue isn’t my “machine” model, rather the compose-ability of
>> (any) such machine.  A-priori, a model writer doesn’t know if they
>> should respond directly to an NMI or not - Hence they dont know if
>> they should implement the TYPE_NMI or not. That’s a decision only
>> the machine composer knows.
>> My suggestion would be to use a GPIO interface to models, which can
>> then be appropriately wired. (And, hence, to have a single place
>> that implements the TYPE_NMI interface and provides the GPIO wire
>> ready for wiring to appropriate devices).
> 
> I feel like that's a long way in the future, but my back-of-the-envelope
> design sketch of that is that the TYPE_MACHINE class that's implementing
> the "I am just a container for all the devices that the user has
> specified and wired together" machine would itself implement TYPE_NMI and
> when an NMI came in it would assert a GPIO line that the user could
> wire up, or not wire up, as they chose.
> 
> Right now we can't do that though, because, among other reasons,
> TYPE_MACHINE isn't a TYPE_DEVICE. (I do want to fix that, though:
> I'm hoping it won't be too difficult.)

Oh that's interesting. Will that introduce an extra level of container
with multiple machines below ?

/qemu
   /machine[0]
     ...
     /peripheral (container)
     /peripheral-anon (container)
   /machine[1]
     ...
     /peripheral (container)
     /peripheral-anon (container)
   /unattached (container)
     ...
     /sysbus (System)
     /system[0] (memory-region)

Thanks,

C.

Re: [PATCH 0/4] hw/nmi: Remove @cpu_index argument

[Peter Maydell]

On Fri, 22 Mar 2024 at 14:08, Cédric Le Goater <clegoate@redhat.com> wrote:
>
> On 3/20/24 16:00, Peter Maydell wrote:
> > On Wed, 20 Mar 2024 at 14:10, Mark Burton <mburton@qti.qualcomm.com> wrote:
> >> I’d broaden this to all ’signals’ (IRQ, Reset etc) - and I guess
> >> similar statements apply, with the “bridge” between the function
> >> and the GPIO mechanism moved closer or further from the originator(s)
> >> of the activity.
> >>
> >> The issue isn’t my “machine” model, rather the compose-ability of
> >> (any) such machine.  A-priori, a model writer doesn’t know if they
> >> should respond directly to an NMI or not - Hence they dont know if
> >> they should implement the TYPE_NMI or not. That’s a decision only
> >> the machine composer knows.
> >> My suggestion would be to use a GPIO interface to models, which can
> >> then be appropriately wired. (And, hence, to have a single place
> >> that implements the TYPE_NMI interface and provides the GPIO wire
> >> ready for wiring to appropriate devices).
> >
> > I feel like that's a long way in the future, but my back-of-the-envelope
> > design sketch of that is that the TYPE_MACHINE class that's implementing
> > the "I am just a container for all the devices that the user has
> > specified and wired together" machine would itself implement TYPE_NMI and
> > when an NMI came in it would assert a GPIO line that the user could
> > wire up, or not wire up, as they chose.
> >
> > Right now we can't do that though, because, among other reasons,
> > TYPE_MACHINE isn't a TYPE_DEVICE. (I do want to fix that, though:
> > I'm hoping it won't be too difficult.)
>
> Oh that's interesting. Will that introduce an extra level of container
> with multiple machines below ?

No, I don't intend that we should have multiple machines in one
simulation, only that the thing which is "container for all the
machine's devices" shouldn't be a weirdly distinct type from
the SoC "container for devices" devices. What I'm primarily hoping
to remedy by making TYPE_MACHINE a subclass of TYPE_DEVICE to
deal with inconsistencies like:
 * reset of machine objects is nonstandard
 * machine models can't use facilities like having qdev gpio
   lines, so wind up calling qemu_allocate_irqs() directly

None of these are big things, but they're a bit paper-cut-ish.

-- PMM