Re: [PATCH v2 1/8] x86/fpu/xstate: Define new macros for supervisor and user xstates

[Borislav Petkov <bp@alien8.de>]

On Tue, Jan 21, 2020 at 12:18:36PM -0800, Yu-cheng Yu wrote:
> From: Fenghua Yu <fenghua.yu@intel.com>
> 
> XCNTXT_MASK is 'all supported xfeatures' before introducing supervisor
> xstates.  Rename it to SUPPORTED_XFEATURES_MASK_USER to make clear that
> these are user xstates.
> 
> XFEATURE_MASK_SUPERVISOR is replaced with the following:
> - SUPPORTED_XFEATURES_MASK_SUPERVISOR: Currently nothing.  ENQCMD and
>   Control-flow Enforcement Technology (CET) will be introduced in separate
>   series.
> - UNSUPPORTED_XFEATURES_MASK_SUPERVISOR: Currently only Processor Trace.
> - ALL_XFEATURES_MASK_SUPERVISOR: the combination of above.
> 
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> Co-developed-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> ---
>  arch/x86/include/asm/fpu/xstate.h | 36 ++++++++++++++++++++-----------
>  arch/x86/kernel/fpu/init.c        |  3 ++-
>  arch/x86/kernel/fpu/xstate.c      | 26 +++++++++++-----------
>  3 files changed, 38 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
> index c6136d79f8c0..014c386deaa3 100644
> --- a/arch/x86/include/asm/fpu/xstate.h
> +++ b/arch/x86/include/asm/fpu/xstate.h
> @@ -21,19 +21,29 @@
>  #define XSAVE_YMM_SIZE	    256
>  #define XSAVE_YMM_OFFSET    (XSAVE_HDR_SIZE + XSAVE_HDR_OFFSET)
>  
> -/* Supervisor features */
> -#define XFEATURE_MASK_SUPERVISOR (XFEATURE_MASK_PT)
> -
> -/* All currently supported features */
> -#define XCNTXT_MASK		(XFEATURE_MASK_FP | \
> -				 XFEATURE_MASK_SSE | \
> -				 XFEATURE_MASK_YMM | \
> -				 XFEATURE_MASK_OPMASK | \
> -				 XFEATURE_MASK_ZMM_Hi256 | \
> -				 XFEATURE_MASK_Hi16_ZMM	 | \
> -				 XFEATURE_MASK_PKRU | \
> -				 XFEATURE_MASK_BNDREGS | \
> -				 XFEATURE_MASK_BNDCSR)
> +/* All currently supported user features */
> +#define SUPPORTED_XFEATURES_MASK_USER (XFEATURE_MASK_FP | \
> +				       XFEATURE_MASK_SSE | \
> +				       XFEATURE_MASK_YMM | \
> +				       XFEATURE_MASK_OPMASK | \
> +				       XFEATURE_MASK_ZMM_Hi256 | \
> +				       XFEATURE_MASK_Hi16_ZMM	 | \
> +				       XFEATURE_MASK_PKRU | \
> +				       XFEATURE_MASK_BNDREGS | \
> +				       XFEATURE_MASK_BNDCSR)
> +
> +/* All currently supported supervisor features */
> +#define SUPPORTED_XFEATURES_MASK_SUPERVISOR (0)
> +
> +/*
> + * Unsupported supervisor features. When a supervisor feature in this mask is
> + * supported in the future, move it to the supported supervisor feature mask.
> + */
> +#define UNSUPPORTED_XFEATURES_MASK_SUPERVISOR (XFEATURE_MASK_PT)
> +
> +/* All supervisor states including supported and unsupported states. */
> +#define ALL_XFEATURES_MASK_SUPERVISOR (SUPPORTED_XFEATURES_MASK_SUPERVISOR | \
> +				       UNSUPPORTED_XFEATURES_MASK_SUPERVISOR)

So frankly having the namespace prepended in those macros makes it more
readable to me: you know that those masks all belong together if you had
this:

XFEATURE_MASK_SUPERVISOR
XFEATURE_MASK_SUPERVISOR_SUPPORTED
XFEATURE_MASK_SUPERVISOR_UNSUPPORTED
XFEATURE_MASK_SUPERVISOR_ALL
XFEATURE_MASK_USER_SUPPORTED

Now they all begin with different words: "ALL", "UNSUPPORTED",
"SUPPORTED", ... and makes you go and look up the mask to make sure it
is the correct type of mask used.

Even more so if the single feature masks also start with
"XFEATURE_MASK_" so it is only logical to have them all start with
XFEATURE_MASK_ IMO.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette