Date:   Tue, 7 Dec 2021 23:43:47 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Lai Jiangshan <jiangshanlai@gmail.com>
Cc:     linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        Lai Jiangshan <laijs@linux.alibaba.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>
Subject: Re: [PATCH 16/15] KVM: X86: Update mmu->pdptrs only when it is
 changed
Message-ID: <Ya/xsx1pcB0Pq/Pm@google.com>
References: <20211108124407.12187-1-jiangshanlai@gmail.com>
 <20211111144527.88852-1-jiangshanlai@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211111144527.88852-1-jiangshanlai@gmail.com>
Precedence: bulk

On Thu, Nov 11, 2021, Lai Jiangshan wrote:
> From: Lai Jiangshan <laijs@linux.alibaba.com>
> 
> It is unchanged in most cases.
> 
> Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
> ---
>  arch/x86/kvm/x86.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 6ca19cac4aff..0176eaa86a35 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -828,10 +828,13 @@ int load_pdptrs(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, unsigned long cr3)
>  		}
>  	}
>  
> -	memcpy(mmu->pdptrs, pdpte, sizeof(mmu->pdptrs));
> -	kvm_register_mark_dirty(vcpu, VCPU_EXREG_PDPTR);
> -	/* Ensure the dirty PDPTEs to be loaded. */
> -	kvm_make_request(KVM_REQ_LOAD_MMU_PGD, vcpu);
> +	kvm_register_mark_available(vcpu, VCPU_EXREG_PDPTR);
> +	if (memcmp(mmu->pdptrs, pdpte, sizeof(mmu->pdptrs))) {
> +		memcpy(mmu->pdptrs, pdpte, sizeof(mmu->pdptrs));
> +		kvm_register_mark_dirty(vcpu, VCPU_EXREG_PDPTR);
> +		/* Ensure the dirty PDPTEs to be loaded. */
> +		kvm_make_request(KVM_REQ_LOAD_MMU_PGD, vcpu);
> +	}

Can this be unqueued until there's sufficient justification that (a) this is
correct and (b) actually provides a meaningful performance optimization?  There
have been far too many PDPTR caching bugs to make this change without an analysis
of why it's safe, e.g. what guarantees the that PDPTRs in the VMCS are sync'd
with mmu->pdptrs?  I'm not saying they aren't, I just want the changelog to prove
that they are.

The next patch does add a fairly heavy unload of the current root for !TDP, but
that's a bug fix and should be ordered before any optimizations anyways.

>  	vcpu->arch.pdptrs_from_userspace = false;
>  
>  	return 1;
> -- 
> 2.19.1.6.gb485710b
>