* [RFC PATCH 01/12] powerpc/tm: Remove struct thread_info param from tm_reclaim_thread()
From: Cyril Bur @ 2018-02-20 0:22 UTC (permalink / raw)
To: mikey, benh, linuxppc-dev
In-Reply-To: <20180220002241.29648-1-cyrilbur@gmail.com>
tm_reclaim_thread() doesn't use the parameter anymore, both callers have
to bother getting it as they have no need for a struct thread_info
either.
It was previously used but became unused in dc3106690b20 ("powerpc: tm:
Always use fp_state and vr_state to store live registers")
Just remove it and adjust the callers.
Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
arch/powerpc/kernel/process.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 1738c4127b32..77dc6d8288eb 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -850,8 +850,7 @@ static inline bool tm_enabled(struct task_struct *tsk)
return tsk && tsk->thread.regs && (tsk->thread.regs->msr & MSR_TM);
}
-static void tm_reclaim_thread(struct thread_struct *thr,
- struct thread_info *ti, uint8_t cause)
+static void tm_reclaim_thread(struct thread_struct *thr, uint8_t cause)
{
/*
* Use the current MSR TM suspended bit to track if we have
@@ -898,7 +897,7 @@ static void tm_reclaim_thread(struct thread_struct *thr,
void tm_reclaim_current(uint8_t cause)
{
tm_enable();
- tm_reclaim_thread(¤t->thread, current_thread_info(), cause);
+ tm_reclaim_thread(¤t->thread, cause);
}
static inline void tm_reclaim_task(struct task_struct *tsk)
@@ -929,7 +928,7 @@ static inline void tm_reclaim_task(struct task_struct *tsk)
thr->regs->ccr, thr->regs->msr,
thr->regs->trap);
- tm_reclaim_thread(thr, task_thread_info(tsk), TM_CAUSE_RESCHED);
+ tm_reclaim_thread(thr, TM_CAUSE_RESCHED);
TM_DEBUG("--- tm_reclaim on pid %d complete\n",
tsk->pid);
--
2.16.2
^ permalink raw reply related
* [RFC PATCH 04/12] selftests/powerpc: Use less common thread names
From: Cyril Bur @ 2018-02-20 0:22 UTC (permalink / raw)
To: mikey, benh, linuxppc-dev
In-Reply-To: <20180220002241.29648-1-cyrilbur@gmail.com>
"ping" and "pong" (in particular "ping") are common names. If a
selftests causes a kernel BUG_ON or any kind of backtrace the process
name is displayed. Setting a more unique name avoids confusion as to
which process caused the problem.
Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
tools/testing/selftests/powerpc/tm/tm-unavailable.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/powerpc/tm/tm-unavailable.c b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
index e6a0fad2bfd0..bcfa8add5748 100644
--- a/tools/testing/selftests/powerpc/tm/tm-unavailable.c
+++ b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
@@ -315,7 +315,7 @@ void test_fp_vec(int fp, int vec, pthread_attr_t *attr)
rc = pthread_create(&t0, attr, ping, (void *) &flags);
if (rc)
pr_err(rc, "pthread_create()");
- rc = pthread_setname_np(t0, "ping");
+ rc = pthread_setname_np(t0, "tm-unavailable-ping");
if (rc)
pr_warn(rc, "pthread_setname_np");
rc = pthread_join(t0, &ret_value);
@@ -359,7 +359,7 @@ int main(int argc, char **argv)
pr_err(rc, "pthread_create()");
/* Name it for systemtap convenience */
- rc = pthread_setname_np(t1, "pong");
+ rc = pthread_setname_np(t1, "tm-unavailable-pong");
if (rc)
pr_warn(rc, "pthread_create()");
--
2.16.2
^ permalink raw reply related
* [RFC PATCH 11/12] [WIP] powerpc/tm: Afterthoughts
From: Cyril Bur @ 2018-02-20 0:22 UTC (permalink / raw)
To: mikey, benh, linuxppc-dev
In-Reply-To: <20180220002241.29648-1-cyrilbur@gmail.com>
---
arch/powerpc/kernel/process.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 674f75c56172..6ce41ee62b24 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1079,6 +1079,12 @@ static inline void __switch_to_tm(struct task_struct *prev,
if (!MSR_TM_ACTIVE(prev->thread.regs->msr) && prev->thread.load_tm == 0)
prev->thread.regs->msr &= ~MSR_TM;
}
+
+ /*
+ * Now that we're reclaiming on kernel entry, we should never
+ * get here still with user checkpointed state on the CPU
+ */
+ BUG_ON(MSR_TM_ACTIVE(mfmsr()));
}
/*
@@ -1326,7 +1332,17 @@ struct task_struct *__switch_to(struct task_struct *prev,
}
if (current_thread_info()->task->thread.regs) {
- restore_math(current_thread_info()->task->thread.regs);
+ /*
+ * Calling this now has reloaded the live state, which
+ * gets overwritten with the checkpointed state right
+ * before the trecheckpoint. BUT the MSR still has
+ * that the live state is on the CPU, which it isn't.
+ *
+ * restore_math(current_thread_info()->task->thread.regs);
+ * Therefore:
+ */
+ if (!MSR_TM_ACTIVE(current_thread_info()->task->thread.regs->msr))
+ restore_math(current_thread_info()->task->thread.regs);
/*
* The copy-paste buffer can only store into foreign real
--
2.16.2
^ permalink raw reply related
* [RFC PATCH 08/12] [WIP] powerpc/tm: Fix *unavailable_tm exceptions
From: Cyril Bur @ 2018-02-20 0:22 UTC (permalink / raw)
To: mikey, benh, linuxppc-dev
In-Reply-To: <20180220002241.29648-1-cyrilbur@gmail.com>
---
arch/powerpc/kernel/process.c | 11 ++++++++++-
arch/powerpc/kernel/traps.c | 3 ---
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 574b05fe7d66..8a32fd062a2b 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -500,11 +500,20 @@ void giveup_all(struct task_struct *tsk)
usermsr = tsk->thread.regs->msr;
+ /*
+ * The *_unavailable_tm() functions might call this in a
+ * transaction but with not FP or VEC or VSX meaning that the
+ * if condition below will be true, this is bad since we will
+ * have preformed a reclaim but not set the TIF flag which
+ * must be set in order to trigger the recheckpoint.
+ *
+ * possibleTODO: Move setting the TIF flag into reclaim code
+ */
+ check_if_tm_restore_required(tsk);
if ((usermsr & msr_all_available) == 0)
return;
msr_check_and_set(msr_all_available);
- check_if_tm_restore_required(tsk);
WARN_ON((usermsr & MSR_VSX) && !((usermsr & MSR_FP) && (usermsr & MSR_VEC)));
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 1e48d157196a..dccfcaf4f603 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -1728,7 +1728,6 @@ void fp_unavailable_tm(struct pt_regs *regs)
* If VMX is in use, the VRs now hold checkpointed values,
* so we don't want to load the VRs from the thread_struct.
*/
- tm_recheckpoint(¤t->thread);
}
void altivec_unavailable_tm(struct pt_regs *regs)
@@ -1742,7 +1741,6 @@ void altivec_unavailable_tm(struct pt_regs *regs)
regs->nip, regs->msr);
tm_reclaim_current(TM_CAUSE_FAC_UNAV);
current->thread.load_vec = 1;
- tm_recheckpoint(¤t->thread);
current->thread.used_vr = 1;
}
@@ -1767,7 +1765,6 @@ void vsx_unavailable_tm(struct pt_regs *regs)
current->thread.load_vec = 1;
current->thread.load_fp = 1;
- tm_recheckpoint(¤t->thread);
}
#endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
--
2.16.2
^ permalink raw reply related
* [RFC PATCH 10/12] [WIP] powerpc/tm: Correctly save/restore checkpointed sprs
From: Cyril Bur @ 2018-02-20 0:22 UTC (permalink / raw)
To: mikey, benh, linuxppc-dev
In-Reply-To: <20180220002241.29648-1-cyrilbur@gmail.com>
---
arch/powerpc/kernel/process.c | 57 +++++++++++++++++++++++++++++++++++++++++--
arch/powerpc/kernel/ptrace.c | 9 +++----
2 files changed, 58 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index cd3ae80a6878..674f75c56172 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -859,6 +859,8 @@ static inline bool tm_enabled(struct task_struct *tsk)
return tsk && tsk->thread.regs && (tsk->thread.regs->msr & MSR_TM);
}
+static inline void save_sprs(struct thread_struct *t);
+
static void tm_reclaim_thread(struct thread_struct *thr, uint8_t cause)
{
/*
@@ -879,6 +881,8 @@ static void tm_reclaim_thread(struct thread_struct *thr, uint8_t cause)
if (!MSR_TM_SUSPENDED(mfmsr()))
return;
+ save_sprs(thr);
+
giveup_all(container_of(thr, struct task_struct, thread));
tm_reclaim(thr, cause);
@@ -991,6 +995,37 @@ void tm_recheckpoint(struct thread_struct *thread)
__tm_recheckpoint(thread);
+ /*
+ * This is a stripped down restore_sprs(), we need to do this
+ * now as we might go straight out to userspace and currently
+ * the checkpointed values are on the CPU.
+ *
+ * TODO: Improve
+ */
+#ifdef CONFIG_ALTIVEC
+ if (cpu_has_feature(CPU_FTR_ALTIVEC))
+ mtspr(SPRN_VRSAVE, thread->vrsave);
+#endif
+#ifdef CONFIG_PPC_BOOK3S_64
+ if (cpu_has_feature(CPU_FTR_DSCR)) {
+ u64 dscr = get_paca()->dscr_default;
+ if (thread->dscr_inherit)
+ dscr = thread->dscr;
+
+ mtspr(SPRN_DSCR, dscr);
+ }
+
+ if (cpu_has_feature(CPU_FTR_ARCH_207S)) {
+ /* The EBB regs aren't checkpointed */
+ mtspr(SPRN_FSCR, thread->fscr);
+
+ mtspr(SPRN_TAR, thread->tar);
+ }
+
+ /* I think we don't need to */
+ if (cpu_has_feature(CPU_FTR_ARCH_300))
+ mtspr(SPRN_TIDR, thread->tidr);
+#endif
local_irq_restore(flags);
}
@@ -1193,6 +1228,11 @@ struct task_struct *__switch_to(struct task_struct *prev,
#endif
new_thread = &new->thread;
+ /*
+ * Why not &prev->thread; ?
+ * What is the difference between &prev->thread and
+ * ¤t->thread ?
+ */
old_thread = ¤t->thread;
WARN_ON(!irqs_disabled());
@@ -1237,8 +1277,16 @@ struct task_struct *__switch_to(struct task_struct *prev,
/*
* We need to save SPRs before treclaim/trecheckpoint as these will
* change a number of them.
+ *
+ * Because we're now reclaiming on kernel entry, we've had to
+ * already save them. Don't do it again.
+ * Note: To deliver a signal in the signal context, we'll have
+ * turned off TM because we don't want the signal context to
+ * have the transactional state of the main thread - what if
+ * we go through switch to at that point? Can we?
*/
- save_sprs(&prev->thread);
+ if (!prev->thread.regs || !MSR_TM_ACTIVE(prev->thread.regs->msr))
+ save_sprs(&prev->thread);
/* Save FPU, Altivec, VSX and SPE state */
giveup_all(prev);
@@ -1260,8 +1308,13 @@ struct task_struct *__switch_to(struct task_struct *prev,
* for this is we manually create a stack frame for new tasks that
* directly returns through ret_from_fork() or
* ret_from_kernel_thread(). See copy_thread() for details.
+ *
+ * It isn't stricly nessesary that we avoid the restore here
+ * because we'll simply restore again after the recheckpoint,
+ * but we can avoid it for performance reasons.
*/
- restore_sprs(old_thread, new_thread);
+ if (!new_thread->regs || !MSR_TM_ACTIVE(new_thread->regs->msr))
+ restore_sprs(old_thread, new_thread);
last = _switch(old_thread, new_thread);
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index ca72d7391d40..16001987ba71 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -135,12 +135,9 @@ static void flush_tmregs_to_thread(struct task_struct *tsk)
if ((!cpu_has_feature(CPU_FTR_TM)) || (tsk != current))
return;
- if (MSR_TM_SUSPENDED(mfmsr())) {
- tm_reclaim_current(TM_CAUSE_SIGNAL);
- } else {
- tm_enable();
- tm_save_sprs(&(tsk->thread));
- }
+ BUG_ON(MSR_TM_SUSPENDED(mfmsr()));
+ tm_enable();
+ tm_save_sprs(&(tsk->thread));
}
#else
static inline void flush_tmregs_to_thread(struct task_struct *tsk) { }
--
2.16.2
^ permalink raw reply related
* [RFC PATCH 12/12] [WIP] selftests/powerpc: Remove incorrect tm-syscall selftest
From: Cyril Bur @ 2018-02-20 0:22 UTC (permalink / raw)
To: mikey, benh, linuxppc-dev
In-Reply-To: <20180220002241.29648-1-cyrilbur@gmail.com>
Currently we perform transactional memory work at late as possible.
That is we run in the kernel with the userspace checkpointed state on
the CPU untill we absolultely must remove it and store it away. Likely
a process switch, but possibly also signals or ptrace.
What this means is that if userspace does a system call in suspended
mode, it is possible that we will handle the system call and return
them without the need to to a reclaim/recheckpoint and so they can
expect to resume their transaction.
This is what tm-syscall tests for - the ability to perform a system
call in suspended state and still resume it afterwards.
TM reworks have meant that we now deal with any transactional state on
entry to the kernel, no matter the reason for entry (some expections
apply). We will categorically doom any suspended transaction that makes
a system call, making that transaction unresumeable.
This test will now always fail no matter what. I would like to note
here that this new behaviour does not break userspace at all. Hardware
Transactional Memory gives zero guarantee of forward progress and any
correct userspace has already had and will always have to implement a
non HTM fallback. Relying on this specific kernel behaviour also meant
relying on the stars aligning in the hardware such that there was no
cache overlaps and that it had a large enough footprint to handle
any system call without dooming a transaction.
---
tools/testing/selftests/powerpc/tm/Makefile | 4 +-
.../testing/selftests/powerpc/tm/tm-syscall-asm.S | 28 ------
tools/testing/selftests/powerpc/tm/tm-syscall.c | 106 ---------------------
3 files changed, 1 insertion(+), 137 deletions(-)
delete mode 100644 tools/testing/selftests/powerpc/tm/tm-syscall-asm.S
delete mode 100644 tools/testing/selftests/powerpc/tm/tm-syscall.c
diff --git a/tools/testing/selftests/powerpc/tm/Makefile b/tools/testing/selftests/powerpc/tm/Makefile
index 7a1e53297588..88d6edffcb24 100644
--- a/tools/testing/selftests/powerpc/tm/Makefile
+++ b/tools/testing/selftests/powerpc/tm/Makefile
@@ -2,7 +2,7 @@
SIGNAL_CONTEXT_CHK_TESTS := tm-signal-context-chk-gpr tm-signal-context-chk-fpu \
tm-signal-context-chk-vmx tm-signal-context-chk-vsx
-TEST_GEN_PROGS := tm-resched-dscr tm-syscall tm-signal-msr-resv tm-signal-stack \
+TEST_GEN_PROGS := tm-resched-dscr tm-signal-msr-resv tm-signal-stack \
tm-vmxcopy tm-fork tm-tar tm-tmspr tm-vmx-unavail tm-unavailable tm-trap \
tm-signal-drop-transaction \
$(SIGNAL_CONTEXT_CHK_TESTS)
@@ -13,8 +13,6 @@ $(TEST_GEN_PROGS): ../harness.c ../utils.c
CFLAGS += -mhtm
-$(OUTPUT)/tm-syscall: tm-syscall-asm.S
-$(OUTPUT)/tm-syscall: CFLAGS += -I../../../../../usr/include
$(OUTPUT)/tm-tmspr: CFLAGS += -pthread
$(OUTPUT)/tm-vmx-unavail: CFLAGS += -pthread -m64
$(OUTPUT)/tm-resched-dscr: ../pmu/lib.o
diff --git a/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S b/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S
deleted file mode 100644
index bd1ca25febe4..000000000000
--- a/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S
+++ /dev/null
@@ -1,28 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#include <ppc-asm.h>
-#include <asm/unistd.h>
-
- .text
-FUNC_START(getppid_tm_active)
- tbegin.
- beq 1f
- li r0, __NR_getppid
- sc
- tend.
- blr
-1:
- li r3, -1
- blr
-
-FUNC_START(getppid_tm_suspended)
- tbegin.
- beq 1f
- li r0, __NR_getppid
- tsuspend.
- sc
- tresume.
- tend.
- blr
-1:
- li r3, -1
- blr
diff --git a/tools/testing/selftests/powerpc/tm/tm-syscall.c b/tools/testing/selftests/powerpc/tm/tm-syscall.c
deleted file mode 100644
index 454b965a2db3..000000000000
--- a/tools/testing/selftests/powerpc/tm/tm-syscall.c
+++ /dev/null
@@ -1,106 +0,0 @@
-/*
- * Copyright 2015, Sam Bobroff, IBM Corp.
- * Licensed under GPLv2.
- *
- * Test the kernel's system call code to ensure that a system call
- * made from within an active HTM transaction is aborted with the
- * correct failure code.
- * Conversely, ensure that a system call made from within a
- * suspended transaction can succeed.
- */
-
-#include <stdio.h>
-#include <unistd.h>
-#include <sys/syscall.h>
-#include <asm/tm.h>
-#include <sys/time.h>
-#include <stdlib.h>
-
-#include "utils.h"
-#include "tm.h"
-
-extern int getppid_tm_active(void);
-extern int getppid_tm_suspended(void);
-
-unsigned retries = 0;
-
-#define TEST_DURATION 10 /* seconds */
-#define TM_RETRIES 100
-
-pid_t getppid_tm(bool suspend)
-{
- int i;
- pid_t pid;
-
- for (i = 0; i < TM_RETRIES; i++) {
- if (suspend)
- pid = getppid_tm_suspended();
- else
- pid = getppid_tm_active();
-
- if (pid >= 0)
- return pid;
-
- if (failure_is_persistent()) {
- if (failure_is_syscall())
- return -1;
-
- printf("Unexpected persistent transaction failure.\n");
- printf("TEXASR 0x%016lx, TFIAR 0x%016lx.\n",
- __builtin_get_texasr(), __builtin_get_tfiar());
- exit(-1);
- }
-
- retries++;
- }
-
- printf("Exceeded limit of %d temporary transaction failures.\n", TM_RETRIES);
- printf("TEXASR 0x%016lx, TFIAR 0x%016lx.\n",
- __builtin_get_texasr(), __builtin_get_tfiar());
-
- exit(-1);
-}
-
-int tm_syscall(void)
-{
- unsigned count = 0;
- struct timeval end, now;
-
- SKIP_IF(!have_htm_nosc());
-
- setbuf(stdout, NULL);
-
- printf("Testing transactional syscalls for %d seconds...\n", TEST_DURATION);
-
- gettimeofday(&end, NULL);
- now.tv_sec = TEST_DURATION;
- now.tv_usec = 0;
- timeradd(&end, &now, &end);
-
- for (count = 0; timercmp(&now, &end, <); count++) {
- /*
- * Test a syscall within a suspended transaction and verify
- * that it succeeds.
- */
- FAIL_IF(getppid_tm(true) == -1); /* Should succeed. */
-
- /*
- * Test a syscall within an active transaction and verify that
- * it fails with the correct failure code.
- */
- FAIL_IF(getppid_tm(false) != -1); /* Should fail... */
- FAIL_IF(!failure_is_persistent()); /* ...persistently... */
- FAIL_IF(!failure_is_syscall()); /* ...with code syscall. */
- gettimeofday(&now, 0);
- }
-
- printf("%d active and suspended transactions behaved correctly.\n", count);
- printf("(There were %d transaction retries.)\n", retries);
-
- return 0;
-}
-
-int main(void)
-{
- return test_harness(tm_syscall, "tm_syscall");
-}
--
2.16.2
^ permalink raw reply related
* Re: [PATCH] fix double ;;s in code
From: Rob Clark @ 2018-02-20 1:25 UTC (permalink / raw)
To: Pavel Machek
Cc: Christophe LEROY, SF Markus Elfring, kernel list, vgupta,
Russell King - ARM Linux, oleg, Catalin Marinas, Will Deacon,
paulus, Benjamin Herrenschmidt, Michael Ellerman, Ard Biesheuvel,
tglx, mingo, hpa, x86, scott.bauer, jonathan.derrick, axboe,
daniel.lezcano, Maxime Ripard, Chen-Yu Tsai, Alexander Deucher,
Christian König, Chunming Zhou, David Airlie,
list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>, ,
Shaohua Li, shawnguo, kernel, Fabio Estevam, Andrew Morton,
Alexey Brodkin, mhocko, vbabka, Vladislav.Zakharov, noamca,
Masahiro Yamada, Stephen Boyd, Viresh Kumar, Linus Walleij,
Heiko Stuebner, aik, ruscur, david, fbarrat, alistair,
Rob Herring, Joe Perches, Harry Wentland, Cheng, Tony,
Wenjing.Liu, Dave Airlie, Ding.Wang, sylvia.tsai, hersenxs.wu,
Rex.Zhu, JinHuiEric.Huang, Dan Carpenter, Archit Taneja,
narmstrong, Ville Syrjälä, Jordan Crouse,
Aishwarya Pant, Noralf Trønnes, andresx7, Monk.Liu,
Nicolai Hähnle, Grodzovsky, Andrey, linux-snps-arc,
linux-arm-kernel, kvm-ppc, linuxppc-dev, linux-efi, linux-block,
amd-gfx, dri-devel, linux-arm-msm, freedreno,
list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>, ,
linux-raid
In-Reply-To: <20180219193312.GA3709@amd>
On Mon, Feb 19, 2018 at 2:33 PM, Pavel Machek <pavel@ucw.cz> wrote:
> On Mon 2018-02-19 16:41:35, Daniel Vetter wrote:
>> On Sun, Feb 18, 2018 at 11:00:56AM +0100, Christophe LEROY wrote:
>> >
>> >
>> > Le 17/02/2018 =C3=A0 22:19, Pavel Machek a =C3=A9crit :
>> > >
>> > > Fix double ;;'s in code.
>> > >
>> > > Signed-off-by: Pavel Machek <pavel@ucw.cz>
>> >
>> > A summary of the files modified on top of the patch would help underst=
and
>> > the impact.
>> >
>> > A maybe there should be one patch by area, eg one for each arch specif=
ic
>> > modif and one for drivers/ and one for block/ ?
>>
>> Yeah, pls split this into one patch per area, with a suitable patch
>> subject prefix. Look at git log of each file to get a feeling for what's
>> the standard in each area.
>
> Yeah I can spend hour spliting it, and then people will ignore it
> anyway.
>
> If you care about one of the files being modified, please fix the
> bug, ";;" is a clear bug.
>
> If you don't care ... well I don't care either.
>
> drivers/gpu/ has four entries, i guess that's something for you.
fwiw, one of those four is dup of a patch that I've already pushed to
msm-next for drm/msm (which seems to be an argument for splitting up a
treewide patch.. which seems something quite scriptable, but up to you
whether you want to bother with that.. either way drm/msm is ;;-clean
now)
BR,
-R
> Pavel
>
>> > > diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c b/driv=
ers/gpu/drm/amd/display/dc/core/dc_link_dp.c
>> > > index 61e8c3e..33d91e4 100644
>> > > --- a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
>> > > +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
>> > > @@ -718,7 +718,7 @@ static enum link_training_result perform_channel=
_equalization_sequence(
>> > > uint32_t retries_ch_eq;
>> > > enum dc_lane_count lane_count =3D lt_settings->link_settin=
gs.lane_count;
>> > > union lane_align_status_updated dpcd_lane_status_updated =
=3D {{0}};
>> > > - union lane_status dpcd_lane_status[LANE_COUNT_DP_MAX] =3D {{{0}}};=
;
>> > > + union lane_status dpcd_lane_status[LANE_COUNT_DP_MAX] =3D {{{0}}};
>> > > hw_tr_pattern =3D get_supported_tp(link);
>> > > diff --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c b/drivers=
/gpu/drm/amd/powerplay/amd_powerplay.c
>> > > index 4c3223a..adb6e7b 100644
>> > > --- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
>> > > +++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
>> > > @@ -162,7 +162,7 @@ static int pp_hw_init(void *handle)
>> > > if(hwmgr->smumgr_funcs->start_smu(pp_handle->hwmgr=
)) {
>> > > pr_err("smc start failed\n");
>> > > hwmgr->smumgr_funcs->smu_fini(pp_handle->h=
wmgr);
>> > > - return -EINVAL;;
>> > > + return -EINVAL;
>> > > }
>> > > if (ret =3D=3D PP_DPM_DISABLED)
>> > > goto exit;
>> > > diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c b/drivers/gpu/d=
rm/msm/mdp/mdp5/mdp5_kms.c
>> > > index 3e9bba4..6d8e3a9 100644
>> > > --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
>> > > +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
>> > > @@ -680,7 +680,7 @@ struct msm_kms *mdp5_kms_init(struct drm_device =
*dev)
>> > > } else {
>> > > dev_info(&pdev->dev,
>> > > "no iommu, fallback to phys contig buffer=
s for scanout\n");
>> > > - aspace =3D NULL;;
>> > > + aspace =3D NULL;
>> > > }
>> > > pm_runtime_put_sync(&pdev->dev);
>> > > diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu=
/drm/scheduler/gpu_scheduler.c
>> > > index 2c18996..0d95888 100644
>> > > --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> > > +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> > > @@ -461,7 +461,7 @@ void drm_sched_hw_job_reset(struct drm_gpu_sched=
uler *sched, struct drm_sched_jo
>> > > {
>> > > struct drm_sched_job *s_job;
>> > > struct drm_sched_entity *entity, *tmp;
>> > > - int i;;
>> > > + int i;
>> > > spin_lock(&sched->job_list_lock);
>> > > list_for_each_entry_reverse(s_job, &sched->ring_mirror_lis=
t, node) {
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/b=
log.html
^ permalink raw reply
* Re: [RFC PATCH 05/12] [WIP] powerpc/tm: Reclaim/recheckpoint on entry/exit
From: Michael Neuling @ 2018-02-20 2:50 UTC (permalink / raw)
To: Cyril Bur, benh, linuxppc-dev
In-Reply-To: <20180220002241.29648-6-cyrilbur@gmail.com>
On Tue, 2018-02-20 at 11:22 +1100, Cyril Bur wrote:
The comment from the cover sheet should be here
> ---
> arch/powerpc/include/asm/exception-64s.h | 25 +++++++++++++++++++++
> arch/powerpc/kernel/entry_64.S | 5 +++++
> arch/powerpc/kernel/process.c | 37 ++++++++++++++++++++++++++=
++----
> 3 files changed, 63 insertions(+), 4 deletions(-)
>=20
> diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/incl=
ude/asm/exception-64s.h
> index 471b2274fbeb..f904f19a9ec2 100644
> --- a/arch/powerpc/include/asm/exception-64s.h
> +++ b/arch/powerpc/include/asm/exception-64s.h
> @@ -35,6 +35,7 @@
> * implementations as possible.
> */
> #include <asm/head-64.h>
> +#include <asm/tm.h>
> =20
> /* PACA save area offsets (exgen, exmc, etc) */
> #define EX_R9 0
> @@ -127,6 +128,26 @@
> hrfid; \
> b hrfi_flush_fallback
> =20
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> +#define TM_KERNEL_ENTRY \
> + ld r3,_MSR(r1); \
> + /* Probably don't need to check if coming from user/kernel */ \
> + /* If TM is suspended or active then we must have come from*/ \
> + /* userspace */ \
> + andi. r0,r3,MSR_PR; \
> + beq 1f; \
> + rldicl. r3,r3,(64-MSR_TS_LG),(64-2); /* SUSPENDED or ACTIVE*/ \
> + beql+ 1f; /* Not SUSPENDED or ACTIVE */ \
> + bl save_nvgprs; \
> + RECONCILE_IRQ_STATE(r10,r11); \
> + li r3,TM_CAUSE_MISC; \
> + bl tm_reclaim_current; /* uint8 cause */ \
> +1:
> +
> +#else /* CONFIG_PPC_TRANSACTIONAL_MEM */
> +#define TM_KERNEL_ENTRY
> +#endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
> +
> #ifdef CONFIG_RELOCATABLE
> #define __EXCEPTION_RELON_PROLOG_PSERIES_1(label, h) \
> mfspr r11,SPRN_##h##SRR0; /* save SRR0 */ \
> @@ -675,6 +696,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_CTRL)
> EXCEPTION_PROLOG_COMMON(trap, area); \
> /* Volatile regs are potentially clobbered here */ \
> additions; \
> + /* This is going to need to go somewhere else as well */\
> + /* See comment in tm_recheckpoint() */\
> + TM_KERNEL_ENTRY; \
> addi r3,r1,STACK_FRAME_OVERHEAD; \
> bl hdlr; \
> b ret
> @@ -689,6 +713,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_CTRL)
> EXCEPTION_PROLOG_COMMON_3(trap); \
> /* Volatile regs are potentially clobbered here */ \
> additions; \
> + TM_KERNEL_ENTRY; \
> addi r3,r1,STACK_FRAME_OVERHEAD; \
> bl hdlr
> =20
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_6=
4.S
> index 2cb5109a7ea3..107c15c6f48b 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -126,6 +126,11 @@ BEGIN_FW_FTR_SECTION
> 33:
> END_FW_FTR_SECTION_IFSET(FW_FEATURE_SPLPAR)
> #endif /* CONFIG_VIRT_CPU_ACCOUNTING_NATIVE && CONFIG_PPC_SPLPAR */
> + TM_KERNEL_ENTRY
> + REST_GPR(0,r1)
> + REST_4GPRS(3,r1)
> + REST_2GPRS(7,r1)
> + addi r9,r1,STACK_FRAME_OVERHEAD
Why are we doing these restores here now?
> =20
> /*
> * A syscall should always be called with interrupts enabled
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.=
c
> index 77dc6d8288eb..ea75da0fd506 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -951,6 +951,23 @@ void tm_recheckpoint(struct thread_struct *thread)
> if (!(thread->regs->msr & MSR_TM))
> return;
> =20
> + /*
> + * This is 'that' comment.
I think I'm in the loop here but I don't actually know what this means.=20
Senior Mikey moment or Crazy Cyril comments? I'll let the peanut gallery de=
cide.
> + *
> + * If we get where with tm suspended or active then something
s/where/here/
> + * has gone wrong. I've added this now as a proof of concept.
> + *
> + * The problem I'm seeing without it is an attempt to
> + * recheckpoint a CPU without a previous reclaim.
> + *
> + * I'm probably missed an exception entry with the
> + * TM_KERNEL_ENTRY macro. Should be easy enough to find.
> + */
> + if (MSR_TM_ACTIVE(mfmsr()))
> + return;
I don't really get this. Wouldn't this test apply now?
> +
> + tm_enable();
Why did we add this?
> +
> /* We really can't be interrupted here as the TEXASR registers can't
> * change and later in the trecheckpoint code, we have a userspace R1.
> * So let's hard disable over this region.
> @@ -1009,6 +1026,13 @@ static inline void tm_recheckpoint_new_task(struct=
task_struct *new)
> static inline void __switch_to_tm(struct task_struct *prev,
> struct task_struct *new)
> {
> + /*
> + * So, with the rework none of this code should not be needed.
> + * I've left in the reclaim for now. This *should* save us
> + * from any mistake in the new code. Also the
> + * enabling/disabling logic of MSR_TM really should be
> + * refactored into a common way with MSR_{FP,VEC,VSX}
> + */
> if (cpu_has_feature(CPU_FTR_TM)) {
> if (tm_enabled(prev) || tm_enabled(new))
> tm_enable();
> @@ -1016,11 +1040,14 @@ static inline void __switch_to_tm(struct task_str=
uct *prev,
> if (tm_enabled(prev)) {
> prev->thread.load_tm++;
> tm_reclaim_task(prev);
> - if (!MSR_TM_ACTIVE(prev->thread.regs->msr) && prev->thread.load_tm =
=3D=3D 0)
> - prev->thread.regs->msr &=3D ~MSR_TM;
> + /*
> + * The disabling logic may be confused don't
> + * disable for now
> + *
> + * if (!MSR_TM_ACTIVE(prev->thread.regs->msr) && prev->thread.load_tm=
=3D=3D 0)
> + * prev->thread.regs->msr &=3D ~MSR_TM;
> + */
Why are you doing this when you just remove all this code in the next patch=
?
> }
> -
> - tm_recheckpoint_new_task(new);
> }
> }
> =20
> @@ -1055,6 +1082,8 @@ void restore_tm_state(struct pt_regs *regs)
> msr_diff =3D current->thread.ckpt_regs.msr & ~regs->msr;
> msr_diff &=3D MSR_FP | MSR_VEC | MSR_VSX;
> =20
> + tm_recheckpoint(¤t->thread);
> +
So why do we do tm_recheckpoint at all? Shouldn't most of the tm_blah code =
go
away in process.c after all this?
> /* Ensure that restore_math() will restore */
> if (msr_diff & MSR_FP)
> current->thread.load_fp =3D 1;
^ permalink raw reply
* Re: [RFC PATCH 06/12] [WIP] powerpc/tm: Remove dead code from __switch_to_tm()
From: Michael Neuling @ 2018-02-20 2:52 UTC (permalink / raw)
To: Cyril Bur, benh, linuxppc-dev
In-Reply-To: <20180220002241.29648-7-cyrilbur@gmail.com>
Not sure I understand this.. should it be merged with the last patch?
Needs a comment here.
On Tue, 2018-02-20 at 11:22 +1100, Cyril Bur wrote:
> ---
> arch/powerpc/kernel/process.c | 24 +++++-------------------
> 1 file changed, 5 insertions(+), 19 deletions(-)
>=20
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.=
c
> index ea75da0fd506..574b05fe7d66 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1027,27 +1027,13 @@ static inline void __switch_to_tm(struct task_str=
uct *prev,
> struct task_struct *new)
> {
> /*
> - * So, with the rework none of this code should not be needed.
> - * I've left in the reclaim for now. This *should* save us
> - * from any mistake in the new code. Also the
> - * enabling/disabling logic of MSR_TM really should be
> + * The enabling/disabling logic of MSR_TM really should be
> * refactored into a common way with MSR_{FP,VEC,VSX}
> */
> - if (cpu_has_feature(CPU_FTR_TM)) {
> - if (tm_enabled(prev) || tm_enabled(new))
> - tm_enable();
> -
> - if (tm_enabled(prev)) {
> - prev->thread.load_tm++;
> - tm_reclaim_task(prev);
> - /*
> - * The disabling logic may be confused don't
> - * disable for now
> - *
> - * if (!MSR_TM_ACTIVE(prev->thread.regs->msr) && prev->thread.load_tm=
=3D=3D 0)
> - * prev->thread.regs->msr &=3D ~MSR_TM;
> - */
> - }
> + if (cpu_has_feature(CPU_FTR_TM) && tm_enabled(prev)) {
> + prev->thread.load_tm++;
> + if (!MSR_TM_ACTIVE(prev->thread.regs->msr) && prev->thread.load_tm =3D=
=3D 0)
> + prev->thread.regs->msr &=3D ~MSR_TM;
> }
> }
> =20
^ permalink raw reply
* Re: [RFC PATCH 10/12] [WIP] powerpc/tm: Correctly save/restore checkpointed sprs
From: Michael Neuling @ 2018-02-20 3:00 UTC (permalink / raw)
To: Cyril Bur, benh, linuxppc-dev
In-Reply-To: <20180220002241.29648-11-cyrilbur@gmail.com>
This needs a description of what you're trying to do. "Correctly" doesn't
really mean anything.
On Tue, 2018-02-20 at 11:22 +1100, Cyril Bur wrote:
> ---
> arch/powerpc/kernel/process.c | 57 +++++++++++++++++++++++++++++++++++++=
++++-
> -
> arch/powerpc/kernel/ptrace.c | 9 +++----
> 2 files changed, 58 insertions(+), 8 deletions(-)
>=20
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.=
c
> index cd3ae80a6878..674f75c56172 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -859,6 +859,8 @@ static inline bool tm_enabled(struct task_struct *tsk=
)
> return tsk && tsk->thread.regs && (tsk->thread.regs->msr & MSR_TM);
> }
> =20
> +static inline void save_sprs(struct thread_struct *t);
> +
> static void tm_reclaim_thread(struct thread_struct *thr, uint8_t cause)
> {
> /*
> @@ -879,6 +881,8 @@ static void tm_reclaim_thread(struct thread_struct *t=
hr,
> uint8_t cause)
> if (!MSR_TM_SUSPENDED(mfmsr()))
> return;
> =20
> + save_sprs(thr);
> +
> giveup_all(container_of(thr, struct task_struct, thread));
> =20
> tm_reclaim(thr, cause);
> @@ -991,6 +995,37 @@ void tm_recheckpoint(struct thread_struct *thread)
> =20
> __tm_recheckpoint(thread);
> =20
> + /*
> + * This is a stripped down restore_sprs(), we need to do this
> + * now as we might go straight out to userspace and currently
> + * the checkpointed values are on the CPU.
> + *
> + * TODO: Improve
> + */
> +#ifdef CONFIG_ALTIVEC
> + if (cpu_has_feature(CPU_FTR_ALTIVEC))
> + mtspr(SPRN_VRSAVE, thread->vrsave);
> +#endif
> +#ifdef CONFIG_PPC_BOOK3S_64
> + if (cpu_has_feature(CPU_FTR_DSCR)) {
> + u64 dscr =3D get_paca()->dscr_default;
> + if (thread->dscr_inherit)
> + dscr =3D thread->dscr;
> +
> + mtspr(SPRN_DSCR, dscr);
> + }
> +
> + if (cpu_has_feature(CPU_FTR_ARCH_207S)) {
> + /* The EBB regs aren't checkpointed */
> + mtspr(SPRN_FSCR, thread->fscr);
> +
> + mtspr(SPRN_TAR, thread->tar);
> + }
> +
> + /* I think we don't need to */
> + if (cpu_has_feature(CPU_FTR_ARCH_300))
> + mtspr(SPRN_TIDR, thread->tidr);
> +#endif
Why are you touching all the above hunk?
> local_irq_restore(flags);
> }
> =20
> @@ -1193,6 +1228,11 @@ struct task_struct *__switch_to(struct task_struct
> *prev,
> #endif
> =20
> new_thread =3D &new->thread;
> + /*
> + * Why not &prev->thread; ?
> + * What is the difference between &prev->thread and
> + * ¤t->thread ?
> + */
Why not just work it out and FIX THE CODE, rather than just rabbiting on ab=
out
it! :-P
> old_thread =3D ¤t->thread;
> =20
> WARN_ON(!irqs_disabled());
> @@ -1237,8 +1277,16 @@ struct task_struct *__switch_to(struct task_struct
> *prev,
> /*
> * We need to save SPRs before treclaim/trecheckpoint as these will
> * change a number of them.
> + *
> + * Because we're now reclaiming on kernel entry, we've had to
> + * already save them. Don't do it again.
> + * Note: To deliver a signal in the signal context, we'll have
> + * turned off TM because we don't want the signal context to
> + * have the transactional state of the main thread - what if
> + * we go through switch to at that point? Can we?
> */
> - save_sprs(&prev->thread);
> + if (!prev->thread.regs || !MSR_TM_ACTIVE(prev->thread.regs->msr))
> + save_sprs(&prev->thread);
> =20
> /* Save FPU, Altivec, VSX and SPE state */
> giveup_all(prev);
> @@ -1260,8 +1308,13 @@ struct task_struct *__switch_to(struct task_struct
> *prev,
> * for this is we manually create a stack frame for new tasks that
> * directly returns through ret_from_fork() or
> * ret_from_kernel_thread(). See copy_thread() for details.
> + *
> + * It isn't stricly nessesary that we avoid the restore here
> + * because we'll simply restore again after the recheckpoint,
> + * but we can avoid it for performance reasons.
> */
> - restore_sprs(old_thread, new_thread);
> + if (!new_thread->regs || !MSR_TM_ACTIVE(new_thread->regs->msr))
> + restore_sprs(old_thread, new_thread);
> =20
> last =3D _switch(old_thread, new_thread);
> =20
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index ca72d7391d40..16001987ba71 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -135,12 +135,9 @@ static void flush_tmregs_to_thread(struct task_struc=
t
> *tsk)
> if ((!cpu_has_feature(CPU_FTR_TM)) || (tsk !=3D current))
> return;
> =20
> - if (MSR_TM_SUSPENDED(mfmsr())) {
> - tm_reclaim_current(TM_CAUSE_SIGNAL);
> - } else {
> - tm_enable();
> - tm_save_sprs(&(tsk->thread));
> - }
> + BUG_ON(MSR_TM_SUSPENDED(mfmsr()));
> + tm_enable();
> + tm_save_sprs(&(tsk->thread));
> }
> #else
> static inline void flush_tmregs_to_thread(struct task_struct *tsk) { }
^ permalink raw reply
* Re: [RFC PATCH 12/12] [WIP] selftests/powerpc: Remove incorrect tm-syscall selftest
From: Michael Neuling @ 2018-02-20 3:04 UTC (permalink / raw)
To: Cyril Bur, benh, linuxppc-dev
In-Reply-To: <20180220002241.29648-13-cyrilbur@gmail.com>
> --- a/tools/testing/selftests/powerpc/tm/tm-syscall.c
> +++ /dev/null
> @@ -1,106 +0,0 @@
> -/*
> - * Copyright 2015, Sam Bobroff, IBM Corp.
> - * Licensed under GPLv2.
> - *
> - * Test the kernel's system call code to ensure that a system call
> - * made from within an active HTM transaction is aborted with the
> - * correct failure code.
The above is still true
> - * Conversely, ensure that a system call made from within a
> - * suspended transaction can succeed.
This is true anymore....
So can we just modify the test to remove the second part?
Mikey
^ permalink raw reply
* Re: [RFC PATCH 12/12] [WIP] selftests/powerpc: Remove incorrect tm-syscall selftest
From: Cyril Bur @ 2018-02-20 3:42 UTC (permalink / raw)
To: Michael Neuling, benh, linuxppc-dev
In-Reply-To: <1519095881.7360.59.camel@neuling.org>
On Tue, 2018-02-20 at 14:04 +1100, Michael Neuling wrote:
> > --- a/tools/testing/selftests/powerpc/tm/tm-syscall.c
> > +++ /dev/null
> > @@ -1,106 +0,0 @@
> > -/*
> > - * Copyright 2015, Sam Bobroff, IBM Corp.
> > - * Licensed under GPLv2.
> > - *
> > - * Test the kernel's system call code to ensure that a system call
> > - * made from within an active HTM transaction is aborted with the
> > - * correct failure code.
>
> The above is still true
>
> > - * Conversely, ensure that a system call made from within a
> > - * suspended transaction can succeed.
>
> This is true anymore....
>
> So can we just modify the test to remove the second part?
>
Oh true I overlooked that
Thanks
> Mikey
^ permalink raw reply
* Re: [RFC PATCH 06/12] [WIP] powerpc/tm: Remove dead code from __switch_to_tm()
From: Cyril Bur @ 2018-02-20 3:43 UTC (permalink / raw)
To: Michael Neuling, benh, linuxppc-dev
In-Reply-To: <1519095125.7360.54.camel@neuling.org>
On Tue, 2018-02-20 at 13:52 +1100, Michael Neuling wrote:
> Not sure I understand this.. should it be merged with the last patch?
>
Its all going to have to be one patch - I've left it split out to make
it more obvious which bits have had to mess with, this series
absolutely doesn't bisect.
> Needs a comment here.
>
>
> On Tue, 2018-02-20 at 11:22 +1100, Cyril Bur wrote:
> > ---
> > arch/powerpc/kernel/process.c | 24 +++++-------------------
> > 1 file changed, 5 insertions(+), 19 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> > index ea75da0fd506..574b05fe7d66 100644
> > --- a/arch/powerpc/kernel/process.c
> > +++ b/arch/powerpc/kernel/process.c
> > @@ -1027,27 +1027,13 @@ static inline void __switch_to_tm(struct task_struct *prev,
> > struct task_struct *new)
> > {
> > /*
> > - * So, with the rework none of this code should not be needed.
> > - * I've left in the reclaim for now. This *should* save us
> > - * from any mistake in the new code. Also the
> > - * enabling/disabling logic of MSR_TM really should be
> > + * The enabling/disabling logic of MSR_TM really should be
> > * refactored into a common way with MSR_{FP,VEC,VSX}
> > */
> > - if (cpu_has_feature(CPU_FTR_TM)) {
> > - if (tm_enabled(prev) || tm_enabled(new))
> > - tm_enable();
> > -
> > - if (tm_enabled(prev)) {
> > - prev->thread.load_tm++;
> > - tm_reclaim_task(prev);
> > - /*
> > - * The disabling logic may be confused don't
> > - * disable for now
> > - *
> > - * if (!MSR_TM_ACTIVE(prev->thread.regs->msr) && prev->thread.load_tm == 0)
> > - * prev->thread.regs->msr &= ~MSR_TM;
> > - */
> > - }
> > + if (cpu_has_feature(CPU_FTR_TM) && tm_enabled(prev)) {
> > + prev->thread.load_tm++;
> > + if (!MSR_TM_ACTIVE(prev->thread.regs->msr) && prev->thread.load_tm == 0)
> > + prev->thread.regs->msr &= ~MSR_TM;
> > }
> > }
> >
^ permalink raw reply
* Re: [RFC PATCH 05/12] [WIP] powerpc/tm: Reclaim/recheckpoint on entry/exit
From: Cyril Bur @ 2018-02-20 3:54 UTC (permalink / raw)
To: Michael Neuling, benh, linuxppc-dev
In-Reply-To: <1519095033.7360.52.camel@neuling.org>
On Tue, 2018-02-20 at 13:50 +1100, Michael Neuling wrote:
> On Tue, 2018-02-20 at 11:22 +1100, Cyril Bur wrote:
>
>
> The comment from the cover sheet should be here
>
> > ---
> > arch/powerpc/include/asm/exception-64s.h | 25 +++++++++++++++++++++
> > arch/powerpc/kernel/entry_64.S | 5 +++++
> > arch/powerpc/kernel/process.c | 37 ++++++++++++++++++++++++++++----
> > 3 files changed, 63 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
> > index 471b2274fbeb..f904f19a9ec2 100644
> > --- a/arch/powerpc/include/asm/exception-64s.h
> > +++ b/arch/powerpc/include/asm/exception-64s.h
> > @@ -35,6 +35,7 @@
> > * implementations as possible.
> > */
> > #include <asm/head-64.h>
> > +#include <asm/tm.h>
> >
> > /* PACA save area offsets (exgen, exmc, etc) */
> > #define EX_R9 0
> > @@ -127,6 +128,26 @@
> > hrfid; \
> > b hrfi_flush_fallback
> >
> > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > +#define TM_KERNEL_ENTRY \
> > + ld r3,_MSR(r1); \
> > + /* Probably don't need to check if coming from user/kernel */ \
> > + /* If TM is suspended or active then we must have come from*/ \
> > + /* userspace */ \
> > + andi. r0,r3,MSR_PR; \
> > + beq 1f; \
> > + rldicl. r3,r3,(64-MSR_TS_LG),(64-2); /* SUSPENDED or ACTIVE*/ \
> > + beql+ 1f; /* Not SUSPENDED or ACTIVE */ \
> > + bl save_nvgprs; \
> > + RECONCILE_IRQ_STATE(r10,r11); \
> > + li r3,TM_CAUSE_MISC; \
> > + bl tm_reclaim_current; /* uint8 cause */ \
> > +1:
> > +
> > +#else /* CONFIG_PPC_TRANSACTIONAL_MEM */
> > +#define TM_KERNEL_ENTRY
> > +#endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
> > +
> > #ifdef CONFIG_RELOCATABLE
> > #define __EXCEPTION_RELON_PROLOG_PSERIES_1(label, h) \
> > mfspr r11,SPRN_##h##SRR0; /* save SRR0 */ \
> > @@ -675,6 +696,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_CTRL)
> > EXCEPTION_PROLOG_COMMON(trap, area); \
> > /* Volatile regs are potentially clobbered here */ \
> > additions; \
> > + /* This is going to need to go somewhere else as well */\
> > + /* See comment in tm_recheckpoint() */\
> > + TM_KERNEL_ENTRY; \
> > addi r3,r1,STACK_FRAME_OVERHEAD; \
> > bl hdlr; \
> > b ret
> > @@ -689,6 +713,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_CTRL)
> > EXCEPTION_PROLOG_COMMON_3(trap); \
> > /* Volatile regs are potentially clobbered here */ \
> > additions; \
> > + TM_KERNEL_ENTRY; \
> > addi r3,r1,STACK_FRAME_OVERHEAD; \
> > bl hdlr
> >
> > diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> > index 2cb5109a7ea3..107c15c6f48b 100644
> > --- a/arch/powerpc/kernel/entry_64.S
> > +++ b/arch/powerpc/kernel/entry_64.S
> > @@ -126,6 +126,11 @@ BEGIN_FW_FTR_SECTION
> > 33:
> > END_FW_FTR_SECTION_IFSET(FW_FEATURE_SPLPAR)
> > #endif /* CONFIG_VIRT_CPU_ACCOUNTING_NATIVE && CONFIG_PPC_SPLPAR */
> > + TM_KERNEL_ENTRY
> > + REST_GPR(0,r1)
> > + REST_4GPRS(3,r1)
> > + REST_2GPRS(7,r1)
> > + addi r9,r1,STACK_FRAME_OVERHEAD
>
> Why are we doing these restores here now?
The syscall handler expects the syscall params to still be in their
respective regs.
>
> >
> > /*
> > * A syscall should always be called with interrupts enabled
> > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> > index 77dc6d8288eb..ea75da0fd506 100644
> > --- a/arch/powerpc/kernel/process.c
> > +++ b/arch/powerpc/kernel/process.c
> > @@ -951,6 +951,23 @@ void tm_recheckpoint(struct thread_struct *thread)
> > if (!(thread->regs->msr & MSR_TM))
> > return;
> >
> > + /*
> > + * This is 'that' comment.
>
> I think I'm in the loop here but I don't actually know what this means.
>
> Senior Mikey moment or Crazy Cyril comments? I'll let the peanut gallery decide.
>
Oh quite possibly crazy Cyril comment that will have to be...
normalised. I should actually delete this and see if that's still the
case.
> > + *
> > + * If we get where with tm suspended or active then something
>
> s/where/here/
>
> > + * has gone wrong. I've added this now as a proof of concept.
> > + *
> > + * The problem I'm seeing without it is an attempt to
> > + * recheckpoint a CPU without a previous reclaim.
> > + *
> > + * I'm probably missed an exception entry with the
> > + * TM_KERNEL_ENTRY macro. Should be easy enough to find.
> > + */
> > + if (MSR_TM_ACTIVE(mfmsr()))
> > + return;
>
> I don't really get this. Wouldn't this test apply now?
>
> > +
> > + tm_enable();
>
> Why did we add this?
>
Ah yes that was a cleanup I noticed along the way and clearly forgot to
finish.
At the moment there's a bunch of tm_enable()s either before calling
functions like tm_recheckpoint() or tm_reclaim_current() or inside
helpers (tm_reclaim_current() for example again). I feel like callers
shouldn't have to worry, it should be up to the function actually doing
the TM work to enable it.
> > +
> > /* We really can't be interrupted here as the TEXASR registers can't
> > * change and later in the trecheckpoint code, we have a userspace R1.
> > * So let's hard disable over this region.
> > @@ -1009,6 +1026,13 @@ static inline void tm_recheckpoint_new_task(struct task_struct *new)
> > static inline void __switch_to_tm(struct task_struct *prev,
> > struct task_struct *new)
> > {
> > + /*
> > + * So, with the rework none of this code should not be needed.
> > + * I've left in the reclaim for now. This *should* save us
> > + * from any mistake in the new code. Also the
> > + * enabling/disabling logic of MSR_TM really should be
> > + * refactored into a common way with MSR_{FP,VEC,VSX}
> > + */
> > if (cpu_has_feature(CPU_FTR_TM)) {
> > if (tm_enabled(prev) || tm_enabled(new))
> > tm_enable();
> > @@ -1016,11 +1040,14 @@ static inline void __switch_to_tm(struct task_struct *prev,
> > if (tm_enabled(prev)) {
> > prev->thread.load_tm++;
> > tm_reclaim_task(prev);
> > - if (!MSR_TM_ACTIVE(prev->thread.regs->msr) && prev->thread.load_tm == 0)
> > - prev->thread.regs->msr &= ~MSR_TM;
> > + /*
> > + * The disabling logic may be confused don't
> > + * disable for now
> > + *
> > + * if (!MSR_TM_ACTIVE(prev->thread.regs->msr) && prev->thread.load_tm == 0)
> > + * prev->thread.regs->msr &= ~MSR_TM;
> > + */
>
> Why are you doing this when you just remove all this code in the next patch?
The next 3 or so patches will need squashing into this one before
merging.
> > }
> > -
> > - tm_recheckpoint_new_task(new);
> > }
> > }
> >
> > @@ -1055,6 +1082,8 @@ void restore_tm_state(struct pt_regs *regs)
> > msr_diff = current->thread.ckpt_regs.msr & ~regs->msr;
> > msr_diff &= MSR_FP | MSR_VEC | MSR_VSX;
> >
> > + tm_recheckpoint(¤t->thread);
> > +
>
> So why do we do tm_recheckpoint at all? Shouldn't most of the tm_blah code go
> away in process.c after all this?
>
I'm not sure I follow, we need to recheckpoint because we're going back
to userspace? Or would you rather calling the tm.S code directly from
the exception return path?
Yes, I hope we'll be able to have a fairly big cleanup commit of tm_
code in process.c at the end of this series.
> > /* Ensure that restore_math() will restore */
> > if (msr_diff & MSR_FP)
> > current->thread.load_fp = 1;
^ permalink raw reply
* Re: [RFC PATCH 10/12] [WIP] powerpc/tm: Correctly save/restore checkpointed sprs
From: Cyril Bur @ 2018-02-20 3:59 UTC (permalink / raw)
To: Michael Neuling, benh, linuxppc-dev
In-Reply-To: <1519095630.7360.57.camel@neuling.org>
On Tue, 2018-02-20 at 14:00 +1100, Michael Neuling wrote:
> This needs a description of what you're trying to do. "Correctly" doesn't
> really mean anything.
>
>
> On Tue, 2018-02-20 at 11:22 +1100, Cyril Bur wrote:
> > ---
> > arch/powerpc/kernel/process.c | 57 +++++++++++++++++++++++++++++++++++++++++-
> > -
> > arch/powerpc/kernel/ptrace.c | 9 +++----
> > 2 files changed, 58 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> > index cd3ae80a6878..674f75c56172 100644
> > --- a/arch/powerpc/kernel/process.c
> > +++ b/arch/powerpc/kernel/process.c
> > @@ -859,6 +859,8 @@ static inline bool tm_enabled(struct task_struct *tsk)
> > return tsk && tsk->thread.regs && (tsk->thread.regs->msr & MSR_TM);
> > }
> >
> > +static inline void save_sprs(struct thread_struct *t);
> > +
> > static void tm_reclaim_thread(struct thread_struct *thr, uint8_t cause)
> > {
> > /*
> > @@ -879,6 +881,8 @@ static void tm_reclaim_thread(struct thread_struct *thr,
> > uint8_t cause)
> > if (!MSR_TM_SUSPENDED(mfmsr()))
> > return;
> >
> > + save_sprs(thr);
> > +
> > giveup_all(container_of(thr, struct task_struct, thread));
> >
> > tm_reclaim(thr, cause);
> > @@ -991,6 +995,37 @@ void tm_recheckpoint(struct thread_struct *thread)
> >
> > __tm_recheckpoint(thread);
> >
> > + /*
> > + * This is a stripped down restore_sprs(), we need to do this
> > + * now as we might go straight out to userspace and currently
> > + * the checkpointed values are on the CPU.
> > + *
> > + * TODO: Improve
> > + */
> > +#ifdef CONFIG_ALTIVEC
> > + if (cpu_has_feature(CPU_FTR_ALTIVEC))
> > + mtspr(SPRN_VRSAVE, thread->vrsave);
> > +#endif
> > +#ifdef CONFIG_PPC_BOOK3S_64
> > + if (cpu_has_feature(CPU_FTR_DSCR)) {
> > + u64 dscr = get_paca()->dscr_default;
> > + if (thread->dscr_inherit)
> > + dscr = thread->dscr;
> > +
> > + mtspr(SPRN_DSCR, dscr);
> > + }
> > +
> > + if (cpu_has_feature(CPU_FTR_ARCH_207S)) {
> > + /* The EBB regs aren't checkpointed */
> > + mtspr(SPRN_FSCR, thread->fscr);
> > +
> > + mtspr(SPRN_TAR, thread->tar);
> > + }
> > +
> > + /* I think we don't need to */
> > + if (cpu_has_feature(CPU_FTR_ARCH_300))
> > + mtspr(SPRN_TIDR, thread->tidr);
> > +#endif
>
> Why are you touching all the above hunk?
I copied restore_sprs. I'm tidying that up now - we can't call
restore_sprs because we don't have a prev and next thread.
>
> > local_irq_restore(flags);
> > }
> >
> > @@ -1193,6 +1228,11 @@ struct task_struct *__switch_to(struct task_struct
> > *prev,
> > #endif
> >
> > new_thread = &new->thread;
> > + /*
> > + * Why not &prev->thread; ?
> > + * What is the difference between &prev->thread and
> > + * ¤t->thread ?
> > + */
>
> Why not just work it out and FIX THE CODE, rather than just rabbiting on about
> it! :-P
Agreed - I started to and then had a mini freakout that things would
end really badly if they're not the same. So I left that comment as a
reminder to investigate.
They should be the same though right?
>
> > old_thread = ¤t->thread;
> >
> > WARN_ON(!irqs_disabled());
> > @@ -1237,8 +1277,16 @@ struct task_struct *__switch_to(struct task_struct
> > *prev,
> > /*
> > * We need to save SPRs before treclaim/trecheckpoint as these will
> > * change a number of them.
> > + *
> > + * Because we're now reclaiming on kernel entry, we've had to
> > + * already save them. Don't do it again.
> > + * Note: To deliver a signal in the signal context, we'll have
> > + * turned off TM because we don't want the signal context to
> > + * have the transactional state of the main thread - what if
> > + * we go through switch to at that point? Can we?
> > */
> > - save_sprs(&prev->thread);
> > + if (!prev->thread.regs || !MSR_TM_ACTIVE(prev->thread.regs->msr))
> > + save_sprs(&prev->thread);
> >
> > /* Save FPU, Altivec, VSX and SPE state */
> > giveup_all(prev);
> > @@ -1260,8 +1308,13 @@ struct task_struct *__switch_to(struct task_struct
> > *prev,
> > * for this is we manually create a stack frame for new tasks that
> > * directly returns through ret_from_fork() or
> > * ret_from_kernel_thread(). See copy_thread() for details.
> > + *
> > + * It isn't stricly nessesary that we avoid the restore here
> > + * because we'll simply restore again after the recheckpoint,
> > + * but we can avoid it for performance reasons.
> > */
> > - restore_sprs(old_thread, new_thread);
> > + if (!new_thread->regs || !MSR_TM_ACTIVE(new_thread->regs->msr))
> > + restore_sprs(old_thread, new_thread);
> >
> > last = _switch(old_thread, new_thread);
> >
> > diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> > index ca72d7391d40..16001987ba71 100644
> > --- a/arch/powerpc/kernel/ptrace.c
> > +++ b/arch/powerpc/kernel/ptrace.c
> > @@ -135,12 +135,9 @@ static void flush_tmregs_to_thread(struct task_struct
> > *tsk)
> > if ((!cpu_has_feature(CPU_FTR_TM)) || (tsk != current))
> > return;
> >
> > - if (MSR_TM_SUSPENDED(mfmsr())) {
> > - tm_reclaim_current(TM_CAUSE_SIGNAL);
> > - } else {
> > - tm_enable();
> > - tm_save_sprs(&(tsk->thread));
> > - }
> > + BUG_ON(MSR_TM_SUSPENDED(mfmsr()));
> > + tm_enable();
> > + tm_save_sprs(&(tsk->thread));
> > }
> > #else
> > static inline void flush_tmregs_to_thread(struct task_struct *tsk) { }
^ permalink raw reply
* Re: [PATCH V5] cxl: Fix timebase synchronization status on P9
From: Andrew Donnellan @ 2018-02-20 4:07 UTC (permalink / raw)
To: Christophe Lombard, linuxppc-dev, fbarrat, vaibhav
In-Reply-To: <1519050491-15326-1-git-send-email-clombard@linux.vnet.ibm.com>
On 20/02/18 01:28, Christophe Lombard wrote:
> The PSL Timebase register is updated by the PSL to maintain the
> timebase.
> On P9, the Timebase value is only provided by the CAPP as received
> the last time a timebase request was performed.
> The timebase requests are initiated through the adapter configuration or
> application registers.
> The specific sysfs entry "/sys/class/cxl/cardxx/psl_timebase_synced" is
> now dynamically updated according the content of the PSL Timebase
> register.
>
> Signed-off-by: Christophe Lombard <clombard@linux.vnet.ibm.com>
Acked-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
--
Andrew Donnellan OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com IBM Australia Limited
^ permalink raw reply
* Re: [RFC PATCH 05/12] [WIP] powerpc/tm: Reclaim/recheckpoint on entry/exit
From: Michael Neuling @ 2018-02-20 5:25 UTC (permalink / raw)
To: Cyril Bur, benh, linuxppc-dev
In-Reply-To: <1519098854.6563.15.camel@gmail.com>
> > > @@ -1055,6 +1082,8 @@ void restore_tm_state(struct pt_regs *regs)
> > > msr_diff =3D current->thread.ckpt_regs.msr & ~regs->msr;
> > > msr_diff &=3D MSR_FP | MSR_VEC | MSR_VSX;
> > > =20
> > > + tm_recheckpoint(¤t->thread);
> > > +
> >=20
> > So why do we do tm_recheckpoint at all? Shouldn't most of the tm_blah c=
ode go
> > away in process.c after all this?
> >=20
>=20
> I'm not sure I follow, we need to recheckpoint because we're going back
> to userspace? Or would you rather calling the tm.S code directly from
> the exception return path?
Yeah, I was thinking the point of this series was. We do tm_reclaim right =
on
entry and tm_recheckpoint right on exit. =20
The bits in between (ie. the tm_blah() calls process.c) would mostly go awa=
y.
> Yes, I hope we'll be able to have a fairly big cleanup commit of tm_
> code in process.c at the end of this series.
Yep, agreed.
Mikey
^ permalink raw reply
* Re: [RFC PATCH 10/12] [WIP] powerpc/tm: Correctly save/restore checkpointed sprs
From: Michael Neuling @ 2018-02-20 5:27 UTC (permalink / raw)
To: Cyril Bur, benh, linuxppc-dev
In-Reply-To: <1519099169.6563.19.camel@gmail.com>
On Tue, 2018-02-20 at 14:59 +1100, Cyril Bur wrote:
> On Tue, 2018-02-20 at 14:00 +1100, Michael Neuling wrote:
> > This needs a description of what you're trying to do. "Correctly" does=
n't
> > really mean anything.
> >=20
> >=20
> > On Tue, 2018-02-20 at 11:22 +1100, Cyril Bur wrote:
> > > ---
> > > arch/powerpc/kernel/process.c | 57 +++++++++++++++++++++++++++++++++=
++++++++-
> > > -
> > > arch/powerpc/kernel/ptrace.c | 9 +++----
> > > 2 files changed, 58 insertions(+), 8 deletions(-)
> > >=20
> > > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/proc=
ess.c
> > > index cd3ae80a6878..674f75c56172 100644
> > > --- a/arch/powerpc/kernel/process.c
> > > +++ b/arch/powerpc/kernel/process.c
> > > @@ -859,6 +859,8 @@ static inline bool tm_enabled(struct task_struct =
*tsk)
> > > return tsk && tsk->thread.regs && (tsk->thread.regs->msr & MSR_TM);
> > > }
> > > =20
> > > +static inline void save_sprs(struct thread_struct *t);
> > > +
> > > static void tm_reclaim_thread(struct thread_struct *thr, uint8_t cau=
se)
> > > {
> > > /*
> > > @@ -879,6 +881,8 @@ static void tm_reclaim_thread(struct thread_struc=
t *thr,
> > > uint8_t cause)
> > > if (!MSR_TM_SUSPENDED(mfmsr()))
> > > return;
> > > =20
> > > + save_sprs(thr);
> > > +
> > > giveup_all(container_of(thr, struct task_struct, thread));
> > > =20
> > > tm_reclaim(thr, cause);
> > > @@ -991,6 +995,37 @@ void tm_recheckpoint(struct thread_struct *threa=
d)
> > > =20
> > > __tm_recheckpoint(thread);
> > > =20
> > > + /*
> > > + * This is a stripped down restore_sprs(), we need to do this
> > > + * now as we might go straight out to userspace and currently
> > > + * the checkpointed values are on the CPU.
> > > + *
> > > + * TODO: Improve
> > > + */
> > > +#ifdef CONFIG_ALTIVEC
> > > + if (cpu_has_feature(CPU_FTR_ALTIVEC))
> > > + mtspr(SPRN_VRSAVE, thread->vrsave);
> > > +#endif
> > > +#ifdef CONFIG_PPC_BOOK3S_64
> > > + if (cpu_has_feature(CPU_FTR_DSCR)) {
> > > + u64 dscr =3D get_paca()->dscr_default;
> > > + if (thread->dscr_inherit)
> > > + dscr =3D thread->dscr;
> > > +
> > > + mtspr(SPRN_DSCR, dscr);
> > > + }
> > > +
> > > + if (cpu_has_feature(CPU_FTR_ARCH_207S)) {
> > > + /* The EBB regs aren't checkpointed */
> > > + mtspr(SPRN_FSCR, thread->fscr);
> > > +
> > > + mtspr(SPRN_TAR, thread->tar);
> > > + }
> > > +
> > > + /* I think we don't need to */
> > > + if (cpu_has_feature(CPU_FTR_ARCH_300))
> > > + mtspr(SPRN_TIDR, thread->tidr);
> > > +#endif
> >=20
> > Why are you touching all the above hunk?
>=20
> I copied restore_sprs. I'm tidying that up now - we can't call
> restore_sprs because we don't have a prev and next thread.
Yeah needs to be tided up... we can't have another copy of the code.. obvio=
usly.
>=20
> >=20
> > > local_irq_restore(flags);
> > > }
> > > =20
> > > @@ -1193,6 +1228,11 @@ struct task_struct *__switch_to(struct task_st=
ruct
> > > *prev,
> > > #endif
> > > =20
> > > new_thread =3D &new->thread;
> > > + /*
> > > + * Why not &prev->thread; ?
> > > + * What is the difference between &prev->thread and
> > > + * ¤t->thread ?
> > > + */
> >=20
> > Why not just work it out and FIX THE CODE, rather than just rabbiting o=
n about
> > it! :-P
>=20
> Agreed - I started to and then had a mini freakout that things would
> end really badly if they're not the same. So I left that comment as a
> reminder to investigate.
>=20
> They should be the same though right?
Should be if prev =3D=3D current.
Mikey
^ permalink raw reply
* Re: [PATCH] fix double ;;s in code
From: Vineet Gupta @ 2018-02-20 5:33 UTC (permalink / raw)
To: Pavel Machek, elfring, kernel list, linux, oleg, catalin.marinas,
will.deacon, paulus, benh, mpe, ard.biesheuvel, tglx, mingo, hpa,
x86, scott.bauer, jonathan.derrick, axboe, daniel.lezcano,
maxime.ripard, wens, alexander.deucher, christian.koenig,
David1.Zhou, airlied, robdclark, joro, shli, shawnguo, kernel,
fabio.estevam, akpm, Alexey.Brodkin, mhocko, vbabka,
Vladislav.Zakharov, noamca, yamada.masahiro, sboyd, viresh.kumar,
linus.walleij, heiko, aik, ruscur, david, fbarrat, alistair, robh,
joe, harry.wentland, tony.cheng, Wenjing.Liu, airlied, Ding.Wang,
sylvia.tsai, hersenxs.wu, Rex.Zhu, JinHuiEric.Huang,
dan.carpenter, architt, daniel.vetter, narmstrong, ville.syrjala,
jcrouse, aishpant, noralf, andresx7, Monk.Liu, nicolai.haehnle,
Andrey.Grodzovsky, linux-snps-arc, linux-arm-kernel, kvm-ppc,
linuxppc-dev, linux-efi, linux-block, amd-gfx, dri-devel,
linux-arm-msm, freedreno, iommu, linux-raid
Cc: Luis de Bethencourt
In-Reply-To: <20180217211955.GE9640@amd>
[-- Attachment #1: Type: text/plain, Size: 1069 bytes --]
On 02/17/2018 01:19 PM, Pavel Machek wrote:
> Fix double ;;'s in code.
>
> Signed-off-by: Pavel Machek<pavel@ucw.cz>
>
> diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
> index 9d27331..ec12fe1 100644
> --- a/arch/arc/kernel/setup.c
> +++ b/arch/arc/kernel/setup.c
> @@ -373,7 +373,7 @@ static void arc_chk_core_config(void)
> {
> struct cpuinfo_arc *cpu = &cpuinfo_arc700[smp_processor_id()];
> int saved = 0, present = 0;
> - char *opt_nm = NULL;;
> + char *opt_nm = NULL;
>
> if (!cpu->extn.timer0)
> panic("Timer0 is not present!\n");
> diff --git a/arch/arc/kernel/unwind.c b/arch/arc/kernel/unwind.c
> index 333daab..183391d 100644
> --- a/arch/arc/kernel/unwind.c
> +++ b/arch/arc/kernel/unwind.c
> @@ -366,7 +366,7 @@ static void init_unwind_hdr(struct unwind_table *table,
> return;
>
> ret_err:
> - panic("Attention !!! Dwarf FDE parsing errors\n");;
> + panic("Attention !!! Dwarf FDE parsing errors\n");
> }
>
I have a fix from Luis to same effect from Jan sitting in my for-curr
Thx for it anyways.
-Vineet
[-- Attachment #2: Type: text/html, Size: 1523 bytes --]
^ permalink raw reply
* Re: [PATCH] fix double ;;s in code
From: Michael Ellerman @ 2018-02-20 6:19 UTC (permalink / raw)
To: Daniel Vetter, Christophe LEROY
Cc: Pavel Machek, elfring, kernel list, vgupta, linux, oleg,
catalin.marinas, will.deacon, paulus, benh, ard.biesheuvel, tglx,
mingo, hpa, x86, scott.bauer, jonathan.derrick, axboe,
daniel.lezcano, maxime.ripard, wens, alexander.deucher,
christian.koenig, David1.Zhou, airlied, robdclark, joro, shli,
shawnguo, kernel, fabio.estevam, akpm, Alexey.Brodkin, mhocko,
vbabka, Vladislav.Zakharov, noamca, yamada.masahiro, sboyd,
viresh.kumar, linus.walleij, heiko, aik, ruscur, david, fbarrat,
alistair, robh, joe, harry.wentland, tony.cheng, Wenjing.Liu,
airlied, Ding.Wang, sylvia.tsai, hersenxs.wu, Rex.Zhu,
JinHuiEric.Huang, dan.carpenter, architt, daniel.vetter,
narmstrong, ville.syrjala, jcrouse, aishpant, noralf, andresx7,
Monk.Liu, nicolai.haehnle, Andrey.Grodzovsky, linux-snps-arc,
linux-arm-kernel, kvm-ppc, linuxppc-dev, linux-efi, linux-block,
amd-gfx, dri-devel, linux-arm-msm, freedreno, iommu, linux-raid
In-Reply-To: <20180219154135.GV22199@phenom.ffwll.local>
Daniel Vetter <daniel@ffwll.ch> writes:
> On Sun, Feb 18, 2018 at 11:00:56AM +0100, Christophe LEROY wrote:
>> Le 17/02/2018 =C3=A0 22:19, Pavel Machek a =C3=A9crit=C2=A0:
>> >=20
>> > Fix double ;;'s in code.
>> >=20
>> > Signed-off-by: Pavel Machek <pavel@ucw.cz>
>>=20
>> A summary of the files modified on top of the patch would help understand
>> the impact.
>>=20
>> A maybe there should be one patch by area, eg one for each arch specific
>> modif and one for drivers/ and one for block/ ?
>
> Yeah, pls split this into one patch per area, with a suitable patch
> subject prefix. Look at git log of each file to get a feeling for what's
> the standard in each area.
This part is actually pretty annoying.
I hacked up a script (below) which seems to do a reasonable job in most
cases.
For this patch it gives:
$ for f in $(git ls-files -m); do ./guess-prefix.py $f; done
ARC:=20
ARC:=20
ARM:=20
arm64: ptrace:=20
KVM: PPC:=20
powerpc/powernv:
x86/efi:
block/sed-opal:
clocksource: mips-gic:=20
clocksource/drivers/sun5i:
drm/amd/display:
drm/amd/powerplay:
drm/msm/mdp5:
drm:=20
iommu/vt-d:
md:=20
soc: imx: gpc:=20
I think those are correct except for:
- "drm:" for "drivers/gpu/drm/scheduler" which has only a single commit.
- "md:" for "drivers/md/raid1.c" which is tricked by inconsistent
usage of "md: raid1:" and "md/raid1:".
But that seems like a reasonable hit rate.
Another approach would be to have a file that defines for each subsystem
what the preferred style is, but that is likely to be a PITA to
maintain.
cheers
#!/usr/bin/python3
import sys
import re
from subprocess import check_output
from collections import Counter
if len(sys.argv) !=3D 2:
print('Usage: %s <path>' % sys.argv[0], file=3Dsys.stderr)
sys.exit(1)
fname =3D sys.argv[1]
cmd =3D ['git', 'log', '--format=3D%s', '-n', '100', fname]
output =3D check_output(cmd).decode('utf-8')
ignore =3D ['License', 'Merge']
# Ordered list of patterns
patterns =3D [
# Common style "foo/bar/baz: Fix the foo"
re.compile('^([\w\-_]+: )+'),
# Less common "foo bar baz: Fix the foo"
re.compile('^([\w\-_]+:? )+: ')
]
words =3D []
for line in output.splitlines():
prefix =3D line.split()[0]
for patt in patterns:
match =3D patt.search(line)
if match:
prefix =3D match.group(0)
break
if prefix in ignore:
continue
words.append(prefix)
# Warn if we didn't find many examples
if len(words) < 5:
print("Warning: only found %d previous commits to guess from for" % len=
(words),
fname, file=3Dsys.stderr)
counts =3D Counter(words)
print(counts.most_common(1)[0][0])
^ permalink raw reply
* Re: [RFC PATCH 05/12] [WIP] powerpc/tm: Reclaim/recheckpoint on entry/exit
From: Cyril Bur @ 2018-02-20 6:32 UTC (permalink / raw)
To: Michael Neuling, benh, linuxppc-dev
In-Reply-To: <1519104300.5655.2.camel@neuling.org>
On Tue, 2018-02-20 at 16:25 +1100, Michael Neuling wrote:
> > > > @@ -1055,6 +1082,8 @@ void restore_tm_state(struct pt_regs *regs)
> > > > msr_diff = current->thread.ckpt_regs.msr & ~regs->msr;
> > > > msr_diff &= MSR_FP | MSR_VEC | MSR_VSX;
> > > >
> > > > + tm_recheckpoint(¤t->thread);
> > > > +
> > >
> > > So why do we do tm_recheckpoint at all? Shouldn't most of the tm_blah code go
> > > away in process.c after all this?
> > >
> >
> > I'm not sure I follow, we need to recheckpoint because we're going back
> > to userspace? Or would you rather calling the tm.S code directly from
> > the exception return path?
>
> Yeah, I was thinking the point of this series was. We do tm_reclaim right on
> entry and tm_recheckpoint right on exit.
>
Yeah that's the ultimate goal, considering I haven't been attacked or
offered more drugs I feel like what I've done isn't crazy. Your
feedback is great, thanks.
> The bits in between (ie. the tm_blah() calls process.c) would mostly go away.
>
>
> > Yes, I hope we'll be able to have a fairly big cleanup commit of tm_
> > code in process.c at the end of this series.
>
> Yep, agreed.
>
> Mikey
^ permalink raw reply
* Re: [PATCH] fix double ;;s in code
From: Jani Nikula @ 2018-02-20 8:03 UTC (permalink / raw)
To: Pavel Machek, Christophe LEROY, elfring, kernel list, vgupta,
linux, oleg, catalin.marinas, will.deacon, paulus, benh, mpe,
ard.biesheuvel, tglx, mingo, hpa, x86, scott.bauer,
jonathan.derrick, axboe, daniel.lezcano, maxime.ripard, wens,
alexander.deucher, christian.koenig, David1.Zhou, airlied,
robdclark, joro, shli, shawnguo, kernel, fabio.estevam, akpm,
Alexey.Brodkin, mhocko, vbabka, Vladislav.Zakharov, noamca,
yamada.masahiro, sboyd, viresh.kumar, linus.walleij, heiko, aik,
ruscur, david, fbarrat, alistair, robh, joe, harry.wentland,
tony.cheng, Wenjing.Liu, airlied, Ding.Wang, sylvia.tsai,
hersenxs.wu, Rex.Zhu, JinHuiEric.Huang, dan.carpenter, architt,
narmstrong, ville.syrjala, jcrouse, aishpant, noralf, andresx7,
Monk.Liu, nicolai.haehnle, Andrey.Grodzovsky, linux-snps-arc,
linux-arm-kernel, kvm-ppc, linuxppc-dev, linux-efi, linux-block,
amd-gfx, dri-devel, linux-arm-msm, freedreno, iommu, linux-raid
In-Reply-To: <20180219193312.GA3709@amd>
On Mon, 19 Feb 2018, Pavel Machek <pavel@ucw.cz> wrote:
> On Mon 2018-02-19 16:41:35, Daniel Vetter wrote:
>> Yeah, pls split this into one patch per area, with a suitable patch
>> subject prefix. Look at git log of each file to get a feeling for what's
>> the standard in each area.
>
> Yeah I can spend hour spliting it, and then people will ignore it
> anyway.
>
> If you care about one of the files being modified, please fix the
> bug, ";;" is a clear bug.
>
> If you don't care ... well I don't care either.
Look, if this causes just one conflict down the line because it touches
the kernel all over the place, then IMO it already wasn't worth
it. Merge conflicts are inevitable, but there's no reason to make life
harder just to cater for a cleanup patch. It's not that important.
Had it been split up, the drm parts would've been merged already.
BR,
Jani.
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply
* Re: [PATCH] scsi: cxlflash: Select SCSI_SCAN_ASYNC
From: Michael Ellerman @ 2018-02-20 8:56 UTC (permalink / raw)
To: Vaibhav Jain, Manoj N . Kumar
Cc: linux-scsi, Matthew R . Ochs, Vaibhav Jain, Frederic Barrat,
linux-kernel, Uma Krishnan, linuxppc-dev
In-Reply-To: <20180219071037.27938-1-vaibhav@linux.vnet.ibm.com>
Vaibhav Jain <vaibhav@linux.vnet.ibm.com> writes:
> The cxlflash driver uses "Asynchronous SCSI scanning" enabled by
> CONFIG_SCSI_SCAN_ASYNC. Without this enabled the modprobe of cxlflash
> module gets hung with following backtrace:
>
> Call Trace:
> __switch_to+0x2cc/0x470
> __schedule+0x288/0xab0
> schedule+0x40/0xc0
> schedule_timeout+0x254/0x4f0
> wait_for_common+0xdc/0x260
> flush_work+0x140/0x2a0
> work_on_cpu+0x88/0xb0
> pci_device_probe+0x1d0/0x220
> driver_probe_device+0x408/0x5b0
> __driver_attach+0x16c/0x1a0
> bus_for_each_dev+0xb8/0x110
> driver_attach+0x3c/0x60
> bus_add_driver+0x1d8/0x370
> driver_register+0x9c/0x180
> __pci_register_driver+0x74/0xa0
> init_cxlflash+0x158/0x1cc
> do_one_initcall+0x68/0x1e0
> do_init_module+0x90/0x254
> load_module+0x2f8c/0x3720
> SyS_finit_module+0xcc/0x140
> system_call+0x58/0x6c
Why does it "hang"? That's kind of bizarre, I would expect either a
build or runtime failure if a feature the driver requires is missing.
> diff --git a/drivers/scsi/cxlflash/Kconfig b/drivers/scsi/cxlflash/Kconfig
> index a011c5dbf214..f054c1b0fff3 100644
> --- a/drivers/scsi/cxlflash/Kconfig
> +++ b/drivers/scsi/cxlflash/Kconfig
> @@ -6,6 +6,7 @@ config CXLFLASH
> tristate "Support for IBM CAPI Flash"
> depends on PCI && SCSI && CXL && EEH
> select IRQ_POLL
> + select SCSI_SCAN_ASYNC
It's user configurable, so it's rude to select it. It can also be
disabled on the kernel command line, so this seems like a fragile
solution.
cheers
^ permalink raw reply
* Re: [PATCH V5] cxl: Fix timebase synchronization status on P9
From: Frederic Barrat @ 2018-02-20 9:09 UTC (permalink / raw)
To: Christophe Lombard, linuxppc-dev, vaibhav, andrew.donnellan
In-Reply-To: <1519050491-15326-1-git-send-email-clombard@linux.vnet.ibm.com>
Le 19/02/2018 à 15:28, Christophe Lombard a écrit :
> The PSL Timebase register is updated by the PSL to maintain the
> timebase.
> On P9, the Timebase value is only provided by the CAPP as received
> the last time a timebase request was performed.
> The timebase requests are initiated through the adapter configuration or
> application registers.
> The specific sysfs entry "/sys/class/cxl/cardxx/psl_timebase_synced" is
> now dynamically updated according the content of the PSL Timebase
> register.
>
> Signed-off-by: Christophe Lombard <clombard@linux.vnet.ibm.com>
>
> ---
> This patch applies on top of this patch:
> http://patchwork.ozlabs.org/patch/873663/
>
> Changelog[v5]
> - Rebased to latest upstream.
> - Changed the type of 'delta'
>
> Changelog[v4]
> - Rebased to latest upstream.
> - Added log message.
>
> Changelog[v3]
> - Rebased to latest upstream.
> - Dynamic update is now applied to P8.
>
> Changelog[v2]
> - Missing Signed-off-by.
> - Spaces required around the ':'.
> ---
> drivers/misc/cxl/pci.c | 17 -----------------
> drivers/misc/cxl/sysfs.c | 10 ++++++++++
> 2 files changed, 10 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
> index 66eed6a..3247eaf 100644
> --- a/drivers/misc/cxl/pci.c
> +++ b/drivers/misc/cxl/pci.c
> @@ -606,9 +606,6 @@ static u64 timebase_read_xsl(struct cxl *adapter)
>
> static void cxl_setup_psl_timebase(struct cxl *adapter, struct pci_dev *dev)
> {
> - u64 psl_tb;
> - int delta;
> - unsigned int retry = 0;
> struct device_node *np;
>
> adapter->psl_timebase_synced = false;
> @@ -636,20 +633,6 @@ static void cxl_setup_psl_timebase(struct cxl *adapter, struct pci_dev *dev)
> cxl_p1_write(adapter, CXL_PSL_Control, 0x0000000000000000);
> cxl_p1_write(adapter, CXL_PSL_Control, CXL_PSL_Control_tb);
>
> - /* Wait until CORE TB and PSL TB difference <= 16usecs */
> - do {
> - msleep(1);
> - if (retry++ > 5) {
> - dev_info(&dev->dev, "PSL timebase can't synchronize\n");
> - return;
> - }
> - psl_tb = adapter->native->sl_ops->timebase_read(adapter);
> - delta = mftb() - psl_tb;
> - if (delta < 0)
> - delta = -delta;
> - } while (tb_to_ns(delta) > 16000);
> -
> - adapter->psl_timebase_synced = true;
> return;
> }
>
> diff --git a/drivers/misc/cxl/sysfs.c b/drivers/misc/cxl/sysfs.c
> index a8b6d6a..a20bf8e 100644
> --- a/drivers/misc/cxl/sysfs.c
> +++ b/drivers/misc/cxl/sysfs.c
> @@ -62,6 +62,16 @@ static ssize_t psl_timebase_synced_show(struct device *device,
> char *buf)
> {
> struct cxl *adapter = to_cxl_adapter(device);
> + u64 psl_tb, delta;
> +
> + psl_tb = adapter->native->sl_ops->timebase_read(adapter);
It would dump core in a lpar.
pHyp is supposed to have it initialized, though we have no way to know
for sure. On p8, we're also only supporting a card which doesn't have
timebase problem on powerVM and it doesn't apply to p9. So I think we
just need to recompute the status on bare-metal only.
Fred
> + delta = abs(mftb() - psl_tb);
> +
> + /* CORE TB and PSL TB difference <= 16usecs ? */
> + adapter->psl_timebase_synced = (tb_to_ns(delta) < 16000) ? true : false;
> + pr_devel("PSL timebase %s - delta: 0x%016llx\n",
> + (tb_to_ns(delta) < 16000) ? "synchronized" :
> + "not synchronized", tb_to_ns(delta));
>
> return scnprintf(buf, PAGE_SIZE, "%i\n", adapter->psl_timebase_synced);
> }
>
^ permalink raw reply
* Re: [RFC][PATCH bpf v2 1/2] bpf: allow 64-bit offsets for bpf function calls
From: Michael Ellerman @ 2018-02-20 9:29 UTC (permalink / raw)
To: Naveen N. Rao, ast, Daniel Borkmann, Sandipan Das
Cc: linuxppc-dev, netdev, Michael Holzheu
In-Reply-To: <1518791626.5484j97if6.naveen@linux.ibm.com>
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> Daniel Borkmann wrote:
>> On 02/15/2018 05:25 PM, Daniel Borkmann wrote:
>>> On 02/13/2018 05:05 AM, Sandipan Das wrote:
>>>> The imm field of a bpf_insn is a signed 32-bit integer. For
>>>> JIT-ed bpf-to-bpf function calls, it stores the offset from
>>>> __bpf_call_base to the start of the callee function.
>>>>
>>>> For some architectures, such as powerpc64, it was found that
>>>> this offset may be as large as 64 bits because of which this
>>>> cannot be accomodated in the imm field without truncation.
>>>>
>>>> To resolve this, we additionally make aux->func within each
>>>> bpf_prog associated with the functions to point to the list
>>>> of all function addresses determined by the verifier.
>>>>
>>>> We keep the value assigned to the off field of the bpf_insn
>>>> as a way to index into aux->func and also set aux->func_cnt
>>>> so that this can be used for performing basic upper bound
>>>> checks for the off field.
>>>>
>>>> Signed-off-by: Sandipan Das <sandipan@linux.vnet.ibm.com>
>>>> ---
>>>> v2: Make aux->func point to the list of functions determined
>>>> by the verifier rather than allocating a separate callee
>>>> list for each function.
>>>
>>> Approach looks good to me; do you know whether s390x JIT would
>>> have similar requirement? I think one limitation that would still
>>> need to be addressed later with such approach would be regarding the
>>> xlated prog dump in bpftool, see 'BPF calls via JIT' in 7105e828c087
>>> ("bpf: allow for correlation of maps and helpers in dump"). Any
>>> ideas for this (potentially if we could use off + imm for calls,
>>> we'd get to 48 bits, but that seems still not be enough as you say)?
>
> All good points. I'm not really sure how s390x works, so I can't comment
> on that, but I'm copying Michael Holzheu for his consideration.
>
> With the existing scheme, 48 bits won't be enough, so we rejected that
> approach. I can also see how this will be a problem with bpftool, but I
> haven't looked into it in detail. I wonder if we can annotate the output
> to indicate the function being referred to?
>
>>
>> One other random thought, although I'm not sure how feasible this
>> is for ppc64 JIT to realize ... but idea would be to have something
>> like the below:
>>
>> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
>> index 29ca920..daa7258 100644
>> --- a/kernel/bpf/core.c
>> +++ b/kernel/bpf/core.c
>> @@ -512,6 +512,11 @@ int bpf_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
>> return ret;
>> }
>>
>> +void * __weak bpf_jit_image_alloc(unsigned long size)
>> +{
>> + return module_alloc(size);
>> +}
>> +
>> struct bpf_binary_header *
>> bpf_jit_binary_alloc(unsigned int proglen, u8 **image_ptr,
>> unsigned int alignment,
>> @@ -525,7 +530,7 @@ bpf_jit_binary_alloc(unsigned int proglen, u8 **image_ptr,
>> * random section of illegal instructions.
>> */
>> size = round_up(proglen + sizeof(*hdr) + 128, PAGE_SIZE);
>> - hdr = module_alloc(size);
>> + hdr = bpf_jit_image_alloc(size);
>> if (hdr == NULL)
>> return NULL;
>>
>> And ppc64 JIT could override bpf_jit_image_alloc() in a similar way
>> like some archs would override the module_alloc() helper through a
>> custom implementation, usually via __vmalloc_node_range(), so we
>> could perhaps fit the range for BPF JITed images in a way that they
>> could use the 32bit imm in the end? There are not that many progs
>> loaded typically, so the range could be a bit narrower in such case
>> anyway. (Not sure if this would work out though, but I thought to
>> bring it up.)
>
> That'd be a good option to consider. I don't think we want to allocate
> anything from the linear memory range since users could load
> unprivileged BPF programs and consume a lot of memory that way. I doubt
> if we can map vmalloc'ed memory into the 0xc0 address range, but I'm not
> entirely sure.
>
> Michael,
> Is the above possible? The question is if we can have BPF programs be
> allocated within 4GB of __bpf_call_base (which is a kernel symbol), so
> that calls to those programs can be encoded in a 32-bit immediate field
> in a BPF instruction.
Hmmm.
It's not technically impossible, but I don't think it's really a good
option.
The 0xc range is a linear mapping of RAM, and the kernel tends to be
near the start of RAM for reasons. That means there generally isn't a
hole in the 0xc range within 4GB for you to map BPF programs.
You could create a hole by making the 0xc mapping non linear, ie.
mapping some RAM near the kernel elsewhere in the 0xc range, to make a
hole that you can then remap BPF programs into. But I think that would
cause a lot of bugs, it's a pretty fundamental assumption that the
linear mapping is 1:1.
> As an extension, we may be able to extend it to
> 48-bits by combining with another BPF instruction field (offset). In
> either case, the vmalloc'ed address range won't work.
48-bits could possibly work, we don't have systems with that much RAM
*yet*. So you could remap the BPF programs at the end of the 0xc range,
or somewhere we have a gap in RAM.
That would probably still confuse some things, ie. the 0xc mapping would
be a 1:1 mapping of RAM plus some other stuff, but it might be tractable
to fix.
We don't have page tables for the 0xc range when using the hash MMU, so
we'd need to either fix that or use some bespoke data structure for
keeping track of the mappings.
It doesn't really appeal :)
We load modules in the 0xd range, and they call functions in the kernel,
we handle that with trampolines. Can you do something similar?
You obviously still need a 64-bit branch somewhere, but perhaps you only
need one, or one per BPF program, rather than one per function call.
> The alternative is to pass the full 64-bit address of the BPF program in
> an auxiliary field (as proposed in this patch set) but we need to fix it
> up for 'bpftool' as well.
OK. You'll have to explain to me how bpftool is related, I thought it
was just for loading/examining BPF programs.
cheers
^ 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