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

[Ingo Molnar]

* Dave Hansen <dave@xxxxxxxx> 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?

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?

If 'compacted' format means "binary blob only the CPU can decode, not the kernel" 
then our answer is "uhm, no, thank you, we'll use standard format instead" ...

And no, "it's not Intel architectural" is a stupid and somewhat circular argument 
IMHO: the kernel always knew how to decompose CPU context dumps and you'll have to 
come up with a damn better reason to break that than pointing at some text in an 
Intel document.

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

So the context layout and structure on such CPUs has to be mapped and properly 
taken into account in the size calculation. How can GDB or any other (kernel) 
debugger display (and change) individual fields reliably if the layout is not 
known?

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

Then it has to be taken it into account - just like user-space has to take it into 
account if it wants to display (and change) individual fields...

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

This will have to be fixed before we move to compacted format.

> 4. It makes the code smaller and simpler, especially if you consider
>    what would happen if we added "real" alignment support.

What would happen?

Thanks,

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