+Vipin
On Wed, Sep 21, 2022, Vitaly Kuznetsov wrote:
> All Hyper-V specific tests issuing hypercalls need this.
>
> Reviewed-by: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> ---
> .../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?
