Re: [PATCH v5 13/21] powerpc/xmon: Use a function for reading instructions

[Jordan Niethe <jniethe5@gmail.com>]

On Tue, Apr 7, 2020 at 9:31 PM Balamuruhan S <bala24@linux.ibm.com> wrote:
>
> On Mon, 2020-04-06 at 18:09 +1000, Jordan Niethe wrote:
> > Currently in xmon, mread() is used for reading instructions. In
> > preparation for prefixed instructions, create and use a new function,
> > mread_instr(), especially for reading instructions.
> >
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> > v5: New to series, seperated from "Add prefixed instructions to
> > instruction data type"
> > ---
> >  arch/powerpc/xmon/xmon.c | 24 ++++++++++++++++++++----
> >  1 file changed, 20 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> > index 5e3949322a6c..6f4cf01a58c1 100644
> > --- a/arch/powerpc/xmon/xmon.c
> > +++ b/arch/powerpc/xmon/xmon.c
> > @@ -125,6 +125,7 @@ extern unsigned int bpt_table[NBPTS * BPT_WORDS];
> >  static int cmds(struct pt_regs *);
> >  static int mread(unsigned long, void *, int);
> >  static int mwrite(unsigned long, void *, int);
> > +static int mread_instr(unsigned long, struct ppc_inst *);
> >  static int handle_fault(struct pt_regs *);
> >  static void byterev(unsigned char *, int);
> >  static void memex(void);
> > @@ -899,7 +900,7 @@ static void insert_bpts(void)
> >       for (i = 0; i < NBPTS; ++i, ++bp) {
> >               if ((bp->enabled & (BP_TRAP|BP_CIABR)) == 0)
> >                       continue;
> > -             if (mread(bp->address, &instr, 4) != 4) {
> > +             if (!mread_instr(bp->address, &instr)) {
>
>
> Are these checks made based on whether `ppc_inst_len()` returns bool from
> mread_instr() ?
No, it was meant to be the length itself returned with a length of 0
indicating an error. I will need to fix that.
>
> -- Bala
>
>
> >                       printf("Couldn't read instruction at %lx, "
> >                              "disabling breakpoint there\n", bp->address);
> >                       bp->enabled = 0;
> > @@ -949,7 +950,7 @@ static void remove_bpts(void)
> >       for (i = 0; i < NBPTS; ++i, ++bp) {
> >               if ((bp->enabled & (BP_TRAP|BP_CIABR)) != BP_TRAP)
> >                       continue;
> > -             if (mread(bp->address, &instr, 4) == 4
> > +             if (mread_instr(bp->address, &instr)
> >                   && ppc_inst_equal(instr, ppc_inst(bpinstr))
> >                   && patch_instruction(
> >                       (struct ppc_inst *)bp->address, ppc_inst_read(bp-
> > >instr)) != 0)
> > @@ -1165,7 +1166,7 @@ static int do_step(struct pt_regs *regs)
> >       force_enable_xmon();
> >       /* check we are in 64-bit kernel mode, translation enabled */
> >       if ((regs->msr & (MSR_64BIT|MSR_PR|MSR_IR)) == (MSR_64BIT|MSR_IR)) {
> > -             if (mread(regs->nip, &instr, 4) == 4) {
> > +             if (mread_instr(regs->nip, &instr)) {
> >                       stepped = emulate_step(regs, instr);
> >                       if (stepped < 0) {
> >                               printf("Couldn't single-step %s instruction\n",
> > @@ -1332,7 +1333,7 @@ static long check_bp_loc(unsigned long addr)
> >               printf("Breakpoints may only be placed at kernel addresses\n");
> >               return 0;
> >       }
> > -     if (!mread(addr, &instr, sizeof(instr))) {
> > +     if (!mread_instr(addr, &instr)) {
> >               printf("Can't read instruction at address %lx\n", addr);
> >               return 0;
> >       }
> > @@ -2125,6 +2126,21 @@ mwrite(unsigned long adrs, void *buf, int size)
> >       return n;
> >  }
> >
> > +static int
> > +mread_instr(unsigned long adrs, struct ppc_inst *instr)
> > +{
> > +     if (setjmp(bus_error_jmp) == 0) {
> > +             catch_memory_errors = 1;
> > +             sync();
> > +             *instr = ppc_inst_read((struct ppc_inst *)adrs);
> > +             sync();
> > +             /* wait a little while to see if we get a machine check */
> > +             __delay(200);
> > +     }
> > +     catch_memory_errors = 0;
> > +     return ppc_inst_len(*instr);
> > +}
> > +
> >  static int fault_type;
> >  static int fault_except;
> >  static char *fault_chars[] = { "--", "**", "##" };
>