Re: [PATCH] KVM: selftests: Avoid KVM_SET_CPUID2 after KVM_RUN in vmx_pmu_msrs_test

[Vitaly Kuznetsov]

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