diff mbox series

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

Message ID 20200325144147.221875-3-npiggin@gmail.com
State New
Headers show
Series ppc: sreset and machine check injection | expand

Commit Message

Nicholas Piggin March 25, 2020, 2:41 p.m. UTC
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(-)

Comments

Cédric Le Goater March 25, 2020, 4:38 p.m. UTC | #1
[ 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 },
>          },
>      },
>      {
>
David Gibson March 26, 2020, 12:15 a.m. UTC | #2
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 },
>          },
>      },
>      {
Alexey Kardashevskiy March 31, 2020, 3:07 a.m. UTC | #3
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,


>          },
>      },
>      {
>
David Gibson March 31, 2020, 3:14 a.m. UTC | #4
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.

> 
> 
> >          },
> >      },
> >      {
> > 
>
Nicholas Piggin April 3, 2020, 7:57 a.m. UTC | #5
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
Nicholas Piggin April 3, 2020, 1:12 p.m. UTC | #6
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)
Cédric Le Goater April 3, 2020, 3:47 p.m. UTC | #7
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
Nicholas Piggin April 4, 2020, 1:58 a.m. UTC | #8
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
diff mbox series

Patch

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 },
         },
     },
     {