Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
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
In-Reply-To: <20210825155413.19673-5-chang.seok.bae@intel.com>
References: <20210825155413.19673-1-chang.seok.bae@intel.com>
 <20210825155413.19673-5-chang.seok.bae@intel.com>
Date:   Fri, 01 Oct 2021 15:15:22 +0200
Message-ID: <87ee946g45.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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