[PATCH] powerpc/64: allow rtas to be called in real-mode, use this in machine check

rtas_call allocates and uses memory in failure paths, which is
not safe for RMA. It also calls local_irq_save() which may not be safe
in all real mode contexts.

Particularly machine check may run with interrupts not "reconciled",
and it may have hit while it was in tracing code that should not be
rentered.

Create minimal rtas call that should be usable by guest machine check
code, use it there to call "ibm,nmi-interlock".

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/rtas.h      |  1 +
 arch/powerpc/kernel/entry_64.S       | 12 ++++++--
 arch/powerpc/kernel/rtas.c           | 43 ++++++++++++++++++++++++++++
 arch/powerpc/platforms/pseries/ras.c |  2 +-
 4 files changed, 54 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
index 3c1887351c71..4ffc499ce1ac 100644
--- a/arch/powerpc/include/asm/rtas.h
+++ b/arch/powerpc/include/asm/rtas.h

@@ -352,6 +352,7 @@ extern struct rtas_t rtas;
 extern int rtas_token(const char *service);
 extern int rtas_service_present(const char *service);
 extern int rtas_call(int token, int, int, int *, ...);
+extern int raw_rtas_call(int token, int, int, int *, ...);
 void rtas_call_unlocked(struct rtas_args *args, int token, int nargs,
 			int nret, ...);
 extern void __noreturn rtas_restart(char *cmd);

diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 51c5b681f70c..309abb677788 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S

@@ -759,6 +759,13 @@ _GLOBAL(enter_rtas)
 	li	r0,0
 	mtcr	r0
 
+	/* enter_rtas called from real-mode may not have irqs reconciled
+	 * but will always have interrupts disabled.
+	 */
+	mfmsr	r6
+	andi.	r7,r6,(MSR_IR|MSR_DR)
+	beq	2f
+
 #ifdef CONFIG_BUG
 	/* There is no way it is acceptable to get here with interrupts enabled,
 	 * check it with the asm equivalent of WARN_ON

@@ -769,10 +776,10 @@ _GLOBAL(enter_rtas)
 #endif
 
 	/* Hard-disable interrupts */
-	mfmsr	r6
 	rldicl	r7,r6,48,1
 	rotldi	r7,r7,16
 	mtmsrd	r7,1
+2:
 
 	/* Unfortunately, the stack pointer and the MSR are also clobbered,
 	 * so they are saved in the PACA which allows us to restore

@@ -795,7 +802,6 @@ _GLOBAL(enter_rtas)
 	ori	r9,r9,MSR_IR|MSR_DR|MSR_FE0|MSR_FE1|MSR_FP|MSR_RI|MSR_LE
 	andc	r6,r0,r9
 
-__enter_rtas:
 	sync				/* disable interrupts so SRR0/1 */
 	mtmsrd	r0			/* don't get trashed */
 

@@ -837,7 +843,7 @@ rtas_return_loc:
 	mtspr	SPRN_SRR1,r4
 	RFI_TO_KERNEL
 	b	.	/* prevent speculative execution */
-_ASM_NOKPROBE_SYMBOL(__enter_rtas)
+_ASM_NOKPROBE_SYMBOL(enter_rtas)
 _ASM_NOKPROBE_SYMBOL(rtas_return_loc)
 
 	.align	3

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index c5fa251b8950..a058dcfb6726 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c

@@ -450,6 +450,8 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...)
 	char *buff_copy = NULL;
 	int ret;
 
+	WARN_ON_ONCE((mfmsr() & (MSR_IR|MSR_DR)) != (MSR_IR|MSR_DR));
+
 	if (!rtas.entry || token == RTAS_UNKNOWN_SERVICE)
 		return -1;
 

@@ -483,6 +485,47 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...)
 }
 EXPORT_SYMBOL(rtas_call);
 
+/*
+ * Like rtas_call but no kmalloc or printk etc in error handling, so
+ * error won't go through log_error. No tracing, may be called in real mode.
+ */
+int notrace raw_rtas_call(int token, int nargs, int nret, int *outputs, ...)
+{
+	va_list list;
+	int i;
+	struct rtas_args *rtas_args;
+	int ret;
+
+	WARN_ON_ONCE((mfmsr() & MSR_EE));
+
+	if (!rtas.entry || token == RTAS_UNKNOWN_SERVICE)
+		return -1;
+
+	/*
+	 * Real mode must have MSR[EE]=0 and we prefer not to touch any
+	 * irq or preempt state (this may be called in machine check).
+	 */
+	preempt_disable_notrace();
+	arch_spin_lock(&rtas.lock);
+
+	/* We use the global rtas args buffer */
+	rtas_args = &rtas.args;
+
+	va_start(list, outputs);
+	va_rtas_call_unlocked(rtas_args, token, nargs, nret, list);
+	va_end(list);
+
+	if (nret > 1 && outputs != NULL)
+		for (i = 0; i < nret-1; ++i)
+			outputs[i] = be32_to_cpu(rtas_args->rets[i+1]);
+	ret = (nret > 0)? be32_to_cpu(rtas_args->rets[0]): 0;
+
+	arch_spin_unlock(&rtas.lock);
+	preempt_enable_notrace();
+
+	return ret;
+}
+
 /* For RTAS_BUSY (-2), delay for 1 millisecond.  For an extended busy status
  * code of 990n, perform the hinted delay of 10^n (last digit) milliseconds.
  */

diff --git a/arch/powerpc/platforms/pseries/ras.c b/arch/powerpc/platforms/pseries/ras.c
index c74d5e740922..e87f86f02569 100644
--- a/arch/powerpc/platforms/pseries/ras.c
+++ b/arch/powerpc/platforms/pseries/ras.c

@@ -458,7 +458,7 @@ static struct rtas_error_log *fwnmi_get_errinfo(struct pt_regs *regs)
  */
 static void fwnmi_release_errinfo(void)
 {
-	int ret = rtas_call(ibm_nmi_interlock_token, 0, 1, NULL);
+	int ret = raw_rtas_call(ibm_nmi_interlock_token, 0, 1, NULL);
 	if (ret != 0)
 		printk(KERN_ERR "FWNMI: nmi-interlock failed: %d\n", ret);
 }

-- 
2.23.0

Ganesh's on March 23, 2020 8:19 pm:

On 3/20/20 8:58 PM, Nicholas Piggin wrote:

quoted

rtas_call allocates and uses memory in failure paths, which is
not safe for RMA. It also calls local_irq_save() which may not be safe
in all real mode contexts.

Particularly machine check may run with interrupts not "reconciled",
and it may have hit while it was in tracing code that should not be
rentered.

Create minimal rtas call that should be usable by guest machine check
code, use it there to call "ibm,nmi-interlock".

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
  arch/powerpc/include/asm/rtas.h      |  1 +
  arch/powerpc/kernel/entry_64.S       | 12 ++++++--
  arch/powerpc/kernel/rtas.c           | 43 ++++++++++++++++++++++++++++
  arch/powerpc/platforms/pseries/ras.c |  2 +-
  4 files changed, 54 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
index 3c1887351c71..4ffc499ce1ac 100644
--- a/arch/powerpc/include/asm/rtas.h
+++ b/arch/powerpc/include/asm/rtas.h

@@ -352,6 +352,7 @@ extern struct rtas_t rtas;
  extern int rtas_token(const char *service);
  extern int rtas_service_present(const char *service);
  extern int rtas_call(int token, int, int, int *, ...);
+extern int raw_rtas_call(int token, int, int, int *, ...);
  void rtas_call_unlocked(struct rtas_args *args, int token, int nargs,
  			int nret, ...);
  extern void __noreturn rtas_restart(char *cmd);

diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 51c5b681f70c..309abb677788 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S

@@ -759,6 +759,13 @@ _GLOBAL(enter_rtas)
  	li	r0,0
  	mtcr	r0
  
+	/* enter_rtas called from real-mode may not have irqs reconciled
+	 * but will always have interrupts disabled.
+	 */
+	mfmsr	r6
+	andi.	r7,r6,(MSR_IR|MSR_DR)
+	beq	2f
+
  #ifdef CONFIG_BUG
  	/* There is no way it is acceptable to get here with interrupts enabled,
  	 * check it with the asm equivalent of WARN_ON

@@ -769,10 +776,10 @@ _GLOBAL(enter_rtas)
  #endif
  
  	/* Hard-disable interrupts */
-	mfmsr	r6
  	rldicl	r7,r6,48,1
  	rotldi	r7,r7,16
  	mtmsrd	r7,1
+2:
  
  	/* Unfortunately, the stack pointer and the MSR are also clobbered,
  	 * so they are saved in the PACA which allows us to restore

@@ -795,7 +802,6 @@ _GLOBAL(enter_rtas)
  	ori	r9,r9,MSR_IR|MSR_DR|MSR_FE0|MSR_FE1|MSR_FP|MSR_RI|MSR_LE
  	andc	r6,r0,r9
  
-__enter_rtas:
  	sync				/* disable interrupts so SRR0/1 */
  	mtmsrd	r0			/* don't get trashed */
  

@@ -837,7 +843,7 @@ rtas_return_loc:
  	mtspr	SPRN_SRR1,r4
  	RFI_TO_KERNEL
  	b	.	/* prevent speculative execution */
-_ASM_NOKPROBE_SYMBOL(__enter_rtas)
+_ASM_NOKPROBE_SYMBOL(enter_rtas)
  _ASM_NOKPROBE_SYMBOL(rtas_return_loc)
  
  	.align	3

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index c5fa251b8950..a058dcfb6726 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c

@@ -450,6 +450,8 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...)
  	char *buff_copy = NULL;
  	int ret;
  
+	WARN_ON_ONCE((mfmsr() & (MSR_IR|MSR_DR)) != (MSR_IR|MSR_DR));
+
  	if (!rtas.entry || token == RTAS_UNKNOWN_SERVICE)
  		return -1;
  

@@ -483,6 +485,47 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...)
  }
  EXPORT_SYMBOL(rtas_call);
  
+/*
+ * Like rtas_call but no kmalloc or printk etc in error handling, so
+ * error won't go through log_error. No tracing, may be called in real mode.
+ */
+int notrace raw_rtas_call(int token, int nargs, int nret, int *outputs, ...)
+{
+	va_list list;
+	int i;
+	struct rtas_args *rtas_args;
+	int ret;
+
+	WARN_ON_ONCE((mfmsr() & MSR_EE));
+
+	if (!rtas.entry || token == RTAS_UNKNOWN_SERVICE)
+		return -1;
+
+	/*
+	 * Real mode must have MSR[EE]=0 and we prefer not to touch any
+	 * irq or preempt state (this may be called in machine check).
+	 */
+	preempt_disable_notrace();
+	arch_spin_lock(&rtas.lock);

I wonder, if its ok to attempt for this lock in nested MCE.

It's not. It will deadlock if an MCE hits after "ibm,nmi-interlock" is
called and before the lock is released on the same CPU.

The struct actually isn't even that big, 84 bytes or so, and we're in
the dedicated machine check stack here so we could just put it on
stack AFAIKS?

Thanks,
Nick

On 3/20/20 8:58 PM, Nicholas Piggin wrote:

I wonder, if its ok to attempt for this lock in nested MCE.