Re: [Xen-devel] [PATCH] x86/fpu: CR0.TS should be set before trap into PV guest's #NM exception handler

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.11.13 at 10:15, Zhu Yanhai <zhu.yanhai@xxxxxxxxx> wrote:
> 2013/11/6 Jan Beulich <JBeulich@xxxxxxxx>:
>> May I direct your attention to the XenoLinux one:
>>
>> asmlinkage void math_state_restore(void)
>> {
>>         struct task_struct *me = current;
>>
>>         /* NB. 'clts' is done for us by Xen during virtual trap. */
>>         __get_cpu_var(xen_x86_cr0) &= ~X86_CR0_TS;
>>         if (!used_math())
>>                 init_fpu(me);
>>         restore_fpu_checking(&me->thread.i387.fxsave);
>>         task_thread_info(me)->status |= TS_USEDFPU;
>> }
>>
>> Note the comment close to the beginning - the fact that CR0.TS
>> is clear at exception handler entry is actually part of the PV ABI,
>> i.e. by altering hypervisor behavior here you break all forward
>> ported kernels.
> 
> I see, XenoLinux kernel doesn't sleep in init_fpu() so it doesn't have
> this issue. But I wonder why PV ABI decide to clear this bit for the
> guest kernel, isn't it better for the guest kernel itself to see bit
> set? Since it's more similar with the hardware. I know the ABI cannot
> be changed, just for curious.

Quite obvious - performance. Since (as you also confirmed) it is
(almost) guaranteed for the handler to want to clear the bit, we
can save it from having to do another hypercall here. In the
forward ported XenoLinux the change is quite trivial (leaving
aside any optimization, as we're on a rarely used path here
anyway - it's being taken only the first time a process accesses
the FPU): stts() before the local_irq_enable(), and clts() after
the local_irq_disable(). But the x86 maintainers probably won't
like this for pvops...

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel