tools/testing/selftests/kvm/x86_64/vmx_pmu_msrs_test.c | 5 +++++ 1 file changed, 5 insertions(+)
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
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
>
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
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.
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
© 2016 - 2026 Red Hat, Inc.