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:   Mon, 3 Oct 2022 17:16:21 -0700
From:   Isaku Yamahata <isaku.yamahata@gmail.com>
To:     isaku.yamahata@intel.com
Cc:     linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        Paolo Bonzini <pbonzini@redhat.com>,
        Sean Christopherson <seanjc@google.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Marc Zyngier <maz@kernel.org>, Will Deacon <will@kernel.org>,
        isaku.yamahata@gmail.com, Kai Huang <kai.huang@intel.com>,
        Chao Gao <chao.gao@intel.com>,
        Atish Patra <atishp@atishpatra.org>,
        Shaokun Zhang <zhangshaokun@hisilicon.com>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        Huang Ying <ying.huang@intel.com>,
        Huacai Chen <chenhuacai@kernel.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Borislav Petkov <bp@alien8.de>
Subject: Re: [PATCH v5 10/30] KVM: Add arch hooks when VM is added/deleted
Message-ID: <20221004001621.GJ2414580@ls.amr.corp.intel.com>
References: <cover.1663869838.git.isaku.yamahata@intel.com>
 <aab342d576fe22b8f5b27e61d4fc635d45a4f866.1663869838.git.isaku.yamahata@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <aab342d576fe22b8f5b27e61d4fc635d45a4f866.1663869838.git.isaku.yamahata@intel.com>
Precedence: bulk

On Thu, Sep 22, 2022 at 11:20:39AM -0700,
isaku.yamahata@intel.com wrote:

> From: Isaku Yamahata <isaku.yamahata@intel.com>
> 
> and pass kvm_usage_count with kvm_lock.  Move kvm_arch_post_init_vm() under
> kvm_arch_add_vm().  Replace enable/disable_hardware_all() with the default
> implementation of kvm_arch_add/del_vm().  Later kvm_arch_post_init_vm() is
> deleted once x86 overrides kvm_arch_add_vm().
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> ---
>  include/linux/kvm_host.h |   2 +
>  virt/kvm/kvm_main.c      | 121 ++++++++++++++++++++-------------------
>  2 files changed, 65 insertions(+), 58 deletions(-)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index eab352902de7..3fbb01bbac98 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1445,6 +1445,8 @@ int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu);
>  bool kvm_arch_dy_runnable(struct kvm_vcpu *vcpu);
>  bool kvm_arch_dy_has_pending_interrupt(struct kvm_vcpu *vcpu);
>  int kvm_arch_post_init_vm(struct kvm *kvm);
> +int kvm_arch_add_vm(struct kvm *kvm, int usage_count);
> +int kvm_arch_del_vm(int usage_count);
>  void kvm_arch_pre_destroy_vm(struct kvm *kvm);
>  int kvm_arch_create_vm_debugfs(struct kvm *kvm);
>  
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index c4b908553726..e2c8823786ff 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -142,8 +142,9 @@ static int kvm_no_compat_open(struct inode *inode, struct file *file)
>  #define KVM_COMPAT(c)	.compat_ioctl	= kvm_no_compat_ioctl,	\
>  			.open		= kvm_no_compat_open
>  #endif
> -static int hardware_enable_all(void);
> -static void hardware_disable_all(void);
> +static void hardware_enable_nolock(void *junk);
> +static void hardware_disable_nolock(void *junk);
> +static void kvm_del_vm(void);
>  
>  static void kvm_io_bus_destroy(struct kvm_io_bus *bus);
>  
> @@ -1106,6 +1107,41 @@ int __weak kvm_arch_post_init_vm(struct kvm *kvm)
>  	return 0;
>  }
>  
> +/*
> + * Called after the VM is otherwise initialized, but just before adding it to
> + * the vm_list.
> + */
> +int __weak kvm_arch_add_vm(struct kvm *kvm, int usage_count)
> +{
> +	int r = 0;
> +
> +	if (usage_count != 1)
> +		return 0;

Oops. This line should be.
+               return kvm_arch_post_init_vm(kvm);

"KVM: x86: Duplicate arch callbacks related to pm events and compat check"
should have same line. "KVM: Eliminate kvm_arch_post_init_vm()" should include
kvm_mmu_post_init_vm().
It creates a kernel thread. kvm unit test didn't catch it.

Thanks,

> +
> +	atomic_set(&hardware_enable_failed, 0);
> +	on_each_cpu(hardware_enable_nolock, NULL, 1);
> +
> +	if (atomic_read(&hardware_enable_failed)) {
> +		r = -EBUSY;
> +		goto err;
> +	}
> +
> +	r = kvm_arch_post_init_vm(kvm);
> +err:
> +	if (r)
> +		on_each_cpu(hardware_disable_nolock, NULL, 1);
> +	return r;
> +}
> +
> +int __weak kvm_arch_del_vm(int usage_count)
> +{
> +	if (usage_count)
> +		return 0;
> +
> +	on_each_cpu(hardware_disable_nolock, NULL, 1);
> +	return 0;
> +}
> +
>  /*
>   * Called just after removing the VM from the vm_list, but before doing any
>   * other destruction.
-- 
Isaku Yamahata <isaku.yamahata@gmail.com>