Re: [PATCH 07/22] tools lib traceevent: Add kvm plugin

[Namhyung Kim <namhyung@kernel.org>]

2013-11-21 (목), 12:01 +0100, Jiri Olsa:
> Backporting kvm plugin.
> 
> Backported from Steven Rostedt's trace-cmd repo (HEAD 0f2c2fb):
> git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/trace-cmd.git
> 
> This plugin adds field resolving functions for following
> tracepoint events:
>   kvm:kvm_exit
>   kvm:kvm_emulate_insn
>   kvm:kvm_nested_vmexit
>   kvm:kvm_nested_vmexit_inject
>   kvmmmu:kvm_mmu_get_page
>   kvmmmu:kvm_mmu_sync_page
>   kvmmmu:kvm_mmu_unsync_page
>   kvmmmu:kvm_mmu_zap_page
>   kvmmmu:kvm_mmu_prepare_zap_page
> 
> The diff of 'perf script' output generated by old and new code:
> (data was generated by 'perf record -e 'kvm:*,kvmmmu:*' -a')
> 
> --- script.kvm.old
> +++ script.kvm.new
>   qemu-system-x86 17414 [000]  6868.995053: kvm:kvm_exit: reason EPT_VIOLATION rip 0xfff0 info 184 0
>   qemu-system-x86 17414 [000]  6868.995109: kvm:kvm_emulate_insn: f0000:c46b:e4 71 (real)
> - qemu-system-x86  3006 [002] 10562.079422: kvmmmu:kvm_mmu_get_page: [FAILED TO PARSE] mmu_valid_gen=0x2 gfn=0 role=122884 root_count=0 unsync=0 created=1
> + qemu-system-x86  3006 [002] 10562.079422: kvmmmu:kvm_mmu_get_page: new sp gfn 0 0/4 q0 direct --- !pge !nxe root 0 sync
> - qemu-system-x86  3006 [002] 10562.080502: kvmmmu:kvm_mmu_prepare_zap_page: [FAILED TO PARSE] mmu_valid_gen=0x2 gfn=0 role=122884 root_count=1 unsync=0
> + qemu-system-x86  3006 [002] 10562.080502: kvmmmu:kvm_mmu_prepare_zap_page: 0/4 q0 direct --- !pge !nxe root 1 sync
>   qemu-system-x86  3290 [002] 10708.755312: kvmmmu:fast_page_fault: [FAILED TO PARSE] vcpu_id=0 gva=4094486080 error_code=3 sptep=0xffff88019f1e3670 old_spte=336391285 new_spte=336391287 retry=1
> -          insmod  2576 [001]   781.731666: kvmmmu:kvm_mmu_sync_page: [FAILED TO PARSE] mmu_valid_gen=0x1 gfn=2 role=24624 root_count=10 unsync=1
> +          insmod  2576 [001]   781.731666: kvmmmu:kvm_mmu_sync_page: 3/0 q0 --- !pge !nxe root 10 unsync
> -          insmod  2576 [001]   781.731668: kvmmmu:kvm_mmu_unsync_page: [FAILED TO PARSE] mmu_valid_gen=0x1 gfn=2 role=24624 root_count=10 unsync=1
> +          insmod  2576 [001]   781.731668: kvmmmu:kvm_mmu_unsync_page: 3/0 q0 --- !pge !nxe root 10 unsync
> 
> Note:
>  - kvm_mmu_zap_page is replaced by kvm_mmu_prepare_zap_page
>    in current kernel, keeping it for backward compatibility
>  - some of the tracepoints keep the same output even with
>    the plugin handling: kvm:kvm_exit, kvm:kvm_emulate_insn
>  - the 'kvmmmu:fast_page_fault' is still broken because of
>    missing is_writable_pte function and is fixed in another patch
>  - ommited following tracepoints from backport because
>    the output was buggy
>      kvm:kvm_nested_vmexit
>      kvm:kvm_nested_vmexit_inject

[SNIP]
> +static int kvm_mmu_print_role(struct trace_seq *s, struct pevent_record *record,
> +			      struct event_format *event, void *context)
> +{
> +	unsigned long long val;
> +	static const char *access_str[] = {
> +		"---", "--x", "w--", "w-x", "-u-", "-ux", "wu-", "wux"
> +	};
> +	union kvm_mmu_page_role role;
> +
> +	if (pevent_get_field_val(s, event, "role", record, &val, 1) < 0)
> +		return -1;
> +
> +	role.word = (int)val;
> +
> +	/*
> +	 * We can only use the structure if file is of the same
> +	 * endianess.
> +	 */
> +	if (pevent_is_file_bigendian(event->pevent) ==
> +	    pevent_is_host_bigendian(event->pevent)) {
> +
> +		trace_seq_printf(s, "%u/%u q%u%s %s%s %spge %snxe",
> +				 role.level,
> +				 role.glevels,
> +				 role.quadrant,
> +				 role.direct ? " direct" : "",
> +				 access_str[role.access],
> +				 role.invalid ? " invalid" : "",
> +				 role.cr4_pge ? "" : "!",
> +				 role.nxe ? "" : "!");
> +	} else
> +		trace_seq_printf(s, "WORD: %08x", role.word);
> +
> +	pevent_print_num_field(s, " root %u ",  event,
> +			       "root_count", record, 1);
> +
> +	if (pevent_get_field_val(s, event, "unsync", record, &val, 1) < 0)
> +		return -1;

Hmm.. it seems this returning from the middle of function can make
output hard to parse.  How about printing 'unknown' if the field not
found?

> +
> +	trace_seq_printf(s, "%s%c",  val ? "unsync" : "sync", 0);

Why did you print %c (0) at the end?

Thanks,
Namhyung


> +	return 0;
> +}