linux-kernel - Re: [PATCH v2 05/10] KVM: x86: Remove defunct setting of XCR0 for guest during vCPU create

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87wnna805m.fsf@vitty.brq.redhat.com>
Date:   Tue, 21 Sep 2021 16:37:41 +0200
From:   Vitaly Kuznetsov <vkuznets@...hat.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Reiji Watanabe <reijiw@...gle.com>
Subject: Re: [PATCH v2 05/10] KVM: x86: Remove defunct setting of XCR0 for
 guest during vCPU create

Sean Christopherson <seanjc@...gle.com> writes:

> Drop code to initialize XCR0 during fx_init(), a.k.a. vCPU creation, as
> XCR0 has been initialized during kvm_vcpu_reset() (for RESET) since
> commit a554d207dc46 ("KVM: X86: Processor States following Reset or INIT").
>
> Back when XCR0 support was added by commit 2acf923e38fb ("KVM: VMX:
> Enable XSAVE/XRSTOR for guest"), KVM didn't differentiate between RESET
> and INIT.  Ignoring the fact that calling fx_init() for INIT is obviously
> wrong, e.g. FPU state after INIT is not the same as after RESET, setting
> XCR0 in fx_init() was correct.
>
> Eventually fx_init() got moved to kvm_arch_vcpu_init(), a.k.a. vCPU
> creation (ignore the terrible name) by commit 0ee6a5172573 ("x86/fpu,
> kvm: Simplify fx_init()").  Finally, commit 95a0d01eef7a ("KVM: x86: Move
> all vcpu init code into kvm_arch_vcpu_create()") killed off
> kvm_arch_vcpu_init(),

Technically, empty kvm_arch_vcpu_init() was still alive for a few more
commits and only ddd259c9aaba ("KVM: Drop kvm_arch_vcpu_init() and
kvm_arch_vcpu_uninit()") killed it for real but a curious reader can
find all these gory details himself)

> leaving behind the oddity of redundant setting of
> guest state during vCPU creation.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>  arch/x86/kvm/x86.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e0bff5473813..6fd3fe21863e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -993,7 +993,7 @@ static int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
>  	/*
>  	 * Do not allow the guest to set bits that we do not support
>  	 * saving.  However, xcr0 bit 0 is always set, even if the
> -	 * emulated CPU does not support XSAVE (see fx_init).
> +	 * emulated CPU does not support XSAVE (see kvm_vcpu_reset()).
>  	 */
>  	valid_bits = vcpu->arch.guest_supported_xcr0 | XFEATURE_MASK_FP;
>  	if (xcr0 & ~valid_bits)
> @@ -10623,11 +10623,6 @@ static void fx_init(struct kvm_vcpu *vcpu)
>  	if (boot_cpu_has(X86_FEATURE_XSAVES))
>  		vcpu->arch.guest_fpu->state.xsave.header.xcomp_bv =
>  			host_xcr0 | XSTATE_COMPACTION_ENABLED;
> -
> -	/*
> -	 * Ensure guest xcr0 is valid for loading
> -	 */
> -	vcpu->arch.xcr0 = XFEATURE_MASK_FP;
>  }
>  
>  void kvm_free_guest_fpu(struct kvm_vcpu *vcpu)

Reviewed-by: Vitaly Kuznetsov <vkuznets@...hat.com>

-- 
Vitaly