Re: [NEEDS-REVIEW] [PATCH v15 03/26] x86/fpu/xstate: Introduce CET MSR XSAVES supervisor states

[Yu, Yu-cheng]

On 11/30/2020 10:12 AM, Dave Hansen wrote:
> On 11/30/20 10:06 AM, Yu, Yu-cheng wrote:
> > > > +            if (!boot_cpu_has(X86_FEATURE_SHSTK) &&
> > > > +                !boot_cpu_has(X86_FEATURE_IBT))
> > > > +                xfeatures_mask_all &= ~BIT_ULL(i);
> > > > +        } else {
> > > > +            if ((xsave_cpuid_features[i] == -1) ||
> > >
> > > Where did the -1 come from?  Was that introduced earlier in this series?
> > >    I don't see any way a xsave_cpuid_features[] can be -1 in the
> > > current tree.
> >
> > Yes, we used to have a hole in xsave_cpuid_features[] and put -1 there.
> > Do we want to keep this in case we again have holes in the future?
>
> So, it's dead code for the moment and it's impossible to tell what -1
> means without looking at git history?  That seems, um, suboptimal.
>
> Shouldn't we have:
>
> #define XFEATURE_NO_DEP -1
>
> ?
>
> And then this code becomes:
>
> 	if ((xsave_cpuid_features[i] == XFEATURE_NO_DEP))
> 		// skip it...
>
> We can even put a comment in xsave_cpuid_features[] to tell folks to use
> it.

Yes, I will work on that.

Yu-cheng