Re: [PATCH 02/11] x86, fpu: rename xfeature_bit

[Ingo Molnar <mingo@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