| Message ID | 20200925212302.3979661-4-bgardon@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce the TDP MMU | expand |
On 25/09/20 23:22, Ben Gardon wrote: > +static bool __read_mostly tdp_mmu_enabled = true; > +module_param_named(tdp_mmu, tdp_mmu_enabled, bool, 0644); > + This would need some custom callbacks to avoid the warning in is_tdp_mmu_enabled(). Paolo
Nit on all the shortlogs, can you use "KVM: x86/mmu" instead of "kvm: mmu"? On Fri, Sep 25, 2020 at 02:22:43PM -0700, Ben Gardon wrote: > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c > new file mode 100644 > index 0000000000000..8241e18c111e6 > --- /dev/null > +++ b/arch/x86/kvm/mmu/tdp_mmu.c > @@ -0,0 +1,34 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +#include "tdp_mmu.h" > + > +static bool __read_mostly tdp_mmu_enabled = true; > +module_param_named(tdp_mmu, tdp_mmu_enabled, bool, 0644); Do y'all actually toggle tdp_mmu_enabled while VMs are running? I can see having a per-VM capability, or a read-only module param, but a writable module param is... interesting. > +static bool is_tdp_mmu_enabled(void) > +{ > + if (!READ_ONCE(tdp_mmu_enabled)) > + return false; > + > + if (WARN_ONCE(!tdp_enabled, > + "Creating a VM with TDP MMU enabled requires TDP.")) This should be enforced, i.e. clear tdp_mmu_enabled if !tdp_enabled. As is, it's a user triggerable WARN, which is not good, e.g. with PANIC_ON_WARN. > + return false; > + > + return true; > +}
On Fri, Sep 25, 2020 at 02:22:43PM -0700, Ben Gardon wrote: > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c > new file mode 100644 > index 0000000000000..8241e18c111e6 > --- /dev/null > +++ b/arch/x86/kvm/mmu/tdp_mmu.c > @@ -0,0 +1,34 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +#include "tdp_mmu.h" > + > +static bool __read_mostly tdp_mmu_enabled = true; > +module_param_named(tdp_mmu, tdp_mmu_enabled, bool, 0644); This param should not exist until the TDP MMU is fully functional, e.g. running KVM against "kvm: mmu: Support zapping SPTEs in the TDP MMU" immediately hits a BUG() in the rmap code. I haven't wrapped my head around the entire series to grok whether it make sense to incrementally enable the TDP MMU, but my gut says that's probably non-sensical. The local variable can exist (default to false) so that you can flip a single switch to enable the code instead of having to plumb in the variable to its consumers. kernel BUG at arch/x86/kvm/mmu/mmu.c:1427! invalid opcode: 0000 [#1] SMP CPU: 4 PID: 1218 Comm: stable Not tainted 5.9.0-rc4+ #44 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015 RIP: 0010:rmap_get_first.isra.0+0x51/0x60 [kvm] Code: <0f> 0b 45 31 c0 4c 89 c0 c3 66 0f 1f 44 00 00 0f 1f 44 00 00 49 b9 RSP: 0018:ffffc9000099fb50 EFLAGS: 00010246 RAX: 0000000000000000 RBX: 0000000000001000 RCX: 0000000000000000 RDX: ffffc9000099fb60 RSI: ffffc9000099fb58 RDI: ffff88816b1a7ec8 RBP: ffff88816b1a7e70 R08: ffff888173c95000 R09: ffff88816b1a7448 R10: 00000000000000f8 R11: ffff88817bd29c70 R12: ffffc90000981000 R13: ffffc9000099fbac R14: ffffc90000989a88 R15: ffff88816b1a7ec8 FS: 00007f7a755fd700(0000) GS:ffff88817bd00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007f7a60141000 CR3: 000000016b031004 CR4: 0000000000172ea0 Call Trace: __kvm_mmu_prepare_zap_page+0x98/0x330 [kvm] kvm_mmu_zap_all_fast+0x100/0x190 [kvm] kvm_page_track_flush_slot+0x54/0x80 [kvm] kvm_set_memslot+0x198/0x640 [kvm] kvm_delete_memslot+0x59/0xc0 [kvm] __kvm_set_memory_region+0x494/0x560 [kvm] ? khugepaged+0x470/0x2230 ? mem_cgroup_charge_statistics.isra.0+0x1c/0x40 kvm_set_memory_region+0x27/0x40 [kvm] kvm_vm_ioctl+0x379/0xca0 [kvm] ? do_user_addr_fault+0x1ad/0x3a7 __x64_sys_ioctl+0x83/0xb0 do_syscall_64+0x33/0x80 entry_SYSCALL_64_after_hwframe+0x44/0xa9
On 30/09/20 18:57, Sean Christopherson wrote: >> + >> +static bool __read_mostly tdp_mmu_enabled = true; >> +module_param_named(tdp_mmu, tdp_mmu_enabled, bool, 0644); > This param should not exist until the TDP MMU is fully functional, e.g. running > KVM against "kvm: mmu: Support zapping SPTEs in the TDP MMU" immediately hits a > BUG() in the rmap code. I haven't wrapped my head around the entire series to > grok whether it make sense to incrementally enable the TDP MMU, but my gut says > that's probably non-sensical. No, it doesn't. Whether to add the module parameter is kind of secondary, but I agree it shouldn't be true---not even at the end of this series, since fast page fault for example is not implemented yet. Paolo
On Tue, Sep 29, 2020 at 10:35 PM Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > Nit on all the shortlogs, can you use "KVM: x86/mmu" instead of "kvm: mmu"? Will do. > > On Fri, Sep 25, 2020 at 02:22:43PM -0700, Ben Gardon wrote: > > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c > > new file mode 100644 > > index 0000000000000..8241e18c111e6 > > --- /dev/null > > +++ b/arch/x86/kvm/mmu/tdp_mmu.c > > @@ -0,0 +1,34 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > + > > +#include "tdp_mmu.h" > > + > > +static bool __read_mostly tdp_mmu_enabled = true; > > +module_param_named(tdp_mmu, tdp_mmu_enabled, bool, 0644); > > Do y'all actually toggle tdp_mmu_enabled while VMs are running? I can see > having a per-VM capability, or a read-only module param, but a writable > module param is... interesting. We don't use this much, but it is useful when running tests to be able to go back and forth between running with and without the TDP MMU. I should have added a note that the module parameter is mostly for development purposes. > > > +static bool is_tdp_mmu_enabled(void) > > +{ > > + if (!READ_ONCE(tdp_mmu_enabled)) > > + return false; > > + > > + if (WARN_ONCE(!tdp_enabled, > > + "Creating a VM with TDP MMU enabled requires TDP.")) > > This should be enforced, i.e. clear tdp_mmu_enabled if !tdp_enabled. As is, > it's a user triggerable WARN, which is not good, e.g. with PANIC_ON_WARN. That's a good point. > > > + return false; > > + > > + return true; > > +}
On Wed, Sep 30, 2020 at 10:39 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 30/09/20 18:57, Sean Christopherson wrote: > >> + > >> +static bool __read_mostly tdp_mmu_enabled = true; > >> +module_param_named(tdp_mmu, tdp_mmu_enabled, bool, 0644); > > This param should not exist until the TDP MMU is fully functional, e.g. running > > KVM against "kvm: mmu: Support zapping SPTEs in the TDP MMU" immediately hits a > > BUG() in the rmap code. I haven't wrapped my head around the entire series to > > grok whether it make sense to incrementally enable the TDP MMU, but my gut says > > that's probably non-sensical. > > No, it doesn't. Whether to add the module parameter is kind of > secondary, but I agree it shouldn't be true---not even at the end of > this series, since fast page fault for example is not implemented yet. > > Paolo > I fully agree, sorry about that. I should have at least defaulted the module parameter to false before sending the series out. I'll remedy that in the next patch set. (Unless you beat me to it, Paolo)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 5303dbc5c9bce..35107819f48ae 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -963,6 +963,15 @@ struct kvm_arch { struct kvm_pmu_event_filter *pmu_event_filter; struct task_struct *nx_lpage_recovery_thread; + + /* + * Whether the TDP MMU is enabled for this VM. This contains a + * snapshot of the TDP MMU module parameter from when the VM was + * created and remains unchanged for the life of the VM. If this is + * true, TDP MMU handler functions will run for various MMU + * operations. + */ + bool tdp_mmu_enabled; }; struct kvm_vm_stat { diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile index cf6a9947955f7..e5b33938f86ed 100644 --- a/arch/x86/kvm/Makefile +++ b/arch/x86/kvm/Makefile @@ -16,7 +16,7 @@ kvm-$(CONFIG_KVM_ASYNC_PF) += $(KVM)/async_pf.o kvm-y += x86.o emulate.o i8259.o irq.o lapic.o \ i8254.o ioapic.o irq_comm.o cpuid.o pmu.o mtrr.o \ hyperv.o debugfs.o mmu/mmu.o mmu/page_track.o \ - mmu/tdp_iter.o + mmu/tdp_iter.o mmu/tdp_mmu.o kvm-intel-y += vmx/vmx.o vmx/vmenter.o vmx/pmu_intel.o vmx/vmcs12.o vmx/evmcs.o vmx/nested.o kvm-amd-y += svm/svm.o svm/vmenter.o svm/pmu.o svm/nested.o svm/avic.o svm/sev.o diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index b48b00c8cde65..0cb0c26939dfc 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -19,6 +19,7 @@ #include "ioapic.h" #include "mmu.h" #include "mmu_internal.h" +#include "tdp_mmu.h" #include "x86.h" #include "kvm_cache_regs.h" #include "kvm_emulate.h" @@ -5865,6 +5866,8 @@ void kvm_mmu_init_vm(struct kvm *kvm) { struct kvm_page_track_notifier_node *node = &kvm->arch.mmu_sp_tracker; + kvm_mmu_init_tdp_mmu(kvm); + node->track_write = kvm_mmu_pte_write; node->track_flush_slot = kvm_mmu_invalidate_zap_pages_in_memslot; kvm_page_track_register_notifier(kvm, node); @@ -5875,6 +5878,8 @@ void kvm_mmu_uninit_vm(struct kvm *kvm) struct kvm_page_track_notifier_node *node = &kvm->arch.mmu_sp_tracker; kvm_page_track_unregister_notifier(kvm, node); + + kvm_mmu_uninit_tdp_mmu(kvm); } void kvm_zap_gfn_range(struct kvm *kvm, gfn_t gfn_start, gfn_t gfn_end) diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c new file mode 100644 index 0000000000000..8241e18c111e6 --- /dev/null +++ b/arch/x86/kvm/mmu/tdp_mmu.c @@ -0,0 +1,34 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#include "tdp_mmu.h" + +static bool __read_mostly tdp_mmu_enabled = true; +module_param_named(tdp_mmu, tdp_mmu_enabled, bool, 0644); + +static bool is_tdp_mmu_enabled(void) +{ + if (!READ_ONCE(tdp_mmu_enabled)) + return false; + + if (WARN_ONCE(!tdp_enabled, + "Creating a VM with TDP MMU enabled requires TDP.")) + return false; + + return true; +} + +/* Initializes the TDP MMU for the VM, if enabled. */ +void kvm_mmu_init_tdp_mmu(struct kvm *kvm) +{ + if (!is_tdp_mmu_enabled()) + return; + + /* This should not be changed for the lifetime of the VM. */ + kvm->arch.tdp_mmu_enabled = true; +} + +void kvm_mmu_uninit_tdp_mmu(struct kvm *kvm) +{ + if (!kvm->arch.tdp_mmu_enabled) + return; +} diff --git a/arch/x86/kvm/mmu/tdp_mmu.h b/arch/x86/kvm/mmu/tdp_mmu.h new file mode 100644 index 0000000000000..dd3764f5a9aa3 --- /dev/null +++ b/arch/x86/kvm/mmu/tdp_mmu.h @@ -0,0 +1,10 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#ifndef __KVM_X86_MMU_TDP_MMU_H +#define __KVM_X86_MMU_TDP_MMU_H + +#include <linux/kvm_host.h> + +void kvm_mmu_init_tdp_mmu(struct kvm *kvm); +void kvm_mmu_uninit_tdp_mmu(struct kvm *kvm); +#endif /* __KVM_X86_MMU_TDP_MMU_H */
The TDP MMU offers an alternative mode of operation to the x86 shadow paging based MMU, optimized for running an L1 guest with TDP. The TDP MMU will require new fields that need to be initialized and torn down. Add hooks into the existing KVM MMU initialization process to do that initialization / cleanup. Currently the initialization and cleanup fucntions do not do very much, however more operations will be added in future patches. Tested by running kvm-unit-tests and KVM selftests on an Intel Haswell machine. This series introduced no new failures. This series can be viewed in Gerrit at: https://linux-review.googlesource.com/c/virt/kvm/kvm/+/2538 Signed-off-by: Ben Gardon <bgardon@google.com> --- arch/x86/include/asm/kvm_host.h | 9 +++++++++ arch/x86/kvm/Makefile | 2 +- arch/x86/kvm/mmu/mmu.c | 5 +++++ arch/x86/kvm/mmu/tdp_mmu.c | 34 +++++++++++++++++++++++++++++++++ arch/x86/kvm/mmu/tdp_mmu.h | 10 ++++++++++ 5 files changed, 59 insertions(+), 1 deletion(-) create mode 100644 arch/x86/kvm/mmu/tdp_mmu.c create mode 100644 arch/x86/kvm/mmu/tdp_mmu.h