Messages in this thread | | | Date | Mon, 3 Oct 2022 12:07:16 -0700 | Subject | Re: [OPTIONAL/RFC v2 36/39] x86/fpu: Add helper for initing features | From | "Chang S. Bae" <> |
| |
On 9/29/2022 3:29 PM, Rick Edgecombe wrote: > 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 > 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. > > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > > --- > > 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 9258fc1169cc..82cee1f2f0c8 100644 > --- a/arch/x86/kernel/fpu/xstate.c > +++ b/arch/x86/kernel/fpu/xstate.c > @@ -942,6 +942,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) > +{ > + /* > + * Do we even *have* xsave state? > + */ > + if (!boot_cpu_has(X86_FEATURE_XSAVE)) > + return 1; > + > + /* > + * We should not ever be requesting features that we > + * 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. > @@ -962,17 +980,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; > > /* > @@ -992,6 +1000,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.
But, this function sets XSTATE_BV bits in the buffer. That does not *initialize* the state, right?
> + * > + * 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().
Looks like this is used in the next patch to help ptracer().
We have the state copy function -- copy_uabi_to_xstate() that retrieves the address using __raw_xsave_addr() instead of get_xsave_addr(), copies the state, and then updates XSTATE_BV.
__raw_xsave_addr() also ensures whether the state is in the compacted format or not. I think you can use it.
Also, I'm curious about the reason why you want to update XSTATE_BV first with this new helper.
Overall, I'm not sure these new helpers are necessary.
Thanks, Chang
| |