Re: [PATCH v7 38/41] x86/fpu: Add helper for initing features

[Edgecombe, Rick P]

On Sat, 2023-03-11 at 13:54 +0100, Borislav Petkov wrote:
> On Mon, Feb 27, 2023 at 02:29:54PM -0800, Rick Edgecombe wrote:
> > Subject: Re: [PATCH v7 38/41] x86/fpu: Add helper for initing
> > features
> 
> "initializing"

Sure.

> 
> > If an xfeature is saved in a buffer, the xfeature's bit will be set
> > in
> > xsave->header.xfeatures. The CPU may opt to not save the xfeature
> > if it
> > is in it's init state. In this case the xfeature buffer address
> > cannot
> 
> "its"

I clearly need to be better about it's and its.

> 
> > be retrieved with get_xsave_addr().
> > 
> > Future patches will need to handle the case of writing to an
> > xfeature
> > that may not be saved. So provide helpers to init an xfeature in an
> > xsave buffer.
> > 
> > This could of course be done directly by reaching into the xsave
> > buffer,
> > however this would not be robust against future changes to optimize
> > the
> > xsave buffer by compacting it. In that case the xsave buffer would
> > need
> > to be re-arranged as well. So the logic properly belongs
> > encapsulated
> > in a helper where the logic can be unified.
> > 
> > Tested-by: Pengfei Xu <pengfei.xu@xxxxxxxxx>
> > Tested-by: John Allen <john.allen@xxxxxxx>
> > Tested-by: Kees Cook <keescook@xxxxxxxxxxxx>
> > Acked-by: Mike Rapoport (IBM) <rppt@xxxxxxxxxx>
> > Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx>
> > 
> > ---
> > v2:
> >  - New patch
> > ---
> >  arch/x86/kernel/fpu/xstate.c | 58 +++++++++++++++++++++++++++++---
> > ----
> >  arch/x86/kernel/fpu/xstate.h |  6 ++++
> >  2 files changed, 53 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/fpu/xstate.c
> > b/arch/x86/kernel/fpu/xstate.c
> > index 13a80521dd51..3ff80be0a441 100644
> > --- a/arch/x86/kernel/fpu/xstate.c
> > +++ b/arch/x86/kernel/fpu/xstate.c
> > @@ -934,6 +934,24 @@ static void *__raw_xsave_addr(struct
> > xregs_state *xsave, int xfeature_nr)
> >  	return (void *)xsave + xfeature_get_offset(xcomp_bv,
> > xfeature_nr);
> >  }
> >  
> > +static int xsave_buffer_access_checks(int xfeature_nr)
> 
> Function name needs a verb.

Right.

> 
> > +{
> > +	/*
> > +	 * Do we even *have* xsave state?
> > +	 */
> 
> That comment is superfluous.
> 
> > +	if (!boot_cpu_has(X86_FEATURE_XSAVE))
> 
> check_for_deprecated_apis: WARNING: arch/x86/kernel/fpu/xstate.c:942:
> Do not use boot_cpu_has() - use cpu_feature_enabled() instead.
> 
> > +		return 1;
> > +
> > +	/*
> > +	 * We should not ever be requesting features that we
> 
> Please use passive voice in your commit message: no "we" or "I", etc,
> and describe your changes in imperative mood.

These two are from the existing code. Basically they get extracted into
a new function.

> 
> > +	 * have not enabled.
> > +	 */
> > +	if (WARN_ON_ONCE(!xfeature_enabled(xfeature_nr)))
> > +		return 1;
> > +
> > +	return 0;
> > +}
> > +
> >  /*
> >   * Given the xsave area and a state inside, this function returns
> > the
> >   * address of the state.
> > @@ -954,17 +972,7 @@ static void *__raw_xsave_addr(struct
> > xregs_state *xsave, int xfeature_nr)
> >   */
> >  void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr)
> >  {
> > -	/*
> > -	 * Do we even *have* xsave state?
> > -	 */
> > -	if (!boot_cpu_has(X86_FEATURE_XSAVE))
> > -		return NULL;
> > -
> > -	/*
> > -	 * We should not ever be requesting features that we
> > -	 * have not enabled.
> > -	 */
> > -	if (WARN_ON_ONCE(!xfeature_enabled(xfeature_nr)))
> > +	if (xsave_buffer_access_checks(xfeature_nr))
> >  		return NULL;
> >  
> >  	/*
> > @@ -984,6 +992,34 @@ void *get_xsave_addr(struct xregs_state
> > *xsave, int xfeature_nr)
> >  	return __raw_xsave_addr(xsave, xfeature_nr);
> >  }
> >  
> > +/*
> > + * Given the xsave area and a state inside, this function
> > + * initializes an xfeature in the buffer.
> 
> s/this function initializes/initialize/

Sure.

> 
> > + *
> > + * get_xsave_addr() will return NULL if the feature bit is
> > + * not present in the header. This function will make it so
> > + * the xfeature buffer address is ready to be retrieved by
> > + * get_xsave_addr().
> 
> So users of get_xsave_addr() would have to know that they would need
> to
> call init_xfeature()?

That is the situation today. FWIW both of these functions are limited
to the FPU internals, so I would think it's not a too unreasonable
assumption.

> 
> I think the better approach would be:
> 
> void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr, bool
> init)
> 
> and then that @init controls whether get_xsave_addr() should init the
> buffer.
> 
> And then you don't have to have a bunch of small functions here and
> there and know when to call what but get_xsave_addr() would simply
> DTRT.

It would have to actually copy the init state to the buffer
from init_fpstate, because otherwise the caller couldn't know 
if get_xsave_addr() was returning valid data or some old data in the
buffer. And I guess the `init` mean to initialize it only if it is in
the init state, not to overwrite the current state with the init state.

I did it up, and it makes the caller code cleaner. But I'm not sure
what to think of it. Is this not mixing two operations together? Today
get_xsave_addr() pretty much just gets a buffer offset with some
checks. Now it would compute the offset and also silently go off and
changes the buffer.

I looked at this fpu code originally and thought I could add some
useful abstractions, but this failed. I came away wondering if this was
just an area with so many special cases and details, that abstractions
just added confusion. I'm just bringing this up because the other
option is to just do this in the regset code:
xsave->header.xfeatures |= BIT_ULL(XFEATURE_CET_USER);

Let me know if you think it would be better to just open code it.

> 
> Thx.
>