Re: [PATCH 12/12] KVM: x86: do not unload MMU roots on all role changes
From: Paolo Bonzini
Date: Mon Feb 14 2022 - 11:34:13 EST
On 2/11/22 19:48, Sean Christopherson wrote:
On Wed, Feb 09, 2022, Paolo Bonzini wrote:
kvm_mmu_reset_context is called on all role changes and right now it
calls kvm_mmu_unload. With the legacy MMU this is a relatively cheap
operation; the previous PGDs remains in the hash table and is picked
up immediately on the next page fault. With the TDP MMU, however, the
roots are thrown away for good and a full rebuild of the page tables is
necessary, which is many times more expensive.
Fortunately, throwing away the roots is not necessary except when
the manual says a TLB flush is required:
- changing CR0.PG from 1 to 0 (because it flushes the TLB according to
the x86 architecture specification)
- changing CPUID (which changes the interpretation of page tables in
ways not reflected by the role).
- changing CR4.SMEP from 0 to 1 (not doing so actually breaks access.c!)
Except for these cases, once the MMU has updated the CPU/MMU roles
and metadata it is enough to force-reload the current value of CR3.
KVM will look up the cached roots for an entry with the right role and
PGD, and only if the cache misses a new root will be created.
Measuring with vmexit.flat from kvm-unit-tests shows the following
improvement:
TDP legacy shadow
before 46754 5096 5150
after 4879 4875 5006
which is for very small page tables. The impact is however much larger
when running as an L1 hypervisor, because the new page tables cause
extra work for L0 to shadow them.
Reported-by: Brad Spengler <spender@xxxxxxxxxxxxxx>
Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
---
arch/x86/kvm/mmu/mmu.c | 7 ++++---
arch/x86/kvm/x86.c | 27 ++++++++++++++++++---------
2 files changed, 22 insertions(+), 12 deletions(-)
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 38b40ddcaad7..dbd4e98ba426 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -5020,8 +5020,8 @@ EXPORT_SYMBOL_GPL(kvm_init_mmu);
void kvm_mmu_after_set_cpuid(struct kvm_vcpu *vcpu)
{
/*
- * Invalidate all MMU roles to force them to reinitialize as CPUID
- * information is factored into reserved bit calculations.
+ * Invalidate all MMU roles and roots to force them to reinitialize,
+ * as CPUID information is factored into reserved bit calculations.
*
* Correctly handling multiple vCPU models with respect to paging and
* physical address properties) in a single VM would require tracking
@@ -5034,6 +5034,7 @@ void kvm_mmu_after_set_cpuid(struct kvm_vcpu *vcpu)
vcpu->arch.root_mmu.mmu_role.ext.valid = 0;
vcpu->arch.guest_mmu.mmu_role.ext.valid = 0;
vcpu->arch.nested_mmu.mmu_role.ext.valid = 0;
+ kvm_mmu_unload(vcpu);
kvm_mmu_reset_context(vcpu);
/*
@@ -5045,8 +5046,8 @@ void kvm_mmu_after_set_cpuid(struct kvm_vcpu *vcpu)
void kvm_mmu_reset_context(struct kvm_vcpu *vcpu)
{
- kvm_mmu_unload(vcpu);
kvm_init_mmu(vcpu);
+ kvm_mmu_new_pgd(vcpu, vcpu->arch.cr3);
This is too risky IMO, there are far more flows than just MOV CR0/CR4 that are
affected, e.g. SMM transitions, KVM_SET_SREG, etc...
SMM exit does flush the TLB because RSM clears CR0.PG (I did check this
:)). SMM re-entry then does not need to flush. But I don't think SMM
exit should flush the TLB *for non-SMM roles*.
For KVM_SET_SREGS I'm not sure if it should flush the TLB, but I agree
it is certainly safer to keep it that way.
Given that kvm_post_set_cr{0,4}() and kvm_vcpu_reset() explicitly handle CR0.PG
and CR4.SMEP toggling, I highly doubt the other flows are correct in all instances.
The call to kvm_mmu_new_pgd() is also
*white noise*
To minimize risk, we should leave kvm_mmu_reset_context() as is (rename it if
necessary) and instead add a new helper to handle kvm_post_set_cr{0,4}(). In
the future we can/should work on avoiding unload in all paths, but again, future
problem.
I disagree on this. There aren't many calls to kvm_mmu_reset_context.
- if ((cr0 ^ old_cr0) & KVM_MMU_CR0_ROLE_BITS)
+ if ((cr0 ^ old_cr0) & KVM_MMU_CR0_ROLE_BITS) {
+ /* Flush the TLB if CR0 is changed 1 -> 0. */
+ if ((old_cr0 & X86_CR0_PG) && !(cr0 & X86_CR0_PG))
+ kvm_mmu_unload(vcpu);
Calling kvm_mmu_unload() instead of requesting a flush isn't coherent with respect
to the comment, or with SMEP handling. And the SMEP handling isn't coherent with
respect to the changelog. Please elaborate :-)
Yep, will do (the CR0.PG=0 case is similar to the CR0.PCIDE=0 case
below). Using kvm_mmu_unload() avoids loading a cached root just to
throw it away immediately after, but I can change this to a new
KVM_REQ_MMU_UPDATE_ROOT flag that does
kvm_mmu_new_pgd(vcpu, vcpu->arch.cr3);
By the way, I have a possibly stupid question. In kvm_set_cr3 (called
e.g. from emulator_set_cr()) there is
if (cr3 != kvm_read_cr3(vcpu))
kvm_mmu_new_pgd(vcpu, cr3);
What makes this work if mmu_is_nested(vcpu)? Should this also have an
"if (... & !tdp_enabled)"?
- else if ((cr4 ^ old_cr4) & X86_CR4_PCIDE)
+ if ((cr4 & X86_CR4_SMEP) && !(old_cr4 & X86_CR4_SMEP))
+ kvm_make_request(KVM_REQ_TLB_FLUSH_GUEST, vcpu);
This mishandles CR4.PGE. Per the comment above, the if-elif-elif sequence relies
on kvm_mmu_reset_context being a superset of KVM_REQ_TLB_FLUSH_GUEST.
For both CR0 and CR4, I think we should disassociate the TLB flush logic from the
MMU role logic, e.g. CR4.PGE _could_ be part of the role, but it's not because KVM
doesn't emulate global pages.
Makes sense, yes. It also needs to handle flushing the current PCID
when changing CR4.PAE (previously done for "free" by
kvm_mmu_reset_context), but I agree with the idea.
Paolo
This is what I'm thinking, assuming CR0.PG 1=>0 really only needs a flush.
---
arch/x86/kvm/mmu/mmu.c | 4 ++--
arch/x86/kvm/x86.c | 42 +++++++++++++++++++++++++++++-------------
2 files changed, 31 insertions(+), 15 deletions(-)
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index e41834748d52..c477c519c784 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -5041,8 +5041,8 @@ EXPORT_SYMBOL_GPL(kvm_init_mmu);
void kvm_mmu_after_set_cpuid(struct kvm_vcpu *vcpu)
{
/*
- * Invalidate all MMU roles to force them to reinitialize as CPUID
- * information is factored into reserved bit calculations.
+ * Invalidate all MMU roles and roots to force them to reinitialize,
+ * as CPUID information is factored into reserved bit calculations.
*
* Correctly handling multiple vCPU models with respect to paging and
* physical address properties) in a single VM would require tracking
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 782dc9cd31d8..b8dad04301ee 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -863,15 +863,28 @@ int load_pdptrs(struct kvm_vcpu *vcpu, unsigned long cr3)
}
EXPORT_SYMBOL_GPL(load_pdptrs);
+static void kvm_post_set_cr_reinit_mmu(struct kvm_vcpu *vcpu)
+{
+ kvm_mmu_init(vcpu);
+ kvm_mmu_new_pgd(vcpu, vcpu->arch.cr3);
+}
+
void kvm_post_set_cr0(struct kvm_vcpu *vcpu, unsigned long old_cr0, unsigned long cr0)
{
if ((cr0 ^ old_cr0) & X86_CR0_PG) {
kvm_clear_async_pf_completion_queue(vcpu);
kvm_async_pf_hash_reset(vcpu);
+
+ /*
+ * Clearing CR0.PG is architecturally defined as flushing the
+ * TLB from the guest's perspective.
+ */
+ if (!(cr0 & X86_CR0_PG))
+ kvm_make_request(KVM_REQ_TLB_FLUSH_GUEST, vcpu);
}
if ((cr0 ^ old_cr0) & KVM_MMU_CR0_ROLE_BITS)
- kvm_mmu_reset_context(vcpu);
+ kvm_post_set_cr_reinit_mmu(vcpu);
if (((cr0 ^ old_cr0) & X86_CR0_CD) &&
kvm_arch_has_noncoherent_dma(vcpu->kvm) &&
@@ -1055,26 +1068,29 @@ EXPORT_SYMBOL_GPL(kvm_is_valid_cr4);
void kvm_post_set_cr4(struct kvm_vcpu *vcpu, unsigned long old_cr4, unsigned long cr4)
{
/*
- * If any role bit is changed, the MMU needs to be reset.
- *
* If CR4.PCIDE is changed 1 -> 0, the guest TLB must be flushed.
* If CR4.PCIDE is changed 0 -> 1, there is no need to flush the TLB
* according to the SDM; however, stale prev_roots could be reused
* incorrectly in the future after a MOV to CR3 with NOFLUSH=1, so we
* free them all. KVM_REQ_MMU_RELOAD is fit for the both cases; it
* is slow, but changing CR4.PCIDE is a rare case.
- *
- * If CR4.PGE is changed, the guest TLB must be flushed.
- *
- * Note: resetting MMU is a superset of KVM_REQ_MMU_RELOAD and
- * KVM_REQ_MMU_RELOAD is a superset of KVM_REQ_TLB_FLUSH_GUEST, hence
- * the usage of "else if".
*/
- if ((cr4 ^ old_cr4) & KVM_MMU_CR4_ROLE_BITS)
- kvm_mmu_reset_context(vcpu);
- else if ((cr4 ^ old_cr4) & X86_CR4_PCIDE)
+ if ((cr4 ^ old_cr4) & X86_CR4_PCIDE) {
kvm_make_request(KVM_REQ_MMU_RELOAD, vcpu);
- else if ((cr4 ^ old_cr4) & X86_CR4_PGE)
+ return;
+ }
+
+ /* If any role bit is changed, the MMU needs to be reinitialized. */
+ if ((cr4 ^ old_cr4) & KVM_MMU_CR4_ROLE_BITS)
+ kvm_post_set_cr_reinit_mmu(vcpu);
+
+ /*
+ * Setting SMEP or toggling PGE is architecturally defined as flushing
+ * the TLB from the guest's perspective. Note, because the shadow MMU
+ * ignores global pages, CR4.PGE is not part of KVM_MMU_CR4_ROLE_BITS.
+ */
+ if (((cr4 ^ old_cr4) & X86_CR4_PGE) ||
+ ((cr4 & X86_CR4_SMEP) && !(old_cr4 & X86_CR4_SMEP)))
kvm_make_request(KVM_REQ_TLB_FLUSH_GUEST, vcpu);
}
EXPORT_SYMBOL_GPL(kvm_post_set_cr4);
base-commit: a8c36d04d70d0b15e696561e1a2134fcbdd3a3bd
--