Re: [PATCH v3 16/22] KVM: kvm_arch.c: Remove a global variable, hardware_enable_failed

[Yuan Yao]

On Thu, Sep 01, 2022 at 07:17:51PM -0700, isaku.yamahata@xxxxxxxxx wrote:
> From: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
>
> A global variable hardware_enable_failed in kvm_arch.c is used only by
> kvm_arch_add_vm() and hardware_enable().  It doesn't have to be a global
> variable.  Make it function local.
>
> Signed-off-by: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> ---
>  virt/kvm/kvm_arch.c | 49 +++++++++++++++++++++------------------------
>  1 file changed, 23 insertions(+), 26 deletions(-)
>
> diff --git a/virt/kvm/kvm_arch.c b/virt/kvm/kvm_arch.c
> index de39d0127584..f7dcde842eb5 100644
> --- a/virt/kvm/kvm_arch.c
> +++ b/virt/kvm/kvm_arch.c
> @@ -13,14 +13,13 @@
>  #include <linux/kvm_host.h>
>
>  static cpumask_t cpus_hardware_enabled = CPU_MASK_NONE;
> -static atomic_t hardware_enable_failed;
>
>  __weak int kvm_arch_post_init_vm(struct kvm *kvm)
>  {
>  	return 0;
>  }
>
> -static void hardware_enable(void *caller_name)
> +static int __hardware_enable(void *caller_name)
>  {
>  	int cpu = raw_smp_processor_id();
>  	int r;
> @@ -28,18 +27,22 @@ static void hardware_enable(void *caller_name)
>  	WARN_ON_ONCE(preemptible());
>
>  	if (cpumask_test_cpu(cpu, &cpus_hardware_enabled))
> -		return;
> -
> -	cpumask_set_cpu(cpu, &cpus_hardware_enabled);
> -
> +		return 0;
>  	r = kvm_arch_hardware_enable();
> -
> -	if (r) {
> -		cpumask_clear_cpu(cpu, &cpus_hardware_enabled);
> -		atomic_inc(&hardware_enable_failed);
> +	if (r)
>  		pr_warn("kvm: enabling virtualization on CPU%d failed during %s()\n",
>  			cpu, (const char *)caller_name);
> -	}
> +	else
> +		cpumask_set_cpu(cpu, &cpus_hardware_enabled);
> +	return r;
> +}
> +
> +static void hardware_enable(void *arg)
> +{
> +	atomic_t *failed = arg;
> +
> +	if (__hardware_enable((void *)__func__))
> +		atomic_inc(failed);
>  }

A side effect: The actual caller_name information introduced in Patch
3 for hardware_enable() is lost. I tend to keep the information, but
depends on you and other guys. :-)

>
>  static void hardware_disable(void *junk)
> @@ -65,15 +68,16 @@ __weak void kvm_arch_pre_hardware_unsetup(void)
>   */
>  __weak int kvm_arch_add_vm(struct kvm *kvm, int usage_count)
>  {
> +	atomic_t failed;
>  	int r = 0;
>
>  	if (usage_count != 1)
>  		return 0;
>
> -	atomic_set(&hardware_enable_failed, 0);
> -	on_each_cpu(hardware_enable, (void *)__func__, 1);
> +	atomic_set(&failed, 0);
> +	on_each_cpu(hardware_enable, &failed, 1);
>
> -	if (atomic_read(&hardware_enable_failed)) {
> +	if (atomic_read(&failed)) {
>  		r = -EBUSY;
>  		goto err;
>  	}
> @@ -96,27 +100,20 @@ __weak int kvm_arch_del_vm(int usage_count)
>
>  __weak int kvm_arch_online_cpu(unsigned int cpu, int usage_count)
>  {
> -	int ret = 0;
> +	int ret;
>
>  	ret = kvm_arch_check_processor_compat();
>  	if (ret)
>  		return ret;
>
> +	if (!usage_count)
> +		return 0;
>  	/*
>  	 * Abort the CPU online process if hardware virtualization cannot
>  	 * be enabled. Otherwise running VMs would encounter unrecoverable
>  	 * errors when scheduled to this CPU.
>  	 */
> -	if (usage_count) {
> -		WARN_ON_ONCE(atomic_read(&hardware_enable_failed));
> -
> -		hardware_enable((void *)__func__);
> -		if (atomic_read(&hardware_enable_failed)) {
> -			atomic_set(&hardware_enable_failed, 0);
> -			ret = -EIO;
> -		}
> -	}
> -	return ret;
> +	return __hardware_enable((void *)__func__);
>  }
>
>  __weak int kvm_arch_offline_cpu(unsigned int cpu, int usage_count)
> @@ -149,5 +146,5 @@ __weak void kvm_arch_resume(int usage_count)
>  		return; /* FIXME: disable KVM */
>
>  	if (usage_count)
> -		hardware_enable((void *)__func__);
> +		(void)__hardware_enable((void *)__func__);
>  }
> --
> 2.25.1
>