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@xxxxxxxxx> > Cc: x86@xxxxxxxxxx > Cc: kvm@xxxxxxxxxxxxxxx > Cc: linux-kernel@xxxxxxxxxxxxxxx > --- > 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".