Re: RFC: convert KVMTRACE to event traces

[Christoph Hellwig <hch@infradead.org>]

On Thu, May 14, 2009 at 05:30:16PM -0300, Marcelo Tosatti wrote:
> +		trace_kvm_cr_write(cr, val);
>  		switch (cr) {
>  		case 0:
> -			kvm_set_cr0(vcpu, kvm_register_read(vcpu, reg));
> +			kvm_set_cr0(vcpu, val);
>  			skip_emulated_instruction(vcpu);

Do we really need one trace point covering all cr writes, _and_ one for
each specific register?

>  	if (!npt_enabled)
> -		KVMTRACE_3D(PAGE_FAULT, &svm->vcpu, error_code,
> -			    (u32)fault_address, (u32)(fault_address >> 32),
> -			    handler);
> +		trace_kvm_page_fault(fault_address, error_code);
>  	else
> -		KVMTRACE_3D(TDP_FAULT, &svm->vcpu, error_code,
> -			    (u32)fault_address, (u32)(fault_address >> 32),
> -			    handler);
> +		trace_kvm_tdp_page_fault(fault_address, error_code);

Again this seems a bit cumbersome.  Why not just one tracepoint for
page faults, with a flag if we're using npt or not?

> +ifeq ($(CONFIG_TRACEPOINTS),y)
> +trace-objs = kvm-traces.o
> +arch-trace-objs = kvm-traces-arch.o
> +endif
> +
>  EXTRA_CFLAGS += -Ivirt/kvm -Iarch/x86/kvm
>  
>  kvm-objs := $(common-objs) x86.o mmu.o x86_emulate.o i8259.o irq.o lapic.o \
> -	i8254.o
> +	i8254.o $(trace-objs)
>  obj-$(CONFIG_KVM) += kvm.o
> -kvm-intel-objs = vmx.o
> +kvm-intel-objs = vmx.o $(arch-trace-objs)
>  obj-$(CONFIG_KVM_INTEL) += kvm-intel.o
> -kvm-amd-objs = svm.o
> +kvm-amd-objs = svm.o $(arch-trace-objs)
>  obj-$(CONFIG_KVM_AMD) += kvm-amd.o

The option to select even tracing bits is CONFIG_EVENT_TRACING and the
makefile syntax used here (both the original makefile and the additions)
is rather awkward.

A proper arch/x86/kvm/Makefile including tracing bits should look like
the following:

-- snip --
EXTRA_CFLAGS += -Ivirt/kvm -Iarch/x86/kvm

kvm-y			+= $(addprefix ../../../virt/kvm/, kvm_main.o ioapic.o \
			   coalesced_mmio.o irq_comm.o)
kvm-$(CONFIG_KVM_TRACE)	+= $(addprefix ../../../virt/kvm/, kvm_trace.o)
kvm-$(CONFIG_IOMMU_API)	+= $(addprefix ../../../virt/kvm/, iommu.o)
kmv-y			+= x86.o mmu.o x86_emulate.o i8259.o irq.o lapic.o \
			   i8254.o

kvm-$(CONFIG_EVENT_TRACING) += kvm-traces.o
kvm-arch-trace-$(CONFIG_EVENT_TRACING) += kvm-traces-arch.o

kvm-intel-y		+= vmx.o $(kvm-arch-trace-y)
kvm-amd-y		+= svm.o $(kvm-arch-trace-y)

obj-$(CONFIG_KVM)	+= kvm.o
obj-$(CONFIG_KVM_INTEL)	+= kvm-intel.o
obj-$(CONFIG_KVM_AMD)	+= kvm-amd.o
-- snip --

and do we actually still need kvm_trace.o after this?

Anyway, I'll send the upstream part of the makefile cleanup out ASAP,
then you can rebase later.

> Index: linux-2.6-x86-2/arch/x86/kvm/kvm-traces.c
> ===================================================================
> --- /dev/null
> +++ linux-2.6-x86-2/arch/x86/kvm/kvm-traces.c
> @@ -0,0 +1,5 @@
> +#include <linux/sched.h>
> +
> +
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/kvm/x86.h>

Can't we just put this into some other common .c file?  That would also
reduce the amount of makefile magic required.

> Index: linux-2.6-x86-2/arch/x86/kvm/kvm-traces-arch.c
> ===================================================================
> --- /dev/null
> +++ linux-2.6-x86-2/arch/x86/kvm/kvm-traces-arch.c
> @@ -0,0 +1,5 @@
> +#include <linux/sched.h>
> +
> +
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/kvm/x86-arch.h>

Same for this one, especially as the makefile hackery required for this
one is even worse..