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.
