[PATCH] x86/fpu: Use consistent test for X86_FEATURE_XSAVES

[PATCH] x86/fpu: Use consistent test for X86_FEATURE_XSAVES

[Chang S. Bae]

When XSAVES is present, the kernel always takes advantage of it, and XSAVES
always uses compacted format.

The macro using_compacted_format() implies that using compacted format may
be possible without XSAVES (say by using XSAVEC), but that is not possible
here, so delete that confusing macro and simply test for what we want to
know in the first place -- if we have XSAVES or not.

Cleanup only. No functional change.

Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Reviewed-by: Len Brown <len.brown@intel.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: linux-kernel@vger.kernel.org
Cc: x86@kernel.org
---
 arch/x86/include/asm/fpu/xstate.h |  1 -
 arch/x86/kernel/fpu/regset.c      |  4 ++--
 arch/x86/kernel/fpu/signal.c      |  2 +-
 arch/x86/kernel/fpu/xstate.c      | 18 ++----------------
 4 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index 47a92232d595..96c43380b8c2 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -102,7 +102,6 @@ extern void __init update_regset_xstate_info(unsigned int size,
 
 void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr);
 const void *get_xsave_field_ptr(int xfeature_nr);
-int using_compacted_format(void);
 int xfeature_size(int xfeature_nr);
 struct membuf;
 void copy_xstate_to_kernel(struct membuf to, struct xregs_state *xsave);
diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
index c413756ba89f..3e52e15a4891 100644
--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -84,7 +84,7 @@ int xstateregs_get(struct task_struct *target, const struct user_regset *regset,
 
 	fpu__prepare_read(fpu);
 
-	if (using_compacted_format()) {
+	if (boot_cpu_has(X86_FEATURE_XSAVES)) {
 		copy_xstate_to_kernel(to, xsave);
 		return 0;
 	} else {
@@ -124,7 +124,7 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
 
 	fpu__prepare_write(fpu);
 
-	if (using_compacted_format()) {
+	if (boot_cpu_has(X86_FEATURE_XSAVES)) {
 		if (kbuf)
 			ret = copy_kernel_to_xstate(xsave, kbuf);
 		else
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index a4ec65317a7f..2d0efb9a27c1 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -405,7 +405,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 	if (use_xsave() && !fx_only) {
 		u64 init_bv = xfeatures_mask_user() & ~user_xfeatures;
 
-		if (using_compacted_format()) {
+		if (boot_cpu_has(X86_FEATURE_XSAVES)) {
 			ret = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
 		} else {
 			ret = __copy_from_user(&fpu->state.xsave, buf_fx, state_size);
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 683749b80ae2..0e5fa511f0a1 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -500,20 +500,6 @@ int xfeature_size(int xfeature_nr)
 	return eax;
 }
 
-/*
- * 'XSAVES' implies two different things:
- * 1. saving of supervisor/system state
- * 2. using the compacted format
- *
- * Use this function when dealing with the compacted format so
- * that it is obvious which aspect of 'XSAVES' is being handled
- * by the calling code.
- */
-int using_compacted_format(void)
-{
-	return boot_cpu_has(X86_FEATURE_XSAVES);
-}
-
 /* Validate an xstate header supplied by userspace (ptrace or sigreturn) */
 int validate_user_xstate_header(const struct xstate_header *hdr)
 {
@@ -634,7 +620,7 @@ static void do_extra_xstate_size_checks(void)
 		 * Supervisor state components can be managed only by
 		 * XSAVES, which is compacted-format only.
 		 */
-		if (!using_compacted_format())
+		if (!boot_cpu_has(X86_FEATURE_XSAVES))
 			XSTATE_WARN_ON(xfeature_is_supervisor(i));
 
 		/* Align from the end of the previous feature */
@@ -646,7 +632,7 @@ static void do_extra_xstate_size_checks(void)
 		 * them for being ordered (increasing offsets) in
 		 * setup_xstate_features().
 		 */
-		if (!using_compacted_format())
+		if (!boot_cpu_has(X86_FEATURE_XSAVES))
 			paranoid_xstate_size = xfeature_uncompacted_offset(i);
 		/*
 		 * The compacted-format offset always depends on where
-- 
2.17.1

Re: [PATCH] x86/fpu: Use consistent test for X86_FEATURE_XSAVES

[Borislav Petkov]

On Tue, Feb 02, 2021 at 06:40:52PM -0800, Chang S. Bae wrote:
> When XSAVES is present, the kernel always takes advantage of it, and XSAVES
> always uses compacted format.
> 
> The macro using_compacted_format() implies that using compacted format may

Not a macro.

> be possible without XSAVES (say by using XSAVEC), but that is not possible
> here, so delete that confusing macro and simply test for what we want to
> know in the first place -- if we have XSAVES or not.

Who's "we"?

> @@ -500,20 +500,6 @@ int xfeature_size(int xfeature_nr)
>  	return eax;
>  }
>  
> -/*
> - * 'XSAVES' implies two different things:
> - * 1. saving of supervisor/system state
> - * 2. using the compacted format
> - *
> - * Use this function when dealing with the compacted format so
> - * that it is obvious which aspect of 'XSAVES' is being handled
> - * by the calling code.

@dhansen, are you still hung up on that "obvious aspect" or can we kill
this?

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg

Re: [PATCH] x86/fpu: Use consistent test for X86_FEATURE_XSAVES

[Dave Hansen]

On 2/3/21 3:23 AM, Borislav Petkov wrote:
>> -/*
>> - * 'XSAVES' implies two different things:
>> - * 1. saving of supervisor/system state
>> - * 2. using the compacted format
>> - *
>> - * Use this function when dealing with the compacted format so
>> - * that it is obvious which aspect of 'XSAVES' is being handled
>> - * by the calling code.
> @dhansen, are you still hung up on that "obvious aspect" or can we kill
> this?

I still want the compacted-format handling code to be marked.  You can
do that with new comments:

	/* Note: XSAVES always uses compacted format: */
	if (boot_cpu_has(X86_FEATURE_XSAVES)) {

or, leave it as-is:

	if (using_compacted_format()) {
	...

Otherwise, we assume that every human being that looks at this code
*KNOWS* that XSAVES==compacted.  That's not a great assumption.

Re: [PATCH] x86/fpu: Use consistent test for X86_FEATURE_XSAVES

[Borislav Petkov]

On Wed, Feb 03, 2021 at 07:40:07AM -0800, Dave Hansen wrote:
> On 2/3/21 3:23 AM, Borislav Petkov wrote:
> >> -/*
> >> - * 'XSAVES' implies two different things:
> >> - * 1. saving of supervisor/system state
> >> - * 2. using the compacted format
> >> - *
> >> - * Use this function when dealing with the compacted format so
> >> - * that it is obvious which aspect of 'XSAVES' is being handled
> >> - * by the calling code.
> > @dhansen, are you still hung up on that "obvious aspect" or can we kill
> > this?
> 
> I still want the compacted-format handling code to be marked.  You can
> do that with new comments:
> 
> 	/* Note: XSAVES always uses compacted format: */
> 	if (boot_cpu_has(X86_FEATURE_XSAVES)) {
> 
> or, leave it as-is:
> 
> 	if (using_compacted_format()) {
> 	...
> 
> Otherwise, we assume that every human being that looks at this code
> *KNOWS* that XSAVES==compacted.  That's not a great assumption.

Well, the reason why I reacted to this is because I was looking at
using_compacted_format() - aha this, checks X86_FEATURE_XSAVES - but
then other code paths in fpu/ check X86_FEATURE_XSAVES directly. And
this is confusing, making me wonder why is that special oneliner there.
Sure, the comment above it says why...

I guess if you wanna keep it, then we need another oneliner for 1. or
really do comments at each call site.

Hmm.

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg