Re: [PATCH v3 07/10] KVM: arm64: selftests: Finish generalizing get-reg-list

From: Andrew Jones <ajones@ventanamicro.com>
To: Haibo Xu <haibo1.xu@intel.com>
Cc: xiaobo55x@gmail.com, maz@kernel.org, oliver.upton@linux.dev,
	seanjc@google.com, Paolo Bonzini <pbonzini@redhat.com>,
	Jonathan Corbet <corbet@lwn.net>,
	Anup Patel <anup@brainfault.org>,
	Atish Patra <atishp@atishpatra.org>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Albert Ou <aou@eecs.berkeley.edu>, Shuah Khan <shuah@kernel.org>,
	James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Zenghui Yu <yuzenghui@huawei.com>,
	David Matlack <dmatlack@google.com>,
	Ben Gardon <bgardon@google.com>,
	Vipin Sharma <vipinsh@google.com>,
	Colton Lewis <coltonlewis@google.com>,
	kvm@vger.kernel.org, linux-doc@vger.kernel.org,
	linux-kernel@vger.kernel.org, kvm-riscv@lists.infradead.org,
	linux-riscv@lists.infradead.org, linux-kselftest@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev
Subject: Re: [PATCH v3 07/10] KVM: arm64: selftests: Finish generalizing get-reg-list
Date: Fri, 9 Jun 2023 14:30:41 +0200	[thread overview]
Message-ID: <20230609-b900162a66c26a004b751b1f@orel> (raw)
In-Reply-To: <450cb59db52ebeaa68f3d77f1bd995618f3612b8.1686275310.git.haibo1.xu@intel.com>

On Fri, Jun 09, 2023 at 10:12:15AM +0800, Haibo Xu wrote:
> From: Andrew Jones <ajones@ventanamicro.com>
> 
> Add some unfortunate #ifdeffery to ensure the common get-reg-list.c
> can be compiled and run with other architectures. The next
> architecture to support get-reg-list should now only need to provide
> $(ARCH_DIR)/get-reg-list.c where arch-specific print_reg() and
> vcpu_configs[] get defined.
> 
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> Signed-off-by: Haibo Xu <haibo1.xu@intel.com>
> ---
>  tools/testing/selftests/kvm/get-reg-list.c | 24 ++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/get-reg-list.c b/tools/testing/selftests/kvm/get-reg-list.c
> index 69bb91087081..c4bd5a5259da 100644
> --- a/tools/testing/selftests/kvm/get-reg-list.c
> +++ b/tools/testing/selftests/kvm/get-reg-list.c
> @@ -98,6 +98,7 @@ void __weak print_reg(const char *prefix, __u64 id)
>  	printf("\t0x%llx,\n", id);
>  }
>  
> +#ifdef __aarch64__
>  static void prepare_vcpu_init(struct vcpu_reg_list *c, struct kvm_vcpu_init *init)
>  {
>  	struct vcpu_reg_sublist *s;
> @@ -120,6 +121,24 @@ static void finalize_vcpu(struct kvm_vcpu *vcpu, struct vcpu_reg_list *c)
>  	}
>  }
>  
> +static struct kvm_vcpu *vcpu_config_get_vcpu(struct vcpu_reg_list *c, struct kvm_vm *vm)
> +{
> +	struct kvm_vcpu_init init = { .target = -1, };
> +	struct kvm_vcpu *vcpu;
> +
> +	prepare_vcpu_init(c, &init);
> +	vcpu = __vm_vcpu_add(vm, 0);
> +	aarch64_vcpu_setup(vcpu, &init);
> +
> +	return vcpu;
> +}
> +#else
> +static struct kvm_vcpu *vcpu_config_get_vcpu(struct vcpu_reg_list *c, struct kvm_vm *vm)
> +{
> +	return __vm_vcpu_add(vm, 0);
> +}
> +#endif
> +
>  static void check_supported(struct vcpu_reg_list *c)
>  {
>  	struct vcpu_reg_sublist *s;
> @@ -139,7 +158,6 @@ static bool print_filtered;
>  
>  static void run_test(struct vcpu_reg_list *c)
>  {
> -	struct kvm_vcpu_init init = { .target = -1, };
>  	int new_regs = 0, missing_regs = 0, i, n;
>  	int failed_get = 0, failed_set = 0, failed_reject = 0;
>  	struct kvm_vcpu *vcpu;
> @@ -149,9 +167,7 @@ static void run_test(struct vcpu_reg_list *c)
>  	check_supported(c);
>  
>  	vm = vm_create_barebones();
> -	prepare_vcpu_init(c, &init);
> -	vcpu = __vm_vcpu_add(vm, 0);
> -	aarch64_vcpu_setup(vcpu, &init);
> +	vcpu = vcpu_config_get_vcpu(c, vm);
>  	finalize_vcpu(vcpu, c);

I just noticed that this has been modified from what I posted to leave
the finalize_vcpu() call here, despite it now being inside the #ifdef
__aarch64__. That breaks the purpose of the patch. Please make sure this
file compiles for other architectures without requiring additional
patches, which would keep the commit message honest. You can either
revert this to what I posted, and then readd the finalize_vcpu() call in
another patch, or you can add a finalize_vcpu() stub to the #else part
of the ifdef in this patch.

Also please don't modify patches authored by others without calling out
the modifications somewhere, either the commit message or under the ---
of the patch or in the cover letter.

Thanks,
drew