Re: [PATCH v2] x86: fpu: Keep xfd_state always in sync with MSR_IA32_XFD

["Chang S. Bae" <chang.seok.bae@intel.com>]

On 5/12/2023 4:38 AM, Adamos Ttofari wrote:
> Commit 672365477ae8 ("x86/fpu: Update XFD state where required") and
> commit 8bf26758ca96 ("x86/fpu: Add XFD state to fpstate") introduced a
> per CPU variable xfd_state to keep the MSR_IA32_XFD value cached. In
> order to avoid unnecessary writes to the MSR.
> 
> On CPU hotplug MSR_IA32_XFD is reset to the init_fpstate.xfd, which
> wipes out any stale state. But the per CPU cached xfd value is not
> reset, which brings them out of sync.
> 
> As a consequence a subsequent xfd_update_state() might fail to update
> the MSR which in turn can result in XRSTOR raising a #NM in kernel
> space, which crashes the kernel.

I have drafted this to reproduce and test the issue in the amx selftest:

diff --git a/tools/testing/selftests/x86/amx.c 
b/tools/testing/selftests/x86/amx.c
index d884fd69dd51..c773de1f3864 100644
--- a/tools/testing/selftests/x86/amx.c
+++ b/tools/testing/selftests/x86/amx.c
@@ -767,15 +767,15 @@ static int create_threads(int num, struct 
futex_info *finfo)
         return 0;
  }

-static void affinitize_cpu0(void)
+static inline void affinitize_cpu(int cpu)
  {
         cpu_set_t cpuset;

         CPU_ZERO(&cpuset);
-       CPU_SET(0, &cpuset);
+       CPU_SET(cpu, &cpuset);

         if (sched_setaffinity(0, sizeof(cpuset), &cpuset) != 0)
-               fatal_error("sched_setaffinity to CPU 0");
+               fatal_error("sched_setaffinity to CPU %d", cpu);
  }

  static void test_context_switch(void)
@@ -784,7 +784,7 @@ static void test_context_switch(void)
         int i;

         /* Affinitize to one CPU to force context switches */
-       affinitize_cpu0();
+       affinitize_cpu(0);

         req_xtiledata_perm();

@@ -926,6 +926,120 @@ static void test_ptrace(void)
                 err(1, "ptrace test");
  }

+/* CPU Hotplug test */
+
+#define STRING_BUF_LEN 1024
+
+void __hotplug_cpu(int online, int cpu)
+{
+       char buf[STRING_BUF_LEN] = {};
+       int ret = 0;
+
+       strncat(buf, "echo ", STRING_BUF_LEN - strlen(buf));
+       snprintf(buf + strlen(buf), STRING_BUF_LEN - strlen(buf), "%d", 
online);
+       strncat(buf, " > /sys/devices/system/cpu/cpu", STRING_BUF_LEN - 
strlen(buf));
+       snprintf(buf + strlen(buf), STRING_BUF_LEN - strlen(buf), "%d", 
cpu);
+       strncat(buf, "/online", STRING_BUF_LEN - strlen(buf));
+
+       ret = system(buf);
+       if (ret)
+               err(1, "%s\n", buf);
+}
+
+void offline_cpu(int cpu)
+{
+       __hotplug_cpu(0, cpu);
+}
+
+void online_cpu(int cpu)
+{
+       __hotplug_cpu(1, cpu);
+}
+
+static jmp_buf jmpbuf;
+
+static void handle_sigsegv(int sig, siginfo_t *si, void *ctx_void)
+{
+       siglongjmp(jmpbuf, 1);
+}
+
+#define RETRY 5
+
+/*
+ * Sanity checks the hotplug CPU for its (re-)initialization.
+ *
+ * An AMX thread is created on a CPU while the other one went offline.
+ * Then, plug the offline CPU, and migrate the thread. Repeat this
+ * on/off switches multiple times to ensure no inconsistent failure.
+ * If something goes wrong, the thread gets a signal or is just
+ * killed.
+ */
+void *switch_cpus(void *arg)
+{
+       unsigned int altstack_size = getauxval(AT_MINSIGSTKSZ) + SIGSTKSZ;
+       int *result = (int *)arg;
+       void *altstack;
+       int i = 0;
+
+       altstack = alloc_altstack(altstack_size);
+       setup_altstack(altstack, altstack_size, SUCCESS_EXPECTED);
+
+       affinitize_cpu(0);
+       offline_cpu(1);
+       load_rand_tiledata(stashed_xsave);
+
+       sethandler(SIGSEGV, handle_sigsegv, SA_ONSTACK);
+       for (i = 0;i < RETRY;i++) {
+               if (i > 0) {
+                       affinitize_cpu(0);
+                       offline_cpu(1);
+               }
+               if (sigsetjmp(jmpbuf, 1) == 0) {
+                       online_cpu(1);
+                       affinitize_cpu(1);
+               } else {
+                       *result = 1;
+                       goto out;
+               }
+       }
+       *result = 0;
+out:
+       clearhandler(SIGSEGV);
+       return result;
+}
+
+void test_cpuhp(void)
+{
+       int max_cpu_num = sysconf(_SC_NPROCESSORS_ONLN) - 1;
+       void *thread_retval;
+       pthread_t thread;
+       int result, rc;
+
+       if (!max_cpu_num) {
+               printf("[SKIP]\tThe running system do not have any spare 
CPU for the hotplug\n");
+               return;
+       }
+
+       printf("[RUN]\tTest AMX state use with CPU hotplug\n");
+
+       if (pthread_create(&thread, NULL, switch_cpus, &result))
+               fatal_error("pthread_creat()\n");
+
+       rc = pthread_join(thread, &thread_retval);
+
+       if (rc)
+               fatal_error("pthread_join()\n");
+
+       /*
+        * Either an invalid retval or a failed result indicates
+        * the test failure.
+        */
+       if (thread_retval != &result || result != 0)
+               printf("[FAIL]\tThe AMX thread had an issue with the CPU 
hotplug.\n");
+       else
+               printf("[OK]\tThe AMX thread has no issue with the CPU 
hotplug.\n");
+}
+
  int main(void)
  {
         /* Check hardware availability at first */
@@ -948,6 +1062,8 @@ int main(void)

         test_ptrace();

+       test_cpuhp();
+
         clearhandler(SIGILL);
         free_stashed_xsave();

Thanks,
Chang