Skip to contentSkip to conversation

x86/xsaves: Fix XSAVES issues

PATCH Superseded Yu-cheng Yu 5 participants 24 messages May 10, 2016 – May 12, 2016 x86

v6 of 7 Superseded Latest is v7 →

lore
Share & tools
lore
#0 lore raw

v6 cover x86/xsaves: Fix XSAVES issues

XSAVES is a kernel-mode instruction. It offers a compacted format and memory-write optimization. These patches fix issues in the first implementation.

Changes since Version 5:

Patch 1, 2 - Change names to fpu_user_xstate_size and fpu_kernel_xstate_size;
fix some comments, etc.

Patch 11 - In addition to a warning, make __raw_xsave_addr() return NULL
when a disabled xstate component address is requested.

Patch 12 - Add more details in the comments of why this patch is needed;
make a new macro for xcomp_bv[63].

Yu-cheng Yu (13):
x86/xsaves: Define and use fpu_user_xstate_size x86/xsaves: Rename xstate_size to fpu_kernel_xstate_size to
distinguish from fpu_user_xstate_size
x86/xsaves: Keep init_fpstate.xsave.header.xfeatures as zero for init
optimization
x86/xsaves: Introduce a new check that allows correct xstates copy
from kernel to user directly
x86/xsaves: Align xstate components according to CPUID x86/xsaves: Supervisor state component offset
x86/xsaves: Fix PTRACE frames for XSAVES
x86/xsaves: Fix XSTATE component offset print out x86/xsaves: Fix xstate_offsets, xstate_sizes for non-extended states x86/xsaves: Fix __fpu_restore_sig() for XSAVES
x86/xsaves: When a disabled xstate component offset is requested,
return NULL
x86/xsaves: Fix fpstate_init() for XRSTORS
x86/xsaves: Re-enable XSAVES

 arch/x86/include/asm/fpu/types.h  |   7 +
 arch/x86/include/asm/fpu/xstate.h |  10 +-
 arch/x86/include/asm/processor.h  |   3 +-
 arch/x86/kernel/fpu/core.c        |  11 +-
 arch/x86/kernel/fpu/init.c        |  34 ++--
 arch/x86/kernel/fpu/regset.c      |  56 +++--
 arch/x86/kernel/fpu/signal.c      |  43 +++-
 arch/x86/kernel/fpu/xstate.c      | 417 ++++++++++++++++++++++++++++++--------
 8 files changed, 440 insertions(+), 141 deletions(-)
Signature · 1 line
1.9.1
#1 lore raw

PATCH v6 1/13 x86/xsaves: Define and use fpu_user_xstate_size

The XSAVE area of kernel can be in standard or compacted format; it is always in standard format for user mode. When XSAVES is enabled, the kernel uses the compacted format and it is necessary to use a separate fpu_user_xstate_size for signal/ptrace frames.

Based on an earlier patch from Fenghua Yu <[email protected]>

[[email protected]: rebase to current, rename to fpu_user_xstate_size]
---
 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      | 27 ++++++++++----
 arch/x86/kernel/fpu/xstate.c      | 76 ++++++++++++++++++++++++---------------
 5 files changed, 73 insertions(+), 37 deletions(-)
arch/x86/include/asm/fpu/xstate.h +0 -1
index 38951b0..16df2c4 100644
@@ -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];
 
arch/x86/include/asm/processor.h +1 -0
index 9264476..8d5df3f 100644
@@ -368,6 +368,7 @@ DECLARE_PER_CPU(struct irq_stack *, softirq_stack);
 #endif	/* X86_64 */
 
 extern unsigned int xstate_size;
+extern unsigned int fpu_user_xstate_size;
 
 struct perf_event;
 
arch/x86/kernel/fpu/init.c +4 -1
index 54c86ff..824d422 100644
@@ -195,7 +195,7 @@ static void __init fpu__init_task_struct_size(void)
 }
 
 /*
- * Set up the xstate_size based on the legacy FPU context size.
+ * Set up the user and kernel xstate_size based on the legacy FPU context size.
  *
  * We set this up first, and later it will be overwritten by
  * fpu__init_system_xstate() if the CPU knows about xstates.
@@ -226,6 +226,9 @@ static void __init fpu__init_system_xstate_size_legacy(void)
 		else
 			xstate_size = sizeof(struct fregs_state);
 	}
+
+	fpu_user_xstate_size = xstate_size;
+
 	/*
 	 * Quirk: we don't yet handle the XSAVES* instructions
 	 * correctly, as we don't correctly convert between
arch/x86/kernel/fpu/signal.c +20 -7
index 31c6a60..23572a9 100644
@@ -31,7 +31,7 @@ static inline int check_for_xstate(struct fxregs_state __user *buf,
 	/* Check for the first magic field and other error scenarios. */
 	if (fx_sw->magic1 != FP_XSTATE_MAGIC1 ||
 	    fx_sw->xstate_size < min_xstate_size ||
-	    fx_sw->xstate_size > xstate_size ||
+	    fx_sw->xstate_size > fpu_user_xstate_size ||
 	    fx_sw->xstate_size > fx_sw->extended_size)
 		return -1;
 
@@ -88,7 +88,8 @@ static inline int save_xstate_epilog(void __user *buf, int ia32_frame)
 	if (!use_xsave())
 		return err;
 
-	err |= __put_user(FP_XSTATE_MAGIC2, (__u32 *)(buf + xstate_size));
+	err |= __put_user(FP_XSTATE_MAGIC2,
+			  (__u32 *)(buf + fpu_user_xstate_size));
 
 	/*
 	 * Read the xfeatures which we copied (directly from the cpu or
@@ -125,7 +126,7 @@ static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf)
 	else
 		err = copy_fregs_to_user((struct fregs_state __user *) buf);
 
-	if (unlikely(err) && __clear_user(buf, xstate_size))
+	if (unlikely(err) && __clear_user(buf, fpu_user_xstate_size))
 		err = -EFAULT;
 	return err;
 }
@@ -175,8 +176,19 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
 		if (ia32_fxstate)
 			copy_fxregs_to_kernel(&tsk->thread.fpu);
 	} else {
+		/*
+		 * It is a *bug* if kernel uses compacted-format for xsave
+		 * area and we copy it out directly to a signal frame. It
+		 * should have been handled above by saving the registers
+		 * directly.
+		 */
+		if (boot_cpu_has(X86_FEATURE_XSAVES)) {
+			WARN_ONCE(1, "x86/fpu: saving compacted-format xsave area to a signal frame!\n");
+			return -1;
+		}
+
 		fpstate_sanitize_xstate(&tsk->thread.fpu);
-		if (__copy_to_user(buf_fx, xsave, xstate_size))
+		if (__copy_to_user(buf_fx, xsave, fpu_user_xstate_size))
 			return -1;
 	}
 
@@ -341,7 +353,8 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 
 static inline int xstate_sigframe_size(void)
 {
-	return use_xsave() ? xstate_size + FP_XSTATE_MAGIC2_SIZE : xstate_size;
+	return use_xsave() ? fpu_user_xstate_size + FP_XSTATE_MAGIC2_SIZE :
+			fpu_user_xstate_size;
 }
 
 /*
@@ -385,12 +398,12 @@ fpu__alloc_mathframe(unsigned long sp, int ia32_frame,
  */
 void fpu__init_prepare_fx_sw_frame(void)
 {
-	int size = xstate_size + FP_XSTATE_MAGIC2_SIZE;
+	int size = fpu_user_xstate_size + FP_XSTATE_MAGIC2_SIZE;
 
 	fx_sw_reserved.magic1 = FP_XSTATE_MAGIC1;
 	fx_sw_reserved.extended_size = size;
 	fx_sw_reserved.xfeatures = xfeatures_mask;
-	fx_sw_reserved.xstate_size = xstate_size;
+	fx_sw_reserved.xstate_size = fpu_user_xstate_size;
 
 	if (config_enabled(CONFIG_IA32_EMULATION) ||
 	    config_enabled(CONFIG_X86_32)) {
arch/x86/kernel/fpu/xstate.c +48 -28

116 lines not on this page · raw message

Signature · 1 line
1.9.1
#2 lore raw

Re: [PATCH v6 01/13] x86/xsaves: Define and use fpu_user_xstate_size

Quoted 1 line · Yu-cheng Yu

The XSAVE area of kernel can be in standard or compacted format;

"The kernel xstate area... "

and can we call it the xstate area as there are a bunch of XSAVE* insns touching it. The file which deals with it is even called that:
arch/x86/kernel/fpu/xstate.c

Quoted 10 lines

it is always in standard format for user mode. When XSAVES is enabled, the kernel uses the compacted format and it is necessary to use a separate fpu_user_xstate_size for signal/ptrace frames.

Based on an earlier patch from Fenghua Yu <[email protected]>

[[email protected]: rebase to current, rename to fpu_user_xstate_size]

Maybe I wasn't as clear as I hoped to be. Let me be more specific:

So you either need to do:

---
From: Fenghua

...

Signed-off-by: Fenghua
Signed-off-by: You
...
---

or

---

Based on an earlier patch from Fenghua Yu <[email protected]>.

Signed-off-by You

---

with the second variant making you the author implicitly because you're the sender.

Makes more sense this way?

Quoted 7 lines
---
 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      | 27 ++++++++++----
 arch/x86/kernel/fpu/xstate.c      | 76 ++++++++++++++++++++++++---------------
 5 files changed, 73 insertions(+), 37 deletions(-)

...

arch/x86/kernel/fpu/xstate.c static unsigned int xstate_sizes[XFEATURE_MAX] = { [ 0 ... XFEATURE_MAX - 1] = · line 47

Quoted 9 lines
arch/x86/kernel/fpu/xstate.c +1 -0
index b48ef35..dfac87d 100644
@@ -44,6 +44,13 @@ static unsigned int xstate_sizes[XFEATURE_MAX]   = { [ 0 ... XFEATURE_MAX - 1] =
 static unsigned int xstate_comp_offsets[sizeof(xfeatures_mask)*8];
 
 /*
+ * The XSAVE area of kernel can be in standard or compacted format;

"The kernel xstate area... "

arch/x86/kernel/fpu/xstate.c static unsigned int xstate_sizes[XFEATURE_MAX] = { [ 0 ... XFEATURE_MAX - 1] = · line 50

Quoted 3 lines
arch/x86/kernel/fpu/xstate.c +3 -0
+ * it is always in standard format for user mode. This is the user
+ * mode standard format size used for signal and ptrace frames.
+ */

But yes, that's very nice commenting.

Quoted 6 lines
arch/x86/kernel/fpu/xstate.c +3 -0
+unsigned int fpu_user_xstate_size;
+
+/*
  * Clear all of the X86_FEATURE_* bits that are unavailable
  * when the CPU has no XSAVE support.
  */

...

arch/x86/kernel/fpu/xstate.c static bool is_supported_xstate_size(unsigned int test_xstate_size) · line 606

Quoted 11 lines
arch/x86/kernel/fpu/xstate.c +6 -1
@@ -591,7 +598,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)

So next time you submit patches against -tip, please merge them with tip/master and see if they still build, at least.

During last review I told you to use boot_cpu_has(X86_FEATURE_XSAVES) because those cpu_has_XXX things are going away and are gone in tip already.

Please be more careful when incorporating review comments - otherwise it is waste of both yours and reviewer's time.

Thanks.

Signature · 5 lines
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 
#3 lore raw

RE: [PATCH v6 01/13] x86/xsaves: Define and use fpu_user_xstate_size

Quoted 53 lines

From: Borislav Petkov [mailto:[email protected]]
Sent: Wednesday, May 11, 2016 10:21 AM
To: Yu, Yu-cheng <[email protected]>
Cc: [email protected]; [email protected]; H. Peter Anvin <[email protected]>; Thomas Gleixner <[email protected]>; Ingo Molnar <[email protected]>; Dave Hansen <[email protected]>; Andy Lutomirski <[email protected]>; Prakhya, Sai Praneeth <[email protected]>; Shankar, Ravi V <[email protected]>; Yu, Fenghua <[email protected]>
Subject: Re: [PATCH v6 01/13] x86/xsaves: Define and use fpu_user_xstate_size

Quoted 1 line · Yu-cheng Yu

The XSAVE area of kernel can be in standard or compacted format;

"The kernel xstate area... "

and can we call it the xstate area as there are a bunch of XSAVE* insns touching it. The file which deals with it is even called that:
arch/x86/kernel/fpu/xstate.c

Quoted 11 lines

it is always in standard format for user mode. When XSAVES is enabled, the kernel uses the compacted format and it is necessary to use a separate fpu_user_xstate_size for signal/ptrace frames.

Based on an earlier patch from Fenghua Yu <[email protected]>

[[email protected]: rebase to current, rename to fpu_user_xstate_size]

Maybe I wasn't as clear as I hoped to be. Let me be more specific:

So you either need to do:

---
From: Fenghua

...

Signed-off-by: Fenghua
Signed-off-by: You
...
---

or

---

Based on an earlier patch from Fenghua Yu <[email protected]>.

Quoted 8 lines
Signed-off-by You

---

with the second variant making you the author implicitly because you're the sender.

Makes more sense this way?

Is this possible to have the third one?

From: Yu-cheng

Signed-off-by Yu-cheng
Signed-off-by Fenghua

Thanks.

-Fenghua

#4 lore raw

Re: [PATCH v6 01/13] x86/xsaves: Define and use fpu_user_xstate_size

Quoted 6 lines · Fenghua

Is this possible to have the third one?

From: Yu-cheng

Signed-off-by Yu-cheng
Signed-off-by Fenghua

This SOB chain says that Yu-cheng is the author and then you've received it from him, have done something to it (which we sometimes put in [] to denote it) and then you've sent it out.

The tip maintainer which applies it will add his SOB too, at the end.

Let me ask you this: what exactly do you want to express with the SOB chain?

Just a reminder: the SOB chain should give a clear and precise representation of the patch path from author to upstream.

Section

11) Sign your work
------------------

in Documentation/SubmittingPatches explains the whole story.

Signature · 5 lines
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 
#5 lore raw

Re: [PATCH v6 01/13] x86/xsaves: Define and use fpu_user_xstate_size

In reply to Yu, Fenghua
Quoted 6 lines · Yu, Fenghua

Is this possible to have the third one?

From: Yu-cheng

Signed-off-by Yu-cheng
Signed-off-by Fenghua

Yeah, that's a valid SOB chain if Yu-cheng wrote much of the patch and you just forwarded it along with some fixes/enhancements. As Boris mentioned, you can further improve on this via:

Quoted 3 lines
Signed-off-by Yu-cheng
[ Fixed bug X, cleaned up Y. ]
Signed-off-by Fenghua

to signal that you made changes over Yu-cheng's version.

Thanks,

Ingo

#6 lore raw

PATCH v6 2/13 x86/xsaves: Rename xstate_size to fpu_kernel_xstate_size to distinguish from fpu_user_xstate_size

In reply to Yu-cheng Yu

User space uses standard format xsave area. fpstate in signal frame should have standard format size.

To explicitly distinguish between xstate size in kernel space and the one in user space, we rename xstate_size to fpu_kernel_xstate_size. This patch is not fixing a bug. It just makes kernel code more clear.

[[email protected]: rebase to current, rename to fpu_kernel_xstate_size]
---
 arch/x86/include/asm/processor.h |  2 +-
 arch/x86/kernel/fpu/core.c       |  7 ++++---
 arch/x86/kernel/fpu/init.c       | 20 +++++++++++---------
 arch/x86/kernel/fpu/signal.c     |  2 +-
 arch/x86/kernel/fpu/xstate.c     |  8 ++++----
 5 files changed, 21 insertions(+), 18 deletions(-)
arch/x86/include/asm/processor.h +1 -1
index 8d5df3f..5fd59c6 100644
@@ -367,7 +367,7 @@ DECLARE_PER_CPU(struct irq_stack *, hardirq_stack);
 DECLARE_PER_CPU(struct irq_stack *, softirq_stack);
 #endif	/* X86_64 */
 
-extern unsigned int xstate_size;
+extern unsigned int fpu_kernel_xstate_size;
 extern unsigned int fpu_user_xstate_size;
 
 struct perf_event;
arch/x86/kernel/fpu/core.c +4 -3
index 8e37cc8..dfd29ac 100644
@@ -222,7 +222,7 @@ void fpstate_init(union fpregs_state *state)
 		return;
 	}
 
-	memset(state, 0, xstate_size);
+	memset(state, 0, fpu_kernel_xstate_size);
 
 	if (cpu_has_fxsr)
 		fpstate_init_fxstate(&state->fxsave);
@@ -247,7 +247,7 @@ int fpu__copy(struct fpu *dst_fpu, struct fpu *src_fpu)
 	 * leak into the child task:
 	 */
 	if (use_eager_fpu())
-		memset(&dst_fpu->state.xsave, 0, xstate_size);
+		memset(&dst_fpu->state.xsave, 0, fpu_kernel_xstate_size);
 
 	/*
 	 * Save current FPU registers directly into the child
@@ -266,7 +266,8 @@ int fpu__copy(struct fpu *dst_fpu, struct fpu *src_fpu)
 	 */
 	preempt_disable();
 	if (!copy_fpregs_to_fpstate(dst_fpu)) {
-		memcpy(&src_fpu->state, &dst_fpu->state, xstate_size);
+		memcpy(&src_fpu->state, &dst_fpu->state,
+		       fpu_kernel_xstate_size);
 
 		if (use_eager_fpu())
 			copy_kernel_to_fpregs(&src_fpu->state);
arch/x86/kernel/fpu/init.c +11 -9
index 824d422..eccc36b 100644
@@ -145,8 +145,8 @@ static void __init fpu__init_system_generic(void)
  * This is inherent to the XSAVE architecture which puts all state
  * components into a single, continuous memory block:
  */
-unsigned int xstate_size;
-EXPORT_SYMBOL_GPL(xstate_size);
+unsigned int fpu_kernel_xstate_size;
+EXPORT_SYMBOL_GPL(fpu_kernel_xstate_size);
 
 /* Get alignment of the TYPE. */
 #define TYPE_ALIGN(TYPE) offsetof(struct { char x; TYPE test; }, test)
@@ -178,7 +178,7 @@ static void __init fpu__init_task_struct_size(void)
 	 * Add back the dynamically-calculated register state
 	 * size.
 	 */
-	task_size += xstate_size;
+	task_size += fpu_kernel_xstate_size;
 
 	/*
 	 * We dynamically size 'struct fpu', so we require that
@@ -195,7 +195,7 @@ static void __init fpu__init_task_struct_size(void)
 }
 
 /*
- * Set up the user and kernel xstate_size based on the legacy FPU context size.
+ * Set up the user and kernel xstate sizes based on the legacy FPU context size.
  *
  * We set this up first, and later it will be overwritten by
  * fpu__init_system_xstate() if the CPU knows about xstates.
@@ -208,7 +208,7 @@ static void __init fpu__init_system_xstate_size_legacy(void)
 	on_boot_cpu = 0;
 
 	/*
-	 * Note that xstate_size might be overwriten later during
+	 * Note that xstate sizes might be overwritten later during
 	 * fpu__init_system_xstate().
 	 */
 
@@ -219,15 +219,17 @@ static void __init fpu__init_system_xstate_size_legacy(void)
 		 */
 		setup_clear_cpu_cap(X86_FEATURE_XSAVE);
 		setup_clear_cpu_cap(X86_FEATURE_XSAVEOPT);
-		xstate_size = sizeof(struct swregs_state);
+		fpu_kernel_xstate_size = sizeof(struct swregs_state);
 	} else {
 		if (cpu_has_fxsr)
-			xstate_size = sizeof(struct fxregs_state);
+			fpu_kernel_xstate_size =
+				sizeof(struct fxregs_state);
 		else
-			xstate_size = sizeof(struct fregs_state);
+			fpu_kernel_xstate_size =
+				sizeof(struct fregs_state);
 	}
 
-	fpu_user_xstate_size = xstate_size;
+	fpu_user_xstate_size = fpu_kernel_xstate_size;
 
 	/*
 	 * Quirk: we don't yet handle the XSAVES* instructions
arch/x86/kernel/fpu/signal.c +1 -1
index 23572a9..1454989 100644
@@ -262,7 +262,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 	int ia32_fxstate = (buf != buf_fx);
 	struct task_struct *tsk = current;
 	struct fpu *fpu = &tsk->thread.fpu;
-	int state_size = xstate_size;
+	int state_size = fpu_kernel_xstate_size;
 	u64 xfeatures = 0;
 	int fx_only = 0;
 
arch/x86/kernel/fpu/xstate.c +4 -4
index dfac87d..875afe9 100644
@@ -537,7 +537,7 @@ static void do_extra_xstate_size_checks(void)
 		 */
 		paranoid_xstate_size += xfeature_size(i);
 	}
-	XSTATE_WARN_ON(paranoid_xstate_size != xstate_size);
+	XSTATE_WARN_ON(paranoid_xstate_size != fpu_kernel_xstate_size);
 }
 
 
@@ -616,7 +616,7 @@ static int init_xstate_size(void)
 	 * The size is OK, we are definitely going to use xsave,
 	 * make it known to the world that we need more space.
 	 */
-	xstate_size = possible_xstate_size;
+	fpu_kernel_xstate_size = possible_xstate_size;
 	do_extra_xstate_size_checks();
 
 	/*
@@ -679,14 +679,14 @@ void __init fpu__init_system_xstate(void)
 		return;
 	}
 
-	update_regset_xstate_info(xstate_size, xfeatures_mask);
+	update_regset_xstate_info(fpu_kernel_xstate_size, xfeatures_mask);
 	fpu__init_prepare_fx_sw_frame();
 	setup_init_fpu_buf();
 	setup_xstate_comp();
 
 	pr_info("x86/fpu: Enabled xstate features 0x%llx, context size is %d bytes, using '%s' format.\n",
 		xfeatures_mask,
-		xstate_size,
+		fpu_kernel_xstate_size,
 		cpu_has_xsaves ? "compacted" : "standard");
 }
 
Signature · 1 line
1.9.1
#7 lore raw

PATCH v6 3/13 x86/xsaves: Keep init_fpstate.xsave.header.xfeatures as zero for init optimization

In reply to Yu-cheng Yu

Keep init_fpstate.xsave.header.xfeatures as zero for init optimization. This is important for init optimization that is implemented in processor. If a bit corresponding to an xstate in xstate_bv is 0, it means the xstate is in init status and will not be read from memory to the processor during XRSTOR/XRSTORS instruction. This largely impacts context switch performance.

---
 arch/x86/kernel/fpu/xstate.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
arch/x86/kernel/fpu/xstate.c +2 -4
index 875afe9..5239d4d 100644
@@ -329,13 +329,11 @@ static void __init setup_init_fpu_buf(void)
 	setup_xstate_features();
 	print_xstate_features();
 
-	if (cpu_has_xsaves) {
+	if (cpu_has_xsaves)
 		init_fpstate.xsave.header.xcomp_bv = (u64)1 << 63 | xfeatures_mask;
-		init_fpstate.xsave.header.xfeatures = xfeatures_mask;
-	}
 
 	/*
-	 * Init all the features state with header_bv being 0x0
+	 * Init all the features state with header.xfeatures being 0x0
 	 */
 	copy_kernel_to_xregs_booting(&init_fpstate.xsave);
 
Signature · 1 line
1.9.1
#8 lore raw

PATCH v6 4/13 x86/xsaves: Introduce a new check that allows correct xstates copy from kernel to user directly

In reply to Yu-cheng Yu

XSAVES is a kernel instruction and uses a compacted format. When working with user space, the kernel should provide standard-format, non-supervisor state data. We cannot do __copy_to_user() from a compacted- format kernel xstate area to a signal frame.

Dave Hansen proposes this method to simplify copy xstate directly to user.

Signed-off by: Yu-cheng Yu <[email protected]>

---
 arch/x86/include/asm/fpu/xstate.h | 1 +
 arch/x86/kernel/fpu/signal.c      | 3 ++-
 arch/x86/kernel/fpu/xstate.c      | 2 +-
 3 files changed, 4 insertions(+), 2 deletions(-)
arch/x86/include/asm/fpu/xstate.h +1 -0
index 16df2c4..d812cf3 100644
@@ -47,5 +47,6 @@ extern void update_regset_xstate_info(unsigned int size, u64 xstate_mask);
 void fpu__xstate_clear_all_cpu_caps(void);
 void *get_xsave_addr(struct xregs_state *xsave, int xstate);
 const void *get_xsave_field_ptr(int xstate_field);
+int using_compacted_format(void);
 
 #endif
arch/x86/kernel/fpu/signal.c +2 -1
index 1454989..9ce2963 100644
@@ -8,6 +8,7 @@
 #include <asm/fpu/internal.h>
 #include <asm/fpu/signal.h>
 #include <asm/fpu/regset.h>
+#include <asm/fpu/xstate.h>
 
 #include <asm/sigframe.h>
 
@@ -168,7 +169,7 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
 			sizeof(struct user_i387_ia32_struct), NULL,
 			(struct _fpstate_32 __user *) buf) ? -1 : 1;
 
-	if (fpregs_active()) {
+	if (fpregs_active() || using_compacted_format()) {
 		/* Save the live register state to the user directly. */
 		if (copy_fpregs_to_sigframe(buf_fx))
 			return -1;
arch/x86/kernel/fpu/xstate.c +1 -1
index 5239d4d..e00ec6a 100644
@@ -420,7 +420,7 @@ static int xfeature_size(int xfeature_nr)
  * that it is obvious which aspect of 'XSAVES' is being handled
  * by the calling code.
  */
-static int using_compacted_format(void)
+int using_compacted_format(void)
 {
 	return cpu_has_xsaves;
 }
Signature · 1 line
1.9.1
#9 lore raw

PATCH v6 5/13 x86/xsaves: Align xstate components according to CPUID

In reply to Yu-cheng Yu

CPUID function 0x0d, sub function (i, i > 1) returns in ecx[1] the alignment requirement of component i when the compacted format is used.

If ecx[1] is 0, component i is located immediately following the preceding component. If ecx[1] is 1, component i is located on the next 64-byte boundary following the preceding component.

---
 arch/x86/kernel/fpu/xstate.c | 60 +++++++++++++++++++++++---------------------
 1 file changed, 32 insertions(+), 28 deletions(-)
arch/x86/kernel/fpu/xstate.c +32 -28
index e00ec6a..afdeb55 100644
@@ -270,6 +270,33 @@ static void __init print_xstate_features(void)
 }
 
 /*
+ * This check is important because it is easy to get XSTATE_*
+ * confused with XSTATE_BIT_*.
+ */
+#define CHECK_XFEATURE(nr) do {		\
+	WARN_ON(nr < FIRST_EXTENDED_XFEATURE);	\
+	WARN_ON(nr >= XFEATURE_MAX);	\
+} while (0)
+
+/*
+ * We could cache this like xstate_size[], but we only use
+ * it here, so it would be a waste of space.
+ */
+static int xfeature_is_aligned(int xfeature_nr)
+{
+	u32 eax, ebx, ecx, edx;
+
+	CHECK_XFEATURE(xfeature_nr);
+	cpuid_count(XSTATE_CPUID, xfeature_nr, &eax, &ebx, &ecx, &edx);
+	/*
+	 * The value returned by ECX[1] indicates the alignment
+	 * of state component i when the compacted format
+	 * of the extended region of an XSAVE area is used
+	 */
+	return !!(ecx & 2);
+}
+
+/*
  * This function sets up offsets and sizes of all extended states in
  * xsave area. This supports both standard format and compacted format
  * of the xsave aread.
@@ -306,10 +333,14 @@ static void __init setup_xstate_comp(void)
 		else
 			xstate_comp_sizes[i] = 0;
 
-		if (i > FIRST_EXTENDED_XFEATURE)
+		if (i > FIRST_EXTENDED_XFEATURE) {
 			xstate_comp_offsets[i] = xstate_comp_offsets[i-1]
 					+ xstate_comp_sizes[i-1];
 
+			if (xfeature_is_aligned(i))
+				xstate_comp_offsets[i] =
+					ALIGN(xstate_comp_offsets[i], 64);
+		}
 	}
 }
 
@@ -366,33 +397,6 @@ static int xfeature_is_user(int xfeature_nr)
 }
 */
 
-/*
- * This check is important because it is easy to get XSTATE_*
- * confused with XSTATE_BIT_*.
- */
-#define CHECK_XFEATURE(nr) do {		\
-	WARN_ON(nr < FIRST_EXTENDED_XFEATURE);	\
-	WARN_ON(nr >= XFEATURE_MAX);	\
-} while (0)
-
-/*
- * We could cache this like xstate_size[], but we only use
- * it here, so it would be a waste of space.
- */
-static int xfeature_is_aligned(int xfeature_nr)
-{
-	u32 eax, ebx, ecx, edx;
-
-	CHECK_XFEATURE(xfeature_nr);
-	cpuid_count(XSTATE_CPUID, xfeature_nr, &eax, &ebx, &ecx, &edx);
-	/*
-	 * The value returned by ECX[1] indicates the alignment
-	 * of state component i when the compacted format
-	 * of the extended region of an XSAVE area is used
-	 */
-	return !!(ecx & 2);
-}
-
 static int xfeature_uncompacted_offset(int xfeature_nr)
 {
 	u32 eax, ebx, ecx, edx;
Signature · 1 line
1.9.1
#10 lore raw

PATCH v6 6/13 x86/xsaves: Supervisor state component offset

In reply to Yu-cheng Yu

CPUID function 0x0d, sub function (i, i > 1) returns in ebx the offset of xstate component i. Zero is returned for a supervisor state. A supervisor state can only be saved by XSAVES and XSAVES uses a compacted format. There is no fixed offset for a supervisor state. This patch checks and makes sure a supervisor state offset is not recorded or mis-used. This has no effect in practice as we currently use no supervisor states, but it would be good to fix.

---
 arch/x86/include/asm/fpu/types.h  |  1 +
 arch/x86/include/asm/fpu/xstate.h |  3 ++
 arch/x86/kernel/fpu/xstate.c      | 62 ++++++++++++++++++++++++---------------
 3 files changed, 43 insertions(+), 23 deletions(-)
arch/x86/include/asm/fpu/types.h +1 -0
index 36b90bb..12dd648 100644
@@ -122,6 +122,7 @@ enum xfeature {
 #define XFEATURE_MASK_OPMASK		(1 << XFEATURE_OPMASK)
 #define XFEATURE_MASK_ZMM_Hi256		(1 << XFEATURE_ZMM_Hi256)
 #define XFEATURE_MASK_Hi16_ZMM		(1 << XFEATURE_Hi16_ZMM)
+#define XFEATURE_MASK_PT		(1 << XFEATURE_PT_UNIMPLEMENTED_SO_FAR)
 #define XFEATURE_MASK_PKRU		(1 << XFEATURE_PKRU)
 
 #define XFEATURE_MASK_FPSSE		(XFEATURE_MASK_FP | XFEATURE_MASK_SSE)
arch/x86/include/asm/fpu/xstate.h +3 -0
index d812cf3..92f376c 100644
@@ -18,6 +18,9 @@
 #define XSAVE_YMM_SIZE	    256
 #define XSAVE_YMM_OFFSET    (XSAVE_HDR_SIZE + XSAVE_HDR_OFFSET)
 
+/* Supervisor features */
+#define XFEATURE_MASK_SUPERVISOR (XFEATURE_MASK_PT)
+
 /* Supported features which support lazy state saving */
 #define XFEATURE_MASK_LAZY	(XFEATURE_MASK_FP | \
 				 XFEATURE_MASK_SSE | \
arch/x86/kernel/fpu/xstate.c +39 -23

84 lines not on this page · raw message

Signature · 1 line
1.9.1
#11 lore raw

PATCH v6 7/13 x86/xsaves: Fix PTRACE frames for XSAVES

In reply to Yu-cheng Yu

XSAVES uses compacted format and is a kernel instruction. The kernel should use standard-format, non-supervisor state data for PTRACE.

---
 arch/x86/include/asm/fpu/xstate.h |   5 +-
 arch/x86/kernel/fpu/regset.c      |  56 +++++++++----
 arch/x86/kernel/fpu/xstate.c      | 167 +++++++++++++++++++++++++++++++++++++-
 3 files changed, 212 insertions(+), 16 deletions(-)
arch/x86/include/asm/fpu/xstate.h +4 -1
index 92f376c..ae55a43 100644
@@ -51,5 +51,8 @@ void fpu__xstate_clear_all_cpu_caps(void);
 void *get_xsave_addr(struct xregs_state *xsave, int xstate);
 const void *get_xsave_field_ptr(int xstate_field);
 int using_compacted_format(void);
-
+int copyout_from_xsaves(unsigned int pos, unsigned int count, void *kbuf,
+			void __user *ubuf, struct xregs_state *xsave);
+int copyin_to_xsaves(const void *kbuf, const void __user *ubuf,
+		     struct xregs_state *xsave);
 #endif
arch/x86/kernel/fpu/regset.c +42 -14

80 lines not on this page · raw message

arch/x86/kernel/fpu/xstate.c +166 -1

188 lines not on this page · raw message

Signature · 1 line
1.9.1
maintainer Reviewed-by on v6 7/13
#12 lore raw

Re: [PATCH v6 07/13] x86/xsaves: Fix PTRACE frames for XSAVES

Quoted 4 lines · Yu-cheng Yu

XSAVES uses compacted format and is a kernel instruction. The kernel should use standard-format, non-supervisor state data for PTRACE.

This looks fine. There are a few comments that could use some sprucing up, like:

+ /*
+ * Reject if the user sets any forbidden features.
+ */

But it's minor stuff.

#13 lore raw

Re: [PATCH v6 07/13] x86/xsaves: Fix PTRACE frames for XSAVES

Quoted 13 lines · Dave Hansen
Quoted 4 lines · Yu-cheng Yu

XSAVES uses compacted format and is a kernel instruction. The kernel should use standard-format, non-supervisor state data for PTRACE.

This looks fine. There are a few comments that could use some sprucing up, like:

arch/x86/kernel/fpu/xstate.c +3 -0
+	/*
+	 * Reject if the user sets any forbidden features.
+	 */
Quoted 3 lines

But it's minor stuff.

Well it's too late for v4.7 due to intrusiveness, so please refine the series and also incorporate the cleanups requested by Boris. I can put it into -tip whenever you guys submit it, but it will have to wait for v4.8 most likely.

Thanks,

Ingo

#14 lore raw

PATCH v6 8/13 x86/xsaves: Fix XSTATE component offset print out

In reply to Yu-cheng Yu

Component offset print out was incorrect for XSAVES. Correct it and move to a separate function.

---
 arch/x86/kernel/fpu/xstate.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)
arch/x86/kernel/fpu/xstate.c +16 -2
index 89adb02..8a89840 100644
@@ -269,8 +269,6 @@ static void __init setup_xstate_features(void)
 		WARN_ONCE(last_good_offset > xstate_offsets[i],
 			"x86/fpu: misordered xstate at %d\n", last_good_offset);
 		last_good_offset = xstate_offsets[i];
-
-		printk(KERN_INFO "x86/fpu: xstate_offset[%d]: %4d, xstate_sizes[%d]: %4d\n", i, ebx, i, eax);
 	}
 }
 
@@ -374,6 +372,21 @@ static void __init setup_xstate_comp(void)
 }
 
 /*
+ * Print out xstate component offsets and sizes
+ */
+static void __init print_xstate_offset_size(void)
+{
+	int i;
+
+	for (i = FIRST_EXTENDED_XFEATURE; i < XFEATURE_MAX; i++) {
+		if (!xfeature_enabled(i))
+			continue;
+		pr_info("x86/fpu: xstate_offset[%d]: %4d, xstate_sizes[%d]: %4d\n",
+			 i, xstate_comp_offsets[i], i, xstate_sizes[i]);
+	}
+}
+
+/*
  * setup the xstate image representing the init state
  */
 static void __init setup_init_fpu_buf(void)
@@ -708,6 +721,7 @@ void __init fpu__init_system_xstate(void)
 	fpu__init_prepare_fx_sw_frame();
 	setup_init_fpu_buf();
 	setup_xstate_comp();
+	print_xstate_offset_size();
 
 	pr_info("x86/fpu: Enabled xstate features 0x%llx, context size is %d bytes, using '%s' format.\n",
 		xfeatures_mask,
Signature · 1 line
1.9.1
#15 lore raw

PATCH v6 9/13 x86/xsaves: Fix xstate_offsets, xstate_sizes for non-extended states

In reply to Yu-cheng Yu

The arrays xstate_offsets[] and xstate_sizes[] record XSAVE standard- format offsets and sizes. Values for non-extended state components fpu and xmm's were not initialized or used. Ptrace format conversion needs them. Fix it.

---
 arch/x86/kernel/fpu/xstate.c | 9 +++++++++
 1 file changed, 9 insertions(+)
arch/x86/kernel/fpu/xstate.c +9 -0
index 8a89840..a913d4e 100644
@@ -246,6 +246,15 @@ static void __init setup_xstate_features(void)
 	/* start at the beginnning of the "extended state" */
 	unsigned int last_good_offset = offsetof(struct xregs_state,
 						 extended_state_area);
+	/*
+	 * The FP xstates and SSE xstates are legacy states. They are always
+	 * in the fixed offsets in the xsave area in either compacted form
+	 * or standard form.
+	 */
+	xstate_offsets[0] = 0;
+	xstate_sizes[0] = offsetof(struct fxregs_state, xmm_space);
+	xstate_offsets[1] = xstate_sizes[0];
+	xstate_sizes[1] = FIELD_SIZEOF(struct fxregs_state, xmm_space);
 
 	for (i = FIRST_EXTENDED_XFEATURE; i < XFEATURE_MAX; i++) {
 		if (!xfeature_enabled(i))
Signature · 1 line
1.9.1
#16 lore raw

PATCH v6 10/13 x86/xsaves: Fix __fpu_restore_sig() for XSAVES

In reply to Yu-cheng Yu

When the kernel is using XSAVES compacted format, we cannot do __copy_from_user() from a signal frame, which has standard-format data. Fix it by using copyin_to_xsaves(), which converts between formats and filters out all supervisor states that we do not allow userspace to write.

---
 arch/x86/kernel/fpu/signal.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
arch/x86/kernel/fpu/signal.c +9 -2
index 9ce2963..abc96b2 100644
@@ -321,8 +321,15 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 		 */
 		fpu__drop(fpu);
 
-		if (__copy_from_user(&fpu->state.xsave, buf_fx, state_size) ||
-		    __copy_from_user(&env, buf, sizeof(env))) {
+		if (using_compacted_format()) {
+			err = copyin_to_xsaves(NULL, buf_fx,
+					       &fpu->state.xsave);
+		} else {
+			err = __copy_from_user(&fpu->state.xsave,
+					       buf_fx, state_size);
+		}
+
+		if (err || __copy_from_user(&env, buf, sizeof(env))) {
 			fpstate_init(&fpu->state);
 			err = -1;
 		} else {
Signature · 1 line
1.9.1
#17 lore raw

PATCH v6 11/13 x86/xsaves: When a disabled xstate component offset is requested, return NULL

In reply to Yu-cheng Yu

It is an error to request a disabled XSAVE/XSAVES component address. For that case, make __raw_xsave_addr() return a NULL and issue a warning.

---
 arch/x86/kernel/fpu/xstate.c | 5 +++++
 1 file changed, 5 insertions(+)
arch/x86/kernel/fpu/xstate.c +5 -0
index a913d4e..7c2cdc4 100644
@@ -761,6 +761,11 @@ void *__raw_xsave_addr(struct xregs_state *xsave, int xstate_feature_mask)
 {
 	int feature_nr = fls64(xstate_feature_mask) - 1;
 
+	if (!xfeature_enabled(feature_nr)) {
+		WARN_ON_FPU(1);
+		return NULL;
+	}
+
 	return (void *)xsave + xstate_comp_offsets[feature_nr];
 }
 
Signature · 1 line
1.9.1
#18 lore raw

PATCH v6 12/13 x86/xsaves: Fix fpstate_init() for XRSTORS

In reply to Yu-cheng Yu

In XSAVES mode if fpstate_init() is used to initialize a task's extended state area, xsave.header.xcomp_bv[63] must be set. Otherwise, when the task is scheduled, a warning is triggered from copy_kernel_to_xregs().

One such test case is: setting an invalid extended state through PTRACE. When xstateregs_set() rejects the syscall and re-initializes the task's extended state area. This triggers the warning mentioned above.

---
 arch/x86/include/asm/fpu/types.h | 6 ++++++
 arch/x86/kernel/fpu/core.c       | 4 ++++
 2 files changed, 10 insertions(+)
arch/x86/include/asm/fpu/types.h +6 -0
index 12dd648..48df486 100644
@@ -232,6 +232,12 @@ struct xstate_header {
 } __attribute__((packed));
 
 /*
+ * xstate_header.xcomp_bv[63] indicates that the extended_state_area
+ * is in compacted format.
+ */
+#define XCOMP_BV_COMPACTED_FORMAT ((u64)1 << 63)
+
+/*
  * This is our most modern FPU state format, as saved by the XSAVE
  * and restored by the XRSTOR instructions.
  *
arch/x86/kernel/fpu/core.c +4 -0
index dfd29ac..9ed096f 100644
@@ -8,6 +8,7 @@
 #include <asm/fpu/internal.h>
 #include <asm/fpu/regset.h>
 #include <asm/fpu/signal.h>
+#include <asm/fpu/types.h>
 #include <asm/traps.h>
 
 #include <linux/hardirq.h>
@@ -224,6 +225,9 @@ void fpstate_init(union fpregs_state *state)
 
 	memset(state, 0, fpu_kernel_xstate_size);
 
+	if (cpu_has_xsaves)
+		state->xsave.header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT;
+
 	if (cpu_has_fxsr)
 		fpstate_init_fxstate(&state->fxsave);
 	else
Signature · 1 line
1.9.1
#19 lore raw

PATCH v6 13/13 x86/xsaves: Re-enable XSAVES

In reply to Yu-cheng Yu

We did not handle XSAVES instructions correctly. There were issues in converting between standard and compacted format when interfacing with user-space. These issues have been corrected.

Add a WARN_ONCE() to make it clear that XSAVES supervisor states are not yet implemented.

---
 arch/x86/kernel/fpu/init.c   | 15 ---------------
 arch/x86/kernel/fpu/xstate.c | 10 ++++++++++
 2 files changed, 10 insertions(+), 15 deletions(-)
arch/x86/kernel/fpu/init.c +0 -15
index eccc36b..2b4f871 100644
@@ -230,21 +230,6 @@ static void __init fpu__init_system_xstate_size_legacy(void)
 	}
 
 	fpu_user_xstate_size = fpu_kernel_xstate_size;
-
-	/*
-	 * Quirk: we don't yet handle the XSAVES* instructions
-	 * correctly, as we don't correctly convert between
-	 * standard and compacted format when interfacing
-	 * with user-space - so disable it for now.
-	 *
-	 * The difference is small: with recent CPUs the
-	 * compacted format is only marginally smaller than
-	 * the standard FPU state format.
-	 *
-	 * ( This is easy to backport while we are fixing
-	 *   XSAVES* support. )
-	 */
-	setup_clear_cpu_cap(X86_FEATURE_XSAVES);
 }
 
 /*
arch/x86/kernel/fpu/xstate.c +10 -0
index 7c2cdc4..fed95f5 100644
@@ -222,6 +222,16 @@ void fpu__init_cpu_xstate(void)
 	if (!cpu_has_xsave || !xfeatures_mask)
 		return;
 
+	/*
+	 * Make it clear that XSAVES supervisor states are not yet
+	 * implemented should anyone expect it to work by changing
+	 * bits in XFEATURE_MASK_* macros and XCR0.
+	 */
+	WARN_ONCE((xfeatures_mask & XFEATURE_MASK_SUPERVISOR),
+		"x86/fpu: XSAVES supervisor states are not yet implemented.\n");
+
+	xfeatures_mask &= ~XFEATURE_MASK_SUPERVISOR;
+
 	cr4_set_bits(X86_CR4_OSXSAVE);
 	xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask);
 }
Signature · 1 line
1.9.1
#20 lore raw
In reply to Yu-cheng Yu
Quoted 5 lines · Yu-cheng Yu

XSAVES is a kernel-mode instruction. It offers a compacted format and memory-write optimization. These patches fix issues in the first implementation.

Changes since Version 5:

Please, for the future, do send your patchset roughly only once a week - you just sent v5 a day or two ago without giving proper time to people (me?) to review them all. Consult Documentation/SubmittingPatches if there are doubts.

Thanks.

Signature · 5 lines
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 
#21 lore raw
Quoted 4 lines · Borislav Petkov

Please, for the future, do send your patchset roughly only once a week - you just sent v5 a day or two ago without giving proper time to people (me?) to review them all. Consult Documentation/SubmittingPatches if there are doubts.

There are no actual functional changes. I wanted to respond to comments quickly. I will try to be normal next time :-)

Yu-cheng

#22 lore raw
Quoted 2 lines · Yu-cheng Yu

There are no actual functional changes. I wanted to respond to comments quickly.

This is not how you respond to comments quickly - you wait roughly a week until people have had time to review the whole thing and *then* you send again. Spamming people with a patchset everyday will make most of them not look at your stuff anymore.

And go read Documentation/SubmittingPatches already because I'm tired of typing the same things over and over again.

Signature · 5 lines
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 
#23 lore raw
In reply to Yu-cheng Yu
Quoted 9 lines · Yu-cheng Yu

XSAVES is a kernel-mode instruction. It offers a compacted format and memory-write optimization. These patches fix issues in the first implementation.

Changes since Version 5:

Patch 1, 2 - Change names to fpu_user_xstate_size and fpu_kernel_xstate_size;
fix some comments, etc.

Quoted 8 lines

Patch 11 - In addition to a warning, make __raw_xsave_addr() return NULL
when a disabled xstate component address is requested.

Patch 12 - Add more details in the comments of why this patch is needed;
make a new macro for xcomp_bv[63].

Yu-cheng Yu (13):
x86/xsaves: Define and use fpu_user_xstate_size

So if you look at the git log of arch/x86/kernel/fpu/ then you'll see that the standard tag for FPU code is 'x86/fpu: ', for purely xstate related patch it's 'x86/fpu/xstate: '.

Quoted 5 lines

x86/xsaves: Rename xstate_size to fpu_kernel_xstate_size to distinguish from fpu_user_xstate_size x86/xsaves: Keep init_fpstate.xsave.header.xfeatures as zero for init optimization x86/xsaves: Introduce a new check that allows correct xstates copy from kernel to user directly x86/xsaves: Align xstate components according to CPUID
x86/xsaves: Supervisor state component offset

Please start all commit titles with verbs.

Also, this series is clearly too big. I was under the impression that the series was ready, but there's a number of small details that were pointed out (none of them very serious) - so how about you send a series of the first 4 patches next time around, and only send another 4 once those 4 first patches get applied to x86/fpu?

This makes it much easier to review, which reduces reviewer frustration; and it also allows you to focus more on incorporating feedback, without having to juggle 13 patches all the time.

Ok?

Thanks,

Ingo