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);
}
> - }
> }
>
> static struct syscore_ops kvm_syscore_ops = {
> --
> 2.25.1
>
