Messages in this thread | | | Date | Wed, 06 May 2015 08:09:04 -0700 | From | Dave Hansen <> | Subject | Re: [PATCH 084/208] x86/fpu: Rename xsave.header::xstate_bv to 'xfeatures' |
| |
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. 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.
> * (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.
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. * * 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). * * 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. */
| |