Re: [PATCH v7 02/28] powerpc/xmon: Move breakpoint instructions to own array

From: Jordan Niethe <jniethe5@gmail.com>
To: Alistair Popple <alistair@popple.id.au>
Cc: Nicholas Piggin <npiggin@gmail.com>,
	Balamuruhan S <bala24@linux.ibm.com>,
	naveen.n.rao@linux.vnet.ibm.com, linuxppc-dev@lists.ozlabs.org,
	Daniel Axtens <dja@axtens.net>
Subject: Re: [PATCH v7 02/28] powerpc/xmon: Move breakpoint instructions to own array
Date: Mon, 4 May 2020 15:52:55 +1000	[thread overview]
Message-ID: <CACzsE9o9zF670T0y7tUsWnuk75ycwFXW+MjA0d050sS3uvZ8-w@mail.gmail.com> (raw)
In-Reply-To: <2060873.nxaIYQu1l1@townsend>

On Mon, May 4, 2020 at 3:41 PM Alistair Popple <alistair@popple.id.au> wrote:
>
> On Friday, 1 May 2020 1:41:54 PM AEST Jordan Niethe wrote:
> > To execute an instruction out of line after a breakpoint, the NIP is set
> > to the address of struct bpt::instr. Here a copy of the instruction that
> > was replaced with a breakpoint is kept, along with a trap so normal flow
> > can be resumed after XOLing. The struct bpt's are located within the
> > data section. This is problematic as the data section may be marked as
> > no execute.
> >
> > Instead of each struct bpt holding the instructions to be XOL'd, make a
> > new array, bpt_table[], with enough space to hold instructions for the
> > number of supported breakpoints. A later patch will move this to the
> > text section.
> > Make struct bpt::instr a pointer to the instructions in bpt_table[]
> > associated with that breakpoint. This association is a simple mapping:
> > bpts[n] -> bpt_table[n * words per breakpoint]. Currently we only need
> > the copied instruction followed by a trap, so 2 words per breakpoint.
> >
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> > v4: New to series
> > v5: - Do not use __section(), use a .space directive in .S file
> >     - Simplify in_breakpoint_table() calculation
> >     - Define BPT_SIZE
> > v6: - Seperate moving to text section
> > ---
> >  arch/powerpc/xmon/xmon.c | 21 ++++++++++++---------
> >  1 file changed, 12 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> > index f91ae2c9adbe..6ba7f66c1dd0 100644
> > --- a/arch/powerpc/xmon/xmon.c
> > +++ b/arch/powerpc/xmon/xmon.c
> > @@ -98,7 +98,7 @@ static long *xmon_fault_jmp[NR_CPUS];
> >  /* Breakpoint stuff */
> >  struct bpt {
> >       unsigned long   address;
> > -     unsigned int    instr[2];
> > +     unsigned int    *instr;
> >       atomic_t        ref_count;
> >       int             enabled;
> >       unsigned long   pad;
> > @@ -117,6 +117,10 @@ static unsigned bpinstr = 0x7fe00008;    /* trap */
> >
> >  #define BP_NUM(bp)   ((bp) - bpts + 1)
> >
> > +#define BPT_SIZE       (sizeof(unsigned int) * 2)
> > +#define BPT_WORDS      (BPT_SIZE / sizeof(unsigned int))
>
> Minor nit-pick but IMHO this would be more logical if you defined BPT_WORDS
> first like so:
>
> #define BPT_WORDS      (2)
> #define BPT_SIZE       (sizeof(unsigned int) * BPT_WORDS)
>
> Otherwise this looks good and I think the offset calculations below are correct
> so:
What I was thinking was BPT_SIZE  would later be defined in terms the
instruction type and by doing it this way
 BPT_WORDS  would be correct for the the instruction type if like on
ppc32, it did not include a suffix.
>
> Reviewed-by: Alistair Popple <alistair@popple.id.au>
>
> > +static unsigned int bpt_table[NBPTS * BPT_WORDS];
> > +
> >  /* Prototypes */
> >  static int cmds(struct pt_regs *);
> >  static int mread(unsigned long, void *, int);
> > @@ -854,15 +858,13 @@ static struct bpt *in_breakpoint_table(unsigned long
> > nip, unsigned long *offp) {
> >       unsigned long off;
> >
> > -     off = nip - (unsigned long) bpts;
> > -     if (off >= sizeof(bpts))
> > +     off = nip - (unsigned long) bpt_table;
> > +     if (off >= sizeof(bpt_table))
> >               return NULL;
> > -     off %= sizeof(struct bpt);
> > -     if (off != offsetof(struct bpt, instr[0])
> > -         && off != offsetof(struct bpt, instr[1]))
> > +     *offp = off % BPT_SIZE;
> > +     if (*offp != 0 && *offp != 4)
> >               return NULL;
> > -     *offp = off - offsetof(struct bpt, instr[0]);
> > -     return (struct bpt *) (nip - off);
> > +     return bpts + (off / BPT_SIZE);
> >  }
> >
> >  static struct bpt *new_breakpoint(unsigned long a)
> > @@ -877,7 +879,8 @@ static struct bpt *new_breakpoint(unsigned long a)
> >       for (bp = bpts; bp < &bpts[NBPTS]; ++bp) {
> >               if (!bp->enabled && atomic_read(&bp->ref_count) == 0) {
> >                       bp->address = a;
> > -                     patch_instruction(&bp->instr[1], bpinstr);
> > +                     bp->instr = bpt_table + ((bp - bpts) * BPT_WORDS);
> > +                     patch_instruction(bp->instr + 1, bpinstr);
> >                       return bp;
> >               }
> >       }
>
>
>
>