Re: [PATCH v3 1/2] powerpc/ptrace: simplify gpr_get/tm_cgpr_get

[Oleg Nesterov]

On 11/19, Christophe Leroy wrote:
>
>
> Le 19/11/2020 à 17:02, Oleg Nesterov a écrit :
> >gpr_get() does membuf_write() twice to override pt_regs->msr in between.
>
> Is there anything wrong with that ?

Nothing wrong, but imo the code and 2/2 looks simpler after this patch.
I tried to explain this in the changelog.

> >  int tm_cgpr_get(struct task_struct *target, const struct user_regset *regset,
> >  		struct membuf to)
> >  {
> >+	struct membuf to_msr = membuf_at(&to, offsetof(struct pt_regs, msr));
> >+
> >  	if (!cpu_has_feature(CPU_FTR_TM))
> >  		return -ENODEV;
> >@@ -97,17 +99,12 @@ int tm_cgpr_get(struct task_struct *target, const struct user_regset *regset,
> >  	flush_altivec_to_thread(target);
> >  	membuf_write(&to, &target->thread.ckpt_regs,
> >-			offsetof(struct pt_regs, msr));
> >-	membuf_store(&to, get_user_ckpt_msr(target));
> >+				sizeof(struct user_pt_regs));
>
> This looks mis-aligned. But it should fit on a single line, now we allow up to 100 chars on a line.

OK, I can change this.

> >-	BUILD_BUG_ON(offsetof(struct pt_regs, orig_gpr3) !=
> >-		     offsetof(struct pt_regs, msr) + sizeof(long));
> >+	membuf_store(&to_msr, get_user_ckpt_msr(target));
> >-	membuf_write(&to, &target->thread.ckpt_regs.orig_gpr3,
> >-			sizeof(struct user_pt_regs) -
> >-			offsetof(struct pt_regs, orig_gpr3));
> >  	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
> >-			sizeof(struct user_pt_regs));
> >+				sizeof(struct user_pt_regs));
>
> I can't see any change here except the alignment. Can you leave it as is ?

I just tried to make tm_cgpr_get() and gpr_get() look similar.

Sure, I can leave it as is.

Better yet, could you please fix this problem somehow so that I could forget
about the bug assigned to me?

I know nothing about powerpc, and personally I do not care about this (minor)
bug, I agree with any changes.

> >-	membuf_write(&to, target->thread.regs, offsetof(struct pt_regs, msr));
> >-	membuf_store(&to, get_user_msr(target));
> >+	membuf_write(&to, target->thread.regs,
> >+				sizeof(struct user_pt_regs));
>
> This should fit on a single line.
>
> >  	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
> >-				 sizeof(struct user_pt_regs));
> >+				sizeof(struct user_pt_regs));
>
> This should not change, it's not part of the changes for this patch.

See above, I can leave it as is.

> >--- a/include/linux/regset.h
> >+++ b/include/linux/regset.h
> >@@ -46,6 +46,18 @@ static inline int membuf_write(struct membuf *s, const void *v, size_t size)
> >  	return s->left;
> >  }
> >+static inline struct membuf membuf_at(const struct membuf *s, size_t offs)
> >+{
> >+	struct membuf n = *s;
>
> Is there any point in using a struct membuf * instaed of a struct membuf as parameter ?

This matches other membuf_ helpers.

Oleg.