From: Leonardo Bras <hidden> Date: 2021-02-05 06:08:11
Before guest entry, TBU40 register is changed to reflect guest timebase.
After exitting guest, the register is reverted to it's original value.
If one tries to get the timestamp from host between those changes, it
will present an incorrect value.
An example would be trying to add a tracepoint in
kvmppc_guest_entry_inject_int(), which depending on last tracepoint
acquired could actually cause the host to crash.
Save the Timebase Offset to PACA and use it on sched_clock() to always
get the correct timestamp.
Signed-off-by: Leonardo Bras <redacted>
Suggested-by: Paul Mackerras <redacted>
---
Changes since v1:
- Subtracts offset only when CONFIG_KVM_BOOK3S_HANDLER and
CONFIG_PPC_BOOK3S_64 are defined.
---
arch/powerpc/include/asm/kvm_book3s_asm.h | 1 +
arch/powerpc/kernel/asm-offsets.c | 1 +
arch/powerpc/kernel/time.c | 8 +++++++-
arch/powerpc/kvm/book3s_hv.c | 2 ++
arch/powerpc/kvm/book3s_hv_rmhandlers.S | 2 ++
5 files changed, 13 insertions(+), 1 deletion(-)
From: Nicholas Piggin <npiggin@gmail.com> Date: 2021-02-05 06:29:46
Excerpts from Leonardo Bras's message of February 5, 2021 4:06 pm:
Before guest entry, TBU40 register is changed to reflect guest timebase.
After exitting guest, the register is reverted to it's original value.
If one tries to get the timestamp from host between those changes, it
will present an incorrect value.
An example would be trying to add a tracepoint in
kvmppc_guest_entry_inject_int(), which depending on last tracepoint
acquired could actually cause the host to crash.
Save the Timebase Offset to PACA and use it on sched_clock() to always
get the correct timestamp.
Ouch. Not sure how reasonable it is to half switch into guest registers
and expect to call into the wider kernel, fixing things up as we go.
What if mftb is used in other places?
Especially as it doesn't seem like there is a reason that function _has_
to be called after the timebase is switched to guest, that's just how
the code is structured.
As a local hack to work out a bug okay. If you really need it upstream
could you put it under a debug config option?
Thanks,
Nick
quoted hunk
Signed-off-by: Leonardo Bras <redacted>
Suggested-by: Paul Mackerras <redacted>
---
Changes since v1:
- Subtracts offset only when CONFIG_KVM_BOOK3S_HANDLER and
CONFIG_PPC_BOOK3S_64 are defined.
---
arch/powerpc/include/asm/kvm_book3s_asm.h | 1 +
arch/powerpc/kernel/asm-offsets.c | 1 +
arch/powerpc/kernel/time.c | 8 +++++++-
arch/powerpc/kvm/book3s_hv.c | 2 ++
arch/powerpc/kvm/book3s_hv_rmhandlers.S | 2 ++
5 files changed, 13 insertions(+), 1 deletion(-)
From: Leonardo Bras <hidden> Date: 2021-02-05 07:02:23
Hey Nick, thanks for reviewing :)
On Fri, 2021-02-05 at 16:28 +1000, Nicholas Piggin wrote:
Excerpts from Leonardo Bras's message of February 5, 2021 4:06 pm:
quoted
Before guest entry, TBU40 register is changed to reflect guest timebase.
After exitting guest, the register is reverted to it's original value.
If one tries to get the timestamp from host between those changes, it
will present an incorrect value.
An example would be trying to add a tracepoint in
kvmppc_guest_entry_inject_int(), which depending on last tracepoint
acquired could actually cause the host to crash.
Save the Timebase Offset to PACA and use it on sched_clock() to always
get the correct timestamp.
Ouch. Not sure how reasonable it is to half switch into guest registers
and expect to call into the wider kernel, fixing things up as we go.
What if mftb is used in other places?
IIUC, the CPU is not supposed to call anything as host between guest
entry and guest exit, except guest-related cases, like
kvmppc_guest_entry_inject_int(), but anyway, if something calls mftb it
will still get the same value as before.
This is only supposed to change stuff that depends on sched_clock, like
Tracepoints, that can happen in those exceptions.
Especially as it doesn't seem like there is a reason that function _has_
to be called after the timebase is switched to guest, that's just how
the code is structured.
Correct, but if called, like in rb routines, used by tracepoints, the
difference between last tb and current (lower) tb may cause the CPU to
trap PROGRAM exception, crashing host.
As a local hack to work out a bug okay. If you really need it upstream
could you put it under a debug config option?
You mean something that is automatically selected whenever those
configs are enabled?
CONFIG_TRACEPOINT && CONFIG_KVM_BOOK3S_HANDLER && CONFIG_PPC_BOOK3S_64
Or something the user need to select himself in menuconfig?
Thanks,
Nick
Thank you!
Leonardo Bras
quoted
Signed-off-by: Leonardo Bras <redacted>
Suggested-by: Paul Mackerras <redacted>
---
Changes since v1:
- Subtracts offset only when CONFIG_KVM_BOOK3S_HANDLER and
CONFIG_PPC_BOOK3S_64 are defined.
---
arch/powerpc/include/asm/kvm_book3s_asm.h | 1 +
arch/powerpc/kernel/asm-offsets.c | 1 +
arch/powerpc/kernel/time.c | 8 +++++++-
arch/powerpc/kvm/book3s_hv.c | 2 ++
arch/powerpc/kvm/book3s_hv_rmhandlers.S | 2 ++
5 files changed, 13 insertions(+), 1 deletion(-)
From: Leonardo Bras <hidden> Date: 2021-02-05 20:38:18
Hello Fabiano,
Thanks for reviewing!
(answers inline)
On Fri, 2021-02-05 at 10:09 -0300, Fabiano Rosas wrote:
Leonardo Bras [off-list ref] writes:
quoted
Before guest entry, TBU40 register is changed to reflect guest timebase.
After exitting guest, the register is reverted to it's original value.
If one tries to get the timestamp from host between those changes, it
will present an incorrect value.
An example would be trying to add a tracepoint in
kvmppc_guest_entry_inject_int(), which depending on last tracepoint
acquired could actually cause the host to crash.
Save the Timebase Offset to PACA and use it on sched_clock() to always
get the correct timestamp.
Signed-off-by: Leonardo Bras <redacted>
Suggested-by: Paul Mackerras <redacted>
---
Changes since v1:
- Subtracts offset only when CONFIG_KVM_BOOK3S_HANDLER and
CONFIG_PPC_BOOK3S_64 are defined.
---
arch/powerpc/include/asm/kvm_book3s_asm.h | 1 +
arch/powerpc/kernel/asm-offsets.c | 1 +
arch/powerpc/kernel/time.c | 8 +++++++-
arch/powerpc/kvm/book3s_hv.c | 2 ++
arch/powerpc/kvm/book3s_hv_rmhandlers.S | 2 ++
5 files changed, 13 insertions(+), 1 deletion(-)
u64 cfar;
u64 ppr;
u64 host_fscr;
+ u64 tb_offset; /* Timebase offset: keeps correct
timebase while on guest */
Couldn't you use the vc->tb_offset_applied for this? We have a reference
for the vcore in the hstate already.
But it's a pointer, which means we would have to keep checking for NULL
every time we need sched_clock().
Potentially it would cost a cache miss for PACA memory region that
contain vc, another for getting the part of *vc that contains the
tb_offset_applied, instead of only one for PACA struct region that
contains tb_offset.
On the other hand, it got me thinking: If the offset is applied per
cpu, why don't we get this info only in PACA, instead of in vc?
It could be a general way to get an offset applied for any purpose and
still get the sched_clock() right.
(Not that I have any idea of any other purpose we could use it)
Best regards!
Leonardo Bras
Before guest entry, TBU40 register is changed to reflect guest timebase.
After exitting guest, the register is reverted to it's original value.
If one tries to get the timestamp from host between those changes, it
will present an incorrect value.
An example would be trying to add a tracepoint in
kvmppc_guest_entry_inject_int(), which depending on last tracepoint
acquired could actually cause the host to crash.
Save the Timebase Offset to PACA and use it on sched_clock() to always
get the correct timestamp.
Signed-off-by: Leonardo Bras <redacted>
Suggested-by: Paul Mackerras <redacted>
---
Changes since v1:
- Subtracts offset only when CONFIG_KVM_BOOK3S_HANDLER and
CONFIG_PPC_BOOK3S_64 are defined.
---
arch/powerpc/include/asm/kvm_book3s_asm.h | 1 +
arch/powerpc/kernel/asm-offsets.c | 1 +
arch/powerpc/kernel/time.c | 8 +++++++-
arch/powerpc/kvm/book3s_hv.c | 2 ++
arch/powerpc/kvm/book3s_hv_rmhandlers.S | 2 ++
5 files changed, 13 insertions(+), 1 deletion(-)
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2021-02-06 02:57:39
Nicholas Piggin [off-list ref] writes:
Excerpts from Leonardo Bras's message of February 5, 2021 4:06 pm:
quoted
Before guest entry, TBU40 register is changed to reflect guest timebase.
After exitting guest, the register is reverted to it's original value.
If one tries to get the timestamp from host between those changes, it
will present an incorrect value.
An example would be trying to add a tracepoint in
kvmppc_guest_entry_inject_int(), which depending on last tracepoint
acquired could actually cause the host to crash.
Save the Timebase Offset to PACA and use it on sched_clock() to always
get the correct timestamp.
Ouch. Not sure how reasonable it is to half switch into guest registers
and expect to call into the wider kernel, fixing things up as we go.
Yeah it's not.
We need to disable tracing on those routines that are called in that
half-exited state.
cheers
From: Nicholas Piggin <npiggin@gmail.com> Date: 2021-02-06 04:01:54
Excerpts from Leonardo Bras's message of February 5, 2021 5:01 pm:
Hey Nick, thanks for reviewing :)
On Fri, 2021-02-05 at 16:28 +1000, Nicholas Piggin wrote:
quoted
Excerpts from Leonardo Bras's message of February 5, 2021 4:06 pm:
quoted
Before guest entry, TBU40 register is changed to reflect guest timebase.
After exitting guest, the register is reverted to it's original value.
If one tries to get the timestamp from host between those changes, it
will present an incorrect value.
An example would be trying to add a tracepoint in
kvmppc_guest_entry_inject_int(), which depending on last tracepoint
acquired could actually cause the host to crash.
Save the Timebase Offset to PACA and use it on sched_clock() to always
get the correct timestamp.
Ouch. Not sure how reasonable it is to half switch into guest registers
and expect to call into the wider kernel, fixing things up as we go.
What if mftb is used in other places?
IIUC, the CPU is not supposed to call anything as host between guest
entry and guest exit, except guest-related cases, like
When I say "call", I'm including tracing in that. If a function is not
marked as no trace, then it will call into the tracing subsystem.
kvmppc_guest_entry_inject_int(), but anyway, if something calls mftb it
will still get the same value as before.
Right, so it'll be out of whack again.
This is only supposed to change stuff that depends on sched_clock, like
Tracepoints, that can happen in those exceptions.
If they depend on sched_clock that's one thing. Do they definitely have
no dependencies on mftb from other calls?
quoted
Especially as it doesn't seem like there is a reason that function _has_
to be called after the timebase is switched to guest, that's just how
the code is structured.
Correct, but if called, like in rb routines, used by tracepoints, the
difference between last tb and current (lower) tb may cause the CPU to
trap PROGRAM exception, crashing host.
Yes, so I agree with Michael any function that is involved when we begin
to switch into guest context (or have not completed switching back to
host going the other way) should be marked as no trace (noinstr even,
perhaps).
quoted
As a local hack to work out a bug okay. If you really need it upstream
could you put it under a debug config option?
You mean something that is automatically selected whenever those
configs are enabled?
CONFIG_TRACEPOINT && CONFIG_KVM_BOOK3S_HANDLER && CONFIG_PPC_BOOK3S_64
Or something the user need to select himself in menuconfig?
Yeah I meant a default n thing under powerpc kernel debugging somewhere.
Thanks,
Nick
From: Leonardo Bras <hidden> Date: 2021-02-08 18:40:45
Hello Nick,
On Sat, 2021-02-06 at 13:03 +1000, Nicholas Piggin wrote:
Excerpts from Leonardo Bras's message of February 5, 2021 5:01 pm:
quoted
Hey Nick, thanks for reviewing :)
On Fri, 2021-02-05 at 16:28 +1000, Nicholas Piggin wrote:
quoted
Excerpts from Leonardo Bras's message of February 5, 2021 4:06 pm:
quoted
Before guest entry, TBU40 register is changed to reflect guest timebase.
After exitting guest, the register is reverted to it's original value.
If one tries to get the timestamp from host between those changes, it
will present an incorrect value.
An example would be trying to add a tracepoint in
kvmppc_guest_entry_inject_int(), which depending on last tracepoint
acquired could actually cause the host to crash.
Save the Timebase Offset to PACA and use it on sched_clock() to always
get the correct timestamp.
Ouch. Not sure how reasonable it is to half switch into guest registers
and expect to call into the wider kernel, fixing things up as we go.
What if mftb is used in other places?
IIUC, the CPU is not supposed to call anything as host between guest
entry and guest exit, except guest-related cases, like
When I say "call", I'm including tracing in that. If a function is not
marked as no trace, then it will call into the tracing subsystem.
quoted
kvmppc_guest_entry_inject_int(), but anyway, if something calls mftb it
will still get the same value as before.
Right, so it'll be out of whack again.
quoted
This is only supposed to change stuff that depends on sched_clock, like
Tracepoints, that can happen in those exceptions.
If they depend on sched_clock that's one thing. Do they definitely have
no dependencies on mftb from other calls?
We could change that on get_tb() or mftb() @ timebase.h, which would
have a broader reach, but would not reach any mftb from asm code.
quoted
quoted
Especially as it doesn't seem like there is a reason that function _has_
to be called after the timebase is switched to guest, that's just how
the code is structured.
Correct, but if called, like in rb routines, used by tracepoints, the
difference between last tb and current (lower) tb may cause the CPU to
trap PROGRAM exception, crashing host.
Yes, so I agree with Michael any function that is involved when we begin
to switch into guest context (or have not completed switching back to
host going the other way) should be marked as no trace (noinstr even,
perhaps).
Sure, that would avoid having to get paca->tb_offset for every mftb()
called, and avoid inconsistencies when different ways to get time are
used in code.
On the other hand, it would make it very hard to debug functions like
kvmppc_guest_entry_inject_int() as I am doing right now.
quoted
quoted
As a local hack to work out a bug okay. If you really need it upstream
could you put it under a debug config option?
You mean something that is automatically selected whenever those
configs are enabled?
CONFIG_TRACEPOINT && CONFIG_KVM_BOOK3S_HANDLER && CONFIG_PPC_BOOK3S_64
Or something the user need to select himself in menuconfig?
Yeah I meant a default n thing under powerpc kernel debugging somewhere.
So, IIUC all we can do is split this in 2 changes:
1 - Adding notrace to those functions
2 - Introducing a kernel debug config that reverts (1) and 'fixes' mftb
If that's correct, I have some ideas we can use.
For debug option, should we add the offset on get_tb() or mftb()?
Another option would be to adding this tb_offset only in the routines
used by tracing. But this could probably mean having to add a function
in arch-generic code, but still an option.
What do you think?