[RFC] KVM: x86: Don't wipe TDP MMU when guest sets %cr4

[RFC] KVM: x86: Don't wipe TDP MMU when guest sets %cr4

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 2555 bytes --]

If I understand things correctly, the point of the TDP MMU is to use
page tables such as EPT for GPA → HPA translations, but let the
virtualization support in the CPU handle all of the *virtual*
addressing and page tables, including the non-root mode %cr3/%cr4.

I have a guest which loves to flip the SMEP bit on and off in %cr4 all
the time. The guest is actually Xen, in its 'PV shim' mode which
enables it to support a single PV guest, while running in a true
hardware virtual machine:
https://lists.xenproject.org/archives/html/xen-devel/2018-01/msg00497.html

The performance is *awful*, since as far as I can tell, on every flip
KVM flushes the entire EPT. I understand why that might be necessary
for the mode where KVM is building up a set of shadow page tables to
directly map GVA → HPA and be loaded into %cr3 of a CPU that doesn't
support native EPT translations. But I don't understand why the TDP MMU
would need to do it. Surely we don't have to change anything in the EPT
just because the stuff in the non-root-mode %cr3/%cr4 changes?

So I tried this, and it went faster and nothing appears to have blown
up.

Am I missing something? Is this stupidly wrong?

--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1072,7 +1074,8 @@ int kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned
long cr4)
        if (kvm_x86_ops.set_cr4(vcpu, cr4))
                return 1;
 
-       kvm_post_set_cr4(vcpu, old_cr4, cr4);
+       if (!vcpu->kvm->arch.tdp_mmu_enabled)
+               kvm_post_set_cr4(vcpu, old_cr4, cr4);
 
        if ((cr4 ^ old_cr4) & (X86_CR4_OSXSAVE | X86_CR4_PKE))
                kvm_update_cpuid_runtime(vcpu);


Also... if I have *two* vCPUs it doesn't go quite as slowly while Xen
starts Grub and then Grub boots a Linux kernel. Until the kernel brings
up its second vCPU and *then* it starts going really slowly again. Is
that because the TDP roots are refcounted, and that idle vCPU holds
onto the unused one and prevents it from being completely thrown away?
Until the vCPU stops being idle and starts flipping SMEP on/off on
Linux←→Xen transitions too?

In practice, there's not a lot of point in Xen using SMEP when it's
purely acting as a library for its *one* guest, living in an HVM
container. The above patch speeds things up but telling Xen not to use
SMEP at all makes things go *much* faster. But if I'm not being
*entirely* stupid, there may be some generic improvements for
KVM+TDPMMU here somewhere so it's worth making a fool of myself by
asking...?

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [RFC] KVM: x86: Don't wipe TDP MMU when guest sets %cr4

[Jim Mattson]

On Tue, Oct 10, 2023 at 2:37 PM David Woodhouse <dwmw2@infradead.org> wrote:
>
> If I understand things correctly, the point of the TDP MMU is to use
> page tables such as EPT for GPA → HPA translations, but let the
> virtualization support in the CPU handle all of the *virtual*
> addressing and page tables, including the non-root mode %cr3/%cr4.
>
> I have a guest which loves to flip the SMEP bit on and off in %cr4 all
> the time. The guest is actually Xen, in its 'PV shim' mode which
> enables it to support a single PV guest, while running in a true
> hardware virtual machine:
> https://lists.xenproject.org/archives/html/xen-devel/2018-01/msg00497.html
>
> The performance is *awful*, since as far as I can tell, on every flip
> KVM flushes the entire EPT. I understand why that might be necessary
> for the mode where KVM is building up a set of shadow page tables to
> directly map GVA → HPA and be loaded into %cr3 of a CPU that doesn't
> support native EPT translations. But I don't understand why the TDP MMU
> would need to do it. Surely we don't have to change anything in the EPT
> just because the stuff in the non-root-mode %cr3/%cr4 changes?
>
> So I tried this, and it went faster and nothing appears to have blown
> up.
>
> Am I missing something? Is this stupidly wrong?

The TDP MMU is essentially a virtual TLB of guest-physical mappings,
so it's oblivious to %cr4 changes. However, the hardware TLBs of the
current logical processor contain combined mappings, which may have to
be flushed, depending on how %cr4 has changed. Since kvm has emulated
the instruction, it is responsible for flushing the hardware TLBs as
required (see the SDM, volume 3, section 4.10.4: Operations that
Invalidate TLBs and Paging-Structure Caches). Some of the logic in
kvm_post_set_cr4() seems to deal with that.

Not your fault, of course, but can't we come up with a better name
than "kvm_post_set_cr4"?

Re: [RFC] KVM: x86: Don't wipe TDP MMU when guest sets %cr4

[Sean Christopherson]

On Tue, Oct 10, 2023, David Woodhouse wrote:
> If I understand things correctly, the point of the TDP MMU is to use
> page tables such as EPT for GPA → HPA translations, but let the
> virtualization support in the CPU handle all of the *virtual*
> addressing and page tables, including the non-root mode %cr3/%cr4.
> 
> I have a guest which loves to flip the SMEP bit on and off in %cr4 all
> the time. The guest is actually Xen, in its 'PV shim' mode which
> enables it to support a single PV guest, while running in a true
> hardware virtual machine:
> https://lists.xenproject.org/archives/html/xen-devel/2018-01/msg00497.html
> 
> The performance is *awful*, since as far as I can tell, on every flip
> KVM flushes the entire EPT. I understand why that might be necessary
> for the mode where KVM is building up a set of shadow page tables to
> directly map GVA → HPA and be loaded into %cr3 of a CPU that doesn't
> support native EPT translations. But I don't understand why the TDP MMU
> would need to do it. Surely we don't have to change anything in the EPT
> just because the stuff in the non-root-mode %cr3/%cr4 changes?
> 
> So I tried this, and it went faster and nothing appears to have blown
> up.
> 
> Am I missing something? Is this stupidly wrong?

Heh, you're in luck, because regardless of what your darn pronoun "this" refers
to, the answer is yes, "this" is stupidly wrong.

The below is stupidly wrong.  KVM needs to at least reconfigure the guest's paging
metadata that is used to translate GVAs to GPAs during emulation.

But the TDP MMU behavior *was* also stupidly wrong.  The reason that two vCPUs
suck less is because KVM would zap SPTEs (EPT roots) if and only if *both* vCPUs
unloaded their roots at the same time.

Commit edbdb43fc96b ("KVM: x86: Preserve TDP MMU roots until they are explicitly
invalidated") should fix the behavior you're seeing.

And if we want to try and make SMEP blazing fast on Intel, we can probably let
the guest write it directly, i.e. give SMEP the same treatment as CR0.WP.  See
commits cf9f4c0eb169 ("KVM: x86/mmu: Refresh CR0.WP prior to checking for emulated
permission faults") and fb509f76acc8 ("KVM: VMX: Make CR0.WP a guest owned bit").

Oh, and if your userspace is doing something silly like constantly creating and
deleting memslots, see commit 0df9dab891ff ("KVM: x86/mmu: Stop zapping invalidated
TDP MMU roots asynchronously").

> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1072,7 +1074,8 @@ int kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned
> long cr4)
>         if (kvm_x86_ops.set_cr4(vcpu, cr4))
>                 return 1;
>  
> -       kvm_post_set_cr4(vcpu, old_cr4, cr4);
> +       if (!vcpu->kvm->arch.tdp_mmu_enabled)
> +               kvm_post_set_cr4(vcpu, old_cr4, cr4);
>  
>         if ((cr4 ^ old_cr4) & (X86_CR4_OSXSAVE | X86_CR4_PKE))
>                 kvm_update_cpuid_runtime(vcpu);
> 
> 
> Also... if I have *two* vCPUs it doesn't go quite as slowly while Xen
> starts Grub and then Grub boots a Linux kernel. Until the kernel brings
> up its second vCPU and *then* it starts going really slowly again. Is
> that because the TDP roots are refcounted, and that idle vCPU holds
> onto the unused one and prevents it from being completely thrown away?
> Until the vCPU stops being idle and starts flipping SMEP on/off on
> Linux←→Xen transitions too?
> 
> In practice, there's not a lot of point in Xen using SMEP when it's
> purely acting as a library for its *one* guest, living in an HVM
> container. The above patch speeds things up but telling Xen not to use
> SMEP at all makes things go *much* faster. But if I'm not being
> *entirely* stupid, there may be some generic improvements for
> KVM+TDPMMU here somewhere so it's worth making a fool of myself by
> asking...?

Re: [RFC] KVM: x86: Don't wipe TDP MMU when guest sets %cr4

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 4941 bytes --]

On Tue, 2023-10-10 at 16:25 -0700, Sean Christopherson wrote:
> On Tue, Oct 10, 2023, David Woodhouse wrote:
> > If I understand things correctly, the point of the TDP MMU is to use
> > page tables such as EPT for GPA → HPA translations, but let the
> > virtualization support in the CPU handle all of the *virtual*
> > addressing and page tables, including the non-root mode %cr3/%cr4.
> > 
> > I have a guest which loves to flip the SMEP bit on and off in %cr4 all
> > the time. The guest is actually Xen, in its 'PV shim' mode which
> > enables it to support a single PV guest, while running in a true
> > hardware virtual machine:
> > https://lists.xenproject.org/archives/html/xen-devel/2018-01/msg00497.html
> > 
> > The performance is *awful*, since as far as I can tell, on every flip
> > KVM flushes the entire EPT. I understand why that might be necessary
> > for the mode where KVM is building up a set of shadow page tables to
> > directly map GVA → HPA and be loaded into %cr3 of a CPU that doesn't
> > support native EPT translations. But I don't understand why the TDP MMU
> > would need to do it. Surely we don't have to change anything in the EPT
> > just because the stuff in the non-root-mode %cr3/%cr4 changes?
> > 
> > So I tried this, and it went faster and nothing appears to have blown
> > up.
> > 
> > Am I missing something? Is this stupidly wrong?
> 
> Heh, you're in luck, because regardless of what your darn pronoun "this" refers
> to, the answer is yes, "this" is stupidly wrong.

Hehe. Thought that might be the case. Thank you for the coherent
explanation and especially for the references.

(I hadn't got the PV shim working in qemu yet. I shall bump that up my
TODO list by a few quanta, as it would have let me run this test on a
newer kernel.)

> The below is stupidly wrong.  KVM needs to at least reconfigure the guest's paging
> metadata that is used to translate GVAs to GPAs during emulation.
> 
> But the TDP MMU behavior *was* also stupidly wrong.  The reason that two vCPUs
> suck less is because KVM would zap SPTEs (EPT roots) if and only if *both* vCPUs
> unloaded their roots at the same time.
> 
> Commit edbdb43fc96b ("KVM: x86: Preserve TDP MMU roots until they are explicitly
> invalidated") should fix the behavior you're seeing.
> 
> And if we want to try and make SMEP blazing fast on Intel, we can probably let
> the guest write it directly, i.e. give SMEP the same treatment as CR0.WP.  See
> commits cf9f4c0eb169 ("KVM: x86/mmu: Refresh CR0.WP prior to checking for emulated
> permission faults") and fb509f76acc8 ("KVM: VMX: Make CR0.WP a guest owned bit").

Thanks. In fact, looking at __kvm_mmu_refresh_passthrough_bits() in
commit cf9f4c0eb169 ("KVM: x86/mmu: Refresh CR0.WP…"), shouldn't it be
looking at the SMEP bit in %cr4 anyway?

In kvm_calc_cpu_role() the value of ____is_cr0_wp() is used *three*
times. For setting role.base.{cr0_wp,smep_andnot_wp,smap_andnot_wp}.

But __kvm_mmu_refresh_passthrough_bits() only refreshes
role.base.cr0_wp and not the other two. Do we need this?

--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -5159,6 +5159,8 @@ void __kvm_mmu_refresh_passthrough_bits(struct kvm_vcpu *vcpu,
                return;
 
        mmu->cpu_role.base.cr0_wp = cr0_wp;
+       mmu->cpu_role.base.smep_andnot_wp = mmu->cpu_role.ext.cr4_smep && !cr0_wp;
+       mmu->cpu_role.base.smap_andnot_wp = mmu->cpu_role.ext.cr4_smap && !cr0_wp;
        reset_guest_paging_metadata(vcpu, mmu);
 }
 
But I'm confused here. Even if I don't go as far as actually making
CR4.SMEP a guest-owned bit, and KVM still ends up handling it in
kvm_post_load_cr4()... why does KVM need to completely unload and
reinit the MMU? Would it not be sufficient just to refresh the role
bits, much like __kvm_mmu_refresh_passthrough_bits() does for CR0.WP?

(And what about flushing the hardware TLB, as Jim mentioned. I guess if
it's guest-owned we trust the CPU to do that, and if it's trapped then
KVM is required to do so)?

> Oh, and if your userspace is doing something silly like constantly creating and
> deleting memslots, see commit 0df9dab891ff ("KVM: x86/mmu: Stop zapping invalidated
> TDP MMU roots asynchronously").

Oooh, I'll play with that. Thanks! Although I bristle at 'silly' :)

Userspace only wants to add and remove single overlay pages. That's not
so silly, that's explicitly requested by the guest and required for PV
drivers to work.

AIUI it's the KVM API which means that userspace needs to stop all
vCPUs, *remove* the original memslot, add back the two halves of the
original memslot with the newly overlaid page in the middle, and then
let all the vCPUs run again. We don't need an atomic way to change
*all* the memslots at once, but a way to atomically overlay a *single*
range would make the whole enterprise feel less 'silly'.





[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [RFC] KVM: x86: Don't wipe TDP MMU when guest sets %cr4

[Jim Mattson]

On Wed, Oct 11, 2023 at 1:21 AM David Woodhouse <dwmw2@infradead.org> wrote:
>
> ...
> But I'm confused here. Even if I don't go as far as actually making
> CR4.SMEP a guest-owned bit, and KVM still ends up handling it in
> kvm_post_load_cr4()... why does KVM need to completely unload and
> reinit the MMU? Would it not be sufficient just to refresh the role
> bits, much like __kvm_mmu_refresh_passthrough_bits() does for CR0.WP?
>
> (And what about flushing the hardware TLB, as Jim mentioned. I guess if
> it's guest-owned we trust the CPU to do that, and if it's trapped then
> KVM is required to do so)?

Yes, guest-owned bits become the responsibility of the CPU.

With the TDP MMU, KVM probably should not intercept writes to %cr4 at
all. I'd argue that it's better to construct guest paging attributes
from first principles every time KVM needs to know, rather than
intercepting any operation that may change guest paging attributes. If
that's too expensive, perhaps KVM is emulating too many instructions.
:)

Particularly aggravating to me is that too many of these cost/benefit
decisions were made without consideration of nested virtualization. At
one time, someone observed that Linux hardly ever writes %cr4, so it
must be cheap to intercept. How ironic that they didn't consider the
fact that KVM is part of Linux, and KVM writes %cr4 all the time!

If the cost of determining paging attributes from first principles is
really prohibitive, maybe the solution is more flexibility. KVM can
start by intercepting writes to %cr4, but if the rate of intercepts is
egregious, it should do something else.

Re: [RFC] KVM: x86: Don't wipe TDP MMU when guest sets %cr4

[Paolo Bonzini]

On 10/11/23 10:20, David Woodhouse wrote:
> But __kvm_mmu_refresh_passthrough_bits() only refreshes
> role.base.cr0_wp and not the other two. Do we need this?
> 
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -5159,6 +5159,8 @@ void __kvm_mmu_refresh_passthrough_bits(struct kvm_vcpu *vcpu,
>                  return;
>   
>          mmu->cpu_role.base.cr0_wp = cr0_wp;
> +       mmu->cpu_role.base.smep_andnot_wp = mmu->cpu_role.ext.cr4_smep && !cr0_wp;
> +       mmu->cpu_role.base.smap_andnot_wp = mmu->cpu_role.ext.cr4_smap && !cr0_wp;
>          reset_guest_paging_metadata(vcpu, mmu);
>   }

{smep,smap}_andnot_wp only matter for shadow paging.  You can remove 
them from this function, and instead assign which is not called for 
shadow paging anyway, and set them in the root_role in kvm_init_shadow_mmu.

Paolo

Re: [RFC] KVM: x86: Don't wipe TDP MMU when guest sets %cr4

[Sean Christopherson]

On Wed, Oct 11, 2023, David Woodhouse wrote:
> On Tue, 2023-10-10 at 16:25 -0700, Sean Christopherson wrote:
> > On Tue, Oct 10, 2023, David Woodhouse wrote:
> But I'm confused here. Even if I don't go as far as actually making
> CR4.SMEP a guest-owned bit, and KVM still ends up handling it in
> kvm_post_load_cr4()... why does KVM need to completely unload and
> reinit the MMU? Would it not be sufficient just to refresh the role
> bits, much like __kvm_mmu_refresh_passthrough_bits() does for CR0.WP?

Maybe?  It largely hasn't happened simply because no one (yet) cares enough about
the performance of other bits to force the issue.
 
> (And what about flushing the hardware TLB, as Jim mentioned. I guess if
> it's guest-owned we trust the CPU to do that, and if it's trapped then
> KVM is required to do so)?

TBH, I forgot that clearing SMEP architecturally requires a TLB flush for the current
PCID on Intel.  I *think* it's actually fine so long as TDP is enabled.  Ah, yes,
it's fine.  Well, either that or kvm_invalidate_pcid() is buggy :-)

Relying on the CPU to flush the hardware TLBs is definitely ok, I was just trying
to think if there were any KVM artifacts that would need to be flushed/invalidated.
I can't think of any, e.g. KVM already disable GVA-based MMIO caching when TDP is
enabled, L1 is responsible for its virtual TLB if L1 is shadowing legacy paging for
L2, and if L1 is using EPT, i.e. KVM is shadowing L1 EPT and thus has a virtual TLB
for L2, then the PCID is irrelevant (doesn't affect EPT translations).