Skip to contentSkip to conversation

score: add regset support

PATCH In review liqin.chen 3 participants 6 messages Jul 10, 2009 – Jul 15, 2009

Subsystems (3)
lore
Share & tools
lore

liqin.chen

author
#0 lore raw

PATCH v2 1/1 score: add regset support

ptrace.c add register sets support for score architecture.

- genregs_get(struct task_struct *target,
const struct user_regset *regset,
unsigned int pos, unsigned int count,
void *kbuf, void __user *ubuf)
Retrieve the contents of score userspace general registers.

- genregs_set(struct task_struct *target,
const struct user_regset *regset,
unsigned int pos, unsigned int count,
const void *kbuf, const void __user *ubuf)
Update the contents of the score userspace general registers.

---

arch/score/include/asm/elf.h    |   18 +++--
arch/score/include/asm/ptrace.h |    2 +
arch/score/kernel/ptrace.c      |  140 ++++++++++++++++++++++++++++++--------
3 files changed, 123 insertions(+), 37 deletions(-)
arch/score/include/asm/elf.h +0 -0
index 8324363..43526d9 100644
@@ -1,9 +1,8 @@

#ifndef _ASM_SCORE_ELF_H
#define _ASM_SCORE_ELF_H

-/* ELF register definitions */
-#define ELF_NGREG 45
-#define ELF_NFPREG 33
+#include <linux/ptrace.h>
+
#define EM_SCORE7 135

/* Relocation types. */
@@ -30,11 +29,15 @@
#define R_SCORE_IMM30 20
#define R_SCORE_IMM32 21

-typedef unsigned long elf_greg_t;
-typedef elf_greg_t elf_gregset_t[ELF_NGREG];
+/* ELF register definitions */
+typedef unsigned long elf_greg_t;
+
+#define ELF_NGREG (sizeof(struct pt_regs) / sizeof(elf_greg_t))
+typedef elf_greg_t elf_gregset_t[ELF_NGREG];

-typedef double elf_fpreg_t;
-typedef elf_fpreg_t elf_fpregset_t[ELF_NFPREG];
+/* Score does not have fp regs. */
+typedef double elf_fpreg_t;
+typedef elf_fpreg_t elf_fpregset_t;

#define elf_check_arch(x) ((x)->e_machine == EM_SCORE7)

@@ -57,6 +60,7 @@ do { \
struct task_struct;
struct pt_regs;

+#define CORE_DUMP_USE_REGSET
#define USE_ELF_CORE_DUMP
#define ELF_EXEC_PAGESIZE PAGE_SIZE

arch/score/include/asm/ptrace.h +0 -0
index 66b14c8..19ce850 100644
@@ -74,6 +74,8 @@ struct pt_regs {
 

#ifdef __KERNEL__

+struct task_struct;
+
/*
* Does the process account for user or for system time? */

arch/score/kernel/ptrace.c +1 -0
index 1db876b..43784d8 100644
@@ -23,11 +23,113 @@
 * 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
 */
 
+#include <linux/elf.h>

#include <linux/kernel.h>
#include <linux/ptrace.h>
+#include <linux/regset.h>

#include <asm/uaccess.h>

+/*
+ * retrieve the contents of SCORE userspace general registers
+ */
+static int genregs_get(struct task_struct *target,

+               const struct user_regset *regset,
+               unsigned int pos, unsigned int count,
+               void *kbuf, void __user *ubuf)

+{
+ const struct pt_regs *regs = task_pt_regs(target);
+ int ret;
+
+ /* skip 8 * sizeof(unsigned long) not use for pt_regs */
+ ret = user_regset_copyout_zero(&pos, &count, &kbuf, &ubuf,
+ 0, offsetof(struct pt_regs, regs));
+
+ /* r0 - r31 */
+ ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,

+                  regs->regs,
+                  offsetof(struct pt_regs, regs),
+                  offsetof(struct pt_regs, cel));

+
+ if (!ret)
+ /* cel, ceh, sr0, sr1, sr2, epc, ema, psr, ecr, condition */
+ ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,

+                    &regs->cel,
+                    offsetof(struct pt_regs, cel),
+                    offsetof(struct pt_regs, is_syscall));

+
+ if (!ret)
+ ret = user_regset_copyout_zero(&pos, &count, &kbuf, &ubuf,
+ sizeof(struct pt_regs), -1);
+
+ return ret;
+}
+
+/*
+ * update the contents of the SCORE userspace general registers
+ */
+static int genregs_set(struct task_struct *target,

+               const struct user_regset *regset,
+               unsigned int pos, unsigned int count,
+               const void *kbuf, const void __user *ubuf)

+{
+ struct pt_regs *regs = task_pt_regs(target);
+ int ret;
+
+ /* skip 8 * sizeof(unsigned long) */
+ ret = user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf,
+ 0, offsetof(struct pt_regs, regs));
+
+ /* r0 - r31 */
+ ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,

+                  regs->regs,
+                  offsetof(struct pt_regs, regs),
+                  offsetof(struct pt_regs, cel));

+
+ if (!ret)
+ /* cel, ceh, sr0, sr1, sr2, epc, ema, psr, ecr, condition */
+ ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,

+                    &regs->cel,
+                    offsetof(struct pt_regs, cel),
+                    offsetof(struct pt_regs, is_syscall));

+
+ if (!ret)
+ ret = user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf,
+ sizeof(struct pt_regs), -1);
+
+ return ret;
+}
+
+/*
+ * Define the register sets available on the score7 under Linux
+ */
+enum score7_regset {
+ REGSET_GENERAL,
+};
+
+static const struct user_regset score7_regsets[] = {
+ [REGSET_GENERAL] = {

+        .core_note_type    = NT_PRSTATUS,
+        .n        = ELF_NGREG,
+        .size        = sizeof(long),
+        .align        = sizeof(long),
+        .get        = genregs_get,
+        .set        = genregs_set,

+ },
+};
+
+static const struct user_regset_view user_score_native_view = {

+    .name        = "score7",
+    .e_machine    = EM_SCORE7,
+    .regsets    = score7_regsets,
+    .n        = ARRAY_SIZE(score7_regsets),

+};
+
+const struct user_regset_view *task_user_regset_view(struct task_struct *task) +{
+ return &user_score_native_view;
+}
+
static int is_16bitinsn(unsigned long insn)

{
    if ((insn & INSN32_MASK) == INSN32_MASK)
@@ -80,34 +182,6 @@ write_tsk_long(struct task_struct *child,
    return copied != sizeof(val) ? -EIO : 0;
}

-/*
- * Get all user integer registers.
- */
-static int ptrace_getregs(struct task_struct *tsk, void __user *uregs) -{
- struct pt_regs *regs = task_pt_regs(tsk);
-
- return copy_to_user(uregs, regs, sizeof(struct pt_regs)) ? -EFAULT : 0;
-}
-
-/*
- * Set all user integer registers.
- */
-static int ptrace_setregs(struct task_struct *tsk, void __user *uregs) -{
- struct pt_regs newregs;
- int ret;
-
- ret = -EFAULT;
- if (copy_from_user(&newregs, uregs, sizeof(struct pt_regs)) == 0) {

-        struct pt_regs *regs = task_pt_regs(tsk);
-        *regs = newregs;
-        ret = 0;

- }
-
- return ret;
-}
-
void user_enable_single_step(struct task_struct *child)

{
    /* far_epc is the target of branch */
@@ -356,11 +430,17 @@ arch_ptrace(struct task_struct *child, long request, long addr, long data)
    }

    case PTRACE_GETREGS:
-        ret = ptrace_getregs(child, (void __user *)datap);
+        return copy_regset_to_user(child, &user_score_native_view,
+                        REGSET_GENERAL,
+                        0, sizeof(struct pt_regs),
+                        (void __user *)datap);
        break;

    case PTRACE_SETREGS:
-        ret = ptrace_setregs(child, (void __user *)datap);
+        return copy_regset_from_user(child, &user_score_native_view,
+                        REGSET_GENERAL,
+                        0, sizeof(struct pt_regs),
+                        (const void __user *)datap);
        break;

    default:
Signature · 1 line
1.6.2
maintainer
#1 lore raw
Quoted 14 lines · liqin.chen
(quoted diff) +14 -0
+static int genregs_get(struct task_struct *target,
+               const struct user_regset *regset,
+               unsigned int pos, unsigned int count,
+               void *kbuf, void __user *ubuf)
+{
+    const struct pt_regs *regs = task_pt_regs(target);
+    int ret;
+
+    /* skip 8 * sizeof(unsigned long) not use for pt_regs */
+    ret = user_regset_copyout_zero(&pos, &count, &kbuf, &ubuf,
+                    0, offsetof(struct pt_regs, regs));
+
+    /* r0 - r31 */
+    ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,

Review

Quoted 10 lines
(quoted diff) +10 -0
+                  regs->regs,
+                  offsetof(struct pt_regs, regs),
+                  offsetof(struct pt_regs, cel));
+
+    if (!ret)
+        /* cel, ceh, sr0, sr1, sr2, epc, ema, psr, ecr, condition */
+        ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
+                    &regs->cel,
+                    offsetof(struct pt_regs, cel),
+                    offsetof(struct pt_regs, is_syscall));

The two user_regset_copyout are consecutive, so AFAICT they can be combined into a single function call.

Review

Quoted 7 lines
(quoted diff) +7 -0
+
+    if (!ret)
+        ret = user_regset_copyout_zero(&pos, &count, &kbuf, &ubuf,
+                        sizeof(struct pt_regs), -1);
+
+    return ret;
+}

While the code looks correct to me now based on Rolands comments, I think it would be nicer to define separate pt_regs and user_regset data structures to make the two independent and give you more flexibility with the kernel stack layout in the future.

Maybe you could change

struct pt_regs {
       unsigned long pad0[6];
       unsigned long orig_r4;
       unsigned long orig_r7;
       unsigned long regs[32];

       unsigned long cel;
       unsigned long ceh;

       unsigned long sr0;      /* cnt */
       unsigned long sr1;      /* lcr */
       unsigned long sr2;      /* scr */

       unsigned long cp0_epc;
       unsigned long cp0_ema;
       unsigned long cp0_psr;
       unsigned long cp0_ecr;
       unsigned long cp0_condition;

       long is_syscall;
};

to

struct pt_regs {
       unsigned long regs[32];

       unsigned long cel;
       unsigned long ceh;

       unsigned long sr0;      /* cnt */
       unsigned long sr1;      /* lcr */
       unsigned long sr2;      /* scr */

       unsigned long cp0_epc;
       unsigned long cp0_ema;
       unsigned long cp0_psr;
       unsigned long cp0_ecr;
       unsigned long cp0_condition;
#ifdef __KERNEL__
       unsigned long orig_r4;
       unsigned long orig_r7;
       long is_syscall;
       unsigned long pad0[3];
#else
       unsigned long pad0[6];

#endif
};

Quoted 20 lines
(quoted diff) +8 -2
@@ -356,11 +430,17 @@ arch_ptrace(struct task_struct *child, long 
 request, long addr, long data)
     }
 
     case PTRACE_GETREGS:
-        ret = ptrace_getregs(child, (void __user *)datap);
+        return copy_regset_to_user(child, &user_score_native_view,
+                        REGSET_GENERAL,
+                        0, sizeof(struct pt_regs),
+                        (void __user *)datap);                        	
         break;
 
     case PTRACE_SETREGS:
-        ret = ptrace_setregs(child, (void __user *)datap);
+        return copy_regset_from_user(child, &user_score_native_view,
+                        REGSET_GENERAL,
+                        0, sizeof(struct pt_regs),
+                        (const void __user *)datap);
         break;
 
Quoted 1 line

default:

I guess you still need to remove the PTRACE_PEEKUSR and PTRACE_POKEUSR code, as mentioned by Roland.

Roland, Christoph: Do you think it would be reasonable to implement this in common code? That would make it possible to have an empty arch_ptrace() function.

kernel/ptrace.c +15 -0
index 61c78b2..a6b7862 100644
@@ -570,6 +570,21 @@ int ptrace_request(struct task_struct *child, long request,
 			return 0;
 		return ptrace_resume(child, request, SIGKILL);
 
+#if defined(PTRACE_GENERIC_GETREGS) && defined(REGSET_GENERAL)
+	case PTRACE_GETREGS:
+		return copy_regset_to_user(child,
+				task_user_regset_view(child),
+				REGSET_GENERAL, 0,
+				sizeof(struct pt_regs),
+				(void __user *)datap);
+	case PTRACE_GETREGS:
+		return copy_regset_from_user(child,
+				task_user_regset_view(child),
+				REGSET_GENERAL, 0,
+				sizeof(struct pt_regs),
+				(void __user *)datap);
+#endif
+
 	default:
 		break;
 	}
#2 lore raw

[email protected] 写于 2009-07-10 20:49:44:

Quoted 46 lines
struct pt_regs {
       unsigned long pad0[6];
       unsigned long orig_r4;
       unsigned long orig_r7;
       unsigned long regs[32];

       unsigned long cel;
       unsigned long ceh;

       unsigned long sr0;      /* cnt */
       unsigned long sr1;      /* lcr */
       unsigned long sr2;      /* scr */

       unsigned long cp0_epc;
       unsigned long cp0_ema;
       unsigned long cp0_psr;
       unsigned long cp0_ecr;
       unsigned long cp0_condition;

       long is_syscall;
};

to

struct pt_regs {
       unsigned long regs[32];

       unsigned long cel;
       unsigned long ceh;

       unsigned long sr0;      /* cnt */
       unsigned long sr1;      /* lcr */
       unsigned long sr2;      /* scr */

       unsigned long cp0_epc;
       unsigned long cp0_ema;
       unsigned long cp0_psr;
       unsigned long cp0_ecr;
       unsigned long cp0_condition;
#ifdef __KERNEL__
       unsigned long orig_r4;
       unsigned long orig_r7;
       long is_syscall;
       unsigned long pad0[3];
#else
       unsigned long pad0[6];
Quoted 3 lines

#endif
};

unsigned long pad0[6];

was place at the start of pt_regs use
to hold the arguments from userland.
caller(exception handler) not need to
change stack pointer.

unsigned long orig_r4;
unsigned long orig_r7;
move to end of the pt_regs haven't
problem.

Thanks,
Liqin

maintainer
#3 lore raw
Quoted 8 lines · On Monday 13 July 2009, [email protected] wrote:

unsigned long pad0[6];

was place at the start of pt_regs use
to hold the arguments from userland.
caller(exception handler) not need to
change stack pointer.

Quoted 4 lines

unsigned long orig_r4;
unsigned long orig_r7;
move to end of the pt_regs haven't
problem.

Ok, I see.

Maybe to get a nicer layout, you can define separate structures then:

struct user_regs_struct {
        unsigned long regs[32];

        unsigned long cel;
        unsigned long ceh;
 
        unsigned long sr0;      /* cnt */
        unsigned long sr1;      /* lcr */
        unsigned long sr2;      /* scr */
 
        unsigned long cp0_epc;
        unsigned long cp0_ema;
        unsigned long cp0_psr;
        unsigned long cp0_ecr;
        unsigned long cp0_condition;

	unsigned long reserved[6]; /* future extensions */
};

#ifdef __KERNEL__

struct pt_regs {
	/* stack arguments */
        unsigned long pad0[6];

	/* internal use */
        unsigned long orig_r4;
        unsigned long orig_r7;
        long is_syscall;

	/* saved registers */
	struct user_regs_struct uregs;
};

#endif

That would keep the user struct relatively clean.

	Arnd <><
#4 lore raw

Arnd Bergmann <[email protected]> 写于 2009-07-13 22:24:28:

Quoted 35 lines

Maybe to get a nicer layout, you can define separate structures then:

struct user_regs_struct {
        unsigned long regs[32];

        unsigned long cel;
        unsigned long ceh;

        unsigned long sr0;      /* cnt */
        unsigned long sr1;      /* lcr */
        unsigned long sr2;      /* scr */

        unsigned long cp0_epc;
        unsigned long cp0_ema;
        unsigned long cp0_psr;
        unsigned long cp0_ecr;
        unsigned long cp0_condition;

unsigned long reserved[6]; /* future extensions */
};

#ifdef __KERNEL__

struct pt_regs {
   /* stack arguments */
        unsigned long pad0[6];

   /* internal use */
        unsigned long orig_r4;
        unsigned long orig_r7;
        long is_syscall;

   /* saved registers */
   struct user_regs_struct uregs;
};
Quoted 3 lines

#endif

That would keep the user struct relatively clean.

OK, I will send this patch soon.

Thanks,
liqin

#5 lore raw
In reply to Arnd Bergmann

IMHO it is certainly a wise choice to have some struct (be it called user_regs_struct or whatever) that describes the exact userland layout. (I think it's also fine to just call that elf_gregset_t and have it be an array, with REG_* defines/enum somewhere to document its indices. It's just a matter of taste for the arch folks.)

Given such a struct, the regset code can refer to it (with offsetof, e.g.) and that will make it far easier to read, understand, and review.

Thanks,
Roland