Re: [PATCH 03/22] kvm: mmu: Init / Uninit the TDP MMU
On Wed, Sep 30, 2020 at 10:39 AM Paolo Bonzini 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)
Re: [PATCH 03/22] kvm: mmu: Init / Uninit the TDP MMU
On Tue, Sep 29, 2020 at 10:35 PM Sean Christopherson
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 0..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;
> > +}
Re: [PATCH 03/22] kvm: mmu: Init / Uninit the TDP MMU
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
Re: [PATCH 03/22] kvm: mmu: Init / Uninit the TDP 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 0..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: [#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:c999fb50 EFLAGS: 00010246 RAX: RBX: 1000 RCX: RDX: c999fb60 RSI: c999fb58 RDI: 88816b1a7ec8 RBP: 88816b1a7e70 R08: 888173c95000 R09: 88816b1a7448 R10: 00f8 R11: 88817bd29c70 R12: c9981000 R13: c999fbac R14: c9989a88 R15: 88816b1a7ec8 FS: 7f7a755fd700() GS:88817bd0() knlGS: CS: 0010 DS: ES: CR0: 80050033 CR2: 7f7a60141000 CR3: 00016b031004 CR4: 00172ea0 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
Re: [PATCH 03/22] kvm: mmu: Init / Uninit the TDP MMU
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 0..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;
> +}
Re: [PATCH 03/22] kvm: mmu: Init / Uninit the TDP MMU
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
[PATCH 03/22] kvm: mmu: Init / Uninit the TDP MMU
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
---
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
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 = >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 = >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 0..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 0..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
+
+void kvm_mmu_init_tdp_mmu(struct kvm *kvm);
+void
