* [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.