* [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