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@xxxxxxxxxxxxx/ 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.
