Nicholas Piggin's on April 3, 2020 5:57 pm:
> Cédric Le Goater's on March 26, 2020 2:38 am:
>> [ Please use c...@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 <npig...@gmail.com>
>>
>> one minor comment,
>>
>> Reviewed-by: Cédric Le Goater <c...@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
