linux-kernel - Re: [PATCH 084/208] x86/fpu: Rename xsave.header::xstate

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: <20150506061603.GA13720@gmail.com>
Date:	Wed, 6 May 2015 08:16:03 +0200
From:	Ingo Molnar <mingo2.kernel.org@...il.com>
To:	Dave Hansen <dave.hansen@...ux.intel.com>
Cc:	linux-kernel@...r.kernel.org,
	Andy Lutomirski <luto@...capital.net>,
	Borislav Petkov <bp@...en8.de>,
	Fenghua Yu <fenghua.yu@...el.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Oleg Nesterov <oleg@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH 084/208] x86/fpu: Rename xsave.header::xstate_bv to
 'xfeatures'

* Dave Hansen <dave.hansen@...ux.intel.com> wrote:

> On 05/05/2015 11:16 AM, Ingo Molnar wrote:
> > We could put the SDM name into a comment, next to the field 
> > definition? Something like, if 'xfeatures' is too long:
> > 
> > struct xstate_header {
> >         u64     xfeat;        /* xstate components,           SDM: XSTATE_BV */
> >         u64     xfeat_comp;   /* compacted xstate components, SDM: XCOMP_BV */
> >         u64     reserved[6];
> > } __attribute__((packed));
> 
> When you're in the depths of the SDM and the kernel code, the fewer 
> context switches you have to make, the better. [...]

But that's not the only consideration. While in general I'm all for 
following reference hardware documentation with names, there's a limit 
for how far we'll go in following stupid vendor names, and I think 
'XSTATE_BV' and 'XCOMP_BV' are well beyond any sane limit (see further 
below my suggestion for better naming).

> [...]  I say this from the perspective of someone who's had a copy 
> of the SDM open to xsave* for about a month straight.

If only one of us worked at the company that invented those 
nonsensical names and complex SDMs, and could complain to them? ;-)

> In any case, having "xfeat" and "xfeat_comp" is a bad idea.  They're 
> not really related concepts other than their bits refer to the same 
> states.  They should not have such similar names.

Agreed.

> XSTATE_BV is the set of states written to the xsave area.
> 
> XCOMP_BV is essentially always XCR0 (aka pcntxt_mask, aka 
> xfeatures_mask) or'd with bit 63.

So how about this naming:

	/*
	 * Mask of xstate components currently not in init state, 
	 * typically written to by XSAVE*.
	 */
	u64 xfeat_mask_used; /* SDM: XSTATE_BV */

	/*
	 * Mask of all state components saved/restored, plus the 
	 * compaction flag. (Note that the XRSTORS instruction caches
	 * this value, and the next SAVES done for this same
	 * area expects it to match, before it can perform the 'were
	 * these registers modified' hardware optimization.)
	 */
	u64 xfeat_mask_all; /* SDM: XCOMP_BV */

(Note that I kept the SDM name easily greppable.)

The 'compaction' aspect of 'xfeat_mask_all' is just an additional 
quirk that does not deserve to be represented in the primary naming: 
bit 63 of 'xfeat_mask_all' is set to 1 if the format is compacted: 
basically 'compaction' can be thought of as an additional, special 
'xfeature' that modifies the offsets in the save area to eliminate 
holes. [*]

Basically this naming tells us the biggest, most relevant 
differentiation between these two fields:

 - the 'xfeat_mask_used' field reflects the current, momentary,
   optimized state of the area. This mask is content dependent,
   and it is a subset of:

 - the 'xfeat_mask_all' field which reflects all states supported by
   that fpstate context. This mask is content independent.

The compaction aspect of 'xfeat_mask_all' is obviously important to 
the hardware (and depending on its value the position of various 
registers in the save area are different), but secondary to the big 
picture.

Note that once you have a good name, a lot of code becomes a lot more 
obvious - and I wish Intel did more than just googling for the first 
available historic QuickBASIC variable name when picking new CPU 
symbols.

Thanks,

	Ingo

[*]

Btw., does Intel have any special plans with xstate compaction?

AFAICS in Linux we just want to enable xfeat_mask_all to the max, 
including compaction, and never really modify it (in the task's 
lifetime).

I'm also wondering whether there will be any real 'holes' in the 
xfeatures capability masks of future CPUs: right now xfeatures tend to 
be already 'compacted' (because new CPUs tend to support all 
xfeatures), so compaction mostly appears to be an academic feature. Or 
is there already hardware out there where it matter?

Maybe once we get AVX512 in addition to MPX we can use compaction 
materially: as there will be lots of tasks without MPX state but with 
AVX512 state - in fact I suspect that will be the common case.

OTOH MPX state is relatively small compared to AVX and AVX512 state, 
so skipping the hole won't buy us much, and the question is, how 
expensive is compaction, will save/restore be slower with compaction 
enabled? Has to be measured I suspect.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/