Re: [PATCH v7 39/42] powerpc: move NMI entry/exit code into wrapper

[Nicholas Piggin <npiggin@gmail.com>]

Excerpts from Nicholas Piggin's message of February 6, 2021 12:46 pm:
> Excerpts from Michael Ellerman's message of February 6, 2021 9:38 am:
>> Nicholas Piggin <npiggin@gmail.com> writes:
>>> Excerpts from Michael Ellerman's message of February 4, 2021 8:15 pm:
>>>> Nicholas Piggin <npiggin@gmail.com> writes:
>>>>> This moves the common NMI entry and exit code into the interrupt handler
>>>>> wrappers.
>>>>>
>>>>> This changes the behaviour of soft-NMI (watchdog) and HMI interrupts, and
>>>>> also MCE interrupts on 64e, by adding missing parts of the NMI entry to
>>>>> them.
>>>>>
>>>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>>> ---
>>>>>  arch/powerpc/include/asm/interrupt.h | 28 ++++++++++++++++++++++
>>>>>  arch/powerpc/kernel/mce.c            | 11 ---------
>>>>>  arch/powerpc/kernel/traps.c          | 35 +++++-----------------------
>>>>>  arch/powerpc/kernel/watchdog.c       | 10 ++++----
>>>>>  4 files changed, 38 insertions(+), 46 deletions(-)
>>>> 
>>>> This is unhappy when injecting SLB multi-hits:
>>>> 
>>>>   root@p86-2:~# echo PPC_SLB_MULTIHIT > /sys/kernel/debug/provoke-crash/DIRECT
>>>>   [  312.496026][ T1344] kernel BUG at arch/powerpc/include/asm/interrupt.h:152!
>>>>   [  312.496037][ T1344] Oops: Exception in kernel mode, sig: 5 [#1]
>>>>   [  312.496045][ T1344] LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
>>>
>>> pseries hash. Blast!
>> 
>> The worst kind.
>> 
>>>> 147 static inline void interrupt_nmi_exit_prepare(struct pt_regs *regs, struct interrupt_nmi_state *state)
>>>> 148 {
>>>> 149 	if (!IS_ENABLED(CONFIG_PPC_BOOK3S_64) ||
>>>> 150 			!firmware_has_feature(FW_FEATURE_LPAR) ||
>>>> 151 			radix_enabled() || (mfmsr() & MSR_DR))
>>>> 152 		nmi_exit();
>>>> 
>>>> 
>>>> So presumably it's:
>>>> 
>>>> #define __nmi_exit()						\
>>>> 	do {							\
>>>> 		BUG_ON(!in_nmi());				\
>>>
>>> Yes that would be it, pseries machine check enables MMU half way through 
>>> so only one side of this triggers.
>>>
>>> The MSR_DR check is supposed to catch the other NMIs that run with MMU 
>>> on (perf, watchdog, etc). Suppose it could test TRAP(regs) explicitly
>>> although I wonder if we should also do this to keep things balanced
>> 
>> Yeah I think I like that. I'll give it a test.
> 
> The msr restore? Looking closer, pseries_machine_check_realmode may have
> expected mce_handle_error to enable the MMU, because irq_work_queue uses
> some per-cpu variables I think.
> 
> So the code might have to be rearranged a bit more than the patch below.

Here is a patch, it should go anywhere before this patch. Seems to
work with some test MCE injection on pseries hash.

Thanks,
Nick
--

powerpc/pseries/mce: restore msr before returning from handler

The pseries real-mode machine check handler can enable the MMU, and
return from the handler with the MMU still enabled.

This works, but real-mode handler wrapper exit handlers want to rely
on the MMU being in real-mode. So change the pseries handler to
restore the MSR after it has finished virtual mode tasks.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/platforms/pseries/ras.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/ras.c b/arch/powerpc/platforms/pseries/ras.c
index 2d9f985fd13a..377439e88598 100644
--- a/arch/powerpc/platforms/pseries/ras.c
+++ b/arch/powerpc/platforms/pseries/ras.c
@@ -722,6 +722,7 @@ static int mce_handle_error(struct pt_regs *regs, struct rtas_error_log *errp)
 	struct pseries_errorlog *pseries_log;
 	struct pseries_mc_errorlog *mce_log = NULL;
 	int disposition = rtas_error_disposition(errp);
+	unsigned long msr;
 	u8 error_type;
 
 	if (!rtas_error_extended(errp))
@@ -747,9 +748,21 @@ static int mce_handle_error(struct pt_regs *regs, struct rtas_error_log *errp)
 	 *       SLB multihit is done by now.
 	 */
 out:
-	mtmsr(mfmsr() | MSR_IR | MSR_DR);
+	msr = mfmsr();
+	mtmsr(msr | MSR_IR | MSR_DR);
+
 	disposition = mce_handle_err_virtmode(regs, errp, mce_log,
 					      disposition);
+
+	/*
+	 * Queue irq work to log this rtas event later.
+	 * irq_work_queue uses per-cpu variables, so do this in virt
+	 * mode as well.
+	 */
+	irq_work_queue(&mce_errlog_process_work);
+
+	mtmsr(msr);
+
 	return disposition;
 }
 
@@ -865,10 +878,8 @@ long pseries_machine_check_realmode(struct pt_regs *regs)
 		 * virtual mode.
 		 */
 		disposition = mce_handle_error(regs, errp);
-		fwnmi_release_errinfo();
 
-		/* Queue irq work to log this rtas event later. */
-		irq_work_queue(&mce_errlog_process_work);
+		fwnmi_release_errinfo();
 
 		if (disposition == RTAS_DISP_FULLY_RECOVERED)
 			return 1;
-- 
2.23.0