Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 21 Sep 2022 21:51:20 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Vitaly Kuznetsov <vkuznets@redhat.com>
Cc:     kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Michael Kelley <mikelley@microsoft.com>,
        Siddharth Chandrasekaran <sidcha@amazon.de>,
        Yuan Yao <yuan.yao@linux.intel.com>,
        Maxim Levitsky <mlevitsk@redhat.com>,
        linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
        Vipin Sharma <vipinsh@google.com>
Subject: Re: [PATCH v10 25/39] KVM: selftests: Move the function doing
 Hyper-V hypercall to a common header
Message-ID: <YyuHWNdZq9/oYTZ+@google.com>
References: <20220921152436.3673454-1-vkuznets@redhat.com>
 <20220921152436.3673454-26-vkuznets@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220921152436.3673454-26-vkuznets@redhat.com>
Precedence: bulk

+Vipin

On Wed, Sep 21, 2022, Vitaly Kuznetsov wrote:
> All Hyper-V specific tests issuing hypercalls need this.
> 
> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  .../selftests/kvm/include/x86_64/hyperv.h      | 16 ++++++++++++++++
>  .../selftests/kvm/x86_64/hyperv_features.c     | 18 +-----------------
>  2 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/x86_64/hyperv.h b/tools/testing/selftests/kvm/include/x86_64/hyperv.h
> index f0a8a93694b2..285e9ff73573 100644
> --- a/tools/testing/selftests/kvm/include/x86_64/hyperv.h
> +++ b/tools/testing/selftests/kvm/include/x86_64/hyperv.h
> @@ -185,6 +185,22 @@
>  /* hypercall options */
>  #define HV_HYPERCALL_FAST_BIT		BIT(16)
>  
> +static inline uint8_t hyperv_hypercall(u64 control, vm_vaddr_t input_address,
> +				       vm_vaddr_t output_address,
> +				       uint64_t *hv_status)
> +{
> +	uint8_t vector;

Newline after the variable declaration.

> +	/* Note both the hypercall and the "asm safe" clobber r9-r11. */
> +	asm volatile("mov %[output_address], %%r8\n\t"
> +		     KVM_ASM_SAFE("vmcall")
> +		     : "=a" (*hv_status),
> +		       "+c" (control), "+d" (input_address),
> +		       KVM_ASM_SAFE_OUTPUTS(vector)
> +		     : [output_address] "r"(output_address)
> +		     : "cc", "memory", "r8", KVM_ASM_SAFE_CLOBBERS);
> +	return vector;
> +}
> +
>  /* Proper HV_X64_MSR_GUEST_OS_ID value */
>  #define HYPERV_LINUX_OS_ID ((u64)0x8100 << 48)
>  
> diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_features.c b/tools/testing/selftests/kvm/x86_64/hyperv_features.c
> index 1144bd1ea626..c464d324cde0 100644
> --- a/tools/testing/selftests/kvm/x86_64/hyperv_features.c
> +++ b/tools/testing/selftests/kvm/x86_64/hyperv_features.c
> @@ -13,22 +13,6 @@
>  #include "processor.h"
>  #include "hyperv.h"
>  
> -static inline uint8_t hypercall(u64 control, vm_vaddr_t input_address,
> -				vm_vaddr_t output_address, uint64_t *hv_status)
> -{
> -	uint8_t vector;
> -
> -	/* Note both the hypercall and the "asm safe" clobber r9-r11. */
> -	asm volatile("mov %[output_address], %%r8\n\t"
> -		     KVM_ASM_SAFE("vmcall")
> -		     : "=a" (*hv_status),
> -		       "+c" (control), "+d" (input_address),
> -		       KVM_ASM_SAFE_OUTPUTS(vector)
> -		     : [output_address] "r"(output_address)
> -		     : "cc", "memory", "r8", KVM_ASM_SAFE_CLOBBERS);
> -	return vector;
> -}
> -
>  struct msr_data {
>  	uint32_t idx;
>  	bool available;
> @@ -78,7 +62,7 @@ static void guest_hcall(vm_vaddr_t pgs_gpa, struct hcall_data *hcall)
>  		input = output = 0;
>  	}
>  
> -	vector = hypercall(hcall->control, input, output, &res);
> +	vector = hyperv_hypercall(hcall->control, input, output, &res);
>  	if (hcall->ud_expected)
>  		GUEST_ASSERT_2(vector == UD_VECTOR, hcall->control, vector);
>  	else

Just out of sight here, but I broke this code in commit cc5851c6be86 ("KVM: selftests:
Use exception fixup for #UD/#GP Hyper-V MSR/hcall tests").  I got too fancy and
inverted the ud_expected logic when checking the result.  The broken code skips the
check when #UD _not_ expected.

I.e. this

	if (hcall->ud_expected)
		GUEST_ASSERT_2(vector == UD_VECTOR, hcall->control, vector);
	else
		GUEST_ASSERT_2(!vector, hcall->control, vector);

	GUEST_ASSERT_2(!hcall->ud_expected || res == hcall->expect,
			hcall->expect, res);

should be

	if (hcall->ud_expected) {
		GUEST_ASSERT_2(vector == UD_VECTOR, hcall->control, vector);
	} else {
		GUEST_ASSERT_2(!vector, hcall->control, vector);
		GUEST_ASSERT_2(res == hcall->expect, hcall->expect, res);
	}

The reason I bring this up here is because of the reason the test passes: gcc
zeros RAX before the hypercall (not entirely sure why), and so res=0 on #UD due
to nothing changing RAX.  But clang doesn't zero RAX and so the test fails due
to RAX holding garbage (probably '1' from the lower 32 bits of HV_X64_MSR_HYPERCALL).

So, what do you think about explicitly setting hv_status, e.g. to -EFAULT, prior
to the hypercall, both to defend against selftest bugs and to verify that _KVM_
actually zeros the result?