Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b0d4b430-afd2-979c-e7d2-b53e131412ad@grsecurity.net>
Date:   Mon, 8 May 2023 11:30:59 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH v4 2/6] KVM: x86: Do not unload MMU roots when only
 toggling CR0.WP with TDP enabled
Content-Language: en-US, de-DE
To:     Robert Hoo <robert.hoo.linux@gmail.com>, kvm@vger.kernel.org
Cc:     linux-kernel@vger.kernel.org,
        Sean Christopherson <seanjc@google.com>,
        Paolo Bonzini <pbonzini@redhat.com>
References: <20230322013731.102955-1-minipli@grsecurity.net>
 <20230322013731.102955-3-minipli@grsecurity.net>
 <e70af22a-f09f-aadb-8353-35b29d2def61@gmail.com>
From:   Mathias Krause <minipli@grsecurity.net>
In-Reply-To: <e70af22a-f09f-aadb-8353-35b29d2def61@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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