[4/8] libpdbg: Make getcr actually get all of the Condition Register

Message ID 20180622045116.12059-5-rashmica.g@gmail.com
State Superseded
Headers show
Series
  • Pre gdbserver additions
Related show

Commit Message

Rashmica Gupta June 22, 2018, 4:51 a.m.
Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>
---
 libpdbg/chip.c    | 25 ++++++++++++++++++-------
 libpdbg/libpdbg.h |  1 +
 2 files changed, 19 insertions(+), 7 deletions(-)

Comments

Alistair Popple June 25, 2018, 6:33 a.m. | #1
> +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, &regs->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, &regs->cr);
>  	printf("CR    : 0x%08" PRIx32 "\n", regs->cr);
>  
>  	ram_getxer(thread, &regs->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);
>
rashmica June 25, 2018, 7:39 a.m. | #2
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, &regs->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, &regs->cr);
>>  	printf("CR    : 0x%08" PRIx32 "\n", regs->cr);
>>  
>>  	ram_getxer(thread, &regs->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);
>>
>

Patch

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, &regs->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, &regs->cr);
 	printf("CR    : 0x%08" PRIx32 "\n", regs->cr);
 
 	ram_getxer(thread, &regs->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);