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

[Namhyung Kim <namhyung@kernel.org>]

On Tue, Jun 23, 2026 at 07:16:40AM +0000, sashiko-bot@kernel.org wrote:
> 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?

Right, will fix in v2.

> 
> [ ... ]
> 
> > 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.

Good point, will add.

Thanks,
Namhyung

> 
> > +	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