| Message ID | 20180622045116.12059-5-rashmica.g@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Pre gdbserver additions | expand |
> +int ram_getcr(struct pdbg_target *thread, uint32_t *value) > +{ > + uint32_t cr_field, cr = 0; > + int i; > + > + for (i = 0; i < 8; i++){ > + cr_field = 0; > + ram_getcr_field(thread, i, &cr_field); > + /* We are not guarenteed that the other bits will be zeroed out */ Why not? ram_getcr_field() is an external API and I think it's suprising that it would return bits not part of the requested field so we should mask off bits there. Also I would expect the field to be right-justified, which would turn this: > + cr |= cr_field & (0xf << 4*i); Into this: cr |= cr_field << 4*i; Do you think that makes sense? Or will users expect the _field() APIs to return fields left-shifted? (The same comments apply to your getxer_fields() series too ;) - Alistair > + } > + > + *value = cr; > + return 0; > +} > + > int ram_putmsr(struct pdbg_target *thread, uint64_t value) > { > uint64_t opcodes[] = {mfspr(0, 277), mtmsr(0)}; > @@ -447,12 +463,7 @@ int ram_state_thread(struct pdbg_target *thread, struct thread_regs *regs) > ram_getspr(thread, 815, ®s->tar); > printf("TAR : 0x%016" PRIx64 "\n", regs->tar); > > - regs->cr = 0; > - for (i = 0; i < 8; i++) { > - uint64_t cr; > - ram_getcr(thread, i, &cr); > - regs->cr |= cr; > - } > + ram_getcr(thread, ®s->cr); > printf("CR : 0x%08" PRIx32 "\n", regs->cr); > > ram_getxer(thread, ®s->xer); > diff --git a/libpdbg/libpdbg.h b/libpdbg/libpdbg.h > index 81d6170..28470c3 100644 > --- a/libpdbg/libpdbg.h > +++ b/libpdbg/libpdbg.h > @@ -138,6 +138,7 @@ int ram_putnia(struct pdbg_target *target, uint64_t val); > int ram_putspr(struct pdbg_target *target, int spr, uint64_t val); > int ram_putgpr(struct pdbg_target *target, int spr, uint64_t val); > int ram_getmsr(struct pdbg_target *target, uint64_t *val); > +int ram_getcr(struct pdbg_target *thread, uint32_t *value); > int ram_getnia(struct pdbg_target *target, uint64_t *val); > int ram_getspr(struct pdbg_target *target, int spr, uint64_t *val); > int ram_getgpr(struct pdbg_target *target, int gpr, uint64_t *val); >
On 25/06/18 16:33, Alistair Popple wrote: >> +int ram_getcr(struct pdbg_target *thread, uint32_t *value) >> +{ >> + uint32_t cr_field, cr = 0; >> + int i; >> + >> + for (i = 0; i < 8; i++){ >> + cr_field = 0; >> + ram_getcr_field(thread, i, &cr_field); >> + /* We are not guarenteed that the other bits will be zeroed out */ > Why not? ram_getcr_field() is an external API and I think it's suprising that it > would return bits not part of the requested field so we should mask off bits > there. Also I would expect the field to be right-justified, which would turn > this: > >> + cr |= cr_field & (0xf << 4*i); > Into this: > > cr |= cr_field << 4*i; From a user POV that makes a lot more sense than the current behaviour :) > Do you think that makes sense? Or will users expect the _field() APIs to return > fields left-shifted? (The same comments apply to your getxer_fields() series too > ;) > > - Alistair > >> + } >> + >> + *value = cr; >> + return 0; >> +} >> + >> int ram_putmsr(struct pdbg_target *thread, uint64_t value) >> { >> uint64_t opcodes[] = {mfspr(0, 277), mtmsr(0)}; >> @@ -447,12 +463,7 @@ int ram_state_thread(struct pdbg_target *thread, struct thread_regs *regs) >> ram_getspr(thread, 815, ®s->tar); >> printf("TAR : 0x%016" PRIx64 "\n", regs->tar); >> >> - regs->cr = 0; >> - for (i = 0; i < 8; i++) { >> - uint64_t cr; >> - ram_getcr(thread, i, &cr); >> - regs->cr |= cr; >> - } >> + ram_getcr(thread, ®s->cr); >> printf("CR : 0x%08" PRIx32 "\n", regs->cr); >> >> ram_getxer(thread, ®s->xer); >> diff --git a/libpdbg/libpdbg.h b/libpdbg/libpdbg.h >> index 81d6170..28470c3 100644 >> --- a/libpdbg/libpdbg.h >> +++ b/libpdbg/libpdbg.h >> @@ -138,6 +138,7 @@ int ram_putnia(struct pdbg_target *target, uint64_t val); >> int ram_putspr(struct pdbg_target *target, int spr, uint64_t val); >> int ram_putgpr(struct pdbg_target *target, int spr, uint64_t val); >> int ram_getmsr(struct pdbg_target *target, uint64_t *val); >> +int ram_getcr(struct pdbg_target *thread, uint32_t *value); >> int ram_getnia(struct pdbg_target *target, uint64_t *val); >> int ram_getspr(struct pdbg_target *target, int spr, uint64_t *val); >> int ram_getgpr(struct pdbg_target *target, int gpr, uint64_t *val); >> >
diff --git a/libpdbg/chip.c b/libpdbg/chip.c index 6907765..a4ee1e4 100644 --- a/libpdbg/chip.c +++ b/libpdbg/chip.c @@ -321,7 +321,7 @@ int ram_getmsr(struct pdbg_target *thread, uint64_t *value) return 0; } -int ram_getcr(struct pdbg_target *thread, int cr, uint64_t *value) +int ram_getcr_field(struct pdbg_target *thread, int cr, uint32_t *value) { uint64_t opcodes[] = {mfocrf(0, cr), mtspr(277, 0)}; uint64_t results[] = {0, 0}; @@ -331,6 +331,22 @@ int ram_getcr(struct pdbg_target *thread, int cr, uint64_t *value) return 0; } +int ram_getcr(struct pdbg_target *thread, uint32_t *value) +{ + uint32_t cr_field, cr = 0; + int i; + + for (i = 0; i < 8; i++){ + cr_field = 0; + ram_getcr_field(thread, i, &cr_field); + /* We are not guarenteed that the other bits will be zeroed out */ + cr |= cr_field & (0xf << 4*i); + } + + *value = cr; + return 0; +} + int ram_putmsr(struct pdbg_target *thread, uint64_t value) { uint64_t opcodes[] = {mfspr(0, 277), mtmsr(0)}; @@ -447,12 +463,7 @@ int ram_state_thread(struct pdbg_target *thread, struct thread_regs *regs) ram_getspr(thread, 815, ®s->tar); printf("TAR : 0x%016" PRIx64 "\n", regs->tar); - regs->cr = 0; - for (i = 0; i < 8; i++) { - uint64_t cr; - ram_getcr(thread, i, &cr); - regs->cr |= cr; - } + ram_getcr(thread, ®s->cr); printf("CR : 0x%08" PRIx32 "\n", regs->cr); ram_getxer(thread, ®s->xer); diff --git a/libpdbg/libpdbg.h b/libpdbg/libpdbg.h index 81d6170..28470c3 100644 --- a/libpdbg/libpdbg.h +++ b/libpdbg/libpdbg.h @@ -138,6 +138,7 @@ int ram_putnia(struct pdbg_target *target, uint64_t val); int ram_putspr(struct pdbg_target *target, int spr, uint64_t val); int ram_putgpr(struct pdbg_target *target, int spr, uint64_t val); int ram_getmsr(struct pdbg_target *target, uint64_t *val); +int ram_getcr(struct pdbg_target *thread, uint32_t *value); int ram_getnia(struct pdbg_target *target, uint64_t *val); int ram_getspr(struct pdbg_target *target, int spr, uint64_t *val); int ram_getgpr(struct pdbg_target *target, int gpr, uint64_t *val);
Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com> --- libpdbg/chip.c | 25 ++++++++++++++++++------- libpdbg/libpdbg.h | 1 + 2 files changed, 19 insertions(+), 7 deletions(-)