[PATCH v3 0/6] perf kvm: A small update in the default arch event

[PATCH v3 0/6] perf kvm: A small update in the default arch event

[Namhyung Kim]

Hello,

Ian said there's an issue in the perf kvm record command line option parsing.
This is my attempt to address the issue, simplify the code and fix memory leaks.

v3 changes)
 * remove unnecessary error check  (Sashiko)
 * update test code after rebase
 
v2 changes)
 * handle kvm top subcommand  (Sashiko)
 * handle other arch/condition for the default events  (Sashiko)
 * add <stdlib.h> for musl compatibility  (Sashiko)
 * fix some memory leaks

Thanks,
Namhyung


Namhyung Kim (6):
  perf kvm: Factor out kvm_need_default_arch_event()
  perf kvm: Check kvm_need_default_arch_event() early
  perf kvm: Kill STRDUP_FAIL_EXIT()
  perf kvm: Do not copy filename string
  perf kvm: Fix a memory leak in the usage string
  perf test: Extend perf kvm tests to check default event

 tools/perf/builtin-kvm.c                      | 109 ++++++++----------
 tools/perf/tests/shell/kvm.sh                 |  10 ++
 .../util/kvm-stat-arch/kvm-stat-powerpc.c     |  31 +----
 tools/perf/util/kvm-stat-arch/kvm-stat-x86.c  |  37 +-----
 tools/perf/util/kvm-stat.c                    |  41 +++++++
 tools/perf/util/kvm-stat.h                    |  18 ++-
 6 files changed, 116 insertions(+), 130 deletions(-)

-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH v3 1/6] perf kvm: Factor out kvm_need_default_arch_event()

[Namhyung Kim]

The kvm_add_default_arch_event() has a similar logic in each arch to
check if there's an existing command line option for events.  Let's
check it in the generic code and remove the duplication.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-kvm.c                      | 17 +++++---
 .../util/kvm-stat-arch/kvm-stat-powerpc.c     | 31 +++-----------
 tools/perf/util/kvm-stat-arch/kvm-stat-x86.c  | 34 +++------------
 tools/perf/util/kvm-stat.c                    | 41 +++++++++++++++++++
 tools/perf/util/kvm-stat.h                    |  8 ++++
 5 files changed, 70 insertions(+), 61 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index 2c6aef1e13a04cf2..2f00cc1fd1c7cdbd 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -51,6 +51,7 @@
 #include <termios.h>
 #include <semaphore.h>
 #include <signal.h>
+#include <stdlib.h>
 #include <math.h>
 #include <perf/mmap.h>
 
@@ -2014,9 +2015,11 @@ static int __cmd_record(const char *file_name, int argc, const char **argv)
 
 	BUG_ON(i + 2 != rec_argc);
 
-	ret = kvm_add_default_arch_event(EM_HOST, &i, rec_argv);
-	if (ret)
-		goto EXIT;
+	if (kvm_need_default_arch_event(EM_HOST, argc, argv)) {
+		ret = kvm_add_default_arch_event(EM_HOST, &i, rec_argv);
+		if (ret)
+			goto EXIT;
+	}
 
 	ret = cmd_record(i, rec_argv);
 
@@ -2101,9 +2104,11 @@ static int __cmd_top(int argc, const char **argv)
 
 	BUG_ON(i != argc);
 
-	ret = kvm_add_default_arch_event(EM_HOST, &i, rec_argv);
-	if (ret)
-		goto EXIT;
+	if (kvm_need_default_arch_event(EM_HOST, argc, argv)) {
+		ret = kvm_add_default_arch_event(EM_HOST, &i, rec_argv);
+		if (ret)
+			goto EXIT;
+	}
 
 	ret = cmd_top(i, rec_argv);
 
diff --git a/tools/perf/util/kvm-stat-arch/kvm-stat-powerpc.c b/tools/perf/util/kvm-stat-arch/kvm-stat-powerpc.c
index 96d9c4ae020940f0..8d4133c35c12f14b 100644
--- a/tools/perf/util/kvm-stat-arch/kvm-stat-powerpc.c
+++ b/tools/perf/util/kvm-stat-arch/kvm-stat-powerpc.c
@@ -9,7 +9,6 @@
 
 #include "book3s_hv_exits.h"
 #include "book3s_hcalls.h"
-#include <subcmd/parse-options.h>
 
 #define NR_TPS 4
 
@@ -177,35 +176,15 @@ int __cpu_isa_init_powerpc(struct perf_kvm_stat *kvm)
  */
 int __kvm_add_default_arch_event_powerpc(int *argc, const char **argv)
 {
-	const char **tmp;
-	bool event = false;
-	int i, j = *argc;
+	int j = *argc;
 
-	const struct option event_options[] = {
-		OPT_BOOLEAN('e', "event", &event, NULL),
-		OPT_END()
-	};
-
-	tmp = calloc(j + 1, sizeof(char *));
-	if (!tmp)
+	if (!perf_pmus__have_event("trace_imc", "trace_cycles"))
 		return -EINVAL;
 
-	for (i = 0; i < j; i++)
-		tmp[i] = argv[i];
-
-	parse_options(j, tmp, event_options, NULL, PARSE_OPT_KEEP_UNKNOWN);
-	if (!event) {
-		if (perf_pmus__have_event("trace_imc", "trace_cycles")) {
-			argv[j++] = strdup("-e");
-			argv[j++] = strdup("trace_imc/trace_cycles/");
-			*argc += 2;
-		} else {
-			free(tmp);
-			return -EINVAL;
-		}
-	}
+	argv[j++] = strdup("-e");
+	argv[j++] = strdup("trace_imc/trace_cycles/");
+	*argc += 2;
 
-	free(tmp);
 	return 0;
 }
 
diff --git a/tools/perf/util/kvm-stat-arch/kvm-stat-x86.c b/tools/perf/util/kvm-stat-arch/kvm-stat-x86.c
index 788d216f0852147d..46f9a0adcb608aab 100644
--- a/tools/perf/util/kvm-stat-arch/kvm-stat-x86.c
+++ b/tools/perf/util/kvm-stat-arch/kvm-stat-x86.c
@@ -7,7 +7,6 @@
 #include "../../../arch/x86/include/uapi/asm/svm.h"
 #include "../../../arch/x86/include/uapi/asm/vmx.h"
 #include "../../../arch/x86/include/uapi/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,38 +210,15 @@ int __cpu_isa_init_x86(struct perf_kvm_stat *kvm, const char *cpuid)
  */
 int __kvm_add_default_arch_event_x86(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;
-	}
+	int ret = 0, j = *argc;
+
+	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/util/kvm-stat.c b/tools/perf/util/kvm-stat.c
index 755ab659a05c30f0..c571d2ed711a8345 100644
--- a/tools/perf/util/kvm-stat.c
+++ b/tools/perf/util/kvm-stat.c
@@ -1,8 +1,10 @@
 // SPDX-License-Identifier: GPL-2.0
 #include "debug.h"
+#include "env.h"
 #include "evsel.h"
 #include "kvm-stat.h"
 #include <dwarf-regs.h>
+#include <subcmd/parse-options.h>
 
 bool kvm_exit_event(struct evsel *evsel)
 {
@@ -268,3 +270,42 @@ int kvm_add_default_arch_event(uint16_t e_machine, int *argc, const char **argv)
 		return 0;
 	}
 }
+
+bool kvm_need_default_arch_event(uint16_t e_machine, int argc, const char **argv)
+{
+	const char **tmp_argv;
+	bool event = false;
+	int i;
+
+	const struct option event_options[] = {
+		OPT_BOOLEAN('e', "event", &event, NULL),
+		OPT_BOOLEAN(0, "pfm-events", &event, NULL),
+		OPT_END()
+	};
+
+	switch (e_machine) {
+	case EM_PPC:
+	case EM_PPC64:
+		break;
+	case EM_X86_64:
+	case EM_386:
+		if (!x86__is_intel_cpu())
+			return false;
+		break;
+	default:
+		return false;
+	}
+
+	/* parse_options() may change the argv, let's make a copy */
+	tmp_argv = calloc(argc + 1, sizeof(char *));
+	if (!tmp_argv)
+		return false;
+
+	for (i = 0; i < argc; i++)
+		tmp_argv[i] = argv[i];
+
+	parse_options(argc, tmp_argv, event_options, NULL, PARSE_OPT_KEEP_UNKNOWN);
+	free(tmp_argv);
+
+	return !event;
+}
diff --git a/tools/perf/util/kvm-stat.h b/tools/perf/util/kvm-stat.h
index cdbd921a555f4e51..04f9f3193555972f 100644
--- a/tools/perf/util/kvm-stat.h
+++ b/tools/perf/util/kvm-stat.h
@@ -174,12 +174,20 @@ const char * const *__kvm_skip_events_riscv(void);
 const char * const *__kvm_skip_events_s390(void);
 const char * const *__kvm_skip_events_x86(void);
 
+bool kvm_need_default_arch_event(uint16_t e_machine, int argc, const char **argv);
 int kvm_add_default_arch_event(uint16_t e_machine, int *argc, const char **argv);
 int __kvm_add_default_arch_event_powerpc(int *argc, const char **argv);
 int __kvm_add_default_arch_event_x86(int *argc, const char **argv);
 
 #else /* !HAVE_LIBTRACEEVENT */
 
+static inline bool kvm_need_default_arch_event(uint16_t e_machine __maybe_unused,
+					       int argc __maybe_unused,
+					       const char **argv __maybe_unused)
+{
+	return false;
+}
+
 static inline int kvm_add_default_arch_event(uint16_t e_machine __maybe_unused,
 					     int *argc __maybe_unused,
 					     const char **argv __maybe_unused)
-- 
2.55.0.rc0.799.gd6f94ed593-goog

Re: [PATCH v3 1/6] perf kvm: Factor out kvm_need_default_arch_event()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The file tools/perf/util/kvm-stat.c introduces usage of calloc and free but fails to explicitly include <stdlib.h>, violating POSIX/musl compatibility guidelines.
--

commit 706198153306324cdd16cca21dab43f87d56e722
Author: Namhyung Kim <namhyung@kernel.org>

perf kvm: Factor out kvm_need_default_arch_event()

This commit extracts the common logic for checking if an event command line
option exists into a new generic function kvm_need_default_arch_event().
This avoids duplicating the checking logic in each architecture's
kvm_add_default_arch_event() function.

> diff --git a/tools/perf/util/kvm-stat.c b/tools/perf/util/kvm-stat.c
> index 755ab659a05c3..c571d2ed711a8 100644
> --- a/tools/perf/util/kvm-stat.c
> +++ b/tools/perf/util/kvm-stat.c
> @@ -1,8 +1,10 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include "debug.h"
> +#include "env.h"
>  #include "evsel.h"
>  #include "kvm-stat.h"
>  #include <dwarf-regs.h>
> +#include <subcmd/parse-options.h>
>  
>  bool kvm_exit_event(struct evsel *evsel)
>  {

[ ... ]

> @@ -268,3 +270,42 @@ int kvm_add_default_arch_event(uint16_t e_machine, int *argc, const char **argv)
>  		return 0;
>  	}
>  }
> +
> +bool kvm_need_default_arch_event(uint16_t e_machine, int argc, const char **argv)
> +{
> +	const char **tmp_argv;
> +	bool event = false;
> +	int i;
> +
> +	const struct option event_options[] = {
> +		OPT_BOOLEAN('e', "event", &event, NULL),
> +		OPT_BOOLEAN(0, "pfm-events", &event, NULL),
> +		OPT_END()
> +	};
> +
> +	switch (e_machine) {
> +	case EM_PPC:
> +	case EM_PPC64:
> +		break;
> +	case EM_X86_64:
> +	case EM_386:
> +		if (!x86__is_intel_cpu())
> +			return false;
> +		break;
> +	default:
> +		return false;
> +	}
> +
> +	/* parse_options() may change the argv, let's make a copy */
> +	tmp_argv = calloc(argc + 1, sizeof(char *));

[Severity: Low]
Does this file need an explicit include of <stdlib.h>?

It looks like calloc() and free() are used here, but <stdlib.h> is not directly 
included in tools/perf/util/kvm-stat.c. Relying on implicit includes could cause 
build failures under musl libc where headers are strictly separated.

> +	if (!tmp_argv)
> +		return false;
> +
> +	for (i = 0; i < argc; i++)
> +		tmp_argv[i] = argv[i];
> +
> +	parse_options(argc, tmp_argv, event_options, NULL, PARSE_OPT_KEEP_UNKNOWN);
> +	free(tmp_argv);
> +
> +	return !event;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701194153.401218-1-namhyung@kernel.org?part=1

[PATCH v3 2/6] perf kvm: Check kvm_need_default_arch_event() early

[Namhyung Kim]

There's a subtle issue with option parsing in perf record.  It calls the
function with PARSE_OPT_STOP_AT_NON_OPTION so that it can pass later
options to the external command it runs.

But perf kvm record passes the default arch events after the argv.  So
if user calls it with command, then it passes the event to the external
command and fails it like below:

  $ sudo perf kvm --host record sleep 1
  sleep: invalid option -- 'e'
  Try 'sleep --help' for more information.
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.046 MB perf.data.kvm (5 samples) ]

We can check if the default options are needed before passing the extra
command line to make sure it's passed to perf record.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-kvm.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index 2f00cc1fd1c7cdbd..24576e4ebb02cade 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -1996,13 +1996,14 @@ static int __cmd_record(const char *file_name, int argc, const char **argv)
 {
 	int rec_argc, i = 0, j, ret;
 	const char **rec_argv;
+	int need_arch_event = !!kvm_need_default_arch_event(EM_HOST, argc, argv);
 
 	/*
 	 * Besides the 2 more options "-o" and "filename",
 	 * kvm_add_default_arch_event() may add 2 extra options,
-	 * so allocate 4 more items.
+	 * so allocate more items conditionally.
 	 */
-	rec_argc = argc + 2 + 2;
+	rec_argc = argc + 2 + (2 * need_arch_event);
 	rec_argv = calloc(rec_argc + 1, sizeof(char *));
 	if (!rec_argv)
 		return -ENOMEM;
@@ -2010,22 +2011,22 @@ static int __cmd_record(const char *file_name, int argc, const char **argv)
 	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] = STRDUP_FAIL_EXIT(argv[j]);
-
-	BUG_ON(i + 2 != rec_argc);
-
-	if (kvm_need_default_arch_event(EM_HOST, argc, argv)) {
+	if (need_arch_event) {
 		ret = kvm_add_default_arch_event(EM_HOST, &i, rec_argv);
 		if (ret)
 			goto EXIT;
 	}
 
+	for (j = 1; j < argc; j++, i++)
+		rec_argv[i] = STRDUP_FAIL_EXIT(argv[j]);
+
+	BUG_ON(i != rec_argc);
+
 	ret = cmd_record(i, rec_argv);
 
 EXIT:
-	for (i = 0; i < rec_argc; i++)
-		free((void *)rec_argv[i]);
+	for (j = 0; j < i; j++)
+		free((void *)rec_argv[j]);
 	free(rec_argv);
 	return ret;
 }
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH v3 3/6] perf kvm: Kill STRDUP_FAIL_EXIT()

[Namhyung Kim]

It's used to pass command line options to a copied argv.  But there's no
reason to make the copies as it's all used in the same function.  It can
simply use stack variables.

In fact, it fixes a subtle double free issue.  As parse_options() can
move contents in argv[], some entries may point to the same item.  So
freeing all items in the argv could trigger a double free.  With stack
variables, we don't need to allocate and free them.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-kvm.c                      | 54 +++++++------------
 .../util/kvm-stat-arch/kvm-stat-powerpc.c     |  4 +-
 tools/perf/util/kvm-stat-arch/kvm-stat-x86.c  |  9 ++--
 tools/perf/util/kvm-stat.h                    | 10 ----
 4 files changed, 24 insertions(+), 53 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index 24576e4ebb02cade..eaf9297cc34d32c1 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -1682,18 +1682,18 @@ kvm_events_record(struct perf_kvm_stat *kvm, int argc, const char **argv)
 		return -ENOMEM;
 
 	for (i = 0; i < ARRAY_SIZE(record_args); i++)
-		rec_argv[i] = STRDUP_FAIL_EXIT(record_args[i]);
+		rec_argv[i] = record_args[i];
 
 	for (j = 0; j < events_tp_size; j++) {
-		rec_argv[i++] = STRDUP_FAIL_EXIT("-e");
-		rec_argv[i++] = STRDUP_FAIL_EXIT(kvm_events_tp(e_machine)[j]);
+		rec_argv[i++] = "-e";
+		rec_argv[i++] = kvm_events_tp(e_machine)[j];
 	}
 
-	rec_argv[i++] = STRDUP_FAIL_EXIT("-o");
-	rec_argv[i++] = STRDUP_FAIL_EXIT(kvm->file_name);
+	rec_argv[i++] = "-o";
+	rec_argv[i++] = kvm->file_name;
 
 	for (j = 1; j < (unsigned int)argc; j++, i++)
-		rec_argv[i] = STRDUP_FAIL_EXIT(argv[j]);
+		rec_argv[i] = argv[j];
 
 	set_option_flag(record_options, 'e', "event", PARSE_OPT_HIDDEN);
 	set_option_flag(record_options, 0, "filter", PARSE_OPT_HIDDEN);
@@ -1717,10 +1717,6 @@ kvm_events_record(struct perf_kvm_stat *kvm, int argc, const char **argv)
 
 	record_usage = kvm_stat_record_usage;
 	ret = cmd_record(i, rec_argv);
-
-EXIT:
-	for (i = 0; i < rec_argc; i++)
-		free((void *)rec_argv[i]);
 	free(rec_argv);
 	return ret;
 }
@@ -2008,9 +2004,9 @@ static int __cmd_record(const char *file_name, int argc, const char **argv)
 	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);
+	rec_argv[i++] = "record";
+	rec_argv[i++] = "-o";
+	rec_argv[i++] = file_name;
 	if (need_arch_event) {
 		ret = kvm_add_default_arch_event(EM_HOST, &i, rec_argv);
 		if (ret)
@@ -2018,15 +2014,13 @@ static int __cmd_record(const char *file_name, int argc, const char **argv)
 	}
 
 	for (j = 1; j < argc; j++, i++)
-		rec_argv[i] = STRDUP_FAIL_EXIT(argv[j]);
+		rec_argv[i] = argv[j];
 
 	BUG_ON(i != rec_argc);
 
 	ret = cmd_record(i, rec_argv);
 
 EXIT:
-	for (j = 0; j < i; j++)
-		free((void *)rec_argv[j]);
 	free(rec_argv);
 	return ret;
 }
@@ -2041,19 +2035,15 @@ static int __cmd_report(const char *file_name, int argc, const char **argv)
 	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);
+	rec_argv[i++] = "report";
+	rec_argv[i++] = "-i";
+	rec_argv[i++] = file_name;
 	for (j = 1; j < argc; j++, i++)
-		rec_argv[i] = STRDUP_FAIL_EXIT(argv[j]);
+		rec_argv[i] = argv[j];
 
 	BUG_ON(i != rec_argc);
 
 	ret = cmd_report(i, rec_argv);
-
-EXIT:
-	for (i = 0; i < rec_argc; i++)
-		free((void *)rec_argv[i]);
 	free(rec_argv);
 	return ret;
 }
@@ -2069,19 +2059,15 @@ __cmd_buildid_list(const char *file_name, int argc, const char **argv)
 	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);
+	rec_argv[i++] = "buildid-list";
+	rec_argv[i++] = "-i";
+	rec_argv[i++] = file_name;
 	for (j = 1; j < argc; j++, i++)
-		rec_argv[i] = STRDUP_FAIL_EXIT(argv[j]);
+		rec_argv[i] = argv[j];
 
 	BUG_ON(i != rec_argc);
 
 	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;
 }
@@ -2101,7 +2087,7 @@ static int __cmd_top(int argc, const char **argv)
 		return -ENOMEM;
 
 	for (i = 0; i < argc; i++)
-		rec_argv[i] = STRDUP_FAIL_EXIT(argv[i]);
+		rec_argv[i] = argv[i];
 
 	BUG_ON(i != argc);
 
@@ -2114,8 +2100,6 @@ static int __cmd_top(int argc, const char **argv)
 	ret = cmd_top(i, rec_argv);
 
 EXIT:
-	for (i = 0; i < rec_argc; i++)
-		free((void *)rec_argv[i]);
 	free(rec_argv);
 	return ret;
 }
diff --git a/tools/perf/util/kvm-stat-arch/kvm-stat-powerpc.c b/tools/perf/util/kvm-stat-arch/kvm-stat-powerpc.c
index 8d4133c35c12f14b..315b24f9a59297dc 100644
--- a/tools/perf/util/kvm-stat-arch/kvm-stat-powerpc.c
+++ b/tools/perf/util/kvm-stat-arch/kvm-stat-powerpc.c
@@ -181,8 +181,8 @@ int __kvm_add_default_arch_event_powerpc(int *argc, const char **argv)
 	if (!perf_pmus__have_event("trace_imc", "trace_cycles"))
 		return -EINVAL;
 
-	argv[j++] = strdup("-e");
-	argv[j++] = strdup("trace_imc/trace_cycles/");
+	argv[j++] = "-e";
+	argv[j++] = "trace_imc/trace_cycles/";
 	*argc += 2;
 
 	return 0;
diff --git a/tools/perf/util/kvm-stat-arch/kvm-stat-x86.c b/tools/perf/util/kvm-stat-arch/kvm-stat-x86.c
index 46f9a0adcb608aab..97041f5100fc812f 100644
--- a/tools/perf/util/kvm-stat-arch/kvm-stat-x86.c
+++ b/tools/perf/util/kvm-stat-arch/kvm-stat-x86.c
@@ -210,16 +210,13 @@ int __cpu_isa_init_x86(struct perf_kvm_stat *kvm, const char *cpuid)
  */
 int __kvm_add_default_arch_event_x86(int *argc, const char **argv)
 {
-	int ret = 0, j = *argc;
+	int j = *argc;
 
-	argv[j++] = STRDUP_FAIL_EXIT("-e");
-	argv[j++] = STRDUP_FAIL_EXIT("cycles");
+	argv[j++] = "-e";
+	argv[j++] = "cycles";
 	*argc += 2;
 
 	return 0;
-
-EXIT:
-	return ret;
 }
 
 const char * const *__kvm_events_tp_x86(void)
diff --git a/tools/perf/util/kvm-stat.h b/tools/perf/util/kvm-stat.h
index 04f9f3193555972f..1db31c0751875875 100644
--- a/tools/perf/util/kvm-stat.h
+++ b/tools/perf/util/kvm-stat.h
@@ -229,14 +229,4 @@ static inline struct kvm_info *kvm_info__new(void)
 	return ki;
 }
 
-#define STRDUP_FAIL_EXIT(s)		\
-	({	char *_p;		\
-		_p = strdup(s);		\
-		if (!_p) {		\
-			ret = -ENOMEM;	\
-			goto EXIT;	\
-		}			\
-		_p;			\
-	})
-
 #endif /* __PERF_KVM_STAT_H */
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH v3 4/6] perf kvm: Do not copy filename string

[Namhyung Kim]

As it removed STRDUP_FAIL_EXIT(), it no longer calls free() for elements
in the copied argv.  Thus, the filename should not be allocated as well.
In fact, it's a pointer to string literals and should be fine to pass
the pointer.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-kvm.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index eaf9297cc34d32c1..20ea59e59a4b65ba 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -607,11 +607,11 @@ static const char *get_filename_for_perf_kvm(void)
 	const char *filename;
 
 	if (perf_host && !perf_guest)
-		filename = strdup("perf.data.host");
+		filename = "perf.data.host";
 	else if (!perf_host && perf_guest)
-		filename = strdup("perf.data.guest");
+		filename = "perf.data.guest";
 	else
-		filename = strdup("perf.data.kvm");
+		filename = "perf.data.kvm";
 
 	return filename;
 }
@@ -2148,15 +2148,9 @@ int cmd_kvm(int argc, const char **argv)
 	if (!perf_host)
 		perf_guest = 1;
 
-	if (!file_name) {
+	if (!file_name)
 		file_name = get_filename_for_perf_kvm();
 
-		if (!file_name) {
-			pr_err("Failed to allocate memory for filename\n");
-			return -ENOMEM;
-		}
-	}
-
 	if (strlen(argv[0]) > 2 && strstarts("record", argv[0]))
 		return __cmd_record(file_name, argc, argv);
 	else if (strlen(argv[0]) > 2 && strstarts("report", argv[0]))
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH v3 5/6] perf kvm: Fix a memory leak in the usage string

[Namhyung Kim]

The first element of the kvm_usage string is allocated dynamically by
parse_options_subcommand() if it's NULL.  Callers should free it after
use.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-kvm.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index 20ea59e59a4b65ba..00baafb056032970 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -2135,6 +2135,7 @@ int cmd_kvm(int argc, const char **argv)
 	const char *const kvm_subcommands[] = { "top", "record", "report", "diff",
 						"buildid-list", "stat", NULL };
 	const char *kvm_usage[] = { NULL, NULL };
+	int ret;
 
 	exclude_GH_default = true;
 	perf_host  = 0;
@@ -2152,18 +2153,18 @@ int cmd_kvm(int argc, const char **argv)
 		file_name = get_filename_for_perf_kvm();
 
 	if (strlen(argv[0]) > 2 && strstarts("record", argv[0]))
-		return __cmd_record(file_name, argc, argv);
+		ret = __cmd_record(file_name, argc, argv);
 	else if (strlen(argv[0]) > 2 && strstarts("report", argv[0]))
-		return __cmd_report(file_name, argc, argv);
+		ret = __cmd_report(file_name, argc, argv);
 	else if (strlen(argv[0]) > 2 && strstarts("diff", argv[0]))
-		return cmd_diff(argc, argv);
+		ret = cmd_diff(argc, argv);
 	else if (!strcmp(argv[0], "top"))
-		return __cmd_top(argc, argv);
+		ret = __cmd_top(argc, argv);
 	else if (strlen(argv[0]) > 2 && strstarts("buildid-list", argv[0]))
-		return __cmd_buildid_list(file_name, argc, argv);
+		ret = __cmd_buildid_list(file_name, argc, argv);
 #if defined(HAVE_LIBTRACEEVENT)
 	else if (strlen(argv[0]) > 2 && strstarts("stat", argv[0]))
-		return kvm_cmd_stat(file_name, argc, argv);
+		ret = kvm_cmd_stat(file_name, argc, argv);
 #endif
 	else
 		usage_with_options(kvm_usage, kvm_options);
@@ -2171,5 +2172,5 @@ int cmd_kvm(int argc, const char **argv)
 	/* free usage string allocated by parse_options_subcommand */
 	free((void *)kvm_usage[0]);
 
-	return 0;
+	return ret;
 }
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH v3 6/6] perf test: Extend perf kvm tests to check default event

[Namhyung Kim]

It can now pass command line arguments to perf kvm record.  Let's pass
'sleep 1' directly and see it doesn't fail.

  $ sudo perf test -vv kvm
   96: perf kvm tests:
  ---- start ----
  test child forked, pid 3686726
  Starting qemu-system-x86_64...
  Testing perf kvm stat
  Recording kvm events for pid 3686746 (duration 1s)...
  perf kvm stat test [Success]
  Testing perf kvm record/report
  Recording kvm profile for pid 3686746 (duration 1s)...
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.576 MB /tmp/__perf_kvm_test.perf.data.0HgX6 (4009 samples) ]
  perf kvm record/report test [Success]
  Testing perf kvm buildid-list
  perf kvm buildid-list test [Success]
  Testing perf kvm stat live
  perf kvm stat live test [Success]
  Testing perf kvm record default event with command line
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.000 MB /dev/null ]
  perf kvm record default event [Success]
  ---- end(0) ----
   96: perf kvm tests                                               : Ok

  === Test Summary ===
  Passed main tests : 1
  Passed subtests   : 0
  Skipped tests     : 0
  Failed tests      : 0

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/shell/kvm.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tools/perf/tests/shell/kvm.sh b/tools/perf/tests/shell/kvm.sh
index a5396f8e6fe5ca82..7b26368051ffdbad 100755
--- a/tools/perf/tests/shell/kvm.sh
+++ b/tools/perf/tests/shell/kvm.sh
@@ -138,6 +138,15 @@ test_kvm_stat_live() {
 	echo "perf kvm stat live test [Success]"
 }
 
+test_kvm_default_event() {
+	echo "Testing perf kvm record default event with command line"
+
+	# Check if kvm record with default events handle command line arguments
+	perf kvm record -p "${qemu_pid}" -o /dev/null sleep 1
+
+	echo "perf kvm record default event [Success]"
+}
+
 setup_qemu() {
 	# Find qemu
 	if [ "$(uname -m)" = "x86_64" ]; then
@@ -191,6 +200,7 @@ if [ $err -eq 0 ]; then
 	test_kvm_record_report
 	test_kvm_buildid_list
 	test_kvm_stat_live
+	test_kvm_default_event
 fi
 
 cleanup
-- 
2.55.0.rc0.799.gd6f94ed593-goog

Re: [PATCH v3 6/6] perf test: Extend perf kvm tests to check default event

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Missing explicit error handling for the `perf kvm record` command in `test_kvm_default_event` causes an ungraceful script abort on failure.
--

commit 0b371c3387f66fb432d87052c8a94cad8f1c734c
Author: Namhyung Kim <namhyung@kernel.org>

perf test: Extend perf kvm tests to check default event

This commit extends the perf kvm tests by adding a check to verify that
perf kvm record can successfully handle command line arguments like
'sleep 1'.

> diff --git a/tools/perf/tests/shell/kvm.sh b/tools/perf/tests/shell/kvm.sh
> index f88e859025c42..ad208abce0a99 100755
> --- a/tools/perf/tests/shell/kvm.sh
> +++ b/tools/perf/tests/shell/kvm.sh
[ ... ]
> @@ -123,6 +123,15 @@ test_kvm_stat_live() {
>  	echo "perf kvm stat live test [Success]"
>  }
>  
> +test_kvm_default_event() {
> +	echo "Testing perf kvm record default event with command line"
> +
> +	# Check if kvm record with default events handle command line arguments
> +	perf kvm record -p "${qemu_pid}" -o /dev/null sleep 1

[Severity: Low]
Since this script runs with set -e enabled globally, could a failure in this
perf command cause an immediate script abort rather than a graceful test
failure?

Other tests in this file appear to use a conditional error check pattern
(if ! ...; then err=1; return; fi) to catch failures and report them properly
without triggering the EXIT trap prematurely. Would it be better to wrap this
command with similar error handling?

> +
> +	echo "perf kvm record default event [Success]"
> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701194153.401218-1-namhyung@kernel.org?part=6

Re: [PATCH v3 0/6] perf kvm: A small update in the default arch event

[Ian Rogers]

On Wed, Jul 1, 2026 at 12:41 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hello,
>
> Ian said there's an issue in the perf kvm record command line option parsing.
> This is my attempt to address the issue, simplify the code and fix memory leaks.
>
> v3 changes)
>  * remove unnecessary error check  (Sashiko)
>  * update test code after rebase
>
> v2 changes)
>  * handle kvm top subcommand  (Sashiko)
>  * handle other arch/condition for the default events  (Sashiko)
>  * add <stdlib.h> for musl compatibility  (Sashiko)
>  * fix some memory leaks
>
> Thanks,
> Namhyung

I built with -fsanitize=address and ran the kvm test. Everything passing.

Tested-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> Namhyung Kim (6):
>   perf kvm: Factor out kvm_need_default_arch_event()
>   perf kvm: Check kvm_need_default_arch_event() early
>   perf kvm: Kill STRDUP_FAIL_EXIT()
>   perf kvm: Do not copy filename string
>   perf kvm: Fix a memory leak in the usage string
>   perf test: Extend perf kvm tests to check default event
>
>  tools/perf/builtin-kvm.c                      | 109 ++++++++----------
>  tools/perf/tests/shell/kvm.sh                 |  10 ++
>  .../util/kvm-stat-arch/kvm-stat-powerpc.c     |  31 +----
>  tools/perf/util/kvm-stat-arch/kvm-stat-x86.c  |  37 +-----
>  tools/perf/util/kvm-stat.c                    |  41 +++++++
>  tools/perf/util/kvm-stat.h                    |  18 ++-
>  6 files changed, 116 insertions(+), 130 deletions(-)
>
> --
> 2.55.0.rc0.799.gd6f94ed593-goog
>