Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 10 Sep 2022 19:39:45 -0700
From:   Isaku Yamahata <isaku.yamahata@gmail.com>
To:     Chao Gao <chao.gao@intel.com>
Cc:     isaku.yamahata@intel.com, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
        Sean Christopherson <seanjc@google.com>,
        Marc Zyngier <maz@kernel.org>, Will Deacon <will@kernel.org>,
        Yuan Yao <yuan.yao@linux.intel.com>, isaku.yamahata@gmail.com,
        Kai Huang <kai.huang@intel.com>,
        Atish Patra <atishp@atishpatra.org>,
        Shaokun Zhang <zhangshaokun@hisilicon.com>,
        Qi Liu <liuqi115@huawei.com>,
        John Garry <john.garry@huawei.com>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        Huang Ying <ying.huang@intel.com>,
        Huacai Chen <chenhuacai@kernel.org>
Subject: Re: [PATCH v4 11/26] KVM: Add arch hooks for PM events with empty
 stub
Message-ID: <20220911023945.GD699006@ls.amr.corp.intel.com>
References: <cover.1662679124.git.isaku.yamahata@intel.com>
 <fa7ecd7305d011940121466f094a544c6de39ea3.1662679124.git.isaku.yamahata@intel.com>
 <YxrCX2nhOyWbbGGY@gao-cwp>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <YxrCX2nhOyWbbGGY@gao-cwp>
Precedence: bulk

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>