[Patch v3 0/6] Perf kvm commands bug fix

[Patch v3 0/6] Perf kvm commands bug fix

[Dapeng Mi]

This patch-set fixes perf kvm commands issues, like missed memory
allocation check/free, out of range memory access and especially the
issue that fails to sample guest with "perf kvm record/top" commands on
Intel platforms.

Since the change "KVM: x86/pmu: Add basic support to enable guest PEBS
 via DS"[1] starts, host loses the capability to sample guest with PEBS
since all PEBS related MSRs are switched to guest value after vm-entry,
like IA32_DS_AREA MSR is switched to guest GVA at vm-entry. This leads
to PEBS events can't be used to sample guest by host, otherwise no guest
PEBS records can be really sampled. The patches 5-6/6 fix this issue by
using "cycles" event instead of PEBS event "cycles:P" to sample guest on
Intel platforms.

Changes:
  v2 -> v3:
  * Correct the fixes tag in patch 5-6/6.
  * Add "acked-by" tag from Namhyung.

  v1 -> v2:
  * Free memory allocated by strdup().
  * Check "--pfm-events" in kvm_add_default_arch_event() as well.
  * Opportunistically fix the missed memory allocation and free issue in
    builtin-kwork.
  * Comments refine.


Tests:
  * Run command "perf kvm record -a && perf kvm report" and "perf kvm
    top" on Intel Sapphire Rapids platform, guest records can be
    captured successfully.
  * Since no powerpc platforms on hand, doesn't check the patches on
    powerpc. Any test on powerpc is appreciated.

Ref:
  [1] https://lore.kernel.org/all/20220411101946.20262-1-likexu@tencent.com/
 

Dapeng Mi (6):
  perf tools kvm: Add missed memory allocation check and free
  perf tools kwork: Add missed memory allocation check and free
  perf tools kvm: Fix the potential out of range memory access issue
  perf tools: Add helper x86__is_intel_cpu()
  perf tools kvm: Use "cycles" to sample guest for "kvm record" on Intel
  perf tools kvm: Use "cycles" to sample guest for "kvm top" on Intel

 tools/perf/arch/x86/util/kvm-stat.c |  51 +++++++++++
 tools/perf/builtin-kvm.c            | 130 ++++++++++++++++++++--------
 tools/perf/builtin-kwork.c          |  27 ++++--
 tools/perf/util/env.c               |  22 +++++
 tools/perf/util/env.h               |   2 +
 tools/perf/util/kvm-stat.h          |  10 +++
 6 files changed, 203 insertions(+), 39 deletions(-)


base-commit: 576bd7a8c90c48e9e93a18de0bcc0fa1e0a5433b
-- 
2.34.1

[Patch v3 1/6] perf tools kvm: Add missed memory allocation check and free

[Dapeng Mi]

Current code allocates rec_argv[] array, but doesn't check if the
allocation is successful and explicitly free the rec_argv[] array.

Add them back.

Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
Acked-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-kvm.c | 81 ++++++++++++++++++++++++++++------------
 1 file changed, 58 insertions(+), 23 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index 7b15b4a705e4..9813f1eb56f9 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -1638,9 +1638,11 @@ static int kvm_events_report_vcpu(struct perf_kvm_stat *kvm)
 
 #define STRDUP_FAIL_EXIT(s)		\
 	({	char *_p;		\
-	_p = strdup(s);		\
-		if (!_p)		\
-			return -ENOMEM;	\
+		_p = strdup(s);		\
+		if (!_p) {		\
+			ret = -ENOMEM;	\
+			goto EXIT;	\
+		}			\
 		_p;			\
 	})
 
@@ -1688,7 +1690,7 @@ kvm_events_record(struct perf_kvm_stat *kvm, int argc, const char **argv)
 		rec_argv[i] = STRDUP_FAIL_EXIT(record_args[i]);
 
 	for (j = 0; j < events_tp_size; j++) {
-		rec_argv[i++] = "-e";
+		rec_argv[i++] = STRDUP_FAIL_EXIT("-e");
 		rec_argv[i++] = STRDUP_FAIL_EXIT(kvm_events_tp[j]);
 	}
 
@@ -1696,7 +1698,7 @@ kvm_events_record(struct perf_kvm_stat *kvm, int argc, const char **argv)
 	rec_argv[i++] = STRDUP_FAIL_EXIT(kvm->file_name);
 
 	for (j = 1; j < (unsigned int)argc; j++, i++)
-		rec_argv[i] = argv[j];
+		rec_argv[i] = STRDUP_FAIL_EXIT(argv[j]);
 
 	set_option_flag(record_options, 'e', "event", PARSE_OPT_HIDDEN);
 	set_option_flag(record_options, 0, "filter", PARSE_OPT_HIDDEN);
@@ -1719,7 +1721,13 @@ kvm_events_record(struct perf_kvm_stat *kvm, int argc, const char **argv)
 	set_option_flag(record_options, 0, "transaction", PARSE_OPT_DISABLED);
 
 	record_usage = kvm_stat_record_usage;
-	return cmd_record(i, rec_argv);
+	ret = cmd_record(i, rec_argv);
+
+EXIT:
+	for (i = 0; i < rec_argc; i++)
+		free((void *)rec_argv[i]);
+	free(rec_argv);
+	return ret;
 }
 
 static int
@@ -2006,52 +2014,79 @@ static int __cmd_record(const char *file_name, int argc, const char **argv)
 
 	rec_argc = argc + 2;
 	rec_argv = calloc(rec_argc + 1, sizeof(char *));
-	rec_argv[i++] = strdup("record");
-	rec_argv[i++] = strdup("-o");
-	rec_argv[i++] = strdup(file_name);
+	if (!rec_argv)
+		return -ENOMEM;
+
+	rec_argv[i++] = STRDUP_FAIL_EXIT("record");
+	rec_argv[i++] = STRDUP_FAIL_EXIT("-o");
+	rec_argv[i++] = STRDUP_FAIL_EXIT(file_name);
 	for (j = 1; j < argc; j++, i++)
-		rec_argv[i] = argv[j];
+		rec_argv[i] = STRDUP_FAIL_EXIT(argv[j]);
 
 	BUG_ON(i != rec_argc);
 
-	return cmd_record(i, rec_argv);
+	ret = cmd_record(i, rec_argv);
+
+EXIT:
+	for (i = 0; i < rec_argc; i++)
+		free((void *)rec_argv[i]);
+	free(rec_argv);
+	return ret;
 }
 
 static int __cmd_report(const char *file_name, int argc, const char **argv)
 {
-	int rec_argc, i = 0, j;
+	int rec_argc, i = 0, j, ret;
 	const char **rec_argv;
 
 	rec_argc = argc + 2;
 	rec_argv = calloc(rec_argc + 1, sizeof(char *));
-	rec_argv[i++] = strdup("report");
-	rec_argv[i++] = strdup("-i");
-	rec_argv[i++] = strdup(file_name);
+	if (!rec_argv)
+		return -ENOMEM;
+
+	rec_argv[i++] = STRDUP_FAIL_EXIT("report");
+	rec_argv[i++] = STRDUP_FAIL_EXIT("-i");
+	rec_argv[i++] = STRDUP_FAIL_EXIT(file_name);
 	for (j = 1; j < argc; j++, i++)
-		rec_argv[i] = argv[j];
+		rec_argv[i] = STRDUP_FAIL_EXIT(argv[j]);
 
 	BUG_ON(i != rec_argc);
 
-	return cmd_report(i, rec_argv);
+	ret = cmd_report(i, rec_argv);
+
+EXIT:
+	for (i = 0; i < rec_argc; i++)
+		free((void *)rec_argv[i]);
+	free(rec_argv);
+	return ret;
 }
 
 static int
 __cmd_buildid_list(const char *file_name, int argc, const char **argv)
 {
-	int rec_argc, i = 0, j;
+	int rec_argc, i = 0, j, ret;
 	const char **rec_argv;
 
 	rec_argc = argc + 2;
 	rec_argv = calloc(rec_argc + 1, sizeof(char *));
-	rec_argv[i++] = strdup("buildid-list");
-	rec_argv[i++] = strdup("-i");
-	rec_argv[i++] = strdup(file_name);
+	if (!rec_argv)
+		return -ENOMEM;
+
+	rec_argv[i++] = STRDUP_FAIL_EXIT("buildid-list");
+	rec_argv[i++] = STRDUP_FAIL_EXIT("-i");
+	rec_argv[i++] = STRDUP_FAIL_EXIT(file_name);
 	for (j = 1; j < argc; j++, i++)
-		rec_argv[i] = argv[j];
+		rec_argv[i] = STRDUP_FAIL_EXIT(argv[j]);
 
 	BUG_ON(i != rec_argc);
 
-	return cmd_buildid_list(i, rec_argv);
+	ret = cmd_buildid_list(i, rec_argv);
+
+EXIT:
+	for (i = 0; i < rec_argc; i++)
+		free((void *)rec_argv[i]);
+	free(rec_argv);
+	return ret;
 }
 
 int cmd_kvm(int argc, const char **argv)
-- 
2.34.1

[Patch v3 2/6] perf tools kwork: Add missed memory allocation check and free

[Dapeng Mi]

Same with previous builtin-kvm code, perf_kwork__record() doesn't check
the memory allocation and explicitly free the allocated memory. Just fix
it.

Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
Acked-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-kwork.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/tools/perf/builtin-kwork.c b/tools/perf/builtin-kwork.c
index d2e08de5976d..7f3068264568 100644
--- a/tools/perf/builtin-kwork.c
+++ b/tools/perf/builtin-kwork.c
@@ -2273,12 +2273,23 @@ static void setup_event_list(struct perf_kwork *kwork,
 	pr_debug("\n");
 }
 
+#define STRDUP_FAIL_EXIT(s)		\
+	({	char *_p;		\
+		_p = strdup(s);		\
+		if (!_p) {		\
+			ret = -ENOMEM;	\
+			goto EXIT;	\
+		}			\
+		_p;			\
+	})
+
 static int perf_kwork__record(struct perf_kwork *kwork,
 			      int argc, const char **argv)
 {
 	const char **rec_argv;
 	unsigned int rec_argc, i, j;
 	struct kwork_class *class;
+	int ret;
 
 	const char *const record_args[] = {
 		"record",
@@ -2298,17 +2309,17 @@ static int perf_kwork__record(struct perf_kwork *kwork,
 		return -ENOMEM;
 
 	for (i = 0; i < ARRAY_SIZE(record_args); i++)
-		rec_argv[i] = strdup(record_args[i]);
+		rec_argv[i] = STRDUP_FAIL_EXIT(record_args[i]);
 
 	list_for_each_entry(class, &kwork->class_list, list) {
 		for (j = 0; j < class->nr_tracepoints; j++) {
-			rec_argv[i++] = strdup("-e");
-			rec_argv[i++] = strdup(class->tp_handlers[j].name);
+			rec_argv[i++] = STRDUP_FAIL_EXIT("-e");
+			rec_argv[i++] = STRDUP_FAIL_EXIT(class->tp_handlers[j].name);
 		}
 	}
 
 	for (j = 1; j < (unsigned int)argc; j++, i++)
-		rec_argv[i] = argv[j];
+		rec_argv[i] = STRDUP_FAIL_EXIT(argv[j]);
 
 	BUG_ON(i != rec_argc);
 
@@ -2317,7 +2328,13 @@ static int perf_kwork__record(struct perf_kwork *kwork,
 		pr_debug("%s ", rec_argv[j]);
 	pr_debug("\n");
 
-	return cmd_record(i, rec_argv);
+	ret = cmd_record(i, rec_argv);
+
+EXIT:
+	for (i = 0; i < rec_argc; i++)
+		free((void *)rec_argv[i]);
+	free(rec_argv);
+	return ret;
 }
 
 int cmd_kwork(int argc, const char **argv)
-- 
2.34.1

[Patch v3 3/6] perf tools kvm: Fix the potential out of range memory access issue

[Dapeng Mi]

kvm_add_default_arch_event() helper may add 2 extra options but it
directly modifies the original argv[] array. This may cause out of range
memory access. Fix this issue.

Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
Acked-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-kvm.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index 9813f1eb56f9..d297a7b2c088 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -2008,11 +2008,12 @@ static int __cmd_record(const char *file_name, int argc, const char **argv)
 	int rec_argc, i = 0, j, ret;
 	const char **rec_argv;
 
-	ret = kvm_add_default_arch_event(&argc, argv);
-	if (ret)
-		return -EINVAL;
-
-	rec_argc = argc + 2;
+	/*
+	 * Besides the 2 more options "-o" and "filename",
+	 * kvm_add_default_arch_event() may add 2 extra options,
+	 * so allocate 4 more items.
+	 */
+	rec_argc = argc + 2 + 2;
 	rec_argv = calloc(rec_argc + 1, sizeof(char *));
 	if (!rec_argv)
 		return -ENOMEM;
@@ -2025,6 +2026,10 @@ static int __cmd_record(const char *file_name, int argc, const char **argv)
 
 	BUG_ON(i != rec_argc);
 
+	ret = kvm_add_default_arch_event(&i, rec_argv);
+	if (ret)
+		goto EXIT;
+
 	ret = cmd_record(i, rec_argv);
 
 EXIT:
-- 
2.34.1

[Patch v3 4/6] perf tools: Add helper x86__is_intel_cpu()

[Dapeng Mi]

Add helper x86__is_intel_cpu() to indicate if it's a x86 intel platform.

Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
Acked-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/env.c | 22 ++++++++++++++++++++++
 tools/perf/util/env.h |  2 ++
 2 files changed, 24 insertions(+)

diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
index c8c248754621..f1626d2032cd 100644
--- a/tools/perf/util/env.c
+++ b/tools/perf/util/env.c
@@ -802,3 +802,25 @@ bool x86__is_amd_cpu(void)
 
 	return is_amd;
 }
+
+bool perf_env__is_x86_intel_cpu(struct perf_env *env)
+{
+	static int is_intel; /* 0: Uninitialized, 1: Yes, -1: No */
+
+	if (is_intel == 0)
+		is_intel = env->cpuid && strstarts(env->cpuid, "GenuineIntel") ? 1 : -1;
+
+	return is_intel >= 1 ? true : false;
+}
+
+bool x86__is_intel_cpu(void)
+{
+	struct perf_env env = { .total_mem = 0, };
+	bool is_intel;
+
+	perf_env__cpuid(&env);
+	is_intel = perf_env__is_x86_intel_cpu(&env);
+	perf_env__exit(&env);
+
+	return is_intel;
+}
diff --git a/tools/perf/util/env.h b/tools/perf/util/env.h
index e00179787a34..9977b85523a8 100644
--- a/tools/perf/util/env.h
+++ b/tools/perf/util/env.h
@@ -201,5 +201,7 @@ void perf_env__find_br_cntr_info(struct perf_env *env,
 
 bool x86__is_amd_cpu(void);
 bool perf_env__is_x86_amd_cpu(struct perf_env *env);
+bool x86__is_intel_cpu(void);
+bool perf_env__is_x86_intel_cpu(struct perf_env *env);
 
 #endif /* __PERF_ENV_H */
-- 
2.34.1

[Patch v3 5/6] perf tools kvm: Use "cycles" to sample guest for "kvm record" on Intel

[Dapeng Mi]

After KVM supports PEBS for guest on Intel platforms
(https://lore.kernel.org/all/20220411101946.20262-1-likexu@tencent.com/),
host loses the capability to sample guest with PEBS since all PEBS related
MSRs are switched to guest value after vm-entry, like IA32_DS_AREA MSR is
switched to guest GVA at vm-entry. This would lead to "perf kvm record"
fails to sample guest on Intel platforms since "cycles:P" event is used to
sample guest by default as below case shows.

sudo perf kvm record -a
^C[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.787 MB perf.data.guest ]

So to ensure guest record can be sampled successfully, use "cycles"
instead of "cycles:P" to sample guest record by default on Intel
platforms. With this patch, the guest record can be sampled
successfully.

sudo perf kvm record -a
^C[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.783 MB perf.data.guest (23 samples) ]

Reported-by: Kevin Tian <kevin.tian@intel.com>
Fixes: cf8e55fe50df ("KVM: x86/pmu: Expose CPUIDs feature bits PDCM, DS, DTES64")
Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
Acked-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/arch/x86/util/kvm-stat.c | 51 +++++++++++++++++++++++++++++
 tools/perf/builtin-kvm.c            | 10 ------
 tools/perf/util/kvm-stat.h          | 10 ++++++
 3 files changed, 61 insertions(+), 10 deletions(-)

diff --git a/tools/perf/arch/x86/util/kvm-stat.c b/tools/perf/arch/x86/util/kvm-stat.c
index 424716518b75..bff36f9345ea 100644
--- a/tools/perf/arch/x86/util/kvm-stat.c
+++ b/tools/perf/arch/x86/util/kvm-stat.c
@@ -3,9 +3,11 @@
 #include <string.h>
 #include "../../../util/kvm-stat.h"
 #include "../../../util/evsel.h"
+#include "../../../util/env.h"
 #include <asm/svm.h>
 #include <asm/vmx.h>
 #include <asm/kvm.h>
+#include <subcmd/parse-options.h>
 
 define_exit_reasons_table(vmx_exit_reasons, VMX_EXIT_REASONS);
 define_exit_reasons_table(svm_exit_reasons, SVM_EXIT_REASONS);
@@ -211,3 +213,52 @@ int cpu_isa_init(struct perf_kvm_stat *kvm, const char *cpuid)
 
 	return 0;
 }
+
+/*
+ * After KVM supports PEBS for guest on Intel platforms
+ * (https://lore.kernel.org/all/20220411101946.20262-1-likexu@tencent.com/),
+ * host loses the capability to sample guest with PEBS since all PEBS related
+ * MSRs are switched to guest value after vm-entry, like IA32_DS_AREA MSR is
+ * switched to guest GVA at vm-entry. This would lead to "perf kvm record"
+ * fails to sample guest on Intel platforms since "cycles:P" event is used to
+ * sample guest by default.
+ *
+ * So, to avoid this issue explicitly use "cycles" instead of "cycles:P" event
+ * by default to sample guest on Intel platforms.
+ */
+int kvm_add_default_arch_event(int *argc, const char **argv)
+{
+	const char **tmp;
+	bool event = false;
+	int ret = 0, i, j = *argc;
+
+	const struct option event_options[] = {
+		OPT_BOOLEAN('e', "event", &event, NULL),
+		OPT_BOOLEAN(0, "pfm-events", &event, NULL),
+		OPT_END()
+	};
+
+	if (!x86__is_intel_cpu())
+		return 0;
+
+	tmp = calloc(j + 1, sizeof(char *));
+	if (!tmp)
+		return -ENOMEM;
+
+	for (i = 0; i < j; i++)
+		tmp[i] = argv[i];
+
+	parse_options(j, tmp, event_options, NULL, PARSE_OPT_KEEP_UNKNOWN);
+	if (!event) {
+		argv[j++] = STRDUP_FAIL_EXIT("-e");
+		argv[j++] = STRDUP_FAIL_EXIT("cycles");
+		*argc += 2;
+	}
+
+	free(tmp);
+	return 0;
+
+EXIT:
+	free(tmp);
+	return ret;
+}
diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index d297a7b2c088..c0d62add4996 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -1636,16 +1636,6 @@ static int kvm_events_report_vcpu(struct perf_kvm_stat *kvm)
 	return ret;
 }
 
-#define STRDUP_FAIL_EXIT(s)		\
-	({	char *_p;		\
-		_p = strdup(s);		\
-		if (!_p) {		\
-			ret = -ENOMEM;	\
-			goto EXIT;	\
-		}			\
-		_p;			\
-	})
-
 int __weak setup_kvm_events_tp(struct perf_kvm_stat *kvm __maybe_unused)
 {
 	return 0;
diff --git a/tools/perf/util/kvm-stat.h b/tools/perf/util/kvm-stat.h
index 4249542544bb..53db3d56108b 100644
--- a/tools/perf/util/kvm-stat.h
+++ b/tools/perf/util/kvm-stat.h
@@ -190,5 +190,15 @@ static inline struct kvm_info *kvm_info__new(void)
 #define kvm_info__zput(ki) do { } while (0)
 #endif /* HAVE_KVM_STAT_SUPPORT */
 
+#define STRDUP_FAIL_EXIT(s)		\
+	({	char *_p;		\
+		_p = strdup(s);		\
+		if (!_p) {		\
+			ret = -ENOMEM;	\
+			goto EXIT;	\
+		}			\
+		_p;			\
+	})
+
 extern int kvm_add_default_arch_event(int *argc, const char **argv);
 #endif /* __PERF_KVM_STAT_H */
-- 
2.34.1

[Patch v3 6/6] perf tools kvm: Use "cycles" to sample guest for "kvm top" on Intel

[Dapeng Mi]

As same reason with previous patch, use "cyles" instead of "cycles:P"
event by default to sample guest for "perf kvm top" command on Intel
platforms.

Reported-by: Kevin Tian <kevin.tian@intel.com>
Fixes: cf8e55fe50df ("KVM: x86/pmu: Expose CPUIDs feature bits PDCM, DS, DTES64")
Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
Acked-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-kvm.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index c0d62add4996..f0f285763f19 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -2084,6 +2084,38 @@ __cmd_buildid_list(const char *file_name, int argc, const char **argv)
 	return ret;
 }
 
+static int __cmd_top(int argc, const char **argv)
+{
+	int rec_argc, i = 0, ret;
+	const char **rec_argv;
+
+	/*
+	 * kvm_add_default_arch_event() may add 2 extra options, so
+	 * allocate 2 more pointers in adavance.
+	 */
+	rec_argc = argc + 2;
+	rec_argv = calloc(rec_argc + 1, sizeof(char *));
+	if (!rec_argv)
+		return -ENOMEM;
+
+	for (i = 0; i < argc; i++)
+		rec_argv[i] = STRDUP_FAIL_EXIT(argv[i]);
+
+	BUG_ON(i != argc);
+
+	ret = kvm_add_default_arch_event(&i, rec_argv);
+	if (ret)
+		goto EXIT;
+
+	ret = cmd_top(i, rec_argv);
+
+EXIT:
+	for (i = 0; i < rec_argc; i++)
+		free((void *)rec_argv[i]);
+	free(rec_argv);
+	return ret;
+}
+
 int cmd_kvm(int argc, const char **argv)
 {
 	const char *file_name = NULL;
@@ -2144,7 +2176,7 @@ int cmd_kvm(int argc, const char **argv)
 	else if (strlen(argv[0]) > 2 && strstarts("diff", argv[0]))
 		return cmd_diff(argc, argv);
 	else if (!strcmp(argv[0], "top"))
-		return cmd_top(argc, argv);
+		return __cmd_top(argc, argv);
 	else if (strlen(argv[0]) > 2 && strstarts("buildid-list", argv[0]))
 		return __cmd_buildid_list(file_name, argc, argv);
 #if defined(HAVE_KVM_STAT_SUPPORT) && defined(HAVE_LIBTRACEEVENT)
-- 
2.34.1

Re: [Patch v3 5/6] perf tools kvm: Use "cycles" to sample guest for "kvm record" on Intel

[Arnaldo Carvalho de Melo]

On Fri, Sep 19, 2025 at 10:16:58AM +0800, Dapeng Mi wrote:
> After KVM supports PEBS for guest on Intel platforms
> (https://lore.kernel.org/all/20220411101946.20262-1-likexu@tencent.com/),

So this isn't something selectable, i.e. with the patch above there is
no way to disable precise samples on the guest and instead allow the
host to use perf kvm with cycles:P to have a more precise view of guest
samples?

I.e. wouldn't it be better to make cycles:P be accepted and since it
fails, it drops precise_ip to zero as its the "most precise" it can use
on the host when the guest is "hoarding"/using PEBS?

> host loses the capability to sample guest with PEBS since all PEBS related
> MSRs are switched to guest value after vm-entry, like IA32_DS_AREA MSR is
> switched to guest GVA at vm-entry. This would lead to "perf kvm record"
> fails to sample guest on Intel platforms since "cycles:P" event is used to
> sample guest by default as below case shows.

Or it is even worse than I thought, the host _can_ ask for cycles:P, get
it but then _when the guest_ vm-entries and while it is running, the
host doesn't have access to it?

Isn't there any programmatic way for the host to know if the guest is
with PEBS and thus make cycles:P turn into plain "cycles"?

- Arnaldo

> sudo perf kvm record -a
> ^C[ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.787 MB perf.data.guest ]
> 
> So to ensure guest record can be sampled successfully, use "cycles"
> instead of "cycles:P" to sample guest record by default on Intel
> platforms. With this patch, the guest record can be sampled
> successfully.

but unconditionally not having access to PEBS :-\

- Arnaldo
 
> sudo perf kvm record -a
> ^C[ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.783 MB perf.data.guest (23 samples) ]
> 
> Reported-by: Kevin Tian <kevin.tian@intel.com>
> Fixes: cf8e55fe50df ("KVM: x86/pmu: Expose CPUIDs feature bits PDCM, DS, DTES64")
> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
> Acked-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/arch/x86/util/kvm-stat.c | 51 +++++++++++++++++++++++++++++
>  tools/perf/builtin-kvm.c            | 10 ------
>  tools/perf/util/kvm-stat.h          | 10 ++++++
>  3 files changed, 61 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/perf/arch/x86/util/kvm-stat.c b/tools/perf/arch/x86/util/kvm-stat.c
> index 424716518b75..bff36f9345ea 100644
> --- a/tools/perf/arch/x86/util/kvm-stat.c
> +++ b/tools/perf/arch/x86/util/kvm-stat.c
> @@ -3,9 +3,11 @@
>  #include <string.h>
>  #include "../../../util/kvm-stat.h"
>  #include "../../../util/evsel.h"
> +#include "../../../util/env.h"
>  #include <asm/svm.h>
>  #include <asm/vmx.h>
>  #include <asm/kvm.h>
> +#include <subcmd/parse-options.h>
>  
>  define_exit_reasons_table(vmx_exit_reasons, VMX_EXIT_REASONS);
>  define_exit_reasons_table(svm_exit_reasons, SVM_EXIT_REASONS);
> @@ -211,3 +213,52 @@ int cpu_isa_init(struct perf_kvm_stat *kvm, const char *cpuid)
>  
>  	return 0;
>  }
> +
> +/*
> + * After KVM supports PEBS for guest on Intel platforms
> + * (https://lore.kernel.org/all/20220411101946.20262-1-likexu@tencent.com/),
> + * host loses the capability to sample guest with PEBS since all PEBS related
> + * MSRs are switched to guest value after vm-entry, like IA32_DS_AREA MSR is
> + * switched to guest GVA at vm-entry. This would lead to "perf kvm record"
> + * fails to sample guest on Intel platforms since "cycles:P" event is used to
> + * sample guest by default.
> + *
> + * So, to avoid this issue explicitly use "cycles" instead of "cycles:P" event
> + * by default to sample guest on Intel platforms.
> + */
> +int kvm_add_default_arch_event(int *argc, const char **argv)
> +{
> +	const char **tmp;
> +	bool event = false;
> +	int ret = 0, i, j = *argc;
> +
> +	const struct option event_options[] = {
> +		OPT_BOOLEAN('e', "event", &event, NULL),
> +		OPT_BOOLEAN(0, "pfm-events", &event, NULL),
> +		OPT_END()
> +	};
> +
> +	if (!x86__is_intel_cpu())
> +		return 0;
> +
> +	tmp = calloc(j + 1, sizeof(char *));
> +	if (!tmp)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < j; i++)
> +		tmp[i] = argv[i];
> +
> +	parse_options(j, tmp, event_options, NULL, PARSE_OPT_KEEP_UNKNOWN);
> +	if (!event) {
> +		argv[j++] = STRDUP_FAIL_EXIT("-e");
> +		argv[j++] = STRDUP_FAIL_EXIT("cycles");
> +		*argc += 2;
> +	}
> +
> +	free(tmp);
> +	return 0;
> +
> +EXIT:
> +	free(tmp);
> +	return ret;
> +}
> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> index d297a7b2c088..c0d62add4996 100644
> --- a/tools/perf/builtin-kvm.c
> +++ b/tools/perf/builtin-kvm.c
> @@ -1636,16 +1636,6 @@ static int kvm_events_report_vcpu(struct perf_kvm_stat *kvm)
>  	return ret;
>  }
>  
> -#define STRDUP_FAIL_EXIT(s)		\
> -	({	char *_p;		\
> -		_p = strdup(s);		\
> -		if (!_p) {		\
> -			ret = -ENOMEM;	\
> -			goto EXIT;	\
> -		}			\
> -		_p;			\
> -	})
> -
>  int __weak setup_kvm_events_tp(struct perf_kvm_stat *kvm __maybe_unused)
>  {
>  	return 0;
> diff --git a/tools/perf/util/kvm-stat.h b/tools/perf/util/kvm-stat.h
> index 4249542544bb..53db3d56108b 100644
> --- a/tools/perf/util/kvm-stat.h
> +++ b/tools/perf/util/kvm-stat.h
> @@ -190,5 +190,15 @@ static inline struct kvm_info *kvm_info__new(void)
>  #define kvm_info__zput(ki) do { } while (0)
>  #endif /* HAVE_KVM_STAT_SUPPORT */
>  
> +#define STRDUP_FAIL_EXIT(s)		\
> +	({	char *_p;		\
> +		_p = strdup(s);		\
> +		if (!_p) {		\
> +			ret = -ENOMEM;	\
> +			goto EXIT;	\
> +		}			\
> +		_p;			\
> +	})
> +
>  extern int kvm_add_default_arch_event(int *argc, const char **argv);
>  #endif /* __PERF_KVM_STAT_H */
> -- 
> 2.34.1

Re: [Patch v3 5/6] perf tools kvm: Use "cycles" to sample guest for "kvm record" on Intel

[Mi, Dapeng]

On 9/20/2025 3:21 AM, Arnaldo Carvalho de Melo wrote:
> On Fri, Sep 19, 2025 at 10:16:58AM +0800, Dapeng Mi wrote:
>> After KVM supports PEBS for guest on Intel platforms
>> (https://lore.kernel.org/all/20220411101946.20262-1-likexu@tencent.com/),
> So this isn't something selectable, i.e. with the patch above there is
> no way to disable precise samples on the guest and instead allow the
> host to use perf kvm with cycles:P to have a more precise view of guest
> samples?
>
> I.e. wouldn't it be better to make cycles:P be accepted and since it
> fails, it drops precise_ip to zero as its the "most precise" it can use
> on the host when the guest is "hoarding"/using PEBS?
>
>> host loses the capability to sample guest with PEBS since all PEBS related
>> MSRs are switched to guest value after vm-entry, like IA32_DS_AREA MSR is
>> switched to guest GVA at vm-entry. This would lead to "perf kvm record"
>> fails to sample guest on Intel platforms since "cycles:P" event is used to
>> sample guest by default as below case shows.
> Or it is even worse than I thought, the host _can_ ask for cycles:P, get
> it but then _when the guest_ vm-entries and while it is running, the
> host doesn't have access to it?

Unfortunately it's the latter one. That's why see the 0 guest records when
running "perf kvm record/top" commands, it would always success that host
creates cycles:P event, but once VM enters guest, guest owns the PEBS HW
resource (All PEBS MSRs are switched to guest values) and host has no way
to touch the PEBS HW resource in guest mode.


>
> Isn't there any programmatic way for the host to know if the guest is
> with PEBS and thus make cycles:P turn into plain "cycles"?

Currently we don't have such kind of code in Kernel. Of course, it can be
added in theory but it could be meaningless since when users try to run
"perf kvm" commands it's probably there are running VMs and the x86 guest
PEBS support in KVM is always enabled. 


>
> - Arnaldo
>
>> sudo perf kvm record -a
>> ^C[ perf record: Woken up 1 times to write data ]
>> [ perf record: Captured and wrote 0.787 MB perf.data.guest ]
>>
>> So to ensure guest record can be sampled successfully, use "cycles"
>> instead of "cycles:P" to sample guest record by default on Intel
>> platforms. With this patch, the guest record can be sampled
>> successfully.
> but unconditionally not having access to PEBS :-\
>
> - Arnaldo
>  
>> sudo perf kvm record -a
>> ^C[ perf record: Woken up 1 times to write data ]
>> [ perf record: Captured and wrote 0.783 MB perf.data.guest (23 samples) ]
>>
>> Reported-by: Kevin Tian <kevin.tian@intel.com>
>> Fixes: cf8e55fe50df ("KVM: x86/pmu: Expose CPUIDs feature bits PDCM, DS, DTES64")
>> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
>> Acked-by: Namhyung Kim <namhyung@kernel.org>
>> ---
>>  tools/perf/arch/x86/util/kvm-stat.c | 51 +++++++++++++++++++++++++++++
>>  tools/perf/builtin-kvm.c            | 10 ------
>>  tools/perf/util/kvm-stat.h          | 10 ++++++
>>  3 files changed, 61 insertions(+), 10 deletions(-)
>>
>> diff --git a/tools/perf/arch/x86/util/kvm-stat.c b/tools/perf/arch/x86/util/kvm-stat.c
>> index 424716518b75..bff36f9345ea 100644
>> --- a/tools/perf/arch/x86/util/kvm-stat.c
>> +++ b/tools/perf/arch/x86/util/kvm-stat.c
>> @@ -3,9 +3,11 @@
>>  #include <string.h>
>>  #include "../../../util/kvm-stat.h"
>>  #include "../../../util/evsel.h"
>> +#include "../../../util/env.h"
>>  #include <asm/svm.h>
>>  #include <asm/vmx.h>
>>  #include <asm/kvm.h>
>> +#include <subcmd/parse-options.h>
>>  
>>  define_exit_reasons_table(vmx_exit_reasons, VMX_EXIT_REASONS);
>>  define_exit_reasons_table(svm_exit_reasons, SVM_EXIT_REASONS);
>> @@ -211,3 +213,52 @@ int cpu_isa_init(struct perf_kvm_stat *kvm, const char *cpuid)
>>  
>>  	return 0;
>>  }
>> +
>> +/*
>> + * After KVM supports PEBS for guest on Intel platforms
>> + * (https://lore.kernel.org/all/20220411101946.20262-1-likexu@tencent.com/),
>> + * host loses the capability to sample guest with PEBS since all PEBS related
>> + * MSRs are switched to guest value after vm-entry, like IA32_DS_AREA MSR is
>> + * switched to guest GVA at vm-entry. This would lead to "perf kvm record"
>> + * fails to sample guest on Intel platforms since "cycles:P" event is used to
>> + * sample guest by default.
>> + *
>> + * So, to avoid this issue explicitly use "cycles" instead of "cycles:P" event
>> + * by default to sample guest on Intel platforms.
>> + */
>> +int kvm_add_default_arch_event(int *argc, const char **argv)
>> +{
>> +	const char **tmp;
>> +	bool event = false;
>> +	int ret = 0, i, j = *argc;
>> +
>> +	const struct option event_options[] = {
>> +		OPT_BOOLEAN('e', "event", &event, NULL),
>> +		OPT_BOOLEAN(0, "pfm-events", &event, NULL),
>> +		OPT_END()
>> +	};
>> +
>> +	if (!x86__is_intel_cpu())
>> +		return 0;
>> +
>> +	tmp = calloc(j + 1, sizeof(char *));
>> +	if (!tmp)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < j; i++)
>> +		tmp[i] = argv[i];
>> +
>> +	parse_options(j, tmp, event_options, NULL, PARSE_OPT_KEEP_UNKNOWN);
>> +	if (!event) {
>> +		argv[j++] = STRDUP_FAIL_EXIT("-e");
>> +		argv[j++] = STRDUP_FAIL_EXIT("cycles");
>> +		*argc += 2;
>> +	}
>> +
>> +	free(tmp);
>> +	return 0;
>> +
>> +EXIT:
>> +	free(tmp);
>> +	return ret;
>> +}
>> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
>> index d297a7b2c088..c0d62add4996 100644
>> --- a/tools/perf/builtin-kvm.c
>> +++ b/tools/perf/builtin-kvm.c
>> @@ -1636,16 +1636,6 @@ static int kvm_events_report_vcpu(struct perf_kvm_stat *kvm)
>>  	return ret;
>>  }
>>  
>> -#define STRDUP_FAIL_EXIT(s)		\
>> -	({	char *_p;		\
>> -		_p = strdup(s);		\
>> -		if (!_p) {		\
>> -			ret = -ENOMEM;	\
>> -			goto EXIT;	\
>> -		}			\
>> -		_p;			\
>> -	})
>> -
>>  int __weak setup_kvm_events_tp(struct perf_kvm_stat *kvm __maybe_unused)
>>  {
>>  	return 0;
>> diff --git a/tools/perf/util/kvm-stat.h b/tools/perf/util/kvm-stat.h
>> index 4249542544bb..53db3d56108b 100644
>> --- a/tools/perf/util/kvm-stat.h
>> +++ b/tools/perf/util/kvm-stat.h
>> @@ -190,5 +190,15 @@ static inline struct kvm_info *kvm_info__new(void)
>>  #define kvm_info__zput(ki) do { } while (0)
>>  #endif /* HAVE_KVM_STAT_SUPPORT */
>>  
>> +#define STRDUP_FAIL_EXIT(s)		\
>> +	({	char *_p;		\
>> +		_p = strdup(s);		\
>> +		if (!_p) {		\
>> +			ret = -ENOMEM;	\
>> +			goto EXIT;	\
>> +		}			\
>> +		_p;			\
>> +	})
>> +
>>  extern int kvm_add_default_arch_event(int *argc, const char **argv);
>>  #endif /* __PERF_KVM_STAT_H */
>> -- 
>> 2.34.1

Re: [Patch v3 0/6] Perf kvm commands bug fix

[Arnaldo Carvalho de Melo]

On Fri, Sep 19, 2025 at 10:16:53AM +0800, Dapeng Mi wrote:
> This patch-set fixes perf kvm commands issues, like missed memory
> allocation check/free, out of range memory access and especially the
> issue that fails to sample guest with "perf kvm record/top" commands on
> Intel platforms.
> 
> Since the change "KVM: x86/pmu: Add basic support to enable guest PEBS
>  via DS"[1] starts, host loses the capability to sample guest with PEBS
> since all PEBS related MSRs are switched to guest value after vm-entry,
> like IA32_DS_AREA MSR is switched to guest GVA at vm-entry. This leads
> to PEBS events can't be used to sample guest by host, otherwise no guest
> PEBS records can be really sampled. The patches 5-6/6 fix this issue by
> using "cycles" event instead of PEBS event "cycles:P" to sample guest on
> Intel platforms.
> 
> Changes:
>   v2 -> v3:
>   * Correct the fixes tag in patch 5-6/6.
>   * Add "acked-by" tag from Namhyung.
> 

Thanks, applied to perf-tools-next,

- Arnaldo