Re: [PATCH] KVM: SVM: drop MSR_IA32_PERF_CAPABILITIES from emulated MSRs

[Paolo Bonzini]

On 17/06/20 13:38, Vitaly Kuznetsov wrote:
> 
> For KVM_GET_MSR_INDEX_LIST, the promise is "guest msrs that are
> supported" and I'm not exactly sure what this means. Personally, I see
> no point in returning MSRs which can't be read with KVM_GET_MSRS (as
> this also means the guest can't read them) and KVM selftests seem to
> rely on that (vcpu_save_state()) but this is not a documented feature.

Yes, this is intended.  KVM_GET_MSR_INDEX_LIST is not the full list of
supported MSRs or KVM_GET_MSRS (especially PMU MSRs are missing) but it
certainly should be a sufficient condition for KVM_GET_MSRS support.

In this case your patch is sort-of correct because AMD machines won't
have X86_FEATURE_PDCM.  However, even in that case there are two things
we can do that are better:

1) force-set X86_FEATURE_PDCM in vmx_set_cpu_caps instead of having it
in kvm_set_cpu_caps.  The latter is incorrect because if AMD for
whatever reason added it we'd lack the support.  This would be basically
a refined version of your patch.

2) emulate the MSR on AMD too (returning zero) if somebody for whatever
reason enables PDCM in there too: this would include returning it in
KVM_GET_FEATURE_MSR_INDEX_LIST, and using kvm_get_msr_feature to set a
default value in kvm_pmu_refresh.  The feature bit then would be
force-set in kvm_set_cpu_caps.  This would be nicer since we have the
value in vcpu->arch already instead of struct vcpu_vmx.

Thanks,

Paolo