[PATCH 3/4] x86/fpu: Clarify the XSTATE clearing only for extended components

[PATCH 3/4] x86/fpu: Clarify the XSTATE clearing only for extended components

[Chang S. Bae]

Commit 087df48c298c ("x86/fpu: Replace KVMs xstate component clearing")
refactored the MPX state clearing code.

But, legacy states are not warranted in this routine. Rename the function
to clarify that only extended components are acceptable.

Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Cc: x86@kernel.org
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 arch/x86/include/asm/fpu/api.h | 2 +-
 arch/x86/kernel/fpu/xstate.c   | 7 +++++--
 arch/x86/kvm/x86.c             | 4 ++--
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/fpu/api.h b/arch/x86/include/asm/fpu/api.h
index 503a577814b2..c9d5dc85ca06 100644
--- a/arch/x86/include/asm/fpu/api.h
+++ b/arch/x86/include/asm/fpu/api.h
@@ -130,7 +130,7 @@ static inline void fpstate_free(struct fpu *fpu) { }
 #endif
 
 /* fpstate-related functions which are exported to KVM */
-extern void fpstate_clear_xstate_component(struct fpstate *fps, unsigned int xfeature);
+extern void fpstate_clear_extended_xstate(struct fpstate *fps, unsigned int xfeature);
 
 extern u64 xstate_get_guest_group_perm(void);
 
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index d7676cfc32eb..a35f91360e3f 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1371,14 +1371,17 @@ void xrstors(struct xregs_state *xstate, u64 mask)
 }
 
 #if IS_ENABLED(CONFIG_KVM)
-void fpstate_clear_xstate_component(struct fpstate *fps, unsigned int xfeature)
+void fpstate_clear_extended_xstate(struct fpstate *fps, unsigned int xfeature)
 {
 	void *addr = get_xsave_addr(&fps->regs.xsave, xfeature);
 
+	if (xfeature <= XFEATURE_SSE)
+		return;
+
 	if (addr)
 		memset(addr, 0, xstate_sizes[xfeature]);
 }
-EXPORT_SYMBOL_GPL(fpstate_clear_xstate_component);
+EXPORT_SYMBOL_GPL(fpstate_clear_extended_xstate);
 #endif
 
 #ifdef CONFIG_X86_64
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 43a6a7efc6ec..82ab270ea734 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -11760,8 +11760,8 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 		if (init_event)
 			kvm_put_guest_fpu(vcpu);
 
-		fpstate_clear_xstate_component(fpstate, XFEATURE_BNDREGS);
-		fpstate_clear_xstate_component(fpstate, XFEATURE_BNDCSR);
+		fpstate_clear_extended_xstate(fpstate, XFEATURE_BNDREGS);
+		fpstate_clear_extended_xstate(fpstate, XFEATURE_BNDCSR);
 
 		if (init_event)
 			kvm_load_guest_fpu(vcpu);
-- 
2.17.1

Re: [PATCH 3/4] x86/fpu: Clarify the XSTATE clearing only for extended components

[Sean Christopherson]

On Fri, Sep 16, 2022, Chang S. Bae wrote:
> Commit 087df48c298c ("x86/fpu: Replace KVMs xstate component clearing")
> refactored the MPX state clearing code.
> 
> But, legacy states are not warranted in this routine.

Why not?  I could probably figure it out eventually, but that info should be
provided in the changelog.

> Rename the function to clarify that only extended components are acceptable.

The function rename isn't the interesting part of the patch, explicitly disallowing
"legacy" states is what's interesting.  The shortlog+changelog should reflect that.

> Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
> Cc: x86@kernel.org
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  arch/x86/include/asm/fpu/api.h | 2 +-
>  arch/x86/kernel/fpu/xstate.c   | 7 +++++--
>  arch/x86/kvm/x86.c             | 4 ++--
>  3 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/include/asm/fpu/api.h b/arch/x86/include/asm/fpu/api.h
> index 503a577814b2..c9d5dc85ca06 100644
> --- a/arch/x86/include/asm/fpu/api.h
> +++ b/arch/x86/include/asm/fpu/api.h
> @@ -130,7 +130,7 @@ static inline void fpstate_free(struct fpu *fpu) { }
>  #endif
>  
>  /* fpstate-related functions which are exported to KVM */
> -extern void fpstate_clear_xstate_component(struct fpstate *fps, unsigned int xfeature);
> +extern void fpstate_clear_extended_xstate(struct fpstate *fps, unsigned int xfeature);
>  
>  extern u64 xstate_get_guest_group_perm(void);
>  
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index d7676cfc32eb..a35f91360e3f 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -1371,14 +1371,17 @@ void xrstors(struct xregs_state *xstate, u64 mask)
>  }
>  
>  #if IS_ENABLED(CONFIG_KVM)
> -void fpstate_clear_xstate_component(struct fpstate *fps, unsigned int xfeature)
> +void fpstate_clear_extended_xstate(struct fpstate *fps, unsigned int xfeature)
>  {
>  	void *addr = get_xsave_addr(&fps->regs.xsave, xfeature);
>  
> +	if (xfeature <= XFEATURE_SSE)

This should WARN_ON_ONCE(), silently doing nothing in the presence of buggy code
isn't much better than clobbering state.

> +		return;
> +
>  	if (addr)
>  		memset(addr, 0, xstate_sizes[xfeature]);
>  }
> -EXPORT_SYMBOL_GPL(fpstate_clear_xstate_component);
> +EXPORT_SYMBOL_GPL(fpstate_clear_extended_xstate);
>  #endif
>  
>  #ifdef CONFIG_X86_64
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 43a6a7efc6ec..82ab270ea734 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -11760,8 +11760,8 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>  		if (init_event)
>  			kvm_put_guest_fpu(vcpu);
>  
> -		fpstate_clear_xstate_component(fpstate, XFEATURE_BNDREGS);
> -		fpstate_clear_xstate_component(fpstate, XFEATURE_BNDCSR);
> +		fpstate_clear_extended_xstate(fpstate, XFEATURE_BNDREGS);
> +		fpstate_clear_extended_xstate(fpstate, XFEATURE_BNDCSR);

From a KVM perspective, I strongly prefer the existing name.  The "component"
part makes it very clear that the helper clears a single component, whereas it's
not obvious at first glances that the version without "component" only affects
the specified feature.

The obvious alternative is something like fpstate_clear_extended_xstate_component(),
but I don't really see the point.  "xstate" is "extended state" after all, which
is partly why I find fpstate_clear_extended_xstate() confusing; it makes me think
the helper is for some fancy "extended extended state".