On Sun, Dec 26, 2010 at 7:24 AM, Mark Kettenis <mark.kettenis@xs4all.nl>
wrote:
quoted text
>> Originally worked out by joshe@; this corrects the timing of the call to
>> fpuinit() for the primary CPU on amd64 so that the saved mask value is
>> initialized correctly.
>
> Hmm, I'm not too happy about moving fpuinit() out of cpu_hatch() on
> amd64, while leaving npxinit() in there for i386.

Well, on i386, npxinit() is called for the primary cpu when the npx is
attached, which is way after the secondary cpus are attached.  Can we
assume it to be safe to call npxinit() before the npx device is
attached?  It wouldn't surprise me to hear that it only works when npx
errors are reported using exceptions and not ISA interrupts...which
would explain why the early calls are fine on multi-CPU systems,
'cause they all use exceptions.

So no, I don't think it's safe to move up the npxinit() call into
cpu_init(), unless you're suggesting that we delay initializing
secondary cpus until after the isa bus is probed (barf).  The
consistent alternative would be to leave the fpuinit() call in
cpu_hatch() and call it for the primary cpu near the end of
init_x86_64(), that being the rough equivalent for the primary cpu.


quoted text
>> +     if (CPU_IS_PRIMARY(ci)) {
>> +             struct fxsave64 fx __attribute__((aligned(16)));
>> +
>> +             bzero(&fx, sizeof(fx));
>> +             fxsave(&fx);
>> +             if (fx.fx_mxcsr_mask)
>> +                     fpu_mxcsr_mask = fx.fx_mxcsr_mask;
>> +             else
>> +                     fpu_mxcsr_mask = __INITIAL_MXCSR_MASK__;
>> +     }
>
> This function will be run again upon resume.  Now overwriting
> fpu_mxcsr_mask shouldn't hurt, but perhaps replacing:
>
>        if (CPU_IS_PRIMARY(ci)) {
>
> with
>
>        if (fpu_mxcsr_mask == 0) {
>
> would be better?

No.  The primary cpu check is necessary, at least on i386, because
secondary cpus call npxinit() before the primary cpu *and* before
setting CR4_OSFXSR.

(Also, you're not planning on hot-upgrading your FPU by replacing the
CPU while it's asleep?!)


quoted text
>> Index: sys/arch/i386/i386/process_machdep.c
>> ===================================================================
>> RCS file: /cvs/src/sys/arch/i386/i386/process_machdep.c,v
>> retrieving revision 1.25
>> diff -u -p -r1.25 process_machdep.c
>> --- sys/arch/i386/i386/process_machdep.c      29 Sep 2010 15:11:31 -0000  
   1.25
quoted text
>> +++ sys/arch/i386/i386/process_machdep.c      25 Dec 2010 19:38:52 -0000
>> @@ -308,6 +309,7 @@ process_write_fpregs(struct proc *p, str
>>               /* XXX Yuck. */
>>               memcpy(&s87, regs, sizeof(*regs));
>>               process_s87_to_xmm(&s87, &frame->sv_xmm);
>> +             frame->sv_xmm.sv_env.en_mxcsr &= fpu_mxcsr_mask;
>>       } else
>>               memcpy(&frame->sv_87, regs, sizeof(*regs));
>
> Oh, this actually points out an interesting problem.  The MXCSR
> register isn't initialized from data passed to us by userland.
> However, if a user calls ptrace(PT_SETFPREGS, ...) on a process that
> didn't use the FPU yet, it isn't quite clear what we initialize the
> XMM state to.  While your approach will make sure we won't cause a
> segfault, we might still be leaking stuff from the kernel to userland.
> I think the diff below is a better way to address the issue.
>
> Index: process_machdep.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/i386/i386/process_machdep.c,v
> retrieving revision 1.25
> diff -u -p -r1.25 process_machdep.c
> --- process_machdep.c   29 Sep 2010 15:11:31 -0000      1.25
> +++ process_machdep.c   26 Dec 2010 15:23:33 -0000
> @@ -299,8 +299,15 @@ process_write_fpregs(struct proc *p, str
>  #if NNPX > 0
>                npxsave_proc(p, 0);
>  #endif
> -       } else
> +       } else {
> +               /*
> +                * Make sure MXCSR and the XMM registers are
> +                * initialized to sane defaults.
> +                */
> +               if (i386_use_fxsave)
> +                       process_fninit_xmm(&frame->sv_xmm);
>                p->p_md.md_flags |= MDP_USEDFPU;
> +       }
>
>        if (i386_use_fxsave) {
>                struct save87 s87;

Yes, I agree.  IMO, that can be committed separately (ok guenther@)
from this mxcsr_mask diff.

What this really points out, however, is that the masking added by my
diff was in the wrong function: it should have been in
process_write_xmmregs(), duh.  I'll fix that in the next rev.

Once we agree where to npxinit()/fpuinit(), I'll post an updated diff.


Philip Guenther