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

[Chao Gao]

On Thu, Sep 08, 2022 at 04:25:27PM -0700, isaku.yamahata@xxxxxxxxx wrote:
>From: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
>
>Add arch hooks for reboot, suspend, resume, and CPU-online/offline events
>with empty stub functions.
>
>Suggested-by: Sean Christopherson <seanjc@xxxxxxxxxx>
>Signed-off-by: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
>---
> 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@xxxxxxxxx>
>+ *                  <isaku.yamahata@xxxxxxxxx>
>+ */
>+
>+#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.