* [PATCH 1/4] KVM: nVMX: Drop obsolete (and pointless) pdptrs_changed() check
2021-04-23 0:06 [PATCH 0/4] KVM: x86: Kill off pdptrs_changed() Sean Christopherson
@ 2021-04-23 0:06 ` Sean Christopherson
2021-04-23 0:06 ` [PATCH 2/4] KVM: nSVM: Drop pointless pdptrs_changed() check on nested transition Sean Christopherson
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Sean Christopherson @ 2021-04-23 0:06 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel
Remove the pdptrs_changed() check when loading L2's CR3. The set of
available registers is always reset when switching VMCSes (see commit
e5d03de5937e, "KVM: nVMX: Reset register cache (available and dirty
masks) on VMCS switch"), thus the "are PDPTRs available" check will
always fail. And even if it didn't fail, reading guest memory to check
the PDPTRs is just as expensive as reading guest memory to load 'em.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/kvm/vmx/nested.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 00339d624c92..eece7fff0441 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -1118,11 +1118,9 @@ static int nested_vmx_load_cr3(struct kvm_vcpu *vcpu, unsigned long cr3, bool ne
* must not be dereferenced.
*/
if (!nested_ept && is_pae_paging(vcpu) &&
- (cr3 != kvm_read_cr3(vcpu) || pdptrs_changed(vcpu))) {
- if (CC(!load_pdptrs(vcpu, vcpu->arch.walk_mmu, cr3))) {
- *entry_failure_code = ENTRY_FAIL_PDPTE;
- return -EINVAL;
- }
+ CC(!load_pdptrs(vcpu, vcpu->arch.walk_mmu, cr3))) {
+ *entry_failure_code = ENTRY_FAIL_PDPTE;
+ return -EINVAL;
}
/*
--
2.31.1.498.g6c1eba8ee3d-goog
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 2/4] KVM: nSVM: Drop pointless pdptrs_changed() check on nested transition
2021-04-23 0:06 [PATCH 0/4] KVM: x86: Kill off pdptrs_changed() Sean Christopherson
2021-04-23 0:06 ` [PATCH 1/4] KVM: nVMX: Drop obsolete (and pointless) pdptrs_changed() check Sean Christopherson
@ 2021-04-23 0:06 ` Sean Christopherson
2021-04-23 0:06 ` [PATCH 3/4] KVM: x86: Always load PDPTRs on CR3 load for SVM w/o NPT and a PAE guest Sean Christopherson
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Sean Christopherson @ 2021-04-23 0:06 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel
Remove the "PDPTRs unchanged" check to skip PDPTR loading during nested
SVM transitions as it's not at all an optimization. Reading guest memory
to get the PDPTRs isn't magically cheaper by doing it in pdptrs_changed(),
and if the PDPTRs did change, KVM will end up doing the read twice.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/kvm/svm/nested.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 540d43ba2cf4..9cc95895866a 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -391,10 +391,8 @@ static int nested_svm_load_cr3(struct kvm_vcpu *vcpu, unsigned long cr3,
return -EINVAL;
if (!nested_npt && is_pae_paging(vcpu) &&
- (cr3 != kvm_read_cr3(vcpu) || pdptrs_changed(vcpu))) {
- if (CC(!load_pdptrs(vcpu, vcpu->arch.walk_mmu, cr3)))
- return -EINVAL;
- }
+ CC(!load_pdptrs(vcpu, vcpu->arch.walk_mmu, cr3)))
+ return -EINVAL;
/*
* TODO: optimize unconditional TLB flush/MMU sync here and in
--
2.31.1.498.g6c1eba8ee3d-goog
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 3/4] KVM: x86: Always load PDPTRs on CR3 load for SVM w/o NPT and a PAE guest
2021-04-23 0:06 [PATCH 0/4] KVM: x86: Kill off pdptrs_changed() Sean Christopherson
2021-04-23 0:06 ` [PATCH 1/4] KVM: nVMX: Drop obsolete (and pointless) pdptrs_changed() check Sean Christopherson
2021-04-23 0:06 ` [PATCH 2/4] KVM: nSVM: Drop pointless pdptrs_changed() check on nested transition Sean Christopherson
@ 2021-04-23 0:06 ` Sean Christopherson
2021-04-23 0:06 ` [PATCH 4/4] KVM: x86: Unexport kvm_read_guest_page_mmu() Sean Christopherson
2021-04-23 7:04 ` [PATCH 0/4] KVM: x86: Kill off pdptrs_changed() Paolo Bonzini
4 siblings, 0 replies; 6+ messages in thread
From: Sean Christopherson @ 2021-04-23 0:06 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel
Kill off pdptrs_changed() and instead go through the full kvm_set_cr3()
for PAE guest, even if the new CR3 is the same as the current CR3. For
VMX, and SVM with NPT enabled, the PDPTRs are unconditionally marked as
unavailable after VM-Exit, i.e. the optimization is dead code except for
SVM without NPT.
In the unlikely scenario that anyone cares about SVM without NPT _and_ a
PAE guest, they've got bigger problems if their guest is loading the same
CR3 so frequently that the performance of kvm_set_cr3() is notable,
especially since KVM's fast PGD switching means reloading the same CR3
does not require a full rebuild. Given that PAE and PCID are mutually
exclusive, i.e. a sync and flush are guaranteed in any case, the actual
benefits of the pdptrs_changed() optimization are marginal at best.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/include/asm/kvm_host.h | 1 -
arch/x86/kvm/x86.c | 34 ++-------------------------------
2 files changed, 2 insertions(+), 33 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 3e5fc80a35c8..30e95c52769c 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1475,7 +1475,6 @@ unsigned long kvm_mmu_calculate_default_mmu_pages(struct kvm *kvm);
void kvm_mmu_change_mmu_pages(struct kvm *kvm, unsigned long kvm_nr_mmu_pages);
int load_pdptrs(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, unsigned long cr3);
-bool pdptrs_changed(struct kvm_vcpu *vcpu);
int emulator_write_phys(struct kvm_vcpu *vcpu, gpa_t gpa,
const void *val, int bytes);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 3bf52ba5f2bb..d099d6e54a6f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -751,13 +751,6 @@ int kvm_read_guest_page_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
}
EXPORT_SYMBOL_GPL(kvm_read_guest_page_mmu);
-static int kvm_read_nested_guest_page(struct kvm_vcpu *vcpu, gfn_t gfn,
- void *data, int offset, int len, u32 access)
-{
- return kvm_read_guest_page_mmu(vcpu, vcpu->arch.walk_mmu, gfn,
- data, offset, len, access);
-}
-
static inline u64 pdptr_rsvd_bits(struct kvm_vcpu *vcpu)
{
return vcpu->arch.reserved_gpa_bits | rsvd_bits(5, 8) | rsvd_bits(1, 2);
@@ -799,30 +792,6 @@ int load_pdptrs(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, unsigned long cr3)
}
EXPORT_SYMBOL_GPL(load_pdptrs);
-bool pdptrs_changed(struct kvm_vcpu *vcpu)
-{
- u64 pdpte[ARRAY_SIZE(vcpu->arch.walk_mmu->pdptrs)];
- int offset;
- gfn_t gfn;
- int r;
-
- if (!is_pae_paging(vcpu))
- return false;
-
- if (!kvm_register_is_available(vcpu, VCPU_EXREG_PDPTR))
- return true;
-
- gfn = (kvm_read_cr3(vcpu) & 0xffffffe0ul) >> PAGE_SHIFT;
- offset = (kvm_read_cr3(vcpu) & 0xffffffe0ul) & (PAGE_SIZE - 1);
- r = kvm_read_nested_guest_page(vcpu, gfn, pdpte, offset, sizeof(pdpte),
- PFERR_USER_MASK | PFERR_WRITE_MASK);
- if (r < 0)
- return true;
-
- return memcmp(pdpte, vcpu->arch.walk_mmu->pdptrs, sizeof(pdpte)) != 0;
-}
-EXPORT_SYMBOL_GPL(pdptrs_changed);
-
void kvm_post_set_cr0(struct kvm_vcpu *vcpu, unsigned long old_cr0, unsigned long cr0)
{
unsigned long update_bits = X86_CR0_PG | X86_CR0_WP;
@@ -1069,7 +1038,8 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
}
#endif
- if (cr3 == kvm_read_cr3(vcpu) && !pdptrs_changed(vcpu)) {
+ /* PDPTRs are always reloaded for PAE paging. */
+ if (cr3 == kvm_read_cr3(vcpu) && !is_pae_paging(vcpu)) {
if (!skip_tlb_flush) {
kvm_mmu_sync_roots(vcpu);
kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu);
--
2.31.1.498.g6c1eba8ee3d-goog
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 4/4] KVM: x86: Unexport kvm_read_guest_page_mmu()
2021-04-23 0:06 [PATCH 0/4] KVM: x86: Kill off pdptrs_changed() Sean Christopherson
` (2 preceding siblings ...)
2021-04-23 0:06 ` [PATCH 3/4] KVM: x86: Always load PDPTRs on CR3 load for SVM w/o NPT and a PAE guest Sean Christopherson
@ 2021-04-23 0:06 ` Sean Christopherson
2021-04-23 7:04 ` [PATCH 0/4] KVM: x86: Kill off pdptrs_changed() Paolo Bonzini
4 siblings, 0 replies; 6+ messages in thread
From: Sean Christopherson @ 2021-04-23 0:06 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel
Unexport kvm_read_guest_page_mmu(), its only current user is for loading
PDPTRs, and with luck, KVM will not have to support similar insanity in
the future.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/include/asm/kvm_host.h | 3 ---
arch/x86/kvm/x86.c | 7 +++----
2 files changed, 3 insertions(+), 7 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 30e95c52769c..be271fdf584e 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1614,9 +1614,6 @@ void kvm_requeue_exception_e(struct kvm_vcpu *vcpu, unsigned nr, u32 error_code)
void kvm_inject_page_fault(struct kvm_vcpu *vcpu, struct x86_exception *fault);
bool kvm_inject_emulated_page_fault(struct kvm_vcpu *vcpu,
struct x86_exception *fault);
-int kvm_read_guest_page_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
- gfn_t gfn, void *data, int offset, int len,
- u32 access);
bool kvm_require_cpl(struct kvm_vcpu *vcpu, int required_cpl);
bool kvm_require_dr(struct kvm_vcpu *vcpu, int dr);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d099d6e54a6f..06bc59c3abb9 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -732,9 +732,9 @@ EXPORT_SYMBOL_GPL(kvm_require_dr);
* running guest. The difference to kvm_vcpu_read_guest_page is that this function
* can read from guest physical or from the guest's guest physical memory.
*/
-int kvm_read_guest_page_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
- gfn_t ngfn, void *data, int offset, int len,
- u32 access)
+static int kvm_read_guest_page_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
+ gfn_t ngfn, void *data, int offset, int len,
+ u32 access)
{
struct x86_exception exception;
gfn_t real_gfn;
@@ -749,7 +749,6 @@ int kvm_read_guest_page_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
return kvm_vcpu_read_guest_page(vcpu, real_gfn, data, offset, len);
}
-EXPORT_SYMBOL_GPL(kvm_read_guest_page_mmu);
static inline u64 pdptr_rsvd_bits(struct kvm_vcpu *vcpu)
{
--
2.31.1.498.g6c1eba8ee3d-goog
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 0/4] KVM: x86: Kill off pdptrs_changed()
2021-04-23 0:06 [PATCH 0/4] KVM: x86: Kill off pdptrs_changed() Sean Christopherson
` (3 preceding siblings ...)
2021-04-23 0:06 ` [PATCH 4/4] KVM: x86: Unexport kvm_read_guest_page_mmu() Sean Christopherson
@ 2021-04-23 7:04 ` Paolo Bonzini
4 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2021-04-23 7:04 UTC (permalink / raw)
To: Sean Christopherson, Maxim Levitsky
Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
linux-kernel
On 23/04/21 02:06, Sean Christopherson wrote:
> Remove pdptrs_changed(), which is mostly dead anyways, and the few bits
> that are still thrashing are useless.
>
> This conflicts with Maxim's work to migrate PDPTRs out-of-band, but I
> think it will conflict in a good way as the "skip load_pdptrs()"
> logic for the out-of-band case won't have to juggle this legacy crud.
Maxim, can you integrate these patches in your series yourself?
Thanks,
Paolo
> Sean Christopherson (4):
> KVM: nVMX: Drop obsolete (and pointless) pdptrs_changed() check
> KVM: nSVM: Drop pointless pdptrs_changed() check on nested transition
> KVM: x86: Always load PDPTRs on CR3 load for SVM w/o NPT and a PAE
> guest
> KVM: x86: Unexport kvm_read_guest_page_mmu()
>
> arch/x86/include/asm/kvm_host.h | 4 ----
> arch/x86/kvm/svm/nested.c | 6 ++---
> arch/x86/kvm/vmx/nested.c | 8 +++----
> arch/x86/kvm/x86.c | 41 ++++-----------------------------
> 4 files changed, 10 insertions(+), 49 deletions(-)
>
^ permalink raw reply [flat|nested] 6+ messages in thread