On Fri, Sep 09, 2022 at 12:34:39PM +0800,
Chao Gao <chao.gao@xxxxxxxxx> wrote:
> 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?
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@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.
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@xxxxxxxxx>
