On 11/19, Christophe Leroy wrote:

Le 19/11/2020 à 17:02, Oleg Nesterov a écrit :

quoted

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.

quoted

 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.

quoted

-	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.

quoted

-	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.

quoted

 	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.

quoted

--- 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.