From: Simon Guo <redacted>
These 2 fields track whether user process has used Altivec/VSX
registers or not. They are used by kernel to setup signal frame
on user stack correctly regarding vector part.
CRIU(Checkpoint and Restore In User space) builds signal frame
for restored process. It will need this export information to
setup signal frame correctly. And CRIU will need to restore these
2 fields for the restored process.
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <redacted>
Cc: Kees Cook <redacted>
Cc: Rashmica Gupta <redacted>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-kernel@vger.kernel.org
Cc: Laurent Dufour <redacted>
Signed-off-by: Simon Guo <redacted>
Reviewed-by: Laurent Dufour <redacted>
--
v3 -> v4:
- Copying 64 bit data from/to user space using get_user/put_user
is not supported on all architectures, and may result in the
following build error on 32 bits build:
arch/powerpc/kernel/built-in.o: In function `arch_ptrace':
>> (.text+0xad4): undefined reference to `__get_user_bad'
using copy_from_user/copy_to_user instead.
v2 -> v3:
- enlarge reg_usage from 32 to 64 bits
- prefix ptrace API with PPC_
v1 -> v2:
- minor change for coding style
---
arch/powerpc/include/uapi/asm/ptrace.h | 11 ++++++++
arch/powerpc/kernel/ptrace.c | 48 ++++++++++++++++++++++++++++++++++
arch/powerpc/kernel/ptrace32.c | 2 ++
3 files changed, 61 insertions(+)
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2016-07-07 11:15:36
On Thu, 2016-07-07 at 07:49:36 UTC, Simon Guo wrote:
From: Simon Guo <redacted>
These 2 fields track whether user process has used Altivec/VSX
registers or not. They are used by kernel to setup signal frame
on user stack correctly regarding vector part.
I still dislike this. It's just exporting internal kernel state, which I know is
the point.
And I'd still like to know why we're the only arch that needs to do this.
I'm not saying I won't merge it, but I'd like to understand it better first.
CRIU(Checkpoint and Restore In User space) builds signal frame
for restored process. It will need this export information to
setup signal frame correctly. And CRIU will need to restore these
2 fields for the restored process.
I don't really know how CRIU works, but ..
Does the kernel write a sigframe for the process that's being checkpointed? If
so can't you infer the state of the bits based on what was written?
Alternately, when restoring, can you setup the sigframe with the Altivec/VSX
fields populated, and the kernel will then load them, regardless of whether
they were actually used or not prior to the checkpoint?
cheers
On Thu, 2016-07-07 at 07:49:36 UTC, Simon Guo wrote:
quoted
From: Simon Guo <redacted>
These 2 fields track whether user process has used Altivec/VSX
registers or not. They are used by kernel to setup signal frame
on user stack correctly regarding vector part.
I still dislike this. It's just exporting internal kernel state, which I know is
the point.
And I'd still like to know why we're the only arch that needs to do this.
I'm not saying I won't merge it, but I'd like to understand it better first.
quoted
CRIU(Checkpoint and Restore In User space) builds signal frame
for restored process. It will need this export information to
setup signal frame correctly. And CRIU will need to restore these
2 fields for the restored process.
I don't really know how CRIU works, but ..
Does the kernel write a sigframe for the process that's being checkpointed? If
so can't you infer the state of the bits based on what was written?
Hi Michael,
Basically, CRIU checkpoints the process register's state through the
ptrace API, and it restores it through a signal frame at restart time.
This is quite odd but that the way it works on all the CRIU's supported
architectures.
Obviously everything is done from/in user space, so the sigframe
building too.
Since we can't know from user space if the thread has used or not the
Altivec/VSX registers, since we can't rely on the MSR bits, we always
dump these registers.
Alternately, when restoring, can you setup the sigframe with the Altivec/VSX
fields populated, and the kernel will then load them, regardless of whether
they were actually used or not prior to the checkpoint?
In the case of Altivec/VSX fields, we currently force the kernel to
retrieve them from the signal frame by setting MSR_VEC/MSR_VSX so
restore_sigcontext() will copy them to the kernel thread's state.
However this doesn't touch to used_vsr and used_vr which may remain at 0.
Most of the time this is fine, but in the case a thread which has really
used those registers is catching a signal just after the restore and
before it has touched to these registers again (and so set used_vsr/vr),
these registers will not be pushed in the newly built signal frame since
setup_sigcontext() check for used_vsr/vr before pushing the registers on
the stack.
This may be an issue in the case the thread wants to changed those
registers (don't ask me why :)) in the stacked signal frame from the
signal handler since they will not be there...
Being able to get and set the used_vr and used_vsr thread's variables,
fixes this issue.
Cheers,
Laurent.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2016-07-07 13:21:56
On Thu, 2016-07-07 at 15:12 +0200, Laurent Dufour wrote:
Basically, CRIU checkpoints the process register's state through the
ptrace API, and it restores it through a signal frame at restart time.
This is quite odd but that the way it works on all the CRIU's supported
architectures.
Obviously everything is done from/in user space, so the sigframe
building too.
Since we can't know from user space if the thread has used or not the
Altivec/VSX registers, since we can't rely on the MSR bits, we always
dump these registers.
Right, however is that an issue ? These days with glibc using V{M,S}X
for things like memcpy I would think there is little to gain in trying
to avoid dumping them.
quoted
Alternately, when restoring, can you setup the sigframe with the Altivec/VSX
fields populated, and the kernel will then load them, regardless of whether
they were actually used or not prior to the checkpoint?
In the case of Altivec/VSX fields, we currently force the kernel to
retrieve them from the signal frame by setting MSR_VEC/MSR_VSX so
restore_sigcontext() will copy them to the kernel thread's state.
Yup, that's the way to go.
However this doesn't touch to used_vsr and used_vr which may remain at 0.
That would be a kernel bug.
Most of the time this is fine, but in the case a thread which has really
used those registers is catching a signal just after the restore and
before it has touched to these registers again (and so set used_vsr/vr),
these registers will not be pushed in the newly built signal frame since
setup_sigcontext() check for used_vsr/vr before pushing the registers on
the stack.
This may be an issue in the case the thread wants to changed those
registers (don't ask me why :)) in the stacked signal frame from the
signal handler since they will not be there...
Being able to get and set the used_vr and used_vsr thread's variables,
fixes this issue.
I think the right fix is that if a restore_sigcontext() has the MSR bits set,
it should set the corresponding used_* flag.
Or is there a reason why that won't work ?
Cheers,
Ben.
@@ -363,9 +363,11 @@ static long restore_sigcontext(struct pt_regs *regs, sigset_t *set, int sig,if(v_regs&&!access_ok(VERIFY_READ,v_regs,34*sizeof(vector128)))return-EFAULT;/* Copy 33 vec registers (vr0..31 and vscr) from the stack */-if(v_regs!=NULL&&(msr&MSR_VEC)!=0)+if(v_regs!=NULL&&(msr&MSR_VEC)!=0){err|=__copy_from_user(¤t->thread.vr_state,v_regs,33*sizeof(vector128));+current->thread.used_vr=true;+}elseif(current->thread.used_vr)memset(¤t->thread.vr_state,0,33*sizeof(vector128));/* Always get VRSAVE back */
@@ -385,9 +387,10 @@ static long restore_sigcontext(struct pt_regs *regs, sigset_t *set, int sig,*bufferforformatting,thenintothetaskstruct.*/v_regs+=ELF_NVRREG;-if((msr&MSR_VSX)!=0)+if((msr&MSR_VSX)!=0){err|=copy_vsx_from_user(current,v_regs);-else+current->thread.used_vsr=true;+}elsefor(i=0;i<32;i++)current->thread.fp_state.fpr[i][TS_VSRLOWOFFSET]=0;#endif
@@ -482,6 +485,7 @@ static long restore_tm_sigcontexts(struct pt_regs *regs,33*sizeof(vector128));err|=__copy_from_user(¤t->thread.transact_vr,tm_v_regs,33*sizeof(vector128));+current->thread.used_vr=true;}elseif(current->thread.used_vr){memset(¤t->thread.vr_state,0,33*sizeof(vector128));
@@ -515,6 +519,7 @@ static long restore_tm_sigcontexts(struct pt_regs *regs,tm_v_regs+=ELF_NVRREG;err|=copy_vsx_from_user(current,v_regs);err|=copy_transact_vsx_from_user(current,tm_v_regs);+current->thread.used_vsr=true;}else{for(i=0;i<32;i++){current->thread.fp_state.fpr[i][TS_VSRLOWOFFSET]=0;
On 07/07/2016 15:21, Benjamin Herrenschmidt wrote:
On Thu, 2016-07-07 at 15:12 +0200, Laurent Dufour wrote:
quoted
Basically, CRIU checkpoints the process register's state through the
ptrace API, and it restores it through a signal frame at restart time.
This is quite odd but that the way it works on all the CRIU's supported
architectures.
Obviously everything is done from/in user space, so the sigframe
building too.
Since we can't know from user space if the thread has used or not the
Altivec/VSX registers, since we can't rely on the MSR bits, we always
dump these registers.
Right, however is that an issue ? These days with glibc using V{M,S}X
for things like memcpy I would think there is little to gain in trying
to avoid dumping them.
quoted
quoted
Alternately, when restoring, can you setup the sigframe with the Altivec/VSX
fields populated, and the kernel will then load them, regardless of whether
they were actually used or not prior to the checkpoint?
In the case of Altivec/VSX fields, we currently force the kernel to
retrieve them from the signal frame by setting MSR_VEC/MSR_VSX so
restore_sigcontext() will copy them to the kernel thread's state.
Yup, that's the way to go.
quoted
However this doesn't touch to used_vsr and used_vr which may remain at 0.
That would be a kernel bug.
quoted
Most of the time this is fine, but in the case a thread which has really
used those registers is catching a signal just after the restore and
before it has touched to these registers again (and so set used_vsr/vr),
these registers will not be pushed in the newly built signal frame since
setup_sigcontext() check for used_vsr/vr before pushing the registers on
the stack.
This may be an issue in the case the thread wants to changed those
registers (don't ask me why :)) in the stacked signal frame from the
signal handler since they will not be there...
Being able to get and set the used_vr and used_vsr thread's variables,
fixes this issue.
I think the right fix is that if a restore_sigcontext() has the MSR bits set,
it should set the corresponding used_* flag.
Or is there a reason why that won't work ?
I got your point and I agree that most of the time now, the Altivec/VSX
registers are used by libc. In that case is there still a need for the
lazy Altivec/VSX registers dump in the signal frame ?
I'm fine with your proposal, except that every restarted process will
have the used_vr/used_vsx turned on after the restart since we can't
check if these registers were used or not at checkpoint time.
But that may be a minor point...
Cheers,
Laurent.
From: Simon Guo <hidden> Date: 2016-07-08 05:39:13
On Thu, Jul 07, 2016 at 11:21:18PM +1000, Benjamin Herrenschmidt wrote:
I think the right fix is that if a restore_sigcontext() has the MSR bits set,
it should set the corresponding used_* flag.
Or is there a reason why that won't work ?
That sounds reaonable to me.
I will prepare a patch based on that.
Michael, Ben, Laurent,
Thanks the discussion and proposal.
- Simon
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2016-07-08 08:26:50
Laurent Dufour [off-list ref] writes:
On 07/07/2016 15:21, Benjamin Herrenschmidt wrote:
quoted
On Thu, 2016-07-07 at 15:12 +0200, Laurent Dufour wrote:
quoted
Most of the time this is fine, but in the case a thread which has really
used those registers is catching a signal just after the restore and
before it has touched to these registers again (and so set used_vsr/vr),
these registers will not be pushed in the newly built signal frame since
setup_sigcontext() check for used_vsr/vr before pushing the registers on
the stack.
This may be an issue in the case the thread wants to changed those
registers (don't ask me why :)) in the stacked signal frame from the
signal handler since they will not be there...
Being able to get and set the used_vr and used_vsr thread's variables,
fixes this issue.
I think the right fix is that if a restore_sigcontext() has the MSR bits set,
it should set the corresponding used_* flag.
Or is there a reason why that won't work ?
I got your point and I agree that most of the time now, the Altivec/VSX
registers are used by libc. In that case is there still a need for the
lazy Altivec/VSX registers dump in the signal frame ?
Probably not for new programs. But it could conceivably break old
software.
I'm fine with your proposal, except that every restarted process will
have the used_vr/used_vsx turned on after the restart since we can't
check if these registers were used or not at checkpoint time.
But that may be a minor point...
Yeah I'd argue that's not worth worrying about, at least for now.
If it *is* a problem then we can fix it later.
cheers
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2016-07-08 10:02:43
Benjamin Herrenschmidt [off-list ref] writes:
On Thu, 2016-07-07 at 23:21 +1000, Benjamin Herrenschmidt wrote:
quoted
=C2=A0
I think the right fix is that if a restore_sigcontext() has the MSR
bits set,
it should set the corresponding used_* flag.
Something like this:
(totally untested)
Simon/Laurent, can you guys test this and let me know if it works for
your usecase.
Cyril, can you give this a review, you've been touching this code the
most lately.
cheers