Re: [PATCH] KVM: x86/pmu: Prevent any host user from enabling PEBS for profiling guest

Like Xu
2023-11-30T07:30:07Z

On 29/11/2023 10:38 pm, Liang, Kan wrote:
>
>
> On 2023-11-29 4:50 a.m., Like Xu wrote:
>> From: Like Xu <likexu@tencent.com>
>>
>> Stop using PEBS counters on host to profiling guest. Limit the range of
>> enabled PEBS counters to only those counters enabled from the guest PEBS
>> emulation perspective.
>>
>> If there is a perf-record agent on host that uses perf-tools events like
>> "cpu-cycles:GP" (G for attr.exclude_host, P for max precise event counter)
>> to capture guest performance events, then the guest will be hanged. This is
>> because Intel DS-based PEBS buffer is addressed using the 64-bit linear
>> address of the current {p/v}CPU context based on MSR_IA32_DS_AREA.
>>
>> Any perf user using PEBS counters to profile guest on host is, in perf/core
>> implementation details, trying to set bits on cpuc->intel_ctrl_guest_mask
>> and arr[pebs_enable].guest, much like the guest PEBS emulation behaviour.
>> But the subsequent PEBS memory write, regardless of whether guest PEBS is
>> enabled, can overshoot guest entry and corrupt guest memory.
>>
>> Profiling guest via PEBS-DS buffer on host is not supported at this time.
>> Fix this by filtering the real configured value of arr[pebs_enable].guest
>> with the emulated state of guest enabled PEBS counters, under the condition
>> of none cross-mapped PEBS counters.
>
> So the counter will be silently disabled. The user never knows why
> nothing is sampled.
> Since we don't support the case, profiling guest via PEBS-DS buffer on
> host. Maybe we should error out when creating the event. For example
> (not tested),

Test failed.

>
> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index 3871267d3237..24b90c70737f 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -3958,6 +3958,10 @@ static int intel_pmu_hw_config(struct perf_event
> *event)
> if ((event->attr.config & INTEL_ARCH_EVENT_MASK) ==
> INTEL_FIXED_VLBR_EVENT)
> return -EINVAL;
>
> + /* Profiling guest via PEBS-DS buffer on host is not supported. */
> + if (event->attr.exclude_host)
> + return -EINVAL;
> +

Guest PEBS emulation also sets this bit, a typical call stack looks like:

intel_pmu_hw_config+0x441/0x4d0
hsw_hw_config+0x12/0xa0
x86_pmu_event_init+0x98/0x370
perf_try_init_event+0x47/0x130
perf_event_alloc+0x446/0xeb0
perf_event_create_kernel_counter+0x38/0x190
pmc_reprogram_counter.constprop.17+0xd9/0x230 [kvm]
kvm_pmu_handle_event+0x1a6/0x310 [kvm]
vcpu_enter_guest+0x1388/0x19b0 [kvm]
vcpu_run+0x117/0x6c0 [kvm]
kvm_arch_vcpu_ioctl_run+0x13d/0x4d0 [kvm]
kvm_vcpu_ioctl+0x301/0x6e0 [kvm]

Alternatively, this path is taken when using PEBS-via-PT to profile guests on host.

The status of the guest can only be queried in the NMI handler and the func
intel_guest_get_msrs() in the perf/core context, where it's easier and more
centrally to review this part of changes that affects vPMU for corner cases.

Maybe adding print info on the perf-tool side would help.

For perf-tool users, it will get 0 number of sample for "cpu-cycles:GP" events,
just like other uncounted perf-tool events.

> if (!(event->attr.freq || (event->attr.wakeup_events &&
> !event->attr.watermark))) {
> event->hw.flags |= PERF_X86_EVENT_AUTO_RELOAD;
> if (!(event->attr.sample_type &
>
>
> Thanks,
> Kan
>
>>
>> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
>> Fixes: c59a1f106f5c ("KVM: x86/pmu: Add IA32_PEBS_ENABLE MSR emulation for extended PEBS")
>> Signed-off-by: Like Xu <likexu@tencent.com>
>> ---
>> arch/x86/events/intel/core.c | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
>> index a08f794a0e79..17afd504c35b 100644
>> --- a/arch/x86/events/intel/core.c
>> +++ b/arch/x86/events/intel/core.c
>> @@ -4103,13 +4103,19 @@ static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr, void *data)
>> .guest = pebs_mask & ~cpuc->intel_ctrl_host_mask,
>> };
>>
>> + /* In any case, clear guest PEBS bits first. */
>> + arr[global_ctrl].guest &= ~arr[pebs_enable].guest;
>> +
>> if (arr[pebs_enable].host) {
>> /* Disable guest PEBS if host PEBS is enabled. */
>> arr[pebs_enable].guest = 0;
>> } else {
>> /* Disable guest PEBS thoroughly for cross-mapped PEBS counters. */
>> arr[pebs_enable].guest &= ~kvm_pmu->host_cross_mapped_mask;
>> - arr[global_ctrl].guest &= ~kvm_pmu->host_cross_mapped_mask;
>> +
>> + /* Prevent any host user from enabling PEBS for profiling guest. */
>> + arr[pebs_enable].guest &= (kvm_pmu->pebs_enable & kvm_pmu->global_ctrl);
>> +
>> /* Set hw GLOBAL_CTRL bits for PEBS counter when it runs for guest */
>> arr[global_ctrl].guest |= arr[pebs_enable].guest;
>> }
>>
>> base-commit: 6803fb00772cc50cd59a66bd8caaee5c84b13fcf