On Fri, Jan 15, 2021, Xu, Like wrote:

On 2021/1/15 2:55, Sean Christopherson wrote:

quoted

On Mon, Jan 04, 2021, Like Xu wrote:

quoted

+	 * Note: KVM disables the co-existence of guest PEBS and host PEBS.

By "KVM", do you mean KVM's loading of the MSRs provided by intel_guest_get_msrs()?
Because the PMU should really be the entity that controls guest vs. host.  KVM
should just be a dumb pipe that handles the mechanics of how values are context
switch.

The intel_guest_get_msrs() and atomic_switch_perf_msrs()
will work together to disable the co-existence of guest PEBS and host PEBS:

https://lore.kernel.org/kvm/961e6135-ff6d-86d1-3b7b-a1846ad0e4c4@intel.com/ (local)

+

static void atomic_switch_perf_msrs(struct vcpu_vmx *vmx)
...
    if (nr_msrs > 2 && (msrs[1].guest & msrs[0].guest)) {
        msrs[2].guest = pmu->ds_area;
        if (nr_msrs > 3)
            msrs[3].guest = pmu->pebs_data_cfg;
    }

   for (i = 0; i < nr_msrs; i++)
...

Yeah, that's exactly what I'm complaining about.  Splitting the logic for
determining the guest values is unnecessarily confusing, and as evidenced by the
PEBS_ENABLE bug, potentially fragile.  Perf should have full knowledge and
control of what values are loaded for the guest.  And, the above indexing magic
is nigh impossible to follow and _super_ fragile.

If we change .guest_get_msrs() to take a struct kvm_pmu pointer, then it can
generate the full set of guest values by grabbing ds_area and pebs_data_cfg.
Alternatively, .guest_get_msrs() could take the desired guest MSR values
directly (ds_area and pebs_data_cfg), but kvm_pmu is vendor agnostic, so I don't
see any reason to not just pass the pointer.