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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
