Re: CVS commit: src/sys/arch

[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.