[PATCH 0/5] Perf kvm commands bug fix

[PATCH 0/5] 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.

The commit 634d36f82517 ("perf record: Just use "cycles:P" as the
 default event") changes to use PEBS event to do sampling by default
including guest sampling. This breaks host to sample guest with commands
"perf kvm record/top" 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 4-5/5 fix this issue by
using "cycles" event instead of PEBS event "cycles:P" to sample guest on
Intel platforms.

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 (5):
  perf tools kvm: 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 | 46 ++++++++++++++++++
 tools/perf/builtin-kvm.c            | 73 ++++++++++++++++++++++++-----
 tools/perf/util/env.c               | 22 +++++++++
 tools/perf/util/env.h               |  2 +
 4 files changed, 131 insertions(+), 12 deletions(-)


base-commit: 6235ce77749f45cac27f630337e2fdf04e8a6c73
-- 
2.34.1

[PATCH 1/5] 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>
---
 tools/perf/builtin-kvm.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index 7b15b4a705e4..f78a67a199ff 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -1719,7 +1719,9 @@ 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);
+	free(rec_argv);
+	return ret;
 }
 
 static int
@@ -2006,6 +2008,9 @@ 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 *));
+	if (!rec_argv)
+		return -ENOMEM;
+
 	rec_argv[i++] = strdup("record");
 	rec_argv[i++] = strdup("-o");
 	rec_argv[i++] = strdup(file_name);
@@ -2014,16 +2019,21 @@ static int __cmd_record(const char *file_name, int argc, const char **argv)
 
 	BUG_ON(i != rec_argc);
 
-	return cmd_record(i, rec_argv);
+	ret = cmd_record(i, rec_argv);
+	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 *));
+	if (!rec_argv)
+		return -ENOMEM;
+
 	rec_argv[i++] = strdup("report");
 	rec_argv[i++] = strdup("-i");
 	rec_argv[i++] = strdup(file_name);
@@ -2032,17 +2042,22 @@ static int __cmd_report(const char *file_name, int argc, const char **argv)
 
 	BUG_ON(i != rec_argc);
 
-	return cmd_report(i, rec_argv);
+	ret = cmd_report(i, rec_argv);
+	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 *));
+	if (!rec_argv)
+		return -ENOMEM;
+
 	rec_argv[i++] = strdup("buildid-list");
 	rec_argv[i++] = strdup("-i");
 	rec_argv[i++] = strdup(file_name);
@@ -2051,7 +2066,9 @@ __cmd_buildid_list(const char *file_name, int argc, const char **argv)
 
 	BUG_ON(i != rec_argc);
 
-	return cmd_buildid_list(i, rec_argv);
+	ret = cmd_buildid_list(i, rec_argv);
+	free(rec_argv);
+	return ret;
 }
 
 int cmd_kvm(int argc, const char **argv)
-- 
2.34.1

[PATCH 2/5] 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>
---
 tools/perf/builtin-kvm.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index f78a67a199ff..7e48d2437710 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -2002,12 +2002,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;
-	rec_argv = calloc(rec_argc + 1, sizeof(char *));
+	/*
+	 * kvm_add_default_arch_event() may add 2 extra options, so
+	 * allocate 2 more pointers in adavance.
+	 */
+	rec_argv = calloc(rec_argc + 2 + 1, sizeof(char *));
 	if (!rec_argv)
 		return -ENOMEM;
 
@@ -2019,6 +2019,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)
+		return -EINVAL;
+
 	ret = cmd_record(i, rec_argv);
 	free(rec_argv);
 	return ret;
-- 
2.34.1

[PATCH 3/5] 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>
---
 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 4/5] 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: 634d36f82517 ("perf record: Just use "cycles:P" as the default event")
Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---
 tools/perf/arch/x86/util/kvm-stat.c | 46 +++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/tools/perf/arch/x86/util/kvm-stat.c b/tools/perf/arch/x86/util/kvm-stat.c
index 424716518b75..cdb5f3e1b5be 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,47 @@ 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 i, j = *argc;
+
+	const struct option event_options[] = {
+		OPT_BOOLEAN('e', "event", &event, NULL),
+		OPT_END()
+	};
+
+	if (!x86__is_intel_cpu())
+		return 0;
+
+	tmp = calloc(j + 1, sizeof(char *));
+	if (!tmp)
+		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) {
+		argv[j++] = strdup("-e");
+		argv[j++] = strdup("cycles");
+		*argc += 2;
+	}
+
+	free(tmp);
+	return 0;
+}
-- 
2.34.1

[PATCH 5/5] 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: 634d36f82517 ("perf record: Just use "cycles:P" as the default event")
Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---
 tools/perf/builtin-kvm.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index 7e48d2437710..d72b40f3df12 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -2075,6 +2075,34 @@ __cmd_buildid_list(const char *file_name, int argc, const char **argv)
 	return ret;
 }
 
+static int __cmd_top(int argc, const char **argv)
+{
+	int 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_argv = calloc(argc + 2 + 1, sizeof(char *));
+	if (!rec_argv)
+		return -ENOMEM;
+
+	for (i = 0; i < argc; i++)
+		rec_argv[i] = argv[i];
+
+	BUG_ON(i != argc);
+
+	ret = kvm_add_default_arch_event(&i, rec_argv);
+	if (ret)
+		return -EINVAL;
+
+	ret = cmd_top(i, rec_argv);
+	free(rec_argv);
+
+	return ret;
+}
+
 int cmd_kvm(int argc, const char **argv)
 {
 	const char *file_name = NULL;
@@ -2135,7 +2163,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 5/5] perf tools kvm: Use "cycles" to sample guest for "kvm top" on Intel

[Mi, Dapeng]

On 8/5/2025 8:46 AM, Dapeng Mi wrote:
> 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: 634d36f82517 ("perf record: Just use "cycles:P" as the default event")
> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
> ---
>  tools/perf/builtin-kvm.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> index 7e48d2437710..d72b40f3df12 100644
> --- a/tools/perf/builtin-kvm.c
> +++ b/tools/perf/builtin-kvm.c
> @@ -2075,6 +2075,34 @@ __cmd_buildid_list(const char *file_name, int argc, const char **argv)
>  	return ret;
>  }
>  
> +static int __cmd_top(int argc, const char **argv)
> +{
> +	int 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_argv = calloc(argc + 2 + 1, sizeof(char *));
> +	if (!rec_argv)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < argc; i++)
> +		rec_argv[i] = argv[i];
> +
> +	BUG_ON(i != argc);
> +
> +	ret = kvm_add_default_arch_event(&i, rec_argv);
> +	if (ret)
> +		return -EINVAL;
> +
> +	ret = cmd_top(i, rec_argv);
> +	free(rec_argv);
> +
> +	return ret;
> +}
> +
>  int cmd_kvm(int argc, const char **argv)
>  {
>  	const char *file_name = NULL;
> @@ -2135,7 +2163,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)

This patch would impact powerpc platform as well. Base on the comments
before kvm_add_default_arch_event() in
tools/perf/arch/powerpc/util/kvm-stat.c, I suppose powerpc also needs this
change, otherwise "perf kvm top" command can't sample guest records. But I
have no any powerpc on my hand, so it's not tested on powerpc platform. Any
test on powerpc is appreciated. Thanks.

Re: [PATCH 5/5] perf tools kvm: Use "cycles" to sample guest for "kvm top" on Intel

[Aditya Bodkhe]

On 05/08/25 6:27 am, Mi, Dapeng wrote:
> On 8/5/2025 8:46 AM, Dapeng Mi wrote:
>> 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: 634d36f82517 ("perf record: Just use "cycles:P" as the default event")
>> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
>> ---
>>   tools/perf/builtin-kvm.c | 30 +++++++++++++++++++++++++++++-
>>   1 file changed, 29 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
>> index 7e48d2437710..d72b40f3df12 100644
>> --- a/tools/perf/builtin-kvm.c
>> +++ b/tools/perf/builtin-kvm.c
>> @@ -2075,6 +2075,34 @@ __cmd_buildid_list(const char *file_name, int argc, const char **argv)
>>   	return ret;
>>   }
>>   
>> +static int __cmd_top(int argc, const char **argv)
>> +{
>> +	int 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_argv = calloc(argc + 2 + 1, sizeof(char *));
>> +	if (!rec_argv)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < argc; i++)
>> +		rec_argv[i] = argv[i];
>> +
>> +	BUG_ON(i != argc);
>> +
>> +	ret = kvm_add_default_arch_event(&i, rec_argv);
>> +	if (ret)
>> +		return -EINVAL;
>> +
>> +	ret = cmd_top(i, rec_argv);
>> +	free(rec_argv);
>> +
>> +	return ret;
>> +}
>> +
>>   int cmd_kvm(int argc, const char **argv)
>>   {
>>   	const char *file_name = NULL;
>> @@ -2135,7 +2163,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)
> This patch would impact powerpc platform as well. Base on the comments
> before kvm_add_default_arch_event() in
> tools/perf/arch/powerpc/util/kvm-stat.c, I suppose powerpc also needs this
> change, otherwise "perf kvm top" command can't sample guest records. But I
> have no any powerpc on my hand, so it's not tested on powerpc platform. Any
> test on powerpc is appreciated. Thanks.
I have powerpc systems . I will test can share the results .
>
>

Re: [PATCH 5/5] perf tools kvm: Use "cycles" to sample guest for "kvm top" on Intel

[Mi, Dapeng]

On 8/5/2025 7:32 PM, Aditya Bodkhe wrote:
> On 05/08/25 6:27 am, Mi, Dapeng wrote:
>> On 8/5/2025 8:46 AM, Dapeng Mi wrote:
>>> 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: 634d36f82517 ("perf record: Just use "cycles:P" as the default event")
>>> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
>>> ---
>>>   tools/perf/builtin-kvm.c | 30 +++++++++++++++++++++++++++++-
>>>   1 file changed, 29 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
>>> index 7e48d2437710..d72b40f3df12 100644
>>> --- a/tools/perf/builtin-kvm.c
>>> +++ b/tools/perf/builtin-kvm.c
>>> @@ -2075,6 +2075,34 @@ __cmd_buildid_list(const char *file_name, int argc, const char **argv)
>>>   	return ret;
>>>   }
>>>   
>>> +static int __cmd_top(int argc, const char **argv)
>>> +{
>>> +	int 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_argv = calloc(argc + 2 + 1, sizeof(char *));
>>> +	if (!rec_argv)
>>> +		return -ENOMEM;
>>> +
>>> +	for (i = 0; i < argc; i++)
>>> +		rec_argv[i] = argv[i];
>>> +
>>> +	BUG_ON(i != argc);
>>> +
>>> +	ret = kvm_add_default_arch_event(&i, rec_argv);
>>> +	if (ret)
>>> +		return -EINVAL;
>>> +
>>> +	ret = cmd_top(i, rec_argv);
>>> +	free(rec_argv);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>>   int cmd_kvm(int argc, const char **argv)
>>>   {
>>>   	const char *file_name = NULL;
>>> @@ -2135,7 +2163,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)
>> This patch would impact powerpc platform as well. Base on the comments
>> before kvm_add_default_arch_event() in
>> tools/perf/arch/powerpc/util/kvm-stat.c, I suppose powerpc also needs this
>> change, otherwise "perf kvm top" command can't sample guest records. But I
>> have no any powerpc on my hand, so it's not tested on powerpc platform. Any
>> test on powerpc is appreciated. Thanks.
> I have powerpc systems . I will test can share the results .

Thanks.


>>

Re: [PATCH 1/5] perf tools kvm: Add missed memory allocation check and free

[Namhyung Kim]

On Tue, Aug 05, 2025 at 08:46:29AM +0800, Dapeng Mi wrote:
> 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>
> ---
>  tools/perf/builtin-kvm.c | 29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> index 7b15b4a705e4..f78a67a199ff 100644
> --- a/tools/perf/builtin-kvm.c
> +++ b/tools/perf/builtin-kvm.c
> @@ -1719,7 +1719,9 @@ 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);
> +	free(rec_argv);

Well.. it's not enough just to free rec_argv.  You also need to free all
items in the rec_argv[].  Probably you want to add more STRDUP_FAIL_EXIT
when copying the original argv (here and other places).

Thanks,
Namhyung


> +	return ret;
>  }
>  
>  static int
> @@ -2006,6 +2008,9 @@ 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 *));
> +	if (!rec_argv)
> +		return -ENOMEM;
> +
>  	rec_argv[i++] = strdup("record");
>  	rec_argv[i++] = strdup("-o");
>  	rec_argv[i++] = strdup(file_name);
> @@ -2014,16 +2019,21 @@ static int __cmd_record(const char *file_name, int argc, const char **argv)
>  
>  	BUG_ON(i != rec_argc);
>  
> -	return cmd_record(i, rec_argv);
> +	ret = cmd_record(i, rec_argv);
> +	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 *));
> +	if (!rec_argv)
> +		return -ENOMEM;
> +
>  	rec_argv[i++] = strdup("report");
>  	rec_argv[i++] = strdup("-i");
>  	rec_argv[i++] = strdup(file_name);
> @@ -2032,17 +2042,22 @@ static int __cmd_report(const char *file_name, int argc, const char **argv)
>  
>  	BUG_ON(i != rec_argc);
>  
> -	return cmd_report(i, rec_argv);
> +	ret = cmd_report(i, rec_argv);
> +	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 *));
> +	if (!rec_argv)
> +		return -ENOMEM;
> +
>  	rec_argv[i++] = strdup("buildid-list");
>  	rec_argv[i++] = strdup("-i");
>  	rec_argv[i++] = strdup(file_name);
> @@ -2051,7 +2066,9 @@ __cmd_buildid_list(const char *file_name, int argc, const char **argv)
>  
>  	BUG_ON(i != rec_argc);
>  
> -	return cmd_buildid_list(i, rec_argv);
> +	ret = cmd_buildid_list(i, rec_argv);
> +	free(rec_argv);
> +	return ret;
>  }
>  
>  int cmd_kvm(int argc, const char **argv)
> -- 
> 2.34.1
> 

Re: [PATCH 2/5] perf tools kvm: Fix the potential out of range memory access issue

[Namhyung Kim]

On Tue, Aug 05, 2025 at 08:46:30AM +0800, Dapeng Mi wrote:
> 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>
> ---
>  tools/perf/builtin-kvm.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> index f78a67a199ff..7e48d2437710 100644
> --- a/tools/perf/builtin-kvm.c
> +++ b/tools/perf/builtin-kvm.c
> @@ -2002,12 +2002,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;

While at it, can we add a comment for this too?  I believe it's for "-o"
and filename.

Thanks,
Namhyung


> -	rec_argv = calloc(rec_argc + 1, sizeof(char *));
> +	/*
> +	 * kvm_add_default_arch_event() may add 2 extra options, so
> +	 * allocate 2 more pointers in adavance.
> +	 */
> +	rec_argv = calloc(rec_argc + 2 + 1, sizeof(char *));
>  	if (!rec_argv)
>  		return -ENOMEM;
>  
> @@ -2019,6 +2019,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)
> +		return -EINVAL;
> +
>  	ret = cmd_record(i, rec_argv);
>  	free(rec_argv);
>  	return ret;
> -- 
> 2.34.1
> 

Re: [PATCH 4/5] perf tools kvm: Use "cycles" to sample guest for "kvm record" on Intel

[Namhyung Kim]

On Tue, Aug 05, 2025 at 08:46:32AM +0800, Dapeng Mi wrote:
> 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.

Do you mean we cannot use "cycles:PG" for perf kvm record?

> 
> 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) ]

What if user already gave some events in the command line?  I think you
need to check if "-e" or "--event" (and "--pfm-events" too) is in the
argv[] before adding these.

Thanks,
Namhyung

> 
> Reported-by: Kevin Tian <kevin.tian@intel.com>
> Fixes: 634d36f82517 ("perf record: Just use "cycles:P" as the default event")
> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
> ---
>  tools/perf/arch/x86/util/kvm-stat.c | 46 +++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/tools/perf/arch/x86/util/kvm-stat.c b/tools/perf/arch/x86/util/kvm-stat.c
> index 424716518b75..cdb5f3e1b5be 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,47 @@ 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 i, j = *argc;
> +
> +	const struct option event_options[] = {
> +		OPT_BOOLEAN('e', "event", &event, NULL),
> +		OPT_END()
> +	};
> +
> +	if (!x86__is_intel_cpu())
> +		return 0;
> +
> +	tmp = calloc(j + 1, sizeof(char *));
> +	if (!tmp)
> +		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) {
> +		argv[j++] = strdup("-e");
> +		argv[j++] = strdup("cycles");
> +		*argc += 2;
> +	}
> +
> +	free(tmp);
> +	return 0;
> +}
> -- 
> 2.34.1
> 

Re: [PATCH 1/5] perf tools kvm: Add missed memory allocation check and free

[Mi, Dapeng]

On 8/7/2025 7:40 AM, Namhyung Kim wrote:
> On Tue, Aug 05, 2025 at 08:46:29AM +0800, Dapeng Mi wrote:
>> 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>
>> ---
>>  tools/perf/builtin-kvm.c | 29 +++++++++++++++++++++++------
>>  1 file changed, 23 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
>> index 7b15b4a705e4..f78a67a199ff 100644
>> --- a/tools/perf/builtin-kvm.c
>> +++ b/tools/perf/builtin-kvm.c
>> @@ -1719,7 +1719,9 @@ 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);
>> +	free(rec_argv);
> Well.. it's not enough just to free rec_argv.  You also need to free all
> items in the rec_argv[].  Probably you want to add more STRDUP_FAIL_EXIT
> when copying the original argv (here and other places).

Oh, yes. Would do. Thanks. 


>
> Thanks,
> Namhyung
>
>
>> +	return ret;
>>  }
>>  
>>  static int
>> @@ -2006,6 +2008,9 @@ 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 *));
>> +	if (!rec_argv)
>> +		return -ENOMEM;
>> +
>>  	rec_argv[i++] = strdup("record");
>>  	rec_argv[i++] = strdup("-o");
>>  	rec_argv[i++] = strdup(file_name);
>> @@ -2014,16 +2019,21 @@ static int __cmd_record(const char *file_name, int argc, const char **argv)
>>  
>>  	BUG_ON(i != rec_argc);
>>  
>> -	return cmd_record(i, rec_argv);
>> +	ret = cmd_record(i, rec_argv);
>> +	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 *));
>> +	if (!rec_argv)
>> +		return -ENOMEM;
>> +
>>  	rec_argv[i++] = strdup("report");
>>  	rec_argv[i++] = strdup("-i");
>>  	rec_argv[i++] = strdup(file_name);
>> @@ -2032,17 +2042,22 @@ static int __cmd_report(const char *file_name, int argc, const char **argv)
>>  
>>  	BUG_ON(i != rec_argc);
>>  
>> -	return cmd_report(i, rec_argv);
>> +	ret = cmd_report(i, rec_argv);
>> +	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 *));
>> +	if (!rec_argv)
>> +		return -ENOMEM;
>> +
>>  	rec_argv[i++] = strdup("buildid-list");
>>  	rec_argv[i++] = strdup("-i");
>>  	rec_argv[i++] = strdup(file_name);
>> @@ -2051,7 +2066,9 @@ __cmd_buildid_list(const char *file_name, int argc, const char **argv)
>>  
>>  	BUG_ON(i != rec_argc);
>>  
>> -	return cmd_buildid_list(i, rec_argv);
>> +	ret = cmd_buildid_list(i, rec_argv);
>> +	free(rec_argv);
>> +	return ret;
>>  }
>>  
>>  int cmd_kvm(int argc, const char **argv)
>> -- 
>> 2.34.1
>>

Re: [PATCH 2/5] perf tools kvm: Fix the potential out of range memory access issue

[Mi, Dapeng]

On 8/7/2025 7:53 AM, Namhyung Kim wrote:
> On Tue, Aug 05, 2025 at 08:46:30AM +0800, Dapeng Mi wrote:
>> 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>
>> ---
>>  tools/perf/builtin-kvm.c | 14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
>> index f78a67a199ff..7e48d2437710 100644
>> --- a/tools/perf/builtin-kvm.c
>> +++ b/tools/perf/builtin-kvm.c
>> @@ -2002,12 +2002,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;
> While at it, can we add a comment for this too?  I believe it's for "-o"
> and filename.

Sure.


>
> Thanks,
> Namhyung
>
>
>> -	rec_argv = calloc(rec_argc + 1, sizeof(char *));
>> +	/*
>> +	 * kvm_add_default_arch_event() may add 2 extra options, so
>> +	 * allocate 2 more pointers in adavance.
>> +	 */
>> +	rec_argv = calloc(rec_argc + 2 + 1, sizeof(char *));
>>  	if (!rec_argv)
>>  		return -ENOMEM;
>>  
>> @@ -2019,6 +2019,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)
>> +		return -EINVAL;
>> +
>>  	ret = cmd_record(i, rec_argv);
>>  	free(rec_argv);
>>  	return ret;
>> -- 
>> 2.34.1
>>

Re: [PATCH 4/5] perf tools kvm: Use "cycles" to sample guest for "kvm record" on Intel

[Mi, Dapeng]

On 8/7/2025 8:08 AM, Namhyung Kim wrote:
> On Tue, Aug 05, 2025 at 08:46:32AM +0800, Dapeng Mi wrote:
>> 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.
> Do you mean we cannot use "cycles:PG" for perf kvm record?

Yes. Here is the output on Intel Sapphire rapids.

sudo ./perf record -e cycles:PG -a
^C[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.801 MB perf.data ]

No guest records are captured with PEBS, and guest PEBS records can be
sampled only without PEBS.

sudo ./perf record -e cycles:G -a
^C[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.798 MB perf.data (60 samples) ]


>
>> 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) ]
> What if user already gave some events in the command line?  I think you
> need to check if "-e" or "--event" (and "--pfm-events" too) is in the
> argv[] before adding these.

kvm_add_default_arch_event() would detect if user already sets events explicitly. If so, it won't add "cycles" event any more. Thanks.

>
> Thanks,
> Namhyung
>
>> Reported-by: Kevin Tian <kevin.tian@intel.com>
>> Fixes: 634d36f82517 ("perf record: Just use "cycles:P" as the default event")
>> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
>> ---
>>  tools/perf/arch/x86/util/kvm-stat.c | 46 +++++++++++++++++++++++++++++
>>  1 file changed, 46 insertions(+)
>>
>> diff --git a/tools/perf/arch/x86/util/kvm-stat.c b/tools/perf/arch/x86/util/kvm-stat.c
>> index 424716518b75..cdb5f3e1b5be 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,47 @@ 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 i, j = *argc;
>> +
>> +	const struct option event_options[] = {
>> +		OPT_BOOLEAN('e', "event", &event, NULL),
>> +		OPT_END()
>> +	};
>> +
>> +	if (!x86__is_intel_cpu())
>> +		return 0;
>> +
>> +	tmp = calloc(j + 1, sizeof(char *));
>> +	if (!tmp)
>> +		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) {
>> +		argv[j++] = strdup("-e");
>> +		argv[j++] = strdup("cycles");
>> +		*argc += 2;
>> +	}
>> +
>> +	free(tmp);
>> +	return 0;
>> +}
>> -- 
>> 2.34.1
>>

Re: [PATCH 4/5] perf tools kvm: Use "cycles" to sample guest for "kvm record" on Intel

[Namhyung Kim]

On Thu, Aug 07, 2025 at 11:08:11AM +0800, Mi, Dapeng wrote:
> 
> On 8/7/2025 8:08 AM, Namhyung Kim wrote:
> > On Tue, Aug 05, 2025 at 08:46:32AM +0800, Dapeng Mi wrote:
> >> 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.
> > Do you mean we cannot use "cycles:PG" for perf kvm record?
> 
> Yes. Here is the output on Intel Sapphire rapids.
> 
> sudo ./perf record -e cycles:PG -a
> ^C[ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.801 MB perf.data ]
> 
> No guest records are captured with PEBS, and guest PEBS records can be
> sampled only without PEBS.
> 
> sudo ./perf record -e cycles:G -a
> ^C[ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.798 MB perf.data (60 samples) ]
> 
> 
> >
> >> 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) ]
> > What if user already gave some events in the command line?  I think you
> > need to check if "-e" or "--event" (and "--pfm-events" too) is in the
> > argv[] before adding these.
> 
> kvm_add_default_arch_event() would detect if user already sets events explicitly. If so, it won't add "cycles" event any more. Thanks.

Oh, ok.  I can see you called parse_options to check the option.
You'd better to check "--pfm-events" as well.

Thanks,
Namhyung

Re: [PATCH 4/5] perf tools kvm: Use "cycles" to sample guest for "kvm record" on Intel

[Mi, Dapeng]

On 8/9/2025 6:10 AM, Namhyung Kim wrote:
> On Thu, Aug 07, 2025 at 11:08:11AM +0800, Mi, Dapeng wrote:
>> On 8/7/2025 8:08 AM, Namhyung Kim wrote:
>>> On Tue, Aug 05, 2025 at 08:46:32AM +0800, Dapeng Mi wrote:
>>>> 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.
>>> Do you mean we cannot use "cycles:PG" for perf kvm record?
>> Yes. Here is the output on Intel Sapphire rapids.
>>
>> sudo ./perf record -e cycles:PG -a
>> ^C[ perf record: Woken up 1 times to write data ]
>> [ perf record: Captured and wrote 0.801 MB perf.data ]
>>
>> No guest records are captured with PEBS, and guest PEBS records can be
>> sampled only without PEBS.
>>
>> sudo ./perf record -e cycles:G -a
>> ^C[ perf record: Woken up 1 times to write data ]
>> [ perf record: Captured and wrote 0.798 MB perf.data (60 samples) ]
>>
>>
>>>> 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) ]
>>> What if user already gave some events in the command line?  I think you
>>> need to check if "-e" or "--event" (and "--pfm-events" too) is in the
>>> argv[] before adding these.
>> kvm_add_default_arch_event() would detect if user already sets events explicitly. If so, it won't add "cycles" event any more. Thanks.
> Oh, ok.  I can see you called parse_options to check the option.
> You'd better to check "--pfm-events" as well.

Sure. Thanks.


>
> Thanks,
> Namhyung
>
>