v3: https://lore.kernel.org/kvm/[email protected]/
This series is the fourth iteration of resurrecting the missing pieces of
Paolo's previous attempt[1] to avoid needless MMU roots unloading.
It's incorporating Sean's feedback to v3 and rebased on top of
kvm-x86/next, namely commit d8708b80fa0e ("KVM: Change return type of
kvm_arch_vm_ioctl() to "int"").
The performance gap between TDP and legacy MMU is still existent,
especially noticeable under grsecurity which implements kernel W^X by
toggling CR0.WP, which happens very frequently. This series tries to fix
this needless performance loss.
Patch 1 is a v3 of [3], addressing Sean's feedback.
Patch 2 implements Sean's feedback[2] to Paolo's original approach and
skips unloading the MMU roots for CR0.WP toggling under TDP.
Patch 3 further micro-optimizes this for non-paging guests -- anyone still
running MS Singularity? ;)
Sean was suggesting another change on top of v2 of this series (and then
again a more elaborate version on top of v3), to skip intercepting CR0.WP
writes completely for VMX[4]. That turned out to be yet another
performance boost and is implemented in patch 6.
Patches 2 and 6 are the most important ones, as they bring the big
performance gains.
I used 'ssdd 10 50000' from rt-tests[5] as a micro-benchmark, running on a
grsecurity L1 VM. Below table shows the results (runtime in seconds, lower
is better):
legacy TDP shadow
kvm-x86/next@d8708b 8.43s 9.45s 70.3s
+ patches 1-3 5.39s 5.63s 70.2s
+ patches 4-6 3.51s 3.47s 67.8s
I re-ran the benchmarks (again) and the numbers changed a little from the
ones in v3. We got a better baseline which is likely caused by the rebase
to a v6.3-rc2 based tree (was v6.2-rc3 based before).
Patches 1, 4 and 5 didn't change from v3, beside minor changelog tweaks.
Patches 2 and 6 have been rewritten.
Patch 3 is new to this series.
Bonus rant^Wbug report:
Actually re-running the benchmarks took me a while because my VM was
constantly crashing on me with a #GP during scheduling. Looking a little
closer, I noticed it was for a IA32_PRED_CMD MSR write which was puzzling,
as the VM's kernel didn't change for my tests (built it more than a month
ago!), so the old test runs should have triggered that code path (and #GP)
too! Digging through some kernel code let me see it's all tied to the x86
feature flag X86_FEATURE_USE_IBPB which gets set when X86_FEATURE_IBPB is,
i.e. the CPU supports IBPB.
*head-scratching pause*
Foolish me probably did a system update of the host and got a microcode
update that added IBPB support to my CPU. Yayyy... NOT! As this implies
announcing IBPB support to the VM as well and, in turn, makes the guest
kernel try to use it, I'm doomed to hit that bug. *bummer*
Something must not be right with KVM / QEMU / the kernel, as this guest
behaviour clearly shouldn't cause KVM to inject a #GP into the guest.
The bugging call chain in the guest kernel is:
-> switch_mm_irqs_off()
-> cond_mitigation()
-> indirect_branch_prediction_barrier()
-> alternative_msr_write(MSR_IA32_PRED_CMD, val, X86_FEATURE_USE_IBPB))
So far I'm working around this by passing 'clearcpuid=ibpb' on the kernel
commandline. But this should probably be fixed, that's why I'm mentioning
it. It's just too late here already to debug this further today. Well, for
some definition of "today."
Thanks,
Mathias
[1] https://lore.kernel.org/kvm/[email protected]/
[2] https://lore.kernel.org/kvm/YhATewkkO%[email protected]/
[3] https://lore.kernel.org/kvm/YhAB1d1%[email protected]/
[4] https://lore.kernel.org/kvm/[email protected]/
[5] https://git.kernel.org/pub/scm/utils/rt-tests/rt-tests.git
Mathias Krause (5):
KVM: x86: Do not unload MMU roots when only toggling CR0.WP with TDP
enabled
KVM: x86: Ignore CR0.WP toggles in non-paging mode
KVM: x86: Make use of kvm_read_cr*_bits() when testing bits
KVM: x86/mmu: Fix comment typo
KVM: VMX: Make CR0.WP a guest owned bit
Paolo Bonzini (1):
KVM: x86/mmu: Avoid indirect call for get_cr3
arch/x86/kvm/kvm_cache_regs.h | 2 +-
arch/x86/kvm/mmu/mmu.c | 31 ++++++++++++++++++++-----------
arch/x86/kvm/mmu/paging_tmpl.h | 2 +-
arch/x86/kvm/mmu/spte.c | 2 +-
arch/x86/kvm/pmu.c | 4 ++--
arch/x86/kvm/vmx/nested.c | 4 ++--
arch/x86/kvm/vmx/vmx.c | 6 +++---
arch/x86/kvm/vmx/vmx.h | 18 ++++++++++++++++++
arch/x86/kvm/x86.c | 18 ++++++++++++++++++
9 files changed, 66 insertions(+), 21 deletions(-)
--
2.39.2
There is no need to unload the MMU roots with TDP enabled when only
CR0.WP has changed -- the paging structures are still valid, only the
permission bitmap needs to be updated.
One heavy user of toggling CR0.WP is grsecurity's KERNEXEC feature to
implement kernel W^X.
The optimization brings a huge performance gain for this case as the
following micro-benchmark running 'ssdd 10 50000' from rt-tests[1] on a
grsecurity L1 VM shows (runtime in seconds, lower is better):
legacy TDP shadow
kvm-x86/next@d8708b 8.43s 9.45s 70.3s
+patch 5.39s 5.63s 70.2s
For legacy MMU this is ~36% faster, for TTP MMU even ~40% faster. Also
TDP and legacy MMU now both have a similar runtime which vanishes the
need to disable TDP MMU for grsecurity.
Shadow MMU sees no measurable difference and is still slow, as expected.
[1] https://git.kernel.org/pub/scm/utils/rt-tests/rt-tests.git
Co-developed-by: Sean Christopherson <[email protected]>
Signed-off-by: Mathias Krause <[email protected]>
---
arch/x86/kvm/x86.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 237c483b1230..c6d909778b2c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -906,6 +906,18 @@ EXPORT_SYMBOL_GPL(load_pdptrs);
void kvm_post_set_cr0(struct kvm_vcpu *vcpu, unsigned long old_cr0, unsigned long cr0)
{
+ /*
+ * CR0.WP is incorporated into the MMU role, but only for non-nested,
+ * indirect shadow MMUs. If TDP is enabled, the MMU's metadata needs
+ * to be updated, e.g. so that emulating guest translations does the
+ * right thing, but there's no need to unload the root as CR0.WP
+ * doesn't affect SPTEs.
+ */
+ if (tdp_enabled && (cr0 ^ old_cr0) == X86_CR0_WP) {
+ kvm_init_mmu(vcpu);
+ return;
+ }
+
if ((cr0 ^ old_cr0) & X86_CR0_PG) {
kvm_clear_async_pf_completion_queue(vcpu);
kvm_async_pf_hash_reset(vcpu);
--
2.39.2
Make use of the kvm_read_cr{0,4}_bits() helper functions when we only
want to know the state of certain bits instead of the whole register.
This not only makes the intent cleaner, it also avoids a potential
VMREAD in case the tested bits aren't guest owned.
Signed-off-by: Mathias Krause <[email protected]>
---
arch/x86/kvm/pmu.c | 4 ++--
arch/x86/kvm/vmx/vmx.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
index 612e6c70ce2e..f4aa170b5b97 100644
--- a/arch/x86/kvm/pmu.c
+++ b/arch/x86/kvm/pmu.c
@@ -540,9 +540,9 @@ int kvm_pmu_rdpmc(struct kvm_vcpu *vcpu, unsigned idx, u64 *data)
if (!pmc)
return 1;
- if (!(kvm_read_cr4(vcpu) & X86_CR4_PCE) &&
+ if (!(kvm_read_cr4_bits(vcpu, X86_CR4_PCE)) &&
(static_call(kvm_x86_get_cpl)(vcpu) != 0) &&
- (kvm_read_cr0(vcpu) & X86_CR0_PE))
+ (kvm_read_cr0_bits(vcpu, X86_CR0_PE)))
return 1;
*data = pmc_read_counter(pmc) & mask;
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index d7bf14abdba1..8fc1a0c7856f 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5517,7 +5517,7 @@ static int handle_cr(struct kvm_vcpu *vcpu)
break;
case 3: /* lmsw */
val = (exit_qualification >> LMSW_SOURCE_DATA_SHIFT) & 0x0f;
- trace_kvm_cr_write(0, (kvm_read_cr0(vcpu) & ~0xful) | val);
+ trace_kvm_cr_write(0, (kvm_read_cr0_bits(vcpu, ~0xful) | val));
kvm_lmsw(vcpu, val);
return kvm_skip_emulated_instruction(vcpu);
@@ -7575,7 +7575,7 @@ static u8 vmx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
if (!kvm_arch_has_noncoherent_dma(vcpu->kvm))
return (MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT) | VMX_EPT_IPAT_BIT;
- if (kvm_read_cr0(vcpu) & X86_CR0_CD) {
+ if (kvm_read_cr0_bits(vcpu, X86_CR0_CD)) {
if (kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_CD_NW_CLEARED))
cache = MTRR_TYPE_WRBACK;
else
--
2.39.2
From: Paolo Bonzini <[email protected]>
Most of the time, calls to get_guest_pgd result in calling
kvm_read_cr3 (the exception is only nested TDP). Hardcode
the default instead of using the get_cr3 function, avoiding
a retpoline if they are enabled.
Signed-off-by: Paolo Bonzini <[email protected]>
Signed-off-by: Mathias Krause <[email protected]>
---
arch/x86/kvm/mmu/mmu.c | 31 ++++++++++++++++++++-----------
arch/x86/kvm/mmu/paging_tmpl.h | 2 +-
2 files changed, 21 insertions(+), 12 deletions(-)
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 144c5a01cd77..9046a892998e 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -242,6 +242,20 @@ static struct kvm_mmu_role_regs vcpu_to_role_regs(struct kvm_vcpu *vcpu)
return regs;
}
+static unsigned long get_guest_cr3(struct kvm_vcpu *vcpu)
+{
+ return kvm_read_cr3(vcpu);
+}
+
+static inline unsigned long kvm_mmu_get_guest_pgd(struct kvm_vcpu *vcpu,
+ struct kvm_mmu *mmu)
+{
+ if (IS_ENABLED(CONFIG_RETPOLINE) && mmu->get_guest_pgd == get_guest_cr3)
+ return kvm_read_cr3(vcpu);
+
+ return mmu->get_guest_pgd(vcpu);
+}
+
static inline bool kvm_available_flush_tlb_with_range(void)
{
return kvm_x86_ops.tlb_remote_flush_with_range;
@@ -3731,7 +3745,7 @@ static int mmu_alloc_shadow_roots(struct kvm_vcpu *vcpu)
int quadrant, i, r;
hpa_t root;
- root_pgd = mmu->get_guest_pgd(vcpu);
+ root_pgd = kvm_mmu_get_guest_pgd(vcpu, mmu);
root_gfn = root_pgd >> PAGE_SHIFT;
if (mmu_check_root(vcpu, root_gfn))
@@ -4181,7 +4195,7 @@ static bool kvm_arch_setup_async_pf(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
arch.token = alloc_apf_token(vcpu);
arch.gfn = gfn;
arch.direct_map = vcpu->arch.mmu->root_role.direct;
- arch.cr3 = vcpu->arch.mmu->get_guest_pgd(vcpu);
+ arch.cr3 = kvm_mmu_get_guest_pgd(vcpu, vcpu->arch.mmu);
return kvm_setup_async_pf(vcpu, cr2_or_gpa,
kvm_vcpu_gfn_to_hva(vcpu, gfn), &arch);
@@ -4200,7 +4214,7 @@ void kvm_arch_async_page_ready(struct kvm_vcpu *vcpu, struct kvm_async_pf *work)
return;
if (!vcpu->arch.mmu->root_role.direct &&
- work->arch.cr3 != vcpu->arch.mmu->get_guest_pgd(vcpu))
+ work->arch.cr3 != kvm_mmu_get_guest_pgd(vcpu, vcpu->arch.mmu))
return;
kvm_mmu_do_page_fault(vcpu, work->cr2_or_gpa, 0, true, NULL);
@@ -4604,11 +4618,6 @@ void kvm_mmu_new_pgd(struct kvm_vcpu *vcpu, gpa_t new_pgd)
}
EXPORT_SYMBOL_GPL(kvm_mmu_new_pgd);
-static unsigned long get_cr3(struct kvm_vcpu *vcpu)
-{
- return kvm_read_cr3(vcpu);
-}
-
static bool sync_mmio_spte(struct kvm_vcpu *vcpu, u64 *sptep, gfn_t gfn,
unsigned int access)
{
@@ -5159,7 +5168,7 @@ static void init_kvm_tdp_mmu(struct kvm_vcpu *vcpu,
context->page_fault = kvm_tdp_page_fault;
context->sync_page = nonpaging_sync_page;
context->invlpg = NULL;
- context->get_guest_pgd = get_cr3;
+ context->get_guest_pgd = get_guest_cr3;
context->get_pdptr = kvm_pdptr_read;
context->inject_page_fault = kvm_inject_page_fault;
@@ -5309,7 +5318,7 @@ static void init_kvm_softmmu(struct kvm_vcpu *vcpu,
kvm_init_shadow_mmu(vcpu, cpu_role);
- context->get_guest_pgd = get_cr3;
+ context->get_guest_pgd = get_guest_cr3;
context->get_pdptr = kvm_pdptr_read;
context->inject_page_fault = kvm_inject_page_fault;
}
@@ -5323,7 +5332,7 @@ static void init_kvm_nested_mmu(struct kvm_vcpu *vcpu,
return;
g_context->cpu_role.as_u64 = new_mode.as_u64;
- g_context->get_guest_pgd = get_cr3;
+ g_context->get_guest_pgd = get_guest_cr3;
g_context->get_pdptr = kvm_pdptr_read;
g_context->inject_page_fault = kvm_inject_page_fault;
diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h
index a056f2773dd9..8417ecbc3887 100644
--- a/arch/x86/kvm/mmu/paging_tmpl.h
+++ b/arch/x86/kvm/mmu/paging_tmpl.h
@@ -324,7 +324,7 @@ static int FNAME(walk_addr_generic)(struct guest_walker *walker,
trace_kvm_mmu_pagetable_walk(addr, access);
retry_walk:
walker->level = mmu->cpu_role.base.level;
- pte = mmu->get_guest_pgd(vcpu);
+ pte = kvm_mmu_get_guest_pgd(vcpu, mmu);
have_ad = PT_HAVE_ACCESSED_DIRTY(mmu);
#if PTTYPE == 64
--
2.39.2
If paging is disabled, there are no permission bits to emulate.
Micro-optimize this case to avoid unnecessary work.
Suggested-and-co-developed-by: Sean Christopherson <[email protected]>
Signed-off-by: Mathias Krause <[email protected]>
---
arch/x86/kvm/x86.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c6d909778b2c..8a66ac7a4878 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -908,14 +908,20 @@ void kvm_post_set_cr0(struct kvm_vcpu *vcpu, unsigned long old_cr0, unsigned lon
{
/*
* CR0.WP is incorporated into the MMU role, but only for non-nested,
- * indirect shadow MMUs. If TDP is enabled, the MMU's metadata needs
- * to be updated, e.g. so that emulating guest translations does the
- * right thing, but there's no need to unload the root as CR0.WP
- * doesn't affect SPTEs.
+ * indirect shadow MMUs. If paging is disabled, no updates are needed
+ * as there are no permission bits to emulate. If TDP is enabled, the
+ * MMU's metadata needs to be updated, e.g. so that emulating guest
+ * translations does the right thing, but there's no need to unload the
+ * root as CR0.WP doesn't affect SPTEs.
*/
- if (tdp_enabled && (cr0 ^ old_cr0) == X86_CR0_WP) {
- kvm_init_mmu(vcpu);
- return;
+ if ((cr0 ^ old_cr0) == X86_CR0_WP) {
+ if (!(cr0 & X86_CR0_PG))
+ return;
+
+ if (tdp_enabled) {
+ kvm_init_mmu(vcpu);
+ return;
+ }
}
if ((cr0 ^ old_cr0) & X86_CR0_PG) {
--
2.39.2
Fix a small comment typo in make_spte().
Signed-off-by: Mathias Krause <[email protected]>
---
arch/x86/kvm/mmu/spte.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kvm/mmu/spte.c b/arch/x86/kvm/mmu/spte.c
index c15bfca3ed15..cf2c6426a6fc 100644
--- a/arch/x86/kvm/mmu/spte.c
+++ b/arch/x86/kvm/mmu/spte.c
@@ -164,7 +164,7 @@ bool make_spte(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
/*
* For simplicity, enforce the NX huge page mitigation even if not
* strictly necessary. KVM could ignore the mitigation if paging is
- * disabled in the guest, as the guest doesn't have an page tables to
+ * disabled in the guest, as the guest doesn't have any page tables to
* abuse. But to safely ignore the mitigation, KVM would have to
* ensure a new MMU is loaded (or all shadow pages zapped) when CR0.PG
* is toggled on, and that's a net negative for performance when TDP is
--
2.39.2
Guests like grsecurity that make heavy use of CR0.WP to implement kernel
level W^X will suffer from the implied VMEXITs.
With EPT there is no need to intercept a guest change of CR0.WP, so
simply make it a guest owned bit if we can do so.
This implies that a read of a guest's CR0.WP bit might need a VMREAD.
However, the only potentially affected user seems to be kvm_init_mmu()
which is a heavy operation to begin with. But also most callers already
cache the full value of CR0 anyway, so no additional VMREAD is needed.
The only exception is nested_vmx_load_cr3().
This change is VMX-specific, as SVM has no such fine grained control
register intercept control.
Suggested-and-co-developed-by: Sean Christopherson <[email protected]>
Signed-off-by: Mathias Krause <[email protected]>
---
arch/x86/kvm/kvm_cache_regs.h | 2 +-
arch/x86/kvm/vmx/nested.c | 4 ++--
arch/x86/kvm/vmx/vmx.c | 2 +-
arch/x86/kvm/vmx/vmx.h | 18 ++++++++++++++++++
4 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h
index 4c91f626c058..e50d353b5c1c 100644
--- a/arch/x86/kvm/kvm_cache_regs.h
+++ b/arch/x86/kvm/kvm_cache_regs.h
@@ -4,7 +4,7 @@
#include <linux/kvm_host.h>
-#define KVM_POSSIBLE_CR0_GUEST_BITS X86_CR0_TS
+#define KVM_POSSIBLE_CR0_GUEST_BITS (X86_CR0_TS | X86_CR0_WP)
#define KVM_POSSIBLE_CR4_GUEST_BITS \
(X86_CR4_PVI | X86_CR4_DE | X86_CR4_PCE | X86_CR4_OSFXSR \
| X86_CR4_OSXMMEXCPT | X86_CR4_PGE | X86_CR4_TSD | X86_CR4_FSGSBASE)
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index f63b28f46a71..61d940fc91ba 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -4481,7 +4481,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
* CR0_GUEST_HOST_MASK is already set in the original vmcs01
* (KVM doesn't change it);
*/
- vcpu->arch.cr0_guest_owned_bits = KVM_POSSIBLE_CR0_GUEST_BITS;
+ vcpu->arch.cr0_guest_owned_bits = vmx_l1_guest_owned_cr0_bits();
vmx_set_cr0(vcpu, vmcs12->host_cr0);
/* Same as above - no reason to call set_cr4_guest_host_mask(). */
@@ -4632,7 +4632,7 @@ static void nested_vmx_restore_host_state(struct kvm_vcpu *vcpu)
*/
vmx_set_efer(vcpu, nested_vmx_get_vmcs01_guest_efer(vmx));
- vcpu->arch.cr0_guest_owned_bits = KVM_POSSIBLE_CR0_GUEST_BITS;
+ vcpu->arch.cr0_guest_owned_bits = vmx_l1_guest_owned_cr0_bits();
vmx_set_cr0(vcpu, vmcs_readl(CR0_READ_SHADOW));
vcpu->arch.cr4_guest_owned_bits = ~vmcs_readl(CR4_GUEST_HOST_MASK);
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 8fc1a0c7856f..e501f6864a72 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4790,7 +4790,7 @@ static void init_vmcs(struct vcpu_vmx *vmx)
/* 22.2.1, 20.8.1 */
vm_entry_controls_set(vmx, vmx_vmentry_ctrl());
- vmx->vcpu.arch.cr0_guest_owned_bits = KVM_POSSIBLE_CR0_GUEST_BITS;
+ vmx->vcpu.arch.cr0_guest_owned_bits = vmx_l1_guest_owned_cr0_bits();
vmcs_writel(CR0_GUEST_HOST_MASK, ~vmx->vcpu.arch.cr0_guest_owned_bits);
set_cr4_guest_host_mask(vmx);
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index 2acdc54bc34b..423e9d3c9c40 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -640,6 +640,24 @@ BUILD_CONTROLS_SHADOW(tertiary_exec, TERTIARY_VM_EXEC_CONTROL, 64)
(1 << VCPU_EXREG_EXIT_INFO_1) | \
(1 << VCPU_EXREG_EXIT_INFO_2))
+static inline unsigned long vmx_l1_guest_owned_cr0_bits(void)
+{
+ unsigned long bits = KVM_POSSIBLE_CR0_GUEST_BITS;
+
+ /*
+ * CR0.WP needs to be intercepted when KVM is shadowing legacy paging
+ * in order to construct shadow PTEs with the correct protections.
+ * Note! CR0.WP technically can be passed through to the guest if
+ * paging is disabled, but checking CR0.PG would generate a cyclical
+ * dependency of sorts due to forcing the caller to ensure CR0 holds
+ * the correct value prior to determining which CR0 bits can be owned
+ * by L1. Keep it simple and limit the optimization to EPT.
+ */
+ if (!enable_ept)
+ bits &= ~X86_CR0_WP;
+ return bits;
+}
+
static __always_inline struct kvm_vmx *to_kvm_vmx(struct kvm *kvm)
{
return container_of(kvm, struct kvm_vmx, kvm);
--
2.39.2
On 22.03.23 02:37, Mathias Krause wrote:
> Bonus rant^Wbug report:
> [VMs #GP'ing on WRMSR(IA32_PRED_CMD,...)]
I see that this is already a well known bug, fully analyzed and taken
care off:
https://lore.kernel.org/kvm/[email protected]/
Thanks,
Mathias
On Wed, 22 Mar 2023 02:37:25 +0100, Mathias Krause wrote:
> v3: https://lore.kernel.org/kvm/[email protected]/
>
> This series is the fourth iteration of resurrecting the missing pieces of
> Paolo's previous attempt[1] to avoid needless MMU roots unloading.
>
> It's incorporating Sean's feedback to v3 and rebased on top of
> kvm-x86/next, namely commit d8708b80fa0e ("KVM: Change return type of
> kvm_arch_vm_ioctl() to "int"").
>
> [...]
Applied 1 and 5 to kvm-x86 mmu, and the rest to kvm-x86 misc, thanks!
[1/6] KVM: x86/mmu: Avoid indirect call for get_cr3
https://github.com/kvm-x86/linux/commit/2fdcc1b32418
[2/6] KVM: x86: Do not unload MMU roots when only toggling CR0.WP with TDP enabled
https://github.com/kvm-x86/linux/commit/01b31714bd90
[3/6] KVM: x86: Ignore CR0.WP toggles in non-paging mode
https://github.com/kvm-x86/linux/commit/e40bcf9f3a18
[4/6] KVM: x86: Make use of kvm_read_cr*_bits() when testing bits
https://github.com/kvm-x86/linux/commit/74cdc836919b
[5/6] KVM: x86/mmu: Fix comment typo
https://github.com/kvm-x86/linux/commit/50f13998451e
[6/6] KVM: VMX: Make CR0.WP a guest owned bit
https://github.com/kvm-x86/linux/commit/fb509f76acc8
--
https://github.com/kvm-x86/linux/tree/next
https://github.com/kvm-x86/linux/tree/fixes
On 3/22/2023 9:37 AM, Mathias Krause wrote:
> Guests like grsecurity that make heavy use of CR0.WP to implement kernel
> level W^X will suffer from the implied VMEXITs.
>
> With EPT there is no need to intercept a guest change of CR0.WP, so
> simply make it a guest owned bit if we can do so.
I'm interested in the performance gain. Do you have data like Patch 2?
> This implies that a read of a guest's CR0.WP bit might need a VMREAD.
> However, the only potentially affected user seems to be kvm_init_mmu()
> which is a heavy operation to begin with. But also most callers already
> cache the full value of CR0 anyway, so no additional VMREAD is needed.
> The only exception is nested_vmx_load_cr3().
>
> This change is VMX-specific, as SVM has no such fine grained control
> register intercept control.
>
> Suggested-and-co-developed-by: Sean Christopherson <[email protected]>
> Signed-off-by: Mathias Krause <[email protected]>
> ---
> arch/x86/kvm/kvm_cache_regs.h | 2 +-
> arch/x86/kvm/vmx/nested.c | 4 ++--
> arch/x86/kvm/vmx/vmx.c | 2 +-
> arch/x86/kvm/vmx/vmx.h | 18 ++++++++++++++++++
> 4 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h
> index 4c91f626c058..e50d353b5c1c 100644
> --- a/arch/x86/kvm/kvm_cache_regs.h
> +++ b/arch/x86/kvm/kvm_cache_regs.h
> @@ -4,7 +4,7 @@
>
> #include <linux/kvm_host.h>
>
> -#define KVM_POSSIBLE_CR0_GUEST_BITS X86_CR0_TS
> +#define KVM_POSSIBLE_CR0_GUEST_BITS (X86_CR0_TS | X86_CR0_WP)
> #define KVM_POSSIBLE_CR4_GUEST_BITS \
> (X86_CR4_PVI | X86_CR4_DE | X86_CR4_PCE | X86_CR4_OSFXSR \
> | X86_CR4_OSXMMEXCPT | X86_CR4_PGE | X86_CR4_TSD | X86_CR4_FSGSBASE)
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index f63b28f46a71..61d940fc91ba 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -4481,7 +4481,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
> * CR0_GUEST_HOST_MASK is already set in the original vmcs01
> * (KVM doesn't change it);
> */
> - vcpu->arch.cr0_guest_owned_bits = KVM_POSSIBLE_CR0_GUEST_BITS;
> + vcpu->arch.cr0_guest_owned_bits = vmx_l1_guest_owned_cr0_bits();
> vmx_set_cr0(vcpu, vmcs12->host_cr0);
>
> /* Same as above - no reason to call set_cr4_guest_host_mask(). */
> @@ -4632,7 +4632,7 @@ static void nested_vmx_restore_host_state(struct kvm_vcpu *vcpu)
> */
> vmx_set_efer(vcpu, nested_vmx_get_vmcs01_guest_efer(vmx));
>
> - vcpu->arch.cr0_guest_owned_bits = KVM_POSSIBLE_CR0_GUEST_BITS;
> + vcpu->arch.cr0_guest_owned_bits = vmx_l1_guest_owned_cr0_bits();
> vmx_set_cr0(vcpu, vmcs_readl(CR0_READ_SHADOW));
>
> vcpu->arch.cr4_guest_owned_bits = ~vmcs_readl(CR4_GUEST_HOST_MASK);
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 8fc1a0c7856f..e501f6864a72 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4790,7 +4790,7 @@ static void init_vmcs(struct vcpu_vmx *vmx)
> /* 22.2.1, 20.8.1 */
> vm_entry_controls_set(vmx, vmx_vmentry_ctrl());
>
> - vmx->vcpu.arch.cr0_guest_owned_bits = KVM_POSSIBLE_CR0_GUEST_BITS;
> + vmx->vcpu.arch.cr0_guest_owned_bits = vmx_l1_guest_owned_cr0_bits();
> vmcs_writel(CR0_GUEST_HOST_MASK, ~vmx->vcpu.arch.cr0_guest_owned_bits);
>
> set_cr4_guest_host_mask(vmx);
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index 2acdc54bc34b..423e9d3c9c40 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -640,6 +640,24 @@ BUILD_CONTROLS_SHADOW(tertiary_exec, TERTIARY_VM_EXEC_CONTROL, 64)
> (1 << VCPU_EXREG_EXIT_INFO_1) | \
> (1 << VCPU_EXREG_EXIT_INFO_2))
>
> +static inline unsigned long vmx_l1_guest_owned_cr0_bits(void)
> +{
> + unsigned long bits = KVM_POSSIBLE_CR0_GUEST_BITS;
> +
> + /*
> + * CR0.WP needs to be intercepted when KVM is shadowing legacy paging
> + * in order to construct shadow PTEs with the correct protections.
> + * Note! CR0.WP technically can be passed through to the guest if
> + * paging is disabled, but checking CR0.PG would generate a cyclical
> + * dependency of sorts due to forcing the caller to ensure CR0 holds
> + * the correct value prior to determining which CR0 bits can be owned
> + * by L1. Keep it simple and limit the optimization to EPT.
> + */
> + if (!enable_ept)
> + bits &= ~X86_CR0_WP;
> + return bits;
> +}
> +
> static __always_inline struct kvm_vmx *to_kvm_vmx(struct kvm *kvm)
> {
> return container_of(kvm, struct kvm_vmx, kvm);
On 27.03.23 10:33, Xiaoyao Li wrote:
> On 3/22/2023 9:37 AM, Mathias Krause wrote:
>> Guests like grsecurity that make heavy use of CR0.WP to implement kernel
>> level W^X will suffer from the implied VMEXITs.
>>
>> With EPT there is no need to intercept a guest change of CR0.WP, so
>> simply make it a guest owned bit if we can do so.
>
> I'm interested in the performance gain. Do you have data like Patch 2?
It's mentioned in the cover letter[1], quoted below:
[1]
https://lore.kernel.org/kvm/[email protected]/
: I used 'ssdd 10 50000' from rt-tests[5] as a micro-benchmark, running on a
: grsecurity L1 VM. Below table shows the results (runtime in seconds, lower
: is better):
:
: legacy TDP shadow
: kvm-x86/next@d8708b 8.43s 9.45s 70.3s
: + patches 1-3 5.39s 5.63s 70.2s
: + patches 4-6 3.51s 3.47s 67.8s
Thanks,
Mathias
On 3/27/2023 4:37 PM, Mathias Krause wrote:
> On 27.03.23 10:33, Xiaoyao Li wrote:
>> On 3/22/2023 9:37 AM, Mathias Krause wrote:
>>> Guests like grsecurity that make heavy use of CR0.WP to implement kernel
>>> level W^X will suffer from the implied VMEXITs.
>>>
>>> With EPT there is no need to intercept a guest change of CR0.WP, so
>>> simply make it a guest owned bit if we can do so.
>>
>> I'm interested in the performance gain. Do you have data like Patch 2?
>
> It's mentioned in the cover letter[1], quoted below:
Sorry I missed it. The data of not intercepting CR0.WP looks great as well.
> [1]
> https://lore.kernel.org/kvm/[email protected]/
>
> : I used 'ssdd 10 50000' from rt-tests[5] as a micro-benchmark, running on a
> : grsecurity L1 VM. Below table shows the results (runtime in seconds, lower
> : is better):
> :
> : legacy TDP shadow
> : kvm-x86/next@d8708b 8.43s 9.45s 70.3s
> : + patches 1-3 5.39s 5.63s 70.2s
> : + patches 4-6 3.51s 3.47s 67.8s
>
>
> Thanks,
> Mathias
On 22.03.23 02:37, Mathias Krause wrote:
> Guests like grsecurity that make heavy use of CR0.WP to implement kernel
> level W^X will suffer from the implied VMEXITs.
>
> With EPT there is no need to intercept a guest change of CR0.WP, so
> simply make it a guest owned bit if we can do so.
>
> This implies that a read of a guest's CR0.WP bit might need a VMREAD.
> However, the only potentially affected user seems to be kvm_init_mmu()
> which is a heavy operation to begin with. But also most callers already
> cache the full value of CR0 anyway, so no additional VMREAD is needed.
> The only exception is nested_vmx_load_cr3().
>
> This change is VMX-specific, as SVM has no such fine grained control
> register intercept control.
Just a heads up! We did more tests, especially with the backports we did
internally already, and ran into a bug when running a nested guest on an
ESXi host.
Setup is like: ESXi (L0) -> Linux (L1) -> Linux (L2)
The Linux system, especially the kernel, is the same for L1 and L2. It's
a grsecurity kernel, so makes use of toggling CR0.WP at runtime.
The bug we see is that when L2 disables CR0.WP and tries to write to an
r/o memory region (implicitly to the r/o GDT via LTR in our use case),
this triggers a fault (EPT violation?) that gets ignored by L1, as,
apparently, everything is fine from its point of view.
I suspect the L0 VMM to be at fault here, as the VMCS structures look
good, IMO. Here is a dump of vmx->loaded_vmcs in handle_triple_fault():
[…] VMX: TRIPLE FAULT!
[…] VMCS ffff8883a9f18000, last attempted VM-entry on CPU 8
[…] *** Guest State ***
[…] CR0: actual=0x0000000080040033, shadow=0x0000000080050033,
gh_mask=fffffffffffefff7
CR0 in the L2 VM has CR0.WP disabled. However, it is set in the shadow
CR0 but masked out via CR0_MASK, so should be read as the guest's value,
according to the SDM.
I also tried masking the shadow CR0 value in vmx_set_cr0(), but that
makes no difference.
[…] CR4: actual=0x00000000003220f0, shadow=0x00000000003208b0,
gh_mask=fffffffffffff871
[…] CR3 = 0x0000000002684000
[…] PDPTR0 = 0x0000000007d39001 PDPTR1 = 0x00000000033b5001
[…] PDPTR2 = 0x000000000238c001 PDPTR3 = 0x0000000001c54001
[…] RSP = 0xfffffe8040087f50 RIP = 0xffffffff8105d435
[…] RFLAGS=0x00010006 DR7 = 0x0000000000000400
[…] Sysenter RSP=0000000000000000 CS:RIP=0000:0000000000000000
[…] CS: sel=0x0010, attr=0x0a09b, limit=0xffffffff,
base=0x0000000000000000
[…] DS: sel=0x0000, attr=0x1c001, limit=0xffffffff,
base=0x0000000000000000
[…] SS: sel=0x0000, attr=0x1c001, limit=0xffffffff,
base=0x0000000000000000
[…] ES: sel=0x0000, attr=0x1c001, limit=0xffffffff,
base=0x0000000000000000
[…] FS: sel=0x0000, attr=0x1c001, limit=0xffffffff,
base=0x0000000000000000
[…] GS: sel=0x0000, attr=0x1c001, limit=0xffffffff,
base=0xffff888232e00000
[…] GDTR: limit=0x00000097,
base=0xfffffe0000201000
[…] LDTR: sel=0x0000, attr=0x00082, limit=0x0000ffff,
base=0x0000000000000000
[…] IDTR: limit=0x000001ff,
base=0xffffffff84004000
[…] TR: sel=0x0000, attr=0x0008b, limit=0x0000ffff,
base=0x0000000000000000
[…] EFER= 0x0000000000000d01 (effective)
[…] PAT = 0x0007040600070406
[…] DebugCtl = 0x0000000000000000 DebugExceptions = 0x0000000000000000
[…] Interruptibility = 00000000 ActivityState = 00000000
[…] *** Host State ***
[…] RIP = 0xffffffff86d28db6 RSP = 0xfffffe8040927d50
[…] CS=0010 SS=0018 DS=0000 ES=0000 FS=0000 GS=0000 TR=0040
[…] FSBase=0000639563fce700 GSBase=ffff88881a800000 TRBase=fffffe0001003000
[…] GDTBase=fffffe0001001000 IDTBase=fffffe0000000000
[…] CR0=0000000080050033 CR3=00000000026a0004 CR4=00000000007626f0
The "host" (which is our L1 VMM, I guess) has CR0.WP enabled and that is
what I think confuses ESXi to enforce the read-only property to the L2
guest as well -- for unknown reasons so far.
[…] Sysenter RSP=fffffe0001003000 CS:RIP=0010:ffffffff810031a0
[…] PAT = 0x0407050600070106
[…] *** Control State ***
[…] PinBased=0000003f CPUBased=b5a06dfa SecondaryExec=001034ee
[…] EntryControls=000053ff ExitControls=000befff
[…] ExceptionBitmap=00060042 PFECmask=00000000 PFECmatch=00000000
[…] VMEntry: intr_info=00000000 errcode=00000003 ilen=00000000
[…] VMExit: intr_info=00000000 errcode=00000000 ilen=00000000
[…] reason=00000002 qualification=0000000000000000
[…] IDTVectoring: info=00000000 errcode=00000000
[…] TSC Offset = 0xffffad7a1057f4cc
[…] TPR Threshold = 0x00
[…] virt-APIC addr = 0x000000015716b000
[…] EPT pointer = 0x0000000583c1e05e
[…] PLE Gap=00000080 Window=00001000
[…] Virtual processor ID = 0x0002
I tried to reproduce the bug with different KVM based L0 VMMs (with and
without this series; vanilla and grsecurity kernels) but no luck. That's
why I'm suspecting a ESXi bug.
I'm leaning to make CR0.WP guest owned only iff we're running on bare
metal or the VMM is KVM to not play whack-a-mole for all the VMMs that
might have similar bugs. (Will try to test a few others here as well.)
However, that would prevent them from getting the performance gain, so
I'd rather have this fixed / worked around in KVM instead.
Any ideas how to investigate this further?
Thanks,
Mathias
PS: ...should have left the chicken bit of v3 to be able to disable the
feature by a module parameter ;)
>
> Suggested-and-co-developed-by: Sean Christopherson <[email protected]>
> Signed-off-by: Mathias Krause <[email protected]>
> ---
> arch/x86/kvm/kvm_cache_regs.h | 2 +-
> arch/x86/kvm/vmx/nested.c | 4 ++--
> arch/x86/kvm/vmx/vmx.c | 2 +-
> arch/x86/kvm/vmx/vmx.h | 18 ++++++++++++++++++
> 4 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h
> index 4c91f626c058..e50d353b5c1c 100644
> --- a/arch/x86/kvm/kvm_cache_regs.h
> +++ b/arch/x86/kvm/kvm_cache_regs.h
> @@ -4,7 +4,7 @@
>
> #include <linux/kvm_host.h>
>
> -#define KVM_POSSIBLE_CR0_GUEST_BITS X86_CR0_TS
> +#define KVM_POSSIBLE_CR0_GUEST_BITS (X86_CR0_TS | X86_CR0_WP)
> #define KVM_POSSIBLE_CR4_GUEST_BITS \
> (X86_CR4_PVI | X86_CR4_DE | X86_CR4_PCE | X86_CR4_OSFXSR \
> | X86_CR4_OSXMMEXCPT | X86_CR4_PGE | X86_CR4_TSD | X86_CR4_FSGSBASE)
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index f63b28f46a71..61d940fc91ba 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -4481,7 +4481,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
> * CR0_GUEST_HOST_MASK is already set in the original vmcs01
> * (KVM doesn't change it);
> */
> - vcpu->arch.cr0_guest_owned_bits = KVM_POSSIBLE_CR0_GUEST_BITS;
> + vcpu->arch.cr0_guest_owned_bits = vmx_l1_guest_owned_cr0_bits();
> vmx_set_cr0(vcpu, vmcs12->host_cr0);
>
> /* Same as above - no reason to call set_cr4_guest_host_mask(). */
> @@ -4632,7 +4632,7 @@ static void nested_vmx_restore_host_state(struct kvm_vcpu *vcpu)
> */
> vmx_set_efer(vcpu, nested_vmx_get_vmcs01_guest_efer(vmx));
>
> - vcpu->arch.cr0_guest_owned_bits = KVM_POSSIBLE_CR0_GUEST_BITS;
> + vcpu->arch.cr0_guest_owned_bits = vmx_l1_guest_owned_cr0_bits();
> vmx_set_cr0(vcpu, vmcs_readl(CR0_READ_SHADOW));
>
> vcpu->arch.cr4_guest_owned_bits = ~vmcs_readl(CR4_GUEST_HOST_MASK);
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 8fc1a0c7856f..e501f6864a72 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4790,7 +4790,7 @@ static void init_vmcs(struct vcpu_vmx *vmx)
> /* 22.2.1, 20.8.1 */
> vm_entry_controls_set(vmx, vmx_vmentry_ctrl());
>
> - vmx->vcpu.arch.cr0_guest_owned_bits = KVM_POSSIBLE_CR0_GUEST_BITS;
> + vmx->vcpu.arch.cr0_guest_owned_bits = vmx_l1_guest_owned_cr0_bits();
> vmcs_writel(CR0_GUEST_HOST_MASK, ~vmx->vcpu.arch.cr0_guest_owned_bits);
>
> set_cr4_guest_host_mask(vmx);
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index 2acdc54bc34b..423e9d3c9c40 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -640,6 +640,24 @@ BUILD_CONTROLS_SHADOW(tertiary_exec, TERTIARY_VM_EXEC_CONTROL, 64)
> (1 << VCPU_EXREG_EXIT_INFO_1) | \
> (1 << VCPU_EXREG_EXIT_INFO_2))
>
> +static inline unsigned long vmx_l1_guest_owned_cr0_bits(void)
> +{
> + unsigned long bits = KVM_POSSIBLE_CR0_GUEST_BITS;
> +
> + /*
> + * CR0.WP needs to be intercepted when KVM is shadowing legacy paging
> + * in order to construct shadow PTEs with the correct protections.
> + * Note! CR0.WP technically can be passed through to the guest if
> + * paging is disabled, but checking CR0.PG would generate a cyclical
> + * dependency of sorts due to forcing the caller to ensure CR0 holds
> + * the correct value prior to determining which CR0 bits can be owned
> + * by L1. Keep it simple and limit the optimization to EPT.
> + */
> + if (!enable_ept)
> + bits &= ~X86_CR0_WP;
> + return bits;
> +}
> +
> static __always_inline struct kvm_vmx *to_kvm_vmx(struct kvm *kvm)
> {
> return container_of(kvm, struct kvm_vmx, kvm);
On Thu, Mar 30, 2023, Mathias Krause wrote:
> On 22.03.23 02:37, Mathias Krause wrote:
> > Guests like grsecurity that make heavy use of CR0.WP to implement kernel
> > level W^X will suffer from the implied VMEXITs.
> >
> > With EPT there is no need to intercept a guest change of CR0.WP, so
> > simply make it a guest owned bit if we can do so.
> >
> > This implies that a read of a guest's CR0.WP bit might need a VMREAD.
> > However, the only potentially affected user seems to be kvm_init_mmu()
> > which is a heavy operation to begin with. But also most callers already
> > cache the full value of CR0 anyway, so no additional VMREAD is needed.
> > The only exception is nested_vmx_load_cr3().
> >
> > This change is VMX-specific, as SVM has no such fine grained control
> > register intercept control.
>
> Just a heads up! We did more tests, especially with the backports we did
> internally already, and ran into a bug when running a nested guest on an
> ESXi host.
>
> Setup is like: ESXi (L0) -> Linux (L1) -> Linux (L2)
>
> The Linux system, especially the kernel, is the same for L1 and L2. It's
> a grsecurity kernel, so makes use of toggling CR0.WP at runtime.
>
> The bug we see is that when L2 disables CR0.WP and tries to write to an
> r/o memory region (implicitly to the r/o GDT via LTR in our use case),
> this triggers a fault (EPT violation?) that gets ignored by L1, as,
> apparently, everything is fine from its point of view.
It's not an EPT violation if there's a triple fault. Given that you're dumping
the VMCS from handle_triple_fault(), I assume that L2 gets an unexpected #PF that
leads to a triple fault in L2.
Just to make sure we're on the same page: L1 is still alive after this, correct?
> I suspect the L0 VMM to be at fault here, as the VMCS structures look
> good, IMO. Here is a dump of vmx->loaded_vmcs in handle_triple_fault():
...
> The "host" (which is our L1 VMM, I guess) has CR0.WP enabled and that is
> what I think confuses ESXi to enforce the read-only property to the L2
> guest as well -- for unknown reasons so far.
...
> I tried to reproduce the bug with different KVM based L0 VMMs (with and
> without this series; vanilla and grsecurity kernels) but no luck. That's
> why I'm suspecting a ESXi bug.
...
> I'm leaning to make CR0.WP guest owned only iff we're running on bare
> metal or the VMM is KVM to not play whack-a-mole for all the VMMs that
> might have similar bugs. (Will try to test a few others here as well.)
> However, that would prevent them from getting the performance gain, so
> I'd rather have this fixed / worked around in KVM instead.
Before we start putting bandaids on this, let's (a) confirm this appears to be
an issue with ESXi and (b) pull in VMware folks to get their input.
> Any ideas how to investigate this further?
Does the host in question support UMIP?
Can you capture a tracepoint log from L1 to verify that L1 KVM is _not_ injecting
any kind of exception? E.g. to get the KVM kitchen sink:
echo 1 > /sys/kernel/debug/tracing/tracing_on
echo 1 > /sys/kernel/debug/tracing/events/kvm/enable
cat /sys/kernel/debug/tracing/trace > log
Or if that's too noisy, a more targeted trace (exception injection + emulation)
woud be:
echo 1 > /sys/kernel/debug/tracing/tracing_on
echo 1 > /sys/kernel/debug/tracing/events/kvm/kvm_emulate_insn/enable
echo 1 > /sys/kernel/debug/tracing/events/kvm/kvm_inj_exception/enable
echo 1 > /sys/kernel/debug/tracing/events/kvm/kvm_entry/enable
echo 1 > /sys/kernel/debug/tracing/events/kvm/kvm_exit/enable
> PS: ...should have left the chicken bit of v3 to be able to disable the
> feature by a module parameter ;)
A chicken bit isn't a good solution for this sort of thing. Toggling a KVM module
param requires (a) knowing that it exists and (b) knowing the conditions under which
it is/isn't safe to toggle the bit.
E.g. if this ends up being an ESXi L0 bug, then an option might be to add something
in vmware_platform_setup() to communicate the bug to KVM so that KVM can precisely
disable the optimization on affected platforms.
On 30.03.23 19:12, Sean Christopherson wrote:
> On Thu, Mar 30, 2023, Mathias Krause wrote:
>> Just a heads up! We did more tests, especially with the backports we did
>> internally already, and ran into a bug when running a nested guest on an
>> ESXi host.
>>
>> Setup is like: ESXi (L0) -> Linux (L1) -> Linux (L2)
>>
>> The Linux system, especially the kernel, is the same for L1 and L2. It's
>> a grsecurity kernel, so makes use of toggling CR0.WP at runtime.
>>
>> The bug we see is that when L2 disables CR0.WP and tries to write to an
>> r/o memory region (implicitly to the r/o GDT via LTR in our use case),
>> this triggers a fault (EPT violation?) that gets ignored by L1, as,
>> apparently, everything is fine from its point of view.
>
> It's not an EPT violation if there's a triple fault. Given that you're dumping
> the VMCS from handle_triple_fault(), I assume that L2 gets an unexpected #PF that
> leads to a triple fault in L2.
Indeed it does, more on this below.
>
> Just to make sure we're on the same page: L1 is still alive after this, correct?
Yes, L1 is still alive and doing fine.
>> I suspect the L0 VMM to be at fault here, as the VMCS structures look
>> good, IMO. Here is a dump of vmx->loaded_vmcs in handle_triple_fault():
>
> ...
>
>> The "host" (which is our L1 VMM, I guess) has CR0.WP enabled and that is
>> what I think confuses ESXi to enforce the read-only property to the L2
>> guest as well -- for unknown reasons so far.
>
> ...
>
>> I tried to reproduce the bug with different KVM based L0 VMMs (with and
>> without this series; vanilla and grsecurity kernels) but no luck. That's
>> why I'm suspecting a ESXi bug.
>
> ...
>
>> I'm leaning to make CR0.WP guest owned only iff we're running on bare
>> metal or the VMM is KVM to not play whack-a-mole for all the VMMs that
>> might have similar bugs. (Will try to test a few others here as well.)
>> However, that would prevent them from getting the performance gain, so
>> I'd rather have this fixed / worked around in KVM instead.
>
> Before we start putting bandaids on this, let's (a) confirm this appears to be
> an issue with ESXi and (b) pull in VMware folks to get their input.
>
>> Any ideas how to investigate this further?
>
> Does the host in question support UMIP?
It should, but I've no access to the host. Within L1 I don't see umip
in /proc/cpuinfo. It's a "Intel(R) Xeon(R) Gold 6258R", though, so
Cascade Lake.
>
> Can you capture a tracepoint log from L1 to verify that L1 KVM is _not_ injecting
> any kind of exception? E.g. to get the KVM kitchen sink:
>
> echo 1 > /sys/kernel/debug/tracing/tracing_on
> echo 1 > /sys/kernel/debug/tracing/events/kvm/enable
>
> cat /sys/kernel/debug/tracing/trace > log
>
> Or if that's too noisy, a more targeted trace (exception injection + emulation)
> woud be:
I've even added 'echo 1 > /sys/kernel/tracing/events/kvmmmu/enable' to
the mix to get some more info and that leads to below (trimmed to the
part covering the error; L2 has 2 vCPUs bound to L1's vCPUs 1 and 2):
qemu-system-x86-8397 [002] d.... 1404.389366: kvm_exit: vcpu 1 reason CR_ACCESS rip 0xffffffff8100007a info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000
qemu-system-x86-8397 [002] ..... 1404.389373: kvm_cr: cr_write 0 = 0x80050033
That's the initialization of L2's CR0 in secondary_startup_64() and
also the last CR0 write we'll see, as toggling CR0.WP won't generate a
VMEXIT because the bit is guest owned. So this is the value KVM will
cache as vcpu->arch.cr0.
qemu-system-x86-8397 [002] ..... 1404.389384: kvm_tdp_mmu_spte_changed: as id 0 gfn 0 level 4 old_spte 2c583e907 new_spte 0
qemu-system-x86-8397 [002] ..... 1404.389384: kvm_mmu_prepare_zap_page: sp gen 0 gfn 0 l3 8-byte q0 direct wux nxe ad root 0 sync
qemu-system-x86-8397 [002] ..... 1404.389386: kvm_tdp_mmu_spte_changed: as id 0 gfn 0 level 3 old_spte 2c583d907 new_spte 5a0
qemu-system-x86-8397 [002] ..... 1404.389386: kvm_mmu_prepare_zap_page: sp gen 0 gfn 0 l2 8-byte q0 direct wux nxe ad root 0 sync
qemu-system-x86-8397 [002] ..... 1404.389388: kvm_tdp_mmu_spte_changed: as id 0 gfn 0 level 2 old_spte 2c583c907 new_spte 5a0
qemu-system-x86-8397 [002] ..... 1404.389388: kvm_mmu_prepare_zap_page: sp gen 0 gfn 0 l1 8-byte q0 direct wux nxe ad root 0 sync
qemu-system-x86-8397 [002] ..... 1404.389391: kvm_tdp_mmu_spte_changed: as id 0 gfn 99 level 1 old_spte 600000455299b77 new_spte 5a0
qemu-system-x86-8397 [002] ..... 1404.389393: kvm_tdp_mmu_spte_changed: as id 0 gfn 9a level 1 old_spte 60000045529ab77 new_spte 5a0
qemu-system-x86-8397 [002] ..... 1404.389395: kvm_tdp_mmu_spte_changed: as id 0 gfn 9c level 1 old_spte 60000045529cb77 new_spte 5a0
qemu-system-x86-8397 [002] ..... 1404.389408: kvm_tdp_mmu_spte_changed: as id 0 gfn 9d level 1 old_spte 60000045529db77 new_spte 5a0
qemu-system-x86-8397 [002] ..... 1404.389411: kvm_tdp_mmu_spte_changed: as id 0 gfn 9e level 1 old_spte 60000045529eb77 new_spte 5a0
qemu-system-x86-8396 [001] d.... 1404.389421: kvm_exit: vcpu 0 reason EXTERNAL_INTERRUPT rip 0xffffffff8108aca7 info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x800000fb error_code 0x00000000
qemu-system-x86-8397 [002] ..... 1404.389425: kvm_tdp_mmu_spte_changed: as id 0 gfn 1000 level 2 old_spte 2c583b907 new_spte 5a0
qemu-system-x86-8397 [002] ..... 1404.389425: kvm_mmu_prepare_zap_page: sp gen 0 gfn 1000 l1 8-byte q0 direct wux nxe ad root 0 sync
qemu-system-x86-8397 [002] ..... 1404.389427: kvm_tdp_mmu_spte_changed: as id 0 gfn 1000 level 1 old_spte 6000002c7c00b77 new_spte 5a0
qemu-system-x86-8397 [002] ..... 1404.389430: kvm_tdp_mmu_spte_changed: as id 0 gfn 2400 level 2 old_spte 60000021be00bf3 new_spte 5a0
qemu-system-x86-8397 [002] ..... 1404.389434: kvm_tdp_mmu_spte_changed: as id 0 gfn 2800 level 2 old_spte 60000021ba00bf3 new_spte 5a0
qemu-system-x86-8396 [001] d.... 1404.389435: kvm_entry: vcpu 0, rip 0xffffffff8108aca7
qemu-system-x86-8397 [002] ..... 1404.389436: kvm_tdp_mmu_spte_changed: as id 0 gfn 2a00 level 2 old_spte 60000021d400bf3 new_spte 5a0
qemu-system-x86-8397 [002] ..... 1404.389439: kvm_tdp_mmu_spte_changed: as id 0 gfn 2c00 level 2 old_spte 60000021d600bf3 new_spte 5a0
qemu-system-x86-8396 [001] d.... 1404.389448: kvm_exit: vcpu 0 reason EXTERNAL_INTERRUPT rip 0xffffffff8113b1f9 info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x800000fb error_code 0x00000000
qemu-system-x86-8396 [001] d.... 1404.389456: kvm_entry: vcpu 0, rip 0xffffffff8113b1f9
qemu-system-x86-8397 [002] d.... 1404.389461: kvm_entry: vcpu 1, rip 0xffffffff8100007d
qemu-system-x86-8396 [001] d.... 1404.389462: kvm_exit: vcpu 0 reason EXTERNAL_INTERRUPT rip 0xffffffff8113b1f9 info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x800000f6 error_code 0x00000000
qemu-system-x86-8396 [001] d.... 1404.389462: kvm_entry: vcpu 0, rip 0xffffffff8113b1f9
qemu-system-x86-8397 [002] d.... 1404.389481: kvm_exit: vcpu 1 reason CR_ACCESS rip 0xffffffff810000a0 info1 0x0000000000000104 info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000
qemu-system-x86-8397 [002] ..... 1404.389488: kvm_cr: cr_write 4 = 0xb0
Above is L2's init of CR4, still in secondary_startup_64().
qemu-system-x86-8397 [002] d.... 1404.389491: kvm_entry: vcpu 1, rip 0xffffffff810000a3
qemu-system-x86-8397 [002] d.... 1404.389501: kvm_exit: vcpu 1 reason CPUID rip 0xffffffff81000127 info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000
qemu-system-x86-8397 [002] ..... 1404.389501: kvm_cpuid: func 80000001 idx d3f0eef rax 50657 rbx 0 rcx 121 rdx 2c100800, cpuid entry found
qemu-system-x86-8397 [002] d.... 1404.389501: kvm_entry: vcpu 1, rip 0xffffffff81000129
qemu-system-x86-8397 [002] d.... 1404.389509: kvm_exit: vcpu 1 reason MSR_READ rip 0xffffffff81000130 info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000
qemu-system-x86-8397 [002] ..... 1404.389509: kvm_msr: msr_read c0000080 = 0xd01
qemu-system-x86-8397 [002] d.... 1404.389510: kvm_entry: vcpu 1, rip 0xffffffff81000132
qemu-system-x86-8397 [002] d.... 1404.389517: kvm_exit: vcpu 1 reason MSR_WRITE rip 0xffffffff81000149 info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000
qemu-system-x86-8397 [002] ..... 1404.389519: kvm_msr: msr_write c0000080 = 0xd01
That was the EFER setup...
qemu-system-x86-8397 [002] d.... 1404.389520: kvm_entry: vcpu 1, rip 0xffffffff8100014b
qemu-system-x86-8397 [002] d.... 1404.389536: kvm_exit: vcpu 1 reason CR_ACCESS rip 0xffffffff81053aee info1 0x0000000000000004 info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000
qemu-system-x86-8397 [002] ..... 1404.389537: kvm_cr: cr_write 4 = 0x3208b0
...remaining CR4 bits in cr4_init()...
qemu-system-x86-8397 [002] d.... 1404.389562: kvm_entry: vcpu 1, rip 0xffffffff81053af1
qemu-system-x86-8397 [002] d.... 1404.389574: kvm_exit: vcpu 1 reason EXTERNAL_INTERRUPT rip 0xffffffff81053af1 info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x800000f6 error_code 0x00000000
qemu-system-x86-8397 [002] d.... 1404.389583: kvm_entry: vcpu 1, rip 0xffffffff81053af1
qemu-system-x86-8397 [002] d.... 1404.389594: kvm_exit: vcpu 1 reason MSR_WRITE rip 0xffffffff81054faa info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000
qemu-system-x86-8397 [002] ..... 1404.389595: kvm_msr: msr_write c0000103 = 0x1
...TSC_AUX in setup_getcpu()...
qemu-system-x86-8397 [002] d.... 1404.389595: kvm_entry: vcpu 1, rip 0xffffffff81054fac
qemu-system-x86-8397 [002] d.... 1404.389646: kvm_exit: vcpu 1 reason LDTR_TR rip 0xffffffff810551b8 info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000
...and, finally, the trapping LTR in cpu_init_exception_handling()!
Now I get why you where asking for UMIP. No, that warning in
handle_desc() doesn't trigger for me. ;)
But now the interesting part: the emulation of LTR:
qemu-system-x86-8397 [002] ..... 1404.389656: kvm_mmu_pagetable_walk: addr ffffffff810551b8 pferr 10 F
~~~~~
Just a nit, but this 'pferr' should probably be 'access' as it's not an ---------------------------^
an error code (yet) but the access mode for this page table walk.
qemu-system-x86-8397 [002] ..... 1404.389661: kvm_mmu_paging_element: pte 2929067 level 4
qemu-system-x86-8397 [002] ..... 1404.389662: kvm_mmu_paging_element: pte 2b2c063 level 3
qemu-system-x86-8397 [002] ..... 1404.389662: kvm_mmu_paging_element: pte 10001e3 level 2
qemu-system-x86-8397 [002] ..... 1404.389664: kvm_emulate_insn: 0:ffffffff810551b8:0f 00 d8 (prot64)
Instruction fetch went fine.
qemu-system-x86-8397 [002] ..... 1404.389666: kvm_mmu_pagetable_walk: addr fffffe0000201040 pferr 0
qemu-system-x86-8397 [002] ..... 1404.389666: kvm_mmu_paging_element: pte 800000000292b063 level 4
qemu-system-x86-8397 [002] ..... 1404.389666: kvm_mmu_paging_element: pte 8000000002b2f063 level 3
qemu-system-x86-8397 [002] ..... 1404.389666: kvm_mmu_paging_element: pte 8000000002d31063 level 2
qemu-system-x86-8397 [002] ..... 1404.389666: kvm_mmu_paging_element: pte 8000000002f3a161 level 1
qemu-system-x86-8397 [002] ..... 1404.389667: kvm_mmu_pagetable_walk: addr fffffe0000201048 pferr 0
qemu-system-x86-8397 [002] ..... 1404.389667: kvm_mmu_paging_element: pte 800000000292b063 level 4
qemu-system-x86-8397 [002] ..... 1404.389667: kvm_mmu_paging_element: pte 8000000002b2f063 level 3
qemu-system-x86-8397 [002] ..... 1404.389667: kvm_mmu_paging_element: pte 8000000002d31063 level 2
qemu-system-x86-8397 [002] ..... 1404.389667: kvm_mmu_paging_element: pte 8000000002f3a161 level 1
Read of the descriptor as well, but...
qemu-system-x86-8397 [002] ..... 1404.389668: kvm_mmu_pagetable_walk: addr fffffe0000201040 pferr 2 W
qemu-system-x86-8397 [002] ..... 1404.389668: kvm_mmu_paging_element: pte 800000000292b063 level 4
qemu-system-x86-8397 [002] ..... 1404.389668: kvm_mmu_paging_element: pte 8000000002b2f063 level 3
qemu-system-x86-8397 [002] ..... 1404.389668: kvm_mmu_paging_element: pte 8000000002d31063 level 2
qemu-system-x86-8397 [002] ..... 1404.389669: kvm_mmu_paging_element: pte 8000000002f3a161 level 1
qemu-system-x86-8397 [002] ..... 1404.389669: kvm_mmu_walker_error: pferr 3 P|W
qemu-system-x86-8397 [002] ..... 1404.389675: kvm_mmu_pagetable_walk: addr fffffe0000201040 pferr 2 W
qemu-system-x86-8397 [002] ..... 1404.389677: kvm_mmu_paging_element: pte 800000000292b063 level 4
qemu-system-x86-8397 [002] ..... 1404.389677: kvm_mmu_paging_element: pte 8000000002b2f063 level 3
qemu-system-x86-8397 [002] ..... 1404.389677: kvm_mmu_paging_element: pte 8000000002d31063 level 2
qemu-system-x86-8397 [002] ..... 1404.389678: kvm_mmu_paging_element: pte 8000000002f3a161 level 1
qemu-system-x86-8397 [002] ..... 1404.389678: kvm_mmu_walker_error: pferr 3 P|W
qemu-system-x86-8397 [002] ..... 1404.389684: kvm_inj_exception: #PF (0x3)
...the write failed and that's the page fault you were looking for...
qemu-system-x86-8397 [002] d.... 1404.389711: kvm_entry: vcpu 1, rip 0xffffffff810551b8
qemu-system-x86-8397 [002] d.... 1404.389730: kvm_exit: vcpu 1 reason TRIPLE_FAULT rip 0xffffffff810551b8 info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000
...and here the triple fault I was seeing.
The page faults look legitimate on first sight, as the write bit isn't
set in the PTE. However, CR0.WP is 0, as show earlier in the VMCS dump,
so this shouldn't trigger a page fault.
Looks like I was too early to blame ESXi and this is more like a bug in
KVM's emulation of guest writes. I just don't see where the
kvm_read_cr0() is missing, yet -- obviously before we do the page table
walk, but even after adding one to init_emulate_ctxt() I still get the
fault, so there must be more to it. :/
Maybe it's not a stale CR0 value but the page table walker not taking
the guest's CR0.WP into account? Maybe a missing role update?
> [...]
>
>> PS: ...should have left the chicken bit of v3 to be able to disable the
>> feature by a module parameter ;)
>
> A chicken bit isn't a good solution for this sort of thing. Toggling a KVM module
> param requires (a) knowing that it exists and (b) knowing the conditions under which
> it is/isn't safe to toggle the bit.
Sure. But it would have allowed me to notice that's indeed the last
patch of the series that's the culprit.
>
> E.g. if this ends up being an ESXi L0 bug, then an option might be to add something
> in vmware_platform_setup() to communicate the bug to KVM so that KVM can precisely
> disable the optimization on affected platforms.
Agreed. But, as noted above, the signs are pointing to a bug in KVM.
Thanks,
Mathias
On 30.03.23 22:15, Mathias Krause wrote:
> [...]
> Maybe it's not a stale CR0 value but the page table walker not taking
> the guest's CR0.WP into account? Maybe a missing role update?
Indeed, it is. This does the trick for me:
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 31be188aa842..6a9e90725c84 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8372,6 +8372,9 @@ static void init_emulate_ctxt(struct kvm_vcpu *vcpu)
init_decode_cache(ctxt);
vcpu->arch.emulate_regs_need_sync_from_vcpu = false;
+ /* Ensure we're doing page table walks with an up2date MMU role */
+ if ((vcpu->arch.cr0 ^ kvm_read_cr0(vcpu)) == X86_CR0_WP)
+ kvm_init_mmu(vcpu);
}
void kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip)
Very heavy weight and misplaced, but a start :)
It should (1) be limited to VMX as that's the only one that would make
CR0.WP a guest owned bit and (2) limited to emulated instructions that
actually do write operations, as read are harmless, obviously.
Mathias
On Thu, Mar 30, 2023, Sean Christopherson wrote:
> On Thu, Mar 30, 2023, Mathias Krause wrote:
> > On 22.03.23 02:37, Mathias Krause wrote:
> > I'm leaning to make CR0.WP guest owned only iff we're running on bare
> > metal or the VMM is KVM to not play whack-a-mole for all the VMMs that
> > might have similar bugs. (Will try to test a few others here as well.)
> > However, that would prevent them from getting the performance gain, so
> > I'd rather have this fixed / worked around in KVM instead.
>
> Before we start putting bandaids on this, let's (a) confirm this appears to be
> an issue with ESXi and (b) pull in VMware folks to get their input.
>
> > Any ideas how to investigate this further?
>
> Does the host in question support UMIP?
>
> Can you capture a tracepoint log from L1 to verify that L1 KVM is _not_ injecting
> any kind of exception? E.g. to get the KVM kitchen sink:
>
> echo 1 > /sys/kernel/debug/tracing/tracing_on
> echo 1 > /sys/kernel/debug/tracing/events/kvm/enable
>
> cat /sys/kernel/debug/tracing/trace > log
>
> Or if that's too noisy, a more targeted trace (exception injection + emulation)
> woud be:
>
> echo 1 > /sys/kernel/debug/tracing/tracing_on
>
> echo 1 > /sys/kernel/debug/tracing/events/kvm/kvm_emulate_insn/enable
> echo 1 > /sys/kernel/debug/tracing/events/kvm/kvm_inj_exception/enable
> echo 1 > /sys/kernel/debug/tracing/events/kvm/kvm_entry/enable
> echo 1 > /sys/kernel/debug/tracing/events/kvm/kvm_exit/enable
Duh, this is likely a KVM bug. I expect the issue will go away if you revert
fb509f76acc8 ("KVM: VMX: Make CR0.WP a guest owned bit")
KVM doesn't consume CR0.WP for _its_ MMU, but it does consume CR0.WP for the
guest walker. By passing through CR0.WP, toggling only CR0.WP will not trap
(obviously) and thus won't run through kvm_post_set_cr0(), thus resulting in stle
information due to not invoking kvm_init_mmu().
I'm preetty sure I even called that we needed to refresh the permissions, but then
obviously forgot to actually make that happen.
I believe this will remedy the issue. If it does, I'll post a proper patch
(likely won't be until next week). Compile tested only.
---
arch/x86/kvm/mmu.h | 8 +++++++-
arch/x86/kvm/mmu/mmu.c | 14 ++++++++++++++
2 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
index 89f532516a45..4a303aa735dd 100644
--- a/arch/x86/kvm/mmu.h
+++ b/arch/x86/kvm/mmu.h
@@ -113,6 +113,7 @@ void kvm_init_shadow_ept_mmu(struct kvm_vcpu *vcpu, bool execonly,
bool kvm_can_do_async_pf(struct kvm_vcpu *vcpu);
int kvm_handle_page_fault(struct kvm_vcpu *vcpu, u64 error_code,
u64 fault_address, char *insn, int insn_len);
+void kvm_mmu_refresh_passthrough_bits(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu);
int kvm_mmu_load(struct kvm_vcpu *vcpu);
void kvm_mmu_unload(struct kvm_vcpu *vcpu);
@@ -184,8 +185,13 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
u64 implicit_access = access & PFERR_IMPLICIT_ACCESS;
bool not_smap = ((rflags & X86_EFLAGS_AC) | implicit_access) == X86_EFLAGS_AC;
int index = (pfec + (not_smap << PFERR_RSVD_BIT)) >> 1;
- bool fault = (mmu->permissions[index] >> pte_access) & 1;
u32 errcode = PFERR_PRESENT_MASK;
+ bool fault;
+
+ if (tdp_enabled)
+ kvm_mmu_refresh_passthrough_bits(vcpu, mmu);
+
+ fault = (mmu->permissions[index] >> pte_access) & 1;
WARN_ON(pfec & (PFERR_PK_MASK | PFERR_RSVD_MASK));
if (unlikely(mmu->pkru_mask)) {
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 4c874d4ec68f..2a63b5725f36 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -5186,6 +5186,20 @@ static union kvm_cpu_role kvm_calc_cpu_role(struct kvm_vcpu *vcpu,
return role;
}
+void kvm_mmu_refresh_passthrough_bits(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu)
+{
+ const bool cr0_wp = kvm_is_cr0_bit_set(vcpu, X86_CR0_WP);
+
+ BUILD_BUG_ON((KVM_MMU_CR0_ROLE_BITS & KVM_POSSIBLE_CR0_GUEST_BITS) != X86_CR0_WP);
+ BUILD_BUG_ON((KVM_MMU_CR4_ROLE_BITS & KVM_POSSIBLE_CR4_GUEST_BITS));
+
+ if (is_cr0_wp(mmu) == cr0_wp)
+ return;
+
+ mmu->cpu_role.base.cr0_wp = cr0_wp;
+ reset_guest_paging_metadata(vcpu, mmu);
+}
+
static inline int kvm_mmu_get_tdp_level(struct kvm_vcpu *vcpu)
{
/* tdp_root_level is architecture forced level, use it if nonzero */
base-commit: 27d6845d258b67f4eb3debe062b7dacc67e0c393
--
On Thu, Mar 30, 2023, Mathias Krause wrote:
> On 30.03.23 22:15, Mathias Krause wrote:
> > [...]
> > Maybe it's not a stale CR0 value but the page table walker not taking
> > the guest's CR0.WP into account? Maybe a missing role update?
>
> Indeed, it is. This does the trick for me:
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 31be188aa842..6a9e90725c84 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8372,6 +8372,9 @@ static void init_emulate_ctxt(struct kvm_vcpu *vcpu)
>
> init_decode_cache(ctxt);
> vcpu->arch.emulate_regs_need_sync_from_vcpu = false;
> + /* Ensure we're doing page table walks with an up2date MMU role */
> + if ((vcpu->arch.cr0 ^ kvm_read_cr0(vcpu)) == X86_CR0_WP)
> + kvm_init_mmu(vcpu);
> }
>
> void kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip)
>
> Very heavy weight and misplaced, but a start :)
>
> It should (1) be limited to VMX as that's the only one that would make
> CR0.WP a guest owned bit and (2) limited to emulated instructions that
> actually do write operations, as read are harmless, obviously.
For the record, I wrote my email before I saw this ;-)
On 30.03.23 22:33, Sean Christopherson wrote:
> Duh, this is likely a KVM bug. I expect the issue will go away if you revert
>
> fb509f76acc8 ("KVM: VMX: Make CR0.WP a guest owned bit")
>
> KVM doesn't consume CR0.WP for _its_ MMU, but it does consume CR0.WP for the
> guest walker. By passing through CR0.WP, toggling only CR0.WP will not trap
> (obviously) and thus won't run through kvm_post_set_cr0(), thus resulting in stle
> information due to not invoking kvm_init_mmu().
That reasoning sounds familiar ;)
>
> I'm preetty sure I even called that we needed to refresh the permissions, but then
> obviously forgot to actually make that happen.
:(
>
> I believe this will remedy the issue. If it does, I'll post a proper patch
> (likely won't be until next week). Compile tested only.
>
> ---
> arch/x86/kvm/mmu.h | 8 +++++++-
> arch/x86/kvm/mmu/mmu.c | 14 ++++++++++++++
> 2 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
> index 89f532516a45..4a303aa735dd 100644
> --- a/arch/x86/kvm/mmu.h
> +++ b/arch/x86/kvm/mmu.h
> @@ -113,6 +113,7 @@ void kvm_init_shadow_ept_mmu(struct kvm_vcpu *vcpu, bool execonly,
> bool kvm_can_do_async_pf(struct kvm_vcpu *vcpu);
> int kvm_handle_page_fault(struct kvm_vcpu *vcpu, u64 error_code,
> u64 fault_address, char *insn, int insn_len);
> +void kvm_mmu_refresh_passthrough_bits(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu);
>
> int kvm_mmu_load(struct kvm_vcpu *vcpu);
> void kvm_mmu_unload(struct kvm_vcpu *vcpu);
> @@ -184,8 +185,13 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
> u64 implicit_access = access & PFERR_IMPLICIT_ACCESS;
> bool not_smap = ((rflags & X86_EFLAGS_AC) | implicit_access) == X86_EFLAGS_AC;
> int index = (pfec + (not_smap << PFERR_RSVD_BIT)) >> 1;
> - bool fault = (mmu->permissions[index] >> pte_access) & 1;
> u32 errcode = PFERR_PRESENT_MASK;
> + bool fault;
> +
> + if (tdp_enabled)
> + kvm_mmu_refresh_passthrough_bits(vcpu, mmu);
> +
> + fault = (mmu->permissions[index] >> pte_access) & 1;
>
> WARN_ON(pfec & (PFERR_PK_MASK | PFERR_RSVD_MASK));
> if (unlikely(mmu->pkru_mask)) {
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 4c874d4ec68f..2a63b5725f36 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -5186,6 +5186,20 @@ static union kvm_cpu_role kvm_calc_cpu_role(struct kvm_vcpu *vcpu,
> return role;
> }
>
> +void kvm_mmu_refresh_passthrough_bits(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu)
> +{
> + const bool cr0_wp = kvm_is_cr0_bit_set(vcpu, X86_CR0_WP);
> +
> + BUILD_BUG_ON((KVM_MMU_CR0_ROLE_BITS & KVM_POSSIBLE_CR0_GUEST_BITS) != X86_CR0_WP);
> + BUILD_BUG_ON((KVM_MMU_CR4_ROLE_BITS & KVM_POSSIBLE_CR4_GUEST_BITS));
> +
> + if (is_cr0_wp(mmu) == cr0_wp)
> + return;
> +
> + mmu->cpu_role.base.cr0_wp = cr0_wp;
> + reset_guest_paging_metadata(vcpu, mmu);
> +}
> +
> static inline int kvm_mmu_get_tdp_level(struct kvm_vcpu *vcpu)
> {
> /* tdp_root_level is architecture forced level, use it if nonzero */
>
> base-commit: 27d6845d258b67f4eb3debe062b7dacc67e0c393
I tested a backported version of this patch on v6.1 as that's what I was
testing with and it worked fine. :)
I'll do more thorough tests tomorrow and actually on kvm-x86/next's HEAD.
Thanks a lot, Sean!
Mathias
On 30.03.23 22:55, Mathias Krause wrote:
> On 30.03.23 22:33, Sean Christopherson wrote:
>> I believe this will remedy the issue. If it does, I'll post a proper patch
>> (likely won't be until next week). Compile tested only.
>>
>> ---
>> arch/x86/kvm/mmu.h | 8 +++++++-
>> arch/x86/kvm/mmu/mmu.c | 14 ++++++++++++++
>> 2 files changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
>> index 89f532516a45..4a303aa735dd 100644
>> --- a/arch/x86/kvm/mmu.h
>> +++ b/arch/x86/kvm/mmu.h
>> @@ -113,6 +113,7 @@ void kvm_init_shadow_ept_mmu(struct kvm_vcpu *vcpu, bool execonly,
>> bool kvm_can_do_async_pf(struct kvm_vcpu *vcpu);
>> int kvm_handle_page_fault(struct kvm_vcpu *vcpu, u64 error_code,
>> u64 fault_address, char *insn, int insn_len);
>> +void kvm_mmu_refresh_passthrough_bits(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu);
>>
>> int kvm_mmu_load(struct kvm_vcpu *vcpu);
>> void kvm_mmu_unload(struct kvm_vcpu *vcpu);
>> @@ -184,8 +185,13 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
>> u64 implicit_access = access & PFERR_IMPLICIT_ACCESS;
>> bool not_smap = ((rflags & X86_EFLAGS_AC) | implicit_access) == X86_EFLAGS_AC;
>> int index = (pfec + (not_smap << PFERR_RSVD_BIT)) >> 1;
>> - bool fault = (mmu->permissions[index] >> pte_access) & 1;
>> u32 errcode = PFERR_PRESENT_MASK;
>> + bool fault;
>> +
>> + if (tdp_enabled)
>> + kvm_mmu_refresh_passthrough_bits(vcpu, mmu);
>> +
>> + fault = (mmu->permissions[index] >> pte_access) & 1;
>>
>> WARN_ON(pfec & (PFERR_PK_MASK | PFERR_RSVD_MASK));
>> if (unlikely(mmu->pkru_mask)) {
>> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
>> index 4c874d4ec68f..2a63b5725f36 100644
>> --- a/arch/x86/kvm/mmu/mmu.c
>> +++ b/arch/x86/kvm/mmu/mmu.c
>> @@ -5186,6 +5186,20 @@ static union kvm_cpu_role kvm_calc_cpu_role(struct kvm_vcpu *vcpu,
>> return role;
>> }
>>
>> +void kvm_mmu_refresh_passthrough_bits(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu)
>> +{
>> + const bool cr0_wp = kvm_is_cr0_bit_set(vcpu, X86_CR0_WP);
>> +
>> + BUILD_BUG_ON((KVM_MMU_CR0_ROLE_BITS & KVM_POSSIBLE_CR0_GUEST_BITS) != X86_CR0_WP);
>> + BUILD_BUG_ON((KVM_MMU_CR4_ROLE_BITS & KVM_POSSIBLE_CR4_GUEST_BITS));
>> +
>> + if (is_cr0_wp(mmu) == cr0_wp)
>> + return;
>> +
>> + mmu->cpu_role.base.cr0_wp = cr0_wp;
>> + reset_guest_paging_metadata(vcpu, mmu);
>> +}
>> +
>> static inline int kvm_mmu_get_tdp_level(struct kvm_vcpu *vcpu)
>> {
>> /* tdp_root_level is architecture forced level, use it if nonzero */
>>
>> base-commit: 27d6845d258b67f4eb3debe062b7dacc67e0c393
>
> I tested a backported version of this patch on v6.1 as that's what I was
> testing with and it worked fine. :)
>
> I'll do more thorough tests tomorrow and actually on kvm-x86/next's HEAD.
I extended the KUT CR0.WP tests and could confirm that (a) commit
fb509f76acc8 ("KVM: VMX: Make CR0.WP a guest owned bit") without the
above change triggers errors both ways in the emulator (denies access
while it should be allowed when CR0.WP=0; allows access while it should
trigger a page fault when CR0.WP=1) and (b) the above patch fixes these.
The tests are available here:
https://lore.kernel.org/kvm/[email protected]/
Thanks,
Mathias
On 3/22/2023 9:37 AM, Mathias Krause wrote:
> There is no need to unload the MMU roots with TDP enabled when only
> CR0.WP has changed -- the paging structures are still valid, only the
> permission bitmap needs to be updated.
>
> One heavy user of toggling CR0.WP is grsecurity's KERNEXEC feature to
> implement kernel W^X.
>
> The optimization brings a huge performance gain for this case as the
> following micro-benchmark running 'ssdd 10 50000' from rt-tests[1] on a
> grsecurity L1 VM shows (runtime in seconds, lower is better):
>
> legacy TDP shadow
> kvm-x86/next@d8708b 8.43s 9.45s 70.3s
> +patch 5.39s 5.63s 70.2s
>
> For legacy MMU this is ~36% faster, for TTP MMU even ~40% faster.
TTP --> TDP
> void kvm_post_set_cr0(struct kvm_vcpu *vcpu, unsigned long old_cr0, unsigned long cr0)
> {
> + /*
> + * CR0.WP is incorporated into the MMU role, but only for non-nested,
> + * indirect shadow MMUs. If TDP is enabled, the MMU's metadata needs
> + * to be updated, e.g. so that emulating guest translations does the
> + * right thing, but there's no need to unload the root as CR0.WP
> + * doesn't affect SPTEs.
> + */
> + if (tdp_enabled && (cr0 ^ old_cr0) == X86_CR0_WP) {
Curiously, this patch only affects tdp_enabled, why does legacy MMU also
see comparable performance gains?
On 07.05.23 09:32, Robert Hoo wrote:
> On 3/22/2023 9:37 AM, Mathias Krause wrote:
>> There is no need to unload the MMU roots with TDP enabled when only
>> CR0.WP has changed -- the paging structures are still valid, only the
>> permission bitmap needs to be updated.
>>
>> One heavy user of toggling CR0.WP is grsecurity's KERNEXEC feature to
>> implement kernel W^X.
>>
>> The optimization brings a huge performance gain for this case as the
>> following micro-benchmark running 'ssdd 10 50000' from rt-tests[1] on a
>> grsecurity L1 VM shows (runtime in seconds, lower is better):
>>
>> legacy TDP shadow
>> kvm-x86/next@d8708b 8.43s 9.45s 70.3s
>> +patch 5.39s 5.63s 70.2s
>>
>> For legacy MMU this is ~36% faster, for TTP MMU even ~40% faster.
>
> TTP --> TDP
Thanks, Sean fixed it up in the final commit:
https://git.kernel.org/linus/01b31714bd90
>
>> void kvm_post_set_cr0(struct kvm_vcpu *vcpu, unsigned long old_cr0,
>> unsigned long cr0)
>> {
>> + /*
>> + * CR0.WP is incorporated into the MMU role, but only for
>> non-nested,
>> + * indirect shadow MMUs. If TDP is enabled, the MMU's metadata
>> needs
>> + * to be updated, e.g. so that emulating guest translations does the
>> + * right thing, but there's no need to unload the root as CR0.WP
>> + * doesn't affect SPTEs.
>> + */
>> + if (tdp_enabled && (cr0 ^ old_cr0) == X86_CR0_WP) {
>
> Curiously, this patch only affects tdp_enabled, why does legacy MMU also
> see comparable performance gains?
Because 'tdp_enabled' just implies EPT / NPT and only 'tdp_mmu_enabled'
decides which MMU mode to use -- either legacy or TDP MMU (see
kvm_configure_mmu() and now gets invoked from vmx.c / svm.c).
Thanks,
Mathias
On 5/8/2023 5:30 PM, Mathias Krause wrote:
>>> void kvm_post_set_cr0(struct kvm_vcpu *vcpu, unsigned long old_cr0,
>>> unsigned long cr0)
>>> {
>>> + /*
>>> + * CR0.WP is incorporated into the MMU role, but only for
>>> non-nested,
>>> + * indirect shadow MMUs. If TDP is enabled, the MMU's metadata
>>> needs
>>> + * to be updated, e.g. so that emulating guest translations does the
>>> + * right thing, but there's no need to unload the root as CR0.WP
>>> + * doesn't affect SPTEs.
>>> + */
>>> + if (tdp_enabled && (cr0 ^ old_cr0) == X86_CR0_WP) {
>>
>> Curiously, this patch only affects tdp_enabled, why does legacy MMU also
>> see comparable performance gains?
>
> Because 'tdp_enabled' just implies EPT / NPT and only 'tdp_mmu_enabled'
> decides which MMU mode to use -- either legacy or TDP MMU (see
> kvm_configure_mmu() and now gets invoked from vmx.c / svm.c).
>
Ah, get it, thanks. The name indeed confuses me (and perhaps others).
After dig into,
1. kvm modules has a param "tdp_mmu_enabled", (in the first place)
indicates KVM level's willingness on enable two dimensional paging.
However, it in the end depends on ept/npt enabled or not on vendor layer.
So, uses a "tdp_mmu_allowed" to intermediately record this willness in kvm
module init phase.
/*
* Snapshot userspace's desire to enable the TDP MMU. Whether or not the
* TDP MMU is actually enabled is determined in kvm_configure_mmu()
* when the vendor module is loaded.
*/
tdp_mmu_allowed = tdp_mmu_enabled;
2. When vendor module init --> kvm_configure_mmu()
tdp_mmu_enabled = tdp_mmu_allowed && tdp_enabled;
tdp_mmu_enabled's semantics becomes, as its name indicates, the
eventual tdp mmu enablement status.
And, tdp_enabled, is the general (ept_enabled | npt_enabled).