Re: [V3 PATCH 2/2] KVM: selftests: x86: Add native hypercall support

[Sean Christopherson <seanjc@google.com>]

On Thu, Dec 22, 2022, Vishal Annapurve wrote:
> Add an API to execute hypercall as per the cpu type by checking the
> underlying CPU. KVM emulates vmcall/vmmcall instruction by modifying
> guest memory contents with hypercall instruction as per the cpu type.
> 
> Confidential VMs need to execute hypercall instruction without it being
> emulated by KVM as KVM can not modify guest memory contents.
> 
> Signed-off-by: Vishal Annapurve <vannapurve@google.com>
> ---
>  .../selftests/kvm/include/x86_64/processor.h   |  3 +++
>  .../selftests/kvm/lib/x86_64/processor.c       | 18 ++++++++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
> index 4d5dd9a467e1..3617f83bb2e5 100644
> --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
> +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
> @@ -1039,6 +1039,9 @@ uint64_t *vm_get_page_table_entry(struct kvm_vm *vm, uint64_t vaddr);
>  uint64_t kvm_hypercall(uint64_t nr, uint64_t a0, uint64_t a1, uint64_t a2,
>  		       uint64_t a3);
>  
> +uint64_t kvm_native_hypercall(uint64_t nr, uint64_t a0, uint64_t a1, uint64_t a2,
> +		       uint64_t a3);
> +
>  void __vm_xsave_require_permission(int bit, const char *name);
>  
>  #define vm_xsave_require_permission(perm)	\
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> index 1e93bb3cb058..429e55f2609f 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> @@ -1202,6 +1202,24 @@ uint64_t kvm_hypercall(uint64_t nr, uint64_t a0, uint64_t a1, uint64_t a2,
>  	return r;
>  }
>  
> +uint64_t kvm_native_hypercall(uint64_t nr, uint64_t a0, uint64_t a1, uint64_t a2,

Just do this in kvm_hypercall().  David didn't say "don't do that", he said "don't
do that in a single patch".  Except for fix_hypercall_test, selftests should always
use the native hypercall instruction and not rely on KVM's patching, e.g. patching
will go sideways if someone gets clever and makes guest code not-writable.

> +		       uint64_t a3)

Align parameters.

> +{
> +	uint64_t r;
> +
> +	if (is_amd_cpu()) {

Curly brace is unnecessary.  Though I think I'd prefer to not duplicate the asm
blob (too many darn inputs).  It's a little ugly, but I prefer it over duplicating
the entire blob.  The approach could also likely be macrofied for other hypercall
usage (future problem).

	asm volatile("test %[use_vmmcall], %[use_vmmcall]\n\t"
		     "jnz 1f\n\t"
		     "vmcall\n\t"
		     "jmp 2f\n\t"
		     "1: vmmcall\n\t"
		     "2:"
		     : "=a"(r)
		     : "a"(nr), "b"(a0), "c"(a1), "d"(a2), "S"(a3),
		       [use_vmmcall] "r" (is_amd_cpu()));
	return r;