Re: [PATCH v8 002/103] Partially revert "KVM: Pass kvm_init()'s opaque param to additional arch funcs"

[Huang, Kai]

On Sun, 2022-08-07 at 15:00 -0700, isaku.yamahata@xxxxxxxxx wrote:
> From: Chao Gao <chao.gao@xxxxxxxxx>
> 
> This partially reverts commit b99040853738 ("KVM: Pass kvm_init()'s opaque
> param to additional arch funcs") remove opaque from
> kvm_arch_check_processor_compat because no one uses this opaque now.
> Address conflicts for ARM (due to file movement) and manually handle RISC-V
> which comes after the commit.  The change about kvm_arch_hardware_setup()
> in original commit are still needed so they are not reverted.
> 
> The current implementation enables hardware (e.g. enable VMX on all CPUs),
> arch-specific initialization for VM creation, 
> 

I guess you need to point out _first_ VM?

> and disables hardware (in
> x86, disable VMX on all CPUs) for last VM destruction.
> 
> TDX requires its initialization on loading KVM module with VMX enabled on
> all available CPUs. It needs to enable/disable hardware on module
> initialization.  To reuse the same logic, one way is to pass around the

To reuse the same logic for what?  I think you need to be specific (and focus)
on why we need this patch:  we will opportunistically move CPU compatibility
check to hardware_enable_nolock(), which doesn't take any argument, and this
patch is a preparation to do that.


> unused opaque argument, another way is to remove the unused opaque
> argument.  This patch is a preparation for the latter by removing the
> argument

So how about replacing the last two paragraphs with:

"
Initializing TDX will be done during module loading time, and in order to do
that hardware_enable_all() will be done during module loading time too, as
initializing TDX requires all cpus being in VMX operation.  As a result, CPU
compatibility check will be opportunistically moved to hardware_enable_nolock(),
which doesn't take any argument.  Instead of passing 'opaque' around to
hardware_enable_nolock() and hardware_enable_all(), just remove the unused
'opaque' argument from kvm_arch_check_processor_compat().
"

Or even simpler:

"
To support TDX, hardware_enable_all() will be done during module loading time. 
As a result, CPU compatibility check will be opportunistically moved to
hardware_enable_nolock(), which doesn't take any argument.  Instead of passing
'opaque' around to hardware_enable_nolock() and hardware_enable_all(), just
remove the unused 'opaque' argument from kvm_arch_check_processor_compat().
"

With changelog updated:

Reviewed-by: Kai Huang <kai.huang@xxxxxxxxx>

> 
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> Reviewed-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> Reviewed-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> Acked-by: Anup Patel <anup@xxxxxxxxxxxxxx>
> Acked-by: Claudio Imbrenda <imbrenda@xxxxxxxxxxxxx>
> Link: https://lore.kernel.org/r/20220216031528.92558-3-chao.gao@xxxxxxxxx
> Signed-off-by: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> ---
>  arch/arm64/kvm/arm.c       |  2 +-
>  arch/mips/kvm/mips.c       |  2 +-
>  arch/powerpc/kvm/powerpc.c |  2 +-
>  arch/riscv/kvm/main.c      |  2 +-
>  arch/s390/kvm/kvm-s390.c   |  2 +-
>  arch/x86/kvm/x86.c         |  2 +-
>  include/linux/kvm_host.h   |  2 +-
>  virt/kvm/kvm_main.c        | 16 +++-------------
>  8 files changed, 10 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 83a7f61354d3..c551ca587f16 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -68,7 +68,7 @@ int kvm_arch_hardware_setup(void *opaque)
>  	return 0;
>  }
>  
> -int kvm_arch_check_processor_compat(void *opaque)
> +int kvm_arch_check_processor_compat(void)
>  {
>  	return 0;
>  }
> diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c
> index a25e0b73ee70..092d09fb6a7e 100644
> --- a/arch/mips/kvm/mips.c
> +++ b/arch/mips/kvm/mips.c
> @@ -140,7 +140,7 @@ int kvm_arch_hardware_setup(void *opaque)
>  	return 0;
>  }
>  
> -int kvm_arch_check_processor_compat(void *opaque)
> +int kvm_arch_check_processor_compat(void)
>  {
>  	return 0;
>  }
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 191992fcb2c2..ca8ef51092c6 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -446,7 +446,7 @@ int kvm_arch_hardware_setup(void *opaque)
>  	return 0;
>  }
>  
> -int kvm_arch_check_processor_compat(void *opaque)
> +int kvm_arch_check_processor_compat(void)
>  {
>  	return kvmppc_core_check_processor_compat();
>  }
> diff --git a/arch/riscv/kvm/main.c b/arch/riscv/kvm/main.c
> index 1549205fe5fe..f8d6372d208f 100644
> --- a/arch/riscv/kvm/main.c
> +++ b/arch/riscv/kvm/main.c
> @@ -20,7 +20,7 @@ long kvm_arch_dev_ioctl(struct file *filp,
>  	return -EINVAL;
>  }
>  
> -int kvm_arch_check_processor_compat(void *opaque)
> +int kvm_arch_check_processor_compat(void)
>  {
>  	return 0;
>  }
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index edfd4bbd0cba..e26d4dd85668 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -254,7 +254,7 @@ int kvm_arch_hardware_enable(void)
>  	return 0;
>  }
>  
> -int kvm_arch_check_processor_compat(void *opaque)
> +int kvm_arch_check_processor_compat(void)
>  {
>  	return 0;
>  }
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index f5ff9b28e119..e533cce7a70b 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -11990,7 +11990,7 @@ void kvm_arch_hardware_unsetup(void)
>  	static_call(kvm_x86_hardware_unsetup)();
>  }
>  
> -int kvm_arch_check_processor_compat(void *opaque)
> +int kvm_arch_check_processor_compat(void)
>  {
>  	struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
>  
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 1c480b1821e1..9643b8eadefe 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1438,7 +1438,7 @@ int kvm_arch_hardware_enable(void);
>  void kvm_arch_hardware_disable(void);
>  int kvm_arch_hardware_setup(void *opaque);
>  void kvm_arch_hardware_unsetup(void);
> -int kvm_arch_check_processor_compat(void *opaque);
> +int kvm_arch_check_processor_compat(void);
>  int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu);
>  bool kvm_arch_vcpu_in_kernel(struct kvm_vcpu *vcpu);
>  int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu);
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index da263c370d00..0f5767e5ae45 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -5793,22 +5793,14 @@ void kvm_unregister_perf_callbacks(void)
>  }
>  #endif
>  
> -struct kvm_cpu_compat_check {
> -	void *opaque;
> -	int *ret;
> -};
> -
> -static void check_processor_compat(void *data)
> +static void check_processor_compat(void *rtn)
>  {
> -	struct kvm_cpu_compat_check *c = data;
> -
> -	*c->ret = kvm_arch_check_processor_compat(c->opaque);
> +	*(int *)rtn = kvm_arch_check_processor_compat();
>  }
>  
>  int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
>  		  struct module *module)
>  {
> -	struct kvm_cpu_compat_check c;
>  	int r;
>  	int cpu;
>  
> @@ -5836,10 +5828,8 @@ int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
>  	if (r < 0)
>  		goto out_free_1;
>  
> -	c.ret = &r;
> -	c.opaque = opaque;
>  	for_each_online_cpu(cpu) {
> -		smp_call_function_single(cpu, check_processor_compat, &c, 1);
> +		smp_call_function_single(cpu, check_processor_compat, &r, 1);
>  		if (r < 0)
>  			goto out_free_2;
>  	}