[patch v3 0/2] updated ptrace/core-dump patches for supporting xstate - v3

[patch v3 0/2] updated ptrace/core-dump patches for supporting xstate - v3

[Suresh Siddha]

Changes from v2:
a. Embrace the iov interface for PTRACE_GETREGSET/PTRACE_SETREGSET
b. fixes and coding style changes based on Roland and Oleg's feedback.

Changes from v1:
a. Split the patch into multiple patches.
b. Fix some bugs and coding style changes based on Roland's feedback.
c. Add generic PTRACE_GETREGSET/PTRACE_SETREGSET commands through which we can
   export the architecture specific regsets using the corresponding
   NT_* codes that userland is already aware of. The "xstate" regset is exported
   only using this interface.

thanks,
suresh

[patch v3 1/2] x86, ptrace: regset extensions to support xstate

[Suresh Siddha]

[-- Attachment #1: add_regset_for_xstate.patch --]
[-- Type: text/plain, Size: 11810 bytes --]

Add the xstate regset support which helps extend the kernel ptrace and the
core-dump interfaces to support AVX state etc.

This regset interface is designed to support all the future state that gets
supported using xsave/xrstor infrastructure.

Looking at the memory layout saved by "xsave", one can't say which state
is represented in the memory layout. This is because if a particular state is
in init state, in the xsave hdr it can be represented by bit '0'. And hence
we can't really say by the xsave header wether a state is in init state or
the state is not saved in the memory layout.

And hence the xsave memory layout available through this regset
interface uses SW usable bytes [464..511] to convey what state is represented
in the memory layout.

First 8 bytes of the sw_usable_bytes[464..467] will be set to OS enabled xstate
mask(which is same as the 64bit mask returned by the xgetbv's xCR0).

The note NT_X86_XSTATE represents the extended state information in the
core file, using the above mentioned memory layout.

Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
Signed-off-by: Hongjiu Lu <hjl.tools@gmail.com>
---
 arch/x86/include/asm/i387.h  |   12 +++++-
 arch/x86/include/asm/user.h  |   58 ++++++++++++++++++++++++++++++
 arch/x86/include/asm/xsave.h |    2 +
 arch/x86/kernel/i387.c       |   83 +++++++++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/ptrace.c     |   34 ++++++++++++++++-
 arch/x86/kernel/xsave.c      |    1 
 include/linux/elf.h          |    1 
 7 files changed, 187 insertions(+), 4 deletions(-)

Index: tip/arch/x86/include/asm/i387.h
===================================================================
--- tip.orig/arch/x86/include/asm/i387.h
+++ tip/arch/x86/include/asm/i387.h
@@ -33,8 +33,16 @@ extern void init_thread_xstate(void);
 extern int dump_fpu(struct pt_regs *, struct user_i387_struct *);
 
 extern user_regset_active_fn fpregs_active, xfpregs_active;
-extern user_regset_get_fn fpregs_get, xfpregs_get, fpregs_soft_get;
-extern user_regset_set_fn fpregs_set, xfpregs_set, fpregs_soft_set;
+extern user_regset_get_fn fpregs_get, xfpregs_get, fpregs_soft_get,
+				xstateregs_get;
+extern user_regset_set_fn fpregs_set, xfpregs_set, fpregs_soft_set,
+				 xstateregs_set;
+
+/*
+ * xstateregs_active == fpregs_active. Please refer to the comment
+ * at the definition of fpregs_active.
+ */
+#define xstateregs_active	fpregs_active
 
 extern struct _fpx_sw_bytes fx_sw_reserved;
 #ifdef CONFIG_IA32_EMULATION
Index: tip/arch/x86/include/asm/xsave.h
===================================================================
--- tip.orig/arch/x86/include/asm/xsave.h
+++ tip/arch/x86/include/asm/xsave.h
@@ -27,9 +27,11 @@
 extern unsigned int xstate_size;
 extern u64 pcntxt_mask;
 extern struct xsave_struct *init_xstate_buf;
+extern u64 xstate_fx_sw_bytes[USER_XSTATE_FX_SW_WORDS];
 
 extern void xsave_cntxt_init(void);
 extern void xsave_init(void);
+extern void update_regset_xstate_info(unsigned int size, u64 xstate_mask);
 extern int init_fpu(struct task_struct *child);
 extern int check_for_xstate(struct i387_fxsave_struct __user *buf,
 			    void __user *fpstate,
Index: tip/arch/x86/kernel/i387.c
===================================================================
--- tip.orig/arch/x86/kernel/i387.c
+++ tip/arch/x86/kernel/i387.c
@@ -164,6 +164,11 @@ int init_fpu(struct task_struct *tsk)
 	return 0;
 }
 
+/*
+ * The xstateregs_active() routine is the same as the fpregs_active() routine,
+ * as the "regset->n" for the xstate regset will be updated based on the feature
+ * capabilites supported by the xsave.
+ */
 int fpregs_active(struct task_struct *target, const struct user_regset *regset)
 {
 	return tsk_used_math(target) ? regset->n : 0;
@@ -224,6 +229,84 @@ int xfpregs_set(struct task_struct *targ
 	return ret;
 }
 
+int xstateregs_get(struct task_struct *target, const struct user_regset *regset,
+		unsigned int pos, unsigned int count,
+		void *kbuf, void __user *ubuf)
+{
+	int ret;
+
+	if (!cpu_has_xsave)
+		return -ENODEV;
+
+	ret = init_fpu(target);
+	if (ret)
+		return ret;
+
+	/*
+	 * First copy the fxsave bytes 0..463.
+	 */
+	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
+				  &target->thread.xstate->xsave, 0,
+				  offsetof(struct user_xstateregs,
+					   i387.xstate_fx_sw));
+	if (ret)
+		return ret;
+
+	/*
+	 * Copy the 48bytes defined by software.
+	 */
+	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
+				  xstate_fx_sw_bytes,
+				  offsetof(struct user_xstateregs,
+					   i387.xstate_fx_sw),
+				  offsetof(struct user_xstateregs,
+					   xsave_hdr));
+	if (ret)
+		return ret;
+
+	/*
+	 * Copy the rest of xstate memory layout.
+	 */
+	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
+				  &target->thread.xstate->xsave.xsave_hdr,
+				  offsetof(struct user_xstateregs,
+					   xsave_hdr), -1);
+	return ret;
+}
+
+int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
+		  unsigned int pos, unsigned int count,
+		  const void *kbuf, const void __user *ubuf)
+{
+	int ret;
+	struct xsave_hdr_struct *xsave_hdr;
+
+	if (!cpu_has_xsave)
+		return -ENODEV;
+
+	ret = init_fpu(target);
+	if (ret)
+		return ret;
+
+	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
+				 &target->thread.xstate->xsave, 0, -1);
+
+	/*
+	 * mxcsr reserved bits must be masked to zero for security reasons.
+	 */
+	target->thread.xstate->fxsave.mxcsr &= mxcsr_feature_mask;
+
+	xsave_hdr = &target->thread.xstate->xsave.xsave_hdr;
+
+	xsave_hdr->xstate_bv &= pcntxt_mask;
+	/*
+	 * These bits must be zero.
+	 */
+	xsave_hdr->reserved1[0] = xsave_hdr->reserved1[1] = 0;
+
+	return ret;
+}
+
 #if defined CONFIG_X86_32 || defined CONFIG_IA32_EMULATION
 
 /*
Index: tip/arch/x86/kernel/ptrace.c
===================================================================
--- tip.orig/arch/x86/kernel/ptrace.c
+++ tip/arch/x86/kernel/ptrace.c
@@ -48,6 +48,7 @@ enum x86_regset {
 	REGSET_FP,
 	REGSET_XFP,
 	REGSET_IOPERM64 = REGSET_XFP,
+	REGSET_XSTATE,
 	REGSET_TLS,
 	REGSET_IOPERM32,
 };
@@ -1560,7 +1561,7 @@ long compat_arch_ptrace(struct task_stru
 
 #ifdef CONFIG_X86_64
 
-static const struct user_regset x86_64_regsets[] = {
+static struct user_regset x86_64_regsets[] __read_mostly = {
 	[REGSET_GENERAL] = {
 		.core_note_type = NT_PRSTATUS,
 		.n = sizeof(struct user_regs_struct) / sizeof(long),
@@ -1573,6 +1574,12 @@ static const struct user_regset x86_64_r
 		.size = sizeof(long), .align = sizeof(long),
 		.active = xfpregs_active, .get = xfpregs_get, .set = xfpregs_set
 	},
+	[REGSET_XSTATE] = {
+		.core_note_type = NT_X86_XSTATE,
+		.size = sizeof(u64), .align = sizeof(u64),
+		.active = xstateregs_active, .get = xstateregs_get,
+		.set = xstateregs_set
+	},
 	[REGSET_IOPERM64] = {
 		.core_note_type = NT_386_IOPERM,
 		.n = IO_BITMAP_LONGS,
@@ -1598,7 +1605,7 @@ static const struct user_regset_view use
 #endif	/* CONFIG_X86_64 */
 
 #if defined CONFIG_X86_32 || defined CONFIG_IA32_EMULATION
-static const struct user_regset x86_32_regsets[] = {
+static struct user_regset x86_32_regsets[] __read_mostly = {
 	[REGSET_GENERAL] = {
 		.core_note_type = NT_PRSTATUS,
 		.n = sizeof(struct user_regs_struct32) / sizeof(u32),
@@ -1617,6 +1624,12 @@ static const struct user_regset x86_32_r
 		.size = sizeof(u32), .align = sizeof(u32),
 		.active = xfpregs_active, .get = xfpregs_get, .set = xfpregs_set
 	},
+	[REGSET_XSTATE] = {
+		.core_note_type = NT_X86_XSTATE,
+		.size = sizeof(u64), .align = sizeof(u64),
+		.active = xstateregs_active, .get = xstateregs_get,
+		.set = xstateregs_set
+	},
 	[REGSET_TLS] = {
 		.core_note_type = NT_386_TLS,
 		.n = GDT_ENTRY_TLS_ENTRIES, .bias = GDT_ENTRY_TLS_MIN,
@@ -1639,6 +1652,23 @@ static const struct user_regset_view use
 };
 #endif
 
+/*
+ * This represents bytes 464..511 in the memory layout exported through
+ * the REGSET_XSTATE interface.
+ */
+u64 xstate_fx_sw_bytes[USER_XSTATE_FX_SW_WORDS];
+
+void update_regset_xstate_info(unsigned int size, u64 xstate_mask)
+{
+#ifdef CONFIG_X86_64
+	x86_64_regsets[REGSET_XSTATE].n = size / sizeof(u64);
+#endif
+#if defined CONFIG_X86_32 || defined CONFIG_IA32_EMULATION
+	x86_32_regsets[REGSET_XSTATE].n = size / sizeof(u64);
+#endif
+	xstate_fx_sw_bytes[USER_XSTATE_XCR0_WORD] = xstate_mask;
+}
+
 const struct user_regset_view *task_user_regset_view(struct task_struct *task)
 {
 #ifdef CONFIG_IA32_EMULATION
Index: tip/arch/x86/kernel/xsave.c
===================================================================
--- tip.orig/arch/x86/kernel/xsave.c
+++ tip/arch/x86/kernel/xsave.c
@@ -337,6 +337,7 @@ void __ref xsave_cntxt_init(void)
 	cpuid_count(0xd, 0, &eax, &ebx, &ecx, &edx);
 	xstate_size = ebx;
 
+	update_regset_xstate_info(xstate_size, pcntxt_mask);
 	prepare_fx_sw_frame();
 
 	setup_xstate_init();
Index: tip/include/linux/elf.h
===================================================================
--- tip.orig/include/linux/elf.h
+++ tip/include/linux/elf.h
@@ -361,6 +361,7 @@ typedef struct elf64_shdr {
 #define NT_PPC_VSX	0x102		/* PowerPC VSX registers */
 #define NT_386_TLS	0x200		/* i386 TLS slots (struct user_desc) */
 #define NT_386_IOPERM	0x201		/* x86 io permission bitmap (1=deny) */
+#define NT_X86_XSTATE	0x202		/* x86 extended state using xsave */
 #define NT_S390_HIGH_GPRS	0x300	/* s390 upper register halves */
 
 
Index: tip/arch/x86/include/asm/user.h
===================================================================
--- tip.orig/arch/x86/include/asm/user.h
+++ tip/arch/x86/include/asm/user.h
@@ -1,5 +1,63 @@
+#ifndef _ASM_X86_USER_H
+#define _ASM_X86_USER_H
+
 #ifdef CONFIG_X86_32
 # include "user_32.h"
 #else
 # include "user_64.h"
 #endif
+
+#include <asm/types.h>
+
+struct user_ymmh_regs {
+	/* 16 * 16 bytes for each YMMH-reg */
+	__u32 ymmh_space[64];
+};
+
+struct user_xsave_hdr {
+	__u64 xstate_bv;
+	__u64 reserved1[2];
+	__u64 reserved2[5];
+};
+
+/*
+ * The structure layout of user_xstateregs, used for exporting the
+ * extended register state through ptrace and core-dump (NT_X86_XSTATE note)
+ * interfaces will be same as the memory layout of xsave used by the processor
+ * (except for the bytes 464..511, which can be used by the software) and hence
+ * the size of this structure varies depending on the features supported by the
+ * processor and OS. The size of the structure that users need to use can be
+ * obtained by doing:
+ *     cpuid_count(0xd, 0, &eax, &ptrace_xstateregs_struct_size, &ecx, &edx);
+ * i.e., cpuid.(eax=0xd,ecx=0).ebx will be the size that user (debuggers, etc.)
+ * need to use.
+ *
+ * For now, only the first 8 bytes of the software usable bytes[464..471] will
+ * be used and will be set to OS enabled xstate mask (which is same as the
+ * 64bit mask returned by the xgetbv's xCR0).  Users (analyzing core dump
+ * remotely, etc.) can use this mask as well as the mask saved in the
+ * xstate_hdr bytes and interpret what states the processor/OS supports
+ * and what states are in modified/initialized conditions for the
+ * particular process/thread.
+ *
+ * Also when the user modifies certain state FP/SSE/etc through the
+ * ptrace interface, they must ensure that the xsave_hdr.xstate_bv
+ * bytes[512..519] of the memory layout are updated correspondingly.
+ * i.e., for example when FP state is modified to a non-init state,
+ * xsave_hdr.xstate_bv's bit 0 must be set to '1', when SSE is modified to
+ * non-init state, xsave_hdr.xstate_bv's bit 1 must to be set to '1', etc.
+ */
+#define USER_XSTATE_FX_SW_WORDS 6
+#define USER_XSTATE_XCR0_WORD	0
+
+struct user_xstateregs {
+	struct {
+		__u64 fpx_space[58];
+		__u64 xstate_fx_sw[USER_XSTATE_FX_SW_WORDS];
+	} i387;
+	struct user_xsave_hdr xsave_hdr;
+	struct user_ymmh_regs ymmh;
+	/* further processor state extensions go here */
+};
+
+#endif /* _ASM_X86_USER_H */

[tip:x86/ptrace] x86, ptrace: regset extensions to support xstate

[tip-bot for Suresh Siddha]

Commit-ID:  5b3efd500854d45d305b53c54c97db5970959980
Gitweb:     http://git.kernel.org/tip/5b3efd500854d45d305b53c54c97db5970959980
Author:     Suresh Siddha <suresh.b.siddha@intel.com>
AuthorDate: Thu, 11 Feb 2010 11:50:59 -0800
Committer:  H. Peter Anvin <hpa@zytor.com>
CommitDate: Thu, 11 Feb 2010 15:08:17 -0800

x86, ptrace: regset extensions to support xstate

Add the xstate regset support which helps extend the kernel ptrace and the
core-dump interfaces to support AVX state etc.

This regset interface is designed to support all the future state that gets
supported using xsave/xrstor infrastructure.

Looking at the memory layout saved by "xsave", one can't say which state
is represented in the memory layout. This is because if a particular state is
in init state, in the xsave hdr it can be represented by bit '0'. And hence
we can't really say by the xsave header wether a state is in init state or
the state is not saved in the memory layout.

And hence the xsave memory layout available through this regset
interface uses SW usable bytes [464..511] to convey what state is represented
in the memory layout.

First 8 bytes of the sw_usable_bytes[464..467] will be set to OS enabled xstate
mask(which is same as the 64bit mask returned by the xgetbv's xCR0).

The note NT_X86_XSTATE represents the extended state information in the
core file, using the above mentioned memory layout.

Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
LKML-Reference: <20100211195614.802495327@sbs-t61.sc.intel.com>
Signed-off-by: Hongjiu Lu <hjl.tools@gmail.com>
Cc: Roland McGrath <roland@redhat.com>
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
---
 arch/x86/include/asm/i387.h  |   12 +++++-
 arch/x86/include/asm/user.h  |   58 +++++++++++++++++++++++++++++
 arch/x86/include/asm/xsave.h |    2 +
 arch/x86/kernel/i387.c       |   83 ++++++++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/ptrace.c     |   34 ++++++++++++++++-
 arch/x86/kernel/xsave.c      |    1 +
 include/linux/elf.h          |    1 +
 7 files changed, 187 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/i387.h b/arch/x86/include/asm/i387.h
index ebfb8a9..da29309 100644
--- a/arch/x86/include/asm/i387.h
+++ b/arch/x86/include/asm/i387.h
@@ -33,8 +33,16 @@ extern void init_thread_xstate(void);
 extern int dump_fpu(struct pt_regs *, struct user_i387_struct *);
 
 extern user_regset_active_fn fpregs_active, xfpregs_active;
-extern user_regset_get_fn fpregs_get, xfpregs_get, fpregs_soft_get;
-extern user_regset_set_fn fpregs_set, xfpregs_set, fpregs_soft_set;
+extern user_regset_get_fn fpregs_get, xfpregs_get, fpregs_soft_get,
+				xstateregs_get;
+extern user_regset_set_fn fpregs_set, xfpregs_set, fpregs_soft_set,
+				 xstateregs_set;
+
+/*
+ * xstateregs_active == fpregs_active. Please refer to the comment
+ * at the definition of fpregs_active.
+ */
+#define xstateregs_active	fpregs_active
 
 extern struct _fpx_sw_bytes fx_sw_reserved;
 #ifdef CONFIG_IA32_EMULATION
diff --git a/arch/x86/include/asm/user.h b/arch/x86/include/asm/user.h
index 999873b..24532c7 100644
--- a/arch/x86/include/asm/user.h
+++ b/arch/x86/include/asm/user.h
@@ -1,5 +1,63 @@
+#ifndef _ASM_X86_USER_H
+#define _ASM_X86_USER_H
+
 #ifdef CONFIG_X86_32
 # include "user_32.h"
 #else
 # include "user_64.h"
 #endif
+
+#include <asm/types.h>
+
+struct user_ymmh_regs {
+	/* 16 * 16 bytes for each YMMH-reg */
+	__u32 ymmh_space[64];
+};
+
+struct user_xsave_hdr {
+	__u64 xstate_bv;
+	__u64 reserved1[2];
+	__u64 reserved2[5];
+};
+
+/*
+ * The structure layout of user_xstateregs, used for exporting the
+ * extended register state through ptrace and core-dump (NT_X86_XSTATE note)
+ * interfaces will be same as the memory layout of xsave used by the processor
+ * (except for the bytes 464..511, which can be used by the software) and hence
+ * the size of this structure varies depending on the features supported by the
+ * processor and OS. The size of the structure that users need to use can be
+ * obtained by doing:
+ *     cpuid_count(0xd, 0, &eax, &ptrace_xstateregs_struct_size, &ecx, &edx);
+ * i.e., cpuid.(eax=0xd,ecx=0).ebx will be the size that user (debuggers, etc.)
+ * need to use.
+ *
+ * For now, only the first 8 bytes of the software usable bytes[464..471] will
+ * be used and will be set to OS enabled xstate mask (which is same as the
+ * 64bit mask returned by the xgetbv's xCR0).  Users (analyzing core dump
+ * remotely, etc.) can use this mask as well as the mask saved in the
+ * xstate_hdr bytes and interpret what states the processor/OS supports
+ * and what states are in modified/initialized conditions for the
+ * particular process/thread.
+ *
+ * Also when the user modifies certain state FP/SSE/etc through the
+ * ptrace interface, they must ensure that the xsave_hdr.xstate_bv
+ * bytes[512..519] of the memory layout are updated correspondingly.
+ * i.e., for example when FP state is modified to a non-init state,
+ * xsave_hdr.xstate_bv's bit 0 must be set to '1', when SSE is modified to
+ * non-init state, xsave_hdr.xstate_bv's bit 1 must to be set to '1', etc.
+ */
+#define USER_XSTATE_FX_SW_WORDS 6
+#define USER_XSTATE_XCR0_WORD	0
+
+struct user_xstateregs {
+	struct {
+		__u64 fpx_space[58];
+		__u64 xstate_fx_sw[USER_XSTATE_FX_SW_WORDS];
+	} i387;
+	struct user_xsave_hdr xsave_hdr;
+	struct user_ymmh_regs ymmh;
+	/* further processor state extensions go here */
+};
+
+#endif /* _ASM_X86_USER_H */
diff --git a/arch/x86/include/asm/xsave.h b/arch/x86/include/asm/xsave.h
index 727acc1..ddc04cc 100644
--- a/arch/x86/include/asm/xsave.h
+++ b/arch/x86/include/asm/xsave.h
@@ -27,9 +27,11 @@
 extern unsigned int xstate_size;
 extern u64 pcntxt_mask;
 extern struct xsave_struct *init_xstate_buf;
+extern u64 xstate_fx_sw_bytes[USER_XSTATE_FX_SW_WORDS];
 
 extern void xsave_cntxt_init(void);
 extern void xsave_init(void);
+extern void update_regset_xstate_info(unsigned int size, u64 xstate_mask);
 extern int init_fpu(struct task_struct *child);
 extern int check_for_xstate(struct i387_fxsave_struct __user *buf,
 			    void __user *fpstate,
diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index f2f8540..7a8a193 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -164,6 +164,11 @@ int init_fpu(struct task_struct *tsk)
 	return 0;
 }
 
+/*
+ * The xstateregs_active() routine is the same as the fpregs_active() routine,
+ * as the "regset->n" for the xstate regset will be updated based on the feature
+ * capabilites supported by the xsave.
+ */
 int fpregs_active(struct task_struct *target, const struct user_regset *regset)
 {
 	return tsk_used_math(target) ? regset->n : 0;
@@ -224,6 +229,84 @@ int xfpregs_set(struct task_struct *target, const struct user_regset *regset,
 	return ret;
 }
 
+int xstateregs_get(struct task_struct *target, const struct user_regset *regset,
+		unsigned int pos, unsigned int count,
+		void *kbuf, void __user *ubuf)
+{
+	int ret;
+
+	if (!cpu_has_xsave)
+		return -ENODEV;
+
+	ret = init_fpu(target);
+	if (ret)
+		return ret;
+
+	/*
+	 * First copy the fxsave bytes 0..463.
+	 */
+	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
+				  &target->thread.xstate->xsave, 0,
+				  offsetof(struct user_xstateregs,
+					   i387.xstate_fx_sw));
+	if (ret)
+		return ret;
+
+	/*
+	 * Copy the 48bytes defined by software.
+	 */
+	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
+				  xstate_fx_sw_bytes,
+				  offsetof(struct user_xstateregs,
+					   i387.xstate_fx_sw),
+				  offsetof(struct user_xstateregs,
+					   xsave_hdr));
+	if (ret)
+		return ret;
+
+	/*
+	 * Copy the rest of xstate memory layout.
+	 */
+	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
+				  &target->thread.xstate->xsave.xsave_hdr,
+				  offsetof(struct user_xstateregs,
+					   xsave_hdr), -1);
+	return ret;
+}
+
+int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
+		  unsigned int pos, unsigned int count,
+		  const void *kbuf, const void __user *ubuf)
+{
+	int ret;
+	struct xsave_hdr_struct *xsave_hdr;
+
+	if (!cpu_has_xsave)
+		return -ENODEV;
+
+	ret = init_fpu(target);
+	if (ret)
+		return ret;
+
+	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
+				 &target->thread.xstate->xsave, 0, -1);
+
+	/*
+	 * mxcsr reserved bits must be masked to zero for security reasons.
+	 */
+	target->thread.xstate->fxsave.mxcsr &= mxcsr_feature_mask;
+
+	xsave_hdr = &target->thread.xstate->xsave.xsave_hdr;
+
+	xsave_hdr->xstate_bv &= pcntxt_mask;
+	/*
+	 * These bits must be zero.
+	 */
+	xsave_hdr->reserved1[0] = xsave_hdr->reserved1[1] = 0;
+
+	return ret;
+}
+
 #if defined CONFIG_X86_32 || defined CONFIG_IA32_EMULATION
 
 /*
diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 017d937..16433a5 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -48,6 +48,7 @@ enum x86_regset {
 	REGSET_FP,
 	REGSET_XFP,
 	REGSET_IOPERM64 = REGSET_XFP,
+	REGSET_XSTATE,
 	REGSET_TLS,
 	REGSET_IOPERM32,
 };
@@ -1584,7 +1585,7 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
 
 #ifdef CONFIG_X86_64
 
-static const struct user_regset x86_64_regsets[] = {
+static struct user_regset x86_64_regsets[] __read_mostly = {
 	[REGSET_GENERAL] = {
 		.core_note_type = NT_PRSTATUS,
 		.n = sizeof(struct user_regs_struct) / sizeof(long),
@@ -1597,6 +1598,12 @@ static const struct user_regset x86_64_regsets[] = {
 		.size = sizeof(long), .align = sizeof(long),
 		.active = xfpregs_active, .get = xfpregs_get, .set = xfpregs_set
 	},
+	[REGSET_XSTATE] = {
+		.core_note_type = NT_X86_XSTATE,
+		.size = sizeof(u64), .align = sizeof(u64),
+		.active = xstateregs_active, .get = xstateregs_get,
+		.set = xstateregs_set
+	},
 	[REGSET_IOPERM64] = {
 		.core_note_type = NT_386_IOPERM,
 		.n = IO_BITMAP_LONGS,
@@ -1622,7 +1629,7 @@ static const struct user_regset_view user_x86_64_view = {
 #endif	/* CONFIG_X86_64 */
 
 #if defined CONFIG_X86_32 || defined CONFIG_IA32_EMULATION
-static const struct user_regset x86_32_regsets[] = {
+static struct user_regset x86_32_regsets[] __read_mostly = {
 	[REGSET_GENERAL] = {
 		.core_note_type = NT_PRSTATUS,
 		.n = sizeof(struct user_regs_struct32) / sizeof(u32),
@@ -1641,6 +1648,12 @@ static const struct user_regset x86_32_regsets[] = {
 		.size = sizeof(u32), .align = sizeof(u32),
 		.active = xfpregs_active, .get = xfpregs_get, .set = xfpregs_set
 	},
+	[REGSET_XSTATE] = {
+		.core_note_type = NT_X86_XSTATE,
+		.size = sizeof(u64), .align = sizeof(u64),
+		.active = xstateregs_active, .get = xstateregs_get,
+		.set = xstateregs_set
+	},
 	[REGSET_TLS] = {
 		.core_note_type = NT_386_TLS,
 		.n = GDT_ENTRY_TLS_ENTRIES, .bias = GDT_ENTRY_TLS_MIN,
@@ -1663,6 +1676,23 @@ static const struct user_regset_view user_x86_32_view = {
 };
 #endif
 
+/*
+ * This represents bytes 464..511 in the memory layout exported through
+ * the REGSET_XSTATE interface.
+ */
+u64 xstate_fx_sw_bytes[USER_XSTATE_FX_SW_WORDS];
+
+void update_regset_xstate_info(unsigned int size, u64 xstate_mask)
+{
+#ifdef CONFIG_X86_64
+	x86_64_regsets[REGSET_XSTATE].n = size / sizeof(u64);
+#endif
+#if defined CONFIG_X86_32 || defined CONFIG_IA32_EMULATION
+	x86_32_regsets[REGSET_XSTATE].n = size / sizeof(u64);
+#endif
+	xstate_fx_sw_bytes[USER_XSTATE_XCR0_WORD] = xstate_mask;
+}
+
 const struct user_regset_view *task_user_regset_view(struct task_struct *task)
 {
 #ifdef CONFIG_IA32_EMULATION
diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
index c5ee17e..782c3a3 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -337,6 +337,7 @@ void __ref xsave_cntxt_init(void)
 	cpuid_count(0xd, 0, &eax, &ebx, &ecx, &edx);
 	xstate_size = ebx;
 
+	update_regset_xstate_info(xstate_size, pcntxt_mask);
 	prepare_fx_sw_frame();
 
 	setup_xstate_init();
diff --git a/include/linux/elf.h b/include/linux/elf.h
index 0cc4d55..a8c4af0 100644
--- a/include/linux/elf.h
+++ b/include/linux/elf.h
@@ -361,6 +361,7 @@ typedef struct elf64_shdr {
 #define NT_PPC_VSX	0x102		/* PowerPC VSX registers */
 #define NT_386_TLS	0x200		/* i386 TLS slots (struct user_desc) */
 #define NT_386_IOPERM	0x201		/* x86 io permission bitmap (1=deny) */
+#define NT_X86_XSTATE	0x202		/* x86 extended state using xsave */
 #define NT_S390_HIGH_GPRS	0x300	/* s390 upper register halves */
 
 

Re: [patch v3 1/2] x86, ptrace: regset extensions to support xstate

[Roland McGrath]

> +	/*
> +	 * First copy the fxsave bytes 0..463.
> +	 */
> +	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> +				  &target->thread.xstate->xsave, 0,
> +				  offsetof(struct user_xstateregs,
> +					   i387.xstate_fx_sw));
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Copy the 48bytes defined by software.
> +	 */
> +	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> +				  xstate_fx_sw_bytes,
> +				  offsetof(struct user_xstateregs,
> +					   i387.xstate_fx_sw),
> +				  offsetof(struct user_xstateregs,
> +					   xsave_hdr));
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Copy the rest of xstate memory layout.
> +	 */
> +	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> +				  &target->thread.xstate->xsave.xsave_hdr,
> +				  offsetof(struct user_xstateregs,
> +					   xsave_hdr), -1);
> +	return ret;

I don't know why this didn't occur to me before.  Is there a reason not to
just copy xstate_fx_sw_bytes into the thread.xstate buffer?  You could just
do it right here, or in the places that really touch thread.xstate contents.

Then the trivial single user_regset_copyout() call would be all you need
here, and that seems simpler.  IMHO it's enough simpler that even if you
pay the 6-words memcpy every time here, it's worth it.

If you don't want to do that, that's fine too.  
The code looks correct as it is.

> --- tip.orig/arch/x86/include/asm/user.h
> +++ tip/arch/x86/include/asm/user.h

I don't have any special opinions about the placement, naming, etc. of
these asm/user.h bits (just that there be some symbolic form of the ABI
somewhere).  But perhaps folks want to think about it a bit more before we
bake it into the source API.

These definitions duplicate the asm/processor.h ones used inside the kernel.
IMHO these should be consolidated so asm/processor.h refers to the public types.

I'm not sure whether 'struct user_*' names make most sense for these or not.
They are processor-defined layouts.

> +struct user_ymmh_regs {
> +	/* 16 * 16 bytes for each YMMH-reg */
> +	__u32 ymmh_space[64];
> +};

This is the high half of %ymmN registers, where %xmmN is the low half.
Right?  Though of course documented in the chip manuals, it is rather
nonobvious that %ymmN are split this way in the storage, so I think it
merits a comment here saying so explicitly.


On these cosmetic issues I leave it up to you and the x86 maintainers.
I don't have strong opinions.  But I wanted to air what had occurred to me.

Modulo any cosmetic changes you want to make, this patch has my ACK
contingent on Oleg's ACK.


Thanks,
Roland

Re: [patch v3 1/2] x86, ptrace: regset extensions to support xstate

[Oleg Nesterov]

On 02/11, Suresh Siddha wrote:
>
> --- tip.orig/arch/x86/include/asm/user.h
> +++ tip/arch/x86/include/asm/user.h
> ...
> +struct user_xstateregs {
> +	struct {
> +		__u64 fpx_space[58];
> +		__u64 xstate_fx_sw[USER_XSTATE_FX_SW_WORDS];
> +	} i387;

Perhaps a small comment can explain that user_xstateregs.i387 is
"equal" to struct i387_fxsave_struct. Otherwise it is not immediately
clear what xstateregs_get() actually copies into *ubuf


I don't think my ACK should be counted here, but the patch looks
good to me.

Oleg.

[patch v3 2/2] ptrace: Add support for generic PTRACE_GETREGSET/PTRACE_SETREGSET

[Suresh Siddha]

[-- Attachment #1: ptrace_support_for_xstate.patch --]
[-- Type: text/plain, Size: 5623 bytes --]

Generic support for PTRACE_GETREGSET/PTRACE_SETREGSET commands which
export the regsets supported by each architecture using the correponding
NT_* types. These NT_* types are already part of the userland ABI, used
in representing the architecture specific register sets as different NOTES
in an ELF core file.

'addr' parameter for the ptrace system call encode the REGSET type (using
the corresppnding NT_* type) and the 'data' parameter points to the
struct iovec having the user buffer and the length of that buffer.

	struct iovec iov = { buf, len};
	ret = ptrace(PTRACE_GETREGSET/PTRACE_SETREGSET, pid, NT_XXX_TYPE, &iov);

On successful completion, iov.len will be updated by the kernel specifying
how much the kernel has written/read to/from the user's iov.buf.

x86 extended state registers are primarily exported using this interface.

Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
Acked-by: Hongjiu Lu <hjl.tools@gmail.com>
---
 include/linux/elf.h    |    6 ++-
 include/linux/ptrace.h |   15 ++++++++
 kernel/ptrace.c        |   88 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 108 insertions(+), 1 deletion(-)

Index: tip/include/linux/ptrace.h
===================================================================
--- tip.orig/include/linux/ptrace.h
+++ tip/include/linux/ptrace.h
@@ -27,6 +27,21 @@
 #define PTRACE_GETSIGINFO	0x4202
 #define PTRACE_SETSIGINFO	0x4203
 
+/*
+ * Generic ptrace interface that exports the architecture specific regsets
+ * using the corresponding NT_* types (which are also used in the core dump).
+ *
+ * This interface usage is as follows:
+ * 	struct iovec iov = { buf, len};
+ *
+ * 	ret = ptrace(PTRACE_GETREGSET/PTRACE_SETREGSET, pid, NT_XXX_TYPE, &iov);
+ *
+ * On the successful completion, iov.len will be updated by the kernel,
+ * specifying how much the kernel has written/read to/from the user's iov.buf.
+ */
+#define PTRACE_GETREGSET	0x4204
+#define PTRACE_SETREGSET	0x4205
+
 /* options set using PTRACE_SETOPTIONS */
 #define PTRACE_O_TRACESYSGOOD	0x00000001
 #define PTRACE_O_TRACEFORK	0x00000002
Index: tip/kernel/ptrace.c
===================================================================
--- tip.orig/kernel/ptrace.c
+++ tip/kernel/ptrace.c
@@ -22,6 +22,7 @@
 #include <linux/pid_namespace.h>
 #include <linux/syscalls.h>
 #include <linux/uaccess.h>
+#include <linux/regset.h>
 
 
 /*
@@ -511,6 +512,47 @@ static int ptrace_resume(struct task_str
 	return 0;
 }
 
+#ifdef CONFIG_HAVE_ARCH_TRACEHOOK
+
+static const struct user_regset *
+find_regset(const struct user_regset_view *view, unsigned int type)
+{
+	const struct user_regset *regset;
+	int n;
+
+	for (n = 0; n < view->n; ++n) {
+		regset = view->regsets + n;
+		if (regset->core_note_type == type)
+			return regset;
+	}
+
+	return NULL;
+}
+
+static int ptrace_regset(struct task_struct *task, int req, unsigned int type,
+			 struct iovec *kiov)
+{
+	const struct user_regset_view *view = task_user_regset_view(task);
+	const struct user_regset *regset = find_regset(view, type);
+	int regset_no;
+
+	if (!regset || (kiov->iov_len % regset->size) != 0)
+		return -EIO;
+
+	regset_no = regset - view->regsets;
+	kiov->iov_len = min(kiov->iov_len,
+			    (__kernel_size_t) (regset->n * regset->size));
+
+	if (req == PTRACE_GETREGSET)
+		return copy_regset_to_user(task, view, regset_no, 0,
+					   kiov->iov_len, kiov->iov_base);
+	else
+		return copy_regset_from_user(task, view, regset_no, 0,
+					     kiov->iov_len, kiov->iov_base);
+}
+
+#endif
+
 int ptrace_request(struct task_struct *child, long request,
 		   long addr, long data)
 {
@@ -573,6 +615,26 @@ int ptrace_request(struct task_struct *c
 			return 0;
 		return ptrace_resume(child, request, SIGKILL);
 
+#ifdef CONFIG_HAVE_ARCH_TRACEHOOK
+	case PTRACE_GETREGSET:
+	case PTRACE_SETREGSET:
+	{
+		struct iovec kiov;
+		struct iovec __user *uiov = (struct iovec __user *) data;
+
+		if (!access_ok(VERIFY_WRITE, uiov, sizeof(*uiov)))
+			return -EFAULT;
+
+		if (__get_user(kiov.iov_base, &uiov->iov_base) ||
+		    __get_user(kiov.iov_len, &uiov->iov_len))
+			return -EFAULT;
+
+		ret = ptrace_regset(child, request, addr, &kiov);
+		if (!ret)
+			ret = __put_user(kiov.iov_len, &uiov->iov_len);
+		break;
+	}
+#endif
 	default:
 		break;
 	}
@@ -711,6 +773,32 @@ int compat_ptrace_request(struct task_st
 		else
 			ret = ptrace_setsiginfo(child, &siginfo);
 		break;
+#ifdef CONFIG_HAVE_ARCH_TRACEHOOK
+	case PTRACE_GETREGSET:
+	case PTRACE_SETREGSET:
+	{
+		struct iovec kiov;
+		struct compat_iovec __user *uiov =
+			(struct compat_iovec __user *) datap;
+		compat_uptr_t ptr;
+		compat_size_t len;
+
+		if (!access_ok(VERIFY_WRITE, uiov, sizeof(*uiov)))
+			return -EFAULT;
+
+		if (__get_user(ptr, &uiov->iov_base) ||
+		    __get_user(len, &uiov->iov_len))
+			return -EFAULT;
+
+		kiov.iov_base = compat_ptr(ptr);
+		kiov.iov_len = len;
+
+		ret = ptrace_regset(child, request, addr, &kiov);
+		if (!ret)
+			ret = __put_user(kiov.iov_len, &uiov->iov_len);
+		break;
+	}
+#endif
 
 	default:
 		ret = ptrace_request(child, request, addr, data);
Index: tip/include/linux/elf.h
===================================================================
--- tip.orig/include/linux/elf.h
+++ tip/include/linux/elf.h
@@ -349,7 +349,11 @@ typedef struct elf64_shdr {
 #define ELF_OSABI ELFOSABI_NONE
 #endif
 
-/* Notes used in ET_CORE */
+/*
+ * Notes used in ET_CORE. Architectures export some of the arch register sets
+ * using the corresponding note types via the PTRACE_GETREGSET and
+ * PTRACE_SETREGSET requests.
+ */
 #define NT_PRSTATUS	1
 #define NT_PRFPREG	2
 #define NT_PRPSINFO	3

[tip:x86/ptrace] ptrace: Add support for generic PTRACE_GETREGSET/PTRACE_SETREGSET

[tip-bot for Suresh Siddha]

Commit-ID:  2225a122ae26d542bdce523d9d87a4a7ba10e07b
Gitweb:     http://git.kernel.org/tip/2225a122ae26d542bdce523d9d87a4a7ba10e07b
Author:     Suresh Siddha <suresh.b.siddha@intel.com>
AuthorDate: Thu, 11 Feb 2010 11:51:00 -0800
Committer:  H. Peter Anvin <hpa@zytor.com>
CommitDate: Thu, 11 Feb 2010 15:08:33 -0800

ptrace: Add support for generic PTRACE_GETREGSET/PTRACE_SETREGSET

Generic support for PTRACE_GETREGSET/PTRACE_SETREGSET commands which
export the regsets supported by each architecture using the correponding
NT_* types. These NT_* types are already part of the userland ABI, used
in representing the architecture specific register sets as different NOTES
in an ELF core file.

'addr' parameter for the ptrace system call encode the REGSET type (using
the corresppnding NT_* type) and the 'data' parameter points to the
struct iovec having the user buffer and the length of that buffer.

	struct iovec iov = { buf, len};
	ret = ptrace(PTRACE_GETREGSET/PTRACE_SETREGSET, pid, NT_XXX_TYPE, &iov);

On successful completion, iov.len will be updated by the kernel specifying
how much the kernel has written/read to/from the user's iov.buf.

x86 extended state registers are primarily exported using this interface.

Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
LKML-Reference: <20100211195614.886724710@sbs-t61.sc.intel.com>
Acked-by: Hongjiu Lu <hjl.tools@gmail.com>
Cc: Roland McGrath <roland@redhat.com>
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
---
 include/linux/elf.h    |    6 +++-
 include/linux/ptrace.h |   15 ++++++++
 kernel/ptrace.c        |   88 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 108 insertions(+), 1 deletions(-)

diff --git a/include/linux/elf.h b/include/linux/elf.h
index a8c4af0..d8e6e61 100644
--- a/include/linux/elf.h
+++ b/include/linux/elf.h
@@ -349,7 +349,11 @@ typedef struct elf64_shdr {
 #define ELF_OSABI ELFOSABI_NONE
 #endif
 
-/* Notes used in ET_CORE */
+/*
+ * Notes used in ET_CORE. Architectures export some of the arch register sets
+ * using the corresponding note types via the PTRACE_GETREGSET and
+ * PTRACE_SETREGSET requests.
+ */
 #define NT_PRSTATUS	1
 #define NT_PRFPREG	2
 #define NT_PRPSINFO	3
diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index 56f2d63..dbfa821 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h
@@ -27,6 +27,21 @@
 #define PTRACE_GETSIGINFO	0x4202
 #define PTRACE_SETSIGINFO	0x4203
 
+/*
+ * Generic ptrace interface that exports the architecture specific regsets
+ * using the corresponding NT_* types (which are also used in the core dump).
+ *
+ * This interface usage is as follows:
+ *	struct iovec iov = { buf, len};
+ *
+ *	ret = ptrace(PTRACE_GETREGSET/PTRACE_SETREGSET, pid, NT_XXX_TYPE, &iov);
+ *
+ * On the successful completion, iov.len will be updated by the kernel,
+ * specifying how much the kernel has written/read to/from the user's iov.buf.
+ */
+#define PTRACE_GETREGSET	0x4204
+#define PTRACE_SETREGSET	0x4205
+
 /* options set using PTRACE_SETOPTIONS */
 #define PTRACE_O_TRACESYSGOOD	0x00000001
 #define PTRACE_O_TRACEFORK	0x00000002
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 23bd09c..13b4554 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -22,6 +22,7 @@
 #include <linux/pid_namespace.h>
 #include <linux/syscalls.h>
 #include <linux/uaccess.h>
+#include <linux/regset.h>
 
 
 /*
@@ -511,6 +512,47 @@ static int ptrace_resume(struct task_struct *child, long request, long data)
 	return 0;
 }
 
+#ifdef CONFIG_HAVE_ARCH_TRACEHOOK
+
+static const struct user_regset *
+find_regset(const struct user_regset_view *view, unsigned int type)
+{
+	const struct user_regset *regset;
+	int n;
+
+	for (n = 0; n < view->n; ++n) {
+		regset = view->regsets + n;
+		if (regset->core_note_type == type)
+			return regset;
+	}
+
+	return NULL;
+}
+
+static int ptrace_regset(struct task_struct *task, int req, unsigned int type,
+			 struct iovec *kiov)
+{
+	const struct user_regset_view *view = task_user_regset_view(task);
+	const struct user_regset *regset = find_regset(view, type);
+	int regset_no;
+
+	if (!regset || (kiov->iov_len % regset->size) != 0)
+		return -EIO;
+
+	regset_no = regset - view->regsets;
+	kiov->iov_len = min(kiov->iov_len,
+			    (__kernel_size_t) (regset->n * regset->size));
+
+	if (req == PTRACE_GETREGSET)
+		return copy_regset_to_user(task, view, regset_no, 0,
+					   kiov->iov_len, kiov->iov_base);
+	else
+		return copy_regset_from_user(task, view, regset_no, 0,
+					     kiov->iov_len, kiov->iov_base);
+}
+
+#endif
+
 int ptrace_request(struct task_struct *child, long request,
 		   long addr, long data)
 {
@@ -573,6 +615,26 @@ int ptrace_request(struct task_struct *child, long request,
 			return 0;
 		return ptrace_resume(child, request, SIGKILL);
 
+#ifdef CONFIG_HAVE_ARCH_TRACEHOOK
+	case PTRACE_GETREGSET:
+	case PTRACE_SETREGSET:
+	{
+		struct iovec kiov;
+		struct iovec __user *uiov = (struct iovec __user *) data;
+
+		if (!access_ok(VERIFY_WRITE, uiov, sizeof(*uiov)))
+			return -EFAULT;
+
+		if (__get_user(kiov.iov_base, &uiov->iov_base) ||
+		    __get_user(kiov.iov_len, &uiov->iov_len))
+			return -EFAULT;
+
+		ret = ptrace_regset(child, request, addr, &kiov);
+		if (!ret)
+			ret = __put_user(kiov.iov_len, &uiov->iov_len);
+		break;
+	}
+#endif
 	default:
 		break;
 	}
@@ -711,6 +773,32 @@ int compat_ptrace_request(struct task_struct *child, compat_long_t request,
 		else
 			ret = ptrace_setsiginfo(child, &siginfo);
 		break;
+#ifdef CONFIG_HAVE_ARCH_TRACEHOOK
+	case PTRACE_GETREGSET:
+	case PTRACE_SETREGSET:
+	{
+		struct iovec kiov;
+		struct compat_iovec __user *uiov =
+			(struct compat_iovec __user *) datap;
+		compat_uptr_t ptr;
+		compat_size_t len;
+
+		if (!access_ok(VERIFY_WRITE, uiov, sizeof(*uiov)))
+			return -EFAULT;
+
+		if (__get_user(ptr, &uiov->iov_base) ||
+		    __get_user(len, &uiov->iov_len))
+			return -EFAULT;
+
+		kiov.iov_base = compat_ptr(ptr);
+		kiov.iov_len = len;
+
+		ret = ptrace_regset(child, request, addr, &kiov);
+		if (!ret)
+			ret = __put_user(kiov.iov_len, &uiov->iov_len);
+		break;
+	}
+#endif
 
 	default:
 		ret = ptrace_request(child, request, addr, data);

Re: [tip:x86/ptrace] ptrace: Add support for generic PTRACE_GETREGSET/PTRACE_SETREGSET

[Ingo Molnar]

* tip-bot for Suresh Siddha <suresh.b.siddha@intel.com> wrote:

> Commit-ID:  2225a122ae26d542bdce523d9d87a4a7ba10e07b
> Gitweb:     http://git.kernel.org/tip/2225a122ae26d542bdce523d9d87a4a7ba10e07b
> Author:     Suresh Siddha <suresh.b.siddha@intel.com>
> AuthorDate: Thu, 11 Feb 2010 11:51:00 -0800
> Committer:  H. Peter Anvin <hpa@zytor.com>
> CommitDate: Thu, 11 Feb 2010 15:08:33 -0800
> 
> ptrace: Add support for generic PTRACE_GETREGSET/PTRACE_SETREGSET

FYI, this commit broke tip:master on PARISC (other architectures are fine):

 kernel/built-in.o: In function `ptrace_request':
 (.text.ptrace_request+0x2cc): undefined reference to `task_user_regset_view'

I'll keep them in tip:master to get them tested, but note that i cannot push 
any of these patches into linux-next until this is fixed, as linux-next 
requires all architectures to build, with no regard to which architectures are 
tested by kernel testers in practice.

	Ingo

linux-next requiements (Was: Re: [tip:x86/ptrace] ptrace: Add support for generic PTRACE_GETREGSET/PTRACE_SETREGSET)

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1161 bytes --]

Hi Ingo,

On Mon, 22 Feb 2010 10:07:10 +0100 Ingo Molnar <mingo@elte.hu> wrote:
>
> I'll keep them in tip:master to get them tested, but note that i cannot push 
> any of these patches into linux-next until this is fixed, as linux-next 
> requires all architectures to build, with no regard to which architectures are 
> tested by kernel testers in practice.

I merely expect people not to push known broken code into linux-next.
linux-next breaks on all sorts of architectures all the time (check
http://kisskb.ellerman.id.au/kisskb/branch/9/).  One of the reasons
linux-next exists is so that all kernel developers don't have to build
test on all architectures all the time.  (And, yes, I know that linux-next
doesn't build all architectures or all configs.)

That being said, I will complain about breakages on some architectures
more than others since they are in my personal builds (as opposed to the
over night builds).

That being said (:-)), I am sure that the PARISC guys would appreciate
the original problem being fixed.
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: linux-next requiements (Was: Re: [tip:x86/ptrace] ptrace: Add support for generic PTRACE_GETREGSET/PTRACE_SETREGSET)

[Ingo Molnar]

* Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> Hi Ingo,
> 
> On Mon, 22 Feb 2010 10:07:10 +0100 Ingo Molnar <mingo@elte.hu> wrote:
> >
> > I'll keep them in tip:master to get them tested, but note that i cannot 
> > push any of these patches into linux-next until this is fixed, as 
> > linux-next requires all architectures to build, with no regard to which 
> > architectures are tested by kernel testers in practice.
> 
> I merely expect people not to push known broken code into linux-next.

FYI, this 'mere' kind of indiscriminate definition of 'breakage' is what i am 
talking about.

The occasional driver build breakage can be tested relatively easily: one 
allyesconfig build and it's done. Build testing 22 architectures is 
exponentially harder: it requires the setup (and constant maintenance) of 
zillions of tool-chains, plus the build time is significant as well.

So this kind of linux-next requirement causes the over-testing of code that 
doesnt get all that much active usage, plus it increases build testing 
overhead 10-fold. That, by definition, causes the under-testing of code that 
_does_ matter a whole lot more to active testers of the Linux kernel.

Which is a problem, obviously.

	Ingo

Re: linux-next requirements (Was: Re: [tip:x86/ptrace] ptrace: Add support for generic PTRACE_GETREGSET/PTRACE_SETREGSET)

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 2346 bytes --]

Hi Ingo,

On Mon, 22 Feb 2010 11:27:45 +0100 Ingo Molnar <mingo@elte.hu> wrote:
>
> * Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> 
> > On Mon, 22 Feb 2010 10:07:10 +0100 Ingo Molnar <mingo@elte.hu> wrote:
> > >
> > > I'll keep them in tip:master to get them tested, but note that i cannot 
> > > push any of these patches into linux-next until this is fixed, as 
> > > linux-next requires all architectures to build, with no regard to which 
> > > architectures are tested by kernel testers in practice.
> > 
> > I merely expect people not to push known broken code into linux-next.
> 
> FYI, this 'mere' kind of indiscriminate definition of 'breakage' is what i am 
> talking about.

OK, let me remove "merely" from this.

I expect people not to push known broken code into linux-next.  Code in
linux-next is meant to be as ready as possible to be sent to Linus.  If
you know that it breaks some architecture then it should obviously be
fixed some how (unless the architecture maintainer really doesn't care,
of course).

This is different from not knowing that it breaks some architecture even
though you have done reasonable testing.

> The occasional driver build breakage can be tested relatively easily: one 
> allyesconfig build and it's done. Build testing 22 architectures is 
> exponentially harder: it requires the setup (and constant maintenance) of 
> zillions of tool-chains, plus the build time is significant as well.
> 
> So this kind of linux-next requirement causes the over-testing of code that 
> doesnt get all that much active usage, plus it increases build testing 
> overhead 10-fold. That, by definition, causes the under-testing of code that 
> _does_ matter a whole lot more to active testers of the Linux kernel.

Which is why linux-next does *not* require that. (Did you read the part
of my email that you removed?)  I do point out when build failures occur
(that is part of the point of linux-next after all) but they only upset
me when it is clear that the code that has been changed was not built at
all (which doesn't happen too often).
 
> Which is a problem, obviously.

It certainly would be.

Maybe I don't understand what you are trying to say.
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: linux-next requirements (Was: Re: [tip:x86/ptrace] ptrace: Add support for generic PTRACE_GETREGSET/PTRACE_SETREGSET)

[H. Peter Anvin]

On 02/22/2010 03:47 AM, Stephen Rothwell wrote:
>>
>> So this kind of linux-next requirement causes the over-testing of code that 
>> doesnt get all that much active usage, plus it increases build testing 
>> overhead 10-fold. That, by definition, causes the under-testing of code that 
>> _does_ matter a whole lot more to active testers of the Linux kernel.
> 
> Which is why linux-next does *not* require that. (Did you read the part
> of my email that you removed?)  I do point out when build failures occur
> (that is part of the point of linux-next after all) but they only upset
> me when it is clear that the code that has been changed was not built at
> all (which doesn't happen too often).
>  
>> Which is a problem, obviously.
> 
> It certainly would be.
> 
> Maybe I don't understand what you are trying to say.

Sounds like a big source of confusion to me.

Either which way, Roland has a mitigation patch -- which basically
disables the broken bits of PARISC until the PARISC maintainers fix it.
 What is the best way to handle that kind of stuff?

	-hpa

Re: linux-next requirements (Was: Re: [tip:x86/ptrace] ptrace: Add support for generic PTRACE_GETREGSET/PTRACE_SETREGSET)

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 956 bytes --]

Hi Peter,

On Mon, 22 Feb 2010 14:57:28 -0800 "H. Peter Anvin" <hpa@zytor.com> wrote:
>
> Either which way, Roland has a mitigation patch -- which basically
> disables the broken bits of PARISC until the PARISC maintainers fix it.
>  What is the best way to handle that kind of stuff?

Well, now that Roland has made at least one of the PARISC maintainers
aware of the problem, we could wait a little while to see if a solution
is forthcoming from them.  If not, then maybe Roland's patch could be
applied to the appropriate tip tree or we could just leave PARISC broken
in linux-next until they decide to fix it.  Note that some of the PARISC
builds are already broken in linux-next for other reasons.

Are there any downsides to Roland's patch as far as PARISC is concerned
(apart from the loss of some functionality, of course)?
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: linux-next requirements (Was: Re: [tip:x86/ptrace] ptrace: Add support for generic PTRACE_GETREGSET/PTRACE_SETREGSET)

[Roland McGrath]

> Are there any downsides to Roland's patch as far as PARISC is concerned
> (apart from the loss of some functionality, of course)?

Kyle ACK'd my parisc patch, and my impression is he wants it to go in
and does not plan to work on the necessary arch support imminently.

I think the only visible effect of turning off HAVE_ARCH_TRACEHOOK
on parisc will be that /proc/pid/syscall is not available.


Thanks,
Roland

Re: linux-next requirements (Was: Re: [tip:x86/ptrace] ptrace: Add support for generic PTRACE_GETREGSET/PTRACE_SETREGSET)

[H. Peter Anvin]

On 02/23/2010 12:20 PM, Roland McGrath wrote:
>> Are there any downsides to Roland's patch as far as PARISC is concerned
>> (apart from the loss of some functionality, of course)?
> 
> Kyle ACK'd my parisc patch, and my impression is he wants it to go in
> and does not plan to work on the necessary arch support imminently.
> 
> I think the only visible effect of turning off HAVE_ARCH_TRACEHOOK
> on parisc will be that /proc/pid/syscall is not available.

Added to tip:x86/ptrace.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: linux-next requirements (Was: Re: [tip:x86/ptrace] ptrace: Add support for generic PTRACE_GETREGSET/PTRACE_SETREGSET)

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 849 bytes --]

On Tue, 23 Feb 2010 12:49:42 -0800 "H. Peter Anvin" <hpa@zytor.com> wrote:
>
> On 02/23/2010 12:20 PM, Roland McGrath wrote:
> >> Are there any downsides to Roland's patch as far as PARISC is concerned
> >> (apart from the loss of some functionality, of course)?
> > 
> > Kyle ACK'd my parisc patch, and my impression is he wants it to go in
> > and does not plan to work on the necessary arch support imminently.
> > 
> > I think the only visible effect of turning off HAVE_ARCH_TRACEHOOK
> > on parisc will be that /proc/pid/syscall is not available.
> 
> Added to tip:x86/ptrace.

And Linus actually put it in his tree yesterday ... commit
15cbf627abcd93c3c668d5a92d58d9fec8f953dd "Revert "parisc:
HAVE_ARCH_TRACEHOOK""

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: linux-next requirements (Was: Re: [tip:x86/ptrace] ptrace: Add support for generic PTRACE_GETREGSET/PTRACE_SETREGSET)

[Ingo Molnar]

* Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> Hi Ingo,
> 
> On Mon, 22 Feb 2010 11:27:45 +0100 Ingo Molnar <mingo@elte.hu> wrote:
> >
> > * Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > 
> > > On Mon, 22 Feb 2010 10:07:10 +0100 Ingo Molnar <mingo@elte.hu> wrote:
> > > >
> > > > I'll keep them in tip:master to get them tested, but note that i cannot 
> > > > push any of these patches into linux-next until this is fixed, as 
> > > > linux-next requires all architectures to build, with no regard to which 
> > > > architectures are tested by kernel testers in practice.
> > > 
> > > I merely expect people not to push known broken code into linux-next.
> > 
> > FYI, this 'mere' kind of indiscriminate definition of 'breakage' is what i am 
> > talking about.
> 
> OK, let me remove "merely" from this.
> 
> I expect people not to push known broken code into linux-next.  Code in
> linux-next is meant to be as ready as possible to be sent to Linus.  If
> you know that it breaks some architecture then it should obviously be
> fixed some how (unless the architecture maintainer really doesn't care,
> of course).
> 
> This is different from not knowing that it breaks some architecture even
> though you have done reasonable testing.
> 
> > The occasional driver build breakage can be tested relatively easily: one 
> > allyesconfig build and it's done. Build testing 22 architectures is 
> > exponentially harder: it requires the setup (and constant maintenance) of 
> > zillions of tool-chains, plus the build time is significant as well.
> > 
> > So this kind of linux-next requirement causes the over-testing of code that 
> > doesnt get all that much active usage, plus it increases build testing 
> > overhead 10-fold. That, by definition, causes the under-testing of code that 
> > _does_ matter a whole lot more to active testers of the Linux kernel.
> 
> Which is why linux-next does *not* require that. [...]

How can you reconcile that with:

> I expect people not to push known broken code into linux-next.  Code in

?

So is it your point that technically you 'expect' but dont 'require'?

That's mostly word games really IMO, as in the end there's not much of a 
difference in practice: because you report build failures of non-mainstream 
architectures pretty much the same way as the main architectures.

I.e. indirectly you push up their importance, while taking away resources from 
the main architectures. Testing and maintainer attention is a finite resource, 
it's all a zero-sum game.

So in the end maintainers either cross-build to all architectures and avoid 
all the squeaky-wheel overhead of linux-next, or avoid pushing to it all that 
frequently. Which causes the collateral damage i mentioned:

> The occasional driver build breakage can be tested relatively easily: one 
> allyesconfig build and it's done. Build testing 22 architectures is 
> exponentially harder: it requires the setup (and constant maintenance) of 
> zillions of tool-chains, plus the build time is significant as well.
>
> So this kind of linux-next requirement causes the over-testing of code that 
> doesnt get all that much active usage, plus it increases build testing 
> overhead 10-fold. That, by definition, causes the under-testing of code that 
> _does_ matter a whole lot more to active testers of the Linux kernel.
>
> Which is a problem, obviously.

The solution? Stop this anti-real-world-usage bias already. Stop pretending 
that those cross-build results are as important as say the thousands of real 
bugzilla entries we have. They are fine info, but the kind of priority you are 
giving them is causing a waste of resources.

The thing is, testing whether the kernel still builds with gcc33 has more 
practical relevance to Linux users than testing about half of the 
architectures. The ancient NE2000 driver probably still has ten times more 
users than half of the architectures we have. Do you boot-test NE2000 with 
linux-next?

Developers simply cannot be expected to build for 22 architectures, and they 
shouldnt be. It's somewhat of a waste of time even on the subsystem maintainer 
level. (although it's certainly more contained there, plus subsystem 
maintainers generally have more hw resources as well.)

The thing is, last i checked you didnt even _test_ x86 as the first step in 
your linux-next build tests. Most of your generic build bug reports are 
against PowerPC. They create the appearance that x86 is a second class citizen 
in linux-next.

IMHO a generic tree like linux-next should be fundamentally neutral and its 
testing should be weighted _towards_ real Linux usage. You should try hard to 
avoid even the _apperance_ of pro-PowerPC (and anti-x86) bias - but AFAICS you 
dont even try.

Which i see as a problem.

Thanks,

	Ingo

Re: linux-next requirements (Was: Re: [tip:x86/ptrace] ptrace: Add support for generic PTRACE_GETREGSET/PTRACE_SETREGSET)

[Al Viro]

On Tue, Feb 23, 2010 at 09:45:52AM +0100, Ingo Molnar wrote:

> The solution? Stop this anti-real-world-usage bias already. Stop pretending 
> that those cross-build results are as important as say the thousands of real 
> bugzilla entries we have. They are fine info, but the kind of priority you are 
> giving them is causing a waste of resources.

Ho-hum...  "Kernel won't build for several thousand boxen"...  "Five lines
contain trailing whitespace"...  Yup, the latter is far higher priority,
all right.

Re: linux-next requirements (Was: Re: [tip:x86/ptrace] ptrace: Add support for generic PTRACE_GETREGSET/PTRACE_SETREGSET)

[Al Viro]

On Tue, Feb 23, 2010 at 07:52:14PM +0000, Al Viro wrote:
> On Tue, Feb 23, 2010 at 09:45:52AM +0100, Ingo Molnar wrote:
> 
> > The solution? Stop this anti-real-world-usage bias already. Stop pretending 
> > that those cross-build results are as important as say the thousands of real 
> > bugzilla entries we have. They are fine info, but the kind of priority you are 
> > giving them is causing a waste of resources.
> 
> Ho-hum...  "Kernel won't build for several thousand boxen"...  "Five lines
> contain trailing whitespace"...  Yup, the latter is far higher priority,
> all right.

IOW, I'd buy that argument from somebody who didn't protect checkpatch.pl
wankers against exactly that kind of criticism.

Re: linux-next requirements

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 2069 bytes --]

[I have removed linux-tip-commits from the cc list]

Hi Ingo,

On Tue, 23 Feb 2010 09:45:52 +0100 Ingo Molnar <mingo@elte.hu> wrote:
>
> Developers simply cannot be expected to build for 22 architectures, and they 
> shouldnt be.

I have agreed with this point of yours several times.  Why do you keep
stating it?

> The thing is, last i checked you didnt even _test_ x86 as the first step in 
> your linux-next build tests. Most of your generic build bug reports are 
> against PowerPC. They create the appearance that x86 is a second class citizen 
> in linux-next.

Lets see.  Over the last 60 days, I have reported 37 build errors.  Of
these, 16 were reported against x86, 14 against ppc, 7 against other
archs.  Of the ppc reports, 10 would not affect x86 builds (due to being
ppc specific problems or dependencies on implicit includes that do happen on
x86).  None of the reports against other arches would affect x86 builds.

I also reported 31 warnings.  15 against x86, 15 against ppc and 1 against
both.  Of those only reported against ppc, 13 did not affect x86.

So of my "generic" reports, 4 errors and 2 warnings were reported against
ppc, 16 errors and 15 warnings again x86.

Also, I am not sure how reports of 37 build errors and 32 warnings over
60 days can tax the resources of our developer base.  Most of these are
fairly trivial to fix (as is shown by how quick they are fixed.  Usually
the developer has just forgotten to test the !CONFIG_SOMETHING case or
used some function without explicitly including the file that declares it.

As to my perceived pro-PowerPC and anti-x86 bias, you are the only one who
has even mentioned it to me.

Anyway, I sick of these discussions.  If people see the way I do
linux-next as a problem, then they can find someone else.  That is not
the impression I gained at the Kernel Summit and (apart from these
occasional "discussions") I am quite happy to continue.
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: linux-next requirements

[Grant Likely]

On Wed, Feb 24, 2010 at 12:25 AM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> Anyway, I sick of these discussions.  If people see the way I do
> linux-next as a problem, then they can find someone else.  That is not
> the impression I gained at the Kernel Summit and (apart from these
> occasional "discussions") I am quite happy to continue.

Please don't stop.  I'd be screwed.

g.

Re: linux-next requirements

[Geert Uytterhoeven]

On Sat, Feb 27, 2010 at 02:53, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Feb 24, 2010 at 12:25 AM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>> Anyway, I sick of these discussions.  If people see the way I do
>> linux-next as a problem, then they can find someone else.  That is not
>> the impression I gained at the Kernel Summit and (apart from these
>> occasional "discussions") I am quite happy to continue.
>
> Please don't stop.  I'd be screwed.

Yes, I like linux-next, so
Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>

(This ack doesn't necessarily apply to the rest of the discussion that
happened on
 linux-tip-commits before. I'm gonna pretend I didn't read it ;-).

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: linux-next requirements

[Jaswinder Singh Rajput]

Hello Stephen,

On Wed, Feb 24, 2010 at 12:55 PM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> Anyway, I sick of these discussions.  If people see the way I do
> linux-next as a problem, then they can find someone else.  That is not
> the impression I gained at the Kernel Summit and (apart from these
> occasional "discussions") I am quite happy to continue.

You are doing great job. linux-next is also very useful before
submitting the patch as we can see all the changes under one roof.
Please do not get upset with useless discussions and continue with
your great work.

Thanks and big Cheers :-)
--
Jaswinder.

Re: linux-next requirements

[Ingo Molnar]

* Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> [I have removed linux-tip-commits from the cc list]
> 
> Hi Ingo,
> 
> On Tue, 23 Feb 2010 09:45:52 +0100 Ingo Molnar <mingo@elte.hu> wrote:
> >
> > Developers simply cannot be expected to build for 22 architectures, and 
> > they shouldnt be.
> 
> I have agreed with this point of yours several times.  Why do you keep 
> stating it?

If you agree with me then why do you put so much focus on cross-arch build 
failures, versus other, more relevant forms of testing?

> > The thing is, last i checked you didnt even _test_ x86 as the first step 
> > in your linux-next build tests. Most of your generic build bug reports are 
> > against PowerPC. They create the appearance that x86 is a second class 
> > citizen in linux-next.
> 
> Lets see.  Over the last 60 days, I have reported 37 build errors.  Of 
> these, 16 were reported against x86, 14 against ppc, 7 against other archs.

So only 43% of them were even relevant on the platform that 95+% of the Linux 
testers use? Seems to support the points i made.

> Of the ppc reports, 10 would not affect x86 builds (due to being ppc 
> specific problems or dependencies on implicit includes that do happen on 
> x86).  None of the reports against other arches would affect x86 builds.
> 
> I also reported 31 warnings.  15 against x86, 15 against ppc and 1 against 
> both.  Of those only reported against ppc, 13 did not affect x86.
> 
> So of my "generic" reports, 4 errors and 2 warnings were reported against 
> ppc, 16 errors and 15 warnings again x86.
> 
> Also, I am not sure how reports of 37 build errors and 32 warnings over 60 
> days can tax the resources of our developer base. [...]

Note that out of those 37 build errors only a small minority were caused by 
any tree i co-maintain. (i dont have the precise numbers but it's below 5)

Why? Because i cross-build before pushing to linux-next. I bug people about 
cross-arch build failures, and about the patch flow delays and hickups this 
causes. Without that you'd see twice that many cross-build failures.

Which in itself is not bad of course (any fix is a good fix) - except the 
forced prioritization and its place in the workflow: it sends the wrong 
testing message.

It sends the message that building on N architectures is more important than 
for the code to work for real people. I've had good developers waste their 
time trying to set up cross-build testing environments and complain to me how 
this complicates their testing.

> [...]  Most of these are fairly trivial to fix (as is shown by how quick 
> they are fixed.  Usually the developer has just forgotten to test the 
> !CONFIG_SOMETHING case or used some function without explicitly including 
> the file that declares it.
> 
> As to my perceived pro-PowerPC and anti-x86 bias, you are the only one who 
> has even mentioned it to me.

Have you asked me recently for example?

> Anyway, I sick of these discussions.  If people see the way I do linux-next 
> as a problem, then they can find someone else.  That is not the impression I 
> gained at the Kernel Summit and (apart from these occasional "discussions") 
> I am quite happy to continue.

Not sure how you jump from my observations to "I will quit if you do this". I 
am simply pointing out problems as i see them - as i do that with every piece 
of the workflow we use. I have expressed my views numerous times about where i 
find linux-next useful and positive - and it's sure a net positive.

	Ingo

Re: linux-next requirements

[Rafael J. Wysocki]

On Saturday 27 February 2010, Ingo Molnar wrote:
> 
> * Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> 
> > [I have removed linux-tip-commits from the cc list]
> > 
> > Hi Ingo,
> > 
> > On Tue, 23 Feb 2010 09:45:52 +0100 Ingo Molnar <mingo@elte.hu> wrote:
> > >
> > > Developers simply cannot be expected to build for 22 architectures, and 
> > > they shouldnt be.
> > 
> > I have agreed with this point of yours several times.  Why do you keep 
> > stating it?
> 
> If you agree with me then why do you put so much focus on cross-arch build 
> failures, versus other, more relevant forms of testing?

I don't really know what this is all about.  Stephen does what he can and
that's generally appreciated very much.  It helps to make sure the code builds
correctly on the architectures it's supposed to build on and there's nothing
wrong with that IMO.

> > > The thing is, last i checked you didnt even _test_ x86 as the first step 
> > > in your linux-next build tests. Most of your generic build bug reports are 
> > > against PowerPC. They create the appearance that x86 is a second class 
> > > citizen in linux-next.
> > 
> > Lets see.  Over the last 60 days, I have reported 37 build errors.  Of 
> > these, 16 were reported against x86, 14 against ppc, 7 against other archs.
> 
> So only 43% of them were even relevant on the platform that 95+% of the Linux 
> testers use? Seems to support the points i made.

Well, I hope you don't mean that because the majority of bug reporters (vs
testers, the number of whom is unknown to me at least) use x86, we are free
to break the other architectures. ;-)

> > Of the ppc reports, 10 would not affect x86 builds (due to being ppc 
> > specific problems or dependencies on implicit includes that do happen on 
> > x86).  None of the reports against other arches would affect x86 builds.
> > 
> > I also reported 31 warnings.  15 against x86, 15 against ppc and 1 against 
> > both.  Of those only reported against ppc, 13 did not affect x86.
> > 
> > So of my "generic" reports, 4 errors and 2 warnings were reported against 
> > ppc, 16 errors and 15 warnings again x86.
> > 
> > Also, I am not sure how reports of 37 build errors and 32 warnings over 60 
> > days can tax the resources of our developer base. [...]
> 
> Note that out of those 37 build errors only a small minority were caused by 
> any tree i co-maintain. (i dont have the precise numbers but it's below 5)
> 
> Why? Because i cross-build before pushing to linux-next. I bug people about 
> cross-arch build failures, and about the patch flow delays and hickups this 
> causes. Without that you'd see twice that many cross-build failures.
> 
> Which in itself is not bad of course (any fix is a good fix) - except the 
> forced prioritization and its place in the workflow: it sends the wrong 
> testing message.
> 
> It sends the message that building on N architectures is more important than 
> for the code to work for real people. I've had good developers waste their 
> time trying to set up cross-build testing environments and complain to me how 
> this complicates their testing.

That's the kind of task linux-next is really good at AFAICT.  Before linux-next
I used to have a cross-build testing environment like this, but I don't need it
any more, because I know linux-next will catch the cross-build problems for
me and I appreciate that very much, because it saves a lot of my time.

Rafael

Re: linux-next requirements

[Ingo Molnar]

* Rafael J. Wysocki <rjw@sisk.pl> wrote:

> > > Lets see.  Over the last 60 days, I have reported 37 build errors.  Of 
> > > these, 16 were reported against x86, 14 against ppc, 7 against other 
> > > archs.
> > 
> > So only 43% of them were even relevant on the platform that 95+% of the 
> > Linux testers use? Seems to support the points i made.
> 
> Well, I hope you don't mean that because the majority of bug reporters (vs 
> testers, the number of whom is unknown to me at least) use x86, we are free 
> to break the other architectures. ;-)

It means exactly that: just like we 'can' break compilation with gcc296, 
ancient versions of binutils, odd bootloaders, can break the boot via odd 
hardware, etc. When someone uses that architectures then the 'easy' bugfixes 
will actually flow in very quickly and without much fuss - and without 
burdening developers to consider cases they have no good ways to test. Why 
should rare architectures be more important than those other rare forms of 
Linux usage?

In fact those rare ways of building and booting the kernel i mentioned are 
probably used _more_ than half of the architectures that linux-next 
build-tests ...

So yes, of course _all_ bugs need fixing if there's enough capacity, but the 
process in general should be healthy, low-overhead and shouldnt concentrate on 
an irrelevant portion of Linux usage in such a prominent way.

Or, if it does, it should _first_ cover the other, much more burning areas of 
testing interest. All the while our _real_ bugreports are often rotting on 
bugzilla.kernel.org ...

Thanks,

	Ingo

Re: linux-next requirements

[Rafael J. Wysocki]

On Saturday 27 February 2010, Ingo Molnar wrote:
> 
> * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> 
> > > > Lets see.  Over the last 60 days, I have reported 37 build errors.  Of 
> > > > these, 16 were reported against x86, 14 against ppc, 7 against other 
> > > > archs.
> > > 
> > > So only 43% of them were even relevant on the platform that 95+% of the 
> > > Linux testers use? Seems to support the points i made.
> > 
> > Well, I hope you don't mean that because the majority of bug reporters (vs 
> > testers, the number of whom is unknown to me at least) use x86, we are free 
> > to break the other architectures. ;-)
> 
> It means exactly that: just like we 'can' break compilation with gcc296, 
> ancient versions of binutils, odd bootloaders, can break the boot via odd 
> hardware, etc. When someone uses that architectures then the 'easy' bugfixes 
> will actually flow in very quickly and without much fuss

Then I don't understand what the problem with getting them in at the linux-next
stage is.  They are necessary anyway, so we'll need to add them sooner or
later and IMO the sooner the better.

Apart from this, that cross-build issues aren't always "easy" and sometimes
they take quite some time and engineering effort to resolve.  IMO that's better
done at the linux-next stage than during a merge window.

> - and without burdening developers to consider cases they have no good ways
> to test.  Why should rare architectures be more important than those other
> rare forms of Linux usage?

Because the Linus' tree is supposed to build on those architectures.  As long
as that's the case, linux-next should build on them too.

> In fact those rare ways of building and booting the kernel i mentioned are 
> probably used _more_ than half of the architectures that linux-next 
> build-tests ...

I don't know and you don't know either.  That's just pure speculation and
therefore meaningless.

> So yes, of course _all_ bugs need fixing if there's enough capacity, but the 
> process in general should be healthy, low-overhead and shouldnt concentrate on 
> an irrelevant portion of Linux usage in such a prominent way.
> 
> Or, if it does, it should _first_ cover the other, much more burning areas of 
> testing interest. All the while our _real_ bugreports are often rotting on 
> bugzilla.kernel.org ...

All right.  There are two _separate_ questions to ask IMO:

(1) Do we need the kind of community service that Stephen has been doing?

(2) Do we need more testing of linux-next and if so, who's task should that be?

I think you agree that the aswer to (1) is "yes, we do".  So _someone_ has to
do it and I'm very grateful to Stephen for taking care of it.

[Thanks, Stephen!]

Now, the part of this service is to check that the resulting tree will actually
build in all conditions it's supposed to build in, if possible, or the whole
merging exercise wouldn't have much practical meaning.  Stephen has been
doing just that and IMO to a good result.

To some extent, though, that's a matter of defining in what conditions the
kernel is supposed to build in, but I think for linux-next these conditions
should be the same as for the Linus' tree, for the simple reason that
linux-next is supposed to be a "future snapshot" of it.   So linux-next should
build on all architectures that the future Linus' tree is supposed to build on.
Even on "exotic" ones.

[IMO that's actually important, because such corner cases tend to reveal
runtime bugs we wouldn't have been aware of otherwise.  Now, in the majority
of cases a casual tester will be discouraged by the kernel not compiling for
him while he might have found a "real" bug otherwise.]

Now, as far as (2) and is concerned, I think the answer here is also "yes, we
do", but that's not a part of the Stephen's job.

Thanks,
Rafael

Re: linux-next requirements

[Geert Uytterhoeven]

On Sat, Feb 27, 2010 at 20:07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Saturday 27 February 2010, Ingo Molnar wrote:
>> * Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>
>> > > > Lets see.  Over the last 60 days, I have reported 37 build errors.  Of
>> > > > these, 16 were reported against x86, 14 against ppc, 7 against other
>> > > > archs.
>> > >
>> > > So only 43% of them were even relevant on the platform that 95+% of the
>> > > Linux testers use? Seems to support the points i made.
>> >
>> > Well, I hope you don't mean that because the majority of bug reporters (vs
>> > testers, the number of whom is unknown to me at least) use x86, we are free
>> > to break the other architectures. ;-)
>>
>> It means exactly that: just like we 'can' break compilation with gcc296,
>> ancient versions of binutils, odd bootloaders, can break the boot via odd
>> hardware, etc. When someone uses that architectures then the 'easy' bugfixes
>> will actually flow in very quickly and without much fuss
>
> Then I don't understand what the problem with getting them in at the linux-next
> stage is.  They are necessary anyway, so we'll need to add them sooner or
> later and IMO the sooner the better.
>
> Apart from this, that cross-build issues aren't always "easy" and sometimes
> they take quite some time and engineering effort to resolve.  IMO that's better
> done at the linux-next stage than during a merge window.
>
>> - and without burdening developers to consider cases they have no good ways
>> to test.  Why should rare architectures be more important than those other
>> rare forms of Linux usage?
>
> Because the Linus' tree is supposed to build on those architectures.  As long
> as that's the case, linux-next should build on them too.
>
>> In fact those rare ways of building and booting the kernel i mentioned are
>> probably used _more_ than half of the architectures that linux-next
>> build-tests ...
>
> I don't know and you don't know either.  That's just pure speculation and
> therefore meaningless.

If only the CE Linux Forum member companies would publish figures about the
number of Linux devices they push onto the world population...

Yes I know, this still excludes `obsolete' architectures like parisc
and alpha, but it would
change the balance towards x86 (and powerpc?) drastically.

>> So yes, of course _all_ bugs need fixing if there's enough capacity, but the
>> process in general should be healthy, low-overhead and shouldnt concentrate on
>> an irrelevant portion of Linux usage in such a prominent way.
>>
>> Or, if it does, it should _first_ cover the other, much more burning areas of
>> testing interest. All the while our _real_ bugreports are often rotting on
>> bugzilla.kernel.org ...
>
> All right.  There are two _separate_ questions to ask IMO:
>
> (1) Do we need the kind of community service that Stephen has been doing?
>
> (2) Do we need more testing of linux-next and if so, who's task should that be?
>
> I think you agree that the aswer to (1) is "yes, we do".  So _someone_ has to
> do it and I'm very grateful to Stephen for taking care of it.
>
> [Thanks, Stephen!]
>
> Now, the part of this service is to check that the resulting tree will actually
> build in all conditions it's supposed to build in, if possible, or the whole
> merging exercise wouldn't have much practical meaning.  Stephen has been
> doing just that and IMO to a good result.
>
> To some extent, though, that's a matter of defining in what conditions the
> kernel is supposed to build in, but I think for linux-next these conditions
> should be the same as for the Linus' tree, for the simple reason that
> linux-next is supposed to be a "future snapshot" of it.   So linux-next should
> build on all architectures that the future Linus' tree is supposed to build on.
> Even on "exotic" ones.
>
> [IMO that's actually important, because such corner cases tend to reveal
> runtime bugs we wouldn't have been aware of otherwise.  Now, in the majority
> of cases a casual tester will be discouraged by the kernel not compiling for
> him while he might have found a "real" bug otherwise.]
>
> Now, as far as (2) and is concerned, I think the answer here is also "yes, we
> do", but that's not a part of the Stephen's job.

While wearing my m68k hat, I can say that I suffer an order of
magnitude more from
build failures than from boot failures. So I'm inclined to agree with
Linus when he says
`if it compiles, it's great; if it boots, it's perfect' :-)

Or perhaps this says more about our review process: we're quite good at catching
logical errors in our code, and worse at catching syntax and dependency errors.
Fortunately we have tools (and linux-next) to catch those...

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: linux-next requirements

[Rafael J. Wysocki]

On Saturday 27 February 2010, Geert Uytterhoeven wrote:
> On Sat, Feb 27, 2010 at 20:07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Saturday 27 February 2010, Ingo Molnar wrote:
> >> * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >>
> >> > > > Lets see.  Over the last 60 days, I have reported 37 build errors.  Of
> >> > > > these, 16 were reported against x86, 14 against ppc, 7 against other
> >> > > > archs.
> >> > >
> >> > > So only 43% of them were even relevant on the platform that 95+% of the
> >> > > Linux testers use? Seems to support the points i made.
> >> >
> >> > Well, I hope you don't mean that because the majority of bug reporters (vs
> >> > testers, the number of whom is unknown to me at least) use x86, we are free
> >> > to break the other architectures. ;-)
> >>
> >> It means exactly that: just like we 'can' break compilation with gcc296,
> >> ancient versions of binutils, odd bootloaders, can break the boot via odd
> >> hardware, etc. When someone uses that architectures then the 'easy' bugfixes
> >> will actually flow in very quickly and without much fuss
> >
> > Then I don't understand what the problem with getting them in at the linux-next
> > stage is.  They are necessary anyway, so we'll need to add them sooner or
> > later and IMO the sooner the better.
> >
> > Apart from this, that cross-build issues aren't always "easy" and sometimes
> > they take quite some time and engineering effort to resolve.  IMO that's better
> > done at the linux-next stage than during a merge window.
> >
> >> - and without burdening developers to consider cases they have no good ways
> >> to test.  Why should rare architectures be more important than those other
> >> rare forms of Linux usage?
> >
> > Because the Linus' tree is supposed to build on those architectures.  As long
> > as that's the case, linux-next should build on them too.
> >
> >> In fact those rare ways of building and booting the kernel i mentioned are
> >> probably used _more_ than half of the architectures that linux-next
> >> build-tests ...
> >
> > I don't know and you don't know either.  That's just pure speculation and
> > therefore meaningless.
> 
> If only the CE Linux Forum member companies would publish figures about the
> number of Linux devices they push onto the world population...
> 
> Yes I know, this still excludes `obsolete' architectures like parisc
> and alpha, but it would
> change the balance towards x86 (and powerpc?) drastically.

You apparently forgot about ARM.

Rafael

Re: linux-next requirements

[Ingo Molnar]

* Rafael J. Wysocki <rjw@sisk.pl> wrote:

> On Saturday 27 February 2010, Ingo Molnar wrote:
> > 
> > * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > 
> > > > > Lets see.  Over the last 60 days, I have reported 37 build errors.  Of 
> > > > > these, 16 were reported against x86, 14 against ppc, 7 against other 
> > > > > archs.
> > > > 
> > > > So only 43% of them were even relevant on the platform that 95+% of the 
> > > > Linux testers use? Seems to support the points i made.
> > > 
> > > Well, I hope you don't mean that because the majority of bug reporters (vs 
> > > testers, the number of whom is unknown to me at least) use x86, we are free 
> > > to break the other architectures. ;-)
> > 
> > It means exactly that: just like we 'can' break compilation with gcc296, 
> > ancient versions of binutils, odd bootloaders, can break the boot via odd 
> > hardware, etc. When someone uses that architectures then the 'easy' 
> > bugfixes will actually flow in very quickly and without much fuss
> 
> Then I don't understand what the problem with getting them in at the 
> linux-next stage is.  They are necessary anyway, so we'll need to add them 
> sooner or later and IMO the sooner the better.

The problem is the dynamics and resulting (non-)cleanliness of code. We have 
architectures that have been conceptually broken for 5 years or more, but 
still those problems get blamed on the last change that 'causes' the breakage: 
the core kernel and the developers who try to make a difference.

I think your perspective and your opinion is correct, while my perspective is 
real and correct as well - there's no contradiction really. Let me try to 
explain how i see it:

You are working in a relatively well-designed piece of code which interfaces 
to the kernel in sane ways - kernel/power/* et al. You might break the 
cross-builds sometimes, but it's not very common, and in those cases it's 
usually your own fault and you are grateful for linux-next to have caught that 
stupidity. (i hope this a fair summary!)

I am not criticising that aspect of linux-next _at all_ - it's useful and 
beneficial - and i'd like to thank Stephen for all his hard work. Other 
aspects of linux-next useful as well: such as the patch conflict mediation 
role.

But as it happens so often, people tend to talk more about the things that are 
not so rosy, not about the things that work well.

The area i am worried about are new core kernel facilities and their 
development and extension of existing facilities. _Those_ facilities are 
affected by 'many architectures' in a different way from how you experience 
it: often we can do very correct changes to them, which still 'break' on some 
architecture due to _that architecture's conceptual fault_.

Let me give you an example that happened just yesterday. My cross-testing 
found that a change in the tracing infrastructure code broke m32r and parisc.

The breakage:

 /home/mingo/tip/kernel/trace/trace_clock.c:86: error: implicit declaration of function 'raw_local_irq_save'
 /home/mingo/tip/kernel/trace/trace_clock.c:112: error: implicit declaration of function 'raw_local_irq_restore'
 make[3]: *** [kernel/trace/trace_clock.o] Error 1
 make[3]: *** Waiting for unfinished jobs....

Is was 'caused by':

 18b4a4d: oprofile: remove tracing build dependency

In linux-next this would be pinned to commit 18b4a4d, which would have to be 
reverted/fixed.

Where does the _real_ blame lie? Clearly in the M32R and HP/PARISC code: why 
dont they, four years after it has been introduced as a core kernel facility 
in 2006, _still_ not support raw_local_irq_save()?

( A similar situation occured in this very thread a well - before the subject 
  of the thread - so it's a real and present problem. We didnt even get _any_ 
  reaction about that particular breakage from the affected architecture ... )

These situations are magnified by how certain linux-next bugs are reported: 
the 'blame' is put on the new commit that exposes that laggy nature of certain 
architectures. Often the developers even believe this false notion and feel 
guilty for 'having broken' an architecture - often an architecture that has 
not contributed a single core kernel facility _in its whole existence_.

The usual end result is that the path of least resistance is taken: the commit 
is reverted or worked around, while the 'laggy' architecture can continue 
business as usual and cause more similar bugs and hickups in the future ...

I.e. there is extra overhead put on clearly 'good' efforts, while 'bad' 
behavior (parasitic hanging-on, passivity, indifference) is rewarded. 
Rewarding bad behavior is very clearly harmful to Linux in many regards, and i 
speak up when i see it.

So i wish linux-next balanced these things more fairly towards those areas of 
code that are actually useful: if it ignored build breakages that are due to 
architectures being lazy - in fact if it required architectures to _help out_ 
with the development of the kernel.

The majority of build-bugs i see trigger in cross-builds (90% of which i catch 
before they get into linux-next) are of this nature, that's why i raised it in 
such a pointed way. Your (and many other people's) experience will differ - so 
you might see this as an unjustified criticism.

Thanks,

	Ingo

Re: linux-next requirements

[Rafael J. Wysocki]

On Sunday 28 February 2010, Ingo Molnar wrote:
> 
> * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> 
> > On Saturday 27 February 2010, Ingo Molnar wrote:
> > > 
> > > * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > 
> > > > > > Lets see.  Over the last 60 days, I have reported 37 build errors.  Of 
> > > > > > these, 16 were reported against x86, 14 against ppc, 7 against other 
> > > > > > archs.
> > > > > 
> > > > > So only 43% of them were even relevant on the platform that 95+% of the 
> > > > > Linux testers use? Seems to support the points i made.
> > > > 
> > > > Well, I hope you don't mean that because the majority of bug reporters (vs 
> > > > testers, the number of whom is unknown to me at least) use x86, we are free 
> > > > to break the other architectures. ;-)
> > > 
> > > It means exactly that: just like we 'can' break compilation with gcc296, 
> > > ancient versions of binutils, odd bootloaders, can break the boot via odd 
> > > hardware, etc. When someone uses that architectures then the 'easy' 
> > > bugfixes will actually flow in very quickly and without much fuss
> > 
> > Then I don't understand what the problem with getting them in at the 
> > linux-next stage is.  They are necessary anyway, so we'll need to add them 
> > sooner or later and IMO the sooner the better.
> 
> The problem is the dynamics and resulting (non-)cleanliness of code. We have 
> architectures that have been conceptually broken for 5 years or more, but 
> still those problems get blamed on the last change that 'causes' the breakage: 
> the core kernel and the developers who try to make a difference.
> 
> I think your perspective and your opinion is correct, while my perspective is 
> real and correct as well - there's no contradiction really. Let me try to 
> explain how i see it:
> 
> You are working in a relatively well-designed piece of code which interfaces 
> to the kernel in sane ways - kernel/power/* et al. You might break the 
> cross-builds sometimes, but it's not very common, and in those cases it's 
> usually your own fault and you are grateful for linux-next to have caught that 
> stupidity. (i hope this a fair summary!)

Fair enough.

> I am not criticising that aspect of linux-next _at all_ - it's useful and 
> beneficial - and i'd like to thank Stephen for all his hard work. Other 
> aspects of linux-next useful as well: such as the patch conflict mediation 
> role.

Great.

> But as it happens so often, people tend to talk more about the things that are 
> not so rosy, not about the things that work well.
> 
> The area i am worried about are new core kernel facilities and their 
> development and extension of existing facilities. _Those_ facilities are 
> affected by 'many architectures' in a different way from how you experience 
> it: often we can do very correct changes to them, which still 'break' on some 
> architecture due to _that architecture's conceptual fault_.
> 
> Let me give you an example that happened just yesterday. My cross-testing 
> found that a change in the tracing infrastructure code broke m32r and parisc.
> 
> The breakage:
> 
>  /home/mingo/tip/kernel/trace/trace_clock.c:86: error: implicit declaration of function 'raw_local_irq_save'
>  /home/mingo/tip/kernel/trace/trace_clock.c:112: error: implicit declaration of function 'raw_local_irq_restore'
>  make[3]: *** [kernel/trace/trace_clock.o] Error 1
>  make[3]: *** Waiting for unfinished jobs....
> 
> Is was 'caused by':
> 
>  18b4a4d: oprofile: remove tracing build dependency
> 
> In linux-next this would be pinned to commit 18b4a4d, which would have to be 
> reverted/fixed.
> 
> Where does the _real_ blame lie? Clearly in the M32R and HP/PARISC code: why 
> dont they, four years after it has been introduced as a core kernel facility 
> in 2006, _still_ not support raw_local_irq_save()?

OK, I see your point.

> ( A similar situation occured in this very thread a well - before the subject 
>   of the thread - so it's a real and present problem. We didnt even get _any_ 
>   reaction about that particular breakage from the affected architecture ... )
> 
> These situations are magnified by how certain linux-next bugs are reported: 
> the 'blame' is put on the new commit that exposes that laggy nature of certain 
> architectures. Often the developers even believe this false notion and feel 
> guilty for 'having broken' an architecture - often an architecture that has 
> not contributed a single core kernel facility _in its whole existence_.
> 
> The usual end result is that the path of least resistance is taken: the commit 
> is reverted or worked around, while the 'laggy' architecture can continue 
> business as usual and cause more similar bugs and hickups in the future ...
> 
> I.e. there is extra overhead put on clearly 'good' efforts, while 'bad' 
> behavior (parasitic hanging-on, passivity, indifference) is rewarded. 
> Rewarding bad behavior is very clearly harmful to Linux in many regards, and i 
> speak up when i see it.
> 
> So i wish linux-next balanced these things more fairly towards those areas of 
> code that are actually useful: if it ignored build breakages that are due to 
> architectures being lazy - in fact if it required architectures to _help out_ 
> with the development of the kernel.
> 
> The majority of build-bugs i see trigger in cross-builds (90% of which i catch 
> before they get into linux-next) are of this nature, that's why i raised it in 
> such a pointed way. Your (and many other people's) experience will differ - so 
> you might see this as an unjustified criticism.

Thanks a lot for the clarification.

Best,
Rafael

Re: linux-next requirements

[Ingo Molnar]

* Rafael J. Wysocki <rjw@sisk.pl> wrote:

> > - and without burdening developers to consider cases they have no good 
> > ways to test.  Why should rare architectures be more important than those 
> > other rare forms of Linux usage?
> 
> Because the Linus' tree is supposed to build on those architectures. [...]

That's not actually true: Linus on multiple occasions has said that only the 
major architectures (x86, powerpc, ARM and a few others) are 'required' to 
build and that the others should be left to fail to build and should be 
_forced to get their act together_.

> [...]  As long as that's the case, linux-next should build on them too.

No, and IMO linux-next is clearly over-interpreting this bit. Linux is not 
supposed to build on all architectures. Maybe that's a core bit of a 
misunderstanding (on either my or on sfr's side) and it should be clarified 
...

	Ingo

Re: linux-next requirements

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1500 bytes --]

Hi Ingo,

On Sun, 28 Feb 2010 08:14:05 +0100 Ingo Molnar <mingo@elte.hu> wrote:
>
> > [...]  As long as that's the case, linux-next should build on them too.
> 
> No, and IMO linux-next is clearly over-interpreting this bit. Linux is not 
> supposed to build on all architectures. Maybe that's a core bit of a 
> misunderstanding (on either my or on sfr's side) and it should be clarified 
> ...

Well, we have no real problem then.  The only architectures for which a
failure will stop new stuff getting into linux-next are the ones I
personally build while constructing the tree (x86, ppc and sparc).  Once
something is in linux-next, even if it causes a build failure overnight,
I am loath to remove it again as it can cause pain for Andrew (who bases
-mm on linux-next).

I will still report such failures (if I have time to notice them - I
mostly hope that architecture maintainers will have a glance over the
build results themselves) and others do as well but such failures do not
generally cause any actions on my part (except in rare cases I may
actually fix the problem or put a provided fix patch in linux-next).

I would like to add arm to the mix of the architectures I build during
construction, but there is no wide ranging config that builds for arm and
building a few of the configs would just end up taking too much time.

Thanks for clarifying.
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: linux-next requirements

[Ingo Molnar]

* Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> Hi Ingo,
> 
> On Sun, 28 Feb 2010 08:14:05 +0100 Ingo Molnar <mingo@elte.hu> wrote:
> >
> > > [...]  As long as that's the case, linux-next should build on them too.
> > 
> > No, and IMO linux-next is clearly over-interpreting this bit. Linux is not 
> > supposed to build on all architectures. Maybe that's a core bit of a 
> > misunderstanding (on either my or on sfr's side) and it should be clarified 
> > ...
> 
> Well, we have no real problem then.  The only architectures for which a 
> failure will stop new stuff getting into linux-next are the ones I 
> personally build while constructing the tree (x86, ppc and sparc).  Once 
> something is in linux-next, even if it causes a build failure overnight, I 
> am loath to remove it again as it can cause pain for Andrew (who bases -mm 
> on linux-next).

Ok - very good. This has apparently been relaxed some time ago, i know 
linux-next used to be more stringent.

> I will still report such failures (if I have time to notice them - I mostly 
> hope that architecture maintainers will have a glance over the build results 
> themselves) and others do as well but such failures do not generally cause 
> any actions on my part (except in rare cases I may actually fix the problem 
> or put a provided fix patch in linux-next).

Yeah. Plus it's never black and white - sometimes a rare arch will show some 
real crappiness in a commit. So we want to know all bugs.

> I would like to add arm to the mix of the architectures I build during 
> construction, but there is no wide ranging config that builds for arm and 
> building a few of the configs would just end up taking too much time.

Yeah, ARM is clearly important from a usage share POV IMHO, and it's also 
actively driving many areas of interest.

It's also a bit difficult to keep ARM going because there's so many 
non-standardized hw variants of ARM, so i'm sure the ARM folks will appreciate 
us not breaking them ...

( Alas, ARM doesnt tend to be a big problem, at least as far as the facilities 
  i'm concerned about go: it has implemented most of the core kernel 
  infrastructures so there's few if any 'self inflicted' breakages that i can 
  remember. )

> Thanks for clarifying.

Thanks,

	Ingo

Re: linux-next requirements

[Al Viro]

On Sun, Feb 28, 2010 at 08:51:05AM +0100, Ingo Molnar wrote:

> ( Alas, ARM doesnt tend to be a big problem, at least as far as the facilities 
>   i'm concerned about go: it has implemented most of the core kernel 
>   infrastructures so there's few if any 'self inflicted' breakages that i can 
>   remember. )

FWIW, it might make sense to run cross-builds for many targets and post
the things that crop up + analysis to linux-arch...  Any takers?

I haven't run a lot of cross-builds lately, but IME most of the breakage
tends to be less dramatic - somebody relying on indirect includes in
driver *or* forgetting to add "depends on" to Kconfig used to be the
most frequent case.

"let other targets rot" attitude has a very nasty effect - it snowballs.
At some point people *can't* check that their patches don't break things,
even if they want to.  And that, IMO, sucks.  At that point architecture
needs to be either removed or brought to the state when it builds in
mainline.

Note that we have filesystems that are built only on some architectures.
I don't know about you, but I *do* care about not leaving half-converted
interfaces in that area.  For entirely rational reasons - people tend
to copy b0rken code from random places in the tree.  Playing whack-a-mole
gets old pretty soon.

Re: linux-next requirements

[Ingo Molnar]

* Al Viro <viro@ZenIV.linux.org.uk> wrote:

> On Sun, Feb 28, 2010 at 08:51:05AM +0100, Ingo Molnar wrote:
> 
> > ( Alas, ARM doesnt tend to be a big problem, at least as far as the facilities 
> >   i'm concerned about go: it has implemented most of the core kernel 
> >   infrastructures so there's few if any 'self inflicted' breakages that i can 
> >   remember. )
> 
> FWIW, it might make sense to run cross-builds for many targets and post the 
> things that crop up + analysis to linux-arch...  Any takers?
> 
> I haven't run a lot of cross-builds lately, but IME most of the breakage 
> tends to be less dramatic - somebody relying on indirect includes in driver 
> *or* forgetting to add "depends on" to Kconfig used to be the most frequent 
> case.
> 
> "let other targets rot" attitude has a very nasty effect - it snowballs. At 
> some point people *can't* check that their patches don't break things, even 
> if they want to.  And that, IMO, sucks.  At that point architecture needs to 
> be either removed or brought to the state when it builds in mainline.

What is happening right now is that our combined _costs_ snowball: generic 
changes are burdened with the overhead of a thousand cuts ...

IMO either there's enough interest in keeping an architecture going, rooted in 
_that_ architecture's importance (or the enthusiasm/clue of their developers), 
or, after a few years of inactivity it really shouldnt be upstream.

Right now we are socializing all the costs, sometimes even pretending that all 
architectures are equal. None of the costs really looks particularly large in 
isolation, but the sum of them does exist and adds up in certain places of the 
kernel.

Thanks,

	Ingo

Re: linux-next requirements

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 544 bytes --]

Hi Al,

On Sun, 28 Feb 2010 08:19:22 +0000 Al Viro <viro@ZenIV.linux.org.uk> wrote:
>
> FWIW, it might make sense to run cross-builds for many targets and post
> the things that crop up + analysis to linux-arch...  Any takers?

See http://kisskb.ellerman.id.au/kisskb/branch/9/ ... we just need
someone to read it regularly and post about them.  There is a set of
builds of Linus' tree there as well (look under "Branches").

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: linux-next requirements

[Ingo Molnar]

* Rafael J. Wysocki <rjw@sisk.pl> wrote:

> > In fact those rare ways of building and booting the kernel i mentioned are 
> > probably used _more_ than half of the architectures that linux-next 
> > build-tests ...
> 
> I don't know and you don't know either.  That's just pure speculation and 
> therefore meaningless.

We know various arch (and hardware) usage stats, such as:

  http://smolt.fedoraproject.org/static/stats/stats.html

Today's stats, done amongst users who are willing to opt in to the Smolt 
daemon:

       x86: 99.7%
   powerpc: 0.3%

x86 used to be 99.5 a year ago, so the world has become even more x86-centric.

There's also the kerneloops.org client, which shows in excess of 95% x86 usage 
as well. You can also grep the linux-kernel folder for arch signatures, etc.

And yes, there are millions of ARM (and MIPS) CPUs running Linux as well. 
(They are only as present as present their developers are: the users almost 
never show up on linux-kernel.)

Plus, a kernel subsystem maintainer like me who does lots of kernel 
infrastructure work can have a pretty good gut feeling about which 
architectures are actively helping out Linux, and which are just hanging on to 
the bandwagon.

So i respectfully disagree with your 'pure speculation' bit. Yes, it's 
somewhat of a guessing game, as so many things in life - but the trend is very 
clear.

	Ingo

Re: linux-next requirements

[Nick Bowler]

On 08:23 Sun 28 Feb     , Ingo Molnar wrote:
> 
> * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> 
> > > In fact those rare ways of building and booting the kernel i mentioned are 
> > > probably used _more_ than half of the architectures that linux-next 
> > > build-tests ...
> > 
> > I don't know and you don't know either.  That's just pure speculation and 
> > therefore meaningless.
> 
> We know various arch (and hardware) usage stats, such as:
> 
>   http://smolt.fedoraproject.org/static/stats/stats.html
> 
> Today's stats, done amongst users who are willing to opt in to the Smolt 
> daemon:
> 
>        x86: 99.7%
>    powerpc: 0.3%
> 
> x86 used to be 99.5 a year ago, so the world has become even more x86-centric.

This only tells us that _smolt users_ have become even more x86-centric.
As a self-selected sample, it is very likely a poor representative of
"the world" and any such extrapolation is indeed "pure speculation".

-- 
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)

Re: linux-next requirements

[Pavel Machek]

> Today's stats, done amongst users who are willing to opt in to the Smolt 
> daemon:
> 
>        x86: 99.7%
>    powerpc: 0.3%

Lies, bad lies, statistics.

> And yes, there are millions of ARM (and MIPS) CPUs running Linux as well. 
> (They are only as present as present their developers are: the users almost 
> never show up on linux-kernel.)

It cuts both ways. Maybe obscure architectures like alpha have more
than their marketshare of interested lkml users...

> Plus, a kernel subsystem maintainer like me who does lots of kernel 
> infrastructure work can have a pretty good gut feeling about which 
> architectures are actively helping out Linux, and which are just hanging on to 
> the bandwagon.

So instead of either fixing those architectures or marking them
deprecated, you blame Stephen for too much testing?

We are in middle of 'stable series', so regressions are not
permitted. You'd like to cause some regressions. Fine, but definitely
not Stephen's problem.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: linux-next requirements

[Thomas Gleixner]

Pavel,

On Wed, 3 Mar 2010, Pavel Machek wrote:
> > Today's stats, done amongst users who are willing to opt in to the Smolt 
> > daemon:
> > 
> >        x86: 99.7%
> >    powerpc: 0.3%
> 
> Lies, bad lies, statistics.

Reality. The vast majority of the feedback we get via LKML, most
subsytems mailinglists, bugzilla and kerneloops comes from x86
machines.

> > And yes, there are millions of ARM (and MIPS) CPUs running Linux as well. 
> > (They are only as present as present their developers are: the users almost 
> > never show up on linux-kernel.)
> 
> It cuts both ways. Maybe obscure architectures like alpha have more
> than their marketshare of interested lkml users...

And that's a valid excuse that such archs are ignoring deprecation
warnings for years ?

I can see that the few users of alpha - or whatever esoteric (sub)arch
you name - who are usually the interested maintainers / developers do
have not the energy to keep up with the changes, but that's simply a
damned maintainence nightmare for the rest of the kernel and
especially the core code in the long run.

Just a few examples from my own experience:
     
 - the generic mutex code was merged in 2.6.16 - 4 years ago
 - the generic timekeeping code was merged in 2.6.18 - 4 years ago
 - the generic interrupt framework was merged in 2.6.18 - 4 years ago
 - the generic clockevents code was merged in 2.6.21 - 3 years ago

and for all four we still drag around workarounds, compatibility
functions and other crap just to keep platforms and drivers alive.

Maintainers happily ignore that the core code has changed simply
because they rely on the "no regressions" rule forever. That _IS_
hilarious.

As a side note: We created checkpatch.pl, to have a tool which helps
us to alert developers about stuff which is deprecated and as a
byproduct the coding style rules. I think it's a useful tool in
general, just the outcome is an utter trainwreck:

We have hordes of whitespace, spelling and codingstyle cleanup
maniacs, while the hard stuff of replacing deprecated interfaces like
semaphore based mutexes / completions, cleaning up the BKL horror,
etc. is left to a few already overworked people who care.

What's even worse is it that developers of new code and the
maintainers who are merging it simply ignore its existance for
whatever reasons. I can accept the whitespace argument, but I have no
grasp why deprecation warnings are ignored at will.

A simple example:

  I'm trying to get rid of init_MUTEX for quite a while just to notice
  that every other kernel release we grow at least one new user of it.

  Latest new user merged in 2.6.33:

    drivers/block/drbd/drbd_bitmap.c:       init_MUTEX(&b->bm_change);

  That crap should not have been merged in the first place. Period. 

  Further the patch to fix that crap has been completely ignored by
  the developers of that very sh*t and the relevant subsystem
  maintainer as well.

Go and grep for the rest of that horror including new users of
lock_kernel() and unlock_kernel() - and I do _not_ mean the ones which
got pushed down by the few people who care.

That's what drives core code maintainers nuts. That's what Ingo is
pointing to:

Core kernel developers/maintainers need to respect every lazy
arch/driver developer/maintainer forever and care about bitrot
themself before getting anything useful done - while at the same time
the very same arch/driver developers impose crap on the kernel forever
and expect that this has to be accepted by the "no regression" rule ?

And _you_ are on the same boat expecting that core code developers
have to fix bitrotted sh*t or take care of removing it:

     "So instead of either fixing those architectures or marking them
      deprecated, you blame Stephen for too much testing?"

No, that's not how it works:

The few people doing core development work _DO NOT_ and _CANNOT_ scale
to fix up all the shit in the kernel. It needs the collaboration of
the whole kernel developer crowd and not finger pointing on those who
make the unmaintained sh*t hit the fan.

> > Plus, a kernel subsystem maintainer like me who does lots of kernel 
> > infrastructure work can have a pretty good gut feeling about which 
> > architectures are actively helping out Linux, and which are just hanging on to 
> > the bandwagon.
> 
> So instead of either fixing those architectures or marking them
> deprecated, you blame Stephen for too much testing?

Ingo did not blame Stephen at all. We all appreciate - and Ingo stated
so explicitly - the work Stephen is taking on to keep linux-next
running.

Ingo merily questioned the complaints about "regressions" from core
code changes resulting in wreckage of barely maintained trainwrecks
and I totally agree with him on that one.

Thanks,

	tglx

Re: linux-next requirements

[Andrew Morton]

On Thu, 4 Mar 2010 01:35:43 +0100 (CET) Thomas Gleixner <tglx@linutronix.de> wrote:

> As a side note: We created checkpatch.pl, to have a tool which helps
> us to alert developers about stuff which is deprecated and as a
> byproduct the coding style rules. I think it's a useful tool in
> general, just the outcome is an utter trainwreck:
> 
> We have hordes of whitespace, spelling and codingstyle cleanup
> maniacs, while the hard stuff of replacing deprecated interfaces like
> semaphore based mutexes / completions, cleaning up the BKL horror,
> etc. is left to a few already overworked people who care.
> 
> What's even worse is it that developers of new code and the
> maintainers who are merging it simply ignore its existance for
> whatever reasons. I can accept the whitespace argument, but I have no
> grasp why deprecation warnings are ignored at will.

um, write checkpatch rules to detect new additions of deprecated features.

I take patches.

Re: linux-next requirements

[Thomas Gleixner]

On Wed, 3 Mar 2010, Andrew Morton wrote:

> On Thu, 4 Mar 2010 01:35:43 +0100 (CET) Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > As a side note: We created checkpatch.pl, to have a tool which helps
> > us to alert developers about stuff which is deprecated and as a
> > byproduct the coding style rules. I think it's a useful tool in
> > general, just the outcome is an utter trainwreck:
> > 
> > We have hordes of whitespace, spelling and codingstyle cleanup
> > maniacs, while the hard stuff of replacing deprecated interfaces like
> > semaphore based mutexes / completions, cleaning up the BKL horror,
> > etc. is left to a few already overworked people who care.
> > 
> > What's even worse is it that developers of new code and the
> > maintainers who are merging it simply ignore its existance for
> > whatever reasons. I can accept the whitespace argument, but I have no
> > grasp why deprecation warnings are ignored at will.
> 
> um, write checkpatch rules to detect new additions of deprecated features.
> 
> I take patches.

Guess what ? There are rules already which warn about init_MUTEX,
init_MUTEX_locked for quite a while and that's why I'm ranting at both
developers and maintainers submitting resp. merging code containing
exactly that shit.

But yeah we do not have one for lock/unlock_kernel, will send one.

Thanks,

	tglx

Re: linux-next requirements

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> On Wed, 3 Mar 2010, Andrew Morton wrote:
> 
> > On Thu, 4 Mar 2010 01:35:43 +0100 (CET) Thomas Gleixner <tglx@linutronix.de> wrote:
> > 
> > > As a side note: We created checkpatch.pl, to have a tool which helps
> > > us to alert developers about stuff which is deprecated and as a
> > > byproduct the coding style rules. I think it's a useful tool in
> > > general, just the outcome is an utter trainwreck:
> > > 
> > > We have hordes of whitespace, spelling and codingstyle cleanup
> > > maniacs, while the hard stuff of replacing deprecated interfaces like
> > > semaphore based mutexes / completions, cleaning up the BKL horror,
> > > etc. is left to a few already overworked people who care.
> > > 
> > > What's even worse is it that developers of new code and the
> > > maintainers who are merging it simply ignore its existance for
> > > whatever reasons. I can accept the whitespace argument, but I have no
> > > grasp why deprecation warnings are ignored at will.
> > 
> > um, write checkpatch rules to detect new additions of deprecated features.
> > 
> > I take patches.
> 
> Guess what ? There are rules already which warn about init_MUTEX,
> init_MUTEX_locked for quite a while and that's why I'm ranting at both
> developers and maintainers submitting resp. merging code containing
> exactly that shit.
> 
> But yeah we do not have one for lock/unlock_kernel, will send one.

That's a good idea i think.

We should probably also rename ->ioctl to ->ioctl_legacy and have 
->unlocked_ioctl as the primary thing for new drivers to use.

	Ingo

Re: [tip:x86/ptrace] ptrace: Add support for generic PTRACE_GETREGSET/PTRACE_SETREGSET

[Roland McGrath]

> FYI, this commit broke tip:master on PARISC (other architectures are fine):
> 
>  kernel/built-in.o: In function `ptrace_request':
>  (.text.ptrace_request+0x2cc): undefined reference to `task_user_regset_view'

This means that parisc failed to meet the documented requirements for
setting CONFIG_HAVE_ARCH_TRACEHOOK, but set it anyway.  If arch folks don't
follow the specs, it defeats the whole purpose of having clear statements
of requirements for arch code.

> I'll keep them in tip:master to get them tested, but note that i cannot push 
> any of these patches into linux-next until this is fixed, as linux-next 
> requires all architectures to build, with no regard to which architectures are 
> tested by kernel testers in practice.

IMHO if parisc does not finish up its requirements RSN, it should instead:

diff --git a/arch/parisc/Kconfig b/arch/parisc/Kconfig
index 524d935..f388dc6 100644
--- a/arch/parisc/Kconfig
+++ b/arch/parisc/Kconfig
@@ -18,7 +18,6 @@ config PARISC
 	select BUG
 	select HAVE_PERF_EVENTS
 	select GENERIC_ATOMIC64 if !64BIT
-	select HAVE_ARCH_TRACEHOOK
 	help
 	  The PA-RISC microprocessor is designed by Hewlett-Packard and used
 	  in many of their workstations & servers (HP9000 700 and 800 series,


Thanks,
Roland

[tip:x86/ptrace] parisc: Disable CONFIG_HAVE_ARCH_TRACEHOOK

[tip-bot for Roland McGrath]

Commit-ID:  5e6dbc260704ce4d22fc9664f517f0bb6748feaa
Gitweb:     http://git.kernel.org/tip/5e6dbc260704ce4d22fc9664f517f0bb6748feaa
Author:     Roland McGrath <roland@redhat.com>
AuthorDate: Mon, 22 Feb 2010 10:37:07 -0800
Committer:  H. Peter Anvin <hpa@zytor.com>
CommitDate: Tue, 23 Feb 2010 10:34:41 -0800

parisc: Disable CONFIG_HAVE_ARCH_TRACEHOOK

> FYI, this commit broke tip:master on PARISC (other architectures are fine):
>
>  kernel/built-in.o: In function `ptrace_request':
>  (.text.ptrace_request+0x2cc): undefined reference to `task_user_regset_view'

This means that parisc failed to meet the documented requirements for
setting CONFIG_HAVE_ARCH_TRACEHOOK, but set it anyway.  If arch folks don't
follow the specs, it defeats the whole purpose of having clear statements
of requirements for arch code.

Until parisc finishes up its requirements, disable CONFIG_HAVE_ARCH_TRACEHOOK.

Signed-off-by: H. Peter Anvin <hpa@zytor.com>
LKML-Reference: <20100222183707.8749D64C@magilla.sf.frob.com>
Cc: <linux-parisc@vger.kernel.org>
Cc: Kyle McMartin <kyle@mcmartin.ca>
Cc: Helge Deller <deller@gmx.de>
Cc: James E.J. Bottomley <jejb@parisc-linux.org>
---
 arch/parisc/Kconfig |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/arch/parisc/Kconfig b/arch/parisc/Kconfig
index 524d935..f388dc6 100644
--- a/arch/parisc/Kconfig
+++ b/arch/parisc/Kconfig
@@ -18,7 +18,6 @@ config PARISC
 	select BUG
 	select HAVE_PERF_EVENTS
 	select GENERIC_ATOMIC64 if !64BIT
-	select HAVE_ARCH_TRACEHOOK
 	help
 	  The PA-RISC microprocessor is designed by Hewlett-Packard and used
 	  in many of their workstations & servers (HP9000 700 and 800 series,

Re: [patch v3 2/2] ptrace: Add support for generic PTRACE_GETREGSET/PTRACE_SETREGSET

[Roland McGrath]

Note that this patch and the xstate user_regset patch are entirely independent.
They can me merged in any order or one without the other.

> +/*
> + * Generic ptrace interface that exports the architecture specific regsets
> + * using the corresponding NT_* types (which are also used in the core dump).

There is a special case here, which I think already works as we intend it
to, but which should be clarified in the comment about this API.  The
NT_PRSTATUS note type in a core dump contains a full 'struct elf_prstatus'.
But the user_regset for NT_PRSTATUS contains just the elf_gregset_t that
is the pr_reg field of 'struct elf_prstatus'.

For all the other user_regset flavors, the user_regset layout and the ELF
core dump note payload are exactly the same layout, as your comment implies.

> +static int ptrace_regset(struct task_struct *task, int req, unsigned int type,
> +			 struct iovec *kiov)
> +{
> +	const struct user_regset_view *view = task_user_regset_view(task);
> +	const struct user_regset *regset = find_regset(view, type);
> +	int regset_no;
> +
> +	if (!regset || (kiov->iov_len % regset->size) != 0)
> +		return -EIO;

My inclination would be to diagnose these more specifically.  For a bad
size, give -EINVAL.  For an unknown regset type, give maybe -EINVAL or
maybe -ENODEV.  (-ENODEV is what you get for a known NT_* type that has a
user_regset implemented in the kernel, but that the particular hardware
we're running on doesn't support.  So perhaps you don't want to overload
that for a wholly unrecognized NT_* type.)

Otherwise, looks good to me.  ACK contingent on Oleg's ACK.


Thanks,
Roland

Re: [patch v3 2/2] ptrace: Add support for generic PTRACE_GETREGSET/PTRACE_SETREGSET

[Oleg Nesterov]

On 02/11, Roland McGrath wrote:
>
> > +static int ptrace_regset(struct task_struct *task, int req, unsigned int type,
> > +			 struct iovec *kiov)
> > +{
> > +	const struct user_regset_view *view = task_user_regset_view(task);
> > +	const struct user_regset *regset = find_regset(view, type);
> > +	int regset_no;
> > +
> > +	if (!regset || (kiov->iov_len % regset->size) != 0)
> > +		return -EIO;
>
> My inclination would be to diagnose these more specifically.

Agreed.

Otherwise I think the patch is fine.

Oleg.

[patch v2 4/4] ptrace: Add support for generic PTRACE_GETREGSET/PTRACE_SETREGSET

[Suresh Siddha]

[-- Attachment #1: ptrace_support_for_xstate.patch --]
[-- Type: text/plain, Size: 8012 bytes --]

Generic support for PTRACE_GETREGSET/PTRACE_SETREGSET commands which
export the regsets supported by each architecture using the correponding
NT_* types.

'addr' parameter for the ptrace system call encode the REGSET type and
the buffer length. 'data' parameter points to the user buffer.

	ptrace(PTRACE_GETREGSET/PTRACE_SETREGSET, pid,
	       (NT_TYPE << 20) | buf_length, buf);

For more information on how to use this API by users like debuggers and core
dump for accessing xstate registers, etc., please refer to comments in
arch/x86/include/asm/ptrace-abi.h

Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
Acked-by: Hongjiu Lu <hjl.tools@gmail.com>
---
 arch/x86/include/asm/ptrace-abi.h |   51 ++++++++++++++++++++++++++++++++++++++
 include/linux/elf.h               |   25 +++++++++++++++++-
 include/linux/ptrace.h            |   14 ++++++++++
 kernel/ptrace.c                   |   34 +++++++++++++++++++++++++
 4 files changed, 122 insertions(+), 2 deletions(-)

Index: tip/include/linux/ptrace.h
===================================================================
--- tip.orig/include/linux/ptrace.h
+++ tip/include/linux/ptrace.h
@@ -26,6 +26,18 @@
 #define PTRACE_GETEVENTMSG	0x4201
 #define PTRACE_GETSIGINFO	0x4202
 #define PTRACE_SETSIGINFO	0x4203
+#define PTRACE_GETREGSET	0x4204
+#define PTRACE_SETREGSET	0x4205
+
+/*
+ * First 20 bits of the addr in the PTRACE_GETREGSET/PTRACE_SETREGSET requests
+ * are reserved for communicating the user buffer size and the remaining 12 bits
+ * are reserved for the NT_* types (defined in include/linux/elf.h) which
+ * indicate the regset that the ptrace user is interested in.
+ */
+#define PTRACE_REGSET_BUF_SIZE(addr)	(addr & 0xfffff)
+#define PTRACE_REGSET_TYPE(addr)	((addr >> 20) & 0xfff)
+#define NOTE_TO_REGSET_TYPE(note)	(note & 0xfff)
 
 /* options set using PTRACE_SETOPTIONS */
 #define PTRACE_O_TRACESYSGOOD	0x00000001
@@ -114,6 +126,8 @@ static inline void ptrace_unlink(struct 
 
 int generic_ptrace_peekdata(struct task_struct *tsk, long addr, long data);
 int generic_ptrace_pokedata(struct task_struct *tsk, long addr, long data);
+int generic_ptrace_regset(struct task_struct *child, long request, long addr,
+			  long data);
 
 /**
  * task_ptrace - return %PT_* flags that apply to a task
Index: tip/kernel/ptrace.c
===================================================================
--- tip.orig/kernel/ptrace.c
+++ tip/kernel/ptrace.c
@@ -22,6 +22,7 @@
 #include <linux/pid_namespace.h>
 #include <linux/syscalls.h>
 #include <linux/uaccess.h>
+#include <linux/regset.h>
 
 
 /*
@@ -573,6 +574,11 @@ int ptrace_request(struct task_struct *c
 			return 0;
 		return ptrace_resume(child, request, SIGKILL);
 
+#ifdef PTRACE_EXPORT_REGSET
+	case PTRACE_GETREGSET:
+	case PTRACE_SETREGSET:
+		return generic_ptrace_regset(child, request, addr, data);
+#endif
 	default:
 		break;
 	}
@@ -664,6 +670,34 @@ int generic_ptrace_pokedata(struct task_
 	return (copied == sizeof(data)) ? 0 : -EIO;
 }
 
+#ifdef PTRACE_EXPORT_REGSET
+int generic_ptrace_regset(struct task_struct *child, long request, long addr,
+			  long data)
+{
+	const struct user_regset_view *view = task_user_regset_view(child);
+	unsigned int buf_size = PTRACE_REGSET_BUF_SIZE(addr);
+	int setno;
+
+	for (setno = 0; setno < view->n; setno++) {
+		const struct user_regset *regset = &view->regsets[setno];
+		if (NOTE_TO_REGSET_TYPE(regset->core_note_type) !=
+		    PTRACE_REGSET_TYPE(addr))
+			continue;
+
+		if (request == PTRACE_GETREGSET)
+			return copy_regset_to_user(child, view, setno, 0,
+						   buf_size,
+						   (void __user *) data);
+		else if (request == PTRACE_SETREGSET)
+			return copy_regset_from_user(child, view, setno, 0,
+						     buf_size,
+						     (void __user *) data);
+	}
+
+	return -EIO;
+}
+#endif
+
 #if defined CONFIG_COMPAT
 #include <linux/compat.h>
 
Index: tip/arch/x86/include/asm/ptrace-abi.h
===================================================================
--- tip.orig/arch/x86/include/asm/ptrace-abi.h
+++ tip/arch/x86/include/asm/ptrace-abi.h
@@ -139,4 +139,54 @@ struct ptrace_bts_config {
    Returns number of BTS records drained.
 */
 
+/*
+ * The extended register state using xsave/xrstor infrastructure can be
+ * be read and set by the user using the following ptrace command.
+ *
+ * 	ptrace(PTRACE_GETREGSET/PTRACE_SETREGSET, pid,
+ *	       (NT_X86_XSTATE << 20) | xstate_buf_length, xstate_buf);
+ *
+ * The structure layout used for the xstate_buf is same as
+ * the memory layout of xsave used by the processor (except for the bytes
+ * 464..511, which can be used by the software) and hence the size
+ * of this structure varies depending on the features supported by the processor
+ * and OS.  The size of the structure that users need to use can be obtained by
+ * doing:
+ *     cpuid_count(0xd, 0, &eax, &ptrace_xstateregs_struct_size, &ecx, &edx);
+ * i.e., cpuid.(eax=0xd,ecx=0).ebx will be the size that user (debuggers, etc.)
+ * need to use.
+ *
+ * The format of this structure will be like:
+ *	struct {
+ *		fxsave_bytes[0..463]
+ *		sw_usable_bytes[464..511]
+ *		xsave_hdr_bytes[512..575]
+ *		avx_bytes[576..831]
+ *		future_state etc
+ *	}
+ *
+ * The same memory layout will be used for the core-dump NT_X86_XSTATE
+ * note representing the xstate registers.
+ *
+ * For now, only the first 8 bytes of the sw_usable_bytes[464..471] will
+ * be used and will be set to OS enabled xstate mask (which is same as the
+ * 64bit mask returned by the xgetbv's xCR0).  Users (analyzing core dump
+ * remotely, etc.) can use this mask as well as the mask saved in the
+ * xstate_hdr bytes and interpret what states the processor/OS supports
+ * and what states are in modified/initialized conditions for the
+ * particular process/thread.
+ *
+ * Also when the user modifies certain state FP/SSE/etc through this
+ * PTRACE_SETXSTATEREGS, they must ensure that the xsave_hdr.xstate_bv
+ * bytes[512..519] of the above memory layout are updated correspondingly.
+ * i.e., for example when FP state is modified to a non-init state,
+ * xsave_hdr.xstate_bv's bit 0 must be set to '1', when SSE is modified to
+ * non-init state, xsave_hdr.xstate_bv's bit 1 must to be set to '1', etc.
+ */
+
+/*
+ * Enable PTRACE_GETREGSET/PTRACE_SETREGSET interface
+ */
+#define PTRACE_EXPORT_REGSET
+
 #endif /* _ASM_X86_PTRACE_ABI_H */
Index: tip/include/linux/elf.h
===================================================================
--- tip.orig/include/linux/elf.h
+++ tip/include/linux/elf.h
@@ -349,7 +349,29 @@ typedef struct elf64_shdr {
 #define ELF_OSABI ELFOSABI_NONE
 #endif
 
-/* Notes used in ET_CORE */
+/*
+ * Notes used in ET_CORE. Architectures export some of the arch register sets
+ * using the corresponding note types via ptrace
+ * PTRACE_GETREGSET/PTRACE_SETREGSET requests.
+ *
+ * For example,
+ * 	long addr = (NT_X86_XSTATE << 20 | buf_length);
+ * 	ptrace(PTRACE_GETREGSET, pid, addr, buf);
+ *
+ * will obtain the x86 extended register state of the pid.
+ *
+ * On 32bit architectures, addr argument is 32bits wide and encodes the length
+ * of the buffer in the first 20 bits, followed by NT_* type encoded in the
+ * remaining 12 bits, representing an unique regset.
+ *
+ * Hence on 32bit architectures, NT_PRXFPREG (0x46e62b7f) will be seen as 0xb7f
+ * and we can't allocate 0xb7f to any other note.
+ *
+ * All the other existing NT_* types fall with in the 12 bits and we have
+ * plenty of room for more NT_* types. For now, on all architectures
+ * we use first 20 bits of the addr for the buffer size and the next 12 bits for
+ * the NT_* type representing the corresponding regset.
+ */
 #define NT_PRSTATUS	1
 #define NT_PRFPREG	2
 #define NT_PRPSINFO	3
@@ -364,7 +386,6 @@ typedef struct elf64_shdr {
 #define NT_X86_XSTATE	0x202		/* x86 extended state using xsave */
 #define NT_S390_HIGH_GPRS	0x300	/* s390 upper register halves */
 
-
 /* Note header in a PT_NOTE section */
 typedef struct elf32_note {
   Elf32_Word	n_namesz;	/* Name size */

[tip:x86/ptrace] ptrace: Add support for generic PTRACE_GETREGSET/PTRACE_SETREGSET

[tip-bot for Suresh Siddha]

Commit-ID:  b26d3b32b860e4667df88dc2407f8823e2aa7cc6
Gitweb:     http://git.kernel.org/tip/b26d3b32b860e4667df88dc2407f8823e2aa7cc6
Author:     Suresh Siddha <suresh.b.siddha@intel.com>
AuthorDate: Tue, 9 Feb 2010 12:13:13 -0800
Committer:  H. Peter Anvin <hpa@zytor.com>
CommitDate: Tue, 9 Feb 2010 14:10:07 -0800

ptrace: Add support for generic PTRACE_GETREGSET/PTRACE_SETREGSET

Generic support for PTRACE_GETREGSET/PTRACE_SETREGSET commands which
export the regsets supported by each architecture using the correponding
NT_* types.

'addr' parameter for the ptrace system call encode the REGSET type and
the buffer length. 'data' parameter points to the user buffer.

	ptrace(PTRACE_GETREGSET/PTRACE_SETREGSET, pid,
	       (NT_TYPE << 20) | buf_length, buf);

For more information on how to use this API by users like debuggers and core
dump for accessing xstate registers, etc., please refer to comments in
arch/x86/include/asm/ptrace-abi.h

Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
LKML-Reference: <20100209202502.406177090@sbs-t61.sc.intel.com>
Acked-by: Hongjiu Lu <hjl.tools@gmail.com>
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
---
 arch/x86/include/asm/ptrace-abi.h |   50 +++++++++++++++++++++++++++++++++++++
 include/linux/elf.h               |   25 +++++++++++++++++-
 include/linux/ptrace.h            |   14 ++++++++++
 kernel/ptrace.c                   |   34 +++++++++++++++++++++++++
 4 files changed, 121 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/ptrace-abi.h b/arch/x86/include/asm/ptrace-abi.h
index 8672303..965a6ce 100644
--- a/arch/x86/include/asm/ptrace-abi.h
+++ b/arch/x86/include/asm/ptrace-abi.h
@@ -139,4 +139,54 @@ struct ptrace_bts_config {
    Returns number of BTS records drained.
 */
 
+/*
+ * The extended register state using xsave/xrstor infrastructure can be
+ * be read and set by the user using the following ptrace command.
+ *
+ *	ptrace(PTRACE_GETREGSET/PTRACE_SETREGSET, pid,
+ *	       (NT_X86_XSTATE << 20) | xstate_buf_length, xstate_buf);
+ *
+ * The structure layout used for the xstate_buf is same as
+ * the memory layout of xsave used by the processor (except for the bytes
+ * 464..511, which can be used by the software) and hence the size
+ * of this structure varies depending on the features supported by the processor
+ * and OS.  The size of the structure that users need to use can be obtained by
+ * doing:
+ *     cpuid_count(0xd, 0, &eax, &ptrace_xstateregs_struct_size, &ecx, &edx);
+ * i.e., cpuid.(eax=0xd,ecx=0).ebx will be the size that user (debuggers, etc.)
+ * need to use.
+ *
+ * The format of this structure will be like:
+ *	struct {
+ *		fxsave_bytes[0..463]
+ *		sw_usable_bytes[464..511]
+ *		xsave_hdr_bytes[512..575]
+ *		avx_bytes[576..831]
+ *		future_state etc
+ *	}
+ *
+ * The same memory layout will be used for the core-dump NT_X86_XSTATE
+ * note representing the xstate registers.
+ *
+ * For now, only the first 8 bytes of the sw_usable_bytes[464..471] will
+ * be used and will be set to OS enabled xstate mask (which is same as the
+ * 64bit mask returned by the xgetbv's xCR0).  Users (analyzing core dump
+ * remotely, etc.) can use this mask as well as the mask saved in the
+ * xstate_hdr bytes and interpret what states the processor/OS supports
+ * and what states are in modified/initialized conditions for the
+ * particular process/thread.
+ *
+ * Also when the user modifies certain state FP/SSE/etc through this
+ * PTRACE_SETXSTATEREGS, they must ensure that the xsave_hdr.xstate_bv
+ * bytes[512..519] of the above memory layout are updated correspondingly.
+ * i.e., for example when FP state is modified to a non-init state,
+ * xsave_hdr.xstate_bv's bit 0 must be set to '1', when SSE is modified to
+ * non-init state, xsave_hdr.xstate_bv's bit 1 must to be set to '1', etc.
+ */
+
+/*
+ * Enable PTRACE_GETREGSET/PTRACE_SETREGSET interface
+ */
+#define PTRACE_EXPORT_REGSET
+
 #endif /* _ASM_X86_PTRACE_ABI_H */
diff --git a/include/linux/elf.h b/include/linux/elf.h
index a8c4af0..5fd7996 100644
--- a/include/linux/elf.h
+++ b/include/linux/elf.h
@@ -349,7 +349,29 @@ typedef struct elf64_shdr {
 #define ELF_OSABI ELFOSABI_NONE
 #endif
 
-/* Notes used in ET_CORE */
+/*
+ * Notes used in ET_CORE. Architectures export some of the arch register sets
+ * using the corresponding note types via ptrace
+ * PTRACE_GETREGSET/PTRACE_SETREGSET requests.
+ *
+ * For example,
+ *	long addr = (NT_X86_XSTATE << 20 | buf_length);
+ *	ptrace(PTRACE_GETREGSET, pid, addr, buf);
+ *
+ * will obtain the x86 extended register state of the pid.
+ *
+ * On 32bit architectures, addr argument is 32bits wide and encodes the length
+ * of the buffer in the first 20 bits, followed by NT_* type encoded in the
+ * remaining 12 bits, representing an unique regset.
+ *
+ * Hence on 32bit architectures, NT_PRXFPREG (0x46e62b7f) will be seen as 0xb7f
+ * and we can't allocate 0xb7f to any other note.
+ *
+ * All the other existing NT_* types fall with in the 12 bits and we have
+ * plenty of room for more NT_* types. For now, on all architectures
+ * we use first 20 bits of the addr for the buffer size and the next 12 bits for
+ * the NT_* type representing the corresponding regset.
+ */
 #define NT_PRSTATUS	1
 #define NT_PRFPREG	2
 #define NT_PRPSINFO	3
@@ -364,7 +386,6 @@ typedef struct elf64_shdr {
 #define NT_X86_XSTATE	0x202		/* x86 extended state using xsave */
 #define NT_S390_HIGH_GPRS	0x300	/* s390 upper register halves */
 
-
 /* Note header in a PT_NOTE section */
 typedef struct elf32_note {
   Elf32_Word	n_namesz;	/* Name size */
diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index 56f2d63..4522e5e 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h
@@ -26,6 +26,18 @@
 #define PTRACE_GETEVENTMSG	0x4201
 #define PTRACE_GETSIGINFO	0x4202
 #define PTRACE_SETSIGINFO	0x4203
+#define PTRACE_GETREGSET	0x4204
+#define PTRACE_SETREGSET	0x4205
+
+/*
+ * First 20 bits of the addr in the PTRACE_GETREGSET/PTRACE_SETREGSET requests
+ * are reserved for communicating the user buffer size and the remaining 12 bits
+ * are reserved for the NT_* types (defined in include/linux/elf.h) which
+ * indicate the regset that the ptrace user is interested in.
+ */
+#define PTRACE_REGSET_BUF_SIZE(addr)	(addr & 0xfffff)
+#define PTRACE_REGSET_TYPE(addr)	((addr >> 20) & 0xfff)
+#define NOTE_TO_REGSET_TYPE(note)	(note & 0xfff)
 
 /* options set using PTRACE_SETOPTIONS */
 #define PTRACE_O_TRACESYSGOOD	0x00000001
@@ -114,6 +126,8 @@ static inline void ptrace_unlink(struct task_struct *child)
 
 int generic_ptrace_peekdata(struct task_struct *tsk, long addr, long data);
 int generic_ptrace_pokedata(struct task_struct *tsk, long addr, long data);
+int generic_ptrace_regset(struct task_struct *child, long request, long addr,
+			  long data);
 
 /**
  * task_ptrace - return %PT_* flags that apply to a task
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 23bd09c..7040208 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -22,6 +22,7 @@
 #include <linux/pid_namespace.h>
 #include <linux/syscalls.h>
 #include <linux/uaccess.h>
+#include <linux/regset.h>
 
 
 /*
@@ -573,6 +574,11 @@ int ptrace_request(struct task_struct *child, long request,
 			return 0;
 		return ptrace_resume(child, request, SIGKILL);
 
+#ifdef PTRACE_EXPORT_REGSET
+	case PTRACE_GETREGSET:
+	case PTRACE_SETREGSET:
+		return generic_ptrace_regset(child, request, addr, data);
+#endif
 	default:
 		break;
 	}
@@ -664,6 +670,34 @@ int generic_ptrace_pokedata(struct task_struct *tsk, long addr, long data)
 	return (copied == sizeof(data)) ? 0 : -EIO;
 }
 
+#ifdef PTRACE_EXPORT_REGSET
+int generic_ptrace_regset(struct task_struct *child, long request, long addr,
+			  long data)
+{
+	const struct user_regset_view *view = task_user_regset_view(child);
+	unsigned int buf_size = PTRACE_REGSET_BUF_SIZE(addr);
+	int setno;
+
+	for (setno = 0; setno < view->n; setno++) {
+		const struct user_regset *regset = &view->regsets[setno];
+		if (NOTE_TO_REGSET_TYPE(regset->core_note_type) !=
+		    PTRACE_REGSET_TYPE(addr))
+			continue;
+
+		if (request == PTRACE_GETREGSET)
+			return copy_regset_to_user(child, view, setno, 0,
+						   buf_size,
+						   (void __user *) data);
+		else if (request == PTRACE_SETREGSET)
+			return copy_regset_from_user(child, view, setno, 0,
+						     buf_size,
+						     (void __user *) data);
+	}
+
+	return -EIO;
+}
+#endif
+
 #if defined CONFIG_COMPAT
 #include <linux/compat.h>