Re: [RFC v2 PATCH 3/7] KVM: arm/arm64: Register perf trace event notifier

Christoffer Dall
2016-09-06T06:33:55Z

On Mon, Sep 05, 2016 at 05:31:33PM +0100, Punit Agrawal wrote:
> Register a notifier to track state changes of perf trace events.
>
> The notifier will enable taking appropriate action for trace events
> targeting VM.
>
> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> Cc: Marc Zyngier <marc.zyngier@arm.com>

Overall this looks reasonable, but I'm wondering if most if this logic
should really go in virt/kvm/perf_trace.c and call into arch-specific
hooks, similar to the way it works for preempt notifiers.

On the other hand, if arm/arm64 are the only two architectures that are
going to use this, creating stubs for the other architectures could be a
bit tedious.

Thanks,
-Christoffer

> ---
> arch/arm/include/asm/kvm_host.h | 3 +
> arch/arm/kvm/arm.c | 2 +
> arch/arm64/include/asm/kvm_host.h | 8 +++
> arch/arm64/kvm/Kconfig | 4 ++
> arch/arm64/kvm/Makefile | 1 +
> arch/arm64/kvm/perf_trace.c | 122 ++++++++++++++++++++++++++++++++++++++
> 6 files changed, 140 insertions(+)
> create mode 100644 arch/arm64/kvm/perf_trace.c
>
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index de338d9..609998e 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -280,6 +280,9 @@ static inline int kvm_arch_dev_ioctl_check_extension(struct kvm *kvm, long ext)
> int kvm_perf_init(void);
> int kvm_perf_teardown(void);
>
> +static inline int kvm_perf_trace_init(void) { return 0; }
> +static inline int kvm_perf_trace_teardown(void) { return 0; }
> +
> void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot);
>
> struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 75f130e..e1b99c4 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -1220,6 +1220,7 @@ static int init_subsystems(void)
> goto out;
>
> kvm_perf_init();
> + kvm_perf_trace_init();
> kvm_coproc_table_init();
>
> out:
> @@ -1411,6 +1412,7 @@ out_err:
> void kvm_arch_exit(void)
> {
> kvm_perf_teardown();
> + kvm_perf_trace_teardown();
> }
>
> static int arm_init(void)
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 3eda975..f6ff8e5 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -345,6 +345,14 @@ int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run,
> int kvm_perf_init(void);
> int kvm_perf_teardown(void);
>
> +#if !defined(CONFIG_KVM_PERF_TRACE)
> +static inline int kvm_perf_trace_init(void) { return 0; }
> +static inline int kvm_perf_trace_teardown(void) { return 0; }
> +#else
> +int kvm_perf_trace_init(void);
> +int kvm_perf_trace_teardown(void);
> +#endif
> +
> struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
>
> static inline void __cpu_init_hyp_mode(phys_addr_t pgd_ptr,
> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
> index 9c9edc9..56e9537 100644
> --- a/arch/arm64/kvm/Kconfig
> +++ b/arch/arm64/kvm/Kconfig
> @@ -19,6 +19,9 @@ if VIRTUALIZATION
> config KVM_ARM_VGIC_V3
> bool
>
> +config KVM_PERF_TRACE
> + bool
> +
> config KVM
> bool "Kernel-based Virtual Machine (KVM) support"
> depends on OF
> @@ -39,6 +42,7 @@ config KVM
> select HAVE_KVM_MSI
> select HAVE_KVM_IRQCHIP
> select HAVE_KVM_IRQ_ROUTING
> + select KVM_PERF_TRACE if EVENT_TRACING && PERF_EVENTS
> ---help---
> Support hosting virtualized guest machines.
> We don't support KVM with 16K page tables yet, due to the multiple
> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> index 695eb3c..7d175e4 100644
> --- a/arch/arm64/kvm/Makefile
> +++ b/arch/arm64/kvm/Makefile
> @@ -19,6 +19,7 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(ARM)/psci.o $(ARM)/perf.o
> kvm-$(CONFIG_KVM_ARM_HOST) += emulate.o inject_fault.o regmap.o
> kvm-$(CONFIG_KVM_ARM_HOST) += hyp.o hyp-init.o handle_exit.o
> kvm-$(CONFIG_KVM_ARM_HOST) += guest.o debug.o reset.o sys_regs.o sys_regs_generic_v8.o
> +kvm-$(CONFIG_KVM_PERF_TRACE) += perf_trace.o
>
> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic.o
> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-init.o
> diff --git a/arch/arm64/kvm/perf_trace.c b/arch/arm64/kvm/perf_trace.c
> new file mode 100644
> index 0000000..8bacd18
> --- /dev/null
> +++ b/arch/arm64/kvm/perf_trace.c
> @@ -0,0 +1,122 @@
> +/*
> + * Copyright (C) 2016 ARM Ltd.
> + * Author: Punit Agrawal <punit.agrawal@arm.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +#include <linux/kvm_host.h>
> +#include <linux/trace_events.h>
> +
> +typedef int (*perf_trace_callback_fn)(struct kvm *kvm, bool enable);
> +
> +struct kvm_trace_hook {
> + char *key;
> + perf_trace_callback_fn setup_fn;
> +};
> +
> +static struct kvm_trace_hook trace_hook[] = {
> + { },
> +};
> +
> +static perf_trace_callback_fn find_trace_callback(const char *trace_key)
> +{
> + int i;
> +
> + for (i = 0; trace_hook[i].key; i++)
> + if (!strcmp(trace_key, trace_hook[i].key))
> + return trace_hook[i].setup_fn;
> +
> + return NULL;
> +}
> +
> +static int kvm_perf_trace_notifier(struct notifier_block *nb,
> + unsigned long event, void *data)
> +{
> + struct perf_event *p_event = data;
> + struct trace_event_call *tp_event = p_event->tp_event;
> + perf_trace_callback_fn setup_trace_fn;
> + struct kvm *kvm = NULL;
> + struct pid *pid;
> + bool found = false;
> +
> + /*
> + * Is this a trace point?
> + */
> + if (!(tp_event->flags & TRACE_EVENT_FL_TRACEPOINT))
> + goto out;
> +
> + /*
> + * We'll get here for events we care to monitor for KVM. As we
> + * only care about events attached to a VM, check that there
> + * is a task associated with the perf event.
> + */
> + if (p_event->attach_state != PERF_ATTACH_TASK)
> + goto out;
> +
> + /*
> + * This notifier gets called when perf trace event instance is
> + * added or removed. Until we can restrict this to events of
> + * interest in core, minimise the overhead below.
> + *
> + * Do we care about it? i.e., is there a callback for this
> + * trace point?
> + */
> + setup_trace_fn = find_trace_callback(tp_event->tp->name);
> + if (!setup_trace_fn)
> + goto out;
> +
> + pid = get_task_pid(p_event->hw.target, PIDTYPE_PID);
> +
> + /*
> + * Does it match any of the VMs?
> + */
> + spin_lock(&kvm_lock);
> + list_for_each_entry(kvm, &vm_list, vm_list) {
> + if (kvm->pid == pid) {
> + found = true;
> + break;
> + }
> + }
> + spin_unlock(&kvm_lock);
> +
> + put_pid(pid);
> + if (!found)
> + goto out;
> +
> + switch (event) {
> + case TRACE_REG_PERF_OPEN:
> + setup_trace_fn(kvm, true);
> + break;
> +
> + case TRACE_REG_PERF_CLOSE:
> + setup_trace_fn(kvm, false);
> + break;
> + }
> +
> +out:
> + return 0;
> +}
> +
> +static struct notifier_block kvm_perf_trace_notifier_block = {
> + .notifier_call = kvm_perf_trace_notifier,
> +};
> +
> +int kvm_perf_trace_init(void)
> +{
> + return perf_trace_notifier_register(&kvm_perf_trace_notifier_block);
> +}
> +
> +int kvm_perf_trace_teardown(void)
> +{
> + return perf_trace_notifier_unregister(&kvm_perf_trace_notifier_block);
> +}
> --
> 2.8.1
>