Re: CVS commit: src/sys/arch

[Jaromír Doleček <jaromir.dolecek%gmail.com@localhost>]

The function is ever called only with x86_fpu_save >= FPU_SAVE_FXSAVE,
so there should be no change in functionality AFAICS.

2018-06-20 10:34 GMT+02:00 Maxime Villard <max%m00nbsd.net@localhost>:
> Le 19/06/2018 à 21:50, Jaromir Dolecek a écrit :
>>
>> Module Name:    src
>> Committed By:   jdolecek
>> Date:           Tue Jun 19 19:50:19 UTC 2018
>>
>> Modified Files:
>>         src/sys/arch/x86/include: fpu.h
>>         src/sys/arch/x86/x86: cpu.c fpu.c identcpu.c
>>         src/sys/arch/xen/x86: cpu.c
>>
>> Log Message:
>> fix FPU initialization on Xen to allow e.g. AVX when supported by
>> hardware;
>> only use XSAVE when the the CPUID OSXSAVE bit is set, as this seems to be
>> reliable indication
>>
>> tested with Xen 4.2.6 DOM0/DOMU on Intel CPU, without and with no-xsave
>> flag,
>> so should work also on those AMD CPUs, which have XSAVE disabled by
>> default;
>> also tested with Xen DOM0 4.8.3
>>
>> fixes PR kern/50332 by Torbjorn Granlund; sorry it took three years to
>> address
>>
>> XXX pullup netbsd-8
>
>
> I don't understand, and it looks wrong.
>
>  -- fxsave
>
> fpuinit_mxcsr_mask calls fxsave, not xsave. The availability of fxsave is
> controlled by CR4_OSFXSR, not CR4_OSXSAVE. It seems to me that the check you
> added in fpuinit_mxcsr_mask is wrong, because it is confusing fxsave and
> xsave.
>
> [x86_fpu_save = FPU_SAVE_FXSAVE] guarantees fxsave is supported.
>
> fpuinit_mxcsr_mask is called only when this condition is true, so there is
> no
> need for a check in the first place.
>
> The reason I disabled the code on Xen, if I remember correctly, was because
> of a stack alignment problem. For some reason this variable
>
>         union savefpu fpusave __aligned(16);
>
> was not always 16-byte aligned on Xen. So fxsave would fault.
>
>  -- xsave
>
> Please keep the initialization logic consistent.
>
> xen/x86/cpu.c
> 554     if (cpu_feature[1] & CPUID2_OSXSAVE)
> 555             cr4 |= CR4_OSXSAVE;
> 556     else {
> 557             x86_xsave_features = 0;
> 558             x86_fpu_save = FPU_SAVE_FXSAVE;
> 559     }
>
> If the availability of xsave on xen is determined by CPUID2_OSXSAVE (as
> opposed to CPUID2_XSAVE on native), then put the code in identcpu.c just
> like native.
>
> That is to say
>
> x86/x86/identcpu.c
> 804     /* See if xsave (for AVX) is supported */
> 805     if ((ci->ci_feat_val[1] & CPUID2_XSAVE) == 0)
> 806             return;
> XXX
> XXX #ifdef XEN
> XXX     /* On Xen we also need to check CPUID2_OSXSAVE */
> XXX     if ((ci->ci_feat_val[1] & CPUID2_OSXSAVE) == 0)
> XXX             return;
> XXX #endif
>
> Note also that your change seems wrong on i386 too, because you enable
> fxsave
> if we don't have xsave, while we may have neither.