* [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;
as well as URLs for NNTP newsgroup(s).