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

[Dave Hansen <dave@sr71.net>]

On 08/05/2015 03:32 AM, Ingo Molnar wrote:
> * Dave Hansen <dave@sr71.net> wrote:
>> From: Dave Hansen <dave.hansen@linux.intel.com>
>>
>> Note: our xsaves support is currently broken and disabled.  This
>> patch does not fix it, but it is an incremental improvement.  It
>> might be useful to someone backporting the entire set of XSAVES
>> patches at some point, but it should not be backported alone.
>>
>> There are currently two xsave buffer formats: standard and
>> compacted.  The standard format is waht 'XSAVE' and 'XSAVEOPT'
>> produce while 'XSAVES' and 'XSAVEC' produce a compacted-formet
>> buffer.  (The kernel never uses XSAVEC)
>>
>> But, the XSAVES buffer *ALSO* contains "system state components"
>> which are never saved by a plain XSAVE.  So, XSAVES has two
>> things that might make its buffer differently-sized from an
>> XSAVE-produced one.
>>
>> The current code assumes that an XSAVES buffer's size is simply
>> the sum of the sizes of the (user) states which are supported.
>> This seems to work in most cases, but it is not consistent with
>> what the SDM says, and it breaks if we 'align' a component in the
>> buffer.  The calculation is also unnecessary work since the CPU
>> *tells* us the size of the buffer directly.
>>
>> This patch just reads the size of the buffer right out of the
>> CPUID leaf instead of trying to derive it.
> 
> So how will we know where to find which field, if we cannot even do a size 
> calculation?

setup_xstate_features() still populates xstate_offsets[] which tells us
where to find each field.  This patch does not change that.

> 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.

> So can we turn this into 'double check the CPUID size and print a warning on 
> mismatch' kind of boot time sanity check? Preferably for all XSAVE* data formats 
> we can run into. I'd be fine with applying such a patch ahead of enabling 
> compaction again.

I don't think that is sufficient.

There are 4 reasons to apply this patch that I can think of:
1. There is no architectural guarantee that the calculation (sum of
   xstate sizes) will match what CPUID gives us as the size of the
   buffer.  I've seen this in practice.
2. The alignment bit indicates that there is space used in the buffer
   which is not part of a state component.  The current code does not
   take that in to account.
3. The code is currently asking for the size of an XSAVE-produced
   buffer.  The code will be wrong the moment we switch to XSAVES
   because XSAVES saves more things than XSAVE and uses more space.
4. It makes the code smaller and simpler, especially if you consider
   what would happen if we added "real" alignment support.