linux-kernel - Re: [PATCH v2 3/3] KVM: selftests: Test Hyper-V invariant TSC control

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Yx9CU++TkHZwVfEs@google.com>
Date:   Mon, 12 Sep 2022 14:29:39 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Vitaly Kuznetsov <vkuznets@...hat.com>
Cc:     kvm@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Michael Kelley <mikelley@...rosoft.com>,
        Maxim Levitsky <mlevitsk@...hat.com>,
        linux-hyperv@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/3] KVM: selftests: Test Hyper-V invariant TSC control

On Wed, Aug 31, 2022, Vitaly Kuznetsov wrote:
> Add a test for the newly introduced Hyper-V invariant TSC control feature:
> - HV_X64_MSR_TSC_INVARIANT_CONTROL is not available without
>  HV_ACCESS_TSC_INVARIANT CPUID bit set and available with it.
> - BIT(0) of HV_X64_MSR_TSC_INVARIANT_CONTROL controls the filtering of
> architectural invariant TSC (CPUID.80000007H:EDX[8]) bit.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@...hat.com>
> ---
>  .../selftests/kvm/x86_64/hyperv_features.c    | 67 +++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_features.c b/tools/testing/selftests/kvm/x86_64/hyperv_features.c
> index 4ec4776662a4..26e8c5f7677e 100644
> --- a/tools/testing/selftests/kvm/x86_64/hyperv_features.c
> +++ b/tools/testing/selftests/kvm/x86_64/hyperv_features.c
> @@ -15,6 +15,9 @@
>  
>  #define LINUX_OS_ID ((u64)0x8100 << 48)
>  
> +/* CPUID.80000007H:EDX */
> +#define X86_FEATURE_INVTSC (1 << 8)
> +
>  static inline uint8_t hypercall(u64 control, vm_vaddr_t input_address,
>  				vm_vaddr_t output_address, uint64_t *hv_status)
>  {
> @@ -60,6 +63,24 @@ static void guest_msr(struct msr_data *msr)
>  		GUEST_ASSERT_2(!vector, msr->idx, vector);
>  	else
>  		GUEST_ASSERT_2(vector == GP_VECTOR, msr->idx, vector);
> +
> +	/* Invariant TSC bit appears when TSC invariant control MSR is written to */
> +	if (msr->idx == HV_X64_MSR_TSC_INVARIANT_CONTROL) {
> +		u32 eax, ebx, ecx, edx;
> +
> +		cpuid(0x80000007, &eax, &ebx, &ecx, &edx);

Add a proper kvm_x86_cpu_feature so that this is simply

		this_cpu_has(X86_FEATURE_INVTSC)

> +
> +		/*
> +		 * TSC invariant bit is present without the feature (legacy) or
> +		 * when the feature is present and enabled.
> +		 */
> +		if ((!msr->should_not_gp && !msr->write) || (msr->write && msr->write_val == 1))

Relying purely on the inputs is rather nasty as it creates a subtle dependency
on the "write 1" testcase coming last.  This function already reads the guest
MSR value, just use that to check if INVTSC should be enabled.  And if we want
to verify KVM "wrote" the correct value, then that can be done in the common
path.

And I think that will make this code self-documenting, e.g.

	if (msr->idx == HV_X64_MSR_TSC_INVARIANT_CONTROL)
		GUEST_ASSERT(this_cpu_has(X86_FEATURE_INVTSC) ==
			     !!(msr_val & ...));

> +			GUEST_ASSERT(edx & X86_FEATURE_INVTSC);
> +		else
> +			GUEST_ASSERT(!(edx & X86_FEATURE_INVTSC));
> +	}
> +
> +
>  	GUEST_DONE();
>  }
>  
> @@ -104,6 +125,15 @@ static void vcpu_reset_hv_cpuid(struct kvm_vcpu *vcpu)
>  	vcpu_clear_cpuid_entry(vcpu, HYPERV_CPUID_SYNDBG_PLATFORM_CAPABILITIES);
>  }
>  
> +static bool guest_has_invtsc(void)
> +{
> +	const struct kvm_cpuid_entry2 *cpuid;
> +
> +	cpuid = kvm_get_supported_cpuid_entry(0x80000007);
> +
> +	return cpuid->edx & X86_FEATURE_INVTSC;
> +}
> +
>  static void guest_test_msrs_access(void)
>  {
>  	struct kvm_cpuid2 *prev_cpuid = NULL;
> @@ -115,6 +145,7 @@ static void guest_test_msrs_access(void)
>  	int stage = 0;
>  	vm_vaddr_t msr_gva;
>  	struct msr_data *msr;
> +	bool has_invtsc = guest_has_invtsc();

Huh, I never added vcpu_has_cpuid_feature()?  Can you add that instead of
open-coding the check?