Messages in this thread | ![/](/images/icornerl.gif) | | Date | Sat, 13 Apr 2024 11:19:11 +0800 | Subject | Re: [RFC PATCH 23/41] KVM: x86/pmu: Implement the save/restore of PMU state for Intel CPU | From | "Mi, Dapeng" <> |
| |
On 4/12/2024 7:31 AM, Sean Christopherson wrote: > On Thu, Apr 11, 2024, Jim Mattson wrote: >> On Thu, Apr 11, 2024 at 2:44 PM Sean Christopherson <seanjc@google.com> wrote: >>>> + /* Clear host global_ctrl and global_status MSR if non-zero. */ >>>> + wrmsrl(MSR_CORE_PERF_GLOBAL_CTRL, 0); >>> Why? PERF_GLOBAL_CTRL will be auto-loaded at VM-Enter, why do it now? >>> >>>> + rdmsrl(MSR_CORE_PERF_GLOBAL_STATUS, global_status); >>>> + if (global_status) >>>> + wrmsrl(MSR_CORE_PERF_GLOBAL_OVF_CTRL, global_status); >>> This seems especially silly, isn't the full MSR being written below? Or am I >>> misunderstanding how these things work? >> LOL! You expect CPU design to follow basic logic?!? >> >> Writing a 1 to a bit in IA32_PERF_GLOBAL_STATUS_SET sets the >> corresponding bit in IA32_PERF_GLOBAL_STATUS to 1. >> >> Writing a 0 to a bit in to IA32_PERF_GLOBAL_STATUS_SET is a nop. >> >> To clear a bit in IA32_PERF_GLOBAL_STATUS, you need to write a 1 to >> the corresponding bit in IA32_PERF_GLOBAL_STATUS_RESET (aka >> IA32_PERF_GLOBAL_OVF_CTRL). > If only C had a way to annotate what the code is doing. :-) > >>>> + wrmsrl(MSR_CORE_PERF_GLOBAL_STATUS_SET, pmu->global_status); > IIUC, that means this should be: > > if (pmu->global_status) > wrmsrl(MSR_CORE_PERF_GLOBAL_STATUS_SET, pmu->global_status); > > or even better: > > toggle = pmu->global_status ^ global_status; > if (global_status & toggle) > wrmsrl(MSR_CORE_PERF_GLOBAL_OVF_CTRL, global_status & toggle); > if (pmu->global_status & toggle) > wrmsrl(MSR_CORE_PERF_GLOBAL_STATUS_SET, pmu->global_status & toggle); Thanks, it looks better. Since PMU v4+, the MSR CORE_PERF_GLOBAL_OVF_CTRL is renamed to CORE_PERF_GLOBAL_STATUS_RESET with supporting more bits. CORE_PERF_GLOBAL_STATUS_RESET looks more easily understand just from name than CORE_PERF_GLOBAL_OVF_CTRL, I would prefer use this name in next version.
| ![\](/images/icornerr.gif) |