linux-hyperv.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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>,
	Maxim Levitsky <mlevitsk@redhat.com>,
	Michael Kelley <mikelley@microsoft.com>,
	linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 6/6] KVM: selftests: Test Hyper-V invariant TSC control
Date: Tue, 11 Oct 2022 19:40:42 +0000	[thread overview]
Message-ID: <Y0XGuk4vwJBTU9oN@google.com> (raw)
In-Reply-To: <20220922143655.3721218-7-vkuznets@redhat.com>

On Thu, Sep 22, 2022, Vitaly Kuznetsov wrote:
> diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_features.c b/tools/testing/selftests/kvm/x86_64/hyperv_features.c
> index d4bd18bc580d..18b44450dfb8 100644
> --- a/tools/testing/selftests/kvm/x86_64/hyperv_features.c
> +++ b/tools/testing/selftests/kvm/x86_64/hyperv_features.c
> @@ -46,20 +46,33 @@ struct hcall_data {
>  
>  static void guest_msr(struct msr_data *msr)
>  {
> -	uint64_t ignored;
> +	uint64_t msr_val = 0;
>  	uint8_t vector;
>  
>  	GUEST_ASSERT(msr->idx);
>  
> -	if (!msr->write)
> -		vector = rdmsr_safe(msr->idx, &ignored);
> -	else
> +	if (!msr->write) {
> +		vector = rdmsr_safe(msr->idx, &msr_val);

This is subtly going to do weird things if the RDMSR faults.  rdmsr_safe()
overwrites @val with whatever happens to be in EDX:EAX if the RDMSR faults, i.e.
this may yield garbage instead of '0'.  Arguably rdmsr_safe() is a bad API, but
at the same time the caller really shouldn't consume the result if RDMSR faults
(though aligning with the kernel is also valuable).

Aha!  Idea.  Assuming none of the MSRs are write-only, what about adding a prep
patch to rework this code so that it verifies RDMSR returns what was written when
a fault didn't occur.

	uint8_t vector = 0;
	uint64_t msr_val;

	GUEST_ASSERT(msr->idx);

	if (msr->write)
		vector = wrmsr_safe(msr->idx, msr->write_val);

	if (!vector)
		vector = rdmsr_safe(msr->idx, &msr_val);

	if (msr->fault_expected)
		GUEST_ASSERT_2(vector == GP_VECTOR, msr->idx, vector);
	else
		GUEST_ASSERT_2(!vector, msr->idx, vector);

	if (vector)
		goto done;

	GUEST_ASSERT_2(msr_val == msr->write_val, msr_val, msr->write_val);

done:
	GUEST_DONE();


and then this patch can just slot in the extra check:

	uint8_t vector = 0;
	uint64_t msr_val;

	GUEST_ASSERT(msr->idx);

	if (msr->write)
		vector = wrmsr_safe(msr->idx, msr->write_val);

	if (!vector)
		vector = rdmsr_safe(msr->idx, &msr_val);

	if (msr->fault_expected)
		GUEST_ASSERT_2(vector == GP_VECTOR, msr->idx, vector);
	else
		GUEST_ASSERT_2(!vector, msr->idx, vector);

	if (vector)
		goto done;

	GUEST_ASSERT_2(msr_val == msr->write_val, msr_val, msr->write_val);

	/* Invariant TSC bit appears when TSC invariant control MSR is written to */
	if (msr->idx == HV_X64_MSR_TSC_INVARIANT_CONTROL) {
		if (!this_cpu_has(HV_ACCESS_TSC_INVARIANT))
			GUEST_ASSERT(this_cpu_has(X86_FEATURE_INVTSC));
		else
			GUEST_ASSERT(this_cpu_has(X86_FEATURE_INVTSC) ==
				     !!(msr_val & HV_INVARIANT_TSC_EXPOSED));
	}

done:
	GUEST_DONE();

  reply	other threads:[~2022-10-11 19:40 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-22 14:36 [PATCH v4 0/6] KVM: x86: Hyper-V invariant TSC control feature Vitaly Kuznetsov
2022-09-22 14:36 ` [PATCH v4 1/6] x86/hyperv: Add HV_INVARIANT_TSC_EXPOSED define Vitaly Kuznetsov
2022-09-22 22:07   ` Michael Kelley (LINUX)
2022-09-22 14:36 ` [PATCH v4 2/6] KVM: x86: Introduce CPUID_8000_0007_EDX 'scattered' leaf Vitaly Kuznetsov
2022-09-22 17:09   ` Jim Mattson
2022-09-22 17:53     ` Sean Christopherson
2022-09-22 17:55       ` Jim Mattson
2022-10-11 19:15   ` Sean Christopherson
2022-09-22 14:36 ` [PATCH v4 3/6] KVM: x86: Hyper-V invariant TSC control Vitaly Kuznetsov
2022-09-22 14:36 ` [PATCH v4 4/6] KVM: selftests: Rename 'msr->availble' to 'msr->fault_exepected' in hyperv_features test Vitaly Kuznetsov
2022-10-11 19:18   ` Sean Christopherson
2022-09-22 14:36 ` [PATCH v4 5/6] KVM: selftests: Convert hyperv_features test to using KVM_X86_CPU_FEATURE() Vitaly Kuznetsov
2022-09-22 14:36 ` [PATCH v4 6/6] KVM: selftests: Test Hyper-V invariant TSC control Vitaly Kuznetsov
2022-10-11 19:40   ` Sean Christopherson [this message]
2022-10-12 12:40     ` Vitaly Kuznetsov
2022-10-12 16:52       ` Sean Christopherson
2022-10-13  9:16         ` Vitaly Kuznetsov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Y0XGuk4vwJBTU9oN@google.com \
    --to=seanjc@google.com \
    --cc=jmattson@google.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-hyperv@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mikelley@microsoft.com \
    --cc=mlevitsk@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=vkuznets@redhat.com \
    --cc=wanpengli@tencent.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).