Re: [PATCH v10 06/28] x86/fpu/xstate: Add new variables to indicate dynamic XSTATE buffer size

[Bae, Chang Seok]

On Oct 1, 2021, at 06:32, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> On Wed, Aug 25 2021 at 08:53, Chang S. Bae wrote:
>> +/**
>> + * struct fpu_xstate_buffer_config - xstate buffer configuration
>> + * @max_size:			The CPUID-enumerated all-feature "maximum" size
>> + *				for xstate per-task buffer.
>> + * @min_size:			The size to fit into the statically-allocated
>> + *				buffer. With dynamic states, this buffer no longer
>> + *				contains all the enabled state components.
>> + * @user_size:			The size of user-space buffer for signal and
>> + *				ptrace frames, in the non-compacted format.
>> + */
> 
>> void fpstate_init(struct fpu *fpu)
>> {
>> 	union fpregs_state *state;
>> +	unsigned int size;
>> +	u64 mask;
>> 
>> -	if (likely(fpu))
>> +	if (likely(fpu)) {
>> 		state = &fpu->state;
>> -	else
>> +		/* The dynamic user states are not prepared yet. */
>> +		mask = xfeatures_mask_all & ~xfeatures_mask_user_dynamic;
> 
> The patch ordering is really odd. Why aren't you adding
> 
>     fpu->state_mask
> and
>     fpu->state_size
> 
> first and initialize state_mask and state_size to the fixed mode and
> then add the dynamic sizing on top?

I once considered the need of referencing fpu->state_size is not that much. At
runtime, the buffer re-allocation needs to calculate it. Then fpstate_init()
and the below are the ones that reference the size.

Maybe I was too conservative in adding this. I’m going to follow your
suggestion in a new patch.

>> 	/*
>> 	 * If the target FPU state is not resident in the CPU registers, just
>> 	 * memcpy() from current, else save CPU state directly to the target.
>> +	 *
>> +	 * KVM does not support dynamic user states yet. Assume the buffer
>> +	 * always has the minimum size.
>> 	 */
>> 	if (test_thread_flag(TIF_NEED_FPU_LOAD))
>> 		memcpy(&fpu->state, &current->thread.fpu.state,
>> -		       fpu_kernel_xstate_size);
>> +		       fpu_buf_cfg.min_size);
> 
> Which completely avoids the export of fpu_buf_cfg for KVM because the
> information is just available via struc fpu. As a bonus the export of
> fpu_kernel_xstate_size can be removed as well.
> 
> Hmm?

Yes, it is. But I suspect it needs to reference size values. KVM’s fpu data
can be allocated with the min size, instead of sizeof(struct fpu). KVM code
might want to know the max to support dynamic features, as similar to [1] or
so.

Thanks,
Chang

[1] https://lore.kernel.org/kvm/20210207154256.52850-4-jing2.liu@xxxxxxxxxxxxxxx/