| 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
| ||
|
Message-Id: <20220209170020.1775368-13-pbonzini@redhat.com>
Date: Wed, 9 Feb 2022 12:00:20 -0500
From: Paolo Bonzini <pbonzini@...hat.com>
To: linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Cc: vkuznets@...hat.com, mlevitsk@...hat.com, dmatlack@...gle.com,
seanjc@...gle.com
Subject: [PATCH 12/12] KVM: x86: do not unload MMU roots on all role changes
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@...ecurity.com>
Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
---
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);
}
EXPORT_SYMBOL_GPL(kvm_mmu_reset_context);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 0d3646535cc5..97c4f5fc291f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -873,8 +873,12 @@ void kvm_post_set_cr0(struct kvm_vcpu *vcpu, unsigned long old_cr0, unsigned lon
kvm_async_pf_hash_reset(vcpu);
}
- 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);
kvm_mmu_reset_context(vcpu);
+ }
if (((cr0 ^ old_cr0) & X86_CR0_CD) &&
kvm_arch_has_noncoherent_dma(vcpu->kvm) &&
@@ -1067,15 +1071,18 @@ void kvm_post_set_cr4(struct kvm_vcpu *vcpu, unsigned long old_cr4, unsigned lon
* 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.
+ * Setting SMEP also needs to flush the TLB, in addition to resetting
+ * the MMU.
*
- * 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.PGE is changed, the guest TLB must be flushed. Because
+ * the shadow MMU ignores global pages, this bit is not part of
+ * KVM_MMU_CR4_ROLE_BITS.
*/
- if ((cr4 ^ old_cr4) & KVM_MMU_CR4_ROLE_BITS)
+ if ((cr4 ^ old_cr4) & KVM_MMU_CR4_ROLE_BITS) {
kvm_mmu_reset_context(vcpu);
- 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);
+ } else if ((cr4 ^ old_cr4) & X86_CR4_PCIDE)
kvm_make_request(KVM_REQ_MMU_RELOAD, vcpu);
else if ((cr4 ^ old_cr4) & X86_CR4_PGE)
kvm_make_request(KVM_REQ_TLB_FLUSH_GUEST, vcpu);
@@ -11329,8 +11336,10 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
* paging related bits are ignored if paging is disabled, i.e. CR0.WP,
* CR4, and EFER changes are all irrelevant if CR0.PG was '0'.
*/
- if (old_cr0 & X86_CR0_PG)
- kvm_mmu_reset_context(vcpu);
+ if (old_cr0 & X86_CR0_PG) {
+ kvm_mmu_unload(vcpu);
+ kvm_init_mmu(vcpu);
+ }
/*
* Intel's SDM states that all TLB entries are flushed on INIT. AMD's
--
2.31.1
Powered by blists - more mailing lists