[03/10] pdbg: Add getxer & putxer commands

Message ID 20180531052915.31171-3-rashmica.g@gmail.com
State Changes Requested
Headers show
Series
  • [01/10] libpdbg: Print the name of the instruction when erroring
Related show

Commit Message

Rashmica Gupta May 31, 2018, 5:29 a.m.
Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>
---
 src/main.c |  2 ++
 src/reg.c  | 37 +++++++++++++++++++++++++++++++++++++
 src/reg.h  |  1 +
 3 files changed, 40 insertions(+)

Comments

Alistair Popple June 4, 2018, 6:26 a.m. | #1
On Thursday, 31 May 2018 3:29:08 PM AEST Rashmica Gupta wrote:
> diff --git a/src/reg.c b/src/reg.c
> index 002cfe9..416236b 100644
> --- a/src/reg.c
> +++ b/src/reg.c
> @@ -18,11 +18,13 @@
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> +#include <debug.h>
>  
>  #include <libpdbg.h>
>  
>  #include "main.h"
>  
> +#define REG_XER -4
>  #define REG_MEM -3
>  #define REG_MSR -2
>  #define REG_NIA -1
> @@ -41,6 +43,8 @@ static void print_proc_reg(struct pdbg_target *target, uint64_t reg, uint64_t va
>  		printf("msr: ");
>  	else if (reg == REG_NIA)
>  		printf("nia: ");
> +	else if (reg == REG_XER)
> +		printf("xer: ");
>  	else if (reg > REG_R31)
>  		printf("spr%03" PRIu64 ": ", reg - REG_R31);
>  	else if (reg >= 0 && reg <= 31)
> @@ -62,6 +66,8 @@ static int putprocreg(struct pdbg_target *target, uint32_t index, uint64_t *reg,
>  		rc = ram_putmsr(target, *value);
>  	else if (*reg == REG_NIA)
>  		rc = ram_putnia(target, *value);
> +	else if (*reg == REG_XER)
> +		rc = ram_putxer(target, *value);
>  	else if (*reg > REG_R31)
>  		rc = ram_putspr(target, *reg - REG_R31, *value);
>  	else if (*reg >= 0 && *reg <= 31)
> @@ -81,6 +87,8 @@ static int getprocreg(struct pdbg_target *target, uint32_t index, uint64_t *reg,
>  		rc = ram_getmsr(target, &value);
>  	else if (*reg == REG_NIA)
>  		rc = ram_getnia(target, &value);
> +	else if (*reg == REG_XER)
> +		rc = ram_getxer(target, &value);
>  	else if (*reg > REG_R31)
>  		rc = ram_getspr(target, *reg - REG_R31, &value);
>  	else if (*reg >= 0 && *reg <= 31)

Most of the above exists for like it does for weird historical reasons, so if
you can see ways of refactoring it feel free to!

> @@ -231,3 +239,32 @@ int handle_msr(int optind, int argc, char *argv[])
>  
>  	return for_each_target("thread", getprocreg, &msr, NULL);
>  }
> +
> +int handle_xer(int optind, int argc, char *argv[])
> +{
> +	uint64_t xer = REG_XER;
> +	char *endptr;
> +
> +	if (strcmp(argv[optind], "putxer") == 0) {
> +		uint64_t data;
> +
> +		if (optind + 1 >= argc) {
> +			printf("%s: command '%s' requires data\n", argv[0], argv[optind]);
> +			return -1;
> +		}
> +
> +		errno = 0;
> +		data = strtoull(argv[optind + 1], &endptr, 0);
> +		if (errno || *endptr != '\0') {
> +			printf("%s: command '%s' couldn't parse data '%s'\n",
> +				argv[0], argv[optind], argv[optind + 1]);
> +			return -1;
> +		}
> +
> +		PR_WARNING("We can only set part of the XER register.\n");

Why? And which part of the XER register? It might be nice if we specified to the
user which bits we can get/set to avoid confusion.

Otherwise everything else looks pretty good! Thanks.

- Alistair

> +		return for_each_target("thread", putprocreg, &xer, &data);
> +	}
> +
> +	PR_WARNING("We can only get part of the XER register.\n");
> +	return for_each_target("thread", getprocreg, &xer, NULL);
> +}
> diff --git a/src/reg.h b/src/reg.h
> index ad41d9d..ca548a6 100644
> --- a/src/reg.h
> +++ b/src/reg.h
> @@ -18,3 +18,4 @@ int handle_gpr(int optind, int argc, char *argv[]);
>  int handle_nia(int optind, int argc, char *argv[]);
>  int handle_spr(int optind, int argc, char *argv[]);
>  int handle_msr(int optind, int argc, char *argv[]);
> +int handle_xer(int optind, int argc, char *argv[]);
>
rashmica June 5, 2018, 12:45 a.m. | #2
On 04/06/18 16:26, Alistair Popple wrote:
> On Thursday, 31 May 2018 3:29:08 PM AEST Rashmica Gupta wrote:
>> diff --git a/src/reg.c b/src/reg.c
>> index 002cfe9..416236b 100644
>> --- a/src/reg.c
>> +++ b/src/reg.c
>> @@ -18,11 +18,13 @@
>>  #include <stdio.h>
>>  #include <stdlib.h>
>>  #include <string.h>
>> +#include <debug.h>
>>  
>>  #include <libpdbg.h>
>>  
>>  #include "main.h"
>>  
>> +#define REG_XER -4
>>  #define REG_MEM -3
>>  #define REG_MSR -2
>>  #define REG_NIA -1
>> @@ -41,6 +43,8 @@ static void print_proc_reg(struct pdbg_target *target, uint64_t reg, uint64_t va
>>  		printf("msr: ");
>>  	else if (reg == REG_NIA)
>>  		printf("nia: ");
>> +	else if (reg == REG_XER)
>> +		printf("xer: ");
>>  	else if (reg > REG_R31)
>>  		printf("spr%03" PRIu64 ": ", reg - REG_R31);
>>  	else if (reg >= 0 && reg <= 31)
>> @@ -62,6 +66,8 @@ static int putprocreg(struct pdbg_target *target, uint32_t index, uint64_t *reg,
>>  		rc = ram_putmsr(target, *value);
>>  	else if (*reg == REG_NIA)
>>  		rc = ram_putnia(target, *value);
>> +	else if (*reg == REG_XER)
>> +		rc = ram_putxer(target, *value);
>>  	else if (*reg > REG_R31)
>>  		rc = ram_putspr(target, *reg - REG_R31, *value);
>>  	else if (*reg >= 0 && *reg <= 31)
>> @@ -81,6 +87,8 @@ static int getprocreg(struct pdbg_target *target, uint32_t index, uint64_t *reg,
>>  		rc = ram_getmsr(target, &value);
>>  	else if (*reg == REG_NIA)
>>  		rc = ram_getnia(target, &value);
>> +	else if (*reg == REG_XER)
>> +		rc = ram_getxer(target, &value);
>>  	else if (*reg > REG_R31)
>>  		rc = ram_getspr(target, *reg - REG_R31, &value);
>>  	else if (*reg >= 0 && *reg <= 31)
> Most of the above exists for like it does for weird historical reasons, so if
> you can see ways of refactoring it feel free to!
>
>> @@ -231,3 +239,32 @@ int handle_msr(int optind, int argc, char *argv[])
>>  
>>  	return for_each_target("thread", getprocreg, &msr, NULL);
>>  }
>> +
>> +int handle_xer(int optind, int argc, char *argv[])
>> +{
>> +	uint64_t xer = REG_XER;
>> +	char *endptr;
>> +
>> +	if (strcmp(argv[optind], "putxer") == 0) {
>> +		uint64_t data;
>> +
>> +		if (optind + 1 >= argc) {
>> +			printf("%s: command '%s' requires data\n", argv[0], argv[optind]);
>> +			return -1;
>> +		}
>> +
>> +		errno = 0;
>> +		data = strtoull(argv[optind + 1], &endptr, 0);
>> +		if (errno || *endptr != '\0') {
>> +			printf("%s: command '%s' couldn't parse data '%s'\n",
>> +				argv[0], argv[optind], argv[optind + 1]);
>> +			return -1;
>> +		}
>> +
>> +		PR_WARNING("We can only set part of the XER register.\n");
> Why? And which part of the XER register? It might be nice if we specified to the
> user which bits we can get/set to avoid confusion.

The bits we can set are different for p8 and p9 when in ramming mode.

On p8 machines we can't seem to get or set the whole XER register with
mtxer/mfxer/mtspr etc. We can only access four fields of the register
with mtxerf/mfxerf. However these fields only cover (IBM) bits 32-39 and
41-43, and only the two bits 33 and 34 seem to be set-able. These are
the only two bits across the four fields that are specified in the ISA.

On p9 we can use mtxer or mtspr, however only (IBM) bits 32-34 and 44-63
were writable on the machine I used. These are all the bits specified in
the ISA plus some 'reserved' ones.

Once I put the chip specific get/put xer behaviour into pxchip.c I could
put more specific warnings there. However I'm not sure I'm comfortable
definitively stating that the above bits are the only ones we can set
based off my limited tests and knowledge...

>
> Otherwise everything else looks pretty good! Thanks.
>
> - Alistair
>
>> +		return for_each_target("thread", putprocreg, &xer, &data);
>> +	}
>> +
>> +	PR_WARNING("We can only get part of the XER register.\n");
>> +	return for_each_target("thread", getprocreg, &xer, NULL);
>> +}
>> diff --git a/src/reg.h b/src/reg.h
>> index ad41d9d..ca548a6 100644
>> --- a/src/reg.h
>> +++ b/src/reg.h
>> @@ -18,3 +18,4 @@ int handle_gpr(int optind, int argc, char *argv[]);
>>  int handle_nia(int optind, int argc, char *argv[]);
>>  int handle_spr(int optind, int argc, char *argv[]);
>>  int handle_msr(int optind, int argc, char *argv[]);
>> +int handle_xer(int optind, int argc, char *argv[]);
>>
>

Patch

diff --git a/src/main.c b/src/main.c
index 07c3677..90fb729 100644
--- a/src/main.c
+++ b/src/main.c
@@ -96,6 +96,8 @@  static struct action expert_actions[] = {
 	{ "putspr",  "<spr> <value>", "Write Special Purpose Register (SPR)", &handle_spr },
 	{ "getmsr",  "", "Get Machine State Register (MSR)", &handle_msr },
 	{ "putmsr",  "<value>", "Write Machine State Register (MSR)", &handle_msr },
+	{ "getxer",  "", "Get Fixed Point Exception Register (XER)", &handle_xer },
+	{ "putxer",  "<value>", "Write Fixed Point Exception Register (XER)", &handle_xer },
 	{ "getring", "<addr> <len>", "Read a ring. Length must be correct", &handle_getring },
 	{ "start",   "", "Start thread", &thread_start },
 	{ "step",    "<count>", "Set a thread <count> instructions", &thread_step },
diff --git a/src/reg.c b/src/reg.c
index 002cfe9..416236b 100644
--- a/src/reg.c
+++ b/src/reg.c
@@ -18,11 +18,13 @@ 
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <debug.h>
 
 #include <libpdbg.h>
 
 #include "main.h"
 
+#define REG_XER -4
 #define REG_MEM -3
 #define REG_MSR -2
 #define REG_NIA -1
@@ -41,6 +43,8 @@  static void print_proc_reg(struct pdbg_target *target, uint64_t reg, uint64_t va
 		printf("msr: ");
 	else if (reg == REG_NIA)
 		printf("nia: ");
+	else if (reg == REG_XER)
+		printf("xer: ");
 	else if (reg > REG_R31)
 		printf("spr%03" PRIu64 ": ", reg - REG_R31);
 	else if (reg >= 0 && reg <= 31)
@@ -62,6 +66,8 @@  static int putprocreg(struct pdbg_target *target, uint32_t index, uint64_t *reg,
 		rc = ram_putmsr(target, *value);
 	else if (*reg == REG_NIA)
 		rc = ram_putnia(target, *value);
+	else if (*reg == REG_XER)
+		rc = ram_putxer(target, *value);
 	else if (*reg > REG_R31)
 		rc = ram_putspr(target, *reg - REG_R31, *value);
 	else if (*reg >= 0 && *reg <= 31)
@@ -81,6 +87,8 @@  static int getprocreg(struct pdbg_target *target, uint32_t index, uint64_t *reg,
 		rc = ram_getmsr(target, &value);
 	else if (*reg == REG_NIA)
 		rc = ram_getnia(target, &value);
+	else if (*reg == REG_XER)
+		rc = ram_getxer(target, &value);
 	else if (*reg > REG_R31)
 		rc = ram_getspr(target, *reg - REG_R31, &value);
 	else if (*reg >= 0 && *reg <= 31)
@@ -231,3 +239,32 @@  int handle_msr(int optind, int argc, char *argv[])
 
 	return for_each_target("thread", getprocreg, &msr, NULL);
 }
+
+int handle_xer(int optind, int argc, char *argv[])
+{
+	uint64_t xer = REG_XER;
+	char *endptr;
+
+	if (strcmp(argv[optind], "putxer") == 0) {
+		uint64_t data;
+
+		if (optind + 1 >= argc) {
+			printf("%s: command '%s' requires data\n", argv[0], argv[optind]);
+			return -1;
+		}
+
+		errno = 0;
+		data = strtoull(argv[optind + 1], &endptr, 0);
+		if (errno || *endptr != '\0') {
+			printf("%s: command '%s' couldn't parse data '%s'\n",
+				argv[0], argv[optind], argv[optind + 1]);
+			return -1;
+		}
+
+		PR_WARNING("We can only set part of the XER register.\n");
+		return for_each_target("thread", putprocreg, &xer, &data);
+	}
+
+	PR_WARNING("We can only get part of the XER register.\n");
+	return for_each_target("thread", getprocreg, &xer, NULL);
+}
diff --git a/src/reg.h b/src/reg.h
index ad41d9d..ca548a6 100644
--- a/src/reg.h
+++ b/src/reg.h
@@ -18,3 +18,4 @@  int handle_gpr(int optind, int argc, char *argv[]);
 int handle_nia(int optind, int argc, char *argv[]);
 int handle_spr(int optind, int argc, char *argv[]);
 int handle_msr(int optind, int argc, char *argv[]);
+int handle_xer(int optind, int argc, char *argv[]);