On Wed, 2011-03-02 at 14:08 +1100, Benjamin Herrenschmidt wrote:
> > --- ptracev2.orig/include/linux/regset.h 2011-02-20 12:15:57.000000000
> > -0600
> > +++ ptracev2/include/linux/regset.h 2011-02-28 13:33:36.685302349 -0600
> > @@ -240,6 +240,34 @@ static inline int user_regset_copyout(un
> > }
> > return 0;
> > }
> > +/* want the count to be the registers 14-31 inclusive */
> > +#define POISON_REG_CNT (PT_R31-PT_R14+1)
> > +static inline int user_regset_copyout_poison(unsigned int *pos,
> > + unsigned int *count,
> > + void **kbuf, void __user **ubuf,
> > + const int start_pos,
> > + const int end_pos)
> > +{
>
> I wouldn't put that in a generic location... especially something
> like POISON_REG_CNT is very specific to powerpc and our ABI.
Yeah I put it there thinking about readability but only use it in the
one place. If we go forward with this current strategy I will remove
the #define and just comment more.
>
> .../...
>
> > static inline int user_regset_copyin(unsigned int *pos, unsigned int
> > *count,
> > const void **kbuf,
> > --- ptracev2.orig/arch/powerpc/kernel/ptrace.c 2011-02-20
> > 12:15:57.000000000 -0600
> > +++ ptracev2/arch/powerpc/kernel/ptrace.c 2011-03-01 13:49:12.686354353
> > -0600
> > @@ -234,11 +234,29 @@ static int gpr_get(struct task_struct *t
> > if (target->thread.regs == NULL)
> > return -EIO;
> >
> > - CHECK_FULL_REGS(target->thread.regs);
>
> Wouldn't it be a simpler/cleaner approach to use a single function call
> here that checks if the reg set is full and if not, put poison values
> in the thread.regs structure itself ?
>
> The resulting code would be more palatable...
That was actually the way I had done the first patch while debugging the
problem but Paul Mackerras didn't like that I changed the thread struct.
It is not clear to me which is better. Reporting a poison value that
isn't actually there or to change the data. Hopefully a consensus can
be reached and then I can submit a new patch based on that.
>
> Cheers,
> Ben.
>
> > - ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> > - target->thread.regs,
> > - 0, offsetof(struct pt_regs, msr));
> > + if (!FULL_REGS(target->thread.regs)) {
> > + /* Don't have the full register set. Copy out register
> > r0-r13 */
> > + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> > + target->thread.regs,
> > + 0, sizeof(long)*PT_R14);
> > +
> > + /* Dont want to change the actual register values so rather
> > than read the */
> > + /* actual register copy a poison value into the buffer instead
> > */
> > + if (!ret)
> > + ret = user_regset_copyout_poison(&pos, &count, &kbuf,
> > &ubuf,
> > + sizeof(long)*PT_R14,
> > offsetof(struct pt_regs, nip));
> > +
> > + /* Copy out the rest of the registers as usual */
> > + if (!ret)
> > + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> > + target->thread.regs,
> > + offsetof(struct pt_regs,
> > nip), offsetof(struct pt_regs, msr));
> > +
> > + } else
> > + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> > + target->thread.regs,
> > + 0, offsetof(struct pt_regs, msr));
> > if (!ret) {
> > unsigned long msr = get_user_msr(target);
> > ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &msr,
> > @@ -645,11 +663,29 @@ static int gpr32_get(struct task_struct
> > if (target->thread.regs == NULL)
> > return -EIO;
> >
> > - CHECK_FULL_REGS(target->thread.regs);
> > -
> > pos /= sizeof(reg);
> > count /= sizeof(reg);
> >
> > + if(!FULL_REGS(target->thread.regs)) {
> > + if (kbuf) {
> > + /* Don't have the full register set. Copy out register
> > r0-r13 */
> > + for (; count > 0 && pos < PT_R14; --count)
> > + *k++ = regs[pos++];
> > +
> > + /* Dont want to change the actual register values so
> > rather than read the */
> > + /* actual register copy a poison value into the buffer
> > instead */
> > + for (; count > 0 && pos <= PT_R31; --count,pos++)
> > + *k++ = 0xdeadbeef;
> > +
> > + } else {
> > + for (; count > 0 && pos < PT_R14; --count)
> > + if (__put_user((compat_ulong_t) regs[pos++],
> > u++))
> > + return -EFAULT;
> > + for (; count > 0 && pos <= PT_R31; --count,pos++)
> > + if (__put_user((compat_ulong_t) 0xdeadbeef,
> > u++))
> > + return -EFAULT;
> > + }
> > + }
> > if (kbuf)
> > for (; count > 0 && pos < PT_MSR; --count)
> > *k++ = regs[pos++];
> >
> >
> > _______________________________________________
> > Linuxppc-dev mailing list
> > [email protected]
> > https://lists.ozlabs.org/listinfo/linuxppc-dev
>
>
_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://lists.ozlabs.org/listinfo/linuxppc-dev
