From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-08-25 04:30:54
These are rebased patches leftover from the unmerged bit of
the idle series.
Based on feedback, I dropped one of the KVM patches, and reworked
the code a bit so it is easier to restore the ability for KVM to
grab secondaries into real mode.
I did a bit more benchmarking, and all up these patches improve 2
CPU ping-pong context switch benchmark on a POWER9 by around 4-6%
(depending on what CPUs and idle states are used).
Nicholas Piggin (4):
KVM: PPC: Book3S HV: POWER9 does not require secondary thread
management
powerpc/64s: idle POWER9 can execute stop without a sync sequence
powerpc/64s: idle POWER9 can execute stop in virtual mode
powerpc/64s: idle ESL=0 stop can avoid MSR and save/restore overhead
arch/powerpc/include/asm/cpuidle.h | 16 -----
arch/powerpc/include/asm/kvm_book3s_asm.h | 4 ++
arch/powerpc/kernel/idle_book3s.S | 103 ++++++++++++++++++++++--------
arch/powerpc/kvm/book3s_hv.c | 14 +++-
arch/powerpc/kvm/book3s_hv_rmhandlers.S | 8 +++
5 files changed, 101 insertions(+), 44 deletions(-)
--
2.13.3
From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-08-25 04:30:57
POWER9 CPUs have independent MMU contexts per thread, so KVM does not
need to quiesce secondary threads, so the hwthread_req/hwthread_state
protocol does not have to be used. So patch it away on POWER9, and patch
away the branch from the Linux idle wakeup to kvm_start_guest that is
never used.
Add a warning and error out of kvmppc_grab_hwthread in case it is ever
called on POWER9.
This avoids a hwsync in the idle wakeup path on POWER9.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/kvm_book3s_asm.h | 4 ++++
arch/powerpc/kernel/idle_book3s.S | 35 +++++++++++++++++++++----------
arch/powerpc/kvm/book3s_hv.c | 14 ++++++++++++-
arch/powerpc/kvm/book3s_hv_rmhandlers.S | 8 +++++++
4 files changed, 49 insertions(+), 12 deletions(-)
@@ -2111,6 +2111,16 @@ static int kvmppc_grab_hwthread(int cpu)structpaca_struct*tpaca;longtimeout=10000;+/*+*ISAv3.0idleroutinesdonotsethwthread_stateortest+*hwthread_req,sotheycannotgrabidlethreads.+*/+if(cpu_has_feature(CPU_FTR_ARCH_300)){+WARN_ON(1);+pr_err("KVM: can not control sibling threads\n");+return-EBUSY;+}+tpaca=&paca[cpu];/* Ensure the thread won't go into the kernel if it wakes */
From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-08-25 04:31:04
The hardware can execute stop in any context, and KVM does not
require real mode because siblings do not share MMU state. This
saves a switch to real-mode when going idle.
Acked-by: Gautham R. Shenoy <redacted>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/idle_book3s.S | 9 +++++++++
1 file changed, 9 insertions(+)
From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-08-25 04:31:08
When stop is executed with EC=ESL=0, it appears to execute like a
normal instruction (resuming from NIP when woken by interrupt).
So all the save/restore handling can be avoided completely. In
particular NV GPRs do not have to be saved, and MSR does not have
to be switched back to kernel MSR.
So move the test for "lite" sleep states out to power9_idle_stop.
Reviewed-by: Gautham R. Shenoy <redacted>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/idle_book3s.S | 35 ++++++++++++++++++++++++-----------
1 file changed, 24 insertions(+), 11 deletions(-)
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2017-08-28 11:49:45
Nicholas Piggin [off-list ref] writes:
POWER9 CPUs have independent MMU contexts per thread, so KVM does not
need to quiesce secondary threads, so the hwthread_req/hwthread_state
protocol does not have to be used. So patch it away on POWER9, and patch
away the branch from the Linux idle wakeup to kvm_start_guest that is
never used.
Add a warning and error out of kvmppc_grab_hwthread in case it is ever
called on POWER9.
This avoids a hwsync in the idle wakeup path on POWER9.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/kvm_book3s_asm.h | 4 ++++
arch/powerpc/kernel/idle_book3s.S | 35 +++++++++++++++++++++----------
arch/powerpc/kvm/book3s_hv.c | 14 ++++++++++++-
arch/powerpc/kvm/book3s_hv_rmhandlers.S | 8 +++++++
4 files changed, 49 insertions(+), 12 deletions(-)
Paulus do you mind acking this and I'll put it in my ppc-kvm topic
branch.
cheers
From: Paul Mackerras <hidden> Date: 2017-08-29 00:20:52
On Fri, Aug 25, 2017 at 02:30:35PM +1000, Nicholas Piggin wrote:
quoted hunk
The hardware can execute stop in any context, and KVM does not
require real mode because siblings do not share MMU state. This
saves a switch to real-mode when going idle.
Acked-by: Gautham R. Shenoy <redacted>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/idle_book3s.S | 9 +++++++++
1 file changed, 9 insertions(+)
From: Paul Mackerras <hidden> Date: 2017-08-29 00:20:52
On Fri, Aug 25, 2017 at 02:30:33PM +1000, Nicholas Piggin wrote:
POWER9 CPUs have independent MMU contexts per thread, so KVM does not
need to quiesce secondary threads, so the hwthread_req/hwthread_state
protocol does not have to be used. So patch it away on POWER9, and patch
away the branch from the Linux idle wakeup to kvm_start_guest that is
never used.
Add a warning and error out of kvmppc_grab_hwthread in case it is ever
called on POWER9.
This avoids a hwsync in the idle wakeup path on POWER9.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Some of the feature sections seem a little unnecessary except for
documentation purposes (e.g. the second one added after the
kvm_no_guest label), but they won't actually hurt, so:
Acked-by: Paul Mackerras <redacted>
We had to do this sequence on POWER7 also, which is architecture
v2.06. Thus the comments and the naming (ARCH207_*) are a bit
misleading here. The actual code change looks OK.
Paul.
From: Paul Mackerras <hidden> Date: 2017-08-29 00:20:52
On Fri, Aug 25, 2017 at 02:30:36PM +1000, Nicholas Piggin wrote:
quoted hunk
When stop is executed with EC=ESL=0, it appears to execute like a
normal instruction (resuming from NIP when woken by interrupt).
So all the save/restore handling can be avoided completely. In
particular NV GPRs do not have to be saved, and MSR does not have
to be switched back to kernel MSR.
So move the test for "lite" sleep states out to power9_idle_stop.
Reviewed-by: Gautham R. Shenoy <redacted>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/idle_book3s.S | 35 ++++++++++++++++++++++++-----------
1 file changed, 24 insertions(+), 11 deletions(-)
I realize you're just moving existing code, but I think this would be
clearer (to me, anyway) as
andis. r4, r3, (PSSCR_EC | PSSCR_ESL)@h
Apart from that very minor nit,
Reviewed-by: Paul Mackerras <redacted>
From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-08-29 01:39:57
On Tue, 29 Aug 2017 10:20:48 +1000
Paul Mackerras [off-list ref] wrote:
On Fri, Aug 25, 2017 at 02:30:36PM +1000, Nicholas Piggin wrote:
quoted
When stop is executed with EC=ESL=0, it appears to execute like a
normal instruction (resuming from NIP when woken by interrupt).
So all the save/restore handling can be avoided completely. In
particular NV GPRs do not have to be saved, and MSR does not have
to be switched back to kernel MSR.
So move the test for "lite" sleep states out to power9_idle_stop.
Reviewed-by: Gautham R. Shenoy <redacted>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/idle_book3s.S | 35 ++++++++++++++++++++++++-----------
1 file changed, 24 insertions(+), 11 deletions(-)
I realize you're just moving existing code, but I think this would be
clearer (to me, anyway) as
andis. r4, r3, (PSSCR_EC | PSSCR_ESL)@h
Agreed, it's not a very helpful extra indirection. Perhaps we could
do a cleanup patch to fix that (and your other points like the 206/207
confusion).
Thanks for the reviews.
Apart from that very minor nit,
Reviewed-by: Paul Mackerras <redacted>
We had to do this sequence on POWER7 also, which is architecture
v2.06. Thus the comments and the naming (ARCH207_*) are a bit
misleading here. The actual code change looks OK.
I'll just drop the name change, I don't think it's crucial. That makes
P9 the special case.
We can come up with a better name or something in future.
Unless Nick objects?
cheers
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2017-08-30 11:26:00
Nicholas Piggin [off-list ref] writes:
When stop is executed with EC=ESL=0, it appears to execute like a
normal instruction (resuming from NIP when woken by interrupt).
So all the save/restore handling can be avoided completely. In
particular NV GPRs do not have to be saved, and MSR does not have
to be switched back to kernel MSR.
So move the test for "lite" sleep states out to power9_idle_stop.
Reviewed-by: Gautham R. Shenoy <redacted>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/idle_book3s.S | 35 ++++++++++++++++++++++++-----------
1 file changed, 24 insertions(+), 11 deletions(-)
This is blowing up for me on mambo:
usbcore: registered new interface driver usb-storage
Disabling lock debugging due to kernel taint
Severe Machine check interrupt [Not recovered]
NIP [c0000000002a0c04]: kmem_cache_free+0x64/0x2c0
Initiator: CPU
Error type: Real address [Load/Store (foreign)]
opal: Hardware platform error: Unrecoverable Machine Check exception
CPU: 0 PID: 0 Comm: swapper/0 Tainted: G M 4.13.0-rc2-gcc-6.3.1-00257-g26268bb39bff #543
task: c0000000016b1200 task.stack: c00000000175c000
NIP: c0000000002a0c04 LR: c0000000001128ec CTR: c000000000112800
REGS: c00000003fff7d80 TRAP: 0200 Tainted: G M (4.13.0-rc2-gcc-6.3.1-00257-g26268bb39bff)
MSR: 9000000000209003 <SF,HV,EE,ME,RI,LE> CR: 28002828 XER: 20000000
CFAR: c0000000001128e8 DAR: c00a0000003c3ce0 DSISR: 00000008 SOFTE: 1
GPR00: c0000000001128ec c00000000175f7b0 c000000001760500 c0000000f001a000
GPR04: c0000000f0f37f00 0000000000000003 00000000000220c3 0000000000000000
GPR08: 0000000000000000 00000000003c3cc0 c0000000017e7758 0000000000000000
GPR12: c000000000112800 c00000000fff0000 c000000000112800 c0000000f0f37f90
GPR16: c00000000175c000 c00000000175c000 0000000000000001 c0000000016d7900
GPR20: c00000000179ba98 7fffffffffffffff c00000000175c000 0000000000000000
GPR24: c00000000017a8c0 000000000000000a c0000000016d8a00 c00000000175f8d0
GPR28: c0000000001128ec c0000000f0f37f00 c00a0000003c3cc0 c0000000f001a000
NIP [c0000000002a0c04] kmem_cache_free+0x64/0x2c0
LR [c0000000001128ec] put_cred_rcu+0xec/0x140
Call Trace:
[c00000000175f7b0] [c00000000175f800] init_thread_union+0x3800/0x4000 (unreliable)
[c00000000175f840] [c0000000001128ec] put_cred_rcu+0xec/0x140
[c00000000175f8b0] [c00000000017a908] rcu_process_callbacks+0x438/0x6a0
[c00000000175f980] [c0000000000e5e28] __do_softirq+0x198/0x310
[c00000000175fa70] [c0000000000e6248] irq_exit+0xf8/0x140
[c00000000175fa90] [c000000000023710] timer_interrupt+0xa0/0xe0
[c00000000175fac0] [c000000000008fcc] decrementer_common+0x11c/0x120
--- interrupt: 901 at replay_interrupt_return+0x0/0x4
LR = arch_local_irq_restore.part.1+0x84/0xb0
[c00000000175fdb0] [c00000000175c000] init_thread_union+0x0/0x4000 (unreliable)
[c00000000175fdd0] [c00000000001cde0] arch_cpu_idle+0xe0/0x140
[c00000000175fe00] [c0000000007f4f44] default_idle_call+0x44/0x84
[c00000000175fe20] [c000000000142e54] do_idle+0x254/0x320
[c00000000175fe90] [c000000000143280] cpu_startup_entry+0x30/0x40
[c00000000175fec0] [c00000000000d2d8] rest_init+0x2f8/0x320
[c00000000175ff00] [c000000001000d74] start_kernel+0x510/0x52c
[c00000000175ff90] [c00000000000ab70] start_here_common+0x1c/0x4ac
Instruction dump:
60000000 e9230008 71280100 40820170 2fbf0000 419e013c 3d420008 394a7258
7bbe8502 7bc93664 ebca0000 7fde4a14 <e93e0020> 712a0001 40820234 e93f0008
[ 1.217346467,0] OPAL: Reboot requested due to Platform error.
[ 1.217351582,3] OPAL: failed to log an error
[ 1.217358188,5] OPAL: Reboot request...
I'll just drop it for now.
cheers
From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-08-30 12:11:05
On Wed, 30 Aug 2017 21:25:59 +1000
Michael Ellerman [off-list ref] wrote:
Nicholas Piggin [off-list ref] writes:
quoted
When stop is executed with EC=ESL=0, it appears to execute like a
normal instruction (resuming from NIP when woken by interrupt).
So all the save/restore handling can be avoided completely. In
particular NV GPRs do not have to be saved, and MSR does not have
to be switched back to kernel MSR.
So move the test for "lite" sleep states out to power9_idle_stop.
Reviewed-by: Gautham R. Shenoy <redacted>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/idle_book3s.S | 35 ++++++++++++++++++++++++-----------
1 file changed, 24 insertions(+), 11 deletions(-)
This is blowing up for me on mambo:
Oh this is a known bug in mambo that does not match the hardware.
You need >= Mambo.7.8.21, or this firmware patch to work around
the issue for old mambos.
mambo.git 58d3162f4d6204fc077ff4a6ba47e4d1e19d5120
https://lists.ozlabs.org/pipermail/skiboot/2017-August/008768.html
Thanks,
Nick
From: Michael Ellerman <hidden> Date: 2017-08-31 11:36:19
On Fri, 2017-08-25 at 04:30:33 UTC, Nicholas Piggin wrote:
POWER9 CPUs have independent MMU contexts per thread, so KVM does not
need to quiesce secondary threads, so the hwthread_req/hwthread_state
protocol does not have to be used. So patch it away on POWER9, and patch
away the branch from the Linux idle wakeup to kvm_start_guest that is
never used.
Add a warning and error out of kvmppc_grab_hwthread in case it is ever
called on POWER9.
This avoids a hwsync in the idle wakeup path on POWER9.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Acked-by: Paul Mackerras <redacted>
From: Michael Ellerman <hidden> Date: 2017-08-31 11:36:20
On Fri, 2017-08-25 at 04:30:35 UTC, Nicholas Piggin wrote:
The hardware can execute stop in any context, and KVM does not
require real mode because siblings do not share MMU state. This
saves a switch to real-mode when going idle.
Acked-by: Gautham R. Shenoy <redacted>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Reviewed-by: Paul Mackerras <redacted>
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2017-09-01 09:39:43
Nicholas Piggin [off-list ref] writes:
On Wed, 30 Aug 2017 21:25:59 +1000
Michael Ellerman [off-list ref] wrote:
quoted
Nicholas Piggin [off-list ref] writes:
quoted
When stop is executed with EC=ESL=0, it appears to execute like a
normal instruction (resuming from NIP when woken by interrupt).
So all the save/restore handling can be avoided completely. In
particular NV GPRs do not have to be saved, and MSR does not have
to be switched back to kernel MSR.
So move the test for "lite" sleep states out to power9_idle_stop.
Reviewed-by: Gautham R. Shenoy <redacted>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/idle_book3s.S | 35 ++++++++++++++++++++++++-----------
1 file changed, 24 insertions(+), 11 deletions(-)
This is blowing up for me on mambo:
Oh this is a known bug in mambo that does not match the hardware.
You need >= Mambo.7.8.21, or this firmware patch to work around
the issue for old mambos.
As discussed elsewhere this still breaks on new mambo with more than one
CPU. So I've dropped it for now.
cheers
From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-09-20 13:56:46
On Fri, 01 Sep 2017 19:39:41 +1000
Michael Ellerman [off-list ref] wrote:
Nicholas Piggin [off-list ref] writes:
quoted
On Wed, 30 Aug 2017 21:25:59 +1000
Michael Ellerman [off-list ref] wrote:
quoted
Nicholas Piggin [off-list ref] writes:
quoted
When stop is executed with EC=ESL=0, it appears to execute like a
normal instruction (resuming from NIP when woken by interrupt).
So all the save/restore handling can be avoided completely. In
particular NV GPRs do not have to be saved, and MSR does not have
to be switched back to kernel MSR.
So move the test for "lite" sleep states out to power9_idle_stop.
Reviewed-by: Gautham R. Shenoy <redacted>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/idle_book3s.S | 35 ++++++++++++++++++++++++-----------
1 file changed, 24 insertions(+), 11 deletions(-)
This is blowing up for me on mambo:
Oh this is a known bug in mambo that does not match the hardware.
You need >= Mambo.7.8.21, or this firmware patch to work around
the issue for old mambos.
As discussed elsewhere this still breaks on new mambo with more than one
CPU. So I've dropped it for now.
This now seems to be properly fixed in mambo with commit 11783550ee11.
Skiboot also has a patch merged which disables the problematic state on
mambo so older versions won't crash.
d2a24406a49 ("idle: disable stop*_lite POWER9 idle states for Mambo platform")
So this could be re-applied now.
Thanks,
Nick