Re: [PATCH 1/2] LoongArch: Add pad structure members for explicit alignment

[Qing Zhang]

Hi, Xuerui and Ruoyao

On 2023/4/20 上午1:24, WANG Xuerui wrote:
> On 4/19/23 19:00, WANG Xuerui wrote:
> > On 2023/4/19 18:42, Xi Ruoyao wrote:
> > > On Tue, 2023-04-18 at 17:13 +0800, Qing Zhang wrote:
> > > > This is done in order to easily calculate the number of breakpoints
> > > > in hw_break_get.
> > > >
> > > > Signed-off-by: Qing Zhang <zhangqing@xxxxxxxxxxx>
> > > > ---
> > > >   arch/loongarch/include/uapi/asm/ptrace.h |  3 ++-
> > > >   arch/loongarch/kernel/ptrace.c           | 13 +++++++++----
> > > >   2 files changed, 11 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/arch/loongarch/include/uapi/asm/ptrace.h
> > > > b/arch/loongarch/include/uapi/asm/ptrace.h
> > > > index 2282ae1fd3b6..06e3be52cb04 100644
> > > > --- a/arch/loongarch/include/uapi/asm/ptrace.h
> > > > +++ b/arch/loongarch/include/uapi/asm/ptrace.h
> > > > @@ -57,11 +57,12 @@ struct user_lasx_state {
> >
> > Drive-by comment to the patch author: there is no "user_lasx_state" 
> > yet. Please always state your base commit if not obvious, or you 
> > should start from some well-known upstream HEAD (e.g. Linus' rc tags, 
> > loongarch-fixes, or loongarch-next).
> >
> > > >   };
> > > >     struct user_watch_state {
> > > > -       uint16_t dbg_info;
> > > > +       uint64_t dbg_info;
> > >
> > > Ouch.  This is a breaking change when we consider user code like
> > > `printf(PRIu16 "\n", ptr->dbg_info);`.  Is it really necessary?
> >
> > Ah right. This is UAPI so without *very* concrete and convicing reason 
> > why the change is not going to impact any potential users, it's gonna 
> > be a presumed NAK. In other words you must demonstrate (1) why it's 
> > absolutely necessary to make the change and (2) that it's impossible 
> > to impact anyone, before any such changes can even be considered.
> Please ignore all of this. The memory layout is actually the same after 
> the change due to the padding, I was somehow thinking in big-endian a 
> few hours ago. (The commit message didn't help either, I think both 
> Ruoyao and me got into the habitual thinking that changes like this are 
> most likely just churn without real benefits, after *not* seeing the 
> rationale in the commit message which was kinda expected.)
> >

This patch does not change the size of the structure. The structure
itself is implicitly aligned. We changed it to explicit alignment for
the convenience of hw_break_get/set (using membuf.left) to calculate the
offset and prevent breaks. Count overflow.

With pad explicit alignment, after membuf_write(&to, &info, 
sizeof(info)); to.left=200-8 bytes,
Thus,
membuf_store(&to, addr);
membuf_store(&to, mask);
membuf_store(&to, ctrl);
membuf_zero(&to, sizeof(u32));
After that, to.left is decremented by 24 bytes each time,
so the number of breakpoints will not overflow.

The user support code has not been submitted to the upstream, so
the current uapi change has no effect.

Thanks,
-Qing
> > > >          struct {
> > > >                  uint64_t    addr;
> > > >                  uint64_t    mask;
> > > >                  uint32_t    ctrl;
> > > > +               uint32_t    pad;
> > > >          } dbg_regs[8];
> > > >   };
> > > >   diff --git a/arch/loongarch/kernel/ptrace.c
> > > > b/arch/loongarch/kernel/ptrace.c
> > > > index 0c7c41e41cad..9c3bc1bbf2ff 100644
> > > > --- a/arch/loongarch/kernel/ptrace.c
> > > > +++ b/arch/loongarch/kernel/ptrace.c
> > > > @@ -475,10 +475,10 @@ static int ptrace_hbp_fill_attr_ctrl(unsigned
> > > > int note_type,
> > > >          return 0;
> > > >   }
> > > >   -static int ptrace_hbp_get_resource_info(unsigned int note_type, u16
> > > > *info)
> > > > +static int ptrace_hbp_get_resource_info(unsigned int note_type, u64
> > > > *info)
> > > >   {
> > > >          u8 num;
> > > > -       u16 reg = 0;
> > > > +       u64 reg = 0;
> > > >            switch (note_type) {
> > > >          case NT_LOONGARCH_HW_BREAK:
> > > > @@ -616,7 +616,7 @@ static int hw_break_get(struct task_struct
> > > > *target,
> > > >                          const struct user_regset *regset,
> > > >                          struct membuf to)
> > > >   {
> > > > -       u16 info;
> > > > +       u64 info;
> > > >          u32 ctrl;
> > > >          u64 addr, mask;
> > > >          int ret, idx = 0;
> > > > @@ -646,6 +646,7 @@ static int hw_break_get(struct task_struct
> > > > *target,
> > > >                  membuf_store(&to, addr);
> > > >                  membuf_store(&to, mask);
> > > >                  membuf_store(&to, ctrl);
> > > > +               membuf_zero(&to, sizeof(u32));
> > > >                  idx++;
> > > >          }
> > > >   @@ -662,7 +663,7 @@ static int hw_break_set(struct task_struct
> > > > *target,
> > > >          int ret, idx = 0, offset, limit;
> > > >          unsigned int note_type = regset->core_note_type;
> > > >   -       /* Resource info */
> > > > +       /* Resource info and pad */
> > > >          offset = offsetof(struct user_watch_state, dbg_regs);
> > > >          user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf, 0,
> > > > offset);
> > > >   @@ -704,6 +705,10 @@ static int hw_break_set(struct task_struct
> > > > *target,
> > > >                  if (ret)
> > > >                          return ret;
> > > >                  offset += PTRACE_HBP_CTRL_SZ;
> > > > +
> > > > +               user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf,
> > > > +                                         offset, offset +
> > > > PTRACE_HBP_PAD_SZ);
> > > > +               offset += PTRACE_HBP_PAD_SZ;
> > > >                  idx++;
> > > >          }