| Message ID | 20240430212508.105117-3-chang.seok.bae@intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | x86/fpu: Extend kernel_fpu_begin_mask() for the In-Field Scan driver | expand |
On Tue, Apr 30, 2024 at 02:25:08PM -0700, Bae, Chang Seok wrote: > The scan test does not start when the AMX state remains active and is not > re-initialized. With the extension of kernel_fpu_begin_mask(), the driver > code can now initialize the state properly. Avoid "re-initialized" and maybe use the same SDM language here as well? Infield Scan aborts if AMX state is not in initialized state. Use the kernel_fpu_begin_mask(KFPU_AMX) to ensure AMX state is initialized. > > Introduce custom FPU handling wrappers to ensure compliant with the > established FPU API semantics, as kernel_fpu_begin() exclusively sets > legacy states. This follows the EFI case from commit b0dc553cfc9d > ("x86/fpu: Make the EFI FPU calling convention explicit"). The EFI and commit mention is worthy of prior use, but the first line isn't reading well... Maybe Introduce custom FPU handling wrappers to ensure compliance with the established FPU API semantics. This change follows the EFI case from commit b0dc553cfc9d ("x86/fpu: Make the EFI FPU calling convention explicit"). I dropped the note about legacy states, I didn't quite know how to word that. > > Then, use these wrappers to surround the MSR_ACTIVATE_SCAN write to > minimize the critical section. To prevent unnecessary delays, invoke > ifs_fpu_begin() before entering the rendezvous loop. > > Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com> > Reviewed-by: Jithu Joseph <jithu.joseph@intel.com> > Tested-by: Jithu Joseph <jithu.joseph@intel.com>
Hi, On 4/30/24 11:25 PM, Chang S. Bae wrote: > The scan test does not start when the AMX state remains active and is not > re-initialized. With the extension of kernel_fpu_begin_mask(), the driver > code can now initialize the state properly. > > Introduce custom FPU handling wrappers to ensure compliant with the > established FPU API semantics, as kernel_fpu_begin() exclusively sets > legacy states. This follows the EFI case from commit b0dc553cfc9d > ("x86/fpu: Make the EFI FPU calling convention explicit"). > > Then, use these wrappers to surround the MSR_ACTIVATE_SCAN write to > minimize the critical section. To prevent unnecessary delays, invoke > ifs_fpu_begin() before entering the rendezvous loop. > > Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com> > Reviewed-by: Jithu Joseph <jithu.joseph@intel.com> > Tested-by: Jithu Joseph <jithu.joseph@intel.com> Thanks, the patch looks good to me. I believe this is best merged through the tip/x86 tree together with patch 1/2 : Acked-by: Hans de Goede <hdegoede@redhat.com> I have checked and this should not cause any conflicts with the ifs changes current in platform-drivers-x86/for-next. Regards, Hans > --- > drivers/platform/x86/intel/ifs/ifs.h | 14 ++++++++++++++ > drivers/platform/x86/intel/ifs/runtest.c | 6 ++++++ > 2 files changed, 20 insertions(+) > > diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h > index 56b9f3e3cf76..71d8b50854b2 100644 > --- a/drivers/platform/x86/intel/ifs/ifs.h > +++ b/drivers/platform/x86/intel/ifs/ifs.h > @@ -325,4 +325,18 @@ int do_core_test(int cpu, struct device *dev); > extern struct attribute *plat_ifs_attrs[]; > extern struct attribute *plat_ifs_array_attrs[]; > > +static inline void ifs_fpu_begin(void) > +{ > + /* > + * The AMX state must be initialized prior to executing In-Field > + * Scan tests, according to Intel SDM. > + */ > + kernel_fpu_begin_mask(KFPU_AMX); > +} > + > +static inline void ifs_fpu_end(void) > +{ > + kernel_fpu_end(); > +} > + > #endif > diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c > index 95b4b71fab53..a35eac7c0b44 100644 > --- a/drivers/platform/x86/intel/ifs/runtest.c > +++ b/drivers/platform/x86/intel/ifs/runtest.c > @@ -188,6 +188,9 @@ 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*/ > + ifs_fpu_begin(); > + > wait_for_sibling_cpu(&scan_cpus_in, NSEC_PER_SEC); > > /* > @@ -199,6 +202,9 @@ static int doscan(void *data) > * are processed in a single pass) before it retires. > */ > wrmsrl(MSR_ACTIVATE_SCAN, params->activate->data); > + > + ifs_fpu_end(); > + > rdmsrl(MSR_SCAN_STATUS, status.data); > > trace_ifs_status(ifsd->cur_batch, start, stop, status.data);
On Tue, 30 Apr 2024, Chang S. Bae wrote: > The scan test does not start when the AMX state remains active and is not > re-initialized. With the extension of kernel_fpu_begin_mask(), the driver > code can now initialize the state properly. > > Introduce custom FPU handling wrappers to ensure compliant with the > established FPU API semantics, as kernel_fpu_begin() exclusively sets > legacy states. This follows the EFI case from commit b0dc553cfc9d > ("x86/fpu: Make the EFI FPU calling convention explicit"). > > Then, use these wrappers to surround the MSR_ACTIVATE_SCAN write to > minimize the critical section. To prevent unnecessary delays, invoke > ifs_fpu_begin() before entering the rendezvous loop. > > Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com> > Reviewed-by: Jithu Joseph <jithu.joseph@intel.com> > Tested-by: Jithu Joseph <jithu.joseph@intel.com> > --- > drivers/platform/x86/intel/ifs/ifs.h | 14 ++++++++++++++ > drivers/platform/x86/intel/ifs/runtest.c | 6 ++++++ > 2 files changed, 20 insertions(+) > > diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h > index 56b9f3e3cf76..71d8b50854b2 100644 > --- a/drivers/platform/x86/intel/ifs/ifs.h > +++ b/drivers/platform/x86/intel/ifs/ifs.h > @@ -325,4 +325,18 @@ int do_core_test(int cpu, struct device *dev); > extern struct attribute *plat_ifs_attrs[]; > extern struct attribute *plat_ifs_array_attrs[]; > > +static inline void ifs_fpu_begin(void) > +{ > + /* > + * The AMX state must be initialized prior to executing In-Field > + * Scan tests, according to Intel SDM. > + */ > + kernel_fpu_begin_mask(KFPU_AMX); > +} > + > +static inline void ifs_fpu_end(void) > +{ > + kernel_fpu_end(); > +} > + > #endif > diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c > index 95b4b71fab53..a35eac7c0b44 100644 > --- a/drivers/platform/x86/intel/ifs/runtest.c > +++ b/drivers/platform/x86/intel/ifs/runtest.c > @@ -188,6 +188,9 @@ 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*/ Missing space
On 5/2/2024 4:00 AM, Ilpo Järvinen wrote: > On Tue, 30 Apr 2024, Chang S. Bae wrote: > >> + /* Prepare FPU state before entering the rendezvous loop*/ > > Missing space Yeah, right. Thanks for spotting this.
On 5/1/2024 1:58 AM, Hans de Goede wrote: > > Thanks, the patch looks good to me. > > I believe this is best merged through the tip/x86 tree together > with patch 1/2 : > > Acked-by: Hans de Goede <hdegoede@redhat.com> > > I have checked and this should not cause any conflicts with > the ifs changes current in platform-drivers-x86/for-next. Thanks!
On 4/30/24 2:25 PM, Chang S. Bae wrote: > The scan test does not start when the AMX state remains active and is not > re-initialized. With the extension of kernel_fpu_begin_mask(), the driver > code can now initialize the state properly. > > Introduce custom FPU handling wrappers to ensure compliant with the > established FPU API semantics, as kernel_fpu_begin() exclusively sets > legacy states. This follows the EFI case from commit b0dc553cfc9d > ("x86/fpu: Make the EFI FPU calling convention explicit"). > > Then, use these wrappers to surround the MSR_ACTIVATE_SCAN write to > minimize the critical section. To prevent unnecessary delays, invoke > ifs_fpu_begin() before entering the rendezvous loop. > > Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com> > Reviewed-by: Jithu Joseph <jithu.joseph@intel.com> > Tested-by: Jithu Joseph <jithu.joseph@intel.com> > --- Looks good to me. Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > drivers/platform/x86/intel/ifs/ifs.h | 14 ++++++++++++++ > drivers/platform/x86/intel/ifs/runtest.c | 6 ++++++ > 2 files changed, 20 insertions(+) > > diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h > index 56b9f3e3cf76..71d8b50854b2 100644 > --- a/drivers/platform/x86/intel/ifs/ifs.h > +++ b/drivers/platform/x86/intel/ifs/ifs.h > @@ -325,4 +325,18 @@ int do_core_test(int cpu, struct device *dev); > extern struct attribute *plat_ifs_attrs[]; > extern struct attribute *plat_ifs_array_attrs[]; > > +static inline void ifs_fpu_begin(void) > +{ > + /* > + * The AMX state must be initialized prior to executing In-Field > + * Scan tests, according to Intel SDM. Nit: May be helpful to include the section title in SDM. > + */ > + kernel_fpu_begin_mask(KFPU_AMX); > +} > + > +static inline void ifs_fpu_end(void) > +{ > + kernel_fpu_end(); > +} > + > #endif > diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c > index 95b4b71fab53..a35eac7c0b44 100644 > --- a/drivers/platform/x86/intel/ifs/runtest.c > +++ b/drivers/platform/x86/intel/ifs/runtest.c > @@ -188,6 +188,9 @@ 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*/ > + ifs_fpu_begin(); > + > wait_for_sibling_cpu(&scan_cpus_in, NSEC_PER_SEC); > > /* > @@ -199,6 +202,9 @@ static int doscan(void *data) > * are processed in a single pass) before it retires. > */ > wrmsrl(MSR_ACTIVATE_SCAN, params->activate->data); > + > + ifs_fpu_end(); > + > rdmsrl(MSR_SCAN_STATUS, status.data); > > trace_ifs_status(ifsd->cur_batch, start, stop, status.data);
diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h index 56b9f3e3cf76..71d8b50854b2 100644 --- a/drivers/platform/x86/intel/ifs/ifs.h +++ b/drivers/platform/x86/intel/ifs/ifs.h @@ -325,4 +325,18 @@ int do_core_test(int cpu, struct device *dev); extern struct attribute *plat_ifs_attrs[]; extern struct attribute *plat_ifs_array_attrs[]; +static inline void ifs_fpu_begin(void) +{ + /* + * The AMX state must be initialized prior to executing In-Field + * Scan tests, according to Intel SDM. + */ + kernel_fpu_begin_mask(KFPU_AMX); +} + +static inline void ifs_fpu_end(void) +{ + kernel_fpu_end(); +} + #endif diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c index 95b4b71fab53..a35eac7c0b44 100644 --- a/drivers/platform/x86/intel/ifs/runtest.c +++ b/drivers/platform/x86/intel/ifs/runtest.c @@ -188,6 +188,9 @@ 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*/ + ifs_fpu_begin(); + wait_for_sibling_cpu(&scan_cpus_in, NSEC_PER_SEC); /* @@ -199,6 +202,9 @@ static int doscan(void *data) * are processed in a single pass) before it retires. */ wrmsrl(MSR_ACTIVATE_SCAN, params->activate->data); + + ifs_fpu_end(); + rdmsrl(MSR_SCAN_STATUS, status.data); trace_ifs_status(ifsd->cur_batch, start, stop, status.data);
The scan test does not start when the AMX state remains active and is not re-initialized. With the extension of kernel_fpu_begin_mask(), the driver code can now initialize the state properly. Introduce custom FPU handling wrappers to ensure compliant with the established FPU API semantics, as kernel_fpu_begin() exclusively sets legacy states. This follows the EFI case from commit b0dc553cfc9d ("x86/fpu: Make the EFI FPU calling convention explicit"). Then, use these wrappers to surround the MSR_ACTIVATE_SCAN write to minimize the critical section. To prevent unnecessary delays, invoke ifs_fpu_begin() before entering the rendezvous loop. Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com> Reviewed-by: Jithu Joseph <jithu.joseph@intel.com> Tested-by: Jithu Joseph <jithu.joseph@intel.com> --- drivers/platform/x86/intel/ifs/ifs.h | 14 ++++++++++++++ drivers/platform/x86/intel/ifs/runtest.c | 6 ++++++ 2 files changed, 20 insertions(+)