Re: [PATCH] x86, fpu: correct XSAVE xstate size calculation

From: Ingo Molnar <mingo@kernel.org>
To: Dave Hansen <dave@sr71.net>
Cc: dave.hansen@linux.intel.com, linux-kernel@vger.kernel.org,
	bp@alien8.de, fenghua.yu@intel.com, hpa@zytor.com,
	x86@kernel.org, Thomas Gleixner <tglx@linutronix.de>,
	Peter Zijlstra <a.p.zijlstra@chello.nl>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Andy Lutomirski <luto@kernel.org>,
	Denys Vlasenko <dvlasenk@redhat.com>
Subject: Re: [PATCH] x86, fpu: correct XSAVE xstate size calculation
Date: Sat, 8 Aug 2015 11:06:15 +0200	[thread overview]
Message-ID: <20150808090615.GA32641@gmail.com> (raw)
In-Reply-To: <55C39730.8060602@sr71.net>

* Dave Hansen <dave@sr71.net> wrote:

> Just to be clear: the current code is OK and correct for non-compacted
> buffers.  Since we currently disable the compacted buffers, this patch
> has no effect on current kernels.

Absolutely, this was my assumption as well.

> This patch fixes the (currently unused) calculation for sizing the
> compacted-format buffer.  I can either send it now, or try to make sure
> it gets picked up by whoever goes back and re-implents
> XSAVES/compact-format support.
> 
> On 08/06/2015 12:15 AM, Ingo Molnar wrote:
> > * Dave Hansen <dave@sr71.net> wrote:
> >>> I realize that the calculation and what CPUID gives us should match, but it's 
> >>> not really good for the kernel to not know the precise layout of a critical 
> >>> task context data structure ...
> >>
> >> There is no architectural guarantee that the sum of xstate sizes will be the 
> >> same as what comes out of that CPUID leaf.  It would be nice, but it's not 
> >> architectural and I've run in to platforms where that assumption does not hold.
> > 
> > WHY?
> 
> From a real dmesg:
> 
> [    0.000000] x86/fpu: xstate_offset[2]: 0240, xstate_sizes[2]: 0100
> [    0.000000] x86/fpu: xstate_offset[3]: 03c0, xstate_sizes[3]: 0040
> [    0.000000] x86/fpu: xstate_offset[4]: 0400, xstate_sizes[4]: 0040
> ...
> 
> Note: 0x240 + 0x100 != 0x3c0.

This kind of alignment related offset padding is indeed harmless.

> 
> > What sense does it make to have a blob we don't know the exact layout of? How will 
> > debuggers or user-space in general be able to print (and change) the register 
> > values if they don't know the layout?
> 
> Ingo, we know the layout.  We know where every component is.  We know
> how big each component is.  This patch does not change the fact that we
> calculate and store that.

The patch you submitted blindly trusts the CPU, and I'm uneasy about that for 
multiple reasons. We can and should do better than that, while still flexibly 
making use of all CPU capabilities that are offered.

Thanks,

	Ingo