[PATCH] KVM: selftests: Avoid KVM_SET_CPUID2 after KVM_RUN in vmx_pmu_msrs

public inbox for kvm@vger.kernel.org
 help / color / mirror / Atom feed

* [PATCH] KVM: selftests: Avoid KVM_SET_CPUID2 after KVM_RUN in vmx_pmu_msrs_test
@ 2021-12-15 16:16 Vitaly Kuznetsov
  2021-12-15 17:59 ` Sean Christopherson
  0 siblings, 1 reply; 5+ messages in thread
From: Vitaly Kuznetsov @ 2021-12-15 16:16 UTC (permalink / raw)
  To: kvm, Paolo Bonzini
  Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, oliver.sang,
	linux-kernel

Commit feb627e8d6f6 ("KVM: x86: Forbid KVM_SET_CPUID{,2} after KVM_RUN")
forbade chaning vCPU's CPUID data after the first KVM_RUN but
vmx_pmu_msrs_test does exactly that. Test VM needs to be re-created after
vcpu_run().

Reported-by: kernel test robot <oliver.sang@intel.com>
Fixes: feb627e8d6f6 ("KVM: x86: Forbid KVM_SET_CPUID{,2} after KVM_RUN")
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 tools/testing/selftests/kvm/x86_64/vmx_pmu_msrs_test.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/testing/selftests/kvm/x86_64/vmx_pmu_msrs_test.c b/tools/testing/selftests/kvm/x86_64/vmx_pmu_msrs_test.c
index 23051d84b907..17882f79deed 100644
--- a/tools/testing/selftests/kvm/x86_64/vmx_pmu_msrs_test.c
+++ b/tools/testing/selftests/kvm/x86_64/vmx_pmu_msrs_test.c
@@ -99,6 +99,11 @@ int main(int argc, char *argv[])
 	vcpu_run(vm, VCPU_ID);
 	ASSERT_EQ(vcpu_get_msr(vm, VCPU_ID, MSR_IA32_PERF_CAPABILITIES), PMU_CAP_FW_WRITES);
 
+	/* Re-create guest VM after KVM_RUN so CPUID can be changed */
+	kvm_vm_free(vm);
+	vm = vm_create_default(VCPU_ID, 0, guest_code);
+	vcpu_set_cpuid(vm, VCPU_ID, cpuid);
+
 	/* testcase 2, check valid LBR formats are accepted */
 	vcpu_set_msr(vm, 0, MSR_IA32_PERF_CAPABILITIES, 0);
 	ASSERT_EQ(vcpu_get_msr(vm, VCPU_ID, MSR_IA32_PERF_CAPABILITIES), 0);
-- 
2.33.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] KVM: selftests: Avoid KVM_SET_CPUID2 after KVM_RUN in vmx_pmu_msrs_test
  2021-12-15 16:16 [PATCH] KVM: selftests: Avoid KVM_SET_CPUID2 after KVM_RUN in vmx_pmu_msrs_test Vitaly Kuznetsov
@ 2021-12-15 17:59 ` Sean Christopherson
  2021-12-16  9:04   ` Vitaly Kuznetsov
  0 siblings, 1 reply; 5+ messages in thread
From: Sean Christopherson @ 2021-12-15 17:59 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: kvm, Paolo Bonzini, Wanpeng Li, Jim Mattson, oliver.sang,
	linux-kernel

On Wed, Dec 15, 2021, Vitaly Kuznetsov wrote:
> Commit feb627e8d6f6 ("KVM: x86: Forbid KVM_SET_CPUID{,2} after KVM_RUN")
> forbade chaning vCPU's CPUID data after the first KVM_RUN but
> vmx_pmu_msrs_test does exactly that. Test VM needs to be re-created after
> vcpu_run().
> 
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Fixes: feb627e8d6f6 ("KVM: x86: Forbid KVM_SET_CPUID{,2} after KVM_RUN")
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  tools/testing/selftests/kvm/x86_64/vmx_pmu_msrs_test.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/vmx_pmu_msrs_test.c b/tools/testing/selftests/kvm/x86_64/vmx_pmu_msrs_test.c
> index 23051d84b907..17882f79deed 100644
> --- a/tools/testing/selftests/kvm/x86_64/vmx_pmu_msrs_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/vmx_pmu_msrs_test.c
> @@ -99,6 +99,11 @@ int main(int argc, char *argv[])
>  	vcpu_run(vm, VCPU_ID);
>  	ASSERT_EQ(vcpu_get_msr(vm, VCPU_ID, MSR_IA32_PERF_CAPABILITIES), PMU_CAP_FW_WRITES);
>  
> +	/* Re-create guest VM after KVM_RUN so CPUID can be changed */
> +	kvm_vm_free(vm);
> +	vm = vm_create_default(VCPU_ID, 0, guest_code);
> +	vcpu_set_cpuid(vm, VCPU_ID, cpuid);

Why is this test even setting CPUID for the below cases?  Guest CPUID shouldn't
affect host_initiated writes.  This part in particular looks wrong:

	entry_1_0->ecx |= X86_FEATURE_PDCM;
	eax.split.version_id = 0;
	entry_1_0->ecx = eax.full;
	vcpu_set_cpuid(vm, VCPU_ID, cpuid);
	ret = _vcpu_set_msr(vm, 0, MSR_IA32_PERF_CAPABILITIES, PMU_CAP_FW_WRITES);
	TEST_ASSERT(ret == 0, "Bad PERF_CAPABILITIES didn't fail.");

As does the KVM code.  The WRMSR path for MSR_IA32_PERF_CAPABILITIES looks especially
wrong, as rejects a bad write iff userspace set PDCM in guest CPUID.

		struct kvm_msr_entry msr_ent = {.index = msr, .data = 0};

		if (!msr_info->host_initiated)
			return 1;
		if (guest_cpuid_has(vcpu, X86_FEATURE_PDCM) && kvm_get_msr_feature(&msr_ent))  <===== Huh?
			return 1;
		if (data & ~msr_ent.data)
			return 1;

		vcpu->arch.perf_capabilities = data;

		return 0;
		}

So I think we should fix KVM and then clean up the test accordingly.

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 85127b3e3690..65e297875405 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3424,7 +3424,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)

                if (!msr_info->host_initiated)
                        return 1;
-               if (guest_cpuid_has(vcpu, X86_FEATURE_PDCM) && kvm_get_msr_feature(&msr_ent))
+               if (kvm_get_msr_feature(&msr_ent))
                        return 1;
                if (data & ~msr_ent.data)
                        return 1;
@@ -3779,14 +3779,12 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
                msr_info->data = vcpu->arch.microcode_version;
                break;
        case MSR_IA32_ARCH_CAPABILITIES:
-               if (!msr_info->host_initiated &&
-                   !guest_cpuid_has(vcpu, X86_FEATURE_ARCH_CAPABILITIES))
+               if (!msr_info->host_initiated)
                        return 1;
                msr_info->data = vcpu->arch.arch_capabilities;
                break;
        case MSR_IA32_PERF_CAPABILITIES:
-               if (!msr_info->host_initiated &&
-                   !guest_cpuid_has(vcpu, X86_FEATURE_PDCM))
+               if (!msr_info->host_initiated)
                        return 1;
                msr_info->data = vcpu->arch.perf_capabilities;
                break;

> +
>  	/* testcase 2, check valid LBR formats are accepted */
>  	vcpu_set_msr(vm, 0, MSR_IA32_PERF_CAPABILITIES, 0);
>  	ASSERT_EQ(vcpu_get_msr(vm, VCPU_ID, MSR_IA32_PERF_CAPABILITIES), 0);
> -- 
> 2.33.1
> 

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] KVM: selftests: Avoid KVM_SET_CPUID2 after KVM_RUN in vmx_pmu_msrs_test
  2021-12-15 17:59 ` Sean Christopherson
@ 2021-12-16  9:04   ` Vitaly Kuznetsov
  2021-12-16 15:48     ` Sean Christopherson
  0 siblings, 1 reply; 5+ messages in thread
From: Vitaly Kuznetsov @ 2021-12-16  9:04 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: kvm, Paolo Bonzini, Wanpeng Li, Jim Mattson, oliver.sang,
	linux-kernel

Sean Christopherson <seanjc@google.com> writes:

> On Wed, Dec 15, 2021, Vitaly Kuznetsov wrote:
>> Commit feb627e8d6f6 ("KVM: x86: Forbid KVM_SET_CPUID{,2} after KVM_RUN")
>> forbade chaning vCPU's CPUID data after the first KVM_RUN but
>> vmx_pmu_msrs_test does exactly that. Test VM needs to be re-created after
>> vcpu_run().
>> 
>> Reported-by: kernel test robot <oliver.sang@intel.com>
>> Fixes: feb627e8d6f6 ("KVM: x86: Forbid KVM_SET_CPUID{,2} after KVM_RUN")
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  tools/testing/selftests/kvm/x86_64/vmx_pmu_msrs_test.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>> 
>> diff --git a/tools/testing/selftests/kvm/x86_64/vmx_pmu_msrs_test.c b/tools/testing/selftests/kvm/x86_64/vmx_pmu_msrs_test.c
>> index 23051d84b907..17882f79deed 100644
>> --- a/tools/testing/selftests/kvm/x86_64/vmx_pmu_msrs_test.c
>> +++ b/tools/testing/selftests/kvm/x86_64/vmx_pmu_msrs_test.c
>> @@ -99,6 +99,11 @@ int main(int argc, char *argv[])
>>  	vcpu_run(vm, VCPU_ID);
>>  	ASSERT_EQ(vcpu_get_msr(vm, VCPU_ID, MSR_IA32_PERF_CAPABILITIES), PMU_CAP_FW_WRITES);
>>  
>> +	/* Re-create guest VM after KVM_RUN so CPUID can be changed */
>> +	kvm_vm_free(vm);
>> +	vm = vm_create_default(VCPU_ID, 0, guest_code);
>> +	vcpu_set_cpuid(vm, VCPU_ID, cpuid);
>
> Why is this test even setting CPUID for the below cases?  Guest CPUID shouldn't
> affect host_initiated writes.  This part in particular looks wrong:
>
> 	entry_1_0->ecx |= X86_FEATURE_PDCM;
> 	eax.split.version_id = 0;
> 	entry_1_0->ecx = eax.full;
> 	vcpu_set_cpuid(vm, VCPU_ID, cpuid);
> 	ret = _vcpu_set_msr(vm, 0, MSR_IA32_PERF_CAPABILITIES, PMU_CAP_FW_WRITES);
> 	TEST_ASSERT(ret == 0, "Bad PERF_CAPABILITIES didn't fail.");
>
> As does the KVM code.

I admit my natural laziness and thanks for going the extra mile here!

> The WRMSR path for MSR_IA32_PERF_CAPABILITIES looks especially
> wrong, as rejects a bad write iff userspace set PDCM in guest CPUID.
>
> 		struct kvm_msr_entry msr_ent = {.index = msr, .data = 0};
>
> 		if (!msr_info->host_initiated)
> 			return 1;
> 		if (guest_cpuid_has(vcpu, X86_FEATURE_PDCM) && kvm_get_msr_feature(&msr_ent))  <===== Huh?
> 			return 1;
> 		if (data & ~msr_ent.data)
> 			return 1;
>
> 		vcpu->arch.perf_capabilities = data;
>
> 		return 0;
> 		}
>
> So I think we should fix KVM and then clean up the test accordingly.
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 85127b3e3690..65e297875405 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3424,7 +3424,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>
>                 if (!msr_info->host_initiated)
>                         return 1;
> -               if (guest_cpuid_has(vcpu, X86_FEATURE_PDCM) && kvm_get_msr_feature(&msr_ent))
> +               if (kvm_get_msr_feature(&msr_ent))
>                         return 1;
>                 if (data & ~msr_ent.data)
>                         return 1;

This looks OK.

> @@ -3779,14 +3779,12 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>                 msr_info->data = vcpu->arch.microcode_version;
>                 break;
>         case MSR_IA32_ARCH_CAPABILITIES:
> -               if (!msr_info->host_initiated &&
> -                   !guest_cpuid_has(vcpu, X86_FEATURE_ARCH_CAPABILITIES))
> +               if (!msr_info->host_initiated)
>                         return 1;
>                 msr_info->data = vcpu->arch.arch_capabilities;
>                 break;
>         case MSR_IA32_PERF_CAPABILITIES:
> -               if (!msr_info->host_initiated &&
> -                   !guest_cpuid_has(vcpu, X86_FEATURE_PDCM))
> +               if (!msr_info->host_initiated)
>                         return 1;
>                 msr_info->data = vcpu->arch.perf_capabilities;
>                 break;
>

Hm, this change will unconditionally forbid reading
MSR_IA32_ARCH_CAPABILITIES/MSR_IA32_PERF_CAPABILITIES from the guest. Is
this what we want?

>> +
>>  	/* testcase 2, check valid LBR formats are accepted */
>>  	vcpu_set_msr(vm, 0, MSR_IA32_PERF_CAPABILITIES, 0);
>>  	ASSERT_EQ(vcpu_get_msr(vm, VCPU_ID, MSR_IA32_PERF_CAPABILITIES), 0);
>> -- 
>> 2.33.1
>> 
>

-- 
Vitaly


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] KVM: selftests: Avoid KVM_SET_CPUID2 after KVM_RUN in vmx_pmu_msrs_test
  2021-12-16  9:04   ` Vitaly Kuznetsov
@ 2021-12-16 15:48     ` Sean Christopherson
  2021-12-16 16:00       ` Vitaly Kuznetsov
  0 siblings, 1 reply; 5+ messages in thread
From: Sean Christopherson @ 2021-12-16 15:48 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: kvm, Paolo Bonzini, Wanpeng Li, Jim Mattson, oliver.sang,
	linux-kernel

On Thu, Dec 16, 2021, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@google.com> writes:
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 85127b3e3690..65e297875405 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -3424,7 +3424,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> >
> >                 if (!msr_info->host_initiated)
> >                         return 1;
> > -               if (guest_cpuid_has(vcpu, X86_FEATURE_PDCM) && kvm_get_msr_feature(&msr_ent))
> > +               if (kvm_get_msr_feature(&msr_ent))
> >                         return 1;
> >                 if (data & ~msr_ent.data)
> >                         return 1;
> 
> This looks OK.
> 
> > @@ -3779,14 +3779,12 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> >                 msr_info->data = vcpu->arch.microcode_version;
> >                 break;
> >         case MSR_IA32_ARCH_CAPABILITIES:
> > -               if (!msr_info->host_initiated &&
> > -                   !guest_cpuid_has(vcpu, X86_FEATURE_ARCH_CAPABILITIES))
> > +               if (!msr_info->host_initiated)
> >                         return 1;
> >                 msr_info->data = vcpu->arch.arch_capabilities;
> >                 break;
> >         case MSR_IA32_PERF_CAPABILITIES:
> > -               if (!msr_info->host_initiated &&
> > -                   !guest_cpuid_has(vcpu, X86_FEATURE_PDCM))
> > +               if (!msr_info->host_initiated)
> >                         return 1;
> >                 msr_info->data = vcpu->arch.perf_capabilities;
> >                 break;
> >
> 
> Hm, this change will unconditionally forbid reading
> MSR_IA32_ARCH_CAPABILITIES/MSR_IA32_PERF_CAPABILITIES from the guest. Is
> this what we want?

No, I completely misread the code.  The kvm_set_msr_common() goof seems to be
the only bug, and that would also explain the selftest's bad testcase.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] KVM: selftests: Avoid KVM_SET_CPUID2 after KVM_RUN in vmx_pmu_msrs_test
  2021-12-16 15:48     ` Sean Christopherson
@ 2021-12-16 16:00       ` Vitaly Kuznetsov
  0 siblings, 0 replies; 5+ messages in thread
From: Vitaly Kuznetsov @ 2021-12-16 16:00 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: kvm, Paolo Bonzini, Wanpeng Li, Jim Mattson, oliver.sang,
	linux-kernel

Sean Christopherson <seanjc@google.com> writes:

> On Thu, Dec 16, 2021, Vitaly Kuznetsov wrote:
>> Sean Christopherson <seanjc@google.com> writes:
>> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> > index 85127b3e3690..65e297875405 100644
>> > --- a/arch/x86/kvm/x86.c
>> > +++ b/arch/x86/kvm/x86.c
>> > @@ -3424,7 +3424,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>> >
>> >                 if (!msr_info->host_initiated)
>> >                         return 1;
>> > -               if (guest_cpuid_has(vcpu, X86_FEATURE_PDCM) && kvm_get_msr_feature(&msr_ent))
>> > +               if (kvm_get_msr_feature(&msr_ent))
>> >                         return 1;
>> >                 if (data & ~msr_ent.data)
>> >                         return 1;
>> 
>> This looks OK.
>> 
>> > @@ -3779,14 +3779,12 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>> >                 msr_info->data = vcpu->arch.microcode_version;
>> >                 break;
>> >         case MSR_IA32_ARCH_CAPABILITIES:
>> > -               if (!msr_info->host_initiated &&
>> > -                   !guest_cpuid_has(vcpu, X86_FEATURE_ARCH_CAPABILITIES))
>> > +               if (!msr_info->host_initiated)
>> >                         return 1;
>> >                 msr_info->data = vcpu->arch.arch_capabilities;
>> >                 break;
>> >         case MSR_IA32_PERF_CAPABILITIES:
>> > -               if (!msr_info->host_initiated &&
>> > -                   !guest_cpuid_has(vcpu, X86_FEATURE_PDCM))
>> > +               if (!msr_info->host_initiated)
>> >                         return 1;
>> >                 msr_info->data = vcpu->arch.perf_capabilities;
>> >                 break;
>> >
>> 
>> Hm, this change will unconditionally forbid reading
>> MSR_IA32_ARCH_CAPABILITIES/MSR_IA32_PERF_CAPABILITIES from the guest. Is
>> this what we want?
>
> No, I completely misread the code.  The kvm_set_msr_common() goof seems to be
> the only bug, and that would also explain the selftest's bad testcase.
>

Ok then, v2 is coming.

-- 
Vitaly


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-12-16 16:00 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-12-15 16:16 [PATCH] KVM: selftests: Avoid KVM_SET_CPUID2 after KVM_RUN in vmx_pmu_msrs_test Vitaly Kuznetsov
2021-12-15 17:59 ` Sean Christopherson
2021-12-16  9:04   ` Vitaly Kuznetsov
2021-12-16 15:48     ` Sean Christopherson
2021-12-16 16:00       ` Vitaly Kuznetsov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox