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@xxxxxxxxxx> 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.
