All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] powerpc/64: allow rtas to be called in real-mode, use this in machine check
@ 2020-03-20 15:28 Nicholas Piggin
  2020-03-23 10:19 ` Ganesh
  0 siblings, 1 reply; 3+ messages in thread
From: Nicholas Piggin @ 2020-03-20 15:28 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: Mahesh Salgaonkar, Ganesh Goudar, Nicholas Piggin

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


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] powerpc/64: allow rtas to be called in real-mode, use this in machine check
  2020-03-20 15:28 [PATCH] powerpc/64: allow rtas to be called in real-mode, use this in machine check Nicholas Piggin
@ 2020-03-23 10:19 ` Ganesh
  2020-03-23 10:29   ` Nicholas Piggin
  0 siblings, 1 reply; 3+ messages in thread
From: Ganesh @ 2020-03-23 10:19 UTC (permalink / raw)
  To: Nicholas Piggin, linuxppc-dev; +Cc: Mahesh Salgaonkar

[-- Attachment #1: Type: text/plain, Size: 5449 bytes --]



On 3/20/20 8:58 PM, Nicholas Piggin wrote:
> 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.

> +
> +	/* 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);
>   }


[-- Attachment #2: Type: text/html, Size: 5772 bytes --]

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] powerpc/64: allow rtas to be called in real-mode, use this in machine check
  2020-03-23 10:19 ` Ganesh
@ 2020-03-23 10:29   ` Nicholas Piggin
  0 siblings, 0 replies; 3+ messages in thread
From: Nicholas Piggin @ 2020-03-23 10:29 UTC (permalink / raw)
  To: Ganesh, linuxppc-dev; +Cc: Mahesh Salgaonkar

Ganesh's on March 23, 2020 8:19 pm:
> 
> 
> On 3/20/20 8:58 PM, Nicholas Piggin wrote:
>> 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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2020-03-23 21:24 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-20 15:28 [PATCH] powerpc/64: allow rtas to be called in real-mode, use this in machine check Nicholas Piggin
2020-03-23 10:19 ` Ganesh
2020-03-23 10:29   ` Nicholas Piggin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.