RE: [PATCH 151/208] x86/fpu: Introduce cpu_has_xfeatures(xfeatures_mask, feature_name)

[Yu, Fenghua]

> From: Ingo Molnar [mailto:mingo.kernel.org@xxxxxxxxx] On Behalf Of Ingo
> Molnar
> Sent: Tuesday, May 05, 2015 10:51 AM
> To: linux-kernel@xxxxxxxxxxxxxxx
> Cc: Andy Lutomirski; Borislav Petkov; Dave Hansen; Yu, Fenghua; H. Peter
> Anvin; Linus Torvalds; Oleg Nesterov; Thomas Gleixner
> Subject: [PATCH 151/208] x86/fpu: Introduce
> cpu_has_xfeatures(xfeatures_mask, feature_name)
> 
> A lot of FPU using driver code is querying complex CPU features to be able to
> figure out whether a given set of xstate features is supported by the CPU or
> not.
> 
> Introduce a simplified API function that can be used on any CPU type to get
> this information. Also add an error string return pointer, so that the driver
> can print a meaningful error message with a standardized feature name.
> 
> Also mark xfeatures_mask as __read_only.
> 
> Cc: Andy Lutomirski <luto@xxxxxxxxxxxxxx>
> Cc: Borislav Petkov <bp@xxxxxxxxx>
> Cc: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> Cc: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> Cc: H. Peter Anvin <hpa@xxxxxxxxx>
> Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Cc: Oleg Nesterov <oleg@xxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Signed-off-by: Ingo Molnar <mingo@xxxxxxxxxx>
> ---
>  arch/x86/include/asm/fpu/api.h |  9 +++++++++
>  arch/x86/kernel/fpu/xstate.c   | 53
> ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 61 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/fpu/api.h
> b/arch/x86/include/asm/fpu/api.h index 62035cc1d961..1429a7c736db
> 100644
> --- a/arch/x86/include/asm/fpu/api.h
> +++ b/arch/x86/include/asm/fpu/api.h
> @@ -36,4 +36,13 @@ extern bool irq_fpu_usable(void);  extern int
> irq_ts_save(void);  extern void irq_ts_restore(int TS_state);
> 
> +/*
> + * Query the presence of one or more xfeatures. Works on any legacy CPU
> as well.
> + *
> + * If 'feature_name' is set then put a human-readable description of
> + * the feature there as well - this can be used to print error (or
> +success)
> + * messages.
> + */
> +extern int cpu_has_xfeatures(u64 xfeatures_mask, const char
> +**feature_name);
> +
>  #endif /* _ASM_X86_FPU_API_H */
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index f549e2a44336..2e52f01f4931 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -11,10 +11,23 @@
>  #include <asm/tlbflush.h>
>  #include <asm/xcr.h>
> 
> +static const char *xfeature_names[] =
> +{
> +	"x87 floating point registers"	,
> +	"SSE registers"			,
> +	"AVX registers"			,
> +	"MPX bounds registers"		,
> +	"MPX CSR"			,
> +	"AVX-512 opmask"		,
> +	"AVX-512 Hi256"			,
> +	"AVX-512 ZMM_Hi256"		,
> +	"unknown xstate feature"	,
> +};
> +
>  /*
>   * Mask of xstate features supported by the CPU and the kernel:
>   */
> -u64 xfeatures_mask;
> +u64 xfeatures_mask __read_mostly;
> 
>  /*
>   * Represents init state for the supported extended state.
> @@ -29,6 +42,44 @@ static unsigned int
> xstate_comp_offsets[sizeof(xfeatures_mask)*8];
>  static unsigned int xfeatures_nr;
> 
>  /*
> + * Return whether the system supports a given xfeature.
> + *
> + * Also return the name of the (most advanced) feature that the caller
> requested:
> + */
> +int cpu_has_xfeatures(u64 xfeatures_needed, const char **feature_name)
> +{
> +	u64 xfeatures_missing = xfeatures_needed & ~xfeatures_mask;
> +
> +	if (unlikely(feature_name)) {
> +		long xfeature_idx, max_idx;
> +		u64 xfeatures_print;
> +		/*
> +		 * So we use FLS here to be able to print the most advanced
> +		 * feature that was requested but is missing. So if a driver
> +		 * asks about "XSTATE_SSE | XSTATE_YMM" we'll print the
> +		 * missing AVX feature - this is the most informative message
> +		 * to users:
> +		 */
> +		if (xfeatures_missing)
> +			xfeatures_print = xfeatures_missing;

If the feature is missing (xfeatures_missing!=0), the xfeature_name will point to the next feature name with the higher idx in xfeatures_mask.
Is that supposed to be?

I think the "if (xfeatures_missing)" and " xfeatures_print = xfeatures_missing;" are not needed, right?

> +		else
> +			xfeatures_print = xfeatures_needed;
> +
> +		xfeature_idx = fls64(xfeatures_print)-1;
> +		max_idx = ARRAY_SIZE(xfeature_names)-1;
> +		xfeature_idx = min(xfeature_idx, max_idx);
> +
> +		*feature_name = xfeature_names[xfeature_idx];
> +	}
> +
> +	if (xfeatures_missing)
> +		return 0;
> +
> +	return 1;
> +}
> +EXPORT_SYMBOL_GPL(cpu_has_xfeatures);
> +
> +/*
>   * When executing XSAVEOPT (optimized XSAVE), if a processor
> implementation
>   * detects that an FPU state component is still (or is again) in its
>   * initialized state, it may clear the corresponding bit in the header.xfeatures
> --
> 2.1.0

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