Message ID | 20210511024214.280733-3-like.xu@linux.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | KVM: x86/pmu: Add *basic* support to enable guest PEBS via DS | expand |
On Tue, May 11, 2021 at 10:42:00AM +0800, Like Xu wrote: > With PEBS virtualization, the guest PEBS records get delivered to the > guest DS, and the host pmi handler uses perf_guest_cbs->is_in_guest() > to distinguish whether the PMI comes from the guest code like Intel PT. > > No matter how many guest PEBS counters are overflowed, only triggering > one fake event is enough. The fake event causes the KVM PMI callback to > be called, thereby injecting the PEBS overflow PMI into the guest. > > KVM may inject the PMI with BUFFER_OVF set, even if the guest DS is > empty. That should really be harmless. Thus guest PEBS handler would > retrieve the correct information from its own PEBS records buffer. > > Originally-by: Andi Kleen <ak@linux.intel.com> > Co-developed-by: Kan Liang <kan.liang@linux.intel.com> > Signed-off-by: Kan Liang <kan.liang@linux.intel.com> > Signed-off-by: Like Xu <like.xu@linux.intel.com> > --- > arch/x86/events/intel/core.c | 40 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c > index b6e45ee10e16..092ecacf8345 100644 > --- a/arch/x86/events/intel/core.c > +++ b/arch/x86/events/intel/core.c > @@ -2780,6 +2780,43 @@ static void intel_pmu_reset(void) > local_irq_restore(flags); > } > > +/* > + * We may be running with guest PEBS events created by KVM, and the > + * PEBS records are logged into the guest's DS and invisible to host. > + * > + * In the case of guest PEBS overflow, we only trigger a fake event > + * to emulate the PEBS overflow PMI for guest PBES counters in KVM. > + * The guest will then vm-entry and check the guest DS area to read > + * the guest PEBS records. > + * > + * The contents and other behavior of the guest event do not matter. > + */ > +static void x86_pmu_handle_guest_pebs(struct pt_regs *regs, > + struct perf_sample_data *data) > +{ > + struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events); > + u64 guest_pebs_idxs = cpuc->pebs_enabled & ~cpuc->intel_ctrl_host_mask; > + struct perf_event *event = NULL; > + int bit; > + > + if (!x86_pmu.pebs_active || !guest_pebs_idxs) > + return; > + > + for_each_set_bit(bit, (unsigned long *)&guest_pebs_idxs, > + INTEL_PMC_IDX_FIXED + x86_pmu.num_counters_fixed) { > + event = cpuc->events[bit]; > + if (!event->attr.precise_ip) > + continue; > + > + perf_sample_data_init(data, 0, event->hw.last_period); > + if (perf_event_overflow(event, data, regs)) > + x86_pmu_stop(event, 0); > + > + /* Inject one fake event is enough. */ > + break; > + } > +} > + > static int handle_pmi_common(struct pt_regs *regs, u64 status) > { > struct perf_sample_data data; > @@ -2831,6 +2868,9 @@ static int handle_pmi_common(struct pt_regs *regs, u64 status) > u64 pebs_enabled = cpuc->pebs_enabled; > > handled++; > + if (x86_pmu.pebs_vmx && perf_guest_cbs && > + perf_guest_cbs->is_in_guest()) > + x86_pmu_handle_guest_pebs(regs, &data); > x86_pmu.drain_pebs(regs, &data); > status &= intel_ctrl | GLOBAL_STATUS_TRACE_TOPAPMI; > I'm thinking you have your conditions in the wrong order; would it not be much cheaper to first check: '!x86_pmu.pebs_active || !guest_pebs_idx' than to do that horrible indirect ->is_in_guest() call? After all, if the guest doesn't have PEBS enabled, who cares if we're currently in a guest or not. Also, something like the below perhaps (arm64 and xen need fixing up at the very least) could make all that perf_guest_cbs stuff suck less. --- diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c index 8e509325c2c3..c8f8fb7c0536 100644 --- a/arch/x86/events/core.c +++ b/arch/x86/events/core.c @@ -90,6 +90,26 @@ DEFINE_STATIC_CALL_NULL(x86_pmu_pebs_aliases, *x86_pmu.pebs_aliases); */ DEFINE_STATIC_CALL_RET0(x86_pmu_guest_get_msrs, *x86_pmu.guest_get_msrs); +DEFINE_STATIC_CALL_RET0(x86_guest_state, *(perf_guest_cbs->state)); +DEFINE_STATIC_CALL_RET0(x86_guest_get_ip, *(perf_guest_cbs->get_ip)); +DEFINE_STATIC_CALL_RET0(x86_guest_handle_intel_pt_intr, *(perf_guest_cbs->handle_intel_pt_intr)); + +void arch_perf_update_guest_cbs(void) +{ + static_call_update(x86_guest_state, (void *)&__static_call_return0); + static_call_update(x86_guest_get_ip, (void *)&__static_call_return0); + static_call_update(x86_guest_handle_intel_pt_intr, (void *)&__static_call_return0); + + if (perf_guest_cbs && perf_guest_cbs->state) + static_call_update(x86_guest_state, perf_guest_cbs->state); + + if (perf_guest_cbs && perf_guest_cbs->get_ip) + static_call_update(x86_guest_get_ip, perf_guest_cbs->get_ip); + + if (perf_guest_cbs && perf_guest_cbs->handle_intel_pt_intr) + static_call_update(x86_guest_handle_intel_pt_intr, perf_guest_cbs->handle_intel_pt_intr); +} + u64 __read_mostly hw_cache_event_ids [PERF_COUNT_HW_CACHE_MAX] [PERF_COUNT_HW_CACHE_OP_MAX] @@ -2736,7 +2756,7 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx *entry, struct pt_regs *re struct unwind_state state; unsigned long addr; - if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) { + if (static_call(x86_guest_state)()) { /* TODO: We don't support guest os callchain now */ return; } @@ -2839,7 +2859,7 @@ perf_callchain_user(struct perf_callchain_entry_ctx *entry, struct pt_regs *regs struct stack_frame frame; const struct stack_frame __user *fp; - if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) { + if (static_call(x86_guest_state)()) { /* TODO: We don't support guest os callchain now */ return; } @@ -2916,18 +2936,21 @@ static unsigned long code_segment_base(struct pt_regs *regs) unsigned long perf_instruction_pointer(struct pt_regs *regs) { - if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) - return perf_guest_cbs->get_guest_ip(); + unsigned long ip = static_call(x86_guest_get_ip)(); + + if (likely(!ip)) + ip = regs->ip + code_segment_base(regs); - return regs->ip + code_segment_base(regs); + return ip; } unsigned long perf_misc_flags(struct pt_regs *regs) { + unsigned int guest = static_call(x86_guest_state)(); int misc = 0; - if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) { - if (perf_guest_cbs->is_user_mode()) + if (guest) { + if (guest & PERF_GUEST_USER) misc |= PERF_RECORD_MISC_GUEST_USER; else misc |= PERF_RECORD_MISC_GUEST_KERNEL; diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c index 2521d03de5e0..ac422c45f940 100644 --- a/arch/x86/events/intel/core.c +++ b/arch/x86/events/intel/core.c @@ -2780,6 +2780,8 @@ static void intel_pmu_reset(void) local_irq_restore(flags); } +DECLARE_STATIC_CALL(x86_guest_handle_intel_pt_intr, *(perf_guest_cbs->handle_intel_pt_intr)); + static int handle_pmi_common(struct pt_regs *regs, u64 status) { struct perf_sample_data data; @@ -2850,10 +2852,7 @@ static int handle_pmi_common(struct pt_regs *regs, u64 status) */ if (__test_and_clear_bit(GLOBAL_STATUS_TRACE_TOPAPMI_BIT, (unsigned long *)&status)) { handled++; - if (unlikely(perf_guest_cbs && perf_guest_cbs->is_in_guest() && - perf_guest_cbs->handle_intel_pt_intr)) - perf_guest_cbs->handle_intel_pt_intr(); - else + if (!static_call(x86_guest_handle_intel_pt_intr)()) intel_pt_interrupt(); } diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 55efbacfc244..2a24e615fa4a 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -1812,7 +1812,7 @@ int kvm_skip_emulated_instruction(struct kvm_vcpu *vcpu); int kvm_complete_insn_gp(struct kvm_vcpu *vcpu, int err); void __kvm_request_immediate_exit(struct kvm_vcpu *vcpu); -int kvm_is_in_guest(void); +unsigned int kvm_guest_state(void); void __user *__x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size); diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c index 827886c12c16..2dcbd1b30004 100644 --- a/arch/x86/kvm/pmu.c +++ b/arch/x86/kvm/pmu.c @@ -87,7 +87,7 @@ static void kvm_perf_overflow_intr(struct perf_event *perf_event, * woken up. So we should wake it, but this is impossible from * NMI context. Do it from irq work instead. */ - if (!kvm_is_in_guest()) + if (!kvm_guest_state()) irq_work_queue(&pmc_to_pmu(pmc)->irq_work); else kvm_make_request(KVM_REQ_PMI, pmc->vcpu); diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index bbc4e04e67ad..88f709b3759c 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -8035,44 +8035,46 @@ static void kvm_timer_init(void) DEFINE_PER_CPU(struct kvm_vcpu *, current_vcpu); EXPORT_PER_CPU_SYMBOL_GPL(current_vcpu); -int kvm_is_in_guest(void) +static unsigned int kvm_guest_state(void) { - return __this_cpu_read(current_vcpu) != NULL; -} - -static int kvm_is_user_mode(void) -{ - int user_mode = 3; + struct kvm_vcpu *vcpu = __this_cpu_read(current_vcpu); + unsigned int state = 0; - if (__this_cpu_read(current_vcpu)) - user_mode = static_call(kvm_x86_get_cpl)(__this_cpu_read(current_vcpu)); + if (vcpu) + state |= PERF_GUEST_ACTIVE; + if (static_call(kvm_x86_get_cpl)(vcpu)) + state |= PERF_GUEST_USER; - return user_mode != 0; + return state; } -static unsigned long kvm_get_guest_ip(void) +static unsigned long kvm_guest_get_ip(void) { + struct kvm_vcpu *vcpu = __this_cpu_read(current_vcpu); unsigned long ip = 0; - if (__this_cpu_read(current_vcpu)) - ip = kvm_rip_read(__this_cpu_read(current_vcpu)); + if (vcpu) + ip = kvm_rip_read(vcpu); return ip; } -static void kvm_handle_intel_pt_intr(void) +static unsigned int kvm_handle_intel_pt_intr(void) { struct kvm_vcpu *vcpu = __this_cpu_read(current_vcpu); + if (!vcpu) + return 0; + kvm_make_request(KVM_REQ_PMI, vcpu); __set_bit(MSR_CORE_PERF_GLOBAL_OVF_CTRL_TRACE_TOPA_PMI_BIT, (unsigned long *)&vcpu->arch.pmu.global_status); + return 1; } static struct perf_guest_info_callbacks kvm_guest_cbs = { - .is_in_guest = kvm_is_in_guest, - .is_user_mode = kvm_is_user_mode, - .get_guest_ip = kvm_get_guest_ip, + .state = kvm_guest_state, + .get_ip = kvm_guest_get_ip, .handle_intel_pt_intr = kvm_handle_intel_pt_intr, }; diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h index f5a6a2f069ed..7eae1fd22db3 100644 --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h @@ -26,11 +26,13 @@ # include <asm/local64.h> #endif +#define PERF_GUEST_ACTIVE 0x01 +#define PERF_GUEST_USER 0x02 + struct perf_guest_info_callbacks { - int (*is_in_guest)(void); - int (*is_user_mode)(void); - unsigned long (*get_guest_ip)(void); - void (*handle_intel_pt_intr)(void); + unsigned int (*state)(void); + unsigned long (*get_ip)(void); + unsigned int (*handle_intel_pt_intr)(void); }; #ifdef CONFIG_HAVE_HW_BREAKPOINT @@ -1237,6 +1239,8 @@ extern void perf_event_bpf_event(struct bpf_prog *prog, u16 flags); extern struct perf_guest_info_callbacks *perf_guest_cbs; +extern void __weak arch_perf_update_guest_cbs(void); + extern int perf_register_guest_info_callbacks(struct perf_guest_info_callbacks *callbacks); extern int perf_unregister_guest_info_callbacks(struct perf_guest_info_callbacks *callbacks); diff --git a/kernel/events/core.c b/kernel/events/core.c index 2e947a485898..aec531fc9c90 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -6486,9 +6486,17 @@ static void perf_pending_event(struct irq_work *entry) */ struct perf_guest_info_callbacks *perf_guest_cbs; +void __weak arch_perf_update_guest_cbs(void) +{ +} + int perf_register_guest_info_callbacks(struct perf_guest_info_callbacks *cbs) { + if (WARN_ON_ONCE(perf_guest_cbs)) + return -EBUSY; + perf_guest_cbs = cbs; + arch_perf_update_guest_cbs(); return 0; } EXPORT_SYMBOL_GPL(perf_register_guest_info_callbacks);
On 2021/5/17 16:16, Peter Zijlstra wrote: > On Tue, May 11, 2021 at 10:42:00AM +0800, Like Xu wrote: >> With PEBS virtualization, the guest PEBS records get delivered to the >> guest DS, and the host pmi handler uses perf_guest_cbs->is_in_guest() >> to distinguish whether the PMI comes from the guest code like Intel PT. >> >> No matter how many guest PEBS counters are overflowed, only triggering >> one fake event is enough. The fake event causes the KVM PMI callback to >> be called, thereby injecting the PEBS overflow PMI into the guest. >> >> KVM may inject the PMI with BUFFER_OVF set, even if the guest DS is >> empty. That should really be harmless. Thus guest PEBS handler would >> retrieve the correct information from its own PEBS records buffer. >> >> Originally-by: Andi Kleen <ak@linux.intel.com> >> Co-developed-by: Kan Liang <kan.liang@linux.intel.com> >> Signed-off-by: Kan Liang <kan.liang@linux.intel.com> >> Signed-off-by: Like Xu <like.xu@linux.intel.com> >> --- >> arch/x86/events/intel/core.c | 40 ++++++++++++++++++++++++++++++++++++ >> 1 file changed, 40 insertions(+) >> >> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c >> index b6e45ee10e16..092ecacf8345 100644 >> --- a/arch/x86/events/intel/core.c >> +++ b/arch/x86/events/intel/core.c >> @@ -2780,6 +2780,43 @@ static void intel_pmu_reset(void) >> local_irq_restore(flags); >> } >> >> +/* >> + * We may be running with guest PEBS events created by KVM, and the >> + * PEBS records are logged into the guest's DS and invisible to host. >> + * >> + * In the case of guest PEBS overflow, we only trigger a fake event >> + * to emulate the PEBS overflow PMI for guest PBES counters in KVM. >> + * The guest will then vm-entry and check the guest DS area to read >> + * the guest PEBS records. >> + * >> + * The contents and other behavior of the guest event do not matter. >> + */ >> +static void x86_pmu_handle_guest_pebs(struct pt_regs *regs, >> + struct perf_sample_data *data) >> +{ >> + struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events); >> + u64 guest_pebs_idxs = cpuc->pebs_enabled & ~cpuc->intel_ctrl_host_mask; >> + struct perf_event *event = NULL; >> + int bit; >> + >> + if (!x86_pmu.pebs_active || !guest_pebs_idxs) >> + return; >> + >> + for_each_set_bit(bit, (unsigned long *)&guest_pebs_idxs, >> + INTEL_PMC_IDX_FIXED + x86_pmu.num_counters_fixed) { >> + event = cpuc->events[bit]; >> + if (!event->attr.precise_ip) >> + continue; >> + >> + perf_sample_data_init(data, 0, event->hw.last_period); >> + if (perf_event_overflow(event, data, regs)) >> + x86_pmu_stop(event, 0); >> + >> + /* Inject one fake event is enough. */ >> + break; >> + } >> +} >> + >> static int handle_pmi_common(struct pt_regs *regs, u64 status) >> { >> struct perf_sample_data data; >> @@ -2831,6 +2868,9 @@ static int handle_pmi_common(struct pt_regs *regs, u64 status) >> u64 pebs_enabled = cpuc->pebs_enabled; >> >> handled++; >> + if (x86_pmu.pebs_vmx && perf_guest_cbs && >> + perf_guest_cbs->is_in_guest()) >> + x86_pmu_handle_guest_pebs(regs, &data); >> x86_pmu.drain_pebs(regs, &data); >> status &= intel_ctrl | GLOBAL_STATUS_TRACE_TOPAPMI; >> > I'm thinking you have your conditions in the wrong order; would it not > be much cheaper to first check: '!x86_pmu.pebs_active || !guest_pebs_idx' > than to do that horrible indirect ->is_in_guest() call? > > After all, if the guest doesn't have PEBS enabled, who cares if we're > currently in a guest or not. Yes, it makes sense. How about: @@ -2833,6 +2867,10 @@ static int handle_pmi_common(struct pt_regs *regs, u64 status) u64 pebs_enabled = cpuc->pebs_enabled; handled++; + if (x86_pmu.pebs_vmx && x86_pmu.pebs_active && + (cpuc->pebs_enabled & ~cpuc->intel_ctrl_host_mask) && + (static_call(x86_guest_state)() & PERF_GUEST_ACTIVE)) + x86_pmu_handle_guest_pebs(regs, &data); x86_pmu.drain_pebs(regs, &data); status &= intel_ctrl | GLOBAL_STATUS_TRACE_TOPAPMI; > > Also, something like the below perhaps (arm64 and xen need fixing up at > the very least) could make all that perf_guest_cbs stuff suck less. How about the commit message for your below patch: From: "Peter Zijlstra (Intel)" <peterz@infradead.org> x86/core: Use static_call to rewrite perf_guest_info_callbacks The two fields named "is_in_guest" and "is_user_mode" in perf_guest_info_callbacks are replaced with a new multiplexed member named "state", and the "get_guest_ip" field will be renamed to "get_ip". The application of DEFINE_STATIC_CALL_RET0 (arm64 and xen need fixing up at the very least) could make all that perf_guest_cbs stuff suck less. For KVM, these callbacks will be updated in the kvm_arch_init(). Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> ---- I'm not sue if you have a strong reason to violate the check-patch rule: ERROR: Using weak declarations can have unintended link defects #238: FILE: include/linux/perf_event.h:1242: +extern void __weak arch_perf_update_guest_cbs(void); ? > > --- > diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c > index 8e509325c2c3..c8f8fb7c0536 100644 > --- a/arch/x86/events/core.c > +++ b/arch/x86/events/core.c > @@ -90,6 +90,26 @@ DEFINE_STATIC_CALL_NULL(x86_pmu_pebs_aliases, *x86_pmu.pebs_aliases); > */ > DEFINE_STATIC_CALL_RET0(x86_pmu_guest_get_msrs, *x86_pmu.guest_get_msrs); > > +DEFINE_STATIC_CALL_RET0(x86_guest_state, *(perf_guest_cbs->state)); > +DEFINE_STATIC_CALL_RET0(x86_guest_get_ip, *(perf_guest_cbs->get_ip)); > +DEFINE_STATIC_CALL_RET0(x86_guest_handle_intel_pt_intr, *(perf_guest_cbs->handle_intel_pt_intr)); > + > +void arch_perf_update_guest_cbs(void) > +{ > + static_call_update(x86_guest_state, (void *)&__static_call_return0); > + static_call_update(x86_guest_get_ip, (void *)&__static_call_return0); > + static_call_update(x86_guest_handle_intel_pt_intr, (void *)&__static_call_return0); > + > + if (perf_guest_cbs && perf_guest_cbs->state) > + static_call_update(x86_guest_state, perf_guest_cbs->state); > + > + if (perf_guest_cbs && perf_guest_cbs->get_ip) > + static_call_update(x86_guest_get_ip, perf_guest_cbs->get_ip); > + > + if (perf_guest_cbs && perf_guest_cbs->handle_intel_pt_intr) > + static_call_update(x86_guest_handle_intel_pt_intr, perf_guest_cbs->handle_intel_pt_intr); > +} > + > u64 __read_mostly hw_cache_event_ids > [PERF_COUNT_HW_CACHE_MAX] > [PERF_COUNT_HW_CACHE_OP_MAX] > @@ -2736,7 +2756,7 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx *entry, struct pt_regs *re > struct unwind_state state; > unsigned long addr; > > - if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) { > + if (static_call(x86_guest_state)()) { > /* TODO: We don't support guest os callchain now */ > return; > } > @@ -2839,7 +2859,7 @@ perf_callchain_user(struct perf_callchain_entry_ctx *entry, struct pt_regs *regs > struct stack_frame frame; > const struct stack_frame __user *fp; > > - if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) { > + if (static_call(x86_guest_state)()) { > /* TODO: We don't support guest os callchain now */ > return; > } > @@ -2916,18 +2936,21 @@ static unsigned long code_segment_base(struct pt_regs *regs) > > unsigned long perf_instruction_pointer(struct pt_regs *regs) > { > - if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) > - return perf_guest_cbs->get_guest_ip(); > + unsigned long ip = static_call(x86_guest_get_ip)(); > + > + if (likely(!ip)) > + ip = regs->ip + code_segment_base(regs); > > - return regs->ip + code_segment_base(regs); > + return ip; > } > > unsigned long perf_misc_flags(struct pt_regs *regs) > { > + unsigned int guest = static_call(x86_guest_state)(); > int misc = 0; > > - if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) { > - if (perf_guest_cbs->is_user_mode()) > + if (guest) { > + if (guest & PERF_GUEST_USER) > misc |= PERF_RECORD_MISC_GUEST_USER; > else > misc |= PERF_RECORD_MISC_GUEST_KERNEL; > diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c > index 2521d03de5e0..ac422c45f940 100644 > --- a/arch/x86/events/intel/core.c > +++ b/arch/x86/events/intel/core.c > @@ -2780,6 +2780,8 @@ static void intel_pmu_reset(void) > local_irq_restore(flags); > } > > +DECLARE_STATIC_CALL(x86_guest_handle_intel_pt_intr, *(perf_guest_cbs->handle_intel_pt_intr)); > + > static int handle_pmi_common(struct pt_regs *regs, u64 status) > { > struct perf_sample_data data; > @@ -2850,10 +2852,7 @@ static int handle_pmi_common(struct pt_regs *regs, u64 status) > */ > if (__test_and_clear_bit(GLOBAL_STATUS_TRACE_TOPAPMI_BIT, (unsigned long *)&status)) { > handled++; > - if (unlikely(perf_guest_cbs && perf_guest_cbs->is_in_guest() && > - perf_guest_cbs->handle_intel_pt_intr)) > - perf_guest_cbs->handle_intel_pt_intr(); > - else > + if (!static_call(x86_guest_handle_intel_pt_intr)()) > intel_pt_interrupt(); > } > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 55efbacfc244..2a24e615fa4a 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -1812,7 +1812,7 @@ int kvm_skip_emulated_instruction(struct kvm_vcpu *vcpu); > int kvm_complete_insn_gp(struct kvm_vcpu *vcpu, int err); > void __kvm_request_immediate_exit(struct kvm_vcpu *vcpu); > > -int kvm_is_in_guest(void); > +unsigned int kvm_guest_state(void); > > void __user *__x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, > u32 size); > diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c > index 827886c12c16..2dcbd1b30004 100644 > --- a/arch/x86/kvm/pmu.c > +++ b/arch/x86/kvm/pmu.c > @@ -87,7 +87,7 @@ static void kvm_perf_overflow_intr(struct perf_event *perf_event, > * woken up. So we should wake it, but this is impossible from > * NMI context. Do it from irq work instead. > */ > - if (!kvm_is_in_guest()) > + if (!kvm_guest_state()) > irq_work_queue(&pmc_to_pmu(pmc)->irq_work); > else > kvm_make_request(KVM_REQ_PMI, pmc->vcpu); > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index bbc4e04e67ad..88f709b3759c 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -8035,44 +8035,46 @@ static void kvm_timer_init(void) > DEFINE_PER_CPU(struct kvm_vcpu *, current_vcpu); > EXPORT_PER_CPU_SYMBOL_GPL(current_vcpu); > > -int kvm_is_in_guest(void) > +static unsigned int kvm_guest_state(void) > { > - return __this_cpu_read(current_vcpu) != NULL; > -} > - > -static int kvm_is_user_mode(void) > -{ > - int user_mode = 3; > + struct kvm_vcpu *vcpu = __this_cpu_read(current_vcpu); > + unsigned int state = 0; > > - if (__this_cpu_read(current_vcpu)) > - user_mode = static_call(kvm_x86_get_cpl)(__this_cpu_read(current_vcpu)); > + if (vcpu) > + state |= PERF_GUEST_ACTIVE; > + if (static_call(kvm_x86_get_cpl)(vcpu)) > + state |= PERF_GUEST_USER; > > - return user_mode != 0; > + return state; > } > > -static unsigned long kvm_get_guest_ip(void) > +static unsigned long kvm_guest_get_ip(void) > { > + struct kvm_vcpu *vcpu = __this_cpu_read(current_vcpu); > unsigned long ip = 0; > > - if (__this_cpu_read(current_vcpu)) > - ip = kvm_rip_read(__this_cpu_read(current_vcpu)); > + if (vcpu) > + ip = kvm_rip_read(vcpu); > > return ip; > } > > -static void kvm_handle_intel_pt_intr(void) > +static unsigned int kvm_handle_intel_pt_intr(void) > { > struct kvm_vcpu *vcpu = __this_cpu_read(current_vcpu); > > + if (!vcpu) > + return 0; > + > kvm_make_request(KVM_REQ_PMI, vcpu); > __set_bit(MSR_CORE_PERF_GLOBAL_OVF_CTRL_TRACE_TOPA_PMI_BIT, > (unsigned long *)&vcpu->arch.pmu.global_status); > + return 1; > } > > static struct perf_guest_info_callbacks kvm_guest_cbs = { > - .is_in_guest = kvm_is_in_guest, > - .is_user_mode = kvm_is_user_mode, > - .get_guest_ip = kvm_get_guest_ip, > + .state = kvm_guest_state, > + .get_ip = kvm_guest_get_ip, > .handle_intel_pt_intr = kvm_handle_intel_pt_intr, > }; > > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h > index f5a6a2f069ed..7eae1fd22db3 100644 > --- a/include/linux/perf_event.h > +++ b/include/linux/perf_event.h > @@ -26,11 +26,13 @@ > # include <asm/local64.h> > #endif > > +#define PERF_GUEST_ACTIVE 0x01 > +#define PERF_GUEST_USER 0x02 > + > struct perf_guest_info_callbacks { > - int (*is_in_guest)(void); > - int (*is_user_mode)(void); > - unsigned long (*get_guest_ip)(void); > - void (*handle_intel_pt_intr)(void); > + unsigned int (*state)(void); > + unsigned long (*get_ip)(void); > + unsigned int (*handle_intel_pt_intr)(void); > }; > > #ifdef CONFIG_HAVE_HW_BREAKPOINT > @@ -1237,6 +1239,8 @@ extern void perf_event_bpf_event(struct bpf_prog *prog, > u16 flags); > > extern struct perf_guest_info_callbacks *perf_guest_cbs; > +extern void __weak arch_perf_update_guest_cbs(void); > + > extern int perf_register_guest_info_callbacks(struct perf_guest_info_callbacks *callbacks); > extern int perf_unregister_guest_info_callbacks(struct perf_guest_info_callbacks *callbacks); > > diff --git a/kernel/events/core.c b/kernel/events/core.c > index 2e947a485898..aec531fc9c90 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -6486,9 +6486,17 @@ static void perf_pending_event(struct irq_work *entry) > */ > struct perf_guest_info_callbacks *perf_guest_cbs; > > +void __weak arch_perf_update_guest_cbs(void) > +{ > +} > + > int perf_register_guest_info_callbacks(struct perf_guest_info_callbacks *cbs) > { > + if (WARN_ON_ONCE(perf_guest_cbs)) > + return -EBUSY; > + > perf_guest_cbs = cbs; > + arch_perf_update_guest_cbs(); > return 0; > } > EXPORT_SYMBOL_GPL(perf_register_guest_info_callbacks);
On Tue, May 18, 2021 at 03:38:52PM +0800, Xu, Like wrote: > > I'm thinking you have your conditions in the wrong order; would it not > > be much cheaper to first check: '!x86_pmu.pebs_active || !guest_pebs_idx' > > than to do that horrible indirect ->is_in_guest() call? > > > > After all, if the guest doesn't have PEBS enabled, who cares if we're > > currently in a guest or not. > > Yes, it makes sense. How about: > > @@ -2833,6 +2867,10 @@ static int handle_pmi_common(struct pt_regs *regs, > u64 status) > u64 pebs_enabled = cpuc->pebs_enabled; > > handled++; > + if (x86_pmu.pebs_vmx && x86_pmu.pebs_active && > + (cpuc->pebs_enabled & ~cpuc->intel_ctrl_host_mask) && > + (static_call(x86_guest_state)() & PERF_GUEST_ACTIVE)) > + x86_pmu_handle_guest_pebs(regs, &data); This is terruble, just call x86_pmu_handle_guest_pebs() unconditionally and put all the ugly inside it. > x86_pmu.drain_pebs(regs, &data); > status &= intel_ctrl | GLOBAL_STATUS_TRACE_TOPAPMI; > > > > > Also, something like the below perhaps (arm64 and xen need fixing up at > > the very least) could make all that perf_guest_cbs stuff suck less. > > How about the commit message for your below patch: > > From: "Peter Zijlstra (Intel)" <peterz@infradead.org> > > x86/core: Use static_call to rewrite perf_guest_info_callbacks > > The two fields named "is_in_guest" and "is_user_mode" in > perf_guest_info_callbacks are replaced with a new multiplexed member > named "state", and the "get_guest_ip" field will be renamed to "get_ip". > > The application of DEFINE_STATIC_CALL_RET0 (arm64 and xen need fixing > up at the very least) could make all that perf_guest_cbs stuff suck less. > For KVM, these callbacks will be updated in the kvm_arch_init(). > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Well, you *do* need to fix up arm64 and xen, we can't very well break their builds can we now. > ---- > > I'm not sue if you have a strong reason to violate the check-patch rule: > > ERROR: Using weak declarations can have unintended link defects > #238: FILE: include/linux/perf_event.h:1242: > +extern void __weak arch_perf_update_guest_cbs(void); Copy/paste fail I think. I didn't really put much effort into the patch, only made sure defconfig+kvm_guest.config compiled.
diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c index b6e45ee10e16..092ecacf8345 100644 --- a/arch/x86/events/intel/core.c +++ b/arch/x86/events/intel/core.c @@ -2780,6 +2780,43 @@ static void intel_pmu_reset(void) local_irq_restore(flags); } +/* + * We may be running with guest PEBS events created by KVM, and the + * PEBS records are logged into the guest's DS and invisible to host. + * + * In the case of guest PEBS overflow, we only trigger a fake event + * to emulate the PEBS overflow PMI for guest PBES counters in KVM. + * The guest will then vm-entry and check the guest DS area to read + * the guest PEBS records. + * + * The contents and other behavior of the guest event do not matter. + */ +static void x86_pmu_handle_guest_pebs(struct pt_regs *regs, + struct perf_sample_data *data) +{ + struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events); + u64 guest_pebs_idxs = cpuc->pebs_enabled & ~cpuc->intel_ctrl_host_mask; + struct perf_event *event = NULL; + int bit; + + if (!x86_pmu.pebs_active || !guest_pebs_idxs) + return; + + for_each_set_bit(bit, (unsigned long *)&guest_pebs_idxs, + INTEL_PMC_IDX_FIXED + x86_pmu.num_counters_fixed) { + event = cpuc->events[bit]; + if (!event->attr.precise_ip) + continue; + + perf_sample_data_init(data, 0, event->hw.last_period); + if (perf_event_overflow(event, data, regs)) + x86_pmu_stop(event, 0); + + /* Inject one fake event is enough. */ + break; + } +} + static int handle_pmi_common(struct pt_regs *regs, u64 status) { struct perf_sample_data data; @@ -2831,6 +2868,9 @@ static int handle_pmi_common(struct pt_regs *regs, u64 status) u64 pebs_enabled = cpuc->pebs_enabled; handled++; + if (x86_pmu.pebs_vmx && perf_guest_cbs && + perf_guest_cbs->is_in_guest()) + x86_pmu_handle_guest_pebs(regs, &data); x86_pmu.drain_pebs(regs, &data); status &= intel_ctrl | GLOBAL_STATUS_TRACE_TOPAPMI;