linux-kernel - Re: [patch 1/2] x86, fpu: split FPU state from task struct

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 11 Mar 2008 13:19:22 -0700
From:	Suresh Siddha <suresh.b.siddha@...el.com>
To:	Alexey Dobriyan <adobriyan@...il.com>
Cc:	Suresh Siddha <suresh.b.siddha@...el.com>, mingo@...e.hu,
	hpa@...or.com, tglx@...utronix.de, andi@...stfloor.org,
	hch@...radead.org, linux-kernel@...r.kernel.org,
	Arjan van de Ven <arjan@...ux.intel.com>
Subject: Re: [patch 1/2] x86, fpu: split FPU state from task struct - v5

On Tue, Mar 11, 2008 at 08:07:34AM +0300, Alexey Dobriyan wrote:
> On Mon, Mar 10, 2008 at 03:28:04PM -0700, Suresh Siddha wrote:
> > Split the FPU save area from the task struct. This allows easy migration
> > of FPU context, and it's generally cleaner. It also allows the following
> > two optimizations:
> > 
> > 1) only allocate when the application actually uses FPU, so in the first
> > lazy FPU trap. This could save memory for non-fpu using apps. Next patch
> > does this lazy allocation.
> > 
> > 2) allocate the right size for the actual cpu rather than 512 bytes always.
> > Patches enabling xsave/xrstor support (coming shortly) will take advantage
> > of this.
> 
> Ugh, not seeing patch, but judging from description it will make
> "choose wrong CONFIG_M* and fxsave will corrupt random FPU state" situation
> likely?

No. CONFIG_M* doesn't determine the size of the state. Feature information from
the 'cpuid' instruction will dictate the size allocated/used. Anyhow, please
wait for the xsave patches.

> 
> > --- linux-2.6-x86.orig/arch/x86/kernel/process_64.c
> > +++ linux-2.6-x86/arch/x86/kernel/process_64.c
> > @@ -634,7 +634,7 @@
> >  
> >  	/* we're going to use this soon, after a few expensive things */
> >  	if (next_p->fpu_counter>5)
> > -		prefetch(&next->i387.fxsave);
> > +		prefetch(next->xstate);
> 
> Can we please give it better name, like fpu_state? It's a member of
> task_struct after all.

It need not be only FPU. We can have non-math state here aswell.

selected 'xstate' for extended state. I am all open for any reasonable name,
reflecting math, extended math(fsave/fxsave/..) and future math/
non-math extensions.

> >  {
> >  	unsigned long oldcr0 = read_cr0();
> > -	extern void __bad_fxsave_alignment(void);
> > -
> > -	if (offsetof(struct task_struct, thread.i387.fxsave) & 15)
> > -		__bad_fxsave_alignment();
> 
> I think removal of such checks needs giving necessary alignment to cache.
> Previously it worked because of __aligned((16)) and L1_CACHE_SHIFT
> combo.

alignment is now specified as part of kmem_cache_create() and checed
in the allocation routines.

thanks,
suresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/