linux-kernel - Re: [PATCH V3 01/23] perf/x86: Support outputting XMM registers

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]

Message-ID: <20190323095610.GB6058@hirez.programming.kicks-ass.net>
Date:   Sat, 23 Mar 2019 10:56:10 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Andi Kleen <ak@...ux.intel.com>
Cc:     kan.liang@...ux.intel.com, acme@...nel.org, mingo@...hat.com,
        linux-kernel@...r.kernel.org, tglx@...utronix.de, jolsa@...nel.org,
        eranian@...gle.com, alexander.shishkin@...ux.intel.com
Subject: Re: [PATCH V3 01/23] perf/x86: Support outputting XMM registers

On Fri, Mar 22, 2019 at 10:22:50AM -0700, Andi Kleen wrote:
> > > diff --git a/arch/x86/include/uapi/asm/perf_regs.h b/arch/x86/include/uapi/asm/perf_regs.h
> > > index f3329cabce5c..b33995313d17 100644
> > > --- a/arch/x86/include/uapi/asm/perf_regs.h
> > > +++ b/arch/x86/include/uapi/asm/perf_regs.h
> > > @@ -28,7 +28,29 @@ enum perf_event_x86_regs {
> > >  	PERF_REG_X86_R14,
> > >  	PERF_REG_X86_R15,
> > >  
> > > -	PERF_REG_X86_32_MAX = PERF_REG_X86_GS + 1,
> > > -	PERF_REG_X86_64_MAX = PERF_REG_X86_R15 + 1,
> > 
> > So this changes UAPI visible symbols... did we think about that?
> 
> Should be fine. Old programs won't use the new bits,
> and it just uses not yet used bits.

Old programs (that used the above symbols) will now fail to compile.
Even if they won't use the new bits, that seems like a bad thing.

> > > +	/* These all need two bits set because they are 128bit */
> > > +	PERF_REG_X86_XMM0  = 32,
> > > +	PERF_REG_X86_XMM1  = 34,
> > > +	PERF_REG_X86_XMM2  = 36,
> > > +	PERF_REG_X86_XMM3  = 38,
> > > +	PERF_REG_X86_XMM4  = 40,
> > > +	PERF_REG_X86_XMM5  = 42,
> > > +	PERF_REG_X86_XMM6  = 44,
> > > +	PERF_REG_X86_XMM7  = 46,
> > > +	PERF_REG_X86_XMM8  = 48,
> > > +	PERF_REG_X86_XMM9  = 50,
> > > +	PERF_REG_X86_XMM10 = 52,
> > > +	PERF_REG_X86_XMM11 = 54,
> > > +	PERF_REG_X86_XMM12 = 56,
> > > +	PERF_REG_X86_XMM13 = 58,
> > > +	PERF_REG_X86_XMM14 = 60,
> > > +	PERF_REG_X86_XMM15 = 62,
> > > +
> > > +	/* This does not include the XMMX registers */
> > > +	PERF_REG_GPR_X86_32_MAX = PERF_REG_X86_GS + 1,
> > > +	PERF_REG_GPR_X86_64_MAX = PERF_REG_X86_R15 + 1,
> > > +
> > > +	/* All registers include the XMMX registers */
> > > +	PERF_REG_X86_MAX = PERF_REG_X86_XMM15 + 2,
> > >  };
> > >  #endif /* _ASM_X86_PERF_REGS_H */
> > 
> > Also, what happens if we run a 32bit kernel or 32bit compat task?
> > 
> > Then the register dump will report PERF_SAMPLE_REGS_ABI_32, should we
> > then still interpret the XMM registers as 2x64bit?
> 
> Yes XMM registers are 128bit in 32bit mode too.
> 
> > 
> > Are they still at the same offset?
> 
> Yes.

I think that is broken.. perf_prepare_sample() does:

 size += hweight(mask) * sizeof(u64);

And since 32bits will not have r8-r15 set, the XMM registers will shift
forward no?

> > Do we need additional PERF_SAMPLE_REGS_ABI_* definitions for this?
> 
> I don't think so.

because....?