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

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

On 5/2/2025 11:04 PM, Sean Christopherson wrote:
> On Sun, Apr 27, 2025, Dapeng Mi wrote:
>> On 4/26/2025 8:13 AM, Sean Christopherson wrote:
>> 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,
>>         };
> Hmm, that's not as clear cut.  PEBS needs to be disabled because leaving it enabled
> will crash the guest.  For the counter itself, unless leaving it enabled breaks
> perf and/or degrades the sampling, I don't think there's an obvious right/wrong
> approach.
>
> E.g. if the host wants to profile host and guest, then keeping the count running
> while the guest is active might be a good thing.  It's still far, far from
> perfect, as a counter that overflows when the guest is active won't generate a
> PEBS record, but without digging further, it's not obvious that even that flaw
> is overall worse than always disabling the counter.

Hmm, if the host PEBS event only samples host side, whether the HW counter
or the PEBS engine would be disabled once VM enters non-root mode, the KVM
PEBS implementation is correct. But for the host PEBS events which sampling
both host and guest, the implementation seems incorrect.

As the below code shows, as long as there are host PEBS events regardless
of the host PEBS events only sample guest or both host and guest, the host
PEBS events would be disabled on both HW counters and PEBS engine once VM
enters non-root mode.

    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,
    };

    if (arr[pebs_enable].host) {
        /* Disable guest PEBS if host PEBS is enabled. */
        arr[pebs_enable].guest = 0;

    }

So the host PEBS events which hopes to sample both host and guest only
samples host side in fact. This is unexpected.

I agree it's not enough to just remove the "~pebs_mask" for guest
global_ctrl, a simple way to fix this issue could be to skip all PEBS
related MSR switch at VM-exit/VM-entry and always keep the host vlaue in
these PEBS MSRS regardless of in root or non-root mode.