From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751568AbbH1FRF (ORCPT <rfc822;w@1wt.eu>);
	Fri, 28 Aug 2015 01:17:05 -0400
Received: from mail-wi0-f173.google.com ([209.85.212.173]:38088 "EHLO
	mail-wi0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1750750AbbH1FRD (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Fri, 28 Aug 2015 01:17:03 -0400
Date: Fri, 28 Aug 2015 07:16:59 +0200
From: Ingo Molnar <mingo@kernel.org>
To: Dave Hansen <dave@sr71.net>
Cc: dave.hansen@linux.intel.com, mingo@redhat.com, x86@kernel.org,
        bp@alien8.de, fenghua.yu@intel.com, tim.c.chen@linux.intel.com,
        linux-kernel@vger.kernel.org,
        Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [PATCH 02/11] x86, fpu: rename xfeature_bit
Message-ID: <20150828051659.GF25556@gmail.com>
References: <20150827171102.1BDF27E5@viggo.jf.intel.com>
 <20150827171103.7041D911@viggo.jf.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20150827171103.7041D911@viggo.jf.intel.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


* Dave Hansen <dave@sr71.net> wrote:

> +++ b/arch/x86/include/asm/fpu/types.h	2015-08-27 10:08:01.572634311 -0700
> @@ -95,27 +95,31 @@ struct swregs_state {
>  /*
>   * List of XSAVE features Linux knows about:
>   */
> -enum xfeature_bit {
> -	XSTATE_BIT_FP,
> -	XSTATE_BIT_SSE,
> -	XSTATE_BIT_YMM,
> -	XSTATE_BIT_BNDREGS,
> -	XSTATE_BIT_BNDCSR,
> -	XSTATE_BIT_OPMASK,
> -	XSTATE_BIT_ZMM_Hi256,
> -	XSTATE_BIT_Hi16_ZMM,
> +enum xfeature_nr {
> +	XFEATURE_NR_FP,
> +	XFEATURE_NR_SSE,
> +	/*
> +	 * Values above here are "legacy states".
> +	 * Those below are "extended states".
> +	 */
> +	XFEATURE_NR_YMM,
> +	XFEATURE_NR_BNDREGS,
> +	XFEATURE_NR_BNDCSR,
> +	XFEATURE_NR_OPMASK,
> +	XFEATURE_NR_ZMM_Hi256,
> +	XFEATURE_NR_Hi16_ZMM,
>  
>  	XFEATURES_NR_MAX,
>  };
> +#define XSTATE_FP		(1 << XFEATURE_NR_FP)
> +#define XSTATE_SSE		(1 << XFEATURE_NR_SSE)
> +#define XSTATE_YMM		(1 << XFEATURE_NR_YMM)
> +#define XSTATE_BNDREGS		(1 << XFEATURE_NR_BNDREGS)
> +#define XSTATE_BNDCSR		(1 << XFEATURE_NR_BNDCSR)
> +#define XSTATE_OPMASK		(1 << XFEATURE_NR_OPMASK)
> +#define XSTATE_ZMM_Hi256	(1 << XFEATURE_NR_ZMM_Hi256)
> +#define XSTATE_Hi16_ZMM		(1 << XFEATURE_NR_Hi16_ZMM)

So I think this is still somewhat confusing.

'NR' is often used as a maximum kind of thing, not as a bit index.

So I think we should instead take up the existing conventions of the cpufeatures.h 
definitions which are pretty sane, and simply name the bit indices XFEATURE_XYZ:

enum xfeatures {
	XFEATURE_FP,
	XFEATURE_SSE,
	...
	XFEATURE_MAX
};

this way we ensure that bitmasks are visibly masks, i.e.:

	#define XFEATURE_MASK_FP		(1 << XFEATURE_FP)
	#define XFEATURE_MASK_SSE		(1 << XFEATURE_SSE)

it's slightly longer to write but unambiguous, and it also matches what we use for 
the x86 CPU ID feature definitions.

Similarly we could rename other mask fields from 'xstate' to 'xfeature_mask', to 
make it all consistent throughout.

What do you think?

Thanks,

	Ingo