Re: [RFC V3 PATCH 4/6] selftests: kvm: x86: Execute hypercall as per the cpu

[Sean Christopherson]

On Fri, Aug 19, 2022, Vishal Annapurve wrote:
> Add support for executing vmmcall/vmcall instruction on amd/intel cpus.
> In general kvm patches the instruction according to the cpu
> implementation at runtime. While executing selftest vms from private
> memory KVM will not be able to update the private memory of the guest.
> 
> Hypercall parameters are fixed to explicitly populate hypercall number
> in eax. Otherwise inlined function calls to kvm_hypercall would call
> vmmcall/vmcall instruction without updating eax with hypercall number.

Can you send a seperate non-RFC series to clean up the selftests mess?  kvm_hypercall()
isn't the only culprit.

  git grep \"vmcall | wc -l
  16

I'm pretty sure things work only because of KVM's dubious behavior of patching
VMCALL/VMMCALL by default.

Note, svm_vmcall_test.c intentionally uses the wrong instructions and shouldn't
be converted.  Actually, we can and should just delete that test, it's superseded
by the wonderfully named fix_hypercall_test.

> Signed-off-by: Vishal Annapurve <vannapurve@xxxxxxxxxx>
> ---
>  .../testing/selftests/kvm/lib/x86_64/processor.c  | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> index 53b115876417..09d757a0b148 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> @@ -1254,10 +1254,21 @@ uint64_t kvm_hypercall(uint64_t nr, uint64_t a0, uint64_t a1, uint64_t a2,
>  		       uint64_t a3)
>  {
>  	uint64_t r;
> +	static bool is_cpu_checked;
> +	static bool is_cpu_amd;
>  
> -	asm volatile("vmcall"
> +	if (!is_cpu_checked)
> +		is_cpu_amd = is_amd_cpu();

This can be done using a single int, e.g.

	static bool is_cpu_amd = -1;

	if (is_cpu_amd < 0)
		is_cpu_amd = is_amd_cpu();

Although... what if we declare main() in lib/kvm_util.c (or maybe a dedicated
file?) and rename all tests to use __main()?  Then add an arch hook to do global
initialization and avoid the "did we check CPUID?!?!?" altogether.

That would allow us to dedup all of the hilarious copy paste:

	/* Tell stdout not to buffer its content */
	setbuf(stdout, NULL);

and we could turn is_amd_cpu() and is_intel_cpu() into bools.

E.g.

int main(int argc, char *argv[])
{
	/* Tell stdout not to buffer its content */
	setbuf(stdout, NULL);

	kvm_arch_main();

	return __main(argc, argv);
}

void kvm_arch_main(void)
{
	is_cpu_amd = cpu_vendor_string_is("AuthenticAMD");
	is_cpu_intel = cpu_vendor_string_is("AuthenticAMD");
}


And then we just need macro magic to emit the right VMCALL/VMMCALL instruction.