Re: [OPTIONAL/RFC v2 36/39] x86/fpu: Add helper for initing features

Edgecombe, Rick P
2022-10-04T23:06:33Z

On Mon, 2022-10-03 at 12:07 -0700, Chang S. Bae wrote:
> > +/*
> > + * 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?

No, it doesn't actually write out the init state to the buffer.

>
> > + *
> > + * 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.

Thomas had experimented with this optimization where init state
features weren't saved:
https://lore.kernel.org/lkml/20220404103741.809025935@linutronix.de/

It made me think non-fpu code should not assume things about the state
of the buffer, as FPU code might have to move things when initing them.
So the operation is worth centralizing in a helper. I think you are
right, today it is not doing much and could be open coded. I guess the
question is, should it be open coded or centralized? I'm fine either
way.