* [PATCH v2 0/2] KVM: x86/mmu: Optimize rsvd pte checks
@ 2020-01-09 23:06 Sean Christopherson
2020-01-09 23:06 ` [PATCH v2 1/2] KVM: x86/mmu: Reorder the reserved bit check in prefetch_invalid_gpte() Sean Christopherson
2020-01-09 23:06 ` [PATCH v2 2/2] KVM: x86/mmu: Micro-optimize nEPT's bad memptype/XWR checks Sean Christopherson
0 siblings, 2 replies; 8+ messages in thread
From: Sean Christopherson @ 2020-01-09 23:06 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel, David Laight, Arvind Sankar
Two patches to optimize the reserved bit and illegal EPT memtype/XWR
checks, and more importantly, clean up an undocumented, subtle bitwise-OR
in the code.
v2: Rewrite everything.
v1: https://lkml.kernel.org/r/20200108001859.25254-1-sean.j.christopherson@intel.com
Sean Christopherson (2):
KVM: x86/mmu: Reorder the reserved bit check in
prefetch_invalid_gpte()
KVM: x86/mmu: Micro-optimize nEPT's bad memptype/XWR checks
arch/x86/kvm/mmu/mmu.c | 26 ++++++++++++++------------
arch/x86/kvm/mmu/paging_tmpl.h | 23 +++++++++++++++++++----
2 files changed, 33 insertions(+), 16 deletions(-)
--
2.24.1
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH v2 1/2] KVM: x86/mmu: Reorder the reserved bit check in prefetch_invalid_gpte() 2020-01-09 23:06 [PATCH v2 0/2] KVM: x86/mmu: Optimize rsvd pte checks Sean Christopherson @ 2020-01-09 23:06 ` Sean Christopherson 2020-01-10 11:37 ` Vitaly Kuznetsov 2020-01-09 23:06 ` [PATCH v2 2/2] KVM: x86/mmu: Micro-optimize nEPT's bad memptype/XWR checks Sean Christopherson 1 sibling, 1 reply; 8+ messages in thread From: Sean Christopherson @ 2020-01-09 23:06 UTC (permalink / raw) To: Paolo Bonzini Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel, David Laight, Arvind Sankar Move the !PRESENT and !ACCESSED checks in FNAME(prefetch_invalid_gpte) above the call to is_rsvd_bits_set(). For a well behaved guest, the !PRESENT and !ACCESSED are far more likely to evaluate true than the reserved bit checks, and they do not require additional memory accesses. Before: Dump of assembler code for function paging32_prefetch_invalid_gpte: 0x0000000000044240 <+0>: callq 0x44245 <paging32_prefetch_invalid_gpte+5> 0x0000000000044245 <+5>: mov %rcx,%rax 0x0000000000044248 <+8>: shr $0x7,%rax 0x000000000004424c <+12>: and $0x1,%eax 0x000000000004424f <+15>: lea 0x0(,%rax,4),%r8 0x0000000000044257 <+23>: add %r8,%rax 0x000000000004425a <+26>: mov %rcx,%r8 0x000000000004425d <+29>: and 0x120(%rsi,%rax,8),%r8 0x0000000000044265 <+37>: mov 0x170(%rsi),%rax 0x000000000004426c <+44>: shr %cl,%rax 0x000000000004426f <+47>: and $0x1,%eax 0x0000000000044272 <+50>: or %rax,%r8 0x0000000000044275 <+53>: jne 0x4427c <paging32_prefetch_invalid_gpte+60> 0x0000000000044277 <+55>: test $0x1,%cl 0x000000000004427a <+58>: jne 0x4428a <paging32_prefetch_invalid_gpte+74> 0x000000000004427c <+60>: mov %rdx,%rsi 0x000000000004427f <+63>: callq 0x44080 <drop_spte> 0x0000000000044284 <+68>: mov $0x1,%eax 0x0000000000044289 <+73>: retq 0x000000000004428a <+74>: xor %eax,%eax 0x000000000004428c <+76>: and $0x20,%ecx 0x000000000004428f <+79>: jne 0x44289 <paging32_prefetch_invalid_gpte+73> 0x0000000000044291 <+81>: mov %rdx,%rsi 0x0000000000044294 <+84>: callq 0x44080 <drop_spte> 0x0000000000044299 <+89>: mov $0x1,%eax 0x000000000004429e <+94>: jmp 0x44289 <paging32_prefetch_invalid_gpte+73> End of assembler dump. After: Dump of assembler code for function paging32_prefetch_invalid_gpte: 0x0000000000044240 <+0>: callq 0x44245 <paging32_prefetch_invalid_gpte+5> 0x0000000000044245 <+5>: test $0x1,%cl 0x0000000000044248 <+8>: je 0x4424f <paging32_prefetch_invalid_gpte+15> 0x000000000004424a <+10>: test $0x20,%cl 0x000000000004424d <+13>: jne 0x4425d <paging32_prefetch_invalid_gpte+29> 0x000000000004424f <+15>: mov %rdx,%rsi 0x0000000000044252 <+18>: callq 0x44080 <drop_spte> 0x0000000000044257 <+23>: mov $0x1,%eax 0x000000000004425c <+28>: retq 0x000000000004425d <+29>: mov %rcx,%rax 0x0000000000044260 <+32>: mov (%rsi),%rsi 0x0000000000044263 <+35>: shr $0x7,%rax 0x0000000000044267 <+39>: and $0x1,%eax 0x000000000004426a <+42>: lea 0x0(,%rax,4),%r8 0x0000000000044272 <+50>: add %r8,%rax 0x0000000000044275 <+53>: mov %rcx,%r8 0x0000000000044278 <+56>: and 0x120(%rsi,%rax,8),%r8 0x0000000000044280 <+64>: mov 0x170(%rsi),%rax 0x0000000000044287 <+71>: shr %cl,%rax 0x000000000004428a <+74>: and $0x1,%eax 0x000000000004428d <+77>: mov %rax,%rcx 0x0000000000044290 <+80>: xor %eax,%eax 0x0000000000044292 <+82>: or %rcx,%r8 0x0000000000044295 <+85>: je 0x4425c <paging32_prefetch_invalid_gpte+28> 0x0000000000044297 <+87>: mov %rdx,%rsi 0x000000000004429a <+90>: callq 0x44080 <drop_spte> 0x000000000004429f <+95>: mov $0x1,%eax 0x00000000000442a4 <+100>: jmp 0x4425c <paging32_prefetch_invalid_gpte+28> End of assembler dump. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/kvm/mmu/paging_tmpl.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h index b53bed3c901c..1fde6a1c506d 100644 --- a/arch/x86/kvm/mmu/paging_tmpl.h +++ b/arch/x86/kvm/mmu/paging_tmpl.h @@ -175,9 +175,6 @@ static bool FNAME(prefetch_invalid_gpte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp, u64 *spte, u64 gpte) { - if (is_rsvd_bits_set(vcpu->arch.mmu, gpte, PT_PAGE_TABLE_LEVEL)) - goto no_present; - if (!FNAME(is_present_gpte)(gpte)) goto no_present; @@ -186,6 +183,9 @@ static bool FNAME(prefetch_invalid_gpte)(struct kvm_vcpu *vcpu, !(gpte & PT_GUEST_ACCESSED_MASK)) goto no_present; + if (is_rsvd_bits_set(vcpu->arch.mmu, gpte, PT_PAGE_TABLE_LEVEL)) + goto no_present; + return false; no_present: -- 2.24.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] KVM: x86/mmu: Reorder the reserved bit check in prefetch_invalid_gpte() 2020-01-09 23:06 ` [PATCH v2 1/2] KVM: x86/mmu: Reorder the reserved bit check in prefetch_invalid_gpte() Sean Christopherson @ 2020-01-10 11:37 ` Vitaly Kuznetsov 0 siblings, 0 replies; 8+ messages in thread From: Vitaly Kuznetsov @ 2020-01-10 11:37 UTC (permalink / raw) To: Sean Christopherson, Paolo Bonzini Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel, David Laight, Arvind Sankar Sean Christopherson <sean.j.christopherson@intel.com> writes: > Move the !PRESENT and !ACCESSED checks in FNAME(prefetch_invalid_gpte) > above the call to is_rsvd_bits_set(). For a well behaved guest, the > !PRESENT and !ACCESSED are far more likely to evaluate true than the > reserved bit checks, and they do not require additional memory accesses. > > Before: > Dump of assembler code for function paging32_prefetch_invalid_gpte: > 0x0000000000044240 <+0>: callq 0x44245 <paging32_prefetch_invalid_gpte+5> > 0x0000000000044245 <+5>: mov %rcx,%rax > 0x0000000000044248 <+8>: shr $0x7,%rax > 0x000000000004424c <+12>: and $0x1,%eax > 0x000000000004424f <+15>: lea 0x0(,%rax,4),%r8 > 0x0000000000044257 <+23>: add %r8,%rax > 0x000000000004425a <+26>: mov %rcx,%r8 > 0x000000000004425d <+29>: and 0x120(%rsi,%rax,8),%r8 > 0x0000000000044265 <+37>: mov 0x170(%rsi),%rax > 0x000000000004426c <+44>: shr %cl,%rax > 0x000000000004426f <+47>: and $0x1,%eax > 0x0000000000044272 <+50>: or %rax,%r8 > 0x0000000000044275 <+53>: jne 0x4427c <paging32_prefetch_invalid_gpte+60> > 0x0000000000044277 <+55>: test $0x1,%cl > 0x000000000004427a <+58>: jne 0x4428a <paging32_prefetch_invalid_gpte+74> > 0x000000000004427c <+60>: mov %rdx,%rsi > 0x000000000004427f <+63>: callq 0x44080 <drop_spte> > 0x0000000000044284 <+68>: mov $0x1,%eax > 0x0000000000044289 <+73>: retq > 0x000000000004428a <+74>: xor %eax,%eax > 0x000000000004428c <+76>: and $0x20,%ecx > 0x000000000004428f <+79>: jne 0x44289 <paging32_prefetch_invalid_gpte+73> > 0x0000000000044291 <+81>: mov %rdx,%rsi > 0x0000000000044294 <+84>: callq 0x44080 <drop_spte> > 0x0000000000044299 <+89>: mov $0x1,%eax > 0x000000000004429e <+94>: jmp 0x44289 <paging32_prefetch_invalid_gpte+73> > End of assembler dump. > > After: > Dump of assembler code for function paging32_prefetch_invalid_gpte: > 0x0000000000044240 <+0>: callq 0x44245 <paging32_prefetch_invalid_gpte+5> > 0x0000000000044245 <+5>: test $0x1,%cl > 0x0000000000044248 <+8>: je 0x4424f <paging32_prefetch_invalid_gpte+15> > 0x000000000004424a <+10>: test $0x20,%cl > 0x000000000004424d <+13>: jne 0x4425d <paging32_prefetch_invalid_gpte+29> > 0x000000000004424f <+15>: mov %rdx,%rsi > 0x0000000000044252 <+18>: callq 0x44080 <drop_spte> > 0x0000000000044257 <+23>: mov $0x1,%eax > 0x000000000004425c <+28>: retq > 0x000000000004425d <+29>: mov %rcx,%rax > 0x0000000000044260 <+32>: mov (%rsi),%rsi > 0x0000000000044263 <+35>: shr $0x7,%rax > 0x0000000000044267 <+39>: and $0x1,%eax > 0x000000000004426a <+42>: lea 0x0(,%rax,4),%r8 > 0x0000000000044272 <+50>: add %r8,%rax > 0x0000000000044275 <+53>: mov %rcx,%r8 > 0x0000000000044278 <+56>: and 0x120(%rsi,%rax,8),%r8 > 0x0000000000044280 <+64>: mov 0x170(%rsi),%rax > 0x0000000000044287 <+71>: shr %cl,%rax > 0x000000000004428a <+74>: and $0x1,%eax > 0x000000000004428d <+77>: mov %rax,%rcx > 0x0000000000044290 <+80>: xor %eax,%eax > 0x0000000000044292 <+82>: or %rcx,%r8 > 0x0000000000044295 <+85>: je 0x4425c <paging32_prefetch_invalid_gpte+28> > 0x0000000000044297 <+87>: mov %rdx,%rsi > 0x000000000004429a <+90>: callq 0x44080 <drop_spte> > 0x000000000004429f <+95>: mov $0x1,%eax > 0x00000000000442a4 <+100>: jmp 0x4425c <paging32_prefetch_invalid_gpte+28> > End of assembler dump. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/kvm/mmu/paging_tmpl.h | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h > index b53bed3c901c..1fde6a1c506d 100644 > --- a/arch/x86/kvm/mmu/paging_tmpl.h > +++ b/arch/x86/kvm/mmu/paging_tmpl.h > @@ -175,9 +175,6 @@ static bool FNAME(prefetch_invalid_gpte)(struct kvm_vcpu *vcpu, > struct kvm_mmu_page *sp, u64 *spte, > u64 gpte) > { > - if (is_rsvd_bits_set(vcpu->arch.mmu, gpte, PT_PAGE_TABLE_LEVEL)) > - goto no_present; > - > if (!FNAME(is_present_gpte)(gpte)) > goto no_present; > > @@ -186,6 +183,9 @@ static bool FNAME(prefetch_invalid_gpte)(struct kvm_vcpu *vcpu, > !(gpte & PT_GUEST_ACCESSED_MASK)) > goto no_present; > > + if (is_rsvd_bits_set(vcpu->arch.mmu, gpte, PT_PAGE_TABLE_LEVEL)) > + goto no_present; > + > return false; > > no_present: FWIW, Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> -- Vitaly ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 2/2] KVM: x86/mmu: Micro-optimize nEPT's bad memptype/XWR checks 2020-01-09 23:06 [PATCH v2 0/2] KVM: x86/mmu: Optimize rsvd pte checks Sean Christopherson 2020-01-09 23:06 ` [PATCH v2 1/2] KVM: x86/mmu: Reorder the reserved bit check in prefetch_invalid_gpte() Sean Christopherson @ 2020-01-09 23:06 ` Sean Christopherson 2020-01-10 11:37 ` Vitaly Kuznetsov 1 sibling, 1 reply; 8+ messages in thread From: Sean Christopherson @ 2020-01-09 23:06 UTC (permalink / raw) To: Paolo Bonzini Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel, David Laight, Arvind Sankar Rework the handling of nEPT's bad memtype/XWR checks to micro-optimize the checks as much as possible. Move the check to a separate helper, __is_bad_mt_xwr(), which allows the guest_rsvd_check usage in paging_tmpl.h to omit the check entirely for paging32/64 (bad_mt_xwr is always zero for non-nEPT) while retaining the bitwise-OR of the current code for the shadow_zero_check in walk_shadow_page_get_mmio_spte(). Add a comment for the bitwise-OR usage in the mmio spte walk to avoid future attempts to "fix" the code, which is what prompted this optimization in the first place[*]. Opportunistically remove the superfluous '!= 0' and parantheses, and use BIT_ULL() instead of open coding its equivalent. The net effect is that code generation is largely unhanged for walk_shadow_page_get_mmio_spte(), marginally better for ept_prefetch_invalid_gpte(), and significantly improved for paging32/64_prefetch_invalid_gpte(). Note, walk_shadow_page_get_mmio_spte() can't use a templated version of the memtype/XRW as it works on the host's shadow PTEs, e.g. checks that KVM hasn't borked its EPT tables. Even if it could be templated, the benefits of having a single implementation far outweight the few uops that would be saved for NPT or non-TDP paging, e.g. most compilers inline it all the way to up kvm_mmu_page_fault(). [*] https://lkml.kernel.org/r/20200108001859.25254-1-sean.j.christopherson@intel.com Cc: Jim Mattson <jmattson@google.com> Cc: Vitaly Kuznetsov <vkuznets@redhat.com> Cc: David Laight <David.Laight@ACULAB.COM> Cc: Arvind Sankar <nivedita@alum.mit.edu> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/kvm/mmu/mmu.c | 26 ++++++++++++++------------ arch/x86/kvm/mmu/paging_tmpl.h | 19 +++++++++++++++++-- 2 files changed, 31 insertions(+), 14 deletions(-) diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index 7269130ea5e2..2992ff7b42a7 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -3968,20 +3968,14 @@ static gpa_t nonpaging_gva_to_gpa_nested(struct kvm_vcpu *vcpu, gpa_t vaddr, static bool __is_rsvd_bits_set(struct rsvd_bits_validate *rsvd_check, u64 pte, int level) { - int bit7 = (pte >> 7) & 1, low6 = pte & 0x3f; + int bit7 = (pte >> 7) & 1; - return (pte & rsvd_check->rsvd_bits_mask[bit7][level-1]) | - ((rsvd_check->bad_mt_xwr & (1ull << low6)) != 0); + return pte & rsvd_check->rsvd_bits_mask[bit7][level-1]; } -static bool is_rsvd_bits_set(struct kvm_mmu *mmu, u64 gpte, int level) +static bool __is_bad_mt_xwr(struct rsvd_bits_validate *rsvd_check, u64 pte) { - return __is_rsvd_bits_set(&mmu->guest_rsvd_check, gpte, level); -} - -static bool is_shadow_zero_bits_set(struct kvm_mmu *mmu, u64 spte, int level) -{ - return __is_rsvd_bits_set(&mmu->shadow_zero_check, spte, level); + return rsvd_check->bad_mt_xwr & BIT_ULL(pte & 0x3f); } static bool mmio_info_in_cache(struct kvm_vcpu *vcpu, u64 addr, bool direct) @@ -4005,9 +3999,12 @@ walk_shadow_page_get_mmio_spte(struct kvm_vcpu *vcpu, u64 addr, u64 *sptep) { struct kvm_shadow_walk_iterator iterator; u64 sptes[PT64_ROOT_MAX_LEVEL], spte = 0ull; + struct rsvd_bits_validate *rsvd_check; int root, leaf; bool reserved = false; + rsvd_check = &vcpu->arch.mmu->shadow_zero_check; + walk_shadow_page_lockless_begin(vcpu); for (shadow_walk_init(&iterator, vcpu, addr), @@ -4022,8 +4019,13 @@ walk_shadow_page_get_mmio_spte(struct kvm_vcpu *vcpu, u64 addr, u64 *sptep) if (!is_shadow_present_pte(spte)) break; - reserved |= is_shadow_zero_bits_set(vcpu->arch.mmu, spte, - iterator.level); + /* + * Use a bitwise-OR instead of a logical-OR to aggregate the + * reserved bit and EPT's invalid memtype/XWR checks to avoid + * adding a Jcc in the loop. + */ + reserved |= __is_bad_mt_xwr(rsvd_check, spte) | + __is_rsvd_bits_set(rsvd_check, spte, iterator.level); } walk_shadow_page_lockless_end(vcpu); diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h index 1fde6a1c506d..eaa00c4daeb1 100644 --- a/arch/x86/kvm/mmu/paging_tmpl.h +++ b/arch/x86/kvm/mmu/paging_tmpl.h @@ -128,6 +128,21 @@ static inline int FNAME(is_present_gpte)(unsigned long pte) #endif } +static bool FNAME(is_bad_mt_xwr)(struct rsvd_bits_validate *rsvd_check, u64 gpte) +{ +#if PTTYPE != PTTYPE_EPT + return false; +#else + return __is_bad_mt_xwr(rsvd_check, gpte); +#endif +} + +static bool FNAME(is_rsvd_bits_set)(struct kvm_mmu *mmu, u64 gpte, int level) +{ + return __is_rsvd_bits_set(&mmu->guest_rsvd_check, gpte, level) || + FNAME(is_bad_mt_xwr)(&mmu->guest_rsvd_check, gpte); +} + static int FNAME(cmpxchg_gpte)(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, pt_element_t __user *ptep_user, unsigned index, pt_element_t orig_pte, pt_element_t new_pte) @@ -183,7 +198,7 @@ static bool FNAME(prefetch_invalid_gpte)(struct kvm_vcpu *vcpu, !(gpte & PT_GUEST_ACCESSED_MASK)) goto no_present; - if (is_rsvd_bits_set(vcpu->arch.mmu, gpte, PT_PAGE_TABLE_LEVEL)) + if (FNAME(is_rsvd_bits_set)(vcpu->arch.mmu, gpte, PT_PAGE_TABLE_LEVEL)) goto no_present; return false; @@ -400,7 +415,7 @@ static int FNAME(walk_addr_generic)(struct guest_walker *walker, if (unlikely(!FNAME(is_present_gpte)(pte))) goto error; - if (unlikely(is_rsvd_bits_set(mmu, pte, walker->level))) { + if (unlikely(FNAME(is_rsvd_bits_set)(mmu, pte, walker->level))) { errcode = PFERR_RSVD_MASK | PFERR_PRESENT_MASK; goto error; } -- 2.24.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] KVM: x86/mmu: Micro-optimize nEPT's bad memptype/XWR checks 2020-01-09 23:06 ` [PATCH v2 2/2] KVM: x86/mmu: Micro-optimize nEPT's bad memptype/XWR checks Sean Christopherson @ 2020-01-10 11:37 ` Vitaly Kuznetsov 2020-01-10 16:04 ` Sean Christopherson 0 siblings, 1 reply; 8+ messages in thread From: Vitaly Kuznetsov @ 2020-01-10 11:37 UTC (permalink / raw) To: Sean Christopherson, Paolo Bonzini Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel, David Laight, Arvind Sankar Sean Christopherson <sean.j.christopherson@intel.com> writes: > Rework the handling of nEPT's bad memtype/XWR checks to micro-optimize > the checks as much as possible. Move the check to a separate helper, > __is_bad_mt_xwr(), which allows the guest_rsvd_check usage in > paging_tmpl.h to omit the check entirely for paging32/64 (bad_mt_xwr is > always zero for non-nEPT) while retaining the bitwise-OR of the current > code for the shadow_zero_check in walk_shadow_page_get_mmio_spte(). > > Add a comment for the bitwise-OR usage in the mmio spte walk to avoid > future attempts to "fix" the code, which is what prompted this > optimization in the first place[*]. > > Opportunistically remove the superfluous '!= 0' and parantheses, and > use BIT_ULL() instead of open coding its equivalent. > > The net effect is that code generation is largely unhanged unchanged? (or who's gonna hang that code? :-) ) > for walk_shadow_page_get_mmio_spte(), marginally better for > ept_prefetch_invalid_gpte(), and significantly improved for > paging32/64_prefetch_invalid_gpte(). > > Note, walk_shadow_page_get_mmio_spte() can't use a templated version of > the memtype/XRW as it works on the host's shadow PTEs, e.g. checks that > KVM hasn't borked its EPT tables. Even if it could be templated, the > benefits of having a single implementation far outweight the few uops > that would be saved for NPT or non-TDP paging, e.g. most compilers > inline it all the way to up kvm_mmu_page_fault(). > > [*] https://lkml.kernel.org/r/20200108001859.25254-1-sean.j.christopherson@intel.com > > Cc: Jim Mattson <jmattson@google.com> > Cc: Vitaly Kuznetsov <vkuznets@redhat.com> > Cc: David Laight <David.Laight@ACULAB.COM> > Cc: Arvind Sankar <nivedita@alum.mit.edu> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/kvm/mmu/mmu.c | 26 ++++++++++++++------------ > arch/x86/kvm/mmu/paging_tmpl.h | 19 +++++++++++++++++-- > 2 files changed, 31 insertions(+), 14 deletions(-) > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > index 7269130ea5e2..2992ff7b42a7 100644 > --- a/arch/x86/kvm/mmu/mmu.c > +++ b/arch/x86/kvm/mmu/mmu.c > @@ -3968,20 +3968,14 @@ static gpa_t nonpaging_gva_to_gpa_nested(struct kvm_vcpu *vcpu, gpa_t vaddr, > static bool > __is_rsvd_bits_set(struct rsvd_bits_validate *rsvd_check, u64 pte, int level) > { > - int bit7 = (pte >> 7) & 1, low6 = pte & 0x3f; > + int bit7 = (pte >> 7) & 1; > > - return (pte & rsvd_check->rsvd_bits_mask[bit7][level-1]) | > - ((rsvd_check->bad_mt_xwr & (1ull << low6)) != 0); > + return pte & rsvd_check->rsvd_bits_mask[bit7][level-1]; > } > > -static bool is_rsvd_bits_set(struct kvm_mmu *mmu, u64 gpte, int level) > +static bool __is_bad_mt_xwr(struct rsvd_bits_validate *rsvd_check, u64 pte) > { > - return __is_rsvd_bits_set(&mmu->guest_rsvd_check, gpte, level); > -} > - > -static bool is_shadow_zero_bits_set(struct kvm_mmu *mmu, u64 spte, int level) > -{ > - return __is_rsvd_bits_set(&mmu->shadow_zero_check, spte, level); > + return rsvd_check->bad_mt_xwr & BIT_ULL(pte & 0x3f); > } > > static bool mmio_info_in_cache(struct kvm_vcpu *vcpu, u64 addr, bool direct) > @@ -4005,9 +3999,12 @@ walk_shadow_page_get_mmio_spte(struct kvm_vcpu *vcpu, u64 addr, u64 *sptep) > { > struct kvm_shadow_walk_iterator iterator; > u64 sptes[PT64_ROOT_MAX_LEVEL], spte = 0ull; > + struct rsvd_bits_validate *rsvd_check; > int root, leaf; > bool reserved = false; > > + rsvd_check = &vcpu->arch.mmu->shadow_zero_check; > + > walk_shadow_page_lockless_begin(vcpu); > > for (shadow_walk_init(&iterator, vcpu, addr), > @@ -4022,8 +4019,13 @@ walk_shadow_page_get_mmio_spte(struct kvm_vcpu *vcpu, u64 addr, u64 *sptep) > if (!is_shadow_present_pte(spte)) > break; > > - reserved |= is_shadow_zero_bits_set(vcpu->arch.mmu, spte, > - iterator.level); > + /* > + * Use a bitwise-OR instead of a logical-OR to aggregate the > + * reserved bit and EPT's invalid memtype/XWR checks to avoid > + * adding a Jcc in the loop. > + */ > + reserved |= __is_bad_mt_xwr(rsvd_check, spte) | > + __is_rsvd_bits_set(rsvd_check, spte, iterator.level); > } > > walk_shadow_page_lockless_end(vcpu); > diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h > index 1fde6a1c506d..eaa00c4daeb1 100644 > --- a/arch/x86/kvm/mmu/paging_tmpl.h > +++ b/arch/x86/kvm/mmu/paging_tmpl.h > @@ -128,6 +128,21 @@ static inline int FNAME(is_present_gpte)(unsigned long pte) > #endif > } > > +static bool FNAME(is_bad_mt_xwr)(struct rsvd_bits_validate *rsvd_check, u64 gpte) > +{ > +#if PTTYPE != PTTYPE_EPT > + return false; > +#else > + return __is_bad_mt_xwr(rsvd_check, gpte); > +#endif > +} > + > +static bool FNAME(is_rsvd_bits_set)(struct kvm_mmu *mmu, u64 gpte, int level) > +{ > + return __is_rsvd_bits_set(&mmu->guest_rsvd_check, gpte, level) || > + FNAME(is_bad_mt_xwr)(&mmu->guest_rsvd_check, gpte); > +} > + Not sure if it would make sense/difference (well, this is famous KVM MMU!) but as FNAME(is_bad_mt_xwr) has only one call site we could've as well merged it, something like: static bool FNAME(is_rsvd_bits_set)(struct kvm_mmu *mmu, u64 gpte, int level) { #if PTTYPE == PTTYPE_EPT bool res = __is_bad_mt_xwr(&mmu->guest_rsvd_check, gpte); #else bool res = false; #endif return __is_rsvd_bits_set(&mmu->guest_rsvd_check, gpte, level) || res; } but keeping it in-line with __is_rsvd_bits_set()/__is_bad_mt_xwr() in mmu.c likely has greater benefits. > static int FNAME(cmpxchg_gpte)(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, > pt_element_t __user *ptep_user, unsigned index, > pt_element_t orig_pte, pt_element_t new_pte) > @@ -183,7 +198,7 @@ static bool FNAME(prefetch_invalid_gpte)(struct kvm_vcpu *vcpu, > !(gpte & PT_GUEST_ACCESSED_MASK)) > goto no_present; > > - if (is_rsvd_bits_set(vcpu->arch.mmu, gpte, PT_PAGE_TABLE_LEVEL)) > + if (FNAME(is_rsvd_bits_set)(vcpu->arch.mmu, gpte, PT_PAGE_TABLE_LEVEL)) > goto no_present; > > return false; > @@ -400,7 +415,7 @@ static int FNAME(walk_addr_generic)(struct guest_walker *walker, > if (unlikely(!FNAME(is_present_gpte)(pte))) > goto error; > > - if (unlikely(is_rsvd_bits_set(mmu, pte, walker->level))) { > + if (unlikely(FNAME(is_rsvd_bits_set)(mmu, pte, walker->level))) { > errcode = PFERR_RSVD_MASK | PFERR_PRESENT_MASK; > goto error; > } FWIW, Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> I've also smoke-tested with with a Hyper-V guest (just in case) and nothing seems to be broken. -- Vitaly ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] KVM: x86/mmu: Micro-optimize nEPT's bad memptype/XWR checks 2020-01-10 11:37 ` Vitaly Kuznetsov @ 2020-01-10 16:04 ` Sean Christopherson 2020-01-10 16:13 ` David Laight 2020-01-15 18:17 ` Paolo Bonzini 0 siblings, 2 replies; 8+ messages in thread From: Sean Christopherson @ 2020-01-10 16:04 UTC (permalink / raw) To: Vitaly Kuznetsov Cc: Paolo Bonzini, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel, David Laight, Arvind Sankar On Fri, Jan 10, 2020 at 12:37:33PM +0100, Vitaly Kuznetsov wrote: > Sean Christopherson <sean.j.christopherson@intel.com> writes: > > --- a/arch/x86/kvm/mmu/paging_tmpl.h > > +++ b/arch/x86/kvm/mmu/paging_tmpl.h > > @@ -128,6 +128,21 @@ static inline int FNAME(is_present_gpte)(unsigned long pte) > > #endif > > } > > > > +static bool FNAME(is_bad_mt_xwr)(struct rsvd_bits_validate *rsvd_check, u64 gpte) > > +{ > > +#if PTTYPE != PTTYPE_EPT > > + return false; > > +#else > > + return __is_bad_mt_xwr(rsvd_check, gpte); > > +#endif > > +} > > + > > +static bool FNAME(is_rsvd_bits_set)(struct kvm_mmu *mmu, u64 gpte, int level) > > +{ > > + return __is_rsvd_bits_set(&mmu->guest_rsvd_check, gpte, level) || > > + FNAME(is_bad_mt_xwr)(&mmu->guest_rsvd_check, gpte); > > +} > > + > > Not sure if it would make sense/difference (well, this is famous KVM > MMU!) but as FNAME(is_bad_mt_xwr) > > has only one call site we could've as well merged it, something like: > > static bool FNAME(is_rsvd_bits_set)(struct kvm_mmu *mmu, u64 gpte, int level) > { > #if PTTYPE == PTTYPE_EPT > bool res = __is_bad_mt_xwr(&mmu->guest_rsvd_check, gpte); > #else > bool res = false; > #endif > return __is_rsvd_bits_set(&mmu->guest_rsvd_check, gpte, level) || res; > } > > but keeping it in-line with __is_rsvd_bits_set()/__is_bad_mt_xwr() in > mmu.c likely has greater benefits. Ya, I don't love the code, but it was the least awful of the options I tried, in that it's the most readable without being too obnoxious. Similar to your suggestion, but it avoids evaluating __is_bad_mt_xwr() if reserved bits are set, which is admittedly rare. return __is_rsvd_bits_set(&mmu->guest_rsvd_check, gpte, level) #if PTTYPE == PTTYPE_EPT || __is_bad_mt_xwr(&mmu->guest_rsvd_check, gpte) #endif ; Or macrofying the call to keep the call site clean, but IMO this obfuscates things too much. return __is_rsvd_bits_set(&mmu->guest_rsvd_check, gpte, level) || IS_BAD_MT_XWR(&mmu->guest_rsvd_check, gpte); ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH v2 2/2] KVM: x86/mmu: Micro-optimize nEPT's bad memptype/XWR checks 2020-01-10 16:04 ` Sean Christopherson @ 2020-01-10 16:13 ` David Laight 2020-01-15 18:17 ` Paolo Bonzini 1 sibling, 0 replies; 8+ messages in thread From: David Laight @ 2020-01-10 16:13 UTC (permalink / raw) To: 'Sean Christopherson', Vitaly Kuznetsov Cc: Paolo Bonzini, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel, Arvind Sankar From: Sean Christopherson <sean.j.christopherson@intel.com> > Sent: 10 January 2020 16:05 ... > Similar to your suggestion, but it avoids evaluating __is_bad_mt_xwr() if > reserved bits are set, which is admittedly rare. > > return __is_rsvd_bits_set(&mmu->guest_rsvd_check, gpte, level) > #if PTTYPE == PTTYPE_EPT > || __is_bad_mt_xwr(&mmu->guest_rsvd_check, gpte) > #endif > ; Or: return __is_rsvd_bits_set(&mmu->guest_rsvd_check, gpte, level) || (PTTYPE == PTTYPE_EPT && __is_bad_mt_xwr(&mmu->guest_rsvd_check, gpte)); Relying in the compiler to optimise it away. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] KVM: x86/mmu: Micro-optimize nEPT's bad memptype/XWR checks 2020-01-10 16:04 ` Sean Christopherson 2020-01-10 16:13 ` David Laight @ 2020-01-15 18:17 ` Paolo Bonzini 1 sibling, 0 replies; 8+ messages in thread From: Paolo Bonzini @ 2020-01-15 18:17 UTC (permalink / raw) To: Sean Christopherson, Vitaly Kuznetsov Cc: Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel, David Laight, Arvind Sankar On 10/01/20 17:04, Sean Christopherson wrote: > Ya, I don't love the code, but it was the least awful of the options I > tried, in that it's the most readable without being too obnoxious. > > > Similar to your suggestion, but it avoids evaluating __is_bad_mt_xwr() if > reserved bits are set, which is admittedly rare. > > return __is_rsvd_bits_set(&mmu->guest_rsvd_check, gpte, level) > #if PTTYPE == PTTYPE_EPT > || __is_bad_mt_xwr(&mmu->guest_rsvd_check, gpte) > #endif > ; > > Or macrofying the call to keep the call site clean, but IMO this obfuscates > things too much. > > return __is_rsvd_bits_set(&mmu->guest_rsvd_check, gpte, level) || > IS_BAD_MT_XWR(&mmu->guest_rsvd_check, gpte); I think what you posted is the best (David's comes second). Queued, thanks. Paolo ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-01-15 18:17 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-09 23:06 [PATCH v2 0/2] KVM: x86/mmu: Optimize rsvd pte checks Sean Christopherson 2020-01-09 23:06 ` [PATCH v2 1/2] KVM: x86/mmu: Reorder the reserved bit check in prefetch_invalid_gpte() Sean Christopherson 2020-01-10 11:37 ` Vitaly Kuznetsov 2020-01-09 23:06 ` [PATCH v2 2/2] KVM: x86/mmu: Micro-optimize nEPT's bad memptype/XWR checks Sean Christopherson 2020-01-10 11:37 ` Vitaly Kuznetsov 2020-01-10 16:04 ` Sean Christopherson 2020-01-10 16:13 ` David Laight 2020-01-15 18:17 ` Paolo Bonzini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).