[PATCH 0/5] ppc: sreset and machine check injection

[PATCH 0/5] ppc: sreset and machine check injection

[Nicholas Piggin]

This adds nmi injection for pnv, and similar mce injection for
spapr and pnv. The mce injection has already uncovered quite a
few bugs in Linux papr guest and one in pnv host code, so it
has been already very useful. The mambo simulator can do similar
injection but it's a bit more clunky and needs to run KVM and
QEMU in the sim to test papr guests, which is quite slow.

HMIs like timebase corruption would be another good candidate
for error injection.

Thanks,
Nick

Nicholas Piggin (5):
  ppc/spapr: tweak change system reset helper
  ppc/pnv: Add support for NMI interface
  nmi: add MCE class for implementing machine check injection commands
  ppc/spapr: Implement mce injection
  ppc/pnv: Implement mce injection

 hmp-commands.hx              | 20 ++++++++-
 hw/core/nmi.c                | 61 ++++++++++++++++++++++++++
 hw/ppc/pnv.c                 | 85 +++++++++++++++++++++++++++++++++++-
 hw/ppc/spapr.c               | 63 ++++++++++++++++++++++++--
 include/hw/nmi.h             | 20 +++++++++
 include/hw/ppc/spapr.h       |  3 ++
 include/monitor/hmp-target.h |  1 -
 include/monitor/hmp.h        |  1 +
 monitor/hmp-cmds.c           |  1 +
 target/ppc/cpu.h             |  3 +-
 target/ppc/excp_helper.c     | 17 ++++++--
 target/ppc/monitor.c         | 11 +++++
 12 files changed, 275 insertions(+), 11 deletions(-)

-- 
2.23.0

[PATCH 1/5] ppc/spapr: tweak change system reset helper

[Nicholas Piggin]

Rather than have the helper take an optional vector address
override, instead have its caller modify env->nip itself.
This is more consistent when adding pnv nmi support, and also
with mce injection added later.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/spapr.c           | 9 ++++++---
 target/ppc/cpu.h         | 2 +-
 target/ppc/excp_helper.c | 5 +----
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 9a2bd501aa..785c41d205 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3385,13 +3385,13 @@ static void spapr_machine_finalizefn(Object *obj)
 void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg)
 {
     SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
 
     cpu_synchronize_state(cs);
     /* If FWNMI is inactive, addr will be -1, which will deliver to 0x100 */
     if (spapr->fwnmi_system_reset_addr != -1) {
         uint64_t rtas_addr, addr;
-        PowerPCCPU *cpu = POWERPC_CPU(cs);
-        CPUPPCState *env = &cpu->env;
 
         /* get rtas addr from fdt */
         rtas_addr = spapr_get_rtas_addr();
@@ -3405,7 +3405,10 @@ void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg)
         stq_be_phys(&address_space_memory, addr + sizeof(uint64_t), 0);
         env->gpr[3] = addr;
     }
-    ppc_cpu_do_system_reset(cs, spapr->fwnmi_system_reset_addr);
+    ppc_cpu_do_system_reset(cs);
+    if (spapr->fwnmi_system_reset_addr != -1) {
+        env->nip = spapr->fwnmi_system_reset_addr;
+    }
 }
 
 static void spapr_nmi(NMIState *n, int cpu_index, Error **errp)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 88d9449555..f4a5304d43 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1220,7 +1220,7 @@ int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
 int ppc32_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
                                int cpuid, void *opaque);
 #ifndef CONFIG_USER_ONLY
-void ppc_cpu_do_system_reset(CPUState *cs, target_ulong vector);
+void ppc_cpu_do_system_reset(CPUState *cs);
 void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector);
 extern const VMStateDescription vmstate_ppc_cpu;
 #endif
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 08bc885ca6..7f2b5899d3 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -961,15 +961,12 @@ static void ppc_hw_interrupt(CPUPPCState *env)
     }
 }
 
-void ppc_cpu_do_system_reset(CPUState *cs, target_ulong vector)
+void ppc_cpu_do_system_reset(CPUState *cs)
 {
     PowerPCCPU *cpu = POWERPC_CPU(cs);
     CPUPPCState *env = &cpu->env;
 
     powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_RESET);
-    if (vector != -1) {
-        env->nip = vector;
-    }
 }
 
 void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector)
-- 
2.23.0

Re: [EXTERNAL] [PATCH 1/5] ppc/spapr: tweak change system reset helper

[Cédric Le Goater]

On 3/25/20 3:41 PM, Nicholas Piggin wrote:
> Rather than have the helper take an optional vector address
> override, instead have its caller modify env->nip itself.
> This is more consistent when adding pnv nmi support, and also
> with mce injection added later.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Reviewed-by: Cédric Le Goater <clg@kaod.org>


> ---
>  hw/ppc/spapr.c           | 9 ++++++---
>  target/ppc/cpu.h         | 2 +-
>  target/ppc/excp_helper.c | 5 +----
>  3 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 9a2bd501aa..785c41d205 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3385,13 +3385,13 @@ static void spapr_machine_finalizefn(Object *obj)
>  void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg)
>  {
>      SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
> 
>      cpu_synchronize_state(cs);
>      /* If FWNMI is inactive, addr will be -1, which will deliver to 0x100 */
>      if (spapr->fwnmi_system_reset_addr != -1) {
>          uint64_t rtas_addr, addr;
> -        PowerPCCPU *cpu = POWERPC_CPU(cs);
> -        CPUPPCState *env = &cpu->env;
> 
>          /* get rtas addr from fdt */
>          rtas_addr = spapr_get_rtas_addr();
> @@ -3405,7 +3405,10 @@ void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg)
>          stq_be_phys(&address_space_memory, addr + sizeof(uint64_t), 0);
>          env->gpr[3] = addr;
>      }
> -    ppc_cpu_do_system_reset(cs, spapr->fwnmi_system_reset_addr);
> +    ppc_cpu_do_system_reset(cs);
> +    if (spapr->fwnmi_system_reset_addr != -1) {
> +        env->nip = spapr->fwnmi_system_reset_addr;
> +    }
>  }
> 
>  static void spapr_nmi(NMIState *n, int cpu_index, Error **errp)
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 88d9449555..f4a5304d43 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1220,7 +1220,7 @@ int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
>  int ppc32_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
>                                 int cpuid, void *opaque);
>  #ifndef CONFIG_USER_ONLY
> -void ppc_cpu_do_system_reset(CPUState *cs, target_ulong vector);
> +void ppc_cpu_do_system_reset(CPUState *cs);
>  void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector);
>  extern const VMStateDescription vmstate_ppc_cpu;
>  #endif
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 08bc885ca6..7f2b5899d3 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -961,15 +961,12 @@ static void ppc_hw_interrupt(CPUPPCState *env)
>      }
>  }
> 
> -void ppc_cpu_do_system_reset(CPUState *cs, target_ulong vector)
> +void ppc_cpu_do_system_reset(CPUState *cs)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>      CPUPPCState *env = &cpu->env;
> 
>      powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_RESET);
> -    if (vector != -1) {
> -        env->nip = vector;
> -    }
>  }
> 
>  void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector)
> 

Re: [PATCH 1/5] ppc/spapr: tweak change system reset helper

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3458 bytes --]

On Thu, Mar 26, 2020 at 12:41:43AM +1000, Nicholas Piggin wrote:
> Rather than have the helper take an optional vector address
> override, instead have its caller modify env->nip itself.
> This is more consistent when adding pnv nmi support, and also
> with mce injection added later.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Applied to ppc-for-5.1.

> ---
>  hw/ppc/spapr.c           | 9 ++++++---
>  target/ppc/cpu.h         | 2 +-
>  target/ppc/excp_helper.c | 5 +----
>  3 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 9a2bd501aa..785c41d205 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3385,13 +3385,13 @@ static void spapr_machine_finalizefn(Object *obj)
>  void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg)
>  {
>      SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
>  
>      cpu_synchronize_state(cs);
>      /* If FWNMI is inactive, addr will be -1, which will deliver to 0x100 */
>      if (spapr->fwnmi_system_reset_addr != -1) {
>          uint64_t rtas_addr, addr;
> -        PowerPCCPU *cpu = POWERPC_CPU(cs);
> -        CPUPPCState *env = &cpu->env;
>  
>          /* get rtas addr from fdt */
>          rtas_addr = spapr_get_rtas_addr();
> @@ -3405,7 +3405,10 @@ void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg)
>          stq_be_phys(&address_space_memory, addr + sizeof(uint64_t), 0);
>          env->gpr[3] = addr;
>      }
> -    ppc_cpu_do_system_reset(cs, spapr->fwnmi_system_reset_addr);
> +    ppc_cpu_do_system_reset(cs);
> +    if (spapr->fwnmi_system_reset_addr != -1) {
> +        env->nip = spapr->fwnmi_system_reset_addr;
> +    }
>  }
>  
>  static void spapr_nmi(NMIState *n, int cpu_index, Error **errp)
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 88d9449555..f4a5304d43 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1220,7 +1220,7 @@ int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
>  int ppc32_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
>                                 int cpuid, void *opaque);
>  #ifndef CONFIG_USER_ONLY
> -void ppc_cpu_do_system_reset(CPUState *cs, target_ulong vector);
> +void ppc_cpu_do_system_reset(CPUState *cs);
>  void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector);
>  extern const VMStateDescription vmstate_ppc_cpu;
>  #endif
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 08bc885ca6..7f2b5899d3 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -961,15 +961,12 @@ static void ppc_hw_interrupt(CPUPPCState *env)
>      }
>  }
>  
> -void ppc_cpu_do_system_reset(CPUState *cs, target_ulong vector)
> +void ppc_cpu_do_system_reset(CPUState *cs)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>      CPUPPCState *env = &cpu->env;
>  
>      powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_RESET);
> -    if (vector != -1) {
> -        env->nip = vector;
> -    }
>  }
>  
>  void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector)

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/5] ppc/pnv: Add support for NMI interface

[Nicholas Piggin]

This implements the NMI interface for the PNV machine, similarly to
commit 3431648272d ("spapr: Add support for new NMI interface") for
SPAPR.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/pnv.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index b75ad06390..671535ebe6 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -27,6 +27,7 @@
 #include "sysemu/runstate.h"
 #include "sysemu/cpus.h"
 #include "sysemu/device_tree.h"
+#include "sysemu/hw_accel.h"
 #include "target/ppc/cpu.h"
 #include "qemu/log.h"
 #include "hw/ppc/fdt.h"
@@ -34,6 +35,7 @@
 #include "hw/ppc/pnv.h"
 #include "hw/ppc/pnv_core.h"
 #include "hw/loader.h"
+#include "hw/nmi.h"
 #include "exec/address-spaces.h"
 #include "qapi/visitor.h"
 #include "monitor/monitor.h"
@@ -1955,10 +1957,35 @@ static void pnv_machine_set_hb(Object *obj, bool value, Error **errp)
     }
 }
 
+static void pnv_cpu_do_nmi_on_cpu(CPUState *cs, run_on_cpu_data arg)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+
+    cpu_synchronize_state(cs);
+    ppc_cpu_do_system_reset(cs);
+    /*
+     * SRR1[42:45] is set to 0100 which the ISA defines as implementation
+     * dependent. POWER processors use this for xscom triggered interrupts,
+     * which come from the BMC or NMI IPIs.
+     */
+    env->spr[SPR_SRR1] |= PPC_BIT(43);
+}
+
+static void pnv_nmi(NMIState *n, int cpu_index, Error **errp)
+{
+    CPUState *cs;
+
+    CPU_FOREACH(cs) {
+        async_run_on_cpu(cs, pnv_cpu_do_nmi_on_cpu, RUN_ON_CPU_NULL);
+    }
+}
+
 static void pnv_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
     InterruptStatsProviderClass *ispc = INTERRUPT_STATS_PROVIDER_CLASS(oc);
+    NMIClass *nc = NMI_CLASS(oc);
 
     mc->desc = "IBM PowerNV (Non-Virtualized)";
     mc->init = pnv_init;
@@ -1975,6 +2002,7 @@ static void pnv_machine_class_init(ObjectClass *oc, void *data)
     mc->default_ram_size = INITRD_LOAD_ADDR + INITRD_MAX_SIZE;
     mc->default_ram_id = "pnv.ram";
     ispc->print_info = pnv_pic_print_info;
+    nc->nmi_monitor_handler = pnv_nmi;
 
     object_class_property_add_bool(oc, "hb-mode",
                                    pnv_machine_get_hb, pnv_machine_set_hb,
@@ -2038,7 +2066,7 @@ static const TypeInfo types[] = {
         .class_size    = sizeof(PnvMachineClass),
         .interfaces = (InterfaceInfo[]) {
             { TYPE_INTERRUPT_STATS_PROVIDER },
-            { },
+            { TYPE_NMI },
         },
     },
     {
-- 
2.23.0

Re: [PATCH 2/5] ppc/pnv: Add support for NMI interface

[Cédric Le Goater]

[ Please use clg@kaod.org ! ]

On 3/25/20 3:41 PM, Nicholas Piggin wrote:
> This implements the NMI interface for the PNV machine, similarly to
> commit 3431648272d ("spapr: Add support for new NMI interface") for
> SPAPR.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

one minor comment,

Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>  hw/ppc/pnv.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index b75ad06390..671535ebe6 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -27,6 +27,7 @@
>  #include "sysemu/runstate.h"
>  #include "sysemu/cpus.h"
>  #include "sysemu/device_tree.h"
> +#include "sysemu/hw_accel.h"
>  #include "target/ppc/cpu.h"
>  #include "qemu/log.h"
>  #include "hw/ppc/fdt.h"
> @@ -34,6 +35,7 @@
>  #include "hw/ppc/pnv.h"
>  #include "hw/ppc/pnv_core.h"
>  #include "hw/loader.h"
> +#include "hw/nmi.h"
>  #include "exec/address-spaces.h"
>  #include "qapi/visitor.h"
>  #include "monitor/monitor.h"
> @@ -1955,10 +1957,35 @@ static void pnv_machine_set_hb(Object *obj, bool value, Error **errp)
>      }
>  }
> 
> +static void pnv_cpu_do_nmi_on_cpu(CPUState *cs, run_on_cpu_data arg)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
> +
> +    cpu_synchronize_state(cs);
> +    ppc_cpu_do_system_reset(cs);
> +    /*
> +     * SRR1[42:45] is set to 0100 which the ISA defines as implementation

I see 'System Reset' in ISA 3.0

> +     * dependent. POWER processors use this for xscom triggered interrupts,
> +     * which come from the BMC or NMI IPIs.
> +     */
> +    env->spr[SPR_SRR1] |= PPC_BIT(43);

So we could have used the skiboot SPR_SRR1_PM_WAKE_SRESET define ? 

> +}
> +
> +static void pnv_nmi(NMIState *n, int cpu_index, Error **errp)
> +{
> +    CPUState *cs;
> +
> +    CPU_FOREACH(cs) {
> +        async_run_on_cpu(cs, pnv_cpu_do_nmi_on_cpu, RUN_ON_CPU_NULL);
> +    }
> +}
> +
>  static void pnv_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
>      InterruptStatsProviderClass *ispc = INTERRUPT_STATS_PROVIDER_CLASS(oc);
> +    NMIClass *nc = NMI_CLASS(oc);
> 
>      mc->desc = "IBM PowerNV (Non-Virtualized)";
>      mc->init = pnv_init;
> @@ -1975,6 +2002,7 @@ static void pnv_machine_class_init(ObjectClass *oc, void *data)
>      mc->default_ram_size = INITRD_LOAD_ADDR + INITRD_MAX_SIZE;
>      mc->default_ram_id = "pnv.ram";
>      ispc->print_info = pnv_pic_print_info;
> +    nc->nmi_monitor_handler = pnv_nmi;
> 
>      object_class_property_add_bool(oc, "hb-mode",
>                                     pnv_machine_get_hb, pnv_machine_set_hb,
> @@ -2038,7 +2066,7 @@ static const TypeInfo types[] = {
>          .class_size    = sizeof(PnvMachineClass),
>          .interfaces = (InterfaceInfo[]) {
>              { TYPE_INTERRUPT_STATS_PROVIDER },
> -            { },
> +            { TYPE_NMI },
>          },
>      },
>      {
> 

Re: [PATCH 2/5] ppc/pnv: Add support for NMI interface

[Nicholas Piggin]

Cédric Le Goater's on March 26, 2020 2:38 am:
> [ Please use clg@kaod.org ! ]
> 
> On 3/25/20 3:41 PM, Nicholas Piggin wrote:
>> This implements the NMI interface for the PNV machine, similarly to
>> commit 3431648272d ("spapr: Add support for new NMI interface") for
>> SPAPR.
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> 
> one minor comment,
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
>> ---
>>  hw/ppc/pnv.c | 30 +++++++++++++++++++++++++++++-
>>  1 file changed, 29 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index b75ad06390..671535ebe6 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -27,6 +27,7 @@
>>  #include "sysemu/runstate.h"
>>  #include "sysemu/cpus.h"
>>  #include "sysemu/device_tree.h"
>> +#include "sysemu/hw_accel.h"
>>  #include "target/ppc/cpu.h"
>>  #include "qemu/log.h"
>>  #include "hw/ppc/fdt.h"
>> @@ -34,6 +35,7 @@
>>  #include "hw/ppc/pnv.h"
>>  #include "hw/ppc/pnv_core.h"
>>  #include "hw/loader.h"
>> +#include "hw/nmi.h"
>>  #include "exec/address-spaces.h"
>>  #include "qapi/visitor.h"
>>  #include "monitor/monitor.h"
>> @@ -1955,10 +1957,35 @@ static void pnv_machine_set_hb(Object *obj, bool value, Error **errp)
>>      }
>>  }
>> 
>> +static void pnv_cpu_do_nmi_on_cpu(CPUState *cs, run_on_cpu_data arg)
>> +{
>> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
>> +    CPUPPCState *env = &cpu->env;
>> +
>> +    cpu_synchronize_state(cs);
>> +    ppc_cpu_do_system_reset(cs);
>> +    /*
>> +     * SRR1[42:45] is set to 0100 which the ISA defines as implementation
> 
> I see 'System Reset' in ISA 3.0
>> +     * dependent. POWER processors use this for xscom triggered interrupts,
>> +     * which come from the BMC or NMI IPIs.
>> +     */
>> +    env->spr[SPR_SRR1] |= PPC_BIT(43);
> 
> So we could have used the skiboot SPR_SRR1_PM_WAKE_SRESET define ? 

Ah, that's only for power-saving wakeup. But you got me to dig further
and I think I've got a few things wrong here.

The architectural power save wakeup due to sreset bit 43 needs to be
set, probably in excp_helper.c if (msr_pow) test.

    case POWERPC_EXCP_RESET:     /* System reset exception                   */
        /* A power-saving exception sets ME, otherwise it is unchanged */
        if (msr_pow) {
            /* indicate that we resumed from power save mode */
            msr |= 0x10000;
            new_msr |= ((target_ulong)1 << MSR_ME);
        }

For non-power save wakeup, it's all implementation defined. POWER9 UM 
has the table, but I got the damn bit wrong, I was probably looking in
the ISA table by mistake. It's bit 44 for that case. Linux doesn't tend 
to care about that case, but it does care about the power save wakeup
case, so this patch seems to generally "work", but I'll have to fix it.

Thanks,
Nick

Re: [PATCH 2/5] ppc/pnv: Add support for NMI interface

[Nicholas Piggin]

Nicholas Piggin's on April 3, 2020 5:57 pm:
> Cédric Le Goater's on March 26, 2020 2:38 am:
>> [ Please use clg@kaod.org ! ]
>> 
>> On 3/25/20 3:41 PM, Nicholas Piggin wrote:
>>> This implements the NMI interface for the PNV machine, similarly to
>>> commit 3431648272d ("spapr: Add support for new NMI interface") for
>>> SPAPR.
>>> 
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> 
>> one minor comment,
>> 
>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>> 
>>> ---
>>>  hw/ppc/pnv.c | 30 +++++++++++++++++++++++++++++-
>>>  1 file changed, 29 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>>> index b75ad06390..671535ebe6 100644
>>> --- a/hw/ppc/pnv.c
>>> +++ b/hw/ppc/pnv.c
>>> @@ -27,6 +27,7 @@
>>>  #include "sysemu/runstate.h"
>>>  #include "sysemu/cpus.h"
>>>  #include "sysemu/device_tree.h"
>>> +#include "sysemu/hw_accel.h"
>>>  #include "target/ppc/cpu.h"
>>>  #include "qemu/log.h"
>>>  #include "hw/ppc/fdt.h"
>>> @@ -34,6 +35,7 @@
>>>  #include "hw/ppc/pnv.h"
>>>  #include "hw/ppc/pnv_core.h"
>>>  #include "hw/loader.h"
>>> +#include "hw/nmi.h"
>>>  #include "exec/address-spaces.h"
>>>  #include "qapi/visitor.h"
>>>  #include "monitor/monitor.h"
>>> @@ -1955,10 +1957,35 @@ static void pnv_machine_set_hb(Object *obj, bool value, Error **errp)
>>>      }
>>>  }
>>> 
>>> +static void pnv_cpu_do_nmi_on_cpu(CPUState *cs, run_on_cpu_data arg)
>>> +{
>>> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
>>> +    CPUPPCState *env = &cpu->env;
>>> +
>>> +    cpu_synchronize_state(cs);
>>> +    ppc_cpu_do_system_reset(cs);
>>> +    /*
>>> +     * SRR1[42:45] is set to 0100 which the ISA defines as implementation
>> 
>> I see 'System Reset' in ISA 3.0
>>> +     * dependent. POWER processors use this for xscom triggered interrupts,
>>> +     * which come from the BMC or NMI IPIs.
>>> +     */
>>> +    env->spr[SPR_SRR1] |= PPC_BIT(43);
>> 
>> So we could have used the skiboot SPR_SRR1_PM_WAKE_SRESET define ? 
> 
> Ah, that's only for power-saving wakeup. But you got me to dig further
> and I think I've got a few things wrong here.
> 
> The architectural power save wakeup due to sreset bit 43 needs to be
> set, probably in excp_helper.c if (msr_pow) test.
> 
>     case POWERPC_EXCP_RESET:     /* System reset exception                   */
>         /* A power-saving exception sets ME, otherwise it is unchanged */
>         if (msr_pow) {
>             /* indicate that we resumed from power save mode */
>             msr |= 0x10000;
>             new_msr |= ((target_ulong)1 << MSR_ME);
>         }

Sorry I'm wrong, that's the old MSR_POW thing I guess G5 had it.

'stop' state wakeup is powerpc_reset_wakeup of course, which seems to
do the right thing with SRR1.

Something like this patch should fix this issue in the ppc-5.1 branch.
This appears to do the right thing with SRR1 in testing with Linux.

---
 hw/ppc/pnv.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index ac83b8698b..596ccfd99e 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1964,12 +1964,21 @@ static void pnv_cpu_do_nmi_on_cpu(CPUState *cs, run_on_cpu_data arg)
 
     cpu_synchronize_state(cs);
     ppc_cpu_do_system_reset(cs);
-    /*
-     * SRR1[42:45] is set to 0100 which the ISA defines as implementation
-     * dependent. POWER processors use this for xscom triggered interrupts,
-     * which come from the BMC or NMI IPIs.
-     */
-    env->spr[SPR_SRR1] |= PPC_BIT(43);
+    if (env->spr[SPR_SRR1] & PPC_BITMASK(46, 47)) {
+        /* system reset caused wake from power saving state */
+        if (!(env->spr[SPR_SRR1] & PPC_BIT(43))) {
+            warn_report("ppc_cpu_do_system_reset does not set system reset wakeup reason");
+            env->spr[SPR_SRR1] |= PPC_BIT(43);
+        }
+    } else {
+        /*
+	 * For non-powersave wakeup system resets, SRR1[42:45] are defined to
+	 * be implementation dependent. Set to 0b0010, which POWER9 UM defines
+	 * to be interrupt caused by SCOM, which can come from the BMC or NMI
+	 * IPIs.
+         */
+        env->spr[SPR_SRR1] |= PPC_BIT(44);
+    }
 }
 
 static void pnv_nmi(NMIState *n, int cpu_index, Error **errp)
-- 
2.23.0

Re: [PATCH 2/5] ppc/pnv: Add support for NMI interface

[Cédric Le Goater]

On 4/3/20 3:12 PM, Nicholas Piggin wrote:
> Nicholas Piggin's on April 3, 2020 5:57 pm:
>> Cédric Le Goater's on March 26, 2020 2:38 am:
>>> [ Please use clg@kaod.org ! ]
>>>
>>> On 3/25/20 3:41 PM, Nicholas Piggin wrote:
>>>> This implements the NMI interface for the PNV machine, similarly to
>>>> commit 3431648272d ("spapr: Add support for new NMI interface") for
>>>> SPAPR.
>>>>
>>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>
>>> one minor comment,
>>>
>>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>>
>>>> ---
>>>>  hw/ppc/pnv.c | 30 +++++++++++++++++++++++++++++-
>>>>  1 file changed, 29 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>>>> index b75ad06390..671535ebe6 100644
>>>> --- a/hw/ppc/pnv.c
>>>> +++ b/hw/ppc/pnv.c
>>>> @@ -27,6 +27,7 @@
>>>>  #include "sysemu/runstate.h"
>>>>  #include "sysemu/cpus.h"
>>>>  #include "sysemu/device_tree.h"
>>>> +#include "sysemu/hw_accel.h"
>>>>  #include "target/ppc/cpu.h"
>>>>  #include "qemu/log.h"
>>>>  #include "hw/ppc/fdt.h"
>>>> @@ -34,6 +35,7 @@
>>>>  #include "hw/ppc/pnv.h"
>>>>  #include "hw/ppc/pnv_core.h"
>>>>  #include "hw/loader.h"
>>>> +#include "hw/nmi.h"
>>>>  #include "exec/address-spaces.h"
>>>>  #include "qapi/visitor.h"
>>>>  #include "monitor/monitor.h"
>>>> @@ -1955,10 +1957,35 @@ static void pnv_machine_set_hb(Object *obj, bool value, Error **errp)
>>>>      }
>>>>  }
>>>>
>>>> +static void pnv_cpu_do_nmi_on_cpu(CPUState *cs, run_on_cpu_data arg)
>>>> +{
>>>> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
>>>> +    CPUPPCState *env = &cpu->env;
>>>> +
>>>> +    cpu_synchronize_state(cs);
>>>> +    ppc_cpu_do_system_reset(cs);
>>>> +    /*
>>>> +     * SRR1[42:45] is set to 0100 which the ISA defines as implementation
>>>
>>> I see 'System Reset' in ISA 3.0
>>>> +     * dependent. POWER processors use this for xscom triggered interrupts,
>>>> +     * which come from the BMC or NMI IPIs.
>>>> +     */
>>>> +    env->spr[SPR_SRR1] |= PPC_BIT(43);
>>>
>>> So we could have used the skiboot SPR_SRR1_PM_WAKE_SRESET define ? 
>>
>> Ah, that's only for power-saving wakeup. But you got me to dig further
>> and I think I've got a few things wrong here.
>>
>> The architectural power save wakeup due to sreset bit 43 needs to be
>> set, probably in excp_helper.c if (msr_pow) test.
>>
>>     case POWERPC_EXCP_RESET:     /* System reset exception                   */
>>         /* A power-saving exception sets ME, otherwise it is unchanged */
>>         if (msr_pow) {
>>             /* indicate that we resumed from power save mode */
>>             msr |= 0x10000;
>>             new_msr |= ((target_ulong)1 << MSR_ME);
>>         }
> 
> Sorry I'm wrong, that's the old MSR_POW thing I guess G5 had it.
> 
> 'stop' state wakeup is powerpc_reset_wakeup of course, which seems to
> do the right thing with SRR1.
> 
> Something like this patch should fix this issue in the ppc-5.1 branch.
> This appears to do the right thing with SRR1 in testing with Linux.
> 
> ---
>  hw/ppc/pnv.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index ac83b8698b..596ccfd99e 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1964,12 +1964,21 @@ static void pnv_cpu_do_nmi_on_cpu(CPUState *cs, run_on_cpu_data arg)
>  
>      cpu_synchronize_state(cs);
>      ppc_cpu_do_system_reset(cs);
> -    /*
> -     * SRR1[42:45] is set to 0100 which the ISA defines as implementation
> -     * dependent. POWER processors use this for xscom triggered interrupts,
> -     * which come from the BMC or NMI IPIs.
> -     */
> -    env->spr[SPR_SRR1] |= PPC_BIT(43);
> +    if (env->spr[SPR_SRR1] & PPC_BITMASK(46, 47)) {
> +        /* system reset caused wake from power saving state */
> +        if (!(env->spr[SPR_SRR1] & PPC_BIT(43))) {
> +            warn_report("ppc_cpu_do_system_reset does not set system reset wakeup reason");
> +            env->spr[SPR_SRR1] |= PPC_BIT(43);
> +        }
> +    } else {
> +        /*
> +	 * For non-powersave wakeup system resets, SRR1[42:45] are defined to
> +	 * be implementation dependent. Set to 0b0010, which POWER9 UM defines
> +	 * to be interrupt caused by SCOM, which can come from the BMC or NMI
> +	 * IPIs.
> +         */
> +        env->spr[SPR_SRR1] |= PPC_BIT(44);
> +    }
>  }

That looks correct according to the UM.

Do we care about the other non-powersave wakeup system resets ? 

  0011 Interrupt caused by hypervisor door bell.
  0101 Interrupt caused by privileged door bell.

The reason is that I sometime see CPU lockups under load, or with a KVM guest, 
and I haven't found why yet.

Thanks,


C. 


[  259.069436] virbr0: port 2(tap0) entered forwarding state
[  259.069523] virbr0: topology change detected, propagating
[  384.427337] watchdog: CPU 1 detected hard LOCKUP on other CPUs 0
[  384.428566] watchdog: CPU 1 TB:255514422948, last SMP heartbeat TB:246648941120 (17315ms ago)
[  384.528958] watchdog: CPU 0 Hard LOCKUP
[  384.529380] watchdog: CPU 0 TB:255566522003, last heartbeat TB:246648941120 (17417ms ago)
[  384.529879] Modules linked in: vhost_net vhost xt_CHECKSUM xt_MASQUERADE xt_conntrack ipt_REJECT nf_reject_ipv4 xt_tcpudp ip6table_mangle ip6table_nat iptable_mangle iptable_nat nf_nat nf_conntrack nf_defrag_ipv6 libcrc32c nf_defrag_ipv4 nf_tables nfnetlink ip6table_filter ip6_tables iptable_filter bpfilter bridge stp llc vmx_crypto crct10dif_vpmsum crc32c_vpmsum uio_pdrv_genirq uio kvm_hv kvm sch_fq_codel ip_tables x_tables autofs4 ipr e1000e ahci libahci
[  384.530400] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.6.0-rc5+ #31
[  384.530473] NIP:  c00000000000a93c LR: c00000000001b944 CTR: 00007094cb3d9680
[  384.530522] REGS: c000000001a47a40 TRAP: 0e81   Not tainted  (5.6.0-rc5+)
[  384.530532] MSR:  9000000000009033 <SF,HV,EE,ME,IR,DR,RI,LE>  CR: 48000242  XER: 00000000
[  384.530649] CFAR: 00000ac5e544b424 IRQMASK: 0 
               GPR00: c000000000c03620 c000000001a47cd0 c000000001a48f00 0000000028000000 
               GPR04: 00000000682a0000 0000003b80f29024 0000003ea9382e20 c000000001cf0000 
               GPR08: 0000000200000000 00000ac5e55200a0 0000000000000000 00000ac61eb65648 
               GPR12: 0000000044022444 00007094cb5ddcc0 
[  384.531164] NIP [c00000000000a93c] replay_interrupt_return+0x0/0x4
[  384.531203] LR [c00000000001b944] arch_local_irq_restore.part.0+0x54/0x70
[  384.531305] Call Trace:
[  384.531360] [c000000001a47cd0] [c000000001a47d50] init_stack+0x3d50/0x4000 (unreliable)
[  384.531450] [c000000001a47cf0] [c000000000c03620] cpuidle_enter_state+0xf0/0x590
[  384.531471] [c000000001a47d70] [c000000000c03b5c] cpuidle_enter+0x4c/0x70
[  384.531503] [c000000001a47db0] [c00000000018c7cc] call_cpuidle+0x4c/0x90
[  384.531514] [c000000001a47dd0] [c00000000018cde8] do_idle+0x308/0x420
[  384.531523] [c000000001a47e70] [c00000000018d148] cpu_startup_entry+0x38/0x40
[  384.531531] [c000000001a47ea0] [c000000000010800] rest_init+0xe0/0xf8
[  384.531543] [c000000001a47ed0] [c00000000130445c] start_kernel+0x7c4/0x810
[  384.531553] [c000000001a47f90] [c00000000000accc] start_here_common+0x1c/0x3e8
[  384.531610] Instruction dump:
[  384.531782] 7d200026 618c8000 2c030900 4182e778 2c030500 4182f3b0 2c030f00 4182f4c8 
[  384.531821] 2c030a00 4182ffbc 2c030e60 4182f158 <4e800020> 7c781b78 48000329 48000341 
[  388.127242] rcu: INFO: rcu_sched detected stalls on CPUs/tasks:
[  388.128093] rcu: 	0-...!: (0 ticks this GP) idle=de0/0/0x0 softirq=40880/40880 fqs=0 
[  388.128760] 	(detected by 1, t=5252 jiffies, g=61401, q=9509)
[  388.128967] Sending NMI from CPU 1 to CPUs 0:
[  388.124766] NMI backtrace for cpu 0
[  388.124789] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.6.0-rc5+ #31
[  388.124799] NIP:  c00000000000a93c LR: c00000000001b944 CTR: 00007094cb3d9680
[  388.124807] REGS: c000000001a47a40 TRAP: 0e81   Not tainted  (5.6.0-rc5+)
[  388.124811] MSR:  9000000000009033 <SF,HV,EE,ME,IR,DR,RI,LE>  CR: 48000242  XER: 00000000
[  388.124839] CFAR: 00000ac5e544b424 IRQMASK: 0 
               GPR00: c000000000c03620 c000000001a47cd0 c000000001a48f00 0000000028000000 
               GPR04: 00000000682a0000 0000003beeaf96c3 0000003ea9382e20 c000000001cf0000 
               GPR08: 0000000200000000 00000ac5e55200a0 0000000000000000 00000ac61eb65648 
               GPR12: 0000000044022444 00007094cb5ddcc0 
[  388.124894] NIP [c00000000000a93c] replay_interrupt_return+0x0/0x4
[  388.124904] LR [c00000000001b944] arch_local_irq_restore.part.0+0x54/0x70
[  388.124909] Call Trace:
[  388.124920] [c000000001a47cd0] [c000000001a47d50] init_stack+0x3d50/0x4000 (unreliable)
[  388.124935] [c000000001a47cf0] [c000000000c03620] cpuidle_enter_state+0xf0/0x590
[  388.124946] [c000000001a47d70] [c000000000c03b5c] cpuidle_enter+0x4c/0x70
[  388.124959] [c000000001a47db0] [c00000000018c7cc] call_cpuidle+0x4c/0x90
[  388.124970] [c000000001a47dd0] [c00000000018cde8] do_idle+0x308/0x420
[  388.124981] [c000000001a47e70] [c00000000018d148] cpu_startup_entry+0x38/0x40
[  388.124991] [c000000001a47ea0] [c000000000010800] rest_init+0xe0/0xf8
[  388.125002] [c000000001a47ed0] [c00000000130445c] start_kernel+0x7c4/0x810
[  388.125013] [c000000001a47f90] [c00000000000accc] start_here_common+0x1c/0x3e8
[  388.125020] Instruction dump:
[  388.125031] 7d200026 618c8000 2c030900 4182e778 2c030500 4182f3b0 2c030f00 4182f4c8 
[  388.125057] 2c030a00 4182ffbc 2c030e60 4182f158 <4e800020> 7c781b78 48000329 48000341 
[  388.129812] rcu: rcu_sched kthread starved for 5252 jiffies! g61401 f0x0 RCU_GP_WAIT_FQS(5) ->state=0x402 ->cpu=0
[  388.130357] rcu: RCU grace-period kthread stack dump:
[  388.130752] rcu_sched       I    0    10      2 0x00000808
[  388.130829] Call Trace:
[  388.131100] [c000000068487890] [c000000068487940] 0xc000000068487940 (unreliable)
[  388.131135] [c000000068487a70] [c000000000021c6c] __switch_to+0x2dc/0x450
[  388.131151] [c000000068487ae0] [c000000000e98a14] __schedule+0x2d4/0x920
[  388.131162] [c000000068487bb0] [c000000000e990d4] schedule+0x74/0x140
[  388.131173] [c000000068487be0] [c000000000e9ed24] schedule_timeout+0x1a4/0x3e0
[  388.131186] [c000000068487cc0] [c0000000001eac88] rcu_gp_kthread+0x788/0xd00
[  388.131197] [c000000068487db0] [c000000000170744] kthread+0x1a4/0x1b0
[  388.131210] [c000000068487e20] [c00000000000b648] ret_from_kernel_thread+0x5c/0x74
[  451.146090] rcu: INFO: rcu_sched detected stalls on CPUs/tasks:
[  451.147979] rcu: 	0-...!: (0 ticks this GP) idle=e30/0/0x0 softirq=40880/40880 fqs=1 
[  451.150009] 	(detected by 1, t=21007 jiffies, g=61401, q=9531)
[  451.150097] Sending NMI from CPU 1 to CPUs 0:
[  451.145933] NMI backtrace for cpu 0
[  451.146028] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.6.0-rc5+ #31
[  451.146095] NIP:  c00000000000a93c LR: c00000000001b944 CTR: c0000000000b9780
[  451.146126] REGS: c000000001a47a40 TRAP: 0e81   Not tainted  (5.6.0-rc5+)
[  451.146139] MSR:  9000000000009033 <SF,HV,EE,ME,IR,DR,RI,LE>  CR: 48000242  XER: 00000000
[  451.146233] CFAR: c0000000000b89a0 IRQMASK: 0 
               GPR00: c000000000c03620 c000000001a47cd0 c000000001a48f00 0000000028000000 
               GPR04: 00000000682a0000 0000004371f05548 0000004659916004 c000000001cf0000 
               GPR08: 0000000200000000 c00a001080060031 0000000000000000 c000000001cb8f00 
               GPR12: 0000000000008000 c000000001cf0000 
[  451.146433] NIP [c00000000000a93c] replay_interrupt_return+0x0/0x4
[  451.146471] LR [c00000000001b944] arch_local_irq_restore.part.0+0x54/0x70
[  451.146492] Call Trace:
[  451.146540] [c000000001a47cd0] [c000000001a47d50] init_stack+0x3d50/0x4000 (unreliable)
[  451.146603] [c000000001a47cf0] [c000000000c03620] cpuidle_enter_state+0xf0/0x590
[  451.146636] [c000000001a47d70] [c000000000c03b5c] cpuidle_enter+0x4c/0x70
[  451.146686] [c000000001a47db0] [c00000000018c7cc] call_cpuidle+0x4c/0x90
[  451.146722] [c000000001a47dd0] [c00000000018cde8] do_idle+0x308/0x420
[  451.146757] [c000000001a47e70] [c00000000018d14c] cpu_startup_entry+0x3c/0x40
[  451.146786] [c000000001a47ea0] [c000000000010800] rest_init+0xe0/0xf8
[  451.146834] [c000000001a47ed0] [c00000000130445c] start_kernel+0x7c4/0x810
[  451.146871] [c000000001a47f90] [c00000000000accc] start_here_common+0x1c/0x3e8
[  451.146894] Instruction dump:
[  451.146933] 7d200026 618c8000 2c030900 4182e778 2c030500 4182f3b0 2c030f00 4182f4c8 
[  451.146998] 2c030a00 4182ffbc 2c030e60 4182f158 <4e800020> 7c781b78 48000329 48000341 
[  451.151652] rcu: rcu_sched kthread starved for 15754 jiffies! g61401 f0x0 RCU_GP_WAIT_FQS(5) ->state=0x402 ->cpu=0
[  451.154297] rcu: RCU grace-period kthread stack dump:
[  451.155712] rcu_sched       I    0    10      2 0x00000808
[  451.155782] Call Trace:
[  451.155861] [c000000068487890] [c000000068487940] 0xc000000068487940 (unreliable)
[  451.155950] [c000000068487a70] [c000000000021c6c] __switch_to+0x2dc/0x450
[  451.155999] [c000000068487ae0] [c000000000e98a14] __schedule+0x2d4/0x920
[  451.156029] [c000000068487bb0] [c000000000e990d4] schedule+0x74/0x140
[  451.156061] [c000000068487be0] [c000000000e9ed24] schedule_timeout+0x1a4/0x3e0
[  451.156103] [c000000068487cc0] [c0000000001eac88] rcu_gp_kthread+0x788/0xd00
[  451.156135] [c000000068487db0] [c000000000170744] kthread+0x1a4/0x1b0
[  451.156177] [c000000068487e20] [c00000000000b648] ret_from_kernel_thread+0x5c/0x74
[  496.617376] rcu: INFO: rcu_sched detected stalls on CPUs/tasks:
[  496.617727] rcu: 	0-...!: (0 ticks this GP) idle=e80/0/0x0 softirq=40880/40880 fqs=0 
[  496.618063] 	(detected by 1, t=5252 jiffies, g=61437, q=7149)
[  496.618085] Sending NMI from CPU 1 to CPUs 0:
[  496.613805] NMI backtrace for cpu 0
[  496.613833] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.6.0-rc5+ #31
[  496.613844] NIP:  c00000000000a93c LR: c00000000001b944 CTR: c0000000005028c0
[  496.613852] REGS: c000000001a47a40 TRAP: 0e81   Not tainted  (5.6.0-rc5+)
[  496.613856] MSR:  9000000000009033 <SF,HV,EE,ME,IR,DR,RI,LE>  CR: 48000242  XER: 00000000
[  496.613884] CFAR: c000000000ea0310 IRQMASK: 0 
               GPR00: c000000000c03620 c000000001a47cd0 c000000001a48f00 0000000028000000 
               GPR04: 00000000682a0000 00000048dd82a1dd 0000004a31bd97db c000000001cf0000 
               GPR08: 0000000200000000 0000000000000000 00007fffef5c14a0 0800000042546c02 
               GPR12: 0000000000002000 c000000001cf0000 
[  496.613941] NIP [c00000000000a93c] replay_interrupt_return+0x0/0x4
[  496.613951] LR [c00000000001b944] arch_local_irq_restore.part.0+0x54/0x70
[  496.613956] Call Trace:
[  496.613970] [c000000001a47cd0] [c000000001a47d50] init_stack+0x3d50/0x4000 (unreliable)
[  496.613988] [c000000001a47cf0] [c000000000c03620] cpuidle_enter_state+0xf0/0x590
[  496.613998] [c000000001a47d70] [c000000000c03b5c] cpuidle_enter+0x4c/0x70
[  496.614011] [c000000001a47db0] [c00000000018c7cc] call_cpuidle+0x4c/0x90
[  496.614021] [c000000001a47dd0] [c00000000018cde8] do_idle+0x308/0x420
[  496.614030] [c000000001a47e70] [c00000000018d148] cpu_startup_entry+0x38/0x40
[  496.614038] [c000000001a47ea0] [c000000000010800] rest_init+0xe0/0xf8
[  496.614060] [c000000001a47ed0] [c00000000130445c] start_kernel+0x7c4/0x810
[  496.614071] [c000000001a47f90] [c00000000000accc] start_here_common+0x1c/0x3e8
[  496.614078] Instruction dump:
[  496.614091] 7d200026 618c8000 2c030900 4182e778 2c030500 4182f3b0 2c030f00 4182f4c8 
[  496.614109] 2c030a00 4182ffbc 2c030e60 4182f158 <4e800020> 7c781b78 48000329 48000341 
[  496.618540] rcu: rcu_sched kthread starved for 5252 jiffies! g61437 f0x0 RCU_GP_WAIT_FQS(5) ->state=0x402 ->cpu=0
[  496.618949] rcu: RCU grace-period kthread stack dump:
[  496.619169] rcu_sched       I    0    10      2 0x00000808
[  496.619184] Call Trace:
[  496.619207] [c000000068487890] [c0000000684878c0] 0xc0000000684878c0 (unreliable)
[  496.619231] [c000000068487a70] [c000000000021c6c] __switch_to+0x2dc/0x450
[  496.619244] [c000000068487ae0] [c000000000e98a14] __schedule+0x2d4/0x920
[  496.619252] [c000000068487bb0] [c000000000e990d4] schedule+0x74/0x140
[  496.619260] [c000000068487be0] [c000000000e9ed24] schedule_timeout+0x1a4/0x3e0
[  496.619271] [c000000068487cc0] [c0000000001eac88] rcu_gp_kthread+0x788/0xd00
[  496.619280] [c000000068487db0] [c000000000170744] kthread+0x1a4/0x1b0
[  496.619291] [c000000068487e20] [c00000000000b648] ret_from_kernel_thread+0x5c/0x74


(qemu) info registers 
NIP c00000000003c95c   LR c0000000000c40c4 CTR c000000000c083f0 XER 0000000000000000 CPU#0
MSR 9000000002803033 HID0 0880000000000000  HF 9000000002802031 iidx 5 didx 5
TB 00000079 342952940705 DECR 466647440485
GPR00 c0000000000c4560 c000000001a47b80 c000000001a48f00 0000000000300331
GPR04 c0000000000c40c4 0000000088000242 0000000000000008 c0000000019c8c00
GPR08 c000000001a88f00 0000000000300331 0000000000000000 ffffffffffffffff
GPR12 c000000000c083f0 c000000001cf0000 0000000000000001 c000000000000000
GPR16 0000000000000001 c000000001af03b0 c000000001a81ff8 0000000000000000
GPR20 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR24 0000000000000000 0000000000000000 0000000000000000 4000000000000000
GPR28 ffffffffffffffff 0000000000000000 0000000080000000 0000000000000000
CR 88000242  [ L  L  -  -  -  E  G  E  ]             RES ffffffffffffffff
FPR00 0000000000000000 0000000000000000 0000000000000000 0000000000000000
FPR04 0000000000000000 0000000000000000 0000000000000000 0000000000000000
FPR08 0000000000000000 0000000000000000 0000000000000000 736f6d6570736575
FPR12 6727fe175ed234df 0000000000000000 0000000000000000 0000000000000000
FPR16 0000000000000000 0000000000000000 0000000000000000 0000000000000000
FPR20 0000000000000000 0000000000000000 0000000000000000 0000000000000000
FPR24 0000000000000000 0000000000000000 0000000000000000 0000000000000000
FPR28 0000000000000000 0000000000000000 0000000000000000 0000000000000000
FPSCR 0000000000000000
 SRR0 00007094cb412eac  SRR1 900000000280f033    PVR 00000000004e1200 VRSAVE ffffffffffffffff
SPRG0 0000000000000000 SPRG1 c000000001cf0000  SPRG2 c000000001cf0000  SPRG3 0000000000000000
SPRG4 0000000000000000 SPRG5 0000000000000000  SPRG6 0000000000000000  SPRG7 0000000000000000
HSRR0 c0000000007f2efc HSRR1 9000000000009033
 CFAR c0000000000c40c0
 LPCR 0040400001d2f012
 PTCR 00000000f7fe0004   DAR 00000ac61eb32280  DSISR 000000000a000000

(qemu) cpu 1
(qemu) info registers 
NIP c00000000003c95c   LR c0000000000c40c4 CTR c000000000c083f0 XER 0000000000000000 CPU#1
MSR 9000000000001033 HID0 0880000000000000  HF 9000000000000031 iidx 5 didx 5
TB 00000081 348632348722 DECR 18446744073709522216
GPR00 c0000000000c4560 c0000000684bbbe0 c000000001a48f00 0000000000300331
GPR04 c0000000000c40c4 0000000088004222 0000000000000808 c000000068479000
GPR08 c000000001a88f00 0000000000300331 0000000000000000 ffffffffffffffff
GPR12 c000000000c083f0 c0000000fffff300 0000000000000001 c000000000000000
GPR16 0000000000000001 c000000001af03b0 c000000001a81ff8 0000000000000000
GPR20 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR24 0000000000000000 0000000000000000 0000000000000000 4000000000000000
GPR28 ffffffffffffffff 0000000000000000 0000000080000000 0000000000000000
CR 88004222  [ L  L  -  -  G  E  E  E  ]             RES ffffffffffffffff
FPR00 0000000000000000 0000000000000000 0000000000000000 0000000000000000
FPR04 0000000000000000 0000000000000000 0000000000000000 0000000000000000
FPR08 0000000000000000 0000000000000000 0000000000000000 0000000000000000
FPR12 0000000000000000 0000000000000000 0000000000000000 0000000000000000
FPR16 0000000000000000 0000000000000000 0000000000000000 0000000000000000
FPR20 0000000000000000 0000000000000000 0000000000000000 0000000000000000
FPR24 0000000000000000 0000000000000000 0000000000000000 0000000000000000
FPR28 0000000000000000 0000000000000000 0000000000000000 0000000000000000
FPSCR 0000000000000000
 SRR0 c00000000000a93c  SRR1 9000000000009033    PVR 00000000004e1200 VRSAVE 0000000000000000
SPRG0 0000000000000000 SPRG1 c000000001cf0000  SPRG2 c000000001cf0000  SPRG3 0000000000000001
SPRG4 0000000000000000 SPRG5 0000000000000000  SPRG6 0000000000000000  SPRG7 0000000000000000
HSRR0 c0000000000c1168 HSRR1 9000000000001033
 CFAR c0000000000c40c0
 LPCR 0040400001d2f012
 PTCR 00000000f7fe0004   DAR 00007be52be83320  DSISR 000000000a000000

Re: [PATCH 2/5] ppc/pnv: Add support for NMI interface

[Nicholas Piggin]

Cédric Le Goater's on April 4, 2020 1:47 am:
> On 4/3/20 3:12 PM, Nicholas Piggin wrote:
>> Nicholas Piggin's on April 3, 2020 5:57 pm:
>>> Cédric Le Goater's on March 26, 2020 2:38 am:
>>>> [ Please use clg@kaod.org ! ]
>>>>
>>>> On 3/25/20 3:41 PM, Nicholas Piggin wrote:
>>>>> This implements the NMI interface for the PNV machine, similarly to
>>>>> commit 3431648272d ("spapr: Add support for new NMI interface") for
>>>>> SPAPR.
>>>>>
>>>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>>
>>>> one minor comment,
>>>>
>>>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>>>
>>>>> ---
>>>>>  hw/ppc/pnv.c | 30 +++++++++++++++++++++++++++++-
>>>>>  1 file changed, 29 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>>>>> index b75ad06390..671535ebe6 100644
>>>>> --- a/hw/ppc/pnv.c
>>>>> +++ b/hw/ppc/pnv.c
>>>>> @@ -27,6 +27,7 @@
>>>>>  #include "sysemu/runstate.h"
>>>>>  #include "sysemu/cpus.h"
>>>>>  #include "sysemu/device_tree.h"
>>>>> +#include "sysemu/hw_accel.h"
>>>>>  #include "target/ppc/cpu.h"
>>>>>  #include "qemu/log.h"
>>>>>  #include "hw/ppc/fdt.h"
>>>>> @@ -34,6 +35,7 @@
>>>>>  #include "hw/ppc/pnv.h"
>>>>>  #include "hw/ppc/pnv_core.h"
>>>>>  #include "hw/loader.h"
>>>>> +#include "hw/nmi.h"
>>>>>  #include "exec/address-spaces.h"
>>>>>  #include "qapi/visitor.h"
>>>>>  #include "monitor/monitor.h"
>>>>> @@ -1955,10 +1957,35 @@ static void pnv_machine_set_hb(Object *obj, bool value, Error **errp)
>>>>>      }
>>>>>  }
>>>>>
>>>>> +static void pnv_cpu_do_nmi_on_cpu(CPUState *cs, run_on_cpu_data arg)
>>>>> +{
>>>>> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
>>>>> +    CPUPPCState *env = &cpu->env;
>>>>> +
>>>>> +    cpu_synchronize_state(cs);
>>>>> +    ppc_cpu_do_system_reset(cs);
>>>>> +    /*
>>>>> +     * SRR1[42:45] is set to 0100 which the ISA defines as implementation
>>>>
>>>> I see 'System Reset' in ISA 3.0
>>>>> +     * dependent. POWER processors use this for xscom triggered interrupts,
>>>>> +     * which come from the BMC or NMI IPIs.
>>>>> +     */
>>>>> +    env->spr[SPR_SRR1] |= PPC_BIT(43);
>>>>
>>>> So we could have used the skiboot SPR_SRR1_PM_WAKE_SRESET define ? 
>>>
>>> Ah, that's only for power-saving wakeup. But you got me to dig further
>>> and I think I've got a few things wrong here.
>>>
>>> The architectural power save wakeup due to sreset bit 43 needs to be
>>> set, probably in excp_helper.c if (msr_pow) test.
>>>
>>>     case POWERPC_EXCP_RESET:     /* System reset exception                   */
>>>         /* A power-saving exception sets ME, otherwise it is unchanged */
>>>         if (msr_pow) {
>>>             /* indicate that we resumed from power save mode */
>>>             msr |= 0x10000;
>>>             new_msr |= ((target_ulong)1 << MSR_ME);
>>>         }
>> 
>> Sorry I'm wrong, that's the old MSR_POW thing I guess G5 had it.
>> 
>> 'stop' state wakeup is powerpc_reset_wakeup of course, which seems to
>> do the right thing with SRR1.
>> 
>> Something like this patch should fix this issue in the ppc-5.1 branch.
>> This appears to do the right thing with SRR1 in testing with Linux.
>> 
>> ---
>>  hw/ppc/pnv.c | 21 +++++++++++++++------
>>  1 file changed, 15 insertions(+), 6 deletions(-)
>> 
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index ac83b8698b..596ccfd99e 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -1964,12 +1964,21 @@ static void pnv_cpu_do_nmi_on_cpu(CPUState *cs, run_on_cpu_data arg)
>>  
>>      cpu_synchronize_state(cs);
>>      ppc_cpu_do_system_reset(cs);
>> -    /*
>> -     * SRR1[42:45] is set to 0100 which the ISA defines as implementation
>> -     * dependent. POWER processors use this for xscom triggered interrupts,
>> -     * which come from the BMC or NMI IPIs.
>> -     */
>> -    env->spr[SPR_SRR1] |= PPC_BIT(43);
>> +    if (env->spr[SPR_SRR1] & PPC_BITMASK(46, 47)) {
>> +        /* system reset caused wake from power saving state */
>> +        if (!(env->spr[SPR_SRR1] & PPC_BIT(43))) {
>> +            warn_report("ppc_cpu_do_system_reset does not set system reset wakeup reason");
>> +            env->spr[SPR_SRR1] |= PPC_BIT(43);
>> +        }
>> +    } else {
>> +        /*
>> +	 * For non-powersave wakeup system resets, SRR1[42:45] are defined to
>> +	 * be implementation dependent. Set to 0b0010, which POWER9 UM defines
>> +	 * to be interrupt caused by SCOM, which can come from the BMC or NMI
>> +	 * IPIs.
>> +         */
>> +        env->spr[SPR_SRR1] |= PPC_BIT(44);
>> +    }
>>  }
> 
> That looks correct according to the UM.
> 
> Do we care about the other non-powersave wakeup system resets ? 
> 
>   0011 Interrupt caused by hypervisor door bell.
>   0101 Interrupt caused by privileged door bell.

I think that's a typo in the UM, and those are powersave ones.

> 
> The reason is that I sometime see CPU lockups under load, or with a KVM guest, 
> and I haven't found why yet.

I can't tell what's going on there, does it keep taking e80 interrupts
and never clear the doorbell message? Linux wakeup replay code has
always been solid.

Thanks,
Nick

Re: [PATCH 2/5] ppc/pnv: Add support for NMI interface

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3118 bytes --]

On Thu, Mar 26, 2020 at 12:41:44AM +1000, Nicholas Piggin wrote:
> This implements the NMI interface for the PNV machine, similarly to
> commit 3431648272d ("spapr: Add support for new NMI interface") for
> SPAPR.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Applied to ppc-for-5.1.

> ---
>  hw/ppc/pnv.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index b75ad06390..671535ebe6 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -27,6 +27,7 @@
>  #include "sysemu/runstate.h"
>  #include "sysemu/cpus.h"
>  #include "sysemu/device_tree.h"
> +#include "sysemu/hw_accel.h"
>  #include "target/ppc/cpu.h"
>  #include "qemu/log.h"
>  #include "hw/ppc/fdt.h"
> @@ -34,6 +35,7 @@
>  #include "hw/ppc/pnv.h"
>  #include "hw/ppc/pnv_core.h"
>  #include "hw/loader.h"
> +#include "hw/nmi.h"
>  #include "exec/address-spaces.h"
>  #include "qapi/visitor.h"
>  #include "monitor/monitor.h"
> @@ -1955,10 +1957,35 @@ static void pnv_machine_set_hb(Object *obj, bool value, Error **errp)
>      }
>  }
>  
> +static void pnv_cpu_do_nmi_on_cpu(CPUState *cs, run_on_cpu_data arg)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
> +
> +    cpu_synchronize_state(cs);
> +    ppc_cpu_do_system_reset(cs);
> +    /*
> +     * SRR1[42:45] is set to 0100 which the ISA defines as implementation
> +     * dependent. POWER processors use this for xscom triggered interrupts,
> +     * which come from the BMC or NMI IPIs.
> +     */
> +    env->spr[SPR_SRR1] |= PPC_BIT(43);
> +}
> +
> +static void pnv_nmi(NMIState *n, int cpu_index, Error **errp)
> +{
> +    CPUState *cs;
> +
> +    CPU_FOREACH(cs) {
> +        async_run_on_cpu(cs, pnv_cpu_do_nmi_on_cpu, RUN_ON_CPU_NULL);
> +    }
> +}
> +
>  static void pnv_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
>      InterruptStatsProviderClass *ispc = INTERRUPT_STATS_PROVIDER_CLASS(oc);
> +    NMIClass *nc = NMI_CLASS(oc);
>  
>      mc->desc = "IBM PowerNV (Non-Virtualized)";
>      mc->init = pnv_init;
> @@ -1975,6 +2002,7 @@ static void pnv_machine_class_init(ObjectClass *oc, void *data)
>      mc->default_ram_size = INITRD_LOAD_ADDR + INITRD_MAX_SIZE;
>      mc->default_ram_id = "pnv.ram";
>      ispc->print_info = pnv_pic_print_info;
> +    nc->nmi_monitor_handler = pnv_nmi;
>  
>      object_class_property_add_bool(oc, "hb-mode",
>                                     pnv_machine_get_hb, pnv_machine_set_hb,
> @@ -2038,7 +2066,7 @@ static const TypeInfo types[] = {
>          .class_size    = sizeof(PnvMachineClass),
>          .interfaces = (InterfaceInfo[]) {
>              { TYPE_INTERRUPT_STATS_PROVIDER },
> -            { },
> +            { TYPE_NMI },
>          },
>      },
>      {

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/5] ppc/pnv: Add support for NMI interface

[Alexey Kardashevskiy]

On 26/03/2020 01:41, Nicholas Piggin wrote:
> This implements the NMI interface for the PNV machine, similarly to
> commit 3431648272d ("spapr: Add support for new NMI interface") for
> SPAPR.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  hw/ppc/pnv.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index b75ad06390..671535ebe6 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -27,6 +27,7 @@
>  #include "sysemu/runstate.h"
>  #include "sysemu/cpus.h"
>  #include "sysemu/device_tree.h"
> +#include "sysemu/hw_accel.h"
>  #include "target/ppc/cpu.h"
>  #include "qemu/log.h"
>  #include "hw/ppc/fdt.h"
> @@ -34,6 +35,7 @@
>  #include "hw/ppc/pnv.h"
>  #include "hw/ppc/pnv_core.h"
>  #include "hw/loader.h"
> +#include "hw/nmi.h"
>  #include "exec/address-spaces.h"
>  #include "qapi/visitor.h"
>  #include "monitor/monitor.h"
> @@ -1955,10 +1957,35 @@ static void pnv_machine_set_hb(Object *obj, bool value, Error **errp)
>      }
>  }
>  
> +static void pnv_cpu_do_nmi_on_cpu(CPUState *cs, run_on_cpu_data arg)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
> +
> +    cpu_synchronize_state(cs);
> +    ppc_cpu_do_system_reset(cs);
> +    /*
> +     * SRR1[42:45] is set to 0100 which the ISA defines as implementation
> +     * dependent. POWER processors use this for xscom triggered interrupts,
> +     * which come from the BMC or NMI IPIs.
> +     */
> +    env->spr[SPR_SRR1] |= PPC_BIT(43);
> +}
> +
> +static void pnv_nmi(NMIState *n, int cpu_index, Error **errp)
> +{
> +    CPUState *cs;
> +
> +    CPU_FOREACH(cs) {
> +        async_run_on_cpu(cs, pnv_cpu_do_nmi_on_cpu, RUN_ON_CPU_NULL);
> +    }
> +}
> +
>  static void pnv_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
>      InterruptStatsProviderClass *ispc = INTERRUPT_STATS_PROVIDER_CLASS(oc);
> +    NMIClass *nc = NMI_CLASS(oc);
>  
>      mc->desc = "IBM PowerNV (Non-Virtualized)";
>      mc->init = pnv_init;
> @@ -1975,6 +2002,7 @@ static void pnv_machine_class_init(ObjectClass *oc, void *data)
>      mc->default_ram_size = INITRD_LOAD_ADDR + INITRD_MAX_SIZE;
>      mc->default_ram_id = "pnv.ram";
>      ispc->print_info = pnv_pic_print_info;
> +    nc->nmi_monitor_handler = pnv_nmi;
>  
>      object_class_property_add_bool(oc, "hb-mode",
>                                     pnv_machine_get_hb, pnv_machine_set_hb,
> @@ -2038,7 +2066,7 @@ static const TypeInfo types[] = {
>          .class_size    = sizeof(PnvMachineClass),
>          .interfaces = (InterfaceInfo[]) {
>              { TYPE_INTERRUPT_STATS_PROVIDER },
> -            { },
> +            { TYPE_NMI },


The interface list must end with {}, otherwise QEMU crashes very early.
Thanks,


>          },
>      },
>      {
> 

-- 
Alexey

Re: [PATCH 2/5] ppc/pnv: Add support for NMI interface

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3458 bytes --]

On Tue, Mar 31, 2020 at 02:07:42PM +1100, Alexey Kardashevskiy wrote:
> 
> 
> On 26/03/2020 01:41, Nicholas Piggin wrote:
> > This implements the NMI interface for the PNV machine, similarly to
> > commit 3431648272d ("spapr: Add support for new NMI interface") for
> > SPAPR.
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >  hw/ppc/pnv.c | 30 +++++++++++++++++++++++++++++-
> >  1 file changed, 29 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > index b75ad06390..671535ebe6 100644
> > --- a/hw/ppc/pnv.c
> > +++ b/hw/ppc/pnv.c
> > @@ -27,6 +27,7 @@
> >  #include "sysemu/runstate.h"
> >  #include "sysemu/cpus.h"
> >  #include "sysemu/device_tree.h"
> > +#include "sysemu/hw_accel.h"
> >  #include "target/ppc/cpu.h"
> >  #include "qemu/log.h"
> >  #include "hw/ppc/fdt.h"
> > @@ -34,6 +35,7 @@
> >  #include "hw/ppc/pnv.h"
> >  #include "hw/ppc/pnv_core.h"
> >  #include "hw/loader.h"
> > +#include "hw/nmi.h"
> >  #include "exec/address-spaces.h"
> >  #include "qapi/visitor.h"
> >  #include "monitor/monitor.h"
> > @@ -1955,10 +1957,35 @@ static void pnv_machine_set_hb(Object *obj, bool value, Error **errp)
> >      }
> >  }
> >  
> > +static void pnv_cpu_do_nmi_on_cpu(CPUState *cs, run_on_cpu_data arg)
> > +{
> > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +    CPUPPCState *env = &cpu->env;
> > +
> > +    cpu_synchronize_state(cs);
> > +    ppc_cpu_do_system_reset(cs);
> > +    /*
> > +     * SRR1[42:45] is set to 0100 which the ISA defines as implementation
> > +     * dependent. POWER processors use this for xscom triggered interrupts,
> > +     * which come from the BMC or NMI IPIs.
> > +     */
> > +    env->spr[SPR_SRR1] |= PPC_BIT(43);
> > +}
> > +
> > +static void pnv_nmi(NMIState *n, int cpu_index, Error **errp)
> > +{
> > +    CPUState *cs;
> > +
> > +    CPU_FOREACH(cs) {
> > +        async_run_on_cpu(cs, pnv_cpu_do_nmi_on_cpu, RUN_ON_CPU_NULL);
> > +    }
> > +}
> > +
> >  static void pnv_machine_class_init(ObjectClass *oc, void *data)
> >  {
> >      MachineClass *mc = MACHINE_CLASS(oc);
> >      InterruptStatsProviderClass *ispc = INTERRUPT_STATS_PROVIDER_CLASS(oc);
> > +    NMIClass *nc = NMI_CLASS(oc);
> >  
> >      mc->desc = "IBM PowerNV (Non-Virtualized)";
> >      mc->init = pnv_init;
> > @@ -1975,6 +2002,7 @@ static void pnv_machine_class_init(ObjectClass *oc, void *data)
> >      mc->default_ram_size = INITRD_LOAD_ADDR + INITRD_MAX_SIZE;
> >      mc->default_ram_id = "pnv.ram";
> >      ispc->print_info = pnv_pic_print_info;
> > +    nc->nmi_monitor_handler = pnv_nmi;
> >  
> >      object_class_property_add_bool(oc, "hb-mode",
> >                                     pnv_machine_get_hb, pnv_machine_set_hb,
> > @@ -2038,7 +2066,7 @@ static const TypeInfo types[] = {
> >          .class_size    = sizeof(PnvMachineClass),
> >          .interfaces = (InterfaceInfo[]) {
> >              { TYPE_INTERRUPT_STATS_PROVIDER },
> > -            { },
> > +            { TYPE_NMI },
> 
> 
> The interface list must end with {}, otherwise QEMU crashes very early.
> Thanks,

I've fixed that inline now.

> 
> 
> >          },
> >      },
> >      {
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 3/5] nmi: add MCE class for implementing machine check injection commands

[Nicholas Piggin]

Like commit 9cb805fd26 ("cpus: Define callback for QEMU "nmi" command")
this implements a machine check injection command framework and defines
a monitor command for ppc.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hmp-commands.hx              | 20 +++++++++++-
 hw/core/nmi.c                | 61 ++++++++++++++++++++++++++++++++++++
 include/hw/nmi.h             | 20 ++++++++++++
 include/monitor/hmp-target.h |  1 -
 include/monitor/hmp.h        |  1 +
 monitor/hmp-cmds.c           |  1 +
 target/ppc/monitor.c         | 11 +++++++
 7 files changed, 113 insertions(+), 2 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 7f0f3974ad..4a9089b431 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1581,12 +1581,30 @@ ERST
         .cmd        = hmp_mce,
     },
 
-#endif
 SRST
 ``mce`` *cpu* *bank* *status* *mcgstatus* *addr* *misc*
   Inject an MCE on the given CPU (x86 only).
 ERST
 
+#endif
+
+#if defined(TARGET_PPC)
+
+    {
+        .name       = "mce",
+        .args_type  = "cpu_index:i,srr1_mask:l,dsisr:i,dar:l,recovered:i",
+        .params     = "cpu srr1_mask dsisr dar recovered",
+        .help       = "inject a MCE on the given CPU",
+        .cmd        = hmp_mce,
+    },
+
+SRST
+``mce`` *cpu* *srr1_mask* *dsisr* *dar* *recovered*
+  Inject an MCE on the given CPU (PPC only).
+ERST
+
+#endif
+
     {
         .name       = "getfd",
         .args_type  = "fdname:s",
diff --git a/hw/core/nmi.c b/hw/core/nmi.c
index 481c4b3c7e..2a79500967 100644
--- a/hw/core/nmi.c
+++ b/hw/core/nmi.c
@@ -86,3 +86,64 @@ static void nmi_register_types(void)
 }
 
 type_init(nmi_register_types)
+
+struct do_mce_s {
+    const QDict *qdict;
+    Error *err;
+    bool handled;
+};
+
+static void mce_children(Object *o, struct do_mce_s *ns);
+
+static int do_mce(Object *o, void *opaque)
+{
+    struct do_mce_s *ms = opaque;
+    MCEState *m = (MCEState *) object_dynamic_cast(o, TYPE_MCE);
+
+    if (m) {
+        MCEClass *mc = MCE_GET_CLASS(m);
+
+        ms->handled = true;
+        mc->mce_monitor_handler(m, ms->qdict, &ms->err);
+        if (ms->err) {
+            return -1;
+        }
+    }
+    mce_children(o, ms);
+
+    return 0;
+}
+
+static void mce_children(Object *o, struct do_mce_s *ms)
+{
+    object_child_foreach(o, do_mce, ms);
+}
+
+void mce_monitor_handle(const QDict *qdict, Error **errp)
+{
+    struct do_mce_s ms = {
+        .qdict = qdict,
+        .err = NULL,
+        .handled = false
+    };
+
+    mce_children(object_get_root(), &ms);
+    if (ms.handled) {
+        error_propagate(errp, ms.err);
+    } else {
+        error_setg(errp, QERR_UNSUPPORTED);
+    }
+}
+
+static const TypeInfo mce_info = {
+    .name          = TYPE_MCE,
+    .parent        = TYPE_INTERFACE,
+    .class_size    = sizeof(MCEClass),
+};
+
+static void mce_register_types(void)
+{
+    type_register_static(&mce_info);
+}
+
+type_init(mce_register_types)
diff --git a/include/hw/nmi.h b/include/hw/nmi.h
index fe37ce3ad8..de39d95c9a 100644
--- a/include/hw/nmi.h
+++ b/include/hw/nmi.h
@@ -43,4 +43,24 @@ typedef struct NMIClass {
 
 void nmi_monitor_handle(int cpu_index, Error **errp);
 
+
+#define TYPE_MCE "mce"
+
+#define MCE_CLASS(klass) \
+     OBJECT_CLASS_CHECK(MCEClass, (klass), TYPE_MCE)
+#define MCE_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(MCEClass, (obj), TYPE_MCE)
+#define MCE(obj) \
+     INTERFACE_CHECK(MCEState, (obj), TYPE_MCE)
+
+typedef struct MCEState MCEState;
+
+typedef struct MCEClass {
+    InterfaceClass parent_class;
+
+    void (*mce_monitor_handler)(MCEState *n, const QDict *qdict, Error **errp);
+} MCEClass;
+
+void mce_monitor_handle(const QDict *qdict, Error **errp);
+
 #endif /* NMI_H */
diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
index 8b7820a3ad..afb8f5bca2 100644
--- a/include/monitor/hmp-target.h
+++ b/include/monitor/hmp-target.h
@@ -45,7 +45,6 @@ CPUState *mon_get_cpu(void);
 
 void hmp_info_mem(Monitor *mon, const QDict *qdict);
 void hmp_info_tlb(Monitor *mon, const QDict *qdict);
-void hmp_mce(Monitor *mon, const QDict *qdict);
 void hmp_info_local_apic(Monitor *mon, const QDict *qdict);
 void hmp_info_io_apic(Monitor *mon, const QDict *qdict);
 
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index e33ca5a911..f747a5e214 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -54,6 +54,7 @@ void hmp_ringbuf_read(Monitor *mon, const QDict *qdict);
 void hmp_cont(Monitor *mon, const QDict *qdict);
 void hmp_system_wakeup(Monitor *mon, const QDict *qdict);
 void hmp_nmi(Monitor *mon, const QDict *qdict);
+void hmp_mce(Monitor *mon, const QDict *qdict);
 void hmp_set_link(Monitor *mon, const QDict *qdict);
 void hmp_balloon(Monitor *mon, const QDict *qdict);
 void hmp_loadvm(Monitor *mon, const QDict *qdict);
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 58724031ea..3664ef2a4f 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -52,6 +52,7 @@
 #include "exec/ramlist.h"
 #include "hw/intc/intc.h"
 #include "hw/rdma/rdma.h"
+#include "hw/nmi.h"
 #include "migration/snapshot.h"
 #include "migration/misc.h"
 
diff --git a/target/ppc/monitor.c b/target/ppc/monitor.c
index a5a177d717..6daf543efc 100644
--- a/target/ppc/monitor.c
+++ b/target/ppc/monitor.c
@@ -28,6 +28,8 @@
 #include "qemu/ctype.h"
 #include "monitor/hmp-target.h"
 #include "monitor/hmp.h"
+#include "qapi/qmp/qdict.h"
+#include "hw/nmi.h"
 
 static target_long monitor_get_ccr(const struct MonitorDef *md, int val)
 {
@@ -72,6 +74,15 @@ void hmp_info_tlb(Monitor *mon, const QDict *qdict)
     dump_mmu(env1);
 }
 
+void hmp_mce(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+
+    mce_monitor_handle(qdict, &err);
+
+    hmp_handle_error(mon, err);
+}
+
 const MonitorDef monitor_defs[] = {
     { "fpscr", offsetof(CPUPPCState, fpscr) },
     /* Next instruction pointer */
-- 
2.23.0

Re: [PATCH 3/5] nmi: add MCE class for implementing machine check injection commands

[Cédric Le Goater]

On 3/25/20 3:41 PM, Nicholas Piggin wrote:
> Like commit 9cb805fd26 ("cpus: Define callback for QEMU "nmi" command")
> this implements a machine check injection command framework and defines
> a monitor command for ppc.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Looks good to me,

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Tested-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C. 

> ---
>  hmp-commands.hx              | 20 +++++++++++-
>  hw/core/nmi.c                | 61 ++++++++++++++++++++++++++++++++++++
>  include/hw/nmi.h             | 20 ++++++++++++
>  include/monitor/hmp-target.h |  1 -
>  include/monitor/hmp.h        |  1 +
>  monitor/hmp-cmds.c           |  1 +
>  target/ppc/monitor.c         | 11 +++++++
>  7 files changed, 113 insertions(+), 2 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 7f0f3974ad..4a9089b431 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1581,12 +1581,30 @@ ERST
>          .cmd        = hmp_mce,
>      },
> 
> -#endif
>  SRST
>  ``mce`` *cpu* *bank* *status* *mcgstatus* *addr* *misc*
>    Inject an MCE on the given CPU (x86 only).
>  ERST
> 
> +#endif
> +
> +#if defined(TARGET_PPC)
> +
> +    {
> +        .name       = "mce",
> +        .args_type  = "cpu_index:i,srr1_mask:l,dsisr:i,dar:l,recovered:i",
> +        .params     = "cpu srr1_mask dsisr dar recovered",
> +        .help       = "inject a MCE on the given CPU",
> +        .cmd        = hmp_mce,
> +    },
> +
> +SRST
> +``mce`` *cpu* *srr1_mask* *dsisr* *dar* *recovered*
> +  Inject an MCE on the given CPU (PPC only).
> +ERST
> +
> +#endif
> +
>      {
>          .name       = "getfd",
>          .args_type  = "fdname:s",
> diff --git a/hw/core/nmi.c b/hw/core/nmi.c
> index 481c4b3c7e..2a79500967 100644
> --- a/hw/core/nmi.c
> +++ b/hw/core/nmi.c
> @@ -86,3 +86,64 @@ static void nmi_register_types(void)
>  }
> 
>  type_init(nmi_register_types)
> +
> +struct do_mce_s {
> +    const QDict *qdict;
> +    Error *err;
> +    bool handled;
> +};
> +
> +static void mce_children(Object *o, struct do_mce_s *ns);
> +
> +static int do_mce(Object *o, void *opaque)
> +{
> +    struct do_mce_s *ms = opaque;
> +    MCEState *m = (MCEState *) object_dynamic_cast(o, TYPE_MCE);
> +
> +    if (m) {
> +        MCEClass *mc = MCE_GET_CLASS(m);
> +
> +        ms->handled = true;
> +        mc->mce_monitor_handler(m, ms->qdict, &ms->err);
> +        if (ms->err) {
> +            return -1;
> +        }
> +    }
> +    mce_children(o, ms);
> +
> +    return 0;
> +}
> +
> +static void mce_children(Object *o, struct do_mce_s *ms)
> +{
> +    object_child_foreach(o, do_mce, ms);
> +}
> +
> +void mce_monitor_handle(const QDict *qdict, Error **errp)
> +{
> +    struct do_mce_s ms = {
> +        .qdict = qdict,
> +        .err = NULL,
> +        .handled = false
> +    };
> +
> +    mce_children(object_get_root(), &ms);
> +    if (ms.handled) {
> +        error_propagate(errp, ms.err);
> +    } else {
> +        error_setg(errp, QERR_UNSUPPORTED);
> +    }
> +}
> +
> +static const TypeInfo mce_info = {
> +    .name          = TYPE_MCE,
> +    .parent        = TYPE_INTERFACE,
> +    .class_size    = sizeof(MCEClass),
> +};
> +
> +static void mce_register_types(void)
> +{
> +    type_register_static(&mce_info);
> +}
> +
> +type_init(mce_register_types)
> diff --git a/include/hw/nmi.h b/include/hw/nmi.h
> index fe37ce3ad8..de39d95c9a 100644
> --- a/include/hw/nmi.h
> +++ b/include/hw/nmi.h
> @@ -43,4 +43,24 @@ typedef struct NMIClass {
> 
>  void nmi_monitor_handle(int cpu_index, Error **errp);
> 
> +
> +#define TYPE_MCE "mce"
> +
> +#define MCE_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(MCEClass, (klass), TYPE_MCE)
> +#define MCE_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(MCEClass, (obj), TYPE_MCE)
> +#define MCE(obj) \
> +     INTERFACE_CHECK(MCEState, (obj), TYPE_MCE)
> +
> +typedef struct MCEState MCEState;
> +
> +typedef struct MCEClass {
> +    InterfaceClass parent_class;
> +
> +    void (*mce_monitor_handler)(MCEState *n, const QDict *qdict, Error **errp);
> +} MCEClass;
> +
> +void mce_monitor_handle(const QDict *qdict, Error **errp);
> +
>  #endif /* NMI_H */
> diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
> index 8b7820a3ad..afb8f5bca2 100644
> --- a/include/monitor/hmp-target.h
> +++ b/include/monitor/hmp-target.h
> @@ -45,7 +45,6 @@ CPUState *mon_get_cpu(void);
> 
>  void hmp_info_mem(Monitor *mon, const QDict *qdict);
>  void hmp_info_tlb(Monitor *mon, const QDict *qdict);
> -void hmp_mce(Monitor *mon, const QDict *qdict);
>  void hmp_info_local_apic(Monitor *mon, const QDict *qdict);
>  void hmp_info_io_apic(Monitor *mon, const QDict *qdict);
> 
> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
> index e33ca5a911..f747a5e214 100644
> --- a/include/monitor/hmp.h
> +++ b/include/monitor/hmp.h
> @@ -54,6 +54,7 @@ void hmp_ringbuf_read(Monitor *mon, const QDict *qdict);
>  void hmp_cont(Monitor *mon, const QDict *qdict);
>  void hmp_system_wakeup(Monitor *mon, const QDict *qdict);
>  void hmp_nmi(Monitor *mon, const QDict *qdict);
> +void hmp_mce(Monitor *mon, const QDict *qdict);
>  void hmp_set_link(Monitor *mon, const QDict *qdict);
>  void hmp_balloon(Monitor *mon, const QDict *qdict);
>  void hmp_loadvm(Monitor *mon, const QDict *qdict);
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 58724031ea..3664ef2a4f 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -52,6 +52,7 @@
>  #include "exec/ramlist.h"
>  #include "hw/intc/intc.h"
>  #include "hw/rdma/rdma.h"
> +#include "hw/nmi.h"
>  #include "migration/snapshot.h"
>  #include "migration/misc.h"
> 
> diff --git a/target/ppc/monitor.c b/target/ppc/monitor.c
> index a5a177d717..6daf543efc 100644
> --- a/target/ppc/monitor.c
> +++ b/target/ppc/monitor.c
> @@ -28,6 +28,8 @@
>  #include "qemu/ctype.h"
>  #include "monitor/hmp-target.h"
>  #include "monitor/hmp.h"
> +#include "qapi/qmp/qdict.h"
> +#include "hw/nmi.h"
> 
>  static target_long monitor_get_ccr(const struct MonitorDef *md, int val)
>  {
> @@ -72,6 +74,15 @@ void hmp_info_tlb(Monitor *mon, const QDict *qdict)
>      dump_mmu(env1);
>  }
> 
> +void hmp_mce(Monitor *mon, const QDict *qdict)
> +{
> +    Error *err = NULL;
> +
> +    mce_monitor_handle(qdict, &err);
> +
> +    hmp_handle_error(mon, err);
> +}
> +
>  const MonitorDef monitor_defs[] = {
>      { "fpscr", offsetof(CPUPPCState, fpscr) },
>      /* Next instruction pointer */
> 

Re: [PATCH 3/5] nmi: add MCE class for implementing machine check injection commands

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 7371 bytes --]

On Thu, Mar 26, 2020 at 12:41:45AM +1000, Nicholas Piggin wrote:
> Like commit 9cb805fd26 ("cpus: Define callback for QEMU "nmi" command")
> this implements a machine check injection command framework and defines
> a monitor command for ppc.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

So, AFAICT, both x86 and ppc have something called an MCE, and while
I'm guessing they're somewhat related, they don't work quite the same
way - different information provided to the handler and so forth.

I think it's reasonable to overload the "mce" HMP command based on
target for the different types.  However, I think calling the
interface classes which are specific to the ppc type just "mce" could
be pretty confusing.

In addition, I think this is adding an HMP command to inject the event
without any corresponding way of injecting via QMP.  I believe that's
frowned upon.

> ---
>  hmp-commands.hx              | 20 +++++++++++-
>  hw/core/nmi.c                | 61 ++++++++++++++++++++++++++++++++++++
>  include/hw/nmi.h             | 20 ++++++++++++
>  include/monitor/hmp-target.h |  1 -
>  include/monitor/hmp.h        |  1 +
>  monitor/hmp-cmds.c           |  1 +
>  target/ppc/monitor.c         | 11 +++++++
>  7 files changed, 113 insertions(+), 2 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 7f0f3974ad..4a9089b431 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1581,12 +1581,30 @@ ERST
>          .cmd        = hmp_mce,
>      },
>  
> -#endif
>  SRST
>  ``mce`` *cpu* *bank* *status* *mcgstatus* *addr* *misc*
>    Inject an MCE on the given CPU (x86 only).
>  ERST
>  
> +#endif
> +
> +#if defined(TARGET_PPC)
> +
> +    {
> +        .name       = "mce",
> +        .args_type  = "cpu_index:i,srr1_mask:l,dsisr:i,dar:l,recovered:i",
> +        .params     = "cpu srr1_mask dsisr dar recovered",
> +        .help       = "inject a MCE on the given CPU",
> +        .cmd        = hmp_mce,
> +    },
> +
> +SRST
> +``mce`` *cpu* *srr1_mask* *dsisr* *dar* *recovered*
> +  Inject an MCE on the given CPU (PPC only).
> +ERST
> +
> +#endif
> +
>      {
>          .name       = "getfd",
>          .args_type  = "fdname:s",
> diff --git a/hw/core/nmi.c b/hw/core/nmi.c
> index 481c4b3c7e..2a79500967 100644
> --- a/hw/core/nmi.c
> +++ b/hw/core/nmi.c
> @@ -86,3 +86,64 @@ static void nmi_register_types(void)
>  }
>  
>  type_init(nmi_register_types)
> +
> +struct do_mce_s {
> +    const QDict *qdict;
> +    Error *err;
> +    bool handled;
> +};
> +
> +static void mce_children(Object *o, struct do_mce_s *ns);
> +
> +static int do_mce(Object *o, void *opaque)
> +{
> +    struct do_mce_s *ms = opaque;
> +    MCEState *m = (MCEState *) object_dynamic_cast(o, TYPE_MCE);
> +
> +    if (m) {
> +        MCEClass *mc = MCE_GET_CLASS(m);
> +
> +        ms->handled = true;
> +        mc->mce_monitor_handler(m, ms->qdict, &ms->err);
> +        if (ms->err) {
> +            return -1;
> +        }
> +    }
> +    mce_children(o, ms);
> +
> +    return 0;
> +}
> +
> +static void mce_children(Object *o, struct do_mce_s *ms)
> +{
> +    object_child_foreach(o, do_mce, ms);
> +}
> +
> +void mce_monitor_handle(const QDict *qdict, Error **errp)
> +{
> +    struct do_mce_s ms = {
> +        .qdict = qdict,
> +        .err = NULL,
> +        .handled = false
> +    };
> +
> +    mce_children(object_get_root(), &ms);
> +    if (ms.handled) {
> +        error_propagate(errp, ms.err);
> +    } else {
> +        error_setg(errp, QERR_UNSUPPORTED);
> +    }
> +}
> +
> +static const TypeInfo mce_info = {
> +    .name          = TYPE_MCE,
> +    .parent        = TYPE_INTERFACE,
> +    .class_size    = sizeof(MCEClass),
> +};
> +
> +static void mce_register_types(void)
> +{
> +    type_register_static(&mce_info);
> +}
> +
> +type_init(mce_register_types)
> diff --git a/include/hw/nmi.h b/include/hw/nmi.h
> index fe37ce3ad8..de39d95c9a 100644
> --- a/include/hw/nmi.h
> +++ b/include/hw/nmi.h
> @@ -43,4 +43,24 @@ typedef struct NMIClass {
>  
>  void nmi_monitor_handle(int cpu_index, Error **errp);
>  
> +
> +#define TYPE_MCE "mce"
> +
> +#define MCE_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(MCEClass, (klass), TYPE_MCE)
> +#define MCE_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(MCEClass, (obj), TYPE_MCE)
> +#define MCE(obj) \
> +     INTERFACE_CHECK(MCEState, (obj), TYPE_MCE)
> +
> +typedef struct MCEState MCEState;
> +
> +typedef struct MCEClass {
> +    InterfaceClass parent_class;
> +
> +    void (*mce_monitor_handler)(MCEState *n, const QDict *qdict, Error **errp);
> +} MCEClass;
> +
> +void mce_monitor_handle(const QDict *qdict, Error **errp);
> +
>  #endif /* NMI_H */
> diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
> index 8b7820a3ad..afb8f5bca2 100644
> --- a/include/monitor/hmp-target.h
> +++ b/include/monitor/hmp-target.h
> @@ -45,7 +45,6 @@ CPUState *mon_get_cpu(void);
>  
>  void hmp_info_mem(Monitor *mon, const QDict *qdict);
>  void hmp_info_tlb(Monitor *mon, const QDict *qdict);
> -void hmp_mce(Monitor *mon, const QDict *qdict);
>  void hmp_info_local_apic(Monitor *mon, const QDict *qdict);
>  void hmp_info_io_apic(Monitor *mon, const QDict *qdict);
>  
> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
> index e33ca5a911..f747a5e214 100644
> --- a/include/monitor/hmp.h
> +++ b/include/monitor/hmp.h
> @@ -54,6 +54,7 @@ void hmp_ringbuf_read(Monitor *mon, const QDict *qdict);
>  void hmp_cont(Monitor *mon, const QDict *qdict);
>  void hmp_system_wakeup(Monitor *mon, const QDict *qdict);
>  void hmp_nmi(Monitor *mon, const QDict *qdict);
> +void hmp_mce(Monitor *mon, const QDict *qdict);
>  void hmp_set_link(Monitor *mon, const QDict *qdict);
>  void hmp_balloon(Monitor *mon, const QDict *qdict);
>  void hmp_loadvm(Monitor *mon, const QDict *qdict);
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 58724031ea..3664ef2a4f 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -52,6 +52,7 @@
>  #include "exec/ramlist.h"
>  #include "hw/intc/intc.h"
>  #include "hw/rdma/rdma.h"
> +#include "hw/nmi.h"
>  #include "migration/snapshot.h"
>  #include "migration/misc.h"
>  
> diff --git a/target/ppc/monitor.c b/target/ppc/monitor.c
> index a5a177d717..6daf543efc 100644
> --- a/target/ppc/monitor.c
> +++ b/target/ppc/monitor.c
> @@ -28,6 +28,8 @@
>  #include "qemu/ctype.h"
>  #include "monitor/hmp-target.h"
>  #include "monitor/hmp.h"
> +#include "qapi/qmp/qdict.h"
> +#include "hw/nmi.h"
>  
>  static target_long monitor_get_ccr(const struct MonitorDef *md, int val)
>  {
> @@ -72,6 +74,15 @@ void hmp_info_tlb(Monitor *mon, const QDict *qdict)
>      dump_mmu(env1);
>  }
>  
> +void hmp_mce(Monitor *mon, const QDict *qdict)
> +{
> +    Error *err = NULL;
> +
> +    mce_monitor_handle(qdict, &err);
> +
> +    hmp_handle_error(mon, err);
> +}
> +
>  const MonitorDef monitor_defs[] = {
>      { "fpscr", offsetof(CPUPPCState, fpscr) },
>      /* Next instruction pointer */

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/5] nmi: add MCE class for implementing machine check injection commands

[Nicholas Piggin]

David Gibson's on March 31, 2020 10:22 am:
> On Thu, Mar 26, 2020 at 12:41:45AM +1000, Nicholas Piggin wrote:
>> Like commit 9cb805fd26 ("cpus: Define callback for QEMU "nmi" command")
>> this implements a machine check injection command framework and defines
>> a monitor command for ppc.
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> 
> So, AFAICT, both x86 and ppc have something called an MCE, and while
> I'm guessing they're somewhat related, they don't work quite the same
> way - different information provided to the handler and so forth.
> 
> I think it's reasonable to overload the "mce" HMP command based on
> target for the different types.  However, I think calling the
> interface classes which are specific to the ppc type just "mce" could
> be pretty confusing.

Okay. So, convert i386 first?

> In addition, I think this is adding an HMP command to inject the event
> without any corresponding way of injecting via QMP.  I believe that's
> frowned upon.

I attempted that but didn't get too far. I guess it's more of a
special test than a management function (nmi has valid uses in 
administering a machine), so maybe we can get an exemption. One issue
is different QMP command for powerpc vs x86.

I think error injection as a general concept might be valid there, but
the better interface for that level would be higher up, e.g, not
specifying register settings but rather "simulate uncorrected memory
error on this byte".

Do you think that is reasonable reason to avoid adding QMP for this
nasty low level thing?

Thanks,
Nick

Re: [PATCH 3/5] nmi: add MCE class for implementing machine check injection commands

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2227 bytes --]

On Fri, Apr 03, 2020 at 06:04:47PM +1000, Nicholas Piggin wrote:
> David Gibson's on March 31, 2020 10:22 am:
> > On Thu, Mar 26, 2020 at 12:41:45AM +1000, Nicholas Piggin wrote:
> >> Like commit 9cb805fd26 ("cpus: Define callback for QEMU "nmi" command")
> >> this implements a machine check injection command framework and defines
> >> a monitor command for ppc.
> >> 
> >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > 
> > So, AFAICT, both x86 and ppc have something called an MCE, and while
> > I'm guessing they're somewhat related, they don't work quite the same
> > way - different information provided to the handler and so forth.
> > 
> > I think it's reasonable to overload the "mce" HMP command based on
> > target for the different types.  However, I think calling the
> > interface classes which are specific to the ppc type just "mce" could
> > be pretty confusing.
> 
> Okay. So, convert i386 first?

Uh.. not necessarily.  But call your version PpcMce or something
instead.

> > In addition, I think this is adding an HMP command to inject the event
> > without any corresponding way of injecting via QMP.  I believe that's
> > frowned upon.
> 
> I attempted that but didn't get too far. I guess it's more of a
> special test than a management function (nmi has valid uses in 
> administering a machine), so maybe we can get an exemption. One issue
> is different QMP command for powerpc vs x86.
> 
> I think error injection as a general concept might be valid there, but
> the better interface for that level would be higher up, e.g, not
> specifying register settings but rather "simulate uncorrected memory
> error on this byte".
> 
> Do you think that is reasonable reason to avoid adding QMP for this
> nasty low level thing?

Hrm, I doubt it will convince the qemu community at large.  I think
the view is that QMP is the "real" management interface for qemu - HMP
is somewhere in between a shim for convenience and something needed
for backwards compat.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 4/5] ppc/spapr: Implement mce injection

[Nicholas Piggin]

This implements mce injection for spapr.

  (qemu) mce 0 0x200000 0x80 0xdeadbeef 1

    Disabling lock debugging due to kernel taint
    MCE: CPU0: machine check (Severe) Host SLB Multihit DAR: 00000000deadbeef [Recovered]
    MCE: CPU0: machine check (Severe) Host SLB Multihit [Recovered]
    MCE: CPU0: PID: 495 Comm: a NIP: [0000000130ee07c8]
    MCE: CPU0: Initiator CPU
    MCE: CPU0: Unknown
[   71.567193] MCE: CPU0: NIP: [c0000000000d7f6c] plpar_hcall_norets+0x1c/0x28
[   71.567249] MCE: CPU0: Initiator CPU
[   71.567308] MCE: CPU0: Unknown

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/spapr.c         | 54 ++++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h |  3 +++
 2 files changed, 57 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 785c41d205..6dbd1858f4 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -83,6 +83,7 @@
 #include "hw/ppc/spapr_nvdimm.h"
 
 #include "monitor/monitor.h"
+#include "qapi/qmp/qdict.h"
 
 #include <libfdt.h>
 
@@ -3420,6 +3421,56 @@ static void spapr_nmi(NMIState *n, int cpu_index, Error **errp)
     }
 }
 
+typedef struct MCEInjectionParams {
+    uint64_t srr1_mask;
+    uint32_t dsisr;
+    uint64_t dar;
+    bool recovered;
+} MCEInjectionParams;
+
+static void spapr_do_mce_on_cpu(CPUState *cs, run_on_cpu_data data)
+{
+    MCEInjectionParams *params = data.host_ptr;
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+    uint64_t srr1_mce_bits = PPC_BITMASK(42, 45) | PPC_BIT(36);
+
+    cpu_synchronize_state(cs);
+
+    env->spr[SPR_SRR0] = env->nip;
+    env->spr[SPR_SRR1] = (env->msr & ~srr1_mce_bits) |
+                         (params->srr1_mask & srr1_mce_bits);
+    if (params->dsisr) {
+        env->spr[SPR_DSISR] = params->dsisr;
+        env->spr[SPR_DAR] = params->dar;
+    }
+
+    spapr_mce_req_event(cpu, params->recovered);
+}
+
+static void spapr_mce(MCEState *m, const QDict *qdict, Error **errp)
+{
+    int cpu_index = qdict_get_int(qdict, "cpu_index");
+    uint64_t srr1_mask = qdict_get_int(qdict, "srr1_mask");
+    uint32_t dsisr = qdict_get_int(qdict, "dsisr");
+    uint64_t dar = qdict_get_int(qdict, "dar");
+    bool recovered = qdict_get_int(qdict, "recovered");
+    CPUState *cs;
+
+    cs = qemu_get_cpu(cpu_index);
+
+    if (cs != NULL) {
+        MCEInjectionParams params = {
+            .srr1_mask = srr1_mask,
+            .dsisr = dsisr,
+            .dar = dar,
+            .recovered = recovered,
+        };
+
+        run_on_cpu(cs, spapr_do_mce_on_cpu, RUN_ON_CPU_HOST_PTR(&params));
+    }
+}
+
 int spapr_lmb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
                           void *fdt, int *fdt_start_offset, Error **errp)
 {
@@ -4467,6 +4518,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(oc);
     FWPathProviderClass *fwc = FW_PATH_PROVIDER_CLASS(oc);
     NMIClass *nc = NMI_CLASS(oc);
+    MCEClass *mcec = MCE_CLASS(oc);
     HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
     PPCVirtualHypervisorClass *vhc = PPC_VIRTUAL_HYPERVISOR_CLASS(oc);
     XICSFabricClass *xic = XICS_FABRIC_CLASS(oc);
@@ -4511,6 +4563,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     smc->resize_hpt_default = SPAPR_RESIZE_HPT_ENABLED;
     fwc->get_dev_path = spapr_get_fw_dev_path;
     nc->nmi_monitor_handler = spapr_nmi;
+    mcec->mce_monitor_handler = spapr_mce;
     smc->phb_placement = spapr_phb_placement;
     vhc->hypercall = emulate_spapr_hypercall;
     vhc->hpt_mask = spapr_hpt_mask;
@@ -4566,6 +4619,7 @@ static const TypeInfo spapr_machine_info = {
     .interfaces = (InterfaceInfo[]) {
         { TYPE_FW_PATH_PROVIDER },
         { TYPE_NMI },
+        { TYPE_MCE },
         { TYPE_HOTPLUG_HANDLER },
         { TYPE_PPC_VIRTUAL_HYPERVISOR },
         { TYPE_XICS_FABRIC },
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 42d64a0368..72f86a2ee8 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -929,4 +929,7 @@ void spapr_check_pagesize(SpaprMachineState *spapr, hwaddr pagesize,
 
 void spapr_set_all_lpcrs(target_ulong value, target_ulong mask);
 hwaddr spapr_get_rtas_addr(void);
+
+void spapr_mce_inject(CPUState *cs, uint64_t srr1_mask, uint32_t dsisr,
+                      uint64_t dar, bool recovered);
 #endif /* HW_SPAPR_H */
-- 
2.23.0

Re: [PATCH 4/5] ppc/spapr: Implement mce injection

[Cédric Le Goater]

On 3/25/20 3:41 PM, Nicholas Piggin wrote:
> This implements mce injection for spapr.
> 
>   (qemu) mce 0 0x200000 0x80 0xdeadbeef 1
> 
>     Disabling lock debugging due to kernel taint
>     MCE: CPU0: machine check (Severe) Host SLB Multihit DAR: 00000000deadbeef [Recovered]
>     MCE: CPU0: machine check (Severe) Host SLB Multihit [Recovered]
>     MCE: CPU0: PID: 495 Comm: a NIP: [0000000130ee07c8]
>     MCE: CPU0: Initiator CPU
>     MCE: CPU0: Unknown
> [   71.567193] MCE: CPU0: NIP: [c0000000000d7f6c] plpar_hcall_norets+0x1c/0x28
> [   71.567249] MCE: CPU0: Initiator CPU
> [   71.567308] MCE: CPU0: Unknown
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>  hw/ppc/spapr.c         | 54 ++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h |  3 +++
>  2 files changed, 57 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 785c41d205..6dbd1858f4 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -83,6 +83,7 @@
>  #include "hw/ppc/spapr_nvdimm.h"
> 
>  #include "monitor/monitor.h"
> +#include "qapi/qmp/qdict.h"
> 
>  #include <libfdt.h>
> 
> @@ -3420,6 +3421,56 @@ static void spapr_nmi(NMIState *n, int cpu_index, Error **errp)
>      }
>  }
> 
> +typedef struct MCEInjectionParams {
> +    uint64_t srr1_mask;
> +    uint32_t dsisr;
> +    uint64_t dar;
> +    bool recovered;
> +} MCEInjectionParams;
> +
> +static void spapr_do_mce_on_cpu(CPUState *cs, run_on_cpu_data data)
> +{
> +    MCEInjectionParams *params = data.host_ptr;
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
> +    uint64_t srr1_mce_bits = PPC_BITMASK(42, 45) | PPC_BIT(36);
> +
> +    cpu_synchronize_state(cs);
> +
> +    env->spr[SPR_SRR0] = env->nip;
> +    env->spr[SPR_SRR1] = (env->msr & ~srr1_mce_bits) |
> +                         (params->srr1_mask & srr1_mce_bits);
> +    if (params->dsisr) {
> +        env->spr[SPR_DSISR] = params->dsisr;
> +        env->spr[SPR_DAR] = params->dar;
> +    }
> +
> +    spapr_mce_req_event(cpu, params->recovered);
> +}
> +
> +static void spapr_mce(MCEState *m, const QDict *qdict, Error **errp)
> +{
> +    int cpu_index = qdict_get_int(qdict, "cpu_index");
> +    uint64_t srr1_mask = qdict_get_int(qdict, "srr1_mask");
> +    uint32_t dsisr = qdict_get_int(qdict, "dsisr");
> +    uint64_t dar = qdict_get_int(qdict, "dar");
> +    bool recovered = qdict_get_int(qdict, "recovered");
> +    CPUState *cs;
> +
> +    cs = qemu_get_cpu(cpu_index);
> +
> +    if (cs != NULL) {
> +        MCEInjectionParams params = {
> +            .srr1_mask = srr1_mask,
> +            .dsisr = dsisr,
> +            .dar = dar,
> +            .recovered = recovered,
> +        };
> +
> +        run_on_cpu(cs, spapr_do_mce_on_cpu, RUN_ON_CPU_HOST_PTR(&params));
> +    }
> +}
> +
>  int spapr_lmb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
>                            void *fdt, int *fdt_start_offset, Error **errp)
>  {
> @@ -4467,6 +4518,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(oc);
>      FWPathProviderClass *fwc = FW_PATH_PROVIDER_CLASS(oc);
>      NMIClass *nc = NMI_CLASS(oc);
> +    MCEClass *mcec = MCE_CLASS(oc);
>      HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
>      PPCVirtualHypervisorClass *vhc = PPC_VIRTUAL_HYPERVISOR_CLASS(oc);
>      XICSFabricClass *xic = XICS_FABRIC_CLASS(oc);
> @@ -4511,6 +4563,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      smc->resize_hpt_default = SPAPR_RESIZE_HPT_ENABLED;
>      fwc->get_dev_path = spapr_get_fw_dev_path;
>      nc->nmi_monitor_handler = spapr_nmi;
> +    mcec->mce_monitor_handler = spapr_mce;
>      smc->phb_placement = spapr_phb_placement;
>      vhc->hypercall = emulate_spapr_hypercall;
>      vhc->hpt_mask = spapr_hpt_mask;
> @@ -4566,6 +4619,7 @@ static const TypeInfo spapr_machine_info = {
>      .interfaces = (InterfaceInfo[]) {
>          { TYPE_FW_PATH_PROVIDER },
>          { TYPE_NMI },
> +        { TYPE_MCE },
>          { TYPE_HOTPLUG_HANDLER },
>          { TYPE_PPC_VIRTUAL_HYPERVISOR },
>          { TYPE_XICS_FABRIC },
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 42d64a0368..72f86a2ee8 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -929,4 +929,7 @@ void spapr_check_pagesize(SpaprMachineState *spapr, hwaddr pagesize,
> 
>  void spapr_set_all_lpcrs(target_ulong value, target_ulong mask);
>  hwaddr spapr_get_rtas_addr(void);
> +
> +void spapr_mce_inject(CPUState *cs, uint64_t srr1_mask, uint32_t dsisr,
> +                      uint64_t dar, bool recovered);
>  #endif /* HW_SPAPR_H */
> 

[PATCH 5/5] ppc/pnv: Implement mce injection

[Nicholas Piggin]

This implements mce injection for pnv.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/pnv.c             | 55 ++++++++++++++++++++++++++++++++++++++++
 target/ppc/cpu.h         |  1 +
 target/ppc/excp_helper.c | 12 +++++++++
 3 files changed, 68 insertions(+)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 671535ebe6..9c515bfeed 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -38,6 +38,7 @@
 #include "hw/nmi.h"
 #include "exec/address-spaces.h"
 #include "qapi/visitor.h"
+#include "qapi/qmp/qdict.h"
 #include "monitor/monitor.h"
 #include "hw/intc/intc.h"
 #include "hw/ipmi/ipmi.h"
@@ -1981,11 +1982,63 @@ static void pnv_nmi(NMIState *n, int cpu_index, Error **errp)
     }
 }
 
+typedef struct MCEInjectionParams {
+    uint64_t srr1_mask;
+    uint32_t dsisr;
+    uint64_t dar;
+    bool recovered;
+} MCEInjectionParams;
+
+static void pnv_do_mce_on_cpu(CPUState *cs, run_on_cpu_data data)
+{
+    MCEInjectionParams *params = data.host_ptr;
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+    uint64_t srr1_mce_bits = PPC_BITMASK(42, 45) | PPC_BIT(36);
+
+    cpu_synchronize_state(cs);
+    ppc_cpu_do_machine_check(cs);
+
+    env->spr[SPR_SRR1] |= (params->srr1_mask & srr1_mce_bits);
+    if (params->dsisr) {
+        env->spr[SPR_DSISR] = params->dsisr;
+        env->spr[SPR_DAR] = params->dar;
+    }
+
+    if (!params->recovered) {
+        env->msr &= ~MSR_RI;
+    }
+}
+
+static void pnv_mce(MCEState *m, const QDict *qdict, Error **errp)
+{
+    int cpu_index = qdict_get_int(qdict, "cpu_index");
+    uint64_t srr1_mask = qdict_get_int(qdict, "srr1_mask");
+    uint32_t dsisr = qdict_get_int(qdict, "dsisr");
+    uint64_t dar = qdict_get_int(qdict, "dar");
+    bool recovered = qdict_get_int(qdict, "recovered");
+    CPUState *cs;
+
+    cs = qemu_get_cpu(cpu_index);
+
+    if (cs != NULL) {
+        MCEInjectionParams params = {
+            .srr1_mask = srr1_mask,
+            .dsisr = dsisr,
+            .dar = dar,
+            .recovered = recovered,
+        };
+
+        run_on_cpu(cs, pnv_do_mce_on_cpu, RUN_ON_CPU_HOST_PTR(&params));
+    }
+}
+
 static void pnv_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
     InterruptStatsProviderClass *ispc = INTERRUPT_STATS_PROVIDER_CLASS(oc);
     NMIClass *nc = NMI_CLASS(oc);
+    MCEClass *mcec = MCE_CLASS(oc);
 
     mc->desc = "IBM PowerNV (Non-Virtualized)";
     mc->init = pnv_init;
@@ -2003,6 +2056,7 @@ static void pnv_machine_class_init(ObjectClass *oc, void *data)
     mc->default_ram_id = "pnv.ram";
     ispc->print_info = pnv_pic_print_info;
     nc->nmi_monitor_handler = pnv_nmi;
+    mcec->mce_monitor_handler = pnv_mce;
 
     object_class_property_add_bool(oc, "hb-mode",
                                    pnv_machine_get_hb, pnv_machine_set_hb,
@@ -2067,6 +2121,7 @@ static const TypeInfo types[] = {
         .interfaces = (InterfaceInfo[]) {
             { TYPE_INTERRUPT_STATS_PROVIDER },
             { TYPE_NMI },
+            { TYPE_MCE },
         },
     },
     {
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index f4a5304d43..9be27f59c5 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1221,6 +1221,7 @@ int ppc32_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
                                int cpuid, void *opaque);
 #ifndef CONFIG_USER_ONLY
 void ppc_cpu_do_system_reset(CPUState *cs);
+void ppc_cpu_do_machine_check(CPUState *cs);
 void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector);
 extern const VMStateDescription vmstate_ppc_cpu;
 #endif
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 7f2b5899d3..81dd8b6f8e 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -279,6 +279,10 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
             cs->halted = 1;
             cpu_interrupt_exittb(cs);
         }
+        if (msr_pow) {
+            /* indicate that we resumed from power save mode */
+            msr |= 0x10000;
+        }
         if (env->msr_mask & MSR_HVB) {
             /*
              * ISA specifies HV, but can be delivered to guest with HV
@@ -969,6 +973,14 @@ void ppc_cpu_do_system_reset(CPUState *cs)
     powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_RESET);
 }
 
+void ppc_cpu_do_machine_check(CPUState *cs)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+
+    powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_MCHECK);
+}
+
 void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector)
 {
     PowerPCCPU *cpu = POWERPC_CPU(cs);
-- 
2.23.0

Re: [EXTERNAL] [PATCH 5/5] ppc/pnv: Implement mce injection

[Cédric Le Goater]

On 3/25/20 3:41 PM, Nicholas Piggin wrote:
> This implements mce injection for pnv.

This would be the command to use ? 

(qemu) mce 0 0x100000 0x80 0xdeadbeef 1

> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  hw/ppc/pnv.c             | 55 ++++++++++++++++++++++++++++++++++++++++
>  target/ppc/cpu.h         |  1 +
>  target/ppc/excp_helper.c | 12 +++++++++
>  3 files changed, 68 insertions(+)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 671535ebe6..9c515bfeed 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -38,6 +38,7 @@
>  #include "hw/nmi.h"
>  #include "exec/address-spaces.h"
>  #include "qapi/visitor.h"
> +#include "qapi/qmp/qdict.h"
>  #include "monitor/monitor.h"
>  #include "hw/intc/intc.h"
>  #include "hw/ipmi/ipmi.h"
> @@ -1981,11 +1982,63 @@ static void pnv_nmi(NMIState *n, int cpu_index, Error **errp)
>      }
>  }
> 
> +typedef struct MCEInjectionParams {
> +    uint64_t srr1_mask;
> +    uint32_t dsisr;
> +    uint64_t dar;
> +    bool recovered;
> +} MCEInjectionParams;
> +
> +static void pnv_do_mce_on_cpu(CPUState *cs, run_on_cpu_data data)
> +{
> +    MCEInjectionParams *params = data.host_ptr;
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
> +    uint64_t srr1_mce_bits = PPC_BITMASK(42, 45) | PPC_BIT(36);
> +
> +    cpu_synchronize_state(cs);

I think this call is superfluous as we don't support any accelerators 
on this machine (but we might one day).
 
> +    ppc_cpu_do_machine_check(cs);
> +
> +    env->spr[SPR_SRR1] |= (params->srr1_mask & srr1_mce_bits);

Don't  we need to clear the previous settings like on spapr ? 

> +    if (params->dsisr) {
> +        env->spr[SPR_DSISR] = params->dsisr;
> +        env->spr[SPR_DAR] = params->dar;
> +    }
> +
> +    if (!params->recovered) {
> +        env->msr &= ~MSR_RI;
> +    }
> +}
> +
> +static void pnv_mce(MCEState *m, const QDict *qdict, Error **errp)
> +{
> +    int cpu_index = qdict_get_int(qdict, "cpu_index");
> +    uint64_t srr1_mask = qdict_get_int(qdict, "srr1_mask");
> +    uint32_t dsisr = qdict_get_int(qdict, "dsisr");
> +    uint64_t dar = qdict_get_int(qdict, "dar");
> +    bool recovered = qdict_get_int(qdict, "recovered");
> +    CPUState *cs;
> +
> +    cs = qemu_get_cpu(cpu_index);
> +
> +    if (cs != NULL) {
> +        MCEInjectionParams params = {
> +            .srr1_mask = srr1_mask,
> +            .dsisr = dsisr,
> +            .dar = dar,
> +            .recovered = recovered,
> +        };
> +
> +        run_on_cpu(cs, pnv_do_mce_on_cpu, RUN_ON_CPU_HOST_PTR(&params));
> +    }
> +}
> +
>  static void pnv_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
>      InterruptStatsProviderClass *ispc = INTERRUPT_STATS_PROVIDER_CLASS(oc);
>      NMIClass *nc = NMI_CLASS(oc);
> +    MCEClass *mcec = MCE_CLASS(oc);
> 
>      mc->desc = "IBM PowerNV (Non-Virtualized)";
>      mc->init = pnv_init;
> @@ -2003,6 +2056,7 @@ static void pnv_machine_class_init(ObjectClass *oc, void *data)
>      mc->default_ram_id = "pnv.ram";
>      ispc->print_info = pnv_pic_print_info;
>      nc->nmi_monitor_handler = pnv_nmi;
> +    mcec->mce_monitor_handler = pnv_mce;
> 
>      object_class_property_add_bool(oc, "hb-mode",
>                                     pnv_machine_get_hb, pnv_machine_set_hb,
> @@ -2067,6 +2121,7 @@ static const TypeInfo types[] = {
>          .interfaces = (InterfaceInfo[]) {
>              { TYPE_INTERRUPT_STATS_PROVIDER },
>              { TYPE_NMI },
> +            { TYPE_MCE },
>          },
>      },
>      {
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index f4a5304d43..9be27f59c5 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1221,6 +1221,7 @@ int ppc32_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
>                                 int cpuid, void *opaque);
>  #ifndef CONFIG_USER_ONLY
>  void ppc_cpu_do_system_reset(CPUState *cs);
> +void ppc_cpu_do_machine_check(CPUState *cs);
>  void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector);
>  extern const VMStateDescription vmstate_ppc_cpu;
>  #endif
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 7f2b5899d3..81dd8b6f8e 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -279,6 +279,10 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>              cs->halted = 1;
>              cpu_interrupt_exittb(cs);
>          }
> +        if (msr_pow) {
> +            /* indicate that we resumed from power save mode */
> +            msr |= 0x10000;

#define ? 

> +        }
>          if (env->msr_mask & MSR_HVB) {
>              /*
>               * ISA specifies HV, but can be delivered to guest with HV
> @@ -969,6 +973,14 @@ void ppc_cpu_do_system_reset(CPUState *cs)
>      powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_RESET);
>  }
> 
> +void ppc_cpu_do_machine_check(CPUState *cs)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
> +
> +    powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_MCHECK);
> +}
> +
>  void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
> 

Re: [EXTERNAL] [PATCH 5/5] ppc/pnv: Implement mce injection

[Nicholas Piggin]

Cédric Le Goater's on March 26, 2020 2:39 am:
> On 3/25/20 3:41 PM, Nicholas Piggin wrote:
>> This implements mce injection for pnv.
> 
> This would be the command to use ? 
> 
> (qemu) mce 0 0x100000 0x80 0xdeadbeef 1
> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>  hw/ppc/pnv.c             | 55 ++++++++++++++++++++++++++++++++++++++++
>>  target/ppc/cpu.h         |  1 +
>>  target/ppc/excp_helper.c | 12 +++++++++
>>  3 files changed, 68 insertions(+)
>> 
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 671535ebe6..9c515bfeed 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -38,6 +38,7 @@
>>  #include "hw/nmi.h"
>>  #include "exec/address-spaces.h"
>>  #include "qapi/visitor.h"
>> +#include "qapi/qmp/qdict.h"
>>  #include "monitor/monitor.h"
>>  #include "hw/intc/intc.h"
>>  #include "hw/ipmi/ipmi.h"
>> @@ -1981,11 +1982,63 @@ static void pnv_nmi(NMIState *n, int cpu_index, Error **errp)
>>      }
>>  }
>> 
>> +typedef struct MCEInjectionParams {
>> +    uint64_t srr1_mask;
>> +    uint32_t dsisr;
>> +    uint64_t dar;
>> +    bool recovered;
>> +} MCEInjectionParams;
>> +
>> +static void pnv_do_mce_on_cpu(CPUState *cs, run_on_cpu_data data)
>> +{
>> +    MCEInjectionParams *params = data.host_ptr;
>> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
>> +    CPUPPCState *env = &cpu->env;
>> +    uint64_t srr1_mce_bits = PPC_BITMASK(42, 45) | PPC_BIT(36);
>> +
>> +    cpu_synchronize_state(cs);
> 
> I think this call is superfluous as we don't support any accelerators 
> on this machine (but we might one day).

Okay. I can remove it, or do you think it's fine to stay?

>> +    ppc_cpu_do_machine_check(cs);
>> +
>> +    env->spr[SPR_SRR1] |= (params->srr1_mask & srr1_mce_bits);
> 
> Don't  we need to clear the previous settings like on spapr ? 

Yes, good catch.

>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index 7f2b5899d3..81dd8b6f8e 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -279,6 +279,10 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>>              cs->halted = 1;
>>              cpu_interrupt_exittb(cs);
>>          }
>> +        if (msr_pow) {
>> +            /* indicate that we resumed from power save mode */
>> +            msr |= 0x10000;
> 
> #define ? 

It matches system reset, but yes it should be put in a define (or
PPC_BIT should be okay I guess because it's architecture). As
discussed in the nmi patch, we have to make some adjustments for
pnv so I'll tidy that up too.

Thanks,
Nick

Re: [PATCH 0/5] ppc: sreset and machine check injection

[Cédric Le Goater]

On 3/25/20 3:41 PM, Nicholas Piggin wrote:
> This adds nmi injection for pnv, and similar mce injection for
> spapr and pnv. The mce injection has already uncovered quite a
> few bugs in Linux papr guest and one in pnv host code, so it
> has been already very useful. 

Nice ! 

> The mambo simulator can do similar
> injection but it's a bit more clunky and needs to run KVM and
> QEMU in the sim to test papr guests, which is quite slow.

You can run a KVM guest under the QEMU powernv machine. You will 
need a patch from Suraj adding radix support for guests, which is 
still in my powernv-5.0 branch.

> HMIs like timebase corruption would be another good candidate
> for error injection.
 
XSCOM errors also. 

Thanks,

C. 


> Thanks,
> Nick
> 
> Nicholas Piggin (5):
>   ppc/spapr: tweak change system reset helper
>   ppc/pnv: Add support for NMI interface
>   nmi: add MCE class for implementing machine check injection commands
>   ppc/spapr: Implement mce injection
>   ppc/pnv: Implement mce injection
> 
>  hmp-commands.hx              | 20 ++++++++-
>  hw/core/nmi.c                | 61 ++++++++++++++++++++++++++
>  hw/ppc/pnv.c                 | 85 +++++++++++++++++++++++++++++++++++-
>  hw/ppc/spapr.c               | 63 ++++++++++++++++++++++++--
>  include/hw/nmi.h             | 20 +++++++++
>  include/hw/ppc/spapr.h       |  3 ++
>  include/monitor/hmp-target.h |  1 -
>  include/monitor/hmp.h        |  1 +
>  monitor/hmp-cmds.c           |  1 +
>  target/ppc/cpu.h             |  3 +-
>  target/ppc/excp_helper.c     | 17 ++++++--
>  target/ppc/monitor.c         | 11 +++++
>  12 files changed, 275 insertions(+), 11 deletions(-)
>