Re: [PATCH 084/208] x86/fpu: Rename xsave.header::xstate_bv to 'xfeatures'

[Ingo Molnar]

* Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx> wrote:


> On 05/06/2015 05:46 AM, Ingo Molnar wrote:
> > So a better name would be:
> > 
> >  	/*
> >  	 * Mask of xstate components currently not in init state, 
> >  	 * typically written to by XSAVE*.
> >  	 */
> > 	u64 xfeat_used_mask;	/* SDM: XSTATE_BV */
> 
> The comment and name make sense if we always call xsave* with an 
> "instruction mask" where it has at least as many bits as we have set 
> in 'pcntxt_mask' (aka xfeatures_mask).
> 
> If we ever get to a point where we are saving only a subset of the 
> supported features (say if we only wanted to consult the MPX 
> registers and none of the other state), then this stops making 
> sense.
> 
> I think 'xfeat_saved_mask' or 'xstate_saved_mask' makes more sense.

Good, that name works for me too: it still expresses the dynamic, 
content-dependent nature of this mask.

> Maybe a comment like:
> 
>   	/*
>   	 * Mask of xstate components currently present in the buffer.
> 	 * A non-present bit can mean that the feature is in the init
> 	 * state or that we did not ask the instruction to save it.
>   	 * typically written to by XSAVE*.
>   	 */
> 
> >  	/*
> >  	 * This mask is non-zero if the CPU supports state compaction: 
> > 	 * it is the mask of all state components to be saved/restored, 
> > 	 * plus the compaction flag at bit 63.
> 
> That's not correct.  It's non-zero if it supports compaction and it 
> was saved using an instruction that supports compaction.  A CPU 
> supporting xsaves, but using xsave will receive an uncompacted 
> version with xcomp_bv=0.

That is what I meant: under Linux this bit is non-zero if the CPU 
supports state compaction.

> > 	 * (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_comp_mask;	/* SDM: XCOMP_BV */
> 
> That seems like a bit of a silly thing to mention in a comment since 
> it never changes.

But that's the reason why in Linux we don't really change it - we 
don't go about trying to slice&dice the xstate into multiple 
components. We just save it all and let the 'init' and 'modified' 
optimizations in the hardware take care of the optimizations.

> How about something like this?
> 
> /*
>  * Must be 0 when compacted state not supported.  If compacted state is
>  * supported and XRSTOR variant understands both formats, Bit 63 tells
>  * instruction which format is to be used.

Yes.

Btw., as a side note, this is a silly hardware interface: not setting 
bit 63 is not valid as per the SDM (will cause a #GP), so it might as 
well have left the whole bit 63 complication out of it: if this mask 
is nonzero then a compacted format is requested. If it's zero, then a 
non-compacted format.

Right?

>  *
>  * This tells you the format of the buffer when using compacted layout.
>  * The format is determined by the features enabled in XCR* along with
>  * the features requested at XSAVE* time (SDM: RFBM).

I has not 2 but 3 inputs: what is being saved/restored is determined 
by this very bitmask here. If a bit is missing from this mask, it 
won't be saved (it's 'compacted'), even if it's otherwise set in XCR* 
or is requested in the instruction mask.

That's the whole point of compaction.

>  *
>  * Note that even if a feature is present in this mask, it may still be
>  * absent from 'xfeat_used_mask', which means that space was allocated
>  * in the layout, but that it was not actually written.

So here I'd mention _why_ it can be left out from xfeat_used_mask: 
when an xstate component is in 'init state', then only 0 is written to 
the xfeat_used_mask but the component area itself is not touched.

This means that previous contents of the saved area are still there 
and are stale, so the kernel has to be careful about not exposing 
these to user-space indiscriminately. That's what the 'sanitization' 
functions are about.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/