* Re: [PATCH v2 05/14] powerpc/pseries/ras: avoid calling rtas_token in NMI paths
From: Christophe Leroy @ 2020-04-03 14:30 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev
Cc: Mahesh Salgaonkar, Ganesh Goudar, Mahesh Salgaonkar
In-Reply-To: <20200403132622.130394-6-npiggin@gmail.com>
Le 03/04/2020 à 15:26, Nicholas Piggin a écrit :
> In the interest of reducing code and possible failures in the
> machine check and system reset paths, grab the "ibm,nmi-interlock"
> token at init time.
>
> Reviewed-by: Mahesh Salgaonkar <mahesh@linux.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/include/asm/firmware.h | 1 +
> arch/powerpc/platforms/pseries/ras.c | 2 +-
> arch/powerpc/platforms/pseries/setup.c | 13 ++++++++++---
> 3 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/firmware.h b/arch/powerpc/include/asm/firmware.h
> index ca33f4ef6cb4..6003c2e533a0 100644
> --- a/arch/powerpc/include/asm/firmware.h
> +++ b/arch/powerpc/include/asm/firmware.h
> @@ -128,6 +128,7 @@ extern void machine_check_fwnmi(void);
>
> /* This is true if we are using the firmware NMI handler (typically LPAR) */
> extern int fwnmi_active;
> +extern int ibm_nmi_interlock_token;
>
> extern unsigned int __start___fw_ftr_fixup, __stop___fw_ftr_fixup;
>
> diff --git a/arch/powerpc/platforms/pseries/ras.c b/arch/powerpc/platforms/pseries/ras.c
> index 1d7f973c647b..c74d5e740922 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(rtas_token("ibm,nmi-interlock"), 0, 1, NULL);
> + int ret = rtas_call(ibm_nmi_interlock_token, 0, 1, NULL);
> if (ret != 0)
> printk(KERN_ERR "FWNMI: nmi-interlock failed: %d\n", ret);
> }
> diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
> index 0c8421dd01ab..b582198be284 100644
> --- a/arch/powerpc/platforms/pseries/setup.c
> +++ b/arch/powerpc/platforms/pseries/setup.c
> @@ -83,6 +83,7 @@ unsigned long CMO_PageSize = (ASM_CONST(1) << IOMMU_PAGE_SHIFT_4K);
> EXPORT_SYMBOL(CMO_PageSize);
>
> int fwnmi_active; /* TRUE if an FWNMI handler is present */
> +int ibm_nmi_interlock_token;
>
> static void pSeries_show_cpuinfo(struct seq_file *m)
> {
> @@ -113,9 +114,14 @@ static void __init fwnmi_init(void)
> struct slb_entry *slb_ptr;
> size_t size;
> #endif
> + int ibm_nmi_register_token;
>
> - int ibm_nmi_register = rtas_token("ibm,nmi-register");
> - if (ibm_nmi_register == RTAS_UNKNOWN_SERVICE)
> + ibm_nmi_register_token = rtas_token("ibm,nmi-register");
> + if (ibm_nmi_register_token == RTAS_UNKNOWN_SERVICE)
> + return;
> +
> + ibm_nmi_interlock_token = rtas_token("ibm,nmi-interlock");
> + if (WARN_ON(ibm_nmi_interlock_token == RTAS_UNKNOWN_SERVICE))
> return;
>
> /* If the kernel's not linked at zero we point the firmware at low
> @@ -123,7 +129,8 @@ static void __init fwnmi_init(void)
> system_reset_addr = __pa(system_reset_fwnmi) - PHYSICAL_START;
> machine_check_addr = __pa(machine_check_fwnmi) - PHYSICAL_START;
>
> - if (0 == rtas_call(ibm_nmi_register, 2, 1, NULL, system_reset_addr,
> + if (0 == rtas_call(ibm_nmi_register_token, 2, 1, NULL,
> + system_reset_addr,
> machine_check_addr))
Alignment is wrong. And you could put system_reset_addr and
machine_check_addr on the same line to limit the number of lines of the if.
> fwnmi_active = 1;
>
>
Christophe
^ permalink raw reply
* Re: [PATCH v2 09/14] powerpc/pseries: limit machine check stack to 4GB
From: Christophe Leroy @ 2020-04-03 14:19 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev
Cc: Mahesh Salgaonkar, Ganesh Goudar, Mahesh Salgaonkar
In-Reply-To: <20200403132622.130394-10-npiggin@gmail.com>
Le 03/04/2020 à 15:26, Nicholas Piggin a écrit :
> This allows rtas_args to be put on the machine check stack, which
> avoids a lot of complications with re-entrancy deadlocks.
>
> Reviewed-by: Mahesh Salgaonkar <mahesh@linux.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/kernel/setup_64.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
> index e05e6dd67ae6..3a2428aa3d9a 100644
> --- a/arch/powerpc/kernel/setup_64.c
> +++ b/arch/powerpc/kernel/setup_64.c
> @@ -692,6 +692,9 @@ void __init exc_lvl_early_init(void)
> void __init emergency_stack_init(void)
> {
> u64 limit;
> +#ifdef CONFIG_PPC_BOOK3S_64
#ifdef not needed, see below
> + u64 mce_limit;
> +#endif
> unsigned int i;
>
> /*
> @@ -710,6 +713,18 @@ void __init emergency_stack_init(void)
> */
> limit = min(ppc64_bolted_size(), ppc64_rma_size);
>
> + /*
> + * Machine check on pseries calls rtas, but can't use the static
> + * rtas_args due to a machine check hitting while the lock is held.
> + * rtas args have to be under 4GB, so the machine check stack is
> + * limited to 4GB so args can be put on stack.
> + */
> +#ifdef CONFIG_PPC_BOOK3S_64
This ifdef is not needed. FW_FEATURE_LPAR is only possible on
CONFIG_PPC_BOOK3S_64 (indeed only on PSERIES or PS3). On others
firmware_has_feature(FW_FEATURE_LPAR) should return 0 at compile time.
> + mce_limit = limit;
> + if (firmware_has_feature(FW_FEATURE_LPAR) && mce_limit > 4UL*1024*1024*1024)
> + mce_limit = 4UL*1024*1024*1024;
You should use SZ_4G instead of hardcoding.
> +#endif
> +
> for_each_possible_cpu(i) {
> paca_ptrs[i]->emergency_sp = alloc_stack(limit, i) + THREAD_SIZE;
>
> @@ -718,7 +733,7 @@ void __init emergency_stack_init(void)
> paca_ptrs[i]->nmi_emergency_sp = alloc_stack(limit, i) + THREAD_SIZE;
>
> /* emergency stack for machine check exception handling. */
> - paca_ptrs[i]->mc_emergency_sp = alloc_stack(limit, i) + THREAD_SIZE;
> + paca_ptrs[i]->mc_emergency_sp = alloc_stack(mce_limit, i) + THREAD_SIZE;
> #endif
> }
> }
>
Christophe
^ permalink raw reply
* Re: [PATCH RESEND 1/4] uaccess: Add user_read_access_begin/end and user_write_access_begin/end
From: Russell King - ARM Linux admin @ 2020-04-03 13:37 UTC (permalink / raw)
To: Catalin Marinas
Cc: linux-arch, linuxppc-dev, Kees Cook, Christian Borntraeger,
airlied, hpa, linux-kernel, linux-mm, Paul Mackerras, Al Viro,
daniel, akpm, torvalds
In-Reply-To: <20200403112609.GB26633@mbp>
On Fri, Apr 03, 2020 at 12:26:10PM +0100, Catalin Marinas wrote:
> On Fri, Apr 03, 2020 at 01:58:31AM +0100, Al Viro wrote:
> > On Thu, Apr 02, 2020 at 11:35:57AM -0700, Kees Cook wrote:
> > > Yup, I think it's a weakness of the ARM implementation and I'd like to
> > > not extend it further. AFAIK we should never nest, but I would not be
> > > surprised at all if we did.
> > >
> > > If we were looking at a design goal for all architectures, I'd like
> > > to be doing what the public PaX patchset did for their memory access
> > > switching, which is to alarm if calling into "enable" found the access
> > > already enabled, etc. Such a condition would show an unexpected nesting
> > > (like we've seen with similar constructs with set_fs() not getting reset
> > > during an exception handler, etc etc).
> >
> > FWIW, maybe I'm misreading the ARM uaccess logics, but... it smells like
> > KERNEL_DS is somewhat more dangerous there than on e.g. x86.
> >
> > Look: with CONFIG_CPU_DOMAINS, set_fs(KERNEL_DS) tells MMU to ignore
> > per-page permission bits in DOMAIN_KERNEL (i.e. for kernel address
> > ranges), allowing them even if they would normally be denied. We need
> > that for actual uaccess loads/stores, since those use insns that pretend
> > to be done in user mode and we want them to access the kernel pages.
> > But that affects the normal loads/stores as well; unless I'm misreading
> > that code, it will ignore (supervisor) r/o on a page. And that's not
> > just for the code inside the uaccess blocks; *everything* done under
> > KERNEL_DS is subject to that.
>
> That's correct. Luckily this only affects ARMv5 and earlier. From ARMv6
> onwards, CONFIG_CPU_USE_DOMAINS is no longer selected and the uaccess
> instructions are just plain ldr/str.
>
> Russell should know the details on whether there was much choice. Since
> the kernel was living in the linear map with full rwx permissions, the
> KERNEL_DS overriding was probably not a concern and the ldrt/strt for
> uaccess deemed more secure. We also have weird permission setting
> pre-ARMv6 (or rather v6k) where RO user pages are writable from the
> kernel with standard str instructions (breaking CoW). I don't recall
> whether it was a choice made by the kernel or something the architecture
> enforced. The vectors page has to be kernel writable (and user RO) to
> store the TLS value in the absence of a TLS register but maybe we could
> do this via the linear alias together with the appropriate cache
> maintenance.
>
> From ARMv6, the domain overriding had the side-effect of ignoring the XN
> bit and causing random instruction fetches from ioremap() areas. So we
> had to remove the domain switching. We also gained a dedicated TLS
> register.
Indeed. On pre-ARMv6, we have the following choices for protection
attributes:
Page tables Control Reg Privileged User
AP S,R permission permission
00 0,0 No access No access
00 1,0 Read-only No access
00 0,1 Read-only Read-only
00 1,1 Unpredictable Unpredictable
01 X,X Read/Write No access
10 X,X Read/Write Read-only
11 X,X Read/Write Read/Write
We use S,R=1,0 under Linux because this allows us to read-protect
kernel pages without making them visible to userspace. If we
changed to S,R=0,1, then we could have our read-only permissions
for both kernel and userspace, drop domain switching, and use the
plain LDR/STR instructions, but we then lose the ability to
write-protect module executable code and other parts of kernel
space without making them visible to userspace.
So, it essentially boils down to making a choice - which set of
security features we think are the most important.
> I think uaccess_enable() could indeed switch the kernel domain if
> KERNEL_DS is set and move this out of set_fs(). It would reduce the
> window the kernel domain permissions are overridden. Anyway,
> uaccess_enable() appeared much later on arm when Russell introduced PAN
> (SMAP) like support by switching the user domain.
Yes, that would be a possibility. Another possibility would be to
eliminate as much usage of KERNEL_DS as possible - I've just found
one instance in sys_oabi-compat.c that can be eliminated (epoll_ctl)
but there's several there that can't with the current code structure,
and re-coding the contents of some fs/* functions to work around that
is a very bad idea. If there's some scope for rejigging some of the
fs/* code, it may be possible to elimate some other cases in there.
I notice that the fs/* code seems like some of the last remaining
users of KERNEL_DS, although I suspect that some aren't possible to
eliminate. :(
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up
^ permalink raw reply
* [PATCH v2 14/14] powerpc: make unrecoverable NMIs die instead of panic
From: Nicholas Piggin @ 2020-04-03 13:26 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Mahesh Salgaonkar, Ganesh Goudar, Nicholas Piggin
In-Reply-To: <20200403132622.130394-1-npiggin@gmail.com>
System Reset and Machine Check interrupts that are not recoverable due
to being nested or interrupting when RI=0 currently panic. This is
not necessary, and can often just kill the current context and recover.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/traps.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index ed7b7a6e2dc0..1f0277f8d40e 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -513,11 +513,11 @@ void system_reset_exception(struct pt_regs *regs)
#ifdef CONFIG_PPC_BOOK3S_64
BUG_ON(get_paca()->in_nmi == 0);
if (get_paca()->in_nmi > 1)
- nmi_panic(regs, "Unrecoverable nested System Reset");
+ die("Unrecoverable nested System Reset", regs, SIGABRT);
#endif
/* Must die if the interrupt is not recoverable */
if (!(regs->msr & MSR_RI))
- nmi_panic(regs, "Unrecoverable System Reset");
+ die("Unrecoverable System Reset", regs, SIGABRT);
if (saved_hsrrs) {
mtspr(SPRN_HSRR0, hsrr0);
@@ -858,7 +858,7 @@ void machine_check_exception(struct pt_regs *regs)
bail:
/* Must die if the interrupt is not recoverable */
if (!(regs->msr & MSR_RI))
- nmi_panic(regs, "Unrecoverable Machine check");
+ die("Unrecoverable Machine check", regs, SIGBUS);
}
void SMIException(struct pt_regs *regs)
--
2.23.0
^ permalink raw reply related
* [PATCH v2 13/14] powerpc/64s: system reset do not trace
From: Nicholas Piggin @ 2020-04-03 13:26 UTC (permalink / raw)
To: linuxppc-dev
Cc: Mahesh Salgaonkar, Naveen N . Rao, Ganesh Goudar, Nicholas Piggin
In-Reply-To: <20200403132622.130394-1-npiggin@gmail.com>
Similarly to the previous patch, do not trace system reset. This code
is used when there is a crash or hang, and tracing disturbs the system
more and has been known to crash in the crash handling path.
Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/traps.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 1845fd7e161a..ed7b7a6e2dc0 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -443,6 +443,9 @@ void system_reset_exception(struct pt_regs *regs)
unsigned long hsrr0, hsrr1;
bool nested = in_nmi();
bool saved_hsrrs = false;
+ u8 ftrace_enabled = local_paca->ftrace_enabled;
+
+ local_paca->ftrace_enabled = 0;
/*
* Avoid crashes in case of nested NMI exceptions. Recoverability
@@ -524,6 +527,8 @@ void system_reset_exception(struct pt_regs *regs)
if (!nested)
nmi_exit();
+ local_paca->ftrace_enabled = ftrace_enabled;
+
/* What should we do here? We could issue a shutdown or hard reset. */
}
--
2.23.0
^ permalink raw reply related
* [PATCH v2 12/14] powerpc/64s: machine check do not trace real-mode handler
From: Nicholas Piggin @ 2020-04-03 13:26 UTC (permalink / raw)
To: linuxppc-dev
Cc: Mahesh Salgaonkar, Naveen N . Rao, Ganesh Goudar, Nicholas Piggin
In-Reply-To: <20200403132622.130394-1-npiggin@gmail.com>
Rather than notrace annotations throughout a significant part of the
machine check code across kernel/ pseries/ and powernv/ which can
easily be broken and is infrequently tested, use paca->ftrace_enabled
to blanket-disable tracing of the real-mode non-maskable handler.
Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/mce.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/mce.c b/arch/powerpc/kernel/mce.c
index c1684be0d8b7..36e99adb8710 100644
--- a/arch/powerpc/kernel/mce.c
+++ b/arch/powerpc/kernel/mce.c
@@ -557,10 +557,13 @@ EXPORT_SYMBOL_GPL(machine_check_print_event_info);
*
* regs->nip and regs->msr contains srr0 and ssr1.
*/
-long machine_check_early(struct pt_regs *regs)
+long notrace machine_check_early(struct pt_regs *regs)
{
long handled = 0;
bool nested = in_nmi();
+ u8 ftrace_enabled = local_paca->ftrace_enabled;
+
+ local_paca->ftrace_enabled = 0;
if (!nested)
nmi_enter();
@@ -574,6 +577,7 @@ long machine_check_early(struct pt_regs *regs)
if (!nested)
nmi_exit();
+ local_paca->ftrace_enabled = ftrace_enabled;
return handled;
}
--
2.23.0
^ permalink raw reply related
* [PATCH v2 11/14] powerpc/64s: machine check interrupt update NMI accounting
From: Nicholas Piggin @ 2020-04-03 13:26 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Mahesh Salgaonkar, Ganesh Goudar, Nicholas Piggin
In-Reply-To: <20200403132622.130394-1-npiggin@gmail.com>
machine_check_early is taken as an NMI, so nmi_enter is used there.
machine_check_exception is no longer taken as an NMI (it's invoked
via irq_work in the case a machine check hits in kernel mode), so
remove the nmi_enter from that case.
In NMI context, hash faults don't try to refill the hash table, which
can lead to crashes accessing non-pinned kernel pages. System reset
still has this potential problem.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/mce.c | 7 +++++++
arch/powerpc/kernel/process.c | 2 +-
arch/powerpc/kernel/traps.c | 13 +------------
3 files changed, 9 insertions(+), 13 deletions(-)
diff --git a/arch/powerpc/kernel/mce.c b/arch/powerpc/kernel/mce.c
index 34c1001e9e8b..c1684be0d8b7 100644
--- a/arch/powerpc/kernel/mce.c
+++ b/arch/powerpc/kernel/mce.c
@@ -560,6 +560,9 @@ EXPORT_SYMBOL_GPL(machine_check_print_event_info);
long machine_check_early(struct pt_regs *regs)
{
long handled = 0;
+ bool nested = in_nmi();
+ if (!nested)
+ nmi_enter();
hv_nmi_check_nonrecoverable(regs);
@@ -568,6 +571,10 @@ long machine_check_early(struct pt_regs *regs)
*/
if (ppc_md.machine_check_early)
handled = ppc_md.machine_check_early(regs);
+
+ if (!nested)
+ nmi_exit();
+
return handled;
}
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 1dea4d280f6f..f06d20678611 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1421,7 +1421,7 @@ void show_regs(struct pt_regs * regs)
pr_cont("DAR: "REG" DSISR: %08lx ", regs->dar, regs->dsisr);
#endif
#ifdef CONFIG_PPC64
- pr_cont("IRQMASK: %lx ", regs->softe);
+ pr_cont("IRQMASK: %lx IN_NMI:%d IN_MCE:%d", regs->softe, (int)get_paca()->in_nmi, (int)get_paca()->in_mce);
#endif
#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
if (MSR_TM_ACTIVE(regs->msr))
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 82a3438300fd..1845fd7e161a 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -823,9 +823,6 @@ int machine_check_generic(struct pt_regs *regs)
void machine_check_exception(struct pt_regs *regs)
{
int recover = 0;
- bool nested = in_nmi();
- if (!nested)
- nmi_enter();
__this_cpu_inc(irq_stat.mce_exceptions);
@@ -851,20 +848,12 @@ void machine_check_exception(struct pt_regs *regs)
if (check_io_access(regs))
goto bail;
- if (!nested)
- nmi_exit();
-
die("Machine check", regs, SIGBUS);
+bail:
/* Must die if the interrupt is not recoverable */
if (!(regs->msr & MSR_RI))
nmi_panic(regs, "Unrecoverable Machine check");
-
- return;
-
-bail:
- if (!nested)
- nmi_exit();
}
void SMIException(struct pt_regs *regs)
--
2.23.0
^ permalink raw reply related
* [PATCH v2 10/14] powerpc/pseries: machine check use rtas_call_unlocked with args on stack
From: Nicholas Piggin @ 2020-04-03 13:26 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Mahesh Salgaonkar, Ganesh Goudar, Nicholas Piggin
In-Reply-To: <20200403132622.130394-1-npiggin@gmail.com>
With the previous patch, machine checks can use rtas_call_unlocked
which avoids the rtas spinlock which would deadlock if a machine
check hits while making an rtas call.
This also avoids the complex rtas error logging which has more rtas calls
and includes kmalloc (which can return memory beyond RMA, which would
also crash).
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/platforms/pseries/ras.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/pseries/ras.c b/arch/powerpc/platforms/pseries/ras.c
index 833ae34b7fec..6938261e4569 100644
--- a/arch/powerpc/platforms/pseries/ras.c
+++ b/arch/powerpc/platforms/pseries/ras.c
@@ -469,7 +469,15 @@ 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);
+ struct rtas_args rtas_args;
+ int ret;
+
+ /*
+ * On pseries, the machine check stack is limited to under 4GB, so
+ * args can be on-stack.
+ */
+ rtas_call_unlocked(&rtas_args, ibm_nmi_interlock_token, 0, 1, NULL);
+ ret = be32_to_cpu(rtas_args.rets[0]);
if (ret != 0)
printk(KERN_ERR "FWNMI: nmi-interlock failed: %d\n", ret);
}
--
2.23.0
^ permalink raw reply related
* [PATCH v2 09/14] powerpc/pseries: limit machine check stack to 4GB
From: Nicholas Piggin @ 2020-04-03 13:26 UTC (permalink / raw)
To: linuxppc-dev
Cc: Mahesh Salgaonkar, Ganesh Goudar, Mahesh Salgaonkar,
Nicholas Piggin
In-Reply-To: <20200403132622.130394-1-npiggin@gmail.com>
This allows rtas_args to be put on the machine check stack, which
avoids a lot of complications with re-entrancy deadlocks.
Reviewed-by: Mahesh Salgaonkar <mahesh@linux.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/setup_64.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index e05e6dd67ae6..3a2428aa3d9a 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -692,6 +692,9 @@ void __init exc_lvl_early_init(void)
void __init emergency_stack_init(void)
{
u64 limit;
+#ifdef CONFIG_PPC_BOOK3S_64
+ u64 mce_limit;
+#endif
unsigned int i;
/*
@@ -710,6 +713,18 @@ void __init emergency_stack_init(void)
*/
limit = min(ppc64_bolted_size(), ppc64_rma_size);
+ /*
+ * Machine check on pseries calls rtas, but can't use the static
+ * rtas_args due to a machine check hitting while the lock is held.
+ * rtas args have to be under 4GB, so the machine check stack is
+ * limited to 4GB so args can be put on stack.
+ */
+#ifdef CONFIG_PPC_BOOK3S_64
+ mce_limit = limit;
+ if (firmware_has_feature(FW_FEATURE_LPAR) && mce_limit > 4UL*1024*1024*1024)
+ mce_limit = 4UL*1024*1024*1024;
+#endif
+
for_each_possible_cpu(i) {
paca_ptrs[i]->emergency_sp = alloc_stack(limit, i) + THREAD_SIZE;
@@ -718,7 +733,7 @@ void __init emergency_stack_init(void)
paca_ptrs[i]->nmi_emergency_sp = alloc_stack(limit, i) + THREAD_SIZE;
/* emergency stack for machine check exception handling. */
- paca_ptrs[i]->mc_emergency_sp = alloc_stack(limit, i) + THREAD_SIZE;
+ paca_ptrs[i]->mc_emergency_sp = alloc_stack(mce_limit, i) + THREAD_SIZE;
#endif
}
}
--
2.23.0
^ permalink raw reply related
* [PATCH v2 08/14] powerpc/pseries/ras: fwnmi sreset should not interlock
From: Nicholas Piggin @ 2020-04-03 13:26 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Mahesh Salgaonkar, Ganesh Goudar, Nicholas Piggin
In-Reply-To: <20200403132622.130394-1-npiggin@gmail.com>
PAPR does not specify that fwnmi sreset should be interlocked, and
PowerVM (and therefore now QEMU) do not require it.
These "ibm,nmi-interlock" calls are ignored by firmware, but there
is a possibility that the sreset could have interrupted a machine
check and release the machine check's interlock too early, corrupting
it if another machine check came in.
This is an extremely rare case, but it should be fixed for clarity
and reducing the code executed in the sreset path. Firmware also
does not provide error information for the sreset case to look at, so
remove that comment.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/platforms/pseries/ras.c | 48 ++++++++++++++++++++--------
1 file changed, 34 insertions(+), 14 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/ras.c b/arch/powerpc/platforms/pseries/ras.c
index a40598e6e525..833ae34b7fec 100644
--- a/arch/powerpc/platforms/pseries/ras.c
+++ b/arch/powerpc/platforms/pseries/ras.c
@@ -406,6 +406,20 @@ static inline struct rtas_error_log *fwnmi_get_errlog(void)
return (struct rtas_error_log *)local_paca->mce_data_buf;
}
+static unsigned long *fwnmi_get_savep(struct pt_regs *regs)
+{
+ unsigned long savep_ra;
+
+ /* Mask top two bits */
+ savep_ra = regs->gpr[3] & ~(0x3UL << 62);
+ if (!VALID_FWNMI_BUFFER(savep_ra)) {
+ printk(KERN_ERR "FWNMI: corrupt r3 0x%016lx\n", regs->gpr[3]);
+ return NULL;
+ }
+
+ return __va(savep_ra);
+}
+
/*
* Get the error information for errors coming through the
* FWNMI vectors. The pt_regs' r3 will be updated to reflect
@@ -423,20 +437,15 @@ static inline struct rtas_error_log *fwnmi_get_errlog(void)
*/
static struct rtas_error_log *fwnmi_get_errinfo(struct pt_regs *regs)
{
- unsigned long savep_ra;
unsigned long *savep;
struct rtas_error_log *h;
- /* Mask top two bits */
- savep_ra = regs->gpr[3] & ~(0x3UL << 62);
-
- if (!VALID_FWNMI_BUFFER(savep_ra)) {
- printk(KERN_ERR "FWNMI: corrupt r3 0x%016lx\n", regs->gpr[3]);
+ savep = fwnmi_get_savep(regs);
+ if (!savep)
return NULL;
- }
- savep = __va(savep_ra);
- regs->gpr[3] = be64_to_cpu(savep[0]); /* restore original r3 */
+ /* restore original r3 */
+ regs->gpr[3] = be64_to_cpu(savep[0]);
h = (struct rtas_error_log *)&savep[1];
/* Use the per cpu buffer from paca to store rtas error log */
@@ -483,11 +492,22 @@ int pSeries_system_reset_exception(struct pt_regs *regs)
#endif
if (fwnmi_active) {
- struct rtas_error_log *errhdr = fwnmi_get_errinfo(regs);
- if (errhdr) {
- /* XXX Should look at FWNMI information */
- }
- fwnmi_release_errinfo();
+ unsigned long *savep;
+
+ /*
+ * Firmware (PowerVM and KVM) saves r3 to a save area like
+ * machine check, which is not exactly what PAPR (2.9)
+ * suggests but there is no way to detect otherwise, so this
+ * is the interface now.
+ *
+ * System resets do not save any error log or require an
+ * "ibm,nmi-interlock" rtas call to release.
+ */
+
+ savep = fwnmi_get_savep(regs);
+ /* restore original r3 */
+ if (savep)
+ regs->gpr[3] = be64_to_cpu(savep[0]);
}
if (smp_handle_nmi_ipi(regs))
--
2.23.0
^ permalink raw reply related
* [PATCH v2 07/14] powerpc/pseries/ras: fwnmi avoid modifying r3 in error case
From: Nicholas Piggin @ 2020-04-03 13:26 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Mahesh Salgaonkar, Ganesh Goudar, Nicholas Piggin
In-Reply-To: <20200403132622.130394-1-npiggin@gmail.com>
If there is some error with the fwnmi save area, r3 has already been
modified which doesn't help with debugging.
Only update r3 when to restore the saved value.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/platforms/pseries/ras.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/ras.c b/arch/powerpc/platforms/pseries/ras.c
index 9a37bda47468..a40598e6e525 100644
--- a/arch/powerpc/platforms/pseries/ras.c
+++ b/arch/powerpc/platforms/pseries/ras.c
@@ -423,18 +423,19 @@ static inline struct rtas_error_log *fwnmi_get_errlog(void)
*/
static struct rtas_error_log *fwnmi_get_errinfo(struct pt_regs *regs)
{
+ unsigned long savep_ra;
unsigned long *savep;
struct rtas_error_log *h;
/* Mask top two bits */
- regs->gpr[3] &= ~(0x3UL << 62);
+ savep_ra = regs->gpr[3] & ~(0x3UL << 62);
- if (!VALID_FWNMI_BUFFER(regs->gpr[3])) {
+ if (!VALID_FWNMI_BUFFER(savep_ra)) {
printk(KERN_ERR "FWNMI: corrupt r3 0x%016lx\n", regs->gpr[3]);
return NULL;
}
- savep = __va(regs->gpr[3]);
+ savep = __va(savep_ra);
regs->gpr[3] = be64_to_cpu(savep[0]); /* restore original r3 */
h = (struct rtas_error_log *)&savep[1];
--
2.23.0
^ permalink raw reply related
* [PATCH v2 06/14] powerpc/pseries/ras: FWNMI_VALID off by one
From: Nicholas Piggin @ 2020-04-03 13:26 UTC (permalink / raw)
To: linuxppc-dev
Cc: Mahesh Salgaonkar, Ganesh Goudar, Mahesh Salgaonkar,
Nicholas Piggin
In-Reply-To: <20200403132622.130394-1-npiggin@gmail.com>
This was discovered developing qemu fwnmi sreset support. This
off-by-one bug means the last 16 bytes of the rtas area can not
be used for a 16 byte save area.
It's not a serious bug, and QEMU implementation has to retain a
workaround for old kernels, but it's good to tighten it.
Acked-by: Mahesh Salgaonkar <mahesh@linux.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/platforms/pseries/ras.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/ras.c b/arch/powerpc/platforms/pseries/ras.c
index c74d5e740922..9a37bda47468 100644
--- a/arch/powerpc/platforms/pseries/ras.c
+++ b/arch/powerpc/platforms/pseries/ras.c
@@ -395,10 +395,11 @@ static irqreturn_t ras_error_interrupt(int irq, void *dev_id)
/*
* Some versions of FWNMI place the buffer inside the 4kB page starting at
* 0x7000. Other versions place it inside the rtas buffer. We check both.
+ * Minimum size of the buffer is 16 bytes.
*/
#define VALID_FWNMI_BUFFER(A) \
- ((((A) >= 0x7000) && ((A) < 0x7ff0)) || \
- (((A) >= rtas.base) && ((A) < (rtas.base + rtas.size - 16))))
+ ((((A) >= 0x7000) && ((A) <= 0x8000 - 16)) || \
+ (((A) >= rtas.base) && ((A) <= (rtas.base + rtas.size - 16))))
static inline struct rtas_error_log *fwnmi_get_errlog(void)
{
--
2.23.0
^ permalink raw reply related
* [PATCH v2 05/14] powerpc/pseries/ras: avoid calling rtas_token in NMI paths
From: Nicholas Piggin @ 2020-04-03 13:26 UTC (permalink / raw)
To: linuxppc-dev
Cc: Mahesh Salgaonkar, Ganesh Goudar, Mahesh Salgaonkar,
Nicholas Piggin
In-Reply-To: <20200403132622.130394-1-npiggin@gmail.com>
In the interest of reducing code and possible failures in the
machine check and system reset paths, grab the "ibm,nmi-interlock"
token at init time.
Reviewed-by: Mahesh Salgaonkar <mahesh@linux.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/firmware.h | 1 +
arch/powerpc/platforms/pseries/ras.c | 2 +-
arch/powerpc/platforms/pseries/setup.c | 13 ++++++++++---
3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/include/asm/firmware.h b/arch/powerpc/include/asm/firmware.h
index ca33f4ef6cb4..6003c2e533a0 100644
--- a/arch/powerpc/include/asm/firmware.h
+++ b/arch/powerpc/include/asm/firmware.h
@@ -128,6 +128,7 @@ extern void machine_check_fwnmi(void);
/* This is true if we are using the firmware NMI handler (typically LPAR) */
extern int fwnmi_active;
+extern int ibm_nmi_interlock_token;
extern unsigned int __start___fw_ftr_fixup, __stop___fw_ftr_fixup;
diff --git a/arch/powerpc/platforms/pseries/ras.c b/arch/powerpc/platforms/pseries/ras.c
index 1d7f973c647b..c74d5e740922 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(rtas_token("ibm,nmi-interlock"), 0, 1, NULL);
+ int ret = rtas_call(ibm_nmi_interlock_token, 0, 1, NULL);
if (ret != 0)
printk(KERN_ERR "FWNMI: nmi-interlock failed: %d\n", ret);
}
diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
index 0c8421dd01ab..b582198be284 100644
--- a/arch/powerpc/platforms/pseries/setup.c
+++ b/arch/powerpc/platforms/pseries/setup.c
@@ -83,6 +83,7 @@ unsigned long CMO_PageSize = (ASM_CONST(1) << IOMMU_PAGE_SHIFT_4K);
EXPORT_SYMBOL(CMO_PageSize);
int fwnmi_active; /* TRUE if an FWNMI handler is present */
+int ibm_nmi_interlock_token;
static void pSeries_show_cpuinfo(struct seq_file *m)
{
@@ -113,9 +114,14 @@ static void __init fwnmi_init(void)
struct slb_entry *slb_ptr;
size_t size;
#endif
+ int ibm_nmi_register_token;
- int ibm_nmi_register = rtas_token("ibm,nmi-register");
- if (ibm_nmi_register == RTAS_UNKNOWN_SERVICE)
+ ibm_nmi_register_token = rtas_token("ibm,nmi-register");
+ if (ibm_nmi_register_token == RTAS_UNKNOWN_SERVICE)
+ return;
+
+ ibm_nmi_interlock_token = rtas_token("ibm,nmi-interlock");
+ if (WARN_ON(ibm_nmi_interlock_token == RTAS_UNKNOWN_SERVICE))
return;
/* If the kernel's not linked at zero we point the firmware at low
@@ -123,7 +129,8 @@ static void __init fwnmi_init(void)
system_reset_addr = __pa(system_reset_fwnmi) - PHYSICAL_START;
machine_check_addr = __pa(machine_check_fwnmi) - PHYSICAL_START;
- if (0 == rtas_call(ibm_nmi_register, 2, 1, NULL, system_reset_addr,
+ if (0 == rtas_call(ibm_nmi_register_token, 2, 1, NULL,
+ system_reset_addr,
machine_check_addr))
fwnmi_active = 1;
--
2.23.0
^ permalink raw reply related
* [PATCH v2 04/14] powerpc/64s/exceptions: machine check reconcile irq state
From: Nicholas Piggin @ 2020-04-03 13:26 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Mahesh Salgaonkar, Ganesh Goudar, Nicholas Piggin
In-Reply-To: <20200403132622.130394-1-npiggin@gmail.com>
pseries fwnmi machine check code pops the soft-irq checks in rtas_call
(after the previous patch to remove rtas_token from this call path).
Rather than play whack a mole with these and forever having fragile
code, it seems better to have the early machine check handler perform
the same kind of reconcile as the other NMI interrupts.
WARNING: CPU: 0 PID: 493 at arch/powerpc/kernel/irq.c:343
CPU: 0 PID: 493 Comm: a Tainted: G W
NIP: c00000000001ed2c LR: c000000000042c40 CTR: 0000000000000000
REGS: c0000001fffd38b0 TRAP: 0700 Tainted: G W
MSR: 8000000000021003 <SF,ME,RI,LE> CR: 28000488 XER: 00000000
CFAR: c00000000001ec90 IRQMASK: 0
GPR00: c000000000043820 c0000001fffd3b40 c0000000012ba300 0000000000000000
GPR04: 0000000048000488 0000000000000000 0000000000000000 00000000deadbeef
GPR08: 0000000000000080 0000000000000000 0000000000000000 0000000000001001
GPR12: 0000000000000000 c0000000014a0000 0000000000000000 0000000000000000
GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR24: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR28: 0000000000000000 0000000000000001 c000000001360810 0000000000000000
NIP [c00000000001ed2c] arch_local_irq_restore.part.0+0xac/0x100
LR [c000000000042c40] unlock_rtas+0x30/0x90
Call Trace:
[c0000001fffd3b40] [c000000001360810] 0xc000000001360810 (unreliable)
[c0000001fffd3b60] [c000000000043820] rtas_call+0x1c0/0x280
[c0000001fffd3bb0] [c0000000000dc328] fwnmi_release_errinfo+0x38/0x70
[c0000001fffd3c10] [c0000000000dcd8c] pseries_machine_check_realmode+0x1dc/0x540
[c0000001fffd3cd0] [c00000000003fe04] machine_check_early+0x54/0x70
[c0000001fffd3d00] [c000000000008384] machine_check_early_common+0x134/0x1f0
--- interrupt: 200 at 0x13f1307c8
LR = 0x7fff888b8528
Instruction dump:
60000000 7d2000a6 71298000 41820068 39200002 7d210164 4bffff9c 60000000
60000000 7d2000a6 71298000 4c820020 <0fe00000> 4e800020 60000000 60000000
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/exceptions-64s.S | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index a42b73efb1a9..072772803b7c 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -1116,11 +1116,30 @@ END_FTR_SECTION_IFSET(CPU_FTR_HVMODE)
li r10,MSR_RI
mtmsrd r10,1
+ /*
+ * Set IRQS_ALL_DISABLED and save PACAIRQHAPPENED (see
+ * system_reset_common)
+ */
+ li r10,IRQS_ALL_DISABLED
+ stb r10,PACAIRQSOFTMASK(r13)
+ lbz r10,PACAIRQHAPPENED(r13)
+ std r10,RESULT(r1)
+ ori r10,r10,PACA_IRQ_HARD_DIS
+ stb r10,PACAIRQHAPPENED(r13)
+
addi r3,r1,STACK_FRAME_OVERHEAD
bl machine_check_early
std r3,RESULT(r1) /* Save result */
ld r12,_MSR(r1)
+ /*
+ * Restore soft mask settings.
+ */
+ ld r10,RESULT(r1)
+ stb r10,PACAIRQHAPPENED(r13)
+ ld r10,SOFTE(r1)
+ stb r10,PACAIRQSOFTMASK(r13)
+
#ifdef CONFIG_PPC_P7_NAP
/*
* Check if thread was in power saving mode. We come here when any
--
2.23.0
^ permalink raw reply related
* [PATCH v2 03/14] powerpc/64s/exceptions: Change irq reconcile for NMIs from reusing _DAR to RESULT
From: Nicholas Piggin @ 2020-04-03 13:26 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Mahesh Salgaonkar, Ganesh Goudar, Nicholas Piggin
In-Reply-To: <20200403132622.130394-1-npiggin@gmail.com>
A spare interrupt stack slot is needed to save irq state when
reconciling NMIs (sreset and decrementer soft-nmi). _DAR is used
for this, but we want to reconcile machine checks as well, which
do use _DAR. Switch to using RESULT instead, as it's used by
system calls.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/exceptions-64s.S | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index 3322000316ab..a42b73efb1a9 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -939,13 +939,13 @@ EXC_COMMON_BEGIN(system_reset_common)
* the right thing. We do not want to reconcile because that goes
* through irq tracing which we don't want in NMI.
*
- * Save PACAIRQHAPPENED to _DAR (otherwise unused), and set HARD_DIS
+ * Save PACAIRQHAPPENED to RESULT (otherwise unused), and set HARD_DIS
* as we are running with MSR[EE]=0.
*/
li r10,IRQS_ALL_DISABLED
stb r10,PACAIRQSOFTMASK(r13)
lbz r10,PACAIRQHAPPENED(r13)
- std r10,_DAR(r1)
+ std r10,RESULT(r1)
ori r10,r10,PACA_IRQ_HARD_DIS
stb r10,PACAIRQHAPPENED(r13)
@@ -966,7 +966,7 @@ EXC_COMMON_BEGIN(system_reset_common)
/*
* Restore soft mask settings.
*/
- ld r10,_DAR(r1)
+ ld r10,RESULT(r1)
stb r10,PACAIRQHAPPENED(r13)
ld r10,SOFTE(r1)
stb r10,PACAIRQSOFTMASK(r13)
@@ -2743,7 +2743,7 @@ EXC_COMMON_BEGIN(soft_nmi_common)
li r10,IRQS_ALL_DISABLED
stb r10,PACAIRQSOFTMASK(r13)
lbz r10,PACAIRQHAPPENED(r13)
- std r10,_DAR(r1)
+ std r10,RESULT(r1)
ori r10,r10,PACA_IRQ_HARD_DIS
stb r10,PACAIRQHAPPENED(r13)
@@ -2757,7 +2757,7 @@ EXC_COMMON_BEGIN(soft_nmi_common)
/*
* Restore soft mask settings.
*/
- ld r10,_DAR(r1)
+ ld r10,RESULT(r1)
stb r10,PACAIRQHAPPENED(r13)
ld r10,SOFTE(r1)
stb r10,PACAIRQSOFTMASK(r13)
--
2.23.0
^ permalink raw reply related
* [PATCH v2 02/14] powerpc/64s/exceptions: Fix in_mce accounting in unrecoverable path
From: Nicholas Piggin @ 2020-04-03 13:26 UTC (permalink / raw)
To: linuxppc-dev
Cc: Mahesh Salgaonkar, Ganesh Goudar, Mahesh Salgaonkar,
Nicholas Piggin
In-Reply-To: <20200403132622.130394-1-npiggin@gmail.com>
Acked-by: Mahesh Salgaonkar <mahesh@linux.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/exceptions-64s.S | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index bbf3109c5cba..3322000316ab 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -1267,6 +1267,10 @@ END_FTR_SECTION_IFSET(CPU_FTR_HVMODE)
andc r10,r10,r3
mtmsrd r10
+ lhz r12,PACA_IN_MCE(r13)
+ subi r12,r12,1
+ sth r12,PACA_IN_MCE(r13)
+
/* Invoke machine_check_exception to print MCE event and panic. */
addi r3,r1,STACK_FRAME_OVERHEAD
bl machine_check_exception
--
2.23.0
^ permalink raw reply related
* [PATCH v2 01/14] powerpc/64s/exception: Fix machine check no-loss idle wakeup
From: Nicholas Piggin @ 2020-04-03 13:26 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Mahesh Salgaonkar, Ganesh Goudar, Nicholas Piggin
In-Reply-To: <20200403132622.130394-1-npiggin@gmail.com>
The architecture allows for machine check exceptions to cause idle
wakeups which resume at the 0x200 address which has to return via
the idle wakeup code, but the early machine check handler is run
first.
The case of a no state-loss sleep is broken because the early
handler uses non-volatile register r1 , which is needed for the wakeup
protocol, but it is not restored.
Fix this by loading r1 from the MCE exception frame before returning
to the idle wakeup code. Also update the comment which has become
stale since the idle rewrite in C.
Fixes: 10d91611f426d ("powerpc/64s: Reimplement book3s idle code in C")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/exceptions-64s.S | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index 728ccb0f560c..bbf3109c5cba 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -1224,17 +1224,19 @@ EXC_COMMON_BEGIN(machine_check_idle_common)
bl machine_check_queue_event
/*
- * We have not used any non-volatile GPRs here, and as a rule
- * most exception code including machine check does not.
- * Therefore PACA_NAPSTATELOST does not need to be set. Idle
- * wakeup will restore volatile registers.
+ * GPR-loss wakeups are relatively straightforward, because the
+ * idle sleep code has saved all non-volatile registers on its
+ * own stack, and r1 in PACAR1.
*
- * Load the original SRR1 into r3 for pnv_powersave_wakeup_mce.
+ * For no-loss wakeups the r1 and lr registers used by the
+ * early machine check handler have to be restored first. r2 is
+ * the kernel TOC, so no need to restore it.
*
* Then decrement MCE nesting after finishing with the stack.
*/
ld r3,_MSR(r1)
ld r4,_LINK(r1)
+ ld r1,GPR1(r1)
lhz r11,PACA_IN_MCE(r13)
subi r11,r11,1
@@ -1243,7 +1245,7 @@ EXC_COMMON_BEGIN(machine_check_idle_common)
mtlr r4
rlwinm r10,r3,47-31,30,31
cmpwi cr1,r10,2
- bltlr cr1 /* no state loss, return to idle caller */
+ bltlr cr1 /* no state loss, return to idle caller with r3=SRR1 */
b idle_return_gpr_loss
#endif
--
2.23.0
^ permalink raw reply related
* [PATCH v2 00/14] powerpc/64: machine check and system reset fixes
From: Nicholas Piggin @ 2020-04-03 13:26 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Mahesh Salgaonkar, Ganesh Goudar, Nicholas Piggin
There's a bunch of problems we hit bringing up fwnmi sreset and testing
with mce injection on QEMU. Mostly pseries issues.
This series of fixes applies on top of next-test, the machine
check reconcile patch won't apply cleanly to previous kernels but
it might want to be backported. We can do that after upstreaming.
This doesn't solve all known problems yet, but fwnmi machine check
and system reset injection in QEMU is significantly better. There
will be more to come but these should be ready for review now.
Thanks,
Nick
v2:
- Added a couple more fixes
- Review comments and tags
- Re-tested with some fixes to my qemu machine check injection patches
Nicholas Piggin (14):
powerpc/64s/exception: Fix machine check no-loss idle wakeup
powerpc/64s/exceptions: Fix in_mce accounting in unrecoverable path
powerpc/64s/exceptions: Change irq reconcile for NMIs from reusing
_DAR to RESULT
powerpc/64s/exceptions: machine check reconcile irq state
powerpc/pseries/ras: avoid calling rtas_token in NMI paths
powerpc/pseries/ras: FWNMI_VALID off by one
powerpc/pseries/ras: fwnmi avoid modifying r3 in error case
powerpc/pseries/ras: fwnmi sreset should not interlock
powerpc/pseries: limit machine check stack to 4GB
powerpc/pseries: machine check use rtas_call_unlocked with args on
stack
powerpc/64s: machine check interrupt update NMI accounting
powerpc/64s: machine check do not trace real-mode handler
powerpc/64s: system reset do not trace
powerpc: make unrecoverable NMIs die instead of panic
arch/powerpc/include/asm/firmware.h | 1 +
arch/powerpc/kernel/exceptions-64s.S | 47 ++++++++++++++-----
arch/powerpc/kernel/mce.c | 13 +++++-
arch/powerpc/kernel/process.c | 2 +-
arch/powerpc/kernel/setup_64.c | 17 ++++++-
arch/powerpc/kernel/traps.c | 24 ++++------
arch/powerpc/platforms/pseries/ras.c | 62 +++++++++++++++++++-------
arch/powerpc/platforms/pseries/setup.c | 13 ++++--
8 files changed, 131 insertions(+), 48 deletions(-)
--
2.23.0
^ permalink raw reply
* [PATCH] powerpc: improve ppc_save_regs
From: Nicholas Piggin @ 2020-04-03 13:10 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin
Make ppc_save_regs a bit more useful:
- Set NIP to our caller rather rather than the caller's caller (which is
what we save to LR in the stack frame).
- Set SOFTE to the current irq soft-mask state rather than
uninitialised.
- Zero CFAR rather than leave it uninitialised.
In qemu, injecting a nmi to an idle CPU gives a nicer stack trace (note
NIP, IRQMASK, CFAR).
Oops: System Reset, sig: 6 [#1]
LE PAGE_SIZE=64K MMU=Hash PREEMPT SMP NR_CPUS=2048 NUMA PowerNV
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.6.0-rc2-00429-ga76e38fd80bf #1277
NIP: c0000000000b6e5c LR: c0000000000b6e5c CTR: c000000000b06270
REGS: c00000000173fb08 TRAP: 0100 Not tainted
MSR: 9000000000001033 <SF,HV,ME,IR,DR,RI,LE> CR: 28000224 XER: 00000000
CFAR: c0000000016a2128 IRQMASK: c00000000173fc80
GPR00: c0000000000b6e5c c00000000173fc80 c000000001743400 c00000000173fb08
GPR04: 0000000000000000 0000000000000000 0000000000000008 0000000000000001
GPR08: 00000001fea80000 0000000000000000 0000000000000000 ffffffffffffffff
GPR12: c000000000b06270 c000000001930000 00000000300026c0 0000000000000000
GPR16: 0000000000000000 0000000000000000 0000000000000003 c0000000016a2128
GPR20: c0000001ffc97148 0000000000000001 c000000000f289a8 0000000000080000
GPR24: c0000000016e1480 000000011dc870ba 0000000000000000 0000000000000003
GPR28: c0000000016a2128 c0000001ffc97148 c0000000016a2260 0000000000000003
NIP [c0000000000b6e5c] power9_idle_type+0x5c/0x70
LR [c0000000000b6e5c] power9_idle_type+0x5c/0x70
Call Trace:
[c00000000173fc80] [c0000000000b6e5c] power9_idle_type+0x5c/0x70 (unreliable)
[c00000000173fcb0] [c000000000b062b0] stop_loop+0x40/0x60
[c00000000173fce0] [c000000000b022d8] cpuidle_enter_state+0xa8/0x660
[c00000000173fd60] [c000000000b0292c] cpuidle_enter+0x4c/0x70
[c00000000173fda0] [c00000000017624c] call_cpuidle+0x4c/0x90
[c00000000173fdc0] [c000000000176768] do_idle+0x338/0x460
[c00000000173fe60] [c000000000176b3c] cpu_startup_entry+0x3c/0x40
[c00000000173fe90] [c0000000000126b4] rest_init+0x124/0x140
[c00000000173fed0] [c0000000010948d4] start_kernel+0x938/0x988
[c00000000173ff90] [c00000000000cdcc] start_here_common+0x1c/0x20
Oops: System Reset, sig: 6 [#1]
LE PAGE_SIZE=64K MMU=Hash PREEMPT SMP NR_CPUS=2048 NUMA PowerNV
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.6.0-rc2-00430-gddce91b8712f #1278
NIP: c00000000001d150 LR: c0000000000b6e5c CTR: c000000000b06270
REGS: c00000000173fb08 TRAP: 0100 Not tainted
MSR: 9000000000001033 <SF,HV,ME,IR,DR,RI,LE> CR: 28000224 XER: 00000000
CFAR: 0000000000000000 IRQMASK: 1
GPR00: c0000000000b6e5c c00000000173fc80 c000000001743400 c00000000173fb08
GPR04: 0000000000000000 0000000000000000 0000000000000008 0000000000000001
GPR08: 00000001fea80000 0000000000000000 0000000000000000 ffffffffffffffff
GPR12: c000000000b06270 c000000001930000 00000000300026c0 0000000000000000
GPR16: 0000000000000000 0000000000000000 0000000000000003 c0000000016a2128
GPR20: c0000001ffc97148 0000000000000001 c000000000f289a8 0000000000080000
GPR24: c0000000016e1480 00000000b68db8ce 0000000000000000 0000000000000003
GPR28: c0000000016a2128 c0000001ffc97148 c0000000016a2260 0000000000000003
NIP [c00000000001d150] replay_system_reset+0x30/0xa0
LR [c0000000000b6e5c] power9_idle_type+0x5c/0x70
Call Trace:
[c00000000173fc80] [c0000000000b6e5c] power9_idle_type+0x5c/0x70 (unreliable)
[c00000000173fcb0] [c000000000b062b0] stop_loop+0x40/0x60
[c00000000173fce0] [c000000000b022d8] cpuidle_enter_state+0xa8/0x660
[c00000000173fd60] [c000000000b0292c] cpuidle_enter+0x4c/0x70
[c00000000173fda0] [c00000000017624c] call_cpuidle+0x4c/0x90
[c00000000173fdc0] [c000000000176768] do_idle+0x338/0x460
[c00000000173fe60] [c000000000176b38] cpu_startup_entry+0x38/0x40
[c00000000173fe90] [c0000000000126b4] rest_init+0x124/0x140
[c00000000173fed0] [c0000000010948d4] start_kernel+0x938/0x988
[c00000000173ff90] [c00000000000cdcc] start_here_common+0x1c/0x20
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
This becomes more important after implementing irq replay in C.
arch/powerpc/kernel/ppc_save_regs.S | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/ppc_save_regs.S b/arch/powerpc/kernel/ppc_save_regs.S
index f3bd0bbf2ae8..2d4d21bb46a9 100644
--- a/arch/powerpc/kernel/ppc_save_regs.S
+++ b/arch/powerpc/kernel/ppc_save_regs.S
@@ -55,14 +55,17 @@ _GLOBAL(ppc_save_regs)
PPC_STL r29,29*SZL(r3)
PPC_STL r30,30*SZL(r3)
PPC_STL r31,31*SZL(r3)
+ lbz r0,PACAIRQSOFTMASK(r13)
+ PPC_STL r0,SOFTE-STACK_FRAME_OVERHEAD(r3)
#endif
/* go up one stack frame for SP */
PPC_LL r4,0(r1)
PPC_STL r4,1*SZL(r3)
/* get caller's LR */
PPC_LL r0,LRSAVE(r4)
- PPC_STL r0,_NIP-STACK_FRAME_OVERHEAD(r3)
PPC_STL r0,_LINK-STACK_FRAME_OVERHEAD(r3)
+ mflr r0
+ PPC_STL r0,_NIP-STACK_FRAME_OVERHEAD(r3)
mfmsr r0
PPC_STL r0,_MSR-STACK_FRAME_OVERHEAD(r3)
mfctr r0
@@ -73,4 +76,5 @@ _GLOBAL(ppc_save_regs)
PPC_STL r0,_CCR-STACK_FRAME_OVERHEAD(r3)
li r0,0
PPC_STL r0,_TRAP-STACK_FRAME_OVERHEAD(r3)
+ PPC_STL r0,ORIG_GPR3-STACK_FRAME_OVERHEAD(r3)
blr
--
2.23.0
^ permalink raw reply related
* Re: [PATCH v11 3/8] powerpc/perf: consolidate read_user_stack_32
From: Michal Suchánek @ 2020-04-03 11:51 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Mark Rutland, Gustavo Luiz Duarte, Alexander Shishkin,
Sebastian Andrzej Siewior, linux-kernel, Paul Mackerras,
Jiri Olsa, Rob Herring, Michael Neuling, Eric Richter,
Masahiro Yamada, Nayna Jain, Peter Zijlstra, Ingo Molnar,
Hari Bathini, Jordan Niethe, Valentin Schneider, Arnd Bergmann,
Arnaldo Carvalho de Melo, Alexander Viro, Jonathan Cameron,
Namhyung Kim, Thomas Gleixner, Andy Shevchenko, Allison Randal,
Greg Kroah-Hartman, Claudio Carvalho, Mauro Carvalho Chehab,
Eric W. Biederman, linux-fsdevel, linuxppc-dev, David S. Miller,
Thiago Jung Bauermann
In-Reply-To: <1585913065.zoacp2kzsv.astroid@bobo.none>
On Fri, Apr 03, 2020 at 09:26:27PM +1000, Nicholas Piggin wrote:
> Michal Suchánek's on April 3, 2020 8:52 pm:
> > Hello,
> >
> > there are 3 variants of the function
> >
> > read_user_stack_64
> >
> > 32bit read_user_stack_32
> > 64bit read_user_Stack_32
>
> Right.
>
> > On Fri, Apr 03, 2020 at 05:13:25PM +1000, Nicholas Piggin wrote:
> [...]
> >> #endif /* CONFIG_PPC64 */
> >>
> >> +static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
> >> +{
> >> + return __read_user_stack(ptr, ret, sizeof(*ret));
> > Does not work for 64bit read_user_stack_32 ^ this should be 4.
> >
> > Other than that it should preserve the existing logic just fine.
>
> sizeof(int) == 4 on 64bit so it should work.
>
Right, the type is different for the 32bit and 64bit version.
Thanks
Michal
^ permalink raw reply
* Re: [PATCH v11 3/8] powerpc/perf: consolidate read_user_stack_32
From: Nicholas Piggin @ 2020-04-03 11:26 UTC (permalink / raw)
To: Michal Suchánek
Cc: Mark Rutland, Gustavo Luiz Duarte, Alexander Shishkin,
Sebastian Andrzej Siewior, linux-kernel, Paul Mackerras,
Jiri Olsa, Rob Herring, Michael Neuling, Eric Richter,
Masahiro Yamada, Nayna Jain, Peter Zijlstra, Ingo Molnar,
Hari Bathini, Jordan Niethe, Valentin Schneider, Arnd Bergmann,
Arnaldo Carvalho de Melo, Alexander Viro, Jonathan Cameron,
Namhyung Kim, Thomas Gleixner, Andy Shevchenko, Allison Randal,
Greg Kroah-Hartman, Claudio Carvalho, Mauro Carvalho Chehab,
Eric W. Biederman, linux-fsdevel, linuxppc-dev, David S. Miller,
Thiago Jung Bauermann
In-Reply-To: <20200403105234.GX25468@kitsune.suse.cz>
Michal Suchánek's on April 3, 2020 8:52 pm:
> Hello,
>
> there are 3 variants of the function
>
> read_user_stack_64
>
> 32bit read_user_stack_32
> 64bit read_user_Stack_32
Right.
> On Fri, Apr 03, 2020 at 05:13:25PM +1000, Nicholas Piggin wrote:
[...]
>> #endif /* CONFIG_PPC64 */
>>
>> +static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
>> +{
>> + return __read_user_stack(ptr, ret, sizeof(*ret));
> Does not work for 64bit read_user_stack_32 ^ this should be 4.
>
> Other than that it should preserve the existing logic just fine.
sizeof(int) == 4 on 64bit so it should work.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH RESEND 1/4] uaccess: Add user_read_access_begin/end and user_write_access_begin/end
From: Catalin Marinas @ 2020-04-03 11:26 UTC (permalink / raw)
To: Al Viro
Cc: linux-arch, linuxppc-dev, Kees Cook, Christian Borntraeger,
airlied, hpa, linux-kernel, Russell King, linux-mm,
Paul Mackerras, daniel, akpm, torvalds
In-Reply-To: <20200403005831.GI23230@ZenIV.linux.org.uk>
On Fri, Apr 03, 2020 at 01:58:31AM +0100, Al Viro wrote:
> On Thu, Apr 02, 2020 at 11:35:57AM -0700, Kees Cook wrote:
> > Yup, I think it's a weakness of the ARM implementation and I'd like to
> > not extend it further. AFAIK we should never nest, but I would not be
> > surprised at all if we did.
> >
> > If we were looking at a design goal for all architectures, I'd like
> > to be doing what the public PaX patchset did for their memory access
> > switching, which is to alarm if calling into "enable" found the access
> > already enabled, etc. Such a condition would show an unexpected nesting
> > (like we've seen with similar constructs with set_fs() not getting reset
> > during an exception handler, etc etc).
>
> FWIW, maybe I'm misreading the ARM uaccess logics, but... it smells like
> KERNEL_DS is somewhat more dangerous there than on e.g. x86.
>
> Look: with CONFIG_CPU_DOMAINS, set_fs(KERNEL_DS) tells MMU to ignore
> per-page permission bits in DOMAIN_KERNEL (i.e. for kernel address
> ranges), allowing them even if they would normally be denied. We need
> that for actual uaccess loads/stores, since those use insns that pretend
> to be done in user mode and we want them to access the kernel pages.
> But that affects the normal loads/stores as well; unless I'm misreading
> that code, it will ignore (supervisor) r/o on a page. And that's not
> just for the code inside the uaccess blocks; *everything* done under
> KERNEL_DS is subject to that.
That's correct. Luckily this only affects ARMv5 and earlier. From ARMv6
onwards, CONFIG_CPU_USE_DOMAINS is no longer selected and the uaccess
instructions are just plain ldr/str.
Russell should know the details on whether there was much choice. Since
the kernel was living in the linear map with full rwx permissions, the
KERNEL_DS overriding was probably not a concern and the ldrt/strt for
uaccess deemed more secure. We also have weird permission setting
pre-ARMv6 (or rather v6k) where RO user pages are writable from the
kernel with standard str instructions (breaking CoW). I don't recall
whether it was a choice made by the kernel or something the architecture
enforced. The vectors page has to be kernel writable (and user RO) to
store the TLS value in the absence of a TLS register but maybe we could
do this via the linear alias together with the appropriate cache
maintenance.
From ARMv6, the domain overriding had the side-effect of ignoring the XN
bit and causing random instruction fetches from ioremap() areas. So we
had to remove the domain switching. We also gained a dedicated TLS
register.
> Why do we do that (modify_domain(), that is) inside set_fs() and not
> in uaccess_enable() et.al.?
I think uaccess_enable() could indeed switch the kernel domain if
KERNEL_DS is set and move this out of set_fs(). It would reduce the
window the kernel domain permissions are overridden. Anyway,
uaccess_enable() appeared much later on arm when Russell introduced PAN
(SMAP) like support by switching the user domain.
--
Catalin
^ permalink raw reply
* Re: [PATCH 1/4] scsi: cxlflash: Access interrupt trigger page from xive directly
From: Cédric Le Goater @ 2020-04-03 11:19 UTC (permalink / raw)
To: Matthew R. Ochs, Frederic Barrat
Cc: ukrishn, ajd, haren, groug, linuxppc-dev, christophe_lombard
In-Reply-To: <20200402231949.GA5469@p8tul1-build.aus.stglabs.ibm.com>
On 4/3/20 1:19 AM, Matthew R. Ochs wrote:
> On Thu, Apr 02, 2020 at 05:43:49PM +0200, Frederic Barrat wrote:
>> xive is already mapping the trigger page in kernel space and it can be
>> accessed through standard APIs, so let's reuse it and simplify the code.
>>
>> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
>> ---
>> drivers/scsi/cxlflash/ocxl_hw.c | 17 +++++++----------
>> drivers/scsi/cxlflash/ocxl_hw.h | 1 -
>> 2 files changed, 7 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/scsi/cxlflash/ocxl_hw.c b/drivers/scsi/cxlflash/ocxl_hw.c
>> index 7018cd802569..59452850f71c 100644
>> --- a/drivers/scsi/cxlflash/ocxl_hw.c
>> +++ b/drivers/scsi/cxlflash/ocxl_hw.c
>> @@ -15,7 +15,8 @@
>> #include <linux/pseudo_fs.h>
>> #include <linux/poll.h>
>> #include <linux/sched/signal.h>
>> -
>> +#include <linux/interrupt.h>
>> +#include <asm/xive.h>
>> #include <misc/ocxl.h>
>>
>> #include <uapi/misc/cxl.h>
>> @@ -180,7 +181,7 @@ static int afu_map_irq(u64 flags, struct ocxlflash_context *ctx, int num,
>> struct ocxl_hw_afu *afu = ctx->hw_afu;
>> struct device *dev = afu->dev;
>> struct ocxlflash_irqs *irq;
>> - void __iomem *vtrig;
>> + struct xive_irq_data *xd;
>> u32 virq;
>> int rc = 0;
>>
>> @@ -204,15 +205,14 @@ static int afu_map_irq(u64 flags, struct ocxlflash_context *ctx, int num,
>> goto err1;
>> }
>>
>> - vtrig = ioremap(irq->ptrig, PAGE_SIZE);
>> - if (unlikely(!vtrig)) {
>> - dev_err(dev, "%s: Trigger page mapping failed\n", __func__);
>> - rc = -ENOMEM;
>> + xd = irq_get_handler_data(virq);
>> + if (unlikely(!xd)) {
>> + dev_err(dev, "%s: Can't get interrupt data\n", __func__);
>
> The removal of setting the return code injects a bug should this error leg
> ever be encountered. So we should either keep the rc statement e.g. -EINVAL,
> -ENXIO, -ENODEV, etc., or remove this error leg. I lean towards keeping the
> statement.
I like ENXIO for XIVE matters.
C.
>> goto err2;
>> }
>>
>> irq->virq = virq;
>> - irq->vtrig = vtrig;
>> + irq->vtrig = xd->trig_mmio;
>> out:
>>
^ permalink raw reply
* Re: [PATCH v2 1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR
From: Nicholas Piggin @ 2020-04-03 11:05 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
Cc: linux-arch, Daniel Borkmann, x86, linux-kernel,
Alexei Starovoitov, Masami Hiramatsu, Linus Torvalds
In-Reply-To: <558b6131-60b4-98b7-dc40-25d8dacea05a@c-s.fr>
Christophe Leroy's on April 3, 2020 8:31 pm:
>
>
> Le 03/04/2020 à 11:35, Nicholas Piggin a écrit :
>> There is no need to allow user accesses when probing kernel addresses.
>
> I just discovered the following commit
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=75a1a607bb7e6d918be3aca11ec2214a275392f4
>
> This commit adds probe_kernel_read_strict() and probe_kernel_write_strict().
>
> When reading the commit log, I understand that probe_kernel_read() may
> be used to access some user memory. Which will not work anymore with
> your patch.
Hmm, I looked at _strict but obviously not hard enough. Good catch.
I don't think probe_kernel_read() should ever access user memory,
the comment certainly says it doesn't, but that patch sort of implies
that they do.
I think it's wrong. The non-_strict maybe could return userspace data to
you if you did pass a user address? I don't see why that shouldn't just
be disallowed always though.
And if the _strict version is required to be safe, then it seems like a
bug or security issue to just allow everyone that doesn't explicitly
override it to use the default implementation.
Also, the way the weak linkage is done in that patch, means parisc and
um archs that were previously overriding probe_kernel_read() now get
the default probe_kernel_read_strict(), which would be wrong for them.
>
> Isn't it probe_kernel_read_strict() and probe_kernel_write_strict() that
> you want to add ?
>
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> v2:
>> - Enable for all powerpc (suggested by Christophe)
>> - Fold helper function together (Christophe)
>> - Rename uaccess.c to maccess.c to match kernel/maccess.c.
>>
>> arch/powerpc/include/asm/uaccess.h | 25 +++++++++++++++-------
>> arch/powerpc/lib/Makefile | 2 +-
>> arch/powerpc/lib/maccess.c | 34 ++++++++++++++++++++++++++++++
>
> x86 does it in mm/maccess.c
Yeah I'll fix that up, thanks.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v11 3/8] powerpc/perf: consolidate read_user_stack_32
From: Michal Suchánek @ 2020-04-03 10:52 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Mark Rutland, Gustavo Luiz Duarte, Alexander Shishkin,
Sebastian Andrzej Siewior, linux-kernel, Paul Mackerras,
Jiri Olsa, Rob Herring, Michael Neuling, Eric Richter,
Masahiro Yamada, Nayna Jain, Peter Zijlstra, Ingo Molnar,
Hari Bathini, Jordan Niethe, Valentin Schneider, Arnd Bergmann,
Arnaldo Carvalho de Melo, Alexander Viro, Jonathan Cameron,
Namhyung Kim, Thomas Gleixner, Andy Shevchenko, Allison Randal,
Greg Kroah-Hartman, Claudio Carvalho, Mauro Carvalho Chehab,
Eric W. Biederman, linux-fsdevel, linuxppc-dev, David S. Miller,
Thiago Jung Bauermann
In-Reply-To: <1585896170.ohti800w9v.astroid@bobo.none>
Hello,
there are 3 variants of the function
read_user_stack_64
32bit read_user_stack_32
64bit read_user_Stack_32
On Fri, Apr 03, 2020 at 05:13:25PM +1000, Nicholas Piggin wrote:
> Michal Suchánek's on March 25, 2020 5:38 am:
> > On Tue, Mar 24, 2020 at 06:48:20PM +1000, Nicholas Piggin wrote:
> >> Michal Suchanek's on March 19, 2020 10:19 pm:
> >> > There are two almost identical copies for 32bit and 64bit.
> >> >
> >> > The function is used only in 32bit code which will be split out in next
> >> > patch so consolidate to one function.
> >> >
> >> > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> >> > Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
> >> > ---
> >> > v6: new patch
> >> > v8: move the consolidated function out of the ifdef block.
> >> > v11: rebase on top of def0bfdbd603
> >> > ---
> >> > arch/powerpc/perf/callchain.c | 48 +++++++++++++++++------------------
> >> > 1 file changed, 24 insertions(+), 24 deletions(-)
> >> >
> >> > diff --git a/arch/powerpc/perf/callchain.c b/arch/powerpc/perf/callchain.c
> >> > index cbc251981209..c9a78c6e4361 100644
> >> > --- a/arch/powerpc/perf/callchain.c
> >> > +++ b/arch/powerpc/perf/callchain.c
> >> > @@ -161,18 +161,6 @@ static int read_user_stack_64(unsigned long __user *ptr, unsigned long *ret)
> >> > return read_user_stack_slow(ptr, ret, 8);
> >> > }
> >> >
> >> > -static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
> >> > -{
> >> > - if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned int) ||
> >> > - ((unsigned long)ptr & 3))
> >> > - return -EFAULT;
> >> > -
> >> > - if (!probe_user_read(ret, ptr, sizeof(*ret)))
> >> > - return 0;
> >> > -
> >> > - return read_user_stack_slow(ptr, ret, 4);
> >> > -}
> >> > -
> >> > static inline int valid_user_sp(unsigned long sp, int is_64)
> >> > {
> >> > if (!sp || (sp & 7) || sp > (is_64 ? TASK_SIZE : 0x100000000UL) - 32)
> >> > @@ -277,19 +265,9 @@ static void perf_callchain_user_64(struct perf_callchain_entry_ctx *entry,
> >> > }
> >> >
> >> > #else /* CONFIG_PPC64 */
> >> > -/*
> >> > - * On 32-bit we just access the address and let hash_page create a
> >> > - * HPTE if necessary, so there is no need to fall back to reading
> >> > - * the page tables. Since this is called at interrupt level,
> >> > - * do_page_fault() won't treat a DSI as a page fault.
> >> > - */
> >> > -static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
> >> > +static int read_user_stack_slow(void __user *ptr, void *buf, int nb)
> >> > {
> >> > - if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned int) ||
> >> > - ((unsigned long)ptr & 3))
> >> > - return -EFAULT;
> >> > -
> >> > - return probe_user_read(ret, ptr, sizeof(*ret));
> >> > + return 0;
> >> > }
> >> >
> >> > static inline void perf_callchain_user_64(struct perf_callchain_entry_ctx *entry,
> >> > @@ -312,6 +290,28 @@ static inline int valid_user_sp(unsigned long sp, int is_64)
> >> >
> >> > #endif /* CONFIG_PPC64 */
> >> >
> >> > +/*
> >> > + * On 32-bit we just access the address and let hash_page create a
> >> > + * HPTE if necessary, so there is no need to fall back to reading
> >> > + * the page tables. Since this is called at interrupt level,
> >> > + * do_page_fault() won't treat a DSI as a page fault.
> >> > + */
> >>
> >> The comment is actually probably better to stay in the 32-bit
> >> read_user_stack_slow implementation. Is that function defined
> >> on 32-bit purely so that you can use IS_ENABLED()? In that case
> > It documents the IS_ENABLED() and that's where it is. The 32bit
> > definition is only a technical detail.
>
> Sorry for the late reply, busy trying to fix bugs in the C rewrite
> series. I don't think it is the right place, it should be in the
> ppc32 implementation detail. ppc64 has an equivalent comment at the
> top of its read_user_stack functions.
>
> >> I would prefer to put a BUG() there which makes it self documenting.
> > Which will cause checkpatch complaints about introducing new BUG() which
> > is frowned on.
>
> It's fine in this case, that warning is about not introducing
> runtime bugs, but this wouldn't be.
>
> But... I actually don't like adding read_user_stack_slow on 32-bit
> and especially not just to make IS_ENABLED work.
>
> IMO this would be better if you really want to consolidate it
>
> ---
>
> diff --git a/arch/powerpc/perf/callchain.c b/arch/powerpc/perf/callchain.c
> index cbc251981209..ca3a599b3f54 100644
> --- a/arch/powerpc/perf/callchain.c
> +++ b/arch/powerpc/perf/callchain.c
> @@ -108,7 +108,7 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx *entry, struct pt_regs *re
> * interrupt context, so if the access faults, we read the page tables
> * to find which page (if any) is mapped and access it directly.
> */
> -static int read_user_stack_slow(void __user *ptr, void *buf, int nb)
> +static int read_user_stack_slow(const void __user *ptr, void *buf, int nb)
> {
> int ret = -EFAULT;
> pgd_t *pgdir;
> @@ -149,28 +149,21 @@ static int read_user_stack_slow(void __user *ptr, void *buf, int nb)
> return ret;
> }
>
> -static int read_user_stack_64(unsigned long __user *ptr, unsigned long *ret)
> +static int __read_user_stack(const void __user *ptr, void *ret, size_t size)
> {
> - if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned long) ||
> - ((unsigned long)ptr & 7))
> + if ((unsigned long)ptr > TASK_SIZE - size ||
> + ((unsigned long)ptr & (size - 1)))
> return -EFAULT;
>
> - if (!probe_user_read(ret, ptr, sizeof(*ret)))
> + if (!probe_user_read(ret, ptr, size))
> return 0;
>
> - return read_user_stack_slow(ptr, ret, 8);
> + return read_user_stack_slow(ptr, ret, size);
> }
>
> -static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
> +static int read_user_stack_64(unsigned long __user *ptr, unsigned long *ret)
> {
> - if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned int) ||
> - ((unsigned long)ptr & 3))
> - return -EFAULT;
> -
> - if (!probe_user_read(ret, ptr, sizeof(*ret)))
> - return 0;
> -
> - return read_user_stack_slow(ptr, ret, 4);
> + return __read_user_stack(ptr, ret, sizeof(*ret));
> }
>
> static inline int valid_user_sp(unsigned long sp, int is_64)
> @@ -283,13 +276,13 @@ static void perf_callchain_user_64(struct perf_callchain_entry_ctx *entry,
> * the page tables. Since this is called at interrupt level,
> * do_page_fault() won't treat a DSI as a page fault.
> */
> -static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
> +static int __read_user_stack(const void __user *ptr, void *ret, size_t size)
> {
> - if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned int) ||
> - ((unsigned long)ptr & 3))
> + if ((unsigned long)ptr > TASK_SIZE - size ||
> + ((unsigned long)ptr & (size - 1)))
> return -EFAULT;
>
> - return probe_user_read(ret, ptr, sizeof(*ret));
> + return probe_user_read(ret, ptr, size);
> }
>
> static inline void perf_callchain_user_64(struct perf_callchain_entry_ctx *entry,
> @@ -312,6 +305,11 @@ static inline int valid_user_sp(unsigned long sp, int is_64)
>
> #endif /* CONFIG_PPC64 */
>
> +static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
> +{
> + return __read_user_stack(ptr, ret, sizeof(*ret));
Does not work for 64bit read_user_stack_32 ^ this should be 4.
Other than that it should preserve the existing logic just fine.
Thanks
Michal
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox