On 15.03.23 23:11, Sean Christopherson wrote: > Can you tweak the shortlog to something like this? I want to make it very clear > that this applies only to the TDP case (more below). I did a double take when I > first read the subject :-) > > KVM: x86: Do not unload MMU roots when only toggling CR0.WP with TDP enabled Sure, will do! > > On Wed, Feb 01, 2023, Mathias Krause wrote: >> There is no need to unload the MMU roots for a direct MMU role 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.git/queue 11.55s 13.91s 75.2s >> kvm.git/queue+patch 7.32s 7.31s 74.6s >> >> For legacy MMU this is ~36% faster, for TTP MMU even ~47% faster. Also >> TDP and legacy MMU now both have around the same 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 <seanjc@xxxxxxxxxx> > > No need for this, I just threw a snippet at you as part of code review. And IMO, > if someone goes through the pain of running benchmarks, they get full credit no > matter what ;-) Reviewers (and in your case maintainers) get far too little credit, so I'd rather keep that tag, if you don't mind that hard ;) > >> Signed-off-by: Mathias Krause <minipli@xxxxxxxxxxxxxx> >> --- >> v2: handle the CR0.WP case directly in kvm_post_set_cr0() and only for >> the direct MMU role -- Sean >> >> I re-ran the benchmark and it's even faster than with my patch, as the >> critical path is now the first one handled and is now inline. Thanks a >> lot for the suggestion, Sean! >> >> arch/x86/kvm/x86.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index 508074e47bc0..f09bfc0a3cc1 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -902,6 +902,15 @@ EXPORT_SYMBOL_GPL(load_pdptrs); >> >> void kvm_post_set_cr0(struct kvm_vcpu *vcpu, unsigned long old_cr0, unsigned long cr0) >> { >> + /* >> + * Toggling just CR0.WP doesn't invalidate page tables per se, only the >> + * permission bits. >> + */ >> + if (vcpu->arch.mmu->root_role.direct && (cr0 ^ old_cr0) == X86_CR0_WP) { > > Past me was wrong, which is a very good thing in this case. Per the APM, > > The host hCR0.WP bit is ignored under nested paging. > See what you did? You even went that far and re-read the manual. Definitely deserves credit! > which means that CR0.WP doesn't need to be incorporated into the role. Ha! And > really-past me even wrote a very nice comment to call that out in commit 31e96bc63655 > ("KVM: nSVM: Add a comment to document why nNPT uses vmcb01, not vCPU state"). > > Double ha! That's all moot, because if this code is reached for a nested MMU, > it means L2 is active and the CR0 being changed is gCR0, not L1's hCR0. > > So more simply, this can be > > if (tdp_enabled && (cr0 ^ old_cr0) == X86_CR0_WP) Looks much simpler, indeed. But might deserve a little comment itself why it's fine test 'tdp_enabled' only... > > or if we want to exempt non-paging mode for the shadow MMU as well... > > if ((cr0 ^ old_cr0) == X86_CR0_WP && (tdp_enabled || !(cr0 & X86_CR0_PG))) > > Actually, if we bother to check CR0.PG, then we might as well get greedy and > skip _all_ updates when paging is disabled. E.g. end up with this over two > patches? First one exempts the tdp_enabled case, second one completely exempts > paging disabled. > ...and there it is already! :D > /* > * CR0.WP is incorporated into the MMU role, but only for non-nested, > * 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 when TDP is enabled. > */ > if ((cr0 ^ old_cr0) == X86_CR0_WP) { > if (!(cr0 & X86_CR0_PG)) > return; > > if (tdp_enabled) { > kvm_init_mmu(vcpu); > return; > } > } Thanks, Sean! Sounds all very good to me. I'll cook up these commits, do some more tests and send a v4 tomorrow, if time allows.
