Re: [PATCH v10 04/28] x86/fpu/xstate: Modify address finders to handle both static and dynamic buffers

From: Thomas Gleixner <tglx@linutronix.de>
To: "Chang S. Bae" <chang.seok.bae@intel.com>,
	bp@suse.de, luto@kernel.org, mingo@kernel.org, x86@kernel.org
Cc: len.brown@intel.com, lenb@kernel.org, dave.hansen@intel.com,
	thiago.macieira@intel.com, jing2.liu@intel.com,
	ravi.v.shankar@intel.com, linux-kernel@vger.kernel.org,
	chang.seok.bae@intel.com, kvm@vger.kernel.org
Subject: Re: [PATCH v10 04/28] x86/fpu/xstate: Modify address finders to handle both static and dynamic buffers
Date: Fri, 01 Oct 2021 15:15:22 +0200	[thread overview]
Message-ID: <87ee946g45.ffs@tglx> (raw)
In-Reply-To: <20210825155413.19673-5-chang.seok.bae@intel.com>

Chang,

On Wed, Aug 25 2021 at 08:53, Chang S. Bae wrote:

> Have all the functions finding XSTATE address take a struct fpu * pointer
> in preparation for dynamic state buffer support.
>
> init_fpstate is a special case, which is indicated by a null pointer
> parameter to get_xsave_addr() and __raw_xsave_addr().

Same comment vs. subject. Prepare ...

> +	if (fpu)
> +		xsave = &fpu->state.xsave;
> +	else
> +		xsave = &init_fpstate.xsave;
> +
> +	return xsave + xstate_comp_offsets[xfeature_nr];

So you have the same conditionals and the same comments vs. that NULL
pointer oddity how many times now all over the place?

That can be completely avoided:

Patch 1:

-union fpregs_state init_fpstate __ro_after_init;
+static union fpregs_state init_fpstate __ro_after_init;
+struct fpu init_fpu = { .state = &init_fpstate } __ro_after_init;

and make all users of init_fpstate access it through init_fpu.

Patches 2..N which change arguments from fpregs_state to fpu:

-	fun(init_fpu->state);
+	fun(&init_fpu);

Patch M which adds state_mask:

@fpu__init_system_xstate()
+	init_fpu.state_mask = xfeatures_mask_all;

Hmm?

Thanks,

        tglx