On Tue, Sep 06, 2022 at 02:25:02PM +0800, Yuan Yao <yuan.yao@xxxxxxxxxxxxxxx> wrote: > On Thu, Sep 01, 2022 at 07:17:46PM -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 | 50 +++++++++++++++++++++++++--------------- > > 4 files changed, 82 insertions(+), 20 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@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; > > +} > > + > > +__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 082d5dbc8d7f..e62240fb8474 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. > > @@ -5033,6 +5025,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); > > @@ -5053,11 +5049,19 @@ 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) > > + if (kvm_usage_count) { > > 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); > > + } > > + } > > mutex_unlock(&kvm_lock); > > - return 0; > > + return ret; > > } > > > > static void hardware_disable_all_nolock(void) > > @@ -5115,6 +5119,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. > > @@ -5123,8 +5129,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 = { > > @@ -5718,11 +5731,10 @@ static int kvm_suspend(void) > > * cpu_hotplug_disable() and other CPUs are offlined. No need for > > * locking. > > */ > > - if (kvm_usage_count) { > > - lockdep_assert_not_held(&kvm_lock); > > + lockdep_assert_not_held(&kvm_lock); > > + if (kvm_usage_count) > > hardware_disable_nolock(NULL); > > - } > > - return 0; > > + return kvm_arch_suspend(kvm_usage_count); > > } > > > e static void kvm_resume(void) > > @@ -5734,10 +5746,10 @@ static void kvm_resume(void) > > */ > > return; /* FIXME: disable KVM */ > > > > - if (kvm_usage_count) { > > - lockdep_assert_not_held(&kvm_lock); > > + lockdep_assert_not_held(&kvm_lock); > > + kvm_arch_resume(kvm_usage_count); > > + if (kvm_usage_count) > > hardware_enable_nolock((void *)__func__); > > Is single kvm_arch_{suspend,resume} enough? > > The sequence is: > > kvm_arch_resume() > hardware_enable_nolock() > > But in patch 12 I see below code in x86's kvm_arch_resume(): > ... > if (!usage_count) > return; > if (kvm_arch_hardware_enable()) > return; > ... > > So kvm_arch_resume() may depend on hardware enable, and it checks the > usage_count again, how about call kvm_arch_resume(bool > hardware_enabled) before/after the hardware_enable_nolock() (or even > give different functions with _before/_after suffix) for better > flexibility since it's common code for all architectures, e.g. > > if (kvm_usage_count) { > kvm_arch_resume(false); > hardware_enable_nolock(__func__); > kvm_arch_resumt(true); > } Nice catch. I fixed it as follows. kvm_resume() if (kvm_usage_count) hardware_enable_nolock() kvm_arch_resume() x86 kvm_arch_resume() doesn't call hardware_enable(). The patch of adjust compatibility check and hardware_enable(). -- Isaku Yamahata <isaku.yamahata@xxxxxxxxx>