On 27/01/21 07:04, Like Xu wrote:
On 2021/1/26 17:42, Paolo Bonzini wrote:
On 08/01/21 02:36, Like Xu wrote:
@@ -401,6 +398,9 @@ static void intel_pmu_init(struct kvm_vcpu *vcpu)
pmu->fixed_counters[i].idx = i + INTEL_PMC_IDX_FIXED;
pmu->fixed_counters[i].current_config = 0;
}
+
+ vcpu->arch.perf_capabilities = guest_cpuid_has(vcpu,
X86_FEATURE_PDCM) ?
+ vmx_get_perf_capabilities() : 0;
There is one thing I don't understand with this patch: intel_pmu_init
is not called when CPUID is changed. So I would have thought that
anything that uses guest_cpuid_has must stay in intel_pmu_refresh. As
I understand it vcpu->arch.perf_capabilities is always set to 0
(vmx_get_perf_capabilities is never called), and kvm_set_msr_common
would fail to set any bit in the MSR. What am I missing?
In addition, the code of patch 4:
+ if (!intel_pmu_lbr_is_enabled(vcpu)) {
+ vcpu->arch.perf_capabilities &= ~PMU_CAP_LBR_FMT;
+ lbr_desc->records.nr = 0;
+ }
is not okay after MSR changes. The value written by the host must be
either rejected (with "return 1") or applied unchanged.
Fortunately I think this code is dead if you move the check in
kvm_set_msr from patch 9 to patch 4. However, in patch 9
vmx_get_perf_capabilities() must only set the LBR format bits if
intel_pmu_lbr_is_compatible(vcpu).
Thanks for the guidance. How about handling it in this way:
In the intel_pmu_init():
vcpu->arch.perf_capabilities = 0;
lbr_desc->records.nr = 0;
In the intel_pmu_refresh():
if (guest_cpuid_has(vcpu, X86_FEATURE_PDCM)) {
vcpu->arch.perf_capabilities = vmx_get_perf_capabilities();
if (!lbr_desc->records.nr)
vcpu->arch.perf_capabilities &= ~PMU_CAP_LBR_FMT;
}
In the vmx_set_msr():
case MSR_IA32_PERF_CAPABILITIES:
// set up lbr_desc->records.nr
if (!intel_pmu_lbr_is_compatible(vcpu))
Maybe pass msr_info.data to intel_pmu_lbr_is_compatible? Otherwise
seems okay, thanks Like.
Paolo
