diff mbox series

[v4,6/6] KVM: selftests: Test Hyper-V invariant TSC control

Message ID 20220922143655.3721218-7-vkuznets@redhat.com (mailing list archive)
State New, archived
Headers show
Series KVM: x86: Hyper-V invariant TSC control feature | expand

Commit Message

Vitaly Kuznetsov Sept. 22, 2022, 2:36 p.m. UTC 
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@redhat.com>
---
 .../selftests/kvm/include/x86_64/hyperv.h     |  3 +
 .../selftests/kvm/include/x86_64/processor.h  |  1 +
 .../selftests/kvm/x86_64/hyperv_features.c    | 58 +++++++++++++++++--
 3 files changed, 58 insertions(+), 4 deletions(-)

Comments

Sean Christopherson Oct. 11, 2022, 7:40 p.m. UTC | #1 
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();
Vitaly Kuznetsov Oct. 12, 2022, 12:40 p.m. UTC | #2 
Sean Christopherson <seanjc@google.com> writes:

> 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.
>

There is at least one read-only MSR which comes to mind:
HV_X64_MSR_EOI. Also, some of the MSRs don't preserve the written value,
e.g. HV_X64_MSR_RESET which always reads as '0'.

I do, however, like the additional check that RDMSR returns what was
written to the MSR, we will just need an additional flag in 'struct
msr_data' ('check_written_value' maybe?).
Sean Christopherson Oct. 12, 2022, 4:52 p.m. UTC | #3 
On Wed, Oct 12, 2022, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@google.com> writes:
> 
> > 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.
> >
> 
> There is at least one read-only MSR which comes to mind:
> HV_X64_MSR_EOI.

I assume s/read-only/write-only since it's EOI?

> Also, some of the MSRs don't preserve the written value,
> e.g. HV_X64_MSR_RESET which always reads as '0'.

Hrm, that's annoying.

> I do, however, like the additional check that RDMSR returns what was
> written to the MSR, we will just need an additional flag in 'struct
> msr_data' ('check_written_value' maybe?).

Rather than force the testcase to specify information that's intrinsic to the MSR,
what about adding helpers to communicate the types?  E.g.

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

        if (!vector && !is_write_only_msr(msr->idx))
                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 (is_read_zero_msr(msr->idx))
		GUEST_ASSERT_2(msr_val == 0, msr_val, 0);
	else
		GUEST_ASSERT_2(msr_val == msr->write_val, msr_val, msr->write_val);

I think that'd make the code a bit less magical and easier to understand for folks
that aren't familiar with Hyper-V.  The number of special MSRs is likely very small,
so the helpers should be trivial, e.g.

static bool is_write_only_msr(uint32_t msr)
{
	return msr == HV_X64_MSR_EOI;
}

static bool is_read_zero_msr(uint32_t msr)
{
	return msr == HV_X64_MSR_RESET || msr == ???;
}
Vitaly Kuznetsov Oct. 13, 2022, 9:16 a.m. UTC | #4 
Sean Christopherson <seanjc@google.com> writes:

> On Wed, Oct 12, 2022, Vitaly Kuznetsov wrote:
>> Sean Christopherson <seanjc@google.com> writes:
>> 

...

>> > 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.
>> >
>> 
>> There is at least one read-only MSR which comes to mind:
>> HV_X64_MSR_EOI.
>
> I assume s/read-only/write-only since it's EOI?
>

Yes, of course)

>> Also, some of the MSRs don't preserve the written value,
>> e.g. HV_X64_MSR_RESET which always reads as '0'.
>
> Hrm, that's annoying.

'Slightly annoying'. In fact, the test never writes anything besides '0'
to the MSR as the code is not ready to handle real vCPU reset. I'll
leave a TODO note about that.

...

> static bool is_write_only_msr(uint32_t msr)
> {
> 	return msr == HV_X64_MSR_EOI;
> }

This is all we need, basically. I'll go with that.
diff mbox series

Patch

diff --git a/tools/testing/selftests/kvm/include/x86_64/hyperv.h b/tools/testing/selftests/kvm/include/x86_64/hyperv.h
index 843748dde1ff..8368d65afbe4 100644
--- a/tools/testing/selftests/kvm/include/x86_64/hyperv.h
+++ b/tools/testing/selftests/kvm/include/x86_64/hyperv.h
@@ -232,4 +232,7 @@ 
 /* hypercall options */
 #define HV_HYPERCALL_FAST_BIT		BIT(16)
 
+/* HV_X64_MSR_TSC_INVARIANT_CONTROL bits */
+#define HV_INVARIANT_TSC_EXPOSED               BIT_ULL(0)
+
 #endif /* !SELFTEST_KVM_HYPERV_H */
diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index 0cbc71b7af50..8d106380b0af 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -128,6 +128,7 @@  struct kvm_x86_cpu_feature {
 #define	X86_FEATURE_GBPAGES		KVM_X86_CPU_FEATURE(0x80000001, 0, EDX, 26)
 #define	X86_FEATURE_RDTSCP		KVM_X86_CPU_FEATURE(0x80000001, 0, EDX, 27)
 #define	X86_FEATURE_LM			KVM_X86_CPU_FEATURE(0x80000001, 0, EDX, 29)
+#define	X86_FEATURE_INVTSC		KVM_X86_CPU_FEATURE(0x80000007, 0, EDX, 8)
 #define	X86_FEATURE_RDPRU		KVM_X86_CPU_FEATURE(0x80000008, 0, EBX, 4)
 #define	X86_FEATURE_AMD_IBPB		KVM_X86_CPU_FEATURE(0x80000008, 0, EBX, 12)
 #define	X86_FEATURE_NPT			KVM_X86_CPU_FEATURE(0x8000000A, 0, EDX, 0)
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);
+	} else {
 		vector = wrmsr_safe(msr->idx, msr->write_val);
+		if (!vector)
+			msr_val = msr->write_val;
+	}
 
 	if (msr->fault_expected)
 		GUEST_ASSERT_2(vector == GP_VECTOR, msr->idx, vector);
 	else
 		GUEST_ASSERT_2(!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) {
+		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));
+	}
+
 	GUEST_DONE();
 }
 
@@ -114,6 +127,7 @@  static void guest_test_msrs_access(void)
 	int stage = 0;
 	vm_vaddr_t msr_gva;
 	struct msr_data *msr;
+	bool has_invtsc = kvm_cpu_has(X86_FEATURE_INVTSC);
 
 	while (true) {
 		vm = vm_create_with_one_vcpu(&vcpu, guest_msr);
@@ -425,6 +439,42 @@  static void guest_test_msrs_access(void)
 			break;
 
 		case 44:
+			/* MSR is not available when CPUID feature bit is unset */
+			if (!has_invtsc)
+				continue;
+			msr->idx = HV_X64_MSR_TSC_INVARIANT_CONTROL;
+			msr->write = 0;
+			msr->fault_expected = 1;
+			break;
+		case 45:
+			/* MSR is vailable when CPUID feature bit is set */
+			if (!has_invtsc)
+				continue;
+			vcpu_set_cpuid_feature(vcpu, HV_ACCESS_TSC_INVARIANT);
+			msr->idx = HV_X64_MSR_TSC_INVARIANT_CONTROL;
+			msr->write = 0;
+			msr->fault_expected = 0;
+			break;
+		case 46:
+			/* Writing bits other than 0 is forbidden */
+			if (!has_invtsc)
+				continue;
+			msr->idx = HV_X64_MSR_TSC_INVARIANT_CONTROL;
+			msr->write = 1;
+			msr->write_val = 0xdeadbeef;
+			msr->fault_expected = 1;
+			break;
+		case 47:
+			/* Setting bit 0 enables the feature */
+			if (!has_invtsc)
+				continue;
+			msr->idx = HV_X64_MSR_TSC_INVARIANT_CONTROL;
+			msr->write = 1;
+			msr->write_val = 1;
+			msr->fault_expected = 0;
+			break;
+
+		default:
 			kvm_vm_free(vm);
 			return;
 		}