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