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

[Ingo Molnar]

* Yu, Fenghua <fenghua.yu@xxxxxxxxx> wrote:

> > 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?

Yes, so this is a reporting detail. The intention here is the 
following: when a driver requests multiple features to be present, for 
example:

	if (!cpu_has_xfeatures(XSTATE_SSE | XSTATE_YMM, NULL)) {

then it makes sense to report the highest feature bit, not the first 
one we find to not exist. Why? Because in the above example, on an old 
CPU we could miss on XSTATE_SSE already, and report that to the user - 
which creates the incorrect assumption that the minimum requirement is 
for SSE. The user then tries the same driver on a more modern system, 
which has SSE but not YMM, and gets a shifting goal post.

So I wanted to report the most modern feature that is missing, to 
avoid such kind of reporting ambiguity on older systems.

Alternatively we could iterate the mask in reverse order as well?

Thanks,

	Ingo
--
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/