Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 06/30] KVM: SVM: always update CR3 in VMCB
To:     Paolo Bonzini <pbonzini@redhat.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org
References: <20200529153934.11694-1-pbonzini@redhat.com>
 <20200529153934.11694-7-pbonzini@redhat.com>
From:   Krish Sadhukhan <krish.sadhukhan@oracle.com>
Message-ID: <07139fc2-39bd-5bc5-ef23-a98681013665@oracle.com>
Date:   Fri, 29 May 2020 10:41:58 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.7.0
MIME-Version: 1.0
In-Reply-To: <20200529153934.11694-7-pbonzini@redhat.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 5/29/20 8:39 AM, Paolo Bonzini wrote:
> svm_load_mmu_pgd is delaying the write of GUEST_CR3 to prepare_vmcs02

Did you mean to say enter_svm_guest_mode here ?
> as
> an optimization, but this is only correct before the nested vmentry.
> If userspace is modifying CR3 with KVM_SET_SREGS after the VM has
> already been put in guest mode, the value of CR3 will not be updated.
> Remove the optimization, which almost never triggers anyway.
> This was was added in commit 689f3bf21628 ("KVM: x86: unify callbacks
> to load paging root", 2020-03-16) just to keep the two vendor-specific
> modules closer, but we'll fix VMX too.
>
> Fixes: 689f3bf21628 ("KVM: x86: unify callbacks to load paging root")
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   arch/x86/kvm/svm/nested.c |  6 +-----
>   arch/x86/kvm/svm/svm.c    | 16 +++++-----------
>   2 files changed, 6 insertions(+), 16 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index dcac4c3510ab..8756c9f463fd 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -256,11 +256,7 @@ void enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb_gpa,
>   	svm_set_efer(&svm->vcpu, nested_vmcb->save.efer);
>   	svm_set_cr0(&svm->vcpu, nested_vmcb->save.cr0);
>   	svm_set_cr4(&svm->vcpu, nested_vmcb->save.cr4);
> -	if (npt_enabled) {
> -		svm->vmcb->save.cr3 = nested_vmcb->save.cr3;
> -		svm->vcpu.arch.cr3 = nested_vmcb->save.cr3;
> -	} else
> -		(void)kvm_set_cr3(&svm->vcpu, nested_vmcb->save.cr3);
> +	(void)kvm_set_cr3(&svm->vcpu, nested_vmcb->save.cr3);
>   
>   	/* Guest paging mode is active - reset mmu */
>   	kvm_mmu_reset_context(&svm->vcpu);
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 545f63ebc720..feb96a410f2d 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3447,7 +3447,6 @@ static fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu)
>   static void svm_load_mmu_pgd(struct kvm_vcpu *vcpu, unsigned long root)
>   {
>   	struct vcpu_svm *svm = to_svm(vcpu);
> -	bool update_guest_cr3 = true;
>   	unsigned long cr3;
>   
>   	cr3 = __sme_set(root);
> @@ -3456,18 +3455,13 @@ static void svm_load_mmu_pgd(struct kvm_vcpu *vcpu, unsigned long root)
>   		mark_dirty(svm->vmcb, VMCB_NPT);
>   
>   		/* Loading L2's CR3 is handled by enter_svm_guest_mode.  */
> -		if (is_guest_mode(vcpu))
> -			update_guest_cr3 = false;
> -		else if (test_bit(VCPU_EXREG_CR3, (ulong *)&vcpu->arch.regs_avail))
> -			cr3 = vcpu->arch.cr3;
> -		else /* CR3 is already up-to-date.  */
> -			update_guest_cr3 = false;
> +		if (!test_bit(VCPU_EXREG_CR3, (ulong *)&vcpu->arch.regs_avail))
> +			return;
> +		cr3 = vcpu->arch.cr3;
>   	}
>   
> -	if (update_guest_cr3) {
> -		svm->vmcb->save.cr3 = cr3;
> -		mark_dirty(svm->vmcb, VMCB_CR);
> -	}
> +	svm->vmcb->save.cr3 = cr3;
> +	mark_dirty(svm->vmcb, VMCB_CR);
>   }
>   
>   static int is_disabled(void)