[PATCH v3 0/3] x86/fpu: Allow the In-Field Scan driver to initialize FPU state

[PATCH v3 0/3] x86/fpu: Allow the In-Field Scan driver to initialize FPU state

[Chang S. Bae]

This revision switches to a new approach by providing a helper to
initialize user FPU states for the driver, which is considerably simpler.
This approach could serve as an example for addressing similar situations
from a non-critical path.

I thought fpu_reset_fpregs() as the helper name. There is already one
with this name. Then, I realized the existing one is a bit misaligned, so
renamed it first here.

Thanks to Dave for the reviews and the suggestion.

Thanks,
Chang

The previous postings:
V2: https://lore.kernel.org/all/20240507235344.249103-1-chang.seok.bae@intel.com
V1: https://lore.kernel.org/all/20240430212508.105117-1-chang.seok.bae@intel.com

Chang S. Bae (3):
  x86/fpu: Rename fpu_reset_fpregs() to fpu_reset_fpstate_regs()
  x86/fpu: Allow FPU users to initialize FPU state
  platform/x86/intel/ifs: Initialize FPU states for the scan test

 arch/x86/include/asm/fpu/api.h           |  2 ++
 arch/x86/kernel/fpu/core.c               | 17 ++++++++++++++---
 drivers/platform/x86/intel/ifs/ifs.h     |  1 +
 drivers/platform/x86/intel/ifs/runtest.c |  7 +++++++
 4 files changed, 24 insertions(+), 3 deletions(-)


base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
-- 
2.34.1

[PATCH v3 1/3] x86/fpu: Rename fpu_reset_fpregs() to fpu_reset_fpstate_regs()

[Chang S. Bae]

The term 'fpregs' typically refers to FPU registers. The function copies
init values to the task's memory image, not hardware registers. Rename it
to reflect what it does.

Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
---
V2 -> V3: New patch to avoids conflict with the upcoming new wrapper.
---
 arch/x86/kernel/fpu/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 1209c7aebb21..2e6f43dfe98b 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -709,7 +709,7 @@ static inline void restore_fpregs_from_init_fpstate(u64 features_mask)
 /*
  * Reset current->fpu memory state to the init values.
  */
-static void fpu_reset_fpregs(void)
+static void fpu_reset_fpstate_regs(void)
 {
 	struct fpu *fpu = &current->thread.fpu;
 
@@ -744,7 +744,7 @@ void fpu__clear_user_states(struct fpu *fpu)
 
 	fpregs_lock();
 	if (!cpu_feature_enabled(X86_FEATURE_FPU)) {
-		fpu_reset_fpregs();
+		fpu_reset_fpstate_regs();
 		fpregs_unlock();
 		return;
 	}
@@ -774,7 +774,7 @@ void fpu__clear_user_states(struct fpu *fpu)
 void fpu_flush_thread(void)
 {
 	fpstate_reset(&current->thread.fpu);
-	fpu_reset_fpregs();
+	fpu_reset_fpstate_regs();
 }
 /*
  * Load FPU context before returning to userspace.
-- 
2.34.1

[PATCH v3 2/3] x86/fpu: Allow FPU users to initialize FPU state

[Chang S. Bae]

The In-Field Scan (IFS) test [1] is a destructive process that overwrites
the existing state to test the logic on the fly. As part of this test,
the architectural state should be saved before the test begins and then
restored upon completion.

Unfortunately, AMX state is excluded from the scope of state recovery.
This exclusion prohibits the IFS process from supporting any software
context when running the test. The kernel generally runs with live user
FPU states, including AMX.

Provide fpu_reset_fpregs() for the IFS driver to reset FPU states, which
is much simpler than specifying state components and is affordable in the
non-critical path. This, along with kernel_fpu_begin(), can allow the IFS
test to proceed.

Alternatively, system administrators may attempt to mitigate this IFS
issue by arranging some workloads not to run on CPUs selected for the
tests. But, this approach is disruptive for managing large-scaled
systems.

[1]: https://docs.kernel.org/arch/x86/ifs.html
Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
---
V2 -> V3: Switch to a simpler solution and clarify the hardware
          problem (Dave Hansen).
V1 -> V2: Revise the changelog (Dave and Ashok)

The IFS Tech Paper [2] elaborates its purpose and the requirements of the
context restoration after the scan test. Additionally, the necessity for
AMX initialization is emphasized in the Intel Software Development Manual
as of March 2024, in Section 18.2 of Vol.1.

[2]: https://www.intel.com/content/www/us/en/content-details/822279/finding-faulty-components-in-a-live-fleet-environment.html
---
 arch/x86/include/asm/fpu/api.h |  2 ++
 arch/x86/kernel/fpu/core.c     | 11 +++++++++++
 2 files changed, 13 insertions(+)

diff --git a/arch/x86/include/asm/fpu/api.h b/arch/x86/include/asm/fpu/api.h
index f86ad3335529..284304171003 100644
--- a/arch/x86/include/asm/fpu/api.h
+++ b/arch/x86/include/asm/fpu/api.h
@@ -179,4 +179,6 @@ extern long fpu_xstate_prctl(int option, unsigned long arg2);
 
 extern void fpu_idle_fpregs(void);
 
+extern void fpu_reset_fpregs(void);
+
 #endif /* _ASM_X86_FPU_API_H */
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 2e6f43dfe98b..51d7689147f4 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -915,3 +915,14 @@ noinstr void fpu_idle_fpregs(void)
 		__this_cpu_write(fpu_fpregs_owner_ctx, NULL);
 	}
 }
+
+/*
+ * Allow FPU users to initialize the entire user FPU register state. The
+ * caller must invoke kernel_fpu_begin() beforehand.
+ */
+void fpu_reset_fpregs(void)
+{
+	WARN_ON_FPU(!this_cpu_read(in_kernel_fpu));
+	restore_fpregs_from_fpstate(&init_fpstate, XFEATURE_MASK_USER_SUPPORTED);
+}
+EXPORT_SYMBOL_GPL(fpu_reset_fpregs);
-- 
2.34.1

[PATCH v3 3/3] platform/x86/intel/ifs: Initialize FPU states for the scan test

[Chang S. Bae]

The In-Field Scan process aborts if AMX state is not in initialized
state. Use fpu_reset_fpregs() to ensure AMX state is initialized before
entering the rendezvous loop.

Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Tested-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
V2 -> V3: Use fpu_reset_user_fpregs().
	  Thanks to everyone who provided tags on previous versions. Due
	  to the code change, I felt it necessary to revoke them, but the
	  high-level logic remains the same.

V1 -> V2: Revised the changelog (Ashok), added a space to the code comment
	  (Ilpo), and included the header file explicitly (0-day).
---
 drivers/platform/x86/intel/ifs/ifs.h     | 1 +
 drivers/platform/x86/intel/ifs/runtest.c | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index 56b9f3e3cf76..5e7ba94b4054 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -129,6 +129,7 @@
  */
 #include <linux/device.h>
 #include <linux/miscdevice.h>
+#include <asm/fpu/api.h>
 
 #define MSR_ARRAY_BIST				0x00000105
 #define MSR_COPY_SCAN_HASHES			0x000002c2
diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
index 282e4bfe30da..68e128ea57ed 100644
--- a/drivers/platform/x86/intel/ifs/runtest.c
+++ b/drivers/platform/x86/intel/ifs/runtest.c
@@ -191,6 +191,10 @@ static int doscan(void *data)
 	/* Only the first logical CPU on a core reports result */
 	first = cpumask_first(cpu_smt_mask(cpu));
 
+	/* Prepare FPU state before entering the rendezvous loop */
+	kernel_fpu_begin();
+	fpu_reset_fpregs();
+
 	wait_for_sibling_cpu(&scan_cpus_in, NSEC_PER_SEC);
 
 	/*
@@ -202,6 +206,9 @@ static int doscan(void *data)
 	 * are processed in a single pass) before it retires.
 	 */
 	wrmsrl(MSR_ACTIVATE_SCAN, params->activate->data);
+
+	kernel_fpu_end();
+
 	rdmsrl(MSR_SCAN_STATUS, status.data);
 
 	trace_ifs_status(ifsd->cur_batch, start, stop, status.data);
-- 
2.34.1

Re: [PATCH v3 0/3] x86/fpu: Allow the In-Field Scan driver to initialize FPU state

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1391 bytes --]

On Thu, 30 May 2024, Chang S. Bae wrote:

> This revision switches to a new approach by providing a helper to
> initialize user FPU states for the driver, which is considerably simpler.
> This approach could serve as an example for addressing similar situations
> from a non-critical path.
> 
> I thought fpu_reset_fpregs() as the helper name. There is already one
> with this name. Then, I realized the existing one is a bit misaligned, so
> renamed it first here.
> 
> Thanks to Dave for the reviews and the suggestion.
> 
> Thanks,
> Chang
> 
> The previous postings:
> V2: https://lore.kernel.org/all/20240507235344.249103-1-chang.seok.bae@intel.com
> V1: https://lore.kernel.org/all/20240430212508.105117-1-chang.seok.bae@intel.com
> 
> Chang S. Bae (3):
>   x86/fpu: Rename fpu_reset_fpregs() to fpu_reset_fpstate_regs()
>   x86/fpu: Allow FPU users to initialize FPU state
>   platform/x86/intel/ifs: Initialize FPU states for the scan test
> 
>  arch/x86/include/asm/fpu/api.h           |  2 ++
>  arch/x86/kernel/fpu/core.c               | 17 ++++++++++++++---
>  drivers/platform/x86/intel/ifs/ifs.h     |  1 +
>  drivers/platform/x86/intel/ifs/runtest.c |  7 +++++++
>  4 files changed, 24 insertions(+), 3 deletions(-)

Acked-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Do x86 maintainers plan to take these through their tree?

-- 
 i.

Re: [PATCH v3 0/3] x86/fpu: Allow the In-Field Scan driver to initialize FPU state

[Dave Hansen]

On 6/3/24 03:11, Ilpo Järvinen wrote:
> Acked-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
> Do x86 maintainers plan to take these through their tree?

Yes, once we've gone over them very carefully.