From: Vitaly Kuznetsov <vkuznets@redhat.com>
To: Sean Christopherson <sean.j.christopherson@intel.com>,
Paolo Bonzini <pbonzini@redhat.com>
Cc: Sean Christopherson <sean.j.christopherson@intel.com>,
Wanpeng Li <wanpengli@tencent.com>,
Jim Mattson <jmattson@google.com>, Joerg Roedel <joro@8bytes.org>,
kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 5/9] KVM: x86: Pull the PGD's level from the MMU instead of recalculating it
Date: Wed, 22 Jul 2020 19:11:26 +0200 [thread overview]
Message-ID: <871rl3pj9d.fsf@vitty.brq.redhat.com> (raw)
In-Reply-To: <20200716034122.5998-6-sean.j.christopherson@intel.com>
Sean Christopherson <sean.j.christopherson@intel.com> writes:
> Use the shadow_root_level from the current MMU as the root level for the
> PGD, i.e. for VMX's EPTP. This eliminates the weird dependency between
> VMX and the MMU where both must independently calculate the same root
> level for things to work correctly. Temporarily keep VMX's calculation
> of the level and use it to WARN if the incoming level diverges.
>
> Opportunistically refactor kvm_mmu_load_pgd() to avoid indentation hell,
> and rename a 'cr3' param in the load_mmu_pgd prototype that managed to
> survive the cr3 purge.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
> arch/x86/include/asm/kvm_host.h | 3 ++-
> arch/x86/kvm/mmu.h | 10 +++++++---
> arch/x86/kvm/svm/svm.c | 3 ++-
> arch/x86/kvm/vmx/nested.c | 2 +-
> arch/x86/kvm/vmx/vmx.c | 18 ++++++++++++------
> arch/x86/kvm/vmx/vmx.h | 3 ++-
> 6 files changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 1bab87a444d78..ce60f4c38843f 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1136,7 +1136,8 @@ struct kvm_x86_ops {
> int (*get_tdp_level)(struct kvm_vcpu *vcpu);
> u64 (*get_mt_mask)(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio);
>
> - void (*load_mmu_pgd)(struct kvm_vcpu *vcpu, unsigned long cr3);
> + void (*load_mmu_pgd)(struct kvm_vcpu *vcpu, unsigned long pgd,
> + int pgd_level);
>
> bool (*has_wbinvd_exit)(void);
>
> diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
> index 9f6554613babc..5efc6081ca138 100644
> --- a/arch/x86/kvm/mmu.h
> +++ b/arch/x86/kvm/mmu.h
> @@ -90,9 +90,13 @@ static inline unsigned long kvm_get_active_pcid(struct kvm_vcpu *vcpu)
>
> static inline void kvm_mmu_load_pgd(struct kvm_vcpu *vcpu)
> {
> - if (VALID_PAGE(vcpu->arch.mmu->root_hpa))
> - kvm_x86_ops.load_mmu_pgd(vcpu, vcpu->arch.mmu->root_hpa |
> - kvm_get_active_pcid(vcpu));
> + u64 root_hpa = vcpu->arch.mmu->root_hpa;
> +
> + if (!VALID_PAGE(root_hpa))
> + return;
> +
> + kvm_x86_ops.load_mmu_pgd(vcpu, root_hpa | kvm_get_active_pcid(vcpu),
> + vcpu->arch.mmu->shadow_root_level);
> }
>
> int kvm_tdp_page_fault(struct kvm_vcpu *vcpu, gpa_t gpa, u32 error_code,
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 783330d0e7b88..c70d7dd333061 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3541,7 +3541,8 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu)
> return exit_fastpath;
> }
>
> -static void svm_load_mmu_pgd(struct kvm_vcpu *vcpu, unsigned long root)
> +static void svm_load_mmu_pgd(struct kvm_vcpu *vcpu, unsigned long root,
> + int root_level)
> {
> struct vcpu_svm *svm = to_svm(vcpu);
> unsigned long cr3;
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 4d561edf6f9ca..50b56622e16a6 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2162,7 +2162,7 @@ static void prepare_vmcs02_constant_state(struct vcpu_vmx *vmx)
> * consistency checks.
> */
> if (enable_ept && nested_early_check)
> - vmcs_write64(EPT_POINTER, construct_eptp(&vmx->vcpu, 0));
> + vmcs_write64(EPT_POINTER, construct_eptp(&vmx->vcpu, 0, 4));
Nit: could we use MMU's PT64_ROOT_4LEVEL instead of '4' here?
>
> /* All VMFUNCs are currently emulated through L0 vmexits. */
> if (cpu_has_vmx_vmfunc())
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 791baa73e5786..244053cff0a3a 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -2933,14 +2933,16 @@ static void vmx_flush_tlb_all(struct kvm_vcpu *vcpu)
>
> static void vmx_flush_tlb_current(struct kvm_vcpu *vcpu)
> {
> - u64 root_hpa = vcpu->arch.mmu->root_hpa;
> + struct kvm_mmu *mmu = vcpu->arch.mmu;
> + u64 root_hpa = mmu->root_hpa;
>
> /* No flush required if the current context is invalid. */
> if (!VALID_PAGE(root_hpa))
> return;
>
> if (enable_ept)
> - ept_sync_context(construct_eptp(vcpu, root_hpa));
> + ept_sync_context(construct_eptp(vcpu, root_hpa,
> + mmu->shadow_root_level));
> else if (!is_guest_mode(vcpu))
> vpid_sync_context(to_vmx(vcpu)->vpid);
> else
> @@ -3078,11 +3080,12 @@ static int get_ept_level(struct kvm_vcpu *vcpu)
> return vmx_get_tdp_level(vcpu);
> }
>
> -u64 construct_eptp(struct kvm_vcpu *vcpu, unsigned long root_hpa)
> +u64 construct_eptp(struct kvm_vcpu *vcpu, unsigned long root_hpa,
> + int root_level)
> {
> u64 eptp = VMX_EPTP_MT_WB;
>
> - eptp |= (get_ept_level(vcpu) == 5) ? VMX_EPTP_PWL_5 : VMX_EPTP_PWL_4;
> + eptp |= (root_level == 5) ? VMX_EPTP_PWL_5 : VMX_EPTP_PWL_4;
>
> if (enable_ept_ad_bits &&
> (!is_guest_mode(vcpu) || nested_ept_ad_enabled(vcpu)))
> @@ -3092,7 +3095,8 @@ u64 construct_eptp(struct kvm_vcpu *vcpu, unsigned long root_hpa)
> return eptp;
> }
>
> -static void vmx_load_mmu_pgd(struct kvm_vcpu *vcpu, unsigned long pgd)
> +static void vmx_load_mmu_pgd(struct kvm_vcpu *vcpu, unsigned long pgd,
> + int pgd_level)
> {
> struct kvm *kvm = vcpu->kvm;
> bool update_guest_cr3 = true;
> @@ -3100,7 +3104,9 @@ static void vmx_load_mmu_pgd(struct kvm_vcpu *vcpu, unsigned long pgd)
> u64 eptp;
>
> if (enable_ept) {
> - eptp = construct_eptp(vcpu, pgd);
> + WARN_ON(pgd_level != get_ept_level(vcpu));
> +
> + eptp = construct_eptp(vcpu, pgd, pgd_level);
> vmcs_write64(EPT_POINTER, eptp);
>
> if (kvm_x86_ops.tlb_remote_flush) {
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index 3c55433ac1b21..26175a4759fa5 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -341,7 +341,8 @@ void set_cr4_guest_host_mask(struct vcpu_vmx *vmx);
> void ept_save_pdptrs(struct kvm_vcpu *vcpu);
> void vmx_get_segment(struct kvm_vcpu *vcpu, struct kvm_segment *var, int seg);
> void vmx_set_segment(struct kvm_vcpu *vcpu, struct kvm_segment *var, int seg);
> -u64 construct_eptp(struct kvm_vcpu *vcpu, unsigned long root_hpa);
> +u64 construct_eptp(struct kvm_vcpu *vcpu, unsigned long root_hpa,
> + int root_level);
> void update_exception_bitmap(struct kvm_vcpu *vcpu);
> void vmx_update_msr_bitmap(struct kvm_vcpu *vcpu);
> bool vmx_nmi_blocked(struct kvm_vcpu *vcpu);
--
Vitaly
next prev parent reply other threads:[~2020-07-22 17:11 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-16 3:41 [PATCH 0/9] KVM: x86: TDP level cleanups and shadow NPT fix Sean Christopherson
2020-07-16 3:41 ` [PATCH 1/9] KVM: nSVM: Correctly set the shadow NPT root level in its MMU role Sean Christopherson
2020-07-22 17:02 ` Vitaly Kuznetsov
2020-07-16 3:41 ` [PATCH 2/9] KVM: x86/mmu: Add separate helper for shadow NPT root page role calc Sean Christopherson
2020-07-22 17:05 ` Vitaly Kuznetsov
2020-07-16 3:41 ` [PATCH 3/9] KVM: VMX: Drop a duplicate declaration of construct_eptp() Sean Christopherson
2020-07-16 3:41 ` [PATCH 4/9] KVM: VMX: Make vmx_load_mmu_pgd() static Sean Christopherson
2020-07-16 3:41 ` [PATCH 5/9] KVM: x86: Pull the PGD's level from the MMU instead of recalculating it Sean Christopherson
2020-07-22 17:11 ` Vitaly Kuznetsov [this message]
2020-07-31 16:14 ` Sean Christopherson
2020-07-16 3:41 ` [PATCH 6/9] KVM: VXM: Remove temporary WARN on expected vs. actual EPTP level mismatch Sean Christopherson
2020-07-16 3:41 ` [PATCH 7/9] KVM: x86: Dynamically calculate TDP level from max level and MAXPHYADDR Sean Christopherson
2020-07-16 3:41 ` [PATCH 8/9] KVM: x86/mmu: Rename max_page_level to max_huge_page_level Sean Christopherson
2020-07-16 3:41 ` [PATCH 9/9] KVM: x86: Specify max TDP level via kvm_configure_mmu() Sean Christopherson
2020-07-30 22:18 ` [PATCH 0/9] KVM: x86: TDP level cleanups and shadow NPT fix Paolo Bonzini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=871rl3pj9d.fsf@vitty.brq.redhat.com \
--to=vkuznets@redhat.com \
--cc=jmattson@google.com \
--cc=joro@8bytes.org \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pbonzini@redhat.com \
--cc=sean.j.christopherson@intel.com \
--cc=wanpengli@tencent.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).