Sean Christopherson <seanjc@xxxxxxxxxx> 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@xxxxxxxxx>
>> Fixes: feb627e8d6f6 ("KVM: x86: Forbid KVM_SET_CPUID{,2} after KVM_RUN")
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
>> ---
>> 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
