linux-kernel - Re: [PATCH V2 07/13] perf/x86/intel: Support PERFEVTSEL extension

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 26 Jun 2024 10:27:04 -0700
From: Ian Rogers <irogers@...gle.com>
To: "Liang, Kan" <kan.liang@...ux.intel.com>
Cc: peterz@...radead.org, mingo@...nel.org, acme@...nel.org, 
	namhyung@...nel.org, adrian.hunter@...el.com, 
	alexander.shishkin@...ux.intel.com, linux-kernel@...r.kernel.org, 
	ak@...ux.intel.com, eranian@...gle.com, 
	Dapeng Mi <dapeng1.mi@...ux.intel.com>
Subject: Re: [PATCH V2 07/13] perf/x86/intel: Support PERFEVTSEL extension

On Wed, Jun 26, 2024 at 6:25 AM Liang, Kan <kan.liang@...ux.intel.com> wrote:
>
>
>
> On 2024-06-25 9:34 p.m., Ian Rogers wrote:
> > On Tue, Jun 25, 2024 at 11:22 AM <kan.liang@...ux.intel.com> wrote:
> >>
> >> From: Kan Liang <kan.liang@...ux.intel.com>
> >>
> >> Two new fields (the unit mask2, and the equal flag) are added in the
> >> IA32_PERFEVTSELx MSRs. They can be enumerated by the CPUID.23H.0.EBX.
> >>
> >> Update the config_mask in x86_pmu and x86_hybrid_pmu for the true layout
> >> of the PERFEVTSEL.
> >> Expose the new formats into sysfs if they are available. The umask
> >> extension reuses the same format attr name "umask" as the previous
> >> umask. Add umask2_show to determine/display the correct format
> >> for the current machine.
> >>
> >> Reviewed-by: Andi Kleen <ak@...ux.intel.com>
> >> Co-developed-by: Dapeng Mi <dapeng1.mi@...ux.intel.com>
> >> Signed-off-by: Dapeng Mi <dapeng1.mi@...ux.intel.com>
> >> Signed-off-by: Kan Liang <kan.liang@...ux.intel.com>
> >> ---
> >>  arch/x86/events/intel/core.c      | 69 +++++++++++++++++++++++++++++--
> >>  arch/x86/include/asm/perf_event.h |  4 ++
> >>  2 files changed, 69 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> >> index 23e074fd25e1..9d50e1049e30 100644
> >> --- a/arch/x86/events/intel/core.c
> >> +++ b/arch/x86/events/intel/core.c
> >> @@ -4632,8 +4632,55 @@ PMU_FORMAT_ATTR(pc,      "config:19"     );
> >>  PMU_FORMAT_ATTR(any,   "config:21"     ); /* v3 + */
> >>  PMU_FORMAT_ATTR(inv,   "config:23"     );
> >>  PMU_FORMAT_ATTR(cmask, "config:24-31"  );
> >> -PMU_FORMAT_ATTR(in_tx,  "config:32");
> >> -PMU_FORMAT_ATTR(in_tx_cp, "config:33");
> >> +PMU_FORMAT_ATTR(in_tx,  "config:32"    );
> >> +PMU_FORMAT_ATTR(in_tx_cp, "config:33"  );
> >
> > nit: It seems unfortunate these 2 lines change for the sake of spaces
> > before the ')'. Perhaps leave unchanged.
>
> The two lines don't follow the existing formats. It kind of ugly when
> reading the code.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/events/intel/core.c#n4569
>
> I just want to make the format consistent to facilitate the future code
> review.

Thanks, I'm easy on just keeping it the same or changing the
whitespace. When I saw the lines changing I was figuring the config
encoding was being modified which could break things like libpfm4 that
directly use configs rather than formats.

Thanks,
Ian