diff mbox series

[v4,2/6] KVM: x86: Do not unload MMU roots when only toggling CR0.WP with TDP enabled

Message ID 20230322013731.102955-3-minipli@grsecurity.net
State New
Headers
Series KVM: MMU: performance tweaks for heavy CR0.WP users |

Commit Message

Mathias Krause March 22, 2023, 1:37 a.m. UTC 
  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 <seanjc@google.com>
Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
 arch/x86/kvm/x86.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Robert Hoo May 7, 2023, 7:32 a.m. UTC | #1 
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?
Mathias Krause May 8, 2023, 9:30 a.m. UTC | #2 
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
Robert Hoo May 9, 2023, 1:04 a.m. UTC | #3 
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).
diff mbox series

Patch

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);