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 address@hidden ! ]
>>>>
>>>> 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 <address@hidden>
>>>>
>>>> one minor comment,
>>>>
>>>> Reviewed-by: Cédric Le Goater <address@hidden>
>>>>
>>>>> ---
>>>>>  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