Re: RFC: convert KVMTRACE to event traces

[Marcelo Tosatti <mtosatti@redhat.com>]

On Fri, May 15, 2009 at 01:10:34PM -0400, Christoph Hellwig wrote:
> 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?

There is one tracepoint named kvm_cr that covers cr reads and writes.

kvm_trace_cr_read/kvm_trace_cr_write are macros that expand to
kvm_trace_cr(rw=1 or rw=0). Perhaps that is not a very good idea.

> 
> >  	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?

Issue is the meaning of these faults is different. With npt disabled the
fault is a guest fault (like a normal pagefault), but with npt enabled
the fault indicates the host pagetables the hardware uses to do the
translation are not set up correctly.

I did unify them as you suggest but reverted back to separate
tracepoints because the unification might be confusing.

Can be unified later if desirable.

> > +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?

Your version looks much nicer. kvm_trace.o can disappear as soon as 
this is in Avi's tree and a decent replacement for user/kvm_trace.c 
is in qemu-kvm.git.

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

OK.

> 
> > 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..

Probably for both. Now that you say I can't explain the reason for the
separate C files. Will put this up in a git tree in a couple of hours.