linux-kernel - Re: [PATCH 16/15] KVM: X86: Update mmu->pdptrs only when it is changed

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: <Ya/xsx1pcB0Pq/Pm@google.com>
Date:   Tue, 7 Dec 2021 23:43:47 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Lai Jiangshan <jiangshanlai@...il.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        Lai Jiangshan <laijs@...ux.alibaba.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH 16/15] KVM: X86: Update mmu->pdptrs only when it is
 changed

On Thu, Nov 11, 2021, Lai Jiangshan wrote:
> From: Lai Jiangshan <laijs@...ux.alibaba.com>
> 
> It is unchanged in most cases.
> 
> Signed-off-by: Lai Jiangshan <laijs@...ux.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
>