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

[Rashmica]

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

> On Wed, 29 Aug 2018 11:50:41 +1000
> Rashmica Gupta <rashmica.g at gmail.com> wrote:
>
>> From: Alistair Popple <alistair at 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
>