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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] `__cmd_top()` unconditionally appends the default architecture event, breaking user-provided event overrides.
- [Low] Missing explicit POSIX <stdlib.h> header inclusion in kvm-stat.c for calloc() and free().
--

commit 3e2018b99366e21f6651143f8ed90e3f8257ec72
Author: Namhyung Kim <namhyung@kernel.org>

perf kvm: Factor out kvm_need_default_arch_event()

This commit factors out the logic for checking existing event command line
options into a generic kvm_need_default_arch_event() function. It removes the
duplicated option checking logic from architecture-specific implementations
of kvm_add_default_arch_event() and updates __cmd_record() to use the new
guard.

> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> index 0c5e6b3aac74d..c236093eac473 100644
> --- a/tools/perf/builtin-kvm.c
> +++ b/tools/perf/builtin-kvm.c
> @@ -2015,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(argc, argv)) {
> +		ret = kvm_add_default_arch_event(EM_HOST, &i, rec_argv);
> +		if (ret)
> +			goto EXIT;
> +	}
>  
>  	ret = cmd_record(i, rec_argv);
>  

[Severity: High]
Is it possible that __cmd_top() is missing the kvm_need_default_arch_event()
guard?

Since the option parsing logic was removed from kvm_add_default_arch_event(),
it now unconditionally appends the default event.

__cmd_top() in tools/perf/builtin-kvm.c still calls it unconditionally:

__cmd_top()
        ...
        ret = kvm_add_default_arch_event(EM_HOST, &i, rec_argv);
        if (ret)
                goto EXIT;

Will this cause perf kvm top to ignore user-provided event overrides
(like -e some_event) and blindly append the default event?

[ ... ]

> diff --git a/tools/perf/util/kvm-stat.c b/tools/perf/util/kvm-stat.c
> index 27f16810498ca..dd599dd0e4322 100644
> --- a/tools/perf/util/kvm-stat.c
> +++ b/tools/perf/util/kvm-stat.c
[ ... ]
> @@ -271,3 +272,29 @@ int kvm_add_default_arch_event(uint16_t e_machine, int *argc, const char **argv)
>  		return 0;
>  	}
>  }
> +
> +bool kvm_need_default_arch_event(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()
> +	};
> +
> +	/* parse_options() may change the argv, let's make a copy */
> +	tmp_argv = calloc(argc + 1, sizeof(char *));

[Severity: Low]
Should we explicitly include <stdlib.h> in this file to support musl libc
builds?

The new kvm_need_default_arch_event() function relies on calloc() and free(),
but <stdlib.h> isn't explicitly included in tools/perf/util/kvm-stat.c.

Because musl libc enforces strict standard header separation, relying on
implicit includes can cause the build to fail.

> +	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/20260623070313.55225-1-namhyung@kernel.org?part=1