On Thu, Jan 04, 2024, Liang, Kan wrote:
On 2024-01-04 10:39 a.m., Paolo Bonzini wrote:
When commit c59a1f106f5c ("KVM: x86/pmu: Add IA32_PEBS_ENABLE MSR emulation for extended PEBS") switched the initialization of cpuc->guest_switch_msrs to use compound literals, it screwed up the boolean logic:
- u64 pebs_mask = cpuc->pebs_enabled & x86_pmu.pebs_capable;
...
- arr[0].guest = intel_ctrl & ~cpuc->intel_ctrl_host_mask;
- arr[0].guest &= ~(cpuc->pebs_enabled & x86_pmu.pebs_capable);
.guest = intel_ctrl & (~cpuc->intel_ctrl_host_mask | ~pebs_mask),Before the patch, the value of arr[0].guest would have been intel_ctrl & ~cpuc->intel_ctrl_host_mask & ~pebs_mask. The intent is to always treat PEBS events as host-only because, while the guest runs, there is no way to tell the processor about the virtual address where to put PEBS records intended for the host.
Unfortunately, the new expression can be expanded to
(intel_ctrl & ~cpuc->intel_ctrl_host_mask) | (intel_ctrl & ~pebs_mask)
which makes no sense; it includes any bit that isn't *both* marked as exclude_guest and using PEBS. So, reinstate the old logic.
I think the old logic will completely disable the PEBS in guest capability. Because the counter which is assigned to a guest PEBS event will also be set in the pebs_mask. The old logic disable the counter in GLOBAL_CTRL in guest. Nothing will be counted.
Like once proposed a fix in the intel_guest_get_msrs(). https://lore.kernel.org/lkml/20231129095055.88060-1-likexu@tencent.com/ It should work for the issue.
No, that patch only affects the path where hardware supports enabling PEBS in the the guest, i.e. intel_guest_get_msrs() will bail before getting to that code due to the lack of x86_pmu.pebs_ept support, which IIUC is all pre-Icelake Intel CPUs.
if (!kvm_pmu || !x86_pmu.pebs_ept) return arr;