Re: [PATCH] perf/x86/intel: KVM: Mask PEBS_ENABLE loaded for guest with vCPU's value.

["Mi, Dapeng" <dapeng1.mi@linux.intel.com>]

On 4/26/2025 8:13 AM, Sean Christopherson wrote:
> When generating the MSR_IA32_PEBS_ENABLE value that will be loaded on
> VM-Entry to a KVM guest, mask the value with the vCPU's desired PEBS_ENABLE
> value.  Consulting only the host kernel's host vs. guest masks results in
> running the guest with PEBS enabled even when the guest doesn't want to use
> PEBS.  Because KVM uses perf events to proxy the guest virtual PMU, simply
> looking at exclude_host can't differentiate between events created by host
> userspace, and events created by KVM on behalf of the guest.
>
> Running the guest with PEBS unexpectedly enabled typically manifests as
> crashes due to a near-infinite stream of #PFs.  E.g. if the guest hasn't
> written MSR_IA32_DS_AREA, the CPU will hit page faults on address '0' when
> trying to record PEBS events.
>
> The issue is most easily reproduced by running `perf kvm top` from before
> commit 7b100989b4f6 ("perf evlist: Remove __evlist__add_default") (after
> which, `perf kvm top` effectively stopped using PEBS).	The userspace side
> of perf creates a guest-only PEBS event, which intel_guest_get_msrs()
> misconstrues a guest-*owned* PEBS event.
>
> Arguably, this is a userspace bug, as enabling PEBS on guest-only events
> simply cannot work, and userspace can kill VMs in many other ways (there
> is no danger to the host).  However, even if this is considered to be bad
> userspace behavior, there's zero downside to perf/KVM restricting PEBS to
> guest-owned events.
>
> Note, commit 854250329c02 ("KVM: x86/pmu: Disable guest PEBS temporarily
> in two rare situations") fixed the case where host userspace is profiling
> KVM *and* userspace, but missed the case where userspace is profiling only
> KVM.
>
> Fixes: c59a1f106f5c ("KVM: x86/pmu: Add IA32_PEBS_ENABLE MSR emulation for extended PEBS")
> Reported-by: Seth Forshee <sforshee@kernel.org>
> Closes: https://lore.kernel.org/all/Z_VUswFkWiTYI0eD@do-x1carbon
> Cc: stable@vger.kernel.org
> Cc: Dapeng Mi <dapeng1.mi@linux.intel.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/events/intel/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index cd6329207311..75a376478b21 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -4625,7 +4625,7 @@ static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr, void *data)
>  	arr[pebs_enable] = (struct perf_guest_switch_msr){
>  		.msr = MSR_IA32_PEBS_ENABLE,
>  		.host = cpuc->pebs_enabled & ~cpuc->intel_ctrl_guest_mask,
> -		.guest = pebs_mask & ~cpuc->intel_ctrl_host_mask,
> +		.guest = pebs_mask & ~cpuc->intel_ctrl_host_mask & kvm_pmu->pebs_enable,

Although I can't reproduce the guest crash issue on local SPR server, the
logic is correct.

Reviewed-by: Dapeng Mi <dapeng1.mi@linux.intel.com>

Furthermore,  the below global_ctrl guest value calculation  seems
incorrect as well.

    u64 pebs_mask = cpuc->pebs_enabled & x86_pmu.pebs_capable;

    arr[global_ctrl] = (struct perf_guest_switch_msr){
        .msr = MSR_CORE_PERF_GLOBAL_CTRL,
        .host = intel_ctrl & ~cpuc->intel_ctrl_guest_mask,
        .guest = intel_ctrl & ~cpuc->intel_ctrl_host_mask & ~pebs_mask,
    };

For guest PEBS events, its HW counter idx would be set into
cpuc->pebs_enabled as well, so the calculated guest value would clear the
corresponding bit of the guest PEBS events. Thus the guest PEBS events
should not be enabled in non-root mode, but coincidentally since the
calculated host and guest value of global_ctrl MSR is same,
atomic_switch_perf_msrs() doesn't really write the guest value into
GLOBAL_CTRL MSR before vm-entry and GLOBAL_CTRL MSR still keeps the default
value (all HW counters are enabled). That's why we see guest PEBS still works.

for (i = 0; i < nr_msrs; i++)
        if (msrs[i].host == msrs[i].guest)
            clear_atomic_switch_msr(vmx, msrs[i].msr);
        else
            add_atomic_switch_msr(vmx, msrs[i].msr, msrs[i].guest,
                    msrs[i].host, false);

Currently we have this Sean's fix, only the guest PEBS event bits of
IA32_PEBS_ENABLE MSR are enabled in non-root mode, suppose we can simply
change global_ctrl guest value calculation to this.

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 09d2d66c9f21..5bc56bb616ec 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -4342,9 +4342,12 @@ static struct perf_guest_switch_msr
*intel_guest_get_msrs(int *nr, void *data)
        arr[global_ctrl] = (struct perf_guest_switch_msr){
                .msr = MSR_CORE_PERF_GLOBAL_CTRL,
                .host = intel_ctrl & ~cpuc->intel_ctrl_guest_mask,
-               .guest = intel_ctrl & ~cpuc->intel_ctrl_host_mask & ~pebs_mask,
+               .guest = intel_ctrl & ~cpuc->intel_ctrl_host_mask,
        };

>  	};
>  
>  	if (arr[pebs_enable].host) {
>
> base-commit: 2492e5aba2be064d0604ae23ae0770ecc0168192