[13/19] pdbg/gdbserver: Add in basic skeleton for a gdbserver on p8

From: Alistair Popple <alistair@popple.id.au>

I have changed a few bits here and there but this patch is largely
authored by Alistair Popple.

Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>
---
 .gitignore           |   1 +
 Makefile.am          |  13 +-
 configure.ac         |   3 +
 libpdbg/libpdbg.h    |   1 +
 libpdbg/operations.h |   3 -
 libpdbg/target.c     |  16 ++
 libpdbg/target.h     |   3 +
 src/gdb_parser.rl    | 143 ++++++++++++++++
 src/main.c           |   6 +-
 src/pdbgproxy.c      | 476 +++++++++++++++++++++++++++++++++++++++++++++++++++
 src/pdbgproxy.h      |  13 ++
 11 files changed, 671 insertions(+), 7 deletions(-)
 create mode 100644 src/gdb_parser.rl
 create mode 100644 src/pdbgproxy.c
 create mode 100644 src/pdbgproxy.h

On Wed, 29 Aug 2018 11:50:41 +1000
Rashmica Gupta <rashmica.g@gmail.com> wrote:

> From: Alistair Popple <alistair@popple.id.au>
> 
> I have changed a few bits here and there but this patch is largely
> authored by Alistair Popple.

So what's missing for P9? enable_attn? Anything else?

> diff --git a/libpdbg/target.c b/libpdbg/target.c
> index 88ae94a..997db3b 100644
> --- a/libpdbg/target.c
> +++ b/libpdbg/target.c
> @@ -391,3 +391,19 @@ void pdbg_target_priv_set(struct pdbg_target *target, void *priv)
>  {
>  	target->priv = priv;
>  }
> +
> +int poll_target(struct pdbg_target *target, uint64_t addr, uint64_t mask, uint64_t *value)
> +{
> +	uint64_t val, val1;
> +	int rc = 0;
> +
> +	do {
> +		pib_read(target, addr, &val);
> +		rc = pib_read(target, addr, &val1);
> +		if (val != val1)
> +			continue;
> +	} while ((val & mask) != *value);
> +
> +	*value = val;
> +	return rc;
> +}

Is this used anywhere?

> +
> +/* 32 registers represented as 16 char hex numbers with null-termination */
> +#define REG_DATA_SIZE (32*16+1)
> +static void get_gprs(uint64_t *stack, void *priv)
> +{
> +	char data[REG_DATA_SIZE] = "";
> +	uint64_t regs[32];
> +	int i;
> +
> +	for (i = 0; i < 32; i++) {
> +		if (ram_getgpr(thread_target, i, &regs[i]))
> +			PR_ERROR("Error reading register %d\n", i);
> +		printf("r%d = 0x%016lx\n", i, regs[i]);
> +		snprintf(data + i*16, 17, "%016lx", __builtin_bswap64(regs[i]));
> +	}
> +
> +	send_response(fd, data);
> +}

It's likely to be a bit faster to batch the ram setup here. If you do a
ram_setup, you can run multiple ram operations then end with a
ram_destroy and it will just do the setup and destroy once.

> +
> +static void get_spr(uint64_t *stack, void *priv)
> +{
> +	char data[REG_DATA_SIZE];
> +	uint64_t value;
> +
> +	switch (stack[0]) {
> +	case 0x40:
> +		/* Get PC/NIA */

It would be nice to have some #defines for these, then you don't even need
the comment.

> +		if (ram_getnia(thread_target, &value))
> +			PR_ERROR("Error reading NIA\n");
> +		snprintf(data, REG_DATA_SIZE, "%016lx", __builtin_bswap64(value));
> +		send_response(fd, data);
> +		break;
> +
> +	case 0x43:
> +		/* Get LR */
> +		if (ram_getspr(thread_target, 8, &value))
> +			PR_ERROR("Error reading LR\n");
> +		snprintf(data, REG_DATA_SIZE, "%016lx", __builtin_bswap64(value));
> +		send_response(fd, data);
> +		break;
> +
> +	default:
> +		send_response(fd, "xxxxxxxxxxxxxxxx");
> +		break;
> +	}
> +}
> +
> +#define MAX_DATA 0x1000
> +
> +/* Returns a real address to use with adu_getmem or -1UL if we
> + * couldn't determine a real address. At the moment we only deal with
> + * kernel linear mapping but in future we could walk that page
> + * tables. */
> +static uint64_t get_addr(uint64_t addr)
> +{
> +	if (GETFIELD(PPC_BITMASK(0, 3), addr) == 0xc)
> +		/* Assume all 0xc... addresses are part of the linux linear map */
> +		addr &= ~PPC_BITMASK(0, 1);
> +	else
> +		addr = -1UL;
> +
> +	return addr;
> +}
> +
> +static void get_mem(uint64_t *stack, void *priv)
> +{
> +	struct pdbg_target *adu;
> +	uint64_t addr, len, linear_map;
> +	int i, err = 0;
> +	uint64_t data[MAX_DATA/sizeof(uint64_t)];
> +	char result[2*MAX_DATA];
> +
> +	/* stack[0] is the address and stack[1] is the length */
> +	addr = stack[0];
> +	len = stack[1];
> +
> +	pdbg_for_each_class_target("adu", adu) {
> +		if (pdbg_target_probe(adu) == PDBG_TARGET_ENABLED)
> +			break;
> +	}
> +
> +	if (adu == NULL) {
> +		PR_ERROR("ADU NOT FOUND\n");
> +		err=3;
> +		goto out;
> +	}
> +
> +	if (len > MAX_DATA) {
> +		printf("Too much memory requested, truncating\n");
> +		len = MAX_DATA;
> +	}
> +
> +	if (!addr) {
> +		err = 2;
> +		goto out;
> +	}
> +
> +	linear_map = get_addr(addr);
> +	if (linear_map != -1UL) {
> +		if (adu_getmem(adu, addr, (uint8_t *) data, len)) {
> +			PR_ERROR("Unable to read memory\n");
> +			err = 1;
> +		}
> +	} else {
> +		/* Virtual address */
> +		for (i = 0; i < len; i += sizeof(uint64_t)) {
> +			if (ram_getmem(thread_target, addr, &data[i/sizeof(uint64_t)])) {
> +				PR_ERROR("Fault reading memory\n");
> +				err = 2;
> +				break;
> +			}
> +		}

Yeah, ram_getmem seems to do horrible, horrible things on P9. If you
look at it wrong it will checkstop and guard out the core.

I'd say we will need to do virtual memory translation by hand, sadly.

> +	printf("put_mem 0x%016lx = 0x%016lx\n", addr, stack[2]);
> +
> +	if (len == 4 && stack[2] == 0x0810827d) {
> +		/* According to linux-ppc-low.c gdb only uses this
> +		 * op-code for sw break points so we replace it with
> +		 * the correct attn opcode which is what we need for
> +		 * breakpoints.
> +		 *
> +		 * TODO: Upstream a patch to gdb so that it uses the
> +		 * right opcode for baremetal debug. */
> +		PR_INFO("Breakpoint opcode detected, replacing with attn\n");
> +		data = attn_opcode;
> +
> +		/* Need to enable the attn instruction in HID0 */
> +		if (thread->enable_attn(thread_target))
> +			goto out;

Do you disable attn on detach or break if it was enabled? How does
this work here, do we save the instruction, replace it with break, and
then restore the instruction after the break?

Do we need to do any kind of icache flushing here? Icache is coherent
on POWER, the flush I think is just needed to synchronize the ifetch
pipeline. That's probably all pretty well shut down while we are
stopped and ramming so that's probably not a problem.

> +	} else
> +		stack[2] = __builtin_bswap64(stack[2]) >> 32;
> +
> +	if (adu_putmem(adu, addr, data, len)) {
> +		PR_ERROR("Unable to write memory\n");
> +		err = 3;
> +	}
> +
> +out:
> +	if (err)
> +		send_response(fd, "E01");
> +	else
> +		send_response(fd, "OK");
> +}
> +
> +static void v_conts(uint64_t *stack, void *priv)
> +{
> +	ram_step_thread(thread_target, 1);
> +	send_response(fd, "S05");
> +}
> +
> +#define VCONT_POLL_DELAY 100000
> +static void v_contc(uint64_t *stack, void *priv)
> +{
> +	ram_start_thread(thread_target);
> +	state = SIGNAL_WAIT;
> +}
> +
> +static void interrupt(uint64_t *stack, void *priv)
> +{
> +	printf("Interrupt\n");
> +	ram_stop_thread(thread_target);
> +	send_response(fd, "S05");

These would be more good #define candidates I think. Also in some cases
you have a newline in the response, and not others. Intentional?

> +static int gdbserver(uint16_t port)
> +{
> +	struct pdbg_target *target;
> +
> +	for_each_class_target("thread", target) {
> +		if (!target_selected(target))
> +			continue;
> +		if (pdbg_target_probe(target) == PDBG_TARGET_ENABLED)
> +			break;
> +	}
> +	assert(!strcmp(target->class, "thread"));
> +	// Temporary until I can get this working a bit smoother on p9
> +	if (strcmp(target->compatible, "ibm,power8-thread")) {
> +		PR_ERROR("GDBSERVER is only tested on POWER8\n");
> +		return -1;
> +	}

I would say since making a lot of improvements to power9 direct
controls, this should just about work out of the box there too.

Thanks,
Nick

On 29/08/18 18:36, Nicholas Piggin wrote:

> On Wed, 29 Aug 2018 11:50:41 +1000
> Rashmica Gupta <rashmica.g@gmail.com> wrote:
>
>> From: Alistair Popple <alistair@popple.id.au>
>>
>> I have changed a few bits here and there but this patch is largely
>> authored by Alistair Popple.
> So what's missing for P9? enable_attn? Anything else?

There's a couple of things that I'm trying to work out regarding P9.

- we can't mtnia from an arbitrary register in ramming mode (can only do it
 from the link register).

- p9 doesn't play happily with breakpoints yet. I have tried clearing
  the icache as you suggested below but that didn't seem to help.

I think there was something else, but it escapes me atm.


>> diff --git a/libpdbg/target.c b/libpdbg/target.c
>> index 88ae94a..997db3b 100644
>> --- a/libpdbg/target.c
>> +++ b/libpdbg/target.c
>> @@ -391,3 +391,19 @@ void pdbg_target_priv_set(struct pdbg_target *target, void *priv)
>>  {
>>  	target->priv = priv;
>>  }
>> +
>> +int poll_target(struct pdbg_target *target, uint64_t addr, uint64_t mask, uint64_t *value)
>> +{
>> +	uint64_t val, val1;
>> +	int rc = 0;
>> +
>> +	do {
>> +		pib_read(target, addr, &val);
>> +		rc = pib_read(target, addr, &val1);
>> +		if (val != val1)
>> +			continue;
>> +	} while ((val & mask) != *value);
>> +
>> +	*value = val;
>> +	return rc;
>> +}
> Is this used anywhere?

Not anymore :) thanks for picking this up.

>> +
>> +/* 32 registers represented as 16 char hex numbers with null-termination */
>> +#define REG_DATA_SIZE (32*16+1)
>> +static void get_gprs(uint64_t *stack, void *priv)
>> +{
>> +	char data[REG_DATA_SIZE] = "";
>> +	uint64_t regs[32];
>> +	int i;
>> +
>> +	for (i = 0; i < 32; i++) {
>> +		if (ram_getgpr(thread_target, i, &regs[i]))
>> +			PR_ERROR("Error reading register %d\n", i);
>> +		printf("r%d = 0x%016lx\n", i, regs[i]);
>> +		snprintf(data + i*16, 17, "%016lx", __builtin_bswap64(regs[i]));
>> +	}
>> +
>> +	send_response(fd, data);
>> +}
> It's likely to be a bit faster to batch the ram setup here. If you do a
> ram_setup, you can run multiple ram operations then end with a
> ram_destroy and it will just do the setup and destroy once.

Good point. Will do this.

>> +
>> +static void get_spr(uint64_t *stack, void *priv)
>> +{
>> +	char data[REG_DATA_SIZE];
>> +	uint64_t value;
>> +
>> +	switch (stack[0]) {
>> +	case 0x40:
>> +		/* Get PC/NIA */
> It would be nice to have some #defines for these, then you don't even need
> the comment.

Touche.

>> +		if (ram_getnia(thread_target, &value))
>> +			PR_ERROR("Error reading NIA\n");
>> +		snprintf(data, REG_DATA_SIZE, "%016lx", __builtin_bswap64(value));
>> +		send_response(fd, data);
>> +		break;
>> +
>> +	case 0x43:
>> +		/* Get LR */
>> +		if (ram_getspr(thread_target, 8, &value))
>> +			PR_ERROR("Error reading LR\n");
>> +		snprintf(data, REG_DATA_SIZE, "%016lx", __builtin_bswap64(value));
>> +		send_response(fd, data);
>> +		break;
>> +
>> +	default:
>> +		send_response(fd, "xxxxxxxxxxxxxxxx");
>> +		break;
>> +	}
>> +}
>> +
>> +#define MAX_DATA 0x1000
>> +
>> +/* Returns a real address to use with adu_getmem or -1UL if we
>> + * couldn't determine a real address. At the moment we only deal with
>> + * kernel linear mapping but in future we could walk that page
>> + * tables. */
>> +static uint64_t get_addr(uint64_t addr)
>> +{
>> +	if (GETFIELD(PPC_BITMASK(0, 3), addr) == 0xc)
>> +		/* Assume all 0xc... addresses are part of the linux linear map */
>> +		addr &= ~PPC_BITMASK(0, 1);
>> +	else
>> +		addr = -1UL;
>> +
>> +	return addr;
>> +}
>> +
>> +static void get_mem(uint64_t *stack, void *priv)
>> +{
>> +	struct pdbg_target *adu;
>> +	uint64_t addr, len, linear_map;
>> +	int i, err = 0;
>> +	uint64_t data[MAX_DATA/sizeof(uint64_t)];
>> +	char result[2*MAX_DATA];
>> +
>> +	/* stack[0] is the address and stack[1] is the length */
>> +	addr = stack[0];
>> +	len = stack[1];
>> +
>> +	pdbg_for_each_class_target("adu", adu) {
>> +		if (pdbg_target_probe(adu) == PDBG_TARGET_ENABLED)
>> +			break;
>> +	}
>> +
>> +	if (adu == NULL) {
>> +		PR_ERROR("ADU NOT FOUND\n");
>> +		err=3;
>> +		goto out;
>> +	}
>> +
>> +	if (len > MAX_DATA) {
>> +		printf("Too much memory requested, truncating\n");
>> +		len = MAX_DATA;
>> +	}
>> +
>> +	if (!addr) {
>> +		err = 2;
>> +		goto out;
>> +	}
>> +
>> +	linear_map = get_addr(addr);
>> +	if (linear_map != -1UL) {
>> +		if (adu_getmem(adu, addr, (uint8_t *) data, len)) {
>> +			PR_ERROR("Unable to read memory\n");
>> +			err = 1;
>> +		}
>> +	} else {
>> +		/* Virtual address */
>> +		for (i = 0; i < len; i += sizeof(uint64_t)) {
>> +			if (ram_getmem(thread_target, addr, &data[i/sizeof(uint64_t)])) {
>> +				PR_ERROR("Fault reading memory\n");
>> +				err = 2;
>> +				break;
>> +			}
>> +		}
> Yeah, ram_getmem seems to do horrible, horrible things on P9. If you
> look at it wrong it will checkstop and guard out the core.
>
> I'd say we will need to do virtual memory translation by hand, sadly.
>
>> +	printf("put_mem 0x%016lx = 0x%016lx\n", addr, stack[2]);
>> +
>> +	if (len == 4 && stack[2] == 0x0810827d) {
>> +		/* According to linux-ppc-low.c gdb only uses this
>> +		 * op-code for sw break points so we replace it with
>> +		 * the correct attn opcode which is what we need for
>> +		 * breakpoints.
>> +		 *
>> +		 * TODO: Upstream a patch to gdb so that it uses the
>> +		 * right opcode for baremetal debug. */
>> +		PR_INFO("Breakpoint opcode detected, replacing with attn\n");
>> +		data = attn_opcode;
>> +
>> +		/* Need to enable the attn instruction in HID0 */
>> +		if (thread->enable_attn(thread_target))
>> +			goto out;
> Do you disable attn on detach or break if it was enabled?

Nope, I probably should do that!

>  How does
> this work here, do we save the instruction, replace it with break, and
> then restore the instruction after the break?

GDB handles this for us. GDB reads and saves the value at the addr. It asks us
to write the attn instruction to the addr, and then tells us to continue execution.
When we detect that we have paused (in the SIGNAL_WAIT case of the poll function)
we send a trap signal to GDB. GDB then asks us to write back the original value
at the address.

>
> Do we need to do any kind of icache flushing here? Icache is coherent
> on POWER, the flush I think is just needed to synchronize the ifetch
> pipeline. That's probably all pretty well shut down while we are
> stopped and ramming so that's probably not a problem.

I wasn't entirely sure if icache flushing was applicable in ramming mode
and it didn't seem to make a difference in behaviour iirc. Will double check
though.

>> +	} else
>> +		stack[2] = __builtin_bswap64(stack[2]) >> 32;
>> +
>> +	if (adu_putmem(adu, addr, data, len)) {
>> +		PR_ERROR("Unable to write memory\n");
>> +		err = 3;
>> +	}
>> +
>> +out:
>> +	if (err)
>> +		send_response(fd, "E01");
>> +	else
>> +		send_response(fd, "OK");
>> +}
>> +
>> +static void v_conts(uint64_t *stack, void *priv)
>> +{
>> +	ram_step_thread(thread_target, 1);
>> +	send_response(fd, "S05");
>> +}
>> +
>> +#define VCONT_POLL_DELAY 100000
>> +static void v_contc(uint64_t *stack, void *priv)
>> +{
>> +	ram_start_thread(thread_target);
>> +	state = SIGNAL_WAIT;
>> +}
>> +
>> +static void interrupt(uint64_t *stack, void *priv)
>> +{
>> +	printf("Interrupt\n");
>> +	ram_stop_thread(thread_target);
>> +	send_response(fd, "S05");
> These would be more good #define candidates I think. Also in some cases
> you have a newline in the response, and not others. Intentional?

I agree and no, not intentional :/

>> +static int gdbserver(uint16_t port)
>> +{
>> +	struct pdbg_target *target;
>> +
>> +	for_each_class_target("thread", target) {
>> +		if (!target_selected(target))
>> +			continue;
>> +		if (pdbg_target_probe(target) == PDBG_TARGET_ENABLED)
>> +			break;
>> +	}
>> +	assert(!strcmp(target->class, "thread"));
>> +	// Temporary until I can get this working a bit smoother on p9
>> +	if (strcmp(target->compatible, "ibm,power8-thread")) {
>> +		PR_ERROR("GDBSERVER is only tested on POWER8\n");
>> +		return -1;
>> +	}
> I would say since making a lot of improvements to power9 direct
> controls, this should just about work out of the box there too.

What are the direct controls?

> Thanks,
> Nick
>

On Thu, 30 Aug 2018 10:18:30 +1000
Rashmica <rashmica.g@gmail.com> wrote:

> On 29/08/18 18:36, Nicholas Piggin wrote:
> 
> > On Wed, 29 Aug 2018 11:50:41 +1000
> > Rashmica Gupta <rashmica.g@gmail.com> wrote:
> >  
> >> From: Alistair Popple <alistair@popple.id.au>
> >>
> >> I have changed a few bits here and there but this patch is largely
> >> authored by Alistair Popple.  
> > So what's missing for P9? enable_attn? Anything else?  
> 
> There's a couple of things that I'm trying to work out regarding P9.
> 
> - we can't mtnia from an arbitrary register in ramming mode (can only do it
>  from the link register).

I had a bit of a hack at this. I'll try to dig it out.

> 
> - p9 doesn't play happily with breakpoints yet. I have tried clearing
>   the icache as you suggested below but that didn't seem to help.

Yeah I didn't *think* icache flush would be an issue but it was worth
a quick try. PC workbook doesn't seem to give any insight. Could make
the break point facility optional until we sort it out?

> 
> I think there was something else, but it escapes me atm.

Okay well that answers my question. You needn't get P9 working
before the P8 support is merged.

> >  How does
> > this work here, do we save the instruction, replace it with break, and
> > then restore the instruction after the break?  
> 
> GDB handles this for us. GDB reads and saves the value at the addr. It asks us
> to write the attn instruction to the addr, and then tells us to continue execution.
> When we detect that we have paused (in the SIGNAL_WAIT case of the poll function)
> we send a trap signal to GDB. GDB then asks us to write back the original value
> at the address.

Okay cool.

> >> +	assert(!strcmp(target->class, "thread"));
> >> +	// Temporary until I can get this working a bit smoother on p9
> >> +	if (strcmp(target->compatible, "ibm,power8-thread")) {
> >> +		PR_ERROR("GDBSERVER is only tested on POWER8\n");
> >> +		return -1;
> >> +	}  
> > I would say since making a lot of improvements to power9 direct
> > controls, this should just about work out of the box there too.  
> 
> What are the direct controls?

Oh just all the stop/start/ram etc scoms.

Thanks,
Nick

On Thu, 30 Aug 2018 10:18:30 +1000
Rashmica <rashmica.g@gmail.com> wrote:

> On 29/08/18 18:36, Nicholas Piggin wrote:
> 
> > On Wed, 29 Aug 2018 11:50:41 +1000
> > Rashmica Gupta <rashmica.g@gmail.com> wrote:
> >  
> >> From: Alistair Popple <alistair@popple.id.au>
> >>
> >> I have changed a few bits here and there but this patch is largely
> >> authored by Alistair Popple.  
> > So what's missing for P9? enable_attn? Anything else?  
> 
> There's a couple of things that I'm trying to work out regarding P9.
> 
> - we can't mtnia from an arbitrary register in ramming mode (can only do it
>  from the link register).

There's a few bits that seem to make P9 work. No guarantee I didn't
break P8 MTNIA because I haven't tested it.

As we can see, the need for target specific .ram_spr function is
increasing. P9 still isn't quite right with all its MTSPRs either.

--

P9 MTNIA and MTMSR fixes

---
 libpdbg/chip.c   | 19 +++++++++++++++----
 libpdbg/p9chip.c |  6 +++---
 src/reg.c        |  2 +-
 3 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/libpdbg/chip.c b/libpdbg/chip.c
index 80a2261..1d41d87 100644
--- a/libpdbg/chip.c
+++ b/libpdbg/chip.c
@@ -145,8 +145,8 @@ int ram_sreset_thread(struct pdbg_target *thread_target)
  * into *results. *results must point to an array the same size as
  * *opcodes. Each entry from *results is put into SCR0 prior to
  * executing an opcode so that it may also be used to pass in
- * data. Note that only register r0 is saved and restored so opcodes
- * must not touch other registers.
+ * data. Note that only registers r0 and r1 are saved and restored so
+ * opcode sequences must preserve other registers.
  */
 static int ram_instructions(struct pdbg_target *thread_target, uint64_t *opcodes,
 			    uint64_t *results, int len, unsigned int lpar)
@@ -242,10 +242,21 @@ int ram_getnia(struct pdbg_target *thread, uint64_t *value)
 	return 0;
 }
 
+/*
+ * P9 must MTNIA from LR, P8 can MTNIA from R0. So we set both LR and R0
+ * to value. LR must be saved and restored.
+ *
+ * This is a hack and should be made much cleaner once we have target
+ * specific putspr commands.
+ */
 int ram_putnia(struct pdbg_target *thread, uint64_t value)
 {
-	uint64_t opcodes[] = {mfspr(0, 277), mtnia(0)};
-	uint64_t results[] = {value, 0};
+	uint64_t opcodes[] = {	mfspr(1, 8),	/* mflr r1 */
+				mfspr(0, 277),	/* value -> r0 */
+				mtspr(8, 0),	/* mtlr r0 */
+				mtnia(0),
+				mtspr(8, 1), };	/* mtlr r1 */
+	uint64_t results[] = {0, value, 0, 0, 0};
 
 	CHECK_ERR(ram_instructions(thread, opcodes, results, ARRAY_SIZE(opcodes), 0));
 	return 0;
diff --git a/libpdbg/p9chip.c b/libpdbg/p9chip.c
index 189d80a..9094c24 100644
--- a/libpdbg/p9chip.c
+++ b/libpdbg/p9chip.c
@@ -291,10 +291,9 @@ static int __p9_ram_instruction(struct thread *thread, uint64_t opcode, uint64_t
 
 	switch(opcode & OPCODE_MASK) {
 	case MTNIA_OPCODE:
+		opcode = 0x4c0000a4;
+		opcode |= 0x001E0000;
 		predecode = 8;
-
-		/* Not currently supported as we can only MTNIA from LR */
-		PR_ERROR("MTNIA is not currently supported\n");
 		break;
 
 	case MFNIA_OPCODE:
@@ -304,6 +303,7 @@ static int __p9_ram_instruction(struct thread *thread, uint64_t opcode, uint64_t
 
 	case MTMSR_OPCODE:
 		predecode = 8;
+		opcode |= 0x001E0000;
 		break;
 
 	case MFSPR_OPCODE:
diff --git a/src/reg.c b/src/reg.c
index aa77a8a..c63f6da 100644
--- a/src/reg.c
+++ b/src/reg.c
@@ -116,7 +116,7 @@ OPTCMD_DEFINE_CMD(getnia, getnia);
 static int putnia(uint64_t nia)
 {
 	uint64_t reg = REG_NIA;
-	return for_each_target("thread", getprocreg, &reg, &nia);
+	return for_each_target("thread", putprocreg, &reg, &nia);
 }
 OPTCMD_DEFINE_CMD_WITH_ARGS(putnia, putnia, (DATA));

On 30/08/18 21:25, Nicholas Piggin wrote:

> On Thu, 30 Aug 2018 10:18:30 +1000
> Rashmica <rashmica.g@gmail.com> wrote:
>> On 29/08/18 18:36, Nicholas Piggin wrote:
>>> On Wed, 29 Aug 2018 11:50:41 +1000
>>> Rashmica Gupta <rashmica.g@gmail.com> wrote:
>>>> From: Alistair Popple <alistair@popple.id.au>
>>>>
>>>> I have changed a few bits here and there but this patch is largely
>>>> authored by Alistair Popple.  
>>> So what's missing for P9? enable_attn? Anything else?  
>> There's a couple of things that I'm trying to work out regarding P9.
>>
>> - we can't mtnia from an arbitrary register in ramming mode (can only do it
>>  from the link register).
> There's a few bits that seem to make P9 work. No guarantee I didn't
> break P8 MTNIA because I haven't tested it.
>
> As we can see, the need for target specific .ram_spr function is
> increasing. P9 still isn't quite right with all its MTSPRs either.

When I tried doing value -> LR -> NIA like below, the last two bits of NIA on p9
were always set to 0. I'll try on some other machines to see if it happens there.

> --
>
> P9 MTNIA and MTMSR fixes
>
> ---
>  libpdbg/chip.c   | 19 +++++++++++++++----
>  libpdbg/p9chip.c |  6 +++---
>  src/reg.c        |  2 +-
>  3 files changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/libpdbg/chip.c b/libpdbg/chip.c
> index 80a2261..1d41d87 100644
> --- a/libpdbg/chip.c
> +++ b/libpdbg/chip.c
> @@ -145,8 +145,8 @@ int ram_sreset_thread(struct pdbg_target *thread_target)
>   * into *results. *results must point to an array the same size as
>   * *opcodes. Each entry from *results is put into SCR0 prior to
>   * executing an opcode so that it may also be used to pass in
> - * data. Note that only register r0 is saved and restored so opcodes
> - * must not touch other registers.
> + * data. Note that only registers r0 and r1 are saved and restored so
> + * opcode sequences must preserve other registers.
>   */
>  static int ram_instructions(struct pdbg_target *thread_target, uint64_t *opcodes,
>  			    uint64_t *results, int len, unsigned int lpar)
> @@ -242,10 +242,21 @@ int ram_getnia(struct pdbg_target *thread, uint64_t *value)
>  	return 0;
>  }
>  
> +/*
> + * P9 must MTNIA from LR, P8 can MTNIA from R0. So we set both LR and R0
> + * to value. LR must be saved and restored.
> + *
> + * This is a hack and should be made much cleaner once we have target
> + * specific putspr commands.
> + */
>  int ram_putnia(struct pdbg_target *thread, uint64_t value)
>  {
> -	uint64_t opcodes[] = {mfspr(0, 277), mtnia(0)};
> -	uint64_t results[] = {value, 0};
> +	uint64_t opcodes[] = {	mfspr(1, 8),	/* mflr r1 */
> +				mfspr(0, 277),	/* value -> r0 */
> +				mtspr(8, 0),	/* mtlr r0 */
> +				mtnia(0),
> +				mtspr(8, 1), };	/* mtlr r1 */
> +	uint64_t results[] = {0, value, 0, 0, 0};
>  
>  	CHECK_ERR(ram_instructions(thread, opcodes, results, ARRAY_SIZE(opcodes), 0));
>  	return 0;
> diff --git a/libpdbg/p9chip.c b/libpdbg/p9chip.c
> index 189d80a..9094c24 100644
> --- a/libpdbg/p9chip.c
> +++ b/libpdbg/p9chip.c
> @@ -291,10 +291,9 @@ static int __p9_ram_instruction(struct thread *thread, uint64_t opcode, uint64_t
>  
>  	switch(opcode & OPCODE_MASK) {
>  	case MTNIA_OPCODE:
> +		opcode = 0x4c0000a4;
> +		opcode |= 0x001E0000;
>  		predecode = 8;
> -
> -		/* Not currently supported as we can only MTNIA from LR */
> -		PR_ERROR("MTNIA is not currently supported\n");
>  		break;
>  
>  	case MFNIA_OPCODE:
> @@ -304,6 +303,7 @@ static int __p9_ram_instruction(struct thread *thread, uint64_t opcode, uint64_t
>  
>  	case MTMSR_OPCODE:
>  		predecode = 8;
> +		opcode |= 0x001E0000;
>  		break;
>  
>  	case MFSPR_OPCODE:
> diff --git a/src/reg.c b/src/reg.c
> index aa77a8a..c63f6da 100644
> --- a/src/reg.c
> +++ b/src/reg.c
> @@ -116,7 +116,7 @@ OPTCMD_DEFINE_CMD(getnia, getnia);
>  static int putnia(uint64_t nia)
>  {
>  	uint64_t reg = REG_NIA;
> -	return for_each_target("thread", getprocreg, &reg, &nia);
> +	return for_each_target("thread", putprocreg, &reg, &nia);
>  }
>  OPTCMD_DEFINE_CMD_WITH_ARGS(putnia, putnia, (DATA));
>  
>

On 04/09/18 16:23, Rashmica wrote:

> On 30/08/18 21:25, Nicholas Piggin wrote:
>
>> On Thu, 30 Aug 2018 10:18:30 +1000
>> Rashmica <rashmica.g@gmail.com> wrote:
>>> On 29/08/18 18:36, Nicholas Piggin wrote:
>>>> On Wed, 29 Aug 2018 11:50:41 +1000
>>>> Rashmica Gupta <rashmica.g@gmail.com> wrote:
>>>>> From: Alistair Popple <alistair@popple.id.au>
>>>>>
>>>>> I have changed a few bits here and there but this patch is largely
>>>>> authored by Alistair Popple.  
>>>> So what's missing for P9? enable_attn? Anything else?  
>>> There's a couple of things that I'm trying to work out regarding P9.
>>>
>>> - we can't mtnia from an arbitrary register in ramming mode (can only do it
>>>  from the link register).
>> There's a few bits that seem to make P9 work. No guarantee I didn't
>> break P8 MTNIA because I haven't tested it.
>>
>> As we can see, the need for target specific .ram_spr function is
>> increasing. P9 still isn't quite right with all its MTSPRs either.
> When I tried doing value -> LR -> NIA like below, the last two bits of NIA on p9
> were always set to 0. I'll try on some other machines to see if it happens there.

derp that's what should happen! Ignore me :)

>> --
>>
>> P9 MTNIA and MTMSR fixes
>>
>> ---
>>  libpdbg/chip.c   | 19 +++++++++++++++----
>>  libpdbg/p9chip.c |  6 +++---
>>  src/reg.c        |  2 +-
>>  3 files changed, 19 insertions(+), 8 deletions(-)
>>
>> diff --git a/libpdbg/chip.c b/libpdbg/chip.c
>> index 80a2261..1d41d87 100644
>> --- a/libpdbg/chip.c
>> +++ b/libpdbg/chip.c
>> @@ -145,8 +145,8 @@ int ram_sreset_thread(struct pdbg_target *thread_target)
>>   * into *results. *results must point to an array the same size as
>>   * *opcodes. Each entry from *results is put into SCR0 prior to
>>   * executing an opcode so that it may also be used to pass in
>> - * data. Note that only register r0 is saved and restored so opcodes
>> - * must not touch other registers.
>> + * data. Note that only registers r0 and r1 are saved and restored so
>> + * opcode sequences must preserve other registers.
>>   */
>>  static int ram_instructions(struct pdbg_target *thread_target, uint64_t *opcodes,
>>  			    uint64_t *results, int len, unsigned int lpar)
>> @@ -242,10 +242,21 @@ int ram_getnia(struct pdbg_target *thread, uint64_t *value)
>>  	return 0;
>>  }
>>  
>> +/*
>> + * P9 must MTNIA from LR, P8 can MTNIA from R0. So we set both LR and R0
>> + * to value. LR must be saved and restored.
>> + *
>> + * This is a hack and should be made much cleaner once we have target
>> + * specific putspr commands.
>> + */
>>  int ram_putnia(struct pdbg_target *thread, uint64_t value)
>>  {
>> -	uint64_t opcodes[] = {mfspr(0, 277), mtnia(0)};
>> -	uint64_t results[] = {value, 0};
>> +	uint64_t opcodes[] = {	mfspr(1, 8),	/* mflr r1 */
>> +				mfspr(0, 277),	/* value -> r0 */
>> +				mtspr(8, 0),	/* mtlr r0 */
>> +				mtnia(0),
>> +				mtspr(8, 1), };	/* mtlr r1 */
>> +	uint64_t results[] = {0, value, 0, 0, 0};
>>  
>>  	CHECK_ERR(ram_instructions(thread, opcodes, results, ARRAY_SIZE(opcodes), 0));
>>  	return 0;
>> diff --git a/libpdbg/p9chip.c b/libpdbg/p9chip.c
>> index 189d80a..9094c24 100644
>> --- a/libpdbg/p9chip.c
>> +++ b/libpdbg/p9chip.c
>> @@ -291,10 +291,9 @@ static int __p9_ram_instruction(struct thread *thread, uint64_t opcode, uint64_t
>>  
>>  	switch(opcode & OPCODE_MASK) {
>>  	case MTNIA_OPCODE:
>> +		opcode = 0x4c0000a4;
>> +		opcode |= 0x001E0000;
>>  		predecode = 8;
>> -
>> -		/* Not currently supported as we can only MTNIA from LR */
>> -		PR_ERROR("MTNIA is not currently supported\n");
>>  		break;
>>  
>>  	case MFNIA_OPCODE:
>> @@ -304,6 +303,7 @@ static int __p9_ram_instruction(struct thread *thread, uint64_t opcode, uint64_t
>>  
>>  	case MTMSR_OPCODE:
>>  		predecode = 8;
>> +		opcode |= 0x001E0000;
>>  		break;
>>  
>>  	case MFSPR_OPCODE:
>> diff --git a/src/reg.c b/src/reg.c
>> index aa77a8a..c63f6da 100644
>> --- a/src/reg.c
>> +++ b/src/reg.c
>> @@ -116,7 +116,7 @@ OPTCMD_DEFINE_CMD(getnia, getnia);
>>  static int putnia(uint64_t nia)
>>  {
>>  	uint64_t reg = REG_NIA;
>> -	return for_each_target("thread", getprocreg, &reg, &nia);
>> +	return for_each_target("thread", putprocreg, &reg, &nia);
>>  }
>>  OPTCMD_DEFINE_CMD_WITH_ARGS(putnia, putnia, (DATA));
>>  
>>
>>