linux-kernel - Re: [PATCH 02/11] x86, fpu: rename xfeature

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150828051659.GF25556@gmail.com>
Date:	Fri, 28 Aug 2015 07:16:59 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	Dave Hansen <dave@...1.net>
Cc:	dave.hansen@...ux.intel.com, mingo@...hat.com, x86@...nel.org,
	bp@...en8.de, fenghua.yu@...el.com, tim.c.chen@...ux.intel.com,
	linux-kernel@...r.kernel.org,
	Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH 02/11] x86, fpu: rename xfeature_bit


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