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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220911023945.GD699006@ls.amr.corp.intel.com>
Date:   Sat, 10 Sep 2022 19:39:45 -0700
From:   Isaku Yamahata <isaku.yamahata@...il.com>
To:     Chao Gao <chao.gao@...el.com>
Cc:     isaku.yamahata@...el.com, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Marc Zyngier <maz@...nel.org>, Will Deacon <will@...nel.org>,
        Yuan Yao <yuan.yao@...ux.intel.com>, isaku.yamahata@...il.com,
        Kai Huang <kai.huang@...el.com>,
        Atish Patra <atishp@...shpatra.org>,
        Shaokun Zhang <zhangshaokun@...ilicon.com>,
        Qi Liu <liuqi115@...wei.com>,
        John Garry <john.garry@...wei.com>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Huang Ying <ying.huang@...el.com>,
        Huacai Chen <chenhuacai@...nel.org>
Subject: Re: [PATCH v4 11/26] KVM: Add arch hooks for PM events with empty
 stub

On Fri, Sep 09, 2022 at 12:34:39PM +0800,
Chao Gao <chao.gao@...el.com> wrote:

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