Message-ID: <554A2E90.3010201@linux.intel.com>
Date: Wed, 06 May 2015 08:09:04 -0700
From: Dave Hansen <dave.hansen@linux.intel.com>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0
MIME-Version: 1.0
To: Ingo Molnar <mingo2.kernel.org@gmail.com>
CC: linux-kernel@vger.kernel.org, Andy Lutomirski <luto@amacapital.net>,
        Borislav Petkov <bp@alien8.de>, Fenghua Yu <fenghua.yu@intel.com>,
        "H. Peter Anvin" <hpa@zytor.com>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Oleg Nesterov <oleg@redhat.com>, Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH 084/208] x86/fpu: Rename xsave.header::xstate_bv to 'xfeatures'
References: <1430848300-27877-1-git-send-email-mingo@kernel.org> <1430848300-27877-6-git-send-email-mingo@kernel.org> <554904A6.8040503@linux.intel.com> <20150505181613.GA28562@gmail.com> <55490B1F.3080409@linux.intel.com> <20150506061603.GA13720@gmail.com> <20150506124618.GA18655@gmail.com>
In-Reply-To: <20150506124618.GA18655@gmail.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2590
Lines: 66

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.
 */
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/