Messages in this thread Patch in this message |  | | | Date | Fri, 5 Feb 2010 13:44:07 +0100 | | From | Oleg Nesterov <> | | Subject | Re: PATCH? process_32.c:__switch_to() calls __math_state_restore() before updating current_task |
| |
On 02/04, Suresh Siddha wrote: > > On Thu, 2010-02-04 at 08:51 -0800, Oleg Nesterov wrote: > > > > --- a/arch/x86/kernel/process_32.c > > +++ b/arch/x86/kernel/process_32.c > > @@ -377,9 +377,6 @@ __switch_to(struct task_struct *prev_p, > > */ > > arch_end_context_switch(next_p); > > > > - if (preload_fpu) > > - __math_state_restore(); > > - > > /* > > * Restore %gs if needed (which is common) > > */ > > @@ -388,6 +385,9 @@ __switch_to(struct task_struct *prev_p, > > > > percpu_write(current_task, next_p); > > > > + if (preload_fpu) > > + __math_state_restore(); > > + > > return prev_p; > > } > > Oleg, __math_state_restore() uses current_thread_info() which at that > point already has the right esp and as such uses the correct thread > struct etc.
Ah, indeed. I didn't notice that, unlike X86_64, X86_32 uses esp, not kernel_stack to get thread_info, and __math_state_restore() pathes never use get_current().
> After saying that, in the past I have also ran into this question and > got satisfied by looking deeper. Best is to make the 32bit and 64bit > code similar as much as possible and as such your patch is acceptable. > > Can you please re-post with a proper changelog (and ofcourse testing > etc)? You can add my Ack to that.
OK, will do once I have a 32bit machine to test.
Thanks a lot for your explanation!
Could you please explain me another issue? I know nothing about fpu handling, I am reading this code trying to understand the unrelated bug with the old kernel.
In short: why restore_i387_xstate() does init_fpu() when !used_math(), can't (or shouldn't) it merely do set_used_math() ?
restore_i387_xstate:
if (!used_math()) { err = init_fpu(tsk); if (err) return err; } note that buf != NULL. This means that used_math() was true when get_sigframe() was called, otherwise buf == sigcontext->fpstate would be NULL, right?
So, the task must have the valid ->thread.xstate, and init_fpu() just resets the contents of *thread.xstate. Why? We are going to reload the FPU registers and set TS_USEDFPU. This means .xstate must be never used until we save the FPU registers back, correct?
IOW, I'd appreciate very much if you can explain me why the patch below is wrong. To clarify, even if the patch is correct I do not mean it is really needed, I am just trying to understand what I have missed here.
Thanks,
Oleg.
--- a/arch/x86/kernel/xsave.c +++ b/arch/x86/kernel/xsave.c @@ -215,11 +215,7 @@ int restore_i387_xstate(void __user *buf if (!access_ok(VERIFY_READ, buf, sig_xstate_size)) return -EACCES; - if (!used_math()) { - err = init_fpu(tsk); - if (err) - return err; - } + set_stopped_child_used_math(tsk); if (!(task_thread_info(current)->status & TS_USEDFPU)) { clts();
|  |