amd64: Only initialize use_xsave once.

[stevek]

Summary

The "use_xsave" variable is a global and that is only supposed to be
initialized early before scheduling gets started. However, with the way
the ifuncs for "fpusave" and "fpurestore" are implemented, the value could
be changed at runtime when scheduling is active if "use_xsave" was set to
0 by the tunable. This leaves a window of opportunity where "use_xsave"
gets re-initialized to 1 and a context switch could occur with a thread
that was not set up to be able to use xsave functionality. This can lead
to an "privileged instruction fault".

The fix is to protect "use_xsave" from being initialized more than once.

Obtained from: Juniper Networks, Inc.

[kib]

I do not like adding static for that. Could you do this instead?

diff --git a/sys/amd64/amd64/fpu.c b/sys/amd64/amd64/fpu.c
index 64974a7210a9..883e25745c85 100644
--- a/sys/amd64/amd64/fpu.c
+++ b/sys/amd64/amd64/fpu.c
@@ -241,13 +241,16 @@ fpurestore_fxrstor(void *addr)
 static void
 init_xsave(void)
 {
+	int use_xsave_local;
 
 	if (use_xsave)
 		return;
 	if ((cpu_feature2 & CPUID2_XSAVE) == 0)
 		return;
-	use_xsave = 1;
-	TUNABLE_INT_FETCH("hw.use_xsave", &use_xsave);
+	use_xsave_local = 1;
+	TUNABLE_INT_FETCH("hw.use_xsave", &use_xsave_local);
+	if (use_xsave != use_xsave_local)
+		use_xsave = use_xsave_local;
 }
 
 DEFINE_IFUNC(, void, fpusave, (void *))

[stevek]

In D39637#902418, @kib wrote:

I do not like adding static for that. Could you do this instead?

Then you spend take the cost of re-evaluating the tunable each time that a ifunc needs to be evaluated.
The tunable is supposed to be a one-and-done thing.

[kib]

In D39637#902419, @stevek wrote:

In D39637#902418, @kib wrote:

I do not like adding static for that. Could you do this instead?

Then you spend take the cost of re-evaluating the tunable each time that a ifunc needs to be evaluated.
The tunable is supposed to be a one-and-done thing.

Ifuncs were supposed to be resolved only once, same as for userspace. Second (late) resolution is a sort of hack.
That said, I do not see a need to waste kernel memory to remember the status there. There is no problem with tunable changing or disappearing, because all kernel ifunc resolvers are called long before userspace is started.

[stevek]

In D39637#902426, @kib wrote:

In D39637#902419, @stevek wrote:

In D39637#902418, @kib wrote:

I do not like adding static for that. Could you do this instead?

Then you spend take the cost of re-evaluating the tunable each time that a ifunc needs to be evaluated.
The tunable is supposed to be a one-and-done thing.

Ifuncs were supposed to be resolved only once, same as for userspace. Second (late) resolution is a sort of hack.
That said, I do not see a need to waste kernel memory to remember the status there. There is no problem with tunable changing or disappearing, because all kernel ifunc resolvers are called long before userspace is started.

That's not entirely true. When a symbol needs to be resolved the ifuncs get called again. The symbol resolution at boot time does not cache the results of the ifunc to be used in subsequent resolutions.

[stevek]

In D39637#902426, @kib wrote:

Ifuncs were supposed to be resolved only once, same as for userspace. Second (late) resolution is a sort of hack.
That said, I do not see a need to waste kernel memory to remember the status there. There is no problem with tunable changing or disappearing, because all kernel ifunc resolvers are called long before userspace is started.

See link_elf_each_function_nameval() in sys/kern/link_elf.c
That will get called by dtrace's FBT functionality via linker_file_function_listall()'s use of LINKER_EACH_FUNCTION_NAMEVAL()

[kib]

In D39637#902490, @stevek wrote:

In D39637#902426, @kib wrote:

Ifuncs were supposed to be resolved only once, same as for userspace. Second (late) resolution is a sort of hack.
That said, I do not see a need to waste kernel memory to remember the status there. There is no problem with tunable changing or disappearing, because all kernel ifunc resolvers are called long before userspace is started.

See link_elf_each_function_nameval() in sys/kern/link_elf.c
That will get called by dtrace's FBT functionality via linker_file_function_listall()'s use of LINKER_EACH_FUNCTION_NAMEVAL()

This should not happen for kernel-only symbols, not used by modules.
But anyway, my main point is that it is redundant to track whether use_xsave is initialized. I posted D39660 that should still do this without adding a state, and without re-evaluating the tunable.

[stevek]

D39660 takes care of this in a better way.