[PATCH v4 11/26] KVM: Add arch hooks for PM events with empty stub

isaku.yamahata@intel.com posted 26 patches 3 years, 6 months ago
There is a newer version of this series
[PATCH v4 11/26] KVM: Add arch hooks for PM events with empty stub
Posted by isaku.yamahata@intel.com 3 years, 6 months ago
From: Isaku Yamahata <isaku.yamahata@intel.com>

Add arch hooks for reboot, suspend, resume, and CPU-online/offline events
with empty stub functions.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
---
 include/linux/kvm_host.h |  6 +++++
 virt/kvm/Makefile.kvm    |  2 +-
 virt/kvm/kvm_arch.c      | 44 ++++++++++++++++++++++++++++++
 virt/kvm/kvm_main.c      | 58 +++++++++++++++++++++++++---------------
 4 files changed, 88 insertions(+), 22 deletions(-)
 create mode 100644 virt/kvm/kvm_arch.c

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index eab352902de7..dd2a6d98d4de 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1448,6 +1448,12 @@ int kvm_arch_post_init_vm(struct kvm *kvm);
 void kvm_arch_pre_destroy_vm(struct kvm *kvm);
 int kvm_arch_create_vm_debugfs(struct kvm *kvm);
 
+int kvm_arch_suspend(int usage_count);
+void kvm_arch_resume(int usage_count);
+int kvm_arch_reboot(int val);
+int kvm_arch_online_cpu(unsigned int cpu, int usage_count);
+int kvm_arch_offline_cpu(unsigned int cpu, int usage_count);
+
 #ifndef __KVM_HAVE_ARCH_VM_ALLOC
 /*
  * All architectures that want to use vzalloc currently also
diff --git a/virt/kvm/Makefile.kvm b/virt/kvm/Makefile.kvm
index 2c27d5d0c367..c4210acabd35 100644
--- a/virt/kvm/Makefile.kvm
+++ b/virt/kvm/Makefile.kvm
@@ -5,7 +5,7 @@
 
 KVM ?= ../../../virt/kvm
 
-kvm-y := $(KVM)/kvm_main.o $(KVM)/eventfd.o $(KVM)/binary_stats.o
+kvm-y := $(KVM)/kvm_main.o $(KVM)/kvm_arch.o $(KVM)/eventfd.o $(KVM)/binary_stats.o
 kvm-$(CONFIG_KVM_VFIO) += $(KVM)/vfio.o
 kvm-$(CONFIG_KVM_MMIO) += $(KVM)/coalesced_mmio.o
 kvm-$(CONFIG_KVM_ASYNC_PF) += $(KVM)/async_pf.o
diff --git a/virt/kvm/kvm_arch.c b/virt/kvm/kvm_arch.c
new file mode 100644
index 000000000000..4748a76bcb03
--- /dev/null
+++ b/virt/kvm/kvm_arch.c
@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * kvm_arch.c: kvm default arch hooks for hardware enabling/disabling
+ * Copyright (c) 2022 Intel Corporation.
+ *
+ * Author:
+ *   Isaku Yamahata <isaku.yamahata@intel.com>
+ *                  <isaku.yamahata@gmail.com>
+ */
+
+#include <linux/kvm_host.h>
+
+/*
+ * Called after the VM is otherwise initialized, but just before adding it to
+ * the vm_list.
+ */
+__weak int kvm_arch_post_init_vm(struct kvm *kvm)
+{
+	return 0;
+}
+
+__weak int kvm_arch_online_cpu(unsigned int cpu, int usage_count)
+{
+	return 0;
+}
+
+__weak int kvm_arch_offline_cpu(unsigned int cpu, int usage_count)
+{
+	return 0;
+}
+
+__weak int kvm_arch_reboot(int val)
+{
+	return NOTIFY_OK;
+}
+
+__weak int kvm_arch_suspend(int usage_count)
+{
+	return 0;
+}
+
+__weak void kvm_arch_resume(int usage_count)
+{
+}
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 05ede37edc31..951f853f6ac9 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -144,6 +144,7 @@ static int kvm_no_compat_open(struct inode *inode, struct file *file)
 #endif
 static int hardware_enable_all(void);
 static void hardware_disable_all(void);
+static void hardware_disable_nolock(void *junk);
 
 static void kvm_io_bus_destroy(struct kvm_io_bus *bus);
 
@@ -1097,15 +1098,6 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, const char *fdname)
 	return ret;
 }
 
-/*
- * Called after the VM is otherwise initialized, but just before adding it to
- * the vm_list.
- */
-int __weak kvm_arch_post_init_vm(struct kvm *kvm)
-{
-	return 0;
-}
-
 /*
  * Called just after removing the VM from the vm_list, but before doing any
  * other destruction.
@@ -5040,6 +5032,10 @@ static int kvm_online_cpu(unsigned int cpu)
 		if (atomic_read(&hardware_enable_failed)) {
 			atomic_set(&hardware_enable_failed, 0);
 			ret = -EIO;
+		} else {
+			ret = kvm_arch_online_cpu(cpu, kvm_usage_count);
+			if (ret)
+				hardware_disable_nolock(NULL);
 		}
 	}
 	mutex_unlock(&kvm_lock);
@@ -5060,6 +5056,8 @@ static void hardware_disable_nolock(void *junk)
 
 static int kvm_offline_cpu(unsigned int cpu)
 {
+	int ret = 0;
+
 	mutex_lock(&kvm_lock);
 	if (kvm_usage_count) {
 		/*
@@ -5069,10 +5067,15 @@ static int kvm_offline_cpu(unsigned int cpu)
 		 */
 		preempt_disable();
 		hardware_disable_nolock(NULL);
+		ret = kvm_arch_offline_cpu(cpu, kvm_usage_count);
+		if (ret) {
+			(void)hardware_enable_nolock(NULL);
+			atomic_set(&hardware_enable_failed, 0);
+		}
 		preempt_enable();
 	}
 	mutex_unlock(&kvm_lock);
-	return 0;
+	return ret;
 }
 
 static void hardware_disable_all_nolock(void)
@@ -5130,6 +5133,8 @@ static int hardware_enable_all(void)
 static int kvm_reboot(struct notifier_block *notifier, unsigned long val,
 		      void *v)
 {
+	int r;
+
 	/*
 	 * Some (well, at least mine) BIOSes hang on reboot if
 	 * in vmx root mode.
@@ -5138,8 +5143,15 @@ static int kvm_reboot(struct notifier_block *notifier, unsigned long val,
 	 */
 	pr_info("kvm: exiting hardware virtualization\n");
 	kvm_rebooting = true;
+
+	/* This hook is called without cpuhotplug disabled.  */
+	cpus_read_lock();
+	mutex_lock(&kvm_lock);
 	on_each_cpu(hardware_disable_nolock, NULL, 1);
-	return NOTIFY_OK;
+	r = kvm_arch_reboot(val);
+	mutex_unlock(&kvm_lock);
+	cpus_read_unlock();
+	return r;
 }
 
 static struct notifier_block kvm_reboot_notifier = {
@@ -5728,6 +5740,8 @@ static void kvm_init_debug(void)
 
 static int kvm_suspend(void)
 {
+	int ret;
+
 	/*
 	 * The caller ensures that CPU hotlug is disabled by
 	 * cpu_hotplug_disable() and other CPUs are offlined.  No need for
@@ -5735,16 +5749,19 @@ static int kvm_suspend(void)
 	 */
 	lockdep_assert_not_held(&kvm_lock);
 
-	if (kvm_usage_count) {
-		preempt_disable();
+	preempt_disable();
+	if (kvm_usage_count)
 		hardware_disable_nolock(NULL);
-		preempt_enable();
-	}
-	return 0;
+	ret = kvm_arch_suspend(kvm_usage_count);
+	preempt_enable();
+
+	return ret;
 }
 
 static void kvm_resume(void)
 {
+	lockdep_assert_not_held(&kvm_lock);
+
 	if (kvm_arch_check_processor_compat())
 		/*
 		 * No warning here because kvm_arch_check_processor_compat()
@@ -5752,12 +5769,11 @@ static void kvm_resume(void)
 		 */
 		return; /* FIXME: disable KVM */
 
-	if (kvm_usage_count) {
-		lockdep_assert_not_held(&kvm_lock);
-		preempt_disable();
+	preempt_disable();
+	if (kvm_usage_count)
 		hardware_enable_nolock((void *)__func__);
-		preempt_enable();
-	}
+	kvm_arch_resume(kvm_usage_count);
+	preempt_enable();
 }
 
 static struct syscore_ops kvm_syscore_ops = {
-- 
2.25.1
Re: [PATCH v4 11/26] KVM: Add arch hooks for PM events with empty stub
Posted by Chao Gao 3 years, 6 months ago
On Thu, Sep 08, 2022 at 04:25:27PM -0700, isaku.yamahata@intel.com wrote:
>From: Isaku Yamahata <isaku.yamahata@intel.com>
>
>Add arch hooks for reboot, suspend, resume, and CPU-online/offline events
>with empty stub functions.
>
>Suggested-by: Sean Christopherson <seanjc@google.com>
>Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
>---
> include/linux/kvm_host.h |  6 +++++
> virt/kvm/Makefile.kvm    |  2 +-
> virt/kvm/kvm_arch.c      | 44 ++++++++++++++++++++++++++++++
> virt/kvm/kvm_main.c      | 58 +++++++++++++++++++++++++---------------
> 4 files changed, 88 insertions(+), 22 deletions(-)
> create mode 100644 virt/kvm/kvm_arch.c
>
>diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>index eab352902de7..dd2a6d98d4de 100644
>--- a/include/linux/kvm_host.h
>+++ b/include/linux/kvm_host.h
>@@ -1448,6 +1448,12 @@ int kvm_arch_post_init_vm(struct kvm *kvm);
> void kvm_arch_pre_destroy_vm(struct kvm *kvm);
> int kvm_arch_create_vm_debugfs(struct kvm *kvm);
> 
>+int kvm_arch_suspend(int usage_count);
>+void kvm_arch_resume(int usage_count);
>+int kvm_arch_reboot(int val);
>+int kvm_arch_online_cpu(unsigned int cpu, int usage_count);
>+int kvm_arch_offline_cpu(unsigned int cpu, int usage_count);

Why not extract each of them with one separate patch?

>+
> #ifndef __KVM_HAVE_ARCH_VM_ALLOC
> /*
>  * All architectures that want to use vzalloc currently also
>diff --git a/virt/kvm/Makefile.kvm b/virt/kvm/Makefile.kvm
>index 2c27d5d0c367..c4210acabd35 100644
>--- a/virt/kvm/Makefile.kvm
>+++ b/virt/kvm/Makefile.kvm
>@@ -5,7 +5,7 @@
> 
> KVM ?= ../../../virt/kvm
> 
>-kvm-y := $(KVM)/kvm_main.o $(KVM)/eventfd.o $(KVM)/binary_stats.o
>+kvm-y := $(KVM)/kvm_main.o $(KVM)/kvm_arch.o $(KVM)/eventfd.o $(KVM)/binary_stats.o
> kvm-$(CONFIG_KVM_VFIO) += $(KVM)/vfio.o
> kvm-$(CONFIG_KVM_MMIO) += $(KVM)/coalesced_mmio.o
> kvm-$(CONFIG_KVM_ASYNC_PF) += $(KVM)/async_pf.o
>diff --git a/virt/kvm/kvm_arch.c b/virt/kvm/kvm_arch.c
>new file mode 100644
>index 000000000000..4748a76bcb03
>--- /dev/null
>+++ b/virt/kvm/kvm_arch.c
>@@ -0,0 +1,44 @@
>+// SPDX-License-Identifier: GPL-2.0-only
>+/*
>+ * kvm_arch.c: kvm default arch hooks for hardware enabling/disabling
>+ * Copyright (c) 2022 Intel Corporation.
>+ *
>+ * Author:
>+ *   Isaku Yamahata <isaku.yamahata@intel.com>
>+ *                  <isaku.yamahata@gmail.com>
>+ */
>+
>+#include <linux/kvm_host.h>
>+
>+/*
>+ * Called after the VM is otherwise initialized, but just before adding it to
>+ * the vm_list.
>+ */
>+__weak int kvm_arch_post_init_vm(struct kvm *kvm)
>+{
>+	return 0;
>+}

use "int __weak" to comply with kernel's convension.

> 
> static int kvm_offline_cpu(unsigned int cpu)
> {
>+	int ret = 0;
>+
> 	mutex_lock(&kvm_lock);
> 	if (kvm_usage_count) {
> 		/*
>@@ -5069,10 +5067,15 @@ static int kvm_offline_cpu(unsigned int cpu)
> 		 */
> 		preempt_disable();
> 		hardware_disable_nolock(NULL);
>+		ret = kvm_arch_offline_cpu(cpu, kvm_usage_count);
>+		if (ret) {
>+			(void)hardware_enable_nolock(NULL);
>+			atomic_set(&hardware_enable_failed, 0);

The error-handling code ignores hardware enabling failure which looks
weird to me. If you extract kvm_arch_offline_cpu() directly like what
you do in patch 14 (rather than add a stub function first and then move
some code to the stub function), the error-handling code isn't needed.
Re: [PATCH v4 11/26] KVM: Add arch hooks for PM events with empty stub
Posted by Isaku Yamahata 3 years, 6 months ago
On Fri, Sep 09, 2022 at 12:34:39PM +0800,
Chao Gao <chao.gao@intel.com> wrote:

> On Thu, Sep 08, 2022 at 04:25:27PM -0700, isaku.yamahata@intel.com wrote:
> >From: Isaku Yamahata <isaku.yamahata@intel.com>
> >
> >Add arch hooks for reboot, suspend, resume, and CPU-online/offline events
> >with empty stub functions.
> >
> >Suggested-by: Sean Christopherson <seanjc@google.com>
> >Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> >---
> > include/linux/kvm_host.h |  6 +++++
> > virt/kvm/Makefile.kvm    |  2 +-
> > virt/kvm/kvm_arch.c      | 44 ++++++++++++++++++++++++++++++
> > virt/kvm/kvm_main.c      | 58 +++++++++++++++++++++++++---------------
> > 4 files changed, 88 insertions(+), 22 deletions(-)
> > create mode 100644 virt/kvm/kvm_arch.c
> >
> >diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> >index eab352902de7..dd2a6d98d4de 100644
> >--- a/include/linux/kvm_host.h
> >+++ b/include/linux/kvm_host.h
> >@@ -1448,6 +1448,12 @@ int kvm_arch_post_init_vm(struct kvm *kvm);
> > void kvm_arch_pre_destroy_vm(struct kvm *kvm);
> > int kvm_arch_create_vm_debugfs(struct kvm *kvm);
> > 
> >+int kvm_arch_suspend(int usage_count);
> >+void kvm_arch_resume(int usage_count);
> >+int kvm_arch_reboot(int val);
> >+int kvm_arch_online_cpu(unsigned int cpu, int usage_count);
> >+int kvm_arch_offline_cpu(unsigned int cpu, int usage_count);
> 
> Why not extract each of them with one separate patch?

Do you mean one patch for each arch callback?  They are convoluted.
See the comment below.


> >diff --git a/virt/kvm/kvm_arch.c b/virt/kvm/kvm_arch.c
> >new file mode 100644
> >index 000000000000..4748a76bcb03
> >--- /dev/null
> >+++ b/virt/kvm/kvm_arch.c
> >@@ -0,0 +1,44 @@
> >+// SPDX-License-Identifier: GPL-2.0-only
> >+/*
> >+ * kvm_arch.c: kvm default arch hooks for hardware enabling/disabling
> >+ * Copyright (c) 2022 Intel Corporation.
> >+ *
> >+ * Author:
> >+ *   Isaku Yamahata <isaku.yamahata@intel.com>
> >+ *                  <isaku.yamahata@gmail.com>
> >+ */
> >+
> >+#include <linux/kvm_host.h>
> >+
> >+/*
> >+ * Called after the VM is otherwise initialized, but just before adding it to
> >+ * the vm_list.
> >+ */
> >+__weak int kvm_arch_post_init_vm(struct kvm *kvm)
> >+{
> >+	return 0;
> >+}
> 
> use "int __weak" to comply with kernel's convension.

Will fix.

> > static int kvm_offline_cpu(unsigned int cpu)
> > {
> >+	int ret = 0;
> >+
> > 	mutex_lock(&kvm_lock);
> > 	if (kvm_usage_count) {
> > 		/*
> >@@ -5069,10 +5067,15 @@ static int kvm_offline_cpu(unsigned int cpu)
> > 		 */
> > 		preempt_disable();
> > 		hardware_disable_nolock(NULL);
> >+		ret = kvm_arch_offline_cpu(cpu, kvm_usage_count);
> >+		if (ret) {
> >+			(void)hardware_enable_nolock(NULL);
> >+			atomic_set(&hardware_enable_failed, 0);
> 
> The error-handling code ignores hardware enabling failure which looks
> weird to me. If you extract kvm_arch_offline_cpu() directly like what
> you do in patch 14 (rather than add a stub function first and then move
> some code to the stub function), the error-handling code isn't needed.

I did it for x86 tsc fix.  It relates to suspend/resume.  I would split those
- introduce suspend/resuem/reboot arch hooks
- fix x86 tsc issue
- move
- introduce cpu online/offline arch hooks
- move out PM hooks. probably this can be combined into the previous one.


-- 
Isaku Yamahata <isaku.yamahata@gmail.com>