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 <[email protected]>
Signed-off-by: Hongjiu Lu <[email protected]>
---
arch/x86/include/asm/i387.h | 12 ++++-
arch/x86/include/asm/xsave.h | 2
arch/x86/kernel/i387.c | 88 ++++++++++++++++++++++++++++++++++++++++++-
arch/x86/kernel/ptrace.c | 29 +++++++++++++-
arch/x86/kernel/xsave.c | 1
include/linux/elf.h | 1
6 files changed, 127 insertions(+), 6 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[6];
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 xfpregs_active() routine is 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;
@@ -204,8 +209,6 @@ int xfpregs_set(struct task_struct *targ
if (ret)
return ret;
- set_stopped_child_used_math(target);
-
ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
&target->thread.xstate->fxsave, 0, -1);
@@ -224,6 +227,87 @@ 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;
+ int size = regset->n * regset->size;
+ struct xsave_hdr_struct *xsave_hdr =
+ &target->thread.xstate->xsave.xsave_hdr;
+
+ 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 i387_fxsave_struct,
+ sw_reserved));
+ if (!ret)
+ /*
+ * Copy the 48bytes defined by software
+ */
+ ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
+ xstate_fx_sw_bytes,
+ offsetof(struct i387_fxsave_struct,
+ sw_reserved),
+ offsetof(struct xsave_struct,
+ xsave_hdr));
+ if (!ret)
+ /*
+ * Copy the rest of xstate memory layout
+ */
+ ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
+ xsave_hdr,
+ offsetof(struct xsave_struct,
+ xsave_hdr), size);
+ 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;
+ int size = regset->n * regset->size;
+ struct xsave_hdr_struct *xsave_hdr =
+ &target->thread.xstate->xsave.xsave_hdr;
+
+
+ if (!cpu_has_xsave)
+ return -ENODEV;
+
+ ret = init_fpu(target);
+ if (ret)
+ return ret;
+
+ set_stopped_child_used_math(target);
+
+ ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
+ &target->thread.xstate->xsave, 0, size);
+
+ /*
+ * mxcsr reserved bits must be masked to zero for security reasons.
+ */
+ target->thread.xstate->fxsave.mxcsr &= mxcsr_feature_mask;
+
+ 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(long), .align = sizeof(long),
+ .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(u32), .align = sizeof(u32),
+ .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,18 @@ static const struct user_regset_view use
};
#endif
+u64 xstate_fx_sw_bytes[6];
+void update_regset_xstate_info(unsigned int size, u64 xstate_mask)
+{
+#ifdef CONFIG_X86_64
+ x86_64_regsets[REGSET_XSTATE].n = size / sizeof(long);
+#endif
+#if defined CONFIG_X86_32 || defined CONFIG_IA32_EMULATION
+ x86_32_regsets[REGSET_XSTATE].n = size / sizeof(u32);
+#endif
+ xstate_fx_sw_bytes[0] = 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 */
Commit-ID: c19ff02dbfbb926913f78556a2be51ce5daddcd5
Gitweb: http://git.kernel.org/tip/c19ff02dbfbb926913f78556a2be51ce5daddcd5
Author: Suresh Siddha <[email protected]>
AuthorDate: Tue, 9 Feb 2010 12:13:11 -0800
Committer: H. Peter Anvin <[email protected]>
CommitDate: Tue, 9 Feb 2010 14:09:53 -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 <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Hongjiu Lu <[email protected]>
Signed-off-by: H. Peter Anvin <[email protected]>
---
arch/x86/include/asm/i387.h | 12 +++++-
arch/x86/include/asm/xsave.h | 2 +
arch/x86/kernel/i387.c | 88 +++++++++++++++++++++++++++++++++++++++++-
arch/x86/kernel/ptrace.c | 29 +++++++++++++-
arch/x86/kernel/xsave.c | 1 +
include/linux/elf.h | 1 +
6 files changed, 127 insertions(+), 6 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/xsave.h b/arch/x86/include/asm/xsave.h
index 727acc1..9e26171 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[6];
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..4719bf9 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 xfpregs_active() routine is 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;
@@ -204,8 +209,6 @@ int xfpregs_set(struct task_struct *target, const struct user_regset *regset,
if (ret)
return ret;
- set_stopped_child_used_math(target);
-
ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
&target->thread.xstate->fxsave, 0, -1);
@@ -224,6 +227,87 @@ 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;
+ int size = regset->n * regset->size;
+ struct xsave_hdr_struct *xsave_hdr =
+ &target->thread.xstate->xsave.xsave_hdr;
+
+ 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 i387_fxsave_struct,
+ sw_reserved));
+ if (!ret)
+ /*
+ * Copy the 48bytes defined by software
+ */
+ ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
+ xstate_fx_sw_bytes,
+ offsetof(struct i387_fxsave_struct,
+ sw_reserved),
+ offsetof(struct xsave_struct,
+ xsave_hdr));
+ if (!ret)
+ /*
+ * Copy the rest of xstate memory layout
+ */
+ ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
+ xsave_hdr,
+ offsetof(struct xsave_struct,
+ xsave_hdr), size);
+ 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;
+ int size = regset->n * regset->size;
+ struct xsave_hdr_struct *xsave_hdr =
+ &target->thread.xstate->xsave.xsave_hdr;
+
+
+ if (!cpu_has_xsave)
+ return -ENODEV;
+
+ ret = init_fpu(target);
+ if (ret)
+ return ret;
+
+ set_stopped_child_used_math(target);
+
+ ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
+ &target->thread.xstate->xsave, 0, size);
+
+ /*
+ * mxcsr reserved bits must be masked to zero for security reasons.
+ */
+ target->thread.xstate->fxsave.mxcsr &= mxcsr_feature_mask;
+
+ 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..5942da4 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(long), .align = sizeof(long),
+ .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(u32), .align = sizeof(u32),
+ .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,18 @@ static const struct user_regset_view user_x86_32_view = {
};
#endif
+u64 xstate_fx_sw_bytes[6];
+void update_regset_xstate_info(unsigned int size, u64 xstate_mask)
+{
+#ifdef CONFIG_X86_64
+ x86_64_regsets[REGSET_XSTATE].n = size / sizeof(long);
+#endif
+#if defined CONFIG_X86_32 || defined CONFIG_IA32_EMULATION
+ x86_32_regsets[REGSET_XSTATE].n = size / sizeof(u32);
+#endif
+ xstate_fx_sw_bytes[0] = 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 */
> 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.
Looks good modulo cosmetic nits below.
> 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).
I'd like to see some macros or struct or something in some user-visible
header (ptrace-abi.h I guess?) that instruct userland where to find this
software-defined word. The rest of the layout and meaning is specified by
the chip manuals and it's only a question of convenience if we want to help
userland out with those bits. But the use of the reserved-for-software
area to store a bitmask (or whatever else Linux might put there in the
future) is entirely a Linux invention that needs to be a clear and explicit
part of this userland ABI format.
> +/*
> + * The xfpregs_active() routine is same as the fpregs_active() routine,
s/xfpregs_active/xstateregs_active/
s/is same/is the same/
> @@ -204,8 +209,6 @@ int xfpregs_set(struct task_struct *targ
> if (ret)
> return ret;
>
> - set_stopped_child_used_math(target);
> -
You didn't mention this change in a comment or log entry.
It looks like it's superfluous after init_fpu. So this change
is right, but AFAICT it belongs in an entirely separate patch
and has nothing to do with xsave support.
> +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;
> + int size = regset->n * regset->size;
[...]
> + /*
> + * Copy the rest of xstate memory layout
> + */
> + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> + xsave_hdr,
> + offsetof(struct xsave_struct,
> + xsave_hdr), size);
This calculation is unnecessary (though it doesn't hurt); you can just use
-1 here. The arch user_regset.get and user_regset.set functions are not
required to worry about bogus arguments. Their callers are required to
pass values that don't violate the .n, .size, and .align constraints in the
user_regset.
> +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;
> + int size = regset->n * regset->size;
[...]
> + ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
> + &target->thread.xstate->xsave, 0, size);
Same here.
> + [REGSET_XSTATE] = {
> + .core_note_type = NT_X86_XSTATE,
> + .size = sizeof(long), .align = sizeof(long),
[...]
> + [REGSET_XSTATE] = {
> + .core_note_type = NT_X86_XSTATE,
> + .size = sizeof(u32), .align = sizeof(u32),
Isn't the xsave format is really the same for 32-bit and 64-bit?
All its components are naturally 64 bits or larger, right?
If that's correct, I think it would be better to make the 32 and 64 version
of the regset uniform with .size and .align being sizeof(u64).
> +u64 xstate_fx_sw_bytes[6];
> +void update_regset_xstate_info(unsigned int size, u64 xstate_mask)
> +{
> +#ifdef CONFIG_X86_64
> + x86_64_regsets[REGSET_XSTATE].n = size / sizeof(long);
> +#endif
> +#if defined CONFIG_X86_32 || defined CONFIG_IA32_EMULATION
> + x86_32_regsets[REGSET_XSTATE].n = size / sizeof(u32);
> +#endif
Just change these to / sizeof(u64) accordingly.
Thanks,
Roland
On 02/09, Roland McGrath wrote:
>
> > @@ -204,8 +209,6 @@ int xfpregs_set(struct task_struct *targ
> > if (ret)
> > return ret;
> >
> > - set_stopped_child_used_math(target);
> > -
>
> You didn't mention this change in a comment or log entry.
> It looks like it's superfluous after init_fpu. So this change
> is right, but AFAICT it belongs in an entirely separate patch
> and has nothing to do with xsave support.
I guess Suresh was going to change xstateregs_set() added by this patch,
it also sets USED_MATH after init_fpu().
Oleg.
On 02/09, Suresh Siddha wrote:
>
> +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;
> + int size = regset->n * regset->size;
> + struct xsave_hdr_struct *xsave_hdr =
> + &target->thread.xstate->xsave.xsave_hdr;
> +
> + 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 i387_fxsave_struct,
> + sw_reserved));
> + if (!ret)
> + /*
> + * Copy the 48bytes defined by software
> + */
> + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> + xstate_fx_sw_bytes,
> + offsetof(struct i387_fxsave_struct,
> + sw_reserved),
> + offsetof(struct xsave_struct,
> + xsave_hdr));
Hmm. Suresh, could you confirm these offsetof's are correct?
We are copying xstate_fx_sw_bytes array which is u64[6], but
start_pos == sizeof(i387_fxsave_struct) - padding ?
While we are here. Perhaps it would be more symmetrical to do
ret = user_regset_copyout();
if (ret)
return ret;
instead of "if (!ret)" which needs a tab, but this is up to you.
> + if (!ret)
> + /*
> + * Copy the rest of xstate memory layout
> + */
> + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> + xsave_hdr,
Another very minor nit, this is the only user of xsave_hdr, but this
var was defined at the top. Perhaps it would be a bit more clean to
have
struct xsave_struct *xsave = target->thread.xstate->xsave;
and use it in 1st and 3rd copyout?
Speaking about the first user_regset_copyout(), perhaps xsave->i387
would be more clear than xsave?
Oleg.
static inline int user_regset_copyout(unsigned int *pos, unsigned int *count,
void **kbuf,
void __user **ubuf, const void *data,
const int start_pos, const int end_pos)
{
if (*count == 0)
return 0;
BUG_ON(*pos < start_pos);
if (end_pos < 0 || *pos < end_pos) {
unsigned int copy = (end_pos < 0 ? *count
: min(*count, end_pos - *pos));
data += *pos - start_pos;
^^^
Is it correct? Shouldn't it be
data += *pos + start_pos;
?
Currently this doesn't matter, start_pos == 0 always. In fact I don't
understand why do we need this arg. In unlikely case the caller wants
to copy the part of *data, it can adjust data/end_pos itself.
Oleg.
On 02/10, Oleg Nesterov wrote:
>
> static inline int user_regset_copyout(unsigned int *pos, unsigned int *count,
> void **kbuf,
> void __user **ubuf, const void *data,
> const int start_pos, const int end_pos)
> {
> if (*count == 0)
> return 0;
> BUG_ON(*pos < start_pos);
> if (end_pos < 0 || *pos < end_pos) {
> unsigned int copy = (end_pos < 0 ? *count
> : min(*count, end_pos - *pos));
> data += *pos - start_pos;
> ^^^
> Is it correct? Shouldn't it be
>
> data += *pos + start_pos;
>
> ?
Ah, I seem to understand. start_pos is not the offset inside *data as
I thought. It is needed to compensate the "*pos += copy" addition which
was done by the previous user_regset_copyout().
This means that xstateregs_get() is right, it copies xstate_fx_sw_bytes
but uses sizeof(i387_fxsave_struct) as start_pos.
So tricky... I must admit, I don't understand the point.
Oleg.
On 02/10, Oleg Nesterov wrote:
>
> On 02/09, Suresh Siddha wrote:
> >
> > +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;
> > + int size = regset->n * regset->size;
> > + struct xsave_hdr_struct *xsave_hdr =
> > + &target->thread.xstate->xsave.xsave_hdr;
> > +
> > + 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 i387_fxsave_struct,
> > + sw_reserved));
> > + if (!ret)
> > + /*
> > + * Copy the 48bytes defined by software
> > + */
> > + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> > + xstate_fx_sw_bytes,
> > + offsetof(struct i387_fxsave_struct,
> > + sw_reserved),
> > + offsetof(struct xsave_struct,
> > + xsave_hdr));
>
> Hmm. Suresh, could you confirm these offsetof's are correct?
>
> We are copying xstate_fx_sw_bytes array which is u64[6], but
> start_pos == sizeof(i387_fxsave_struct) - padding ?
Sorry for noise. Now I see this should be correct, see another email I sent.
In fact, unless I missed something again, the 2nd and 3rd _copyout's could
use pos as start_pos instead of offsetof(what_we_already_copied).
Oleg.
> In fact, unless I missed something again, the 2nd and 3rd _copyout's could
> use pos as start_pos instead of offsetof(what_we_already_copied).
No, you should never do that. start_pos and end_pos should be constants.
Thanks,
Roland