On Mon, May 09, 2016 at 01:45:58PM -0700, Yu-cheng Yu wrote:
> If "xsaves" is enabled, kernel always uses compacted format of xsave area.
> But user space still uses standard format of xsave area. Thus, xstate size
> in kernel's xsave area is smaller than xstate size in user's xsave area.
> The xstate in signal frame should be in standard format for user's signal
> handler to access.
>
> In no "xsaves" case, xsave area in both user space and kernel space are in
> standard format. Therefore, user's and kernel's xstate sizes are equal.
>
> In "xsaves" case, xsave area in user space is in standard format while
> xsave area in kernel space is in compacted format. Therefore, kernel's
> xstate size is smaller than user's xstate size.
So this repeats what the first paragraph said.
> So here is the problem: currently kernel assumes its own xstate size is
> signal frame's xstate size. This is not a problem in no "xsaves" case.
^
the
This whole text is missing a bunch of "the"s...
> It is an issue in "xsaves" case because kernel's xstate size is smaller
^ ^
the the
and so on...
> than user's xstate size. In fpu__alloc_mathframe(), a smaller fpstate
> buffer is allocated for the standard format xstate in signal frame.
> Then kernel saves only part of xstate registers into this smaller
> user's fpstate buffer and user will see part of the xstate registers in
> signal context. Similar issue happens after returning from signal handler:
> kernel will only restore part of xstate registers from user's fpstate
> buffer in signal frame.
>
> This patch defines and uses user_xstate_size for xstate size in signal
> frame. It's read from returned value in ebx from CPUID leaf 0x0D subleaf
> 0x0. This is maximum size required by enabled states in XCR0 and may be
> different from ecx when states at the end of the xsave area are not
> enabled. This value indicates the size required for XSAVE to save all
> supported user states in legacy/standard format.
>
> Signed-off-by: Fenghua Yu <[email protected]>
> Signed-off-by: Yu-cheng Yu <[email protected]>
That SOB chain needs clarification: if Fenghua is the author, the patch
should contain his From: at the top. If it is based on an earlier patch
from him, commit message should say:
Based on an earlier patch from Fenghua... without the SOB.
> Reviewed-by: Dave Hansen <[email protected]>
> ---
> arch/x86/include/asm/fpu/xstate.h | 1 -
> arch/x86/include/asm/processor.h | 1 +
> arch/x86/kernel/fpu/init.c | 5 ++-
> arch/x86/kernel/fpu/signal.c | 26 ++++++++++----
> arch/x86/kernel/fpu/xstate.c | 71
> ++++++++++++++++++++++++---------------
> 5 files changed, 67 insertions(+), 37 deletions(-)
>
> diff --git a/arch/x86/include/asm/fpu/xstate.h
> b/arch/x86/include/asm/fpu/xstate.h
> index 38951b0..16df2c4 100644
> --- a/arch/x86/include/asm/fpu/xstate.h
> +++ b/arch/x86/include/asm/fpu/xstate.h
> @@ -39,7 +39,6 @@
> #define REX_PREFIX
> #endif
>
> -extern unsigned int xstate_size;
> extern u64 xfeatures_mask;
> extern u64 xstate_fx_sw_bytes[USER_XSTATE_FX_SW_WORDS];
>
> diff --git a/arch/x86/include/asm/processor.h
> b/arch/x86/include/asm/processor.h
> index 9264476..132b4ca 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -368,6 +368,7 @@ DECLARE_PER_CPU(struct irq_stack *, softirq_stack);
> #endif /* X86_64 */
>
> extern unsigned int xstate_size;
> +extern unsigned int user_xstate_size;
If this is going to be exported, let's prefix it pls:
fpu_user_xstate_size
or
xstate_user_state_size
or somesuch.
And let's add a comment over its definition what exactly it represents.
I.e., the aspect about the signal frame...
...
> @@ -591,7 +593,15 @@ static bool is_supported_xstate_size(unsigned int
> test_xstate_size)
> static int init_xstate_size(void)
> {
> /* Recompute the context size for enabled features: */
> - unsigned int possible_xstate_size = calculate_xstate_size();
> + unsigned int possible_xstate_size;
> + unsigned int xsave_size;
> +
> + xsave_size = get_xsave_size();
> +
> + if (cpu_has_xsaves)
if (boot_cpu_has(X86_FEATURE_XSAVES))
> + possible_xstate_size = get_xsaves_size();
> + else
> + possible_xstate_size = xsave_size;
>
> /* Ensure we have the space to store all enabled: */
> if (!is_supported_xstate_size(possible_xstate_size))
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284
(AG Nürnberg)
--
