QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Nicholas Piggin <npiggin@gmail.com>
To: "Cédric Le Goater" <clg@kaod.org>, qemu-ppc@nongnu.org
Cc: Alexey Kardashevskiy <aik@ozlabs.ru>,
	Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>,
	qemu-devel@nongnu.org, Greg Kurz <groug@kaod.org>,
	Ganesh Goudar <ganeshgr@linux.ibm.com>,
	David Gibson <david@gibson.dropbear.id.au>
Subject: Re: [PATCH 2/5] ppc/pnv: Add support for NMI interface
Date: Fri, 03 Apr 2020 23:12:36 +1000
Message-ID: <1585915002.kqdz9mya1i.astroid@bobo.none> (raw)
In-Reply-To: <1585899319.9tofsl4fd9.astroid@bobo.none>

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



  reply index

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-25 14:41 [PATCH 0/5] ppc: sreset and machine check injection Nicholas Piggin
2020-03-25 14:41 ` [PATCH 1/5] ppc/spapr: tweak change system reset helper Nicholas Piggin
2020-03-25 16:14   ` [EXTERNAL] " Cédric Le Goater
2020-03-26  0:04   ` David Gibson
2020-03-25 14:41 ` [PATCH 2/5] ppc/pnv: Add support for NMI interface Nicholas Piggin
2020-03-25 16:38   ` Cédric Le Goater
2020-04-03  7:57     ` Nicholas Piggin
2020-04-03 13:12       ` Nicholas Piggin [this message]
2020-04-03 15:47         ` Cédric Le Goater
2020-04-04  1:58           ` Nicholas Piggin
2020-03-26  0:15   ` David Gibson
2020-03-31  3:07   ` Alexey Kardashevskiy
2020-03-31  3:14     ` David Gibson
2020-03-25 14:41 ` [PATCH 3/5] nmi: add MCE class for implementing machine check injection commands Nicholas Piggin
2020-03-25 16:46   ` Cédric Le Goater
2020-03-31  0:22   ` David Gibson
2020-04-03  8:04     ` Nicholas Piggin
2020-04-06  6:45       ` David Gibson
2020-03-25 14:41 ` [PATCH 4/5] ppc/spapr: Implement mce injection Nicholas Piggin
2020-03-25 16:38   ` Cédric Le Goater
2020-03-25 14:41 ` [PATCH 5/5] ppc/pnv: " Nicholas Piggin
2020-03-25 16:39   ` [EXTERNAL] " Cédric Le Goater
2020-04-03  8:07     ` Nicholas Piggin
2020-03-25 16:42 ` [PATCH 0/5] ppc: sreset and machine check injection Cédric Le Goater

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1585915002.kqdz9mya1i.astroid@bobo.none \
    --to=npiggin@gmail.com \
    --cc=aik@ozlabs.ru \
    --cc=clg@kaod.org \
    --cc=david@gibson.dropbear.id.au \
    --cc=ganeshgr@linux.ibm.com \
    --cc=groug@kaod.org \
    --cc=mahesh@linux.vnet.ibm.com \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-ppc@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git