Re: [PATCH v3 08/21] x86/fpu/xstate: Define the scope of the initial xstate data

[Bae, Chang Seok]

On Feb 8, 2021, at 04:33, Borislav Petkov <bp@xxxxxxx> wrote:
> On Wed, Dec 23, 2020 at 07:57:04AM -0800, Chang S. Bae wrote:
>> init_fpstate is used to record the initial xstate value for convenience
> 
> convenience?

Yes, this is vague. I think the usage is when (re-)initializing the register
states, e.g. from fpu__clear().

Maybe, drop ‘for convenience’ from this sentence, since the buffer’s usage is
not much relevant in this changelog.

>> and covers all the states. But it is wasteful to cover large states all
>> with trivial initial data.
>> 
>> Limit init_fpstate by clarifying its size and coverage, which are all but
>> dynamic user states. The dynamic states are assumed to be large but having
>> initial data with zeros.
>> 
>> No functional change until the kernel supports dynamic user states.
> 
> What does that mean?
> 
> This patch either makes no functional change or it does...

It does functional change, but it is conditional to AMX enabling.

It includes all the initial states when AMX states not enabled. But it will
exclude the AMX state (with 8KB zeros) with the change.

>> extern union fpregs_state init_fpstate;
>> 
>> +static inline u64 get_init_fpstate_mask(void)
>> +{
>> +	/* init_fpstate covers states in fpu->state. */
>> +	return (xfeatures_mask_all & ~xfeatures_mask_user_dynamic);
>> +}
> 
> If you're going to introduce such a helper, then use it everywhere in the code:
> 
> $ git grep "xfeatures_mask_all & ~xfeatures_mask_user_dynamic"
> arch/x86/kernel/fpu/core.c:239: dst_fpu->state_mask = xfeatures_mask_all & ~xfeatures_mask_user_dynamic;
> arch/x86/kernel/fpu/xstate.c:148:       else if (mask == (xfeatures_mask_all & ~xfeatures_mask_user_dynamic))
> arch/x86/kernel/fpu/xstate.c:932:       current->thread.fpu.state_mask = (xfeatures_mask_all & ~xfeatures_mask_user_dynamic);
> 
> and if you do that, do that in a separate pre-patch which does only this
> conversion.

I think they are in a different context.

The helper indicates the mask for the ‘init_fpstate’ buffer. The rest is the
initial mask value for the per-task xstate buffer.

Since you suggested to introduce get_xstate_buffer_attr(), how about replacing
what you found with something like:

get_xstate_buffer_attr(XSTATE_INIT_MASK)

>> +
>> extern void fpstate_init(struct fpu *fpu);
>> #ifdef CONFIG_MATH_EMULATION
>> extern void fpstate_init_soft(struct swregs_state *soft);
>> @@ -269,12 +281,12 @@ static inline void copy_fxregs_to_kernel(struct fpu *fpu)
>> 		     : "memory")
>> 
>> /*
>> - * This function is called only during boot time when x86 caps are not set
>> - * up and alternative can not be used yet.
>> + * Use this function to dump the initial state, only during boot time when x86
>> + * caps not set up and alternative not available yet.
>>  */
> 
> What's the point of this change? Also, "dump"?!

Yeah, right now, I don’t see the change is really necessary here. Sorry.

>> -	memset(state, 0, fpu_kernel_xstate_min_size);
>> +	memset(state, 0, fpu ? get_xstate_size(fpu->state_mask) : get_init_fpstate_size());
>> 
>> 	if (static_cpu_has(X86_FEATURE_XSAVES))
>> -		fpstate_init_xstate(&state->xsave, xfeatures_mask_all);
>> +		fpstate_init_xstate(&state->xsave, fpu ? fpu->state_mask : get_init_fpstate_mask());
> 
> <---- newline here.

Okay. Will do that.

Thanks,
Chang